From 3c13db45fd6b8eaaf88427d1da01761abab4e5de Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Fri, 24 May 2024 18:36:58 -0300 Subject: [PATCH] fix: validate form in login() (#7435) * fix: validate form in login() * refactor: custom LoginView subclass for logins Preserves old behavior, but avoids some hacks. * test: reverse with strings, not view refs * chore: remove unused imports * fix: restore logout() call --- ietf/ietfauth/tests.py | 53 ++++++++++++------------ ietf/ietfauth/urls.py | 2 +- ietf/ietfauth/views.py | 92 ++++++++++++++++++++++-------------------- 3 files changed, 74 insertions(+), 73 deletions(-) diff --git a/ietf/ietfauth/tests.py b/ietf/ietfauth/tests.py index 29afa56d7..56b4638f6 100644 --- a/ietf/ietfauth/tests.py +++ b/ietf/ietfauth/tests.py @@ -50,8 +50,6 @@ from ietf.utils.test_utils import TestCase, login_testing_unauthorized from ietf.utils.timezone import date_today -import ietf.ietfauth.views - if os.path.exists(settings.HTPASSWD_COMMAND): skip_htpasswd_command = False skip_message = "" @@ -83,30 +81,30 @@ class IetfAuthTests(TestCase): super().tearDown() def test_index(self): - self.assertEqual(self.client.get(urlreverse(ietf.ietfauth.views.index)).status_code, 200) + self.assertEqual(self.client.get(urlreverse("ietf.ietfauth.views.index")).status_code, 200) def test_login_and_logout(self): PersonFactory(user__username='plain') # try logging in without a next - r = self.client.get(urlreverse(ietf.ietfauth.views.login)) + r = self.client.get(urlreverse("ietf.ietfauth.views.login")) self.assertEqual(r.status_code, 200) - r = self.client.post(urlreverse(ietf.ietfauth.views.login), {"username":"plain", "password":"plain+password"}) + r = self.client.post(urlreverse("ietf.ietfauth.views.login"), {"username":"plain", "password":"plain+password"}) self.assertEqual(r.status_code, 302) - self.assertEqual(urlsplit(r["Location"])[2], urlreverse(ietf.ietfauth.views.profile)) + self.assertEqual(urlsplit(r["Location"])[2], urlreverse("ietf.ietfauth.views.profile")) # try logging out r = self.client.post(urlreverse('django.contrib.auth.views.logout'), {}) self.assertEqual(r.status_code, 200) self.assertNotContains(r, "accounts/logout") - r = self.client.get(urlreverse(ietf.ietfauth.views.profile)) + r = self.client.get(urlreverse("ietf.ietfauth.views.profile")) self.assertEqual(r.status_code, 302) - self.assertEqual(urlsplit(r["Location"])[2], urlreverse(ietf.ietfauth.views.login)) + self.assertEqual(urlsplit(r["Location"])[2], urlreverse("ietf.ietfauth.views.login")) # try logging in with a next - r = self.client.post(urlreverse(ietf.ietfauth.views.login) + "?next=/foobar", {"username":"plain", "password":"plain+password"}) + r = self.client.post(urlreverse("ietf.ietfauth.views.login") + "?next=/foobar", {"username":"plain", "password":"plain+password"}) self.assertEqual(r.status_code, 302) self.assertEqual(urlsplit(r["Location"])[2], "/foobar") @@ -137,19 +135,19 @@ class IetfAuthTests(TestCase): # try with a trivial next _test_login("/") # try with a next that requires login - _test_login(urlreverse(ietf.ietfauth.views.profile)) + _test_login(urlreverse("ietf.ietfauth.views.profile")) def test_login_with_different_email(self): person = PersonFactory(user__username='plain') email = EmailFactory(person=person) # try logging in without a next - r = self.client.get(urlreverse(ietf.ietfauth.views.login)) + r = self.client.get(urlreverse("ietf.ietfauth.views.login")) self.assertEqual(r.status_code, 200) - r = self.client.post(urlreverse(ietf.ietfauth.views.login), {"username":email, "password":"plain+password"}) + r = self.client.post(urlreverse("ietf.ietfauth.views.login"), {"username":email, "password":"plain+password"}) self.assertEqual(r.status_code, 302) - self.assertEqual(urlsplit(r["Location"])[2], urlreverse(ietf.ietfauth.views.profile)) + self.assertEqual(urlsplit(r["Location"])[2], urlreverse("ietf.ietfauth.views.profile")) def extract_confirm_url(self, confirm_email): # dig out confirm_email link @@ -176,7 +174,7 @@ class IetfAuthTests(TestCase): # For the lowered barrier to account creation period, we are disabling this kind of failure # def test_create_account_failure(self): - # url = urlreverse(ietf.ietfauth.views.create_account) + # url = urlreverse("ietf.ietfauth.views.create_account") # # get # r = self.client.get(url) @@ -195,7 +193,7 @@ class IetfAuthTests(TestCase): self.assertTrue("Additional Assistance Required" in r) def register(self, email): - url = urlreverse(ietf.ietfauth.views.create_account) + url = urlreverse("ietf.ietfauth.views.create_account") # register email empty_outbox() @@ -240,7 +238,7 @@ class IetfAuthTests(TestCase): 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) + self.assertIn(urlreverse("ietf.ietfauth.views.password_reset"), note) def test_ietfauth_profile(self): EmailFactory(person__user__username='plain') @@ -249,7 +247,7 @@ class IetfAuthTests(TestCase): username = "plain" email_address = Email.objects.filter(person__user__username=username).first().address - url = urlreverse(ietf.ietfauth.views.profile) + url = urlreverse("ietf.ietfauth.views.profile") login_testing_unauthorized(self, username, url) @@ -400,7 +398,7 @@ class IetfAuthTests(TestCase): def test_email_case_insensitive_protection(self): EmailFactory(address="TestAddress@example.net") person = PersonFactory() - url = urlreverse(ietf.ietfauth.views.profile) + url = urlreverse("ietf.ietfauth.views.profile") login_testing_unauthorized(self, person.user.username, url) data = { @@ -441,7 +439,7 @@ class IetfAuthTests(TestCase): def test_reset_password(self): - url = urlreverse(ietf.ietfauth.views.password_reset) + url = urlreverse("ietf.ietfauth.views.password_reset") email = 'someone@example.com' password = 'foobar' @@ -507,7 +505,7 @@ class IetfAuthTests(TestCase): self.assertEqual(len(outbox), 1) confirm_url = self.extract_confirm_url(outbox[-1]) - r = self.client.post(urlreverse(ietf.ietfauth.views.login), {'username': email, 'password': password}) + r = self.client.post(urlreverse("ietf.ietfauth.views.login"), {'username': email, 'password': password}) r = self.client.get(confirm_url) self.assertEqual(r.status_code, 404) @@ -589,7 +587,7 @@ class IetfAuthTests(TestCase): availability="unavailable", ) - url = urlreverse(ietf.ietfauth.views.review_overview) + url = urlreverse("ietf.ietfauth.views.review_overview") login_testing_unauthorized(self, reviewer.user.username, url) @@ -633,10 +631,9 @@ class IetfAuthTests(TestCase): def test_change_password(self): - - chpw_url = urlreverse(ietf.ietfauth.views.change_password) - prof_url = urlreverse(ietf.ietfauth.views.profile) - login_url = urlreverse(ietf.ietfauth.views.login) + chpw_url = urlreverse("ietf.ietfauth.views.change_password") + prof_url = urlreverse("ietf.ietfauth.views.profile") + login_url = urlreverse("ietf.ietfauth.views.login") redir_url = '%s?next=%s' % (login_url, chpw_url) # get without logging in @@ -681,9 +678,9 @@ class IetfAuthTests(TestCase): def test_change_username(self): - chun_url = urlreverse(ietf.ietfauth.views.change_username) - prof_url = urlreverse(ietf.ietfauth.views.profile) - login_url = urlreverse(ietf.ietfauth.views.login) + chun_url = urlreverse("ietf.ietfauth.views.change_username") + prof_url = urlreverse("ietf.ietfauth.views.profile") + login_url = urlreverse("ietf.ietfauth.views.login") redir_url = '%s?next=%s' % (login_url, chun_url) # get without logging in diff --git a/ietf/ietfauth/urls.py b/ietf/ietfauth/urls.py index 30e639ad6..7493fe5c9 100644 --- a/ietf/ietfauth/urls.py +++ b/ietf/ietfauth/urls.py @@ -14,7 +14,7 @@ urlpatterns = [ url(r'^confirmnewemail/(?P[^/]+)/$', views.confirm_new_email), url(r'^create/$', views.create_account), url(r'^create/confirm/(?P[^/]+)/$', views.confirm_account), - url(r'^login/$', views.login), + url(r'^login/$', views.AnyEmailLoginView.as_view(), name="ietf.ietfauth.views.login"), url(r'^logout/$', LogoutView.as_view(), name="django.contrib.auth.views.logout"), url(r'^password/$', views.change_password), url(r'^profile/$', views.profile), diff --git a/ietf/ietfauth/views.py b/ietf/ietfauth/views.py index 8c61b8356..7c3f72108 100644 --- a/ietf/ietfauth/views.py +++ b/ietf/ietfauth/views.py @@ -45,7 +45,7 @@ import django.core.signing from django import forms from django.contrib import messages from django.conf import settings -from django.contrib.auth import update_session_auth_hash, logout, authenticate +from django.contrib.auth import logout, update_session_auth_hash from django.contrib.auth.decorators import login_required from django.contrib.auth.forms import AuthenticationForm from django.contrib.auth.hashers import identify_hasher @@ -752,53 +752,57 @@ def change_username(request): return render(request, 'registration/change_username.html', {'form': form}) - -def login(request, extra_context=None): - """ - This login function is a wrapper around django's login() for the purpose - of providing a notification if the user's password has been cleared. The - warning will be triggered if the password field has been set to something - which is not recognized as a valid password hash. +class AnyEmailAuthenticationForm(AuthenticationForm): + """AuthenticationForm that allows any email address as the username + + Also performs a check for a cleared password field and provides a helpful error message + if that applies to the user attempting to log in. """ + _unauthenticated_user = None - if request.method == "POST": - form = AuthenticationForm(request, data=request.POST) - username = form.data.get('username') - user = User.objects.filter(username__iexact=username).first() # Consider _never_ actually looking for the User username and only looking at Email - if not user: - # try to find user ID from the email address + def clean_username(self): + username = self.cleaned_data.get("username", None) + if username is None: + raise self.get_invalid_login_error() + user = User.objects.filter(username__iexact=username).first() + if user is None: email = Email.objects.filter(address=username).first() - if email and email.person and email.person.user: - u2 = email.person.user - # be conservative, only accept this if login is valid - if u2: - pw = form.data.get('password') - au = authenticate(request, username=u2.username, password=pw) - if au: - # kludge to change the querydict - q2 = request.POST.copy() - q2['username'] = u2.username - request.POST = q2 - user = u2 - # - if user: - try: - identify_hasher(user.password) + if email and email.person: + user = email.person.user # might be None + if user is None: + raise self.get_invalid_login_error() + self._unauthenticated_user = user # remember this for the clean() method + return user.username + + def clean(self): + if self._unauthenticated_user is not None: + try: + identify_hasher(self._unauthenticated_user.password) except ValueError: - extra_context = {"alert": - "Note: Your password has been cleared because " - "of possible password leakage. " - "Please use the password reset link below " - "to set a new password for your account.", - } - response = LoginView.as_view(extra_context=extra_context)(request) - if isinstance(response, HttpResponseRedirect) and user and user.is_authenticated: - try: - user.person - except Person.DoesNotExist: - logout(request) - response = render(request, 'registration/missing_person.html') - return response + self.add_error( + "password", + 'Your password has been cleared because of possible password leakage. ' + 'Please use the "Forgot your password?" button below to set a new password ' + 'for your account.', + ) + return super().clean() + + +class AnyEmailLoginView(LoginView): + """LoginView that allows any email address as the username + + Redirects to the missing_person page instead of logging in if the user does not have a Person + """ + form_class = AnyEmailAuthenticationForm + + def form_valid(self, form): + """Security check complete. Log the user in if they have a Person.""" + user = form.get_user() # user has authenticated at this point + if not hasattr(user, "person"): + logout(self.request) # should not be logged in yet, but just in case... + return render(self.request, "registration/missing_person.html") + return super().form_valid(form) + @login_required @person_required