Opened 7 years ago

Closed 7 years ago

#29132 closed Bug (fixed)

update_last_login signal handler shouldn't be connected if User.last_login attribute isn't a field

Reported by: Mikhail Porokhovnichenko Owned by: Mikhail Porokhovnichenko
Component: contrib.auth Version: 2.0
Severity: Normal Keywords: last_login, update_last_login, signals, user_logged_in
Cc: 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 (last modified by Mikhail Porokhovnichenko)

Please note! It's not a #26823 duplicate, and it's just an issue related this one.

In the current implementation, a signal handler connects with a user model when this model has a last_login field.

if hasattr(get_user_model(), 'last_login'):
    from .models import update_last_login
    user_logged_in.connect(update_last_login, dispatch_uid='update_last_login')

And it would work when there isn't a last_login attribute in the custom user model. But what if I've inherited my custom model from AbstractUser and dropped the last_login by setting it to None?

class User(AbstractBaseUser):
    last_login = None     # Drop ``last_login`` field off

Technically, the model has a last_login attribute, but it's not a field!

Suggesting change the check condition something like that:

last_login = getattr(get_user_model(), 'last_login', None)
if last_login is not None:
    # ...

or even

if isisintance(last_login, models.Field):
    # ...

Change History (9)

comment:1 by Mikhail Porokhovnichenko, 7 years ago

Description: modified (diff)

comment:2 by Tim Graham, 7 years ago

Summary: Incorrect update_last_login signal handler behavior when using custom User model with an arbitrary last_login attribute (not a Field instance)update_last_login signal handler shouldn't be connected if User.last_login attribute isn't a field
Triage Stage: UnreviewedAccepted

I was thinking that isinstance(... Field) might be incorrect if last_login was a settable property, but in that case, I guess user.save(update_fields['last_login']) probably wouldn't be desired.

comment:3 by Mikhail Porokhovnichenko, 7 years ago

Sorry, I didn't get it. What approach you'd like to use here? Need I create a patch?

comment:4 by Tim Graham, 7 years ago

Try the isinstance() approach. You're welcome to create a patch. Most of the work will be writing a test.

comment:5 by Mikhail Porokhovnichenko, 7 years ago

Owner: changed from nobody to Mikhail Porokhovnichenko
Status: newassigned

Ok, will do it ASAP

comment:6 by Mikhail Porokhovnichenko, 7 years ago

Seems to be done. Can be reviewed here

comment:7 by Mikhail Porokhovnichenko, 7 years ago

Has patch: set

Please check PR#9712.

comment:8 by Carlton Gibson, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 14e34dcf:

Fixed #29132 -- Avoided connecting update_last_login() handler if User.last_login isn't a field.

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