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

Return an error message when an admin try to delete the only admin account #192

Conversation

monsieurswag
Copy link
Contributor

@monsieurswag monsieurswag commented Apr 2, 2024

I removed import { fail } from 'assert'; in "frontend/src/routes/(app)/users/[id=uuid]/edit/+page.server.ts", it seems like it was an error surely from someone who auto-imported the wrong fail function in a previous commit.

According to this Mixin definition the "name" field isn't unique which means there may multiple UserGroup object with the same name :

class NameDescriptionMixin(AbstractBaseModel):
    name = models.CharField(max_length=200, verbose_name=_("Name"), unique=False)

I am not sure having 2 user groups with the exact same name should be authorized by the application, is this normal ?
If some evil user could create a user group with the same name as the admin group and bypass some permission checks.
To address this potential issue in my code i always added builtin=True when fetching the admin group since builtin objects are meant to be immutable.

@eric-intuitem
Copy link
Collaborator

Indeed, once groups will be user-defined, we'll need to add fields_to_check = ["name"]

Copy link
Collaborator

@eric-intuitem eric-intuitem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing translation

@monsieurswag
Copy link
Contributor Author

Done

Copy link
Collaborator

@Mohamed-Hacene Mohamed-Hacene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0k

backend/iam/models.py Outdated Show resolved Hide resolved
backend/core/views.py Outdated Show resolved Hide resolved
@monsieurswag
Copy link
Contributor Author

monsieurswag commented Apr 3, 2024

Remove builtin and user_groups__builtin, and make the languageTag: languageTag synthax like it was before

@monsieurswag monsieurswag force-pushed the CA-329-An-admin-can-remove-his-admin-permissions-and-even-delete-his-account branch from 7d3d7e4 to 8051e52 Compare April 5, 2024 06:13
Copy link
Collaborator

@Mohamed-Hacene Mohamed-Hacene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0k

Copy link
Collaborator

@Mohamed-Hacene Mohamed-Hacene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0k

Copy link
Collaborator

@eric-intuitem eric-intuitem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eric-intuitem eric-intuitem merged commit b6cda17 into main Apr 5, 2024
12 checks passed
@eric-intuitem eric-intuitem deleted the CA-329-An-admin-can-remove-his-admin-permissions-and-even-delete-his-account branch April 5, 2024 09:21
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants