From f732b0181d88d0dd8f676ff85f8ea6fe40257758 Mon Sep 17 00:00:00 2001
From: Simon Backx <simon@ghost.org>
Date: Tue, 10 May 2022 13:34:12 +0200
Subject: [PATCH] Fixed `last_seen` updated for suspended users (#14715)

refs https://github.com/TryGhost/Team/issues/1461

- A suspended user was able to make it through the Express middlewares to the `updateUserLastSeen` middleware, until it was halted when checking the user permissions in the API pipeline. This was only the case for session logins, not for API keys.
- For API keys, the user status is checked:
https://github.com/TryGhost/Ghost/blob/6dc3f1bf5653adf010aca06f1bad4226d551c281/core/server/services/auth/api-key/admin.js#L178-L181
- In the session middleware, the `findUserById` in `getUserForSession` didn't filter on the active status of users:
https://github.com/TryGhost/Ghost/blob/be4146e3245e0812f1a7497d9280e5064568327d/core/server/services/auth/session/middleware.js#L22-L27
- This has been fixed now by updating the sessionService's `findUserById` method.
---
 core/server/services/auth/session/index.js    |   2 +-
 .../api/admin/update-user-last-seen.test.js   | 115 ++++++++++++++++++
 2 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 test/regression/api/admin/update-user-last-seen.test.js

diff --git a/core/server/services/auth/session/index.js b/core/server/services/auth/session/index.js
index cac9b6c388..e166d9e4b0 100644
--- a/core/server/services/auth/session/index.js
+++ b/core/server/services/auth/session/index.js
@@ -32,7 +32,7 @@ const sessionService = createSessionService({
     getOriginOfRequest,
     getSession: expressSession.getSession,
     findUserById({id}) {
-        return models.User.findOne({id});
+        return models.User.findOne({id, status: 'active'});
     }
 });
 
diff --git a/test/regression/api/admin/update-user-last-seen.test.js b/test/regression/api/admin/update-user-last-seen.test.js
new file mode 100644
index 0000000000..7a3ef600f0
--- /dev/null
+++ b/test/regression/api/admin/update-user-last-seen.test.js
@@ -0,0 +1,115 @@
+const {DataGenerator} = require('../../../utils');
+const {agentProvider, mockManager, fixtureManager, matchers} = require('../../../utils/e2e-framework');
+const models = require('../../../../core/server/models');
+const sinon = require('sinon');
+require('should');
+
+let agent;
+let clock;
+let sandbox;
+
+// We currently use the owner, but it would be better to switch this to an Administrator in the future
+// for these tests, when the issue with roles in test fixtures is resolved.
+const userId = DataGenerator.Content.users[0].id;
+
+describe('Update User Last Seen', function () {
+    before(async function () {
+        agent = await agentProvider.getAdminAPIAgent();
+        await fixtureManager.init();
+
+        // Important to enable the fake timers before logging in
+        // Because the last_seen of the owner will be set already here
+        sandbox = sinon.createSandbox();
+        clock = sinon.useFakeTimers();
+
+        await agent.loginAsOwner();
+
+        // Fixtures aren't working for roles. So need to use the owner for now.
+        /*await fixtureManager.init('roles', 'users:no-owner');
+        await agent.loginAs(
+            DataGenerator.Content.users[1].email,
+            DataGenerator.Content.users[1].password
+        );*/
+    });
+
+    after(function () {
+        clock.restore();
+        sandbox.restore();
+    });
+
+    it('Should update last seen for active users', async function () {
+        // Fetching should work fine
+        await agent
+            .get(`posts/`)
+            .expectStatus(200);
+
+        const user = await models.User.findOne({id: userId});
+        should.exist(user);
+        const lastSeen = user.get('last_seen');
+        should.exist(lastSeen);
+
+        clock.tick(1000 * 60 * 60 * 24);
+
+        await agent
+            .get(`posts/`)
+            .expectStatus(200);
+
+        const ownerAfter = await models.User.findOne({id: userId});
+        should.exist(ownerAfter);
+        should(ownerAfter.get('last_seen')).not.eql(lastSeen);
+    });
+
+    it('Should only update last seen after 1 hour', async function () {
+        const user = await models.User.findOne({id: userId});
+        const lastSeen = user.get('last_seen');
+        should.exist(lastSeen);
+
+        clock.tick(1000 * 60 * 30);
+
+        // Fetching should work fine
+        await agent
+            .get(`posts/`)
+            .expectStatus(200);
+
+        const ownerAfter = await models.User.findOne({id: userId});
+        should.exist(ownerAfter);
+        should(ownerAfter.get('last_seen')).eql(lastSeen);
+    });
+
+    it('Should always update last seen after login', async function () {
+        const user = await models.User.findOne({id: userId});
+        const lastSeen = user.get('last_seen');
+        should.exist(lastSeen);
+
+        await agent.loginAsOwner();
+
+        const ownerAfter = await models.User.findOne({id: userId});
+        should.exist(ownerAfter);
+        should(ownerAfter.get('last_seen')).not.eql(lastSeen);
+    });
+
+    it('Should not update last seen for suspended users', async function () {
+        // Fetching should work fine
+        await agent
+            .get(`posts/`)
+            .expectStatus(200);
+
+        // Suspend the user
+        const user = await models.User.findOne({id: userId});
+        should.exist(user);
+
+        await models.User.edit({status: 'inactive'}, {id: userId});
+        const lastSeen = user.get('last_seen');
+        should.exist(lastSeen);
+
+        clock.tick(1000 * 60 * 60 * 24);
+
+        await agent
+            .get(`posts/`)
+            .expectStatus(403);
+
+        const ownerAfter = await models.User.findOne({id: userId});
+        should.exist(ownerAfter);
+        should(ownerAfter.get('last_seen')).eql(lastSeen);
+    });
+});