Merge branch 'fix-approver-permission-checks' into 'master'

Don't allow removing MR approvers when project setting is set

Closes #2807

See merge request !2395

app/controllers/projects/approver_groups_controller.rb

 class Projects::ApproverGroupsController < Projects::ApplicationController
+  before_action :authorize_for_subject!
+
   def destroy
+    subject.approver_groups.find(params[:id]).destroy
+
+    redirect_back_or_default(default: { action: 'index' })
+  end
+
+  private
+
+  def authorize_for_subject!
+    access_denied! unless can?(current_user, :update_approvers, subject)
+  end
+
+  def subject
+    @subject ||=
       if params[:merge_request_id]
-      authorize_create_merge_request!
-      merge_request = project.merge_requests.find_by!(iid: params[:merge_request_id])
-      merge_request.approver_groups.find(params[:id]).destroy
+        project.merge_requests.find_by!(iid: params[:merge_request_id])
       else
-      authorize_admin_project!
-      project.approver_groups.find(params[:id]).destroy
+        project
       end
-
-    redirect_back_or_default(default: { action: 'index' })
   end
 end

app/controllers/projects/approvers_controller.rb

 class Projects::ApproversController < Projects::ApplicationController
+  before_action :authorize_for_subject!
+
   def destroy
+    subject.approvers.find(params[:id]).destroy
+
+    redirect_back_or_default(default: { action: 'index' })
+  end
+
+  private
+
+  def authorize_for_subject!
+    access_denied! unless can?(current_user, :update_approvers, subject)
+  end
+
+  def subject
+    @subject ||=
       if params[:merge_request_id]
-      authorize_create_merge_request!
-      merge_request = project.merge_requests.find_by!(iid: params[:merge_request_id])
-      merge_request.approvers.find(params[:id]).destroy
+        project.merge_requests.find_by!(iid: params[:merge_request_id])
       else
-      authorize_admin_project!
-      project.approvers.find(params[:id]).destroy
+        project
       end
-
-    redirect_back_or_default(default: { action: 'index' })
   end
 end

app/policies/ee/merge_request_policy.rb

+module EE
+  module MergeRequestPolicy
+    extend ActiveSupport::Concern
+
+    prepended do
+      with_scope :subject
+      condition(:can_override_approvers, score: 0) do
+        @subject.target_project&.can_override_approvers?
+      end
+
+      rule { ~can_override_approvers }.prevent :update_approvers
+      rule { can?(:update_merge_request) }.enable :update_approvers
+    end
+  end
+end

app/policies/ee/project_policy.rb

@@ -49,6 +49,7 @@ module EE
       rule { can?(:master_access) }.policy do
         enable :push_code_to_protected_branches
         enable :admin_path_locks
+        enable :update_approvers
       end
 
       rule { auditor }.policy do

app/policies/merge_request_policy.rb

 class MergeRequestPolicy < IssuablePolicy
+  prepend EE::MergeRequestPolicy
+
   # pass
 end

app/services/ee/merge_requests/base_service.rb

+module EE
+  module MergeRequests
+    module BaseService
+      private
+
+      def filter_params(merge_request)
+        unless current_user.can?(:update_approvers, merge_request)
+          params.delete(:approvals_before_merge)
+          params.delete(:approver_ids)
+          params.delete(:approver_group_ids)
+        end
+
+        super
+      end
+    end
+  end
+end

app/services/merge_requests/base_service.rb

 module MergeRequests
   class BaseService < ::IssuableBaseService
+    prepend EE::MergeRequests::BaseService
+
     def create_note(merge_request)
       SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
     end

app/services/merge_requests/create_service.rb

@@ -7,15 +7,8 @@ module MergeRequests
       source_project = @project
       @project = Project.find(params[:target_project_id]) if params[:target_project_id]
 
-      params[:target_project_id] ||= source_project.id
-
-      unless @project.can_override_approvers?
-        params.delete(:approvals_before_merge)
-        params.delete(:approver_ids)
-        params.delete(:approver_group_ids)
-      end
-
       merge_request = MergeRequest.new
+      merge_request.target_project = @project
       merge_request.source_project = source_project
       merge_request.source_branch = params[:source_branch]
       merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)

app/services/merge_requests/update_service.rb

@@ -17,12 +17,6 @@ module MergeRequests
         merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
       end
 
-      unless project.can_override_approvers?
-        params.delete(:approvals_before_merge)
-        params.delete(:approver_ids)
-        params.delete(:approver_group_ids)
-      end
-
       old_approvers = merge_request.overall_approvers.to_a
 
       handle_wip_event(merge_request)

app/views/shared/issuable/_approvals.html.haml

@@ -4,13 +4,13 @@
 - return unless issuable.is_a?(MergeRequest)
 - return unless issuable.requires_approve?
 
-- can_override_approvers = issuable.target_project.can_override_approvers?
+- can_update_approvers = can?(current_user, :update_approvers, issuable)
 
 .form-group
   = form.label :approver_ids, class: 'control-label' do
     Approvers
   .col-sm-10
-    - if can_override_approvers
+    - if can_update_approvers
       = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups, project: issuable.target_project)
       .help-block
         This merge request must be approved by these users.
@@ -34,7 +34,7 @@
           - issuable.overall_approvers.each do |approver|
             %li{ id: dom_id(approver.user), class: item_classes + ['approver'] }
               = link_to approver.user.name, approver.user
-              - if can_override_approvers
+              - if can_update_approvers
                 .pull-right
                   - if unsaved_approvers
                     = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
@@ -48,7 +48,7 @@
             %li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] }
               Group:
               = link_to approver_group.group.name, approver_group.group
-              - if can_override_approvers
+              - if can_update_approvers
                 .pull-right
                   - if unsaved_approvers
                     = link_to "#", data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, class: "btn-xs btn btn-remove", title: 'Remove group' do
@@ -63,9 +63,9 @@
       .form-group
         = form.label :approvals_before_merge, class: 'label-light' do
           Approvals required
-        = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_override_approvers
+        = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_update_approvers
 
-    - if can_override_approvers
+    - if can_update_approvers
       .help-block.suggested-approvers
         - if @suggested_approvers.any?
           Suggested approvers:

spec/controllers/projects/approver_groups_controller_spec.rb

@@ -2,65 +2,105 @@ require 'rails_helper'
 
 describe Projects::ApproverGroupsController do
   describe '#destroy' do
+    let(:user) { create(:user) }
+    let(:project) { create(:empty_project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+
     before do
       # Allow redirect_back_or_default to work
       request.env['HTTP_REFERER'] = '/'
+
+      project.add_guest(user)
+      sign_in(user)
     end
 
     context 'on a merge request' do
-      it 'authorizes create_merge_request' do
-        merge    = create(:merge_request)
-        project  = stub_project(merge.target_project)
-        approver = create(:approver, target: merge)
+      let!(:approver_group) { create(:approver_group, target: merge_request) }
 
-        expect(controller).to receive(:authorize_create_merge_request!)
-
-        go_delete(project, merge_request_id: merge.to_param, id: approver.id)
+      def destroy_merge_request_approver_group
+        delete :destroy,
+               namespace_id: project.namespace.to_param,
+               project_id: project.to_param,
+               merge_request_id: merge_request.to_param,
+               id: approver_group.id
       end
 
-      it 'destroys the provided approver group' do
-        merge          = create(:merge_request)
-        project        = stub_project(merge.target_project)
-        approver_group = create(:approver_group, target: merge)
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_merge_request_approver_group
 
-        allow(controller).to receive(:authorize_create_merge_request!)
+          expect(response).to have_http_status(404)
+        end
 
-        expect { go_delete(project, merge_request_id: merge.to_param, id: approver_group.id) }
-          .to change { merge.reload.approver_groups.count }.by(-1)
+        it 'does not destroy any approver groups' do
+          expect { destroy_merge_request_approver_group }
+            .not_to change { merge_request.reload.approver_groups.count }
         end
       end
 
-    context 'on a project' do
-      it 'authorizes admin_project' do
-        project        = stub_project
-        approver_group = create(:approver_group, target: project)
+      context 'when the user cannot update approvers because of the project setting' do
+        before do
+          project.add_developer(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
 
-        expect(controller).to receive(:authorize_admin_project!)
+        it 'returns a 404' do
+          destroy_merge_request_approver_group
 
-        go_delete(project, id: approver_group.id)
+          expect(response).to have_http_status(404)
         end
 
-      it 'destroys the provided approver' do
-        project        = stub_project
-        approver_group = create(:approver_group, target: project)
+        it 'does not destroy any approver groups' do
+          expect { destroy_merge_request_approver_group }
+            .not_to change { merge_request.reload.approver_groups.count }
+        end
+      end
 
-        allow(controller).to receive(:authorize_admin_project!).and_return(true)
+      context 'when the user can update approvers' do
+        before do
+          project.add_developer(user)
+        end
 
-        expect { go_delete(project, id: approver_group.id) }
-          .to change { project.approver_groups.count }.by(-1)
+        it 'destroys the provided approver group' do
+          expect { destroy_merge_request_approver_group }
+            .to change { merge_request.reload.approver_groups.count }.by(-1)
         end
       end
+    end
+
+    context 'on a project' do
+      let!(:approver_group) { create(:approver_group, target: project) }
 
-    def go_delete(project, params = {})
-      delete :destroy, {
+      def destroy_project_approver_group
+        delete :destroy,
                namespace_id: project.namespace.to_param,
-        project_id: project.to_param
-      }.merge(params)
+               project_id: project.to_param,
+               id: approver_group.id
+      end
+
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_project_approver_group
+
+          expect(response).to have_http_status(404)
+        end
+
+        it 'does not destroy any approver groups' do
+          expect { destroy_project_approver_group }
+            .not_to change { merge_request.reload.approver_groups.count }
+        end
       end
 
-    def stub_project(project = build_stubbed(:empty_project))
-      project.tap do |p|
-        allow(controller).to receive(:project).and_return(p)
+      context 'when the user can update approvers' do
+        before do
+          project.add_master(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
+
+        it 'destroys the provided approver' do
+          expect { destroy_project_approver_group }
+            .to change { project.reload.approver_groups.count }.by(-1)
+        end
       end
     end
   end

spec/controllers/projects/approvers_controller_spec.rb

@@ -2,65 +2,105 @@ require 'rails_helper'
 
 describe Projects::ApproversController do
   describe '#destroy' do
+    let(:user) { create(:user) }
+    let(:project) { create(:empty_project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+
     before do
       # Allow redirect_back_or_default to work
       request.env['HTTP_REFERER'] = '/'
+
+      project.add_guest(user)
+      sign_in(user)
     end
 
     context 'on a merge request' do
-      it 'authorizes create_merge_request' do
-        merge    = create(:merge_request)
-        project  = stub_project(merge.target_project)
-        approver = create(:approver, target: merge)
+      let!(:approver) { create(:approver, target: merge_request) }
 
-        expect(controller).to receive(:authorize_create_merge_request!)
-
-        go_delete(project, merge_request_id: merge.to_param, id: approver.id)
+      def destroy_merge_request_approver
+        delete :destroy,
+               namespace_id: project.namespace.to_param,
+               project_id: project.to_param,
+               merge_request_id: merge_request.to_param,
+               id: approver.id
       end
 
-      it 'destroys the provided approver' do
-        merge    = create(:merge_request)
-        project  = stub_project(merge.target_project)
-        approver = create(:approver, target: merge)
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_merge_request_approver
 
-        allow(controller).to receive(:authorize_create_merge_request!)
+          expect(response).to have_http_status(404)
+        end
 
-        expect { go_delete(project, merge_request_id: merge.to_param, id: approver.id) }
-          .to change { merge.reload.approvers.count }.by(-1)
+        it 'does not destroy any approvers' do
+          expect { destroy_merge_request_approver }
+            .not_to change { merge_request.reload.approvers.count }
         end
       end
 
-    context 'on a project' do
-      it 'authorizes admin_project' do
-        project  = stub_project
-        approver = create(:approver, target: project)
+      context 'when the user cannot update approvers because of the project setting' do
+        before do
+          project.add_developer(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
 
-        expect(controller).to receive(:authorize_admin_project!)
+        it 'returns a 404' do
+          destroy_merge_request_approver
 
-        go_delete(project, id: approver.id)
+          expect(response).to have_http_status(404)
         end
 
-      it 'destroys the provided approver' do
-        project  = stub_project
-        approver = create(:approver, target: project)
+        it 'does not destroy any approvers' do
+          expect { destroy_merge_request_approver }
+            .not_to change { merge_request.reload.approvers.count }
+        end
+      end
 
-        allow(controller).to receive(:authorize_admin_project!).and_return(true)
+      context 'when the user can update approvers' do
+        before do
+          project.add_developer(user)
+        end
 
-        expect { go_delete(project, id: approver.id) }
-          .to change { project.approvers.count }.by(-1)
+        it 'destroys the provided approver' do
+          expect { destroy_merge_request_approver }
+            .to change { merge_request.reload.approvers.count }.by(-1)
         end
       end
+    end
+
+    context 'on a project' do
+      let!(:approver) { create(:approver, target: project) }
 
-    def go_delete(project, params = {})
-      delete :destroy, {
+      def destroy_project_approver
+        delete :destroy,
                namespace_id: project.namespace.to_param,
-        project_id: project.to_param
-      }.merge(params)
+               project_id: project.to_param,
+               id: approver.id
+      end
+
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_project_approver
+
+          expect(response).to have_http_status(404)
+        end
+
+        it 'does not destroy any approvers' do
+          expect { destroy_project_approver }
+            .not_to change { merge_request.reload.approvers.count }
+        end
       end
 
-    def stub_project(project = build_stubbed(:empty_project))
-      project.tap do |p|
-        allow(controller).to receive(:project).and_return(p)
+      context 'when the user can update approvers' do
+        before do
+          project.add_master(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
+
+        it 'destroys the provided approver' do
+          expect { destroy_project_approver }
+            .to change { project.reload.approvers.count }.by(-1)
+        end
       end
     end
   end

spec/policies/ee/merge_request_policy_spec.rb

+require 'spec_helper'
+
+describe MergeRequestPolicy, models: true do
+  let(:guest) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:master) { create(:user) }
+
+  let(:fork_guest) { create(:user) }
+  let(:fork_developer) { create(:user) }
+  let(:fork_master) { create(:user) }
+
+  let(:project) { create(:empty_project, :public) }
+  let(:fork_project) { create(:empty_project, :public, forked_from_project: project) }
+
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+  let(:fork_merge_request) { create(:merge_request, author: fork_developer, source_project: fork_project, target_project: project) }
+
+  before do
+    project.add_guest(guest)
+    project.add_developer(developer)
+    project.add_master(master)
+
+    fork_project.add_guest(fork_guest)
+    fork_project.add_developer(fork_guest)
+    fork_project.add_master(fork_master)
+  end
+
+  context 'for a merge request within the same project' do
+    def policy_for(user)
+      described_class.new(user, merge_request)
+    end
+
+    context 'when overwriting approvers is disabled on the project' do
+      before do
+        project.update!(disable_overriding_approvers_per_merge_request: true)
+      end
+
+      it 'does not allow anyone to update approvers' do
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(master)).to be_disallowed(:update_approvers)
+
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+
+    context 'when overwriting approvers is enabled on the project' do
+      it 'allows only project developers and above to update the approvers' do
+        expect(policy_for(developer)).to be_allowed(:update_approvers)
+        expect(policy_for(master)).to be_allowed(:update_approvers)
+
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+  end
+
+  context 'for a merge request from a fork' do
+    def policy_for(user)
+      described_class.new(user, fork_merge_request)
+    end
+
+    context 'when overwriting approvers is disabled on the target project' do
+      before do
+        project.update!(disable_overriding_approvers_per_merge_request: true)
+      end
+
+      it 'does not allow anyone to update approvers' do
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(master)).to be_disallowed(:update_approvers)
+
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+
+    context 'when overwriting approvers is disabled on the source project' do
+      before do
+        fork_project.update!(disable_overriding_approvers_per_merge_request: true)
+      end
+
+      it 'has no effect - project developers and above, as well as the author, can update the approvers' do
+        expect(policy_for(developer)).to be_allowed(:update_approvers)
+        expect(policy_for(master)).to be_allowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_allowed(:update_approvers)
+
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+
+    context 'when overwriting approvers is enabled on the target project' do
+      it 'allows project developers and above, as well as the author, to update the approvers' do
+        expect(policy_for(developer)).to be_allowed(:update_approvers)
+        expect(policy_for(master)).to be_allowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_allowed(:update_approvers)
+
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+  end
+end

spec/views/shared/issuable/_approvals.html.haml_spec.rb

 require 'spec_helper'
 
-describe 'shared/issuable/_approvals.html.haml' do
+describe 'shared/issuable/_approvals.html.haml', :view do
+  let(:user) { create(:user) }
   let(:project) { build(:empty_project) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:form) { double('form') }
 
   before do
+    allow(view).to receive(:can?).and_return(true)
+    allow(view).to receive(:current_user).and_return(user)
     allow(form).to receive(:label)
     allow(form).to receive(:number_field)
     allow(merge_request).to receive(:requires_approve?).and_return(true)
@@ -22,7 +25,6 @@ describe 'shared/issuable/_approvals.html.haml' do
 
     context 'can override approvers' do
       before do
-        allow(project).to receive(:can_override_approvers?).and_return(true)
         render 'shared/issuable/approvals', form: form, issuable: merge_request
       end
 
@@ -41,7 +43,7 @@ describe 'shared/issuable/_approvals.html.haml' do
 
     context 'can not override approvers' do
       before do
-        allow(project).to receive(:can_override_approvers?).and_return(false)
+        allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
         render 'shared/issuable/approvals', form: form, issuable: merge_request
       end
 
@@ -86,7 +88,7 @@ describe 'shared/issuable/_approvals.html.haml' do
 
     context 'can not override approvers' do
       it 'hides remove button' do
-        allow(project).to receive(:can_override_approvers?).and_return(false)
+        allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
 
         render 'shared/issuable/approvals', form: form, issuable: merge_request