Skip to content

Refactor to pass group access token to policy code

Radamanthus Batnag requested to merge 434291-refactor-pat-policy into master

Context

In !136655 (merged) we needed to add authorization code that checks if a token has the required scopes to access a group's dependency proxy. Because we only have a handle to the user and not the token in the policy code, we had to implement the authorization check not in a code inside app/policies, but instead in a service called by the controller, where we have a handle to the token.

Adding dependency proxy policy code that checks if a personal access token has the required scopes is hard because we pass the token user, and not the token itself, to the can?(...) check. Given a token user, it is impossible to get the token because there's a 1 to many association between users and personal access tokens.

This is not the case with a deploy token. With deploy tokens, we pass the token itself. With a handle to the token, we can write policy code that checks if the token has the required scopes, like:

app/policies/group_policy.rb#L409

  def valid_dependency_proxy_deploy_token
    @user.is_a?(DeployToken) && @user&.valid_for_dependency_proxy? && @user&.has_access_to_group?(@subject)
  end

app/models/deploy_token.rb#L57

  def valid_for_dependency_proxy?
    group_type? &&
      active? &&
      (Gitlab::Auth::REGISTRY_SCOPES & scopes).size == Gitlab::Auth::REGISTRY_SCOPES.size
  end

We can refactor how things are passed around, so that the actual personal access token is what is passed to dependency proxy policy code, not the personal access token user.

Why We Need to Refactor

We'll be adding new scopes for dependency proxy in #336800. This means we'll be doing more "Does this token have the required scopes" checks. We'll need this refactoring so that we can do the scope checks in policy code when we work on #336800.

Summary of Code Changes

The refactor will need to modify GroupPolicy which is used by many other services in GitLab. To minimize the risk of this refactor breaking things, dependency proxy policy changes have been moved to a dedicated policy class, DependencyProxy::GroupPolicy. The changes have also been gated behind a feature flag.

  • Modified DependencyProxy::GroupAccess:
    • If personal_access_token is present and the user is a human, a project bot, or a service account, pass personal_access_token instead of auth_user to the can?(...) call
    • Use DependencyProxy::GroupPolicy for the can?(...) call above

This is the heart of the refactor. All the code changes below are supporting changes to make the token accessible to the can?(...) call above.

  • Modified Auth::DependencyProxyAuthenticationService to include the PersonalAccessToken's token in the encoded JsonWebToken response
  • Modified the spec helper DependencyProxyHelpers#build_jwt to mirror the change in Auth::DependencyProxyAuthenticationService
  • Modified DependencyProxy::AuthTokenService to extract the token string, and return the matching PersonalAccessToken. Rename #user_or_deploy_token_from_jwt to #user_or_token_from_jwt
  • Modified Groups::DependencyProxy::ApplicationController#authenticate_user_from_jwt_token! to handle PersonalAccessTokens, not just Users and DeployTokens.

There's a lot in common between the code that checks a (human user) personal access token and a group access token. Group access tokens are, after all, currently implemented as a Personal Access Token. But this can change in the future, and so this MR kept different code paths for these two token types.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

NA

How to set up and validate locally

There should be no behavior changes. Dependency Proxy should work just like before with personal access tokens, group access tokens, and deploy tokens.

Test the following for human user personal access token, group access token, and deploy token.

docker logout http://gdk.test:3000
docker login gdk.test:3000 -p <token>
docker pull gdk.test:3000/flightjs/dependency_proxy/containers/alpine:latest

# Note down the Image ID of alpine:latest
docker images
# Delete alpine:latest so we can pull it again
docker rmi <image-id-of-alpine-latest>

We can also retest the validation steps for !136655 (merged)

Related to #434291

Edited by Radamanthus Batnag

Merge request reports