diff --git a/app/policies/protected_branch_policy.rb b/app/policies/protected_branch_policy.rb index 8d44cff1b42..29dd897194d 100644 --- a/app/policies/protected_branch_policy.rb +++ b/app/policies/protected_branch_policy.rb @@ -6,10 +6,14 @@ class ProtectedBranchPolicy < BasePolicy end rule { can?(:admin_project) }.policy do + enable :create_protected_branch enable :update_protected_branch + enable :destroy_protected_branch end rule { requires_admin_to_unprotect? & ~admin }.policy do + prevent :create_protected_branch prevent :update_protected_branch + prevent :destroy_protected_branch end end diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb index 6212fd69077..9d947f73af1 100644 --- a/app/services/protected_branches/create_service.rb +++ b/app/services/protected_branches/create_service.rb @@ -1,11 +1,20 @@ module ProtectedBranches class CreateService < BaseService - attr_reader :protected_branch - def execute(skip_authorization: false) - raise Gitlab::Access::AccessDeniedError unless skip_authorization || can?(current_user, :admin_project, project) + raise Gitlab::Access::AccessDeniedError unless skip_authorization || authorized? - project.protected_branches.create(params) + protected_branch.save + protected_branch + end + + def authorized? + can?(current_user, :create_protected_branch, protected_branch) + end + + private + + def protected_branch + @protected_branch ||= project.protected_branches.new(params) end end end diff --git a/app/services/protected_branches/destroy_service.rb b/app/services/protected_branches/destroy_service.rb index 74fdb900c56..8172c896e76 100644 --- a/app/services/protected_branches/destroy_service.rb +++ b/app/services/protected_branches/destroy_service.rb @@ -1,6 +1,8 @@ module ProtectedBranches class DestroyService < BaseService def execute(protected_branch) + raise Gitlab::Access::AccessDeniedError unless can?(current_user, :destroy_protected_branch, protected_branch) + protected_branch.destroy end end diff --git a/spec/controllers/projects/protected_branches_controller_spec.rb b/spec/controllers/projects/protected_branches_controller_spec.rb index d9f6878d6b1..096e29bc39f 100644 --- a/spec/controllers/projects/protected_branches_controller_spec.rb +++ b/spec/controllers/projects/protected_branches_controller_spec.rb @@ -36,6 +36,19 @@ describe Projects::ProtectedBranchesController do post(:create, project_params.merge(protected_branch: create_params)) end.to change(ProtectedBranch, :count).by(1) end + + context 'when a policy restricts rule deletion' do + before do + policy = instance_double(ProtectedBranchPolicy, can?: false) + allow(ProtectedBranchPolicy).to receive(:new).and_return(policy) + end + + it "prevents creation of the protected branch rule" do + post(:create, project_params.merge(protected_branch: create_params)) + + expect(ProtectedBranch.count).to eq 0 + end + end end describe "PUT #update" do @@ -51,6 +64,21 @@ describe Projects::ProtectedBranchesController do expect(protected_branch.reload.name).to eq('new_name') expect(json_response["name"]).to eq('new_name') end + + context 'when a policy restricts rule deletion' do + before do + policy = instance_double(ProtectedBranchPolicy, can?: false) + allow(ProtectedBranchPolicy).to receive(:new).and_return(policy) + end + + it "prevents update of the protected branch rule" do + old_name = protected_branch.name + + put(:update, base_params.merge(protected_branch: update_params)) + + expect(protected_branch.reload.name).to eq(old_name) + end + end end describe "DELETE #destroy" do @@ -63,5 +91,18 @@ describe Projects::ProtectedBranchesController do expect { ProtectedBranch.find(protected_branch.id) }.to raise_error(ActiveRecord::RecordNotFound) end + + context 'when a policy restricts rule deletion' do + before do + policy = instance_double(ProtectedBranchPolicy, can?: false) + allow(ProtectedBranchPolicy).to receive(:new).and_return(policy) + end + + it "prevents deletion of the protected branch rule" do + delete(:destroy, base_params) + + expect(response.status).to eq(403) + end + end end end diff --git a/spec/services/protected_branches/create_service_spec.rb b/spec/services/protected_branches/create_service_spec.rb index 53b3e5e365d..786493c3577 100644 --- a/spec/services/protected_branches/create_service_spec.rb +++ b/spec/services/protected_branches/create_service_spec.rb @@ -35,5 +35,18 @@ describe ProtectedBranches::CreateService do expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError) end end + + context 'when a policy restricts rule creation' do + before do + policy = instance_double(ProtectedBranchPolicy, can?: false) + expect(ProtectedBranchPolicy).to receive(:new).and_return(policy) + end + + it "prevents creation of the protected branch rule" do + expect do + service.execute + end.to raise_error(Gitlab::Access::AccessDeniedError) + end + end end end diff --git a/spec/services/protected_branches/destroy_service_spec.rb b/spec/services/protected_branches/destroy_service_spec.rb index 79eff417943..4a391b6c25c 100644 --- a/spec/services/protected_branches/destroy_service_spec.rb +++ b/spec/services/protected_branches/destroy_service_spec.rb @@ -13,5 +13,18 @@ describe ProtectedBranches::DestroyService do expect(protected_branch).to be_destroyed end + + context 'when a policy restricts rule deletion' do + before do + policy = instance_double(ProtectedBranchPolicy, can?: false) + expect(ProtectedBranchPolicy).to receive(:new).and_return(policy) + end + + it "prevents deletion of the protected branch rule" do + expect do + service.execute(protected_branch) + end.to raise_error(Gitlab::Access::AccessDeniedError) + end + end end end diff --git a/spec/services/protected_branches/update_service_spec.rb b/spec/services/protected_branches/update_service_spec.rb index 9fa5983db66..3f6f8e09565 100644 --- a/spec/services/protected_branches/update_service_spec.rb +++ b/spec/services/protected_branches/update_service_spec.rb @@ -22,5 +22,16 @@ describe ProtectedBranches::UpdateService do expect { service.execute(protected_branch) }.to raise_error(Gitlab::Access::AccessDeniedError) end end + + context 'when a policy restricts rule creation' do + before do + policy = instance_double(ProtectedBranchPolicy, can?: false) + expect(ProtectedBranchPolicy).to receive(:new).and_return(policy) + end + + it "prevents creation of the protected branch rule" do + expect { service.execute(protected_branch) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end end end