From 5672df16c7c110c18d1c4f07c0702557ab5b616e Mon Sep 17 00:00:00 2001
From: Michael Becker <mbecker@gitlab.com>
Date: Wed, 12 Feb 2025 15:34:28 +0700
Subject: [PATCH] Add a `/disabled_filters` route to the dependencies
controllers
Context
-------------------------
We have some filters that we disable when certain conditions
are met, for example if a group has too many vulnerabilities.
Currently, if you try filtering with a disabled filter, you
will get a "too many vulnerabilities" error response. We would
like to give a tool-tip in the UI to explain the filter is
disabled ahead of any error response. We have done that for a
few filters by injecting booleans into the page on load.
As we continue adding filters that need to be disabled in certain
circumstances, we are looking for a way frontend could reliably know
what filters are disabled for a given `vulnerable`. We achieve this
via a graphql extension.
Future work
-------------------------
rather than calculating these disabled filters every time, we could
have frontend request specific "filter hints" and only build the array
if requested
This change
-------------------------
1. Adds a `disabled_filters` array to the graphql response if the
vulnerability resolver is invoked
- behind a de-risk FF
2. DRYs up the code that disables the identifier search
epic: https://gitlab.com/groups/gitlab-org/-/epics/13340
related to: https://gitlab.com/gitlab-org/gitlab/-/issues/517915
resolves: https://gitlab.com/gitlab-org/gitlab/-/work_items/517925
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180973
Changelog: changed
EE: truewe have some filters that we disable when certain conditions
are met, for example if a group has too many vulnerabilities.
---
.../security/group_identifier_search.rb | 26 ++++++++
.../identifier_search_resolver.rb | 12 ++--
.../resolvers/vulnerabilities_resolver.rb | 27 +++++---
.../resolvers/vulnerability_filterable.rb | 14 +---
...disabled_vulnerability_graphql_filters.yml | 9 +++
ee/spec/graphql/api/vulnerabilities_spec.rb | 66 ++++++++++++++++++-
.../vulnerabilities_resolver_spec.rb | 2 +-
...rability_severities_count_resolver_spec.rb | 2 +-
8 files changed, 125 insertions(+), 33 deletions(-)
create mode 100644 ee/app/controllers/concerns/security/group_identifier_search.rb
create mode 100644 ee/config/feature_flags/gitlab_com_derisk/return_disabled_vulnerability_graphql_filters.yml
diff --git a/ee/app/controllers/concerns/security/group_identifier_search.rb b/ee/app/controllers/concerns/security/group_identifier_search.rb
new file mode 100644
index 00000000000000..3189b05157a53b
--- /dev/null
+++ b/ee/app/controllers/concerns/security/group_identifier_search.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module Security
+ # rubocop:disable Search/NamespacedClass -- not that kind of search
+ module GroupIdentifierSearch
+ MAX_VULNERABILITY_COUNT_GROUP_SUPPORT = 20_000
+
+ private
+
+ def search_by_identifier_allowed?(vulnerable:)
+ return true if vulnerable.is_a?(::Project)
+ return false unless vulnerable.is_a?(::Group)
+
+ vulnerability_count = ::Security::ProjectStatistics.sum_vulnerability_count_for_group(vulnerable)
+ vulnerability_count <= MAX_VULNERABILITY_COUNT_GROUP_SUPPORT
+ end
+
+ def search_by_identifier_allowed!(vulnerable:)
+ return if search_by_identifier_allowed?(vulnerable: vulnerable)
+
+ raise ::Gitlab::Graphql::Errors::ArgumentError,
+ "#{vulnerable.type} has more than 20k vulnerabilities."
+ end
+ end
+ # rubocop:enable Search/NamespacedClass
+end
diff --git a/ee/app/graphql/resolvers/vulnerabilities/identifier_search_resolver.rb b/ee/app/graphql/resolvers/vulnerabilities/identifier_search_resolver.rb
index ce7217b65fdefc..e64517a97b6b26 100644
--- a/ee/app/graphql/resolvers/vulnerabilities/identifier_search_resolver.rb
+++ b/ee/app/graphql/resolvers/vulnerabilities/identifier_search_resolver.rb
@@ -4,8 +4,7 @@ module Resolvers
module Vulnerabilities
class IdentifierSearchResolver < BaseResolver
include Gitlab::Graphql::Authorize::AuthorizeResource
-
- MAX_VULNERABILITY_COUNT_GROUP_SUPPORT = 20_000
+ include ::Security::GroupIdentifierSearch
authorize :read_security_resource
@@ -23,7 +22,7 @@ def resolve(**args)
validate_args(args)
- group_allowed_for_search?
+ search_by_identifier_allowed!(vulnerable: object)
if object.is_a?(::Project)
::Vulnerabilities::Identifier.search_identifier_name(
@@ -54,13 +53,10 @@ def authorize!
raise_resource_not_available_error!
end
- def group_allowed_for_search?
+ def group_allowed_for_search!
return unless object.is_a?(Group)
- vulnerability_count = ::Security::ProjectStatistics.sum_vulnerability_count_for_group(object)
- allowed = vulnerability_count <= MAX_VULNERABILITY_COUNT_GROUP_SUPPORT
-
- raise ::Gitlab::Graphql::Errors::ArgumentError, 'Group has more than 20k vulnerabilities.' unless allowed
+ search_by_identifier_allowed?(vulnerable: object)
end
end
end
diff --git a/ee/app/graphql/resolvers/vulnerabilities_resolver.rb b/ee/app/graphql/resolvers/vulnerabilities_resolver.rb
index b6b1c854606580..73103a646e8e21 100644
--- a/ee/app/graphql/resolvers/vulnerabilities_resolver.rb
+++ b/ee/app/graphql/resolvers/vulnerabilities_resolver.rb
@@ -100,6 +100,11 @@ def resolve_with_lookahead(**args)
args[:scanner_id] = resolve_gids(args[:scanner_id], ::Vulnerabilities::Scanner) if args[:scanner_id]
+ if Feature.enabled?(:return_disabled_vulnerability_graphql_filters, vulnerable_to_actor)
+ disabled_filters = context.response_extensions["disabled_filters"] ||= []
+ disabled_filters << :identifier_name unless search_by_identifier_allowed?(vulnerable: vulnerable)
+ end
+
vulnerabilities(args)
.with_findings_scanner_and_identifiers
end
@@ -129,16 +134,18 @@ def vulnerabilities(params)
end
def resolve_with_duo_filtering_enabled?
- actor = case vulnerable
- when ::InstanceSecurityDashboard
- current_user
- when Project
- vulnerable.group
- else
- vulnerable
- end
-
- Feature.enabled?(:vulnerability_report_vr_filter, actor)
+ Feature.enabled?(:vulnerability_report_vr_filter, vulnerable_to_actor)
+ end
+
+ def vulnerable_to_actor
+ case vulnerable
+ when ::InstanceSecurityDashboard
+ current_user
+ when Project
+ vulnerable.group
+ else
+ vulnerable
+ end
end
def after_severity
diff --git a/ee/app/graphql/resolvers/vulnerability_filterable.rb b/ee/app/graphql/resolvers/vulnerability_filterable.rb
index 4e8e50d5ee2f07..eca6da8e43730e 100644
--- a/ee/app/graphql/resolvers/vulnerability_filterable.rb
+++ b/ee/app/graphql/resolvers/vulnerability_filterable.rb
@@ -3,23 +3,13 @@
module Resolvers
module VulnerabilityFilterable
extend ActiveSupport::Concern
-
- MAX_VULNERABILITY_COUNT_GROUP_SUPPORT = 20_000
+ include ::Security::GroupIdentifierSearch
private
def validate_filters(filters)
validate_owasp_top_ten(filters) if filters[:owasp_top_10].present?
- validate_filter_by_identifier_name if filters[:identifier_name].present?
- end
-
- def validate_filter_by_identifier_name
- return unless object.is_a?(Group)
-
- vulnerability_count = ::Security::ProjectStatistics.sum_vulnerability_count_for_group(object)
- allowed = vulnerability_count <= MAX_VULNERABILITY_COUNT_GROUP_SUPPORT
-
- raise ::Gitlab::Graphql::Errors::ArgumentError, 'Group has more than 20k vulnerabilities.' unless allowed
+ search_by_identifier_allowed!(vulnerable: object) if filters[:identifier_name].present?
end
def validate_owasp_top_ten(filters)
diff --git a/ee/config/feature_flags/gitlab_com_derisk/return_disabled_vulnerability_graphql_filters.yml b/ee/config/feature_flags/gitlab_com_derisk/return_disabled_vulnerability_graphql_filters.yml
new file mode 100644
index 00000000000000..2ee4d829eb0b38
--- /dev/null
+++ b/ee/config/feature_flags/gitlab_com_derisk/return_disabled_vulnerability_graphql_filters.yml
@@ -0,0 +1,9 @@
+---
+name: return_disabled_vulnerability_graphql_filters
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/work_items/517925
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180973
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/work_items/520703
+milestone: '17.10'
+group: group::security insights
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/ee/spec/graphql/api/vulnerabilities_spec.rb b/ee/spec/graphql/api/vulnerabilities_spec.rb
index acccca1e873f7e..85c56956cceee4 100644
--- a/ee/spec/graphql/api/vulnerabilities_spec.rb
+++ b/ee/spec/graphql/api/vulnerabilities_spec.rb
@@ -2,7 +2,8 @@
require 'spec_helper'
-RSpec.describe 'vulnerabilities' do
+RSpec.describe 'vulnerabilities', feature_category: :vulnerability_management do
+ let_it_be(:group) { create(:group) }
let_it_be(:project) { create(:project) }
let_it_be(:user) { project.first_owner }
let_it_be(:vulnerability_1) { create(:vulnerability, project: project) }
@@ -28,6 +29,7 @@
end
let(:vulnerabilities) { execute.dig('data', 'project', 'vulnerabilities') }
+ let(:extensions) { execute['extensions'] }
subject(:execute) { GitlabSchema.execute(query, context: { current_user: user }).as_json }
@@ -40,6 +42,68 @@
create(:vulnerabilities_finding, vulnerability: vulnerability_1, project: project)
end
+ describe 'extensions' do
+ context 'when the query could use vulnerability identifier filtering' do
+ let(:query) do
+ %(
+ query {
+ #{vulnerable.class.name.downcase}(fullPath: "#{vulnerable.full_path}") {
+ name
+ vulnerabilities { nodes { id } } }})
+ end
+
+ before do
+ test_vulnerability_limit = 1
+ stub_const(
+ '::Security::GroupIdentifierSearch::MAX_VULNERABILITY_COUNT_GROUP_SUPPORT',
+ test_vulnerability_limit
+ )
+ allow(::Security::ProjectStatistics).to receive(:sum_vulnerability_count_for_group)
+ .with(group).and_return(test_vulnerability_limit + 1)
+ end
+
+ context 'when vulnerable is project' do
+ let(:vulnerable) { project }
+
+ it { expect(extensions["disabled_filters"]).to eq [] }
+
+ context 'when the return_disabled_vulnerability_graphql_filters FF is disabled' do
+ before do
+ stub_feature_flags(return_disabled_vulnerability_graphql_filters: false)
+ end
+
+ it "does not calculate disabled vulnerability filters" do
+ expect(extensions).to be_nil
+ end
+ end
+ end
+
+ context 'when vulnerable is group' do
+ let(:vulnerable) { group }
+
+ context 'when the return_disabled_vulnerability_graphql_filters FF is disabled' do
+ before do
+ stub_feature_flags(return_disabled_vulnerability_graphql_filters: false)
+ end
+
+ it "does not calculate disabled vulnerability filters" do
+ expect(extensions).to be_nil
+ end
+ end
+
+ it { expect(extensions["disabled_filters"]).to match_array %w[identifier_name] }
+ end
+
+ context 'when the vulnerability type is not in the query' do
+ let(:query) { %(query { group(fullPath: "#{group.full_path}") { name } }) }
+
+ it "does not calculate disabled vulnerability filters" do
+ expect(extensions).to be_nil
+ end
+ end
+ end
+ end
+
describe 'nodes' do
let(:nodes) { vulnerabilities['nodes'] }
let(:vulnerability_ids) { nodes.map { |node| node['id'] } }
diff --git a/ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb b/ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb
index 88f261bfbbafd9..6dbb1ae552baf7 100644
--- a/ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb
+++ b/ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb
@@ -399,7 +399,7 @@
before do
stub_const(
- 'Resolvers::VulnerabilityFilterable::MAX_VULNERABILITY_COUNT_GROUP_SUPPORT',
+ '::Security::GroupIdentifierSearch::MAX_VULNERABILITY_COUNT_GROUP_SUPPORT',
max
)
diff --git a/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb b/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb
index f45ac4f7bcd5ac..dab9651f348257 100644
--- a/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb
+++ b/ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb
@@ -312,7 +312,7 @@
before do
stub_const(
- 'Resolvers::VulnerabilityFilterable::MAX_VULNERABILITY_COUNT_GROUP_SUPPORT', max
+ '::Security::GroupIdentifierSearch::MAX_VULNERABILITY_COUNT_GROUP_SUPPORT', max
)
allow(::Security::ProjectStatistics).to receive(:sum_vulnerability_count_for_group)
.with(group).and_return(group.vulnerabilities.count)
--
GitLab