From 7c9e83de2a599698498e9d0f7400749a052e6b14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Tue, 10 Dec 2019 13:40:05 +0530 Subject: [PATCH] Merge pull request #2441 from overleaf/em-cg-null-users Remove existing sessions of deleted users GitOrigin-RevId: cde9f8421fd9745b0922849a2269b44508d670f1 --- .../app/src/Features/Project/ProjectController.js | 12 ++++++++++++ services/web/app/src/Features/User/UserDeleter.js | 5 ++--- .../app/src/Features/User/UserSessionsManager.js | 9 ++++++++- .../unit/src/Project/ProjectControllerTests.js | 4 ++++ .../web/test/unit/src/User/UserDeleterTests.js | 14 ++++++++++++++ 5 files changed, 40 insertions(+), 4 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index 58eff4f834..893d99cb8c 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -35,6 +35,7 @@ const Features = require('../../infrastructure/Features') const BrandVariationsHandler = require('../BrandVariations/BrandVariationsHandler') const { getUserAffiliations } = require('../Institutions/InstitutionsAPI') const V1Handler = require('../V1/V1Handler') +const UserController = require('../User/UserController') const SystemMessageManager = require('../SystemMessages/SystemMessageManager') const ProjectController = { @@ -421,6 +422,11 @@ const ProjectController = { results.v1Projects.noConnection = true } const { notifications, user, userAffiliations } = results + // Handle case of deleted user + if (user == null) { + UserController.logout(req, res, next) + return + } const v1Tags = (results.v1Projects != null ? results.v1Projects.tags : undefined) || [] @@ -667,6 +673,12 @@ const ProjectController = { cb(null, defaultSettingsForAnonymousUser(userId)) } else { User.findById(userId, (err, user) => { + // Handle case of deleted user + if (user == null) { + UserController.logout(req, res, next) + return + } + logger.log({ projectId, userId }, 'got user') cb(err, user) }) diff --git a/services/web/app/src/Features/User/UserDeleter.js b/services/web/app/src/Features/User/UserDeleter.js index 09134ecc7e..814eb9c581 100644 --- a/services/web/app/src/Features/User/UserDeleter.js +++ b/services/web/app/src/Features/User/UserDeleter.js @@ -9,6 +9,7 @@ const SubscriptionHandler = require('../Subscription/SubscriptionHandler') const SubscriptionUpdater = require('../Subscription/SubscriptionUpdater') const SubscriptionLocator = require('../Subscription/SubscriptionLocator') const UserMembershipsHandler = require('../UserMembership/UserMembershipsHandler') +const UserSessionsManager = require('./UserSessionsManager') const InstitutionsAPI = require('../Institutions/InstitutionsAPI') const Errors = require('../Errors/Errors') @@ -106,9 +107,7 @@ async function _createDeletedUser(user, options) { } async function _cleanupUser(user) { - if (user == null) { - throw new Error('no user supplied') - } + await UserSessionsManager.promises.revokeAllUserSessions(user._id, []) 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/UserSessionsManager.js b/services/web/app/src/Features/User/UserSessionsManager.js index c6fa6b2d89..efed2420c1 100644 --- a/services/web/app/src/Features/User/UserSessionsManager.js +++ b/services/web/app/src/Features/User/UserSessionsManager.js @@ -3,10 +3,11 @@ const Settings = require('settings-sharelatex') const logger = require('logger-sharelatex') const Async = require('async') const _ = require('underscore') +const { promisify } = require('util') const UserSessionsRedis = require('./UserSessionsRedis') const rclient = UserSessionsRedis.client() -module.exports = UserSessionsManager = { +UserSessionsManager = { // mimic the key used by the express sessions _sessionKey(sessionId) { return `sess:${sessionId}` @@ -231,3 +232,9 @@ module.exports = UserSessionsManager = { }) } } + +UserSessionsManager.promises = { + revokeAllUserSessions: promisify(UserSessionsManager.revokeAllUserSessions) +} + +module.exports = UserSessionsManager diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index b3e64ef7dd..be3f9ba370 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -84,6 +84,9 @@ describe('ProjectController', function() { getSessionUser: sinon.stub().returns(this.user), isUserLoggedIn: sinon.stub().returns(true) } + this.UserController = { + logout: sinon.stub() + } this.AnalyticsManager = { getLastOccurrence: sinon.stub() } this.TokenAccessHandler = { getRequestToken: sinon.stub().returns(this.token), @@ -157,6 +160,7 @@ describe('ProjectController', function() { './ProjectDuplicator': this.ProjectDuplicator, './ProjectCreationHandler': this.ProjectCreationHandler, '../Editor/EditorController': this.EditorController, + '../User/UserController': this.UserController, './ProjectHelper': this.ProjectHelper, '../Subscription/SubscriptionLocator': this.SubscriptionLocator, '../Subscription/LimitationsManager': this.LimitationsManager, diff --git a/services/web/test/unit/src/User/UserDeleterTests.js b/services/web/test/unit/src/User/UserDeleterTests.js index 76456abb3d..08fefe8c1d 100644 --- a/services/web/test/unit/src/User/UserDeleterTests.js +++ b/services/web/test/unit/src/User/UserDeleterTests.js @@ -74,6 +74,12 @@ describe('UserDeleter', function() { } } + this.UserSessionsManager = { + promises: { + revokeAllUserSessions: sinon.stub().resolves() + } + } + this.InstitutionsApi = { promises: { deleteAffiliations: sinon.stub().resolves() @@ -85,6 +91,7 @@ describe('UserDeleter', function() { '../../models/User': { User: User }, '../../models/DeletedUser': { DeletedUser: DeletedUser }, '../Newsletter/NewsletterManager': this.NewsletterManager, + './UserSessionsManager': this.UserSessionsManager, '../Subscription/SubscriptionHandler': this.SubscriptionHandler, '../Subscription/SubscriptionUpdater': this.SubscriptionUpdater, '../Subscription/SubscriptionLocator': this.SubscriptionLocator, @@ -187,6 +194,13 @@ describe('UserDeleter', function() { ).to.have.been.calledWith(this.userId) }) + it('should stop the user sessions', async function() { + await this.UserDeleter.promises.deleteUser(this.userId) + expect( + this.UserSessionsManager.promises.revokeAllUserSessions + ).to.have.been.calledWith(this.userId, []) + }) + it('should remove user from group subscriptions', async function() { await this.UserDeleter.promises.deleteUser(this.userId) expect(