[WIP] Improvements to SimpleRbac by Xymanek · Pull Request #411 · CakeDC/users

Xymanek · 2016-08-08T05:41:06Z

This is my take on #407 plus some additional things. It is completely BC (according to existing tests) and features several improvements:

Permission is evaluated top-down (using foreach) so if a rule fails, it won't compare the next one and comparison is done in user-defined order (vs previously hard-coded role -> prefix -> plugin -> extension -> controller -> action -> allowed)
Callables/instances of \CakeDC\Users\Auth\Rules\Rule do not have to be placed under allowed key and you can use many of those inside one permission
You can access any value from user array using dot notation (eg. 'role.title' => 'Admin' or 'role.id' => 2)
You can access values from user array that are reserved (eg. allowed) by prepending user. to the array key (eg. user.allowed)
Anything under allowed key will be cast to boolean
If the array key begins with * the result will be inverted (eg. '*controller' => 'Test' will match any controller besides Test)
If the permission array is missing controller and/or action key(s), it will be discarded and a message will be logged (previously it would fail silently)

I think that's about it... If people think this is a good change I'll go ahead and write the additional tests.

One things that bugs be however is that the $role variable that is passed to _matchRule is not used anywhere besides being passed to callable/\CakeDC\Users\Auth\Rules\Rule since we are reading directly from user array now

coveralls · 2016-08-08T05:46:50Z

Coverage decreased (-0.9%) to 80.04% when pulling 383fde6 on Xymanek:rbac-user-params into 3af032e on CakeDC:develop.

Xymanek · 2016-08-08T05:53:33Z

This is weird... I swear the tests pass locally...

Can anyone take a look? Local tests

Xymanek · 2016-08-08T07:00:16Z

PHP 5.3 or later, the default value is E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED. This setting does not show E_NOTICE, E_STRICT and E_DEPRECATED level errors.

Well this screwed me over

coveralls · 2016-08-08T07:07:37Z

Coverage decreased (-0.5%) to 80.37% when pulling 43b95ed on Xymanek:rbac-user-params into 3af032e on CakeDC:develop.

steinkel · 2016-08-09T10:54:49Z

Hi @Xymanek this is looking promising, I'm concerned about the BC issue forcing people to refactor their rules if we go ahead with this PR, could you allow a legacy mode or move the changes to a new ImprovedRbac Auth object (possibly reusing most of the code in an abstract class) so we can ensure users will keep their rules working? Keep in mind rules are critical to maintain in existing apps...

Thanks!

Xymanek · 2016-08-09T11:51:18Z

The changes were aimed to be completely BC which can be seen by existing tests (only CS check failed, I'll fix that if we decide to go on with this).

However the $role variable that is passed to _matchRule is not used anywhere besides being passed to callable or \CakeDC\Users\Auth\Rules\Rule since we are reading directly from user array now. This can be a slightly breaking change in case someone relies on default_role config value in case specific user's role isn't specified. I haven't seen that case covered anywhere in tests.
One possible way to fix this would be to add role as an exception and if the dev wants to bypass he can use user.role... Thoughts?

As for new Auth object... I've thought about that but decided against since the changes are BC and that would simply make it harder to upgrade for existing users. Although I do agree that calling this Simple might be a bit of understatement...
That said, I'm thinking about a universal auth system for Cake that would include automatic public/user-only action switching, abstraction from certain controllers/actions, naming rules/group of rules... But I feel like that would either require changes to core or something else, dunno yet... 💭

TL; DR I aim to not alter the behavior of current use cases in any shape or form.

steinkel · 2016-08-09T11:53:40Z

Excellent! I would be happy to help in any improvements you have in mind 👍

Xymanek · 2016-08-09T11:58:19Z

Well the 'universal auth system' is a very long shot 😀 so it's a topic for another discussion.

As for this PR, do you think the idea of making the role another exception is a good one? If yes, I'll go on with writing tests

steinkel · 2016-08-09T12:08:54Z

Ok,

About the role, the reason is using the defaults when no role provided, or role is defined in an associated table and not directly in the users table. I guess we could inject the value into the user array so it can be accessed later on?

Xymanek · 2016-08-09T12:19:00Z

I thought about injecting it into the user array but I would rather leave user.role as an option to check what's the original value is. I'll put it in the $reserved array, this way we will allow both legacy usage as well as new one

steinkel · 2016-08-09T12:22:10Z

makes sense for me

coveralls · 2016-08-09T12:47:13Z

Coverage decreased (-0.8%) to 80.106% when pulling 13470b2 on Xymanek:rbac-user-params into 3af032e on CakeDC:develop.

Xymanek · 2016-08-11T06:20:38Z

I'm almost done with the tests but I have a couple of questions:

What is the "Cake" way of writing multi-line if conditions?
How do I test if a message was logged?

steinkel · 2016-08-11T08:43:20Z

What is the "Cake" way of writing multi-line if conditions?
There is no specific recommendation here http://book.cakephp.org/3.0/en/contributing/cakephp-coding-conventions.html (there is for multi line function calls), but our "in house" rule is:
If the condition is too long or complex, we create a protected method and try to name it the best we can to describe what we are going to check.

How do I test if a message was logged?
You could use $this->log(...), mocking the log method first and check params passed are correct.

Thanks

coveralls · 2016-08-13T10:30:01Z

Coverage increased (+0.08%) to 80.987% when pulling 6ad5d1b on Xymanek:rbac-user-params into 3af032e on CakeDC:develop.

Xymanek · 2016-08-13T10:33:24Z

Okay, tests are almost finished. I also corrected some of existing tests so that they actually run (some array keys were same so only one out of two sets of arguments run). Also some fixes to my previous code (because what are the tests for?:smile:)
The only two things that are left:

Test the behavior when default_role is changed
And test this log message

When I was asking about multi-line IFs I meant this that caused this error. Also how do I fix this?

Xymanek · 2016-08-14T10:22:47Z

ping @steinkel

steinkel · 2016-08-22T11:03:45Z

Hi @Xymanek this is looking good...

About this looks like we'll need to put the first condition in the if line, and break after the ||

About this did you try this instead?

/** 
 * @var \PHPUnit_Framework_MockObject_MockObject|SimpleRbacAuthorize $simpleRbacAuthorize 
 */

If this is causing issues to phpcs we might need to send a PR to the sniff rules, please remove it if causing issues to don't block this PR moving fw. Thanks!

Xymanek · 2016-08-24T12:57:14Z

Figured out how to use the cs checker locally (I don't use it myself), unfortunately it's unhappy with the docblock either way 😞. As per your request I removed it.

coveralls · 2016-08-24T13:01:48Z

Coverage increased (+0.08%) to 80.987% when pulling d4e7ae8 on Xymanek:rbac-user-params into 3af032e on CakeDC:develop.

Xymanek · 2016-08-24T13:09:03Z

I have no idea why tests involving code that I didn't touch at all failed....
@steinkel can you please take a look?

Xymanek · 2016-08-27T08:39:28Z

I've found bugs in my code, please don't merge yet

…nd updated imports

Xymanek · 2016-09-04T10:32:53Z

Sorry that this took so long, I have too much stuff going on in my life right now :(
But I feel like this is ready... @steinkel check it out (especially the updated docs)

Xymanek · 2016-09-04T10:34:28Z

+Creating rule classes
+---------------------
+
+The only requirement is to implement `\CakeDC\Users\Auth\Rules\Rule` interface which has one method:


Shouldn't this be called RuleInterface as per conventions?

I don't see mentions to the Interface convention here http://book.cakephp.org/3.0/en/intro/conventions.html or http://www.php-fig.org/psr/psr-2/ but makes sense for me to add it and match the interfaces defined in the core. Let's do this BBC change in a future milestone.

coveralls · 2016-09-04T10:35:03Z

Coverage increased (+0.2%) to 81.143% when pulling 34924f0 on Xymanek:rbac-user-params into 3af032e on CakeDC:develop.

Xymanek · 2016-09-04T10:36:02Z

     * @param string $role role of the user
     * @param Request $request current request, used to get a default table if not provided
     * @return bool
-     * @throws \OutOfBoundsException if table is not found or it doesn't have the expected fields


I feel like this belongs to the abstract class since custom logic may or may throw any exceptions and we are not catching any specific ones

Yes, makes sense

steinkel · 2016-09-05T09:11:29Z

thank you @Xymanek I'll take a look and let you know

steinkel · 2016-09-08T15:13:06Z

I think the failing tests would be corrected after merging develop branch, could you do that please?

Thanks,

Xymanek · 2016-09-08T20:09:13Z

Sorry, I'm not very experienced with github and cross-fork merging... I've ticked "Allow edits from maintainers" on the right of this PR, I guess you have the rights to do it yourself?

steinkel · 2016-09-12T09:27:34Z

Merged with develop branch and created new PR #423

steinkel · 2016-09-21T20:36:06Z

Closed per #423

Improvements to SimpleRbac

383fde6

(Should) fix failing tests

43b95ed

steinkel mentioned this pull request Aug 9, 2016

[Suggestion] Allow any user property used in SimpleRbac #407

Closed

Fixed CS errors as well as added role to reserved keys list

13470b2

Xymanek added 2 commits August 13, 2016 13:21

Fixed some erroneous behavior in SimpleRbac

cbef4be

Added tests, fixed some old ones, removed unused import

6ad5d1b

CS fix

d4e7ae8

Fixed small bug, updated SimpleRbac documentation, added more tests a…

34924f0

…nd updated imports

Xymanek reviewed Sep 4, 2016
View reviewed changes

steinkel added this to the 3.3.0 milestone Sep 12, 2016

steinkel closed this Sep 21, 2016

Uh oh!

Conversation

Xymanek commented Aug 8, 2016

Uh oh!

coveralls commented Aug 8, 2016

Uh oh!

Xymanek commented Aug 8, 2016

Uh oh!

Xymanek commented Aug 8, 2016

Uh oh!

coveralls commented Aug 8, 2016

Uh oh!

steinkel commented Aug 9, 2016

Uh oh!

Xymanek commented Aug 9, 2016

Uh oh!

steinkel commented Aug 9, 2016

Uh oh!

Xymanek commented Aug 9, 2016

Uh oh!

steinkel commented Aug 9, 2016

Uh oh!

Xymanek commented Aug 9, 2016

Uh oh!

steinkel commented Aug 9, 2016

Uh oh!

coveralls commented Aug 9, 2016

Uh oh!

Xymanek commented Aug 11, 2016

Uh oh!

steinkel commented Aug 11, 2016

Uh oh!

coveralls commented Aug 13, 2016

Uh oh!

Xymanek commented Aug 13, 2016

Uh oh!

Xymanek commented Aug 14, 2016

Uh oh!

steinkel commented Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xymanek commented Aug 24, 2016

Uh oh!

coveralls commented Aug 24, 2016

Uh oh!

Xymanek commented Aug 24, 2016

Uh oh!

Xymanek commented Aug 27, 2016

Uh oh!

Xymanek commented Sep 4, 2016

Uh oh!

Xymanek Sep 4, 2016

Choose a reason for hiding this comment

Uh oh!

steinkel Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 4, 2016

Uh oh!

Xymanek Sep 4, 2016

Choose a reason for hiding this comment

Uh oh!

steinkel Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

steinkel commented Sep 5, 2016

Uh oh!

steinkel commented Sep 8, 2016

Uh oh!

Xymanek commented Sep 8, 2016

Uh oh!

steinkel commented Sep 12, 2016

Uh oh!

steinkel commented Sep 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

steinkel commented Aug 22, 2016 •

edited

Loading