fix: remove connect-ensure-login to avoid implicit req.session.return…

…To change - OKTA-255316 Resolves: #209
okta · Apr 6, 2020 · b43d2ec · b43d2ec
1 parent 5c42b6c
commit b43d2ec
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 17 deletions.
diff --git a/packages/oidc-middleware/package.json b/packages/oidc-middleware/package.json
@@ -35,7 +35,6 @@
   "dependencies": {
     "@okta/configuration-validation": "^0.4.1",
     "body-parser": "^1.18.2",
-    "connect-ensure-login": "^0.1.1",
     "csurf": "^1.9.0",
     "express": "^4.16.3",
     "lodash": "^4.17.5",

diff --git a/packages/oidc-middleware/src/ExpressOIDC.js b/packages/oidc-middleware/src/ExpressOIDC.js
@@ -87,8 +87,7 @@ module.exports = class ExpressOIDC extends EventEmitter {
           path: '/login'
         },
         loginCallback: {
-          path: '/authorization-code/callback',
-          afterCallback: '/'
+          path: '/authorization-code/callback'
         },
         logout: {
           path: '/logout'

diff --git a/packages/oidc-middleware/src/connectUtil.js b/packages/oidc-middleware/src/connectUtil.js
@@ -86,10 +86,16 @@ connectUtil.createLoginCallbackHandler = context => {
   const customHandler = routes.loginCallback.handler;
 
   if (!customHandler) {
-    return passport.authenticate('oidc', {
-      successReturnToOrRedirect: routes.loginCallback.afterCallback,
-      failureRedirect: routes.loginCallback.failureRedirect
-    });
+    // Passport successReturnToOrRedirect always try req.session.returnTo first if it's assigned
+    // Use successRedirect field if afterCallback url is explicitly set in config
+    const redirectOptions = { failureRedirect: routes.loginCallback.failureRedirect };
+    if (routes.loginCallback.afterCallback) {
+      redirectOptions.successRedirect = routes.loginCallback.afterCallback;
+    } else {
+      redirectOptions.successReturnToOrRedirect = '/';
+    }
+
+    return passport.authenticate('oidc', redirectOptions);
   }
 
   const customHandlerArity = customHandler.length;

diff --git a/packages/oidc-middleware/src/oidcUtil.js b/packages/oidc-middleware/src/oidcUtil.js
@@ -12,7 +12,6 @@
 
 const passport = require('passport');
 const OpenIdClient = require('openid-client');
-const ensureLoggedIn = require('connect-ensure-login').ensureLoggedIn;
 const Negotiator = require('negotiator');
 const os = require('os');
 
@@ -102,15 +101,24 @@ oidcUtil.bootstrapPassportStrategy = context => {
   passport.use('oidc', oidcStrategy);
 };
 
-oidcUtil.ensureAuthenticated = (context, options) => {
-  options = options || context.options.routes.login.path;
+oidcUtil.ensureAuthenticated = (context, options = {}) => {
   return (req, res, next) => {
-    if (req.isAuthenticated && req.isAuthenticated()) {
+    const isAuthenticated = req.isAuthenticated && req.isAuthenticated();
+    if (isAuthenticated) {
       return next();
     }
     const negotiator = new Negotiator(req);
     if (negotiator.mediaType() === 'text/html') {
-      ensureLoggedIn(options)(req, res, next);
+      if (!isAuthenticated) {
+        if (req.session) {
+          req.session.returnTo = req.originalUrl || req.url;
+        }
+
+        const url = options.redirectTo || context.options.routes.login.path;
+        return res.redirect(url);
+      }
+
+      next();
     } else {
       res.sendStatus(401);
     }

diff --git a/packages/oidc-middleware/yarn.lock b/packages/oidc-middleware/yarn.lock
@@ -1112,11 +1112,6 @@ [email protected]:
   resolved "https://registry.yarnpkg.com/concat-map/-/concat-map-0.0.1.tgz#d8a96bd77fd68df7793a73036a3ba0d5405d477b"
   integrity sha1-2Klr13/Wjfd5OnMDajug1UBdR3s=
 
-connect-ensure-login@^0.1.1:
-  version "0.1.1"
-  resolved "https://registry.yarnpkg.com/connect-ensure-login/-/connect-ensure-login-0.1.1.tgz#174dcc51243b9eac23f8d98215aeb6694e2e8a12"
-  integrity sha1-F03MUSQ7nqwj+NmCFa62aU4uihI=
-
 [email protected]:
   version "0.5.3"
   resolved "https://registry.yarnpkg.com/content-disposition/-/content-disposition-0.5.3.tgz#e130caf7e7279087c5616c2007d0485698984fbd"