Terminate control connections of deleted users (#1030)

* Terminate control connections of deleted users

ControlConnection keeps the User object it authenticated with and checks
permissions against it. Deleting the user removed the record but left
its sessions open, so a deleted user kept all its permissions until the
WebSocket closed.

Repro: create "victim" with role 0 and connect as it. Another admin
sends UserDeleteRequest("victim"). Victim can still send
UserCreateRequest("mallory") and UserUpdateRolesRequest("mallory", [0]).

Terminate the deleted user's connections like revoke-token does. When
a user deletes itself, defer terminating the requesting connection so
the response is sent first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Terminate deleted user sessions immediately

Drop the special case that deferred terminating the connection issuing
the delete, so deleting yourself behaves the same as revoke-token and
every session of the deleted user is closed before the handler returns.

Set tokenValidAfter to the current time in createUser instead of 0.
Deleting a user and recreating it under the same name left tokens issued
before the deletion valid, as the check is iat < tokenValidAfter. Doing
it in createUser covers every path that makes a user. A token signed in
the same second as creation is still accepted, so create-admin and the
other production paths sign after the user exists and are unaffected.

The integration harness signed the control token at import time, before
bootstrap create-admin created the user. Sign it in the root before hook
instead and reuse it in link.js.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Handle SessionLost when deleting your own account

Deleting your own account now terminates the session immediately, so the
web UI received a SessionLost rejection and reported "Error deleting
user" for a delete that had succeeded. Use the same branch revoke does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com>

packages/controller/src/ControlConnection.ts

@@ -1074,6 +1074,11 @@ export default class ControlConnection extends BaseConnection {
 		}
 
 		this._controller.users.deleteUser(user);
+		for (let controlConnection of this._controller.wsServer.controlConnections.values()) {
+			if (controlConnection.user.id === user.id) {
+				controlConnection.connector.terminate();
+			}
+		}
 
 		if (user.isAdmin) {
 			this._controller.sendTo("allInstances", new lib.InstanceAdminlistUpdateEvent(name, false));

packages/controller/src/UserManager.ts

@@ -119,7 +119,9 @@ export default class UserManager {
 			roles.add(defaultRoleId);
 		}
 
-		const user = new User(this.records, this._controllerRoles, 0, name, roles);
+		// Reject tokens issued before the user existed, e.g. for a previous user by the same name
+		const tokenValidAfter = Math.floor(Date.now() / 1000);
+		const user = new User(this.records, this._controllerRoles, tokenValidAfter, name, roles);
 		this.records.set(user);
 		return user;
 	}

packages/web_ui/src/components/UserViewPage.tsx

@@ -146,6 +146,13 @@ export default function UserViewPage() {
 							new lib.UserDeleteRequest(userName)
 						).then(() => {
 							navigate("/users");
+						}, err => {
+							if (err instanceof lib.SessionLost && userName === account.name) {
+								// Got kicked out after deleting our own account
+								navigate("/users");
+								return;
+							}
+							throw err;
 						}).catch(notifyErrorHandler("Error deleting user"));
 					}}
 				>

test/controller/ControlConnection.test.js

@@ -2,7 +2,7 @@ import assert from "node:assert/strict";
 import events from "node:events";
 import * as lib from "@clusterio/lib";
 import { Controller, ControlConnection, ControllerHooks } from "@clusterio/controller";
-import { MockConnector, MockLogger } from "../mock.js";
+import { MockConnector, MockController, MockLogger } from "../mock.js";
 
 const addr = lib.Address.fromShorthand;
 
@@ -123,4 +123,55 @@ describe("controller/src/ControlConnection", function() {
 			assert.equal(mockController.debugEvents.listenerCount("message"), 0);
 		});
 	});
+
+	describe(".handleUserDeleteRequest()", function() {
+		let mockController;
+		let nextId;
+
+		beforeEach(function() {
+			mockController = new MockController();
+			mockController.sendTo = () => {};
+			mockController.wsServer.controlConnections = new Map();
+			nextId = 1;
+		});
+
+		function connect(name) {
+			const connection = {
+				_controller: mockController,
+				user: mockController.users.getByName(name),
+				connector: {
+					terminated: false,
+					terminate() { this.terminated = true; },
+				},
+			};
+			mockController.wsServer.controlConnections.set(nextId, connection);
+			nextId += 1;
+			return connection;
+		}
+
+		async function deleteUser(connection, name) {
+			await ControlConnection.prototype.handleUserDeleteRequest.call(
+				connection, new lib.UserDeleteRequest(name)
+			);
+		}
+
+		it("terminates connections of the deleted user", async function() {
+			const admin = connect("test");
+			const player1 = connect("player");
+			const player2 = connect("player");
+
+			await deleteUser(admin, "player");
+			assert.equal(mockController.users.getByName("player"), undefined);
+			assert.equal(player1.connector.terminated, true);
+			assert.equal(player2.connector.terminated, true);
+			assert.equal(admin.connector.terminated, false);
+		});
+
+		it("terminates its own connection when deleting itself", async function() {
+			const player = connect("player");
+
+			await deleteUser(player, "player");
+			assert.equal(player.connector.terminated, true);
+		});
+	});
 });

test/controller/UserManager.test.js

@@ -79,6 +79,23 @@ describe("controller/UserManager", function () {
 			const user = userManager.createUser("Eve");
 			assert(user.roleIds.has(1));
 		});
+
+		it("should set tokenValidAfter to the current time", function () {
+			const before = Math.floor(Date.now() / 1000);
+			const user = userManager.createUser("Mallory");
+			assert(user.tokenValidAfter >= before, "tokenValidAfter predates the call");
+			assert(user.tokenValidAfter <= Math.floor(Date.now() / 1000), "tokenValidAfter is in the future");
+		});
+
+		it("should invalidate tokens issued before a user of the same name was recreated", function () {
+			const user = userManager.createUser("Niaj");
+			const oldTokenIat = user.tokenValidAfter - 60;
+			userManager.deleteUser(user);
+
+			// WsServer.registerControl rejects the token when iat < tokenValidAfter
+			const recreated = userManager.createUser("Niaj");
+			assert(oldTokenIat < recreated.tokenValidAfter, "token from before the deletion is still valid");
+		});
 	});
 
 	describe(".getOrCreateUser()", function () {

test/integration/index.js

@@ -191,7 +191,8 @@ let haveFactorioInstall;
 const baseHostConfig = loadJSON("config-host.json");
 
 export let url = "https://localhost:4443/";
-export let controlToken = jwt.sign({ aud: "user", user: "test" }, Buffer.from("TestSecretDoNotUse", "base64"));
+// Signed in the root before hook, as the user has to exist before the token is issued
+export let controlToken;
 export let instancesDir = path.join("temp", "test", "instances");
 export let modsDir = path.join("temp", "test", "mods");
 export let databaseDir = path.join("temp", "test", "database");
@@ -373,6 +374,7 @@ before(async function() {
 	console.log("Bootstrapping");
 	await execController("bootstrap create-admin test");
 	await execController("bootstrap create-ctl-config test");
+	controlToken = jwt.sign({ aud: "user", user: "test" }, Buffer.from("TestSecretDoNotUse", "base64"));
 
 	controllerProcess = await spawnNode("controller:", "../../packages/controller run", /Started controller/);
 

test/integration/link.js

@@ -2,10 +2,11 @@ import assert from "node:assert/strict";
 import events from "node:events";
 import jwt from "jsonwebtoken";
 
-import { TestControlConnector, TestHostConnector, TestControl, get, exec, url, slowTest } from "./index.js";
+import {
+	TestControlConnector, TestHostConnector, TestControl, get, exec, url, controlToken, slowTest,
+} from "./index.js";
 import { ConnectionClosed, ProtocolError, PolicyViolation, AuthenticationFailed } from "@clusterio/lib";
 
-let token = jwt.sign({ aud: "user", user: "test" }, Buffer.from("TestSecretDoNotUse", "base64"));
 let tokenHost = jwt.sign({ aud: "host", host: 0 }, Buffer.from("TestSecretDoNotUse", "base64"));
 
 describe("Integration of lib/link", function() {
@@ -14,7 +15,7 @@ describe("Integration of lib/link", function() {
 	let hostConnector;
 	beforeEach(async function() {
 		controlConnector = new TestControlConnector(url, 0.2);
-		controlConnector.token = token;
+		controlConnector.token = controlToken;
 		control = new TestControl(controlConnector, false);
 		hostConnector = new TestHostConnector(url, 0.2);
 		hostConnector.token = tokenHost;
@@ -100,7 +101,7 @@ describe("Integration of lib/link", function() {
 	});
 
 	it("should refuse connection with incorrect audience", async function() {
-		hostConnector.token = token;
+		hostConnector.token = controlToken;
 		await assert.rejects(
 			hostConnector.connect(),
 			new Error("Authentication failed: jwt audience invalid. expected: host or slave")