[web] Update revokeAllUserSessions and rename it to removeSessionsFromRedis (#18360)

* Fix `revokeAllUserSessions` call in `_cleanupUser` The user object should be passed, not the _id * Change `revokeAllUserSessions` signature, take `req` and `stayLoggedIn` arguments * Update uses of `revokeAllUserSessions` * Fix promisified `revokeAllUserSessions` args * Update tests * Destroy or Regenerate the session in the end of `revokeAllUserSessions` Per https://github.com/overleaf/internal/issues/17036#issuecomment-1938398570 * Revert "Destroy or Regenerate the session in the end of `revokeAllUserSessions`" This reverts commit fe30734dbe45b27d2931d2e43a711d591bb85787. * Rename `revokeAllUserSessions` to `removeSessionsFromRedis` * Fixup tests * Fix: add optional chaining in `req.sessionID` (!!) GitOrigin-RevId: d41676bf00f463230af495e09c65fb9ee521f49f
2024-11-21 20:47:08 -05:00 · 2024-05-17 12:15:14 +02:00 · 2024-05-17 12:15:14 +02:00 · 25d8e053be
commit 25d8e053be
parent e8fe5abc82
15 changed files with 79 additions and 61 deletions
--- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js
+++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js
@ -57,10 +57,7 @@ async function setNewUserPassword(req, res, next) {
        message: req.i18n.translate('error_performing_request'),
      })
    }
-    await UserSessionsManager.promises.revokeAllUserSessions(
-      { _id: userId },
-      []
-    )
+    await UserSessionsManager.promises.removeSessionsFromRedis({ _id: userId })
    await UserUpdater.promises.removeReconfirmFlag(userId)
    if (!req.session.doLoginAfterPasswordReset) {
      return res.sendStatus(200)
--- a/services/web/app/src/Features/User/UserController.js
+++ b/services/web/app/src/Features/User/UserController.js
@ -130,9 +130,9 @@ async function changePassword(req, res, next) {
  // no need to wait, errors are logged and not passed back
  _sendSecurityAlertPasswordChanged(user)

-  await UserSessionsManager.promises.revokeAllUserSessions(user, [
-    req.sessionID,
-  ])
+  await UserSessionsManager.promises.removeSessionsFromRedis(user, req, {
+    stayLoggedIn: true,
+  })

  await OneTimeTokenHandler.promises.expireAllTokensForUser(
    userId.toString(),
@ -162,9 +162,9 @@ async function clearSessions(req, res, next) {
    req.ip,
    { sessions }
  )
-  await UserSessionsManager.promises.revokeAllUserSessions(user, [
-    req.sessionID,
-  ])
+  await UserSessionsManager.promises.removeSessionsFromRedis(user, req, {
+    stayLoggedIn: true,
+  })

  await _sendSecurityAlertClearedSessions(user)

--- a/services/web/app/src/Features/User/UserDeleter.js
+++ b/services/web/app/src/Features/User/UserDeleter.js
@ -152,7 +152,7 @@ async function _createDeletedUser(user, options) {
 }

 async function _cleanupUser(user) {
-  await UserSessionsManager.promises.revokeAllUserSessions(user._id, [])
+  await UserSessionsManager.promises.removeSessionsFromRedis(user)
  await NewsletterManager.promises.unsubscribe(user, { delete: true })
  await SubscriptionHandler.promises.cancelSubscription(user)
  await InstitutionsAPI.promises.deleteAffiliations(user._id)
--- a/services/web/app/src/Features/User/UserEmailsController.js
+++ b/services/web/app/src/Features/User/UserEmailsController.js
@ -510,9 +510,10 @@ const UserEmailsController = {
        }
        SessionManager.setInSessionUser(req.session, { email })
        const user = SessionManager.getSessionUser(req.session)
-        UserSessionsManager.revokeAllUserSessions(
+        UserSessionsManager.removeSessionsFromRedis(
          user,
-          [req.sessionID],
+          req,
+          { stayLoggedIn: true },
          err => {
            if (err)
              logger.warn(
--- a/services/web/app/src/Features/User/UserPagesController.js
+++ b/services/web/app/src/Features/User/UserPagesController.js
@ -64,8 +64,11 @@ async function settingsPage(req, res) {
  const user = await UserGetter.promises.getUser(userId)
  if (!user) {
    // The user has just deleted their account.
-    return UserSessionsManager.revokeAllUserSessions({ _id: userId }, [], () =>
-      res.redirect('/')
+    return UserSessionsManager.removeSessionsFromRedis(
+      { _id: userId },
+      req,
+      { stayLoggedIn: false },
+      () => res.redirect('/')
    )
  }

--- a/services/web/app/src/Features/User/UserSessionsManager.js
+++ b/services/web/app/src/Features/User/UserSessionsManager.js
@ -126,11 +126,14 @@ const UserSessionsManager = {
    })
  },

-  revokeAllUserSessions(user, retain, callback) {
-    if (!retain) {
-      retain = []
-    }
-    retain = retain.map(i => UserSessionsManager._sessionKey(i))
+  /**
+   * @param {{_id: string}} user
+   * @param {(import('express').Request & {sessionID?: string}) | undefined} req - the request object. Can be omitted if stayLoggedIn is false.
+   * @param {{stayLoggedIn: boolean}?} options
+   * @param {(err: Error | null, data?: unknown) => void} callback
+   */
+  removeSessionsFromRedis(user, req, options, callback) {
+    const stayLoggedIn = options?.stayLoggedIn ?? false
    if (!user) {
      return callback(null)
    }
@ -143,10 +146,13 @@ const UserSessionsManager = {
        })
        return callback(err)
      }
-      const keysToDelete = _.filter(
-        sessionKeys,
-        k => !Array.from(retain).includes(k)
-      )
+      const keysToDelete =
+        stayLoggedIn && req?.sessionID
+          ? _.without(
+              sessionKeys,
+              UserSessionsManager._sessionKey(req.sessionID)
+            )
+          : sessionKeys
      if (keysToDelete.length === 0) {
        logger.debug(
          { userId: user._id },
@ -242,7 +248,8 @@ const UserSessionsManager = {

 UserSessionsManager.promises = {
  getAllUserSessions: promisify(UserSessionsManager.getAllUserSessions),
-  revokeAllUserSessions: promisify(UserSessionsManager.revokeAllUserSessions),
+  removeSessionsFromRedis: (user, req = null, options = null) =>
+    promisify(UserSessionsManager.removeSessionsFromRedis)(user, req, options),
  untrackSession: promisify(UserSessionsManager.untrackSession),
 }

--- a/services/web/modules/server-ce-scripts/scripts/migrate-user-emails.js
+++ b/services/web/modules/server-ce-scripts/scripts/migrate-user-emails.js
@ -142,9 +142,9 @@ async function doMigration(emails) {
        `Updating user ${userWithEmail._id} email "${oldEmail}" to "${newEmail}"\n`
      )
      try {
-        await UserSessionsManager.promises.revokeAllUserSessions(
-          userWithEmail,
-          [] // log out all the user's sessions before changing the email address
+        // log out all the user's sessions before changing the email address
+        await UserSessionsManager.promises.removeSessionsFromRedis(
+          userWithEmail
        )

        await UserUpdater.promises.migrateDefaultEmailAddress(
--- a/services/web/scripts/back_fill_staff_access.js
+++ b/services/web/scripts/back_fill_staff_access.js
@ -77,7 +77,7 @@ async function main() {
        { $set: { staffAccess: FULL_STAFF_ACCESS } }
      )
      if (!KEEP_SESSIONS) {
-        await UserSessionsManager.promises.revokeAllUserSessions(user, [])
+        await UserSessionsManager.promises.removeSessionsFromRedis(user)
      }
    }
  } else {
--- a/services/web/scripts/clear_sessions_set_must_reconfirm.js
+++ b/services/web/scripts/clear_sessions_set_must_reconfirm.js
@ -37,15 +37,20 @@ function _handleUser(userId, callback) {
      processLogger.failedSet.push(userId)
      return callback()
    } else {
-      UserSessionsManager.revokeAllUserSessions({ _id: userId }, [], error => {
-        if (error) {
-          console.log(`Failed to clear sessions for ${userId}`, error)
-          processLogger.failedClear.push(userId)
-        } else {
-          processLogger.success.push(userId)
+      UserSessionsManager.removeSessionsFromRedis(
+        { _id: userId },
+        null,
+        null,
+        error => {
+          if (error) {
+            console.log(`Failed to clear sessions for ${userId}`, error)
+            processLogger.failedClear.push(userId)
+          } else {
+            processLogger.success.push(userId)
+          }
+          return callback()
        }
-        return callback()
-      })
+      )
    }
  })
 }
--- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js
+++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js
@ -103,7 +103,7 @@ describe('AuthenticationController', function () {
        '../User/UserSessionsManager': (this.UserSessionsManager = {
          trackSession: sinon.stub(),
          untrackSession: sinon.stub(),
-          revokeAllUserSessions: sinon.stub().yields(null),
+          removeSessionsFromRedis: sinon.stub().yields(null),
        }),
        '../../infrastructure/Modules': (this.Modules = {
          hooks: { fire: sinon.stub().yields(null, []) },
--- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js
+++ b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js
@ -50,7 +50,7 @@ describe('PasswordResetController', function () {
    }
    this.UserSessionsManager = {
      promises: {
-        revokeAllUserSessions: sinon.stub().resolves(),
+        removeSessionsFromRedis: sinon.stub().resolves(),
      },
    }
    this.UserUpdater = {
@ -264,7 +264,7 @@ describe('PasswordResetController', function () {

    it('should clear sessions', function (done) {
      this.res.sendStatus = code => {
-        this.UserSessionsManager.promises.revokeAllUserSessions.callCount.should.equal(
+        this.UserSessionsManager.promises.removeSessionsFromRedis.callCount.should.equal(
          1
        )
        done()
--- a/services/web/test/unit/src/User/UserControllerTests.js
+++ b/services/web/test/unit/src/User/UserControllerTests.js
@ -91,7 +91,7 @@ describe('UserController', function () {
    this.UserSessionsManager = {
      promises: {
        getAllUserSessions: sinon.stub().resolves(),
-        revokeAllUserSessions: sinon.stub().resolves(),
+        removeSessionsFromRedis: sinon.stub().resolves(),
        untrackSession: sinon.stub().resolves(),
      },
    }
@ -603,9 +603,9 @@ describe('UserController', function () {

  describe('clearSessions', function () {
    describe('success', function () {
-      it('should call revokeAllUserSessions', function (done) {
+      it('should call removeSessionsFromRedis', function (done) {
        this.res.sendStatus.callsFake(() => {
-          this.UserSessionsManager.promises.revokeAllUserSessions.should.have
+          this.UserSessionsManager.promises.removeSessionsFromRedis.should.have
            .been.calledOnce
          done()
        })
@ -662,9 +662,9 @@ describe('UserController', function () {
        })
      })

-      describe('when revokeAllUserSessions produces an error', function () {
+      describe('when removeSessionsFromRedis produces an error', function () {
        it('should call next with an error', function (done) {
-          this.UserSessionsManager.promises.revokeAllUserSessions.rejects(
+          this.UserSessionsManager.promises.removeSessionsFromRedis.rejects(
            new Error('woops')
          )
          this.UserController.clearSessions(this.req, this.res, error => {
--- a/services/web/test/unit/src/User/UserDeleterTests.js
+++ b/services/web/test/unit/src/User/UserDeleterTests.js
@ -75,7 +75,7 @@ describe('UserDeleter', function () {

    this.UserSessionsManager = {
      promises: {
-        revokeAllUserSessions: sinon.stub().resolves(),
+        removeSessionsFromRedis: sinon.stub().resolves(),
      },
    }

@ -250,8 +250,8 @@ describe('UserDeleter', function () {
              ipAddress: this.ipAddress,
            })
            expect(
-              this.UserSessionsManager.promises.revokeAllUserSessions
-            ).to.have.been.calledWith(this.userId, [])
+              this.UserSessionsManager.promises.removeSessionsFromRedis
+            ).to.have.been.calledWith(this.user)
          })

          it('should remove user from group subscriptions', async function () {
--- a/services/web/test/unit/src/User/UserEmailsControllerTests.js
+++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js
@ -38,7 +38,7 @@ describe('UserEmailsController', function () {
      hasFeature: sinon.stub(),
    }
    this.UserSessionsManager = {
-      revokeAllUserSessions: sinon.stub().yields(),
+      removeSessionsFromRedis: sinon.stub().yields(),
    }
    this.UserUpdater = {
      addEmailAddress: sinon.stub(),
@ -565,8 +565,8 @@ describe('UserEmailsController', function () {

      this.res.callback = () => {
        expect(
-          this.UserSessionsManager.revokeAllUserSessions
-        ).to.have.been.calledWith(this.user, [this.req.sessionID])
+          this.UserSessionsManager.removeSessionsFromRedis
+        ).to.have.been.calledWith(this.user, this.req)
        done()
      }

@ -576,7 +576,7 @@ describe('UserEmailsController', function () {
    it('handles error from revoking sessions and returns 200', function (done) {
      this.UserUpdater.setDefaultEmailAddress.yields()
      const redisError = new Error('redis error')
-      this.UserSessionsManager.revokeAllUserSessions = sinon
+      this.UserSessionsManager.removeSessionsFromRedis = sinon
        .stub()
        .yields(redisError)

--- a/services/web/test/unit/src/User/UserSessionsManagerTests.js
+++ b/services/web/test/unit/src/User/UserSessionsManagerTests.js
@ -340,17 +340,19 @@ describe('UserSessionsManager', function () {
    })
  })

-  describe('revokeAllUserSessions', function () {
+  describe('removeSessionsFromRedis', function () {
    beforeEach(function () {
      this.sessionKeys = ['sess:one', 'sess:two']
-      this.retain = []
+      this.currentSessionID = undefined
+      this.stayLoggedIn = false
      this.rclient.smembers.callsArgWith(1, null, this.sessionKeys)
      this.rclient.del = sinon.stub().callsArgWith(1, null)
      this.rclient.srem = sinon.stub().callsArgWith(2, null)
      return (this.call = callback => {
-        return this.UserSessionsManager.revokeAllUserSessions(
+        return this.UserSessionsManager.removeSessionsFromRedis(
          this.user,
-          this.retain,
+          { sessionID: this.currentSessionID },
+          { stayLoggedIn: this.stayLoggedIn },
          callback
        )
      })
@ -387,13 +389,15 @@ describe('UserSessionsManager', function () {
    describe('when a session is retained', function () {
      beforeEach(function () {
        this.sessionKeys = ['sess:one', 'sess:two', 'sess:three', 'sess:four']
-        this.retain = ['two']
+        this.currentSessionID = 'two'
+        this.stayLoggedIn = true
        this.rclient.smembers.callsArgWith(1, null, this.sessionKeys)
        this.rclient.del = sinon.stub().callsArgWith(1, null)
        return (this.call = callback => {
-          return this.UserSessionsManager.revokeAllUserSessions(
+          return this.UserSessionsManager.removeSessionsFromRedis(
            this.user,
-            this.retain,
+            { sessionID: this.currentSessionID },
+            { stayLoggedIn: this.stayLoggedIn },
            callback
          )
        })
@ -457,9 +461,10 @@ describe('UserSessionsManager', function () {
    describe('when no user is supplied', function () {
      beforeEach(function () {
        return (this.call = callback => {
-          return this.UserSessionsManager.revokeAllUserSessions(
+          return this.UserSessionsManager.removeSessionsFromRedis(
            null,
-            this.retain,
+            { sessionID: this.currentSessionID },
+            { stayLoggedIn: this.stayLoggedIn },
            callback
          )
        })