Open
Conversation
Contributor
WalkthroughThis PR adds Prism node endpoint configuration and implements post-logout synchronization. It introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OIDCHandler as OIDC Handler
participant UtilsFunc as Utils (logout_sync)
participant LiveNode1 as Live Node 1
participant LiveNode2 as Live Node 2
Client->>OIDCHandler: POST /logout request
activate OIDCHandler
OIDCHandler->>OIDCHandler: Determine redirect response
OIDCHandler->>UtilsFunc: await logout_sync(session, tenant_id)
deactivate OIDCHandler
activate UtilsFunc
UtilsFunc->>LiveNode1: POST /o/logout/sync (sessionKey)
UtilsFunc->>LiveNode2: POST /o/logout/sync (sessionKey)
LiveNode1-->>UtilsFunc: OK
LiveNode2-->>UtilsFunc: OK
deactivate UtilsFunc
OIDCHandler-->>Client: Redirect Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/mod.rs (1)
309-337: Consider logging or handling failed sync responses.The function propagates transport errors but does not check HTTP response status codes. A node returning 4xx/5xx will be silently ignored. While this may be intentional for resilience (logout should proceed even if some nodes fail), consider logging failed sync attempts for observability.
This pattern is consistent with
login_syncabove, so if intentional, no change needed.♻️ Optional: Add response status check with logging
async move { - INTRA_CLUSTER_CLIENT + let response = INTRA_CLUSTER_CLIENT .post(url) .header(header::AUTHORIZATION, node.token) .header(header::CONTENT_TYPE, "application/json") .json(&json!( { "sessionKey": _session } )) .send() .await?; + if !response.status().is_success() { + tracing::warn!("logout_sync failed for node: status {}", response.status()); + } Ok::<(), anyhow::Error>(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/mod.rs` around lines 309 - 337, The logout_sync loop currently sends requests via INTRA_CLUSTER_CLIENT inside for_each_live_node but does not inspect HTTP response status, so 4xx/5xx from nodes are ignored; update the async block in logout_sync to capture the response from .send().await, check response.status().is_success(), and when not successful use the project's logging (e.g., tracing::error! or the same logger used elsewhere) to record the node.identity/domain and status/body to aid observability; ensure the function still returns Ok(()) on per-node failures if you want to preserve resilience (i.e., log and continue rather than propagating an error), matching the behavior of login_sync if that pattern is intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/mod.rs`:
- Around line 256-263: The loop over request cookies currently calls
map.insert(...) which overwrites the COOKIE header so only the last cookie
remains; change the call to map.append(reqwest::header::COOKIE,
reqwest::header::HeaderValue::from_bytes(cookie.as_bytes()).unwrap()) so each
iteration adds a cookie value instead of replacing the previous one (update the
code where req.get_all(...), cookie, and map are used).
---
Nitpick comments:
In `@src/utils/mod.rs`:
- Around line 309-337: The logout_sync loop currently sends requests via
INTRA_CLUSTER_CLIENT inside for_each_live_node but does not inspect HTTP
response status, so 4xx/5xx from nodes are ignored; update the async block in
logout_sync to capture the response from .send().await, check
response.status().is_success(), and when not successful use the project's
logging (e.g., tracing::error! or the same logger used elsewhere) to record the
node.identity/domain and status/body to aid observability; ensure the function
still returns Ok(()) on per-node failures if you want to preserve resilience
(i.e., log and continue rather than propagating an error), matching the behavior
of login_sync if that pattern is intended.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cli.rssrc/handlers/http/oidc.rssrc/rbac/user.rssrc/utils/mod.rs
💤 Files with no reviewable changes (1)
- src/rbac/user.rs
Comment on lines
+256
to
+263
| } else if req.contains_key(actix_web::http::header::COOKIE) { | ||
| // multiple cookies | ||
| for cookie in req.get_all(actix_web::http::header::COOKIE) { | ||
| map.insert( | ||
| reqwest::header::COOKIE, | ||
| reqwest::header::HeaderValue::from_bytes(cookie.as_bytes()).unwrap(), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: Only the last cookie is retained due to insert() overwriting.
The loop iterates over all cookies but insert() replaces the previous value for the same key. Only the last cookie will be preserved. Use append() instead to add multiple cookie values.
🐛 Proposed fix
} else if req.contains_key(actix_web::http::header::COOKIE) {
// multiple cookies
for cookie in req.get_all(actix_web::http::header::COOKIE) {
- map.insert(
+ map.append(
reqwest::header::COOKIE,
reqwest::header::HeaderValue::from_bytes(cookie.as_bytes()).unwrap(),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if req.contains_key(actix_web::http::header::COOKIE) { | |
| // multiple cookies | |
| for cookie in req.get_all(actix_web::http::header::COOKIE) { | |
| map.insert( | |
| reqwest::header::COOKIE, | |
| reqwest::header::HeaderValue::from_bytes(cookie.as_bytes()).unwrap(), | |
| ); | |
| } | |
| } else if req.contains_key(actix_web::http::header::COOKIE) { | |
| // multiple cookies | |
| for cookie in req.get_all(actix_web::http::header::COOKIE) { | |
| map.append( | |
| reqwest::header::COOKIE, | |
| reqwest::header::HeaderValue::from_bytes(cookie.as_bytes()).unwrap(), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/mod.rs` around lines 256 - 263, The loop over request cookies
currently calls map.insert(...) which overwrites the COOKIE header so only the
last cookie remains; change the call to map.append(reqwest::header::COOKIE,
reqwest::header::HeaderValue::from_bytes(cookie.as_bytes()).unwrap()) so each
iteration adds a cookie value instead of replacing the previous one (update the
code where req.get_all(...), cookie, and map are used).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Improvements
Chores