Apply codeowner validations to web requests
Background
Originally, the security vulnerability was closed in master with https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/346.
However, at least one large customer on GitLab.com uses CODEOWNERS
that references ancestor groups of the project. When the above MR was deployed, the customer's workflow became blocked. We did not realize that this was "expected" due to the unresolved #32432 (closed). So, we reverted the security fix to unblock the customer.
We attempted to fix the CODEOWNERS
logic https://gitlab.com/gitlab-org/gitlab/-/issues/216345#note_336154279, however we mistakenly only fixed it for the case where the project's parent group was referenced in CODEOWNERS
. So after rereverting the security fix, the customer was blocked again.
The security fix and the CODEOWNERS
change were reverted.
There were still unanswered questions regarding how the CODEOWNERS
logic should work in these cases. It's being worked on, but it cannot be rushed.
Update: !31152 (merged) was merged. We expect it to fix the majority of customer cases where CODEOWNERS
was unexpectedly blocking Merge via the UI.
What does this MR do?
This MR reapplies the original security fix but adds a feature flag to let you skip this fix for specific projects, or for all. This helps us rollout the fix carefully for known previously affected customers, and allows self-hosted customers to skip validations on the Merge button on the web UI if they really want to.
The additional feature flag logic sounds trivial but it is easy to make a mistake. Reviewers, please scrutinize it. Unit tests of the affected method are now tested with the feature flag on and off.
- We can skip (or not skip) these validations for everyone, at a moment's notice
- We can skip these validations for specific projects, if customers find a new edge case of
CODEOWNERS
blocking them, and workarounds are not feasible
Rollout plan
https://gitlab.com/gitlab-org/gitlab/-/issues/216345#note_340599018:
The feature flag is now
skip_web_ui_code_owner_validations
to make it easy to skip it for specific customers if needed.Rollout plan:
Enable skip_web_ui_code_owner_validations
feature flag on GitLab.com for all => https://gitlab.slack.com/archives/CK75EF2A2/p1589258447358900Merge/deploy this MR (to no effect) Disable skip_web_ui_code_owner_validations
feature flag on GitLab.com for allEnable skip_web_ui_code_owner_validations
feature flag on GitLab.com for known previously affected customers (because we want extreme care for these cases at first) (would it work to do this before the previous step?)Validate CODEOWNERS
logic + enforcement in web UI on GitLab.comWait ~2 days Search for issues Disable skip_web_ui_code_owner_validations
feature flag on GitLab.com for allRemove feature flag #217427 (closed)
Does this MR meet the acceptance criteria?
Conformity
- [-] Changelog entry
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
-
Security reports checked/validated by a reviewer from the AppSec team