diff --git a/ee/app/models/vulnerabilities/read.rb b/ee/app/models/vulnerabilities/read.rb index 1030a0aafba509b6a5000d0d8cd97c4c88aa5f9b..2cac5e118730dd59e95d9579cb13a30226af854c 100644 --- a/ee/app/models/vulnerabilities/read.rb +++ b/ee/app/models/vulnerabilities/read.rb @@ -16,6 +16,9 @@ class Read < ::SecApplicationRecord declarative_enum DismissalReasonEnum + # Included after scopes and relationships to avoid the warning + include BulkInsertSafe + SEVERITY_COUNT_LIMIT = 1001 OWASP_TOP_10_DEFAULT = -1 @@ -42,7 +45,7 @@ class Read < ::SecApplicationRecord validates :report_type, presence: true validates :severity, presence: true validates :state, presence: true - validates :uuid, uniqueness: { case_sensitive: false }, presence: true + validates :uuid, presence: true validates :location_image, length: { maximum: 2048 } validates :has_issues, inclusion: { in: [true, false], message: N_('must be a boolean value') } diff --git a/ee/app/services/security/findings/severity_override_service.rb b/ee/app/services/security/findings/severity_override_service.rb index 185c120cce687c5c8ec68aa8628552490739ec63..8fdc0fdc063209865a1b89771891993b28fd82f3 100644 --- a/ee/app/services/security/findings/severity_override_service.rb +++ b/ee/app/services/security/findings/severity_override_service.rb @@ -52,8 +52,10 @@ def update_severity(vulnerability) create_severity_override_record(vulnerability) vulnerability.update!(severity: @severity) vulnerability.finding.update!(severity: @severity) + + Vulnerabilities::StatisticsUpdateService.update_for(vulnerability) + Vulnerabilities::Reads::UpsertService.new(vulnerability, { severity: @severity }, projects: @project).execute end - Vulnerabilities::StatisticsUpdateService.update_for(vulnerability) end def create_severity_override_record(vulnerability) diff --git a/ee/app/services/security/ingestion/mark_as_resolved_service.rb b/ee/app/services/security/ingestion/mark_as_resolved_service.rb index b8bf5f3ea49f85c039e1d86f04a9dc4b7c9b258f..1d5c55e1026b3bf0aefb191d69db8ea44a3802a4 100644 --- a/ee/app/services/security/ingestion/mark_as_resolved_service.rb +++ b/ee/app/services/security/ingestion/mark_as_resolved_service.rb @@ -91,6 +91,11 @@ def mark_as_no_longer_detected(vulnerabilities) relation.update_all(resolved_on_default_branch: true) end + Vulnerabilities::Reads::UpsertService.new(vulnerabilities_relation, + { resolved_on_default_branch: true }, + projects: project + ).execute + CreateVulnerabilityRepresentationInformation.execute(pipeline, no_longer_detected_vulnerability_ids) track_no_longer_detected_vulnerabilities(no_longer_detected_vulnerability_ids.count) diff --git a/ee/app/services/vulnerabilities/auto_resolve_service.rb b/ee/app/services/vulnerabilities/auto_resolve_service.rb index d6eb1f954e0d6e949a45727430705a0a3efee9fb..257f5b88bb30234d2f266d035ee5643f88768bee 100644 --- a/ee/app/services/vulnerabilities/auto_resolve_service.rb +++ b/ee/app/services/vulnerabilities/auto_resolve_service.rb @@ -99,6 +99,11 @@ def resolve_vulnerabilities Vulnerabilities::BulkEsOperationService.new(vulnerabilities_to_update).execute(&:itself) trigger_webhook_events(vulnerabilities_to_update) end + + Vulnerabilities::Reads::UpsertService.new(vulnerabilities_to_update, + { state: :resolved, auto_resolved: true }, + projects: project + ).execute end Note.transaction do diff --git a/ee/app/services/vulnerabilities/base_state_transition_service.rb b/ee/app/services/vulnerabilities/base_state_transition_service.rb index b481a973687cd71db905420ab562c6b41e29953e..a81858e09c576b6a6d765bf8d08c4465efe90a8f 100644 --- a/ee/app/services/vulnerabilities/base_state_transition_service.rb +++ b/ee/app/services/vulnerabilities/base_state_transition_service.rb @@ -21,23 +21,23 @@ def execute ) update_vulnerability! - update_vulnerability_reads! update_risk_score + + # the dismiss_service does not inherit from the + # BaseStateTransitionService so this check is a + # redundant safety check + if to_state != :dismissed + Vulnerabilities::Reads::UpsertService.new(@vulnerability, + { state: to_state, dismissal_reason: nil }, + projects: @project + ).execute + end end end @vulnerability end - def update_vulnerability_reads! - # the dismiss_service does not inherit from the - # BaseStateTransitionService so this check is a - # redundant safety check - return if to_state == :dismissed - - Vulnerabilities::Read.by_vulnerabilities(@vulnerability).update(dismissal_reason: nil) - end - def update_risk_score return unless Vulnerability.active_states.include?(to_state.to_s) diff --git a/ee/app/services/vulnerabilities/bulk_dismiss_service.rb b/ee/app/services/vulnerabilities/bulk_dismiss_service.rb index c3dd7957abca078c2c9c77494294a788061ec77f..46a039c0a11457e2752ab4c388958c1edf636592 100644 --- a/ee/app/services/vulnerabilities/bulk_dismiss_service.rb +++ b/ee/app/services/vulnerabilities/bulk_dismiss_service.rb @@ -17,14 +17,11 @@ def update(vulnerabilities_ids) return if vulnerability_attrs.empty? db_attributes = db_attributes_for(vulnerability_attrs) + projects = selected_vulnerabilities.with_projects.map(&:project).uniq SecApplicationRecord.transaction do - update_support_tables(selected_vulnerabilities, db_attributes) + update_support_tables(selected_vulnerabilities, db_attributes, projects) selected_vulnerabilities.update_all(db_attributes[:vulnerabilities]) - - SecApplicationRecord.current_transaction.after_commit do - ::Vulnerabilities::BulkEsOperationService.new(selected_vulnerabilities).execute(&:itself) - end end attrs = vulnerability_attrs.map do |id, _, project_id, namespace_id| @@ -45,14 +42,11 @@ def vulnerabilities_to_update(ids) Vulnerability.id_in(ids) end - def update_support_tables(vulnerabilities, db_attributes) + def update_support_tables(vulnerabilities, db_attributes, project) Vulnerabilities::StateTransition.insert_all!(db_attributes[:state_transitions]) - # The `insert_or_update_vulnerability_reads` database trigger does not - # update the dismissal_reason and we are moving away from using - # database triggers to keep tables up to date. - Vulnerabilities::Read - .by_vulnerabilities(vulnerabilities) - .update_all(dismissal_reason: dismissal_reason, auto_resolved: false) + + Vulnerabilities::Reads::UpsertService.new(vulnerabilities, { dismissal_reason: dismissal_reason }, + projects: project).execute end def vulnerabilities_attributes(vulnerabilities) diff --git a/ee/app/services/vulnerabilities/bulk_severity_override_service.rb b/ee/app/services/vulnerabilities/bulk_severity_override_service.rb index 604d40c2d6ef4d238a726eb8f14ef4e0cb5a90e1..58377498ae5497350bc9604ae69356e414ffe54d 100644 --- a/ee/app/services/vulnerabilities/bulk_severity_override_service.rb +++ b/ee/app/services/vulnerabilities/bulk_severity_override_service.rb @@ -30,12 +30,14 @@ def update_vulnerabilities!(vulnerabilities) attributes = vulnerabilities_attributes(vulnerabilities) db_attributes = db_attributes_for(attributes) - SecApplicationRecord.transaction do - update_support_tables(vulnerabilities, db_attributes) + vulnerability_ids_to_be_updated = vulnerabilities.map(&:id) + projects = vulnerabilities.with_projects.map(&:project).uniq + SecApplicationRecord.transaction do vulnerability_ids = vulnerabilities.map(&:id) vulnerabilities.update_all(db_attributes[:vulnerabilities]) + update_support_tables(Vulnerability.by_ids(vulnerability_ids_to_be_updated), db_attributes, projects) SecApplicationRecord.current_transaction.after_commit do vulnerabilities_to_sync = Vulnerability.id_in(vulnerability_ids) @@ -183,9 +185,11 @@ def vulnerabilities_update_attributes } end - def update_support_tables(vulnerabilities, db_attributes) + def update_support_tables(vulnerabilities, db_attributes, projects) Vulnerabilities::Finding.by_vulnerability(vulnerabilities).update_all(severity: @new_severity, updated_at: now) Vulnerabilities::SeverityOverride.insert_all!(db_attributes[:severity_overrides]) + Vulnerabilities::Reads::UpsertService.new(vulnerabilities, { severity: @new_severity }, + projects: projects).execute end def system_note_metadata_action diff --git a/ee/app/services/vulnerabilities/create_service.rb b/ee/app/services/vulnerabilities/create_service.rb index 77a369639e0deee39145750283a1540ee4a49b00..16e34d72e7f51561a8cbc92bb4473a287b2786f6 100644 --- a/ee/app/services/vulnerabilities/create_service.rb +++ b/ee/app/services/vulnerabilities/create_service.rb @@ -40,8 +40,15 @@ def execute end if vulnerability.persisted? + attributes = {} + attributes[:dismissal_reason] = @dismissal_reason if @dismissal_reason + Vulnerabilities::StatisticsUpdateService.update_for(vulnerability) Vulnerabilities::Findings::RiskScoreCalculationService.calculate_for(vulnerability) + + if @present_on_default_branch + Vulnerabilities::Reads::UpsertService.new(vulnerability, attributes, projects: @project).execute + end end vulnerability diff --git a/ee/app/services/vulnerabilities/dismiss_service.rb b/ee/app/services/vulnerabilities/dismiss_service.rb index 696cfe8d35b9ce56582cc863961225624fccac37..0481a18418100a7d5c9bb6115ea3d32b498c1dc6 100644 --- a/ee/app/services/vulnerabilities/dismiss_service.rb +++ b/ee/app/services/vulnerabilities/dismiss_service.rb @@ -28,7 +28,6 @@ def execute author: @user ) - Vulnerabilities::Read.by_vulnerabilities(@vulnerability).update(dismissal_reason: @dismissal_reason) rescue ActiveRecord::RecordInvalid => invalid errors = invalid.record.errors messages = errors.full_messages.join @@ -45,6 +44,9 @@ def execute end end + Vulnerabilities::Reads::UpsertService.new(@vulnerability, + { state: :dismissed, dismissal_reason: @dismissal_reason }, projects: @project).execute + @vulnerability end diff --git a/ee/app/services/vulnerabilities/find_or_create_from_security_finding_service.rb b/ee/app/services/vulnerabilities/find_or_create_from_security_finding_service.rb index 7f0eebdf0a90c805656720e62efab6183d9b05d3..ec0c5a96f822a1f1c8f095db261fc104047391b2 100644 --- a/ee/app/services/vulnerabilities/find_or_create_from_security_finding_service.rb +++ b/ee/app/services/vulnerabilities/find_or_create_from_security_finding_service.rb @@ -48,7 +48,7 @@ def find_or_create_vulnerability(vulnerability_finding) end def update_state_for(vulnerability) - vulnerability.transaction do + Vulnerability.transaction do state_transition_params = { vulnerability: vulnerability, from_state: vulnerability.state, @@ -57,7 +57,9 @@ def update_state_for(vulnerability) } state_transition_params[:comment] = params[:comment] if params[:comment] - state_transition_params[:dismissal_reason] = params[:dismissal_reason] if params[:dismissal_reason] + + dismissal_reason = params[:dismissal_reason] if params[:dismissal_reason] + state_transition_params[:dismissal_reason] = dismissal_reason Vulnerabilities::StateTransition.create!(state_transition_params) @@ -67,7 +69,8 @@ def update_state_for(vulnerability) vulnerability.update!(update_params) if params[:dismissal_reason] - vulnerability.vulnerability_read&.update!(dismissal_reason: params[:dismissal_reason]) + Vulnerabilities::Reads::UpsertService.new(vulnerability, + { dismissal_reason: dismissal_reason, state: @state }, projects: @project).execute end create_system_note(vulnerability, @current_user) @@ -84,11 +87,12 @@ def update_existing_state_transition(vulnerability) state_transition = vulnerability.state_transitions.by_to_states(:dismissed).last return unless state_transition && (params[:comment] || params[:dismissal_reason]) - vulnerability.transaction do + Vulnerability.transaction do state_transition.update!(params.slice(:comment, :dismissal_reason).compact) - if params[:dismissal_reason] - vulnerability.vulnerability_read&.update!(dismissal_reason: params[:dismissal_reason]) + if @present_on_default_branch && params[:dismissal_reason] + Vulnerabilities::Reads::UpsertService.new(vulnerability, + { dismissal_reason: params[:dismissal_reason] }, projects: @project).execute end end end diff --git a/ee/app/services/vulnerabilities/manually_create_service.rb b/ee/app/services/vulnerabilities/manually_create_service.rb index 716bfba145a8e0a0b07d64adfac29abf37191d42..2ee993d0235d57e69d4b23788d7aad9aa85db33e 100644 --- a/ee/app/services/vulnerabilities/manually_create_service.rb +++ b/ee/app/services/vulnerabilities/manually_create_service.rb @@ -39,7 +39,8 @@ def execute vulnerability.save! finding.update!(vulnerability_id: vulnerability.id) - vulnerability.vulnerability_read.update!(traversal_ids: project.namespace.traversal_ids) + Vulnerabilities::Reads::UpsertService.new(vulnerability, + { traversal_ids: project.namespace.traversal_ids }, projects: @project).execute update_security_statistics! diff --git a/ee/app/services/vulnerabilities/reads/upsert_service.rb b/ee/app/services/vulnerabilities/reads/upsert_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..7a36f8d5d6aff4ba255454154e5c3627b9d19b2e --- /dev/null +++ b/ee/app/services/vulnerabilities/reads/upsert_service.rb @@ -0,0 +1,149 @@ +# ee/app/services/vulnerabilities/reads/upsert_service.rb +# frozen_string_literal: true + +module Vulnerabilities + module Reads + # This class will update vulnerabilty reads for a set of corresponding vulnerability records + # according to the passed attributes set. + # + # If any of the passed vulnerabilities does not have a corresponding read, it will ensure it + # exists in the created state. + # + # If no attributes are passed, it will only ensure that the vulnerabilities have corresponding + # vulnerability reads created. + + class UpsertService + include BaseServiceUtility + + BATCH_SIZE = 1000 + + def initialize(vulnerabilities, attributes = {}, projects: []) + @attributes = attributes + @batch_size = BATCH_SIZE + @vulnerabilities = ensure_vulnerability_relation(vulnerabilities) + # Project is only needed for feature flag purposes + @projects = Array(projects) + end + + def execute + return if @vulnerabilities.empty? + + # Until we globally enable the FF, we have to filter off vulns for projects where the FF is is not on so + # so that we can make sure to run the service for projects where the trigger is switched off, else we'll + # cause data inconsistencies. + @projects = @projects.select do |p| + Feature.enabled?(:turn_off_vulnerability_read_create_db_trigger_function, p) + end + @vulnerabilities = @vulnerabilities.with_project(@projects) + + @vulnerabilities.each_batch(of: @batch_size) do |vulnerability_batch| + vulns_missing_reads = vulnerability_batch.left_joins(:vulnerability_read) + .merge(Vulnerabilities::Read.by_vulnerabilities(nil)) + .pluck_primary_key + new_read_ids = create_missing_reads(vulns_missing_reads) + + next unless attributes.any? + + vulnerability_read_batch = Vulnerabilities::Read.by_vulnerabilities(vulnerability_batch) + .id_not_in(new_read_ids) + + vulnerability_read_batch.update_all(attributes) + + SecApplicationRecord.current_transaction.after_commit do + Vulnerabilities::BulkEsOperationService.new(vulnerability_batch).execute(&:itself) + end + end + + success + end + + private + + attr_reader :attributes, :batch_size + + # We do this to avoid nesting a subquery of the vulnerabilities in itself if passed a relation. + def ensure_vulnerability_relation(vulnerabilities_parameter) + if vulnerabilities_parameter.is_a?(ActiveRecord::Relation) + vulnerabilities_parameter.dup + else + Vulnerability.by_ids(vulnerabilities_parameter) + end + end + + def create_missing_reads(missing_read_vuln_ids) + return [] if missing_read_vuln_ids.empty? + + # rubocop:disable CodeReuse/ActiveRecord -- This is a very specific set of eager and pre loads needed for + # building a full vulnerability record in as few queries as possible, but needing to cross DB boundaries. + missing_vulnerability_batch = Vulnerability.by_ids(missing_read_vuln_ids) + .eager_load( + :issue_links, + :merge_request_links, + findings: [ + :remediations, + :scanner, + :identifiers, + { finding_identifiers: :identifier } + ] + ) + .preload( + :notes, + :merge_requests, + :related_issues, + project: [ + :route, + { project_namespace: :route }, + { namespace: :route } + ] + ) + # rubocop:enable CodeReuse/ActiveRecord + + new_read_ids = perform_bulk_insert(build_vulnerability_reads_batch(missing_vulnerability_batch)) + ::SecApplicationRecord.current_transaction.after_commit do + Vulnerabilities::BulkEsOperationService.new(missing_vulnerability_batch).execute(&:itself) + end + new_read_ids + end + + def build_vulnerability_reads_batch(vulnerability_batch) + vulnerability_batch.filter_map do |vulnerability| + next unless vulnerability.present_on_default_branch? + next unless vulnerability.finding + + build_vulnerability_read(vulnerability) + end + end + + def perform_bulk_insert(vulnerability_reads_for_insert) + ::Vulnerabilities::Read.bulk_upsert!( + vulnerability_reads_for_insert, + unique_by: %i[uuid], + returns: :ids + ) + end + + def build_vulnerability_read(vulnerability) + ::Vulnerabilities::Read.new( + { + vulnerability_id: vulnerability.id, + project_id: vulnerability.project_id, + uuid: vulnerability.finding.uuid, + scanner_id: vulnerability.finding.scanner_id, + severity: vulnerability.severity, + state: vulnerability.state, + report_type: vulnerability.report_type, + resolved_on_default_branch: vulnerability.resolved_on_default_branch, + # This map won't cause an N+1 thanks to the identifier eager_load earlier in the service + identifier_names: vulnerability.finding.identifiers.map(&:name), + location_image: vulnerability.finding.location['image'], + has_remediations: vulnerability.has_remediations?, + cluster_agent_id: vulnerability.location.dig('kubernetes_resource', 'agent_id'), + traversal_ids: vulnerability.project.namespace.traversal_ids, + archived: vulnerability.project.archived?, + auto_resolved: vulnerability.auto_resolved? + }.merge!(@attributes) + ) + end + end + end +end diff --git a/ee/app/services/vulnerabilities/resolve_service.rb b/ee/app/services/vulnerabilities/resolve_service.rb index 120305a53b8a5e14b1ee0c77cdbd14dc82955b99..26d19dcaefea1ab678eaf02520083b34c8717d24 100644 --- a/ee/app/services/vulnerabilities/resolve_service.rb +++ b/ee/app/services/vulnerabilities/resolve_service.rb @@ -24,6 +24,9 @@ def update_vulnerability! namespace: @vulnerability.project.namespace, project: @vulnerability.project ) + + Vulnerabilities::Reads::UpsertService.new(@vulnerability, + { state: :resolved, auto_resolved: @auto_resolved }, projects: @project).execute end end diff --git a/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb b/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb index 0044feb2f94c17338c68d633f09a55020460b0dc..c23fa8091789eafa357b1d4cef910c0716a01d32 100644 --- a/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb +++ b/ee/app/services/vulnerabilities/starboard_vulnerability_create_service.rb @@ -45,7 +45,14 @@ def execute vulnerability.save! finding.update!(vulnerability_id: vulnerability.id) - vulnerability.vulnerability_read.update!(traversal_ids: project.namespace.traversal_ids) + Vulnerabilities::Reads::UpsertService.new(vulnerability, + { + cluster_agent_id: @agent.id, + traversal_ids: project.namespace.traversal_ids, + identifier_names: identifiers.map(&:name) + }, + projects: @project).execute + update_security_statistics! Vulnerabilities::StatisticsUpdateService.update_for(vulnerability) diff --git a/ee/app/services/vulnerabilities/starboard_vulnerability_resolve_service.rb b/ee/app/services/vulnerabilities/starboard_vulnerability_resolve_service.rb index c1758f0d47d2f64df1b0aa3c1dc155ae96bf38c0..391ccc1d4d8a9b9ce5203c87c64ead98ad650e0b 100644 --- a/ee/app/services/vulnerabilities/starboard_vulnerability_resolve_service.rb +++ b/ee/app/services/vulnerabilities/starboard_vulnerability_resolve_service.rb @@ -26,7 +26,11 @@ def execute undetected.each_batch(of: BATCH_SIZE) do |batch| Vulnerabilities::BulkEsOperationService.new(batch).execute do |vulnerabilities| - vulnerabilities.update_all(resolved_on_default_branch: true, state: :resolved, updated_at: now) + Vulnerability.transaction do + vulnerabilities.update_all(resolved_on_default_branch: true, state: :resolved, updated_at: now) + Vulnerabilities::Reads::UpsertService.new(vulnerabilities, + { resolved_on_default_branch: true, state: :resolved }, projects: @project).execute + end end end diff --git a/ee/app/services/vulnerabilities/update_service.rb b/ee/app/services/vulnerabilities/update_service.rb index 3ccb33f69832d14b8890813f2e53ccdf4254d484..d8ddd4aaf1d6796b9c6d3445ccf0c3b7f0a647aa 100644 --- a/ee/app/services/vulnerabilities/update_service.rb +++ b/ee/app/services/vulnerabilities/update_service.rb @@ -18,9 +18,15 @@ def initialize(project, author, finding:, resolved_on_default_branch: nil) def execute raise Gitlab::Access::AccessDeniedError unless can?(author, :admin_vulnerability, project) - vulnerability.update!(vulnerability_params) - Vulnerabilities::StatisticsUpdateService.update_for(vulnerability) - Vulnerabilities::Findings::RiskScoreCalculationService.calculate_for(vulnerability) + Vulnerability.transaction do + vulnerability.update!(vulnerability_params) + attributes = {} + attributes[:resolved_on_default_branch] = resolved_on_default_branch if @resolved_on_default_branch + + Vulnerabilities::Reads::UpsertService.new(vulnerability, attributes, projects: project).execute + Vulnerabilities::StatisticsUpdateService.update_for(vulnerability) + Vulnerabilities::Findings::RiskScoreCalculationService.calculate_for(vulnerability) + end vulnerability end diff --git a/ee/app/services/vulnerability_issue_links/bulk_create_service.rb b/ee/app/services/vulnerability_issue_links/bulk_create_service.rb index 9702268945279da64fbeebc3d1e659b5e8508120..9a079485b272751ec4488322ea9cb43140a80fbc 100644 --- a/ee/app/services/vulnerability_issue_links/bulk_create_service.rb +++ b/ee/app/services/vulnerability_issue_links/bulk_create_service.rb @@ -13,7 +13,11 @@ def execute return ServiceResponse.error(message: "No Issue given") unless @issue attributes = issue_links_attributes(@issue, @vulnerabilities) - issue_links = bulk_insert_issue_links(attributes) + + issue_links = Vulnerability.transaction do + Vulnerabilities::Reads::UpsertService.new(@vulnerabilities, { has_issues: true }, projects: @project).execute + bulk_insert_issue_links(attributes) + end ServiceResponse.success( payload: { issue_links: issue_links } diff --git a/ee/app/services/vulnerability_issue_links/create_service.rb b/ee/app/services/vulnerability_issue_links/create_service.rb index 2ccaac484cff22f88d797cb932340bbbd7bd6c67..2a3e82d1d553adf9cff49c0e943897bea9fcb354 100644 --- a/ee/app/services/vulnerability_issue_links/create_service.rb +++ b/ee/app/services/vulnerability_issue_links/create_service.rb @@ -12,7 +12,7 @@ def initialize(user, vulnerability, issue, link_type: Vulnerabilities::IssueLink def execute raise Gitlab::Access::AccessDeniedError unless can?(@user, :admin_vulnerability_issue_link, issue_link) - if issue_link.save + if save success else error @@ -21,6 +21,14 @@ def execute private + def save + ::Vulnerability.transaction do + raise ActiveRecord::Rollback unless issue_link.save + + Vulnerabilities::Reads::UpsertService.new(@vulnerability, { has_issues: true }, projects: @project).execute + end + end + def issue_link @issue_link ||= Vulnerabilities::IssueLink.new(vulnerability: @vulnerability, issue: @issue, link_type: @link_type) end diff --git a/ee/app/services/vulnerability_issue_links/delete_service.rb b/ee/app/services/vulnerability_issue_links/delete_service.rb index 0b2fbc663e9164037aaf6bf8ec30e1846b6bb648..a66810c86fb6cf597258fc7649c969f80da861cb 100644 --- a/ee/app/services/vulnerability_issue_links/delete_service.rb +++ b/ee/app/services/vulnerability_issue_links/delete_service.rb @@ -10,7 +10,15 @@ def initialize(user, vulnerability_issue_link) def execute raise Gitlab::Access::AccessDeniedError unless can?(user, :admin_vulnerability_issue_link, link) - link.destroy! + vulnerability = link.vulnerability + + ::Vulnerability.transaction do + link.destroy! + Vulnerabilities::Reads::UpsertService.new(vulnerability, + { has_issues: vulnerability.issue_links.exists? }, + projects: @project + ).execute + end success end diff --git a/ee/app/services/vulnerability_merge_request_links/create_service.rb b/ee/app/services/vulnerability_merge_request_links/create_service.rb index c4912606232cbb63c3623d10274dad9a01dcf750..94c14ebc6ee3370d6f4b4d08c0b725e08ab57783 100644 --- a/ee/app/services/vulnerability_merge_request_links/create_service.rb +++ b/ee/app/services/vulnerability_merge_request_links/create_service.rb @@ -9,6 +9,8 @@ def execute return max_vulnerabilities_error if merge_request_links_limit_exceeded? if merge_request_link.save + Vulnerabilities::Reads::UpsertService.new(vulnerability, { has_merge_request: true }, + projects: @project).execute success_response else error_response @@ -17,9 +19,15 @@ def execute private + def vulnerability + @vulnerability ||= params[:vulnerability] + end + def merge_request_link - @merge_request_link ||= Vulnerabilities::MergeRequestLink.new(vulnerability: params[:vulnerability], - merge_request: params[:merge_request]) + @merge_request_link ||= Vulnerabilities::MergeRequestLink.new( + vulnerability: vulnerability, + merge_request: params[:merge_request] + ) end def merge_request_links_limit_exceeded? diff --git a/ee/spec/models/vulnerabilities/read_spec.rb b/ee/spec/models/vulnerabilities/read_spec.rb index 1235e1f6204fab68ce27aae6b6bd565a74b24251..726b062d5d0adb611540d532596c408072e20625 100644 --- a/ee/spec/models/vulnerabilities/read_spec.rb +++ b/ee/spec/models/vulnerabilities/read_spec.rb @@ -28,7 +28,6 @@ it { is_expected.to validate_length_of(:location_image).is_at_most(2048) } it { is_expected.to validate_uniqueness_of(:vulnerability_id) } - it { is_expected.to validate_uniqueness_of(:uuid).case_insensitive } end describe 'triggers' do diff --git a/ee/spec/requests/api/graphql/mutations/security/finding/severity_override_spec.rb b/ee/spec/requests/api/graphql/mutations/security/finding/severity_override_spec.rb index f90b01c8193ac381fd7439f600371ed24499d146..3d4f01f2256ce2212bdb5dadcf39ed7670b672d0 100644 --- a/ee/spec/requests/api/graphql/mutations/security/finding/severity_override_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/security/finding/severity_override_spec.rb @@ -102,7 +102,9 @@ def create_mutation(mutation_input) context 'when the severity override succeeds' do it 'returns the security finding with updated severity' do - post_graphql_mutation(mutation, current_user: current_user) + Gitlab::QueryLimiting.with_suppressed do + post_graphql_mutation(mutation, current_user: current_user) + end expect(response_finding).to match(expected_finding) expect(mutation_response['errors']).to be_empty diff --git a/ee/spec/services/security/findings/severity_override_service_spec.rb b/ee/spec/services/security/findings/severity_override_service_spec.rb index 9d1afe0312ba8740a7374db13bfeef75dcb1c955..725053e6dbe660b9649d4ddf588e996840f1658a 100644 --- a/ee/spec/services/security/findings/severity_override_service_spec.rb +++ b/ee/spec/services/security/findings/severity_override_service_spec.rb @@ -69,6 +69,7 @@ def override_severity(severity: new_severity) expect { execute }.to change { Vulnerability.count }.by(1) .and change { Vulnerabilities::Finding.count }.by(1) .and not_change { Vulnerabilities::SeverityOverride.count } + .and not_change { Vulnerabilities::Read.count } end it 'doesnt create audit event' do @@ -84,6 +85,7 @@ def override_severity(severity: new_severity) expect { execute }.to change { Vulnerability.count }.by(1) .and change { Vulnerabilities::Finding.count }.by(1) .and change { Vulnerabilities::SeverityOverride.count }.by(1) + .and not_change { Vulnerabilities::Read.count } expect(security_finding.reload.vulnerability.severity).to eq(new_severity) expect(security_finding.vulnerability.finding.severity).to eq(new_severity) @@ -143,6 +145,14 @@ def override_severity(severity: new_severity) end end + it 'updates the vulnerability read record with the new severity' do + vulnerability = security_finding.reload.vulnerability + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, severity: previous_severity) + + expect { execute }.to change { vulnerability_read.reload.severity }.from(previous_severity).to(new_severity) + end + it_behaves_like 'creates project audit event' end end diff --git a/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb b/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb index 7456bdbb8980c18bf3d7413e7d2c25fc4847ce22..7169fa99a3fbebe7a5434f82137a21978af6cb44 100644 --- a/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb +++ b/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb @@ -50,6 +50,7 @@ def expect_vulnerability_not_to_be_resolved(vulnerability) command.execute expect_vulnerability_to_be_resolved(vulnerability.reload) + expect(vulnerability.vulnerability_read.resolved_on_default_branch).to be_truthy end context 'when there is a no longer detected vulnerability' do @@ -139,6 +140,8 @@ def expect_vulnerability_not_to_be_resolved(vulnerability) # Finally, check that both vulnerabilities are still resolved_on_default_branch as before. expect(vulnerability.reload).to be_resolved_on_default_branch expect(second_vulnerability.reload).to be_resolved_on_default_branch + expect(vulnerability.vulnerability_read.resolved_on_default_branch).to be_truthy + expect(second_vulnerability.vulnerability_read.resolved_on_default_branch).to be_truthy end context 'when AutoResolveService returns an error' do @@ -195,6 +198,8 @@ def expect_vulnerability_not_to_be_resolved(vulnerability) command.execute expect(vulnerability.reload).to be_resolved_on_default_branch + expect(vulnerability.vulnerability_read.resolved_on_default_branch).to be_truthy + representation_info = Vulnerabilities::RepresentationInformation .find_or_initialize_by(vulnerability_id: vulnerability.id) representation_info.update!( diff --git a/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb b/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb index 22c0d10ebc490b8c9559d9476c11b486409159b3..83f0fb8a24b98e8f28e3f55075f9824d97274fa0 100644 --- a/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb +++ b/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb @@ -123,7 +123,7 @@ end it 'triggers webhook events for resolved vulnerabilities' do - expect_next_found_instance_of(Vulnerability) do |vulnerability| + expect_any_instance_of(Vulnerability) do |vulnerability| expect(vulnerability).to receive(:trigger_webhook_event) end @@ -265,5 +265,66 @@ expect(ordered_vulnerabilities[1]).to be_auto_resolved end end + + context 'when resolving vulnerabilities' do + it 'updates the vulnerability read records with correct state and auto_resolved flag' do + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, + vulnerability: vulnerability, + state: 'detected', + auto_resolved: false) + + vulnerability_read.update!(state: 'detected', auto_resolved: false) + + service.execute + + vulnerability_read.reload + expect(vulnerability_read.state).to eq('resolved') + expect(vulnerability_read.auto_resolved).to be(true) + end + + it 'updates multiple vulnerability read records when resolving multiple vulnerabilities' do + vulnerabilities = create_list(:vulnerability, 2, :with_findings, :detected, project: project) + vulnerability_ids = vulnerabilities.map(&:id) + + vulnerability_reads = vulnerabilities.map do |vuln| + read = Vulnerabilities::Read.find_by(vulnerability_id: vuln.id) || + create(:vulnerability_read, vulnerability: vuln) + + read.update!(state: 'detected', auto_resolved: false) + read + end + + described_class.new(pipeline, vulnerability_ids, budget).execute + + vulnerability_reads.each do |read| + read.reload + expect(read.state).to eq('resolved') + expect(read.auto_resolved).to be(true) + end + end + + it 'respects the budget when updating vulnerability reads' do + vulnerabilities = create_list(:vulnerability, 3, :with_findings, :detected, project: project) + vulnerability_ids = vulnerabilities.map(&:id) + + vulnerability_reads = vulnerabilities.map do |vuln| + read = Vulnerabilities::Read.find_by(vulnerability_id: vuln.id) || + create(:vulnerability_read, vulnerability: vuln) + + read.update!(state: 'detected', auto_resolved: false) + read + end + + described_class.new(pipeline, vulnerability_ids, 2).execute + + updated_reads = vulnerability_reads.select { |read| read.reload.state == 'resolved' } + expect(updated_reads.count).to eq(2) + + updated_reads.each do |read| + expect(read.auto_resolved).to be(true) + end + end + end end end diff --git a/ee/spec/services/vulnerabilities/bulk_severity_override_service_spec.rb b/ee/spec/services/vulnerabilities/bulk_severity_override_service_spec.rb index 2ea2ce0ce4937a28fbdf97ed57dd0a0a01ecf878..65b650342111554337987e51c0ece20a65fa68f1 100644 --- a/ee/spec/services/vulnerabilities/bulk_severity_override_service_spec.rb +++ b/ee/spec/services/vulnerabilities/bulk_severity_override_service_spec.rb @@ -195,6 +195,16 @@ expect(result.payload[:vulnerabilities].count).to eq(vulnerability_ids.count) end + it 'updates vulnerability read records with the new severity' do + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, severity: original_severity) + + service.execute + + vulnerability_read.reload + expect(vulnerability_read.severity).to eq(new_severity) + end + it 'creates audit events for each vulnerability', :request_store do expect { service.execute }.to change { AuditEvent.count }.by(1) diff --git a/ee/spec/services/vulnerabilities/create_service_spec.rb b/ee/spec/services/vulnerabilities/create_service_spec.rb index b128658aab9b3408d1071c2a05b9cc7fe0f4282f..0c458de4e1e894adb7999653939c4ac44e3c8677 100644 --- a/ee/spec/services/vulnerabilities/create_service_spec.rb +++ b/ee/spec/services/vulnerabilities/create_service_spec.rb @@ -75,6 +75,19 @@ )) end + it 'creates vulnerability read record when present_on_default_branch is true' do + expect { subject }.to change { Vulnerabilities::Read.count }.by(1) + + vulnerability_read = Vulnerabilities::Read.last + expect(vulnerability_read).to have_attributes( + vulnerability_id: vulnerability.id, + project_id: project.id, + severity: finding.severity, + state: finding.state, + report_type: finding.report_type + ) + end + it_behaves_like 'creates a vulnerability state transition record with note' it 'creates a vulnerability_finding_risk_scores record' do @@ -96,6 +109,19 @@ expect(vulnerability.dismissed_by_id).to eq(user.id) end end + + it 'creates vulnerability read record with dismissed state' do + expect { subject }.to change { Vulnerabilities::Read.count }.by(1) + + vulnerability_read = Vulnerabilities::Read.last + expect(vulnerability_read).to have_attributes( + vulnerability_id: vulnerability.id, + project_id: project.id, + severity: finding.severity, + state: 'dismissed', + report_type: finding.report_type + ) + end end end diff --git a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb index 2f855182537fb8c0176591d1f3dbd555bdfd4adc..0396e1f99c880e9d8a8aa7fd8e2f2cb0ff4ce5a1 100644 --- a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb +++ b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb @@ -15,7 +15,7 @@ let!(:pipeline) { create(:ee_ci_pipeline, :with_dast_report, :success, project: project) } let!(:build) { create(:ee_ci_build, :sast, pipeline: pipeline) } let(:state) { :detected } - let(:vulnerability) { create(:vulnerability, state, :with_findings, project: project) } + let(:vulnerability) { create(:vulnerability, state, :with_finding, project: project) } let(:state_transition) { create(:vulnerability_state_transition, vulnerability: vulnerability) } let(:dismiss_findings) { true } let(:comment) { nil } @@ -37,7 +37,7 @@ end context 'when a vulnerability read record does not exist' do - let(:vulnerability) { create(:vulnerability, :detected, :with_findings, project: project, present_on_default_branch: false) } + let(:vulnerability) { create(:vulnerability, :detected, :with_finding, project: project, present_on_default_branch: false) } let(:vulnerability_read) { vulnerability.vulnerability_read } it 'does not fail' do diff --git a/ee/spec/services/vulnerabilities/find_or_create_from_security_finding_service_spec.rb b/ee/spec/services/vulnerabilities/find_or_create_from_security_finding_service_spec.rb index eb3a3f01c489c23326e1cb4c2cc9284a285e5cc1..2029630e8f3f0f9195134926d553027854d41174 100644 --- a/ee/spec/services/vulnerabilities/find_or_create_from_security_finding_service_spec.rb +++ b/ee/spec/services/vulnerabilities/find_or_create_from_security_finding_service_spec.rb @@ -47,6 +47,7 @@ let_it_be(:security_finding) { create(:security_finding) } context 'when vulnerability is present on default branch' do + let(:present_on_default_branch) { true } let!(:vulnerability) do create( :vulnerability, diff --git a/ee/spec/services/vulnerabilities/manually_create_service_spec.rb b/ee/spec/services/vulnerabilities/manually_create_service_spec.rb index 0d8b10900d79fa1aa45692ead868607630807535..ca6174a707cbae7ace9f0096d316bd405956ab15 100644 --- a/ee/spec/services/vulnerabilities/manually_create_service_spec.rb +++ b/ee/spec/services/vulnerabilities/manually_create_service_spec.rb @@ -160,6 +160,9 @@ expect(finding.raw_metadata).to eq("{}") expect(vulnerability.finding_id).to eq(finding.id) + expect(vulnerability.vulnerability_read.state).to eq(params.dig(:vulnerability, :state)) + expect(vulnerability.vulnerability_read.severity).to eq(params.dig(:vulnerability, :severity)) + scanner = finding.scanner expect(scanner.name).to eq(params.dig(:vulnerability, :scanner, :name)) diff --git a/ee/spec/services/vulnerabilities/reads/upsert_service_spec.rb b/ee/spec/services/vulnerabilities/reads/upsert_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..6a618a8699c0b847415fcacb13223d3ff4bf84d3 --- /dev/null +++ b/ee/spec/services/vulnerabilities/reads/upsert_service_spec.rb @@ -0,0 +1,394 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Vulnerabilities::Reads::UpsertService, feature_category: :vulnerability_management do + let_it_be(:namespace) { create(:namespace) } + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, namespace: namespace) } + let_it_be(:scanner) { create(:vulnerabilities_scanner, project: project) } + + let(:vulnerability) do + create(:vulnerability, + project: project, + author: user, + severity: :high, + state: :detected, + resolved_on_default_branch: false, + present_on_default_branch: true) + end + + let!(:finding) do + create(:vulnerabilities_finding, + project: project, + scanner: scanner, + vulnerability: vulnerability, + location: { 'image' => 'alpine:3.4' }) + end + + describe '#execute' do + before do + Vulnerabilities::Read.where(vulnerability: vulnerability).delete_all + end + + let(:execute_service) { described_class.new(vulnerabilities, attributes, projects: project).execute } + + context 'with single vulnerability' do + let(:vulnerabilities) { vulnerability } + let(:attributes) { {} } + + context 'when vulnerability is present on default branch' do + context 'when no vulnerability_read exists' do + it 'creates a vulnerability_read record' do + expect { execute_service }.to change { Vulnerabilities::Read.count }.by(1) + end + + it 'sets correct attributes from vulnerability' do + execute_service + created_read = Vulnerabilities::Read.find_by(vulnerability: vulnerability) + + expect(created_read).to have_attributes( + vulnerability_id: vulnerability.id, + project_id: vulnerability.project_id, + scanner_id: finding.scanner_id, + report_type: vulnerability.report_type, + severity: vulnerability.severity, + state: vulnerability.state, + resolved_on_default_branch: vulnerability.resolved_on_default_branch, + uuid: finding.uuid_v5, + location_image: 'alpine:3.4', + identifier_names: finding.identifiers.pluck(:name), + has_remediations: vulnerability.has_remediations? + ) + end + + it 'handles cluster_agent_id from vulnerability location' do + finding.update!(location: { 'kubernetes_resource' => { 'agent_id' => '123' } }) + + execute_service + created_read = Vulnerabilities::Read.find_by(vulnerability: vulnerability) + + expect(created_read.cluster_agent_id).to eq('123') + end + end + + context 'when vulnerability_read already exists' do + let!(:existing_read) do + create(:vulnerability_read, + vulnerability: vulnerability, + project: project, + scanner: scanner, + severity: :low, + state: :dismissed, + uuid: finding.uuid_v5, + report_type: vulnerability.report_type, + resolved_on_default_branch: false) + end + + it 'updates the existing record without creating a new one' do + expect { execute_service }.not_to change { Vulnerabilities::Read.count } + end + + it 'updates only changed attributes' do + described_class.new(vulnerability, { severity: :critical }, projects: project).execute + + expect(existing_read.reload.severity).to eq('critical') + end + end + end + + context 'when vulnerability is not present on default branch' do + before do + vulnerability.update!(present_on_default_branch: false) + end + + it 'does not create or update any records' do + expect { execute_service }.not_to change { Vulnerabilities::Read.count } + end + end + + context 'when vulnerability has no finding' do + before do + vulnerability.finding.destroy! + vulnerability.reload + end + + it 'does not create or update any records' do + expect { execute_service }.not_to change { Vulnerabilities::Read.count } + end + end + end + + context 'with multiple vulnerabilities' do + let(:vulnerabilities) { [vulnerability, vulnerability2] } + let(:attributes) { {} } + let(:created_reads) { Vulnerabilities::Read.where(vulnerability: vulnerabilities) } + + let(:vulnerability2) do + create(:vulnerability, + project: project, + author: user, + severity: :medium, + state: :confirmed, + present_on_default_branch: true) + end + + let!(:finding2) do + create(:vulnerabilities_finding, + project: project, + scanner: scanner, + vulnerability: vulnerability2) + end + + before do + Vulnerabilities::Read.where(vulnerability_id: [vulnerability.id, vulnerability2.id]).delete_all + end + + it 'processes all valid vulnerabilities' do + expect { execute_service }.to change { Vulnerabilities::Read.count }.by(2) + end + + it 'creates vulnerability reads for all valid vulnerabilities' do + execute_service + + expect(created_reads.pluck(:vulnerability_id)).to contain_exactly(vulnerability.id, vulnerability2.id) + end + + it 'skips invalid vulnerabilities' do + vulnerability2.update!(present_on_default_branch: false) + + execute_service + + created_read = Vulnerabilities::Read.find_by(vulnerability: vulnerability) + expect(created_read.vulnerability_id).to eq(vulnerability.id) + expect(Vulnerabilities::Read.find_by(vulnerability: vulnerability2)).to be_nil + end + end + + context 'with custom attributes' do + let(:vulnerabilities) { vulnerability } + let(:attributes) { { dismissal_reason: 'used_in_tests', has_issues: true } } + + context 'when creating new record' do + it 'applies custom attributes during creation' do + execute_service + created_read = Vulnerabilities::Read.find_by(vulnerability: vulnerability) + + expect(created_read).to have_attributes( + dismissal_reason: 'used_in_tests', + has_issues: true + ) + end + end + + context 'when updating existing record' do + let!(:existing_read) do + create(:vulnerability_read, + vulnerability: vulnerability, + project: project, + scanner: scanner, + severity: :low, + state: :dismissed, + uuid: finding.uuid_v5, + report_type: vulnerability.report_type, + resolved_on_default_branch: false) + end + + it 'updates with custom attributes' do + expect(existing_read.reload).to have_attributes( + dismissal_reason: nil, + has_issues: false + ) + + execute_service + + expect(existing_read.reload).to have_attributes( + dismissal_reason: 'used_in_tests', + has_issues: true + ) + end + end + end + + context 'with attribute update conditions' do + let(:vulnerabilities) { vulnerability } + let(:attributes) { { severity: :critical, state: :resolved, resolved_on_default_branch: true } } + + let!(:existing_read) do + create(:vulnerability_read, + vulnerability: vulnerability, + project: project, + scanner: scanner, + severity: :low, + state: :dismissed, + uuid: finding.uuid_v5, + report_type: vulnerability.report_type, + resolved_on_default_branch: false) + end + + it 'updates multiple attributes' do + execute_service + + expect(existing_read.reload).to have_attributes( + severity: 'critical', + state: 'resolved', + resolved_on_default_branch: true + ) + end + + context 'with explicit nil attributes' do + let(:attributes) { { dismissal_reason: nil } } + + it 'updates explicit nil attributes' do + existing_read.update!(dismissal_reason: 'used_in_tests') + + execute_service + + expect(existing_read.reload.dismissal_reason).to be_nil + end + end + + context 'with other supported attributes' do + let(:attributes) do + { + has_issues: true, + has_merge_request: true, + traversal_ids: [1, 2, 3], + has_remediations: true, + archived: true, + auto_resolved: true, + identifier_names: ['CVE-2023-1234'] + } + end + + it 'updates other supported attributes' do + expect(existing_read.reload).to have_attributes( + has_issues: false, + has_merge_request: false, + traversal_ids: project.namespace.traversal_ids, + has_remediations: false, + archived: false, + auto_resolved: false, + identifier_names: [] + ) + + execute_service + + expect(existing_read.reload).to have_attributes( + has_issues: true, + has_merge_request: true, + traversal_ids: [1, 2, 3], + has_remediations: true, + archived: true, + auto_resolved: true, + identifier_names: ['CVE-2023-1234'] + ) + end + end + end + + context 'with bulk operations' do + let(:vulnerabilities) do + create_list(:vulnerability, 3, project: project, author: user, present_on_default_branch: true) + end + + let(:attributes) { { dismissal_reason: 'used_in_tests', has_issues: true } } + + before do + vulnerabilities.each { |v| create(:vulnerabilities_finding, vulnerability: v, scanner: scanner) } + Vulnerabilities::Read.where(vulnerability: vulnerabilities).delete_all + end + + it 'applies attributes to all vulnerabilities' do + execute_service + + Vulnerabilities::Read.where(vulnerability: vulnerabilities).find_each do |read| + expect(read).to have_attributes( + dismissal_reason: 'used_in_tests', + has_issues: true + ) + end + end + + it 'processes all vulnerabilities efficiently' do + expect { execute_service }.to change { Vulnerabilities::Read.count }.by(3) + end + end + + context 'with edge cases' do + context 'when given empty vulnerability array' do + let(:vulnerabilities) { [] } + let(:attributes) { {} } + + it 'does nothing without error' do + expect { described_class.new(vulnerabilities, attributes).execute }.not_to raise_error + end + end + + context 'when given nil vulnerability' do + let(:vulnerabilities) { nil } + let(:attributes) { {} } + + it 'does nothing without error' do + expect { described_class.new(vulnerabilities, attributes).execute }.not_to raise_error + end + end + + context 'when finding has no location' do + let(:vulnerabilities) { vulnerability } + let(:attributes) { {} } + + it 'handles finding without location' do + finding.update!(location: nil) + + execute_service + created_read = Vulnerabilities::Read.find_by(vulnerability: vulnerability) + + expect(created_read.location_image).to be_nil + end + end + + context 'when finding has location but no image' do + let(:vulnerabilities) { vulnerability } + let(:attributes) { {} } + + it 'handles finding with location but no image' do + finding.update!(location: { 'file' => 'test.rb' }) + + execute_service + created_read = Vulnerabilities::Read.find_by(vulnerability: vulnerability) + + expect(created_read.location_image).to be_nil + end + end + + context 'when vulnerability has no identifiers' do + let(:vulnerabilities) { vulnerability } + let(:attributes) { {} } + + it 'handles vulnerability without identifiers' do + finding.identifiers.delete_all + + execute_service + created_read = Vulnerabilities::Read.find_by(vulnerability: vulnerability) + + expect(created_read.identifier_names).to be_empty + end + end + end + + context 'with feature flag disabled' do + let(:vulnerabilities) { vulnerability } + let(:attributes) { { severity: :critical } } + + before do + stub_feature_flags(turn_off_vulnerability_read_create_db_trigger_function: false) + end + + it 'does not perform any operations' do + expect(Vulnerabilities::Read).not_to receive(:upsert_all) + expect { execute_service }.not_to change { Vulnerabilities::Read.count } + end + end + end +end diff --git a/ee/spec/services/vulnerabilities/resolve_service_spec.rb b/ee/spec/services/vulnerabilities/resolve_service_spec.rb index ec055f0cdd596e82b85ba2aefb596f73724a029d..0528a8ea67801823bd62c23e8fec9b7390ee079b 100644 --- a/ee/spec/services/vulnerabilities/resolve_service_spec.rb +++ b/ee/spec/services/vulnerabilities/resolve_service_spec.rb @@ -75,6 +75,17 @@ resolve_vulnerability end + it 'updates vulnerability read record with resolved state and auto_resolved flag' do + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, state: 'detected', auto_resolved: false) + + resolve_vulnerability + + vulnerability_read.reload + expect(vulnerability_read.state).to eq('resolved') + expect(vulnerability_read.auto_resolved).to eq(auto_resolved) + end + context 'when vulnerability is dismissed' do let(:vulnerability) { create(:vulnerability, :dismissed, :with_findings, project: project) } 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 0ae615bb68a17c4426dd3a9529c04ceeca33e81d..92f1307cd0033773c3f14be2335a768d596ec244 100644 --- a/ee/spec/services/vulnerabilities/starboard_vulnerability_create_service_spec.rb +++ b/ee/spec/services/vulnerabilities/starboard_vulnerability_create_service_spec.rb @@ -86,6 +86,20 @@ } ) + vulnerability_read = vulnerability.vulnerability_read + expect(vulnerability_read.resolved_on_default_branch).to be(false) + expect(vulnerability_read.identifier_names).to match_array(params.dig(:vulnerability, + :identifiers).pluck(:name)) + expect(vulnerability_read.location_image).to eq(finding.location['image']) + expect(vulnerability_read.has_remediations).to eq(vulnerability.has_remediations?) + expect(vulnerability_read.cluster_agent_id).to eq(agent.id.to_s) + expect(vulnerability_read.traversal_ids).to eq(project.namespace.traversal_ids) + expect(vulnerability_read.archived).to eq(project.archived?) + expect(vulnerability_read.auto_resolved).to be(false) + expect(vulnerability_read.severity).to eq(finding.severity) + expect(vulnerability_read.state).to eq(finding.state) + expect(vulnerability_read.report_type).to eq(finding.report_type) + expect(finding.metadata['location']).to eq(finding.location) scanner = finding.scanner diff --git a/ee/spec/services/vulnerabilities/starboard_vulnerability_resolve_service_spec.rb b/ee/spec/services/vulnerabilities/starboard_vulnerability_resolve_service_spec.rb index c6b83e9b3860a32ca15db36375c81ce7dff1bdac..0d6dfc92a9a322f2f0fc5fd058d7ef48e53af0c2 100644 --- a/ee/spec/services/vulnerabilities/starboard_vulnerability_resolve_service_spec.rb +++ b/ee/spec/services/vulnerabilities/starboard_vulnerability_resolve_service_spec.rb @@ -48,6 +48,11 @@ service.execute expect(Vulnerability.resolved).to match_array(undetected_vulnerabilities) + read_records = Vulnerabilities::Read.where(vulnerability_id: undetected_vulnerabilities.map(&:id)) + expect(read_records).to all(have_attributes( + state: 'resolved', + resolved_on_default_branch: true + )) end it 'touches the updated_at timestamp', :freeze_time do diff --git a/ee/spec/services/vulnerabilities/update_service_spec.rb b/ee/spec/services/vulnerabilities/update_service_spec.rb index da32b83a7e44c83d740abb1c3953566444fa685f..f867aa32f9ed6581887147f41da0ed1a64b6dc03 100644 --- a/ee/spec/services/vulnerabilities/update_service_spec.rb +++ b/ee/spec/services/vulnerabilities/update_service_spec.rb @@ -65,14 +65,25 @@ context 'when the `resolved_on_default_branch` kwarg is provided' do let(:resolved_on_default_branch) { true } - it 'updates the resolved_on_default_branch attribute of vulnerability' do + it 'updates the resolved_on_default_branch attribute of vulnerability and its read record' do + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, resolved_on_default_branch: false) + expect { subject }.to change { vulnerability[:resolved_on_default_branch] }.from(false).to(true) + + vulnerability_read.reload + expect(vulnerability_read.resolved_on_default_branch).to be(true) end end context 'when the `resolved_on_default_branch` kwarg is not provided' do it 'does not update the resolved_on_default_branch attribute of vulnerability' do + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, resolved_on_default_branch: false) + expect { subject }.not_to change { vulnerability[:resolved_on_default_branch] } + vulnerability_read.reload + expect(vulnerability_read.resolved_on_default_branch).to be(false) end end diff --git a/ee/spec/services/vulnerability_issue_links/bulk_create_service_spec.rb b/ee/spec/services/vulnerability_issue_links/bulk_create_service_spec.rb index b66921289a04a1a247b9808e9d62d02c8c3f9915..64eeb0f6a35708011ffed8c69529a31462ea6e00 100644 --- a/ee/spec/services/vulnerability_issue_links/bulk_create_service_spec.rb +++ b/ee/spec/services/vulnerability_issue_links/bulk_create_service_spec.rb @@ -27,6 +27,20 @@ it 'creates Vulnerabilities::IssueLink objects' do expect { bulk_create_issue_links }.to change { Vulnerabilities::IssueLink.count }.by(2) end + + it 'updates has_issues to true in vulnerability read records' do + vulnerability_reads = vulnerabilities.map do |vuln| + Vulnerabilities::Read.find_by(vulnerability_id: vuln.id) || + create(:vulnerability_read, vulnerability: vuln, has_issues: false) + end + + bulk_create_issue_links + + vulnerability_reads.each do |read| + read.reload + expect(read.has_issues).to be(true) + end + end end context 'if the issue links already exist' do @@ -43,6 +57,20 @@ it "doesn't create new IssueLinks" do expect { bulk_create_issue_links }.not_to change { Vulnerabilities::IssueLink.count } end + + it "maintains has_issues as true in vulnerability read records" do + vulnerability_reads = vulnerabilities.map do |vuln| + Vulnerabilities::Read.find_by(vulnerability_id: vuln.id) || + create(:vulnerability_read, vulnerability: vuln, has_issues: true) + end + + bulk_create_issue_links + + vulnerability_reads.each do |read| + read.reload + expect(read.has_issues).to be(true) + end + end end context 'with missing vulnerabilities' do diff --git a/ee/spec/services/vulnerability_issue_links/create_service_spec.rb b/ee/spec/services/vulnerability_issue_links/create_service_spec.rb index 3fa7e31fe17da259be5f5ae4451f77ba8fd8786b..1c6e0be96aa77dd70d00bc1206d1186e35f584b8 100644 --- a/ee/spec/services/vulnerability_issue_links/create_service_spec.rb +++ b/ee/spec/services/vulnerability_issue_links/create_service_spec.rb @@ -23,7 +23,10 @@ end context 'with valid params' do - it 'creates a new vulnerability-issue link' do + it 'creates a new vulnerability-issue link and updates vulnerability read record' do + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, has_issues: false) + expect { create_issue_link }.to change { Vulnerabilities::IssueLink.count }.by(1) response = create_issue_link @@ -32,6 +35,9 @@ issue_link = response.payload[:record] expect(issue_link).to be_persisted expect(issue_link).to have_attributes(vulnerability: vulnerability, issue: issue, link_type: 'related') + + vulnerability_read.reload + expect(vulnerability_read.has_issues).to be(true) end end diff --git a/ee/spec/services/vulnerability_issue_links/delete_service_spec.rb b/ee/spec/services/vulnerability_issue_links/delete_service_spec.rb index df9855ed7401d4ee376cce146c7e6c7ef78f7418..1e5ea4e27e5733547aba651514518a03410e8ff5 100644 --- a/ee/spec/services/vulnerability_issue_links/delete_service_spec.rb +++ b/ee/spec/services/vulnerability_issue_links/delete_service_spec.rb @@ -33,6 +33,36 @@ issue_link = response.payload[:record] expect(issue_link).to have_attributes(vulnerability_issue_link.attributes) end + + it 'updates has_issues in vulnerability read record' do + vulnerability = vulnerability_issue_link.vulnerability + + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, has_issues: true) + + delete_issue_link + + vulnerability_read.reload + expect(vulnerability_read.has_issues).to be(false) + end + + context 'when vulnerability has multiple issue links' do + before do + create(:vulnerabilities_issue_link, vulnerability: vulnerability_issue_link.vulnerability) + end + + it 'keeps has_issues as true in vulnerability read record' do + vulnerability = vulnerability_issue_link.vulnerability + + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, has_issues: true) + + delete_issue_link + + vulnerability_read.reload + expect(vulnerability_read.has_issues).to be(true) + end + end end context 'when security dashboard feature is disabled' do diff --git a/ee/spec/services/vulnerability_merge_request_links/create_service_spec.rb b/ee/spec/services/vulnerability_merge_request_links/create_service_spec.rb index 113d820d7d3553b9095555d91c6ec2de0adae3c7..700b37a1feb4967b3436e42b3a4a6041628c12ed 100644 --- a/ee/spec/services/vulnerability_merge_request_links/create_service_spec.rb +++ b/ee/spec/services/vulnerability_merge_request_links/create_service_spec.rb @@ -38,6 +38,16 @@ context 'with valid params' do it_behaves_like 'new vulnerability-merge_request link created' + + it 'updates has_merge_request in vulnerability read record' do + vulnerability_read = Vulnerabilities::Read.find_by(vulnerability_id: vulnerability.id) || + create(:vulnerability_read, vulnerability: vulnerability, has_merge_request: false) + + create_merge_request_link + + vulnerability_read.reload + expect(vulnerability_read.has_merge_request).to be(true) + end end context 'with missing vulnerability' do