[WIP] Improvements to SimpleRbac

[Xymanek]

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]

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

[Xymanek]

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

Can anyone take a look? Local tests

[Xymanek]

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]

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

[steinkel]

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]

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]

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

[Xymanek]

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]

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]

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]

makes sense for me

[coveralls]

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

[Xymanek]

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]

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]

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

[Xymanek]

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]

ping @steinkel

[steinkel]

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]

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]

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

[Xymanek]

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

[Xymanek]

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

[Xymanek]

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]

Shouldn't this be called RuleInterface as per conventions?

[steinkel]

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]

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

[Xymanek]

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

[steinkel]

Yes, makes sense

[steinkel]

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

[steinkel]

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

Thanks,

[Xymanek]

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]

Merged with develop branch and created new PR #423

[steinkel]

Closed per #423