Skip to content

Reorder protected ref access check for deploy keys

Joe Woodward requested to merge chore/468689-reorder-sequence-for-perf into master

What does this MR do and why?

Closes Reorder protected_ref_access checks (#468689)

Reorder protected ref access check for deploy keys

This change improves performance by rearranging the conditions for deploy keys to have access to a project.

Prior to this the enabled_deploy_key_for_user? method called current_user.can?(:read_project, project) first, however, this call fires db queries to get a result. The following condition checks if deploy_key.user_id == current_user.id, both of which are loaded already.

If the deploy_key does not belong to the current user then there's no need loading the associations required for :read_project.

Also included is a small refactor to improve readability. The enabled_deploy_key_for_user? method is now named deploy_key_access_allowed?. The deploy_key_access_allowed? method is also refactored to name the actions that are happening.

Previously:

    current_user.can?(:read_project, project) &&
      deploy_key.user_id == current_user.id &&
      deploy_key_owner_project_member? &&
      deploy_key_has_write_access_to_project?

Now with change in order:

    deploy_key.user_id == current_user.id &&
      current_user.can?(:read_project, project) &&
      deploy_key_owner_project_member? &&
      deploy_key_has_write_access_to_project?

After refactoring:

    deploy_key_owned_by?(current_user) && valid_deploy_key_status?

This is much clearer and shows the intention.

No spec changes as no behavior has changed.

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

Loading