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
| switch (main.server.players.isAllowedToJoin(conn.user.?.newKeyString.?)) { | ||
| .allowed => {}, | ||
| .blocked => { | ||
| std.log.info("Rejected connection from '{s}': blocked", .{name}); |
There was a problem hiding this comment.
The name alone is rather unhelpful, it would make sense to also print the public key string
|
|
||
| pub fn parse(_: NeverFailingAllocator, name: []const u8, arg: []const u8, errorMessage: *ListManaged(u8)) error{ParseError}!KeyString { | ||
| const colonIndex = std.mem.indexOfScalar(u8, arg, ':') orelse { | ||
| errorMessage.print("Expected a public key of the form \"<keyType>:<base64>\" for <{s}>, found \"{s}\"", .{name, arg}); |
There was a problem hiding this comment.
Instead of adding your own validation logic, please use authentication.PublicKey.initFromBase64
| 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.
| pub const description = "Manages the connection whitelist"; | ||
| pub const usage = | ||
| \\/whitelist <add/block> <keyType>:<base64Key> | ||
| \\/whitelist <add/block> @<playerIndex> |
There was a problem hiding this comment.
Would it be possible to also have whitelist enable/disable commands?
To implement this in a thread-safe manner you can just make the bool atomic.
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