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 support for reading credentials from .netrc files #925

Merged
merged 4 commits into from Dec 8, 2021

Conversation

frerich
Copy link
Contributor

@frerich frerich commented Dec 1, 2021

This makes Hex check an optional .netrc file for login credentials whenever making HTTP requests.

In case no authorization header is present in the request (e.g. as a result of configuring an auth_key for the repository), Hex will try to locate a .netrc file by checking the NETRC environment variable and by checking the user's home directory. In case a file is found, Hex parses it and tries to read login credentials for the host to which the HTTP request is made.

The file format understood by Hex largely follows the .netrc File Format of cURL: it is a plain-text file with multiple records, each consisting of three lines:

  • machine defines the host name to which the login credentials should apply
  • login gives the username
  • password gives the password

White space is not relevant, i.e. sections can be separated by empty lines and individual lines can be indented, e.g.:

machine api.example.com
  login foo
  password yoyodyne

machine another_machine.example.com
login bar
password 12345

Closes #924 .

@frerich
Copy link
Contributor Author

frerich commented Dec 1, 2021

At this time, the functionality is only tested on macOS (but I'm fairly confident that it works on all Unix-like systems).

Making this work on Windows might require some changes: it appears that the default configuration file name on Windows is _netrc, and I suspect that the parser would also need to be able to deal with Windows line-endings.

However, my understanding is that the netrc concept is very uncommon on Windows, so I guess that there's no pressing need to have this working on all supported platforms right away.

lib/hex/netrc.ex Outdated
end
end

defp parse_contents(contents) when is_binary(contents) do
Copy link
Member

Choose a reason for hiding this comment

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

We need to cache the result of the parsing to avoid reading the file on every HTTP request.

Copy link
Contributor Author

@frerich frerich Dec 2, 2021

Choose a reason for hiding this comment

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

I considered that, but then decided against it for three reasons:

  • I suspect the operating system is going to cache repeated reads of the file anyway, i.e. all but the first accesses are going to the cache.
  • The file is typically rather small (a couple hundred bytes) and the parsing logic is straightforward -- I don't expect a lot of CPU cycles to be burnt on reading the file.
  • Caching invalidation is hard to get right. 🤔

My gut feeling was that any time spent on reading this file is dwarfed by the time spent on actually doing the HTTP request, so I didn't pursue this idea further. 😬

That being said, I'm not dogmatic about this -- I don't mind adding some sort of cache if it helps with getting this feature in. 😄 Would you prefer if I go ahead and implement e.g. an ETS-based cache?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect the operating system is going to cache repeated reads of the file anyway, i.e. all but the first accesses are going to the cache.

That's probably true in many cases but in my experience it is hard to rely on this on machines with varying operating systems and settings.

The file is typically rather small (a couple hundred bytes) and the parsing logic is straightforward -- I don't expect a lot of CPU cycles to be burnt on reading the file.

I am more concerned about the disk IO. Some projects may have up to 100 dependencies which means we would be trying to read this file 200 times even if all packages were cached on disk.

Caching invalidation is hard to get right.

In this case it is easy because hex is always called from a short lived mix task so we don't have to invalidate. I think the tricky part is preventing the N first concurrent requests to not read the file in parallel when the cache is empty in the beginning.

Let's wait with the caching for now, I will think about if there is an easy way of implementing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's wait with the caching for now, I will think about if there is an easy way of implementing it.

How about renaming Hex.Netrc to Hex.Netrc.Parser and instead have Hex.Netrc be an agent which reads the .netrc file (if any) on startup and which can then be queried by callers via Hex.Netrc.lookup?

Copy link
Member

Choose a reason for hiding this comment

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

Reading at startup isn't great either because in many cases we wont be be doing HTTP requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true -- how about the Agent launches at startup, but only actually reads the .netrc file the first time lookup is called (i.e. it memorises the result of parsing in its state)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that would work 👍

lib/hex/netrc.ex Outdated Show resolved Hide resolved
@frerich
Copy link
Contributor Author

frerich commented Dec 2, 2021

I just re-wrote this PR to take care of all the incompatibilities with earlier Elixir versions detected by the CI.

@frerich
Copy link
Contributor Author

frerich commented Dec 2, 2021

@ericmj I now incorporated the Agent-based cache idea we discussed: instead of having a single Hex.Netrc module, there are now three modules:

  • Hex.Netrc.Parser implements the raw parsing logic (it is what Hex.Netrc used to be). It does not provide any means to do lookups.
  • Hex.Netrc.Cache implements an Agent which exposes a parse function. The agent checks if there's a cached parse result for the given path. If it is, it'll return that. Otherwise, it'll try to parse the given path -- if it succeeds, the parse result is memorised and returns. In case there's a parse error, nothing is memorised: the parse will be re-attempted on the next try. I.e. the agent tries to 'recover'.
  • Hex.Netrc is the public API, implementing just a single lookup function which attempts to lookup credentials in a .netrc file.

In my tests, this seems to invoke the parser just once -- but it turns out that the parser is still invoked. This is because the .netrc file is only checked if there's no authorization HTTP header set, which is always the case for me.

What do you think? :-)

Copy link
Member

@ericmj ericmj left a comment

Choose a reason for hiding this comment

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

This looks great. I only have one small comment.

{machines, Map.put(cache, path, machines)}

error ->
{error, cache}
Copy link
Member

Choose a reason for hiding this comment

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

If it fails to parse we should cache that also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing this up! 👍

By now, I tend to agree. My initial idea was to let the agent recover: if parsing fails, the agent will re-try so that you have a chance to fix things up (e.g. by creating the file) and have your changes kick in without restarting.

However, given how short-lived the cache (and the Hex application in general) is, I guess that doesn't help much. Hex does multiple successive requests in very little time. If the .netrc file would be stored on a high-latency filesystem, the errors might add up quite a bit. I'll fix this! 😊

Copy link
Member

@wojtekmach wojtekmach left a comment

Choose a reason for hiding this comment

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

This is really great work, I left a few comments below.

test "parse/2 fails on unreadable file" do
in_tmp(fn ->
File.write!(".netrc", "")
File.chmod(".netrc", 0o000)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
File.chmod(".netrc", 0o000)
File.chmod!(".netrc", 0o000)

Comment on lines 20 to 26
test "parse/2 fails on non-file" do
in_tmp(fn ->
File.mkdir(".netrc")
assert {:error, :eisdir} = Parser.parse(".netrc")
end)
end

Copy link
Member

Choose a reason for hiding this comment

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

it doesn't hurt to keep it, but I'd remove this test since at this point we're testing multiple possible errors of File.read, we already have tests for the most common :enoent and :eacces.

Suggested change
test "parse/2 fails on non-file" do
in_tmp(fn ->
File.mkdir(".netrc")
assert {:error, :eisdir} = Parser.parse(".netrc")
end)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was unsure about this myself but since the test is written already and it shouldn't do any harm, I'm inclined to just keep it now that I spent time on writing it. 😊

Copy link
Member

Choose a reason for hiding this comment

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

if the only reason to keep it is that we spent effort writing it, in other words, we shouldn't have written in the first place, then we should totally remove it. The effort wasn't definitely wasted though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll drop the test -- I concur that it provides very little extra value. 😬

end)
end

test "parse/2 ignores excessive whitespace" do
Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines 34 to 41
test "parse/2 fails on non-string" do
in_tmp(fn ->
assert_raise FunctionClauseError, fn ->
Parser.parse(true)
end
end)
end

Copy link
Member

Choose a reason for hiding this comment

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

it doesn't hurt to keep this either but I think it is totally fine if we have this path, totally invalid input, untested.

Suggested change
test "parse/2 fails on non-string" do
in_tmp(fn ->
assert_raise FunctionClauseError, fn ->
Parser.parse(true)
end
end)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After sleeping over it, I agree -- this test is maybe really taking things too far. I'll drop the test. 🥲

lib/hex/http.ex Outdated
url = URI.parse(url)

cond do
credentials = Hex.Netrc.lookup(url.host) ->
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: how about the following?

if credentials = ... do
  ...
else
  ...
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that! I somehow picked up the habit of preferring cond over if, but I'm not dogmatic about it. Will change!

lib/hex/netrc.ex Outdated
alias Hex.Netrc.Cache

def lookup(host) when is_binary(host) do
case Cache.parse() do
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: what do you think about calling it a more common for caches fetch?

Suggested change
case Cache.parse() do
case Cache.fetch() do

btw, instead of fetch() should we have fetch(host), that is we look up the machines in the agent process and not in the caller? The benefit is less copying though it definitely doesn't matter much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea -- Cache.fetch (or Cache.query?) sounds like a much more appropriate name! ❤️

As for where the lookup is done: in my first iteration, I did indeed have the lookup in the agent process itself. I decided to move it to the caller since I felt that this makes the purpose of the Hex.Netrc.Cache module more 'crisp': it only acts as a cache for the parser, it provides no functionality on top of that.

I don't mind changing this though, if you insist on avoiding the copying -- what do you think? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, even thought it is a micro optimisation (since .netrc files tend to be rather short) I feel like it is more natural to fetch only what you need from the cache, as opposed to fetching everything and then filtering it out in the caller.

end

defp default_path() do
Path.join(System.user_home(), ".netrc")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path.join(System.user_home(), ".netrc")
Path.join(System.user_home!(), ".netrc")

lib/hex/netrc.ex Outdated
Comment on lines 9 to 12
Map.get(machines, host)

error ->
error
Copy link
Member

Choose a reason for hiding this comment

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

looking at this it seems the return value is value | {:error, reason}, what do you think about a more common {:ok, value} | {:error, reason}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that sounds like a great idea. ❤️

Hmm, how would an unsuccessful lookup be represented, i.e. when parsing succeeds but the .netrc file has no credentials for the given host? {:ok, nil}? I guess that would be plausible...

I'll give it a shot! 🤟

@frerich
Copy link
Contributor Author

frerich commented Dec 3, 2021

Thanks to both of you for taking the time to review the code! ❤️

I now re-wrote this PR, incorporating most of your proposed changes. I also added new tests for both Hex.Netrc.Agent as well as Hex.Netrc to verify the desired behaviour as far as caching errors (or successes) goes.

I'd love to hear what you think! 😃

@frerich frerich force-pushed the feature/netrc_support branch 3 times, most recently from a71be42 to aa575f5 Compare December 4, 2021 13:14
lib/hex/netrc.ex Outdated
{:ok, %{} = machines} ->
{:ok, Map.get(machines, host)}

error ->
Copy link
Member

Choose a reason for hiding this comment

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

can Cache.fetch return {:ok, nil} and it would go to this clause? If so, instead of error, let's call this other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! 🥇

Indeed, {:ok, nil} is a perfectly legal cache result. error wouldn't fit that term at all. Will fix!


def fetch(path \\ Parser.netrc_path()) when is_binary(path) do
Agent.get_and_update(@agent_name, fn cache ->
case Map.get(cache, path) do
Copy link
Member

Choose a reason for hiding this comment

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

please use Map.fetch. Otherwise, I don't think we can cache a nil value!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point! I totally missed that this part would need to be able to cache nil values. 😊 Will fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I now realised that this part would not need to cache nil values -- at least in its current form (in which the result of Parser.parse is cached). However, Map.fetch reflects my intention better.

Comment on lines 20 to 26
test "parse/2 fails on non-file" do
in_tmp(fn ->
File.mkdir(".netrc")
assert {:error, :eisdir} = Parser.parse(".netrc")
end)
end

Copy link
Member

Choose a reason for hiding this comment

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

if the only reason to keep it is that we spent effort writing it, in other words, we shouldn't have written in the first place, then we should totally remove it. The effort wasn't definitely wasted though!

defp parse_line(_line, _parse_state), do: :parse_error

def netrc_path() do
System.get_env("NETRC", default_path())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
System.get_env("NETRC", default_path())
System.get_env("NETRC") || default_path()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, but I'm curious: is there a benefit to doing it that way? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

the benefit is it works on older Elixir versions where get_env/2 was not yet available. :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! That's a pretty compelling reason! 😄

Thanks for pointing that out -- I didn't even know that this is (or is going to be) a problem with older Elixir versions. :-)

Comment on lines +7 to +9
# Restart Hex between tests to get a fresh cache.
Application.stop(:hex)
:ok = Application.start(:hex)
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should work too, could you give it a shot?

Suggested change
# Restart Hex between tests to get a fresh cache.
Application.stop(:hex)
:ok = Application.start(:hex)
GenServer.stop(Hex.Netrc.Cache)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, that doesn't seem to work. It runs into

  1) test fetch/1 remembers parse errors (Hex.Netrc.CacheTest)
     test/hex/netrc/cache_test.exs:17
     ** (exit) exited in: GenServer.call(Hex.Netrc.Cache, {:get_and_update, #Function<0.20981771/1 in Hex.Netrc.Cache.fetch/1>}, 5000)
         ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started
     code: in_tmp(fn ->
     stacktrace:
       (elixir 1.12.3) lib/gen_server.ex:1014: GenServer.call/3
       test/hex/netrc/cache_test.exs:20: anonymous fn/0 in Hex.Netrc.CacheTest."test fetch/1 remembers parse errors"/1
       (elixir 1.12.3) lib/file.ex:1560: File.cd!/2
       test/hex/netrc/cache_test.exs:18: (test)

I indeed considered just stopping the cache but I wasn't sure whether it would get restarted in time for the next test to launch.

The approach of calling Application.stop/1 and Application.start/1 like that was taken straight from the Mix & OTP Guide which has

defmodule KVServerTest do
  use ExUnit.Case

  setup do
    Application.stop(:kv)
    :ok = Application.start(:kv)
  end
  ...

I thought -- if the guide does it, who am I to argue? :-)

Copy link
Member

Choose a reason for hiding this comment

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

oh, I thought I tested and it seemed to work, if we stop it, the application supervisor would restart it. But yeah, let's go with the recommendation from the guides.

in_tmp(fn ->
File.write!(".netrc", "")
assert {:error, :parse} = Cache.fetch(".netrc")
File.rm(".netrc")
Copy link
Member

Choose a reason for hiding this comment

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

remember to use !-variants as much as possible in the tests!

Suggested change
File.rm(".netrc")
File.rm!(".netrc")

Copy link
Contributor Author

@frerich frerich Dec 5, 2021

Choose a reason for hiding this comment

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

Thanks for pointing this out! I just forgot about that (in the other test for the cache, I did use rm! as intended). 😊

Will fix! 👍

lib/hex/netrc.ex Outdated
defmodule Hex.Netrc do
@moduledoc false

alias Hex.Netrc.{Cache, Parser}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
alias Hex.Netrc.{Cache, Parser}
alias Hex.Netrc.Cache
alias Hex.Netrc.Parser

please check any other build failures for older Elixir versions!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already started grinding my way through them. In the previous iterations of this PR (I keep force-pushing), you can see me doing away with with expression and use Agent. Fixing the latter made the compilation proceed, so I see it now complained about using this alias syntax with Elixir < 1.2. It's amazing how many quality of life improvements Elixir got in the past years. :-)

Will fix!

Netrc.lookup("foo.example.com", ".netrc")
end)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

great job on writing this and the other tests. We don't have the most important test though, the one for Hex.HTTP, that would make sure everything works end-to-end! Could you add it? Since we have great lower-level coverage just one happy and one sad path example should be all that we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I totally forgot about that! 🤦

I'll look into how the tests work and add some basic ones to exercise the success & failure cases. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wojtekmach I suppose it would be feasible to move the logic for constructing request headers into a separate function and then testing that, but I think it would be even nicer to be able to test Hex.HTTP.request as a whole.

Right now, this function is defined in terms of :httpc.request. Mocking that doesn't seem to be totally straightforward, but I see that Hex already uses 'Bypass' which maybe could be used here? It might be considered abuse, but what do you think about doing something along the lines of this to use Bypass for testing the HTTP requests sent?

diff --git a/test/hex/http_test.exs b/test/hex/http_test.exs
index 42e5cb7..f6c673a 100644
--- a/test/hex/http_test.exs
+++ b/test/hex/http_test.exs
@@ -10,7 +10,8 @@ defmodule Hex.HTTPTest do
       end)
     end)

-    :ok
+    bypass = Bypass.open()
+    {:ok, bypass: bypass}
   end

   test "proxy_config returns no credentials when no proxy supplied" do
@@ -51,4 +52,13 @@ defmodule Hex.HTTPTest do
     Hex.HTTP.handle_hex_message('"oops, you done goofed";level=fatal  ')
     assert_received {:mix_shell, :error, ["API error: oops, you done goofed"]}
   end
+
+  test "request", %{bypass: bypass} do
+    Bypass.expect(bypass, fn %Plug.Conn{req_headers: headers} = conn ->
+      assert Enum.any?(headers, fn {k, _v} -> k === "authorization" end)
+      Plug.Conn.resp(conn, 200, "")
+    end)
+
+    Hex.HTTP.request(:get, "http://localhost:#{bypass.port}", %{}, nil)
+  end
 end

I'd love to hear what you think. 😊

Copy link
Member

Choose a reason for hiding this comment

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

Yes, lets definitely test Hex.HTTP.request with the help of bypass. If we only use it in that one test, no need to adjust the setup block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! I now added a few tests which exercise the Hex.HTTP.request function a bit and check the submitted authorization HTTP header. Will re-write the PR shortly. :-)

@frerich frerich force-pushed the feature/netrc_support branch 2 times, most recently from 45e4e0c to 565e38b Compare December 5, 2021 14:04
@frerich
Copy link
Contributor Author

frerich commented Dec 5, 2021

@wojtekmach Thanks again for your thorough review! 😍

I now incorporated your suggestions and added some Hex.HTTP.request tests which verify that the function adds an authorization header as expected.

The one suggestion I did not end up implementing was changing the location where the cache lookup happens: having Cache mirror the Parser closely just seemed to nice to implement the optimisation you suggested. I hope this is acceptable. 😊

As far as I can tell, the only missing step would now be to iron out any remaining CI issues, if any. :-)

@frerich
Copy link
Contributor Author

frerich commented Dec 5, 2021

Looks like I was a little too optimistic; I now noticed

** (UndefinedFunctionError) undefined function: String.trim/1

In the test output. Will have as look later.

@ericmj
Copy link
Member

ericmj commented Dec 5, 2021

You can use Hex.Stdlib.string_trim/1 instead.

This parser aims to implement the file format understood by cURL:

* Whitespace is not relevant and can be used freely to align/group
  values

* 'machine', 'login' and 'password' are supported, and must come on
  their own lines.

In particular, commented-out lines are not supported, nor is having all
keywords on a single line ('machine foo.com login yoyo password dyne').

Also, none of this was tested on a Windows system. It might be that line
endings or default file locations should be adjusted here.
@frerich
Copy link
Contributor Author

frerich commented Dec 6, 2021

I now rewrote the parser commit such that it avoids using String.trim in order to fix compilation with Elixir <= 1.2. Let's see!

For the record, please don't hesitate to let me know if you prefer me doing incremental additions to this PR instead of rewriting history. 😊

@wojtekmach
Copy link
Member

it's up to you how you want to commit it, we'd most likely squash it before merging anyway :)

Copy link
Member

@wojtekmach wojtekmach left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a couple minor nitpicks left and let's ship it!

Comment on lines 22 to 27
:error ->
parse_result = Parser.parse(path)
{parse_result, Map.put(cache, path, parse_result)}

{:ok, cached_parse_result} ->
{cached_parse_result, cache}
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: happy path first:

Suggested change
:error ->
parse_result = Parser.parse(path)
{parse_result, Map.put(cache, path, parse_result)}
{:ok, cached_parse_result} ->
{cached_parse_result, cache}
{:ok, cached_parse_result} ->
{cached_parse_result, cache}
:error ->
parse_result = Parser.parse(path)
{parse_result, Map.put(cache, path, parse_result)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I like to have the happy path first, too. Thanks for pointing this out! 👍

Comment on lines 62 to 63
Bypass.expect(bypass, fn %Plug.Conn{req_headers: headers} = conn ->
assert not Enum.any?(headers, fn {k, _v} -> k === "authorization" end)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Bypass.expect(bypass, fn %Plug.Conn{req_headers: headers} = conn ->
assert not Enum.any?(headers, fn {k, _v} -> k === "authorization" end)
Bypass.expect(bypass, fn conn ->
assert Plug.Conn.get_req_header(conn, "authorization") == []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! ❤️

I knew there must be a better way of doing this, but I failed to find Conn.get_req_header. Thanks for the hint! :-)

Comment on lines 81 to 87
expected_auth_header = {"authorization", "Basic #{:base64.encode("john:doe")}"}

Bypass.expect(bypass, fn %Plug.Conn{req_headers: headers} = conn ->
assert expected_auth_header in headers
Plug.Conn.resp(conn, 200, "")
end)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expected_auth_header = {"authorization", "Basic #{:base64.encode("john:doe")}"}
Bypass.expect(bypass, fn %Plug.Conn{req_headers: headers} = conn ->
assert expected_auth_header in headers
Plug.Conn.resp(conn, 200, "")
end)
Bypass.expect(bypass, fn conn ->
assert Plug.Conn.get_req_header(conn, "authorization") == [
"Basic #{:base64.encode("john:doe")}"
]
Plug.Conn.resp(conn, 200, "")
end)

could you perform similar change below too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do -- that's a nice improvement. Thanks :-)

This agent is started at the very beginning; it avoids having to parse
.netrc files repeatedly. Instead of having to invoke
Hex.Netrc.Parser.parse/1, callers can use Hex.Netrc.Cache.parse/1 to
have the result of the parser get cached.

Both successful as well as unsuccessful parse results will be cached,
i.e. if parsing the given file failed once, that error will be returned
on subsequent cache requests for the same path. This is for two reasons:

* The cache process is rather short-lived: there's very little
  opportunity to fix any parse problems (e.g. by creating a .netrc file)
  while Hex is running.
* The cache receives multiple quiries in quick succession; afailing
  parse for a file which is stored on a high-latency file system (e.g.
  NFS) might slow down dependency resolution a lot.
This is the entry point to the .netrc-based login credential resolution.

Given a host name and an (optional) path to a .netrc file, it tries to
lookup login credentials to be used for that host.
@frerich
Copy link
Contributor Author

frerich commented Dec 6, 2021

@wojtekmach Here's another iteration of the PR, incorporating the improvements you suggested. :-)

@frerich
Copy link
Contributor Author

frerich commented Dec 6, 2021

I must confess that the reason for the hex_api test failure eludes me. They work locally. 🤔

I noticed that it's not every combination of OTP and Elixir which fails, but it's always authorisation-related.

Working theory: there is an interplay between two separate behaviours:

  1. tests are executed in an undefined order (or maybe even concurrently?) and in the failing case, the new netrc-related tests and/or the Hex.HTTP tests execute before the Hex.APITest tests.
  2. the netrc-related tests (or the HTTP tests) fail to clean up after themselves: the last test case does not re-set the NETRC environment variable or the Cache agent.

The consequence might be that the NETRC variable and the Cache agent state spills over to the Hex.APITest, i.e. the HTTP.request function unexpectedly picks up the fake cached .netrc credentials and adds an unexpected authorisation header.

Does this sound plausible? If so, I suppose a good fix might be to have the tests un-set the NETRC environment variable when they are done with it -- or find a different way (other than setting NETRC) to point Hex to a custom .netrc file when unit-testing the cache or HTTP.request.

What do you think?

In case request/5 is invoked without passing any 'authorization' header,
the function queries an optional .netrc configuration file for login
credentials. If found, a Basic Auth HTTP header is computed and added to
the request.

This permits making requests to websites which use HTTP Basic auth
without having to encode username and password right into the URL.
@frerich
Copy link
Contributor Author

frerich commented Dec 7, 2021

I now augmented the Let Hex.HTTP.request/5 consider .netrc files commit such that the NETRC variable is deleted after each test. This is a quick test to see if it helps with the other CI test failures which are supposedly caused by the environment variable spilling over to other tests.

@wojtekmach wojtekmach merged commit 1f9550a into hexpm:main Dec 8, 2021
@wojtekmach
Copy link
Member

Thank you!

@frerich frerich deleted the feature/netrc_support branch February 14, 2023 14:24
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 this pull request may close these issues.

mix hex.package fetch: Add support for .netrc files
3 participants