Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#11025 closed (fixed)

Ability to specify LOGIN_URL as full qualified absolute URL, ex: https://passport.example.com/passport?mode=login

Reported by: arikon Owned by: SmileyChris
Component: contrib.auth Version: master
Severity: Keywords:
Cc: thasonic@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

We need the ability to specify LOGIN_URL as full qualified absolute URL, for example:

LOGIN_URL="https://passport.example.com/passport?mode=login"

We need this abiliy in case we use custom auth backend to validate auth set by another "passport" domain.

Attachments (3)

django-contrib-auth-absolute-login-url.patch (3.0 KB) - added by arikon 6 years ago.
django.contrib.auth absolute LOGIN_URL patch ver. 1
django-contrib-auth-absolute-login-url.2.patch (3.4 KB) - added by thasonic 5 years ago.
11025.diff (14.9 KB) - added by SmileyChris 5 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 in reply to: ↑ description Changed 6 years ago by arikon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Also we need the ability to specify REDIRECT_FIELD_NAME.
It can differ from default "next" in case we use external "passport" auth server.

Changed 6 years ago by arikon

django.contrib.auth absolute LOGIN_URL patch ver. 1

comment:2 Changed 6 years ago by arikon

  • Has patch set

comment:3 Changed 6 years ago by arikon

  • Cc thasonic@… added

comment:4 Changed 6 years ago by arikon

  • Summary changed from Ability to specify LOGIN_URL as full qualified absolute URL, ex: https://passport.example.com/passport?mode=login to [patch] Ability to specify LOGIN_URL as full qualified absolute URL, ex: https://passport.example.com/passport?mode=login

comment:5 Changed 5 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Accepting the idea, but there are better ways to implement it - urlparse contains much better tools for reconstructing URL request parameters.

comment:6 Changed 5 years ago by thasonic

I've updated patch according to comment of Russel. Is it OK now?

Changed 5 years ago by thasonic

Changed 5 years ago by SmileyChris

comment:7 Changed 5 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Patch needs improvement unset
  • Status changed from new to assigned

This started as a small change to thasonic's code, then grew into a larger fix and some refactoring.

Better diff can be found here: http://github.com/SmileyChris/django/compare/master...11025-absolute-login_url

comment:8 Changed 4 years ago by carljm

If there's to be a refactor of LOGIN_URL to support fully qualified URLs, I think it should also support (or even prefer) URL pattern names, as discussed here: http://groups.google.com/group/django-developers/browse_thread/thread/fa3661888716f940/2265736038d5a21e

It makes me cringe a bit to see yet another setting (LOGIN_URL_NEXT_ARG) added in this patch. (Also a module-import-time settings dependency, which IMO is to be generally avoided). Is that really an important enough axis of configuration to warrant a setting, as opposed to the view argument that already exists? I'd like to see some discussion of use cases: ISTM that if you're using the django.contrib.auth builtin login view, there's really no reason you can't just use "next," and if you must use something else it's already possible to configure that via argument. If you have more esoteric needs, it's unlikely this setting will help anyway: you're more likely to need the flexibility allowed by e.g. allowing LOGIN_URL to point to a callable (as proposed in http://groups.google.com/group/django-developers/browse_thread/thread/63ceb6269f18e52c/e9830f62dba8ef49).

comment:9 Changed 4 years ago by SmileyChris

Thanks for the review, Carl.

The support of pattern names is a separate issue, and doesn't need to tie in to this ticket.
That said, while this ticket doesn't address it directly, my current changes do actually now allow using a LOGIN_URL and a lazy reverse pattern. I just tested it and this works in settings:

from django.utils.functional import lazy
from django.core.urlresolvers import reverse
LOGIN_URL = lazy(reverse, str)('login')

(#5925 would just become adding a lazy_reverse so that people don't need to repeat the lazy(reverse, str) pattern which I can see people getting wrong by trying lazy(reverse, unicode))

Regarding the setting, I also initially cringed. One reason for it is that a third-party app may be using the login_required decorator, meaning that you'd have to rewrite its entire urlconf to alter the next arg. I guess it is a rare case that people will care about changing the next arg though, and like you say, it *is* possible to get around...

comment:10 Changed 4 years ago by jezdez

  • milestone set to 1.3
  • Summary changed from [patch] Ability to specify LOGIN_URL as full qualified absolute URL, ex: https://passport.example.com/passport?mode=login to Ability to specify LOGIN_URL as full qualified absolute URL, ex: https://passport.example.com/passport?mode=login
  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 4 years ago by SmileyChris

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

(In [14733]) Fixes #11025 -- ability to specify LOGIN_URL as full qualified absolute URL.

auth.views.login now allows for login redirections for different schemes
with the same host (or no host even, e.g. 'https:///login/')

auth.decorators.login_required can now use lazy urls (refs #5925)

comment:12 Changed 4 years ago by timo

I'm not sure this is 100% backwards compatible. For example, if I have the following assertion:

self.assertRedirects(response, "/login/?next=/foo/")

I need to change it to:

self.assertRedirects(response, "/login/?next=%2Ffoo%2F")

Might warrant a mention in the release notes?

comment:13 Changed 4 years ago by SmileyChris

It's being fixed in #14809 - but thanks for the report!

comment:14 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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