Merge branch 'ee-1137-follow-up-protected-branch-users-and-groups-ee' into 'master'

EE-specific changes for gitlab-org/gitlab-ee#1137

## Summary

- gitlab-org/gitlab-ee#1137 is a `technical debt` issue to clean up the EE protected branch access levels (for users and groups) implementation.
- Some of this cleanup bleeds over to code shared by CE and EE. This portion is covered in this CE merge request: gitlab-org/gitlab-ce!7821

## References

- Closes #1137 

## Tasks

-  [#1137/!7821/!927] Follow-up from restricting pushes / merges by group
    - [x]  Implementation
        - [x]  Prefer `validates` with `:uniqueness` option!
            - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645#note_16390918
        - [x]  Could this be moved into `ProtectedBranchAccess`?
            - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645#note_16391013
            - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645#note_16391018
            - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645#note_16391023
        - [x]  Please name controller action specs after the method and action: `describe "GET project_groups"`
            - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645#note_16391093
        - [x]  I think we need more extensive integration specs here
            - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645#note_16391229
        - [x]  Does this need to be a proc?
            - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645#note_17145530
    - [x]  Tests
        - [x]  CE Passing
        - [x]  EE Passing
    - [x]  Meta
        - [x]  CHANGELOG entry created
        - [x]  EE branch has no merge conflicts with EE `master`
        - [x]  CE branch has no merge conflicts with CE `master`
        - [x]  Squashed related commits together
    - [x]  Review
        - [x]  CE Endboss
        - [x]  EE Endboss
            - [x]  Break javascript, make sure integration specs catch the failure
    - [ ]  Merge
        - [x]  CE
        - [ ]  EE


See merge request !927

app/models/concerns/protected_branch_access.rb

@@ -2,12 +2,8 @@ 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) }
+    include EE::ProtectedBranchAccess
+
     belongs_to :protected_branch
     delegate :project, to: :protected_branch
 
@@ -15,20 +11,7 @@ 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
-  end
-
   def humanize
-    return self.user.name if self.user.present?
-    return self.group.name if self.group.present?
-
     self.class.human_access_levels[self.access_level]
   end
 

app/models/ee/protected_branch_access.rb

+# 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

app/models/protected_branch/merge_access_level.rb

 class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   include ProtectedBranchAccess
 
-  belongs_to :user
-  belongs_to :group
-
   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

app/models/protected_branch/push_access_level.rb

 class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   include ProtectedBranchAccess
 
-  belongs_to :user
-  belongs_to :group
-
   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 +15,6 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
 
   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

changelogs/unreleased-ee/ee-1137-follow-up-protected-branch-users-and-groups-ee.yml

+---
+title: Technical debt follow-up from restricting pushes / merges by group
+merge_request: 927
+author: