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

fix: Id token state not updated throughout the app #4706

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jorelosorio
Copy link
Member

@jorelosorio jorelosorio commented Sep 9, 2024

Closes:

https://github.com/AtB-AS/kundevendt/issues/18830

Authentication comes from Firebase & token ID comes from the mobile token SDK, so if the token is not set for an issue like no Wi-Fi but then Wi-Fi returns and updates the token ID and the user is in their profile, the app is not checking the token Id to be set, only checks for authentication, which leads into an issue that still shows the wrong auth.

@@ -103,7 +104,12 @@ function requestHandler(config: InternalAxiosRequestConfig): InternalAxiosReques
async function requestIdTokenHandler(config: InternalAxiosRequestConfig) {
if (config.authWithIdToken) {
const user = auth().currentUser;
const idToken = await user?.getIdToken(config.forceRefreshIdToken);
const tokenResult = await user?.getIdTokenResult(config.forceRefreshIdToken);
if (config.forceRefreshIdToken && tokenResult?.claims['customer_number']) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@gorandalum @rosvik I was looking at and testing this change, however, if I remove config.forceRefreshIdToken to test the logic of the timer, it works it forces a retryAuth every 10 seconds. But config.forceRefreshIdToken is in the condition and testing on staging, the issue seems like the retryAut is not called, it could be because the config.forceRefreshIdToken is never set to true because the response did not get a 401 as a response instead might get something different because no internet.

Copy link
Member

Choose a reason for hiding this comment

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

You need to use a proxy to introduce some 401 errors to test it appropriately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, working on testing it on different types of responses.

Copy link
Member

@rosvik rosvik left a comment

Choose a reason for hiding this comment

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

This looks like a good solution to me! Would be nice to have some monitoring ref. my comment bellow.

@@ -155,6 +160,13 @@ export const AuthContextProvider = ({children}: PropsWithChildren<{}>) => {
resubscribe();
}, [resubscribe]);

useInterval(() => {
if (SHOULD_FORCE_REFRESH.value) {
retryAuth();
Copy link
Member

Choose a reason for hiding this comment

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

Would it be a good idea to log to Bugsnag at this point? So we know how many retry attempts are triggered by the new logic. Could probably be a breadcrumb in the future, but the count might be interesting at least in the next release.

Copy link
Member

@gorandalum gorandalum left a comment

Choose a reason for hiding this comment

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

I started this locally and when using the test account with missing custom claims (+4794444444) it ends up refetching id token from server in an endless loop. I'm not really sure what or why it happens, but this can't be merged as it is now, as it would hit the Firebase Auth usage limits quickly.

I'm still not sure it can be solved in a clean manner using Axios interceptors, and making react-query be the handler of retries might be the way to go.

if (error.config && error.response?.status === 401) {
error.config.forceRefreshIdToken = true;
SHOULD_FORCE_REFRESH.value = true;
return client(error.config);
Copy link
Member

Choose a reason for hiding this comment

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

Not really sure about returning a client invocation here 🤔 Could you sum up some reasoning behind it?

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