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

@@ -50,8 +50,6 @@
 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 @@ def tearDown(self):
         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 @@ def _test_login(url):
         # 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 @@ def username_in_htpasswd_file(self, username):
 # 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 @@ def test_create_account_failure_template(self):
         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 @@ def test_create_existing_account(self):
         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 @@ def test_ietfauth_profile(self):
         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 @@ def test_ietfauth_profile(self):
     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 @@ def test_nomcom_dressing_on_profile(self):
 
 
     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 @@ def test_reset_password(self):
         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 @@ def test_review_overview(self):
             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 @@ def test_htpasswd_file_with_htpasswd_binary(self):
 
 
     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 @@ def test_change_password(self):
 
     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

ietf/ietfauth/urls.py

@@ -14,7 +14,7 @@
         url(r'^confirmnewemail/(?P<auth>[^/]+)/$', views.confirm_new_email),
         url(r'^create/$', views.create_account),
         url(r'^create/confirm/(?P<auth>[^/]+)/$', 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),

ietf/ietfauth/views.py

@@ -45,7 +45,7 @@
 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.
     """
-
-    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
+    _unauthenticated_user = None
+
+    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