Skip to content

Commit 508a9ec

Browse files
authored
feat(authentication-controller): prevent login race conditions (MetaMask#6353)
## Explanation This PR adds a deferred login mechanism to `SRPJwtBearerAuth` that prevents race conditions when multiple concurrent authentication attempts occur. The implementation uses Promise caching to ensure only one login operation executes at a time, with subsequent concurrent calls sharing the same Promise result. Changes: - Extract login logic into dedicated `#performLogin` method - Add `#deferredLogin` method with Promise map caching for race condition prevention ## References Related to: https://consensyssoftware.atlassian.net/browse/MUL-641 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
1 parent 5feebfb commit 508a9ec

3 files changed

Lines changed: 97 additions & 0 deletions

File tree

packages/profile-sync-controller/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Changed
11+
12+
- Implement deferred login pattern in `SRPJwtBearerAuth` to prevent race conditions during concurrent authentication attempts ([#6353](https://github.com/MetaMask/core/pull/6353))
13+
- Add `#deferredLogin` method that ensures only one login operation executes at a time using Promise map caching
14+
1015
## [24.0.0]
1116

1217
### Added

packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/flow-srp.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,9 @@ export class SRPJwtBearerAuth implements IBaseAuth {
6868

6969
readonly #metametrics?: MetaMetricsAuth;
7070

71+
// Map to store ongoing login promises by entropySourceId
72+
readonly #ongoingLogins = new Map<string, Promise<LoginResponse>>();
73+
7174
#customProvider?: Eip1193Provider;
7275

7376
constructor(
@@ -169,6 +172,11 @@ export class SRPJwtBearerAuth implements IBaseAuth {
169172
}
170173

171174
async #login(entropySourceId?: string): Promise<LoginResponse> {
175+
// Use a deferred login to avoid race conditions
176+
return await this.#deferredLogin(entropySourceId);
177+
}
178+
179+
async #performLogin(entropySourceId?: string): Promise<LoginResponse> {
172180
// Nonce
173181
const publicKey = await this.getIdentifier(entropySourceId);
174182
const nonceRes = await getNonce(publicKey, this.#config.env);
@@ -206,6 +214,32 @@ export class SRPJwtBearerAuth implements IBaseAuth {
206214
return result;
207215
}
208216

217+
async #deferredLogin(entropySourceId?: string): Promise<LoginResponse> {
218+
// Use a key that accounts for undefined entropySourceId
219+
const loginKey = entropySourceId ?? '__default__';
220+
221+
// Check if there's already an ongoing login for this entropySourceId
222+
const existingLogin = this.#ongoingLogins.get(loginKey);
223+
if (existingLogin) {
224+
return existingLogin;
225+
}
226+
227+
// Create a new login promise
228+
const loginPromise = this.#performLogin(entropySourceId);
229+
230+
// Store the promise in the map
231+
this.#ongoingLogins.set(loginKey, loginPromise);
232+
233+
try {
234+
// Wait for the login to complete
235+
const result = await loginPromise;
236+
return result;
237+
} finally {
238+
// Always clean up the ongoing login promise when done
239+
this.#ongoingLogins.delete(loginKey);
240+
}
241+
}
242+
209243
#createSrpLoginRawMessage(
210244
nonce: string,
211245
publicKey: string,

packages/profile-sync-controller/src/sdk/authentication.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { arrangeAuthAPIs } from './__fixtures__/auth';
44
import type { MockVariable } from './__fixtures__/test-utils';
55
import { arrangeAuth, arrangeMockProvider } from './__fixtures__/test-utils';
66
import { JwtBearerAuth } from './authentication';
7+
import * as AuthServices from './authentication-jwt-bearer/services';
78
import type { LoginResponse, Pair } from './authentication-jwt-bearer/types';
89
import {
910
NonceRetrievalError,
@@ -196,6 +197,63 @@ describe('Authentication - SRP Flow - getAccessToken(), getUserProfile() & getUs
196197
expect(mockUserProfileLineageUrl.isDone()).toBe(true);
197198
});
198199

200+
it('prevents race conditions with concurrent login attempts', async () => {
201+
const { auth, mockGetLoginResponse } = arrangeAuth('SRP', MOCK_SRP);
202+
203+
// Mock expired token that will force re-authentication on the 4th call
204+
const expiredToken = createMockStoredProfile();
205+
expiredToken.token.expiresIn = 1; // 1 second expiration
206+
expiredToken.token.obtainedAt = Date.now() - 2000; // 2 seconds ago (expired)
207+
208+
// First call returns null (no cached session), subsequent calls return expired token
209+
mockGetLoginResponse
210+
.mockResolvedValueOnce(null)
211+
.mockResolvedValue(expiredToken);
212+
213+
arrangeAuthAPIs();
214+
215+
// Spy on the service methods to count how many times they're called
216+
const getNonceSpy = jest.spyOn(AuthServices, 'getNonce');
217+
const authenticateSpy = jest.spyOn(AuthServices, 'authenticate');
218+
const authorizeOIDCSpy = jest.spyOn(AuthServices, 'authorizeOIDC');
219+
220+
// Make three concurrent login attempts
221+
const loginPromise1 = auth.getAccessToken();
222+
const loginPromise2 = auth.getAccessToken();
223+
const loginPromise3 = auth.getAccessToken();
224+
225+
// Wait for all promises to resolve
226+
const [token1, token2, token3] = await Promise.all([
227+
loginPromise1,
228+
loginPromise2,
229+
loginPromise3,
230+
]);
231+
232+
// All should return the same token
233+
expect(token1).toBe(MOCK_ACCESS_JWT);
234+
expect(token2).toBe(MOCK_ACCESS_JWT);
235+
expect(token3).toBe(MOCK_ACCESS_JWT);
236+
237+
// Verify that each service was called exactly once despite three concurrent requests
238+
expect(getNonceSpy).toHaveBeenCalledTimes(1);
239+
expect(authenticateSpy).toHaveBeenCalledTimes(1);
240+
expect(authorizeOIDCSpy).toHaveBeenCalledTimes(1);
241+
242+
// After the concurrent promises resolve, make another call with expired token
243+
// This should trigger a second login because the cached token is expired
244+
const token4 = await auth.getAccessToken();
245+
expect(token4).toBe(MOCK_ACCESS_JWT);
246+
247+
// Service methods should now have been called twice (initial login + expired token refresh)
248+
expect(getNonceSpy).toHaveBeenCalledTimes(2);
249+
expect(authenticateSpy).toHaveBeenCalledTimes(2);
250+
expect(authorizeOIDCSpy).toHaveBeenCalledTimes(2);
251+
252+
getNonceSpy.mockRestore();
253+
authenticateSpy.mockRestore();
254+
authorizeOIDCSpy.mockRestore();
255+
});
256+
199257
it('the SRP signIn failed: nonce error', async () => {
200258
const { auth } = arrangeAuth('SRP', MOCK_SRP);
201259

0 commit comments

Comments
 (0)