diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index 3d09587745f66fcc2116305986bfb5ddf45bd633..f540fbd686b9c7cb3e65266f12a4e380e13eb5a4 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -33503,7 +33503,6 @@ Returns [`FindingReportsComparer`](#findingreportscomparer). | Name | Type | Description | | ---- | ---- | ----------- | | `reportType` | [`ComparableSecurityReportType!`](#comparablesecurityreporttype) | Filter vulnerability findings by report type. | -| `scanMode` | [`ScanModeEnum`](#scanmodeenum) | Filter results by scan mode. | ##### `MergeRequest.notes` @@ -48930,16 +48929,6 @@ Values for sbom source types. | `DEPENDENCY_SCANNING` | Source Type: dependency_scanning. | | `NIL_SOURCE` | Enum source nil. | -### `ScanModeEnum` - -Options for filtering by scan mode. - -| Value | Description | -| ----- | ----------- | -| `ALL` | Return results from all scans. | -| `FULL` | Return results from full scans. | -| `PARTIAL` | Return results from partial scans. | - ### `ScanStatus` The status of the security scan. diff --git a/ee/app/controllers/ee/projects/merge_requests_controller.rb b/ee/app/controllers/ee/projects/merge_requests_controller.rb index 3b7f5aedfd4f6c8738767dbda0fa9a6fc85d0099..1ca9b998531a6f18dcea48e5efe7413eebaadd16 100644 --- a/ee/app/controllers/ee/projects/merge_requests_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests_controller.rb @@ -97,9 +97,7 @@ def api_fuzzing_reports end def security_reports - params[:report_type] = params[:type] - comparison_params = params.permit(:report_type, :scan_mode) - report = ::Security::MergeRequestSecurityReportGenerationService.execute(merge_request, comparison_params) + report = ::Security::MergeRequestSecurityReportGenerationService.execute(merge_request, params[:type]) reports_response(report, head_pipeline) rescue ::Security::MergeRequestSecurityReportGenerationService::InvalidReportTypeError diff --git a/ee/app/finders/security/findings_finder.rb b/ee/app/finders/security/findings_finder.rb index 01fb0986415ca92cc28a23249acce0b8cefc2ed9..7da86d1ece8defff8e9e529fb5ec6bc5d5902336 100644 --- a/ee/app/finders/security/findings_finder.rb +++ b/ee/app/finders/security/findings_finder.rb @@ -8,11 +8,10 @@ # Arguments: # pipeline - object to filter findings # params: -# severity: Array -# report_type: Array -# scan_mode: String -# scope: String -# limit: Int +# severity: Array +# report_type: Array +# scope: String +# limit: Int module Security class FindingsFinder @@ -66,7 +65,6 @@ def execute .then { |relation| by_scanner_external_ids(relation) } .then { |relation| by_state(relation) } .then { |relation| by_include_dismissed(relation) } - .then { |relation| by_scan_mode(relation) } .limit(limit + 1) from_sql = <<~SQL.squish @@ -140,34 +138,6 @@ def by_report_types(relation) relation.merge(::Security::Scan.by_scan_types(params[:report_type])) end - def by_scan_mode(relation) - scan_mode = params.fetch(:scan_mode, 'all') - - return relation if ::Feature.disabled?(:vulnerability_partial_scans, project) || scan_mode == 'all' - - exists_clause = <<~SQL - EXISTS ( - SELECT - 1 - FROM - "vulnerability_partial_scans" - WHERE - "vulnerability_partial_scans"."scan_id" = "security_scans"."id" - ) - SQL - - # rubocop:disable CodeReuse/ActiveRecord -- Requires lateral join with security scans - case scan_mode - when 'full' - relation.where.not(exists_clause) - when 'partial' - relation.where(exists_clause) - else - relation - end - # rubocop:enable CodeReuse/ActiveRecord - end - def severities if params[:severity] Security::Finding.severities.fetch_values(*params[:severity]) diff --git a/ee/app/graphql/resolvers/security_report/finding_reports_comparer_resolver.rb b/ee/app/graphql/resolvers/security_report/finding_reports_comparer_resolver.rb index bb7131fb5603a7458e0cb8852be0986cf0ec318b..0ae3fb3d665691ac2a50eaf778bdd9c494b84c3b 100644 --- a/ee/app/graphql/resolvers/security_report/finding_reports_comparer_resolver.rb +++ b/ee/app/graphql/resolvers/security_report/finding_reports_comparer_resolver.rb @@ -14,15 +14,11 @@ class FindingReportsComparerResolver < BaseResolver required: true, description: 'Filter vulnerability findings by report type.' - argument :scan_mode, Types::Security::ScanModeEnum, - required: false, - description: 'Filter results by scan mode.' - - def resolve(**params) + def resolve(report_type:) # Necessary to use project as actor in FF check context[:project] = object.project - ::Security::MergeRequestSecurityReportGenerationService.execute(object, params) + ::Security::MergeRequestSecurityReportGenerationService.execute(object, report_type) end end end diff --git a/ee/app/graphql/types/security/scan_mode_enum.rb b/ee/app/graphql/types/security/scan_mode_enum.rb deleted file mode 100644 index 83e3f918266cfb1cc1e685128629ec06b6c3e244..0000000000000000000000000000000000000000 --- a/ee/app/graphql/types/security/scan_mode_enum.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -module Types - module Security - class ScanModeEnum < Types::BaseEnum - graphql_name 'ScanModeEnum' - description 'Options for filtering by scan mode.' - - value 'ALL', value: 'all', description: "Return results from all scans." - value 'FULL', value: 'full', description: "Return results from full scans." - value 'PARTIAL', value: 'partial', description: "Return results from partial scans." - end - end -end diff --git a/ee/app/services/ci/compare_security_reports_service.rb b/ee/app/services/ci/compare_security_reports_service.rb index 4cbafda355f518d3423ac4a85f4c4d02c00317dd..1965615c0efe15175e89a78dbbbd43b62c08667f 100644 --- a/ee/app/services/ci/compare_security_reports_service.rb +++ b/ee/app/services/ci/compare_security_reports_service.rb @@ -68,7 +68,6 @@ def get_report(pipeline) params: { report_type: [params[:report_type]], scope: 'all', - scan_mode: params[:scan_mode], limit: Gitlab::Ci::Reports::Security::SecurityFindingsReportsComparer::MAX_FINDINGS_COUNT } ).execute.with_api_scopes diff --git a/ee/app/services/security/merge_request_security_report_generation_service.rb b/ee/app/services/security/merge_request_security_report_generation_service.rb index 666993e8c1e78fc5c8b41e585aa32f595f56569e..6ede9cac4bbb7bd50dcb694b2debe76f17f6cfc5 100644 --- a/ee/app/services/security/merge_request_security_report_generation_service.rb +++ b/ee/app/services/security/merge_request_security_report_generation_service.rb @@ -3,7 +3,6 @@ module Security class MergeRequestSecurityReportGenerationService include Gitlab::Utils::StrongMemoize - include ReactiveCaching DEFAULT_FINDING_STATE = 'detected' ALLOWED_REPORT_TYPES = %w[sast secret_detection container_scanning @@ -11,24 +10,13 @@ class MergeRequestSecurityReportGenerationService InvalidReportTypeError = Class.new(ArgumentError) - self.reactive_cache_work_type = :no_dependency - self.reactive_cache_worker_finder = ->(id, params) { from_cache(id, params) } - - def self.execute(merge_request, params) - new(merge_request, params).execute - end - - def self.from_cache(merge_request_id, params) - merge_request = ::MergeRequest.find_by_id(merge_request_id) - - return unless merge_request - - new(merge_request, params) + def self.execute(merge_request, report_type) + new(merge_request, report_type).execute end - def initialize(merge_request, params) + def initialize(merge_request, report_type) @merge_request = merge_request - @params = params.to_h.symbolize_keys + @report_type = report_type end def execute @@ -40,15 +28,9 @@ def execute report end - delegate :project, :id, to: :merge_request - private - attr_reader :merge_request, :params - - def report_type - params[:report_type] - end + attr_reader :merge_request, :report_type def report_available? report[:status] == :parsed @@ -106,30 +88,26 @@ def fixed_findings strong_memoize_attr def report validate_report_type! - with_reactive_cache(params.stringify_keys) do |data| - latest = Ci::CompareSecurityReportsService.new(project, nil, params).latest?(base_pipeline, head_pipeline, data) - raise InvalidateReactiveCache unless latest - - data - end || { status: :parsing } + case report_type + when 'sast' + merge_request.compare_sast_reports(nil) + when 'secret_detection' + merge_request.compare_secret_detection_reports(nil) + when 'container_scanning' + merge_request.compare_container_scanning_reports(nil) + when 'dependency_scanning' + merge_request.compare_dependency_scanning_reports(nil) + when 'dast' + merge_request.compare_dast_reports(nil) + when 'coverage_fuzzing' + merge_request.compare_coverage_fuzzing_reports(nil) + when 'api_fuzzing' + merge_request.compare_api_fuzzing_reports(nil) + end end def validate_report_type! raise InvalidReportTypeError unless ALLOWED_REPORT_TYPES.include?(report_type) end - - def calculate_reactive_cache(cache_params) - Ci::CompareSecurityReportsService - .new(project, nil, cache_params.symbolize_keys) - .execute(base_pipeline, head_pipeline) - end - - def base_pipeline - merge_request.comparison_base_pipeline(Ci::CompareSecurityReportsService) - end - - def head_pipeline - merge_request.diff_head_pipeline - end end end diff --git a/ee/spec/factories/ci/builds.rb b/ee/spec/factories/ci/builds.rb index 0812746a8522be74829cc5872e88f036f80542b2..e5da1bc0aa59066c941aa67a9a91d622af8ad803 100644 --- a/ee/spec/factories/ci/builds.rb +++ b/ee/spec/factories/ci/builds.rb @@ -91,12 +91,6 @@ end end - trait :sast_differential_scan do - after(:build) do |build| - build.job_artifacts << build(:ee_ci_job_artifact, :sast_differential_scan, job: build) - end - end - trait :secret_detection_feature_branch do after(:build) do |build| build.job_artifacts << build(:ee_ci_job_artifact, :secret_detection_feature_branch, job: build) diff --git a/ee/spec/factories/ci/pipelines.rb b/ee/spec/factories/ci/pipelines.rb index d7092e0b2396d5d214577db07b1848fd56d50746..19cf911270afdf057500b739771da21ba9eabb3b 100644 --- a/ee/spec/factories/ci/pipelines.rb +++ b/ee/spec/factories/ci/pipelines.rb @@ -71,14 +71,6 @@ end end - trait :sast_differential_scan do - status { :success } - - after(:build) do |pipeline, evaluator| - pipeline.builds << build(:ee_ci_build, :sast_differential_scan, pipeline: pipeline, project: pipeline.project) - end - end - trait :with_secret_detection_feature_branch do status { :success } diff --git a/ee/spec/finders/security/findings_finder_spec.rb b/ee/spec/finders/security/findings_finder_spec.rb index c2f6d82afab478d7b93982e96ade2beaf11b60a8..e7afc6036d4acdc827d77f631c26b0885db9dec7 100644 --- a/ee/spec/finders/security/findings_finder_spec.rb +++ b/ee/spec/finders/security/findings_finder_spec.rb @@ -49,8 +49,6 @@ context 'when the pipeline has security findings' do let(:findings) { service_object.execute.to_a } - let(:duplicate_security_finding_uuids) { [Security::Finding.second[:uuid]] } - let(:expected_uuids) { result_uuids - duplicate_security_finding_uuids } before_all do ds_content = File.read(artifact_ds.file.path) @@ -106,7 +104,11 @@ describe '#findings' do context 'when the `security_findings` records have `overridden_uuid`s' do let(:security_findings) { Security::Finding.by_build_ids(build_1) } - let(:result_uuids) { Security::Finding.pluck(:uuid) } + let(:security_finding_uuids) { Security::Finding.pluck(:uuid) } + let(:nondeduplicated_security_finding_uuid) { Security::Finding.second[:uuid] } + let(:expected_uuids) do + security_finding_uuids - Array(nondeduplicated_security_finding_uuid) + end subject { findings.map(&:uuid) } @@ -137,7 +139,7 @@ subject { findings.map(&:uuid) } context 'with the default parameters' do - let(:result_uuids) { Security::Finding.pluck(:uuid) } + let(:expected_uuids) { Security::Finding.pluck(:uuid) - [Security::Finding.second[:uuid]] } it { is_expected.to match_array(expected_uuids) } end @@ -177,8 +179,9 @@ context 'when the `report_types` is provided' do let(:report_types) { :dependency_scanning } - let(:result_uuids) do - Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) + let(:expected_uuids) do + Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) - + [Security::Finding.second[:uuid]] end it { is_expected.to match_array(expected_uuids) } @@ -186,7 +189,8 @@ context 'when the `scope` is provided as `all`' do let(:scope) { 'all' } - let(:result_uuids) { Security::Finding.pluck(:uuid) } + + let(:expected_uuids) { Security::Finding.pluck(:uuid) - [Security::Finding.second[:uuid]] } it { is_expected.to match_array(expected_uuids) } end @@ -216,8 +220,9 @@ let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning, job: retried_build) } let(:report) { create(:ci_reports_security_report, pipeline: pipeline, type: :dependency_scanning) } let(:report_types) { :dependency_scanning } - let(:result_uuids) do - Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) + let(:expected_uuids) do + Security::Finding.by_scan(Security::Scan.find_by(scan_type: 'dependency_scanning')).pluck(:uuid) - + [Security::Finding.second[:uuid]] end before do @@ -285,66 +290,6 @@ it { is_expected.to eq(vulnerability_finding.vulnerability) } end end - - context 'when there is a partial scan' do - let_it_be(:sast_scan) { Security::Scan.find_by(scan_type: 'sast') } - let_it_be(:other_scans) { Security::Scan.id_not_in(sast_scan.id) } - - before_all do - create(:vulnerabilities_partial_scan, scan: sast_scan) - end - - context 'with no params' do - let(:params) { {} } - let(:result_uuids) { Security::Finding.pluck(:uuid) } - - it 'returns both full and partial scan results' do - is_expected.to match_array(expected_uuids) - end - end - - context 'when scan_mode is full' do - let(:params) { { scan_mode: 'full' } } - let(:result_uuids) { other_scans.flat_map(&:findings).pluck(:uuid) } - - it 'returns only full scan results' do - is_expected.to match_array(expected_uuids) - end - - context 'when feature is disabled' do - before do - stub_feature_flags(vulnerability_partial_scans: false) - end - - let(:result_uuids) { Security::Finding.pluck(:uuid) } - - it 'returns both full and partial scan results' do - is_expected.to match_array(expected_uuids) - end - end - end - - context 'when scan_mode is partial' do - let(:params) { { scan_mode: 'partial' } } - let(:result_uuids) { sast_scan.findings.pluck(:uuid) } - - it 'returns only partial scan results' do - is_expected.to match_array(expected_uuids) - end - - context 'when feature is disabled' do - before do - stub_feature_flags(vulnerability_partial_scans: false) - end - - let(:result_uuids) { Security::Finding.pluck(:uuid) } - - it 'returns both full and partial scan results' do - is_expected.to match_array(expected_uuids) - end - end - end - end end end @@ -385,7 +330,7 @@ end context 'with the default parameters' do - let(:result_uuids) { Security::Finding.pluck(:uuid) } + let(:expected_uuids) { Security::Finding.pluck(:uuid) - [Security::Finding.second[:uuid]] } it 'includes child build findings' do expect(finding_uuids).to match_array(expected_uuids) diff --git a/ee/spec/graphql/types/security/scan_mode_enum_spec.rb b/ee/spec/graphql/types/security/scan_mode_enum_spec.rb deleted file mode 100644 index 9c90d549e2ad9fcb1d2e3c9e2191952266680d79..0000000000000000000000000000000000000000 --- a/ee/spec/graphql/types/security/scan_mode_enum_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe GitlabSchema.types['ScanModeEnum'], feature_category: :vulnerability_management do - specify { expect(described_class.graphql_name).to eq('ScanModeEnum') } - - describe 'statuses' do - using RSpec::Parameterized::TableSyntax - - where(:graphql_value, :param_value) do - 'ALL' | 'all' - 'FULL' | 'full' - 'PARTIAL' | 'partial' - end - - with_them do - it 'exposes a status with the correct value' do - expect(described_class.values[graphql_value].value).to eq(param_value) - end - end - end -end diff --git a/ee/spec/requests/api/graphql/merge_requests/finding_reports_comparer_spec.rb b/ee/spec/requests/api/graphql/merge_requests/finding_reports_comparer_spec.rb index 22c9493fd042cbee3bee7e74a1e939d9d0dd5730..f91d00152c9461a4b844a0c58926861c9d80dae6 100644 --- a/ee/spec/requests/api/graphql/merge_requests/finding_reports_comparer_spec.rb +++ b/ee/spec/requests/api/graphql/merge_requests/finding_reports_comparer_spec.rb @@ -96,7 +96,7 @@ let(:finding_reports_comparer_fields) do <<~QUERY - findingReportsComparer(reportType: SAST, scanMode: PARTIAL) { + findingReportsComparer(reportType: SAST) { status statusReason report { @@ -193,9 +193,7 @@ subject(:result) { graphql_data_at(:project, :merge_request, :finding_reports_comparer) } before do - allow(::Security::MergeRequestSecurityReportGenerationService).to receive(:execute) - .with(merge_request, { report_type: 'sast', scan_mode: 'partial' }) - .and_return(mock_report) + allow(::Security::MergeRequestSecurityReportGenerationService).to receive(:execute).and_return(mock_report) end context 'when the user is not authorized to read the field' do diff --git a/ee/spec/requests/projects/merge_requests_controller_spec.rb b/ee/spec/requests/projects/merge_requests_controller_spec.rb index ddd7540ace7e72af2ef20a9003b4383deda02866..6e57d8a2cb93c81451fd56ab62c73299006cfeea 100644 --- a/ee/spec/requests/projects/merge_requests_controller_spec.rb +++ b/ee/spec/requests/projects/merge_requests_controller_spec.rb @@ -138,11 +138,8 @@ def get_index describe 'security_reports' do let_it_be(:merge_request) { create(:merge_request, :with_head_pipeline) } let_it_be(:user) { create(:user) } - let(:params) { { type: :sast } } - subject(:request_report) do - get security_reports_project_merge_request_path(project, merge_request, params: params, format: :json) - end + subject(:request_report) { get security_reports_project_merge_request_path(project, merge_request, type: :sast, format: :json) } before do stub_licensed_features(security_dashboard: true) @@ -197,7 +194,7 @@ def get_index context 'when the given type is valid' do before do allow(::Security::MergeRequestSecurityReportGenerationService) - .to receive(:execute).with(an_instance_of(MergeRequest), hash_including(report_type: 'sast')).and_return(report_payload) + .to receive(:execute).with(an_instance_of(MergeRequest), 'sast').and_return(report_payload) end context 'when comparison is being processed' do diff --git a/ee/spec/services/ci/compare_security_reports_service_spec.rb b/ee/spec/services/ci/compare_security_reports_service_spec.rb index a4e0627e5d4c9483843e16a41bfc9206a104d399..fc309bb8e0ba6b522f19d060fdd1a37ce03b2234 100644 --- a/ee/spec/services/ci/compare_security_reports_service_spec.rb +++ b/ee/spec/services/ci/compare_security_reports_service_spec.rb @@ -21,15 +21,13 @@ with_sast_feature_branch: create(:ee_ci_pipeline, :with_sast_feature_branch, project: project), with_secret_detection_feature_branch: create(:ee_ci_pipeline, :with_secret_detection_feature_branch, project: project), with_corrupted_dependency_scanning_report: create(:ee_ci_pipeline, :with_corrupted_dependency_scanning_report, project: project), - with_corrupted_container_scanning_report: create(:ee_ci_pipeline, :with_corrupted_container_scanning_report, project: project), - sast_differential_scan: create(:ee_ci_pipeline, :sast_differential_scan, project: project) + with_corrupted_container_scanning_report: create(:ee_ci_pipeline, :with_corrupted_container_scanning_report, project: project) } end - let(:params) { { report_type: scan_type.to_s } } - let(:service) { described_class.new(project, current_user, params) } + let(:service) { described_class.new(project, current_user, report_type: scan_type.to_s) } - def create_scan_with_findings(scan_type, pipeline, count = 1, partial: false) + def create_scan_with_findings(scan_type, pipeline, count = 1) scan = create( :security_scan, :latest_successful, @@ -38,8 +36,6 @@ def create_scan_with_findings(scan_type, pipeline, count = 1, partial: false) scan_type: scan_type ) - create(:vulnerabilities_partial_scan, scan: scan) if partial - create_list( :security_finding, count, @@ -59,7 +55,6 @@ def create_scan_with_findings(scan_type, pipeline, count = 1, partial: false) create_scan_with_findings('container_scanning', test_pipelines[:with_container_scanning_feature_branch], 8) create_scan_with_findings('dast', test_pipelines[:with_dast_feature_branch], 20) create_scan_with_findings('sast', test_pipelines[:with_sast_feature_branch], 5) - create_scan_with_findings('sast', test_pipelines[:sast_differential_scan], 6, partial: true) create_scan_with_findings('secret_detection', test_pipelines[:with_secret_detection_feature_branch]) end @@ -308,26 +303,6 @@ def create_scan_with_findings(scan_type, pipeline, count = 1, partial: false) end it_behaves_like 'when a pipeline has scan that is not in the `succeeded` state' - - context 'when scan_mode is partial' do - let(:params) { { report_type: 'sast', scan_mode: 'partial' } } - - it_behaves_like 'when only the head pipeline has a report' do - let_it_be(:head_pipeline) { test_pipelines[:sast_differential_scan] } - let(:num_findings_in_fixture) { 6 } - end - - it_behaves_like 'when base and head pipelines have scanning reports' do - let_it_be(:base_pipeline) { test_pipelines[:with_sast_report] } - let_it_be(:head_pipeline) { test_pipelines[:sast_differential_scan] } - - # Since the sast report on the base pipeline is a full scan and not a partial, - # it should not be considered in the comparison. Partial scans should only run - # on feature branches. - let(:num_fixed_findings) { 0 } - let(:num_added_findings) { 6 } - end - end end context 'with secret detection' do diff --git a/ee/spec/services/security/merge_request_security_report_generation_service_spec.rb b/ee/spec/services/security/merge_request_security_report_generation_service_spec.rb index 6c51c90d312688601102f1b5304220133e9cf289..117b6bd73729ae8d22287373d0bf7b01bd6c4059 100644 --- a/ee/spec/services/security/merge_request_security_report_generation_service_spec.rb +++ b/ee/spec/services/security/merge_request_security_report_generation_service_spec.rb @@ -2,29 +2,13 @@ require 'spec_helper' -RSpec.describe Security::MergeRequestSecurityReportGenerationService, :use_clean_rails_memory_store_caching, :sidekiq_inline, feature_category: :vulnerability_management do - include ReactiveCachingHelpers - +RSpec.describe Security::MergeRequestSecurityReportGenerationService, feature_category: :vulnerability_management do describe '#execute' do let_it_be(:merge_request) { create(:merge_request) } - let(:params) { { report_type: } } - let(:report_generation_service) { described_class.new(merge_request, params) } - - subject(:report) { report_generation_service.execute } + let(:service_object) { described_class.new(merge_request, report_type) } - def stub_report(data) - allow_next_instance_of(Ci::CompareSecurityReportsService, merge_request.project, nil, params) do |instance| - expect(instance).to receive(:latest?) - .with( - merge_request.comparison_base_pipeline(Ci::CompareSecurityReportsService), - merge_request.diff_head_pipeline, - data - ) - .and_return(true) - end - stub_reactive_cache(report_generation_service, data, params.stringify_keys) - end + subject(:report) { service_object.execute } context 'when the given report type is invalid' do let(:report_type) { 'foo' } @@ -79,7 +63,7 @@ def stub_report(data) end before do - stub_report(mock_report) + allow(merge_request).to receive(:compare_sast_reports).with(nil).and_return(mock_report) end it 'returns the report with overridden severity' do @@ -107,6 +91,8 @@ def stub_report(data) end context 'when the given report type is valid' do + using RSpec::Parameterized::TableSyntax + let_it_be(:confirmed_finding) { create(:vulnerabilities_finding, :confirmed, severity: :critical) } let_it_be(:dismissed_finding) { create(:vulnerabilities_finding, :dismissed, severity: :medium) } @@ -114,16 +100,14 @@ def stub_report(data) let(:confirmed_uuid) { confirmed_finding.uuid } let(:dismissed_uuid) { dismissed_finding.uuid } - where(:report_type) do - %w[ - sast - secret_detection - container_scanning - dependency_scanning - dast - coverage_fuzzing - api_fuzzing - ] + where(:report_type, :mr_report_method) do + 'sast' | :compare_sast_reports + 'secret_detection' | :compare_secret_detection_reports + 'container_scanning' | :compare_container_scanning_reports + 'dependency_scanning' | :compare_dependency_scanning_reports + 'dast' | :compare_dast_reports + 'coverage_fuzzing' | :compare_coverage_fuzzing_reports + 'api_fuzzing' | :compare_api_fuzzing_reports end with_them do @@ -163,86 +147,62 @@ def stub_report(data) } end - context 'with no data cached' do - it 'queues reactive caching update and returns parsing' do - expect_reactive_cache_update_queued(report_generation_service, params.stringify_keys) - - expect(report).to eq({ status: :parsing }) - end + before do + allow(merge_request).to receive(mr_report_method).with(nil).and_return(mock_report) end - context 'with data cached' do - before do - stub_report(mock_report) - end + context 'when the report status is `parsing`' do + let(:report_status) { :parsing } - context 'when the report status is `parsing`' do - let(:report_status) { :parsing } - - it 'returns the report' do - expect(report).to eq(mock_report) - end + it 'returns the report' do + expect(report).to eq(mock_report) end + end - context 'when the report status is `parsed`' do - let(:report_status) { :parsed } - let(:expected_report) do - { - status: report_status, - data: { - 'base_report_created_at' => nil, - 'base_report_out_of_date' => false, - 'head_report_created_at' => '2023-01-18T11:30:01.035Z', - 'added' => [ - { - 'id' => nil, - 'name' => 'Test vulnerability 1', - 'uuid' => new_uuid, - 'severity' => 'critical', - 'state' => 'detected', - 'severity_override' => nil - }, - { - 'id' => nil, - 'name' => 'Test vulnerability 2', - 'uuid' => confirmed_uuid, - 'severity' => 'critical', - 'state' => 'confirmed', - 'severity_override' => nil - } - ], - 'fixed' => [ - { - 'id' => nil, - 'name' => 'Test vulnerability 3', - 'uuid' => dismissed_uuid, - 'severity' => 'medium', - 'state' => 'dismissed', - 'severity_override' => nil - } - ] - } + context 'when the report status is `parsed`' do + let(:report_status) { :parsed } + let(:expected_report) do + { + status: report_status, + data: { + 'base_report_created_at' => nil, + 'base_report_out_of_date' => false, + 'head_report_created_at' => '2023-01-18T11:30:01.035Z', + 'added' => [ + { + 'id' => nil, + 'name' => 'Test vulnerability 1', + 'uuid' => new_uuid, + 'severity' => 'critical', + 'state' => 'detected', + 'severity_override' => nil + }, + { + 'id' => nil, + 'name' => 'Test vulnerability 2', + 'uuid' => confirmed_uuid, + 'severity' => 'critical', + 'state' => 'confirmed', + 'severity_override' => nil + } + ], + 'fixed' => [ + { + 'id' => nil, + 'name' => 'Test vulnerability 3', + 'uuid' => dismissed_uuid, + 'severity' => 'medium', + 'state' => 'dismissed', + 'severity_override' => nil + } + ] } - end - - context 'when cached results is not latest' do - before do - allow(ReactiveCachingWorker).to receive(:perform_async).and_call_original - allow_next_instance_of(Ci::CompareSecurityReportsService) do |instance| - allow(instance).to receive(:latest?).and_return(false) - end - end - - it 'queues reactive cache update' do - expect_reactive_cache_update_queued(report_generation_service, params.stringify_keys) - - expect(report).to eq({ status: :parsing }) - end - end - - it 'returns all the fields along with the calculated state of the findings' do - expect(report).to eq(expected_report) - end + } + end + + it 'returns all the fields along with the calculated state of the findings' do + expect(report).to eq(expected_report) + expect(merge_request).to have_received(mr_report_method).with(nil) end end end diff --git a/spec/support/helpers/reactive_caching_helpers.rb b/spec/support/helpers/reactive_caching_helpers.rb index 604abbe2edfd8efa57839ce37dd8ceba4b80e4dc..00192a5d58722c883304af7c830952f33f511d19 100644 --- a/spec/support/helpers/reactive_caching_helpers.rb +++ b/spec/support/helpers/reactive_caching_helpers.rb @@ -45,9 +45,9 @@ def start_reactive_cache_lifetime(subject, *qualifiers) Rails.cache.write(alive_reactive_cache_key(subject, *qualifiers), true) end - def expect_reactive_cache_update_queued(subject, *args, worker_klass: ReactiveCachingWorker) + def expect_reactive_cache_update_queued(subject, worker_klass: ReactiveCachingWorker) expect(worker_klass) .to receive(:perform_in) - .with(subject.class.reactive_cache_refresh_interval, subject.class.name, subject.id, *args) + .with(subject.class.reactive_cache_refresh_interval, subject.class.name, subject.id) end end