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_access
is aDeployKeyAccess
, ie. when a deploy key pushes to a branch (first case highlighted above - change introduced by original MR). - when
user_access
is 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 one
role 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 one
role selected and the deploy key was removed from the list:test_push_commit
job fails as expected. -
Protected branch has only
No one
role. Ran new pipeline withCI_NEW_TAG
env. variable set tov0.10.0
:test_push_tags
runs 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)