Added can_push_for_ref? method RUN AS-IF-FOSS
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_accessis aDeployKeyAccess, ie. when a deploy key pushes to a branch (first case highlighted above - change introduced by original MR). - when
user_accessis aUserAccess, 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
-
Protected branch has both the
No onerole and the deploy key mentioned in the.gitlab-ci.yml(see below) selected inAllowed to push. Rantest_push_commit: job succeeds. -
Protected branch has only
No onerole selected and the deploy key was removed from the list:test_push_commitjob fails as expected. -
Protected branch has only
No onerole. Ran new pipeline withCI_NEW_TAGenv. variable set tov0.10.0:test_push_tagsruns and tag is created successfully.
Context 2: MR from a forked project to be merged back into original project
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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)







