From 323ed342cb42a5d7cfac9084ebd7b200499cf1ed Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 29 Nov 2016 16:58:59 +0530
Subject: [PATCH 1/4] Extract a `ProtectedBranchAccessEe` module

- EE-specific protected branch access level code lives in this new module, while
  CE or CE/EE code lives in the existing `ProtectedBranchAccess` module. This
  allows us to make changes without introducing conflicts.

- The access level model classes first include `ProtectedBranchAccess`, followed
  by `ProtectedBranchAccessEe`, which preserves the inheritance chain:

  {Push,Merge}AccessLevel > ProtectedBranchAccessEe > ProtectedBranchAccess
---
 .../concerns/protected_branch_access.rb       | 23 ++------
 .../concerns/protected_branch_access_ee.rb    | 53 +++++++++++++++++++
 .../protected_branch/merge_access_level.rb    | 15 +-----
 .../protected_branch/push_access_level.rb     | 10 +---
 4 files changed, 60 insertions(+), 41 deletions(-)
 create mode 100644 app/models/concerns/protected_branch_access_ee.rb

diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb
index 28c9c73b246bb..f410dbbab80a8 100644
--- a/app/models/concerns/protected_branch_access.rb
+++ b/app/models/concerns/protected_branch_access.rb
@@ -2,12 +2,6 @@ module ProtectedBranchAccess
   extend ActiveSupport::Concern
 
   included do
-    validates_uniqueness_of :group_id, scope: :protected_branch, allow_nil: true
-    validates_uniqueness_of :user_id, scope: :protected_branch, allow_nil: true
-    validates_uniqueness_of :access_level,
-                            scope: :protected_branch,
-                            unless: Proc.new { |access_level| access_level.user_id? || access_level.group_id? },
-                            conditions: -> { where(user_id: nil, group_id: nil) }
     belongs_to :protected_branch
     delegate :project, to: :protected_branch
 
@@ -15,21 +9,14 @@ module ProtectedBranchAccess
     scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) }
   end
 
-  def type
-    if self.user.present?
-      :user
-    elsif self.group.present?
-      :group
-    else
-      :role
-    end
+  def humanize
+    self.class.human_access_levels[self.access_level]
   end
 
-  def humanize
-    return self.user.name if self.user.present?
-    return self.group.name if self.group.present?
+  def check_access(user)
+    return true if user.is_admin?
 
-    self.class.human_access_levels[self.access_level]
+    project.team.max_member_access(user.id) >= access_level
   end
 
   def check_access(user)
diff --git a/app/models/concerns/protected_branch_access_ee.rb b/app/models/concerns/protected_branch_access_ee.rb
new file mode 100644
index 0000000000000..1006453c16fed
--- /dev/null
+++ b/app/models/concerns/protected_branch_access_ee.rb
@@ -0,0 +1,53 @@
+# EE-specific code related to protected branch access levels.
+#
+# Note: Include `ProtectedBranchAccess` _before_ including this module, since
+# a number of methods here override methods in `ProtectedBranchAccess`
+
+module ProtectedBranchAccessEe
+  extend ActiveSupport::Concern
+
+  included do
+    belongs_to :user
+    belongs_to :group
+
+    validates_uniqueness_of :group_id, scope: :protected_branch, allow_nil: true
+    validates_uniqueness_of :user_id, scope: :protected_branch, allow_nil: true
+    validates_uniqueness_of :access_level,
+                            scope: :protected_branch,
+                            if: :role?,
+                            conditions: -> { where(user_id: nil, group_id: nil) }
+
+    scope :by_user, -> (user) { where(user: user ) }
+    scope :by_group, -> (group) { where(group: group ) }
+  end
+
+  def type
+    if self.user.present?
+      :user
+    elsif self.group.present?
+      :group
+    else
+      :role
+    end
+  end
+
+  # Is this a role-based access level?
+  def role?
+    type == :role
+  end
+
+  def humanize
+    return self.user.name if self.user.present?
+    return self.group.name if self.group.present?
+
+    super
+  end
+
+  def check_access(user)
+    return true if user.is_admin?
+    return user.id == self.user_id if self.user.present?
+    return group.users.exists?(user.id) if self.group.present?
+
+    super
+  end
+end
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index a5c059d995e20..19f2fdd8d3bd2 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -1,27 +1,14 @@
 class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   include ProtectedBranchAccess
-
-  belongs_to :user
-  belongs_to :group
+  include ProtectedBranchAccessEe
 
   validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
                                                              Gitlab::Access::DEVELOPER] }
 
-  scope :by_user, -> (user) { where(user: user ) }
-  scope :by_group, -> (group) { where(group: group ) }
-
   def self.human_access_levels
     {
       Gitlab::Access::MASTER => "Masters",
       Gitlab::Access::DEVELOPER => "Developers + Masters"
     }.with_indifferent_access
   end
-
-  def check_access(user)
-    return true if user.is_admin?
-    return user.id == self.user_id if self.user.present?
-    return group.users.exists?(user.id) if self.group.present?
-
-    super
-  end
 end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index a4d50d25fb675..881a918519506 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -1,16 +1,11 @@
 class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   include ProtectedBranchAccess
-
-  belongs_to :user
-  belongs_to :group
+  include ProtectedBranchAccessEe
 
   validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
                                                              Gitlab::Access::DEVELOPER,
                                                              Gitlab::Access::NO_ACCESS] }
 
-  scope :by_user, -> (user) { where(user: user ) }
-  scope :by_group, -> (group) { where(group: group ) }
-
   def self.human_access_levels
     {
       Gitlab::Access::MASTER => "Masters",
@@ -21,9 +16,6 @@ def self.human_access_levels
 
   def check_access(user)
     return false if access_level == Gitlab::Access::NO_ACCESS
-    return true if user.is_admin?
-    return user.id == self.user_id if self.user.present?
-    return group.users.exists?(user.id) if self.group.present?
 
     super
   end
-- 
GitLab


From 336c988dc3d19f7cf1bea30727679d5aff720aa1 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 29 Nov 2016 17:22:25 +0530
Subject: [PATCH 2/4] Prefer the `validates ... uniqueness` syntax

- Over `validates_uniqueness_of`
---
 app/models/concerns/protected_branch_access_ee.rb | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/app/models/concerns/protected_branch_access_ee.rb b/app/models/concerns/protected_branch_access_ee.rb
index 1006453c16fed..8f6e6e578cc50 100644
--- a/app/models/concerns/protected_branch_access_ee.rb
+++ b/app/models/concerns/protected_branch_access_ee.rb
@@ -10,12 +10,10 @@ module ProtectedBranchAccessEe
     belongs_to :user
     belongs_to :group
 
-    validates_uniqueness_of :group_id, scope: :protected_branch, allow_nil: true
-    validates_uniqueness_of :user_id, scope: :protected_branch, allow_nil: true
-    validates_uniqueness_of :access_level,
-                            scope: :protected_branch,
-                            if: :role?,
-                            conditions: -> { where(user_id: nil, group_id: nil) }
+    validates :group_id, uniqueness: { scope: :protected_branch, allow_nil: true }
+    validates :user_id, uniqueness: { scope: :protected_branch, allow_nil: true }
+    validates :access_level, uniqueness: { scope: :protected_branch, if: :role?,
+                                           conditions: -> { where(user_id: nil, group_id: nil) } }
 
     scope :by_user, -> (user) { where(user: user ) }
     scope :by_group, -> (group) { where(group: group ) }
-- 
GitLab


From 2a3b3545273fb0eaf9b2096942d88b9d34edf533 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 29 Nov 2016 17:43:01 +0530
Subject: [PATCH 3/4] Add #1137 to the CHANGELOG

---
 ...ee-1137-follow-up-protected-branch-users-and-groups-ee.yml | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml

diff --git a/changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml b/changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml
new file mode 100644
index 0000000000000..43578100727bd
--- /dev/null
+++ b/changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml
@@ -0,0 +1,4 @@
+---
+title: Technical debt follow-up from restricting pushes / merges by group
+merge_request: 927
+author: Timothy Andrew
-- 
GitLab


From 60a38e5cb2ef618e76a5a4af34d0ac3cacfba6b1 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 6 Dec 2016 11:38:45 +0530
Subject: [PATCH 4/4] Implement changes from @DouweM's review of !927

- Move `ProctedBranchAccessEe` to `EE::ProtectedBranchAccess`. This is a lot
  cleaner, and has a precedent (`EE::Group`).

- Include `EE::ProtectedBranchAccess` inside `ProtectedBranchAccess`, instead of
  including both in the model classes. `EE::ProtectedBranchAccess` depends on
  `ProtectedBranchAccess` - this is a good way to model that dependency.

- Remove author credit from CHANGELOG entry
---
 .../concerns/protected_branch_access.rb       |  8 +--
 .../concerns/protected_branch_access_ee.rb    | 51 -----------------
 app/models/ee/protected_branch_access.rb      | 55 +++++++++++++++++++
 .../protected_branch/merge_access_level.rb    |  1 -
 .../protected_branch/push_access_level.rb     |  1 -
 ...p-protected-branch-users-and-groups-ee.yml |  2 +-
 6 files changed, 58 insertions(+), 60 deletions(-)
 delete mode 100644 app/models/concerns/protected_branch_access_ee.rb
 create mode 100644 app/models/ee/protected_branch_access.rb

diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb
index f410dbbab80a8..e46a2d4d04e86 100644
--- a/app/models/concerns/protected_branch_access.rb
+++ b/app/models/concerns/protected_branch_access.rb
@@ -2,6 +2,8 @@ module ProtectedBranchAccess
   extend ActiveSupport::Concern
 
   included do
+    include EE::ProtectedBranchAccess
+
     belongs_to :protected_branch
     delegate :project, to: :protected_branch
 
@@ -18,10 +20,4 @@ def check_access(user)
 
     project.team.max_member_access(user.id) >= access_level
   end
-
-  def check_access(user)
-    return true if user.is_admin?
-
-    project.team.max_member_access(user.id) >= access_level
-  end
 end
diff --git a/app/models/concerns/protected_branch_access_ee.rb b/app/models/concerns/protected_branch_access_ee.rb
deleted file mode 100644
index 8f6e6e578cc50..0000000000000
--- a/app/models/concerns/protected_branch_access_ee.rb
+++ /dev/null
@@ -1,51 +0,0 @@
-# EE-specific code related to protected branch access levels.
-#
-# Note: Include `ProtectedBranchAccess` _before_ including this module, since
-# a number of methods here override methods in `ProtectedBranchAccess`
-
-module ProtectedBranchAccessEe
-  extend ActiveSupport::Concern
-
-  included do
-    belongs_to :user
-    belongs_to :group
-
-    validates :group_id, uniqueness: { scope: :protected_branch, allow_nil: true }
-    validates :user_id, uniqueness: { scope: :protected_branch, allow_nil: true }
-    validates :access_level, uniqueness: { scope: :protected_branch, if: :role?,
-                                           conditions: -> { where(user_id: nil, group_id: nil) } }
-
-    scope :by_user, -> (user) { where(user: user ) }
-    scope :by_group, -> (group) { where(group: group ) }
-  end
-
-  def type
-    if self.user.present?
-      :user
-    elsif self.group.present?
-      :group
-    else
-      :role
-    end
-  end
-
-  # Is this a role-based access level?
-  def role?
-    type == :role
-  end
-
-  def humanize
-    return self.user.name if self.user.present?
-    return self.group.name if self.group.present?
-
-    super
-  end
-
-  def check_access(user)
-    return true if user.is_admin?
-    return user.id == self.user_id if self.user.present?
-    return group.users.exists?(user.id) if self.group.present?
-
-    super
-  end
-end
diff --git a/app/models/ee/protected_branch_access.rb b/app/models/ee/protected_branch_access.rb
new file mode 100644
index 0000000000000..065ce88f57b02
--- /dev/null
+++ b/app/models/ee/protected_branch_access.rb
@@ -0,0 +1,55 @@
+# EE-specific code related to protected branch access levels.
+#
+# Note: Don't directly include this concern into a model class.
+# Instead, include `ProtectedBranchAccess`, which in turn includes
+# this concern. A number of methods here depend on `ProtectedBranchAccess`
+# being next up in the ancestor chain.
+
+module EE
+  module ProtectedBranchAccess
+    extend ActiveSupport::Concern
+
+    included do
+      belongs_to :user
+      belongs_to :group
+
+      validates :group_id, uniqueness: { scope: :protected_branch, allow_nil: true }
+      validates :user_id, uniqueness: { scope: :protected_branch, allow_nil: true }
+      validates :access_level, uniqueness: { scope: :protected_branch, if: :role?,
+                                             conditions: -> { where(user_id: nil, group_id: nil) } }
+
+      scope :by_user, -> (user) { where(user: user ) }
+      scope :by_group, -> (group) { where(group: group ) }
+    end
+
+    def type
+      if self.user.present?
+        :user
+      elsif self.group.present?
+        :group
+      else
+        :role
+      end
+    end
+
+    # Is this a role-based access level?
+    def role?
+      type == :role
+    end
+
+    def humanize
+      return self.user.name if self.user.present?
+      return self.group.name if self.group.present?
+
+      super
+    end
+
+    def check_access(user)
+      return true if user.is_admin?
+      return user.id == self.user_id if self.user.present?
+      return group.users.exists?(user.id) if self.group.present?
+
+      super
+    end
+  end
+end
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 19f2fdd8d3bd2..771e337661355 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -1,6 +1,5 @@
 class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   include ProtectedBranchAccess
-  include ProtectedBranchAccessEe
 
   validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
                                                              Gitlab::Access::DEVELOPER] }
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 881a918519506..14610cb42b7eb 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -1,6 +1,5 @@
 class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   include ProtectedBranchAccess
-  include ProtectedBranchAccessEe
 
   validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
                                                              Gitlab::Access::DEVELOPER,
diff --git a/changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml b/changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml
index 43578100727bd..6cb7e2a3a4964 100644
--- a/changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml
+++ b/changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml
@@ -1,4 +1,4 @@
 ---
 title: Technical debt follow-up from restricting pushes / merges by group
 merge_request: 927
-author: Timothy Andrew
+author:
-- 
GitLab