Conversation
ericmj
left a comment
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 |
maennchen
force-pushed
the
jm/hex_auth
branch
2 times, most recently
from
4321c4e to
e56c3db
Compare
wojtekmach
left a comment
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
| %% @private | ||
| %% Resolve OAuth token with global lock to prevent concurrent refresh attempts. | ||
| resolve_oauth_token_with_context(Callbacks, Config) -> | ||
| global:trans( |
There was a problem hiding this comment.
It sounds like a good idea to have this but on principle I think we wanna keep hex_core low-level (maybe to a fault), process-less, ets-less, etc, not have any global things whatsoever. So yeah, not 100% sure it belongs here.
There was a problem hiding this comment.
What would you do as an alternative? hex has Hex.OnceCache to do the same and we need something like that in rebar as well.
Instead of a cache with locking, the new implementation persists all results directly. Therefore no cache is needed. I however still need a lock.
I could theoretically expose lock as a function that the implementation can provide instead of directly using global, but we would reimplement the same thing in both hex and rebar3.
| %% 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.
maennchen
force-pushed
the
jm/hex_auth
branch
2 times, most recently
from
cb231fa to
b8cea90
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
3 participants
Base Module organizing Repo / API Auth Management.
This PR is WIP. I'm exploring how it could look.
Implementation PRs:
hex_authmodule hex#1143