Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#14809 closed (fixed)

Broken login related tests after r14733

Reported by: Jannis Leidel Owned by: Chris Beaven
Component: contrib.auth Version: dev
Severity: Keywords:
Cc: 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

There seems to be a urlencoding related issues with the redirect URLs.

Test results: https://gist.github.com/070ed7b16247f04ae388

This is on Mac OS X 10.6.5, Python 2.6.1

Attachments (5)

14809.diff (2.1 KB ) - added by Chris Beaven 13 years ago.
14809.2.diff (2.1 KB ) - added by Chris Beaven 13 years ago.
14809.3.diff (2.8 KB ) - added by Chris Beaven 13 years ago.
with tests for new QueryDict.urlencode behaviour
14809.4.diff (6.9 KB ) - added by Jannis Leidel 13 years ago.
Patch that uses django.utils.http.urlquote and has docs.
14809.5.diff (6.6 KB ) - added by Jannis Leidel 13 years ago.
Removed forced use of urlquote

Download all attachments as: .zip

Change History (16)

comment:1 by Chris Beaven, 13 years ago

Has patch: set
milestone: 1.3
Owner: changed from nobody to Chris Beaven
Status: newassigned
Triage Stage: UnreviewedAccepted

My bad :-/

A recent commit of mine changed the auth code to urlencode for the login querystring (so that other querystring arguments could also safely be included).
I did notice this at some point (but see now that there are no specific tests around this in contrib.admin)

To keep the old behaviour but still allow for extra login_url querystring arguments, I think the easiest way will be to extend the QueryDict.encode method. See the patch I'm attaching.

by Chris Beaven, 13 years ago

Attachment: 14809.diff added

by Chris Beaven, 13 years ago

Attachment: 14809.2.diff added

by Chris Beaven, 13 years ago

Attachment: 14809.3.diff added

with tests for new QueryDict.urlencode behaviour

by Jannis Leidel, 13 years ago

Attachment: 14809.4.diff added

Patch that uses django.utils.http.urlquote and has docs.

comment:2 by Jannis Leidel, 13 years ago

I've slightly updated your patch to use Django's own django.utils.http.urlquote instead of urllib's, added a bit of documentation and updated the auth tests.

comment:3 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:4 by Chris Beaven, 13 years ago

Doesn't Django's version just ensure it has been cast to a bytestring though? We're doing that explicitly in the urlencode method anyway.

Thanks for the docs (but note to self: there's a missing mutable=True on the example)

comment:5 by Jannis Leidel, 13 years ago

I suppose using our own utility functions is a worthwhile effort to prevent reimplementing the same thing over and over again.

comment:6 by Chris Beaven, 13 years ago

The difference here is that smart_str is being done explicitly anyway since a QueryDict may have its own specified encoding. But I guess it doesn't hurt passing it through our functions again -- just seems rather pointless to me (and a negligibly larger change, since previously it didn't use Django's urlencode).

comment:7 by Jannis Leidel, 13 years ago

Hm, yeah, let's go without using our quote then.

by Jannis Leidel, 13 years ago

Attachment: 14809.5.diff added

Removed forced use of urlquote

comment:8 by Chris Beaven, 13 years ago

Some minor tweaks to your code (as shown in the recent commit): https://github.com/SmileyChris/django/compare/master...14809-broken-login-related-tests

It's looking good, you happy with it?

comment:9 by Jannis Leidel, 13 years ago

Totally, thanks :)

comment:10 by Chris Beaven, 13 years ago

Resolution: fixed
Status: assignedclosed

(In [14764]) Fixed #14809 -- broken login related tests after r14733.

comment:11 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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