Revamp of the false positive history feature

[adiffpirate]

The old fp history code is kinda old and clunky, that's why I feel most people doesn't use it. Hopefully this PR will change that. Personally, I use it for the following workflow:

• Not use dedup;
• One engagement per branch;
• Reimport everything (to avoid a bunch of duplicates);
• Mark the same finding as false positive in the entire product regardless of which branch/engagement has found it (to avoid marking the same thing as fp over and over again).

But it should be generic enough to fit other use cases.

Anyway, this PR makes the following changes:

• [x] FP History now takes the deduplication algorithm into account in order to determine if a finding is false positive or not;
• [x] FP History now only works when deduplication is disabled (it doesn't make sense to use both);
• [x] The FP History method (do_false_positive_history at dojo/utils.py) retroactively mark all findings in the product as false positive if the finding passed as argument is (or is being marked as) a false positive;
• [x] New method at dojo/utils.py called match_finding_to_existing_findings that takes one finding as argument and returns a list of existing findings based on its deduplication algorithm and a custom filter (this method can return matched findings in three diferents scopes: product wide, engagement wide, test wide). I feel like this is a useful method to have as it can be used for multiple purposes (including replace the similar, but less versatile, match_new_finding_to_existing_finding method at dojo/importers/reimporter/utils.py).
• [ ] Retroactively (un)mark all findings in the product as false positive when editing finding.
• [x] Retroactively (un)mark all findings in the product as false positive when bulk editing findings.
• [ ] Unittests (missing tests for retroactive (un)mark when editing and when bulk editing)

[adiffpirate]

@valentijnscholten Sorry to disturb you, but what are your thoughts about the new match_finding_to_existing_findings method?

[valentijnscholten]

Not sure if I'm the best one to advise on the FP feature as I never understood the difference with deduplication. But I like the use case where you do NOT use deduplication, but still want false positives to be "synced" up across tests/engagements. But:

• Wouldn't the same use case maybe apply for "Out of Scope"(or Risk Accepter or future other statuses like "Fix in progress")?
• The algorithm is called "deduplication algorithm". but now we're using it for something else and while deduplication is (possibly) turned off. We already do this also with reimport, but it might become confusing, so maybe a rename is needed at some point? Also pinging @devGregA as he was the biggest sponsor/fan of the existing FP history feature :-)

[adiffpirate]

I think the same use case does apply to "Out of Scope", but "Risk Accepted" is more complicated. A false positive is a false positive regardless of where it was found, but I see cases where the same finding can be accepted in dev/qa but not in prod. I was thinking about writing a "Risk Acceptance History" feature after this one, but it seems more complicated than FP History (the RA History would need a custom check per engagement to enable/disable it).

Maybe we could make dedup more customizable, like give it the ability to duplicate only a few set of status (fp, out of scope, ra). I think this has the potential to eliminate the need of "history alike" features and the confusion of using dedup alg for multiple stuff, but honestly I don't know how time consuming this implementation is.

[devGregA]

Been looking and thinking about this a little bit. I'm not opposed to revamping. It's true FP has stopped functioning as intended. It now actually seems to do the exact opposite of what was it's intended functionality 😂 . I thought one specific call was a little over aggressive, but I can't find it right this second. If we're going to reintroduce FP rather than sunsetting it, I think it would make sense to label it as experimental. I know I am the slowest at giving feedback 😪 If you can give me to next Wednesday it would be appreciated.

@adiffpirate thank you for taking this on. There is a ton of work in this PR and it is greatly appreciated. If you're interested in taking on other enhancements the #defectdojo-dev channel is a great place to get early feedback although not in anyway required.

[devGregA]

Just a couple thoughts. Thinking out loud, not saying these have to be changed.

• Most smart features in Dojo are forward looking. I see the use case for retroactive application, but we may want to make that toggle-able
• I'm a little concerned that tuning the same areas of dedupe could impact FPH without a user knowing it. We recommended that users don't use both features at the same time generally, but we would probably just to want to really clearly document that.
• Originally FPH aimed to work across tools because how noisy specifically dynamic scanners. That usecase is mostly gone, and I like that your implementation is much cleaner. Maybe someday we can correlate unique ids from different scanners.

Let me do some more noodling and I'll come back with concrete things rather than just thinking out loud. Will also pull the code and play with it a little bit.

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[devGregA]

Sorry for the delay @adiffpirate. To confirm, I would request the following changes be made to be inline with the other smart features:

• [ ] Add an experimental label in the help text of the system setting.
• [ ] Add toggle for retroactive vs non-retroactive false positive treatment.

[adiffpirate]

@devGregA Thanks for the feedback and sorry for taking so long to respond it. So, about each item:

"Most smart features in Dojo are forward looking. I see the use case for retroactive application, but we may want to make that toggle-able" Makes sense, it has a considerable impact so it's better to be opt-in.

"I'm a little concerned that tuning the same areas of dedupe could impact FPH without a user knowing it. We recommended that users don't use both features at the same time generally, but we would probably just to want to really clearly document that." I share the same concern. My proposal is to make FPH mutually exclusive with dedup since I can't think of a reason to have both on. This is already implemented on the current code but it's not clear on the system settings page, will improve that.

"Originally FPH aimed to work across tools because how noisy specifically dynamic scanners. That usecase is mostly gone, and I like that your implementation is much cleaner. Maybe someday we can correlate unique ids from different scanners." How nice of you 😊 The goal of reusing dedup logic was to make the code more "familiar", simple and easier to maintain, although to be honest I'm not 100% satisfied with the current state, I still think we should use the match_finding_to_existing_findings method for dedup, import, reimport, FPH and any other feature that requires matching finding to existing findings, but this is for a different PR. And I also agree that would be neat if you could correlate unique ids, although I'm sure how we could do that hahaha

I guess that's it, I'm gonna resolve the conflicts and implement your feedback (hopefully this week). Thanks for time and attention 💛

[devGregA]

Thank you @adiffpirate! Looking forward to it.

[devGregA]

@adiffpirate are you still working on this?

[devGregA]

Closing due to lack or response. Please feel free to re-engage when/if you are able.

[adiffpirate]

Hi @devGregA. I'm so so sorry for the absence. I was very focused on my work and dealing with some personal issues the past months, so I was lacking the time/mental health needed to finish this. But I couldn't forgave myself if this PR just died and the feature was never implemented.

So we're back on track and the following items are now completed:

1. Resolved conflicts (the branch was rebased on top of dev)
2. Added setting to toggle the retroactive behaviour (which is off by default)
3. Added unit tests to make sure things work as expected when retroactive is disabled (unit tests for when retroactive is enabled already existed, so both scenarios are being covered)
4. Improved help text on system setting to state very cleary that this is a experimental feature, what it does and how it works

Is it possible to reopen this PR or should I create a new one?

I believe that the current state is deliverable, although the unit tests for retroactive fp history when editing/bulk-editing are still missing. You think it's ok if we added these later?

[adiffpirate]

Hi team, I've rebased the branch on top of dev and finally implemented the missing tests. After a really long time (sorry about that :sweat_smile:) I believe this feature is 100% completed.

I saw that in the mean time Defect Dojo's stopped accepting new features, but I don't know if this falls in the same category since it's a revamp of an existing feature. What you guys think?