diff --git a/ee/app/services/security/sync_license_scanning_rules_service.rb b/ee/app/services/security/sync_license_scanning_rules_service.rb
index c025afa6dd4b27e54933a6cf9429901b32950dc6..ae4a65b9fe2099e92f18d709f7c426b9107938be 100644
--- a/ee/app/services/security/sync_license_scanning_rules_service.rb
+++ b/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
diff --git a/ee/config/feature_flags/wip/save_policy_violation_data.yml b/ee/config/feature_flags/wip/save_policy_violation_data.yml
new file mode 100644
index 0000000000000000000000000000000000000000..5703e4ef578d63252baa5443a87f635196b63d44
--- /dev/null
+++ b/ee/config/feature_flags/wip/save_policy_violation_data.yml
@@ -0,0 +1,9 @@
+---
+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
diff --git a/ee/spec/services/security/sync_license_scanning_rules_service_spec.rb b/ee/spec/services/security/sync_license_scanning_rules_service_spec.rb
index 887ce8b7e94f9df2a08a520e44a3906a9a3ddbc2..ab064a3dd9060717ae53aca9d39bb1541803a459 100644
--- a/ee/spec/services/security/sync_license_scanning_rules_service_spec.rb
+++ b/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