New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Digest and Bearer Auth modes to http request node #2061
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Bart , couple of thoughts... the way you have it works just some small style points :-)
<label for="node-input-password"><i class="fa fa-lock"></i> <span data-i18n="common.label.password"></span></label> | ||
<input type="password" id="node-input-password"> | ||
</div> | ||
<div class="form-row node-input-bearer-row hide"> | ||
<label for="node-input-bearerToken"><i class="fa fa-lock"></i> <span data-i18n="httpin.label.bearerToken"></span></label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Bart - any reason not to re-use the password field as it is effectively the same - and you can't have both :-) It makes some of the following logic slightly easier as you don't then need to re-merge either this or that, test for both, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Dave (@dceejay),
I have been thinking the same, but I introduced a new token field because I thought you guys wouldn't like me to use the same field for two different purposes. I can indeed re-use the password field if you like. But is it allowed to change the 'password' label to 'token' (as soon as bearer authentication is selected)? Or do you always want the label to be 'password'??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the field can be the same as it's serving effectively the same purpose - the label can be different as the word token is more appropriate in that context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Dave (@dceejay),
I have changed and tested everything, except from the dynamically changed label.
Do you have any advice on how that can be done easily?
<label for="node-input-password"><i class="fa fa-lock"></i> <span data-i18n="common.label.password"></span></label>
<input type="password" id="node-input-password">
So it should become "common.label.bearerToken"
...
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you use the same .show() .hide() technique as before ?
hmm - not sure bearerToken is that "common" so it probably ought to be in "httpin.label.bearerToken"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dceejay,
Do you mean two label tags pointing to the same input element? Or two span tags into a single label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dave,
my time is up for this evening ...
Seems that an input tag is allowed to have multiple labels.
I added a last code snippet to switch between two labels, but the layout is not what I expected:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of two labels - have one label tag with two more span tags inside - give each span the id you want to show/hide. (they then each have the <i... icon> and inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evening Dave (@dceejay),
With the two span elements it indeed looks better now:
I think all your feedback is implemented.
Is it ok, or does the House of Commons need to vote now?
Bart
@@ -33,6 +33,7 @@ module.exports = function(RED) { | |||
var tlsNode = RED.nodes.getNode(n.tls); | |||
} | |||
this.ret = n.ret || "txt"; | |||
this.authType = n.authType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we usually force the default here (as per line 35 above) - again making the following logic then just need to handle the new paths
this.authType = n.authType || "basic";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Dave (@dceejay),
I had it like you say in my first version, but I rewrote it since I had to know (further in the code) whether it was undefined (i.e. version < 0.20.x). But in the current implementation that isn't required anymore. So I will change and test it this evening ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - look forward to merging it tomorrow !
@knolleary - Looks all good to me. Can you give the once over and merge ? |
Proposed changes
As discussed on the forum, this PR introduces Digest and Bearer authentication for the HttpRequest node.
Following flow can be used to test the 3 authentication methods:
All 6 tests in this flow make use of httpbin.org, which offers endpoints for all 3 authentication methods.
I have tried to have no impact on existing http-request nodes, i.e. nodes that have been created with a previous version. I have tested this by adding a few http-request nodes to my flow (with the previous version), and afterwards I have installed this new version:
No credentials will be set (i.e. we won't arrive at line 183), since there is no username. This is the same logic as in the previous version.
The credentials will be set (i.e. we will arrive at line 183): indeed I treat empty authType (in case credentials are available) as 'basic' authentication.
Thanks for reviewing this PR !!!!
Bart
Checklist
grunt
to verify the unit tests pass