Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

user_logged_in instead of update_last_login #752

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Ou7law007
Copy link

I did not see a reason to call update_last_login directly.

This allows code like the following to function properly.

# my_app.signals
# Track last_login & previous_login (custom field)
def update_last_and_previous_login(sender, user, **kwargs):
    User.objects.filter(pk=user.pk).update(previous_login=user.last_login, last_login=timezone.now())

user_logged_in.disconnect(update_last_login, dispatch_uid='update_last_login')
user_logged_in.connect(update_last_and_previous_login, dispatch_uid='update_last_and_previous_login')

I also thought of providing the data variable as a kwarg to allow even more flexibility.

user_logged_in.send(sender=self.user.__class__, user=self.user, **data) or user_logged_in.send(sender=self.user.__class__, user=self.user, data=data)

@Ou7law007
Copy link
Author

By pure coincidence, I just tried the library django-axes and without this code, it raises:

TypeError: AxesProxyHandler.user_logged_in() missing 1 required positional argument: 'request'

So this code seems to fix this issue as well. I don't understand the reason for calling the signals directly anyway!

Ou7law007 and others added 2 commits September 21, 2023 13:34
Fixed compatibility with `django-axes` error.

```
TypeError: AxesProxyHandler.user_logged_in() missing 1 required positional argument: 'request'
```

According to `django-axes`:
```
Authenticating users¶
Axes needs a request attribute to be supplied to the stock Django authenticate method in the django.contrib.auth module in order to function correctly.
```
https://django-axes.readthedocs.io/en/latest/3_usage.html
@Ou7law007
Copy link
Author

The new commit fixes the django-axes issue mentioned above.

self.__class__ is more accurate.
@Ou7law007
Copy link
Author

Also, the 2 user_logged_in signals should no be tied to api_settings.UPDATE_LAST_LOGIN.

I think api_settings.UPDATE_LAST_LOGIN is something else and that's why the original code called djangorestframework-simplejwt's signal directly.

This must be updated. Example:

# Send this signal anyway
user_logged_in.send(sender=self.__class__, user=self.user, data=data, request=self.context.get('request'))

if api_settings.UPDATE_LAST_LOGIN:
    # Send the signal that updates the user's last login (as in the original code)

@Ou7law007
Copy link
Author

One could (or even should) also add user_logged_out and user_login_failed signals to TokenObtainSerializer and TokenBlacklistSerializer respectively.

In which case, I don't know if any of the signals also applies to any of the rest of the serializers (TokenRefreshSerializer, TokenRefreshSlidingSerializer, TokenVerifySerializer).

@Ou7law007
Copy link
Author

Let me know if this is going anywhere or not.

@Andrew-Chen-Wang Andrew-Chen-Wang self-requested a review December 5, 2023 15:03
@Andrew-Chen-Wang
Copy link
Member

Hi please rebase with master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants