Connexion: Renvoie de e-mail de confirmation #5428

calummackervoy · 2025-01-20T13:46:28Z

🤔 Pourquoi ?

Actuellement, si je m'inscris sur le site, mais que je confirme mon courriel après son expiration (dans 3 jours, par défaut), je verrai apparaître la page indiquant que le jeton n'est pas valide. Si je clique sur « nouvelle demande de confirmation », je serai en fait redirigé vers la page de connexion, ce qui est également le comportement par défaut d'allauth. Ceci peut arriver pour les nouveaux utilisateurs, mais aussi pour les utilisateurs qui a changé leur adresse e-mail.

🍰 Comment ?

Dans le cadre de mes travails à remplacer django-allauth avec notre propre logique, j'ai proposé stocké la logique dans une nouvelle application, accounts. J'ai l'intention de continuer à étendre cette application dans des futurs PRs afin de préparer le remplacement d'allauth et réduire la taille du PR qui va aufin le supprimer.

⚠️ Concernes de sécurité

Avec ce PR on signale effectivement si un lien de confirmation est totalement invalide ou si c'est expiré.
On empêche l'envoie des e-mails de confirmations à une adresse déjà vérifiée.
On empêche l’envoie de plus de 3 e-mails par jour (protéction contre DoS).

Je ne peux pas réutiliser les liens de confirmation, et je ne suis pas connecté avec le lien de confirmation.

🚨 À vérifier

Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Tu peux créer une confirmation expirée dans le shell et visiter l'URL récupéré :

from freezegun import freeze_time
from allauth.account.models import EmailAddress, EmailConfirmationHMAC
from django.urls import reverse
from itou.utils.urls import get_absolute_url

email = EmailAddress.objects.filter(verified=False).first()
with freeze_time("2023-01-01"):
    token = EmailConfirmationHMAC(email).key
    print(get_absolute_url(reverse("account_confirm_email", kwargs={"key": token})))

💻 Captures d'écran

Lien de confirmation invalide :

Lien de confirmation expiré :

Reconfirmation envoyée :

Remove direct use of EmailAddress for verification

For creating a user with an unverified EmailAddress

Testing redirect and authentication behavior during email confirmation

Add ResendConfirmationView which triggers the sending of new confirmation emails if provided an expired confirmation. Adapt ConfirmEmailView so that a link to the resend view can be provided directly

Prevent a user from being able to trigger more than 3 emails to a given email address daily, to limit misuse

francoisfreitag

Je ne comprends pas bien pourquoi on ajoute des fonctionnalités à allauth avant de le remplacer ? Je m’attendais à ce qu’on remplace des fonctionnalités existantes. Si une fonctionnalité n’est actuellement pas supportée, pourquoi ne pas attendre d’avoir fait la migration pour l’implémenter ?

En l’état, je ne vois pas en quoi cette PR nous facilite la transition pour quitter allauth ?

francoisfreitag · 2025-01-20T15:04:52Z

config/settings/base.py

@@ -339,6 +340,7 @@
 ACCOUNT_LOGIN_ON_EMAIL_CONFIRMATION = True
 ACCOUNT_USERNAME_REQUIRED = False
 ACCOUNT_USER_DISPLAY = "itou.users.models.get_allauth_account_user_display"
+ACCOUNT_MAX_DAILY_EMAIL_CONFIRMATION_REQUESTS = 3


Peut-être être un poil plus généreux, histoire d’éviter de la charge au support ? J’imagine que 20 ne posera pas de problème de sécurité.

OK pour moi !

francoisfreitag · 2025-01-20T15:21:08Z

itou/www/accounts/views.py

+            pk = signing.loads(key, salt=app_settings.SALT)
+            return EmailConfirmationHMAC(EmailAddress.objects.get(pk=pk, verified=False)).email_address
+        except (
+            signing.SignatureExpired,


On ne veut pas traiter différemment SignatureExpired pour continuer à afficher la possibilité de renvoyer un email de confirmation ?

Oui, bien vu
En fait ce serait jamais enlévé car j'ai rétiré le paramètre expiration - je devrais rétirer l'exception aussi

francoisfreitag · 2025-01-20T15:21:36Z

itou/www/accounts/views.py

+            raise Http404()
+
+
+class ConfirmEmailView(BaseConfirmEmailView, ExpiredConfirmationMixin):


Les mixins devraient être avant les classes concrètes pour éviter des surprises dans la MRO.

francoisfreitag · 2025-01-20T15:24:37Z

itou/www/accounts/views.py

+            try:
+                self.get_object_with_expired_key()  # Raises error for invalid keys.
+                key = self.kwargs["key"]
+            except Http404:


Pourquoi utiliser une Http404 si on ne veut pas afficher de 404 à l’utilisateur ?

C'est du code copié de django-allauth. J'ai considéré des autres exceptions sans remarquer qu'on pourrait juste répondre None dans get_object_with_expired_key() 🤦

francoisfreitag · 2025-01-20T15:25:01Z

itou/www/accounts/views.py

+                key = self.kwargs["key"]
+            except Http404:
+                key = None
+        context.update({"provided_key": key})


Plus clair et plus performant.

Suggested change

context.update({"provided_key": key})

context["provided_key"] = key

Bien sûr 😅 merci

francoisfreitag · 2025-01-20T15:26:39Z

itou/www/accounts/views.py

+        if (
+            Email.objects.filter(
+                to__contains=[email_address.email],
+                created_at__gte=this_morning,
+                subject__icontains=confirmation_subject_query,
+            ).count()
+            >= settings.ACCOUNT_MAX_DAILY_EMAIL_CONFIRMATION_REQUESTS
+        ):
+            return redirect(reverse("account_email_rate_limit_exceeded"))


Humpf. On a Redis pour implémenter des rate limiting. Demander à la DB de filtrer sur une table avec des millions d’entrées me semble coûteux et complexe, se baser sur le sujet de l’email me semble très instable.

En reflexion t'as raison, je vais suggerer une nouvelle solution en utilisant le cache. Je vois que le cache est utilisé par des autres solutions rate limiting et au bout d'un jour, ces données ne sont plus intéressantes. Je suis sous l'impression qu'on a bien besoin de logique sur mesure pour ce besoin.

J'avais ces deux concernes, je pensais qu'avec l'indexe sur le champ to ce ne serait pas si couteux mais je n'ai pas testé. Autrement le test va se planter si le contenu du sujet change, je pense que cette partie est stable en practique (même si c'est un peu hacky).

francoisfreitag · 2025-01-20T15:42:44Z

itou/www/accounts/views.py

+
+        # Do not send a confirmation to a verified address.
+        if email_address.verified:
+            return self.fail()


On devrait ajouter un message pour indiquer que l’email est déjà vérifié.

On devrait ajouter un message pour indiquer que l’email est déjà vérifié.

C'est une bonne idée merci.

calummackervoy · 2025-01-20T16:22:33Z

Je ne comprends pas bien pourquoi on ajoute des fonctionnalités à allauth avant de le remplacer ? En l’état, je ne vois pas en quoi cette PR nous facilite la transition pour quitter allauth ?

En ce moment si un utilisateur manque la fenêtre de son e-mail de confirmation il ne peut plus le valider, je pense que c'est un bug qui est à l'extérieur du cadre remplacer allauth ?

La façon dont j'ai corrigé a un oeil sur le PR supprimer django-allauth, j'ai besoin de supprimer nos utilisations du email_view et je préviens ajouter plus des fonctionnalités dans la nouvelle application itou.www.accounts

calummackervoy added 5 commits January 20, 2025 14:31

Refactor prescriber organization tests to use factory trait

8085193

Remove direct use of EmailAddress for verification

Add with_unverified_email trait to UserFactory

4682e2c

For creating a user with an unverified EmailAddress

Add explicit checks that user is authenticated in TestJobSeekerSignup

dca082d

Testing redirect and authentication behavior during email confirmation

Allow re-sending of confirmation emails

fd4ca8a

Add ResendConfirmationView which triggers the sending of new confirmation emails if provided an expired confirmation. Adapt ConfirmEmailView so that a link to the resend view can be provided directly

Add rate limiting to ResendConfirmationView

7e5861c

Prevent a user from being able to trigger more than 3 emails to a given email address daily, to limit misuse

calummackervoy added modifié bug labels Jan 20, 2025

calummackervoy requested review from xavfernandez, francoisfreitag and tonial January 20, 2025 13:46

calummackervoy self-assigned this Jan 20, 2025

francoisfreitag reviewed Jan 20, 2025

View reviewed changes

francoisfreitag mentioned this pull request Jan 24, 2025

Utiliser une socket UNIX pour se connecter à la base de données en dev #5455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connexion: Renvoie de e-mail de confirmation #5428

Connexion: Renvoie de e-mail de confirmation #5428

calummackervoy commented Jan 20, 2025

francoisfreitag left a comment

francoisfreitag Jan 20, 2025

calummackervoy Jan 20, 2025

francoisfreitag Jan 20, 2025

calummackervoy Jan 20, 2025

francoisfreitag Jan 20, 2025

francoisfreitag Jan 20, 2025

calummackervoy Jan 20, 2025 •

edited

Loading

francoisfreitag Jan 20, 2025

calummackervoy Jan 20, 2025

francoisfreitag Jan 20, 2025

calummackervoy Jan 20, 2025

francoisfreitag Jan 20, 2025

calummackervoy Jan 20, 2025

calummackervoy commented Jan 20, 2025

		raise Http404()


		class ConfirmEmailView(BaseConfirmEmailView, ExpiredConfirmationMixin):

	context.update({"provided_key": key})
	context["provided_key"] = key

Connexion: Renvoie de e-mail de confirmation #5428

Are you sure you want to change the base?

Connexion: Renvoie de e-mail de confirmation #5428

Conversation

calummackervoy commented Jan 20, 2025

🤔 Pourquoi ?

🍰 Comment ?

⚠️ Concernes de sécurité

🚨 À vérifier

🏝️ Comment tester

💻 Captures d'écran

francoisfreitag left a 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

calummackervoy Jan 20, 2025 • 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

calummackervoy commented Jan 20, 2025

calummackervoy Jan 20, 2025 •

edited

Loading