Code

Opened 7 years ago

Closed 7 years ago

#3185 closed enhancement (fixed)

Allow django.contrib.auth's LOGIN_URL, LOGOUT_URL and ACCOUNT_URL to be set in settings.py

Reported by: Vasily Sulatskov <redvasily@…> Owned by: adrian
Component: Contrib apps Version: master
Severity: normal Keywords:
Cc: Vasily, Sulatskov, <redvasily@…>, jonasvp@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Currently it's impossible to change LOGIN_URL varaible which is used in django.contrib.auth

I found this discussion on mailing list.

Adrian Holovaty said that LOGIN_URL should be made configurable. I made tiny patch to make it possible to specify LOGIN_URL in site settings.py module. This patch works at least for me and doesn't break anything else (as far as I know).

Attachments (9)

login_url.diff (597 bytes) - added by Vasily Sulatskov <redvasily@…> 7 years ago.
login_url2.diff (1.8 KB) - added by Vasily Sulatskov <redvasily@…> 7 years ago.
login_url3.diff (1.6 KB) - added by Gary Wilson <gary.wilson@…> 7 years ago.
using getattr instead of try/except
3185_docs.patch (2.8 KB) - added by Marc Fargas <telenieko@…> 7 years ago.
Documentation for this ticket.
3185.diff (4.3 KB) - added by Gary Wilson <gary.wilson@…> 7 years ago.
code and docs in one patch
3185_2.diff (4.3 KB) - added by Gary Wilson <gary.wilson@…> 7 years ago.
use relative link, as mentioned in #3333
3185_3.diff (6.6 KB) - added by Vasily Sulatskov <redvasily@…> 7 years ago.
Added LOGOUT_URL variable to auth/init.py. Modified comments app to use it.
3185_4.diff (6.7 KB) - added by Vasily Sulatskov <redvasily@…> 7 years ago.
Uber patch. Everything included
login_url.patch (10.4 KB) - added by Collin Grady <cgrady@…> 7 years ago.

Download all attachments as: .zip

Change History (31)

Changed 7 years ago by Vasily Sulatskov <redvasily@…>

comment:1 Changed 7 years ago by Vasily Sulatskov <redvasily@…>

  • Summary changed from Possibility of overriding django.contrib.auth.LOGIN_URL in settings.py to [patch]Possibility of overriding django.contrib.auth.LOGIN_URL in settings.py

comment:2 Changed 7 years ago by Gary Wilson <gary.wilson@…>

While we're at it we should go ahead and make the profile URL configurable too, which is currently hardcoded in the contrib.auth.views.login view.

comment:3 Changed 7 years ago by Vasily Sulatskov <redvasily@…>

  • Cc Vasily, Sulatskov, <redvasily@…> added
  • Summary changed from [patch]Possibility of overriding django.contrib.auth.LOGIN_URL in settings.py to [patch]Possibility of overriding django.contrib.auth.LOGIN_URL and account URL used in auth.views,login() in settings.py

I noticed that I myself use custom version of django.contrib.auth.views.login() that used different account URL, so I made it configurable also.

Patch: login_url2.diff

Changed 7 years ago by Vasily Sulatskov <redvasily@…>

comment:4 Changed 7 years ago by Vasily Sulatskov <redvasily@…>

  • Summary changed from [patch]Possibility of overriding django.contrib.auth.LOGIN_URL and account URL used in auth.views,login() in settings.py to [patch]Possibility of overriding django.contrib.auth.LOGIN_URL and account URL used in auth.views.login() in settings.py

Changed 7 years ago by Gary Wilson <gary.wilson@…>

using getattr instead of try/except

comment:5 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Keywords auth LOGIN_URL removed
  • Needs documentation set
  • Summary changed from [patch]Possibility of overriding django.contrib.auth.LOGIN_URL and account URL used in auth.views.login() in settings.py to Allow django.contrib.auth's LOGIN_URL and ACCOUNT_URL to be set in settings.py
  • Triage Stage changed from Unreviewed to Accepted

Adrian gave the ok in the thread mentioned in the ticket description.

Changed 7 years ago by Marc Fargas <telenieko@…>

Documentation for this ticket.

comment:6 follow-up: Changed 7 years ago by Marc Fargas <telenieko@…>

  • Needs documentation unset

Documentation added, please take one thing in account:
I set a link to @login_decorator twice (in the ACCOUNT_URL and LOGIN_URL sections) thus the line ".. _@login_decorator" is at the bottom of the file, I have no idea if this works, and this silly PC doesn't want to download docutils now to let me check... so please take this into account before appliying!

Also, I have no idea if the "@" can be there on the link... ;)

Hope I didn't miss anything to change.

comment:7 Changed 7 years ago by Vasily Sulatskov <redvasily@…>

Some more advocacy for this ticket: in this thread Why I'm giving up on Django one of the reasons for giving Django up was:

"This is a biggie for me. I can't believe that the authentication
module forces you to use hard coded urls for login/logout pages --
that's just maddening!"

Ahd that's what this patch fixes.

comment:8 in reply to: ↑ 6 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Triage Stage changed from Accepted to Ready for checkin

Replying to Marc Fargas <telenieko@telenieko.com>:

Documentation added, please take one thing in account:
I set a link to @login_decorator twice (in the ACCOUNT_URL and LOGIN_URL sections) thus the line ".. _@login_decorator" is at the bottom of the file, I have no idea if this works, and this silly PC doesn't want to download docutils now to let me check... so please take this into account before appliying!

Also, I have no idea if the "@" can be there on the link... ;)

Seems to work for me. Thanks for the patch.

Changed 7 years ago by Gary Wilson <gary.wilson@…>

code and docs in one patch

Changed 7 years ago by Gary Wilson <gary.wilson@…>

use relative link, as mentioned in #3333

comment:9 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

After playing around with contrib.comments, it appears that we should probably have a LOGOUT_URL also since in the comment app's form.html template there is a hardcoded /accounts/logout/. This would be simple enough to add, that we should just add it to this ticket instead of creating a new one.

comment:10 Changed 7 years ago by Marc Fargas <telenieko@…>

  • Needs documentation set
  • Summary changed from Allow django.contrib.auth's LOGIN_URL and ACCOUNT_URL to be set in settings.py to Allow django.contrib.auth's LOGIN_URL, LOGOUT_URL and ACCOUNT_URL to be set in settings.py

+1
Adding need documentation to document the settings.LOGOUT_URL when the patch comes in.

Changed 7 years ago by Vasily Sulatskov <redvasily@…>

Added LOGOUT_URL variable to auth/init.py. Modified comments app to use it.

comment:11 Changed 7 years ago by Vasily Sulatskov <redvasily@…>

I didn't test 3185_3.diff because I don't use comments app myself. But changes were obvious so perhaps patch will work fine.

comment:12 Changed 7 years ago by anonymous

  • Needs documentation unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 7 years ago by anonymous

Thanks for the patch Vasily.

comment:14 Changed 7 years ago by Gary Wilson <gary.wilson@…>

The above comments were made by me :)

comment:15 Changed 7 years ago by Jannis Leidel <jl@…>

+1 from me

comment:16 Changed 7 years ago by anonymous

  • Cc jonasvp@… added

comment:17 Changed 7 years ago by jacob

  • Patch needs improvement set

It's not clear to me which of these patches should be applied, and in which order. Can one of the patch authors upload an über-patch that does it all in one fell swoop?

Changed 7 years ago by Vasily Sulatskov <redvasily@…>

Uber patch. Everything included

comment:18 Changed 7 years ago by Vasily Sulatskov <redvasily@…>

I think that ACCOUNT_URL variable name is misleading, because it's not necessary to redirect user after successful login to account page. So I changed it to POSTLOGIN_REDIRECT_URL in 3185_4.diff.

3185_4.diff contains all needed changes.

comment:19 Changed 7 years ago by Marc Fargas <telenieko@…>

  • Patch needs improvement unset

Removing "needs better patch" as Vasily uploaded what jacob requested ;)

comment:20 Changed 7 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

The most recent patch still has a few subtle problems:

  1. Anything that is configurable should always be read from settings, not shoved in as a constant into some other module (it's not a constant any longer, after all). I realise LOGIN_URL is already a constant in the auth module, but that now becomes a proxy for the value from settings for backwards compatibility reasons. Everything else comes straight out of settings (including all current internal uses of LOGIN_URL).
  1. We should not be reading anything from settings at import time; only at execution time. The reason for this is manually configured settings. The developer needs to be able to import the necessary modules and then call settings.configure(). The current code reads from settings at import time and so will raise a configuration error. This problem exists in other parts of Django as well, but that's not a reason to perpetuate the problem.

Changed 7 years ago by Collin Grady <cgrady@…>

comment:21 Changed 7 years ago by Collin Grady <cgrady@…>

Took a stab at fixing the patch based on comments, as I would like this as well :)

comment:22 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [5072]) Fixed #3185 -- Made values for login, logout and post-login redirect URLs
configurable. This is a combined patch from Vasily Sulatskov, Marc Fargas and
Collin Grady.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.