cakephp / authentication

Authentication plugin for CakePHP. Can also be used in PSR7 based applications.
MIT License
116 stars 100 forks source link

clearIdentity : Purpose of Response #38

Closed harikt closed 7 years ago

harikt commented 7 years ago

Hi,

I would like to know what is the purpose of clearIdentity(ServerRequestInterface $request, ResponseInterface $response); method.

I have been looking into the code and don't see any changes happening to response object.

Eg : https://github.com/cakephp/authentication/blob/f60dd14ff34e171481d557b62dbecc20e5dc0750/src/Authenticator/SessionAuthenticator.php#L90

Digged the rest of the classes via a search : https://github.com/cakephp/authentication/search?utf8=%E2%9C%93&q=clearIdentity

I think returning only Request is needed if response object is not altered .

ADmad commented 7 years ago

Yeah the $response object is currently unused. It's kept on the off change that some custom authenticator might want to modify the response too when identity is cleared.

harikt commented 7 years ago

@ADmad Do you have a real world usage of such custom authenticator adaptors ? I think it is not necessary.

josegonzalez commented 7 years ago

You could have an auth adapter that accepts a username/password and responds with a token as a header that should be used for further authenticated requests.

harikt commented 7 years ago

@josegonzalez so you mean multiple authentications happening. I have not came across in my life. If you think it is really necessary I will close this.

Thank you for the help.

ADmad commented 7 years ago

Thanks @harikt. Your interest and inputs for this plugin are really appreciated.

josegonzalez commented 7 years ago

@harikt it's a pretty normal thing to support username/password => token exchange, though normally I see it with a dedicated endpoint.

harikt commented 7 years ago

Thanks @harikt. Your interest and inputs for this plugin are really appreciated.

Sure. I am planning to dig more into this and play with zend-expressive or slim. One thing I noticed is a problem with the Router class and login url. Before I create yet another issue I want to play and see how things work :-) .

Another one I feel is dependency on orm can be moved https://github.com/cakephp/authentication/blob/b319693bcd9b6ea6eeead199e0244fefc166ed74/composer.json#L14 . So people can just use oauth adapter + this.. Anyway let me play and bring more PR or issues :) .

@josegonzalez sure, I have no / little experience with it.

Thank you guys.

ADmad commented 7 years ago

Before I create yet another issue I want to play and see how things work :-) .

Sure, do report back. Having an "outside" perspective is great.

harikt commented 7 years ago

Hi @ADmad,

I have played to configure with zend-expressive. It took me sometime to understand this probably will only work with cakephp.

One of the problem I noticed is with session : https://github.com/cakephp/authentication/blob/7b8d44b868fc76a9ef9e7d9aae4163c58641eb64/src/Authenticator/SessionAuthenticator.php#L51

I don't know if there is any interface for the session. It expects a read method. But I was using Aura.Session which doesn't have the same. So what I learned is, it is probably a bad idea to add all to the request attribute of psr-7. But make them explicit dependency. So it can be type hinted to interface.

I have not created a separate issue for I re-read the docs to understand this aims to cake only.

Commenting here for you were interested in the feedback.

Thank you

ADmad commented 7 years ago

@harikt Yeah to start with we developed the plugin with CakePHP in mind but it would be really nice to be able to use it outside CakePHP too.

What solution do you propose regarding the session issue? Add an interface?

ADmad commented 7 years ago

Also instead of the provided SessionAuthenticator you could aways make your own authenticator class which uses Aura.Session.

harikt commented 7 years ago

Also instead of the provided SessionAuthenticator you could aways make your own authenticator class which uses Aura.Session.

@ADmad sure ie also a nice alternative.

Aura does have a Auth component : It has a SessionInterface https://github.com/auraphp/Aura.Auth/tree/f83060d3004af3777f0fc2b6c6672174e6133f7d/src/Session . The SessionInterface and explicit dependencies help to understand more the underlying code.

I am not too familiar with cake may be the reason I could not make things work.

Thanks for your time.

ADmad commented 7 years ago

@harikt The plugin does provide Authentication\Authenticator\PersistenceInterface so if you want to use your own session handling lib you would make your own authenticator which implements that. Sorry for the late response.

harikt commented 7 years ago

@ADmad thanks for the information. No issues for late reply :-) . Nothing critical, just experimenting ;-) .

ADmad commented 7 years ago

The quick back and forth is more for our benefit than yours, so that we can keep moving ahead with the plugin development in a manner which will be useful to people :stuck_out_tongue:

harikt commented 7 years ago

The quick back and forth is more for our benefit than yours, so that we can keep moving ahead with the plugin development in a manner which will be useful to people

I agree with you. I have projects that are thrown for the delays happened, and sometime when choosing certain libraries we always remember the pain.

harikt commented 7 years ago

@ADmad just a quick note : When the concept points to middleware https://github.com/cakephp/authentication/blob/24ba867f12717d1e4ec1f28860b6e2fd2216e09f/src/Middleware/AuthenticationMiddleware.php (it is old one as in expressive ) . I hope you guys are aware of the psr proposals https://github.com/http-interop/http-middleware and https://github.com/http-interop/http-factory which is already used https://github.com/zendframework/zend-stratigility/blob/ff2fa693bde0d5886c3f6fff0aec92e2a0d6bb17/composer.json#L26 . May be adding something like that will help ?

markstory commented 7 years ago

@harikt We had just finished implementing the 'double pass' style when the http-interop project started and decided to go for 'single pass'. We would need to add support for the 'single pass' middleware interfaces defined in the http-interp/http-middleware in a future version of CakePHP. I think we can do that in a backwards compatible way using type checking.

harikt commented 7 years ago

@markstory yes you are right I believe. Haven't looked more on stratigility, but it has implemented this. Don't know if there is BC breaks though.

harikt commented 7 years ago

Hi guys,

Sorry for asking again. I have been looking at the PersistenceInterface again .

interface PersistenceInterface
{
    public function persistIdentity(ServerRequestInterface $request, $identity);

    public function clearIdentity(ServerRequestInterface $request, ResponseInterface $response);
}

I am having one more question : Are you guys thinking about using the $request / $response to read / write to the headers and make the session work?

I guess everyone is going to rely on $_SESSION ( in one way or the other ) itself for currently the session / cookie is not as easy as to create with psr-7. In that case we probably don't need the request and response object itself I guess.

Also it looks to me it may be a good idea not to rely too much on the withAttribute / getAttribute to store the outside objects. Proper dependencies may really help.

markstory commented 7 years ago

I am having one more question : Are you guys thinking about using the $request / $response to read / write to the headers and make the session work?

Yes, or to set cookies/response headers for token/cookie auth.

I guess everyone is going to rely on $_SESSION ( in one way or the other ) itself for currently the session / cookie is not as easy as to create with psr-7.

Right now we have a mutable object inside the request to access the session. I'm not a big fan of using $_SESSION directly as it has a few gotchas. Which is why we ended up on the session being a request attribute. Its not ideal, but its better than $_SESSION.

harikt commented 7 years ago

I'm not a big fan of using $_SESSION directly as it has a few gotchas.

You can actually use some class and wrap $_SESSION inside it. So you can also mock it.

My question was basically to understand what are the things going to be done with $request and $response which are going to be passed to the Persistence interface.

markstory commented 7 years ago

You can actually use some class and wrap $_SESSION inside it. So you can also mock it.

That's basically what the Session object does in CakePHP. We currently have it attached to the request as an attribute. We didn't originally separate the session from the request as it was more consistent to accept request/response objects and leave each authenticator to use the request/response features as they need.

Having a formal dependency on a session object would require coupling to a cakephp specific interface, as I don't think there is an inter-operable interface for sessions.