[go: up one dir, main page]

Skip to content

Refactor ProtectedRef logic to remove the inherited nature of `#check_access`

The following discussion from !156664 (merged) should be addressed:

  • @jwoodwardgl started a discussion: (+3 comments)

    After reading through these comments I have re-read the code and realised that it's currently quite confusing due to us overriding the check_access method and using

    super do
      ...
      yield if block_given?
    end

    At the moment we don't have do not have clear separation between the access level types. We could make the lines less blurry by removing all of the overrides and using the access_level.type to call the correct method e.g.

      def check_access(current_user, current_project = project)
        return false if current_user.nil? || no_access?
        return current_user.admin? if admin_access?
    
        # 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 -- metaprogramming
      end

    Then we just implement the access_allowed? methods for each type. I think we should also rename check_access as it's really a boolean method. I think it would make more sense to name it access_allowed?(current_user, current_project = project).

    This feels like multiple MRs so will look into splitting these changes.