Merge pull request #20106 from mozilla/FXA-13176

feat(auth): Disallow oauth exchange in non-Sync non-2FA unverified session state via config

Author: LZoog

packages/fxa-auth-server/config/index.ts

@@ -1682,7 +1682,7 @@ const convictConf = convict({
   },
   signinConfirmation: {
     forcedEmailAddresses: {
-      doc: 'Force sign-in confirmation for email addresses matching this regex for those that do not request scoped keys. Sets "mustVerify: 0" on created session tokens but creates an entry in unverifiedTokens, simulating an unverified session state',
+      doc: 'Force sign-in confirmation for email addresses matching this regex for those that do not request scoped keys. Sets "mustVerify: 0" on created session tokens but creates an entry in unverifiedTokens, simulating a non-Sync non-2FA unverified session state',
       format: RegExp,
       default: /.+@mozilla\.com$/,
       env: 'SIGNIN_CONFIRMATION_FORCE_EMAIL_REGEX',
@@ -1695,7 +1695,7 @@ const convictConf = convict({
     },
     skipForNewAccounts: {
       enabled: {
-        doc: 'Skip sign-in confirmation for newly-created accounts. Use this in tandem with forcedEmailAddresses',
+        doc: 'Skip all sign-in email confirmations for newly-created accounts',
         default: true,
         env: 'SIGNIN_CONFIRMATION_SKIP_FOR_NEW_ACCOUNTS',
       },
@@ -1859,8 +1859,8 @@ const convictConf = convict({
     env: 'SIGNIN_CODE_SIZE',
   },
   servicesWithEmailVerification: {
-    doc: 'Services that will alwasy send a session verification email on sign in',
-    default: ['e6eb0d1e856335fc'],
+    doc: 'For users in a non-2FA non-Sync unverified session state going through an RP redirect flow, we skip 1) redirecting the user to enter emailed code verification page, and we skip 2) sending that email verification. Services in this list will NOT skip the redirect or email, and are blocked in the BE from completing the oauth flow by hitting our API directly in this state. Content-server has this config as well.',
+    default: ['32aaeb6f1c21316a'],
     format: Array,
     env: 'SERVICES_WITH_EMAIL_VERIFICATION',
   },

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

@@ -1299,15 +1299,12 @@ export class AccountHandler {
       // If the request wants keys , user *must* confirm their login session before they can actually
       // use it. Otherwise, they don't *have* to verify their session. All sessions are created
       // unverified because it prevents them from being used for sync.
-      // Also require verification if the service is in the servicesWithEmailVerification list.
       let mustVerifySession =
         needsVerificationId &&
         (verificationForced === 'suspect' ||
           verificationForced === 'global' ||
           verificationForced === 'email' ||
-          requestHelper.wantsKeys(request) ||
-          (service &&
-            this.config.servicesWithEmailVerification.includes(service)));
+          requestHelper.wantsKeys(request));
 
       // For accounts with TOTP, we always force verifying a session.
       if (verificationMethod === 'totp-2fa') {
@@ -2382,16 +2379,20 @@ export class AccountHandler {
       this.db.getRecoveryKeyRecordWithHint(uid),
       Container.get(RecoveryPhoneService).hasConfirmed(uid),
       request.app.geo?.location?.countryCode
-        ? Container.get(RecoveryPhoneService).available(uid, request.app.geo.location.countryCode)
+        ? Container.get(RecoveryPhoneService).available(
+            uid,
+            request.app.geo.location.countryCode
+          )
         : Promise.resolve(false),
       this.db.securityEventsByUid({ uid }),
       this.db.devices(uid),
       listAuthorizedClients(uid),
     ]);
 
-    const recoveryPhoneAvailable = recoveryPhoneAvailableResult.status === 'fulfilled'
-      ? recoveryPhoneAvailableResult.value
-      : false;
+    const recoveryPhoneAvailable =
+      recoveryPhoneAvailableResult.status === 'fulfilled'
+        ? recoveryPhoneAvailableResult.value
+        : false;
 
     // Account record is required
     if (accountRecord.status === 'rejected') {

packages/fxa-auth-server/lib/routes/oauth/authorization.js

@@ -402,6 +402,20 @@ module.exports = ({ log, oauthDB, config }) => {
       handler: async function (req) {
         checkDisabledClientId(req.payload);
         const sessionToken = req.auth.credentials;
+
+        // For services that require email verification for non-2FA non-Sync
+        // flows, reject sessions in this state. This is accounted for in the
+        // front-end as well, but this guards in our BE. This purposefully
+        // does NOT check session.mustVerify because users in this kind of
+        // unverified session don't have that flag set.
+        const clientId = req.payload.client_id;
+        if (
+          config.servicesWithEmailVerification.includes(clientId) &&
+          !sessionToken.tokenVerified
+        ) {
+          throw AuthError.unverifiedSession();
+        }
+
         req.payload.assertion = await makeAssertionJWT(config, sessionToken);
         const result = await authorizationHandler(req);
 
@@ -415,8 +429,8 @@ module.exports = ({ log, oauthDB, config }) => {
           countryCode,
           deviceCount: devices.length,
           email,
-          service: req.payload.client_id,
-          clientId: req.payload.client_id,
+          service: clientId,
+          clientId,
           uid,
           userAgent: req.headers['user-agent'],
         });

packages/fxa-auth-server/test/local/routes/utils/signin.js

@@ -809,7 +809,7 @@ describe('sendSigninNotifications', () => {
     };
     config = {
       otp: otpOptions,
-      servicesWithEmailVerification: ['e6eb0d1e856335fc'],
+      servicesWithEmailVerification: ['32aaeb6f1c21316a'],
     };
 
     sendSigninNotifications = makeSigninUtils({
@@ -1346,8 +1346,8 @@ describe('sendSigninNotifications', () => {
       });
     });
 
-    it('sends verification email when service is VPN', () => {
-      request.payload.service = 'e6eb0d1e856335fc';
+    it('sends verification email when service is in servicesWithEmailVerification', () => {
+      request.payload.service = '32aaeb6f1c21316a';
       return sendSigninNotifications(
         request,
         accountRecord,

packages/fxa-auth-server/test/oauth/routes/authorization.js

@@ -17,6 +17,9 @@ const PKCE_CODE_CHALLENGE_METHOD = 'S256';
 const DISABLED_CLIENT_ID = 'd15ab1edd15ab1ed';
 
 const _ = () => {};
+
+const SERVICES_WITH_EMAIL_VERIFICATION_CLIENT = '32aaeb6f1c21316a';
+
 const route = require('../../../lib/routes/oauth/authorization')({
   log: {
     info: _,
@@ -26,6 +29,28 @@ const route = require('../../../lib/routes/oauth/authorization')({
   oauthDB: {},
 })[1];
 
+const sessionTokenRoute = require('../../../lib/routes/oauth/authorization')({
+  log: {
+    info: _,
+    debug: _,
+    warn: _,
+    notifyAttachedServices: _,
+  },
+  oauthDB: {},
+  config: {
+    oauthServer: {
+      expiration: { accessToken: 3600000 },
+      disabledClients: [DISABLED_CLIENT_ID],
+      allowHttpRedirects: false,
+      contentUrl: 'http://localhost',
+    },
+    oauth: {
+      disableNewConnectionsForClients: [],
+    },
+    servicesWithEmailVerification: [SERVICES_WITH_EMAIL_VERIFICATION_CLIENT],
+  },
+})[2];
+
 describe('/authorization POST', function () {
   describe('input validation', () => {
     const validation = route.config.validate.payload;
@@ -220,3 +245,104 @@ describe('/authorization POST', function () {
     });
   });
 });
+
+describe('/oauth/authorization POST', function () {
+  describe('servicesWithEmailVerification enforcement', () => {
+    it('rejects unverified sessions for clients in servicesWithEmailVerification', async () => {
+      const request = {
+        headers: {},
+        auth: {
+          credentials: {
+            tokenVerified: false,
+            uid: 'abc123',
+            email: '[email protected]',
+          },
+        },
+        payload: {
+          client_id: SERVICES_WITH_EMAIL_VERIFICATION_CLIENT,
+          state: 'foo',
+          scope: 'profile',
+        },
+      };
+
+      try {
+        await sessionTokenRoute.handler(request);
+        assert.fail('should have errored');
+      } catch (err) {
+        assert.equal(err.errno, 138); // Unverified session
+      }
+    });
+
+    it('allows verified sessions for clients in servicesWithEmailVerification', async () => {
+      const request = {
+        headers: {},
+        auth: {
+          credentials: {
+            tokenVerified: true,
+            uid: 'abc123',
+            email: '[email protected]',
+            emailVerified: true,
+            verifierSetAt: Date.now(),
+            lastAuthAt: () => Date.now(),
+            authenticationMethods: new Set(['pwd']),
+            authenticatorAssuranceLevel: 1,
+            profileChangedAt: Date.now(),
+            keysChangedAt: Date.now(),
+            id: 'sessionTokenId',
+          },
+        },
+        payload: {
+          client_id: SERVICES_WITH_EMAIL_VERIFICATION_CLIENT,
+          state: 'foo',
+          scope: 'profile',
+        },
+      };
+
+      try {
+        await sessionTokenRoute.handler(request);
+        assert.fail('should have errored');
+      } catch (err) {
+        // Should pass the servicesWithEmailVerification check and fail
+        // further downstream (e.g., at assertion verification or grant
+        // validation), not at unverified session check.
+        assert.notEqual(err.errno, 138);
+      }
+    });
+
+    it('allows unverified sessions for clients NOT in servicesWithEmailVerification', async () => {
+      const request = {
+        headers: {},
+        auth: {
+          credentials: {
+            tokenVerified: false,
+            uid: 'abc123',
+            email: '[email protected]',
+            emailVerified: true,
+            verifierSetAt: Date.now(),
+            lastAuthAt: () => Date.now(),
+            authenticationMethods: new Set(['pwd']),
+            authenticatorAssuranceLevel: 1,
+            profileChangedAt: Date.now(),
+            keysChangedAt: Date.now(),
+            id: 'sessionTokenId',
+            mustVerify: false,
+          },
+        },
+        payload: {
+          client_id: CLIENT_ID, // Not in servicesWithEmailVerification
+          state: 'foo',
+          scope: 'profile',
+        },
+      };
+
+      try {
+        await sessionTokenRoute.handler(request);
+        assert.fail('should have errored');
+      } catch (err) {
+        // Should NOT fail with unverified session error, but may fail
+        // further downstream for other reasons.
+        assert.notEqual(err.errno, 138);
+      }
+    });
+  });
+});

packages/fxa-content-server/server/lib/configuration.js

@@ -908,8 +908,8 @@ const conf = (module.exports = convict({
     },
   },
   servicesWithEmailVerification: {
-    doc: 'Services that will alwasy send a session verification email on sign in',
-    default: ['e6eb0d1e856335fc'],
+    doc: 'For users in a non-2FA non-Sync unverified session state going through an RP redirect flow, we skip 1) redirecting the user to enter emailed code verification page, and we skip 2) sending that email verification. Services in this list will NOT skip the redirect or email, and are blocked in the BE from completing the oauth flow by hitting our API directly in this state. Auth-server has this config as well.',
+    default: ['32aaeb6f1c21316a'], // SubPlat dev
     format: Array,
     env: 'SERVICES_WITH_EMAIL_VERIFICATION',
   },

packages/fxa-settings/src/lib/config.ts

@@ -206,7 +206,7 @@ export function getDefault() {
       enabled: true,
       preview: true,
     },
-    servicesWithEmailVerification: ['e6eb0d1e856335fc'],
+    servicesWithEmailVerification: ['32aaeb6f1c21316a'],
   } as Config;
 }
 

packages/fxa-settings/src/pages/Authorization/container.test.tsx

@@ -6,6 +6,7 @@ import React from 'react';
 import { renderWithLocalizationProvider } from 'fxa-react/lib/test-utils/localizationProvider';
 import { screen, waitFor } from '@testing-library/react';
 import AuthorizationContainer from './container';
+import { Config } from '../../lib/config';
 import { OAUTH_ERRORS } from '../../lib/oauth/oauth-errors';
 import { Integration } from '../../models';
 import AuthClient from 'fxa-auth-client/browser';
@@ -52,6 +53,7 @@ describe('AuthorizationContainer', () => {
   const mockUseLocation = jest.spyOn(ReachRouterModule, 'useLocation');
   const mockUseSession = jest.spyOn(ModelsHooksModule, 'useSession');
   const mockUseAuthClient = jest.spyOn(ModelsHooksModule, 'useAuthClient');
+  const mockUseConfig = jest.spyOn(ModelsHooksModule, 'useConfig');
   const mockIsOauthWebIntegration = jest.spyOn(
     OAuthWebIntegrationModule,
     'isOAuthWebIntegration'
@@ -78,6 +80,9 @@ describe('AuthorizationContainer', () => {
     mockLocation.search = '';
     mockUseLocation.mockReturnValue(mockLocation);
     mockUseAuthClient.mockReturnValue(mockAuthClient);
+    mockUseConfig.mockReturnValue({
+      servicesWithEmailVerification: ['test-service-id'],
+    } as Config);
     mockUseSession.mockReturnValue(mockSession);
     mockIsOauthWebIntegration.mockReturnValue(true);
     mockCurrentAccount.mockReturnValue(mockAccount);
@@ -127,6 +132,7 @@ describe('AuthorizationContainer', () => {
       },
       wantsPromptNone: jest.fn().mockReturnValue(true),
       returnOnError: jest.fn().mockReturnValue(false),
+      getClientId: jest.fn().mockReturnValue('some-other-client'),
       validatePromptNoneRequest: jest.fn().mockResolvedValue(undefined),
     };
 
@@ -230,6 +236,56 @@ describe('AuthorizationContainer', () => {
     });
   });
 
+  it('rejects prompt=none and shows error when session is unverified and returnOnError=false for servicesWithEmailVerification client', async () => {
+    mockCachedSignIn.mockResolvedValue({
+      data: {
+        verificationMethod: VerificationMethods.EMAIL_OTP,
+        verificationReason: VerificationReasons.SIGN_IN,
+        uid: mockAccount.uid,
+        sessionVerified: false,
+        emailVerified: true,
+        totpIsActive: false,
+      },
+      error: undefined,
+    });
+
+    const mockIntegration = {
+      data: {
+        redirectTo: '/settings',
+      },
+      wantsPromptNone: jest.fn().mockReturnValue(true),
+      returnOnError: jest.fn().mockReturnValue(false),
+      getClientId: jest.fn().mockReturnValue('test-service-id'),
+      validatePromptNoneRequest: jest.fn().mockResolvedValue(undefined),
+    };
+
+    render(mockIntegration);
+
+    await waitFor(() => {
+      expect(screen.getByText('Bad Request')).toBeInTheDocument();
+    });
+    expect(screen.getByText('Unverified user or session')).toBeInTheDocument();
+    expect(SigninUtilsModule.handleNavigation).not.toHaveBeenCalled();
+  });
+
+  it('allows prompt=none with verified session for servicesWithEmailVerification client', async () => {
+    const mockIntegration = {
+      data: {
+        redirectTo: '/settings',
+      },
+      wantsPromptNone: jest.fn().mockReturnValue(true),
+      returnOnError: jest.fn().mockReturnValue(false),
+      getClientId: jest.fn().mockReturnValue('test-service-id'),
+      validatePromptNoneRequest: jest.fn().mockResolvedValue(undefined),
+    };
+    render(mockIntegration);
+
+    await waitFor(() => {
+      expect(SigninUtilsModule.handleNavigation).toHaveBeenCalledTimes(1);
+    });
+    expect(screen.queryByText('Bad Request')).not.toBeInTheDocument();
+  });
+
   it("navigates to '/oauth' when no action is provided and not prompt=none", async () => {
     const mockOAuthWebIntegration = {
       data: {