Development #41489
ratelimiting sur les interfaces envoyant des emails
0%
Description
Inscription et rappel de mot de passe, ratelimiter selon l'adresse IP source mais aussi selon l'email demandé, pour ne pas permettre d'envoyer 50 mails à une même adresse.
Fichiers
Demandes liées
Révisions associées
Historique
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
Si DEBUG=True pas de rate-limit (c'est énervant durant les tests).
Je me demande si ce n'est pas l'occasion de reconstruire le reset de password et l'enregistrement sur la base du modèle authentic.models.Token, il suffirait de considérer que tant qu'un token est actif pour un email donné on ne doit pas en renvoyer (si c'est l'antispam qui bloque ça ne sert à rien d'envoyer 36 mails).
Ça évitera aussi qu'un lien puisse être utilisable plusieurs fois, ce qui est un souci actuellement, et ça simplifiera la mécanique coté registration et aussi reset qui se basent actuellement sur une signature.
Mis à jour par Valentin Deniaud il y a presque 4 ans
D'accord je vais aller dans cette direction.
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
Le contexte de la demande peut-être sauvegardé dans Token.content (JSONField), faudra éventuellement voir comment l'indexer si tu veux pouvoir faire une recherche Token.objects.filter(content__email=email)
ou alors rajouter un champ nonce=TextField(blank=True, null=True)
pour ce besoin (c'est peut-être plus simple et moins coûteux).
Mis à jour par Frédéric Péters il y a presque 4 ans
il suffirait de considérer que tant qu'un token est actif pour un email donné on ne doit pas en renvoyer (si c'est l'antispam qui bloque ça ne sert à rien d'envoyer 36 mails).
Ça arrive quand même qu'un premier envoi ne soit pas reçu, que du coup il soit dit à la personne d'ajouter nepasrepondre@... dans son carnet d'adresse et de réessayer la demande; là on veut que le nouveau message soit envoyé.
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
- vérifier si un jeton est déjà en cours
- afficher un warning avec l'histoire du "on vous a déjà envoyé un jeton il y a 10 minutes, ajouter nepasrepondre@ à vos spam etc..."
- mettre un bouton "renvoyer quand même", avec là du ratelimiting (3 par jours ?)
- si ça passe, invalider le jeton existant et envoyer un nouveau mail.
Pour l'IP il faudra quand même du ratelimiting, disons 10 par heure.
Mis à jour par Valentin Deniaud il y a presque 4 ans
Benjamin Dauvergne a écrit :
- mettre un bouton "renvoyer quand même", avec là du ratelimiting (3 par jours ?)
Pour l'IP il faudra quand même du ratelimiting, disons 10 par heure.
Ce qui commence à limiter l'utilité du jeton, il lui reste l'avantage de permettre l'usage unique.
Et là si on distingue les deux cas :- Pour les mails de création de compte, on génère un token signé à la main etc, pas d'usage unique
- Pour les mails de reset de mot de passe, on utilise ce que propose django (django.contrib.auth.tokens.PasswordResetTokenGenerator), et ça gère l'usage unique (de façon très jolie https://www.sjoerdlangkemper.nl/2016/04/07/djangos-reset-password-mechanism/)
Conclusion, il n'y a pas d'intérêt à utiliser le système de token d'authentic pour les mails de reset. Ou à la limite pour avoir un truc cohérent, genre faire la même chose partout. Sauf qu'à lire la doc il est possible de partir dans la direction opposée : https://docs.djangoproject.com/en/1.11/topics/auth/default/#django.contrib.auth.views.PasswordResetView, introduit en 1.11 donc probablement pas considéré au moment de coder le reset dans a2. C'est-à-dire qu'on pourrait choisir de plutôt se rapprocher de ce que propose django par défaut, et qu'il y a peut-être possibilité de supprimer pas mal de code se faisant ?
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
Valentin Deniaud a écrit :
- Pour les mails de reset de mot de passe, on utilise ce que propose django (django.contrib.auth.tokens.PasswordResetTokenGenerator), et ça gère l'usage unique (de façon très jolie https://www.sjoerdlangkemper.nl/2016/04/07/djangos-reset-password-mechanism/)
Ça simplifie bien oui, mais ça ne permet pas de savoir qu'on en a déjà envoyé un à la même adresse, ou alors on peut ajouter un cookie.
Conclusion, il n'y a pas d'intérêt à utiliser le système de token d'authentic pour les mails de reset. Ou à la limite pour avoir un truc cohérent, genre faire la même chose partout. Sauf qu'à lire la doc il est possible de partir dans la direction opposée : https://docs.djangoproject.com/en/1.11/topics/auth/default/#django.contrib.auth.views.PasswordResetView, introduit en 1.11 donc probablement pas considéré au moment de coder le reset dans a2. C'est-à-dire qu'on pourrait choisir de plutôt se rapprocher de ce que propose django par défaut, et qu'il y a peut-être possibilité de supprimer pas mal de code se faisant ?
Si tu peux vérifier que ça fournit les mêmes fonctionnalités sans trop surcharger et qu'on peut y ajouter le ratelimiting et éventuellement la vérification du dernier envoi, pourquoi pas; faut qu'on évite d'avoir à modifier les templates de mail et des vues déjà.
Mis à jour par Valentin Deniaud il y a presque 4 ans
Je ne m'en suis pas sorti pour revenir au code de django de reset du mot de passe, mais ça m'a quand même l'air possible et souhaitable. Bref je remets ça à Demain, pour qu'il le donne à Jamais, et back to the ticket.
Benjamin Dauvergne a écrit :
Allons bon, alors il faudrait prévoir un truc comme ça :
- vérifier si un jeton est déjà en cours
Considérons que la durée de vie du cache des essais est inférieure ou égale à celle du jeton, on a donc pas besoin de s'embêter à vérifier ça en premier lieu.
- afficher un warning avec l'histoire du "on vous a déjà envoyé un jeton il y a 10 minutes, ajouter nepasrepondre@ à vos spam etc..."
Ce texte est déjà présent après la validation du formulaire, il ne sert à rien de le remettre dans un warning.
- mettre un bouton "renvoyer quand même", avec là du ratelimiting (3 par jours ?)
Je me passerais bien de l'ajout d'un bouton.
- si ça passe, invalider le jeton existant et envoyer un nouveau mail.
Pour l'IP il faudra quand même du ratelimiting, disons 10 par heure.
Ça me parait relou d'avoir un délai en jours et l'autre en heures, des paramètres aussi similaires devraient avoir la même unité.
Donc nouveau plan :- Mettre du ratelimiting sur la création de compte et le reset du mot de passe, avec les mêmes paramètres
- 10 par heure par IP, 3 par heure par adresse mail, tout ça mis dans les settings
- Message d'erreur générique « réessayer plus tard », en se disant qu'il n'y a pas de raison légitime de faire autant d'essais et qu'on a donc pas à être super gentil avec la personne en face en lui parlant précisément du délai, de la limite etc
- Switch vers le système de token pour la création de compte, parce que gagner l'usage unique c'est cool
- Mais pas touche au reset de mot de passe car token déjà à usage unique et le point suivant ne justifie pas à lui seul ce changement
- Exploitation du token pour afficher une erreur de validation « on vous a déjà envoyé un mail, si vous voulez vraiment qu'on le fasse à nouveau vous pouvez resoumettre le formulaire », et laisser passer cette resoumission.
J'ai quelques trucs en local, normalement que du code simple et des petits patches, je pousse demain.
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
Valentin Deniaud a écrit :
Donc nouveau plan :
- Mettre du ratelimiting sur la création de compte et le reset du mot de passe, avec les mêmes paramètres
- 10 par heure par IP, 3 par heure par adresse mail, tout ça mis dans les settings
- Message d'erreur générique « réessayer plus tard », en se disant qu'il n'y a pas de raison légitime de faire autant d'essais et qu'on a donc pas à être super gentil avec la personne en face en lui parlant précisément du délai, de la limite etc
C'est l'inverse la plupart des raisons sont légitimes pour l'utilisateur qui subit son SI plus qu'il ne le contrôle (antispam, PBKC,etc..) on a quasiment zéro attaque et que des gens qui se plantent, il faut les prendre par la main et leur dire ce qu'on sait pour les rassurer (« oui oui on a vu que t'as essayé 3 fois ces dix dernières minutes, mais certainement c'est ton antispam »), c'est pas de la pédanterie.
Et en bonus,
- Switch vers le système de token pour la création de compte, parce que gagner l'usage unique c'est cool
- Mais pas touche au reset de mot de passe car token déjà à usage unique et le point suivant ne justifie pas à lui seul ce changement
- Exploitation du token pour afficher une erreur de validation « on vous a déjà envoyé un mail, si vous voulez vraiment qu'on le fasse à nouveau vous pouvez resoumettre le formulaire », et laisser passer cette resoumission.
C'est dommage de ne pas l'avoir aussi sur le reset de mot de passe, le changement me parait anodin, sauf à voir des tonnes de fonctionnalités géniales coté Django (l'authentification et la gestion de compte c'est l'unique but d'authentic, c'est pas étonnant que django.contrib.auth ne sous satisfasse pas complètement en général, la réutilisation de django.contrib.auth n'est vraiment pas une politique ici).
J'ai quelques trucs en local, normalement que du code simple et des petits patches, je pousse demain.
Ok.
Mis à jour par Valentin Deniaud il y a presque 4 ans
- Fichier 0001-forms-rate-limit-registration-and-password-reset-ema.patch 0001-forms-rate-limit-registration-and-password-reset-ema.patch ajouté
- Statut changé de Nouveau à Solution proposée
- Patch proposed changé de Non à Oui
OK je suis convaincu mais comme c'est plus long et un peu le bordel de toucher à ce code, je préfère arrêter d'encombrer/ralentir ce ticket, et j'ai créé #41792.
Voilà donc le patch uniquement pour l'ajout du ratelimiting et ça sera tout pour ici.
Mis à jour par Valentin Deniaud il y a presque 4 ans
- Fichier 0001-forms-rate-limit-registration-and-password-reset-ema.patch 0001-forms-rate-limit-registration-and-password-reset-ema.patch ajouté
Benjamin Dauvergne a écrit :
Si DEBUG=True pas de rate-limit (c'est énervant durant les tests).
J'avais oublié ça, hop ajouté.
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
Mais tu n'utilises pas django-ratelimit ?
Mis à jour par Frédéric Péters il y a presque 4 ans
À propos de désactiver en mode debug, je préférerais ne pas, parce qu'on va avoir nos environnements de développements et nos déploiements de recette en debug et du coup ce code ne sera jamais effectivement jamais utilisé avant de passer en production, je me disais initialement que la remarque de Benjamin portait sur l'exécutions des tests unitaires, i.e. qu'on aurait juste un paramètre désactivant ça dans tests/settings.py.
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
Frédéric Péters a écrit :
À propos de désactiver en mode debug, je préférerais ne pas, parce qu'on va avoir nos environnements de développements et nos déploiements de recette en debug et du coup ce code ne sera jamais effectivement jamais utilisé avant de passer en production, je me disais initialement que la remarque de Benjamin portait sur l'exécutions des tests unitaires, i.e. qu'on aurait juste un paramètre désactivant ça dans tests/settings.py.
Le pauvre malheureux (généralement un client ou un CPF) qui voudra tester plus de 10 enregistrements en une heure sur une instance de test depuis la même IP sera bien embêté et les tests sont exécutés sans DEBUG sur a2 (ça gêne plus qu'autre chose). Les test unitaires et la prod sont suffisant pour savoir que ça marche, me semble-t-il.
Pour les tests unitaires le plus simple c'est de de vider le cache entre les tests, si ça pose un souci, c'est déjà fait ponctuellement mais ça devrait être la politique générale avec une fixture clear_cache(autouse=True)
dans conftest.py.
Mis à jour par Frédéric Péters il y a presque 4 ans
Le pauvre malheureux (généralement un client ou un CPF) qui voudra tester plus de 10 enregistrements en une heure sur une instance de test depuis la même IP sera bien embêté.
Il créera ces comptes depuis le backoffice, pas de soucis; vraiment je préfère avoir les fonctionnalités sur la recette parce que pour ma part, à répétition, la recette révèle des bugs, malgré les tests unitaires.
Mis à jour par Paul Marillonnet il y a presque 4 ans
- Lié à Development #41813: tests: utiliser une fixture autouse pour le nettoyage du cache ajouté
Mis à jour par Valentin Deniaud il y a presque 4 ans
Benjamin Dauvergne a écrit :
Mais tu n'utilises pas django-ratelimit ?
J'étais totalement passé à côté de cette dépendance, j'ai surtout vu que tout était from scratch dans le mécanisme de ratelimiting du login, et donc j'ai fait pareil. Je peux reprendre.
Benjamin Dauvergne a écrit :
Pour les tests unitaires le plus simple c'est de de vider le cache entre les tests, si ça pose un souci, c'est déjà fait ponctuellement mais ça devrait être la politique générale avec une fixture
clear_cache(autouse=True)
dans conftest.py.
Pour l'instant je désactive le ratelimiting dans tests/settings.py et je l'active pour les tests qui le testent.
Mis à jour par Valentin Deniaud il y a presque 4 ans
- Fichier 0001-views-ratelimit-email-form-views-41489.patch 0001-views-ratelimit-email-form-views-41489.patch ajouté
Voilà du coup c'est plus simple.
Une imprécision à noter, si on est ratelimité selon son IP, les tentatives infructueuses suivantes vont quand même incrémenter le compeur de mails envoyés à la même adresse. Idéalement ça ne devrait pas mais vu l'API fournie par django-ratelimit ça rendrait le code sensiblement plus moche, donc je néglige ce cas.
Mis à jour par Benjamin Dauvergne il y a presque 4 ans
- Statut changé de Solution proposée à Solution validée
Mis à jour par Valentin Deniaud il y a presque 4 ans
- Statut changé de Solution validée à Résolu (à déployer)
commit afcec6c51405cf967405ba017b967d5fc3e3a309 Author: Valentin Deniaud <vdeniaud@entrouvert.com> Date: Mon Apr 20 17:32:47 2020 +0200 views: ratelimit email form views (#41489)
Mis à jour par Frédéric Péters il y a presque 4 ans
- Statut changé de Résolu (à déployer) à Solution déployée
views: ratelimit email form views (#41489)