Merge pull request #15519 from overleaf/em-upgrade-passport

Upgrade passport GitOrigin-RevId: b93bfcab39ba3d2ab4efb4814371defec8ca95c4
2024-11-21 20:47:08 -05:00 · 2023-12-11 12:43:22 +01:00 · 2023-12-11 12:43:22 +01:00 · d96283e593
commit d96283e593
parent 96c11e8755
6 changed files with 85 additions and 95 deletions
--- a/package-lock.json
+++ b/package-lock.json
@ -30573,15 +30573,20 @@
      }
    },
    "node_modules/passport": {
-      "version": "0.4.1",
-      "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz",
-      "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==",
+      "version": "0.6.0",
+      "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz",
+      "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==",
      "dependencies": {
        "passport-strategy": "1.x.x",
-        "pause": "0.0.1"
+        "pause": "0.0.1",
+        "utils-merge": "^1.0.1"
      },
      "engines": {
        "node": ">= 0.4.0"
+      },
+      "funding": {
+        "type": "github",
+        "url": "https://github.com/sponsors/jaredhanson"
      }
    },
    "node_modules/passport-google-oauth20": {
@ -43525,7 +43530,7 @@
        "otplib": "^12.0.1",
        "p-limit": "^2.3.0",
        "parse-data-url": "^2.0.0",
-        "passport": "^0.4.1",
+        "passport": "^0.6.0",
        "passport-google-oauth20": "^2.0.0",
        "passport-ldapauth": "^2.1.4",
        "passport-local": "^1.0.0",
@ -52267,7 +52272,7 @@
        "otplib": "^12.0.1",
        "p-limit": "^2.3.0",
        "parse-data-url": "^2.0.0",
-        "passport": "^0.4.1",
+        "passport": "^0.6.0",
        "passport-google-oauth20": "^2.0.0",
        "passport-ldapauth": "^2.1.4",
        "passport-local": "^1.0.0",
@ -70997,12 +71002,13 @@
      }
    },
    "passport": {
-      "version": "0.4.1",
-      "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz",
-      "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==",
+      "version": "0.6.0",
+      "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz",
+      "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==",
      "requires": {
        "passport-strategy": "1.x.x",
-        "pause": "0.0.1"
+        "pause": "0.0.1",
+        "utils-merge": "^1.0.1"
      }
    },
    "passport-google-oauth20": {
--- a/services/web/app/src/Features/Authentication/AuthenticationController.js
+++ b/services/web/app/src/Features/Authentication/AuthenticationController.js
@ -80,30 +80,36 @@ const AuthenticationController = {
    // This function is middleware which wraps the passport.authenticate middleware,
    // so we can send back our custom `{message: {text: "", type: ""}}` responses on failure,
    // and send a `{redir: ""}` response on success
-    passport.authenticate('local', function (err, user, info) {
-      if (err) {
-        return next(err)
-      }
-      if (user) {
-        // `user` is either a user object or false
-        AuthenticationController.setAuditInfo(req, { method: 'Password login' })
-        return AuthenticationController.finishLogin(user, req, res, next)
-      } else {
-        if (info.redir != null) {
-          return res.json({ redir: info.redir })
+    passport.authenticate(
+      'local',
+      { keepSessionInfo: true },
+      function (err, user, info) {
+        if (err) {
+          return next(err)
+        }
+        if (user) {
+          // `user` is either a user object or false
+          AuthenticationController.setAuditInfo(req, {
+            method: 'Password login',
+          })
+          return AuthenticationController.finishLogin(user, req, res, next)
        } else {
-          res.status(info.status || 200)
-          delete info.status
-          const body = { message: info }
-          const { errorReason } = info
-          if (errorReason) {
-            body.errorReason = errorReason
-            delete info.errorReason
+          if (info.redir != null) {
+            return res.json({ redir: info.redir })
+          } else {
+            res.status(info.status || 200)
+            delete info.status
+            const body = { message: info }
+            const { errorReason } = info
+            if (errorReason) {
+              body.errorReason = errorReason
+              delete info.errorReason
+            }
+            return res.json(body)
          }
-          return res.json(body)
        }
      }
-    })(req, res, next)
+    )(req, res, next)
  },

  finishLogin(user, req, res, next) {
@ -557,55 +563,34 @@ const AuthenticationController = {
 }

 function _afterLoginSessionSetup(req, user, callback) {
-  if (callback == null) {
-    callback = function () {}
-  }
-  req.login(user, function (err) {
+  req.login(user, { keepSessionInfo: true }, function (err) {
    if (err) {
      OError.tag(err, 'error from req.login', {
        user_id: user._id,
      })
      return callback(err)
    }
-    // Regenerate the session to get a new sessionID (cookie value) to
-    // protect against session fixation attacks
-    const oldSession = req.session
-    req.session.destroy(function (err) {
+    delete req.session.__tmp
+    delete req.session.csrfSecret
+    req.session.save(function (err) {
      if (err) {
-        OError.tag(err, 'error when trying to destroy old session', {
+        OError.tag(err, 'error saving regenerated session after login', {
          user_id: user._id,
        })
        return callback(err)
      }
-      req.sessionStore.generate(req)
-      // Note: the validation token is not writable, so it does not get
-      // transferred to the new session below.
-      for (const key in oldSession) {
-        const value = oldSession[key]
-        if (key !== '__tmp' && key !== 'csrfSecret') {
-          req.session[key] = value
-        }
+      UserSessionsManager.trackSession(user, req.sessionID, function () {})
+      if (!req.deviceHistory) {
+        // Captcha disabled or SSO-based login.
+        return callback()
      }
-      req.session.save(function (err) {
-        if (err) {
-          OError.tag(err, 'error saving regenerated session after login', {
-            user_id: user._id,
-          })
-          return callback(err)
-        }
-        UserSessionsManager.trackSession(user, req.sessionID, function () {})
-        if (!req.deviceHistory) {
-          // Captcha disabled or SSO-based login.
-          return callback()
-        }
-        req.deviceHistory.add(user.email)
-        req.deviceHistory
-          .serialize(req.res)
-          .catch(err => {
-            logger.err({ err }, 'cannot serialize deviceHistory')
-          })
-          .finally(() => callback())
-      })
+      req.deviceHistory.add(user.email)
+      req.deviceHistory
+        .serialize(req.res)
+        .catch(err => {
+          logger.err({ err }, 'cannot serialize deviceHistory')
+        })
+        .finally(() => callback())
    })
  })
 }
--- a/services/web/app/src/Features/User/UserController.js
+++ b/services/web/app/src/Features/User/UserController.js
@ -266,7 +266,8 @@ async function tryDeleteUser(req, res, next) {
  const sessionId = req.sessionID

  if (typeof req.logout === 'function') {
-    req.logout()
+    const logout = promisify(req.logout)
+    await logout()
  }

  const destroySession = promisify(req.session.destroy.bind(req.session))
@ -431,9 +432,10 @@ async function doLogout(req) {
  logger.debug({ user }, 'logging out')
  const sessionId = req.sessionID

-  // passport logout
  if (typeof req.logout === 'function') {
-    req.logout()
+    // passport logout
+    const logout = promisify(req.logout.bind(req))
+    await logout()
  }

  const destroySession = promisify(req.session.destroy.bind(req.session))
--- a/services/web/package.json
+++ b/services/web/package.json
@ -137,7 +137,7 @@
    "otplib": "^12.0.1",
    "p-limit": "^2.3.0",
    "parse-data-url": "^2.0.0",
-    "passport": "^0.4.1",
+    "passport": "^0.6.0",
    "passport-google-oauth20": "^2.0.0",
    "passport-ldapauth": "^2.1.4",
    "passport-local": "^1.0.0",
--- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js
+++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js
@ -77,7 +77,7 @@ describe('AuthenticationController', function () {
        '../User/UserSessionsManager': (this.UserSessionsManager = {
          trackSession: sinon.stub(),
          untrackSession: sinon.stub(),
-          revokeAllUserSessions: sinon.stub().callsArgWith(1, null),
+          revokeAllUserSessions: sinon.stub().yields(null),
        }),
        '../../infrastructure/Modules': (this.Modules = {
          hooks: { fire: sinon.stub().yields(null, []) },
@ -184,17 +184,17 @@ describe('AuthenticationController', function () {
  describe('passportLogin', function () {
    beforeEach(function () {
      this.info = null
-      this.req.login = sinon.stub().callsArgWith(1, null)
+      this.req.login = sinon.stub().yields(null)
      this.res.json = sinon.stub()
      this.req.session = {
        passport: { user: this.user },
        postLoginRedirect: '/path/to/redir/to',
      }
-      this.req.session.destroy = sinon.stub().callsArgWith(0, null)
-      this.req.session.save = sinon.stub().callsArgWith(0, null)
+      this.req.session.destroy = sinon.stub().yields(null)
+      this.req.session.save = sinon.stub().yields(null)
      this.req.sessionStore = { generate: sinon.stub() }
      this.AuthenticationController.finishLogin = sinon.stub()
-      this.passport.authenticate.callsArgWith(1, null, this.user, this.info)
+      this.passport.authenticate.yields(null, this.user, this.info)
      this.err = new Error('woops')
    })

@ -205,7 +205,7 @@ describe('AuthenticationController', function () {

    describe('when authenticate produces an error', function () {
      beforeEach(function () {
-        this.passport.authenticate.callsArgWith(1, this.err)
+        this.passport.authenticate.yields(this.err)
      })

      it('should return next with an error', function () {
@ -221,7 +221,7 @@ describe('AuthenticationController', function () {
    describe('when authenticate produces a user', function () {
      beforeEach(function () {
        this.req.session.postLoginRedirect = 'some_redirect'
-        this.passport.authenticate.callsArgWith(1, null, this.user, this.info)
+        this.passport.authenticate.yields(null, this.user, this.info)
      })

      afterEach(function () {
@ -244,7 +244,7 @@ describe('AuthenticationController', function () {
    describe('when authenticate does not produce a user', function () {
      beforeEach(function () {
        this.info = { text: 'a', type: 'b' }
-        this.passport.authenticate.callsArgWith(1, null, false, this.info)
+        this.passport.authenticate.yields(null, false, this.info)
      })

      it('should not call finishLogin', function () {
@ -273,8 +273,7 @@ describe('AuthenticationController', function () {
    beforeEach(function () {
      this.AuthenticationController._recordFailedLogin = sinon.stub()
      this.AuthenticationController._recordSuccessfulLogin = sinon.stub()
-      this.Modules.hooks.fire = sinon.stub().callsArgWith(3, null, [])
-      // @AuthenticationController.establishUserSession = sinon.stub().callsArg(2)
+      this.Modules.hooks.fire = sinon.stub().yields(null, [])
      this.req.body = {
        email: this.email,
        password: this.password,
@ -290,7 +289,7 @@ describe('AuthenticationController', function () {
      beforeEach(function () {
        this.Modules.hooks.fire = sinon
          .stub()
-          .callsArgWith(3, null, [null, { redir: '/somewhere' }, null])
+          .yields(null, [null, { redir: '/somewhere' }, null])
      })

      it('should stop early and call done with this info object', function (done) {
@ -311,7 +310,7 @@ describe('AuthenticationController', function () {

    describe('when the users rate limit', function () {
      beforeEach(function () {
-        this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, false)
+        this.LoginRateLimiter.processLoginRequest.yields(null, false)
      })

      it('should block the request if the limit has been exceeded', function (done) {
@ -330,10 +329,10 @@ describe('AuthenticationController', function () {
    describe('when the user is authenticated', function () {
      beforeEach(function () {
        this.cb = sinon.stub()
-        this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true)
+        this.LoginRateLimiter.processLoginRequest.yields(null, true)
        this.AuthenticationManager.authenticate = sinon
          .stub()
-          .callsArgWith(3, null, this.user)
+          .yields(null, this.user)
        this.req.sessionID = Math.random()
      })

@ -361,7 +360,7 @@ describe('AuthenticationController', function () {
        beforeEach(function () {
          this.AuthenticationManager.authenticate = sinon
            .stub()
-            .callsArgWith(3, new AuthenticationErrors.ParallelLoginError())
+            .yields(new AuthenticationErrors.ParallelLoginError())
          this.AuthenticationController.doPassportLogin(
            this.req,
            this.req.body.email,
@ -437,10 +436,10 @@ describe('AuthenticationController', function () {

    describe('when the user is not authenticated', function () {
      beforeEach(function () {
-        this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true)
+        this.LoginRateLimiter.processLoginRequest.yields(null, true)
        this.AuthenticationManager.authenticate = sinon
          .stub()
-          .callsArgWith(3, null, null)
+          .yields(null, null)
        this.cb = sinon.stub()
        this.AuthenticationController.doPassportLogin(
          this.req,
@ -937,7 +936,7 @@ describe('AuthenticationController', function () {

  describe('_recordSuccessfulLogin', function () {
    beforeEach(function () {
-      this.UserUpdater.updateUser = sinon.stub().callsArg(2)
+      this.UserUpdater.updateUser = sinon.stub().yields()
      this.AuthenticationController._recordSuccessfulLogin(
        this.user._id,
        this.callback
@ -1078,8 +1077,8 @@ describe('AuthenticationController', function () {
      this.req.session = {
        passport: { user: { _id: 'one' } },
      }
-      this.req.session.destroy = sinon.stub().callsArgWith(0, null)
-      this.req.session.save = sinon.stub().callsArgWith(0, null)
+      this.req.session.destroy = sinon.stub().yields(null)
+      this.req.session.save = sinon.stub().yields(null)
      this.req.sessionStore = { generate: sinon.stub() }
      this.req.login = sinon.stub().yields(null)

@ -1345,9 +1344,7 @@ describe('AuthenticationController', function () {

      describe('when req.session.save produces an error', function () {
        beforeEach(function () {
-          this.req.session.save = sinon
-            .stub()
-            .callsArgWith(0, new Error('woops'))
+          this.req.session.save = sinon.stub().yields(new Error('woops'))
        })

        it('should produce an error', function (done) {
--- a/services/web/test/unit/src/User/UserControllerTests.js
+++ b/services/web/test/unit/src/User/UserControllerTests.js
@ -173,7 +173,7 @@ describe('UserController', function () {
  describe('tryDeleteUser', function () {
    beforeEach(function () {
      this.req.body.password = 'wat'
-      this.req.logout = sinon.stub()
+      this.req.logout = sinon.stub().yields()
      this.req.session.destroy = sinon.stub().yields()
      this.SessionManager.getLoggedInUserId = sinon
        .stub()