Add unit tests and rename env var to GRIST_OIDC_SP_HTTP_TIMEOUT

gristlabs · Aug 24, 2024 · a35baf8 · a35baf8
1 parent 246e9eb
commit a35baf8
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 11 deletions.
diff --git a/app/server/lib/OIDCConfig.ts b/app/server/lib/OIDCConfig.ts
@@ -47,8 +47,9 @@
  *        A JSON object with extra client metadata to pass to openid-client. Optional.
  *        Be aware that setting this object may override any other values passed to the openid client.
  *        More info: https://github.com/panva/node-openid-client/tree/main/docs#new-clientmetadata-jwks-options
- *    env GRIST_OIDC_HTTP_TIMEOUT
- *        The timeout in milliseconds for HTTP requests to the IdP. Defaults to 3500.
+ *    env GRIST_OIDC_SP_HTTP_TIMEOUT
+ *        The timeout in milliseconds for HTTP requests to the IdP. The default value is set to 3500 by the
+ *        openid-client library. See: https://github.com/panva/node-openid-client/blob/main/docs/README.md#customizing-http-requests
  *
  * This version of OIDCConfig has been tested with Keycloak OIDC IdP following the instructions
  * at:
@@ -67,7 +68,8 @@
 import * as express from 'express';
 import { GristLoginSystem, GristServer } from './GristServer';
 import {
-  Client, ClientMetadata, custom, Issuer, errors as OIDCError, TokenSet, UserinfoResponse
+  Client, ClientMetadata, custom, Issuer,
+  errors as OIDCError, HttpOptions as OIDCHttpOptions, TokenSet, UserinfoResponse
 } from 'openid-client';
 import { Sessions } from './Sessions';
 import log from 'app/server/lib/log';
@@ -139,10 +141,7 @@ export class OIDCConfig {
       censor: true,
     });
     const httpTimeout = section.flag('httpTimeout').readInt({
-      envVar: 'GRIST_OIDC_HTTP_TIMEOUT',
-      // Default value matching that of node-openid-client
-      // See https://github.com/panva/node-openid-client/blob/main/docs/README.md#customizing-http-requests for more details.
-      defaultValue: 3500,
+      envVar: 'GRIST_OIDC_SP_HTTP_TIMEOUT',
     });
     this._namePropertyKey = section.flag('namePropertyKey').readString({
       envVar: 'GRIST_OIDC_SP_PROFILE_NAME_ATTR',
@@ -181,10 +180,10 @@ export class OIDCConfig {
 
     this._redirectUrl = new URL(CALLBACK_URL, spHost).href;
     await this._initClient({ issuerUrl, clientId, clientSecret, extraMetadata });
-
-    custom.setHttpOptionsDefaults({
-      timeout: httpTimeout,
+    this._client[custom.http_options] = (): OIDCHttpOptions => ({
+      ...(httpTimeout !== undefined ? {timeout: httpTimeout} : {})
     });
+
     if (this._client.issuer.metadata.end_session_endpoint === undefined &&
       !this._endSessionEndpoint && !this._skipEndSessionEndpoint) {
       throw new Error('The Identity provider does not propose end_session_endpoint. ' +

diff --git a/test/server/lib/OIDCConfig.ts b/test/server/lib/OIDCConfig.ts
@@ -5,7 +5,7 @@ import {Sessions} from "app/server/lib/Sessions";
 import log from "app/server/lib/log";
 import {assert} from "chai";
 import Sinon from "sinon";
-import {Client, generators, errors as OIDCError} from "openid-client";
+import {Client, custom, CustomHttpOptionsProvider, generators, errors as OIDCError} from "openid-client";
 import express from "express";
 import _ from "lodash";
 import {RequestWithLogin} from "app/server/lib/Authorizer";
@@ -38,6 +38,7 @@ class ClientStub {
   public callback = Sinon.stub().returns({});
   public userinfo = Sinon.stub().returns(undefined);
   public endSessionUrl = Sinon.stub().returns(undefined);
+  public [custom.http_options]: CustomHttpOptionsProvider;
   public issuer: {
     metadata: {
       end_session_endpoint: string | undefined;
@@ -192,6 +193,55 @@ describe('OIDCConfig', () => {
         });
       });
     });
+
+    describe('GRIST_OIDC_SP_HTTP_TIMEOUT', function () {
+      [
+        {
+          itMsg: 'when omitted should not override openid-client default value',
+          expectedUserDefinedHttpOptions: {}
+        },
+        {
+          itMsg: 'should reject when the provided value is not a number',
+          env: {
+            GRIST_OIDC_SP_HTTP_TIMEOUT: '__NOT_A_NUMBER__',
+          },
+          expectedErrorMsg: /__NOT_A_NUMBER__ does not look like a number/,
+        },
+        {
+          itMsg: 'should override openid-client timeout accordingly to the provided value',
+          env: {
+            GRIST_OIDC_SP_HTTP_TIMEOUT: '10000',
+          },
+          shouldSetTimeout: true,
+          expectedUserDefinedHttpOptions: {
+            timeout: 10000
+          }
+        },
+        {
+          itMsg: 'should allow disabling the timeout by having its value set to 0',
+          env: {
+            GRIST_OIDC_SP_HTTP_TIMEOUT: '0',
+          },
+          expectedUserDefinedHttpOptions: {
+            timeout: 0
+          }
+        }
+      ].forEach(ctx => {
+        it(ctx.itMsg, async () => {
+          setEnvVars();
+          Object.assign(process.env, ctx.env);
+          const clientStub = new ClientStub();
+          const promise = OIDCConfigStubbed.buildWithStub(clientStub.asClient());
+          if (ctx.expectedErrorMsg) {
+            await assert.isRejected(promise, ctx.expectedErrorMsg);
+          } else {
+            await assert.isFulfilled(promise, 'initOIDC should have been fulfilled');
+            const userDefinedOptions = (clientStub[custom.http_options] as Function)();
+            assert.deepEqual(userDefinedOptions, ctx.expectedUserDefinedHttpOptions);
+          }
+        });
+      });
+    });
   });
 
   describe('GRIST_OIDC_IDP_ENABLED_PROTECTIONS', () => {