remove identity provider & improve compatibility with zf2+3

[basz]

• Are we in agreement that isGranted($identity, $permission, $context = null) will work?
• I'll assume a NULL identity is allowed which would imply the guest role

The authorisation service simply call the role service but now provides the identity

// ZfcRbac\Service\AuthorizationService
    public function isGranted($identity, $permission, $context = null) {
        $roles = $this->roleService->getIdentityRoles($identity);
    ...

And the main change to the role service is be the removal of the identity provider and its getIdentityRoles method would now accept an identity.

// ZfcRbac\Service\RoleService
    public function getIdentityRoles($identity)
    {
        if ($identity === null) {
            return $this->convertRoles([$this->guestRole]);
        }
    ...

[bakura10]

That looks wonderful to me. @danizord anything to say?

[bakura10]

Should be something like IdentityInterface|string|null (or mixed) ?

[bakura10]

Use Yoda comparison.

[danizord]

@basz and @bakura10 what do you think about removing the whole "identity" concept from this package and work only with roles?

Currently we have "identity" for 2 purposes:

• Fetch roles;
• Pass $identity to assertions.

The second, passing $identity to assertions allows developers to write some custom logic for authorzation checks. While this is flexible, I think checking authorization rules against $identity is far from role-based access control. So, I think it does not fit this package.

If we get rid of identity-aware assertions, it would make no sense to require an identity implementing IdentityInterface, we could skip this step and tell developers to write their own role providers as middlewaes:

namespace UserLand\Middleware; 

class MyCustomRoleProviderMiddleware
{
    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next)
    {
        $roles = /* Custom logic to fetch identity roles */;

        return $next($request->withAttribute('roles', $roles), $response);
    }
}

namespace UserLand\Action; 

class SomeAppAction
{
    /* ... Inject ZfcRbac AuthorizationService dependency ... */

    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next)
    {
        // ZfcRbac works here
        if (!$this->authorizationService->isGranted($request, 'somePermission')) {
            throw new YouShallNotPass();
        }

        // Actually do some app action
    }
}

namespace ZfcRbac\Service;

class AuthorizationService
{
    /* ... Inject Zend\Permissions\Rbac ... */

    public function isGranted(ServerRequestInterface $request, $permission)
    {
        $roles = $request->getAttribute('roles');

        return $this->rbac->isGranted($roles, $permission);
    }
}

Sounds sane?

[basz]

To me this does sound sane. nish, I think...

One thing that bothers me a little is that we are placing potentially a large blob of data on the request. Since these are immutable that might take a performance hits when cloned (multiple times).

Additionally - not specific to this package - placing stuff on the request seems handy dandy but am I the only one being paranoid if I worry about that would turn out to be a new global/static anti pattern?

[danizord]

One thing that bothers me a little is that we are placing potentially a large blob of data on the request. Since these are immutable that might take a performance hits when cloned (multiple times).

Since $identity has roles inside, storing only the roles will actually decrease the amount of data. But honestly I don't care about about performance at this point.

Additionally - not specific to this package - placing stuff on the request seems handy dandy but am I the only one being paranoid if I worry about that would turn out to be a new global/static anti pattern?

Nope, actually this is a very powerful concept because we have no super-globals nor stateful services. When you tie all the request context to the request object, you allow your application to work also for multiple isolated dispatch cycles. PSR7Session is a good example of this concept, it allow us to run our app in long running processes like React, since all the request context is dropped when you start a new request.

[bakura10]

@basz for now I'm thinking the same thing as you. I feel a bit bad about packing so much things into the request and it feels like a new Zend_Registry. But at the same time I understand that this is the only and and an elegant way to pass info between middlewares. Your thoughts @weierophinney ? Was it the intended purpose of that?

Regarding removing identity, the problem is assertion. I DEFINITELY want to keep that. I mean, 99% of assertions are doing that:

if ($identity->getId() === $post->getId()) {

}

I don't want to get rid of this behaviour, as it's nearly the only use case of assertions. Or did I misunderstood soemthing?

[basz]

@danizord thanks for clearing that up. @bakura I am reassured danizord is correct in the use of request attributes, but lets hear if @weierophinney has something to say on this.

If 'assertions' would be zapped from this package - which strictly speaking don't seem to belong in this package indeed - what would you propose to handle assertions as these are a definite requirement of any application. An AuthorizationService library that would be able to handle dynamic assertions, rbac and/or acl?

Almost feels zfc-rbac is a too strict name.

[danizord]

I'd like to summon @Ocramius as well to give us his opinion :P

[danizord]

Regarding removing identity, the problem is assertion. I DEFINITELY want to keep that.

About the assertions, I don't think it fits ZfcRbac because it's not role-based access control. But yeah we can keep assertions there and pass the entire request object to assertions, instead of $identity and the mixed $context. So that assertions can grab identity or any other context information from the request object.

namespace UserLand\Authorization\Assertion; 

class IsPostOwnerAssertion
{
    public function __invoke(ServerRequestInterface $request, string $permission) : bool
    {
        // Note that identity attribute is not forced by ZfcRbac
        $identity = $request->getAttribute('identity');
        $postId  = $request->getAttribute('postId');

        return $this->postService->findPost($postId)->getOwnerId() === $identity;
    }
}

Yeah, it couples your authorization stuff to HTTP messages, but I really see this new version of ZfcRbac as an authorization library specific for HTTP layer.

I feel a bit bad about packing so much things into the request and it feels like a new Zend_Registry

The problem of Zend_Registry is that it was a global singleton. The request object is not global, it holds all the context information about a single request lifecycle, and you can dispatch multiple request objects each with its own set of informations.

[Wilt]

@ALL Please don't drop assertions and dynamic roles, rather change the name of the package...

I drop in from #318

I like @basz his suggestion. His RoleService is the same concept as my RoleResolver (see #318)

Regarding @danizord his comment:

the problem is assertion. I DEFINITELY want to keep that.

How does that become an issue?

My current (custom) assertion solution uses IdentityInterface. Why use ServerRequestInterface?
My AssertionInterface looks something like this:

<?php

namespace Rbac\Assertion;

interface AssertionInterface
{
    /**
     * Check the assertion
     *
     * @param IdentityInterface $identity
     * @param object $resource
     * @return bool
     */
    public function assert(IdentityInterface $identity, $resource);
}

Is it strange to wish that roles could be accessible through $identity?

$roles = $identity->getRolesForResouce($resource);

[Wilt]

My assertion config looks like this:

        'resource_privileges' => array(
            // Resource class name
            Resource::class => array(
                // Role name
                'Admin' => array(
                    // Action name - assertions (null, string or array of strings (multiple assertions))
                    // null - No additional assertions
                    'delete' => null,
                    'update' => null
                ),
                'Owner' => array(
                    'read' => null 
                    // array - Multiple assertions
                    'update' => array( 
                         'Application\Assertion\AssertResourceStateIsActive'
                         'Application\Assertion\AssertOwnerHasBlondHair'
                    )
                ),
                'Member' => array(
                    // string - single assertion
                    'read' => null,
                    'update' => 'Application\Assertion\AssertResourceIsShared'
                )
                'Guest' => array(
                    // string - single assertion
                    'read' => 'Application\Assertion\AssertResourceIsPublic'
                )
            )
        )

How would this look? Or are there examples already available?

[basz]

My current (custom) assertion solution uses IdentityInterface. Why use ServerRequestInterface?

@Wilt as the next version of zfc-rbac will be PSR7 Middleware we can diminish reliance on stateful services while doing the work. Instead we get the identity and/or roles early (from a service or whatever) and place that information on the request object. It then makes sense to pass the request object to any assertions.

Is it strange to wish that roles could be accessible through $identity?

Hmm, the concept of resources. While this is fine if it works for you I'm fairly sure resources shouldn't belong in this package. Are you sure you aren't using ACL permissions? If the identity interface would be removed from this package you can do what ever you want I would suggest :-)

acl vs rbac "RBAC differs from access control lists (ACL) by putting the emphasis on roles and their permissions rather than objects (resources)."

[Wilt]

I know this ACL vs RBAC discussion. But I have an application with roles and resources. The resource privileges depend on a known list of roles (Admin, Guest, Owner, etc). But the roles for the current Identity can differ depending on the resource instance (same resource class).

An example:
So for Resource with id 1 I am owner, so I am allowed to read and delete.
For Resource with id 2 I am a Guest so delete is never allowed, but I can read if resource is public (assertion ResourceIsPublic). Try to squeeze that in any available ACL solution.

My thought was that such an authorization concept is nothing revolutionary and I am really waiting for a supported ZF2 Module that delivers this out-of-the-box. I was hoping RBAC would be my solution. But apparently that was "wishful thinking"...

If you know an alternative that suits my need please let me know...

[bakura10]

@Wilt this problem can actually be solved using assertions.

[bakura10]

Hi again,

I've thought a bit more and definitely, I don't want to get rid of assertions. I don't want this module to be some lightweight that it will require an other library just to perform what belongs to all apps that need authorization (and actually I'm not against creating a new repo zfr-authorization if the fact that the module contains rbac is annoying you).

The thing I don't like about that:

class IsPostOwnerAssertion
{
    public function __invoke(ServerRequestInterface $request, string $permission) : bool
    {
        // Note that identity attribute is not forced by ZfcRbac
        $identity = $request->getAttribute('identity');
        $postId  = $request->getAttribute('postId');

        return $this->postService->findPost($postId)->getOwnerId() === $identity;
    }
}

Is once again the fact that our "controllers" (middleware, or whatever), will just pollute the request with a lot of info. While you've convinced me for identity as it's could be thought as part of the current request, storing a post ID or something like that feels wrong.

Maybe we could go the path of using a ParameterBag to pass as the isGranted method, so that we avoid polluting the request with unrelated stuff.

@Wilt : your use case definitely makes sense, and whenever I had to face this I had the same complexity, where I needed to write complex assertions. But it was possible.

As said, AWS IAM is one of the best authorization service I know: http://docs.aws.amazon.com/IAM/latest/UserGuide/access_permissions.html

They have good abstractions around request, conditions... (http://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_evaluation-logic.html) that we may re-use at some point, or we could even think a new architecture around those abstractions, so that AWS documentaiton would also apply to us =).

[Wilt]

And something like the MvcAuthEvent as they use in zfcampus/zf-mvc-auth? Would that be a solution to get roles, identity and resource together at one central spot?

For resolving the resource they use a ResourceResolverListener. Like this you could also easily add a RolesResolverListener.

---

About the complex assertions that @bakura10 is talking about. Splitting it into a role-assertion config was really the only way to do this. To illustrate this I added a Member role to my previous example. If you want to do update in one assertion you get:

AssertUserIsAdminOrUserIsOwnerAndHasBlondHairAndResouceStateIsActiveOrUserIsMemberAndResourceIsShared

in other words: AssertUserIsAdmin or UserIsOwnerAndHasBlondHairAndResouceStateIsActive or
UserIsMemberAndResourceIsShared in one assertion...

All the ands and ors make such an assertion impossible to understand and is bound to leas to mistakes (authorization errors). I think assertions should be nicely separated in small understandable tasks following the single responsibility principle.

[bakura10]

Same for this one @basz and @Orkin :)

[basz]

I have a new use case I want to post for reference. The application I am developing is a prooph application build with expressive.

So I decorate the request object with an identity instance from a simple middleware on the appropriate routes.
Prooph has its own middleware to map routes to commands and dispatch these commands into a command bus. A special plugin gathers (the identity, amongst other things) information from the request object and passes that as metadata with the commands (or queries) to the bus.

Since all data going into a bus must be serialised no instances may be passed. For the identity we only pass the identifier then.

In the command handlers we would need to get the identity instance again from the metadata via an application service to pass to any is granted methods.

This works different from the 2 version because there we have an identity service that would contain identity state.

[bakura10]

Hi,

Passing the identifier sounds correct but I'm not sure to understand. You mean that this identifier is passed to the handler?

Then you should have a service to retrieve the user from the id, but I don't think this is the responsability of ZfcRbac to provide that.

@danizord ?

[svycka]

what benefits we get from removing IdentityProvider and IdentityProviderInterface? I see this may be good for middlewares, but what if we don't use them or we need to check access in services we will need one more dependancy like IdentityService or Request. Maybe I am missing something?

Maybe before merging this write few examples how this will work or some kind of migration guide?

[basz]

@svycka A lot of functionality has been removed from this package, even before this PR (i expect some pushback on that).

I hope I can explain it correctly as I did not think of it, but I think the main argument was to remove internal state and decouple from zend authentication service. But yes that does leave getting an identity up to the user which should be documented.

[svycka]

just saying that it's hard to grasp benefits when there is not real examples how this will be better than before. :D

I think the main argument was to remove internal state and decouple from zend authentication service

I have successfully used this module without zend-authorization in fact I never used it with zend-authorization so this was possible and before so this change does not convince me.

I hope soon I will see why this is better :D

[basz]

@bakura10 you able to convince svycka? what's the rationale of moving identity out of this package?

[basz]

Identity can come from various places at different times in an application. It might come from a oauth token or a user_id. Via some middleware or a cookie/session hook or in a command handler that sits behind a different process. One might need a IdentityProvider per context/location which would complicate. I could imagine a plugin service for that, but that should be a separate PR IMHO (this one too big already).

[svycka]

@basz i understand that this is a lot more flexible, but also more complex when you don't need that flexibility because you have just one identity per request and IdentityProvider can give you that identity does not matter where it comes from cookie/ session, oauth token or anything else. But I guess flexibility has higher priority in this module :D

[bakura10]

To be honest it's been months I didn't use this module so I may be a bit lost.

But the rationale is to keep the module focused on what it is supposed to do, and favour stateless services.

In this case, the only thing that a service that check roles would need is to retrieve the roles, not the identity.

To that extent, we should feed to the service that check the permission the identity. This Role service does not have to know how to retrieve the identity, its not its task.

Indeed, identity can now come from a lot of various sources, and we want to try to embrace what is doing now. You may have your identity stored in a cookie, you could have an identity persisted in a JWT token... its really up to your application to decide how to extract this identity, and it's also up to you to then call your database to retrieve it.

I don't have much more rationale expect that we should try to reduce dependencies and improve flexibility :p. Writing a small middleware that get your identity from your database is easy, short, and replace advantageously the confusing usage of something like Zend\AuthenticationService.

[danizord]

Hey everyone, let me give my 2 cents here.

The problem of IdentityProvider is that it holds the authenticated identity in its inner state, so that you can grab it from whenever you want as soon as you inject the IdentityProvider. While this is very convenient (no need to pass identity via arguments), this also brings some drawbacks:

• It makes implicit a concept that should be explicit - authorization requires an identity to be authorized, so you should always pass an identity to AuthorizationService instead of expecting that it would automatically fetch it from somewhere else.
• It makes debugging and testing hard because your application won't follow the functional approach of middlewares anymore, as you'll need to rely on side effects of an authentication middleware that should run before everything and setup your IdentityProvider with an authenticated identity.
• It makes your application impossible to run with multiple dispatch cycles using some long running process like React or PHPFastCGI

So, my suggestion is to make everything stateless, store authenticated identity as PSR-7 request attribute, use guards as configured middlewares and always pass $identity as argument to AuthorizationService.

[basz]

Not everything is tested as it should, but I have implemented this quickly into my application and it seems to operate fine. So for i've tried the in-memory and repository role providers (flat and hierarchical) and custom assertions. I've been used to bjynauthorize, but this is much cleaner...

Invite to all for a review. hopefully we are able to merge this soon and continue to expand (Adding docs or features such as ZendAuthenticationMiddleware) on this ground work.

[Wilt]

I like the changes so far, but I have one request...

Can we pass $context also as a parameter to the getIdentityRoles method. In our case roles depend on the resource ($context) and it would be great if we can pass it.
It could of course be null by default, so nothing changes for people who are not interested in $context inside the RoleService. But customized RoleService instances could use the parameter optionally inside custom logic.

So this line would change to:

$roles = $this->roleService->getIdentityRoles($identity, $context);

And in RoleService the getIdentityRoles method would change to:

public function getIdentityRoles(IdentityInterface $identity = null, $context = null)
{             
    //...
}

[basz]

You're using a custom RoleService then?

I see no problem with such an approach. I believe we should introduce a RoleServiceInterface in the AuthorizationService because then it becomes easy to swap out a RoleService with one that can do what you want.

However it would be better to postpone such a feature after merging this via a new PR. Could you create an issue to that end? Marked for 3.0.0 or perhaps it can be implemented for the current version as well.

[basz]

You're using a custom RoleService then?

I see no problem with such an approach. However I think it would be better to postpone it after merging via a new PR. Could you create a issue for this. Perhaps it can be implemented for the current version as well.

Op 30 aug. 2016 om 15:54 heeft Wilt [email protected] het volgende geschreven:

I like the changes so far, but I have one request...

Can we pass $context also as param to the getIdentityRoles method. In our case roles depend on the resource ($context) and it would be great if we can pass it... It could of course be null by default, so nothing changes for people who are not interested in $context inside the RoleService.

So this line would change to:

$roles = $this->roleService->getIdentityRoles($identity, $context);
And in RoleService the getIdentityRoles method would change to:

public function getIdentityRoles(IdentityInterface $identity = null, $context = null)
{
//...
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[danizord]

Can you provide a default [] if not set?

[svycka]

@danizord there it is https://github.com/basz/zfc-rbac/blob/master/config/module.config.php#L88

[basz]

@danizord still want this? Should be added to other factories as well then...

[danizord]

nvm :)

[Wilt]

Thanks for including that feature request 👍

[Wilt]

Yes we use a custom role service. Thanks for adding this feature.