fix: Only send password reset email to known, active addresses (#5061)
* fix: Only send password reset email to known, active addresses Limits password reset to Users with a Person and at least one active address on file. Avoids the possibility of sending a password reset to a spoofed address as in CVE-2019-19844. * test: Use factory instead of explicit construction * test: Test that a User with no Person cannot reset password * fix: Fix handling of User.person field when it's null * test: Test that reset emails are sent to known, active addresses
This commit is contained in:
Jennifer Richards
GitHub
parent
afac1f8f19
commit
98d7b15dfb
|
|
@ -21,6 +21,7 @@ from ietf.person.models import Person, Email
|
|||
from ietf.mailinglists.models import Allowlisted
|
||||
from ietf.utils.text import isascii
|
||||
|
||||
|
||||
class RegistrationForm(forms.Form):
|
||||
email = forms.EmailField(label="Your email (lowercase)")
|
||||
|
||||
|
|
@ -193,10 +194,18 @@ class ResetPasswordForm(forms.Form):
|
|||
username = forms.EmailField(label="Your email (lowercase)")
|
||||
|
||||
def clean_username(self):
|
||||
import ietf.ietfauth.views
|
||||
"""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=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))))
|
||||
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
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -414,12 +414,10 @@ class IetfAuthTests(TestCase):
|
|||
email = 'someone@example.com'
|
||||
password = 'foobar'
|
||||
|
||||
user = User.objects.create(username=email, email=email)
|
||||
user = PersonFactory(user__email=email).user
|
||||
user.set_password(password)
|
||||
user.save()
|
||||
p = Person.objects.create(name="Some One", ascii="Some One", user=user)
|
||||
Email.objects.create(address=user.username, person=p, origin=user.username)
|
||||
|
||||
|
||||
# get
|
||||
r = self.client.get(url)
|
||||
self.assertEqual(r.status_code, 200)
|
||||
|
|
@ -497,6 +495,39 @@ class IetfAuthTests(TestCase):
|
|||
r = self.client.get(confirm_url)
|
||||
self.assertEqual(r.status_code, 404)
|
||||
|
||||
def test_reset_password_without_person(self):
|
||||
"""No password reset for account without a person"""
|
||||
url = urlreverse('ietf.ietfauth.views.password_reset')
|
||||
user = UserFactory()
|
||||
user.set_password('some password')
|
||||
user.save()
|
||||
empty_outbox()
|
||||
r = self.client.post(url, { 'username': user.username})
|
||||
self.assertContains(r, 'No known active email addresses', status_code=200)
|
||||
q = PyQuery(r.content)
|
||||
self.assertTrue(len(q("form .is-invalid")) > 0)
|
||||
self.assertEqual(len(outbox), 0)
|
||||
|
||||
def test_reset_password_address_handling(self):
|
||||
"""Reset password links are only sent to known, active addresses"""
|
||||
url = urlreverse('ietf.ietfauth.views.password_reset')
|
||||
person = PersonFactory()
|
||||
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)
|
||||
q = PyQuery(r.content)
|
||||
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.assertEqual(len(outbox), 1)
|
||||
to = outbox[0].get('To')
|
||||
self.assertIn(active_address, to)
|
||||
self.assertNotIn(person.user.username, to)
|
||||
|
||||
def test_review_overview(self):
|
||||
review_req = ReviewRequestFactory()
|
||||
assignment = ReviewAssignmentFactory(review_request=review_req,reviewer=EmailFactory(person__user__username='reviewer'))
|
||||
|
|
|
|||
|
|
@ -413,32 +413,39 @@ def password_reset(request):
|
|||
if request.method == 'POST':
|
||||
form = ResetPasswordForm(request.POST)
|
||||
if form.is_valid():
|
||||
username = form.cleaned_data['username']
|
||||
submitted_username = form.cleaned_data['username']
|
||||
# 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.
|
||||
user = User.objects.filter(username=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:
|
||||
data = {
|
||||
'username': user.username,
|
||||
'password': user.password and user.password[-4:],
|
||||
'last_login': user.last_login.timestamp() if user.last_login else None,
|
||||
}
|
||||
auth = django.core.signing.dumps(data, salt="password_reset")
|
||||
|
||||
data = { 'username': username }
|
||||
if User.objects.filter(username=username).exists():
|
||||
user = User.objects.get(username=username)
|
||||
data['password'] = user.password and user.password[-4:]
|
||||
if user.last_login:
|
||||
data['last_login'] = user.last_login.timestamp()
|
||||
else:
|
||||
data['last_login'] = None
|
||||
|
||||
auth = django.core.signing.dumps(data, salt="password_reset")
|
||||
|
||||
domain = Site.objects.get_current().domain
|
||||
subject = 'Confirm password reset at %s' % domain
|
||||
from_email = settings.DEFAULT_FROM_EMAIL
|
||||
to_email = username # form validation makes sure that this is an email address
|
||||
|
||||
send_mail(request, to_email, from_email, subject, 'registration/password_reset_email.txt', {
|
||||
'domain': domain,
|
||||
'auth': auth,
|
||||
'username': username,
|
||||
'expire': settings.MINUTES_TO_EXPIRE_RESET_PASSWORD_LINK,
|
||||
})
|
||||
|
||||
success = True
|
||||
domain = Site.objects.get_current().domain
|
||||
subject = 'Confirm password reset at %s' % domain
|
||||
from_email = settings.DEFAULT_FROM_EMAIL
|
||||
# Send email to addresses from the database, NOT to the address from the form.
|
||||
# This prevents unicode spoofing tricks (https://nvd.nist.gov/vuln/detail/CVE-2019-19844).
|
||||
to_emails = list(set(email.address for email in user.person.email_set.filter(active=True)))
|
||||
to_emails.sort()
|
||||
send_mail(request, to_emails, from_email, subject, 'registration/password_reset_email.txt', {
|
||||
'domain': domain,
|
||||
'auth': auth,
|
||||
'username': submitted_username,
|
||||
'expire': settings.MINUTES_TO_EXPIRE_RESET_PASSWORD_LINK,
|
||||
})
|
||||
success = True
|
||||
else:
|
||||
form = ResetPasswordForm()
|
||||
return render(request, 'registration/password_reset.html', {
|
||||
|
|
|
|||
Loading…
Reference in a new issue