fix: Re-authentication with OIDC doesn't work after 24 hours #1473
Conversation
WalkthroughImplements session lifecycle and OIDC refresh: middleware now detects expired sessions and attempts token refresh; Bearer and expiry are propagated through OIDC/user/session flows; route APIs stop accepting an OpenIdClient parameter; RBAC expiry semantics and session helpers updated; CLI default OIDC scope includes Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Sessions
participant RBAC
participant OIDC as DiscoveredClient
Client->>Middleware: HTTP request (cookie/session key)
Middleware->>Sessions: extract session key & check presence
Middleware->>Sessions: is_session_expired?(key)
alt not expired
Middleware->>RBAC: load user & permissions
Middleware->>Client: forward request
else expired
Middleware->>RBAC: get user (reads optional Bearer)
RBAC-->>Middleware: User + optional Bearer
alt bearer present & OIDC available
Middleware->>OIDC: refresh token using Bearer
OIDC-->>Middleware: new Bearer + expires_in
Middleware->>RBAC: update_user_if_changed(..., bearer)
Middleware->>Sessions: new_session(user, key, expires_in)
Middleware->>Client: forward request (refreshed)
else refresh fails or missing data
Middleware->>Client: 401 Unauthorized
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (3)
src/handlers/http/middleware.rs (1)
212-223: Casting logic is safe but could be clearer.The
expires_inhandling correctly guards against overflow by checking if the value exceedsu32::MAXbefore casting. However, the cast*expires_in as u32relies on the prior check to be safe. Consider usingtry_into()for clarity.let expires_in = if let Some(expires_in) = refreshed_token.expires_in.as_ref() { - // need an i64 somehow - if *expires_in > u32::MAX.into() { - EXPIRY_DURATION - } else { - let v = i64::from(*expires_in as u32); - Duration::seconds(v) - } + match i64::try_from(*expires_in) { + Ok(v) => Duration::seconds(v), + Err(_) => EXPIRY_DURATION, + } } else { EXPIRY_DURATION };src/handlers/http/oidc.rs (2)
330-335: Fragile URL construction: string concatenation for query parameters.Appending query parameters via string concatenation assumes the URL already contains a
?. Whileauth_urltypically includes query params, using proper URL manipulation is safer.- let mut url: String = auth_url.into(); - url.push_str("&access_type=offline&prompt=consent"); + let mut url: Url = auth_url.into(); + url.query_pairs_mut() + .append_pair("access_type", "offline") + .append_pair("prompt", "consent"); HttpResponse::TemporaryRedirect() - .insert_header((header::LOCATION, url)) + .insert_header((header::LOCATION, url.to_string())) .finish()
248-258: Consider extracting duplicate expires_in calculation logic.This expires_in handling logic is duplicated in
src/handlers/http/middleware.rs(lines 212-223). Consider extracting to a shared utility function.// In a shared module, e.g., src/rbac/mod.rs pub fn expires_in_to_duration(expires_in: Option<&u64>) -> Duration { match expires_in { Some(&v) if v <= u32::MAX as u64 => Duration::seconds(v as i64), _ => EXPIRY_DURATION, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/cli.rs(1 hunks)src/handlers/http/middleware.rs(2 hunks)src/handlers/http/modal/ingest_server.rs(2 hunks)src/handlers/http/modal/mod.rs(4 hunks)src/handlers/http/modal/query_server.rs(2 hunks)src/handlers/http/modal/server.rs(3 hunks)src/handlers/http/oidc.rs(17 hunks)src/rbac/map.rs(2 hunks)src/rbac/mod.rs(4 hunks)src/rbac/user.rs(6 hunks)src/utils/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-16T09:50:38.636Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1346
File: src/parseable/streams.rs:319-331
Timestamp: 2025-06-16T09:50:38.636Z
Learning: In Parseable's Ingest or Query mode, the node_id is always available because it's generated during server initialization itself, before the get_node_id_string() function in streams.rs would be called. This makes the .expect() calls on QUERIER_META.get() and INGESTOR_META.get() safe in this context.
Applied to files:
src/handlers/http/modal/ingest_server.rssrc/handlers/http/modal/query_server.rs
📚 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/query_server.rs
📚 Learning: 2025-09-05T09:37:33.939Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1422
File: src/handlers/http/cluster/mod.rs:285-296
Timestamp: 2025-09-05T09:37:33.939Z
Learning: In the Parseable codebase, userid values do not require URL encoding when used in path segments because: (1) for basic/native auth, userid is the same as username, and (2) for OAuth users, userid is the "sub" claim from JWT tokens. Both of these are inherently URL-safe identifiers.
Applied to files:
src/rbac/user.rs
🧬 Code graph analysis (7)
src/utils/mod.rs (1)
src/utils/actix.rs (2)
extract_session_key_from_req(51-71)req(31-31)
src/handlers/http/modal/ingest_server.rs (3)
src/handlers/http/modal/mod.rs (1)
configure_routes(74-76)src/handlers/http/modal/query_server.rs (1)
configure_routes(52-88)src/handlers/http/modal/server.rs (1)
configure_routes(72-114)
src/handlers/http/middleware.rs (6)
src/rbac/map.rs (6)
mut_sessions(99-105)mut_users(67-73)sessions(91-97)users(59-65)from(315-323)from(367-375)src/rbac/mod.rs (1)
roles_to_permission(233-245)src/utils/mod.rs (1)
get_user_from_request(61-69)src/handlers/http/oidc.rs (3)
req(77-77)req(148-148)req(178-178)src/utils/actix.rs (2)
req(31-31)extract_session_key(29-49)src/rbac/user.rs (3)
userid(87-92)userid(253-255)from(215-225)
src/rbac/mod.rs (2)
src/rbac/map.rs (2)
mut_sessions(99-105)roles(75-81)src/rbac/user.rs (1)
roles(113-115)
src/handlers/http/modal/query_server.rs (1)
src/handlers/http/modal/server.rs (2)
configure_routes(72-114)get_oauth_webscope(572-577)
src/handlers/http/modal/mod.rs (3)
src/handlers/http/modal/ingest_server.rs (1)
configure_routes(65-86)src/handlers/http/modal/query_server.rs (1)
configure_routes(52-88)src/handlers/http/modal/server.rs (1)
configure_routes(72-114)
src/handlers/http/oidc.rs (2)
src/rbac/user.rs (6)
username(249-251)roles(113-115)new(355-357)userid(87-92)userid(253-255)new_oauth(70-85)src/rbac/mod.rs (1)
put_user(54-57)
⏰ 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). (10)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
🔇 Additional comments (21)
src/cli.rs (1)
458-461: LGTM! offline_access scope added for refresh token support.The addition of
offline_accessto the default OIDC scope is correct and aligns with the PR's objective to implement OAuth token refresh functionality. The help text has been updated consistently.src/handlers/http/modal/ingest_server.rs (1)
65-65: LGTM! Route configuration simplified.The removal of the
oidc_clientparameter fromconfigure_routesis consistent with the trait definition change and the broader refactoring. This aligns with the comment at line 134 indicating that ingestors shouldn't handle OpenId auth flow.src/utils/mod.rs (1)
62-62: LGTM! Error handling improved.Replacing
unwrap()withmap_err(|_| RBACError::UserDoesNotExist)?is a good improvement that avoids potential panics and provides more explicit error handling.src/handlers/http/modal/query_server.rs (2)
52-52: LGTM! Signature updated consistently.The removal of the
oidc_clientparameter fromconfigure_routesis consistent with the trait definition change and aligns with similar updates in other modal server implementations.
69-69: LGTM! OAuth scope usage updated.The call to
Server::get_oauth_webscope()without parameters is consistent with the updated method signature in server.rs.src/handlers/http/modal/mod.rs (3)
74-76: LGTM! Trait signature simplified.Removing the
oidc_clientparameter from theconfigure_routestrait method is a good refactoring that simplifies the interface. The OIDC client is now injected viaapp_datain thestartmethod instead.
103-103: LGTM! Removed unnecessary Arc wrapping.Changing from
Some(Arc::new(client))toSome(client)is correct becauseOpenIdClientis already defined asArc<openid::Client<Discovered, Claims>>(line 64), so the extraArc::newwas creating double-wrapping.
123-125: LGTM! Route configuration refactored correctly.The OIDC client is now passed via
app_data(line 123) andconfigure_routesis called without parameters (line 125), which is cleaner than threading the client through the call chain.src/rbac/mod.rs (3)
40-40: LGTM! Session expiry duration defined.The
EXPIRY_DURATIONconstant of 1 hour is a reasonable default for OAuth session expiry, especially when paired with refresh token functionality.
152-159: LGTM! Session expiry made configurable.Updating
new_sessionto accept a configurableexpires_inparameter (instead of a hardcoded 7-day duration) provides better flexibility for different authentication scenarios, particularly for OAuth sessions with refresh tokens.
233-245: LGTM! API exposure expanded.Making
roles_to_permissionpublic is appropriate as it's needed by other modules (e.g., OIDC flows) to convert roles to permissions for session management.src/handlers/http/modal/server.rs (2)
72-114: LGTM! Route configuration simplified.The removal of the
oidc_clientparameter fromconfigure_routesis consistent with the trait definition change and simplifies the implementation.
572-577: LGTM! OAuth scope construction simplified.Removing the
oidc_clientparameter fromget_oauth_webscopeis a good refactoring. The OIDC client is now accessed viaapp_datawithin the route handlers instead of being passed through the scope construction.src/rbac/map.rs (1)
171-189: Verify the semantics of the twofalsereturns for consistency.The expiry comparison on line 187 (
expiry < &Utc::now()) is correct for identifying expired sessions. However, the method returnsfalsewhen a session key exists inactive_sessionsbut the corresponding user has no sessions inuser_sessions. This suggests either:
- This is a valid edge case where inconsistent state should be treated as "not expired"
- There's a data consistency issue that should be prevented elsewhere
Clarify whether the second early return (line 183) is intended behavior or if it indicates a bug in session lifecycle management.
src/handlers/http/middleware.rs (2)
240-248: Verify intended behavior: non-OAuth expired sessions are silently renewed.When a non-OAuth user's session expires, this branch creates a new session without re-authentication. This effectively extends sessions indefinitely. Confirm this is the intended behavior, as it bypasses the session expiry for native users.
172-176: LGTM: Session extraction pattern follows actix middleware conventions.The extraction of session key and user id before
service.call(req)is correct. The service future is created but not polled until after validation in the async block.src/rbac/user.rs (2)
70-85: LGTM: Bearer token integration in OAuth user creation.The
new_oauthfunction correctly accepts an optionalBearerand properly boxes theOAuthvalue before assignment toUserType. The boxing is a good optimization to reduce enum size variance.
272-289: LGTM: GroupUser construction correctly handles boxed OAuth.The pattern matching and field access for the boxed
OAuthvariant is correct. The display username fallback chain (name → email → userid) is reasonable.src/handlers/http/oidc.rs (3)
418-438: LGTM: Bearer token stored only in memory, not persisted.The implementation correctly creates the user without the bearer for metadata persistence (lines 424, 429), then adds the bearer only to the in-memory cache (lines 432-435). This is good security practice—refresh tokens should not be persisted to storage.
460-467: LGTM: Early return optimization when no changes detected.The comparison correctly includes bearer in the change detection, avoiding unnecessary metadata updates and session invalidations when nothing has changed.
390-406: LGTM: request_token correctly returns Bearer for downstream use.The function signature update to return
(Claims, Userinfo, Bearer)is appropriate for propagating the bearer token through the OIDC flow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/handlers/http/oidc.rs (1)
248-258: Duplicateexpires_incalculation logic.This logic is duplicated from
middleware.rs. As noted in that file's review, consider extracting to a shared helper.
🧹 Nitpick comments (4)
src/handlers/http/middleware.rs (1)
220-230: Consider extracting duplicateexpires_incalculation logic.This exact logic for handling
expires_inoverflow (checking againstu32::MAX, falling back toEXPIRY_DURATION) is duplicated insrc/handlers/http/oidc.rs(lines 248-258). Consider extracting to a helper function in the RBAC module to avoid divergence.Example helper:
// In src/rbac/mod.rs or a utils module pub fn compute_session_expiry(expires_in: Option<u64>) -> Duration { if let Some(expires_in) = expires_in { if expires_in > u32::MAX.into() { EXPIRY_DURATION } else { Duration::seconds(i64::from(expires_in as u32)) } } else { EXPIRY_DURATION } }src/handlers/http/oidc.rs (3)
330-331: Consider using proper URL query parameter handling.String concatenation assumes
auth_urlalready contains query parameters. While this is typically true for OAuth authorization URLs, usingurl::Urlparsing with proper query parameter APIs would be more robust and self-documenting.- let mut url: String = auth_url.into(); - url.push_str("&access_type=offline&prompt=consent"); + let mut url = Url::parse(&auth_url.to_string()) + .expect("auth_url should be valid URL"); + url.query_pairs_mut() + .append_pair("access_type", "offline") + .append_pair("prompt", "consent"); + let url = url.to_string();
418-437: Add a comment explaining why Bearer is not persisted to metadata.The pattern of passing
Nonetonew_oauthfor metadata, then setting Bearer in memory (lines 431-434) is intentional to avoid persisting sensitive tokens. A brief comment would clarify this design decision for future maintainers.put_metadata(&metadata).await?; - // modify before storing + // Set bearer only in memory - tokens should not be persisted to storage if let user::UserType::OAuth(oauth) = &mut user.ty { oauth.bearer = Some(bearer); }
493-496: Same pattern as put_user - consider adding explanatory comment.Same suggestion as for
put_user: a comment explaining that Bearer is only set in memory (not persisted) would clarify the design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/middleware.rs(2 hunks)src/handlers/http/oidc.rs(17 hunks)src/rbac/user.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T09:37:33.939Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1422
File: src/handlers/http/cluster/mod.rs:285-296
Timestamp: 2025-09-05T09:37:33.939Z
Learning: In the Parseable codebase, userid values do not require URL encoding when used in path segments because: (1) for basic/native auth, userid is the same as username, and (2) for OAuth users, userid is the "sub" claim from JWT tokens. Both of these are inherently URL-safe identifiers.
Applied to files:
src/rbac/user.rs
🧬 Code graph analysis (2)
src/handlers/http/middleware.rs (10)
src/event/mod.rs (1)
map(137-137)src/rbac/map.rs (5)
mut_users(67-73)sessions(91-97)users(59-65)from(315-323)from(367-375)src/rbac/mod.rs (1)
roles_to_permission(233-245)src/utils/mod.rs (1)
get_user_from_request(61-69)src/handlers/http/oidc.rs (3)
req(77-77)req(148-148)req(178-178)src/handlers/livetail.rs (1)
extract_session_key(244-267)src/rbac/user.rs (3)
userid(87-92)userid(259-261)from(221-231)src/parseable/streams.rs (1)
v(603-605)src/storage/localfs.rs (1)
from(744-746)src/utils/time.rs (1)
from(337-341)
src/handlers/http/oidc.rs (2)
src/utils/actix.rs (2)
extract_session_key_from_req(51-71)req(31-31)src/rbac/user.rs (7)
username(255-257)from(221-231)roles(113-115)new(361-363)userid(87-92)userid(259-261)new_oauth(70-85)
⏰ 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). (10)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (12)
src/handlers/http/middleware.rs (4)
27-46: LGTM: Import additions support the new session refresh functionality.The imports are well-organized and necessary for the session lifecycle management implementation.
172-182: LGTM: Session key extraction with proper error handling.The early validation of the session key with a descriptive error message is appropriate for the authentication flow.
197-218: Approve: TOCTOU issue addressed with single lookup pattern.The code now uses
if let Some(user) = users().get(&userid)for a single atomic lookup, addressing the previous race condition concern. The token refresh error handling appropriately returns Unauthorized.
246-261: LGTM: Session tracking with appropriate fallback handling.The implementation correctly handles both the token refresh path (with new expiry from provider) and the fallback path for users without bearer tokens. The session is properly re-tracked with updated permissions in both cases.
src/rbac/user.rs (4)
42-42: LGTM: Boxing OAuth variant is appropriate.The change to
OAuth(Box<OAuth>)is a good optimization sinceOAuthnow contains anOption<Bearer>which increases its size. Boxing keeps theUserTypeenum size reasonable.
70-85: LGTM: Bearer integration in OAuth user creation.The signature update correctly accepts an optional Bearer, allowing for both initial user creation (without bearer) and in-memory updates (with bearer).
188-197: Documentation comment addresses the safety invariant as discussed.The documentation clearly explains the panic condition and why it's safe in practice - the trait is only called after verifying
bearer.is_some(). This addresses the previous review feedback.
278-294: LGTM: GroupUser::from_user correctly handles boxed OAuth.The pattern matching and field access adjustments are appropriate for the boxed OAuth variant.
src/handlers/http/oidc.rs (4)
77-83: LGTM: Safe OIDC client extraction in login and logout handlers.The pattern safely handles the case where OIDC is not configured, avoiding potential panics.
Also applies to: 148-154
308-317: LGTM: Updated signature supports configurable session expiry.The function now accepts
expires_inallowing consistent session duration handling for basic auth exchanges.
390-406: LGTM: Token extraction now includes Bearer for downstream use.The updated return type enables Bearer propagation through the authentication flow.
459-466: Verify: Bearer comparison may trigger updates on every login.The comparison
oauth_user.bearer.as_ref() == Some(&bearer)includes access tokens which change frequently. This means the function won't early-return on subsequent logins even if only the access token changed.This may be intentional to ensure the in-memory Bearer is always current, but verify this is the desired behavior since it bypasses the optimization for unchanged users.
This PR changes session management by storing the refresh token returned by the provider in memory. It is used to renew the session once it expires. Expiry is set based on the expires_in value returned by the provider
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/handlers/http/middleware.rs (1)
184-261: Refactor: Extract session refresh logic to a helper function.The session refresh implementation within the middleware is complex (78 lines) and handles multiple concerns: expiry checking, OIDC client retrieval, token refresh, expiry calculation, user updates, and session tracking. Consider extracting this into a dedicated helper function for better maintainability and testability.
Additionally, the
expires_incalculation logic (lines 220-230) is duplicated insrc/handlers/http/oidc.rs(lines 248-258). Consider extracting this into a shared utility function.Example refactor:
// In a shared utility module (e.g., src/rbac/session_helpers.rs) pub fn calculate_expires_in(bearer: &Bearer) -> Duration { if let Some(expires_in) = bearer.expires_in.as_ref() { if *expires_in > u32::MAX.into() { EXPIRY_DURATION } else { Duration::seconds(i64::from(*expires_in as u32)) } } else { EXPIRY_DURATION } } async fn refresh_expired_session( key: &SessionKey, userid: &str, oidc_client: &DiscoveredClient, ) -> Result<(), Error> { // Move lines 197-259 here // Return Result for cleaner error handling }Then in the middleware:
if sessions().is_session_expired(&key) { if let Some(oidc_client) = get_oidc_client(&http_req) { if let Ok(userid) = userid { refresh_expired_session(&key, &userid, &oidc_client).await?; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/cli.rs(1 hunks)src/handlers/http/middleware.rs(2 hunks)src/handlers/http/modal/ingest_server.rs(2 hunks)src/handlers/http/modal/mod.rs(4 hunks)src/handlers/http/modal/query_server.rs(2 hunks)src/handlers/http/modal/server.rs(3 hunks)src/handlers/http/oidc.rs(17 hunks)src/rbac/map.rs(2 hunks)src/rbac/mod.rs(4 hunks)src/rbac/user.rs(6 hunks)src/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/cli.rs
- src/handlers/http/modal/ingest_server.rs
- src/handlers/http/modal/mod.rs
- src/rbac/map.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-05T09:37:33.939Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1422
File: src/handlers/http/cluster/mod.rs:285-296
Timestamp: 2025-09-05T09:37:33.939Z
Learning: In the Parseable codebase, userid values do not require URL encoding when used in path segments because: (1) for basic/native auth, userid is the same as username, and (2) for OAuth users, userid is the "sub" claim from JWT tokens. Both of these are inherently URL-safe identifiers.
Applied to files:
src/rbac/user.rs
📚 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/query_server.rs
📚 Learning: 2025-06-16T09:50:38.636Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1346
File: src/parseable/streams.rs:319-331
Timestamp: 2025-06-16T09:50:38.636Z
Learning: In Parseable's Ingest or Query mode, the node_id is always available because it's generated during server initialization itself, before the get_node_id_string() function in streams.rs would be called. This makes the .expect() calls on QUERIER_META.get() and INGESTOR_META.get() safe in this context.
Applied to files:
src/handlers/http/modal/query_server.rs
🧬 Code graph analysis (5)
src/rbac/mod.rs (2)
src/rbac/map.rs (2)
mut_sessions(99-105)roles(75-81)src/rbac/user.rs (1)
roles(113-115)
src/utils/mod.rs (2)
src/utils/actix.rs (2)
extract_session_key_from_req(51-71)req(31-31)src/handlers/http/oidc.rs (3)
req(77-77)req(148-148)req(178-178)
src/handlers/http/middleware.rs (3)
src/rbac/map.rs (6)
mut_sessions(99-105)mut_users(67-73)sessions(91-97)users(59-65)from(315-323)from(367-375)src/utils/mod.rs (1)
get_user_from_request(61-69)src/handlers/http/oidc.rs (3)
req(77-77)req(148-148)req(178-178)
src/handlers/http/modal/query_server.rs (1)
src/handlers/http/modal/server.rs (2)
configure_routes(72-114)get_oauth_webscope(572-577)
src/handlers/http/oidc.rs (2)
src/utils/actix.rs (2)
extract_session_key_from_req(51-71)req(31-31)src/rbac/user.rs (7)
username(255-257)from(221-231)roles(113-115)new(361-363)userid(87-92)userid(259-261)new_oauth(70-85)
⏰ 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 aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Kafka x86_64-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 (15)
src/utils/mod.rs (1)
62-62: LGTM! Improved error handling.The change from
unwrap()tomap_err(|_| RBACError::UserDoesNotExist)?properly propagates errors instead of panicking, making the code more robust.src/handlers/http/modal/query_server.rs (1)
52-69: LGTM! Consistent with OIDC handling refactor.The removal of the
oidc_clientparameter fromconfigure_routesand the updated call toget_oauth_webscope()align with the PR's architectural change to centralize OIDC client handling via app data injection rather than route-level configuration.src/handlers/http/modal/server.rs (2)
72-93: LGTM! Simplified route configuration.The removal of the
oidc_clientparameter fromconfigure_routesand the updated OAuth webscope wiring streamline the server configuration and are consistent with the PR's centralized OIDC handling approach.
572-577: LGTM! Simplified OAuth webscope.The
get_oauth_webscopemethod now takes no parameters and returns a scope with login, logout, and code callback routes. This aligns with the new pattern where OIDC client data is accessed via app data instead of being passed through route configuration.src/rbac/mod.rs (3)
40-41: LGTM! Default session expiry constant.The
EXPIRY_DURATIONconstant provides a sensible default of 1 hour for session expiry and supports the OAuth token refresh implementation. This is used as a fallback when OAuth providers don't return anexpires_invalue.
152-159: LGTM! Parameterized session expiry.The addition of the
expires_inparameter tonew_sessionenables dynamic session expiry based on OAuth token lifetimes, which is essential for proper token refresh handling.
233-233: LGTM! Public permissions helper.Making
roles_to_permissionpublic allows the middleware and other modules to compute permissions directly, which is necessary for session refresh logic where permissions need to be recomputed after token refresh.src/handlers/http/middleware.rs (2)
172-182: LGTM! Early extraction of session and user data.Extracting the session key and user ID early in the middleware flow sets up proper authentication context for the subsequent session refresh logic.
252-259: Verify fallback session expiry behavior.When a user exists but has no bearer token to refresh (lines 252-259), the code creates a new session with
EXPIRY_DURATION(1 hour). This could reduce the session lifetime if the original session had a longer expiry. Confirm this is the intended behavior for non-OAuth users or sessions without refresh tokens.src/rbac/user.rs (2)
70-85: LGTM! Bearer token support added to OAuth users.The
new_oauthfunction now accepts an optionalbearerparameter, enabling storage of OAuth tokens for refresh. The Bearer is initially set toNonein metadata (line 424) but populated toSome(bearer)in memory (line 433), which is the correct pattern for sensitive token data.
188-198: LGTM! Documented panic invariant.The
AsRef<Bearer>implementation includes documentation clarifying the panic condition and the safety invariant (bearer is always Some when OIDC is configured and this trait is called). This addresses the previous review concern.src/handlers/http/oidc.rs (4)
77-83: LGTM! Safe OIDC client retrieval.The code safely retrieves the OIDC client from app data with proper None handling, avoiding potential panics.
180-181: LGTM! Bearer token propagation.The
request_tokenfunction now returns theBearertoken along with claims and userinfo, enabling proper session expiry and token refresh implementation.
390-406: LGTM! Bearer token extraction from OIDC response.The function now extracts and returns the Bearer token from the OIDC token response, enabling proper token refresh and session expiry management.
414-437: LGTM! Bearer persistence pattern.The
put_userfunction correctly persists user metadata without the Bearer token (line 424) but stores the Bearer in memory (line 433). This is the appropriate pattern for sensitive token data that shouldn't be persisted to storage.
| let expires_in = if let Some(expires_in) = bearer.expires_in.as_ref() { | ||
| // need an i64 somehow | ||
| if *expires_in > u32::MAX.into() { | ||
| EXPIRY_DURATION | ||
| } else { | ||
| let v = i64::from(*expires_in as u32); | ||
| Duration::seconds(v) | ||
| } | ||
| } else { | ||
| EXPIRY_DURATION | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: expires_in calculation.
The expires_in calculation logic (lines 248-258) is duplicated in src/handlers/http/middleware.rs (lines 220-230). Consider extracting this into a shared utility function to maintain consistency and reduce duplication.
Example:
// In src/rbac/session_helpers.rs or similar
pub fn calculate_session_expiry(bearer: &Bearer) -> Duration {
bearer.expires_in
.as_ref()
.filter(|&exp| *exp <= u32::MAX.into())
.map(|exp| Duration::seconds(i64::from(*exp as u32)))
.unwrap_or(EXPIRY_DURATION)
}Then use calculate_session_expiry(&bearer) in both locations.
🤖 Prompt for AI Agents
In src/handlers/http/oidc.rs around lines 248 to 258 the expires_in calculation
is duplicated elsewhere (src/handlers/http/middleware.rs lines ~220-230);
extract the logic into a shared function (e.g.,
src/rbac/session_helpers.rs::calculate_session_expiry or similar) that accepts a
&Bearer and returns a chrono::Duration, implement the same safe-check (treat
values > u32::MAX or None as EXPIRY_DURATION, otherwise convert to i64 seconds),
make the function public where needed and replace both inline blocks with calls
to that helper to remove duplication and ensure consistent behavior.
| let mut url: String = auth_url.into(); | ||
| url.push_str("&access_type=offline&prompt=consent"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if offline_access is documented or configured in scope handling
rg -n "offline_access" -A3 -B3Repository: parseablehq/parseable
Length of output: 493
🏁 Script executed:
#!/bin/bash
# Also search for access_type and prompt parameters to understand their usage
rg -n "access_type|prompt=consent" -A2 -B2Repository: parseablehq/parseable
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Look for OIDC provider configuration and scope handling
rg -n "scope" src/handlers/http/oidc.rs -A2 -B2 | head -50Repository: parseablehq/parseable
Length of output: 790
🏁 Script executed:
#!/bin/bash
# Check for any provider-specific configuration or comments
rg -n "OIDC|provider" src/handlers/http/oidc.rs -A2 -B2 | head -50Repository: parseablehq/parseable
Length of output: 2037
🏁 Script executed:
#!/bin/bash
# Look at the context around lines 330-331 to understand the full flow
sed -n '320,340p' src/handlers/http/oidc.rsRepository: parseablehq/parseable
Length of output: 877
🏁 Script executed:
#!/bin/bash
# Check if there's any provider detection or conditional logic
rg -n "provider|issuer|DiscoveredClient" src/handlers/http/oidc.rs -A2 -B2 | head -60Repository: parseablehq/parseable
Length of output: 2228
🏁 Script executed:
#!/bin/bash
# Search for any documentation or issues about these parameters
rg -n "access_type|Google|prompt" src/ -A1 -B1Repository: parseablehq/parseable
Length of output: 2826
🏁 Script executed:
#!/bin/bash
# Check the Options struct to understand what parameters are supported
fd -t f "\.rs$" | xargs grep -l "struct Options" | head -5Repository: parseablehq/parseable
Length of output: 75
🏁 Script executed:
#!/bin/bash
# Verify what openid library is being used and check its Options struct
fd -t f "\.rs$" src/ | xargs grep -l "Options\|auth_url" | grep -E "(oidc|openid)" | head -5Repository: parseablehq/parseable
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Check the openid crate being used in Cargo.toml
cat Cargo.toml | grep -A2 -B2 "openid\|oidc"Repository: parseablehq/parseable
Length of output: 218
Remove or conditionally apply Google-specific OAuth parameters to support multiple OIDC providers.
Lines 330-331 append &access_type=offline&prompt=consent to the authorization URL. This creates compatibility issues:
access_type=offlineis a Google OAuth 2.0 parameter, not standard OIDC—it will be ignored or rejected by other OIDC providers (Azure, Okta, etc.)- The
offline_accessscope is already correctly configured in the default CLI options (openid profile email offline_access), which is the standard OIDC approach for obtaining refresh tokens prompt=consentforces users to consent on every login with no configuration option to make this optional
Consider removing the hardcoded parameters or adding provider detection to conditionally apply Google-specific parameters only when the OIDC issuer is Google.
🤖 Prompt for AI Agents
In src/handlers/http/oidc.rs around lines 330-331, the code unconditionally
appends the Google-only query params "&access_type=offline&prompt=consent" to
the auth URL; remove these hardcoded params or apply them only for Google
issuers to avoid breaking other OIDC providers. Fix by either (a) deleting the
push_str call so the standard "offline_access" scope handles refresh tokens, or
(b) add a conditional check using the configured issuer or parsed auth_url host
(e.g., issuer/host contains "accounts.google.com" or exact Google issuer) and
append those query params only when true; optionally make this behavior
configurable via a provider flag. Ensure tests/flows with Azure/Okta still work
and update comments accordingly.
nitisht
changed the title
2 participants
This PR changes session management by storing the refresh token returned by the provider in memory. This is done by asking for the
offline_accessscopeIt is used to renew the session once it expires.
Expiry is set based on the expires_in value returned by the provider
Fixes #1469
Description
This PR has:
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.