[RFC] Assertion refactor

[aeneasr]

Hi,

this RFC is related to #181 but I wanted to make a clean approach.

The problem

• Assertions need additional data to process the logic. This explicitly includes the requested permission.

  Musts

• The AuthorizationService must remain stateless

  A possible solution

interface ContextAwareAssertionInterface
{
    public function assert(AuthorizationServiceInterface $service, AuthorizationContextInterface $context);
}

interface AuthorizationContextInterface
{
    public function getPermission();
    public function getContext();
}

class AuthorizationContext implements AuthorizationContextInterface
{
     public function __construct($context, $permission)
     {
          // ...
     }

     // ...
}

    protected function assert($assertion, AuthorizationContextInterface $authorizationContext)
    {
        $context = $authorizationContext->getContext();

        if (is_callable($assertion)) {
            return $assertion($this, $context);
        } elseif ($assertion instanceof AssertionInterface) {
            return $assertion->assert($this, $context);
        } elseif ($assertion instanceof ContextAwareAssertionInterface) {
            return $assertion->assert($this, $authorizationContext);
        } elseif (is_string($assertion)) {
            $assertion = $this->assertionPluginManager->get($assertion);

            return $assertion->assert($this, $result);
        }
    }

[aeneasr]

To remove boilerplating, we could create a new interface called

interface IdentityAwareInterface
{
    public function setIdentity(IdentityInterface $identity);
}

And register an Initializer in the AssertionPluginManager for that.

However if the identity would change in a request, that could be a problem! Maybe we have to add set/getIdentity to the ResultInterface, because it actually requires a state.

[bakura10]

Having a context inside a result does not make sense to me :/. I'm not sure about the "AuthorizationResult". This is confusing, as someone may think that because you receive a "Result" object in an assertion, then you could possibly alter it (using $result->setResult(false) false instance), while the logic is to return a boolean.

I'd rather prefer to have an "AuthorizationContext" object.

[aeneasr]

I'd rather prefer to have an "AuthorizationContext" object.

:100: - I tried to find a good name but I didn't come up with one. AuthorizationContext is perfect.

Having a context inside a result does not make sense to me :/

Do you mean public function setContext($context);?

[aeneasr]

I updated the first comment accorcding to your feedback

[bakura10]

Furthermore, where is the AuthorizationService? To my experience, I've ALWAYS used the AuthorizationService in all of my assertions. I clearly don't want to write a factory for each of them just to be able to inject the authorization service.

To me the assert signature should be the following one:

public function assert(AuthorizationServiceInterface $service, AuthorizationContext $context);

where AuthorizationContext:

interface AuthorizationContextInterface
{
    public function getContext();
    public function getPermission();
}

with implementation:

class AuthorizationContext implements AuthorizationContextInterface
{
     public function __construct($context, $permission)
     {
          // ...
     }
}

We should not provide setter for the authorization context because this is expected to be a kind of immutable object.

[aeneasr]

We should not provide setter for the authorization context because this is expected to be a kind of immutable object.

You're right, again :)

Furthermore, where is the AuthorizationService? To my experience, I've ALWAYS used the AuthorizationService in all of my assertions. I clearly don't want to write a factory for each of them just to be able to inject the authorization service.

Well, the only public methods the AuthorizationService provides are ìsGranted and getIdentity. With

interface AuthorizationContextInterface
{
    public function getContext();
    public function getIdentity();
    public function getPermission();
}

you won't need the AS any more.

[bakura10]

But how do you check the isGranted ? 99% of my assertions are like that:

public function assert(AuthoirzationService $service, $context)
{
    if ($service->getIdentity() === $context) {
       return true;
    }

    return $service->isGranted('read_others');
}

How am I supposed to do that without the authorizations ervice?

[aeneasr]

If we implement an initializer in the AssertionPluginManager which "listens" on AuthorizationServiceAwareInterface?

[bakura10]

No no no initializer, pleeaaaase. That makes the code very fragile.

[bakura10]

We should enforce the AuthorizationService to always be in the assertion. Not through an initializer.

[aeneasr]

Hm but I think it is the responsibility of the factory to inject the AS dependency, not of the AS itself

How about an AbstractFactory?

[aeneasr]

-> but I absolutely understand your usecase, just trying to come up with alternatives :)

[bakura10]

No abstract factory. Everything kills performance and make code fragile, because it rely on the "did the initializer run correctly? did I register the abstract factory?". Even more configuration, where it should be simple.

I'm not sure about the factory. My code has around 30 permissions, with nearly 25 having a specific assertion. None of them has a hard dependency. I cleary don't want to write a factory for each of them. Lot of boilerplate code for 0 benefits.

In this case, this assertion is nearly always created and handled by the AuthorizationService, so to me it makes a lot of sense for the authorization service to pass itself to the "assert" method.

[aeneasr]

Ok, I'll update the RFC

[aeneasr]

Like this?

[bakura10]

This looks good, however I don't understand the point of ContextAwareAssertionInterface. This is nothing more than AssertionInterface.

Maybe you did this because you wanted to include this in ZfcRbac to keep BC ? But PHP does not support method overloading...

[aeneasr]

Yes, it aims to keep BC. See:

    protected function assert($assertion, AuthorizationContextInterface $authorizationContext)
    {
        $context = $authorizationContext->getContext();

        if (is_callable($assertion)) {
            return $assertion($this, $context);
        } elseif ($assertion instanceof AssertionInterface) { // "old" assertions work fine
            return $assertion->assert($this, $context);
        } elseif ($assertion instanceof ContextAwareAssertionInterface) { // new ones work as well
            return $assertion->assert($this, $authorizationContext);
        } elseif (is_string($assertion)) {
            $assertion = $this->assertionPluginManager->get($assertion);

            return $assertion->assert($this, $result);
        }
    }

[aeneasr]

But if you want to tag a new version and break BC lets just change the AssertionInterface :)

[bakura10]

But this cannot work :p. AssertionInterface already enforces a specific assert signature.

I'd honestly prefer to wait a bit more and tag a major version that has BC. I'd like to wait for the RoutePermissionsGuard a bit, so this will be 2.4.x branch. I think we can wait a bit to stabilize the new feature, and start 3.x branch!

[aeneasr]

The idea behind that was to choose one of those two interfaces:

class MyAssertionThatNeedsToKnowWhichPermissionToUse implements ContextAwareAssertionInterface
{
    // ...
}

class MyLegacyAssertion implements AssertionInterface
{
    // ...
}

Not knowing which permission was called is a major setback in the current assertion logic, so I thought that a backwards compatible solution should be implemented until this feature is released under 3.0 :/

[bakura10]

I see... So let's do that and submit a PR with those interfaces :). You'll also need to modify the plugin manager to accept this new interface: https://github.com/ZF-Commons/zfc-rbac/blob/master/src/ZfcRbac/Assertion/AssertionPluginManager.php#L39

I'm just not happy to the fact that it will also make this code very ugly: https://github.com/ZF-Commons/zfc-rbac/blob/master/src/ZfcRbac/Service/AuthorizationService.php#L156 as you will need to check the type, either create a Context or not...

[aeneasr]

I'm not very happy with providing two different APIs as well, it is indeed a "meh" workaround but I guess we'll need to include some more cool features in 3.0 (parametrized permissions for example :D ) which could take some time. I think we could solve https://github.com/ZF-Commons/zfc-rbac/blob/master/src/ZfcRbac/Service/AuthorizationService.php#L156 like this:

    // API updated
    protected function assert($assertion, AuthorizationContextInterface $authorizationContext)
    {
        $context = $authorizationContext->getContext();

        if (is_callable($assertion)) {
            return $assertion($this, $context);
        } elseif ($assertion instanceof AssertionInterface) {
            return $assertion->assert($this, $context);
        } elseif ($assertion instanceof ContextAwareAssertionInterface) {
            return $assertion->assert($this, $authorizationContext);
        } elseif (is_string($assertion)) {
            $assertion = $this->assertionPluginManager->get($assertion);

            return $this->assert($assertion, $authorizationContext);
        }
    }

[bakura10]

elseif (is_string($assertion)) {
            $assertion = $this->assertionPluginManager->get($assertion);

            return $this->assert($assertion, $authorizationContext);
        }

You need to add a check here too. The plugin manager may either return a AssertionInterface or ContextAwareAssertionInterface :). Lot of if/else indeed.

I'd like you to create a RFC about parametrized permissions :D. Taht's more exciting.

But anyway, I may accept to do that for the 2.5 branch. Don't hesitate to create a PR.

[aeneasr]

return $this->assert($assertion, $authorizationContext);

$this ;)

[aeneasr]

Yeah I absolutely would love to create a RFC about parametrized permissions, they work awesome in our framework but are done quite hacky - if zfc-rbac could do that out of the box..damn! :D

[bakura10]

I'm quite curious about it. Could you give me a quick example ? :)

Envoyé de mon iPhone

Le 14 juin 2014 à 23:32, arekkas notifications@github.com a écrit :

Yeah I absolutely would love to create a RFC about parametrized permissions, they work awesome in our framework but are done quite hacky - if zfc-rbac could do that out of the box..damn! :D

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

[aeneasr]

Well, you basically have different characteristics for one permission. Let's take "article.create" which can be identified by its key. Now, you add a second, third or fourth parameter to get a granular permission:

• article.create
  • parameter foo: bar
  • parameter bar: acme
• article.create
  • parameter foo: acme
  • parameter bar: space

Now you have 2 permissions with the same key but different characteristics. If you provide a context for example, you may check for that exact permission. Let's take a real world example - Multi Tenancy:

• article.create
  • parameter tenant: myCompany
• article.create
  • parameter tenant: notMyCompany

now, let's see if you can create a new article on the tenant myCompany:

$tenant = $tenantManager->getTenant('myCompany');
$as->isGranted('article.create', $tenant);

That's the basic idea, I'll try to find a good way for solving this :)