working / better UnitTests for dynamic assertions

[manuakasam]

See #135, we should have four unit tests covering this feature

1) A required permission is not granted but the assertion returns true (result allow) 2) A required permission is not granted and the assertion returns false (result exception) 3) A required permission is granted and the assertion returns true (result allow) 4) A required permission is granted but the assertion returns false (result allow)

Due to the vacation days I won't get around to write those, but imo those should be there (and obviously are required) ;)

[bakura10]

We already have tests there and there.

1) A required permission is not granted but the assertion returns true (result allow)

No, should not result allow, but fallback to testing permissions.

2) A required permission is not granted and the assertion returns false

No, should return false (if the assertion returns false, there is no need to test permissions).

3) A required permission is granted and the assertion returns true (result allow)

Yes.

4) A required permission is granted but the assertion returns false (result allow)

No, should return false.

@danizord and @arekkas, do you agree?

[aeneasr]

Not completely getting why we should throw an exception? I think it should look like:

1) Disallow 2) Disallow 3) Allow 4) Disallow

[bakura10]

Clearly, not a single case should throw an exception (except when you give an invalid assertion)

Envoyé de mon iPhone

Le 21 déc. 2013 à 13:35, arekkas notifications@github.com a écrit :

Not completely getting why we should throw an exception? I think it should look like:

1) Disallow 2) Disallow 3) Allow 4) Disallow

— Reply to this email directly or view it on GitHub.

[aeneasr]

Agreed

[aeneasr]

Maybe we should check with === false and === true and throw an exception if we get other results?

[bakura10]

No, that's too complicated. Useless. The assertion interface ask for returning boolean anyway. The scrutinizr score on this method is already bad enough :D.

[bakura10]

ping @arekkas , ok after some discussion with @manuakasam maybe Assertion need some more work, or maybe an additional parameters to the isGranted.

Use case: imagine a blog post where you want to allow featured blog posts. So admin can create member account. Those members can write blog post on their behalf (they are "featured") but cannot delete any post. In addition, admin can also write post, BUT they only can delete their own post (not the ones from other admins).

So the admin has the "deletePost" permission. Member only have the permission "writePost". To ensure the second condition, we must write an assertion. code would look like this:

$auth->isGranted('deletePost', function($identity) use ($post) {
    return $identity === $post->getAuthor();
});

This code will remove the write for any "member" to delete any post (the assertion will return true, but the permission will fail). Admin can delete their own post (assertion will return true and permission will be okey). However who can delete blog posts of member?

What should be the best way to do this? Adding a new "deleteFeaturedPermission" ?

[aeneasr]

How does ZF2\ACL handle this? They have assertions too, right?

The current assertion approach would require a "deleteOwningPermission", which always gets executed with:

$auth->isGranted('deleteOwningPermission', function($identity) use ($post) {
    return $identity === $post->getAuthor();
});

And additionally an "deletePermission", which is owned by admins.

Maybe we could think of a better way for assertions to be reusable? BjyAuthorize for example requires them to be a class and fetches them from the ServiceLocator, so DI is possible. I think that's a pretty decent approach! Maybe we could also couple permissions and assertions in a config, so it is even better useable:

'assertions' => [
  'deleteOwningPermission' => 'SomeAssertion' ,
]

We could also do a permission aggregator, which returns true if at least one checked permission returns true:

'aggregate' => [
  'deletePost' => ['deleteOwningPermission', 'deletePermission']
]

$auth->isGranted('deletePost');

Not a 100% sure about this tho as everything needs to be done in a config file and can't be loaded from e.g. a database (without hacking like PermissionInterface::getAssertionClassName or some rubbish like that :D).

To conclude: I don't think the use case can be solved by using one permission only! If you think about it, they are really two different permissions, one is "delete a post" and one is "delete my post".

[aeneasr]

Or, by using argument packing, so we could do:

$auth->isGranted('deleteOwningPermission', 'deletePermission')

With the new php version coming, argument pack has gotten very pretty so we could even typehint this in future versions:

function isGranted(...$permissionsToCheck);

[bakura10]

Yes you're right about multiple permissions. However we've discussed a bit with @manuakasam regarding the isGranted accepting multiple parameters. The problem is that it's ocnfusing( what happen if one fail ?) and furthermore it's very hard to know which permission failed and report correct errors.

Actually the AssertionPluginManager is something I've thought about (I've even included it while developing ZfcRbac) but I was not satisfied with it so I removed it.

What we could do is injecting an AssertionPluginManager into the AuthorizationService, and if $assertion is a string, we pull it from the plugin manager.

Could be could. I'm just wondering if this is not overkill. Most of the time assertions are really simple (like checking the object is created by the right user), so I'd like not to introduce something if it's never used. What we could do is ship ZfcRbac as it is, and if someone has request for it, we add it depending n the use case.

[danizord]

First, I think the permission should be checked prior to the assertion. If the permission is not granted, then it should return false without calling the assertion.

Assertions sounds more like a complement when checking if the user has the permission is not enough.

The authorization logic is basically check if permission is granted to the user. If you need to check something else, then you should check an assertion. Note the emphasis on "something else".

So, I'd change this logic:

1) A required permission is not granted but the assertion returns true (result allow) 2) A required permission is not granted and the assertion returns false (result exception)

If the required permission is not granted, it should return false without calling the assertion.

3) A required permission is granted and the assertion returns true (result allow)

Perfect.

4) A required permission is granted but the assertion returns false (result allow)

Here, it should return false instead.

[aeneasr]

First, I think the permission should be checked prior to the assertion. If the permission is not granted, then it should return false without calling the assertion.

This

[danizord]

@arekkas sorry for my bad english :laughing: I'm brazilian.

[aeneasr]

Oh no, it wasn't meant in that way. It was meant as an :+1: :)

[danizord]

Ah! Got it, my bad :)

[aeneasr]

I wanted to implement this but it's actually more work to do it the other way round, so I would just leave it like this. I guess this can be closed, because like @bakura10 and me pointed out: You need a second permission to do this! Also @danizord had the same feeling about assertions like us :)

[bakura10]

I'm closing, as it has been done :).