From 226ebb17290b604ef29e82fb5c1fbac3594ac163 Mon Sep 17 00:00:00 2001 From: Hasan Ramezani Date: Fri, 23 Aug 2019 17:14:07 +0200 Subject: [PATCH] Fixed #28622 -- Allowed specifying password reset link expiration in seconds and deprecated PASSWORD_RESET_TIMEOUT_DAYS. --- django/conf/__init__.py | 35 ++++++++ django/conf/global_settings.py | 4 + django/contrib/auth/tokens.py | 26 +++--- docs/internals/deprecation.txt | 2 + docs/ref/settings.txt | 17 ++++ docs/releases/3.1.txt | 8 +- .../test_password_reset_timeout_days.py | 88 +++++++++++++++++++ tests/auth_tests/test_tokens.py | 25 +++--- 8 files changed, 178 insertions(+), 27 deletions(-) create mode 100644 tests/auth_tests/test_password_reset_timeout_days.py diff --git a/django/conf/__init__.py b/django/conf/__init__.py index 5731fe912c..b32e56184d 100644 --- a/django/conf/__init__.py +++ b/django/conf/__init__.py @@ -9,14 +9,23 @@ for a list of all possible variables. import importlib import os import time +import traceback +import warnings from pathlib import Path +import django from django.conf import global_settings from django.core.exceptions import ImproperlyConfigured +from django.utils.deprecation import RemovedInDjango40Warning from django.utils.functional import LazyObject, empty ENVIRONMENT_VARIABLE = "DJANGO_SETTINGS_MODULE" +PASSWORD_RESET_TIMEOUT_DAYS_DEPRECATED_MSG = ( + 'The PASSWORD_RESET_TIMEOUT_DAYS setting is deprecated. Use ' + 'PASSWORD_RESET_TIMEOUT instead.' +) + class SettingsReference(str): """ @@ -105,6 +114,20 @@ class LazySettings(LazyObject): """Return True if the settings have already been configured.""" return self._wrapped is not empty + @property + def PASSWORD_RESET_TIMEOUT_DAYS(self): + stack = traceback.extract_stack() + # Show a warning if the setting is used outside of Django. + # Stack index: -1 this line, -2 the caller. + filename, _, _, _ = stack[-2] + if not filename.startswith(os.path.dirname(django.__file__)): + warnings.warn( + PASSWORD_RESET_TIMEOUT_DAYS_DEPRECATED_MSG, + RemovedInDjango40Warning, + stacklevel=2, + ) + return self.__getattr__('PASSWORD_RESET_TIMEOUT_DAYS') + class Settings: def __init__(self, settings_module): @@ -137,6 +160,15 @@ class Settings: if not self.SECRET_KEY: raise ImproperlyConfigured("The SECRET_KEY setting must not be empty.") + if self.is_overridden('PASSWORD_RESET_TIMEOUT_DAYS'): + if self.is_overridden('PASSWORD_RESET_TIMEOUT'): + raise ImproperlyConfigured( + 'PASSWORD_RESET_TIMEOUT_DAYS/PASSWORD_RESET_TIMEOUT are ' + 'mutually exclusive.' + ) + setattr(self, 'PASSWORD_RESET_TIMEOUT', self.PASSWORD_RESET_TIMEOUT_DAYS * 60 * 60 * 24) + warnings.warn(PASSWORD_RESET_TIMEOUT_DAYS_DEPRECATED_MSG, RemovedInDjango40Warning) + if hasattr(time, 'tzset') and self.TIME_ZONE: # When we can, attempt to validate the timezone. If we can't find # this file, no check happens and it's harmless. @@ -180,6 +212,9 @@ class UserSettingsHolder: def __setattr__(self, name, value): self._deleted.discard(name) + if name == 'PASSWORD_RESET_TIMEOUT_DAYS': + setattr(self, 'PASSWORD_RESET_TIMEOUT', value * 60 * 60 * 24) + warnings.warn(PASSWORD_RESET_TIMEOUT_DAYS_DEPRECATED_MSG, RemovedInDjango40Warning) super().__setattr__(name, value) def __delattr__(self, name): diff --git a/django/conf/global_settings.py b/django/conf/global_settings.py index 398696530a..75935df0e1 100644 --- a/django/conf/global_settings.py +++ b/django/conf/global_settings.py @@ -506,6 +506,10 @@ LOGOUT_REDIRECT_URL = None # The number of days a password reset link is valid for PASSWORD_RESET_TIMEOUT_DAYS = 3 +# The minimum number of seconds a password reset link is valid for +# (default: 3 days). +PASSWORD_RESET_TIMEOUT = 60 * 60 * 24 * 3 + # the first hasher in this list is the preferred algorithm. any # password using different algorithms will be converted automatically # upon login diff --git a/django/contrib/auth/tokens.py b/django/contrib/auth/tokens.py index 80fe699395..c4f2641003 100644 --- a/django/contrib/auth/tokens.py +++ b/django/contrib/auth/tokens.py @@ -1,4 +1,4 @@ -from datetime import date +from datetime import datetime from django.conf import settings from django.utils.crypto import constant_time_compare, salted_hmac @@ -18,7 +18,7 @@ class PasswordResetTokenGenerator: Return a token that can be used once to do a password reset for the given user. """ - return self._make_token_with_timestamp(user, self._num_days(self._today())) + return self._make_token_with_timestamp(user, self._num_seconds(self._now())) def check_token(self, user, token): """ @@ -41,19 +41,15 @@ class PasswordResetTokenGenerator: if not constant_time_compare(self._make_token_with_timestamp(user, ts), token): return False - # Check the timestamp is within limit. Timestamps are rounded to - # midnight (server time) providing a resolution of only 1 day. If a - # link is generated 5 minutes before midnight and used 6 minutes later, - # that counts as 1 day. Therefore, PASSWORD_RESET_TIMEOUT_DAYS = 1 means - # "at least 1 day, could be up to 2." - if (self._num_days(self._today()) - ts) > settings.PASSWORD_RESET_TIMEOUT_DAYS: + # Check the timestamp is within limit. + if (self._num_seconds(self._now()) - ts) > settings.PASSWORD_RESET_TIMEOUT: return False return True def _make_token_with_timestamp(self, user, timestamp): - # timestamp is number of days since 2001-1-1. Converted to - # base 36, this gives us a 3 digit string until about 2121 + # timestamp is number of seconds since 2001-1-1. Converted to base 36, + # this gives us a 6 digit string until about 2069. ts_b36 = int_to_base36(timestamp) hash_string = salted_hmac( self.key_salt, @@ -71,7 +67,7 @@ class PasswordResetTokenGenerator: same password is chosen, due to password salting). 2. The last_login field will usually be updated very shortly after a password reset. - Failing those things, settings.PASSWORD_RESET_TIMEOUT_DAYS eventually + Failing those things, settings.PASSWORD_RESET_TIMEOUT eventually invalidates the token. Running this data through salted_hmac() prevents password cracking @@ -82,12 +78,12 @@ class PasswordResetTokenGenerator: login_timestamp = '' if user.last_login is None else user.last_login.replace(microsecond=0, tzinfo=None) return str(user.pk) + user.password + str(login_timestamp) + str(timestamp) - def _num_days(self, dt): - return (dt - date(2001, 1, 1)).days + def _num_seconds(self, dt): + return int((dt - datetime(2001, 1, 1)).total_seconds()) - def _today(self): + def _now(self): # Used for mocking in tests - return date.today() + return datetime.now() default_token_generator = PasswordResetTokenGenerator() diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 03e9fa93ce..d79641a19c 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -34,6 +34,8 @@ details on these changes. * ``django.utils.http.is_safe_url()`` will be removed. +* The ``PASSWORD_RESET_TIMEOUT_DAYS`` setting will be removed. + See the :ref:`Django 3.1 release notes ` for more details on these changes. diff --git a/docs/ref/settings.txt b/docs/ref/settings.txt index a517a72f2f..fe103162ef 100644 --- a/docs/ref/settings.txt +++ b/docs/ref/settings.txt @@ -2872,6 +2872,19 @@ doesn't have a ``next_page`` attribute. If ``None``, no redirect will be performed and the logout view will be rendered. +.. setting:: PASSWORD_RESET_TIMEOUT + +``PASSWORD_RESET_TIMEOUT`` +-------------------------- + +.. versionadded:: 3.1 + +Default: ``259200`` (3 days, in seconds) + +The minimum number of seconds a password reset link is valid for. + +Used by the :class:`~django.contrib.auth.views.PasswordResetConfirmView`. + .. setting:: PASSWORD_RESET_TIMEOUT_DAYS ``PASSWORD_RESET_TIMEOUT_DAYS`` @@ -2884,6 +2897,10 @@ when the link is generated, it will be valid for up to a day longer. Used by the :class:`~django.contrib.auth.views.PasswordResetConfirmView`. +.. deprecated:: 3.1 + + This setting is deprecated. Use :setting:`PASSWORD_RESET_TIMEOUT` instead. + .. setting:: PASSWORD_HASHERS ``PASSWORD_HASHERS`` diff --git a/docs/releases/3.1.txt b/docs/releases/3.1.txt index 865ae60ec3..d448fc57b0 100644 --- a/docs/releases/3.1.txt +++ b/docs/releases/3.1.txt @@ -46,6 +46,11 @@ Minor features * The default iteration count for the PBKDF2 password hasher is increased from 180,000 to 216,000. +* Added the :setting:`PASSWORD_RESET_TIMEOUT` setting to define the minimum + number of seconds a password reset link is valid for. This is encouraged + instead of deprecated ``PASSWORD_RESET_TIMEOUT_DAYS``, which will be removed + in Django 4.0. + :mod:`django.contrib.contenttypes` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -222,7 +227,8 @@ Features deprecated in 3.1 Miscellaneous ------------- -* ... +* ``PASSWORD_RESET_TIMEOUT_DAYS`` setting is deprecated in favor of + :setting:`PASSWORD_RESET_TIMEOUT`. .. _removed-features-3.1: diff --git a/tests/auth_tests/test_password_reset_timeout_days.py b/tests/auth_tests/test_password_reset_timeout_days.py new file mode 100644 index 0000000000..db9aa62726 --- /dev/null +++ b/tests/auth_tests/test_password_reset_timeout_days.py @@ -0,0 +1,88 @@ +import sys +from datetime import datetime, timedelta +from types import ModuleType + +from django.conf import ( + PASSWORD_RESET_TIMEOUT_DAYS_DEPRECATED_MSG, Settings, settings, +) +from django.contrib.auth.models import User +from django.contrib.auth.tokens import PasswordResetTokenGenerator +from django.core.exceptions import ImproperlyConfigured +from django.test import TestCase, ignore_warnings +from django.utils.deprecation import RemovedInDjango40Warning + + +class DeprecationTests(TestCase): + msg = PASSWORD_RESET_TIMEOUT_DAYS_DEPRECATED_MSG + + @ignore_warnings(category=RemovedInDjango40Warning) + def test_timeout(self): + """The token is valid after n days, but no greater.""" + # Uses a mocked version of PasswordResetTokenGenerator so we can change + # the value of 'now'. + class Mocked(PasswordResetTokenGenerator): + def __init__(self, now): + self._now_val = now + + def _now(self): + return self._now_val + + user = User.objects.create_user('tokentestuser', 'test2@example.com', 'testpw') + p0 = PasswordResetTokenGenerator() + tk1 = p0.make_token(user) + p1 = Mocked(datetime.now() + timedelta(settings.PASSWORD_RESET_TIMEOUT_DAYS)) + self.assertTrue(p1.check_token(user, tk1)) + p2 = Mocked(datetime.now() + timedelta(settings.PASSWORD_RESET_TIMEOUT_DAYS + 1)) + self.assertFalse(p2.check_token(user, tk1)) + with self.settings(PASSWORD_RESET_TIMEOUT_DAYS=1): + self.assertEqual(settings.PASSWORD_RESET_TIMEOUT, 60 * 60 * 24) + p3 = Mocked(datetime.now() + timedelta(settings.PASSWORD_RESET_TIMEOUT_DAYS)) + self.assertTrue(p3.check_token(user, tk1)) + p4 = Mocked(datetime.now() + timedelta(settings.PASSWORD_RESET_TIMEOUT_DAYS + 1)) + self.assertFalse(p4.check_token(user, tk1)) + + def test_override_settings_warning(self): + with self.assertRaisesMessage(RemovedInDjango40Warning, self.msg): + with self.settings(PASSWORD_RESET_TIMEOUT_DAYS=2): + pass + + def test_settings_init_warning(self): + settings_module = ModuleType('fake_settings_module') + settings_module.SECRET_KEY = 'foo' + settings_module.PASSWORD_RESET_TIMEOUT_DAYS = 2 + sys.modules['fake_settings_module'] = settings_module + try: + with self.assertRaisesMessage(RemovedInDjango40Warning, self.msg): + Settings('fake_settings_module') + finally: + del sys.modules['fake_settings_module'] + + def test_access_warning(self): + with self.assertRaisesMessage(RemovedInDjango40Warning, self.msg): + settings.PASSWORD_RESET_TIMEOUT_DAYS + # Works a second time. + with self.assertRaisesMessage(RemovedInDjango40Warning, self.msg): + settings.PASSWORD_RESET_TIMEOUT_DAYS + + @ignore_warnings(category=RemovedInDjango40Warning) + def test_access(self): + with self.settings(PASSWORD_RESET_TIMEOUT_DAYS=2): + self.assertEqual(settings.PASSWORD_RESET_TIMEOUT_DAYS, 2) + # Works a second time. + self.assertEqual(settings.PASSWORD_RESET_TIMEOUT_DAYS, 2) + + def test_use_both_settings_init_error(self): + msg = ( + 'PASSWORD_RESET_TIMEOUT_DAYS/PASSWORD_RESET_TIMEOUT are ' + 'mutually exclusive.' + ) + settings_module = ModuleType('fake_settings_module') + settings_module.SECRET_KEY = 'foo' + settings_module.PASSWORD_RESET_TIMEOUT_DAYS = 2 + settings_module.PASSWORD_RESET_TIMEOUT = 2000 + sys.modules['fake_settings_module'] = settings_module + try: + with self.assertRaisesMessage(ImproperlyConfigured, msg): + Settings('fake_settings_module') + finally: + del sys.modules['fake_settings_module'] diff --git a/tests/auth_tests/test_tokens.py b/tests/auth_tests/test_tokens.py index ede7b007fa..6d6caab9b1 100644 --- a/tests/auth_tests/test_tokens.py +++ b/tests/auth_tests/test_tokens.py @@ -1,4 +1,4 @@ -from datetime import date, timedelta +from datetime import datetime, timedelta from django.conf import settings from django.contrib.auth.models import User @@ -28,25 +28,28 @@ class TokenGeneratorTest(TestCase): self.assertEqual(tk1, tk2) def test_timeout(self): - """ - The token is valid after n days, but no greater. - """ + """The token is valid after n seconds, but no greater.""" # Uses a mocked version of PasswordResetTokenGenerator so we can change - # the value of 'today' + # the value of 'now'. class Mocked(PasswordResetTokenGenerator): - def __init__(self, today): - self._today_val = today + def __init__(self, now): + self._now_val = now - def _today(self): - return self._today_val + def _now(self): + return self._now_val user = User.objects.create_user('tokentestuser', 'test2@example.com', 'testpw') p0 = PasswordResetTokenGenerator() tk1 = p0.make_token(user) - p1 = Mocked(date.today() + timedelta(settings.PASSWORD_RESET_TIMEOUT_DAYS)) + p1 = Mocked(datetime.now() + timedelta(seconds=settings.PASSWORD_RESET_TIMEOUT)) self.assertTrue(p1.check_token(user, tk1)) - p2 = Mocked(date.today() + timedelta(settings.PASSWORD_RESET_TIMEOUT_DAYS + 1)) + p2 = Mocked(datetime.now() + timedelta(seconds=(settings.PASSWORD_RESET_TIMEOUT + 1))) self.assertFalse(p2.check_token(user, tk1)) + with self.settings(PASSWORD_RESET_TIMEOUT=60 * 60): + p3 = Mocked(datetime.now() + timedelta(seconds=settings.PASSWORD_RESET_TIMEOUT)) + self.assertTrue(p3.check_token(user, tk1)) + p4 = Mocked(datetime.now() + timedelta(seconds=(settings.PASSWORD_RESET_TIMEOUT + 1))) + self.assertFalse(p4.check_token(user, tk1)) def test_check_token_with_nonexistent_token_and_user(self): user = User.objects.create_user('tokentestuser', 'test2@example.com', 'testpw')