feat(auth): Create backfill script for accountAuthorzations table#20624
feat(auth): Create backfill script for accountAuthorzations table#20624nshirley wants to merge 1 commit into
Conversation
2d7a1fd to
4327c06
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new backfill script to populate accountAuthorizations rows from existing refreshTokens, plus unit and script-integration tests to validate row selection (service-scope matching, allowlist gating, and LEAST/GREATEST aggregation) and batching behavior.
Changes:
- Introduces
scripts/backfill-account-authorizations/backfill-account-authorizations.tsto scanrefreshTokensand upsertaccountAuthorizationsby(uid, scope, service, clientId). - Adds unit tests for row resolution, batching, retry behavior, and stats emission.
- Adds integration tests that insert real
refreshTokensrows and assert resultingaccountAuthorizationsrows.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/fxa-auth-server/test/scripts/backfill-account-authorizations.in.spec.ts | Script integration tests that seed MySQL refreshTokens and assert accountAuthorizations output. |
| packages/fxa-auth-server/scripts/backfill-account-authorizations/backfill-account-authorizations.ts | New backfill script with batching, retry/backoff, allowlist gating, and metrics/logging. |
| packages/fxa-auth-server/scripts/backfill-account-authorizations/backfill-account-authorizations.spec.ts | Unit tests covering config building, row resolution, upsert SQL shape, fetch pagination, retries, and run-loop behavior. |
Comments suppressed due to low confidence (3)
packages/fxa-auth-server/scripts/backfill-account-authorizations/backfill-account-authorizations.ts:585
config.oauthServer.exchangeis not defined in the current config schema (seepackages/fxa-auth-server/config/index.ts, which definesoauthServer.tokenExchangebut nooauthServer.exchange). As written,exchangeCfgwill always be{}, causingserviceScopesto be empty and the script to immediately return a config error in all environments. Please either addoauthServer.exchange.serviceScopes/allowedClientsForServiceto the config schema + env/config files, or update the script to read from the correct existing config keys.
const dbConfig = config.oauthServer.mysql;
const exchangeCfg = config.oauthServer.exchange ?? {};
const serviceScopes: Record<string, string> = exchangeCfg.serviceScopes ?? {};
const allowedClientsForService: Record<string, string[]> =
exchangeCfg.allowedClientsForService ?? {};
packages/fxa-auth-server/scripts/backfill-account-authorizations/backfill-account-authorizations.ts:421
- The skip counters/metrics are labeled as “tokens_skipped”, but the code increments them by
allowlistDenied.length(number of denied services on the token) and can also count tokens that still produced upserts (e.g., one allowed service scope + one denied scope). This makesskippedAllowlistand theaccount_authz.backfill.tokens_skipped{reason=client_not_allowed}metric not actually represent “tokens skipped”. Consider either (a) counting 1 per token when it produces zero rows due to allowlist denial, or (b) renaming the fields/metric to reflect “service scopes skipped” and adjusting logging accordingly.
if (allowlistDenied.length > 0) {
totalSkippedAllowlist += allowlistDenied.length;
batchSkippedAllowlist += allowlistDenied.length;
}
if (resolved.length === 0) {
if (allowlistDenied.length === 0) {
totalSkippedNoMatch++;
batchSkippedNoMatch++;
packages/fxa-auth-server/scripts/backfill-account-authorizations/backfill-account-authorizations.ts:281
withRetrywill throwundefinedif called withattempts <= 0because the loop body never runs andlastErris never set. SincewithRetryis exported, it would be safer to validate/clampattemptsto at least 1 (or throw a clear error) to avoid hard-to-debug failures if a caller passes 0.
async function withRetry<T>(
operation: () => Promise<T>,
context: {
opName: string;
attempts: number;
initialDelayMs: number;
log: Logger;
statsd?: StatsD;
}
): Promise<T> {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * allowlist gate. Mirrors the runtime ingress in lib/routes/oauth/authorization.js | ||
| * so backfilled rows look identical to organic ones. |
There was a problem hiding this comment.
I worked on this with a cherry-pick of the updates to accountAuthorizations so that's why the reference is different
| // Pull a real VPN-allowlisted clientId from config so the allowlist gate | ||
| // passes in the same way it would in production. | ||
| const exchangeCfg = config.oauthServer.exchange; | ||
| const VPN_ALLOWED_CLIENT_ID: string = | ||
| exchangeCfg.allowedClientsForService?.vpn?.[0]; |
4327c06 to
d1c4963
Compare
Because: - We want to be able to backfill refreshTokens into the accountAuthorizations table This commit: - Adds a backfill script to walk the refreshTokens table, inserting a new accountAuthoriztions for the most recent token/scope/service combo - Adds unit tests for the backfill script Closes: FXA-12932
d1c4963 to
0dac240
Compare
| } from './backfill-account-authorizations'; | ||
|
|
||
| const VPN_SCOPE = 'https://identity.mozilla.com/apps/vpn'; | ||
| const SMARTWINDOW_SCOPE = 'https://identity.mozilla.com/apps/smartwindow'; |
There was a problem hiding this comment.
Just noting, there are not any refresh tokens in the DB with the smartwindow scope since Desktop is using the session token to create those smartwindow-scoped access tokens. These will have to be forward-filling (new logins get set). Still fine for tests.
| // Mirrors lib/routes/oauth/authorization.js:recordAuthorizationRows semantics | ||
| // for the backfill side: | ||
| // - For each scope on the token, resolve to a configured service via | ||
| // scopeToService; non-service scopes (profile/openid/etc.) produce no row. |
There was a problem hiding this comment.
We need to backfill all RPs, not just the ones that correspond with a browser service. If I sign into 123done, then on the auth table we'll have two rows, one for each scope granted (openid and profile). In this case, service is just null, but we still log that the user had authorized those two scopes with that client_id.
This helps us because later, if some RP changes their terms of service and decides users must approve the new ToS, we can reject the silent token exchange, and we can reject prompt=none based on that.
| } | ||
|
|
||
| // LEAST(firstAuthorizedTosAt) preserves the earliest discovered grant across | ||
| // arbitrary cursor-scan order. GREATEST(lastAuthorizedTosAt) keeps the latest |
There was a problem hiding this comment.
I think what you have here is what we want, but it's probably worth calling out somewhere that for all of these that have been backfilled, if the user logged in via prompt=none, we have recorded that as lastAuthorizedTosAt when they would not have seen the ToS in that case.
Probably not worth checking what RPs have that set up since it could've changed and then what would we do, just set it to 0? Eh. This'll be more up to date as time goes on.
Because:
This commit:
Closes: FXA-12932
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Other information (Optional)
Any other information that is important to this pull request.