Commit ccbea94c authored by Mark Florian's avatar Mark Florian

Never hide vulnerability modal footer

This ensures that the vulnerability modal always has an actions
area/footer, even if the vulnerability it's displaying has been resolved
(which can only happen within MRs).

Therefore, this should only have a visual effect on vulnerabilities
fixed within a particular MR, when viewing that MR. The vulnerability
modal is used elsewhere, for instance the Project Security Dashboard,
but in those other places, vulnerabilities can't be "fixed", and so this
should have no effect there.

Before this change, fixed vulnerabilities couldn't be interacted with at
all, since their actions footer wasn't displayed. This meant that the
user could not:
 - dismiss the vulnerability
 - create an issue for the vulnerability
 - create a merge request to fix the vulnerability
 - download a patch to fix the vulnerability

All of these restrictions still make sense, I think. The problem was
that they weren't allowed implicitly before, whereas now they're
disallowed explicitly.

Some more details about the changes:
 - The modal determines what to display based on a handful of `can*`
   props, e.g., `canDismissVulnerability`. These are simply permissions
   about vulnerabilities in general for the current user, and not
   specific to any given vulnerability. Since it's not meaningful to
   dismiss a resolved vulnerability, these permissions have been
   augmented to consider the current status of the vulnerability, via
   the new `can*ThisVulnerability` computed properties.
 - An unused computed prop `renderSolutionCard` was removed. Some test
   code was cleaned up around the `SolutionCard` (preferring finding
   elements via refs and component classes over `js-*` classes).
 - Added/clarified some tests around when and why the modal rendered the
   footer. Previously, permissions were conflated with simply the
   existence of the footer.

See https://gitlab.com/gitlab-org/gitlab/issues/32767.
parent d46862fa
...@@ -57,14 +57,27 @@ export default { ...@@ -57,14 +57,27 @@ export default {
dismissalCommentErrorMessage: '', dismissalCommentErrorMessage: '',
}), }),
computed: { computed: {
canDownloadPatch() { canCreateIssueForThisVulnerability() {
return Boolean(!this.isResolved && !this.vulnerability.hasIssue && this.canCreateIssue);
},
canCreateMergeRequestForThisVulnerability() {
return Boolean(!this.isResolved && !this.vulnerability.hasMergeRequest && this.remediation);
},
canDismissThisVulnerability() {
return Boolean(!this.isResolved && this.canDismissVulnerability);
},
canDownloadPatchForThisVulnerability() {
const remediationDiff = this.remediation && this.remediation.diff; const remediationDiff = this.remediation && this.remediation.diff;
return Boolean( return Boolean(
!this.isResolved &&
remediationDiff && remediationDiff &&
remediationDiff.length > 0 && remediationDiff.length > 0 &&
(!this.vulnerability.hasMergeRequest && this.remediation), (!this.vulnerability.hasMergeRequest && this.remediation),
); );
}, },
isResolved() {
return Boolean(this.modal.isResolved);
},
hasRemediation() { hasRemediation() {
return Boolean(this.remediation); return Boolean(this.remediation);
}, },
...@@ -87,12 +100,6 @@ export default { ...@@ -87,12 +100,6 @@ export default {
this.vulnerability && this.vulnerability.remediations && this.vulnerability.remediations[0] this.vulnerability && this.vulnerability.remediations && this.vulnerability.remediations[0]
); );
}, },
renderSolutionCard() {
return this.solution || this.remediation;
},
shouldRenderFooterSection() {
return !this.modal.isResolved && (this.canCreateIssue || this.canDismissVulnerability);
},
vulnerability() { vulnerability() {
return this.modal.vulnerability; return this.modal.vulnerability;
}, },
...@@ -194,7 +201,6 @@ export default { ...@@ -194,7 +201,6 @@ export default {
<modal <modal
id="modal-mrwidget-security-issue" id="modal-mrwidget-security-issue"
:header-title-text="modal.title" :header-title-text="modal.title"
:class="{ 'modal-hide-footer': !shouldRenderFooterSection }"
data-qa-selector="vulnerability_modal_content" data-qa-selector="vulnerability_modal_content"
class="modal-security-report-dast" class="modal-security-report-dast"
> >
...@@ -206,7 +212,7 @@ export default { ...@@ -206,7 +212,7 @@ export default {
:remediation="remediation" :remediation="remediation"
:has-mr="vulnerability.hasMergeRequest" :has-mr="vulnerability.hasMergeRequest"
:has-remediation="hasRemediation" :has-remediation="hasRemediation"
:has-download="canDownloadPatch" :has-download="canDownloadPatchForThisVulnerability"
:vulnerability-feedback-help-path="vulnerabilityFeedbackHelpPath" :vulnerability-feedback-help-path="vulnerabilityFeedbackHelpPath"
/> />
...@@ -269,14 +275,15 @@ export default { ...@@ -269,14 +275,15 @@ export default {
@cancel="$emit('closeDismissalCommentBox')" @cancel="$emit('closeDismissalCommentBox')"
/> />
<modal-footer <modal-footer
v-else-if="shouldRenderFooterSection" v-else
ref="footer"
:modal="modal" :modal="modal"
:vulnerability="vulnerability" :vulnerability="vulnerability"
:disabled="modal.isShowingDeleteButtons" :disabled="modal.isShowingDeleteButtons"
:can-create-issue="Boolean(!vulnerability.hasIssue && canCreateIssue)" :can-create-issue="canCreateIssueForThisVulnerability"
:can-create-merge-request="Boolean(!vulnerability.hasMergeRequest && remediation)" :can-create-merge-request="canCreateMergeRequestForThisVulnerability"
:can-download-patch="canDownloadPatch" :can-download-patch="canDownloadPatchForThisVulnerability"
:can-dismiss-vulnerability="canDismissVulnerability" :can-dismiss-vulnerability="canDismissThisVulnerability"
:is-dismissed="vulnerability.isDismissed" :is-dismissed="vulnerability.isDismissed"
@createMergeRequest="$emit('createMergeRequest')" @createMergeRequest="$emit('createMergeRequest')"
@createNewIssue="$emit('createNewIssue')" @createNewIssue="$emit('createNewIssue')"
......
...@@ -67,7 +67,7 @@ export default { ...@@ -67,7 +67,7 @@ export default {
}; };
</script> </script>
<template> <template>
<div class="card js-solution-card my-4"> <div class="card my-4">
<div v-if="solutionText" class="card-body d-flex align-items-center"> <div v-if="solutionText" class="card-body d-flex align-items-center">
<div class="col-2 d-flex align-items-center pl-0"> <div class="col-2 d-flex align-items-center pl-0">
<div class="circle-icon-container" aria-hidden="true"><icon name="bulb" /></div> <div class="circle-icon-container" aria-hidden="true"><icon name="bulb" /></div>
......
---
title: Show actions area for fixed vulnerabilities in merge requests
merge_request: 20867
author:
type: fixed
import Vue from 'vue'; import Vue from 'vue';
import component from 'ee/vue_shared/security_reports/components/modal.vue'; import component from 'ee/vue_shared/security_reports/components/modal.vue';
import createState from 'ee/vue_shared/security_reports/store/state'; import createState from 'ee/vue_shared/security_reports/store/state';
import SolutionCard from 'ee/vue_shared/security_reports/components/solution_card.vue';
import { mount, shallowMount } from '@vue/test-utils'; import { mount, shallowMount } from '@vue/test-utils';
describe('Security Reports modal', () => { describe('Security Reports modal', () => {
...@@ -66,8 +67,8 @@ describe('Security Reports modal', () => { ...@@ -66,8 +67,8 @@ describe('Security Reports modal', () => {
mountComponent({ propsData }, mount); mountComponent({ propsData }, mount);
}); });
it('renders the footer', () => { it('allows the vulnerability to be dismissed', () => {
expect(wrapper.classes('modal-hide-footer')).toBe(false); expect(wrapper.find({ ref: 'footer' }).props('canDismissVulnerability')).toBe(true);
}); });
}); });
...@@ -137,6 +138,29 @@ describe('Security Reports modal', () => { ...@@ -137,6 +138,29 @@ describe('Security Reports modal', () => {
); );
}); });
}); });
describe('with a resolved issue', () => {
beforeEach(() => {
const propsData = {
modal: createState().modal,
canCreateIssue: true,
canCreateMergeRequest: true,
canDismissVulnerability: true,
};
propsData.modal.vulnerability.remediations = [{ diff: '123' }];
propsData.modal.isResolved = true;
mountComponent({ propsData });
});
it('disallows any actions in the footer', () => {
expect(wrapper.find({ ref: 'footer' }).props()).toMatchObject({
canCreateIssue: false,
canCreateMergeRequest: false,
canDownloadPatch: false,
canDismissVulnerability: false,
});
});
});
}); });
describe('without permissions', () => { describe('without permissions', () => {
...@@ -147,8 +171,13 @@ describe('Security Reports modal', () => { ...@@ -147,8 +171,13 @@ describe('Security Reports modal', () => {
mountComponent({ propsData }); mountComponent({ propsData });
}); });
it('does not display the footer', () => { it('disallows any actions in the footer', () => {
expect(wrapper.classes('modal-hide-footer')).toBe(true); expect(wrapper.find({ ref: 'footer' }).props()).toMatchObject({
canCreateIssue: false,
canCreateMergeRequest: false,
canDownloadPatch: false,
canDismissVulnerability: false,
});
}); });
}); });
...@@ -237,8 +266,13 @@ describe('Security Reports modal', () => { ...@@ -237,8 +266,13 @@ describe('Security Reports modal', () => {
mountComponent({ propsData }); mountComponent({ propsData });
}); });
it('does not display the footer', () => { it('disallows any actions in the footer', () => {
expect(wrapper.classes('modal-hide-footer')).toBe(true); expect(wrapper.find({ ref: 'footer' }).props()).toMatchObject({
canCreateIssue: false,
canCreateMergeRequest: false,
canDownloadPatch: false,
canDismissVulnerability: false,
});
}); });
}); });
...@@ -291,7 +325,7 @@ describe('Security Reports modal', () => { ...@@ -291,7 +325,7 @@ describe('Security Reports modal', () => {
propsData.modal.vulnerability.solution = solution; propsData.modal.vulnerability.solution = solution;
mountComponent({ propsData }, mount); mountComponent({ propsData }, mount);
const solutionCard = wrapper.find('.js-solution-card'); const solutionCard = wrapper.find(SolutionCard);
expect(solutionCard.exists()).toBe(true); expect(solutionCard.exists()).toBe(true);
expect(solutionCard.text()).toContain(solution); expect(solutionCard.text()).toContain(solution);
...@@ -303,13 +337,15 @@ describe('Security Reports modal', () => { ...@@ -303,13 +337,15 @@ describe('Security Reports modal', () => {
modal: createState().modal, modal: createState().modal,
}; };
const summary = 'Upgrade to 123'; const summary = 'Upgrade to 123';
propsData.modal.vulnerability.remediations = [{ summary }]; const diff = 'foo';
propsData.modal.vulnerability.remediations = [{ summary, diff }];
mountComponent({ propsData }, mount); mountComponent({ propsData }, mount);
const solutionCard = wrapper.find('.js-solution-card'); const solutionCard = wrapper.find(SolutionCard);
expect(solutionCard.exists()).toBe(true); expect(solutionCard.exists()).toBe(true);
expect(solutionCard.text()).toContain(summary); expect(solutionCard.text()).toContain(summary);
expect(solutionCard.props('hasDownload')).toBe(true);
expect(wrapper.contains('hr')).toBe(false); expect(wrapper.contains('hr')).toBe(false);
}); });
...@@ -319,7 +355,7 @@ describe('Security Reports modal', () => { ...@@ -319,7 +355,7 @@ describe('Security Reports modal', () => {
}; };
mountComponent({ propsData }, mount); mountComponent({ propsData }, mount);
const solutionCard = wrapper.find('.js-solution-card'); const solutionCard = wrapper.find(SolutionCard);
expect(solutionCard.exists()).toBe(true); expect(solutionCard.exists()).toBe(true);
expect(wrapper.contains('hr')).toBe(false); expect(wrapper.contains('hr')).toBe(false);
......
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