From c0f70ca367545839a9e9b67ded467324ea514484 Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Wed, 23 Sep 2026 09:40:35 +0200 Subject: [PATCH 1/2] refactor!: remove the inert PROJECT_NAME and the auth_diagnostics module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the SDK carried that no longer do anything. `auth_diagnostics` existed to explain a 401 the api would not: it decoded the `organization` claim of the token just sent and reconstructed which of the validator's branches had rejected it, because the authentication entry point answered with an empty body. It does not any more — a 401 carries a problem document whose `detail` names the failed check, in wording that covers the same five branches. The SDK was appending a near-duplicate of what the server had already said. The `base64` dependency existed only for this and goes with it. The multi-tenant test that asserted on the reconstructed message now asserts on the server's own wording, so it still pins that the explanation survives the whole path from entry point to `ResponseError`. `PROJECT_NAME` was accepted, stored and never read, and the backend has no such concept on either side of the wire — while AGENTS.md and README.md both listed it as a supported variable. A config value that reads as real and affects nothing is worse than an absent one. BREAKING CHANGE: `DataHubConfig::from_vars` loses its `project_name` parameter, as do the `DataHubClient` and `AsyncDataHubClient` constructors in the Python bindings. Callers passing it positionally must drop the argument; callers passing `project_name=` by keyword must remove it. Needs a note in datahub-sdk-docs: the env-var list and the client constructor signatures both mention it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: jgjesdal --- AGENTS.md | 10 +- Cargo.toml | 3 - README.md | 1 - datahub_python_bindings/src/lib.rs | 8 - python_tests/README.md | 2 +- python_tests/test_auth_constructor.py | 2 - src/auth_diagnostics.rs | 221 -------------------------- src/buffer_integration.rs | 1 - src/datahub.rs | 8 - src/generic.rs | 129 ++------------- src/lib.rs | 1 - src/multi_tenant_integration.rs | 18 +-- src/problem_integration.rs | 6 +- src/timeseries/mod.rs | 1 - 14 files changed, 26 insertions(+), 385 deletions(-) delete mode 100644 src/auth_diagnostics.rs diff --git a/AGENTS.md b/AGENTS.md index ba43bd5..9e32b81 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -42,7 +42,7 @@ rather than run unoptimised, and the same applies to the PyO3 module, which need Most tests are integration tests that call a live backend via `create_api_service()`. They read configuration from a local `.env` file (gitignored). Required: - `BASE_URL` — backend root, e.g. `http://localhost:8081` -- Either `TOKEN` (bearer token used as-is, no expiry) **or** the OAuth2 client-credentials set: `CLIENT_ID`, `CLIENT_SECRET`, `TOKEN_URI` (optional: `PROJECT_NAME`) +- Either `TOKEN` (bearer token used as-is, no expiry) **or** the OAuth2 client-credentials set: `CLIENT_ID`, `CLIENT_SECRET`, `TOKEN_URI` Tests that mutate backend state (create/delete) often `sleep` a few seconds between operations and are sensitive to race conditions — prefer running them serially or isolating by unique external IDs. @@ -245,10 +245,10 @@ refusal, so a by-id 404 carries `type: …/errors/not-found` and **`slug()` matc stays as the regression guard. 401s carry a problem document too, with `type: …/errors/unauthorized` and a `detail` naming the -check that failed — a missing, empty, malformed or ambiguous `organization` claim. That is the -reason `src/auth_diagnostics.rs` reconstructs from the token it just sent, so the SDK now appends -a near-duplicate of what the server already said. Note every entry-point 401 shares the -`unauthorized` slug, so the cause is in the prose and cannot be branched on. +check that failed — a missing, empty, malformed or ambiguous `organization` claim. That retired +the SDK's own `auth_diagnostics` module, which existed only to reconstruct the reason from the +token it had just sent. Note every entry-point 401 shares the `unauthorized` slug, so the cause is +in the prose and cannot be branched on. ### Filters (`src/filters.rs`) diff --git a/Cargo.toml b/Cargo.toml index 2d07d25..8bc63be 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,9 +39,6 @@ infer = { version = "0.19", default-features = false, features = ["std"] } thiserror = "2.0.17" zstd = "0.13" geojson = "1" -# Reading (never verifying) the payload of a JWT the SDK already holds, to explain an -# otherwise-unexplained 401 — see `auth_diagnostics`. -base64 = "0.22" arrow-array = "59.3" arrow-schema = "59.3" arrow-ipc = "59.3" diff --git a/README.md b/README.md index 0653c0b..fdf472b 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,6 @@ environment: - `BASE_URL` — backend root, e.g. `http://localhost:8081` (required) - Either `TOKEN` (bearer token used as-is, never considered expired), **or** the OAuth2 client-credentials set: `CLIENT_ID`, `CLIENT_SECRET`, `TOKEN_URI` -- `PROJECT_NAME` — optional - `SCOPE` — against DataHub, needed when the realm uses Keycloak Organizations: that claim comes from a dynamic client scope, so the request must name it (`organization:*`, or `organization:` to pin one tenant). Without it the token carries no tenant and every call diff --git a/datahub_python_bindings/src/lib.rs b/datahub_python_bindings/src/lib.rs index 874c9c5..3a55d17 100644 --- a/datahub_python_bindings/src/lib.rs +++ b/datahub_python_bindings/src/lib.rs @@ -150,7 +150,6 @@ fn build_buffered_config( token_url: Option, client_id: Option, client_secret: Option, - project_name: Option, enable_buffering: bool, buffer_retention_secs: Option, buffer_max_bytes: Option, @@ -171,7 +170,6 @@ fn build_buffered_config( token_url, client_id, client_secret, - project_name, ); if let Some(secs) = buffer_retention_secs { config.set_buffer_retention_secs(secs); @@ -226,7 +224,6 @@ impl PySyncClient { token_url=None, client_id=None, client_secret=None, - project_name=None, enable_buffering=false, buffer_retention_secs=None, buffer_max_bytes=None, @@ -247,7 +244,6 @@ impl PySyncClient { token_url: Option, client_id: Option, client_secret: Option, - project_name: Option, enable_buffering: bool, buffer_retention_secs: Option, buffer_max_bytes: Option, @@ -269,7 +265,6 @@ impl PySyncClient { token_url, client_id, client_secret, - project_name, enable_buffering, buffer_retention_secs, buffer_max_bytes, @@ -404,7 +399,6 @@ impl PyAsyncClient { token_url=None, client_id=None, client_secret=None, - project_name=None, enable_buffering=false, buffer_retention_secs=None, buffer_max_bytes=None, @@ -425,7 +419,6 @@ impl PyAsyncClient { token_url: Option, client_id: Option, client_secret: Option, - project_name: Option, enable_buffering: bool, buffer_retention_secs: Option, buffer_max_bytes: Option, @@ -447,7 +440,6 @@ impl PyAsyncClient { token_url, client_id, client_secret, - project_name, enable_buffering, buffer_retention_secs, buffer_max_bytes, diff --git a/python_tests/README.md b/python_tests/README.md index 25f2ca7..20f250b 100644 --- a/python_tests/README.md +++ b/python_tests/README.md @@ -108,7 +108,7 @@ The fixtures build a client from an env file containing at least: - `BASE_URL` — backend root, e.g. `http://localhost:8081` - **either** `TOKEN` (a bearer token used as-is) **or** the OAuth2 client-credentials set - `CLIENT_ID` / `CLIENT_SECRET` / `TOKEN_URI` (optional `PROJECT_NAME`) + `CLIENT_ID` / `CLIENT_SECRET` / `TOKEN_URI` A gitignored `.env` already exists at the repo root. Make sure it points at a backend you can reach and that the credentials are valid. diff --git a/python_tests/test_auth_constructor.py b/python_tests/test_auth_constructor.py index 1070e82..c22f8af 100644 --- a/python_tests/test_auth_constructor.py +++ b/python_tests/test_auth_constructor.py @@ -57,7 +57,6 @@ def _client_credentials_kwargs(env): client_secret=client_secret, scope=env.get("SCOPE"), audience=env.get("AUDIENCE"), - project_name=env.get("PROJECT_NAME"), ) @@ -75,7 +74,6 @@ def _federated_kwargs(env): token_url=env["TOKEN_URI"], scope=env.get("SCOPE"), audience=env.get("AUDIENCE"), - project_name=env.get("PROJECT_NAME"), assertion=env.get("ASSERTION"), assertion_token_url=env.get("ASSERTION_TOKEN_URI"), assertion_client_id=env.get("ASSERTION_CLIENT_ID"), diff --git a/src/auth_diagnostics.rs b/src/auth_diagnostics.rs deleted file mode 100644 index 4e7dbda..0000000 --- a/src/auth_diagnostics.rs +++ /dev/null @@ -1,221 +0,0 @@ -//! Explaining a 401 that the API declines to explain. -//! -//! DataHub resolves the caller's tenant from the access token's `organization` claim, and rejects -//! the token outright when that claim is missing, malformed, or names more than one organization. -//! The reason never reaches the caller: the API installs a custom authentication entry point that -//! answers with a bare `WWW-Authenticate: Bearer realm="Restricted Content"` and a generic body, -//! logging the real reason server-side only. What arrives here is -//! `ResponseError { status: 401, message: "" }` — indistinguishable from a rotated secret, which -//! is why the usual first response is to go and rotate the secret. -//! -//! The token itself carries the answer, so the SDK reconstructs it locally: decode the payload, -//! look at `organization`, and report which case the server would have hit. -//! -//! # This discloses nothing -//! -//! Only the caller's *own* token is inspected — one the SDK already holds in memory — and a JWT -//! payload is base64, not encrypted, so its holder can read it at any time (`cut -d. -f2 | -//! base64 -d`). Nothing crosses the network, no signature is verified, and no server-side state is -//! consulted. This is diagnosis, never an access decision: the API remains the only thing that -//! decides whether a token is acceptable, and a token this module considers well-formed can still -//! be rejected for reasons it cannot see (expiry, revocation, audience, signature). - -use base64::engine::general_purpose::URL_SAFE_NO_PAD; -use base64::Engine; - -/// What the `organization` claim of a token looks like, in the terms the API's -/// `OrganizationValidator` reasons about. -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) enum OrganizationClaim { - /// Not a readable JWT — an opaque token supplied through `TOKEN`, say. Nothing to say about it. - NotAJwt, - /// A JWT, but with no `organization` claim: the token request asked for no organization scope, - /// or the realm emits the claim only under a selector. - Absent, - /// Present but empty (`{}`), which the API treats the same as absent. - Empty, - /// A JSON array rather than an object. Keycloak merges same-named multivalued claims, so this - /// means two mappers are writing `organization`, or the membership mapper has - /// `addOrganizationId` switched off and is emitting bare aliases. - Array, - /// Exactly one organization — the shape the API accepts. - One, - /// Several organizations. The API refuses to guess which tenant is meant. - Many(Vec), -} - -/// Inspect the `organization` claim of `token`. Best-effort and infallible: anything that does not -/// parse is [`OrganizationClaim::NotAJwt`], because the alternative — guessing — would blame the -/// wrong thing for an opaque token. -pub(crate) fn inspect_organization_claim(token: &str) -> OrganizationClaim { - // A JWS is `header.payload.signature`; we want the middle segment and never the signature. - let Some(payload) = token.split('.').nth(1).filter(|_| token.split('.').count() == 3) else { - return OrganizationClaim::NotAJwt; - }; - let Ok(bytes) = URL_SAFE_NO_PAD.decode(payload) else { - return OrganizationClaim::NotAJwt; - }; - let Ok(claims) = serde_json::from_slice::(&bytes) else { - return OrganizationClaim::NotAJwt; - }; - if !claims.is_object() { - return OrganizationClaim::NotAJwt; - } - - match claims.get("organization") { - None | Some(serde_json::Value::Null) => OrganizationClaim::Absent, - Some(serde_json::Value::Array(_)) => OrganizationClaim::Array, - Some(serde_json::Value::Object(orgs)) if orgs.is_empty() => OrganizationClaim::Empty, - Some(serde_json::Value::Object(orgs)) if orgs.len() == 1 => OrganizationClaim::One, - Some(serde_json::Value::Object(orgs)) => { - // Sorted: a JSON object's key order is not meaningful (and depends on whether - // serde_json is built with `preserve_order`), while an error message that reshuffles - // between runs is hard to grep for and hard to assert on. - let mut aliases: Vec = orgs.keys().cloned().collect(); - aliases.sort(); - OrganizationClaim::Many(aliases) - } - // Any other JSON type is malformed in a way the API also rejects, and "array" is the - // closest actionable advice we have. - Some(_) => OrganizationClaim::Array, - } -} - -/// A sentence explaining why this token's tenant could not be resolved, or `None` when the claim is -/// well-formed and the 401 must have another cause. -pub(crate) fn organization_hint(token: &str) -> Option { - let advice = match inspect_organization_claim(token) { - OrganizationClaim::NotAJwt | OrganizationClaim::One => return None, - OrganizationClaim::Absent => "the access token carries no `organization` claim, so the \ - server cannot tell which tenant you mean. DataHub resolves the tenant from that \ - claim; set the OAuth2 scope (`SCOPE=organization:*`, or `organization:` to \ - pin one) if your realm emits it under a selector." - .to_string(), - OrganizationClaim::Empty => "the access token's `organization` claim is empty, so the \ - server cannot tell which tenant you mean. Check that this principal is a member of \ - an organization, and that the token request asks for the organization scope." - .to_string(), - OrganizationClaim::Array => "the access token's `organization` claim is a JSON array, \ - which the server rejects. Two protocol mappers are writing that claim, or the \ - organization membership mapper has `addOrganizationId` switched off." - .to_string(), - OrganizationClaim::Many(aliases) => { - format!( - "the access token names {} organizations ({}), and the server will not choose one \ - for you. Pin a single tenant with `SCOPE=organization:`.", - aliases.len(), - aliases.join(", ") - ) - } - }; - Some(format!( - "{advice} (Diagnosed by the SDK from your own token; the server does not report this.)" - )) -} - -#[cfg(test)] -mod tests { - use super::*; - use serde_json::json; - - /// A token whose payload is `claims`. The header and signature are junk on purpose — nothing - /// here verifies them, and a test that supplied a real signature would imply otherwise. - fn jwt(claims: serde_json::Value) -> String { - format!( - "aGVhZGVy.{}.c2ln", - URL_SAFE_NO_PAD.encode(claims.to_string()) - ) - } - - #[test] - fn a_single_organization_is_well_formed_and_gets_no_hint() { - let token = jwt(json!({"organization": {"acme": {"id": "abc"}}})); - assert_eq!(inspect_organization_claim(&token), OrganizationClaim::One); - // The token is fine, so whatever caused the 401 is something this module can't see — - // inventing an explanation would send the reader down the wrong path. - assert_eq!(organization_hint(&token), None); - } - - #[test] - fn several_organizations_are_named_in_the_message() { - let token = jwt(json!({"organization": { - "beta": {"id": "2"}, - "acme": {"id": "1"}, - }})); - // Sorted, not in the order they appeared in the claim. - assert_eq!( - inspect_organization_claim(&token), - OrganizationClaim::Many(vec!["acme".into(), "beta".into()]) - ); - - let hint = organization_hint(&token).expect("a two-organization token should be explained"); - assert!(hint.contains("names 2 organizations"), "{hint}"); - // Sorted, so the message is stable across runs rather than following JSON key order. - assert!(hint.contains("(acme, beta)"), "{hint}"); - // The actionable part: what to actually change. - assert!(hint.contains("SCOPE=organization:"), "{hint}"); - } - - #[test] - fn a_missing_claim_points_at_the_scope() { - let token = jwt(json!({"sub": "service-account-x"})); - assert_eq!(inspect_organization_claim(&token), OrganizationClaim::Absent); - let hint = organization_hint(&token).unwrap(); - assert!(hint.contains("no `organization` claim"), "{hint}"); - assert!(hint.contains("SCOPE=organization:*"), "{hint}"); - } - - #[test] - fn a_null_claim_is_treated_as_absent() { - // Keycloak emits `"organization": null` for a principal with no resolvable membership. - let token = jwt(json!({"organization": null})); - assert_eq!(inspect_organization_claim(&token), OrganizationClaim::Absent); - assert!(organization_hint(&token).is_some()); - } - - #[test] - fn an_empty_claim_is_reported_separately_from_a_missing_one() { - let token = jwt(json!({"organization": {}})); - assert_eq!(inspect_organization_claim(&token), OrganizationClaim::Empty); - let hint = organization_hint(&token).unwrap(); - assert!(hint.contains("is empty"), "{hint}"); - } - - #[test] - fn an_array_claim_points_at_the_mapper_configuration() { - // What two mappers writing the same claim produces — the shape that cost this project a - // day: one mapper emitting bare aliases, another the nested object, merged by Keycloak. - let token = jwt(json!({"organization": ["acme", {"acme": {"id": "1"}}]})); - assert_eq!(inspect_organization_claim(&token), OrganizationClaim::Array); - let hint = organization_hint(&token).unwrap(); - assert!(hint.contains("JSON array"), "{hint}"); - assert!(hint.contains("addOrganizationId"), "{hint}"); - } - - #[test] - fn a_flat_alias_array_is_also_reported_as_an_array() { - // `addOrganizationId` off, single mapper: `["acme"]`. Same rejection, same advice. - let token = jwt(json!({"organization": ["acme"]})); - assert_eq!(inspect_organization_claim(&token), OrganizationClaim::Array); - } - - #[test] - fn an_opaque_token_is_never_speculated_about() { - // A user-supplied `TOKEN=` need not be a JWT. Reporting "you have no organization claim" - // for one would blame the wrong thing entirely. - for opaque in ["", "not-a-jwt", "two.parts", "a.b.c.d", "aGVhZGVy.!!!.c2ln"] { - assert_eq!( - inspect_organization_claim(opaque), - OrganizationClaim::NotAJwt, - "{opaque:?} should not be treated as a JWT" - ); - assert_eq!(organization_hint(opaque), None, "{opaque:?}"); - } - } - - #[test] - fn a_payload_that_is_not_a_json_object_is_not_a_jwt() { - let token = format!("aGVhZGVy.{}.c2ln", URL_SAFE_NO_PAD.encode("[1,2,3]")); - assert_eq!(inspect_organization_claim(&token), OrganizationClaim::NotAJwt); - } -} diff --git a/src/buffer_integration.rs b/src/buffer_integration.rs index f6fd6b4..fe56bf5 100644 --- a/src/buffer_integration.rs +++ b/src/buffer_integration.rs @@ -55,7 +55,6 @@ fn unreachable_buffered_service(dir: &PathBuf) -> Arc { None, None, None, - None, ); config .set_buffer_dir(dir.clone()) diff --git a/src/datahub.rs b/src/datahub.rs index c0984e3..eadfaec 100644 --- a/src/datahub.rs +++ b/src/datahub.rs @@ -94,8 +94,6 @@ pub struct OAuthConfig { #[serde(alias = "ASSERTION_GRANT")] pub(crate) assertion_grant: Option, - #[serde(alias = "PROJECT_NAME")] - pub(crate) project_name: Option, } impl OAuthConfig { @@ -201,7 +199,6 @@ impl DataHubConfig { token_uri: Option, client_id: Option, client_secret: Option, - project_name: Option, ) -> DataHubConfig { let oauthconfig = OAuthConfig { client_id, @@ -216,7 +213,6 @@ impl DataHubConfig { assertion_scope: None, assertion_audience: None, assertion_grant: None, - project_name, }; // Oauth client will only be configured if all required fields are present @@ -851,7 +847,6 @@ mod jwt_bearer_tests { Some(token_uri), Some("datahub-jwt-grant".to_string()), Some("kc-secret".to_string()), - None, ) } @@ -915,7 +910,6 @@ mod jwt_bearer_tests { Some(url), Some("datahub-exchange".to_string()), None, - None, ); api.set_assertion("header.payload.signature"); let token = api.get_api_token().await.unwrap(); @@ -952,7 +946,6 @@ mod jwt_bearer_tests { Some(url), None, None, - None, ); api.set_assertion("header.payload.signature"); api.set_assertion_grant("jwt-bearer"); @@ -980,7 +973,6 @@ mod jwt_bearer_tests { Some("http://127.0.0.1:1".to_string()), None, None, - None, ); api.set_assertion("header.payload.signature"); api.set_assertion_grant("password"); diff --git a/src/generic.rs b/src/generic.rs index 9822e1e..a48259c 100644 --- a/src/generic.rs +++ b/src/generic.rs @@ -691,11 +691,11 @@ pub trait ApiServiceProvider { /// for its whole lifetime. Clearing it here means the next call mints a fresh one, so a /// client that started a few seconds too early recovers on its next attempt instead of /// re-sending the same rejected credential until it expires. - async fn on_request_error(&self, error: ResponseError, token: &str) -> ResponseError { + async fn on_request_error(&self, error: ResponseError, _token: &str) -> ResponseError { if error.get_status() == http::StatusCode::UNAUTHORIZED { self.get_api_service().config.invalidate_token().await; } - explain_auth_failure(error, token) + error } async fn get_token(&self) -> Result { @@ -902,50 +902,14 @@ pub trait ApiServiceProvider { .get(reqwest::header::CONTENT_TYPE) .and_then(|value| value.to_str().ok()) .map(str::to_string); - Err(explain_auth_failure( - ResponseError { - status, - message: response - .text() - .await - .unwrap_or_else(|_| "Failed to read response body".to_string()), - content_type, - }, - &token, - )) - } -} - -/// Add a reason to a 401 that arrived without one. -/// -/// The API rejects a token whose `organization` claim is missing, malformed or ambiguous, but its -/// authentication entry point sends no `error_description` and an empty body — so the caller gets -/// `401` and nothing else, which reads as a bad credential. The token the SDK just sent carries -/// enough to say which it was; see [`crate::auth_diagnostics`] for why reading it discloses -/// nothing. -/// -/// Anything already explained is left alone: a non-401, or a 401 that did come with a body, keeps -/// its own message, and a well-formed claim adds nothing (the 401 then has a cause this cannot -/// see — expiry, revocation, audience, signature). -fn explain_auth_failure(error: ResponseError, token: &str) -> ResponseError { - if error.get_status() != http::StatusCode::UNAUTHORIZED { - return error; - } - let Some(hint) = crate::auth_diagnostics::organization_hint(token) else { - return error; - }; - let existing = error.get_message(); - let message = if existing.trim().is_empty() { - hint - } else { - format!("{existing} — {hint}") - }; - ResponseError { - status: error.get_status(), - message, - // Preserved: appending the organization hint does not change what the server sent, and - // dropping it here would make an explained 401 look like one that never reached the wire. - content_type: error.content_type().map(str::to_string), + Err(ResponseError { + status, + message: response + .text() + .await + .unwrap_or_else(|_| "Failed to read response body".to_string()), + content_type, + }) } } @@ -1148,79 +1112,6 @@ mod search_body_tests { } } -#[cfg(test)] -mod auth_failure_tests { - use super::explain_auth_failure; - use crate::http::ResponseError; - use base64::engine::general_purpose::URL_SAFE_NO_PAD; - use base64::Engine; - use oauth2::http::StatusCode; - - fn jwt(payload: &str) -> String { - format!("aGVhZGVy.{}.c2ln", URL_SAFE_NO_PAD.encode(payload)) - } - - fn error(code: u16, message: &str) -> ResponseError { - ResponseError { - status: StatusCode::from_u16(code).unwrap(), - message: message.to_string(), - content_type: None, - } - } - - #[test] - fn an_unexplained_401_gains_the_reason_the_server_withheld() { - // What a caller in two organizations actually gets back: 401, empty body, no - // `WWW-Authenticate` detail. Without this the only signal is "401", which reads as a bad - // secret and sends people off to rotate credentials. - let token = jwt(r#"{"organization":{"beta":{"id":"2"},"acme":{"id":"1"}}}"#); - let explained = explain_auth_failure(error(401, ""), &token); - - assert_eq!(explained.get_status(), StatusCode::UNAUTHORIZED, "status is untouched"); - let message = explained.get_message(); - assert!(message.contains("names 2 organizations"), "{message}"); - assert!(message.contains("acme, beta"), "{message}"); - assert!(message.contains("SCOPE=organization:"), "{message}"); - } - - #[test] - fn a_401_that_came_with_a_body_keeps_it() { - // Should the API ever start explaining itself, its words win and ours are appended — - // never replace a real server message with a guess. - let token = jwt(r#"{"organization":{"acme":{"id":"1"},"beta":{"id":"2"}}}"#); - let explained = explain_auth_failure(error(401, "Bearer token expired"), &token); - let message = explained.get_message(); - assert!(message.starts_with("Bearer token expired"), "{message}"); - assert!(message.contains("names 2 organizations"), "{message}"); - } - - #[test] - fn a_well_formed_token_is_left_alone() { - // Exactly one organization, so the 401 has some other cause (expiry, revocation, audience, - // signature). Volunteering an organization explanation here would misdirect the reader. - let token = jwt(r#"{"organization":{"acme":{"id":"1"}}}"#); - let explained = explain_auth_failure(error(401, ""), &token); - assert_eq!(explained.get_message(), ""); - } - - #[test] - fn an_opaque_token_is_left_alone() { - // A user-supplied `TOKEN=` need not be a JWT at all. - let explained = explain_auth_failure(error(401, ""), "an-opaque-api-key"); - assert_eq!(explained.get_message(), ""); - } - - #[test] - fn only_401s_are_touched() { - // A dataset-ACL 403 already carries a problem+json body explaining itself; appending - // organization advice to it would be noise, and wrong. - let token = jwt(r#"{"organization":{"acme":{"id":"1"},"beta":{"id":"2"}}}"#); - for code in [400u16, 403, 404, 500] { - let explained = explain_auth_failure(error(code, "original"), &token); - assert_eq!(explained.get_message(), "original", "{code} should pass through"); - } - } -} #[cfg(test)] mod delete_filter_tests { diff --git a/src/lib.rs b/src/lib.rs index 23fd824..a6c1212 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,7 +18,6 @@ pub use crate::relations::EdgesService; pub use crate::subscriptions::SubscriptionsService; /// Explaining an unexplained 401 from the token the SDK already holds. -pub(crate) mod auth_diagnostics; #[cfg(feature = "blocking")] pub mod blocking; pub mod assets; diff --git a/src/multi_tenant_integration.rs b/src/multi_tenant_integration.rs index 69baa2b..e5206a7 100644 --- a/src/multi_tenant_integration.rs +++ b/src/multi_tenant_integration.rs @@ -66,11 +66,9 @@ //! server's own words it reads. `WWW-Authenticate` still carries no `error_description`; the //! reason is in the body. //! -//! [`crate::auth_diagnostics`] reconstructs the same reason from the token the SDK just sent and -//! appends it, so the message a caller sees can carry both. Every 401 here shares the -//! `unauthorized` slug, so the cause is only in the prose: a caller cannot branch on `type` to -//! tell "no token" from "ambiguous organization". Cases that differ only by fixture are asserted -//! on **status alone**. +//! Every 401 here shares the `unauthorized` slug, so the cause is only in the prose: a caller +//! cannot branch on `type` to tell "no token" from "ambiguous organization". Cases that differ +//! only by fixture are asserted on **status alone**. //! //! (403s are different: those carry a real RFC 9457 `problem+json` body from the server, with //! `dataSetId` and `permission`, which the ACL tests assert on directly.) @@ -283,7 +281,6 @@ fn principal(test: &str, prefix: &str, scope: Option<&str>) -> Option Some(token_uri), Some(client_id.clone()), Some(client_secret), - None, ); if let Some(scope) = scope { config.set_scope(scope); @@ -463,10 +460,9 @@ async fn multi_tenant_multi_org_principal_with_wildcard_scope_is_rejected( "a token naming two organizations", ); - // The reason reaches the caller two ways now: the server writes it into the problem's - // `detail`, and `crate::auth_diagnostics` appends its own from the token just used. Asserting - // it against a real two-organization token from a real realm is what proves the explanation - // survives the whole path, entry point to `ResponseError`. + // The reason is the server's own: the organization validator writes it into the problem's + // `detail`. Asserting it against a real two-organization token from a real realm is what + // proves the explanation survives the whole path, entry point to `ResponseError`. let message = error.get_message(); assert!( message.contains("names 2 organizations"), @@ -484,7 +480,7 @@ async fn multi_tenant_multi_org_principal_with_wildcard_scope_is_rejected( } } assert!( - message.contains("SCOPE=organization:"), + message.contains("scope=organization:"), "the message should say what to change — got {message:?}" ); Ok(()) diff --git a/src/problem_integration.rs b/src/problem_integration.rs index 35a5b81..c1fc888 100644 --- a/src/problem_integration.rs +++ b/src/problem_integration.rs @@ -550,9 +550,9 @@ mod unified { } /// An unauthenticated call answers 401 with a full problem document — `type`, `detail`, - /// `requestId` and `retry` — rather than the empty body it used to send. That empty body is - /// why [`crate::auth_diagnostics`] exists, reconstructing the reason from the token just - /// sent; the server now says it directly. + /// `requestId` and `retry` — rather than the empty body it used to send. The SDK carried an + /// `auth_diagnostics` module to reconstruct that reason from the token it had just sent; the + /// server saying it directly is what retired it. #[tokio::test] async fn an_unauthenticated_call_says_why() { let refusal = Probe::new().get_unauthenticated("/timeseries?limit=1").await; diff --git a/src/timeseries/mod.rs b/src/timeseries/mod.rs index bcb837c..0c276e1 100644 --- a/src/timeseries/mod.rs +++ b/src/timeseries/mod.rs @@ -882,7 +882,6 @@ mod unbuffered_insert_tests { None, None, None, - None, ); if let Some(parallelism) = parallelism { config.set_datapoint_insert_parallelism(parallelism); From b0df19e4b0dde4e1b7b2902017dda6a627f92a85 Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Wed, 23 Sep 2026 13:34:31 +0200 Subject: [PATCH 2/2] fix: drop the doc comment orphaned by removing auth_diagnostics Deleting the `mod auth_diagnostics;` line left its `///` behind, so it bound to the next item and documented the `blocking` module as "Explaining an unexplained 401 from the token the SDK already holds." That would have shipped to docs.rs as the blocking client's description. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: jgjesdal --- src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index a6c1212..eedb1a6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,7 +17,6 @@ pub use crate::labels::LabelsService; pub use crate::relations::EdgesService; pub use crate::subscriptions::SubscriptionsService; -/// Explaining an unexplained 401 from the token the SDK already holds. #[cfg(feature = "blocking")] pub mod blocking; pub mod assets;