From d5e1c6ae1a310dfd84b3571dfb96d65b145f4071 Mon Sep 17 00:00:00 2001 From: Aaron Granick Date: Mon, 6 Jan 2020 15:12:17 -0800 Subject: [PATCH] oidc-middleware: remove logout callback middleware --- packages/oidc-middleware/src/ExpressOIDC.js | 6 +----- packages/oidc-middleware/src/connectUtil.js | 12 ------------ packages/oidc-middleware/src/logout.js | 14 +++++++------- packages/oidc-middleware/test/unit/logout.spec.js | 12 +++++------- 4 files changed, 13 insertions(+), 31 deletions(-) diff --git a/packages/oidc-middleware/src/ExpressOIDC.js b/packages/oidc-middleware/src/ExpressOIDC.js index 6dec8a6bf..01925ebb0 100644 --- a/packages/oidc-middleware/src/ExpressOIDC.js +++ b/packages/oidc-middleware/src/ExpressOIDC.js @@ -92,10 +92,6 @@ module.exports = class ExpressOIDC extends EventEmitter { }, logout: { path: '/logout' - }, - logoutCallback: { - path: '/logout/callback', - afterCallback: '/' } }, sessionKey: sessionKey || `oidc:${issuer}`, @@ -104,7 +100,7 @@ module.exports = class ExpressOIDC extends EventEmitter { // Build redirect uri unless explicitly set options.loginRedirectUri = loginRedirectUri || `${appBaseUrl}${options.routes.loginCallback.path}`; - options.logoutRedirectUri = logoutRedirectUri || `${appBaseUrl}${options.routes.logoutCallback.path}`; + options.logoutRedirectUri = logoutRedirectUri || `${appBaseUrl}`; // Validate the redirect_uri param assertRedirectUri(options.loginRedirectUri); diff --git a/packages/oidc-middleware/src/connectUtil.js b/packages/oidc-middleware/src/connectUtil.js index bdafac86f..bd9d8c6e5 100644 --- a/packages/oidc-middleware/src/connectUtil.js +++ b/packages/oidc-middleware/src/connectUtil.js @@ -31,12 +31,10 @@ connectUtil.createOIDCRouter = context => { const loginPath = routes.login.path; const loginCallbackPath = routes.loginCallback.path; const logoutPath = routes.logout.path; - const logoutCallbackPath = routes.logoutCallback.path; oidcRouter.use(loginPath, bodyParser.urlencoded({ extended: false}), connectUtil.createLoginHandler(context)); oidcRouter.use(loginCallbackPath, connectUtil.createLoginCallbackHandler(context)); oidcRouter.post(logoutPath, connectUtil.createLogoutHandler(context)); - oidcRouter.use(logoutCallbackPath, connectUtil.createLogoutCallbackHandler(context)); oidcRouter.use((err, req, res, next) => { // Cast all errors from the passport strategy as 401 (rather than 500, which would happen if we just call through to next()) @@ -115,13 +113,3 @@ connectUtil.createLoginCallbackHandler = context => { connectUtil.createLogoutHandler = context => logout.forceLogoutAndRevoke(context); -connectUtil.createLogoutCallbackHandler = context => { - return (req, res) => { - if ( req.session[context.options.sessionKey].state !== req.query.state ) { - context.emitter.emit('error', new OIDCMiddlewareError('logoutError', `'state' parameter did not match value in session`)); - } else { - req.logout(); - res.redirect(context.options.routes.logoutCallback.afterCallback); - } - }; -}; diff --git a/packages/oidc-middleware/src/logout.js b/packages/oidc-middleware/src/logout.js index 4a7602eaf..53c3e2636 100644 --- a/packages/oidc-middleware/src/logout.js +++ b/packages/oidc-middleware/src/logout.js @@ -53,23 +53,23 @@ logout.forceLogoutAndRevoke = context => { issuer = issuer + '/oauth2'; } const revokeToken = makeTokenRevoker({ issuer, client_id, client_secret, errorHandler: makeErrorHandler(emitter) }); - return async (req, res /*, next */) => { + return async (req, res /*, next */) => { const tokens = req.userContext.tokens; const revokeIfExists = token_hint => tokens[token_hint] ? revokeToken({token_hint, token: tokens[token_hint]}) : null; const revokes = REVOKABLE_TOKENS.map( revokeIfExists ); - // attempt all revokes before logout + + // clear local session + req.logout(); + + // attempt all revokes await Promise.all(revokes); // these capture (emit) all rejections, no wrapping catch needed, no early fail of .all() - const state = uuid.v4(); const params = { - state, id_token_hint: tokens.id_token, post_logout_redirect_uri: context.options.logoutRedirectUri, }; - // TODO: investigate potential race-condition with this line - // eslint-disable-next-line require-atomic-updates - req.session[context.options.sessionKey] = { state }; + // redirect to Okta to clear SSO session const endOktaSessionEndpoint = `${issuer}/v1/logout?${querystring.stringify(params)}`; return res.redirect(endOktaSessionEndpoint); }; diff --git a/packages/oidc-middleware/test/unit/logout.spec.js b/packages/oidc-middleware/test/unit/logout.spec.js index f64a50094..903435ff7 100644 --- a/packages/oidc-middleware/test/unit/logout.spec.js +++ b/packages/oidc-middleware/test/unit/logout.spec.js @@ -23,7 +23,6 @@ describe('logout', () => { const client_secret = 'testSecret'; const id_token = 'testIdToken'; const logoutRedirectUri = 'testLogoutUri'; - const mockState = 'fake-state'; const sessionKey = 'fake-session-key'; testSuite('ORG issuer', orgIssuer); @@ -36,7 +35,7 @@ describe('logout', () => { const baseUri = issuer.indexOf('oauth2') > 0 ? `${issuer}` : `${issuer}/oauth2`; const revokeUri = `${baseUri}/v1/revoke`; - const expectedUri = `${baseUri}/v1/logout?state=${mockState}&id_token_hint=${id_token}&post_logout_redirect_uri=${logoutRedirectUri}`; + const expectedUri = `${baseUri}/v1/logout?id_token_hint=${id_token}&post_logout_redirect_uri=${logoutRedirectUri}`; describe(label, () => { beforeEach(() => { @@ -48,8 +47,6 @@ describe('logout', () => { }); nodeFetch.mockImplementation(fetch); - uuid.v4 = jest.fn().mockReturnValue(mockState); - context = { options: { issuer, @@ -72,7 +69,8 @@ describe('logout', () => { tokens: { id_token } - } + }, + logout: jest.fn() }; res = { redirect: jest.fn() @@ -168,9 +166,9 @@ describe('logout', () => { }); describe('session', () => { - it('sets the session object', async () => { + it('calls req.logout()', async () => { await logout(req, res); - expect(req.session[sessionKey]).toEqual({ state: mockState }); + expect(req.logout).toHaveBeenCalled(); }) }) });