From eb2e4a3c11f6dbb22397bd1727817866fdf34dae Mon Sep 17 00:00:00 2001 From: phil-accelbyte <225106921+phil-accelbyte@users.noreply.github.com> Date: Tue, 12 May 2026 21:19:20 +0800 Subject: [PATCH 1/2] fix(auth): harden login self-healing across probe, refresh, and cleanup The auth login flow short-circuits when a session is already valid, refreshes when only the refresh token survives, and falls through to a full grant otherwise. Several edge cases produced confusing output or silent regressions: - Refused the session-probe short-circuit when the profile config is unreadable (e.g. `chmod 000`). Previously it reported a stale "already_authenticated" or silently fell through; now the unreadable profile is treated as no session and `auth login --format json` cleanly asks for `--base-url`. - Classified session-refresh failures from helper-observed state so a refresh that fails for transient reasons (network, server 5xx) is distinguished from permanent ones (invalid grant). The login JSON status field now matches what actually happened. - Surfaced stale-token cleanup failures instead of swallowing them, and replaced the 0-sentinel `expires_in_secs` with the real value so JSON consumers never see a phantom "0s" remaining on a valid token. - Documented the three real JSON status values in `auth login --help` (`logged_in`, `already_authenticated`, `refreshed`). - Consolidated env-guard helpers under `tests/common/env_guard.rs` and `src/support/test_helpers.rs`, and added coverage for the authorization-code refreshed JSON output path. --- src/frontend/human/commands/auth.rs | 19 +- src/frontend/json/commands/auth.rs | 5 +- src/invocation/builder.rs | 5 +- src/invocation/commands/auth/mod.rs | 75 ++- src/protocol/output_views.rs | 2 + src/runtime/auth/operations.rs | 607 ++++++++++++++++++-- src/runtime/auth/session.rs | 706 ++++++++++++++++++++---- src/runtime/facade/auth.rs | 41 +- src/runtime/mod.rs | 26 +- src/support/mod.rs | 3 + src/support/test_helpers.rs | 51 ++ tests/common/env_guard.rs | 50 ++ tests/common/mod.rs | 2 + tests/functional/auth/login.rs | 677 +++++++++++++++++++++++ tests/integration/auth.rs | 121 ++++ tests/integration/config.rs | 22 +- tests/integration/namespace.rs | 29 +- tests/integration/profile.rs | 28 +- tests/integration/token_refresh_race.rs | 37 +- 19 files changed, 2209 insertions(+), 297 deletions(-) create mode 100644 src/support/test_helpers.rs create mode 100644 tests/common/env_guard.rs diff --git a/src/frontend/human/commands/auth.rs b/src/frontend/human/commands/auth.rs index 35b7b82..ce695a0 100644 --- a/src/frontend/human/commands/auth.rs +++ b/src/frontend/human/commands/auth.rs @@ -7,7 +7,8 @@ use crate::frontend::style; use crate::frontend::RenderOptions; use crate::frontend::RenderedOutput; use crate::protocol::output::{ - AuthActionData, AuthOutput, AuthStatusData, AuthView, LogoutData, Presence, TokenState, + AuthActionData, AuthActionStatus, AuthOutput, AuthStatusData, AuthView, LogoutData, Presence, + TokenState, }; /// Render auth command output as human-readable text @@ -37,10 +38,18 @@ fn render_auth_view_text(view: &AuthView) -> (String, Option) { ), Some(render_attention_details(data)), ), - AuthView::LoginSuccess(data) => ( - style::success("Authenticated", style::is_stdout_enabled()), - Some(render_login_success_details(data)), - ), + AuthView::LoginSuccess(data) => { + let headline_text = match data.status { + AuthActionStatus::LoggedIn | AuthActionStatus::AlreadyAuthenticated => { + "Authenticated" + } + AuthActionStatus::Refreshed => "Session refreshed", + }; + ( + style::success(headline_text, style::is_stdout_enabled()), + Some(render_login_success_details(data)), + ) + } AuthView::LogoutSuccess(data) => ( style::success("Credentials cleared", style::is_stdout_enabled()), Some(render_logout_details(data)), diff --git a/src/frontend/json/commands/auth.rs b/src/frontend/json/commands/auth.rs index 1d45225..2842b63 100644 --- a/src/frontend/json/commands/auth.rs +++ b/src/frontend/json/commands/auth.rs @@ -46,8 +46,9 @@ fn render_auth_view_json(view: &AuthView) -> Result { } AuthView::LoginSuccess(data) => { let status = match data.status { - AuthActionStatus::LoggedIn => "logged in", - AuthActionStatus::AlreadyAuthenticated => "already authenticated", + AuthActionStatus::LoggedIn => "logged_in", + AuthActionStatus::AlreadyAuthenticated => "already_authenticated", + AuthActionStatus::Refreshed => "refreshed", }; serde_json::json!({ "status": status }) } diff --git a/src/invocation/builder.rs b/src/invocation/builder.rs index 0820d88..72396ac 100644 --- a/src/invocation/builder.rs +++ b/src/invocation/builder.rs @@ -469,7 +469,10 @@ fn build_login_subcommand() -> Command { \x20 Base URL: --base-url → AGS_BASE_URL → config → prompt\n\ \x20 Client ID: --client-id → AGS_CLIENT_ID → config → prompt\n\ \x20 Client Secret: --client-secret → AGS_CLIENT_SECRET → keychain → prompt\n\n\ - \x20 With --format json, outputs {\"status\": \"authenticated\"} on success.", + \x20 With --format json, outputs one of:\n\ + \x20 {\"status\": \"logged_in\"} — fresh successful grant\n\ + \x20 {\"status\": \"already_authenticated\"} — probe found a valid session\n\ + \x20 {\"status\": \"refreshed\"} — probe refreshed an expired token", ) .arg( Arg::new("grant") diff --git a/src/invocation/commands/auth/mod.rs b/src/invocation/commands/auth/mod.rs index 735831c..27c05ec 100644 --- a/src/invocation/commands/auth/mod.rs +++ b/src/invocation/commands/auth/mod.rs @@ -86,19 +86,6 @@ async fn handle_auth_login( handle_login_with_client_credentials(matches, flags, profile, runtime, frontend).await } GrantType::AuthorizationCode => { - // Authorization code login opens a browser and prints instructions - // to stderr. That is incompatible with any non-interactive mode: - // --no-input, or --format json which is inherently machine-driven. - if flags.is_no_input - || flags.format == Some(crate::protocol::request::OutputFormat::Json) - { - return Err(CliError::Usage { - message: "Authorization code flow requires browser interaction and cannot run non-interactively.\n\ - Use '--grant client-credentials' for headless authentication." - .to_string(), - metadata: None, - }); - } handle_login_with_browser(matches, flags, profile, runtime, frontend).await } } @@ -111,7 +98,7 @@ async fn handle_auth_login( /// the token exchange + persistence to `operations::login_with_authorization_code`. async fn handle_login_with_browser( matches: &ArgMatches, - _flags: &GlobalFlags, + flags: &GlobalFlags, profile: &str, runtime: &crate::runtime::Runtime, frontend: &mut dyn crate::frontend::Frontend, @@ -123,24 +110,57 @@ async fn handle_login_with_browser( .and_then(|p| p.parse::().ok()) .unwrap_or(8080); + // Non-interactive modes can't drive the browser flow, but they CAN still + // benefit from the probe (a valid stored session means no browser is + // needed). So we resolve identity → probe → reject only if both fail. + let is_prompt_blocked = + flags.is_no_input || flags.format == Some(crate::protocol::request::OutputFormat::Json); + let base_url = resolve_login_value( flag_base_url, crate::runtime::auth::credentials::resolve_base_url_value(profile), - false, + is_prompt_blocked, "Enter Base URL (e.g. https://demo.accelbyte.io): ", "Base URL is required.", - "When reading from stdin, --base-url or AGS_BASE_URL must be provided.", + "Provide --base-url or set AGS_BASE_URL when running non-interactively.", )?; let client_id = resolve_login_value( flag_client_id, crate::runtime::auth::credentials::resolve_client_id_value(profile), - false, + is_prompt_blocked, "Enter Client ID: ", "Client ID is required.", - "When reading from stdin, --client-id or AGS_CLIENT_ID must be provided.", + "Provide --client-id or set AGS_CLIENT_ID when running non-interactively.", )?; + // Probe BEFORE binding the callback port or printing any browser URL. + // If the user already has a usable (or refreshable) session, we report + // that and stop here — no browser tab, no callback wait. + if let Some(view) = runtime + .auth_probe_existing_session( + profile, + base_url.clone(), + client_id.clone(), + "authorization code", + frontend.progress_sink(), + ) + .await? + { + return Ok(CommandOutput::Auth(AuthOutput { view })); + } + + // Probe didn't short-circuit and the browser flow needs a TTY. Reject + // cleanly instead of opening a browser the caller can't drive. + if is_prompt_blocked { + return Err(CliError::Usage { + message: "Authorization code flow requires browser interaction and cannot run non-interactively.\n\ + Use '--grant client-credentials' for headless authentication." + .to_string(), + metadata: None, + }); + } + // Generate PKCE pair and state let (code_verifier, code_challenge) = oauth::generate_pkce_pair(); let state = oauth::generate_state(); @@ -203,7 +223,9 @@ async fn handle_login_with_client_credentials( ); } - let is_prompt_blocked = flags.is_no_input || flag_client_secret_stdin; + let is_prompt_blocked = flags.is_no_input + || flag_client_secret_stdin + || flags.format == Some(crate::protocol::request::OutputFormat::Json); let base_url = resolve_login_value( flag_base_url, @@ -223,6 +245,21 @@ async fn handle_login_with_client_credentials( "Provide --client-id or set AGS_CLIENT_ID when using --no-input.", )?; + // Probe BEFORE prompting for the client secret. If the existing session + // is good (or can be refreshed), we don't need the secret at all. + if let Some(view) = runtime + .auth_probe_existing_session( + profile, + base_url.clone(), + client_id.clone(), + "client credentials", + frontend.progress_sink(), + ) + .await? + { + return Ok(CommandOutput::Auth(AuthOutput { view })); + } + let client_secret = resolve_client_secret_for_login( profile, flag_client_secret, diff --git a/src/protocol/output_views.rs b/src/protocol/output_views.rs index 564ba59..cb87b6e 100644 --- a/src/protocol/output_views.rs +++ b/src/protocol/output_views.rs @@ -147,6 +147,8 @@ pub enum AuthActionStatus { LoggedIn, /// Login was a no-op because valid credentials already exist. AlreadyAuthenticated, + /// Login refreshed a stale session in place; no fresh OAuth ran. + Refreshed, } /// Data from a successful login action for the confirmation display. diff --git a/src/runtime/auth/operations.rs b/src/runtime/auth/operations.rs index 8088875..a943e9d 100644 --- a/src/runtime/auth/operations.rs +++ b/src/runtime/auth/operations.rs @@ -57,9 +57,10 @@ pub struct LoginOutcome { /// Friendly login-type label, e.g. `"client credentials"` or /// `"authorization code"`. Used by the renderer to display the login type. pub login_type: &'static str, - /// Token lifetime in seconds. `0` if the user was already authenticated - /// (no new token was fetched). - pub expires_in_secs: u64, + /// Token lifetime in seconds for `LoggedIn` and `Refreshed`. `None` when + /// no new token was fetched (`AlreadyAuthenticated`), since the stored + /// token's remaining lifetime is reported through the `tip` instead. + pub expires_in_secs: Option, } /// What the login function actually did. @@ -70,6 +71,9 @@ pub enum LoginOutcomeKind { /// A valid (or refreshable) session already existed; no work was done. /// `tip` is the message to surface to the user. AlreadyAuthenticated { tip: String }, + /// The stored access token was stale and was refreshed in place — no + /// fresh OAuth flow was run. + Refreshed, } /// Result of clearing credentials for a single profile. @@ -137,8 +141,9 @@ pub enum RefreshTokenState { Missing, } -/// Perform a client-credentials grant login. Returns an `AlreadyAuthenticated` -/// outcome (no work done) if a valid or refreshable session already exists. +/// Perform a client-credentials grant login. The caller is responsible for +/// calling [`probe_existing_session`] first so we don't issue a redundant +/// grant when a usable session already exists. pub async fn login_with_client_credentials( client: &Client, request: ClientCredentialsLogin, @@ -147,16 +152,6 @@ pub async fn login_with_client_credentials( use crate::protocol::event::ProgressEvent; use crate::support::unix_now; - if let Some(tip) = check_already_authenticated(&request.profile) { - return Ok(LoginOutcome { - kind: LoginOutcomeKind::AlreadyAuthenticated { tip }, - base_url: request.base_url, - client_id: request.client_id, - login_type: "client credentials", - expires_in_secs: 0, - }); - } - sink.on_event(ProgressEvent::Started { message: "Authenticating (client credentials)...".to_string(), }); @@ -198,12 +193,14 @@ pub async fn login_with_client_credentials( base_url: request.base_url, client_id: request.client_id, login_type: "client credentials", - expires_in_secs: token_result.expires_in, + expires_in_secs: Some(token_result.expires_in), }) } /// Complete an authorization-code grant login. The caller is responsible for -/// obtaining the `code` and `code_verifier` from the OAuth callback server. +/// (a) obtaining the `code` and `code_verifier` from the OAuth callback +/// server and (b) calling [`probe_existing_session`] first so we don't run +/// the browser flow when a refreshable session already exists. pub async fn login_with_authorization_code( client: &Client, request: AuthorizationCodeLogin, @@ -212,16 +209,6 @@ pub async fn login_with_authorization_code( use crate::protocol::event::ProgressEvent; use crate::support::unix_now; - if let Some(tip) = check_already_authenticated(&request.profile) { - return Ok(LoginOutcome { - kind: LoginOutcomeKind::AlreadyAuthenticated { tip }, - base_url: request.base_url, - client_id: request.client_id, - login_type: "authorization code", - expires_in_secs: 0, - }); - } - sink.on_event(ProgressEvent::Started { message: "Authenticating (authorization code)...".to_string(), }); @@ -265,7 +252,7 @@ pub async fn login_with_authorization_code( base_url: request.base_url, client_id: request.client_id, login_type: "authorization code", - expires_in_secs: token_result.expires_in, + expires_in_secs: Some(token_result.expires_in), }) } @@ -418,10 +405,46 @@ fn refresh_token_state( // ── Internal helpers ── +/// True if the profile's stored base URL and client ID match the requested +/// values, or if either side is absent (no stored value to compare against). +/// A mismatch means the stored token belongs to a different identity than +/// the caller is asking to log into, so the probe must not short-circuit. +/// +/// If the profile config cannot be loaded (corruption, permissions), we +/// cannot verify identity, so refuse to short-circuit — the stored token +/// may have been minted for a different base URL or client ID, and using +/// it against the caller's requested target could leak a dev token to +/// prod (or vice versa). +fn stored_identity_matches( + profile: &str, + requested_base_url: &str, + requested_client_id: &str, +) -> bool { + use crate::runtime::config::ProfileConfig; + + let Ok(config) = ProfileConfig::load(profile) else { + return false; + }; + if let Some(stored) = config.base_url.as_deref() { + if stored != requested_base_url { + return false; + } + } + if let Some(stored) = config.client_id.as_deref() { + if stored != requested_client_id { + return false; + } + } + true +} + /// Inspect stored token data and return a "you are already authenticated" tip -/// if a usable session exists. Returns `None` if the user should proceed with -/// a fresh login. -fn check_already_authenticated(profile: &str) -> Option { +/// if the access token is comfortably within its expiry window. Returns `None` +/// when the caller should probe for a refreshable session or proceed with a +/// fresh login — the refresh-token branch that previously lived in +/// `check_already_authenticated` moved to `session::try_refresh_stored_session`, +/// invoked via `probe_existing_session`. +fn existing_access_token_still_valid(profile: &str) -> Option { use crate::support::unix_now; let token_data = store::get_token_data(profile).ok()??; @@ -435,19 +458,79 @@ fn check_already_authenticated(profile: &str) -> Option { )); } - let has_refresh = token_data.refresh_token.is_some() - && token_data - .refresh_expires_at - .map(|exp| now < exp) - .unwrap_or(true); + None +} + +/// Probe whether the profile already has a usable session, refreshing in +/// place if the access token is stale but the refresh token still works. +/// +/// Returns: +/// - `Ok(Some(AlreadyAuthenticated))` — access token still comfortably valid; +/// caller should not start a fresh flow. +/// - `Ok(Some(Refreshed))` — access token was stale but the refresh token +/// was successfully used to mint a fresh one; new token already persisted; +/// caller should not start a fresh flow. +/// - `Ok(None)` — caller must proceed with a fresh OAuth or grant flow. Any +/// stale stored token has already been cleared via best-effort delete; an +/// informational `ProgressEvent::Message` was emitted to the sink if +/// `None` is due to a server-side refresh rejection. +/// - `Err(_)` — transport-level failure during the probe (e.g. network down); +/// caller should propagate the error and abort. +pub async fn probe_existing_session( + client: &Client, + profile: &str, + base_url: String, + client_id: String, + login_type: &'static str, + sink: &mut dyn ProgressSink, +) -> Result, RuntimeError> { + use crate::protocol::event::ProgressEvent; - if has_refresh { - return Some( - "Session is active (token will auto-refresh on next API call). Run 'ags auth logout' first to re-authenticate.".to_string() - ); + // The stored token was minted for the profile's stored identity. If the + // caller is asking to log into a different base URL or client ID, the + // stored session isn't a match — fall through to a fresh flow. + if !stored_identity_matches(profile, &base_url, &client_id) { + return Ok(None); } - None + if let Some(tip) = existing_access_token_still_valid(profile) { + return Ok(Some(LoginOutcome { + kind: LoginOutcomeKind::AlreadyAuthenticated { tip }, + base_url, + client_id, + login_type, + expires_in_secs: None, + })); + } + + match session::try_refresh_stored_session(client, profile).await? { + session::RefreshOutcome::Refreshed { + expires_in_secs, .. + } => Ok(Some(LoginOutcome { + kind: LoginOutcomeKind::Refreshed, + base_url, + client_id, + login_type, + expires_in_secs: Some(expires_in_secs), + })), + session::RefreshOutcome::Rejected { .. } => { + sink.on_event(ProgressEvent::Message { + text: "Existing session was invalid — starting fresh login...".to_string(), + }); + // Best-effort clear. If this fails (keychain locked, read-only + // config dir) the rejected token will keep coming back on every + // invocation, so surface the failure rather than swallowing it — + // the next login flow will still proceed and overwrite the + // stored token on success. + if let Err(error) = store::delete_token_data_async(profile).await { + sink.on_event(ProgressEvent::Message { + text: format!("Warning: could not clear stale session data: {error}"), + }); + } + Ok(None) + } + session::RefreshOutcome::Unavailable { .. } => Ok(None), + } } /// Persist a successful login: write token data, save base URL + client ID, @@ -570,3 +653,445 @@ mod tests { std::env::remove_var("AGS_NO_KEYCHAIN"); } } + +#[cfg(test)] +mod probe_tests { + use super::*; + use crate::protocol::event::{ProgressEvent, ProgressSink}; + use crate::runtime::auth::store::TokenData; + use crate::runtime::config::ProfileConfig; + use crate::support::test_helpers::{now_secs, TempEnvGuard}; + use wiremock::matchers::{body_string_contains, method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + /// Test sink that captures every emitted progress message for assertions. + #[derive(Default)] + struct CapturingSink { + messages: Vec, + } + + impl ProgressSink for CapturingSink { + /// Capture message events; drop other events. + fn on_event(&mut self, event: ProgressEvent) { + if let ProgressEvent::Message { text } = event { + self.messages.push(text); + } + } + } + + /// A fresh stored access token shortcuts straight to AlreadyAuthenticated + /// with no network call. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_already_authenticated_when_access_fresh() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let client = reqwest::Client::new(); + let mut sink = CapturingSink::default(); + let outcome = probe_existing_session( + &client, + profile, + "https://unused.invalid".to_string(), + "cid".to_string(), + "authorization code", + &mut sink, + ) + .await + .unwrap(); + + match outcome { + Some(LoginOutcome { + kind: LoginOutcomeKind::AlreadyAuthenticated { tip }, + .. + }) => { + assert!(tip.contains("Already authenticated")); + } + other => panic!("expected AlreadyAuthenticated, got {other:?}"), + } + assert!( + sink.messages.is_empty(), + "no progress messages expected, got {:?}", + sink.messages + ); + } + + /// Successful refresh: probe returns Refreshed and the stored token is + /// updated. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_refreshed_when_refresh_succeeds() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(200).set_body_string( + r#"{"access_token":"new-access","expires_in":3600,"token_type":"Bearer","refresh_token":"rotated","refresh_expires_in":7200}"#, + )) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("valid-rt".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let client = reqwest::Client::new(); + let mut sink = CapturingSink::default(); + let outcome = probe_existing_session( + &client, + profile, + server.uri(), + "cid".to_string(), + "authorization code", + &mut sink, + ) + .await + .unwrap(); + + assert!( + matches!( + outcome, + Some(LoginOutcome { + kind: LoginOutcomeKind::Refreshed, + .. + }) + ), + "expected Refreshed, got {outcome:?}" + ); + let stored = store::get_token_data(profile).unwrap().unwrap(); + assert_eq!(stored.access_token, "new-access"); + assert!( + sink.messages.is_empty(), + "no probe-failure message expected on success, got {:?}", + sink.messages + ); + } + + /// Rejection path: probe returns None, emits the user-facing message, + /// and deletes the stale stored token data. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_none_and_clears_state_on_rejection() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(401).set_body_string( + r#"{"error":"invalid_grant","error_description":"refresh expired"}"#, + )) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("dead-rt".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let client = reqwest::Client::new(); + let mut sink = CapturingSink::default(); + let outcome = probe_existing_session( + &client, + profile, + server.uri(), + "cid".to_string(), + "authorization code", + &mut sink, + ) + .await + .unwrap(); + + assert!(outcome.is_none(), "expected None, got {outcome:?}"); + assert!( + store::get_token_data(profile).unwrap().is_none(), + "stale token data must be cleared on probe rejection" + ); + assert!( + sink.messages + .iter() + .any(|m| m.contains("Existing session was invalid")), + "expected probe-failure message, got {:?}", + sink.messages + ); + } + + /// Unavailable path (no refresh token): probe returns None, no message + /// is emitted, and any stored token is left alone (it's already useless + /// but we don't proactively clear). + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_none_silently_when_unavailable() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: None, + refresh_expires_at: None, + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let client = reqwest::Client::new(); + let mut sink = CapturingSink::default(); + let outcome = probe_existing_session( + &client, + profile, + "https://unused.invalid".to_string(), + "cid".to_string(), + "authorization code", + &mut sink, + ) + .await + .unwrap(); + + assert!(outcome.is_none(), "expected None, got {outcome:?}"); + assert!( + sink.messages.is_empty(), + "no message expected for Unavailable, got {:?}", + sink.messages + ); + } + + /// Probe rejection must not delete the profile's saved base_url / + /// client_id — only the token data. Otherwise users would have to + /// reconfigure after every server-side session expiry. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_rejection_preserves_profile_config() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .respond_with( + ResponseTemplate::new(401).set_body_string(r#"{"error":"invalid_grant"}"#), + ) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + namespace: Some("preserved-ns".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("dead-rt".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let client = reqwest::Client::new(); + let mut sink = CapturingSink::default(); + let _ = probe_existing_session( + &client, + profile, + server.uri(), + "cid".to_string(), + "authorization code", + &mut sink, + ) + .await + .unwrap(); + + let cfg = ProfileConfig::load(profile).unwrap(); + assert_eq!(cfg.base_url.as_deref(), Some(server.uri().as_str())); + assert_eq!(cfg.client_id.as_deref(), Some("cid")); + assert_eq!(cfg.namespace.as_deref(), Some("preserved-ns")); + } + + /// When the caller asks to log into a base URL that differs from the + /// stored one, the probe must NOT short-circuit — the stored token + /// belongs to a different identity. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_none_when_base_url_differs_from_stored() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://dev.example.com".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let client = reqwest::Client::new(); + let mut sink = CapturingSink::default(); + let outcome = probe_existing_session( + &client, + profile, + "https://demo.example.com".to_string(), + "cid".to_string(), + "authorization code", + &mut sink, + ) + .await + .unwrap(); + + assert!( + outcome.is_none(), + "expected None when base_url differs, got {outcome:?}" + ); + } + + /// When the caller asks to log in with a client ID that differs from + /// the stored one, the probe must NOT short-circuit. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_none_when_client_id_differs_from_stored() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://dev.example.com".to_string()), + client_id: Some("original-cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let client = reqwest::Client::new(); + let mut sink = CapturingSink::default(); + let outcome = probe_existing_session( + &client, + profile, + "https://dev.example.com".to_string(), + "different-cid".to_string(), + "authorization code", + &mut sink, + ) + .await + .unwrap(); + + assert!( + outcome.is_none(), + "expected None when client_id differs, got {outcome:?}" + ); + } +} diff --git a/src/runtime/auth/session.rs b/src/runtime/auth/session.rs index 2c177df..aa7a1fd 100644 --- a/src/runtime/auth/session.rs +++ b/src/runtime/auth/session.rs @@ -67,6 +67,205 @@ pub struct TokenResolution { pub warnings: Vec, } +/// Why a refresh attempt could not even be made. +/// +/// The caller uses this to choose the correct error to surface for +/// authorization-code profiles, without having to re-read storage and risk a +/// TOCTOU race with concurrent CLI processes. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum UnavailableReason { + /// No stored token at all — the user has never logged in for this + /// profile (or has logged out). + NoStoredToken, + /// A token is stored but it has no refresh token (e.g. minted via + /// client-credentials grant). + NoRefreshToken, + /// A refresh token exists but its local expiry has already passed. + RefreshTokenExpired, +} + +/// Outcome of an attempt to refresh a stored session in place. +/// +/// Distinguishes server-side rejection (recoverable by a fresh OAuth flow) +/// from transport errors (caller should propagate as failure). +#[derive(Debug)] +pub(crate) enum RefreshOutcome { + /// A usable token is in place — either freshly fetched from the + /// refresh endpoint or the stored access token was still valid after + /// re-checking under the refresh lock. + Refreshed { + token: String, + source: TokenSource, + expires_in_secs: u64, + warnings: Vec, + }, + /// No refresh attempt is possible. The reason is decided from the + /// stored token state observed during this call, so callers do not + /// need to re-read storage. + Unavailable { reason: UnavailableReason }, + /// The server rejected the refresh. Carries the human-readable + /// message for diagnostics; the caller decides how to react. + Rejected { message: String }, +} + +/// Probe whether the stored session for `profile` can be refreshed in place. +/// +/// Sequence: +/// 1. Fast-path read of stored token — if access is still fresh, return +/// `Refreshed` reusing that token. +/// 2. If no refresh token (or it's locally expired), return `Unavailable`. +/// 3. Acquire in-process refresh lock + cross-process file lock. +/// 4. Re-read stored token under locks (double-check: another caller may +/// have just refreshed). If now fresh, return `Refreshed`. +/// 5. Call the refresh-token endpoint. +/// - Success: persist new token, return `Refreshed`. +/// - HTTP rejection (non-2xx): return `Rejected`. +/// - Transport error: return `Err`. +/// +/// The locking and double-check pattern mirror `resolve_access_token` — this +/// helper is the shared core; the resolver and login flows both call it. +pub(crate) async fn try_refresh_stored_session( + client: &Client, + profile: &str, +) -> Result { + // Fast path: read stored token without locks. + let stored = store::get_token_data_async(profile).await.ok().flatten(); + let now = unix_now(); + let Some(token_data) = stored else { + return Ok(RefreshOutcome::Unavailable { + reason: UnavailableReason::NoStoredToken, + }); + }; + + if let Some(outcome) = refreshed_from_fresh_stored(&token_data, now) { + return Ok(outcome); + } + + // Determine refreshability locally before paying for any locks. + let Some(refresh_token_value) = token_data.refresh_token.clone() else { + return Ok(RefreshOutcome::Unavailable { + reason: UnavailableReason::NoRefreshToken, + }); + }; + let is_refresh_locally_valid = token_data + .refresh_expires_at + .map(|exp| now < exp) + .unwrap_or(true); + if !is_refresh_locally_valid { + return Ok(RefreshOutcome::Unavailable { + reason: UnavailableReason::RefreshTokenExpired, + }); + } + + // Slow path: acquire locks before the network call. + let refresh_mutex = profile_refresh_lock(profile); + let _refresh_guard = refresh_mutex.lock().await; + let _lock = locking::acquire_async_token_lock(profile).await?; + + // Re-read after both locks are held. If the token has vanished between + // the unlocked read and now (another process logged out), treat it the + // same as never having had one. + let stored = store::get_token_data_async(profile).await.ok().flatten(); + let Some(token_data) = stored else { + return Ok(RefreshOutcome::Unavailable { + reason: UnavailableReason::NoStoredToken, + }); + }; + let now = unix_now(); + if let Some(outcome) = refreshed_from_fresh_stored(&token_data, now) { + return Ok(outcome); + } + + let credentials = credentials::resolve_credentials(profile); + let base_url = credentials + .base_url + .as_ref() + .ok_or_else(|| RuntimeError::from(AuthError::BaseUrlMissing))?; + let client_id = credentials + .client_id + .as_ref() + .ok_or_else(|| RuntimeError::from(AuthError::ClientIdMissing))?; + + let fetch_result = tokens::fetch_refresh_token( + client, + base_url, + client_id, + credentials.client_secret.as_deref(), + &refresh_token_value, + ) + .await; + + let mut result = match fetch_result { + Ok(result) => result, + Err(error) => { + // Network/transport failures should abort. Anything else (HTTP + // 4xx/5xx mapped through AuthError::TokenRefreshFailed) is a + // server-side rejection and the caller decides how to react. + return classify_refresh_error(error); + } + }; + + let expires_in_warning = result.expires_in_warning.take(); + let new_token_data = tokens::token_result_to_token_data( + &result, + token_data + .grant_type + .unwrap_or(crate::protocol::request::GrantType::AuthorizationCode), + now, + ); + // `_lock` (acquired above) must still be in scope across this await — + // `store_token_data_unlocked_async` writes without re-acquiring the + // cross-process file lock, relying on the caller's lock to guard the + // write. Do not refactor the lock guards out of this function without + // moving the write with them. + let outcome = store::store_token_data_unlocked_async(profile, new_token_data).await?; + + Ok(RefreshOutcome::Refreshed { + token: result.access_token.clone(), + source: TokenSource::Refreshed, + expires_in_secs: result.expires_in, + warnings: merge_warnings(expires_in_warning, outcome.warning), + }) +} + +/// Map a RuntimeError from `fetch_refresh_token` into either a transport +/// failure (caller propagates) or a server-side rejection (Ok(Rejected)). +/// +/// Transport errors arrive as `RuntimeErrorKind::Network`. Everything else +/// from the auth-token endpoint is `NotAuthenticated` (the +/// `AuthError::TokenRefreshFailed` conversion), which we treat as rejection. +fn classify_refresh_error(error: RuntimeError) -> Result { + use crate::protocol::error::RuntimeErrorKind; + if matches!(error.kind, RuntimeErrorKind::Network) { + Err(error) + } else { + Ok(RefreshOutcome::Rejected { + message: error.message, + }) + } +} + +/// If `token_data` is still comfortably within its expiry window, return +/// the `RefreshOutcome::Refreshed` that reuses it; otherwise `None`. +/// +/// Called twice in `try_refresh_stored_session` — once unlocked and once +/// after both locks are held — so the double-check cannot drift. +fn refreshed_from_fresh_stored( + token_data: &crate::runtime::auth::store::TokenData, + now: u64, +) -> Option { + if now + TOKEN_EXPIRY_BUFFER_SECS < token_data.expires_at { + Some(RefreshOutcome::Refreshed { + token: token_data.access_token.clone(), + source: TokenSource::Stored, + expires_in_secs: token_data.expires_at.saturating_sub(now), + warnings: vec![], + }) + } else { + None + } +} + /// Resolve an access token by trying each source in priority order: /// environment variable → stored token → refresh token → client credentials grant. pub async fn resolve_access_token( @@ -82,122 +281,76 @@ pub async fn resolve_access_token( }); } - let stored = store::get_token_data_async(profile).await.ok().flatten(); - if let Some(ref token_data) = stored { - let now = unix_now(); - if now + TOKEN_EXPIRY_BUFFER_SECS < token_data.expires_at { - return Ok(TokenResolution { - token: token_data.access_token.clone(), - source: TokenSource::Stored, - expires_in_secs: Some(token_data.expires_at.saturating_sub(now)), - warnings: vec![], - }); - } - } - - // Slow path: serialize refresh work across concurrent tasks in this - // process before taking the cross-process file lock below. - let refresh_mutex = profile_refresh_lock(profile); - let _refresh_guard = refresh_mutex.lock().await; - // Cross-process lock: prevents a second CLI process from refreshing or - // replacing the stored token at the same time. - let _lock = locking::acquire_async_token_lock(profile).await?; + // Probe stored session: fast path for fresh tokens, refresh attempt + // for stale ones with a refresh token. + let stored_grant_type = store::get_token_data_async(profile) + .await + .ok() + .flatten() + .and_then(|t| t.grant_type); + let is_authorization_code = matches!( + stored_grant_type, + None | Some(crate::protocol::request::GrantType::AuthorizationCode) + ); - // Re-read after both locks are held. Another task or process may have - // refreshed the token while we were waiting, in which case we can skip - // the network call entirely. - let stored = store::get_token_data_async(profile).await.ok().flatten(); - if let Some(ref token_data) = stored { - let now = unix_now(); - if now + TOKEN_EXPIRY_BUFFER_SECS < token_data.expires_at { + match try_refresh_stored_session(client, profile).await? { + RefreshOutcome::Refreshed { + token, + source, + expires_in_secs, + warnings, + } => { return Ok(TokenResolution { - token: token_data.access_token.clone(), - source: TokenSource::Stored, - expires_in_secs: Some(token_data.expires_at.saturating_sub(now)), - warnings: vec![], + token, + source, + expires_in_secs: Some(expires_in_secs), + warnings, }); } - } - - let credentials = credentials::resolve_credentials(profile); - - if let Some(token_data) = stored { - let now = unix_now(); - // Treat tokens stored without a recorded `grant_type` as authorization- - // code tokens. Older CLI versions persisted tokens before this field - // existed, and a legacy refresh-failure must surface as "Session - // expired" rather than silently falling through to client credentials - // (which then errors with the misleading "Not authenticated"). - let is_authorization_code = matches!( - token_data.grant_type, - None | Some(crate::protocol::request::GrantType::AuthorizationCode) - ); - - if let Some(ref refresh_token) = token_data.refresh_token { - let is_refresh_valid = token_data - .refresh_expires_at - .map(|exp| now < exp) - .unwrap_or(true); - - if is_refresh_valid { - let base_url = credentials - .base_url - .as_ref() - .ok_or_else(|| RuntimeError::from(AuthError::BaseUrlMissing))?; - let client_id = credentials - .client_id - .as_ref() - .ok_or_else(|| RuntimeError::from(AuthError::ClientIdMissing))?; - - match tokens::fetch_refresh_token( - client, - base_url, - client_id, - credentials.client_secret.as_deref(), - refresh_token, - ) - .await - { - Ok(mut result) => { - let expires_in_warning = result.expires_in_warning.take(); - let new_token_data = tokens::token_result_to_token_data( - &result, - token_data - .grant_type - .unwrap_or(crate::protocol::request::GrantType::AuthorizationCode), - now, - ); - // `_lock` must remain in scope until the write future resolves — - // it guards the cross-process critical section across this await. - let outcome = - store::store_token_data_unlocked_async(profile, new_token_data).await?; - return Ok(TokenResolution { - token: result.access_token.clone(), - source: TokenSource::Refreshed, - expires_in_secs: Some(result.expires_in), - warnings: merge_warnings(expires_in_warning, outcome.warning), - }); + RefreshOutcome::Rejected { message } => { + if is_authorization_code { + return Err(RuntimeError::from(AuthError::SessionExpiredRefreshFailed( + message, + ))); + } + // Confidential clients fall through to a fresh client-credentials grant. + } + RefreshOutcome::Unavailable { reason } => { + // For authorization-code profiles with an existing stored token + // that can't be refreshed, surface a precise session-expiry + // error using the reason the refresh helper already observed — + // avoids a second storage read and the TOCTOU window it would + // introduce. `NoStoredToken` is left to fall through so callers + // configured via `AGS_CLIENT_ID`/`AGS_CLIENT_SECRET` env vars + // can still obtain a token via the client-credentials grant + // below; if those credentials are also missing the grant's own + // missing-credentials error surfaces with the right suggestion. + // Confidential clients always fall through. + if is_authorization_code { + match reason { + UnavailableReason::NoStoredToken => {} + UnavailableReason::NoRefreshToken => { + return Err(RuntimeError::from(AuthError::SessionExpiredNoRefreshToken)); } - Err(error) => { - if is_authorization_code { - return Err(RuntimeError::from( - AuthError::SessionExpiredRefreshFailed(error.to_string()), - )); - } + UnavailableReason::RefreshTokenExpired => { + return Err(RuntimeError::from( + AuthError::SessionExpiredRefreshTokenExpired, + )); } } - } else if is_authorization_code { - return Err(RuntimeError::from( - AuthError::SessionExpiredRefreshTokenExpired, - )); } - } else if is_authorization_code { - return Err(RuntimeError::from(AuthError::SessionExpiredNoRefreshToken)); } } - // Last resort: if no usable stored token remains, fall back to client - // credentials using the resolved profile credentials. + // Last resort: client-credentials grant. Reached only for confidential + // clients where stored state was insufficient or refresh was rejected. + // + // `try_refresh_stored_session` already acquired and released the + // per-profile mutex and file lock above. We re-acquire them here to + // guard the `store_token_data_unlocked_async` write below — the two + // critical sections are independent (no usable stored token survives + // the gap), so the brief lock release between them is intentional. + let credentials = credentials::resolve_credentials(profile); let base_url = credentials .base_url .clone() @@ -211,6 +364,10 @@ pub async fn resolve_access_token( .clone() .ok_or_else(|| RuntimeError::from(AuthError::ClientSecretMissing))?; + let refresh_mutex = profile_refresh_lock(profile); + let _refresh_guard = refresh_mutex.lock().await; + let _lock = locking::acquire_async_token_lock(profile).await?; + let mut result = tokens::fetch_client_credentials_token(client, &base_url, &client_id, &client_secret) .await?; @@ -267,3 +424,342 @@ mod refresh_lock_registry_tests { ); } } + +#[cfg(test)] +mod try_refresh_tests { + use super::*; + use crate::runtime::auth::store::{self, TokenData}; + use crate::runtime::config::ProfileConfig; + use crate::support::test_helpers::{now_secs, TempEnvGuard}; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + /// Standard test setup: creates an isolated `AGS_HOME`, disables the OS + /// keychain, saves a `ProfileConfig` pointing at `base_url`, and persists + /// `token` to the "default" profile. Returns the two env guards (must be + /// kept alive for the duration of the test) and the tempdir. + /// + /// The token is stored with `grant_type` defaulted to `AuthorizationCode` + /// when callers pass `None`, matching the rest of the test suite's fixtures. + fn setup_profile( + base_url: &str, + token: TokenData, + ) -> (tempfile::TempDir, TempEnvGuard, TempEnvGuard) { + let tmp = tempfile::tempdir().unwrap(); + let home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + ProfileConfig { + base_url: Some(base_url.to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save("default") + .unwrap(); + store::store_token_data("default", &token).unwrap(); + + (tmp, home, no_kc) + } + + /// When the stored access token is comfortably within its expiry window, + /// the helper returns Refreshed reusing the existing token without any + /// network activity. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_refreshed_when_access_token_still_fresh() { + let now = now_secs(); + let (_tmp, _home, _no_kc) = setup_profile( + "https://unused.invalid", + TokenData { + access_token: "fresh-token".to_string(), + expires_at: now + 3600, + refresh_token: Some("refresh".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ); + + let client = reqwest::Client::new(); + let outcome = try_refresh_stored_session(&client, "default") + .await + .unwrap(); + + match outcome { + RefreshOutcome::Refreshed { token, source, .. } => { + assert_eq!(token, "fresh-token"); + assert!(matches!(source, TokenSource::Stored)); + } + other => panic!("expected Refreshed, got {other:?}"), + } + } + + /// When no token has ever been stored for the profile, the helper + /// returns `Unavailable { NoStoredToken }` so callers can distinguish + /// "never authenticated" from "session expired". + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_unavailable_no_stored_token_when_storage_empty() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save("default") + .unwrap(); + + let client = reqwest::Client::new(); + let outcome = try_refresh_stored_session(&client, "default") + .await + .unwrap(); + + assert!(matches!( + outcome, + RefreshOutcome::Unavailable { + reason: UnavailableReason::NoStoredToken + } + )); + } + + /// When the access token is expired and no refresh token is stored, + /// the helper returns Unavailable without attempting any network call. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_unavailable_when_no_refresh_token() { + let now = now_secs(); + let (_tmp, _home, _no_kc) = setup_profile( + "https://unused.invalid", + TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: None, + refresh_expires_at: None, + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ); + + let client = reqwest::Client::new(); + let outcome = try_refresh_stored_session(&client, "default") + .await + .unwrap(); + + assert!(matches!( + outcome, + RefreshOutcome::Unavailable { + reason: UnavailableReason::NoRefreshToken + } + )); + } + + /// When the access token is expired and the refresh token's recorded + /// expiry has also passed, the helper returns Unavailable without + /// touching the network. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_unavailable_when_refresh_locally_expired() { + let now = now_secs(); + let (_tmp, _home, _no_kc) = setup_profile( + "https://unused.invalid", + TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("refresh".to_string()), + refresh_expires_at: Some(now.saturating_sub(60)), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ); + + let client = reqwest::Client::new(); + let outcome = try_refresh_stored_session(&client, "default") + .await + .unwrap(); + + assert!(matches!( + outcome, + RefreshOutcome::Unavailable { + reason: UnavailableReason::RefreshTokenExpired + } + )); + } + + /// When the access token is expired but the refresh token is valid, + /// the helper calls the token endpoint, persists the new token, and + /// returns Refreshed with TokenSource::Refreshed. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_refreshed_on_successful_refresh() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .respond_with(ResponseTemplate::new(200).set_body_string( + r#"{"access_token":"new-access","expires_in":3600,"token_type":"Bearer","refresh_token":"rotated","refresh_expires_in":7200}"#, + )) + .expect(1) + .mount(&server) + .await; + + let now = now_secs(); + let (_tmp, _home, _no_kc) = setup_profile( + &server.uri(), + TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("valid-refresh".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ); + + let client = reqwest::Client::new(); + let outcome = try_refresh_stored_session(&client, "default") + .await + .unwrap(); + + match outcome { + RefreshOutcome::Refreshed { + token, + source, + expires_in_secs, + .. + } => { + assert_eq!(token, "new-access"); + assert!(matches!(source, TokenSource::Refreshed)); + assert_eq!(expires_in_secs, 3600); + } + other => panic!("expected Refreshed, got {other:?}"), + } + + let stored = store::get_token_data("default").unwrap().unwrap(); + assert_eq!(stored.access_token, "new-access"); + assert_eq!(stored.refresh_token.as_deref(), Some("rotated")); + } + + /// When the token endpoint rejects the refresh (HTTP 401), the helper + /// returns Ok(Rejected) — NOT Err. The caller decides how to react. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_returns_rejected_on_server_401() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .respond_with(ResponseTemplate::new(401).set_body_string( + r#"{"error":"invalid_grant","error_description":"refresh token expired"}"#, + )) + .expect(1) + .mount(&server) + .await; + + let now = now_secs(); + let (_tmp, _home, _no_kc) = setup_profile( + &server.uri(), + TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("dead-refresh".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ); + + let client = reqwest::Client::new(); + let outcome = try_refresh_stored_session(&client, "default") + .await + .unwrap(); + + assert!( + matches!(outcome, RefreshOutcome::Rejected { .. }), + "expected Rejected, got {outcome:?}" + ); + } + + /// Transport-level failures (server unreachable) surface as Err so the + /// caller can abort instead of chasing a doomed fresh flow. + #[tokio::test] + #[serial_test::serial] + async fn test_probe_propagates_network_error() { + let now = now_secs(); + // Point at an unroutable address — any TCP connect attempt fails. + let (_tmp, _home, _no_kc) = setup_profile( + "http://127.0.0.1:1", + TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("valid-refresh".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ); + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_millis(500)) + .build() + .unwrap(); + let result = try_refresh_stored_session(&client, "default").await; + assert!( + matches!( + &result, + Err(e) if matches!(e.kind, crate::protocol::error::RuntimeErrorKind::Network) + ), + "expected Err(Network), got {result:?}" + ); + } + + /// Two concurrent callers must result in exactly ONE refresh request: + /// the second caller, on entering the slow path, sees the freshly-stored + /// token after the locks unwind and returns without calling the endpoint. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + #[serial_test::serial] + async fn test_concurrent_callers_share_one_refresh_request() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .respond_with(ResponseTemplate::new(200).set_body_string( + r#"{"access_token":"shared-new","expires_in":3600,"token_type":"Bearer","refresh_token":"rotated","refresh_expires_in":7200}"#, + )) + .expect(1) + .mount(&server) + .await; + + let now = now_secs(); + let (_tmp, _home, _no_kc) = setup_profile( + &server.uri(), + TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("valid-refresh".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(crate::protocol::request::GrantType::AuthorizationCode), + }, + ); + + let client = reqwest::Client::new(); + let a = { + let client = client.clone(); + let p = "default".to_string(); + tokio::spawn(async move { try_refresh_stored_session(&client, &p).await }) + }; + let b = { + let client = client.clone(); + let p = "default".to_string(); + tokio::spawn(async move { try_refresh_stored_session(&client, &p).await }) + }; + + let (ra, rb) = tokio::join!(a, b); + let ra = ra.unwrap().unwrap(); + let rb = rb.unwrap().unwrap(); + + // Both must succeed with the new token. Mock's .expect(1) (verified on + // drop) asserts only one of them actually called the endpoint. + for outcome in [ra, rb] { + match outcome { + RefreshOutcome::Refreshed { token, .. } => { + assert_eq!(token, "shared-new"); + } + other => panic!("expected Refreshed, got {other:?}"), + } + } + } +} diff --git a/src/runtime/facade/auth.rs b/src/runtime/facade/auth.rs index 0177ad1..0705192 100644 --- a/src/runtime/facade/auth.rs +++ b/src/runtime/facade/auth.rs @@ -55,6 +55,31 @@ impl crate::runtime::Runtime { })) } + /// Probe whether `profile` has a usable session. Returns `Some(AuthView)` if + /// the user is already covered (either still-valid or just-refreshed) and + /// the caller should NOT start a fresh login flow. Returns `None` when a + /// fresh flow is required. + pub async fn auth_probe_existing_session( + &self, + profile: &str, + base_url: String, + client_id: String, + login_type: &'static str, + sink: &mut dyn crate::protocol::event::ProgressSink, + ) -> Result, crate::protocol::error::RuntimeError> + { + let outcome = crate::runtime::auth::operations::probe_existing_session( + &self.reqwest_client, + profile, + base_url, + client_id, + login_type, + sink, + ) + .await?; + Ok(outcome.map(login_outcome_to_view)) + } + /// Complete the authorization-code flow by exchanging `code` for a token and persisting it. pub async fn auth_login_authorization_code( &self, @@ -67,9 +92,8 @@ impl crate::runtime::Runtime { ) -> Result { use crate::runtime::auth::operations; - let http_client = crate::runtime::dispatch::http::build_http_client(None)?; let outcome = operations::login_with_authorization_code( - &http_client, + &self.reqwest_client, operations::AuthorizationCodeLogin { profile: profile.to_string(), base_url, @@ -95,9 +119,8 @@ impl crate::runtime::Runtime { ) -> Result { use crate::runtime::auth::operations; - let http_client = crate::runtime::dispatch::http::build_http_client(None)?; let outcome = operations::login_with_client_credentials( - &http_client, + &self.reqwest_client, operations::ClientCredentialsLogin { profile: profile.to_string(), base_url, @@ -288,7 +311,15 @@ fn login_outcome_to_view( base_url: Some(outcome.base_url), login_type: Some(outcome.login_type.to_string()), client_id: Some(outcome.client_id), - token_expires_in_secs: Some(outcome.expires_in_secs), + token_expires_in_secs: outcome.expires_in_secs, + tip: None, + }), + LoginOutcomeKind::Refreshed => AuthView::LoginSuccess(AuthActionData { + status: AuthActionStatus::Refreshed, + base_url: Some(outcome.base_url), + login_type: Some(outcome.login_type.to_string()), + client_id: Some(outcome.client_id), + token_expires_in_secs: outcome.expires_in_secs, tip: None, }), } diff --git a/src/runtime/mod.rs b/src/runtime/mod.rs index 97072e3..888cce0 100644 --- a/src/runtime/mod.rs +++ b/src/runtime/mod.rs @@ -25,28 +25,40 @@ pub struct Runtime { pub(crate) catalogue: Catalogue, pub(crate) context: ExecutionContext, pub(crate) http_client: Box, + /// Concrete reqwest client used by auth flows (token exchange, probe, + /// refresh). Shares the user-configured timeout with `http_client` so + /// `--timeout` applies uniformly across dispatch and auth. + pub(crate) reqwest_client: reqwest::Client, } impl Runtime { - /// Build a runtime from a resolved execution context and any `HttpClient` - /// implementation. Use this constructor for test injection or alternate - /// transports; production callers typically use [`Runtime::from_reqwest`]. - pub(crate) fn new(context: ExecutionContext, http_client: Box) -> Self { + /// Build a runtime from a resolved execution context, an `HttpClient` + /// implementation, and the concrete `reqwest::Client` used for auth. + /// Use this for test injection or alternate dispatch transports; + /// production callers typically use [`Runtime::from_reqwest`]. + pub(crate) fn new( + context: ExecutionContext, + http_client: Box, + reqwest_client: reqwest::Client, + ) -> Self { Self { catalogue: Catalogue::new(), context, http_client, + reqwest_client, } } /// Convenience constructor that wraps a real `reqwest::Client` in the - /// production `ReqwestHttpClient` adapter. + /// production `ReqwestHttpClient` adapter and retains the original for + /// auth flows that need the concrete type. pub fn from_reqwest(context: ExecutionContext, http_client: reqwest::Client) -> Self { Self::new( context, Box::new(crate::runtime::dispatch::http::ReqwestHttpClient::new( - http_client, + http_client.clone(), )), + http_client, ) } } @@ -71,6 +83,6 @@ mod constructor_tests { #[test] fn test_runtime_new_accepts_dyn_http_client() { let ctx = ExecutionContext::default(); - let _runtime = Runtime::new(ctx, Box::new(DummyClient)); + let _runtime = Runtime::new(ctx, Box::new(DummyClient), reqwest::Client::new()); } } diff --git a/src/support/mod.rs b/src/support/mod.rs index 4f95fb7..19502e8 100644 --- a/src/support/mod.rs +++ b/src/support/mod.rs @@ -4,6 +4,9 @@ pub mod file_system; pub mod output_sink; pub mod strings; +#[cfg(test)] +pub(crate) mod test_helpers; + pub use file_system::FileLock; static LOCK_CONTENTION_REPORTER: std::sync::OnceLock = std::sync::OnceLock::new(); diff --git a/src/support/test_helpers.rs b/src/support/test_helpers.rs new file mode 100644 index 0000000..dff7551 --- /dev/null +++ b/src/support/test_helpers.rs @@ -0,0 +1,51 @@ +//! Test-only helpers shared across in-source `#[cfg(test)]` modules. +//! +//! Integration tests under `tests/` cannot reach this module — they use the +//! parallel helpers in `tests/common/env_guard.rs` instead. Both copies +//! intentionally implement the same RAII semantics; keep them in sync when +//! either side changes. + +use std::time::{SystemTime, UNIX_EPOCH}; + +/// RAII guard restoring an environment variable when dropped. +/// +/// Captures the prior value on construction and restores it (or removes the +/// var if it was previously unset) when the guard goes out of scope. +pub struct TempEnvGuard { + key: &'static str, + original: Option, +} + +impl TempEnvGuard { + /// Set the env var to `value`, capturing the prior value for restoration. + pub fn set(key: &'static str, value: &str) -> Self { + let original = std::env::var(key).ok(); + std::env::set_var(key, value); + Self { key, original } + } + + /// Unset the env var, capturing the prior value for restoration. + #[allow(dead_code)] + pub fn remove(key: &'static str) -> Self { + let original = std::env::var(key).ok(); + std::env::remove_var(key); + Self { key, original } + } +} + +impl Drop for TempEnvGuard { + fn drop(&mut self) { + match &self.original { + Some(val) => std::env::set_var(self.key, val), + None => std::env::remove_var(self.key), + } + } +} + +/// Current wall-clock seconds since the Unix epoch. +pub fn now_secs() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_secs() +} diff --git a/tests/common/env_guard.rs b/tests/common/env_guard.rs new file mode 100644 index 0000000..9bdfa87 --- /dev/null +++ b/tests/common/env_guard.rs @@ -0,0 +1,50 @@ +//! Shared test helpers for scoped env-var manipulation and clock readings. + +use std::time::{SystemTime, UNIX_EPOCH}; + +/// Scoped guard that sets an env var on construction and restores its prior +/// value on drop. Keeps the test process (and any spawned subprocess) pointed +/// at the same `AGS_HOME` / `AGS_NO_KEYCHAIN` for the duration of one test. +pub struct TempEnvGuard { + /// The env var name being managed. + key: &'static str, + /// The original value present before this guard was constructed, or + /// `None` if the variable was unset. + original: Option, +} + +impl TempEnvGuard { + /// Set the env var to `value`, capturing the prior value for restoration + /// when the guard is dropped. + pub fn set(key: &'static str, value: &str) -> Self { + let original = std::env::var(key).ok(); + std::env::set_var(key, value); + Self { key, original } + } + + /// Unset the env var, capturing the prior value for restoration when the + /// guard is dropped. + pub fn remove(key: &'static str) -> Self { + let original = std::env::var(key).ok(); + std::env::remove_var(key); + Self { key, original } + } +} + +impl Drop for TempEnvGuard { + /// Restore the env var to its prior state when the guard goes out of scope. + fn drop(&mut self) { + match &self.original { + Some(val) => std::env::set_var(self.key, val), + None => std::env::remove_var(self.key), + } + } +} + +/// Current wall-clock seconds since the Unix epoch. +pub fn now_secs() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_secs() +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 3ddff2e..4587292 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,6 +1,8 @@ #[allow(dead_code)] pub mod cli_helpers; #[allow(dead_code)] +pub mod env_guard; +#[allow(dead_code)] pub mod error_helpers; #[allow(dead_code)] pub mod fixture_helpers; diff --git a/tests/functional/auth/login.rs b/tests/functional/auth/login.rs index 0864904..4a019fe 100644 --- a/tests/functional/auth/login.rs +++ b/tests/functional/auth/login.rs @@ -1,5 +1,40 @@ use crate::common::cli_helpers::{ags, ags_isolated}; +use crate::common::env_guard::{now_secs, TempEnvGuard}; +use ags::runtime::auth::store::{self, TokenData}; +use ags::runtime::config::{GlobalConfig, ProfileConfig}; use predicates::prelude::*; +use wiremock::matchers::{body_string_contains, method, path}; +use wiremock::{Mock, MockServer, ResponseTemplate}; + +/// Set "default" as the active profile in the global config so the +/// subprocess's profile-name resolver finds it instead of erroring with +/// "No active profile". +fn activate_default_profile() { + GlobalConfig { + active_profile: Some("default".to_string()), + ..Default::default() + } + .save() + .unwrap(); +} + +/// Persist a stale stored token to the default profile. If `refresh_token` is +/// Some, the refresh token is set with a still-valid local expiry; if None, +/// no refresh token is stored. +fn write_stale_token(profile: &str, refresh_token: Option<&str>) { + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "expired-access".to_string(), + expires_at: now.saturating_sub(300), + refresh_token: refresh_token.map(|s| s.to_string()), + refresh_expires_at: refresh_token.map(|_| now + 86_400), + grant_type: Some(ags::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); +} /// Login help advertises all supported flags so users discover available auth options #[test] @@ -290,3 +325,645 @@ fn test_auth_login_stdin_occupied_without_client_id_errors() { .failure() .stderr(predicate::str::contains("AGS_CLIENT_ID")); } + +/// When the stored access token is still valid, login is a no-op and +/// no network call is made. +#[tokio::test] +#[serial_test::serial] +async fn test_login_no_op_when_access_token_fresh() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(ags::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args(["auth", "login"]) + .output() + .unwrap(); + + let combined = String::from_utf8_lossy(&output.stdout).to_string() + + &String::from_utf8_lossy(&output.stderr); + assert!( + combined.contains("Already authenticated"), + "expected 'Already authenticated' headline, got: {combined}" + ); +} + +/// When the access token is expired but the refresh token is valid, +/// login refreshes in place — no browser flow runs — and reports the +/// 'Session refreshed' outcome. +#[tokio::test] +#[serial_test::serial] +async fn test_login_refreshes_when_access_expired_and_refresh_valid() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(200).set_body_string( + r#"{"access_token":"refreshed-access","expires_in":3600,"token_type":"Bearer","refresh_token":"rotated","refresh_expires_in":7200}"#, + )) + .expect(1) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + write_stale_token(profile, Some("valid-refresh")); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args(["auth", "login"]) + .output() + .unwrap(); + + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Session refreshed"), + "expected 'Session refreshed' headline, got stdout: {stdout}\nstderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let stored = store::get_token_data(profile).unwrap().unwrap(); + assert_eq!(stored.access_token, "refreshed-access"); +} + +/// When the access token is expired and the server rejects the refresh, +/// login emits the 'Existing session was invalid' message, clears the +/// stored token, and proceeds to print the OAuth URL. +#[tokio::test] +#[serial_test::serial] +async fn test_login_falls_through_to_fresh_flow_on_refresh_rejection() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(401).set_body_string( + r#"{"error":"invalid_grant","error_description":"refresh token expired"}"#, + )) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + write_stale_token(profile, Some("dead-refresh")); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .env("AGS_AUTH_TIMEOUT", "1") // abort callback server fast + .args(["auth", "login"]) + .output() + .unwrap(); + + let combined = String::from_utf8_lossy(&output.stdout).to_string() + + &String::from_utf8_lossy(&output.stderr); + assert!( + combined.contains("Existing session was invalid"), + "expected probe-failure message, got: {combined}" + ); + let after = store::get_token_data(profile).unwrap(); + assert!( + after.is_none(), + "stale token should have been cleared after probe rejection, got: {after:?}" + ); +} + +/// When the stored access token is expired and no refresh token is stored, +/// login does NOT call /oauth/token for a refresh — it proceeds directly +/// to the fresh OAuth prompt. +#[tokio::test] +#[serial_test::serial] +async fn test_login_skips_probe_when_no_refresh_token() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + write_stale_token(profile, None); + + let _output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .env("AGS_AUTH_TIMEOUT", "1") + .args(["auth", "login"]) + .output() + .unwrap(); + + // No assertion on stdout — the .expect(0) on the mock is the assertion. + drop(server); +} + +/// JSON output for a refreshed session via the client-credentials login +/// path must carry the 'refreshed' status so downstream consumers can +/// distinguish 'refreshed in place' from a fresh OAuth flow (status +/// 'logged_in') and from a no-op (status 'already_authenticated'). +#[tokio::test] +#[serial_test::serial] +async fn test_client_credentials_login_json_output_carries_refreshed_status() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(200).set_body_string( + r#"{"access_token":"refreshed","expires_in":3600,"token_type":"Bearer","refresh_token":"rotated","refresh_expires_in":7200}"#, + )) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + write_stale_token(profile, Some("valid-refresh")); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args([ + "--format", + "json", + "auth", + "login", + "--grant", + "client-credentials", + "--client-secret", + "unused", + ]) + .output() + .unwrap(); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + // The actual JSON envelope shape is what the JsonFrontend produces. We + // search the entire JSON tree for a "status" field whose value is the + // expected string. If this assertion fails, inspect stdout to see the + // real shape. + let parsed: serde_json::Value = serde_json::from_str(&stdout).unwrap_or_else(|e| { + panic!("expected JSON output\nstdout: {stdout}\nstderr: {stderr}\nerror: {e}") + }); + + let status = find_string_field(&parsed, "status") + .unwrap_or_else(|| panic!("could not find a 'status' string field in JSON: {parsed}")); + assert_eq!( + status, "refreshed", + "expected status='refreshed' in JSON, full output: {parsed}" + ); +} + +/// JSON output for a refreshed session via the authorization-code login +/// path must also carry the 'refreshed' status. The probe runs for both +/// grants, so both must serialise the outcome identically. +#[tokio::test] +#[serial_test::serial] +async fn test_authorization_code_login_json_output_carries_refreshed_status() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(200).set_body_string( + r#"{"access_token":"refreshed","expires_in":3600,"token_type":"Bearer","refresh_token":"rotated","refresh_expires_in":7200}"#, + )) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + write_stale_token(profile, Some("valid-refresh")); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args([ + "--format", + "json", + "auth", + "login", + "--grant", + "authorization-code", + ]) + .output() + .unwrap(); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + let parsed: serde_json::Value = serde_json::from_str(&stdout).unwrap_or_else(|e| { + panic!("expected JSON output\nstdout: {stdout}\nstderr: {stderr}\nerror: {e}") + }); + + let status = find_string_field(&parsed, "status") + .unwrap_or_else(|| panic!("could not find a 'status' string field in JSON: {parsed}")); + assert_eq!( + status, "refreshed", + "expected status='refreshed' in JSON for authorization-code probe, full output: {parsed}" + ); +} + +/// `auth login --no-input` with a still-valid stored access token must +/// short-circuit via the probe and exit 0 with "Already authenticated" +/// instead of rejecting on non-interactive mode. The probe makes the +/// browser flow unnecessary, so the non-interactive guard should not fire. +#[tokio::test] +#[serial_test::serial] +async fn test_login_no_input_short_circuits_when_token_fresh() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(ags::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args(["--no-input", "auth", "login"]) + .output() + .unwrap(); + + assert!( + output.status.success(), + "expected exit 0, got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let combined = String::from_utf8_lossy(&output.stdout).to_string() + + &String::from_utf8_lossy(&output.stderr); + assert!( + combined.contains("Already authenticated"), + "expected 'Already authenticated' headline, got: {combined}" + ); +} + +/// `auth login --format json` with a still-valid stored access token must +/// short-circuit via the probe and emit JSON `{"status":"already_authenticated"}` +/// instead of an error envelope. +#[tokio::test] +#[serial_test::serial] +async fn test_login_json_short_circuits_when_token_fresh() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(ags::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args(["--format", "json", "auth", "login"]) + .output() + .unwrap(); + + assert!( + output.status.success(), + "expected exit 0, got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let stdout = String::from_utf8_lossy(&output.stdout); + let parsed: serde_json::Value = serde_json::from_str(&stdout) + .unwrap_or_else(|e| panic!("expected JSON output\nstdout: {stdout}\nerror: {e}")); + let status = find_string_field(&parsed, "status") + .unwrap_or_else(|| panic!("could not find a 'status' string field in JSON: {parsed}")); + assert_eq!( + status, "already_authenticated", + "expected status='already_authenticated' in JSON, full output: {parsed}" + ); +} + +/// `auth login --no-input` with no stored session has nothing to probe and +/// can't drive the browser flow either, so it must reject with the +/// "requires browser interaction" guidance. Regression guard for the +/// non-interactive fallback once the probe is in place. +#[tokio::test] +#[serial_test::serial] +async fn test_login_no_input_rejects_when_no_existing_session() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + + ags_isolated() + .env("AGS_HOME", tmp.path()) + .args(["--no-input", "auth", "login"]) + .assert() + .failure() + .stderr(predicate::str::contains("requires browser interaction")); +} + +/// `auth login --format json` with no stored session must emit a JSON error +/// envelope carrying the "requires browser interaction" message. Mirrors +/// the --no-input case but verifies the error renders as JSON. +#[tokio::test] +#[serial_test::serial] +async fn test_login_json_rejects_when_no_existing_session() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://unused.invalid".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args(["--format", "json", "auth", "login"]) + .output() + .unwrap(); + + assert!( + !output.status.success(), + "expected non-zero exit, got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + // JSON error envelopes are emitted on stderr (see `JsonFrontend::render_error`). + let stderr = String::from_utf8_lossy(&output.stderr); + let parsed: serde_json::Value = serde_json::from_str(stderr.trim()) + .unwrap_or_else(|e| panic!("expected JSON error envelope on stderr\nstderr: {stderr}\nerror: {e}")); + let error = find_string_field(&parsed, "error") + .unwrap_or_else(|| panic!("could not find an 'error' string field in JSON: {parsed}")); + assert!( + error.contains("requires browser interaction"), + "expected JSON error to mention 'requires browser interaction', got: {error}" + ); +} + +/// When `--base-url` differs from the value the stored session was minted +/// against, the probe must not short-circuit; non-interactive mode then +/// rejects with the browser-interaction guidance instead of falsely +/// reporting "Already authenticated". +#[tokio::test] +#[serial_test::serial] +async fn test_login_no_input_proceeds_when_base_url_differs() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://dev.example.com".to_string()), + client_id: Some("cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(ags::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + ags_isolated() + .env("AGS_HOME", tmp.path()) + .args([ + "--no-input", + "auth", + "login", + "--base-url", + "https://demo.example.com", + ]) + .assert() + .failure() + .stderr(predicate::str::contains("requires browser interaction")); +} + +/// Same identity guard as above but exercised via a differing `--client-id` +/// and JSON output. Verifies the probe doesn't claim the stored session +/// belongs to a different OAuth client. +#[tokio::test] +#[serial_test::serial] +async fn test_login_json_proceeds_when_client_id_differs() { + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let profile = "default"; + ProfileConfig { + base_url: Some("https://dev.example.com".to_string()), + client_id: Some("original-cid".to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + activate_default_profile(); + + let now = now_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "fresh".to_string(), + expires_at: now + 3600, + refresh_token: Some("rt".to_string()), + refresh_expires_at: Some(now + 7200), + grant_type: Some(ags::protocol::request::GrantType::AuthorizationCode), + }, + ) + .unwrap(); + + let output = ags_isolated() + .env("AGS_HOME", tmp.path()) + .args([ + "--format", + "json", + "auth", + "login", + "--client-id", + "different-cid", + ]) + .output() + .unwrap(); + + assert!( + !output.status.success(), + "expected non-zero exit (no short-circuit), got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let combined = String::from_utf8_lossy(&output.stdout).to_string() + + &String::from_utf8_lossy(&output.stderr); + let json_start = combined + .find('{') + .unwrap_or_else(|| panic!("no JSON object in output: {combined}")); + let parsed: serde_json::Value = serde_json::from_str(combined[json_start..].trim()) + .unwrap_or_else(|e| panic!("expected JSON\noutput: {combined}\nerror: {e}")); + let error = find_string_field(&parsed, "error") + .unwrap_or_else(|| panic!("could not find 'error' string in JSON: {parsed}")); + assert!( + error.contains("requires browser interaction"), + "expected JSON error to mention 'requires browser interaction', got: {error}" + ); +} + +/// Recursively walk a JSON value looking for the first occurrence of the +/// named field with a string value. Returns the string if found. +fn find_string_field<'a>(value: &'a serde_json::Value, field: &str) -> Option<&'a str> { + match value { + serde_json::Value::Object(map) => { + if let Some(v) = map.get(field).and_then(|v| v.as_str()) { + return Some(v); + } + for v in map.values() { + if let Some(found) = find_string_field(v, field) { + return Some(found); + } + } + None + } + serde_json::Value::Array(arr) => { + for v in arr { + if let Some(found) = find_string_field(v, field) { + return Some(found); + } + } + None + } + _ => None, + } +} diff --git a/tests/integration/auth.rs b/tests/integration/auth.rs index 5f66f20..7135d09 100644 --- a/tests/integration/auth.rs +++ b/tests/integration/auth.rs @@ -3,6 +3,8 @@ use wiremock::{Mock, MockServer, ResponseTemplate}; use ags::runtime::auth::tokens as service; +use crate::common::env_guard::TempEnvGuard; + const TEST_CLIENT_ID: &str = "test-client-id"; const TEST_CLIENT_SECRET: &str = "test-client-secret"; @@ -409,3 +411,122 @@ fn test_auth_login_rejects_invalid_base_url_without_panic() { .stderr(contains("Invalid base URL").and(contains("panicked").not())); } } + +/// Client-credentials self-heals: when the stored token is stale and the +/// stored refresh token is rejected by the server, probe_existing_session +/// returns None (after clearing the stale state), and the subsequent +/// client-credentials grant succeeds. +#[tokio::test] +#[serial_test::serial] +async fn test_client_credentials_self_heals_on_refresh_rejection() { + use ags::runtime::auth::store::{self, TokenData}; + use ags::runtime::config::ProfileConfig; + use std::time::{SystemTime, UNIX_EPOCH}; + + let tmp = tempfile::tempdir().unwrap(); + let _home = TempEnvGuard::set("AGS_HOME", tmp.path().to_str().unwrap()); + let _no_kc = TempEnvGuard::set("AGS_NO_KEYCHAIN", "1"); + + let server = MockServer::start().await; + + // Refresh attempt: server rejects. + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(401).set_body_string( + r#"{"error":"invalid_grant","error_description":"refresh token expired"}"#, + )) + .mount(&server) + .await; + + // Client-credentials grant: succeeds. + Mock::given(method("POST")) + .and(path("/iam/v3/oauth/token")) + .and(body_string_contains("grant_type=client_credentials")) + .respond_with(ResponseTemplate::new(200).set_body_string( + r#"{"access_token":"fresh-cc-token","expires_in":3600,"token_type":"Bearer"}"#, + )) + .mount(&server) + .await; + + let profile = "default"; + ProfileConfig { + base_url: Some(server.uri()), + client_id: Some(TEST_CLIENT_ID.to_string()), + ..Default::default() + } + .save(profile) + .unwrap(); + + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_secs(); + store::store_token_data( + profile, + &TokenData { + access_token: "expired".to_string(), + expires_at: now.saturating_sub(60), + refresh_token: Some("dead-refresh".to_string()), + refresh_expires_at: Some(now + 86_400), + grant_type: Some(ags::protocol::request::GrantType::ClientCredentials), + }, + ) + .unwrap(); + + /// Sink that drops every progress event — tests don't care about the stream. + struct TestSink; + impl ags::protocol::event::ProgressSink for TestSink { + /// Drop every event. + fn on_event(&mut self, _event: ags::protocol::event::ProgressEvent) {} + } + + let client = reqwest::Client::new(); + let mut sink = TestSink; + + // Step 1: probe runs first (as the CLI handler now does). Expected: None + // because the refresh was rejected. The stored token must have been + // cleared as a side effect. + let probed = ags::runtime::auth::operations::probe_existing_session( + &client, + profile, + server.uri(), + TEST_CLIENT_ID.to_string(), + "client credentials", + &mut sink, + ) + .await + .unwrap(); + assert!( + probed.is_none(), + "probe should return None after rejection, got {probed:?}" + ); + assert!( + store::get_token_data(profile).unwrap().is_none(), + "stale token data should have been cleared by the probe" + ); + + // Step 2: probe returned None → caller proceeds with fresh grant. + let outcome = ags::runtime::auth::operations::login_with_client_credentials( + &client, + ags::runtime::auth::operations::ClientCredentialsLogin { + profile: profile.to_string(), + base_url: server.uri(), + client_id: TEST_CLIENT_ID.to_string(), + client_secret: TEST_CLIENT_SECRET.to_string(), + }, + &mut sink, + ) + .await + .unwrap(); + + use ags::runtime::auth::operations::LoginOutcomeKind; + assert!( + matches!(outcome.kind, LoginOutcomeKind::LoggedIn), + "expected LoggedIn from fresh grant, got {:?}", + outcome.kind + ); + + let stored = store::get_token_data(profile).unwrap().unwrap(); + assert_eq!(stored.access_token, "fresh-cc-token"); +} diff --git a/tests/integration/config.rs b/tests/integration/config.rs index 610f3cf..a6a6328 100644 --- a/tests/integration/config.rs +++ b/tests/integration/config.rs @@ -1,26 +1,6 @@ use ags::runtime::config::{self, ConfigScope, GlobalConfig, ProfileConfig}; -struct TempEnvGuard { - key: &'static str, - original: Option, -} - -impl TempEnvGuard { - fn set(key: &'static str, value: &str) -> Self { - let original = std::env::var(key).ok(); - std::env::set_var(key, value); - Self { key, original } - } -} - -impl Drop for TempEnvGuard { - fn drop(&mut self) { - match &self.original { - Some(val) => std::env::set_var(self.key, val), - None => std::env::remove_var(self.key), - } - } -} +use crate::common::env_guard::TempEnvGuard; /// Profile config values round-trip through the type-owned accessors. #[test] diff --git a/tests/integration/namespace.rs b/tests/integration/namespace.rs index f103528..69c70dc 100644 --- a/tests/integration/namespace.rs +++ b/tests/integration/namespace.rs @@ -1,34 +1,7 @@ use ags::runtime::config::ProfileConfig; use ags::runtime::execution::ResolutionInput; -/// RAII guard that sets an env var and restores the original value on drop. -struct TempEnvGuard { - key: &'static str, - original: Option, -} - -impl TempEnvGuard { - fn set(key: &'static str, value: &str) -> Self { - let original = std::env::var(key).ok(); - std::env::set_var(key, value); - Self { key, original } - } - - fn remove(key: &'static str) -> Self { - let original = std::env::var(key).ok(); - std::env::remove_var(key); - Self { key, original } - } -} - -impl Drop for TempEnvGuard { - fn drop(&mut self) { - match &self.original { - Some(val) => std::env::set_var(self.key, val), - None => std::env::remove_var(self.key), - } - } -} +use crate::common::env_guard::TempEnvGuard; /// --namespace flag overrides env vars and config to give explicit invocations deterministic behavior #[tokio::test] diff --git a/tests/integration/profile.rs b/tests/integration/profile.rs index 7f54056..75ebc47 100644 --- a/tests/integration/profile.rs +++ b/tests/integration/profile.rs @@ -1,33 +1,7 @@ use ags::runtime::config::{self, ProfileConfig}; use ags::runtime::execution::ResolutionInput; -struct TempEnvGuard { - key: &'static str, - original: Option, -} - -impl TempEnvGuard { - fn set(key: &'static str, value: &str) -> Self { - let original = std::env::var(key).ok(); - std::env::set_var(key, value); - Self { key, original } - } - - fn remove(key: &'static str) -> Self { - let original = std::env::var(key).ok(); - std::env::remove_var(key); - Self { key, original } - } -} - -impl Drop for TempEnvGuard { - fn drop(&mut self) { - match &self.original { - Some(val) => std::env::set_var(self.key, val), - None => std::env::remove_var(self.key), - } - } -} +use crate::common::env_guard::TempEnvGuard; /// Two profiles with different namespaces resolve independently #[tokio::test] diff --git a/tests/integration/token_refresh_race.rs b/tests/integration/token_refresh_race.rs index 7c2edcd..a74089b 100644 --- a/tests/integration/token_refresh_race.rs +++ b/tests/integration/token_refresh_race.rs @@ -18,48 +18,13 @@ #![allow(clippy::await_holding_lock)] -use std::time::{SystemTime, UNIX_EPOCH}; - use ags::runtime::auth::session as service; use ags::runtime::auth::store::{self, TokenData}; use ags::runtime::config::ProfileConfig; use wiremock::matchers::{body_string_contains, method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; -struct TempEnvGuard { - key: &'static str, - original: Option, -} - -impl TempEnvGuard { - fn set(key: &'static str, value: &str) -> Self { - let original = std::env::var(key).ok(); - std::env::set_var(key, value); - Self { key, original } - } - - fn remove(key: &'static str) -> Self { - let original = std::env::var(key).ok(); - std::env::remove_var(key); - Self { key, original } - } -} - -impl Drop for TempEnvGuard { - fn drop(&mut self) { - match &self.original { - Some(val) => std::env::set_var(self.key, val), - None => std::env::remove_var(self.key), - } - } -} - -fn now_secs() -> u64 { - SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_secs() -} +use crate::common::env_guard::{now_secs, TempEnvGuard}; /// Ten concurrent `resolve_access_token` calls with a stale access token /// must make exactly ONE refresh request — losers wait on the in-flight From 75b3acd10a423a9ddecc0ddcf5eac9cc9f451726 Mon Sep 17 00:00:00 2001 From: phil-accelbyte <225106921+phil-accelbyte@users.noreply.github.com> Date: Tue, 12 May 2026 21:23:29 +0800 Subject: [PATCH 2/2] style: cargo fmt --- tests/functional/auth/login.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/functional/auth/login.rs b/tests/functional/auth/login.rs index 4a019fe..27bd168 100644 --- a/tests/functional/auth/login.rs +++ b/tests/functional/auth/login.rs @@ -815,8 +815,9 @@ async fn test_login_json_rejects_when_no_existing_session() { ); // JSON error envelopes are emitted on stderr (see `JsonFrontend::render_error`). let stderr = String::from_utf8_lossy(&output.stderr); - let parsed: serde_json::Value = serde_json::from_str(stderr.trim()) - .unwrap_or_else(|e| panic!("expected JSON error envelope on stderr\nstderr: {stderr}\nerror: {e}")); + let parsed: serde_json::Value = serde_json::from_str(stderr.trim()).unwrap_or_else(|e| { + panic!("expected JSON error envelope on stderr\nstderr: {stderr}\nerror: {e}") + }); let error = find_string_field(&parsed, "error") .unwrap_or_else(|| panic!("could not find an 'error' string field in JSON: {parsed}")); assert!(