Commit 28e707fc authored by Sean McGivern's avatar Sean McGivern

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

When the `disable_overriding_approvers_per_merge_request` setting is set, we
shouldn't let people remove approvers or groups from merge requests, but should
let them do so from the project.
parent 8b8e169d
class Projects::ApproverGroupsController < Projects::ApplicationController
before_action :authorize_for_subject!
def destroy
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
else
authorize_admin_project!
project.approver_groups.find(params[:id]).destroy
end
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]
project.merge_requests.find_by!(iid: params[:merge_request_id])
else
project
end
end
end
class Projects::ApproversController < Projects::ApplicationController
before_action :authorize_for_subject!
def destroy
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
else
authorize_admin_project!
project.approvers.find(params[:id]).destroy
end
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]
project.merge_requests.find_by!(iid: params[:merge_request_id])
else
project
end
end
end
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
......@@ -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
......
class MergeRequestPolicy < IssuablePolicy
prepend EE::MergeRequestPolicy
# pass
end
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
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
......
......@@ -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)
......
......@@ -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)
......
......@@ -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:
......
......@@ -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) }
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
context 'when the user cannot update approvers because they do not have access' do
it 'returns a 404' do
destroy_merge_request_approver_group
expect(controller).to receive(:authorize_create_merge_request!)
expect(response).to have_http_status(404)
end
go_delete(project, merge_request_id: merge.to_param, id: approver.id)
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
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 of the project setting' do
before do
project.add_developer(user)
project.update!(disable_overriding_approvers_per_merge_request: true)
end
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 'when the user can update approvers' do
before do
project.add_developer(user)
end
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
it 'authorizes admin_project' do
project = stub_project
approver_group = create(:approver_group, target: project)
let!(:approver_group) { create(:approver_group, target: project) }
expect(controller).to receive(:authorize_admin_project!)
go_delete(project, id: approver_group.id)
def destroy_project_approver_group
delete :destroy,
namespace_id: project.namespace.to_param,
project_id: project.to_param,
id: approver_group.id
end
it 'destroys the provided approver' do
project = stub_project
approver_group = create(:approver_group, target: project)
context 'when the user cannot update approvers because they do not have access' do
it 'returns a 404' do
destroy_project_approver_group
allow(controller).to receive(:authorize_admin_project!).and_return(true)
expect(response).to have_http_status(404)
end
expect { go_delete(project, id: approver_group.id) }
.to change { project.approver_groups.count }.by(-1)
it 'does not destroy any approver groups' do
expect { destroy_project_approver_group }
.not_to change { merge_request.reload.approver_groups.count }
end
end
end
def go_delete(project, params = {})
delete :destroy, {
namespace_id: project.namespace.to_param,
project_id: project.to_param
}.merge(params)
end
context 'when the user can update approvers' do
before do
project.add_master(user)
project.update!(disable_overriding_approvers_per_merge_request: true)
end
def stub_project(project = build_stubbed(:empty_project))
project.tap do |p|
allow(controller).to receive(:project).and_return(p)
it 'destroys the provided approver' do
expect { destroy_project_approver_group }
.to change { project.reload.approver_groups.count }.by(-1)
end
end
end
end
......
......@@ -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) }
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
context 'when the user cannot update approvers because they do not have access' do
it 'returns a 404' do
destroy_merge_request_approver
expect(controller).to receive(:authorize_create_merge_request!)
expect(response).to have_http_status(404)
end
go_delete(project, merge_request_id: merge.to_param, id: approver.id)
it 'does not destroy any approvers' do
expect { destroy_merge_request_approver }
.not_to change { merge_request.reload.approvers.count }
end
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 of the project setting' do
before do
project.add_developer(user)
project.update!(disable_overriding_approvers_per_merge_request: true)
end
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 'when the user can update approvers' do
before do
project.add_developer(user)
end
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
it 'authorizes admin_project' do
project = stub_project
approver = create(:approver, target: project)
let!(:approver) { create(:approver, target: project) }
expect(controller).to receive(:authorize_admin_project!)
go_delete(project, id: approver.id)
def destroy_project_approver
delete :destroy,
namespace_id: project.namespace.to_param,
project_id: project.to_param,
id: approver.id
end
it 'destroys the provided approver' do
project = stub_project
approver = create(:approver, target: project)
context 'when the user cannot update approvers because they do not have access' do
it 'returns a 404' do
destroy_project_approver
allow(controller).to receive(:authorize_admin_project!).and_return(true)
expect(response).to have_http_status(404)
end
expect { go_delete(project, id: approver.id) }
.to change { project.approvers.count }.by(-1)
it 'does not destroy any approvers' do
expect { destroy_project_approver }
.not_to change { merge_request.reload.approvers.count }
end
end
end
def go_delete(project, params = {})
delete :destroy, {
namespace_id: project.namespace.to_param,
project_id: project.to_param
}.merge(params)
end
context 'when the user can update approvers' do
before do
project.add_master(user)
project.update!(disable_overriding_approvers_per_merge_request: true)
end
def stub_project(project = build_stubbed(:empty_project))
project.tap do |p|
allow(controller).to receive(:project).and_return(p)
it 'destroys the provided approver' do
expect { destroy_project_approver }
.to change { project.reload.approvers.count }.by(-1)
end
end
end
end
......
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
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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment