Opened 5 years ago

Last modified 11 months ago

#12233 assigned New feature

Redirect logged in user

Reported by: dmathieu Owned by: aaronbassett
Component: contrib.auth Version:
Severity: Normal Keywords: authentication, login
Cc: loginzdupy@…, erik@…, buchanae, torsten@…, olethanh Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In django.contrib.auth.views.login, if a user is already logged in, he gets the log in form anyway when it'd seem logical to be able to redirect him to an other page.
The following patch adds that feature.

We can use the view in the urls.py like the following :

(r'^login$', 'django.contrib.auth.views.login', {'redirect_if_logged_in': '/'})

Then if the user goes to the login page and isn't yet logged in, he'll see the log in form.
If he goes there and is already logged in, he'll be redirected to /.

Attachments (1)

patch.diff (3.0 KB) - added by dmathieu 5 years ago.
Patch

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by dmathieu

Patch

comment:1 Changed 5 years ago by nieproszenieja

I agree with dmathieu - authenticated user shouldn't be able to see the login form. But in opposite to the given patch, I think that redirection should be mandatory (and as simple as possible, while this is the default authentication system) - in default case, user should be redirected to LOGIN_REDIRECT_URL (or if not set, to accounts/profile).

===============================
Implementation:

if request.user.is_authenticated():

return HttpResponseRedirect(LOGIN_REDIRECT_URL)

+ in the documentation one simple information - If authenticated user tries to access to the login view, it will be redirected to LOGIN_REDIRECT_URL.

I think it will be enough for a default authentication system.
==================================

P.S. What is the status of the patch? Will it be included in next releases?

comment:2 Changed 5 years ago by nieproszenieja

  • Cc nieproszenieja@… added

comment:3 Changed 5 years ago by anonymous

  • Cc loginzdupy@… added; nieproszenieja@… removed

comment:4 Changed 5 years ago by anonymous

  • Cc erik@… added

comment:5 Changed 4 years ago by SmileyChris

  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted
  • Version 1.1 deleted

New arguments should be added to the end. Needs docs.

comment:6 Changed 4 years ago by SmileyChris

  • Triage Stage changed from Accepted to Design decision needed

Sorry, let me to clarify in case that wasn't clear:

To help maintain backwards compatibility in case people are using positional arguments when calling the login function, new arguments should be added to the end.

And I'm shuffling this to design decision (how it got to ready for checkin isn't actually in the ticket history :-/ )

comment:7 Changed 4 years ago by mattmcc

  • Severity set to Normal
  • Type set to New feature

comment:8 Changed 4 years ago by SmileyChris

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted

Small feature, but is generic enough to be useful.

comment:9 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 3 years ago by lgp171188@…

Bump. If it is still not in, would love to see it in asap.

comment:11 Changed 3 years ago by kmtracey

Before this can go in, someone interested in the feature needs to correct the current patch to fix the problems SmileyChris noted above. The current patch adds an arg in the middle of existing args, this is backwards-incompatible. We also can't make changes to the arguments to a documented function (https://docs.djangoproject.com/en/1.4/topics/auth/#django.contrib.auth.views.login) without updating the docs. So, if you'd like to "bump" this ticket along, the way to do that would be to provide an updated patch (or pull request) that fixes the problems with the existing one.

comment:12 Changed 2 years ago by aaronbassett

  • Owner changed from nobody to aaronbassett
  • Status changed from new to assigned

comment:13 Changed 17 months ago by buchanae

I've done some work on this here: https://github.com/abuchanan/django/tree/ticket_12233
Still needs work.

I changed the argument name to "redirect_authenticated_user" and the value type to Boolean (whether authenticated users should be redirected) which defaults to False because I figured that would be more backwards-compatible.

The redirect URL would use the same mechanism as normal user login.

I'm not sure yet how to handle a case where the redirect url is the login page (creating an infinite redirect loop). I guess I could chalk that up to user error and just let it happen.

I've also considered that this may be cleaner as a view decorator. Thoughts on that?
Something to the effect of:

@redirect_authenticated_user(redirect_field_name=REDIRECT_FIELD_NAME, redirect_url=settings.LOGIN_REDIRECT_URL)
def your_view(request): pass

comment:14 Changed 17 months ago by buchanae

  • Cc buchanae added

comment:15 Changed 15 months ago by scel

  • Cc torsten@… added

comment:16 Changed 11 months ago by olethanh

I have improved the branch a little bit and rebased it on top of master in https://github.com/olethanh/django/tree/ticket_12233
I added a simple loop detection which will raise an Exception but it won't catch all cases and improved the docs

Not sure what's the best way to handle this redirection problem either:

  • We could just do nothing and let the user browser automatically detect it but then the admin won't know that something is wrong.
  • We cannot detect this at start time since there is no settings for the LOGIN_URL and a project can potentially have multiple login url.
  • We can raise an Exception if we detect it (what is currently done in my branch)
  • We can return a 500 if we detect it (what's was previously done in my branch)
  • But more importantly we don't really have a reliable way to detect it, my current patch won't catch redirection if they have GET parameters inside or if we redirect to another login url or if there is another redirection afterwards.

Not sure how important this problem is since it is a corner case that will only happen if the settings are badly configured.

comment:17 Changed 11 months ago by olethanh

  • Cc olethanh added

comment:18 Changed 11 months ago by erikr

Raising an exception makes sense to me if we can't detect it earlier. The exception will probably produce a 500 to the user, and a useful error message to the end user. There should be a warning about this issue in the docs though.

Could you make a proper pull request? That makes it much easier to review and keep track of comments. Add a reference to this ticket in the PR, and post the link to the PR here.

Note: See TracTickets for help on using tickets.
Back to Top