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(oauth): Move keyFetchToken and unwrapBKey to sensitiveDataClient #17994

Merged
merged 1 commit into from
Nov 8, 2024
Merged

fix(oauth): Move keyFetchToken and unwrapBKey to sensitiveDataClient #17994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 29 additions & 0 deletions packages/fxa-settings/src/lib/oauth/hooks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
OAuthIntegration,
isOAuthNativeIntegrationSync,
isOAuthIntegration,
isSyncDesktopV3Integration,
} from '../../models';
import { createEncryptedBundle } from '../crypto/scoped-keys';
import { Constants } from '../constants';
Expand Down Expand Up @@ -283,3 +284,31 @@ export function useFinishOAuthFlowHandler(
}
return { oAuthDataError: null, finishOAuthFlowHandler };
}

/**
* If we don't have `keyFetchToken` or `unwrapBKey` and it's required, the user needs to
* restart the flow. We only store these values in memory, so they don't persist across page
* refreshes or browser session restarts. We can't redirect to /signin and reprompt for
* password in a browser session restart/crash because the browser stores the flow state in
* memory and the query params won't match after a browser session is restored.
*
* TODO: Can we check session storage for if the user just refreshed so we can redirect them
* to /signin instead of showing an error component? FXA-10707
*/
export function useOAuthKeysCheck(
integration: Pick<Integration, 'type' | 'wantsKeys'>,
keyFetchToken?: hexstring,
unwrapBKey?: hexstring
) {
if (
(isOAuthIntegration(integration) ||
isSyncDesktopV3Integration(integration)) &&
integration.wantsKeys() &&
(!keyFetchToken || !unwrapBKey)
) {
return {
LZoog marked this conversation as resolved.
Show resolved Hide resolved
oAuthKeysCheckError: OAUTH_ERRORS.TRY_AGAIN,
};
}
return { oAuthKeysCheckError: null };
}
14 changes: 14 additions & 0 deletions packages/fxa-settings/src/lib/sensitive-data-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,20 @@

export const AUTH_DATA_KEY = 'auth';
LZoog marked this conversation as resolved.
Show resolved Hide resolved

export type SensitiveDataClientData = {
[AUTH_DATA_KEY]: {
emailForAuth?: string;
dschom marked this conversation as resolved.
Show resolved Hide resolved
authPW?: string;
keyFetchToken?: hexstring;
unwrapBKey?: hexstring;
};
};

export type SensitiveDataClientAuthKeys = Pick<
SensitiveDataClientData[typeof AUTH_DATA_KEY],
'keyFetchToken' | 'unwrapBKey'
>;

/**
* Class representing a client for handling sensitive data.
*
Expand Down
3 changes: 3 additions & 0 deletions packages/fxa-settings/src/models/integrations/oauth-native-integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ export function isOAuthNativeIntegration(integration: {

export type OAuthIntegration = OAuthWebIntegration | OAuthNativeIntegration;

/**
* Check if the integration is OAuthWeb or OAuthNative
*/
export function isOAuthIntegration(integration: {
type: IntegrationType;
}): integration is OAuthIntegration {
Expand Down
1 change: 1 addition & 0 deletions packages/fxa-settings/src/pages/InlineRecoveryKeySetup/container.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ function mockModelsModule() {
mockSensitiveDataClient.getData = jest.fn().mockReturnValue({
emailForAuth: '[email protected]',
authPW: MOCK_AUTH_PW,
unwrapBKey: MOCK_UNWRAP_BKEY,
});
}

Expand Down
21 changes: 14 additions & 7 deletions packages/fxa-settings/src/pages/InlineRecoveryKeySetup/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@ export const InlineRecoveryKeySetupContainer = (_: RouteComponentProps) => {
const location = useLocation() as ReturnType<typeof useLocation> & {
state?: SigninLocationState;
};
const { email, uid, sessionToken, unwrapBKey } = location.state || {};
const { email, uid, sessionToken } = location.state || {};

const sensitiveDataClient = useSensitiveDataClient();
const sensitiveData = sensitiveDataClient.getData(AUTH_DATA_KEY);
const { authPW, emailForAuth } =
(sensitiveData as unknown as { emailForAuth: string; authPW: string }) ||
{};
const { authPW, emailForAuth, unwrapBKey } =
(sensitiveData as {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reuse the sensitive data client type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm just going to punt this to that same follow up mentioned. There are files I didn't touch in this PR that are doing something similar to this as well and I think it makes sense to look at this in a non-dot release scenario.

emailForAuth?: string;
authPW?: string;
unwrapBKey?: hexstring;
}) || {};

const navigateForward = useCallback(() => {
setCurrentStep(currentStep + 1);
Expand All @@ -44,7 +47,9 @@ export const InlineRecoveryKeySetupContainer = (_: RouteComponentProps) => {
(
uid: string,
sessionToken: string,
unwrapBKey: string
unwrapBKey: string,
emailForAuth: string,
authPW: string
): (() => Promise<CreateRecoveryKeyHandler>) =>
async () => {
try {
Expand Down Expand Up @@ -93,7 +98,7 @@ export const InlineRecoveryKeySetupContainer = (_: RouteComponentProps) => {
};
}
},
[authClient, emailForAuth, authPW, navigateForward, ftlMsgResolver]
[authClient, navigateForward, ftlMsgResolver]
);

const updateRecoveryHint = useCallback(
Expand Down Expand Up @@ -124,7 +129,9 @@ export const InlineRecoveryKeySetupContainer = (_: RouteComponentProps) => {
const createRecoveryKeyHandler = createRecoveryKey(
uid,
sessionToken,
unwrapBKey
unwrapBKey,
emailForAuth,
authPW
);
const updateRecoveryHintHandler = updateRecoveryHint(sessionToken);

Expand Down
26 changes: 24 additions & 2 deletions packages/fxa-settings/src/pages/InlineRecoverySetup/container.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import { LocationProvider } from '@reach/router';
import { renderWithLocalizationProvider } from 'fxa-react/lib/test-utils/localizationProvider';
import { AuthUiError, AuthUiErrors } from '../../lib/auth-errors/auth-errors';
import { MozServices } from '../../lib/types';
import { OAuthIntegration } from '../../models';
import { OAuthIntegration, useSensitiveDataClient } from '../../models';
import { mockSensitiveDataClient as createMockSensitiveDataClient } from '../../models/mocks';
import {
MOCK_QUERY_PARAMS,
MOCK_SIGNIN_LOCATION_STATE,
Expand All @@ -27,7 +28,11 @@ import {
MOCK_OAUTH_FLOW_HANDLER_RESPONSE,
MOCK_TOTP_STATUS_VERIFIED,
} from '../Signin/mocks';
import { useFinishOAuthFlowHandler } from '../../lib/oauth/hooks';
import {
useFinishOAuthFlowHandler,
useOAuthKeysCheck,
} from '../../lib/oauth/hooks';
import { AUTH_DATA_KEY } from '../../lib/sensitive-data-client';

let mockLocationState = {};
const mockSearch = '?' + new URLSearchParams(MOCK_QUERY_PARAMS);
Expand All @@ -52,9 +57,12 @@ jest.mock('../../lib/oauth/hooks.tsx', () => {
return {
__esModule: true,
useFinishOAuthFlowHandler: jest.fn(),
useOAuthKeysCheck: jest.fn(),
};
});

const mockSensitiveDataClient = createMockSensitiveDataClient();
mockSensitiveDataClient.getData = jest.fn();
const mockAuthClient = new AuthClient('http://localhost:9000', {
keyStretchVersion: 1,
});
Expand All @@ -64,6 +72,7 @@ jest.mock('../../models', () => {
...jest.requireActual('../../models'),
useSession: jest.fn(() => mockSessionHook()),
useAuthClient: jest.fn(() => mockAuthClient),
useSensitiveDataClient: jest.fn(),
};
});

Expand Down Expand Up @@ -119,6 +128,12 @@ function setMocks() {
.mockReturnValueOnce(MOCK_OAUTH_FLOW_HANDLER_RESPONSE),
oAuthDataError: null,
}));
(useSensitiveDataClient as jest.Mock).mockImplementation(
() => mockSensitiveDataClient
);
(useOAuthKeysCheck as jest.Mock).mockImplementation(() => ({
oAuthKeysCheckError: null,
}));
}

const defaultProps = {
Expand Down Expand Up @@ -201,6 +216,13 @@ describe('InlineRecoverySetupContainer', () => {
});
});

it('reads data from sensitive data client', () => {
render();
expect(mockSensitiveDataClient.getData).toHaveBeenCalledWith(
AUTH_DATA_KEY
);
});

describe('callbacks', () => {
describe('cancelSetupHandler', () => {
it('redirects when returnOnError is true', async () => {
Expand Down
37 changes: 33 additions & 4 deletions packages/fxa-settings/src/pages/InlineRecoverySetup/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,15 @@ import { AuthUiErrors } from '../../lib/auth-errors/auth-errors';
import {
FinishOAuthFlowHandlerResult,
useFinishOAuthFlowHandler,
useOAuthKeysCheck,
} from '../../lib/oauth/hooks';
import { getCode } from '../../lib/totp';
import { MozServices } from '../../lib/types';
import { OAuthIntegration, useAuthClient } from '../../models';
import {
OAuthIntegration,
useAuthClient,
useSensitiveDataClient,
} from '../../models';
import { VERIFY_TOTP_MUTATION } from './gql';
import InlineRecoverySetup from './index';
import { hardNavigate } from 'fxa-react/lib/utils';
Expand All @@ -23,6 +28,10 @@ import { TotpStatusResponse } from '../Signin/SigninTokenCode/interfaces';
import { GET_TOTP_STATUS } from '../../components/App/gql';
import OAuthDataError from '../../components/OAuthDataError';
import { isFirefoxService } from '../../models/integrations/utils';
import {
AUTH_DATA_KEY,
SensitiveDataClientAuthKeys,
} from '../../lib/sensitive-data-client';

export const InlineRecoverySetupContainer = ({
isSignedIn,
Expand All @@ -46,6 +55,16 @@ export const InlineRecoverySetupContainer = ({
};
const signinRecoveryLocationState = location.state;
const { totp, ...signinLocationState } = signinRecoveryLocationState || {};
const sensitiveDataClient = useSensitiveDataClient();
const sensitiveData = sensitiveDataClient.getData(AUTH_DATA_KEY);
const { keyFetchToken, unwrapBKey } =
(sensitiveData as SensitiveDataClientAuthKeys) || {};

const { oAuthKeysCheckError } = useOAuthKeysCheck(
integration,
keyFetchToken,
unwrapBKey
);

const [recoveryCodes, setRecoveryCodes] = useState<string[]>();
const [verifyTotp] = useMutation<{ verifyTotp: { success: boolean } }>(
Expand Down Expand Up @@ -78,15 +97,20 @@ export const InlineRecoverySetupContainer = ({
const { redirect, error } = await finishOAuthFlowHandler(
signinRecoveryLocationState!.uid,
signinRecoveryLocationState!.sessionToken,
signinRecoveryLocationState!.keyFetchToken,
signinRecoveryLocationState!.unwrapBKey
keyFetchToken,
unwrapBKey
);
if (error) {
setOAuthError(error);
return;
}
hardNavigate(redirect);
}, [signinRecoveryLocationState, finishOAuthFlowHandler]);
}, [
signinRecoveryLocationState,
finishOAuthFlowHandler,
keyFetchToken,
unwrapBKey,
]);

const cancelSetupHandler = useCallback(() => {
const error = AuthUiErrors.TOTP_REQUIRED;
Expand Down Expand Up @@ -125,6 +149,11 @@ export const InlineRecoverySetupContainer = ({
if (oAuthDataError) {
return <OAuthDataError error={oAuthDataError} />;
}
// Note that we don't currently need this check on this page right now since AMO is the only
// RP requiring 2FA and it doesn't require keys. However it's here for consistency.
Copy link
Contributor

Choose a reason for hiding this comment

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

Very much appreciate the comments you've sprinkled throughout 🫶

if (oAuthKeysCheckError) {
return <OAuthDataError error={oAuthKeysCheckError} />;
}

return (
<InlineRecoverySetup
Expand Down
24 changes: 22 additions & 2 deletions packages/fxa-settings/src/pages/Signin/SigninPushCode/container.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,22 @@ import { SigninPushCodeProps } from './interfaces';
import * as ReactUtils from 'fxa-react/lib/utils';
import * as CacheModule from '../../../lib/cache';

import { Integration } from '../../../models';
import { Integration, useSensitiveDataClient } from '../../../models';
import { renderWithLocalizationProvider } from 'fxa-react/lib/test-utils/localizationProvider';
import { LocationProvider } from '@reach/router';
import SigninPushCodeContainer from './container';
import { waitFor } from '@testing-library/react';
import { MOCK_EMAIL, MOCK_STORED_ACCOUNT } from '../../mocks';
import {
MOCK_EMAIL,
MOCK_KEY_FETCH_TOKEN,
MOCK_STORED_ACCOUNT,
MOCK_UNWRAP_BKEY,
} from '../../mocks';
import {
createMockSigninLocationState,
createMockSyncIntegration,
} from './mocks';
import { mockSensitiveDataClient as createMockSensitiveDataClient } from '../../../models/mocks';

import { MozServices } from '../../../lib/types';

Expand All @@ -35,8 +41,11 @@ function applyDefaultMocks() {

mockSigninPushCodeModule();
mockCurrentAccount();
resetMockSensitiveDataClient();
}

const mockSensitiveDataClient = createMockSensitiveDataClient();
mockSensitiveDataClient.setData = jest.fn();
let mockHasTotpAuthClient = false;
let mockSessionStatus = 'verified';
let mockSendLoginPushRequest = jest.fn().mockResolvedValue({});
Expand All @@ -54,6 +63,7 @@ jest.mock('../../../models', () => {
sendLoginPushRequest: mockSendLoginPushRequest,
};
},
useSensitiveDataClient: jest.fn(),
};
});

Expand Down Expand Up @@ -96,6 +106,16 @@ function mockCurrentAccount(storedAccount = { uid: '123' }) {
jest.spyOn(CacheModule, 'currentAccount').mockReturnValue(storedAccount);
}

function resetMockSensitiveDataClient() {
(useSensitiveDataClient as jest.Mock).mockImplementation(
() => mockSensitiveDataClient
);
mockSensitiveDataClient.getData = jest.fn().mockReturnValue({
keyFetchToken: MOCK_KEY_FETCH_TOKEN,
unwrapBKey: MOCK_UNWRAP_BKEY,
});
}

async function render() {
renderWithLocalizationProvider(
<LocationProvider>
Expand Down
17 changes: 14 additions & 3 deletions packages/fxa-settings/src/pages/Signin/SigninPushCode/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,23 @@ import SigninPushCode from '.';
import { MozServices } from '../../../lib/types';
import { getSigninState, handleNavigation } from '../utils';
import { SigninLocationState } from '../interfaces';
import { Integration, isWebIntegration, useAuthClient } from '../../../models';
import {
Integration,
isWebIntegration,
useAuthClient,
useSensitiveDataClient,
} from '../../../models';
import { useFinishOAuthFlowHandler } from '../../../lib/oauth/hooks';
import { hardNavigate } from 'fxa-react/lib/utils';
import LoadingSpinner from 'fxa-react/components/LoadingSpinner';
import OAuthDataError from '../../../components/OAuthDataError';
import { useWebRedirect } from '../../../lib/hooks/useWebRedirect';
import { useEffect, useState } from 'react';
import { useNavigateWithQuery as useNavigate } from '../../../lib/hooks/useNavigateWithQuery';
import {
AUTH_DATA_KEY,
SensitiveDataClientAuthKeys,
} from '../../../lib/sensitive-data-client';

export type SigninPushCodeContainerProps = {
integration: Integration;
Expand All @@ -35,8 +44,10 @@ export const SigninPushCodeContainer = ({
const location = useLocation() as ReturnType<typeof useLocation> & {
state: SigninLocationState;
};

const signinState = getSigninState(location.state);
const sensitiveDataClient = useSensitiveDataClient();
const sensitiveData = sensitiveDataClient.getData(AUTH_DATA_KEY);
const { unwrapBKey } = (sensitiveData as SensitiveDataClientAuthKeys) || {};

const webRedirectCheck = useWebRedirect(integration.data.redirectTo);

Expand Down Expand Up @@ -82,7 +93,7 @@ export const SigninPushCodeContainer = ({
const navigationOptions = {
email: signinState.email,
signinData: { ...signinState, verified: true },
unwrapBKey: signinState.unwrapBKey,
unwrapBKey,
integration,
finishOAuthFlowHandler,
queryParams: location.search,
Expand Down
Loading