oidc-middleware: remove logout callback middleware

okta · Jan 6, 2020 · d5e1c6a · d5e1c6a
1 parent 8fe896a
commit d5e1c6a
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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();
         })
       })
     });