Re: [Django] #7220: Last_login in django.contrib.auth should has null=True
Django
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: auth last login | Triage Stage:
user | Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mohitbagga):
* owner: nobody => anonymous
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Severity: Normal | Triage Stage:
Keywords: schemamigration | Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* keywords: auth last login user => schemamigration
* type: Bug => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:8>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Severity: Normal | Triage Stage:
Keywords: schemamigration | Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: simon@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:9>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Severity: Normal | Triage Stage:
Keywords: schemamigration | Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: martin.paquette@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:10>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Keywords: schemamigration | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* cc: timo (added)
* has_patch: 0 => 1
* stage: Someday/Maybe => Accepted
Comment:
One problem that came up when I tried to implement this is that
`user.last_login` is used in `PasswordResetTokenGenerator`. Can we
[https://github.com/django/django/blob/master/django/contrib/auth/tokens.py#L59
substitute a fixed date] if `last_login` is `None` without losing
security?
I'm also interested in opinions about including a data migration that sets
`last_login` to null for any users where `last_login == date_created`.
This would make the field on existing users consistent with new users.
[https://github.com/django/django/pull/2997 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:11>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: schemamigration | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
Replying to [comment:11 timo]:
> One problem that came up when I tried to implement this is that
`user.last_login` is used in `PasswordResetTokenGenerator`. Can we
[https://github.com/django/django/blob/master/django/contrib/auth/tokens.py#L59
substitute a fixed date] if `last_login` is `None` without losing
security?
What about not using the `last_login` date to generate the hash it's
`None`?
> I'm also interested in opinions about including a data migration that
sets `last_login` to null for any users where `last_login ==
date_created`. This would make the field on existing users consistent with
new users.
It makes sense to me. Makes me wonder if we have any ways of testing a
data migration yet?
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:12>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: schemamigration | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
1. I think your suggestion is okay; PR updated with it.
2. I added a data migration to the PR. I am not sure if it's a good idea,
especially because `User` is swappable. It may be better to put that code
in the release notes and ask them to run it if they'd like. For testing, I
would probably just test the function in the migration outside of the
migrations framework itself.
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:13>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: schemamigration | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
Replying to [comment:13 timo]:
> 1. I think your suggestion is okay; PR updated with it.
The changes LGTM.
> 2. I added a data migration to the PR. I am not sure if it's a good
idea, especially because `User` is swappable. It may be better to put that
code in the release notes and ask them to run it if they'd like. For
testing, I would probably just test the function in the migration outside
of the migrations framework itself.
I won't strongly oppose to a mention in the release notes instead but it
should be pretty safe to rely on the `User` model if it's retrieved from
the provided `apps` based on the `AUTH_USER_MODEL` setting.
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:14>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: schemamigration | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
I removed the data migration and added a mention in the release notes. It
seems safer and is less code for us to maintain.
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:15>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution:
Keywords: schemamigration | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
Except for the test comment it looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:16>
Django
null=True
-------------------------------------+-------------------------------------
Reporter: veena | Owner: anonymous
Cleanup/optimization | Version: master
Component: contrib.auth | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: schemamigration | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a2479f46f3d05b37254ad701b6b76f84624d3cb5"]:
{{{
#!CommitTicketReference repository=""
revision="a2479f46f3d05b37254ad701b6b76f84624d3cb5"
Fixed #7220 -- Allowed AbstractBaseUser.last_login to be null.
Thanks veena for the suggestion and Simon Charette and Kévin Etienne for
reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/7220#comment:17>