-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
You get a mention #12
Comments
Thanks, @chippyash! That's awesome!
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. |
Sorry Jeremy, closing seems to have locked comments, or it could just by so here are my comments OK, I can see the root problem. You have it in private function As a preface, consider calling IdentityInterface something like You have to insist that the middleware authenticator supports the Use the interfaces to keep it loosely coupled but, in this case insisting Your current getRole method is a fudge - insist on your interface. If you If you want to be completely purist about it,abstract your middleware Regards On 26 December 2014 at 15:30, Jeremy Kendall [email protected]
Kind regards |
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. 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. |
Sorry chap - just seen a network opening - comment posted at github. On 26 December 2014 at 15:30, Jeremy Kendall [email protected]
Kind regards |
Reopening to continue discussion ... |
The problem that I see with a
Agreed, but as the |
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. |
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. |
Absolutely true, although my point to integrate ZF with Slim to support authentication and authorization, so I don't have a problem requiring the
I feel like that should be the responsibility of the authentication adapter, as whatever the adapter returns in the
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 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? |
Hi, sorry about delay in response; life etc!
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 ;-) |
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
The text was updated successfully, but these errors were encountered: