feat(auth): enable otp by clientid and service#20205
Conversation
ddb4598 to
a7bb469
Compare
vbudhram
left a comment
There was a problem hiding this comment.
@StaberindeZA What are your thoughts about having a test to do the signup flow through a browser service? We have a test for signin, but for this feature seems useful.
It would be roughly combining those two tests.
a7bb469 to
d30a905
Compare
There was a problem hiding this comment.
Pull request overview
Enables passwordless OTP eligibility to be controlled by OAuth clientId + service, instead of a flat clientId allowlist, by propagating service through metrics context and /account/status calls.
Changes:
- Replace
allowedClientIdswithallowedClientServices(clientId → allowed services, with*wildcard) for passwordless OTP gating. - Plumb
servicethrough settings → auth-client → auth-server (/account/status) and into server-side metrics context. - Update passwordless route logic and local/unit tests to enforce client+service allowlisting.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signin/container.tsx | Passes service to accountStatusByEmail requests. |
| packages/fxa-settings/src/pages/Index/container.tsx | Passes service to accountStatusByEmail requests. |
| packages/fxa-auth-server/test/local/routes/passwordless.js | Updates local route tests to use allowedClientServices config shape. |
| packages/fxa-auth-server/test/local/routes/account.js | Updates local account route tests to use allowedClientServices. |
| packages/fxa-auth-server/lib/routes/utils/passwordless.ts | Implements clientId+service allowlist check with wildcard support. |
| packages/fxa-auth-server/lib/routes/utils/passwordless.spec.ts | Adds unit coverage for the new allowlist behavior. |
| packages/fxa-auth-server/lib/routes/passwordless.ts | Enforces allowlist using metricsContext.service for passwordless endpoints. |
| packages/fxa-auth-server/lib/routes/account.ts | Accepts service on /account/status and uses it for passwordless support checks. |
| packages/fxa-auth-server/lib/metrics/context.js | Extends metrics context schema to include service. |
| packages/fxa-auth-server/config/index.ts | Introduces PASSWORDLESS_ALLOWED_CLIENT_SERVICES and removes the old allowlist config field. |
| packages/fxa-auth-server/config/dev.json | Updates dev config example to the new allowlist structure. |
| packages/fxa-auth-client/lib/client.ts | Extends accountStatusByEmail options to include service. |
| libs/shared/metrics/glean/src/lib/metrics.context.ts | Adds service to shared MetricsContext and parses it from query params. |
| .circleci/config.yml | Adds PASSWORDLESS_ALLOWED_CLIENT_SERVICES to CI environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
8a1b485 to
8a3e204
Compare
Because: - Allow Passwordless OTP to be enabled for a clientId and then only specific services of that clientId or all services. This commit: - Adds a new env var, PASSWORDLESS_ALLOWED_CLIENT_SERVICES, to configure which client and service are allowed to use the passwordless OTP feature. - Removes env var, PASSWORDLESS_ALLOWED_CLIENTIDS env var. - Updates metricsContext to include service Closes #FXA-13178
8a3e204 to
525832a
Compare
vbudhram
left a comment
There was a problem hiding this comment.
@StaberindeZA Tested this locally
- Smartwindow signup (email code) ✅
- Sync signup (prompts for password) ✅
- Settings signup (email code) ✅
Once an account has email code enabled, they will always get it. I also tested the enable sync and set password flows and they worked, ![]()
Because
This pull request
Issue that this pull request solves
Closes: # FXA-13178
Checklist
Put an
xin the boxes that apply