From beec2f523a303528270ef4ddff35ae8e228e4cd5 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:11:28 +0000 Subject: [PATCH 1/2] Drop subscriptions when a user loses permission for them The permission for a subscription was only checked when subscribing, so a control connection kept receiving events after the user's roles or role permissions changed to no longer allow it. Add SubscriptionController.unsubscribeUnpermitted which rechecks every subscription of a link and removes the ones the user is no longer permitted, and call it for the affected control connections from userPermissionsUpdated and rolesUpdated. Co-Authored-By: Claude Opus 5.5 --- packages/controller/src/Controller.ts | 2 ++ packages/lib/src/subscriptions.ts | 25 +++++++++++++ test/controller/Controller.js | 25 +++++++++++++ test/lib/subscriptions.test.js | 51 +++++++++++++++++++++++++++ 4 files changed, 103 insertions(+) diff --git a/packages/controller/src/Controller.ts b/packages/controller/src/Controller.ts index 75a0e084..68cf411a 100644 --- a/packages/controller/src/Controller.ts +++ b/packages/controller/src/Controller.ts @@ -1086,6 +1086,7 @@ 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, @@ -1104,6 +1105,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 => ({ diff --git a/packages/lib/src/subscriptions.ts b/packages/lib/src/subscriptions.ts index 298bf5d7..0222e1ed 100644 --- a/packages/lib/src/subscriptions.ts +++ b/packages/lib/src/subscriptions.ts @@ -329,6 +329,31 @@ export class SubscriptionController { } } + /** + * Unsubscribe a link from all events the user no longer has permission to subscribe to. + * 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) { + for (const [eventName, eventData] of this._events) { + for (const [addressIndex, subscriber] of eventData.subscriptions) { + if (subscriber.link !== link) { + continue; + } + const request = new SubscriptionRequest(eventName, "subscribe", 0, subscriber.filters); + const message = new MessageRequest( + 0, subscriber.address, new Address(Address.controller, 0), "SubscriptionRequest", request.toJSON(), + ); + try { + SubscriptionRequest.permission(user, message); + } catch { + eventData.subscriptions.delete(addressIndex); + } + } + } + } + /** * Handle incoming subscription requests on a link * @param link - Link message was received on diff --git a/test/controller/Controller.js b/test/controller/Controller.js index efcc69ee..fa5444db 100644 --- a/test/controller/Controller.js +++ b/test/controller/Controller.js @@ -261,6 +261,31 @@ 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]); + }); + afterEach(function() { + controller.wsServer = originalWsServer; + delete controller.subscriptions.unsubscribeUnpermitted; + }); + it("should recheck subscriptions when a user's permissions change", function() { + controller.userPermissionsUpdated(connection.user); + assert.deepEqual(checked, [[connection, connection.user]]); + }); + it("should recheck subscriptions of users with an updated role", function() { + controller.rolesUpdated([role]); + assert.deepEqual(checked, [[connection, connection.user]]); + }); + }); + describe(".instances", function() { it("should set the dirty flag if the config is updated", function() { controller.instances.records.dirty = false; diff --git a/test/lib/subscriptions.test.js b/test/lib/subscriptions.test.js index 780d58b8..ebc6a79c 100644 --- a/test/lib/subscriptions.test.js +++ b/test/lib/subscriptions.test.js @@ -552,6 +552,57 @@ describe("lib/subscriptions", function() { }); }); + describe(".unsubscribeUnpermitted()", function() { + class DenyingUser { + constructor(denied) { + this.denied = denied; + } + + checkPermission(permission) { + if (permission === this.denied) { + throw new lib.PermissionError("Permission denied"); + } + } + } + beforeEach(async function() { + subscriptions.handle(StringPermissionEvent); + subscriptions.handle(FunctionPermissionEvent); + for (const connectorData of connectorSetupDate) { + for (const Event of [StringPermissionEvent, FunctionPermissionEvent]) { + const request = new lib.SubscriptionRequest(Event.name, "subscribe"); + await subscriptions.handleRequest(getLink(connectorData.id), request, connectorData.dst); + } + } + }); + + it("should remove subscriptions with a string permission the user lost", async function() { + subscriptions.unsubscribeUnpermitted(getLink(0), new DenyingUser("StringPermission")); + subscriptions.broadcast(new StringPermissionEvent()); + await onceConnectorSend(1); + assertNoEvent(0); + subscriptions.broadcast(new FunctionPermissionEvent()); + await onceConnectorSend(0); + assertLastEvent(0, FunctionPermissionEvent); + }); + + it("should remove subscriptions with a function permission the user lost", async function() { + subscriptions.unsubscribeUnpermitted(getLink(0), new DenyingUser("FunctionPermission")); + subscriptions.broadcast(new FunctionPermissionEvent()); + await onceConnectorSend(1); + assertNoEvent(0); + subscriptions.broadcast(new StringPermissionEvent()); + await onceConnectorSend(0); + assertLastEvent(0, StringPermissionEvent); + }); + + it("should keep subscriptions the user still has permission for", async function() { + subscriptions.unsubscribeUnpermitted(getLink(0), new DenyingUser(null)); + subscriptions.broadcast(new StringPermissionEvent()); + await onceConnectorSend(0); + assertLastEvent(0, StringPermissionEvent); + }); + }); + describe(".handleRequest()", function() { beforeEach(function() { subscriptions.handle(RegisteredEvent); From d10da0f5fcb2ae85049f7b2c56b7daf3bc7875b4 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Thu, 8 Oct 2026 15:21:02 +0000 Subject: [PATCH 2/2] Rename to checkLinkPermissions and stop log following on revoke The old name read as an unconditional unsubscribe. Log following has its own subscription outside SubscriptionController, so check core.log.follow on permission changes too. Move the per-connection work into one helper and split the tests into userPermissionsUpdated and rolesUpdated, checking the account update event is sent. Co-Authored-By: Claude Opus 5.5 --- packages/controller/src/ControlConnection.ts | 10 +++ packages/controller/src/Controller.ts | 34 ++++----- packages/lib/src/subscriptions.ts | 5 +- test/controller/ControlConnection.test.js | 19 +++++ test/controller/Controller.js | 80 +++++++++++++++----- test/lib/subscriptions.test.js | 8 +- 6 files changed, 112 insertions(+), 44 deletions(-) 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);