Merge branch '433401-persist-license_scanning-violation-data' into 'master'

Persist license policy violations data

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



Merged-by: Patrick Cyiza <jpcyiza@gitlab.com>
Approved-by: Marcos Rocha <mrocha@gitlab.com>
Approved-by: Patrick Cyiza <jpcyiza@gitlab.com>
Reviewed-by: Patrick Cyiza <jpcyiza@gitlab.com>
Reviewed-by: Martin Čavoj <mcavoj@gitlab.com>
Co-authored-by: Martin Čavoj <mcavoj@gitlab.com>

ee/app/services/security/sync_license_scanning_rules_service.rb

@@ -36,17 +36,17 @@ def sync_license_finding_rules(merge_requests)
 
     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
+                                 .approval_rules
+                                 .report_approver
+                                 .license_scanning
+                                 .with_scan_result_policy_read
+                                 .including_scan_result_policy_read
 
       return if license_approval_rules.empty?
 
       violations = Security::SecurityOrchestrationPolicies::UpdateViolationsService.new(merge_request)
       violated_rules, unviolated_rules = license_approval_rules.partition do |rule|
-        violates_policy?(merge_request, rule)
+        violates_policy?(merge_request, rule, violations)
       end
 
       update_required_approvals(merge_request, violated_rules, unviolated_rules)
@@ -66,34 +66,48 @@ def update_required_approvals(merge_request, violated_rules, unviolated_rules)
     ##     with license type violating the policy.
     ##   - If match_on_inclusion_license is false, any detected licenses that does not match
     ##     the licenses from `license_types` should require approval
-    def violates_policy?(merge_request, rule)
+    def violates_policy?(merge_request, rule, violations)
       scan_result_policy_read = rule.scan_result_policy_read
-      check_denied_licenses = scan_result_policy_read.match_on_inclusion_license
       target_branch_report = target_branch_report(merge_request)
 
+      license_policies = license_policies(scan_result_policy_read)
+      license_ids_from_policy, license_names_from_policy = license_policy_ids_and_names(license_policies)
+      licenses_from_policy = join_ids_and_names(license_ids_from_policy, license_names_from_policy)
+
       license_ids, license_names = licenses_to_check(target_branch_report, scan_result_policy_read)
-      license_policies = project
-        .software_license_policies
-        .including_license
-        .for_scan_result_policy_read(scan_result_policy_read.id)
 
-      license_names_from_policy = license_names_from_policy(license_policies)
-      if check_denied_licenses
-        denied_licenses = license_names_from_policy
+      if scan_result_policy_read.match_on_inclusion_license
+        all_denied_licenses = licenses_from_policy
+        policy_denied_license_names = (all_denied_licenses & licenses_from_report) - license_ids_from_policy
         violates_license_policy = report.violates_for_licenses?(license_policies, license_ids, license_names)
       else
-        # when match_on_inclusion_license is false, only allowed licenses are mentioned in policy
-        denied_licenses = (license_names_from_report - license_names_from_policy).uniq
-        license_names_from_ids = license_names_from_ids(license_ids, license_names)
-        violates_license_policy = (license_names_from_ids - license_names_from_policy).present?
+        # when match_on_inclusion_license is false, only the licenses mentioned in the policy are allowed
+        all_denied_licenses = (licenses_from_report - licenses_from_policy).uniq
+        comparison_licenses = join_ids_and_names(license_ids, license_names)
+        policy_denied_license_names = (comparison_licenses - licenses_from_policy).uniq - license_ids
+        violates_license_policy = policy_denied_license_names.present?
       end
 
-      return true if scan_result_policy_read.newly_detected? &&
-        new_dependency_with_denied_license?(target_branch_report, denied_licenses)
+      # when there are no license violations, but new dependency with policy licenses is added, require approval
+      if scan_result_policy_read.newly_detected?
+        new_license_dependency_map = new_dependencies_with_denied_licenses(target_branch_report, all_denied_licenses)
+        if new_license_dependency_map.present?
+          violates_license_policy = true
+          policy_denied_license_names = new_license_dependency_map.keys.uniq
+        end
+      end
 
+      set_violation_data(violations, rule, policy_denied_license_names) if violates_license_policy
       violates_license_policy
     end
 
+    def license_policies(scan_result_policy_read)
+      project
+        .software_license_policies
+        .including_license
+        .for_scan_result_policy_read(scan_result_policy_read.id)
+    end
+
     def licenses_to_check(target_branch_report, scan_result_policy_read)
       only_newly_detected = scan_result_policy_read.license_states == [ApprovalProjectRule::NEWLY_DETECTED]
 
@@ -112,23 +126,24 @@ def licenses_to_check(target_branch_report, scan_result_policy_read)
       [license_ids, license_names]
     end
 
-    def license_names_from_policy(license_policies)
+    def license_policy_ids_and_names(license_policies)
       ids = license_policies.map(&:spdx_identifier)
       names = license_policies.map(&:name)
 
-      ids.concat(names).compact
+      [ids, names].map(&:compact)
     end
 
-    def license_names_from_ids(ids, names)
-      ids.concat(names).compact.uniq
+    def join_ids_and_names(ids, names)
+      (ids + names).compact.uniq
     end
 
-    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)
+    def new_dependencies_with_denied_licenses(target_branch_report, denied_licenses)
+      new_dependency_names_in_report = new_dependency_names(target_branch_report)
 
-      (dependencies_with_denied_licenses & new_dependency_names(target_branch_report)).present?
+      report.licenses
+            .select { |license| denied_licenses.include?(license.name) || denied_licenses.include?(license.id) }
+            .to_h { |license| [license.name, license.dependencies.map(&:name)] }
+            .select { |_license, dependency_names| (dependency_names & new_dependency_names_in_report).present? }
     end
 
     def target_branch_report(merge_request)
@@ -148,10 +163,10 @@ def report
     end
     strong_memoize_attr :report
 
-    def license_names_from_report
+    def licenses_from_report
       report.license_names.concat(report.licenses.filter_map(&:id)).compact.uniq
     end
-    strong_memoize_attr :license_names_from_report
+    strong_memoize_attr :licenses_from_report
 
     def log_violated_rules(merge_request, rules)
       return unless rules.any?
@@ -175,5 +190,15 @@ def log_update_approval_rule(merge_request, **attributes)
       }
       Gitlab::AppJsonLogger.info(message: 'Updating MR approval rule', **default_attributes.merge(attributes))
     end
+
+    def set_violation_data(violations, rule, policy_denied_licenses)
+      return if ::Feature.disabled?(:save_policy_violation_data, project)
+      return if policy_denied_licenses.blank?
+
+      violations.set_violation_data(
+        rule.scan_result_policy_id,
+        { violations: { licenses: policy_denied_licenses } }
+      )
+    end
   end
 end

ee/config/feature_flags/wip/save_policy_violation_data.yml

+---
+name: save_policy_violation_data
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/433390
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/144883
+rollout_issue_url:
+milestone: '16.10'
+group: group::security policies
+type: wip
+default_enabled: false

ee/spec/services/security/sync_license_scanning_rules_service_spec.rb

@@ -78,10 +78,18 @@
       let(:sbom_scanner) { instance_double('Gitlab::LicenseScanning::SbomScanner', report: target_branch_report) }
       let(:target_branch_report) { create(:ci_reports_license_scanning_report) }
 
-      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(:case5) do
+        [
+          ['GPL v3', 'GNU 3', 'A'],
+          ['MIT', 'MIT License', 'B'],
+          ['GPL v3', 'GNU 3', 'C'],
+          ['Apache 2', 'Apache License 2', 'D']
+        ]
+      end
+
+      let(:case4) { [['GPL v3', 'GNU 3', 'A'], ['MIT', 'MIT License', 'B'], ['GPL v3', 'GNU 3', 'C']] }
+      let(:case3) { [['GPL v3', 'GNU 3', 'A'], ['MIT', 'MIT License', 'B']] }
+      let(:case2) { [['GPL v3', 'GNU 3', 'A']] }
       let(:case1) { [] }
 
       context 'when target branch pipeline is empty' do
@@ -189,70 +197,119 @@
         end
       end
 
-      using RSpec::Parameterized::TableSyntax
-
-      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
-        ref(:case4) | ref(:case5) | ['newly_detected'] | 'GPL v3' | :denied  | false
-        ref(:case1) | ref(:case2) | ['detected'] | 'GPL v3' | :denied  | false
-        ref(:case2) | ref(:case3) | ['detected'] | 'GPL v3' | :denied  | true
-        ref(:case3) | ref(:case4) | ['detected'] | 'GPL v3' | :denied  | true
-        ref(:case4) | ref(:case5) | ['detected'] | 'GPL v3' | :denied  | true
-
-        ref(:case1) | ref(:case2) | ['newly_detected'] | 'MIT' | :allowed  | true
-        ref(:case2) | ref(:case3) | ['newly_detected'] | 'MIT' | :allowed  | false
-        ref(:case3) | ref(:case4) | ['newly_detected'] | 'MIT' | :allowed  | true
-        ref(:case4) | ref(:case5) | ['newly_detected'] | 'MIT' | :allowed  | true
-        ref(:case1) | ref(:case2) | ['detected'] | 'MIT' | :allowed  | false
-        ref(:case2) | ref(:case3) | ['detected'] | 'MIT' | :allowed  | true
-        ref(:case3) | ref(:case4) | ['detected'] | 'MIT' | :allowed  | true
-        ref(:case4) | ref(:case5) | ['detected'] | 'MIT' | :allowed  | true
-      end
+      describe 'possible combinations' do
+        using RSpec::Parameterized::TableSyntax
+
+        where(:target_branch, :pipeline_branch, :states, :policy_license, :policy_state, :violated_license, :result) do
+          ref(:case1) | ref(:case2) | ['newly_detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case1) | ref(:case2) | ['newly_detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case2) | ref(:case3) | ['newly_detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | false
+          ref(:case2) | ref(:case3) | ['newly_detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | false
+          ref(:case3) | ref(:case4) | ['newly_detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case3) | ref(:case4) | ['newly_detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case4) | ref(:case5) | ['newly_detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | false
+          ref(:case4) | ref(:case5) | ['newly_detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | false
+          ref(:case1) | ref(:case2) | ['detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | false
+          ref(:case1) | ref(:case2) | ['detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | false
+          ref(:case2) | ref(:case3) | ['detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case2) | ref(:case3) | ['detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case3) | ref(:case4) | ['detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case3) | ref(:case4) | ['detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case4) | ref(:case5) | ['detected'] | ['GPL v3', 'GNU 3'] | :denied | 'GNU 3' | true
+          ref(:case4) | ref(:case5) | ['detected'] | [nil, 'GNU 3'] | :denied | 'GNU 3' | true
+
+          ref(:case1) | ref(:case2) | ['newly_detected'] | ['MIT', 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case1) | ref(:case2) | ['newly_detected'] | [nil, 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case2) | ref(:case3) | ['newly_detected'] | ['MIT', 'MIT License'] | :allowed | nil | false
+          ref(:case3) | ref(:case4) | ['newly_detected'] | ['MIT', 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case3) | ref(:case4) | ['newly_detected'] | [nil, 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case4) | ref(:case5) | ['newly_detected'] | ['MIT', 'MIT License'] | :allowed | 'Apache License 2' | true
+          ref(:case4) | ref(:case5) | ['newly_detected'] | [nil, 'MIT License'] | :allowed | 'Apache License 2' | true
+          ref(:case1) | ref(:case2) | ['detected'] | ['MIT', 'MIT License'] | :allowed | nil | false
+          ref(:case1) | ref(:case2) | ['detected'] | [nil, 'MIT License'] | :allowed | nil | false
+          ref(:case2) | ref(:case3) | ['detected'] | ['MIT', 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case2) | ref(:case3) | ['detected'] | [nil, 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case3) | ref(:case4) | ['detected'] | ['MIT', 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case3) | ref(:case4) | ['detected'] | [nil, 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case4) | ref(:case5) | ['detected'] | ['MIT', 'MIT License'] | :allowed | 'GNU 3' | true
+          ref(:case4) | ref(:case5) | ['detected'] | [nil, 'MIT License'] | :allowed | 'GNU 3' | true
+
+          # TODO: These cases fail. Related to https://gitlab.com/gitlab-org/gitlab/-/issues/438584
+          # When spdx_identifier is used in policy instead of name, match_on_inclusion_license is evaluated incorrectly
+          # ref(:case2) | ref(:case3) | ['newly_detected'] | [nil, 'MIT'] | :allowed | nil | false
+          # ref(:case2) | ref(:case2) | ['detected'] | [nil, 'GPL v3'] | :allowed | nil | false
+        end
 
-      with_them do
-        let(:match_on_inclusion_license) { policy_state == :denied }
-        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) }
+        with_them do
+          let(:match_on_inclusion_license) { policy_state == :denied }
+          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, spdx_identifier: policy_license[0], name: policy_license[1]) }
 
-        before do
-          target_branch.each do |ld|
-            target_branch_report.add_license(id: nil, name: ld[0]).add_dependency(name: ld[1])
-          end
+          before do
+            target_branch.each do |ld|
+              target_branch_report.add_license(id: ld[0], name: ld[1]).add_dependency(name: ld[2])
+            end
 
-          pipeline_branch.each do |ld|
-            pipeline_report.add_license(id: nil, name: ld[0]).add_dependency(name: ld[1])
-          end
+            pipeline_branch.each do |ld|
+              pipeline_report.add_license(id: ld[0], name: ld[1]).add_dependency(name: ld[2])
+            end
 
-          create(:software_license_policy, policy_state,
-            project: project,
-            software_license: license,
-            scan_result_policy_read: scan_result_policy_read
-          )
+            create(:software_license_policy, policy_state,
+              project: project,
+              software_license: license,
+              scan_result_policy_read: scan_result_policy_read
+            )
 
-          allow(service).to receive(:report).and_return(pipeline_report)
-          allow(service).to receive(:target_branch_report).and_return(target_branch_report)
-        end
+            allow(service).to receive(:report).and_return(pipeline_report)
+            allow(service).to receive(:target_branch_report).and_return(target_branch_report)
+          end
 
-        it 'syncs approvals_required' do
-          if result
-            expect { execute }.not_to change { license_finding_rule.reload.approvals_required }
-          else
-            expect { execute }.to change { license_finding_rule.reload.approvals_required }.from(1).to(0)
+          it 'syncs approvals_required' do
+            if result
+              expect { execute }.not_to change { license_finding_rule.reload.approvals_required }
+            else
+              expect { execute }.to change { license_finding_rule.reload.approvals_required }.from(1).to(0)
+            end
           end
-        end
 
-        it 'logs only violated rules' do
-          if result
-            expect(Gitlab::AppJsonLogger).to receive(:info).with(hash_including(message: 'Updating MR approval rule'))
-          else
-            expect(Gitlab::AppJsonLogger).not_to receive(:info)
+          it 'logs only violated rules' do
+            if result
+              expect(Gitlab::AppJsonLogger).to receive(:info).with(hash_including(message: 'Updating MR approval rule'))
+            else
+              expect(Gitlab::AppJsonLogger).not_to receive(:info)
+            end
+
+            execute
           end
 
-          execute
+          describe 'violation data' do
+            it 'persists violation data' do
+              if result
+                expect { execute }.to change { scan_result_policy_read.violations.count }.by(1)
+                expect(scan_result_policy_read.violations.last.violation_data)
+                  .to eq({ 'violations' => { 'licenses' => [violated_license] } })
+              else
+                expect { execute }.not_to change { scan_result_policy_read.violations.count }
+              end
+            end
+
+            context 'when feature flag "save_policy_violation_data" is disabled' do
+              before do
+                stub_feature_flags(save_policy_violation_data: false)
+              end
+
+              it 'adds violations without data' do
+                if result
+                  expect { execute }.to change { scan_result_policy_read.violations.count }.by(1)
+                  expect(scan_result_policy_read.violations.last.violation_data).to be_nil
+                else
+                  expect { execute }.not_to change { scan_result_policy_read.violations.count }
+                end
+              end
+            end
+          end
         end
       end
     end