Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add evidence tied to reports #365

Closed

Conversation

ColonelThirtyTwo
Copy link
Collaborator

@ColonelThirtyTwo ColonelThirtyTwo commented Nov 22, 2023

Issue

None

Description of the Change

Add ability to create evidence on reports rather than findings.

Allows users to create evidence not tide to findings and used in the top level of a report template, which is a frequent user request.

This adds a foreign key to the reports on the Evidence model, requiring that either that one is set or the key to a finding is set, but not both. This is enforced with a database CHECK clause.

This also updates the HTML templates and views to support the new evidence functionality. A table of evidence is now shown on the report, with an Add button. The Add Evidence pages and modals now have an extra path parameter to distinguish between the "parent" of the evidence. Report-level evidence is serialized to templates under the evidence field at the top level.

Alternate Designs

Instead of two foreign keys, one of which can be set at a time, Django's GenericForeignKey from the contenttypes module could be used. This may be worth pursuing in the future if we want Evidences to be attached to a wider variety of objects, but I decided not to go with something overly generic for now.

The "parent type" path parameter to the evidence creation views is a plain string, which isn't restricted at the type level to being either "finding" or "report". A Python Enum would help restrict it a bit more, but Django does not support enum path parameters without additional extensions.

To have "global evidence" without this PR, it may be possible to add a finding whose sole purpose is to hold the "global evidence" and hide that finding in reports, either by name or via a custom field once those are merged. It's awkward, but it's possible without needing new code.

Possible Drawbacks

Extra complexity added to the Evidence model.

Verification Process

I've exercised the UI for creating evidences on findings and reports, as well as cloning reports. I've also extended the test cases to test evidences tied to reports.

Release Notes

  • Support for creating evidence tied to reports

Fixes lint issue about set_upload_destination, a regular method,
referenced via a class. Can't use `@staticmethod` because it's not
serializable and won't work with migrations.
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (dacc579) 92.27% compared to head (d1ec2e5) 92.26%.

Files Patch % Lines
ghostwriter/reporting/views.py 79.16% 10 Missing ⚠️
ghostwriter/reporting/tests/test_forms.py 92.00% 2 Missing ⚠️
ghostwriter/reporting/tests/test_views.py 98.41% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
- Coverage   92.27%   92.26%   -0.01%     
==========================================
  Files         255      256       +1     
  Lines       15996    16106     +110     
==========================================
+ Hits        14760    14861     +101     
- Misses       1236     1245       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ColonelThirtyTwo
Copy link
Collaborator Author

Merged into the v4-1-dev branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant