From fc87d2bcb3de2a389f5bbe669779cb671325d69e Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Tue, 19 Oct 2021 11:47:12 +0200 Subject: [PATCH] refactor: remove automatic lookup of /.well-known/oauth-authorization-server BREAKING CHANGE: Issuer.discover will no longer attempt to load `/.well-known/oauth-authorization-server`. To load such discovery documents pass full well-known URL to Issuer.discover. --- README.md | 3 +- lib/client.js | 2 +- lib/helpers/consts.js | 2 - lib/issuer.js | 57 +++++--------- package.json | 1 - test/client/client_instance.test.js | 2 +- test/issuer/discover_issuer.test.js | 116 ++++------------------------ 7 files changed, 38 insertions(+), 145 deletions(-) diff --git a/README.md b/README.md index 586a506a..3ea9ea9a 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ openid-client. - client_secret_jwt - private_key_jwt - Consuming Self-Issued OpenID Provider ID Token response -- [RFC8414 - OAuth 2.0 Authorization Server Metadata][feature-oauth-discovery] and [OpenID Connect Discovery 1.0][feature-discovery] +- [OpenID Connect Discovery 1.0][feature-discovery] - Discovery of OpenID Provider (Issuer) Metadata - Discovery of OpenID Provider (Issuer) Metadata via user provided inputs (via [webfinger][documentation-webfinger]) - [OpenID Connect Dynamic Client Registration 1.0][feature-registration] @@ -288,7 +288,6 @@ See [Customizing (docs)](https://github.com/panva/node-openid-client/blob/master [openid-connect]: https://openid.net/connect/ [feature-core]: https://openid.net/specs/openid-connect-core-1_0.html [feature-discovery]: https://openid.net/specs/openid-connect-discovery-1_0.html -[feature-oauth-discovery]: https://tools.ietf.org/html/rfc8414 [feature-registration]: https://openid.net/specs/openid-connect-registration-1_0.html [feature-revocation]: https://tools.ietf.org/html/rfc7009 [feature-introspection]: https://tools.ietf.org/html/rfc7662 diff --git a/lib/client.js b/lib/client.js index 43c7524f..69ca0f6f 100644 --- a/lib/client.js +++ b/lib/client.js @@ -113,7 +113,7 @@ async function claimJWT(label, jwt) { } return jose.JWT.verify(jwt, key); } catch (err) { - if (err instanceof RPError || err instanceof OPError || err.name === 'AggregateError') { + if (err instanceof RPError || err instanceof OPError) { throw err; } else { throw new RPError({ diff --git a/lib/helpers/consts.js b/lib/helpers/consts.js index 41e20ca9..f652be81 100644 --- a/lib/helpers/consts.js +++ b/lib/helpers/consts.js @@ -1,5 +1,4 @@ const OIDC_DISCOVERY = '/.well-known/openid-configuration'; -const OAUTH2_DISCOVERY = '/.well-known/oauth-authorization-server'; const WEBFINGER = '/.well-known/webfinger'; const REL = 'http://openid.net/specs/connect/1.0/issuer'; const AAD_MULTITENANT_DISCOVERY = [ @@ -55,7 +54,6 @@ module.exports = { HTTP_OPTIONS, ISSUER_DEFAULTS, JWT_CONTENT, - OAUTH2_DISCOVERY, OIDC_DISCOVERY, REL, WEBFINGER, diff --git a/lib/issuer.js b/lib/issuer.js index 58e8a6fc..1b6170ca 100644 --- a/lib/issuer.js +++ b/lib/issuer.js @@ -3,7 +3,6 @@ const { inspect } = require('util'); const url = require('url'); -const AggregateError = require('aggregate-error'); const jose = require('jose'); const LRU = require('lru-cache'); const objectHash = require('object-hash'); @@ -17,7 +16,7 @@ const instance = require('./helpers/weak_cache'); const request = require('./helpers/request'); const { assertIssuerConfiguration } = require('./helpers/assert'); const { - ISSUER_DEFAULTS, OIDC_DISCOVERY, OAUTH2_DISCOVERY, WEBFINGER, REL, AAD_MULTITENANT_DISCOVERY, + ISSUER_DEFAULTS, OIDC_DISCOVERY, WEBFINGER, REL, AAD_MULTITENANT_DISCOVERY, } = require('./helpers/consts'); const AAD_MULTITENANT = Symbol('AAD_MULTITENANT'); @@ -226,46 +225,28 @@ class Issuer { }); } - const pathnames = []; + let pathname; if (parsed.pathname.endsWith('/')) { - pathnames.push(`${parsed.pathname}${OIDC_DISCOVERY.substring(1)}`); + pathname = `${parsed.pathname}${OIDC_DISCOVERY.substring(1)}`; } else { - pathnames.push(`${parsed.pathname}${OIDC_DISCOVERY}`); - } - if (parsed.pathname === '/') { - pathnames.push(`${OAUTH2_DISCOVERY}`); - } else { - pathnames.push(`${OAUTH2_DISCOVERY}${parsed.pathname}`); - } - - const errors = []; - // eslint-disable-next-line no-restricted-syntax - for (const pathname of pathnames) { - try { - const wellKnownUri = url.format({ ...parsed, pathname }); - // eslint-disable-next-line no-await-in-loop - const response = await request.call(this, { - method: 'GET', - responseType: 'json', - url: wellKnownUri, - }); - const body = processResponse(response); - return new Issuer({ - ...ISSUER_DEFAULTS, - ...body, - [AAD_MULTITENANT]: !!AAD_MULTITENANT_DISCOVERY.find( - (discoveryURL) => wellKnownUri.startsWith(discoveryURL), - ), - }); - } catch (err) { - errors.push(err); - } + pathname = `${parsed.pathname}${OIDC_DISCOVERY}`; } - const err = new AggregateError(errors); - err.message = `Issuer.discover() failed.${err.message.split('\n') - .filter((line) => !line.startsWith(' at')).join('\n')}`; - throw err; + const wellKnownUri = url.format({ ...parsed, pathname }); + // eslint-disable-next-line no-await-in-loop + const response = await request.call(this, { + method: 'GET', + responseType: 'json', + url: wellKnownUri, + }); + const body = processResponse(response); + return new Issuer({ + ...ISSUER_DEFAULTS, + ...body, + [AAD_MULTITENANT]: !!AAD_MULTITENANT_DISCOVERY.find( + (discoveryURL) => wellKnownUri.startsWith(discoveryURL), + ), + }); } /* istanbul ignore next */ diff --git a/package.json b/package.json index 1e534a9e..4e69f708 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,6 @@ ] }, "dependencies": { - "aggregate-error": "^3.1.0", "got": "^11.8.0", "jose": "^2.0.5", "lru-cache": "^6.0.0", diff --git a/test/client/client_instance.test.js b/test/client/client_instance.test.js index df4a886e..d4b30600 100644 --- a/test/client/client_instance.test.js +++ b/test/client/client_instance.test.js @@ -3295,7 +3295,7 @@ describe('Client', () => { return this.client.unpackAggregatedClaims(userinfo) .then(fail, (error) => { expect(discovery.isDone()).to.be.true; - expect(error.name).to.equal('AggregateError'); + expect(error.name).to.equal('OPError'); expect(error.src).to.equal('cliff'); }); }); diff --git a/test/issuer/discover_issuer.test.js b/test/issuer/discover_issuer.test.js index de6f5bae..cfbf57af 100644 --- a/test/issuer/discover_issuer.test.js +++ b/test/issuer/discover_issuer.test.js @@ -125,30 +125,6 @@ describe('Issuer#discover()', () => { expect(issuer).to.have.property('issuer', 'https://op.example.com/oauth2'); }); }); - - it('can be discovered by ommiting the well-known part', function () { - nock('https://op.example.com', { allowUnmocked: true }) - .get('/.well-known/oauth-authorization-server') - .reply(200, { - issuer: 'https://op.example.com', - }); - - return Issuer.discover('https://op.example.com').then(function (issuer) { - expect(issuer).to.have.property('issuer', 'https://op.example.com'); - }); - }); - - it('discovering issuers with path components (with trailing slash)', function () { - nock('https://op.example.com', { allowUnmocked: true }) - .get('/.well-known/oauth-authorization-server/oauth2/') - .reply(200, { - issuer: 'https://op.example.com/oauth2', - }); - - return Issuer.discover('https://op.example.com/oauth2/').then(function (issuer) { - expect(issuer).to.have.property('issuer', 'https://op.example.com/oauth2'); - }); - }); }); it('assigns Discovery 1.0 defaults 1/2', function () { @@ -209,19 +185,9 @@ describe('Issuer#discover()', () => { return Issuer.discover('https://op.example.com') .then(fail, function (error) { - expect(error.name).to.equal('AggregateError'); - expect(error.message).to.match(/^Issuer\.discover\(\) failed\.\n/); - expect(error.message).not.to.contain('node_modules'); - expect([...error].some((err) => { - try { - expect(err.name).to.equal('OPError'); - expect(err).to.have.property('error', 'server_error'); - expect(err).to.have.property('error_description', 'bad things are happening'); - return true; - } catch (e) { - return false; - } - })).to.be.true; + expect(error.name).to.equal('OPError'); + expect(error).to.have.property('error', 'server_error'); + expect(error).to.have.property('error_description', 'bad things are happening'); }); }); @@ -243,19 +209,9 @@ describe('Issuer#discover()', () => { return Issuer.discover('https://op.example.com') .then(fail, function (error) { - expect(error.name).to.equal('AggregateError'); - expect(error.message).to.match(/^Issuer\.discover\(\) failed\.\n/); - expect(error.message).not.to.contain('node_modules'); - expect([...error].some((err) => { - try { - expect(err.name).to.equal('OPError'); - expect(err.message).to.eql('expected 200 OK, got: 400 Bad Request'); - expect(err).to.have.property('response'); - return true; - } catch (e) { - return false; - } - })).to.be.true; + expect(error.name).to.equal('OPError'); + expect(error.message).to.eql('expected 200 OK, got: 400 Bad Request'); + expect(error).to.have.property('response'); }); }); @@ -266,19 +222,9 @@ describe('Issuer#discover()', () => { return Issuer.discover('https://op.example.com') .then(fail, function (error) { - expect(error.name).to.equal('AggregateError'); - expect(error.message).to.match(/^Issuer\.discover\(\) failed\.\n/); - expect(error.message).not.to.contain('node_modules'); - expect([...error].some((err) => { - try { - expect(err.name).to.equal('OPError'); - expect(err.message).to.eql('expected 200 OK, got: 500 Internal Server Error'); - expect(err).to.have.property('response'); - return true; - } catch (e) { - return false; - } - })).to.be.true; + expect(error.name).to.equal('OPError'); + expect(error.message).to.eql('expected 200 OK, got: 500 Internal Server Error'); + expect(error).to.have.property('response'); }); }); @@ -289,19 +235,9 @@ describe('Issuer#discover()', () => { return Issuer.discover('https://op.example.com') .then(fail, function (error) { - expect(error.name).to.equal('AggregateError'); - expect(error.message).to.match(/^Issuer\.discover\(\) failed\.\n/); - expect(error.message).not.to.contain('node_modules'); - expect([...error].some((err) => { - try { - expect(err.name).to.eql('ParseError'); - expect(err.message).to.eql('Unexpected token } in JSON at position 12 in "https://op.example.com/.well-known/openid-configuration"'); - expect(err).to.have.property('response'); - return true; - } catch (e) { - return false; - } - })).to.be.true; + expect(error.name).to.eql('ParseError'); + expect(error.message).to.eql('Unexpected token } in JSON at position 12 in "https://op.example.com/.well-known/openid-configuration"'); + expect(error).to.have.property('response'); }); }); @@ -312,18 +248,8 @@ describe('Issuer#discover()', () => { return Issuer.discover('https://op.example.com') .then(fail, function (error) { - expect(error.name).to.equal('AggregateError'); - expect(error.message).to.match(/^Issuer\.discover\(\) failed\.\n/); - expect(error.message).not.to.contain('node_modules'); - expect([...error].some((err) => { - try { - expect(err.name).to.equal('OPError'); - expect(err).to.have.property('message', 'expected 200 OK with body but no body was returned'); - return true; - } catch (e) { - return false; - } - })).to.be.true; + expect(error.name).to.equal('OPError'); + expect(error).to.have.property('message', 'expected 200 OK with body but no body was returned'); }); }); @@ -334,18 +260,8 @@ describe('Issuer#discover()', () => { return Issuer.discover('https://op.example.com') .then(fail, function (error) { - expect(error.name).to.equal('AggregateError'); - expect(error.message).to.match(/^Issuer\.discover\(\) failed\.\n/); - expect(error.message).not.to.contain('node_modules'); - expect([...error].some((err) => { - try { - expect(err.name).to.equal('OPError'); - expect(err).to.have.property('message', 'expected 200 OK, got: 301 Moved Permanently'); - return true; - } catch (e) { - return false; - } - })).to.be.true; + expect(error.name).to.equal('OPError'); + expect(error).to.have.property('message', 'expected 200 OK, got: 301 Moved Permanently'); }); });