Merge pull request #19470 from mozilla/bug-fix-mfa-strategy-return-type

bug(auth): Fix mfa strategy return type

Author: dschom

packages/fxa-auth-server/lib/routes/auth-schemes/mfa.ts

@@ -46,6 +46,7 @@ export type Credentials = {
   verifiedAt?: string | null;
   metricsOptOutAt?: string | null;
   providerId?: string | null;
+  scope?: string[];
 };
 
 export const strategy = (
@@ -58,7 +59,7 @@ export const strategy = (
 
       // Make sure auth header is at least semi valid.
       if (!auth || auth.indexOf('Bearer') !== 0) {
-        throw AppError.unauthorized('Bearer token not provided');
+        throw AppError.unauthorized('Token not found');
       }
 
       // Extract jwt value
@@ -83,7 +84,7 @@ export const strategy = (
           scope?: string[];
         };
       } catch (err) {
-        throw AppError.invalidToken(err.message);
+        throw AppError.unauthorized('Token invalid');
       }
 
       // Ensure required state
@@ -97,17 +98,20 @@ export const strategy = (
 
       const sessionToken = await getCredentialsFunc(decoded.stid);
       if (!sessionToken) {
-        throw AppError.invalidToken('Parent session token not found!');
+        throw AppError.unauthorized('Token not found');
       }
 
-      // Check the underlying session
+      if (sessionToken.uid !== decoded.sub) {
+        throw AppError.unauthorized('Token invalid');
+      }
+
+      // Decorate session token with scope
+      sessionToken.scope = decoded.scope;
+
       // Finalize auth
       return h.authenticated({
-        credentials: {
-          ...sessionToken,
-          uid: decoded.sub,
-          scope: decoded.scope,
-        },
+        // Return actual session token instance!
+        credentials: sessionToken,
       });
     },
   });

packages/fxa-auth-server/test/local/routes/auth-schemes/mfa.js

@@ -0,0 +1,142 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const { assert } = require('chai');
+const sinon = require('sinon');
+const AppError = require('../../../../lib/error');
+const { strategy } = require('../../../../lib/routes/auth-schemes/mfa');
+const jwt = require('jsonwebtoken');
+const uuid = require('uuid');
+
+function makeJwt(account, sessionToken, config) {
+  const now = Math.floor(Date.now() / 1000);
+  const claims = {
+    sub: account.uid,
+    scope: [`mfa:test`],
+    iat: now,
+    jti: uuid.v4(),
+    stid: sessionToken.id,
+  };
+  const opts = {
+    algorithm: 'HS256',
+    expiresIn: config.mfa.jwt.expiresInSec,
+    audience: config.mfa.jwt.audience,
+    issuer: config.mfa.jwt.issuer,
+  };
+  const key = config.mfa.jwt.secretKey;
+  return jwt.sign(claims, key, opts);
+}
+
+describe('lib/routes/auth-schemes/mfa', () => {
+  const mockSessionToken = {
+    uid: 'account-123',
+    id: 'session-123',
+    get foo() {
+      return 'bar';
+    },
+  };
+  const mockAccount = { uid: 'account-123' };
+  const mockConfig = {
+    mfa: {
+      jwt: {
+        expiresInSec: 1,
+        audience: 'fxa',
+        issuer: 'accounts.firefox.com',
+        secretKey: 'foxes'.repeat(13),
+      },
+    },
+  };
+
+  it('should authenticate with valid jwt token', async () => {
+    const jwt = makeJwt(mockAccount, mockSessionToken, mockConfig);
+    const request = {
+      headers: { authorization: `Bearer ${jwt}` },
+      auth: { mode: 'required' },
+    };
+    const h = { authenticated: sinon.fake() };
+    const getCredentialsFunc = sinon.fake.resolves(mockSessionToken);
+    const authStrategy = strategy(mockConfig, getCredentialsFunc)();
+
+    await authStrategy.authenticate(request, h);
+
+    // Important! Session token should be returned as credentials,
+    // AND object reference should not change!
+    assert.isTrue(
+      h.authenticated.calledOnceWithExactly({
+        credentials: sinon.match.same(mockSessionToken),
+      })
+    );
+
+    // Session token should be decorated with a scope.
+    assert.equal(mockSessionToken.scope[0], 'mfa:test');
+  });
+
+  it('should throw an error if no authorization header is provided', async () => {
+    const getCredentialsFunc = sinon.fake.resolves(null);
+    const authStrategy = strategy(mockConfig, getCredentialsFunc)();
+
+    const request = { headers: {}, auth: { mode: 'required' } };
+    const h = { continue: Symbol('continue') };
+
+    try {
+      await authStrategy.authenticate(request, h);
+      assert.fail('Should have thrown an error');
+    } catch (err) {
+      assert.instanceOf(err, AppError);
+      const errorResponse = err.output.payload;
+      assert.equal(errorResponse.code, 401);
+      assert.equal(errorResponse.errno, 110);
+      assert.equal(errorResponse.message, 'Unauthorized for route');
+      assert.equal(errorResponse.detail, 'Token not found');
+    }
+  });
+
+  it('should not authenticate if the parent session cannot be found', async () => {
+    const getCredentialsFunc = sinon.fake.resolves(null);
+    const authStrategy = strategy(mockConfig, getCredentialsFunc)();
+    const jwt = makeJwt(mockAccount, mockSessionToken, mockConfig);
+
+    const request = {
+      headers: { authorization: `Bearer ${jwt}` },
+      auth: { mode: 'required' },
+    };
+    const h = { continue: Symbol('continue') };
+
+    try {
+      await authStrategy.authenticate(request, h);
+      assert.fail('Should have thrown an error');
+    } catch (err) {
+      assert.instanceOf(err, AppError);
+      const errorResponse = err.output.payload;
+      assert.equal(errorResponse.code, 401);
+      assert.equal(errorResponse.errno, 110);
+      assert.equal(errorResponse.message, 'Unauthorized for route');
+      assert.equal(errorResponse.detail, 'Token not found');
+    }
+  });
+
+  it('should not authenticate with invalid jwt token due to sub mismatch', async () => {
+    const getCredentialsFunc = sinon.fake.resolves({ sub: 'account-234' });
+    const authStrategy = strategy(mockConfig, getCredentialsFunc)();
+    const jwt = makeJwt(mockAccount, mockSessionToken, mockConfig);
+
+    const request = {
+      headers: { authorization: `Bearer ${jwt}` },
+      auth: { mode: 'required' },
+    };
+    const h = { continue: Symbol('continue') };
+
+    try {
+      await authStrategy.authenticate(request, h);
+      assert.fail('Should have thrown an error');
+    } catch (err) {
+      assert.instanceOf(err, AppError);
+      const errorResponse = err.output.payload;
+      assert.equal(errorResponse.code, 401);
+      assert.equal(errorResponse.errno, 110);
+      assert.equal(errorResponse.message, 'Unauthorized for route');
+      assert.equal(errorResponse.detail, 'Token invalid');
+    }
+  });
+});

packages/fxa-auth-server/test/local/routes/mfa.js

@@ -99,6 +99,7 @@ describe('mfa', () => {
       // There's typically much more data returned by this callback, but
       // for testing purposes this is sufficient.
       id: SESSION_TOKEN_ID,
+      uid: UID,
       uaBrowser: UA_BROWSER,
     });
 
@@ -118,7 +119,7 @@ describe('mfa', () => {
   });
 
   it('sends otp, verifies otp, and gets a valid jwt in return', async () => {
-    const requestResult = await await runTest(
+    const requestResult = await runTest(
       '/mfa/otp/request',
       {
         credentials: {
@@ -179,7 +180,7 @@ describe('mfa', () => {
     }
     assert.isDefined(error);
     assert.equal(error.errno, 110);
-    assert.equal(error.message, 'jwt malformed');
+    assert.equal(error.message, 'Unauthorized for route');
   });
 
   it('will not allow an expired token', async () => {
@@ -194,6 +195,6 @@ describe('mfa', () => {
     }
     assert.isDefined(error);
     assert.equal(error.errno, 110);
-    assert.equal(error.message, 'jwt expired');
+    assert.equal(error.message, 'Unauthorized for route');
   });
 });

packages/fxa-settings/src/components/Settings/MfaGuard/error-boundary.test.tsx

@@ -53,7 +53,7 @@ describe('MfaErrorBoundary', () => {
       </MfaErrorBoundary>
     );
 
-    const authError: any = new Error('invalid jwt');
+    const authError: any = new Error('Unauthorized for route');
     authError.code = 401;
     authError.errno = 110;