From a96a2f50fae790b27c176f6a1b4987db24d27828 Mon Sep 17 00:00:00 2001 From: chapeau Date: Sun, 29 Nov 2020 17:17:42 +0100 Subject: [PATCH 1/5] =?UTF-8?q?modification=20des=20acl=20pour=20g=C3=A9re?= =?UTF-8?q?r=20les=20vues=20de=20l'api?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- re2o/acl.py | 82 ++++++++++++++++++++++++++++++++++++++------------ re2o/mixins.py | 24 ++++++++++++++- 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/re2o/acl.py b/re2o/acl.py index e73ffdfa..0a5dd1ee 100644 --- a/re2o/acl.py +++ b/re2o/acl.py @@ -35,6 +35,7 @@ from django.contrib import messages from django.shortcuts import redirect from django.urls import reverse from django.utils.translation import ugettext as _ +from rest_framework.response import Response from re2o.utils import get_group_having_permission @@ -43,11 +44,13 @@ def acl_error_message(msg, permissions): """Create an error message for msg and permissions.""" if permissions is None: return msg - groups = ", ".join([g.name for g in get_group_having_permission(*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 these groups: %s.") % groups + message + + _("You need to be a member of one of these groups: %s.") % groups ) else: return message + _("No group has the %s permission(s)!") % " or ".join( @@ -60,7 +63,7 @@ def acl_error_message(msg, permissions): # This is the function of main interest of this file. Almost all the decorators # use it, and it is a fairly complicated piece of code. Let me guide you through # this ! 🌈😸 -def acl_base_decorator(method_name, *targets, on_instance=True): +def acl_base_decorator(method_name, *targets, on_instance=True, api=False): """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 @@ -121,6 +124,9 @@ on_instance=False) method `get_instance` of the model, with the arguments originally passed to the view. + api: when set to True, errors will no longer trigger redirection and + messages but will send a 403 with errors in JSON + 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 @@ -192,7 +198,8 @@ ModelC) # and store it to pass it to the view. if on_instance: try: - target = target.get_instance(target_id, *args, **kwargs) + target = target.get_instance( + target_id, *args, **kwargs) instances.append(target) except target.DoesNotExist: # A non existing instance is a valid reason to deny @@ -238,28 +245,37 @@ ModelC) # Store the messages at the right place. for can, msg, permissions in process_target(target, fields, target_id): if not can: - error_messages.append(acl_error_message(msg, permissions)) + error_messages.append( + acl_error_message(msg, permissions)) elif msg: - warning_messages.append(acl_error_message(msg, permissions)) + warning_messages.append( + acl_error_message(msg, permissions)) # Display the warning messages - if warning_messages: - for msg in warning_messages: - messages.warning(request, msg) + if not api: + if warning_messages: + for msg in warning_messages: + messages.warning(request, msg) # If there is any error message, then the request must be denied. if error_messages: # We display the message - for msg in error_messages: - messages.error( - request, - msg or _("You don't have the right to access this menu."), - ) + if not api: + for msg in error_messages: + messages.error( + request, + msg or _( + "You don't have the right to access this menu."), + ) # And redirect the user to the right place. if request.user.id is not None: - return redirect( - reverse("users:profil", kwargs={"userid": str(request.user.id)}) - ) + if not api: + return redirect( + reverse("users:profil", kwargs={ + "userid": str(request.user.id)}) + ) + else: + return Response(data={"errors": error_messages, "warning": warning_messages}, status=403) else: return redirect(reverse("index")) return view(request, *chain(instances, args), **kwargs) @@ -328,7 +344,8 @@ def can_delete_set(model): request, _("You don't have the right to access this menu.") ) return redirect( - reverse("users:profil", kwargs={"userid": str(request.user.id)}) + reverse("users:profil", kwargs={ + "userid": str(request.user.id)}) ) return view(request, instances, *args, **kwargs) @@ -375,9 +392,36 @@ def can_edit_history(view): """ if request.user.has_perm("admin.change_logentry"): return view(request, *args, **kwargs) - messages.error(request, _("You don't have the right to edit the history.")) + messages.error(request, _( + "You don't have the right to edit the history.")) return redirect( reverse("users:profil", kwargs={"userid": str(request.user.id)}) ) return wrapper + + +def can_view_all_api(*models): + """Decorator to check if an user can see an api page + Only used on functionnal api views (class-based api views ACL are checked + in api/permissions.py) + """ + return acl_base_decorator("can_view_all", *models, on_instance=False, api=True) + + +def can_edit_all_api(*models): + """Decorator to check if an user can edit via the api + We do not always know which instances will be edited, so we may need to know + if the user can edit any instance. + Only used on functionnal api views (class-based api views ACL are checked + in api/permissions.py) + """ + return acl_base_decorator("can_edit_all", *models, on_instance=False, api=True) + + +def can_create_api(*models): + """Decorator to check if an user can create the given models. via the api + Only used on functionnal api views (class-based api views ACL are checked + in api/permissions.py) + """ + return acl_base_decorator("can_create", *models, on_instance=False, api=True) diff --git a/re2o/mixins.py b/re2o/mixins.py index 44cd517e..fbe715f0 100644 --- a/re2o/mixins.py +++ b/re2o/mixins.py @@ -61,7 +61,8 @@ class FormRevMixin(object): ) elif self.changed_data: reversion.set_comment( - "Field(s) edited: %s" % ", ".join(field for field in self.changed_data) + "Field(s) edited: %s" % ", ".join( + field for field in self.changed_data) ) return super(FormRevMixin, self).save(*args, **kwargs) @@ -187,6 +188,27 @@ class AclMixin(object): (permission,), ) + @classmethod + def can_edit_all(cls, user_request, *_args, **_kwargs): + """Check if a user can edit all instances of an object + + Parameters: + user_request: User calling for this action + + Returns: + Boolean: True if user_request has the right access to do it, else + false with reason for reject authorization + """ + permission = cls.get_modulename() + ".change_" + cls.get_classname() + can = user_request.has_perm(permission) + return ( + can, + _("You don't have the right to edit every %s object.") % cls.get_classname() + if not can + else None, + (permission,), + ) + def can_view(self, user_request, *_args, **_kwargs): """Check if a user can view an instance of an object From ba9d5211b9e2f04cbdfb4bd7154f71f1492626a3 Mon Sep 17 00:00:00 2001 From: chapeau Date: Sun, 29 Nov 2020 17:21:28 +0100 Subject: [PATCH 2/5] patch --- preferences/views.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/preferences/views.py b/preferences/views.py index 077fd105..5b5f6b03 100644 --- a/preferences/views.py +++ b/preferences/views.py @@ -96,7 +96,8 @@ def edit_options_template_function(request, section, forms, models): return redirect(reverse("preferences:display-options")) options_instance, _created = model.objects.get_or_create() - _is_allowed_to_edit, msg, permissions = options_instance.can_edit(request.user) + _is_allowed_to_edit, msg, permissions = options_instance.can_edit( + request.user) if not _is_allowed_to_edit: messages.error(request, acl_error_message(msg, permissions)) return redirect(reverse("index")) @@ -150,7 +151,7 @@ def display_options(request): optionnal_templates_list = [ app.preferences.views.aff_preferences(request) for app in optionnal_apps - if hasattr(app.preferences.views, "aff_preferences") + if hasattr(app, "preferences") and hasattr(app.preferences.views, "aff_preferences") ] return form( @@ -301,7 +302,8 @@ def add_radiuskey(request): @can_edit(RadiusKey) def edit_radiuskey(request, radiuskey_instance, **_kwargs): """View used to edit RADIUS keys.""" - radiuskey = RadiusKeyForm(request.POST or None, instance=radiuskey_instance) + radiuskey = RadiusKeyForm(request.POST or None, + instance=radiuskey_instance) if radiuskey.is_valid(): radiuskey.save() messages.success(request, _("The RADIUS key was edited.")) @@ -344,10 +346,11 @@ def add_switchmanagementcred(request): switchmanagementcred = SwitchManagementCredForm(request.POST or None) if switchmanagementcred.is_valid(): switchmanagementcred.save() - messages.success(request, _("The switch management credentials were added.")) + messages.success(request, _( + "The switch management credentials were added.")) return redirect(reverse("preferences:display-options")) return form( - {"preferenceform": switchmanagementcred, "action_name": _("Add"),}, + {"preferenceform": switchmanagementcred, "action_name": _("Add"), }, "preferences/preferences.html", request, ) @@ -361,7 +364,8 @@ def edit_switchmanagementcred(request, switchmanagementcred_instance, **_kwargs) ) if switchmanagementcred.is_valid(): switchmanagementcred.save() - messages.success(request, _("The switch management credentials were edited.")) + messages.success(request, _( + "The switch management credentials were edited.")) return redirect(reverse("preferences:display-options")) return form( {"preferenceform": switchmanagementcred, "action_name": _("Edit")}, @@ -410,7 +414,7 @@ def add_mailcontact(request): messages.success(request, _("The contact email address was created.")) return redirect(reverse("preferences:display-options")) return form( - {"preferenceform": mailcontact, "action_name": _("Add"),}, + {"preferenceform": mailcontact, "action_name": _("Add"), }, "preferences/preferences.html", request, ) @@ -438,12 +442,14 @@ def edit_mailcontact(request, mailcontact_instance, **_kwargs): @can_delete_set(MailContact) def del_mailcontact(request, instances): """View used to delete one or several contact email addresses.""" - mailcontacts = DelMailContactForm(request.POST or None, instances=instances) + mailcontacts = DelMailContactForm( + request.POST or None, instances=instances) if mailcontacts.is_valid(): mailcontacts_dels = mailcontacts.cleaned_data["mailcontacts"] for mailcontacts_del in mailcontacts_dels: mailcontacts_del.delete() - messages.success(request, _("The contact email adress was deleted.")) + messages.success(request, _( + "The contact email adress was deleted.")) return redirect(reverse("preferences:display-options")) return form( {"preferenceform": mailcontacts, "action_name": _("Delete")}, From 5a79ffb0f5dfb8759790902c297751a486594a2e Mon Sep 17 00:00:00 2001 From: chapeau Date: Sun, 29 Nov 2020 18:19:46 +0100 Subject: [PATCH 3/5] lets be sure that api permissions wont trigger on functional views --- api/permissions.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/permissions.py b/api/permissions.py index 1983bdc8..3ee61f33 100644 --- a/api/permissions.py +++ b/api/permissions.py @@ -239,6 +239,9 @@ class AutodetectACLPermission(permissions.BasePermission): if getattr(view, "_ignore_model_permissions", False): return True + if not getattr(view, "queryset", getattr(view, "get_queryset", None)): + return True + if not request.user or not request.user.is_authenticated: return False @@ -273,7 +276,8 @@ class AutodetectACLPermission(permissions.BasePermission): # they have read permissions to see 403, or not, and simply see # a 404 response. - SAFE_METHODS = ("GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE") + SAFE_METHODS = ("GET", "OPTIONS", "HEAD", + "POST", "PUT", "PATCH", "DELETE") if request.method in SAFE_METHODS: # Read permissions already checked and failed, no need From 70b492c31574a7f454d73a46c28c2de84575e15d Mon Sep 17 00:00:00 2001 From: chapeau Date: Sun, 29 Nov 2020 22:17:52 +0100 Subject: [PATCH 4/5] documentation --- api/permissions.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/permissions.py b/api/permissions.py index 3ee61f33..ab7453f4 100644 --- a/api/permissions.py +++ b/api/permissions.py @@ -239,6 +239,8 @@ class AutodetectACLPermission(permissions.BasePermission): if getattr(view, "_ignore_model_permissions", False): return True + # Bypass permission verifications if it is a functional view + # (permissions are handled by ACL) if not getattr(view, "queryset", getattr(view, "get_queryset", None)): return True From a1c08c4e968bb596af21306988ed9b9d6cc9d782 Mon Sep 17 00:00:00 2001 From: Hugo Levy-Falk Date: Mon, 28 Dec 2020 20:59:35 +0100 Subject: [PATCH 5/5] Just blacked everything. --- api/permissions.py | 3 +-- preferences/views.py | 31 +++++++++++++------------- re2o/acl.py | 52 ++++++++++++++++++++------------------------ re2o/mixins.py | 17 +++++++-------- 4 files changed, 48 insertions(+), 55 deletions(-) diff --git a/api/permissions.py b/api/permissions.py index ab7453f4..646682c4 100644 --- a/api/permissions.py +++ b/api/permissions.py @@ -278,8 +278,7 @@ class AutodetectACLPermission(permissions.BasePermission): # they have read permissions to see 403, or not, and simply see # a 404 response. - SAFE_METHODS = ("GET", "OPTIONS", "HEAD", - "POST", "PUT", "PATCH", "DELETE") + SAFE_METHODS = ("GET", "OPTIONS", "HEAD", "POST", "PUT", "PATCH", "DELETE") if request.method in SAFE_METHODS: # Read permissions already checked and failed, no need diff --git a/preferences/views.py b/preferences/views.py index 5b5f6b03..5eab39f4 100644 --- a/preferences/views.py +++ b/preferences/views.py @@ -96,8 +96,7 @@ def edit_options_template_function(request, section, forms, models): return redirect(reverse("preferences:display-options")) options_instance, _created = model.objects.get_or_create() - _is_allowed_to_edit, msg, permissions = options_instance.can_edit( - request.user) + _is_allowed_to_edit, msg, permissions = options_instance.can_edit(request.user) if not _is_allowed_to_edit: messages.error(request, acl_error_message(msg, permissions)) return redirect(reverse("index")) @@ -151,7 +150,8 @@ def display_options(request): optionnal_templates_list = [ app.preferences.views.aff_preferences(request) for app in optionnal_apps - if hasattr(app, "preferences") and hasattr(app.preferences.views, "aff_preferences") + if hasattr(app, "preferences") + and hasattr(app.preferences.views, "aff_preferences") ] return form( @@ -302,8 +302,7 @@ def add_radiuskey(request): @can_edit(RadiusKey) def edit_radiuskey(request, radiuskey_instance, **_kwargs): """View used to edit RADIUS keys.""" - radiuskey = RadiusKeyForm(request.POST or None, - instance=radiuskey_instance) + radiuskey = RadiusKeyForm(request.POST or None, instance=radiuskey_instance) if radiuskey.is_valid(): radiuskey.save() messages.success(request, _("The RADIUS key was edited.")) @@ -346,11 +345,13 @@ def add_switchmanagementcred(request): switchmanagementcred = SwitchManagementCredForm(request.POST or None) if switchmanagementcred.is_valid(): switchmanagementcred.save() - messages.success(request, _( - "The switch management credentials were added.")) + messages.success(request, _("The switch management credentials were added.")) return redirect(reverse("preferences:display-options")) return form( - {"preferenceform": switchmanagementcred, "action_name": _("Add"), }, + { + "preferenceform": switchmanagementcred, + "action_name": _("Add"), + }, "preferences/preferences.html", request, ) @@ -364,8 +365,7 @@ def edit_switchmanagementcred(request, switchmanagementcred_instance, **_kwargs) ) if switchmanagementcred.is_valid(): switchmanagementcred.save() - messages.success(request, _( - "The switch management credentials were edited.")) + messages.success(request, _("The switch management credentials were edited.")) return redirect(reverse("preferences:display-options")) return form( {"preferenceform": switchmanagementcred, "action_name": _("Edit")}, @@ -414,7 +414,10 @@ def add_mailcontact(request): messages.success(request, _("The contact email address was created.")) return redirect(reverse("preferences:display-options")) return form( - {"preferenceform": mailcontact, "action_name": _("Add"), }, + { + "preferenceform": mailcontact, + "action_name": _("Add"), + }, "preferences/preferences.html", request, ) @@ -442,14 +445,12 @@ def edit_mailcontact(request, mailcontact_instance, **_kwargs): @can_delete_set(MailContact) def del_mailcontact(request, instances): """View used to delete one or several contact email addresses.""" - mailcontacts = DelMailContactForm( - request.POST or None, instances=instances) + mailcontacts = DelMailContactForm(request.POST or None, instances=instances) if mailcontacts.is_valid(): mailcontacts_dels = mailcontacts.cleaned_data["mailcontacts"] for mailcontacts_del in mailcontacts_dels: mailcontacts_del.delete() - messages.success(request, _( - "The contact email adress was deleted.")) + messages.success(request, _("The contact email adress was deleted.")) return redirect(reverse("preferences:display-options")) return form( {"preferenceform": mailcontacts, "action_name": _("Delete")}, diff --git a/re2o/acl.py b/re2o/acl.py index 0a5dd1ee..f74427ea 100644 --- a/re2o/acl.py +++ b/re2o/acl.py @@ -44,13 +44,11 @@ def acl_error_message(msg, permissions): """Create an error message for msg and permissions.""" if permissions is None: return msg - groups = ", ".join( - [g.name for g in get_group_having_permission(*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 these groups: %s.") % groups + message + _("You need to be a member of one of these groups: %s.") % groups ) else: return message + _("No group has the %s permission(s)!") % " or ".join( @@ -181,8 +179,7 @@ ModelC) # `wrapper` inside the `decorator` function, you need to read some #  documentation on decorators ! def decorator(view): - """The decorator to use on a specific view - """ + """The decorator to use on a specific view""" def wrapper(request, *args, **kwargs): """The wrapper used for a specific request""" @@ -198,8 +195,7 @@ ModelC) # and store it to pass it to the view. if on_instance: try: - target = target.get_instance( - target_id, *args, **kwargs) + target = target.get_instance(target_id, *args, **kwargs) instances.append(target) except target.DoesNotExist: # A non existing instance is a valid reason to deny @@ -245,11 +241,9 @@ ModelC) # Store the messages at the right place. for can, msg, permissions in process_target(target, fields, target_id): if not can: - error_messages.append( - acl_error_message(msg, permissions)) + error_messages.append(acl_error_message(msg, permissions)) elif msg: - warning_messages.append( - acl_error_message(msg, permissions)) + warning_messages.append(acl_error_message(msg, permissions)) # Display the warning messages if not api: @@ -264,18 +258,24 @@ ModelC) for msg in error_messages: messages.error( request, - msg or _( - "You don't have the right to access this menu."), + msg or _("You don't have the right to access this menu."), ) # And redirect the user to the right place. if request.user.id is not None: if not api: return redirect( - reverse("users:profil", kwargs={ - "userid": str(request.user.id)}) + reverse( + "users:profil", kwargs={"userid": str(request.user.id)} + ) ) else: - return Response(data={"errors": error_messages, "warning": warning_messages}, status=403) + return Response( + data={ + "errors": error_messages, + "warning": warning_messages, + }, + status=403, + ) else: return redirect(reverse("index")) return view(request, *chain(instances, args), **kwargs) @@ -326,12 +326,10 @@ def can_delete_set(model): If none of them, return an error""" def decorator(view): - """The decorator to use on a specific view - """ + """The decorator to use on a specific view""" def wrapper(request, *args, **kwargs): - """The wrapper used for a specific request - """ + """The wrapper used for a specific request""" all_objects = model.objects.all() instances_id = [] for instance in all_objects: @@ -344,8 +342,7 @@ def can_delete_set(model): request, _("You don't have the right to access this menu.") ) return redirect( - reverse("users:profil", kwargs={ - "userid": str(request.user.id)}) + reverse("users:profil", kwargs={"userid": str(request.user.id)}) ) return view(request, instances, *args, **kwargs) @@ -373,8 +370,7 @@ def can_view_all(*targets): def can_view_app(*apps_name): - """Decorator to check if an user can view the applications. - """ + """Decorator to check if an user can view the applications.""" for app_name in apps_name: assert app_name in sys.modules.keys() return acl_base_decorator( @@ -388,12 +384,10 @@ def can_edit_history(view): """Decorator to check if an user can edit history.""" def wrapper(request, *args, **kwargs): - """The wrapper used for a specific request - """ + """The wrapper used for a specific request""" if request.user.has_perm("admin.change_logentry"): return view(request, *args, **kwargs) - messages.error(request, _( - "You don't have the right to edit the history.")) + messages.error(request, _("You don't have the right to edit the history.")) return redirect( reverse("users:profil", kwargs={"userid": str(request.user.id)}) ) diff --git a/re2o/mixins.py b/re2o/mixins.py index fbe715f0..26034e07 100644 --- a/re2o/mixins.py +++ b/re2o/mixins.py @@ -29,9 +29,9 @@ from django.utils.translation import ugettext as _ class RevMixin(object): - """ A mixin to subclass the save and delete function of a model + """A mixin to subclass the save and delete function of a model to enforce the versioning of the object before those actions - really happen """ + really happen""" def save(self, *args, **kwargs): """ Creates a version of this object and save it to database """ @@ -49,8 +49,8 @@ class RevMixin(object): class FormRevMixin(object): - """ A mixin to subclass the save function of a form - to enforce the versionning of the object before it is really edited """ + """A mixin to subclass the save function of a form + to enforce the versionning of the object before it is really edited""" def save(self, *args, **kwargs): """ Create a version of this object and save it to database """ @@ -61,8 +61,7 @@ class FormRevMixin(object): ) elif self.changed_data: reversion.set_comment( - "Field(s) edited: %s" % ", ".join( - field for field in self.changed_data) + "Field(s) edited: %s" % ", ".join(field for field in self.changed_data) ) return super(FormRevMixin, self).save(*args, **kwargs) @@ -130,7 +129,7 @@ class AclMixin(object): Parameters: user_request: User calling for this action - self: Instance to edit + self: Instance to edit Returns: Boolean: True if user_request has the right access to do it, else @@ -151,7 +150,7 @@ class AclMixin(object): Parameters: user_request: User calling for this action - self: Instance to delete + self: Instance to delete Returns: Boolean: True if user_request has the right access to do it, else @@ -214,7 +213,7 @@ class AclMixin(object): Parameters: user_request: User calling for this action - self: Instance to view + self: Instance to view Returns: Boolean: True if user_request has the right access to do it, else