Skip to content
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

Updated BASIC AUTH support #172

Closed
wants to merge 5 commits into from
Closed

Updated BASIC AUTH support #172

wants to merge 5 commits into from

Conversation

egeek
Copy link

@egeek egeek commented Apr 2, 2014

Added BASIC AUTH support to the current version of iodocs. Support for BASIC AUTH was the subject of these previous pull requests:

#73
#63
#32

and these issues:

#157 - the "rejectUnauthorized" flag in app.js in this patch allows self-signed certs. The initial value of this could be moved to a configuration flag instead of always being fale.
#10

This change adds support for BASIC AUTH. In apiconfig.json set the auth type to basic

"auth":"basicAuth"

for any API requiring basic authorization. This will result in the apiKey on the page being replaced with username and password fields.

Note that this pull request does not address using basic auth and apikey at the same time. It would be good if this change could be merged in, then extended to support using both auth types at the same time later.

@KramKroc
Copy link

KramKroc commented Apr 2, 2014

It would be great if this was accepted.

egeek and others added 5 commits April 3, 2014 00:35
- added support for unsigned certs
- added support for unsigned certs
@@ -56,7 +56,7 @@ if (argv.help) {
// Configuration
var configFilePath = path.resolve(argv['config-file']);
try {
var config = require(configFilePath);
var config = require('./config.json');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should nix this line change because we've merged in the use of optimist which allows optional use of command line flag to set configFilePath. e.g.

node app.js --config-file=/foo/bar/test1.json

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. Would you like me to submit a new pull request or can you
change the line during the merge?
On 4 Apr 2014 23:13, "Neil Mansilla" notifications@github.com wrote:

In app.js:

@@ -56,7 +56,7 @@ if (argv.help) {
// Configuration
var configFilePath = path.resolve(argv['config-file']);
try {

  • var config = require(configFilePath);
  • var config = require('./config.json');

Perhaps we should nix this line change because we've merged in the use of
optimist which allows optional use of command line flag to set
configFilePath. e.g.

node app.js --config-file=/foo/bar/test1.json

Reply to this email directly or view it on GitHubhttps://github.com//pull/172/files#r11316196
.

@mansilladev
Copy link
Contributor

Would you ever have a case where you would want both auth by API key && with basic auth?

@egeek
Copy link
Author

egeek commented Apr 5, 2014

I can't think of a case where we'd need both basic auth and API key auth at the same time.

@challgren
Copy link
Contributor

I have a site where that is in use right now.
On Apr 5, 2014 3:24 AM, "Stephen Houston" notifications@github.com wrote:

I can't think of a case where we'd need both basic auth and API key auth
at the same time.


Reply to this email directly or view it on GitHubhttps://github.com//pull/172#issuecomment-39632236
.

@egeek
Copy link
Author

egeek commented Apr 5, 2014

Both basic auth and apikey in the header? Are they both used for auth or
does one take precedence over the other?
On 5 Apr 2014 11:30, "Chris Hallgren" notifications@github.com wrote:

I have a site where that is in use right now.
On Apr 5, 2014 3:24 AM, "Stephen Houston" notifications@github.com
wrote:

I can't think of a case where we'd need both basic auth and API key auth
at the same time.

Reply to this email directly or view it on GitHub<
https://github.com/mashery/iodocs/pull/172#issuecomment-39632236>
.

Reply to this email directly or view it on GitHubhttps://github.com//pull/172#issuecomment-39634315
.

@challgren
Copy link
Contributor

I just use http basic auth and a api key as a parameter
On Apr 5, 2014 7:39 AM, "Stephen Houston" notifications@github.com wrote:

Both basic auth and apikey in the header? Are they both used for auth or
does one take precedence over the other?
On 5 Apr 2014 11:30, "Chris Hallgren" notifications@github.com wrote:

I have a site where that is in use right now.
On Apr 5, 2014 3:24 AM, "Stephen Houston" notifications@github.com
wrote:

I can't think of a case where we'd need both basic auth and API key
auth
at the same time.

Reply to this email directly or view it on GitHub<
https://github.com/mashery/iodocs/pull/172#issuecomment-39632236>
.

Reply to this email directly or view it on GitHub<
https://github.com/mashery/iodocs/pull/172#issuecomment-39634315>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/172#issuecomment-39636689
.

@egeek
Copy link
Author

egeek commented Apr 6, 2014

@challgren Do you expect API Key on the header or on the query string? How are you handling having IODocs send both of these at the minute?

@challgren
Copy link
Contributor

Originally it was in the headers but since I used iodocs we had to write a patch to accept it as a query string parameter.

@egeek
Copy link
Author

egeek commented Apr 7, 2014

@challgren Can you continue to use that method? I'm not sure what we could set the value of "auth" to that would convey using both the Basic Auth and API Key forms of authentication on a single request.

@challgren
Copy link
Contributor

Yep it only took 1 line of code for me to use the query parameter over the
header
On Apr 7, 2014 4:56 PM, "Stephen Houston" notifications@github.com wrote:

@challgren https://github.com/challgren Can you continue to use that
method? I'm not sure what we could set the value of "auth" to that would
convey using both the Basic Auth and API Key forms of authentication on a
single request.


Reply to this email directly or view it on GitHubhttps://github.com//pull/172#issuecomment-39788935
.

@egeek
Copy link
Author

egeek commented Apr 7, 2014

Cool @challgren

@mansilladev Think we are ok to merge in this pull request? Is there anything else we need to do?

@mlconnor
Copy link

This looks really good, I need this desperately. Right now, I'm running off egeeks fork. Thanks for the mods egeek.

@mlconnor
Copy link

One thing to point out, app.js line 814 throws an error if your config doesn't have any header data in it. I think the following should be added...

        if (apiConfig.auth == 'basicAuth') {
            if ( typeof options.headers == "undefined" ) {
              options.headers = {} ;
            }

@mansilladev
Copy link
Contributor

Basic auth feature added for both Node server level and API request level.

@matthewsmoore
Copy link

I was super excited to see that iodocs has added support for HTTP Basic Auth. I cannot seem to get it to work. Could you possibly help me a bit?

I’ve just in the last hour grabbed the latest code from:
git clone http://github.com/mashery/iodocs.git

but no matter where I make the change:
"auth":"basicAuth"

The IO docs page for the API is asking for an API Key and not a Username & Password.

By any chance do you have an example set of config file(s) that demonstrate how to configure basic auth?

Thanks so much!
-Matt

@phairow
Copy link
Contributor

phairow commented Oct 15, 2014

@matthewsmoore It looks like this API level basic auth feature no longer exists. also looks like the Node server level basic auth is broken. Fixing right now!

@phairow phairow reopened this Oct 15, 2014
This was referenced Oct 15, 2014
@phairow
Copy link
Contributor

phairow commented Oct 15, 2014

opened #217 and #216 to track these respectively

@phairow phairow closed this Oct 15, 2014
@phairow
Copy link
Contributor

phairow commented Oct 15, 2014

@matthewsmoore Both should work now, let me know if anything else needs fixing

@matthewsmoore
Copy link

WOW! That was fast. Got latest and it is now working exactly, perfectly right. Thank you!

@phairow
Copy link
Contributor

phairow commented Oct 15, 2014

@matthewsmoore Awesome! The code was there in the original PR but somehow it was lost in translation. Glad to hear it's working as expected :)

@amilkr
Copy link

amilkr commented Jan 16, 2015

+1 to this feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants