Authorize branch rules on read_branch_rule
What does this MR do and why?
Fix branchRules returning the "All branches" and "All protected branches" pseudo-rules to users who can't read them.
Types::Projects::BranchRuleType authorized on read_protected_branch, which passes through to the project. Users with the admin_protected_branch custom role have read_protected_branch, so they could see both pseudo-rules. The policies for those pseudo-rules deny read_branch_rule to that role. Only Maintainers and the manage_merge_request_settings custom role can read them.
This MR makes two changes:
BranchRuleTypenow authorizes onread_branch_rule.approvalRulesandexternalStatusCheckskeep their own field-level permissions (manage_merge_request_settingsandread_external_status_check).- The
branchRulesresolver checksread_branch_ruleon each pseudo-rule before it builds the page and drops any the user can't read. CE handles "All branches" and EE handles "All protected branches".
Why filter in the resolver as well as the type
GraphQL type authorization runs after the page is built. If we only filtered at type level we'd return short or empty pages. For example, first: 1 for an admin_protected_branch user returned an empty page because the only item was "All branches", and that item was then filtered out.
The resolver uses Ability.allowed? so a signed-out (nil) user is handled.
Request specs cover both custom roles (admin_protected_branch and manage_merge_request_settings) across every combination of persisted/unpersisted "All protected branches" rule and buildMissing: true/false. They also cover page size with first: 1 and a signed-out user.
This MR depends on !258973 (merged), which must merge first.
References
- #584313
- !258973 (merged) (dependency)
Screenshots or screen recordings
N/A, backend only change.
| Before | After |
|---|---|
How to set up and validate locally
1. Set up the project
- Create a group and a project inside it.
- Protect one branch in the project.
- Go to Settings > Merge requests and add an approval rule that targets all protected branches.
2. Create the custom role
- Create a custom role based on Guest with Manage Protected Branches (
admin_protected_branch). - Add a user to the project with that role.
3. Query as the admin_protected_branch user
-
Sign in as that user and open
/-/graphql-explorer. -
Run:
query { project(fullPath: "<path>") { branchRules(buildMissing: true) { nodes { name approvalRules { nodes { name } } externalStatusChecks { nodes { name } } } } } }
Expected: Only the protected branch rule is returned. approvalRules and externalStatusChecks are null.
4. Check page size
- Run the same query with
branchRules(buildMissing: true, first: 1).
Expected: One item is returned and it's the protected branch rule, not an empty page.
5. Query as a manage_merge_request_settings user
- Create a second custom role based on Guest with Manage merge request approvals and settings (
manage_merge_request_settings), and assign it to the user instead. - Run the query from step 3 again.
Expected: "All branches", "All protected branches" and the protected branch are all returned. approvalRules and externalStatusChecks return data instead of null, and "All protected branches" lists the approval rule from step 1.
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.