-
Notifications
You must be signed in to change notification settings - Fork 111
Embracing middlewares #307
Comments
@bakura10 awesome! I was a bit unsure about the future of Rbac component and ZfcRbac in this middleware age, but your ideas motivated me again :D Absolutely 👍, finally we can have We could provide some middleware abstraction to replace 2.x guards: // You need "access_admin" permission to access /admin/* routes
$app->pipe('/admin', new PermissionGuardMiddleware('access_admin')); Tell me how I can help (I know, I owe you some migration docs, heh. Will finish it by the end of the week ;-)) |
That's a nice idea Daniel! Well how you can help me? First you need to finish your rbac PR so we can have it as a Zend component. And then you can start implementing those ideas. I'm still overwhelmed with front end work for months so I don't have much time for working on PHP stuff unfortunately ;( Envoyé de mon iPhone
|
Any news @danizord about the work you could do for Rbac ? :) |
👍 I actually left php for Go. Middleware was one of the many reasons ;D |
Oh ok… sad to see you leave, so I’d just want to thank you for all the work you’ve done on ZfcRbac, if the library is as good and stable as it was it was also because of you :).
|
Thanks man, appreciated! Keep on rocking! 👍 |
@bakura10 Working on that right now. Will push some stuff tomorrow or next weekend ;) |
@danizord , could I have your help to update all the tests? We've been quite tricked by ZfcRbac test suite that relied heavily on the module and service manager, which now is a big hassle :(. I'd like to make the test suite pass first before doing more refactoring and fixing design issues. Here are the things that need to be fixed in tests:
|
@bakura10 sure, but I have to leave now and I will be back later tonight, ok? |
Sure! |
Hi @bakura10 can I offer some assistance here? Anything simple to get me started. I'm anxious to get into middleware style applications and this seems like a good starting point. |
Sure. The first thing right now is to start fixing tests (you could make some PR to this branch: #312) Also, it would be nice to think about the general architecture. As outlined in the other discussion: I'm not sure to understand how the flow would work actually. The current identity is provided by the "RoleService", which itself has a dependency to an IdentityProviderInterface (which used to use Zend\Authentication). Because we are using a middleware approach, I'd think that the good approach would be to have a middleware that would do the authentication logic and store the logged user (if any) as part of the request attributes. However how the RoleService would retrieve the request to get the identity? The workflow is a bit confusing for me :(. |
ke, made a start with some of the factories tests. |
Yes please :). |
You say : Util likely to be removed... Seems only ZfcRbacTest\Role\ObjectRepositoryRoleProviderTest uses ServiceManagerFactory::getServiceManager() currently. I assume mocking will do... shall I remove it? |
Thanks @basz for your work. So now, we need to find a way to find a way to using middleware. My initial thought was to actually add the identity as part of the "isGranted" signature in the "AuthorizationServiceInterface". However this does not work because you may not have an identity, so there is still logic that must be performed by the RoleService to eventually use the guest role. My only idea right now is to write a middleware (that will need to be inserted as a pre-middleware into the Expressive pipeline) that will read into the request for a "identity" attribute. If there's one, it will set the identity as part of the RoleService. I'm not a big fan of this idea because it makes the identity part of the role service, actually. In all cases, this means that the user will be, at some point, responsible for authenticating the user, and modify the incoming request with an attribute that may (or may not) be configurable, but we could use a sane name like "identity". If you or @danizord think of a different idea, ... :) |
Any news on what you wanted to do @danizord ? :) |
@bakura10 yeah news coming this weekend :) |
@bakura10 is this in line with what you are considering? Provide a (multiple) middleware's to retrieve an identity and that set it (if an identity exists) on the request. https://gist.github.com/basz/b78e3da1ebb36bf46290 In any other middleware executed after this one you could do;
thoughts;
|
Yes, but my only problem is the workflow. Let's say you are in a controller. This is typically there that you will check your permission: class MyControllerMiddleware
{
public function __construct(AuthorizationServiceInterface $authService)
{
$this->authService = $authService;
}
public function __invoke($req, $res, $callable)
{
if (!$this->authService->isGranted('readSomething')) {
throw new NotAuthorized();
}
}
} As we can see, the auth service is... well... a service. We just give a permission name. The AuthService will actually delegate its work to the RoleService to extract all roles from the given identity (or the guest identity if none is found). But as the RoleService is not a middleware it does not have access to the request. So as we've thought before, one of the solution would be to pass an identity (either an IdentityInterface or a string) to the isGranted, so that if basically becomes $this->authService->isGranted($identity, 'readSomething'). But this means that this logic: $identity = $request->getAttribute('identity', 'guest'); // defaults to guest if none found Will need to be either duplicated whenever you want to check a permission (we definitely do not want that), or delegate that to your other middleware, so that it ALWAYS fill the "identity" property beforehand, even when no identity is found. Which is what you were bascially suggesting. Not sure if I'm clear :o |
With duplicating I presume you mean you don’t want the consuming user to remember to use ‘guest’ as default value? Can't we simply treat the absence (null) of an identity as ‘guest’? authService could do that before delegating to RoleService? $identity = $request->getAttribute('identity’); $this->authService->isGranted($identity, 'readSomething’); |
I thought about that, but this signature bothers me: public function isGranted($identity = null, $permission, $context = null); of course we could reverse: public function isGranted($permission, $identity = null, $context = null); But it feels a bit wrong, I feel that identity should come first. You grant an identity with a permission, not the opposiite :o. |
?
this works |
Oh I'm stupid. That's what I win at doing front-end work only since 1 year :D. |
someone needs a kick under their back-end… :-p |
Hi everyone,
In overall, this package has been very stable over the last few months, and I'd like to think about the future of it.
With the increase of API, and tools like Zend\Expressive, it may be interesting to simplify this package. I'd like to support two different branches: 2.x that would be based as a ZF2 module, and a 3.x branch, that would remove dependency completely toward ZF, and embrace middlewares and an API first design.
Here are the various things I'm thinking, let me know if you have any suggestion.
Removing guards
Guards were nice, but to be honest, they were a bit problematic. They were tied very strongly to the ZF2 router / ZF2 MVC model, and while useful to "guard" a bunch of route (/admin/*), the advent of middlewares allow to create much better solution to this issue.
Also, guards introduced potential security issue, as some methods could be called outside the context of a route/controller, hence bypassing the guard.
This also made testing harder, because, well... testing taht kind of things is hard because it's 100% config.
Instead, we should encourage people to check their permissions at the controller/service level. I've always done it at the controller level, and it was super nice. Very easy to see which permissions were needed, very easy to test...
** Removed dependencies **: zend\mvc
Removing views
As a package that would be API driven does not need the views anymore.
** Removed dependencies ** : zend\view
Removing collector and various ZfTool
No longer really needed, and we don't have yet a tool for that in middleware's world.
Authentication
ZfcRbac would no longer rely on Zend\Authentication. Instead, the
isGranted
signature would be changed so that the first parameter is anIdentityInterface
:Consumer would be responsible to extract it. In PSR-7, each request can be set attribute, for instance here is a possible controller:
Maybe we could provide some simple, common interfaces for retrieving logged identity, I'm not so sure.
ping @danizord @weierophinney
The text was updated successfully, but these errors were encountered: