Skip to content

Stop deleting merged MRs merge request approval rules when deleting a scan policy

Summary

When testing the new approval rules work I noticed that when we delete a security rule we also delete all the approval rules related to it, regardless of if the merge request is merged or not. This is a problem because we should keep the approval rules post merge for compliance reasons https://gitlab.com/gitlab-org/gitlab/-/blob/fa6287ba9f7feaba94127a8ef9a494600ce0a067/ee/app/models/concerns/security/scan_result_policy.rb#L34

Steps to reproduce

Example Project

What is the current bug behavior?

When Scan Result Policy is updated or deleted MR approval rules are deleted for merged MRs.

What is the expected correct behavior?

When Scan Result Policy is updated or deleted MR approval rules should not be deleted for merged MRs.

Relevant logs and/or screenshots

Output of checks

Results of GitLab environment info

Expand for output related to GitLab environment info 

(For installations with omnibus-gitlab package run and paste the output of:
`sudo gitlab-rake gitlab:env:info`)

(For installations from source run and paste the output of:
`sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)

Results of GitLab application Check

Expand for output related to the GitLab application check 
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true)


(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true)


(we will only investigate if the tests are passing)

Possible fixes

      def delete_scan_finding_rules
        delete_in_batches(approval_merge_request_rules.for_unmerged_merge_requests)
        delete_in_batches(approval_project_rules)
      end
  • ApprovalMergeRequestRule has foreign key on scan_result_policy_id. We delete and recreate ScanResultPolicyRead whenever policy is updated, so deleting them would remove ApprovalMergeRequestRule too. So scan_result_policy_id should be nullified in approval_merge_request_rules for merged MRs.

Verification steps

  1. Create a new project
  2. Make sure the project has at least one other member with developer access
  3. Add a .gitlab-ci.yml file with the content
include:
  - template: Jobs/Dependency-Scanning.gitlab-ci.yml
  1. Add an empty Gemfile.lock file
  2. Go to Secure > Policies.
  3. Click on New Policy.
  4. Select Scan result policy.
  5. Change to yaml mode and copy the yaml content below, adjusting the user_approvers_ids as needed:
type: scan_result_policy
name: Deny new MIT licensed dependencies
description: 
enabled: true
actions:
  - type: require_approval
    approvals_required: 1
    user_approvers_ids:
      - 49
rules:
  - type: license_finding
    match_on_inclusion: true
    license_types:
      - MIT
    license_states:
      - newly_detected
    branch_type: protected

  1. Click on Configure with a merge request

  2. Merge the new MR to add the new policy.

  3. Create a new MR

  4. Check the approval rules in the database. Adjust the target_project_id and iid according to your project and MR:

SELECT id, name, scan_result_policy_id FROM approval_merge_request_rules WHERE merge_request_id = (SELECT id FROM merge_requests WHERE target_project_id = 1 AND iid = 1);

Check the id of approval_merge_request_rules linked to a scan_result_policy.

    id     |                name                | scan_result_policy_id 
-----------+------------------------------------+-----------------------
 1 | Deny new MIT licensed dependencies |              17

  1. Merge the MR

  2. Make sure the MR is marked as merged in the database. The state_id should 3(merged)

SELECT state_id FROM merge_requests WHERE target_project_id = 1 AND iid = 1;
 state_id 
----------
        3
  1. Go to Secure > Policies.
  2. Update the policy description
  3. Verify the approvals for the merged MR was not deleted and the previous linked scan_result_policy was nullified.
    id     |                name                | scan_result_policy_id 
-----------+------------------------------------+-----------------------
 1 | Deny new MIT licensed dependencies |
Edited by Marcos Rocha