Api: Invalid Error Codes in Authentication Failed

[alexrayan]

Hi, how are you?

I'm using OAuth2 package from Luca Degasperi as an authentication provider.
When the authentication fails due to invalid credentials, League\OAuth2\Server\Exception\InvalidClientException seems to be triggered which results in message "Client authentication failed." and should have a code 401.
However, the api returns 500 as a status_code in json response.

Is there something missing? At this point I'm sill not sure whether it's assigning invalid code in Luca's package or this one, I'm still investigating this. So, my apologies if the question shouldn't be for you.

Best regards,
Alex

[alexrayan]

Ok, so I traced it to the line where the code checks for exception being an instance of HttpExceptionInterface in Handler.php on line 169 and replaced it with the following check:

if($exception instanceof HttpExceptionInterface){
      $code = $exception->getStatusCode();
} else if($exception instanceof \Exception){
      $code = property_exists($exception,'httpStatusCode') ? $exception->httpStatusCode : 500;
} else {
      $code = 500;
}
return $code;

Now the response is getting the correct error code. The issue seems to be that $exception originating in league/oauth2-server/Exception classes that extend OAuthException all have $httpStatusCode as a property but no actual error code is being set (getCode() method returns 0).

It'd be great if someone could offer a better way of solving the above issue.

Regards,
Alex

[jasonlewis]

Are you using this packages authentication middleware?

On Fri, 28 Aug 2015 10:48 alexrayan [email protected] wrote:

Ok, so I traced it to the line where the code check for exception being an
instance of HttpExceptionInterface in Handler.php on line 169 and replaced
it with the following check:

if($exception instanceof HttpExceptionInterface){
$code = $exception->getStatusCode();
} else if($exception instanceof \Exception){
$code = property_exists($exception,'httpStatusCode') ?
$exception->httpStatusCode : 500;
} else {
$code = 500;
}
return $code;

Now the response is getting the correct error code. The issue seems to be
that $exception originating in league/oauth2-server/Exception classes that
extend OAuthException all have $httpStatusCode as a property but no actual
error code is being set (getCode() method returns 0).

It'd be great if someone could offer a better way of solving the above
issue.

Regards,
Alex

—
Reply to this email directly or view it on GitHub
https://github.com/dingo/api/issues/613#issuecomment-135593483.

[alexrayan]

Hi Jason,

Yes, I'm using oauth provider with this package with a client_credentials and password grants.

I'm also using Postman for API calls testing. Whenever the authentication errors occurred (either with a missing param or an invalid param), 500 error code was returned (which is default in the handler) instead of the actual error code set in exception. I'm also using Luca Degasperi's OAuth2Server package for OAuth2.

[asantanacu]

Hi
I think the problem is due to the class "League\OAuth2\Server\Exception\OAuthException" from the library "league/oauth2-server" did not implement the interface "Symfony\Component\HttpKernel\Exception\HttpExceptionInterface" and then the methods "getStatusCode" and "getExceptionStatusCode" in the class "Dingo\Api\Exception\Handler" return the code 500.
I tried to bind "api.exception" to create my own handler but did not work, the below function always bind "api.exception" to ExceptionHandler (Dingo\Api\Exception\Handler).

protected function registerExceptionHandler()
{
    $this->app->singleton('api.exception', function ($app) {
        $config = $app['config']['api'];

        return new ExceptionHandler($config['errorFormat'], $config['debug']);
    });
}

The only solution worked for me without change the vendor code was create my own provider extended the LaravelServiceProvider and overwrite the function registerExceptionHandler

class ApiServiceProvider extends LaravelServiceProvider
{
    protected function registerExceptionHandler()
    {
        $this->app->singleton('api.exception', function ($app) {
            $config = $app['config']['api'];
            return new MyExceptionHandler($config['errorFormat'], $config['debug']);
        });
    }   
}

and MyExceptionHandler extend \Dingo\Api\Exception\Handler and overwrite the functions getStatusCode and getExceptionStatusCode

class MyExceptionHandler extends \Dingo\Api\Exception\Handler
{
    protected function getStatusCode(Exception $exception)
    {
        if($exception instanceof HttpExceptionInterface){
            $code = $exception->getStatusCode();
        } else if($exception instanceof \Exception){
            $code = property_exists($exception,'httpStatusCode') ? $exception->httpStatusCode : 500;
        } else {
            $code = 500;
        }
        return $code;
    }

   protected function getExceptionStatusCode(Exception $exception, $defaultStatusCode = 500)
    {
        if($exception instanceof HttpExceptionInterface){
            $code = $exception->getStatusCode();
        } else if($exception instanceof \Exception){
            $code = property_exists($exception,'httpStatusCode') ? $exception->httpStatusCode : 500;
        } else {
            $code = $defaultStatusCode;
        }
        return $code;
    }
 }

[jasonlewis]

I'm not even sure why you're seeing an InvalidClientException.

If you're using the api.auth middleware with the Dingo\Api\Auth\Provider\OAuth2 provider then it will catch ALL OAuthException's and rethrow as an UnauthorizedHttpException.

Refer to here: https://github.com/dingo/api/blob/master/src/Auth/Provider/OAuth2.php#L79-L87

Can you 100% confirm to me that you're using the authentication mechanisms provided by this package.

[refear99]

@asantanacu This works for me

[alexrayan]

@jasonlewis
Hi Jason,

Yes, we're using the api.auth middleware; however, the issue with incorrect error code happens on the unprotected route which should return an initial access token. Here's our use case:

$api = app('Dingo\Api\Routing\Router');
$api->version('v1', function ($api) {
    $api->post('access_token', 'App\Http\Controllers\Api\AuthenticateController@index', array('only' => array('index')));
    $api->group(['middleware' => 'api.auth', 'providers' => ['oauth']], function ($api) {
    });
}

The route access_token generated the token and then the rest of the routes are grouped and a middleware is applied to all the routes.

In Postman, when we do POST to http://example.com/v1/access_token with the header:

grant_type client_credentials
client_id our_client_id
client_secret our_client_secret

and we deliberately put the wrong credentials, we get:

{
  "message": "Client authentication failed.",
  "status_code": 500
}

while we would expect to get "status_code" : 401.

The status code is set within either getStatusCode() or getExceptionStatusCode() and it's derived from either an exception (if it is an instance of HttpExceptionInterface) or is set to default which is 500.

Hope this illustrates better what the issue is for us. The whole process still works, it's just the error message that is wrong. And since we're building custom API and would like to keep response codes consistent with guidelines.

Best regards,
Alex

[jasonlewis]

Ah I see I see this is from when you're issuing an access token. My bad, I thought it was from when you were authenticating a token.

Okay, in this case you'll just need to catch these exceptions yourself, and throw an HttpException.

Something like this:

try {
    return app('oauth2-server.authorizer')->issueAccessToken();
} catch (League\OAuth2\Server\Exception\OAuthException $e) {
    throw new Symfony\Component\HttpKernel\Exception\HttpException($e->httpStatusCode, $e->getMessage(), $e);
}

That will catch any of the exceptions thrown by the OAuth 2.0 package and throw an HttpException with the correct status code and using the exceptions message.

[jasonlewis]

Or you could register a handler for the exception if you'd like to keep that out of your routes.

app('Dingo\Api\Exception\Handler')->register(function (League\OAuth2\Server\Exception\OAuthException $e) {
    throw new Symfony\Component\HttpKernel\Exception\HttpException($e->httpStatusCode, $e->getMessage(), $e);
});

[refear99]

app('Dingo\Api\Exception\Handler')->register(function (League\OAuth2\Server\Exception\OAuthException $e) {
    throw new Symfony\Component\HttpKernel\Exception\HttpException($e->httpStatusCode, $e->getMessage(), $e);
});

use this code in AppServiceProvider, It's working, But how can I catch it using \App\Exceptions\Handler ?

[jasonlewis]

Boot method is fine.

You can't. The API handler is used for API requests. Register it with the API handler.

[refear99]

@jasonlewis How to register a new handler with the API handler? Like @asantanacu 's solution?

[jasonlewis]

I demonstrated how to register a handler.

You get an instance of the exception handler.

$handler = app('Dingo\Api\Exception\Handler');

Then you register a callback just like with Laravel.

$handler->register(function (ExceptionClassNameHere $exception) {
    // return a response or something here
});

There is literally no need to override the exception handler like @asantanacu if all you want to do is catch the exception thrown when attempting to issue an access token. Just catch it in the route or catch it in an exception handler.

[refear99]

@jasonlewis I want catch exception and send report to my error_log services

[jasonlewis]

See my previous comment. You can do exactly that in the callback.

Where are you having problems?

[refear99]

@jasonlewis , thanks for your reply.

I put this to AppServiceProvider boot() method

app('Dingo\Api\Exception\Handler')->register(function (\League\OAuth2\Server\Exception\OAuthException $e) {
            $config = config('api');
            return new \App\Exceptions\ApiExceptionHandler($config['errorFormat'], $config['debug']);
});

ApiExceptionHandler is a copy form Dingo\Api\Exception\Handler.

And I get this error

{"error":"The Response content must be a string or object implementing __toString(), \"object\" given."}

I think it's completely wrong.....

[jasonlewis]

Yeah you're mixing things up a bit. You don't NEED to replace the exception handler. Forget what @asantanacu has posted above.

app('Dingo\Api\Exception\Handler')->register(function (ExceptionClassNameHere $exception) {
    // return a response or something here
});

The above does not REPLACE the handler, it just allows you to perform certain actions and return a response when an exception is encountered.

For example:

app('Dingo\Api\Exception\Handler')->register(function (\League\OAuth2\Server\Exception\OAuthException $exception) {
    $this->app['your.service']->reportError($exception);

    return new Response($exception->getMessage(), $exception->httpStatusCode);
});

In this example I'm calling the reportError method on a service you may have bound to the container. This could be anything though. You could call Laravel's logger and log some sort of error. Fire an event.

Then I return a new Response instance. Again, you could simply throw an HttpException instance here and let the API format the response so it's consistent.

IF you want to register an entirely new exception handler instance that replaces the API one, for whatever reason (there aren't many, unless you need some very specific functionality), then you can do so like this in the boot method of a service provider.

$this->app->singleton('api.exception', function ($app) {
    $config = $app['config']['api'];

    return new CustomExceptionHandler($app['log'], $config['errorFormat'], $config['debug']);
});

Again, the likelihood you should be replacing it isn't very high. For something as simple as this simply registering the exception with the handler is fine.

[refear99]

Thanks again, @jasonlewis

I try to replace Exception Handler

<?php

namespace App\Exceptions;

use Exception;

class ApiExceptionHandler extends \Dingo\Api\Exception\Handler
{
    public function __construct(array $format, $debug)
    {
        dd('111'); // This working.
        parent::__construct($format, $debug);
    }

    public function handle(Exception $exception)
    {
        dd(123);
    }

    public function render($request, Exception $exception)
    {
        dd(456);
    }
}

It's not working too...

[asantanacu]

Hi @refear99
As @jasonlewis told you is better to forget about register a new CustomExceptionHandler.
My post above was the only solution I found at that moment but the option to register a handler is easier.
This code works for me

app('Dingo\Api\Exception\Handler')->register(function (\League\OAuth2\Server\Exception\OAuthException $e) {
    return new \Illuminate\Http\Response(['message'=>$e->getMessage(),'status_code'=>$e->httpStatusCode], $e->httpStatusCode, $e->getHttpHeaders());
});

I don't like the option to rethrow the exception but this option also works and maybe is better if we are thinking modify the format of the response in the file config/api.php

[refear99]

Hi, @asantanacu.
This only catch \League\OAuth2\Server\Exception\OAuthException

such as FatalErrorException, QueryException, catch theme and report an high level error to the log system

This work fine

app('Dingo\Api\Exception\Handler')->register(function (\League\OAuth2\Server\Exception\OAuthException $e) {
            throw new \Symfony\Component\HttpKernel\Exception\HttpException($e->httpStatusCode, $e->getMessage(), $e);
});

But when a QueryException triggered

{
  "error": "SQLSTATE[42S22]: Column not found: 1054 Unknown column 'active1' in 'where clause' (SQL: select * from `events` where `active1` = 1 order by `weight` desc, `created` desc)",
  "code": 42
}

I tried set API_DEBUG and APP_DEBUG false, not working.

I just want to catch this major exception, log only them, and response like this

{"error":"System Error"}

[asantanacu]

Hi @refear99
You can register as many handlers you want (one per each exception) or create a handler for all the exceptions, this code below should catch all the exceptions

app('Dingo\Api\Exception\Handler')->register(function (\Exception $e) {
    //process the exception
});