#14881 closed New feature (fixed)
[nonrel] Do not assume ``User.id`` to be an integer in django.contrib.auth's pasword reset feature
Reported by: | Jonas H. | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | nonrel |
Cc: | timograham@… | 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
This is a request to cherry-pick a recently submitted patch to Django-nonrel that makes it possible to use django.contrib.auth
with databases that don't use integer-based auto IDs.
The URL token has to be changed anyway when Django-nonrel gets merged into Django trunk in 1.4 (or whenever), so it might be more convenient to change this behaviour earlier.
The patch does not do any backwards compatibility handling because I think it's not worth the effort although it should be rather easy (simply a compatibility URL associated with a view that un-base36s the id and then calls the wrapped view with the base64d ID).
Attachments (2)
Change History (26)
by , 14 years ago
Attachment: | django-auth-string-pk-support.patch added |
---|
comment:1 by , 14 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
comment:2 by , 14 years ago
Needs tests: | unset |
---|
comment:3 by , 14 years ago
I was thinking, about something that tests that this works with a model with non-integer pk, but maybe I misunderstood the purpose of the ticket.
I also fail to see how changing to base64 is related to this issue. I don't know what was the original cause to use base36, but I think it must had been something more then desire to use '-' in URL.
follow-up: 5 comment:4 by , 14 years ago
The base36 functions only work with integers. Since we have to deal with string here we just switched to base64.
The purpose of this ticket is indeed to allow for a string-based AutoField
(note, I don't mean CharField(primary_key=True)
), but this is practically impossible to test on a SQL database because AutoField
is always an integer. At least, I don't know how to change the id
field to a CharField
at runtime on SQL (not with a reasonable amount of effort).
BTW, Jonas, you indeed still have to update the documentation. The current docs state that the reset views get a "uidb36" parameter which is base36-encoded.
comment:5 by , 14 years ago
Replying to wkornewald:
The base36 functions only work with integers. Since we have to deal with string here we just switched to base64.
Right, I kept reading base32. Sorry for the noise.
follow-up: 12 comment:6 by , 14 years ago
milestone: | 1.3 |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Design decision needed |
So there are multiple things pretty unclear to me here.
First, the assumption of merging the 3rd party fork "Django-nonrel" into trunk is wrong. There has never been an endorsement nor an announcement of this. Please refrain from giving this as a reason for a feature request.
Second and more important, I don't see a reason to support non-integer IDs in 1.3 since none of the core ORM backends support that kind of thing. In other words, this has to wait till we have a proper story ourselves.
Marking accordingly.
comment:7 by , 14 years ago
Cc: | added |
---|
comment:8 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:9 by , 13 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Design decision needed → Someday/Maybe |
I'm going to shunt this to 'someday/maybe'.
comment:10 by , 13 years ago
Keywords: | nonrel added |
---|---|
Summary: | Do not assume ``User.id`` to be an integer in django.contrib.auth's pasword reset feature → [nonrel] Do not assume ``User.id`` to be an integer in django.contrib.auth's pasword reset feature |
UI/UX: | unset |
comment:11 by , 13 years ago
Cc: | removed |
---|
comment:12 by , 13 years ago
Replying to jezdez:
I don't see a reason to support non-integer IDs in 1.3 since none of the core ORM backends support that kind of thing.
People want to use Django with non-rel backends not in core today. That's what this patch allows.
The patch seems straightforward, the only big issue I can see is that it breaks already generated password resets. Is that an OK backwards incompatibility tradeoff to get the possibility to do auth with non-rel backends?
comment:13 by , 12 years ago
Triage Stage: | Someday/Maybe → Accepted |
---|
With the introduction of custom user models in 1.5, it's now possible to have a User model with a non-integer primary key.
comment:16 by , 11 years ago
Cc: | added |
---|
I've worked to get the current patch to apply cleanly and the tests passing on Python 2 & 3.
We need a decision whether or not we are comfortable introducing this as a backwards incompatible change or if we need to do something to allow password reset URLs generated in prior versions of Django to continue to work.
If this approach is accepted, I'll also update the documentation and look into adding an additional test.
https://github.com/timgraham/django/commit/87b2613ec25e356aed4e9d82e620d386e7f7ae33
comment:17 by , 11 years ago
I think it wouldn't be too hard to support both forms, at least for 2-3 releases, as far as both regexes cannot overlap (to be confirmed). So unless faced with a major obstacle, I'd vote for keeping the old form too (only for the decoding part, of course).
comment:18 by , 11 years ago
In the past, for similar things where we need backwards compatibility with short-lived data, rather than code or long-lived data, we've normally gone for a deprecation process where we have one release which has transitional support i.e. supports the old data, then we remove it.
https://docs.djangoproject.com/en/dev/releases/1.4/#compatibility-with-old-signed-data
comment:19 by , 11 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Thanks for the feedback. I've added documentation and a backwards compatible shim: https://github.com/django/django/pull/1303
comment:20 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I've not run the tests, but looking at the patch, it looks good. Thanks!
comment:21 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
The patch already contains changes to the auth unit tests. Isn't this enough? If not, please explain what is missing and please re-enable the "needs tests" checkbox.