From bdf02a5ea260d60d909a1739939a4b84641d4f4d Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Mon, 11 Nov 2024 22:04:57 +0000 Subject: [PATCH] Made user ids case insensitive --- CHANGELOG.md | 1 + packages/controller/controller.ts | 10 ++--- packages/controller/src/ControlConnection.ts | 20 ++++----- packages/controller/src/HostConnection.ts | 8 ++-- packages/controller/src/UserManager.ts | 46 +++++++++++++++----- packages/controller/src/WsServer.ts | 2 +- packages/controller/src/routes.ts | 2 +- packages/lib/src/data/PlayerStats.ts | 15 +++++++ packages/lib/src/data/User.ts | 34 ++++++++++++++- packages/web_ui/src/model/user.tsx | 2 +- plugins/player_auth/controller.ts | 4 +- test/controller/UserManager.js | 2 +- test/controller/routes.js | 2 +- test/integration/clusterio.js | 25 ++++++++--- test/integration/routing.js | 2 +- 15 files changed, 130 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68b00884..5de689e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,7 @@ Many thanks to the following for contributing to this release: - @clusterio/controller export InstanceInfo has added factorioVersion parameter. - Argument order for `Config.canAccess` changed to require an access mode passed as the second argument. - Instances can now have `factorio.enable_script_commands` disabled which will throw an error when any script command is used over rcon. [#681](https://github.com/clusterio/clusterio/pull/681). +- User IDs are now case insensitive, duplicate users will be automatically merged with a backup created. [#682](https://github.com/clusterio/clusterio/pull/682) Many thanks to the following for contributing to this release: [@CCpersonguy](https://github.com/CCpersonguy) diff --git a/packages/controller/controller.ts b/packages/controller/controller.ts index b4b9d4e2..d9bdaf9c 100644 --- a/packages/controller/controller.ts +++ b/packages/controller/controller.ts @@ -139,7 +139,7 @@ async function handleBootstrapCommand( return; } - let admin = userManager.users.get(args.name); + let admin = userManager.getByName(args.name); if (!admin) { admin = userManager.createUser(args.name); } @@ -150,14 +150,14 @@ async function handleBootstrapCommand( await userManager.save(path.join(controllerConfig.get("controller.database_directory"), "users.json")); } else if (subCommand === "generate-user-token") { - let user = userManager.users.get(args.name); + let user = userManager.getByName(args.name); if (!user) { logger.error(`No user named '${args.name}'`); process.exitCode = 1; return; } // eslint-disable-next-line no-console - console.log(userManager.signUserToken(user.name)); + console.log(userManager.signUserToken(user)); } else if (subCommand === "generate-host-token") { // eslint-disable-next-line no-console @@ -167,7 +167,7 @@ async function handleBootstrapCommand( )); } else if (subCommand === "create-ctl-config") { - let admin = userManager.users.get(args.name); + let admin = userManager.getByName(args.name); if (!admin) { logger.error(`No user named '${args.name}'`); process.exitCode = 1; @@ -178,7 +178,7 @@ async function handleBootstrapCommand( controlConfig.set("control.controller_url", Controller.calculateControllerUrl(controllerConfig)); controlConfig.set( "control.controller_token", - userManager.signUserToken(admin.name), + userManager.signUserToken(admin), ); let content = JSON.stringify(controlConfig, null, "\t"); diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index ba59deaf..8ef23516 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -134,7 +134,7 @@ export default class ControlConnection extends BaseConnection { this.checkPermission(message, entry); } catch (err: any) { this.connector.sendResponseError(new lib.ResponseError(err.message, err.code), message.src); - logger.audit(`Permission denied for ${message.name} by ${this.user.name} from ${this.connector.dst}`); + logger.audit(`Permission denied for ${message.name} by ${this.user.id} from ${this.connector.dst}`); throw err; } } @@ -626,7 +626,7 @@ export default class ControlConnection extends BaseConnection { async handleUserGetRequest(request: lib.UserGetRequest): Promise { let name = request.name; - let user = this._controller.userManager.users.get(name); + let user = this._controller.userManager.getByName(name); if (!user) { throw new lib.RequestError(`User ${name} does not exist`); } @@ -644,17 +644,17 @@ export default class ControlConnection extends BaseConnection { } async handleUserRevokeTokenRequest(request: lib.UserRevokeTokenRequest) { - let user = this._controller.userManager.users.get(request.name); + let user = this._controller.userManager.getByName(request.name); if (!user) { throw new lib.RequestError(`User '${request.name}' does not exist`); } - if (user.name !== this.user.name) { + if (user.id !== this.user.id) { this.user.checkPermission("core.user.revoke_other_token"); } user.invalidateToken(); for (let controlConnection of this._controller.wsServer.controlConnections.values()) { - if (controlConnection.user.name === user.name) { + if (controlConnection.user.id === user.id) { controlConnection.connector.terminate(); } } @@ -662,7 +662,7 @@ export default class ControlConnection extends BaseConnection { } async handleUserUpdateRolesRequest(request: lib.UserUpdateRolesRequest) { - let user = this._controller.userManager.users.get(request.name); + let user = this._controller.userManager.getByName(request.name); if (!user) { throw new lib.RequestError(`User '${request.name}' does not exist`); } @@ -680,7 +680,7 @@ export default class ControlConnection extends BaseConnection { async handleUserSetAdminRequest(request: lib.UserSetAdminRequest) { let { name, create, admin } = request; - let user = this._controller.userManager.users.get(name); + let user = this._controller.userManager.getByName(name); if (!user) { if (create) { this.user.checkPermission("core.user.create"); @@ -697,7 +697,7 @@ export default class ControlConnection extends BaseConnection { async handleUserSetBannedRequest(request: lib.UserSetBannedRequest) { let { name, create, banned, reason } = request; - let user = this._controller.userManager.users.get(name); + let user = this._controller.userManager.getByName(name); if (!user) { if (create) { this.user.checkPermission("core.user.create"); @@ -715,7 +715,7 @@ export default class ControlConnection extends BaseConnection { async handleUserSetWhitelistedRequest(request: lib.UserSetWhitelistedRequest) { let { name, create, whitelisted } = request; - let user = this._controller.userManager.users.get(name); + let user = this._controller.userManager.getByName(name); if (!user) { if (create) { this.user.checkPermission("core.user.create"); @@ -732,7 +732,7 @@ export default class ControlConnection extends BaseConnection { async handleUserDeleteRequest(request: lib.UserDeleteRequest) { let name = request.name; - let user = this._controller.userManager.users.get(name); + let user = this._controller.userManager.getByName(name); if (!user) { throw new lib.RequestError(`User '${name}' does not exist`); } diff --git a/packages/controller/src/HostConnection.ts b/packages/controller/src/HostConnection.ts index 6da8e93c..3b2155f1 100644 --- a/packages/controller/src/HostConnection.ts +++ b/packages/controller/src/HostConnection.ts @@ -279,13 +279,13 @@ export default class HostConnection extends BaseConnection { for (let user of this._controller.userManager.users.values()) { if (user.isAdmin) { - adminlist.add(user.name); + adminlist.add(user.id); } if (user.isBanned) { - banlist.set(user.name, user.banReason); + banlist.set(user.id, user.banReason); } if (user.isWhitelisted) { - whitelist.add(user.name); + whitelist.add(user.id); } } @@ -326,7 +326,7 @@ export default class HostConnection extends BaseConnection { async handleInstancePlayerUpdateEvent(event: lib.InstancePlayerUpdateEvent, src: lib.Address) { let instanceId = src.id; - let user = this._controller.userManager.users.get(event.name); + let user = this._controller.userManager.getByName(event.name); if (!user) { user = this._controller.userManager.createUser(event.name); } diff --git a/packages/controller/src/UserManager.ts b/packages/controller/src/UserManager.ts index e9239428..10d5787f 100644 --- a/packages/controller/src/UserManager.ts +++ b/packages/controller/src/UserManager.ts @@ -10,8 +10,8 @@ import ControllerUser from "./ControllerUser"; * @alias module:controller/src/UserManager */ export default class UserManager { - roles: Map = new Map(); - users: Map = new Map(); + roles: Map = new Map(); + users: Map = new Map(); dirty = false; /** @@ -24,6 +24,10 @@ export default class UserManager { ) { } + getByName(name: string) { + return this.users.get(name.toLowerCase()); + } + async load(filePath: string): Promise { try { let content = JSON.parse(await fs.readFile(filePath, { encoding: "utf8" })); @@ -32,9 +36,29 @@ export default class UserManager { this.roles.set(role.id, role); } + let duplicates = 0; for (let serializedUser of content.users) { let user = ControllerUser.fromJSON(serializedUser, this); - this.users.set(user.name, user); + const existingUser = this.users.get(user.id); + if (existingUser) { + // Required migration to all lowercase ids in alpha 19 + duplicates += 1; + // We assume the user with the lower online time is the duplicate + if (user.playerStats.onlineTimeMs <= existingUser.playerStats.onlineTimeMs) { + existingUser.merge(user); + continue; // Skip users.set + } + user.merge(existingUser); + } + this.users.set(user.id, user); + } + + if (duplicates) { + const backupPath = `${filePath}.${Date.now()}.bak`; + lib.logger.warn( + `A total of ${duplicates} users were merged, a backup was written to: ${backupPath}` + ); + await lib.safeOutputFile(backupPath, content); } } catch (err: any) { @@ -48,7 +72,7 @@ export default class UserManager { } } - async save(filePath:string): Promise { + async save(filePath: string): Promise { let serializedRoles = []; for (let role of this.roles.values()) { serializedRoles.push(role.toJSON()); @@ -72,8 +96,8 @@ export default class UserManager { * @param name - Name of the user to create. * @returns The created user. */ - createUser(name:string): ControllerUser { - if (this.users.has(name)) { + createUser(name: string): ControllerUser { + if (this.getByName(name)) { throw new Error(`User '${name}' already exists`); } @@ -84,20 +108,20 @@ export default class UserManager { } let user = new ControllerUser(this, 0, name, roles); - this.users.set(name, user); + this.users.set(user.id, user); this.dirty = true; return user; } /** - * Sign access token for the given user name + * Sign access token for the given user * - * @param name - user name to sign token for + * @param user - user to sign token for * @returns JWT access token for the user. */ - signUserToken(name: string): string { + signUserToken(user: ControllerUser): string { return jwt.sign( - { aud: "user", user: name }, + { aud: "user", user: user.id }, Buffer.from(this._config.get("controller.auth_secret"), "base64") ); } diff --git a/packages/controller/src/WsServer.ts b/packages/controller/src/WsServer.ts index 9def1c50..c303eb10 100644 --- a/packages/controller/src/WsServer.ts +++ b/packages/controller/src/WsServer.ts @@ -327,7 +327,7 @@ ${err.stack}` throw new Error("unexpected JsonWebToken type"); } - user = this.controller.userManager.users.get(tokenPayload.user); + user = this.controller.userManager.getByName(tokenPayload.user); if (!user) { throw new Error("invalid user"); } diff --git a/packages/controller/src/routes.ts b/packages/controller/src/routes.ts index 068bf3e5..1e2b3b1f 100644 --- a/packages/controller/src/routes.ts +++ b/packages/controller/src/routes.ts @@ -185,7 +185,7 @@ function validateUserToken(req: Request, res: Response, next: any) { if (typeof tokenPayload === "string") { throw new Error("unexpected JsonWebToken type"); } - let user = req.app.locals.controller.userManager.users.get(tokenPayload.user); + let user = req.app.locals.controller.userManager.getByName(tokenPayload.user); if (!user) { throw new Error("invalid user"); } diff --git a/packages/lib/src/data/PlayerStats.ts b/packages/lib/src/data/PlayerStats.ts index 61ca3aed..fd306ca3 100644 --- a/packages/lib/src/data/PlayerStats.ts +++ b/packages/lib/src/data/PlayerStats.ts @@ -92,4 +92,19 @@ export default class PlayerStats { } return json; } + + merge(other: PlayerStats) { + this.joinCount += other.joinCount; + this.onlineTimeMs += other.onlineTimeMs; + if (!this.firstJoinAt || (other.firstJoinAt && other.firstJoinAt > this.firstJoinAt)) { + this.firstJoinAt = other.firstJoinAt; + } + if (!this.lastJoinAt || (other.lastJoinAt && other.lastJoinAt > this.lastJoinAt)) { + this.lastJoinAt = other.lastJoinAt; + } + if (!this.lastLeaveAt || (other.lastLeaveAt && other.lastLeaveAt > this.lastLeaveAt)) { + this.lastLeaveAt = other.lastLeaveAt; + this.lastLeaveReason = other.lastLeaveReason; + } + } } diff --git a/packages/lib/src/data/User.ts b/packages/lib/src/data/User.ts index 9cae6fde..2aa63766 100644 --- a/packages/lib/src/data/User.ts +++ b/packages/lib/src/data/User.ts @@ -150,9 +150,39 @@ export default class User { return playerStats; } - /** Name of the user, used by event subscriptions */ + /** + * Merge another user into this one, must have matching ids, user is not deleted + * @param otherUser - User who's details are merged from + */ + merge(otherUser: User) { + if (this.id !== otherUser.id) { + throw new Error("Cannot merge users with different ids"); + } + + // Merge properties + this.roleIds = new Set([...this.roleIds, ...otherUser.roleIds]); + this.instances = new Set([...this.instances, ...otherUser.instances]); + this.isAdmin = this.isAdmin && otherUser.isAdmin; // More secure to use && rather || + this.isBanned = this.isBanned || otherUser.isBanned; + this.isBanned = this.isWhitelisted || otherUser.isWhitelisted; + this.banReason = this.banReason.length > otherUser.banReason.length ? this.banReason : otherUser.banReason; + this.updatedAtMs = Date.now(); + + // Merge instance stats + const thisInstanceStats = this.instanceStats; + for (const [instanceId, instanceStats] of otherUser.instanceStats.entries()) { + if (thisInstanceStats.has(instanceId)) { + thisInstanceStats.get(instanceId)!.merge(instanceStats); + } else { + thisInstanceStats.set(instanceId, instanceStats); + } + } + this.recalculatePlayerStats(); + } + + /** Name of the user, used by event subscriptions and to ensure case insensitivity */ get id() { - return this.name; + return this.name.toLowerCase(); } } diff --git a/packages/web_ui/src/model/user.tsx b/packages/web_ui/src/model/user.tsx index 69536cf3..981a57e0 100644 --- a/packages/web_ui/src/model/user.tsx +++ b/packages/web_ui/src/model/user.tsx @@ -73,7 +73,7 @@ export function sortLastSeen(userA: lib.User, userB: lib.User, instanceIdA?: num export function useUser(name?: string) { const [users, synced] = useUsers(); - return [name !== undefined ? users.get(name) : undefined, synced] as const; + return [name !== undefined ? users.get(name.toLowerCase()) : undefined, synced] as const; } export function useUsers() { diff --git a/plugins/player_auth/controller.ts b/plugins/player_auth/controller.ts index 77832ee7..01a69aef 100644 --- a/plugins/player_auth/controller.ts +++ b/plugins/player_auth/controller.ts @@ -152,13 +152,13 @@ export class ControllerPlugin extends BaseControllerPlugin { for (let [player, entry] of this.players) { if (entry.playerCode === playerCode && entry.expiresMs > Date.now()) { if (entry.verifyCode === verifyCode) { - let user = this.controller.userManager.users.get(player); + let user = this.controller.userManager.getByName(player); if (!user) { res.send({ error: true, message: "invalid user" }); return; } - let token = this.controller.userManager.signUserToken(user.name); + let token = this.controller.userManager.signUserToken(user); res.send({ verified: true, token }); return; diff --git a/test/controller/UserManager.js b/test/controller/UserManager.js index a25067c3..392b3669 100644 --- a/test/controller/UserManager.js +++ b/test/controller/UserManager.js @@ -8,7 +8,7 @@ describe("controller/src/UserManager", function() { const userManager = new UserManager({}); it("should track online users", function() { let user = ControllerUser.fromJSON({ name: "admin", roles: [1] }); - userManager.users.set(user.name, user); + userManager.users.set(user.id, user); assert(!userManager.onlineUsers.has(user)); assert.deepEqual(user.instances, new Set()); diff --git a/test/controller/routes.js b/test/controller/routes.js index e69449ca..b2d5d07d 100644 --- a/test/controller/routes.js +++ b/test/controller/routes.js @@ -104,7 +104,7 @@ describe("controller/src/routes", function() { data: "totally a zip file", }); assert.equal(response.statusCode, 401); - controller.userManager.users.get("test").tokenValidAfter = Math.floor((Date.now() + 60e3) / 1000); + controller.userManager.getByName("test").tokenValidAfter = Math.floor((Date.now() + 60e3) / 1000); response = await phin({ url: `http://localhost:${port}/api/upload-save?instance_id=123&filename=file.zip`, method: "POST", headers: { diff --git a/test/integration/clusterio.js b/test/integration/clusterio.js index 9b35c4f4..1a0702a5 100644 --- a/test/integration/clusterio.js +++ b/test/integration/clusterio.js @@ -97,7 +97,7 @@ async function deleteSave(instanceId, save) { async function getUsers() { let users = await getControl().send(new lib.UserListRequest()); - return new Map(users.map(user => [user.name, user])); + return new Map(users.map(user => [user.id, user])); } function jsonArg(value) { @@ -663,6 +663,11 @@ describe("Integration of Clusterio", function() { const removeCommandStateUpper = await sendRcon(44, "/banlist get test_RCON_ban"); assert.equal(removeCommandStateLower, "test_rcon_ban is not banned.\n", "User is banned"); assert.equal(removeCommandStateUpper, "test_RCON_ban is not banned.\n", "User is banned"); + + // Check it is case insensitive by checking it was converted correctly + const users = await getUsers(); + assert(users.has("test_rcon_ban"), "User ID was not lowercase"); + assert(!users.has("test_RCON_ban"), "Username was not converted to User ID"); }); it("should send whitelist commands to running instances", async function() { slowTest(this); @@ -691,6 +696,11 @@ describe("Integration of Clusterio", function() { "test_rcon_whitelist is not whitelisted.\n", "User is whitelisted"); assert.equal(removeCommandStateUpper, "test_RCON_whitelist is not whitelisted.\n", "User is whitelisted"); + + // Check it is case insensitive by checking it was converted correctly + const users = await getUsers(); + assert(users.has("test_rcon_whitelist"), "User ID was not lowercase"); + assert(!users.has("test_RCON_whitelist"), "Username was not converted to User ID"); }); it("should send admin list commands to running instances", async function() { this.skip(); // It is not currently possible to get admin state without the player joining the game @@ -714,6 +724,11 @@ describe("Integration of Clusterio", function() { const removeCommandStateUpper = await sendRcon(44, "/admins get test_RCON_admin"); assert.equal(removeCommandStateLower, "test_rcon_admin is not admin.\n", "User is admin"); assert.equal(removeCommandStateUpper, "test_RCON_admin is not admin.\n", "User is admin"); + + // Check it is case insensitive by checking it was converted correctly + const users = await getUsers(); + assert(users.has("test_rcon_admin"), "User ID was not lowercase"); + assert(!users.has("test_RCON_admin"), "Username was not converted to User ID"); }); }); @@ -1278,10 +1293,10 @@ describe("Integration of Clusterio", function() { getControl().userUpdates = []; await execCtl("user create temp"); let users = await getControl().send(new lib.UserListRequest()); - let tempUser = users.find(user => user.name === "temp"); + let tempUser = users.find(user => user.id === "temp"); assert(tempUser, "user was not created"); assert.equal(getControl().userUpdates.length, 1); - assert.equal(getControl().userUpdates[0].name, "temp"); + assert.equal(getControl().userUpdates[0].id, "temp"); }); }); @@ -1310,7 +1325,7 @@ describe("Integration of Clusterio", function() { getControl().userUpdates = []; await execCtl('user set-roles temp "Cluster Admin"'); let users = await getControl().send(new lib.UserListRequest()); - let tempUser = users.find(user => user.name === "temp"); + let tempUser = users.find(user => user.id === "temp"); assert.deepEqual(tempUser.roleIds, new Set([0])); assert.equal(getControl().userUpdates.length, 1); }); @@ -1346,7 +1361,7 @@ describe("Integration of Clusterio", function() { getControl().userUpdates = []; await execCtl("user delete temp"); let users = await getControl().send(new lib.UserListRequest()); - let tempUser = users.find(user => user.name === "temp"); + let tempUser = users.find(user => user.id === "temp"); assert(!tempUser, "user was note deleted"); assert.equal(getControl().userUpdates.length, 1); assert.equal(getControl().userUpdates[0].isDeleted, true); diff --git a/test/integration/routing.js b/test/integration/routing.js index 3b423786..f2b17d34 100644 --- a/test/integration/routing.js +++ b/test/integration/routing.js @@ -53,7 +53,7 @@ function connectInstance(controller, host, instanceId) { function connectControl(controller, controlId) { const [controllerSide, controlSide] = lib.VirtualConnector.makePair(addr("controller"), addr({ controlId })); const registerData = new lib.RegisterControlData("", "0.0.0"); - let user = controller.userManager.users.get("test"); + let user = controller.userManager.getByName("test"); if (!user) { user = controller.userManager.createUser("test"); }