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

Refresh accessToken when JWT access token has expired #1120

Merged
merged 9 commits into from
Aug 11, 2023

Conversation

mohammadranjbarz
Copy link
Collaborator

related #1007

@mohammadranjbarz
Copy link
Collaborator Author

@kristoferlund
I added refreshing accessToken with refreshToken when getting 401 error
It's just a working draft but would get happy if you leave your feedbacks for me

Comment on lines 41 to 42
delete recoilPersist!['ActiveTokenSet'];
localStorage.setItem(JSON.stringify(recoilPersist), 'recoil-persist');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kristoferlund
It seems we just can use recoil inside components so I had to manually put/get to local storage

Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for slow review @mohammadranjbarz!

I'd like to see two small changes, other than that all good!

Or btw. Have you tried how the user experience is when token expires and gets refreshed? Since the original request fails, the UI is left in an unclear state. Should we force a UI reload to make sure all looks good?

recoilPersist &&
err?.response?.status === 401 &&
// 1092, 1107 are the error codes for invalid jwt token that defined in backend
(err?.response?.data?.code === 1092 || err?.response?.data?.code === 1107)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1092 means unauthenticated user trying to access authenticated resource. This should not trigger a refresh.

}>
> => {
let refreshToken;
const recoilPersist = localStorage.getItem('recoil-persist');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use getRecoil(ActiveTokenSet), see auth.ts for example.

@kristoferlund
Copy link
Member

@mohammadranjbarz I finished this as I want to get it out to communities soon.

@kristoferlund kristoferlund self-requested a review August 11, 2023 09:00
kristoferlund
kristoferlund previously approved these changes Aug 11, 2023
@kristoferlund kristoferlund marked this pull request as ready for review August 11, 2023 09:01
@kristoferlund kristoferlund merged commit 51812c6 into main Aug 11, 2023
3 checks passed
@kristoferlund kristoferlund deleted the 1007_handle_refresh_token_in_frontend branch August 11, 2023 09:03
@mohammadranjbarz
Copy link
Collaborator Author

@mohammadranjbarz I finished this as I want to get it out to communities soon.

Thanks @kristoferlund , sorry I was busy last week , I couldn't spend time on this

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.

2 participants