Merge pull request #2441 from overleaf/em-cg-null-users

Remove existing sessions of deleted users

GitOrigin-RevId: cde9f8421fd9745b0922849a2269b44508d670f1

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)
             })

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)

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

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,

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(