Merge pull request #17851 from mozilla/FXA-10416

feat(auth): Update 'sync' service references to account for 'relay'

packages/fxa-auth-server/lib/routes/account.ts

@@ -249,6 +249,8 @@ export class AccountHandler {
 
   private setMetricsFlowCompleteSignal(request: AuthRequest, service?: string) {
     let flowCompleteSignal;
+    // 'account.signed' is only used for 'sync'
+    // use the default for browser sign-ins that are not sync (e.g., service=relay)
     if (service === 'sync') {
       flowCompleteSignal = 'account.signed';
     } else {

packages/fxa-auth-server/lib/routes/utils/signup.js

@@ -65,8 +65,16 @@ module.exports = (log, db, mailer, push, verificationReminders, glean) => {
 
       // Our post-verification email is very specific to sync,
       // so only send it if we're sure this is for sync or sync scoped client.
+      // Do not send for browser sign-ins that are not sync.
       const scopeSet = ScopeSet.fromArray(scopes);
-      if (service === 'sync' || scopeSet.intersects(NOTIFICATION_SCOPES)) {
+      if (
+        service === 'sync' ||
+        // if legacy sync scope is included, only consider the service
+        // to be sync if there is no service specified
+        // (we already accounted for service === 'sync' above,
+        // so any other service will not be sync)
+        (scopeSet.intersects(NOTIFICATION_SCOPES) && !service)
+      ) {
         const onMobileDevice = request.app.ua.deviceType === 'mobile';
         const mailOptions = {
           acceptLanguage: request.app.acceptLanguage,

packages/fxa-auth-server/lib/senders/email.js

@@ -552,8 +552,7 @@ module.exports = function (log, config, bounces) {
       'X-Verify-Code': message.code,
     };
 
-    const clientInfo = await oauthClientInfo.fetch(message.service);
-    const serviceName = clientInfo.name;
+    const { name: serviceName } = await oauthClientInfo.fetch(message.service);
 
     return this.send({
       ...message,
@@ -771,7 +770,7 @@ module.exports = function (log, config, bounces) {
     });
   };
 
-  Mailer.prototype.verifyLoginEmail = function (message) {
+  Mailer.prototype.verifyLoginEmail = async function (message) {
     log.trace('mailer.verifyLoginEmail', {
       email: message.email,
       uid: message.uid,
@@ -805,40 +804,31 @@ module.exports = function (log, config, bounces) {
       'X-Verify-Code': message.code,
     };
 
-    return oauthClientInfo.fetch(message.service).then((clientInfo) => {
-      let clientName = clientInfo.name;
-      const [time, date] = this._constructLocalTimeString(
-        message.timeZone,
-        message.acceptLanguage
-      );
+    const { name: clientName } = await oauthClientInfo.fetch(message.service);
 
-      /**
-       * Edge case. We assume the service is firefox, which is true when the service is sync. However, if
-       * the service is not sync we must be more general. A user could be signing directly into settings.
-       */
-      if (clientName === 'Firefox' && message.service !== 'sync') {
-        clientName = 'Mozilla';
-      }
+    const [time, date] = this._constructLocalTimeString(
+      message.timeZone,
+      message.acceptLanguage
+    );
 
-      return this.send({
-        ...message,
-        headers,
-        template: templateName,
-        templateValues: {
-          clientName,
-          date,
-          device: this._formatUserAgentInfo(message),
-          email: message.email,
-          link: links.link,
-          oneClickLink: links.oneClickLink,
-          passwordChangeLink: links.passwordChangeLink,
-          passwordChangeLinkAttributes: links.passwordChangeLinkAttributes,
-          privacyUrl: links.privacyUrl,
-          supportLinkAttributes: links.supportLinkAttributes,
-          supportUrl: links.supportUrl,
-          time,
-        },
-      });
+    return this.send({
+      ...message,
+      headers,
+      template: templateName,
+      templateValues: {
+        clientName,
+        date,
+        device: this._formatUserAgentInfo(message),
+        email: message.email,
+        link: links.link,
+        oneClickLink: links.oneClickLink,
+        passwordChangeLink: links.passwordChangeLink,
+        passwordChangeLinkAttributes: links.passwordChangeLinkAttributes,
+        privacyUrl: links.privacyUrl,
+        supportLinkAttributes: links.supportLinkAttributes,
+        supportUrl: links.supportUrl,
+        time,
+      },
     });
   };
 
@@ -879,15 +869,7 @@ module.exports = function (log, config, bounces) {
       'X-Signin-Verify-Code': message.code,
     };
 
-    let { name: serviceName } = await oauthClientInfo.fetch(message.service);
-
-    /**
-     * Edge case. We assume the service is firefox, which is true when the service is sync. However, if
-     * the service is not sync we must be more general. A user could be signing directly into settings.
-     */
-    if (serviceName === 'Firefox' && message.service !== 'sync') {
-      serviceName = 'Mozilla';
-    }
+    const { name: serviceName } = await oauthClientInfo.fetch(message.service);
 
     return this.send({
       ...message,
@@ -1230,7 +1212,7 @@ module.exports = function (log, config, bounces) {
     });
   };
 
-  Mailer.prototype.newDeviceLoginEmail = function (message) {
+  Mailer.prototype.newDeviceLoginEmail = async function (message) {
     log.trace('mailer.newDeviceLoginEmail', {
       email: message.email,
       uid: message.uid,
@@ -1242,39 +1224,28 @@ module.exports = function (log, config, bounces) {
       'X-Link': links.passwordChangeLink,
     };
 
-    return oauthClientInfo.fetch(message.service).then((clientInfo) => {
-      let clientName = clientInfo.name;
-      const [time, date] = this._constructLocalTimeString(
-        message.timeZone,
-        message.acceptLanguage
-      );
-
-      /**
-       * Edge case. We assume the service is firefox, which is true when the service is sync. However, if
-       * the service is not sync we must be more general. A user could be signing directly into settings.
-       */
-      if (clientName === 'Firefox' && message.service !== 'sync') {
-        clientName = 'Mozilla';
-      }
-
-      return this.send({
-        ...message,
-        headers,
-        template: templateName,
-        templateValues: {
-          clientName,
-          date,
-          device: this._formatUserAgentInfo(message),
+    const { name: clientName } = await oauthClientInfo.fetch(message.service);
+    const [time, date] = this._constructLocalTimeString(
+      message.timeZone,
+      message.acceptLanguage
+    );
 
-          link: links.link,
-          passwordChangeLink: links.passwordChangeLink,
-          passwordChangeLinkAttributes: links.passwordChangeLinkAttributes,
-          privacyUrl: links.privacyUrl,
-          supportLinkAttributes: links.supportLinkAttributes,
-          supportUrl: links.supportUrl,
-          time,
-        },
-      });
+    return this.send({
+      ...message,
+      headers,
+      template: templateName,
+      templateValues: {
+        clientName,
+        date,
+        device: this._formatUserAgentInfo(message),
+        link: links.link,
+        passwordChangeLink: links.passwordChangeLink,
+        passwordChangeLinkAttributes: links.passwordChangeLinkAttributes,
+        privacyUrl: links.privacyUrl,
+        supportLinkAttributes: links.supportLinkAttributes,
+        supportUrl: links.supportUrl,
+        time,
+      },
     });
   };
 

packages/fxa-auth-server/lib/senders/oauth_client_info.js

@@ -13,6 +13,10 @@ module.exports = (log, config) => {
   const FIREFOX_CLIENT = {
     name: 'Firefox',
   };
+
+  const MOZILLA_CLIENT = {
+    name: 'Mozilla',
+  };
   // TODO: prob don't need this cache anymore now that it's just a db call
   const clientCache = new Keyv({
     ttl: OAUTH_CLIENT_INFO_CACHE_TTL,
@@ -22,18 +26,29 @@ module.exports = (log, config) => {
   /**
    * Fetches OAuth client info from the OAuth server.
    * Stores the data into server memory.
-   * @param clientId
+   * @param service
    * @returns {Promise<any>}
    */
-  async function fetch(clientId) {
+  async function fetch(service) {
     log.trace('fetch.start');
 
-    if (!clientId || clientId === 'sync') {
-      log.trace('fetch.sync');
+    // Default to 'Mozilla' if the service is undefined
+    // If the service is undefined for a sync sign-in (where scope is provided but not service),
+    // this may result in some edge cases where the client name is 'Mozilla' instead of 'Firefox' in emails
+    // however, defaulting to Mozilla works best for web sign-ins with other browsers (e.g. Chrome)
+    // We might want to consider passing in scopes as well to more accurately determine the client name
+    if (!service) {
+      log.trace('fetch.noService');
+      return MOZILLA_CLIENT;
+    }
+
+    // Set the client name to 'Firefox' if the service is browser-based
+    if (service === 'sync' || service === 'relay') {
+      log.trace('fetch.firefoxClient');
       return FIREFOX_CLIENT;
     }
 
-    const cachedRecord = await clientCache.get(clientId);
+    const cachedRecord = await clientCache.get(service);
     if (cachedRecord) {
       // used the cachedRecord if it exists
       log.trace('fetch.usedCache');
@@ -42,21 +57,22 @@ module.exports = (log, config) => {
 
     let clientInfo;
     try {
-      clientInfo = await client.getClientById(clientId);
+      clientInfo = await client.getClientById(service);
     } catch (err) {
       // fallback to the Firefox client if request fails
       if (!err.statusCode) {
         log.fatal('fetch.failed', { err });
       } else {
-        log.warn('fetch.failedForClient', { clientId });
+        log.warn('fetch.failedForClient', { service });
       }
-      return FIREFOX_CLIENT;
+      // default to 'Mozilla' if there is an error fetching client info
+      return MOZILLA_CLIENT;
     }
 
     log.trace('fetch.usedServer', { body: clientInfo });
     // We deliberately don't wait for this to resolve, since the
     // client doesn't need to wait for us to write to the cache.
-    clientCache.set(clientId, clientInfo);
+    clientCache.set(service, clientInfo);
     return clientInfo;
   }
 

packages/fxa-auth-server/test/local/senders/oauth_client_info.js

@@ -48,9 +48,9 @@ describe('lib/senders/oauth_client_info:', () => {
       return clientInfo.__clientCache.clear();
     });
 
-    it('returns Firefox if no client id', () => {
+    it('returns Mozilla if no service', () => {
       return fetch().then((res) => {
-        assert.equal(res.name, 'Firefox');
+        assert.equal(res.name, 'Mozilla');
       });
     });
 
@@ -60,9 +60,15 @@ describe('lib/senders/oauth_client_info:', () => {
       });
     });
 
-    it('falls back to Firefox if error', () => {
-      return fetch('0000000000000000').then((res) => {
+    it('returns Firefox if service=relay', () => {
+      return fetch('relay').then((res) => {
         assert.equal(res.name, 'Firefox');
+      });
+    });
+
+    it('falls back to Mozilla if error', () => {
+      return fetch('0000000000000000').then((res) => {
+        assert.equal(res.name, 'Mozilla');
         assert.ok(mockLog.fatal.calledOnce, 'called fatal log');
       });
     });

packages/fxa-settings/src/models/integrations/base-integration.ts

@@ -176,6 +176,10 @@ export abstract class Integration<
     return this.data.service;
   }
 
+  getClientId() {
+    return this.data.clientId;
+  }
+
   isTrusted() {
     return true;
   }

packages/fxa-settings/src/models/integrations/oauth-web-integration.test.ts

@@ -174,9 +174,16 @@ describe('models/integrations/oauth-relier', function () {
   });
 
   describe('getService', () => {
-    it('returns clientId as service', () => {
+    it('returns service', () => {
+      model.data.modelData.set('service', 'sync');
+      expect(model.getService()).toBe('sync');
+    });
+  });
+
+  describe('getClientId', () => {
+    it('returns clientId', () => {
       model.data.modelData.set('client_id', '123');
-      expect(model.getService()).toBe('123');
+      expect(model.getClientId()).toBe('123');
     });
   });
 

packages/fxa-settings/src/models/integrations/oauth-web-integration.ts

@@ -222,6 +222,10 @@ export class OAuthWebIntegration extends BaseIntegration {
   }
 
   getService() {
+    return this.data.service;
+  }
+
+  getClientId() {
     return this.data.clientId;
   }
 

packages/fxa-settings/src/pages/Index/index.tsx

@@ -21,7 +21,7 @@ export const Index = ({
   integration,
   serviceName,
 }: IndexProps & RouteComponentProps) => {
-  const clientId = integration.getService();
+  const clientId = integration.getClientId();
   const isSync = integration.isSync();
   const isOAuth = isOAuthIntegration(integration);
   const isPocketClient = isOAuth && isClientPocket(clientId);

packages/fxa-settings/src/pages/Index/interfaces.ts

@@ -7,7 +7,7 @@ import { Integration } from '../../models';
 
 export type IndexIntegration = Pick<
   Integration,
-  'type' | 'isSync' | 'getService'
+  'type' | 'isSync' | 'getClientId'
 >;
 
 export interface IndexProps {

packages/fxa-settings/src/pages/Index/mocks.tsx

@@ -5,7 +5,7 @@
 import React from 'react';
 import { LocationProvider } from '@reach/router';
 import { MozServices } from '../../lib/types';
-import { IntegrationType, OAuthIntegration } from '../../models';
+import { IntegrationType } from '../../models';
 import { IndexIntegration } from './interfaces';
 import Index from '.';
 import { MOCK_CLIENT_ID } from '../mocks';
@@ -16,22 +16,22 @@ export function createMockIndexOAuthIntegration({
   return {
     type: IntegrationType.OAuthWeb,
     isSync: () => false,
-    getService: () => clientId,
+    getClientId: () => clientId,
   };
 }
 export function createMockIndexSyncIntegration(): IndexIntegration {
   return {
     type: IntegrationType.OAuthNative,
     isSync: () => true,
-    getService: () => MOCK_CLIENT_ID,
+    getClientId: () => MOCK_CLIENT_ID,
   };
 }
 
 export function createMockIndexWebIntegration(): IndexIntegration {
   return {
     type: IntegrationType.Web,
     isSync: () => false,
-    getService: () => undefined,
+    getClientId: () => undefined,
   };
 }