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

Add netrc as auth type #384

Merged
merged 6 commits into from Jul 31, 2017
Merged

Add netrc as auth type #384

merged 6 commits into from Jul 31, 2017

Conversation

@moxuz
Copy link
Contributor

moxuz commented Jul 27, 2017

What

Added the option for users to select netrc as an authentication type on a request.

Why

Let users use authentication found in their .netrc file rather than have to enter credentials into insomnia.

Details

A new dropdown item has been added to the Authentication dropdown per request. This item is named Netrc and displays text letting the user know their .netrc file will be used in the auth panel. This uses the Curl.option.NETRC option. Curl will automatically take the creds from the .netrc file for Unix systems and _netrc file for Windows systems.

Notes

  • Netrc is capitalized in the dropdown. This keeps it standardized with the other items, but let me know if you think it should stay lowercase.
  • The NETRCAuth component just returns some rendered text as JSX. Let me know if you think it needs some sort of user interaction.
  • The text of the Netrc auth panel might need some changing.

Testing

  • Works with netrc authenticated requests
  • Works with netrc authenticated requests with no .netrc file on system. (Fallsback to non-authenticated)
  • Other authentication still functions
  • Have not tested on Windows as I do not have access to a Windows OS with Insomnia. This documentation mentions Curl will automatically grab the _netrc file on windows.

Screenshots

screenshot 2017-07-27 17 10 11

screenshot 2017-07-27 16 29 29

Related Issue: Closes #241

Christophe Pouliot added 4 commits Jul 27, 2017
Christophe Pouliot
Christophe Pouliot
Christophe Pouliot
Copy link
Contributor

gschier left a comment

Looking pretty good @moxuz! Just a few minor things 👍

@@ -0,0 +1,14 @@
import React from 'react';

const NETRCAuth = () => {

This comment has been minimized.

Copy link
@gschier

gschier Jul 28, 2017

Contributor

Please use a PureComponent class here since it's more performant (functional components re-render every time no matter what).

I think I've removed all functional components from the codebase now.

@@ -14,6 +14,7 @@ declare class Curl {
HTTPPOST: number,
INFILESIZE: number,
KEYPASSWD: number,
NETRC: number,

This comment has been minimized.

Copy link
@gschier

gschier Jul 28, 2017

Contributor

Can you also add these to __mocks__/node-libcurl and add a basic test to network.test.js?

This comment has been minimized.

Copy link
@moxuz

moxuz Jul 28, 2017

Author Contributor

Sure! The only test I could think of would be pulling creds out of a mocked .netrc file and making sure the creds are in the request. Is this what you were thinking? Or is there a simpler way to just make sure the curl options were set?

This comment has been minimized.

Copy link
@gschier

gschier Jul 30, 2017

Contributor

I was thinking of something even simpler. If you look at the other tests in there, they basically just make sure the correct options are passed to libcurl. It's safe to assume that libcurl will do the right thing if it gets the correct options.

This comment has been minimized.

Copy link
@moxuz

moxuz Jul 30, 2017

Author Contributor

Added a simple test!

import BasicAuth from './basic-auth';
import DigestAuth from './digest-auth';
import BearerAuth from './bearer-auth';
import NTLMAuth from './ntlm-auth';
import OAuth2 from './o-auth-2';
import AWSAuth from './aws-auth';
import NETRCAuth from './netrc-auth';

This comment has been minimized.

Copy link
@gschier

gschier Jul 28, 2017

Contributor

Minor, but should this be called NetrcAuth instead?

This comment has been minimized.

Copy link
@gschier

gschier Jul 28, 2017

Contributor

Also, can you change OAuth2 to OAuth2Auth while you're here?

@@ -467,6 +467,8 @@ export function _actuallySend (
for (const header of extraHeaders) {
headers.push(header);
}
} else if (renderedRequest.authentication.type === AUTH_NETRC) {
setOpt(Curl.option.NETRC, Curl.netrc.REQUIRED);

This comment has been minimized.

Copy link
@gschier

gschier Jul 28, 2017

Contributor

What does REQUIRED mean here?

This comment has been minimized.

Copy link
@moxuz

moxuz Jul 28, 2017

Author Contributor

A user:password in the URL will be ignored. Unless one is set programmatically, the .netrc will be queried.
I thought this was a good way for it to behave, since falling back to the password in the URL might be werid behaviour. Could be set to Optional if you think that's better tho!

Christophe Pouliot added 2 commits Jul 28, 2017
Christophe Pouliot
Christophe Pouliot
Copy link
Contributor

gschier left a comment

Looks great @moxuz! Merging this in now. Thanks for the PR 😄

@gschier gschier merged commit 36d3bd5 into Kong:develop Jul 31, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.