Opened 13 years ago

Closed 11 years ago

Last modified 9 years ago

#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)

django-auth-string-pk-support.patch (5.4 KB ) - added by Jonas H. 13 years ago.
1.4.patch (7.1 KB ) - added by Jonas H. 12 years ago.
new patch against current SVN

Download all attachments as: .zip

Change History (26)

by Jonas H., 13 years ago

comment:1 by Łukasz Rekucki, 13 years ago

Needs documentation: set
Needs tests: set

comment:2 by Waldemar Kornewald, 13 years ago

Needs tests: unset

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.

comment:3 by Łukasz Rekucki, 13 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.

comment:4 by Waldemar Kornewald, 13 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.

in reply to:  4 comment:5 by Łukasz Rekucki, 13 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.

comment:6 by Jannis Leidel, 13 years ago

milestone: 1.3
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign 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 Waldemar Kornewald, 13 years ago

Cc: wkornewald@… added

comment:8 by James Addison, 13 years ago

Severity: Normal
Type: New feature

comment:9 by Chris Beaven, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededSomeday/Maybe

I'm going to shunt this to 'someday/maybe'.

comment:10 by Jonas H., 12 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

by Jonas H., 12 years ago

Attachment: 1.4.patch added

new patch against current SVN

comment:11 by Waldemar Kornewald, 12 years ago

Cc: wkornewald@… removed

in reply to:  6 comment:12 by anonymous, 12 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 Aymeric Augustin, 11 years ago

Triage Stage: Someday/MaybeAccepted

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:14 by Russell Keith-Magee <russell@…>, 11 years ago

In 91c26eadc9b4efa5399ec0f6c84b56a3f8eb84f4:

Refs #14881 -- Document that User models need to have an integer primary key.

Thanks to Kaloian Minkov for the reminder about this undocumented requirement.

comment:15 by Russell Keith-Magee <russell@…>, 11 years ago

In 461d6e22957770449cd99367358c5e419bc3a86d:

[1.5.x] Refs #14881 -- Document that User models need to have an integer primary key.

Thanks to Kaloian Minkov for the reminder about this undocumented requirement.

(cherry picked from commit 91c26eadc9b4efa5399ec0f6c84b56a3f8eb84f4)

comment:16 by Tim Graham, 11 years ago

Cc: timograham@… 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 Claude Paroz, 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 Luke Plant, 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 Tim Graham, 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 Claude Paroz, 11 years ago

Triage Stage: AcceptedReady for checkin

I've not run the tests, but looking at the patch, it looks good. Thanks!

comment:21 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 1184d077893ff1bc947e45b00a4d565f3df81776:

Fixed #14881 -- Modified password reset to work with a non-integer UserModel.pk.

uid is now base64 encoded in password reset URLs/views. A backwards compatible
password_reset_confirm view/URL will allow password reset links generated before
this change to continue to work. This view will be removed in Django 1.7.

Thanks jonash for the initial patch and claudep for the review.

comment:22 by Tim Graham <timograham@…>, 9 years ago

In a7aaabfaf1fa4c20065ab1133d49f40d4de6b409:

Removed doc note about PasswordResetForm requiring an integer PK.

This limitation was lifted in refs #14881.

comment:23 by Tim Graham <timograham@…>, 9 years ago

In e17d98ff0292caf02b62a3e84e10e3aaff3f4015:

[1.6.x] Removed doc note about PasswordResetForm requiring an integer PK.

This limitation was lifted in refs #14881.

Backport of a7aaabfaf1fa4c20065ab1133d49f40d4de6b409 from master

comment:24 by Tim Graham <timograham@…>, 9 years ago

In 8e68b590abd22dc46e8aa3c963c32713755f6172:

[1.7.x] Removed doc note about PasswordResetForm requiring an integer PK.

This limitation was lifted in refs #14881.

Backport of a7aaabfaf1fa4c20065ab1133d49f40d4de6b409 from master

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