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

auth: Add :netrc | {:netrc, path} as possible values #52

Closed
wojtekmach opened this issue Nov 10, 2021 · 4 comments · Fixed by #75
Closed

auth: Add :netrc | {:netrc, path} as possible values #52

wojtekmach opened this issue Nov 10, 2021 · 4 comments · Fixed by #75

Comments

@wojtekmach
Copy link
Owner

Instead of having a distinct load_netrc step, make it an option to auth, e.g.: auth: :netrc which also would be the default?

@wojtekmach
Copy link
Owner Author

Let's still keep it as a distinct step, it's just auth/2 is going to use it under the hood.

@wojtekmach wojtekmach changed the title auth: Merge netrc step? auth: Add :netrc | {:netrc, path} as possible values Dec 27, 2021
@srowley
Copy link
Contributor

srowley commented Feb 6, 2022

Looking at this, I am not sure why keeping load_netrc as a step is desirable. Could you confirm?

If you run both steps, it will send two authorization headers, which I believe is not consistent with RFC7230.

It makes sense to me to keep load_netrc only as a private function to run under the hood - maybe this was the intent?

@srowley
Copy link
Contributor

srowley commented Feb 6, 2022

Separate question - load_netrc seems to be looking for a file with lines like:

machine localhost
username foo
password bar

But the documentation I can find here and here suggests it should be:

machine localhost
login foo
password bar

or

machine localhost login foo password bar

Should I stick with username or use login?

@wojtekmach
Copy link
Owner Author

oops, it should have been login, thanks!

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

Successfully merging a pull request may close this issue.

2 participants