feat(auth-server): replace accountAuthorizations with accountConsents#20590
feat(auth-server): replace accountAuthorizations with accountConsents#20590vbudhram wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous accountAuthorizations approach with a new accountConsents ledger in the OAuth DB, and wires token-exchange authorization to be consent- and config-driven (scope→service mapping plus deny/bypass policy), with new unit/integration coverage and migration patch-level updates.
Changes:
- Add
accountConsentsschema (and patch chain updates) and OAuth DB repository methods for recording/querying consents. - Record one consent row per scope on offline
/authorizationgrants and use consent-based decisions during/oauth/tokentoken-exchange. - Add integration/unit tests for the consent repository and token-exchange decision matrix; add new
oauthServer.exchange.*config blocks.
Reviewed changes
Copilot reviewed 15 out of 15 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 | New remote/integration coverage for repository behavior and decision matrix. |
| packages/fxa-auth-server/test/remote/account_consents_exchange.in.spec.ts | New remote/integration coverage for /oauth/token token-exchange behavior against accountConsents. |
| packages/fxa-auth-server/lib/routes/oauth/token.spec.ts | Updates unit tests to stub hasConsentForExchange and adds decision-matrix tests. |
| packages/fxa-auth-server/lib/routes/oauth/token.js | Replaces token-exchange allowlist logic with scope-by-scope consent decisions + metrics. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.js | Writes accountConsents rows on offline grants and adds service to payload validation. |
| packages/fxa-auth-server/lib/oauth/db/mysql/index.js | Adds MySQL queries and store methods for accountConsents upsert/find/list/delete. |
| packages/fxa-auth-server/lib/oauth/db/index.js | Adds OauthDB APIs for recording/reading consents and exchange decision logic driven by config. |
| packages/fxa-auth-server/config/index.ts | Introduces oauthServer.exchange config (serviceScopes map + deny/bypass lists). |
| packages/db-migrations/databases/fxa_oauth/target-patch.json | Bumps migration target patch level to 36. |
| packages/db-migrations/databases/fxa_oauth/patches/patch-033-034.sql | Reintroduces accountAuthorizations create migration (for chain completeness). |
| packages/db-migrations/databases/fxa_oauth/patches/patch-034-033.sql | Commented reverse patch for 34→33. |
| packages/db-migrations/databases/fxa_oauth/patches/patch-034-035.sql | Drops accountAuthorizations if present. |
| packages/db-migrations/databases/fxa_oauth/patches/patch-035-034.sql | Commented reverse patch for 35→34. |
| packages/db-migrations/databases/fxa_oauth/patches/patch-035-036.sql | Creates accountConsents table + index and advances patch level to 36. |
| packages/db-migrations/databases/fxa_oauth/patches/patch-036-035.sql | Commented reverse patch for 36→35. |
Comments suppressed due to low confidence (3)
packages/fxa-auth-server/lib/routes/oauth/token.js:458
- In
validateTokenExchangeGrant, afall-throughdecision currently just records a metric andcontinues, which effectively authorizes any unmapped scope for token-exchange. Sinceparams.scopeis only syntactically validated (converted to aScopeSet) and token-exchange does not authenticate a client, this becomes a scope-escalation path (any well-formed https:// scope would be granted). Consider enforcing that fall-through scopes are explicitly allowlisted (e.g., legacyoauthServer.tokenExchange.allowedScopes) and/or validated against the original OAuth client’sallowedScopesbefore allowing the exchange; otherwise reject withOauthError.forbidden().
continue;
}
seenServices.add(decision.service);
packages/fxa-auth-server/lib/routes/oauth/token.js:458
- The metric/log dedupe (
seenServices) is only applied for non-fall-throughdecisions. If a request includes multiple unmapped scopes, this code will emit multipleoauth.token_exchange.resolutionincrements forservice=legacy, which contradicts the nearby intent to dedupe per service. Consider addinglegacyto the dedupe set (or otherwise ensuring one metric per service) to avoid noisy metrics.
if (decision.result === 'fall-through') {
recordOutcome('legacy', 'granted_fall_through');
continue;
}
// Dedupe metric/log emissions when several scopes share a service.
if (seenServices.has(decision.service)) {
continue;
}
seenServices.add(decision.service);
packages/fxa-auth-server/lib/routes/oauth/token.spec.ts:774
- This unit test uses
reason: 'no-row'for a denied decision, but the productionOauthDB.hasConsentForExchangereturnsreason: 'no-consent'(and other tests/assertions use that wording). Aligning the test data with the real decision shape will reduce confusion and prevent accidental divergence.
existingDeviceId: MOCK_DEVICE_ID,
clientId: FIREFOX_IOS_CLIENT_ID,
}
);
expect(sessionTokenStub).not.toHaveBeenCalled();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // OauthDB.hasConsentForExchange, which resolves scope to service via | ||
| // the oauthServer.exchange.serviceScopes config map and applies the | ||
| // deny/bypass policy flags before consulting accountConsents. |
| @@ -0,0 +1,12 @@ | |||
| -- Recreates the accountAuthorizations table from the reverted level-34 | |||
| -- attempt so the forward chain is complete on environments still at 33. | |||
| -- Patch 34->35 drops the table; nothing in current code reads or writes it. | |||
| -- /authorization pre-prompt check and by token-exchange. Removed only on | ||
| -- account deletion. |
vbudhram
force-pushed
the
account-consent
branch
3 times, most recently
from
2500e1e to
5c58b5d
Compare
| -- consent acceptance (upserts, bumping lastAuthorizedTosAt). Read by the | ||
| -- /authorization pre-prompt check and by token-exchange. Cleared by | ||
| -- OauthDB.removeTokensAndCodes, which runs on account deletion and on | ||
| -- password reset. |
There was a problem hiding this comment.
This should not delete on password reset
There was a problem hiding this comment.
Ha, I was going to flag this after running the fxa-review skill. Should the comment be updated then?
| doc: 'Map from browser-service name to its authoritative scope URL. Drives token-exchange scope resolution, /authorization service= validation, and the per-RP ToS audit. Adding a new browser service is a config change plus deploy, with no schema migration required.', | ||
| format: Object, | ||
| default: { | ||
| sync: 'https://identity.mozilla.com/apps/oldsync', |
There was a problem hiding this comment.
Adding this config here saves us from having to add a database to do the service -> scope mapping. This mapping needs to live somewhere and this seemed the most straightforward way. We can always add a table later if there are more services.
| }); | ||
| }); | ||
|
|
||
| it('rejects unauthorized scopes', async () => { |
There was a problem hiding this comment.
These tests are moved below
vbudhram
force-pushed
the
account-consent
branch
2 times, most recently
from
9dc385a to
4aa099d
Compare
Because: - The accountAuthorizations design conflated consent tracking with session activity, sign-out cleanup, and per-scope revocation, which made the lifecycle hard to reason about. - A simpler ledger is enough: record one row per (uid, scope, service, clientId) on /authorization completion, keep it immutable until account deletion, and let the existing clients.allowedScopes plus a small policy config decide what scopes may be silently exchanged. - The new shape preserves cross-device silent sign-in (the URL service= signal is the consent bucket), supports per-RP ToS tracking via firstAuthorizedTosAt and lastAuthorizedTosAt, and keeps the existing Sync deny + Relay carve-out semantics. - Consent must survive password reset, so the sweep belongs in the account-delete path only, not in OauthDB.removeTokensAndCodes (which password reset also calls). - Consent is a property of the user's Allow decision, not of token persistence. The writer fires on every successful /authorization regardless of access_type; the previous offline-only gate was a holdover from the browser-services-only framing. - The scope-to-service mapping lives in auth-server Convict config, not on a column in the scopes table. Adding a new browser service is a config plus deploy, with no schema migration required. This commit: Schema (packages/db-migrations/databases/fxa_oauth): - patch-033-034.sql: restores the original accountAuthorizations CREATE TABLE that was reverted. Keeps the forward chain complete for environments still at level 33; stage (already at 34) skips it. - patch-034-035.sql: DROP TABLE IF EXISTS accountAuthorizations and bump dbMetadata to 35. - patch-035-036.sql: CREATE TABLE accountConsents with PK (uid, scope, service, clientId) plus idx_uid_service to support the token-exchange lookup. Bumps dbMetadata to 36. - patch-034-033.sql, patch-035-034.sql, patch-036-035.sql: commented rollback DDL per repo convention. - target-patch.json bumped from 33 to 36. Repository (packages/fxa-auth-server/lib/oauth/db): - Four MysqlStore methods on accountConsents: _upsertAccountConsent (preserves firstAuthorizedTosAt; bumps lastAuthorizedTosAt using GREATEST), _findAccountConsentForSignIn, _hasConsentForService, _deleteAllAccountConsentsForUser, plus _listAccountConsentsByUid for audit. - OauthDB wrappers: recordSignInConsent, hasConsentForSignIn, hasConsentForExchange (resolves scope to service via config map, applies deny/bypass policy, and returns allowed | bypass | denied silent-disallowed | denied no-consent | fall-through), deleteAllConsentsForUser, listAccountConsentsByUid. isKnownService is a synchronous in-memory Set lookup against the config-driven service map. - removeTokensAndCodes does NOT touch consent. Password reset reuses this path and must preserve the consent ledger. - AccountDeleteManager.deleteOAuthAccountData (renamed from deleteOAuthTokens) sweeps tokens, codes, and consent rows together. Consent removal is wired only here. Config (packages/fxa-auth-server/config/index.ts): - oauthServer.exchange.serviceScopes: map of service name to its canonical scope URL (sync, relay, smartwindow, vpn). Drives the exchange path scope-to-service resolution and backs isKnownService. - oauthServer.exchange.denySilentForServices (default sync): hard reject regardless of consent rows. - oauthServer.exchange.bypassConsentForServices (default relay): grant silent-exchange without consulting accountConsents (legacy carve-out, to be cleared once application-services lands a 4xx handler in iOS/Android Relay code). OAuth flow rewires: - lib/routes/oauth/authorization.js: on every successful /authorization (offline or online), awaits one accountConsents upsert per requested scope. service = URL service= when it appears in oauthServer.exchange.serviceScopes, otherwise empty. Bookkeeping is wrapped in try/catch so failures cannot break sign-in but still log via statsd. The accountConsent.recorded metric is tagged with access_type (offline/online) for dashboard visibility into the online vs offline write split. - lib/routes/oauth/token.js (validateTokenExchangeGrant): per-scope loop calling oauthDB.hasConsentForExchange. The switch on the decision has a default arm that fails closed on any unknown variant. Outcome names: granted, granted_relay_bypass, rejected_silent_disallowed, rejected_no_row, granted_fall_through, and rejected_not_in_allowed_scopes. Fall-through scopes are gated by the subject_token client allowedScopes so an exchange cannot inflate a token beyond what the issuing client was ever granted. - lib/routes/attached-clients.js: drops the destroy handler snapshot and cleanup plumbing for accountAuthorizations. The new ledger is immutable until account deletion. Tests: - test/remote/account_consents.in.spec.ts: covers isKnownService via an it.each truth table; the repository round-trip; timestamp preservation and bump behaviour; the hasConsentForExchange decision matrix; cross-device and cross-clientId cases; integration block driving createAuthorizationCode against /authorization; asserts that removeTokensAndCodes preserves consent rows (password reset path) while deleteAllConsentsForUser sweeps them; and asserts online grants write consent rows alongside offline ones. - test/remote/account_consents_exchange.in.spec.ts: end-to-end flow tests against /oauth/token grantOAuthTokens covering Relay bypass, SmartWindow allowed/rejected, VPN cross-device, Sync hard deny, multi-scope Sync-smuggling rejection, and unmapped-scope fall-through. - lib/routes/oauth/token.spec.ts: adds a default hasConsentForExchange stub to the in-arg mocks. Replaces the legacy "rejects unauthorized scopes" assertion with an equivalent test against the subject_token client allowedScopes in the new fall-through path. - lib/account-delete.spec.ts: asserts deleteAllConsentsForUser is called in both the full-delete and quickDelete flows. Also removes dead code from the prior accountAuthorizations implementation: the browserServices Convict block; the upsert/get/delete/deleteAll/list wrappers on OauthDB and the matching MysqlStore methods plus SQL constants; the snapshotAuthorizedServices and cleanupAccountAuthorizations shims in attached-clients; and the SUPERSEDED token-exchange describe.skip block in token.spec.ts.
LZoog
left a comment
LZoog
left a comment
There was a problem hiding this comment.
Started reviewing but haven't finished yet.
| @@ -0,0 +1,12 @@ | |||
| -- Recreates the accountAuthorizations table from the reverted level-34 | |||
| -- attempt so the forward chain is complete on environments still at 33. | |||
There was a problem hiding this comment.
Are you sure we need all this? I thought when we tag a new release to stage, since the table doesn't actually exist now and got dropped successfully, that the patcher wouldn't try to go backwards?
There was a problem hiding this comment.
Yea we do, the patcher looks at the current db level which is 35, so it will only go forward. These backwards patches are pulled from the train branch so that everything is in sync.
| `service` VARCHAR(64) NOT NULL DEFAULT '', | ||
| `clientId` BINARY(8) NOT NULL, | ||
| `firstAuthorizedTosAt` BIGINT UNSIGNED NOT NULL, | ||
| `lastAuthorizedTosAt` BIGINT UNSIGNED NOT NULL, |
There was a problem hiding this comment.
Could we call the table accountAuthorizations still? Or change the column names for these to be lastConsentedTosAt? I prefer authorize, but could live with consents if Nick/Wil like it too.
There was a problem hiding this comment.
I would second this - just being consistent between table name and columns would be ideal imo, though I do like the consent suffix as it is explicit that it tracks user consent to applications
There was a problem hiding this comment.
Going to stick with the consent name and update table column to be consistent.
| bypassConsentForServices: { | ||
| doc: 'Services granted at token-exchange without consulting accountConsents. Today: relay, pending application-services landing a 4xx handler in iOS/Android Relay code. To be cleared once that ships.', | ||
| format: Array, | ||
| default: ['relay'], |
There was a problem hiding this comment.
Let's go ahead and add vpn here. As soon as we ship to stage, they can test.
I don't think I understand Services granted at token-exchange without consulting accountConsents - the entire point is that it checks the account consent table and sees that the user has consented, then denies or grants the request?
I wonder also if we should find a way to reference "Project" in these docs somehow referring to the "project umbrella" that Google uses for their cross-platform identity model.
edit: yep this replaces that hardcoded allowlist so this is nicer and just needs a doc update 👍 https://github.com/mozilla/fxa/pull/20590/changes#r3249064091
We should go ahead and add VPN to the other allowlist.
There was a problem hiding this comment.
I wonder also if we should find a way to reference "Project" in these docs somehow referring to the "project umbrella" that Google uses for their cross-platform identity model.
Its a goal and the eventual state we want idk if needs to be in docs yet.
| return firstRow(rows) || null; | ||
| } | ||
|
|
||
| // True iff at least one consent row exists for (uid, service). Used |
There was a problem hiding this comment.
Did Claude make a typo with iff? 😀
There was a problem hiding this comment.
Nah, just means if and only if
| doc: 'Services for which silent token-exchange is always rejected, irrespective of accountConsents rows. Wins over bypass. Today: sync, the Sync refresh token is the keys-bearing master credential.', | ||
| format: Array, | ||
| default: ['sync'], | ||
| env: 'OAUTH_EXCHANGE_DENY_SILENT_FOR_SERVICES', |
There was a problem hiding this comment.
Since we have the allowlist that we have to specify services for, do we need this at all? Is it so we don't accidentally add Sync to the other list? 🤔
| // { result: 'allowed', service } | ||
| // { result: 'bypass', service } | ||
| // { result: 'denied', service, reason: 'silent-disallowed' | 'no-consent' } | ||
| // { result: 'fall-through' } |
There was a problem hiding this comment.
Should these be an enum? Would be great to have that with a JSDoc comment so we know what each does. Reading just this chunk, I don't think I get what 'bypass' is for. It literally checks that the service is in the allow list, but it doesn't check that the user has ever consented? (I haven't tested this out, just trying to read the code)
| } | ||
|
|
||
| module.exports = ({ log, oauthDB, config }) => { | ||
| module.exports = ({ log, oauthDB, config, statsd }) => { |
There was a problem hiding this comment.
Are the call sites for this updated to pass in statsd now too? I don't see changes for that
|
|
||
| const Client = clientFactory(); | ||
|
|
||
| const IOS = '1b1a3e44c54fbb58'; |
There was a problem hiding this comment.
👍 static test constants, love it!
| // the oauthServer.exchange.serviceScopes config map and applies the | ||
| // deny/bypass policy flags before consulting accountConsents. | ||
| const requestedScope = params.scope; | ||
| if (!TOKEN_EXCHANGE_ALLOWED_SCOPES.contains(requestedScope)) { |
There was a problem hiding this comment.
We need to leave this here until Mobile has error handling in place specifically for Relay, but, maybe this relates to this comment where I didn't understand the "bypass" piece? If this is meant only for Relay then we should have documentation very clearly noting that "bypassing" should never be allowed, and the only reason Relay is allowed now is because for Relay on Mobile MVP, they do a check of connected services first before they hit our endpoint to give them confidence the user has consented, and we are going to be removing the option to bypass in the very near future.
edit: yep this is the case 👍
| statsd?.increment('accountConsent.recorded', { | ||
| service: serviceValue || 'unset', | ||
| access_type: grant.offline ? 'offline' : 'online', | ||
| count: scopeValues.length, |
There was a problem hiding this comment.
Since scopes are pulled off the request, there's a chance for an unbound length here it looks like. Pretty minimal, but might be worth clamping it down to a maximum in some way, or just removing the count since I"m not sure what value the count would add to the metric
| // Record consent here, not at /oauth/token, so service= from the URL | ||
| // is available. Fires on every successful /authorization regardless | ||
| // of access_type. Errors are swallowed; bookkeeping cannot break sign-in. | ||
| try { |
There was a problem hiding this comment.
I see there are integration tests for account_consent which is great! But, I don't see any updates to unit tests for authorizations that ensure we're calling recordSignInConsent. Especially since this is JS, if the signature for recordSignInConsent changes, we won't catch that here - kind of a nit, but at least an updated unit test would be good!
| } | ||
|
|
||
|
|
||
| // Consent rows are written exclusively at /authorization, where |
There was a problem hiding this comment.
Is this supposed to be here?
| verify: jest.fn().mockResolvedValue({ user: UID }), | ||
| })); | ||
|
|
||
| const routes = require('./token')({ |
There was a problem hiding this comment.
I think we've talked about it before, but part of the reason for using an inline require like this is because of some of the code at runtime from requires needs mocked so it has to be done this way?
| .description(DESCRIPTION.acrValues), | ||
| assertion: Joi.forbidden(), | ||
| resource: Joi.forbidden(), | ||
| service: validators.service |
There was a problem hiding this comment.
Maybe I missed it, but I'm not seeing where this is being used in the handler, do we need it?
Because
accountAuthorizationsdesign mixed consent tracking with session activity and sign-out cleanup. The lifecycle was hard to follow, and the version that shipped to stage had to be reverted.firstAuthorizedTosAt/lastAuthorizedTosAt) and per-service revocation need a real ledger keyed on(uid, scope, service, clientId), not a side effect of refresh-token state.This pull request
accountConsentstable infxa_oauthvia three forward patches. 33→34 puts back the originalaccountAuthorizationsCREATE that was reverted, so the chain is complete. 34→35 dropsaccountAuthorizations. 35→36 createsaccountConsentswith anidx_uid_serviceindex that matches the token-exchange lookup.target-patch.jsongoes to 36.lib/routes/oauth/authorization.jswrites one consent row per requested scope on offline grants. Failures are caught and logged via statsd so they cannot break sign-in.lib/routes/oauth/token.js(validateTokenExchangeGrant) now callshasConsentForExchangeper scope. The decision returns one of:allowed,bypass,denied(with reasonsilent-disallowedorno-row), orfall-throughfor unmapped scopes. Unknown shapes fail closed.oauthServer.exchange:serviceScopes(the scope-to-service map),denySilentForServices(default['sync']), andbypassConsentForServices(default['relay']).browserServicesConvict block, theaccountAuthorizationsrepository methods and SQL constants, and thesnapshotAuthorizedServices/cleanupAccountAuthorizationsshims inattached-clients.js. The unusedstatsdparameter onattached-clientsis gone too, no caller ever passed it.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13736
Checklist
Other information
How to test:
nx run db-migrations:migrateand checkdbMetadatashowsschema-patch-level=36, thataccountConsentsexists, and thataccountAuthorizationsdoes not.nx test-unit fxa-auth-server, including the newdecision matrixdescribe block.account_consents.in.spec.tsandaccount_consents_exchange.in.spec.tsagainst local infra.?service=smartwindow&access_type=offline, accept the consent prompt, and confirm a row lands inaccountConsents. From a second device, token-exchange the same scope should now succeed.