Skip to content
Snippets Groups Projects

Remove admin override for ProtectedRef Access

Merged Joe Woodward requested to merge refactor/protected_ref_access-check_access into master
All threads resolved!

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)

Edited by Joe Woodward

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Patrick Cyiza approved this merge request

    approved this merge request

  • Patrick Cyiza requested review from @ck3g

    requested review from @ck3g

  • :wave: @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:

  • Joe Woodward added 861 commits

    added 861 commits

    Compare with previous version

  • Joe Woodward added 1 commit

    added 1 commit

    • edab23e1 - Remove admin override for ProtectedRef Access

    Compare with previous version

  • Joe Woodward changed the description

    changed the description

  • Author Maintainer

    @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 from Projects::JobsController#erase and Api::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 Woodward
  • Joe Woodward requested review from @stanhu

    requested 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.

  • Stan Hu approved this merge request

    approved this merge request

  • Stan Hu removed review request for @stanhu

    removed review request for @stanhu

  • assigned to @cmaxim

  • Joe Woodward requested review from @cmaxim

    requested review from @cmaxim

  • unassigned @cmaxim

  • Patrick Cyiza removed review request for @jpcyiza

    removed review request for @jpcyiza

  • Vitali Tatarintev approved this merge request

    approved this merge request

  • Costel Maxim approved this merge request

    approved this merge request

  • Robert May approved this merge request

    approved this merge request

  • Robert May resolved all threads

    resolved all threads

  • Robert May requested review from @robotmay_gitlab

    requested review from @robotmay_gitlab

  • Documentation is updated in !117737 (merged)

  • Robert May enabled an automatic merge when the pipeline for b4994558 succeeds

    enabled an automatic merge when the pipeline for b4994558 succeeds

  • Amy Qualls requested review from @aqualls

    requested review from @aqualls

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for edab23e1

    expand 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: :x: test report for edab23e1

    expand test summary
    +-------------------------------------------------------------+
    |                       suites summary                        |
    +--------+--------+--------+---------+-------+-------+--------+
    |        | passed | failed | skipped | flaky | total | result |
    +--------+--------+--------+---------+-------+-------+--------+
    | Create | 661    | 1      | 80      | 61    | 742   | ❌     |
    +--------+--------+--------+---------+-------+-------+--------+
    | Total  | 661    | 1      | 80      | 61    | 742   | ❌     |
    +--------+--------+--------+---------+-------+-------+--------+
  • Amy Qualls approved this merge request

    approved this merge request

  • Robert May resolved all threads

    resolved all threads

  • merged

  • Robert May mentioned in commit 268656d4

    mentioned in commit 268656d4

  • Amy Qualls mentioned in merge request !119564 (merged)

    mentioned in merge request !119564 (merged)

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue #409722 (closed)

  • mentioned in issue #12776 (closed)

  • Jay McCure mentioned in merge request !119629 (merged)

    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 #410822 (closed)

  • mentioned in issue #428273

  • Please register or sign in to reply
    Loading