Merge branch '14793-missing-security-reports-be' into 'master'

Abort rendering of security reports that aren't set up

See merge request gitlab-org/gitlab!20381

ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue

@@ -275,6 +275,7 @@ export default {
         :head-blob-path="mr.headBlobPath"
         :source-branch="mr.sourceBranch"
         :base-blob-path="mr.baseBlobPath"
+        :enabled-reports="mr.enabledSecurityReports"
         :sast-head-path="mr.sast.head_path"
         :sast-base-path="mr.sast.base_path"
         :sast-help-path="mr.sastHelp"

ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

 import CEMergeRequestStore from '~/vue_merge_request_widget/stores/mr_widget_store';
+import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
 import { mapApprovalsResponse, mapApprovalRulesResponse } from '../mappers';
 import CodeQualityComparisonWorker from '../workers/code_quality_comparison_worker';
 
@@ -38,6 +39,8 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     this.licenseManagement = data.license_management;
     this.metricsReportsPath = data.metrics_reports_path;
 
+    this.enabledSecurityReports = convertObjectPropsToCamelCase(data.enabled_reports);
+
     this.blockingMergeRequests = data.blocking_merge_requests;
 
     this.hasApprovalsAvailable = data.has_approvals_available;

ee/app/assets/javascripts/vue_shared/security_reports/grouped_security_reports_app.vue

@@ -20,6 +20,11 @@ export default {
   },
   mixins: [securityReportsMixin],
   props: {
+    enabledReports: {
+      type: Object,
+      required: false,
+      default: () => ({}),
+    },
     headBlobPath: {
       type: String,
       required: true,
@@ -168,25 +173,22 @@ export default {
     securityTab() {
       return `${this.pipelinePath}/security`;
     },
-    shouldRenderSastContainer() {
+    hasContainerScanningReports() {
+      const type = 'containerScanning';
+      if (this.isMergeRequestReportApiEnabled(type)) {
+        return this.enabledReports[type];
+      }
       const { head, diffEndpoint } = this.sastContainer.paths;
-
-      return head || diffEndpoint;
+      return Boolean(head || diffEndpoint);
     },
-    shouldRenderDependencyScanning() {
-      const { head, diffEndpoint } = this.dependencyScanning.paths;
-
-      return head || diffEndpoint;
+    hasDependencyScanningReports() {
+      return this.hasReportsType('dependencyScanning');
     },
-    shouldRenderDast() {
-      const { head, diffEndpoint } = this.dast.paths;
-
-      return head || diffEndpoint;
+    hasDastReports() {
+      return this.hasReportsType('dast');
     },
-    shouldRenderSast() {
-      const { head, diffEndpoint } = this.sast.paths;
-
-      return head || diffEndpoint;
+    hasSastReports() {
+      return this.hasReportsType('sast');
     },
   },
 
@@ -209,7 +211,7 @@ export default {
 
     const sastDiffEndpoint = gl && gl.mrWidgetData && gl.mrWidgetData.sast_comparison_path;
 
-    if (gon.features && gon.features.sastMergeRequestReportApi && sastDiffEndpoint) {
+    if (this.isMergeRequestReportApiEnabled('sast') && sastDiffEndpoint && this.hasSastReports) {
       this.setSastDiffEndpoint(sastDiffEndpoint);
       this.fetchSastDiff();
     } else if (this.sastHeadPath) {
@@ -225,9 +227,9 @@ export default {
       gl && gl.mrWidgetData && gl.mrWidgetData.container_scanning_comparison_path;
 
     if (
-      gon.features &&
-      gon.features.containerScanningMergeRequestReportApi &&
-      sastContainerDiffEndpoint
+      this.isMergeRequestReportApiEnabled('containerScanning') &&
+      sastContainerDiffEndpoint &&
+      this.hasContainerScanningReports
     ) {
       this.setSastContainerDiffEndpoint(sastContainerDiffEndpoint);
       this.fetchSastContainerDiff();
@@ -242,7 +244,7 @@ export default {
 
     const dastDiffEndpoint = gl && gl.mrWidgetData && gl.mrWidgetData.dast_comparison_path;
 
-    if (gon.features && gon.features.dastMergeRequestReportApi && dastDiffEndpoint) {
+    if (this.isMergeRequestReportApiEnabled('dast') && dastDiffEndpoint && this.hasDastReports) {
       this.setDastDiffEndpoint(dastDiffEndpoint);
       this.fetchDastDiff();
     } else if (this.dastHeadPath) {
@@ -258,9 +260,9 @@ export default {
       gl && gl.mrWidgetData && gl.mrWidgetData.dependency_scanning_comparison_path;
 
     if (
-      gon.features &&
-      gon.features.dependencyScanningMergeRequestReportApi &&
-      dependencyScanningDiffEndpoint
+      this.isMergeRequestReportApiEnabled('dependencyScanning') &&
+      dependencyScanningDiffEndpoint &&
+      this.hasDependencyScanningReports
     ) {
       this.setDependencyScanningDiffEndpoint(dependencyScanningDiffEndpoint);
       this.fetchDependencyScanningDiff();
@@ -321,6 +323,16 @@ export default {
       fetchSastReports: 'fetchReports',
       fetchSastDiff: 'fetchDiff',
     }),
+    isMergeRequestReportApiEnabled(type) {
+      return Boolean(gon.features && gon.features[`${type}MergeRequestReportApi`]);
+    },
+    hasReportsType(type) {
+      if (this.isMergeRequestReportApiEnabled(type)) {
+        return this.enabledReports[type];
+      }
+      const { head, diffEndpoint } = this[type].paths;
+      return Boolean(head || diffEndpoint);
+    },
   },
 };
 </script>
@@ -340,12 +352,13 @@ export default {
         target="_blank"
         class="btn btn-default btn-sm float-right append-right-default"
       >
-        <span>{{ s__('ciReport|View full report') }}</span> <icon :size="16" name="external-link" />
+        <span>{{ s__('ciReport|View full report') }}</span>
+        <icon :size="16" name="external-link" />
       </a>
     </div>
 
     <div slot="body" class="mr-widget-grouped-section report-block">
-      <template v-if="shouldRenderSast">
+      <template v-if="hasSastReports">
         <summary-row
           :summary="groupedSastText"
           :status-icon="sastStatusIcon"
@@ -364,7 +377,7 @@ export default {
         />
       </template>
 
-      <template v-if="shouldRenderDependencyScanning">
+      <template v-if="hasDependencyScanningReports">
         <summary-row
           :summary="groupedDependencyText"
           :status-icon="dependencyScanningStatusIcon"
@@ -382,7 +395,7 @@ export default {
         />
       </template>
 
-      <template v-if="shouldRenderSastContainer">
+      <template v-if="hasContainerScanningReports">
         <summary-row
           :summary="groupedSastContainerText"
           :status-icon="sastContainerStatusIcon"
@@ -400,7 +413,7 @@ export default {
         />
       </template>
 
-      <template v-if="shouldRenderDast">
+      <template v-if="hasDastReports">
         <summary-row
           :summary="groupedDastText"
           :status-icon="dastStatusIcon"

ee/app/models/ee/merge_request.rb

@@ -134,6 +134,16 @@ module EE
       end
     end
 
+    def enabled_reports
+      {
+        sast: report_type_enabled?(:sast),
+        container_scanning: report_type_enabled?(:container_scanning),
+        dast: report_type_enabled?(:dast),
+        dependency_scanning: report_type_enabled?(:dependency_scanning),
+        license_management: report_type_enabled?(:license_management)
+      }
+    end
+
     def has_dependency_scanning_reports?
       !!(actual_head_pipeline&.has_reports?(::Ci::JobArtifact.dependency_list_reports))
     end
@@ -208,5 +218,9 @@ module EE
     def missing_report_error(report_type)
       { status: :error, status_reason: "This merge request does not have #{report_type} reports" }
     end
+
+    def report_type_enabled?(report_type)
+      !!actual_head_pipeline&.batch_lookup_report_artifact_for_file_type(report_type)
+    end
   end
 end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -36,6 +36,10 @@ module EE
         end
       end
 
+      expose :enabled_reports do |merge_request|
+        merge_request.enabled_reports
+      end
+
       expose :sast, if: -> (mr, _) { head_pipeline_downloadable_path_for_report_type(:sast) } do
         expose :head_path do |merge_request|
           head_pipeline_downloadable_path_for_report_type(:sast)

ee/changelogs/unreleased/14793-missing-security-reports-be.yml

+---
+title: Abort rendering of security reports that aren't enabled
+merge_request: 20381
+author:
+type: fixed

ee/spec/javascripts/vue_mr_widget/mock_data.js

@@ -10,6 +10,13 @@ export default Object.assign({}, mockData, {
     head_path: 'blob_path',
   },
   vulnerability_feedback_help_path: '/help/user/application_security/index',
+  enabled_reports: {
+    sast: true,
+    container_scanning: false,
+    dast: true,
+    dependency_scanning: false,
+    license_management: true,
+  },
 });
 
 // Codeclimate

ee/spec/javascripts/vue_shared/security_reports/grouped_security_reports_app_spec.js

@@ -275,8 +275,22 @@ describe('Grouped security reports app', () => {
   describe('with the reports API enabled', () => {
     describe('container scanning reports', () => {
       const sastContainerEndpoint = 'sast_container.json';
+      const props = {
+        headBlobPath: 'path',
+        baseBlobPath: 'path',
+        sastHelpPath: 'path',
+        sastContainerHelpPath: 'path',
+        dastHelpPath: 'path',
+        dependencyScanningHelpPath: 'path',
+        vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
+        vulnerabilityFeedbackHelpPath: 'path',
+        pipelineId: 123,
+        canCreateIssue: true,
+        canCreateMergeRequest: true,
+        canDismissVulnerability: true,
+      };
 
-      beforeEach(done => {
+      beforeEach(() => {
         gon.features = gon.features || {};
         gon.features.containerScanningMergeRequestReportApi = true;
 
@@ -289,42 +303,67 @@ describe('Grouped security reports app', () => {
         });
 
         mock.onGet('vulnerability_feedback_path.json').reply(200, []);
+      });
 
-        vm = mountComponent(Component, {
-          headBlobPath: 'path',
-          baseBlobPath: 'path',
-          sastHelpPath: 'path',
-          sastContainerHelpPath: 'path',
-          dastHelpPath: 'path',
-          dependencyScanningHelpPath: 'path',
-          vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
-          vulnerabilityFeedbackHelpPath: 'path',
-          pipelineId: 123,
-          canCreateIssue: true,
-          canCreateMergeRequest: true,
-          canDismissVulnerability: true,
+      describe('with reports disabled', () => {
+        beforeEach(() => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              containerScanning: false,
+            },
+          });
         });
 
-        waitForMutation(vm.$store, types.RECEIVE_SAST_CONTAINER_DIFF_SUCCESS)
-          .then(done)
-          .catch(done.fail);
+        it('should not render the widget', () => {
+          expect(vm.$el.querySelector('.js-sast-container')).toBeNull();
+        });
       });
 
-      it('should set setSastContainerDiffEndpoint', () => {
-        expect(vm.sastContainer.paths.diffEndpoint).toEqual(sastContainerEndpoint);
-      });
+      describe('with reports enabled', () => {
+        beforeEach(done => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              containerScanning: true,
+            },
+          });
+
+          waitForMutation(vm.$store, types.RECEIVE_SAST_CONTAINER_DIFF_SUCCESS)
+            .then(done)
+            .catch(done.fail);
+        });
 
-      it('should display the correct numbers of vulnerabilities', () => {
-        expect(vm.$el.textContent).toContain(
-          'Container scanning detected 2 new, and 1 fixed vulnerabilities',
-        );
+        it('should set setSastContainerDiffEndpoint', () => {
+          expect(vm.sastContainer.paths.diffEndpoint).toEqual(sastContainerEndpoint);
+        });
+
+        it('should display the correct numbers of vulnerabilities', () => {
+          expect(vm.$el.textContent).toContain(
+            'Container scanning detected 2 new, and 1 fixed vulnerabilities',
+          );
+        });
       });
     });
 
     describe('dependency scanning reports', () => {
       const dependencyScanningEndpoint = 'dependency_scanning.json';
+      const props = {
+        headBlobPath: 'path',
+        baseBlobPath: 'path',
+        sastHelpPath: 'path',
+        sastContainerHelpPath: 'path',
+        dastHelpPath: 'path',
+        dependencyScanningHelpPath: 'path',
+        vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
+        vulnerabilityFeedbackHelpPath: 'path',
+        pipelineId: 123,
+        canCreateIssue: true,
+        canCreateMergeRequest: true,
+        canDismissVulnerability: true,
+      };
 
-      beforeEach(done => {
+      beforeEach(() => {
         gon.features = gon.features || {};
         gon.features.dependencyScanningMergeRequestReportApi = true;
 
@@ -337,42 +376,67 @@ describe('Grouped security reports app', () => {
         });
 
         mock.onGet('vulnerability_feedback_path.json').reply(200, []);
+      });
 
-        vm = mountComponent(Component, {
-          headBlobPath: 'path',
-          baseBlobPath: 'path',
-          sastHelpPath: 'path',
-          sastContainerHelpPath: 'path',
-          dastHelpPath: 'path',
-          dependencyScanningHelpPath: 'path',
-          vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
-          vulnerabilityFeedbackHelpPath: 'path',
-          pipelineId: 123,
-          canCreateIssue: true,
-          canCreateMergeRequest: true,
-          canDismissVulnerability: true,
+      describe('with reports disabled', () => {
+        beforeEach(() => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              dependencyScanning: false,
+            },
+          });
         });
 
-        waitForMutation(vm.$store, types.RECEIVE_DEPENDENCY_SCANNING_DIFF_SUCCESS)
-          .then(done)
-          .catch(done.fail);
+        it('should not render the widget', () => {
+          expect(vm.$el.querySelector('.js-dependency-scanning-widget')).toBeNull();
+        });
       });
 
-      it('should set setDependencyScanningDiffEndpoint', () => {
-        expect(vm.dependencyScanning.paths.diffEndpoint).toEqual(dependencyScanningEndpoint);
-      });
+      describe('with reports enabled', () => {
+        beforeEach(done => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              dependencyScanning: true,
+            },
+          });
+
+          waitForMutation(vm.$store, types.RECEIVE_DEPENDENCY_SCANNING_DIFF_SUCCESS)
+            .then(done)
+            .catch(done.fail);
+        });
 
-      it('should display the correct numbers of vulnerabilities', () => {
-        expect(vm.$el.textContent).toContain(
-          'Dependency scanning detected 2 new, and 1 fixed vulnerabilities',
-        );
+        it('should set setDependencyScanningDiffEndpoint', () => {
+          expect(vm.dependencyScanning.paths.diffEndpoint).toEqual(dependencyScanningEndpoint);
+        });
+
+        it('should display the correct numbers of vulnerabilities', () => {
+          expect(vm.$el.textContent).toContain(
+            'Dependency scanning detected 2 new, and 1 fixed vulnerabilities',
+          );
+        });
       });
     });
 
     describe('dast reports', () => {
       const dastEndpoint = 'dast.json';
+      const props = {
+        headBlobPath: 'path',
+        baseBlobPath: 'path',
+        sastHelpPath: 'path',
+        sastContainerHelpPath: 'path',
+        dastHelpPath: 'path',
+        dependencyScanningHelpPath: 'path',
+        vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
+        vulnerabilityFeedbackHelpPath: 'path',
+        pipelineId: 123,
+        canCreateIssue: true,
+        canCreateMergeRequest: true,
+        canDismissVulnerability: true,
+      };
 
-      beforeEach(done => {
+      beforeEach(() => {
         gon.features = gon.features || {};
         gon.features.dastMergeRequestReportApi = true;
 
@@ -385,40 +449,65 @@ describe('Grouped security reports app', () => {
         });
 
         mock.onGet('vulnerability_feedback_path.json').reply(200, []);
+      });
 
-        vm = mountComponent(Component, {
-          headBlobPath: 'path',
-          baseBlobPath: 'path',
-          sastHelpPath: 'path',
-          sastContainerHelpPath: 'path',
-          dastHelpPath: 'path',
-          dependencyScanningHelpPath: 'path',
-          vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
-          vulnerabilityFeedbackHelpPath: 'path',
-          pipelineId: 123,
-          canCreateIssue: true,
-          canCreateMergeRequest: true,
-          canDismissVulnerability: true,
+      describe('with reports disabled', () => {
+        beforeEach(() => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              dast: false,
+            },
+          });
         });
 
-        waitForMutation(vm.$store, types.RECEIVE_DAST_DIFF_SUCCESS)
-          .then(done)
-          .catch(done.fail);
+        it('should not render the widget', () => {
+          expect(vm.$el.querySelector('.js-dast-widget')).toBeNull();
+        });
       });
 
-      it('should set setDastDiffEndpoint', () => {
-        expect(vm.dast.paths.diffEndpoint).toEqual(dastEndpoint);
-      });
+      describe('with reports enabled', () => {
+        beforeEach(done => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              dast: true,
+            },
+          });
+
+          waitForMutation(vm.$store, types.RECEIVE_DAST_DIFF_SUCCESS)
+            .then(done)
+            .catch(done.fail);
+        });
+
+        it('should set setDastDiffEndpoint', () => {
+          expect(vm.dast.paths.diffEndpoint).toEqual(dastEndpoint);
+        });
 
-      it('should display the correct numbers of vulnerabilities', () => {
-        expect(vm.$el.textContent).toContain('DAST detected 1 new, and 2 fixed vulnerabilities');
+        it('should display the correct numbers of vulnerabilities', () => {
+          expect(vm.$el.textContent).toContain('DAST detected 1 new, and 2 fixed vulnerabilities');
+        });
       });
     });
 
     describe('sast reports', () => {
       const sastEndpoint = 'sast.json';
+      const props = {
+        headBlobPath: 'path',
+        baseBlobPath: 'path',
+        sastHelpPath: 'path',
+        sastContainerHelpPath: 'path',
+        dastHelpPath: 'path',
+        dependencyScanningHelpPath: 'path',
+        vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
+        vulnerabilityFeedbackHelpPath: 'path',
+        pipelineId: 123,
+        canCreateIssue: true,
+        canCreateMergeRequest: true,
+        canDismissVulnerability: true,
+      };
 
-      beforeEach(done => {
+      beforeEach(() => {
         gon.features = gon.features || {};
         gon.features.sastMergeRequestReportApi = true;
 
@@ -432,33 +521,44 @@ describe('Grouped security reports app', () => {
         });
 
         mock.onGet('vulnerability_feedback_path.json').reply(200, []);
+      });
 
-        vm = mountComponent(Component, {
-          headBlobPath: 'path',
-          baseBlobPath: 'path',
-          sastHelpPath: 'path',
-          sastContainerHelpPath: 'path',
-          dastHelpPath: 'path',
-          dependencyScanningHelpPath: 'path',
-          vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
-          vulnerabilityFeedbackHelpPath: 'path',
-          pipelineId: 123,
-          canCreateIssue: true,
-          canCreateMergeRequest: true,
-          canDismissVulnerability: true,
+      describe('with reports disabled', () => {
+        beforeEach(() => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              sast: false,
+            },
+          });
         });
 
-        waitForMutation(vm.$store, `sast/${sastTypes.RECEIVE_DIFF_SUCCESS}`)
-          .then(done)
-          .catch(done.fail);
+        it('should not render the widget', () => {
+          expect(vm.$el.querySelector('.js-sast-widget')).toBeNull();
+        });
       });
 
-      it('should set setSastDiffEndpoint', () => {
-        expect(vm.sast.paths.diffEndpoint).toEqual(sastEndpoint);
-      });
+      describe('with reports enabled', () => {
+        beforeEach(done => {
+          vm = mountComponent(Component, {
+            ...props,
+            enabledReports: {
+              sast: true,
+            },
+          });
+
+          waitForMutation(vm.$store, `sast/${sastTypes.RECEIVE_DIFF_SUCCESS}`)
+            .then(done)
+            .catch(done.fail);
+        });
 
-      it('should display the correct numbers of vulnerabilities', () => {
-        expect(vm.$el.textContent).toContain('SAST detected 1 new, and 2 fixed vulnerabilities');
+        it('should set setSastDiffEndpoint', () => {
+          expect(vm.sast.paths.diffEndpoint).toEqual(sastEndpoint);
+        });
+
+        it('should display the correct numbers of vulnerabilities', () => {
+          expect(vm.$el.textContent).toContain('SAST detected 1 new, and 2 fixed vulnerabilities');
+        });
       });
     });
   });

ee/spec/models/merge_request_spec.rb

@@ -118,6 +118,38 @@ describe MergeRequest do
     end
   end
 
+  describe '#enabled_reports' do
+    let(:project) { create(:project, :repository) }
+
+    where(:report_type, :with_reports) do
+      :sast | :with_sast_reports
+      :container_scanning | :with_container_scanning_reports
+      :dast | :with_dast_reports
+      :dependency_scanning | :with_dependency_scanning_reports
+      :license_management | :with_license_management_reports
+    end
+
+    with_them do
+      subject { merge_request.enabled_reports[report_type] }
+
+      before do
+        stub_licensed_features({ report_type => true })
+      end
+
+      context "when head pipeline has reports" do
+        let(:merge_request) { create(:ee_merge_request, with_reports, source_project: project) }
+
+        it { is_expected.to be_truthy }
+      end
+
+      context "when head pipeline does not have reports" do
+        let(:merge_request) { create(:ee_merge_request, source_project: project) }
+
+        it { is_expected.to be_falsy }
+      end
+    end
+  end
+
   describe '#participant_approvers with approval_rules disabled' do
     let!(:approver) { create(:approver, target: project) }
     let(:code_owners) { [double(:code_owner)] }

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -59,6 +59,37 @@ describe MergeRequestWidgetEntity do
     expect { serializer.represent(merge_request) }.not_to exceed_query_limit(control)
   end
 
+  describe 'enabled_reports' do
+    it 'marks all reports as disabled by default' do
+      expect(subject.as_json).to include(:enabled_reports)
+      expect(subject.as_json[:enabled_reports]).to eq({
+        sast: false,
+        container_scanning: false,
+        dast: false,
+        dependency_scanning: false,
+        license_management: false
+      })
+    end
+
+    it 'marks reports as enabled if artifacts exist' do
+      allow(merge_request).to receive(:enabled_reports).and_return({
+        sast: true,
+        container_scanning: true,
+        dast: true,
+        dependency_scanning: true,
+        license_management: true
+      })
+      expect(subject.as_json).to include(:enabled_reports)
+      expect(subject.as_json[:enabled_reports]).to eq({
+        sast: true,
+        container_scanning: true,
+        dast: true,
+        dependency_scanning: true,
+        license_management: true
+      })
+    end
+  end
+
   describe 'test report artifacts', :request_store do
     using RSpec::Parameterized::TableSyntax