Skip to content

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

1️⃣ Remove stricter checks

2️⃣ Revert the order when deciding to return either a User or a DeployToken from the JWT token

3️⃣ Additional specs

  • 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 in 1️⃣, 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

1️⃣ Remove stricter checks

Old policy checks

  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

New policy checks

        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.

2️⃣ Revert the order when deciding to return either a User or a DeployToken from the JWT token

Old order

      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

New order

      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

Edited by Radamanthus Batnag

Merge request reports