Merge pull request #3024 from overleaf/jel-log-cleared-sessions

Log cleared user sessions in user.auditLog

GitOrigin-RevId: 829c4ea037d897689c02c1dfe12d38542a7216c0

services/web/app/src/Features/User/UserController.js

@@ -9,6 +9,7 @@ const metrics = require('metrics-sharelatex')
 const AuthenticationManager = require('../Authentication/AuthenticationManager')
 const AuthenticationController = require('../Authentication/AuthenticationController')
 const Features = require('../../infrastructure/Features')
+const UserAuditLogHandler = require('./UserAuditLogHandler')
 const UserSessionsManager = require('./UserSessionsManager')
 const UserUpdater = require('./UserUpdater')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
@@ -18,6 +19,7 @@ const OError = require('@overleaf/o-error')
 const EmailHandler = require('../Email/EmailHandler')
 const UrlHelper = require('../Helpers/UrlHelper')
 const { promisify } = require('util')
+const { expressify } = require('../../util/promises')
 
 async function _ensureAffiliation(userId, emailData) {
   if (emailData.samlProviderId) {
@@ -27,6 +29,25 @@ async function _ensureAffiliation(userId, emailData) {
   }
 }
 
+async function clearSessions(req, res, next) {
+  metrics.inc('user.clear-sessions')
+  const user = AuthenticationController.getSessionUser(req)
+  const sessions = await UserSessionsManager.promises.getAllUserSessions(user, [
+    req.sessionID
+  ])
+  await UserAuditLogHandler.promises.addEntry(
+    user._id,
+    'clear-sessions',
+    user._id,
+    req.ip,
+    { sessions }
+  )
+  await UserSessionsManager.promises.revokeAllUserSessions(user, [
+    req.sessionID
+  ])
+  res.sendStatus(201)
+}
+
 async function ensureAffiliation(user) {
   if (!Features.hasFeature('affiliations')) {
     return
@@ -67,6 +88,8 @@ async function ensureAffiliationMiddleware(req, res, next) {
 }
 
 const UserController = {
+  clearSessions: expressify(clearSessions),
+
   tryDeleteUser(req, res, next) {
     const userId = AuthenticationController.getLoggedInUserId(req)
     const { password } = req.body
@@ -354,17 +377,6 @@ const UserController = {
     )
   },
 
-  clearSessions(req, res, next) {
-    metrics.inc('user.clear-sessions')
-    const user = AuthenticationController.getSessionUser(req)
-    UserSessionsManager.revokeAllUserSessions(user, [req.sessionID], err => {
-      if (err != null) {
-        return next(err)
-      }
-      res.sendStatus(201)
-    })
-  },
-
   changePassword(req, res, next) {
     metrics.inc('user.password-change')
     const internalError = {

services/web/app/src/Features/User/UserSessionsManager.js

@@ -234,6 +234,7 @@ UserSessionsManager = {
 }
 
 UserSessionsManager.promises = {
+  getAllUserSessions: promisify(UserSessionsManager.getAllUserSessions),
   revokeAllUserSessions: promisify(UserSessionsManager.revokeAllUserSessions)
 }
 

services/web/test/acceptance/src/SessionTests.js

@@ -1,30 +1,14 @@
-/* eslint-disable
-    handle-callback-err,
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const { expect } = require('chai')
 const async = require('async')
 const User = require('./helpers/User')
-const request = require('./helpers/request')
-const settings = require('settings-sharelatex')
 const redis = require('./helpers/redis')
-const MockV1Api = require('./helpers/MockV1Api')
 
 describe('Sessions', function() {
   beforeEach(function(done) {
     this.timeout(20000)
     this.user1 = new User()
     this.site_admin = new User({ email: 'admin@example.com' })
-    return async.series(
+    async.series(
       [cb => this.user1.login(cb), cb => this.user1.logout(cb)],
       done
     )
@@ -32,43 +16,45 @@ describe('Sessions', function() {
 
   describe('one session', function() {
     it('should have one session in UserSessions set', function(done) {
-      return async.series(
+      async.series(
         [
           next => {
-            return redis.clearUserSessions(this.user1, next)
+            redis.clearUserSessions(this.user1, next)
           },
 
           // login, should add session to set
           next => {
-            return this.user1.login(err => next(err))
+            this.user1.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(1)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // should be able to access project list page
           next => {
-            return this.user1.getProjectListPage((err, statusCode) => {
+            this.user1.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(200)
-              return next()
+              next()
             })
           },
 
           // logout, should remove session from set
           next => {
-            return this.user1.logout(err => next(err))
+            this.user1.logout(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(0)
-              return next()
+              next()
             })
           }
         ],
@@ -76,7 +62,7 @@ describe('Sessions', function() {
           if (err) {
             throw err
           }
-          return done()
+          done()
         }
       )
     })
@@ -87,108 +73,112 @@ describe('Sessions', function() {
       // set up second session for this user
       this.user2 = new User()
       this.user2.email = this.user1.email
-      return (this.user2.password = this.user1.password)
+      this.user2.password = this.user1.password
     })
 
     it('should have two sessions in UserSessions set', function(done) {
-      return async.series(
+      async.series(
         [
           next => {
-            return redis.clearUserSessions(this.user1, next)
+            redis.clearUserSessions(this.user1, next)
           },
 
           // login, should add session to set
           next => {
-            return this.user1.login(err => next(err))
+            this.user1.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(1)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // login again, should add the second session to set
           next => {
-            return this.user2.login(err => next(err))
+            this.user2.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(2)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
               expect(sessions[1].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // both should be able to access project list page
           next => {
-            return this.user1.getProjectListPage((err, statusCode) => {
+            this.user1.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(200)
-              return next()
+              next()
             })
           },
 
           next => {
-            return this.user2.getProjectListPage((err, statusCode) => {
+            this.user2.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(200)
-              return next()
+              next()
             })
           },
 
           // logout first session, should remove session from set
           next => {
-            return this.user1.logout(err => next(err))
+            this.user1.logout(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(1)
-              return next()
+              next()
             })
           },
 
           // first session should not have access to project list page
           next => {
-            return this.user1.getProjectListPage((err, statusCode) => {
+            this.user1.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(302)
-              return next()
+              next()
             })
           },
 
           // second session should still have access to settings
           next => {
-            return this.user2.getProjectListPage((err, statusCode) => {
+            this.user2.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(200)
-              return next()
+              next()
             })
           },
 
           // logout second session, should remove last session from set
           next => {
-            return this.user2.logout(err => next(err))
+            this.user2.logout(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(0)
-              return next()
+              next()
             })
           },
 
           // second session should not have access to project list page
           next => {
-            return this.user2.getProjectListPage((err, statusCode) => {
+            this.user2.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(302)
-              return next()
+              next()
             })
           }
         ],
@@ -196,7 +186,7 @@ describe('Sessions', function() {
           if (err) {
             throw err
           }
-          return done()
+          done()
         }
       )
     })
@@ -210,104 +200,109 @@ describe('Sessions', function() {
       this.user2.password = this.user1.password
       this.user3 = new User()
       this.user3.email = this.user1.email
-      return (this.user3.password = this.user1.password)
+      this.user3.password = this.user1.password
     })
 
     it('should erase both sessions when password is reset', function(done) {
-      return async.series(
+      async.series(
         [
           next => {
-            return redis.clearUserSessions(this.user1, next)
+            redis.clearUserSessions(this.user1, next)
           },
 
           // login, should add session to set
           next => {
-            return this.user1.login(err => next(err))
+            this.user1.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(1)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // login again, should add the second session to set
           next => {
-            return this.user2.login(err => next(err))
+            this.user2.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(2)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
               expect(sessions[1].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // login third session, should add the second session to set
           next => {
-            return this.user3.login(err => next(err))
+            this.user3.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(3)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
               expect(sessions[1].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // password reset from second session, should erase two of the three sessions
           next => {
-            return this.user2.changePassword(err => next(err))
+            this.user2.changePassword(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user2, (err, sessions) => {
+            redis.getUserSessions(this.user2, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(1)
-              return next()
+              next()
             })
           },
 
           // users one and three should not be able to access project list page
           next => {
-            return this.user1.getProjectListPage((err, statusCode) => {
+            this.user1.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(302)
-              return next()
+              next()
             })
           },
 
           next => {
-            return this.user3.getProjectListPage((err, statusCode) => {
+            this.user3.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(302)
-              return next()
+              next()
             })
           },
 
           // user two should still be logged in, and able to access project list page
           next => {
-            return this.user2.getProjectListPage((err, statusCode) => {
+            this.user2.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(200)
-              return next()
+              next()
             })
           },
 
           // logout second session, should remove last session from set
           next => {
-            return this.user2.logout(err => next(err))
+            this.user2.logout(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(0)
-              return next()
+              next()
             })
           }
         ],
@@ -315,7 +310,7 @@ describe('Sessions', function() {
           if (err) {
             throw err
           }
-          return done()
+          done()
         }
       )
     })
@@ -330,7 +325,7 @@ describe('Sessions', function() {
       this.user3 = new User()
       this.user3.email = this.user1.email
       this.user3.password = this.user1.password
-      return async.series(
+      async.series(
         [
           this.user2.login.bind(this.user2),
           this.user2.activateSudoMode.bind(this.user2)
@@ -340,58 +335,61 @@ describe('Sessions', function() {
     })
 
     it('should allow the user to erase the other two sessions', function(done) {
-      return async.series(
+      async.series(
         [
           next => {
-            return redis.clearUserSessions(this.user1, next)
+            redis.clearUserSessions(this.user1, next)
           },
 
           // login, should add session to set
           next => {
-            return this.user1.login(err => next(err))
+            this.user1.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(1)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // login again, should add the second session to set
           next => {
-            return this.user2.login(err => next(err))
+            this.user2.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(2)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
               expect(sessions[1].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // login third session, should add the second session to set
           next => {
-            return this.user3.login(err => next(err))
+            this.user3.login(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(3)
               expect(sessions[0].slice(0, 5)).to.equal('sess:')
               expect(sessions[1].slice(0, 5)).to.equal('sess:')
-              return next()
+              next()
             })
           },
 
           // enter sudo-mode
           next => {
-            return this.user2.getCsrfToken(err => {
+            this.user2.getCsrfToken(err => {
               expect(err).to.be.oneOf([null, undefined])
-              return this.user2.request.post(
+              this.user2.request.post(
                 {
                   uri: '/confirm-password',
                   json: {
@@ -401,7 +399,7 @@ describe('Sessions', function() {
                 (err, response, body) => {
                   expect(err).to.be.oneOf([null, undefined])
                   expect(response.statusCode).to.equal(200)
-                  return next()
+                  next()
                 }
               )
             })
@@ -409,23 +407,23 @@ describe('Sessions', function() {
 
           // check the sessions page
           next => {
-            return this.user2.request.get(
+            this.user2.request.get(
               {
                 uri: '/user/sessions'
               },
               (err, response, body) => {
                 expect(err).to.be.oneOf([null, undefined])
                 expect(response.statusCode).to.equal(200)
-                return next()
+                next()
               }
             )
           },
 
           // clear sessions from second session, should erase two of the three sessions
           next => {
-            return this.user2.getCsrfToken(err => {
+            this.user2.getCsrfToken(err => {
               expect(err).to.be.oneOf([null, undefined])
-              return this.user2.request.post(
+              this.user2.request.post(
                 {
                   uri: '/user/sessions/clear'
                 },
@@ -435,47 +433,63 @@ describe('Sessions', function() {
           },
 
           next => {
-            return redis.getUserSessions(this.user2, (err, sessions) => {
+            redis.getUserSessions(this.user2, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(1)
-              return next()
+              next()
             })
           },
 
           // users one and three should not be able to access project list page
           next => {
-            return this.user1.getProjectListPage((err, statusCode) => {
+            this.user1.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(302)
-              return next()
+              next()
             })
           },
 
           next => {
-            return this.user3.getProjectListPage((err, statusCode) => {
+            this.user3.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(302)
-              return next()
+              next()
             })
           },
 
           // user two should still be logged in, and able to access project list page
           next => {
-            return this.user2.getProjectListPage((err, statusCode) => {
+            this.user2.getProjectListPage((err, statusCode) => {
               expect(err).to.equal(null)
               expect(statusCode).to.equal(200)
-              return next()
+              next()
             })
           },
 
           // logout second session, should remove last session from set
           next => {
-            return this.user2.logout(err => next(err))
+            this.user2.logout(err => next(err))
           },
 
           next => {
-            return redis.getUserSessions(this.user1, (err, sessions) => {
+            redis.getUserSessions(this.user1, (err, sessions) => {
+              expect(err).to.not.exist
               expect(sessions.length).to.equal(0)
-              return next()
+              next()
+            })
+          },
+
+          // the user audit log should have been updated
+          next => {
+            this.user1.get((error, user) => {
+              expect(error).not.to.exist
+              expect(user.auditLog).to.exist
+              expect(user.auditLog[0].operation).to.equal('clear-sessions')
+              expect(user.auditLog[0].ipAddress).to.exist
+              expect(user.auditLog[0].initiatorId).to.exist
+              expect(user.auditLog[0].timestamp).to.exist
+              expect(user.auditLog[0].info.sessions.length).to.equal(2)
+              next()
             })
           }
         ],
@@ -483,7 +497,7 @@ describe('Sessions', function() {
           if (err) {
             throw err
           }
-          return done()
+          done()
         }
       )
     })

services/web/test/unit/src/User/UserControllerTests.js

@@ -67,7 +67,11 @@ describe('UserController', function() {
     this.UserSessionsManager = {
       trackSession: sinon.stub(),
       untrackSession: sinon.stub(),
-      revokeAllUserSessions: sinon.stub().callsArgWith(2, null)
+      revokeAllUserSessions: sinon.stub().callsArgWith(2, null),
+      promises: {
+        getAllUserSessions: sinon.stub().resolves(),
+        revokeAllUserSessions: sinon.stub().resolves()
+      }
     }
     this.SudoModeHandler = { clearSudoMode: sinon.stub() }
     this.HttpErrorHandler = {
@@ -97,6 +101,11 @@ describe('UserController', function() {
         '../Referal/ReferalAllocator': this.ReferalAllocator,
         '../Subscription/SubscriptionDomainHandler': this
           .SubscriptionDomainHandler,
+        './UserAuditLogHandler': (this.UserAuditLogHandler = {
+          promises: {
+            addEntry: sinon.stub().resolves()
+          }
+        }),
         './UserHandler': this.UserHandler,
         './UserSessionsManager': this.UserSessionsManager,
         '../SudoMode/SudoModeHandler': this.SudoModeHandler,
@@ -557,31 +566,57 @@ describe('UserController', function() {
 
   describe('clearSessions', function() {
     it('should call revokeAllUserSessions', function(done) {
+      this.res.sendStatus.callsFake(() => {
+        this.UserSessionsManager.promises.revokeAllUserSessions.callCount.should.equal(
+          1
+        )
+        done()
+      })
       this.UserController.clearSessions(this.req, this.res)
-      this.UserSessionsManager.revokeAllUserSessions.callCount.should.equal(1)
-      done()
     })
 
     it('send a 201 response', function(done) {
-      this.res.sendStatus = status => {
+      this.res.sendStatus.callsFake(status => {
         status.should.equal(201)
         done()
-      }
-      this.UserController.clearSessions(this.req, this.res)
+      })
+
+      this.UserController.clearSessions(this.req, this.res, () => {
+        done()
+      })
+    })
+
+    describe('when getAllUserSessions produces an error', function() {
+      it('should return an error', function(done) {
+        this.UserSessionsManager.promises.getAllUserSessions.rejects(
+          new Error('woops')
+        )
+        this.UserController.clearSessions(this.req, this.res, error => {
+          expect(error).to.be.instanceof(Error)
+          done()
+        })
+      })
+    })
+
+    describe('when audit log addEntry produces an error', function() {
+      it('should call next with an error', function(done) {
+        this.UserAuditLogHandler.promises.addEntry.rejects(new Error('woops'))
+        this.UserController.clearSessions(this.req, this.res, error => {
+          expect(error).to.be.instanceof(Error)
+          done()
+        })
+      })
     })
 
     describe('when revokeAllUserSessions produces an error', function() {
       it('should call next with an error', function(done) {
-        this.UserSessionsManager.revokeAllUserSessions.callsArgWith(
-          2,
+        this.UserSessionsManager.promises.revokeAllUserSessions.rejects(
           new Error('woops')
         )
-        const next = err => {
-          expect(err).to.not.equal(null)
-          expect(err).to.be.instanceof(Error)
+        this.UserController.clearSessions(this.req, this.res, error => {
+          expect(error).to.be.instanceof(Error)
           done()
-        }
-        this.UserController.clearSessions(this.req, this.res, next)
+        })
       })
     })
   })