aaugustin / django-sesame

"Magic Links" - URLs with authentication tokens for one-click login
https://django-sesame.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
983 stars 57 forks source link

Drop token from URL after login #9

Closed jacebrowning closed 5 years ago

jacebrowning commented 6 years ago

It might be nice if there was an option to redirect to drop the token from the URL after successful authentication. Would this be possible?

aaugustin commented 6 years ago

It's technically possible to add this feature to the middleware.

I'm wondering if it needs to be configurable or if we should just make a choice here.

That choice is a tradeoff between page load time (an extra redirect) and URL cleanliness.

Currently django-sesame supports running without django.contrib.sessions (theoretically — I never tested it and I don't expect anyone to do that). We shouldn't redirect in that case.

jacebrowning commented 6 years ago

I can't imagine many scenarios where I'd want to keep the token in the URL.

I'm building an app (nothing sensitive and well within the security caveats) that will generate unique event URLs to be shared via text/chat/etc. and take advantage of the password-less login provided by this package. If the token remains in the URL, it increases the chances that someone will share a URL that inadvertently logs in as that user.

aaugustin commented 6 years ago

Agreed, that's why I'm thinking it could be the default behavior (with no option to change it).

That would require modifying this block: https://github.com/aaugustin/django-sesame/blob/b546c024796fc7ea5a9c41e26cfa358eb35f31c6/sesame/middleware.py#L32-L35

to add something like:

    # deconstruct URL
    # remove url_auth_token parameter
    # reconstruct URL
    # redirect
jacebrowning commented 6 years ago

👍 That flow is very similar to what I've implemented in another app to achieve this effect.

aaugustin commented 6 years ago

PR welcome :-)

NB - you'll need to tweak __call__ as well.

jacebrowning commented 6 years ago

Should the token be removed from the URL if authentication fails?

aaugustin commented 6 years ago

Good question! Probably not?

jacebrowning commented 6 years ago

I think I agree. The PR I submitted keeps the token on failed auth.

aaugustin commented 6 years ago

I pushed version 1.4 to PyPI with this feature.

paterlinimatias commented 5 years ago

Unfortunately this is not working for safari, as it is against setting cookies in 302 redirects.

When someone wraps the url with the token (for example to make a bit.ly short url, or an automated url defense software ) it fails in safari (both ios and desktop.)

paterlinimatias commented 5 years ago

is there any chance for you to make this optional ?

aaugustin commented 5 years ago

This sounds like a side-effect of the over-zealous "Protection Against First Party Bounce Trackers" in ITP 2.0.

Do all redirects happen on the same domain? Or do you change first-party domain e.g. from https://example.com/ to https://www.example.com/?

paterlinimatias commented 5 years ago

The domain was different, which Is probably the case when you do url shortening, same goes for url defenders.

Unfortunately this failed for us in the middle of a huge demo when the guys tried using it with their corporate email (which uses proof point.com)

aaugustin commented 5 years ago

To be honest I'd rather implement something that works well for most users rather expect them to learn the intricacies of ITP and determine that they may need to tweak a setting. ` Perhaps we could redirect only in circumstances that are deemed "ITP-safe", for example when there is no referrer from a different domain — there should be one from proofpoint.com in your example.

paterlinimatias commented 5 years ago

I think that's fair enough.

aaugustin commented 5 years ago

For the record, there've been several discussions of ITP on the django-developers mailing-list. Apparently there are many use cases where ITP is over-zealous and blocks perfectly legitimate behavior.

aaugustin commented 5 years ago

Probably we should just sniff the user agent disable the redirection on Safari... It isn't critical to django-sesame's functionality.

aaugustin commented 5 years ago

That would have been a good plan if it had been possible to detect Safari reliably with its user agent...

aaugustin commented 5 years ago

Does #22 work for you? Don't forget to pip install ua-parser, else it won't do anything.

I didn't make ua-parser a mandatory dependency because I'm not comfortable adding 0.2-0.5s to the boot time of every app that uses django-sesame just because Safari is annoying.