diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index 1984cd6a..f0a005cd 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -875,6 +875,16 @@ export default class ControlConnection extends BaseConnection { this.updateLogSubscriptions(); } + /** Stop following logs if the user no longer has permission to */ + checkLogPermission() { + try { + this.user.checkPermission("core.log.follow"); + } catch { + this.logSubscriptions = { all: false, controller: false, hostIds: [], instanceIds: [] }; + this.updateLogSubscriptions(); + } + } + updateLogSubscriptions() { let { all, controller, hostIds, instanceIds } = this.logSubscriptions; if (all || controller || hostIds.length || instanceIds.length) { diff --git a/packages/controller/src/Controller.ts b/packages/controller/src/Controller.ts index 68cf411a..b3d727df 100644 --- a/packages/controller/src/Controller.ts +++ b/packages/controller/src/Controller.ts @@ -1,6 +1,7 @@ import type winston from "winston"; import { BlockList, type AddressInfo, isIPv4, isIPv6 } from "net"; import type { ControllerArgs } from "../controller.js"; +import type ControlConnection from "./ControlConnection.js"; import express, { type Request, type Response, type NextFunction, type Application } from "express"; import type { Static } from "@sinclair/typebox"; import finalhandler from "finalhandler"; @@ -1086,18 +1087,24 @@ export default class Controller { userPermissionsUpdated(user: Readonly) { for (let controlConnection of this.wsServer.controlConnections.values()) { if (controlConnection.user === user) { - this.subscriptions.unsubscribeUnpermitted(controlConnection, user); - controlConnection.send( - new lib.AccountUpdateEvent([...user.roles].map(r => ({ - name: r.name, - id: r.id, - permissions: [...r.permissions], - }))) - ); + this._controlPermissionsUpdated(controlConnection); } } } + _controlPermissionsUpdated(controlConnection: ControlConnection) { + const user = controlConnection.user; + this.subscriptions.checkLinkPermissions(controlConnection, user); + controlConnection.checkLogPermission(); + controlConnection.send( + new lib.AccountUpdateEvent([...user.roles].map(r => ({ + name: r.name, + id: r.id, + permissions: [...r.permissions], + }))) + ); + } + rolesUpdated(roles: lib.Role[]) { this.subscriptions.broadcast(new lib.RoleUpdatesEvent(roles)); this.hooks.rolesUpdated.invoke(roles); @@ -1105,16 +1112,7 @@ export default class Controller { for (const role of roles) { for (let controlConnection of this.wsServer.controlConnections.values()) { if (controlConnection.user.roles.has(role)) { - this.subscriptions.unsubscribeUnpermitted(controlConnection, controlConnection.user); - controlConnection.send( - new lib.AccountUpdateEvent( - [...controlConnection.user.roles].map(r => ({ - name: r.name, - id: r.id, - permissions: [...r.permissions], - })) - ) - ); + this._controlPermissionsUpdated(controlConnection); } } } diff --git a/packages/lib/src/subscriptions.ts b/packages/lib/src/subscriptions.ts index 0222e1ed..df42c6c1 100644 --- a/packages/lib/src/subscriptions.ts +++ b/packages/lib/src/subscriptions.ts @@ -330,12 +330,13 @@ export class SubscriptionController { } /** - * Unsubscribe a link from all events the user no longer has permission to subscribe to. + * Check the subscriptions of a link against the permissions of a user, + * removing those the user no longer has permission for. * Used when the permissions of a user changes. * @param link - Link to check the subscriptions of. * @param user - User the link is authenticated as. */ - unsubscribeUnpermitted(link: Link, user: IUser) { + checkLinkPermissions(link: Link, user: IUser) { for (const [eventName, eventData] of this._events) { for (const [addressIndex, subscriber] of eventData.subscriptions) { if (subscriber.link !== link) { diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index 53110f8d..88dd2b80 100644 --- a/test/controller/ControlConnection.test.js +++ b/test/controller/ControlConnection.test.js @@ -189,6 +189,25 @@ describe("controller/src/ControlConnection", function() { assert.equal(connection.logTransport, null); }); + it("stops following logs when the user loses permission", async function() { + await connection.handleLogSetSubscriptionsRequest( + new lib.LogSetSubscriptionsRequest(true, false, [], []) + ); + connection.user = { checkPermission() { throw new lib.PermissionError("Permission denied"); } }; + connection.checkLogPermission(); + assert.equal(mockController.clusterLogger.transports.size, 0); + assert.equal(connection.logTransport, null); + }); + + it("keeps following logs when the user has permission", async function() { + await connection.handleLogSetSubscriptionsRequest( + new lib.LogSetSubscriptionsRequest(true, false, [], []) + ); + connection.user = { checkPermission() {} }; + connection.checkLogPermission(); + assert.equal(mockController.clusterLogger.transports.size, 1); + }); + it("removes the debug dump listener after repeated requests", async function() { await connection.handleDebugDumpWsRequest(new lib.DebugDumpWsRequest()); await connection.handleDebugDumpWsRequest(new lib.DebugDumpWsRequest()); diff --git a/test/controller/Controller.js b/test/controller/Controller.js index fa5444db..ac0955af 100644 --- a/test/controller/Controller.js +++ b/test/controller/Controller.js @@ -10,7 +10,7 @@ import { fileURLToPath } from "node:url"; import { ControllerConfig, Address, RequestError, - InstanceConfig, SystemInfo, Role, ModPack, + InstanceConfig, SystemInfo, Role, ModPack, AccountUpdateEvent, addPluginConfigFields, } from "@clusterio/lib"; @@ -261,28 +261,68 @@ describe("controller/src/Controller", function() { }); }); - describe("permission changes", function() { - let role, connection, checked, originalWsServer; - beforeEach(function() { - role = new Role(1, "Test", "", new Set()); - const user = { roles: new Set([role]) }; - connection = { user, send() {} }; - originalWsServer = controller.wsServer; - controller.wsServer = { controlConnections: new Map([[1, connection]]) }; - checked = []; - controller.subscriptions.unsubscribeUnpermitted = (link, linkUser) => checked.push([link, linkUser]); + function mockPermissionChanges() { + const role = new Role(1, "Test", "", new Set(["core.log.follow"])); + const otherRole = new Role(2, "Other", "", new Set()); + const user = { roles: new Set([role]) }; + const otherUser = { roles: new Set([otherRole]) }; + const sent = []; + const checked = []; + const connection = { + user, + logChecked: false, + send(message) { sent.push(message); }, + checkLogPermission() { this.logChecked = true; }, + }; + const otherConnection = { + user: otherUser, + send(message) { sent.push(message); }, + checkLogPermission() {}, + }; + const originalWsServer = controller.wsServer; + controller.wsServer = { controlConnections: new Map([[1, connection], [2, otherConnection]]) }; + controller.subscriptions.checkLinkPermissions = (link, linkUser) => checked.push([link, linkUser]); + return { + role, user, connection, sent, checked, + restore() { + controller.wsServer = originalWsServer; + delete controller.subscriptions.checkLinkPermissions; + }, + }; + } + function assertAccountUpdate(sent, role) { + assert.equal(sent.length, 1); + assert(sent[0] instanceof AccountUpdateEvent); + assert.deepEqual(sent[0].roles, [{ name: role.name, id: role.id, permissions: [...role.permissions] }]); + } + + describe(".userPermissionsUpdated()", function() { + let mocked; + beforeEach(function() { mocked = mockPermissionChanges(); }); + afterEach(function() { mocked.restore(); }); + it("should send an account update to connections of the user", function() { + controller.userPermissionsUpdated(mocked.user); + assertAccountUpdate(mocked.sent, mocked.role); }); - afterEach(function() { - controller.wsServer = originalWsServer; - delete controller.subscriptions.unsubscribeUnpermitted; + it("should recheck subscriptions and log following of the user", function() { + controller.userPermissionsUpdated(mocked.user); + assert.deepEqual(mocked.checked, [[mocked.connection, mocked.user]]); + assert(mocked.connection.logChecked, "log permission was not checked"); }); - it("should recheck subscriptions when a user's permissions change", function() { - controller.userPermissionsUpdated(connection.user); - assert.deepEqual(checked, [[connection, connection.user]]); + }); + + describe(".rolesUpdated()", function() { + let mocked; + beforeEach(function() { mocked = mockPermissionChanges(); }); + afterEach(function() { mocked.restore(); }); + it("should send an account update to connections with the role", function() { + controller.rolesUpdated([mocked.role]); + assertAccountUpdate(mocked.sent, mocked.role); }); - it("should recheck subscriptions of users with an updated role", function() { - controller.rolesUpdated([role]); - assert.deepEqual(checked, [[connection, connection.user]]); + it("should recheck subscriptions and log following of users with the role", function() { + controller.rolesUpdated([mocked.role]); + assert.deepEqual(mocked.checked, [[mocked.connection, mocked.user]]); + assert(mocked.connection.logChecked, "log permission was not checked"); }); }); diff --git a/test/lib/subscriptions.test.js b/test/lib/subscriptions.test.js index ebc6a79c..55ddd684 100644 --- a/test/lib/subscriptions.test.js +++ b/test/lib/subscriptions.test.js @@ -552,7 +552,7 @@ describe("lib/subscriptions", function() { }); }); - describe(".unsubscribeUnpermitted()", function() { + describe(".checkLinkPermissions()", function() { class DenyingUser { constructor(denied) { this.denied = denied; @@ -576,7 +576,7 @@ describe("lib/subscriptions", function() { }); it("should remove subscriptions with a string permission the user lost", async function() { - subscriptions.unsubscribeUnpermitted(getLink(0), new DenyingUser("StringPermission")); + subscriptions.checkLinkPermissions(getLink(0), new DenyingUser("StringPermission")); subscriptions.broadcast(new StringPermissionEvent()); await onceConnectorSend(1); assertNoEvent(0); @@ -586,7 +586,7 @@ describe("lib/subscriptions", function() { }); it("should remove subscriptions with a function permission the user lost", async function() { - subscriptions.unsubscribeUnpermitted(getLink(0), new DenyingUser("FunctionPermission")); + subscriptions.checkLinkPermissions(getLink(0), new DenyingUser("FunctionPermission")); subscriptions.broadcast(new FunctionPermissionEvent()); await onceConnectorSend(1); assertNoEvent(0); @@ -596,7 +596,7 @@ describe("lib/subscriptions", function() { }); it("should keep subscriptions the user still has permission for", async function() { - subscriptions.unsubscribeUnpermitted(getLink(0), new DenyingUser(null)); + subscriptions.checkLinkPermissions(getLink(0), new DenyingUser(null)); subscriptions.broadcast(new StringPermissionEvent()); await onceConnectorSend(0); assertLastEvent(0, StringPermissionEvent);