From fd23c06023a0585ee743c0752dc94da66694cf63 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Mon, 31 Mar 2014 20:16:09 -0400 Subject: [PATCH] Fixed #21649 -- Added optional invalidation of sessions when user password changes. Thanks Paul McMillan, Aymeric Augustin, and Erik Romijn for reviews. --- .../project_template/project_name/settings.py | 1 + django/contrib/auth/__init__.py | 24 ++++++- django/contrib/auth/admin.py | 2 + django/contrib/auth/middleware.py | 19 +++++ django/contrib/auth/models.py | 9 ++- django/contrib/auth/tests/test_middleware.py | 35 +++++++++ django/contrib/auth/tests/test_views.py | 58 ++++++++++++++- django/contrib/auth/views.py | 8 ++- docs/ref/middleware.txt | 9 +++ docs/releases/1.7.txt | 9 +++ docs/topics/auth/customizing.txt | 7 ++ docs/topics/auth/default.txt | 71 +++++++++++++++++++ 12 files changed, 246 insertions(+), 6 deletions(-) create mode 100644 django/contrib/auth/tests/test_middleware.py diff --git a/django/conf/project_template/project_name/settings.py b/django/conf/project_template/project_name/settings.py index efe8091e81..49982995b8 100644 --- a/django/conf/project_template/project_name/settings.py +++ b/django/conf/project_template/project_name/settings.py @@ -43,6 +43,7 @@ MIDDLEWARE_CLASSES = ( 'django.middleware.common.CommonMiddleware', 'django.middleware.csrf.CsrfViewMiddleware', 'django.contrib.auth.middleware.AuthenticationMiddleware', + 'django.contrib.auth.middleware.SessionAuthenticationMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', ) diff --git a/django/contrib/auth/__init__.py b/django/contrib/auth/__init__.py index 7b44fe2351..95511dd688 100644 --- a/django/contrib/auth/__init__.py +++ b/django/contrib/auth/__init__.py @@ -12,6 +12,7 @@ from .signals import user_logged_in, user_logged_out, user_login_failed SESSION_KEY = '_auth_user_id' BACKEND_SESSION_KEY = '_auth_user_backend' +HASH_SESSION_KEY = '_auth_user_hash' REDIRECT_FIELD_NAME = 'next' @@ -76,11 +77,16 @@ def login(request, user): have to reauthenticate on every request. Note that data set during the anonymous session is retained when the user logs in. """ + session_auth_hash = '' if user is None: user = request.user - # TODO: It would be nice to support different login methods, like signed cookies. + if hasattr(user, 'get_session_auth_hash'): + session_auth_hash = user.get_session_auth_hash() + if SESSION_KEY in request.session: - if request.session[SESSION_KEY] != user.pk: + if request.session[SESSION_KEY] != user.pk or ( + session_auth_hash and + request.session[HASH_SESSION_KEY] != session_auth_hash): # To avoid reusing another user's session, create a new, empty # session if the existing session corresponds to a different # authenticated user. @@ -89,6 +95,7 @@ def login(request, user): request.session.cycle_key() request.session[SESSION_KEY] = user.pk request.session[BACKEND_SESSION_KEY] = user.backend + request.session[HASH_SESSION_KEY] = session_auth_hash if hasattr(request, 'user'): request.user = user rotate_token(request) @@ -158,4 +165,17 @@ def get_permission_codename(action, opts): return '%s_%s' % (action, opts.model_name) +def update_session_auth_hash(request, user): + """ + Updating a user's password logs out all sessions for the user if + django.contrib.auth.middleware.SessionAuthenticationMiddleware is enabled. + + This function takes the current request and the updated user object from + which the new session hash will be derived and updates the session hash + appropriately to prevent a password change from logging out the session + from which the password was changed. + """ + if hasattr(user, 'get_session_auth_hash') and request.user == user: + request.session[HASH_SESSION_KEY] = user.get_session_auth_hash() + default_app_config = 'django.contrib.auth.apps.AuthConfig' diff --git a/django/contrib/auth/admin.py b/django/contrib/auth/admin.py index d8fff7ce0f..2eab7efec8 100644 --- a/django/contrib/auth/admin.py +++ b/django/contrib/auth/admin.py @@ -3,6 +3,7 @@ from django.conf import settings from django.conf.urls import url from django.contrib import admin from django.contrib.admin.options import IS_POPUP_VAR +from django.contrib.auth import update_session_auth_hash from django.contrib.auth.forms import (UserCreationForm, UserChangeForm, AdminPasswordChangeForm) from django.contrib.auth.models import User, Group @@ -131,6 +132,7 @@ class UserAdmin(admin.ModelAdmin): self.log_change(request, request.user, change_message) msg = ugettext('Password changed successfully.') messages.success(request, msg) + update_session_auth_hash(request, form.user) return HttpResponseRedirect('..') else: form = self.change_password_form(user) diff --git a/django/contrib/auth/middleware.py b/django/contrib/auth/middleware.py index 31d42e13cf..b2f392262a 100644 --- a/django/contrib/auth/middleware.py +++ b/django/contrib/auth/middleware.py @@ -2,6 +2,7 @@ from django.contrib import auth from django.contrib.auth import load_backend from django.contrib.auth.backends import RemoteUserBackend from django.core.exceptions import ImproperlyConfigured +from django.utils.crypto import constant_time_compare from django.utils.functional import SimpleLazyObject @@ -22,6 +23,24 @@ class AuthenticationMiddleware(object): request.user = SimpleLazyObject(lambda: get_user(request)) +class SessionAuthenticationMiddleware(object): + """ + Middleware for invalidating a user's sessions that don't correspond to the + user's current session authentication hash (generated based on the user's + password for AbstractUser). + """ + def process_request(self, request): + user = request.user + if user and hasattr(user, 'get_session_auth_hash'): + session_hash = request.session.get(auth.HASH_SESSION_KEY) + session_hash_verified = session_hash and constant_time_compare( + session_hash, + user.get_session_auth_hash() + ) + if not session_hash_verified: + auth.logout(request) + + class RemoteUserMiddleware(object): """ Middleware for utilizing Web-server-provided authentication. diff --git a/django/contrib/auth/models.py b/django/contrib/auth/models.py index 5b89fe845c..8cc28bc85d 100644 --- a/django/contrib/auth/models.py +++ b/django/contrib/auth/models.py @@ -4,7 +4,7 @@ from django.core.mail import send_mail from django.core import validators from django.db import models from django.db.models.manager import EmptyManager -from django.utils.crypto import get_random_string +from django.utils.crypto import get_random_string, salted_hmac from django.utils import six from django.utils.translation import ugettext_lazy as _ from django.utils import timezone @@ -249,6 +249,13 @@ class AbstractBaseUser(models.Model): def get_short_name(self): raise NotImplementedError('subclasses of AbstractBaseUser must provide a get_short_name() method.') + def get_session_auth_hash(self): + """ + Returns an HMAC of the password field. + """ + key_salt = "django.contrib.auth.models.AbstractBaseUser.get_session_auth_hash" + return salted_hmac(key_salt, self.password).hexdigest() + # A few helper functions for common logic between User and AnonymousUser. def _user_get_all_permissions(user, obj): diff --git a/django/contrib/auth/tests/test_middleware.py b/django/contrib/auth/tests/test_middleware.py new file mode 100644 index 0000000000..2bdc77a16e --- /dev/null +++ b/django/contrib/auth/tests/test_middleware.py @@ -0,0 +1,35 @@ +from django.contrib.auth.middleware import SessionAuthenticationMiddleware +from django.contrib.auth.models import User +from django.http import HttpRequest +from django.test import TestCase + + +class TestSessionAuthenticationMiddleware(TestCase): + def setUp(self): + self.user_password = 'test_password' + self.user = User.objects.create_user('test_user', + 'test@example.com', + self.user_password) + + def test_changed_password_invalidates_session(self): + """ + Tests that changing a user's password invalidates the session. + """ + verification_middleware = SessionAuthenticationMiddleware() + self.assertTrue(self.client.login( + username=self.user.username, + password=self.user_password, + )) + request = HttpRequest() + request.session = self.client.session + request.user = self.user + verification_middleware.process_request(request) + self.assertIsNotNone(request.user) + self.assertFalse(request.user.is_anonymous()) + + # After password change, user should be anonymous + request.user.set_password('new_password') + request.user.save() + verification_middleware.process_request(request) + self.assertIsNotNone(request.user) + self.assertTrue(request.user.is_anonymous()) diff --git a/django/contrib/auth/tests/test_views.py b/django/contrib/auth/tests/test_views.py index c9df787c06..01e3825bf4 100644 --- a/django/contrib/auth/tests/test_views.py +++ b/django/contrib/auth/tests/test_views.py @@ -49,9 +49,9 @@ class AuthViewsTestCase(TestCase): fixtures = ['authtestdata.json'] urls = 'django.contrib.auth.tests.urls' - def login(self, password='password'): + def login(self, username='testclient', password='password'): response = self.client.post('/login/', { - 'username': 'testclient', + 'username': username, 'password': password, }) self.assertTrue(SESSION_KEY in self.client.session) @@ -443,6 +443,25 @@ class ChangePasswordTest(AuthViewsTestCase): self.assertURLEqual(response.url, '/password_reset/') +@override_settings(MIDDLEWARE_CLASSES=list(settings.MIDDLEWARE_CLASSES) + [ + 'django.contrib.auth.middleware.SessionAuthenticationMiddleware' +]) +class SessionAuthenticationTests(AuthViewsTestCase): + def test_user_password_change_updates_session(self): + """ + #21649 - Ensure contrib.auth.views.password_change updates the user's + session auth hash after a password change so the session isn't logged out. + """ + self.login() + response = self.client.post('/password_change/', { + 'old_password': 'password', + 'new_password1': 'password1', + 'new_password2': 'password1', + }) + # if the hash isn't updated, retrieving the redirection page will fail. + self.assertRedirects(response, '/password_change/done/') + + @skipIfCustomUser class LoginTest(AuthViewsTestCase): @@ -546,6 +565,36 @@ class LoginTest(AuthViewsTestCase): # Check the CSRF token switched self.assertNotEqual(token1, token2) + def test_session_key_flushed_on_login(self): + """ + To avoid reusing another user's session, ensure a new, empty session is + created if the existing session corresponds to a different authenticated + user. + """ + self.login() + original_session_key = self.client.session.session_key + + self.login(username='staff') + self.assertNotEqual(original_session_key, self.client.session.session_key) + + def test_session_key_flushed_on_login_after_password_change(self): + """ + As above, but same user logging in after a password change. + """ + self.login() + original_session_key = self.client.session.session_key + + # If no password change, session key should not be flushed. + self.login() + self.assertEqual(original_session_key, self.client.session.session_key) + + user = User.objects.get(username='testclient') + user.set_password('foobar') + user.save() + + self.login(password='foobar') + self.assertNotEqual(original_session_key, self.client.session.session_key) + @skipIfCustomUser class LoginURLSettings(AuthViewsTestCase): @@ -731,6 +780,11 @@ class LogoutTest(AuthViewsTestCase): @skipIfCustomUser @override_settings( + # Redirect in test_user_change_password will fail if session auth hash + # isn't updated after password change (#21649) + MIDDLEWARE_CLASSES=list(settings.MIDDLEWARE_CLASSES) + [ + 'django.contrib.auth.middleware.SessionAuthenticationMiddleware' + ], PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',), ) class ChangelistTests(AuthViewsTestCase): diff --git a/django/contrib/auth/views.py b/django/contrib/auth/views.py index bcb4b6a733..969ab810d8 100644 --- a/django/contrib/auth/views.py +++ b/django/contrib/auth/views.py @@ -11,7 +11,8 @@ from django.views.decorators.cache import never_cache from django.views.decorators.csrf import csrf_protect # Avoid shadowing the login() and logout() views below. -from django.contrib.auth import REDIRECT_FIELD_NAME, login as auth_login, logout as auth_logout, get_user_model +from django.contrib.auth import (REDIRECT_FIELD_NAME, login as auth_login, + logout as auth_logout, get_user_model, update_session_auth_hash) from django.contrib.auth.decorators import login_required from django.contrib.auth.forms import AuthenticationForm, PasswordResetForm, SetPasswordForm, PasswordChangeForm from django.contrib.auth.tokens import default_token_generator @@ -264,6 +265,11 @@ def password_change(request, form = password_change_form(user=request.user, data=request.POST) if form.is_valid(): form.save() + # Updating the password logs out all other sessions for the user + # except the current one if + # django.contrib.auth.middleware.SessionAuthenticationMiddleware + # is enabled. + update_session_auth_hash(request, form.user) return HttpResponseRedirect(post_change_redirect) else: form = password_change_form(user=request.user) diff --git a/docs/ref/middleware.txt b/docs/ref/middleware.txt index 0898ca516a..f7a6768d3c 100644 --- a/docs/ref/middleware.txt +++ b/docs/ref/middleware.txt @@ -204,6 +204,15 @@ Adds the ``user`` attribute, representing the currently-logged-in user, to every incoming ``HttpRequest`` object. See :ref:`Authentication in Web requests `. +.. class:: SessionAuthenticationMiddleware + +.. versionadded:: 1.7 + +Allows a user's sessions to be invalidated when their password changes. See +:ref:`session-invalidation-on-password-change` for details. This middleware must +appear after :class:`django.contrib.auth.middleware.AuthenticationMiddleware` +in :setting:`MIDDLEWARE_CLASSES`. + CSRF protection middleware -------------------------- diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 8bcb9e8693..90208e4c8b 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -331,6 +331,15 @@ Minor features ``html_email_template_name`` parameter used to send a multipart HTML email for password resets. +* The :meth:`AbstractBaseUser.get_session_auth_hash() + ` + method was added and if your :setting:`AUTH_USER_MODEL` inherits from + :class:`~django.contrib.auth.models.AbstractBaseUser`, changing a user's + password now invalidates old sessions if the + :class:`~django.contrib.auth.middleware.SessionAuthenticationMiddleware` is + enabled. See :ref:`session-invalidation-on-password-change` for more details + including upgrade considerations when enabling this new middleware. + :mod:`django.contrib.formtools` ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/docs/topics/auth/customizing.txt b/docs/topics/auth/customizing.txt index cc8b944ada..388ebdd1ac 100644 --- a/docs/topics/auth/customizing.txt +++ b/docs/topics/auth/customizing.txt @@ -591,6 +591,13 @@ The following methods are available on any subclass of :meth:`~django.contrib.auth.models.AbstractBaseUser.set_unusable_password()` has been called for this user. + .. method:: models.AbstractBaseUser.get_session_auth_hash() + + .. versionadded:: 1.7 + + Returns an HMAC of the password field. Used for + :ref:`session-invalidation-on-password-change`. + You should also define a custom manager for your ``User`` model. If your ``User`` model defines ``username``, ``email``, ``is_staff``, ``is_active``, ``is_superuser``, ``last_login``, and ``date_joined`` fields the same as diff --git a/docs/topics/auth/default.txt b/docs/topics/auth/default.txt index 58633c556d..ec94400a7e 100644 --- a/docs/topics/auth/default.txt +++ b/docs/topics/auth/default.txt @@ -111,6 +111,12 @@ Django also provides :ref:`views ` and :ref:`forms ` that may be used to allow users to change their own passwords. +.. versionadded:: 1.7 + +Changing a user's password will log out all their sessions if the +:class:`~django.contrib.auth.middleware.SessionAuthenticationMiddleware` is +enabled. See :ref:`session-invalidation-on-password-change` for details. + Authenticating Users -------------------- @@ -575,6 +581,71 @@ To apply a permission to a :doc:`class-based generic view :ref:`decorating-class-based-views` for details. Another approach is to :ref:`write a mixin that wraps as_view() `. +.. _session-invalidation-on-password-change: + +Session invalidation on password change +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +.. versionadded:: 1.7 + +.. warning:: + + This protection only applies if + :class:`~django.contrib.auth.middleware.SessionAuthenticationMiddleware` + is enabled in :setting:`MIDDLEWARE_CLASSES`. It's included if + ``settings.py`` was generated by :djadmin:`startproject` on Django ≥ 1.7. + +If your :setting:`AUTH_USER_MODEL` inherits from +:class:`~django.contrib.auth.models.AbstractBaseUser` or implements its own +:meth:`~django.contrib.auth.models.AbstractBaseUser.get_session_auth_hash()` +method, authenticated sessions will include the hash returned by this function. +In the :class:`~django.contrib.auth.models.AbstractBaseUser` case, this is an +HMAC of the password field. If the +:class:`~django.contrib.auth.middleware.SessionAuthenticationMiddleware` is +enabled, Django verifies that the hash sent along with each request matches +the one that's computed server-side. This allows a user to log out all of their +sessions by changing their password. + +The default password change views included with Django, +:func:`django.contrib.auth.views.password_change` and the +``user_change_password`` view in the :mod:`django.contrib.auth` admin, update +the session with the new password hash so that a user changing their own +password won't log themselves out. If you have a custom password change view +and wish to have similar behavior, use this function: + +.. function:: update_session_auth_hash(request, user) + + This function takes the current request and the updated user object from + which the new session hash will be derived and updates the session hash + appropriately. Example usage:: + + from django.contrib.auth import update_session_auth_hash + + def password_change(request): + if request.method == 'POST': + form = PasswordChangeForm(user=request.user, data=request.POST) + if form.is_valid(): + form.save() + update_session_auth_hash(request, form.user) + else: + ... + +If you are upgrading an existing site and wish to enable this middleware without +requiring all your users to re-login afterward, you should first upgrade to +Django 1.7 and run it for a while so that as sessions are naturally recreated +as users login, they include the session hash as described above. Once you +start running your site with +:class:`~django.contrib.auth.middleware.SessionAuthenticationMiddleware`, any +users who have not logged in and had their session updated with the verification +hash will have their existing session invalidated and be required to login. + +.. note:: + + Since + :meth:`~django.contrib.auth.models.AbstractBaseUser.get_session_auth_hash()` + is based on :setting:`SECRET_KEY`, updating your site to use a new secret + will invalidate all existing sessions. + .. _built-in-auth-views: Authentication Views