From d0b1a4930014d5e7310312e4b416163664d98073 Mon Sep 17 00:00:00 2001 From: imperosol Date: Mon, 13 Jan 2025 17:31:04 +0100 Subject: [PATCH] deprecate `CanCreateMixin` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Les motifs de cette déprécation sont indiqués dans la documentation. Le mixin a été remplacé par `PermissionRequiredMixin` dans les endroits où ce remplacement était aisé. --- accounting/views.py | 7 +++++-- club/views.py | 3 ++- com/tests/test_views.py | 6 +++--- core/auth/mixins.py | 19 ++++++++++++++++++ core/management/commands/populate.py | 4 +++- core/tests/test_core.py | 9 +++------ core/views/group.py | 6 ++++-- docs/tutorial/perms.md | 30 +++++++++++++++++++++++++--- election/views.py | 24 +++++++++------------- launderette/views.py | 14 ++++++------- pedagogy/tests/tests.py | 9 +++++---- pedagogy/views.py | 15 ++++++-------- sith/settings.py | 4 ++-- 13 files changed, 94 insertions(+), 56 deletions(-) diff --git a/accounting/views.py b/accounting/views.py index d379b60d..f9fd8412 100644 --- a/accounting/views.py +++ b/accounting/views.py @@ -17,6 +17,7 @@ import collections from django import forms from django.conf import settings +from django.contrib.auth.mixins import PermissionRequiredMixin from django.core.exceptions import PermissionDenied, ValidationError from django.db import transaction from django.db.models import Sum @@ -86,12 +87,13 @@ class SimplifiedAccountingTypeEditView(CanViewMixin, UpdateView): template_name = "core/edit.jinja" -class SimplifiedAccountingTypeCreateView(CanCreateMixin, CreateView): +class SimplifiedAccountingTypeCreateView(PermissionRequiredMixin, CreateView): """Create an accounting type (for the admins).""" model = SimplifiedAccountingType fields = ["label", "accounting_type"] template_name = "core/create.jinja" + permission_required = "accounting.add_simplifiedaccountingtype" # Accounting types @@ -113,12 +115,13 @@ class AccountingTypeEditView(CanViewMixin, UpdateView): template_name = "core/edit.jinja" -class AccountingTypeCreateView(CanCreateMixin, CreateView): +class AccountingTypeCreateView(PermissionRequiredMixin, CreateView): """Create an accounting type (for the admins).""" model = AccountingType fields = ["code", "label", "movement_type"] template_name = "core/create.jinja" + permission_required = "accounting.add_accountingtype" # BankAccount views diff --git a/club/views.py b/club/views.py index 498e16e1..de5ccaee 100644 --- a/club/views.py +++ b/club/views.py @@ -473,13 +473,14 @@ class ClubEditPropView(ClubTabsMixin, CanEditPropMixin, UpdateView): current_tab = "props" -class ClubCreateView(CanCreateMixin, CreateView): +class ClubCreateView(PermissionRequiredMixin, CreateView): """Create a club (for the Sith admin).""" model = Club pk_url_kwarg = "club_id" fields = ["name", "unix_name", "parent"] template_name = "core/edit.jinja" + permission_required = "club.add_club" class MembershipSetOldView(CanEditMixin, DetailView): diff --git a/com/tests/test_views.py b/com/tests/test_views.py index c526dee5..100a83ef 100644 --- a/com/tests/test_views.py +++ b/com/tests/test_views.py @@ -159,13 +159,13 @@ class TestNews(TestCase): def test_news_viewer(self): """Test that moderated news can be viewed by anyone - and not moderated news only by com admins. + and not moderated news only by com admins and by their author. """ - # by default a news isn't moderated + # by default news aren't moderated assert self.new.can_be_viewed_by(self.com_admin) + assert self.new.can_be_viewed_by(self.author) assert not self.new.can_be_viewed_by(self.sli) assert not self.new.can_be_viewed_by(self.anonymous) - assert not self.new.can_be_viewed_by(self.author) self.new.is_moderated = True self.new.save() diff --git a/core/auth/mixins.py b/core/auth/mixins.py index 422d7bce..c25ecb57 100644 --- a/core/auth/mixins.py +++ b/core/auth/mixins.py @@ -23,6 +23,7 @@ # import types +import warnings from typing import TYPE_CHECKING, Any, LiteralString from django.contrib.auth.mixins import AccessMixin, PermissionRequiredMixin @@ -148,6 +149,24 @@ class CanCreateMixin(View): to create the object of the view. """ + def __init_subclass__(cls, **kwargs): + warnings.warn( + f"{cls.__name__} is deprecated and should be replaced " + "by other permission verification mecanism.", + DeprecationWarning, + stacklevel=2, + ) + super().__init_subclass__(**kwargs) + + def __init__(self, *args, **kwargs): + warnings.warn( + f"{self.__class__.__name__} is deprecated and should be replaced " + "by other permission verification mecanism.", + DeprecationWarning, + stacklevel=2, + ) + super().__init__(*args, **kwargs) + def dispatch(self, request, *arg, **kwargs): res = super().dispatch(request, *arg, **kwargs) if not request.user.is_authenticated: diff --git a/core/management/commands/populate.py b/core/management/commands/populate.py index a5131d64..5e0f099d 100644 --- a/core/management/commands/populate.py +++ b/core/management/commands/populate.py @@ -894,7 +894,9 @@ Welcome to the wiki page! public_group = Group.objects.create(name="Public") subscribers = Group.objects.create(name="Subscribers") - subscribers.permissions.add(*list(perms.filter(codename__in=["add_news"]))) + subscribers.permissions.add( + *list(perms.filter(codename__in=["add_news", "add_uvcommentreport"])) + ) old_subscribers = Group.objects.create(name="Old subscribers") old_subscribers.permissions.add( *list( diff --git a/core/tests/test_core.py b/core/tests/test_core.py index 878db4e4..e6b37e5c 100644 --- a/core/tests/test_core.py +++ b/core/tests/test_core.py @@ -327,12 +327,9 @@ http://git.an class TestUserTools: def test_anonymous_user_unauthorized(self, client): """An anonymous user shouldn't have access to the tools page.""" - response = client.get(reverse("core:user_tools")) - assertRedirects( - response, - expected_url="/login?next=%2Fuser%2Ftools%2F", - target_status_code=301, - ) + url = reverse("core:user_tools") + response = client.get(url) + assertRedirects(response, expected_url=reverse("core:login") + f"?next={url}") @pytest.mark.parametrize("username", ["guy", "root", "skia", "comunity"]) def test_page_is_working(self, client, username): diff --git a/core/views/group.py b/core/views/group.py index afd6874d..e17db138 100644 --- a/core/views/group.py +++ b/core/views/group.py @@ -16,12 +16,13 @@ """Views to manage Groups.""" from django import forms +from django.contrib.auth.mixins import PermissionRequiredMixin from django.urls import reverse_lazy from django.utils.translation import gettext_lazy as _ from django.views.generic import ListView from django.views.generic.edit import CreateView, DeleteView, UpdateView -from core.auth.mixins import CanCreateMixin, CanEditMixin +from core.auth.mixins import CanEditMixin from core.models import Group, User from core.views import DetailFormView from core.views.widgets.select import AutoCompleteSelectMultipleUser @@ -74,13 +75,14 @@ class GroupEditView(CanEditMixin, UpdateView): fields = ["name", "description"] -class GroupCreateView(CanCreateMixin, CreateView): +class GroupCreateView(PermissionRequiredMixin, CreateView): """Add a new Group.""" model = Group queryset = Group.objects.filter(is_manually_manageable=True) template_name = "core/create.jinja" fields = ["name", "description"] + permission_required = "core.add_group" class GroupTemplateView(CanEditMixin, DetailFormView): diff --git a/docs/tutorial/perms.md b/docs/tutorial/perms.md index 0100f24c..8a84fda7 100644 --- a/docs/tutorial/perms.md +++ b/docs/tutorial/perms.md @@ -173,13 +173,37 @@ class ArticlesCreateView(CanCreateMixin, CreateView): Les mixins suivants sont implémentés : - [CanCreateMixin][core.auth.mixins.CanCreateMixin] : l'utilisateur peut-il créer l'objet ? + Ce mixin existe, mais est déprécié et ne doit plus être utilisé ! - [CanEditPropMixin][core.auth.mixins.CanEditPropMixin] : l'utilisateur peut-il éditer les propriétés de l'objet ? - [CanEditMixin][core.auth.mixins.CanEditMixin] : L'utilisateur peut-il éditer l'objet ? - [CanViewMixin][core.auth.mixins.CanViewMixin] : L'utilisateur peut-il voir l'objet ? - [FormerSubscriberMixin][core.auth.mixins.FormerSubscriberMixin] : L'utilisateur a-t-il déjà été cotisant ? -- [PermissionOrAuthorRequiredMixin][core.auth.mixins.PermissionOrAuthorRequiredMixin] : - L'utilisateur a-t-il la permission requise, ou bien est-il l'auteur de l'objet - auquel on veut accéder ? + +!!!danger "CanCreateMixin" + + L'usage de `CanCreateMixin` est dangereux et ne doit en aucun cas être + étendu. + La façon dont ce mixin marche est qu'il valide le formulaire + de création et crée l'objet sans le persister en base de données, puis + vérifie les droits sur cet objet non-persisté. + Le danger de ce système vient de multiples raisons : + - Les vérifications se faisant sur un objet non persisté, + l'utilisation de mécanismes nécessitant une persistance préalable + peut mener à des comportements indésirés, voire à des erreurs. + - Les développeurs de django ayant tendance à restreindre progressivement + les actions qui peuvent être faites sur des objets non-persistés, + les mises-à-jour de django deviennent plus compliquées. + - La vérification des droits ne se fait que dans les requêtes POST, + à la toute fin de la requête. + Tout ce qui arrive avant n'est absolument pas protégé. + Toute opération (même les suppressions et les créations) qui ont + lieu avant la persistance de l'objet seront appliquées, + même sans permission. + - Si un développeur du site fait l'erreur de surcharger + la méthode `form_valid` (ce qui est plutôt courant, + lorsqu'on veut accomplir certaines actions + quand un formulaire est valide), on peut se retrouver + dans une situation où l'objet est persisté sans aucune protection. !!!danger "Performance" diff --git a/election/views.py b/election/views.py index 1b5439f3..cd367b63 100644 --- a/election/views.py +++ b/election/views.py @@ -1,6 +1,7 @@ from typing import TYPE_CHECKING from django import forms +from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin from django.core.exceptions import PermissionDenied from django.db import transaction from django.db.models.query import QuerySet @@ -300,7 +301,7 @@ class VoteFormView(CanCreateMixin, FormView): # Create views -class CandidatureCreateView(CanCreateMixin, CreateView): +class CandidatureCreateView(LoginRequiredMixin, CreateView): """View dedicated to a cundidature creation.""" form_class = CandidateForm @@ -326,12 +327,13 @@ class CandidatureCreateView(CanCreateMixin, CreateView): def form_valid(self, form): """Verify that the selected user is in candidate group.""" obj = form.instance - obj.election = Election.objects.get(id=self.election.id) - obj.user = obj.user if hasattr(obj, "user") else self.request.user + obj.election = self.election + if not hasattr(obj, "user"): + obj.user = self.request.user if (obj.election.can_candidate(obj.user)) and ( obj.user == self.request.user or self.can_edit ): - return super(CreateView, self).form_valid(form) + return super().form_valid(form) raise PermissionDenied def get_context_data(self, **kwargs): @@ -343,22 +345,14 @@ class CandidatureCreateView(CanCreateMixin, CreateView): return reverse_lazy("election:detail", kwargs={"election_id": self.election.id}) -class ElectionCreateView(CanCreateMixin, CreateView): +class ElectionCreateView(PermissionRequiredMixin, CreateView): model = Election form_class = ElectionForm template_name = "core/create.jinja" - - def dispatch(self, request, *args, **kwargs): - if not request.user.is_subscribed: - raise PermissionDenied - return super().dispatch(request, *args, **kwargs) - - def form_valid(self, form): - """Allow every user that had passed the dispatch to create an election.""" - return super(CreateView, self).form_valid(form) + permission_required = "election.add_election" def get_success_url(self, **kwargs): - return reverse_lazy("election:detail", kwargs={"election_id": self.object.id}) + return reverse("election:detail", kwargs={"election_id": self.object.id}) class RoleCreateView(CanCreateMixin, CreateView): diff --git a/launderette/views.py b/launderette/views.py index be2bfceb..92a81dad 100644 --- a/launderette/views.py +++ b/launderette/views.py @@ -19,6 +19,7 @@ from datetime import timezone as tz from django import forms from django.conf import settings +from django.contrib.auth.mixins import PermissionRequiredMixin from django.db import transaction from django.template import defaultfilters from django.urls import reverse_lazy @@ -28,12 +29,7 @@ from django.views.generic import DetailView, ListView, TemplateView from django.views.generic.edit import BaseFormView, CreateView, DeleteView, UpdateView from club.models import Club -from core.auth.mixins import ( - CanCreateMixin, - CanEditMixin, - CanEditPropMixin, - CanViewMixin, -) +from core.auth.mixins import CanEditMixin, CanEditPropMixin, CanViewMixin from core.models import Page, User from counter.forms import GetUserForm from counter.models import Counter, Customer, Selling @@ -191,12 +187,13 @@ class LaunderetteEditView(CanEditPropMixin, UpdateView): template_name = "core/edit.jinja" -class LaunderetteCreateView(CanCreateMixin, CreateView): +class LaunderetteCreateView(PermissionRequiredMixin, CreateView): """Create a new launderette.""" model = Launderette fields = ["name"] template_name = "core/create.jinja" + permission_required = "launderette.add_launderette" def form_valid(self, form): club = Club.objects.filter( @@ -497,12 +494,13 @@ class MachineDeleteView(CanEditPropMixin, DeleteView): success_url = reverse_lazy("launderette:launderette_list") -class MachineCreateView(CanCreateMixin, CreateView): +class MachineCreateView(PermissionRequiredMixin, CreateView): """Create a new machine.""" model = Machine fields = ["name", "launderette", "type"] template_name = "core/create.jinja" + permission_required = "launderette.add_machine" def get_initial(self): ret = super().get_initial() diff --git a/pedagogy/tests/tests.py b/pedagogy/tests/tests.py index cc36f3c3..6e04b949 100644 --- a/pedagogy/tests/tests.py +++ b/pedagogy/tests/tests.py @@ -26,6 +26,7 @@ from django.conf import settings from django.test import Client, TestCase from django.urls import reverse from django.utils.translation import gettext_lazy as _ +from pytest_django.asserts import assertRedirects from core.models import Notification, User from pedagogy.models import UV, UVComment, UVCommentReport @@ -106,7 +107,7 @@ class TestUVCreation(TestCase): def test_create_uv_unauthorized_fail(self): # Test with anonymous user response = self.client.post(self.create_uv_url, create_uv_template(0)) - assert response.status_code == 403 + assertRedirects(response, reverse("core:login") + f"?next={self.create_uv_url}") # Test with subscribed user self.client.force_login(self.sli) @@ -815,11 +816,11 @@ class TestUVCommentReportCreate(TestCase): self.create_report_test("guy", success=False) def test_create_report_anonymous_fail(self): + url = reverse("pedagogy:comment_report", kwargs={"comment_id": self.comment.id}) response = self.client.post( - reverse("pedagogy:comment_report", kwargs={"comment_id": self.comment.id}), - {"comment": self.comment.id, "reporter": 0, "reason": "C'est moche"}, + url, {"comment": self.comment.id, "reporter": 0, "reason": "C'est moche"} ) - assert response.status_code == 403 + assertRedirects(response, reverse("core:login") + f"?next={url}") assert not UVCommentReport.objects.all().exists() def test_notifications(self): diff --git a/pedagogy/views.py b/pedagogy/views.py index 770dc2a4..88e9c186 100644 --- a/pedagogy/views.py +++ b/pedagogy/views.py @@ -22,7 +22,7 @@ # from django.conf import settings -from django.contrib.auth.mixins import LoginRequiredMixin +from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin from django.core.exceptions import PermissionDenied from django.db.models import Exists, OuterRef from django.shortcuts import get_object_or_404 @@ -35,12 +35,7 @@ from django.views.generic import ( UpdateView, ) -from core.auth.mixins import ( - CanCreateMixin, - CanEditPropMixin, - CanViewMixin, - FormerSubscriberMixin, -) +from core.auth.mixins import CanEditPropMixin, CanViewMixin, FormerSubscriberMixin from core.models import Notification, User from core.views import DetailFormView from pedagogy.forms import ( @@ -136,12 +131,13 @@ class UVGuideView(LoginRequiredMixin, FormerSubscriberMixin, TemplateView): } -class UVCommentReportCreateView(CanCreateMixin, CreateView): +class UVCommentReportCreateView(PermissionRequiredMixin, CreateView): """Create a new report for an inapropriate comment.""" model = UVCommentReport form_class = UVCommentReportForm template_name = "core/edit.jinja" + permission_required = "pedagogy.add_uvcommentreport" def dispatch(self, request, *args, **kwargs): self.uv_comment = get_object_or_404(UVComment, pk=kwargs["comment_id"]) @@ -202,12 +198,13 @@ class UVModerationFormView(FormView): return reverse_lazy("pedagogy:moderation") -class UVCreateView(CanCreateMixin, CreateView): +class UVCreateView(PermissionRequiredMixin, CreateView): """Add a new UV (Privileged).""" model = UV form_class = UVForm template_name = "pedagogy/uv_edit.jinja" + permission_required = "pedagogy.add_uv" def get_form_kwargs(self): kwargs = super().get_form_kwargs() diff --git a/sith/settings.py b/sith/settings.py index 9929995e..d3acabec 100644 --- a/sith/settings.py +++ b/sith/settings.py @@ -292,8 +292,8 @@ STORAGES = { AUTH_USER_MODEL = "core.User" AUTH_ANONYMOUS_MODEL = "core.models.AnonymousUser" AUTHENTICATION_BACKENDS = ["core.auth.backends.SithModelBackend"] -LOGIN_URL = "/login" -LOGOUT_URL = "/logout" +LOGIN_URL = "/login/" +LOGOUT_URL = "/logout/" LOGIN_REDIRECT_URL = "/" DEFAULT_FROM_EMAIL = "bibou@git.an" SITH_COM_EMAIL = "bibou_com@git.an"