From 9fafff194b34691524cde2bbdf6b94d8d5202204 Mon Sep 17 00:00:00 2001
From: Michael Becker <11881043-wandering_person@users.noreply.gitlab.com>
Date: Wed, 30 Aug 2023 17:03:01 +0000
Subject: [PATCH] Change `Vulnerability*Finder` interaction with archived
projects
In [issue 213144][0] we defined new default expected behavior. We want
the default behavior for all finders to be:
Do not include vulnerability` or `vulnerability_read` objects in the
result-set if the project they are associated with is currently
`archived`
At the same time we want to, in a followup commit, provide API access
to toggle on/off the inclusion of `archived` objects
While implementing this behavior the following behavior was
encountered:
1. `vulnerability_read` vs `vulnerability` differences
The [existing behavior][1] of `Group.vulnerabilities` is to ignore
vulnerabilities associated with archived projects.
The [existing behavior][2] of `Group.vulnerability_reads` does not
necessarily account for the archived status of associated
projects
This commit unifies both to ignoring archive status, leaving that
to be done in filters for the finders.
[0]:https://gitlab.com/gitlab-org/gitlab/-/issues/213144
[1]:https://gitlab.com/gitlab-org/gitlab/-/blob/45e01fad9dffb4c447bf701e040d552077f60dac/ee/app/models/ee/group.rb#L555-557
[2]:https://gitlab.com/gitlab-org/gitlab/-/blob/45e01fad9dffb4c447bf701e040d552077f60dac/ee/app/models/ee/group.rb#L559-561
Changelog: fixed
EE: true
---
.../security/vulnerabilities_finder.rb | 20 +++++-
.../security/vulnerability_reads_finder.rb | 20 +++++-
.../models/concerns/vulnerability_scopes.rb | 3 +-
ee/app/models/ee/group.rb | 4 +-
.../security/vulnerabilities_finder_spec.rb | 64 +++++++++++++++++--
.../vulnerability_reads_finder_spec.rb | 64 +++++++++++++++++--
ee/spec/models/ee/group_spec.rb | 4 +-
...lnerability_and_finding_shared_examples.rb | 40 +++++++-----
8 files changed, 183 insertions(+), 36 deletions(-)
diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb
index 342d486d3442e..015a52171a97a 100644
--- a/ee/app/finders/security/vulnerabilities_finder.rb
+++ b/ee/app/finders/security/vulnerabilities_finder.rb
@@ -9,6 +9,8 @@
# 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
@@ -22,7 +24,7 @@ class VulnerabilitiesFinder
include FinderMethods
def initialize(vulnerable, params = {})
- @params = params
+ @params = { include_archived_projects: false }.merge(params)
@vulnerable = vulnerable
@vulnerabilities = vulnerable.vulnerabilities
end
@@ -31,6 +33,7 @@ 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
@@ -58,9 +61,22 @@ def filter_by_present_on_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])
+ @vulnerabilities = vulnerabilities.for_projects(
+ params[:project_id],
+ params[:include_archived_projects]
+ )
end
end
diff --git a/ee/app/finders/security/vulnerability_reads_finder.rb b/ee/app/finders/security/vulnerability_reads_finder.rb
index dcc2753d86803..f8d06217b76ab 100644
--- a/ee/app/finders/security/vulnerability_reads_finder.rb
+++ b/ee/app/finders/security/vulnerability_reads_finder.rb
@@ -9,6 +9,8 @@
# 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
@@ -25,13 +27,14 @@ class VulnerabilityReadsFinder
include FinderMethods
def initialize(vulnerable, params = {})
- @params = params
+ @params = { include_archived_projects: false }.merge(params)
@vulnerable = vulnerable
@vulnerability_reads = vulnerable.vulnerability_reads
end
def execute
use_unnested_filters
+ filter_archived_projects
filter_by_projects
filter_by_image
filter_by_report_types
@@ -60,10 +63,23 @@ def use_unnested_filters?
vulnerable.is_a?(Project) || vulnerable.is_a?(Group)
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?
+
+ @vulnerability_reads = vulnerability_reads.without_archived_projects
+ end
+
def filter_by_projects
return unless params[:project_id].present?
- @vulnerability_reads = vulnerability_reads.for_projects(params[:project_id])
+ @vulnerability_reads = vulnerability_reads.for_projects(
+ params[:project_id],
+ params[:include_archived_projects]
+ )
end
def filter_by_report_types
diff --git a/ee/app/models/concerns/vulnerability_scopes.rb b/ee/app/models/concerns/vulnerability_scopes.rb
index ce660205e2f58..3556fa15068c4 100644
--- a/ee/app/models/concerns/vulnerability_scopes.rb
+++ b/ee/app/models/concerns/vulnerability_scopes.rb
@@ -4,11 +4,12 @@ module VulnerabilityScopes
extend ActiveSupport::Concern
included do
+ scope :without_archived_projects, -> { joins(:project).merge(::Project.non_archived) }
scope :for_projects, ->(project_ids, include_archived = false) do
if include_archived
where(project_id: project_ids)
else
- joins(:project).merge(::Project.non_archived).where(project_id: project_ids)
+ without_archived_projects.where(project_id: project_ids)
end
end
end
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 859515f2b208d..259207a01cc1c 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -556,7 +556,9 @@ def adjourned_deletion?
end
def vulnerabilities
- ::Vulnerability.where(project: projects_for_group_and_its_subgroups_without_deleted)
+ ::Vulnerability.where(
+ project: ::Project.for_group_and_its_subgroups(self).without_deleted
+ )
end
def vulnerability_reads
diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb
index 46f98a420b957..585c54ddcef89 100644
--- a/ee/spec/finders/security/vulnerabilities_finder_spec.rb
+++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb
@@ -2,8 +2,15 @@
require 'spec_helper'
-RSpec.describe Security::VulnerabilitiesFinder do
+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)
@@ -19,6 +26,7 @@
let(:filters) { {} }
let(:vulnerable) { project }
+ let(:archive_associated_vulnerabilities) { archived_project.vulnerabilities }
subject { described_class.new(vulnerable, filters).execute }
@@ -34,6 +42,33 @@
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] } }
@@ -74,19 +109,34 @@
end
end
- context 'when filtered by project' do
- let(:group) { create(:group) }
+ 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(:filters) { { project_id: [another_project.id] } }
- let(:vulnerable) { group }
+
+ let_it_be(:group) { create(:group) }
+ let_it_be(:archived_project) { create(:project, :archived, namespace: group) }
before do
project.update!(namespace: group)
end
- it 'only returns vulnerabilities matching the given projects' do
- is_expected.to contain_exactly(another_vulnerability)
+ 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
diff --git a/ee/spec/finders/security/vulnerability_reads_finder_spec.rb b/ee/spec/finders/security/vulnerability_reads_finder_spec.rb
index 1b6220cb1b3f2..ed30bc6cf19be 100644
--- a/ee/spec/finders/security/vulnerability_reads_finder_spec.rb
+++ b/ee/spec/finders/security/vulnerability_reads_finder_spec.rb
@@ -52,6 +52,43 @@
subject(:execute) { described_class.new(vulnerable, filters).execute }
+ context 'when using the include_archived_projects param' do
+ using RSpec::Parameterized::TableSyntax
+
+ let(:archive_associated_vulns) { archived_project.vulnerabilities.map(&:vulnerability_read) }
+ let(:not_archived_vulns) { project.vulnerabilities.map(&:vulnerability_read) }
+ let(:all_vulns) { archive_associated_vulns + not_archived_vulns }
+ let_it_be(:group) { create(:group) }
+
+ let_it_be(:project) do
+ create(:project, namespace: group).tap do |p|
+ create(:vulnerability, :with_finding, project: p)
+ end
+ end
+
+ let_it_be(:archived_project) do
+ create(:project, :archived, namespace: group).tap do |p|
+ create(:vulnerability, :with_finding, project: p)
+ end
+ end
+
+ where(:vulnerable_object, :include_archived_projects, :result) do
+ ref(:archived_project) | true | ref(:archive_associated_vulns)
+ ref(:archived_project) | false | ref(:archive_associated_vulns)
+ ref(:group) | true | ref(:all_vulns)
+ ref(:group) | false | ref(:not_archived_vulns)
+ 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 eq(result)
+ end
+ end
+ end
+
context 'when not given a second argument' do
subject { described_class.new(project).execute }
@@ -136,19 +173,34 @@
end
end
- context 'when filtered by project' do
- let(:group) { create(:group) }
+ context 'when vulnerable is a Group' do
let(:another_project) { create(:project, namespace: group) }
- let!(:another_vulnerability) { create(:vulnerability, :with_finding, project: another_project) }
let(:vulnerable) { group }
- let(:filters) { { project_id: [another_project.id] } }
+
+ let!(:another_vulnerability) { create(:vulnerability, :with_finding, 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
- it 'only returns vulnerabilities matching the given projects' do
- is_expected.to contain_exactly(another_vulnerability.vulnerability_read)
+ 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.vulnerability_read)
+ 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.vulnerability_read, archived_vulnerability.vulnerability_read)
+ end
+ end
end
end
diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb
index 21149b6861f63..6b81f8c37cc93 100644
--- a/ee/spec/models/ee/group_spec.rb
+++ b/ee/spec/models/ee/group_spec.rb
@@ -681,8 +681,8 @@
let!(:archived_vulnerability) { create(:vulnerability, project: archived_project) }
let!(:deleted_vulnerability) { create(:vulnerability, project: deleted_project) }
- it 'returns vulnerabilities for all non-archived, non-deleted projects in the group and its subgroups' do
- is_expected.to contain_exactly(group_vulnerability, subgroup_vulnerability)
+ it 'returns vulnerabilities for all non-deleted projects in the group and its subgroups' do
+ is_expected.to contain_exactly(group_vulnerability, subgroup_vulnerability, archived_vulnerability)
end
end
diff --git a/ee/spec/support/shared_examples/models/vulnerability_and_finding_shared_examples.rb b/ee/spec/support/shared_examples/models/vulnerability_and_finding_shared_examples.rb
index 34c04b21252dd..a896b7b69307b 100644
--- a/ee/spec/support/shared_examples/models/vulnerability_and_finding_shared_examples.rb
+++ b/ee/spec/support/shared_examples/models/vulnerability_and_finding_shared_examples.rb
@@ -4,39 +4,49 @@
RSpec.shared_examples 'vulnerability and finding shared examples' do
describe 'scopes' do
- describe '.for_projects' do
- subject { described_class.for_projects(*params) }
+ let(:result_set_transformer) { transformer_method || :itself }
+ let(:archive_associated_objects) { archived_project.vulnerabilities.map(&result_set_transformer) }
+ let(:all_excluding_archive_associations) { all_vulns_for_params - archive_associated_objects }
- let(:params) { [[project.id, archived_project.id], include_archived].compact }
+ let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) }
+ let_it_be(:archived_project) do
+ create(:project, :archived).tap { |p| create(:vulnerability, :with_finding, project: p) }
+ end
- let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) }
- let_it_be(:archived_project) { create(:project, :archived) }
+ describe '.without_archived_projects' do
+ subject { described_class.without_archived_projects }
- let(:result_set_transformer) { transformer_method || :itself }
- let(:expected_all_projects) do
- (project.vulnerabilities + archived_project.vulnerabilities).map(&result_set_transformer)
+ let(:all_vulns_for_params) { described_class.all }
+
+ it 'returns objects where the associated project is not archived' do
+ is_expected.to contain_exactly(*all_excluding_archive_associations)
end
+ end
- let(:expected_excluding_archives) { project.vulnerabilities.map(&result_set_transformer) }
+ describe '.for_projects' do
+ subject { described_class.for_projects(*params) }
let(:include_archived) { nil }
- let!(:archived_projects_vuln) { create(:vulnerability, :with_finding, project: archived_project) }
+
+ let(:params) { [[project.id, archived_project.id], include_archived].compact }
+ let(:all_vulns_for_params) do
+ (project.vulnerabilities + archived_project.vulnerabilities).map(&result_set_transformer)
+ end
before do
- # create a vulnerability associated on a project that will be
- # filtered out
+ # vulnerability associated on a project that will be filtered out
create(:vulnerability, :with_finding, project: create(:project))
end
it 'returns objects related to the given project IDs' do
- is_expected.to contain_exactly(*expected_excluding_archives)
+ is_expected.to contain_exactly(*all_excluding_archive_associations)
end
context 'when including archived projects' do
let(:include_archived) { true }
it 'returns objects related to the given project IDs' do
- is_expected.to contain_exactly(*expected_all_projects)
+ is_expected.to contain_exactly(*all_vulns_for_params)
end
end
@@ -44,7 +54,7 @@
let(:include_archived) { false }
it 'returns objects related to the given project IDs' do
- is_expected.to contain_exactly(*expected_excluding_archives)
+ is_expected.to contain_exactly(*all_excluding_archive_associations)
end
end
end
--
GitLab