From eaf960902478b53528ff8134fd63d96d37933ef7 Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 01:08:53 +0200 Subject: [PATCH 01/11] helpful acl messages structure --- re2o/acl.py | 40 +++++++++++++++++++++++++++++++--------- re2o/utils.py | 11 +++++++++++ 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/re2o/acl.py b/re2o/acl.py index 59b09716..5d3601b4 100644 --- a/re2o/acl.py +++ b/re2o/acl.py @@ -36,6 +36,22 @@ from django.shortcuts import redirect from django.urls import reverse from django.utils.translation import ugettext as _ +from re2o.utils import get_group_having_permission + + +def group_list(permissions): + """Create a string listing every groups having one of the given + `permissions`.""" + if permissions: + return ", ".join([ + g.name for g in get_group_having_permission(*permissions) + ]) or "No group have the %s permission(s) !" % " or ".join([ + ",".join(permissions[:-1]), + permissions[-1]] if len(permissions) > 2 else permissions + ) + else: + return "" + def acl_base_decorator(method_name, *targets, on_instance=True): """Base decorator for acl. It checks if the `request.user` has the @@ -53,9 +69,10 @@ def acl_base_decorator(method_name, *targets, on_instance=True): then no error will be triggered, the decorator will act as if permission was granted. This is to allow you to run ACL tests on fields only. If the method exists, it has to return a 2-tuple - `(can, reason)` with `can` being a boolean stating whether the - access is granted and `reason` a message to be displayed if `can` - equals `False` (can be `None`) + `(can, reason, permissions)` with `can` being a boolean stating + whether the access is granted, `reason` a message to be + displayed if `can` equals `False` (can be `None`) and `permissions` + a list of permissions needed for access (can be `None`). *targets: The targets. Targets are specified like a sequence of models and fields names. As an example ``` @@ -139,7 +156,7 @@ ModelC) target = target.get_instance(*args, **kwargs) instances.append(target) except target.DoesNotExist: - yield False, _("Nonexistent entry.") + yield False, _("Nonexistent entry."), [] return if hasattr(target, method_name): can_fct = getattr(target, method_name) @@ -148,11 +165,16 @@ ModelC) can_change_fct = getattr(target, 'can_change_' + field) yield can_change_fct(request.user, *args, **kwargs) - error_messages = [ - x[1] for x in chain.from_iterable( - process_target(x[0], x[1]) for x in group_targets() - ) if not x[0] - ] + error_messages = [] + for target, fields in group_targets(): + for can, msg, permissions in process_target(target, fields): + if not can: + error_messages.append( + msg + _( + " You need to be a member of one of those" + " groups : %s" + ) % group_list(permissions) + ) if error_messages: for msg in error_messages: messages.error( diff --git a/re2o/utils.py b/re2o/utils.py index 2470cf50..a5cb2238 100644 --- a/re2o/utils.py +++ b/re2o/utils.py @@ -38,12 +38,23 @@ from __future__ import unicode_literals from django.utils import timezone from django.db.models import Q +from django.contrib.auth.models import Permission from cotisations.models import Cotisation, Facture, Vente from machines.models import Interface, Machine from users.models import Adherent, User, Ban, Whitelist from preferences.models import AssoOption +def get_group_having_permission(*permission_name): + """Returns every group having the permission `permission_name` + """ + groups = set() + for name in permission_name: + app_label, codename = name.split('.') + permission = Permission.objects.get(content_type__app_label=app_label, codename=codename) + groups = groups.union(permission.group_set.all()) + return groups + def all_adherent(search_time=None, including_asso=True): """ Fonction renvoyant tous les users adherents. Optimisee pour n'est qu'une seule requete sql From 90defb5fcc3d32ff897e118a13459a49db27db41 Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 01:09:13 +0200 Subject: [PATCH 02/11] helpful acl messages for users.models --- users/models.py | 245 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 166 insertions(+), 79 deletions(-) diff --git a/users/models.py b/users/models.py index 821eaffc..16779acc 100755 --- a/users/models.py +++ b/users/models.py @@ -864,29 +864,38 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, if (self == user_request or user_request.has_perm('users.change_user') or user_request.adherent in self.club.administrators.all()): - return True, None + return True, None, None else: - return False, _("You don't have the right to edit this club.") + return False, _("You don't have the right to edit this club."), ('users.change_user',) else: if self == user_request: - return True, None + return True, None, None elif user_request.has_perm('users.change_all_users'): - return True, None + return True, None, None elif user_request.has_perm('users.change_user'): if self.groups.filter(listright__critical=True): - return False, (_("User with critical rights, can't be" - " edited.")) + return ( + False, + _("User with critical rights, can't be edited. "), + ('users.change_all_users',) + ) elif self == AssoOption.get_cached_value('utilisateur_asso'): - return False, (_("Impossible to edit the organisation's" - " user without the 'change_all_users'" - " right.")) + return ( + False, + _("Impossible to edit the organisation's" + " user without the 'change_all_users' right."), + ('users.change_all_users', ) + ) else: - return True, None + return True, None, None elif user_request.has_perm('users.change_all_users'): - return True, None + return True, None, None else: - return False, (_("You don't have the right to edit another" - " user.")) + return ( + False, + _("You don't have the right to edit another user."), + ('users.change_user', 'users.change_all_users') + ) def can_change_password(self, user_request, *_args, **_kwargs): """Check if a user can change a user's password @@ -901,21 +910,28 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, if (self == user_request or user_request.has_perm('users.change_user_password') or user_request.adherent in self.club.administrators.all()): - return True, None + return True, None, None else: - return False, _("You don't have the right to edit this club.") + return ( + False, + _("You don't have the right to edit this club."), + ('users.change_user_password',) + ) else: if (self == user_request or user_request.has_perm('users.change_user_groups')): # Peut éditer les groupes d'un user, # c'est un privilège élevé, True - return True, None + return True, None, None elif (user_request.has_perm('users.change_user') and not self.groups.all()): - return True, None + return True, None, None else: - return False, (_("You don't have the right to edit another" - " user.")) + return ( + False, + _("You don't have the right to edit another user."), + ('users.change_user_groups', 'users.change_user') + ) def check_selfpasswd(self, user_request, *_args, **_kwargs): """ Returns (True, None) if user_request is self, else returns @@ -932,9 +948,13 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ if not ((self.pk == user_request.pk and OptionalUser.get_cached_value('self_change_room')) or user_request.has_perm('users.change_user')): - return False, _("Permission required to change the room.") + return ( + False, + _("Permission required to change the room."), + ('users.change_user',) + ) else: - return True, None + return True, None, None @staticmethod def can_change_state(user_request, *_args, **_kwargs): @@ -946,7 +966,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( user_request.has_perm('users.change_user_state'), - _("Permission required to change the state.") + _("Permission required to change the state."), + ('users.change_user_state',) ) def can_change_shell(self, user_request, *_args, **_kwargs): @@ -958,9 +979,13 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ if not ((self.pk == user_request.pk and OptionalUser.get_cached_value('self_change_shell')) or user_request.has_perm('users.change_user_shell')): - return False, _("Permission required to change the shell.") + return ( + False, + _("Permission required to change the shell."), + ('users.change_user_shell',) + ) else: - return True, None + return True, None, None @staticmethod def can_change_local_email_redirect(user_request, *_args, **_kwargs): @@ -972,7 +997,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( OptionalUser.get_cached_value('local_email_accounts_enabled'), - _("Local email accounts must be enabled.") + _("Local email accounts must be enabled."), + None ) @staticmethod @@ -985,7 +1011,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( OptionalUser.get_cached_value('local_email_accounts_enabled'), - _("Local email accounts must be enabled.") + _("Local email accounts must be enabled."), + None ) @staticmethod @@ -998,7 +1025,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( user_request.has_perm('users.change_user_force'), - _("Permission required to force the move.") + _("Permission required to force the move."), + ('users.change_user_force',) ) @staticmethod @@ -1011,7 +1039,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( user_request.has_perm('users.change_user_groups'), - _("Permission required to edit the user's groups of rights.") + _("Permission required to edit the user's groups of rights."), + ('users.change_user_groups') ) @staticmethod @@ -1023,7 +1052,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( user_request.is_superuser, - _("'superuser' right required to edit the superuser flag.") + _("'superuser' right required to edit the superuser flag."), + [] ) def can_view(self, user_request, *_args, **_kwargs): @@ -1039,16 +1069,23 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, user_request.has_perm('users.view_user') or user_request.adherent in self.club.administrators.all() or user_request.adherent in self.club.members.all()): - return True, None + return True, None, None else: - return False, _("You don't have the right to view this club.") + return ( + False, + _("You don't have the right to view this club."), + ('users.view_user',) + ) else: if (self == user_request or user_request.has_perm('users.view_user')): - return True, None + return True, None, None else: - return False, (_("You don't have the right to view another" - " user.")) + return ( + False, + _("You don't have the right to view another user."), + ('users.view_user',) + ) @staticmethod def can_view_all(user_request, *_args, **_kwargs): @@ -1060,7 +1097,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( user_request.has_perm('users.view_user'), - _("You don't have the right to view the list of users.") + _("You don't have the right to view the list of users."), + ('users.view_user',) ) def can_delete(self, user_request, *_args, **_kwargs): @@ -1073,7 +1111,8 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, """ return ( user_request.has_perm('users.delete_user'), - _("You don't have the right to delete this user.") + _("You don't have the right to delete this user."), + ('users.delete_user',) ) def __init__(self, *args, **kwargs): @@ -1160,15 +1199,16 @@ class Adherent(User): """ if (not user_request.is_authenticated and not OptionalUser.get_cached_value('self_adhesion')): - return False, None + return False, _("Self adhesion is disabled."), None else: if (OptionalUser.get_cached_value('all_can_create_adherent') or OptionalUser.get_cached_value('self_adhesion')): - return True, None + return True, None, None else: return ( user_request.has_perm('users.add_user'), - _("You don't have the right to create a user.") + _("You don't have the right to create a user."), + ('users.add_user',) ) def clean(self, *args, **kwargs): @@ -1216,14 +1256,15 @@ class Club(User): an user or if the `options.all_can_create` is set. """ if not user_request.is_authenticated: - return False, None + return False, _("You must be authenticated."), None else: if OptionalUser.get_cached_value('all_can_create_club'): - return True, None + return True, None, None else: return ( user_request.has_perm('users.add_user'), - _("You don't have the right to create a club.") + _("You don't have the right to create a club."), + ('users.add_user',) ) @staticmethod @@ -1235,13 +1276,17 @@ class Club(User): message. """ if user_request.has_perm('users.view_user'): - return True, None + return True, None, None if (hasattr(user_request, 'is_class_adherent') and user_request.is_class_adherent): if (user_request.adherent.club_administrator.all() or user_request.adherent.club_members.all()): - return True, None - return False, _("You don't have the right to view the list of users.") + return True, None, None + return ( + False, + _("You don't have the right to view the list of users."), + ('users.view_user',) + ) @classmethod def get_instance(cls, clubid, *_args, **_kwargs): @@ -1553,10 +1598,13 @@ class Ban(RevMixin, AclMixin, models.Model): """ if (not user_request.has_perm('users.view_ban') and self.user != user_request): - return False, (_("You don't have the right to view bans other" - " than yours.")) + return ( + False, + _("You don't have the right to view bans other than yours."), + ('users.view_ban',) + ) else: - return True, None + return True, None, None def __str__(self): return str(self.user) + ' ' + str(self.raison) @@ -1620,10 +1668,13 @@ class Whitelist(RevMixin, AclMixin, models.Model): """ if (not user_request.has_perm('users.view_whitelist') and self.user != user_request): - return False, (_("You don't have the right to view whitelists" - " other than yours.")) + return ( + False, + _("You don't have the right to view whitelists other than yours."), + ('users.view_whitelist',) + ) else: - return True, None + return True, None, None def __str__(self): return str(self.user) + ' ' + str(self.raison) @@ -1892,17 +1943,29 @@ class EMailAddress(RevMixin, AclMixin, models.Model): a local email account. """ if user_request.has_perm('users.add_emailaddress'): - return True, None + return True, None, None if not OptionalUser.get_cached_value('local_email_accounts_enabled'): - return False, _("The local email accounts are not enabled.") - if int(user_request.id) != int(userid): - return False, _("You don't have the right to add a local email" - " account to another user.") - elif user_request.email_address.count() >= OptionalUser.get_cached_value('max_email_address'): - return False, _("You reached the limit of {} local email accounts.").format( - OptionalUser.get_cached_value('max_email_address') + return ( + False, + _("The local email accounts are not enabled."), + None ) - return True, None + if int(user_request.id) != int(userid): + return ( + False, + _("You don't have the right to add a local email" + " account to another user."), + ('users.add_emailaddress',) + ) + elif user_request.email_address.count() >= OptionalUser.get_cached_value('max_email_address'): + return ( + False, + _("You reached the limit of {} local email accounts.").format( + OptionalUser.get_cached_value('max_email_address') + ), + None + ) + return True, None, None def can_view(self, user_request, *_args, **_kwargs): """Check if a user can view the local email account @@ -1915,13 +1978,21 @@ class EMailAddress(RevMixin, AclMixin, models.Model): the local email account. """ if user_request.has_perm('users.view_emailaddress'): - return True, None + return True, None, None if not OptionalUser.get_cached_value('local_email_accounts_enabled'): - return False, _("The local email accounts are not enabled.") + return ( + False, + _("The local email accounts are not enabled."), + None + ) if user_request == self.user: - return True, None - return False, _("You don't have the right to edit another user's local" - " email account.") + return True, None, None + return ( + False, + _("You don't have the right to edit another user's local" + " email account."), + ('users.view_emailaddress',) + ) def can_delete(self, user_request, *_args, **_kwargs): """Check if a user can delete the alias @@ -1934,16 +2005,24 @@ class EMailAddress(RevMixin, AclMixin, models.Model): the local email account. """ if self.local_part == self.user.pseudo.lower(): - return False, _("You can't delete a local email account whose" - " local part is the same as the username.") + return ( + False, + _("You can't delete a local email account whose" + " local part is the same as the username."), + None + ) if user_request.has_perm('users.delete_emailaddress'): - return True, None + return True, None, None if not OptionalUser.get_cached_value('local_email_accounts_enabled'): - return False, _("The local email accounts are not enabled.") + return False, _("The local email accounts are not enabled."), None if user_request == self.user: - return True, None - return False, _("You don't have the right to delete another user's" - " local email account") + return True, None, None + return ( + False, + _("You don't have the right to delete another user's" + " local email account"), + ('users.delete_emailaddress',) + ) def can_edit(self, user_request, *_args, **_kwargs): """Check if a user can edit the alias @@ -1956,16 +2035,24 @@ class EMailAddress(RevMixin, AclMixin, models.Model): the local email account. """ if self.local_part == self.user.pseudo.lower(): - return False, _("You can't edit a local email account whose local" - " part is the same as the username.") + return ( + False, + _("You can't edit a local email account whose local" + " part is the same as the username."), + None + ) if user_request.has_perm('users.change_emailaddress'): - return True, None + return True, None, None if not OptionalUser.get_cached_value('local_email_accounts_enabled'): - return False, _("The local email accounts are not enabled.") + return False, _("The local email accounts are not enabled."), None if user_request == self.user: - return True, None - return False, _("You don't have the right to edit another user's local" - " email account.") + return True, None, None + return ( + False, + _("You don't have the right to edit another user's local" + " email account."), + ('users.change_emailaddress',) + ) def clean(self, *args, **kwargs): self.local_part = self.local_part.lower() From dd57daffd1dc7e5f140e3bb4c6f90a7e3c26ad7f Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 14:14:21 +0200 Subject: [PATCH 03/11] helpful acl messages for tickets. --- tickets/models.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/tickets/models.py b/tickets/models.py index c069f0b3..aa5dbd57 100644 --- a/tickets/models.py +++ b/tickets/models.py @@ -17,7 +17,7 @@ class Ticket(AclMixin, models.Model): """Model of a ticket""" user = models.ForeignKey( - 'users.User', + 'users.User', on_delete=models.CASCADE, related_name="tickets", blank=True, @@ -35,7 +35,7 @@ class Ticket(AclMixin, models.Model): date = models.DateTimeField(auto_now_add=True) email = models.EmailField( help_text = _("An email address to get back to you"), - max_length=100, + max_length=100, null=True) solved = models.BooleanField(default=False) @@ -67,26 +67,31 @@ class Ticket(AclMixin, models.Model): GeneralOption.get_cached_value('email_from'), [to_addr], fail_silently = False) - + def can_view(self, user_request, *_args, **_kwargs): """ Check that the user has the right to view the ticket or that it is the author""" if (not user_request.has_perm('tickets.view_ticket') and self.user != user_request): - return False, _("You don't have the right to view other tickets than yours.") + return ( + False, + _("You don't have the right to view other tickets than yours."), + ('tickets.view_ticket',) + ) else: - return True, None - + return True, None, None + @staticmethod def can_view_all(user_request, *_args, **_kwargs): """ Check that the user has access to the list of all tickets""" return( user_request.has_perm('tickets.view_tickets'), - _("You don't have the right to view the list of tickets.") + _("You don't have the right to view the list of tickets."), + ('tickets.view_tickets',) ) def can_create(user_request,*_args, **_kwargs): """ Authorise all users to open tickets """ - return True,None + return True, None, None @receiver(post_save, sender=Ticket) def ticket_post_save(**kwargs): From ce659348bec436eb56b94463650af25d67782e3d Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 14:14:33 +0200 Subject: [PATCH 04/11] helpful acl messages for machines. --- machines/models.py | 348 +++++++++++++++++++++++++++++---------------- 1 file changed, 229 insertions(+), 119 deletions(-) diff --git a/machines/models.py b/machines/models.py index 61834149..a42a767b 100644 --- a/machines/models.py +++ b/machines/models.py @@ -105,8 +105,11 @@ class Machine(RevMixin, FieldPermissionModelMixin, models.Model): A tuple with a boolean stating if edition is allowed and an explanation message. """ - return (user_request.has_perm('machines.change_machine_user'), - _("You don't have the right to change the machine's user.")) + return ( + user_request.has_perm('machines.change_machine_user'), + _("You don't have the right to change the machine's user."), + ('machines.change_machine_user',) + ) @staticmethod def can_view_all(user_request, *_args, **_kwargs): @@ -115,9 +118,12 @@ class Machine(RevMixin, FieldPermissionModelMixin, models.Model): :param user_request: instance user qui fait l'edition :return: True ou False avec la raison de l'échec le cas échéant""" if not user_request.has_perm('machines.view_machine'): - return False, _("You don't have the right to view all the" - " machines.") - return True, None + return ( + False, + _("You don't have the right to view all the machines."), + ('machines.view_machine',) + ) + return True, None, None @staticmethod def can_create(user_request, userid, *_args, **_kwargs): @@ -129,7 +135,7 @@ class Machine(RevMixin, FieldPermissionModelMixin, models.Model): try: user = users.models.User.objects.get(pk=userid) except users.models.User.DoesNotExist: - return False, _("Nonexistent user.") + return False, _("Nonexistent user."), None max_lambdauser_interfaces = (preferences.models.OptionalMachine .get_cached_value( 'max_lambdauser_interfaces' @@ -137,15 +143,23 @@ class Machine(RevMixin, FieldPermissionModelMixin, models.Model): if not user_request.has_perm('machines.add_machine'): if not (preferences.models.OptionalMachine .get_cached_value('create_machine')): - return False, (_("You don't have the right to add a machine.")) + return ( + False, + _("You don't have the right to add a machine."), + ('machines.add_machine',) + ) if user != user_request: - return False, (_("You don't have the right to add a machine" - " to another user.")) + return ( + False, + _("You don't have the right to add a machine" + " to another user."), + ('machines.add_machine',) + ) if user.user_interfaces().count() >= max_lambdauser_interfaces: - return False, (_("You reached the maximum number of interfaces" + return False, _("You reached the maximum number of interfaces" " that you are allowed to create yourself" - " (%s)." % max_lambdauser_interfaces)) - return True, None + " (%s)." % max_lambdauser_interfaces), None + return True, None, None def can_edit(self, user_request, *args, **kwargs): """Vérifie qu'on peut bien éditer cette instance particulière (soit @@ -154,16 +168,22 @@ class Machine(RevMixin, FieldPermissionModelMixin, models.Model): :param user_request: instance user qui fait l'edition :return: True ou False avec la raison le cas échéant""" if self.user != user_request: - if (not user_request.has_perm('machines.change_interface') or - not self.user.can_edit( - self.user, - user_request, - *args, - **kwargs - )[0]): - return False, (_("You don't have the right to edit a machine" - " of another user.")) - return True, None + can_user, _, permissions = self.user.can_edit( + self.user, + user_request, + *args, + **kwargs + ) + if not ( + user_request.has_perm('machines.change_interface') and + can_user): + return ( + False, + _("You don't have the right to edit a machine" + " of another user."), + ('machines.change_interface',) + permissions + ) + return True, None, None def can_delete(self, user_request, *args, **kwargs): """Vérifie qu'on peut bien supprimer cette instance particulière (soit @@ -172,16 +192,22 @@ class Machine(RevMixin, FieldPermissionModelMixin, models.Model): :param user_request: instance user qui fait l'edition :return: True ou False avec la raison de l'échec le cas échéant""" if self.user != user_request: - if (not user_request.has_perm('machines.change_interface') or - not self.user.can_edit( - self.user, - user_request, - *args, - **kwargs - )[0]): - return False, _("You don't have the right to delete a machine" - " of another user.") - return True, None + can_user, _, permissions = self.user.can_edit( + self.user, + user_request, + *args, + **kwargs + ) + if not ( + user_request.has_perm('machines.change_interface') and + can_user): + return ( + False, + _("You don't have the right to delete a machine" + " of another user."), + ('machines.change_interface',) + permissions + ) + return True, None, None def can_view(self, user_request, *_args, **_kwargs): """Vérifie qu'on peut bien voir cette instance particulière (soit @@ -191,9 +217,13 @@ class Machine(RevMixin, FieldPermissionModelMixin, models.Model): :return: True ou False avec la raison de l'échec le cas échéant""" if (not user_request.has_perm('machines.view_machine') and self.user != user_request): - return False, _("You don't have the right to view other machines" - " than yours.") - return True, None + return ( + False, + _("You don't have the right to view other machines" + " than yours."), + ('machines.view_machine',) + ) + return True, None, None @cached_property def short_name(self): @@ -285,9 +315,12 @@ class MachineType(RevMixin, AclMixin, models.Model): message is acces is not allowed. """ if not user_request.has_perm('machines.use_all_machinetype'): - return False, (_("You don't have the right to use all machine" - " types.")) - return True, None + return ( + False, + _("You don't have the right to use all machine types."), + ('machines.use_all_machinetype',) + ) + return True, None, None def __str__(self): return self.name @@ -528,7 +561,11 @@ class IpType(RevMixin, AclMixin, models.Model): restrictions :param user_request: instance user qui fait l'edition :return: True ou False avec la raison de l'échec le cas échéant""" - return user_request.has_perm('machines.use_all_iptype'), None + return ( + user_request.has_perm('machines.use_all_iptype'), + None, + ('machines.use_all_iptype',) + ) def __str__(self): return self.name @@ -766,7 +803,11 @@ class Extension(RevMixin, AclMixin, models.Model): restrictions :param user_request: instance user qui fait l'edition :return: True ou False avec la raison de l'échec le cas échéant""" - return user_request.has_perm('machines.use_all_extension'), None + return ( + user_request.has_perm('machines.use_all_extension'), + _("You cannot use all extensions."), + ('machines.use_all_extension',) + ) def __str__(self): return self.name @@ -1222,31 +1263,42 @@ class Interface(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): try: machine = Machine.objects.get(pk=machineid) except Machine.DoesNotExist: - return False, _("Nonexistent machine.") + return False, _("Nonexistent machine."), None if not user_request.has_perm('machines.add_interface'): if not (preferences.models.OptionalMachine .get_cached_value('create_machine')): - return False, _("You can't add a machine.") + return False, _("You can't add a machine."), ('machines.add_interface',) max_lambdauser_interfaces = (preferences.models.OptionalMachine .get_cached_value( 'max_lambdauser_interfaces' )) if machine.user != user_request: - return False, _("You don't have the right to add an interface" - " to a machine of another user.") + return ( + False, + _("You don't have the right to add an interface" + " to a machine of another user."), + ('machines.add_interface',) + ) if (machine.user.user_interfaces().count() >= max_lambdauser_interfaces): - return False, (_("You reached the maximum number of interfaces" - " that you are allowed to create yourself" - " (%s)." % max_lambdauser_interfaces)) - return True, None + return ( + False, + _("You reached the maximum number of interfaces" + " that you are allowed to create yourself" + " (%s)." % max_lambdauser_interfaces), + ('machines.add_interface',) + ) + return True, None, None @staticmethod def can_change_machine(user_request, *_args, **_kwargs): """Check if a user can change the machine associated with an Interface object """ - return (user_request.has_perm('machines.change_interface_machine'), - _("Permission required to edit the machine.")) + return ( + user_request.has_perm('machines.change_interface_machine'), + _("Permission required to edit the machine."), + ('machines.change_interface_machine',) + ) def can_edit(self, user_request, *args, **kwargs): """Verifie que l'user a les bons droits infra pour editer @@ -1255,15 +1307,21 @@ class Interface(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): :param user_request: Utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" if self.machine.user != user_request: - if (not user_request.has_perm('machines.change_interface') or - not self.machine.user.can_edit( - user_request, - *args, - **kwargs - )[0]): - return False, _("You don't have the right to edit a machine of" - " another user.") - return True, None + can_user, _, permissions = self.machine.user.can_edit( + user_request, + *args, + **kwargs + ) + if not ( + user_request.has_perm('machines.change_interface') and + can_user ): + return ( + False, + _("You don't have the right to edit a machine of" + " another user."), + ('machines.change_interface',) + permissions + ) + return True, None, None def can_delete(self, user_request, *args, **kwargs): """Verifie que l'user a les bons droits delete object pour del @@ -1272,15 +1330,21 @@ class Interface(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): :param user_request: Utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" if self.machine.user != user_request: - if (not user_request.has_perm('machines.change_interface') or - not self.machine.user.can_edit( - user_request, - *args, - **kwargs - )[0]): - return False, _("You don't have the right to edit a machine of" - " another user.") - return True, None + can_user, _, permissions = self.machine.user.can_edit( + user_request, + *args, + **kwargs + ) + if not ( + user_request.has_perm('machines.change_interface') and + can_user): + return ( + False, + _("You don't have the right to edit a machine of" + " another user."), + ('machines.change_interface',) + permissions + ) + return True, None, None def can_view(self, user_request, *_args, **_kwargs): """Vérifie qu'on peut bien voir cette instance particulière avec @@ -1290,9 +1354,12 @@ class Interface(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): :return: True ou False avec la raison de l'échec le cas échéant""" if (not user_request.has_perm('machines.view_interface') and self.machine.user != user_request): - return False, _("You don't have the right to view machines other" - " than yours.") - return True, None + return ( + False, + _("You don't have the right to view machines other than yours."), + ('machines.view_interface',) + ) + return True, None, None def __init__(self, *args, **kwargs): super(Interface, self).__init__(*args, **kwargs) @@ -1340,19 +1407,26 @@ class Ipv6List(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): try: interface = Interface.objects.get(pk=interfaceid) except Interface.DoesNotExist: - return False, _("Nonexistent interface.") + return False, _("Nonexistent interface."), None if not user_request.has_perm('machines.add_ipv6list'): if interface.machine.user != user_request: - return False, _("You don't have the right to add an alias to a" - " machine of another user.") - return True, None + return ( + False, + _("You don't have the right to add an alias to a" + " machine of another user."), + ('machines.add_ipv6list',) + ) + return True, None, None @staticmethod def can_change_slaac_ip(user_request, *_args, **_kwargs): """ Check if a user can change the slaac value """ - return (user_request.has_perm('machines.change_ipv6list_slaac_ip'), - _("Permission required to change the SLAAC value of an IPv6" - " address")) + return ( + user_request.has_perm('machines.change_ipv6list_slaac_ip'), + _("Permission required to change the SLAAC value of an IPv6" + " address"), + ('machines.change_ipv6list_slaac_ip',) + ) def can_edit(self, user_request, *args, **kwargs): """Verifie que l'user a les bons droits infra pour editer @@ -1361,15 +1435,21 @@ class Ipv6List(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): :param user_request: Utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" if self.interface.machine.user != user_request: - if (not user_request.has_perm('machines.change_ipv6list') or - not self.interface.machine.user.can_edit( - user_request, - *args, - **kwargs - )[0]): - return False, _("You don't have the right to edit a machine of" - " another user.") - return True, None + can_user, _, permissions = self.interface.machine.user.can_edit( + user_request, + *args, + **kwargs + ) + if not ( + user_request.has_perm('machines.change_ipv6list') and + can_user): + return ( + False, + _("You don't have the right to edit a machine of" + " another user."), + ('machines.change_ipv6list',) + ) + return True, None, None def can_delete(self, user_request, *args, **kwargs): """Verifie que l'user a les bons droits delete object pour del @@ -1378,15 +1458,20 @@ class Ipv6List(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): :param user_request: Utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" if self.interface.machine.user != user_request: - if (not user_request.has_perm('machines.change_ipv6list') or - not self.interface.machine.user.can_edit( - user_request, - *args, - **kwargs - )[0]): - return False, _("You don't have the right to edit a machine of" - " another user.") - return True, None + can_user, _, permissions = self.interface.machine.user.can_edit( + user_request, + *args, + **kwargs + ) + if not (user_request.has_perm('machines.change_ipv6list') and + can_user): + return ( + False, + _("You don't have the right to edit a machine of" + " another user."), + ('machines.change_ipv6list',) + permissions + ) + return True, None, None def can_view(self, user_request, *_args, **_kwargs): """Vérifie qu'on peut bien voir cette instance particulière avec @@ -1396,9 +1481,12 @@ class Ipv6List(RevMixin, AclMixin, FieldPermissionModelMixin, models.Model): :return: True ou False avec la raison de l'échec le cas échéant""" if (not user_request.has_perm('machines.view_ipv6list') and self.interface.machine.user != user_request): - return False, _("You don't have the right to view machines other" - " than yours.") - return True, None + return ( + False, + _("You don't have the right to view machines other than yours."), + ('machines.view_ipv6list',) + ) + return True, None, None def __init__(self, *args, **kwargs): super(Ipv6List, self).__init__(*args, **kwargs) @@ -1554,25 +1642,33 @@ class Domain(RevMixin, AclMixin, models.Model): try: interface = Interface.objects.get(pk=interfaceid) except Interface.DoesNotExist: - return False, _("Nonexistent interface.") + return False, _("Nonexistent interface."), None if not user_request.has_perm('machines.add_domain'): max_lambdauser_aliases = (preferences.models.OptionalMachine .get_cached_value( 'max_lambdauser_aliases' )) if interface.machine.user != user_request: - return False, _("You don't have the right to add an alias to a" - " machine of another user.") + return ( + False, + _("You don't have the right to add an alias to a" + " machine of another user."), + ('machines.add_domain',) + ) if Domain.objects.filter( cname__in=Domain.objects.filter( interface_parent__in=(interface.machine.user .user_interfaces()) ) ).count() >= max_lambdauser_aliases: - return False, _("You reached the maximum number of alias that" - " you are allowed to create yourself (%s). " - % max_lambdauser_aliases) - return True, None + return ( + False, + _("You reached the maximum number of alias that" + " you are allowed to create yourself (%s). " + % max_lambdauser_aliases), + ('machines.add_domain',) + ) + return True, None, None def can_edit(self, user_request, *_args, **_kwargs): """Verifie que l'user a les bons droits pour editer @@ -1582,9 +1678,13 @@ class Domain(RevMixin, AclMixin, models.Model): :return: soit True, soit False avec la raison de l'échec""" if (not user_request.has_perm('machines.change_domain') and self.get_source_interface.machine.user != user_request): - return False, _("You don't have the right to edit an alias of a" - " machine of another user.") - return True, None + return ( + False, + _("You don't have the right to edit an alias of a" + " machine of another user."), + ('machines.change_domain',) + ) + return True, None, None def can_delete(self, user_request, *_args, **_kwargs): """Verifie que l'user a les bons droits delete object pour del @@ -1594,9 +1694,13 @@ class Domain(RevMixin, AclMixin, models.Model): :return: soit True, soit False avec la raison de l'échec""" if (not user_request.has_perm('machines.delete_domain') and self.get_source_interface.machine.user != user_request): - return False, _("You don't have the right to delete an alias of a" - " machine of another user.") - return True, None + return ( + False, + _("You don't have the right to delete an alias of a" + " machine of another user."), + ('machines.delete_domain',) + ) + return True, None, None def can_view(self, user_request, *_args, **_kwargs): """Vérifie qu'on peut bien voir cette instance particulière avec @@ -1606,9 +1710,12 @@ class Domain(RevMixin, AclMixin, models.Model): :return: True ou False avec la raison de l'échec le cas échéant""" if (not user_request.has_perm('machines.view_domain') and self.get_source_interface.machine.user != user_request): - return False, _("You don't have the right to view machines other" - " than yours.") - return True, None + return ( + False, + _("You don't have the right to view machines other than yours."), + ('machines.view_domain',) + ) + return True, None, None def __str__(self): return str(self.name) + str(self.extension) @@ -1840,11 +1947,14 @@ class OuverturePortList(RevMixin, AclMixin, models.Model): :param user_request: Utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" if not user_request.has_perm('machines.delete_ouvertureportlist'): - return False, _("You don't have the right to delete a ports" - " opening list.") + return ( + False, + _("You don't have the right to delete a ports opening list."), + ('machines.delete_ouvertureportlist',) + ) if self.interface_set.all(): - return False, _("This ports opening list is used.") - return True, None + return False, _("This ports opening list is used."), None + return True, None, None def __str__(self): return self.name From 225731b25cbd07943cb7de9b53c4f39c78d2b347 Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 14:45:51 +0200 Subject: [PATCH 05/11] helpful acl messages for cotisations. --- cotisations/models.py | 212 ++++++++++++++++++++++++++++++------------ 1 file changed, 150 insertions(+), 62 deletions(-) diff --git a/cotisations/models.py b/cotisations/models.py index c6b7cd1c..a2b44b52 100644 --- a/cotisations/models.py +++ b/cotisations/models.py @@ -169,44 +169,78 @@ class Facture(BaseInvoice): return self.vente_set.all() def can_edit(self, user_request, *args, **kwargs): + user_can, _, permissions = self.user.can_edit( + user_request, *args, **kwargs) if not user_request.has_perm('cotisations.change_facture'): - return False, _("You don't have the right to edit an invoice.") + return ( + False, + _("You don't have the right to edit an invoice."), + ('cotisations.change_facture',) + ) elif not user_request.has_perm('cotisations.change_all_facture') and \ - not self.user.can_edit(user_request, *args, **kwargs)[0]: - return False, _("You don't have the right to edit this user's " - "invoices.") + not user_can: + return ( + False, + _("You don't have the right to edit this user's invoices."), + ('cotisations.change_all_facture',) + permissions + ) elif not user_request.has_perm('cotisations.change_all_facture') and \ (self.control or not self.valid): - return False, _("You don't have the right to edit an invoice " - "already controlled or invalidated.") + return ( + False, + _("You don't have the right to edit an invoice " + "already controlled or invalidated."), + ('cotisations.change_all_facture',) + ) else: - return True, None + return True, None, None def can_delete(self, user_request, *args, **kwargs): + user_can, _, permissions = self.user.can_edit( + user_request, *args, **kwargs) if not user_request.has_perm('cotisations.delete_facture'): - return False, _("You don't have the right to delete an invoice.") + return ( + False, + _("You don't have the right to delete an invoice."), + ('cotisations.delete_facture',) + ) elif not user_request.has_perm('cotisations.change_all_facture') and \ - not self.user.can_edit(user_request, *args, **kwargs)[0]: - return False, _("You don't have the right to delete this user's " - "invoices.") + not user_can: + return ( + False, + _("You don't have the right to delete this user's invoices."), + ('cotisations.change_all_facture',) + permissions + ) elif not user_request.has_perm('cotisations.change_all_facture') and \ (self.control or not self.valid): - return False, _("You don't have the right to delete an invoice " - "already controlled or invalidated.") + return ( + False, + _("You don't have the right to delete an invoice " + "already controlled or invalidated."), + ('cotisations.change_all_facture',) + ) else: - return True, None + return True, None, None def can_view(self, user_request, *_args, **_kwargs): if not user_request.has_perm('cotisations.view_facture'): if self.user != user_request: - return False, _("You don't have the right to view someone else's " - "invoices history.") + return ( + False, + _("You don't have the right to view someone else's " + "invoices history."), + ('cotisations.view_facture',) + ) elif not self.valid: - return False, _("The invoice has been invalidated.") + return ( + False, + _("The invoice has been invalidated."), + ('cotisations.view_facture',) + ) else: - return True, None + return True, None, None else: - return True, None + return True, None, None @staticmethod def can_change_control(user_request, *_args, **_kwargs): @@ -214,7 +248,8 @@ class Facture(BaseInvoice): this invoice """ return ( user_request.has_perm('cotisations.change_facture_control'), - _("You don't have the right to edit the \"controlled\" state.") + _("You don't have the right to edit the \"controlled\" state."), + ('cotisations.change_facture_control',) ) @staticmethod @@ -226,12 +261,12 @@ class Facture(BaseInvoice): an invoice or if the `options.allow_self_subscription` is set. """ if user_request.has_perm('cotisations.add_facture'): - return True, None + return True, None, None if len(Paiement.find_allowed_payments(user_request)) <= 0: - return False, _("There are no payment method which you can use.") + return False, _("There are no payment method which you can use."), ('cotisations.add_facture',) if len(Article.find_allowed_articles(user_request, user_request)) <= 0: - return False, _("There are no article that you can buy.") - return True, None + return False, _("There are no article that you can buy."), ('cotisations.add_facture',) + return True, None, None def __init__(self, *args, **kwargs): super(Facture, self).__init__(*args, **kwargs) @@ -360,12 +395,18 @@ class CostEstimate(CustomInvoice): def can_delete(self, user_request, *args, **kwargs): if not user_request.has_perm('cotisations.delete_costestimate'): - return False, _("You don't have the right " - "to delete a cost estimate.") + return ( + False, + _("You don't have the right to delete a cost estimate."), + ('cotisations.delete_costestimate',) + ) if self.final_invoice is not None: - return False, _("The cost estimate has an " - "invoice and can't be deleted.") - return True, None + return ( + False, + _("The cost estimate has an invoice and can't be deleted."), + None + ) + return True, None, None # TODO : change Vente to Purchase @@ -505,40 +546,66 @@ class Vente(RevMixin, AclMixin, models.Model): super(Vente, self).save(*args, **kwargs) def can_edit(self, user_request, *args, **kwargs): + user_can, _, permissions = self.facture.user.can_edit( + user_request, *args, **kwargs + ) if not user_request.has_perm('cotisations.change_vente'): - return False, _("You don't have the right to edit the purchases.") - elif (not user_request.has_perm('cotisations.change_all_facture') and - not self.facture.user.can_edit( - user_request, *args, **kwargs - )[0]): - return False, _("You don't have the right to edit this user's " - "purchases.") + return ( + False, + _("You don't have the right to edit the purchases."), + ('cotisations.change_vente',) + ) + elif not ( + user_request.has_perm('cotisations.change_all_facture') or + user_can): + return ( + False, + _("You don't have the right to edit this user's purchases."), + ('cotisations.change_all_facture',) + permissions + ) elif (not user_request.has_perm('cotisations.change_all_vente') and (self.facture.control or not self.facture.valid)): - return False, _("You don't have the right to edit a purchase " - "already controlled or invalidated.") + return ( + False, + _("You don't have the right to edit a purchase " + "already controlled or invalidated."), + ('cotisations.change_all_vente',) + ) else: - return True, None + return True, None, None def can_delete(self, user_request, *args, **kwargs): + user_can, _, permissions = self.facture.user.can_edit( + user_request, *args, **kwargs) if not user_request.has_perm('cotisations.delete_vente'): - return False, _("You don't have the right to delete a purchase.") - if not self.facture.user.can_edit(user_request, *args, **kwargs)[0]: - return False, _("You don't have the right to delete this user's " - "purchases.") + return ( + False, + _("You don't have the right to delete a purchase."), + ('cotisations.delete_vente',) + ) + if not user_can: + return ( + False, + _("You don't have the right to delete this user's purchases."), + permissions + ) if self.facture.control or not self.facture.valid: return False, _("You don't have the right to delete a purchase " - "already controlled or invalidated.") + "already controlled or invalidated."), None else: - return True, None + return True, None, None def can_view(self, user_request, *_args, **_kwargs): if (not user_request.has_perm('cotisations.view_vente') and self.facture.user != user_request): - return False, _("You don't have the right to view someone " - "else's purchase history.") + return ( + False, + _("You don't have the right to view someone " + "else's purchase history."), + ('cotisations.view_vente',) + ) else: - return True, None + return True, None, None def __str__(self): return str(self.name) + ' ' + str(self.facture) @@ -683,7 +750,8 @@ class Article(RevMixin, AclMixin, models.Model): self.available_for_everyone or user.has_perm('cotisations.buy_every_article') or user.has_perm('cotisations.add_facture'), - _("You can't buy this article.") + _("You can't buy this article."), + ('cotisations.buy_every_article', 'cotisations.add_facture') ) @classmethod @@ -838,7 +906,8 @@ class Paiement(RevMixin, AclMixin, models.Model): self.available_for_everyone or user.has_perm('cotisations.use_every_payment') or user.has_perm('cotisations.add_facture'), - _("You can't use this payment method.") + _("You can't use this payment method."), + ('cotisations.use_every_payment', 'cotisations.add_facture') ) @classmethod @@ -907,32 +976,51 @@ class Cotisation(RevMixin, AclMixin, models.Model): def can_edit(self, user_request, *_args, **_kwargs): if not user_request.has_perm('cotisations.change_cotisation'): - return False, _("You don't have the right to edit a subscription.") + return ( + False, + _("You don't have the right to edit a subscription."), + ('cotisations.change_cotisation',) + ) elif not user_request.has_perm('cotisations.change_all_cotisation') \ and (self.vente.facture.control or not self.vente.facture.valid): - return False, _("You don't have the right to edit a subscription " - "already controlled or invalidated.") + return ( + False, + _("You don't have the right to edit a subscription " + "already controlled or invalidated."), + ('cotisations.change_all_cotisation',) + ) else: - return True, None + return True, None, None def can_delete(self, user_request, *_args, **_kwargs): if not user_request.has_perm('cotisations.delete_cotisation'): - return False, _("You don't have the right to delete a " - "subscription.") + return ( + False, + _("You don't have the right to delete a subscription."), + ('cotisations.delete_cotisation',) + ) if self.vente.facture.control or not self.vente.facture.valid: - return False, _("You don't have the right to delete a subscription " - "already controlled or invalidated.") + return ( + False, + _("You don't have the right to delete a subscription " + "already controlled or invalidated."), + None + ) else: - return True, None + return True, None, None def can_view(self, user_request, *_args, **_kwargs): if not user_request.has_perm('cotisations.view_cotisation') and\ self.vente.facture.user != user_request: - return False, _("You don't have the right to view someone else's " - "subscription history.") + return ( + False, + _("You don't have the right to view someone else's " + "subscription history."), + ('cotisations.view_cotisation',) + ) else: - return True, None + return True, None, None def __str__(self): return str(self.vente) From 9b3bc1d053611bbcef3a8aeaf61d200725274784 Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 14:52:41 +0200 Subject: [PATCH 06/11] ACL for applications. --- api/acl.py | 7 ++++--- cotisations/acl.py | 8 ++++++-- machines/acl.py | 9 +++++++-- preferences/acl.py | 8 ++++++-- topologie/acl.py | 8 ++++++-- users/acl.py | 8 ++++++-- 6 files changed, 35 insertions(+), 13 deletions(-) diff --git a/api/acl.py b/api/acl.py index 0c336281..ec46ee0c 100644 --- a/api/acl.py +++ b/api/acl.py @@ -33,7 +33,7 @@ from django.utils.translation import ugettext as _ def _create_api_permission(): """Creates the 'use_api' permission if not created. - + The 'use_api' is a fake permission in the sense it is not associated with an existing model and this ensure the permission is created every time this file is imported. @@ -70,6 +70,7 @@ def can_view(user): 'app_label': settings.API_CONTENT_TYPE_APP_LABEL, 'codename': settings.API_PERMISSION_CODENAME } - can = user.has_perm('%(app_label)s.%(codename)s' % kwargs) + permission = '%(app_label)s.%(codename)s' % kwargs + can = user.has_perm(permission) return can, None if can else _("You don't have the right to see this" - " application.") + " application."), (permission,) diff --git a/cotisations/acl.py b/cotisations/acl.py index 06c62fb8..1b1611ab 100644 --- a/cotisations/acl.py +++ b/cotisations/acl.py @@ -40,7 +40,11 @@ def can_view(user): """ can = user.has_module_perms('cotisations') if can: - return can, None + return can, None, ('cotisations',) else: - return can, _("You don't have the right to view this application.") + return ( + can, + _("You don't have the right to view this application."), + ('cotisations',) + ) diff --git a/machines/acl.py b/machines/acl.py index 53f70c27..477df6f9 100644 --- a/machines/acl.py +++ b/machines/acl.py @@ -39,5 +39,10 @@ def can_view(user): viewing is granted and msg is a message (can be None). """ can = user.has_module_perms('machines') - return can, None if can else _("You don't have the right to view this" - " application.") + return ( + can, + None if can else _("You don't have the right to view this" + " application."), + ('machines',) + ) + diff --git a/preferences/acl.py b/preferences/acl.py index d4b22cfe..08bfd8e7 100644 --- a/preferences/acl.py +++ b/preferences/acl.py @@ -39,6 +39,10 @@ def can_view(user): viewing is granted and msg is a message (can be None). """ can = user.has_module_perms('preferences') - return can, None if can else _("You don't have the right to view this" - " application.") + return ( + can, + None if can else _("You don't have the right to view this" + " application."), + ('preferences',) + ) diff --git a/topologie/acl.py b/topologie/acl.py index 62e51d8a..2a7227fd 100644 --- a/topologie/acl.py +++ b/topologie/acl.py @@ -39,6 +39,10 @@ def can_view(user): viewing is granted and msg is a message (can be None). """ can = user.has_module_perms('topologie') - return can, None if can else _("You don't have the right to view this" - " application.") + return ( + can, + None if can else _("You don't have the right to view this" + " application."), + ('topologie',) + ) diff --git a/users/acl.py b/users/acl.py index cb3a16db..33ad864e 100644 --- a/users/acl.py +++ b/users/acl.py @@ -38,6 +38,10 @@ def can_view(user): viewing is granted and msg is a message (can be None). """ can = user.has_module_perms('users') - return can, None if can else _("You don't have the right to view this" - " application.") + return ( + can, + None if can else _("You don't have the right to view this" + " application."), + ('users',) + ) From 592b96912960d16d0f554d39e3e195036213092d Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 15:27:03 +0200 Subject: [PATCH 07/11] Helpful acl messages for default mixin. --- re2o/mixins.py | 50 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/re2o/mixins.py b/re2o/mixins.py index f3858428..9d7f7b2f 100644 --- a/re2o/mixins.py +++ b/re2o/mixins.py @@ -104,12 +104,12 @@ class AclMixin(object): un object :param user_request: instance utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" + permission = cls.get_modulename() + '.add_' + cls.get_classname() return ( - user_request.has_perm( - cls.get_modulename() + '.add_' + cls.get_classname() - ), - (_("You don't have the right to create a %s object.") - % cls.get_classname()) + user_request.has_perm(permission), + _("You don't have the right to create a %s object.") + % cls.get_classname(), + (permission,) ) def can_edit(self, user_request, *_args, **_kwargs): @@ -118,12 +118,12 @@ class AclMixin(object): :param self: Instance à editer :param user_request: Utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" + permission = self.get_modulename() + '.change_' + self.get_classname() return ( - user_request.has_perm( - self.get_modulename() + '.change_' + self.get_classname() - ), - (_("You don't have the right to edit a %s object.") - % self.get_classname()) + user_request.has_perm(permission), + _("You don't have the right to edit a %s object.") + % self.get_classname(), + (permission,) ) def can_delete(self, user_request, *_args, **_kwargs): @@ -132,12 +132,12 @@ class AclMixin(object): :param self: Instance à delete :param user_request: Utilisateur qui fait la requête :return: soit True, soit False avec la raison de l'échec""" + permission = self.get_modulename() + '.delete_' + self.get_classname() return ( - user_request.has_perm( - self.get_modulename() + '.delete_' + self.get_classname() - ), - (_("You don't have the right to delete a %s object.") - % self.get_classname()) + user_request.has_perm(permission), + _("You don't have the right to delete a %s object.") + % self.get_classname(), + (permission,) ) @classmethod @@ -146,12 +146,12 @@ class AclMixin(object): droit particulier view objet correspondant :param user_request: instance user qui fait l'edition :return: True ou False avec la raison de l'échec le cas échéant""" + permission = cls.get_modulename() + '.view_' + cls.get_classname() return ( - user_request.has_perm( - cls.get_modulename() + '.view_' + cls.get_classname() - ), - (_("You don't have the right to view every %s object.") - % cls.get_classname()) + user_request.has_perm(permission), + _("You don't have the right to view every %s object.") + % cls.get_classname(), + (permission,) ) def can_view(self, user_request, *_args, **_kwargs): @@ -160,11 +160,11 @@ class AclMixin(object): :param self: instance à voir :param user_request: instance user qui fait l'edition :return: True ou False avec la raison de l'échec le cas échéant""" + permission = self.get_modulename() + '.view_' + self.get_classname() return ( - user_request.has_perm( - self.get_modulename() + '.view_' + self.get_classname() - ), - (_("You don't have the right to view a %s object.") - % self.get_classname()) + user_request.has_perm(permission), + _("You don't have the right to view a %s object.") + % self.get_classname(), + (permission,) ) From 104d5b1d479e4a6f9590dc0ed3ce201ca6907b0d Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 15:27:26 +0200 Subject: [PATCH 08/11] Adapt templatetags to new acl API. --- re2o/templatetags/acl.py | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/re2o/templatetags/acl.py b/re2o/templatetags/acl.py index 6fd00e19..f32c8507 100644 --- a/re2o/templatetags/acl.py +++ b/re2o/templatetags/acl.py @@ -147,6 +147,7 @@ def get_callback(tag_name, obj=None): return acl_fct( lambda x: ( not any(not sys.modules[o].can_view(x)[0] for o in obj), + None, None ), False @@ -155,28 +156,29 @@ def get_callback(tag_name, obj=None): return acl_fct( lambda x: ( not any(not sys.modules[o].can_view(x)[0] for o in obj), + None, None ), True ) if tag_name == 'can_edit_history': return acl_fct( - lambda user: (user.has_perm('admin.change_logentry'), None), + lambda user: (user.has_perm('admin.change_logentry'), None, None), False ) if tag_name == 'cannot_edit_history': return acl_fct( - lambda user: (user.has_perm('admin.change_logentry'), None), + lambda user: (user.has_perm('admin.change_logentry'), None, None), True ) if tag_name == 'can_view_any_app': return acl_fct( - lambda x: (any(sys.modules[o].can_view(x)[0] for o in obj), None), + lambda x: (any(sys.modules[o].can_view(x)[0] for o in obj), None, None), False ) if tag_name == 'cannot_view_any_app': return acl_fct( - lambda x: (any(sys.modules[o].can_view(x)[0] for o in obj), None), + lambda x: (any(sys.modules[o].can_view(x)[0] for o in obj), None, None), True ) @@ -194,8 +196,8 @@ def acl_fct(callback, reverse): def acl_fct_reverse(user, *args, **kwargs): """The cannot_xxx checker callback""" - can, msg = callback(user, *args, **kwargs) - return not can, msg + can, msg, permissions = callback(user, *args, **kwargs) + return not can, msg, permissions return acl_fct_reverse if reverse else acl_fct_normal @@ -217,7 +219,7 @@ def acl_history_filter(parser, token): assert token.contents == 'acl_end' - return AclNode(callback, oknodes, konodes) + return AclNode(tag_name, callback, oknodes, konodes) @register.tag('can_view_any_app') @@ -254,7 +256,7 @@ def acl_app_filter(parser, token): assert token.contents == 'acl_end' - return AclNode(callback, oknodes, konodes) + return AclNode(tag_name, callback, oknodes, konodes) @register.tag('can_change') @@ -264,7 +266,7 @@ def acl_change_filter(parser, token): try: tag_content = token.split_contents() - # tag_name = tag_content[0] + tag_name = tag_content[0] model_name = tag_content[1] field_name = tag_content[2] args = tag_content[3:] @@ -291,7 +293,7 @@ def acl_change_filter(parser, token): # {% can_create_end %} assert token.contents == 'acl_end' - return AclNode(callback, oknodes, konodes, *args) + return AclNode(tag_name, callback, oknodes, konodes, *args) @register.tag('can_create') @@ -333,7 +335,7 @@ def acl_model_filter(parser, token): # {% can_create_end %} assert token.contents == 'acl_end' - return AclNode(callback, oknodes, konodes, *args) + return AclNode(tag_name, callback, oknodes, konodes, *args) @register.tag('can_edit') @@ -377,7 +379,8 @@ class AclNode(Node): """A node for the compiled ACL block when acl callback doesn't require context.""" - def __init__(self, callback, oknodes, konodes, *args): + def __init__(self, tag_name, callback, oknodes, konodes, *args): + self.tag_name = tag_name self.callback = callback self.oknodes = oknodes self.konodes = konodes @@ -388,11 +391,14 @@ class AclNode(Node): if context['user'].is_anonymous(): can = False else: - can, _ = self.callback(context['user'], *(resolved_args)) + can, _, _ = self.callback(context['user'], *(resolved_args)) if can: return self.oknodes.render(context) return self.konodes.render(context) + def __repr__(self): + return '' % self.tag_name + class AclInstanceNode(Node): """A node for the compiled ACL block when acl is based on instance""" @@ -410,7 +416,10 @@ class AclInstanceNode(Node): if context['user'].is_anonymous(): can = False else: - can, _ = callback(context['user'], *(resolved_args)) + can, _, _ = callback(context['user'], *(resolved_args)) if can: return self.oknodes.render(context) return self.konodes.render(context) + + def __repr__(self): + return '' % self.tag_name From 04abc64cc973f2b2534282decb2bb292602c2098 Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 15:39:17 +0200 Subject: [PATCH 09/11] Better error messages. --- logs/acl.py | 8 ++++++-- re2o/acl.py | 35 ++++++++++++++++++++--------------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/logs/acl.py b/logs/acl.py index ee9a7b1b..b4df34af 100644 --- a/logs/acl.py +++ b/logs/acl.py @@ -39,6 +39,10 @@ def can_view(user): viewing is granted and msg is a message (can be None). """ can = user.has_module_perms('admin') - return can, None if can else _("You don't have the right to view this" - " application.") + return ( + can, + None if can else _("You don't have the right to view this" + " application."), + 'admin' + ) diff --git a/re2o/acl.py b/re2o/acl.py index 5d3601b4..23f51210 100644 --- a/re2o/acl.py +++ b/re2o/acl.py @@ -42,15 +42,9 @@ from re2o.utils import get_group_having_permission def group_list(permissions): """Create a string listing every groups having one of the given `permissions`.""" - if permissions: - return ", ".join([ - g.name for g in get_group_having_permission(*permissions) - ]) or "No group have the %s permission(s) !" % " or ".join([ - ",".join(permissions[:-1]), - permissions[-1]] if len(permissions) > 2 else permissions - ) - else: - return "" + return ", ".join([ + g.name for g in get_group_having_permission(*permissions) + ]) def acl_base_decorator(method_name, *targets, on_instance=True): @@ -169,12 +163,23 @@ ModelC) for target, fields in group_targets(): for can, msg, permissions in process_target(target, fields): if not can: - error_messages.append( - msg + _( - " You need to be a member of one of those" - " groups : %s" - ) % group_list(permissions) - ) + groups = group_list(permissions) + if groups: + error_messages.append( + msg + _( + " You need to be a member of one of those" + " groups : %s" + ) % groups + ) + else: + error_messages.append( + msg + " No group have the %s permission(s) !" \ + % " or ".join([ + ",".join(permissions[:-1]), + permissions[-1]] + if len(permissions) > 2 else permissions + ) + ) if error_messages: for msg in error_messages: messages.error( From 9161e0f7e3501b861cd09b7e6d8be5c837127beb Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 16:05:45 +0200 Subject: [PATCH 10/11] Reusable acl error messages. --- preferences/views.py | 7 +++---- re2o/acl.py | 39 ++++++++++++++++++--------------------- re2o/field_permissions.py | 2 +- 3 files changed, 22 insertions(+), 26 deletions(-) diff --git a/preferences/views.py b/preferences/views.py index 471207e0..90afa2e8 100644 --- a/preferences/views.py +++ b/preferences/views.py @@ -43,7 +43,7 @@ from reversion import revisions as reversion from importlib import import_module from re2o.settings_local import OPTIONNAL_APPS_RE2O from re2o.views import form -from re2o.acl import can_create, can_edit, can_delete_set, can_view_all, can_delete +from re2o.acl import can_create, can_edit, can_delete_set, can_view_all, can_delete, acl_error_message from .forms import MailContactForm, DelMailContactForm from .forms import ( @@ -130,10 +130,9 @@ def edit_options(request, section): return redirect(reverse('preferences:display-options')) options_instance, _created = model.objects.get_or_create() - can, msg = options_instance.can_edit(request.user) + can, msg, permissions = options_instance.can_edit(request.user) if not can: - messages.error(request, msg or _("You don't have the right to edit" - " this option.")) + messages.error(request, acl_error_message(msg, permissions)) return redirect(reverse('index')) options = form_instance( request.POST or None, diff --git a/re2o/acl.py b/re2o/acl.py index 23f51210..e445a28c 100644 --- a/re2o/acl.py +++ b/re2o/acl.py @@ -39,12 +39,24 @@ from django.utils.translation import ugettext as _ from re2o.utils import get_group_having_permission -def group_list(permissions): - """Create a string listing every groups having one of the given - `permissions`.""" - return ", ".join([ +def acl_error_message(msg, permissions): + """Create an error message for msg and permissions.""" + groups = ", ".join([ g.name for g in get_group_having_permission(*permissions) ]) + message = msg or _("You don't have the right to edit" + " this option.") + if groups: + return message + _( + " You need to be a member of one of those" + " groups : %s" + ) % groups + else: + return message + " No group have the %s permission(s) !" % " or ".join([ + ",".join(permissions[:-1]), + permissions[-1]] + if len(permissions) > 2 else permissions + ) def acl_base_decorator(method_name, *targets, on_instance=True): @@ -163,23 +175,8 @@ ModelC) for target, fields in group_targets(): for can, msg, permissions in process_target(target, fields): if not can: - groups = group_list(permissions) - if groups: - error_messages.append( - msg + _( - " You need to be a member of one of those" - " groups : %s" - ) % groups - ) - else: - error_messages.append( - msg + " No group have the %s permission(s) !" \ - % " or ".join([ - ",".join(permissions[:-1]), - permissions[-1]] - if len(permissions) > 2 else permissions - ) - ) + error_messages.append(acl_error_message(msg, permissions)) + if error_messages: for msg in error_messages: messages.error( diff --git a/re2o/field_permissions.py b/re2o/field_permissions.py index 5febeb63..4e3be7f3 100644 --- a/re2o/field_permissions.py +++ b/re2o/field_permissions.py @@ -69,7 +69,7 @@ class FieldPermissionModelMixin: # Try to find a user setting that qualifies them for permission. for perm in checks: if callable(perm): - result, _reason = perm(user_request=user) + result, _reason, _permissions = perm(user_request=user) if result is not None: return result else: From 5e21a0c4943ae17f09dcddb52ecd5b8f95b879a8 Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Fri, 6 Sep 2019 18:32:51 +0200 Subject: [PATCH 11/11] Fix check_selfpasswd --- users/models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/users/models.py b/users/models.py index 16779acc..1de58ea5 100755 --- a/users/models.py +++ b/users/models.py @@ -934,10 +934,10 @@ class User(RevMixin, FieldPermissionModelMixin, AbstractBaseUser, ) def check_selfpasswd(self, user_request, *_args, **_kwargs): - """ Returns (True, None) if user_request is self, else returns - (False, None) + """ Returns (True, None, None) if user_request is self, else returns + (False, None, None) """ - return user_request == self, None + return user_request == self, None, None def can_change_room(self, user_request, *_args, **_kwargs): """ Check if a user can change a room