feat(RiskAcc-Eng): Clean RiskAcc-Eng mapping + Allow correct handling by API

[kiblik]

This PR is trying to fix "not the best" implementation of mapping between Engagements and RiskAcc.
Until now, it was set as ManyToManyField in Engagements, and from RiskAcc, it was just using the first possible match. This implementation is eventually a one-to-many relation, which should be solved via ForeignKey in RiskAcc.

[kiblik]

@mtesauro & @Maffooch, this PR will need more work. But I prepared an initial commit to open discussion: Was there any specific reason why this was implemented as ManyToManyField? It just consumes (a bit) more resources and performance, + adds more operations. Have I missed sth?

[valentijnscholten]

I think the initial idea was that a Risk Acceptance could apply to more than just one engagement. For example accept the risk of a CVE that occurs in multiple branches. I think via the UI you cannot actually do this, but it might be possible via the API?
There's a risk we kill someone's current use case, or our own future use cases. All in all changing it to a 1-to-many relationship doesn't really provide huge savings or performance gains, so I think it should be left as is.

[kiblik]

I think the initial idea was that a Risk Acceptance could apply to more than just one engagement. For example accept the risk of a CVE that occurs in multiple branches. I think via the UI you cannot actually do this, but it might be possible via the API? There's a risk we kill someone's current use case, or our own future use cases. All in all changing it to a 1-to-many relationship doesn't really provide huge savings or performance gains, so I think it should be left as is.

API would not be broken; there is a checker correctly (see below). In fact, API needs to be fixed - you can create RiskAcc (via POST /api/v2/risk_acceptance/), but it is not possible to map it to Eng (there is a missing endpoint for ManyToManyField) so it is not possible to see it in UI (which is bug).
Ok, performance overhead is probably not that large.
But still, it is good to keep the code clean and mainly to be consistent.

• If we want to have RiskAcc, which applies across multiple Eng
  • We should list them on a different page (not in <dd>/engagement/{id})
    

    django-DefectDojo/dojo/templates/dojo/view_eng.html

    Line 407 in 477583e

    {% for risk_acceptance in risks_accepted %}

  • We should not limit grouping accepted findings only to one Eng
    

    django-DefectDojo/dojo/api_v2/serializers.py

    Lines 1589 to 1594 in 477583e

    	def validate(self, data):
    	def validate_findings_have_same_engagement(finding_objects: list[Finding]):
    	engagements = finding_objects.values_list("test__engagement__id", flat=True).distinct().count()
    	if engagements > 1:
    	msg = "You are not permitted to add findings from multiple engagements"
    	raise PermissionDenied(msg)

• If we want to have n:1 mapping between RiskAcc:Eng, we should do it properly (as I'm trying to offer in this PR).

I'm not strictly to one or another option. Both might make sense. It is the reason why I didn't continue with the implementation, but raised the question.
I incline towards n:1 on Risk:Prod level. But if there was a reason in history to design it towards another model, I would like to ask for reasoning, and we can add it to the documentation so it will be available to users as well.

Regarding breaking the current setup - I do not think so because we would just extend current abilities (but please correct me if I'm wrong). It might not be true about future use cases. But it is the reason why I'm asking about original intentions.

[kiblik]

@mtesauro & @Maffooch, can we have your opinion on which way we should move forward? Thank you.

[dryrunsecurity]

This pull request introduces potential risks in database migration and model relationships, including null reference scenarios, incomplete migration logic, unintended data deletion through CASCADE strategy, and changes to data relationships that could impact existing data tracking between Engagement and Risk_Acceptance models.

💭 Unconfirmed Findings (4)

Vulnerability	Potential Null Reference Risk
Description	In database migration file, null foreign key could lead to unexpected behavior and might cause null reference scenarios if engagement relationship is assumed to always exist.

Vulnerability	Placeholder Migration with No Actual Implementation
Description	In migration file 0230, contains an empty function with no implementation, suggesting incomplete migration logic that might not perform intended data transformations.

Vulnerability	Potential Unintended Data Deletion Risk
Description	In migration file 0231, uses Django's CASCADE deletion strategy which means if an engagement is deleted, all associated risk acceptances will be automatically deleted, potentially causing unintended data loss.

Vulnerability	Potential Data Integrity Risk in Relationship Modeling
Description	In models.py, changes database relationship between Engagement and Risk_Acceptance models by removing ManyToManyField and adding direct ForeignKey, which could impact existing data relationships and cause challenges in tracking risk acceptances with engagements.

---

All finding details can be found in the DryRun Security Dashboard.

[Maffooch]

Hi @kiblik, thank you for pausing on this feature before continuing any further. Risk Exceptions are one of those features that is used a little differently by everyone. As Val pointed out, we don't want to break any current use cases, or point ourselves into a corner for future use cases. Minimizing changes to the risk exception object would be the best move we could make at this time

[kiblik]

Hi @kiblik, thank you for pausing on this feature before continuing any further. Risk Exceptions are one of those features that is used a little differently by everyone. As Val pointed out, we don't want to break any current use cases, or point ourselves into a corner for future use cases. Minimizing changes to the risk exception object would be the best move we could make at this time

Thank you @Maffooch. I will prepare the implementation, which will:

• Behave the same as you can experience in UI
• Enable full management via API (it is not possible right now)
• Simplify data structure
• Keep the door open to change it to "less strict" implementation in the future