search

feathersjs-ecosystem / authentication-jwt

[MOVED] JWT authentication strategy for feathers-authentication using Passport
https://github.com/feathersjs/feathers
MIT License
30 stars 10 forks source link

Swallowing errors when getting user from service? #14

Closed lvivier closed 6 years ago

lvivier commented 7 years ago

I'm using this plugin with feathers-authentication@1.0.2 and I've run into a confusing situation.

If the storage backing the user service is unavailable, I want my error handler to say "503 Service Unavailable". But if someone tries to authenticate a request with a JWT, the error from the user service gets swallowed and the authentication "succeeds". In my application, the next hook is typically to restrict access by user, so the error handler says "403 Forbidden".

The code in question is in src/verifier.js and there's a test backing it, so that clarifies for me it's not a bug.

I looked at a few other plugins (local, oauth1, oauth2) and they all seem to call out with the error from the service. Why does the JWT plugin behave differently, and would it be ok to make a PR that treats a service error as a failure to authenticate?

elfey commented 7 years ago

Is what you're describing similar to: https://github.com/feathersjs/feathers-authentication/issues/421 ?

lvivier commented 7 years ago

@elfey I don't think so…my situation arises when the request comes with a valid jwt but the user service has an error (which is ignored/swallowed by the jwt plugin so I can't handle it).

ekryski commented 7 years ago

@lvivier sounds like a legit issue. What are you getting back from your /users service as an error?

Basically, that was in place in case the user could not be found and that basically we should just not populate the user into the JWT payload. The JWT validation is the part that succeeds, and the thinking was that the population of a user should not block that.

However, we might need to be a bit more explicit about error codes by differentiating between a 404 and some other error.

Thoughts?

lvivier commented 7 years ago

@ekryski differentiating the errors makes sense to me. I understand the use case where there is no user. In my case, the MySQL server that backs the user service was down.

Maybe I can put a PR together with a few test cases?

ekryski commented 7 years ago

@lvivier I'd love if you could do that! 😄

ahdinosaur commented 7 years ago

hey :cat:

i just ran into a confusing bug hunt caused by feathers-authentication-jwt failing to get the user.

in my case, it was because we accidentally put before hooks on users.get that expected hook.params.user (oops!). was an easy fix using feathers-hooks-common.isProvider, but took some time to track down due to lack of any error message and instead the user object was empty {}. so yeah in my case not swallowing the error would have been helpful. :+1: