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