Skip to content

feat(auth): Create backfill script for accountAuthorzations table#20624

Open
nshirley wants to merge 1 commit into
mainfrom
worktree-FXA-12932
Open

feat(auth): Create backfill script for accountAuthorzations table#20624
nshirley wants to merge 1 commit into
mainfrom
worktree-FXA-12932

Conversation

@nshirley
Copy link
Copy Markdown
Contributor

@nshirley nshirley commented May 19, 2026

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

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • Key files/areas to focus on:
  • Suggested review order:
  • Risky or complex parts:

Screenshots (Optional)

Screenshot 2026-05-20 at 09 18 16

Other information (Optional)

Any other information that is important to this pull request.

@nshirley nshirley force-pushed the worktree-FXA-12932 branch 2 times, most recently from 2d7a1fd to 4327c06 Compare May 19, 2026 23:03
@nshirley nshirley marked this pull request as ready for review May 20, 2026 13:45
Copilot AI review requested due to automatic review settings May 20, 2026 13:45
@nshirley nshirley requested a review from a team as a code owner May 20, 2026 13:45
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.ts to scan refreshTokens and upsert accountAuthorizations by (uid, scope, service, clientId).
  • Adds unit tests for row resolution, batching, retry behavior, and stats emission.
  • Adds integration tests that insert real refreshTokens rows and assert resulting accountAuthorizations rows.

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.exchange is not defined in the current config schema (see packages/fxa-auth-server/config/index.ts, which defines oauthServer.tokenExchange but no oauthServer.exchange). As written, exchangeCfg will always be {}, causing serviceScopes to be empty and the script to immediately return a config error in all environments. Please either add oauthServer.exchange.serviceScopes/allowedClientsForService to 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 makes skippedAllowlist and the account_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

  • withRetry will throw undefined if called with attempts <= 0 because the loop body never runs and lastErr is never set. Since withRetry is exported, it would be safer to validate/clamp attempts to 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.

@nshirley nshirley force-pushed the worktree-FXA-12932 branch 2 times, most recently from d1c4963 to 0dac240 Compare May 20, 2026 16:20
// 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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did want to circle back and note that I fixed the backfill for this case. If there is no browser service that matches it will still attempt to write the backfill with a null service

}

// LEAST(firstAuthorizedTosAt) preserves the earliest discovered grant across
// arbitrary cursor-scan order. GREATEST(lastAuthorizedTosAt) keeps the latest
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:
 - 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
@nshirley nshirley force-pushed the worktree-FXA-12932 branch from 0dac240 to 074bc4c Compare May 26, 2026 16:27
@nshirley nshirley requested a review from LZoog May 28, 2026 14:35
Copy link
Copy Markdown
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

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

@nshirley This is looking great! I appreciate all of your work on this. I spent some time trying to find edge cases and other things to look out for, and I feel you've basically covered it all at this point.

We only have VPN in ourallowedClientsForService allowlist which you check against. I think we could go ahead and add relay to that list to be safe - I don't think any other clients ever request the Relay scope, but as-is if I had a refresh token from 123done or some RP that had the Relay scope, the backfill adds service=relay when 123done would not be considered to be under the same "project umbrella." We don't need to worry about any other services since Mobile (or rather refresh tokens) don't have any other potential "services" besides these two.

On the flip side to the above, I think your backfill script would completely skip the 123done entry with Relay scope if we update that list - it would just drop it, right? And I think the right thing to do is to still record the row, but without service=relay.

I think in reality we don't have outlier cases for the above so I'd say it's just nice to have. I'll test your PR out and come back and r+.

Comment on lines +265 to +266
firstAuthorizedTosAt = LEAST(firstAuthorizedTosAt, VALUES(firstAuthorizedTosAt)),
lastAuthorizedTosAt = GREATEST(lastAuthorizedTosAt, VALUES(lastAuthorizedTosAt))`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Great for a user with multiple refresh tokens or having an entry in our auth table before this backfill runs.

* read for non-browser RPs.
*
* Per service notes:
* - relay rows are written even though token-exchange currently bypasses
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is fine also because on the Firefox side, they query the user's connected services list first before they do the token exchange for Relay. They only do the query now if they have Relay on their refresh token list.

* services ships a 4xx handler" comment in lib/routes/oauth/token.js.
* Backfilled rows prevent existing users from being re-prompted when
* that bypass is eventually removed.
* - sync rows materialize from mobile only — Firefox Desktop calls /destroy
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sync rows, but also all service logins like Smart Window etc. (I know you know, and not saying we even need to update the comment, just noting)

Copy link
Copy Markdown
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

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

r+ as the above notes are non-blocking since I don't think we have to deal with those cases IRL and you tested this locally with some seed scripts and the tests etc. look good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants