Add validation to approvals_required in scan result policies
Description
We have a maximum value of 100 for approvals_required
in project approval rules. But we don't have any validation on approvals_required
for scan result policy. Because of this, approval rules could not be created on merge requests if the validation fails.
Implementation Plan
As a part of this issue, we want to do 3 things:
-
backend Update the validation of approvals_required
in policy (Both rule mode and yaml mode) to check if max is 100 -
backend Surface the validation error in ApprovalRules::CreateService
toSecurity::SecurityOrchestrationPolicies::ProcessScanResultPolicyService
-
backend Don't push to audit events if the approval rule create/update is failed -
documentation Update approval rules documentation to reflect the limit
Slack Thread (internal):
alan > :waves: I've been testing Scan Result Policies recently and I've noticed that for new project with new security policy project Approval Rules are not applied :thinking_face: Could someone help me understand why? :thinking_face: https://gitlab.com/gitlab-org/govern/demos/sandbox/fail-closed-test/-/merge_requests/3
Sashi > Looks like the approval rules are created on project level but it does not apply to MRs :thinking_face: I'll look into this once I get access rails console
Sashi > There are no failures from logs :thinking_face:
alan > Interesting, approval rules for MRs are created automatically from project approval rules, this means that this is probably not related to any changes that we have introduced recently :thinking_face:
Sashi > I just looked in to the DB and found the project did not have approval rules created. We have a validation for approvals_required to be a maximum of 100 and our rule seems to have 1000 I just changed to 99 and now its working fine. There are 3 things we have to improve:
- Update the validation of approvals_required in policy (Both rule mode and yaml mode) to check if max is 100
- Surface the validation error in ApprovalRules::CreateService to ProcessScanResultPolicyService
- Don't push to audit events if the approval rule create/update is failed
Martin Čavoj > Nice catch! Should we also update the documentation with this restriction? https://docs.gitlab.com/ee/user/project/merge_requests/approvals/rules.html#add-an-approval-rule
Sashi > Good catch! We have inconsistency in docs. Have created an issue for this: https://gitlab.com/gitlab-org/gitlab/-/issues/409469
Grant Hickman > We have a validation for approvals_required to be a maximum of 100 and our rule seems to have 1000 I just changed to 99 and now its working fine. Does this mean that if any customers experienced the same bug, this would now be resolved for them as well? Or is there any further action? Or was this addressed for a single project? I'm not 100% clear on the impact.
Sashi > No I changed the value to 99 only for the test project. The bug still exists. https://gitlab.com/gitlab-org/gitlab/-/issues/409469 would fix the bug
Grant Hickman > Ah ok, thanks for that. Is there a way to know how many users are affected? Would this be the case for all new policies being created?
alan > We would need to check this. Most probably, not many users are affected, as it makes practically no sense to require approval from 100+ people in the MR :thinking_face:
Would this be the case for all new policies being created?
This affects all policies (old and new).
Sashi > I just did a check by getting all policies and found no policies with approvals_required > 100
Grant Hickman > That makes sense. Sometimes bugs manifest and impact customers in unexpected ways so I wanted to confirm (such as projects pending deletion impacting subsequent projects). Looks like this is lower priority :+1:
Edited by Martin Čavoj