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

[ID-1178] keep login when silent login failed #1117

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

carmen0208
Copy link
Contributor

Summary

Customer Impact

Added

Changed

Deprecated

Removed

Fixed

Security

Things worth calling out

Before submitting the PR, please consider the following:

  • Prefix your PR title with feat: , fix: , chore: , docs:, or refactor:.

@carmen0208 carmen0208 marked this pull request as ready for review November 3, 2023 00:40
@carmen0208 carmen0208 requested a review from a team as a code owner November 3, 2023 00:40
Comment on lines +128 to +129
// eslint-disable-next-line no-console
console.warn('loginSilent failed with error:', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're expecting loginSilent to fail under certain circumstances, do we need this console.warn?

Copy link
Contributor Author

@carmen0208 carmen0208 Nov 3, 2023

Choose a reason for hiding this comment

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

Sorry, I wasn't aware of the situation that we expect loginSilent to fail. Can you make an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the purpose of this PR to handle situations in which loginSilent throws an exception? I'm guessing if your token has expired then it will throw an exception?

Copy link
Contributor Author

@carmen0208 carmen0208 Nov 6, 2023

Choose a reason for hiding this comment

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

Yes, if the fresh token is used and expired, we can escape it and throw "Unknown or invalid refresh token." error. But the problem is it doesn't always mean the token is expired. with expectation.
Because the token will expired if

  • Exceed very long period of time(around 1 year)
  • Or it is used before

But ideally, it should not used before because if it is used, we expect the other tokens have been updated.

So that's why I feel it is still better to shown then warning as to build the awareness

@carmen0208 carmen0208 merged commit 1b7606f into main Nov 6, 2023
6 checks passed
@carmen0208 carmen0208 deleted the fix/ID-1178-keep-login-when-silent-login-failed branch November 6, 2023 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants