WIP

app/server/lib/OIDCConfig.ts

@@ -80,7 +80,7 @@ export class OIDCConfig {
     return config;
   }
 
-  private _client: Client;
+  protected _client: Client;
   private _redirectUrl: string;
   private _namePropertyKey?: string;
   private _emailPropertyKey: string;
@@ -89,7 +89,7 @@ export class OIDCConfig {
   private _ignoreEmailVerified: boolean;
   private _enabledProtections: EnabledProtectionsString[] = [];
 
-  private constructor() {
+  protected constructor() {
   }
 
   public async initOIDC(): Promise<void> {
@@ -133,15 +133,9 @@ export class OIDCConfig {
     })!;
 
     this._enabledProtections = this._buildEnabledProtections(section);
-
-    const issuer = await Issuer.discover(issuerUrl);
     this._redirectUrl = new URL(CALLBACK_URL, spHost).href;
-    this._client = new issuer.Client({
-      client_id: clientId,
-      client_secret: clientSecret,
-      redirect_uris: [ this._redirectUrl ],
-      response_types: [ 'code' ],
-    });
+    await this._initClient({issuerUrl, clientId, clientSecret});
+
     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. ' +
@@ -160,7 +154,7 @@ export class OIDCConfig {
     try {
       const params = this._client.callbackParams(req);
       const { state, targetUrl } = mreq.session?.oidc ?? {};
-      if (!state && this._supportsProtection('STATE')) {
+      if (!state && this.supportsProtection('STATE')) {
         throw new Error('Login or logout failed to complete');
       }
 
@@ -228,19 +222,35 @@ export class OIDCConfig {
     });
   }
 
+  public supportsProtection(protection: EnabledProtectionsString) {
+    return this._enabledProtections.includes(protection);
+  }
+
+  protected async _initClient({issuerUrl, clientId, clientSecret}:
+    {issuerUrl: string, clientId: string, clientSecret: string}
+  ): Promise<void> {
+    const issuer = await Issuer.discover(issuerUrl);
+    this._client = new issuer.Client({
+      client_id: clientId,
+      client_secret: clientSecret,
+      redirect_uris: [ this._redirectUrl ],
+      response_types: [ 'code' ],
+    });
+  }
+
   private async _generateAndStoreConnectionInfo(req: express.Request, targetUrl: string) {
     const mreq = req as RequestWithLogin;
     if (!mreq.session) { throw new Error('no session available'); }
     const oidcInfo: {[key: string]: string} = {
       targetUrl
     };
-    if (this._supportsProtection('PKCE')) {
+    if (this.supportsProtection('PKCE')) {
       oidcInfo.codeVerifier = generators.codeVerifier();
     }
-    if (this._supportsProtection('STATE')) {
+    if (this.supportsProtection('STATE')) {
       oidcInfo.state = generators.state();
     }
-    if (this._supportsProtection('NONCE')) {
+    if (this.supportsProtection('NONCE')) {
       oidcInfo.nonce = generators.nonce();
     }
 
@@ -249,18 +259,14 @@ export class OIDCConfig {
     return _.pick(oidcInfo, ['codeVerifier', 'state', 'nonce']);
   }
 
-  private _supportsProtection(protection: EnabledProtectionsString) {
-    return this._enabledProtections.includes(protection);
-  }
-
   private _buildEnabledProtections(section: AppSettings): EnabledProtectionsString[] {
     const enabledProtections = section.flag('enabledProtections').readString({
-      envVar: 'GRIST_OIDC_ENABLED_PROTECTIONS',
+      envVar: 'GRIST_OIDC_IDP_ENABLED_PROTECTIONS',
       defaultValue: 'PKCE,STATE',
     })!.split(',');
     for (const protection of enabledProtections) {
-      if (!ENABLED_PROTECTIONS[protection as EnabledProtectionsString]) {
-        throw new Error(`OIDC: Invalid protection in GRIST_OIDC_ENABLED_PROTECTIONS: ${protection}`);
+      if (!ENABLED_PROTECTIONS.hasOwnProperty(protection as EnabledProtectionsString)) {
+        throw new Error(`OIDC: Invalid protection in GRIST_OIDC_IDP_ENABLED_PROTECTIONS: ${protection}`);
       }
     }
     return enabledProtections as EnabledProtectionsString[];

test/server/lib/OIDCConfig.ts

@@ -1,17 +1,51 @@
 import {OIDCConfig} from "app/server/lib/OIDCConfig";
 import {assert} from "chai";
 import {EnvironmentSnapshot} from "../testUtils";
+import Sinon from "sinon";
+import {Client} from "openid-client";
 
+class OIDCConfigStubbed extends OIDCConfig {
+  public static async build(clientStub?: Client): Promise<OIDCConfig> {
+    const result = new OIDCConfigStubbed();
+    if (clientStub) {
+      result._initClient = Sinon.spy(() => {
+        result._client = clientStub!;
+      });
+    }
+    await result.initOIDC();
+    return result;
+  }
+
+  public _initClient: Sinon.SinonSpy;
+}
+
+class ClientStub {
+  public issuer: {
+    metadata: {
+      end_session_endpoint: string | undefined;
+    }
+  } = {
+    metadata: {
+      end_session_endpoint: 'http://localhost:8484/logout',
+    }
+  };
+}
 
 describe('OIDCConfig', () => {
   let oldEnv: EnvironmentSnapshot;
+  let sandbox: Sinon.SinonSandbox;
 
   before(() => {
     oldEnv = new EnvironmentSnapshot();
   });
 
+  beforeEach(() => {
+    sandbox = Sinon.createSandbox();
+  });
+
   afterEach(() => {
     oldEnv.restore();
+    sandbox.restore();
   });
 
   function setEnvVars() {
@@ -22,24 +56,104 @@ describe('OIDCConfig', () => {
     process.env.GRIST_OIDC_IDP_SCOPES = 'openid email profile';
     process.env.GRIST_OIDC_SP_PROFILE_NAME_ATTR = ''; // use the default behavior
     process.env.GRIST_OIDC_SP_PROFILE_EMAIL_ATTR = ''; // use the default behavior
-    process.env.GRIST_OIDC_IDP_END_SESSION_ENDPOINT = 'http://localhost:8484/logout';
-    process.env.GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT = 'false';
-    process.env.GRIST_OIDC_SP_IGNORE_EMAIL_VERIFIED = 'false';
   }
 
-  it('should throw when required env variables are not passed', async () => {
-    for (const envVar of [
-      'GRIST_OIDC_SP_HOST',
-      'GRIST_OIDC_IDP_ISSUER',
-      'GRIST_OIDC_IDP_CLIENT_ID',
-      'GRIST_OIDC_IDP_CLIENT_SECRET',
-    ]) {
+  describe('build', () => {
+    it('should reject when required env variables are not passed', async () => {
+      for (const envVar of [
+        'GRIST_OIDC_SP_HOST',
+        'GRIST_OIDC_IDP_ISSUER',
+        'GRIST_OIDC_IDP_CLIENT_ID',
+        'GRIST_OIDC_IDP_CLIENT_SECRET',
+      ]) {
+        setEnvVars();
+        delete process.env[envVar];
+        const promise = OIDCConfig.build();
+        await assert.isRejected(promise, `missing environment variable: ${envVar}`);
+      }
+    });
+
+    it('should reject when the client initialization fails', async () => {
       setEnvVars();
-      delete process.env[envVar];
-      const promise = OIDCConfig.build();
-      await assert.isRejected(promise, `missing environment variable: ${envVar}`);
-    }
+      sandbox.stub(OIDCConfigStubbed.prototype, '_initClient').rejects(new Error('client init failed'));
+      const promise = OIDCConfigStubbed.build();
+      await assert.isRejected(promise, 'client init failed');
+    });
+
+    describe('End Session Endpoint', () => {
+      [
+        {
+          itMsg: 'should fulfill when the end_session_endpoint is not known ' +
+            'and GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT=true',
+          end_session_endpoint: undefined,
+          env: {
+            GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT: 'true'
+          }
+        },
+        {
+          itMsg: 'should fulfill when the end_session_endpoint is not known ' +
+            'and GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT=true',
+          end_session_endpoint: undefined,
+          env: {
+            GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT: 'true'
+          }
+        },
+        {
+          itMsg: 'should fulfill when the end_session_endpoint is provided with GRIST_OIDC_IDP_END_SESSION_ENDPOINT',
+          end_session_endpoint: undefined,
+          env: {
+            GRIST_OIDC_IDP_END_SESSION_ENDPOINT: 'http://localhost:8484/logout'
+          }
+        },
+        {
+          itMsg: 'should fulfill when the end_session_endpoint is provided with the issuer',
+          end_session_endpoint: 'http://localhost:8484/logout',
+        },
+        {
+          itMsg: 'should reject when the end_session_endpoint is not known',
+          errorMsg: /If that is expected, please set GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT/,
+          end_session_endpoint: undefined,
+        }
+      ].forEach((ctx) => {
+        it(ctx.itMsg, async () => {
+          setEnvVars();
+          Object.assign(process.env, ctx.env);
+          const client = new ClientStub();
+          client.issuer.metadata.end_session_endpoint = ctx.end_session_endpoint;
+          const promise = OIDCConfigStubbed.build(client as Client);
+          if (ctx.errorMsg) {
+            await assert.isRejected(promise, ctx.errorMsg);
+          } else {
+            await assert.isFulfilled(promise);
+          }
+        });
+      });
+    });
   });
 
-});
+  describe('GRIST_OIDC_IDP_ENABLED_PROTECTIONS', () => {
+    it('should throw when GRIST_OIDC_IDP_ENABLED_PROTECTIONS contains unsupported values', async () => {
+      setEnvVars();
+      process.env.GRIST_OIDC_IDP_ENABLED_PROTECTIONS = 'STATE,NONCE,PKCE,invalid';
+      const promise = OIDCConfig.build();
+      await assert.isRejected(promise, 'OIDC: Invalid protection in GRIST_OIDC_IDP_ENABLED_PROTECTIONS: invalid');
+    });
 
+    it('should successfully change the supported protections', async function () {
+      setEnvVars();
+      process.env.GRIST_OIDC_IDP_ENABLED_PROTECTIONS = 'NONCE';
+      const config = await OIDCConfigStubbed.build(new ClientStub() as Client);
+      assert.isTrue(config.supportsProtection("NONCE"));
+      assert.isFalse(config.supportsProtection("PKCE"));
+      assert.isFalse(config.supportsProtection("STATE"));
+    });
+
+    it('if omitted, should defaults to "STATE,PKCE"', async function () {
+      setEnvVars();
+      const config = await OIDCConfigStubbed.build(new ClientStub() as Client);
+      assert.isFalse(config.supportsProtection("NONCE"));
+      assert.isTrue(config.supportsProtection("PKCE"));
+      assert.isTrue(config.supportsProtection("STATE"));
+    });
+  });
+});