Opened 15 years ago

Closed 13 years ago

Last modified 12 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: Sergey Belov Owned by: Chris Beaven
Component: contrib.auth Version: dev
Severity: Keywords:
Cc: thasonic@… 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

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 Sergey Belov 15 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 14 years ago.
11025.diff (14.9 KB ) - added by Chris Beaven 13 years ago.

Download all attachments as: .zip

Change History (17)

in reply to:  description comment:1 by Sergey Belov, 15 years ago

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

by Sergey Belov, 15 years ago

django.contrib.auth absolute LOGIN_URL patch ver. 1

comment:2 by Sergey Belov, 15 years ago

Has patch: set

comment:3 by Sergey Belov, 15 years ago

Cc: thasonic@… added

comment:4 by Sergey Belov, 15 years ago

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

comment:5 by Russell Keith-Magee, 14 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

comment:6 by thasonic, 14 years ago

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

by Chris Beaven, 13 years ago

Attachment: 11025.diff added

comment:7 by Chris Beaven, 13 years ago

Owner: changed from nobody to Chris Beaven
Patch needs improvement: unset
Status: newassigned

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 by Carl Meyer, 13 years ago

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 by Chris Beaven, 13 years ago

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 by Jannis Leidel, 13 years ago

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

comment:11 by Chris Beaven, 13 years ago

Resolution: fixed
Status: assignedclosed

(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 by Tim Graham, 13 years ago

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 by Chris Beaven, 13 years ago

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

comment:14 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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