diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index 87c10d258e..8dd8d92c46 100644 --- 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) diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 6d018da715..10e7e5ec6c 100644 --- 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) diff --git a/services/web/app/src/Features/User/UserDeleter.js b/services/web/app/src/Features/User/UserDeleter.js index 6f739bfba0..d5437243a9 100644 --- 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) diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index cfb692798d..7ba273ad0b 100644 --- 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( diff --git a/services/web/app/src/Features/User/UserPagesController.js b/services/web/app/src/Features/User/UserPagesController.js index 54a3789f55..2ee8abb18a 100644 --- 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('/') ) } diff --git a/services/web/app/src/Features/User/UserSessionsManager.js b/services/web/app/src/Features/User/UserSessionsManager.js index adfcbe3101..634834b44d 100644 --- 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), } diff --git a/services/web/modules/server-ce-scripts/scripts/migrate-user-emails.js b/services/web/modules/server-ce-scripts/scripts/migrate-user-emails.js index eb7fb052c4..d266ad46d2 100644 --- 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( diff --git a/services/web/scripts/back_fill_staff_access.js b/services/web/scripts/back_fill_staff_access.js index 573fc81b81..82437ee8d2 100644 --- 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 { diff --git a/services/web/scripts/clear_sessions_set_must_reconfirm.js b/services/web/scripts/clear_sessions_set_must_reconfirm.js index ca134957e5..1b99cc978a 100644 --- 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() - }) + ) } }) } diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index b709f62a6a..2dbf94a629 100644 --- 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, []) }, diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js index b794e58d87..76da4ae83b 100644 --- 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() diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 5c0d9f5e3d..2089ec63da 100644 --- 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 => { diff --git a/services/web/test/unit/src/User/UserDeleterTests.js b/services/web/test/unit/src/User/UserDeleterTests.js index dba3b71317..f474e53d30 100644 --- 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 () { diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index 14fd99b841..08cb790e35 100644 --- 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) diff --git a/services/web/test/unit/src/User/UserSessionsManagerTests.js b/services/web/test/unit/src/User/UserSessionsManagerTests.js index fa42300c56..3d3f8bbec4 100644 --- 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 ) })