Sécurité: activation de l'intergiciel `LoginRequiredMiddleware` #5178

xavfernandez · 2024-11-29T16:00:33Z

🤔 Pourquoi ?

Pour avoir des vues protégées par défaut (en gardant en tête qu'être authentifié n'apporte aucune garantie, vu la facilité à se créer un compte).

(il reste le nettoyage des LoginRequiredMixin & login_required)

🍰 Comment ?

Les débats à trancher:

pour les Class Based Views:
- utilisation d'un décorateur sur le dispatch
- ou définition/utilisation d'un mixin pour décorer dans as_view
=> la seconde solution me semble plus propre/claire
pour les vues utilisant un user_passes_test pour rediriger les utilisateurs loggés vers une autre vue: cela rentre en conflit avec l'intergiciel, qui utilise le login_url défini par ce décorateur pour rediriger lors de l'authentification ce qui n'est pas du tout ce que l'on souhaite:
- soit utilisation de mon unset_login_redirect
- soit redéfinition d'un décorateur maison qui fait le redirect
- soit incorporation de ce check dans les différentes vues
=> je serais plutôt en faveur d'un décorateur maison qui aurait le mérite, comme user_passes_test actuellement de mettre en évidence les utilisateurs susceptibles d'accéder à cette vue)

🚨 À vérifier

Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

francoisfreitag · 2024-12-02T12:46:46Z

itou/www/apply/views/process_views.py

@@ -272,7 +273,9 @@ def details_for_company(request, job_application_id, template_name="apply/proces
    return render(request, template_name, context)


-@login_required
+# XXX this is dirty
+# Should we stop using user_passes_test ? Define our own ?


Stop using user_passes_test. De toute façon, je ne vois pas comment un employeur pourrait se retrouver sur cette vue. À mon avis, c’est du ceinture bretelle peu utile pour éviter une 404. J’aimerais autant que le système évite de rattraper silencieusement les erreurs des dévs.

La ligne date de 2021, je ne suis vraiment pas convaincu qu’elle ait déjà servi. (a532e86)

Moi j'aime bien comme préfiltre pour dire rapidement qui est censé avoir accès à cette vue.
Plutôt d'accord qu'il vaudrait mieux servir une 403/404 plutôt que de de renvoyer discrètement vers le dashboard.

Je verrais bien des décorateurs à la require_http_methods du genre def require_user_kinds(*user_kinds).
@require_user_kind(UserKind.PRESCRIBER, UserKind.EMPLOYER).
Cela sert à la fois de garde-fou et de documentation.

Si tu veux. En général on va chercher plus que ça pour les autorisations d’accès, mais ça fait une description basique des permissions 🤷

Dans l'idée le décorateur permet globalement de s'éviter le boilerplate if not has_rights(): raise PermissionDenied au début de chaque fonction (il n'y a pas ça dans les vues stats et ça nous pète à la gueule de temps en temps) mais je suis d'accord que renvoyer vers la page de login est assez inutile dans notre cas car souvent la personne est déjà connectée donc elle peux rien faire de plus (i.e. erreur 403).
Et au vu des @user_passes_test() actuel le user.is_active pourrais probablement être généralisé, et ensuite on ne check que le UserKind (modulo les is_prescriber_with_authorized_org), et l'avantage des décorateurs c'est qu'on peux facilement les empiler au besoin.

Actuellement on utilise 3 infos dans nos user_passes_test:

le kind (via is_prescriber, etc)

is_superuser

is_prescriber_with_authorized_org
Donc je serais plutôt pour garder une syntaxe similaire à user_passes_test, ou à défaut lister les attributs à vérifier :
@require_user_passes(["is_employer", "is_prescriber_with_authorized_org"]

Par contre, en effet, la redirection vers login_url ou vers le dashboard n'est pas pertinente, autant lever une 404 et se rendre compte qu'il y a un trou dans la passoire quelque part.

francoisfreitag · 2024-12-02T12:49:02Z

itou/www/signup/views.py

@@ -266,6 +278,10 @@ class CompanyUserView(CompanyBaseView, TemplateView):

    template_name = "signup/employer.html"

+    @login_not_required


À se demander si on ne voudrait pas un LoginNotRequiredMixin ? (réponse C de la 1ere question 😛)

Tu veux dire la réponse B/ce Mixin ?

Oui, mis à part que j’aurais décoré dispatch pour être symmétrique avec le LoginRequiredMixin de Django. 🤷

Alors oui et non.
Le LoginRequiredMixin fait des choses dans le dispatch.
Alors que le LoginNotRequiredMixin.dispatch ne ferait rien.
Le but est vraiment de mettre l'attribut login_required à False sur la vue retournée par as_view() pour que LoginRequiredMiddleware la laisse tranquille.
Et Django autorise de décorer la fonction dispatch mais à la fin il fait juste https://github.com/django/django/blob/5.1.3/django/views/generic/base.py#L115-L117 🙈

👍 pour le mixin, je trouve que ça fait bizarre de devoir utiliser un décorateur sur une méthode très spécifique (et pas forcément surcharger) dans le cas des CBV.

Hummm, merci pour l’explication. Dans ce cas, 2. (as_view) me semble également préférable.

rsebille

Vu qu'on à clairement plus de vue connectée que non-connectée ça me semble OK comme logique.

rsebille · 2024-12-02T21:49:10Z

itou/www/signup/views.py

@@ -266,6 +278,10 @@ class CompanyUserView(CompanyBaseView, TemplateView):

    template_name = "signup/employer.html"

+    @login_not_required


👍 pour le mixin, je trouve que ça fait bizarre de devoir utiliser un décorateur sur une méthode très spécifique (et pas forcément surcharger) dans le cas des CBV.

rsebille · 2024-12-02T21:58:07Z

itou/www/apply/views/process_views.py

@@ -272,7 +273,9 @@ def details_for_company(request, job_application_id, template_name="apply/proces
    return render(request, template_name, context)


-@login_required
+# XXX this is dirty
+# Should we stop using user_passes_test ? Define our own ?


Dans l'idée le décorateur permet globalement de s'éviter le boilerplate if not has_rights(): raise PermissionDenied au début de chaque fonction (il n'y a pas ça dans les vues stats et ça nous pète à la gueule de temps en temps) mais je suis d'accord que renvoyer vers la page de login est assez inutile dans notre cas car souvent la personne est déjà connectée donc elle peux rien faire de plus (i.e. erreur 403).
Et au vu des @user_passes_test() actuel le user.is_active pourrais probablement être généralisé, et ensuite on ne check que le UserKind (modulo les is_prescriber_with_authorized_org), et l'avantage des décorateurs c'est qu'on peux facilement les empiler au besoin.

xavfernandez added the no-changelog Ne doit pas figurer dans le journal des changements. label Nov 29, 2024

xavfernandez self-assigned this Nov 29, 2024

xavfernandez force-pushed the xfernandez/LoginRequiredMiddleware branch from 6bcbd50 to 1fe34fa Compare December 2, 2024 09:22

xavfernandez requested review from francoisfreitag, calummackervoy, tonial and rsebille December 2, 2024 12:21

francoisfreitag reviewed Dec 2, 2024

View reviewed changes

rsebille reviewed Dec 2, 2024

View reviewed changes

xavfernandez force-pushed the xfernandez/LoginRequiredMiddleware branch 2 times, most recently from 61aeaa0 to 886545e Compare December 3, 2024 08:03

xavfernandez added 6 commits December 3, 2024 09:28

wip add LoginRequiredMiddleware

f755d31

alternative: use mixin instead of dispatch decorator

0c73855

fixup! wip add LoginRequiredMiddleware

595f1f6

adapt tests

94fcadb

fix issue with user_passes_test

0081a0d

fixup! wip add LoginRequiredMiddleware

8deac73

xavfernandez force-pushed the xfernandez/LoginRequiredMiddleware branch 3 times, most recently from 5bd0e1b to 210d8b1 Compare December 3, 2024 08:36

more mixin

d8d8806

xavfernandez force-pushed the xfernandez/LoginRequiredMiddleware branch from 210d8b1 to d8d8806 Compare December 3, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sécurité: activation de l'intergiciel `LoginRequiredMiddleware` #5178

Sécurité: activation de l'intergiciel `LoginRequiredMiddleware` #5178

xavfernandez commented Nov 29, 2024 •

edited

Loading

francoisfreitag Dec 2, 2024

xavfernandez Dec 2, 2024 •

edited

Loading

francoisfreitag Dec 2, 2024

rsebille Dec 2, 2024

tonial Dec 4, 2024

francoisfreitag Dec 2, 2024

xavfernandez Dec 2, 2024

francoisfreitag Dec 2, 2024

xavfernandez Dec 2, 2024

rsebille Dec 2, 2024

francoisfreitag Dec 3, 2024

rsebille left a comment

rsebille Dec 2, 2024

rsebille Dec 2, 2024

		@@ -266,6 +278,10 @@ class CompanyUserView(CompanyBaseView, TemplateView):

		template_name = "signup/employer.html"

		@login_not_required

Sécurité: activation de l'intergiciel LoginRequiredMiddleware #5178

Are you sure you want to change the base?

Sécurité: activation de l'intergiciel LoginRequiredMiddleware #5178

Conversation

xavfernandez commented Nov 29, 2024 • edited Loading

🤔 Pourquoi ?

🍰 Comment ?

🚨 À vérifier

🏝️ Comment tester

💻 Captures d'écran

Choose a reason for hiding this comment

xavfernandez Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsebille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sécurité: activation de l'intergiciel `LoginRequiredMiddleware` #5178

Sécurité: activation de l'intergiciel `LoginRequiredMiddleware` #5178

xavfernandez commented Nov 29, 2024 •

edited

Loading

xavfernandez Dec 2, 2024 •

edited

Loading