Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the refreshToken auth scheme used for the Devices API so that device registration/management is allowed based on an allowlist of Firefox OAuth client_ids, rather than requiring the legacy oldsync scope. This supports Firefox sign-ins that obtain refresh tokens without Sync scope, while still permitting device registration.
Changes:
- Replace the refresh-token scheme gate from “must include oldsync scope” to “must be an allowlisted Firefox client_id”.
- Add config for
oauth.deviceRegistrationClientIds(with default Firefox client IDs) to control which clients may use refresh tokens for device operations. - Add/adjust unit + integration tests to cover allowlisted client behavior with non-Sync scopes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/fxa-auth-server/test/remote/device_tests_refresh_tokens.in.spec.ts | Adds an integration test ensuring device registration works with a Firefox client_id refresh token that lacks oldsync scope. |
| packages/fxa-auth-server/lib/routes/auth-schemes/refresh-token.spec.ts | Updates auth-scheme tests to validate allowlisted vs non-allowlisted client_id behavior and allow non-Sync scopes for allowlisted clients. |
| packages/fxa-auth-server/lib/routes/auth-schemes/refresh-token.js | Implements the client_id allowlist gate for refresh-token authentication to device endpoints. |
| packages/fxa-auth-server/config/index.ts | Introduces oauth.deviceRegistrationClientIds convict config with defaults for Firefox clients. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default: ['5882386c6d801776', '1b1a3e44c54fbb58'], | ||
| env: 'OAUTH_OLD_SYNC_CLIENT_IDS', | ||
| }, | ||
| deviceRegistrationClientIds: { |
There was a problem hiding this comment.
Just noting here that this is similar to the converstation, what belongs in database vs config. Since its a client capability, makes sense to be added to the clients table long term. Might be useful to add to our database cleanup epic.
| default: ['5882386c6d801776', '1b1a3e44c54fbb58'], | ||
| env: 'OAUTH_OLD_SYNC_CLIENT_IDS', | ||
| }, | ||
| deviceRegistrationClientIds: { |
There was a problem hiding this comment.
The name here is misleading. This allows clients to manage devices, including register, send commands, revoke etc. If we only intend to do the registration it might be better to have this check in the registration route.
There was a problem hiding this comment.
Thank you for calling this out, really appreciate it. After looking into it and DM'ing you/Mark it sounds like this is what we want but I'll update the config name at least and file an issue to pull this into admin panel.
Because: * Currently, the only OAuth scope allowed to register/manage a device via refresh token is the Sync scope and other scopes are explicitly rejected by the refreshToken auth scheme's scope allowlist. When Firefox logins occur without Sync but with a refresh token, we still want the device registered. This commit: * Changes the gate for device management from a refresh token with the Sync scope, to a refresh token tied to a Firefox client ID closes FXA-13704
LZoog
changed the title
vbudhram
left a comment
vbudhram
left a comment
There was a problem hiding this comment.
Discussed that this was good to land 👍🏽
Because:
This commit:
closes FXA-13704