From 3adaf4acc8ee45d219075fbd05289307a00ce1e6 Mon Sep 17 00:00:00 2001 From: Kieran Simpson Date: Fri, 16 Oct 2020 19:01:35 +1100 Subject: [PATCH] Fix to not override option values with defaults. (#127) * Fix to not override option values with defaults. Also added extra test to check `options.apiUrl` had correct default assigned when missing. * Update strategy to user option to hold expected issuer. This also facilitates passing the issuer from an OIDC discovery config. Co-authored-by: David Patrick --- lib/index.js | 5 ++-- lib/verifyWrapper.js | 16 +++++----- test/strategy.test.js | 68 ++++++++++++++++++++++++++++++++++++------- 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/lib/index.js b/lib/index.js index 0f808be..ae6b53f 100644 --- a/lib/index.js +++ b/lib/index.js @@ -46,13 +46,14 @@ function Strategy(options, verify) { ); var defaultOptions = { + expectedIssuer: 'https://' + options.domain + '/', authorizationURL: 'https://' + options.domain + '/authorize', tokenURL: 'https://' + options.domain + '/oauth/token', userInfoURL: 'https://' + options.domain + '/userinfo', apiUrl: 'https://' + options.domain + '/api' } - this.options = Object.assign({}, options, defaultOptions); + this.options = Object.assign({}, defaultOptions, options); if (this.options.state === undefined) { this.options.state = true; @@ -88,7 +89,7 @@ Strategy.prototype.authenticate = function (req, options) { req.session.authParams.scope = options.scope; req.session.authParams.nonce = crypto.randomBytes(16).toString('hex'); this.authParams = req.session.authParams - } + } } else if (options.scope && options.scope.includes('openid')) { throw new Error('Scope "openid" is not allowed without Auth0Strategy state true') } diff --git a/lib/verifyWrapper.js b/lib/verifyWrapper.js index df9f2e5..341220b 100644 --- a/lib/verifyWrapper.js +++ b/lib/verifyWrapper.js @@ -3,9 +3,9 @@ var jwt = require ('./jwt'); /** * Adds ID token validation to Passport verification process. * - * Parent passport-oauth2 library handles the verifier based on the number - * of arguments and changes the order if passReqToCallback is passed - * in with the strategy options. This wrapper will make the length of + * Parent passport-oauth2 library handles the verifier based on the number + * of arguments and changes the order if passReqToCallback is passed + * in with the strategy options. This wrapper will make the length of * arguments consistent and add support for passReqToCallback. * * @param {Function} verify @@ -29,16 +29,16 @@ function verifyWrapper (verify, strategyOptions, authParams) { /** * Perform ID token validation if an ID token was requested during login. - * - * @param {Object} strategyOptions - * @param {Object} authParams - * @param {Object} params + * + * @param {Object} strategyOptions + * @param {Object} authParams + * @param {Object} params */ function handleIdTokenValidation (strategyOptions, authParams, params) { if (authParams && authParams.scope && authParams.scope.includes('openid')) { jwt.verify(params.id_token, { aud: strategyOptions.clientID, - iss: 'https://' + strategyOptions.domain + '/', + iss: strategyOptions.expectedIssuer, leeway: strategyOptions.leeway, maxAge: strategyOptions.maxAge, nonce: authParams.nonce diff --git a/test/strategy.test.js b/test/strategy.test.js index a4ddf2a..0b85452 100644 --- a/test/strategy.test.js +++ b/test/strategy.test.js @@ -15,19 +15,65 @@ describe('auth0 strategy', function () { ); }); - it('authorizationURL should have the domain', function () { - this.strategy.options - .authorizationURL.should.eql('https://test.auth0.com/authorize'); - }); + describe('options', function() { + describe('defaults', function() { + it('expectedIssuer should have the domain', function () { + this.strategy.options + .expectedIssuer.should.eql('https://test.auth0.com/'); + }); - it('tokenURL should have the domain', function () { - this.strategy.options - .tokenURL.should.eql('https://test.auth0.com/oauth/token'); - }); + it('authorizationURL should have the domain', function () { + this.strategy.options + .authorizationURL.should.eql('https://test.auth0.com/authorize'); + }); + + it('tokenURL should have the domain', function () { + this.strategy.options + .tokenURL.should.eql('https://test.auth0.com/oauth/token'); + }); + + it('userInfoURL should have the domain', function () { + this.strategy.options + .userInfoURL.should.eql('https://test.auth0.com/userinfo'); + }); + + it('apiURL should have the domain', function () { + this.strategy.options + .apiUrl.should.eql('https://test.auth0.com/api'); + }); + }); + + it('should not override options with defaults', function() { + const strategy = new Auth0Strategy({ + domain: 'test.auth0.com', + clientID: 'testid', + clientSecret: 'testsecret', + callbackURL: '/callback', + + expectedIssuer: 'https://foobar.com/', + authorizationURL: 'https://foobar.com/authorize', + tokenURL: 'https://foobar.com/oauth/token', + userInfoURL: 'https://foobar.com/userinfo', + apiUrl: 'https://foobar.com/api' + }, + function(accessToken, idToken, profile, done) {} + ); + + strategy.options + .expectedIssuer.should.eql('https://foobar.com/'); - it('userInfoURL should have the domain', function () { - this.strategy.options - .userInfoURL.should.eql('https://test.auth0.com/userinfo'); + strategy.options + .authorizationURL.should.eql('https://foobar.com/authorize'); + + strategy.options + .tokenURL.should.eql('https://foobar.com/oauth/token'); + + strategy.options + .userInfoURL.should.eql('https://foobar.com/userinfo'); + + strategy.options + .apiUrl.should.eql('https://foobar.com/api'); + }); }); it('should include a telemetry header by default', function() {