Merge branch 'fix_121_prettier_ACL_decorators' into 'master'

Fix #121 prettier acl decorators

Closes #121

See merge request federez/re2o!155

logs/views.py

@@ -195,9 +195,7 @@ def revert_action(request, revision_id):
 
 
 @login_required
-@can_view_all(IpList)
-@can_view_all(Interface)
-@can_view_all(User)
+@can_view_all(IpList, Interface, User)
 def stats_general(request):
     """Statistiques générales affinées sur les ip, activées, utilisées par
     range, et les statistiques générales sur les users : users actifs,
@@ -313,10 +311,7 @@ def stats_general(request):
 
 
 @login_required
-@can_view_app('users')
-@can_view_app('cotisations')
-@can_view_app('machines')
-@can_view_app('topologie')
+@can_view_app('users', 'cotisations', 'machines', 'topologie')
 def stats_models(request):
     """Statistiques générales, affiche les comptages par models:
     nombre d'users, d'écoles, de droits, de bannissements,

machines/views.py

@@ -1272,12 +1272,7 @@ def index_nas(request):
 
 
 @login_required
-@can_view_all(SOA)
-@can_view_all(Mx)
-@can_view_all(Ns)
-@can_view_all(Txt)
-@can_view_all(Srv)
-@can_view_all(Extension)
+@can_view_all(SOA, Mx, Ns, Txt, Srv, Extension)
 def index_extension(request):
     """ View displaying the list of existing extensions, the list of
     existing SOA records, the list of existing MX records , the list of

preferences/views.py

@@ -58,13 +58,8 @@ from . import forms
 
 
 @login_required
-@can_view_all(OptionalUser)
-@can_view_all(OptionalMachine)
-@can_view_all(OptionalTopologie)
-@can_view_all(GeneralOption)
-@can_view_all(AssoOption)
-@can_view_all(MailMessageOption)
-@can_view_all(HomeOption)
+@can_view_all(OptionalUser, OptionalMachine, OptionalTopologie, GeneralOption,
+              AssoOption, MailMessageOption, HomeOption)
 def display_options(request):
     """Vue pour affichage des options (en vrac) classé selon les models
     correspondants dans un tableau"""
@@ -149,7 +144,11 @@ def add_service(request):
 @can_edit(Service)
 def edit_service(request, service_instance, **_kwargs):
     """Edition des services affichés sur la page d'accueil"""
-    service = ServiceForm(request.POST or None, request.FILES or None,instance=service_instance)
+    service = ServiceForm(
+        request.POST or None,
+        request.FILES or None,
+        instance=service_instance
+    )
     if service.is_valid():
         with transaction.atomic(), reversion.create_revision():
             service.save()

re2o/acl.py

@@ -28,135 +28,177 @@ Here are defined some decorators that can be used in views to handle ACL.
 from __future__ import unicode_literals
 
 import sys
+from itertools import chain
 
+from django.db.models import Model
 from django.contrib import messages
 from django.shortcuts import redirect
 from django.urls import reverse
 
 
-def can_create(model):
-    """Decorator to check if an user can create a model.
-    It assumes that a valid user exists in the request and that the model has a
-    method can_create(user) which returns true if the user can create this kind
-    of models.
+def acl_base_decorator(method_name, *targets, on_instance=True):
+    """Base decorator for acl. It checks if the `request.user` has the
+    permission by calling model.method_name. If the flag on_instance is True,
+    tries to get an instance of the model by calling
+    `model.get_instance(*args, **kwargs)` and runs `instance.mehod_name`
+    rather than model.method_name.
+
+    It is not intended to be used as is. It is a base for others ACL
+    decorators.
+
+    Args:
+        method_name: The name of the method which is to to be used for ACL.
+            (ex: 'can_edit') WARNING: if no method called 'method_name' exists,
+            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`)
+        *targets: The targets. Targets are specified like a sequence of models
+            and fields names. As an example
+            ```
+                acl_base_decorator('can_edit', ModelA, 'field1', 'field2', \
+ModelB, ModelC, 'field3', on_instance=False)
+            ```
+            will make the following calls (where `user` is the current user,
+            `*args` and `**kwargs` are the arguments initially passed to the
+            view):
+                - `ModelA.can_edit(user, *args, **kwargs)`
+                - `ModelA.can_change_field1(user, *args, **kwargs)`
+                - `ModelA.can_change_field2(user, *args, **kwargs)`
+                - `ModelB.can_edit(user, *args, **kwargs)`
+                - `ModelC.can_edit(user, *args, **kwargs)`
+                - `ModelC.can_change_field3(user, *args, **kwargs)`
+
+            Note that
+            ```
+                acl_base_decorator('can_edit', 'field1', ModelA, 'field2', \
+on_instance=False)
+            ```
+            would have the same effect that
+            ```
+                acl_base_decorator('can_edit', ModelA, 'field1', 'field2', \
+on_instance=False)
+            ```
+            But don't do that, it's silly.
+        on_instance: When `on_instance` equals `False`, the decorator runs the
+            ACL method on the model class rather than on an instance. If an
+            instance need to fetched, it is done calling the assumed existing
+            method `get_instance` of the model, with the arguments originally
+            passed to the view.
+
+    Returns:
+        The user is either redirected to their own page with an explanation
+        message if at least one access is not granted, or to the view. In order
+        to avoid duplicate DB calls, when the `on_instance` flag equals `True`,
+        the instances are passed to the view. Example, with this decorator:
+        ```
+            acl_base_decorator('can_edit', ModelA, 'field1', 'field2', ModelB,\
+ModelC)
+        ```
+        The view will be called like this:
+        ```
+            view(request, instance_of_A, instance_of_b, *args, **kwargs)
+        ```
+        where `*args` and `**kwargs` are the original view arguments.
     """
+
+    def group_targets():
+        """This generator parses the targets of the decorator, yielding
+        2-tuples of (model, [fields]).
+        """
+        current_target = None
+        current_fields = []
+        for target in targets:
+            if not isinstance(target, str):
+                if current_target:
+                    yield (current_target, current_fields)
+                current_target = target
+                current_fields = []
+            else:
+                current_fields.append(target)
+        yield (current_target, current_fields)
+
     def decorator(view):
         """The decorator to use on a specific view
         """
         def wrapper(request, *args, **kwargs):
-            """The wrapper used for a specific request
-            """
-            can, msg = model.can_create(request.user, *args, **kwargs)
-            if not can:
-                messages.error(
-                    request, msg or "Vous ne pouvez pas accéder à ce menu")
-                return redirect(reverse('index'))
-            return view(request, *args, **kwargs)
-        return wrapper
-    return decorator
+            """The wrapper used for a specific request"""
+            instances = []
 
-
-def can_edit(model, *field_list):
-    """Decorator to check if an user can edit a model.
-    It tries to get an instance of the model, using
-    `model.get_instance(*args, **kwargs)` and assumes that the model has a
-    method `can_edit(user)` which returns `true` if the user can edit this
-    kind of models.
-    """
-    def decorator(view):
-        """The decorator to use on a specific view
-        """
-        def wrapper(request, *args, **kwargs):
-            """The wrapper used for a specific request
+            def process_target(target, fields):
+                """This function calls the methods on the target and checks for
+                the can_change_`field` method with the given fields. It also
+                stores the instances of models in order to avoid duplicate DB
+                calls for the view.
                 """
+                if on_instance:
                     try:
-                instance = model.get_instance(*args, **kwargs)
-            except model.DoesNotExist:
-                messages.error(request, u"Entrée inexistante")
-                return redirect(reverse(
-                    'users:profil',
-                    kwargs={'userid': str(request.user.id)}
-                ))
-            can, msg = instance.can_edit(request.user)
-            if not can:
+                        target = target.get_instance(*args, **kwargs)
+                        instances.append(target)
+                    except target.DoesNotExist:
+                        yield False, u"Entrée inexistante"
+                        return
+                if hasattr(target, method_name):
+                    can_fct = getattr(target, method_name)
+                    yield can_fct(request.user, *args, **kwargs)
+                for field in fields:
+                    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]
+            ]
+            if error_messages:
+                for msg in error_messages:
                     messages.error(
                         request, msg or "Vous ne pouvez pas accéder à ce menu")
                 return redirect(reverse(
                     'users:profil',
                     kwargs={'userid': str(request.user.id)}
                 ))
-            for field in field_list:
-                can_change_fct = getattr(instance, 'can_change_' + field)
-                can, msg = can_change_fct(request.user, *args, **kwargs)
-                if not can:
-                    messages.error(
-                        request, msg or "Vous ne pouvez pas accéder à ce menu")
-                    return redirect(reverse(
-                        'users:profil',
-                        kwargs={'userid': str(request.user.id)}
-                    ))
-            return view(request, instance, *args, **kwargs)
+            return view(request, *chain(instances, args), **kwargs)
         return wrapper
     return decorator
 
 
-def can_change(model, *field_list):
+def can_create(*models):
+    """Decorator to check if an user can create the given models. It runs
+    `acl_base_decorator` with the flag `on_instance=False` and the method
+    'can_create'. See `acl_base_decorator` documentation for further details.
+    """
+    return acl_base_decorator('can_create', *models, on_instance=False)
+
+
+def can_edit(*targets):
+    """Decorator to check if an user can edit the models.
+    It runs `acl_base_decorator` with the flag `on_instance=True` and the
+    method 'can_edit'. See `acl_base_decorator` documentation for further
+    details.
+    """
+    return acl_base_decorator('can_edit', *targets)
+
+
+def can_change(*targets):
     """Decorator to check if an user can edit a field of a model class.
-    Difference with can_edit : take a class and not an instance
+    Difference with can_edit : takes a class and not an instance
+    It runs `acl_base_decorator` with the flag `on_instance=False` and the
+    method 'can_change'. See `acl_base_decorator` documentation for further
+    details.
     """
-    def decorator(view):
-        """The decorator to use on a specific view
-        """
-        def wrapper(request, *args, **kwargs):
-            """The wrapper used for a specific request
-            """
-            for field in field_list:
-                can_change_fct = getattr(model, 'can_change_' + field)
-                can, msg = can_change_fct(request.user, *args, **kwargs)
-                if not can:
-                    messages.error(
-                        request, msg or "Vous ne pouvez pas accéder à ce menu")
-                    return redirect(reverse(
-                        'users:profil',
-                        kwargs={'userid': str(request.user.id)}
-                    ))
-            return view(request, *args, **kwargs)
-        return wrapper
-    return decorator
+    return acl_base_decorator('can_change', *targets)
 
 
-def can_delete(model):
+def can_delete(*targets):
     """Decorator to check if an user can delete a model.
-    It tries to get an instance of the model, using
-    `model.get_instance(*args, **kwargs)` and assumes that the model has a
-    method `can_delete(user)` which returns `true` if the user can delete this
-    kind of models.
+    It runs `acl_base_decorator` with the flag `on_instance=True` and the
+    method 'can_edit'. See `acl_base_decorator` documentation for further
+    details.
     """
-    def decorator(view):
-        """The decorator to use on a specific view
-        """
-        def wrapper(request, *args, **kwargs):
-            """The wrapper used for a specific request
-            """
-            try:
-                instance = model.get_instance(*args, **kwargs)
-            except model.DoesNotExist:
-                messages.error(request, u"Entrée inexistante")
-                return redirect(reverse(
-                    'users:profil',
-                    kwargs={'userid': str(request.user.id)}
-                ))
-            can, msg = instance.can_delete(request.user)
-            if not can:
-                messages.error(
-                    request, msg or "Vous ne pouvez pas accéder à ce menu")
-                return redirect(reverse(
-                    'users:profil',
-                    kwargs={'userid': str(request.user.id)}
-                ))
-            return view(request, instance, *args, **kwargs)
-        return wrapper
-    return decorator
+    return acl_base_decorator('can_delete', *targets)
 
 
 def can_delete_set(model):
@@ -187,84 +229,34 @@ def can_delete_set(model):
     return decorator
 
 
-def can_view(model):
+def can_view(*targets):
     """Decorator to check if an user can view a model.
-    It tries to get an instance of the model, using
-    `model.get_instance(*args, **kwargs)` and assumes that the model has a
-    method `can_view(user)` which returns `true` if the user can view this
-    kind of models.
+    It runs `acl_base_decorator` with the flag `on_instance=True` and the
+    method 'can_view'. See `acl_base_decorator` documentation for further
+    details.
     """
-    def decorator(view):
-        """The decorator to use on a specific view
-        """
-        def wrapper(request, *args, **kwargs):
-            """The wrapper used for a specific request
-            """
-            try:
-                instance = model.get_instance(*args, **kwargs)
-            except model.DoesNotExist:
-                messages.error(request, u"Entrée inexistante")
-                return redirect(reverse(
-                    'users:profil',
-                    kwargs={'userid': str(request.user.id)}
-                ))
-            can, msg = instance.can_view(request.user)
-            if not can:
-                messages.error(
-                    request, msg or "Vous ne pouvez pas accéder à ce menu")
-                return redirect(reverse(
-                    'users:profil',
-                    kwargs={'userid': str(request.user.id)}
-                ))
-            return view(request, instance, *args, **kwargs)
-        return wrapper
-    return decorator
+    return acl_base_decorator('can_view', *targets)
 
 
-def can_view_all(model):
+def can_view_all(*targets):
     """Decorator to check if an user can view a class of model.
+    It runs `acl_base_decorator` with the flag `on_instance=False` and the
+    method 'can_view_all'. See `acl_base_decorator` documentation for further
+    details.
     """
-    def decorator(view):
-        """The decorator to use on a specific view
-        """
-        def wrapper(request, *args, **kwargs):
-            """The wrapper used for a specific request
-            """
-            can, msg = model.can_view_all(request.user)
-            if not can:
-                messages.error(
-                    request, msg or "Vous ne pouvez pas accéder à ce menu")
-                return redirect(reverse(
-                    'users:profil',
-                    kwargs={'userid': str(request.user.id)}
-                ))
-            return view(request, *args, **kwargs)
-        return wrapper
-    return decorator
+    return acl_base_decorator('can_view_all', *targets, on_instance=False)
 
 
-def can_view_app(app_name):
-    """Decorator to check if an user can view an application.
+def can_view_app(*apps_name):
+    """Decorator to check if an user can view the applications.
     """
+    for app_name in apps_name:
         assert app_name in sys.modules.keys()
-
-    def decorator(view):
-        """The decorator to use on a specific view
-        """
-        def wrapper(request, *args, **kwargs):
-            """The wrapper used for a specific request
-            """
-            app = sys.modules[app_name]
-            can, msg = app.can_view(request.user)
-            if can:
-                return view(request, *args, **kwargs)
-            messages.error(request, msg)
-            return redirect(reverse(
-                'users:profil',
-                kwargs={'userid': str(request.user.id)}
-            ))
-        return wrapper
-    return decorator
+    return acl_base_decorator(
+        'can_view',
+        *chain(sys.modules[app_name] for app_name in apps_name),
+        on_instance=False
+    )
 
 
 def can_edit_history(view):

topologie/views.py

@@ -200,9 +200,7 @@ def index_ap(request):
 
 
 @login_required
-@can_view_all(Stack)
-@can_view_all(Building)
-@can_view_all(SwitchBay)
+@can_view_all(Stack, Building, SwitchBay)
 def index_physical_grouping(request):
     """Affichage de la liste des stacks (affiche l'ensemble des switches)"""
     stack_list = (Stack.objects
@@ -241,8 +239,7 @@ def index_physical_grouping(request):
 
 
 @login_required
-@can_view_all(ModelSwitch)
-@can_view_all(ConstructorSwitch)
+@can_view_all(ModelSwitch, ConstructorSwitch)
 def index_model_switch(request):
     """ Affichage de l'ensemble des modèles de switches"""
     model_switch_list = ModelSwitch.objects.select_related('constructor')