Skip to content

Commit 7e670e8

Browse files
dschomdschom
authored
Merge pull request #20244 from mozilla/fix-client-ids
polish(auth): Cache registered client ids for 1 minute
2 parents b0ec181 + c684f8e commit 7e670e8

4 files changed

Lines changed: 191 additions & 14 deletions

File tree

‎packages/fxa-auth-server/config/index.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2795,6 +2795,14 @@ const convictConf = convict({
27952795
},
27962796
},
27972797
},
2798+
registeredClients: {
2799+
refreshInterval: {
2800+
default: 60_000,
2801+
doc: 'The interval in ms at which we update the list of known registered client ids',
2802+
env: 'REGISTERED_CLIENTS__REFRESH_INTERVAL',
2803+
format: Number,
2804+
},
2805+
},
27982806
});
27992807

28002808
// handle configuration files. you can specify a CSV list of configuration
@@ -2956,4 +2964,3 @@ export type ConfigType = ReturnType<conf['getProperties']>;
29562964

29572965
export { convictConf as config };
29582966
export default convictConf;
2959-

‎packages/fxa-auth-server/lib/registeredClients.spec.ts‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
4+
5+
import * as Sentry from '@sentry/node';
6+
import { OAuthDb } from './registeredClients';
7+
import { getRegisteredClientIds, reset } from './registeredClients';
8+
9+
jest.mock('@sentry/node', () => ({
10+
captureException: jest.fn(),
11+
}));
12+
13+
/**
14+
* Creates a mock OAuthDb whose `getAllClientIds` resolves with the given
15+
* hex-encoded client ID strings converted to Buffer rows.
16+
*/
17+
function makeDb(ids: string[]): OAuthDb {
18+
return {
19+
mysql: {
20+
getAllClientIds: jest
21+
.fn()
22+
.mockResolvedValue(ids.map((id) => ({ id: Buffer.from(id, 'hex') }))),
23+
},
24+
};
25+
}
26+
27+
describe('getRegisteredClientIds', () => {
28+
beforeEach(() => {
29+
jest.useFakeTimers();
30+
jest.clearAllMocks();
31+
});
32+
33+
afterEach(() => {
34+
reset();
35+
jest.useRealTimers();
36+
});
37+
38+
it('fetches from DB on first call and returns client IDs', async () => {
39+
const db = makeDb(['aabbccdd11223344', 'deadbeefdeadbeef']);
40+
41+
const result = await getRegisteredClientIds(db);
42+
43+
expect(db.mysql.getAllClientIds).toHaveBeenCalledTimes(1);
44+
expect(result).toEqual(new Set(['aabbccdd11223344', 'deadbeefdeadbeef']));
45+
});
46+
47+
it('returns cached IDs on second call within 60 seconds', async () => {
48+
const db = makeDb(['aabbccdd11223344']);
49+
50+
await getRegisteredClientIds(db);
51+
jest.advanceTimersByTime(30_000);
52+
const result = await getRegisteredClientIds(db);
53+
54+
expect(db.mysql.getAllClientIds).toHaveBeenCalledTimes(1);
55+
expect(result).toEqual(new Set(['aabbccdd11223344']));
56+
});
57+
58+
it('refetches from DB after 60 seconds', async () => {
59+
const db = makeDb(['aabbccdd11223344']);
60+
61+
await getRegisteredClientIds(db);
62+
jest.advanceTimersByTime(61_000);
63+
await getRegisteredClientIds(db);
64+
65+
expect(db.mysql.getAllClientIds).toHaveBeenCalledTimes(2);
66+
});
67+
68+
it('returns empty Set and captures to Sentry when DB fails on first call', async () => {
69+
const error = new Error('DB unavailable');
70+
const db: OAuthDb = {
71+
mysql: { getAllClientIds: jest.fn().mockRejectedValue(error) },
72+
};
73+
74+
const result = await getRegisteredClientIds(db);
75+
76+
expect(result).toEqual(new Set());
77+
expect(Sentry.captureException).toHaveBeenCalledWith(error);
78+
});
79+
80+
it('returns last successful Set and captures to Sentry when DB fails on subsequent call', async () => {
81+
const db = makeDb(['aabbccdd11223344']);
82+
83+
// Successful first fetch
84+
await getRegisteredClientIds(db);
85+
86+
// DB fails on refetch
87+
jest.advanceTimersByTime(61_000);
88+
const error = new Error('DB unavailable');
89+
(db.mysql.getAllClientIds as jest.Mock).mockRejectedValueOnce(error);
90+
const result = await getRegisteredClientIds(db);
91+
92+
expect(result).toEqual(new Set(['aabbccdd11223344']));
93+
expect(Sentry.captureException).toHaveBeenCalledWith(error);
94+
});
95+
96+
it('returns an empty Set when DB returns no clients', async () => {
97+
const db = makeDb([]);
98+
99+
const result = await getRegisteredClientIds(db);
100+
101+
expect(result).toEqual(new Set());
102+
});
103+
104+
it('re-fetches exactly at the 60-second boundary', async () => {
105+
const db = makeDb(['aabbccdd11223344']);
106+
107+
await getRegisteredClientIds(db);
108+
jest.advanceTimersByTime(60_001);
109+
await getRegisteredClientIds(db);
110+
111+
expect(db.mysql.getAllClientIds).toHaveBeenCalledTimes(2);
112+
});
113+
});

‎packages/fxa-auth-server/lib/registeredClients.ts‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
4+
5+
import * as Sentry from '@sentry/node';
6+
import { config } from '../config';
7+
8+
const refreshInterval =
9+
config.get('registeredClients.refreshInterval') ?? 60_000;
10+
let registeredClientIds: Set<string> | null = null;
11+
let lastFetch = Date.now();
12+
13+
/**
14+
* Minimal interface for the OAuth database required by this module.
15+
* Only the `getAllClientIds` method is needed to populate the registered-client cache.
16+
*/
17+
export type OAuthDb = {
18+
mysql: {
19+
getAllClientIds: () => Promise<Array<{ id: Buffer }>>;
20+
};
21+
};
22+
23+
/**
24+
* Returns the set of registered OAuth client IDs, refreshing from the database
25+
* at most once per `refreshInterval` milliseconds.
26+
*
27+
* Results are cached in module-level state to avoid a DB round-trip on every
28+
* request. If the database call fails, the previous successful set is returned
29+
* (or an empty set on the very first call) and the error is forwarded to Sentry.
30+
*
31+
* @param oauthDb - Database handle exposing `mysql.getAllClientIds`.
32+
* @returns A Set of lowercase hex-encoded client ID strings.
33+
*/
34+
export async function getRegisteredClientIds(oauthDb: OAuthDb) {
35+
// Refresh the cache on the initial call or after the configured interval
36+
// to avoid a DB round-trip on every request.
37+
if (Date.now() - lastFetch > refreshInterval || registeredClientIds == null) {
38+
// Record the fetch time before awaiting so concurrent callers don't
39+
// all trigger simultaneous refreshes.
40+
lastFetch = Date.now();
41+
42+
// Validate clientId/service against registered OAuth clients to prevent
43+
// unbounded cardinality and normalize onto request.app for StatsD/Sentry tags.
44+
// Client IDs are loaded from the fxa_oauth.clients table on each request;
45+
// the table is small and MySQL serves it from cache.
46+
try {
47+
const clients = await oauthDb.mysql.getAllClientIds();
48+
registeredClientIds = new Set(clients.map((c) => c.id.toString('hex')));
49+
} catch (err) {
50+
Sentry.captureException(err);
51+
}
52+
}
53+
54+
// Return the last successful set, or an empty set to guard against an initial failure.
55+
return registeredClientIds ?? new Set();
56+
}
57+
58+
/**
59+
* Resets the in-memory cache so the next call to `getRegisteredClientIds`
60+
* triggers a fresh database fetch. Intended for use in tests.
61+
*/
62+
export async function reset() {
63+
registeredClientIds = null;
64+
lastFetch = Date.now();
65+
}

‎packages/fxa-auth-server/lib/server.js‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ async function create(
9797
oauthDb
9898
) {
9999
const getGeoData = require('./geodb')(log);
100+
const { getRegisteredClientIds } = require('./registeredClients');
100101
const metricsContext = require('./metrics/context')(log, config);
101102
const metricsEvents = require('./metrics/events')(log, config, glean);
102103
const { sharedSecret: SUBSCRIPTIONS_SECRET } = config.subscriptions;
@@ -327,19 +328,10 @@ async function create(
327328
await customs.checkIpOnly(request, action);
328329
}
329330

330-
// Validate clientId/service against registered OAuth clients to prevent
331-
// unbounded cardinality and normalize onto request.app for StatsD/Sentry tags.
332-
// Client IDs are loaded from the fxa_oauth.clients table on each request;
333-
// the table is small and MySQL serves it from cache.
334-
let registeredClientIds;
335-
try {
336-
const clients = await oauthDb.mysql.getAllClientIds();
337-
registeredClientIds = new Set(clients.map((c) => c.id.toString('hex')));
338-
} catch (err) {
339-
Sentry.captureException(err);
340-
registeredClientIds = new Set();
341-
}
342-
const tags = await resolveClientTags(request, registeredClientIds);
331+
const tags = await resolveClientTags(
332+
request,
333+
await getRegisteredClientIds(oauthDb)
334+
);
343335
if (tags.clientId) {
344336
request.app.clientIdTag = tags.clientId;
345337
Sentry.setTag('clientId', tags.clientId);

0 commit comments

Comments
 (0)