mikenicholson / passport-jwt

Passport authentication using JSON Web Tokens
MIT License
1.96k stars 213 forks source link

Error: No auth token - is not set as Error #160

Open kolbma opened 6 years ago

kolbma commented 6 years ago

If there is no Authentication header, the passport.authenticate()-callback gets set the info parameter to Error: No auth token. But the 1st error parameter is undef.
Shouldn't the error parameter be an error if an error occurs?

whitebyte commented 6 years ago

Same goes for TokenExpiredError. Is this intended behavior?

mikenicholson commented 5 years ago

Handling missing token with fail() rather than error() is consistent with the way passport-local works. I assume since this is written by the author of passport it's the canonical example.

See the documentation for Strategy.error() and Strategy.fail().

fail() is meant for a failed authentication attempt - I would consider expired credentials or missing credentials to be a failure to authenticate.

error() is meant for application errors like a failure to connect to the database.

Based on my interpretation of the docs and other strategies I disagree with this request and don't currently expect to merge the corresponding pull request.

I'm open to discussion on this if you can provide documentation or examples suggesting I'm misreading the passportjs documentation.

jerroydmoore commented 5 years ago

In that case, can a callback to passed to _jwtFromRequest() that acts similar to passport's done method? In particular, can the callback's form be (error, token, info)? That way, we can use fail() correctly to indicated a failed authentication attempt while also allowing _jwtFromRequest() to include more information in the info. I'm trying to give my users better information on why a jwt token could not be extracted from the request.

The work around has been to throw something in my jwtFromRequest(), but is less elegant.

edit: spelling

mikenicholson commented 5 years ago

@jerroydmoore I really like the idea of switching to an extractor function that accepts a done callback rather than simply returning the JWT.

I’ll probably have to wait until I get some time around the holidays to get around this but PR’s are welcome if they maintain the existing level of test coverage.

Acerbic commented 4 years ago

@mikenicholson

See the documentation for Strategy.error() and Strategy.fail().

That documenation states that fail() should be called with arguments of types String, Number, and error() with argument of type Error. You, however, pass an Error object to fail().

Perhaps, instead of

return self.fail(new Error("No auth token"));

you should be doing

return self.fail("No auth token", 401);

?