-
Notifications
You must be signed in to change notification settings - Fork 551
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
feat: Implement JWT refresh token for authorization #2264
base: development
Are you sure you want to change the base?
Conversation
Will test and see |
@@ -66,7 +65,7 @@ class AuthService( | |||
|
|||
fun logout(): Completable { | |||
return Completable.fromAction { | |||
authHolder.token = null | |||
authHolder.accessToken = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove refresh token as well
} | ||
set(value) { | ||
if (value != null && JWTUtils.isExpired(value)) | ||
throw IllegalStateException("Cannot set expired refreshToken") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this will crash the app or what? User should be logged out in this case
if (headerType == JWT_REFRESH_TOKEN) | ||
authHolder.getRefreshAuthorization() | ||
else | ||
authHolder.getAccessAuthorization() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no, no. How does it refresh the token when it is expired?
See the answers here https://stackoverflow.com/questions/22450036/refreshing-oauth-token-using-retrofit-without-modifying-all-calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good approach. 👍
While injecting auth service in the authorization, the app is crashing with module definition. Any idea why we can inject auth holder but not auth service? |
Problem is how to do API call from the authenticator to refresh token, @iamareebjamal can I use view model in authenticator? |
See the comment in the same answer. There is a circular dependency |
Are you talking about this: https://stackoverflow.com/a/51360214
|
I meant don't use dependency injection there. Manually create a separate service |
I have a doubt, it needs to create a different HTTP client or using the same dependency injection module. |
Different HTTP client |
Status? |
If anyone isn't working i can I've already implemented the same in my project 😅 |
Go ahead |
@iamareebjamal i guess the flow will be like if i get 401 in response then i need to use the refresh token to get another jwt token and replace that token from the existing right? |
thanks |
the current mechanism uses an interceptor for attaching the jwt token should I make pr for that first to move it to authenticator ? |
Do it in one PR And, we have to use interceptor, or else, there'll be 2 requests for every unauthenticated resource. It was happening like this before, I changed it to Interceptor. We can have both authenticator and interceptor, but it may be more complicated for you and delay the PR. So, let's keep interceptor for now. Authenticator only triggers on API failure, meaning for any unauthenticated resource, it'll try things without token and obviously fail, and then trigger the authenticator, which'll attach the token and do the request. This'll make 2 API calls for each resource. Ideal case is using Interceptor + Authenticator, but your choice between Interceptor or A+I |
@iamareebjamal yeah I got figured it out that we have to use interceptor by default and authenticator for refreshing the token |
Fixes #2230