Merge branch 'sk/395701-fix-license-approval-for-non-default-branch' into 'master'

Fix license approval policies to consider non-default branches

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114459



Merged-by: Roy Zwambag <rzwambag@gitlab.com>
Approved-by: Dominic Bauer <dbauer@gitlab.com>
Approved-by: Roy Zwambag <rzwambag@gitlab.com>
Co-authored-by: Sashi Kumar <skumar@gitlab.com>

ee/app/services/security/sync_license_scanning_rules_service.rb

@@ -39,16 +39,22 @@ def sync_license_check_rule(merge_requests)
     end
 
     def sync_license_finding_rules(merge_requests)
-      license_approval_rules = ApprovalMergeRequestRule
+      merge_requests.each do |merge_request|
+        remove_required_license_finding_approval(merge_request)
+      end
+    end
+
+    def remove_required_license_finding_approval(merge_request)
+      license_approval_rules = merge_request
+        .approval_rules
         .report_approver
         .license_scanning
         .with_scan_result_policy_read
         .including_scan_result_policy_read
-        .for_unmerged_merge_requests(merge_requests)
 
       return if license_approval_rules.empty?
 
-      denied_rules = license_approval_rules.reject { |rule| violates_policy?(rule) }
+      denied_rules = license_approval_rules.reject { |rule| violates_policy?(merge_request, rule) }
       ApprovalMergeRequestRule.remove_required_approved(denied_rules)
     end
 
@@ -57,11 +63,12 @@ def sync_license_finding_rules(merge_requests)
     ##     with license type violating the policy.
     ##   - If match_on_inclusion is false, any detected licenses that does not match
     ##     the licenses from `license_types` should require approval
-    def violates_policy?(rule)
+    def violates_policy?(merge_request, rule)
       scan_result_policy_read = rule.scan_result_policy_read
       check_denied_licenses = scan_result_policy_read.match_on_inclusion
+      target_branch_report = target_branch_report(merge_request)
 
-      license_ids, license_names = licenses_to_check(scan_result_policy_read)
+      license_ids, license_names = licenses_to_check(target_branch_report, scan_result_policy_read)
       license_policies = project
         .software_license_policies
         .including_license
@@ -78,24 +85,25 @@ def violates_policy?(rule)
         violates_license_policy = (license_names_from_ids - license_names_from_policy).present?
       end
 
-      return true if scan_result_policy_read.newly_detected? && new_dependency_with_denied_license?(denied_licenses)
+      return true if scan_result_policy_read.newly_detected? &&
+        new_dependency_with_denied_license?(target_branch_report, denied_licenses)
 
       violates_license_policy
     end
 
-    def licenses_to_check(scan_result_policy_read)
+    def licenses_to_check(target_branch_report, scan_result_policy_read)
       only_newly_detected = scan_result_policy_read.license_states == [ApprovalProjectRule::NEWLY_DETECTED]
 
       if only_newly_detected
-        diff = default_branch_report.diff_with(report)
+        diff = target_branch_report.diff_with(report)
         license_names = diff[:added].map(&:name)
         license_ids = diff[:added].filter_map(&:id)
       elsif scan_result_policy_read.newly_detected?
         license_names = report.license_names
         license_ids = report.licenses.filter_map(&:id)
       else
-        license_names = default_branch_report.license_names
-        license_ids = default_branch_report.licenses.filter_map(&:id)
+        license_names = target_branch_report.license_names
+        license_ids = target_branch_report.licenses.filter_map(&:id)
       end
 
       [license_ids, license_names]
@@ -112,28 +120,26 @@ def license_names_from_ids(ids, names)
       ids.concat(names).compact.uniq
     end
 
-    def new_dependency_with_denied_license?(denied_licenses)
+    def new_dependency_with_denied_license?(target_branch_report, denied_licenses)
       dependencies_with_denied_licenses = report.licenses
         .select { |license| denied_licenses.include?(license.name) || denied_licenses.include?(license.id) }
         .flat_map(&:dependencies).map(&:name)
 
-      (dependencies_with_denied_licenses & new_dependency_names).present?
+      (dependencies_with_denied_licenses & new_dependency_names(target_branch_report)).present?
     end
 
-    def report
-      project.license_compliance(pipeline).license_scanning_report
+    def target_branch_report(merge_request)
+      project.license_compliance(merge_request.latest_pipeline_for_target_branch).license_scanning_report
     end
-    strong_memoize_attr :report
 
-    def default_branch_report
-      project.license_compliance.license_scanning_report
+    def new_dependency_names(target_branch_report)
+      report.dependency_names - target_branch_report.dependency_names
     end
-    strong_memoize_attr :default_branch_report
 
-    def new_dependency_names
-      report.dependency_names - default_branch_report.dependency_names
+    def report
+      project.license_compliance(pipeline).license_scanning_report
     end
-    strong_memoize_attr :new_dependency_names
+    strong_memoize_attr :report
 
     def license_names_from_report
       report.license_names.concat(report.licenses.filter_map(&:id)).compact.uniq

ee/spec/services/security/sync_license_scanning_rules_service_spec.rb

@@ -102,15 +102,21 @@
       end
 
       context 'when license_scanning_policies is enabled' do
-        let(:case1) { [] }
-        let(:case2) { [['GPL v3', 'A']] }
-        let(:case3) { [['GPL v3', 'A'], ['MIT', 'B']] }
-        let(:case4) { [['GPL v3', 'A'], ['MIT', 'B'], ['GPL v3', 'C']] }
         let(:case5) { [['GPL v3', 'A'], ['MIT', 'B'], ['GPL v3', 'C'], ['Apache 2', 'D']] }
+        let(:case4) { [['GPL v3', 'A'], ['MIT', 'B'], ['GPL v3', 'C']] }
+        let(:case3) { [['GPL v3', 'A'], ['MIT', 'B']] }
+        let(:case2) { [['GPL v3', 'A']] }
+        let(:case1) { [] }
+
+        context 'when target branch pipeline is empty' do
+          it 'does not require approval' do
+            expect { subject }.to change { license_finding_rule.reload.approvals_required }.from(1).to(0)
+          end
+        end
 
         using RSpec::Parameterized::TableSyntax
 
-        where(:default_branch, :pipeline_branch, :states, :policy_license, :policy_state, :result) do
+        where(:target_branch, :pipeline_branch, :states, :policy_license, :policy_state, :result) do
           ref(:case1) | ref(:case2) | ['newly_detected'] | 'GPL v3' | :denied  | true
           ref(:case2) | ref(:case3) | ['newly_detected'] | 'GPL v3' | :denied  | false
           ref(:case3) | ref(:case4) | ['newly_detected'] | 'GPL v3' | :denied  | true
@@ -132,7 +138,7 @@
 
         with_them do
           let(:match_on_inclusion) { policy_state == :denied }
-          let(:default_branch_report) { create(:ci_reports_license_scanning_report) }
+          let(:target_branch_report) { create(:ci_reports_license_scanning_report) }
           let(:pipeline_report) { create(:ci_reports_license_scanning_report) }
           let(:license_states) { states }
           let(:license) { create(:software_license, name: policy_license) }
@@ -140,8 +146,8 @@
           before do
             stub_feature_flags(license_scanning_policies: true)
 
-            default_branch.each do |ld|
-              default_branch_report.add_license(id: nil, name: ld[0]).add_dependency(name: ld[1])
+            target_branch.each do |ld|
+              target_branch_report.add_license(id: nil, name: ld[0]).add_dependency(name: ld[1])
             end
 
             pipeline_branch.each do |ld|
@@ -155,7 +161,7 @@
             )
 
             allow(service).to receive(:report).and_return(pipeline_report)
-            allow(service).to receive(:default_branch_report).and_return(default_branch_report)
+            allow(service).to receive(:target_branch_report).and_return(target_branch_report)
           end
 
           it 'sync approvals_required' do