Merge branch 'chore/468688-refactor-check_access' into 'master'

Refactor check_access method in protected ref modules

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/168549



Merged-by: Joe Woodward <jwoodward@gitlab.com>
Approved-by: Costel Maxim <cmaxim@gitlab.com>
Approved-by: Emerald-Jayde Henao <ejhenao@gitlab.com>
Approved-by: Dylan Griffith <dyl.griffith@gmail.com>
Reviewed-by: Emerald-Jayde Henao <ejhenao@gitlab.com>
Co-authored-by: Joe Woodward <j@joewoodward.me>

app/models/concerns/protected_branch_access.rb

@@ -7,7 +7,7 @@ module ProtectedBranchAccess
   included do
     belongs_to :protected_branch
-    delegate :project, to: :protected_branch, allow_nil: true
+    delegate :project, to: :protected_branch, allow_nil: true, prefix: :protected_ref
     # We cannot delegate to :protected_branch here (even with allow_nil: true)
     # like above because it results in
@@ -17,5 +17,3 @@ def protected_branch_group
     end
   end
 end
-
-ProtectedBranchAccess.prepend_mod_with('ProtectedBranchAccess')

app/models/concerns/protected_ref_access.rb

@@ -52,10 +52,6 @@ def non_role_types
       if: :role?
   end
-  def humanize
-    self.class.humanize(access_level)
-  end
-
   def type
     :role
   end
@@ -64,17 +60,31 @@ def role?
     type == :role
   end
-  def check_access(current_user, current_project = project)
+  def humanize
+    # humanize_role
+    # humanize_user
+    # humanize_group
+    # humanize_deploy_key
+    send(:"humanize_#{type}") # rubocop:disable GitlabSecurity/PublicSend -- Intentional meta programming to direct to correct type
+  end
+
+  def check_access(current_user, current_project = protected_ref_project)
     return false if current_user.nil? || no_access?
     return current_user.admin? if admin_access?
-    yield if block_given?
-
-    user_can_access?(current_user, current_project)
+    # role_access_allowed?
+    # user_access_allowed?
+    # group_access_allowed?
+    # deploy_key_access_allowed?
+    send(:"#{type}_access_allowed?", current_user, current_project) # rubocop:disable GitlabSecurity/PublicSend -- Intentional meta programming to direct check to correct type
   end
   private
+  def humanize_role
+    self.class.humanize(access_level)
+  end
+
   def admin_access?
     role? && access_level == ::Gitlab::Access::ADMIN
   end
@@ -83,12 +93,12 @@ def no_access?
     role? && access_level == Gitlab::Access::NO_ACCESS
   end
-  def user_can_access?(current_user, current_project)
+  def role_access_allowed?(current_user, current_project)
     # NOTE: A user could be a group member which would be inherited in
-    # projects, however, the same user can have direct membership to a project
-    # with a higher role. For this reason we need to check group-level rules
-    # against the current project when merging an MR or pushing changes to a
-    # protected branch.
+    # projects, however, the same user can have direct membership to a
+    # project with a higher role. For this reason we need to check group-level
+    # rules against the current project when merging an MR or pushing changes
+    # to a protected branch.
     if current_project
       current_user.can?(:push_code, current_project) &&
         current_project.team.max_member_access(current_user.id) >= access_level

app/models/concerns/protected_ref_deploy_key_access.rb

@@ -19,31 +19,15 @@ def non_role_types
   end
   def type
-    return :deploy_key if deploy_key_id.present? || deploy_key.present?
+    return :deploy_key if deploy_key_id || deploy_key
     super
   end
-  def humanize
-    return humanize_deploy_key if deploy_key?
-
-    super
-  end
-
-  def check_access(current_user, current_project = project)
-    super do
-      break deploy_key_access_allowed?(current_user) if deploy_key?
-
-      yield if block_given?
-    end
-  end
-
   private
   def humanize_deploy_key
-    return deploy_key.title if deploy_key.present?
-
-    'Deploy key'
+    deploy_key&.title || 'Deploy key'
   end
   def deploy_key?
@@ -56,7 +40,10 @@ def validate_deploy_key_membership
     errors.add(:deploy_key, 'is not enabled for this project')
   end
-  def deploy_key_access_allowed?(current_user)
+  # current_project is only available when evaluating a group-level protected
+  # branch. We only allow role based access levels at the group-level so we can
+  # ignore it here.
+  def deploy_key_access_allowed?(current_user, _current_project)
     deploy_key_owned_by?(current_user) && valid_deploy_key_status?
   end
@@ -65,16 +52,16 @@ def deploy_key_owned_by?(current_user)
   end
   def valid_deploy_key_status?
-    deploy_key.user.can?(:read_project, project) &&
+    deploy_key.user.can?(:read_project, protected_ref_project) &&
       deploy_key_owner_project_member? &&
       deploy_key_has_write_access_to_project?
   end
   def deploy_key_owner_project_member?
-    project.member?(deploy_key.user)
+    protected_ref_project.member?(deploy_key.user)
   end
   def deploy_key_has_write_access_to_project?
-    DeployKey.with_write_access_for_project(project, deploy_key: deploy_key).exists?
+    DeployKey.with_write_access_for_project(protected_ref_project, deploy_key: deploy_key).exists?
   end
 end

app/models/concerns/protected_tag_access.rb

@@ -7,6 +7,6 @@ module ProtectedTagAccess
   included do
     belongs_to :protected_tag
-    delegate :project, to: :protected_tag, allow_nil: true
+    delegate :project, to: :protected_tag, allow_nil: true, prefix: :protected_ref
   end
 end

ee/app/models/concerns/ee/protected_ref_access.rb

@@ -60,24 +60,6 @@ def type
       super
     end
-    override :humanize
-    def humanize
-      return humanize_user if user?
-      return humanize_group if group?
-
-      super
-    end
-
-    override :check_access
-    def check_access(current_user, current_project = project)
-      super do
-        break user_access_allowed?(current_user) if user?
-        break group_access_allowed?(current_user) if group?
-
-        yield if block_given?
-      end
-    end
-
     private
     def humanize_user
@@ -88,21 +70,26 @@ def humanize_group
       group&.name || 'Group'
     end
-    def user_access_allowed?(current_user)
-      current_user.id == user_id && project.member?(current_user)
+    # current_project is only available when evaluating a group-level protected
+    # branch. We only allow role based access levels at the group-level so we
+    # can ignore it here.
+    def user_access_allowed?(current_user, _current_project)
+      current_user.id == user_id && protected_ref_project.member?(current_user)
     end
-    def group_access_allowed?(current_user)
-      # For protected branches, only groups that are invited to the project
-      # can granted push and merge access. This feature does not work for groups
-      # that are ancestors of the project.
-      # See https://gitlab.com/gitlab-org/gitlab/-/issues/427486.
-      # Hence, we only consider the role of the user in the group limited by
-      # the max role of the project_group_link.
+    def group_access_allowed?(current_user, _current_project)
+      # For protected branches, only groups that are invited to the
+      # protected_ref_project can granted push and merge access. This feature
+      # does not work for groups that are ancestors of the
+      # protected_ref_project.  See
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/427486.  Hence, we only
+      # consider the role of the user in the group limited by the max role of
+      # the project_group_link.
       #
-      # We do not use the access level provided by direct membership to the project
-      # or inherited through ancestor groups of the project.
-      # See https://gitlab.com/gitlab-org/gitlab/-/issues/423835
+      # We do not use the access level provided by direct membership to the
+      # protected_ref_project or inherited through ancestor groups of the
+      # protected_ref_project.  See
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/423835
       return false unless group_has_developer_access_to_project?
@@ -119,10 +106,10 @@ def group?
     # We don't need to validate the license if this access applies to a role.
     #
-    # If it applies to a user/group we can only skip validation `nil`-validation
-    # if the feature is available
+    # If it applies to a user/group we can only skip validation
+    # `nil`-validation if the feature is available
     def user_or_group_assignable?
-      !role? && project&.feature_available?(:protected_refs_for_users)
+      !role? && protected_ref_project&.feature_available?(:protected_refs_for_users)
     end
     def user_and_group_not_assignable?
@@ -136,13 +123,13 @@ def validate_group_membership
     end
     def validate_user_membership
-      return if project.member?(user)
+      return if protected_ref_project.member?(user)
       errors.add(:user, 'is not a member of the project')
     end
     def group_has_developer_access_to_project?
-      project.project_group_links.exists?(group: group, group_access: DEVELOPER_OWNER_ACCESS)
+      protected_ref_project.project_group_links.exists?(group: group, group_access: DEVELOPER_OWNER_ACCESS)
     end
   end
 end

spec/support/shared_examples/models/concerns/protected_branch_access_shared_examples.rb

@@ -5,19 +5,19 @@
   it { is_expected.to belong_to(:protected_branch) }
-  describe '#project' do
+  describe '#protected_ref_project' do
     include_context 'for protected ref access'
-    it 'delegates project to protected_branch association' do
+    it 'delegates to protected_branch.project' do
       allow(protected_ref).to receive(:project)
-      described_class.new(protected_branch: protected_ref).project
+      described_class.new(protected_branch: protected_ref).protected_ref_project
       expect(protected_ref).to have_received(:project)
     end
     it 'does not error when protected_branch is nil' do
-      expect(described_class.new.project).to be_nil
+      expect(described_class.new.protected_ref_project).to be_nil
     end
   end

spec/support/shared_examples/models/concerns/protected_tag_access_shared_examples.rb

@@ -5,19 +5,19 @@
   it { is_expected.to belong_to(:protected_tag) }
-  describe '#project' do
+  describe '#protected_ref_project' do
     let_it_be(:protected_tag) { create(:protected_tag) }
-    it 'delegates project to protected_tag association' do
+    it 'delegates to protected_tag.project' do
       allow(protected_tag).to receive(:project)
-      described_class.new(protected_tag: protected_tag).project
+      described_class.new(protected_tag: protected_tag).protected_ref_project
       expect(protected_tag).to have_received(:project)
     end
     it 'does not error when protected_tag is nil' do
-      expect(described_class.new.project).to be_nil
+      expect(described_class.new.protected_ref_project).to be_nil
     end
   end
 end