Conversation
|
Could you give an example of how this would look? Also one relevant case would be to allow for localized permissions to e.g. forbid editing in a region (#81) |
|
Example for you can chain the dots as long as we don't reach the limit with recursions (for checking and adding permissions) |
|
Yeah, that seems reasonable (though with how much this is like a file system, I'd prefer if it used slashes). And of course we should pick a reasonable hierarchy, there should be a subcategory for commands, and inside of commands there should be a subcategory of get commands. Also one problem I see in your code is that if you enable a parent node then all the subtree information is lost. So if I give you permissions to run all commands, then take this permission away at a later point, you cannot even run /help anymore. |
|
okay so to adresss a few of your things I think I need somekind of global saved permission tree. So I think I will change the system so that as an example commands register themself like
I think I will introduce an optional
This is already supported? There is just not a good way to enable that. But through the global saved permission tree you could do things like: Also in that regard what do you think about * ? should it mabe just be usable at the top so |
I don't see how a global permission tree makes this easier, if at all I think it adds more edge cases, since the tree can change between updates (or just during normal gameplay). |
A lot can probably be done without the global tree but this not for what I see. |
|
The tree could maybe be also only used to get a picture of the current tree. So "*" instead of giving .all could just loop over all subPermissions in the global tree and add them all to the user. |
Yeah, that's what I would propose, just add an enum to each node which has 3 states allowed, forbidden and neutral. Another approach would be to kill recursion (and I'd always be in favor of that), and have just a single per player white and a single per player black list with the full permission path, then to check permission you'd basically just do: var iterator = std.mem.findRightMostIteratorWhateverIDontKnow(permissionPath, '/');
while(iterator.next()) |endIndex| {
if(blacklist.has(permissionPath[0..endIndex])) return false;
if(whitelist.has(permissionPath[0..endIndex])) return true;
}
return false; |
Do I understand that right that your |
|
Yeah, exactly. |
|
Changed to the black/whitelist system. (much better) For the whitelist / blacklist system as long as one group has whitelist access you are good to go. And for commands in general: I don't want to spend as much time into things like seperate permissions for the seperate subcommands as I think this would be much easier with #1425 |
|
The other important thing would be sharing with the client. This is relevant for anything that has client-side prediction (e.g. gamemode permissions, protector block, #2414, ...) For that I'd suggest to precompute the full set of permission from the user+group permission and send that to the client on each change. I think that would be easier and more efficient. Also another important reminder: Add testing! The permission system should have unit tests for all of its functionality. |
|
added tests and updated the description of the PR to reflect the changes |
|
Oh what I also wanted to mention is that I am thinking of adding a kinda root permission. currently there is no way for someone to have access to literally everything (without specifing every top level permission like "command"). So maybe if someone has access to "root" or some other keyword they just skip every check. Or we start everything with "/" and then you can get access to that |
|
Starting everything with a slash seems to be a reasonable option. |
|
The permissions / groups are now saved into zon files. (player permission in player zon and groups into a new file
So either we update every player zon file when a group is deleted |
IntegratedQuantum
left a comment
There was a problem hiding this comment.
So either we update every player zon file when a group is deleted
or
groups save the players instead (I think this solves the problem better, but I would like some input)
groups saving which players use them might cause other problems.
What we can do instead is give groups a unique ID. Every time you delete a group id, you don't reuse it, and when you load a player with a deleted id, then you can just detect this and remove it.
Also a thought on multithreading: I want to multithread player actions in the future, so we need to support reading permissions from multiple threads. In order to achieve this without adding locks everywhere, I'd suggest to add assertions to every modifying function that assert that we are on the server thread (see sync.threadContext).
Because of the commands there is also now the requirement that groups don't start with "/"
Why is that a problem?
| } else if (std.ascii.eqlIgnoreCase(arg, "join")) { | ||
| op = .join; | ||
| } else if (std.ascii.eqlIgnoreCase(arg, "leave")) { | ||
| op = .leave; |
There was a problem hiding this comment.
std.meta.stringToEnum()
src/server/permissionLayer.zig
Outdated
| .string => |string| fillMapHelper(allocator, map, string), | ||
| .stringOwned => |string| fillMapHelper(allocator, map, string), |
There was a problem hiding this comment.
No need for a poorly named helper:
| .string => |string| fillMapHelper(allocator, map, string), | |
| .stringOwned => |string| fillMapHelper(allocator, map, string), | |
| .string, .stringOwned => |string| { | |
| ... | |
| }, |
src/server/permissionLayer.zig
Outdated
| if (zon != .array) return; | ||
|
|
||
| for (zon.array.items) |item| { |
There was a problem hiding this comment.
| if (zon != .array) return; | |
| for (zon.array.items) |item| { | |
| for (zon.toSlice()) |item| { |
src/server/permissionLayer.zig
Outdated
| map.put(allocator.allocator, duped, {}) catch unreachable; | ||
| } | ||
|
|
||
| fn fillMap(allocator: NeverFailingAllocator, map: *std.StringHashMapUnmanaged(void), zon: ZonElement) void { |
There was a problem hiding this comment.
I'd prefer symmetrical naming, mapFromZon, mapToZon
src/server/permissionLayer.zig
Outdated
| } | ||
|
|
||
| fn mapToZon(allocator: NeverFailingAllocator, map: *std.StringHashMapUnmanaged(void)) ZonElement { | ||
| var zon: ZonElement = .initArray(allocator); |
There was a problem hiding this comment.
| var zon: ZonElement = .initArray(allocator); | |
| const zon: ZonElement = .initArray(allocator); |
src/server/permissionLayer.zig
Outdated
| var it = std.mem.splitBackwardsScalar(u8, permissionPath, '/'); | ||
| var current = permissionPath; | ||
|
|
||
| while (it.next()) |path| { |
There was a problem hiding this comment.
Since this iterator is not really suited, I'd suggest
while(std.mem.lastIndexOfScalar(u8, current, '/)) |nextPos| {
...
current = current[0..nextPos];
}
src/server/permissionLayer.zig
Outdated
| const _key = self.list(listType).getKeyPtr(permissionPath); | ||
| if (_key) |key| { |
There was a problem hiding this comment.
| const _key = self.list(listType).getKeyPtr(permissionPath); | |
| if (_key) |key| { | |
| const key = self.list(listType).getKeyPtr(permissionPath) orelse return false; |
src/server/permissionLayer.zig
Outdated
| fillMap(self.allocator, self.list(listType), zon); | ||
| } | ||
|
|
||
| pub fn listToZon(self: *Permissions, allocator: NeverFailingAllocator, listType: ListType) ZonElement { |
There was a problem hiding this comment.
Instead of leaving the serialization of both lists to the caller, I'd prefer a function that stores it all, you can do this by taking the ZonElement instead of returning it.
src/server/permissionLayer.zig
Outdated
| pub const PermissionGroup = struct { | ||
| allocator: NeverFailingAllocator, | ||
| permissions: Permissions, | ||
| members: std.StringHashMapUnmanaged(void) = .{}, |
There was a problem hiding this comment.
You do a lot of bookkeeping here, but I cannot see any place where you actually use these for anything.
| groups.put(allocator.dupe(u8, name), .init(allocator)) catch unreachable; | ||
| } | ||
|
|
||
| pub fn deleteGroup(name: []const u8) bool { |
There was a problem hiding this comment.
Many of these C-style functions should be member functions (of either the PermissionGroup or the User or if you want them in this file, then I'd suggest a wrapper struct around all the stuff that's stored in the player) with only a single global getGroup(name) function to access them by name.
There was a problem hiding this comment.
Everything that has not directly something to do with the groups list is now somewhere else.
everything else is also for now I think done.
One problem I have is that the asserts to cause the tests to fail...
There was a problem hiding this comment.
In that case I'd suggest to just disable the assert when in testing (do this inside the assert function)
There was a problem hiding this comment.
added. Now all test pass again
poorly made commands... thats the problem. will fix when I get back to this |
IntegratedQuantum
left a comment
There was a problem hiding this comment.
Also now that I think about more, you are freeing resources everywhere, so in order to access them from other threads you need to either make use of garbage collection or also assert that they are on the server thread.
src/server/command/_list.zig
Outdated
| pub const mask = @import("worldedit/mask.zig"); | ||
| pub const replace = @import("worldedit/replace.zig"); | ||
|
|
||
| pub const perm = @import("permission/perm.zig"); |
There was a problem hiding this comment.
Please sort them alphabetically, permission comes before worledit
There was a problem hiding this comment.
Files should be named in snake_case, unless they have top-level fields.
Also I would suggest to just call it permission
src/server/permission.zig
Outdated
| if (zon != .array) return; | ||
|
|
||
| for (zon.toSlice()) |item| { |
There was a problem hiding this comment.
The check is unnecessary, toSlice does the check internally.
| if (zon != .array) return; | |
| for (zon.toSlice()) |item| { | |
| for (zon.toSlice()) |item| { |
src/server/permission.zig
Outdated
| const ZonElement = main.ZonElement; | ||
| const sync = main.sync; | ||
|
|
||
| fn mapFromZon(allocator: NeverFailingAllocator, map: *std.StringHashMapUnmanaged(void), zon: ZonElement) void { |
There was a problem hiding this comment.
Just a thought, but we have a specific hashmap, and two member functions, how about making a wrapper struct for it?
src/server/permission.zig
Outdated
| return zon; | ||
| } | ||
|
|
||
| pub const Permissions = struct { |
There was a problem hiding this comment.
Please add MARK comments for all the big types and the start of the test section
src/server/permission.zig
Outdated
| } | ||
| }; | ||
|
|
||
| var groups: std.StringHashMap(PermissionGroup) = undefined; |
There was a problem hiding this comment.
I think you forgot to reset the groups. I'd suggest to tie the init/deinit call to the world init/deinit to prevent any problems with persisting resources.
| groups.deinit(); | ||
| } | ||
|
|
||
| pub fn groupsToZon(allocator: NeverFailingAllocator) ZonElement { |
There was a problem hiding this comment.
Could unified with init (see comment above)
or if not, should assert that groups is empty.
There was a problem hiding this comment.
I unified init and groupsFromZon.
I let groupsToZon stay if we want to save it sometimes in between joining and leaving a world.
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
This is a first step in the direction of #1961
Usage:
Users / Groups can be either whitelisted or blacklisted from a permissionPath.
For example a user can be granted the permission: "command" but be blacklisted from "command/spawn"
This would mean the user can run any command except spawn.
Users can be added to groups.
User permissions precede the group permissions, so if a user is blacklisted from the spawn command and joines a group who is whitelisted for the spawn command, the user would still have no permission to use spawn.
For handling all this 2 Commands are introduced:
/permto add permssionPaths to the whitelist / blacklist of the user or a certain group/groupto create / delete / join or leave groupsTasks for a complete permission layer (can be follow-up issues):
/command)