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 87739e81..ef82a15b 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"; @@ -1087,17 +1088,24 @@ export default class Controller { userPermissionsUpdated(user: Readonly) { for (let controlConnection of this.wsServer.controlConnections.values()) { if (controlConnection.user === 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,15 +1113,7 @@ export default class Controller { for (const role of roles) { for (let controlConnection of this.wsServer.controlConnections.values()) { if (controlConnection.user.roles.has(role)) { - 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 bcad8b2a..c6279375 100644 --- a/packages/lib/src/subscriptions.ts +++ b/packages/lib/src/subscriptions.ts @@ -355,6 +355,32 @@ export class SubscriptionController { } } + /** + * 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. + */ + checkLinkPermissions(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/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 26b80ba1..5ca508ad 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"; @@ -279,6 +279,71 @@ describe("controller/src/Controller", function() { }); }); + 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); + }); + 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"); + }); + }); + + 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 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"); + }); + }); + 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 e605a0fc..585549f4 100644 --- a/test/lib/subscriptions.test.js +++ b/test/lib/subscriptions.test.js @@ -552,6 +552,57 @@ describe("lib/subscriptions", function() { }); }); + describe(".checkLinkPermissions()", 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.checkLinkPermissions(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.checkLinkPermissions(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.checkLinkPermissions(getLink(0), new DenyingUser(null)); + subscriptions.broadcast(new StringPermissionEvent()); + await onceConnectorSend(0); + assertLastEvent(0, StringPermissionEvent); + }); + }); + describe(".handleRequest()", function() { beforeEach(function() { subscriptions.handle(RegisteredEvent);