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(auth): improve error handling while refreshing auth tokens #12802

Merged
merged 9 commits into from
Jan 8, 2024

Conversation

israx
Copy link
Contributor

@israx israx commented Jan 5, 2024

Description of changes

Main changes:

  • improves error handling when a user wants to refresh a session while authenticated with implicit grant oauth flow.
  • dispatches a tokenRefresh_failure event when the refresh_token is expired.

Issue #, if available

Description of how you validated changes

  • add unit tests
  • smoked test- changed the expiration date of the refresh_token in the Cognito console and was able to see that the refreshToken_failure event was dispatched

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@israx israx requested review from a team as code owners January 5, 2024 20:20
@israx israx requested a review from a team as a code owner January 5, 2024 20:33
packages/auth/src/errors/constants.ts Outdated Show resolved Hide resolved
packages/auth/src/providers/cognito/utils/types.ts Outdated Show resolved Hide resolved
});
});

describe('negative cases', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In all the test cases, should we checking for the failure hub event to be dispatched as well along with the throwing of the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the refreshToken doesn't dispatches hub events. Probably we can test that use case in the token orchestrator

ashika112
ashika112 previously approved these changes Jan 5, 2024
Copy link
Contributor

@ashika112 ashika112 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -384,7 +384,7 @@
"name": "[Auth] confirmSignIn (Cognito)",
"path": "./dist/esm/auth/index.mjs",
"import": "{ confirmSignIn }",
"limit": "25.70 kB"
"limit": "25.89 kB"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why are we increasing the limits in this PR?

Copy link
Contributor Author

@israx israx Jan 8, 2024

Choose a reason for hiding this comment

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

some APIs depend on fetchAuthSession and adding additional code to that API might result in a bundle size bump

jimblanc
jimblanc previously approved these changes Jan 8, 2024
Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

lgtm, one nit

packages/auth/src/providers/cognito/utils/types.ts Outdated Show resolved Hide resolved
Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>
@israx israx merged commit d0911f5 into aws-amplify:main Jan 8, 2024
30 checks passed
@@ -113,3 +127,16 @@ export interface OAuthStore {
clearOAuthInflightData(): Promise<void>;
clearOAuthData(): Promise<void>;
}
function isAuthenticated(tokens?: CognitoAuthTokens | null) {
return tokens?.accessToken && tokens?.idToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ||

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.

None yet

5 participants