Opened 17 years ago
Closed 10 years ago
#7220 closed Cleanup/optimization (fixed)
django.contrib.auth.models.AbstractBaseUser.last_login should allow null=True
Reported by: | veena | Owned by: | anonymous |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | schemamigration |
Cc: | simon@…, martin.paquette@…, Tim Graham | 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
I think attribute last_login in model User in django.contrib.auth should has property null=True.
Now it has only default=datetime.datetime.now
So, when you create new user the last_login is set to now datetime.
But when you create new user it doesn't mean that user has been logged in. For example in django-registration there is first user created and link to activation is sended to his email. After user activates his account he can login.
Change History (17)
comment:1 by , 17 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 14 years ago
comment:3 by , 14 years ago
Component: | Contrib apps → contrib.auth |
---|
comment:4 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:5 by , 14 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Design decision needed → Someday/Maybe |
The problem is that it leaves any User model created previously in an indeterminate state, since we have no way to provide migrations in core.
For that reason, I'm shunting this to a 'someday' issue.
comment:7 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 11 years ago
Keywords: | schemamigration added; auth last login user removed |
---|---|
Summary: | Last_login in django.contrib.auth should has null=True → django.contrib.auth.models.AbstractBaseUser.last_login should allow null=True |
Type: | Bug → Cleanup/optimization |
comment:9 by , 11 years ago
Cc: | added |
---|
comment:10 by , 11 years ago
Cc: | added |
---|
follow-up: 12 comment:11 by , 10 years ago
Cc: | added |
---|---|
Has patch: | set |
Triage Stage: | Someday/Maybe → Accepted |
One problem that came up when I tried to implement this is that user.last_login
is used in PasswordResetTokenGenerator
. Can we 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.
comment:12 by , 10 years ago
Replying to timo:
One problem that came up when I tried to implement this is that
user.last_login
is used inPasswordResetTokenGenerator
. Can we substitute a fixed date iflast_login
isNone
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 wherelast_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?
follow-up: 14 comment:13 by , 10 years ago
- I think your suggestion is okay; PR updated with it.
- 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.
comment:14 by , 10 years ago
Replying to timo:
- I think your suggestion is okay; PR updated with it.
The changes LGTM.
- 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.
comment:15 by , 10 years ago
I removed the data migration and added a mention in the release notes. It seems safer and is less code for us to maintain.
comment:16 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Except for the test comment it looks good to me.
comment:17 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
We would like to use the field last_login to determine if a user has ever logged in.
now we need to check if last_login == date_joined to do that but that's pretty weird,
as the user actually has not logged in yet, but has a date for last_login set.
why no just have the field null until first login as suggested in this ticket?