#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)
Change History (16)
comment:1 by , 14 years ago
| Has patch: | set |
|---|---|
| milestone: | → 1.3 |
| Owner: | changed from to |
| Status: | new → assigned |
| Triage Stage: | Unreviewed → Accepted |
by , 14 years ago
| Attachment: | 14809.diff added |
|---|
by , 14 years ago
| Attachment: | 14809.2.diff added |
|---|
by , 14 years ago
| Attachment: | 14809.4.diff added |
|---|
Patch that uses django.utils.http.urlquote and has docs.
comment:2 by , 14 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 , 14 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
comment:4 by , 14 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 , 14 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 , 14 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:8 by , 14 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:10 by , 14 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
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.encodemethod. See the patch I'm attaching.