We need to be able to dismiss pipeline findings. There is a mutation vulnerabilityFindingDismiss, but this does not work for pipelines. The only ID available to security findings is the UUID. We need to accept the UUID of a finding to dismiss the vulnerability finding and/or the security finding. We also need to deprecate the use of id as a parameter. However, that deprecation should come in a separate issue.
Relevant links
N/A
Non-functional requirements
Documentation: Update GraphQL docs
Testing: Add relevant tests
Implementation plan
backend Add uuid as an input for the vulnerabilityFindingDismiss mutation
@dpisek, we've blocked this against another issue in &5629 (closed). Subashis said that it will be a lot easier to do if we wait for that.
We have borrowed a backend engineer to help us with &5629 (closed), so it shouldn't take too long.
I'll tentatively swap the blocker issue with this one (ie move #334266 to %15.1 and move this one to %15.2). I don't have an estimate at this stage as we're just getting started, so please treat this due date as low-confidence.
One problem with this we figured out on the backend issue refinement call:
There's a MR with a new security finding (this creates a security_finding)
Someone goes there and confirms that security finding
That action will "promote" that security finding into a Vulnerability
MR author fixes that security finding in the MR
MR gets merged
Now we have a Vulnerability record for a Vulnerability that was never on the master branch
Basically, some actions don't make a lot of sense when you fix the security finding in the same MR before it's actually merged. If you resolve/needs triage/confirm a security finding, and it gets fixed in the same MR then we're creating records for no reason
@matt_wilson Me and @jschafer agree that the lack of well-defined workflow on how vulnerabilities found on not-default-branches are supposed to work is kind of problematic when refining this stuff
@dpisek we did have a brief conversation about how to address this frontend wise (limiting actions to Dismiss when we're looking at security findings on a MR for example), @jschafer should create a GDoc in which we can collaborate on this since setting up a sync chat looks to be impossible
The Epic is about replacing the existing backend calls without affecting the user-facing feature. As currently written, this issue changes functionality. The way I understand it, this would be out-of-scope for the Epic.
We have a specific epic (&3430) to implement tracking of findings outside the default branch, which is sort of what is being proposed here.
As currently written, this issue changes functionality.
The reason why I suggested the additional states (on top of DISMISSED) is because they are currently supported by the PipelineSecurityReportFinding's state field.
But given that we currently only to dismiss a finding I think the proposal to only allow that action makes sense!
If it's unlikely that we ever support those other states on a finding, maybe we should change PipelineSecurityReportFinding.state's type to reflect that 🤔
maybe we should change PipelineSecurityReportFinding.state's type to reflect that
We should keep the state the way it is. Because although you cannot set the state through the pipeline, it can be read as any state. For example, for an existing Vulnerability, it could be confirmed, and it will show up in the pipeline for subsequent runs.
I'm thinking we can close down this issue as a won't do. Or we change the scope of the issue. Because of what you stated in the description:
I could not get it working by using a finding's UUID
The UUID is not properly used in our Vulnerability GraphQL API. If we add that in as an argument, we can then dismiss by UUID, which is what we probably should be doing.
We should keep the state the way it is. Because although you cannot set the state through the pipeline, it can be read as any state. For example, for an existing Vulnerability, it could be confirmed, and it will show up in the pipeline for subsequent runs.
@jschafer Thanks for the explanation, that makes sense!
And just as a reminder: Since I don't have enough domain knowledge, the backend issues I created are more placeholders and starting-points for discussions, than anything else.
But I am glad we got the ball rolling 🙂
does this sound like a reasonable change of scope for this issue?
@matt_wilson We should deprecate the use of id here as an input parameter in favor of uuid. I think we can get this is at the same time as the other deprecations.
@jschafer, thanks for finishing the refinement. Could you please write a deprecation issue for this and the other fields? We might as well write an issue for the removal - WDYT? Once you tag @matt_wilson he should have all the information needed to write the deprecation post.
@dpisek@jschafer@thiagocsf It looks like I stayed on PTO long enough that my input is no longer needed here 😄 If I missed something you still need my input on, please let me know.
And @jschafer I'll add the deprecation you suggest to my open "omnibus" MR where we've been collecting similar items. Just ping me in the deprecation issue when you get to it and I'll add it.
Jonathan Schaferchanged title from Add GraphQL mutations for changing a pipeline finding's status to Change vulnerabilityFindingDismiss mutation to accept uuid argument
changed title from Add GraphQL mutations for changing a pipeline finding's status to Change vulnerabilityFindingDismiss mutation to accept uuid argument
Change vulnerabilityFindingDismiss mutation to ... (!91923 - merged) will satisfy this issue. However, we still cannot dismiss a security finding with this route. The GraphQL only searches for Vulnerability Findings at the moment, and we will need to allow it to search for security findings. This should be a follow-on issue.