Remove stricter checks when authorizing for Dependency Proxy
🌱 Context
!141358 (merged) inadvertently introduced stricter checks on personal access tokens, behind the feature flag packages_dependency_proxy_pass_token_to_policy
. When packages_dependency_proxy_pass_token_to_policy
was enabled for everyone, we received reports of issues from users who use Dependency proxy within a pipeline triggered by a group access token.
What does this MR do and why?
!141358 (merged) is supposedly just a refactor and not meant to introduce new behavior.
This MR reverts some changes introduced by !141358 (merged) so that the code behind the feature flag
packages_dependency_proxy_pass_token_to_policy
is as close as possible to the old code.
Summary
User
or a DeployToken
from the JWT token
- Additional specs in
spec/policies/packages/policies/dependency_proxy/group_policy_spec.rb
. These were specs we had for the old policy but weren't added to the specs for the new policy. Without the policy changes in this MR, these specs fail. - Additional feature specs in
spec/features/groups/dependency_proxy_for_containers_spec.rb
. These specs pass with or without the changes in1️⃣ , but I added it here just to increase our confidence in the code.
I wasn't able to replicate the reported errors locally, but at least with the additional tests we'll have more confidence in the changes.
Details
condition(:dependency_proxy_access_allowed) do
access_level(for_any_session: true) >= GroupMember::GUEST || valid_dependency_proxy_deploy_token
end
The new policy also checks deploy token validity. The implementation of the old and the new policies are the same
desc "Personal access or group access token with read access to dependency proxy"
condition(:read_dependency_proxy_personal_access_token) do
user_is_personal_access_token? &&
(
user.user.human? ||
user.user.service_account? ||
(user.user.project_bot? && user.user.resource_bot_resource.is_a?(::Group))
) &&
(access_level(for_any_session: true) >= GroupMember::GUEST)
end
This MR changes the condition above to:
desc "Personal access or group access token with read access to dependency proxy"
condition(:read_dependency_proxy_personal_access_token) do
!deploy_token_user? && (access_level(for_any_session: true) >= GroupMember::GUEST)
end
We need to add the !deploy_token_user?
check, otherwise, deploy tokens without the required scopes will pass the condition.
User
or a DeployToken
from the JWT token
if token_payload['user_id']
User.find(token_payload['user_id'])
elsif token_payload['deploy_token']
DeployToken.active.find_by_token(token_payload['deploy_token'])
end
if token_payload['personal_access_token']
get_personal_access_token(token_payload['personal_access_token'])
elsif token_payload['group_access_token']
# a group access token is a personal access token in disguise
get_personal_access_token(token_payload['group_access_token'])
elsif token_payload['deploy_token']
get_deploy_token(token_payload['deploy_token'])
elsif token_payload['user_id']
get_user(token_payload['user_id'])
end
This MR changes the above code to check for the user first before the deploy token, just like with the old code:
if token_payload['personal_access_token']
get_personal_access_token(token_payload['personal_access_token'])
elsif token_payload['group_access_token']
# a group access token is a personal access token in disguise
get_personal_access_token(token_payload['group_access_token'])
elsif token_payload['user_id']
get_user(token_payload['user_id'])
elsif token_payload['deploy_token']
get_deploy_token(token_payload['deploy_token'])
end
We still need to keep the checks for personal access tokens and group access tokens first, otherwise these will be treated as a User
and not as tokens, which is the point for the refactor in !141358 (merged).
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
No UI changes
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, deploy tokens, and job tokens.
Test the following for human user personal access token, group access token, deploy token, and job token. Test with the feature flag packages_dependency_proxy_pass_token_to_policy
enabled and disabled.
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>
Related to #459991