From e2798c26fca5d1c564f831b71af0dec844906cbf Mon Sep 17 00:00:00 2001 From: June Kelly Date: Wed, 20 Oct 2021 09:45:03 +0100 Subject: [PATCH] Merge pull request #5484 from overleaf/tm-show-current-session Show current session on user sessions page GitOrigin-RevId: 6ae130bfa8c3d82a305fd865e162c19f5c8b208c --- .../app/src/Features/User/UserController.js | 7 +++++- .../src/Features/User/UserPagesController.js | 25 ++++++++----------- .../src/Features/User/UserSessionsManager.js | 22 ++++++++++++---- services/web/app/views/user/sessions.pug | 13 ++++++++++ services/web/locales/en.json | 2 ++ .../test/unit/src/User/UserControllerTests.js | 21 +++++++++++++++- .../unit/src/User/UserPagesControllerTests.js | 4 +-- .../unit/src/User/UserSessionsManagerTests.js | 4 +-- 8 files changed, 73 insertions(+), 25 deletions(-) diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 4e016e499e..a581fd39cf 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -22,6 +22,7 @@ const { expressify } = require('../../util/promises') const { acceptsJson, } = require('../../infrastructure/RequestContentTypeDetection') +const _ = require('lodash') async function _sendSecurityAlertClearedSessions(user) { const emailOptions = { @@ -132,7 +133,11 @@ async function clearSessions(req, res, next) { 'clear-sessions', user._id, req.ip, - { sessions } + { + sessions: sessions.map( + session => _.pick(session, ['ip_address', 'session_created']) // omit other session data from log + ), + } ) await UserSessionsManager.promises.revokeAllUserSessions(user, [ req.sessionID, diff --git a/services/web/app/src/Features/User/UserPagesController.js b/services/web/app/src/Features/User/UserPagesController.js index 975f13ac1b..a653164033 100644 --- a/services/web/app/src/Features/User/UserPagesController.js +++ b/services/web/app/src/Features/User/UserPagesController.js @@ -137,22 +137,19 @@ const UserPagesController = { sessionsPage(req, res, next) { const user = SessionManager.getSessionUser(req.session) logger.log({ userId: user._id }, 'loading sessions page') - UserSessionsManager.getAllUserSessions( - user, - [req.sessionID], - (err, sessions) => { - if (err != null) { - OError.tag(err, 'error getting all user sessions', { - userId: user._id, - }) - return next(err) - } - res.render('user/sessions', { - title: 'sessions', - sessions, + UserSessionsManager.getAllUserSessions(user, (err, sessions) => { + if (err != null) { + OError.tag(err, 'error getting all user sessions', { + userId: user._id, }) + return next(err) } - ) + res.render('user/sessions', { + title: 'sessions', + sessions: sessions.filter(session => session.id !== req.sessionID), + currentSession: sessions.find(session => session.id === req.sessionID), + }) + }) }, _restructureThirdPartyIds(user) { diff --git a/services/web/app/src/Features/User/UserSessionsManager.js b/services/web/app/src/Features/User/UserSessionsManager.js index ce1abf1f0c..a4f2b73575 100644 --- a/services/web/app/src/Features/User/UserSessionsManager.js +++ b/services/web/app/src/Features/User/UserSessionsManager.js @@ -77,6 +77,10 @@ const UserSessionsManager = { }, getAllUserSessions(user, exclude, callback) { + if (typeof exclude === 'function') { + callback = exclude + exclude = [] + } exclude = _.map(exclude, UserSessionsManager._sessionKey) const sessionSetKey = UserSessionsRedis.sessionSetKey(user) rclient.smembers(sessionSetKey, function (err, sessionKeys) { @@ -94,7 +98,14 @@ const UserSessionsManager = { Async.mapSeries( sessionKeys, - (k, cb) => rclient.get(k, cb), + (k, cb) => { + rclient.get(k, (err, res) => { + if (err) { + return cb(err) + } + cb(null, { id: k, data: res }) + }) + }, function (err, sessions) { if (err) { OError.tag(err, 'error getting all sessions for user from redis', { @@ -104,17 +115,18 @@ const UserSessionsManager = { } const result = [] - for (let session of Array.from(sessions)) { + for (const session of Array.from(sessions)) { if (!session) { continue } - session = JSON.parse(session) - let sessionUser = session.passport && session.passport.user + const sessionData = JSON.parse(session.data) + let sessionUser = sessionData.passport && sessionData.passport.user if (!sessionUser) { - sessionUser = session.user + sessionUser = sessionData.user } result.push({ + id: session.id.replace('sess:', ''), ip_address: sessionUser.ip_address, session_created: sessionUser.session_created, }) diff --git a/services/web/app/views/user/sessions.pug b/services/web/app/views/user/sessions.pug index 708a909849..45755fcf2a 100644 --- a/services/web/app/views/user/sessions.pug +++ b/services/web/app/views/user/sessions.pug @@ -9,6 +9,19 @@ block content .page-header h1 #{translate("your_sessions")} + if currentSession + h3 #{translate("current_session")} + div + table.table.table-striped + thead + tr + th #{translate("ip_address")} + th #{translate("session_created_at")} + tr + td #{currentSession.ip_address} + td #{moment(currentSession.session_created).utc().format('Do MMM YYYY, h:mm a')} UTC + + h3 #{translate("other_sessions")} div p.small | !{translate("clear_sessions_description")} diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 5b3300088a..c93ac876fd 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -625,6 +625,8 @@ "clear_sessions_success": "Sessions Cleared", "sessions": "Sessions", "manage_sessions": "Manage Your Sessions", + "current_session": "Current Session", + "other_sessions": "Other Sessions", "syntax_validation": "Code check", "history": "History", "joining": "Joining", diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 77cf549500..a5a4c46555 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -74,7 +74,7 @@ describe('UserController', function () { untrackSession: sinon.stub(), revokeAllUserSessions: sinon.stub().callsArgWith(2, null), promises: { - getAllUserSessions: sinon.stub().resolves(), + getAllUserSessions: sinon.stub().resolves([]), revokeAllUserSessions: sinon.stub().resolves(), }, } @@ -621,6 +621,25 @@ describe('UserController', function () { this.UserController.clearSessions(this.req, this.res) }) + + it('should include only relevant session data in the audit log', function (done) { + this.UserSessionsManager.promises.getAllUserSessions.resolves([ + { id: 'session-id', ip_address: 'ip', session_created: 'created' }, + ]) + this.res.sendStatus.callsFake(status => { + this.UserAuditLogHandler.promises.addEntry.callCount.should.equal(1) + const addEntryCall = this.UserAuditLogHandler.promises.addEntry + .lastCall + expect(addEntryCall.args[4].sessions).to.be.instanceOf(Array) + expect(addEntryCall.args[4].sessions[0]).to.have.keys([ + 'ip_address', + 'session_created', + ]) + expect(addEntryCall.args[4].sessions[0]).to.not.have.keys(['id']) + done() + }) + this.UserController.clearSessions(this.req, this.res) + }) }) describe('errors', function () { diff --git a/services/web/test/unit/src/User/UserPagesControllerTests.js b/services/web/test/unit/src/User/UserPagesControllerTests.js index cc60bb104e..b0dfb56e1c 100644 --- a/services/web/test/unit/src/User/UserPagesControllerTests.js +++ b/services/web/test/unit/src/User/UserPagesControllerTests.js @@ -154,7 +154,7 @@ describe('UserPagesController', function () { describe('sessionsPage', function () { beforeEach(function () { return this.UserSessionsManager.getAllUserSessions.callsArgWith( - 2, + 1, null, [] ) @@ -179,7 +179,7 @@ describe('UserPagesController', function () { describe('when getAllUserSessions produces an error', function () { beforeEach(function () { return this.UserSessionsManager.getAllUserSessions.callsArgWith( - 2, + 1, new Error('woops') ) }) diff --git a/services/web/test/unit/src/User/UserSessionsManagerTests.js b/services/web/test/unit/src/User/UserSessionsManagerTests.js index ea5e354000..e3615b6fa3 100644 --- a/services/web/test/unit/src/User/UserSessionsManagerTests.js +++ b/services/web/test/unit/src/User/UserSessionsManagerTests.js @@ -606,8 +606,8 @@ describe('UserSessionsManager', function () { it('should get sessions', function (done) { return this.call((err, sessions) => { expect(sessions).to.deep.equal([ - { ip_address: 'a', session_created: 'b' }, - { ip_address: 'c', session_created: 'd' }, + { id: 'one', ip_address: 'a', session_created: 'b' }, + { id: 'three', ip_address: 'c', session_created: 'd' }, ]) return done() })