Skip to content
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

Open
chippyash opened this issue Dec 21, 2014 · 11 comments
Open

You get a mention #12

chippyash opened this issue Dec 21, 2014 · 11 comments

Comments

@chippyash
Copy link

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
Copy link
Owner

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
Copy link
Author

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 [email protected]
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
#12 (comment)
.

Kind regards
Ashley Kitson
Email: [email protected]
Tel: +44 (0)7885 376269
Skype: chippyash
LinkedIn: http://uk.linkedin.com/in/ashleykitson

@chippyash
Copy link
Author

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
Copy link
Author

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

On 26 December 2014 at 15:30, Jeremy Kendall [email protected]
wrote:

Closed #12 #12.


Reply to this email directly or view it on GitHub
#12 (comment).

Kind regards
Ashley Kitson
Email: [email protected]
Tel: +44 (0)7885 376269
Skype: chippyash
LinkedIn: http://uk.linkedin.com/in/ashleykitson

@jeremykendall
Copy link
Owner

Reopening to continue discussion ...

@jeremykendall
Copy link
Owner

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
Copy link
Owner

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 😄

@chippyash
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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):

  • IdentityInterface becomes RoleAwareInterface with a getRole method (implementation details to be determined later).
  • Add a getRoleCallback to JeremyKendall\Slim\Auth\Middleware\Authorization, allowing for a custom implementation.
    • If the callback is present, use the callback.
    • If not, use the existing getRole method.

Thoughts?

@chippyash
Copy link
Author

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 ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants