Permission Groups by Wunka · Pull Request #2587 · PixelGuys/Cubyz

Wunka · 2026-02-23T18:57:48Z

Follow Up PR for #2530

This adds the rest from the permissionLayer PR which was split of. For Documentation see #2530

This reverts commit 072a8f0.

IntegratedQuantum · 2026-03-02T17:31:36Z

I would suggest to add this to the group command instead, maybe /group <whitelist/blacklist> <add/remove> <groupName> <path>

I did /group <whitelist/blacklist> <groupName> <add/remove/[blank]> <path>
I switch groupName and add/remove/[blank] for easy looking up if [blank] is used. but If you really want the other way around I can look into that

IntegratedQuantum · 2026-03-02T17:43:21Z

+}
+
+pub fn getGroup(name: []const u8) error{GroupNotFound}!*PermissionGroup {
+	return groups.getPtr(name) orelse return error.GroupNotFound;


hashmaps don't have stable pointers

I added the assert but thats probaly not enough right? I am sorry I am not that knowledgeable to know what would be the best course of action here.

I finally brought myself back to this pr. I have changed the hashmap from StringHashMapUnmanaged(PermissionGroup) to StringHashMapUnmanaged(*PermissionGroup).
This should from my understanding fix the problem with the stable pointers

IntegratedQuantum · 2026-03-02T17:48:42Z

 	}

+	pub fn loadPermissionGroups(self: *ServerWorld) !void {
+		const path = std.fmt.allocPrint(main.stackAllocator.allocator, "saves/{s}/groups.zig.zon", .{self.path}) catch unreachable;


before we put everything into a single file, I would like to at least see an estimation of how big the file could be on a large server (for comparison the survival playtest despite having a peak of only around 10 concurrent players has seen over 500 distinct accounts).

Its for me the question what these groups will be used for. If we say only for command permissions I don't see the file being big. But if we say that for #81 there will actually be a group for every block then the file could grow quite big.

But separating them opens other issues. Should we load all groups at the start of a world? Or should we only load the groups in use? (would prevent commands on them).

For #81 you should be allowed to assign the same group to multiple blocks. But it will still likely end up at 1 group per player on average.

So you want seperate files correct? What is then the answer to to the my follow up question about when to load the zon then?

Since that makes management easier, I think for now we can just load them all into memory.

after finnally looking back at this. Where should I then save the currentId? in the world.zig.zon?

I have fixed the problem with a metadata.zon file inisde the group folder.
The groups are now split into seperate files per group.

I do want to update the group commands a bit, but I will do that in a follow up PR (@ syntax, list subcommand to list all groups a player is member of etc)

Argmaster · 2026-04-15T12:06:36Z

+					source.sendMessage("#ff0000Permission path {s} is not present inside group {s} permission {s}list", .{path, groupName, @tagName(listType)});
+				}
+			},
+		}


Imo all code paths should end with return, to prevent mistakes in future, where behavior is by mistake added to multiple code paths. This makes sense because all code paths are mutually exclusive. Such change would also flatten the structure a little bit by moving else contents directly into function body. Definitely not critical tho.

Argmaster · 2026-04-15T12:07:45Z

+	}
+	if (pathOp) |_op| {
+		switch (_op) {
+			.add => group.permissions.addPermission(listType, path),


Please add feedback to all positive paths.
"Permission path added to group X" or alike.

Argmaster · 2026-04-15T12:13:01Z

+		allocator.destroy(self);
+	}
+
+	pub fn hasPermission(self: *PermissionGroup, permissionPath: []const u8) Permissions.PermissionResult {


I think this is excessively descriptive. You have permission.zig PermissionGroup, addPermission and permissionPath. We get it, permissions, we don't need to repeat it 4 times unless there is ambiguity. If there is, we should be consistent, so you should savePermissionGroup instead of saveGroups.
But there isn't. It can be Group, hasPermission, path and it should be fine.

Argmaster · 2026-04-15T13:12:52Z

+	while (it.next()) |group| {
+		const path = std.fmt.allocPrint(allocator.allocator, "{s}/{d}.zon", .{groupsPath, group.value_ptr.*.id}) catch unreachable;
+		defer allocator.free(path);
+		var groupZon: ZonElement = .initObject(allocator);


Move into PermissionGroup next to fromZon as toZon

Argmaster · 2026-04-15T14:24:25Z

+
+	pub fn removeFromGroup(self: *User, groupName: []const u8) bool {
+		sync.threadContext.assertCorrectContext(.server);
+		const slice = (self.permissionGroups.getKeyPtr(groupName) orelse return false).*;


Variable names should carry the logical meaning of their content, not the type they contain. Type system already describe contained type, you can retrieve it at any time using language server. Comptime type parameters are an exception, as zig doesn't have contracts/protocols/interfaces, but that's not what we have here.

After this lengthy explanation, please rename to... Idk, groupNamePtr? Not sure what it contains.

Argmaster · 2026-04-15T14:34:30Z

+		defer groupDir.close();
+		var iterator = groupDir.iterate();
+		while (try iterator.next()) |file| {
+			if (file.kind == .file and std.mem.endsWith(u8, file.name, ".zon")) {


Would be better to make two guard clauses out of that condition.

IntegratedQuantum

In order to keep the line changes down, could you please split this into 3 PRs:

one only adds the core groups system and saving/loading in the world
one adds the changes to the User
one adds the group command

IntegratedQuantum · 2026-04-26T08:22:54Z

+			if (file.kind == .file and std.mem.endsWith(u8, file.name, ".zon")) {
+				const zon = try groupDir.readToZon(main.stackAllocator, file.name);
+				defer zon.deinit(main.stackAllocator);
+				if (std.mem.eql(u8, file.name, "metadata.zon")) continue;


This is stored outside the groups subdirectory, so no need to check for it here right?

no. its inside the groups folder.

It seems I saved it inside and tried to load it outside.... I don't know how i didn't see that. But I do want it to b inside the folder

IntegratedQuantum · 2026-04-26T08:23:22Z

 		try files.cubyzDir().writeZon(path, worldData);
 	}

+	pub fn loadPermissionGroups(_: *ServerWorld, dir: main.files.Dir) !void {


I think this function should also be in permission.zig.

IntegratedQuantum · 2026-04-26T08:23:55Z

+
+	pub fn init(allocator: NeverFailingAllocator) *PermissionGroup {
+		sync.threadContext.assertCorrectContext(.server);
+		currentId += 1;


Should immediately save the metadata file.

IntegratedQuantum · 2026-04-26T08:24:47Z

+	try std.testing.expectError(error.GroupNotFound, getGroup("root"));
+}
+
+test "invalidGroupPermissionEmptyGroups" {


Also test the success case at least once.

IntegratedQuantum · 2026-04-26T08:25:34Z

 		try self.saveWorldConfig();

 		try self.saveAllPlayers();
+		try self.savePermissionGroups();


Should be done if and only if the data changed.

Wunka · 2026-05-01T15:15:37Z

Ok I did a few changes, here is a summary:

for each group and the metadata zon exists now a bool which says if we need to save it. That means on saving only changed things are overwritten (means also we can save not only on closing of the world but also in between, but currently not done)
I noticed with the change to one file per group I never addressed deleting the file, so I introduced a queue for saving these removed groups (inspired by the userdeinitList) to make this work.
added more tests for success cases as asked by quantum
after making the most changes, I removed in commit 6d272bb and e82e54a the command and changes to the user, so that will be done in follow up PRs

Wunka added 5 commits February 23, 2026 19:53

Revert "remove groups for follow up PR"

4a22940

This reverts commit 072a8f0.

make groups functional again

32882ad

finish commands

98b6608

look at loading and saving again

38a0749

small change

71ea540

Wunka marked this pull request as ready for review February 24, 2026 18:35

Update description of /perm

0994e7a

BoySanic added this to PRs to review Feb 26, 2026

BoySanic moved this to High Priority in PRs to review Feb 26, 2026

IntegratedQuantum requested changes Mar 2, 2026

View reviewed changes

IntegratedQuantum moved this from High Priority to In review in PRs to review Mar 2, 2026

Wunka added 4 commits March 2, 2026 19:17

remove invalidfree and add comment explaining the group id

fa2ad8c

add commands to /group

8f6e6d9

revert perm.zig to before

9577a89

format

bb0787d

Wunka mentioned this pull request Mar 13, 2026

Default permission group #2540

Open

Wunka added 3 commits March 18, 2026 21:13

Operations -> Operation

ded687f

groups are now saved as pointers in the hashmap

00c0f23

fix memory leak for keys inside users permissiongroups

519e53c

This was referenced Mar 22, 2026

Permission layers and administration #1961

Open

add more command Target options #2776

Open

Wunka added 3 commits April 5, 2026 10:25

Merge branch 'master' into permisisonGroups

019a9fc

split groups in different files

1fbc5f0

update tests

b9c4cd5

Argmaster reviewed Apr 15, 2026

View reviewed changes

Comment thread src/server/command/permission/group.zig Outdated

Argmaster reviewed Apr 15, 2026

View reviewed changes

Comment thread src/server/command/permission/group.zig Outdated

Argmaster reviewed Apr 15, 2026

View reviewed changes

Wunka added 3 commits April 24, 2026 20:27

Merge remote-tracking branch 'upstream/master' into permisisonGroups

039a18c

io

2b298a8

add playerIndex syntax to add and removing users from groups

7f1f389

IntegratedQuantum requested changes Apr 26, 2026

View reviewed changes

Wunka added 9 commits April 26, 2026 13:17

rename PermissionGroup -> Group

dbfd171

move loadGroups into permission.zig

84215fc

add a lot more color resets

05e5d05

fix spelling

3b6ec92

add update only on change, deleting of files and more tests

790c11f

Merge branch 'master' into permisisonGroups

e80e96e

remove group command for later pr

6d272bb

remove player changes for later pr

e82e54a

remove test print

3a94232

Uh oh!

Conversation

Wunka commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IntegratedQuantum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wunka commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Wunka commented Feb 23, 2026 •

edited

Loading

Argmaster Apr 15, 2026 •

edited

Loading

Wunka commented May 1, 2026 •

edited

Loading