bshaffer / oauth2-server-php

A library for implementing an OAuth2 Server in php
http://bshaffer.github.io/oauth2-server-php-docs
MIT License
3.26k stars 952 forks source link

ResourceController::getAccessTokenData fails with laravel/lumen #576

Open tthiery opened 9 years ago

tthiery commented 9 years ago

I am implementing an OAuth2 server using Laravel's Lumen framework but fail to create a tokeninfo endpoint. I utilize the ResourceController::getAccessTokenData method but cannot access it when invoking using /tokeninfo?access_token=23412341.

The method internally calls Bearer::getAccessTokenParameter which returns a null instead of the access token. Why? Because it counts the Illuminate\Http\Request->query and ->request both. Unfortunatey, the Laravel project has recently redefined the meaning of request ParameterBag from containing the form data to containing the primary input method leaving the query AND the request parameter bag containing the access token which again the OAuth implementation reject of not standard compliant.

I do not know, if this is the right project to file the issue, but I think here it has a immediate effect

Issue in the Bearer class: https://github.com/bshaffer/oauth2-server-php/blob/develop/src/OAuth2/TokenType/Bearer.php#L74

Symfony Request->request ParameterBag contains only true form data Laravel's HttpFoundation's Request->request ParameterBag is reflecting the primary input method (e.g. also JSON or GET input). Details: https://github.com/laravel/framework/pull/7052 Details: https://github.com/thephpleague/oauth2-server/issues/292 @bshaffer @julien-c Thanks for your work. @hannesvdvreken Funnily, by your PR you fixed one OAuth2 implementation and broke another one ;)

tthiery commented 9 years ago

And another similar issue. For whatever reason the original symfony HttpFoundation Request is not containing the authorize header. Taking the authorize header using getallheaders and injecting it later in the header bag, will make the thing working.

File a extra issue if you like for it.

hannesvdvreken commented 9 years ago

@bshaffer I had no idea :-) sorry if I broke something for you.

bshaffer commented 9 years ago

@tthiery Thank you for finding this. As this seems like a laravel-specific issue (a framework I am not familiar with) could you please recommend a fix?

we have fixed the second issue you mentioned with apache_get_headers, obviously only for apache, but I am assuming you aren't using apache if you still have the issue.

tthiery commented 9 years ago

@bshaffer i am using apache

bshaffer commented 9 years ago

@tthiery do you believe this behavior is the fault of my library? It seems to me this is the fault of Laravel including the parameters in two separate properties of the request object.

I could maybe include a way to override the check, or something along those lines, but I don't want to get rid of the current validation. Can you suggest a solution?

tthiery commented 9 years ago

@bshaffer .. With your httpfoundation bridge you support laravel so the issue has to be analyzed starting there. Either they have changed the interface contract (@hannesvdvreken can you make a statement here) and then your adaptation is wrong ... or laravel has a bug.

I am back to PHP programming after 10 years of absence. I have not idea/feeling how laravel should correctly behave. But since it is an reviewed PR in the laravel project ... I assume so far it is a contract change .. and therefore your issue in your projects.

bshaffer commented 9 years ago

This strikes me as a WONT FIX - I do not want to add logic in this library specifically to undo poorly conceived logic in laravel's createFromBase method.

I am assuming you're calling OAuth2\HttpFoundationBridge\Request::createFromRequest correct? If so, this would be an OK place to add logic to say "If this is a Laravel request, AND it is a GET request, unset the request body data".

I would be willing to add that logic there as a solution to this problem, since this library is a bridge to other libraries, and framework-specific logic can be added appropriately to handle framework-specific issues. What do you think @tthiery ?

tthiery commented 9 years ago

If you fix it in the HttpFoundationBridge .. that is fine for me. Sounds like a solution. I agree, that it is the wrong place to do it in your server library. The bridge is in the end an adapter pattern responsible exactly for issues like this one.

Do not forget the second issue (first comment) ... similar case.

Shift the bug to the http foundation bridge! Thanks @bshaffer

JerryBels commented 9 years ago

Hello, @tthiery , @bshaffer ...

I have the same issue here. How do I unset the request body data please ? Still a beginner with Laravel and Oauth2 both soooo... Thanks ahead :)

tthiery commented 9 years ago

@JerryBels actually, i unset to POST body by just assigning a new ParameterBag. something like

$request->request = new \Symfony\Component\HttpFoundation\ParameterBag();

and for the second topic

$rawHeaders = getallheaders();
if (isset($rawHeaders["Authorization"])) {
     $authorizationHeader = $rawHeaders["Authorization"];
     $bridgedRequest->headers->add([ 'Authorization' => $authorizationHeader]);
}
JerryBels commented 9 years ago

Hey, thanks for responding! In the route, $request is not defined, so I added a parameter Request $request and then set what you gave me... It didn't work, I still have a count of 2 and so a fail in getAccessTokenParameter. I guess I failed to unset...

While I'm at it, for the header, I need to call the header Authorization with value Bearer mytokenvalue right?

tthiery commented 9 years ago

@JerryBels "Bearer token" is IMHO correct.

JerryBels commented 9 years ago

Okay, thanks @tthiery ... And about the unset, you don't know where I got wrong ?

Route::get('private', function(Request $request)
{
    $bridgedRequest  = OAuth2\HttpFoundationBridge\Request::createFromRequest(Request::instance());
    $bridgedResponse = new OAuth2\HttpFoundationBridge\Response();

    // fix laravel
    $request->request = new \Symfony\Component\HttpFoundation\ParameterBag();
    $rawHeaders = getallheaders();
    if (isset($rawHeaders["Authorization"])) {
        $authorizationHeader = $rawHeaders["Authorization"];
        $bridgedRequest->headers->add([ 'Authorization' => $authorizationHeader]);
    }

    if (App::make('oauth2')->verifyResourceRequest($bridgedRequest, $bridgedResponse)) {

        $token = App::make('oauth2')->getAccessTokenData($bridgedRequest);

        return Response::json(array(
            'private' => 'stuff',
            'user_id' => $token['user_id'],
            'client'  => $token['client_id'],
            'expires' => $token['expires'],
        ));
    }
    else {
        return Response::json(array(
            'error' => 'Unauthorized'
        ), $bridgedResponse->getStatusCode());
    }
});
tthiery commented 9 years ago

2 issues

JerryBels commented 9 years ago

Oh, thank you so much ! I actually succeeded into correcting it also by simply replacing $request with $bridgedRequest... But your comment helped me kickstart my brain to understand how all of this works. You really helped me a lot ! Thanks again :)

ghost commented 9 years ago

@tthiery I'm having the same issue. I implemented your fix along with the advice you gave @JerryBels and it did not work. I tried replacing the variable, as he did, to no avail. I receive the same error from Laravel each time and that error is:

Argument 1 passed to OAuth2\HttpFoundationBridge\Request::createFromRequest() must be an instance of Symfony\Component\HttpFoundation\Request, instance of Illuminate\Support\Facades\Request given

Here is my current code:

Route::get('private', function(Request $request)
{
    $request->request = new \Symfony\Component\HttpFoundation\ParameterBag();
    $rawHeaders = getallheaders();
    if (isset($rawHeaders["Authorization"])) {
        $authorizationHeader = $rawHeaders["Authorization"];
        $bridgedRequest->headers->add([ 'Authorization' => $authorizationHeader]);
    }

    $bridgedRequest  = OAuth2\HttpFoundationBridge\Request::createFromRequest($request);
    $bridgedResponse = new OAuth2\HttpFoundationBridge\Response();

    if(App::make('oauth2')->verifyResourceRequest($bridgedRequest, $bridgedResponse))
    {

        $token = App::make('oauth2')->getAccessTokenData($bridgedRequest);

        return Response::json(array(
            'private' => 'stuff',
            'user_id' => $token['user_id'],
            'client'  => $token['client_id'],
            'expires' => $token['expires'],
        ));
    }
    else
    {
        return Response::json(array(
            'error' => 'Unauthorized'
        ), $bridgedResponse->getStatusCode());
    }
});
JerryBels commented 9 years ago

@beznez I think you need to declare the bridge before the fix... I did it this way :

App::singleton('oauth2', function() {
    $storage = new OAuth2\Storage\Pdo(array('dsn' => 'mysql:dbname=oauth2;host=localhost', 'username' => 'root', 'password' => 'root'));
    $server = new OAuth2\Server($storage);

    $server->addGrantType(new OAuth2\GrantType\ClientCredentials($storage));
    $server->addGrantType(new OAuth2\GrantType\UserCredentials($storage));

    return $server;
});

Route::get('private', function()
{
    $bridgedRequest  = OAuth2\HttpFoundationBridge\Request::createFromRequest(Request::instance());
    $bridgedResponse = new OAuth2\HttpFoundationBridge\Response();

        // fix for laravel
        $bridgedRequest->request = new \Symfony\Component\HttpFoundation\ParameterBag();
        $rawHeaders = getallheaders();
        if (isset($rawHeaders["Authorization"])) {
            $authorizationHeader = $rawHeaders["Authorization"];
            $bridgedRequest->headers->add([ 'Authorization' => $authorizationHeader]);
        }

    if (App::make('oauth2')->verifyResourceRequest($bridgedRequest, $bridgedResponse)) {

        $token = App::make('oauth2')->getAccessTokenData($bridgedRequest);

        return Response::json(array(
            'private' => 'stuff',
            'user_id' => $token['user_id'],
            'client'  => $token['client_id'],
            'expires' => $token['expires'],
        ));
    }
    else {
        return Response::json(array(
            'error' => 'Unauthorized'
        ), $bridgedResponse->getStatusCode());
    }
});

And it worked perfectly (of course use your own credentials). Hope it helps.

ghost commented 9 years ago

Perhaps my workflow is incorrect? I have the same code you have above with my own credentials. First I get an access token:

curl -u testclient:testpass "http://domain.com/oauth/token" -d "grant_type=password&username=bshaffer&password=brent123"

I receive:

{"access_token":"62883e3fda62855c02f4e3f6e34a430fdf324c23","expires_in":3600,"token_type":"Bearer","scope":null,"refresh_token":"cedeb556632b3728ceff367fc885d10af4a7bbb6"}

Then I use the access_token to try to get the private resource:

curl -u testclient:testpass "http://domain.com/private?access_token=62883e3fda62855c02f4e3f6e34a430fdf324c23"

I receive:

{"error":"Unauthorized"}