cloudcreativity / laravel-json-api

JSON API (jsonapi.org) package for Laravel applications.
http://laravel-json-api.readthedocs.io/en/latest/
Apache License 2.0
780 stars 109 forks source link

[Question] Customize Authorization Exception Response #493

Closed lucianholt97 closed 3 years ago

lucianholt97 commented 4 years ago

I am working with passport scopes to authenticate specific actions. First of all, I ran into the issue that the scope middleware, provided by passport, does not play nicely with the json-api. For Example $api->resource('my-resource')->only('index')->middleware('scope:my-resource:index'); only returns a 500 internal server error instead of the 403 with details on the missing scope. As an alternative, I'd like to move the logic from the routes file to the authorizer. Unfortunately, I don't see any possibility to customize the response error text for the authorization exception, since the api only returns {"title": "Unauthenticated"} along with the 403 response code.

Is there any possibility to customize the response like "You need scope XY to access this resource"?

lindyhopchris commented 4 years ago

Hey! I should think this package should play nicely with Passport, so it'd be good to investigate why you're getting a 500 for the scope problem and get that fixed.

It must be throwing an Exception that I haven't got code to handle. Do you know what exception it throws in that scenario?

lucianholt97 commented 4 years ago

@lindyhopchris that would also be great. Passport throws a MissingScopeException which should have a 403 status code.

lucianholt97 commented 4 years ago

It would also be nice to allow throwing a MissingScopeException in the Authorizer

thoresuenert commented 4 years ago

@lindyhopchris as far as I understand the solution you have to provider a function in the translator class for each error we want to translate. that is not so flexible because you have to add laravel/passport to the composer dependency in case you want to handle the MissingScopeException.

It would be nice to have a look-up table or resolving logic where a user of the package can register there own error/translator classes.

Are you open for Pull Request?

lindyhopchris commented 4 years ago

Yeah the error translator class was never meant to be exposed - it was simply for internal use to generate the errors that this package created. The messages are meant to be customised using the lang files, and really I don't want the error translator changed because people really shouldn't need to touch it. The fundamental problem is that the error handling probably needs a rethink - it's always been tricky because the Laravel exception handler isn't easy to customise by packages.

To deal with the immediate problem though - the MissingScopeException. I can't see why this isn't working as it extends the AuthorizationException that is already handled by the package. So I don't think extensive re-writing should be necessary, nor requiring the Passport package.

What's the stack trace for the 500 error that caused this to be reported in the first place?

To customise the error messages that the translator generates, there isn't a need for a PR - the language files just need to be published. I think I need to add those as publishable.

lindyhopchris commented 4 years ago

Quick question - what versions of this package are you both on?

osteel commented 4 years ago

Hi,

I don't mean to hijack this issue, but it looks like it might be related to my current problem. I am working on a multilingual API and I'm having trouble translating the returned errors. Basically it seems error messages are overwritten by what's in the json-api-errors.php config file no matter what.

Are you saying there is a way to publish those as language files somehow @lindyhopchris ?

lindyhopchris commented 4 years ago

Sorry for slow reply, not been working the last two weeks.

Yes the translations should be published, as per this: https://laravel.com/docs/7.x/packages#translations

But I don't think I've put that in yet. So would need to do a release with that added in. I'd take a PR if someone has time before I get to it.

lindyhopchris commented 4 years ago

Added to the 1.x milestone for publishing the package translation files.

osteel commented 4 years ago

@lindyhopchris hey, thanks for your reply – and no worries.

I had a quick look and, if I understand correctly, you're saying that these translation files should be publishable. I gave that a go, but the problem is I don't think all of the errors are covered by errors.php.

Some other ones seem to be hard-coded in this configuration file. Even if that file were to be published, I can't use translations in there (the file would basically need to contain translation keys instead of the final messages it seems to contain at the moment).

I might be looking at this the wrong way though. What do you think?

lindyhopchris commented 4 years ago

Hey @osteel - I think you can delete the json-api-errors file as it was deprecated a long time ago. The translations replace that config file, which is why it is no longer necessary.

Can you give it a go deleting that file, then I think I just need to add publishing the translations?

osteel commented 4 years ago

Hi @lindyhopchris

So I made a few tests. All of them are based on an Illuminate\Auth\AuthenticationException exception being thrown. I'm using Postman to hit a JSON:API endpoint registered with JsonApi::register, which requires being authenticated.

First, I deleted json-api-errors – this is the error I got:

Illuminate\Support\ServiceProvider::mergeConfigFrom(): Failed opening required '/var/www/vendor/cloudcreativity/laravel-json-api/src/../config/json-api-errors.php' (include_path='.:/usr/share/php7') {"exception":"[object] (Symfony\\Component\\Debug\\Exception\\FatalErrorException(code: 64): Illuminate\\Support\\ServiceProvider::mergeConfigFrom(): Failed opening required '/var/www/vendor/cloudcreativity/laravel-json-api/src/../config/json-api-errors.php' (include_path='.:/usr/share/php7') at /var/www/vendor/laravel/framework/src/Illuminate/Support/ServiceProvider.php:64)

I then restored the file, and tried hitting the endpoint without changing anything. I get this response:

{
    "errors": [
        {
            "status": "401",
            "title": "Unauthenticated"
        }
    ]
}

Now if I open json-api-errors.php, go all the way down to replace this bit:

    /**
     * Error used when a user is not authenticated.
     */
    AuthenticationException::class => [
        Error::TITLE => 'Unauthenticated',
        Error::STATUS => Response::HTTP_UNAUTHORIZED,
    ],

With this:

    /**
     * Error used when a user is not authenticated.
     */
    AuthenticationException::class => [
        Error::TITLE => 'NOPE',
        Error::STATUS => Response::HTTP_UNAUTHORIZED,
    ],

This is what I now get in Postman:

{
    "errors": [
        {
            "status": "401",
            "title": "NOPE"
        }
    ]
}

I tried adding AuthenticationException::class as a translation key in the errors.php translation file, but it's not being picked up. So I don't think publishing the translations will help, at least for the errors and exceptions picked up from json-api-errors.php.

Am I missing something? I feel like there's an obvious way to translate these errors I'm not seeing

lindyhopchris commented 4 years ago

@osteel can you confirm what version of this package you are on?

osteel commented 4 years ago

@lindyhopchris it's version 1.6.0

lindyhopchris commented 4 years ago

Hmmm... yeah not sure how to essentially fix this, as the solution is to delete the errors config file (which has already happened on the 2.0 releases). When are you planning to go to Laravel 7?

osteel commented 4 years ago

@lindyhopchris I'm not, unfortunately. Company policy is to stick to LTS versions.

To be fair, this is not a dramatic issue. If I can say for sure that I've done what I could and that it's a package limitation (not minimising its quality here – it's incredibly useful) that will be addressed in a future release that will require Laravel 7, that's probably fine.

lindyhopchris commented 3 years ago

Closing this as I think we reached a conclusion.