Opened 7 years ago

Closed 13 months 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: master
Severity: Normal Keywords: schemamigration
Cc: simon@…, martin.paquette@…, timo 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 Changed 7 years ago by Simon Greenhill

  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 5 years ago by sven.hoffmann@…

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?

comment:3 Changed 4 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.auth

comment:4 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 4 years ago by SmileyChris

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to 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:6 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 2 years ago by mohitbagga

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:8 Changed 23 months ago by timo

  • Keywords schemamigration added; auth last login user removed
  • Summary changed from Last_login in django.contrib.auth should has null=True to django.contrib.auth.models.AbstractBaseUser.last_login should allow null=True
  • Type changed from Bug to Cleanup/optimization

comment:9 Changed 19 months ago by simon29

  • Cc simon@… added

comment:10 Changed 15 months ago by martinpaquette

  • Cc martin.paquette@… added

comment:11 follow-up: Changed 13 months ago by timo

  • Cc timo added
  • Has patch set
  • Triage Stage changed from Someday/Maybe to 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.

PR

comment:12 in reply to: ↑ 11 Changed 13 months ago by charettes

Replying to timo:

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?

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?

comment:13 follow-up: Changed 13 months ago by timo

  1. I think your suggestion is okay; PR updated with it.
  1. 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 in reply to: ↑ 13 Changed 13 months ago by charettes

Replying to timo:

  1. I think your suggestion is okay; PR updated with it.

The changes LGTM.

  1. 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 Changed 13 months ago 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.

comment:16 Changed 13 months ago by charettes

  • Triage Stage changed from Accepted to Ready for checkin

Except for the test comment it looks good to me.

comment:17 Changed 13 months ago by Tim Graham <timograham@…>

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

In a2479f46f3d05b37254ad701b6b76f84624d3cb5:

Fixed #7220 -- Allowed AbstractBaseUser.last_login to be null.

Thanks veena for the suggestion and Simon Charette and Kévin Etienne for reviews.

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