Merge branch '325620-group-to-project-mr-approval-settings' into 'master'

Add group <> project level MR approval settings cascade

See merge request gitlab-org/gitlab!68476

ee/app/policies/ee/project_policy.rb

@@ -32,6 +32,10 @@ module EE
           ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request
       end
 
+      condition(:group_merge_request_approval_settings_enabled) do
+        @subject.feature_available?(:group_merge_request_approval_settings)
+      end
+
       with_scope :global
       condition(:locked_merge_request_author_setting) do
         License.feature_available?(:admin_merge_request_approvers_rules) &&
@@ -387,6 +391,10 @@ module EE
       end
 
       rule { auditor | can?(:developer_access) }.enable :add_project_to_instance_security_dashboard
+
+      rule { (admin | owner) & group_merge_request_approval_settings_enabled }.policy do
+        enable :admin_merge_request_approval_settings
+      end
     end
 
     override :lookup_access_level!

ee/app/services/merge_request_approval_settings/update_service.rb

@@ -5,13 +5,30 @@ module MergeRequestApprovalSettings
     def execute
       return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
 
-      setting = GroupMergeRequestApprovalSetting.find_or_initialize_by_group(container)
-      setting.assign_attributes(params)
+      if container.is_a?(Group)
+        setting = GroupMergeRequestApprovalSetting.find_or_initialize_by_group(container)
+        setting.assign_attributes(params)
 
-      if setting.save
-        ServiceResponse.success(payload: setting)
-      else
-        ServiceResponse.error(message: setting.errors.messages)
+        if setting.save
+          ServiceResponse.success(payload: setting)
+        else
+          ServiceResponse.error(message: setting.errors.messages)
+        end
+      elsif container.is_a?(Project)
+        resolved_params = {
+          merge_requests_author_approval: params[:allow_author_approval],
+          merge_requests_disable_committers_approval: !params[:allow_committer_approval],
+          disable_overriding_approvers_per_merge_request: !params[:allow_overrides_to_approver_list_per_merge_request],
+          reset_approvals_on_push: !params[:retain_approvals_on_push],
+          require_password_to_approve: params[:require_password_to_approve]
+        }
+        result = ::Projects::UpdateService.new(container, current_user, resolved_params).execute
+
+        if result[:status] == :success
+          ServiceResponse.success(payload: container)
+        else
+          ServiceResponse.error(message: container.errors.messages)
+        end
       end
     end
 

ee/lib/api/entities/group_merge_request_approval_setting.rb → ee/lib/api/entities/merge_request_approval_setting.rb

@@ -2,7 +2,7 @@
 
 module API
   module Entities
-    class GroupMergeRequestApprovalSetting < Grape::Entity
+    class MergeRequestApprovalSetting < Grape::Entity
       expose :allow_author_approval
       expose :allow_committer_approval
       expose :allow_overrides_to_approver_list_per_merge_request

ee/lib/api/group_merge_request_approval_settings.rb

-# frozen_string_literal: true
-
-module API
-  class GroupMergeRequestApprovalSettings < ::API::Base
-    feature_category :source_code_management
-
-    before do
-      authenticate!
-      not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_group)
-
-      authorize! :admin_merge_request_approval_settings, user_group
-    end
-
-    params do
-      requires :id, type: String, desc: 'The ID of a group'
-    end
-    resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
-      segment ':id/merge_request_approval_setting' do
-        desc 'Get group merge request approval setting' do
-          detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
-          success ::API::Entities::GroupMergeRequestApprovalSetting
-        end
-        get do
-          setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
-
-          present setting, with: ::API::Entities::GroupMergeRequestApprovalSetting
-        end
-
-        desc 'Update existing merge request approval setting' do
-          detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
-          success ::API::Entities::GroupMergeRequestApprovalSetting
-        end
-        params do
-          optional :allow_author_approval, type: Boolean, desc: 'Allow authors to self-approve merge requests'
-          optional :allow_committer_approval, type: Boolean, desc: 'Allow committers to approve merge requests'
-          optional :allow_overrides_to_approver_list_per_merge_request,
-                   type: Boolean, desc: 'Allow overrides to approver list per merge request'
-          optional :retain_approvals_on_push, type: Boolean, desc: 'Retain approval count on a new push'
-          optional :require_password_to_approve,
-                   type: Boolean, desc: 'Require approver to authenticate before approving'
-
-          at_least_one_of :allow_author_approval,
-                          :allow_committer_approval,
-                          :allow_overrides_to_approver_list_per_merge_request,
-                          :retain_approvals_on_push,
-                          :require_password_to_approve
-        end
-        put do
-          setting_params = declared_params(include_missing: false)
-
-          response = ::MergeRequestApprovalSettings::UpdateService
-            .new(container: user_group, current_user: current_user, params: setting_params).execute
-
-          if response.success?
-            setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
-
-            present setting, with: ::API::Entities::GroupMergeRequestApprovalSetting
-          else
-            render_api_error!(response.message, :bad_request)
-          end
-        end
-      end
-    end
-  end
-end

ee/lib/api/merge_request_approval_settings.rb

+# frozen_string_literal: true
+
+module API
+  class MergeRequestApprovalSettings < ::API::Base
+    feature_category :source_code_management
+
+    before do
+      authenticate!
+    end
+
+    helpers do
+      params :merge_request_approval_settings do
+        optional :allow_author_approval, type: Boolean, desc: 'Allow authors to self-approve merge requests', allow_blank: false
+        optional :allow_committer_approval, type: Boolean, desc: 'Allow committers to approve merge requests', allow_blank: false
+        optional :allow_overrides_to_approver_list_per_merge_request,
+                 type: Boolean, desc: 'Allow overrides to approver list per merge request', allow_blank: false
+        optional :retain_approvals_on_push, type: Boolean, desc: 'Retain approval count on a new push', allow_blank: false
+        optional :require_password_to_approve,
+                 type: Boolean, desc: 'Require approver to authenticate before approving', allow_blank: false
+
+        at_least_one_of :allow_author_approval,
+                        :allow_committer_approval,
+                        :allow_overrides_to_approver_list_per_merge_request,
+                        :retain_approvals_on_push,
+                        :require_password_to_approve
+      end
+    end
+
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
+    resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      before do
+        authorize! :admin_merge_request_approval_settings, user_project
+      end
+      segment ':id/merge_request_approval_setting' do
+        desc 'Get project-level MR approval settings' do
+          detail 'This feature was introduced in 14.3 behind the :group_merge_request_approval_settings_feature_flag'
+          success EE::API::Entities::MergeRequestApprovalSettings
+        end
+        get do
+          not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_project.root_ancestor)
+
+          group = user_project.group.present? ? user_project.root_ancestor : nil
+          setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: user_project).execute
+
+          present setting, with: ::API::Entities::MergeRequestApprovalSetting
+        end
+        desc 'Update existing merge request approval setting' do
+          detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
+          success ::API::Entities::MergeRequestApprovalSetting
+        end
+        params do
+          use :merge_request_approval_settings
+        end
+        put do
+          setting_params = declared_params(include_missing: false)
+
+          response = ::MergeRequestApprovalSettings::UpdateService
+                       .new(container: user_project, current_user: current_user, params: setting_params).execute
+
+          if response.success?
+            group = user_project.group.present? ? user_project.root_ancestor : nil
+
+            setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: user_project).execute
+
+            present setting, with: ::API::Entities::MergeRequestApprovalSetting
+          else
+            render_api_error!(response.message, :bad_request)
+          end
+        end
+      end
+    end
+
+    params do
+      requires :id, type: String, desc: 'The ID of a group'
+    end
+    resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      before do
+        not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_group)
+
+        authorize! :admin_merge_request_approval_settings, user_group
+      end
+      segment ':id/merge_request_approval_setting' do
+        desc 'Get group merge request approval setting' do
+          detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
+          success ::API::Entities::MergeRequestApprovalSetting
+        end
+        get do
+          setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
+
+          present setting, with: ::API::Entities::MergeRequestApprovalSetting
+        end
+
+        desc 'Update existing merge request approval setting' do
+          detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
+          success ::API::Entities::MergeRequestApprovalSetting
+        end
+        params do
+          use :merge_request_approval_settings
+        end
+        put do
+          setting_params = declared_params(include_missing: false)
+
+          response = ::MergeRequestApprovalSettings::UpdateService
+            .new(container: user_group, current_user: current_user, params: setting_params).execute
+
+          if response.success?
+            setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
+
+            present setting, with: ::API::Entities::MergeRequestApprovalSetting
+          else
+            render_api_error!(response.message, :bad_request)
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/compliance_management/merge_request_approval_settings/resolver.rb

@@ -2,8 +2,9 @@
 module ComplianceManagement
   module MergeRequestApprovalSettings
     class Resolver
-      def initialize(group)
+      def initialize(group, project: nil)
         @group = group
+        @project = project
       end
 
       def execute
@@ -17,38 +18,45 @@ module ComplianceManagement
       end
 
       def allow_author_approval
-        instance_value = instance_settings.current_application_settings.prevent_merge_requests_author_approval
+        instance_value = !instance_settings.prevent_merge_requests_author_approval
         group_value = group_settings&.allow_author_approval
+        project_value = @project&.merge_requests_author_approval
 
-        setting(:allow_author_approval, instance_value, group_value, default: true)
+        setting(instance_value, group_value, project_value)
       end
 
       def allow_committer_approval
-        instance_value = instance_settings.prevent_merge_requests_committers_approval
+        instance_value = !instance_settings.prevent_merge_requests_committers_approval
         group_value = group_settings&.allow_committer_approval
+        project_value = @project ? !@project.merge_requests_disable_committers_approval : nil
 
-        setting(:allow_committer_approval, instance_value, group_value, default: true)
+        setting(instance_value, group_value, project_value)
       end
 
       def allow_overrides_to_approver_list_per_merge_request
-        instance_value = instance_settings.disable_overriding_approvers_per_merge_request
+        instance_value = !instance_settings.disable_overriding_approvers_per_merge_request
         group_value = group_settings&.allow_overrides_to_approver_list_per_merge_request
+        project_value = @project ? !@project.disable_overriding_approvers_per_merge_request : nil
 
-        setting(:allow_overrides_to_approver_list_per_merge_request, instance_value, group_value, default: true)
+        setting(instance_value, group_value, project_value)
       end
 
       def retain_approvals_on_push
-        instance_value = nil
         group_value = group_settings&.retain_approvals_on_push
+        project_value = @project ? !@project.reset_approvals_on_push : nil
 
-        setting(:retain_approvals_on_push, instance_value, group_value, default: true)
+        setting(nil, group_value, project_value)
       end
 
       def require_password_to_approve
-        instance_value = nil
         group_value = group_settings&.require_password_to_approve
+        project_value = @project&.require_password_to_approve
 
-        setting(:require_password_to_approve, instance_value, group_value, default: false)
+        ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
+          value: require_password_value(group_value, project_value),
+          locked: require_password_locked?(group_value, false),
+          inherited_from: (:group if require_password_locked?(group_value, false))
+        )
       end
 
       private
@@ -61,16 +69,39 @@ module ComplianceManagement
         @group&.group_merge_request_approval_setting
       end
 
-      def setting(name, instance_value, group_value, default:)
-        # Setting value should be the instance-level, if set.
-        # Otherwise group-level.
-        # Finally, fallback on to the default defined here.
-        value = instance_value ? !instance_value : group_value
+      def value(instance_value, group_value, project_value)
+        [instance_value, group_value, project_value].compact.all?
+      end
+
+      def require_password_value(group_value, project_value)
+        [group_value, project_value].any?
+      end
+
+      def locked?(instance_value, group_value, project_value)
+        _, *inherited = [project_value, group_value, instance_value].compact
+
+        inherited.any? { |value| value == false }
+      end
+
+      def require_password_locked?(group_value, default)
+        return false if @project.nil?
+        return false if @group.nil?
+
+        group_value || default
+      end
+
+      def inherited_from(instance_value, group_value, project_value)
+        return :instance if instance_value == false
+        return :group if group_value == false && !project_value.nil?
+
+        nil
+      end
 
+      def setting(instance_value, group_value, project_value)
         ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
-          value: value.nil? ? default : value,
-          locked: instance_value.nil? ? false : instance_value,
-          inherited_from: instance_value ? :instance : nil
+          value: value(instance_value, group_value, project_value),
+          locked: locked?(instance_value, group_value, project_value),
+          inherited_from: inherited_from(instance_value, group_value, project_value)
         )
       end
     end

ee/lib/ee/api/api.rb

@@ -30,7 +30,7 @@ module EE
         mount ::API::GroupPushRule
         mount ::API::MergeTrains
         mount ::API::GroupHooks
-        mount ::API::GroupMergeRequestApprovalSettings
+        mount ::API::MergeRequestApprovalSettings
         mount ::API::Scim
         mount ::API::ManagedLicenses
         mount ::API::ProjectApprovals

ee/spec/lib/api/entities/group_merge_request_approval_setting_spec.rb → ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe API::Entities::GroupMergeRequestApprovalSetting do
+RSpec.describe API::Entities::MergeRequestApprovalSetting do
   let(:group_merge_request_approval_setting) { build(:group_merge_request_approval_setting) }
 
   subject { described_class.new(group_merge_request_approval_setting).as_json }

ee/spec/policies/project_policy_spec.rb

@@ -1404,6 +1404,35 @@ RSpec.describe ProjectPolicy do
     end
   end
 
+  describe ':admin_merge_request_approval_settings' do
+    let(:project) { private_project }
+
+    using RSpec::Parameterized::TableSyntax
+
+    where(:role, :licensed, :allowed) do
+      :guest      | true  | false
+      :reporter   | true  | false
+      :developer  | true  | false
+      :maintainer | false | false
+      :maintainer | true  | false
+      :owner      | false | false
+      :owner      | true  | true
+      :admin      | true  | true
+      :admin      | false | false
+    end
+
+    with_them do
+      let(:current_user) { public_send(role) }
+
+      before do
+        stub_licensed_features(group_merge_request_approval_settings: licensed)
+        enable_admin_mode!(current_user) if role == :admin
+      end
+
+      it { is_expected.to(allowed ? be_allowed(:admin_merge_request_approval_settings) : be_disallowed(:admin_merge_request_approval_settings)) }
+    end
+  end
+
   describe ':modify_approvers_rules' do
     it_behaves_like 'merge request approval settings' do
       let(:app_setting) { :disable_overriding_approvers_per_merge_request }

ee/spec/requests/api/group_merge_request_approval_settings_spec.rb → ee/spec/requests/api/merge_request_approval_settings_spec.rb

@@ -2,13 +2,12 @@
 
 require 'spec_helper'
 
-RSpec.describe API::GroupMergeRequestApprovalSettings do
+RSpec.describe API::MergeRequestApprovalSettings do
   let_it_be_with_reload(:group) { create(:group) }
   let_it_be(:user) { create(:user) }
+  let_it_be_with_reload(:project) { create(:project, group: group) }
   let_it_be(:setting) { create(:group_merge_request_approval_setting, group: group) }
 
-  let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
-
   shared_examples "resolvable" do
     using RSpec::Parameterized::TableSyntax
 
@@ -44,6 +43,8 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
   end
 
   describe 'GET /groups/:id/merge_request_approval_settings' do
+    let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
+
     context 'when feature flag is disabled' do
       before do
         stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
@@ -131,6 +132,7 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
   end
 
   describe 'PUT /groups/:id/merge_request_approval_setting' do
+    let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
     let(:params) { { allow_author_approval: true } }
 
     context 'when feature flag is disabled' do
@@ -172,13 +174,13 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
         end
 
         context 'when update fails' do
-          let(:params) { { allow_author_approval: nil } }
+          let(:params) { { allow_author_approval: 45 } }
 
           it 'returns 400 status', :aggregate_failures do
             put api(url, user), params: params
 
             expect(response).to have_gitlab_http_status(:bad_request)
-            expect(json_response['message']).to eq('allow_author_approval' => ['must be a boolean value'])
+            expect(json_response['error']).to match(/allow_.*_approval is invalid/)
           end
         end
 
@@ -209,4 +211,99 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
       end
     end
   end
+
+  describe 'GET /projects/:id/merge_request_approval_settings' do
+    let(:url) { "/projects/#{project.id}/merge_request_approval_setting" }
+
+    context 'when feature flag is disabled' do
+      before do
+        stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
+      end
+
+      it 'returns 404 status' do
+        get api(url, user)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when feature flag is enabled' do
+      before do
+        group.add_owner(user)
+        allow(Ability).to receive(:allowed?).and_call_original
+        stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
+        stub_licensed_features(group_merge_request_approval_settings: true)
+        allow(Ability).to receive(:allowed?)
+                            .with(user, :admin_merge_request_approval_settings, project)
+                            .and_return(true)
+      end
+
+      it 'matches the response schema' do
+        get api(url, user)
+
+        expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee')
+      end
+
+      context 'when the project does not have existing settings' do
+        before do
+          project.update!(
+            merge_requests_author_approval: nil,
+            merge_requests_disable_committers_approval: nil,
+            disable_overriding_approvers_per_merge_request: nil,
+            reset_approvals_on_push: nil,
+            require_password_to_approve: nil
+          )
+        end
+
+        it 'returns in-memory default settings', :aggregate_failures do
+          get api(url, user)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['allow_author_approval']['value']).to eq(false)
+          expect(json_response['allow_committer_approval']['value']).to eq(false)
+          expect(json_response['allow_overrides_to_approver_list_per_merge_request']['value']).to eq(false)
+          expect(json_response['retain_approvals_on_push']['value']).to eq(false)
+          expect(json_response['require_password_to_approve']['value']).to eq(false)
+        end
+      end
+    end
+  end
+
+  describe 'PUT /projects/:id/merge_request_approval_settings' do
+    let(:url) { "/projects/#{project.id}/merge_request_approval_setting" }
+
+    context 'when feature flag is disabled' do
+      before do
+        stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
+      end
+
+      it 'returns 404 status' do
+        put api(url, user)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when feature flag is enabled' do
+      let(:params) { { retain_approvals_on_push: false } }
+
+      before do
+        group.add_owner(user)
+        allow(Ability).to receive(:allowed?).and_call_original
+        stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
+        stub_licensed_features(group_merge_request_approval_settings: true)
+        allow(Ability).to receive(:allowed?)
+                            .with(user, :admin_merge_request_approval_settings, project)
+                            .and_return(true)
+      end
+
+      it 'matches the response schema and updates the params' do
+        put api(url, user), params: params
+
+        expect(project.reset_approvals_on_push).to be true
+
+        expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee')
+      end
+    end
+  end
 end

ee/spec/services/merge_request_approval_settings/update_service_spec.rb

@@ -3,20 +3,58 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequestApprovalSettings::UpdateService do
-  let_it_be_with_reload(:group) { create(:group) }
+  let!(:group) { create(:group) }
+  let!(:project) { create(:project, merge_requests_author_approval: true) }
   let_it_be(:user) { create(:user) }
 
-  let(:params) { { allow_author_approval: true } }
+  let(:params) { { allow_author_approval: false } }
 
   subject(:service) do
     described_class.new(
-      container: group,
+      container: container,
       current_user: user,
       params: params
     )
   end
 
-  describe 'execute' do
+  describe 'execute with a Project as container' do
+    let(:container) { project }
+
+    context 'user does not have permissions' do
+      before do
+        allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, container).and_return(false)
+      end
+
+      it 'responds with an error response', :aggregate_failures do
+        response = subject.execute
+
+        expect(response.status).to eq(:error)
+        expect(response.message).to eq('Insufficient permissions')
+      end
+
+      it 'does not change any of the approval settings' do
+        expect { subject.execute }.not_to change { project.attributes }
+      end
+    end
+
+    context 'user has permissions' do
+      before do
+        allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, container).and_return(true)
+      end
+
+      it 'responds with a successful service response', :aggregate_failures do
+        response = subject.execute
+
+        expect(response).to be_success
+        expect(response.payload.reload.merge_requests_author_approval).to be(false)
+        expect(project.reload.merge_requests_author_approval).to be(false)
+      end
+    end
+  end
+
+  describe 'execute with a Group as container' do
+    let(:container) { group }
+
     context 'user does not have permissions' do
       before do
         allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, group).and_return(false)
@@ -45,7 +83,7 @@ RSpec.describe MergeRequestApprovalSettings::UpdateService do
         response = subject.execute
 
         expect(response).to be_success
-        expect(response.payload.allow_author_approval).to be(true)
+        expect(response.payload.allow_author_approval).to be(false)
       end
 
       context 'when group has an existing setting' do
@@ -53,16 +91,14 @@ RSpec.describe MergeRequestApprovalSettings::UpdateService do
         let_it_be(:existing_setting) { create(:group_merge_request_approval_setting, group: group) }
 
         it 'does not create a new setting' do
-          expect { subject.execute }
-            .to change { GroupMergeRequestApprovalSetting.count }.by(0)
-            .and change { existing_setting.reload.allow_author_approval }.to(true)
+          expect { subject.execute }.not_to change { GroupMergeRequestApprovalSetting.count }
         end
 
         it 'responds with a successful service response', :aggregate_failures do
           response = subject.execute
 
           expect(response).to be_success
-          expect(response.payload.allow_author_approval).to be(true)
+          expect(response.payload.allow_author_approval).to be(false)
         end
       end