lotgd / core

Core functionality for Legend of the Green Dragon, a text-based RPG game.
GNU Affero General Public License v3.0
152 stars 15 forks source link

Introduces Permissions to the core. #78

Closed Vassyli closed 7 years ago

Vassyli commented 7 years ago

Adds a permission model, a manager, as well as traits and interfaces required for models who want to implement permissions.

Every actor has many permission associations, every permission association references a permission as well as a state. This results in ternary permission system:

This is later important if a crate wants to implement a multi-layered permission system with groups and inheritance.

austenmc commented 7 years ago

Good observation about supporting inherited permissions.

austenmc commented 7 years ago

I'm concerned there is some unnecessary complexity here that will cause problems for the noob coders we expect to be writing permission checks.

  1. In the current PR, there are two ways to check for a permission: one is through the PermissionManager and one is through the actor class. It's hard to know, just from reading the APIs of User and PermissionManager, which one to use.
  2. Presumably, the cause of (1) is b/c you want to have permissionable entities that aren't Users, like maybe Groups? Are there any other entities you think will implement PermissionableInterface? I can imagine maybe modules have permissions, if you want to establish stronger boundaries between the code you pull in off the web. I'm not sure how useful this is honestly, or whether it's worth the complexity. What if we restricted permissions to be only for Users, then you check permissions via the Manager, and the manager implements a hook that can support Groups via a module.

Open to your thoughts here.

Vassyli commented 7 years ago

Yes, that's what I intended. Now that you mention it, there is not really much anything besides User and Groups that might use this. I didn't even think about modules. Maybe bots, but they could easily just be a User instead.

I will refactor the Permissionable trait to an abstract Actor model class instead. Also, I will provide a PermissionGroup Interface that the PermissionManager can understand and work with it, as well as add events at least to "isAllowed" and "isDenied".

Alternatively, instead of events I could introduce a GroupableInterface that a User entity might or might not implement whose only function returns a PermissionGroup. Or additionally to the events.

Thoughts?

austenmc commented 7 years ago

Still planning on working on this?

As far as your question is concerned, I would still with something simple, just the events, for now. We're losing steam at this point :) so I say we push toward some kind of MVP.

Vassyli commented 7 years ago

Yes, I'm still planning, just didn't find time to sit down and get it right.