Merge branch 'kerrizor/code_owner_checks_in_commits_create_service' into 'master'

Move codeowner validation checks into Commits::CreateService

See merge request gitlab-org/gitlab!34471

ee/app/controllers/ee/projects/blob_controller.rb

@@ -12,6 +12,7 @@ module EE
       private
 
       def validate_codeowner_rules
+        return unless ::Feature.enabled?(:use_legacy_codeowner_validations)
         return if params[:file_path].blank?
 
         codeowners_error = codeowners_check_error(project, branch_name, params[:file_path])

ee/app/services/ee/commits/create_service.rb

@@ -11,6 +11,7 @@ module EE
       def validate!
         super
 
+        validate_against_codeowner_rules!
         validate_repository_size!
       end
 
@@ -21,6 +22,56 @@ module EE
           raise_error(size_checker.error_message.commit_error)
         end
       end
+
+      def validate_against_codeowner_rules!
+        return if ::Feature.enabled?(:use_legacy_codeowner_validations)
+
+        codeowners_error = check_against_codeowners(project, params[:branch_name], extracted_paths)
+
+        if codeowners_error.present?
+          raise_error(codeowners_error.tr("\n", " "))
+        end
+      end
+
+      def check_against_codeowners(project, branch, paths)
+        return if paths.empty?
+
+        ::Gitlab::CodeOwners::Validator.new(project, branch, paths).execute
+      end
+
+      def extracted_paths
+        paths = []
+
+        paths << params[:file_path].presence
+        paths << paths_from_actions
+        paths << paths_from_start_sha
+        paths << paths_from_commit(params[:commit])
+
+        paths.flatten.compact.uniq
+      end
+
+      def paths_from_actions
+        return unless params[:actions].present?
+
+        params[:actions].flat_map do |entry|
+          [entry[:file_path], entry[:previous_path]]
+        end
+      end
+
+      def paths_from_start_sha
+        return unless params[:start_sha].present?
+
+        commit = project.commit(params[:start_sha])
+        return unless commit
+
+        paths_from_commit(commit)
+      end
+
+      def paths_from_commit(commit)
+        return unless commit.present?
+
+        commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }
+      end
     end
   end
 end

ee/lib/ee/gitlab/checks/diff_check.rb

@@ -31,6 +31,8 @@ module EE
         # Issue to remove this feature flag:
         # https://gitlab.com/gitlab-org/gitlab/-/issues/217427
         def skip_web_ui_code_owner_validations?
+          return true unless ::Feature.enabled?(:use_legacy_codeowner_validations)
+
           ::Feature.enabled?(:skip_web_ui_code_owner_validations, project)
         end
 

ee/spec/controllers/ee/projects/blob_controller_spec.rb

@@ -8,9 +8,30 @@ RSpec.describe Projects::BlobController do
   let(:project) { create(:project, :public, :repository) }
 
   shared_examples "file matches a codeowners rule" do
+    # Expected behavior for both these contexts should be equivalent, however
+    #   the feature flag is used in code to control which code path is followed.
+    #
+    context ":use_legacy_codeowner_validations is true" do
+      before do
+        stub_feature_flags(use_legacy_codeowner_validations: true)
+      end
+
+      it_behaves_like "renders to the expected_view with an error msg"
+    end
+
+    context ":use_legacy_codeowner_validations is false" do
+      before do
+        stub_feature_flags(use_legacy_codeowner_validations: false)
+      end
+
+      it_behaves_like "renders to the expected_view with an error msg"
+    end
+  end
+
+  shared_examples "renders to the expected_view with an error msg" do
     let(:error_msg) { "Example error msg" }
 
-    it "renders to the edit page with an error msg" do
+    it "renders to the expected_view with an error msg" do
       default_params[:file_path] = "CHANGELOG"
 
       expect_next_instance_of(Gitlab::CodeOwners::Validator) do |validator|

ee/spec/lib/gitlab/checks/diff_check_spec.rb

@@ -125,24 +125,42 @@ RSpec.describe Gitlab::Checks::DiffCheck do
             expect(subject).to receive(:updated_from_web?).and_return(true)
           end
 
-          context "when skip_web_ui_code_owner_validations is disabled" do
+          context "when use_legacy_codeowner_validations is enabled" do
             before do
-              stub_feature_flags(skip_web_ui_code_owner_validations: false)
-              allow(project).to receive(:branch_requires_code_owner_approval?)
-                .once.and_return(true)
+              stub_feature_flags(use_legacy_codeowner_validations: true)
             end
 
-            it "returns an array of Proc(s)" do
-              validations = subject.send(:path_validations)
-
-              expect(validations.any?).to be_truthy
-              expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
+            context "when skip_web_ui_code_owner_validations is disabled" do
+              before do
+                stub_feature_flags(skip_web_ui_code_owner_validations: false)
+                allow(project).to receive(:branch_requires_code_owner_approval?)
+                  .once.and_return(true)
+              end
+
+              it "returns an array of Proc(s)" do
+                validations = subject.send(:path_validations)
+
+                expect(validations.any?).to be_truthy
+                expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
+              end
+
+              context "when skip_web_ui_code_owner_validations is enabled" do
+                before do
+                  stub_feature_flags(skip_web_ui_code_owner_validations: true)
+                  expect(project).not_to receive(:branch_requires_code_owner_approval?)
+                end
+
+                it "returns an empty array" do
+                  expect(subject.send(:path_validations)).to eq([])
+                end
+              end
             end
           end
 
-          context "when skip_web_ui_code_owner_validations is enabled" do
+          context "when use_legacy_codeowner_validations is disabled" do
             before do
-              stub_feature_flags(skip_web_ui_code_owner_validations: true)
+              stub_feature_flags(use_legacy_codeowner_validations: false)
+
               expect(project).not_to receive(:branch_requires_code_owner_approval?)
             end
 

ee/spec/requests/api/commits_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe API::Commits do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, creator: user, path: "my.project") }
+  let(:project_id) { project.id }
+
+  before do
+    project.add_maintainer(user)
+  end
+
+  shared_examples_for "returns a 400 from a codeowners violation" do
+    let(:error_msg) { "CodeOwners error msg" }
+
+    before do
+      allow(ProtectedBranch)
+        .to receive(:branch_requires_code_owner_approval?)
+        .with(project, branch).and_return(code_owner_approval_required)
+    end
+
+    it "creates a new validator with expected parameters" do
+      expect(Gitlab::CodeOwners::Validator)
+        .to receive(:new).with(project, branch, Array(paths)).and_call_original
+
+      subject
+    end
+
+    specify do
+      expect_next_instance_of(Gitlab::CodeOwners::Validator) do |validator|
+        expect(validator).to receive(:execute).and_return(error_msg)
+      end
+
+      subject
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+      expect(json_response["message"]).to include(error_msg)
+    end
+  end
+
+  shared_examples_for "does not create a CodeOwners::Validator object" do
+    specify do
+      expect(Gitlab::CodeOwners::Validator).not_to receive(:new)
+
+      subject
+    end
+  end
+
+  describe "POST /projects/:id/repository/commits" do
+    let!(:url) { "/projects/#{project_id}/repository/commits" }
+
+    subject(:request) { post api(url, user), params: params }
+
+    context "create" do
+      let(:message) { "Created a new file with a very very looooooooooooooooooooooooooooooooooooooooooooooong commit message" }
+      let(:valid_c_params) do
+        {
+          branch: "master",
+          commit_message: message,
+          actions: [
+            {
+              action: "create",
+              file_path: "foo/bar/baz.txt",
+              content: "puts 8"
+            }
+          ]
+        }
+      end
+
+      context "a new file that does not match a codeowners entry" do
+        before do
+          post api(url, user), params: valid_c_params
+        end
+
+        it "creates the commit" do
+          expect(response).to have_gitlab_http_status(:created)
+          expect(json_response['title']).to eq(message)
+          expect(json_response['committer_name']).to eq(user.name)
+          expect(json_response['committer_email']).to eq(user.email)
+        end
+      end
+
+      context "a new file that matches a codeowner entry" do
+        context "when codeowners are required" do
+          let(:code_owner_approval_required) { true }
+          let(:params) { valid_c_params }
+          let(:branch) { valid_c_params[:branch] }
+          let(:paths)  { valid_c_params[:actions].first[:file_path] }
+
+          context ":use_legacy_codeowner_validations is true" do
+            before do
+              stub_feature_flags(use_legacy_codeowner_validations: true)
+            end
+
+            it_behaves_like "does not create a CodeOwners::Validator object"
+          end
+
+          context ":use_legacy_codeowner_validations is false" do
+            before do
+              stub_feature_flags(use_legacy_codeowner_validations: false)
+            end
+
+            it_behaves_like "returns a 400 from a codeowners violation"
+          end
+        end
+      end
+    end
+
+    context "delete" do
+      let(:message) { "Deleted file" }
+      let(:valid_d_params) do
+        {
+          branch: "markdown",
+          commit_message: message,
+          actions: [
+            {
+              action: "delete",
+              file_path: "doc/api/users.md"
+            }
+          ]
+        }
+      end
+
+      context "a deleted file that does not match a codeowner entry" do
+        it "creates the commit" do
+          post api(url, user), params: valid_d_params
+
+          expect(response).to have_gitlab_http_status(:created)
+          expect(json_response['title']).to eq(message)
+        end
+      end
+
+      context "a deleted file that matches a codeowner entry" do
+        let(:code_owner_approval_required) { true }
+        let(:params) { valid_d_params }
+        let(:branch) { valid_d_params[:branch] }
+        let(:paths)  { valid_d_params[:actions].first[:file_path] }
+
+        context ":use_legacy_codeowner_validations is true" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: true)
+          end
+
+          it_behaves_like "does not create a CodeOwners::Validator object"
+        end
+
+        context ":use_legacy_codeowner_validations is false" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: false)
+          end
+
+          it_behaves_like "returns a 400 from a codeowners violation"
+        end
+      end
+    end
+
+    describe "move" do
+      let(:message) { "Moved file" }
+      let(:valid_m_params) do
+        {
+          branch: "feature",
+          commit_message: message,
+          actions: [
+            {
+              action: "move",
+              file_path: "VERSION.txt",
+              previous_path: "VERSION",
+              content: "6.7.0.pre"
+            }
+          ]
+        }
+      end
+
+      context "a deleted file that does not match a codeowner entry" do
+        it "creates the commit" do
+          post api(url, user), params: valid_m_params
+
+          expect(response).to have_gitlab_http_status(:created)
+          expect(json_response['title']).to eq(message)
+        end
+      end
+
+      context "a moved file that matches a codeowner entry" do
+        let(:code_owner_approval_required) { true }
+        let(:params) { valid_m_params }
+        let(:branch) { valid_m_params[:branch] }
+        let(:paths) do
+          action = valid_m_params[:actions].first
+          [action[:file_path], action[:previous_path]]
+        end
+
+        context ":use_legacy_codeowner_validations is true" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: true)
+          end
+
+          it_behaves_like "does not create a CodeOwners::Validator object"
+        end
+
+        context ":use_legacy_codeowner_validations is false" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: false)
+          end
+
+          it_behaves_like "returns a 400 from a codeowners violation"
+        end
+      end
+    end
+  end
+
+  describe "POST :id/repository/commits/:sha/cherry_pick" do
+    let(:commit)    { project.commit("7d3b0f7cff5f37573aea97cebfd5692ea1689924") }
+    let(:commit_id) { commit.id }
+    let(:branch)    { "master" }
+    let(:route)     { "/projects/#{project_id}/repository/commits/#{commit_id}/cherry_pick" }
+
+    subject(:request) { post api(route, user), params: { branch: branch } }
+
+    context "no file in the cherry-picked commit matches a codeowner entry" do
+      it "cherry-picks the ref commit" do
+        post api(route, user), params: { branch: branch }
+
+        expect(response).to have_gitlab_http_status(:created)
+        expect(response).to match_response_schema("public_api/v4/commit/basic")
+        expect(json_response["title"]).to eq(commit.title)
+        expect(json_response["message"]).to eq(commit.cherry_pick_message(user))
+        expect(json_response["author_name"]).to eq(commit.author_name)
+        expect(json_response["committer_name"]).to eq(user.name)
+      end
+    end
+
+    context "a file in the cherry-picked commit matches a codeowner entry" do
+      context "when codeowners are required" do
+        let(:code_owner_approval_required) { true }
+        let(:paths) { commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }.uniq }
+
+        context ":use_legacy_codeowner_validations is true" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: true)
+          end
+
+          it_behaves_like "does not create a CodeOwners::Validator object"
+        end
+
+        context ":use_legacy_codeowner_validations is false" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: false)
+          end
+
+          it_behaves_like "returns a 400 from a codeowners violation"
+        end
+      end
+    end
+  end
+
+  describe "POST :id/repository/commits/:sha/revert" do
+    let_it_be(:project) { create(:project, :repository, creator: user, path: "my.project") }
+
+    let(:commit_id) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
+    let(:commit)    { project.commit(commit_id) }
+    let(:branch)    { 'master' }
+    let(:route)     { "/projects/#{project_id}/repository/commits/#{commit_id}/revert" }
+
+    subject(:request) { post api(route, user), params: { branch: branch } }
+
+    context "no file in the revert commit matches a codeowner entry" do
+      it "reverts the ref commit" do
+        post api(route, user), params: { branch: branch }
+
+        expect(response).to have_gitlab_http_status(:created)
+        expect(response).to match_response_schema('public_api/v4/commit/basic')
+
+        expect(json_response['message']).to eq(commit.revert_message(user))
+        expect(json_response['author_name']).to eq(user.name)
+        expect(json_response['committer_name']).to eq(user.name)
+        expect(json_response['parent_ids']).to contain_exactly(commit_id)
+      end
+    end
+
+    context "a file in the revert commit matches a codeowner entry" do
+      context "when codeowners are required" do
+        let(:code_owner_approval_required) { true }
+        let(:paths) { commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }.uniq }
+
+        context ":use_legacy_codeowner_validations is true" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: true)
+          end
+
+          it_behaves_like "does not create a CodeOwners::Validator object"
+        end
+
+        context ":use_legacy_codeowner_validations is false" do
+          before do
+            stub_feature_flags(use_legacy_codeowner_validations: false)
+          end
+          it_behaves_like "returns a 400 from a codeowners violation"
+        end
+      end
+    end
+  end
+end