Conversation
There was a problem hiding this comment.
Pull request overview
Implements ADR 0049 by moving “source of truth” for OAuthNative requested scopes to the auth-server when scope= is omitted (resolved from service=), and threads the granted scope back through fxa-settings and WebChannel so Firefox can rely on the server-granted scope.
Changes:
- Auth-server: adds
/oauth/authorizationgate to resolve scopes fromservice=for OAuthNative clients, conditionally appendingapps/oldsyncwhenkeys_jweis present; always echoes grantedscopein authorization responses. - fxa-settings: propagates server-returned
scopetofxaOAuthLogin(renamingscopes→scope), and updates query-param validation to allow OAuthNative URLs withoutscope=. - Tests: expands unit/route coverage and adds functional tests + shared scope constants to validate end-to-end scope resolution and WebChannel payloads.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/interfaces.ts | Removes getGrantedScopes from integration picks. |
| packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/index.tsx | Threads scope from finishOAuthFlowHandler into fxaOAuthLogin. |
| packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/index.test.tsx | Updates expectations to include scope in fxaOAuthLogin payload. |
| packages/fxa-settings/src/pages/Signin/utils.ts | Sends scope (not scopes) in fxaOAuthLogin and threads through OAuth navigation state. |
| packages/fxa-settings/src/pages/Signin/utils.test.ts | Updates assertions to include scope. |
| packages/fxa-settings/src/pages/Signin/SigninUnblock/mocks.tsx | Removes getGrantedScopes from mocks. |
| packages/fxa-settings/src/pages/Signin/SigninTotpCode/mocks.tsx | Removes getGrantedScopes from mocks. |
| packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx | Threads scope from OAuth finish handler into fxaOAuthLogin. |
| packages/fxa-settings/src/pages/Signin/mocks.tsx | Removes getGrantedScopes from mocks. |
| packages/fxa-settings/src/pages/Signin/interfaces.ts | Removes getGrantedScopes from integration interface picks. |
| packages/fxa-settings/src/pages/Signin/index.test.tsx | Updates OAuth login expectations to include scope. |
| packages/fxa-settings/src/pages/Signin/container.tsx | Adds OAuthNative query-param validator where scope is optional. |
| packages/fxa-settings/src/pages/Signin/container.test.tsx | Verifies correct validator selection for native vs web RP flows. |
| packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/container.tsx | Threads scope to fxaOAuthLogin after reset-password OAuth completion. |
| packages/fxa-settings/src/pages/PostVerify/SetPassword/container.test.tsx | Updates OAuth login payload assertions to use scope. |
| packages/fxa-settings/src/pages/mocks.tsx | Adds scope to shared OAuth flow handler mock response. |
| packages/fxa-settings/src/pages/Index/mocks.tsx | Removes getGrantedScopes and updates OAuth handler mock to include scope. |
| packages/fxa-settings/src/pages/Index/interfaces.ts | Removes getGrantedScopes from integration picks. |
| packages/fxa-settings/src/models/pages/signin/oauth-query-params.ts | Splits web vs native OAuth query validation; native scope becomes optional. |
| packages/fxa-settings/src/models/integrations/oauth-native-integration.ts | Supports missing URL scope (empty normalized scope) and adjusts keys-request logic. |
| packages/fxa-settings/src/models/integrations/oauth-native-integration.test.ts | Adds tests for missing-scope behavior and keys decision logic. |
| packages/fxa-settings/src/models/integrations/integration.ts | Removes base getGrantedScopes API. |
| packages/fxa-settings/src/lib/oauth/hooks.tsx | Omits scope when empty (to trigger server resolution) and returns granted scope from server response. |
| packages/fxa-settings/src/lib/integrations/integration-factory.test.ts | Removes getGrantedScopes test (API removed). |
| packages/fxa-settings/src/lib/channels/firefox.ts | Renames WebChannel OAuth login field scopes → scope. |
| packages/fxa-settings/src/components/Settings/ConnectedServices/Service.tsx | Uses shared OAUTH_NATIVE_CLIENT_IDS set to detect native clients. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.spec.ts | Adds route-level tests for service-driven scope resolution gate + consent row behavior. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.js | Implements service-driven scope resolution and echoes granted scope in responses. |
| packages/fxa-auth-server/lib/oauth/db/resolve-scopes-for-service.spec.ts | Unit tests for service→scopes resolver and conditional scope append. |
| packages/fxa-auth-server/lib/oauth/db/resolve-scopes-for-service.js | Implements service scope resolution helper with conditional scope append + dedupe. |
| packages/fxa-auth-server/lib/oauth/db/index.js | Wires resolver into OauthDB and loads new authorization config. |
| packages/fxa-auth-server/lib/metrics/client-tags.ts | Reuses shared OAUTH_NATIVE_CLIENT_IDS set for service tagging. |
| packages/fxa-auth-server/config/index.ts | Adds oauthServer.authorization.serviceScopes and keysConditionalScope config. |
| packages/fxa-auth-client/lib/client.ts | Updates /oauth/authorization return type to include granted scope. |
| packages/functional-tests/tests/passkeyAuth/passkeyPasswordFallback.spec.ts | Updates WebChannel scope assertion helper usage and uses shared scope constants. |
| packages/functional-tests/tests/oauth/serverSideScopeResolution.spec.ts | Adds end-to-end functional tests for server-side scope resolution behavior. |
| packages/functional-tests/tests/misc/vpnIntegration.spec.ts | Updates WebChannel scope assertions to new payload field and shared constants. |
| packages/functional-tests/pages/layout.ts | Renames scope assertion helper to checkWebChannelMessageScope and reads scope field. |
| packages/functional-tests/lib/scopes.ts | Introduces shared scope constants for functional tests. |
| packages/functional-tests/lib/query-params.ts | Refactors query param builders to reuse shared scope constants; adds no-scope variants. |
| libs/accounts/oauth/src/lib/oauth.ts | Exposes shared OAUTH_NATIVE_CLIENT_IDS allowlist set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const scope = await getOAuthLoginScope(signin); | ||
| expect(scope).toEqual( | ||
| expect.arrayContaining([SMARTWINDOW_SCOPE, PROFILE_UID_SCOPE]) |
There was a problem hiding this comment.
REALLY like seeing this! I do wonder if we have coverage for this lower in integration or unit tests, but I can see the argument for including these functional tests since they exercise the webchannel messages
| resolveScopesForService(SCOPES, OLDSYNC, 'totally-unknown', false) | ||
| ).toBeUndefined(); | ||
| // Even with keys, an unknown service has no resolution. | ||
| expect( |
There was a problem hiding this comment.
small nit, these should be two separate tests, but also the second isn't really necessary since we check check serviceScopes first and so withKeys has no impact
| VPN, | ||
| PROFILE, | ||
| ]); | ||
| expect( |
There was a problem hiding this comment.
small nit, similar to the above that could be split I'm not sure the extra expect here is necessary. If the first expectation exercise the expected code path, the second just becomes another failure point. Unless I'm missing a reason to include the two explicit tests? In which case, I would still suggest it's split up
There was a problem hiding this comment.
maybe a dumb question - why js and not ts?
There was a problem hiding this comment.
Not a dumb question. Claude made this a JS file, and I meant to actually ask it to use TS. We have other JS files that import TS, and there were so many other things I needed Claude to change that I forgot to circle back to this one so good call out!
nshirley
left a comment
There was a problem hiding this comment.
Thanks for this! I tested as many browser services as I could while omitting the scope parameter from the url and it appears to work as intended for signing IN and UP in those flows.
75cbbb9 to
59a935b
Compare
Because: - Firefox-native OAuth flows hard-code requested scopes on the client. ADR 0049 moves the source of truth to the auth-server: a Firefox URL can omit scope= and the server resolves the full set from service=, appending apps/oldsync when the user enters a password. This commit: - /oauth/authorization resolves scope from service= for OAuthNative clients when scope= is omitted. Returns invalid_request(scope) for non-native clients and invalid_request(service) for unknown or disallowed service/client pairs. - Server emits the full scope set per oauthServer.authorization .serviceScopes; keysConditionalScope (apps/oldsync) is appended when keys_jwe is in the payload, preserving today's "non-Sync browser service + password derives scoped keys" behavior. - Both /authorization and /oauth/authorization responses now echo the granted scope per RFC 6749 §5.1. - fxa-settings threads the returned scope to fxaOAuthLogin (renamed scopes → scope to match the OAuth spec). OAuthNativeIntegration overrides getNormalizedScope / _scopeRequestsKeys to support the no-URL-scope case; URL-scope-present path delegates to super.* and is preserved.. - Adds shared OAUTH_NATIVE_CLIENT_IDS set in libs/accounts/oauth, and creates a scopes file in functional-tests for shared scopes closes FXA-13495
Because:
This commit:
closes FXA-13495
How to review (Optional)
scope=parameter, including cached sign-in and sign-in with password, to ensure things work as expected. Since Desktop just uses the session token to create access tokens though, it's hard to actually test this until Desktop uses a refresh token and pulls these scope values out from the web channel message, or until Mobile does their Sync decoupling work, so we can just verify the correct scope is sent to them in the web channel message