Leonhard Kuboschek: 1 add option for auto-expiring impersonate sesions, implements #45 6 files changed, 75 insertions(+), 1 deletions(-)
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.code.netlandish.com/~petersanchez/public-inbox/patches/5/mbox | git am -3Learn more about email & git
# HG changeset patch # User Leonhard Kuboschek <leo@jacobs-alumni.de> # Date 1606156336 -3600 # Mon Nov 23 19:32:16 2020 +0100 # Node ID 5ae6838bdebd6fa362f7fd730476a045c6f2772d # Parent 1e3192bbb6762d1756fd203d28714620ef43da15 add option for auto-expiring impersonate sesions, implements #45 diff --git a/README.md b/README.md --- a/README.md +++ b/README.md @@ -395,6 +395,13 @@ Default is `True` + MAX_DURATION + +A number specifying the maximum allowed duration of impersonation +sessions in **seconds**. + +Default is `None` + Admin ===== diff --git a/README.rst b/README.rst --- a/README.rst +++ b/README.rst @@ -464,6 +464,15 @@ Default is ``True`` +:: + + MAX_DURATION + +A number specifying the maximum allowed duration of impersonation +sessions in **seconds**. + +Default is `None` + Admin ===== diff --git a/impersonate/middleware.py b/impersonate/middleware.py --- a/impersonate/middleware.py +++ b/impersonate/middleware.py @@ -1,4 +1,6 @@ # -*- coding: utf-8 -*- +from datetime import datetime, timedelta
I wonder if it's not better to use the `django.utils.timezone` module here? Also use UTC aware timestamps.
+ from django.http import HttpResponseNotAllowed from django.utils.deprecation import MiddlewareMixin @@ -12,6 +14,17 @@ request.impersonator = None if request.user.is_authenticated and '_impersonate' in request.session: + if settings.MAX_DURATION: + if '_impersonate_start' not in request.session: + return + + start_time = datetime.fromtimestamp(request.session['_impersonate_start']) + if datetime.now() - start_time > timedelta(seconds=settings.MAX_DURATION):
Use `datetime.utcfromtimestamp()` and `timezone.now()` which returns a UTC aware copy of datetime.now(). We'd have to also use `timezone.make_aware()` on the `start_time` variable when subtracting. `if timezone.now() - timezone.make_aware(start_time) > ...` I could be overthinking it but I'm imagining a crazy bug where an app is deployed across multiple time zones, and systems are not setup to use UTC, and then we end up invalidating sessions prematurely. What do you think?
+ del request.session['_impersonate'] + del request.session['_impersonate_start'] + + return + new_user_id = request.session['_impersonate'] if isinstance(new_user_id, User): # Edge case for issue 15 diff --git a/impersonate/settings.py b/impersonate/settings.py --- a/impersonate/settings.py +++ b/impersonate/settings.py @@ -23,6 +23,7 @@ 'ADMIN_DELETE_PERMISSION': False, 'ADMIN_ADD_PERMISSION': False, 'ADMIN_READ_ONLY': True, + 'MAX_DURATION': None, } diff --git a/impersonate/tests.py b/impersonate/tests.py --- a/impersonate/tests.py +++ b/impersonate/tests.py @@ -22,6 +22,7 @@ from distutils.version import LooseVersion from unittest.mock import PropertyMock, patch from urllib.parse import urlencode, urlsplit +from datetime import datetime, timedelta import django from django.urls import include, path @@ -110,11 +111,12 @@ return None self.middleware = ImpersonateMiddleware(dummy_get_response) - def _impersonated_request(self, use_id=True): + def _impersonated_request(self, use_id=True, _impersonate_start=None): request = self.factory.get('/') request.user = self.superuser request.session = { '_impersonate': self.user.pk if use_id else self.user, + '_impersonate_start': _impersonate_start } self.middleware.process_request(request) @@ -133,6 +135,45 @@ ''' self._impersonated_request(use_id=False) + @override_settings(IMPERSONATE={'MAX_DURATION': 3600}) + def test_impersonated_request_with_max_duration(self): + self._impersonated_request(_impersonate_start=datetime.now().timestamp()) + + @override_settings(IMPERSONATE={'MAX_DURATION': 3600}) + def test_reject_without_start_time(self): + ''' Test to ensure that requests without a start time + are rejected when MAX_DURATION is set + ''' + request = self.factory.get('/') + request.user = self.superuser + request.session = { + '_impersonate': self.user.pk, + } + self.middleware.process_request(request) + + self.assertEqual(request.user, self.superuser) + self.assertFalse(request.user.is_impersonate) + + + @override_settings(IMPERSONATE={'MAX_DURATION': 3600}) + def test_reject_expired_impersonation(self): + ''' Test to ensure that requests with a start time before (now - MAX_DURATION) + are rejected + ''' + request = self.factory.get('/') + request.user = self.superuser + request.session = { + '_impersonate': self.user.pk, + '_impersonate_start': (datetime.now() - timedelta(seconds=3601)).timestamp() + } + self.middleware.process_request(request) + + self.assertEqual(request.user, self.superuser) + self.assertFalse(request.user.is_impersonate) + self.assertNotIn('_impersonate', request.session) + self.assertNotIn('_impersonate_start', request.session) + + def test_not_impersonated_request(self, use_id=True): """Check the real_user request attr is set correctly when **not** impersonating.""" request = self.factory.get('/') diff --git a/impersonate/views.py b/impersonate/views.py --- a/impersonate/views.py +++ b/impersonate/views.py @@ -1,6 +1,8 @@ # -*- coding: utf-8 -*- import logging +from datetime import datetime + from django.db.models import Q from django.http import Http404 from django.shortcuts import get_object_or_404, redirect, render @@ -36,6 +38,7 @@ raise Http404('Invalid value given.') if check_allow_for_user(request, new_user): request.session['_impersonate'] = new_user.pk + request.session['_impersonate_start'] = datetime.now().timestamp()
If we make the change, this obviously would also have to move to `timezone.now()`
prev_path = request.META.get('HTTP_REFERER') if prev_path: request.session[