From fe9164bfef8348cefe2020f6b36b4cfbc09691c3 Mon Sep 17 00:00:00 2001 From: Bartuccio Antoine Date: Wed, 16 Oct 2019 19:28:32 +0200 Subject: [PATCH 1/6] core: don't use try/except to catch type of view in permissions mixins --- core/views/__init__.py | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/core/views/__init__.py b/core/views/__init__.py index bdf76455..f6627cf7 100644 --- a/core/views/__init__.py +++ b/core/views/__init__.py @@ -172,13 +172,13 @@ class CanEditPropMixin(View): """ def dispatch(self, request, *arg, **kwargs): - try: + + if hasattr(self, "get_object"): self.object = self.get_object() - if can_edit_prop(self.object, request.user): - return super(CanEditPropMixin, self).dispatch(request, *arg, **kwargs) - return forbidden(request) - except: - pass + if not can_edit_prop(self.object, request.user): + raise PermissionDenied + return super(CanEditPropMixin, self).dispatch(request, *arg, **kwargs) + # If we get here, it's a ListView l_id = [o.id for o in self.get_queryset() if can_edit_prop(o, request.user)] if not l_id and self.get_queryset().count() != 0: @@ -201,13 +201,13 @@ class CanEditMixin(View): """ def dispatch(self, request, *arg, **kwargs): - try: + + if hasattr(self, "get_object"): self.object = self.get_object() - if can_edit(self.object, request.user): - return super(CanEditMixin, self).dispatch(request, *arg, **kwargs) - return forbidden(request) - except: - pass + if not can_edit(self.object, request.user): + raise PermissionDenied + return super(CanEditMixin, self).dispatch(request, *arg, **kwargs) + # If we get here, it's a ListView l_id = [o.id for o in self.get_queryset() if can_edit(o, request.user)] if not l_id and self.get_queryset().count() != 0: @@ -231,13 +231,12 @@ class CanViewMixin(View): def dispatch(self, request, *arg, **kwargs): - try: + if hasattr(self, "get_object"): self.object = self.get_object() - if can_view(self.object, request.user): - return super(CanViewMixin, self).dispatch(request, *arg, **kwargs) - return forbidden(request) - except: - pass + if not can_view(self.object, request.user): + raise PermissionDenied + return super(CanViewMixin, self).dispatch(request, *arg, **kwargs) + # If we get here, it's a ListView queryset = self.get_queryset() From 811809895e2cab0722e9508df7815ff0e951bcbb Mon Sep 17 00:00:00 2001 From: Bartuccio Antoine Date: Wed, 16 Oct 2019 21:21:06 +0200 Subject: [PATCH 2/6] club: fix mailing list form that unexpectedly relied on try catch in permissions --- club/tests.py | 7 ++++++- club/views.py | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/club/tests.py b/club/tests.py index 2536390e..33e6016e 100644 --- a/club/tests.py +++ b/club/tests.py @@ -641,7 +641,7 @@ class MailingFormTest(TestCase): {"action": MailingForm.ACTION_NEW_MAILING, "mailing_email": "mde"}, ) mde = Mailing.objects.get(email="mde") - response = self.client.post( + self.client.post( reverse("club:mailing", kwargs={"club_id": self.bdf.id}), { "action": MailingForm.ACTION_NEW_SUBSCRIPTION, @@ -650,6 +650,11 @@ class MailingFormTest(TestCase): "subscription_mailing": mde.id, }, ) + + response = self.client.get( + reverse("club:mailing", kwargs={"club_id": self.bdf.id}) + ) + self.assertContains(response, "comunity@git.an") self.assertContains(response, "richard@git.an") self.assertContains(response, "krophil@git.an") diff --git a/club/views.py b/club/views.py index da9824e2..a5b3aef8 100644 --- a/club/views.py +++ b/club/views.py @@ -574,7 +574,8 @@ class ClubMailingView(ClubTabsMixin, CanEditMixin, DetailFormView): except ValidationError as validation_error: return validation_error - users_to_save.append(sub.save()) + sub.save() + users_to_save.append(sub) if cleaned_data["subscription_email"]: sub = MailingSubscription( From 241650c171628ce584bd194324ab8d65be83e84d Mon Sep 17 00:00:00 2001 From: Bartuccio Antoine Date: Wed, 16 Oct 2019 21:21:51 +0200 Subject: [PATCH 3/6] counter: fix eticket server crash induced by old permission system and fix Selling permission --- counter/models.py | 2 ++ counter/views.py | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/counter/models.py b/counter/models.py index d3d3b39a..46b7ceae 100644 --- a/counter/models.py +++ b/counter/models.py @@ -478,6 +478,8 @@ class Selling(models.Model): return user.is_owner(self.counter) and self.payment_method != "CARD" def can_be_viewed_by(self, user): + if not hasattr(self, "customer"): # Customer can be set to Null + return False return user == self.customer.user def delete(self, *args, **kwargs): diff --git a/counter/views.py b/counter/views.py index ce6693e0..8b20e036 100644 --- a/counter/views.py +++ b/counter/views.py @@ -1752,7 +1752,11 @@ class EticketPDFView(CanViewMixin, DetailView): from reportlab.graphics.barcode.qr import QrCodeWidget from reportlab.graphics import renderPDF - self.object = self.get_object() + if not ( + hasattr(self.object, "product") and hasattr(self.object.product, "eticket") + ): + raise Http404 + eticket = self.object.product.eticket user = self.object.customer.user code = "%s %s %s %s" % ( From a6088c0e4a6542f9e53c9bcf1e3330957bbfdf1d Mon Sep 17 00:00:00 2001 From: Bartuccio Antoine Date: Thu, 17 Oct 2019 11:24:51 +0200 Subject: [PATCH 4/6] core: refactor permissions mixins --- core/views/__init__.py | 112 ++++++++++++++++++----------------------- 1 file changed, 50 insertions(+), 62 deletions(-) diff --git a/core/views/__init__.py b/core/views/__init__.py index f6627cf7..f9ae590f 100644 --- a/core/views/__init__.py +++ b/core/views/__init__.py @@ -140,6 +140,50 @@ def can_view(obj, user): return can_edit(obj, user) +class GenericContentPermission(View): + """ + Used to build permission mixins + This view protect any child view that would be showing an object that is restricted based + on two properties + + :prop permission_function: function to test permission with, takes an object and an user an return a bool + :prop raised_error: permission to be raised + + :raises: raised_error + """ + + permission_function = lambda obj, user: False + raised_error = PermissionDenied + + @classmethod + def get_permission_function(cls, obj, user): + return cls.permission_function(obj, user) + + def dispatch(self, request, *arg, **kwargs): + + if hasattr(self, "get_object") and callable(self.get_object): + self.object = self.get_object() + if not self.get_permission_function(self.object, request.user): + raise self.raised_error + return super(GenericContentPermission, self).dispatch( + request, *arg, **kwargs + ) + + # If we get here, it's a ListView + + queryset = self.get_queryset() + l_id = [o.id for o in queryset if self.get_permission_function(o, request.user)] + if not l_id and queryset.count() != 0: + raise self.raised_error + self._get_queryset = self.get_queryset + + def get_qs(self2): + return self2._get_queryset().filter(id__in=l_id) + + self.get_queryset = types.MethodType(get_qs, self) + return super(GenericContentPermission, self).dispatch(request, *arg, **kwargs) + + class CanCreateMixin(View): """ This view is made to protect any child view that would create an object, and thus, that can not be protected by any @@ -161,7 +205,7 @@ class CanCreateMixin(View): raise PermissionDenied -class CanEditPropMixin(View): +class CanEditPropMixin(GenericContentPermission): """ This view is made to protect any child view that would be showing some properties of an object that are restricted to only the owner group of the given object. @@ -171,28 +215,10 @@ class CanEditPropMixin(View): :raises: PermissionDenied """ - def dispatch(self, request, *arg, **kwargs): - - if hasattr(self, "get_object"): - self.object = self.get_object() - if not can_edit_prop(self.object, request.user): - raise PermissionDenied - return super(CanEditPropMixin, self).dispatch(request, *arg, **kwargs) - - # If we get here, it's a ListView - l_id = [o.id for o in self.get_queryset() if can_edit_prop(o, request.user)] - if not l_id and self.get_queryset().count() != 0: - raise PermissionDenied - self._get_queryset = self.get_queryset - - def get_qs(self2): - return self2._get_queryset().filter(id__in=l_id) - - self.get_queryset = types.MethodType(get_qs, self) - return super(CanEditPropMixin, self).dispatch(request, *arg, **kwargs) + permission_function = can_edit_prop -class CanEditMixin(View): +class CanEditMixin(GenericContentPermission): """ This view makes exactly the same thing as its direct parent, but checks the group on the edit_groups field of the object @@ -200,28 +226,10 @@ class CanEditMixin(View): :raises: PermissionDenied """ - def dispatch(self, request, *arg, **kwargs): - - if hasattr(self, "get_object"): - self.object = self.get_object() - if not can_edit(self.object, request.user): - raise PermissionDenied - return super(CanEditMixin, self).dispatch(request, *arg, **kwargs) - - # If we get here, it's a ListView - l_id = [o.id for o in self.get_queryset() if can_edit(o, request.user)] - if not l_id and self.get_queryset().count() != 0: - raise PermissionDenied - self._get_queryset = self.get_queryset - - def get_qs(self2): - return self2._get_queryset().filter(id__in=l_id) - - self.get_queryset = types.MethodType(get_qs, self) - return super(CanEditMixin, self).dispatch(request, *arg, **kwargs) + permission_function = can_edit -class CanViewMixin(View): +class CanViewMixin(GenericContentPermission): """ This view still makes exactly the same thing as its direct parent, but checks the group on the view_groups field of the object @@ -229,27 +237,7 @@ class CanViewMixin(View): :raises: PermissionDenied """ - def dispatch(self, request, *arg, **kwargs): - - if hasattr(self, "get_object"): - self.object = self.get_object() - if not can_view(self.object, request.user): - raise PermissionDenied - return super(CanViewMixin, self).dispatch(request, *arg, **kwargs) - - # If we get here, it's a ListView - queryset = self.get_queryset() - - l_id = [o.id for o in queryset if can_view(o, request.user)] - if not l_id and queryset.count() != 0: - raise PermissionDenied - self._get_queryset = self.get_queryset - - def get_qs(self2): - return self2._get_queryset().filter(id__in=l_id) - - self.get_queryset = types.MethodType(get_qs, self) - return super(CanViewMixin, self).dispatch(request, *arg, **kwargs) + permission_function = can_view class FormerSubscriberMixin(View): From 566dcc7aeee71f3761b47c235330c99adfd86d25 Mon Sep 17 00:00:00 2001 From: Bartuccio Antoine Date: Thu, 17 Oct 2019 11:11:13 +0200 Subject: [PATCH 5/6] counter: fix Selling view permission --- counter/models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/counter/models.py b/counter/models.py index 46b7ceae..aff1f2b5 100644 --- a/counter/models.py +++ b/counter/models.py @@ -478,7 +478,9 @@ class Selling(models.Model): return user.is_owner(self.counter) and self.payment_method != "CARD" def can_be_viewed_by(self, user): - if not hasattr(self, "customer"): # Customer can be set to Null + if ( + not hasattr(self, "customer") or self.customer is None + ): # Customer can be set to Null return False return user == self.customer.user From 92784193450fe2c701171e8b9b9b9acda4e5a49f Mon Sep 17 00:00:00 2001 From: Bartuccio Antoine Date: Thu, 17 Oct 2019 11:56:02 +0200 Subject: [PATCH 6/6] core: rename GenericContentPermission into GenericContentPermissionMixinBuilder --- core/views/__init__.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/core/views/__init__.py b/core/views/__init__.py index f9ae590f..a99a768a 100644 --- a/core/views/__init__.py +++ b/core/views/__init__.py @@ -140,7 +140,7 @@ def can_view(obj, user): return can_edit(obj, user) -class GenericContentPermission(View): +class GenericContentPermissionMixinBuilder(View): """ Used to build permission mixins This view protect any child view that would be showing an object that is restricted based @@ -165,7 +165,7 @@ class GenericContentPermission(View): self.object = self.get_object() if not self.get_permission_function(self.object, request.user): raise self.raised_error - return super(GenericContentPermission, self).dispatch( + return super(GenericContentPermissionMixinBuilder, self).dispatch( request, *arg, **kwargs ) @@ -181,7 +181,9 @@ class GenericContentPermission(View): return self2._get_queryset().filter(id__in=l_id) self.get_queryset = types.MethodType(get_qs, self) - return super(GenericContentPermission, self).dispatch(request, *arg, **kwargs) + return super(GenericContentPermissionMixinBuilder, self).dispatch( + request, *arg, **kwargs + ) class CanCreateMixin(View): @@ -205,7 +207,7 @@ class CanCreateMixin(View): raise PermissionDenied -class CanEditPropMixin(GenericContentPermission): +class CanEditPropMixin(GenericContentPermissionMixinBuilder): """ This view is made to protect any child view that would be showing some properties of an object that are restricted to only the owner group of the given object. @@ -218,7 +220,7 @@ class CanEditPropMixin(GenericContentPermission): permission_function = can_edit_prop -class CanEditMixin(GenericContentPermission): +class CanEditMixin(GenericContentPermissionMixinBuilder): """ This view makes exactly the same thing as its direct parent, but checks the group on the edit_groups field of the object @@ -229,7 +231,7 @@ class CanEditMixin(GenericContentPermission): permission_function = can_edit -class CanViewMixin(GenericContentPermission): +class CanViewMixin(GenericContentPermissionMixinBuilder): """ This view still makes exactly the same thing as its direct parent, but checks the group on the view_groups field of the object