discovery errors should be handled in express middleware (#371)

auth0 · Jun 29, 2022 · 0849939 · 0849939
1 parent 000dcff
commit 0849939
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 59 deletions.
diff --git a/lib/context.js b/lib/context.js
@@ -179,50 +179,52 @@ class ResponseContext {
   async login(options = {}) {
     let { config, req, res, next, transient } = weakRef(this);
     next = cb(next).once();
-    const client = await getClient(config);
-
-    // Set default returnTo value, allow passed-in options to override or use originalUrl on GET
-    let returnTo = config.baseURL;
-    if (options.returnTo) {
-      returnTo = options.returnTo;
-      debug('req.oidc.login() called with returnTo: %s', returnTo);
-    } else if (req.method === 'GET' && req.originalUrl) {
-      // Collapse any leading slashes to a single slash to prevent Open Redirects
-      returnTo = req.originalUrl.replace(/^\/+/, '/');
-      debug('req.oidc.login() without returnTo, using: %s', returnTo);
-    }
+    try {
+      const client = await getClient(config);
+
+      // Set default returnTo value, allow passed-in options to override or use originalUrl on GET
+      let returnTo = config.baseURL;
+      if (options.returnTo) {
+        returnTo = options.returnTo;
+        debug('req.oidc.login() called with returnTo: %s', returnTo);
+      } else if (req.method === 'GET' && req.originalUrl) {
+        // Collapse any leading slashes to a single slash to prevent Open Redirects
+        returnTo = req.originalUrl.replace(/^\/+/, '/');
+        debug('req.oidc.login() without returnTo, using: %s', returnTo);
+      }
 
-    options = {
-      authorizationParams: {},
-      returnTo,
-      ...options,
-    };
+      options = {
+        authorizationParams: {},
+        returnTo,
+        ...options,
+      };
 
-    // Ensure a redirect_uri, merge in configuration options, then passed-in options.
-    options.authorizationParams = {
-      redirect_uri: this.getRedirectUri(),
-      ...config.authorizationParams,
-      ...options.authorizationParams,
-    };
+      // Ensure a redirect_uri, merge in configuration options, then passed-in options.
+      options.authorizationParams = {
+        redirect_uri: this.getRedirectUri(),
+        ...config.authorizationParams,
+        ...options.authorizationParams,
+      };
 
-    const stateValue = await config.getLoginState(req, options);
-    if (typeof stateValue !== 'object') {
-      next(new Error('Custom state value must be an object.'));
-    }
-    stateValue.nonce = transient.generateNonce();
-    if (options.silent) {
-      stateValue.attemptingSilentLogin = true;
-    }
+      const stateValue = await config.getLoginState(req, options);
+      if (typeof stateValue !== 'object') {
+        next(new Error('Custom state value must be an object.'));
+      }
+      stateValue.nonce = transient.generateNonce();
+      if (options.silent) {
+        stateValue.attemptingSilentLogin = true;
+      }
 
-    const usePKCE = options.authorizationParams.response_type.includes('code');
-    if (usePKCE) {
-      debug(
-        'response_type includes code, the authorization request will use PKCE'
+      const usePKCE = options.authorizationParams.response_type.includes(
+        'code'
       );
-      stateValue.code_verifier = transient.generateCodeVerifier();
-    }
+      if (usePKCE) {
+        debug(
+          'response_type includes code, the authorization request will use PKCE'
+        );
+        stateValue.code_verifier = transient.generateCodeVerifier();
+      }
 
-    try {
       const validResponseTypes = ['id_token', 'code id_token', 'code'];
       assert(
         validResponseTypes.includes(options.authorizationParams.response_type),
@@ -275,31 +277,31 @@ class ResponseContext {
   async logout(params = {}) {
     let { config, req, res, next } = weakRef(this);
     next = cb(next).once();
-    const client = await getClient(config);
-
     let returnURL = params.returnTo || config.routes.postLogoutRedirect;
     debug('req.oidc.logout() with return url: %s', returnURL);
 
-    if (url.parse(returnURL).host === null) {
-      returnURL = urlJoin(config.baseURL, returnURL);
-    }
+    try {
+      const client = await getClient(config);
 
-    cancelSilentLogin(req, res);
+      if (url.parse(returnURL).host === null) {
+        returnURL = urlJoin(config.baseURL, returnURL);
+      }
 
-    if (!req.oidc.isAuthenticated()) {
-      debug('end-user already logged out, redirecting to %s', returnURL);
-      return res.redirect(returnURL);
-    }
+      cancelSilentLogin(req, res);
 
-    const { idToken: id_token_hint } = req.oidc;
-    req[config.session.name] = undefined;
+      if (!req.oidc.isAuthenticated()) {
+        debug('end-user already logged out, redirecting to %s', returnURL);
+        return res.redirect(returnURL);
+      }
 
-    if (!config.idpLogout) {
-      debug('performing a local only logout, redirecting to %s', returnURL);
-      return res.redirect(returnURL);
-    }
+      const { idToken: id_token_hint } = req.oidc;
+      req[config.session.name] = undefined;
+
+      if (!config.idpLogout) {
+        debug('performing a local only logout, redirecting to %s', returnURL);
+        return res.redirect(returnURL);
+      }
 
-    try {
       returnURL = client.endSessionUrl({
         ...config.logoutParams,
         ...params.logoutParams,

diff --git a/test/login.tests.js b/test/login.tests.js
@@ -1,6 +1,7 @@
 const assert = require('chai').assert;
 const url = require('url');
 const querystring = require('querystring');
+const nock = require('nock');
 const request = require('request-promise-native').defaults({
   simple: false,
   resolveWithFullResponse: true,
@@ -419,4 +420,32 @@ describe('auth', () => {
 
     assert.include(fetchAuthCookie(res), 'SameSite=None');
   });
+
+  it('should pass discovery errors to the express mw', async () => {
+    nock('https://example.com')
+      .get('/.well-known/openid-configuration')
+      .reply(500);
+    nock('https://example.com')
+      .get('/.well-known/oauth-authorization-server')
+      .reply(500);
+
+    server = await createServer(
+      auth({
+        ...defaultConfig,
+        issuerBaseURL: 'https://example.com',
+      })
+    );
+    const res = await request.get('/login', {
+      baseUrl,
+      followRedirect: false,
+      json: true,
+    });
+    assert.equal(res.statusCode, 500);
+    console.log(res.body.err.message);
+    assert.match(
+      res.body.err.message,
+      /^Issuer.discover\(\) failed/,
+      'Should get error json from server error middleware'
+    );
+  });
 });
diff --git a/test/logout.tests.js b/test/logout.tests.js
@@ -1,3 +1,4 @@
+const nock = require('nock');
 const { assert } = require('chai');
 const { URL } = require('url');
 const { create: createServer } = require('./fixture/server');
@@ -320,4 +321,33 @@ describe('logout route', async () => {
     assert.isFalse(url.searchParams.has('baz'));
     assert.equal(url.searchParams.get('qux'), '');
   });
+
+  it('should pass discovery errors to the express mw', async () => {
+    nock('https://example.com')
+      .get('/.well-known/openid-configuration')
+      .reply(500);
+    nock('https://example.com')
+      .get('/.well-known/oauth-authorization-server')
+      .reply(500);
+
+    server = await createServer(
+      auth({
+        ...defaultConfig,
+        issuerBaseURL: 'https://example.com',
+      })
+    );
+    const res = await request.get({
+      uri: '/logout',
+      baseUrl: 'http://localhost:3000',
+      followRedirect: false,
+      json: true,
+    });
+    assert.equal(res.statusCode, 500);
+    console.log(res.body.err.message);
+    assert.match(
+      res.body.err.message,
+      /^Issuer.discover\(\) failed/,
+      'Should get error json from server error middleware'
+    );
+  });
 });
diff --git a/test/setup.js b/test/setup.js
@@ -7,22 +7,22 @@ let warn;
 
 beforeEach(function () {
   warn = sinon.stub(global.console, 'warn');
-  nock('https://op.example.com', { allowUnmocked: true })
+  nock('https://op.example.com')
     .persist()
     .get('/.well-known/openid-configuration')
     .reply(200, wellKnown);
 
-  nock('https://op.example.com', { allowUnmocked: true })
+  nock('https://op.example.com')
     .persist()
     .get('/.well-known/jwks.json')
     .reply(200, certs.jwks);
 
-  nock('https://test.eu.auth0.com', { allowUnmocked: true })
+  nock('https://test.eu.auth0.com')
     .persist()
     .get('/.well-known/openid-configuration')
     .reply(200, { ...wellKnown, end_session_endpoint: undefined });
 
-  nock('https://test.eu.auth0.com', { allowUnmocked: true })
+  nock('https://test.eu.auth0.com')
     .persist()
     .get('/.well-known/jwks.json')
     .reply(200, certs.jwks);