Merge branch 'security-372-fix-approvers-override' into 'master'

Don't override approval rules if not allowed

See merge request gitlab/gitlab-ee!1055

ee/app/services/approval_rules/params_filtering_service.rb

@@ -21,6 +21,8 @@ module ApprovalRules
     end
 
     def execute
+      params.delete(:approval_rules_attributes) unless current_user.can?(:update_approvers, target)
+
       return params unless params.key?(:approval_rules_attributes)
 
       params[:approval_rules_attributes].each do |rule_attributes|

ee/changelogs/unreleased/security-372-fix-approvers-override.yml

+---
+title: Don't override approval rules if not allowed
+merge_request:
+author:
+type: security

ee/spec/controllers/projects/merge_requests/creations_controller_spec.rb

@@ -11,6 +11,8 @@ describe Projects::MergeRequests::CreationsController do
   end
 
   describe 'POST #create' do
+    let(:created_merge_request) { assigns(:merge_request) }
+
     def create_merge_request(overrides = {})
       params = {
         namespace_id: project.namespace.to_param,
@@ -27,8 +29,6 @@ describe Projects::MergeRequests::CreationsController do
     end
 
     context 'the approvals_before_merge param' do
-      let(:created_merge_request) { assigns(:merge_request) }
-
       before do
         project.update(approvals_before_merge: 2)
       end
@@ -97,5 +97,45 @@ describe Projects::MergeRequests::CreationsController do
         end
       end
     end
+
+    context 'overriding approvers per MR' do
+      let(:new_approver) { create(:user) }
+
+      before do
+        project.add_developer(new_approver)
+        project.update(disable_overriding_approvers_per_merge_request: disable_overriding_approvers_per_merge_request)
+
+        create_merge_request(
+          approval_rules_attributes: [
+            {
+              name: 'Test',
+              user_ids: [new_approver.id],
+              approvals_required: 1
+            }
+          ]
+        )
+      end
+
+      context 'enabled' do
+        let(:disable_overriding_approvers_per_merge_request) { false }
+
+        it 'does create approval rules' do
+          approval_rules = created_merge_request.reload.approval_rules
+
+          expect(approval_rules.count).to eq(1)
+          expect(approval_rules.first.name).to eq('Test')
+          expect(approval_rules.first.user_ids).to eq([new_approver.id])
+          expect(approval_rules.first.approvals_required).to eq(1)
+        end
+      end
+
+      context 'disabled' do
+        let(:disable_overriding_approvers_per_merge_request) { true }
+
+        it 'does not create approval rules' do
+          expect(created_merge_request.reload.approval_rules).to be_empty
+        end
+      end
+    end
   end
 end

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -190,6 +190,20 @@ describe Projects::MergeRequestsController do
 
           expect(merge_request.reload.approver_group_ids).to be_empty
         end
+
+        it 'does not create approval rules' do
+          update_merge_request(
+            approval_rules_attributes: [
+              {
+                name: 'Test',
+                user_ids: [new_approver.id],
+                approvals_required: 1
+              }
+            ]
+          )
+
+          expect(merge_request.reload.approval_rules).to be_empty
+        end
       end
     end
 

ee/spec/services/approval_rules/params_filtering_service_spec.rb

@@ -18,20 +18,39 @@ describe ApprovalRules::ParamsFilteringService do
         project.add_reporter(project_member)
 
         accessible_group.add_developer(user)
+
+        allow(Ability).to receive(:allowed?).and_call_original
+
+        allow(Ability)
+          .to receive(:allowed?)
+          .with(user, :update_approvers, merge_request)
+          .and_return(can_update_approvers?)
       end
 
-      it 'only assigns eligible users and groups' do
-        params = service.execute
+      context 'user can update approvers' do
+        let(:can_update_approvers?) { true }
+
+        it 'only assigns eligible users and groups' do
+          params = service.execute
+
+          rule1 = params[:approval_rules_attributes].first
 
-        rule1 = params[:approval_rules_attributes].first
+          expect(rule1[:user_ids]).to contain_exactly(project_member.id)
 
-        expect(rule1[:user_ids]).to contain_exactly(project_member.id)
+          rule2 = params[:approval_rules_attributes].last
+          expected_group_ids = expected_groups.map(&:id)
 
-        rule2 = params[:approval_rules_attributes].last
-        expected_group_ids = expected_groups.map(&:id)
+          expect(rule2[:user_ids]).to be_empty
+          expect(rule2[:group_ids]).to contain_exactly(*expected_group_ids)
+        end
+      end
 
-        expect(rule2[:user_ids]).to be_empty
-        expect(rule2[:group_ids]).to contain_exactly(*expected_group_ids)
+      context 'user cannot update approvers' do
+        let(:can_update_approvers?) { false }
+
+        it 'deletes the approval_rules_attributes from params' do
+          expect(service.execute).not_to have_key(:approval_rules_attributes)
+        end
       end
     end