dwyl / hapi-auth-jwt2

:lock: Secure Hapi.js authentication plugin using JSON Web Tokens (JWT) in Headers, URL or Cookies
ISC License
798 stars 126 forks source link

Documentation: key function return object isValid field: field not used by implementation #302

Closed dpmott closed 5 years ago

dpmott commented 5 years ago

The documentation here suggests that a function assigned to the key field of the strategy options should return an object which contains a field isValid to indicate success or failure.

However, the implementation here only looks for the field key (and gathers up everything else as extraInfo), and does not explicitly examine the isValid field as part of retrieving the keys.

While it's possible for a developer to look at request.plugins['hapi-auth-jwt2'].extraInfo.isValid later in the validate function, there's no examples or explicit guidance that suggests that the developer should be responsible for this. I'm inclined to believe that this is not the intent, and that the documentation simply refers to isValid erroneously in the context of the key function documentation. If I can get confirmation of intent here, I'll happily create a PR to update the docs to reflect this.

Additionally, it appears to be completely valid to throw new Error() from the key function to abort processing and cause a 401 error to be returned to the user. However, the documentation doesn't discuss this as an option for returning an error from the key function. If I can get confirmation that this is the intent, I'll happily include these changes in a PR.

Thanks!

nelsonic commented 5 years ago

@dpmott agree that the documentation could use an update to make it more coherent ... (my bad!)

isValid is used in index.js to reject a request on line 155:

https://github.com/dwyl/hapi-auth-jwt2/blob/1f96cd76039f33ed10f8735895f2cad95f6fdfee/lib/index.js#L155-L160

it is a valid step to raiseError in the course of checking a token but I agree that this is insufficiently documented. Thank you for reading our docs with a "beginner's mind" to spot the incompleteness.

A pull request would be very much welcome! 🥇

shaharyar123 commented 5 years ago

@nelsonic so isValid should be removed from the documentation?