~petersanchez/public-inbox

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
1

[PATCH django-impersonate] add option for auto-expiring impersonate sesions, implements #45

Leonhard Kuboschek
Details
Message ID
<5ae6838bdebd6fa362f7.1606156358@red>
DKIM signature
missing
Download raw message
Patch: +75 -1
# 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

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):
                    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()
        prev_path = request.META.get('HTTP_REFERER')
        if prev_path:
            request.session[
Details
Message ID
<20201124012648.dagh3gncwpsnhhvv@thinkpad>
In-Reply-To
<5ae6838bdebd6fa362f7.1606156358@red> (view parent)
DKIM signature
missing
Download raw message
On 11/23, Leonhard Kuboschek wrote:
>+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.

>+                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?

>     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()`
Reply to thread Export thread (mbox)