ProtectedBranchPolicy used from Controller for destroy/update

7 files changed, 97 insertions, 4 deletions

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?
+
+      protected_branch.save
+      protected_branch
+    end
+
+    def authorized?
+      can?(current_user, :create_protected_branch, protected_branch)
+    end
+
+    private
 
-      project.protected_branches.create(params)
+    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