Refactor to pass group access token to policy code
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, passpersonal_access_token
instead ofauth_user
to thecan?(...)
call - Use
DependencyProxy::GroupPolicy
for thecan?(...)
call above
- If
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 thePersonalAccessToken
's token in the encoded JsonWebToken response - Modified the spec helper
DependencyProxyHelpers#build_jwt
to mirror the change inAuth::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 handlePersonalAccessToken
s, not justUser
s andDeployToken
s.
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 (closed)