jeremykendall / slim-auth

Authorization and authentication for the Slim Framework using ZF2 Authentication and Acl components
MIT License
244 stars 38 forks source link

You get a mention #12

Open chippyash opened 9 years ago

chippyash commented 9 years ago

Hi Jeremy

Cannot see anywhere else I might do this, but thought you might like to know you get a mention at http://the-matrix.github.io/php/creating-zend-acl-definitions-from-xml/

I'm still looking to see how I can incorporate my work into slim-auth, as it's a good fit for my current project. So happy to take on any ideas.

Regards - and have a good Christmas!

Ashley

jeremykendall commented 9 years ago

Thanks, @chippyash! That's awesome!

So happy to take on any ideas.

If you come up with anything specific I can help you with, please let me know. I'll be happy to help if I can.

I'm going to close this issue, but not lock it. Feel free to use it for comments and questions.

chippyash commented 9 years ago

Sorry Jeremy, closing seems to have locked comments, or it could just by that I'm sitting on a feckin island at the moment with no data connection!

so here are my comments

OK, I can see the root problem. You have it in private function getRole($identity = null) of JeremyKendall\Slim\Auth\Middleware\Authorization.

As a preface, consider calling IdentityInterface something like RoleAwareInterface. with a getRole() [NB - no uid required - you are acting on the current identity,] method as you currently have and optionally a setRole() method. RoleAwareInterface then extends Zend\Authentication\AuthenticationServiceInterface

You have to insist that the middleware authenticator supports the RoleAwareInterface, by insisting that public function __construct(RoleAwareInterface $auth, AclInterface $acl) [NB AclInterface not Acl]

Use the interfaces to keep it loosely coupled but, in this case insisting that you have full support for Zend + plus your extension to it.

Your current getRole method is a fudge - insist on your interface. If you do this, then it becomes much simpler to be able to wire in via DI, a middleware Authorization model. It also takes a way the dependency on using a particular implementation, thus in my case, completely bypassing the dependency on a database.

If you want to be completely purist about it,abstract your middleware requirement out to a set of interface requirements and then leave it as in implementation detail as to how it is implemented. Perhaps provide your DB implementation as a a concrete example.

Regards

On 26 December 2014 at 15:30, Jeremy Kendall notifications@github.com wrote:

Thanks, @chippyash https://github.com/chippyash! That's awesome!

So happy to take on any ideas.

If you come up with anything specific I can help you with, please let me know. I'll be happy to help if I can.

I'm going to close this issue, but not lock it. Feel free to use it for comments and questions.

— Reply to this email directly or view it on GitHub https://github.com/jeremykendall/slim-auth/issues/12#issuecomment-68145120 .

Kind regards Ashley Kitson Email: ashley@zf4.biz Tel: +44 (0)7885 376269 Skype: chippyash LinkedIn: http://uk.linkedin.com/in/ashleykitson

chippyash commented 9 years ago

I can see a problem. You have it in private function getRole($identity = null) of JeremyKendall\Slim\Auth\Middleware\Authorization.

As a preface, consider calling IdentityInterface something like RoleAwareInterface. with a getRole() [NB - no uid required - you are acting on the current identity,] method as you currently have and optionally a setRole() method. RoleAwareInterface then extends Zend\Authentication\AuthenticationServiceInterface

You have to insist that the middleware authenticator supports the RoleAwareInterface, by insisting that public function __construct(RoleAwareInterface $auth, AclInterface $acl) [NB AclInterface not Acl]

Use the interfaces to keep it loosely coupled but, in this case insisting that you have full support for Zend + plus your extension to it.

Your current getRole method is a fudge - insist on your interface. If you do this, then it becomes much simpler to be able to wire in via DI, a middleware Authorization model. It also takes a way the dependency on using a particular implementation, thus in my case, completely bypassing the dependency on a database.

If you want to be completely purist about it,abstract your middleware requirement out to a set of interface requirements and then leave it as in implementation detail as to how it is implemented. Perhaps provide your DB implementation as a a concrete example.

chippyash commented 9 years ago

Sorry chap - just seen a network opening - comment posted at github.

On 26 December 2014 at 15:30, Jeremy Kendall notifications@github.com wrote:

Closed #12 https://github.com/jeremykendall/slim-auth/issues/12.

— Reply to this email directly or view it on GitHub https://github.com/jeremykendall/slim-auth/issues/12#event-212079000.

Kind regards Ashley Kitson Email: ashley@zf4.biz Tel: +44 (0)7885 376269 Skype: chippyash LinkedIn: http://uk.linkedin.com/in/ashleykitson

jeremykendall commented 9 years ago

Reopening to continue discussion ...

jeremykendall commented 9 years ago

The problem that I see with a RoleAwareInterface extending Zend\Authentication\AuthenticationServiceInterface is that the Identity and the Authentication Service are two completely separate things: One is and entity (or an array, perhaps) while the other is a service. The service lets me know if an identity exists and retrieves is from storage, but I don't know what I'm getting back.

Your current getRole method is a fudge ...

Agreed, but as the Zend\Authentication\AuthenticationServiceInterface sets no restriction on the return value of getIdentity(), then neither can I (not as far as I can see). That makes a getRole method a little more challenging to write as I don't really know what getIdentity will return. Off the top of my head, I don't see a way around that.

jeremykendall commented 9 years ago

Thanks for pointing out my use of concrete implementations rather than interfaces with Acl and AuthenticationService.

image

I'll be changing that in the next dot release :smile:

chippyash commented 9 years ago

So, what you have to remember is that you are under no compunction to support an upstream implementation. Your middleware requires the ability to get a role from an identity, hence the RoleAwareInterface. It is your requirement, not Zend's.

Your implementation can deal with the detail.

For instance, I usually do not allow Zend to save it's own, rather limited version of an identity (i.e. array,) but extend the AuthenticationService etc, to save a class that is an identity. This means that as part of the authenticate() method, your service class knows how to get a role for a just authenticated identity and can create and save an identity class.

Or, your logon routine, having authenticated ok, can get the identity and save it back again with the role implementation appended. This second method illustrated here using array notation identity:

    //get role and add to identity manually
    $role = $this->roles->getRoleForUid(new StringType($this->request->post('uid')));
    $identity = $this->authenticator->getIdentity();
    $identity['role'] = $role;
    $this->authenticator->getStorage()->write($identity);
    //or perhaps more succinctly
    //$this->authenticator->setRole($this->roles->getRoleForUid(new StringType($this->request->post('uid'))));

The call to AuthenticationService->getRole() simply queries the stored array or class and returns the role name.

    public function getRole()
    {
    $identity = $this->getIdentity();
    if ($identity instanceof RoleAwareInterface) {
        return $identity->getRole();
    }
    if (is_array($identity)) { .../etc ...}

Moving on from my earlier post, you can now create an auth service, that implements the AuthenticationServiceInterface plus the RoleAwareInterface, and simply extends the Zend/AuthenticationService to add getRole (and perhaps setRole). This further separation means that you can create an identity class also implementing the RoleAwareInterface.

chippyash commented 9 years ago

That last bit : public function getRole() implementation simply reinforces Zend's mistake in letting getIdentity() return anything. You could write

    if (!$identity instanceof RoleAwareInterface) {
        throw new \Exception();
    }

and reinforce your usage pattern.

Just a thought.

jeremykendall commented 9 years ago

So, what you have to remember is that you are under no compunction to support an upstream implementation.

Absolutely true, although my point to integrate ZF with Slim to support authentication and authorization, so I don't have a problem requiring the AuthenticationServiceInterface and supporting the upstream implementation.

For instance, I usually do not allow Zend to save it's own, rather limited version of an identity (i.e. array,) but extend the AuthenticationService etc, to save a class that is an identity.

I feel like that should be the responsibility of the authentication adapter, as whatever the adapter returns in the identity property of the authentication result is what the Authentication Service will write to storage. That requires a custom authentication adapter, but shouldn't require a custom authentication service, IMO.

The call to AuthenticationService->getRole() simply queries the stored array or class and returns the role name.

I like that idea, except I really don't like requiring implementers to write a subclass of the authentication service to make use of Slim Auth. While the Authorization middleware's private getRole method is kind of a cheat, I think it's an acceptable workaround.

Here's my proposal for a compromise solution (because I think your insights are excellent, we're just not coming to an agreement on possible implementations):

Thoughts?

chippyash commented 9 years ago

Hi, sorry about delay in response; life etc!

I feel like that should be the responsibility of the authentication adapter

Very true and in most situations I'd do the same thing. I'd been remembering a situation where this wasn't feasible and created an auth service descendent that 'massaged' responses from various disparate auth adapters into a single identity type. So, let's go with identity creation being responsibility of the auth adapter.

As to your proposal, let's go with it, though I do suggest adding a setRole() method to the RoleAwareInterface 'cus it lends more flexibility for implementers. If you want to shout when done, I'll pull the branch and put it through my app to see what it shakes out. That part of the app has been waiting on the outcome of this, so more than happy to help ;-)