[FIX] Edit Finding's mitigated time (EDITABLE_MITIGATED_DATA)

[X0x1RG9f]

When using "EDITABLE_MITIGATED_DATA = True", several issues are present within the DefectDojo application:

• Closing a finding with a mitigation date in the past close the finding badly:
  • The "active" parameter is not set to false and thus, Finding does not appear at ALL in the "Closed Finding" tab 👎.
  • The asset(s) linked to the finding is / are not affected and remain vulnerable, even if Finding is closed 👎.
• The form is added within the "Edit Finding" form, whereas there is a dedicated "Close Finding Form". Not very logical.
• Calendar Box used is not the standard DefectDojo one.

The Fix contains:

• Close date is added within the "Close Finding" formulary and removed from "Edit Finding", which is much more logical.
• Calendar is set to the default one used in the whole app.
• What do you mean by "a findings assets"?
• Closing a finding with a date in the past sets also "active" to "False" and closes the linked asset. Nothing touched here, as we are now using functions of the "Close Finding" form !

[valentijnscholten]

Thanks for the PR.

Things I like:

• Adding these fields to the close finding form makes sense;
• Using the normal date picker makes sense. I think the original author or others were in favor to use this new one though;

Things I didn't like or don't agree with:

• I think active==False is the correct state for a closed / mitigated finding?
• The idea is that the mitigation date can be edited/corrected after the findings was closed, that's it is and probably should remain on the edit finding form
• Please remove code instead of commenting it out (but in this case it probably should remain)
• If you remove the splitdatetimefield from the form, you should probably remove that code from the codebase alltogether as it's not used elsewhere
• You may need to check/add/update the unit tests to cover the check logic
• Please base your PR against dev

[X0x1RG9f]

Hi,

Thanks for answer:

• Totally agree, active==False for me also is the correct state for a mitigated one. But in the actual usage "Edit Finding" setting a mitigation past date remain active unchanged. Thus, Finding is mitigated but does not appear in "Closed Findings". Only way I found was closing it first and then Edit again and change Mitigation Date (2 steps).
• Agree also (just did not though about it), as Closed Finding could also be edited, I will put back code but with official pick date form
• Code uncommented then as remaining in "Edit Finding" Form
• splitdatetimefield removed from code, including html template that only this class was using.
• Will have a look at the unit test, but not very familiar with so may take a while
• PR (all I made) changed from master to dev, sorry for that !

[X0x1RG9f]

Tested lot of scenarios, everything seems to work fine by now except bugs that already existed within the "Finding Edit" form. However, I don't know the framework that much and my skills do not help to resolve:

• When "active" is unchecked and Mitigated Date is entered, vulnerability does not appear in "Closed Findings" page, because in the database "is_mitigated" remains False.
• When "active" is unchecked and Mitigated Date is entered, linked assets remains unchanged and active.

[valentijnscholten]

Thanks! Could you take a look at https://github.com/DefectDojo/django-DefectDojo/issues/6078 while you're at it?

[X0x1RG9f]

Just commited code part allowing the /api/v2/findings/{id}/close/ request with 2 parameters, is_mitigated and mitigated:

• Date is timezone.now() if none is provided or EDITABLE_MITIGATED_DATA is False
• Date is provided date or timezone.now() if EDITABLE_MITIGATED_DATA is true
• Corresponding Endpoint status are also updated

Tell me what you think about it when you have time to review.

[valentijnscholten]

Not sure, but when somebody edits a finding via the API, I don't think checks are made if the user is allowed to edit the mitigated date field? I think the finding helper prevents the fields from being modified, but this is a silent action I think? Not sure as there is no testcase for the API.

[X0x1RG9f]

Sorry, no time to check this week but I'll try on next one. For me CAN_MITIGATE_DATA was a general field (for all users or not) and then I checked if it was activated or not in order to update database. But maybe wrong ?

Will also check if I can add the mitigation date within the Edit Finding API call and not just Close call.

[github-actions[bot]]

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

[saldam72]

Hello team, please resolve conflicts

best, Sal

[saldam72]

Hello team, resolve conflict would be a nice gift for my 50th birthday (august 20th) :-)))

best, Sal

[Maffooch]

@X0x1RG9f please resolve conflicts for @saldam72 birthday 😂

[github-actions[bot]]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[Maffooch]

@X0x1RG9f thanks for fixing the conflicts so quickly! This is looking a lot better. The last thing from me would be some unit tests. I think this one would be a good starting place, but for findings instead

[X0x1RG9f]

Hello, I'm working on it to make unit test successful. For now, I tested and saw one bug I did not have before with JQuery. When modifying a vulnerability, there is now a JQuery issue and so, a lot of Issues on the webpage, with impossibility to validate the form ! I don't know what's happening, so i'll check and try to push something working by the end of the day

[X0x1RG9f]

@Maffooch @saldam72, should be okay by now :) Just simplified a little the code on the View and added permissions for the users list displayed in the menu. About the JQuery Error, I also reproduced the issue on a fresh install and on the https://demo.defectdojo.org, so I will open a different issue. Other issues are resolved.

[X0x1RG9f]

@Maffooch, @valentijnscholten, any idea why this remaining check is failing ? Have absolutely no idea on my side.

[mtesauro]

@X0x1RG9f Re-ran the tests and that failing one passed. GH runners aren't 100% so sometimes a re-run is needed.

[Maffooch]

@X0x1RG9f looks like some extra commits got into your PR

[github-actions[bot]]

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

[github-actions[bot]]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[devGregA]

@X0x1RG9f will review further after the release.