fix: include username in server metadata key (#194)

addaleax · web-flow · commit eb6643366621 · 2024-08-01T11:04:25.000+02:00
This is something we should have ideally always been doing (as hinted at by the doc comment for `getAuthState()`), but which the driver only added back support for in 6.7.0. This shouldn't have an immediate impact on our products, because we already have some mitigations in place for the fact that usernames were previously not being taken into account (e.g. here: https://github.com/mongodb-js/mongosh/blob/adc530e7ffcf092677d944fd69670c5cbd8e103d/packages/service-provider-server/src/cli-service-provider.ts#L172 ), but is semantically the right thing to do and should give us a bit more flexibility in the future. Also, add a test that confirms that changes to server metadata/username actually have the expected effect, and emit a message bus event that shares information about the server metadata, as that may be helpful debugging information.
diff --git a/src/plugin.spec.ts b/src/plugin.spec.ts
@@ -55,12 +55,14 @@ async function fetchBrowser({ url }: OpenBrowserOptions): Promise<void> {
 function requestToken(
   plugin: MongoDBOIDCPlugin,
   oidcParams: IdPServerInfo,
-  abortSignal?: OIDCAbortSignal
+  abortSignal?: OIDCAbortSignal,
+  username?: string
 ): ReturnType<OIDCCallbackFunction> {
   return plugin.mongoClientOptions.authMechanismProperties.OIDC_HUMAN_CALLBACK({
     timeoutContext: abortSignal,
     version: 1,
     idpInfo: oidcParams,
+    username,
   });
 }
 
@@ -500,6 +502,47 @@ describe('OIDC plugin (local OIDC provider)', function () {
         expect(pluginOptions.allowedFlows).to.have.been.calledTwice;
       });
     });
+
+    context('when the server metadata/user data changes', function () {
+      beforeEach(function () {
+        (pluginOptions.allowedFlows = sinon.stub().resolves(['auth-code'])),
+          (plugin = createMongoDBOIDCPlugin(pluginOptions));
+      });
+
+      it('it will perform two different auth flows', async function () {
+        await requestToken(
+          plugin,
+          provider.getMongodbOIDCDBInfo(),
+          undefined,
+          'usera'
+        );
+        expect(pluginOptions.allowedFlows).to.have.callCount(1);
+        await requestToken(
+          plugin,
+          provider.getMongodbOIDCDBInfo(),
+          undefined,
+          'userb'
+        );
+        expect(pluginOptions.allowedFlows).to.have.callCount(2);
+        await requestToken(
+          plugin,
+          provider.getMongodbOIDCDBInfo(),
+          undefined,
+          'userb'
+        );
+        expect(pluginOptions.allowedFlows).to.have.callCount(2);
+        await requestToken(
+          plugin,
+          {
+            ...provider.getMongodbOIDCDBInfo(),
+            extraKey: 'asdf',
+          } as IdPServerInfo,
+          undefined,
+          'userb'
+        );
+        expect(pluginOptions.allowedFlows).to.have.callCount(3);
+      });
+    });
   });
 
   context('when the user aborts an auth code flow', function () {
diff --git a/src/plugin.ts b/src/plugin.ts
@@ -52,7 +52,7 @@ type LastIdTokenClaims =
 interface UserOIDCAuthState {
   // The information that the driver forwarded to us from the server
   // about the OIDC Identity Provider config.
-  serverOIDCMetadata: IdPServerInfo;
+  serverOIDCMetadata: IdPServerInfo & Pick<OIDCCallbackParams, 'username'>;
   // A Promise that resolves when the current authentication attempt
   // is finished, if there is one at the moment.
   currentAuthAttempt: Promise<IdPServerResponse> | null;
@@ -288,7 +288,9 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
 
   // Return the current state for a given [server, username] configuration,
   // or create a new one if none exists.
-  private getAuthState(serverMetadata: IdPServerInfo): UserOIDCAuthState {
+  private getAuthState(
+    serverMetadata: IdPServerInfo & Pick<OIDCCallbackParams, 'username'>
+  ): UserOIDCAuthState {
     if (!serverMetadata.issuer || typeof serverMetadata.issuer !== 'string') {
       throw new MongoDBOIDCError(`'issuer' is missing`);
     }
@@ -909,6 +911,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
   public async requestToken(
     params: OIDCCallbackParams
   ): Promise<IdPServerResponse> {
+    this.logger.emit('mongodb-oidc-plugin:received-server-params', { params });
     if (params.version !== 1) {
       throw new MongoDBOIDCError(
         // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
@@ -926,7 +929,10 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin {
       throw new MongoDBOIDCError('No IdP information provided');
     }
 
-    const state = this.getAuthState(params.idpInfo);
+    const state = this.getAuthState({
+      ...params.idpInfo,
+      username: params.username,
+    });
 
     if (state.currentAuthAttempt) {
       return await state.currentAuthAttempt;
diff --git a/src/types.ts b/src/types.ts
@@ -75,6 +75,9 @@ export interface MongoDBOIDCLogEventsMap {
   'mongodb-oidc-plugin:missing-id-token': () => void;
   'mongodb-oidc-plugin:outbound-http-request': (event: { url: string }) => void;
   'mongodb-oidc-plugin:inbound-http-request': (event: { url: string }) => void;
+  'mongodb-oidc-plugin:received-server-params': (event: {
+    params: OIDCCallbackParams;
+  }) => void;
 }
 
 /** @public */

Original file line number	Diff line number	Diff line change
`@@ -75,6 +75,9 @@ export interface MongoDBOIDCLogEventsMap {`
`75`	`75`	`'mongodb-oidc-plugin:missing-id-token': () => void;`
`76`	`76`	`'mongodb-oidc-plugin:outbound-http-request': (event: { url: string }) => void;`
`77`	`77`	`'mongodb-oidc-plugin:inbound-http-request': (event: { url: string }) => void;`
	`78`	`+ 'mongodb-oidc-plugin:received-server-params': (event: {`
	`79`	`+ params: OIDCCallbackParams;`
	`80`	`+ }) => void;`
`78`	`81`	`}`
`79`	`82`
`80`	`83`	`/** @public */`