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:
6dc3f1bf56/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:
be4146e324/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

@@ -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'});
     }
 });
 

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