From 2abc8fb3b57f7b80033df7aa782afb641e52c893 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Wed, 4 Nov 2026 11:31:06 +0800 Subject: [PATCH 1/9] fix(oauth): serialize revoke with token refresh; guard Java revoke/clear against close - Rust: OAuth::revoke holds the token lock for the whole request, so a concurrent access_token refresh cannot rotate the refresh token between reading and revoking it (revoking the stale token while the new one stays valid) - Java: OAuth.revoke()/clear() are synchronized with close(), so a concurrent close() cannot free the native handle before the JNI call clones it (use-after-free); their errors now surface as OpenApiException like other SDK calls --- CHANGELOG.md | 4 ++-- .../src/main/java/com/longbridge/OAuth.java | 6 +++-- java/src/oauth.rs | 4 ++-- rust/crates/oauth/src/client.rs | 23 +++++++++++-------- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7928c97e4..ba44a4359 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- **All SDKs:** `OAuth.revoke()` revokes the current token on the server through the existing RFC 7009 endpoint (`POST /oauth2/revoke`): the refresh token when present, otherwise the access token. It only contacts the server; the in-memory and cached tokens are left untouched. On a client created with `from_token` (no client ID) it fails with a `RevokeFailed` error instead of calling the server. +- **All SDKs:** `OAuth.revoke()` revokes the current token on the server through the existing RFC 7009 endpoint (`POST /oauth2/revoke`): the refresh token when present, otherwise the access token. It only contacts the server; the in-memory and cached tokens are left untouched. On a client created with `from_token` (no client ID) it fails with a `RevokeFailed` error instead of calling the server. The token lock is held for the whole request, so a concurrent token refresh cannot rotate the refresh token in between (which would revoke the stale token and leave the new one valid). - **All SDKs:** `OAuth.clear()` drops the token locally: it clears the in-memory copy and deletes the cached token file (`~/.longbridge/openapi/tokens/`; a missing file is not an error). It does not contact the server, so call `revoke()` first to disconnect the application completely. After `clear()`, `access_token()` fails with `NoTokenAvailable`. The persisted token is deleted first and the in-memory token is cleared only when that succeeds, so a failed delete leaves the handle usable and the error is returned. - - Rust: `OAuth::revoke` / `OAuth::clear` (async), `OAuthError::RevokeFailed`, `OAuthError::TokenFileDelete`. C: `lb_oauth_revoke` / `lb_oauth_clear`. C++: `OAuth::revoke` / `OAuth::clear`. Java: `OAuth.revoke()` / `OAuth.clear()` returning `CompletableFuture`. Node.js: `oauth.revoke()` / `oauth.clear()` returning promises. Python: `OAuth.revoke()` / `OAuth.clear()` plus `revoke_async()` / `clear_async()`; the `openapi.pyi` stub is updated. The blocking Python calls release the GIL while waiting. In C/C++, a NULL handle (e.g. a moved-from C++ `OAuth`) is reported through the callback as an error instead of crashing. + - Rust: `OAuth::revoke` / `OAuth::clear` (async), `OAuthError::RevokeFailed`, `OAuthError::TokenFileDelete`. C: `lb_oauth_revoke` / `lb_oauth_clear`. C++: `OAuth::revoke` / `OAuth::clear`. Java: `OAuth.revoke()` / `OAuth.clear()` returning `CompletableFuture`, which fails with `OpenApiException` like other SDK calls; both are `synchronized` with `close()`, so a concurrent `close()` cannot free the native handle while it is in use (a closed handle throws `IllegalStateException`). Node.js: `oauth.revoke()` / `oauth.clear()` returning promises. Python: `OAuth.revoke()` / `OAuth.clear()` plus `revoke_async()` / `clear_async()`; the `openapi.pyi` stub is updated. The blocking Python calls release the GIL while waiting. In C/C++, a NULL handle (e.g. a moved-from C++ `OAuth`) is reported through the callback as an error instead of crashing. - **Rust:** `TokenStorage::delete(client_id)` removes a persisted token. It has a default implementation that returns an error, so existing custom storage backends keep compiling but `OAuth::clear` fails loudly (and keeps the in-memory token) instead of reporting success without wiping anything; override it to support `clear`. `FileTokenStorage` deletes the token file. ### Breaking changes diff --git a/java/javasrc/src/main/java/com/longbridge/OAuth.java b/java/javasrc/src/main/java/com/longbridge/OAuth.java index 787637bc5..3538e65e2 100644 --- a/java/javasrc/src/main/java/com/longbridge/OAuth.java +++ b/java/javasrc/src/main/java/com/longbridge/OAuth.java @@ -57,8 +57,9 @@ long getRaw() { * (see {@link #clear()}). * * @return A Future that completes when the token has been revoked + * @throws IllegalStateException If the handle has already been closed */ - public CompletableFuture revoke() { + public synchronized CompletableFuture revoke() { long r = getRaw(); return AsyncCallback.executeTask((callback) -> { SdkNative.oauthRevoke(r, callback); @@ -74,8 +75,9 @@ public CompletableFuture revoke() { * contacted (see {@link #revoke()}). * * @return A Future that completes when the local token has been cleared + * @throws IllegalStateException If the handle has already been closed */ - public CompletableFuture clear() { + public synchronized CompletableFuture clear() { long r = getRaw(); return AsyncCallback.executeTask((callback) -> { SdkNative.oauthClear(r, callback); diff --git a/java/src/oauth.rs b/java/src/oauth.rs index 66d75e518..5668ada8c 100644 --- a/java/src/oauth.rs +++ b/java/src/oauth.rs @@ -81,7 +81,7 @@ pub unsafe extern "system" fn Java_com_longbridge_SdkNative_oauthRevoke( oauth .revoke() .await - .map_err(|e| crate::error::JniError::Other(e.to_string())) + .map_err(|e| longbridge::Error::OAuth(e.to_string()).into()) })?; Ok(()) }) @@ -101,7 +101,7 @@ pub unsafe extern "system" fn Java_com_longbridge_SdkNative_oauthClear( oauth .clear() .await - .map_err(|e| crate::error::JniError::Other(e.to_string())) + .map_err(|e| longbridge::Error::OAuth(e.to_string()).into()) })?; Ok(()) }) diff --git a/rust/crates/oauth/src/client.rs b/rust/crates/oauth/src/client.rs index cb70f4363..51e7b634f 100644 --- a/rust/crates/oauth/src/client.rs +++ b/rust/crates/oauth/src/client.rs @@ -127,6 +127,11 @@ impl OAuth { /// The refresh token is revoked when present, otherwise the access token. /// This only calls the server: the in-memory and persisted tokens are left /// untouched. Call [`clear`](OAuth::clear) afterwards to drop them locally. + /// + /// The token lock is held for the whole request, so a concurrent + /// [`access_token`](OAuth::access_token) refresh cannot rotate the refresh + /// token between reading and revoking it (which would revoke a stale token + /// while the new one stays valid). pub async fn revoke(&self) -> OAuthResult<()> { // `from_token` clients have no client ID: the token was issued // elsewhere, so the server cannot revoke it on our behalf. @@ -138,19 +143,16 @@ impl OAuth { }); } - let token = self - .0 - .token - .lock() - .await - .clone() - .ok_or(OAuthError::NoTokenAvailable)?; + let guard = self.0.token.lock().await; + let token = guard.as_ref().ok_or(OAuthError::NoTokenAvailable)?; - let revocable = match token.refresh_token { + let revocable = match &token.refresh_token { Some(refresh_token) => { - StandardRevocableToken::RefreshToken(RefreshToken::new(refresh_token)) + StandardRevocableToken::RefreshToken(RefreshToken::new(refresh_token.clone())) + } + None => { + StandardRevocableToken::AccessToken(AccessToken::new(token.access_token.clone())) } - None => StandardRevocableToken::AccessToken(AccessToken::new(token.access_token)), }; tracing::debug!(client_id = %self.0.client_id, "revoking OAuth token"); @@ -173,6 +175,7 @@ impl OAuth { OAuthError::RevokeFailed { message } })?; + drop(guard); tracing::info!(client_id = %self.0.client_id, "token revoked"); Ok(()) } From 623df05887f50b2d1fb3bd7190fe54df01a9ffc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Wed, 4 Nov 2026 11:48:17 +0800 Subject: [PATCH 2/9] fix(oauth): bound revoke with a timeout and test that it holds the token lock - the revocation request now runs under a 30s timeout: it holds the token lock, so a hung request would otherwise block access_token() and every request that needs a token - build the oauth2 client (which may probe the region) before taking the lock - split out revoke_with() taking an injectable HTTP client and add a test asserting the lock is held while the request is in flight --- CHANGELOG.md | 2 +- rust/crates/oauth/src/client.rs | 98 ++++++++++++++++++++++++++++----- 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba44a4359..b709d988f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- **All SDKs:** `OAuth.revoke()` revokes the current token on the server through the existing RFC 7009 endpoint (`POST /oauth2/revoke`): the refresh token when present, otherwise the access token. It only contacts the server; the in-memory and cached tokens are left untouched. On a client created with `from_token` (no client ID) it fails with a `RevokeFailed` error instead of calling the server. The token lock is held for the whole request, so a concurrent token refresh cannot rotate the refresh token in between (which would revoke the stale token and leave the new one valid). +- **All SDKs:** `OAuth.revoke()` revokes the current token on the server through the existing RFC 7009 endpoint (`POST /oauth2/revoke`): the refresh token when present, otherwise the access token. It only contacts the server; the in-memory and cached tokens are left untouched. On a client created with `from_token` (no client ID) it fails with a `RevokeFailed` error instead of calling the server. The token lock is held for the whole request, so a concurrent token refresh cannot rotate the refresh token in between (which would revoke the stale token and leave the new one valid). While it runs, `access_token()` and every request that needs a token wait for it; the request is bounded by a 30-second timeout (`RevokeFailed` on expiry). - **All SDKs:** `OAuth.clear()` drops the token locally: it clears the in-memory copy and deletes the cached token file (`~/.longbridge/openapi/tokens/`; a missing file is not an error). It does not contact the server, so call `revoke()` first to disconnect the application completely. After `clear()`, `access_token()` fails with `NoTokenAvailable`. The persisted token is deleted first and the in-memory token is cleared only when that succeeds, so a failed delete leaves the handle usable and the error is returned. - Rust: `OAuth::revoke` / `OAuth::clear` (async), `OAuthError::RevokeFailed`, `OAuthError::TokenFileDelete`. C: `lb_oauth_revoke` / `lb_oauth_clear`. C++: `OAuth::revoke` / `OAuth::clear`. Java: `OAuth.revoke()` / `OAuth.clear()` returning `CompletableFuture`, which fails with `OpenApiException` like other SDK calls; both are `synchronized` with `close()`, so a concurrent `close()` cannot free the native handle while it is in use (a closed handle throws `IllegalStateException`). Node.js: `oauth.revoke()` / `oauth.clear()` returning promises. Python: `OAuth.revoke()` / `OAuth.clear()` plus `revoke_async()` / `clear_async()`; the `openapi.pyi` stub is updated. The blocking Python calls release the GIL while waiting. In C/C++, a NULL handle (e.g. a moved-from C++ `OAuth`) is reported through the callback as an error instead of crashing. - **Rust:** `TokenStorage::delete(client_id)` removes a persisted token. It has a default implementation that returns an error, so existing custom storage backends keep compiling but `OAuth::clear` fails loudly (and keeps the in-memory token) instead of reporting success without wiping anything; override it to support `clear`. `FileTokenStorage` deletes the token file. diff --git a/rust/crates/oauth/src/client.rs b/rust/crates/oauth/src/client.rs index 51e7b634f..657f408a7 100644 --- a/rust/crates/oauth/src/client.rs +++ b/rust/crates/oauth/src/client.rs @@ -1,11 +1,11 @@ -use std::{fmt, sync::Arc}; +use std::{fmt, future::Future, sync::Arc, time::Duration}; use longbridge_geo::is_cn; use oauth2::{ - AccessToken, AuthUrl, AuthorizationCode, ClientId, CsrfToken, PkceCodeChallenge, - PkceCodeVerifier, RedirectUrl, RefreshToken, RequestTokenError, RevocationErrorResponseType, - RevocationUrl, Scope, StandardErrorResponse, StandardRevocableToken, TokenUrl, - basic::BasicClient, reqwest::async_http_client, + AccessToken, AuthUrl, AuthorizationCode, ClientId, CsrfToken, HttpRequest, HttpResponse, + PkceCodeChallenge, PkceCodeVerifier, RedirectUrl, RefreshToken, RequestTokenError, + RevocationErrorResponseType, RevocationUrl, Scope, StandardErrorResponse, + StandardRevocableToken, TokenUrl, basic::BasicClient, reqwest::async_http_client, }; use poem::listener::{Acceptor, Listener, TcpListener}; @@ -131,7 +131,9 @@ impl OAuth { /// The token lock is held for the whole request, so a concurrent /// [`access_token`](OAuth::access_token) refresh cannot rotate the refresh /// token between reading and revoking it (which would revoke a stale token - /// while the new one stays valid). + /// while the new one stays valid). While it runs, `access_token` (and every + /// request that needs a token) waits; the request is therefore bounded by a + /// 30-second timeout. pub async fn revoke(&self) -> OAuthResult<()> { // `from_token` clients have no client ID: the token was issued // elsewhere, so the server cannot revoke it on our behalf. @@ -143,6 +145,23 @@ impl OAuth { }); } + // Built before taking the lock: the first call may probe the region. + let client = create_oauth_client( + &self.0.client_id, + &format!("http://localhost:{}/callback", self.0.callback_port), + ) + .await; + self.revoke_with(&client, async_http_client).await + } + + /// Revoke through `http_client` (injectable for tests), holding the token + /// lock for the whole request. + async fn revoke_with(&self, client: &BasicClient, http_client: C) -> OAuthResult<()> + where + C: FnOnce(HttpRequest) -> F, + F: Future>, + RE: std::error::Error + 'static, + { let guard = self.0.token.lock().await; let token = guard.as_ref().ok_or(OAuthError::NoTokenAvailable)?; @@ -157,18 +176,20 @@ impl OAuth { tracing::debug!(client_id = %self.0.client_id, "revoking OAuth token"); - let client = create_oauth_client( - &self.0.client_id, - &format!("http://localhost:{}/callback", self.0.callback_port), - ) - .await; - client + let request = client .revoke_token(revocable) .map_err(|e| OAuthError::RevokeFailed { message: e.to_string(), })? - .request_async(async_http_client) + .request_async(http_client); + tokio::time::timeout(REVOKE_TIMEOUT, request) .await + .map_err(|_| { + tracing::error!(client_id = %self.0.client_id, "token revocation timed out"); + OAuthError::RevokeFailed { + message: format!("timed out after {}s", REVOKE_TIMEOUT.as_secs()), + } + })? .map_err(|e| { let message = revoke_error_message(&e); tracing::error!(client_id = %self.0.client_id, error = %message, "failed to revoke token"); @@ -296,6 +317,9 @@ impl OAuth { } } +/// Upper bound for a revocation request, which holds the token lock. +const REVOKE_TIMEOUT: Duration = Duration::from_secs(30); + /// Describe a revocation failure, including the server's `error` and /// `error_description` (the `Display` of `ServerResponse` omits them). fn revoke_error_message( @@ -465,6 +489,54 @@ mod tests { })) } + #[tokio::test] + async fn revoke_holds_token_lock_for_the_whole_request() { + let oauth = OAuth(Arc::new(OAuthInner { + client_id: "my-client".to_string(), + callback_port: DEFAULT_CALLBACK_PORT, + storage: Arc::new(LegacyStorage), + token: tokio::sync::Mutex::new(Some(OAuthToken { + client_id: "my-client".to_string(), + access_token: "access".to_string(), + refresh_token: Some("refresh".to_string()), + expires_at: u64::MAX, + })), + })); + let client = build_oauth_client( + OAUTH_BASE_URL, + "my-client", + "http://localhost:60355/callback", + ); + + let (started_tx, started_rx) = tokio::sync::oneshot::channel::(); + let (release_tx, release_rx) = tokio::sync::oneshot::channel::<()>(); + let http_client = move |req: HttpRequest| async move { + started_tx.send(req).unwrap(); + release_rx.await.unwrap(); + Ok::<_, std::io::Error>(HttpResponse { + status_code: oauth2::http::StatusCode::OK, + headers: Default::default(), + body: Vec::new(), + }) + }; + + let task = { + let oauth = oauth.clone(); + tokio::spawn(async move { oauth.revoke_with(&client, http_client).await }) + }; + + let req = started_rx.await.unwrap(); + let body = String::from_utf8(req.body).unwrap(); + assert!(body.contains("token=refresh"), "{body}"); + assert!(body.contains("token_type_hint=refresh_token"), "{body}"); + // A refresh (access_token) cannot run while the request is in flight. + assert!(oauth.0.token.try_lock().is_err()); + + release_tx.send(()).unwrap(); + task.await.unwrap().unwrap(); + assert!(oauth.0.token.try_lock().is_ok()); + } + #[tokio::test] async fn clear_keeps_memory_token_when_storage_delete_fails() { let oauth = oauth_with_storage(Arc::new(FailingDelete)); From 642c868817d8751db2fce58c7c71fa987499cf6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Wed, 4 Nov 2026 16:42:11 +0800 Subject: [PATCH 3/9] fix(oauth): bound refresh with a timeout; typed OAuth error codes across SDKs - token refresh runs under the token lock like revoke, so bound it with the same 30s timeout (TOKEN_REQUEST_TIMEOUT); add a paused-clock test for the revoke timeout releasing the lock - OAuthError::code() gives every variant a stable numeric code; Error::OAuth / HttpClientError::OAuth hold the typed OAuthError and SimpleError::OAuth carries the code, so C/C++/Java/Python callers can tell no-token, refresh, revoke and file failures apart; a request that cannot obtain a token reports kind OAuth instead of Other - Java: OAuthBuilder.build() fails with OpenApiException like revoke()/clear(); GridContext/FundContext.create read the Config handle inside synchronized (config) --- CHANGELOG.md | 5 ++ c/src/oauth.rs | 10 +-- .../java/com/longbridge/OpenApiException.java | 13 ++-- .../java/com/longbridge/fund/FundContext.java | 2 +- .../java/com/longbridge/grid/GridContext.java | 2 +- java/src/error.rs | 2 +- java/src/oauth.rs | 6 +- python/pysrc/longbridge/openapi.pyi | 4 +- python/src/oauth.rs | 2 +- rust/crates/httpclient/src/error.rs | 4 +- rust/crates/httpclient/src/request.rs | 5 +- rust/crates/oauth/Cargo.toml | 2 +- rust/crates/oauth/src/client.rs | 69 ++++++++++++++++--- rust/crates/oauth/src/error.rs | 53 ++++++++++++++ rust/src/error.rs | 50 +++++++++++--- 15 files changed, 190 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b709d988f..f74bc7127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **All SDKs:** OAuth errors carry a stable numeric code (`OAuthError::code()` in Rust), exposed wherever the error kind is `OAuth`: `lb_error_code` (C), `Status::code()` (C++), `OpenApiException.getCode()` (Java) and `OpenApiException.code` (Python). Callers can tell failures apart without parsing messages, e.g. `1` = no token available, `3` = refresh failed, `4` = revoke failed, `16` = token file delete failed (full table on `OAuthError::code`). A request that fails because the access token cannot be obtained is reported the same way (kind `OAuth` with the code) instead of kind `Other`. The Node.js binding still reports errors as plain messages. - **All SDKs:** `OAuth.revoke()` revokes the current token on the server through the existing RFC 7009 endpoint (`POST /oauth2/revoke`): the refresh token when present, otherwise the access token. It only contacts the server; the in-memory and cached tokens are left untouched. On a client created with `from_token` (no client ID) it fails with a `RevokeFailed` error instead of calling the server. The token lock is held for the whole request, so a concurrent token refresh cannot rotate the refresh token in between (which would revoke the stale token and leave the new one valid). While it runs, `access_token()` and every request that needs a token wait for it; the request is bounded by a 30-second timeout (`RevokeFailed` on expiry). - **All SDKs:** `OAuth.clear()` drops the token locally: it clears the in-memory copy and deletes the cached token file (`~/.longbridge/openapi/tokens/`; a missing file is not an error). It does not contact the server, so call `revoke()` first to disconnect the application completely. After `clear()`, `access_token()` fails with `NoTokenAvailable`. The persisted token is deleted first and the in-memory token is cleared only when that succeeds, so a failed delete leaves the handle usable and the error is returned. - Rust: `OAuth::revoke` / `OAuth::clear` (async), `OAuthError::RevokeFailed`, `OAuthError::TokenFileDelete`. C: `lb_oauth_revoke` / `lb_oauth_clear`. C++: `OAuth::revoke` / `OAuth::clear`. Java: `OAuth.revoke()` / `OAuth.clear()` returning `CompletableFuture`, which fails with `OpenApiException` like other SDK calls; both are `synchronized` with `close()`, so a concurrent `close()` cannot free the native handle while it is in use (a closed handle throws `IllegalStateException`). Node.js: `oauth.revoke()` / `oauth.clear()` returning promises. Python: `OAuth.revoke()` / `OAuth.clear()` plus `revoke_async()` / `clear_async()`; the `openapi.pyi` stub is updated. The blocking Python calls release the GIL while waiting. In C/C++, a NULL handle (e.g. a moved-from C++ `OAuth`) is reported through the callback as an error instead of crashing. @@ -15,10 +16,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Breaking changes +- **Rust:** `Error::OAuth` now holds the typed `longbridge::oauth::OAuthError` instead of a `String` (it is also `From`), `SimpleError::OAuth` becomes `SimpleError::OAuth { code, message }`, and `HttpClientError::OAuth` holds an `OAuthError`. Code that constructs or destructures these variants needs updating; `Display` output is unchanged. +- **Java:** `OAuthBuilder.build()` now fails with `OpenApiException` (kind `OAuth`, with the code above) instead of a bare `RuntimeException`, like `revoke()` / `clear()` and other SDK calls. Code catching `RuntimeException` around it should catch `OpenApiException`. - **Rust:** `OAuthError` is now `#[non_exhaustive]` and gains `RevokeFailed` and `TokenFileDelete`. Exhaustive `match`es on `OAuthError` outside this crate need a wildcard arm; adding variants will no longer be a breaking change. ### Fixed +- **All SDKs:** an OAuth token refresh is bounded by a 30-second timeout (`RefreshTokenFailed: timed out after 30s`). It runs under the token lock, so a hung refresh previously blocked every request needing a token, and `revoke()` / `clear()`, indefinitely. +- **Java:** `GridContext.create` / `FundContext.create` read the `Config` native handle inside `synchronized (config)`, like every other context, so a concurrent `Config.close()` can no longer free it while it is being used. - **All SDKs:** the OAuth browser authorization flow now uses PKCE (RFC 7636, `S256`): the authorization request carries a `code_challenge` and the token exchange sends the matching `code_verifier`. The server requires PKCE for `public` clients, so previously such clients (e.g. those created through dynamic client registration) could not authorize through the SDK and failed with `OAuth authorization failed: server_error`. Clients that don't require PKCE are unaffected. ## [5.3.0] - 2026-10-23 diff --git a/c/src/oauth.rs b/c/src/oauth.rs index d27d650a7..81fd41c5e 100644 --- a/c/src/oauth.rs +++ b/c/src/oauth.rs @@ -55,7 +55,7 @@ pub unsafe extern "C" fn lb_oauth_new( open_url_callback(c_url.as_ptr(), open_url_userdata_usize as *mut c_void); }) .await - .map_err(|e| longbridge::Error::OAuth(e.to_string()))?; + .map_err(longbridge::Error::OAuth)?; Ok(Box::into_raw(Box::new(COAuth { inner: oauth }))) }); } @@ -86,7 +86,9 @@ unsafe fn clone_inner(oauth: *const COAuth) -> Option { } fn null_oauth_error() -> longbridge::Error { - longbridge::Error::OAuth("OAuth client is NULL".to_string()) + longbridge::Error::OAuth(longbridge::oauth::OAuthError::Other( + "OAuth client is NULL".to_string(), + )) } /// Asynchronously revoke the current token on the server (RFC 7009) @@ -110,7 +112,7 @@ pub unsafe extern "C" fn lb_oauth_revoke( .ok_or_else(null_oauth_error)? .revoke() .await - .map_err(|e| longbridge::Error::OAuth(e.to_string()))?; + .map_err(longbridge::Error::OAuth)?; Ok(()) }); } @@ -136,7 +138,7 @@ pub unsafe extern "C" fn lb_oauth_clear( .ok_or_else(null_oauth_error)? .clear() .await - .map_err(|e| longbridge::Error::OAuth(e.to_string()))?; + .map_err(longbridge::Error::OAuth)?; Ok(()) }); } diff --git a/java/javasrc/src/main/java/com/longbridge/OpenApiException.java b/java/javasrc/src/main/java/com/longbridge/OpenApiException.java index 7e040265d..cc5492de6 100644 --- a/java/javasrc/src/main/java/com/longbridge/OpenApiException.java +++ b/java/javasrc/src/main/java/com/longbridge/OpenApiException.java @@ -7,8 +7,9 @@ * pieces of diagnostic information: *
    *
  • {@link #getKind()} – broad category of the error (HTTP, OpenAPI, OAuth, Other)
    • - *
  • {@link #getCode()} – numeric error code returned by the server, or {@code null} for - * client-side errors
    • + *
  • {@link #getCode()} – numeric error code: the HTTP status or OpenAPI code returned + * by the server, a stable OAuth error code for {@link ErrorKind#OAuth} (see + * {@link #getCode()}), or {@code null} for other client-side errors
    • *
  • {@link #getMessage()} – human-readable error description
    • *
*/ @@ -40,9 +41,13 @@ public ErrorKind getKind() { } /** - * Returns the numeric error code returned by the server. + * Returns the numeric error code. * - * @return error code, or {@code null} for client-side errors + *

+ * For {@link ErrorKind#OAuth} errors this is a stable code identifying the + * failure: 1 no token available, 2 no refresh token, 3 refresh failed, 4 revoke failed, 5 authorization failed, 6 authorization timeout, 7 code exchange failed, 8 CSRF mismatch, 9 callback bind failed, 10 local address failed, 11 callback channel closed, 12 no home dir, 13 token file read, 14 token file parse, 15 token file write, 16 token file delete, 17 create dir failed, 18 serialize token, 0 other. + * + * @return error code, or {@code null} for client-side errors other than OAuth */ public Long getCode() { return code; diff --git a/java/javasrc/src/main/java/com/longbridge/fund/FundContext.java b/java/javasrc/src/main/java/com/longbridge/fund/FundContext.java index 14597d95e..040c36393 100644 --- a/java/javasrc/src/main/java/com/longbridge/fund/FundContext.java +++ b/java/javasrc/src/main/java/com/longbridge/fund/FundContext.java @@ -26,7 +26,7 @@ private long raw() { */ public static FundContext create(Config config) { FundContext ctx = new FundContext(); - ctx.raw = SdkNative.newFundContext(config.getRaw()); + synchronized (config) { ctx.raw = SdkNative.newFundContext(config.getRaw()); } return ctx; } diff --git a/java/javasrc/src/main/java/com/longbridge/grid/GridContext.java b/java/javasrc/src/main/java/com/longbridge/grid/GridContext.java index 42f29f37d..fd8445a10 100644 --- a/java/javasrc/src/main/java/com/longbridge/grid/GridContext.java +++ b/java/javasrc/src/main/java/com/longbridge/grid/GridContext.java @@ -26,7 +26,7 @@ private long raw() { */ public static GridContext create(Config config) { GridContext ctx = new GridContext(); - ctx.raw = SdkNative.newGridContext(config.getRaw()); + synchronized (config) { ctx.raw = SdkNative.newGridContext(config.getRaw()); } return ctx; } diff --git a/java/src/error.rs b/java/src/error.rs index 0e8b3d43b..3f0302c7e 100644 --- a/java/src/error.rs +++ b/java/src/error.rs @@ -29,7 +29,7 @@ impl From for JniError { impl From for JniError { fn from(e: longbridge::oauth::OAuthError) -> Self { - JniError::Other(e.to_string()) + JniError::OpenApi(Box::new(longbridge::Error::OAuth(e))) } } diff --git a/java/src/oauth.rs b/java/src/oauth.rs index 5668ada8c..a80a4d804 100644 --- a/java/src/oauth.rs +++ b/java/src/oauth.rs @@ -50,7 +50,7 @@ pub unsafe extern "system" fn Java_com_longbridge_SdkNative_oauthBuild( } }) .await - .map_err(|e| crate::error::JniError::Other(e.to_string()))?; + .map_err(crate::error::JniError::from)?; Ok(JavaLong::from(Box::into_raw(Box::new(oauth)) as jlong)) })?; Ok(()) @@ -81,7 +81,7 @@ pub unsafe extern "system" fn Java_com_longbridge_SdkNative_oauthRevoke( oauth .revoke() .await - .map_err(|e| longbridge::Error::OAuth(e.to_string()).into()) + .map_err(|e| longbridge::Error::OAuth(e).into()) })?; Ok(()) }) @@ -101,7 +101,7 @@ pub unsafe extern "system" fn Java_com_longbridge_SdkNative_oauthClear( oauth .clear() .await - .map_err(|e| longbridge::Error::OAuth(e.to_string()).into()) + .map_err(|e| longbridge::Error::OAuth(e).into()) })?; Ok(()) }) diff --git a/python/pysrc/longbridge/openapi.pyi b/python/pysrc/longbridge/openapi.pyi index 11e6258a3..49c39e8c1 100644 --- a/python/pysrc/longbridge/openapi.pyi +++ b/python/pysrc/longbridge/openapi.pyi @@ -45,7 +45,9 @@ class OpenApiException(Exception): code: int """ - Error code + Error code: the HTTP status or OpenAPI code returned by the server, or for + ``ErrorKind.OAuth`` a stable code identifying the failure (1 no token available, 2 no refresh token, 3 refresh failed, 4 revoke failed, 5 authorization failed, 6 authorization timeout, 7 code exchange failed, 8 CSRF mismatch, 9 callback bind failed, 10 local address failed, 11 callback channel closed, 12 no home dir, 13 token file read, 14 token file parse, 15 token file write, 16 token file delete, 17 create dir failed, 18 serialize token, 0 other). + `None` for other client-side errors. """ message: str diff --git a/python/src/oauth.rs b/python/src/oauth.rs index f29f7ea0a..e847a14e6 100644 --- a/python/src/oauth.rs +++ b/python/src/oauth.rs @@ -4,7 +4,7 @@ use pyo3::prelude::*; use crate::error::ErrorNewType; fn oauth_err(e: OAuthError) -> PyErr { - ErrorNewType(longbridge::Error::OAuth(e.to_string())).into() + ErrorNewType(longbridge::Error::OAuth(e)).into() } /// OAuth 2.0 client handle for Longbridge OpenAPI diff --git a/rust/crates/httpclient/src/error.rs b/rust/crates/httpclient/src/error.rs index e19a27ec1..cc12e2f43 100644 --- a/rust/crates/httpclient/src/error.rs +++ b/rust/crates/httpclient/src/error.rs @@ -106,9 +106,9 @@ pub enum HttpClientError { online: i32, }, - /// OAuth error + /// OAuth error (e.g. the access token could not be obtained) #[error("oauth error: {0}")] - OAuth(String), + OAuth(#[from] longbridge_oauth::OAuthError), /// Server-sent events stream error #[error("sse stream error: {0}")] diff --git a/rust/crates/httpclient/src/request.rs b/rust/crates/httpclient/src/request.rs index ae9db9289..dc71aadbd 100644 --- a/rust/crates/httpclient/src/request.rs +++ b/rust/crates/httpclient/src/request.rs @@ -314,10 +314,7 @@ where DcRegion::from_credentials(&[app_key, access_token, app_secret]), ), AuthConfig::OAuth(oauth) => { - let token = oauth - .access_token() - .await - .map_err(|e| HttpClientError::OAuth(e.to_string()))?; + let token = oauth.access_token().await.map_err(HttpClientError::OAuth)?; // Derive DC region from the token prefix (us_→US, others→AP). // The token is sent as-is (including any prefix); the gateway // accepts the full token and routes via the x-dc-region header. diff --git a/rust/crates/oauth/Cargo.toml b/rust/crates/oauth/Cargo.toml index 288dc19f6..0035e6efd 100644 --- a/rust/crates/oauth/Cargo.toml +++ b/rust/crates/oauth/Cargo.toml @@ -21,4 +21,4 @@ tokio = { workspace = true, features = ["rt", "time", "sync", "net"] } tracing.workspace = true [dev-dependencies] -tokio = { workspace = true, features = ["rt-multi-thread", "macros"] } +tokio = { workspace = true, features = ["rt-multi-thread", "macros", "test-util"] } diff --git a/rust/crates/oauth/src/client.rs b/rust/crates/oauth/src/client.rs index 657f408a7..5c838d954 100644 --- a/rust/crates/oauth/src/client.rs +++ b/rust/crates/oauth/src/client.rs @@ -133,7 +133,7 @@ impl OAuth { /// token between reading and revoking it (which would revoke a stale token /// while the new one stays valid). While it runs, `access_token` (and every /// request that needs a token) waits; the request is therefore bounded by a - /// 30-second timeout. + /// 30-second timeout, as is a token refresh it may have to wait for. pub async fn revoke(&self) -> OAuthResult<()> { // `from_token` clients have no client ID: the token was issued // elsewhere, so the server cannot revoke it on our behalf. @@ -182,12 +182,12 @@ impl OAuth { message: e.to_string(), })? .request_async(http_client); - tokio::time::timeout(REVOKE_TIMEOUT, request) + tokio::time::timeout(TOKEN_REQUEST_TIMEOUT, request) .await .map_err(|_| { tracing::error!(client_id = %self.0.client_id, "token revocation timed out"); OAuthError::RevokeFailed { - message: format!("timed out after {}s", REVOKE_TIMEOUT.as_secs()), + message: format!("timed out after {}s", TOKEN_REQUEST_TIMEOUT.as_secs()), } })? .map_err(|e| { @@ -293,10 +293,20 @@ impl OAuth { &format!("http://localhost:{}/callback", self.0.callback_port), ) .await; - let token_response = client - .exchange_refresh_token(&RefreshToken::new(refresh_token_str.to_string())) - .request_async(async_http_client) + // Refreshes run under the token lock (see `access_token`), so a hung + // request must not hold it forever. + let refresh_token = RefreshToken::new(refresh_token_str.to_string()); + let request = client + .exchange_refresh_token(&refresh_token) + .request_async(async_http_client); + let token_response = tokio::time::timeout(TOKEN_REQUEST_TIMEOUT, request) .await + .map_err(|_| { + tracing::error!(client_id = %self.0.client_id, "token refresh timed out"); + OAuthError::RefreshTokenFailed { + message: format!("timed out after {}s", TOKEN_REQUEST_TIMEOUT.as_secs()), + } + })? .map_err(|e| { tracing::error!(client_id = %self.0.client_id, error = %e, "failed to refresh token"); OAuthError::RefreshTokenFailed { @@ -317,8 +327,8 @@ impl OAuth { } } -/// Upper bound for a revocation request, which holds the token lock. -const REVOKE_TIMEOUT: Duration = Duration::from_secs(30); +/// Upper bound for a refresh or revocation request; both hold the token lock. +const TOKEN_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); /// Describe a revocation failure, including the server's `error` and /// `error_description` (the `Display` of `ServerResponse` omits them). @@ -537,6 +547,49 @@ mod tests { assert!(oauth.0.token.try_lock().is_ok()); } + #[tokio::test(start_paused = true)] + async fn revoke_times_out_and_releases_the_token_lock() { + let oauth = oauth_with_storage(Arc::new(LegacyStorage)); + let client = build_oauth_client( + OAUTH_BASE_URL, + "my-client", + "http://localhost:60355/callback", + ); + // The request never completes; the paused clock auto-advances to the + // timeout. + let http_client = + |_req: HttpRequest| std::future::pending::>(); + + match oauth.revoke_with(&client, http_client).await { + Err(OAuthError::RevokeFailed { message }) => { + assert_eq!(message, "timed out after 30s") + } + other => panic!("unexpected result: {other:?}"), + } + assert_eq!(oauth.access_token().await.unwrap(), "access"); + } + + #[test] + fn oauth_error_codes_are_stable() { + assert_eq!(OAuthError::Other(String::new()).code(), 0); + assert_eq!(OAuthError::NoTokenAvailable.code(), 1); + assert_eq!( + OAuthError::RevokeFailed { + message: String::new() + } + .code(), + 4 + ); + assert_eq!( + OAuthError::TokenFileDelete { + path: Default::default(), + source: std::io::Error::other("x"), + } + .code(), + 16 + ); + } + #[tokio::test] async fn clear_keeps_memory_token_when_storage_delete_fails() { let oauth = oauth_with_storage(Arc::new(FailingDelete)); diff --git a/rust/crates/oauth/src/error.rs b/rust/crates/oauth/src/error.rs index 22fcddb5e..e5567f565 100644 --- a/rust/crates/oauth/src/error.rs +++ b/rust/crates/oauth/src/error.rs @@ -134,5 +134,58 @@ pub enum OAuthError { Other(String), } +impl OAuthError { + /// Stable numeric code identifying the kind of error, so that callers of + /// the language bindings (where the error crosses the FFI boundary as a + /// message plus a code) can tell failures apart without parsing messages. + /// + /// | Code | Variant | + /// |------|---------| + /// | 1 | [`NoTokenAvailable`](Self::NoTokenAvailable) | + /// | 2 | [`NoRefreshToken`](Self::NoRefreshToken) | + /// | 3 | [`RefreshTokenFailed`](Self::RefreshTokenFailed) | + /// | 4 | [`RevokeFailed`](Self::RevokeFailed) | + /// | 5 | [`AuthorizationFailed`](Self::AuthorizationFailed) | + /// | 6 | [`AuthorizationTimeout`](Self::AuthorizationTimeout) | + /// | 7 | [`ExchangeCodeFailed`](Self::ExchangeCodeFailed) | + /// | 8 | [`CsrfTokenMismatch`](Self::CsrfTokenMismatch) | + /// | 9 | [`BindCallbackFailed`](Self::BindCallbackFailed) | + /// | 10 | [`LocalAddressFailed`](Self::LocalAddressFailed) | + /// | 11 | [`CallbackChannelClosed`](Self::CallbackChannelClosed) | + /// | 12 | [`NoHomeDir`](Self::NoHomeDir) | + /// | 13 | [`TokenFileRead`](Self::TokenFileRead) | + /// | 14 | [`TokenFileParse`](Self::TokenFileParse) | + /// | 15 | [`TokenFileWrite`](Self::TokenFileWrite) | + /// | 16 | [`TokenFileDelete`](Self::TokenFileDelete) | + /// | 17 | [`CreateDirFailed`](Self::CreateDirFailed) | + /// | 18 | [`SerializeToken`](Self::SerializeToken) | + /// | 0 | [`Other`](Self::Other) | + /// + /// Codes are never reused; a new variant gets a new code. + pub fn code(&self) -> i64 { + match self { + OAuthError::Other(_) => 0, + OAuthError::NoTokenAvailable => 1, + OAuthError::NoRefreshToken => 2, + OAuthError::RefreshTokenFailed { .. } => 3, + OAuthError::RevokeFailed { .. } => 4, + OAuthError::AuthorizationFailed { .. } => 5, + OAuthError::AuthorizationTimeout => 6, + OAuthError::ExchangeCodeFailed { .. } => 7, + OAuthError::CsrfTokenMismatch => 8, + OAuthError::BindCallbackFailed { .. } => 9, + OAuthError::LocalAddressFailed => 10, + OAuthError::CallbackChannelClosed => 11, + OAuthError::NoHomeDir => 12, + OAuthError::TokenFileRead { .. } => 13, + OAuthError::TokenFileParse { .. } => 14, + OAuthError::TokenFileWrite { .. } => 15, + OAuthError::TokenFileDelete { .. } => 16, + OAuthError::CreateDirFailed { .. } => 17, + OAuthError::SerializeToken { .. } => 18, + } + } +} + /// Result type for OAuth operations pub type OAuthResult = std::result::Result; diff --git a/rust/src/error.rs b/rust/src/error.rs index 84edabfd8..ced88eeba 100644 --- a/rust/src/error.rs +++ b/rust/src/error.rs @@ -70,7 +70,7 @@ pub enum Error { /// OAuth error #[error("oauth error: {0}")] - OAuth(String), + OAuth(#[from] longbridge_oauth::OAuthError), /// A conversation event stream ended before a final result was observed #[error("conversation stream ended before a final result was observed")] @@ -118,6 +118,12 @@ impl Error { SimpleError::Other(err.to_string()) } } + Error::HttpClient(HttpClientError::OAuth(err)) | Error::OAuth(err) => { + SimpleError::OAuth { + code: err.code(), + message: err.to_string(), + } + } Error::WsClient(WsClientError::ResponseError { detail: Some(detail), .. @@ -138,7 +144,6 @@ impl Error { | Error::ConversationStreamEnded => SimpleError::Other(self.to_string()), #[cfg(feature = "blocking")] Error::Blocking(_) => SimpleError::Other(self.to_string()), - Error::OAuth(msg) => SimpleError::OAuth(msg), } } } @@ -169,8 +174,14 @@ pub enum SimpleError { #[error("other error: {0}")] Other(String), /// OAuth error - #[error("oauth error: {0}")] - OAuth(String), + #[error("oauth error: {message}")] + OAuth { + /// Stable error code, see + /// [`OAuthError::code`](longbridge_oauth::OAuthError::code) + code: i64, + /// Error message + message: String, + }, } impl From for SimpleError { @@ -200,7 +211,7 @@ impl SimpleError { SimpleError::Http { .. } => SimpleErrorKind::Http, SimpleError::OpenApi { .. } => SimpleErrorKind::OpenApi, SimpleError::Other(_) => SimpleErrorKind::Other, - SimpleError::OAuth(_) => SimpleErrorKind::OAuth, + SimpleError::OAuth { .. } => SimpleErrorKind::OAuth, } } @@ -210,7 +221,7 @@ impl SimpleError { SimpleError::Http { status_code } => Some(*status_code as i64), SimpleError::OpenApi { code, .. } => Some(*code), SimpleError::Other(_) => None, - SimpleError::OAuth(_) => None, + SimpleError::OAuth { code, .. } => Some(*code), } } @@ -220,7 +231,7 @@ impl SimpleError { SimpleError::Http { .. } => None, SimpleError::OpenApi { trace_id, .. } => Some(trace_id), SimpleError::Other(_) => None, - SimpleError::OAuth(_) => None, + SimpleError::OAuth { .. } => None, } } @@ -230,7 +241,30 @@ impl SimpleError { SimpleError::Http { .. } => "bad status code", SimpleError::OpenApi { message, .. } => message.as_str(), SimpleError::Other(message) => message.as_str(), - SimpleError::OAuth(message) => message.as_str(), + SimpleError::OAuth { message, .. } => message.as_str(), } } } + +#[cfg(test)] +mod tests { + use longbridge_oauth::OAuthError; + + use super::*; + + #[test] + fn oauth_errors_keep_their_code() { + let err = Error::OAuth(OAuthError::NoTokenAvailable).into_simple_error(); + assert_eq!(err.kind(), SimpleErrorKind::OAuth); + assert_eq!(err.code(), Some(1)); + assert_eq!(err.message(), "no token available"); + + // A request that could not obtain a token reports the same code. + let err = Error::HttpClient(HttpClientError::OAuth(OAuthError::RevokeFailed { + message: "x".to_string(), + })) + .into_simple_error(); + assert_eq!(err.kind(), SimpleErrorKind::OAuth); + assert_eq!(err.code(), Some(4)); + } +} From 477f4f883f0e812568784d7c51aafd6bf474195c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Wed, 4 Nov 2026 17:00:54 +0800 Subject: [PATCH 4/9] docs(oauth): document OAuth error codes in C/C++/Python; fix changelog wording - lb_error_code documents the OAuth code table (propagated to longbridge.h); C++ Status::code() points to it - openapi.pyi: OpenApiException.code is Optional[int] and ErrorKind has the OAuth member - OAuthBuilder.build() javadoc names the failure type - assert every OAuthError code, not a sample - CHANGELOG: the Java change is the future's failure cause, not a thrown exception; note binding exception text changes and the refresh-timeout trade-off --- CHANGELOG.md | 5 +- c/csrc/include/longbridge.h | 31 +++++++ c/src/error.rs | 29 +++++++ cpp/include/status.hpp | 4 +- .../java/com/longbridge/OAuthBuilder.java | 4 +- python/pysrc/longbridge/openapi.pyi | 7 +- rust/crates/oauth/src/client.rs | 81 +++++++++++++++---- 7 files changed, 139 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f74bc7127..f6ccde763 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,12 +17,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Breaking changes - **Rust:** `Error::OAuth` now holds the typed `longbridge::oauth::OAuthError` instead of a `String` (it is also `From`), `SimpleError::OAuth` becomes `SimpleError::OAuth { code, message }`, and `HttpClientError::OAuth` holds an `OAuthError`. Code that constructs or destructures these variants needs updating; `Display` output is unchanged. -- **Java:** `OAuthBuilder.build()` now fails with `OpenApiException` (kind `OAuth`, with the code above) instead of a bare `RuntimeException`, like `revoke()` / `clear()` and other SDK calls. Code catching `RuntimeException` around it should catch `OpenApiException`. +- **Java:** the future returned by `OAuthBuilder.build()` now completes exceptionally with `OpenApiException` (kind `OAuth`, with the code above) instead of a bare `RuntimeException`, like `revoke()` / `clear()` and other SDK calls. `build().join()` / `get()` still throw `CompletionException` / `ExecutionException`; only code that inspects the cause (`getCause() instanceof RuntimeException`, a cast in `exceptionally`) needs updating. +- **Bindings:** because OAuth errors now carry a code, the exception text of the bindings changes where it includes the code: Python `str(OpenApiException)` shows `(kind=..., code=N, ...)` and Java `OpenApiException.toString()` shows `code=N` instead of `null`. A request that fails because the token cannot be obtained now reports kind `OAuth` with the bare OAuth message (e.g. `no token available`) instead of kind `Other` with `oauth error: no token available`. - **Rust:** `OAuthError` is now `#[non_exhaustive]` and gains `RevokeFailed` and `TokenFileDelete`. Exhaustive `match`es on `OAuthError` outside this crate need a wildcard arm; adding variants will no longer be a breaking change. ### Fixed -- **All SDKs:** an OAuth token refresh is bounded by a 30-second timeout (`RefreshTokenFailed: timed out after 30s`). It runs under the token lock, so a hung refresh previously blocked every request needing a token, and `revoke()` / `clear()`, indefinitely. +- **All SDKs:** an OAuth token refresh is bounded by a 30-second timeout (`RefreshTokenFailed: timed out after 30s`). It runs under the token lock, so a hung refresh previously blocked every request needing a token, and `revoke()` / `clear()`, indefinitely; a hung refresh while building the client now falls back to the browser authorization after 30 s instead of never returning. Trade-off: if the server rotated the refresh token but its response arrives after the timeout, the new token is lost and the next refresh fails with `invalid_grant`, requiring a new authorization. - **Java:** `GridContext.create` / `FundContext.create` read the `Config` native handle inside `synchronized (config)`, like every other context, so a concurrent `Config.close()` can no longer free it while it is being used. - **All SDKs:** the OAuth browser authorization flow now uses PKCE (RFC 7636, `S256`): the authorization request carries a `code_challenge` and the token exchange sends the matching `code_verifier`. The server requires PKCE for `public` clients, so previously such clients (e.g. those created through dynamic client registration) could not authorize through the SDK and failed with `OAuth authorization failed: server_error`. Clients that don't require PKCE are unaffected. diff --git a/c/csrc/include/longbridge.h b/c/csrc/include/longbridge.h index 8b35cf19b..76a7e6597 100644 --- a/c/csrc/include/longbridge.h +++ b/c/csrc/include/longbridge.h @@ -14762,6 +14762,37 @@ void lb_error_free(struct lb_error_t *error); const char *lb_error_message(const struct lb_error_t *error); +/** + * Returns the error code. + * + * For `ErrorKindHttp` it is the HTTP status, for + * `ErrorKindOpenApi` the OpenAPI error code. For `ErrorKindOAuth` + * it is a stable code identifying the failure: + * + * | Code | Meaning | + * |------|---------| + * | 0 | other (e.g. NULL OAuth handle) | + * | 1 | no token available | + * | 2 | no refresh token | + * | 3 | token refresh failed | + * | 4 | token revocation failed | + * | 5 | authorization failed | + * | 6 | authorization timeout | + * | 7 | authorization code exchange failed | + * | 8 | CSRF token mismatch | + * | 9 | failed to bind the callback server | + * | 10 | failed to get the local address | + * | 11 | callback channel closed | + * | 12 | cannot determine the home directory | + * | 13 | failed to read the token file | + * | 14 | failed to parse the token file | + * | 15 | failed to write the token file | + * | 16 | failed to delete the token file | + * | 17 | failed to create the token directory | + * | 18 | failed to serialize the token | + * + * It is 0 when the error carries no code; check `lb_error_kind` first. + */ int64_t lb_error_code(const struct lb_error_t *error); enum lb_error_kind_t lb_error_kind(const struct lb_error_t *error); diff --git a/c/src/error.rs b/c/src/error.rs index c4c5b0046..2d44d5d5b 100644 --- a/c/src/error.rs +++ b/c/src/error.rs @@ -62,6 +62,35 @@ pub unsafe extern "C" fn lb_error_message(error: *const CError) -> *const c_char (*error).message.to_ffi_type() } +/// Returns the error code. +/// +/// For `ErrorKindHttp` it is the HTTP status, for +/// `ErrorKindOpenApi` the OpenAPI error code. For `ErrorKindOAuth` +/// it is a stable code identifying the failure: +/// +/// | Code | Meaning | +/// |------|---------| +/// | 0 | other (e.g. NULL OAuth handle) | +/// | 1 | no token available | +/// | 2 | no refresh token | +/// | 3 | token refresh failed | +/// | 4 | token revocation failed | +/// | 5 | authorization failed | +/// | 6 | authorization timeout | +/// | 7 | authorization code exchange failed | +/// | 8 | CSRF token mismatch | +/// | 9 | failed to bind the callback server | +/// | 10 | failed to get the local address | +/// | 11 | callback channel closed | +/// | 12 | cannot determine the home directory | +/// | 13 | failed to read the token file | +/// | 14 | failed to parse the token file | +/// | 15 | failed to write the token file | +/// | 16 | failed to delete the token file | +/// | 17 | failed to create the token directory | +/// | 18 | failed to serialize the token | +/// +/// It is 0 when the error carries no code; check `lb_error_kind` first. #[unsafe(no_mangle)] pub unsafe extern "C" fn lb_error_code(error: *const CError) -> i64 { (*error).code diff --git a/cpp/include/status.hpp b/cpp/include/status.hpp index 6e6e1689b..8461f3edd 100644 --- a/cpp/include/status.hpp +++ b/cpp/include/status.hpp @@ -40,7 +40,9 @@ class Status /// Returns the error kind if an error occurs std::optional kind() const; - /// Returns the error code if an error occurs + /// Returns the error code if an error occurs: the HTTP status, the OpenAPI + /// error code, or for `ErrorKind::OAuth` a stable code identifying the + /// failure (see `lb_error_code` in longbridge.h for the table) std::optional code() const; /// Returns the error message if an error occurs diff --git a/java/javasrc/src/main/java/com/longbridge/OAuthBuilder.java b/java/javasrc/src/main/java/com/longbridge/OAuthBuilder.java index 696531095..ff34aa459 100644 --- a/java/javasrc/src/main/java/com/longbridge/OAuthBuilder.java +++ b/java/javasrc/src/main/java/com/longbridge/OAuthBuilder.java @@ -69,7 +69,9 @@ public OAuthBuilder setCallbackPort(int port) { * * @param onOpenUrl Called with the authorization URL; open it in a browser * or print it to the console - * @return CompletableFuture that resolves to a new {@link OAuth} handle + * @return CompletableFuture that resolves to a new {@link OAuth} handle; on + * failure it completes exceptionally with an + * {@link OpenApiException} of kind {@link ErrorKind#OAuth} */ public CompletableFuture build(Consumer onOpenUrl) { return AsyncCallback.executeTask((callback) -> { diff --git a/python/pysrc/longbridge/openapi.pyi b/python/pysrc/longbridge/openapi.pyi index 49c39e8c1..51f2144b1 100644 --- a/python/pysrc/longbridge/openapi.pyi +++ b/python/pysrc/longbridge/openapi.pyi @@ -33,6 +33,11 @@ class ErrorKind: Other error """ + class OAuth(ErrorKind): + """ + OAuth error + """ + class OpenApiException(Exception): """ OpenAPI exception @@ -43,7 +48,7 @@ class OpenApiException(Exception): Error kind """ - code: int + code: Optional[int] """ Error code: the HTTP status or OpenAPI code returned by the server, or for ``ErrorKind.OAuth`` a stable code identifying the failure (1 no token available, 2 no refresh token, 3 refresh failed, 4 revoke failed, 5 authorization failed, 6 authorization timeout, 7 code exchange failed, 8 CSRF mismatch, 9 callback bind failed, 10 local address failed, 11 callback channel closed, 12 no home dir, 13 token file read, 14 token file parse, 15 token file write, 16 token file delete, 17 create dir failed, 18 serialize token, 0 other). diff --git a/rust/crates/oauth/src/client.rs b/rust/crates/oauth/src/client.rs index 5c838d954..2d54f3b44 100644 --- a/rust/crates/oauth/src/client.rs +++ b/rust/crates/oauth/src/client.rs @@ -571,23 +571,70 @@ mod tests { #[test] fn oauth_error_codes_are_stable() { - assert_eq!(OAuthError::Other(String::new()).code(), 0); - assert_eq!(OAuthError::NoTokenAvailable.code(), 1); - assert_eq!( - OAuthError::RevokeFailed { - message: String::new() - } - .code(), - 4 - ); - assert_eq!( - OAuthError::TokenFileDelete { - path: Default::default(), - source: std::io::Error::other("x"), - } - .code(), - 16 - ); + let io = || std::io::Error::other("x"); + let json = || serde_json::from_str::<()>("x").unwrap_err(); + let path = std::path::PathBuf::new; + let msg = String::new; + let cases = [ + (OAuthError::Other(msg()), 0), + (OAuthError::NoTokenAvailable, 1), + (OAuthError::NoRefreshToken, 2), + (OAuthError::RefreshTokenFailed { message: msg() }, 3), + (OAuthError::RevokeFailed { message: msg() }, 4), + (OAuthError::AuthorizationFailed { message: msg() }, 5), + (OAuthError::AuthorizationTimeout, 6), + (OAuthError::ExchangeCodeFailed { message: msg() }, 7), + (OAuthError::CsrfTokenMismatch, 8), + ( + OAuthError::BindCallbackFailed { + port: 0, + message: msg(), + }, + 9, + ), + (OAuthError::LocalAddressFailed, 10), + (OAuthError::CallbackChannelClosed, 11), + (OAuthError::NoHomeDir, 12), + ( + OAuthError::TokenFileRead { + path: path(), + source: io(), + }, + 13, + ), + ( + OAuthError::TokenFileParse { + path: path(), + source: json(), + }, + 14, + ), + ( + OAuthError::TokenFileWrite { + path: path(), + source: io(), + }, + 15, + ), + ( + OAuthError::TokenFileDelete { + path: path(), + source: io(), + }, + 16, + ), + ( + OAuthError::CreateDirFailed { + path: path(), + source: io(), + }, + 17, + ), + (OAuthError::SerializeToken { source: json() }, 18), + ]; + for (err, code) in cases { + assert_eq!(err.code(), code, "{err:?}"); + } } #[tokio::test] From d5f778ad1dc1f1bd5baa48392116bf9058ce4463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Wed, 4 Nov 2026 18:59:48 +0800 Subject: [PATCH 5/9] docs(python): match OpenApiException stub to its real constructor The stub declared __init__(code, message) and no trace_id; the class in longbridge/__init__.py takes (kind, code, trace_id, message). --- python/pysrc/longbridge/openapi.pyi | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/python/pysrc/longbridge/openapi.pyi b/python/pysrc/longbridge/openapi.pyi index 51f2144b1..e4d583e59 100644 --- a/python/pysrc/longbridge/openapi.pyi +++ b/python/pysrc/longbridge/openapi.pyi @@ -55,12 +55,23 @@ class OpenApiException(Exception): `None` for other client-side errors. """ + trace_id: Optional[str] + """ + Trace ID of the request, if any + """ + message: str """ Error message """ - def __init__(self, code: int, message: str) -> None: ... + def __init__( + self, + kind: ErrorKind, + code: Optional[int], + trace_id: Optional[str], + message: str, + ) -> None: ... class HttpClient: """ From fdb9d3357a37b01787118726b4851f3cdb16f980 Mon Sep 17 00:00:00 2001 From: mitnick Date: Wed, 4 Nov 2026 19:30:22 +0800 Subject: [PATCH 6/9] chore(oauth): upgrade oauth2 to 5 Ports longbridge/openapi#615 (by @Tickwhiz) onto #617: - oauth2 4.4 -> 5: typestate BasicClient (LongbridgeClient alias), TokenResponse bound, async requests through a reqwest 0.12 client that refuses redirects like the previous transport - revoke_with takes any oauth2::AsyncHttpClient; the #617 timeout and lock-holding tests use Fn clients as oauth2 5 requires - loopback regression tests for authorization, denial, CSRF, refresh rotation/omission, HTTP and storage failures, reauthorization, redirect refusal, default expiry, blocking cached-token loading and endpoint selection (task-local test endpoint override) - the separate reqwest 0.11 / hyper 0.14 / rustls 0.21 chain is gone --- CHANGELOG.md | 4 + Cargo.toml | 2 +- rust/crates/oauth/Cargo.toml | 2 +- rust/crates/oauth/src/client.rs | 110 ++- rust/crates/oauth/src/lib.rs | 2 + rust/crates/oauth/src/tests/flows.rs | 1024 ++++++++++++++++++++++++++ rust/crates/oauth/src/token.rs | 9 +- 7 files changed, 1108 insertions(+), 45 deletions(-) create mode 100644 rust/crates/oauth/src/tests/flows.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index f6ccde763..5be7c0010 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Rust: `OAuth::revoke` / `OAuth::clear` (async), `OAuthError::RevokeFailed`, `OAuthError::TokenFileDelete`. C: `lb_oauth_revoke` / `lb_oauth_clear`. C++: `OAuth::revoke` / `OAuth::clear`. Java: `OAuth.revoke()` / `OAuth.clear()` returning `CompletableFuture`, which fails with `OpenApiException` like other SDK calls; both are `synchronized` with `close()`, so a concurrent `close()` cannot free the native handle while it is in use (a closed handle throws `IllegalStateException`). Node.js: `oauth.revoke()` / `oauth.clear()` returning promises. Python: `OAuth.revoke()` / `OAuth.clear()` plus `revoke_async()` / `clear_async()`; the `openapi.pyi` stub is updated. The blocking Python calls release the GIL while waiting. In C/C++, a NULL handle (e.g. a moved-from C++ `OAuth`) is reported through the callback as an error instead of crashing. - **Rust:** `TokenStorage::delete(client_id)` removes a persisted token. It has a default implementation that returns an error, so existing custom storage backends keep compiling but `OAuth::clear` fails loudly (and keeps the in-memory token) instead of reporting success without wiping anything; override it to support `clear`. `FileTokenStorage` deletes the token file. +### Changed + +- **Rust:** the internal OAuth dependency is upgraded from `oauth2` 4.4 to 5, which uses the workspace's `reqwest` 0.12 stack instead of a separate `reqwest` 0.11 / `hyper` 0.14 / `rustls` 0.21 chain. The public OAuth API, token persistence and redirect protection (token requests do not follow redirects) are unchanged; error text coming from the HTTP layer may differ. + ### Breaking changes - **Rust:** `Error::OAuth` now holds the typed `longbridge::oauth::OAuthError` instead of a `String` (it is also `From`), `SimpleError::OAuth` becomes `SimpleError::OAuth { code, message }`, and `HttpClientError::OAuth` holds an `OAuthError`. Code that constructs or destructures these variants needs updating; `Display` output is unchanged. diff --git a/Cargo.toml b/Cargo.toml index ee2c61a03..d74cd0772 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,6 +70,6 @@ darling = "0.21.2" convert_case = "0.8.0" Inflector = "0.11.4" cbindgen = "0.29.2" -oauth2 = "4.4" +oauth2 = "5" open = "5" dirs = "6" diff --git a/rust/crates/oauth/Cargo.toml b/rust/crates/oauth/Cargo.toml index 0035e6efd..33cbd9ae0 100644 --- a/rust/crates/oauth/Cargo.toml +++ b/rust/crates/oauth/Cargo.toml @@ -21,4 +21,4 @@ tokio = { workspace = true, features = ["rt", "time", "sync", "net"] } tracing.workspace = true [dev-dependencies] -tokio = { workspace = true, features = ["rt-multi-thread", "macros", "test-util"] } +tokio = { workspace = true, features = ["rt-multi-thread", "macros", "io-util", "test-util"] } diff --git a/rust/crates/oauth/src/client.rs b/rust/crates/oauth/src/client.rs index 2d54f3b44..4b855ae36 100644 --- a/rust/crates/oauth/src/client.rs +++ b/rust/crates/oauth/src/client.rs @@ -1,11 +1,11 @@ -use std::{fmt, future::Future, sync::Arc, time::Duration}; +use std::{fmt, sync::Arc, time::Duration}; use longbridge_geo::is_cn; use oauth2::{ - AccessToken, AuthUrl, AuthorizationCode, ClientId, CsrfToken, HttpRequest, HttpResponse, - PkceCodeChallenge, PkceCodeVerifier, RedirectUrl, RefreshToken, RequestTokenError, + AccessToken, AsyncHttpClient, AuthUrl, AuthorizationCode, ClientId, CsrfToken, EndpointNotSet, + EndpointSet, PkceCodeChallenge, PkceCodeVerifier, RedirectUrl, RefreshToken, RequestTokenError, RevocationErrorResponseType, RevocationUrl, Scope, StandardErrorResponse, - StandardRevocableToken, TokenUrl, basic::BasicClient, reqwest::async_http_client, + StandardRevocableToken, TokenUrl, basic::BasicClient, reqwest, }; use poem::listener::{Acceptor, Listener, TcpListener}; @@ -20,19 +20,36 @@ const OAUTH_BASE_URL: &str = "https://openapi.longbridge.com/oauth2"; const OAUTH_BASE_URL_CN: &str = "https://openapi.longbridge.cn/oauth2"; const OAUTH_BASE_URL_TEST: &str = "https://openapi-global.longbridge.xyz/oauth2"; -async fn oauth_base_url() -> &'static str { +#[cfg(test)] +tokio::task_local! { + pub(crate) static TEST_OAUTH_BASE_URL: String; +} + +async fn oauth_base_url() -> String { + // Keep protocol tests on their own loopback server without process-wide + // state. + #[cfg(test)] + if let Ok(base) = TEST_OAUTH_BASE_URL.try_with(Clone::clone) { + return base; + } + if std::env::var("LONGBRIDGE_ENV") .map(|v| v == "staging") .unwrap_or(false) { - OAUTH_BASE_URL_TEST + OAUTH_BASE_URL_TEST.to_string() } else if is_cn().await { - OAUTH_BASE_URL_CN + OAUTH_BASE_URL_CN.to_string() } else { - OAUTH_BASE_URL + OAUTH_BASE_URL.to_string() } } +/// The oauth2 client with the authorization, token and revocation endpoints +/// set. +type LongbridgeClient = + BasicClient; + /// Default port for the local OAuth callback server. pub(crate) const DEFAULT_CALLBACK_PORT: u16 = 60355; @@ -151,16 +168,17 @@ impl OAuth { &format!("http://localhost:{}/callback", self.0.callback_port), ) .await; - self.revoke_with(&client, async_http_client).await + let http_client = create_http_client().map_err(|e| OAuthError::RevokeFailed { + message: e.to_string(), + })?; + self.revoke_with(&client, &http_client).await } /// Revoke through `http_client` (injectable for tests), holding the token /// lock for the whole request. - async fn revoke_with(&self, client: &BasicClient, http_client: C) -> OAuthResult<()> + async fn revoke_with(&self, client: &LongbridgeClient, http_client: &C) -> OAuthResult<()> where - C: FnOnce(HttpRequest) -> F, - F: Future>, - RE: std::error::Error + 'static, + C: for<'c> AsyncHttpClient<'c>, { let guard = self.0.token.lock().await; let token = guard.as_ref().ok_or(OAuthError::NoTokenAvailable)?; @@ -262,10 +280,13 @@ impl OAuth { } tracing::debug!(client_id = %self.0.client_id, "exchanging authorization code for token"); + let http_client = create_http_client().map_err(|e| OAuthError::ExchangeCodeFailed { + message: e.to_string(), + })?; let token_response = client .exchange_code(AuthorizationCode::new(code)) .set_pkce_verifier(pkce_verifier) - .request_async(async_http_client) + .request_async(&http_client) .await .map_err(|e| { tracing::error!(client_id = %self.0.client_id, error = %e, "failed to exchange authorization code for token"); @@ -295,10 +316,13 @@ impl OAuth { .await; // Refreshes run under the token lock (see `access_token`), so a hung // request must not hold it forever. + let http_client = create_http_client().map_err(|e| OAuthError::RefreshTokenFailed { + message: e.to_string(), + })?; let refresh_token = RefreshToken::new(refresh_token_str.to_string()); let request = client .exchange_refresh_token(&refresh_token) - .request_async(async_http_client); + .request_async(&http_client); let token_response = tokio::time::timeout(TOKEN_REQUEST_TIMEOUT, request) .await .map_err(|_| { @@ -352,7 +376,9 @@ where /// PKCE (RFC 7636, `S256`) is required by the server for public clients and /// ignored for clients that don't need it. The returned verifier must be sent /// with the token exchange. -fn build_authorize_url(client: &BasicClient) -> (oauth2::url::Url, CsrfToken, PkceCodeVerifier) { +fn build_authorize_url( + client: &LongbridgeClient, +) -> (oauth2::url::Url, CsrfToken, PkceCodeVerifier) { let (pkce_challenge, pkce_verifier) = PkceCodeChallenge::new_random_sha256(); let (auth_url, csrf_token) = client .authorize_url(CsrfToken::new_random) @@ -363,25 +389,32 @@ fn build_authorize_url(client: &BasicClient) -> (oauth2::url::Url, CsrfToken, Pk } /// Build the oauth2 BasicClient for Longbridge endpoints. -async fn create_oauth_client(client_id: &str, redirect_uri: &str) -> BasicClient { - build_oauth_client(oauth_base_url().await, client_id, redirect_uri) +async fn create_oauth_client(client_id: &str, redirect_uri: &str) -> LongbridgeClient { + build_oauth_client(&oauth_base_url().await, client_id, redirect_uri) +} + +fn build_oauth_client(base: &str, client_id: &str, redirect_uri: &str) -> LongbridgeClient { + BasicClient::new(ClientId::new(client_id.to_string())) + .set_auth_uri(AuthUrl::new(format!("{base}/authorize")).unwrap()) + .set_token_uri(TokenUrl::new(format!("{base}/token")).unwrap()) + .set_redirect_uri(RedirectUrl::new(redirect_uri.to_string()).unwrap()) + .set_revocation_url(RevocationUrl::new(format!("{base}/revoke")).unwrap()) } -fn build_oauth_client(base: &str, client_id: &str, redirect_uri: &str) -> BasicClient { - BasicClient::new( - ClientId::new(client_id.to_string()), - None, - AuthUrl::new(format!("{base}/authorize")).unwrap(), - Some(TokenUrl::new(format!("{base}/token")).unwrap()), - ) - .set_redirect_uri(RedirectUrl::new(redirect_uri.to_string()).unwrap()) - .set_revocation_uri(RevocationUrl::new(format!("{base}/revoke")).unwrap()) +/// HTTP client for token requests. +fn create_http_client() -> Result { + reqwest::Client::builder() + // Preserve the previous OAuth transport's protection against redirects. + .redirect(reqwest::redirect::Policy::none()) + .build() } #[cfg(test)] mod tests { use std::collections::HashMap; + use oauth2::{HttpRequest, HttpResponse}; + use super::*; use crate::storage::StoredToken; @@ -518,25 +551,26 @@ mod tests { "http://localhost:60355/callback", ); + // oauth2 5 requires an `Fn` client; it is called exactly once here. let (started_tx, started_rx) = tokio::sync::oneshot::channel::(); let (release_tx, release_rx) = tokio::sync::oneshot::channel::<()>(); - let http_client = move |req: HttpRequest| async move { - started_tx.send(req).unwrap(); - release_rx.await.unwrap(); - Ok::<_, std::io::Error>(HttpResponse { - status_code: oauth2::http::StatusCode::OK, - headers: Default::default(), - body: Vec::new(), - }) + let channels = std::sync::Mutex::new(Some((started_tx, release_rx))); + let http_client = move |req: HttpRequest| { + let (started_tx, release_rx) = channels.lock().unwrap().take().unwrap(); + async move { + started_tx.send(req).unwrap(); + release_rx.await.unwrap(); + Ok::<_, std::io::Error>(HttpResponse::new(Vec::new())) + } }; let task = { let oauth = oauth.clone(); - tokio::spawn(async move { oauth.revoke_with(&client, http_client).await }) + tokio::spawn(async move { oauth.revoke_with(&client, &http_client).await }) }; let req = started_rx.await.unwrap(); - let body = String::from_utf8(req.body).unwrap(); + let body = String::from_utf8(req.into_body()).unwrap(); assert!(body.contains("token=refresh"), "{body}"); assert!(body.contains("token_type_hint=refresh_token"), "{body}"); // A refresh (access_token) cannot run while the request is in flight. @@ -560,7 +594,7 @@ mod tests { let http_client = |_req: HttpRequest| std::future::pending::>(); - match oauth.revoke_with(&client, http_client).await { + match oauth.revoke_with(&client, &http_client).await { Err(OAuthError::RevokeFailed { message }) => { assert_eq!(message, "timed out after 30s") } diff --git a/rust/crates/oauth/src/lib.rs b/rust/crates/oauth/src/lib.rs index 02cc74f0b..b0f4039fe 100644 --- a/rust/crates/oauth/src/lib.rs +++ b/rust/crates/oauth/src/lib.rs @@ -42,6 +42,8 @@ pub use storage::{FileTokenStorage, StoredToken, TokenStorage}; #[cfg(test)] mod tests { + mod flows; + use std::{ sync::{Arc, Mutex}, time::{SystemTime, UNIX_EPOCH}, diff --git a/rust/crates/oauth/src/tests/flows.rs b/rust/crates/oauth/src/tests/flows.rs new file mode 100644 index 000000000..6fd69e7f5 --- /dev/null +++ b/rust/crates/oauth/src/tests/flows.rs @@ -0,0 +1,1024 @@ +//! Regression tests for the public OAuth token lifecycle and endpoint +//! selection. + +use std::{ + collections::{BTreeMap, VecDeque}, + process::Command, + sync::{Arc, Mutex}, + time::Duration, +}; + +use oauth2::url::{Url, form_urlencoded}; +use poem::{ + EndpointExt, Request, Response, Route, Server, handler, + http::{Method, StatusCode}, + listener::{Acceptor, Listener, TcpListener}, + web::Data, +}; +use tokio::{io::AsyncWriteExt, net::TcpStream, task::JoinHandle, time::timeout}; + +use super::{MemoryStorage, now_secs}; +use crate::{ + OAuth, OAuthBuilder, OAuthError, OAuthResult, StoredToken, TokenStorage, + client::TEST_OAUTH_BASE_URL, +}; + +impl TokenStorage for Arc { + fn load(&self, client_id: &str) -> Option { + self.as_ref().load(client_id) + } + + fn save(&self, token: &StoredToken) -> OAuthResult<()> { + self.as_ref().save(token) + } + + fn delete(&self, client_id: &str) -> OAuthResult<()> { + self.as_ref().delete(client_id) + } +} + +struct FailSecondSaveStorage { + inner: Arc, + save_attempts: Mutex, +} + +impl TokenStorage for FailSecondSaveStorage { + fn load(&self, client_id: &str) -> Option { + self.inner.load(client_id) + } + + fn save(&self, token: &StoredToken) -> OAuthResult<()> { + let mut attempts = self.save_attempts.lock().unwrap(); + *attempts += 1; + if *attempts == 2 { + return Err(OAuthError::Other("test storage save failed".to_string())); + } + self.inner.save(token) + } +} + +#[derive(Clone)] +struct TokenRequest { + method: Method, + path: String, + content_type: Option, + authorization: Option, + body: String, +} + +struct TokenServerState { + responses: VecDeque<(StatusCode, String)>, + requests: Vec, + redirect_location: Option, +} + +struct TokenServer { + base_url: String, + state: Arc>, + task: JoinHandle<()>, +} + +impl TokenServer { + async fn start(responses: impl IntoIterator) -> Self { + #[handler] + async fn token_handler( + request: &Request, + body: String, + state: Data<&Arc>>, + ) -> Response { + let mut state = state.lock().unwrap(); + state.requests.push(TokenRequest { + method: request.method().clone(), + path: request.uri().path().to_string(), + content_type: request + .headers() + .get("content-type") + .map(|value| value.to_str().unwrap().to_string()), + authorization: request + .headers() + .get("authorization") + .map(|value| value.to_str().unwrap().to_string()), + body, + }); + let (status, body) = state + .responses + .pop_front() + .expect("unexpected token request"); + let mut response = Response::builder() + .status(status) + .content_type("application/json"); + if status.is_redirection() + && let Some(location) = state.redirect_location.as_deref() + { + response = response.header("location", location); + } + response.body(body) + } + + let acceptor = TcpListener::bind("127.0.0.1:0") + .into_acceptor() + .await + .unwrap(); + let port = acceptor + .local_addr() + .into_iter() + .find_map(|address| address.as_socket_addr().map(|address| address.port())) + .unwrap(); + let state = Arc::new(Mutex::new(TokenServerState { + responses: responses.into_iter().collect(), + requests: Vec::new(), + redirect_location: None, + })); + let app = Route::new() + .at("/oauth2/token", poem::post(token_handler)) + .at("/oauth2/revoke", poem::post(token_handler)) + .data(Arc::clone(&state)); + let task = tokio::spawn(async move { + Server::new_with_acceptor(acceptor) + .run(app) + .await + .expect("token server failed"); + }); + + Self { + base_url: format!("http://127.0.0.1:{port}/oauth2"), + state, + task, + } + } + + fn requests(&self) -> Vec { + self.state.lock().unwrap().requests.clone() + } + + fn redirect_to(&self, location: String) { + self.state.lock().unwrap().redirect_location = Some(location); + } +} + +impl Drop for TokenServer { + fn drop(&mut self) { + self.task.abort(); + } +} + +fn successful_token_response( + access_token: &str, + refresh_token: &str, + expires_in: u64, +) -> (StatusCode, String) { + ( + StatusCode::OK, + serde_json::json!({ + "access_token": access_token, + "refresh_token": refresh_token, + "token_type": "Bearer", + "expires_in": expires_in + }) + .to_string(), + ) +} + +fn assert_refresh_requests(server: &TokenServer, expected_refresh_tokens: &[&str]) { + let requests = server.requests(); + assert_eq!(requests.len(), expected_refresh_tokens.len() + 1); + for (request, refresh_token) in requests[1..].iter().zip(expected_refresh_tokens) { + assert_eq!(request.method, Method::POST); + assert_eq!(request.path, "/oauth2/token"); + assert!(request.authorization.is_none()); + let form: BTreeMap<_, _> = form_urlencoded::parse(request.body.as_bytes()) + .into_owned() + .collect(); + assert_eq!( + form, + BTreeMap::from([ + ("client_id".to_string(), "test-client".to_string()), + ("grant_type".to_string(), "refresh_token".to_string()), + ("refresh_token".to_string(), (*refresh_token).to_string()), + ]) + ); + } +} + +fn callback_url(redirect_uri: &str, code: &str, state: &str) -> Url { + let mut url = Url::parse(redirect_uri).unwrap(); + assert_eq!(url.path(), "/callback"); + url.query_pairs_mut() + .append_pair("code", code) + .append_pair("state", state); + url +} + +async fn send_callback(url: Url) { + assert_eq!(url.scheme(), "http"); + assert_eq!(url.host_str(), Some("localhost")); + let port = url.port().expect("callback must use an explicit port"); + let mut stream = TcpStream::connect(("127.0.0.1", port)).await.unwrap(); + let request = format!( + "GET {}?{} HTTP/1.1\r\nHost: localhost:{port}\r\nConnection: close\r\n\r\n", + url.path(), + url.query().unwrap() + ); + stream.write_all(request.as_bytes()).await.unwrap(); + stream.shutdown().await.unwrap(); +} + +async fn build_with_callback(storage: impl TokenStorage) -> OAuthResult { + let callback = Mutex::new(None); + let result = OAuthBuilder::new("test-client") + .callback_port(0) + .token_storage(storage) + .build(|url| { + let url = Url::parse(url).unwrap(); + let params: BTreeMap<_, _> = url.query_pairs().into_owned().collect(); + let redirect = callback_url( + params.get("redirect_uri").unwrap(), + "authorization-code", + params.get("state").unwrap(), + ); + *callback.lock().unwrap() = Some(tokio::spawn(send_callback(redirect))); + }) + .await; + let callback = callback.lock().unwrap().take(); + if let Some(callback) = callback { + callback.await.unwrap(); + } + result +} + +#[tokio::test] +async fn build_authorizes_and_persists_token() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([( + StatusCode::OK, + serde_json::json!({ + "access_token": "authorized-access-token", + "refresh_token": "authorized-refresh-token", + "token_type": "Bearer", + "expires_in": 3600 + }) + .to_string(), + )]) + .await; + let storage = Arc::new(MemoryStorage::default()); + let authorization_url = Mutex::new(None); + let callback = Mutex::new(None); + let started_at = now_secs(); + + let oauth = TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = OAuthBuilder::new("test-client") + .callback_port(0) + .token_storage(Arc::clone(&storage)) + .build(|url| { + let url = Url::parse(url).unwrap(); + let params: BTreeMap<_, _> = url.query_pairs().into_owned().collect(); + assert_eq!(url.path(), "/oauth2/authorize"); + assert_eq!(params.get("client_id").unwrap(), "test-client"); + assert_eq!(params.get("response_type").unwrap(), "code"); + assert!(!params.get("state").unwrap().is_empty()); + let redirect = callback_url( + params.get("redirect_uri").unwrap(), + "authorization-code+ /value", + params.get("state").unwrap(), + ); + *authorization_url.lock().unwrap() = Some(url); + *callback.lock().unwrap() = Some(tokio::spawn(send_callback(redirect))); + }) + .await + .unwrap(); + assert_eq!( + oauth.access_token().await.unwrap(), + "authorized-access-token" + ); + oauth + }) + .await; + + let callback = callback.lock().unwrap().take().unwrap(); + callback.await.unwrap(); + assert_eq!(oauth.client_id(), "test-client"); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.client_id, "test-client"); + assert_eq!(stored.access_token, "authorized-access-token"); + assert_eq!( + stored.refresh_token.as_deref(), + Some("authorized-refresh-token") + ); + assert!(stored.expires_at >= started_at + 3600); + assert!(stored.expires_at <= now_secs() + 3600); + assert_eq!(storage.save_count(), 1); + + let authorization_url = authorization_url.lock().unwrap().take().unwrap(); + let authorization_params: BTreeMap<_, _> = + authorization_url.query_pairs().into_owned().collect(); + let requests = server.requests(); + assert_eq!(requests.len(), 1); + let request = &requests[0]; + assert_eq!(request.method, Method::POST); + assert_eq!(request.path, "/oauth2/token"); + assert_eq!( + request.content_type.as_deref(), + Some("application/x-www-form-urlencoded") + ); + assert!(request.authorization.is_none()); + let form: BTreeMap<_, _> = form_urlencoded::parse(request.body.as_bytes()) + .into_owned() + .collect(); + assert_eq!(authorization_params["code_challenge_method"], "S256"); + let verifier = oauth2::PkceCodeVerifier::new(form["code_verifier"].clone()); + assert_eq!( + authorization_params["code_challenge"], + oauth2::PkceCodeChallenge::from_code_verifier_sha256(&verifier).as_str() + ); + let expected = BTreeMap::from([ + ("client_id".to_string(), "test-client".to_string()), + ("grant_type".to_string(), "authorization_code".to_string()), + ("code".to_string(), "authorization-code+ /value".to_string()), + ("code_verifier".to_string(), form["code_verifier"].clone()), + ( + "redirect_uri".to_string(), + authorization_params.get("redirect_uri").unwrap().clone(), + ), + ]); + assert_eq!(form, expected); + assert!(!form.contains_key("client_secret")); + }) + .await + .expect("OAuth authorization flow timed out"); +} + +#[tokio::test] +async fn revoke_rejects_insecure_endpoint_without_changing_local_tokens() { + timeout(Duration::from_secs(5), async { + for refresh_token in [Some("cached-refresh-token"), None] { + let server = TokenServer::start([]).await; + let storage = Arc::new(MemoryStorage::with_token(StoredToken { + client_id: "test-client".to_string(), + access_token: "cached-access-token".to_string(), + refresh_token: refresh_token.map(str::to_string), + expires_at: now_secs() + 7200, + })); + + TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = OAuthBuilder::new("test-client") + .token_storage(Arc::clone(&storage)) + .build(|_| panic!("cached token must not trigger authorization")) + .await + .unwrap(); + assert!(matches!( + oauth.revoke().await, + Err(OAuthError::RevokeFailed { message }) + if message == "Scheme for revocation endpoint URL must be HTTPS" + )); + assert_eq!(oauth.access_token().await.unwrap(), "cached-access-token"); + }) + .await; + + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.access_token, "cached-access-token"); + assert_eq!(stored.refresh_token.as_deref(), refresh_token); + assert_eq!(storage.save_count(), 0); + assert!(server.requests().is_empty()); + } + }) + .await + .expect("OAuth insecure revocation rejection timed out"); +} + +#[tokio::test] +async fn build_rejects_mismatched_csrf_state_without_exchanging_token() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([]).await; + let storage = Arc::new(MemoryStorage::default()); + let callback = Mutex::new(None); + + let result = TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + OAuthBuilder::new("test-client") + .callback_port(0) + .token_storage(Arc::clone(&storage)) + .build(|url| { + let url = Url::parse(url).unwrap(); + let params: BTreeMap<_, _> = url.query_pairs().into_owned().collect(); + let mismatched_state = + format!("{}-mismatched", params.get("state").unwrap()); + let redirect = callback_url( + params.get("redirect_uri").unwrap(), + "authorization-code", + &mismatched_state, + ); + *callback.lock().unwrap() = Some(tokio::spawn(send_callback(redirect))); + }) + .await + }) + .await; + + let callback = callback.lock().unwrap().take().unwrap(); + callback.await.unwrap(); + assert!( + matches!(&result, Err(OAuthError::CsrfTokenMismatch)), + "unexpected build result: {result:?}" + ); + assert!(server.requests().is_empty()); + assert_eq!(storage.save_count(), 0); + assert!(storage.load("test-client").is_none()); + }) + .await + .expect("OAuth CSRF rejection flow timed out"); +} + +#[tokio::test] +async fn build_propagates_access_denied_callback_without_exchanging_token() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([]).await; + let storage = Arc::new(MemoryStorage::default()); + let callback = Mutex::new(None); + + let result = TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + OAuthBuilder::new("test-client") + .callback_port(0) + .token_storage(Arc::clone(&storage)) + .build(|url| { + let url = Url::parse(url).unwrap(); + let params: BTreeMap<_, _> = url.query_pairs().into_owned().collect(); + let mut redirect = Url::parse(params.get("redirect_uri").unwrap()).unwrap(); + assert_eq!(redirect.path(), "/callback"); + redirect + .query_pairs_mut() + .append_pair("error", "access_denied"); + *callback.lock().unwrap() = Some(tokio::spawn(send_callback(redirect))); + }) + .await + }) + .await; + + let callback = callback.lock().unwrap().take().unwrap(); + callback.await.unwrap(); + match result { + Err(OAuthError::AuthorizationFailed { message }) => { + assert_eq!(message, "access_denied"); + } + other => panic!("unexpected access_denied callback result: {other:?}"), + } + assert!(server.requests().is_empty()); + assert_eq!(storage.save_count(), 0); + assert!(storage.load("test-client").is_none()); + }) + .await + .expect("OAuth access_denied callback flow timed out"); +} + +async fn assert_refresh_persists_and_caches(response: String, expected_refresh_token: &str) { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([ + ( + StatusCode::OK, + serde_json::json!({ + "access_token": "expired-access-token", + "refresh_token": "old-refresh-token", + "token_type": "Bearer", + "expires_in": 0 + }) + .to_string(), + ), + (StatusCode::OK, response), + ]) + .await; + let storage = Arc::new(MemoryStorage::default()); + let started_at = now_secs(); + + TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = build_with_callback(Arc::clone(&storage)).await.unwrap(); + assert_eq!(storage.save_count(), 1); + assert_eq!( + storage.load("test-client").unwrap().access_token, + "expired-access-token" + ); + assert_eq!(server.requests().len(), 1); + + assert_eq!( + oauth.access_token().await.unwrap(), + "refreshed-access-token" + ); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.client_id, "test-client"); + assert_eq!(stored.access_token, "refreshed-access-token"); + assert_eq!( + stored.refresh_token.as_deref(), + Some(expected_refresh_token) + ); + assert!(stored.expires_at >= started_at + 3600); + assert!(stored.expires_at <= now_secs() + 3600); + assert_eq!(storage.save_count(), 2); + + let requests = server.requests(); + assert_eq!(requests.len(), 2); + let request = &requests[1]; + assert_eq!(request.method, Method::POST); + assert_eq!(request.path, "/oauth2/token"); + assert!(request.authorization.is_none()); + let form: BTreeMap<_, _> = form_urlencoded::parse(request.body.as_bytes()) + .into_owned() + .collect(); + assert_eq!( + form, + BTreeMap::from([ + ("client_id".to_string(), "test-client".to_string()), + ("grant_type".to_string(), "refresh_token".to_string()), + ("refresh_token".to_string(), "old-refresh-token".to_string()), + ]) + ); + + assert_eq!( + oauth.access_token().await.unwrap(), + "refreshed-access-token" + ); + assert_eq!(server.requests().len(), 2); + assert_eq!(storage.save_count(), 2); + }) + .await; + }) + .await + .expect("OAuth refresh flow timed out"); +} + +#[tokio::test] +async fn access_token_rotates_refresh_token_and_caches_result() { + assert_refresh_persists_and_caches( + serde_json::json!({ + "access_token": "refreshed-access-token", + "refresh_token": "new-refresh-token", + "token_type": "Bearer", + "expires_in": 3600 + }) + .to_string(), + "new-refresh-token", + ) + .await; +} + +#[tokio::test] +async fn access_token_preserves_refresh_token_when_response_omits_it() { + assert_refresh_persists_and_caches( + serde_json::json!({ + "access_token": "refreshed-access-token", + "token_type": "Bearer", + "expires_in": 3600 + }) + .to_string(), + "old-refresh-token", + ) + .await; +} + +#[tokio::test] +async fn access_token_retries_failed_refresh_with_original_token() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([ + successful_token_response("expired-access-token", "old-refresh-token", 0), + ( + StatusCode::BAD_REQUEST, + serde_json::json!({ + "error": "invalid_grant", + "error_description": "refresh token rejected" + }) + .to_string(), + ), + successful_token_response("retry-access-token", "retry-refresh-token", 3600), + ]) + .await; + let storage = Arc::new(MemoryStorage::default()); + + TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = build_with_callback(Arc::clone(&storage)).await.unwrap(); + let error = oauth.access_token().await.unwrap_err(); + assert!( + matches!(&error, OAuthError::RefreshTokenFailed { message } if !message.is_empty()), + "unexpected refresh error: {error:?}" + ); + assert_eq!(storage.save_count(), 1); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.access_token, "expired-access-token"); + assert_eq!(stored.refresh_token.as_deref(), Some("old-refresh-token")); + assert_refresh_requests(&server, &["old-refresh-token"]); + + assert_eq!(oauth.access_token().await.unwrap(), "retry-access-token"); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.access_token, "retry-access-token"); + assert_eq!(stored.refresh_token.as_deref(), Some("retry-refresh-token")); + assert_eq!(storage.save_count(), 2); + assert_refresh_requests(&server, &["old-refresh-token", "old-refresh-token"]); + + assert_eq!(oauth.access_token().await.unwrap(), "retry-access-token"); + assert_eq!(server.requests().len(), 3); + assert_eq!(storage.save_count(), 2); + }) + .await; + }) + .await + .expect("OAuth failed-refresh retry flow timed out"); +} + +#[tokio::test] +async fn access_token_preserves_cached_token_until_refreshed_token_is_saved() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([ + successful_token_response("expired-access-token", "old-refresh-token", 0), + successful_token_response("unsaved-access-token", "unsaved-refresh-token", 3600), + successful_token_response("retry-access-token", "retry-refresh-token", 3600), + ]) + .await; + let storage = Arc::new(MemoryStorage::default()); + + TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = build_with_callback(FailSecondSaveStorage { + inner: Arc::clone(&storage), + save_attempts: Mutex::new(0), + }) + .await + .unwrap(); + let error = oauth.access_token().await.unwrap_err(); + assert!( + matches!(&error, OAuthError::Other(message) if message == "test storage save failed"), + "unexpected storage error: {error:?}" + ); + assert_eq!(storage.save_count(), 1); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.access_token, "expired-access-token"); + assert_eq!(stored.refresh_token.as_deref(), Some("old-refresh-token")); + assert_refresh_requests(&server, &["old-refresh-token"]); + + assert_eq!(oauth.access_token().await.unwrap(), "retry-access-token"); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.access_token, "retry-access-token"); + assert_eq!(stored.refresh_token.as_deref(), Some("retry-refresh-token")); + assert_eq!(storage.save_count(), 2); + assert_refresh_requests(&server, &["old-refresh-token", "old-refresh-token"]); + + assert_eq!(oauth.access_token().await.unwrap(), "retry-access-token"); + assert_eq!(server.requests().len(), 3); + assert_eq!(storage.save_count(), 2); + }) + .await; + }) + .await + .expect("OAuth failed-save retry flow timed out"); +} + +#[tokio::test] +async fn build_propagates_authorization_exchange_error_without_saving_token() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([( + StatusCode::BAD_REQUEST, + serde_json::json!({ + "error": "invalid_grant", + "error_description": "authorization code rejected" + }) + .to_string(), + )]) + .await; + let storage = Arc::new(MemoryStorage::default()); + + let error = TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + build_with_callback(Arc::clone(&storage)).await + }) + .await + .unwrap_err(); + + assert!( + matches!(&error, OAuthError::ExchangeCodeFailed { message } if !message.is_empty()), + "unexpected authorization exchange error: {error:?}" + ); + assert_eq!(storage.save_count(), 0); + assert!(storage.load("test-client").is_none()); + let requests = server.requests(); + assert_eq!(requests.len(), 1); + assert_eq!(requests[0].method, Method::POST); + assert_eq!(requests[0].path, "/oauth2/token"); + let form: BTreeMap<_, _> = form_urlencoded::parse(requests[0].body.as_bytes()) + .into_owned() + .collect(); + assert_eq!(form.get("client_id").unwrap(), "test-client"); + assert_eq!(form.get("grant_type").unwrap(), "authorization_code"); + assert_eq!(form.get("code").unwrap(), "authorization-code"); + }) + .await + .expect("OAuth authorization exchange error flow timed out"); +} + +#[tokio::test] +async fn build_reauthorizes_when_cached_token_refresh_fails() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([ + ( + StatusCode::BAD_REQUEST, + serde_json::json!({ + "error": "invalid_grant", + "error_description": "cached refresh token rejected" + }) + .to_string(), + ), + successful_token_response("authorized-access-token", "authorized-refresh-token", 3600), + ]) + .await; + let storage = Arc::new(MemoryStorage::with_token(StoredToken { + client_id: "test-client".to_string(), + access_token: "expired-access-token".to_string(), + refresh_token: Some("old-refresh-token".to_string()), + expires_at: 0, + })); + + TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = build_with_callback(Arc::clone(&storage)).await.unwrap(); + assert_eq!( + oauth.access_token().await.unwrap(), + "authorized-access-token" + ); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.client_id, "test-client"); + assert_eq!(stored.access_token, "authorized-access-token"); + assert_eq!( + stored.refresh_token.as_deref(), + Some("authorized-refresh-token") + ); + assert_eq!(storage.save_count(), 1); + + let requests = server.requests(); + assert_eq!(requests.len(), 2); + let refresh_form: BTreeMap<_, _> = + form_urlencoded::parse(requests[0].body.as_bytes()) + .into_owned() + .collect(); + assert_eq!( + refresh_form, + BTreeMap::from([ + ("client_id".to_string(), "test-client".to_string()), + ("grant_type".to_string(), "refresh_token".to_string()), + ("refresh_token".to_string(), "old-refresh-token".to_string()), + ]) + ); + let authorization_form: BTreeMap<_, _> = + form_urlencoded::parse(requests[1].body.as_bytes()) + .into_owned() + .collect(); + assert_eq!(authorization_form.get("client_id").unwrap(), "test-client"); + assert_eq!( + authorization_form.get("grant_type").unwrap(), + "authorization_code" + ); + assert_eq!( + authorization_form.get("code").unwrap(), + "authorization-code" + ); + }) + .await; + }) + .await + .expect("OAuth refresh fallback authorization flow timed out"); +} + +#[tokio::test] +async fn build_does_not_follow_authorization_token_redirect() { + timeout(Duration::from_secs(5), async { + let probe = TokenServer::start([successful_token_response( + "redirected-access-token", + "redirected-refresh-token", + 3600, + )]) + .await; + let server = TokenServer::start([(StatusCode::TEMPORARY_REDIRECT, "{}".to_string())]).await; + server.redirect_to(format!("{}/token", probe.base_url)); + let storage = Arc::new(MemoryStorage::default()); + + let error = TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + build_with_callback(Arc::clone(&storage)).await + }) + .await + .unwrap_err(); + + assert!( + matches!(&error, OAuthError::ExchangeCodeFailed { message } if !message.is_empty()), + "unexpected authorization redirect error: {error:?}" + ); + assert_eq!(storage.save_count(), 0); + assert!(storage.load("test-client").is_none()); + let requests = server.requests(); + assert_eq!(requests.len(), 1); + let form: BTreeMap<_, _> = form_urlencoded::parse(requests[0].body.as_bytes()) + .into_owned() + .collect(); + assert_eq!(form.get("grant_type").unwrap(), "authorization_code"); + assert!(probe.requests().is_empty()); + }) + .await + .expect("OAuth authorization redirect rejection flow timed out"); +} + +#[tokio::test] +async fn access_token_does_not_follow_refresh_redirect_and_can_retry() { + timeout(Duration::from_secs(5), async { + let probe = TokenServer::start([successful_token_response( + "redirected-access-token", + "redirected-refresh-token", + 3600, + )]) + .await; + let server = TokenServer::start([ + successful_token_response("expired-access-token", "old-refresh-token", 0), + (StatusCode::TEMPORARY_REDIRECT, "{}".to_string()), + successful_token_response("retry-access-token", "retry-refresh-token", 3600), + ]) + .await; + server.redirect_to(format!("{}/token", probe.base_url)); + let storage = Arc::new(MemoryStorage::default()); + + TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = build_with_callback(Arc::clone(&storage)).await.unwrap(); + let error = oauth.access_token().await.unwrap_err(); + assert!( + matches!(&error, OAuthError::RefreshTokenFailed { message } if !message.is_empty()), + "unexpected refresh redirect error: {error:?}" + ); + assert_eq!(storage.save_count(), 1); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.access_token, "expired-access-token"); + assert_eq!(stored.refresh_token.as_deref(), Some("old-refresh-token")); + assert_refresh_requests(&server, &["old-refresh-token"]); + assert!(probe.requests().is_empty()); + + assert_eq!(oauth.access_token().await.unwrap(), "retry-access-token"); + let stored = storage.load("test-client").unwrap(); + assert_eq!(stored.access_token, "retry-access-token"); + assert_eq!(stored.refresh_token.as_deref(), Some("retry-refresh-token")); + assert_eq!(storage.save_count(), 2); + assert_refresh_requests(&server, &["old-refresh-token", "old-refresh-token"]); + + assert_eq!(oauth.access_token().await.unwrap(), "retry-access-token"); + assert_eq!(server.requests().len(), 3); + assert_eq!(storage.save_count(), 2); + assert!(probe.requests().is_empty()); + }) + .await; + }) + .await + .expect("OAuth refresh redirect rejection flow timed out"); +} + +#[tokio::test] +async fn build_defaults_missing_expiry_to_one_hour() { + timeout(Duration::from_secs(5), async { + let server = TokenServer::start([( + StatusCode::OK, + serde_json::json!({ + "access_token": "default-expiry-access-token", + "refresh_token": "default-expiry-refresh-token", + "token_type": "Bearer" + }) + .to_string(), + )]) + .await; + let storage = Arc::new(MemoryStorage::default()); + let started_at = now_secs(); + + TEST_OAUTH_BASE_URL + .scope(server.base_url.clone(), async { + let oauth = build_with_callback(Arc::clone(&storage)).await.unwrap(); + let stored = storage.load("test-client").unwrap(); + assert!(stored.expires_at >= started_at + 3600); + assert!(stored.expires_at <= now_secs() + 3600); + assert_eq!( + oauth.access_token().await.unwrap(), + "default-expiry-access-token" + ); + assert_eq!(storage.save_count(), 1); + assert_eq!(server.requests().len(), 1); + }) + .await; + }) + .await + .expect("OAuth default expiry flow timed out"); +} + +#[cfg(feature = "blocking")] +#[test] +fn build_blocking_loads_valid_cached_token_without_authorizing() { + let storage = Arc::new(MemoryStorage::with_token(StoredToken { + client_id: "test-client".to_string(), + access_token: "cached-access-token".to_string(), + refresh_token: Some("cached-refresh-token".to_string()), + expires_at: now_secs() + 7200, + })); + let oauth = OAuthBuilder::new("test-client") + .token_storage(Arc::clone(&storage)) + .build_blocking(|_| panic!("valid cached token must not trigger authorization")) + .unwrap(); + assert_eq!(oauth.client_id(), "test-client"); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let token = runtime.block_on(async { + timeout(Duration::from_secs(5), oauth.access_token()) + .await + .expect("cached access token lookup timed out") + .unwrap() + }); + assert_eq!(token, "cached-access-token"); + assert_eq!(storage.save_count(), 0); +} + +#[test] +fn authorization_endpoint_selection_uses_isolated_environments() { + for (environment, region, expected_url) in [ + ( + Some("staging"), + "CN", + "https://openapi-global.longbridge.xyz/oauth2/authorize", + ), + (None, "CN", "https://openapi.longbridge.cn/oauth2/authorize"), + ( + None, + "US", + "https://openapi.longbridge.com/oauth2/authorize", + ), + ] { + let mut command = Command::new(std::env::current_exe().unwrap()); + command + .args([ + "--ignored", + "--exact", + "tests::flows::authorization_endpoint_selection_child", + "--nocapture", + ]) + .env("LONGBRIDGE_REGION", region) + .env_remove("LONGPORT_REGION") + .env("TEST_OAUTH_EXPECTED_AUTHORIZATION_URL", expected_url); + if let Some(environment) = environment { + command.env("LONGBRIDGE_ENV", environment); + } else { + command.env_remove("LONGBRIDGE_ENV"); + } + let output = command + .output() + .expect("failed to launch endpoint selection child"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + output.status.success() + && stdout.contains(&format!("verified authorization endpoint: {expected_url}")), + "endpoint selection failed for environment={environment:?}, region={region}:\n{}\n{}", + stdout, + String::from_utf8_lossy(&output.stderr) + ); + } +} + +#[tokio::test] +#[ignore = "launched by authorization_endpoint_selection_uses_isolated_environments"] +async fn authorization_endpoint_selection_child() { + let expected_url = std::env::var("TEST_OAUTH_EXPECTED_AUTHORIZATION_URL") + .expect("endpoint selection child must be launched by its parent test"); + timeout(Duration::from_secs(5), async { + let storage = Arc::new(MemoryStorage::default()); + let callback = Mutex::new(None); + let result = OAuthBuilder::new("test-client") + .callback_port(0) + .token_storage(Arc::clone(&storage)) + .build(|url| { + let url = Url::parse(url).unwrap(); + assert_eq!( + format!("{}{}", url.origin().ascii_serialization(), url.path()), + expected_url + ); + let params: BTreeMap<_, _> = url.query_pairs().into_owned().collect(); + let mismatched_state = format!("{}-mismatched", params.get("state").unwrap()); + let redirect = callback_url( + params.get("redirect_uri").unwrap(), + "authorization-code", + &mismatched_state, + ); + *callback.lock().unwrap() = Some(tokio::spawn(send_callback(redirect))); + }) + .await; + let callback = callback.lock().unwrap().take().unwrap(); + callback.await.unwrap(); + assert!( + matches!(&result, Err(OAuthError::CsrfTokenMismatch)), + "unexpected endpoint selection result: {result:?}" + ); + assert_eq!(storage.save_count(), 0); + assert!(storage.load("test-client").is_none()); + println!("verified authorization endpoint: {expected_url}"); + }) + .await + .expect("OAuth endpoint selection callback flow timed out"); +} diff --git a/rust/crates/oauth/src/token.rs b/rust/crates/oauth/src/token.rs index 2adc68562..7461b1034 100644 --- a/rust/crates/oauth/src/token.rs +++ b/rust/crates/oauth/src/token.rs @@ -37,11 +37,10 @@ pub(crate) struct OAuthToken { impl OAuthToken { /// Build from oauth2 crate token response. - pub(crate) fn from_oauth2_response(client_id: String, token_response: &T) -> Self - where - TT: oauth2::TokenType, - T: TokenResponse, - { + pub(crate) fn from_oauth2_response( + client_id: String, + token_response: &T, + ) -> Self { let now = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() From 55dfacfbb3d8688b0f760f442044b9726b3e7278 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Wed, 4 Nov 2026 19:30:34 +0800 Subject: [PATCH 7/9] test(oauth): give the loopback OAuth flows a 30s hang guard The 5s per-flow timeout only guards against hangs; 30s avoids spurious failures on slow CI runners. --- rust/crates/oauth/src/tests/flows.rs | 32 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/rust/crates/oauth/src/tests/flows.rs b/rust/crates/oauth/src/tests/flows.rs index 6fd69e7f5..9ff3b77e2 100644 --- a/rust/crates/oauth/src/tests/flows.rs +++ b/rust/crates/oauth/src/tests/flows.rs @@ -18,6 +18,10 @@ use poem::{ use tokio::{io::AsyncWriteExt, net::TcpStream, task::JoinHandle, time::timeout}; use super::{MemoryStorage, now_secs}; + +/// Upper bound for a whole loopback flow. It only guards against hangs, so it +/// is generous for slow CI runners. +const HANG_GUARD: Duration = Duration::from_secs(30); use crate::{ OAuth, OAuthBuilder, OAuthError, OAuthResult, StoredToken, TokenStorage, client::TEST_OAUTH_BASE_URL, @@ -248,7 +252,7 @@ async fn build_with_callback(storage: impl TokenStorage) -> OAuthResult { #[tokio::test] async fn build_authorizes_and_persists_token() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([( StatusCode::OK, serde_json::json!({ @@ -350,7 +354,7 @@ async fn build_authorizes_and_persists_token() { #[tokio::test] async fn revoke_rejects_insecure_endpoint_without_changing_local_tokens() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { for refresh_token in [Some("cached-refresh-token"), None] { let server = TokenServer::start([]).await; let storage = Arc::new(MemoryStorage::with_token(StoredToken { @@ -389,7 +393,7 @@ async fn revoke_rejects_insecure_endpoint_without_changing_local_tokens() { #[tokio::test] async fn build_rejects_mismatched_csrf_state_without_exchanging_token() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([]).await; let storage = Arc::new(MemoryStorage::default()); let callback = Mutex::new(None); @@ -431,7 +435,7 @@ async fn build_rejects_mismatched_csrf_state_without_exchanging_token() { #[tokio::test] async fn build_propagates_access_denied_callback_without_exchanging_token() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([]).await; let storage = Arc::new(MemoryStorage::default()); let callback = Mutex::new(None); @@ -472,7 +476,7 @@ async fn build_propagates_access_denied_callback_without_exchanging_token() { } async fn assert_refresh_persists_and_caches(response: String, expected_refresh_token: &str) { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([ ( StatusCode::OK, @@ -577,7 +581,7 @@ async fn access_token_preserves_refresh_token_when_response_omits_it() { #[tokio::test] async fn access_token_retries_failed_refresh_with_original_token() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([ successful_token_response("expired-access-token", "old-refresh-token", 0), ( @@ -626,7 +630,7 @@ async fn access_token_retries_failed_refresh_with_original_token() { #[tokio::test] async fn access_token_preserves_cached_token_until_refreshed_token_is_saved() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([ successful_token_response("expired-access-token", "old-refresh-token", 0), successful_token_response("unsaved-access-token", "unsaved-refresh-token", 3600), @@ -673,7 +677,7 @@ async fn access_token_preserves_cached_token_until_refreshed_token_is_saved() { #[tokio::test] async fn build_propagates_authorization_exchange_error_without_saving_token() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([( StatusCode::BAD_REQUEST, serde_json::json!({ @@ -715,7 +719,7 @@ async fn build_propagates_authorization_exchange_error_without_saving_token() { #[tokio::test] async fn build_reauthorizes_when_cached_token_refresh_fails() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([ ( StatusCode::BAD_REQUEST, @@ -787,7 +791,7 @@ async fn build_reauthorizes_when_cached_token_refresh_fails() { #[tokio::test] async fn build_does_not_follow_authorization_token_redirect() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let probe = TokenServer::start([successful_token_response( "redirected-access-token", "redirected-refresh-token", @@ -825,7 +829,7 @@ async fn build_does_not_follow_authorization_token_redirect() { #[tokio::test] async fn access_token_does_not_follow_refresh_redirect_and_can_retry() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let probe = TokenServer::start([successful_token_response( "redirected-access-token", "redirected-refresh-token", @@ -876,7 +880,7 @@ async fn access_token_does_not_follow_refresh_redirect_and_can_retry() { #[tokio::test] async fn build_defaults_missing_expiry_to_one_hour() { - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let server = TokenServer::start([( StatusCode::OK, serde_json::json!({ @@ -928,7 +932,7 @@ fn build_blocking_loads_valid_cached_token_without_authorizing() { .build() .unwrap(); let token = runtime.block_on(async { - timeout(Duration::from_secs(5), oauth.access_token()) + timeout(HANG_GUARD, oauth.access_token()) .await .expect("cached access token lookup timed out") .unwrap() @@ -987,7 +991,7 @@ fn authorization_endpoint_selection_uses_isolated_environments() { async fn authorization_endpoint_selection_child() { let expected_url = std::env::var("TEST_OAUTH_EXPECTED_AUTHORIZATION_URL") .expect("endpoint selection child must be launched by its parent test"); - timeout(Duration::from_secs(5), async { + timeout(HANG_GUARD, async { let storage = Arc::new(MemoryStorage::default()); let callback = Mutex::new(None); let result = OAuthBuilder::new("test-client") From b3ba93ad474126a885e8316c73a60801c5cf3ead Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Wed, 4 Nov 2026 23:38:21 +0800 Subject: [PATCH 8/9] test(oauth): run the OAuth tests in CI and keep them off real endpoints - CI runs clippy (all targets, -D warnings) and tests for longbridge-oauth; only the longbridge crate was tested, so the loopback regression suite never ran in CI - the endpoint-selection child skips instead of panicking when run without its parent (e.g. --include-ignored) - outside TEST_OAUTH_BASE_URL.scope, tests fail closed instead of silently reaching the real OAuth endpoints; the revoke-without-token test no longer probes the region over the network - the reqwest client has connect/total timeouts, so the authorization code exchange is bounded too --- .github/workflows/ci.yml | 8 ++++++-- rust/crates/oauth/src/client.rs | 18 ++++++++++++++++-- rust/crates/oauth/src/lib.rs | 9 +++++---- rust/crates/oauth/src/tests/flows.rs | 7 +++++-- 4 files changed, 32 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a3375cb9f..6608cfade 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,10 +48,14 @@ jobs: cache-bin: false - name: Check with clippy - run: cargo clippy -p longbridge --all-features + run: | + cargo clippy -p longbridge --all-features + cargo clippy -p longbridge-oauth --all-features --all-targets -- -D warnings - name: Test - run: cargo test -p longbridge --all-features + run: | + cargo test -p longbridge --all-features + cargo test -p longbridge-oauth --all-features check-python-sdk: needs: diff --git a/rust/crates/oauth/src/client.rs b/rust/crates/oauth/src/client.rs index 4b855ae36..5f72809e1 100644 --- a/rust/crates/oauth/src/client.rs +++ b/rust/crates/oauth/src/client.rs @@ -29,8 +29,18 @@ async fn oauth_base_url() -> String { // Keep protocol tests on their own loopback server without process-wide // state. #[cfg(test)] - if let Ok(base) = TEST_OAUTH_BASE_URL.try_with(Clone::clone) { - return base; + { + if let Ok(base) = TEST_OAUTH_BASE_URL.try_with(Clone::clone) { + return base; + } + // Fail closed: a test outside the scope (e.g. a request moved into a + // spawned task, which does not inherit task-locals) must not silently + // reach the real endpoints. Only the endpoint-selection child process + // resolves them on purpose, with the region fixed by environment. + assert!( + std::env::var("TEST_OAUTH_EXPECTED_AUTHORIZATION_URL").is_ok(), + "test reached the real OAuth endpoint; run it inside TEST_OAUTH_BASE_URL.scope" + ); } if std::env::var("LONGBRIDGE_ENV") @@ -406,6 +416,10 @@ fn create_http_client() -> Result { reqwest::Client::builder() // Preserve the previous OAuth transport's protection against redirects. .redirect(reqwest::redirect::Policy::none()) + // Bounds every token request, including the authorization code + // exchange (refresh and revoke are also wrapped in a tokio timeout). + .connect_timeout(Duration::from_secs(10)) + .timeout(TOKEN_REQUEST_TIMEOUT) .build() } diff --git a/rust/crates/oauth/src/lib.rs b/rust/crates/oauth/src/lib.rs index b0f4039fe..affe3fad0 100644 --- a/rust/crates/oauth/src/lib.rs +++ b/rust/crates/oauth/src/lib.rs @@ -477,10 +477,11 @@ mod tests { storage: default_storage(), token: tokio::sync::Mutex::new(None), })); - assert!(matches!( - oauth.revoke().await, - Err(crate::OAuthError::NoTokenAvailable) - )); + // Keep `revoke` off the real endpoints and the region probe. + let result = crate::client::TEST_OAUTH_BASE_URL + .scope("http://127.0.0.1:9/oauth2".to_string(), oauth.revoke()) + .await; + assert!(matches!(result, Err(crate::OAuthError::NoTokenAvailable))); } #[test] diff --git a/rust/crates/oauth/src/tests/flows.rs b/rust/crates/oauth/src/tests/flows.rs index 9ff3b77e2..10e1f5b6c 100644 --- a/rust/crates/oauth/src/tests/flows.rs +++ b/rust/crates/oauth/src/tests/flows.rs @@ -989,8 +989,11 @@ fn authorization_endpoint_selection_uses_isolated_environments() { #[tokio::test] #[ignore = "launched by authorization_endpoint_selection_uses_isolated_environments"] async fn authorization_endpoint_selection_child() { - let expected_url = std::env::var("TEST_OAUTH_EXPECTED_AUTHORIZATION_URL") - .expect("endpoint selection child must be launched by its parent test"); + // Only meaningful when launched by the parent test (which checks this + // test's output); skip under a plain `--include-ignored` run. + let Ok(expected_url) = std::env::var("TEST_OAUTH_EXPECTED_AUTHORIZATION_URL") else { + return; + }; timeout(HANG_GUARD, async { let storage = Arc::new(MemoryStorage::default()); let callback = Mutex::new(None); From 5c34eb00571eca01d7fef0f4d103552f07d79e2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A2=81=E7=AB=A0=E6=B4=AA?= Date: Thu, 5 Nov 2026 10:13:12 +0800 Subject: [PATCH 9/9] fix(oauth): let the 30s token timeout fire first; report transport causes - with both the reqwest client timeout and the tokio timeout at 30s, the client always won and refresh/revoke failed with oauth2 5's bare "Request failed" instead of "timed out after 30s". The client's total timeout is now 35s (compile-time checked to exceed TOKEN_REQUEST_TIMEOUT); it still bounds the code exchange - exchange/refresh/revoke errors include the source chain, so transport failures say what went wrong (oauth2 5 only says "Request failed") - CHANGELOG: code exchange bound and transport error detail --- CHANGELOG.md | 3 ++- rust/crates/oauth/src/client.rs | 45 +++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5be7c0010..29bb2f2eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **All SDKs:** an OAuth token refresh is bounded by a 30-second timeout (`RefreshTokenFailed: timed out after 30s`). It runs under the token lock, so a hung refresh previously blocked every request needing a token, and `revoke()` / `clear()`, indefinitely; a hung refresh while building the client now falls back to the browser authorization after 30 s instead of never returning. Trade-off: if the server rotated the refresh token but its response arrives after the timeout, the new token is lost and the next refresh fails with `invalid_grant`, requiring a new authorization. +- **All SDKs:** an OAuth token refresh is bounded by a 30-second timeout (`RefreshTokenFailed: timed out after 30s`). It runs under the token lock, so a hung refresh previously blocked every request needing a token, and `revoke()` / `clear()`, indefinitely; a hung refresh while building the client now falls back to the browser authorization after 30 s instead of never returning. Trade-off: if the server rotated the refresh token but its response arrives after the timeout, the new token is lost and the next refresh fails with `invalid_grant`, requiring a new authorization. The authorization code exchange is bounded too (10 s to connect, 35 s in total). +- **All SDKs:** OAuth transport failures report their cause (e.g. `Request failed: client error: error sending request ...: tcp connect error: ...`) instead of a bare `Request failed`. - **Java:** `GridContext.create` / `FundContext.create` read the `Config` native handle inside `synchronized (config)`, like every other context, so a concurrent `Config.close()` can no longer free it while it is being used. - **All SDKs:** the OAuth browser authorization flow now uses PKCE (RFC 7636, `S256`): the authorization request carries a `code_challenge` and the token exchange sends the matching `code_verifier`. The server requires PKCE for `public` clients, so previously such clients (e.g. those created through dynamic client registration) could not authorize through the SDK and failed with `OAuth authorization failed: server_error`. Clients that don't require PKCE are unaffected. diff --git a/rust/crates/oauth/src/client.rs b/rust/crates/oauth/src/client.rs index 5f72809e1..19ed99032 100644 --- a/rust/crates/oauth/src/client.rs +++ b/rust/crates/oauth/src/client.rs @@ -301,7 +301,7 @@ impl OAuth { .map_err(|e| { tracing::error!(client_id = %self.0.client_id, error = %e, "failed to exchange authorization code for token"); OAuthError::ExchangeCodeFailed { - message: e.to_string(), + message: error_chain(&e), } })?; @@ -344,7 +344,7 @@ impl OAuth { .map_err(|e| { tracing::error!(client_id = %self.0.client_id, error = %e, "failed to refresh token"); OAuthError::RefreshTokenFailed { - message: e.to_string(), + message: error_chain(&e), } })?; @@ -364,6 +364,27 @@ impl OAuth { /// Upper bound for a refresh or revocation request; both hold the token lock. const TOKEN_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); +/// Total timeout of the HTTP client, which also bounds the authorization code +/// exchange. It must exceed `TOKEN_REQUEST_TIMEOUT`, so that for refresh and +/// revoke the tokio timeout fires first and reports "timed out after 30s" +/// instead of the client's generic transport error. +const HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(35); +const _: () = assert!(HTTP_REQUEST_TIMEOUT.as_secs() > TOKEN_REQUEST_TIMEOUT.as_secs()); + +/// `e` followed by its `source()` chain. oauth2 5 reports every transport +/// failure as a bare "Request failed"; the cause (connection refused, timed +/// out, ...) is only in the sources. +fn error_chain(e: &dyn std::error::Error) -> String { + let mut message = e.to_string(); + let mut source = e.source(); + while let Some(cause) = source { + message.push_str(": "); + message.push_str(&cause.to_string()); + source = cause.source(); + } + message +} + /// Describe a revocation failure, including the server's `error` and /// `error_description` (the `Display` of `ServerResponse` omits them). fn revoke_error_message( @@ -377,7 +398,7 @@ where Some(desc) => format!("{}: {desc}", resp.error()), None => resp.error().to_string(), }, - other => other.to_string(), + other => error_chain(other), } } @@ -417,9 +438,10 @@ fn create_http_client() -> Result { // Preserve the previous OAuth transport's protection against redirects. .redirect(reqwest::redirect::Policy::none()) // Bounds every token request, including the authorization code - // exchange (refresh and revoke are also wrapped in a tokio timeout). + // exchange. Refresh and revoke are wrapped in TOKEN_REQUEST_TIMEOUT, + // which must fire first so they report "timed out after 30s". .connect_timeout(Duration::from_secs(10)) - .timeout(TOKEN_REQUEST_TIMEOUT) + .timeout(HTTP_REQUEST_TIMEOUT) .build() } @@ -595,6 +617,19 @@ mod tests { assert!(oauth.0.token.try_lock().is_ok()); } + #[test] + fn error_chain_includes_the_transport_cause() { + let e: RequestTokenError< + std::io::Error, + StandardErrorResponse, + > = RequestTokenError::Request(std::io::Error::other("connection refused")); + assert_eq!(error_chain(&e), "Request failed: connection refused"); + assert_eq!( + revoke_error_message(&e), + "Request failed: connection refused" + ); + } + #[tokio::test(start_paused = true)] async fn revoke_times_out_and_releases_the_token_lock() { let oauth = oauth_with_storage(Arc::new(LegacyStorage));