Opened 6 years ago

Closed 3 years ago

Last modified 21 months 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: master
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. 6 years ago.
1.4.patch (7.1 KB) - added by Jonas H. 5 years ago.
new patch against current SVN

Download all attachments as: .zip

Change History (26)

Changed 6 years ago by Jonas H.

comment:1 Changed 6 years ago by Łukasz Rekucki

Needs documentation: set
Needs tests: set
Patch needs improvement: unset

comment:2 Changed 6 years ago by Waldemar Kornewald

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 Changed 6 years ago by Łukasz Rekucki

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 Changed 6 years ago by Waldemar Kornewald

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 in reply to:  4 Changed 6 years ago by Łukasz Rekucki

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

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 Changed 6 years ago by Waldemar Kornewald

Cc: wkornewald@… added

comment:8 Changed 5 years ago by James Addison

Severity: Normal
Type: New feature

comment:9 Changed 5 years ago by Chris Beaven

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

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

comment:10 Changed 5 years ago by Jonas H.

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

Changed 5 years ago by Jonas H.

Attachment: 1.4.patch added

new patch against current SVN

comment:11 Changed 5 years ago by Waldemar Kornewald

Cc: wkornewald@… removed

comment:12 in reply to:  6 Changed 5 years ago by anonymous

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 Changed 4 years ago by Aymeric Augustin

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 Changed 4 years ago by Russell Keith-Magee <russell@…>

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 Changed 4 years ago by Russell Keith-Magee <russell@…>

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 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Claude Paroz

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 Changed 3 years ago by Luke Plant

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 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

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

comment:21 Changed 3 years ago by Tim Graham <timograham@…>

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 Changed 21 months ago by Tim Graham <timograham@…>

In a7aaabfaf1fa4c20065ab1133d49f40d4de6b409:

Removed doc note about PasswordResetForm requiring an integer PK.

This limitation was lifted in refs #14881.

comment:23 Changed 21 months ago by Tim Graham <timograham@…>

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 Changed 21 months ago by Tim Graham <timograham@…>

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