Skip to content

G
gitlab-ce
Commit 9f17a8b9 authored by Daniel Tian's avatar Daniel Tian Committed by Martin Wortschack
Browse files
Options

Improve bulk dismissal on security dashboard 

Hide vulnerability after dismissal if not in filter list and process
each dismissal instead of stopping after the first failure
parent c4372c85
Hide whitespace changes
Inline Side-by-side
Showing with 145 additions and 68 deletions
ee/app/assets/javascripts/security_dashboard/components/issues_badge.vue
View file @ 9f17a8b9
......@@ -23,6 +23,9 @@ export default {
popoverTitle() {
return n__('1 Issue', '%d Issues', this.numberOfIssues);
},
issueBadgeEl() {
return () => this.$refs.issueBadge?.$el;
},
},
};
</script>
......@@ -33,7 +36,7 @@ export default {
<gl-icon name="issues" class="gl-mr-2" />
{{ numberOfIssues }}
</gl-badge>
<gl-popover ref="popover" :target="() => $refs.issueBadge.$el" triggers="hover" placement="top">
<gl-popover ref="popover" :target="issueBadgeEl" triggers="hover" placement="top">
<template #title>
{{ popoverTitle }}
</template>
......
ee/app/assets/javascripts/security_dashboard/components/selection_summary.vue
View file @ 9f17a8b9
......@@ -2,7 +2,7 @@
import { GlButton, GlFormSelect } from '@gitlab/ui';
import { s__, n__ } from '~/locale';
import toast from '~/vue_shared/plugins/global_toast';
import { deprecatedCreateFlash as createFlash } from '~/flash';
import createFlash from '~/flash';
import vulnerabilityDismiss from '../graphql/vulnerability_dismiss.mutation.graphql';
const REASON_NONE = s__('SecurityReports|[No reason]');
......@@ -48,30 +48,46 @@ export default {
this.dismissSelectedVulnerabilities();
},
dismissSelectedVulnerabilities() {
let fulfilledCount = 0;
let rejectedCount = 0;
const promises = this.selectedVulnerabilities.map(vulnerability =>
this.$apollo.mutate({
mutation: vulnerabilityDismiss,
variables: { id: vulnerability.id, comment: this.dismissalReason },
}),
this.$apollo
.mutate({
mutation: vulnerabilityDismiss,
variables: { id: vulnerability.id, comment: this.dismissalReason },
})
.then(() => {
fulfilledCount += 1;
this.$emit('vulnerability-updated', vulnerability.id);
})
.catch(() => {
rejectedCount += 1;
}),
);
Promise.all(promises)
.then(() => {
toast(
n__(
'%d vulnerability dismissed',
'%d vulnerabilities dismissed',
this.selectedVulnerabilities.length,
),
);
if (fulfilledCount > 0) {
toast(
n__('%d vulnerability dismissed', '%d vulnerabilities dismissed', fulfilledCount),
);
}
this.$emit('deselect-all-vulnerabilities');
if (rejectedCount > 0) {
createFlash({
message: n__(
'SecurityReports|There was an error dismissing %d vulnerability. Please try again later.',
'SecurityReports|There was an error dismissing %d vulnerabilities. Please try again later.',
rejectedCount,
),
});
}
})
.catch(() => {
createFlash(
s__('SecurityReports|There was an error dismissing the vulnerabilities.'),
'alert',
);
createFlash({
message: s__('SecurityReports|There was an error dismissing the vulnerabilities.'),
});
});
},
},
......
ee/app/assets/javascripts/security_dashboard/components/vulnerability_list.vue
View file @ 9f17a8b9
......@@ -15,6 +15,7 @@ import SeverityBadge from 'ee/vue_shared/security_reports/components/severity_ba
import VulnerabilityCommentIcon from 'ee/security_dashboard/components/vulnerability_comment_icon.vue';
import convertReportType from 'ee/vue_shared/security_reports/store/utils/convert_report_type';
import getPrimaryIdentifier from 'ee/vue_shared/security_reports/store/utils/get_primary_identifier';
import { VULNERABILITY_STATES } from 'ee/vulnerabilities/constants';
import SecurityScannerAlert from './security_scanner_alert.vue';
import LocalStorageSync from '~/vue_shared/components/local_storage_sync.vue';
import { formatDate } from '~/lib/utils/datetime_utility';
......@@ -87,11 +88,19 @@ export default {
};
},
computed: {
// This is a workaround to remove vulnerabilities from the list when their state has changed
// through the bulk update feature, but no longer matches the filters. For more details:
// https://gitlab.com/gitlab-org/gitlab/-/merge_requests/43468#note_420050017
filteredVulnerabilities() {
return this.vulnerabilities.filter(x =>
this.filters.state?.length ? this.filters.state.includes(x.state) : true,
);
},
isSortable() {
return Boolean(this.$listeners['sort-changed']);
},
hasAnyScannersOtherThanGitLab() {
return this.vulnerabilities.some(v => v.scanner?.vendor !== 'GitLab');
return this.filteredVulnerabilities.some(v => v.scanner?.vendor !== 'GitLab');
},
notEnabledSecurityScanners() {
const { available = [], enabled = [] } = this.securityScanners;
......@@ -112,16 +121,16 @@ export default {
return Object.keys(this.filters).length > 0;
},
hasSelectedAllVulnerabilities() {
if (!this.vulnerabilities.length) {
if (!this.filteredVulnerabilities.length) {
return false;
}
return this.numOfSelectedVulnerabilities === this.vulnerabilities.length;
return this.numOfSelectedVulnerabilities === this.filteredVulnerabilities.length;
},
numOfSelectedVulnerabilities() {
return Object.keys(this.selectedVulnerabilities).length;
},
shouldShowSelectionSummary() {
return this.shouldShowSelection && Boolean(this.numOfSelectedVulnerabilities);
return this.shouldShowSelection && this.numOfSelectedVulnerabilities > 0;
},
theadClass() {
return this.shouldShowSelectionSummary ? 'below-selection-summary' : '';
......@@ -195,8 +204,8 @@ export default {
filters() {
this.selectedVulnerabilities = {};
},
vulnerabilities(vulnerabilities) {
const ids = new Set(vulnerabilities.map(v => v.id));
filteredVulnerabilities() {
const ids = new Set(this.filteredVulnerabilities.map(v => v.id));
Object.keys(this.selectedVulnerabilities).forEach(vulnerabilityId => {
if (!ids.has(vulnerabilityId)) {
......@@ -219,6 +228,9 @@ export default {
return file;
},
deselectVulnerability(vulnerabilityId) {
this.$delete(this.selectedVulnerabilities, vulnerabilityId);
},
deselectAllVulnerabilities() {
this.selectedVulnerabilities = {};
},
......@@ -282,6 +294,11 @@ export default {
this.$emit('sort-changed', { ...args, sortBy: convertToSnakeCase(args.sortBy) });
}
},
getVulnerabilityState(state = '') {
const stateName = state.toLowerCase();
// Use the raw state name if we don't have a localization for it.
return VULNERABILITY_STATES[stateName] || stateName;
},
},
VULNERABILITIES_PER_PAGE,
SCANNER_ALERT_DISMISSED_LOCAL_STORAGE_KEY,
......@@ -298,12 +315,12 @@ export default {
<selection-summary
v-if="shouldShowSelectionSummary"
:selected-vulnerabilities="Object.values(selectedVulnerabilities)"
@deselect-all-vulnerabilities="deselectAllVulnerabilities"
@vulnerability-updated="deselectVulnerability"
/>
<gl-table
:busy="isLoading"
:fields="fields"
:items="vulnerabilities"
:items="filteredVulnerabilities"
:thead-class="theadClass"
:sort-desc="sortDesc"
:sort-by="sortBy"
......@@ -313,6 +330,7 @@ export default {
class="vulnerability-list"
show-empty
responsive
primary-key="id"
@sort-changed="handleSortChange"
>
<template #head(checkbox)>
......@@ -350,7 +368,7 @@ export default {
</template>
<template #cell(state)="{ item }">
<span class="text-capitalize js-status">{{ item.state.toLowerCase() }}</span>
<span class="text-capitalize js-status">{{ getVulnerabilityState(item.state) }}</span>
</template>
<template #cell(severity)="{ item }">
......
ee/changelogs/unreleased/217530-improve-bulk-dismissal.yml 0 → 100644
View file @ 9f17a8b9
---
title: Remove item when dismissed on security dashboard if it no longer matches filter
merge_request: 43468
author:
type: changed
ee/spec/frontend/security_dashboard/components/mock_data.js
View file @ 9f17a8b9
......@@ -14,7 +14,7 @@ export const generateVulnerabilities = () => [
],
title: 'Vulnerability 0',
severity: 'critical',
state: 'dismissed',
state: 'DISMISSED',
reportType: 'SAST',
resolvedOnDefaultBranch: true,
location: {
......@@ -39,7 +39,7 @@ export const generateVulnerabilities = () => [
],
title: 'Vulnerability 1',
severity: 'high',
state: 'opened',
state: 'DETECTED',
reportType: 'DEPENDENCY_SCANNING',
location: {
file: 'src/main/java/com/gitlab/security_products/tests/App.java',
......@@ -58,7 +58,7 @@ export const generateVulnerabilities = () => [
identifiers: [],
title: 'Vulnerability 2',
severity: 'high',
state: 'opened',
state: 'DETECTED',
reportType: 'CUSTOM_SCANNER_WITHOUT_TRANSLATION',
location: {
file: 'src/main/java/com/gitlab/security_products/tests/App.java',
......@@ -74,7 +74,7 @@ export const generateVulnerabilities = () => [
id: 'id_3',
title: 'Vulnerability 3',
severity: 'high',
state: 'opened',
state: 'DETECTED',
location: {
file: 'yarn.lock',
},
......@@ -87,7 +87,7 @@ export const generateVulnerabilities = () => [
id: 'id_4',
title: 'Vulnerability 4',
severity: 'critical',
state: 'dismissed',
state: 'DISMISSED',
location: {},
project: {
nameWithNamespace: 'Administrator / Security reports',
......
ee/spec/frontend/security_dashboard/components/selection_summary_spec.js
View file @ 9f17a8b9
......@@ -2,7 +2,7 @@ import { mount } from '@vue/test-utils';
import SelectionSummary from 'ee/security_dashboard/components/selection_summary.vue';
import { GlFormSelect, GlButton } from '@gitlab/ui';
import waitForPromises from 'helpers/wait_for_promises';
import { deprecatedCreateFlash as createFlash } from '~/flash';
import createFlash from '~/flash';
import toast from '~/vue_shared/plugins/global_toast';
jest.mock('~/flash');
......@@ -25,11 +25,7 @@ describe('Selection Summary component', () => {
const dismissButton = () => wrapper.find(GlButton);
const dismissMessage = () => wrapper.find({ ref: 'dismiss-message' });
const formSelect = () => wrapper.find(GlFormSelect);
const createComponent = ({ props = {}, data = defaultData, mocks = defaultMocks }) => {
if (wrapper) {
throw new Error('Please avoid recreating components in the same spec');
}
const createComponent = ({ props = {}, data = defaultData, mocks = defaultMocks } = {}) => {
spyMutate = mocks.$apollo.mutate;
wrapper = mount(SelectionSummary, {
mocks: {
......@@ -63,7 +59,7 @@ describe('Selection Summary component', () => {
expect(dismissButton().attributes('disabled')).toBe('disabled');
});
it('should have the button enabled if a vulnerability is selected and an option is selected', () => {
it('should have the button enabled if a vulnerability is selected and an option is selected', async () => {
expect(wrapper.vm.dismissalReason).toBe(null);
expect(wrapper.findAll('option')).toHaveLength(4);
formSelect()
......@@ -71,15 +67,15 @@ describe('Selection Summary component', () => {
.at(1)
.setSelected();
formSelect().trigger('change');
return wrapper.vm.$nextTick().then(() => {
expect(wrapper.vm.dismissalReason).toEqual(expect.any(String));
expect(dismissButton().attributes('disabled')).toBe(undefined);
});
await wrapper.vm.$nextTick();
expect(wrapper.vm.dismissalReason).toEqual(expect.any(String));
expect(dismissButton().attributes('disabled')).toBe(undefined);
});
});
});
describe('with 1 vulnerabilities selected', () => {
describe('with multiple vulnerabilities selected', () => {
beforeEach(() => {
createComponent({ props: { selectedVulnerabilities: [{ id: 'id_0' }, { id: 'id_1' }] } });
});
......@@ -93,10 +89,12 @@ describe('Selection Summary component', () => {
let mutateMock;
beforeEach(() => {
mutateMock = jest.fn().mockResolvedValue();
mutateMock = jest.fn(data =>
data.variables.id % 2 === 0 ? Promise.resolve() : Promise.reject(),
);
createComponent({
props: { selectedVulnerabilities: [{ id: 'id_0' }, { id: 'id_1' }] },
props: { selectedVulnerabilities: [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }, { id: 5 }] },
data: { dismissalReason: 'Will Not Fix' },
mocks: { $apollo: { mutate: mutateMock } },
});
......@@ -104,31 +102,29 @@ describe('Selection Summary component', () => {
it('should make an API request for each vulnerability', () => {
dismissButton().trigger('submit');
expect(spyMutate).toHaveBeenCalledTimes(2);
expect(spyMutate).toHaveBeenCalledTimes(5);
});
it('should show toast with the right message if all calls were successful', () => {
it('should show toast with the right message for the successful calls', async () => {
dismissButton().trigger('submit');
return waitForPromises().then(() => {
expect(toast).toHaveBeenCalledWith('2 vulnerabilities dismissed');
});
await waitForPromises();
expect(toast).toHaveBeenCalledWith('2 vulnerabilities dismissed');
});
it('should show flash with the right message if some calls failed', () => {
mutateMock.mockRejectedValue();
it('should show flash with the right message for the failed calls', async () => {
dismissButton().trigger('submit');
return waitForPromises().then(() => {
expect(createFlash).toHaveBeenCalledWith(
'There was an error dismissing the vulnerabilities.',
'alert',
);
await waitForPromises();
expect(createFlash).toHaveBeenCalledWith({
message: 'There was an error dismissing 3 vulnerabilities. Please try again later.',
});
});
});
describe('when vulnerabilities are not selected', () => {
beforeEach(() => {
createComponent({});
createComponent();
});
it('should have the button disabled', () => {
......
ee/spec/frontend/security_dashboard/components/vulnerability_list_spec.js
View file @ 9f17a8b9
......@@ -12,6 +12,7 @@ import VulnerabilityList, {
import FiltersProducedNoResults from 'ee/security_dashboard/components/empty_states/filters_produced_no_results.vue';
import DashboardHasNoVulnerabilities from 'ee/security_dashboard/components/empty_states/dashboard_has_no_vulnerabilities.vue';
import { trimText } from 'helpers/text_helper';
import { capitalize } from 'lodash';
import { generateVulnerabilities, vulnerabilities } from './mock_data';
describe('Vulnerability list component', () => {
......@@ -19,7 +20,7 @@ describe('Vulnerability list component', () => {
let wrapper;
const createWrapper = ({ props = {}, listeners }) => {
const createWrapper = ({ props = {}, listeners } = {}) => {
return mount(VulnerabilityList, {
propsData: {
vulnerabilities: [],
......@@ -42,7 +43,9 @@ describe('Vulnerability list component', () => {
const findTable = () => wrapper.find(GlTable);
const findSortableColumn = () => wrapper.find('[aria-sort="descending"]');
const findCell = label => wrapper.find(`.js-${label}`);
const findRow = (index = 0) => wrapper.findAll('tbody tr').at(index);
const findRows = () => wrapper.findAll('tbody tr');
const findRow = (index = 0) => findRows().at(index);
const findRowById = id => wrapper.find(`tbody tr[data-pk="${id}"`);
const findIssuesBadge = () => wrapper.find(IssuesBadge);
const findRemediatedBadge = () => wrapper.find(RemediatedBadge);
const findSecurityScannerAlert = () => wrapper.find(SecurityScannerAlert);
......@@ -76,7 +79,7 @@ describe('Vulnerability list component', () => {
it('should correctly render the status', () => {
const cell = findCell('status');
expect(cell.text()).toBe(newVulnerabilities[0].state);
expect(cell.text()).toBe(capitalize(newVulnerabilities[0].state));
});
it('should correctly render the severity', () => {
......@@ -133,12 +136,11 @@ describe('Vulnerability list component', () => {
expect(findSelectionSummary().exists()).toBe(false);
});
it('should show the selection summary when a checkbox is selected', () => {
it('should show the selection summary when a checkbox is selected', async () => {
findDataCell('vulnerability-checkbox').setChecked(true);
await wrapper.vm.$nextTick();
return wrapper.vm.$nextTick().then(() => {
expect(findSelectionSummary().exists()).toBe(true);
});
expect(findSelectionSummary().exists()).toBe(true);
});
it('should sync selected vulnerabilities when the vulnerability list is updated', async () => {
......@@ -154,6 +156,18 @@ describe('Vulnerability list component', () => {
expect(findSelectionSummary().exists()).toBe(false);
});
it('should uncheck a selected vulnerability after the vulnerability is updated', async () => {
const checkbox = () => findDataCell('vulnerability-checkbox');
checkbox().setChecked(true);
expect(checkbox().element.checked).toBe(true);
await wrapper.vm.$nextTick();
findSelectionSummary().vm.$emit('vulnerability-updated', newVulnerabilities[0].id);
await wrapper.vm.$nextTick();
expect(checkbox().element.checked).toBe(false);
});
});
describe('when vulnerability selection is disabled', () => {
......@@ -371,7 +385,7 @@ describe('Vulnerability list component', () => {
describe('with no vulnerabilities when there are no filters', () => {
beforeEach(() => {
wrapper = createWrapper({});
wrapper = createWrapper();
});
it('should show the empty state', () => {
......@@ -398,6 +412,26 @@ describe('Vulnerability list component', () => {
});
});
describe('with vulnerabilities when there are filters', () => {
it.each`
state
${['DETECTED']}
${['DISMISSED']}
${[]}
${['DETECTED', 'DISMISSED']}
`('should only show vulnerabilities that match filter $state', state => {
wrapper = createWrapper({ props: { vulnerabilities, filters: { state } } });
const filteredVulnerabilities = vulnerabilities.filter(x =>
state.length ? state.includes(x.state) : true,
);
expect(findRows().length).toBe(filteredVulnerabilities.length);
filteredVulnerabilities.forEach(vulnerability => {
expect(findRowById(vulnerability.id).exists()).toBe(true);
});
});
});
describe('security scanner alerts', () => {
describe.each`
available | enabled | pipelineRun | expectAlertShown
......@@ -492,7 +526,7 @@ describe('Vulnerability list component', () => {
describe('when does not have a sort-changed listener defined', () => {
beforeEach(() => {
wrapper = createWrapper({});
wrapper = createWrapper();
});
it('is not sortable', () => {
......
locale/gitlab.pot
View file @ 9f17a8b9
......@@ -23027,6 +23027,11 @@ msgstr ""
msgid "SecurityReports|There was an error deleting the comment."
msgstr ""
msgid "SecurityReports|There was an error dismissing %d vulnerability. Please try again later."
msgid_plural "SecurityReports|There was an error dismissing %d vulnerabilities. Please try again later."
msgstr[0] ""
msgstr[1] ""
msgid "SecurityReports|There was an error dismissing the vulnerabilities."
msgstr ""
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7