Fixed #21649 -- Added invalidation of old sessions when user password changes #2494

Closed
wants to merge 8 commits into from

2 participants

@timgraham
Django member

No description provided.

@aaugustin aaugustin and 1 other commented on an outdated diff Mar 30, 2014
django/contrib/auth/__init__.py
if SESSION_KEY in request.session:
- if request.session[SESSION_KEY] != user.pk:
+ if request.session[SESSION_KEY] != user.pk or (
+ # TODO: add a test for this
@aaugustin
Django member

Still TODO?

@timgraham
Django member

Done now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin and 2 others commented on an outdated diff Mar 30, 2014
django/contrib/auth/middleware.py
+class SessionVerificationMiddleware(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.SESSION_HASH_KEY)
+ session_hash_verified = session_hash and constant_time_compare(
+ session_hash,
+ user.get_session_auth_hash()
+ )
+ if not session_hash_verified:
+ request.user = auth.models.AnonymousUser()
@aaugustin
Django member

I'm not sure about this line. Shouldn't we call auth.logout?

@timgraham
Django member

Possibly... this is the approach used by django.contrib.auth.get_user() where this logic was in the original PR.

@PaulMcMillan could you review?

I agree with @aaugustin, it seems like this should be auth.logout. We should avoid code that incidentally logs a user out without calling auth.logout, as is done here.

@timgraham
Django member

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin and 1 other commented on an outdated diff Mar 30, 2014
django/contrib/auth/views.py
@@ -264,6 +265,12 @@ 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.SessionVerificationMiddleware
+ # is enabled.
+ if hasattr(form.user, 'get_session_auth_hash') and request.user == form.user:
+ request.session[SESSION_HASH_KEY] = form.user.get_session_auth_hash()
@aaugustin
Django member

I understand that this cannot be handled in form.save() because the form doesn't know about the request.

However, it would be nice to factor it into a public function for people defining their own password change forms.

@timgraham
Django member

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin and 1 other commented on an outdated diff Mar 30, 2014
docs/topics/auth/default.txt
@@ -575,6 +581,57 @@ 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() <mixins_that_wrap_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.SessionVerificationMiddleware`
+ is enabled in :setting:`MIDDLEWARE_CLASSES`.
@aaugustin
Django member

Can we add it to the default project template?

@timgraham
Django member

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin and 1 other commented on an outdated diff Apr 3, 2014
django/contrib/auth/__init__.py
@@ -158,4 +165,17 @@ def get_permission_codename(action, opts):
return '%s_%s' % (action, opts.model_name)
+def update_session_auth_token(request, user):
@aaugustin
Django member

Why is this called "token" and not "hash"?

@timgraham
Django member

I got lazy and used "hash" and "token" somewhat interchangeably -- replaced all usage of "token" with "hash", thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin and 1 other commented on an outdated diff Apr 3, 2014
docs/topics/auth/default.txt
@@ -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() <mixins_that_wrap_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`` is generated by :djadmin:`startproject`.
@aaugustin
Django member

"was generated by :djadmin:startproject on Django ≥ 1.7"?

@timgraham
Django member

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin
Django member

I would s/update_session_auth_token/update_session_auth_hash/. Otherwise this is RFC.

@erikr erikr and 1 other commented on an outdated diff Apr 4, 2014
django/contrib/auth/__init__.py
@@ -12,6 +12,7 @@
SESSION_KEY = '_auth_user_id'
BACKEND_SESSION_KEY = '_auth_user_backend'
+SESSION_HASH_KEY = '_auth_user_hash'
@erikr
Django member
erikr added a note Apr 4, 2014

This seems inconsistent with line 14: perhaps rename it to HASH_SESSION_KEY ?

@timgraham
Django member

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@erikr erikr and 1 other commented on an outdated diff Apr 4, 2014
django/contrib/auth/__init__.py
if SESSION_KEY in request.session:
- if request.session[SESSION_KEY] != user.pk:
+ session_key = request.session[SESSION_KEY]
+ if session_key != user.pk or (
+ session_auth_hash and session_key != session_auth_hash):
@erikr
Django member
erikr added a note Apr 4, 2014

Shouldn't that second test be against request.session[SESSION_HASH_KEY]? Because now, the value must be equal to both the users pk and their session auth hash, or the session is flushed.

@timgraham
Django member

Yes, I had that at first and then refactored it incorrectly. Fixed and test added, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Django member

merged in fd23c06.

@timgraham timgraham closed this Apr 5, 2014
@timgraham timgraham deleted the timgraham:21649a branch Apr 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment