diff --git a/ee/app/finders/sbom/possibly_affected_occurrences_finder.rb b/ee/app/finders/sbom/possibly_affected_occurrences_finder.rb index b49d9204390afda0206a5fbf94c6a027aa5e6d3e..ed84f649c978bf085b5f85a85bfe30299427c76c 100644 --- a/ee/app/finders/sbom/possibly_affected_occurrences_finder.rb +++ b/ee/app/finders/sbom/possibly_affected_occurrences_finder.rb @@ -20,7 +20,8 @@ def execute_in_batches(of: BATCH_SIZE) search_scope.each_batch(of: of) do |batch| yield batch - .with_component_source_version_project_and_pipeline + .with_component_source_version_and_project + .with_pipeline_project_and_namespace .filter_by_non_nil_component_version end end diff --git a/ee/app/models/sbom/occurrence.rb b/ee/app/models/sbom/occurrence.rb index 42be7cdce88e57d21ee6df866236f9d99e4e6d02..55525048ae3353b7c8f547d3cf5131022c7804fc 100644 --- a/ee/app/models/sbom/occurrence.rb +++ b/ee/app/models/sbom/occurrence.rb @@ -119,8 +119,9 @@ class Occurrence < ApplicationRecord scope :with_project_namespace, -> { includes(project: [namespace: :route]) } scope :with_source, -> { includes(:source) } scope :with_version, -> { includes(:component_version) } - scope :with_component_source_version_project_and_pipeline, -> do - includes(:component, :source, :component_version, :project).preload(:pipeline) + scope :with_pipeline_project_and_namespace, -> { preload(pipeline: { project: :namespace }) } + scope :with_component_source_version_and_project, -> do + includes(:component, :source, :component_version, :project) end scope :filter_by_non_nil_component_version, -> { where.not(component_version: nil) } diff --git a/ee/app/services/security/ingestion/finding_map.rb b/ee/app/services/security/ingestion/finding_map.rb index 17245a18b8d0d89b81dc60d419672bc68cbfc5f4..a6f4845168634bdc69945dba5b2dd9a4cda46afd 100644 --- a/ee/app/services/security/ingestion/finding_map.rb +++ b/ee/app/services/security/ingestion/finding_map.rb @@ -15,7 +15,7 @@ class FindingMap delegate :uuid, :scanner_id, :severity, to: :security_finding delegate :scan, to: :security_finding, private: true - delegate :project_id, to: :pipeline + delegate :project, to: :pipeline delegate :project_fingerprint, to: :report_finding, private: true delegate :evidence, to: :report_finding @@ -32,7 +32,7 @@ def identifiers def identifier_data identifiers.map do |identifier| - identifier.to_hash.merge(project_id: project_id) + identifier.to_hash.merge(project_id: project.id) end end @@ -50,7 +50,7 @@ def to_hash primary_identifier_id: identifier_ids.first, location: report_finding.location_data, location_fingerprint: report_finding.location_fingerprint, - project_id: project_id, + project_id: project.id, initial_pipeline_id: pipeline.id, latest_pipeline_id: pipeline.id ) diff --git a/ee/app/services/security/ingestion/ingest_cvs_slice_service.rb b/ee/app/services/security/ingestion/ingest_cvs_slice_service.rb index 2962e2589adbdeecf1682b9c120ccc209f59b797..0ab509ceffb293bb3ef29d37bf749539524f7efa 100644 --- a/ee/app/services/security/ingestion/ingest_cvs_slice_service.rb +++ b/ee/app/services/security/ingestion/ingest_cvs_slice_service.rb @@ -14,6 +14,7 @@ class IngestCvsSliceService < IngestSliceBaseService IngestFindingSignatures IngestFindingEvidence IngestVulnerabilityFlags + IngestVulnerabilityReads IngestVulnerabilityStatistics HooksExecution ].freeze diff --git a/ee/app/services/security/ingestion/tasks/ingest_identifiers.rb b/ee/app/services/security/ingestion/tasks/ingest_identifiers.rb index b46d2d6eef70e9577c4d7f36a855d75cb622655b..86b7e00b7fe88caf7a06bbd48b7f7bb9eb78b2b9 100644 --- a/ee/app/services/security/ingestion/tasks/ingest_identifiers.rb +++ b/ee/app/services/security/ingestion/tasks/ingest_identifiers.rb @@ -21,7 +21,7 @@ def after_ingest map[project_id][fingerprint] = id end - finding_maps.each { |finding_map| finding_map.set_identifier_ids_by(fingerprint_map[finding_map.project_id]) } + finding_maps.each { |finding_map| finding_map.set_identifier_ids_by(fingerprint_map[finding_map.project.id]) } end # Important Note: diff --git a/ee/app/services/security/ingestion/tasks/ingest_vulnerabilities/create.rb b/ee/app/services/security/ingestion/tasks/ingest_vulnerabilities/create.rb index 57b812a046ebee697ab52eeeea560caf3dbaa3e5..2591f3faad184be1a23ce7b312908761321d8e55 100644 --- a/ee/app/services/security/ingestion/tasks/ingest_vulnerabilities/create.rb +++ b/ee/app/services/security/ingestion/tasks/ingest_vulnerabilities/create.rb @@ -32,7 +32,7 @@ def attributes_for_finding(finding_map) { author_id: finding_map.pipeline.user_id, - project_id: finding_map.project_id, + project_id: finding_map.project.id, title: report_finding.name.to_s.truncate(::Issuable::TITLE_LENGTH_MAX), state: :detected, # this will be detected because if there is any interaction for dismissal for finding, there will be vulnerability already severity: report_finding.severity, diff --git a/ee/app/services/security/ingestion/tasks/ingest_vulnerability_reads/update.rb b/ee/app/services/security/ingestion/tasks/ingest_vulnerability_reads/update.rb index 2a72048e3acfdd847648b831fc29ed582d422960..041c782bb70954dd92f27f2b69ffc8cde974fee3 100644 --- a/ee/app/services/security/ingestion/tasks/ingest_vulnerability_reads/update.rb +++ b/ee/app/services/security/ingestion/tasks/ingest_vulnerability_reads/update.rb @@ -14,8 +14,6 @@ class Update < AbstractTask VALID_OWASP_PRIORITY_LABELS = %w[A1 A2 A3 A4 A5 A6 A7 A8 A9 A10].freeze VALID_OWASP_YEARS = %w[2017 2021].freeze - delegate :project, to: :pipeline, private: true - private # Note: https://gitlab.com/gitlab-org/gitlab/-/issues/430025 can iterate by @@ -25,8 +23,8 @@ def attributes finding_maps.map do |finding_map| attributes = { vulnerability_id: finding_map.vulnerability_id, - traversal_ids: project.namespace.traversal_ids, - archived: project.archived, + traversal_ids: finding_map.project.namespace.traversal_ids, + archived: finding_map.project.archived, owasp_top_10: nil } diff --git a/ee/app/services/security/ingestion/tasks/ingest_vulnerability_statistics.rb b/ee/app/services/security/ingestion/tasks/ingest_vulnerability_statistics.rb index f09e31179c9c1fe1a66b72abbefa9dd06b7f933f..c3f906d5d9b65bee7c6db1dda136c9387b4c8576 100644 --- a/ee/app/services/security/ingestion/tasks/ingest_vulnerability_statistics.rb +++ b/ee/app/services/security/ingestion/tasks/ingest_vulnerability_statistics.rb @@ -53,9 +53,9 @@ def insert_attributes end def insert_values - severity_counts.map do |project_id, counts| + severity_counts.map do |project, counts| [ - project_id, + project.id, letter_grade(counts), Arel.sql('now()'), Arel.sql('now()') @@ -84,7 +84,7 @@ def severity_levels def severity_counts @severity_counts ||= finding_maps .select(&:new_or_transitioned_to_detected?) - .group_by(&:project_id) + .group_by(&:project) .transform_values { |finding_maps| fill_in_zeros(finding_maps.map(&:severity).tally) } end diff --git a/ee/app/services/security/vulnerability_scanning/finding_map.rb b/ee/app/services/security/vulnerability_scanning/finding_map.rb index 3f0dc6cd5505dcb2a84109d614a090b48e3d4103..dbfe12da7a30eb962095178ba048b6c5208c46b6 100644 --- a/ee/app/services/security/vulnerability_scanning/finding_map.rb +++ b/ee/app/services/security/vulnerability_scanning/finding_map.rb @@ -24,7 +24,7 @@ class FindingMap delegate :uuid, :severity, :evidence, to: :report_finding delegate :project_fingerprint, to: :report_finding, private: true - delegate :project_id, to: :pipeline + delegate :project, to: :pipeline def initialize(pipeline:, report_finding:, scanner_id:) @pipeline = pipeline @@ -51,7 +51,7 @@ def set_identifier_ids_by(fingerprint_id_map) def identifier_data identifiers.map do |identifier| - identifier.to_hash.merge(project_id: project_id) + identifier.to_hash.merge(project_id: project.id) end end @@ -65,7 +65,7 @@ def to_hash primary_identifier_id: identifier_ids.first, location: report_finding.location_data, location_fingerprint: report_finding.location_fingerprint, - project_id: project_id, + project_id: project.id, initial_pipeline_id: pipeline.id, latest_pipeline_id: pipeline.id ) diff --git a/ee/app/services/vulnerabilities/create_service_base.rb b/ee/app/services/vulnerabilities/create_service_base.rb index 73c130fad463450a9741f8f28e94188ca8a2c13d..13e1d09e5b4663b09eae0637269ba5f15d87d20b 100644 --- a/ee/app/services/vulnerabilities/create_service_base.rb +++ b/ee/app/services/vulnerabilities/create_service_base.rb @@ -14,6 +14,8 @@ def initialize(project, author, params:) private + attr_reader :project + def authorized? can?(@author, :admin_vulnerability, @project) end diff --git a/ee/app/services/vulnerabilities/manually_create_service.rb b/ee/app/services/vulnerabilities/manually_create_service.rb index 587c85e6831f0581ea10f1add429c013220fdf14..7009fab50e08e3d5bb581c639b12138eda078120 100644 --- a/ee/app/services/vulnerabilities/manually_create_service.rb +++ b/ee/app/services/vulnerabilities/manually_create_service.rb @@ -38,6 +38,9 @@ def execute vulnerability.vulnerability_finding = finding vulnerability.save! finding.update!(vulnerability_id: vulnerability.id) + + vulnerability.vulnerability_read.update!(traversal_ids: project.namespace.traversal_ids) + Statistics::UpdateService.update_for(vulnerability) ServiceResponse.success(payload: { vulnerability: vulnerability }) diff --git a/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb b/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb index 232437bb371ae685024867fff4103fdcaf0b211b..6a3936f492311a89da395ae2b9443d5921e9a8b5 100644 --- a/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb +++ b/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb @@ -42,6 +42,8 @@ def execute finding.update!(vulnerability_id: vulnerability.id) agent.update!(has_vulnerabilities: true) unless agent.has_vulnerabilities? + vulnerability.vulnerability_read.update!(traversal_ids: project.namespace.traversal_ids) + Statistics::UpdateService.update_for(vulnerability) ServiceResponse.success(payload: { vulnerability: vulnerability }) diff --git a/ee/spec/models/sbom/occurrence_spec.rb b/ee/spec/models/sbom/occurrence_spec.rb index bb376ba34778792b53daeb9ac6bb46abc0afdafa..6ab03bac9c925f48488d6ef82b62d12b8ca53bcc 100644 --- a/ee/spec/models/sbom/occurrence_spec.rb +++ b/ee/spec/models/sbom/occurrence_spec.rb @@ -134,17 +134,16 @@ end end - describe '.with_component_source_version_project_and_pipeline scope' do + describe '.with_component_source_version_and_project scope' do let_it_be(:occurrence) { create(:sbom_occurrence, component: create(:sbom_component)) } it 'pre-loads relations to avoid executing additional queries' do - record = described_class.with_component_source_version_project_and_pipeline.first + record = described_class.with_component_source_version_and_project.first queries = ActiveRecord::QueryRecorder.new do record.component record.component_version record.source - record.pipeline record.project end @@ -152,6 +151,22 @@ end end + describe '.with_pipeline_project_and_namespace' do + before do + create(:sbom_occurrence, component: create(:sbom_component)) + end + + it 'preloads the pipeline, project, and namespace associations' do + record = described_class.with_pipeline_project_and_namespace.first + + queries = ActiveRecord::QueryRecorder.new do + record.pipeline.project.namespace + end + + expect(queries.count).to be_zero + end + end + describe '.filter_by_non_nil_component_version scope' do let_it_be(:matching_occurrence) { create(:sbom_occurrence) } let_it_be(:non_matching_occurrence) { create(:sbom_occurrence, component_version: nil) } diff --git a/ee/spec/services/security/ingestion/finding_map_spec.rb b/ee/spec/services/security/ingestion/finding_map_spec.rb index 8df7b22c339deec8a48eab60343f6084feb4cf1a..8d422e8b9f7caf808f3ed7782f13dc6afc0b869a 100644 --- a/ee/spec/services/security/ingestion/finding_map_spec.rb +++ b/ee/spec/services/security/ingestion/finding_map_spec.rb @@ -18,7 +18,7 @@ it { is_expected.to delegate_method(:uuid).to(:security_finding) } it { is_expected.to delegate_method(:scanner_id).to(:security_finding) } it { is_expected.to delegate_method(:severity).to(:security_finding) } - it { is_expected.to delegate_method(:project_id).to(:pipeline) } + it { is_expected.to delegate_method(:project).to(:pipeline) } it { is_expected.to delegate_method(:evidence).to(:report_finding) } end diff --git a/ee/spec/services/security/ingestion/tasks/ingest_vulnerabilities/create_spec.rb b/ee/spec/services/security/ingestion/tasks/ingest_vulnerabilities/create_spec.rb index ebfd921b1ad93272e95f2aa1f2e295eb9960abdd..b93977431db358efe5bc4ae3a9f73a2006b3ab59 100644 --- a/ee/spec/services/security/ingestion/tasks/ingest_vulnerabilities/create_spec.rb +++ b/ee/spec/services/security/ingestion/tasks/ingest_vulnerabilities/create_spec.rb @@ -27,7 +27,7 @@ def create_finding_map expect(created_vulnerabilities.size).to eq(2) expect(created_vulnerabilities.map(&:author_id)).to match_array( finding_maps.map { |finding_map| finding_map.pipeline.user_id }) - expect(created_vulnerabilities.map(&:project_id)).to match_array(finding_maps.map(&:project_id)) + expect(created_vulnerabilities.map(&:project)).to match_array(finding_maps.map(&:project)) end end diff --git a/ee/spec/services/security/ingestion/tasks/ingest_vulnerability_reads/update_spec.rb b/ee/spec/services/security/ingestion/tasks/ingest_vulnerability_reads/update_spec.rb index b258f766d0ff144615e232484a172a836d6eaf07..9abb65bc9908f98ae99a7d36ac1dabdfc79a3eb6 100644 --- a/ee/spec/services/security/ingestion/tasks/ingest_vulnerability_reads/update_spec.rb +++ b/ee/spec/services/security/ingestion/tasks/ingest_vulnerability_reads/update_spec.rb @@ -35,14 +35,10 @@ location_fingerprint: location.fingerprint) end - let!(:finding_map) { create(:finding_map, report_finding: ci_reports_security_finding) } + let!(:finding_map) { create(:finding_map, pipeline: pipeline, report_finding: ci_reports_security_finding) } let!(:update_service) { described_class.new(pipeline, [finding_map]) } before do - # For existing vulnerabilities it is set in IngestFindings. - # See: https://gitlab.com/gitlab-org/gitlab/-/blob/e802e761b93d4166a6741e0d2b1b09f694605b01/ee/app/services/security/ingestion/tasks/ingest_findings.rb#L20 - # For new vulnerabilities it is set in IngestVulnerabilities::Create. - # See: https://gitlab.com/gitlab-org/gitlab/-/blob/cab15cc37bf5ee6e847325170d49da0b84ea3aca/ee/app/services/security/ingestion/tasks/ingest_vulnerabilities/create.rb#L21 finding_map.vulnerability_id = vulnerability.id end diff --git a/ee/spec/services/security/vulnerability_scanning/create_vulnerability_service_spec.rb b/ee/spec/services/security/vulnerability_scanning/create_vulnerability_service_spec.rb index 031b4031fd2e4d805b46c29cd4bc38afcd2f1d45..0a76be4a2637e042823f5b5c57c29d6228664be6 100644 --- a/ee/spec/services/security/vulnerability_scanning/create_vulnerability_service_spec.rb +++ b/ee/spec/services/security/vulnerability_scanning/create_vulnerability_service_spec.rb @@ -4,7 +4,9 @@ RSpec.describe Security::VulnerabilityScanning::CreateVulnerabilityService, feature_category: :software_composition_analysis do let_it_be(:user) { create(:user) } - let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let_it_be_with_refind(:project) { create(:project) } + let_it_be_with_refind(:pipeline) { create(:ci_pipeline, user: user, project: project) } + let(:sbom_source) { build(:ci_reports_sbom_source) } let(:affected_components) do [ @@ -26,28 +28,49 @@ describe '#execute' do context 'when the component type is supported' do shared_examples 'vulnerability creation' do + let(:created_vulnerability) { Vulnerability.find(service_response.payload[:vulnerability_ids].first) } + it 'creates a new vulnerabilities' do expect(service_response.payload[:error]).to be_nil expect(service_response.success?).to be(true) + expect(service_response.payload[:vulnerability_ids].length).to be(1) + + expect(created_vulnerability).to have_attributes( + author_id: user.id, + project_id: pipeline.project_id, + state: 'detected', + confidence: 'unknown', + report_type: report_type, + present_on_default_branch: true, + title: advisory.title, + severity: advisory.cvss_v3.severity.downcase, + finding_description: advisory.description, + solution: advisory.solution + ) + end + + describe 'updating the `vulnerability_reads`' do + let(:vulnerability_read) { created_vulnerability.vulnerability_read } + + it 'sets the `traversal_ids` for the `vulnerability_reads` records' do + expect(vulnerability_read.traversal_ids).to eq(project.namespace.traversal_ids) + end + + context 'when the project is unarchived' do + it 'marks the associated `vulnerability_reads` record as unarchived' do + expect(vulnerability_read.archived).to be_falsey + end + end - vuln_ids = service_response.payload[:vulnerability_ids] - expect(vuln_ids).not_to be_empty - - vulns_created = Vulnerability.find(vuln_ids) - expect(vulns_created).to match_array([ - have_attributes( - author_id: user.id, - project_id: pipeline.project_id, - state: 'detected', - confidence: 'unknown', - report_type: report_type, - present_on_default_branch: true, - title: advisory.title, - severity: advisory.cvss_v3.severity.downcase, - finding_description: advisory.description, - solution: advisory.solution - ) - ]) + context 'when the project is archived' do + before do + project.update!(archived: true) + end + + it 'marks the associated `vulnerability_reads` record as archived' do + expect(vulnerability_read.archived).to be_truthy + end + end end end diff --git a/ee/spec/services/security/vulnerability_scanning/finding_map_spec.rb b/ee/spec/services/security/vulnerability_scanning/finding_map_spec.rb index 64875331f2beadcdfbe0b2fd590dfc204dae8e69..f96e9fcb739435d797f1aa492340282d34f14179 100644 --- a/ee/spec/services/security/vulnerability_scanning/finding_map_spec.rb +++ b/ee/spec/services/security/vulnerability_scanning/finding_map_spec.rb @@ -32,7 +32,7 @@ it { is_expected.to delegate_method(:uuid).to(:report_finding) } it { is_expected.to delegate_method(:severity).to(:report_finding) } it { is_expected.to delegate_method(:evidence).to(:report_finding) } - it { is_expected.to delegate_method(:project_id).to(:pipeline) } + it { is_expected.to delegate_method(:project).to(:pipeline) } end describe '#identifiers' do diff --git a/ee/spec/services/vulnerabilities/manually_create_service_spec.rb b/ee/spec/services/vulnerabilities/manually_create_service_spec.rb index 0f9a7e740b173031c9fc2966c79033cfa9a4cad4..8db9250752b1750c22cec4be804d05e1bad4dd26 100644 --- a/ee/spec/services/vulnerabilities/manually_create_service_spec.rb +++ b/ee/spec/services/vulnerabilities/manually_create_service_spec.rb @@ -179,6 +179,10 @@ expect(uuids.first).not_to eq(uuids.last) end + it 'sets the `traversal_ids` of the `vulnerability_reads` record' do + expect(vulnerability.vulnerability_read.traversal_ids).to eq(project.namespace.traversal_ids) + end + context "when state fields match state" do let(:params) do { diff --git a/ee/spec/services/vulnerabilities/starboard_vulnerability_create_service_spec.rb b/ee/spec/services/vulnerabilities/starboard_vulnerability_create_service_spec.rb index 67e436b2100405cc992e3714775f9af2a6cbe679..2f5e690afd2c113e03f227f097836c929b184372 100644 --- a/ee/spec/services/vulnerabilities/starboard_vulnerability_create_service_spec.rb +++ b/ee/spec/services/vulnerabilities/starboard_vulnerability_create_service_spec.rb @@ -98,6 +98,10 @@ expect { subject }.to change { agent.reload.has_vulnerabilities }.from(false).to(true) end + it 'sets the `traversal_ids` of the `vulnerability_reads` record' do + expect(vulnerability.vulnerability_read.traversal_ids).to eq(project.namespace.traversal_ids) + end + context 'when there is an identifier for a different project' do let_it_be(:name) { params.dig(:vulnerability, :identifiers).first[:name] } let_it_be(:other_identifier) { create(:vulnerabilities_identifier, name: name) }