Refactor user and instance manager (#837)

* Refactor UserManager to use Datastore

* Rename lib.User to UserDetails also adds to/fromUserDetails

* Fix existing tests

* Add user class tests

* Add tests for finaliseUsers

* Move HostDetails to own file with tests

* Move InstanceDetails to its own file with tests

* Rename HostInfo and add tests

* Rename InstanceInfo and add tests

- Also changes the names of the host properties related to instances
- Also removes the need to specify the location of InstanceInfo

* Make json round trip test validate schema

* Add InstanceManager

* Fix lint errors

* Extra tests

* Add final test coverage

* Add one last test to InstanceManager

* Response to comments

* Use generator rather than return anonumous generator

Co-authored-by: Hornwitser <github@hornwitser.no>

* Fix lint issue with generator

---------

Co-authored-by: Hornwitser <github@hornwitser.no>

packages/controller/src/ControlConnection.ts

@@ -6,7 +6,8 @@ import * as lib from "@clusterio/lib";
 const { logFilter, logger } = lib;
 
 import BaseConnection from "./BaseConnection";
-import ControllerUser from "./ControllerUser";
+import User from "./User";
+import UserRecord from "./UserRecord";
 import * as routes from "./routes";
 import Controller from "./Controller";
 
@@ -40,7 +41,7 @@ export default class ControlConnection extends BaseConnection {
 		registerData: { version: string },
 		connector: WsServerConnector,
 		controller: Controller,
-		public user: ControllerUser, // The user making this connection.
+		public user: User, // The user making this connection.
 		public id: number
 	) {
 		super(connector, controller);
@@ -250,7 +251,7 @@ export default class ControlConnection extends BaseConnection {
 	}
 
 	async handleInstanceDetailsGetRequest(request: lib.InstanceDetailsGetRequest) {
-		return this._controller.getRequestInstance(request.instanceId).toInstanceDetails();
+		return this._controller.instances.getForRequest(request.instanceId).toInstanceDetails();
 	}
 
 	async handleInstanceDetailsListRequest() {
@@ -268,20 +269,20 @@ export default class ControlConnection extends BaseConnection {
 			instanceConfig.set("instance.assigned_host", null); // New instances are unassigned
 		}
 		instanceConfig.update(request.config, false, "control");
-		await this._controller.instanceCreate(instanceConfig);
+		await this._controller.instances.createInstance(instanceConfig);
 	}
 
 	async handleInstanceDeleteRequest(request: lib.InstanceDeleteRequest) {
-		await this._controller.instanceDelete(request.instanceId);
+		await this._controller.instances.deleteInstance(request.instanceId);
 	}
 
 	async handleInstanceConfigGetRequest(request: lib.InstanceConfigGetRequest) {
-		let instance = this._controller.getRequestInstance(request.instanceId);
+		let instance = this._controller.instances.getForRequest(request.instanceId);
 		return instance.config.toRemote("control");
 	}
 
 	async handleInstanceConfigSetFieldRequest(request: lib.InstanceConfigSetFieldRequest) {
-		let instance = this._controller.getRequestInstance(request.instanceId);
+		let instance = this._controller.instances.getForRequest(request.instanceId);
 		if (request.field === "instance.assigned_host") {
 			throw new lib.RequestError("instance.assigned_host must be set through the assign-host interface");
 		}
@@ -296,14 +297,14 @@ export default class ControlConnection extends BaseConnection {
 	}
 
 	async handleInstanceConfigSetPropRequest(request: lib.InstanceConfigSetPropRequest) {
-		let instance = this._controller.getRequestInstance(request.instanceId);
+		let instance = this._controller.instances.getForRequest(request.instanceId);
 		let { field, prop, value } = request;
 		instance.config.setProp(field as keyof lib.InstanceConfigFields, prop, value, "control");
 		await this._controller.instanceConfigUpdated(instance);
 	}
 
 	async handleInstanceAssignRequest(request: lib.InstanceAssignRequest) {
-		await this._controller.instanceAssign(request.instanceId, request.hostId);
+		await this._controller.instances.assignInstance(request.instanceId, request.hostId ?? null);
 	}
 
 	async handleInstanceSaveDetailsListRequest() {
@@ -337,8 +338,8 @@ export default class ControlConnection extends BaseConnection {
 		if (request.sourceInstanceId === request.targetInstanceId) {
 			throw new lib.RequestError("Source and target instance may not be the same");
 		}
-		let sourceInstance = this._controller.getRequestInstance(request.sourceInstanceId);
-		let targetInstance = this._controller.getRequestInstance(request.targetInstanceId);
+		let sourceInstance = this._controller.instances.getForRequest(request.sourceInstanceId);
+		let targetInstance = this._controller.instances.getForRequest(request.targetInstanceId);
 		let sourceHostId = sourceInstance.config.get("instance.assigned_host");
 		let targetHostId = targetInstance.config.get("instance.assigned_host");
 		if (sourceHostId === null) {
@@ -870,35 +871,33 @@ export default class ControlConnection extends BaseConnection {
 		}
 		this._controller.roles.delete(role);
 
-		this._controller.userManager.dirty = true;
-		for (let user of this._controller.userManager.users.values()) {
-			if (user.roleIds.delete(id)) {
+		for (const user of this._controller.users.values()) {
+			if (user.removeRole(id)) {
 				this._controller.userPermissionsUpdated(user);
 			}
 		}
 	}
 
-	async handleUserGetRequest(request: lib.UserGetRequest): Promise<lib.User> {
+	async handleUserGetRequest(request: lib.UserGetRequest): Promise<lib.UserDetails> {
 		let name = request.name;
-		let user = this._controller.userManager.getByName(name);
+		let user = this._controller.users.getByName(name);
 		if (!user) {
 			throw new lib.RequestError(`User ${name} does not exist`);
 		}
 
-		return user;
+		return user.toUserDetails();
 	}
 
-	async handleUserListRequest(): Promise<lib.User[]> {
-		return [...this._controller.userManager.users.values()];
+	async handleUserListRequest(): Promise<lib.UserDetails[]> {
+		return [...this._controller.users.records.values()].map(user => user.toUserDetails());
 	}
 
 	async handleUserCreateRequest(request: lib.UserCreateRequest) {
-		let user = this._controller.userManager.createUser(request.name);
-		this._controller.usersUpdated([user]);
+		this._controller.users.createUser(request.name);
 	}
 
 	async handleUserRevokeTokenRequest(request: lib.UserRevokeTokenRequest) {
-		let user = this._controller.userManager.getByName(request.name);
+		let user = this._controller.users.getByName(request.name);
 		if (!user) {
 			throw new lib.RequestError(`User '${request.name}' does not exist`);
 		}
@@ -912,11 +911,10 @@ export default class ControlConnection extends BaseConnection {
 				controlConnection.connector.terminate();
 			}
 		}
-		this._controller.usersUpdated([user]);
 	}
 
 	async handleUserUpdateRolesRequest(request: lib.UserUpdateRolesRequest) {
-		let user = this._controller.userManager.getByName(request.name);
+		let user = this._controller.users.getByName(request.name);
 		if (!user) {
 			throw new lib.RequestError(`User '${request.name}' does not exist`);
 		}
@@ -927,35 +925,33 @@ export default class ControlConnection extends BaseConnection {
 			}
 		}
 
-		user.roleIds = new Set(request.roles);
+		user.set("roleIds", new Set(request.roles));
 		this._controller.userPermissionsUpdated(user);
-		this._controller.usersUpdated([user]);
 	}
 
 	async handleUserSetAdminRequest(request: lib.UserSetAdminRequest) {
 		let { name, create, admin } = request;
-		let user = this._controller.userManager.getByName(name);
+		let user = this._controller.users.getByName(name);
 		if (!user) {
 			if (create) {
 				this.user.checkPermission("core.user.create");
-				user = this._controller.userManager.createUser(name);
+				user = this._controller.users.createUser(name);
 			} else {
 				throw new lib.RequestError(`User '${name}' does not exist`);
 			}
 		}
 
-		user.isAdmin = admin;
-		this._controller.usersUpdated([user]);
+		user.set("isAdmin", admin);
 		this._controller.sendTo("allInstances", new lib.InstanceAdminlistUpdateEvent(name, admin));
 	}
 
 	async handleUserSetBannedRequest(request: lib.UserSetBannedRequest) {
 		let { name, create, banned, reason } = request;
-		let user = this._controller.userManager.getByName(name);
+		let user = this._controller.users.getByNameMutable(name);
 		if (!user) {
 			if (create) {
 				this.user.checkPermission("core.user.create");
-				user = this._controller.userManager.createUser(name);
+				user = this._controller.users.createUser(name);
 			} else {
 				throw new lib.RequestError(`User '${name}' does not exist`);
 			}
@@ -963,37 +959,34 @@ export default class ControlConnection extends BaseConnection {
 
 		user.isBanned = banned;
 		user.banReason = reason;
-		this._controller.usersUpdated([user]);
+		user.saveRecord();
 		this._controller.sendTo("allInstances", new lib.InstanceBanlistUpdateEvent(name, banned, reason));
 	}
 
 	async handleUserSetWhitelistedRequest(request: lib.UserSetWhitelistedRequest) {
 		let { name, create, whitelisted } = request;
-		let user = this._controller.userManager.getByName(name);
+		let user = this._controller.users.getByName(name);
 		if (!user) {
 			if (create) {
 				this.user.checkPermission("core.user.create");
-				user = this._controller.userManager.createUser(name);
+				user = this._controller.users.createUser(name);
 			} else {
 				throw new lib.RequestError(`User '${name}' does not exist`);
 			}
 		}
 
-		user.isWhitelisted = whitelisted;
-		this._controller.usersUpdated([user]);
+		user.set("isWhitelisted", whitelisted);
 		this._controller.sendTo("allInstances", new lib.InstanceWhitelistUpdateEvent(name, whitelisted));
 	}
 
 	async handleUserDeleteRequest(request: lib.UserDeleteRequest) {
 		let name = request.name;
-		let user = this._controller.userManager.getByName(name);
+		let user = this._controller.users.getByName(name);
 		if (!user) {
 			throw new lib.RequestError(`User '${name}' does not exist`);
 		}
 
-		user.isDeleted = true;
-		this._controller.userManager.users.delete(name);
-		this._controller.usersUpdated([user]);
+		this._controller.users.deleteUser(user);
 
 		if (user.isAdmin) {
 			this._controller.sendTo("allInstances", new lib.InstanceAdminlistUpdateEvent(name, false));
@@ -1006,14 +999,15 @@ export default class ControlConnection extends BaseConnection {
 		}
 	}
 
-	async handleUserBulkRestoreRequest(request: lib.UserBulkImportRequest, updated: Map<string, ControllerUser>) {
+	/** This does not do a full restore, rather it demotes / unbanns / unwhitelistes users */
+	async handleUserBulkRestoreRequest(request: lib.UserBulkImportRequest, updated: Map<string, UserRecord>) {
 		if (request.importType === "users") {
 			// Update all fields for all users
 			const users = request.users as Extract<typeof request.users, { username: string }[]>;
 			const admin = new Set(users.filter(u => u.is_admin).map(u => u.username.toLowerCase()));
 			const banned = new Set(users.filter(u => u.is_banned).map(u => u.username.toLowerCase()));
 			const whitelist = new Set(users.filter(u => u.is_whitelisted).map(u => u.username.toLowerCase()));
-			for (const user of this._controller.userManager.users.values()) {
+			for (const user of this._controller.users.valuesMutable()) {
 				if (user.isAdmin && !admin.has(user.name)) {
 					user.isAdmin = false;
 					updated.set(user.id, user);
@@ -1034,7 +1028,7 @@ export default class ControlConnection extends BaseConnection {
 			const expected = new Set(
 				users.map(u => (typeof u === "string" ? u.toLowerCase() : u.username.toLowerCase()))
 			);
-			for (const user of this._controller.userManager.users.values()) {
+			for (const user of this._controller.users.valuesMutable()) {
 				if (user.isBanned && !expected.has(user.name)) {
 					user.isBanned = false;
 					user.banReason = "";
@@ -1046,7 +1040,7 @@ export default class ControlConnection extends BaseConnection {
 			const users = request.users as string[];
 			const expected = new Set(users.map(u => u.toLowerCase()));
 			const prop = request.importType === "admins" ? "isAdmin" : "isWhitelisted";
-			for (const user of this._controller.userManager.users.values()) {
+			for (const user of this._controller.users.valuesMutable()) {
 				if (user[prop] && !expected.has(user.name)) {
 					user[prop] = false;
 					updated.set(user.id, user);
@@ -1055,11 +1049,9 @@ export default class ControlConnection extends BaseConnection {
 		}
 	}
 
-	// A large number of small if statements are unavoidable when reducing the number of updates sent
-
 	async handleUserBulkImportRequest(request: lib.UserBulkImportRequest) {
 		let backup: undefined | Awaited<ReturnType<ControlConnection["handleUserBulkExportRequest"]>>;
-		const updated = new Map<ControllerUser["id"], ControllerUser>();
+		const updated = new Map<User["id"], User>();
 		if (request.restore) {
 			// Unban / Demote / Unwhitelist players not on the list
 			backup = await this.handleUserBulkExportRequest(new lib.UserBulkExportRequest(request.importType));
@@ -1067,11 +1059,12 @@ export default class ControlConnection extends BaseConnection {
 		}
 
 		// Will get a user or attempt to create one
+		// UserManager.getOrCreateUser not used due to inclusion of checkPermission
 		const getUserOrCreate = (username: string) => {
-			const user = this._controller.userManager.getByName(username);
-			if (user) { return user; }
+			const user = this._controller.users.getByNameMutable(username);
+			if (user) { return updated.get(user.id) ?? user; }
 			this.user.checkPermission("core.user.create");
-			return this._controller.userManager.createUser(username);
+			return this._controller.users.createUser(username);
 		};
 
 		// Merge the imported data with existing data, this is strictly additive
@@ -1129,14 +1122,14 @@ export default class ControlConnection extends BaseConnection {
 
 		// Send the necessary update events
 		const updatedUsers = [...updated.values()];
-		this._controller.usersUpdated(updatedUsers);
+		this._controller.users.records.setMany(updatedUsers);
 
 		// Resync the host user lists
 		const adminlist: Set<string> = new Set();
 		const banlist: Map<string, string> = new Map();
 		const whitelist: Set<string> = new Set();
 
-		for (let user of this._controller.userManager.users.values()) {
+		for (let user of this._controller.users.values()) {
 			if (user.isAdmin) {
 				adminlist.add(user.id);
 			}
@@ -1160,7 +1153,7 @@ export default class ControlConnection extends BaseConnection {
 		if (request.exportType === "users") {
 			// Send a full user export
 			const usersToSend = [] as lib.ClusterioUserExport["users"];
-			for (const user of this._controller.userManager.users.values()) {
+			for (const user of this._controller.users.values()) {
 				let send = false;
 				const userToSend = { username: user.name } as typeof usersToSend[0];
 				if (user.isBanned) {
@@ -1186,7 +1179,7 @@ export default class ControlConnection extends BaseConnection {
 		} else if (request.exportType === "bans") {
 			// Bans have extra logic to handle ban reason
 			const usersToSend = [] as Array<string | { username: string, reason: string }>;
-			for (const user of this._controller.userManager.users.values()) {
+			for (const user of this._controller.users.values()) {
 				if (user.isBanned) {
 					if (user.banReason && user.banReason !== "") {
 						usersToSend.push({ username: user.name, reason: user.banReason });
@@ -1201,7 +1194,7 @@ export default class ControlConnection extends BaseConnection {
 		// This is a whitelist or admins export
 		const usersToSend = [] as string[];
 		const prop = request.exportType === "admins" ? "isAdmin" : "isWhitelisted";
-		for (const user of this._controller.userManager.users.values()) {
+		for (const user of this._controller.users.values()) {
 			if (user[prop]) {
 				usersToSend.push(user.name);
 			}