laminas-api-tools / api-tools-oauth2

Laminas module for implementing an OAuth2 server
https://api-tools.getlaminas.org/documentation
BSD 3-Clause "New" or "Revised" License
11 stars 15 forks source link

OAuth2ServerFactory is not returning instance of OAuth2\Server #14

Open weierophinney opened 4 years ago

weierophinney commented 4 years ago

The ZF\OAuth2\Factory\OAuth2ServerFactory is no longer returning a OAuth2\Server instance?

https://github.com/zfcampus/zf-oauth2/blob/master/src/Factory/OAuth2ServerFactory.php

The file has been changed in the latest version and it returns a closure (Factory!?) now. My code is broken since I simply used $serviceLocator->get('ZF\OAuth2\Service\OAuth2Server'); elsewhere to get the OAuth2\Server instance from the server manager.

The documentation has also not been updated accordingly and is no longer up to date since it still states it will give me an OAuth\Server.

Is returning another factory from a factory really the way to go? How should I now get my oauth server instance? Is it maybe possible to separate the keys so getting the server is still possible?


Originally posted by @Wilt at https://github.com/zfcampus/zf-oauth2/issues/114

weierophinney commented 4 years ago

My code editor also doesn't agree on a closure being returned by ServiceManager :) I get big fat red lines under my DefaultAuthenticationListenerFactory class.

Deep down in the ZF2 ServiceLocatorInterface the return values from a service manager should be either an object or an array:

    /**
     * Retrieve a registered instance
     *
     * @param  string  $name
     * @throws Exception\ServiceNotFoundException
     * @return object|array
     */
    public function get($name);

In the ZF\MvcAuth\Factory\DefaultAuthenticationListenerFactory the following is done:

return new OAuth2Adapter($factory()); (https://github.com/zfcampus/zf-mvc-auth/blob/master/src/Factory/DefaultAuthenticationListenerFactory.php#L105)

That is why I get the following error comment: Function name must be callable - a string, Closure or class implementing _invoke, currently array |object

I also get an error here

return new OAuth2Adapter($serverFactory(null)); (https://github.com/zfcampus/zf-mvc-auth/blob/master/src/Factory/DefaultAuthenticationListenerFactory.php#L128)

Since the php doc is not correct it looks like we are instantiating OAuth2\Server class here which is not possible.

I think it would be better to return the OAuth2\Server instead of the factory here or at least make sure all documentation is up to date so that it makes sense to my SDK. I can make a PR if necessary.


Originally posted by @Wilt at https://github.com/zfcampus/zf-oauth2/issues/114#issuecomment-125589143

weierophinney commented 4 years ago

@Wilt PR ? :)


Originally posted by @jguittard at https://github.com/zfcampus/zf-oauth2/issues/114#issuecomment-228500229

weierophinney commented 4 years ago

@jguittard I was awaiting response from someone working on this repository to get some feedback on how to improve this...

@weierophinney Do you have time to take a quick look and make a suggestion on how to deal with this in a PR?

IMHO it would be good if this essential part of the zf-mvc-auth module becomes a bit more readable/understandable... All these $factory calls inside other factories are a bit ambiguous. As I mentioned before my code editor also doesn't agree with it:

image


Originally posted by @Wilt at https://github.com/zfcampus/zf-oauth2/issues/114#issuecomment-229278352

weierophinney commented 4 years ago

@Wilt This was done due to issues with the design of oauth2-server. Essentially, the OAuth2\Server instance does work on initialization that can lead to issues with the HTTP request, database connections, and more. We had to provide a way to lazy-load it to solve the problems.

We can maybe solve the IDE issues with typehints, and some of these may be solved now (I did some work on the AuthControllerFactory earlier today). But I'm not sure we can make any other changes unless we change out the oauth2 server infrastructure or get changes pushed upstream to oauth2-server-php.


Originally posted by @weierophinney at https://github.com/zfcampus/zf-oauth2/issues/114#issuecomment-231218728

weierophinney commented 4 years ago

@weierophinney Thanks for the explanation.

Would it be possible to use a lazy services pattern (using delegators) for this instead of this invokable factory? Or is this not sufficient to solve those issues you refer to?


Originally posted by @Wilt at https://github.com/zfcampus/zf-oauth2/issues/114#issuecomment-231294779