Allow additional strategies to authenticate if no token is found #104. #105
Conversation
…#104. Require Bearer to be present in authorisation header.
|
The word "Bearer" is only required in OAuth 2.0 authorisation as per RFC 6750 what is the _advantage_ of requiring it when we are not doing OAuth...? |
|
True. For JWT I've only seen been Bearer to be used. But if you don't want to be locked in to use the word Bearer then maybe it should be configurable as seen in https://github.com/johnbrett/hapi-auth-bearer-token |
|
I'm 100% on board with having a
If you don't mind updating your PR we'll gladly merge it in. Or if you need any input let us know. thanks! |
| if (!token) { | ||
| return reply(Boom.unauthorized('Missing auth token')); | ||
| return reply(Boom.unauthorized(null, 'Token')); |
There was a problem hiding this comment.
First parameter of Boom.unauthorized is the error message
https://github.com/hapijs/boom#boomunauthorizedmessage-scheme-attributes
We could do:
return reply(Boom.unauthorized('Missing JWT Auth token', 'Token'));Is there a reason you want to return null as the first argument?
There was a problem hiding this comment.
This is what enables additional strategies to be attempted.
From hapi documentation http://hapijs.com/api#serverauthschemename-scheme:
If the err returned by the reply() method includes a message, no additional strategies will be attempted. If the err does not include a message but does include a scheme name (e.g. Boom.unauthorized(null, 'Custom')), additional strategies will be attempted in order of preference.
There was a problem hiding this comment.
Right, but have you tried using the strategy with try mode and checking if that prevents other strategies from being attempted?
There was a problem hiding this comment.
No, I havn't.
For this plugin to be useful for me it needs to support different strategies with required mode.
There was a problem hiding this comment.
Ok. I haven't seen two auth strategies required for the same request/route but I'm all ears. Can you describe the scenario in a bit more detail so we understand the use-case? we'd love to help you solve this because others will probably have similar needs in the future. 👍
There was a problem hiding this comment.
For our API we use JWT for user authenticated requests, but for services consuming the API we use an apiKey because we want to handle them differently.
There was a problem hiding this comment.
can't your API Key be a JWT?
(or do you already have an existing/legacy format for API Keys)
There was a problem hiding this comment.
It would be possible, but thats not what we want to do and it's outside the point here.
Hapi offers this functionality for authentication plugins and its was supported by the library i want to replace hapi-auth-jwt thats why I submitted this PR.
There was a problem hiding this comment.
understood. I was just wondering if you could simplify your own development by using JTWs everywhere.
There was a problem hiding this comment.
Hello! I'm also interested in seeing this particular line merged.
I have two auth strategies in place for several routes, although the second strategy does little more than change the value of request.auth.credentials at this point :)
In any case, I did test the difference between try and required and did not see a difference in behavior. If the first argument to Boom.unauthorized is not null, then Hapi doesn't try other authentication strategies, regardless of mode selected.
Thanks!
|
Any chance on a merge ? |
|
Hi @martinj yes, as previously discussed, we would welcome this PR, however we noted a few changes above to keep the header prefix optional. |
|
I agree with @nelsonic, 'Bearer' should be the default. If more flexibility is required, one can always use Further, this probably should have been two PRs, since the ability to continue validation has nothing to do with the flexibility of accepting content in an auth header. Just my $0.02 gang, thanks! Looking forward to a merge of the former. |
|
@nelsonic sorry, I missed that part with your suggestion about updating the PR. Anyway its done now. This update keeps the old behaviour with ignoring tokenType "auth scheme" in the Authorization header if nothing is specified in the options. |
|
Agree with @aulvi that this should have been two separate PRs because two things have changed. However, given that @martinj's second commit maintains the backward compatibility by optionally allowing alternative token prefixes none of the existing apps using this should be affected. As for the error handling change. return reply(Boom.unauthorized('Missing auth token'));to return reply(Boom.unauthorized(null, 'Token'));To "break" anything... if anyone sees an issue with us merging/publishing this PR please let us know! CC: @walling @eventhough @alanshaw @walling @duro @diffsky @dherault @ssassi @kenhowardpdx Thanks! 👍 |
Allow additional strategies to authenticate if no token is found #104.
|
@martinj @aulvi see: https://github.com/dwyl/hapi-auth-jwt2/releases/tag/v5.1.0 |
|
Thank you. |
Require Bearer to be present in authorisation header.