Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#14809 closed (fixed)

Broken login related tests after r14733

Reported by: Jannis Leidel Owned by: Chris Beaven
Component: contrib.auth Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by Chris Beaven

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.

Changed 6 years ago by Chris Beaven

Attachment: 14809.diff added

Changed 6 years ago by Chris Beaven

Attachment: 14809.2.diff added

Changed 6 years ago by Chris Beaven

Attachment: 14809.3.diff added

with tests for new QueryDict.urlencode behaviour

Changed 6 years ago by Jannis Leidel

Attachment: 14809.4.diff added

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

comment:2 Changed 6 years ago by Jannis Leidel

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 Changed 6 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

comment:4 Changed 6 years ago by Chris Beaven

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 Changed 6 years ago by Jannis Leidel

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

comment:6 Changed 6 years ago by Chris Beaven

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 Changed 6 years ago by Jannis Leidel

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

Changed 6 years ago by Jannis Leidel

Attachment: 14809.5.diff added

Removed forced use of urlquote

comment:8 Changed 6 years ago by Chris Beaven

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 Changed 6 years ago by Jannis Leidel

Totally, thanks :)

comment:10 Changed 6 years ago by Chris Beaven

Resolution: fixed
Status: assignedclosed

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

comment:11 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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