From 51f3e7c560d09fc4b806f48a7c3bcb773f7b9220 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 9 Jan 2020 17:27:57 +0800
Subject: [PATCH 01/10] Add ApprovalProjectRulesProtectedBranch join table

---
 ...proval_project_rules_protected_branches.rb | 21 +++++++++++++++++++
 db/schema.rb                                  |  9 ++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb

diff --git a/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
new file mode 100644
index 00000000000000..7ab1041cea348a
--- /dev/null
+++ b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class CreateApprovalProjectRulesProtectedBranches < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :approval_project_rules_protected_branches, id: :bigserial do |t|
+      t.references :approval_project_rule,
+                   null: false,
+                   index: false,
+                   foreign_key: { on_delete: :cascade }
+      t.references :protected_branch,
+                   null: false,
+                   index: { name: 'index_approval_project_rules_protected_branches_pb_id' },
+                   foreign_key: { on_delete: :cascade }
+      t.index [:approval_project_rule_id, :protected_branch_id], name: 'index_approval_project_rules_protected_branches_unique', unique: true, using: :btree
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index c2c5bb43c5e744..754a750fbdb36b 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -437,6 +437,13 @@
     t.index ["group_id"], name: "index_approval_project_rules_groups_2"
   end
 
+  create_table "approval_project_rules_protected_branches", force: :cascade do |t|
+    t.bigint "approval_project_rule_id", null: false
+    t.bigint "protected_branch_id", null: false
+    t.index ["approval_project_rule_id", "protected_branch_id"], name: "index_approval_project_rules_protected_branches_unique", unique: true
+    t.index ["protected_branch_id"], name: "index_approval_project_rules_protected_branches_pb_id"
+  end
+
   create_table "approval_project_rules_users", force: :cascade do |t|
     t.bigint "approval_project_rule_id", null: false
     t.integer "user_id", null: false
@@ -4441,6 +4448,8 @@
   add_foreign_key "approval_project_rules", "projects", on_delete: :cascade
   add_foreign_key "approval_project_rules_groups", "approval_project_rules", on_delete: :cascade
   add_foreign_key "approval_project_rules_groups", "namespaces", column: "group_id", on_delete: :cascade
+  add_foreign_key "approval_project_rules_protected_branches", "approval_project_rules", on_delete: :cascade
+  add_foreign_key "approval_project_rules_protected_branches", "protected_branches", on_delete: :cascade
   add_foreign_key "approval_project_rules_users", "approval_project_rules", on_delete: :cascade
   add_foreign_key "approval_project_rules_users", "users", on_delete: :cascade
   add_foreign_key "approvals", "merge_requests", name: "fk_310d714958", on_delete: :cascade
-- 
GitLab


From 622ea42a5aa03b79cefa2f716d866b1cbf5cea5a Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 9 Jan 2020 18:50:37 +0800
Subject: [PATCH 02/10] Update API endpoints to support new param

A new `protected_branch_ids` has been added so we can associate
`ProtectedBranch` records to a `ApprovalProjectRule`. The IDs
get filtered and will only allow those coming from the same
project.
---
 app/models/concerns/protected_ref.rb          |  2 ++
 ee/app/models/approval_project_rule.rb        |  1 +
 .../concerns/approval_rules/updater.rb        | 10 +++++++
 .../helpers/project_approval_rules_helpers.rb |  2 ++
 ee/lib/api/project_approval_rules.rb          | 12 ++++----
 ee/lib/api/project_approval_settings.rb       |  8 ++---
 ee/lib/ee/api/entities.rb                     |  8 +++--
 .../public_api/v4/project_approval_rule.json  |  7 +++++
 .../v4/project_approval_setting.json          |  7 +++++
 .../approval_rules/create_service_spec.rb     | 29 ++++++++++++++++++
 .../approval_rules/update_service_spec.rb     | 29 +++++++++++++++++-
 ...ject_approval_rules_api_shared_examples.rb | 30 +++++++++++++++++++
 lib/api/entities.rb                           |  1 +
 13 files changed, 133 insertions(+), 13 deletions(-)

diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb
index d9a7f0a96dc657..cddca72f91f779 100644
--- a/app/models/concerns/protected_ref.rb
+++ b/app/models/concerns/protected_ref.rb
@@ -10,6 +10,8 @@ module ProtectedRef
     validates :project, presence: true
 
     delegate :matching, :matches?, :wildcard?, to: :ref_matcher
+
+    scope :for_project, ->(project) { where(project: project) }
   end
 
   def commit
diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb
index 4fc3a185734d25..9c86f164444400 100644
--- a/ee/app/models/approval_project_rule.rb
+++ b/ee/app/models/approval_project_rule.rb
@@ -4,6 +4,7 @@ class ApprovalProjectRule < ApplicationRecord
   include ApprovalRuleLike
 
   belongs_to :project
+  has_and_belongs_to_many :protected_branches
 
   enum rule_type: {
     regular: 0,
diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb
index f360c944c6a999..091b759409c30a 100644
--- a/ee/app/services/concerns/approval_rules/updater.rb
+++ b/ee/app/services/concerns/approval_rules/updater.rb
@@ -5,6 +5,7 @@ module Updater
     def action
       filter_eligible_users!
       filter_eligible_groups!
+      filter_eligible_protected_branches!
 
       if rule.update(params)
         rule.reset
@@ -27,5 +28,14 @@ def filter_eligible_groups!
 
       params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
     end
+
+    def filter_eligible_protected_branches!
+      return unless params.key?(:protected_branch_ids)
+
+      params[:protected_branches] =
+        ProtectedBranch
+          .id_in(params.delete(:protected_branch_ids))
+          .for_project(project)
+    end
   end
 end
diff --git a/ee/lib/api/helpers/project_approval_rules_helpers.rb b/ee/lib/api/helpers/project_approval_rules_helpers.rb
index 90f7e3c5f25104..3e43e7c27fd7f7 100644
--- a/ee/lib/api/helpers/project_approval_rules_helpers.rb
+++ b/ee/lib/api/helpers/project_approval_rules_helpers.rb
@@ -13,6 +13,7 @@ module ProjectApprovalRulesHelpers
         optional :rule_type, type: String, desc: 'The type of approval rule'
         optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
         optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
+        optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule'
       end
 
       params :update_project_approval_rule do
@@ -21,6 +22,7 @@ module ProjectApprovalRulesHelpers
         optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
         optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
         optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
+        optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule'
         optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed'
       end
 
diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb
index a251e568e98a30..2e7a26870d6fdf 100644
--- a/ee/lib/api/project_approval_rules.rb
+++ b/ee/lib/api/project_approval_rules.rb
@@ -12,33 +12,33 @@ class ProjectApprovalRules < ::Grape::API
     resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       segment ':id/approval_rules' do
         desc 'Get all project approval rules' do
-          success EE::API::Entities::ApprovalRule
+          success EE::API::Entities::ProjectApprovalRule
         end
         get do
           authorize_create_merge_request_in_project
 
-          present user_project.visible_approval_rules, with: EE::API::Entities::ApprovalRule, current_user: current_user
+          present user_project.visible_approval_rules, with: EE::API::Entities::ProjectApprovalRule, current_user: current_user
         end
 
         desc 'Create new project approval rule' do
-          success EE::API::Entities::ApprovalRule
+          success EE::API::Entities::ProjectApprovalRule
         end
         params do
           use :create_project_approval_rule
         end
         post do
-          create_project_approval_rule(present_with: EE::API::Entities::ApprovalRule)
+          create_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalRule)
         end
 
         segment ':approval_rule_id' do
           desc 'Update project approval rule' do
-            success EE::API::Entities::ApprovalRule
+            success EE::API::Entities::ProjectApprovalRule
           end
           params do
             use :update_project_approval_rule
           end
           put do
-            update_project_approval_rule(present_with: EE::API::Entities::ApprovalRule)
+            update_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalRule)
           end
 
           desc 'Destroy project approval rule'
diff --git a/ee/lib/api/project_approval_settings.rb b/ee/lib/api/project_approval_settings.rb
index 1a2afe02b9ecdb..9c54b07016148c 100644
--- a/ee/lib/api/project_approval_settings.rb
+++ b/ee/lib/api/project_approval_settings.rb
@@ -24,25 +24,25 @@ class ProjectApprovalSettings < ::Grape::API
         segment 'rules' do
           desc 'Create new approval rule' do
             detail 'Private API subject to change'
-            success EE::API::Entities::ApprovalSettingRule
+            success EE::API::Entities::ProjectApprovalSettingRule
           end
           params do
             use :create_project_approval_rule
           end
           post do
-            create_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule)
+            create_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalSettingRule)
           end
 
           segment ':approval_rule_id' do
             desc 'Update approval rule' do
               detail 'Private API subject to change'
-              success EE::API::Entities::ApprovalSettingRule
+              success EE::API::Entities::ProjectApprovalSettingRule
             end
             params do
               use :update_project_approval_rule
             end
             put do
-              update_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule)
+              update_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalSettingRule)
             end
 
             desc 'Delete an approval rule' do
diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb
index c03b1672aed1f1..9b6923107b4e99 100644
--- a/ee/lib/ee/api/entities.rb
+++ b/ee/lib/ee/api/entities.rb
@@ -420,6 +420,10 @@ def initialize(object, options = {})
         expose :contains_hidden_groups?, as: :contains_hidden_groups
       end
 
+      class ProjectApprovalRule < ApprovalRule
+        expose :protected_branches, using: ::API::Entities::ProtectedBranch
+      end
+
       class MergeRequestApprovalRule < ApprovalRule
         class SourceRule < Grape::Entity
           expose :approvals_required
@@ -446,7 +450,7 @@ class MergeRequestApprovalState < Grape::Entity
       # This overrides the `eligible_approvers` to be exposed as `approvers`.
       #
       # To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574.
-      class ApprovalSettingRule < ApprovalRule
+      class ProjectApprovalSettingRule < ProjectApprovalRule
         expose :approvers, using: ::API::Entities::UserBasic, override: true
       end
 
@@ -454,7 +458,7 @@ class ApprovalSettingRule < ApprovalRule
       #
       # To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574.
       class ProjectApprovalSettings < Grape::Entity
-        expose :visible_approval_rules, as: :rules, using: ApprovalSettingRule
+        expose :visible_approval_rules, as: :rules, using: ProjectApprovalSettingRule
         expose :min_fallback_approvals, as: :fallback_approvals_required
       end
 
diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json
index d27a218bf41556..76ecb81514aa38 100644
--- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json
+++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json
@@ -26,6 +26,13 @@
         "type": "object",
         "properties": {}
       }
+    },
+    "protected_branches": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "properties": {}
+      }
     }
   },
   "additionalProperties": false
diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json
index 226c692a2f3ee0..77b135e52d800d 100644
--- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json
+++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json
@@ -26,6 +26,13 @@
         "type": "object",
         "properties": {}
       }
+    },
+    "protected_branches": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "properties": {}
+      }
     }
   },
   "additionalProperties": false
diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb
index 42674cb87ad6d8..a99ed5936adeb2 100644
--- a/ee/spec/services/approval_rules/create_service_spec.rb
+++ b/ee/spec/services/approval_rules/create_service_spec.rb
@@ -83,6 +83,35 @@
 
     it_behaves_like "creatable"
 
+    context 'when protected_branch_ids param is present' do
+      let(:protected_branch) { create(:protected_branch, project: target) }
+
+      subject do
+        described_class.new(
+          target,
+          user,
+          name: 'developers',
+          approvals_required: 1,
+          protected_branch_ids: [protected_branch.id]
+        ).execute
+      end
+
+      it 'associates the approval rule to the protected branch' do
+        expect(subject[:status]).to eq(:success)
+        expect(subject[:rule].protected_branches).to eq([protected_branch])
+      end
+
+      context 'but protected branch is for another project' do
+        let(:another_project) { create(:project) }
+        let(:protected_branch) { create(:protected_branch, project: another_project) }
+
+        it 'does not associate the approval rule to the protected branch' do
+          expect(subject[:status]).to eq(:success)
+          expect(subject[:rule].protected_branches).to be_empty
+        end
+      end
+    end
+
     ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
       context "when the rule name is `#{rule_name}`" do
         subject { described_class.new(target, user, { name: rule_name, approvals_required: 1 }) }
diff --git a/ee/spec/services/approval_rules/update_service_spec.rb b/ee/spec/services/approval_rules/update_service_spec.rb
index f00b4d2e0025cf..d4d4b47da78b9f 100644
--- a/ee/spec/services/approval_rules/update_service_spec.rb
+++ b/ee/spec/services/approval_rules/update_service_spec.rb
@@ -5,9 +5,9 @@
 describe ApprovalRules::UpdateService do
   let(:project) { create(:project) }
   let(:user) { project.creator }
+  let(:approval_rule) { target.approval_rules.create(name: 'foo') }
 
   shared_examples 'editable' do
-    let(:approval_rule) { target.approval_rules.create(name: 'foo') }
     let(:new_approvers) { create_list(:user, 2) }
     let(:new_groups) { create_list(:group, 2, :private) }
 
@@ -138,6 +138,33 @@
     let(:target) { project }
 
     it_behaves_like "editable"
+
+    context 'when protected_branch_ids param is present' do
+      let(:protected_branch) { create(:protected_branch, project: target) }
+
+      subject do
+        described_class.new(
+          approval_rule,
+          user,
+          protected_branch_ids: [protected_branch.id]
+        ).execute
+      end
+
+      it 'associates the approval rule to the protected branch' do
+        expect(subject[:status]).to eq(:success)
+        expect(subject[:rule].protected_branches).to eq([protected_branch])
+      end
+
+      context 'but protected branch is for another project' do
+        let(:another_project) { create(:project) }
+        let(:protected_branch) { create(:protected_branch, project: another_project) }
+
+        it 'does not associate the approval rule to the protected branch' do
+          expect(subject[:status]).to eq(:success)
+          expect(subject[:rule].protected_branches).to be_empty
+        end
+      end
+    end
   end
 
   context 'when target is merge request' do
diff --git a/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb b/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb
index 5ef1b198173a57..9410b410c1e00b 100644
--- a/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb
+++ b/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb
@@ -45,6 +45,21 @@
       expect(json_response.symbolize_keys).to include(params)
     end
 
+    context 'when protected_branch_ids param is present' do
+      let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } }
+
+      before do
+        post api(url, current_user), params: params.merge(protected_branch_ids: protected_branches.map(&:id))
+      end
+
+      it 'creates approval rule associated to specified protected branches' do
+        expect(json_response['protected_branches']).to contain_exactly(
+          a_hash_including('id' => protected_branches.first.id),
+          a_hash_including('id' => protected_branches.last.id)
+        )
+      end
+    end
+
     ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
       context "when creating a '#{rule_name}' approval rule" do
         it 'specifies a `rule_type` of `report_approver`' do
@@ -65,6 +80,21 @@
       project.add_developer(approver)
     end
 
+    context 'when protected_branch_ids param is present' do
+      let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } }
+
+      before do
+        put api(url, current_user), params: { protected_branch_ids: protected_branches.map(&:id) }
+      end
+
+      it 'associates approval rule to specified protected branches' do
+        expect(json_response['protected_branches']).to contain_exactly(
+          a_hash_including('id' => protected_branches.first.id),
+          a_hash_including('id' => protected_branches.last.id)
+        )
+      end
+    end
+
     context 'when approver already exists' do
       before do
         approval_rule.users << approver
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 74f8edb07847ff..1be73e606d9f94 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -613,6 +613,7 @@ class ProtectedRefAccess < Grape::Entity
     end
 
     class ProtectedBranch < Grape::Entity
+      expose :id
       expose :name
       expose :push_access_levels, using: Entities::ProtectedRefAccess
       expose :merge_access_levels, using: Entities::ProtectedRefAccess
-- 
GitLab


From 6a0efa8616d50b3f997a9e4be8545a5f30518ed6 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 9 Jan 2020 22:13:52 +0800
Subject: [PATCH 03/10] Only consider applicable rules for approval

Use the associated `ProtectedBranch` records to determine if the
MR's target branch matches the rules.

If a rule is not scoped to any protected branches, should be
applicable.
---
 ee/app/models/approval_project_rule.rb       |  8 +++
 ee/app/models/approval_state.rb              |  2 +-
 ee/app/models/ee/project.rb                  | 17 +++--
 ee/spec/models/approval_project_rule_spec.rb | 29 ++++++++
 ee/spec/models/approval_state_spec.rb        | 75 ++++++++++++++++++++
 5 files changed, 123 insertions(+), 8 deletions(-)

diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb
index 9c86f164444400..49c33daa952d8b 100644
--- a/ee/app/models/approval_project_rule.rb
+++ b/ee/app/models/approval_project_rule.rb
@@ -18,6 +18,14 @@ class ApprovalProjectRule < ApplicationRecord
 
   validates :name, uniqueness: { scope: :project_id }
 
+  def self.applicable_to_branch(branch)
+    includes(:protected_branches).select do |rule|
+      next true if rule.protected_branches.empty?
+
+      rule.protected_branches.matching(branch).any?
+    end
+  end
+
   def source_rule
     nil
   end
diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb
index b92e0efb7cb894..fea989262669e6 100644
--- a/ee/app/models/approval_state.rb
+++ b/ee/app/models/approval_state.rb
@@ -157,7 +157,7 @@ def user_defined_rules
       if approval_rules_overwritten?
         user_defined_merge_request_rules
       else
-        project.visible_user_defined_rules.map do |rule|
+        project.visible_user_defined_rules(merge_request.target_branch).map do |rule|
           ApprovalWrappedRule.wrap(merge_request, rule)
         end
       end
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index a4f151687274bc..159b59459306fb 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -408,14 +408,11 @@ def visible_approval_rules
       end
     end
 
-    def visible_user_defined_rules
-      strong_memoize(:visible_user_defined_rules) do
-        rules = approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc)
+    def visible_user_defined_rules(branch = nil)
+      return user_defined_rules.take(1) unless multiple_approval_rules_available?
+      return user_defined_rules unless branch
 
-        next rules.take(1) unless multiple_approval_rules_available?
-
-        rules
-      end
+      user_defined_rules.applicable_to_branch(branch)
     end
 
     # TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329
@@ -771,5 +768,11 @@ def check_pull_mirror_branch_prefix
         errors.add(:pull_mirror_branch_prefix, _('Invalid Git ref'))
       end
     end
+
+    def user_defined_rules
+      strong_memoize(:user_defined_rules) do
+        approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc)
+      end
+    end
   end
 end
diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb
index b738a2c43100f7..ee1b606841de94 100644
--- a/ee/spec/models/approval_project_rule_spec.rb
+++ b/ee/spec/models/approval_project_rule_spec.rb
@@ -147,4 +147,33 @@
       expect { rule.save }.to raise_error(ActiveRecord::RecordNotUnique)
     end
   end
+
+  describe '::applicable_to_branch' do
+    let!(:rule) { create(:approval_project_rule) }
+    let(:branch) { 'stable' }
+
+    subject { described_class.applicable_to_branch(branch) }
+
+    context 'when there are no associated protected branches' do
+      it { is_expected.to eq([rule]) }
+    end
+
+    context 'when there are associated protected branches' do
+      before do
+        rule.update!(protected_branches: protected_branches)
+      end
+
+      context 'and branch matches' do
+        let(:protected_branches) { [create(:protected_branch, name: branch)] }
+
+        it { is_expected.to eq([rule]) }
+      end
+
+      context 'but branch does not match anything' do
+        let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+
+        it { is_expected.to be_empty }
+      end
+    end
+  end
 end
diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb
index f44581c274ec4c..335124f445e328 100644
--- a/ee/spec/models/approval_state_spec.rb
+++ b/ee/spec/models/approval_state_spec.rb
@@ -1380,4 +1380,79 @@ def create_project_member(role)
       end
     end
   end
+
+  describe '#user_defined_rules' do
+    context 'when approval rules are not overwritten' do
+      let!(:project_rule) { create(:approval_project_rule, project: project) }
+      let!(:another_project_rule) { create(:approval_project_rule, project: project) }
+
+      context 'and multiple approval rules is disabled' do
+        it 'returns the first rule' do
+          expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+            project_rule
+          ])
+        end
+      end
+
+      context 'and multiple approval rules is enabled' do
+        before do
+          stub_licensed_features(multiple_approval_rules: true)
+        end
+
+        it 'returns the rules as is' do
+          expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+            project_rule,
+            another_project_rule
+          ])
+        end
+
+        context 'and rules are scoped by protected branches' do
+          let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
+          let(:another_protected_branch) { create(:protected_branch, project: project, name: '*-stable') }
+
+          before do
+            merge_request.update!(target_branch: 'stable-1')
+            another_project_rule.update!(protected_branches: [protected_branch])
+            project_rule.update!(protected_branches: [another_protected_branch])
+          end
+
+          it 'returns the rules that are applicable to the merge request target branch' do
+            expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+              another_project_rule
+            ])
+          end
+        end
+      end
+    end
+
+    context 'when approval rules are overwritten' do
+      let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
+      let!(:another_mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
+
+      before do
+        project.update!(disable_overriding_approvers_per_merge_request: false)
+      end
+
+      context 'when multiple approval rules is disabled' do
+        it 'returns the first rule' do
+          expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+            mr_rule
+          ])
+        end
+      end
+
+      context 'when multiple approval rules is enabled' do
+        before do
+          stub_licensed_features(multiple_approval_rules: true)
+        end
+
+        it 'returns the rules as is' do
+          expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+            mr_rule,
+            another_mr_rule
+          ])
+        end
+      end
+    end
+  end
 end
-- 
GitLab


From f06d3a66ddfaac667791165dd2ea7375bc7f0ff8 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 9 Jan 2020 22:54:03 +0800
Subject: [PATCH 04/10] Only enable feature for GitLab Premium

This feature is only useful when `multiple_approval_rules`
feature is enabled which is only applicable for GitLab Premium
and above.
---
 .../concerns/approval_rules/updater.rb        |  6 ++++-
 ee/lib/ee/api/entities.rb                     |  2 +-
 .../approval_rules/create_service_spec.rb     | 27 ++++++++++++++-----
 .../approval_rules/update_service_spec.rb     | 27 ++++++++++++++-----
 ...ject_approval_rules_api_shared_examples.rb |  2 ++
 5 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb
index 091b759409c30a..ed286763f9419a 100644
--- a/ee/app/services/concerns/approval_rules/updater.rb
+++ b/ee/app/services/concerns/approval_rules/updater.rb
@@ -32,9 +32,13 @@ def filter_eligible_groups!
     def filter_eligible_protected_branches!
       return unless params.key?(:protected_branch_ids)
 
+      protected_branch_ids = params.delete(:protected_branch_ids)
+
+      return unless project.multiple_approval_rules_available?
+
       params[:protected_branches] =
         ProtectedBranch
-          .id_in(params.delete(:protected_branch_ids))
+          .id_in(protected_branch_ids)
           .for_project(project)
     end
   end
diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb
index 9b6923107b4e99..bda0822dc69a70 100644
--- a/ee/lib/ee/api/entities.rb
+++ b/ee/lib/ee/api/entities.rb
@@ -421,7 +421,7 @@ def initialize(object, options = {})
       end
 
       class ProjectApprovalRule < ApprovalRule
-        expose :protected_branches, using: ::API::Entities::ProtectedBranch
+        expose :protected_branches, using: ::API::Entities::ProtectedBranch, if: -> (rule, _) { rule.project.multiple_approval_rules_available? }
       end
 
       class MergeRequestApprovalRule < ApprovalRule
diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb
index a99ed5936adeb2..5f3cd8a61da6b3 100644
--- a/ee/spec/services/approval_rules/create_service_spec.rb
+++ b/ee/spec/services/approval_rules/create_service_spec.rb
@@ -96,15 +96,28 @@
         ).execute
       end
 
-      it 'associates the approval rule to the protected branch' do
-        expect(subject[:status]).to eq(:success)
-        expect(subject[:rule].protected_branches).to eq([protected_branch])
-      end
+      context 'and multiple approval rules is enabled' do
+        before do
+          stub_licensed_features(multiple_approval_rules: true)
+        end
 
-      context 'but protected branch is for another project' do
-        let(:another_project) { create(:project) }
-        let(:protected_branch) { create(:protected_branch, project: another_project) }
+        it 'associates the approval rule to the protected branch' do
+          expect(subject[:status]).to eq(:success)
+          expect(subject[:rule].protected_branches).to eq([protected_branch])
+        end
+
+        context 'but protected branch is for another project' do
+          let(:another_project) { create(:project) }
+          let(:protected_branch) { create(:protected_branch, project: another_project) }
+
+          it 'does not associate the approval rule to the protected branch' do
+            expect(subject[:status]).to eq(:success)
+            expect(subject[:rule].protected_branches).to be_empty
+          end
+        end
+      end
 
+      context 'and multiple approval rules is disabled' do
         it 'does not associate the approval rule to the protected branch' do
           expect(subject[:status]).to eq(:success)
           expect(subject[:rule].protected_branches).to be_empty
diff --git a/ee/spec/services/approval_rules/update_service_spec.rb b/ee/spec/services/approval_rules/update_service_spec.rb
index d4d4b47da78b9f..b3411ba2fcf7db 100644
--- a/ee/spec/services/approval_rules/update_service_spec.rb
+++ b/ee/spec/services/approval_rules/update_service_spec.rb
@@ -150,15 +150,28 @@
         ).execute
       end
 
-      it 'associates the approval rule to the protected branch' do
-        expect(subject[:status]).to eq(:success)
-        expect(subject[:rule].protected_branches).to eq([protected_branch])
-      end
+      context 'and multiple approval rules is enabled' do
+        before do
+          stub_licensed_features(multiple_approval_rules: true)
+        end
+
+        it 'associates the approval rule to the protected branch' do
+          expect(subject[:status]).to eq(:success)
+          expect(subject[:rule].protected_branches).to eq([protected_branch])
+        end
+
+        context 'but protected branch is for another project' do
+          let(:another_project) { create(:project) }
+          let(:protected_branch) { create(:protected_branch, project: another_project) }
 
-      context 'but protected branch is for another project' do
-        let(:another_project) { create(:project) }
-        let(:protected_branch) { create(:protected_branch, project: another_project) }
+          it 'does not associate the approval rule to the protected branch' do
+            expect(subject[:status]).to eq(:success)
+            expect(subject[:rule].protected_branches).to be_empty
+          end
+        end
+      end
 
+      context 'and multiple approval rules is disabled' do
         it 'does not associate the approval rule to the protected branch' do
           expect(subject[:status]).to eq(:success)
           expect(subject[:rule].protected_branches).to be_empty
diff --git a/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb b/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb
index 9410b410c1e00b..33a1c5838351c6 100644
--- a/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb
+++ b/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb
@@ -49,6 +49,7 @@
       let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } }
 
       before do
+        stub_licensed_features(multiple_approval_rules: true)
         post api(url, current_user), params: params.merge(protected_branch_ids: protected_branches.map(&:id))
       end
 
@@ -84,6 +85,7 @@
       let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } }
 
       before do
+        stub_licensed_features(multiple_approval_rules: true)
         put api(url, current_user), params: { protected_branch_ids: protected_branches.map(&:id) }
       end
 
-- 
GitLab


From 8fb12dff394b119cf91336d7f0f87f16f173b404 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Thu, 9 Jan 2020 23:01:51 +0800
Subject: [PATCH 05/10] Add changelog entry

---
 .../460-approval-project-rules-protected-branches-api.yml    | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 ee/changelogs/unreleased/460-approval-project-rules-protected-branches-api.yml

diff --git a/ee/changelogs/unreleased/460-approval-project-rules-protected-branches-api.yml b/ee/changelogs/unreleased/460-approval-project-rules-protected-branches-api.yml
new file mode 100644
index 00000000000000..22a48d715d688d
--- /dev/null
+++ b/ee/changelogs/unreleased/460-approval-project-rules-protected-branches-api.yml
@@ -0,0 +1,5 @@
+---
+title: Scope approval rules by protected branches via API
+merge_request: 22673
+author:
+type: added
-- 
GitLab


From 543b7ab272df542bf7ab96b4ee77d3101065ca4d Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Fri, 10 Jan 2020 12:32:22 +0800
Subject: [PATCH 06/10] Consider applicable MR level rules for approval

When project level rules get overridden and they are added as
source rules of merge request level rules, scoping should still
occur.
---
 ee/app/models/approval_merge_request_rule.rb  |  8 +++
 ee/app/models/approval_project_rule.rb        | 10 ++--
 ee/app/models/approval_state.rb               | 10 +++-
 .../approval_merge_request_rule_spec.rb       | 56 ++++++++++++++++---
 ee/spec/models/approval_project_rule_spec.rb  |  2 +-
 ee/spec/models/approval_state_spec.rb         | 21 +++++++
 6 files changed, 92 insertions(+), 15 deletions(-)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 055a14f78eb9a7..7f69a02eb5f917 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -70,6 +70,14 @@ def self.find_or_create_code_owner_rule(merge_request, pattern)
     retry
   end
 
+  def self.applicable_to_branch(branch)
+    includes(approval_project_rule: :protected_branches).select do |rule|
+      next true unless rule.approval_project_rule.present?
+
+      rule.approval_project_rule.applies_to_branch?(branch)
+    end
+  end
+
   def project
     merge_request.target_project
   end
diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb
index 49c33daa952d8b..85427f8b3d0e1a 100644
--- a/ee/app/models/approval_project_rule.rb
+++ b/ee/app/models/approval_project_rule.rb
@@ -19,11 +19,13 @@ class ApprovalProjectRule < ApplicationRecord
   validates :name, uniqueness: { scope: :project_id }
 
   def self.applicable_to_branch(branch)
-    includes(:protected_branches).select do |rule|
-      next true if rule.protected_branches.empty?
+    includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) }
+  end
+
+  def applies_to_branch?(branch)
+    return true if protected_branches.empty?
 
-      rule.protected_branches.matching(branch).any?
-    end
+    protected_branches.matching(branch).any?
   end
 
   def source_rule
diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb
index fea989262669e6..f5157cb902563b 100644
--- a/ee/app/models/approval_state.rb
+++ b/ee/app/models/approval_state.rb
@@ -157,7 +157,7 @@ def user_defined_rules
       if approval_rules_overwritten?
         user_defined_merge_request_rules
       else
-        project.visible_user_defined_rules(merge_request.target_branch).map do |rule|
+        project.visible_user_defined_rules(target_branch).map do |rule|
           ApprovalWrappedRule.wrap(merge_request, rule)
         end
       end
@@ -201,9 +201,15 @@ def report_approver_rules
 
   def wrapped_rules
     strong_memoize(:wrapped_rules) do
-      merge_request.approval_rules.map do |rule|
+      merge_request.approval_rules.applicable_to_branch(target_branch).map do |rule|
         ApprovalWrappedRule.wrap(merge_request, rule)
       end
     end
   end
+
+  def target_branch
+    strong_memoize(:target_branch) do
+      merge_request.target_branch
+    end
+  end
 end
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index e389f3294d576b..0dd0b7a2725ad3 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -116,12 +116,12 @@
     end
   end
 
-  context 'scopes'  do
-    set(:rb_rule) { create(:code_owner_rule, name: '*.rb') }
-    set(:js_rule) { create(:code_owner_rule, name: '*.js') }
-    set(:css_rule) { create(:code_owner_rule, name: '*.css') }
-    set(:approval_rule) { create(:approval_merge_request_rule) }
-    set(:report_approver_rule) { create(:report_approver_rule) }
+  context 'scopes' do
+    let!(:rb_rule) { create(:code_owner_rule, name: '*.rb') }
+    let!(:js_rule) { create(:code_owner_rule, name: '*.js') }
+    let!(:css_rule) { create(:code_owner_rule, name: '*.css') }
+    let!(:approval_rule) { create(:approval_merge_request_rule) }
+    let!(:report_approver_rule) { create(:report_approver_rule) }
 
     describe '.not_matching_pattern' do
       it 'returns the correct rules' do
@@ -153,8 +153,7 @@
   end
 
   describe '.find_or_create_code_owner_rule' do
-    set(:merge_request) { create(:merge_request) }
-    set(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) }
+    let!(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) }
 
     it 'finds an existing rule' do
       expect(described_class.find_or_create_code_owner_rule(merge_request, '*.rb'))
@@ -182,6 +181,47 @@
     end
   end
 
+  describe '.applicable_to_branch' do
+    let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
+    let(:branch) { 'stable' }
+
+    subject { described_class.applicable_to_branch(branch) }
+
+    context 'when there are no associated source rules' do
+      it { is_expected.to eq([rule]) }
+    end
+
+    context 'when there are associated source rules' do
+      let(:source_rule) { create(:approval_project_rule, project: merge_request.target_project) }
+
+      before do
+        rule.update!(approval_project_rule: source_rule)
+      end
+
+      context 'and there are no associated protected branches to source rule' do
+        it { is_expected.to eq([rule]) }
+      end
+
+      context 'and there are associated protected branches to source rule' do
+        before do
+          source_rule.update!(protected_branches: protected_branches)
+        end
+
+        context 'and branch matches' do
+          let(:protected_branches) { [create(:protected_branch, name: branch)] }
+
+          it { is_expected.to eq([rule]) }
+        end
+
+        context 'but branch does not match anything' do
+          let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+
+          it { is_expected.to be_empty }
+        end
+      end
+    end
+  end
+
   describe '#project' do
     it 'returns project of MergeRequest' do
       expect(subject.project).to be_present
diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb
index ee1b606841de94..31bdc409da60ac 100644
--- a/ee/spec/models/approval_project_rule_spec.rb
+++ b/ee/spec/models/approval_project_rule_spec.rb
@@ -148,7 +148,7 @@
     end
   end
 
-  describe '::applicable_to_branch' do
+  describe '.applicable_to_branch' do
     let!(:rule) { create(:approval_project_rule) }
     let(:branch) { 'stable' }
 
diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb
index 335124f445e328..f48dd12f574b0c 100644
--- a/ee/spec/models/approval_state_spec.rb
+++ b/ee/spec/models/approval_state_spec.rb
@@ -1452,6 +1452,27 @@ def create_project_member(role)
             another_mr_rule
           ])
         end
+
+        context 'and rules have source rules that are scoped by protected branches' do
+          let(:source_rule) { create(:approval_project_rule, project: project) }
+          let(:another_source_rule) { create(:approval_project_rule, project: project) }
+          let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
+          let(:another_protected_branch) { create(:protected_branch, project: project, name: '*-stable') }
+
+          before do
+            merge_request.update!(target_branch: 'stable-1')
+            source_rule.update!(protected_branches: [protected_branch])
+            another_source_rule.update!(protected_branches: [another_protected_branch])
+            mr_rule.update!(approval_project_rule: another_source_rule)
+            another_mr_rule.update!(approval_project_rule: source_rule)
+          end
+
+          it 'returns the rules that are applicable to the merge request target branch' do
+            expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+              another_mr_rule
+            ])
+          end
+        end
       end
     end
   end
-- 
GitLab


From cca39b8a5cd470384c76190157e557dbb293ef9c Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Mon, 13 Jan 2020 08:29:53 +0800
Subject: [PATCH 07/10] Do not include migration helpers

They are not needed in this case.
---
 ...09085206_create_approval_project_rules_protected_branches.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
index 7ab1041cea348a..ef2d83c78df3fb 100644
--- a/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
+++ b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 class CreateApprovalProjectRulesProtectedBranches < ActiveRecord::Migration[5.2]
-  include Gitlab::Database::MigrationHelpers
-
   DOWNTIME = false
 
   def change
-- 
GitLab


From 1573ace85587cade61cd5aa01cfdca11b5ca52ae Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Tue, 14 Jan 2020 07:41:50 +0800
Subject: [PATCH 08/10] Remove ID from join table migration

---
 ...09085206_create_approval_project_rules_protected_branches.rb | 2 +-
 db/schema.rb                                                    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
index ef2d83c78df3fb..4e75f7d41fdf09 100644
--- a/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
+++ b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb
@@ -4,7 +4,7 @@ class CreateApprovalProjectRulesProtectedBranches < ActiveRecord::Migration[5.2]
   DOWNTIME = false
 
   def change
-    create_table :approval_project_rules_protected_branches, id: :bigserial do |t|
+    create_table :approval_project_rules_protected_branches, id: false do |t|
       t.references :approval_project_rule,
                    null: false,
                    index: false,
diff --git a/db/schema.rb b/db/schema.rb
index 754a750fbdb36b..158f8cc100d9be 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -437,7 +437,7 @@
     t.index ["group_id"], name: "index_approval_project_rules_groups_2"
   end
 
-  create_table "approval_project_rules_protected_branches", force: :cascade do |t|
+  create_table "approval_project_rules_protected_branches", id: false, force: :cascade do |t|
     t.bigint "approval_project_rule_id", null: false
     t.bigint "protected_branch_id", null: false
     t.index ["approval_project_rule_id", "protected_branch_id"], name: "index_approval_project_rules_protected_branches_unique", unique: true
-- 
GitLab


From dd75ae2b4d4cfedaa252bf22cd4ff0373563f6c0 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Wed, 15 Jan 2020 14:47:52 +0800
Subject: [PATCH 09/10] Add scoped_approval_rules feature flag

When enabled, project and merge request level approval rules will
be checked whether it applies to the MR's target branch.

When disabled, it'll returns rules unscoped.

This feature flag is enabled by default. Also being pushed to frontend.
---
 app/controllers/projects_controller.rb       |  9 ++--
 ee/app/controllers/ee/projects_controller.rb | 10 ++++-
 ee/app/models/approval_state.rb              |  9 +++-
 ee/app/models/ee/project.rb                  |  6 ++-
 ee/spec/models/approval_state_spec.rb        | 46 ++++++++++++++++----
 5 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 1d9f81aaa23e3d..76acca3b3bc45a 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -51,7 +51,7 @@ def new
 
   def edit
     @badge_api_endpoint = expose_url(api_v4_projects_badges_path(id: @project.id))
-    render 'edit'
+    render_edit
   end
 
   def create
@@ -85,7 +85,7 @@ def update
       else
         flash.now[:alert] = result[:message]
 
-        format.html { render 'edit' }
+        format.html { render_edit }
       end
 
       format.js
@@ -387,7 +387,6 @@ def project_params_attributes
       :merge_method,
       :initialize_with_readme,
       :autoclose_referenced_issues,
-      :suggestion_commit_message,
 
       project_feature_attributes: %i[
         builds_access_level
@@ -488,6 +487,10 @@ def export_rate_limit
   def rate_limiter
     ::Gitlab::ApplicationRateLimiter
   end
+
+  def render_edit
+    render 'edit'
+  end
 end
 
 ProjectsController.prepend_if_ee('EE::ProjectsController')
diff --git a/ee/app/controllers/ee/projects_controller.rb b/ee/app/controllers/ee/projects_controller.rb
index 169cd6b831e78b..004e3e905a554f 100644
--- a/ee/app/controllers/ee/projects_controller.rb
+++ b/ee/app/controllers/ee/projects_controller.rb
@@ -23,7 +23,7 @@ def restore
       else
         flash.now[:alert] = result[:message]
 
-        render 'edit'
+        render_edit
       end
     end
 
@@ -41,7 +41,7 @@ def destroy
       else
         flash.now[:alert] = result[:message]
 
-        render 'edit'
+        render_edit
       end
     end
 
@@ -137,5 +137,11 @@ def log_archive_audit_event
     def log_unarchive_audit_event
       log_audit_event(message: 'Project unarchived')
     end
+
+    override :render_edit
+    def render_edit
+      push_frontend_feature_flag(:scoped_approval_rules, project, default_enabled: true)
+      super
+    end
   end
 end
diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb
index f5157cb902563b..6fe4ef13d6a82d 100644
--- a/ee/app/models/approval_state.rb
+++ b/ee/app/models/approval_state.rb
@@ -157,7 +157,9 @@ def user_defined_rules
       if approval_rules_overwritten?
         user_defined_merge_request_rules
       else
-        project.visible_user_defined_rules(target_branch).map do |rule|
+        branch = project.scoped_approval_rules_enabled? ? target_branch : nil
+
+        project.visible_user_defined_rules(branch: branch).map do |rule|
           ApprovalWrappedRule.wrap(merge_request, rule)
         end
       end
@@ -201,7 +203,10 @@ def report_approver_rules
 
   def wrapped_rules
     strong_memoize(:wrapped_rules) do
-      merge_request.approval_rules.applicable_to_branch(target_branch).map do |rule|
+      merge_request_rules = merge_request.approval_rules
+      merge_request_rules = merge_request_rules.applicable_to_branch(target_branch) if project.scoped_approval_rules_enabled?
+
+      merge_request_rules.map do |rule|
         ApprovalWrappedRule.wrap(merge_request, rule)
       end
     end
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index 159b59459306fb..4cef4bc20e2176 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -323,6 +323,10 @@ def code_owner_approval_required_available?
       feature_available?(:code_owner_approval_required)
     end
 
+    def scoped_approval_rules_enabled?
+      ::Feature.enabled?(:scoped_approval_rules, self, default_enabled: true)
+    end
+
     def service_desk_enabled
       ::EE::Gitlab::ServiceDesk.enabled?(project: self) && super
     end
@@ -408,7 +412,7 @@ def visible_approval_rules
       end
     end
 
-    def visible_user_defined_rules(branch = nil)
+    def visible_user_defined_rules(branch: nil)
       return user_defined_rules.take(1) unless multiple_approval_rules_available?
       return user_defined_rules unless branch
 
diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb
index f48dd12f574b0c..be580fd802cb8d 100644
--- a/ee/spec/models/approval_state_spec.rb
+++ b/ee/spec/models/approval_state_spec.rb
@@ -1416,10 +1416,25 @@ def create_project_member(role)
             project_rule.update!(protected_branches: [another_protected_branch])
           end
 
-          it 'returns the rules that are applicable to the merge request target branch' do
-            expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
-              another_project_rule
-            ])
+          context 'and scoped_approval_rules feature is enabled' do
+            it 'returns the rules that are applicable to the merge request target branch' do
+              expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
+                another_project_rule
+              ])
+            end
+          end
+
+          context 'but scoped_approval_rules feature is disabled' do
+            before do
+              stub_feature_flags(scoped_approval_rules: false)
+            end
+
+            it 'returns unscoped rules' do
+              expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+                project_rule,
+                another_project_rule
+              ])
+            end
           end
         end
       end
@@ -1467,10 +1482,25 @@ def create_project_member(role)
             another_mr_rule.update!(approval_project_rule: source_rule)
           end
 
-          it 'returns the rules that are applicable to the merge request target branch' do
-            expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
-              another_mr_rule
-            ])
+          context 'and scoped_approval_rules feature is enabled' do
+            it 'returns the rules that are applicable to the merge request target branch' do
+              expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
+                another_mr_rule
+              ])
+            end
+          end
+
+          context 'but scoped_approval_rules feature is disabled' do
+            before do
+              stub_feature_flags(scoped_approval_rules: false)
+            end
+
+            it 'returns unscoped rules' do
+              expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
+                mr_rule,
+                another_mr_rule
+              ])
+            end
           end
         end
       end
-- 
GitLab


From 7ce639c0d88ee5a423fd39052e8b300dec9df311 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Wed, 15 Jan 2020 15:20:50 +0800
Subject: [PATCH 10/10] Restrict users without admin_project ability

Users without admin_project ability shouldn't be able to set
protected branches for approval rules.
---
 ee/app/services/concerns/approval_rules/updater.rb   |  2 +-
 .../services/approval_rules/create_service_spec.rb   | 12 ++++++++++++
 .../services/approval_rules/update_service_spec.rb   | 12 ++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb
index ed286763f9419a..a9dd5acbc24d44 100644
--- a/ee/app/services/concerns/approval_rules/updater.rb
+++ b/ee/app/services/concerns/approval_rules/updater.rb
@@ -34,7 +34,7 @@ def filter_eligible_protected_branches!
 
       protected_branch_ids = params.delete(:protected_branch_ids)
 
-      return unless project.multiple_approval_rules_available?
+      return unless project.multiple_approval_rules_available? && can?(current_user, :admin_project, project)
 
       params[:protected_branches] =
         ProtectedBranch
diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb
index 5f3cd8a61da6b3..73fcd502dda7a0 100644
--- a/ee/spec/services/approval_rules/create_service_spec.rb
+++ b/ee/spec/services/approval_rules/create_service_spec.rb
@@ -106,6 +106,18 @@
           expect(subject[:rule].protected_branches).to eq([protected_branch])
         end
 
+        context 'but user cannot administer project' do
+          before do
+            allow(Ability).to receive(:allowed?).and_call_original
+            allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false)
+          end
+
+          it 'does not associate the approval rule to the protected branch' do
+            expect(subject[:status]).to eq(:success)
+            expect(subject[:rule].protected_branches).to be_empty
+          end
+        end
+
         context 'but protected branch is for another project' do
           let(:another_project) { create(:project) }
           let(:protected_branch) { create(:protected_branch, project: another_project) }
diff --git a/ee/spec/services/approval_rules/update_service_spec.rb b/ee/spec/services/approval_rules/update_service_spec.rb
index b3411ba2fcf7db..5f948b979c1d44 100644
--- a/ee/spec/services/approval_rules/update_service_spec.rb
+++ b/ee/spec/services/approval_rules/update_service_spec.rb
@@ -160,6 +160,18 @@
           expect(subject[:rule].protected_branches).to eq([protected_branch])
         end
 
+        context 'but user cannot administer project' do
+          before do
+            allow(Ability).to receive(:allowed?).and_call_original
+            allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false)
+          end
+
+          it 'does not associate the approval rule to the protected branch' do
+            expect(subject[:status]).to eq(:success)
+            expect(subject[:rule].protected_branches).to be_empty
+          end
+        end
+
         context 'but protected branch is for another project' do
           let(:another_project) { create(:project) }
           let(:protected_branch) { create(:protected_branch, project: another_project) }
-- 
GitLab