Fix signing up with very-common password

This prevents an integer-parsing error from causing 500s when trying to
sign up with a password that's quite common in HIBP (so common that
there are over 999 known pwnages and a comma is included in the count).

I should probably add some unit tests, but I expect this to be a
short-lived workaround, and I've done some end-to-end testing locally.

ws/auth.py

@@ -0,0 +1,64 @@
+import logging
+import re
+from typing import Optional
+
+from django.core.exceptions import ValidationError
+from django.utils.translation import ngettext
+from pwned_passwords_django.api import pwned_password
+from pwned_passwords_django.validators import PwnedPasswordsValidator
+
+from ws import settings
+
+logger = logging.getLogger(__name__)
+
+# As of 2022-08-27, pwned_password sometimes includes commas in `times`
+# This hack can go away once the library is updated to handle commas
+# https://github.com/ubernostrum/pwned-passwords-django/issues/35
+INT_WITH_COMMA = re.compile(
+    r"invalid literal for int\(\) with base 10: '(?P<times>[\d,]+)'"
+)
+
+
+def times_seen_in_hibp(password: str) -> Optional[int]:
+    """Return times password has been seen in HIBP."""
+    if settings.DEBUG and password in settings.WHITELISTED_BAD_PASSWORDS:
+        return 0
+
+    try:
+        return pwned_password(password)
+    except ValueError as err:
+        has_comma = INT_WITH_COMMA.match(str(err))
+        if not has_comma:
+            raise
+        return int(has_comma.group('times').replace(',', ''))
+    except Exception:  # pylint: disable=broad-except
+        # Let Sentry know we had problems, but don't break the flow.
+        # Sentry should scrub `password` automatically.
+        logger.exception("Encountered an error with django-pwned-passwords")
+        return None
+
+
+class CommaPwnedPasswordsValidator(PwnedPasswordsValidator):
+    """Temporarily work around an issue with `pwned-passwords-django`.
+
+    This prevents 500s when trying to sign up with a very common password.
+    Specifically, if a comma is found in the count of times the password is
+    seen, we should fix the parsing to an integer.
+    """
+
+    def validate(self, password, user=None):
+        try:
+            super().validate(password, user=user)
+        except ValueError as err:
+            has_comma = INT_WITH_COMMA.match(str(err))
+            if not has_comma:
+                raise
+
+            amount = int(has_comma.group('times').replace(',', ''))
+            raise ValidationError(  # pylint:disable=raise-missing-from
+                ngettext(
+                    self.error_message["singular"], self.error_message["plural"], amount
+                ),
+                params={"amount": amount},
+                code="pwned_password",
+            )

ws/settings.py

@@ -76,7 +76,8 @@
 )
 
 AUTH_PASSWORD_VALIDATORS = [
-    {'NAME': 'pwned_passwords_django.validators.PwnedPasswordsValidator'}
+    # {'NAME': 'pwned_passwords_django.validators.PwnedPasswordsValidator'}
+    {'NAME': 'ws.auth.CommaPwnedPasswordsValidator'}
 ]
 
 LOGIN_REDIRECT_URL = '/'

ws/tests/views/test_account.py

@@ -7,7 +7,7 @@
 from freezegun import freeze_time
 from pwned_passwords_django import api
 
-from ws import models
+from ws import auth, models
 from ws.tests import factories
 from ws.views import account
 
@@ -35,7 +35,7 @@ def test_previously_insecure_password_marked_secure(self):
         """
         par = factories.ParticipantFactory.create(user=self.user)
         factories.PasswordQualityFactory.create(participant=par, is_insecure=True)
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.return_value = None  # (API was down)
             response = self._login()
 
@@ -50,7 +50,7 @@ def test_previously_insecure_password_marked_secure(self):
         # Because the API was down, we did not write that we checked the password
         self.assertIsNone(quality.last_checked)
 
-    @mock.patch.object(account, 'settings')
+    @mock.patch.object(auth, 'settings')
     def test_known_bad_password(self, mocked_settings):
         """We include a debug mode that supports passing known bad passwords."""
         par = factories.ParticipantFactory.create(user_id=self.user.pk)
@@ -59,7 +59,7 @@ def test_known_bad_password(self, mocked_settings):
         mocked_settings.DEBUG = True
         mocked_settings.WHITELISTED_BAD_PASSWORDS = ['football']
 
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             response = self.client.post('/accounts/login/', self.form_data)
 
         # We don't bother hitting the API!
@@ -83,7 +83,7 @@ def test_known_bad_password(self, mocked_settings):
         self.assertEqual(mocked_settings.WHITELISTED_BAD_PASSWORDS, ['football'])
 
         # This time, we hit the API and mark the user as having an insecure password.
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.return_value = 2022
             response = self.client.post('/accounts/login/', self.form_data)
         pwned_password.assert_called_once_with('football')
@@ -96,7 +96,7 @@ def test_previously_secure_password(self):
         factories.PasswordQualityFactory.create(
             participant=participant, is_insecure=True
         )
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.return_value = 15  # password found in 15 separate breaches!
             with mock.patch.object(account.logger, 'info') as logger_info:
                 response = self._login()
@@ -122,7 +122,7 @@ def test_user_without_participant(self):
         """Users may log in without having an associated participant!"""
         self.assertIsNone(models.Participant.from_user(self.user))
 
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.return_value = 1  # password found in 1 breach!
             with mock.patch.object(account.logger, 'info') as logger_info:
                 response = self._login()
@@ -148,7 +148,7 @@ def test_password_is_marked_secure(self):
         """If the login's password returned no breaches, then we can mark it secure."""
         factories.ParticipantFactory.create(user_id=self.user.pk)
 
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.return_value = 0  # (0 breaches total)
             response = self.client.post('/accounts/login/', self.form_data)
         pwned_password.assert_called_once_with('football')
@@ -167,7 +167,7 @@ def test_password_is_marked_secure(self):
 
     def test_redirect_should_be_preserved(self):
         """If attempting to login with a redirect, it should be preserved!."""
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.return_value = 1  # Password has been breached once!
             response = self.client.post(
                 '/accounts/login/?next=/preferences/lottery/', self.form_data
@@ -182,7 +182,7 @@ def test_password_over_999_times(self):
         """Test a quick hack for often-breached passwords that include commas in the count."""
         participant = factories.ParticipantFactory.create(user_id=self.user.pk)
 
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.side_effect = lambda pw: int("1,234")
             self._login()
         pwned_password.assert_called_once_with('football')
@@ -194,7 +194,7 @@ def test_uncaught_error_with_django_hipb(self):
         """We don't block login if there was an unhandled error."""
         participant = factories.ParticipantFactory.create(user_id=self.user.pk)
 
-        with mock.patch.object(account, 'pwned_password') as pwned_password:
+        with mock.patch.object(auth, 'pwned_password') as pwned_password:
             pwned_password.side_effect = RuntimeError
             self._login()
         pwned_password.assert_called_once_with('football')

ws/views/account.py

@@ -2,7 +2,6 @@
 Views relating to account management.
 """
 import logging
-import re
 from typing import Optional, Tuple
 from urllib.parse import urlencode
 
@@ -12,22 +11,14 @@
 from django.http import HttpResponse
 from django.urls import reverse
 from django.utils.decorators import method_decorator
-from pwned_passwords_django.api import pwned_password
 
-from ws import models, settings
+from ws import models
+from ws.auth import times_seen_in_hibp
 from ws.utils.dates import local_now
 
 logger = logging.getLogger(__name__)
 
 
-# As of 2022-08-27, pwned_password sometimes includes commas in `times`
-# This hack can go away once the library is updated to handle commas
-# https://github.com/ubernostrum/pwned-passwords-django/issues/35
-INT_WITH_COMMA = re.compile(
-    r"invalid literal for int\(\) with base 10: '(?P<times>[\d,]+)'"
-)
-
-
 class CustomPasswordChangeView(PasswordChangeView):
     """Custom password change view that makes two key changes:
 
@@ -76,25 +67,6 @@ class CheckIfPwnedOnLoginView(LoginView):
       of breached passwords - we should check on every login.
     """
 
-    @staticmethod
-    def _times_seen(password: str) -> Optional[int]:
-        """Return times password has been seen in HIBP."""
-        if settings.DEBUG and password in settings.WHITELISTED_BAD_PASSWORDS:
-            return 0
-
-        try:
-            return pwned_password(password)
-        except ValueError as err:
-            has_comma = INT_WITH_COMMA.match(str(err))
-            if not has_comma:
-                raise
-            return int(has_comma.group('times').replace(',', ''))
-        except Exception:  # pylint: disable=broad-except
-            # Let Sentry know we had problems, but don't break the flow.
-            # Sentry should scrub `password` automatically.
-            logger.exception("Encountered an error with django-pwned-passwords")
-            return None
-
     def _form_valid_perform_login(self, form) -> Tuple[Optional[int], HttpResponse]:
         """Performs login with a correct username/password.
 
@@ -103,7 +75,7 @@ def _form_valid_perform_login(self, form) -> Tuple[Optional[int], HttpResponse]:
 
         As a side effect, this populates `self.request.user`
         """
-        times_seen = self._times_seen(form.cleaned_data['password'])
+        times_seen = times_seen_in_hibp(form.cleaned_data['password'])
 
         if times_seen:
             change_password_url = reverse('account_change_password')