Conversation
…c DAU Because: - Firefox Desktop requests the Sync scope on every browser-initiated OAuth flow, whatever service the user is signing into. So a Smart Window, Relay or VPN sign-in mints an apps/oldsync access token and writes an apps/oldsync consent row — inflating both Sync DAU and Sync authorizations, even for passwordless accounts that could never have used Sync. - Product confirmed entering a password is not Sync consent either: the user gets Sync keys so they can enable Sync later, but saw nothing about Sync. This commit: - Adds dropUnconsentedSyncScope: on Firefox Desktop, the Sync scope counts as consented only when the resolved service is sync, or absent (old Desktop browsers or Mobile, which default to Sync). - Applies that one decision to both consumers. It keeps the row out of accountAuthorizations, and rides the auth code to /oauth/token — which never sees service= — so the access_token.created Glean event is tagged exclude_dau. The flag is stashed in Redis against the code's hash and expires with the code. - Leaves the grant and the issued tokens untouched, so Sync keeps working for these users. Recording real consent when the user turns Sync on in the browser needs a signal Firefox does not send; FXA-14295 covers that endpoint. An underlying fix is Desktop dropping scope= for ADR 0049 resolution. When that has been in Release long enough, lib/oauth/desktop-sync-consent-bandaid.ts can be deleted. Closes FXA-14263
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a Firefox Desktop–specific “bandaid” to avoid inflating Sync authorizations and Sync DAU when Desktop requests the Sync scope for non-Sync browser services (e.g., Smart Window). It keeps OAuth grants/tokens unchanged while preventing unconsented Sync consent rows and tagging token-created telemetry as exclude_dau via an authorization-code–scoped Redis flag.
Changes:
- Add
dropUnconsentedSyncScope+excludeDauCacheKeyhelper module and unit tests. - Update
/oauth/authorizationto (a) drop oldsync from consent writes for non-Sync Desktop flows and (b) write an exclude-DAU flag to Redis keyed by the auth-code hash. - Update
/oauth/tokento read the Redis flag during code redemption and tag the GleantokenCreatedevent accordingly; add route-level tests and a remote integration test covering real DB rows.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-auth-server/test/remote/account_consents.in.spec.ts | Adds an integration test ensuring non-Sync Desktop sign-ins don’t create an oldsync consent row but still mint tokens with oldsync scope. |
| packages/fxa-auth-server/lib/routes/oauth/token.spec.ts | Adds tests asserting /oauth/token reads the code-hash Redis flag and sets excludeDau on emitted metrics. |
| packages/fxa-auth-server/lib/routes/oauth/token.js | Reads the exclude-DAU Redis flag during authorization-code redemption and merges server-side/client-provided exclude_dau. |
| packages/fxa-auth-server/lib/routes/oauth/index.js | Wires authServerCacheRedis into the authorization route constructor. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.spec.ts | Adds tests for dropping oldsync from consent writes and for writing the exclude-DAU flag to Redis with code TTL. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.js | Implements service resolution helper, drops unconsented oldsync from ledger, sets grant.excludeDau, and writes the decision to Redis for /oauth/token to consume. |
| packages/fxa-auth-server/lib/oauth/desktop-sync-dau-authorization-bandaid.ts | New shared logic for dropping unconsented oldsync and generating the Redis cache key. |
| packages/fxa-auth-server/lib/oauth/desktop-sync-dau-authorization-bandaid.spec.ts | Unit tests for the new bandaid helper behaviors and cache key format. |
Comment on lines
+240
to
+256
| function shouldExcludeSyncDau(req, grant) { | ||
| const clientIdHex = hex(grant.clientId); | ||
| // Cheapest gate first. dropUnconsentedSyncScope checks this too, but | ||
| // short-circuit here if we can. | ||
| if (clientIdHex !== OAuthNativeClients.FirefoxDesktop) { | ||
| return false; | ||
| } | ||
| const requestedScopes = grant.scope.getScopeValues(); | ||
| if (requestedScopes.length === 0) { | ||
| return false; | ||
| } | ||
| return dropUnconsentedSyncScope({ | ||
| scopes: requestedScopes, | ||
| serviceValue: resolveServiceValue(req, requestedScopes), | ||
| clientIdHex: hex(grant.clientId), | ||
| }).droppedSyncScope; | ||
| } |
| }); | ||
|
|
||
| // The bandaid must touch the consent ledger and nothing else. | ||
| // Graned refresh and access token scopes should be untouched. |
Comment on lines
+96
to
+101
| } catch (err) { | ||
| statsd?.increment('accountAuthorization.exclude_dau_write_failed'); | ||
| log.warn('accountAuthorization.exclude_dau_write_failed', { | ||
| err: err.message, | ||
| }); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because:
This commit:
Recording real consent when the user turns Sync on in the browser needs a signal Firefox does not send; FXA-14295 covers that endpoint. An underlying fix is Desktop dropping scope= for ADR 0049 resolution. When that has been in Release long enough, lib/oauth/desktop-sync-consent-bandaid.ts can be deleted.
Closes FXA-14263
How to review (Optional)
Check out this doc, which has some test cases + screenshots of the account auth table, and an output from a temp tool I had Claude make to make grepping through auth-server DAU logs here less painful. (Please ping me if you'd like the script, otherwise you can search auth server logs for
exclude_dauand see the events)