Code

Opened 3 years ago

Closed 10 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: jonash 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 jonash 3 years ago.
1.4.patch (7.1 KB) - added by jonash 2 years ago.
new patch against current SVN

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by jonash

comment:1 Changed 3 years ago by lrekucki

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

comment:2 Changed 3 years ago by wkornewald

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

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 follow-up: Changed 3 years ago by wkornewald

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

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 follow-up: Changed 3 years ago by jezdez

  • milestone 1.3 deleted
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to 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 Changed 3 years ago by wkornewald

  • Cc wkornewald@… added

comment:8 Changed 3 years ago by jaddison

  • Severity set to Normal
  • Type set to New feature

comment:9 Changed 3 years ago by SmileyChris

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Someday/Maybe

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

comment:10 Changed 2 years ago by jonash

  • Keywords nonrel added
  • Summary changed from Do not assume ``User.id`` to be an integer in django.contrib.auth's pasword reset feature to [nonrel] Do not assume ``User.id`` to be an integer in django.contrib.auth's pasword reset feature
  • UI/UX unset

Changed 2 years ago by jonash

new patch against current SVN

comment:11 Changed 2 years ago by wkornewald

  • Cc wkornewald@… removed

comment:12 in reply to: ↑ 6 Changed 2 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 16 months ago by aaugustin

  • Triage Stage changed from Someday/Maybe to 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:14 Changed 14 months 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 14 months 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 10 months ago by timo

  • 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 10 months ago by claudep

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 10 months ago by lukeplant

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 10 months ago by timo

  • 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 10 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

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

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

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

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.

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.