fix: create new client if token decoding fails #1495
Conversation
WalkthroughReplaces per-request OIDC client storage with a global, lazily-initialized Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client
participant Handler as HTTP Handler
participant Global as OIDC_CLIENT\n(OnceCell + RwLock<GlobalClient>)
participant Provider as OIDC Provider
participant Middleware as Auth Middleware
Browser->>Handler: GET /login (authorize)
Handler->>Global: read() -> DiscoveredClient
Global-->>Handler: client
Handler->>Provider: redirect / exchange code
Provider-->>Handler: auth code / tokens
Handler->>Global: read() use client to decode id_token
alt decode succeeds
Handler-->>Browser: set session / redirect
else decode fails
Handler->>Provider: build new client, retry decode
Handler->>Global: write() replace GlobalClient
Global-->>Handler: new client installed
Handler-->>Browser: proceed if decode succeeds
end
Browser->>Middleware: request with session
Middleware->>Global: read() client for refresh
Middleware->>Provider: refresh token
alt refresh succeeds
Provider-->>Middleware: new tokens
Middleware-->>Browser: continue request
else refresh fails
Middleware->>Middleware: log, remove session
Middleware-->>Browser: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-28T02:10:41.140ZApplied to files:
📚 Learning: 2025-10-21T02:22:24.403ZApplied to files:
🧬 Code graph analysis (1)src/handlers/http/oidc.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (5)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/http/oidc.rs (1)
151-158: Inconsistent:logoutstill usesapp_datainstead of globalOIDC_CLIENT.The
logoutfunction still retrieves the OIDC client viareq.app_data::<Data<Option<DiscoveredClient>>>(), while all other functions have been migrated to use the globalOIDC_CLIENT. This inconsistency may cause logout functionality to fail since the app_data is no longer being populated.🔎 Proposed fix to use global OIDC_CLIENT
pub async fn logout(req: HttpRequest, query: web::Query<RedirectAfterLogin>) -> HttpResponse { - let oidc_client = match req.app_data::<Data<Option<DiscoveredClient>>>() { - Some(client) => { - let c = client.clone().into_inner(); - c.as_ref().clone() - } - None => None, + let oidc_client = match OIDC_CLIENT.get() { + Some(Some(client)) => Some(client.read().await.client().clone()), + _ => None, }; let Some(session) = extract_session_key_from_req(&req).ok() else { return redirect_to_client(query.redirect.as_str(), None); };Note: This will require making
logoutanasyncfunction if it isn't already, which it is.
🧹 Nitpick comments (2)
src/handlers/http/modal/mod.rs (1)
116-122: Remove unnecessary.clone()on client.The
client.clone()on line 121 is unnecessary sinceGlobalClient::newtakes ownership of the client, and it's not used elsewhere after this point.🔎 Proposed fix
if let Some(config) = oidc_client { let client = config .connect(&format!("{API_BASE_PATH}/{API_VERSION}/o/code")) .await?; OIDC_CLIENT - .get_or_init(|| Some(Arc::new(RwLock::new(GlobalClient::new(client.clone()))))); + .get_or_init(|| Some(Arc::new(RwLock::new(GlobalClient::new(client))))); }src/handlers/http/oidc.rs (1)
415-420: Replace.unwrap()with proper error handling.While reaching this code path implies OIDC was configured, using
.unwrap()is fragile. If configuration state changes or there's a race condition, this will cause a panic.🔎 Proposed fix
- let new_client = PARSEABLE + let openid_config = PARSEABLE .options .openid() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("OIDC configuration not available"))?; + let new_client = openid_config .connect(&format!("{API_BASE_PATH}/{API_VERSION}/o/code")) .await?;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.tomlsrc/handlers/http/middleware.rssrc/handlers/http/modal/mod.rssrc/handlers/http/oidc.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T02:10:41.140Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1453
File: src/parseable/mod.rs:397-400
Timestamp: 2025-10-28T02:10:41.140Z
Learning: In Parseable enterprise deployments with multiple query nodes, hot tier configuration must be persisted in object storage so that newly started query nodes can fetch and synchronize the hot tier settings at startup (file: src/parseable/mod.rs, function: create_stream_and_schema_from_storage).
Applied to files:
src/handlers/http/modal/mod.rs
🧬 Code graph analysis (2)
src/handlers/http/middleware.rs (1)
src/handlers/http/modal/mod.rs (1)
client(75-77)
src/handlers/http/modal/mod.rs (2)
src/cli.rs (1)
openid(528-548)src/handlers/http/modal/ssl_acceptor.rs (1)
get_ssl_acceptor(27-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (6)
src/handlers/http/modal/mod.rs (1)
65-82: LGTM!The
GlobalClientwrapper struct provides a clean API for managing the OIDC client lifecycle, withset()enabling client replacement when JWKS keys become invalid.src/handlers/http/middleware.rs (2)
185-192: LGTM!The migration to the global
OIDC_CLIENTis correctly implemented. The read lock is acquired and released within a narrow scope, with the client being cloned to avoid holding the lock during the subsequent async operations.
211-224: Good security practice: invalidating session on refresh failure.The explicit error handling ensures that users with expired/invalid tokens are properly logged out rather than left in an ambiguous state. The session removal on line 219 prevents lingering invalid sessions.
src/handlers/http/oidc.rs (2)
83-99: LGTM!The login function correctly retrieves the OIDC client from the global
OIDC_CLIENTand handles both the None (no OIDC configured) and Some cases appropriately.
178-193: LGTM!The
reply_loginfunction correctly retrieves the OIDC client from the global state and handles the unavailable case by returning Unauthorized.Cargo.toml (1)
65-65: Compatibility confirmed for openid 0.18.3.The codebase uses standard OpenID Connect client APIs (
request_token,decode_token,validate_token,request_userinfo,refresh_token) that remain compatible with openid 0.18.3. All imported types (Client, Discovered, StandardClaims, Bearer, Token, Userinfo, Options) are documented in the latest version with consistent signatures. No breaking changes found for this upgrade path.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/http/oidc.rs (1)
151-174: Inconsistency:logoutfunction still uses per-request app data.The
logoutfunction (lines 152-158) still retrieves the OIDC client viareq.app_data::<Data<Option<DiscoveredClient>>>(), whileloginandreply_loginhave been migrated to use the globalOIDC_CLIENTstatic. This inconsistency could lead to the logout function receivingNonewhen the OIDC client is actually configured, since the app data is no longer populated with the OIDC client (based on the changes insrc/handlers/http/modal/mod.rsline 138-146 wherecreate_app_fnno longer adds OIDC client to app data).🔎 Proposed fix to use global OIDC_CLIENT
pub async fn logout(req: HttpRequest, query: web::Query<RedirectAfterLogin>) -> HttpResponse { - let oidc_client = match req.app_data::<Data<Option<DiscoveredClient>>>() { - Some(client) => { - let c = client.clone().into_inner(); - c.as_ref().clone() - } - None => None, - }; + let oidc_client = if let Some(client) = OIDC_CLIENT.get() + && let Some(client) = client + { + Some(client.read().await.client().clone()) + } else { + None + }; + let Some(session) = extract_session_key_from_req(&req).ok() else { return redirect_to_client(query.redirect.as_str(), None); };
🧹 Nitpick comments (2)
src/handlers/http/modal/mod.rs (1)
116-124: Consider removing unnecessaryclone().Line 121 calls
client.clone()before passingclienttoGlobalClient::new(), but sinceclientis moved into the constructor anyway, the clone is unnecessary.🔎 Proposed fix
if let Some(config) = oidc_client { let client = config .connect(&format!("{API_BASE_PATH}/{API_VERSION}/o/code")) .await?; OIDC_CLIENT - .get_or_init(|| Some(Arc::new(RwLock::new(GlobalClient::new(client.clone()))))); + .get_or_init(|| Some(Arc::new(RwLock::new(GlobalClient::new(client))))); } else { OIDC_CLIENT.get_or_init(|| None); }Note: The else branch correctly addresses the previous review comment about explicit
Noneinitialization.src/handlers/http/oidc.rs (1)
83-86: Consider simplifying the OIDC client retrieval.The pattern matching for extracting the OIDC client could be simplified for better readability.
🔎 Proposed refactor
- let oidc_client = match OIDC_CLIENT.get() { - Some(c) => c.as_ref().cloned(), - None => None, - }; + let oidc_client = OIDC_CLIENT.get().and_then(|c| c.as_ref().cloned());
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/modal/mod.rssrc/handlers/http/oidc.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T02:10:41.140Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1453
File: src/parseable/mod.rs:397-400
Timestamp: 2025-10-28T02:10:41.140Z
Learning: In Parseable enterprise deployments with multiple query nodes, hot tier configuration must be persisted in object storage so that newly started query nodes can fetch and synchronize the hot tier settings at startup (file: src/parseable/mod.rs, function: create_stream_and_schema_from_storage).
Applied to files:
src/handlers/http/modal/mod.rs
🧬 Code graph analysis (1)
src/handlers/http/oidc.rs (1)
src/handlers/http/modal/mod.rs (1)
client(75-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: coverage
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (5)
src/handlers/http/modal/mod.rs (2)
21-21: LGTM: Imports support the global OIDC client mechanism.The added imports (
from_fn,OnceCell,RwLock, andDiscoveredClient) are necessary for implementing the global OIDC client pattern.Also applies to: 28-28, 34-34, 43-43
63-82: LGTM: GlobalClient struct provides proper encapsulation.The
GlobalClientwrapper with accessor methods provides a clean interface for managing the OIDC client globally. TheOnceCell<Option<Arc<RwLock<GlobalClient>>>>type properly distinguishes between "not initialized" and "OIDC not configured" states.src/handlers/http/oidc.rs (3)
19-19: LGTM: Imports updated for global OIDC client access.The import changes correctly support the new global
OIDC_CLIENTpattern, bringing inArc,RwLock,GlobalClient, and the staticOIDC_CLIENT.Also applies to: 32-32, 37-43
178-193: LGTM: Correctly migrated to global OIDC client access.The signature change removes the unnecessary
HttpRequestparameter, and the function correctly retrieves the OIDC client from the globalOIDC_CLIENTstatic, returningUnauthorizedwhen unavailable.
400-443: Recovery logic correctly addresses past review comments.The updated
request_tokenimplementation properly handles JWKS refresh:
- Authorization code reuse fixed: Line 423 reuses the already-obtained token instead of re-exchanging the consumed authorization code, addressing the critical issue from previous reviews.
- Duplicate decode removed: Line 437 no longer calls
decode_tokenagain; the token is decoded on line 423 (recovery path) or the success path validates only.- Proper lock management: Line 431 explicitly drops the read lock before acquiring the write lock on line 433, preventing deadlock.
Note on concurrency: A minor race exists between releasing the read lock (line 431) and acquiring the write lock (line 433), where multiple threads could sequentially update the client. However, since all threads would create fresh clients with updated JWKS, the end result remains correct.
parmesant
force-pushed
the
oidc-token-refresh
branch
from
ccfa067 to
0ab84c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/handlers/http/modal/mod.rs (1)
116-124: Remove unnecessary clone when initializing GlobalClient.On line 121,
client.clone()is unnecessary becauseclientis not used after this point and can be moved directly intoGlobalClient::new().🔎 Proposed fix
if let Some(config) = oidc_client { let client = config .connect(&format!("{API_BASE_PATH}/{API_VERSION}/o/code")) .await?; OIDC_CLIENT - .get_or_init(|| Some(Arc::new(RwLock::new(GlobalClient::new(client.clone()))))); + .get_or_init(|| Some(Arc::new(RwLock::new(GlobalClient::new(client))))); } else { OIDC_CLIENT.get_or_init(|| None); }src/handlers/http/oidc.rs (2)
83-86: Simplify OIDC client retrieval pattern.The current pattern
c.as_ref().cloned()is correct but verbose. Consider simplifying to improve readability.🔎 Proposed refactor
- let oidc_client = match OIDC_CLIENT.get() { - Some(c) => c.as_ref().cloned(), - None => None, - }; + let oidc_client = OIDC_CLIENT.get().and_then(|opt| opt.clone());Apply the same simplification at line 139 where this pattern is repeated.
429-429: Remove unnecessary explicit drop.The explicit
drop(old_client)is unnecessary becauseold_clientis a clonedDiscoveredClient(not a lock guard) and will be automatically dropped at the end of the scope. Dropping it before acquiring the write lock doesn't reduce lock contention.🔎 Proposed cleanup
let userinfo = new_client.request_userinfo(&token).await?; let bearer = token.bearer; - // replace old client with new one - drop(old_client); - oidc_client.write().await.set(new_client); return Ok((claims, userinfo, bearer));
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/modal/mod.rssrc/handlers/http/oidc.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T02:10:41.140Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1453
File: src/parseable/mod.rs:397-400
Timestamp: 2025-10-28T02:10:41.140Z
Learning: In Parseable enterprise deployments with multiple query nodes, hot tier configuration must be persisted in object storage so that newly started query nodes can fetch and synchronize the hot tier settings at startup (file: src/parseable/mod.rs, function: create_stream_and_schema_from_storage).
Applied to files:
src/handlers/http/modal/mod.rs
🧬 Code graph analysis (2)
src/handlers/http/modal/mod.rs (2)
src/cli.rs (1)
openid(528-548)src/handlers/http/modal/ssl_acceptor.rs (1)
get_ssl_acceptor(27-62)
src/handlers/http/oidc.rs (1)
src/handlers/http/modal/mod.rs (3)
client(75-77)token(590-590)token(600-602)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (3)
src/handlers/http/modal/mod.rs (1)
63-82: LGTM: Global OIDC client wrapper is well-structured.The
GlobalClientstruct and staticOIDC_CLIENTprovide a clean global access pattern with thread-safe mutation viaRwLock. The initialization paths below properly handle both the configured and unconfigured cases.src/handlers/http/oidc.rs (2)
176-191: LGTM: Safe OIDC client access with proper error handling.The let-else chain correctly unwraps the nested
Optionwithout risk of panicking, and error handling appropriately returnsUnauthorizedwhen OIDC is unavailable or token exchange fails.
411-433: LGTM: Recovery logic correctly reuses the obtained token.The recovery path properly creates a new client with fresh JWKS keys and reuses the already-obtained token rather than re-exchanging the authorization code (which would fail since auth codes are single-use). This correctly addresses the previous review feedback.
parmesant
force-pushed
the
oidc-token-refresh
branch
from
0ab84c5 to
2dcfa19
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/handlers/http/oidc.rs (1)
152-155: Critical: Unwrap will panic when OIDC is not configured.Line 153 calls
.unwrap()onc.as_ref(), which will panic when OIDC is not configured. WhenOIDC_CLIENTis initialized toSome(None)(line 122 in modal/mod.rs),c.as_ref()returnsNone, causing the unwrap to panic.This issue was previously reported and marked as addressed, but the problematic code remains.
🔎 Proposed fix
- let oidc_client = match OIDC_CLIENT.get() { - Some(c) => Some(c.as_ref().unwrap().read().await.client().clone()), - None => None, - }; + let oidc_client = if let Some(Some(client)) = OIDC_CLIENT.get() { + Some(client.read().await.client().clone()) + } else { + None + };
🧹 Nitpick comments (2)
src/handlers/http/oidc.rs (1)
411-433: Consider guarding against concurrent client recreation.When token decoding fails, multiple concurrent requests may all attempt to create new OIDC clients (lines 413-418). While only one will succeed in replacing the global client (line 431), the others will waste resources creating clients that are discarded.
Additionally, if JWKS validation fails repeatedly, every request will trigger client recreation without rate limiting, potentially amplifying issues with the identity provider.
For the current use case this is likely acceptable, but if token decode failures become frequent, consider adding:
- A flag/timestamp to coordinate client recreation across concurrent requests
- Rate limiting or circuit breaker logic for client recreation attempts
src/handlers/http/modal/mod.rs (1)
63-82: Consider adding documentation for public API.The public static
OIDC_CLIENTand structGlobalClientlack doc comments. While these are internal APIs, documentation would help maintainers understand:
- The purpose of the
GlobalClientwrapper- Thread-safety guarantees and usage patterns
- Initialization lifecycle and access patterns
Example documentation
/// Global OIDC client store, initialized once at server startup. /// - `None`: OIDC_CLIENT has not been initialized yet /// - `Some(None)`: OIDC is not configured /// - `Some(Some(client))`: OIDC is configured and client is available pub static OIDC_CLIENT: OnceCell<Option<Arc<RwLock<GlobalClient>>>> = OnceCell::new(); /// Wrapper around DiscoveredClient to enable global state management. /// Thread-safe via RwLock; allows client replacement during JWKS refresh. #[derive(Debug)] pub struct GlobalClient { client: DiscoveredClient, }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/modal/mod.rssrc/handlers/http/oidc.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T02:10:41.140Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1453
File: src/parseable/mod.rs:397-400
Timestamp: 2025-10-28T02:10:41.140Z
Learning: In Parseable enterprise deployments with multiple query nodes, hot tier configuration must be persisted in object storage so that newly started query nodes can fetch and synchronize the hot tier settings at startup (file: src/parseable/mod.rs, function: create_stream_and_schema_from_storage).
Applied to files:
src/handlers/http/modal/mod.rs
🧬 Code graph analysis (1)
src/handlers/http/oidc.rs (3)
src/option.rs (1)
url(139-141)src/utils/actix.rs (2)
extract_session_key_from_req(51-71)req(31-31)src/handlers/http/modal/mod.rs (1)
client(75-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (1)
src/handlers/http/modal/mod.rs (1)
116-123: LGTM! OIDC_CLIENT initialization handles both cases correctly.The initialization now properly handles both configured and unconfigured OIDC scenarios, addressing the previously reported concern. The use of
get_or_initensures thread-safe initialization.
parmesant
force-pushed
the
oidc-token-refresh
branch
from
2dcfa19 to
6e44dce
Compare
|
I can confirm my oidc auth flow works with a setup of dex+MS-ADFS after at least approx 20 hours. No 401 detected after logout+login or logout+new auth flow to ADFS+login. |
3 participants
In case the jwks becomes invalid, create a new oidc client with a set of fresh keys
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
Chores
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.