Skip to content

Commit

Permalink
refactor: remove automatic lookup of /.well-known/oauth-authorization…
Browse files Browse the repository at this point in the history 
…-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.
  • Loading branch information
@panva
panva committed Oct 24, 2021
1 parent 377b6d7 commit fc87d2b
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 145 deletions.
3 changes: 1 addition & 2 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
2 changes: 0 additions & 2 deletions lib/helpers/consts.js
View file
Original file line number Diff line number Diff line change
@@ -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 = [
Expand Down Expand Up @@ -55,7 +54,6 @@ module.exports = {
HTTP_OPTIONS,
ISSUER_DEFAULTS,
JWT_CONTENT,
OAUTH2_DISCOVERY,
OIDC_DISCOVERY,
REL,
WEBFINGER,
Expand Down
57 changes: 19 additions & 38 deletions lib/issuer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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 */
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
]
},
"dependencies": {
"aggregate-error": "^3.1.0",
"got": "^11.8.0",
"jose": "^2.0.5",
"lru-cache": "^6.0.0",
Expand Down
2 changes: 1 addition & 1 deletion test/client/client_instance.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
Expand Down
116 changes: 16 additions & 100 deletions test/issuer/discover_issuer.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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');
});
});

Expand All @@ -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');
});
});

Expand All @@ -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');
});
});

Expand All @@ -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');
});
});

Expand All @@ -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');
});
});

Expand All @@ -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');
});
});

Expand Down

0 comments on commit fc87d2b

Please sign in to comment.