fix: allow device auth flow without id_token MONGOSH-1669 (#187)

* fix: allow device auth flow without id_token MONGOSH-1669 * test: add integration test * Update src/log-hook.ts Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com> * test: reuse mock provider * extend error message * bump oidc-mock-provider * cleanup * types * fix: catch both inconsistencies in id_token presence * test: update test * update comment * refactor: use single type for LastIdTokenClaims * fix: check * test --------- Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>
mongodb-js · Feb 7, 2024 · 6bef820 · 6bef820
1 parent 14de2c8
commit 6bef820
Show file tree

Hide file tree

Showing 6 changed files with 128 additions and 40 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -57,7 +57,7 @@
     "@mongodb-js/eslint-config-devtools": "^0.9.9",
     "@mongodb-js/mocha-config-devtools": "^1.0.0",
     "@mongodb-js/monorepo-tools": "^1.1.4",
-    "@mongodb-js/oidc-mock-provider": "^0.6.2",
+    "@mongodb-js/oidc-mock-provider": "^0.7.1",
     "@mongodb-js/prettier-config-devtools": "^1.0.1",
     "@mongodb-js/tsconfig-devtools": "^1.0.0",
     "@types/chai": "^4.2.21",

diff --git a/src/integration.spec.ts b/src/integration.spec.ts
@@ -5,12 +5,17 @@ import {
   OIDCTestProvider,
   functioningAuthCodeBrowserFlow,
 } from '../test/oidc-test-provider';
+import type {
+  OIDCMockProviderConfig,
+  TokenMetadata,
+} from '@mongodb-js/oidc-mock-provider';
 import { MongoClient } from 'mongodb';
 import type { OpenBrowserOptions } from './';
 import { createMongoDBOIDCPlugin } from './';
 import { OIDCMockProvider } from '@mongodb-js/oidc-mock-provider';
 import { MongoCluster } from 'mongodb-runner';
 import path from 'path';
+import sinon from 'sinon';
 
 // node-fetch@3 is ESM-only...
 // eslint-disable-next-line @typescript-eslint/consistent-type-imports
@@ -120,6 +125,16 @@ describe('integration test with mongod', function () {
   context('can authenticate with a mock IdP', function () {
     let provider: OIDCMockProvider;
     let connectionString: string;
+    let getTokenPayload: OIDCMockProviderConfig['getTokenPayload'];
+    const tokenPayload = {
+      expires_in: 3600,
+      payload: {
+        // Define the user information stored inside the access tokens
+        groups: ['testgroup'],
+        sub: 'testuser',
+        aud: 'resource-server-audience-value',
+      },
+    };
 
     before(async function () {
       if (+process.version.slice(1).split('.')[0] < 16) {
@@ -128,16 +143,8 @@ describe('integration test with mongod', function () {
         return this.skip();
       }
       provider = await OIDCMockProvider.create({
-        getTokenPayload() {
-          return {
-            expires_in: 3600,
-            payload: {
-              // Define the user information stored inside the access tokens
-              groups: ['testgroup'],
-              sub: 'testuser',
-              aud: 'resource-server-audience-value',
-            },
-          };
+        getTokenPayload(metadata: TokenMetadata) {
+          return getTokenPayload(metadata);
         },
       });
 
@@ -160,6 +167,10 @@ describe('integration test with mongod', function () {
       await Promise.all([cluster?.close?.(), provider?.close?.()]);
     });
 
+    beforeEach(function () {
+      getTokenPayload = () => tokenPayload;
+    });
+
     it('can successfully authenticate with a fake Auth Code Flow', async function () {
       const plugin = createMongoDBOIDCPlugin({
         openBrowserTimeout: 60_000,
@@ -234,5 +245,41 @@ describe('integration test with mongod', function () {
         await client.close();
       }
     });
+
+    it('can successfully authenticate with a fake Device Auth Flow without an id_token - with a warning', async function () {
+      getTokenPayload = () => ({
+        ...tokenPayload,
+        // id_token will not be included
+        skipIdToken: true,
+      });
+
+      const { mongoClientOptions, logger } = createMongoDBOIDCPlugin({
+        notifyDeviceFlow: () => {},
+        allowedFlows: ['device-auth'],
+      });
+      const logEmitSpy = sinon.spy(logger, 'emit');
+      const client = await MongoClient.connect(connectionString, {
+        ...mongoClientOptions,
+      });
+      // without the id token, a warning will be logged
+      sinon.assert.calledWith(
+        logEmitSpy,
+        'mongodb-oidc-plugin:missing-id-token'
+      );
+      try {
+        const status = await client
+          .db('admin')
+          .command({ connectionStatus: 1 });
+        expect(status).to.deep.equal({
+          ok: 1,
+          authInfo: {
+            authenticatedUsers: [{ user: 'dev/testuser', db: '$external' }],
+            authenticatedUserRoles: [{ role: 'dev/testgroup', db: 'admin' }],
+          },
+        });
+      } finally {
+        await client.close();
+      }
+    });
   });
 });
diff --git a/src/log-hook.ts b/src/log-hook.ts
@@ -260,4 +260,13 @@ export function hookLoggerToMongoLogWriter(
       'Destroyed OIDC plugin instance'
     );
   });
+
+  emitter.on('mongodb-oidc-plugin:missing-id-token', () => {
+    log.warn(
+      'OIDC-PLUGIN',
+      mongoLogId(1_002_000_022),
+      `${contextPrefix}-oidc`,
+      'Missing ID token in IdP response'
+    );
+  });
 }
diff --git a/src/plugin.ts b/src/plugin.ts
@@ -36,6 +36,14 @@ import { kDefaultOpenBrowserTimeout } from './api';
 import { spawn } from 'child_process';
 
 /** @internal */
+
+// The `sub` and `aud` claims in the ID token of the last-received
+// TokenSet, if any.
+// 'no-id-token' means that the previous token set contained no ID token
+type LastIdTokenClaims =
+  | (Pick<IdTokenClaims, 'aud' | 'sub'> & { noIdToken?: never })
+  | { noIdToken: true };
+
 interface UserOIDCAuthState {
   // The information that the driver forwarded to us from the server
   // about the OIDC Identity Provider config.
@@ -52,9 +60,7 @@ interface UserOIDCAuthState {
   // A timer attached to this state that automatically calls
   // currentTokenSet.tryRefresh() before the token expires.
   timer?: ReturnType<typeof setTimeout>;
-  // The `sub` and `aud` claims in the ID token of the last-received
-  // TokenSet, if any.
-  lastIdTokenClaims?: Pick<IdTokenClaims, 'aud' | 'sub'>;
+  lastIdTokenClaims?: LastIdTokenClaims;
   // A cached Client instance that uses the issuer metadata as discovered
   // through serverOIDCMetadata.
   client?: Client;
@@ -201,7 +207,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
       }
 
       for (const [key, serializedState] of original.state) {
-        const state = {
+        const state: UserOIDCAuthState = {
           serverOIDCMetadata: { ...serializedState.serverOIDCMetadata },
           currentAuthAttempt: null,
           currentTokenSet: null,
@@ -433,28 +439,53 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
     // for client A, the token expires before it is requested again by client A,
     // then the plugin is passed to client B which requests a token, and we
     // receive mismatching tokens for different users or different audiences.
-    const idTokenClaims = tokenSet.claims();
-    if (state.lastIdTokenClaims) {
-      for (const claim of ['aud', 'sub'] as const) {
-        const normalize = (value: string | string[]): string => {
-          return JSON.stringify(
-            Array.isArray(value) ? [...value].sort() : value
-          );
-        };
-        const knownClaim = normalize(state.lastIdTokenClaims[claim]);
-        const newClaim = normalize(idTokenClaims[claim]);
+    if (
+      !tokenSet.id_token &&
+      state.lastIdTokenClaims &&
+      !state.lastIdTokenClaims.noIdToken
+    ) {
+      throw new MongoDBOIDCError(
+        `ID token expected, but not found. Expected claims: ${JSON.stringify(
+          state.lastIdTokenClaims
+        )}`
+      );
+    }
 
-        if (knownClaim !== newClaim) {
-          throw new MongoDBOIDCError(
-            `Unexpected '${claim}' field in id token: Expected ${knownClaim}, saw ${newClaim}`
-          );
+    if (
+      tokenSet.id_token &&
+      state.lastIdTokenClaims &&
+      state.lastIdTokenClaims.noIdToken
+    ) {
+      throw new MongoDBOIDCError(`Unexpected ID token received.`);
+    }
+
+    if (tokenSet.id_token) {
+      const idTokenClaims = tokenSet.claims();
+      if (state.lastIdTokenClaims && !state.lastIdTokenClaims.noIdToken) {
+        for (const claim of ['aud', 'sub'] as const) {
+          const normalize = (value: string | string[]): string => {
+            return JSON.stringify(
+              Array.isArray(value) ? [...value].sort() : value
+            );
+          };
+          const knownClaim = normalize(state.lastIdTokenClaims[claim]);
+          const newClaim = normalize(idTokenClaims[claim]);
+
+          if (knownClaim !== newClaim) {
+            throw new MongoDBOIDCError(
+              `Unexpected '${claim}' field in id token: Expected ${knownClaim}, saw ${newClaim}`
+            );
+          }
         }
       }
+      state.lastIdTokenClaims = {
+        aud: idTokenClaims.aud,
+        sub: idTokenClaims.sub,
+      };
+    } else {
+      state.lastIdTokenClaims = { noIdToken: true };
+      this.logger.emit('mongodb-oidc-plugin:missing-id-token');
     }
-    state.lastIdTokenClaims = {
-      aud: idTokenClaims.aud,
-      sub: idTokenClaims.sub,
-    };
 
     const timerDuration = automaticRefreshTimeoutMS(tokenSet);
     // Use `.call()` because in browsers, `setTimeout()` requires that it is called

diff --git a/src/types.ts b/src/types.ts
@@ -46,6 +46,7 @@ export interface MongoDBOIDCLogEventsMap {
     expiresAt: string | null;
   }) => void;
   'mongodb-oidc-plugin:destroyed': () => void;
+  'mongodb-oidc-plugin:missing-id-token': () => void;
 }
 
 /** @public */