Skip to content

Commit

Permalink
Fixed #28622 -- Allowed specifying password reset link expiration in …
Browse files Browse the repository at this point in the history
…seconds and deprecated PASSWORD_RESET_TIMEOUT_DAYS.
  • Loading branch information
hramezani authored and felixxm committed Sep 20, 2019
1 parent 0719edc commit 226ebb1
Show file tree
Hide file tree
Showing 8 changed files with 178 additions and 27 deletions.
35 changes: 35 additions & 0 deletions django/conf/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,23 @@
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):
"""
Expand Down Expand Up @@ -105,6 +114,20 @@ def configured(self):
"""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):
Expand Down Expand Up @@ -137,6 +160,15 @@ def __init__(self, settings_module):
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.
Expand Down Expand Up @@ -180,6 +212,9 @@ def __getattr__(self, name):

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):
Expand Down
4 changes: 4 additions & 0 deletions django/conf/global_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,10 @@ def gettext_noop(s):
# 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
Expand Down
26 changes: 11 additions & 15 deletions django/contrib/auth/tokens.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -18,7 +18,7 @@ def make_token(self, user):
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):
"""
Expand All @@ -41,19 +41,15 @@ def check_token(self, user, token):
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,
Expand All @@ -71,7 +67,7 @@ def _make_hash_value(self, user, timestamp):
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
Expand All @@ -82,12 +78,12 @@ def _make_hash_value(self, user, timestamp):
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()
2 changes: 2 additions & 0 deletions docs/internals/deprecation.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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 <deprecated-features-3.1>` for more
details on these changes.

Expand Down
17 changes: 17 additions & 0 deletions docs/ref/settings.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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``
Expand All @@ -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``
Expand Down
8 changes: 7 additions & 1 deletion docs/releases/3.1.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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`
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down Expand Up @@ -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:

Expand Down
88 changes: 88 additions & 0 deletions tests/auth_tests/test_password_reset_timeout_days.py
Original file line number Diff line number Diff line change
@@ -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']
25 changes: 14 additions & 11 deletions tests/auth_tests/test_tokens.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -28,25 +28,28 @@ def test_10265(self):
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')
Expand Down

0 comments on commit 226ebb1

Please sign in to comment.