SESAME_ONE_TIME failing about django-sesame HOT 16 CLOSED

I pushed version 1.7 which includes this fix.

from django-sesame.

Just to confirm: I've set SESAME_ONE_TIME as follows in settings.py:

SESAME_ONE_TIME = True

i.e. at the top level and not in any dictionary or list. That's the correct syntax, isn't it?

from django-sesame.

Calling django.contrib.auth.login(request, user) invalidates one-time tokens.

Calling django.contrib.auth.authenticate(url_auth_token=token) doesn't.

On one hand, given the current documentation, it's quite reasonable to expect that a successful authenticate will invalidate a one-time token.

On the other hand, the current behavior is a straightforward consequence of how django.contrib.auth works and can be extended.

I think the best way forwards is to keep the current behavior and improve the documentation.

from django-sesame.

Thanks for the quick response, @aaugustin

I tested with authenticate, because I thought that's what's used under the hood. I'm not actually calling any methods explicitly. Rather, I was testing using the query string twice in the following test and accompanying implementation:

    def test_reuse(self):
        """Test token re-user"""
        alice = User.objects.create_user(username='[email protected]', password='foobar123')
        qs = get_query_string(alice)

        res = self.client.get('/api/users/me/' + qs, follow=True)
        assert res.status_code == 200
        assert res.data['id'] == alice.id

        res = self.client.get('/api/users/me/' + qs, follow=True)
        assert res.status_code == 403  # <-- fails, getting 200

class UserViewSet(viewsets.ModelViewSet):
    queryset = User.objects.all().order_by('-date_joined')
    serializer_class = UserSerializer

    @action(detail=False)
    def me(self, request):
        user = get_user(request)
        if user is None:
            raise PermissionDenied
        serializer = self.get_serializer(user)
        return Response(serializer.data)

This is the variant without the middleware in between but I'm getting the same results if I let the middleware process the query string and simply use request.user.

Should the middleware using and unwrapping the token - hence in my mind "logging the user in" - not invalidate the token?

Or do I need to explicitly call login on all endpoints in order to update last_login? That seems error prone. If not now, I'm surely going to forget instrumenting one later on.🤔

from django-sesame.

PS: I've tried self.client.session.clear() in between requests

from django-sesame.

The intended use case for SESAME_ONE_TIME is password reset links. They should expire when they've been used.

I'm not sure why you're using SESAME_ONE_TIME but it looks like something else. If I understood your use case, probably I'd be in a better position to help.

from django-sesame.

What do you think about #32?

from django-sesame.

Thanks a lot for the follow-up, @aaugustin .

I think our use case is quite similar to a password reset link. We're sending out links to documents and want to be able to sign the user in with a one-use link.

Maybe the following test that goes through the api layer explains this better:

    def test_reuse_2(self):
        alice = User.objects.create_user(username='[email protected]', password='foobar123')
        qs = get_query_string(alice)

        res = self.client.get('/api/users/me/' + qs, follow=True)
        assert res.status_code == 200
        assert res.data['id'] == alice.id

        res = self.client.get('/api/users/me/' + qs, follow=True)
        assert res.status_code == 403  # FIXME: failing, 200 instead

I'm using an api endpoint that's equivalent to /api/users/{id} here to get back user info for the authenticated user.

I imagine this is quite similar to what a password reset link should do, isn't it?

I can obviously go into the endpoint implementation and ensure that last_login is updated but we're going to have various endpoints handling those links and it's easy to miss one.

Mainly, I wanted to mention that I thought some underlying process that maps the token to the user object ensures the token is invalidated at the same time.

For reference, this is the endpoint:

class UserViewSet(viewsets.ModelViewSet):
    queryset = User.objects.all().order_by('-date_joined')
    serializer_class = UserSerializer

    @action(detail=False)
    def me(self, request):
        user = request.user
        if user is None or not user.is_authenticated:
            raise PermissionDenied
        serializer = self.get_serializer(request.user)
        return Response(serializer.data)

My understanding is that the sesame middleware populates request.user in the above snippet. At that point I was expecting the token to be invalidated (i.e. last_login to be set) without having to remember to do so myself.

This may be a totally wrong assumption - this is simply me asking if it's an oversight or intended behaviour :)

from django-sesame.

As to #32 I think you're saying it is indeed intended behaviour and up to us to ensure we invalidate the tokens by setting last_login or calling login ourselves in endpoints that handle token :)

from django-sesame.

At that point I was expecting the token to be invalidated (i.e. last_login to be set) without having to remember to do so myself.

Yes, I'm expecting this as well. I don't understand the behavior you're seeing.

Since I don't have access to your setup to debug this, could you confirm if code execution goes through the following lines?

• django-sesame/sesame/middleware.py

  Line 33 in b3a9435

  login(request, user)

• https://github.com/django/django/blob/4b6dfe16226a81fea464ac5f77942f4d6ba266e8/django/contrib/auth/__init__.py#L131
• https://github.com/django/django/blob/master/django/contrib/auth/models.py#L20

from django-sesame.

I haven't had a chance yet to step through all three places yet but here's what happens in the middleware for this test:

    def test_reuse_2(self):
        """Test token re-user"""
        alice = User.objects.create_user(username='[email protected]', password='foobar123')
        qs = get_query_string(alice)

        breakpoint()
        res = self.client.get('/api/users/me/' + qs, follow=True)
        assert res.status_code == 200
        assert res.data['id'] == alice.id

        res = self.client.get('/api/users/me/' + qs, follow=True)
        assert res.status_code == 403  # FIXME: failing, 200 instead

The first line is line 35 in a larger test file and I set a breakpoint here:

    def process_request(self, request):
        """
        Log user in if `request` contains a valid login token.

        Return a HTTP redirect response that removes the token from the URL
        after a successful login when sessions are enabled, else ``None``.

        """
        user = get_user(request)

        # If the sessions framework is enabled and the token is valid,
        # persist the login in session.
        breakpoint()
        if hasattr(request, "session") and user is not None:
            login(request, user)

Here's a gist with the debug session output: https://gist.github.com/finestructure/8b3e757d528252c59a8047c43cf12a97

The first time process_request is reached, it goes in and does a login. The subsequent two times user is None.

I'm not sure why the breakpoint in process_request is hit three times. I guess I would have expected either twice or four times, depending on whether the redirect after handling the token comes before or after this gets called.

from django-sesame.

As to #32 I think you're saying it is indeed intended behaviour and up to us to ensure we invalidate the tokens by setting last_login or calling login ourselves in endpoints that handle token :)

I wanted to say the exact opposite :-(

The only situation where you need to do so is if you're using the low-level django.contrib.auth.authenticate() API. If you don't use django-sesame's helpers, there isn't much I can do to help.

from django-sesame.

I thought some underlying process that maps the token to the user object ensures the token is invalidated at the same time.

That process consists in updating the user's last_login date. However, since you don't want to make a database write with every HTTP request, this needs to be done carefully.

In a cookie-based authentication scenario, the sesame token is validated, then the user is logged-in (which updates last_login) and then authentication is preserved in cookies.

from django-sesame.

I wrote this test case to reproduce your scenario. I made it as similar as possible to the code you provided, minus DRF.

from __future__ import unicode_literals

from django.contrib.auth.models import User
from django.core.exceptions import PermissionDenied
from django.http import JsonResponse
from django.test import TestCase, override_settings
from django.urls import path
from sesame.utils import get_query_string, get_user

# Necessary override_settings(SESAME_ONE_TIME=True)
from sesame import test_signals


def me(request):
    user = get_user(request)
    if user is None:
        raise PermissionDenied
    user_data = vars(user)
    user_data.pop('_state')
    return JsonResponse(user.__dict__)


urlpatterns = [path('api/users/me/', me)]


@override_settings(ROOT_URLCONF=__name__, SESAME_ONE_TIME=True)
class TestIssue29(TestCase):

    def test_reuse_2(self):
        alice = User.objects.create_user(username='[email protected]', password='foobar123')
        qs = get_query_string(alice)

        res = self.client.get('/api/users/me/' + qs, follow=True)
        self.assertEqual(res.status_code, 200)
        self.assertEqual(res.json()['id'], alice.id)

        res = self.client.get('/api/users/me/' + qs, follow=True)
        self.assertEqual(res.status_code, 403)

It fails on master and passes on the branch for #32.

So, can we consider that #32 is the way to go, with better documentation (since my initial attempt confused you)?

from django-sesame.

That looks great, thanks a lot for following up!

from django-sesame.

Can confirm that after updating to 1.7 my tests are now all passing. Thanks again for the fix!

from django-sesame.