Merge branch '215570-disable-vuln-feedback-loading-on-unauthorized' into 'master'

Disable loading vulnerability feedback in MR when user is unauthorized

See merge request gitlab-org/gitlab!36172

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

@@ -421,6 +421,7 @@ export default {
         :container-scanning-help-path="mr.containerScanningHelp"
         :dependency-scanning-help-path="mr.dependencyScanningHelp"
         :secret-scanning-help-path="mr.secretScanningHelp"
+        :can-read-vulnerability-feedback="mr.canReadVulnerabilityFeedback"
         :vulnerability-feedback-path="mr.vulnerabilityFeedbackPath"
         :vulnerability-feedback-help-path="mr.vulnerabilityFeedbackHelpPath"
         :create-vulnerability-feedback-issue-path="mr.createVulnerabilityFeedbackIssuePath"

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

@@ -17,6 +17,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     this.secretScanningHelp = data.secret_scanning_help_path;
     this.dependencyScanningHelp = data.dependency_scanning_help_path;
     this.vulnerabilityFeedbackPath = data.vulnerability_feedback_path;
+    this.canReadVulnerabilityFeedback = data.can_read_vulnerability_feedback;
     this.vulnerabilityFeedbackHelpPath = data.vulnerability_feedback_help_path;
     this.approvalsHelpPath = data.approvals_help_path;
     this.codequalityHelpPath = data.codequality_help_path;

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

@@ -77,6 +77,11 @@ export default {
       required: false,
       default: '',
     },
+    canReadVulnerabilityFeedback: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
     vulnerabilityFeedbackPath: {
       type: String,
       required: false,
@@ -204,6 +209,7 @@ export default {
     this.setBaseBlobPath(this.baseBlobPath);
     this.setSourceBranch(this.sourceBranch);
 
+    this.setCanReadVulnerabilityFeedback(this.canReadVulnerabilityFeedback);
     this.setVulnerabilityFeedbackPath(this.vulnerabilityFeedbackPath);
     this.setVulnerabilityFeedbackHelpPath(this.vulnerabilityFeedbackHelpPath);
     this.setCreateVulnerabilityFeedbackIssuePath(this.createVulnerabilityFeedbackIssuePath);
@@ -253,6 +259,7 @@ export default {
       'setHeadBlobPath',
       'setBaseBlobPath',
       'setSourceBranch',
+      'setCanReadVulnerabilityFeedback',
       'setVulnerabilityFeedbackPath',
       'setVulnerabilityFeedbackHelpPath',
       'setCreateVulnerabilityFeedbackIssuePath',

ee/app/assets/javascripts/vue_shared/security_reports/store/actions.js

@@ -24,6 +24,9 @@ export const setBaseBlobPath = ({ commit }, blobPath) => commit(types.SET_BASE_B
 
 export const setSourceBranch = ({ commit }, branch) => commit(types.SET_SOURCE_BRANCH, branch);
 
+export const setCanReadVulnerabilityFeedback = ({ commit }, value) =>
+  commit(types.SET_CAN_READ_VULNERABILITY_FEEDBACK, value);
+
 export const setVulnerabilityFeedbackPath = ({ commit }, path) =>
   commit(types.SET_VULNERABILITY_FEEDBACK_PATH, path);
 
@@ -41,6 +44,19 @@ export const setCreateVulnerabilityFeedbackDismissalPath = ({ commit }, path) =>
 
 export const setPipelineId = ({ commit }, id) => commit(types.SET_PIPELINE_ID, id);
 
+export const fetchDiffData = (state, endpoint, category) => {
+  const requests = [pollUntilComplete(endpoint)];
+
+  if (state.canReadVulnerabilityFeedback) {
+    requests.push(axios.get(state.vulnerabilityFeedbackPath, { params: { category } }));
+  }
+
+  return Promise.all(requests).then(([diffResponse, enrichResponse]) => ({
+    diff: diffResponse.data,
+    enrichData: enrichResponse?.data ?? [],
+  }));
+};
+
 /**
  * CONTAINER SCANNING
  */
@@ -60,19 +76,9 @@ export const receiveContainerScanningDiffError = ({ commit }) =>
 export const fetchContainerScanningDiff = ({ state, dispatch }) => {
   dispatch('requestContainerScanningDiff');
 
-  return Promise.all([
-    pollUntilComplete(state.containerScanning.paths.diffEndpoint),
-    axios.get(state.vulnerabilityFeedbackPath, {
-      params: {
-        category: 'container_scanning',
-      },
-    }),
-  ])
-    .then(values => {
-      dispatch('receiveContainerScanningDiffSuccess', {
-        diff: values[0].data,
-        enrichData: values[1].data,
-      });
+  return fetchDiffData(state, state.containerScanning.paths.diffEndpoint, 'container_scanning')
+    .then(data => {
+      dispatch('receiveContainerScanningDiffSuccess', data);
     })
     .catch(() => {
       dispatch('receiveContainerScanningDiffError');
@@ -99,19 +105,9 @@ export const receiveDastDiffError = ({ commit }) => commit(types.RECEIVE_DAST_DI
 export const fetchDastDiff = ({ state, dispatch }) => {
   dispatch('requestDastDiff');
 
-  return Promise.all([
-    pollUntilComplete(state.dast.paths.diffEndpoint),
-    axios.get(state.vulnerabilityFeedbackPath, {
-      params: {
-        category: 'dast',
-      },
-    }),
-  ])
-    .then(values => {
-      dispatch('receiveDastDiffSuccess', {
-        diff: values[0].data,
-        enrichData: values[1].data,
-      });
+  return fetchDiffData(state, state.dast.paths.diffEndpoint, 'dast')
+    .then(data => {
+      dispatch('receiveDastDiffSuccess', data);
     })
     .catch(() => {
       dispatch('receiveDastDiffError');
@@ -137,19 +133,9 @@ export const receiveDependencyScanningDiffError = ({ commit }) =>
 export const fetchDependencyScanningDiff = ({ state, dispatch }) => {
   dispatch('requestDependencyScanningDiff');
 
-  return Promise.all([
-    pollUntilComplete(state.dependencyScanning.paths.diffEndpoint),
-    axios.get(state.vulnerabilityFeedbackPath, {
-      params: {
-        category: 'dependency_scanning',
-      },
-    }),
-  ])
-    .then(values => {
-      dispatch('receiveDependencyScanningDiffSuccess', {
-        diff: values[0].data,
-        enrichData: values[1].data,
-      });
+  return fetchDiffData(state, state.dependencyScanning.paths.diffEndpoint, 'dependency_scanning')
+    .then(data => {
+      dispatch('receiveDependencyScanningDiffSuccess', data);
     })
     .catch(() => {
       dispatch('receiveDependencyScanningDiffError');
@@ -177,19 +163,9 @@ export const receiveSecretScanningDiffError = ({ commit }) =>
 export const fetchSecretScanningDiff = ({ state, dispatch }) => {
   dispatch('requestSecretScanningDiff');
 
-  return Promise.all([
-    pollUntilComplete(state.secretScanning.paths.diffEndpoint),
-    axios.get(state.vulnerabilityFeedbackPath, {
-      params: {
-        category: 'secret_scanning',
-      },
-    }),
-  ])
-    .then(values => {
-      dispatch('receiveSecretScanningDiffSuccess', {
-        diff: values[0].data,
-        enrichData: values[1].data,
-      });
+  return fetchDiffData(state, state.secretScanning.paths.diffEndpoint, 'secret_scanning')
+    .then(data => {
+      dispatch('receiveSecretScanningDiffSuccess', data);
     })
     .catch(() => {
       dispatch('receiveSecretScanningDiffError');

ee/app/assets/javascripts/vue_shared/security_reports/store/modules/sast/actions.js

-import axios from '~/lib/utils/axios_utils';
-import pollUntilComplete from '~/lib/utils/poll_until_complete';
 import * as types from './mutation_types';
+import { fetchDiffData } from '../../actions';
 
 export const setDiffEndpoint = ({ commit }, path) => commit(types.SET_DIFF_ENDPOINT, path);
 
@@ -18,19 +17,9 @@ export const receiveDiffError = ({ commit }, response) =>
 export const fetchDiff = ({ state, rootState, dispatch }) => {
   dispatch('requestDiff');
 
-  return Promise.all([
-    pollUntilComplete(state.paths.diffEndpoint),
-    axios.get(rootState.vulnerabilityFeedbackPath, {
-      params: {
-        category: 'sast',
-      },
-    }),
-  ])
-    .then(values => {
-      dispatch('receiveDiffSuccess', {
-        diff: values[0].data,
-        enrichData: values[1].data,
-      });
+  return fetchDiffData(rootState, state.paths.diffEndpoint, 'sast')
+    .then(data => {
+      dispatch('receiveDiffSuccess', data);
     })
     .catch(() => {
       dispatch('receiveDiffError');

ee/app/assets/javascripts/vue_shared/security_reports/store/mutation_types.js

 export const SET_HEAD_BLOB_PATH = 'SET_HEAD_BLOB_PATH';
 export const SET_BASE_BLOB_PATH = 'SET_BASE_BLOB_PATH';
 export const SET_SOURCE_BRANCH = 'SET_SOURCE_BRANCH';
+export const SET_CAN_READ_VULNERABILITY_FEEDBACK = 'SET_CAN_READ_VULNERABILITY_FEEDBACK';
 export const SET_VULNERABILITY_FEEDBACK_PATH = 'SET_VULNERABILITY_FEEDBACK_PATH';
 export const SET_VULNERABILITY_FEEDBACK_HELP_PATH = 'SET_VULNERABILITY_FEEDBACK_HELP_PATH';
 export const SET_CREATE_VULNERABILITY_FEEDBACK_ISSUE_PATH =

ee/app/assets/javascripts/vue_shared/security_reports/store/mutations.js

@@ -16,6 +16,10 @@ export default {
     state.sourceBranch = branch;
   },
 
+  [types.SET_CAN_READ_VULNERABILITY_FEEDBACK](state, value) {
+    state.canReadVulnerabilityFeedback = value;
+  },
+
   [types.SET_VULNERABILITY_FEEDBACK_PATH](state, path) {
     state.vulnerabilityFeedbackPath = path;
   },

ee/app/assets/javascripts/vue_shared/security_reports/store/state.js

@@ -5,6 +5,7 @@ export default () => ({
   },
 
   sourceBranch: null,
+  canReadVulnerabilityFeedback: false,
   vulnerabilityFeedbackPath: null,
   vulnerabilityFeedbackHelpPath: null,
   createVulnerabilityFeedbackIssuePath: null,

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -81,6 +81,10 @@ module EE
         merge_request.head_pipeline.id
       end
 
+      expose :can_read_vulnerability_feedback do |merge_request|
+        can?(current_user, :read_vulnerability_feedback, merge_request.project)
+      end
+
       expose :vulnerability_feedback_path do |merge_request|
         project_vulnerability_feedback_index_path(merge_request.project)
       end

ee/spec/fixtures/api/schemas/entities/merge_request_poll_widget.json

@@ -12,6 +12,7 @@
           }
         },
         "pipeline_id": { "type": "integer" },
+        "can_read_vulnerability_feedback": { "type": "boolean" },
         "vulnerability_feedback_path": { "type": "string" },
         "create_vulnerability_feedback_issue_path": { "type": "string" },
         "create_vulnerability_feedback_merge_request_path": { "type": "string" },

ee/spec/fixtures/api/schemas/entities/merge_request_widget.json

@@ -18,6 +18,7 @@
         "api_approve_path": { "type": ["string", "null"] },
         "api_unapprove_path": { "type": ["string", "null"] },
         "vulnerability_feedback_path": { "type": "string" },
+        "can_read_vulnerability_feedback": { "type": "boolean" },
         "create_vulnerability_feedback_issue_path": { "type": ["string", "null"] },
         "create_vulnerability_feedback_merge_request_path": { "type": ["string", "null"] },
         "create_vulnerability_feedback_dismissal_path": { "type": ["string", "null"] }

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

@@ -45,6 +45,7 @@ describe('Grouped security reports app', () => {
     dastHelpPath: 'path',
     dependencyScanningHelpPath: 'path',
     secretScanningHelpPath: 'path',
+    canReadVulnerabilityFeedbackPath: true,
     vulnerabilityFeedbackPath: 'vulnerability_feedback_path.json',
     vulnerabilityFeedbackHelpPath: 'path',
     pipelineId: 123,

ee/spec/frontend/vue_shared/security_reports/store/actions_spec.js

@@ -2,6 +2,7 @@ import MockAdapter from 'axios-mock-adapter';
 import {
   setHeadBlobPath,
   setBaseBlobPath,
+  setCanReadVulnerabilityFeedback,
   setVulnerabilityFeedbackPath,
   setVulnerabilityFeedbackHelpPath,
   setPipelineId,
@@ -148,6 +149,24 @@ describe('security reports actions', () => {
     });
   });
 
+  describe('setCanReadVulnerabilityFeedback', () => {
+    it('should commit set vulnerabulity feedback path', done => {
+      testAction(
+        setCanReadVulnerabilityFeedback,
+        true,
+        mockedState,
+        [
+          {
+            type: types.SET_CAN_READ_VULNERABILITY_FEEDBACK,
+            payload: true,
+          },
+        ],
+        [],
+        done,
+      );
+    });
+  });
+
   describe('setVulnerabilityFeedbackPath', () => {
     it('should commit set vulnerabulity feedback path', done => {
       testAction(
@@ -414,6 +433,7 @@ describe('security reports actions', () => {
     it('with error should dispatch `receiveDismissVulnerabilityError`', done => {
       mock.onPost('dismiss_vulnerability_path').reply(500, {});
       mockedState.vulnerabilityFeedbackPath = 'dismiss_vulnerability_path';
+      mockedState.canReadVulnerabilityFeedback = true;
 
       testAction(
         dismissVulnerability,
@@ -852,6 +872,7 @@ describe('security reports actions', () => {
     it('with error should dispatch `receiveCreateIssueError`', done => {
       mock.onPost('create_issue_path').reply(500, {});
       mockedState.vulnerabilityFeedbackPath = 'create_issue_path';
+      mockedState.canReadVulnerabilityFeedback = true;
 
       testAction(
         createNewIssue,
@@ -980,6 +1001,7 @@ describe('security reports actions', () => {
     it('with error should dispatch `receiveCreateMergeRequestError`', done => {
       mock.onPost('create_merge_request_path').reply(500, {});
       mockedState.vulnerabilityFeedbackPath = 'create_merge_request_path';
+      mockedState.canReadVulnerabilityFeedback = true;
 
       testAction(
         createMergeRequest,
@@ -1143,6 +1165,7 @@ describe('security reports actions', () => {
 
     beforeEach(() => {
       mockedState.vulnerabilityFeedbackPath = 'vulnerabilities_feedback';
+      mockedState.canReadVulnerabilityFeedback = true;
       mockedState.containerScanning.paths.diffEndpoint = endpoint;
     });
 
@@ -1179,6 +1202,35 @@ describe('security reports actions', () => {
       });
     });
 
+    describe('when diff endpoint responds successfully and fetching vulnerability feedback is not authorized', () => {
+      beforeEach(() => {
+        mockedState.canReadVulnerabilityFeedback = false;
+        mock.onGet(endpoint).reply(200, diff);
+      });
+
+      it('should dispatch `receiveContainerScanningDiffSuccess`', done => {
+        testAction(
+          fetchContainerScanningDiff,
+          null,
+          mockedState,
+          [],
+          [
+            {
+              type: 'requestContainerScanningDiff',
+            },
+            {
+              type: 'receiveContainerScanningDiffSuccess',
+              payload: {
+                diff,
+                enrichData: [],
+              },
+            },
+          ],
+          done,
+        );
+      });
+    });
+
     describe('when vulnerabilities path errors', () => {
       it('should dispatch `receiveContainerScanningError`', done => {
         mock.onGet(endpoint).reply(500);
@@ -1300,6 +1352,7 @@ describe('security reports actions', () => {
 
     beforeEach(() => {
       mockedState.vulnerabilityFeedbackPath = 'vulnerabilities_feedback';
+      mockedState.canReadVulnerabilityFeedback = true;
       mockedState.dependencyScanning.paths.diffEndpoint = 'dependency_scanning_diff.json';
     });
 
@@ -1336,6 +1389,35 @@ describe('security reports actions', () => {
       });
     });
 
+    describe('when diff endpoint responds successfully and fetching vulnerability feedback is not authorized', () => {
+      beforeEach(() => {
+        mockedState.canReadVulnerabilityFeedback = false;
+        mock.onGet('dependency_scanning_diff.json').reply(200, diff);
+      });
+
+      it('should dispatch `receiveDependencyScanningDiffSuccess`', done => {
+        testAction(
+          fetchDependencyScanningDiff,
+          null,
+          mockedState,
+          [],
+          [
+            {
+              type: 'requestDependencyScanningDiff',
+            },
+            {
+              type: 'receiveDependencyScanningDiffSuccess',
+              payload: {
+                diff,
+                enrichData: [],
+              },
+            },
+          ],
+          done,
+        );
+      });
+    });
+
     describe('when vulnerabilities path errors', () => {
       it('should dispatch `receiveDependencyScanningError`', done => {
         mock.onGet('dependency_scanning_diff.json').reply(500);
@@ -1457,6 +1539,7 @@ describe('security reports actions', () => {
 
     beforeEach(() => {
       mockedState.vulnerabilityFeedbackPath = 'vulnerabilities_feedback';
+      mockedState.canReadVulnerabilityFeedback = true;
       mockedState.dast.paths.diffEndpoint = 'dast_diff.json';
     });
 
@@ -1493,6 +1576,35 @@ describe('security reports actions', () => {
       });
     });
 
+    describe('when diff endpoint responds successfully and fetching vulnerability feedback is not authorized', () => {
+      beforeEach(() => {
+        mockedState.canReadVulnerabilityFeedback = false;
+        mock.onGet('dast_diff.json').reply(200, diff);
+      });
+
+      it('should dispatch `receiveDastDiffSuccess`', done => {
+        testAction(
+          fetchDastDiff,
+          null,
+          mockedState,
+          [],
+          [
+            {
+              type: 'requestDastDiff',
+            },
+            {
+              type: 'receiveDastDiffSuccess',
+              payload: {
+                diff,
+                enrichData: [],
+              },
+            },
+          ],
+          done,
+        );
+      });
+    });
+
     describe('when vulnerabilities path errors', () => {
       it('should dispatch `receiveDastError`', done => {
         mock.onGet('dast_diff.json').reply(500);
@@ -1615,6 +1727,7 @@ describe('security reports actions', () => {
 
     beforeEach(() => {
       mockedState.vulnerabilityFeedbackPath = 'vulnerabilities_feedback';
+      mockedState.canReadVulnerabilityFeedback = true;
       mockedState.secretScanning.paths.diffEndpoint = endpoint;
     });
 
@@ -1651,6 +1764,35 @@ describe('security reports actions', () => {
       });
     });
 
+    describe('when diff endpoint responds successfully and fetching vulnerability feedback is not authorized', () => {
+      beforeEach(() => {
+        mockedState.canReadVulnerabilityFeedback = false;
+        mock.onGet(endpoint).reply(200, diff);
+      });
+
+      it('should dispatch `secret_scanning`', done => {
+        testAction(
+          fetchSecretScanningDiff,
+          null,
+          mockedState,
+          [],
+          [
+            {
+              type: 'requestSecretScanningDiff',
+            },
+            {
+              type: 'receiveSecretScanningDiffSuccess',
+              payload: {
+                diff,
+                enrichData: [],
+              },
+            },
+          ],
+          done,
+        );
+      });
+    });
+
     describe('when vulnerabilities path errors', () => {
       it('should dispatch `receiveSecretScanningError`', done => {
         mock.onGet(endpoint).reply(500);

ee/spec/frontend/vue_shared/security_reports/store/modules/sast/actions_spec.js

@@ -92,6 +92,7 @@ describe('sast report actions', () => {
     beforeEach(() => {
       mock = new MockAdapter(axios);
       state.paths.diffEndpoint = diffEndpoint;
+      rootState.canReadVulnerabilityFeedback = true;
     });
 
     afterEach(() => {
@@ -129,6 +130,35 @@ describe('sast report actions', () => {
       });
     });
 
+    describe('when diff endpoint responds successfully and fetching vulnerability feedback is not authorized', () => {
+      beforeEach(() => {
+        rootState.canReadVulnerabilityFeedback = false;
+        mock.onGet(diffEndpoint).replyOnce(200, reports.diff);
+      });
+
+      it('should dispatch the `receiveDiffSuccess` action with empty enrich data', done => {
+        const { diff } = reports;
+        const enrichData = [];
+        testAction(
+          actions.fetchDiff,
+          {},
+          { ...rootState, ...state },
+          [],
+          [
+            { type: 'requestDiff' },
+            {
+              type: 'receiveDiffSuccess',
+              payload: {
+                diff,
+                enrichData,
+              },
+            },
+          ],
+          done,
+        );
+      });
+    });
+
     describe('when the vulnerability feedback endpoint fails', () => {
       beforeEach(() => {
         mock

ee/spec/frontend/vue_shared/security_reports/store/mutations_spec.js

@@ -31,6 +31,14 @@ describe('security reports mutations', () => {
     });
   });
 
+  describe('SET_CAN_READ_VULNERABILITY_FEEDBACK', () => {
+    it('should set the vulnerabilities endpoint', () => {
+      mutations[types.SET_CAN_READ_VULNERABILITY_FEEDBACK](stateCopy, false);
+
+      expect(stateCopy.canReadVulnerabilityFeedback).toEqual(false);
+    });
+  });
+
   describe('SET_VULNERABILITY_FEEDBACK_PATH', () => {
     it('should set the vulnerabilities endpoint', () => {
       mutations[types.SET_VULNERABILITY_FEEDBACK_PATH](stateCopy, 'vulnerability_path');

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -252,6 +252,32 @@ RSpec.describe MergeRequestWidgetEntity do
     expect(subject.as_json).to include(:create_vulnerability_feedback_dismissal_path)
   end
 
+  describe '#can_read_vulnerability_feedback' do
+    context 'when user has permissions to read vulnerability feedback' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'is set to true' do
+        expect(subject.as_json[:can_read_vulnerability_feedback]).to eq(true)
+      end
+    end
+
+    context 'when user has no permissions to read vulnerability feedback' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'is set to false' do
+        expect(subject.as_json[:can_read_vulnerability_feedback]).to eq(false)
+      end
+    end
+  end
+
+  it 'has can_read_vulnerability_feedback property' do
+    expect(subject.as_json).to include(:can_read_vulnerability_feedback)
+  end
+
   it 'has pipeline id' do
     allow(merge_request).to receive(:head_pipeline).and_return(pipeline)