Remove admin override for ProtectedRef Access
What does this MR do and why?
Remove admin override for ProtectedRef Access
Removes admin override for ProtectedBranch push ProtectedBranch merge ProtectedBranch unprotect ProtectedTag create
Related to #12776 (closed)
Merge request reports
Activity
changed milestone to %16.0
added backend devopscreate sectiondev typefeature + 1 deleted label
assigned to @jwoodwardgl
added Category:Source Code Management groupsource code labels
1 Warning This MR changes code in ee/
, but its Changelog commit is missing theEE: true
trailer. Consider adding it to your Changelog commits.Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Tarun Vellishetty (
@tvellishetty
) (UTC+5.5, 4.5 hours ahead of@jwoodwardgl
)Vitali Tatarintev (
@ck3g
) (UTC+2, 1 hour ahead of@jwoodwardgl
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded 1 commit
- 517555ec - Remove admin override for ProtectedRef Access
added 209 commits
-
517555ec...7565bc6a - 208 commits from branch
master
- 69d84c03 - Remove admin override for ProtectedRef Access
-
517555ec...7565bc6a - 208 commits from branch
added 1 commit
- 80f8f665 - Remove admin override for ProtectedRef Access
added 1 commit
- 3f30a3e9 - Remove admin override for ProtectedRef Access
1 Warning This MR changes code in ee/
, but its Changelog commit is missing theEE: true
trailer. Consider adding it to your Changelog commits.Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend David Dieulivol (
@ddieulivol
) (UTC+2, 1 hour ahead of@jwoodwardgl
)Terri Chu (
@terrichu
) (UTC-4, 5 hours behind@jwoodwardgl
)~"Verify" Reviewer review is optional for ~"Verify" Drew Cimino (
@drew
) (UTC+0, 1 hour behind@jwoodwardgl
)~"Verify backend" Reviewer review is optional for ~"Verify backend" Drew Cimino (
@drew
) (UTC+0, 1 hour behind@jwoodwardgl
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded 1 commit
- 444aa9cf - Remove admin override for ProtectedRef Access
added 1 commit
- b040eb81 - Remove admin override for ProtectedRef Access
added 1 commit
- 75e2373e - Remove admin override for ProtectedRef Access
- Resolved by Robert May
requested review from @jpcyiza
- Resolved by Joe Woodward
requested review from @ck3g
@jpcyiza
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
added 861 commits
-
75e2373e...74b138f8 - 860 commits from branch
master
- 0eed7506 - Remove admin override for ProtectedRef Access
-
75e2373e...74b138f8 - 860 commits from branch
added 1 commit
- edab23e1 - Remove admin override for ProtectedRef Access
@stanhu could you review this change to the ci/build_policy please?
We've changed the protected ref access logic to prevent admin users from being able perform actions on protected refs unless they have been explicitly allowed to do so. These actions include pushing and merging into a protected branch, unprotecting a branch, and creating protected tags.
We also allow admins to erase CI builds artefacts and traces (
CI::BulidEraseService
, called fromProjects::JobsController#erase
andApi::Ci::Jobs(post ':id/jobs/:job_id/erase')
). The policy for CI builds uses ProtectedTag and ProtectedBranch to determine if a build can be erased. The current behavior allows admins to erase builds regardless of protection status, while preventing non-admin users from erasing builds that are for protected branches or tags.I have modified the policy so this behavior remains intact.
Edited by Joe Woodwardrequested review from @stanhu
- Resolved by Amy Qualls
@jwoodwardgl While this change makes sense to me, I wonder how many workflows depend on the current access permissions. On GitLab.com, we shouldn't ever see admins bypassing permissions, but I do wonder on self-managed hosts whether users have automations set up that would break as a result of this change.
It sounds like we're probably okay making this change in 16.0, though we may need to document this very clearly and explain how to fix worfklows that depended on the previous behavior.
- Resolved by Costel Maxim
I think this change should probably be reviewed also by @gitlab-com/gl-security/appsec in case it has other implications.
removed review request for @stanhu
assigned to @cmaxim
requested review from @cmaxim
unassigned @cmaxim
removed review request for @jpcyiza
requested review from @robotmay_gitlab
Documentation is updated in !117737 (merged)
enabled an automatic merge when the pipeline for b4994558 succeeds
requested review from @aqualls
added Technical Writing documentation labels
Allure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for edab23e1expand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Manage | 1 | 0 | 0 | 0 | 1 | ✅ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Create | 8 | 0 | 1 | 0 | 9 | ✅ | | Plan | 4 | 0 | 0 | 0 | 4 | ✅ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | | Govern | 2 | 0 | 0 | 0 | 2 | ✅ | | Data Stores | 2 | 0 | 0 | 0 | 2 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 21 | 0 | 2 | 0 | 23 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for edab23e1expand test summary
+-------------------------------------------------------------+ | suites summary | +--------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +--------+--------+--------+---------+-------+-------+--------+ | Create | 661 | 1 | 80 | 61 | 742 | ❌ | +--------+--------+--------+---------+-------+-------+--------+ | Total | 661 | 1 | 80 | 61 | 742 | ❌ | +--------+--------+--------+---------+-------+-------+--------+
added docsfeature tw-weight1 twdoing labels
mentioned in commit 268656d4
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
mentioned in merge request !119564 (merged)
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
mentioned in issue #409722 (closed)
mentioned in issue #12776 (closed)
mentioned in issue gitlab-org/quality/pipeline-triage#196 (closed)
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
mentioned in merge request !119629 (merged)
One of the E2E tests is failing because it was (mistakenly) pushing to a different user's protected branch as an admin. Test fix is: !119629 (merged)
mentioned in issue gitlab-org/quality/pipeline-triage#197 (closed)
added releasedcandidate label
mentioned in issue #410822 (closed)
mentioned in merge request kubitus-project/kubitus-installer!2145 (merged)
mentioned in merge request gitlab-com/www-gitlab-com!122232 (merged)
mentioned in merge request gitlab-com/www-gitlab-com!122228 (merged)
mentioned in issue #428273