Merge branch 'jej/branch-unprotection-restrictions' into 'master'

Branch Unprotection Restrictions using group/user access levels

Closes #4800

See merge request gitlab-org/gitlab-ee!5103

app/models/concerns/protected_ref_access.rb

@@ -4,7 +4,8 @@ module ProtectedRefAccess
   ALLOWED_ACCESS_LEVELS = [
     Gitlab::Access::MASTER,
     Gitlab::Access::DEVELOPER,
-    Gitlab::Access::NO_ACCESS
+    Gitlab::Access::NO_ACCESS,
+    Gitlab::Access::ADMIN
   ].freeze
 
   HUMAN_ACCESS_LEVELS = {

app/models/protected_branch.rb

@@ -2,6 +2,7 @@ class ProtectedBranch < ActiveRecord::Base
   include Gitlab::ShellAdapter
   include ProtectedRef
   prepend EE::ProtectedRef
+  prepend EE::ProtectedBranch
 
   protected_ref_access_levels :merge, :push
 

app/policies/protected_branch_policy.rb

 class ProtectedBranchPolicy < BasePolicy
+  prepend EE::ProtectedBranchPolicy
+
   delegate { @subject.project }
 
   rule { can?(:admin_project) }.policy do

app/services/protected_branches/api_service.rb

@@ -5,13 +5,15 @@ module ProtectedBranches
     def create
       @push_params = AccessLevelParams.new(:push, params)
       @merge_params = AccessLevelParams.new(:merge, params)
+      @unprotect_params = AccessLevelParams.new(:unprotect, params)
 
       verify_params!
 
       protected_branch_params = {
         name: params[:name],
         push_access_levels_attributes: @push_params.access_levels,
-        merge_access_levels_attributes: @merge_params.access_levels
+        merge_access_levels_attributes: @merge_params.access_levels,
+        unprotect_access_levels_attributes: @unprotect_params.access_levels
       }
 
       ::ProtectedBranches::CreateService.new(@project, @current_user, protected_branch_params).execute

db/schema.rb

@@ -2082,6 +2082,17 @@ ActiveRecord::Schema.define(version: 20180405101928) do
   add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
   add_index "protected_branch_push_access_levels", ["user_id"], name: "index_protected_branch_push_access_levels_on_user_id", using: :btree
 
+  create_table "protected_branch_unprotect_access_levels", force: :cascade do |t|
+    t.integer "protected_branch_id", null: false
+    t.integer "access_level", default: 40
+    t.integer "user_id"
+    t.integer "group_id"
+  end
+
+  add_index "protected_branch_unprotect_access_levels", ["group_id"], name: "index_protected_branch_unprotect_access_levels_on_group_id", using: :btree
+  add_index "protected_branch_unprotect_access_levels", ["protected_branch_id"], name: "index_protected_branch_unprotect_access", using: :btree
+  add_index "protected_branch_unprotect_access_levels", ["user_id"], name: "index_protected_branch_unprotect_access_levels_on_user_id", using: :btree
+
   create_table "protected_branches", force: :cascade do |t|
     t.integer "project_id", null: false
     t.string "name", null: false
@@ -2785,6 +2796,9 @@ ActiveRecord::Schema.define(version: 20180405101928) do
   add_foreign_key "protected_branch_push_access_levels", "namespaces", column: "group_id", name: "fk_7111b68cdb", on_delete: :cascade
   add_foreign_key "protected_branch_push_access_levels", "protected_branches", name: "fk_9ffc86a3d9", on_delete: :cascade
   add_foreign_key "protected_branch_push_access_levels", "users"
+  add_foreign_key "protected_branch_unprotect_access_levels", "namespaces", column: "group_id", on_delete: :cascade
+  add_foreign_key "protected_branch_unprotect_access_levels", "protected_branches", on_delete: :cascade
+  add_foreign_key "protected_branch_unprotect_access_levels", "users", on_delete: :cascade
   add_foreign_key "protected_branches", "projects", name: "fk_7a9c6d93e7", on_delete: :cascade
   add_foreign_key "protected_tag_create_access_levels", "namespaces", column: "group_id", name: "fk_b4eb82fe3c", on_delete: :cascade
   add_foreign_key "protected_tag_create_access_levels", "protected_tags", name: "fk_f7dfda8c51", on_delete: :cascade

ee/app/models/concerns/ee/protected_branch.rb

+module EE
+  module ProtectedBranch
+    extend ActiveSupport::Concern
+
+    included do
+      protected_ref_access_levels :unprotect
+    end
+
+    def can_unprotect?(user)
+      return true if unprotect_access_levels.empty?
+
+      unprotect_access_levels.any? do |access_level|
+        access_level.check_access(user)
+      end
+    end
+  end
+end

ee/app/models/ee/user.rb

@@ -29,8 +29,9 @@ module EE
       has_many :approvers,                dependent: :destroy # rubocop: disable Cop/ActiveRecordDependent
 
       # Protected Branch Access
-      has_many :protected_branch_merge_access_levels, dependent: :destroy, class_name: ProtectedBranch::MergeAccessLevel # rubocop:disable Cop/ActiveRecordDependent
-      has_many :protected_branch_push_access_levels, dependent: :destroy, class_name: ProtectedBranch::PushAccessLevel # rubocop:disable Cop/ActiveRecordDependent
+      has_many :protected_branch_merge_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::MergeAccessLevel # rubocop:disable Cop/ActiveRecordDependent
+      has_many :protected_branch_push_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::PushAccessLevel # rubocop:disable Cop/ActiveRecordDependent
+      has_many :protected_branch_unprotect_access_levels, dependent: :destroy, class_name: ::ProtectedBranch::UnprotectAccessLevel # rubocop:disable Cop/ActiveRecordDependent
     end
 
     module ClassMethods

ee/app/models/license.rb

@@ -53,6 +53,7 @@ class License < ActiveRecord::Base
     object_storage
     group_saml
     service_desk
+    unprotection_restrictions
     variable_environment_scope
     reject_unsigned_commits
     commit_committer_check

ee/app/models/protected_branch/unprotect_access_level.rb

+class ProtectedBranch::UnprotectAccessLevel < ActiveRecord::Base
+  include ProtectedBranchAccess
+end

ee/app/policies/ee/protected_branch_policy.rb

+module EE
+  module ProtectedBranchPolicy
+    extend ActiveSupport::Concern
+
+    prepended do
+      condition(:can_unprotect) do
+        @subject.can_unprotect?(@user)
+      end
+
+      condition(:unprotect_restrictions_enabled, scope: :subject) do
+        @subject.project.feature_available?(:unprotection_restrictions)
+      end
+
+      rule { unprotect_restrictions_enabled & ~can_unprotect }.policy do
+        prevent :create_protected_branch # Prevent a user creating a rule they wouldn't be able to update or destroy
+        prevent :update_protected_branch
+        prevent :destroy_protected_branch
+      end
+    end
+  end
+end

ee/app/services/ee/protected_branches/api_service.rb

@@ -26,14 +26,14 @@ module EE
       end
 
       def groups_accessible?
-        group_ids = @merge_params.group_ids + @push_params.group_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        group_ids = @merge_params.group_ids + @push_params.group_ids + @unprotect_params.group_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
         allowed_groups = @project.invited_groups.where(id: group_ids) # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
         group_ids.count == allowed_groups.count
       end
 
       def users_accessible?
-        user_ids = @merge_params.user_ids + @push_params.user_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        user_ids = @merge_params.user_ids + @push_params.user_ids + @unprotect_params.user_ids # rubocop:disable Gitlab/ModuleWithInstanceVariables
         allowed_users = @project.team.users.where(id: user_ids) # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
         user_ids.count == allowed_users.count

ee/db/migrate/20180325034910_create_protected_branch_unprotect_access_levels.rb

+class CreateProtectedBranchUnprotectAccessLevels < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  GITLAB_ACCESS_MASTER = 40
+
+  def change
+    create_table :protected_branch_unprotect_access_levels do |t|
+      t.references :protected_branch, index: { name: "index_protected_branch_unprotect_access" }, foreign_key: { on_delete: :cascade }, null: false
+      t.integer :access_level, default: GITLAB_ACCESS_MASTER, null: true
+      t.references :user, foreign_key:  { on_delete: :cascade }, index: true
+      t.integer :group_id, index: true
+    end
+
+    add_foreign_key :protected_branch_unprotect_access_levels, :namespaces, column: :group_id, on_delete: :cascade # rubocop: disable Migration/AddConcurrentForeignKey
+  end
+end

ee/spec/factories/protected_branches/unprotect_access_levels.rb

+FactoryBot.define do
+  factory :protected_branch_unprotect_access_level, class: ProtectedBranch::UnprotectAccessLevel do
+    user nil
+    group nil
+    protected_branch
+    access_level { Gitlab::Access::DEVELOPER }
+  end
+end

ee/spec/models/ee/protected_branch_spec.rb

+require 'spec_helper'
+
+describe ProtectedBranch do
+  subject { create(:protected_branch) }
+
+  let(:project) { subject.project }
+
+  describe '#can_unprotect?' do
+    let(:user) { create(:user) }
+    let(:admin) { create(:user, :admin) }
+    let(:master) do
+      create(:user).tap { |user| project.add_master(user) }
+    end
+
+    context 'without unprotect_access_levels' do
+      it "doesn't add any additional restriction" do
+        expect(subject.can_unprotect?(user)).to eq true
+      end
+    end
+
+    context 'with access level set to MASTER' do
+      before do
+        subject.unprotect_access_levels.create!(access_level: Gitlab::Access::MASTER)
+      end
+
+      it 'defaults to requiring master access' do
+        expect(subject.can_unprotect?(user)).to eq false
+        expect(subject.can_unprotect?(master)).to eq true
+        expect(subject.can_unprotect?(admin)).to eq true
+      end
+    end
+
+    context 'with access level set to ADMIN' do
+      before do
+        subject.unprotect_access_levels.create!(access_level: Gitlab::Access::ADMIN)
+      end
+
+      it 'prevents access to masters' do
+        expect(subject.can_unprotect?(master)).to eq false
+      end
+
+      it 'grants access to admins' do
+        expect(subject.can_unprotect?(admin)).to eq true
+      end
+    end
+
+    context 'multiple access levels' do
+      before do
+        subject.unprotect_access_levels.create!(user: master)
+        subject.unprotect_access_levels.create!(user: user)
+      end
+
+      it 'grants access if any grant access' do
+        expect(subject.can_unprotect?(user)).to eq true
+      end
+    end
+  end
+end

ee/spec/models/protected_branch/unprotect_access_level_spec.rb

+require 'spec_helper'
+
+describe ProtectedBranch::UnprotectAccessLevel do
+  it { is_expected.to validate_inclusion_of(:access_level).in_array([Gitlab::Access::MASTER, Gitlab::Access::DEVELOPER, Gitlab::Access::NO_ACCESS]) }
+end

ee/spec/policies/protected_branch_policy_spec.rb

+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 }
+  let(:allowed_group) { create(:group) }
+
+  subject { described_class.new(user, protected_branch) }
+
+  before do
+    project.add_master(user)
+  end
+
+  context 'when unprotection is limited by access levels' do
+    before do
+      protected_branch.unprotect_access_levels.create!(group: allowed_group)
+    end
+
+    context 'when unprotection restriction feature is unlicensed' do
+      it "users can remove protections" do
+        is_expected.to be_allowed(:update_protected_branch)
+        is_expected.to be_allowed(:destroy_protected_branch)
+      end
+    end
+
+    context 'when unprotection restriction feature is licensed' do
+      before do
+        stub_licensed_features(unprotection_restrictions: true)
+      end
+
+      it "users can't remove protections without specific access" do
+        is_expected.not_to be_allowed(:update_protected_branch)
+        is_expected.not_to be_allowed(:destroy_protected_branch)
+      end
+
+      context "and access levels grant the user control" do
+        before do
+          allowed_group.add_user(user, :guest)
+        end
+
+        it 'users can manage protections' do
+          is_expected.to be_allowed(:update_protected_branch)
+          is_expected.to be_allowed(:update_protected_branch)
+          is_expected.to be_allowed(:destroy_protected_branch)
+        end
+      end
+    end
+  end
+
+  context 'creating restrictions' do
+    let(:unprotect_access_levels) { [{ group_id: allowed_group.id }] }
+    let(:protected_branch) { build(:protected_branch, name: name, unprotect_access_levels_attributes: unprotect_access_levels) }
+
+    before do
+      stub_licensed_features(unprotection_restrictions: true)
+    end
+
+    it "is prevented if the user wouldn't be able to remove the restriction" do
+      is_expected.not_to be_allowed(:create_protected_branch)
+    end
+
+    context 'when the user can remove the restriction' do
+      before do
+        allowed_group.add_user(user, :guest)
+      end
+
+      it "is allowed" do
+        is_expected.to be_allowed(:create_protected_branch)
+      end
+    end
+  end
+end

lib/api/entities.rb

@@ -394,6 +394,7 @@ module API
       expose :name
       expose :push_access_levels, using: Entities::ProtectedRefAccess
       expose :merge_access_levels, using: Entities::ProtectedRefAccess
+      expose :unprotect_access_levels, using: Entities::ProtectedRefAccess
     end
 
     class Milestone < Grape::Entity

lib/api/protected_branches.rb

@@ -45,6 +45,9 @@ module API
         optional :merge_access_level, type: Integer,
                                       values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS,
                                       desc: 'Access levels allowed to merge (defaults: `40`, master access level)'
+        optional :unprotect_access_level, type: Integer,
+                                          values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS,
+                                          desc: 'Access levels allowed to unprotect (defaults: `40`, master access level)'
         optional :allowed_to_push, type: Array, desc: 'An array of users/groups allowed to push' do
           optional :access_level, type: Integer, values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS
           optional :user_id, type: Integer
@@ -55,6 +58,11 @@ module API
           optional :user_id, type: Integer
           optional :group_id, type: Integer
         end
+        optional :allowed_to_unprotect, type: Array, desc: 'An array of users/groups allowed to unprotect' do
+          optional :access_level, type: Integer, values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS
+          optional :user_id, type: Integer
+          optional :group_id, type: Integer
+        end
       end
       post ':id/protected_branches' do
         protected_branch = user_project.protected_branches.find_by(name: params[:name])

lib/gitlab/access.rb

@@ -15,6 +15,7 @@ module Gitlab
     DEVELOPER = 30
     MASTER    = 40
     OWNER     = 50
+    ADMIN     = 60
 
     # Branch protection settings
     PROTECTION_NONE          = 0

lib/gitlab/import_export/import_export.yml

@@ -61,6 +61,7 @@ project_tree:
   - protected_branches:
     - :merge_access_levels
     - :push_access_levels
+    - :unprotect_access_levels
   - protected_tags:
     - :create_access_levels
   - :project_feature

lib/gitlab/import_export/relation_factory.rb

@@ -11,6 +11,7 @@ module Gitlab
                     hooks: 'ProjectHook',
                     merge_access_levels: 'ProtectedBranch::MergeAccessLevel',
                     push_access_levels: 'ProtectedBranch::PushAccessLevel',
+                    unprotect_access_levels: 'ProtectedBranch::UnprotectAccessLevel',
                     create_access_levels: 'ProtectedTag::CreateAccessLevel',
                     labels: :project_labels,
                     priorities: :label_priorities,

spec/factories/protected_branches.rb

@@ -7,8 +7,10 @@ FactoryBot.define do
       # EE
       authorize_user_to_push nil
       authorize_user_to_merge nil
+      authorize_user_to_unprotect nil
       authorize_group_to_push nil
       authorize_group_to_merge nil
+      authorize_group_to_unprotect nil
 
       default_push_level true
       default_merge_level true
@@ -65,6 +67,10 @@ FactoryBot.define do
         protected_branch.merge_access_levels.new(user: user)
       end
 
+      if user = evaluator.authorize_user_to_unprotect
+        protected_branch.unprotect_access_levels.new(user: user)
+      end
+
       if group = evaluator.authorize_group_to_push
         protected_branch.push_access_levels.new(group: group)
       end
@@ -73,6 +79,10 @@ FactoryBot.define do
         protected_branch.merge_access_levels.new(group: group)
       end
 
+      if group = evaluator.authorize_group_to_unprotect
+        protected_branch.unprotect_access_levels.new(group: group)
+      end
+
       next unless protected_branch.merge_access_levels.empty?
 
       if evaluator.default_access_level && evaluator.default_push_level

spec/lib/gitlab/import_export/all_models.yml

@@ -171,6 +171,7 @@ protected_branches:
 - project
 - merge_access_levels
 - push_access_levels
+- unprotect_access_levels
 protected_tags:
 - project
 - create_access_levels
@@ -182,6 +183,10 @@ push_access_levels:
 - user
 - protected_branch
 - group
+unprotect_access_levels:
+- user
+- protected_branch
+- group
 create_access_levels:
 - user
 - protected_tag

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -509,6 +509,14 @@ ProtectedBranch::PushAccessLevel:
 - updated_at
 - user_id
 - group_id
+ProtectedBranch::UnprotectAccessLevel:
+- id
+- protected_branch_id
+- access_level
+- created_at
+- updated_at
+- user_id
+- group_id
 ProtectedTag::CreateAccessLevel:
 - id
 - protected_tag_id

spec/requests/api/protected_branches_spec.rb

@@ -56,6 +56,7 @@ describe API::ProtectedBranches do
         expect(json_response['name']).to eq(branch_name)
         expect(json_response['push_access_levels'][0]['access_level']).to eq(::Gitlab::Access::MASTER)
         expect(json_response['merge_access_levels'][0]['access_level']).to eq(::Gitlab::Access::MASTER)
+        expect(json_response['unprotect_access_levels']).to eq([])
       end
 
       context 'when protected branch does not exist' do
@@ -70,10 +71,12 @@ describe API::ProtectedBranches do
       context 'with per user/group access levels' do
         let(:push_user) { create(:user) }
         let(:merge_group) { create(:group) }
+        let(:unprotect_group) { create(:group) }
 
         before do
           protected_branch.push_access_levels.create!(user: push_user)
           protected_branch.merge_access_levels.create!(group: merge_group)
+          protected_branch.unprotect_access_levels.create!(group: unprotect_group)
         end
 
         it 'returns access level details' do
@@ -81,10 +84,12 @@ describe API::ProtectedBranches do
 
           push_user_ids = json_response['push_access_levels'].map {|level| level['user_id']}
           merge_group_ids = json_response['merge_access_levels'].map {|level| level['group_id']}
+          unprotect_group_ids = json_response['unprotect_access_levels'].map {|level| level['group_id']}
 
           expect(response).to have_gitlab_http_status(200)
           expect(push_user_ids).to include(push_user.id)
           expect(merge_group_ids).to include(merge_group.id)
+          expect(unprotect_group_ids).to include(unprotect_group.id)
         end
       end
     end
@@ -141,6 +146,7 @@ describe API::ProtectedBranches do
         expect(json_response['name']).to eq(branch_name)
         expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
         expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
+        expect(json_response['unprotect_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
       end
 
       it 'protects a single branch and developers can push' do
@@ -188,6 +194,16 @@ describe API::ProtectedBranches do
         expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::NO_ACCESS)
       end
 
+      it 'protects a single branch and only admins can unprotect' do
+        post post_endpoint, name: branch_name, unprotect_access_level: Gitlab::Access::ADMIN
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['name']).to eq(branch_name)
+        expect(json_response['push_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
+        expect(json_response['merge_access_levels'][0]['access_level']).to eq(Gitlab::Access::MASTER)
+        expect(json_response['unprotect_access_levels'][0]['access_level']).to eq(Gitlab::Access::ADMIN)
+      end
+
       it 'protects a single branch and no one can push or merge' do
         post post_endpoint, name: branch_name, push_access_level: 0, merge_access_level: 0
 
@@ -224,6 +240,15 @@ describe API::ProtectedBranches do
           expect(json_response['merge_access_levels'][0]['user_id']).to eq(merge_user.id)
         end
 
+        it 'can protect a branch while allowing an individual user to unprotect' do
+          unprotect_user = project_member
+
+          post post_endpoint, name: branch_name, allowed_to_unprotect: [{ user_id: unprotect_user.id }]
+
+          expect_protection_to_be_successful
+          expect(json_response['unprotect_access_levels'][0]['user_id']).to eq(unprotect_user.id)
+        end
+
         it 'can protect a branch while allowing a group to push' do
           push_group = invited_group
 
@@ -242,6 +267,15 @@ describe API::ProtectedBranches do
           expect(json_response['merge_access_levels'][0]['group_id']).to eq(merge_group.id)
         end
 
+        it 'can protect a branch while allowing a group to unprotect' do
+          unprotect_group = invited_group
+
+          post post_endpoint, name: branch_name, allowed_to_unprotect: [{ group_id: unprotect_group.id }]
+
+          expect_protection_to_be_successful
+          expect(json_response['unprotect_access_levels'][0]['group_id']).to eq(unprotect_group.id)
+        end
+
         it "fails if users don't all have access to the project" do
           push_user = create(:user)