devilry / trix2

Next generation Trix. Detailed task control and statistics app for better learning outcome.
BSD 3-Clause "New" or "Revised" License
2 stars 3 forks source link

Login should return to the page it was invoked from #126

Closed torgeirl closed 7 months ago

torgeirl commented 1 year ago

If you log in, you are redirected to the front page upon your return from Dataporten. If possible, we should instead redirect to the page where the user clicked login on.

Edit: I thought we didn't get to adding this in v3.0.0, but the login link actually has a next link ie.: /authenticate/login/?next=/course/11 (when current page is /course/11). It seems to be discarded by Dataporten, but it might be fixable by adjusting its configuration.

torgeirl commented 1 year ago

As far as I can tell the next parameter is discarded by Allauth itself, never reaching our login provider. I believe it is caused by a known issue with Allauth, where it checks whether the URL is among allowed hosts but never consults the allowed hosts in Django settings - therefore discarding the return URL as unsafe.

The problem is addressed by https://github.com/pennersr/django-allauth/pull/3264 but unclear if and when the fix might be included in Allauth.

When that's solved we might also have to use full URLs instead of URIs for the parameter.

torgeirl commented 1 year ago

The problem is addressed by pennersr/django-allauth#3264 but unclear if and when the fix might be included in Allauth.

Starting with v0.55, Allauth's is_safe_url should finally respect Django's allowed_hosts (fixed with pennersr/django-allauth#3329).

torgeirl commented 11 months ago

Starting with v0.55, Allauth's is_safe_url should finally respect Django's allowed_hosts (fixed with pennersr/django-allauth#3329).

v0.55 was released on August 22, but due to an incompatibility it doesn't work with Trix (#138).

torgeirl commented 7 months ago

Without LOGIN_REDIRECT_URL (#141) users are redirected to /accounts/profile/, which is default in Django 4.2:

LOGIN_REDIRECT_URL Default: '/accounts/profile/' The URL or named URL pattern where requests are redirected after login when the LoginView doesn’t get a next GET parameter.

This suggests the next parameter still gets discarded, or further setting adjustments are needed.

Levijatan commented 7 months ago

So found out that it was how redirecting to the correct login view was handled. The TrixLogin view redirected the user to the AllauthLoginView when 'DATAPORTEN_LOGIN' was set to True and when redirecting not passing the ?next= url to AllauthLoginView. So I added TrixLoginRedirectView to handle the redirect and made sure it propagates ?next= correctly. 97f6dd4.

I think this should also "fix" #141, but the fallbacks should still be set since even if all instances of the login button always sends a ?next= to current view, user could stil remove the ?next= or go to the login view url manually.

torgeirl commented 7 months ago

I still get a HTTP 404 at accounts/profile/, but it looks like the ?next= parameter is dropped one step later than before.

v4.0.0b1 trix-4 0 0b1

vs

v4.0.0b2 trix-4 0 0b2

torgeirl commented 7 months ago

Solved in v4.0.0b4! :tada: