Add managing protected branches as custom permission
What does this MR do and why?
It adds managing protected branches as a custom ability.
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
Screen_Recording_2024-08-12_at_18.09.50
How to set up and validate locally
- Create a new member group, that enables "manage protected branch" ability, for self-managed on admin - Roles and permissions page, eg. http://gdk.test:3000/admin/application_settings/roles_and_permissions
- Visit any group or project member pages such as
http://gdk.test:3000/groups/flightjs/-/group_members
and assign this custom role to a user - Impersonate this user and go to the project settings - repository page, eg. http://gdk.test:3000/flightjs/Flight/-/settings/repository
- Make sure you see Branch rules and Protected branches section
- Play around with protected branches
Related to #448823 (closed)
Merge request reports
Activity
changed milestone to %17.3
assigned to @jarka
added pipelinetier-1 label
- A deleted user
added backend documentation frontend labels
3 Warnings This merge request is quite big (877 lines changed), please consider splitting it into multiple merge requests. c7ec62ca: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 31e8f23d: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
-
doc/api/graphql/reference/index.md
(Link to current live version) -
doc/user/custom_roles/abilities.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Category Reviewer Maintainer backend @aluthra2
(UTC+5.5, 3.5 hours ahead of author)
@sgarg_gitlab
(UTC+5.5, 3.5 hours ahead of author)
frontend @ccharnolevsky
(UTC+3, 1 hour ahead of author)
@wortschi
(UTC+2, same timezone as author)
test for spec/features/*
@hmuralidhar
(UTC+10, 8 hours ahead of author)
Maintainer review is optional for test for spec/features/*
groupauthorization Reviewer review is optional for groupauthorization @alexbuijs
(UTC+2, same timezone as author)
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger-
added 164 commits
-
b3dd1b3c...68db2e0a - 163 commits from branch
master
- ace1aa02 - Add managing protected branches as custom permission
-
b3dd1b3c...68db2e0a - 163 commits from branch
- Resolved by Jarka Košanová
Security policy violations have been resolved.
Edited by GitLab Security Bot- Resolved by Scott de Jonge
Generated bygitlab_quality-test_tooling
.
Slow tests detected in this merge request. These slow tests might be related to this merge request's changes.Click to expand
Job File Name Duration Expected duration #7543683988 ee/spec/features/projects/protected_branches_spec.rb#L42
Protected Branches when logged in as maintainer behaves like setting project protected branches explicit protected branches allows creating explicit protected branches 53.4 s < 50.13 s #7598174063 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 85.65 s < 27.12 s #7601139991 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 95.3 s < 27.12 s #7607292087 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 78.96 s < 27.12 s #7609729647 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 74.04 s < 27.12 s #7628308947 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 86.3 s < 27.12 s #7636237476 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 85.52 s < 27.12 s #7689445111 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 77.14 s < 27.12 s #7728420910 spec/lib/release_highlights/validator_spec.rb#L82
ReleaseHighlights::Validator when validating all files they should have no errors 79.07 s < 27.12 s - A deleted user
added rspec:slow test detected label
mentioned in issue #448823 (closed)
- Resolved by Jarka Košanová
@hmehra could you do the initial & authorization review?
requested review from @hmehra
changed milestone to %17.4
added 280 commits
-
133c3bde...f0c30941 - 279 commits from branch
master
- c317272d - Add managing protected branches as custom permission
-
133c3bde...f0c30941 - 279 commits from branch
- Resolved by Hinam Mehra
- Resolved by Hinam Mehra
added 1 commit
- c1014a52 - Remove access to branch rules for manage prot branches custom ability
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits a4cb1501 and 327e41fb
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 4.33 MB 4.33 MB - 0.0 % mainChunk 3.27 MB 3.27 MB - 0.0 %
Note: We do not have exact data for a4cb1501. So we have used data from: d3c19f6c.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.Please look at the full report for more details
Read more about how this report works.
Generated by
Dangeradded 1 commit
- 99421abd - Remove access to branch rules for manage prot branches custom ability
- Resolved by Jarka Košanová
Thanks @hmehra , could you take another look?
requested review from @hmehra
added pipeline:mr-approved label
added pipelinetier-3 pipeline:run-e2e-omnibus-once labels and removed pipelinetier-1 label
Before you set this MR to auto-merge
This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.
Before you set this MR to auto-merge, please check the following:
- You are the last maintainer of this merge request
- The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
- This pipeline is recent enough (created in the last 8 hours)
If all the criteria above apply, please set auto-merge for this merge request.
See pipeline tiers and merging a merge request for more details.
requested review from @apennells, @hmerscher, and @digitalmoksha
removed review request for @hmehra
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 327e41fbexpand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Data Stores | 31 | 0 | 1 | 0 | 32 | ✅ | | Verify | 44 | 0 | 2 | 0 | 46 | ✅ | | Create | 128 | 0 | 16 | 0 | 144 | ✅ | | Govern | 71 | 0 | 0 | 0 | 71 | ✅ | | Plan | 73 | 0 | 0 | 0 | 73 | ✅ | | Fulfillment | 2 | 0 | 0 | 0 | 2 | ✅ | | Package | 20 | 0 | 12 | 0 | 32 | ✅ | | Release | 5 | 0 | 0 | 0 | 5 | ✅ | | Manage | 1 | 0 | 1 | 0 | 2 | ✅ | | Analytics | 2 | 0 | 0 | 0 | 2 | ✅ | | Monitor | 8 | 0 | 0 | 0 | 8 | ✅ | | Secure | 3 | 0 | 0 | 0 | 3 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 388 | 0 | 32 | 0 | 420 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for 99421abdexpand test summary
+-------------------------------------------------------------+ | suites summary | +--------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +--------+--------+--------+---------+-------+-------+--------+ | Govern | 107 | 0 | 6 | 0 | 113 | ✅ | | Create | 270 | 0 | 34 | 0 | 304 | ✅ | +--------+--------+--------+---------+-------+-------+--------+ | Total | 377 | 0 | 40 | 0 | 417 | ✅ | +--------+--------+--------+---------+-------+-------+--------+
removed pipeline:run-e2e-omnibus-once label
added 1 commit
- ae581a6d - Push frontend abilities from group repo settings controller
reset approvals from @hmehra by pushing to the branch
added 1 commit
- 5de5bef9 - Push frontend abilities from controllers using access_dropdown
added 1090 commits
-
5de5bef9...4adb323d - 1087 commits from branch
master
- 19f5d8d1 - Add managing protected branches as custom permission
- 5bbe8efc - Remove access to branch rules for manage prot branches custom ability
- ecc2abb5 - Push frontend abilities from controllers using access_dropdown
Toggle commit list-
5de5bef9...4adb323d - 1087 commits from branch
- Resolved by Alex Pennells
- Resolved by Alex Pennells
- Resolved by Alex Pennells
- Resolved by Jarka Košanová
- Resolved by Alex Pennells
- Resolved by Jarka Košanová
- Resolved by Alex Pennells
- Resolved by Jarka Košanová
- Resolved by Jarka Košanová
- Resolved by Jarka Košanová
@jarka LGTM, just a couple nits.
removed review request for @digitalmoksha
- Resolved by Jarka Košanová
added 1 commit
- cf723c81 - Small improvements, adding tests, fixing typos
- Resolved by Jarka Košanová
Thanks @apennells @digitalmoksha , I applied your suggestions / answered your comments.
requested review from @apennells
requested review from @digitalmoksha
removed review request for @digitalmoksha
- Resolved by Alex Pennells
mentioned in issue gitlab-com/www-gitlab-com#34897 (closed)
reset approvals from @digitalmoksha by pushing to the branch
- Resolved by Jarka Košanová
Thanks @apennells , I am back from PTO, could you take another look?
- Resolved by Hercules Merscher
removed review request for @hmerscher
requested review from @apennells
requested review from @sdejonge and removed review request for @apennells
added 1766 commits
-
04709324...8c287fc6 - 1760 commits from branch
master
- 3217cc5e - Add managing protected branches as custom permission
- 31e8f23d - Remove access to branch rules for manage prot branches custom ability
- 06d50d92 - Push frontend abilities from controllers using access_dropdown
- c7ec62ca - Small improvements, adding tests, fixing typos
- c18b6a93 - Use glAbilitiesMixin instead of gon
- 327e41fb - Improve specs for settings menu when custom ab taking effect
Toggle commit list-
04709324...8c287fc6 - 1760 commits from branch
removed review request for @sdejonge
mentioned in issue #481958
requested review from @hmehra
- Resolved by Hinam Mehra
@hmehra could you please give the final approval? It was reset due to some pushes
started a merge train
mentioned in commit fcff268a
mentioned in incident gitlab-org/quality/engineering-productivity/master-broken-incidents#8214 (closed)
added workflowstaging-canary label and removed workflowin review label
mentioned in merge request !164964 (merged)
mentioned in issue #482942 (closed)
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-production label and removed workflowproduction label
added releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!3354 (merged)
mentioned in merge request gitlab-com/www-gitlab-com!136283 (merged)
added releasedpublished label and removed releasedcandidate label
6 6 extend ActiveSupport::Concern 7 7 extend Grape::API::Helpers 8 8 9 def authorize_create_protected_branch! 10 authorize!(:create_protected_branch, user_project) 11 end 12 13 def authorize_update_protected_branch!(protected_branch) 14 authorize!(:update_protected_branch, protected_branch) 15 end 16 17 def authorize_destroy_protected_branch!(protected_branch) 18 authorize!(:read_protected_branch, protected_branch) @jarka I'm curious why this is
:read_protected_branch
and not:destroy_protected_branch
?@imand3r Hmm, that's a good question
I think this is a bug and should be fixed It is, however, not a vulnerability as theread_protected_branch
permission is assigned to maintainer of a group or owner of the project, same asdestroy_protected_branch
- this is project authorization.Never mind, I am going to submit merge request to fix this.
Edited by Jarka Košanová
mentioned in merge request !175876 (merged)
- Resolved by Joe Randazzo
Hi @jarka, thanks for working on this! Do we have any plans to break this down further into the individual (create, read, update, destroy) underlying permissions? One of my customers would be very interested in this.
mentioned in issue #515305