From c6fbdef6dfca05d4fdd6a2c826bf46ddba164185 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Sun, 23 Apr 2023 18:58:05 -0400 Subject: [PATCH] fix: Handle integrity violations when confirming email address (#5506) * fix: Handle integrity violations when confirming email address * test: Add tests of confirm_new_email view --- ietf/ietfauth/tests.py | 69 ++++++++++++++++++- ietf/ietfauth/views.py | 15 +++- .../registration/confirm_new_email.html | 12 +++- 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/ietf/ietfauth/tests.py b/ietf/ietfauth/tests.py index 219b273df..631da9870 100644 --- a/ietf/ietfauth/tests.py +++ b/ietf/ietfauth/tests.py @@ -24,10 +24,11 @@ from pyquery import PyQuery from unittest import skipIf from urllib.parse import urlsplit +import django.core.signing from django.urls import reverse as urlreverse from django.contrib.auth.models import User from django.conf import settings -from django.template.loader import render_to_string +from django.template.loader import render_to_string from django.utils import timezone import debug # pyflakes:ignore @@ -937,6 +938,72 @@ class IetfAuthTests(TestCase): self.assertEqual(person.personextresource_set.get(name__slug='github_repo').display_name, 'Some display text') self.assertIn(person.personextresource_set.first().name.slug, str(person.personextresource_set.first())) + def test_confirm_new_email(self): + person = PersonFactory() + valid_auth = django.core.signing.dumps( + [person.user.username, "new_email@example.com"], salt="add_email" + ) + invalid_auth = django.core.signing.dumps( + [person.user.username, "not_this_one@example.com"], salt="pepper" + ) + + # Test that we check the salt + r = self.client.get( + urlreverse("ietf.ietfauth.views.confirm_new_email", kwargs={"auth": invalid_auth}) + ) + self.assertEqual(r.status_code, 404) + r = self.client.post( + urlreverse("ietf.ietfauth.views.confirm_new_email", kwargs={"auth": invalid_auth}) + ) + self.assertEqual(r.status_code, 404) + + # Now check that the valid auth works + self.assertFalse( + person.email_set.filter(address__icontains="new_email@example.com").exists() + ) + confirm_url = urlreverse( + "ietf.ietfauth.views.confirm_new_email", kwargs={"auth": valid_auth} + ) + r = self.client.get(confirm_url) + self.assertContains(r, urllib.parse.quote(confirm_url), status_code=200) + r = self.client.post(confirm_url, data={"action": "confirm"}) + self.assertContains(r, "has been updated", status_code=200) + self.assertTrue( + person.email_set.filter(address__icontains="new_email@example.com").exists() + ) + + # Authorizing a second time should be handled gracefully + r = self.client.post(confirm_url, data={"action": "confirm"}) + self.assertContains(r, "already includes", status_code=200) + + # Another person should not be able to add the same address and should be told so, + # whether they use the same or different letter case + other_person = PersonFactory() + other_auth = django.core.signing.dumps( + [other_person.user.username, "new_email@example.com"], salt="add_email" + ) + r = self.client.post( + urlreverse("ietf.ietfauth.views.confirm_new_email", kwargs={"auth": other_auth}), + data={"action": "confirm"}, + ) + self.assertContains(r, "in use by another user", status_code=200) + + other_auth = django.core.signing.dumps( + [other_person.user.username, "NeW_eMaIl@eXaMpLe.CoM"], salt="add_email" + ) + r = self.client.post( + urlreverse("ietf.ietfauth.views.confirm_new_email", kwargs={"auth": other_auth}), + data={"action": "confirm"}, + ) + + self.assertContains(r, "in use by another user", status_code=200) + self.assertFalse( + other_person.email_set.filter(address__icontains="new_email@example.com").exists() + ) + self.assertTrue( + person.email_set.filter(address__icontains="new_email@example.com").exists() + ) + class OpenIDConnectTests(TestCase): def request_matcher(self, request): diff --git a/ietf/ietfauth/views.py b/ietf/ietfauth/views.py index 4dd21be43..e186b1a54 100644 --- a/ietf/ietfauth/views.py +++ b/ietf/ietfauth/views.py @@ -53,6 +53,7 @@ from django.contrib.auth.models import User from django.contrib.auth.views import LoginView from django.contrib.sites.models import Site from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.db import IntegrityError from django.urls import reverse as urlreverse from django.http import Http404, HttpResponseRedirect, HttpResponseForbidden from django.shortcuts import render, redirect, get_object_or_404 @@ -440,8 +441,19 @@ def confirm_new_email(request, auth): form = NewEmailForm({ "new_email": email }) can_confirm = form.is_valid() and email new_email_obj = None + created = False if request.method == 'POST' and can_confirm and request.POST.get("action") == "confirm": - new_email_obj = Email.objects.create(address=email, person=person, origin=username) + try: + new_email_obj, created = Email.objects.get_or_create( + address=email, + person=person, + defaults={'origin': username}, + ) + except IntegrityError: + can_confirm = False + form.add_error( + None, "Email address is in use by another user. Please contact the secretariat for assistance." + ) return render(request, 'registration/confirm_new_email.html', { 'username': username, @@ -449,6 +461,7 @@ def confirm_new_email(request, auth): 'can_confirm': can_confirm, 'form': form, 'new_email_obj': new_email_obj, + 'already_confirmed': new_email_obj and not created, }) def password_reset(request): diff --git a/ietf/templates/registration/confirm_new_email.html b/ietf/templates/registration/confirm_new_email.html index 959956437..c5449c5e1 100644 --- a/ietf/templates/registration/confirm_new_email.html +++ b/ietf/templates/registration/confirm_new_email.html @@ -13,9 +13,15 @@ Edit profile {% elif new_email_obj %} -

- Your account {{ username }} has been updated to include the email address {{ email|linkify }}. -

+ {% if already_confirmed %} +

+ Your account {{ username }} already includes the email address {{ email|linkify }}. +

+ {% else %} +

+ Your account {{ username }} has been updated to include the email address {{ email|linkify }}. +

+ {% endif %} Edit profile {% else %}