From a4f0e9aec76ee40c938d3bf450ff63211b7ba1f1 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Tue, 20 Mar 2018 17:19:27 -0400 Subject: [PATCH] Fixed #28718 -- Allowed user to request a password reset if their password doesn't use an enabled hasher. Regression in aeb1389442d0f9669edf6660b747fd10693b63a7. Reverted changes to is_password_usable() from 703c266682be39f7153498ad0d8031231f12ee79 and documentation changes from 92f48680dbd2e02f2b33f6ad0e35b7d337889fb2. --- django/contrib/auth/base_user.py | 4 +--- django/contrib/auth/hashers.py | 18 ++++++++++-------- docs/ref/contrib/auth.txt | 12 +++++++++--- docs/releases/2.1.txt | 8 ++++++++ docs/topics/auth/passwords.txt | 12 ++++++++++-- tests/auth_tests/test_hashers.py | 8 +++++--- tests/auth_tests/test_models.py | 7 +++++++ 7 files changed, 50 insertions(+), 19 deletions(-) diff --git a/django/contrib/auth/base_user.py b/django/contrib/auth/base_user.py index a165677f4c..7e3161bd98 100644 --- a/django/contrib/auth/base_user.py +++ b/django/contrib/auth/base_user.py @@ -116,9 +116,7 @@ class AbstractBaseUser(models.Model): def has_usable_password(self): """ - Return False if set_unusable_password() has been called for this user, - or if the password is None, or if the password uses a hasher that's not - in the PASSWORD_HASHERS setting. + Return False if set_unusable_password() has been called for this user. """ return is_password_usable(self.password) diff --git a/django/contrib/auth/hashers.py b/django/contrib/auth/hashers.py index 3bab95e745..75664836dc 100644 --- a/django/contrib/auth/hashers.py +++ b/django/contrib/auth/hashers.py @@ -21,13 +21,11 @@ UNUSABLE_PASSWORD_SUFFIX_LENGTH = 40 # number of random chars to add after UNUS def is_password_usable(encoded): - if encoded is None or encoded.startswith(UNUSABLE_PASSWORD_PREFIX): - return False - try: - identify_hasher(encoded) - except ValueError: - return False - return True + """ + Return True if this password wasn't generated by + User.set_unusable_password(), i.e. make_password(None). + """ + return encoded is None or not encoded.startswith(UNUSABLE_PASSWORD_PREFIX) def check_password(password, encoded, setter=None, preferred='default'): @@ -42,7 +40,11 @@ def check_password(password, encoded, setter=None, preferred='default'): return False preferred = get_hasher(preferred) - hasher = identify_hasher(encoded) + try: + hasher = identify_hasher(encoded) + except ValueError: + # encoded is gibberish or uses a hasher that's no longer installed. + return False hasher_changed = hasher.algorithm != preferred.algorithm must_update = hasher_changed or preferred.must_update(encoded) diff --git a/docs/ref/contrib/auth.txt b/docs/ref/contrib/auth.txt index e05280367c..2b1aa9ae08 100644 --- a/docs/ref/contrib/auth.txt +++ b/docs/ref/contrib/auth.txt @@ -212,9 +212,15 @@ Methods Returns ``False`` if :meth:`~django.contrib.auth.models.User.set_unusable_password()` has - been called for this user, or if the password is ``None``, or if the - password uses a hasher that's not in the :setting:`PASSWORD_HASHERS` - setting. + been called for this user. + + .. versionchanged:: 2.1 + + In older versions, this also returns ``False`` if the password is + ``None`` or an empty string, or if the password uses a hasher + that's not in the :setting:`PASSWORD_HASHERS` setting. That + behavior is considered a bug as it prevents users with such + passwords from requesting a password reset. .. method:: get_group_permissions(obj=None) diff --git a/docs/releases/2.1.txt b/docs/releases/2.1.txt index 439dc275ce..463fad0b6c 100644 --- a/docs/releases/2.1.txt +++ b/docs/releases/2.1.txt @@ -358,6 +358,14 @@ Miscellaneous changed from 0 to an empty string, which mainly may require some adjustments in tests that compare HTML. +* :meth:`.User.has_usable_password` and the + :func:`~django.contrib.auth.hashers.is_password_usable` function no longer + return ``False`` if the password is ``None`` or an empty string, or if the + password uses a hasher that's not in the :setting:`PASSWORD_HASHERS` setting. + This undocumented behavior was a regression in Django 1.6 and prevented users + with such passwords from requesting a password reset. Audit your code to + confirm that your usage of these APIs don't rely on the old behavior. + .. _deprecated-features-2.1: Features deprecated in 2.1 diff --git a/docs/topics/auth/passwords.txt b/docs/topics/auth/passwords.txt index 4dc6748075..0125118de6 100644 --- a/docs/topics/auth/passwords.txt +++ b/docs/topics/auth/passwords.txt @@ -409,8 +409,16 @@ from the ``User`` model. .. function:: is_password_usable(encoded_password) - Checks if the given string is a hashed password that has a chance - of being verified against :func:`check_password`. + Returns ``False`` if the password is a result of + :meth:`.User.set_unusable_password`. + + .. versionchanged:: 2.1 + + In older versions, this also returns ``False`` if the password is + ``None`` or an empty string, or if the password uses a hasher that's + not in the :setting:`PASSWORD_HASHERS` setting. That behavior is + considered a bug as it prevents users with such passwords from + requesting a password reset. .. _password-validation: diff --git a/tests/auth_tests/test_hashers.py b/tests/auth_tests/test_hashers.py index 51aaa3d72c..ab34ad78b6 100644 --- a/tests/auth_tests/test_hashers.py +++ b/tests/auth_tests/test_hashers.py @@ -276,9 +276,11 @@ class TestUtilsHashPass(SimpleTestCase): with self.assertRaisesMessage(ValueError, msg % 'lolcat'): identify_hasher('lolcat$salt$hash') - def test_bad_encoded(self): - self.assertFalse(is_password_usable('lètmein_badencoded')) - self.assertFalse(is_password_usable('')) + def test_is_password_usable(self): + passwords = ('lètmein_badencoded', '', None) + for password in passwords: + with self.subTest(password=password): + self.assertIs(is_password_usable(password), True) def test_low_level_pbkdf2(self): hasher = PBKDF2PasswordHasher() diff --git a/tests/auth_tests/test_models.py b/tests/auth_tests/test_models.py index 9438ed8aff..bd83b47677 100644 --- a/tests/auth_tests/test_models.py +++ b/tests/auth_tests/test_models.py @@ -158,6 +158,13 @@ class UserManagerTestCase(TestCase): class AbstractBaseUserTests(TestCase): + def test_has_usable_password(self): + """ + Passwords are usable even if they don't correspond to a hasher in + settings.PASSWORD_HASHERS. + """ + self.assertIs(User(password='some-gibbberish').has_usable_password(), True) + def test_normalize_username(self): self.assertEqual(IntegerUsernameUser().normalize_username(123), 123)