Do not force that deletion can only be done by superuser

[dekkers]

We should not force that deletion can only be done by a superuser. By default users won't have this permissions, so only the superuser can do it. If the permission to delete is given to a user we should honor that.

---

Code Checklist

• All the commits in this PR are properly PGP-signed and verified;
• This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
• I have written unit tests for the changes or fixes I made.
• For any non-trivial functionality, I have added integration and/or end-to-end tests.
• I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

• I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
• I have made corresponding changes to the documentation, if necessary.

---

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.

---

Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

[Donnype]

@dekkers Makes sense. But I was also wondering what the current effect would be and checking when users now gain the "delete_organization" permission, which was interesting:

has_perm("delete_organization")	KATUser is Superuser	KATUser is Admin	Member is redteamer	Member is admin
KATUser	True	False	False	False
OrganizationMember	False	False	False	False

I'd say the first row, reflecting Django defaults I suppose, makes sense, but due to the implementation of OrganizationMember.has_perm() where we both collect the group permissions and KATUser permissions through KATUser.get_all_permissions(), we do not inherit this permission as the latter call does not return these "defaults". Is this what we expect, or should logged in superuser be able to delete organizations regardless of their role as an organization member?

If not, perhaps we should rewrite the has_perm method of the OrganizationMember. Or at least change it to

    def has_perm(self, perm: str) -> bool:
        return perm in self.all_permissions or self.user.has_perm(perm)

And remove the | self.user.get_all_permissions() from the return statement in the all_permissions() method. How I read the code this seems like the intended behavior here, but I could be wrong.

[dekkers]

At https://github.com/minvws/nl-kat-coordination/blob/main/rocky/tools/models.py#L239 we return all permissions if the user is the superuser. At https://github.com/minvws/nl-kat-coordination/blob/main/rocky/tools/models.py#L255 we call self.user.get_all_permissions to also get the global permissions. Your proposed change of calling self.user.has_perm doesn't really add anything. This is also unit tested in https://github.com/minvws/nl-kat-coordination/blob/main/rocky/tests/test_models.py#L25

[underdarknl]

An admin user should be able to delete their own organisations. Where 'their own' means, where they have admin rights.

[Donnype]

@dekkers What I found and why I suggested the change was the following:

>>> k
<KATUser: k@k.nl>
>>> k.is_superuser
True
>>> k.get_all_permissions()
set()
>>> k.has_perm("delete_organization")
True

And what I just read is that apparently this is the consequence of superusers always having all permissions.. My guess is that if we check the OrganizationMember model for the delete_organization permissions somewhere outside the admin interface (KATUser is checked so this works fine), the default superusers we use e.g. in development will not have that permission.

So the question arises: should an OrganizationMember attached to a superuser also have all permissions? Or should we add more default permissions like delete_organization to e.g. the admin group?

[ammar92]

Good discussions. I think we should use our permissions/ role system as much as possible as it simplifies things. It's usually also safer to gradually assign permissions than already having a set of (possibly destructive) permissions

[Darwinkel]

Checklist for QA:

• I have checked out this branch, and successfully ran a fresh make reset.
• I confirmed that there are no unintended functional regressions in this branch:
  • I have managed to pass the onboarding flow
  • Objects and Findings are created properly
  • Tasks are created and completed properly
• I confirmed that the PR's advertised feature or hotfix works as intended.

What works:

• Superuser can still delete users and organizations through the Django admin.

[dekkers]

I do get all the permissions for the superuser. Did you change the user to superuser before the call to get_all_permissions? That might explain it because the permissions are cached.

>>> user = KATUser.objects.get(id=1)
>>> user.is_superuser
True
>>> user.get_all_permissions()
{'tools.can_mute_findings', 'tools.view_ooiinformation', 'account.change_katuser', 'account.add_katuser', 'sessions.view_session', 'auth.add_permission', 'fmea.view_failuremodeeffect', 'tools.delete_organizationtag', 'password_history.delete_passwordhistory', 
'tools.change_organization', 'fmea.change_failuremodeeffect', 'admin.change_logentry', 'password_history.change_passwordhistory', 'fmea.change_failuremodetreeobject', 'otp_static.add_statictoken', 'fmea.view_failuremode', 'fmea.view_failuremodetreeobject', 'to
ols.add_indemnification', 'otp_totp.add_totpdevice', 'otp_static.delete_statictoken', 'sessions.add_session', 'tools.view_organizationtag', 'otp_static.change_statictoken', 'password_history.add_userpasswordhistoryconfig', 'auth.delete_permission', 'account.de
lete_katuser', 'auth.delete_group', 'password_history.change_userpasswordhistoryconfig', 'tools.delete_organizationmember', 'otp_static.delete_staticdevice', 'fmea.add_failuremode', 'tools.add_organization', 'tools.view_organizationmember', 'tools.can_view_kat
alogus_settings', 'auth.view_group', 'account.view_katuser', 'contenttypes.delete_contenttype', 'tools.delete_job', 'contenttypes.add_contenttype', 'auth.change_group', 'tools.change_ooiinformation', 'fmea.change_failuremodeaffectedobject', 'tools.delete_ooiin
formation', 'admin.view_logentry', 'contenttypes.view_contenttype', 'tools.view_organization', 'fmea.view_failuremodeaffectedobject', 'password_history.view_passwordhistory', 'auth.add_group', 'sessions.change_session', 'tools.can_delete_oois', 'fmea.delete_fa
iluremode', 'tools.can_switch_organization', 'tools.add_organizationmember', 'tools.view_job', 'otp_static.view_staticdevice', 'fmea.change_failuremode', 'auth.change_permission', 'tools.delete_indemnification', 'fmea.delete_failuremodeeffect', 'sessions.delet
e_session', 'password_history.view_userpasswordhistoryconfig', 'tools.can_scan_organization', 'tools.add_ooiinformation', 'fmea.add_failuremodetreeobject', 'fmea.delete_failuremodeaffectedobject', 'tools.view_indemnification', 'tools.can_set_katalogus_settings
', 'otp_totp.change_totpdevice', 'tools.add_job', 'tools.add_organizationtag', 'auth.view_permission', 'otp_totp.delete_totpdevice', 'tools.change_job', 'fmea.add_failuremodeeffect', 'otp_static.add_staticdevice', 'fmea.add_failuremodeaffectedobject', 'tools.c
an_set_clearance_level', 'tools.can_recalculate_bits', 'otp_static.change_staticdevice', 'admin.add_logentry', 'otp_static.view_statictoken', 'fmea.delete_failuremodetreeobject', 'tools.delete_organization', 'tools.change_organizationmember', 'tools.change_org
anizationtag', 'tools.can_enable_disable_boefje', 'contenttypes.change_contenttype', 'password_history.add_passwordhistory', 'tools.change_indemnification', 'admin.delete_logentry', 'password_history.delete_userpasswordhistoryconfig', 'otp_totp.view_totpdevice
'}

[Donnype]

@dekkers Ahhh I thought I had tried that but apparently not 👍