Conversation
Because: * In the verifyLoginCode email, Smart Window and VPN say '... Mozilla?' (the default) in the subject line/body copy, and we want to be consistent with Sync and Relay and focus on '... Firefox?' wording This commit: * Changes the hardcoded check against sync/relay to include the OAuthNativeServices enum to return 'Firefox' as the name closes FXA-13080
There was a problem hiding this comment.
Pull request overview
Updates how oauth_client_info decides when to label an OAuth flow as coming from “Firefox” (vs “Mozilla”) by switching from a hardcoded sync/relay check to an OAuthNativeServices allowlist, to make Smart Window and VPN emails consistent.
Changes:
- Use
OAuthNativeServicesto classify “native Firefox services” and return theFirefoxclient name for all of them. - Add tests asserting
smartwindowandvpnresolve toFirefox.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/senders/oauth_client_info.js | Switches Firefox/Mozilla naming logic to use OAuthNativeServices instead of hardcoded service strings. |
| packages/fxa-auth-server/lib/senders/oauth_client_info.spec.ts | Adds test cases for smartwindow and vpn returning Firefox. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('returns Firefox if service=smartwindow', async () => { | ||
| const res = await fetch('smartwindow'); | ||
| expect(res.name).toBe('Firefox'); | ||
| }); | ||
|
|
||
| it('returns Firefox if service=vpn', async () => { | ||
| const res = await fetch('vpn'); | ||
| expect(res.name).toBe('Firefox'); | ||
| }); |
There was a problem hiding this comment.
The new smartwindow/vpn assertions will pass even if the code doesn't treat those services as native, because the client mock’s default branch returns { name: 'Firefox' } for any unknown id. To validate the intended behavior, make the mock return a non-Firefox name for these values (or spy on getClientById and assert it is not called) so the test fails without the native-services short-circuit.
| const client = require('../oauth/client'); | ||
| const { OAuthNativeServices } = require('@fxa/accounts/oauth'); | ||
|
|
There was a problem hiding this comment.
New runtime dependency added via require('@fxa/accounts/oauth'), but packages/fxa-auth-server/package.json does not list @fxa/accounts/oauth. This can break installs that focus/pack auth-server (e.g., Yarn workspaces focus/production) with MODULE_NOT_FOUND. Add @fxa/accounts/oauth as a workspace dependency of fxa-auth-server.
| // Set the client name to 'Firefox' if the service is a native Firefox service | ||
| const nativeServices = new Set(Object.values(OAuthNativeServices)); | ||
| if (nativeServices.has(service)) { |
There was a problem hiding this comment.
nativeServices is re-created on every fetch() call. Since OAuthNativeServices is static, build the Set once (e.g., in the module factory scope) and reuse it to avoid repeated allocations on a hot path.
Because:
This commit:
closes FXA-13080
This is easiest to check in Maildev by running
yarn pm2 stop inboxandsudo maildev -s 9999then checking port 1080 to compare.Please see the ticket for a comment with a chart of the state of things before this change. This affects the email code email (you may need to use a
@mozillaemail locally and setSIGNIN_CONFIRMATION_SKIP_FOR_NEW_ACCOUNTStofalse) and the new device email.I messaged Ipsita and Ross for how we want to make these consistent and they prefer the "Firefox" wording.