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.

Sign up for GitHub

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

Jump to bottom

fix: ID-2650 Handle unknown or invalid refresh token error #2399

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: ID-2650 Handle unknown or invalid refresh token error #2399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d6f381
ID-2650 Handle refresh token error
imx-mikhala Nov 14, 2024
c27110f
Merge branch 'main' into ID-2650
imx-mikhala Nov 14, 2024
806f470
Handle error in forceUserRefresh
imx-mikhala Nov 15, 2024
2c059d2
fix
imx-mikhala Nov 15, 2024
bb83e34
Merge branch 'main' into ID-2650
imx-mikhala Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/passport/sdk/src/Passport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,8 @@ export class Passport {
try {
user = await this.authManager.getUser();
} catch (error) {
if (error instanceof Error) {
trackError('passport', 'login', error);
}
if (useCachedSession) {
if (error instanceof Error) trackError('passport', 'login', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think where the trackError call was before made sense, but I think the right solution is to no longer surface unknown or invalid refresh token "errors" as errors. Maybe something like this:

} catch (error) {
  // pseudo
  if (errorIsUnknownOrInvalidRefreshToken) {
    flow.addEvent('unknown or invalid refresh token');
  } else {
    trackError('passport', 'login', error);
  }

Copy link
Contributor Author

@imx-mikhala imx-mikhala Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We only throw in the useCachedSession scenario which is why I moved the track here - do you think we should track other errors caught here even if they do not throw and just ignore any related to the invalid refresh? I suppose this would highlight any errors thrown here that we might not be handling so I think that makes sense.

throw error;
}
logger.warn('Failed to retrieve a cached user session', error);
Expand Down
5 changes: 4 additions & 1 deletion packages/passport/sdk/src/authManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,10 @@ export default class AuthManager {
}

public async forceUserRefresh(): Promise<User | null> {
return this.refreshTokenAndUpdatePromise();
return this.refreshTokenAndUpdatePromise().catch((error) => {
logger.warn('Failed to refresh user token', error);
return null;
});
}

/**
Expand Down
4 changes: 3 additions & 1 deletion packages/passport/sdk/src/zkEvm/zkEvmProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,9 @@ export class ZkEvmProvider implements Provider {
this.#callSessionActivity(user.zkEvm.ethAddress);
}
}
}).catch(); // User does not exist, don't initialise an eth signer
}).catch(() => {
// User does not exist, don't initialise an eth signer
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was where the error was not being caught - catch requires a function to handle the error


passportEventEmitter.on(PassportEvents.LOGGED_IN, (user: User) => {
this.#initialiseEthSigner(user);
Expand Down