Merge pull request #2157 from overleaf/jel-saml-logout-redirect

Redirect handling after logging out

GitOrigin-RevId: 01562dbe71ff4f3571fb0d433b96ccca34aad24e

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -11,8 +11,8 @@ const UserSessionsManager = require('../User/UserSessionsManager')
 const Analytics = require('../Analytics/AnalyticsManager')
 const passport = require('passport')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
+const UrlHelper = require('../Helpers/UrlHelper')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
-const { URL } = require('url')
 const _ = require('lodash')
 
 const AuthenticationController = (module.exports = {
@@ -351,7 +351,7 @@ const AuthenticationController = (module.exports = {
       !/^\/(socket.io|js|stylesheets|img)\/.*$/.test(value) &&
       !/^.*\.(png|jpeg|svg)$/.test(value)
     ) {
-      const safePath = AuthenticationController._getSafeRedirectPath(value)
+      const safePath = UrlHelper.getSafeRedirectPath(value)
       return (req.session.postLoginRedirect = safePath)
     }
   },
@@ -433,7 +433,7 @@ const AuthenticationController = (module.exports = {
     let safePath
     const value = _.get(req, ['session', 'postLoginRedirect'])
     if (value) {
-      safePath = AuthenticationController._getSafeRedirectPath(value)
+      safePath = UrlHelper.getSafeRedirectPath(value)
     }
     return safePath || null
   },
@@ -442,15 +442,5 @@ const AuthenticationController = (module.exports = {
     if (req.session != null) {
       delete req.session.postLoginRedirect
     }
-  },
-
-  _getSafeRedirectPath(value) {
-    const baseURL = Settings.siteUrl // base URL is required to construct URL from path
-    const url = new URL(value, baseURL)
-    let safePath = `${url.pathname}${url.search}${url.hash}`
-    if (safePath === '/') {
-      safePath = undefined
-    }
-    return safePath
   }
 })

services/web/app/src/Features/Helpers/UrlHelper.js

@@ -1,18 +1,18 @@
-/* eslint-disable
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let UrlHelper
 const Settings = require('settings-sharelatex')
+const { URL } = require('url')
 
-module.exports = UrlHelper = {
+function getSafeRedirectPath(value) {
+  const baseURL = Settings.siteUrl // base URL is required to construct URL from path
+  const url = new URL(value, baseURL)
+  let safePath = `${url.pathname}${url.search}${url.hash}`
+  if (safePath === '/') {
+    safePath = undefined
+  }
+  return safePath
+}
+
+const UrlHelper = {
+  getSafeRedirectPath,
   wrapUrlWithProxy(url) {
     // TODO: Consider what to do for Community and Enterprise edition?
     if (!Settings.apis.linkedUrlProxy.url) {
@@ -28,3 +28,5 @@ module.exports = UrlHelper = {
     return url
   }
 }
+
+module.exports = UrlHelper

services/web/app/src/Features/User/UserController.js

@@ -15,6 +15,7 @@ const Errors = require('../Errors/Errors')
 const OError = require('@overleaf/o-error')
 const HttpErrors = require('@overleaf/o-error/http')
 const EmailHandler = require('../Email/EmailHandler')
+const UrlHelper = require('../Helpers/UrlHelper')
 
 const UserController = {
   tryDeleteUser(req, res, next) {
@@ -253,11 +254,15 @@ const UserController = {
   },
 
   logout(req, res, next) {
+    const requestedRedirect = req.body.redirect
+      ? UrlHelper.getSafeRedirectPath(req.body.redirect)
+      : undefined
+    const redirectUrl = requestedRedirect || '/login'
+
     UserController._doLogout(req, err => {
       if (err != null) {
         return next(err)
       }
-      const redirectUrl = '/login'
       res.redirect(redirectUrl)
     })
   },

services/web/test/unit/src/Authentication/AuthenticationControllerTests.js

@@ -80,9 +80,16 @@ describe('AuthenticationController', function() {
           server: {
             authenticate: sinon.stub()
           }
+        }),
+        '../Helpers/UrlHelper': (this.UrlHelper = {
+          getSafeRedirectPath: sinon.stub()
         })
       }
     })
+    this.UrlHelper.getSafeRedirectPath
+      .withArgs('https://evil.com')
+      .returns(undefined)
+    this.UrlHelper.getSafeRedirectPath.returnsArg(0)
     this.user = {
       _id: ObjectId(),
       email: (this.email = 'USER@example.com'),
@@ -104,10 +111,7 @@ describe('AuthenticationController', function() {
 
   describe('isUserLoggedIn', function() {
     beforeEach(function() {
-      return (this.stub = sinon.stub(
-        this.AuthenticationController,
-        'getLoggedInUserId'
-      ))
+      this.stub = sinon.stub(this.AuthenticationController, 'getLoggedInUserId')
     })
 
     afterEach(function() {
@@ -1010,32 +1014,6 @@ describe('AuthenticationController', function() {
     })
   })
 
-  describe('_getSafeRedirectPath', function() {
-    it('sanitize redirect path to prevent open redirects', function() {
-      expect(
-        this.AuthenticationController._getSafeRedirectPath('https://evil.com')
-      ).to.be.undefined
-
-      expect(this.AuthenticationController._getSafeRedirectPath('//evil.com'))
-        .to.be.undefined
-
-      expect(
-        this.AuthenticationController._getSafeRedirectPath('//ol.com/evil')
-      ).to.equal('/evil')
-
-      expect(this.AuthenticationController._getSafeRedirectPath('////evil.com'))
-        .to.be.undefined
-
-      expect(
-        this.AuthenticationController._getSafeRedirectPath('%2F%2Fevil.com')
-      ).to.equal('/%2F%2Fevil.com')
-
-      return expect(
-        this.AuthenticationController._getSafeRedirectPath('.evil.com')
-      ).to.equal('/.evil.com')
-    })
-  })
-
   describe('_clearRedirectFromSession', function() {
     beforeEach(function() {
       return (this.req = { session: { postLoginRedirect: '/a?b=c' } })

services/web/test/unit/src/HelperFiles/UrlHelperTests.js

@@ -0,0 +1,35 @@
+const chai = require('chai')
+const { expect } = chai
+const SandboxedModule = require('sandboxed-module')
+const modulePath = require('path').join(
+  __dirname,
+  '../../../../app/src/Features/Helpers/UrlHelper.js'
+)
+
+describe('UrlHelper', function() {
+  beforeEach(function() {
+    this.UrlHelper = SandboxedModule.require(modulePath, {})
+  })
+  describe('getSafeRedirectPath', function() {
+    it('sanitize redirect path to prevent open redirects', function() {
+      expect(this.UrlHelper.getSafeRedirectPath('https://evil.com')).to.be
+        .undefined
+
+      expect(this.UrlHelper.getSafeRedirectPath('//evil.com')).to.be.undefined
+
+      expect(this.UrlHelper.getSafeRedirectPath('//ol.com/evil')).to.equal(
+        '/evil'
+      )
+
+      expect(this.UrlHelper.getSafeRedirectPath('////evil.com')).to.be.undefined
+
+      expect(this.UrlHelper.getSafeRedirectPath('%2F%2Fevil.com')).to.equal(
+        '/%2F%2Fevil.com'
+      )
+
+      return expect(this.UrlHelper.getSafeRedirectPath('.evil.com')).to.equal(
+        '/.evil.com'
+      )
+    })
+  })
+})

services/web/test/unit/src/User/UserControllerTests.js

@@ -460,6 +460,27 @@ describe('UserController', function() {
 
       return this.UserController.logout(this.req, this.res)
     })
+
+    it('should redirect after logout', function(done) {
+      this.req.body.redirect = '/institutional-login'
+      this.req.session.destroy = sinon.stub().callsArgWith(0)
+      this.SudoModeHandler.clearSudoMode = sinon.stub()
+      this.res.redirect = url => {
+        url.should.equal(this.req.body.redirect)
+        return done()
+      }
+      return this.UserController.logout(this.req, this.res)
+    })
+
+    it('should redirect to login after logout when no redirect set', function(done) {
+      this.req.session.destroy = sinon.stub().callsArgWith(0)
+      this.SudoModeHandler.clearSudoMode = sinon.stub()
+      this.res.redirect = url => {
+        url.should.equal('/login')
+        return done()
+      }
+      return this.UserController.logout(this.req, this.res)
+    })
   })
 
   describe('register', function() {