fix: Avoid DoesNotExist exception in TokenRefreshSerializer

[yuekui]

#860
For deleted users, they should be treated the same as when no active user is found. This DoesNotExist exception was introduced in the previous version.

[vgrozdanic]

Looks good to me, thank you for the contribution!

I added this PR on a checklist for next major release since it does some breaking changes: #871

[Andrew-Chen-Wang]

this may not be expected since the USER_AUTHENTICATION_RULE expects a user object, not None

djangorestframework-simplejwt/rest_framework_simplejwt/authentication.py

Line 170 in faeea49

def default_user_authentication_rule(user: AuthUser) -> bool:

suggestion: simply check if the user exists OR check authentication rule does not pass rather than solely rely on the authentication rule

[yuekui]

Since the USER_AUTHENTICATION_RULE method already performs the user is not None check, would adding another check outside the method introduce unnecessary code duplication?

djangorestframework-simplejwt/rest_framework_simplejwt/authentication.py

Line 178 in faeea49

return user is not None and (

[Andrew-Chen-Wang]

yea, the problem is the initial typing was typed assuming the user object is valid...

I mean that's my fault for not checking that. I guess this is fine, but we should also update the typing for the authentication rule?

[yuekui]

Makes sense, updated

[Andrew-Chen-Wang]

awesome thank you!

[Andrew-Chen-Wang]

qq @yuekui @mjbogusz : are you facing 500 error exceptions with this? In other words, are you having to manually catch this edge case? I noticed that the DRF exception does not inherit from the user not found exception, making this a breaking change

[mjbogusz]

I didn't get as far as getting 500s, as I was upgrading from 5.0 and our unit tests failed on previously unexpected UserNotFound.

AuthenticationFailed is already handled by DRF's APIView:handle_exception() and it's consistent with neighboring behavior for a not active account, so I've added handling this edge case by raising the same type of error and our tests passed again without any additional error handling.

[yuekui]

qq @yuekui @mjbogusz : are you facing 500 error exceptions with this? In other words, are you having to manually catch this edge case? I noticed that the DRF exception does not inherit from the user not found exception, making this a breaking change

Yes, it's a 500 error and a breaking change for me. That's why I opened the issue and submitted the patch right after v5.4.0 was released.

[Andrew-Chen-Wang]

Got it, it sounds like a patch release is necessary rather than a minor/major version upgrade.

[mjbogusz]

I'd say this sounds like a minor-release-level change, as it shouldn't require immediate code adjustments whether or not someone has implemented a workaround like my example in #860; the behavior will change slightly though.

The exception thrown is the same as previously and is handled by DRF already so I wouldn't say it's a breaking change either.

But of course the final decision is up to the maintainers - I'm just hoping the fix will land soon ;)