diff --git a/CONTEXT.md b/CONTEXT.md index 6d6b269..5c14e2a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -307,6 +307,10 @@ error rather than launching a login flow (the 401 path never pops an interactive prompt). Does not own a `TokenProvider` directly — the handler retrieves the provider via `AppContext::opt_token_provider()` and constructs the client itself. The framework does not pre-build or vend a shared instance. +Its `send` is the **single attempt**: the request goes out once and the response +comes back whatever its status, body intact; only the 401 auth-layer retry +applies, never the inner transient budget, so a write is re-sent only after a +401. _Avoid_: "auth client" (overloaded with the OIDC client concept). **cli-framework-oidc**: diff --git a/README.md b/README.md index e45a1b1..de1c507 100644 --- a/README.md +++ b/README.md @@ -393,6 +393,22 @@ let client = AuthenticatedHttpClient::new( let resp = client.get("https://api.example.com/data").await?; ``` +`get`, `post` and the other verb methods go through the inner client's retry +policy and turn a 4xx or 5xx into an error, so the response body is lost. When +the caller needs the body of an error answer, or must not send a write twice, +use `send`: it sends the request once and returns the response whatever its +status. Only a 401 is retried, once, with a renewed token; a 5xx, a 429 or a +network error is not. + +```rust +let resp = client + .send(|| client.client().post("https://api.example.com/things").json(&body)) + .await?; +if !resp.status().is_success() { + let problem: ApiError = resp.json().await?; // the body is still there +} +``` + ## Managed Configuration (`config`, `config-managed`, `config-service`) Three independently-gated layers build on each other: the manifest data model and diff --git a/skill/references/auth-and-oidc.md b/skill/references/auth-and-oidc.md index 7b30995..5992c00 100644 --- a/skill/references/auth-and-oidc.md +++ b/skill/references/auth-and-oidc.md @@ -264,7 +264,9 @@ let http = AuthenticatedHttpClient::new( let resp = http.get("https://api.example.com/things").await?; ``` -The client exposes `get`, `post`, `put`, `delete`, `patch`, `head`, `options`. Each takes a `&str` URL and returns `anyhow::Result`. +The client exposes `get`, `post`, `put`, `delete`, `patch`, `head`, `options`. Each takes a `&str` URL and returns `anyhow::Result`. They retry 5xx, 429 and network errors through the inner client and turn any 4xx or 5xx into an error, dropping the body. + +`send(|| client.client().post(url).json(&body))` sends once and returns the response whatever its status, so an error body can be decoded and a write is never sent twice, except after a 401: the token is renewed and the request sent once more. A second 401 comes back as the response. `NotAuthenticated` comes back as an `AuthError` inside the `anyhow::Error`. ### Accessing the provider from a command diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 40087f9..3c33583 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -183,6 +183,33 @@ impl AuthenticatedHttpClient { } } + /// Send the request once, with the bearer token, and return the response + /// whatever its status, so the caller can read the body of an error. + /// + /// A 401 is handled as in [`execute_with_retry`](Self::execute_with_retry): + /// the token is invalidated and re-acquired, and the request is sent once + /// more. Nothing else is retried: not a 5xx, a 429 or a network error. A + /// request that changes something is therefore sent twice only when the + /// first attempt was refused as unauthenticated. The inner client's retry + /// policy and its `http.client.request` span are not used. + /// + /// Errors are a [`AuthError`] when no token can be had (including + /// `NotAuthenticated` after the 401), or the `reqwest::Error` of a request + /// that got no answer. + pub async fn send(&self, build: F) -> anyhow::Result + where + F: Fn() -> reqwest::RequestBuilder + Send + Sync, + { + let token = self.provider.token().await?; + let response = build().bearer_auth(token.as_bearer()).send().await?; + if response.status() != reqwest::StatusCode::UNAUTHORIZED { + return Ok(response); + } + self.provider.invalidate().await; + let fresh = self.provider.token().await?; + Ok(build().bearer_auth(fresh.as_bearer()).send().await?) + } + pub async fn get(&self, url: &str) -> anyhow::Result { self.execute_with_retry(|| self.inner.client().get(url)) .await diff --git a/tests/unit/auth_provider.rs b/tests/unit/auth_provider.rs index 9aabbd7..c24703e 100644 --- a/tests/unit/auth_provider.rs +++ b/tests/unit/auth_provider.rs @@ -295,3 +295,168 @@ fn stderr_auth_flow_reporter_compiles() { // Calling these writes to real stderr; we just assert they don't panic. reporter.message("test message"); } + +// ── AuthenticatedHttpClient::send: one attempt, any status ─────────────────── + +/// Hands out `token-v1`, then `token-v2` after `invalidate`, counting both. +struct RotatingProvider { + calls: AtomicUsize, + invalidated: AtomicUsize, +} + +impl RotatingProvider { + fn new() -> Arc { + Arc::new(Self { + calls: AtomicUsize::new(0), + invalidated: AtomicUsize::new(0), + }) + } +} + +#[async_trait::async_trait] +impl TokenProvider for RotatingProvider { + async fn token(&self) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + let token = match self.invalidated.load(Ordering::SeqCst) { + 0 => "token-v1", + _ => "token-v2", + }; + Ok(AccessToken::new(token.to_string(), None)) + } + async fn invalidate(&self) { + self.invalidated.fetch_add(1, Ordering::SeqCst); + } +} + +fn send_client(provider: Arc) -> AuthenticatedHttpClient { + AuthenticatedHttpClient::new(RetryableHttpClient::new(reqwest::Client::new()), provider) +} + +#[tokio::test] +async fn send_returns_an_error_answer_with_its_body_and_does_not_retry_it() { + for status in [400u16, 403, 409, 429, 500, 503] { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/write")) + .and(header("authorization", "Bearer token-v1")) + .respond_with(ResponseTemplate::new(status).set_body_string(r#"{"code":"no"}"#)) + .expect(1) + .mount(&mock) + .await; + let provider = RotatingProvider::new(); + let client = send_client(provider.clone()); + + let url = format!("{}/api/write", mock.uri()); + let response = client + .send(|| client.client().post(&url).body("once")) + .await + .expect("an error status is an answer, not an error"); + assert_eq!(response.status(), status); + assert_eq!(response.text().await.unwrap(), r#"{"code":"no"}"#); + assert_eq!(provider.invalidated.load(Ordering::SeqCst), 0, "{status}"); + mock.verify().await; + } +} + +#[tokio::test] +async fn send_renews_the_token_once_on_401_and_returns_the_second_answer() { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/write")) + .and(header("authorization", "Bearer token-v1")) + .respond_with(ResponseTemplate::new(401)) + .expect(1) + .mount(&mock) + .await; + Mock::given(method("POST")) + .and(path("/api/write")) + .and(header("authorization", "Bearer token-v2")) + .respond_with(ResponseTemplate::new(201).set_body_string("made")) + .expect(1) + .mount(&mock) + .await; + let provider = RotatingProvider::new(); + let client = send_client(provider.clone()); + + let url = format!("{}/api/write", mock.uri()); + let response = client.send(|| client.client().post(&url)).await.unwrap(); + assert_eq!(response.status(), 201); + assert_eq!(response.text().await.unwrap(), "made"); + assert_eq!(provider.invalidated.load(Ordering::SeqCst), 1); + assert_eq!(provider.calls.load(Ordering::SeqCst), 2); + mock.verify().await; +} + +#[tokio::test] +async fn send_returns_a_second_401_as_an_answer() { + let mock = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/secure")) + .respond_with(ResponseTemplate::new(401).set_body_string("who")) + .expect(2) + .mount(&mock) + .await; + let provider = RotatingProvider::new(); + let client = send_client(provider.clone()); + + let url = format!("{}/api/secure", mock.uri()); + let response = client.send(|| client.client().get(&url)).await.unwrap(); + assert_eq!(response.status(), 401); + assert_eq!(response.text().await.unwrap(), "who"); + assert_eq!(provider.invalidated.load(Ordering::SeqCst), 1); + mock.verify().await; +} + +#[tokio::test] +async fn send_surfaces_not_authenticated_and_sends_nothing_without_a_token() { + struct NoLogin; + #[async_trait::async_trait] + impl TokenProvider for NoLogin { + async fn token(&self) -> Result { + Err(AuthError::NotAuthenticated) + } + async fn invalidate(&self) {} + } + let mock = MockServer::start().await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&mock) + .await; + let client = send_client(Arc::new(NoLogin)); + + let url = format!("{}/api/x", mock.uri()); + let err = client.send(|| client.client().get(&url)).await.unwrap_err(); + assert!( + matches!( + err.downcast_ref::(), + Some(AuthError::NotAuthenticated) + ), + "{err:?}" + ); + mock.verify().await; +} + +#[tokio::test] +async fn send_does_not_retry_a_request_that_got_no_answer() { + // A port nothing listens on: bind, note it, close. + let port = std::net::TcpListener::bind("127.0.0.1:0") + .unwrap() + .local_addr() + .unwrap() + .port(); + let provider = RotatingProvider::new(); + let client = send_client(provider.clone()); + + let url = format!("http://127.0.0.1:{port}/api/write"); + let err = client + .send(|| client.client().post(&url)) + .await + .unwrap_err(); + assert!( + err.downcast_ref::() + .is_some_and(reqwest::Error::is_connect), + "{err:?}" + ); + assert_eq!(provider.calls.load(Ordering::SeqCst), 1, "sent once"); +}