diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml index 7fe8c9fffdac3d1cc385e27e2dabcb51fddf3b0f..849fa398b7f2cfff584a1d0d8ff76ce878ca88fb 100644 --- a/.rubocop_todo/layout/line_length.yml +++ b/.rubocop_todo/layout/line_length.yml @@ -487,6 +487,7 @@ Layout/LineLength: - 'ee/app/finders/projects/integrations/jira/by_ids_finder.rb' - 'ee/app/finders/projects/integrations/jira/issues_finder.rb' - 'ee/app/finders/security/pipeline_vulnerabilities_finder.rb' + - 'ee/app/finders/security/vulnerabilities_finder.rb' - 'ee/app/graphql/ee/mutations/boards/lists/create.rb' - 'ee/app/graphql/mutations/analytics/devops_adoption/enabled_namespaces/bulk_enable.rb' - 'ee/app/graphql/mutations/audit_events/external_audit_event_destinations/create.rb' @@ -1102,6 +1103,7 @@ Layout/LineLength: - 'ee/spec/finders/projects/integrations/jira/by_ids_finder_spec.rb' - 'ee/spec/finders/projects/integrations/jira/issues_finder_spec.rb' - 'ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb' + - 'ee/spec/finders/security/vulnerabilities_finder_spec.rb' - 'ee/spec/finders/security/vulnerability_reads_finder_spec.rb' - 'ee/spec/finders/snippets_finder_spec.rb' - 'ee/spec/finders/template_finder_spec.rb' diff --git a/.rubocop_todo/rspec/context_wording.yml b/.rubocop_todo/rspec/context_wording.yml index 3f945187a52b9c140bcdaf621cda46e34c902384..ce5e0ff5ec345d25e90dc83b88bc1e41caf85a6e 100644 --- a/.rubocop_todo/rspec/context_wording.yml +++ b/.rubocop_todo/rspec/context_wording.yml @@ -183,6 +183,7 @@ RSpec/ContextWording: - 'ee/spec/finders/productivity_analytics_finder_spec.rb' - 'ee/spec/finders/scim_finder_spec.rb' - 'ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb' + - 'ee/spec/finders/security/vulnerabilities_finder_spec.rb' - 'ee/spec/finders/security/vulnerability_reads_finder_spec.rb' - 'ee/spec/finders/snippets_finder_spec.rb' - 'ee/spec/finders/template_finder_spec.rb' diff --git a/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml index dda55df600c199e74862827ecb429479a18d7cff..d1baf2b3c31b57cb09353904be562d8c7928486d 100644 --- a/.rubocop_todo/rspec/named_subject.yml +++ b/.rubocop_todo/rspec/named_subject.yml @@ -82,6 +82,7 @@ RSpec/NamedSubject: - 'ee/spec/finders/projects/integrations/jira/issues_finder_spec.rb' - 'ee/spec/finders/security/approval_groups_finder_spec.rb' - 'ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb' + - 'ee/spec/finders/security/vulnerabilities_finder_spec.rb' - 'ee/spec/finders/security/vulnerability_feedbacks_finder_spec.rb' - 'ee/spec/finders/security/vulnerability_reads_finder_spec.rb' - 'ee/spec/finders/snippets_finder_spec.rb' diff --git a/.rubocop_todo/style/guard_clause.yml b/.rubocop_todo/style/guard_clause.yml index cd26a4f819c13d244862d7abf4e8b7677ef05f92..dfdf20d9a422c0b32ab4780942bbee350fa30267 100644 --- a/.rubocop_todo/style/guard_clause.yml +++ b/.rubocop_todo/style/guard_clause.yml @@ -215,6 +215,7 @@ Style/GuardClause: - 'ee/app/controllers/smartcard_controller.rb' - 'ee/app/finders/ee/template_finder.rb' - 'ee/app/finders/iterations_finder.rb' + - 'ee/app/finders/security/vulnerabilities_finder.rb' - 'ee/app/graphql/mutations/concerns/mutations/shared_epic_arguments.rb' - 'ee/app/graphql/mutations/iterations/create.rb' - 'ee/app/graphql/mutations/projects/set_locked.rb' diff --git a/.rubocop_todo/style/if_unless_modifier.yml b/.rubocop_todo/style/if_unless_modifier.yml index cb7716057f2a769171c6bd15f0d301cf327e4ed3..1ec427fa02ce68ac0d873d29a35c54700b52cea1 100644 --- a/.rubocop_todo/style/if_unless_modifier.yml +++ b/.rubocop_todo/style/if_unless_modifier.yml @@ -224,6 +224,7 @@ Style/IfUnlessModifier: - 'ee/app/controllers/projects/path_locks_controller.rb' - 'ee/app/controllers/projects/push_rules_controller.rb' - 'ee/app/finders/security/pipeline_vulnerabilities_finder.rb' + - 'ee/app/finders/security/vulnerabilities_finder.rb' - 'ee/app/graphql/mutations/audit_events/external_audit_event_destinations/create.rb' - 'ee/app/graphql/mutations/audit_events/external_audit_event_destinations/destroy.rb' - 'ee/app/graphql/mutations/boards/scoped_board_mutation.rb' diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 42a924109539beb03db6f9d597fd498564a1f12d..050dd3616aa7d9a7752597b05f26466378b9d036 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -80,9 +80,6 @@ def noteable_for_type_by_id(type, id, iid) { iid: iid } end - # the reads finder needs to query by vulnerability_id - return noteables_for_type(type).find_by!(vulnerability_id: query[:id]) if type == 'vulnerability' # rubocop: disable CodeReuse/ActiveRecord - noteables_for_type(type).find_by!(query) # rubocop: disable CodeReuse/ActiveRecord end diff --git a/ee/app/finders/autocomplete/vulnerabilities_autocomplete_finder.rb b/ee/app/finders/autocomplete/vulnerabilities_autocomplete_finder.rb index 1a8e412ea3951488c74fc60e046295c32355d14d..c126af26e08ed4ff0ccd150212184caab6fdfae5 100644 --- a/ee/app/finders/autocomplete/vulnerabilities_autocomplete_finder.rb +++ b/ee/app/finders/autocomplete/vulnerabilities_autocomplete_finder.rb @@ -10,7 +10,7 @@ class VulnerabilitiesAutocompleteFinder # params - a Hash containing additional parameters to set # # The supported parameters are those supported by - # `Security::VulnerabilityReadsFinder`. + # `Security::VulnerabilitiesFinder`. def initialize(current_user, vulnerable, params = {}) @current_user = current_user @vulnerable = vulnerable @@ -22,61 +22,13 @@ def initialize(current_user, vulnerable, params = {}) def execute return ::Vulnerability.none unless current_user && vulnerable.feature_available?(:security_dashboard) - if vulnerable.is_a?(Group) - group_vulnerabilities - else - project_vulnerabilities - end - end - - # Due to the Sec decomposition, it is not possible to simply perform a full joined query to extract a - # set of vulnerabilities for a namespace hierarchy that honours project authorizations to exclude unpermitted - # projects. - # - # Additionally, large `IN` clauses can cause performance concerns. As such, the below operation needs to: - # 1. Query the set of authorized projects (that contain vulnerabilities) from the desired namespace hierarchy - # 2. Query Vulnerability::Reads using the same namespace hierarchy, but then apply the batch of project_ids from - # the first query as a tighter scope. This prevents us from reading unpermitted projects in the hierarchy. - # - # While needing to execute this query in batches is not ideal, the combination of traversal_id and project_id - # scoping on vulnerability_reads should be well indexed and quick to respond if there are no results. - # If there are DEFAULT_AUTOCOMPLETE_LIMIT many results, then we can escape early. - # - # There are known performance concerns relating to the way this works due to the sheer possible scale of - # vulnerabilities that may need to be read if no autocomplete results are found. This will need to be improved - # in further iterations. For more context, see: - # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/175947#note_2289221607 - - def group_vulnerabilities - result = ::Vulnerability.none - - vulnerable.all_projects - .has_vulnerabilities - .visible_to_user_and_access_level(current_user, ::Gitlab::Access::DEVELOPER) - .each_batch(of: 10) do |projects| - result += ::Security::VulnerabilityReadsFinder # rubocop: disable CodeReuse/Finder - .new(vulnerable, project_id: projects.pluck_primary_key) - .execute - .autocomplete_search(params[:search].to_s) - .with_limit(DEFAULT_AUTOCOMPLETE_LIMIT) - .order_id_desc - .as_vulnerabilities - break if result.length > DEFAULT_AUTOCOMPLETE_LIMIT - end - - result - end - - def project_vulnerabilities - return ::Vulnerability.none unless current_user.authorized_project?(vulnerable, Gitlab::Access::DEVELOPER) - - ::Security::VulnerabilityReadsFinder # rubocop: disable CodeReuse/Finder + ::Security::VulnerabilitiesFinder # rubocop: disable CodeReuse/Finder .new(vulnerable) .execute .autocomplete_search(params[:search].to_s) .with_limit(DEFAULT_AUTOCOMPLETE_LIMIT) .order_id_desc - .as_vulnerabilities + .visible_to_user_and_access_level(current_user, ::Gitlab::Access::DEVELOPER) end end end diff --git a/ee/app/finders/ee/notes_finder.rb b/ee/app/finders/ee/notes_finder.rb index 948596385a501ef282c859975e9bb75a72bb7508..7b043c533e845c83ce9d14a300dd71a52589d9fc 100644 --- a/ee/app/finders/ee/notes_finder.rb +++ b/ee/app/finders/ee/notes_finder.rb @@ -11,21 +11,10 @@ def noteables_for_type(noteable_type) when 'epic' return EpicsFinder.new(@current_user, group_id: @params[:group_id]) # rubocop:disable Gitlab/ModuleWithInstanceVariables when 'vulnerability' - return ::Security::VulnerabilityReadsFinder.new(@project) # rubocop:disable Gitlab/ModuleWithInstanceVariables + return ::Security::VulnerabilitiesFinder.new(@project) # rubocop:disable Gitlab/ModuleWithInstanceVariables end super end - - override :notes_on_target - def notes_on_target - if target.respond_to?(:related_notes) - target.related_notes - elsif target.is_a?(::Vulnerabilities::Read) - target.vulnerability.notes - else - target.notes - end - end end end diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb new file mode 100644 index 0000000000000000000000000000000000000000..015a52171a97a46b9c94f404854b069576054528 --- /dev/null +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +# Security::VulnerabilitiesFinder +# +# Used to filter Vulnerability records for Vulnerabilities API +# +# Arguments: +# vulnerable: any object that has a #vulnerabilities method that returns a collection of `Vulnerability`s +# params: optional! a hash with one or more of the following: +# project_ids: if `vulnerable` includes multiple projects (like a Group), this filter will restrict +# the vulnerabilities returned to those in the group's projects that also match these IDs +# include_archived_projects: defaulted to `false`. Determines if results will include vulnerabilities +# associated with archived projects +# image: only return vulnerabilities with these location images +# report_types: only return vulnerabilities from these report types +# severities: only return vulnerabilities with these severities +# states: only return vulnerabilities in these states +# has_resolution: only return vulnerabilities that have resolution +# has_issues: only return vulnerabilities that have issues linked +# sort: return vulnerabilities ordered by severity_asc or severity_desc + +module Security + class VulnerabilitiesFinder + include FinderMethods + + def initialize(vulnerable, params = {}) + @params = { include_archived_projects: false }.merge(params) + @vulnerable = vulnerable + @vulnerabilities = vulnerable.vulnerabilities + end + + def execute + # As we are creating vulnerability with default branch set to false irrespective of feature flag + # from user interaction (issue/mr creation and dismissal of finding), we always need to do this filtering + filter_by_present_on_default_branch + filter_archived_projects + filter_by_projects + filter_by_image + filter_by_report_types + filter_by_severities + filter_by_states + filter_by_scanner_external_id + filter_by_scanner_ids + filter_by_resolution + filter_by_issues + filter_by_cluster_id + filter_by_cluster_agent_id + + sort(vulnerabilities) + end + + private + + attr_reader :params, :vulnerable, :vulnerabilities + + def filter_by_present_on_default_branch + @vulnerabilities = if params[:present_on_default_branch].eql?(false) + vulnerabilities + else + vulnerabilities.for_default_branch + end + end + + def filter_archived_projects + return if params[:include_archived_projects] == true + return unless vulnerable.is_a?(Group) + + # `filter_by_projects` will handle archived projects + return if params[:project_id].present? + + @vulnerabilities = vulnerabilities.without_archived_projects + end + + def filter_by_projects + if params[:project_id].present? + @vulnerabilities = vulnerabilities.for_projects( + params[:project_id], + params[:include_archived_projects] + ) + end + end + + def filter_by_report_types + if params[:report_type].present? + @vulnerabilities = vulnerabilities.with_report_types(params[:report_type]) + end + end + + def filter_by_severities + if params[:severity].present? + @vulnerabilities = vulnerabilities.with_severities(params[:severity]) + end + end + + def filter_by_states + if params[:state].present? + @vulnerabilities = vulnerabilities.with_states(params[:state]) + end + end + + def filter_by_scanner_ids + if params[:scanner_id].present? + @vulnerabilities = vulnerabilities.by_scanner_ids(params[:scanner_id]) + end + end + + def filter_by_scanner_external_id + if params[:scanner].present? + @vulnerabilities = vulnerabilities.with_scanner_external_ids(params[:scanner]) + end + end + + def filter_by_resolution + if params[:has_resolution].in?([true, false]) + @vulnerabilities = vulnerabilities.with_resolution(params[:has_resolution]) + end + end + + def filter_by_issues + if params[:has_issues].in?([true, false]) + @vulnerabilities = vulnerabilities.with_issues(params[:has_issues]) + end + end + + def filter_by_image + # This filter will not work for InstanceSecurityDashboard, because InstanceSecurityDashboard could have multiple projects. + return if vulnerable.is_a?(InstanceSecurityDashboard) + + if params[:image].present? + @vulnerabilities = vulnerabilities.with_container_image(params[:image]) + end + end + + def filter_by_cluster_id + if params[:cluster_id].present? + @vulnerabilities = vulnerabilities.with_cluster_ids(params[:cluster_id]) + end + end + + def filter_by_cluster_agent_id + if params[:cluster_agent_id].present? + @vulnerabilities = vulnerabilities.with_cluster_agent_ids(params[:cluster_agent_id]) + end + end + + def sort(items) + items.order_by(params[:sort]) + end + end +end diff --git a/ee/app/finders/security/vulnerability_reads_finder.rb b/ee/app/finders/security/vulnerability_reads_finder.rb index 0196e3949594e323f9adbef6084bf05f27107d7c..0880ad07d90fd6f6d5d62b3b7d80eb5e6ed68198 100644 --- a/ee/app/finders/security/vulnerability_reads_finder.rb +++ b/ee/app/finders/security/vulnerability_reads_finder.rb @@ -7,7 +7,7 @@ # Arguments: # vulnerable: any object that has a #vulnerabilities method that returns a collection of `Vulnerability`s # params: optional! a hash with one or more of the following: -# project_id: if `vulnerable` includes multiple projects (like a Group), this filter will restrict +# project_ids: if `vulnerable` includes multiple projects (like a Group), this filter will restrict # the vulnerabilities returned to those in the group's projects that also match these IDs # include_archived_projects: defaulted to `false`. Determines if results will include vulnerabilities # associated with archived projects @@ -86,10 +86,14 @@ def use_unnested_filters? def filter_archived_projects return if params[:include_archived_projects] == true - return unless vulnerable.is_a?(Group) - @vulnerability_reads = vulnerability_reads.unarchived + return @vulnerability_reads = vulnerability_reads.unarchived if vulnerability_is_a_group? + + # `filter_by_projects` will handle archived projects + return if params[:project_id].present? + + @vulnerability_reads = vulnerability_reads.without_archived_projects end def filter_by_projects diff --git a/ee/app/models/instance_security_dashboard.rb b/ee/app/models/instance_security_dashboard.rb index 0f415fb3a28e1fc88d62391ef897b4b147c72c58..7e611ac9c4d06938005f65dbd5d582445d750ea8 100644 --- a/ee/app/models/instance_security_dashboard.rb +++ b/ee/app/models/instance_security_dashboard.rb @@ -33,6 +33,12 @@ def projects .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/485658') end + def vulnerabilities + return Vulnerability.none if projects.empty? + + Vulnerability.for_projects(projects).allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/485658') + end + def vulnerability_reads return Vulnerabilities::Read.none if projects.empty? diff --git a/ee/app/models/vulnerabilities/read.rb b/ee/app/models/vulnerabilities/read.rb index 59be3639f2f74119cba99dcaf26158587956052d..234b015e319e596b6563aeac673010a65a462919 100644 --- a/ee/app/models/vulnerabilities/read.rb +++ b/ee/app/models/vulnerabilities/read.rb @@ -7,7 +7,6 @@ class Read < Gitlab::Database::SecApplicationRecord include UnnestedInFilters::Dsl include FromUnion include SafelyChangeColumnDefault - include ::Gitlab::SQL::Pattern ignore_column :namespace_id, remove_with: '17.7', remove_after: '2024-11-21' @@ -21,10 +20,6 @@ class Read < Gitlab::Database::SecApplicationRecord columns_changing_default :owasp_top_10 - delegate :group_name, :title, :created_at, :project_name, :finding_scanner_name, :finding_description, :cve_value, :cwe_value, :location, :notes_summary, :full_path, to: :vulnerability, allow_nil: true - delegate :other_identifier_values, :cvss_vectors_with_vendor, to: :vulnerability, allow_nil: true - delegate :dismissed?, to: :vulnerability - belongs_to :vulnerability, inverse_of: :vulnerability_read belongs_to :project belongs_to :scanner, class_name: 'Vulnerabilities::Scanner' @@ -145,21 +140,6 @@ class << self where(traversal_ids: all_vulnerable_traversal_ids_for(group)) end - scope :with_findings_scanner_identifiers_and_notes, -> { with_findings_scanner_and_identifiers.includes(vulnerability: :notes) } - scope :with_limit, ->(maximum) { limit(maximum) } - scope :order_id_desc, -> { reorder(arel_table[:vulnerability_id].desc) } - - scope :autocomplete_search, ->(query) do - return self if query.blank? - - id_as_text = Arel::Nodes::NamedFunction.new('CAST', [arel_table[:vulnerability_id].as('TEXT')]) - - joins(:vulnerability) - .select('vulnerability_reads.*, vulnerabilities.title') - .fuzzy_search(query, [Vulnerability.arel_table[:title]]) - .or(where(id_as_text.matches("%#{sanitize_sql_like(query.squish)}%"))) - end - def self.arel_grouping_by_traversal_ids_and_vulnerability_id arel_table.grouping([arel_table['traversal_ids'], arel_table['vulnerability_id']]) end diff --git a/ee/app/services/ee/groups/autocomplete_service.rb b/ee/app/services/ee/groups/autocomplete_service.rb index a02e53a5c7f41b5d1a15953e2cdc84ba80ef5ec8..3cfe4b2411651ff44c3b8eec4dba6ad4f0edf31d 100644 --- a/ee/app/services/ee/groups/autocomplete_service.rb +++ b/ee/app/services/ee/groups/autocomplete_service.rb @@ -38,6 +38,7 @@ def vulnerabilities ::Autocomplete::VulnerabilitiesAutocompleteFinder .new(current_user, group, params) .execute + .select([:id, :title, :project_id]) end end end diff --git a/ee/app/services/ee/projects/autocomplete_service.rb b/ee/app/services/ee/projects/autocomplete_service.rb index 494a831d22b2a5638b6d51cf4b2c810beaadf0c6..d95c1b981154f4c06d3089b7d64b9a090ead6069 100644 --- a/ee/app/services/ee/projects/autocomplete_service.rb +++ b/ee/app/services/ee/projects/autocomplete_service.rb @@ -20,6 +20,7 @@ def vulnerabilities ::Autocomplete::VulnerabilitiesAutocompleteFinder .new(current_user, project, params) .execute + .select([:id, :title]) end end end diff --git a/ee/app/services/vulnerability_exports/export_service.rb b/ee/app/services/vulnerability_exports/export_service.rb index 48ccec744fb7b369b28b93f6a197e13a0ee1f236..63bdafe8f6d4879a7ffd071a889e2735e7dd19b1 100644 --- a/ee/app/services/vulnerability_exports/export_service.rb +++ b/ee/app/services/vulnerability_exports/export_service.rb @@ -174,8 +174,7 @@ def vulnerabilities if exportable.is_a?(Group) group_vulnerabilities_enumerator else - Security::VulnerabilityReadsFinder.new(exportable).execute - .with_findings_scanner_identifiers_and_notes + Security::VulnerabilitiesFinder.new(exportable).execute.with_findings_scanner_identifiers_and_notes end end diff --git a/ee/app/services/vulnerability_exports/exporters/csv_service.rb b/ee/app/services/vulnerability_exports/exporters/csv_service.rb index e0debe388abc2bc85815d0bfa2aa228ebab38059..95b4a1c86277e8223d0b5e8573b97d5399007ee1 100644 --- a/ee/app/services/vulnerability_exports/exporters/csv_service.rb +++ b/ee/app/services/vulnerability_exports/exporters/csv_service.rb @@ -13,9 +13,8 @@ class CsvService attr_reader :vulnerabilities - def initialize(vulnerabilities, order_hint: Vulnerabilities::Read.arel_table[:vulnerability_id]) + def initialize(vulnerabilities) @vulnerabilities = vulnerabilities - @order_hint = order_hint end def generate(&block) @@ -30,20 +29,16 @@ def header def preloads [ - { - vulnerability: [ - { project: [:route] }, - :group, - { notes: [:updated_by, :author] }, - { findings: [:scanner, :identifiers, { finding_identifiers: :identifier }] } - ] - } + { project: [:route] }, + :group, + :vulnerability_read, + { notes: [:updated_by, :author] }, + { findings: [:scanner, :identifiers, { finding_identifiers: :identifier }] } ] end def csv_builder - @csv_builder ||= CsvBuilder.new(vulnerabilities, mapping, preloads, - replace_newlines: true, order_hint: @order_hint) + @csv_builder ||= CsvBuilder.new(vulnerabilities, mapping, preloads, replace_newlines: true) end def mapping diff --git a/ee/spec/controllers/ee/projects/autocomplete_sources_controller_spec.rb b/ee/spec/controllers/ee/projects/autocomplete_sources_controller_spec.rb index 6b5460eb4e9c3daa0c2bbbc311161d14e1d0e8cc..d0b15bc945ea41a596c99e4fc5095c2b41c1e7ee 100644 --- a/ee/spec/controllers/ee/projects/autocomplete_sources_controller_spec.rb +++ b/ee/spec/controllers/ee/projects/autocomplete_sources_controller_spec.rb @@ -8,7 +8,7 @@ let_it_be(:project) { create(:project, :public, group: group) } let_it_be(:epic) { create(:epic, group: group) } let_it_be(:epic2) { create(:epic, group: group2) } - let_it_be(:vulnerability) { create(:vulnerability, :with_read, project: project) } + let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) } before do sign_in(user) diff --git a/ee/spec/controllers/projects/security/vulnerabilities/notes_controller_spec.rb b/ee/spec/controllers/projects/security/vulnerabilities/notes_controller_spec.rb index ee8424f6bc93ffdfb1a8c7896d6e762296793aa5..152c23e62cbc51d7509fdd69a871011a6cff17bb 100644 --- a/ee/spec/controllers/projects/security/vulnerabilities/notes_controller_spec.rb +++ b/ee/spec/controllers/projects/security/vulnerabilities/notes_controller_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Projects::Security::Vulnerabilities::NotesController, feature_category: :vulnerability_management do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } - let_it_be(:vulnerability) { create(:vulnerability, :with_read, project: project) } + let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) } let!(:note) { create(:note, noteable: vulnerability, project: project) } diff --git a/ee/spec/finders/autocomplete/vulnerabilities_autocomplete_finder_spec.rb b/ee/spec/finders/autocomplete/vulnerabilities_autocomplete_finder_spec.rb index d568298c29acb1ab57ef8f1bf8c68b0ec6543dc5..c09ecb836e98892b06c95625115f42aadd334438 100644 --- a/ee/spec/finders/autocomplete/vulnerabilities_autocomplete_finder_spec.rb +++ b/ee/spec/finders/autocomplete/vulnerabilities_autocomplete_finder_spec.rb @@ -6,10 +6,10 @@ describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:group, refind: true) { create(:group) } - let_it_be(:project, refind: true) { create(:project, group: group).tap(&:mark_as_vulnerable!) } + let_it_be(:project, refind: true) { create(:project, group: group) } let_it_be(:vulnerabilities) do max_plus_one = described_class::DEFAULT_AUTOCOMPLETE_LIMIT + 1 - create_list(:vulnerability, max_plus_one, :with_read, project: project, group: group) do |vulnerability, i| + create_list(:vulnerability, max_plus_one, :with_finding, project: project) do |vulnerability, i| # The default title provided by FactoryBot is "My title {i}" # We test fuzzy-finding by title in these specs. The # fuzzy_find module has a `use_minimum_char_limit:`, which we diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..21b61b1847c4f5a9c52b817cbefbcfcf78dfcae1 --- /dev/null +++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb @@ -0,0 +1,320 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::VulnerabilitiesFinder, feature_category: :vulnerability_management do + let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + + let_it_be(:archived_project) do + create(:project, :archived, namespace: group).tap do |p| + create(:vulnerability, :with_finding, project: p) + end + end + + let_it_be(:vulnerability1) do + create(:vulnerability, :with_findings, :with_issue_links, severity: :low, report_type: :sast, state: :detected, project: project) + end + + let_it_be(:vulnerability2) do + create(:vulnerability, :with_findings, resolved_on_default_branch: true, severity: :high, report_type: :dependency_scanning, state: :confirmed, project: project) + end + + let_it_be(:vulnerability3) do + create(:vulnerability, :with_findings, severity: :medium, report_type: :dast, state: :dismissed, project: project) + end + + let(:archive_associated_vulnerabilities) { archived_project.vulnerabilities } + let(:filters) { {} } + let(:vulnerable) { project } + + shared_context 'with vulnerability dismissed with a reason' do + let_it_be(:dismissed_vulnerability) { create(:vulnerability, :dismissed, report_type: :dependency_scanning, severity: :low, project: project) } + let_it_be(:dismissed_vulnerability_read) do + create( + :vulnerability_read, + :used_in_tests, + report_type: dismissed_vulnerability.report_type, + state: dismissed_vulnerability.state, + severity: dismissed_vulnerability.severity, + vulnerability: dismissed_vulnerability, + project: project + ) + end + end + + subject { described_class.new(vulnerable, filters).execute } + + it 'returns vulnerabilities of a project' do + expect(subject).to match_array(project.vulnerabilities) + end + + context 'when not given a second argument' do + subject { described_class.new(project).execute } + + it 'does not filter the vulnerability list' do + expect(subject).to match_array(project.vulnerabilities) + end + end + + context 'when using the include_archived_projects param' do + using RSpec::Parameterized::TableSyntax + + let(:result_including_archived) { result_excluding_archived + archive_associated_vulnerabilities } + let(:result_excluding_archived) { group.vulnerabilities.without_archived_projects } + + before do + project.update!(namespace: group) + end + + where(:vulnerable_object, :include_archived_projects, :result) do + ref(:archived_project) | true | ref(:archive_associated_vulnerabilities) + ref(:archived_project) | false | ref(:archive_associated_vulnerabilities) + ref(:group) | true | ref(:result_including_archived) + ref(:group) | false | ref(:result_excluding_archived) + end + + with_them do + let(:vulnerable) { vulnerable_object } + let(:filters) { super().merge(include_archived_projects: include_archived_projects) } + + it 'filters out vulnerabilities associated with archived projects as defined' do + expect(subject).to match_array(result) + end + end + end + + context 'when filtered by report type' do + let(:filters) { { report_type: %w[sast dast] } } + + it 'only returns vulnerabilities matching the given report types' do + is_expected.to contain_exactly(vulnerability1, vulnerability3) + end + end + + context 'when filtered by severity' do + let(:filters) { { severity: %w[medium high] } } + + it 'only returns vulnerabilities matching the given severities' do + is_expected.to contain_exactly(vulnerability3, vulnerability2) + end + end + + context 'when filtered by state' do + let(:filters) { { state: %w[detected confirmed] } } + + it 'only returns vulnerabilities matching the given states' do + is_expected.to contain_exactly(vulnerability1, vulnerability2) + end + + context 'when combined with other filters' do + let(:filters) { { state: %w[dismissed], report_type: %w[dast] } } + + it 'respects the other filters' do + is_expected.to contain_exactly(vulnerability3) + end + end + end + + context 'when filtered by scanner external ID' do + let(:filters) { { scanner: [vulnerability1.finding_scanner_external_id, vulnerability2.finding_scanner_external_id] } } + + it 'only returns vulnerabilities matching the given scanner IDs' do + is_expected.to contain_exactly(vulnerability1, vulnerability2) + end + end + + context 'when filtered by scanner_id' do + let(:filters) { { scanner_id: [vulnerability1.finding_scanner_id, vulnerability3.finding_scanner_id] } } + + it 'only returns vulnerabilities matching the given scanner IDs' do + is_expected.to contain_exactly(vulnerability1, vulnerability3) + end + end + + context 'when the vulnerable object is a Group' do + let(:vulnerable) { group } + let(:another_project) { create(:project, namespace: group) } + + let!(:another_vulnerability) { create(:vulnerability, :with_findings, project: another_project) } + + let_it_be(:group) { create(:group) } + let_it_be(:archived_project) { create(:project, :archived, namespace: group) } + + before do + project.update!(namespace: group) + end + + context 'when filtered by project' do + let!(:archived_vulnerability) { create(:vulnerability, :with_findings, project: archived_project) } + let(:filters) { { project_id: [another_project.id, archived_project.id] } } + + it 'only returns vulnerabilities matching the given projects' do + is_expected.to contain_exactly(another_vulnerability) + end + + context 'when including archived projects' do + let(:filters) { super().merge(include_archived_projects: true) } + + it 'returns vulnerabilities matching the given projects' do + is_expected.to contain_exactly(another_vulnerability, archived_vulnerability) + end + end + end + end + + context 'when sorted' do + let(:filters) { { sort: method } } + + context 'ascending by severity' do + let(:method) { :severity_asc } + + it { is_expected.to eq([vulnerability1, vulnerability3, vulnerability2]) } + end + + context 'descending by severity' do + let(:method) { :severity_desc } + + it { is_expected.to eq([vulnerability2, vulnerability3, vulnerability1]) } + end + end + + context 'when filtered by has_issues argument' do + let(:filters) { { has_issues: has_issues } } + + context 'when has_issues is set to true' do + let(:has_issues) { true } + + it 'only returns vulnerabilities that have issues' do + is_expected.to contain_exactly(vulnerability1) + end + end + + context 'when has_issues is set to false' do + let(:has_issues) { false } + + it 'only returns vulnerabilities that does not have issues' do + is_expected.to contain_exactly(vulnerability2, vulnerability3) + end + end + end + + context 'when filtered by has_resolution argument' do + let(:filters) { { has_resolution: has_resolution } } + + context 'when has_resolution is set to true' do + let(:has_resolution) { true } + + it 'only returns vulnerabilities that have resolution' do + is_expected.to contain_exactly(vulnerability2) + end + end + + context 'when has_resolution is set to false' do + let(:has_resolution) { false } + + it 'only returns vulnerabilities that do not have resolution' do + is_expected.to contain_exactly(vulnerability1, vulnerability3) + end + end + end + + context 'when filtered by more than one property' do + let_it_be(:vulnerability4) do + create(:vulnerability, :with_findings, severity: :medium, report_type: :sast, state: :detected, project: project) + end + + let(:filters) { { report_type: %w[sast], severity: %w[medium] } } + + it 'only returns vulnerabilities matching all of the given filters' do + is_expected.to contain_exactly(vulnerability4) + end + end + + context 'when filtered by image' do + let_it_be(:cluster_vulnerability) { create(:vulnerability, :cluster_image_scanning, project: project) } + let_it_be(:finding) { create(:vulnerabilities_finding, :with_cluster_image_scanning_scanning_metadata, project: project, vulnerability: cluster_vulnerability) } + + let(:filters) { { image: [finding.location['image']] } } + let(:feature_enabled) { true } + + it 'only returns vulnerabilities matching the given image' do + is_expected.to contain_exactly(cluster_vulnerability) + end + + context 'when different report_type is passed' do + let(:filters) { { report_type: %w[dast], image: [finding.location['image']] } } + + it 'returns empty list' do + is_expected.to be_empty + end + end + + context 'when vulnerable is InstanceSecurityDashboard' do + let(:vulnerable) { InstanceSecurityDashboard.new(project.users.first) } + + it 'does not include cluster vulnerability' do + is_expected.not_to contain_exactly(cluster_vulnerability) + end + end + end + + context 'when filtered by cluster_id' do + let_it_be(:cluster_vulnerability) { create(:vulnerability, :cluster_image_scanning, project: project) } + let_it_be(:finding) { create(:vulnerabilities_finding, :with_cluster_image_scanning_scanning_metadata, project: project, vulnerability: cluster_vulnerability) } + + let(:filters) { { cluster_id: [finding.location['kubernetes_resource']['cluster_id']] } } + + it 'only returns vulnerabilities matching the given cluster_id' do + is_expected.to contain_exactly(cluster_vulnerability) + end + + context 'when different report_type is passed' do + let(:filters) { { report_type: %w[dast], cluster_id: [finding.location['kubernetes_resource']['cluster_id']] } } + + it 'returns empty list' do + is_expected.to be_empty + end + end + end + + context 'when filtered by cluster_agent_id' do + let_it_be(:cluster_agent) { create(:cluster_agent, project: project) } + let_it_be(:cluster_vulnerability) { create(:vulnerability, :cluster_image_scanning, project: project) } + let_it_be(:finding) { create(:vulnerabilities_finding, :with_cluster_image_scanning_scanning_metadata, agent_id: cluster_agent.id.to_s, project: project, vulnerability: cluster_vulnerability) } + + let(:filters) { { cluster_agent_id: [finding.location['kubernetes_resource']['agent_id']] } } + + it 'only returns vulnerabilities matching the given agent_id' do + is_expected.to contain_exactly(cluster_vulnerability) + end + + context 'when different report_type is passed' do + let(:filters) { { report_type: %w[dast], cluster_agent_id: [finding.location['kubernetes_resource']['agent_id']] } } + + it 'returns empty list' do + is_expected.to be_empty + end + end + end + + context 'when there are vulnerabilities on non default branches' do + let_it_be(:vulnerability4) do + create(:vulnerability, report_type: :dast, project: project, present_on_default_branch: false) + end + + let(:filters) { { report_type: %w[dast] } } + + it 'only returns vulnerabilities on the default branch by default' do + is_expected.to contain_exactly(vulnerability3) + end + + context 'when present_on_default_branch is passed' do + let(:filters) { { report_type: %w[dast], present_on_default_branch: false } } + + it 'returns vulnerabilities on all branches' do + is_expected.to contain_exactly(vulnerability3, vulnerability4) + end + end + end +end diff --git a/ee/spec/models/instance_security_dashboard_spec.rb b/ee/spec/models/instance_security_dashboard_spec.rb index f5d2c57ab025b8ce325b4816abd14cd328002495..3c2772a61b016ed23445d64d5bb9def52ab6176d 100644 --- a/ee/spec/models/instance_security_dashboard_spec.rb +++ b/ee/spec/models/instance_security_dashboard_spec.rb @@ -183,6 +183,25 @@ end end + describe '#vulnerabilities' do + let_it_be(:vulnerability1) { create(:vulnerability, project: project1) } + let_it_be(:vulnerability2) { create(:vulnerability, project: project2) } + + context 'when the user cannot read all resources' do + it 'returns only vulnerabilities from projects on their dashboard that they can read' do + expect(subject.vulnerabilities).to contain_exactly(vulnerability1) + end + end + + context 'when the user can read all resources' do + let(:user) { create(:auditor) } + + it "returns vulnerabilities from all projects on the user's dashboard" do + expect(subject.vulnerabilities).to contain_exactly(vulnerability1, vulnerability2) + end + end + end + describe '#vulnerability_reads' do let_it_be(:vulnerability1) { create(:vulnerability, :with_findings, project: project1) } let_it_be(:vulnerability2) { create(:vulnerability, :with_findings, project: project2) } diff --git a/ee/spec/requests/ee/groups/autocomplete_sources_spec.rb b/ee/spec/requests/ee/groups/autocomplete_sources_spec.rb index 9a8c2e1b7c387453539bb4cd06c25552f09581b6..e7a5cce2f71e7818f54dcce22fe43c2b3c6a3f27 100644 --- a/ee/spec/requests/ee/groups/autocomplete_sources_spec.rb +++ b/ee/spec/requests/ee/groups/autocomplete_sources_spec.rb @@ -122,11 +122,10 @@ def visit describe '#vulnerabilities' do let_it_be_with_reload(:project) { create(:project, :private, group: group) } - let_it_be(:vulnerability) { create(:vulnerability, :with_read, project: project) } + let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) } before do project.add_developer(user) - project.mark_as_vulnerable! stub_licensed_features(security_dashboard: true) end diff --git a/ee/spec/services/ee/groups/autocomplete_service_spec.rb b/ee/spec/services/ee/groups/autocomplete_service_spec.rb index b4131728ed0ae52fb27530027491338543f63e7b..7c4012baf03e96efa0057b73f606fcb8e8690339 100644 --- a/ee/spec/services/ee/groups/autocomplete_service_spec.rb +++ b/ee/spec/services/ee/groups/autocomplete_service_spec.rb @@ -168,8 +168,8 @@ def expect_labels_to_equal(labels, expected_labels) end describe '#vulnerability' do - let_it_be_with_refind(:project) { create(:project, group: group).tap(&:mark_as_vulnerable!) } - let_it_be(:vulnerability) { create(:vulnerability, :with_read, project: project) } + let_it_be_with_refind(:project) { create(:project, group: group) } + let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) } let_it_be(:guest) { create(:user) } let(:autocomplete_user) { user } diff --git a/ee/spec/services/vulnerability_exports/export_service_spec.rb b/ee/spec/services/vulnerability_exports/export_service_spec.rb index 896e7771ecbc7825a358028ea0191507f5b930a6..f1c4e9995133c6f23371d3ea9938a5919484b6f9 100644 --- a/ee/spec/services/vulnerability_exports/export_service_spec.rb +++ b/ee/spec/services/vulnerability_exports/export_service_spec.rb @@ -351,15 +351,15 @@ end context 'when the export format is csv' do - let(:vulnerabilities) { Vulnerabilities::Read.none } + let(:vulnerabilities) { Vulnerability.none } let(:mock_relation) { double(:relation, with_findings_scanner_identifiers_and_notes: vulnerabilities) } - let(:mock_vulnerability_finder_service_object) { instance_double(Security::VulnerabilityReadsFinder, execute: mock_relation) } + let(:mock_vulnerability_finder_service_object) { instance_double(Security::VulnerabilitiesFinder, execute: mock_relation) } let(:exportable_full_path) { 'foo' } let(:time_suffix) { Time.current.utc.strftime('%FT%H%M') } let(:expected_file_name) { "#{exportable_full_path}_vulnerabilities_#{time_suffix}.csv" } before do - allow(Security::VulnerabilityReadsFinder).to receive(:new).and_return(mock_vulnerability_finder_service_object) + allow(Security::VulnerabilitiesFinder).to receive(:new).and_return(mock_vulnerability_finder_service_object) allow(vulnerability_export.exportable).to receive(:full_path).and_return(exportable_full_path) end diff --git a/ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb b/ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb index 017f10cbecdcf22c63536cd7b6932e93ed00b07c..41b004f1050b3c1f56e2e8afb019134f29f79529 100644 --- a/ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb +++ b/ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb @@ -8,7 +8,7 @@ let_it_be(:group) { create(:group) } let_it_be(:project) { project_with_valid_findings(group: group) } let_it_be(:user) { create(:user, maintainer_of: project) } - let(:vulnerabilities) { ::Vulnerabilities::Read.all } + let(:vulnerabilities) { Vulnerability.all } let(:export_csv_service) { described_class.new(vulnerabilities) } subject(:csv) { CSV.parse(export_csv_service.generate, headers: true) } @@ -130,13 +130,8 @@ end context 'when a vulnerability is dismissed with a dismissal reason' do - let_it_be(:vulnerability) do - create(:vulnerability, :with_read, :dismissed, project: project) do |vulnerability| - vulnerability.vulnerability_read.dismissal_reason = 3 - end - end - - let_it_be(:vulnerabilities) { [vulnerability] } + let_it_be(:vulnerability) { create(:vulnerability, :dismissed, project: project) } + let_it_be(:vulnerability_read) { create(:vulnerability_read, :used_in_tests, vulnerability: vulnerability, project: project) } it 'populates the dismissal reason column' do expect(csv[0]['Status']).to eq 'dismissed' @@ -160,7 +155,7 @@ describe 'with Active Record relation' do before do - create(:vulnerability, :with_read) + create(:vulnerability) end it 'generates the CSV content for records returned by the relation' do diff --git a/gems/csv_builder/lib/csv_builder.rb b/gems/csv_builder/lib/csv_builder.rb index f19d4727488d5646113c99a66cfd354f7edc67f3..0f5aa5386332e78dc029d42895d2121979b13f06 100644 --- a/gems/csv_builder/lib/csv_builder.rb +++ b/gems/csv_builder/lib/csv_builder.rb @@ -29,20 +29,16 @@ module CsvBuilder # * +header_to_value_hash+ - A hash of 'Column Heading' => 'value_method'. # * +associations_to_preload+ - An array of records to preload with a batch of records. # * +replace_newlines+ - default: false - If true, replaces newline characters with a literal "\n" - # * +order_hint+ - default: :created_at - The column used to order the rows # # The value method will be called once for each object in the collection, to # determine the value for that row. It can either be the name of a method on # the object, or a lamda to call passing in the object. - def self.new( - collection, header_to_value_hash, associations_to_preload = [], replace_newlines: false, - order_hint: :created_at) + def self.new(collection, header_to_value_hash, associations_to_preload = [], replace_newlines: false) CsvBuilder::Builder.new( collection, header_to_value_hash, associations_to_preload, - replace_newlines: replace_newlines, - order_hint: order_hint + replace_newlines: replace_newlines ) end end diff --git a/gems/csv_builder/lib/csv_builder/builder.rb b/gems/csv_builder/lib/csv_builder/builder.rb index faeda38db3374fc9f8c93602e29aafbd3ffc96b4..c270db77f847458e18458747b6f0a7f25a4c9233 100644 --- a/gems/csv_builder/lib/csv_builder/builder.rb +++ b/gems/csv_builder/lib/csv_builder/builder.rb @@ -6,16 +6,13 @@ class Builder attr_reader :rows_written - def initialize( - collection, header_to_value_hash, associations_to_preload = [], replace_newlines: false, - order_hint: :created_at) + def initialize(collection, header_to_value_hash, associations_to_preload = [], replace_newlines: false) @header_to_value_hash = header_to_value_hash @collection = collection @truncated = false @rows_written = 0 @associations_to_preload = associations_to_preload @replace_newlines = replace_newlines - @order_hint = order_hint end # Renders the csv to a string @@ -60,7 +57,7 @@ def status def each(&block) if @associations_to_preload&.any? && @collection.respond_to?(:each_batch) - @collection.each_batch(order_hint: @order_hint) do |relation| + @collection.each_batch(order_hint: :created_at) do |relation| relation.preload(@associations_to_preload).order(:id).each(&block) end elsif @collection.respond_to?(:find_each) diff --git a/spec/support/finder_collection_allowlist.yml b/spec/support/finder_collection_allowlist.yml index e79c96bb591fc1ab47b2100c543ed27ff64cde91..56fcdd02e4cd20d31fadc98a71f2c5ae16fcce62 100644 --- a/spec/support/finder_collection_allowlist.yml +++ b/spec/support/finder_collection_allowlist.yml @@ -73,4 +73,3 @@ - UserGroupNotificationSettingsFinder - UserGroupsCounter - Ai::FeatureSettings::FeatureSettingFinder -- Autocomplete::VulnerabilitiesAutocompleteFinder diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index 19541e824f7581bb592cb9b074161b38f7e14cc4..4da798e7b811ef134f22edff06822ab2e0f4fffc 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -331,6 +331,7 @@ - './ee/spec/finders/security/findings_finder_spec.rb' - './ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb' - './ee/spec/finders/security/scan_execution_policies_finder_spec.rb' +- './ee/spec/finders/security/vulnerabilities_finder_spec.rb' - './ee/spec/finders/security/vulnerability_feedbacks_finder_spec.rb' - './ee/spec/finders/snippets_finder_spec.rb' - './ee/spec/finders/template_finder_spec.rb'