Introduce hex_auth module#178
Conversation
ericmj
left a comment
There was a problem hiding this comment.
I am not sure this fits in hex_core. It's very specific to implementing OAuth for a CLI, specifically to how Hex and rebar3 work with config resolution order and environment variables. hex_core should be more generic than that, for example you should be able to use it in web services, like hexdocs or hexdiff, and this wouldn't fit for that.
I understand we want to avoid auth logic between Hex and rebar3 if it's mostly shared, but I am not sure it fits in hex_core.
|
@ericmj I'm aware that this is very much scoped towards CLIs. Is there a reason to not include a CLI only module into I would love to be able to reuse the auth logic that is frankly quite complicated between our clients. That way we can minimize maintenance work, are able to roll out features more quickly and also ensure better compatibility. |
I think we can do that if we make that clear in the docs and maybe scoping under a I am still am bit hesitant at keeping it so close to how Hex and Rebar3 works with environment variables and config resolution. Would every CLI client want to do it the same way? |
That sounds like a reasonable name.
Let's not do env variables. In the case of rebar, that would be better handled in the |
4321c4e to
e56c3db
Compare
wojtekmach
left a comment
There was a problem hiding this comment.
This looks solid and well tested, I left some comments below.
| case proplists:get_value(name, Opts) of | ||
| undefined -> Params0; | ||
| Name -> Params0#{<<"name">> => Name} | ||
| undefined -> get_hostname(); |
There was a problem hiding this comment.
just double-checking, why do we need to send user's hostname?
There was a problem hiding this comment.
This is passed here to identify the device code reuqest:
https://github.com/hexpm/hexpm/blob/main/lib/hexpm/oauth/device_codes.ex#L33
We already do this presently: https://github.com/hexpm/hex/blob/main/lib/mix/tasks/hex.ex#L141
| %% hex_cli_auth:with_api(Callbacks, write, Config, fun(C) -> | ||
| %% hex_api_release:publish(C, Tarball) | ||
| %% end, [{optional, false}, {auth_inline, true}]). |
There was a problem hiding this comment.
I'm curious about callbacks-based API and wrapping user code (here, hex_api_release:publish/2) in fun vs a more traditional imperative API. Are we wrapping user code in a fun because we might be doing multiple requests? Or we wanna trigger lifecycle hooks when we exit user code? (Or both?) Maybe just as an exercise worth exploring how an imperative API without callbacks would look like--or such is impossible, i.e. misses the point of having this module in the first place?
There was a problem hiding this comment.
Yes, this will possibly do multiple requests. For example to ask for an OTP token.
cb231fa to
b8cea90
Compare
|
One thing we have think about this approach, where we model hex_core very closely to how Hex/Rebar3 currently works. What happens when I want to add a new feature to Hex, for example meta organizations/repos that can contain sub repos. Rebar3 might lag behind adding this feature or may not want to add it all, so now |
|
@ericmj I believe that we can deal with this when we are making changes as long as we consider rebar when making them. We can choose to build it in a way where there's no trouble not implementing a new feature, make it conditional behind a flag etc. Worst case, we can choose to fork the module. |
e13da1b to
c4c38cc
Compare
Add a high-level function that handles the complete OAuth device authorization flow: initiating authorization, prompting the user, and polling for token completion. - Add oauth_tokens/0 and device_auth_error/0 types - Handle slow_down, authorization_pending, expired_token, and access_denied responses during polling
Add config options for hex_cli_auth authentication handling: - trusted: whether the repo connection is trusted for auth_key usage - oauth_exchange: whether to exchange api_key/auth_key for OAuth tokens - oauth_exchange_url: optional custom URL for OAuth token exchange
Provides callback-based authentication for build tools (rebar3, Mix) with shared auth logic and customizable prompting/persistence. Features: - with_api/4,5 and with_repo/3,4 wrappers for authenticated requests - resolve_api_auth/3 and resolve_repo_auth/2 for credential resolution - Auth resolution order: config, per-repo keys, parent repo, OAuth - OAuth token exchange via client_credentials grant - Automatic token refresh when expired - OTP/TOTP prompt handling with retry logic - Device auth flow for interactive authentication - Support for trusted/untrusted repo connections Includes comprehensive test suite covering: - API and repo auth resolution - Trusted vs untrusted scenarios - OAuth token exchange (new, valid, expired) - OTP prompts and retries - Token refresh on 401 - Device auth flow
| %% Resolve OAuth token with global lock to prevent concurrent refresh attempts. | ||
| resolve_oauth_token_with_context(Callbacks, Config) -> | ||
| global:trans( | ||
| {{?MODULE, token_refresh}, self()}, |
There was a problem hiding this comment.
Should we remove self()? Afaict this will only serialize for the current process.
There was a problem hiding this comment.
My understanding of global is that we need to identify who is requesting the lock. It does not have to be a pid, but needs to be unique for each request.
There was a problem hiding this comment.
Sorry, I think I'm wrong. I'll write a good test and see what happens.
There was a problem hiding this comment.
It was correct, but [] nodes was not. I added a test specifically for it.
| {"cmd", ["/c", "start", "", UrlStr]} | ||
| end, | ||
| Port = open_port({spawn_executable, os:find_executable(Cmd)}, [{args, Args}]), | ||
| port_close(Port), |
There was a problem hiding this comment.
Should we immediately close here? That's not how we do it in hex, we just let the OS clean it up. I am afraid this could race since open_port is async afaik.
| {win32, _} -> | ||
| {"cmd", ["/c", "start", "", UrlStr]} | ||
| end, | ||
| Port = open_port({spawn_executable, os:find_executable(Cmd)}, [{args, Args}]), |
There was a problem hiding this comment.
We should handle if os:find_executable/1 cannot find an executable.
| with_repo(Callbacks, BaseConfig, Fun, Opts) -> | ||
| Optional = proplists:get_value(optional, Opts, true), | ||
| AuthInline = proplists:get_value(auth_inline, Opts, false), | ||
| case resolve_repo_auth(Callbacks, BaseConfig) of |
There was a problem hiding this comment.
(Callbacks, BaseConfig)
Just wondering if we should put callbacks under config?
Would users specify different functions for the same callback names in different call sites? (Even then, they could just modify Config appropriately before calling the function I suppose.) That said, config already has a bunch of stuff so I suppose it'd have to be under oauth_callbacks or similar.
I think the call sites would look something like this:
f(#{oauth_callbacks := #{should_authenticate := Callback}} = Config) ->
Callback(...),which I think is ergonomic enough and it avoids the call_callback indirection. (Nothing wrong with it of course.)
There was a problem hiding this comment.
So far in each implementation I used a fixed set of callbacks.
I did not add it to config since that has the type hex_core:config() and I did not want to add it globally.
There was a problem hiding this comment.
I am ok with leaving this as is.
Just mentioning in case it resonates with anyone,
I did not add it to config since that has the type hex_core:config() and I did not want to add it globally.
I think on the hex core side, the change would probably be something like this:
--- a/src/hex_core.erl
+++ b/src/hex_core.erl
@@ -126,7 +126,17 @@
metadata_fields => all | [binary()],
trusted => boolean(),
oauth_exchange => boolean(),
- oauth_exchange_url => binary() | undefined
+ oauth_exchange_url => binary() | undefined,
+ oauth_callbacks =>
+ undefined |
+ #{
+ get_auth_config => (binary() -> ...),
+ get_oauth_tokens => (-> ...),
+ persist_oauth_tokens => (..., ..., ..., ... -> ...),
+ prompt_otp => (binary() -> ...),
+ should_authenticate => (... -> ...),
+ get_client_id => (-> ...)
+ }
}.
-spec default_config() -> config().
@@ -157,5 +167,6 @@ default_config() ->
metadata_fields => all,
trusted => true,
oauth_exchange => true,
- oauth_exchange_url => undefined
+ oauth_exchange_url => undefined,
+ oauth_callbacks => undefined
}.and call sites in this PR. And then in hexpm/hex, we have:
$ rg mix_hex_core.default_config lib
lib/hex/repo.ex
307: :mix_hex_core.default_config()
lib/hex/http.ex
14: :mix_hex_core.default_config()
lib/hex/api/client.ex
6: :mix_hex_core.default_config()
lib/hex/tar.ex
18: :mix_hex_core.default_config()
so I think it'd be a good forcing function to have, perhaps, a single:
defmodule Hex do
def hex_core_config do
%{:hex_core.default_config | ...}
end
endwhich seems like maybe a step in the right direction, having a single place to instantiate the config and thread it through the code.
There was a problem hiding this comment.
I’m coming around to the idea. I wouldn’t call them oauth_callbacks though. Possibly cli_auth_callbacks.
@ericmj What do you think?
Base Module organizing Repo / API Auth Management.
This PR is WIP. I'm exploring how it could look.
Implementation PRs:
hex_authmodule hex#1143