Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#14809 closed (fixed)

Broken login related tests after r14733

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

Download all attachments as: .zip

Change History (16)

comment:1 Changed 3 years ago by SmileyChris

  • Has patch set
  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to SmileyChris
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by SmileyChris

Changed 3 years ago by SmileyChris

Changed 3 years ago by SmileyChris

with tests for new QueryDict.urlencode behaviour

Changed 3 years ago by jezdez

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

comment:2 Changed 3 years ago by jezdez

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 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:4 Changed 3 years ago by SmileyChris

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 3 years ago by jezdez

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

comment:6 Changed 3 years ago by SmileyChris

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 3 years ago by jezdez

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

Changed 3 years ago by jezdez

Removed forced use of urlquote

comment:8 Changed 3 years ago by SmileyChris

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 3 years ago by jezdez

Totally, thanks :)

comment:10 Changed 3 years ago by SmileyChris

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

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

comment:11 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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.