Rework Permissions

[rourke750]

@Maxopoly @ProgrammerDan Thoughts

[Maxopoly]

Looks pretty solid.

Whats the purpose of the permission format?

[rourke750]

Nothing from now. Going to try work something in that will allow you to give access to groups through permissions.

[Maxopoly]

What additional functionality would that have compared to /nlmp ?

[rourke750]

Nothing for players. Mainly for administrative purposes.

[Maxopoly]

Hmm okay. Imo you can merge this, but letting Dan take a look probably wont hurt.

[rourke750]

All plug-ins will be broken lol.

[rourke750]

@ProgrammerDan Any suggestions otherwise going to merge. This will break all plugins but should be fairly simple to fix.

[ProgrammerDan]

Uh, can you rebase against master, then I'll review.

[Maxopoly]

Bump, we should get this merged soonish.

[ttk2]

bumping again?

[rourke750]

Bumping your bump.

[ttk2]

so rebase and merge?

[rourke750]

@ProgrammerDan Wew I know git. Merge conflict fixed. @Maxopoly Now do we actually want to merge this now or wait until things are more stable?

[Maxopoly]

I'm in favor of just going through with it, but dont care too much, if Dan wants to wait we should wait.

Also that pull doesn't even compile yet.

[ProgrammerDan]

Seriously Rourke, don't worry about this right now. Prison Pearl needs your love.

The issue here is this change impacts every plugin that uses Namelayer, generating a bunch more work for a static change. I do not recommend merging it now. @ttk2 .

After JukeAlert is firing correctly on all shards, then we could consider a systemic change like this.

[ProgrammerDan]

@rourke750 so I've been thinking about this and struggling a bit. Clearly the big weakness of the current enum system is that other plugins have to "force-fit" into the permissions that Namelayer provides. In theory the changes in this pull should allow us to get out of that mess, but in practice I think it might not. A few specific changes would be necessary.

1) Remove plugin-specific isPermitted from Namelayer. Anything that leverages Block, for instance, doesn't belong in Namelayer.

2) Add way for plugins to register a permission as a default permission based on PlayerType within Namelayer. E.g. Citadel on startup should be able to set BLOCKS as a default permission for MODS and up, with DOORS, CHESTS as default permission for MEMBERS and up. This should impact both the in-memory list of permissions but most importantly the DB query that creates a new group. Currently a plugin could register a new permission on startup using registerPermission, but it won't impact the default set that gets written to a new group, and each contributing plugin would have to manually append it to any group that they leverage; this would get stupid messy and is probably impossible as the plugins don't know when a new group gets created, meaning that players would have to manually add those permissions to every new group. Just needs to be some exposure to defaults.

3) Plugins using Namelayer should have their own isPermissible, that calls Namelayer's first, but deals with any PermissionTypes that they have registered. E.g. Citadel adds BLOCKS, DOORS, CHESTS and the code currently in Namelayer's isPermissible gets moved to a comparable method in Citadel.

4) These changes would enforce consistency across all plugins and elevate this change to something better then what we have. Right now the change as written would encourage additional tight-coupling and generally be... not good.

[suirad]

Tagging related issues, to keep things organized.

125 #150 #122

[ProgrammerDan]

Also #126

[ttk2]

Subgroups and supergroups are proving to be nearly as much trouble as I predicated. Overall I'd their inclusion worth all the edge cases we need to handle?

I guess from a player perspective maybe. We will have to gather some graylog data on their prod use to really be sure. On Mar 18, 2016 5:39 PM, "Daniel Boston" notifications@github.com wrote:

Also #126 https://github.com/Civcraft/NameLayer/issues/126

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/Civcraft/NameLayer/pull/161#issuecomment-198554968

[Maxopoly]

Bump. The other NameLayer issues are fixed and some other issues came up recently that could be fixed with this change. We should get this on the way.

[ProgrammerDan]

@Maxopoly I don't disagree but the system in this pull will not work based on the critique I posted.

[rourke750]

Agreed. I say scrap this pull and start over.

[Maxopoly]

I'll fix this up

[Maxopoly]

Replaced by https://github.com/Civcraft/NameLayer/pull/191