Merge branch '33918-403-error-messages-when-fetching-vulnerabilities' into 'master'

Display proper error messages on vulnerabilities fetch failure

See merge request gitlab-org/gitlab!23812

ee/app/assets/javascripts/security_dashboard/components/app.vue

@@ -7,6 +7,7 @@ import SecurityDashboardTable from './security_dashboard_table.vue';
 import VulnerabilityChart from './vulnerability_chart.vue';
 import VulnerabilityCountList from './vulnerability_count_list.vue';
 import VulnerabilitySeverity from './vulnerability_severity.vue';
+import LoadingError from './loading_error.vue';
 
 export default {
   name: 'SecurityDashboardApp',
@@ -17,6 +18,7 @@ export default {
     VulnerabilityChart,
     VulnerabilityCountList,
     VulnerabilitySeverity,
+    LoadingError,
   },
   props: {
     vulnerabilitiesEndpoint: {
@@ -55,8 +57,9 @@ export default {
     },
   },
   computed: {
-    ...mapState('vulnerabilities', ['modal', 'pageInfo']),
+    ...mapState('vulnerabilities', ['modal', 'pageInfo', 'loadingVulnerabilitiesErrorCode']),
     ...mapGetters('filters', ['activeFilters']),
+    ...mapGetters('vulnerabilities', ['loadingVulnerabilitiesFailedWithRecognizedErrorCode']),
     canCreateIssue() {
       const path = this.vulnerability.create_vulnerability_feedback_issue_path;
       return Boolean(path);
@@ -138,48 +141,54 @@ export default {
 
 <template>
   <section>
-    <header>
-      <filters />
-    </header>
+    <loading-error
+      v-if="loadingVulnerabilitiesFailedWithRecognizedErrorCode"
+      :error-code="loadingVulnerabilitiesErrorCode"
+    />
+    <template v-else>
+      <header>
+        <filters />
+      </header>
 
-    <vulnerability-count-list v-if="shouldShowCountList" class="mb-0" />
+      <vulnerability-count-list v-if="shouldShowCountList" class="mb-0" />
 
-    <div class="row mt-4">
-      <article class="col" :class="{ 'col-xl-7': !isLockedToProject }">
-        <security-dashboard-table>
-          <template #emptyState>
-            <slot name="emptyState"></slot>
-          </template>
-        </security-dashboard-table>
-      </article>
+      <div class="row mt-4">
+        <article class="col" :class="{ 'col-xl-7': !isLockedToProject }">
+          <security-dashboard-table>
+            <template #emptyState>
+              <slot name="emptyState"></slot>
+            </template>
+          </security-dashboard-table>
+        </article>
 
-      <aside v-if="shouldShowAside" class="col-xl-5">
-        <vulnerability-chart v-if="shouldShowChart" class="mb-3" />
-        <vulnerability-severity
-          v-if="shouldShowVulnerabilitySeverities"
-          :endpoint="vulnerableProjectsEndpoint"
-        />
-      </aside>
-    </div>
+        <aside v-if="shouldShowAside" class="col-xl-5">
+          <vulnerability-chart v-if="shouldShowChart" class="mb-3" />
+          <vulnerability-severity
+            v-if="shouldShowVulnerabilitySeverities"
+            :endpoint="vulnerableProjectsEndpoint"
+          />
+        </aside>
+      </div>
 
-    <issue-modal
-      :modal="modal"
-      :vulnerability-feedback-help-path="vulnerabilityFeedbackHelpPath"
-      :can-create-issue="canCreateIssue"
-      :can-create-merge-request="canCreateMergeRequest"
-      :can-dismiss-vulnerability="canDismissVulnerability"
-      @addDismissalComment="addDismissalComment({ vulnerability, comment: $event })"
-      @editVulnerabilityDismissalComment="openDismissalCommentBox()"
-      @showDismissalDeleteButtons="showDismissalDeleteButtons"
-      @hideDismissalDeleteButtons="hideDismissalDeleteButtons"
-      @deleteDismissalComment="deleteDismissalComment({ vulnerability })"
-      @closeDismissalCommentBox="closeDismissalCommentBox()"
-      @createMergeRequest="createMergeRequest({ vulnerability })"
-      @createNewIssue="createIssue({ vulnerability })"
-      @dismissVulnerability="dismissVulnerability({ vulnerability, comment: $event })"
-      @openDismissalCommentBox="openDismissalCommentBox()"
-      @revertDismissVulnerability="undoDismiss({ vulnerability })"
-      @downloadPatch="downloadPatch({ vulnerability })"
-    />
+      <issue-modal
+        :modal="modal"
+        :vulnerability-feedback-help-path="vulnerabilityFeedbackHelpPath"
+        :can-create-issue="canCreateIssue"
+        :can-create-merge-request="canCreateMergeRequest"
+        :can-dismiss-vulnerability="canDismissVulnerability"
+        @addDismissalComment="addDismissalComment({ vulnerability, comment: $event })"
+        @editVulnerabilityDismissalComment="openDismissalCommentBox()"
+        @showDismissalDeleteButtons="showDismissalDeleteButtons"
+        @hideDismissalDeleteButtons="hideDismissalDeleteButtons"
+        @deleteDismissalComment="deleteDismissalComment({ vulnerability })"
+        @closeDismissalCommentBox="closeDismissalCommentBox()"
+        @createMergeRequest="createMergeRequest({ vulnerability })"
+        @createNewIssue="createIssue({ vulnerability })"
+        @dismissVulnerability="dismissVulnerability({ vulnerability, comment: $event })"
+        @openDismissalCommentBox="openDismissalCommentBox()"
+        @revertDismissVulnerability="undoDismiss({ vulnerability })"
+        @downloadPatch="downloadPatch({ vulnerability })"
+      />
+    </template>
   </section>
 </template>

ee/app/assets/javascripts/security_dashboard/components/loading_error.vue

+<script>
+import { GlEmptyState } from '@gitlab/ui';
+import { s__, __ } from '~/locale';
+import { LOADING_VULNERABILITIES_ERROR_CODES as ERROR_CODES } from '../store/modules/vulnerabilities/constants';
+import { imagePath } from '~/lib/utils/common_utils';
+
+const description = s__(
+  'Security Reports|Security reports can only be accessed by authorized users.',
+);
+
+export default {
+  emptyStatePropsMap: {
+    [ERROR_CODES.UNAUTHORIZED]: {
+      title: s__('Security Reports|You must sign in as an authorized user to see this report'),
+      description,
+      primaryButtonText: __('Sign in'),
+      primaryButtonLink: '/users/sign_in',
+      svgPath: imagePath('illustrations/user-not-logged-in.svg'),
+    },
+    [ERROR_CODES.FORBIDDEN]: {
+      title: s__('Security Reports|You do not have sufficient permissions to access this report'),
+      description,
+      svgPath: imagePath('illustrations/lock_promotion.svg'),
+    },
+  },
+  components: {
+    GlEmptyState,
+  },
+  props: {
+    errorCode: {
+      type: Number,
+      required: true,
+      validator: value => Object.values(ERROR_CODES).includes(value),
+    },
+  },
+};
+</script>
+
+<template>
+  <gl-empty-state v-bind="$options.emptyStatePropsMap[errorCode]" />
+</template>

ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/actions.js

@@ -81,8 +81,8 @@ export const fetchVulnerabilities = ({ state, dispatch }, params = {}) => {
       const { headers, data } = response;
       dispatch('receiveVulnerabilitiesSuccess', { headers, data });
     })
-    .catch(() => {
-      dispatch('receiveVulnerabilitiesError');
+    .catch(error => {
+      dispatch('receiveVulnerabilitiesError', error.response.status);
     });
 };
 
@@ -98,8 +98,8 @@ export const receiveVulnerabilitiesSuccess = ({ commit }, { headers, data }) =>
   commit(types.RECEIVE_VULNERABILITIES_SUCCESS, { pageInfo, vulnerabilities });
 };
 
-export const receiveVulnerabilitiesError = ({ commit }) => {
-  commit(types.RECEIVE_VULNERABILITIES_ERROR);
+export const receiveVulnerabilitiesError = ({ commit }, errorCode) => {
+  commit(types.RECEIVE_VULNERABILITIES_ERROR, errorCode);
 };
 
 export const openModal = ({ commit }, payload = {}) => {

ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/constants.js

+import httpStatusCodes from '~/lib/utils/http_status';
+
 export const CRITICAL = 'critical';
 export const HIGH = 'high';
 export const MEDIUM = 'medium';
@@ -10,3 +12,8 @@ export const DAYS = {
   SIXTY: 60,
   NINETY: 90,
 };
+
+export const LOADING_VULNERABILITIES_ERROR_CODES = {
+  UNAUTHORIZED: httpStatusCodes.UNAUTHORIZED,
+  FORBIDDEN: httpStatusCodes.FORBIDDEN,
+};

ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/getters.js

+import { LOADING_VULNERABILITIES_ERROR_CODES } from './constants';
+
 export const dashboardError = state =>
   state.errorLoadingVulnerabilities && state.errorLoadingVulnerabilitiesCount;
 export const dashboardListError = state =>
@@ -5,6 +7,12 @@ export const dashboardListError = state =>
 export const dashboardCountError = state =>
   !state.errorLoadingVulnerabilities && state.errorLoadingVulnerabilitiesCount;
 
+export const loadingVulnerabilitiesFailedWithRecognizedErrorCode = state =>
+  state.errorLoadingVulnerabilities &&
+  Object.values(LOADING_VULNERABILITIES_ERROR_CODES).includes(
+    state.loadingVulnerabilitiesErrorCode,
+  );
+
 export const getVulnerabilityHistoryByName = state => name =>
   state.vulnerabilitiesHistory[name.toLowerCase()];
 

ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js

@@ -19,15 +19,17 @@ export default {
   [types.REQUEST_VULNERABILITIES](state) {
     state.isLoadingVulnerabilities = true;
     state.errorLoadingVulnerabilities = false;
+    state.loadingVulnerabilitiesErrorCode = null;
   },
   [types.RECEIVE_VULNERABILITIES_SUCCESS](state, payload) {
     state.isLoadingVulnerabilities = false;
     state.pageInfo = payload.pageInfo;
     state.vulnerabilities = payload.vulnerabilities;
   },
-  [types.RECEIVE_VULNERABILITIES_ERROR](state) {
+  [types.RECEIVE_VULNERABILITIES_ERROR](state, errorCode = null) {
     state.isLoadingVulnerabilities = false;
     state.errorLoadingVulnerabilities = true;
+    state.loadingVulnerabilitiesErrorCode = errorCode;
   },
   [types.SET_VULNERABILITIES_COUNT_ENDPOINT](state, payload) {
     state.vulnerabilitiesCountEndpoint = payload;

ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/state.js

@@ -3,6 +3,7 @@ import { __, s__ } from '~/locale';
 export default () => ({
   isLoadingVulnerabilities: true,
   errorLoadingVulnerabilities: false,
+  loadingVulnerabilitiesErrorCode: null,
   vulnerabilities: [],
   isLoadingVulnerabilitiesCount: true,
   errorLoadingVulnerabilitiesCount: false,

ee/changelogs/unreleased/33918-403-error-messages-when-fetching-vulnerabilities.yml

+---
+title: Display proper error messages on vulnerabilities fetch failure
+merge_request: 23812
+author:
+type: changed

ee/spec/frontend/security_dashboard/components/__snapshots__/loading_error_spec.js.snap

+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`LoadingError component with error code 401 empty state has correct props 1`] = `
+Object {
+  "compact": false,
+  "description": "Security reports can only be accessed by authorized users.",
+  "primaryButtonLink": "/users/sign_in",
+  "primaryButtonText": "Sign in",
+  "secondaryButtonLink": null,
+  "secondaryButtonText": null,
+  "svgPath": "/assets/illustrations/user-not-logged-in.svg",
+  "title": "You must sign in as an authorized user to see this report",
+}
+`;
+
+exports[`LoadingError component with error code 403 empty state has correct props 1`] = `
+Object {
+  "compact": false,
+  "description": "Security reports can only be accessed by authorized users.",
+  "primaryButtonLink": null,
+  "primaryButtonText": null,
+  "secondaryButtonLink": null,
+  "secondaryButtonText": null,
+  "svgPath": "/assets/illustrations/lock_promotion.svg",
+  "title": "You do not have sufficient permissions to access this report",
+}
+`;

ee/spec/frontend/security_dashboard/components/app_spec.js

@@ -8,6 +8,7 @@ import SecurityDashboardTable from 'ee/security_dashboard/components/security_da
 import VulnerabilityChart from 'ee/security_dashboard/components/vulnerability_chart.vue';
 import VulnerabilityCountList from 'ee/security_dashboard/components/vulnerability_count_list.vue';
 import VulnerabilitySeverity from 'ee/security_dashboard/components/vulnerability_severity.vue';
+import LoadingError from 'ee/security_dashboard/components/loading_error.vue';
 
 import createStore from 'ee/security_dashboard/store';
 import { getParameterValues } from '~/lib/utils/url_utility';
@@ -156,14 +157,9 @@ describe('Security Dashboard app', () => {
 
   describe('dismissed vulnerabilities', () => {
     beforeEach(() => {
-      getParameterValues.mockImplementation(() => [true]);
       setup();
     });
 
-    afterEach(() => {
-      getParameterValues.mockRestore();
-    });
-
     it.each`
       description                                                        | getParameterValuesReturnValue | expected
       ${'hides dismissed vulnerabilities by default'}                    | ${[]}                         | ${true}
@@ -175,4 +171,25 @@ describe('Security Dashboard app', () => {
       expect(wrapper.vm.$store.state.filters.hideDismissed).toBe(expected);
     });
   });
+
+  describe('on error', () => {
+    beforeEach(() => {
+      setup();
+      createComponent();
+    });
+
+    it.each([401, 403])('displays an error on error %s', errorCode => {
+      store.dispatch('vulnerabilities/receiveVulnerabilitiesError', errorCode);
+      return wrapper.vm.$nextTick().then(() => {
+        expect(wrapper.find(LoadingError).exists()).toBe(true);
+      });
+    });
+
+    it.each([404, 500])('does not display an error on error %s', errorCode => {
+      store.dispatch('vulnerabilities/receiveVulnerabilitiesError', errorCode);
+      return wrapper.vm.$nextTick().then(() => {
+        expect(wrapper.find(LoadingError).exists()).toBe(false);
+      });
+    });
+  });
 });

ee/spec/frontend/security_dashboard/components/loading_error_spec.js

+import { shallowMount } from '@vue/test-utils';
+
+import { GlEmptyState } from '@gitlab/ui';
+
+import LoadingError from 'ee/security_dashboard/components/loading_error.vue';
+
+describe('LoadingError component', () => {
+  let wrapper;
+
+  const createWrapper = errorCode => {
+    wrapper = shallowMount(LoadingError, {
+      propsData: {
+        errorCode,
+      },
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+    wrapper = null;
+  });
+
+  describe.each([401, 403])('with error code %s', errorCode => {
+    beforeEach(() => {
+      createWrapper(errorCode);
+    });
+
+    it('renders an empty state', () => {
+      expect(wrapper.find(GlEmptyState).exists()).toBe(true);
+    });
+
+    it('empty state has correct props', () => {
+      expect(wrapper.find(GlEmptyState).props()).toMatchSnapshot();
+    });
+  });
+});

ee/spec/javascripts/security_dashboard/store/vulnerabilities/actions_spec.js

@@ -290,8 +290,10 @@ describe('vulnerabilities actions', () => {
     });
 
     describe('on error', () => {
+      const errorCode = 404;
+
       beforeEach(() => {
-        mock.onGet(state.vulnerabilitiesEndpoint).replyOnce(404, {});
+        mock.onGet(state.vulnerabilitiesEndpoint).replyOnce(errorCode, {});
       });
 
       it('should dispatch the request and error actions', done => {
@@ -300,7 +302,10 @@ describe('vulnerabilities actions', () => {
           {},
           state,
           [],
-          [{ type: 'requestVulnerabilities' }, { type: 'receiveVulnerabilitiesError' }],
+          [
+            { type: 'requestVulnerabilities' },
+            { type: 'receiveVulnerabilitiesError', payload: errorCode },
+          ],
           done,
         );
       });
@@ -337,11 +342,13 @@ describe('vulnerabilities actions', () => {
 
   describe('receiveVulnerabilitiesError', () => {
     it('should commit the error mutation', done => {
+      const errorCode = 403;
+
       testAction(
         actions.receiveVulnerabilitiesError,
-        {},
+        errorCode,
         state,
-        [{ type: types.RECEIVE_VULNERABILITIES_ERROR }],
+        [{ type: types.RECEIVE_VULNERABILITIES_ERROR, payload: errorCode }],
         [],
         done,
       );

ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js

@@ -53,6 +53,7 @@ describe('vulnerabilities module mutations', () => {
   describe('REQUEST_VULNERABILITIES', () => {
     beforeEach(() => {
       state.errorLoadingVulnerabilities = true;
+      state.loadingVulnerabilitiesErrorCode = 403;
       mutations[types.REQUEST_VULNERABILITIES](state);
     });
 
@@ -63,6 +64,10 @@ describe('vulnerabilities module mutations', () => {
     it('should set `errorLoadingVulnerabilities` to `false`', () => {
       expect(state.errorLoadingVulnerabilities).toBeFalsy();
     });
+
+    it('should reset `loadingVulnerabilitiesErrorCode`', () => {
+      expect(state.loadingVulnerabilitiesErrorCode).toBe(null);
+    });
   });
 
   describe('RECEIVE_VULNERABILITIES_SUCCESS', () => {
@@ -90,11 +95,19 @@ describe('vulnerabilities module mutations', () => {
   });
 
   describe('RECEIVE_VULNERABILITIES_ERROR', () => {
-    it('should set `isLoadingVulnerabilities` to `false`', () => {
-      mutations[types.RECEIVE_VULNERABILITIES_ERROR](state);
+    const errorCode = 403;
 
+    beforeEach(() => {
+      mutations[types.RECEIVE_VULNERABILITIES_ERROR](state, errorCode);
+    });
+
+    it('should set `isLoadingVulnerabilities` to `false`', () => {
       expect(state.isLoadingVulnerabilities).toBeFalsy();
     });
+
+    it('should set `loadingVulnerabilitiesErrorCode`', () => {
+      expect(state.loadingVulnerabilitiesErrorCode).toBe(errorCode);
+    });
   });
 
   describe('SET_VULNERABILITIES_COUNT_ENDPOINT', () => {

locale/gitlab.pot

@@ -16773,6 +16773,9 @@ msgstr ""
 msgid "Security Reports|Oops, something doesn't seem right."
 msgstr ""
 
+msgid "Security Reports|Security reports can only be accessed by authorized users."
+msgstr ""
+
 msgid "Security Reports|There was an error adding the comment."
 msgstr ""
 
@@ -16797,6 +16800,12 @@ msgstr ""
 msgid "Security Reports|Undo dismiss"
 msgstr ""
 
+msgid "Security Reports|You do not have sufficient permissions to access this report"
+msgstr ""
+
+msgid "Security Reports|You must sign in as an authorized user to see this report"
+msgstr ""
+
 msgid "Security configuration help link"
 msgstr ""