Enforce whitelist and add command - #3467
Conversation
Wunka
left a comment
Wunka
left a comment
There was a problem hiding this comment.
Thanks for continuing with this!
| const keys = zon.getChild("keys"); | ||
| try conn.user.?.identifyFromKeysAndName(name, keys); | ||
|
|
||
| if (main.server.world.?.settings.whitelistEnabled and !main.server.players.isAllowedToJoin(conn.user.?.newKeyString.?)) { |
There was a problem hiding this comment.
I don't like this. In my mind this can perfectly work in sync with a ban system (blocked = banned)
I would suggest to go in the direction I went with permissions. Instead of returning in isAllowedToJoin a bool return an enum: {allowed, neutral, blocked} neutral would then be the case where you need to look if whitelist is enabled
| .alreadyAllowed => source.sendMessage("#ff0000{s}§#ff0000 is already on the whitelist", .{key}), | ||
| }, | ||
| .block => switch (players.block(key)) { | ||
| .blocked => source.sendMessage("#00ff00Blocked {s}§#00ff00 from connecting", .{key}), |
There was a problem hiding this comment.
If we go by the principle that blocked = ban then I would suggest to also kick the player here
| } | ||
| } | ||
|
|
||
| const KeyString = struct { |
There was a problem hiding this comment.
In general we put these kinds of helper in command.zig I can already imagine there being a command to for example update the key where this would help
|
True, it does make more sense to treat an explicit block as a ban |
3a68e6c to
49bd0fd
Compare
| const keys = zon.getChild("keys"); | ||
| try conn.user.?.identifyFromKeysAndName(name, keys); | ||
|
|
||
| switch (main.server.players.isAllowedToJoin(conn.user.?.newKeyString.?)) { |
There was a problem hiding this comment.
I'd prefer to have this switch and the check for whitelistEnabled inside the isAllowedToJoin function, also in my opinion the same generic message for both paths would be enough.
| return null; | ||
| } | ||
|
|
||
| pub fn getUserByKey(key: []const u8) ?*User { |
There was a problem hiding this comment.
This is a one-time use case, there is no need to pollute the namespace with it, everything used inside here is already public, so please inline it to the implementation site.
| if (@errorReturnTrace()) |trace| { | ||
| std.log.info("{f}", .{main.fmt.FormatErrorTrace{.stackTrace = trace.*}}); | ||
| switch (err) { | ||
| error.NotWhitelisted => {}, |
There was a problem hiding this comment.
I don't think this code here should worry about the error sets of arbitrary network protocols. I assume you want to get rid of the error popup? I think the right solution here would be to change the error to a warning, there are many other cases where this code is run (e.g. version differences) that probably shouldn't have the error popup either.
|
Sure, but if |
|
WorldSettings will also include stuff like gamerules in the future, which I would also like to have configurable in-game. |
Actually they will change: #3499 (maybe not from within the world, but that can also change) |
|
Ahh okay. Fine to save like this then? |
| applyAction(source, params.action, key); | ||
| }, | ||
| .@"/whitelist <enable/disable>" => |params| { | ||
| main.server.world.?.settings.whitelistEnabled.store(params.toggle == .enable, .monotonic); |
There was a problem hiding this comment.
The change also needs to be stored to disk immediately to avoid losing it when the game doesn't close correctly. (world.saveWorldConfig)
| const keys = zon.getChild("keys"); | ||
| try conn.user.?.identifyFromKeysAndName(name, keys); | ||
|
|
||
| if (!main.server.players.isAllowedToJoin(conn.user.?.newKeyString.?, main.server.world.?.settings.whitelistEnabled.load(.monotonic))) { |
There was a problem hiding this comment.
There is a problem with this. There are multiple keys in order to allow switching to a new crypto algorithm when one is broken. So when the server switches this (for testing you can do this in the launchConfig), it will then reject the old player which is still in their list with one of the old keys.
To fix this I think the best solution would be to do this check together with players.lookupIndex in User.identifyFromKeysAndName, so that the old entries are found correctly.
…that identified player
|
Ok, I merged |
3 participants
Enforces the player whitelist during the connection handshake and adds a
/whitelistcommand to manage it.After a connecting player's key is resolved, the server checks
players.isAllowedToJoinwhenworld.settings.whitelistEnabledis set, rejecting the connection witherror.NotWhitelisted.Command syntax:
/whitelist <add/block> <keyType>:<base64Key>or/whitelist <add/block> @<playerIndex>Also removes error logging for
error.NotWhitelistedsince it's expected to reject and already logs the reason right after rejection.Closes #2566