Civcraft / NameLayer

Do not open issues here; open them on the maintained fork @ DevotedMC
https://github.com/DevotedMC/NameLayer
BSD 3-Clause "New" or "Revised" License
4 stars 14 forks source link

Rework Permissions #191

Closed Maxopoly closed 8 years ago

Maxopoly commented 8 years ago

Permission rework

This PR is meant to completly rework the way permissions are handled. Previously there was only a hardcoded enum of permissions, which were force fit for all applications of NameLayer. Especially with other plugins, which have various usage, this would often leave a lot to desire.

To fix this I changed the system so permissions can be registered dynamically by other plugins. The old permissions table was replaced by a system of 2 tables, the first one includes groupId, PlayerType and permissionId, while the second one maps permissionIds to actual permissions. When starting the new version the first time, the old table will automatically be converted over to this format, this will even work with permissions that are not registered yet.

This also conveniently adresses https://github.com/Civcraft/NameLayer/issues/160

Any plugin registering a permission most provide a list of default permission levels (Member, Mods etc.), to which this permission is automatically given when creating a new group. This list may be empty, though that wouldn't make much sense. If the registered permission was never registered before (meaning it's not in the lookup table), it's added to that and all existing groups are updated according to the default permission levels specified. This is only done in the database, so all permissions have to be registered on startup, before any groups are loaded from the db, otherwise cached permissions will overwrite the ones in the db. Registering permissions and then removing them at a later point (meaning not registering them on startup) is no problem, it will simply mean that this permission is ignored when updating the cache from the db, it will still exist but not be touched.

Blacklists

This has been a long requested feature and I decided to add this as well, while I'm on it. Every group now has a blacklist associated with it, which is basically just a collection of UUIDS. To make this work I added a new PlayerType, additionally to MEMBERS, MODS, ADMINS and OWNER, there is now also NOT_BLACKLISTED. By default anyone has the NOT_BLACKLISTED permission on any group, unless he's explicitly blacklisted. This will be very useful to allow players to control exactly who can access what, especially with the new permissions from other plugins which will be added.

Members of a group may add or remove other players to/from the blacklist of a group at any time, to make this possible I added 3 new commands, one to add players, one to remove them and one to list all the blacklisted players for a group.

Permission inheritation

Previously linking groups would only inherit members and permission in a very confusing way. To improve this I reworked the way inheritation works for linked groups works. Instead of inheriting permissions or members, only PlayerTypes will now be inherited. This means that if you have MOD on a supergroup, you will also have all permissions associated with MOD on the subgroup, but just because you have a permission on the supergroup, doesn't necessarily mean you will also have it on the subgroup. This should allow setting up clear and complex permission structures at the same time.

Also I removed the SUBGROUP playertype, but that shouldn't be an issue as it had literally no use in the code.

Other issues

This also includes fixes for various other issues, the ones which had issues are:

https://github.com/Civcraft/NameLayer/issues/186 https://github.com/Civcraft/NameLayer/issues/185 https://github.com/Civcraft/NameLayer/issues/184

I am still testing this and might update this pull if I run into any issues, but so far things have been working pretty good. Review would be appreciated.

Once this pull is ready to merge, I'll prepare a PR for every other NameLayer dependent plugin so we can then update all of them at once.

rourke750 commented 8 years ago

uh unfortuantely ttk has expressed he doesn't want anything to do with blacklists. @ttk2

Maxopoly commented 8 years ago

Huh, why wouldnt we want that?

Maxopoly commented 8 years ago

Also I would like to suggest that we completly remove public groups from NameLayer. Codewise they are basically the same thing as private groups and there are just occasional checks whether a group is public in some random places. Permissioncontrol for public groups is only understandable if you actually read up in the code which default perms they get and even then you can't control everything. Through the possibilites this pull adds to private groups, the functionality would be completly picked up and even extended, so public groups would be obsolete anyway.

Additionally we could convert existing public groups over according to the permission logic that were previously applied to them.

ProgrammerDan commented 8 years ago

I'll try to grab a few mins today to review this. Thanks again for diving headlong in!

ProgrammerDan commented 8 years ago

Clearly a ton of work. I've read through it largely, and most of my concerns that remain are resolved by testing. If you're feeling ambitious I have some unit test / mocking examples over in WordBank plugin that we could probably adapt to statically test some of these features.

Maxopoly commented 8 years ago

Hmm, I've been just throwing it on the test server and trying to break it with a few people, but I guess if you think it's worth the effort we could do that.

ProgrammerDan commented 8 years ago

Just having PTSD from trying to fix subgroups w/ merging and all those null groups n' shit, could have caught 80% of those errors with some decent test coverage.

Maxopoly commented 8 years ago

I already ran into some issues with deleted groups, but I'll adress those with another commit shortly. I dont really think it's worth the effort to write tests for this if it's already pretty much working and we can just throw it on prod/our main test server where the worst thing to happen is minor user inconvenience

ProgrammerDan commented 8 years ago

TBH if we screw it up too badly we'll have a valid excuse to wipe and restart ;P

Oh did I say that out loud

Maxopoly commented 8 years ago

@ProgrammerDan Whats your opinion on ditching public groups?

ProgrammerDan commented 8 years ago

Only thing that public groups would give us over NOT_BLACKLISTED is dealing with non-player citadel boundaries; e.g. "public group hoppers" and chests, etc.

Maxopoly commented 8 years ago

Couldnt you still do all of that with insecure reinforcements and the CHESTS permission?

ProgrammerDan commented 8 years ago

Yeah probably, I wasn't sure if insecure still did anything though. Haven't used them in quite a while.

Maxopoly commented 8 years ago

Yeah, insecure works perfectly fine

ProgrammerDan commented 8 years ago

I have no objections then

Maxopoly commented 8 years ago

https://youtu.be/L5P-P00oogY?t=81