PSR-7

[bshaffer]

We should remove our Request/Response interfaces in favor of PSR-7:

https://github.com/php-fig/fig-standards/blob/master/proposed/http-message.md

[zerrvox]

Any timeline on this?

[bshaffer]

None yet, but thank you for voicing your interest

[nueko]

+1

[oytuntez]

This was among my notes about this package, great it's on the roadmap.

[benbor]

+1

[bshaffer]

here's some great reasons to switch. I'll be working on this soon. Zend's Diactoros seems to be the best library for implementing the actual request object, although this eventually will matter very little, as this library fits the middleware profile perfectly.

[bshaffer]

There is a question of how to implement error handling. The Response object is now immutable, which means the current model will not work. We will need to make the following changes:

1. AuthorizeController::handleAuthorizeRequest - returns ResponseInterface
2. AuthorizeController::validateAuthorizeRequest - returns true on success, ResponseInterface on error
3. ResourceController::verifyResourceRequest - returns true on success, ResponseInterface on error
4. ResourceController::getAccessTokenData- returns array on success, ResponseInterface on error
5. TokenController::handleTokenRequest - returns ResponseInterface
6. TokenController::grantAccessToken - returns array on success, ResponseInterface on error
7. TokenController::revoke - returns true on success, ResponseInterface on error

There is a pattern forming here, where you get a response object on error. This isn't a terrible solution, but is there a better one?

[danopz]

Maybe we should just throw some Exceptions on error and return the expected type. As right now errors result in different responses - so it will be possible using Exceptions and transform them to that response on catch.

[bshaffer]

@copynpaste I do like this solution. It's also the model implemented by firebase/jwt. However, the general sentiment exists that Exception classes should not be used for error handling and validation. Also, requiring users to surround certain function calls with try-catch is a pain.

What if we made the last parameter of these functions &$errors = null, and made this into an array of error information if the call failed. Then the Controller classes could transform these errors into Response objects.

[danopz]

And return type would stay bool. I think this would be a good solution.

[Lansoweb]

@bshaffer The problem with the &$error solution is that you need to handle different scenarios with the array. You can have just and array with an error code and message, you to add some headers like tokenController: grantAccessToken or parameters like in tokenController:handleRevokeRequest.

You will end up with a large method (or some small ones, like now) to handle all those.

What if you return the ResponseInterface like you first proposed? Than the complexities of the error response falls with each method that needs them. So, in most error cases, you will just need:

$response = $response
    ->withStatusCode(400)
    ->getBody()->write("The grant type was not specified in the request");

Or

$response = $response
    ->withStatusCode(405)
    ->withAddedHeader('Allow', 'POST')
    ->getBody()->write("'The request method must be POST when requesting an access token'");

Just need to handle content-type (html, json or xml) and body format accordingly.

If it's a redirect response, just use RedirectResponse.

[zerrvox]

@Lansoweb I really like the solution with returning response interfaces as well, then if it is already an psr-7 ready application you can just return the response directly if you don't need additional error handling.

Maybe create an extended version of the response object, OAuthErrorResponse or similar named, which can hold additional error information you can access if you want to do specific error handling.

Today In our application we are using the Symfony bridge and if it is an response with $response->isSuccessful() equals false, we alway just return it directly without doing any extra error handling. An solution with error response objects would allow similar behaviour.

Another solution could be to always return a boolean on calls such as ResourceController::verifyResourceRequest and TokenController::revoke and then make a response object available through another method such as ResourceController::getErrorResponse. ResourceController::getAccessTokenData could either return false or empty array that you would have to test against to make sure it was succesful.

In this way you could always expect handle request to return response objects and all other methods you would have to to request the errorResponse explicit.

[Lansoweb]

@zerrvox True. If the return of a method is a Response (ErrorResponse, RedirectResponse, etc), we could just return it directly.

We could have something like a ContentTypeDecorator injected to the controllers, so they can call it to format the body (son ou xml).

Well ... just saw a new branch called psr-7 with most of this already implemented =) And Brent already implemented the &$errors approach.

Another consideration @bshaffer, is that Diactoros requires php >= 5.4 (uses traits) and this lib requires >= 5.3.9, so we should increase the php minimum version with this new version.

[tthiery]

First of all +1. This issue is really important.

I am a strict advocate of using only one return type instead of "mixed" return types. Also, @bshaffer is right, for normal cases, exceptions are the wrong method (they are not exceptional then). Using an extra out parameter (&$errror) is just a strange way to implement a result with more than one value.

I would recommend returning a type with a status (true/false) property and a prepared response object property if required (like @zerrvox proposed).

Also do not be afraid of breaking your class design/contracts here (and also raise the PHP level a bit). For most users, changing to PSR-7 is anyway a big breaking thing. The bindings to this libraries anyway need to be rewritten. A nice transition tutorial is all what would be required for that.

I am using Slim Framework 2.5 and just a few days ago build a ugly bridge to this library. With Slim Framework 3.0 and a new version of this library ... yeahhhh!

[bshaffer]

I appreciate everyone's feedback on this thread. We've discussed the following possibilities:

1. &$errors (see this branch) - No.
2. Mixed return type (see this comment) - I am not a huge fan... this will end up in instance of checks after function calls.
3. combined return type - i.e. array like [ 0 => $status, 1 => $response] - I hate this also.
4. Exceptions - Not super great, as some responses require special handling, and this could get convoluted real quick.
5. Custom return type - I'm not sure I'd want this on every function

Awesome, so I hate all these options.

[codeliner]

@bshaffer I'd prefer your first suggestion: https://github.com/bshaffer/oauth2-server-php/issues/523#issuecomment-149941966 Whenever ResponseInterface is returned it should be used as response no matter if it is an error response or not.

The better alternative would be to rewrite everything as middleware. If no error occurred call next middleware otherwise return ResponseInterface

[bshaffer]

I don't think this library (which will function as a middleware) needs to have internal middlewares... seems a little malcovichian to me.

[codeliner]

These

• AuthorizeController::handleAuthorizeRequest
• ResourceController::getAccessTokenData
• TokenController::handleTokenRequest

look like different endpoints which in turn should be different middleware. But I don't know the internals of your library. Just used it together with Apigility in the past. Looking for a way to add oauth2 to zend-expressive. However, if the entire oauth2-server should act as a single middleware then +1 for: https://github.com/bshaffer/oauth2-server-php/issues/523#issuecomment-149941966

[Lansoweb]

@codeliner I'm too trying to use this with zend-expressive. For now i'm using a wrapper middleware around this library, but would be better if we have "native" support for middleware (either as 1 or 3).

[codeliner]

@Lansoweb is your wrapper middleware open source? Link? :)

[bshaffer]

@codeliner With the exception of the ResourceController method (we should probably add something like ResourceController::handleResourceRequest for this), you are correct, and those should all function as middlewares.

[Lansoweb]

@codeliner Not yet, but i can link the code somewhere. But i'm at home now (20:11), will do tomorrow at work ok?

[Lansoweb]

@codeliner It simply creates a oauth2-server from a factory and injects it in my middlewares (constructor) that are also created by factories, and each middleware uses accordingly it's necessity (much like zf-oauth does)

[codeliner]

@Lansoweb awesome! Would be nice to take a look.

@bshaffer sounds good. Let me know if you need help. Next two weeks I'll be busy with prooph components but then I have some time and we need PSR-7 oauth2 in January because we'll refactor an Apigility project to use expressive instead (oauth2 endpoints should continue to work without changes in the client) So I'd like to help to get PSR-7 support

[Lansoweb]

@codeliner @bshaffer Me too.

[Lansoweb]

@codeliner Apigility 2 will be middleware, so they will refactor other modules (zf-oauth specifically) to PSR-7, but no ETA yet ...

[basz]

@Lansoweb mind sharing that link again?

ps. I'm guessing as soon as zend-expressive lands as 1.0.0 work will commence on apigility 2.0

[Lansoweb]

@basz Hi! I needed this for a big project that is micro-service oriented, but since expressive is not stable yet (still on RC) and we still have some changes on that, we decided to stick with apigility (v1) on our micro-services. So i stopped the work on this middleware :-(

[basz]

ha, ok thanks

[Lansoweb]

@basz But there is already a branch here to psr-7, but it's not release yet: https://github.com/bshaffer/oauth2-server-php/tree/psr-7

[pabloroca]

+1 I am totally for this.

How is the status?

[xles]

+1

[lucasantarella]

+1

[northern]

+1

Just a comment on the use of exceptions for error handling. This is exactly a scenario where you would use exceptions, I mean, if exceptions are not suitable for this scenario then please give an example of when exceptions are supposed to be suitable.

Returning multiple data types is obviously a total no no. Error handling by using a parameter in which errors are returned is in my opinion clumsy at best because it allows the caller to simply ignore errors completely.

Exceptions are used when you deviate from the intended code path. In a normal circumstance the method should end at it's return statement. If something doesn't validate or is erroring otherwise and the method is prevented from completing its intended code path, that "is" the exception. I.e. an exception to the intended code path.

By throwing named exceptions will allow the caller to implement fine grained error handling, generic error handling, or, when exceptions are ignored they simply bubble up to the top. In any event, when something is supposed to work but it isn't there is no reason for the program to continue unless it's handled accordingly.

Using exceptions will also set the correct default, i.e, handle the exception or receive a general error. Using an errors parameter will make it more easy to just ignore errors completely and when an error does occur you don't have any real idea of where the error happened in the first place.

If I ignore the exceptions thrown and I suddenly see a OAuthAuthenticationException in my error logs it will give me a pretty good idea of there to start investigating the issue.

[chadicus]

If anyone is interested I've created bridge library for going to and from PSR-7 and OAuth2 requests/responses You can find it here. The library is meant to work with Slim 3, however it should (in theory) work with any PSR-7 message

[steverhoades]

@bshaffer @Lansoweb Any update on the remaining tasks, issues, status of the psr-7 branch located here: https://github.com/bshaffer/oauth2-server-php/tree/psr-7

I'd like to create a middleware server leveraging the library and would be happy to pitch in where needed.

[maryamshoeybi]

@bshaffer any update on the status of psr-7?

[kwhat]

Is this still under development?