Added AuthorizationResult

[aeneasr]

This is related to #180

I know that this PR creates some overhead, because the AuthorizationService actually implements already getIdentity but the permission's name as well as the identities roles are really really really important for many Assertions and since I do not want to break BC, I couldn't find another way. Also it's maybe not that useless, since we can now do:

if($as->isGranted('something')){
   $result = $as->getAuthorizationResult();
   // Do something
}

This is explicitly written in that matter, that we do not need to tag 3.0

[aeneasr]

ping @bakura10 @danizord

[coveralls]

Coverage remained the same when pulling 78ed9c8 on arekkas:authorization-result into 8b833c7 on ZF-Commons:master.

[aeneasr]

I actually think that it is good the way it is. The identity probably needs to be retrievable anyways from the AuthorizationService - not only when "IsGranted" gets executed. So I actually guess that this is a decent addition to current functionality

[aeneasr]

Btw @danizord :

With this it would be rather easy implementing the multi-tenancy rbac via assertions (I finally understood what you mean haha):

public function assert(AuthorizationService $as, $context = null) {
   $result = $as->getAuthorizationResult();
   $roles = $result->getRoles();
   $permission = $result->getPermission();
   $tenant = $context->getTenant();

   return /*do criteria magic here: $role->matching($permission, $tenant) blabla */ ;
}

[aeneasr]

The question is of course, wether the AuthorizationResult should be singleton (like it is now) or of multiple instances like this:

$as->isGranted('a');
$resultA = $as->getAuthorizationResult();
$as->isGranted('b');
$resultB = $as->getAuthorizationResult();

echo $resultA === $resultB; // true with singleton, false with multi-POPO

[danizord]

I agree that this can be useful, but we don't want to make AuthorizationService stateful =/

We can implement something like that in 3.0, so isGranted() would return an instance of AuthorizationResult instead of bool. But 3.0 is too far though.

[aeneasr]

Without a stateful service, I don't think that solving #180 is possible without breaking BC - it is however needed to avoid bad practices like:

• Creating assertions for permissions: MyPermissionAssertion, SomeOtherPermissionAssertion
• Recomputing already computed results: $this->roleService->getIdentityRoles()

[danizord]

@arekkas Got it, but I'm still not sure if it's the best solution. I'll think about it this weekend.

[aeneasr]

Well, we could make a container instead and register the result there. If needed, the container can be injected into the Assertion:

class ResultContainer // maybe extend doctrine's ArrayCollection?
{
   public function first();
   public function last();
   public function previous();
   public function add(Result $result);
}

class Result {
   public function setPermission($permission);
   // ...
}

class MyAssertion implements AssertionInterface 
{
   public function __construct(ResultContainer $container){
      $this->container = $container;
   }

   public function assert(AuthorizationService $as) {
      $result = $this->container->last();
   }
}

class AuthorizationService
{
   public function assert($permission, $context = null) {
      // ...
      $result = new Result($permission, $whateverElse);
      $this->container->add($result);
   }
}

[aeneasr]

Thoughts?

[bakura10]

Should be IdentityInterface $identity = null

[bakura10]

I don't like this the way it's done. Ideally, the "isGranted" method should return an AuthorizationResult but this is a BC. As said in the comment, you make the authorization service aware of the current authorization state which looks wrong to me. This is the same thing we have in Zend\Validator and we are going to move toward making them stateless in ZF3, so that each validator instead returns a ValidationResult.

Maybe that we could avoid BC if we make sure that AuthorizationResult has a __toString method which returns true or false (as currently), so I suppose that $authorizationService->isGranted() === true will still work.

This is something we must try however, I'm not sure about it.

[bakura10]

Hi,

Just read the @danizord feedback and it seems we have the same feeling about it :D.

[aeneasr]

__toString must return a string, it won't work. How about the container thingy suggested above?

[aeneasr]

The __toString() method allows a class to decide how it will react when it is treated like a string. For example, what echo $obj; will print. This method must return a string, as otherwise a fatal E_RECOVERABLE_ERROR level error is emitted.

[aeneasr]

So, there's still another option:

• AssertionInterface
• ResultAwareAssertionInterface

Where the AssertionInterface is the current one (-> no bc), and the ResultAwareAssertionInterface is an assertion aware of a Result (containing roles, permissions, etc).

In the AuthroizationService a strategy is used to determine what and how to inject:

    protected function assert($assertion, $result, $context = null)
    {
        if (is_callable($assertion)) {
            try {
                return $assertion($this, $context);
            } catch (InvalidArgumentException $e) {
                return $assertion($result, $context);
            }
        } elseif ($assertion instanceof AssertionInterface) {
            return $assertion->assert($this, $context);
        } elseif ($assertion instanceof ResultAwareAssertionInterface) {
            return $assertion->assert($result, $context);
        } elseif (is_string($assertion)) {
            $assertion = $this->assertionPluginManager->get($assertion);

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

        throw new Exception\InvalidArgumentException(sprintf(
            'Assertion must be callable, string or implement ZfcRbac\Assertion\AssertionInterface, "%s" given',
            is_object($assertion) ? get_class($assertion) : gettype($assertion)
        ));
    }

I admit that this is quite "hacky" but I don't see another way without breaking bc except for making the service stateful.

[danizord]

I see another way, just a single word: EventManager 😄

[aeneasr]

I have very little time currently, could you maybe start with an implementation? Eventmanager sounds very good to me! If you're short on time, maybe I'll manage in march to get some free time for this

[bakura10]

What about a simpler approach: we use a setter like "setCompleteValidation" on the authorization service, that if enabled return an AuthoirzationResult instead?

[aeneasr]

That could work for non-assertions, however I dislike the syntax caused by this:

$as->setCompleteValidation(true);
$as->isGranted('ab')->isValid();

But, assertions are the focus here, so I'd rather provide multiple assertion interfaces:

interface AssertionInterface {
   public function assert(AuthorizationService $as);
}

interface ResultAwareAssertionInterface extends AssertionInterface {
   public function setResult(AuthorizationResult $result);
}

And decide with a strategy what should be done. However, I don't think setCompleteValidation helps with Assertions, because we don't have the result from isGranted anyways there.

[Pittiplatsch]

@bakura10 This approach would make the AuthService stateful again, which is clearly discouraged.

[aeneasr]

No because it influences the returnvalue of isGranted

[Pittiplatsch]

Right. It influences the return value - but not only for the one intended call of isGranted, but transparently also all other calls (of isGranted) by other callers which might not be prepared for this return value "modding".
Storing any information within a class which influences its behaviour transparently isn't desired for any kind of service => services should be stateless...

[aeneasr]

Got me :D

[bakura10]

What about a new method "isAuthorized" that lived next to isGranted?

isGranted only returns a boolean while isAuthorized returns a more complete object?

[danizord]

@bakura10 authorize() plz!

[bakura10]

I'm okey with "authorize".

[aeneasr]

Guys, it's not about the returnvalue of isGranted, it's about assertions knowing which permission was called (at least this PR is)!

But +1 on authorize.

[aeneasr]

What do you think of the ResultAwareAssertionInterface idea?

[bakura10]

It is. I am trying to find a way to do it by keeping the stateless and without breaking any BC. So the only solution is to add a new method to the AuthorizaitonService.

[bakura10]

Ha. I think I may have misunderstood the PR then. Your goal is to inject something into each Assertion? So yes, a ResultAwareAssertionInterface is a good idea.

[aeneasr]

The problem is the AssertionInterface, which does not allow any kind of result to be passed:

public function assert(AuthorizationService $service, $context = null) {
   // how do I get the called permission?
}

[aeneasr]

Ok cool, @danizord @Pittiplatsch , ok with ResultAwareAssertionInterface too? I'd propose this signature for an AuthorizationResult:

class AuthorizationResult
{
   public function isValid(); // maybe isGranted is better?
   public function getPermission(); // returns the permission that has been used
   public function getRoles(); // returns the identities roles (actually only useful for "guest")

   public function getIdentity(); // avoids possible future BC breaks
   // public function getMessage(); // "permission not granted" or "rejected by assertion" not sure if that's useful tho :D

   // anything else? ...
}

Updates:

• Kept setIdentity

[aeneasr]

Oh, I understand why this PR got misunderstood, should have used a better description. My bad, sorry!

[Pittiplatsch]

@arekkas AuthorizationResult might be an option.
However I'm not sure about getRoles. I indeed favor getIdentity instead to have an explicit reference to the identity the permission check was performed for. (I see concrete use cases where another but the current identity used.)
If anybody (e.g. an assertion) actually needs the roles, he could then gather them via the authorisation result's identity.

[aeneasr]

Not if the identity is null (-> guest):

returns the identities roles (actually only useful for "guest")

[Pittiplatsch]

Agreed. But keep getIdentity as well plz.

[aeneasr]

Yeah could be useful if the AuthService signature changes in favor of something like setIdentity() (which currently only can be set by the constructor).

[danizord]

@arekkas I like your ResultAwareAssertionInterface idea, but I'm pretty sure that will not be enough/best to way to handle your problem of language check, because the AuthorizationResult will know all the identity roles, but not specific granted role. So you'll need to check language+permission against the whole role collection again, which is clearly overhead.

Also, the authorize() thing will need a lot of hacks to be implemented, because the method AuthorizationService::assert() will need to know to AuthorizationResult but we can't add one more argument to its interface without breaking BC (that method is protected). So, we'll need to add a private $result property in AuthorizationService to make it work (hackish).

The EventManager thing can solve your problem easily because it will allow you to attach your own check logic, but that will be also hard to implement without breaking BC =/

So, I'd suggest you to extend Rbac\Rbac to implement your check logic, then inject it in AuthorizationService.

[aeneasr]

Thanks @danizord , actually I've already done that (well, actually I extended the AuthorizationService as of yet). There's still room for improvement (like you said, I need to iterate again through the whole role tree) so extending Rbac is probably the wisest choice. But for now, everything works and I can fullfill my deadlines :D

Making Rbac not casting every permission to string is actually a huge deal for this, because I'll probably just extend the isGranted($permission, LanguageAwareInterface|mixed $context = null) method and inject a PermissionRepository into it and check with rbac against that specific object.

[danizord]

@arekkas great! So we can have more time to discuss and think about that proposals to ZfcRbac 3.0.

[aeneasr]

Absolutely!
Regarding events: I think they are super useful, but I am unsure if using events (and therefore very strong decoupling) couldn't actually hurt more than it helps - in regards to a security suite. Especially refactoring any listener logic or bc breaking the trigger logic is always a pain (identifiying all listeners etc) and most commonly leaves many hard-to-find bugs (:= unwanted access) along the way. However I'm open to other viewpoints on this :)

To be honest: The assertion map seems to be also quite "lash" - you forget to add an assertion to the assertion map and the whole auth logic for that permission is broken.

[danizord]

I'm thinking about something like the BaconAuthentication approach, that seems quite flexible yet secure.

[aeneasr]

Looks cool, I especially like the challenge part - this one has been in the back of my head for quite some time now. Maybe we should provide a FloodGuard/ChallengeGuard as well :D

If we provide a custom event, I think that the messaging could get implemented in a way that refactoring is possible. +1

[bakura10]

+1 for a RateLimitGuard :D. We should provide more built in guards for typical web use cases.

Envoyé de mon iPhone

Le 8 févr. 2014 à 10:29, arekkas notifications@github.com a écrit :

Looks cool, I especially like the challenge part - this one has been in the back of my head for quite some time now. Maybe we should provide a FloodGuard as well :D

If we provide a custom event, I think that the messaging could get implemented in a way that refactoring is possible. +1

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

[danizord]

👎 Nothing to do with RBAC. I'd suggest create another module to put that stuff.

[aeneasr]

Hi, this PR has gotten a bit OT, I'll try to revive it in the next couple of days as I'm still confident that it is neccessary to know the permission's name. Got some reading to do first, but I'll try to summarize the results here and if everyone agrees, I'll implement it.