diff --git a/ee/app/controllers/ee/projects/merge_requests_controller.rb b/ee/app/controllers/ee/projects/merge_requests_controller.rb index f78c0e8caa4821f3e2c040c89107e207f2a87b74..58fb98f9515f3d28a78f21e2b371ee4f14f61d5c 100644 --- a/ee/app/controllers/ee/projects/merge_requests_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests_controller.rb @@ -42,6 +42,10 @@ def license_management_reports reports_response(merge_request.compare_license_management_reports) end + def container_scanning_reports + reports_response(merge_request.compare_container_scanning_reports) + end + def metrics_reports reports_response(merge_request.compare_metrics_reports) end diff --git a/ee/app/models/ee/ci/job_artifact.rb b/ee/app/models/ee/ci/job_artifact.rb index 105bd848b200915d673f210401139692e74e4747..f61cf856609194b131c1c025501699d19e6cc4eb 100644 --- a/ee/app/models/ee/ci/job_artifact.rb +++ b/ee/app/models/ee/ci/job_artifact.rb @@ -15,6 +15,7 @@ module Ci::JobArtifact LICENSE_MANAGEMENT_REPORT_FILE_TYPES = %w[license_management].freeze DEPENDENCY_LIST_REPORT_FILE_TYPES = %w[dependency_scanning].freeze METRICS_REPORT_FILE_TYPES = %w[metrics].freeze + CONTAINER_SCANNING_REPORT_TYPES = %w[container_scanning].freeze scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) } scope :project_id_in, ->(ids) { joins(:project).merge(::Project.id_in(ids)) } @@ -33,6 +34,10 @@ module Ci::JobArtifact with_file_types(DEPENDENCY_LIST_REPORT_FILE_TYPES) end + scope :container_scanning_reports, -> do + with_file_types(CONTAINER_SCANNING_REPORT_TYPES) + end + scope :metrics_reports, -> do with_file_types(METRICS_REPORT_FILE_TYPES) end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 3bc249fb76ef1f45b0094992fe1175148148cf37..6814fabc72f61c1c7bb3a6194ea173454161b5cc 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -116,6 +116,18 @@ def has_license_management_reports? actual_head_pipeline&.has_reports?(::Ci::JobArtifact.license_management_reports) end + def has_container_scanning_reports? + actual_head_pipeline&.has_reports?(::Ci::JobArtifact.container_scanning_reports) + end + + def compare_container_scanning_reports + unless has_container_scanning_reports? + return { status: :error, status_reason: 'This merge request does not have container scanning reports' } + end + + compare_reports(::Ci::CompareContainerScanningReportsService) + end + def compare_license_management_reports unless has_license_management_reports? return { status: :error, status_reason: 'This merge request does not have license management reports' } diff --git a/ee/app/serializers/vulnerabilities/occurence_reports_comparer_entity.rb b/ee/app/serializers/vulnerabilities/occurence_reports_comparer_entity.rb new file mode 100644 index 0000000000000000000000000000000000000000..a3ae443afa080cd47a2652bf08db963890992e25 --- /dev/null +++ b/ee/app/serializers/vulnerabilities/occurence_reports_comparer_entity.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class Vulnerabilities::OccurrenceReportsComparerEntity < Grape::Entity + expose :added, using: Vulnerabilities::OccurrenceReportEntity + expose :fixed, using: Vulnerabilities::OccurrenceReportEntity + expose :existing, using: Vulnerabilities::OccurrenceReportEntity +end diff --git a/ee/app/serializers/vulnerabilities/occurrence_diff_serializer.rb b/ee/app/serializers/vulnerabilities/occurrence_diff_serializer.rb new file mode 100644 index 0000000000000000000000000000000000000000..527f9589eb74ed08fb13be4ec5d6ba7c9a71c9c6 --- /dev/null +++ b/ee/app/serializers/vulnerabilities/occurrence_diff_serializer.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class Vulnerabilities::OccurrenceDiffSerializer < BaseSerializer + include WithPagination + + entity Vulnerabilities::OccurrenceReportsComparerEntity +end diff --git a/ee/app/serializers/vulnerabilities/occurrence_report_entity.rb b/ee/app/serializers/vulnerabilities/occurrence_report_entity.rb new file mode 100644 index 0000000000000000000000000000000000000000..3cd62ed833ed7bb82686cc2886a8646907c31b76 --- /dev/null +++ b/ee/app/serializers/vulnerabilities/occurrence_report_entity.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class Vulnerabilities::OccurrenceReportEntity < Grape::Entity + expose :report_type, :name, :severity, :confidence, :compare_key, :identifiers, :scanner, :project_fingerprint, :uuid, :metadata_version, :location +end diff --git a/ee/app/services/ci/compare_container_scanning_reports_service.rb b/ee/app/services/ci/compare_container_scanning_reports_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..e0b0abe11b2015a469dab99eff628d651c20b067 --- /dev/null +++ b/ee/app/services/ci/compare_container_scanning_reports_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Ci + class CompareContainerScanningReportsService < ::Ci::CompareReportsBaseService + def comparer_class + Gitlab::Ci::Reports::Security::ContainerScanningReportsComparer + end + + def serializer_class + Vulnerabilities::OccurrenceDiffSerializer + end + + def get_report(pipeline) + report = pipeline&.security_reports&.get_report('container_scanning') + + raise report.error if report&.errored? # propagate error to base class's execute method + + report + end + end +end diff --git a/ee/app/views/projects/merge_requests/show.html.haml b/ee/app/views/projects/merge_requests/show.html.haml index a061cc5c081f37e342acc6e9334adf2b62817b05..9eea23476cb5c777d9978aa56dbc468f5e20e278 100644 --- a/ee/app/views/projects/merge_requests/show.html.haml +++ b/ee/app/views/projects/merge_requests/show.html.haml @@ -17,3 +17,4 @@ window.gl.mrWidgetData.approvals_help_path = '#{help_page_path("user/project/merge_requests/merge_request_approvals")}'; window.gl.mrWidgetData.visual_review_app_available = '#{@project.feature_available?(:visual_review_app)}' === 'true'; window.gl.mrWidgetData.license_management_comparsion_path = '#{license_management_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:license_management)}' + window.gl.mrWidgetData.container_scanning_comparsion_path = '#{container_scanning_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:container_scanning)}' diff --git a/ee/changelogs/unreleased/12004-move-cs-reports-to-mr-widget-backend.yml b/ee/changelogs/unreleased/12004-move-cs-reports-to-mr-widget-backend.yml new file mode 100644 index 0000000000000000000000000000000000000000..957046e0c9148f2e2309d58aff1af676fe9dec4a --- /dev/null +++ b/ee/changelogs/unreleased/12004-move-cs-reports-to-mr-widget-backend.yml @@ -0,0 +1,5 @@ +--- +title: Present container scanning report comparison via API +merge_request: 14898 +author: +type: changed diff --git a/ee/config/routes/project.rb b/ee/config/routes/project.rb index 8c8e910bfd36eb319da3044171a5e653580192fe..01ce3f3c8eb0fb6420b61ea38815bf540820a10e 100644 --- a/ee/config/routes/project.rb +++ b/ee/config/routes/project.rb @@ -69,6 +69,7 @@ member do get :metrics_reports get :license_management_reports + get :container_scanning_reports end end diff --git a/ee/lib/gitlab/ci/reports/security/container_scanning_reports_comparer.rb b/ee/lib/gitlab/ci/reports/security/container_scanning_reports_comparer.rb new file mode 100644 index 0000000000000000000000000000000000000000..28469530d85f5e93f9b1043cc22940c0a47548ae --- /dev/null +++ b/ee/lib/gitlab/ci/reports/security/container_scanning_reports_comparer.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Reports + module Security + class ContainerScanningReportsComparer + include Gitlab::Utils::StrongMemoize + + attr_reader :base_report, :head_report + + def initialize(base_report, head_report) + @base_report = base_report || ::Gitlab::Ci::Reports::Security::Report.new('container_scanning', '') + @head_report = head_report + end + + def added + strong_memoize(:added) do + head_report.occurrences - base_report.occurrences + end + end + + def fixed + strong_memoize(:fixed) do + base_report.occurrences - head_report.occurrences + end + end + + def existing + strong_memoize(:existing) do + base_report.occurrences & head_report.occurrences + end + end + end + end + end + end +end diff --git a/ee/lib/gitlab/ci/reports/security/occurrence.rb b/ee/lib/gitlab/ci/reports/security/occurrence.rb index db20953cbad6f2d36c0a9765084723c1a282ead4..7850ae321c9b58b3b8d50d6e465a01ab3853eb8d 100644 --- a/ee/lib/gitlab/ci/reports/security/occurrence.rb +++ b/ee/lib/gitlab/ci/reports/security/occurrence.rb @@ -70,6 +70,12 @@ def ==(other) other.location == location && other.primary_identifier == primary_identifier end + + # Array.difference (-) method uses hash and eq? methods to do comparison + def hash + compare_key.hash + end + alias_method :eql?, :== # eql? is necessary in some cases like array intersection private diff --git a/ee/spec/controllers/projects/merge_requests_controller_spec.rb b/ee/spec/controllers/projects/merge_requests_controller_spec.rb index 6ac7724fd5ac338618fed24ac28852c37fa5de3b..cdbdb540dfcc1fd51a7b22eb5199ffdbdc48fcd0 100644 --- a/ee/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/ee/spec/controllers/projects/merge_requests_controller_spec.rb @@ -393,6 +393,91 @@ def expect_rebase_worker_for(user) end end + describe 'GET #container_scanning_reports' do + let(:merge_request) { create(:ee_merge_request, :with_container_scanning_reports, source_project: project, author: create(:user)) } + let(:params) do + { + namespace_id: project.namespace.to_param, + project_id: project, + id: merge_request.iid + } + end + + subject { get :container_scanning_reports, params: params, format: :json } + + before do + allow_any_instance_of(::MergeRequest).to receive(:compare_reports) + .with(::Ci::CompareContainerScanningReportsService).and_return(comparison_status) + end + + context 'when comparison is being processed' do + let(:comparison_status) { { status: :parsing } } + + it 'sends polling interval' do + expect(::Gitlab::PollingInterval).to receive(:set_header) + + subject + end + + it 'returns 204 HTTP status' do + subject + + expect(response).to have_gitlab_http_status(:no_content) + end + end + + context 'when comparison is done' do + let(:comparison_status) { { status: :parsed, data: { added: [], fixed: [], existing: [] } } } + + it 'does not send polling interval' do + expect(::Gitlab::PollingInterval).not_to receive(:set_header) + + subject + end + + it 'returns 200 HTTP status' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq({ "added" => [], "fixed" => [], "existing" => [] }) + end + end + + context 'when user created corrupted vulnerability reports' do + let(:comparison_status) { { status: :error, status_reason: 'Failed to parse container scanning reports' } } + + it 'does not send polling interval' do + expect(::Gitlab::PollingInterval).not_to receive(:set_header) + + subject + end + + it 'returns 400 HTTP status' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response).to eq({ 'status_reason' => 'Failed to parse container scanning reports' }) + end + end + + context 'when something went wrong on our system' do + let(:comparison_status) { {} } + + it 'does not send polling interval' do + expect(::Gitlab::PollingInterval).not_to receive(:set_header) + + subject + end + + it 'returns 500 HTTP status' do + subject + + expect(response).to have_gitlab_http_status(:internal_server_error) + expect(json_response).to eq({ 'status_reason' => 'Unknown error' }) + end + end + end + describe 'GET #license_management_reports' do let(:merge_request) { create(:ee_merge_request, :with_license_management_reports, source_project: project, author: create(:user)) } let(:params) do diff --git a/ee/spec/factories/ci/builds.rb b/ee/spec/factories/ci/builds.rb index b3ca25cf3821e591c2a9884dfe32108b01970ef8..144b3829e048815fbacafb53ab86ae88e682bd25 100644 --- a/ee/spec/factories/ci/builds.rb +++ b/ee/spec/factories/ci/builds.rb @@ -48,6 +48,18 @@ end end + trait :container_scanning_feature_branch do + after(:build) do |build| + build.job_artifacts << create(:ee_ci_job_artifact, :container_scanning_feature_branch, job: build) + end + end + + trait :corrupted_container_scanning_report do + after(:build) do |build| + build.job_artifacts << create(:ee_ci_job_artifact, :corrupted_container_scanning_report, job: build) + end + end + trait :license_management_feature_branch do after(:build) do |build| build.job_artifacts << create(:ee_ci_job_artifact, :license_management_feature_branch, job: build) diff --git a/ee/spec/factories/ci/job_artifacts.rb b/ee/spec/factories/ci/job_artifacts.rb index 91708762d0dc0546d4c6477efdbb43f3be086f12..714d13c1b2ad4161cf453fa8e0463c091ee255ae 100644 --- a/ee/spec/factories/ci/job_artifacts.rb +++ b/ee/spec/factories/ci/job_artifacts.rb @@ -122,6 +122,26 @@ end end + trait :container_scanning_feature_branch do + file_format :raw + file_type :container_scanning + + after(:build) do |artifact, _| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/security-reports/feature-branch/gl-container-scanning-report.json'), 'application/json') + end + end + + trait :corrupted_container_scanning_report do + file_format :raw + file_type :container_scanning + + after(:build) do |artifact, evaluator| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/trace/sample_trace'), 'application/json') + end + end + trait :dast do file_format :raw file_type :dast diff --git a/ee/spec/factories/ci/pipelines.rb b/ee/spec/factories/ci/pipelines.rb index 0ba8fd3bd73ce9ee12ac7858413d3f545272441e..f4515c4d8372183f2880d8fb8f1ed7f85bfa3a8a 100644 --- a/ee/spec/factories/ci/pipelines.rb +++ b/ee/spec/factories/ci/pipelines.rb @@ -7,7 +7,7 @@ config_source :webide_source end - %i[license_management dependency_list dependency_scanning sast].each do |report_type| + %i[license_management dependency_list dependency_scanning sast container_scanning].each do |report_type| trait "with_#{report_type}_report".to_sym do status :success @@ -17,6 +17,22 @@ end end + trait :with_container_scanning_feature_branch do + status :success + + after(:build) do |pipeline, evaluator| + pipeline.builds << build(:ee_ci_build, :container_scanning_feature_branch, pipeline: pipeline, project: pipeline.project) + end + end + + trait :with_corrupted_container_scanning_report do + status :success + + after(:build) do |pipeline, evaluator| + pipeline.builds << build(:ee_ci_build, :corrupted_container_scanning_report, pipeline: pipeline, project: pipeline.project) + end + end + trait :with_license_management_feature_branch do status :success diff --git a/ee/spec/factories/merge_requests.rb b/ee/spec/factories/merge_requests.rb index 3423db515170df15208c67c68814059811136fd6..58758277fea466cdcddc9da77e458b8a5af54ea4 100644 --- a/ee/spec/factories/merge_requests.rb +++ b/ee/spec/factories/merge_requests.rb @@ -61,6 +61,18 @@ end end + trait :with_container_scanning_reports do + after(:build) do |merge_request| + merge_request.head_pipeline = build( + :ee_ci_pipeline, + :success, + :with_container_scanning_report, + project: merge_request.source_project, + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha) + end + end + trait :with_metrics_reports do after(:build) do |merge_request| merge_request.head_pipeline = build( diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index fe42b5a5ec44250f318f486c92786a848a96d3f4..f897976a2ca0640432ff727e0e62e4ef8cbbf514 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -139,6 +139,27 @@ end end + describe '#has_container_scanning_reports?' do + subject { merge_request.has_container_scanning_reports? } + let(:project) { create(:project, :repository) } + + before do + stub_licensed_features(container_scanning: true) + end + + context 'when head pipeline has container scannning reports' do + let(:merge_request) { create(:ee_merge_request, :with_container_scanning_reports, source_project: project) } + + it { is_expected.to be_truthy } + end + + context 'when head pipeline does not have container scanning reports' do + let(:merge_request) { create(:ee_merge_request, source_project: project) } + + it { is_expected.to be_falsey } + end + end + describe '#has_metrics_reports?' do subject { merge_request.has_metrics_reports? } let(:project) { create(:project, :repository) } @@ -160,6 +181,65 @@ end end + describe '#compare_container_scanning_reports' do + subject { merge_request.compare_container_scanning_reports } + + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project) } + + let!(:base_pipeline) do + create(:ee_ci_pipeline, + :with_container_scanning_report, + project: project, + ref: merge_request.target_branch, + sha: merge_request.diff_base_sha) + end + + before do + merge_request.update!(head_pipeline_id: head_pipeline.id) + end + + context 'when head pipeline has container scanning reports' do + let!(:head_pipeline) do + create(:ee_ci_pipeline, + :with_container_scanning_report, + project: project, + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha) + end + + context 'when reactive cache worker is parsing asynchronously' do + it 'returns status' do + expect(subject[:status]).to eq(:parsing) + end + end + + context 'when reactive cache worker is inline' do + before do + synchronous_reactive_cache(merge_request) + end + + it 'returns status and data' do + expect_any_instance_of(Ci::CompareContainerScanningReportsService) + .to receive(:execute).with(base_pipeline, head_pipeline).and_call_original + + subject + end + + context 'when cached results is not latest' do + before do + allow_any_instance_of(Ci::CompareContainerScanningReportsService) + .to receive(:latest?).and_return(false) + end + + it 'raises and InvalidateReactiveCache error' do + expect { subject }.to raise_error(ReactiveCaching::InvalidateReactiveCache) + end + end + end + end + end + describe '#compare_license_management_reports' do subject { merge_request.compare_license_management_reports } diff --git a/ee/spec/serializers/vulnerabilities/occurrence_reports_comparer_entity_spec.rb b/ee/spec/serializers/vulnerabilities/occurrence_reports_comparer_entity_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..12a9bd13a1edf119e101b1c02007cfa052050a20 --- /dev/null +++ b/ee/spec/serializers/vulnerabilities/occurrence_reports_comparer_entity_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Vulnerabilities::OccurrenceReportsComparerEntity do + let!(:base_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report) } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_feature_branch) } + let(:base_report) { base_pipeline.security_reports.get_report('container_scanning')} + let(:head_report) { head_pipeline.security_reports.get_report('container_scanning')} + let(:comparer) { Gitlab::Ci::Reports::Security::ContainerScanningReportsComparer.new(base_report, head_report) } + let(:entity) { described_class.new(comparer) } + + before do + stub_licensed_features(container_scanning: true) + end + + describe '#as_json' do + subject { entity.as_json } + + it 'contains the added existing and fixed vulnerabilities for container scanning' do + expect(subject.keys).to match_array([:added, :existing, :fixed]) + end + end +end diff --git a/ee/spec/services/ci/compare_container_scanning_reports_service_spec.rb b/ee/spec/services/ci/compare_container_scanning_reports_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..f2ffaaa62addc7fd7e0552518aa4e9e015a78dc6 --- /dev/null +++ b/ee/spec/services/ci/compare_container_scanning_reports_service_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' + +describe Ci::CompareContainerScanningReportsService do + let(:service) { described_class.new(project) } + let(:project) { create(:project, :repository) } + + before do + stub_licensed_features(container_scanning: true) + end + + describe '#execute' do + subject { service.execute(base_pipeline, head_pipeline) } + + context 'when head pipeline has container scanning reports' do + let!(:base_pipeline) { create(:ee_ci_pipeline) } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report, project: project) } + + it 'reports new licenses' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]['added'].count).to eq(8) + expect(subject[:data]['existing'].count).to eq(0) + expect(subject[:data]['fixed'].count).to eq(0) + end + end + + context 'when base and head pipelines have container scanning reports' do + let!(:base_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_report, project: project) } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_container_scanning_feature_branch, project: project) } + + it 'reports status as parsed' do + expect(subject[:status]).to eq(:parsed) + end + + it 'reports new vulnerability' do + expect(subject[:data]['added'].count).to eq(1) + expect(subject[:data]['added']).to include(a_hash_including('compare_key' => 'CVE-2017-15650')) + end + + it 'reports existing container vulenerabilities' do + expect(subject[:data]['existing'].count).to eq(0) + end + + it 'reports fixed container scanning vulnerabilities' do + expect(subject[:data]['fixed'].count).to eq(8) + compare_keys = subject[:data]['fixed'].map { |t| t['compare_key'] } + expected_keys = %w(CVE-2017-16997 CVE-2017-18269 CVE-2018-1000001 CVE-2016-10228 CVE-2010-4052 CVE-2018-18520 CVE-2018-16869 CVE-2018-18311) + expect(compare_keys - expected_keys).to eq([]) + end + end + + context 'when head pipeline has corrupted container scanning vulnerability reports' do + let!(:base_pipeline) { nil } + let!(:head_pipeline) { create(:ee_ci_pipeline, :with_corrupted_container_scanning_report, project: project) } + + it 'returns status and error message' do + expect(subject[:status]).to eq(:error) + expect(subject[:status_reason]).to include('JSON parsing failed') + end + end + end +end