From 3873936aa355f21aa6029481bb5d122b5f69201e Mon Sep 17 00:00:00 2001 From: ieow <4881057+ieow@users.noreply.github.com> Date: Tue, 18 Nov 2025 13:13:04 +0800 Subject: [PATCH 1/2] fix: add delay before revoke tokens (#22769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** app is using revoked tokens to refresh token. That should not be the case, as we only revoke token after successful renew refresh token. We are speculating that user might closed the app before the redux state are persisted on the files This PR add a delay of 15 seconds after successful renew refresh token before call revoke token Jira Link https://consensyssoftware.atlassian.net/browse/SL-297 ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- > [!NOTE] > Wait 15s after renewing the Seedless refresh token before revoking pending tokens to ensure persistence. > > - **OAuth service** (`app/core/OAuthService/SeedlessControllerHelper.ts`): > - Add `delay` utility. > - Update `renewSeedlessControllerRefreshTokens` to wait 15s after `renewRefreshToken` before calling `revokePendingRefreshTokens`. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit c1effb311ee8ba44a9d4d8af667de3e123987bbd. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- app/core/OAuthService/SeedlessControllerHelper.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/core/OAuthService/SeedlessControllerHelper.ts b/app/core/OAuthService/SeedlessControllerHelper.ts index c0616aeda403..1baa60e2e79a 100644 --- a/app/core/OAuthService/SeedlessControllerHelper.ts +++ b/app/core/OAuthService/SeedlessControllerHelper.ts @@ -1,9 +1,20 @@ import Engine from '../Engine'; +// delay function +const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Renews the refresh tokens for the seedless onboarding controller and revokes the pending revoke tokens. + * @param password - The password to use to unlock seedless controller in order to renew the refresh tokens. + * @returns A promise that resolves when the refresh tokens have been renewed. + */ export const renewSeedlessControllerRefreshTokens = async ( password: string, ) => { const { SeedlessOnboardingController } = Engine.context; await SeedlessOnboardingController.renewRefreshToken(password); + + // delay to allow new refresh token to be persisted + await delay(15_000); await SeedlessOnboardingController.revokePendingRefreshTokens(); }; From ce11468607dcee798b4c962b35d1ba33eab89b9f Mon Sep 17 00:00:00 2001 From: ieow <4881057+ieow@users.noreply.github.com> Date: Tue, 18 Nov 2025 13:16:42 +0800 Subject: [PATCH 2/2] chore: update seedless controller 6.1.0 (#21991) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Update seedless controller to v6.1.0 Update required types checking ( throw on not matching type / undefined ) Remove unused code in basehandler add jsdoc to AuthTokenHandler Jira Link https://consensyssoftware.atlassian.net/browse/SL-253?atlOrigin=eyJpIjoiNzc2ZjgxYTRlZGEzNGFhY2I4ZWZjNDc3ZjExNzg4NzkiLCJwIjoiaiJ9 ## **Changelog** CHANGELOG entry: update seedless controller v6 ## **Related issues** Fixes: https://github.com/MetaMask/metamask-mobile/issues/21933 ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- > [!NOTE] > Upgrades seedless controller to v6.1.0 and updates OAuth services to stricter token typing/validation, new v2 revoke/renew endpoints, and refreshed tests. > > - **OAuth services**: > - **AuthTokenHandler**: > - Implements controller types; validates presence of `id_token`, `access_token`, `metadata_access_token`. > - Adds strict checks for `refresh_token`/`revoke_token` on renew; throws on missing tokens. > - Switches endpoints: `AUTH_SERVER_RENEW_PATH` and `AUTH_SERVER_REVOKE_PATH` to `/api/v2/...`. > - **OAuthService**: > - Enforces non-empty `loginHandler.login()` result. > - Requires `refresh_token` and `revoke_token` before `authenticate`; throws controller errors if missing. > - **Base handler**: > - Removes refresh/revoke helpers; keeps `getAuthTokens` with explicit typing. > - **Interfaces**: > - Adds `AuthRefreshTokenResponse`; documents response types. > - **Tests**: > - Updates and expands unit tests for new validations, error handling, request bodies, and handler behavior across `AuthTokenHandler`, base handler, login handlers, and service. > - **Dependencies**: > - Bumps `@metamask/seedless-onboarding-controller` to `^6.1.0` and related transitive packages. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5b829409f809cfb8ea31ef1940c6fa501e1bce49. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../OAuthService/AuthTokenHandler.test.ts | 17 +++-- app/core/OAuthService/AuthTokenHandler.ts | 54 ++++++++++++-- app/core/OAuthService/OAuthInterface.ts | 13 ++++ .../OAuthLoginHandlers/baseHandler.test.ts | 53 +++++-------- .../OAuthLoginHandlers/baseHandler.ts | 74 +------------------ .../OAuthLoginHandlers/index.test.ts | 42 ++++++----- app/core/OAuthService/OAuthService.test.ts | 71 ++++++++++-------- app/core/OAuthService/OAuthService.ts | 39 +++++++++- package.json | 2 +- yarn.lock | 53 ++++++++----- 10 files changed, 225 insertions(+), 193 deletions(-) diff --git a/app/core/OAuthService/AuthTokenHandler.test.ts b/app/core/OAuthService/AuthTokenHandler.test.ts index 563ff05610cc..25bb079ae2f9 100644 --- a/app/core/OAuthService/AuthTokenHandler.test.ts +++ b/app/core/OAuthService/AuthTokenHandler.test.ts @@ -292,20 +292,18 @@ describe('AuthTokenHandler', () => { fetchSpy.mockResolvedValueOnce({ ok: true, + statusText: 'OK', json: jest.fn().mockResolvedValueOnce(mockResponse), }); // Act - const result = await AuthTokenHandler.renewRefreshToken({ + const pendingPromise = AuthTokenHandler.renewRefreshToken({ connection: mockConnection, revokeToken: mockRevokeToken, }); // Assert - expect(result).toEqual({ - newRefreshToken: undefined, - newRevokeToken: undefined, - }); + await expect(pendingPromise).rejects.toThrow(); }); }); @@ -503,6 +501,15 @@ describe('AuthTokenHandler', () => { refreshToken: 'test-token', }); + const refreshMockResponse = { + refresh_token: 'new-refresh-token', + revoke_token: 'new-revoke-token', + }; + fetchSpy.mockResolvedValue({ + ok: true, + json: jest.fn().mockResolvedValue(refreshMockResponse), + }); + await AuthTokenHandler.renewRefreshToken({ connection: AuthConnection.Apple, revokeToken: 'test-token', diff --git a/app/core/OAuthService/AuthTokenHandler.ts b/app/core/OAuthService/AuthTokenHandler.ts index e4b97ba19163..f8430f74a142 100644 --- a/app/core/OAuthService/AuthTokenHandler.ts +++ b/app/core/OAuthService/AuthTokenHandler.ts @@ -1,12 +1,31 @@ import { Platform } from 'react-native'; -import { AuthConnection } from './OAuthInterface'; +import { AuthConnection, AuthRefreshTokenResponse } from './OAuthInterface'; import { createLoginHandler } from './OAuthLoginHandlers'; +import type { + RefreshJWTToken, + RenewRefreshToken, + RevokeRefreshToken, +} from '@metamask/seedless-onboarding-controller/dist/types.d.cts'; export const AUTH_SERVER_RENEW_PATH = '/api/v2/oauth/renew_refresh_token'; export const AUTH_SERVER_REVOKE_PATH = '/api/v2/oauth/revoke'; export const AUTH_SERVER_TOKEN_PATH = '/api/v1/oauth/token'; -class AuthTokenHandler { +interface AuthTokenHandlerInterface { + refreshJWTToken: RefreshJWTToken; + renewRefreshToken: RenewRefreshToken; + revokeRefreshToken: RevokeRefreshToken; +} + +class AuthTokenHandler implements AuthTokenHandlerInterface { + /** + * Refresh the JWT Token using the refresh token. + * + * @param params - The params from the login handler + * @param params.connection - The connection type (Google, Apple, etc.) + * @param params.refreshToken - The refresh token from the Web3Auth Authentication Server. + * @returns The id token, access token, and metadata access token. + */ async refreshJWTToken(params: { connection: AuthConnection; refreshToken: string; @@ -41,7 +60,7 @@ class AuthTokenHandler { throw new Error('Failed to refresh JWT token'); } - const refreshTokenData = await response.json(); + const refreshTokenData: AuthRefreshTokenResponse = await response.json(); const idToken = refreshTokenData.id_token; if ( @@ -62,6 +81,14 @@ class AuthTokenHandler { }; } + /** + * Renew the refresh token. + * + * @param params - The params from the login handler + * @param params.connection - The connection type (Google, Apple, etc.) + * @param params.revokeToken - The revoke token from the Web3Auth Authentication Server. + * @returns The new refresh token and revoke token. + */ async renewRefreshToken(params: { connection: AuthConnection; revokeToken: string; @@ -89,12 +116,28 @@ class AuthTokenHandler { } const responseData = await response.json(); + + const newRefreshToken = responseData.refresh_token; + const newRevokeToken = responseData.revoke_token; + + if (!newRefreshToken || !newRevokeToken) { + throw new Error('Failed to renew refresh token - ' + response.statusText); + } + return { - newRefreshToken: responseData.refresh_token, - newRevokeToken: responseData.revoke_token, + newRefreshToken, + newRevokeToken, }; } + /** + * Revoke the refresh token. + * + * @param params - The params from the login handler + * @param params.connection - The connection type (Google, Apple, etc.) + * @param params.revokeToken - The revoke token from the Web3Auth Authentication Server. + * @returns void + */ async revokeRefreshToken(params: { connection: AuthConnection; revokeToken: string; @@ -122,7 +165,6 @@ class AuthTokenHandler { 'Failed to revoke refresh token - ' + response.statusText, ); } - return; } } diff --git a/app/core/OAuthService/OAuthInterface.ts b/app/core/OAuthService/OAuthInterface.ts index 4d6db5750aa6..cd93fba50512 100644 --- a/app/core/OAuthService/OAuthInterface.ts +++ b/app/core/OAuthService/OAuthInterface.ts @@ -68,6 +68,8 @@ export type AuthRequestParams = | AuthRequestCodeParams | AuthRequestIdTokenParams; +// return type for auth request with +// grant type : authorization_code, access_type: offline export interface AuthResponse { id_token: string; access_token: string; @@ -78,6 +80,17 @@ export interface AuthResponse { revoke_token?: string; } +// return type for auth request with +// grant type : refresh_token +// grant type : authorization_code, access_type: online +export interface AuthRefreshTokenResponse { + id_token: string; + access_token: string; + metadata_access_token: string; + indexes: number[]; + endpoints: Record; +} + export interface LoginHandler { get authConnection(): AuthConnection; get scope(): string[]; diff --git a/app/core/OAuthService/OAuthLoginHandlers/baseHandler.test.ts b/app/core/OAuthService/OAuthLoginHandlers/baseHandler.test.ts index eebef0267dd1..b01493640123 100644 --- a/app/core/OAuthService/OAuthLoginHandlers/baseHandler.test.ts +++ b/app/core/OAuthService/OAuthLoginHandlers/baseHandler.test.ts @@ -183,10 +183,11 @@ describe('BaseLoginHandler', () => { success: true, id_token: 'mock-id-token', refresh_token: 'mock-refresh-token', - indexes: [1, 2, 3], - endpoints: { endpoint1: 'value1' }, - message: 'Success', - jwt_tokens: { token1: 'value1' }, + revoke_token: 'mock-revoke-token', + access_token: 'mock-access-token', + metadata_access_token: 'mock-metadata-access-token', + token_type: 'Bearer', + expires_in: 3600, }; (global.fetch as jest.Mock).mockResolvedValueOnce({ @@ -225,44 +226,16 @@ describe('BaseLoginHandler', () => { ); expect(result).toEqual(mockResponse); - - jest - .spyOn(global, 'fetch') - .mockResolvedValueOnce(new Response(JSON.stringify(mockResponse))); - - const refreshResult = await mockHandler.refreshAuthToken('refresh-token'); - - expect(refreshResult).toEqual(mockResponse); - - const mockRevokeResponse = { - new_refresh_token: 'refresh-token', - new_revoke_token: 'revoke-token', - }; - - jest - .spyOn(global, 'fetch') - .mockResolvedValueOnce( - new Response(JSON.stringify(mockRevokeResponse)), - ); - - const revokeResult = - await mockHandler.revokeRefreshToken('refresh-token'); - - expect(revokeResult).toEqual({ - refresh_token: 'refresh-token', - revoke_token: 'revoke-token', - }); }); it('successfully gets auth tokens with idToken', async () => { const mockResponse = { - success: true, id_token: 'mock-id-token', refresh_token: 'mock-refresh-token', - indexes: [1, 2, 3], - endpoints: { endpoint1: 'value1' }, - message: 'Success', - jwt_tokens: { token1: 'value1' }, + revoke_token: 'mock-revoke-token', + access_token: 'mock-access-token', + metadata_access_token: 'mock-metadata-access-token', + expires_in: 3600, }; (global.fetch as jest.Mock).mockResolvedValueOnce({ @@ -361,6 +334,10 @@ describe('BaseLoginHandler', () => { const mockResponse = { success: true, id_token: 'mock-id-token', + refresh_token: 'mock-refresh-token', + revoke_token: 'mock-revoke-token', + access_token: 'mock-access-token', + metadata_access_token: 'mock-metadata-access-token', message: 'Success', }; @@ -410,6 +387,10 @@ describe('BaseLoginHandler', () => { const mockResponse = { success: true, id_token: 'mock-id-token', + refresh_token: 'mock-refresh-token', + revoke_token: 'mock-revoke-token', + access_token: 'mock-access-token', + metadata_access_token: 'mock-metadata-access-token', message: 'Success', }; diff --git a/app/core/OAuthService/OAuthLoginHandlers/baseHandler.ts b/app/core/OAuthService/OAuthLoginHandlers/baseHandler.ts index fc6a36debd57..eaea36a99a39 100644 --- a/app/core/OAuthService/OAuthLoginHandlers/baseHandler.ts +++ b/app/core/OAuthService/OAuthLoginHandlers/baseHandler.ts @@ -38,7 +38,7 @@ export async function getAuthTokens( }); if (res.status === 200 || res.status === 201) { - const data = (await res.json()) satisfies AuthResponse; + const data: AuthResponse = (await res.json()) satisfies AuthResponse; return data; } @@ -140,78 +140,6 @@ export abstract class BaseLoginHandler { } } - /** - * Refresh the JWT Token using the refresh token. - * - * @param refreshToken - The refresh token from the Web3Auth Authentication Server. - * @returns The JWT Token from the Web3Auth Authentication Server and new refresh token. - */ - async refreshAuthToken(refreshToken: string): Promise { - const { web3AuthNetwork } = this.options; - const requestData = { - client_id: this.options.clientId, - login_provider: this.authConnection, - network: web3AuthNetwork, - refresh_token: refreshToken, - grant_type: 'refresh_token', // specify refresh token flow - }; - const res = await this.requestAuthToken(JSON.stringify(requestData)); - return res; - } - - /** - * Revoke the refresh token. - * - * @param revokeToken - The revoke token from the Web3Auth Authentication Server. - */ - async revokeRefreshToken(revokeToken: string): Promise<{ - refresh_token: string; - revoke_token: string; - }> { - const requestData = { - revoke_token: revokeToken, - }; - - const res = await fetch( - `${this.options.authServerUrl}${this.AUTH_SERVER_REVOKE_PATH}`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify(requestData), - }, - ); - - const data = await res.json(); - return { - refresh_token: data.new_refresh_token, - revoke_token: data.new_revoke_token, - }; - } - - /** - * Make a request to the Web3Auth Authentication Server to get the JWT Token. - * - * @param requestData - The request data for the Web3Auth Authentication Server. - * @returns The JWT Token from the Web3Auth Authentication Server. - */ - protected async requestAuthToken(requestData: string): Promise { - const res = await fetch( - `${this.options.authServerUrl}${this.AUTH_SERVER_TOKEN_PATH}`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: requestData, - }, - ); - - const data = await res.json(); - return data; - } - /** * Generate a nonce value. * diff --git a/app/core/OAuthService/OAuthLoginHandlers/index.test.ts b/app/core/OAuthService/OAuthLoginHandlers/index.test.ts index 39b7e6d40ad3..a1fb91049f44 100644 --- a/app/core/OAuthService/OAuthLoginHandlers/index.test.ts +++ b/app/core/OAuthService/OAuthLoginHandlers/index.test.ts @@ -147,7 +147,9 @@ describe('OAuth login handlers', () => { new Response( JSON.stringify({ access_token: 'access-token', + metadata_access_token: 'metadata-access-token', refresh_token: 'refresh-token', + revoke_token: 'revoke-token', id_token: 'id-token', indexes: [1, 2, 3], endpoints: { @@ -184,7 +186,7 @@ describe('OAuth login handlers', () => { jest.clearAllMocks(); }); - it('should throw UserCancelled error when user cancels', async () => { + it('throw UserCancelled error when user cancels', async () => { mockSignInAsync.mockRejectedValue( new Error('The user canceled the authorization attempt'), ); @@ -201,7 +203,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UnknownError for other errors', async () => { + it('throw UnknownError for other errors', async () => { mockSignInAsync.mockRejectedValue(new Error('Network error')); const handler = createLoginHandler('ios', AuthConnection.Apple); @@ -215,7 +217,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UnknownError when no identity token is returned', async () => { + it('throw UnknownError when no identity token is returned', async () => { mockSignInAsync.mockResolvedValue({ identityToken: null }); const handler = createLoginHandler('ios', AuthConnection.Apple); @@ -229,7 +231,7 @@ describe('OAuth login handlers', () => { } }); - it('should re-throw existing OAuthError instances', async () => { + it('re-throw existing OAuthError instances', async () => { const existingError = new OAuthError( 'Test error', OAuthErrorType.LoginError, @@ -247,7 +249,7 @@ describe('OAuth login handlers', () => { jest.clearAllMocks(); }); - it('should throw UserCancelled error when user cancels', async () => { + it('throw UserCancelled error when user cancels', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'cancel', }); @@ -264,7 +266,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UserDismissed error when user dismisses', async () => { + it('throw UserDismissed error when user dismisses', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'dismiss', }); @@ -281,7 +283,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UnknownError for other result types', async () => { + it('throw UnknownError for other result types', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'error', error: 'Some error', @@ -298,7 +300,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw error when promptAsync throws exception', async () => { + it('throw error when promptAsync throws exception', async () => { mockExpoAuthSessionPromptAsync.mockRejectedValue( new Error('Network error'), ); @@ -314,7 +316,7 @@ describe('OAuth login handlers', () => { jest.clearAllMocks(); }); - it('should throw UserCancelled error when user cancels', async () => { + it('throw UserCancelled error when user cancels', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'cancel', }); @@ -331,7 +333,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UserDismissed error when user dismisses', async () => { + it('throw UserDismissed error when user dismisses', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'dismiss', }); @@ -348,7 +350,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw LoginError when error with message is returned', async () => { + it('throw LoginError when error with message is returned', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'error', error: { message: 'Authentication failed' }, @@ -366,7 +368,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UnknownError when error without message is returned', async () => { + it('throw UnknownError when error without message is returned', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'error', error: null, @@ -383,7 +385,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UnknownError for unexpected result types', async () => { + it('throw UnknownError for unexpected result types', async () => { mockExpoAuthSessionPromptAsync.mockResolvedValue({ type: 'unknown', }); @@ -399,7 +401,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw error when promptAsync throws exception', async () => { + it('throw error when promptAsync throws exception', async () => { mockExpoAuthSessionPromptAsync.mockRejectedValue( new Error('Network error'), ); @@ -415,7 +417,7 @@ describe('OAuth login handlers', () => { jest.clearAllMocks(); }); - it('should throw UserCancelled error when user cancels', async () => { + it('throw UserCancelled error when user cancels', async () => { mockSignInWithGoogle.mockRejectedValue(new Error('User cancelled')); const handler = createLoginHandler('android', AuthConnection.Google); @@ -430,7 +432,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UnknownError for other errors', async () => { + it('throw UnknownError for other errors', async () => { mockSignInWithGoogle.mockRejectedValue(new Error('Network error')); const handler = createLoginHandler('android', AuthConnection.Google); @@ -445,7 +447,7 @@ describe('OAuth login handlers', () => { } }); - it('should throw UnknownError when result type is not google-signin', async () => { + it('throw UnknownError when result type is not google-signin', async () => { mockSignInWithGoogle.mockResolvedValue({ type: 'unknown', }); @@ -463,7 +465,7 @@ describe('OAuth login handlers', () => { }); // no credentials - it('should throw GoogleLoginNoCredential when no credentials are found', async () => { + it('throw GoogleLoginNoCredential when no credentials are found', async () => { const message = 'e1 error Mo.m: No credential available'; mockSignInWithGoogle.mockRejectedValue(new Error(message)); @@ -498,7 +500,7 @@ describe('OAuth login handlers', () => { expect(mockSignInAsync).toHaveBeenCalledTimes(0); }); - it('should throw GoogleLoginNoMatchingCredential when no matching credential is found', async () => { + it('throw GoogleLoginNoMatchingCredential when no matching credential is found', async () => { const message = 'During begin signin, failure response from one tap. 16: [28433] Cannot find matching credential error'; mockSignInWithGoogle.mockRejectedValue(new Error(message)); @@ -535,7 +537,7 @@ describe('OAuth login handlers', () => { expect(mockSignInAsync).toHaveBeenCalledTimes(0); }); - it('should re-throw existing OAuthError instances', async () => { + it('re-throw existing OAuthError instances', async () => { const existingError = new OAuthError( 'Test error', OAuthErrorType.LoginError, diff --git a/app/core/OAuthService/OAuthService.test.ts b/app/core/OAuthService/OAuthService.test.ts index a4152508bf80..e11fc01fba7f 100644 --- a/app/core/OAuthService/OAuthService.test.ts +++ b/app/core/OAuthService/OAuthService.test.ts @@ -1,8 +1,4 @@ -import { - AuthConnection, - AuthResponse, - LoginHandlerResult, -} from './OAuthInterface'; +import { AuthConnection } from './OAuthInterface'; import ReduxService, { ReduxStore } from '../redux'; import Engine from '../Engine'; import { OAuthError, OAuthErrorType } from './error'; @@ -45,29 +41,26 @@ jest.mock('./OAuthLoginHandlers/constants', () => ({ })); import OAuthLoginService from './OAuthService'; -let mockLoginHandlerResponse: () => LoginHandlerResult | undefined = jest - .fn() - .mockImplementation(() => ({ - idToken: MOCK_JWT_TOKEN, - authConnection: AuthConnection.Google, - clientId: 'clientId', - web3AuthNetwork: Web3AuthNetwork.Mainnet, - })); - -let mockGetAuthTokens: () => Promise = jest - .fn() - .mockImplementation(() => ({ - id_token: MOCK_JWT_TOKEN, - access_token: 'mock-access-token', - indexes: [1, 2, 3], - endpoints: { endpoint1: 'value1' }, - refresh_token: 'mock-refresh-token', - })); +const mockLoginHandlerResponse = jest.fn().mockImplementation(() => ({ + idToken: MOCK_JWT_TOKEN, + authConnection: AuthConnection.Google, + clientId: 'clientId', + web3AuthNetwork: Web3AuthNetwork.Mainnet, +})); + +const mockGetAuthTokens = jest.fn().mockImplementation(() => ({ + id_token: MOCK_JWT_TOKEN, + access_token: 'mock-access-token', + indexes: [1, 2, 3], + endpoints: { endpoint1: 'value1' }, + refresh_token: 'mock-refresh-token', + revoke_token: 'mock-revoke-token', +})); const mockCreateLoginHandler = jest.fn().mockImplementation(() => ({ authConnection: AuthConnection.Google, login: () => mockLoginHandlerResponse(), - getAuthTokens: mockGetAuthTokens, + getAuthTokens: () => mockGetAuthTokens(), decodeIdToken: () => JSON.stringify({ email: 'swnam909@gmail.com', @@ -96,7 +89,7 @@ jest.mock('../Engine', () => ({ }, })); -let mockAuthenticate = jest.fn().mockImplementation(() => ({ +const mockAuthenticate = jest.fn().mockImplementation(() => ({ nodeAuthTokens: [], isNewUser: true, })); @@ -149,7 +142,7 @@ describe('OAuth login service', () => { it('return a type success, existing user', async () => { const loginHandler = mockCreateLoginHandler(); - mockAuthenticate = jest.fn().mockImplementation(() => ({ + mockAuthenticate.mockImplementation(() => ({ nodeAuthTokens: [], isNewUser: false, })); @@ -170,7 +163,7 @@ describe('OAuth login service', () => { it('throw on SeedlessOnboardingController error', async () => { const loginHandler = mockCreateLoginHandler(); - mockAuthenticate = jest.fn().mockImplementation(() => { + mockAuthenticate.mockImplementation(() => { throw new Error('Test error'); }); jest @@ -188,7 +181,7 @@ describe('OAuth login service', () => { }); it('throw on AuthServerError', async () => { - mockGetAuthTokens = jest.fn().mockImplementation(() => { + mockGetAuthTokens.mockImplementation(() => { throw new OAuthError('Auth server error', OAuthErrorType.AuthServerError); }); const loginHandler = mockCreateLoginHandler(); @@ -210,7 +203,7 @@ describe('OAuth login service', () => { dispatch: jest.fn(), } as unknown as ReduxStore); - mockLoginHandlerResponse = jest.fn().mockImplementation(() => { + mockLoginHandlerResponse.mockImplementation(() => { throw new OAuthError('Login dismissed', OAuthErrorType.UserDismissed); }); @@ -226,7 +219,7 @@ describe('OAuth login service', () => { it('throw on login error', async () => { const loginHandler = mockCreateLoginHandler(); - mockLoginHandlerResponse = jest.fn().mockImplementation(() => { + mockLoginHandlerResponse.mockImplementation(() => { throw new OAuthError('Login error', OAuthErrorType.LoginError); }); @@ -239,6 +232,24 @@ describe('OAuth login service', () => { expect(mockGetAuthTokens).toHaveBeenCalledTimes(0); expect(mockAuthenticate).toHaveBeenCalledTimes(0); }); + + // use for loop to test undefine and null cases + for (const value of [undefined, null]) { + it(`throws error when login handler returns ${value}`, async () => { + mockLoginHandlerResponse.mockClear(); + mockLoginHandlerResponse.mockImplementation(() => value); + const loginHandler = mockCreateLoginHandler(); + + await expectOAuthError( + OAuthLoginService.handleOAuthLogin(loginHandler, false), + OAuthErrorType.LoginError, + ); + + expect(mockLoginHandlerResponse).toHaveBeenCalledTimes(1); + expect(mockGetAuthTokens).toHaveBeenCalledTimes(0); + expect(mockAuthenticate).toHaveBeenCalledTimes(0); + }); + } }); describe('updateMarketingOptInStatus', () => { diff --git a/app/core/OAuthService/OAuthService.ts b/app/core/OAuthService/OAuthService.ts index b0c1dad8a7e9..8539aef89c90 100644 --- a/app/core/OAuthService/OAuthService.ts +++ b/app/core/OAuthService/OAuthService.ts @@ -8,6 +8,7 @@ import { AuthResponse, OAuthUserInfo, OAuthLoginResultType, + LoginHandlerResult, } from './OAuthInterface'; import { Web3AuthNetwork } from '@metamask/seedless-onboarding-controller'; import { @@ -20,6 +21,10 @@ import { import { OAuthError, OAuthErrorType } from './error'; import { BaseLoginHandler } from './OAuthLoginHandlers/baseHandler'; import { Platform } from 'react-native'; +import { + SeedlessOnboardingControllerError, + SeedlessOnboardingControllerErrorType, +} from '../Engine/controllers/seedless-onboarding-controller/error'; import { MetaMetrics } from '../Analytics'; import { MetricsEventBuilder } from '../Analytics/MetricsEventBuilder'; import { MetaMetricsEvents } from '../Analytics/MetaMetrics.events'; @@ -114,6 +119,23 @@ export class OAuthService { authConnection ]; + const refreshToken = data.refresh_token; + const revokeToken = data.revoke_token; + + if (!refreshToken) { + throw new SeedlessOnboardingControllerError( + SeedlessOnboardingControllerErrorType.AuthenticationError, + 'No refresh token found', + ); + } + + if (!revokeToken) { + throw new SeedlessOnboardingControllerError( + SeedlessOnboardingControllerErrorType.AuthenticationError, + 'No revoke token found', + ); + } + const result = await Engine.context.SeedlessOnboardingController.authenticate({ idTokens: [data.id_token], @@ -122,8 +144,8 @@ export class OAuthService { groupedAuthConnectionId: authConnectionConfig.groupedAuthConnectionId, userId, socialLoginEmail: accountName, - refreshToken: data.refresh_token, - revokeToken: data.revoke_token, + refreshToken, + revokeToken, accessToken: data.access_token, metadataAccessToken: data.metadata_access_token, }); @@ -195,14 +217,23 @@ export class OAuthService { this.#dispatchLogin(); try { - let result, data, handleCodeFlowResult; + let result: LoginHandlerResult, + data: AuthResponse, + handleCodeFlowResult: HandleOAuthLoginResult; let providerLoginSuccess = false; try { trace({ name: TraceName.OnboardingOAuthProviderLogin, op: TraceOperation.OnboardingSecurityOp, }); - result = await loginHandler.login(); + const loginResult = await loginHandler.login(); + if (!loginResult) { + throw new OAuthError( + 'Login handler return empty result', + OAuthErrorType.LoginError, + ); + } + result = loginResult; providerLoginSuccess = true; } catch (error) { const errorMessage = diff --git a/package.json b/package.json index 03cf760a6a7d..21eab195705f 100644 --- a/package.json +++ b/package.json @@ -269,7 +269,7 @@ "@metamask/scure-bip39": "^2.1.0", "@metamask/sdk-analytics": "0.0.5", "@metamask/sdk-communication-layer": "0.33.1", - "@metamask/seedless-onboarding-controller": "^5.0.0", + "@metamask/seedless-onboarding-controller": "^6.1.0", "@metamask/selected-network-controller": "^25.0.0", "@metamask/signature-controller": "^35.0.0", "@metamask/slip44": "^4.2.0", diff --git a/yarn.lock b/yarn.lock index 0526f8d68d35..115b59c74119 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7002,7 +7002,7 @@ __metadata: languageName: node linkType: hard -"@metamask/auth-network-utils@npm:^0.3.0, @metamask/auth-network-utils@npm:^0.3.1": +"@metamask/auth-network-utils@npm:^0.3.0": version: 0.3.1 resolution: "@metamask/auth-network-utils@npm:0.3.1" dependencies: @@ -7019,6 +7019,23 @@ __metadata: languageName: node linkType: hard +"@metamask/auth-network-utils@npm:^0.4.0": + version: 0.4.0 + resolution: "@metamask/auth-network-utils@npm:0.4.0" + dependencies: + "@noble/curves": "npm:^1.8.1" + "@noble/hashes": "npm:^1.7.1" + "@toruslabs/bs58": "npm:^1.0.0" + "@toruslabs/constants": "npm:^15.0.0" + "@toruslabs/eccrypto": "npm:^6.2.0" + bn.js: "npm:^5.2.2" + elliptic: "npm:^6.6.1" + json-stable-stringify-without-jsonify: "npm:^1.0.1" + loglevel: "npm:^1.9.2" + checksum: 10/db528a23607dc010c0c6ab94cd1d7042041cabbb0f3ca0e11745f23dcdb733d79b72ed1ca16733d96a97aa953c8af8a5c190f2287c2131862cccb2a9991a9006 + languageName: node + linkType: hard + "@metamask/auto-changelog@npm:^5.1.0": version: 5.1.0 resolution: "@metamask/auto-changelog@npm:5.1.0" @@ -8471,14 +8488,14 @@ __metadata: languageName: node linkType: hard -"@metamask/seedless-onboarding-controller@npm:^5.0.0": - version: 5.0.0 - resolution: "@metamask/seedless-onboarding-controller@npm:5.0.0" +"@metamask/seedless-onboarding-controller@npm:^6.1.0": + version: 6.1.0 + resolution: "@metamask/seedless-onboarding-controller@npm:6.1.0" dependencies: "@metamask/auth-network-utils": "npm:^0.3.0" "@metamask/base-controller": "npm:^9.0.0" "@metamask/messenger": "npm:^0.3.0" - "@metamask/toprf-secure-backup": "npm:^0.7.1" + "@metamask/toprf-secure-backup": "npm:^0.10.0" "@metamask/utils": "npm:^11.8.1" "@noble/ciphers": "npm:^1.3.0" "@noble/curves": "npm:^1.9.2" @@ -8486,7 +8503,7 @@ __metadata: async-mutex: "npm:^0.5.0" peerDependencies: "@metamask/keyring-controller": ^24.0.0 - checksum: 10/e64637c873b71c235aadfc6b4acc209bc287f505dcd1a056e83ca6cdb8b0280492a6869b16bc7e6e9b23e16a00d5d2a7d988801e8243a26d32c5af71472ca14d + checksum: 10/f2e2bdaf7d2a32f22fce19b8384ea5f22d8064e9a8fcb3b097f6ac4cd4e2e9ad14a4c7bb45bc10f6a867782a4ec40289669afe281378301808c0104444b8d5db languageName: node linkType: hard @@ -8815,21 +8832,21 @@ __metadata: languageName: node linkType: hard -"@metamask/toprf-secure-backup@npm:^0.7.1": - version: 0.7.1 - resolution: "@metamask/toprf-secure-backup@npm:0.7.1" +"@metamask/toprf-secure-backup@npm:^0.10.0": + version: 0.10.0 + resolution: "@metamask/toprf-secure-backup@npm:0.10.0" dependencies: - "@metamask/auth-network-utils": "npm:^0.3.1" + "@metamask/auth-network-utils": "npm:^0.4.0" "@noble/ciphers": "npm:^1.2.1" "@noble/curves": "npm:^1.8.1" "@noble/hashes": "npm:^1.7.1" "@sentry/core": "npm:^9.10.0" "@toruslabs/constants": "npm:^15.0.0" - "@toruslabs/eccrypto": "npm:^6.1.0" + "@toruslabs/eccrypto": "npm:^6.2.0" "@toruslabs/fetch-node-details": "npm:^15.0.0" "@toruslabs/http-helpers": "npm:^8.1.1" - bn.js: "npm:^5.2.1" - checksum: 10/3089a58bb613ed75e2ee825bdee23c526f564687e7ee7143e5166eba7a759067499cec8a1ee65f46586f26cd8ff7aca75db3c04cade42753486fc3bfc11fdfec + bn.js: "npm:^5.2.2" + checksum: 10/07d6e9d96072a79de1ae0b60cea6dc1e593286a72739e865043ddd14e50b106b70193f44865f5ade191de0fdf87c3b1e14062ed1f6479a03da40d5c1bf4c98d8 languageName: node linkType: hard @@ -16186,12 +16203,12 @@ __metadata: languageName: node linkType: hard -"@toruslabs/eccrypto@npm:^6.1.0": - version: 6.1.0 - resolution: "@toruslabs/eccrypto@npm:6.1.0" +"@toruslabs/eccrypto@npm:^6.1.0, @toruslabs/eccrypto@npm:^6.2.0": + version: 6.2.0 + resolution: "@toruslabs/eccrypto@npm:6.2.0" dependencies: elliptic: "npm:^6.6.1" - checksum: 10/8f79621ec4bd712eb12e70c0385353aa70221fe2b501ee674718c74a4147f82ede3ff38a045254b9da4bc9a5d1f891b87025904b7de8f6b8962791681ee65837 + checksum: 10/58da3aa5128c29dda8ccd4b643b6f3d31a19fb25dec555d4016f801a9680502bbce9715aa1ebd7ab4aa1c1e7b7cf46979c9cb6ccd14bcf104f4278d994c01d09 languageName: node linkType: hard @@ -34391,7 +34408,7 @@ __metadata: "@metamask/scure-bip39": "npm:^2.1.0" "@metamask/sdk-analytics": "npm:0.0.5" "@metamask/sdk-communication-layer": "npm:0.33.1" - "@metamask/seedless-onboarding-controller": "npm:^5.0.0" + "@metamask/seedless-onboarding-controller": "npm:^6.1.0" "@metamask/selected-network-controller": "npm:^25.0.0" "@metamask/signature-controller": "npm:^35.0.0" "@metamask/slip44": "npm:^4.2.0"