Fix: Disable refresh token for inactive user.

[ajay09]

Issue Description:
A refresh token generated for a previously active user was allowed to issue new access token although the user is inactive.

Fix:
Added a check to verify if the user claim in refresh token corresponds to an active user.

Related issues

• TokenRefreshView should prevent users from refreshing token if the user is inactive #812

[wissam-al-kahwaji]

@ajay09
Actually, I suggest using Django signals to automatically update the BlacklistedToken list when user accounts are disabled. Once a user account is disabled, we'll add all of their tokens to the blacklist to prevent their future use. Since self.check_blacklist is available by default, I think this would be better than performing an additional query every time.

[ajay09]

@wissam-al-kahwaji
Thank you for your suggestion.
Signals are quite powerful, but I would hesitate using them here.
As a signal will be associated with the save() method in User models, but a user can be disabled without a call to save().

[wissam-al-kahwaji]

@ajay09
Do you mean to disable the user from the database directly?

[ajay09]

@wissam-al-kahwaji
Yes when the db is directly updated.
But also when we use bulk_update() as that doesn't call the save() method of the model.
https://docs.djangoproject.com/en/5.0/ref/models/querysets/#django.db.models.query.QuerySet.bulk_update

[wissam-al-kahwaji]

@ajay09
Oops, I totally forgot about that. I believe we can provide a function built on bulk_update to disable users and add them to the blacklist. This, in addition to providing signals, might indeed save a significant number of queries. Perhaps you have a different perspective, but I'm keen on avoiding any extra queries we can do without.

[ajay09]

@wissam-al-kahwaji
Less queries are always better.
I think it won't be possible in this scenario. AFAIK the library doesn't store per user refresh tokens. So even though we will have the user instance in the signal receiver, we won't have the refresh token associated with that user.

Please let me know if there is a way of getting the associated token.

[wissam-al-kahwaji]

@ajay09
Actually, all you need to do is add a boolean field to inform us if it's active or not. Thus, you're retaining only one query here, and the reason for adding the boolean field is to express the suspension of the user account in general, not just the refresh token. Please note that in this solution, you'll be storing the user without the refresh token, thus relying on determining whether the user or the refresh token is listed in the blacklist.

[ajay09]

I don't think we should modify the blacklist table to store whether use is active/inactive, preserving Separation of concerns.

[wissam-al-kahwaji]

It will be responsible for clarifying whether the user is completely blacklisted or whether it is just the token

[vainu-arto]

Hmm. I don't think this is the correct fix. Instead the refresh serializers should be calling api_settings.USER_AUTHENTICATION_RULE in their validate methods, like TokenObtainSerializer already does.

The implementation you have in this PR requires that blacklisting (an optional feature) is enabled.

Also JWTAuthentication should be using USER_AUTHENTICATION_RULE instead of only checking User.is_active (to gain access to any custom rules).

Also please add a test to verify the fix.

[ajay09]

@vainu-arto
That's a very nice suggestion.
I will move the logic to TokenRefreshSerializer.

Thanks.

[vainu-arto]

LGTM.

[ajay09]

@wissam-al-kahwaji
I have updated the code. Could you please provide a re-review?
Thanks.

[doctornkz-intelas]

@wissam-al-kahwaji Excuse me, but can we move ahead with this?

[wissam-al-kahwaji]

Nothing else, everything looks good, Thanks for your contribution

[timmyomahony]

Can we get this merged?

[ajay09]

@timmyomahony
I can't merge it, it says "At least 1 approving review is required by reviewers with write access"

[50-Course]

Uhmm.. can i take a look at this?

[ajay09]

Yes please!

[50-Course]

Following the conversation, the patches made and the review comments, this looks good to me.

[50-Course]

let's push this out there :)

[doctornkz-intelas]

I don't believe it, folks.

[barseghyanartur]

Please, make a PyPI release ASAP.