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): dispatch signInWithRedirect error #12653

Merged
merged 6 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
141 changes: 141 additions & 0 deletions packages/auth/__tests__/providers/cognito/signInWithRedirect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -19,6 +56,110 @@ describe('signInWithRedirect API', () => {
});
});

describe('signInWithRedirect API error cases', () => {
israx marked this conversation as resolved.
Show resolved Hide resolved
const oauthErrorMessage = 'an oauth error has occurred';
const oauthError = new AuthError({
name: 'OAuthSignInException',
message: oauthErrorMessage,
});
const mockOpenAuthSession = openAuthSession as jest.Mock;
israx marked this conversation as resolved.
Show resolved Hide resolved
israx marked this conversation as resolved.
Show resolved Hide resolved

it('should throw and dispatch when an error is returned in the URL in RN', async () => {
mockOpenAuthSession.mockResolvedValueOnce({
type: 'error',
error: oauthErrorMessage,
});

try {
await signInWithRedirect();
expect(Hub.dispatch).toHaveBeenCalledWith(
'auth',
{
event: 'signInWithRedirect_failure',
data: {
error: oauthError,
},
},
'Auth',
AMPLIFY_SYMBOL
);
} catch (error: any) {
expect(error).toBeInstanceOf(AuthError);
expect(error.name).toBe(oauthError.name);
}
israx marked this conversation as resolved.
Show resolved Hide resolved
});

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',
});
try {
israx marked this conversation as resolved.
Show resolved Hide resolved
await signInWithRedirect();
expect(Hub.dispatch).toHaveBeenCalledWith(
'auth',
{
event: 'signInWithRedirect_failure',
data: {
error: oauthError,
},
},
israx marked this conversation as resolved.
Show resolved Hide resolved
'Auth',
AMPLIFY_SYMBOL
);
} catch (error: any) {
expect(error).toBeInstanceOf(AuthError);
expect(error.message).toBe(
'An error occurred while validating the state'
);
}
});

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,
});
expect(parseRedirectURL).not.toThrow();
await parseRedirectURL();
expect(Hub.dispatch).toHaveBeenCalledWith(
israx marked this conversation as resolved.
Show resolved Hide resolved
'auth',
{
event: 'signInWithRedirect_failure',
data: {
error: oauthError,
},
},
israx marked this conversation as resolved.
Show resolved Hide resolved
'Auth',
AMPLIFY_SYMBOL
);
});

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,
});
expect(parseRedirectURL).not.toThrow();
await parseRedirectURL();
expect(Hub.dispatch).toHaveBeenCalledWith(
israx marked this conversation as resolved.
Show resolved Hide resolved
'auth',
{
event: 'signInWithRedirect_failure',
data: {
error: oauthError,
},
},
'Auth',
AMPLIFY_SYMBOL
);
});
});

describe('getRedirectUrl on web', () => {
const originalWindowLocation = window.location;

Expand Down
83 changes: 53 additions & 30 deletions packages/auth/src/providers/cognito/apis/signInWithRedirect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export async function oauthSignIn({
});
}
if (type === 'error') {
handleFailure(String(error));
await handleFailure(String(error));
israx marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -154,11 +154,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();

if (err instanceof AuthError) {
israx marked this conversation as resolved.
Show resolved Hide resolved
await handleFailure(err.message);
}
return;
}
const code = url.searchParams.get('code');
Expand Down Expand Up @@ -197,6 +199,7 @@ async function handleCodeFlow({
refresh_token,
id_token,
error,
error_message,
token_type,
expires_in,
} = await (
Expand All @@ -212,7 +215,7 @@ async function handleCodeFlow({

if (error) {
invokeAndClearPromise();
handleFailure(error);
await handleFailure(error_message ?? error);
}

await store.clearOAuthInflightData();
Expand Down Expand Up @@ -249,9 +252,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('='))
Expand All @@ -261,17 +270,28 @@ 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();
if (error instanceof AuthError) {
await handleFailure(error.message);
}
return;
}

Expand All @@ -286,7 +306,11 @@ async function handleImplicitFlow({
ExpiresIn: expires_in,
});

return completeFlow({ redirectUri, state, preferPrivateSession });
return completeFlow({
redirectUri,
state: validatedState,
preferPrivateSession,
});
}

async function completeFlow({
Expand Down Expand Up @@ -345,7 +369,7 @@ async function handleAuthResponse({
const errorMessage = urlParams.searchParams.get('error_description');

if (error) {
handleFailure(errorMessage);
await handleFailure(errorMessage);
}

if (responseType === 'code') {
Expand All @@ -369,36 +393,35 @@ async function handleAuthResponse({
}
}

async function validateStateFromURL(urlParams: URL): Promise<string> {
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<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since this is now actually returning something, I would opt for a name that better reflects the new behavior. E.g. getValidatedState

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

});
await store.clearOAuthInflightData();
Hub.dispatch(
'auth',
{ event: 'signInWithRedirect_failure' },
{ event: 'signInWithRedirect_failure', data: { error } },
'Auth',
AMPLIFY_SYMBOL
);
Expand All @@ -409,7 +432,7 @@ function handleFailure(errorMessage: string | null) {
});
}

async function parseRedirectURL() {
export async function parseRedirectURL() {
elorzafe marked this conversation as resolved.
Show resolved Hide resolved
const authConfig = Amplify.getConfig().Auth?.Cognito;
try {
assertTokenProviderConfig(authConfig);
Expand All @@ -435,7 +458,7 @@ async function parseRedirectURL() {
const { loginWith, userPoolClientId } = authConfig;
const { domain, redirectSignIn, responseType } = loginWith.oauth;

handleAuthResponse({
await handleAuthResponse({
israx marked this conversation as resolved.
Show resolved Hide resolved
currentUrl,
clientId: userPoolClientId,
domain,
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-amplify/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down
11 changes: 9 additions & 2 deletions packages/core/src/Hub/types/AuthTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 | unknown };
}
/** Dispatched when auth tokens are successfully refreshed.*/
| { event: 'tokenRefresh' }
/** Dispatched when there is an error in the refresh of tokens.*/
Expand Down
Loading