From 271546de73cbbc8958ccd107a30bd5465ed01740 Mon Sep 17 00:00:00 2001 From: israx <70438514+israx@users.noreply.github.com> Date: Mon, 4 Dec 2023 11:00:11 -0500 Subject: [PATCH] fix(auth): dispatch signInWithRedirect error (#12653) * fix: dispatch signInWithRedirect failure event * chore: add unit tests * chore: bundle size test * chore: address feedback * chore: remove unknown type --- .../cognito/signInWithRedirect.test.ts | 318 +++++++++++++----- .../cognito/apis/signInWithRedirect.ts | 88 +++-- packages/aws-amplify/package.json | 2 +- packages/core/src/Hub/types/AuthTypes.ts | 11 +- 4 files changed, 293 insertions(+), 126 deletions(-) diff --git a/packages/auth/__tests__/providers/cognito/signInWithRedirect.test.ts b/packages/auth/__tests__/providers/cognito/signInWithRedirect.test.ts index 393854f3fee..da55fbc15f8 100644 --- a/packages/auth/__tests__/providers/cognito/signInWithRedirect.test.ts +++ b/packages/auth/__tests__/providers/cognito/signInWithRedirect.test.ts @@ -2,12 +2,49 @@ // SPDX-License-Identifier: Apache-2.0 import { AuthError } from '../../../src/errors/AuthError'; +import { Hub } from '@aws-amplify/core'; import { INVALID_ORIGIN_EXCEPTION, INVALID_REDIRECT_EXCEPTION, } from '../../../src/errors/constants'; import { getRedirectUrl } from '../../../src/providers/cognito/utils/oauth/getRedirectUrl'; import { getRedirectUrl as getRedirectUrlRN } from '../../../src/providers/cognito/utils/oauth/getRedirectUrl.native'; +import { + parseRedirectURL, + signInWithRedirect, +} from '../../../src/providers/cognito/apis/signInWithRedirect'; +import { openAuthSession } from '../../../src/utils'; +import { AMPLIFY_SYMBOL } from '@aws-amplify/core/internals/utils'; +jest.mock('../../../src/utils/openAuthSession'); +jest.mock('@aws-amplify/core', () => ({ + ...(jest.createMockFromModule('@aws-amplify/core') as object), + Amplify: { + getConfig: jest.fn(() => ({ + Auth: { + Cognito: { + userPoolClientId: '111111-aaaaa-42d8-891d-ee81a1549398', + userPoolId: 'us-west-2_zzzzz', + identityPoolId: 'us-west-2:xxxxxx', + loginWith: { + oauth: { + domain: 'my_cognito_domain', + redirectSignIn: ['http://localhost:3000/'], + redirectSignOut: ['http://localhost:3000/'], + responseType: 'code', + scopes: [ + 'email', + 'openid', + 'profile', + 'aws.cognito.signin.user.admin', + ], + }, + }, + }, + }, + })), + }, + Hub: { dispatch: jest.fn(), listen: jest.fn() }, +})); describe('signInWithRedirect API', () => { it('should pass correct arguments to oauth', () => { @@ -17,110 +54,205 @@ describe('signInWithRedirect API', () => { it('should try to clear oauth data before starting an oauth flow.', async () => { // TODO: ADD Test: previous test was invalid }); -}); -describe('getRedirectUrl on web', () => { - const originalWindowLocation = window.location; - - const currentWindownLocationParamsList: { - origin: string; - pathname: string; - }[] = [ - { origin: 'https://example.com', pathname: '/' }, - { origin: 'https://example.com', pathname: '/app' }, - { origin: 'https://example.com', pathname: '/app/page' }, - { origin: 'http://localhost:3000', pathname: '/' }, - { origin: 'http://localhost:3000', pathname: '/app' }, - ]; - afterEach(() => { - Object.defineProperty(globalThis, 'window', { - value: originalWindowLocation, + describe('signInWithRedirect API error cases', () => { + const oauthErrorMessage = 'an oauth error has occurred'; + const oauthError = new AuthError({ + name: 'OAuthSignInException', + message: oauthErrorMessage, }); - }); - it.each(currentWindownLocationParamsList)( - 'should pick the url that matches the current window', - async windowParams => { - const { origin, pathname } = windowParams; - Object.defineProperty(globalThis, 'window', { - value: { location: { origin, pathname } }, + const invalidStateOauthError = new AuthError({ + name: 'OAuthSignInException', + message: "An error occurred while validating the state", + }); + const mockOpenAuthSession = openAuthSession as jest.Mock; + const mockHubDispatch = Hub.dispatch as jest.Mock; + + afterEach(() => { + mockOpenAuthSession.mockReset(); + }); + + it('should throw and dispatch when an error is returned in the URL in RN', async () => { + mockOpenAuthSession.mockResolvedValueOnce({ + type: 'error', + error: oauthErrorMessage, + }); + + await expect(signInWithRedirect()).rejects.toThrow(oauthError); + expect(Hub.dispatch).toHaveBeenCalledWith( + 'auth', + { + event: 'signInWithRedirect_failure', + data: { error: oauthError }, + }, + 'Auth', + AMPLIFY_SYMBOL + ); + }); + + it('should throw when state is not valid after calling signInWithRedirect', async () => { + mockOpenAuthSession.mockResolvedValueOnce({ + type: 'success', + url: 'http:localhost:3000/oauth2/redirect?state=invalid_state&code=mock_code&scope=openid%20email%20profile&session_state=mock_session_state', + }); + + await expect(signInWithRedirect()).rejects.toThrow(invalidStateOauthError); + expect(mockHubDispatch).toHaveBeenCalledWith( + 'auth', + { + event: 'signInWithRedirect_failure', + data: { error: invalidStateOauthError }, + }, + 'Auth', + AMPLIFY_SYMBOL + ); + + }); + + it('should dispatch the signInWithRedirect_failure event when an error is returned in the URL', async () => { + Object.defineProperty(window, 'location', { + value: { + href: 'http:localhost:3000/oauth2/redirect?error=OAuthSignInException&error_description=an+oauth+error+has+occurred', + }, writable: true, }); - const redirectsFromConfig = [ - 'http://localhost:3000/', - 'https://example.com/', - 'https://example.com/app', - 'https://example.com/app/page', - 'http://localhost:3000/app', - ]; - const redirect = getRedirectUrl(redirectsFromConfig); - expect(redirect).toBe( - redirectsFromConfig.find(redir => redir === redirect) + await expect(parseRedirectURL).not.toThrow(); + expect(mockHubDispatch).toHaveBeenCalledWith( + 'auth', + { + event: 'signInWithRedirect_failure', + data: { error: oauthError }, + }, + 'Auth', + AMPLIFY_SYMBOL ); - } - ); - it('should pick the first url that is comming from a different pathname but same domain', async () => { - Object.defineProperty(globalThis, 'window', { - value: { - location: { - origin: 'https://example.com', - pathname: '/app', - hostname: 'example.com', + }); + + it('should dispatch the signInWithRedirect_failure event when state is not valid', async () => { + Object.defineProperty(window, 'location', { + value: { + href: `http:localhost:3000/oauth2/redirect?state='invalid_state'&code=mock_code&scope=openid%20email%20profile&session_state=mock_session_state`, }, - }, - writable: true, + writable: true, + }); + await expect(parseRedirectURL).not.toThrow(); + expect(mockHubDispatch).toHaveBeenCalledWith( + 'auth', + { + event: 'signInWithRedirect_failure', + data: { error: oauthError }, + }, + 'Auth', + AMPLIFY_SYMBOL + ); }); - const redirect = getRedirectUrl(['https://example.com/another-app']); - expect(redirect).toBe('https://example.com/another-app'); }); - - it('should throw if the url is not comming from the same origin', async () => { - Object.defineProperty(globalThis, 'window', { - value: { - location: { origin: 'https://differentorigin.com', pathname: '/app' }, - }, - writable: true, + + describe('getRedirectUrl on web', () => { + const originalWindowLocation = window.location; + + const currentWindownLocationParamsList: { + origin: string; + pathname: string; + }[] = [ + { origin: 'https://example.com', pathname: '/' }, + { origin: 'https://example.com', pathname: '/app' }, + { origin: 'https://example.com', pathname: '/app/page' }, + { origin: 'http://localhost:3000', pathname: '/' }, + { origin: 'http://localhost:3000', pathname: '/app' }, + ]; + afterEach(() => { + Object.defineProperty(globalThis, 'window', { + value: originalWindowLocation, + }); + }); + it.each(currentWindownLocationParamsList)( + 'should pick the url that matches the current window', + async windowParams => { + const { origin, pathname } = windowParams; + Object.defineProperty(globalThis, 'window', { + value: { location: { origin, pathname } }, + writable: true, + }); + const redirectsFromConfig = [ + 'http://localhost:3000/', + 'https://example.com/', + 'https://example.com/app', + 'https://example.com/app/page', + 'http://localhost:3000/app', + ]; + const redirect = getRedirectUrl(redirectsFromConfig); + expect(redirect).toBe( + redirectsFromConfig.find(redir => redir === redirect) + ); + } + ); + it('should pick the first url that is comming from a different pathname but same domain', async () => { + Object.defineProperty(globalThis, 'window', { + value: { + location: { + origin: 'https://example.com', + pathname: '/app', + hostname: 'example.com', + }, + }, + writable: true, + }); + const redirect = getRedirectUrl(['https://example.com/another-app']); + expect(redirect).toBe('https://example.com/another-app'); + }); + + it('should throw if the url is not comming from the same origin', async () => { + Object.defineProperty(globalThis, 'window', { + value: { + location: { origin: 'https://differentorigin.com', pathname: '/app' }, + }, + writable: true, + }); + + try { + return getRedirectUrl(['http://localhost:3000/', 'https://example.com/']); + } catch (error: any) { + expect(error).toBeInstanceOf(AuthError); + expect(error.name).toBe(INVALID_ORIGIN_EXCEPTION); + } + }); + + it('should throw if the url is not found or invalid', async () => { + Object.defineProperty(globalThis, 'window', { + value: { + location: { origin: 'http://localhost:3000', pathname: '/' }, + }, + writable: true, + }); + + try { + return getRedirectUrl(['novalid']); + } catch (error: any) { + expect(error).toBeInstanceOf(AuthError); + expect(error.name).toBe(INVALID_REDIRECT_EXCEPTION); + } }); - - try { - return getRedirectUrl(['http://localhost:3000/', 'https://example.com/']); - } catch (error: any) { - expect(error).toBeInstanceOf(AuthError); - expect(error.name).toBe(INVALID_ORIGIN_EXCEPTION); - } }); - - it('should throw if the url is not found or invalid', async () => { - Object.defineProperty(globalThis, 'window', { - value: { - location: { origin: 'http://localhost:3000', pathname: '/' }, - }, - writable: true, + + describe('getRedirectUrl on React Native', () => { + it('should pick the first non http or https redirect', async () => { + const redirect = getRedirectUrlRN([ + 'app:custom', + 'https://example.com/', + 'http://localhost:3000/', + ]); + expect(redirect).toBe('app:custom'); + }); + it('should throw if the redirect is invalid or not found', async () => { + try { + return getRedirectUrlRN(['invalid']); + } catch (error: any) { + expect(error).toBeInstanceOf(AuthError); + expect(error.name).toBe(INVALID_REDIRECT_EXCEPTION); + } }); - - try { - return getRedirectUrl(['novalid']); - } catch (error: any) { - expect(error).toBeInstanceOf(AuthError); - expect(error.name).toBe(INVALID_REDIRECT_EXCEPTION); - } }); }); -describe('getRedirectUrl on React Native', () => { - it('should pick the first non http or https redirect', async () => { - const redirect = getRedirectUrlRN([ - 'app:custom', - 'https://example.com/', - 'http://localhost:3000/', - ]); - expect(redirect).toBe('app:custom'); - }); - it('should throw if the redirect is invalid or not found', async () => { - try { - return getRedirectUrlRN(['invalid']); - } catch (error: any) { - expect(error).toBeInstanceOf(AuthError); - expect(error.name).toBe(INVALID_REDIRECT_EXCEPTION); - } - }); -}); + diff --git a/packages/auth/src/providers/cognito/apis/signInWithRedirect.ts b/packages/auth/src/providers/cognito/apis/signInWithRedirect.ts index 76f99752cf1..4794f860632 100644 --- a/packages/auth/src/providers/cognito/apis/signInWithRedirect.ts +++ b/packages/auth/src/providers/cognito/apis/signInWithRedirect.ts @@ -116,6 +116,8 @@ export async function oauthSignIn({ const { type, error, url } = (await openAuthSession(oAuthUrl, redirectSignIn, preferPrivateSession)) ?? {}; + // This code will run in RN applications only as calling signInWithRedirect will + // resolve the promise. if (type === 'success' && url) { // ensure the code exchange completion resolves the signInWithRedirect // returned promise in react-native @@ -129,8 +131,10 @@ export async function oauthSignIn({ preferPrivateSession, }); } + // This code will run in RN applications only as calling signInWithRedirect will + // resolve the promise. if (type === 'error') { - handleFailure(String(error)); + await handleFailure(String(error)); } } @@ -154,11 +158,13 @@ async function handleCodeFlow({ const url = new AmplifyUrl(currentUrl); let validatedState: string; try { - validatedState = await validateStateFromURL(url); + validatedState = await validateState(getStateFromURL(url)); } catch (err) { invokeAndClearPromise(); - // clear temp values - await store.clearOAuthInflightData(); + // validateState method will always throw an AuthError when the state is not valid. The if statement is making TS happy. + if (err instanceof AuthError) { + await handleFailure(err.message); + } return; } const code = url.searchParams.get('code'); @@ -197,6 +203,7 @@ async function handleCodeFlow({ refresh_token, id_token, error, + error_message, token_type, expires_in, } = await ( @@ -212,7 +219,7 @@ async function handleCodeFlow({ if (error) { invokeAndClearPromise(); - handleFailure(error); + await handleFailure(error_message ?? error); } await store.clearOAuthInflightData(); @@ -249,9 +256,15 @@ async function handleImplicitFlow({ const url = new AmplifyUrl(currentUrl); - const { id_token, access_token, state, token_type, expires_in } = ( - url.hash ?? '#' - ) + const { + id_token, + access_token, + state, + token_type, + expires_in, + error_description, + error, + } = (url.hash ?? '#') .substring(1) // Remove # from returned code .split('&') .map(pairings => pairings.split('=')) @@ -261,17 +274,29 @@ async function handleImplicitFlow({ state: undefined, token_type: undefined, expires_in: undefined, + error_description: undefined, + error: undefined, }); + + if (error) { + invokeAndClearPromise(); + await handleFailure(error_description ?? error); + } + if (!access_token) { await store.clearOAuthData(); invokeAndClearPromise(); return; } - + let validatedState; try { - validateState(state); + validatedState = await validateState(state); } catch (error) { invokeAndClearPromise(); + // validateState method will always throw an AuthError when the state is not valid. The if statement is making TS happy. + if (error instanceof AuthError) { + await handleFailure(error.message); + } return; } @@ -286,7 +311,11 @@ async function handleImplicitFlow({ ExpiresIn: expires_in, }); - return completeFlow({ redirectUri, state, preferPrivateSession }); + return completeFlow({ + redirectUri, + state: validatedState, + preferPrivateSession, + }); } async function completeFlow({ @@ -345,7 +374,7 @@ async function handleAuthResponse({ const errorMessage = urlParams.searchParams.get('error_description'); if (error) { - handleFailure(errorMessage); + await handleFailure(errorMessage); } if (responseType === 'code') { @@ -369,36 +398,35 @@ async function handleAuthResponse({ } } -async function validateStateFromURL(urlParams: URL): Promise { - if (!urlParams) { - } - const returnedState = urlParams.searchParams.get('state'); - - validateState(returnedState); - return returnedState; +function getStateFromURL(urlParams: URL): string | null { + return urlParams.searchParams.get('state'); } -function validateState(state?: string | null): asserts state { - let savedState: string | undefined | null; - - store.loadOAuthState().then(resp => { - savedState = resp; - }); +async function validateState(state?: string | null): Promise { + const savedState = await store.loadOAuthState(); // This is because savedState only exists if the flow was initiated by Amplify - if (savedState && state && savedState !== state) { + const validatedState = state === savedState ? savedState : undefined; + if (!validatedState) { throw new AuthError({ name: AuthErrorTypes.OAuthSignInError, message: 'An error occurred while validating the state', recoverySuggestion: 'Try to initiate an OAuth flow from Amplify', }); } + return validatedState; } -function handleFailure(errorMessage: string | null) { +async function handleFailure(errorMessage: string | null) { + const error = new AuthError({ + message: errorMessage ?? 'An error has occurred during the oauth proccess', + name: AuthErrorCodes.OAuthSignInError, + recoverySuggestion: authErrorMessages.oauthSignInError.log, + }); + await store.clearOAuthInflightData(); Hub.dispatch( 'auth', - { event: 'signInWithRedirect_failure' }, + { event: 'signInWithRedirect_failure', data: { error } }, 'Auth', AMPLIFY_SYMBOL ); @@ -409,7 +437,7 @@ function handleFailure(errorMessage: string | null) { }); } -async function parseRedirectURL() { +export async function parseRedirectURL() { const authConfig = Amplify.getConfig().Auth?.Cognito; try { assertTokenProviderConfig(authConfig); @@ -435,7 +463,7 @@ async function parseRedirectURL() { const { loginWith, userPoolClientId } = authConfig; const { domain, redirectSignIn, responseType } = loginWith.oauth; - handleAuthResponse({ + await handleAuthResponse({ currentUrl, clientId: userPoolClientId, domain, diff --git a/packages/aws-amplify/package.json b/packages/aws-amplify/package.json index b1d2a5de93c..6b6cb096b5a 100644 --- a/packages/aws-amplify/package.json +++ b/packages/aws-amplify/package.json @@ -445,7 +445,7 @@ "name": "[Auth] OAuth Auth Flow (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ signInWithRedirect, signOut, fetchAuthSession }", - "limit": "19.61 kB" + "limit": "19.68 kB" }, { "name": "[Storage] copy (S3)", diff --git a/packages/core/src/Hub/types/AuthTypes.ts b/packages/core/src/Hub/types/AuthTypes.ts index eb7533b1e2e..127acaf0f49 100644 --- a/packages/core/src/Hub/types/AuthTypes.ts +++ b/packages/core/src/Hub/types/AuthTypes.ts @@ -5,12 +5,19 @@ type AuthUser = { username: string; userId: string; }; - +type AuthError = { + name: string; + message: string; + recoverySuggestion?: string; +}; export type AuthHubEventData = /** Dispatched when a user signs in with an oauth provider such as Google.*/ | { event: 'signInWithRedirect' } /** Dispatched when there is an error in the oauth flow process.*/ - | { event: 'signInWithRedirect_failure' } + | { + event: 'signInWithRedirect_failure'; + data: { error?: AuthError }; + } /** Dispatched when auth tokens are successfully refreshed.*/ | { event: 'tokenRefresh' } /** Dispatched when there is an error in the refresh of tokens.*/