joselfonseca / lighthouse-graphql-passport-auth

Add GraphQL mutations to get tokens from passport for https://lighthouse-php.com/
https://lighthouse-php-auth.com/
MIT License
229 stars 56 forks source link

Wrong exceptions thrown in case of invalid configuration #99

Closed MilosMosovsky closed 4 years ago

MilosMosovsky commented 4 years ago

@joselfonseca Hello, I would like to ask you a question about improving exception handling of the login. Currently in my project I have set up sentry for error reporting which started to catch all invalid username/password errors as exceptions, thus I filtered them out.

Which works fine, but when I accidentally did wrong deploy and did not setup passport keys correctly, it exhausted my sentry quota with new 10 000 events (because everyone who tried to log in get this exception).

I was investigating further and saw that here

https://github.com/joselfonseca/lighthouse-graphql-passport-auth/blob/master/src/GraphQL/Mutations/BaseAuthResolver.php#L46

        if ($response->getStatusCode() != 200) {
            throw new AuthenticationException(__('Authentication exception'), __('Incorrect username or password'));
        }

You are just checking response code and immediately throwing AuthenticationException but the response from the passports clearly shows either invalid_client or invalid_grant error.

Do you think (I could make PR) it would be worth to check if response.error field is invalid_grant and then throw AuthenticationException and in all other cases just throw Laravel\Passport\Exceptions\OAuthServerException so we can filter out sentry errors for bad username/password but still have reporting for all other cases ?

joselfonseca commented 4 years ago

@MilosMosovsky PRs are always welcome!

silasrm commented 4 years ago

Hi,

I'm following this article https://dev.to/joselfonseca/graphql-auth-with-passport-and-lighthouse-php-14g5 and I'm get a error about DiactorosFactory in log file:

[2020-07-20 17:23:32] local.ERROR: Class 'Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory' not found {"exception":"[object] (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Class 'Symfony\\Bridge\\PsrHttpMessage\\Factory\\DiactorosFactory' not found at /var/www/vendor/laravel/framework/src/Illuminate/Routing/RoutingServiceProvider.php:131)
[stacktrace]

And a "Incorrect username or password" error. This email/password is OK in app, only graphql has this error: image

Versions: "laravel/framework": "^6.0", <=== 6.4.1 "laravel/passport": "9.3.0", "joselfonseca/lighthouse-graphql-passport-auth": "^4.1", <=== 4.1.12 "symfony/psr-http-message-bridge": "^2.0", <=== 2.0.1

joselfonseca commented 4 years ago

@silasrm hi, I don't think it has anything to do with the package, we don't use that and the error is produced in the routing service provider in Laravel which is not modified or used by this package.

silasrm commented 4 years ago

Seeing the stacktrace, this error is launched in this line: $response = app()->handle($request); vendor/joselfonseca/lighthouse-graphql-passport-auth/src/GraphQL/Mutations/BaseAuthResolver.php:42

I'll trying to upgrade all packages.

joselfonseca commented 4 years ago

Yes, we make a request to get the passport tokens, but that request is handled by Laravel. We do not write any code to handle that request where it is failing. If you do find the issue and is in the code of this package please make a PR so we can see the issue as we have never faced this issue before and can't reproduce it.

silasrm commented 4 years ago

I upgrade to Laravel 6.18 and now, has this error on log:

[2020-07-21 10:52:11] local.ERROR: Client authentication failed {"exception":"[object] (Laravel\\Passport\\Exceptions\\OAuthServerException(code: 4): Client authentication failed at /var/www/vendor/laravel/passport/src/Http/Controllers/HandlesOAuthErrors.php:26)
[stacktrace]
#0 /var/www/vendor/laravel/passport/src/Http/Controllers/AccessTokenController.php(65): Laravel\\Passport\\Http\\Controllers\\AccessTokenController->withErrorHandling(Object(Closure))
#1 /var/www/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(48): Laravel\\Passport\\Http\\Controllers\\AccessTokenController->issueToken(Object(Nyholm\\Psr7\\ServerRequest))
#2 /var/www/vendor/laravel/framework/src/Illuminate/Routing/Route.php(219): Illuminate\\Routing\\ControllerDispatcher->dispatch(Object(Illuminate\\Routing\\Route), Object(Laravel\\Passport\\Http\\Controllers\\AccessTokenController), 'issueToken')
#3 /var/www/vendor/laravel/framework/src/Illuminate/Routing/Route.php(176): Illuminate\\Routing\\Route->runController()
#4 /var/www/vendor/laravel/framework/src/Illuminate/Routing/Router.php(681): Illuminate\\Routing\\Route->run()

Any idea? I'm lost.

joselfonseca commented 4 years ago

Client authentication failed means the client ID and secret are wrong. Get them from the DB and set it up in the env vars.

silasrm commented 4 years ago

I'm using this info and is ok now: https://github.com/laravel/passport/issues/589#issuecomment-357515110

joselfonseca commented 4 years ago

Added better error handling here https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/110 Will go out on next release.