Skip to content

Move membership check when authenticated with deploy key

Joe Woodward requested to merge chore/461213-something into master

What does this MR do and why?

Move membership check when authenticated with deploy key

We fixed a bug in Prevent existing undeleted user access to retur... (!132380 - merged) which allowed owners of a deploy key to access projects that they were no longer members of.

This exploit consisted of gaining membership to a project, uploading a deploy key, removing the membership, then pushing to the project using the deploy key.

This exploit was possible because we only checked that the deploy key was active and not that the owner was a member during the checks flow.

To fix this we have added a condition in the ProtectedRefAccess#check_access method to check if the user is a project member.

While this does fix the bug, the issue is specific to deploy keys so doesn't really make sense to check membership for role, user, or group based access levels.

  • For role based levels we check the users max membership within the project's team so we are already checking they are a member.
  • For user based levels we don't want to check if the user is a member as non-members should also be allowed to perform the actions. We exit early so this doesn't affect those access levels.
  • For groups we don't want check if the user is a member of the project, we check if the group has access to the project and if the user is a member of the group. Again we exit early so this doesn't affect group access levels.

This change moves the membership check to the correct location. Now we will only check if the user is a project member when we are checking deploy_key based access levels.

Related to branchRuleUpdate mutation bug - unexpected Acc... (#461213)

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

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Merge request reports