feat(auth): attribute Monitor web and VPN app sign-ins to their service by nshirley · Pull Request #21588 · mozilla/fxa

nshirley · 2026-10-23T13:38:23Z

Because

Monitor web and the Mozilla VPN app sign in without their service's canonical scope, so no consent row exists and the Firefox token exchange sends existing users through "Get started".
The accountAuthorizations backfill (FXA-14086) could not recognise those tokens, and had no way to resume after a killed pod.

This pull request

Adds oauthServer.exchange.serviceForClient, a client_id → service map, defaulting Monitor web → monitor and the VPN app → vpn.
/oauth/authorization falls back to that map when no requested scope is canonical, so the existing writer records the canonical row. Grant and tokens are unchanged.
The backfill script reads the same map, adds the canonical scope row when a token lacks it, and gains --omit-scope and --start-cursor. The map is logged in backfill.start.
Each backfill.batch log line now carries the cursor for resume.
Mappings for services absent from serviceScopes are ignored in both paths.
Deauthorization treats a mapped client's tokens as covering its service's canonical scope, so the bandaid row isn't deauthorized by an unrelated disconnect while the app is still signed in.

Issue that this pull request solves

Closes: FXA-14724

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: resolveServiceValue in lib/routes/oauth/authorization.js; inferServiceForToken and resolveTargetRows in the backfill script.
Suggested review order: config → oauth DB helper → authorization route → backfill script → specs.
Risky or complex parts: the sign-in fallback writes a consent row for a scope the RP did not request. It only runs when zero canonical scopes were requested, and the allowedClientsForService gate still applies.

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

The monitor mapping relies on #21550 (FXA-14723), now merged. Removal is tracked in FXA-14813; it covers the sign-in fallback, the backfill map, and the deauthorization rule, gated on a count of pre-change tokens.

nshirley · 2026-10-23T14:47:34Z

Open question for review: --client-service vs. config only

The backfill currently reads the client → service map from oauthServer.exchange.serviceForClient and also accepts --client-service <id>=<service> to add to or override it per run. That's two sources of truth for what should be one mapping, and it means backfilled rows could differ from the rows sign-in writes.

The alternative is to drop the flag and lean on the config alone, overriding per run with the OAUTH_EXCHANGE_SERVICE_FOR_CLIENT env var when needed. The ticket AC named the flag as an example, so I kept it, but I'd be happy to remove it if we'd rather have the single source. Opinions welcome.

LZoog · 2026-10-23T22:42:50Z

+        doc: "Bandaid (FXA-14724). OAuth client_id (hex) -> service for first-party clients that sign in without their service's canonical scope (Monitor web, the Mozilla VPN app); /oauth/authorization and the accountAuthorizations backfill attribute those sign-ins to the service. Entries for services missing from serviceScopes are ignored. Remove each entry once that client requests its canonical scope (FXA-14813).",
+        format: Object,
+        default: {
+          '802d56ef2a9af9fa': 'monitor',


The backfill currently reads the client → service map from oauthServer.exchange.serviceForClient and also accepts --client-service = to add to or override it per run. That's two sources of truth for what should be one mapping, and it means backfilled rows could differ from the rows sign-in writes.

I know you've already ran this on stage - do you plan to rerun it there again before prod? I guess this could just be a webservices-infra change across envs. If you don't plan to actually use --client-service I'd just remove it and rely on the config and make sure the dry run prints these out but 🤷‍♀️ I'm OK if you want to leave it for flexibility.

Also just a tiny nit, this key is a string and the other isn't? I might also call this serviceForClientBandaid to match the other "bandaids" but you also have that noted here in the docs so I think it's fine as is too.

Dropped the flag in 61ca73b; the map now comes only from config and is logged in backfill.start, so the dry run prints it. On the quoting: prettier forces quotes on the key that starts with a digit and strips them from the one that doesn't, so the mismatch is tool-enforced. Left the name as is since the doc string already says bandaid.

LZoog · 2026-10-23T22:57:57Z

+    )
+    .option(
+      '--omit-scope <scope>',
+      'Do not write rows for this scope (e.g. profile). Repeatable. Service inference still sees the full token scope.',


To state here on the PR: for now we won't use this. If we choose to delete these rows we can do them all in a script later. Dan isn't too concerned about the oauth db memory and us adding rows.

Understood, leaving it in but unused for the prod run.

coderabbitai · 2026-10-27T15:41:41Z

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one): 1

coderabbit

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

Source mode: Auto
Repository: mozilla/fxa
Branch: FXA-14724
File: .coderabbit.yaml
Commit: 34c4665
Layers: Repository YAML
Review profile: CHILL
Plan: Enterprise
Run ID: 303748ea-8de0-4f09-acbc-e68674f11a99

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

nshirley · 2026-10-28T19:18:52Z

Rolled in the deauthorization patch from Slack as c4dd6ff7f8, thanks for putting it together! Agreed it's better not to leave the data gap. I merged main in rather than rebasing this time. I've also added a note to FXA-14813 so the deauthorization rule comes out with the rest of the bandaid, using the token-count query as the gate for when it's safe.

LZoog

r+ with that de-authorization patch in here that we chatted about. Let's make sure to, in the follow up, mention it, as maybe we will just be OK with removing the config at that point and adding a "forward-fill" approach for that as well (e.g. we might reject the token exchange for these logins but that's OK given the alternative is to keep these around indefinitely etc.).

I tested the login patch and see it working. I did not run the script but did review it manually and asked Claude questions about output given certain inputs.

Because: - Monitor web and the Mozilla VPN app sign in without their service's canonical scope, so no consent row exists for the Firefox token exchange. - The accountAuthorizations backfill could not recognise those tokens and had no way to resume after a killed pod. - A disconnect of any allowlisted client deauthorized the fallback row while the app was still signed in. This commit: - Adds oauthServer.exchange.serviceForClient (client_id -> service), read by /oauth/authorization as a last-resort fallback and by the backfill script. - Writes the service's canonical scope row when the token lacks it, in both the sign-in path and the backfill. - Adds --omit-scope and --start-cursor to the backfill and logs the cursor per batch. - Treats a mapped client's tokens as covering its service's canonical scope in authorizationRowsToDeauthorize. - Ignores mappings for services absent from serviceScopes. Removal tracked in FXA-14813.

LZoog reviewed Oct 23, 2026

View reviewed changes

nshirley force-pushed the FXA-14724 branch from 2f9d5ad to 61ca73b Compare October 27, 2026 15:41

nshirley marked this pull request as ready for review October 27, 2026 15:58

nshirley requested a review from a team as a code owner October 27, 2026 15:58

Copilot AI lite review requested due to automatic review settings October 27, 2026 15:58

Copilot started reviewing on behalf of nshirley October 27, 2026 15:59

View session

Copilot AI reviewed Oct 27, 2026

LZoog approved these changes Oct 28, 2026

View reviewed changes

nshirley force-pushed the FXA-14724 branch from c4dd6ff to 34c4665 Compare October 29, 2026 16:40

nshirley merged commit d491c2d into main Oct 29, 2026
21 checks passed

nshirley deleted the FXA-14724 branch October 29, 2026 16:56

nshirley mentioned this pull request Oct 29, 2026

test(auth): drop the Monitor case from the fallback-service spec #21645

Draft

3 of 5 tasks

Navigation Menu

Repository navigation

feat(auth): attribute Monitor web and VPN app sign-ins to their service - #21588

feat(auth): attribute Monitor web and VPN app sign-ins to their service#21588
nshirley merged 1 commit into
mainfrom
FXA-14724

nshirley commented Oct 23, 2026 •

edited

Loading

Uh oh!

nshirley commented Oct 23, 2026

Uh oh!

LZoog Oct 23, 2026

Uh oh!

nshirley Oct 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LZoog Oct 23, 2026

Uh oh!

nshirley Oct 27, 2026

Uh oh!

coderabbitai Bot commented Oct 27, 2026 •

edited by atlassian Bot

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

nshirley commented Oct 28, 2026

Uh oh!

LZoog left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nshirley commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Because

This pull request

Issue that this pull request solves

Checklist

How to review (Optional)

Screenshots (Optional)

Other information (Optional)

Uh oh!

nshirley commented Oct 23, 2026

Uh oh!

LZoog Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LZoog Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Oct 27, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

nshirley commented Oct 28, 2026

Uh oh!

LZoog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nshirley commented Oct 23, 2026 •

edited

Loading

coderabbitai Bot commented Oct 27, 2026 •

edited by atlassian Bot

Loading