Merge pull request #832 from Cooldude2606/refactor/move-ensure-role

Move ensure default roles onto Role class

packages/controller/controller.ts

@@ -155,9 +155,8 @@ async function handleBootstrapCommand(
 			admin = userManager.createUser(args.name);
 		}
 
-		let adminRole = lib.ensureDefaultAdminRole(roles);
-		admin.roleIds.add(adminRole.id);
 		admin.isAdmin = true;
+		admin.roleIds.add(lib.Role.DefaultAdminRoleId);
 		await userManager.save(path.join(controllerConfig.get("controller.database_directory"), "users.json"));
 		await controllerConfigLock.release();
 

packages/controller/src/Controller.ts

@@ -161,8 +161,8 @@ export default class Controller {
 		}
 
 		// Add default roles
-		lib.ensureDefaultAdminRole(roles);
-		lib.ensureDefaultPlayerRole(roles);
+		lib.Role.ensureDefaultAdminRole(roles);
+		lib.Role.ensureDefaultPlayerRole(roles);
 
 		return [
 			systems,

packages/lib/src/data/Role.ts

@@ -1,15 +1,23 @@
 import { Static, Type } from "@sinclair/typebox";
 import { permissions as globalPermissions } from "../permissions";
 
+interface RoleCollection {
+	get(id: number): Role;
+	set(role: Role): void;
+}
+
 /**
  * Represents a collection of granted permissions
  */
 export default class Role {
+	static DefaultAdminRoleId = 0 as const;
+	static DefaultPlayerRoleId = 1 as const;
+
 	constructor(
 		public id: number,
 		public name: string,
 		public description: string,
-		public permissions: Set<string>,
+		public permissions = new Set<string>(),
 		public updatedAtMs = 0,
 		public isDeleted = false,
 	) { }
@@ -45,6 +53,13 @@ export default class Role {
 		};
 	}
 
+	static ensureDefaultPlayerRole(roles: RoleCollection) {
+		const playerRole = roles.get(this.DefaultPlayerRoleId)
+			?? new this(this.DefaultPlayerRoleId, "Player", "Default player role.");
+		playerRole.grantDefaultPermissions();
+		roles.set(playerRole);
+	}
+
 	grantDefaultPermissions() {
 		for (let permission of globalPermissions.values()) {
 			if (permission.grantByDefault) {
@@ -52,4 +67,15 @@ export default class Role {
 			}
 		}
 	}
+
+	static ensureDefaultAdminRole(roles: RoleCollection) {
+		const adminRole = roles.get(this.DefaultAdminRoleId)
+			?? new this(this.DefaultAdminRoleId, "Cluster Admin", "Cluster wide administrator.");
+		adminRole.grantAdminPermissions();
+		roles.set(adminRole);
+	}
+
+	grantAdminPermissions() {
+		this.permissions.add("core.admin");
+	}
 }

packages/lib/src/permissions.ts

@@ -6,9 +6,7 @@
  * @author Hornwitser
  * @module lib/users
  */
-import { Permission, Role } from "./data";
-import { SubscribableDatastore } from "./datastore";
-
+import { Permission } from "./data";
 
 export const permissions = new Map<string, Permission>();
 
@@ -58,54 +56,6 @@ export function definePermission({
 	permissions.set(name, new Permission(name, title, description, grantByDefault));
 }
 
-
-/**
- * Ensure the default admin role has full access
- *
- * Ensures the role with id 0 exisits and has the core.admin permission.
- *
- * @param roles - role storage to modify.
- * @returns the default admin role
- */
-export function ensureDefaultAdminRole(roles: SubscribableDatastore<Role>) {
-	let admin = roles.get(0);
-	if (!admin) {
-		admin = Role.fromJSON({
-			id: 0,
-			name: "Cluster Admin",
-			description: "Cluster wide administrator.",
-			permissions: [],
-		});
-	}
-	admin.permissions.add("core.admin");
-	roles.set(admin);
-	return admin;
-}
-
-/**
- * Ensure the default player role has default accesses
- *
- * Ensures the role with id 1 exists and has all the permissions that are
- * assigned by default.  Note that this may not be the same role as the one
- * that is assigned to new users by default.
- *
- * @param roles - role storage to modify.
- */
-export function ensureDefaultPlayerRole(roles: SubscribableDatastore<Role>) {
-	let player = roles.get(1);
-	if (!player) {
-		player = Role.fromJSON({
-			id: 1,
-			name: "Player",
-			description: "Default player role.",
-			permissions: [],
-		});
-	}
-	player.grantDefaultPermissions();
-	roles.set(player);
-}
-
-
 // Definitions for the built in permissions used in Clusterio.
 // description should answer "this permission allows you to ___"
 definePermission({

test/lib/data/Role.test.js

@@ -0,0 +1,160 @@
+"use strict";
+const assert = require("assert").strict;
+const lib = require("@clusterio/lib");
+
+describe("lib/data/Role", function() {
+	let defaultPermissions;
+	before(function() {
+		// We need to do this here because plugins may add new default permissions
+		defaultPermissions = new Set([...lib.permissions.values()]
+			.filter(permission => permission.grantByDefault)
+			.map(permission => permission.name));
+	});
+
+	describe("class Role", function() {
+		describe("JSON serialization", function() {
+			it("should round trip serialize with default optional values", function() {
+				const role = new lib.Role(11, "Role", "My Role");
+				assert.equal(role.updatedAtMs, 0);
+				assert.equal(role.isDeleted, false);
+
+				const json = role.toJSON();
+				assert.equal(json.updated_at_ms, 0);
+				assert.equal(json.is_deleted, false);
+
+				const copy = lib.Role.fromJSON(json);
+				assert.deepEqual(copy, role);
+			});
+			it("should preserve optional fields once set", function() {
+				const role = new lib.Role(11, "Test", "My Role", new Set(["test"]), 1234, true);
+
+				const json = role.toJSON();
+				assert.deepEqual(json.permissions, ["test"]);
+				assert.equal(json.updated_at_ms, 1234);
+				assert.equal(json.is_deleted, true);
+
+				const copy = lib.Role.fromJSON(json);
+				assert.deepEqual(copy, role);
+			});
+		});
+
+		describe(".grantDefaultPermissions()", function() {
+			it("should only grant permissions marked grantByDefault", function() {
+				const role = new lib.Role(20, "Defaults", "Defaults");
+				role.grantDefaultPermissions();
+
+				assert.deepEqual(
+					role.permissions, defaultPermissions,
+					"Granted default permissions does not match"
+				);
+
+				for (const permName of role.permissions) {
+					const perm = lib.permissions.get(permName);
+					assert(perm, `Permission ${permName} does not exist`);
+					assert.equal(
+						perm.grantByDefault, true,
+						`Permission ${permName} was not grantByDefault`
+					);
+				}
+			});
+		});
+
+		describe(".grantAdminPermissions()", function() {
+			it("should grant core.admin permission", function() {
+				const role = new lib.Role(1, "Admin", "Admin role");
+				role.grantAdminPermissions();
+				assert(role.permissions.has("core.admin"));
+			});
+		});
+
+		describe("static ensureDefaultPlayerRole()", function() {
+			it("should create player role if missing", function() {
+				let storedRole;
+
+				const roles = {
+					get(id) {
+						assert.equal(id, lib.Role.DefaultPlayerRoleId);
+						return undefined;
+					},
+					set(role) {
+						storedRole = role;
+					},
+				};
+
+				lib.Role.ensureDefaultPlayerRole(roles);
+
+				assert(storedRole, "Expected role to be created");
+				assert.equal(storedRole.id, lib.Role.DefaultPlayerRoleId);
+				assert.equal(storedRole.name, "Player");
+				assert.deepEqual(
+					storedRole.permissions, defaultPermissions,
+					"Granted default permissions does not match"
+				);
+			});
+			it("should reuse existing player role", function() {
+				const storedRole = new lib.Role(
+					lib.Role.DefaultPlayerRoleId,
+					"Existing Player",
+					"Existing"
+				);
+
+				const roles = {
+					get() {
+						return storedRole;
+					},
+					set(role) {
+						assert.equal(role, storedRole);
+					},
+				};
+
+				lib.Role.ensureDefaultPlayerRole(roles);
+				assert.deepEqual(
+					storedRole.permissions, defaultPermissions,
+					"Granted default permissions does not match"
+				);
+			});
+		});
+
+		describe("static ensureDefaultAdminRole()", function() {
+			it("should create admin role if missing", function() {
+				let storedRole;
+
+				const roles = {
+					get(id) {
+						assert.equal(id, lib.Role.DefaultAdminRoleId);
+						return undefined;
+					},
+					set(role) {
+						storedRole = role;
+					},
+				};
+
+				lib.Role.ensureDefaultAdminRole(roles);
+
+				assert(storedRole, "Expected admin role to be created");
+				assert.equal(storedRole.id, lib.Role.DefaultAdminRoleId);
+				assert.equal(storedRole.name, "Cluster Admin");
+				assert(storedRole.permissions.has("core.admin"));
+			});
+			it("should reuse existing admin role", function() {
+				const existing = new lib.Role(
+					lib.Role.DefaultAdminRoleId,
+					"Existing Admin",
+					"Existing"
+				);
+
+				const roles = {
+					get() {
+						return existing;
+					},
+					set(role) {
+						assert.equal(role, existing);
+					},
+				};
+
+				lib.Role.ensureDefaultAdminRole(roles);
+				assert(existing.permissions.has("core.admin"));
+			});
+		});
+	});
+});

test/lib/data/Role.ts

@@ -1,28 +0,0 @@
-"use strict";
-const assert = require("assert").strict;
-const lib = require("@clusterio/lib");
-
-
-describe("lib/data/Role", function() {
-	describe("class Role", function() {
-		it("should round trip serialize", function() {
-			let orig = lib.Role.fromJSON({ id: 11, name: "Role", description: "My Role", permissions: ["test"] });
-			let copy = lib.Role.fromJSON(orig.toJSON());
-			assert.deepEqual(copy, orig);
-		});
-
-		describe(".grantDefaultRoles()", function() {
-			it("should only grant permissions with grantByDefault", function() {
-				let role = lib.Role.fromJSON({ id: 11, name: "Role", description: "My Role" });
-				role.grantDefaultPermissions();
-				assert(role.permissions.size > 0, "No permissions were granted");
-				for (let permission of role.permissions) {
-					assert(
-						lib.permissions.get(permission).grantByDefault === true,
-						"Non-default permission granted"
-					);
-				}
-			});
-		});
-	});
-});