Scope policy approval rule name uniqueness to the policy rule

Alan (Maciej) Paruszewskirequested to merge
616205-policy-approval-rule-name-collision into master

What does this MR do and why?

Fixes a case where a recreated approval policy ends up with no project approval rule. Adds approval_policy_rule_id to the ApprovalProjectRule name uniqueness scope.

Related to #616205

Reviewable LOC: 1 (plus 27 in tests)

The issue stays open. I could not prove the reported link to bypass approvers (see the second block below).

Root cause

Deleting an approval policy only soft-deletes it. Its approval_project_rules stay until Security::DeleteSecurityPolicyWorker runs, which is async.

A policy recreated with the same name at the same index builds a project rule with the same name, rule type, policy configuration, orchestration_policy_idx and approval_policy_action_idx as the leftover rule. The name uniqueness validation on ApprovalProjectRule rejected it. Security::ScanResultPolicies::ApprovalRules::CreateService logged approval_rule_creation_failed at debug level only and moved on.

When the worker later removed the old rule, the project had no approval rule for the new policy. It stays that way until the next policy change.

approval_project_rules has no unique index on name, so the validation was the only thing blocking the insert. Scoping it to approval_policy_rule_id is enough.

What is not proven

The issue links this to bypass approvers set at creation on a project that already has approval rules. I found no code path where that changes enforcement. Both sync paths ignore bypass_settings, and enforcement reads it live from the policy content.

Best guess: the reporter's second scenario deleted and recreated the policy with the same name. That is why this MR says "Related to", not "Closes". The issue should stay open until support confirms.

Context for LLM agents

Invariant: the uniqueness scope for policy-sourced project rules must include approval_policy_rule_id. If the merge request rule index is rebuilt, it should match.

Rejected alternatives:

  • Same scope change on ApprovalMergeRequestRule. The DB unique index idx_unique_approval_mr_rules_for_scan_result_policy (merge_request_id, name, rule_type, security_orchestration_policy_configuration_id, orchestration_policy_idx, approval_policy_action_idx, WHERE report_type IN (2, 4, 5)) enforces the old scope. Changing only the validation would turn a handled RecordInvalid into an unhandled RecordNotUnique. It needs an index migration, so it is deferred to a follow-up issue.
  • Deleting or adopting the stale rule inside CreateService. More code, and it races the delete worker.
  • A feature flag. Not added. The change only narrows a model validation.

References

Screenshots or screen recordings

Not applicable. Backend-only change, no UI.

How to set up and validate locally

Run the new spec. It failed before the fix with name: ["has already been taken"] and passes after.

bundle exec rspec ee/spec/services/security/scan_result_policies/approval_rules/create_service_spec.rb -e "when a deleted policy at the same index left a same-name approval rule behind"
Test runs
  • create_service_spec plus both approval rule model specs: 283 examples, 0 failures.
  • Earlier broader run (approval rules services, sync_report_approver_approval_rules, delete_security_policy_worker, sync_project_service): 400 examples, 0 failures.
  • RuboCop clean.

Owed elsewhere

  • Merge request approval rules have the same collision, but need an index migration first: #631830

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports