Add ability to change dismissal comment and reason for security findings
Why are we doing this work
We want to be able to add, edit, or remove dismissal comments for security findings. The idea is to use the existing securityFindingDismiss
with a comment, and it will change the comment if a dismissal already exists. We would need to change code in the Vulnerabilities::FindOrCreateFromSecurityFindingService
, specifically utilizing the update_state_for
method. I think Vulnerabilities::DismissService already handles this properly, but it needs to be tested.
Refinement analysis
The securityFindingDismiss
GraphQL mutation calls the Security::Findings::DismissService
to dismiss the finding. This service can be broken down into 2 parts:
- Create Dismissal Feedback
- Create and Dismiss the Vulnerability
Create Dismissal Feedback
Creating dismissal feedback is handled by the create_feedback
method. This method calls another service called ::VulnerabilityFeedback::CreateService
to create the feedback. This service creates 3 types of feedback: issue, merge request, and dismissal. The first thing this service does is find the existing dismissal feedback and update any attributes or initialize a dismissal feedback if none exists. Note that in both of these instances, nothing is saved yet for the feedback. This happens in the dismiss_existing_vulnerability
method.
NOTE: This feedback creation will be going away with the feedback deprecation work.
Create and Dismiss the Vulnerability
When the feedback is for a dismissal, the Security::Findings::DismissService
calls the dismiss_existing_vulnerability
method.
The dismiss_existing_vulnerability
method does the following:
- (line 142) If a vulnerability does not already exist for the finding:
- This service creates a vulnerability in the
dismissed
state using theVulnerabilities::FindOrCreateFromSecurityFindingService
.
- This service creates a vulnerability in the
- (line 144) If a vulnerability already exists for the finding:
- This service calls the
Vulnerabilities::DismissService
to dismiss the vulnerability.
- This service calls the
- (line 151) The dismissal feedback is saved if the feedback is a new feedback.
Changes Needed
This needs to be done in the same MR, otherwise we would update the comment in the feedback and not in the state transition, or vice versa.
-
::VulnerabilityFeedback::CreateService
- Save the feedback record if the comment and reason are updated. This can be done in the block that starts on line 151 where it's only saved if not persisted. The attributes are already being updated; they just need to be saved.
-
Vulnerabilities::FindOrCreateFromSecurityFindingService
- Modify the
update_state_for
method (line 45) to update the state transition if the comment and dismissal reason change.
- Modify the
Relevant links
Non-functional requirements
-
Testing: - Ensure existing feedback is updated
- Create a new state transition if data changed
Implementation plan
As mentioned above, these changes cannot be split into multiple MRs, otherwise we'll get a data inconsistency. I think a weight of 3 is appropriate here. The changes should not be too complex, but they do need to be made in multiple places in different ways, and cannot be split up.
-
backend ::VulnerabilityFeedback::CreateService
- Save the feedback record if the comment and reason are updated. This can be done in the block that starts on line 151 where it's only saved if not persisted. The attributes are already being updated; they just need to be saved.
-
backend Vulnerabilities::FindOrCreateFromSecurityFindingService
- Modify the
update_state_for
method (line 45) to update the state transition if the comment and dismissal reason change.
- Modify the
Verification steps
- Call the
securityFindingDismiss
mutation with a comment and dismissal reason.
mutation {
securityFindingDismiss(
input: { uuid: "<uuid>", comment: "Original comment", dismissalReason: ACCEPTABLE_RISK }
) {
uuid
}
}
- Use the dismissal data fields to check the comment and reason (in review)
{
project(fullPath: "<path>") {
id
pipeline(iid: "<pipeline iid>") {
securityReportFinding(uuid:<uuid>) {
nodes {
uuid
state
dismissedAt
dismissedBy {
name
}
dismissalReason
stateComment
}
}
}
}
}
- Call the
securityFindingDismiss
mutation with a new comment and dismissal reason.
mutation {
securityFindingDismiss(
input: { uuid: "<uuid>", comment: "This is a new comment", dismissalReason: MITIGATING_CONTROL }
) {
uuid
}
}
- Use the dismissal data fields to check the comment and reason (in review)
{
project(fullPath: "<path>") {
id
pipeline(iid: "<pipeline iid>") {
securityReportFinding(uuid:<uuid>) {
nodes {
uuid
state
dismissedAt
dismissedBy {
name
}
dismissalReason
stateComment
}
}
}
}
}