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

@@ -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):

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

ietf/templates/registration/confirm_new_email.html

@@ -13,9 +13,15 @@
         <a class="btn btn-primary my-3"
            href="{% url "ietf.ietfauth.views.profile" %}">Edit profile</a>
     {% elif new_email_obj %}
-        <p class="alert alert-success my-3">
-            Your account {{ username }} has been updated to include the email address {{ email|linkify }}.
-        </p>
+        {% if already_confirmed %} 
+            <p class="alert alert-info my-3">
+                Your account {{ username }} already includes the email address {{ email|linkify }}.
+            </p>
+        {% else %}
+            <p class="alert alert-success my-3">
+                Your account {{ username }} has been updated to include the email address {{ email|linkify }}.
+            </p>
+        {% endif %}
         <a class="btn btn-primary my-3"
            href="{% url "ietf.ietfauth.views.profile" %}">Edit profile</a>
     {% else %}