Skip to content

Commit eced864

Browse files
authored
Risk Exceptions: Add/Remove notes when finding is added/removed from risk exception (#10934)
* Risk Exceptions: Add/Remove notes when finding is added/removed from risk exception * Fix Flake8 * Correct tests * Add user ID to finding note * use jira user
1 parent c1e0d50 commit eced864

6 files changed

Lines changed: 71 additions & 25 deletions

File tree

dojo/api_v2/serializers.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1468,7 +1468,8 @@ class RiskAcceptanceSerializer(serializers.ModelSerializer):
14681468

14691469
def create(self, validated_data):
14701470
instance = super().create(validated_data)
1471-
add_findings_to_risk_acceptance(instance, instance.accepted_findings.all())
1471+
user = getattr(self.context.get("request", None), "user", None)
1472+
add_findings_to_risk_acceptance(user, instance, instance.accepted_findings.all())
14721473
return instance
14731474

14741475
def update(self, instance, validated_data):
@@ -1482,11 +1483,12 @@ def update(self, instance, validated_data):
14821483
findings_to_remove = Finding.objects.filter(id__in=[x.id for x in findings_to_remove])
14831484
# Make the update in the database
14841485
instance = super().update(instance, validated_data)
1486+
user = getattr(self.context.get("request", None), "user", None)
14851487
# Add the new findings
1486-
add_findings_to_risk_acceptance(instance, findings_to_add)
1488+
add_findings_to_risk_acceptance(user, instance, findings_to_add)
14871489
# Remove the ones that were not present in the payload
14881490
for finding in findings_to_remove:
1489-
remove_finding_from_risk_acceptance(instance, finding)
1491+
remove_finding_from_risk_acceptance(user, instance, finding)
14901492
return instance
14911493

14921494
@extend_schema_field(serializers.CharField())

dojo/api_v2/views.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -654,7 +654,7 @@ def destroy(self, request, pk=None):
654654
instance = self.get_object()
655655
# Remove any findings on the risk acceptance
656656
for finding in instance.accepted_findings.all():
657-
remove_finding_from_risk_acceptance(instance, finding)
657+
remove_finding_from_risk_acceptance(request.user, instance, finding)
658658
# return the response of the object being deleted
659659
return super().destroy(request, pk=pk)
660660

dojo/engagement/views.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1250,7 +1250,7 @@ def add_risk_acceptance(request, eid, fid=None):
12501250

12511251
findings = form.cleaned_data["accepted_findings"]
12521252

1253-
risk_acceptance = ra_helper.add_findings_to_risk_acceptance(risk_acceptance, findings)
1253+
risk_acceptance = ra_helper.add_findings_to_risk_acceptance(request.user, risk_acceptance, findings)
12541254

12551255
messages.add_message(
12561256
request,
@@ -1360,7 +1360,7 @@ def view_edit_risk_acceptance(request, eid, raid, edit_mode=False):
13601360
finding = get_object_or_404(
13611361
Finding, pk=request.POST["remove_finding_id"])
13621362

1363-
ra_helper.remove_finding_from_risk_acceptance(risk_acceptance, finding)
1363+
ra_helper.remove_finding_from_risk_acceptance(request.user, risk_acceptance, finding)
13641364

13651365
messages.add_message(
13661366
request,
@@ -1391,7 +1391,7 @@ def view_edit_risk_acceptance(request, eid, raid, edit_mode=False):
13911391
if not errors:
13921392
findings = add_findings_form.cleaned_data["accepted_findings"]
13931393

1394-
ra_helper.add_findings_to_risk_acceptance(risk_acceptance, findings)
1394+
ra_helper.add_findings_to_risk_acceptance(request.user, risk_acceptance, findings)
13951395

13961396
messages.add_message(
13971397
request,

dojo/finding/views.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -991,10 +991,10 @@ def process_finding_form(self, request: HttpRequest, finding: Finding, context:
991991
# Handle risk exception related things
992992
if "risk_accepted" in context["form"].cleaned_data and context["form"]["risk_accepted"].value():
993993
if new_finding.test.engagement.product.enable_simple_risk_acceptance:
994-
ra_helper.simple_risk_accept(new_finding, perform_save=False)
994+
ra_helper.simple_risk_accept(request.user, new_finding, perform_save=False)
995995
else:
996996
if new_finding.risk_accepted:
997-
ra_helper.risk_unaccept(new_finding, perform_save=False)
997+
ra_helper.risk_unaccept(request.user, new_finding, perform_save=False)
998998
# Save and add new endpoints
999999
finding_helper.add_endpoints(new_finding, context["form"])
10001000
# Remove unrelated endpoints
@@ -1270,7 +1270,7 @@ def close_finding(request, fid):
12701270
status.last_modified = timezone.now()
12711271
status.save()
12721272
# Clear the risk acceptance, if present
1273-
ra_helper.risk_unaccept(finding)
1273+
ra_helper.risk_unaccept(request.user, finding)
12741274

12751275
# Manage the jira status changes
12761276
push_to_jira = False
@@ -1446,7 +1446,7 @@ def reopen_finding(request, fid):
14461446
status.last_modified = timezone.now()
14471447
status.save()
14481448
# Clear the risk acceptance, if present
1449-
ra_helper.risk_unaccept(finding)
1449+
ra_helper.risk_unaccept(request.user, finding)
14501450

14511451
# Manage the jira status changes
14521452
push_to_jira = False
@@ -1626,7 +1626,7 @@ def simple_risk_accept(request, fid):
16261626
if not finding.test.engagement.product.enable_simple_risk_acceptance:
16271627
raise PermissionDenied
16281628

1629-
ra_helper.simple_risk_accept(finding)
1629+
ra_helper.simple_risk_accept(request.user, finding)
16301630

16311631
messages.add_message(
16321632
request, messages.WARNING, "Finding risk accepted.", extra_tags="alert-success",
@@ -1640,7 +1640,7 @@ def simple_risk_accept(request, fid):
16401640
@user_is_authorized(Finding, Permissions.Risk_Acceptance, "fid")
16411641
def risk_unaccept(request, fid):
16421642
finding = get_object_or_404(Finding, id=fid)
1643-
ra_helper.risk_unaccept(finding)
1643+
ra_helper.risk_unaccept(request.user, finding)
16441644

16451645
messages.add_message(
16461646
request,
@@ -2851,9 +2851,9 @@ def finding_bulk_update_all(request, pid=None):
28512851
):
28522852
skipped_risk_accept_count += 1
28532853
else:
2854-
ra_helper.simple_risk_accept(finding)
2854+
ra_helper.simple_risk_accept(request.user, finding)
28552855
elif form.cleaned_data["risk_unaccept"]:
2856-
ra_helper.risk_unaccept(finding)
2856+
ra_helper.risk_unaccept(request.user, finding)
28572857

28582858
for prod in prods:
28592859
calculate_grade(prod)

dojo/jira_link/helper.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1623,7 +1623,7 @@ def process_resolution_from_jira(finding, resolution_id, resolution_name, assign
16231623
owner=finding.reporter,
16241624
)
16251625
finding.test.engagement.risk_acceptance.add(ra)
1626-
ra_helper.add_findings_to_risk_acceptance(ra, [finding])
1626+
ra_helper.add_findings_to_risk_acceptance(User.objects.get_or_create(username="JIRA")[0], ra, [finding])
16271627
status_changed = True
16281628
elif jira_instance and resolution_name in jira_instance.false_positive_resolutions:
16291629
if not finding.false_p:
@@ -1633,7 +1633,7 @@ def process_resolution_from_jira(finding, resolution_id, resolution_name, assign
16331633
finding.mitigated = None
16341634
finding.is_mitigated = False
16351635
finding.false_p = True
1636-
ra_helper.risk_unaccept(finding)
1636+
ra_helper.risk_unaccept(User.objects.get_or_create(username="JIRA")[0], finding)
16371637
status_changed = True
16381638
else:
16391639
# Mitigated by default as before
@@ -1645,7 +1645,7 @@ def process_resolution_from_jira(finding, resolution_id, resolution_name, assign
16451645
finding.mitigated_by, _created = User.objects.get_or_create(username="JIRA")
16461646
finding.endpoints.clear()
16471647
finding.false_p = False
1648-
ra_helper.risk_unaccept(finding)
1648+
ra_helper.risk_unaccept(User.objects.get_or_create(username="JIRA")[0], finding)
16491649
status_changed = True
16501650
else:
16511651
if not finding.active:
@@ -1655,7 +1655,7 @@ def process_resolution_from_jira(finding, resolution_id, resolution_name, assign
16551655
finding.mitigated = None
16561656
finding.is_mitigated = False
16571657
finding.false_p = False
1658-
ra_helper.risk_unaccept(finding)
1658+
ra_helper.risk_unaccept(User.objects.get_or_create(username="JIRA")[0], finding)
16591659
status_changed = True
16601660

16611661
# for findings in a group, there is no jira_issue attached to the finding

dojo/risk_acceptance/helper.py

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import logging
2+
from contextlib import suppress
23

34
from dateutil.relativedelta import relativedelta
45
from django.core.exceptions import PermissionDenied
@@ -8,7 +9,7 @@
89
import dojo.jira_link.helper as jira_helper
910
from dojo.celery import app
1011
from dojo.jira_link.helper import escape_for_jira
11-
from dojo.models import Finding, Risk_Acceptance, System_Settings
12+
from dojo.models import Dojo_User, Finding, Notes, Risk_Acceptance, System_Settings
1213
from dojo.notifications.helper import create_notification
1314
from dojo.utils import get_full_url, get_system_setting
1415

@@ -102,7 +103,7 @@ def delete(eng, risk_acceptance):
102103
risk_acceptance.delete()
103104

104105

105-
def remove_finding_from_risk_acceptance(risk_acceptance, finding):
106+
def remove_finding_from_risk_acceptance(user: Dojo_User, risk_acceptance: Risk_Acceptance, finding: Finding) -> None:
106107
logger.debug("removing finding %i from risk acceptance %i", finding.id, risk_acceptance.id)
107108
risk_acceptance.accepted_findings.remove(finding)
108109
finding.active = True
@@ -112,9 +113,20 @@ def remove_finding_from_risk_acceptance(risk_acceptance, finding):
112113
finding.save(dedupe_option=False)
113114
# best effort jira integration, no status changes
114115
post_jira_comments(risk_acceptance, [finding], unaccepted_message_creator)
116+
# Add a note to reflect that the finding was removed from the risk acceptance
117+
if user is not None:
118+
finding.notes.add(Notes.objects.create(
119+
entry=(
120+
f"{Dojo_User.generate_full_name(user)} ({user.id}) removed this finding from the risk acceptance: "
121+
f'"{risk_acceptance.name}" ({get_view_risk_acceptance(risk_acceptance)})'
122+
),
123+
author=user,
124+
))
115125

126+
return None
116127

117-
def add_findings_to_risk_acceptance(risk_acceptance, findings):
128+
129+
def add_findings_to_risk_acceptance(user: Dojo_User, risk_acceptance: Risk_Acceptance, findings: list[Finding]) -> None:
118130
for finding in findings:
119131
if not finding.duplicate or finding.risk_accepted:
120132
finding.active = False
@@ -123,11 +135,21 @@ def add_findings_to_risk_acceptance(risk_acceptance, findings):
123135
# Update any endpoint statuses on each of the findings
124136
update_endpoint_statuses(finding, accept_risk=True)
125137
risk_acceptance.accepted_findings.add(finding)
138+
# Add a note to reflect that the finding was removed from the risk acceptance
139+
if user is not None:
140+
finding.notes.add(Notes.objects.create(
141+
entry=(
142+
f"{Dojo_User.generate_full_name(user)} ({user.id}) added this finding to the risk acceptance: "
143+
f'"{risk_acceptance.name}" ({get_view_risk_acceptance(risk_acceptance)})'
144+
),
145+
author=user,
146+
))
126147
risk_acceptance.save()
127-
128148
# best effort jira integration, no status changes
129149
post_jira_comments(risk_acceptance, findings, accepted_message_creator)
130150

151+
return None
152+
131153

132154
@app.task
133155
def expiration_handler(*args, **kwargs):
@@ -174,6 +196,16 @@ def expiration_handler(*args, **kwargs):
174196
risk_acceptance.save()
175197

176198

199+
def get_view_risk_acceptance(risk_acceptance: Risk_Acceptance) -> str:
200+
"""Return the full qualified URL of the view risk acceptance page."""
201+
# Suppressing this error because it does not happen under most circumstances that a risk acceptance does not have engagement
202+
with suppress(AttributeError):
203+
get_full_url(
204+
reverse("view_risk_acceptance", args=(risk_acceptance.engagement.id, risk_acceptance.id)),
205+
)
206+
return ""
207+
208+
177209
def expiration_message_creator(risk_acceptance, heads_up_days=0):
178210
return "Risk acceptance [({})|{}] with {} findings has expired".format(
179211
escape_for_jira(risk_acceptance.name),
@@ -267,7 +299,7 @@ def prefetch_for_expiration(risk_acceptances):
267299
)
268300

269301

270-
def simple_risk_accept(finding, perform_save=True):
302+
def simple_risk_accept(user: Dojo_User, finding: Finding, perform_save=True) -> None:
271303
if not finding.test.engagement.product.enable_simple_risk_acceptance:
272304
raise PermissionDenied
273305

@@ -282,9 +314,15 @@ def simple_risk_accept(finding, perform_save=True):
282314
# post_jira_comment might reload from database so see unaccepted finding. but the comment
283315
# only contains some text so that's ok
284316
post_jira_comment(finding, accepted_message_creator)
317+
# Add a note to reflect that the finding was removed from the risk acceptance
318+
if user is not None:
319+
finding.notes.add(Notes.objects.create(
320+
entry=(f"{Dojo_User.generate_full_name(user)} ({user.id}) has risk accepted this finding"),
321+
author=user,
322+
))
285323

286324

287-
def risk_unaccept(finding, perform_save=True):
325+
def risk_unaccept(user: Dojo_User, finding: Finding, perform_save=True) -> None:
288326
logger.debug("unaccepting finding %i:%s if it is currently risk accepted", finding.id, finding)
289327
if finding.risk_accepted:
290328
logger.debug("unaccepting finding %i:%s", finding.id, finding)
@@ -302,6 +340,12 @@ def risk_unaccept(finding, perform_save=True):
302340
# post_jira_comment might reload from database so see unaccepted finding. but the comment
303341
# only contains some text so that's ok
304342
post_jira_comment(finding, unaccepted_message_creator)
343+
# Add a note to reflect that the finding was removed from the risk acceptance
344+
if user is not None:
345+
finding.notes.add(Notes.objects.create(
346+
entry=(f"{Dojo_User.generate_full_name(user)} ({user.id}) removed a risk exception from this finding"),
347+
author=user,
348+
))
305349

306350

307351
def remove_from_any_risk_acceptance(finding):

0 commit comments

Comments
 (0)