From 3300811d3a8494f040c00e8b782671d16448303d Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Tue, 21 May 2024 12:42:47 +0200 Subject: [PATCH] [web] Simplify `removeSessionsFromRedis` signature (#18440) * Simplify `removeSessionsFromRedis` signature * Update usage of `removeSessionsFromRedis` * Fix tests around `removeSessionsFromRedis` * Add comments "remove all sessions except the current session" GitOrigin-RevId: 03bf99c14faf2c8e403bc4bcc16463a70e031284 --- .../app/src/Features/User/UserController.js | 14 +++++----- .../src/Features/User/UserEmailsController.js | 3 +-- .../src/Features/User/UserPagesController.js | 3 +-- .../src/Features/User/UserSessionsManager.js | 26 +++++++++---------- .../clear_sessions_set_must_reconfirm.js | 1 - .../src/User/UserEmailsControllerTests.js | 2 +- .../unit/src/User/UserSessionsManagerTests.js | 11 +++----- 7 files changed, 27 insertions(+), 33 deletions(-) diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 10e7e5ec6c..227cef4e74 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -130,9 +130,10 @@ async function changePassword(req, res, next) { // no need to wait, errors are logged and not passed back _sendSecurityAlertPasswordChanged(user) - await UserSessionsManager.promises.removeSessionsFromRedis(user, req, { - stayLoggedIn: true, - }) + await UserSessionsManager.promises.removeSessionsFromRedis( + user, + req.sessionID // remove all sessions except the current session + ) await OneTimeTokenHandler.promises.expireAllTokensForUser( userId.toString(), @@ -162,9 +163,10 @@ async function clearSessions(req, res, next) { req.ip, { sessions } ) - await UserSessionsManager.promises.removeSessionsFromRedis(user, req, { - stayLoggedIn: true, - }) + await UserSessionsManager.promises.removeSessionsFromRedis( + user, + req.sessionID // remove all sessions except the current session + ) await _sendSecurityAlertClearedSessions(user) diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index 7ba273ad0b..4d3a5afe17 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/services/web/app/src/Features/User/UserEmailsController.js @@ -512,8 +512,7 @@ const UserEmailsController = { const user = SessionManager.getSessionUser(req.session) UserSessionsManager.removeSessionsFromRedis( user, - req, - { stayLoggedIn: true }, + req.sessionID, // remove all sessions except the current session err => { if (err) logger.warn( diff --git a/services/web/app/src/Features/User/UserPagesController.js b/services/web/app/src/Features/User/UserPagesController.js index 2ee8abb18a..3a7fc52cd8 100644 --- a/services/web/app/src/Features/User/UserPagesController.js +++ b/services/web/app/src/Features/User/UserPagesController.js @@ -66,8 +66,7 @@ async function settingsPage(req, res) { // The user has just deleted their account. return UserSessionsManager.removeSessionsFromRedis( { _id: userId }, - req, - { stayLoggedIn: false }, + null, () => res.redirect('/') ) } diff --git a/services/web/app/src/Features/User/UserSessionsManager.js b/services/web/app/src/Features/User/UserSessionsManager.js index 634834b44d..eb62d4b369 100644 --- a/services/web/app/src/Features/User/UserSessionsManager.js +++ b/services/web/app/src/Features/User/UserSessionsManager.js @@ -128,12 +128,10 @@ const UserSessionsManager = { /** * @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 {string | null | undefined} retainSessionID - the session ID to exclude from deletion * @param {(err: Error | null, data?: unknown) => void} callback */ - removeSessionsFromRedis(user, req, options, callback) { - const stayLoggedIn = options?.stayLoggedIn ?? false + removeSessionsFromRedis(user, retainSessionID, callback) { if (!user) { return callback(null) } @@ -146,13 +144,12 @@ const UserSessionsManager = { }) return callback(err) } - const keysToDelete = - stayLoggedIn && req?.sessionID - ? _.without( - sessionKeys, - UserSessionsManager._sessionKey(req.sessionID) - ) - : sessionKeys + const keysToDelete = retainSessionID + ? _.without( + sessionKeys, + UserSessionsManager._sessionKey(retainSessionID) + ) + : sessionKeys if (keysToDelete.length === 0) { logger.debug( { userId: user._id }, @@ -248,8 +245,11 @@ const UserSessionsManager = { UserSessionsManager.promises = { getAllUserSessions: promisify(UserSessionsManager.getAllUserSessions), - removeSessionsFromRedis: (user, req = null, options = null) => - promisify(UserSessionsManager.removeSessionsFromRedis)(user, req, options), + removeSessionsFromRedis: (user, retainSessionID = null) => + promisify(UserSessionsManager.removeSessionsFromRedis)( + user, + retainSessionID + ), untrackSession: promisify(UserSessionsManager.untrackSession), } diff --git a/services/web/scripts/clear_sessions_set_must_reconfirm.js b/services/web/scripts/clear_sessions_set_must_reconfirm.js index 1b99cc978a..a40ef9c807 100644 --- a/services/web/scripts/clear_sessions_set_must_reconfirm.js +++ b/services/web/scripts/clear_sessions_set_must_reconfirm.js @@ -40,7 +40,6 @@ function _handleUser(userId, callback) { UserSessionsManager.removeSessionsFromRedis( { _id: userId }, null, - null, error => { if (error) { console.log(`Failed to clear sessions for ${userId}`, error) diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index 08cb790e35..5dcaa5aad2 100644 --- a/services/web/test/unit/src/User/UserEmailsControllerTests.js +++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js @@ -566,7 +566,7 @@ describe('UserEmailsController', function () { this.res.callback = () => { expect( this.UserSessionsManager.removeSessionsFromRedis - ).to.have.been.calledWith(this.user, this.req) + ).to.have.been.calledWith(this.user, this.req.sessionID) done() } diff --git a/services/web/test/unit/src/User/UserSessionsManagerTests.js b/services/web/test/unit/src/User/UserSessionsManagerTests.js index 3d3f8bbec4..f300676bf4 100644 --- a/services/web/test/unit/src/User/UserSessionsManagerTests.js +++ b/services/web/test/unit/src/User/UserSessionsManagerTests.js @@ -344,15 +344,13 @@ describe('UserSessionsManager', function () { beforeEach(function () { this.sessionKeys = ['sess:one', 'sess:two'] 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.removeSessionsFromRedis( this.user, - { sessionID: this.currentSessionID }, - { stayLoggedIn: this.stayLoggedIn }, + this.currentSessionID, callback ) }) @@ -390,14 +388,12 @@ describe('UserSessionsManager', function () { beforeEach(function () { this.sessionKeys = ['sess:one', 'sess:two', 'sess:three', 'sess:four'] 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.removeSessionsFromRedis( this.user, - { sessionID: this.currentSessionID }, - { stayLoggedIn: this.stayLoggedIn }, + this.currentSessionID, callback ) }) @@ -463,8 +459,7 @@ describe('UserSessionsManager', function () { return (this.call = callback => { return this.UserSessionsManager.removeSessionsFromRedis( null, - { sessionID: this.currentSessionID }, - { stayLoggedIn: this.stayLoggedIn }, + this.currentSessionID, callback ) })