From 766ae6190b9ae3bd53c7bd7234b582be60e927e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sun, 2 Aug 2026 22:48:19 +0200 Subject: [PATCH 1/5] Carry the organizations a session must re-authenticate for An organization can require its members to authenticate through its identity provider, and that authentication expires on a clock the organization sets. When it lapses, the token grant drops the organization's scopes and names them in sso_reauth_required rather than leaving the client to guess why a fetch started 403ing. The name is the whole point: a scope dropped because the member was removed is not named, because authenticating again would not give it back. Only the ones a browser visit would fix are. sso_authorization/2 asks for the URL that does the fixing. It is bound to the session asking, so opening it renews that session rather than starting a new one, and refresh_tokens/1 is how a build tool picks up the scopes afterwards without waiting out an access token that has not expired. The sso_reauth callback is optional and reports the flagged set after every grant, including as an empty list. Which of them the running command actually needs is the build tool's question, not this module's. --- src/hex_api_oauth.erl | 62 ++++++++++++++--- src/hex_cli_auth.erl | 56 ++++++++++++++- test/hex_api_SUITE.erl | 36 ++++++++++ test/hex_cli_auth_SUITE.erl | 120 +++++++++++++++++++++++++++++++++ test/support/hex_http_test.erl | 9 +++ 5 files changed, 272 insertions(+), 11 deletions(-) diff --git a/src/hex_api_oauth.erl b/src/hex_api_oauth.erl index e1e0e6ce..59c8c785 100644 --- a/src/hex_api_oauth.erl +++ b/src/hex_api_oauth.erl @@ -8,6 +8,8 @@ device_auth_flow/5, poll_device_token/3, refresh_token/3, + sso_authorization/2, + open_browser/1, revoke_token/3, client_credentials_token/4, client_credentials_token/5 @@ -18,7 +20,11 @@ -type oauth_tokens() :: #{ access_token := binary(), refresh_token => binary() | undefined, - expires_at := integer() + expires_at := integer(), + %% Organizations the session must authenticate against their identity + %% provider for. Their scopes are not in this token and re-requesting them + %% will not help; see sso_authorization/2. + sso_reauth_required => [binary()] }. -type device_auth_error() :: @@ -181,7 +187,8 @@ poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) -> {ok, #{ access_token => AccessToken, refresh_token => RefreshToken, - expires_at => TokenExpiresAt + expires_at => TokenExpiresAt, + sso_reauth_required => sso_reauth_required(TokenResponse) }}; {ok, {400, _, #{<<"error">> := <<"authorization_pending">>}}} -> poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt); @@ -260,6 +267,30 @@ refresh_token(Config, ClientId, RefreshToken) -> }, hex_api:post(Config, Path, Params). +%% @doc +%% Requests a URL for authenticating the current session against organizations +%% that require single sign-on. +%% +%% The session the access token belongs to is the one being authorized: its +%% owner opens the URL in a browser, completes SSO, and the next token refresh +%% carries the scopes again. The URL is single-use and short-lived. +%% +%% Examples: +%% +%% ``` +%% 1> Config = hex_core:default_config(). +%% 2> hex_api_oauth:sso_authorization(Config, [<<"acme">>]). +%% {ok, {201, _, #{ +%% <<"verification_uri">> => <<"https://hex.pm/sso/authorize/...">>, +%% <<"expires_in">> => 600 +%% }}} +%% ''' +%% @end +-spec sso_authorization(hex_core:config(), [binary()]) -> hex_api:response(). +sso_authorization(Config, Organizations) -> + Path = <<"oauth/sso_authorization">>, + hex_api:post(Config, Path, #{<<"organizations">> => Organizations}). + %% @doc %% Exchanges an API key for an OAuth access token using the client credentials grant. %% @@ -341,13 +372,13 @@ revoke_token(Config, ClientId, Token) -> }, hex_api:post(Config, Path, Params). -%%==================================================================== -%% Internal functions -%%==================================================================== - -%% @private -%% Open a URL in the default browser. -%% Uses platform-specific commands: open (macOS), xdg-open (Linux), start (Windows). +%% @doc +%% Opens a URL in the default browser. +%% +%% Uses the platform's opener: `open' on macOS, `xdg-open' on Linux, `start' +%% on Windows. Returns `{error, browser_not_found}' when none of them exists, +%% which is the ordinary case on a headless machine. +%% @end -spec open_browser(binary()) -> ok | {error, browser_not_found}. open_browser(Url) when is_binary(Url) -> ok = ensure_valid_http_url(Url), @@ -369,6 +400,19 @@ open_browser(Url) when is_binary(Url) -> ok end. +%%==================================================================== +%% Internal functions +%%==================================================================== + +%% @private +%% Older servers do not send the field at all, which means nothing is lapsed. +-spec sso_reauth_required(map()) -> [binary()]. +sso_reauth_required(TokenResponse) -> + case maps:get(<<"sso_reauth_required">>, TokenResponse, []) of + Organizations when is_list(Organizations) -> Organizations; + _Other -> [] + end. + %% @private %% Validates that a URL uses http:// or https:// scheme. -spec ensure_valid_http_url(binary()) -> ok. diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index 11a7fdd1..97d98dd8 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -35,6 +35,13 @@ %% %% holding the token-refresh lock. %% clear_oauth_tokens => fun(() -> ok), %% +%% %% Report the organizations the server says this session has to +%% %% authenticate against their identity provider for (optional). Called +%% %% after every token grant, with the empty list when there are none, so +%% %% the build tool always holds the current set. It is not told which of +%% %% them the running command needs; deciding that is the build tool's job. +%% sso_reauth => fun(([binary()]) -> ok), +%% %% %% User interaction %% prompt_otp => fun((Message :: binary()) -> {ok, OtpCode :: binary()} | cancelled), %% should_authenticate => fun((Reason :: no_credentials | token_refresh_failed) -> boolean()), @@ -87,7 +94,8 @@ with_repo/2, with_repo/3, resolve_api_auth/2, - resolve_repo_auth/1 + resolve_repo_auth/1, + refresh_tokens/1 ]). -export_type([ @@ -120,6 +128,7 @@ ) -> ok ), clear_oauth_tokens => fun(() -> ok), + sso_reauth => fun((Organizations :: [binary()]) -> ok), prompt_otp := fun((Message :: binary()) -> {ok, OtpCode :: binary()} | cancelled), should_authenticate := fun((Reason :: auth_prompt_reason()) -> boolean()), get_client_id := fun(() -> binary()) @@ -394,6 +403,32 @@ execute_optional_with_retry(BaseConfig, Fun, Opts) -> Other end. +%% @doc +%% Refreshes the stored global OAuth token now, whether or not it has expired. +%% +%% What a token carries can change without it expiring: authenticating a +%% session against an organization's identity provider grants scopes the +%% current access token was minted without. This is how a build tool picks +%% those up rather than waiting out the access token. +-spec refresh_tokens(hex_core:config()) -> ok | {error, auth_error()}. +refresh_tokens(Config) -> + global:trans( + {{?MODULE, token_refresh}, self()}, + fun() -> + case call_callback(Config, get_oauth_tokens, []) of + {ok, Tokens} -> + case maybe_refresh_token_with_context(Config, Tokens) of + {ok, _BearerToken, _AuthContext} -> ok; + {error, _Reason} = Error -> Error + end; + error -> + {error, {auth_error, no_credentials}} + end + end, + [node()], + infinity + ). + %%==================================================================== %% Internal functions - Device Auth %%==================================================================== @@ -412,10 +447,13 @@ device_auth(Config, Scope, Opts) -> end, FlowOpts = [{open_browser, OpenBrowser}], case hex_api_oauth:device_auth_flow(Config, ClientId, Scope, PromptUser, FlowOpts) of - {ok, #{access_token := AccessToken, refresh_token := RefreshToken, expires_at := ExpiresAt}} -> + {ok, + #{access_token := AccessToken, refresh_token := RefreshToken, expires_at := ExpiresAt} = + Tokens} -> ok = call_callback(Config, persist_oauth_tokens, [ global, AccessToken, RefreshToken, ExpiresAt ]), + report_sso_reauth(Config, Tokens), {ok, #{ access_token => AccessToken, refresh_token => RefreshToken, @@ -648,6 +686,7 @@ maybe_refresh_token_with_context(Config, #{refresh_token := RefreshToken}) when ok = call_callback(Config, persist_oauth_tokens, [ global, NewAccessToken, NewRefreshToken, ExpiresAt ]), + report_sso_reauth(Config, TokenResponse), BearerToken = <<"Bearer ", NewAccessToken/binary>>, HasRefreshToken = is_binary(NewRefreshToken), {ok, BearerToken, #{source => oauth, has_refresh_token => HasRefreshToken}}; @@ -780,6 +819,19 @@ call_callback(Config, Name, Args) -> Fun = maps:get(Name, Callbacks), erlang:apply(Fun, Args). +%% @private +%% Hands the build tool the organizations this session has to authenticate for. +%% Always called after a grant, including with the empty list, so a set that +%% has been resolved does not linger. +report_sso_reauth(Config, #{sso_reauth_required := Organizations}) when is_list(Organizations) -> + maybe_call_callback(Config, sso_reauth, [Organizations]); +report_sso_reauth(Config, #{<<"sso_reauth_required">> := Organizations}) when + is_list(Organizations) +-> + maybe_call_callback(Config, sso_reauth, [Organizations]); +report_sso_reauth(Config, _Tokens) -> + maybe_call_callback(Config, sso_reauth, [[]]). + %% @private %% Like call_callback/3 but for optional callbacks: returns ok without doing %% anything when the callback is not provided. diff --git a/test/hex_api_SUITE.erl b/test/hex_api_SUITE.erl index 78412f3d..60eeaceb 100644 --- a/test/hex_api_SUITE.erl +++ b/test/hex_api_SUITE.erl @@ -33,6 +33,8 @@ all() -> oauth_device_auth_flow_denied_test, oauth_device_auth_flow_timeout_test, oauth_refresh_token_test, + oauth_sso_authorization_test, + oauth_device_auth_flow_sso_reauth_test, oauth_revoke_test, oauth_client_credentials_test, publish_with_expect_header_test, @@ -242,6 +244,40 @@ oauth_refresh_token_test(_Config) -> ?assert(is_integer(ExpiresIn)), ok. +oauth_sso_authorization_test(_Config) -> + {ok, {201, _, Response}} = hex_api_oauth:sso_authorization(?CONFIG, [<<"acme">>]), + #{ + <<"verification_uri">> := VerificationUri, + <<"expires_in">> := ExpiresIn + } = Response, + ?assertEqual(<<"https://hex.pm/sso/authorize/acme">>, VerificationUri), + ?assert(is_integer(ExpiresIn)), + ok. + +oauth_device_auth_flow_sso_reauth_test(_Config) -> + % The organizations a token was minted without reach the caller + ClientId = <<"cli">>, + Scope = <<"repositories">>, + Self = self(), + PromptUser = fun(_VerificationUri, _UserCode) -> ok end, + + SuccessPayload = #{ + <<"access_token">> => <<"test_access_token">>, + <<"refresh_token">> => <<"test_refresh_token">>, + <<"token_type">> => <<"Bearer">>, + <<"expires_in">> => 3600, + <<"sso_reauth_required">> => [<<"acme">>] + }, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + Self ! + {hex_http_test, oauth_device_response, + {ok, {200, Headers, term_to_binary(SuccessPayload)}}}, + + {ok, Tokens} = hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser), + + ?assertEqual([<<"acme">>], maps:get(sso_reauth_required, Tokens)), + ok. + oauth_revoke_test(_Config) -> % Test token revocation ClientId = <<"cli">>, diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index 732da0a7..04c6eba8 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -50,6 +50,12 @@ all() -> with_api_otp_cancelled_test, with_api_otp_max_retries_test, + %% sso re-authorization + sso_reauth_reported_on_refresh_test, + sso_reauth_reported_empty_test, + refresh_tokens_forces_a_refresh_test, + refresh_tokens_without_credentials_test, + %% with_api tests - token refresh on 401 with_api_token_expired_refresh_test, @@ -1171,6 +1177,118 @@ device_auth_concurrent_serialized_reuses_login_test(_Config) -> %% Helper Functions %%==================================================================== +sso_reauth_reported_on_refresh_test(_Config) -> + %% The organizations the server flags on a refresh reach the build tool. + Now = erlang:system_time(second), + Self = self(), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"expired_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now - 100 + }}, + sso_reauth => fun(Organizations) -> + Self ! {sso_reauth, Organizations}, + ok + end + }), + + queue_refresh_response(#{<<"sso_reauth_required">> => [<<"acme">>]}), + + {ok, _ApiKey, _AuthContext} = hex_cli_auth:resolve_api_auth(read, Config), + + receive + {sso_reauth, Organizations} -> ?assertEqual([<<"acme">>], Organizations) + after 100 -> + error(sso_reauth_not_called) + end, + ok. + +sso_reauth_reported_empty_test(_Config) -> + %% A server that says nothing means nothing is lapsed, and the build tool + %% is told so rather than left holding a stale set. + Now = erlang:system_time(second), + Self = self(), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"expired_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now - 100 + }}, + sso_reauth => fun(Organizations) -> + Self ! {sso_reauth, Organizations}, + ok + end + }), + + {ok, _ApiKey, _AuthContext} = hex_cli_auth:resolve_api_auth(read, Config), + + receive + {sso_reauth, Organizations} -> ?assertEqual([], Organizations) + after 100 -> + error(sso_reauth_not_called) + end, + ok. + +refresh_tokens_forces_a_refresh_test(_Config) -> + %% A token that has not expired is still refreshed: what it carries can + %% change without its lifetime running out. + Now = erlang:system_time(second), + Self = self(), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"valid_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }}, + persist_oauth_tokens => fun(Scope, Access, Refresh, Expires) -> + Self ! {persisted, Scope, Access, Refresh, Expires}, + ok + end + }), + + queue_refresh_response(#{<<"access_token">> => <<"renewed_token">>}), + + ?assertEqual(ok, hex_cli_auth:refresh_tokens(Config)), + + receive + {persisted, global, Access, _Refresh, _Expires} -> + ?assertEqual(<<"renewed_token">>, Access) + after 100 -> + error(token_not_persisted) + end, + ok. + +refresh_tokens_without_credentials_test(_Config) -> + Config = config_with_callbacks(#{}), + + ?assertEqual( + {error, {auth_error, no_credentials}}, + hex_cli_auth:refresh_tokens(Config) + ), + ok. + +%% @private +%% Plants the next refresh response the test HTTP adapter will hand back, +%% merged over a working one so a test only states what it cares about. +queue_refresh_response(Overrides) -> + Payload = maps:merge( + #{ + <<"access_token">> => <<"new_access_token">>, + <<"refresh_token">> => <<"new_refresh_token">>, + <<"token_type">> => <<"Bearer">>, + <<"expires_in">> => 3600 + }, + Overrides + ), + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + self() ! + {hex_http_test, oauth_refresh_response, {ok, {200, Headers, term_to_binary(Payload)}}}, + ok. + config_with_callbacks(Opts) -> ?CONFIG#{cli_auth_callbacks => make_callbacks(Opts)}. @@ -1180,6 +1298,7 @@ make_callbacks(Opts) -> ShouldAuthenticate = maps:get(should_authenticate, Opts, fun(_) -> false end), PersistFn = maps:get(persist_oauth_tokens, Opts, fun(_, _, _, _) -> ok end), ClearFn = maps:get(clear_oauth_tokens, Opts, fun() -> ok end), + SsoReauthFn = maps:get(sso_reauth, Opts, fun(_Organizations) -> ok end), DefaultGetOAuthTokens = fun() -> maps:get(oauth_tokens, Opts, error) end, GetOAuthTokensFn = maps:get(get_oauth_tokens, Opts, DefaultGetOAuthTokens), @@ -1188,6 +1307,7 @@ make_callbacks(Opts) -> get_oauth_tokens => GetOAuthTokensFn, persist_oauth_tokens => PersistFn, clear_oauth_tokens => ClearFn, + sso_reauth => SsoReauthFn, prompt_otp => PromptOtp, should_authenticate => ShouldAuthenticate, get_client_id => fun() -> <<"test_client">> end diff --git a/test/support/hex_http_test.erl b/test/support/hex_http_test.erl index 01a88e76..efb2b800 100644 --- a/test/support/hex_http_test.erl +++ b/test/support/hex_http_test.erl @@ -405,6 +405,15 @@ fixture(post, <>, _, {_, Body}) -> {ok, {400, api_headers(), term_to_binary(ErrorPayload)}} end; +fixture(post, <>, _, {_, Body}) -> + #{<<"organizations">> := Organizations} = binary_to_term(Body), + Joined = iolist_to_binary(lists:join(<<"-">>, Organizations)), + Payload = #{ + <<"verification_uri">> => <<"https://hex.pm/sso/authorize/", Joined/binary>>, + <<"expires_in">> => 600 + }, + {ok, {201, api_headers(), term_to_binary(Payload)}}; + fixture(post, <>, _, _) -> % OAuth revoke always returns 200 OK per RFC 7009 {ok, {200, api_headers(), term_to_binary(nil)}}; From eb5508af1d711cb496bbbdfd64069a5d36339742 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sat, 22 Aug 2026 13:33:42 +0200 Subject: [PATCH 2/5] Normalize sso_reauth_required once and share the refresh lock report_sso_reauth/2 is one clause taking the list; the refresh path normalizes the raw token response with hex_api_oauth:sso_reauth_required/1 (now an exported private helper) and the device path destructures the map device_auth_flow already normalized. with_token_refresh_lock/2 carries the global lock and stored-token fetch for both resolution and forced refresh; a failed forced refresh still leaves the stored token in place for the caller to handle. has_refresh_token is derived from the device-auth result instead of hardcoded, the unreachable env source leaves the auth_context type, open_browser/1 is private again, and is_token_expired/1 is exported so build tools stop reimplementing the expiry buffer. --- src/hex_api_oauth.erl | 42 +++++++-------- src/hex_cli_auth.erl | 123 +++++++++++++++++++++--------------------- 2 files changed, 84 insertions(+), 81 deletions(-) diff --git a/src/hex_api_oauth.erl b/src/hex_api_oauth.erl index 59c8c785..363c983a 100644 --- a/src/hex_api_oauth.erl +++ b/src/hex_api_oauth.erl @@ -9,7 +9,7 @@ poll_device_token/3, refresh_token/3, sso_authorization/2, - open_browser/1, + sso_reauth_required/1, revoke_token/3, client_credentials_token/4, client_credentials_token/5 @@ -372,13 +372,26 @@ revoke_token(Config, ClientId, Token) -> }, hex_api:post(Config, Path, Params). -%% @doc -%% Opens a URL in the default browser. -%% -%% Uses the platform's opener: `open' on macOS, `xdg-open' on Linux, `start' -%% on Windows. Returns `{error, browser_not_found}' when none of them exists, -%% which is the ordinary case on a headless machine. -%% @end +%% @private +%% Organizations a token response says the session has to authenticate against +%% their identity provider for. Older servers do not send the field at all, +%% which means nothing is lapsed. +-spec sso_reauth_required(map()) -> [binary()]. +sso_reauth_required(TokenResponse) -> + case maps:get(<<"sso_reauth_required">>, TokenResponse, []) of + Organizations when is_list(Organizations) -> Organizations; + _Other -> [] + end. + +%%==================================================================== +%% Internal functions +%%==================================================================== + +%% @private +%% Opens a URL in the default browser using the platform's opener: `open' on +%% macOS, `xdg-open' on Linux, `start' on Windows. Returns +%% `{error, browser_not_found}' when none of them exists, which is the ordinary +%% case on a headless machine. -spec open_browser(binary()) -> ok | {error, browser_not_found}. open_browser(Url) when is_binary(Url) -> ok = ensure_valid_http_url(Url), @@ -400,19 +413,6 @@ open_browser(Url) when is_binary(Url) -> ok end. -%%==================================================================== -%% Internal functions -%%==================================================================== - -%% @private -%% Older servers do not send the field at all, which means nothing is lapsed. --spec sso_reauth_required(map()) -> [binary()]. -sso_reauth_required(TokenResponse) -> - case maps:get(<<"sso_reauth_required">>, TokenResponse, []) of - Organizations when is_list(Organizations) -> Organizations; - _Other -> [] - end. - %% @private %% Validates that a URL uses http:// or https:// scheme. -spec ensure_valid_http_url(binary()) -> ok. diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index 7f6009b9..a1529f99 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -78,7 +78,7 @@ %% %% Internally, authentication resolution tracks context via `auth_context()': %% %% @@ -95,7 +95,8 @@ with_repo/3, resolve_api_auth/2, resolve_repo_auth/1, - refresh_tokens/1 + refresh_tokens/1, + is_token_expired/1 ]). -export_type([ @@ -163,7 +164,7 @@ | {auth_error, term()}. -type auth_context() :: #{ - source => env | config | oauth, + source => config | oauth, has_refresh_token => boolean() }. @@ -376,10 +377,11 @@ prompt_and_device_auth(BaseConfig, Fun, Reason, Opts) -> case call_callback(BaseConfig, should_authenticate, [Reason]) of true -> case device_auth(BaseConfig, <<"api repositories">>, Opts) of - {ok, #{access_token := Token}} -> + {ok, #{access_token := Token} = Tokens} -> BearerToken = <<"Bearer ", Token/binary>>, Config = BaseConfig#{api_key => BearerToken}, - AuthContext = #{source => oauth, has_refresh_token => true}, + HasRefreshToken = is_binary(maps:get(refresh_token, Tokens, undefined)), + AuthContext = #{source => oauth, has_refresh_token => HasRefreshToken}, execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); {error, _} = Error -> Error @@ -412,22 +414,25 @@ execute_optional_with_retry(BaseConfig, Fun, Opts) -> %% those up rather than waiting out the access token. -spec refresh_tokens(hex_core:config()) -> ok | {error, auth_error()}. refresh_tokens(Config) -> - global:trans( - {{?MODULE, token_refresh}, self()}, - fun() -> - case call_callback(Config, get_oauth_tokens, []) of - {ok, Tokens} -> - case maybe_refresh_token_with_context(Config, Tokens) of - {ok, _BearerToken, _AuthContext} -> ok; - {error, _Reason} = Error -> Error - end; - error -> - {error, {auth_error, no_credentials}} - end - end, - [node()], - infinity - ). + Refresh = fun(Tokens) -> + %% A failed refresh leaves the stored token in place; the caller warns + %% and continues with it. + case maybe_refresh_token_with_context(Config, Tokens) of + {ok, _BearerToken, _AuthContext} -> ok; + {error, _Reason} = Error -> Error + end + end, + case with_token_refresh_lock(Config, Refresh) of + error -> {error, {auth_error, no_credentials}}; + Result -> Result + end. + +%% @private +%% Check if a token is expired (within 5 minute buffer). +-spec is_token_expired(integer()) -> boolean(). +is_token_expired(ExpiresAt) -> + Now = erlang:system_time(second), + ExpiresAt - Now < ?EXPIRY_BUFFER_SECONDS. %%==================================================================== %% Internal functions - Device Auth @@ -447,13 +452,17 @@ device_auth(Config, Scope, Opts) -> end, FlowOpts = [{open_browser, OpenBrowser}], case hex_api_oauth:device_auth_flow(Config, ClientId, Scope, PromptUser, FlowOpts) of - {ok, - #{access_token := AccessToken, refresh_token := RefreshToken, expires_at := ExpiresAt} = - Tokens} -> + {ok, #{ + access_token := AccessToken, + refresh_token := RefreshToken, + expires_at := ExpiresAt, + sso_reauth_required := SsoReauthRequired + }} -> ok = call_callback(Config, persist_oauth_tokens, [ global, AccessToken, RefreshToken, ExpiresAt ]), - report_sso_reauth(Config, Tokens), + report_sso_reauth(Config, SsoReauthRequired), + %% sso_reauth_required reaches the build tool through the sso_reauth callback. {ok, #{ access_token => AccessToken, refresh_token => RefreshToken, @@ -471,13 +480,6 @@ device_auth(Config, Scope, Opts) -> {error, {auth_error, Reason}} end. -%% @private -%% Check if a token is expired (within 5 minute buffer). --spec is_token_expired(integer()) -> boolean(). -is_token_expired(ExpiresAt) -> - Now = erlang:system_time(second), - ExpiresAt - Now < ?EXPIRY_BUFFER_SECONDS. - %%==================================================================== %% Internal functions - Auth Resolution %%==================================================================== @@ -635,33 +637,40 @@ get_parent_repo_key(Config, RepoName, KeyType) -> %% @private %% Resolve OAuth token with global lock to prevent concurrent refresh attempts. resolve_oauth_token_with_context(Config) -> + Resolve = fun(#{access_token := AccessToken, expires_at := ExpiresAt} = Tokens) -> + HasRefreshToken = + maps:is_key(refresh_token, Tokens) andalso + is_binary(maps:get(refresh_token, Tokens)), + case is_token_expired(ExpiresAt) of + true -> + refresh_or_clear(Config, Tokens); + false -> + BearerToken = <<"Bearer ", AccessToken/binary>>, + {ok, BearerToken, #{source => oauth, has_refresh_token => HasRefreshToken}} + end + end, + case with_token_refresh_lock(Config, Resolve) of + error -> {error, no_auth}; + Result -> Result + end. + +%% @private +%% Fetch the stored global tokens and hand them to Fun under the token-refresh +%% lock, so concurrent callers do not each refresh the same token. Returns +%% `error' without calling Fun when no tokens are stored. +with_token_refresh_lock(Config, Fun) -> global:trans( {{?MODULE, token_refresh}, self()}, fun() -> - do_resolve_oauth_token_with_context(Config) + case call_callback(Config, get_oauth_tokens, []) of + {ok, Tokens} -> Fun(Tokens); + error -> error + end end, [node()], infinity ). -%% @private -do_resolve_oauth_token_with_context(Config) -> - case call_callback(Config, get_oauth_tokens, []) of - {ok, #{access_token := AccessToken, expires_at := ExpiresAt} = Tokens} -> - HasRefreshToken = - maps:is_key(refresh_token, Tokens) andalso - is_binary(maps:get(refresh_token, Tokens)), - case is_token_expired(ExpiresAt) of - true -> - refresh_or_clear(Config, Tokens); - false -> - BearerToken = <<"Bearer ", AccessToken/binary>>, - {ok, BearerToken, #{source => oauth, has_refresh_token => HasRefreshToken}} - end; - error -> - {error, no_auth} - end. - %% @private %% Refresh an expired global token; if the refresh fails, invalidate the stored %% token via the optional clear_oauth_tokens callback. This runs inside the @@ -693,7 +702,7 @@ maybe_refresh_token_with_context(Config, #{refresh_token := RefreshToken}) when ok = call_callback(Config, persist_oauth_tokens, [ global, NewAccessToken, NewRefreshToken, ExpiresAt ]), - report_sso_reauth(Config, TokenResponse), + report_sso_reauth(Config, hex_api_oauth:sso_reauth_required(TokenResponse)), BearerToken = <<"Bearer ", NewAccessToken/binary>>, HasRefreshToken = is_binary(NewRefreshToken), {ok, BearerToken, #{source => oauth, has_refresh_token => HasRefreshToken}}; @@ -830,14 +839,8 @@ call_callback(Config, Name, Args) -> %% Hands the build tool the organizations this session has to authenticate for. %% Always called after a grant, including with the empty list, so a set that %% has been resolved does not linger. -report_sso_reauth(Config, #{sso_reauth_required := Organizations}) when is_list(Organizations) -> - maybe_call_callback(Config, sso_reauth, [Organizations]); -report_sso_reauth(Config, #{<<"sso_reauth_required">> := Organizations}) when - is_list(Organizations) --> - maybe_call_callback(Config, sso_reauth, [Organizations]); -report_sso_reauth(Config, _Tokens) -> - maybe_call_callback(Config, sso_reauth, [[]]). +report_sso_reauth(Config, Organizations) when is_list(Organizations) -> + maybe_call_callback(Config, sso_reauth, [Organizations]). %% @private %% Like call_callback/3 but for optional callbacks: returns ok without doing From c7cbc92f02e69cc8e574b5912ea1f86bd6bde755 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sat, 22 Aug 2026 16:27:13 +0200 Subject: [PATCH 3/5] Close what the second CLI review found A repository request that triggered inline device auth retried with the API key set, and repository requests read repo_key, so the retry went out unauthenticated and 401ed again. It re-resolves repository auth after authenticating. with_repo also executed once and never acted on a 401 from a token it had already resolved, so a token the server rejects as expired was never refreshed or re-exchanged; it retries once with renewed credentials. A transport failure during a refresh was reported as an expired session and dropped the stored token, so a DNS blip printed that the session had expired and downgraded the rest of the run to anonymous. Only a refusal from the server clears the token now. with_repo documented auth_inline as defaulting to false and then took true on the 401 path, so a private package could open an interactive device flow against the caller's stated default. One transport error while polling ended a device authorization the user was minutes into. Polling continues until the device code expires. A malformed verification URI crashed the CLI, because the URL check threw where its caller expected a return value. The refresh token sentinel had two owners: an absent one was undefined in one place and absent in another, and undefined reached persistence, where Elixir stored it as a refresh token. The grant is normalized once. has_refresh_token was derived three ways in one module, and the source member of the auth context was written on eight paths and read nowhere. --- src/hex_api_oauth.erl | 62 +++++--- src/hex_cli_auth.erl | 271 +++++++++++++++++++++------------ test/hex_api_SUITE.erl | 89 +++++++++++ test/hex_cli_auth_SUITE.erl | 207 ++++++++++++++++++++++++- test/support/hex_http_test.erl | 36 +++-- 5 files changed, 528 insertions(+), 137 deletions(-) diff --git a/src/hex_api_oauth.erl b/src/hex_api_oauth.erl index 363c983a..113e2939 100644 --- a/src/hex_api_oauth.erl +++ b/src/hex_api_oauth.erl @@ -19,7 +19,7 @@ -type oauth_tokens() :: #{ access_token := binary(), - refresh_token => binary() | undefined, + refresh_token => binary(), expires_at := integer(), %% Organizations the session must authenticate against their identity %% provider for. Their scopes are not in this token and re-requesting them @@ -182,14 +182,12 @@ poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) -> <<"access_token">> := AccessToken, <<"expires_in">> := ExpiresIn } = TokenResponse, - RefreshToken = maps:get(<<"refresh_token">>, TokenResponse, undefined), - TokenExpiresAt = erlang:system_time(second) + ExpiresIn, - {ok, #{ + Tokens = #{ access_token => AccessToken, - refresh_token => RefreshToken, - expires_at => TokenExpiresAt, + expires_at => erlang:system_time(second) + ExpiresIn, sso_reauth_required => sso_reauth_required(TokenResponse) - }}; + }, + {ok, put_refresh_token(Tokens, TokenResponse)}; {ok, {400, _, #{<<"error">> := <<"authorization_pending">>}}} -> poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt); {ok, {400, _, #{<<"error">> := <<"slow_down">>}}} -> @@ -203,8 +201,12 @@ poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) -> {error, {access_denied, Status, Body}}; {ok, {Status, _, Body}} -> {error, {poll_failed, Status, Body}}; - {error, Reason} -> - {error, Reason} + {error, _Reason} -> + %% A request that did not get through says nothing about the + %% authorization, which the user may be minutes into. The + %% device code lives on the server until it expires, so keep + %% polling until then. + poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) end end. @@ -391,19 +393,27 @@ sso_reauth_required(TokenResponse) -> %% Opens a URL in the default browser using the platform's opener: `open' on %% macOS, `xdg-open' on Linux, `start' on Windows. Returns %% `{error, browser_not_found}' when none of them exists, which is the ordinary -%% case on a headless machine. --spec open_browser(binary()) -> ok | {error, browser_not_found}. +%% case on a headless machine, and `{error, invalid_url}' for anything that is +%% not an http(s) URL. +-spec open_browser(binary()) -> ok | {error, browser_not_found | invalid_url}. open_browser(Url) when is_binary(Url) -> - ok = ensure_valid_http_url(Url), - UrlStr = binary_to_list(Url), + case valid_http_url(Url) of + true -> + spawn_browser(binary_to_list(Url)); + false -> + {error, invalid_url} + end. + +%% @private +spawn_browser(Url) -> {Cmd, Args} = case os:type() of {unix, darwin} -> - {"open", [UrlStr]}; + {"open", [Url]}; {unix, _} -> - {"xdg-open", [UrlStr]}; + {"xdg-open", [Url]}; {win32, _} -> - {"cmd", ["/c", "start", "", UrlStr]} + {"cmd", ["/c", "start", "", Url]} end, case os:find_executable(Cmd) of false -> @@ -414,15 +424,23 @@ open_browser(Url) when is_binary(Url) -> end. %% @private -%% Validates that a URL uses http:// or https:// scheme. --spec ensure_valid_http_url(binary()) -> ok. -ensure_valid_http_url(Url) when is_binary(Url) -> +%% Whether a URL uses the http:// or https:// scheme. +-spec valid_http_url(binary()) -> boolean(). +valid_http_url(Url) when is_binary(Url) -> case uri_string:parse(Url) of - #{scheme := <<"https">>} -> ok; - #{scheme := <<"http">>} -> ok; - _ -> throw({invalid_url, Url}) + #{scheme := <<"https">>} -> true; + #{scheme := <<"http">>} -> true; + _ -> false end. +%% @private +%% A response without a refresh token carries no key at all rather than a +%% placeholder, so what a build tool stores is only ever a real token. +put_refresh_token(Tokens, #{<<"refresh_token">> := RefreshToken}) when is_binary(RefreshToken) -> + Tokens#{refresh_token => RefreshToken}; +put_refresh_token(Tokens, _TokenResponse) -> + Tokens. + %% @private %% Get the hostname of the current machine. -spec get_hostname() -> binary(). diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index a1529f99..ca948463 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -78,7 +78,6 @@ %% %% Internally, authentication resolution tracks context via `auth_context()': %%
    -%%
  • `source' - Where the credentials came from (`config' or `oauth')
  • %%
  • `has_refresh_token' - Whether token refresh is possible on 401
  • %%
%% @@ -115,6 +114,12 @@ %% Maximum OTP retry attempts -define(MAX_OTP_RETRIES, 3). +%% Both ways a refresh can leave us without a usable token: the server rejected +%% the refresh token, or the request never got an answer. +-define(IS_REFRESH_FAILURE(Reason), + (Reason =:= token_refresh_failed orelse Reason =:= token_refresh_unavailable) +). + -type permission() :: read | write. -type callbacks() :: #{ @@ -157,14 +162,16 @@ | {auth_error, auth_declined} | {auth_error, otp_cancelled} | {auth_error, otp_max_retries} + %% The server refused to refresh the token | {auth_error, token_refresh_failed} + %% The refresh request got no answer: DNS, connect, timeout, TLS + | {auth_error, token_refresh_unavailable} | {auth_error, device_auth_timeout} | {auth_error, device_auth_denied} | {auth_error, oauth_exchange_failed} | {auth_error, term()}. -type auth_context() :: #{ - source => config | oauth, has_refresh_token => boolean() }. @@ -245,16 +252,16 @@ with_api(Permission, BaseConfig, Fun, Opts) -> execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); {error, no_auth} when Optional =:= true -> %% Auth is optional, try without credentials first - execute_optional_with_retry(BaseConfig, Fun, Opts); + execute_optional_with_retry(api, BaseConfig, Fun, AuthInline, Opts); {error, no_auth} when AuthInline =:= true -> %% No auth found, ask user if they want to authenticate - maybe_authenticate_and_retry(BaseConfig, Fun, no_credentials, Opts); + maybe_authenticate_and_retry(api, BaseConfig, Fun, no_credentials, Opts); {error, no_auth} -> %% auth_inline is false, just return error {error, {auth_error, no_credentials}}; - {error, {auth_error, token_refresh_failed}} when Optional =:= true -> + {error, {auth_error, Reason}} when Optional =:= true, ?IS_REFRESH_FAILURE(Reason) -> %% Token refresh failed but auth is optional, fall back to no credentials - execute_optional_with_retry(BaseConfig, Fun, Opts); + execute_optional_with_retry(api, BaseConfig, Fun, AuthInline, Opts); {error, _} = Error -> Error end. @@ -286,6 +293,10 @@ with_repo(BaseConfig, Fun) -> %%
  • Prompt via `should_authenticate' when `auth_inline' is true
  • %% %% +%% A resolved token the server answers with a `token_expired' 401 is renewed at +%% its source (a per-repo token is exchanged again, the global token is +%% refreshed) and the request is run once more. +%% %% The repository name is taken from the config (`repo_name' or `repo_organization'). %% %% Callbacks are taken from the `cli_auth_callbacks' key in the config map. @@ -313,20 +324,19 @@ with_repo(BaseConfig, Fun, Opts) -> AuthInline = proplists:get_value(auth_inline, Opts, false), case resolve_repo_auth(BaseConfig) of {ok, RepoKey, _AuthContext} when is_binary(RepoKey) -> - Config = BaseConfig#{repo_key => RepoKey}, - Fun(Config); + execute_repo_with_retry(BaseConfig, Fun, RepoKey); no_auth when Optional =:= true -> %% Auth is optional, try without credentials first - execute_optional_with_retry(BaseConfig, Fun, Opts); + execute_optional_with_retry(repo, BaseConfig, Fun, AuthInline, Opts); no_auth when AuthInline =:= true -> %% No auth found, ask user if they want to authenticate - maybe_authenticate_and_retry(BaseConfig, Fun, no_credentials, Opts); + maybe_authenticate_and_retry(repo, BaseConfig, Fun, no_credentials, Opts); no_auth -> %% auth_inline is false, return error {error, {auth_error, no_credentials}}; - {error, {auth_error, token_refresh_failed}} when Optional =:= true -> + {error, {auth_error, Reason}} when Optional =:= true, ?IS_REFRESH_FAILURE(Reason) -> %% Token refresh failed but auth is optional, fall back to no credentials - execute_optional_with_retry(BaseConfig, Fun, Opts); + execute_optional_with_retry(repo, BaseConfig, Fun, AuthInline, Opts); {error, _} = Error -> Error end. @@ -343,15 +353,18 @@ repo_name(_) -> %% @private %% Ask user if they want to authenticate, and if yes, initiate device auth. %% +%% Kind says which credential the retried request needs: `api' takes the token +%% as api_key, `repo' resolves repository auth and takes it as repo_key. +%% %% Serialized with a global lock so concurrent callers don't each trigger their %% own device auth flow. The first caller to acquire the lock runs device auth %% and persists the resulting token; subsequent callers re-check for an existing %% (now-valid) token inside the lock and reuse it instead of re-authenticating. -maybe_authenticate_and_retry(BaseConfig, Fun, Reason, Opts) -> +maybe_authenticate_and_retry(Kind, BaseConfig, Fun, Reason, Opts) -> global:trans( {{?MODULE, device_auth}, self()}, fun() -> - do_maybe_authenticate_and_retry(BaseConfig, Fun, Reason, Opts) + do_maybe_authenticate_and_retry(Kind, BaseConfig, Fun, Reason, Opts) end, [node()], infinity @@ -362,27 +375,31 @@ maybe_authenticate_and_retry(BaseConfig, Fun, Reason, Opts) -> %% and, if we get a token that differs from the one we arrived with (none when %% credentials were missing; the rejected one on token_refresh_failed), reuse it %% instead of prompting again. Otherwise proceed to prompt + device auth. -do_maybe_authenticate_and_retry(BaseConfig, Fun, Reason, Opts) -> +do_maybe_authenticate_and_retry(api, BaseConfig, Fun, Reason, Opts) -> CurrentApiKey = maps:get(api_key, BaseConfig, undefined), case resolve_api_auth(write, BaseConfig) of {ok, ApiKey, AuthContext} when ApiKey =/= CurrentApiKey -> Config = BaseConfig#{api_key => ApiKey}, execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); _ -> - prompt_and_device_auth(BaseConfig, Fun, Reason, Opts) + prompt_and_device_auth(api, BaseConfig, Fun, Reason, Opts) + end; +do_maybe_authenticate_and_retry(repo, BaseConfig, Fun, Reason, Opts) -> + CurrentRepoKey = maps:get(repo_key, BaseConfig, undefined), + case resolve_repo_auth(BaseConfig) of + {ok, RepoKey, _AuthContext} when is_binary(RepoKey), RepoKey =/= CurrentRepoKey -> + execute_repo_with_retry(BaseConfig, Fun, RepoKey); + _ -> + prompt_and_device_auth(repo, BaseConfig, Fun, Reason, Opts) end. %% @private -prompt_and_device_auth(BaseConfig, Fun, Reason, Opts) -> +prompt_and_device_auth(Kind, BaseConfig, Fun, Reason, Opts) -> case call_callback(BaseConfig, should_authenticate, [Reason]) of true -> case device_auth(BaseConfig, <<"api repositories">>, Opts) of - {ok, #{access_token := Token} = Tokens} -> - BearerToken = <<"Bearer ", Token/binary>>, - Config = BaseConfig#{api_key => BearerToken}, - HasRefreshToken = is_binary(maps:get(refresh_token, Tokens, undefined)), - AuthContext = #{source => oauth, has_refresh_token => HasRefreshToken}, - execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); + {ok, Tokens} -> + retry_authenticated(Kind, BaseConfig, Fun, Tokens, Opts); {error, _} = Error -> Error end; @@ -390,14 +407,32 @@ prompt_and_device_auth(BaseConfig, Fun, Reason, Opts) -> {error, {auth_error, auth_declined}} end. +%% @private +%% Run the request with the credentials device auth just produced. The token is +%% the user's API token; what a repository request needs is repository auth, +%% which the token may only be one input to, so resolve that instead of reusing +%% the API-shaped one. +retry_authenticated(api, BaseConfig, Fun, #{access_token := AccessToken} = Tokens, Opts) -> + Config = BaseConfig#{api_key => <<"Bearer ", AccessToken/binary>>}, + AuthContext = #{has_refresh_token => has_refresh_token(Tokens)}, + execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); +retry_authenticated(repo, BaseConfig, Fun, _Tokens, _Opts) -> + case resolve_repo_auth(BaseConfig) of + {ok, RepoKey, _AuthContext} when is_binary(RepoKey) -> + execute_repo_with_retry(BaseConfig, Fun, RepoKey); + no_auth -> + {error, {auth_error, no_credentials}}; + {error, _} = Error -> + Error + end. + %% @private %% Execute function without auth, but retry with auth if we get a 401. -execute_optional_with_retry(BaseConfig, Fun, Opts) -> - AuthInline = proplists:get_value(auth_inline, Opts, true), +execute_optional_with_retry(Kind, BaseConfig, Fun, AuthInline, Opts) -> case Fun(BaseConfig) of {ok, {401, _Headers, _Body}} when AuthInline =:= true -> %% Got 401, need auth - ask user if they want to authenticate - maybe_authenticate_and_retry(BaseConfig, Fun, no_credentials, Opts); + maybe_authenticate_and_retry(Kind, BaseConfig, Fun, no_credentials, Opts); {ok, {401, _Headers, _Body}} -> %% Got 401 but auth_inline is false, return error {error, {auth_error, no_credentials}}; @@ -405,6 +440,32 @@ execute_optional_with_retry(BaseConfig, Fun, Opts) -> Other end. +%% @private +%% Run a repository request with a resolved token. A 401 that says the token +%% expired is answered by renewing the credential at its source and running the +%% request once more; a second 401 is the caller's to handle. +execute_repo_with_retry(BaseConfig, Fun, RepoKey) -> + case Fun(BaseConfig#{repo_key => RepoKey}) of + {ok, {401, Headers, _Body}} = Response -> + case detect_auth_error(Headers) of + token_expired -> + renew_repo_auth_and_retry(BaseConfig, Fun, RepoKey, Response); + _Other -> + Response + end; + Other -> + Other + end. + +%% @private +renew_repo_auth_and_retry(BaseConfig, Fun, RepoKey, Response) -> + case resolve_repo_auth(BaseConfig, true) of + {ok, NewRepoKey, _AuthContext} when is_binary(NewRepoKey), NewRepoKey =/= RepoKey -> + Fun(BaseConfig#{repo_key => NewRepoKey}); + _Other -> + Response + end. + %% @doc %% Refreshes the stored global OAuth token now, whether or not it has expired. %% @@ -452,22 +513,13 @@ device_auth(Config, Scope, Opts) -> end, FlowOpts = [{open_browser, OpenBrowser}], case hex_api_oauth:device_auth_flow(Config, ClientId, Scope, PromptUser, FlowOpts) of - {ok, #{ - access_token := AccessToken, - refresh_token := RefreshToken, - expires_at := ExpiresAt, - sso_reauth_required := SsoReauthRequired - }} -> - ok = call_callback(Config, persist_oauth_tokens, [ - global, AccessToken, RefreshToken, ExpiresAt - ]), + {ok, #{sso_reauth_required := SsoReauthRequired} = Response} -> + %% sso_reauth_required reaches the build tool through the sso_reauth + %% callback rather than with the tokens. + Tokens = maps:without([sso_reauth_required], Response), + ok = persist_tokens(Config, global, Tokens), report_sso_reauth(Config, SsoReauthRequired), - %% sso_reauth_required reaches the build tool through the sso_reauth callback. - {ok, #{ - access_token => AccessToken, - refresh_token => RefreshToken, - expires_at => ExpiresAt - }}; + {ok, Tokens}; {error, timeout} -> {error, {auth_error, device_auth_timeout}}; {error, {access_denied, _Status, _Body}} -> @@ -489,21 +541,21 @@ device_auth(Config, Scope, Opts) -> {ok, binary(), auth_context()} | {error, no_auth} | {error, auth_error()}. resolve_api_auth(_Permission, #{api_key := ApiKey}) when is_binary(ApiKey) -> %% api_key already in config, pass through directly - {ok, ApiKey, #{source => config, has_refresh_token => false}}; + {ok, ApiKey, #{has_refresh_token => false}}; resolve_api_auth(_Permission, Config) -> RepoName = repo_name(Config), %% 1. Check per-repo api_key case call_callback(Config, get_auth_config, [RepoName]) of #{api_key := ApiKey} when is_binary(ApiKey) -> - {ok, ApiKey, #{source => config, has_refresh_token => false}}; + {ok, ApiKey, #{has_refresh_token => false}}; _ -> %% 2. Check parent repo (for "hexpm:org" organizations) case get_parent_repo_key(Config, RepoName, api_key) of {ok, ApiKey} -> - {ok, ApiKey, #{source => config, has_refresh_token => false}}; + {ok, ApiKey, #{has_refresh_token => false}}; error -> %% 3. Try global OAuth token - resolve_oauth_token_with_context(Config) + resolve_oauth_token_with_context(Config, false) end end. @@ -517,46 +569,52 @@ resolve_api_auth(_Permission, Config) -> %% 5. Fallthrough to no_auth (handled by with_repo/3 for optional/auth_inline) -spec resolve_repo_auth(hex_core:config()) -> {ok, binary(), auth_context()} | no_auth | {error, auth_error()}. -resolve_repo_auth(#{repo_key := RepoKey}) when is_binary(RepoKey) -> - %% repo_key already in config, pass through directly - {ok, RepoKey, #{source => config, has_refresh_token => false}}; resolve_repo_auth(Config) -> + resolve_repo_auth(Config, false). + +%% @private +%% Renew says the credential we already have was rejected, so a stored token +%% that has not run out of time is exchanged or refreshed anyway. +resolve_repo_auth(#{repo_key := RepoKey}, _Renew) when is_binary(RepoKey) -> + %% repo_key already in config, pass through directly + {ok, RepoKey, #{has_refresh_token => false}}; +resolve_repo_auth(Config, Renew) -> RepoName = repo_name(Config), global:trans( {{?MODULE, repo, RepoName}, self()}, fun() -> - do_resolve_repo_auth(RepoName, RepoName, Config) + do_resolve_repo_auth(RepoName, RepoName, Config, Renew) end, [node()], infinity ). -do_resolve_repo_auth(RepoName, LookupRepo, Config) -> +do_resolve_repo_auth(RepoName, LookupRepo, Config, Renew) -> Trusted = maps:get(trusted, Config, false), OAuthExchange = maps:get(oauth_exchange, Config, false), case call_callback(Config, get_auth_config, [LookupRepo]) of #{repo_key := RepoKey} when is_binary(RepoKey) -> %% 1. repo_key from get_auth_config => passthrough - {ok, RepoKey, #{source => config, has_refresh_token => false}}; + {ok, RepoKey, #{has_refresh_token => false}}; #{oauth_token := OAuthToken, auth_key := AuthKey} when is_binary(AuthKey) and OAuthExchange, Trusted -> %% 2. trusted + oauth_token + auth_key + oauth_exchange => use/refresh existing token - resolve_repo_oauth_token(RepoName, Config, AuthKey, OAuthToken); + resolve_repo_oauth_token(RepoName, Config, AuthKey, OAuthToken, Renew); #{auth_key := AuthKey} when is_binary(AuthKey) and OAuthExchange, Trusted -> %% 3. trusted + auth_key + oauth_exchange => exchange for new OAuth token exchange_for_oauth_token(RepoName, Config, AuthKey, <<"repositories">>); #{auth_key := AuthKey} when is_binary(AuthKey), Trusted -> %% 4. trusted + auth_key => use directly - {ok, AuthKey, #{source => config, has_refresh_token => false}}; + {ok, AuthKey, #{has_refresh_token => false}}; _ when Trusted -> %% 5. Check parent repo (for "hexpm:org" organizations) case binary:split(LookupRepo, <<":">>) of [ParentName, _OrgName] -> - do_resolve_repo_auth(RepoName, ParentName, Config); + do_resolve_repo_auth(RepoName, ParentName, Config, Renew); _ -> %% 6. trusted Hex.pm or child repository + global OAuth tokens => use those - resolve_global_oauth_for_repo(RepoName, Config) + resolve_global_oauth_for_repo(RepoName, Config, Renew) end; _ -> %% 7. Not trusted, no auth @@ -564,15 +622,15 @@ do_resolve_repo_auth(RepoName, LookupRepo, Config) -> end. %% @private -resolve_global_oauth_for_repo(<<"hexpm">>, Config) -> - resolve_global_oauth_for_repo(Config); -resolve_global_oauth_for_repo(<<"hexpm:", _/binary>>, Config) -> - resolve_global_oauth_for_repo(Config); -resolve_global_oauth_for_repo(_RepoName, _Config) -> +resolve_global_oauth_for_repo(<<"hexpm">>, Config, Renew) -> + resolve_global_oauth_for_repo(Config, Renew); +resolve_global_oauth_for_repo(<<"hexpm:", _/binary>>, Config, Renew) -> + resolve_global_oauth_for_repo(Config, Renew); +resolve_global_oauth_for_repo(_RepoName, _Config, _Renew) -> no_auth. -resolve_global_oauth_for_repo(Config) -> - case resolve_oauth_token_with_context(Config) of +resolve_global_oauth_for_repo(Config, Renew) -> + case resolve_oauth_token_with_context(Config, Renew) of {ok, Token, AuthContext} -> {ok, Token, AuthContext}; {error, no_auth} -> @@ -582,15 +640,19 @@ resolve_global_oauth_for_repo(Config) -> end. %% @private -%% Resolve repo OAuth token: use if valid, re-exchange if expiring. -resolve_repo_oauth_token(RepoName, Config, AuthKey, #{ - access_token := AccessToken, expires_at := ExpiresAt -}) -> - case is_token_expired(ExpiresAt) of +%% Resolve repo OAuth token: use if valid, re-exchange if expiring or rejected. +resolve_repo_oauth_token( + RepoName, + Config, + AuthKey, + #{access_token := AccessToken, expires_at := ExpiresAt}, + Renew +) -> + case Renew orelse is_token_expired(ExpiresAt) of false -> %% Token is still valid, use it BearerToken = <<"Bearer ", AccessToken/binary>>, - {ok, BearerToken, #{source => oauth, has_refresh_token => false}}; + {ok, BearerToken, #{has_refresh_token => false}}; true -> %% Token expired, do a new exchange exchange_for_oauth_token(RepoName, Config, AuthKey, <<"repositories">>) @@ -608,12 +670,13 @@ exchange_for_oauth_token(RepoName, Config, AuthKey, Scope) -> end, case hex_api_oauth:client_credentials_token(ExchangeConfig, ClientId, AuthKey, Scope) of {ok, {200, _, #{<<"access_token">> := AccessToken, <<"expires_in">> := ExpiresIn}}} -> - ExpiresAt = erlang:system_time(second) + ExpiresIn, - ok = call_callback(Config, persist_oauth_tokens, [ - RepoName, AccessToken, undefined, ExpiresAt - ]), + Tokens = #{ + access_token => AccessToken, + expires_at => erlang:system_time(second) + ExpiresIn + }, + ok = persist_tokens(Config, RepoName, Tokens), BearerToken = <<"Bearer ", AccessToken/binary>>, - {ok, BearerToken, #{source => oauth, has_refresh_token => false}}; + {ok, BearerToken, #{has_refresh_token => false}}; {ok, {_Status, _, _Body}} -> {error, {auth_error, oauth_exchange_failed}}; {error, _} -> @@ -636,17 +699,16 @@ get_parent_repo_key(Config, RepoName, KeyType) -> %% @private %% Resolve OAuth token with global lock to prevent concurrent refresh attempts. -resolve_oauth_token_with_context(Config) -> +%% Renew refreshes a token that has not run out of time, for when the server +%% has rejected it anyway. +resolve_oauth_token_with_context(Config, Renew) -> Resolve = fun(#{access_token := AccessToken, expires_at := ExpiresAt} = Tokens) -> - HasRefreshToken = - maps:is_key(refresh_token, Tokens) andalso - is_binary(maps:get(refresh_token, Tokens)), - case is_token_expired(ExpiresAt) of + case Renew orelse is_token_expired(ExpiresAt) of true -> refresh_or_clear(Config, Tokens); false -> BearerToken = <<"Bearer ", AccessToken/binary>>, - {ok, BearerToken, #{source => oauth, has_refresh_token => HasRefreshToken}} + {ok, BearerToken, #{has_refresh_token => has_refresh_token(Tokens)}} end end, case with_token_refresh_lock(Config, Resolve) of @@ -672,17 +734,20 @@ with_token_refresh_lock(Config, Fun) -> ). %% @private -%% Refresh an expired global token; if the refresh fails, invalidate the stored -%% token via the optional clear_oauth_tokens callback. This runs inside the -%% token_refresh lock, so the unusable token is dropped exactly once and the -%% callers serialized behind the lock re-read it as absent instead of each -%% retrying the doomed refresh against the server. +%% Refresh an expired global token; if the server rejected the refresh token, +%% invalidate the stored token via the optional clear_oauth_tokens callback. +%% This runs inside the token_refresh lock, so the unusable token is dropped +%% exactly once and the callers serialized behind the lock re-read it as absent +%% instead of each retrying the doomed refresh against the server. A refresh +%% that never reached the server says nothing about the token, so it is kept. refresh_or_clear(Config, Tokens) -> case maybe_refresh_token_with_context(Config, Tokens) of {ok, _Bearer, _Ctx} = Ok -> Ok; - {error, _} = Error -> + {error, {auth_error, token_refresh_failed}} = Error -> maybe_call_callback(Config, clear_oauth_tokens, []), + Error; + {error, _} = Error -> Error end. @@ -698,22 +763,40 @@ maybe_refresh_token_with_context(Config, #{refresh_token := RefreshToken}) when <<"expires_in">> := ExpiresIn } = TokenResponse, NewRefreshToken = maps:get(<<"refresh_token">>, TokenResponse, RefreshToken), - ExpiresAt = erlang:system_time(second) + ExpiresIn, - ok = call_callback(Config, persist_oauth_tokens, [ - global, NewAccessToken, NewRefreshToken, ExpiresAt - ]), + NewTokens = #{ + access_token => NewAccessToken, + refresh_token => NewRefreshToken, + expires_at => erlang:system_time(second) + ExpiresIn + }, + ok = persist_tokens(Config, global, NewTokens), report_sso_reauth(Config, hex_api_oauth:sso_reauth_required(TokenResponse)), BearerToken = <<"Bearer ", NewAccessToken/binary>>, - HasRefreshToken = is_binary(NewRefreshToken), - {ok, BearerToken, #{source => oauth, has_refresh_token => HasRefreshToken}}; + {ok, BearerToken, #{has_refresh_token => has_refresh_token(NewTokens)}}; {ok, {_Status, _, _Body}} -> {error, {auth_error, token_refresh_failed}}; {error, _Reason} -> - {error, {auth_error, token_refresh_failed}} + {error, {auth_error, token_refresh_unavailable}} end; maybe_refresh_token_with_context(_Config, _Tokens) -> {error, {auth_error, token_refresh_failed}}. +%% @private +%% Whether these tokens can be refreshed. +-spec has_refresh_token(oauth_tokens()) -> boolean(). +has_refresh_token(Tokens) -> + is_binary(maps:get(refresh_token, Tokens, undefined)). + +%% @private +%% The one place tokens are handed to the build tool for storage. A token map +%% without a refresh token is persisted as `undefined', which is what the +%% persist_oauth_tokens callback documents for "there is none". +-spec persist_tokens(hex_core:config(), global | binary(), oauth_tokens()) -> ok. +persist_tokens(Config, Scope, #{access_token := AccessToken, expires_at := ExpiresAt} = Tokens) -> + RefreshToken = maps:get(refresh_token, Tokens, undefined), + ok = call_callback(Config, persist_oauth_tokens, [ + Scope, AccessToken, RefreshToken, ExpiresAt + ]). + %%==================================================================== %% Internal functions - Retry Logic %%==================================================================== @@ -777,7 +860,7 @@ handle_token_refresh_retry(Config, Fun, AuthContext, Opts) -> %% Only attempt refresh if we have a refresh token case maps:get(has_refresh_token, AuthContext, false) of true -> - case resolve_oauth_token_with_context(Config) of + case resolve_oauth_token_with_context(Config, false) of {ok, NewBearerToken, NewAuthContext} -> NewConfig = Config#{api_key => NewBearerToken}, execute_with_retry( @@ -797,7 +880,7 @@ maybe_reauthenticate(Config, Fun, Opts) -> AuthInline = proplists:get_value(auth_inline, Opts, true), case AuthInline of true -> - maybe_authenticate_and_retry(Config, Fun, token_refresh_failed, Opts); + maybe_authenticate_and_retry(api, Config, Fun, token_refresh_failed, Opts); false -> {error, {auth_error, token_refresh_failed}} end. diff --git a/test/hex_api_SUITE.erl b/test/hex_api_SUITE.erl index 60eeaceb..1bf9b508 100644 --- a/test/hex_api_SUITE.erl +++ b/test/hex_api_SUITE.erl @@ -32,6 +32,9 @@ all() -> oauth_device_auth_flow_success_test, oauth_device_auth_flow_denied_test, oauth_device_auth_flow_timeout_test, + oauth_device_auth_flow_poll_error_test, + oauth_device_auth_flow_no_refresh_token_test, + oauth_device_auth_flow_invalid_verification_uri_test, oauth_refresh_token_test, oauth_sso_authorization_test, oauth_device_auth_flow_sso_reauth_test, @@ -226,6 +229,92 @@ oauth_device_auth_flow_timeout_test(_Config) -> {error, timeout} = hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser), ok. +oauth_device_auth_flow_poll_error_test(_Config) -> + % A poll that fails to reach the server keeps polling: the authorization the + % user is part way through outlives one dropped request. + ClientId = <<"cli">>, + Scope = <<"api:write">>, + Self = self(), + PromptUser = fun(_VerificationUri, _UserCode) -> ok end, + + SuccessPayload = #{ + <<"access_token">> => <<"test_access_token">>, + <<"refresh_token">> => <<"test_refresh_token">>, + <<"token_type">> => <<"Bearer">>, + <<"expires_in">> => 3600 + }, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + Self ! {hex_http_test, oauth_device_response, {error, timeout}}, + Self ! + {hex_http_test, oauth_device_response, + {ok, {200, Headers, term_to_binary(SuccessPayload)}}}, + + {ok, Tokens} = hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser), + + ?assertEqual(<<"test_access_token">>, maps:get(access_token, Tokens)), + ok. + +oauth_device_auth_flow_no_refresh_token_test(_Config) -> + % A grant without a refresh token carries no key, rather than a placeholder + % a build tool would go on to store as if it were a token. + ClientId = <<"cli">>, + Scope = <<"api:write">>, + Self = self(), + PromptUser = fun(_VerificationUri, _UserCode) -> ok end, + + SuccessPayload = #{ + <<"access_token">> => <<"test_access_token">>, + <<"token_type">> => <<"Bearer">>, + <<"expires_in">> => 3600 + }, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + Self ! + {hex_http_test, oauth_device_response, + {ok, {200, Headers, term_to_binary(SuccessPayload)}}}, + + {ok, Tokens} = hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser), + + ?assertNot(maps:is_key(refresh_token, Tokens)), + ok. + +oauth_device_auth_flow_invalid_verification_uri_test(_Config) -> + % A verification URI that is not http(s) is not opened, and not a reason to + % end the flow either. + ClientId = <<"cli">>, + Scope = <<"api:write">>, + Self = self(), + PromptUser = fun(_VerificationUri, _UserCode) -> ok end, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + + DevicePayload = #{ + <<"device_code">> => <<"device_code">>, + <<"user_code">> => <<"1234-5678">>, + <<"verification_uri">> => <<"javascript:alert(1)">>, + <<"verification_uri_complete">> => <<"javascript:alert(1)">>, + <<"expires_in">> => 600, + <<"interval">> => 0 + }, + Self ! + {hex_http_test, oauth_device_authorization_response, + {ok, {200, Headers, term_to_binary(DevicePayload)}}}, + + SuccessPayload = #{ + <<"access_token">> => <<"test_access_token">>, + <<"refresh_token">> => <<"test_refresh_token">>, + <<"token_type">> => <<"Bearer">>, + <<"expires_in">> => 3600 + }, + Self ! + {hex_http_test, oauth_device_response, + {ok, {200, Headers, term_to_binary(SuccessPayload)}}}, + + {ok, Tokens} = hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser, [ + {open_browser, true} + ]), + + ?assertEqual(<<"test_access_token">>, maps:get(access_token, Tokens)), + ok. + oauth_refresh_token_test(_Config) -> % Test token refresh ClientId = <<"cli">>, diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index a979fb80..0f0eab65 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -76,6 +76,13 @@ all() -> with_repo_optional_test, with_repo_trusted_with_auth_test, with_repo_optional_token_refresh_failed_test, + with_repo_optional_401_does_not_prompt_test, + with_repo_device_auth_sets_repo_key_test, + with_repo_token_expired_refresh_test, + with_repo_token_expired_exchange_test, + + %% token refresh failure modes + refresh_transport_error_keeps_token_test, %% concurrency tests resolve_oauth_token_concurrent_refresh_serialized_test, @@ -94,7 +101,7 @@ resolve_api_auth_config_passthrough_test(_Config) -> {ok, ApiKey, AuthContext} = hex_cli_auth:resolve_api_auth(read, ConfigWithKey), ?assertEqual(<<"config_api_key">>, ApiKey), - ?assertEqual(#{source => config, has_refresh_token => false}, AuthContext), + ?assertEqual(#{has_refresh_token => false}, AuthContext), ok. resolve_api_auth_per_repo_test(_Config) -> @@ -105,7 +112,7 @@ resolve_api_auth_per_repo_test(_Config) -> {ok, ApiKey, AuthContext} = hex_cli_auth:resolve_api_auth(write, Config), ?assertEqual(<<"repo_api_key">>, ApiKey), - ?assertEqual(#{source => config, has_refresh_token => false}, AuthContext), + ?assertEqual(#{has_refresh_token => false}, AuthContext), ok. resolve_api_auth_parent_repo_test(_Config) -> @@ -134,7 +141,7 @@ resolve_api_auth_oauth_test(_Config) -> {ok, ApiKey, AuthContext} = hex_cli_auth:resolve_api_auth(read, Config), ?assertEqual(<<"Bearer oauth_token">>, ApiKey), - ?assertEqual(#{source => oauth, has_refresh_token => true}, AuthContext), + ?assertEqual(#{has_refresh_token => true}, AuthContext), ok. resolve_api_auth_oauth_expired_refresh_test(_Config) -> @@ -158,7 +165,7 @@ resolve_api_auth_oauth_expired_refresh_test(_Config) -> {ok, ApiKey, AuthContext} = hex_cli_auth:resolve_api_auth(read, Config), %% Should have refreshed and got a new token ?assertMatch(<<"Bearer ", _/binary>>, ApiKey), - ?assertEqual(#{source => oauth, has_refresh_token => true}, AuthContext), + ?assertEqual(#{has_refresh_token => true}, AuthContext), %% Verify token was persisted receive @@ -181,7 +188,7 @@ resolve_api_auth_oauth_no_refresh_token_test(_Config) -> {ok, ApiKey, AuthContext} = hex_cli_auth:resolve_api_auth(read, Config), ?assertEqual(<<"Bearer oauth_token">>, ApiKey), - ?assertEqual(#{source => oauth, has_refresh_token => false}, AuthContext), + ?assertEqual(#{has_refresh_token => false}, AuthContext), ok. resolve_api_auth_no_auth_test(_Config) -> @@ -206,7 +213,7 @@ resolve_repo_auth_config_passthrough_test(_Config) -> {ok, RepoKey, AuthContext} = hex_cli_auth:resolve_repo_auth(ConfigWithKey), ?assertEqual(<<"config_repo_key">>, RepoKey), - ?assertEqual(#{source => config, has_refresh_token => false}, AuthContext), + ?assertEqual(#{has_refresh_token => false}, AuthContext), ok. resolve_repo_auth_callback_repo_key_test(_Config) -> @@ -313,7 +320,7 @@ resolve_repo_auth_oauth_exchange_new_token_test(_Config) -> Config#{trusted => true, oauth_exchange => true} ), ?assertMatch(<<"Bearer ", _/binary>>, RepoKey), - ?assertEqual(#{source => oauth, has_refresh_token => false}, AuthContext), + ?assertEqual(#{has_refresh_token => false}, AuthContext), %% Verify token was persisted with repo name receive @@ -895,6 +902,172 @@ with_repo_optional_token_refresh_failed_test(_Config) -> ?assertEqual(undefined, Result), ok. +with_repo_optional_401_does_not_prompt_test(_Config) -> + %% with_repo defaults auth_inline to false, so a 401 on a private package + %% returns instead of opening a device auth flow the caller did not ask for. + Self = self(), + Config = config_with_callbacks(#{ + should_authenticate => fun(_Reason) -> + Self ! prompted, + false + end + }), + + Result = hex_cli_auth:with_repo( + Config#{trusted => true}, + fun(_Cfg) -> {ok, {401, #{}, <<"">>}} end + ), + ?assertEqual({error, {auth_error, no_credentials}}, Result), + + receive + prompted -> error(prompted_without_auth_inline) + after 0 -> ok + end, + ok. + +with_repo_device_auth_sets_repo_key_test(_Config) -> + %% Authenticating inline from a repository request must retry with + %% repository auth: hex_repo only reads repo_key, so an api_key-shaped retry + %% goes out with no authorization header at all. + TokenStore = ets:new(token_store, [public, set]), + true = ets:insert(TokenStore, {oauth_tokens, error}), + + Config = config_with_callbacks(#{ + get_oauth_tokens => fun() -> + [{oauth_tokens, Tokens}] = ets:lookup(TokenStore, oauth_tokens), + Tokens + end, + should_authenticate => fun(no_credentials) -> true end, + persist_oauth_tokens => fun(global, Access, Refresh, Expires) -> + ets:insert( + TokenStore, + {oauth_tokens, + {ok, #{ + access_token => Access, + refresh_token => Refresh, + expires_at => Expires + }}} + ), + ok + end + }), + + queue_device_response(<<"device_token">>), + + Fun = fun(Cfg) -> + case maps:get(repo_key, Cfg, undefined) of + undefined -> {ok, {401, #{}, <<"">>}}; + RepoKey -> RepoKey + end + end, + + Result = hex_cli_auth:with_repo( + Config#{trusted => true}, + Fun, + [{auth_inline, true}, {oauth_open_browser, false}] + ), + ?assertEqual(<<"Bearer device_token">>, Result), + + ets:delete(TokenStore), + ok. + +with_repo_token_expired_refresh_test(_Config) -> + %% A repository token the server rejects as expired is refreshed and the + %% request runs again, rather than the 401 reaching the caller. + Now = erlang:system_time(second), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"stale_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }} + }), + + queue_refresh_response(#{<<"access_token">> => <<"renewed_token">>}), + + Fun = fun(Cfg) -> + case maps:get(repo_key, Cfg) of + <<"Bearer stale_token">> -> token_expired_response(); + RepoKey -> RepoKey + end + end, + + Result = hex_cli_auth:with_repo(Config#{trusted => true}, Fun), + ?assertEqual(<<"Bearer renewed_token">>, Result), + ok. + +with_repo_token_expired_exchange_test(_Config) -> + %% Same for a per-repo token: it is exchanged again from the auth_key it + %% came from, even though its stored expiry has not passed. + Now = erlang:system_time(second), + Self = self(), + Config = config_with_callbacks(#{ + auth_config => #{ + <<"hexpm">> => #{ + auth_key => <<"repo_auth_key">>, + oauth_token => #{ + access_token => <<"stale_repo_token">>, + expires_at => Now + 3600 + } + } + }, + persist_oauth_tokens => fun(Scope, Access, Refresh, Expires) -> + Self ! {persisted, Scope, Access, Refresh, Expires}, + ok + end + }), + + Fun = fun(Cfg) -> + case maps:get(repo_key, Cfg) of + <<"Bearer stale_repo_token">> -> token_expired_response(); + RepoKey -> RepoKey + end + end, + + Result = hex_cli_auth:with_repo(Config#{trusted => true, oauth_exchange => true}, Fun), + ?assertMatch(<<"Bearer ", _/binary>>, Result), + ?assertNotEqual(<<"Bearer stale_repo_token">>, Result), + + receive + {persisted, <<"hexpm">>, _Access, RefreshToken, _Expires} -> + ?assertEqual(undefined, RefreshToken) + after 100 -> + error(token_not_exchanged) + end, + ok. + +refresh_transport_error_keeps_token_test(_Config) -> + %% A refresh that never reached the server says nothing about the stored + %% token, so it is kept and the caller is told the difference. + Now = erlang:system_time(second), + Self = self(), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"expired_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now - 100 + }}, + clear_oauth_tokens => fun() -> + Self ! cleared, + ok + end + }), + + Self ! {hex_http_test, oauth_refresh_response, {error, timeout}}, + + ?assertEqual( + {error, {auth_error, token_refresh_unavailable}}, + hex_cli_auth:resolve_api_auth(read, Config) + ), + + receive + cleared -> error(token_cleared_on_transport_error) + after 0 -> ok + end, + ok. + %%==================================================================== %% Test Cases - Concurrency %%==================================================================== @@ -1320,6 +1493,26 @@ queue_refresh_response(Overrides) -> {hex_http_test, oauth_refresh_response, {ok, {200, Headers, term_to_binary(Payload)}}}, ok. +%% @private +%% Plants the token the next device auth poll hands back. +queue_device_response(AccessToken) -> + Payload = #{ + <<"access_token">> => AccessToken, + <<"refresh_token">> => <<"device_refresh">>, + <<"token_type">> => <<"Bearer">>, + <<"expires_in">> => 3600 + }, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + self() ! + {hex_http_test, oauth_device_response, {ok, {200, Headers, term_to_binary(Payload)}}}, + ok. + +%% @private +%% The 401 hexpm answers a request whose token it considers expired. +token_expired_response() -> + Headers = #{<<"www-authenticate">> => <<"Bearer realm=\"hex\", error=\"token_expired\"">>}, + {ok, {401, Headers, <<"">>}}. + config_with_callbacks(Opts) -> ?CONFIG#{cli_auth_callbacks => make_callbacks(Opts)}. diff --git a/test/support/hex_http_test.erl b/test/support/hex_http_test.erl index 089f79db..990ee939 100644 --- a/test/support/hex_http_test.erl +++ b/test/support/hex_http_test.erl @@ -33,6 +33,22 @@ api_headers() -> <<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">> }. +device_authorization_fixture() -> + DeviceCode = base64:encode(crypto:strong_rand_bytes(32)), + UserCode = iolist_to_binary([ + integer_to_binary(rand:uniform(9999)), "-", + integer_to_binary(rand:uniform(9999)) + ]), + Payload = #{ + <<"device_code">> => DeviceCode, + <<"user_code">> => UserCode, + <<"verification_uri">> => <<"https://hex.pm/oauth/device">>, + <<"verification_uri_complete">> => <<"https://hex.pm/oauth/device?user_code=", UserCode/binary>>, + <<"expires_in">> => 600, + <<"interval">> => 0 + }, + {ok, {200, api_headers(), term_to_binary(Payload)}}. + fixture(get, _, #{<<"if-none-match">> := <<"\"dummy\"">> = ETag}, _) -> Headers = #{ <<"etag">> => ETag @@ -346,20 +362,12 @@ fixture(post, <>, _, {_, Body}) -> fixture(post, <>, _, {_, Body}) -> DecodedBody = binary_to_term(Body), #{<<"client_id">> := _ClientId, <<"scope">> := _Scope} = DecodedBody, - DeviceCode = base64:encode(crypto:strong_rand_bytes(32)), - UserCode = iolist_to_binary([ - integer_to_binary(rand:uniform(9999)), "-", - integer_to_binary(rand:uniform(9999)) - ]), - Payload = #{ - <<"device_code">> => DeviceCode, - <<"user_code">> => UserCode, - <<"verification_uri">> => <<"https://hex.pm/oauth/device">>, - <<"verification_uri_complete">> => <<"https://hex.pm/oauth/device?user_code=", UserCode/binary>>, - <<"expires_in">> => 600, - <<"interval">> => 0 - }, - {ok, {200, api_headers(), term_to_binary(Payload)}}; + receive + {hex_http_test, oauth_device_authorization_response, Response} -> + Response + after 0 -> + device_authorization_fixture() + end; fixture(post, <>, _, {_, Body}) -> DecodedBody = binary_to_term(Body), From 9ea52a083e00f471639ec5e419642e0367c3c893 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sun, 23 Aug 2026 22:09:23 +0200 Subject: [PATCH 4/5] Close what the third CLI review found Escape cmd.exe metacharacters before handing a server-supplied URL to the Windows browser opener. erts quotes an argument only when it contains a space or a quote, so a verification URI carrying a bare & reached cmd as a command separator. Classify a refresh response by what it says. Only 400 and 401 mean the refresh token is dead and clear the stored credentials; every other status, a transport failure, and a 200 whose body does not carry a usable access_token and expires_in leave the token in place. Renew on a token_expired challenge instead of resolving the same bearer again, and bound the renewals. The api path resolved with renewal disabled, so a token outside its expiry window came back unchanged and the request retried forever. The retry budget lives with the other retry state so it survives a reauthentication rather than resetting. Validate the device authorization, poll and refresh responses before destructuring them, so a malformed 200 returns an error the callers already handle instead of a badmatch, a badarith, or a badarg out of timer:sleep. Distinguish a missing sso_reauth_required from a malformed one. Missing still means nothing lapsed, for servers that do not send it; a value that is not a list of binaries no longer reads as an authoritative empty set that clears the stored organizations. Hold the device authentication lock over credential acquisition only. Taking a lock the same process already holds adds no reference, and releasing the inner one deleted the entry, so the outer body ran unlocked and a second process could start a concurrent device authentication. --- src/hex_api_oauth.erl | 157 ++++++++++++++----- src/hex_cli_auth.erl | 228 +++++++++++++++++---------- test/hex_api_SUITE.erl | 150 ++++++++++++++++++ test/hex_cli_auth_SUITE.erl | 301 +++++++++++++++++++++++++++++++++++- 4 files changed, 717 insertions(+), 119 deletions(-) diff --git a/src/hex_api_oauth.erl b/src/hex_api_oauth.erl index 113e2939..b1f35269 100644 --- a/src/hex_api_oauth.erl +++ b/src/hex_api_oauth.erl @@ -12,7 +12,8 @@ sso_reauth_required/1, revoke_token/3, client_credentials_token/4, - client_credentials_token/5 + client_credentials_token/5, + win_cmd_args/1 ]). -export_type([oauth_tokens/0, device_auth_error/0]). @@ -146,28 +147,47 @@ device_auth_flow(Config, ClientId, Scope, PromptUser) -> ) -> {ok, oauth_tokens()} | {error, device_auth_error()}. device_auth_flow(Config, ClientId, Scope, PromptUser, Opts) -> case device_authorization(Config, ClientId, Scope, Opts) of - {ok, {200, _, DeviceResponse}} when is_map(DeviceResponse) -> - #{ - <<"device_code">> := DeviceCode, - <<"user_code">> := UserCode, - <<"verification_uri_complete">> := VerificationUri, - <<"expires_in">> := ExpiresIn, - <<"interval">> := IntervalSeconds - } = DeviceResponse, - ok = PromptUser(VerificationUri, UserCode), - OpenBrowser = proplists:get_value(open_browser, Opts, false), - case OpenBrowser of - true -> open_browser(VerificationUri); - false -> ok - end, - ExpiresAt = erlang:system_time(second) + ExpiresIn, - poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt); + {ok, {200, _, DeviceResponse}} -> + case device_authorization_fields(DeviceResponse) of + {ok, DeviceCode, UserCode, VerificationUri, ExpiresIn, IntervalSeconds} -> + ok = PromptUser(VerificationUri, UserCode), + OpenBrowser = proplists:get_value(open_browser, Opts, false), + case OpenBrowser of + true -> open_browser(VerificationUri); + false -> ok + end, + ExpiresAt = erlang:system_time(second) + ExpiresIn, + poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt); + error -> + {error, {device_auth_failed, 200, DeviceResponse}} + end; {ok, {Status, _, Body}} -> {error, {device_auth_failed, Status, Body}}; {error, Reason} -> {error, Reason} end. +%% @private +%% The fields the flow goes on to use, in the types it uses them as: the +%% interval is slept on and the expiry is added to a timestamp. +device_authorization_fields(#{ + <<"device_code">> := DeviceCode, + <<"user_code">> := UserCode, + <<"verification_uri_complete">> := VerificationUri, + <<"expires_in">> := ExpiresIn, + <<"interval">> := Interval +}) when + is_binary(DeviceCode), + is_binary(UserCode), + is_binary(VerificationUri), + is_integer(ExpiresIn), + is_integer(Interval), + Interval >= 0 +-> + {ok, DeviceCode, UserCode, VerificationUri, ExpiresIn, Interval}; +device_authorization_fields(_DeviceResponse) -> + error. + %% @private poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) -> Now = erlang:system_time(second), @@ -177,17 +197,20 @@ poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) -> false -> timer:sleep(IntervalSeconds * 1000), case poll_device_token(Config, ClientId, DeviceCode) of - {ok, {200, _, TokenResponse}} when is_map(TokenResponse) -> - #{ - <<"access_token">> := AccessToken, - <<"expires_in">> := ExpiresIn - } = TokenResponse, - Tokens = #{ - access_token => AccessToken, - expires_at => erlang:system_time(second) + ExpiresIn, - sso_reauth_required => sso_reauth_required(TokenResponse) - }, - {ok, put_refresh_token(Tokens, TokenResponse)}; + {ok, {200, _, TokenResponse}} -> + case token_response_fields(TokenResponse) of + {ok, AccessToken, ExpiresIn} -> + Tokens = #{ + access_token => AccessToken, + expires_at => erlang:system_time(second) + ExpiresIn + }, + {ok, + put_sso_reauth_required( + put_refresh_token(Tokens, TokenResponse), TokenResponse + )}; + error -> + {error, {poll_failed, 200, TokenResponse}} + end; {ok, {400, _, #{<<"error">> := <<"authorization_pending">>}}} -> poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt); {ok, {400, _, #{<<"error">> := <<"slow_down">>}}} -> @@ -374,16 +397,25 @@ revoke_token(Config, ClientId, Token) -> }, hex_api:post(Config, Path, Params). -%% @private +%% @doc %% Organizations a token response says the session has to authenticate against -%% their identity provider for. Older servers do not send the field at all, -%% which means nothing is lapsed. --spec sso_reauth_required(map()) -> [binary()]. -sso_reauth_required(TokenResponse) -> - case maps:get(<<"sso_reauth_required">>, TokenResponse, []) of - Organizations when is_list(Organizations) -> Organizations; - _Other -> [] - end. +%% their identity provider for. +%% +%% Returns `{ok, []}' when the response does not carry the field, which is what +%% servers that predate it send and means nothing is lapsed. Returns `error' +%% when the field is there in a shape that cannot be read, which says nothing +%% about what has lapsed and must not be taken for the empty set. +%% @end +-spec sso_reauth_required(map()) -> {ok, [binary()]} | error. +sso_reauth_required(#{<<"sso_reauth_required">> := Organizations}) when is_list(Organizations) -> + case lists:all(fun is_binary/1, Organizations) of + true -> {ok, Organizations}; + false -> error + end; +sso_reauth_required(#{<<"sso_reauth_required">> := _Organizations}) -> + error; +sso_reauth_required(_TokenResponse) -> + {ok, []}. %%==================================================================== %% Internal functions @@ -413,7 +445,7 @@ spawn_browser(Url) -> {unix, _} -> {"xdg-open", [Url]}; {win32, _} -> - {"cmd", ["/c", "start", "", Url]} + {"cmd", win_cmd_args(Url)} end, case os:find_executable(Cmd) of false -> @@ -423,6 +455,37 @@ spawn_browser(Url) -> ok end. +%% @private +%% `start' takes its first quoted argument as the window title, so the empty +%% string keeps the URL in the position `start' opens. +-spec win_cmd_args(string()) -> [string()]. +win_cmd_args(Url) -> + ["/c", "start", "", escape_win_cmd(Url)]. + +%% @private +%% cmd.exe parses the command line before `start' sees it, and erts only quotes +%% an argument that contains whitespace, so every character cmd acts on is +%% prefixed with a caret. `%' is included because cmd expands `%NAME%' before it +%% scans for separators. +escape_win_cmd(Url) -> + lists:flatmap(fun escape_win_cmd_character/1, Url). + +%% @private +escape_win_cmd_character(Character) when + Character =:= $^; + Character =:= $&; + Character =:= $|; + Character =:= $<; + Character =:= $>; + Character =:= $(; + Character =:= $); + Character =:= $"; + Character =:= $% +-> + [$^, Character]; +escape_win_cmd_character(Character) -> + [Character]. + %% @private %% Whether a URL uses the http:// or https:// scheme. -spec valid_http_url(binary()) -> boolean(). @@ -433,6 +496,15 @@ valid_http_url(Url) when is_binary(Url) -> _ -> false end. +%% @private +%% The access token and its lifetime, in the types the caller uses them as. +token_response_fields(#{<<"access_token">> := AccessToken, <<"expires_in">> := ExpiresIn}) when + is_binary(AccessToken), is_integer(ExpiresIn) +-> + {ok, AccessToken, ExpiresIn}; +token_response_fields(_TokenResponse) -> + error. + %% @private %% A response without a refresh token carries no key at all rather than a %% placeholder, so what a build tool stores is only ever a real token. @@ -441,6 +513,15 @@ put_refresh_token(Tokens, #{<<"refresh_token">> := RefreshToken}) when is_binary put_refresh_token(Tokens, _TokenResponse) -> Tokens. +%% @private +%% A response whose sso_reauth_required cannot be read carries no key, so the +%% caller is not handed the empty set as if the server had sent it. +put_sso_reauth_required(Tokens, TokenResponse) -> + case sso_reauth_required(TokenResponse) of + {ok, Organizations} -> Tokens#{sso_reauth_required => Organizations}; + error -> Tokens + end. + %% @private %% Get the hostname of the current machine. -spec get_hostname() -> binary(). diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index ca948463..65a77530 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -28,8 +28,8 @@ %% RefreshToken :: binary() | undefined, %% ExpiresAt :: integer()) -> ok), %% -%% %% Invalidate the stored global OAuth token after it expired and could -%% %% not be refreshed (optional). Lets the build tool drop the unusable +%% %% Invalidate the stored global OAuth token after the server refused to +%% %% refresh it (optional). Lets the build tool drop the unusable %% %% token so concurrent and subsequent callers stop retrying the doomed %% %% refresh, and warn the user. Invoked at most once per resolution, while %% %% holding the token-refresh lock. @@ -37,9 +37,10 @@ %% %% %% Report the organizations the server says this session has to %% %% authenticate against their identity provider for (optional). Called -%% %% after every token grant, with the empty list when there are none, so -%% %% the build tool always holds the current set. It is not told which of -%% %% them the running command needs; deciding that is the build tool's job. +%% %% after every token grant that carried a readable set, with the empty +%% %% list when there are none, so the build tool always holds the current +%% %% set. It is not told which of them the running command needs; deciding +%% %% that is the build tool's job. %% sso_reauth => fun(([binary()]) -> ok), %% %% %% User interaction @@ -114,8 +115,12 @@ %% Maximum OTP retry attempts -define(MAX_OTP_RETRIES, 3). -%% Both ways a refresh can leave us without a usable token: the server rejected -%% the refresh token, or the request never got an answer. +%% Maximum times a 401 that says the token expired is answered by renewing the +%% credential and running the request again. +-define(MAX_TOKEN_RETRIES, 1). + +%% Both ways a refresh can leave us without a usable token: the server refused +%% the refresh token, or the refresh got no usable answer. -define(IS_REFRESH_FAILURE(Reason), (Reason =:= token_refresh_failed orelse Reason =:= token_refresh_unavailable) ). @@ -162,9 +167,10 @@ | {auth_error, auth_declined} | {auth_error, otp_cancelled} | {auth_error, otp_max_retries} - %% The server refused to refresh the token + %% The server refused the refresh token: 400 or 401 | {auth_error, token_refresh_failed} - %% The refresh request got no answer: DNS, connect, timeout, TLS + %% The refresh got no usable answer: DNS, connect, timeout, TLS, a 429 or a + %% 5xx, or a 200 whose body is not a token response | {auth_error, token_refresh_unavailable} | {auth_error, device_auth_timeout} | {auth_error, device_auth_denied} @@ -175,6 +181,13 @@ has_refresh_token => boolean() }. +%% How much of each retry budget a request has already spent. +-type retries() :: #{ + otp := non_neg_integer(), + otp_error := invalid_totp | undefined, + token := non_neg_integer() +}. + -type opts() :: [ {optional, boolean()} | {auth_inline, boolean()} @@ -249,13 +262,15 @@ with_api(Permission, BaseConfig, Fun, Opts) -> case resolve_api_auth(Permission, BaseConfig) of {ok, ApiKey, AuthContext} -> Config = BaseConfig#{api_key => ApiKey}, - execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); + execute_with_retry(Config, Fun, AuthContext, initial_retries(), Opts); {error, no_auth} when Optional =:= true -> %% Auth is optional, try without credentials first execute_optional_with_retry(api, BaseConfig, Fun, AuthInline, Opts); {error, no_auth} when AuthInline =:= true -> %% No auth found, ask user if they want to authenticate - maybe_authenticate_and_retry(api, BaseConfig, Fun, no_credentials, Opts); + maybe_authenticate_and_retry( + api, BaseConfig, Fun, no_credentials, initial_retries(), Opts + ); {error, no_auth} -> %% auth_inline is false, just return error {error, {auth_error, no_credentials}}; @@ -330,7 +345,9 @@ with_repo(BaseConfig, Fun, Opts) -> execute_optional_with_retry(repo, BaseConfig, Fun, AuthInline, Opts); no_auth when AuthInline =:= true -> %% No auth found, ask user if they want to authenticate - maybe_authenticate_and_retry(repo, BaseConfig, Fun, no_credentials, Opts); + maybe_authenticate_and_retry( + repo, BaseConfig, Fun, no_credentials, initial_retries(), Opts + ); no_auth -> %% auth_inline is false, return error {error, {auth_error, no_credentials}}; @@ -351,55 +368,65 @@ repo_name(_) -> <<"hexpm">>. %% @private -%% Ask user if they want to authenticate, and if yes, initiate device auth. +%% Ask user if they want to authenticate, and if yes, initiate device auth, then +%% run the request with what that produced. %% %% Kind says which credential the retried request needs: `api' takes the token %% as api_key, `repo' resolves repository auth and takes it as repo_key. %% -%% Serialized with a global lock so concurrent callers don't each trigger their -%% own device auth flow. The first caller to acquire the lock runs device auth -%% and persists the resulting token; subsequent callers re-check for an existing -%% (now-valid) token inside the lock and reuse it instead of re-authenticating. -maybe_authenticate_and_retry(Kind, BaseConfig, Fun, Reason, Opts) -> - global:trans( +%% Acquiring the credential is serialized with a global lock so concurrent +%% callers don't each trigger their own device auth flow. The first caller to +%% acquire the lock runs device auth and persists the resulting token; +%% subsequent callers re-check for an existing (now-valid) token inside the lock +%% and reuse it instead of re-authenticating. The request runs outside the lock. +%% A 401 brings it back here, and global:trans/4 on a lock this process already +%% holds does not take a second reference: the inner transaction's exit deletes +%% the lock entry while the outer one is still running. +maybe_authenticate_and_retry(Kind, BaseConfig, Fun, Reason, Retries, Opts) -> + Credential = global:trans( {{?MODULE, device_auth}, self()}, fun() -> - do_maybe_authenticate_and_retry(Kind, BaseConfig, Fun, Reason, Opts) + acquire_credential(Kind, BaseConfig, Reason, Opts) end, [node()], infinity - ). + ), + case Credential of + {ok, Key, AuthContext} -> + execute_authenticated(Kind, BaseConfig, Fun, Key, AuthContext, Retries, Opts); + {error, _} = Error -> + Error + end. %% @private %% Another caller may have authenticated while we waited for the lock. Re-resolve %% and, if we get a token that differs from the one we arrived with (none when %% credentials were missing; the rejected one on token_refresh_failed), reuse it %% instead of prompting again. Otherwise proceed to prompt + device auth. -do_maybe_authenticate_and_retry(api, BaseConfig, Fun, Reason, Opts) -> +acquire_credential(api, BaseConfig, Reason, Opts) -> CurrentApiKey = maps:get(api_key, BaseConfig, undefined), case resolve_api_auth(write, BaseConfig) of {ok, ApiKey, AuthContext} when ApiKey =/= CurrentApiKey -> - Config = BaseConfig#{api_key => ApiKey}, - execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); + {ok, ApiKey, AuthContext}; _ -> - prompt_and_device_auth(api, BaseConfig, Fun, Reason, Opts) + prompt_and_device_auth(api, BaseConfig, Reason, Opts) end; -do_maybe_authenticate_and_retry(repo, BaseConfig, Fun, Reason, Opts) -> +acquire_credential(repo, BaseConfig, Reason, Opts) -> CurrentRepoKey = maps:get(repo_key, BaseConfig, undefined), case resolve_repo_auth(BaseConfig) of - {ok, RepoKey, _AuthContext} when is_binary(RepoKey), RepoKey =/= CurrentRepoKey -> - execute_repo_with_retry(BaseConfig, Fun, RepoKey); + {ok, RepoKey, AuthContext} when is_binary(RepoKey), RepoKey =/= CurrentRepoKey -> + {ok, RepoKey, AuthContext}; _ -> - prompt_and_device_auth(repo, BaseConfig, Fun, Reason, Opts) + prompt_and_device_auth(repo, BaseConfig, Reason, Opts) end. %% @private -prompt_and_device_auth(Kind, BaseConfig, Fun, Reason, Opts) -> +prompt_and_device_auth(Kind, BaseConfig, Reason, Opts) -> case call_callback(BaseConfig, should_authenticate, [Reason]) of true -> case device_auth(BaseConfig, <<"api repositories">>, Opts) of {ok, Tokens} -> - retry_authenticated(Kind, BaseConfig, Fun, Tokens, Opts); + authenticated_credential(Kind, BaseConfig, Tokens); {error, _} = Error -> Error end; @@ -408,31 +435,36 @@ prompt_and_device_auth(Kind, BaseConfig, Fun, Reason, Opts) -> end. %% @private -%% Run the request with the credentials device auth just produced. The token is -%% the user's API token; what a repository request needs is repository auth, -%% which the token may only be one input to, so resolve that instead of reusing -%% the API-shaped one. -retry_authenticated(api, BaseConfig, Fun, #{access_token := AccessToken} = Tokens, Opts) -> - Config = BaseConfig#{api_key => <<"Bearer ", AccessToken/binary>>}, - AuthContext = #{has_refresh_token => has_refresh_token(Tokens)}, - execute_with_retry(Config, Fun, AuthContext, 0, undefined, Opts); -retry_authenticated(repo, BaseConfig, Fun, _Tokens, _Opts) -> +%% The credential device auth just produced. The token is the user's API token; +%% what a repository request needs is repository auth, which the token may only +%% be one input to, so resolve that instead of reusing the API-shaped one. +authenticated_credential(api, _BaseConfig, #{access_token := AccessToken} = Tokens) -> + {ok, <<"Bearer ", AccessToken/binary>>, #{has_refresh_token => has_refresh_token(Tokens)}}; +authenticated_credential(repo, BaseConfig, _Tokens) -> case resolve_repo_auth(BaseConfig) of - {ok, RepoKey, _AuthContext} when is_binary(RepoKey) -> - execute_repo_with_retry(BaseConfig, Fun, RepoKey); + {ok, RepoKey, AuthContext} when is_binary(RepoKey) -> + {ok, RepoKey, AuthContext}; no_auth -> {error, {auth_error, no_credentials}}; {error, _} = Error -> Error end. +%% @private +execute_authenticated(api, BaseConfig, Fun, ApiKey, AuthContext, Retries, Opts) -> + execute_with_retry(BaseConfig#{api_key => ApiKey}, Fun, AuthContext, Retries, Opts); +execute_authenticated(repo, BaseConfig, Fun, RepoKey, _AuthContext, _Retries, _Opts) -> + execute_repo_with_retry(BaseConfig, Fun, RepoKey). + %% @private %% Execute function without auth, but retry with auth if we get a 401. execute_optional_with_retry(Kind, BaseConfig, Fun, AuthInline, Opts) -> case Fun(BaseConfig) of {ok, {401, _Headers, _Body}} when AuthInline =:= true -> %% Got 401, need auth - ask user if they want to authenticate - maybe_authenticate_and_retry(Kind, BaseConfig, Fun, no_credentials, Opts); + maybe_authenticate_and_retry( + Kind, BaseConfig, Fun, no_credentials, initial_retries(), Opts + ); {ok, {401, _Headers, _Body}} -> %% Got 401 but auth_inline is false, return error {error, {auth_error, no_credentials}}; @@ -513,12 +545,13 @@ device_auth(Config, Scope, Opts) -> end, FlowOpts = [{open_browser, OpenBrowser}], case hex_api_oauth:device_auth_flow(Config, ClientId, Scope, PromptUser, FlowOpts) of - {ok, #{sso_reauth_required := SsoReauthRequired} = Response} -> + {ok, Response} -> %% sso_reauth_required reaches the build tool through the sso_reauth - %% callback rather than with the tokens. + %% callback rather than with the tokens. The response carries no key + %% when the server sent a set that could not be read. Tokens = maps:without([sso_reauth_required], Response), ok = persist_tokens(Config, global, Tokens), - report_sso_reauth(Config, SsoReauthRequired), + report_sso_reauth(Config, maps:find(sso_reauth_required, Response)), {ok, Tokens}; {error, timeout} -> {error, {auth_error, device_auth_timeout}}; @@ -739,7 +772,7 @@ with_token_refresh_lock(Config, Fun) -> %% This runs inside the token_refresh lock, so the unusable token is dropped %% exactly once and the callers serialized behind the lock re-read it as absent %% instead of each retrying the doomed refresh against the server. A refresh -%% that never reached the server says nothing about the token, so it is kept. +%% that got no usable answer says nothing about the token, so it is kept. refresh_or_clear(Config, Tokens) -> case maybe_refresh_token_with_context(Config, Tokens) of {ok, _Bearer, _Ctx} = Ok -> @@ -752,34 +785,50 @@ refresh_or_clear(Config, Tokens) -> end. %% @private +%% Only 400 and 401 are the server refusing the refresh token, which is what +%% makes the stored credential dead. A 429, a 5xx and a 200 whose body cannot be +%% read all leave the refresh token as good as it was. maybe_refresh_token_with_context(Config, #{refresh_token := RefreshToken}) when is_binary(RefreshToken) -> ClientId = call_callback(Config, get_client_id, []), case hex_api_oauth:refresh_token(Config, ClientId, RefreshToken) of - {ok, {200, _, TokenResponse}} when is_map(TokenResponse) -> - #{ - <<"access_token">> := NewAccessToken, - <<"expires_in">> := ExpiresIn - } = TokenResponse, - NewRefreshToken = maps:get(<<"refresh_token">>, TokenResponse, RefreshToken), + {ok, + {200, _, + #{ + <<"access_token">> := NewAccessToken, + <<"expires_in">> := ExpiresIn + } = TokenResponse}} when + is_binary(NewAccessToken), is_integer(ExpiresIn) + -> NewTokens = #{ access_token => NewAccessToken, - refresh_token => NewRefreshToken, + refresh_token => new_refresh_token(TokenResponse, RefreshToken), expires_at => erlang:system_time(second) + ExpiresIn }, ok = persist_tokens(Config, global, NewTokens), report_sso_reauth(Config, hex_api_oauth:sso_reauth_required(TokenResponse)), BearerToken = <<"Bearer ", NewAccessToken/binary>>, {ok, BearerToken, #{has_refresh_token => has_refresh_token(NewTokens)}}; - {ok, {_Status, _, _Body}} -> + {ok, {Status, _, _Body}} when Status =:= 400; Status =:= 401 -> {error, {auth_error, token_refresh_failed}}; + {ok, {_Status, _, _Body}} -> + {error, {auth_error, token_refresh_unavailable}}; {error, _Reason} -> {error, {auth_error, token_refresh_unavailable}} end; maybe_refresh_token_with_context(_Config, _Tokens) -> {error, {auth_error, token_refresh_failed}}. +%% @private +%% A refresh that does not rotate the refresh token keeps the one we sent. +new_refresh_token(#{<<"refresh_token">> := RefreshToken}, _CurrentRefreshToken) when + is_binary(RefreshToken) +-> + RefreshToken; +new_refresh_token(_TokenResponse, CurrentRefreshToken) -> + CurrentRefreshToken. + %% @private %% Whether these tokens can be refreshed. -spec has_refresh_token(oauth_tokens()) -> boolean(). @@ -802,18 +851,23 @@ persist_tokens(Config, Scope, #{access_token := AccessToken, expires_at := Expir %%==================================================================== %% @private -execute_with_retry(Config, Fun, AuthContext, OtpRetries, LastOtpError, Opts) -> +-spec initial_retries() -> retries(). +initial_retries() -> + #{otp => 0, otp_error => undefined, token => 0}. + +%% @private +execute_with_retry(Config, Fun, AuthContext, Retries, Opts) -> case Fun(Config) of {error, otp_required} -> handle_otp_retry( - Config, Fun, AuthContext, OtpRetries, <<"Enter OTP code:">>, Opts + Config, Fun, AuthContext, Retries, <<"Enter OTP code:">>, Opts ); {error, invalid_totp} -> handle_otp_retry( Config, Fun, AuthContext, - OtpRetries, + Retries, <<"Invalid OTP code. Please try again:">>, Opts ); @@ -821,17 +875,17 @@ execute_with_retry(Config, Fun, AuthContext, OtpRetries, LastOtpError, Opts) -> case detect_auth_error(Headers) of otp_required -> handle_otp_retry( - Config, Fun, AuthContext, OtpRetries, <<"Enter OTP code:">>, Opts + Config, Fun, AuthContext, Retries, <<"Enter OTP code:">>, Opts ); invalid_totp -> Msg = - case LastOtpError of + case maps:get(otp_error, Retries) of invalid_totp -> <<"Invalid OTP code. Please try again:">>; _ -> <<"Enter OTP code:">> end, - handle_otp_retry(Config, Fun, AuthContext, OtpRetries, Msg, Opts); + handle_otp_retry(Config, Fun, AuthContext, Retries, Msg, Opts); token_expired -> - handle_token_refresh_retry(Config, Fun, AuthContext, Opts); + handle_token_refresh_retry(Config, Fun, AuthContext, Retries, Response, Opts); none -> Response end; @@ -840,47 +894,57 @@ execute_with_retry(Config, Fun, AuthContext, OtpRetries, LastOtpError, Opts) -> end. %% @private -handle_otp_retry(_Config, _Fun, _AuthContext, OtpRetries, _Message, _Opts) when +handle_otp_retry(_Config, _Fun, _AuthContext, #{otp := OtpRetries}, _Message, _Opts) when OtpRetries >= ?MAX_OTP_RETRIES -> {error, {auth_error, otp_max_retries}}; -handle_otp_retry(Config, Fun, AuthContext, OtpRetries, Message, Opts) -> +handle_otp_retry(Config, Fun, AuthContext, #{otp := OtpRetries} = Retries, Message, Opts) -> case call_callback(Config, prompt_otp, [Message]) of {ok, OtpCode} -> NewConfig = Config#{api_otp => OtpCode}, - execute_with_retry( - NewConfig, Fun, AuthContext, OtpRetries + 1, invalid_totp, Opts - ); + NewRetries = Retries#{otp := OtpRetries + 1, otp_error := invalid_totp}, + execute_with_retry(NewConfig, Fun, AuthContext, NewRetries, Opts); cancelled -> {error, {auth_error, otp_cancelled}} end. %% @private -handle_token_refresh_retry(Config, Fun, AuthContext, Opts) -> +%% A 401 that says the token expired is answered by renewing the credential at +%% its source and running the request once more, the way a repository request +%% is. The renewal is counted, so a server that answers token_expired to every +%% bearer we send it gets a bounded number of requests rather than a loop. +handle_token_refresh_retry( + _Config, _Fun, _AuthContext, #{token := TokenRetries}, Response, _Opts +) when + TokenRetries >= ?MAX_TOKEN_RETRIES +-> + Response; +handle_token_refresh_retry( + Config, Fun, AuthContext, #{token := TokenRetries} = Retries, _Response, Opts +) -> + NewRetries = Retries#{token := TokenRetries + 1}, %% Only attempt refresh if we have a refresh token case maps:get(has_refresh_token, AuthContext, false) of true -> - case resolve_oauth_token_with_context(Config, false) of + case resolve_oauth_token_with_context(Config, true) of {ok, NewBearerToken, NewAuthContext} -> NewConfig = Config#{api_key => NewBearerToken}, - execute_with_retry( - NewConfig, Fun, NewAuthContext, 0, undefined, Opts - ); + execute_with_retry(NewConfig, Fun, NewAuthContext, NewRetries, Opts); {error, _} -> - maybe_reauthenticate(Config, Fun, Opts) + maybe_reauthenticate(Config, Fun, NewRetries, Opts) end; false -> - maybe_reauthenticate(Config, Fun, Opts) + maybe_reauthenticate(Config, Fun, NewRetries, Opts) end. %% @private %% After token refresh failure, prompt the user to re-authenticate via device auth %% (only when auth_inline is true). Mirrors Hex.OAuth.reauthenticate/1. -maybe_reauthenticate(Config, Fun, Opts) -> +maybe_reauthenticate(Config, Fun, Retries, Opts) -> AuthInline = proplists:get_value(auth_inline, Opts, true), case AuthInline of true -> - maybe_authenticate_and_retry(api, Config, Fun, token_refresh_failed, Opts); + maybe_authenticate_and_retry(api, Config, Fun, token_refresh_failed, Retries, Opts); false -> {error, {auth_error, token_refresh_failed}} end. @@ -920,10 +984,14 @@ call_callback(Config, Name, Args) -> %% @private %% Hands the build tool the organizations this session has to authenticate for. -%% Always called after a grant, including with the empty list, so a set that -%% has been resolved does not linger. -report_sso_reauth(Config, Organizations) when is_list(Organizations) -> - maybe_call_callback(Config, sso_reauth, [Organizations]). +%% A grant that flagged none carries the empty list, so a set that has been +%% resolved does not linger. A grant whose set could not be read is not +%% reported: the empty list would be taken for the server saying there is +%% nothing, and the build tool would drop the organizations it holds. +report_sso_reauth(Config, {ok, Organizations}) when is_list(Organizations) -> + maybe_call_callback(Config, sso_reauth, [Organizations]); +report_sso_reauth(_Config, error) -> + ok. %% @private %% Like call_callback/3 but for optional callbacks: returns ok without doing diff --git a/test/hex_api_SUITE.erl b/test/hex_api_SUITE.erl index 1bf9b508..53a3335c 100644 --- a/test/hex_api_SUITE.erl +++ b/test/hex_api_SUITE.erl @@ -35,9 +35,14 @@ all() -> oauth_device_auth_flow_poll_error_test, oauth_device_auth_flow_no_refresh_token_test, oauth_device_auth_flow_invalid_verification_uri_test, + oauth_device_auth_flow_malformed_device_response_test, + oauth_device_auth_flow_malformed_token_response_test, oauth_refresh_token_test, oauth_sso_authorization_test, oauth_device_auth_flow_sso_reauth_test, + oauth_device_auth_flow_malformed_sso_reauth_test, + oauth_sso_reauth_required_test, + oauth_win_cmd_args_escapes_metacharacters_test, oauth_revoke_test, oauth_client_credentials_test, publish_with_expect_header_test, @@ -315,6 +320,77 @@ oauth_device_auth_flow_invalid_verification_uri_test(_Config) -> ?assertEqual(<<"test_access_token">>, maps:get(access_token, Tokens)), ok. +oauth_device_auth_flow_malformed_device_response_test(_Config) -> + % A 200 that does not carry the fields the flow uses is a failed device + % authorization, not a badmatch or a timer:sleep/1 badarg in the caller. + ClientId = <<"cli">>, + Scope = <<"api:write">>, + Self = self(), + PromptUser = fun(_VerificationUri, _UserCode) -> error(prompt_called) end, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + + Complete = #{ + <<"device_code">> => <<"device_code">>, + <<"user_code">> => <<"1234-5678">>, + <<"verification_uri_complete">> => <<"https://hex.pm/oauth/device?user_code=1234-5678">>, + <<"expires_in">> => 600, + <<"interval">> => 0 + }, + Malformed = [ + maps:remove(<<"device_code">>, Complete), + maps:remove(<<"verification_uri_complete">>, Complete), + Complete#{<<"interval">> => <<"5">>}, + Complete#{<<"interval">> => -1}, + Complete#{<<"expires_in">> => <<"600">>}, + Complete#{<<"verification_uri_complete">> => 42}, + <<"not a map">> + ], + + [ + begin + Self ! + {hex_http_test, oauth_device_authorization_response, + {ok, {200, Headers, term_to_binary(Payload)}}}, + ?assertEqual( + {error, {device_auth_failed, 200, Payload}}, + hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser) + ) + end + || Payload <- Malformed + ], + ok. + +oauth_device_auth_flow_malformed_token_response_test(_Config) -> + % Same for the poll: a 200 without a usable access token ends the flow with + % an error the caller already handles. + ClientId = <<"cli">>, + Scope = <<"api:write">>, + Self = self(), + PromptUser = fun(_VerificationUri, _UserCode) -> ok end, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + + Malformed = [ + #{<<"expires_in">> => 3600}, + #{<<"access_token">> => <<"test_access_token">>}, + #{<<"access_token">> => <<"test_access_token">>, <<"expires_in">> => <<"3600">>}, + #{<<"access_token">> => 42, <<"expires_in">> => 3600}, + <<"not a map">> + ], + + [ + begin + Self ! + {hex_http_test, oauth_device_response, + {ok, {200, Headers, term_to_binary(Payload)}}}, + ?assertEqual( + {error, {poll_failed, 200, Payload}}, + hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser) + ) + end + || Payload <- Malformed + ], + ok. + oauth_refresh_token_test(_Config) -> % Test token refresh ClientId = <<"cli">>, @@ -367,6 +443,80 @@ oauth_device_auth_flow_sso_reauth_test(_Config) -> ?assertEqual([<<"acme">>], maps:get(sso_reauth_required, Tokens)), ok. +oauth_device_auth_flow_malformed_sso_reauth_test(_Config) -> + % A set the server sent in a shape we cannot read carries no key at all. The + % empty list means "nothing lapsed", which is not what the response said. + ClientId = <<"cli">>, + Scope = <<"repositories">>, + Self = self(), + PromptUser = fun(_VerificationUri, _UserCode) -> ok end, + + SuccessPayload = #{ + <<"access_token">> => <<"test_access_token">>, + <<"refresh_token">> => <<"test_refresh_token">>, + <<"token_type">> => <<"Bearer">>, + <<"expires_in">> => 3600, + <<"sso_reauth_required">> => <<"acme">> + }, + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + Self ! + {hex_http_test, oauth_device_response, + {ok, {200, Headers, term_to_binary(SuccessPayload)}}}, + + {ok, Tokens} = hex_api_oauth:device_auth_flow(?CONFIG, ClientId, Scope, PromptUser), + + ?assertNot(maps:is_key(sso_reauth_required, Tokens)), + ok. + +oauth_sso_reauth_required_test(_Config) -> + % A response that does not carry the field is a server that predates it and + % means nothing is lapsed; one that carries an unreadable value means the + % response says nothing at all. + ?assertEqual({ok, []}, hex_api_oauth:sso_reauth_required(#{})), + ?assertEqual( + {ok, []}, + hex_api_oauth:sso_reauth_required(#{<<"sso_reauth_required">> => []}) + ), + ?assertEqual( + {ok, [<<"acme">>]}, + hex_api_oauth:sso_reauth_required(#{<<"sso_reauth_required">> => [<<"acme">>]}) + ), + ?assertEqual( + error, + hex_api_oauth:sso_reauth_required(#{<<"sso_reauth_required">> => <<"acme">>}) + ), + ?assertEqual( + error, + hex_api_oauth:sso_reauth_required(#{<<"sso_reauth_required">> => [<<"acme">>, 42]}) + ), + ?assertEqual( + error, + hex_api_oauth:sso_reauth_required(#{<<"sso_reauth_required">> => null}) + ), + ok. + +oauth_win_cmd_args_escapes_metacharacters_test(_Config) -> + % cmd.exe parses the command line before `start' sees it, and erts only + % quotes an argument containing whitespace, so a server-supplied URL reaches + % cmd with its separators inert or it runs whatever trails them. + ?assertEqual( + ["/c", "start", "", "https://example.com/^&calc.exe"], + hex_api_oauth:win_cmd_args("https://example.com/&calc.exe") + ), + ?assertEqual( + ["/c", "start", "", "https://example.com/^%PATH^%"], + hex_api_oauth:win_cmd_args("https://example.com/%PATH%") + ), + ?assertEqual( + ["/c", "start", "", "^^^&^|^<^>^(^)^\"^%"], + hex_api_oauth:win_cmd_args("^&|<>()\"%") + ), + ?assertEqual( + ["/c", "start", "", "https://example.com/plain"], + hex_api_oauth:win_cmd_args("https://example.com/plain") + ), + ok. + oauth_revoke_test(_Config) -> % Test token revocation ClientId = <<"cli">>, diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index 0f0eab65..07443ad2 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -55,11 +55,15 @@ all() -> %% sso re-authorization sso_reauth_reported_on_refresh_test, sso_reauth_reported_empty_test, + sso_reauth_malformed_not_reported_test, refresh_tokens_forces_a_refresh_test, refresh_tokens_without_credentials_test, %% with_api tests - token refresh on 401 with_api_token_expired_refresh_test, + with_api_token_expired_renews_test, + with_api_token_expired_retry_bounded_test, + with_api_token_expired_reauth_retry_bounded_test, %% with_api tests - reauthentication after refresh failure with_api_token_expired_reauth_yes_test, @@ -83,11 +87,15 @@ all() -> %% token refresh failure modes refresh_transport_error_keeps_token_test, + refresh_server_error_keeps_token_test, + refresh_malformed_body_keeps_token_test, + refresh_refusal_clears_token_test, %% concurrency tests resolve_oauth_token_concurrent_refresh_serialized_test, resolve_oauth_token_refresh_failure_clears_once_test, - device_auth_concurrent_serialized_reuses_login_test + device_auth_concurrent_serialized_reuses_login_test, + device_auth_lock_released_before_request_test ]. %%==================================================================== @@ -609,6 +617,97 @@ with_api_token_expired_refresh_test(_Config) -> end, ok. +with_api_token_expired_renews_test(_Config) -> + %% An API token the server rejects as expired is refreshed even though its + %% stored expiry has not passed, the way a repository token is, and the + %% request runs again with the token that came back. + Now = erlang:system_time(second), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"stale_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }} + }), + + queue_refresh_response(#{<<"access_token">> => <<"renewed_token">>}), + + Fun = fun(Cfg) -> + case maps:get(api_key, Cfg) of + <<"Bearer stale_token">> -> token_expired_response(); + ApiKey -> {ok, {200, #{}, ApiKey}} + end + end, + + ?assertEqual( + {ok, {200, #{}, <<"Bearer renewed_token">>}}, + hex_cli_auth:with_api(write, Config, Fun) + ), + ok. + +with_api_token_expired_retry_bounded_test(_Config) -> + %% A server that answers token_expired to every bearer it is sent gets a + %% bounded number of requests: the renewed token is tried once and the 401 + %% is handed back, rather than renewed and retried without end. + Now = erlang:system_time(second), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"stale_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }}, + should_authenticate => fun(_Reason) -> error(should_not_be_called) end + }), + + CallCount = counters:new(1, []), + Result = hex_cli_auth:with_api(write, Config, fun(_Cfg) -> + counters:add(CallCount, 1, 1), + token_expired_response() + end), + + ?assertMatch({ok, {401, _Headers, _Body}}, Result), + ?assertEqual(2, counters:get(CallCount, 1)), + ok. + +with_api_token_expired_reauth_retry_bounded_test(_Config) -> + %% Same bound when the renewal is a device auth: the user authenticates + %% once, the request runs again with the new token, and a second + %% token_expired is the caller's to handle rather than a second prompt. + Now = erlang:system_time(second), + PromptCount = counters:new(1, []), + CallCount = counters:new(1, []), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"initial_token">>, + %% No refresh_token, so the 401 goes straight to reauth + expires_at => Now + 3600 + }}, + should_authenticate => fun(token_refresh_failed) -> + counters:add(PromptCount, 1, 1), + true + end + }), + + queue_device_response(<<"device_token">>), + + Result = hex_cli_auth:with_api( + write, + Config, + fun(_Cfg) -> + counters:add(CallCount, 1, 1), + token_expired_response() + end, + [{oauth_open_browser, false}] + ), + + ?assertMatch({ok, {401, _Headers, _Body}}, Result), + ?assertEqual(1, counters:get(PromptCount, 1)), + ?assertEqual(2, counters:get(CallCount, 1)), + ok. + %%==================================================================== %% Test Cases - with_api reauthentication after refresh failure %%==================================================================== @@ -1068,6 +1167,95 @@ refresh_transport_error_keeps_token_test(_Config) -> end, ok. +refresh_server_error_keeps_token_test(_Config) -> + %% A 429 or a 5xx is the server having a bad minute, not a refusal of the + %% refresh token, so the stored token survives it. + Statuses = [429, 500, 502, 503], + + [ + begin + Self = self(), + Config = refresh_failure_config(Self), + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + Self ! + {hex_http_test, oauth_refresh_response, + {ok, {Status, Headers, term_to_binary(#{<<"error">> => <<"server_error">>})}}}, + + ?assertEqual( + {error, {auth_error, token_refresh_unavailable}}, + hex_cli_auth:resolve_api_auth(read, Config) + ), + + receive + cleared -> error({token_cleared_on_server_error, Status}) + after 0 -> ok + end + end + || Status <- Statuses + ], + ok. + +refresh_malformed_body_keeps_token_test(_Config) -> + %% A 200 whose body is not a token response says nothing about the refresh + %% token either, so it is not read as the server refusing it. + Bodies = [ + <<"not a map">>, + #{<<"expires_in">> => 3600}, + #{<<"access_token">> => <<"new_access_token">>}, + #{<<"access_token">> => <<"new_access_token">>, <<"expires_in">> => <<"3600">>}, + #{<<"access_token">> => 42, <<"expires_in">> => 3600} + ], + + [ + begin + Self = self(), + Config = refresh_failure_config(Self), + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + Self ! + {hex_http_test, oauth_refresh_response, {ok, {200, Headers, term_to_binary(Body)}}}, + + ?assertEqual( + {error, {auth_error, token_refresh_unavailable}}, + hex_cli_auth:resolve_api_auth(read, Config) + ), + + receive + cleared -> error({token_cleared_on_malformed_body, Body}) + after 0 -> ok + end + end + || Body <- Bodies + ], + ok. + +refresh_refusal_clears_token_test(_Config) -> + %% A 400 or a 401 is the server refusing the refresh token itself: it will + %% not work again, so the stored token is dropped. + Statuses = [400, 401], + + [ + begin + Self = self(), + Config = refresh_failure_config(Self), + Headers = #{<<"content-type">> => <<"application/vnd.hex+erlang; charset=utf-8">>}, + Self ! + {hex_http_test, oauth_refresh_response, + {ok, {Status, Headers, term_to_binary(#{<<"error">> => <<"invalid_grant">>})}}}, + + ?assertEqual( + {error, {auth_error, token_refresh_failed}}, + hex_cli_auth:resolve_api_auth(read, Config) + ), + + receive + cleared -> ok + after 100 -> error({token_not_cleared, Status}) + end + end + || Status <- Statuses + ], + ok. + %%==================================================================== %% Test Cases - Concurrency %%==================================================================== @@ -1377,6 +1565,48 @@ device_auth_concurrent_serialized_reuses_login_test(_Config) -> ets:delete(TokenStore), ok. +device_auth_lock_released_before_request_test(_Config) -> + %% The device auth lock covers acquiring the credential, not running the + %% request. A request answering 401 comes back to the same lock, and + %% global:trans/4 on a lock this process already holds does not nest: the + %% inner transaction releases it while the outer one is still running. + Self = self(), + Config = config_with_callbacks(#{ + oauth_tokens => error, + should_authenticate => fun(no_credentials) -> + Self ! {locked_during_prompt, device_auth_lock_held()}, + true + end + }), + + queue_device_response(<<"device_token">>), + + Result = hex_cli_auth:with_api( + write, + Config, + fun(Cfg) -> + Self ! {locked_during_request, device_auth_lock_held()}, + maps:get(api_key, Cfg) + end, + [{oauth_open_browser, false}] + ), + ?assertEqual(<<"Bearer device_token">>, Result), + + receive + {locked_during_prompt, LockedDuringPrompt} -> + ?assertEqual(true, LockedDuringPrompt) + after 100 -> + error(should_authenticate_not_called) + end, + + receive + {locked_during_request, LockedDuringRequest} -> + ?assertEqual(false, LockedDuringRequest) + after 100 -> + error(request_not_run) + end, + ok. + %%==================================================================== %% Helper Functions %%==================================================================== @@ -1436,6 +1666,35 @@ sso_reauth_reported_empty_test(_Config) -> end, ok. +sso_reauth_malformed_not_reported_test(_Config) -> + %% A set the server sent in a shape we cannot read is not reported at all. + %% The build tool takes the empty list for "nothing lapsed" and deletes the + %% organizations it holds, which drops the prompt the user needs. + Now = erlang:system_time(second), + Self = self(), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"expired_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now - 100 + }}, + sso_reauth => fun(Organizations) -> + Self ! {sso_reauth, Organizations}, + ok + end + }), + + queue_refresh_response(#{<<"sso_reauth_required">> => <<"acme">>}), + + {ok, _ApiKey, _AuthContext} = hex_cli_auth:resolve_api_auth(read, Config), + + receive + {sso_reauth, Organizations} -> error({sso_reauth_reported, Organizations}) + after 0 -> ok + end, + ok. + refresh_tokens_forces_a_refresh_test(_Config) -> %% A token that has not expired is still refreshed: what it carries can %% change without its lifetime running out. @@ -1513,6 +1772,46 @@ token_expired_response() -> Headers = #{<<"www-authenticate">> => <<"Bearer realm=\"hex\", error=\"token_expired\"">>}, {ok, {401, Headers, <<"">>}}. +%% @private +%% An expired global token whose refresh is about to fail, with the clear +%% callback reporting to Pid so a test can say whether the token was dropped. +refresh_failure_config(Pid) -> + Now = erlang:system_time(second), + config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"expired_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now - 100 + }}, + clear_oauth_tokens => fun() -> + Pid ! cleared, + ok + end + }). + +%% @private +%% Whether the device auth lock is held by anyone other than the process asking. +%% A concurrent caller arrives with its own pid as the lock requester id, which +%% is what makes global refuse it while another process holds the lock. +device_auth_lock_held() -> + Parent = self(), + spawn(fun() -> + Id = {{hex_cli_auth, device_auth}, self()}, + case global:set_lock(Id, [node()], 0) of + true -> + Parent ! {device_auth_lock, false}, + global:del_lock(Id, [node()]); + false -> + Parent ! {device_auth_lock, true} + end + end), + receive + {device_auth_lock, Held} -> Held + after 5000 -> + error(device_auth_lock_probe_timed_out) + end. + config_with_callbacks(Opts) -> ?CONFIG#{cli_auth_callbacks => make_callbacks(Opts)}. From c57b198371cce4c72866c570128c801c8060963c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Mon, 24 Aug 2026 18:50:09 +0200 Subject: [PATCH 5/5] Offer to authenticate when the server refuses the refresh token `with_api/4` consulted `should_authenticate` only for `{error, no_auth}`. A refused refresh resolves to `{error, {auth_error, token_refresh_failed}}`, whose only clause required `optional`, which `with_session_api` does not pass, so a caller that asked to be prompted up front got three "run mix hex.user auth" messages in one `mix deps.get` and no prompt. Only for a refused refresh, not `token_refresh_unavailable`: there the refresh got no answer at all, so the token may still be good and the network is what is wrong, and a device flow needing the same network is no help. --- src/hex_cli_auth.erl | 13 +++++++++++++ test/hex_cli_auth_SUITE.erl | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index 65a77530..12b0631b 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -277,6 +277,19 @@ with_api(Permission, BaseConfig, Fun, Opts) -> {error, {auth_error, Reason}} when Optional =:= true, ?IS_REFRESH_FAILURE(Reason) -> %% Token refresh failed but auth is optional, fall back to no credentials execute_optional_with_retry(api, BaseConfig, Fun, AuthInline, Opts); + {error, {auth_error, token_refresh_failed}} when AuthInline =:= true -> + %% The server refused the refresh token, which leaves us with no + %% usable token, same as having none, so it gets the same offer to + %% authenticate. Without this the caller that asked to be prompted up + %% front is told to run mix hex.user auth instead of being asked. + %% + %% Not token_refresh_unavailable: there the refresh got no answer at + %% all, so the token may well still be good and the network is what + %% is wrong. Offering a device flow that needs the same network is no + %% help. + maybe_authenticate_and_retry( + api, BaseConfig, Fun, token_refresh_failed, initial_retries(), Opts + ); {error, _} = Error -> Error end. diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index 07443ad2..451ce020 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -73,6 +73,7 @@ all() -> %% with_api tests - wrapper behavior with_api_optional_test, with_api_optional_token_refresh_failed_test, + with_api_refused_refresh_prompts_test, with_api_auth_inline_test, with_api_device_auth_test, @@ -898,6 +899,41 @@ with_api_optional_token_refresh_failed_test(_Config) -> ?assertEqual(undefined, Result), ok. +with_api_refused_refresh_prompts_test(_Config) -> + %% A refused refresh leaves no usable token, same as having none, so the + %% caller that asked to be prompted up front is asked rather than told to + %% run mix hex.user auth. + Now = erlang:system_time(second), + Self = self(), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"expired_token">>, + expires_at => Now - 100 + }}, + should_authenticate => fun(Reason) -> + Self ! {should_authenticate, Reason}, + false + end + }), + + Result = hex_cli_auth:with_api( + write, + Config, + fun(_) -> error(should_not_be_called) end, + [{optional, false}, {auth_inline, true}] + ), + + receive + {should_authenticate, Reason} -> + ?assertEqual(token_refresh_failed, Reason) + after 0 -> + ct:fail("should_authenticate was never called") + end, + + ?assertEqual({error, {auth_error, auth_declined}}, Result), + ok. + with_api_auth_inline_test(_Config) -> %% Test auth_inline => false returns error instead of prompting Config = config_with_callbacks(#{oauth_tokens => error}),