Projet

Général

Profil

Development #41489

ratelimiting sur les interfaces envoyant des emails

Ajouté par Frédéric Péters il y a presque 4 ans. Mis à jour il y a presque 4 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Catégorie:
-
Version cible:
-
Début:
08 avril 2020
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

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

Lié à Authentic 2 - Development #41813: tests: utiliser une fixture autouse pour le nettoyage du cacheFermé17 avril 2020

Actions

Révisions associées

Révision afcec6c5 (diff)
Ajouté par Valentin Deniaud il y a presque 4 ans

views: ratelimit email form views (#41489)

Historique

#1

Mis à jour par Valentin Deniaud il y a presque 4 ans

  • Assigné à mis à Valentin Deniaud
#2

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.

#4

Mis à jour par Valentin Deniaud il y a presque 4 ans

D'accord je vais aller dans cette direction.

#5

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).

#6

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é.

#7

Mis à jour par Benjamin Dauvergne il y a presque 4 ans

Allons bon, alors il faudrait prévoir un truc comme ça :
  • 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.

#8

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 :

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 ?

#9

Mis à jour par Benjamin Dauvergne il y a presque 4 ans

Valentin Deniaud a écrit :

Ç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à.

#10

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
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.

J'ai quelques trucs en local, normalement que du code simple et des petits patches, je pousse demain.

#11

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.

#12

Mis à jour par Valentin Deniaud il y a presque 4 ans

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.

#13

Mis à jour par Valentin Deniaud il y a presque 4 ans

Benjamin Dauvergne a écrit :

Si DEBUG=True pas de rate-limit (c'est énervant durant les tests).

J'avais oublié ça, hop ajouté.

#14

Mis à jour par Benjamin Dauvergne il y a presque 4 ans

Mais tu n'utilises pas django-ratelimit ?

#15

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.

#16

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.

#17

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.

#18

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é
#19

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.

#20

Mis à jour par Valentin Deniaud il y a presque 4 ans

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.

#21

Mis à jour par Benjamin Dauvergne il y a presque 4 ans

  • Statut changé de Solution proposée à Solution validée
#22

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)
#23

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

Formats disponibles : Atom PDF