Move group_access_allowed logic to ProtectedRefAccess
What does this MR do and why?
Move group_access_allowed logic to ProtectedRefAccess
We fixed a security issue in
Unauthorized member can gain `Allowed to push a... (#423835 - closed) which allowed
project members with roles lower than Developer to push and merge
changes.
To fix this we created EE::ProtectedBranchAccess to override the
EE::ProtectedRefAccess#group_access_allowed?(current_user) method.
The new logic checks that the group has been invited to the project,
that the max access level for the group is at least Developer, and the
developer has at least Developer role within the group.
The security issue linked above only discussed the flaw in the context
of ProtectedBranch::PushAccessLevel and
ProtectedBranch::MergeAccessLevel records.
We can also assign groups to ProtectedBranch::UnprotectAccessLevel and
ProtectedTag::CreateAccessLevel. We should be using the same logic
when assessing a group members permissions for these.
Currently the UIs and APIs that allow users to unprotect protected branches and create protected tags are protected by our policies so we are not experiencing any problems with this logic but it should still be corrected.
Removing the additional EE::ProtectedBranchAccess module reduces
technical debt and also allows us to combine the RSpec shared_example
blocks for user and group access levels intoa single shared_example.
Related to branchRuleUpdate mutation bug - unexpected Acc... (#461213 - closed)
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.