django.contrib.auth.models.AbstractBaseUser.last_login should allow null=True

[veena]

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.

[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?

[Chris Beaven]

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.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Tim Graham]

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

[Simon Charette]

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?

[Tim Graham]

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.

[Simon Charette]

Replying to 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.

[Tim Graham]

I removed the data migration and added a mention in the release notes. It seems safer and is less code for us to maintain.

[Simon Charette]

Except for the test comment it looks good to me.

[Tim Graham <timograham@…>]

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.