Skip to content

fix: Make don't create and save new refresh token with None when refr… #634

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

juchajam
Copy link

fix: Make don't create and save new refresh token with None when refresh token is not included in the request.

@iMerica
Copy link
Owner

iMerica commented Jul 7, 2024

Sorry I don't understand the intent of this change. Can you explain it in very simple terms? The grammar of your PR title is confusing.

@juchajam
Copy link
Author

juchajam commented Jul 7, 2024

Sorry I don't understand the intent of this change. Can you explain it in very simple terms? The grammar of your PR title is confusing.

Sorry for my bad English.

When USE_JWT is True and logout without refresh token, response.status_code will be 401.

And create new refresh token and blacklist it.
I fixed this part.

@bartvanandel
Copy link

bartvanandel commented Mar 13, 2025

I think I can see a couple code paths which would indeed cause trouble, all requiring that USE_JWT is True:

  • JWT_AUTH_HTTPONLY set to True, but no valid cookie, hence RefreshToken(the_cookie) will fail.
  • JWT_AUTH_HTTPONLY set to False, and the request body either not containing a refresh value at all, or an invalid or expired one, causing RefreshToken(the_value) to fail.
  • JWT_AUTH_HTTPONLY set to False, and a request body of {"refresh": null}. This will actually result in a valid RefreshToken, but obviously a new one. In this case, HTTP 401 isn't even triggered before the proposed fix.

In all these cases, a new token is being blacklisted. This indeed shouldn't happen.

The proposed PR fixes all of the above, as far as I can tell. Looking at the code, more can be improved, e.g. DRY (in this block, about half the lines are duplicates, both before and after the PR) never mind, different error message required

A bit further down (outside of the current scope of the PR), there are some changes needed as well to cope with newer versions of rest_framework_simplejwt, which has now split "Token is invalid" from "Token is expired". I'll have a look myself, we're running into this right now.

@bartvanandel
Copy link

@iMerica Does the above answer your question?

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

Successfully merging this pull request may close these issues.

3 participants