Merge branch 'jej/add-protected-branch-policy' into 'master'

Add protected branch policy

See merge request gitlab-org/gitlab-ce!17982

16 files changed, 281 insertions, 22 deletions

diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index d1719f12072..64954ac9a42 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -5,12 +5,8 @@ class Projects::ProtectedBranchesController < Projects::ProtectedRefsController
     @project.repository.branches
   end
 
-  def create_service_class
-    ::ProtectedBranches::CreateService
-  end
-
-  def update_service_class
-    ::ProtectedBranches::UpdateService
+  def service_namespace
+    ::ProtectedBranches
   end
 
   def load_protected_ref
diff --git a/app/controllers/projects/protected_refs_controller.rb b/app/controllers/projects/protected_refs_controller.rb
index b51bdf7aa78..9e757a8d25f 100644
--- a/app/controllers/projects/protected_refs_controller.rb
+++ b/app/controllers/projects/protected_refs_controller.rb
@@ -37,7 +37,7 @@ class Projects::ProtectedRefsController < Projects::ApplicationController
   end
 
   def destroy
-    @protected_ref.destroy
+    destroy_service_class.new(@project, current_user).execute(@protected_ref)
 
     respond_to do |format|
       format.html { redirect_to_repository_settings(@project) }
@@ -47,6 +47,18 @@ class Projects::ProtectedRefsController < Projects::ApplicationController
 
   protected
 
+  def create_service_class
+    service_namespace::CreateService
+  end
+
+  def update_service_class
+    service_namespace::UpdateService
+  end
+
+  def destroy_service_class
+    service_namespace::DestroyService
+  end
+
   def access_level_attributes
     %i(access_level id)
   end
diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb
index a5dbd7e46ae..198c938ff35 100644
--- a/app/controllers/projects/protected_tags_controller.rb
+++ b/app/controllers/projects/protected_tags_controller.rb
@@ -5,12 +5,8 @@ class Projects::ProtectedTagsController < Projects::ProtectedRefsController
     @project.repository.tags
   end
 
-  def create_service_class
-    ::ProtectedTags::CreateService
-  end
-
-  def update_service_class
-    ::ProtectedTags::UpdateService
+  def service_namespace
+    ::ProtectedTags
   end
 
   def load_protected_ref
diff --git a/app/policies/protected_branch_policy.rb b/app/policies/protected_branch_policy.rb
new file mode 100644
index 00000000000..1a7faa4db40
--- /dev/null
+++ b/app/policies/protected_branch_policy.rb
@@ -0,0 +1,9 @@
+class ProtectedBranchPolicy < BasePolicy
+  delegate { @subject.project }
+
+  rule { can?(:admin_project) }.policy do
+    enable :create_protected_branch
+    enable :update_protected_branch
+    enable :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
new file mode 100644
index 00000000000..8172c896e76
--- /dev/null
+++ b/app/services/protected_branches/destroy_service.rb
@@ -0,0 +1,9 @@
+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
+end
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index 4b3337a5c9d..95e46645374 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -1,7 +1,7 @@
 module ProtectedBranches
   class UpdateService < BaseService
     def execute(protected_branch)
-      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_protected_branch, protected_branch)
 
       protected_branch.update(params)
       protected_branch
diff --git a/app/services/protected_tags/destroy_service.rb b/app/services/protected_tags/destroy_service.rb
new file mode 100644
index 00000000000..c868d7ad8e6
--- /dev/null
+++ b/app/services/protected_tags/destroy_service.rb
@@ -0,0 +1,7 @@
+module ProtectedTags
+  class DestroyService < BaseService
+    def execute(protected_tag)
+      protected_tag.destroy
+    end
+  end
+end
diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb
index 33321db46e9..aa7cab4a741 100644
--- a/lib/api/protected_branches.rb
+++ b/lib/api/protected_branches.rb
@@ -70,7 +70,10 @@ module API
       delete ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         protected_branch = user_project.protected_branches.find_by!(name: params[:name])
 
-        destroy_conditionally!(protected_branch)
+        destroy_conditionally!(protected_branch) do
+          destroy_service = ::ProtectedBranches::DestroyService.new(user_project, current_user)
+          destroy_service.execute(protected_branch)
+        end
       end
     end
   end
diff --git a/spec/controllers/projects/protected_branches_controller_spec.rb b/spec/controllers/projects/protected_branches_controller_spec.rb
index 80be135b5d8..096e29bc39f 100644
--- a/spec/controllers/projects/protected_branches_controller_spec.rb
+++ b/spec/controllers/projects/protected_branches_controller_spec.rb
@@ -1,6 +1,16 @@
 require('spec_helper')
 
 describe Projects::ProtectedBranchesController do
+  let(:project) { create(:project, :repository) }
+  let(:protected_branch) { create(:protected_branch, project: project) }
+  let(:project_params) { { namespace_id: project.namespace.to_param, project_id: project } }
+  let(:base_params) { project_params.merge(id: protected_branch.id) }
+  let(:user) { create(:user) }
+
+  before do
+    project.add_master(user)
+  end
+
   describe "GET #index" do
     let(:project) { create(:project_empty_repo, :public) }
 
@@ -8,4 +18,91 @@ describe Projects::ProtectedBranchesController do
       get(:index, namespace_id: project.namespace.to_param, project_id: project)
     end
   end
+
+  describe "POST #create" do
+    let(:master_access_level) { [{ access_level: Gitlab::Access::MASTER }] }
+    let(:access_level_params) do
+      { merge_access_levels_attributes: master_access_level,
+        push_access_levels_attributes: master_access_level }
+    end
+    let(:create_params) { attributes_for(:protected_branch).merge(access_level_params) }
+
+    before do
+      sign_in(user)
+    end
+
+    it 'creates the protected branch rule' do
+      expect 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
+    let(:update_params) { { name: 'new_name' } }
+
+    before do
+      sign_in(user)
+    end
+
+    it 'updates the protected branch rule' do
+      put(:update, base_params.merge(protected_branch: update_params))
+
+      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
+    before do
+      sign_in(user)
+    end
+
+    it "deletes the protected branch rule" do
+      delete(:destroy, base_params)
+
+      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/policies/protected_branch_policy_spec.rb b/spec/policies/protected_branch_policy_spec.rb
new file mode 100644
index 00000000000..b39de42d721
--- /dev/null
+++ b/spec/policies/protected_branch_policy_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe ProtectedBranchPolicy do
+  let(:user) { create(:user) }
+  let(:name) { 'feature' }
+  let(:protected_branch) { create(:protected_branch, name: name) }
+  let(:project) { protected_branch.project }
+
+  subject { described_class.new(user, protected_branch) }
+
+  it 'branches can be updated via project masters' do
+    project.add_master(user)
+
+    is_expected.to be_allowed(:update_protected_branch)
+  end
+
+  it "branches can't be updated by guests" do
+    project.add_guest(user)
+
+    is_expected.to be_disallowed(:update_protected_branch)
+  end
+end
diff --git a/spec/requests/api/protected_branches_spec.rb b/spec/requests/api/protected_branches_spec.rb
index 1d23e023bb6..576fde46615 100644
--- a/spec/requests/api/protected_branches_spec.rb
+++ b/spec/requests/api/protected_branches_spec.rb
@@ -193,6 +193,19 @@ describe API::ProtectedBranches do
           expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
         end
       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
+          post post_endpoint, name: branch_name
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
     end
 
     context 'when authenticated as a guest' do
@@ -209,18 +222,20 @@ describe API::ProtectedBranches do
   end
 
   describe "DELETE /projects/:id/protected_branches/unprotect/:branch" do
+    let(:delete_endpoint) { api("/projects/#{project.id}/protected_branches/#{branch_name}", user) }
+
     before do
       project.add_master(user)
     end
 
     it "unprotects a single branch" do
-      delete api("/projects/#{project.id}/protected_branches/#{branch_name}", user)
+      delete delete_endpoint
 
       expect(response).to have_gitlab_http_status(204)
     end
 
     it_behaves_like '412 response' do
-      let(:request) { api("/projects/#{project.id}/protected_branches/#{branch_name}", user) }
+      let(:request) { delete_endpoint }
     end
 
     it "returns 404 if branch does not exist" do
@@ -229,11 +244,24 @@ describe API::ProtectedBranches do
       expect(response).to have_gitlab_http_status(404)
     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
+        delete delete_endpoint
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
     context 'when branch has a wildcard in its name' do
       let(:protected_name) { 'feature*' }
 
       it "unprotects a wildcard branch" do
-        delete api("/projects/#{project.id}/protected_branches/#{branch_name}", user)
+        delete delete_endpoint
 
         expect(response).to have_gitlab_http_status(204)
       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
new file mode 100644
index 00000000000..4a391b6c25c
--- /dev/null
+++ b/spec/services/protected_branches/destroy_service_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe ProtectedBranches::DestroyService do
+  let(:protected_branch) { create(:protected_branch) }
+  let(:project) { protected_branch.project }
+  let(:user) { project.owner }
+
+  describe '#execute' do
+    subject(:service) { described_class.new(project, user) }
+
+    it 'destroys a protected branch' do
+      service.execute(protected_branch)
+
+      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
diff --git a/spec/services/protected_tags/destroy_service_spec.rb b/spec/services/protected_tags/destroy_service_spec.rb
new file mode 100644
index 00000000000..e12f53a2221
--- /dev/null
+++ b/spec/services/protected_tags/destroy_service_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe ProtectedTags::DestroyService do
+  let(:protected_tag) { create(:protected_tag) }
+  let(:project) { protected_tag.project }
+  let(:user) { project.owner }
+
+  describe '#execute' do
+    subject(:service) { described_class.new(project, user) }
+
+    it 'destroy a protected tag' do
+      service.execute(protected_tag)
+
+      expect(protected_tag).to be_destroyed
+    end
+  end
+end