Refactor of internal role hierarchy model + changes for 3.0.0 #34

ezimuel · 2018-02-13T17:07:28Z

This PR contains a big refactoring of the role hierarchy model with minor changes in the APIs. This PR contains a potential 3.0.0 release for zend-permissions-rbac, containing BC breaks.

This PR includes the following changes:

Added

Check for circular references in role hierarchy when using Role::addChild() and Role::addParent() functions.

Changed

Role::addChild(RoleInterface $child), accepts only RoleInterface parameter, no string anymore.
Role::addParent(RoleInterface $parent), accepts only RoleInterface parameter, no string anymore.
Zend\Permissions\Rbac\AssertionInterface, added the optional parameters $permission and $role in the assert() function, as follows:
assert(Rbac $rbac, string $permission = null, RoleInterface $role = null)

Deprecated

Nothing.

Removed

AbstractIterator, removed the class. The role hierarchy is not based anymore on RecursiveIterator.
AbstractRole, removed the class. All the functions have been moved in Zend\Permissions\Rbac\Role.
Role::setParent(), use Role::addParent() instead.

Fixed

#30, Fixed circular references with the protected functions Role::hasAncestor($role) used in Role::addChild() and Role::hasDescendant($role) in Role::addParent().

michalbundyra · 2018-02-13T17:19:08Z

@ezimuel just a quick question - are we not going to bump PHP requirements to version 7.1 with 3.0.0 release??

ezimuel · 2018-02-13T17:25:27Z

@webimpress yes, I'm removing the support for PHP 5.6 and PHP 7.0, thanks for the reminder.

ezimuel · 2018-02-13T18:00:01Z

@webimpress fixed now, requiring PHP 7.1+.

ezimuel · 2018-02-13T18:04:20Z

Ping @Ocramius, @webimpress for review.
Ping also @olegkrivtsov, @asgrim, @DominicDetta, @codeaid for comments and feedback.

michalbundyra

@ezimuel I've tried not to duplicate my comments, but basically what I've pointed should be applied across the whole PR. Also as we are using PHP 7.1 we should add strict type declarations for all files (can be done later on with other PHP 7.1 improvements, to keep this PR clear).

In general it looks good, I need to have another detailed look on functionality changes.

michalbundyra · 2018-02-13T18:16:24Z

composer.json

@@ -3,7 +3,7 @@
    "description": "provides a role-based access control management",
    "license": "BSD-3-Clause",
    "keywords": [
-        "zf2",
+        "zf3",


We stop using version with zf components, so I'd suggest to use only zf and add please zendframework (please see maintainers repository and templates there)

michalbundyra · 2018-02-13T18:16:53Z

composer.json

@@ -29,7 +29,7 @@
        }
    },
    "require-dev": {
-        "phpunit/phpunit": "^5.7.15|| ^6.2.1",
+        "phpunit/phpunit": "^6.4.4",


We can switch to PHPUnit 7.0.1 as it supports only PHP 7.1+ !

michalbundyra · 2018-02-13T18:17:38Z

doc/book/examples.md

@@ -98,7 +98,7 @@ class AssertUserIdMatches implements AssertionInterface
        $this->article = $article;
    }

-    public function assert(Rbac $rbac)
+    public function assert(Rbac $rbac, $permission = null, $role = null)


Can we add some typehints? If these types are mixed could we please add PHPDocs about allowed types?

michalbundyra · 2018-02-13T18:18:13Z

src/Assertion/CallbackAssertion.php

-     *
-     * @param Rbac $rbac
-     * @return bool
+     * {@inheritDoc}


This comment doesn't have any value, I would remove it.

michalbundyra · 2018-02-13T18:18:41Z

src/Assertion/CallbackAssertion.php

     */
-    public function assert(Rbac $rbac)
+    public function assert(Rbac $rbac, string $permission = null, RoleInterface $role = null)


Can we add also return type in function declaration, please?

michalbundyra · 2018-02-13T18:20:44Z

src/Exception/RuntimeException.php

+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2018 Zend Technologies USA Inc. (http://www.zend.com)


For new files we should use only current year.

Where is this requirement defined?

michalbundyra · 2018-02-13T18:21:14Z

src/Exception/RuntimeException.php

+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository


I think it would be nice to update all headers to point to the repository (as we have in many others repos already, for example see any expressive repo)

michalbundyra · 2018-02-13T18:21:43Z

src/Exception/RuntimeException.php

+ *
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2018 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license   http://framework.zend.com/license/new-bsd New BSD License


The same here, this link doesn't even work....

michalbundyra · 2018-02-13T18:22:09Z

src/Rbac.php

     * @return \Zend\Permissions\Rbac\Rbac
     */
-    public function setCreateMissingRoles($createMissingRoles)
+    public function setCreateMissingRoles(bool $createMissingRoles): Rbac


We are using single space before colon in return type declaration.

Also as a return type I would use self (it's clearer when you read class in the middle then we know straight away that we return the same class).

Or we should drop the fluent interface altogether, as we have in other components, and return void. (This comment applies to all setter methods that currently implement a fluent interface.)

I removed fluent interface in all the library.

michalbundyra · 2018-02-13T18:28:18Z

src/Rbac.php

@@ -22,20 +25,19 @@ class Rbac extends AbstractIterator
    protected $createMissingRoles = false;

    /**
-     * @param  bool                     $createMissingRoles
+     * @param  bool $createMissingRoles
     * @return \Zend\Permissions\Rbac\Rbac


Imho this should be updated to return $this

codeaid · 2018-02-14T10:57:25Z

Hi all,

Thanks for including me in this conversation!

As you can imagine, a lot has happened since I posted the original issue. I ended up developing my own solution, which, to be honest, I'm really happy with and having successfully used it for several months in production environment there is no real need for me to switch back to Zend RBAC.

My solution literally consists of two classes - container and role - and supports adding roles by names and works around circular references. I posted the related files in this Gist in case anyone's interested to have a look. There's a usage.php file at the bottom showing how I'm using the container by building it from my service, which could really be anything.

Speaking about the PR - looks good to me at a glance. I like that the add* methods use type hinting -
stricter is always better in my books. Also, getting rid of the iterators should result in some performance increase (even if it's a small one). It's always good to strip out something that is not absolutely needed.

Other than that, I can only go by what @weierophinney says as he's the expert when it comes to framework standards. Sorry, if I'm not really being helpful :/

froschdesign · 2018-02-14T11:06:20Z

@codeaid

…there is no real need for me to switch back to Zend RBAC.

Maybe (unit) tests? 😉

codeaid · 2018-02-14T11:18:14Z

@froschdesign You mean unit testing my implementation or zend-permissions-rbac?

froschdesign · 2018-02-14T11:21:33Z

@codeaid
Do you have unit tests for your implementation? If not, this should be a reason to use zend-permissions-rbac. // off-topic ends here

ezimuel · 2018-02-14T11:34:55Z

@codeaid thanks for sharing your code with us. It's always interesting to compare different solutions to the same problem. If this works for you that's great! Of course, as @froschdesign suggested unit test will help. I suggest to add it in your implementation to find issues before they will happens in production. Thanks again for your feedbacks!

codeaid · 2018-02-14T11:53:44Z

@froschdesign @ezimuel - Of course I have unit tests! How could you even assume otherwise!? :D I added them to the gist (if it helps). I believe they yield a 100% coverage too so I'm covered (pun intended).

… improved code coverage

ezimuel · 2018-02-14T14:16:58Z

@webimpress, @weierophinney I incorporated all the feedbacks, including: declare(strict_types=1); on each file, and removing fluent interface. Moreover, I added some unit tests to improve the code coverage.

ezimuel · 2018-02-14T15:15:41Z

Now we have 100% code coverage!

Since it exists...

If a param or return annotation does not provide information beyond the typing and name already present in the signature, they are redundant.

- Adds a `callable` typehint to the `$callback` argument of the constructor. - Decorates the callback in a closure to ensure type safety when invoked; no more casting!

- Remove redundant `@param` and `@return` annotations. - Add descriptions for `@throws` annotations. - Remove unnecessary whitespace. - Add necessary whitespace. - Throw exceptions only from conditions.

- Removes redundant `@param` and `@return` annotations. - Corrects `@throws` annotations. - Adds whitespace where necessary. - Removes whitespace where necessary.

weierophinney · 2018-02-21T22:43:07Z

src/AssertionInterface.php

     */
-    public function assert(Rbac $rbac);
+    public function assert(Rbac $rbac, RoleInterface $role = null, string $permission = null) : bool;


I'm still not entirely convinced these arguments should be optional.

While I understand the argument of keeping BC breaks to a minimum, I also think we could address this in a new v2.X minor release by doing the following:

Updating the Rbac class to pass the role and permission when calling an assertion.

Documenting in migration notes to users that they should add these arguments to their assertions in order to prepare for the v3 release. Their assertions can ignore them for now — or even update their logic to make use of them!

Taking those steps allows the v3 release to make these required, as they are always passed.

@weierophinney ok, this plan sounds good to me.

In talking with @ezimuel today, we realized that for anybody implementing the AssertionInterface, they will need to make a signature change when upgrading to version 3 regardless of any changes we'd make in the v2 release branch. As such, we will not do another v2 release, and move ahead instead with the v3 release.

weierophinney

Only blocker at this point is a migration guide, as we already discussed, @ezimuel!

ezimuel · 2018-02-22T17:20:14Z

@weierophinney I wrote the migration guide, removed the optionals in AssertionInterface and fixed an issue on CHANGELOG.

Adds a complete section on `RoleInterface` changes, incorporating the sections on `setParent` and `addChild` changes, and adding information on type hint changes, renaming of getParent to getParents, and addition of getChildren.

weierophinney · 2018-02-22T19:25:30Z

@ezimuel I've edited and expanded the migration document; I think this is good to go at this point!

froschdesign · 2018-02-22T19:42:31Z

doc/book/methods.md


 ## `Zend\Permissions\Rbac\Rbac`

 `Rbac` is the object with which you will interact within your application in
-order to query for permissions. It extends `AbstractIterator`.
+order to query for permissions.

 Method signature                                                            | Description
 --------------------------------------------------------------------------- | -----------
 `addRole(string|RoleInterface $child, array|RoleInterface $parents = null)` | Add a role to the RBAC. If `$parents` is non-null, the `$child` is also added to any parents provided.


Backslash is required for pipe:

addRole(string\|RoleInterface $child, array\|RoleInterface $parents = null)

Problem is the rendering inside tables.

froschdesign · 2018-02-22T19:42:56Z

doc/book/methods.md


 Method signature                                                            | Description
 --------------------------------------------------------------------------- | -----------
 `addRole(string|RoleInterface $child, array|RoleInterface $parents = null)` | Add a role to the RBAC. If `$parents` is non-null, the `$child` is also added to any parents provided.
-`getRole(string|RoleInterface $role) : RoleInterface`                       | Recursively queries the RBAC for the given role, returning it if found, and raising an exception otherwise.
+`getRole(string $role) : RoleInterface`                                     | Get the role specified by name, raising an exception if not found.
 `hasRole(string|RoleInterface $role) : bool`                                | Recursively queries the RBAC for the given role, returning `true` if found, `false` otherwise.


Same here:

hasRole(string\|RoleInterface $role) : bool

froschdesign · 2018-02-22T19:43:21Z

doc/book/methods.md


 Method signature                                                            | Description
 --------------------------------------------------------------------------- | -----------
 `addRole(string|RoleInterface $child, array|RoleInterface $parents = null)` | Add a role to the RBAC. If `$parents` is non-null, the `$child` is also added to any parents provided.
-`getRole(string|RoleInterface $role) : RoleInterface`                       | Recursively queries the RBAC for the given role, returning it if found, and raising an exception otherwise.
+`getRole(string $role) : RoleInterface`                                     | Get the role specified by name, raising an exception if not found.
 `hasRole(string|RoleInterface $role) : bool`                                | Recursively queries the RBAC for the given role, returning `true` if found, `false` otherwise.
 `getCreateMissingRoles() : bool`                                            | Retrieve the flag that determines whether or not `$parent` roles are added automatically if not present when calling `addRole()`.
 `setCreateMissingRoles(bool $flag) : void`                                  | Set the flag that determines whether or not `$parent` roles are added automatically if not present when calling `addRole()`.
 `isGranted(string|RoleInterface $role, string $permission, $assert = null)` | Determine if the role has the given permission. If `$assert` is provided and either an `AssertInterface` instance or callable, it will be queried before checking against the given role.


Same here:

isGranted(string\|RoleInterface $role, string $permission, $assert = null)

…ure/release-3.0.0

ezimuel · 2018-03-01T17:24:43Z

@froschdesign I added the backslash for pipe in docs, as you suggested. Thanks!

Close #34

weierophinney · 2018-03-05T16:31:23Z

Thanks, @ezimuel - great improvement! Merged to develop.

ezimuel added 6 commits February 1, 2018 10:55

Updated composer for release 2.6.0

c10ad55

Updated CHANGELOG

1f8a414

Refactoring RBAC for version 3.0.0 with BC breaks

ad2ca66

Updated CHANGELOG

238f7e5

Updated README

4768f64

Fixing grammar

725a782

ezimuel added this to the 3.0.0 milestone Feb 13, 2018

ezimuel added enhancement BC Break labels Feb 13, 2018

Merge branch 'develop' into feature/release-3.0.0

a256e3b

ezimuel added 5 commits February 13, 2018 18:38

Requiring PHP 7.1+

4a5b6e6

Fixed MERGE conflict

0a507b4

Fixed CHANGELOG

a5d84cd

Fixed composer JSON format

ae79491

Fixed CallbackAssertion using AssertionInterface

8c5bb1c

michalbundyra reviewed Feb 13, 2018

View reviewed changes

Included feedbacks from zendframework#34 + removed fluent interface +…

487b847

… improved code coverage

Improving code coverage

b8bf7ea

weierophinney added 9 commits February 21, 2018 16:11

s/children/child/

49ec38e

Rephrase description to reflect argument order.

6f44772

Extend package-specific RuntimeException

26fc89d

Since it exists...

Remove redundant docblock annotations

b9098a8

If a param or return annotation does not provide information beyond the typing and name already present in the signature, they are redundant.

Make CallbackAssertion type-safe

2e8430f

- Adds a `callable` typehint to the `$callback` argument of the constructor. - Decorates the callback in a closure to ensure type safety when invoked; no more casting!

Do not wrap if line is not over length limit

0f792b2

Do not wrap line if it does not exceed line limit

125f3aa

Consistency updates for Rbac class

e0ced4c

- Remove redundant `@param` and `@return` annotations. - Add descriptions for `@throws` annotations. - Remove unnecessary whitespace. - Add necessary whitespace. - Throw exceptions only from conditions.

Fix method docblocks

def5290

- Removes redundant `@param` and `@return` annotations. - Corrects `@throws` annotations. - Adds whitespace where necessary. - Removes whitespace where necessary.

weierophinney reviewed Feb 21, 2018

View reviewed changes

weierophinney suggested changes Feb 22, 2018

View reviewed changes

Added migation guide + removed optionals in AssertionInterface

b1c91bb

weierophinney added 3 commits February 22, 2018 13:06

Fixes assert() signature in CHANGELOG entry

6d934ce

Fixes grammar of entry detailing circular reference fixes

ee968a6

Completes migration notes

23275ec

Adds a complete section on `RoleInterface` changes, incorporating the sections on `setParent` and `addChild` changes, and adding information on type hint changes, renaming of getParent to getParents, and addition of getChildren.

weierophinney approved these changes Feb 22, 2018

View reviewed changes

froschdesign reviewed Feb 22, 2018

View reviewed changes

ezimuel added 2 commits March 1, 2018 18:19

Added backslash for pipe in docs

7a0dbf8

Merge remote-tracking branch 'origin/feature/release-3.0.0' into feat…

60bf337

…ure/release-3.0.0

ezimuel mentioned this pull request Mar 5, 2018

AssertionInterface is inconvenient #24

Closed

weierophinney merged commit 60bf337 into zendframework:develop Mar 5, 2018

weierophinney added a commit that referenced this pull request Mar 5, 2018

Merge branch 'feature/34' into develop

ba20d96

Close #34

michalbundyra mentioned this pull request Aug 3, 2019

Child roles not added #45

Open

ezimuel mentioned this pull request Aug 7, 2019

Fix #45 #48

Open

Refactor of internal role hierarchy model + changes for 3.0.0 #34

Refactor of internal role hierarchy model + changes for 3.0.0 #34

Conversation

ezimuel commented Feb 13, 2018

Added

Changed

Deprecated

Removed

Fixed

michalbundyra commented Feb 13, 2018

ezimuel commented Feb 13, 2018

ezimuel commented Feb 13, 2018

ezimuel commented Feb 13, 2018

michalbundyra left a 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

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

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

Choose a reason for hiding this comment

codeaid commented Feb 14, 2018

froschdesign commented Feb 14, 2018

codeaid commented Feb 14, 2018

froschdesign commented Feb 14, 2018 • edited Loading

ezimuel commented Feb 14, 2018

codeaid commented Feb 14, 2018

ezimuel commented Feb 14, 2018

ezimuel commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

ezimuel commented Feb 22, 2018

weierophinney commented Feb 22, 2018

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

ezimuel commented Mar 1, 2018

weierophinney commented Mar 5, 2018

froschdesign commented Feb 14, 2018 •

edited

Loading