Skip to content

Added can_push_for_ref? method RUN AS-IF-FOSS

Etienne Baqué requested to merge 30769-fix-call-to-push-check into master

What does this MR do?

This MR addresses the root cause of the bug that triggered a regression yesterday. That regression was introduced after this backend MR was merged in (let's call it the original MR).

In the original MR, this change was the root cause of the bug:

user_access.can_push_to_branch?(ref) was introduced. This makes sense in the case of a deploy key pushing to a branch: the issue linked to the original MR says that a deploy key can push to a protected branch, when it is selected in the protected branch configuration, when the deploy key owner is only a guest or a reporter. From the issue proposal:

Deploy keys are able to push to protected branch if the owner does not have 
permission to do, but does have access to the project.

Given that new requirement, it made sense to replace that line user_access.can_do_action?(:push_code) then. And this is where the regression kicked in: this line removal was done to the detriment of checking if a user could push to a repository, ie. if that user was a member of that project as a Developer or above. That's what users reported in the regression issue.

About the long term fix:

Having only user_access.can_push_to_branch?(ref) for both context is the mistake. We need a custom push_check in the following two contexts:

  • when user_access is a DeployKeyAccess, ie. when a deploy key pushes to a branch (first case highlighted above - change introduced by original MR).
  • when user_access is a UserAccess, ie. second case above, which is what we had before introduction of the bug via original MR (user_access.can_do_action?(:push_code)).

This is what this MR does. In very short, it's about having the old AND the new line of code, not the new over the old one.

Screenshots (strongly suggested)

As manual testing, I reproduced a scenario that was carried out in the original MR, as well as the scenario to test the regression fix:

Scenario 1: Deploy key pushing to a protected branch

  1. Protected branch has both the No one role and the deploy key mentioned in the .gitlab-ci.yml (see below) selected in Allowed to push. Ran test_push_commit: job succeeds.

    Screenshot_from_2020-12-10_17-06-34

    Screenshot_from_2020-12-10_17-13-24

    Screenshot_from_2020-12-10_17-13-50

  2. Protected branch has only No one role selected and the deploy key was removed from the list: test_push_commit job fails as expected.

    Screenshot_from_2020-12-10_17-53-05

  3. Protected branch has only No one role. Ran new pipeline with CI_NEW_TAG env. variable set to v0.10.0: test_push_tags runs and tag is created successfully.

    Screenshot_from_2020-12-10_17-20-06_tag

    Screenshot_from_2020-12-10_17-54-14

Context 2: MR from a forked project to be merged back into original project

  1. A MR is created from forked project. We manage to merge it back to the original project's master.

    Screenshot_from_2020-12-10_17-58-52

    Screenshot_from_2020-12-10_17-26-32

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Related to #30769 (closed)

Edited by Thong Kuah

Merge request reports