fix: Don't expose existing emails via reset password and account creation forms (ietf-tools#5288)

* Rebase to feat/postgres

* Attempt to address further review comments

Author: larseggert

ietf/ietfauth/forms.py

@@ -10,8 +10,6 @@
 from django.core.exceptions import ValidationError
 from django.db import models
 from django.contrib.auth.models import User
-from django.utils.html import mark_safe # type:ignore
-from django.urls import reverse as urlreverse
 
 from django_password_strength.widgets import PasswordStrengthInput, PasswordConfirmationInput
 
@@ -31,8 +29,6 @@ def clean_email(self):
             return email
         if email.lower() != email:
             raise forms.ValidationError('The supplied address contained uppercase letters.  Please use a lowercase email address.')
-        if User.objects.filter(username__iexact=email).exists():
-            raise forms.ValidationError('An account with the email address you provided already exists.')
         return email
 
 
@@ -164,11 +160,6 @@ class NewEmailForm(forms.Form):
 
     def clean_new_email(self):
         email = self.cleaned_data.get("new_email", "")
-        if email:
-            existing = Email.objects.filter(address=email).first()
-            if existing:
-                raise forms.ValidationError("Email address '%s' is already assigned to account '%s' (%s)" % (existing, existing.person and existing.person.user, existing.person))
-
         for pat in settings.EXCLUDED_PERSONAL_EMAIL_REGEX_PATTERNS:
             if re.search(pat, email):
                 raise ValidationError("This email address is not valid in a datatracker account")
@@ -193,21 +184,6 @@ def __init__(self, role, *args, **kwargs):
 class ResetPasswordForm(forms.Form):
     username = forms.EmailField(label="Your email (lowercase)")
 
-    def clean_username(self):
-        """Verify that the username is valid
-
-        In addition to EmailField's checks, verifies that a User matching the username exists.
-        """
-        username = self.cleaned_data["username"]
-        if not User.objects.filter(username__iexact=username).exists():
-            raise forms.ValidationError(mark_safe(
-                "Didn't find a matching account. "
-                "If you don't have an account yet, you can <a href=\"{}\">create one</a>.".format(
-                    urlreverse('ietf.ietfauth.views.create_account')
-                )
-            ))
-        return username
-
 
 class TestEmailForm(forms.Form):
     email = forms.EmailField(required=False)

ietf/ietfauth/tests.py

@@ -165,7 +165,7 @@ def test_create_account_failure_template(self):
         r = render_to_string('registration/manual.html', { 'account_request_email': settings.ACCOUNT_REQUEST_EMAIL })
         self.assertTrue("Additional Assistance Required" in r)
 
-    def register_and_verify(self, email):
+    def register(self, email):
         url = urlreverse(ietf.ietfauth.views.create_account)
 
         # register email
@@ -175,6 +175,9 @@ def register_and_verify(self, email):
         self.assertContains(r, "Account request received")
         self.assertEqual(len(outbox), 1)
 
+    def register_and_verify(self, email):
+        self.register(email)
+
         # go to confirm page
         confirm_url = self.extract_confirm_url(outbox[-1])
         r = self.client.get(confirm_url)
@@ -229,6 +232,20 @@ def test_create_subscribed_account(self):
         self.register_and_verify(email)
         settings.LIST_ACCOUNT_DELAY = saved_delay
 
+    def test_create_existing_account(self):
+        # create account once
+        email = "new-account@example.com"
+        self.register_and_verify(email)
+
+        # create account again
+        self.register(email)
+
+        # check notification
+        note = get_payload_text(outbox[-1])
+        self.assertIn(email, note)
+        self.assertIn("A datatracker account for that email already exists", note)
+        self.assertIn(urlreverse(ietf.ietfauth.views.password_reset), note)
+
     def test_ietfauth_profile(self):
         EmailFactory(person__user__username='plain')
         GroupFactory(acronym='mars')
@@ -317,11 +334,14 @@ def test_ietfauth_profile(self):
         self.assertEqual(r.status_code, 200)
         self.assertEqual(Email.objects.filter(address=new_email_address, person__user__username=username, active=1).count(), 1)
 
-        # check that we can't re-add it - that would give a duplicate
-        r = self.client.get(confirm_url)
+        # try and add it again
+        empty_outbox()
+        r = self.client.post(url, with_new_email_address)
         self.assertEqual(r.status_code, 200)
-        q = PyQuery(r.content)
-        self.assertEqual(len(q('[name="action"][value="confirm"]')), 0)
+        self.assertEqual(len(outbox), 1)
+        note = get_payload_text(outbox[-1])
+        self.assertIn(new_email_address, note)
+        self.assertIn("already associated with your account", note)
 
         pronoundish = base_data.copy()
         pronoundish["pronouns_freetext"] = "baz/boom"
@@ -395,7 +415,7 @@ def test_email_case_insensitive_protection(self):
             "new_email": "testaddress@example.net",
         }
         r = self.client.post(url, data)
-        self.assertContains(r, "Email address 'TestAddress@example.net' is already assigned", status_code=200)
+        self.assertContains(r, "A confirmation email has been sent to", status_code=200)
 
     def test_nomcom_dressing_on_profile(self):
         url = urlreverse('ietf.ietfauth.views.profile')
@@ -437,11 +457,11 @@ def test_reset_password(self):
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
 
-        # ask for reset, wrong username
+        # ask for reset, wrong username (form should not fail)
         r = self.client.post(url, { 'username': "nobody@example.com" })
         self.assertEqual(r.status_code, 200)
         q = PyQuery(r.content)
-        self.assertTrue(len(q("form .is-invalid")) > 0)
+        self.assertTrue(len(q("form .is-invalid")) == 0)
 
         # ask for reset
         empty_outbox()
@@ -518,9 +538,9 @@ def test_reset_password_without_person(self):
         user.save()
         empty_outbox()
         r = self.client.post(url, { 'username': user.username})
-        self.assertContains(r, 'No known active email addresses', status_code=200)
+        self.assertContains(r, 'We have sent you an email with instructions', status_code=200)
         q = PyQuery(r.content)
-        self.assertTrue(len(q("form .is-invalid")) > 0)
+        self.assertTrue(len(q("form .is-invalid")) == 0)
         self.assertEqual(len(outbox), 0)
 
     def test_reset_password_address_handling(self):
@@ -530,14 +550,14 @@ def test_reset_password_address_handling(self):
         person.email_set.update(active=False)
         empty_outbox()
         r = self.client.post(url, { 'username': person.user.username})
-        self.assertContains(r, 'No known active email addresses', status_code=200)
+        self.assertContains(r, 'We have sent you an email with instructions', status_code=200)
         q = PyQuery(r.content)
-        self.assertTrue(len(q("form .is-invalid")) > 0)
+        self.assertTrue(len(q("form .is-invalid")) == 0)
         self.assertEqual(len(outbox), 0)
 
         active_address = EmailFactory(person=person).address
         r = self.client.post(url, {'username': person.user.username})
-        self.assertNotContains(r, 'No known active email addresses', status_code=200)
+        self.assertContains(r, 'We have sent you an email with instructions', status_code=200)
         self.assertEqual(len(outbox), 1)
         to = outbox[0].get('To')
         self.assertIn(active_address, to)

ietf/ietfauth/views.py

@@ -112,33 +112,51 @@ def index(request):
 #         redirect_to = settings.LOGIN_REDIRECT_URL
 #     return HttpResponseRedirect(redirect_to)
 
+
 def create_account(request):
-    to_email = None
+    new_account_email = None
 
-    if request.method == 'POST':
+    if request.method == "POST":
         form = RegistrationForm(request.POST)
         if form.is_valid():
-            to_email = form.cleaned_data['email'] # This will be lowercase if form.is_valid()
-
-            # For the IETF 113 Registration period (at least) we are lowering the barriers for account creation
-            # to the simple email round-trip check
-            send_account_creation_email(request, to_email)
-
-            # The following is what to revert to should that lowered barrier prove problematic
-            # existing = Subscribed.objects.filter(email__iexact=to_email).first()
-            # ok_to_create = ( Allowlisted.objects.filter(email__iexact=to_email).exists()
-            #     or existing and (existing.time + TimeDelta(seconds=settings.LIST_ACCOUNT_DELAY)) < DateTime.now() )
-            # if ok_to_create:
-            #     send_account_creation_email(request, to_email)
-            # else:
-            #     return render(request, 'registration/manual.html', { 'account_request_email': settings.ACCOUNT_REQUEST_EMAIL })
+            new_account_email = form.cleaned_data[
+                "email"
+            ]  # This will be lowercase if form.is_valid()
+
+            user = User.objects.filter(username__iexact=new_account_email)
+            email = Email.objects.filter(address__iexact=new_account_email)
+            if user.exists() or email.exists():
+                person_to_contact = user.first().person if user else email.first().person
+                to_email = person_to_contact.email_address()
+                if to_email:
+                    send_account_creation_exists_email(request, new_account_email, to_email)
+                else:
+                    raise ValidationError(f"Account for {{new_account_email}} exists, but cannot email it")
+            else:
+                # For the IETF 113 Registration period (at least) we are lowering the
+                # barriers for account creation to the simple email round-trip check
+                send_account_creation_email(request, new_account_email)
+
+                # The following is what to revert to should that lowered barrier prove problematic
+                # existing = Subscribed.objects.filter(email__iexact=new_account_email).first()
+                # ok_to_create = ( Allowlisted.objects.filter(email__iexact=new_account_email).exists()
+                #     or existing and (existing.time + TimeDelta(seconds=settings.LIST_ACCOUNT_DELAY)) < DateTime.now() )
+                # if ok_to_create:
+                #     send_account_creation_email(request, new_account_email)
+                # else:
+                #     return render(request, 'registration/manual.html', { 'account_request_email': settings.ACCOUNT_REQUEST_EMAIL })
     else:
         form = RegistrationForm()
 
-    return render(request, 'registration/create.html', {
-        'form': form,
-        'to_email': to_email,
-    })
+    return render(
+        request,
+        "registration/create.html",
+        {
+            "form": form,
+            "to_email": new_account_email,
+        },
+    )
+
 
 def send_account_creation_email(request, to_email):
     auth = django.core.signing.dumps(to_email, salt="create_account")
@@ -153,6 +171,23 @@ def send_account_creation_email(request, to_email):
     })
 
 
+def send_account_creation_exists_email(request, new_account_email, to_email):
+    domain = Site.objects.get_current().domain
+    subject = "Attempted account creation at %s" % domain
+    from_email = settings.DEFAULT_FROM_EMAIL
+    send_mail(
+        request,
+        to_email,
+        from_email,
+        subject,
+        "registration/creation_exists_email.txt",
+        {
+            "domain": domain,
+            "username": new_account_email,
+        },
+    )
+
+
 def confirm_account(request, auth):
     try:
         email = django.core.signing.loads(auth, salt="create_account", max_age=settings.DAYS_TO_EXPIRE_REGISTRATION_LINK * 24 * 60 * 60)
@@ -255,17 +290,25 @@ def profile(request):
                 auth = django.core.signing.dumps([person.user.username, to_email], salt="add_email")
 
                 domain = Site.objects.get_current().domain
-                subject = 'Confirm email address for %s' % person.name
                 from_email = settings.DEFAULT_FROM_EMAIL
 
-                send_mail(request, to_email, from_email, subject, 'registration/add_email_email.txt', {
-                    'domain': domain,
-                    'auth': auth,
-                    'email': to_email,
-                    'person': person,
-                    'expire': settings.DAYS_TO_EXPIRE_REGISTRATION_LINK,
-                })
-                
+                existing = Email.objects.filter(address=to_email).first()
+                if existing:
+                    subject = 'Attempt to add your email address by %s' % person.name
+                    send_mail(request, to_email, from_email, subject, 'registration/add_email_exists_email.txt', {
+                        'domain': domain,
+                        'email': to_email,
+                        'person': person,
+                    })
+                else:
+                    subject = 'Confirm email address for %s' % person.name
+                    send_mail(request, to_email, from_email, subject, 'registration/add_email_email.txt', {
+                        'domain': domain,
+                        'auth': auth,
+                        'email': to_email,
+                        'person': person,
+                        'expire': settings.DAYS_TO_EXPIRE_REGISTRATION_LINK,
+                    })
 
             for r in roles:
                 e = r.email_form.cleaned_data["email"]
@@ -417,14 +460,10 @@ def password_reset(request):
             # The form validation checks that a matching User exists. Add the person__isnull check
             # because the OneToOne field does not gracefully handle checks for user.person is Null.
             # If we don't get a User here, we know it's because there's no related Person.
+            # We still report that the action succeeded, so we're not leaking the existence of user
+            # email addresses.
             user = User.objects.filter(username__iexact=submitted_username, person__isnull=False).first()
-            if not (user and user.person.email_set.filter(active=True).exists()):
-                form.add_error(
-                    'username',
-                    'No known active email addresses are associated with this account. '
-                    'Please contact the secretariat for assistance.',
-                )
-            else:
+            if user and user.person.email_set.filter(active=True).exists():
                 data = {
                     'username': user.username,
                     'password': user.password and user.password[-4:],
@@ -445,7 +484,7 @@ def password_reset(request):
                     'username': submitted_username,
                     'expire': settings.MINUTES_TO_EXPIRE_RESET_PASSWORD_LINK,
                 })
-                success = True
+            success = True
     else:
         form = ResetPasswordForm()
     return render(request, 'registration/password_reset.html', {

ietf/templates/registration/add_email_exists_email.txt

@@ -0,0 +1,14 @@
+{% autoescape off %}{% load ietf_filters %}
+Hello,
+
+{% filter wordwrap:78 %}We have received a request to add the email address {{ email }} to the user account '{{ person.user }}' at {{ domain }}.
+This email address {{ email }} is already associated with your account at {{ domain }} and cannot be associated with two accounts.{% endfilter %}
+
+If you did not request this change, you may safely ignore this email,
+as no actions have been taken.
+
+Best regards,
+
+	The datatracker login manager service
+	(for the IETF Secretariat)
+{% endautoescape %}

ietf/templates/registration/creation_exists_email.txt

@@ -0,0 +1,17 @@
+{% autoescape off %}{% load ietf_filters %}
+Hello,
+
+{% filter wordwrap:78 %}We have received an account creation request for {{ username }} at {{ domain }}.{% endfilter %}
+
+{% filter wordwrap:78 %}A datatracker account for that email already exists. If you have forgotten the password for the {{ username }} account, please go to the following link and follow the instructions there:{% endfilter %}
+
+   https://{{ domain }}{% url "ietf.ietfauth.views.password_reset" %}
+
+If you have not requested the account creation you can ignore this email, your
+credentials have been left untouched.
+
+Best regards,
+
+    The datatracker login manager service
+    (for the IETF Secretariat)
+{% endautoescape %}