Introduces Permissions to the core. #78

Vassyli · 2016-11-01T09:55:18Z

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:

Allowed (actor has a permission association for this permission id, with state = Allowed)
Denied (actor has a permission association for this permission id, with state = Denied)
Undefined/Not set (actor has not a permission association for this permission id).

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

austenmc · 2016-11-02T19:13:58Z

src/Models/PermissionableInterface.php

+namespace LotGD\Core\Models;
+
+/**
+ * Implement this interface if an entity has associates permissions.


nit: "has associated permissions"

austenmc · 2016-11-02T19:14:26Z

Good observation about supporting inherited permissions.

austenmc · 2016-11-02T19:33:19Z

src/PermissionManager.php

+    const Denied = -1;
+
+    const Superuser = "lotgd/core/superuser";
+    const AddScenes = "lotgd/core/scene/add";


I think these scene/character-based permissions should be added as part of the scene or character class.

austenmc · 2016-11-02T19:34:27Z

tests/ModelTestCase.php

@@ -47,6 +47,7 @@
                $libraryConfigurationManager = new LibraryConfigurationManager($composerManager, getcwd());
                $directories = $libraryConfigurationManager->getEntityDirectories();
                $directories[] = implode(DIRECTORY_SEPARATOR, [__DIR__, '..', 'src', 'Models']);
+                $directories[] = implode(DIRECTORY_SEPARATOR, [__DIR__, 'Ressources', 'TestModels']);


nit: "Resources"

austenmc · 2016-11-02T19:36:58Z

src/Tools/Model/Permissionable.php

+        }
+    }
+
+    public function hasPermission(string $permissionId): bool


hasPermissionSet is clearer that it's a presence flag. hasPermission sounds like "Do I have permission?" which sounds like "Is allowed?".

austenmc · 2016-11-02T20:35:11Z

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

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.
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 · 2016-11-03T09:17:48Z

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 · 2016-12-31T22:18:54Z

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 · 2017-01-02T08:24:52Z

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

austenmc · 2017-01-18T20:38:49Z

src/Models/Actor.php

+abstract class Actor
+{
+    /** @var array Associations between permission-id and PermissionAssociation entity. */
+    private $_permissions = [];


nit: is prefixing private vars with _ in a style guide somewhere? Is that common in PHP?

No it's not. I need to get rid of it, that's something that carried over from python - thanks for pointing it out.

austenmc · 2017-01-18T20:41:19Z

src/PermissionManager.php

+use LotGD\Core\Models\Permission;
+
+/**
+ * The PermissionManager manages (checks and manipulates) of actors.


nit: "manipulates) permissions of actors"

austenmc · 2017-01-18T20:41:54Z

src/PermissionManager.php

+ * The PermissionManager class provides methods to work with permissions and is
+ * the only way to check and manipulate permissions. It can be used to create or
+ * delete permissions, to remove, allow or deny permissions to actors and to
+ * check whether an actor has a certain permission or if it is explicitely


nit: explicitly
nit: denied to him

austenmc · 2017-01-18T20:45:53Z

tests/Resources/TestModels/User.php

+use Doctrine\ORM\Mapping\Table;
+
+use Lotgd\Core\Models\Actor;
+#use LotGD\Core\Models\PermissionableInterface;


Remove the commented out code?

… actor name.

The implicit API requirements via class properties has been changed to relay now on abstract methods that the extending class must implement.

austenmc reviewed Nov 2, 2016

View reviewed changes

Vassyli force-pushed the feature/permissions branch from 6ef2252 to 6f2f88d Compare January 2, 2017 15:20

austenmc approved these changes Jan 18, 2017

View reviewed changes

Vassyli added 7 commits January 19, 2017 10:02

Adds Permission model and framework for testing permission manager.

3bf23f3

Adds methods to read and check permissions.

3b8537f

Adds removal of exceptions as well as error handling.

1c89d8f

Adds logging to permission manager and requires an actor to return an…

f805707

… actor name.

Replaced Permissionable/-Interface with an abstract Actor class.

64cb22d

Applied suggested changes

9ecd0dd

Abstract actor model has more straightforward API

e82e72a

The implicit API requirements via class properties has been changed to relay now on abstract methods that the extending class must implement.

Vassyli force-pushed the feature/permissions branch from 6f2f88d to e82e72a Compare January 19, 2017 09:33

Vassyli merged commit e82e72a into master Jan 19, 2017

Vassyli deleted the feature/permissions branch January 21, 2017 15:13

Vassyli mentioned this pull request Jan 21, 2017

Build a permissions system to control who can post motds, delete users, etc. #26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces Permissions to the core. #78

Introduces Permissions to the core. #78

Vassyli commented Nov 1, 2016

austenmc Nov 2, 2016

austenmc commented Nov 2, 2016

austenmc Nov 2, 2016

austenmc Nov 2, 2016

austenmc Nov 2, 2016

austenmc commented Nov 2, 2016

Vassyli commented Nov 3, 2016

austenmc commented Dec 31, 2016

Vassyli commented Jan 2, 2017

austenmc Jan 18, 2017

Vassyli Jan 19, 2017

austenmc Jan 18, 2017

austenmc Jan 18, 2017

austenmc Jan 18, 2017

Introduces Permissions to the core. #78

Introduces Permissions to the core. #78

Conversation

Vassyli commented Nov 1, 2016

Choose a reason for hiding this comment

austenmc commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austenmc commented Nov 2, 2016

Vassyli commented Nov 3, 2016

austenmc commented Dec 31, 2016

Vassyli commented Jan 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment