From 0faa2e40e73285889ecfec3d921cb5b7b716d99b Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 16 Mar 2023 00:27:29 +0200 Subject: [PATCH] fix: Don't expose existing emails via reset password and account creation forms (#5288) * Rebase to feat/postgres * Attempt to address further review comments --- ietf/ietfauth/forms.py | 24 ---- ietf/ietfauth/tests.py | 46 ++++++-- ietf/ietfauth/views.py | 111 ++++++++++++------ .../registration/add_email_exists_email.txt | 14 +++ .../registration/creation_exists_email.txt | 17 +++ 5 files changed, 139 insertions(+), 73 deletions(-) create mode 100644 ietf/templates/registration/add_email_exists_email.txt create mode 100644 ietf/templates/registration/creation_exists_email.txt diff --git a/ietf/ietfauth/forms.py b/ietf/ietfauth/forms.py index 2c2c57047..ce9f58f49 100644 --- a/ietf/ietfauth/forms.py +++ b/ietf/ietfauth/forms.py @@ -10,8 +10,6 @@ from django.conf import settings 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 @@ class RegistrationForm(forms.Form): 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 @@ class RoleEmailForm(forms.Form): 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 create one.".format( - urlreverse('ietf.ietfauth.views.create_account') - ) - )) - return username - class TestEmailForm(forms.Form): email = forms.EmailField(required=False) diff --git a/ietf/ietfauth/tests.py b/ietf/ietfauth/tests.py index 37aea4f8c..5ce299d3d 100644 --- a/ietf/ietfauth/tests.py +++ b/ietf/ietfauth/tests.py @@ -165,7 +165,7 @@ class IetfAuthTests(TestCase): 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 @@ class IetfAuthTests(TestCase): 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 @@ class IetfAuthTests(TestCase): 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 @@ class IetfAuthTests(TestCase): 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 @@ class IetfAuthTests(TestCase): "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 @@ class IetfAuthTests(TestCase): 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 @@ class IetfAuthTests(TestCase): 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 @@ class IetfAuthTests(TestCase): 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) diff --git a/ietf/ietfauth/views.py b/ietf/ietfauth/views.py index 9adb62b31..fcd1d75d2 100644 --- a/ietf/ietfauth/views.py +++ b/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 - if request.method == 'POST': +def create_account(request): + new_account_email = None + + 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() + new_account_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) + 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=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 }) + # 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', { diff --git a/ietf/templates/registration/add_email_exists_email.txt b/ietf/templates/registration/add_email_exists_email.txt new file mode 100644 index 000000000..ed23a746e --- /dev/null +++ b/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 %} diff --git a/ietf/templates/registration/creation_exists_email.txt b/ietf/templates/registration/creation_exists_email.txt new file mode 100644 index 000000000..a7dc363ac --- /dev/null +++ b/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 %}