Opened 14 years ago

Closed 8 years ago

#12233 closed New feature (fixed)

Allow the login view to redirect an already logged in user

Reported by: dmathieu Owned by: aaronbassett
Component: contrib.auth Version:
Severity: Normal Keywords: authentication, login
Cc: loginzdupy@…, erik@…, buchanae, torsten@…, Olivier Le Thanh Duong Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 14 years ago.
Patch

Download all attachments as: .zip

Change History (24)

by dmathieu, 14 years ago

Attachment: patch.diff added

Patch

comment:1 by nieproszenieja, 14 years ago

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 by nieproszenieja, 14 years ago

Cc: nieproszenieja@… added

comment:3 by anonymous, 14 years ago

Cc: loginzdupy@… added; nieproszenieja@… removed

comment:4 by anonymous, 14 years ago

Cc: erik@… added

comment:5 by Chris Beaven, 14 years ago

Needs documentation: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted
Version: 1.1

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

comment:6 by Chris Beaven, 14 years ago

Triage Stage: AcceptedDesign 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 by Matt McClanahan, 13 years ago

Severity: Normal
Type: New feature

comment:8 by Chris Beaven, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted

Small feature, but is generic enough to be useful.

comment:9 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:10 by lgp171188@…, 12 years ago

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

comment:11 by Karen Tracey, 12 years ago

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 by aaronbassett, 11 years ago

Owner: changed from nobody to aaronbassett
Status: newassigned

comment:13 by buchanae, 10 years ago

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 by buchanae, 10 years ago

Cc: buchanae added

comment:15 by Torsten Rehn, 10 years ago

Cc: torsten@… added

comment:16 by Olivier Le Thanh Duong, 10 years ago

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 by Olivier Le Thanh Duong, 10 years ago

Cc: Olivier Le Thanh Duong added

comment:18 by Sasha Romijn, 10 years ago

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.

comment:19 by Olivier Le Thanh Duong, 8 years ago

I've updated my branch, rebased it on master, improved the documentation and the tests.

Opened a PR here : https://github.com/django/django/pull/5604

comment:20 by Carl Meyer, 8 years ago

Needs documentation: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:21 by Tim Graham, 8 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Left a few more comments for improvement. Please bump back to RFC when updated.

comment:22 by Tim Graham, 8 years ago

Patch needs improvement: unset
Summary: Redirect logged in userAllow the login view to redirect an already logged in user
Triage Stage: AcceptedReady for checkin

comment:23 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 10781b4:

Fixed #12233 -- Allowed redirecting authenticated users away from the login view.

contrib.auth.views.login() has a new parameter redirect_authenticated_user
to automatically redirect authenticated users visiting the login page.

Thanks to dmathieu and Alex Buchanan for the original code and to Carl Meyer
for the help and review.

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