Skip to content
This repository was archived by the owner on Aug 29, 2018. It is now read-only.

Swallowing errors when getting user from service? #14

Closed
lvivier opened this issue Mar 7, 2017 · 6 comments
Closed

Swallowing errors when getting user from service? #14

lvivier opened this issue Mar 7, 2017 · 6 comments

Comments

@lvivier
Copy link

lvivier commented Mar 7, 2017

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
Copy link

elfey commented Mar 7, 2017

Is what you're describing similar to: feathersjs-ecosystem/authentication#421 ?

@lvivier
Copy link
Author

lvivier commented Mar 7, 2017

@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
Copy link
Member

ekryski commented Mar 31, 2017

@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
Copy link
Author

lvivier commented Apr 1, 2017

@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
Copy link
Member

ekryski commented Apr 15, 2017

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

@ahdinosaur
Copy link

hey 🐱

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. 👍

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants