From 5b0cd78f00aab4ad80c4272f82e2bcb3757b1e0d Mon Sep 17 00:00:00 2001 From: oleksandrpravosudko-okta <71440851+oleksandrpravosudko-okta@users.noreply.github.com> Date: Tue, 24 Nov 2020 10:55:50 +0200 Subject: [PATCH] Update oidcUtil to allow a callback without userinfo [revisited] (#949) * Update oidcUtil to allow a callback without userinfo * replace undefined done reference with last argument * fall back to fixed parameters number in verification callback * add UT for passport strategy verification callback * preserve conditional branch for response w/o token * remove debug output * remove extra blank line and place semicolons consistently * bump oidc-middleware to 4.0.2 and update CHANGELOG Co-authored-by: Bradley Cornford --- packages/oidc-middleware/CHANGELOG.md | 7 ++ packages/oidc-middleware/package.json | 2 +- packages/oidc-middleware/src/oidcUtil.js | 30 ++++-- .../test/unit/oidcUtil.spec.js | 99 +++++++++++++++++++ 4 files changed, 130 insertions(+), 8 deletions(-) create mode 100644 packages/oidc-middleware/test/unit/oidcUtil.spec.js diff --git a/packages/oidc-middleware/CHANGELOG.md b/packages/oidc-middleware/CHANGELOG.md index 2e5eb48f1..c32115765 100644 --- a/packages/oidc-middleware/CHANGELOG.md +++ b/packages/oidc-middleware/CHANGELOG.md @@ -1,3 +1,10 @@ +# 4.0.2 + +### Bug Fixes + +- [#949](https://github.com/okta/okta-oidc-js/pull/949) + - oidcUtil: support for passport strategy callback without userinfo + # 4.0.1 ### Bug Fixes diff --git a/packages/oidc-middleware/package.json b/packages/oidc-middleware/package.json index 8779adf88..3b3f0ec03 100644 --- a/packages/oidc-middleware/package.json +++ b/packages/oidc-middleware/package.json @@ -1,6 +1,6 @@ { "name": "@okta/oidc-middleware", - "version": "4.0.1", + "version": "4.0.2", "description": "OpenId Connect middleware for authorization code flows", "repository": "https://github.com/okta/okta-oidc-js", "homepage": "https://github.com/okta/okta-oidc-js/tree/master/packages/oidc-middleware", diff --git a/packages/oidc-middleware/src/oidcUtil.js b/packages/oidc-middleware/src/oidcUtil.js index dd9505e22..ca8862486 100644 --- a/packages/oidc-middleware/src/oidcUtil.js +++ b/packages/oidc-middleware/src/oidcUtil.js @@ -86,13 +86,29 @@ oidcUtil.bootstrapPassportStrategy = context => { }, sessionKey: context.options.sessionKey, client: context.client - }, (tokenSet, userinfo, done) => { - return tokenSet && userinfo - ? done(null, { - userinfo: userinfo, - tokens: tokenSet - }) - : done(null); + }, (tokenSet, callbackArg1, callbackArg2) => { + let done; + let userinfo; + + if (typeof(callbackArg2) !== 'undefined') { + done = callbackArg2; + userinfo = callbackArg1; + } else { + done = callbackArg1; + } + + if(tokenSet) { + return userinfo ? + done(null, { + userinfo, + tokens: tokenSet, + }) : + done(null, { + tokens: tokenSet + }); + } else { + return done(null); + } }); // bypass passport's serializers diff --git a/packages/oidc-middleware/test/unit/oidcUtil.spec.js b/packages/oidc-middleware/test/unit/oidcUtil.spec.js new file mode 100644 index 000000000..cda26b744 --- /dev/null +++ b/packages/oidc-middleware/test/unit/oidcUtil.spec.js @@ -0,0 +1,99 @@ +const passport = require('passport'); +const OpenIdClient = require('openid-client'); +const oidcUtil = require('../../src/oidcUtil.js'); + +function createMockOpenIdClient(config={}) { + const Issuer = OpenIdClient.Issuer; + + const defaultConfig = { + issuer: 'https://foo', + token_endpoint: 'https://foo/token', + userinfo_endpoint: 'https://foo/userinfo' + }; + const issuer = new Issuer({ + ...defaultConfig, + ...config + }); + const client = new issuer.Client({ + client_id: 'foo', + client_secret: 'foo', + }); + + client.callback = jest.fn(() => ({ + access_token: 'token_value' + })); + + client.userinfo = jest.fn(() => ({ + cid: '123' + })); + + return client; +} + +function createMockRedirectRequest() { + const request = jest.mock(); + request.method = 'GET'; + request.url = 'http://foo/authorization-code/callback?code=foo'; + request.session = { + 'oidc:foo': { + response_type: 'code', + }, + }; + return request; +} + +describe('oidcUtil', function () { + describe('Passport strategy setup', function () { + beforeEach(function () { + jest.clearAllMocks(); + }); + + it('includes verification function which tolerates authentication repsonses w/o userInfo', function (next) { + const passportStrategySetter = jest.spyOn(passport, 'use').mockImplementation(() => {}); + + const context = { + options: { + scope: ['openid'] + }, + client: createMockOpenIdClient({ + userinfo_endpoint: undefined + }) + }; + oidcUtil.bootstrapPassportStrategy(context); + const passportStrategy = passportStrategySetter.mock.calls[0][1]; + + passportStrategy.success = function (response) { + expect(response.userinfo).toBe(undefined); + expect(response.tokens).toEqual({access_token: 'token_value'}); + next(); + } + passportStrategy.error = function(error) { + expect(error).toEqual(undefined); + }; + passportStrategy.authenticate(createMockRedirectRequest()); + }); + + it('includes verification function which returns userinfo whenever it is available', function (next) { + const passportStrategySetter = jest.spyOn(passport, 'use').mockImplementation(() => {}); + const context = { + options: { + scope: ['openid', 'profile'] + }, + client: createMockOpenIdClient() + }; + + oidcUtil.bootstrapPassportStrategy(context); + const passportStrategy = passportStrategySetter.mock.calls[0][1]; + + passportStrategy.success = function (response) { + expect(response.userinfo).toEqual({cid: '123'}); + expect(response.tokens).toEqual({access_token: 'token_value'}); + next(); + }; + passportStrategy.error = function(error) { + expect(error).toEqual(undefined); + }; + passportStrategy.authenticate(createMockRedirectRequest()); + }) + }) +})