Merge pull request #1056 from bbassie/fix/subscription-permission-revoke

Drop subscriptions when a user loses permission for them

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) {

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<User>) {
 		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);
 				}
 			}
 		}

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

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());

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;

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