feat: dashboard implementation #1305
Conversation
|
""" WalkthroughThe changes refactor the dashboard feature by standardizing all dashboard and tile identifiers to use ULIDs instead of custom string hashes. The dashboard data model is simplified, removing complex nested structures, version migrations, and user-based filtering. The HTTP handler functions are updated to validate and use ULIDs, and session key extraction is removed from their signatures. The dashboard storage logic is streamlined, discarding version migrations and manual ID assignments. Function signatures and internal logic are adjusted throughout to reflect these updates, resulting in a flatter, more concise dashboard management flow. A new endpoint to add tiles to existing dashboards is introduced with authorization checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP_Handler
participant Dashboards_Store
Client->>HTTP_Handler: Request (list/get/create/update/delete/add_tile dashboard)
HTTP_Handler->>Dashboards_Store: Validate/parse ULID(s)
HTTP_Handler->>Dashboards_Store: Perform action (list, get, create, update, delete, add tile)
Dashboards_Store-->>HTTP_Handler: Dashboard(s) data (using ULIDs)
HTTP_Handler-->>Client: Response (dashboard(s) data or status)
Possibly related PRs
Suggested labels
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/prism/home/mod.rs (1)
93-109: 🛠️ Refactor suggestion
⚠️ Potential issueRemoval of authorisation filter exposes all dashboards to every user
get_dashboard_titles()is now invoked without aSessionKey, so the home page will show titles/ids of dashboards belonging to all users.
This is a regression from the previous behaviour (filtered bySessionKey) and leaks potentially sensitive metadata.Consider either:
- Restoring a keyed
list_dashboards(key)variant, or- Filtering the returned dashboards by
dashboard.author == key.user_hashafter the call.Failing to do so turns an authenticated-but-isolated view into a global, privacy-breaking one.
🧹 Nitpick comments (2)
src/users/dashboards.rs (2)
38-43: Inconsistent ID types betweenTileandLayout
Tile.tile_idis aUlid, butLayout.tile_idis anOption<String>.
This increases the risk of runtime conversions and invalid data sneaking in.Unless there is a compelling reason, store the same concrete type:
-pub struct Layout { - pub tile_id: Option<String>, +pub struct Layout { + pub tile_id: Option<Ulid>,(Serde will serialise
Ulidto/from strings automatically.)
102-111:list_dashboardsclones every dashboard – consider returning referencesFor read-heavy workloads the full clone can become expensive.
You could returnArc<Dashboard>or borrow-guarded slices instead, e.g.:pub async fn list_dashboards(&self) -> Vec<Dashboard> { self.0.read().await.clone() }Not critical but may help with memory pressure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/users/dashboards.rs(4 hunks)src/prism/home/mod.rs(2 hunks)src/users/dashboards.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
update list dashboards
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/users/dashboards.rs (1)
59-78:⚠️ Potential issue
load()never purges dashboards removed from storage → the in-memory index drifts.
Dashboards::load()accumulates bys.append(&mut this)after the write-lock.
If an object is deleted in the bucket,thissimply omits it, leaving the stale copy already present inself.0.Replace the append with a full swap so RAM reflects the source-of-truth:
let mut s = self.0.write().await; -s.append(&mut this); +*s = this;Also consider returning the old vector (or its length) for observability.
♻️ Duplicate comments (4)
src/handlers/http/users/dashboards.rs (4)
38-60:⚠️ Potential issue
list()leaks all dashboards to every caller and may panic onmodified.unwrap().
- Ownership / tenancy checks that were requested in previous reviews are still missing – any authenticated user can enumerate dashboards system-wide.
dashboard.modified.unwrap()will crash for dashboards created before this field existed.-let dashboards: Vec<Map<_, _>> = dashboards - .iter() - .map(|dashboard| { - let mut map = Map::new(); - … - map.insert( - "modified".to_string(), - serde_json::Value::String(dashboard.modified.unwrap().to_string()), - ); - … - }) - .collect(); +let caller = get_hash(&get_user_from_request(&req)?); +let dashboards: Vec<Map<_, _>> = dashboards + .into_iter() + .filter(|d| d.author == caller) + .map(|d| { + let mut map = Map::new(); + map.insert("title".into(), d.title.into()); + map.insert("author".into(), d.author.into()); + map.insert( + "modified".into(), + d.modified + .map(|ts| ts.to_rfc3339()) + .unwrap_or_default() + .into(), + ); + map + }) + .collect();This closes the multi-tenant data leak (previously reported) and removes the potential panic.
111-123:⚠️ Potential issue
update()still lets a user hijack another user’s dashboard.You only check existence, not ownership, then overwrite
dashboard.authorwith the caller’s hash.
See prior reviewer notes – the attack vector is unchanged.Add an explicit author match before allowing the update.
148-154:⚠️ Potential issue
delete()repeats the cross-tenant deletion defect.Same pattern: absence of
existing.author == callercheck allows any user to evict another user’s dashboard from the in-memory cache (while leaving stale objects in storage).Apply the author check analogue to the one suggested for
update().
165-189: 🛠️ Refactor suggestion
add_tilelacks ownership validation and rewritesdashboard.versionblindly.
- Missing
dashboard.author == callercheck → tile injection into someone else’s board.- Overwriting
versionon every call loses history and hinders concurrent edits.Add an author guard and only bump
modified, not the semantic version (or introduce a revision field).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/modal/server.rs(1 hunks)src/handlers/http/users/dashboards.rs(4 hunks)src/users/dashboards.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-unknown-linux-gnu
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/handlers/http/users/dashboards.rs (1)
63-74:⚠️ Potential issueSecurity concern:
get()function allows cross-tenant accessSimilar to the
list()function, theget()function retrieves a dashboard without verifying that the requester is the owner. This allows any user to access dashboards they don't own if they know the dashboard ID.-pub async fn get(dashboard_id: Path<String>) -> Result<impl Responder, DashboardError> { +pub async fn get(req: HttpRequest, dashboard_id: Path<String>) -> Result<impl Responder, DashboardError> { + let user_id = get_hash(&get_user_from_request(&req)?); let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; if let Some(dashboard) = DASHBOARDS.get_dashboard(dashboard_id).await { + if dashboard.author.as_ref().map_or(true, |author| author != &user_id) { + return Err(DashboardError::Metadata("Unauthorized to access dashboard")); + } return Ok((web::Json(dashboard), StatusCode::OK)); }
♻️ Duplicate comments (3)
src/handlers/http/users/dashboards.rs (3)
117-122:⚠️ Potential issueSecurity concern:
update()allows hijacking another user's dashboardThe current implementation checks if the dashboard exists but not if the user making the request is the owner. Additionally, it overwrites the
authorfield with the current user's hash, effectively allowing a user to take ownership of any dashboard.- if DASHBOARDS.get_dashboard(dashboard_id).await.is_none() { - return Err(DashboardError::Metadata("Dashboard does not exist")); - } - dashboard.dashboard_id = Some(dashboard_id); - dashboard.author = Some(user_id.clone()); + let existing = DASHBOARDS.get_dashboard(dashboard_id) + .await + .ok_or(DashboardError::Metadata("Dashboard does not exist"))?; + + if existing.author.as_ref().map_or(true, |author| author != &user_id) { + return Err(DashboardError::Metadata("Unauthorized to modify dashboard")); + } + + dashboard.dashboard_id = Some(dashboard_id); + dashboard.author = existing.author;
148-160:⚠️ Potential issueSecurity concern:
delete()function allows cross-tenant deletionSimilar to other endpoints, the
delete()function allows a user to delete any dashboard without verifying ownership.let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; - if DASHBOARDS.get_dashboard(dashboard_id).await.is_none() { - return Err(DashboardError::Metadata("Dashboard does not exist")); + + let existing = DASHBOARDS.get_dashboard(dashboard_id) + .await + .ok_or(DashboardError::Metadata("Dashboard does not exist"))?; + + if existing.author.as_ref().map_or(true, |author| author != &user_id) { + return Err(DashboardError::Metadata("Unauthorized to delete dashboard")); } let path = dashboard_path(&user_id, &format!("{}.json", dashboard_id));
38-60: 💡 Verification agent🧩 Analysis chain
Security concern:
list()function allows cross-tenant accessThe current implementation of
list()returns all dashboards in the system without any user-based filtering. This means any authenticated user can access dashboards created by other users, violating multi-tenant security boundaries.-pub async fn list() -> Result<impl Responder, DashboardError> { - let dashboards = DASHBOARDS.list_dashboards().await; +pub async fn list(req: HttpRequest) -> Result<impl Responder, DashboardError> { + let user_id = get_hash(&get_user_from_request(&req)?); + let dashboards = DASHBOARDS.list_dashboards().await + .into_iter() + .filter(|d| d.author.as_ref().map_or(false, |a| a == &user_id)) + .collect::<Vec<_>>();
🏁 Script executed:
#!/bin/bash # Checking if there are any access controls for the list endpoint elsewhere in the code rg -n "list_dashboards.*filter" --type rustLength of output: 45
Let’s verify how
DASHBOARDSand itslist_dashboardsmethod are defined to confirm whether tenant‐scoping is already enforced:
🏁 Script executed:
#!/bin/bash # Locate the DASHBOARDS constant or static rg -n "DASHBOARDS" --type rust # Locate definition(s) of list_dashboards rg -n "fn list_dashboards" --type rustLength of output: 1899
Let’s inspect the
list_dashboardsimplementation to confirm whether tenant scoping is applied:
🏁 Script executed:
#!/bin/bash rg -n -A20 'pub async fn list_dashboards' --type rust src/users/dashboards.rsLength of output: 368
Enforce tenant scoping in
list()
Thelist()handler currently returns every dashboard in memory—there’s no per-user or per-tenant filter. This allows any authenticated user to see dashboards they don’t own.• File:
src/handlers/http/users/dashboards.rs(around lines 38–60)
• Change the signature to accept the request context so you can extract the caller’s user/tenant ID.
• Extract the user ID (e.g. viaget_user_from_request(&req)?) and filter the returned dashboards bydashboard.author == user_id.
• Optionally, push this logic into the storage layer as a new method (e.g.list_dashboards_for_user).Suggested diff:
-pub async fn list() -> Result<impl Responder, DashboardError> { +pub async fn list(req: HttpRequest) -> Result<impl Responder, DashboardError> { // load all dashboards… let dashboards = DASHBOARDS.list_dashboards().await; + // extract and enforce per-user tenant scope + let user_id = get_user_from_request(&req)?; + let dashboards = dashboards + .into_iter() + .filter(|d| d.author.as_ref().map_or(false, |a| a == &user_id)) + .collect::<Vec<_>>(); // build JSON response let dashboards: Vec<Map<String, serde_json::Value>> = dashboards .iter() .map(|dashboard| { … }) .collect(); Ok((web::Json(dashboards), StatusCode::OK))
🧹 Nitpick comments (1)
src/users/dashboards.rs (1)
31-36: Consider implementing a builder pattern for the Tile structThe
Tilestruct has a simplistic design with just atile_idandother_fields, but creating and managing these objects could be made more ergonomic with a builder pattern.Consider implementing a builder pattern for the
Tilestruct to make it easier to create and manipulate tiles:impl Tile { pub fn new(tile_id: Ulid) -> Self { Self { tile_id, other_fields: None, } } pub fn with_field(mut self, key: &str, value: Value) -> Self { let map = self.other_fields.get_or_insert_with(|| serde_json::Map::new()); map.insert(key.to_string(), value); self } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/users/dashboards.rs(4 hunks)src/prism/home/mod.rs(2 hunks)src/users/dashboards.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/prism/home/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
src/users/dashboards.rs (3)
162-169:⚠️ Potential issuePotential null reference in get_dashboard
Similar to the delete method, this function unconditionally unwraps the optional
dashboard_idfield, which could lead to runtime panics.Use a safer approach with
map_or:pub async fn get_dashboard(&self, dashboard_id: Ulid) -> Option<Dashboard> { self.0 .read() .await .iter() - .find(|d| *d.dashboard_id.as_ref().unwrap() == dashboard_id) + .find(|d| d.dashboard_id.as_ref().map_or(false, |id| *id == dashboard_id)) .cloned() }
154-154:⚠️ Potential issuePotential null reference in retain clause
The
unwrap()call ondashboard_idcould lead to runtime panics if any dashboard has a null ID.Use a safer approach with
map_or:-s.retain(|d| *d.dashboard_id.as_ref().unwrap() != dashboard_id); +s.retain(|d| d.dashboard_id.as_ref().map_or(true, |id| *id != dashboard_id));
48-57: 🛠️ Refactor suggestionSimplified
Dashboardstruct with optional fieldsThe refactored dashboard structure removes several complex fields and uses optional types for flexibility. However, without proper initialization safeguards, this could lead to runtime panics when accessing these optional fields.
Implement a constructor method to ensure all fields are properly initialized:
pub struct Dashboard { pub version: Option<String>, pub title: String, pub author: Option<String>, pub dashboard_id: Option<Ulid>, pub modified: Option<DateTime<Utc>>, dashboard_type: Option<DashboardType>, pub tiles: Vec<Tile>, +} + +impl Dashboard { + pub fn new(title: String) -> Self { + Self { + version: Some(CURRENT_DASHBOARD_VERSION.to_string()), + title, + author: None, + dashboard_id: Some(Ulid::new()), + modified: Some(Utc::now()), + dashboard_type: Some(DashboardType::Dashboard), + tiles: Vec::new(), + } + } + + pub fn with_author(mut self, author: String) -> Self { + self.author = Some(author); + self + } + + pub fn add_tile(&mut self, tile: Tile) { + self.tiles.push(tile); + self.modified = Some(Utc::now()); + } }src/handlers/http/users/dashboards.rs (4)
59-68:⚠️ Potential issueSecurity issue: The get endpoint bypasses ownership checks
The
gethandler doesn't verify if the requesting user is the author of the dashboard, allowing unauthorized access to dashboards.Add authorization checks:
-pub async fn get(dashboard_id: Path<String>) -> Result<impl Responder, DashboardError> { +pub async fn get(req: HttpRequest, dashboard_id: Path<String>) -> Result<impl Responder, DashboardError> { + let mut user_id = get_user_from_request(&req)?; + user_id = get_hash(&user_id); let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; if let Some(dashboard) = DASHBOARDS.get_dashboard(dashboard_id).await { + if dashboard.author.as_deref() != Some(&user_id) { + return Err(DashboardError::Metadata("Unauthorized to access dashboard")); + } return Ok((web::Json(dashboard), StatusCode::OK)); }
92-97:⚠️ Potential issueSecurity issue: The update endpoint could hijack dashboards
The
updatehandler sets the author field to the current user without verifying if the requesting user is the original author, allowing users to hijack dashboards created by others.Check dashboard ownership before updating:
let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; - dashboard.author = Some(user_id.clone()); + + // Check if dashboard exists and user is the author + let existing = DASHBOARDS + .get_dashboard(dashboard_id) + .await + .ok_or(DashboardError::Metadata("Dashboard does not exist"))?; + + if existing.author.as_deref() != Some(&user_id) { + return Err(DashboardError::Metadata("Unauthorized to modify dashboard")); + } + + // Preserve original author + dashboard.author = existing.author;
111-116:⚠️ Potential issueSecurity issue: The delete endpoint lacks ownership verification
The
deletehandler doesn't verify if the requesting user is the author of the dashboard, allowing unauthorized deletion.Add ownership verification:
let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; + + // Check if dashboard exists and user is the author + let existing = DASHBOARDS + .get_dashboard(dashboard_id) + .await + .ok_or(DashboardError::Metadata("Dashboard does not exist"))?; + + if existing.author.as_deref() != Some(&user_id) { + return Err(DashboardError::Metadata("Unauthorized to delete dashboard")); + } + DASHBOARDS.delete_dashboard(&user_id, dashboard_id).await?;
121-146:⚠️ Potential issueSecurity issue: The add_tile endpoint lacks authorization checks
The newly added
add_tilehandler doesn't verify if the requesting user is the author of the dashboard, allowing unauthorized modifications.Add authorization checks:
let mut dashboard = DASHBOARDS .get_dashboard(dashboard_id) .await .ok_or(DashboardError::Metadata("Dashboard does not exist"))?; + + // Verify that the user is the author of the dashboard + if dashboard.author.as_deref() != Some(&user_id) { + return Err(DashboardError::Metadata("Unauthorized to modify dashboard")); + } + if tile.tile_id.is_nil() { tile.tile_id = Ulid::new(); }
🧹 Nitpick comments (1)
src/users/dashboards.rs (1)
67-67: Check if deconstruction is necessaryThis line is iterating over key-value pairs, but only the values (dashboards) are used inside the loop. Consider simplifying this.
-for (_, dashboards) in all_dashboards { +for dashboards in all_dashboards.into_values() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/users/dashboards.rs(4 hunks)src/users/dashboards.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (3)
src/users/dashboards.rs (3)
33-33: Version reset fromv3tov1The dashboard version constant has been changed from "v3" to "v1". This suggests a reset in the versioning scheme, which is consistent with the simplified data model approach.
35-40: NewDashboardTypeenum looks goodThe addition of the
DashboardTypeenum provides a clear categorization system for dashboards.
42-47: SimplifiedTilestruct withUlidfor tile_idThe refactored
Tilestruct now usesUlidfor identifiers instead of string hashes and flattens additional properties into a flexible JSON map. This provides better type safety and standardization.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/handlers/http/users/dashboards.rs (2)
77-87: 🛠️ Refactor suggestionThe
postfunction should validate the Dashboard before storageThe function properly assigns authorship but doesn't validate the dashboard before storing. Add validation to ensure:
- Required fields are present (title at minimum)
- Tile IDs are valid ULIDs or get assigned new ones
- Dashboard ID is set to a new ULID if not present
let mut user_id = get_user_from_request(&req)?; user_id = get_hash(&user_id); dashboard.author = Some(user_id.clone()); + + // Validate required fields + if dashboard.title.trim().is_empty() { + return Err(DashboardError::Metadata("Dashboard title cannot be empty")); + } + + // Ensure dashboard ID is set + if dashboard.dashboard_id.is_none() || dashboard.dashboard_id.unwrap().is_nil() { + dashboard.dashboard_id = Some(Ulid::new()); + } + + // Ensure all tiles have valid IDs + for tile in &mut dashboard.tiles { + if tile.tile_id.is_nil() { + tile.tile_id = Ulid::new(); + } + } DASHBOARDS.create(&user_id, &mut dashboard).await?;
153-183: 🛠️ Refactor suggestionConsider refactoring common ULID validation into a helper function
The same ULID parsing and validation logic is repeated in multiple places. Extract it to a helper function:
fn validate_dashboard_id(id_str: String) -> Result<Ulid, DashboardError> { Ulid::from_string(&id_str) .map_err(|_| DashboardError::Metadata("Invalid dashboard ID")) }Then use it in all handlers to reduce duplication:
- let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { - dashboard_id - } else { - return Err(DashboardError::Metadata("Invalid dashboard ID")); - }; + let dashboard_id = validate_dashboard_id(dashboard_id.into_inner())?;
♻️ Duplicate comments (5)
src/handlers/http/users/dashboards.rs (5)
63-74:⚠️ Potential issueEnforce author-bound access in GET
/dashboards/{id}The current handler retrieves and returns any dashboard without checking ownership. This allows any user who knows a valid dashboard ID to access dashboards belonging to other users.
if let Some(dashboard) = DASHBOARDS.get_dashboard(dashboard_id).await { + // Get the current user's ID to verify ownership + let mut user_id = get_user_from_request(&req)?; + user_id = get_hash(&user_id); + + // Verify the dashboard belongs to the requesting user + if dashboard.author.as_ref().map_or(true, |author| author != &user_id) { + return Err(DashboardError::Metadata("Unauthorized to access dashboard")); + } return Ok((web::Json(dashboard), StatusCode::OK)); }You need to:
- Add HttpRequest parameter to the function signature
- Extract the user_id from the request
- Check if the dashboard author matches the requesting user
89-107:⚠️ Potential issue
updatelets a user hijack another user's dashboardThe function doesn't verify that the user updating the dashboard is its owner. This allows users to modify dashboards they don't own.
let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; + + // Verify that the dashboard exists and belongs to the user + if let Some(existing) = DASHBOARDS.get_dashboard(dashboard_id).await { + if existing.author.as_ref().map_or(true, |author| author != &user_id) { + return Err(DashboardError::Metadata("Unauthorized to modify dashboard")); + } + } else { + return Err(DashboardError::Metadata("Dashboard does not exist")); + } + dashboard.author = Some(user_id.clone()); DASHBOARDS .update(&user_id, dashboard_id, &mut dashboard) .await?;
109-123:⚠️ Potential issue
deletesuffers from the same cross-tenant issueWithout verifying ownership, a malicious user can delete dashboards they don't own.
let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; + + // Verify that the dashboard exists and belongs to the user + if let Some(existing) = DASHBOARDS.get_dashboard(dashboard_id).await { + if existing.author.as_ref().map_or(true, |author| author != &user_id) { + return Err(DashboardError::Metadata("Unauthorized to delete dashboard")); + } + } else { + return Err(DashboardError::Metadata("Dashboard does not exist")); + } + DASHBOARDS.delete_dashboard(&user_id, dashboard_id).await?;
34-60:⚠️ Potential issueSecurity issue: The list endpoint leaks dashboards across tenants
The
list()function doesn't filter dashboards by the caller's identity, allowing any authenticated user to see all dashboards in the system regardless of ownership.Implement authorization checks:
-pub async fn list() -> Result<impl Responder, DashboardError> { - let dashboards = DASHBOARDS.list_dashboards().await; +pub async fn list(req: HttpRequest) -> Result<impl Responder, DashboardError> { + let mut user_id = get_user_from_request(&req)?; + user_id = get_hash(&user_id); + let dashboards = DASHBOARDS.list_dashboards().await + .into_iter() + .filter(|d| d.author.as_ref().map_or(false, |author| author == &user_id)) + .collect::<Vec<_>>();Also, the function unconditionally unwraps author and modified fields, which could cause runtime panics:
- map.insert( - "author".to_string(), - serde_json::Value::String(dashboard.author.as_ref().unwrap().clone()), - ); + if let Some(author) = &dashboard.author { + map.insert( + "author".to_string(), + serde_json::Value::String(author.clone()), + ); + }Apply similar changes for the modified and dashboard_id fields.
125-151:⚠️ Potential issueSecurity concern: New
add_tile()function also lacks authorization checksThe newly added function allows users to modify any dashboard without verifying ownership.
let dashboard_id = if let Ok(dashboard_id) = Ulid::from_string(&dashboard_id.into_inner()) { dashboard_id } else { return Err(DashboardError::Metadata("Invalid dashboard ID")); }; let mut dashboard = DASHBOARDS .get_dashboard(dashboard_id) .await .ok_or(DashboardError::Metadata("Dashboard does not exist"))?; + + // Verify the dashboard belongs to the user + if dashboard.author.as_ref().map_or(true, |author| author != &user_id) { + return Err(DashboardError::Metadata("Unauthorized to modify dashboard")); + } + if tile.tile_id.is_nil() { tile.tile_id = Ulid::new(); }Also consider validating the tile data for required fields and consistent format before adding it to the dashboard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/users/dashboards.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/handlers/http/users/dashboards.rs (1)
34-59:⚠️ Potential issueHandle potential null values to prevent panics in
listThe code unconditionally unwraps optional values, which could cause runtime panics:
- map.insert( - "author".to_string(), - serde_json::Value::String(dashboard.author.as_ref().unwrap().clone()), - ); + if let Some(author) = &dashboard.author { + map.insert( + "author".to_string(), + serde_json::Value::String(author.clone()), + ); + }Apply similar checks for
modifiedanddashboard_idfields as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/users/dashboards.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/handlers/http/users/dashboards.rs (2)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.832Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:63-71
Timestamp: 2025-05-01T10:22:45.684Z
Learning: Dashboard design in Parseable follows a sharing model where any user can read (list/get) any dashboard, enabling dashboard sharing functionality, but only the dashboard author can modify (update/delete) their own dashboards.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- 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: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (4)
src/handlers/http/users/dashboards.rs (4)
63-74: The dashboard access model allows reading by any userThis implementation correctly follows the sharing model described in the retrieved learnings, where any user can read any dashboard (enabling sharing functionality).
This approach allows dashboard sharing but puts the responsibility of proper authorization on the modify operations.
77-87: Dashboard creation logic looks goodThe code correctly sets the author to the authenticated user and delegates dashboard creation to the centralized storage method.
89-107:✅ Verification successful
Update function correctly handles dashboard ID validation
The function properly validates the dashboard ID format and delegates update responsibility to
DASHBOARDS.update, which should handle ownership verification internally.
🏁 Script executed:
#!/bin/bash # Check if DASHBOARDS.update verifies ownership rg -A 10 "fn update.*user_id.*dashboard_id" src/users/dashboards.rsLength of output: 69
To verify that
DASHBOARDS.updateenforces ownership, let’s first locate whereDASHBOARDSis defined and inspect itsupdateimplementation:
🏁 Script executed:
#!/bin/bash # Locate the definition of DASHBOARDS and the Dashboards struct rg -n "DASHBOARDS" . rg -n "struct Dashboards" .Length of output: 1634
Let’s inspect the
Dashboardsimplementation to confirm whetherupdateenforces ownership:
🏁 Script executed:
#!/bin/bash # Show the impl block and the update method in the storage layer rg -A20 "impl Dashboards" src/users/dashboards.rs rg -A20 "fn update" src/users/dashboards.rsLength of output: 1711
Update handler and ownership enforcement are correct
The HTTP handler validates the dashboard ID format and delegates to
DASHBOARDS.update, which internally callsget_dashboard_by_userto ensure the dashboard belongs to the current user before applying any changes. No further action is needed.
109-123:✅ Verification successful
Delete function correctly validates dashboard ID
The function properly validates the dashboard ID format and delegates deletion to
DASHBOARDS.delete_dashboard, which should handle ownership verification internally.
🏁 Script executed:
#!/bin/bash # Check if DASHBOARDS.delete_dashboard verifies ownership rg -A 10 "fn delete_dashboard.*user_id.*dashboard_id" src/users/dashboards.rsLength of output: 79
I couldn’t find the
delete_dashboardimplementation in the handler file; let’s search the entire codebase to locate its definition and confirm ownership checks.
🏁 Script executed:
#!/bin/bash # Search for the delete_dashboard function and the DASHBOARDS static rg -n "fn delete_dashboard" . rg -n "DASHBOARDS" .Length of output: 1625
🏁 Script executed:
#!/bin/bash # Extract implementation of delete_dashboard from business logic layer rg -n -A 20 "pub async fn delete_dashboard" src/users/dashboards.rsLength of output: 819
Ownership validation in
delete_dashboardconfirmedThe
deletehandler correctly parses and validates the dashboard ID, then callsDASHBOARDS.delete_dashboard(&user_id, dashboard_id). The implementation ofdelete_dashboardinsrc/users/dashboards.rs(lines 151–157) explicitly checks ownership viaget_dashboard_by_user(...).await.is_none()and returns an error if the dashboard doesn’t belong to the user.No further changes are needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/users/dashboards.rs (3)
146-169: Clippy warning: Use is_some_and instead of map_orThe code contains a pattern that can be simplified according to Clippy.
- self.0.write().await.retain(|d| { - d.dashboard_id - .as_ref() - .map_or(false, |id| *id == dashboard_id) - }); + self.0.write().await.retain(|d| { + d.dashboard_id + .as_ref() + .is_some_and(|id| *id == dashboard_id) + });🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 163-165: Clippy lint: this
map_orcan be simplified. Useis_some_andinstead of.map_or(false, |id| *id == dashboard_id)(clippy::unnecessary_map_or).
171-182: Clippy warning: Use is_some_and instead of map_orSimilar to the previous comment, this code can be simplified.
- self.0 - .read() - .await - .iter() - .find(|d| { - d.dashboard_id - .as_ref() - .map_or(false, |id| *id == dashboard_id) - }) - .cloned() + self.0 + .read() + .await + .iter() + .find(|d| { + d.dashboard_id + .as_ref() + .is_some_and(|id| *id == dashboard_id) + }) + .cloned()🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 177-179: Clippy lint: this
map_orcan be simplified. Useis_some_andinstead of.map_or(false, |id| *id == dashboard_id)(clippy::unnecessary_map_or).
184-200: Clippy warning: Use is_some_and instead of map_orThe same pattern appears here and should be simplified according to Clippy.
- .find(|d| { - d.dashboard_id - .as_ref() - .map_or(false, |id| *id == dashboard_id) - && d.author == Some(user_id.to_string()) - }) + .find(|d| { + d.dashboard_id + .as_ref() + .is_some_and(|id| *id == dashboard_id) + && d.author == Some(user_id.to_string()) + })🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 194-196: Clippy lint: this
map_orcan be simplified. Useis_some_andinstead of.map_or(false, |id| *id == dashboard_id)(clippy::unnecessary_map_or).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/users/dashboards.rs(6 hunks)src/users/dashboards.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/handlers/http/users/dashboards.rs (3)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.832Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:63-71
Timestamp: 2025-05-01T10:22:45.684Z
Learning: Dashboard design in Parseable follows a sharing model where any user can read (list/get) any dashboard, enabling dashboard sharing functionality, but only the dashboard author can modify (update/delete) their own dashboards.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:125-148
Timestamp: 2025-05-01T10:33:51.746Z
Learning: When adding a tile to a dashboard in `add_tile()` function, the tile ID must be provided by the client and should not be generated by the server. If the tile ID is missing (nil), the API should fail the operation with an appropriate error message.
src/users/dashboards.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.832Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
🪛 GitHub Actions: Lint, Test and Coverage Report
src/users/dashboards.rs
[error] 163-165: Clippy lint: this map_or can be simplified. Use is_some_and instead of .map_or(false, |id| *id == dashboard_id) (clippy::unnecessary_map_or).
[error] 177-179: Clippy lint: this map_or can be simplified. Use is_some_and instead of .map_or(false, |id| *id == dashboard_id) (clippy::unnecessary_map_or).
[error] 194-196: Clippy lint: this map_or can be simplified. Use is_some_and instead of .map_or(false, |id| *id == dashboard_id) (clippy::unnecessary_map_or).
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (12)
src/handlers/http/users/dashboards.rs (7)
33-65: Dashboard listing correctly implements the sharing modelThe
listmethod follows the dashboard sharing model design where any user can read any dashboard. The method safely handles optional fields with appropriate null checks forauthor,modified, anddashboard_id, preventing potential runtime panics.
68-75: Get endpoint properly validates dashboard IDThe
getfunction correctly uses the newvalidate_dashboard_idhelper, improving error handling for invalid IDs. Following the sharing model, this endpoint allows any user to view any dashboard.
82-84: Good title validation in the POST endpointThe title validation check prevents creation of dashboards with empty titles, improving data quality and user experience.
102-108: Well-implemented tile ID validationThe function correctly validates that all tiles have a non-nil
tile_id, aligning with the requirement that tile IDs must be provided by the client and not generated by the server.
123-127: Delete endpoint properly delegates authorization checkThe delete function correctly validates the dashboard ID and delegates the deletion to
DASHBOARDS.delete_dashboard, which ensures that only the dashboard author can delete the dashboard.
169-182: Good addition of Unauthorized error typeAdding a dedicated
Unauthorizederror variant with appropriate HTTP status code (401) improves the API's error handling for authorization failures. This makes the authorization errors more descriptive and sets the appropriate status code.
129-155:✅ Verification successful
Well-implemented add_tile endpoint with proper authorization
The
add_tilefunction correctly:
- Validates the dashboard ID
- Checks that the tile ID is not nil (client must provide it)
- Uses
get_dashboard_by_userto ensure only the dashboard author can add tiles- Safely handles the optional tiles vector with
get_or_insert_withThis implementation aligns with the requirement that the function should use
get_dashboard_by_userinstead ofget_dashboardto ensure proper authorization.
🏁 Script executed:
#!/bin/bash # Verify the implementation of add_tile respects authorization and requires tile ID # Check if we're using get_dashboard_by_user for proper authorization rg -A 5 "get_dashboard_by_user" src/handlers/http/users/dashboards.rs # Check if we're validating tile ID is present rg "tile_id.is_nil" src/handlers/http/users/dashboards.rsLength of output: 417
add_tile endpoint correctly enforces authorization and validation
All required checks are present and correctly implemented:
- Dashboard ID is validated via
validate_dashboard_id- Tile ID presence is enforced with
tile.tile_id.is_nil()- Authorization is enforced by calling
DASHBOARDS.get_dashboard_by_user(dashboard_id, &user_id)- Existing tiles vector is safely handled with
get_or_insert_withNo further changes needed—approving as-is.
src/users/dashboards.rs (5)
42-47: Dashboard tile structure simplified appropriatelyThe new
Tilestruct is well-designed with a requiredtile_idfield and an optional map for additional fields, providing flexibility while maintaining the required identifier.
48-57: Dashboard structure streamlined with proper optional fieldsThe
Dashboardstruct has been appropriately streamlined:
- Required fields:
title- Optional fields:
version,author,dashboard_id,modified,dashboard_type,tilesThis design provides flexibility while maintaining the core requirements.
110-144: Good authorization check in update methodThe
updatemethod correctly verifies that the user is the author of the dashboard before allowing updates, usingget_dashboard_by_userfor authorization.
202-211: List dashboards implementation aligns with sharing modelThe
list_dashboardsmethod returns all dashboards without filtering by user, which aligns with the dashboard sharing model where any user can read any dashboard.
214-216: Good utility function for dashboard ID validationThe
validate_dashboard_idfunction provides a centralized way to parse and validate dashboard IDs, improving consistency and error handling throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/users/dashboards.rs (3)
208-212: Simplify map_or with is_none_orThe pipeline is suggesting to simplify this code with Rust's newer
is_none_ormethod.- self.0.write().await.retain(|d| { - d.dashboard_id - .as_ref() - .map_or(true, |id| *id != dashboard_id) - }); + self.0.write().await.retain(|d| { + d.dashboard_id + .as_ref() + .is_none_or(|id| *id != dashboard_id) + });🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 209-211: Clippy lint: this
map_orcan be simplified. Suggestion: useis_none_orinstead of.map_or(true, |id| *id != dashboard_id).
222-226: Simplify map_or with is_some_andThe pipeline is suggesting to simplify this code with Rust's newer
is_some_andmethod.- .find(|d| { - d.dashboard_id - .as_ref() - .map_or(false, |id| *id == dashboard_id) - }) + .find(|d| { + d.dashboard_id + .as_ref() + .is_some_and(|id| *id == dashboard_id) + })🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 223-225: Clippy lint: this
map_orcan be simplified. Suggestion: useis_some_andinstead of.map_or(false, |id| *id == dashboard_id).
239-244: Simplify map_or with is_some_andThe pipeline is suggesting to simplify this code with Rust's newer
is_some_andmethod.- .find(|d| { - d.dashboard_id - .as_ref() - .map_or(false, |id| *id == dashboard_id) - && d.author == Some(user_id.to_string()) - }) + .find(|d| { + d.dashboard_id + .as_ref() + .is_some_and(|id| *id == dashboard_id) + && d.author == Some(user_id.to_string()) + })🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 240-242: Clippy lint: this
map_orcan be simplified. Suggestion: useis_some_andinstead of.map_or(false, |id| *id == dashboard_id).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/modal/server.rs(1 hunks)src/handlers/http/users/dashboards.rs(3 hunks)src/users/dashboards.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handlers/http/users/dashboards.rs
- src/handlers/http/modal/server.rs
🧰 Additional context used
🧠 Learnings (1)
src/users/dashboards.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.832Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
🪛 GitHub Actions: Lint, Test and Coverage Report
src/users/dashboards.rs
[error] 209-211: Clippy lint: this map_or can be simplified. Suggestion: use is_none_or instead of .map_or(true, |id| *id != dashboard_id).
[error] 223-225: Clippy lint: this map_or can be simplified. Suggestion: use is_some_and instead of .map_or(false, |id| *id == dashboard_id).
[error] 240-242: Clippy lint: this map_or can be simplified. Suggestion: use is_some_and instead of .map_or(false, |id| *id == dashboard_id).
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (11)
src/users/dashboards.rs (11)
33-33: Version reset:CURRENT_DASHBOARD_VERSIONchanged from "v3" to "v1"The version constant has been reset from
"v3"to"v1", indicating a significant simplification of the dashboard system and removal of version migration logic. This aligns with the overall refactoring to flatten the dashboard data model.
35-40: NewDashboardTypeenum provides better categorizationAdding the
DashboardTypeenum withDashboardandReportvariants creates clearer semantics for different dashboard use cases.
42-47: SimplifiedTilestructure with ULID identifiersThe
Tilestruct has been greatly simplified to useUlidfor identifiers and a flattened map for additional fields. This addresses previous friction between ID types and provides more flexibility.
48-57: Dashboard struct now uses optional fieldsThe
Dashboardstruct has been refactored to use optional fields fordashboard_idandtiles. While this provides flexibility, it introduces potential null reference issues if not carefully handled.Make sure these optional fields are properly initialized and checked before use throughout the codebase.
59-66: Good addition ofset_metadatamethodThe
set_metadatamethod provides consistent initialization of dashboard metadata, addressing the previous concern about defensive initialization in the Dashboard struct.
101-104: ULID validation function provides better error handlingThe
validate_dashboard_idfunction centralizes the validation logic for dashboard IDs, making error handling more consistent throughout the codebase.
154-165: Create method now properly initializes metadataThe
createmethod now uses theset_metadatahelper to ensure consistent dashboard initialization before saving to storage.
167-179: Authorization check added to update methodThe
updatemethod now properly checks if the dashboard exists for the specified user usingget_dashboard_by_userand returnsDashboardError::Unauthorizedif not found. This addresses a previous security concern about missing authorization checks.
191-202: Authorization check added to delete_dashboard methodSimilar to the update method, the
delete_dashboardmethod now verifies user authorization before allowing deletion. This is a good security improvement that protects against unauthorized dashboard modifications.
248-250: List dashboards returns all dashboards without user filteringThe
list_dashboardsmethod now returns all dashboards without filtering by user. This simplifies the API but might expose dashboards across users if access control isn't implemented at the API layer.Consider adding an optional user filtering parameter if you need to maintain isolation between different users' dashboards.
230-246: Added get_dashboard_by_user method for authorization checksThe new
get_dashboard_by_usermethod properly checks both dashboard ID and user ID, providing the foundation for proper authorization throughout the codebase. This correctly implements the learning from previous PR feedback.🧰 Tools
🪛 GitHub Actions: Lint, Test and Coverage Report
[error] 240-242: Clippy lint: this
map_orcan be simplified. Suggestion: useis_some_andinstead of.map_or(false, |id| *id == dashboard_id).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/users/dashboards.rs (2)
191-202: Apply the same ownership check improvement todelete_dashboard.The
delete_dashboardmethod has the same authorization check pattern as theupdatemethod and should use the same centralized function.pub async fn delete_dashboard( &self, user_id: &str, dashboard_id: Ulid, ) -> Result<(), DashboardError> { - if self - .get_dashboard_by_user(dashboard_id, user_id) - .await - .is_none() - { - return Err(DashboardError::Unauthorized); - } + // Verify ownership + let _ = self.ensure_dashboard_ownership(dashboard_id, user_id).await?;
49-57: 🛠️ Refactor suggestionImplement constructor method for
Dashboardstruct.The
Dashboardstruct has several optional fields but doesn't provide a constructor method to ensure they're properly initialized, which could lead to the unwrap issues identified in previous reviews.#[derive(Debug, Serialize, Deserialize, Default, Clone)] pub struct Dashboard { pub version: Option<String>, pub title: String, pub author: Option<String>, pub dashboard_id: Option<Ulid>, pub modified: Option<DateTime<Utc>>, dashboard_type: Option<DashboardType>, pub tiles: Option<Vec<Tile>>, +} + +impl Dashboard { + pub fn new(title: String) -> Self { + Self { + version: Some(CURRENT_DASHBOARD_VERSION.to_string()), + title, + author: None, + dashboard_id: Some(Ulid::new()), + modified: Some(Utc::now()), + dashboard_type: Some(DashboardType::Dashboard), + tiles: Some(Vec::new()), + } + } }
🧹 Nitpick comments (8)
src/users/dashboards.rs (8)
35-40: Add documentation for theDashboardTypeenum.The enum is properly defined with a default value, but it lacks documentation explaining the difference between a "Dashboard" and a "Report" type.
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Default)] +/// Represents the type of dashboard visualization. pub enum DashboardType { + /// Standard interactive dashboard with multiple tiles. #[default] Dashboard, + /// Static report format, typically for scheduled delivery or exports. Report, }
43-47: Implement constructor method forTilestruct.The
Tilestruct has a requiredtile_idfield but no constructor method to ensure it's properly initialized.#[derive(Debug, Serialize, Deserialize, Default, Clone)] pub struct Tile { pub tile_id: Ulid, #[serde(flatten)] pub other_fields: Option<serde_json::Map<String, Value>>, +} + +impl Tile { + pub fn new(tile_id: Option<Ulid>) -> Self { + Self { + tile_id: tile_id.unwrap_or_else(Ulid::new), + other_fields: None, + } + } + + pub fn with_fields(mut self, fields: serde_json::Map<String, Value>) -> Self { + self.other_fields = Some(fields); + self + } }
101-104: Improve error message invalidate_dashboard_id.The error message "Dashboard ID must be provided" when
Ulid::from_stringfails is misleading, as it suggests the ID is missing rather than invalid.pub fn validate_dashboard_id(dashboard_id: String) -> Result<Ulid, DashboardError> { Ulid::from_string(&dashboard_id) - .map_err(|_| DashboardError::Metadata("Dashboard ID must be provided")) + .map_err(|_| DashboardError::Metadata("Invalid dashboard ID format - must be a valid ULID")) }
120-121: Handle malformed JSON with better error messages.The current implementation silently discards dashboards that fail to parse without logging the error or the offending dashboard.
if dashboard.is_empty() { continue; } -let dashboard_value = serde_json::from_slice::<serde_json::Value>(&dashboard)?; -if let Ok(dashboard) = serde_json::from_value::<Dashboard>(dashboard_value) { +let dashboard_value = match serde_json::from_slice::<serde_json::Value>(&dashboard) { + Ok(value) => value, + Err(err) => { + tracing::warn!("Failed to parse dashboard JSON: {}", err); + continue; + } +}; + +if let Ok(dashboard) = serde_json::from_value::<Dashboard>(dashboard_value.clone()) { this.retain(|d: &Dashboard| d.dashboard_id != dashboard.dashboard_id); this.push(dashboard); +} else { + tracing::warn!("Failed to deserialize dashboard: {:?}", dashboard_value); }
167-179: Use centralized authorization check inupdatemethod.The
updatemethod uses a specific conditional to check dashboard ownership. This authentication check should be extracted into a reusable function for consistency with other methods.+async fn ensure_dashboard_ownership( + &self, + dashboard_id: Ulid, + user_id: &str, +) -> Result<Dashboard, DashboardError> { + self.get_dashboard_by_user(dashboard_id, user_id) + .await + .ok_or(DashboardError::Unauthorized) +} + pub async fn update( &self, user_id: &str, dashboard_id: Ulid, dashboard: &mut Dashboard, ) -> Result<(), DashboardError> { - if self - .get_dashboard_by_user(dashboard_id, user_id) - .await - .is_none() - { - return Err(DashboardError::Unauthorized); - } + // Verify ownership + let _ = self.ensure_dashboard_ownership(dashboard_id, user_id).await?;
208-212: Simplify dashboard retention logic.The dashboard retention logic is awkwardly phrased with double negation. This could be simplified for better readability.
- self.0.write().await.retain(|d| { - d.dashboard_id - .as_ref() - .is_some_and(|id| *id == dashboard_id) - }); + self.0.write().await.retain(|d| { + !d.dashboard_id + .as_ref() + .is_some_and(|id| *id == dashboard_id) + });Or even better, for positive logic:
- self.0.write().await.retain(|d| { - d.dashboard_id - .as_ref() - .is_some_and(|id| *id == dashboard_id) - }); + self.0.write().await.retain(|d| { + d.dashboard_id + .as_ref() + .map_or(true, |id| *id != dashboard_id) + });
248-250: Consider adding pagination tolist_dashboards.The
list_dashboardsmethod simply returns a clone of all dashboards with no filtering or pagination, which could be inefficient for large numbers of dashboards.-pub async fn list_dashboards(&self) -> Vec<Dashboard> { - self.0.read().await.clone() +pub async fn list_dashboards( + &self, + offset: Option<usize>, + limit: Option<usize>, +) -> Vec<Dashboard> { + let dashboards = self.0.read().await; + let offset = offset.unwrap_or(0); + let limit = limit.unwrap_or(dashboards.len()); + + dashboards + .iter() + .skip(offset) + .take(limit) + .cloned() + .collect() }
230-246: Add missing functionality for adding tiles to a dashboard.Based on the retrieved learning, there should be an
add_tilefunction in the HTTP handler that usesget_dashboard_by_user. However, there's no corresponding method in theDashboardsstruct to support this operation.+pub async fn add_tile( + &self, + user_id: &str, + dashboard_id: Ulid, + tile: Tile, +) -> Result<(), DashboardError> { + let mut dashboard = self.ensure_dashboard_ownership(dashboard_id, user_id).await?; + + // Initialize tiles vector if it doesn't exist + if dashboard.tiles.is_none() { + dashboard.tiles = Some(Vec::new()); + } + + // Check for duplicate tile_id + if let Some(tiles) = &dashboard.tiles { + if tiles.iter().any(|t| t.tile_id == tile.tile_id) { + return Err(DashboardError::Metadata("Tile with this ID already exists")); + } + } + + // Add the new tile + if let Some(tiles) = &mut dashboard.tiles { + tiles.push(tile); + } + + // Update modification timestamp + dashboard.modified = Some(Utc::now()); + + // Save the updated dashboard + self.update(user_id, dashboard_id, &mut dashboard).await +}Would you like me to add this implementation to support the
add_tilefunctionality mentioned in the retrieved learning?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/users/dashboards.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/users/dashboards.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.832Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default 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
- GitHub Check: coverage
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (2)
src/users/dashboards.rs (2)
25-25: Good addition of theuliddependency for standardized IDs.Using ULIDs for dashboard and tile identifiers is a strong improvement over custom string hashes, as it provides:
- Global uniqueness guarantee
- Natural ordering for sorting
- Millisecond timestamp precision for easy chronological ordering
33-33: Confirm version reset from "v3" to "v1" is intentional.The dashboard version has been reset from a previous "v3" to "v1". This suggests a breaking change in the dashboard data model that removes backward compatibility with older dashboard structures.
Please verify that this change is intentional and that any existing dashboards will be properly migrated or handled.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/prism/home/mod.rs (1)
369-392:⚠️ Potential issueDashboard listing still ignores RBAC – leaks titles across tenants
This helper fetches all dashboards (
DASHBOARDS.list_dashboards()) and filters only by the query text, not by ownership or visibility.
The same issue was flagged in a previous review and remains unresolved; any authenticated user can enumerate every dashboard title/ID in the cluster.Please enforce authorisation, e.g. by filtering on
dashboard.author == key.user_idor moving the check into the data layer.
🧹 Nitpick comments (3)
src/handlers/http/prism_home.rs (1)
45-64: Avoid the extraserde_json::to_valuehop and clarify variable naming
generate_home_search_responsealready returns aHomeSearchResponsethat implementsSerialize, so you can hand it straight toweb::Json.
Serialising it into aserde_json::Valuefirst creates an unnecessary allocation and copies the whole structure once more.At the same time, the query parameter you expect is literally called
"key", while the local variablekeyholds the session key. The over-loaded wording is easy to mis-read in reviews and logs.- let res = generate_home_search_response(&key, &query_value).await?; - let json_res = serde_json::to_value(res) - .map_err(|err| PrismHomeError::Anyhow(anyhow::Error::msg(err.to_string())))?; - - Ok(web::Json(json_res)) + let res = generate_home_search_response(&key, &query_value).await?; + Ok(web::Json(res))If you want to keep the constant unchanged, consider renaming the local variable to
session_keyto avoid the mental clash.src/prism/home/mod.rs (2)
141-147: LargeHashMapcloned per-day – switch to a shared reference to save memory
stream_wise_meta.clone()is executed once for every day (7× by default).
When the map holds thousands of streams ‑ each withObjectStoreFormatblobs ‑ this multiplies memory pressure and CPU time unnecessarily.Passing
&stream_wise_meta(or wrapping the value in anArc/Rc) lets all async tasks read the same data without replication.
286-294: Minor allocation hotspot in dataset search
title.to_lowercase()allocates a freshStringfor every dataset on every search.
For a large number of streams, that’s avoidable churn.You can either
- call
unicode_segmentation::UnicodeSegmentation::graphemesetc. for case-insensitive checks without allocation, or- cache
title.to_ascii_lowercase()once per iteration:let lower_title = title.to_ascii_lowercase(); if lower_title.contains(query_value) { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/modal/server.rs(2 hunks)src/handlers/http/prism_home.rs(2 hunks)src/prism/home/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/modal/server.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/prism/home/mod.rs (1)
src/alerts/mod.rs (4)
get_alerts_info(982-1019)new(114-120)new(155-167)new(178-184)
| let mut dates = (0..7) | ||
| .map(|i| { | ||
| Utc::now() | ||
| .checked_sub_signed(chrono::Duration::days(i)) | ||
| .ok_or_else(|| anyhow::Error::msg("Date conversion failed")) | ||
| .unwrap() | ||
| .expect("Date conversion failed") | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Production code should not expect() – bubble the error instead
checked_sub_signed can fail on very large offsets or if the system clock is skewed.
Calling .expect("Date conversion failed") will panic the whole HTTP worker thread and drop in-flight requests.
- Utc::now()
- .checked_sub_signed(chrono::Duration::days(i))
- .expect("Date conversion failed")
+ Utc::now()
+ .checked_sub_signed(chrono::Duration::days(i))
+ .ok_or_else(|| PrismHomeError::Anyhow(anyhow::anyhow!("Failed to compute date")) )?Returning a proper PrismHomeError keeps the service alive and surfaces the root cause to the caller.
📝 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.
| let mut dates = (0..7) | |
| .map(|i| { | |
| Utc::now() | |
| .checked_sub_signed(chrono::Duration::days(i)) | |
| .ok_or_else(|| anyhow::Error::msg("Date conversion failed")) | |
| .unwrap() | |
| .expect("Date conversion failed") | |
| }) | |
| let mut dates = (0..7) | |
| .map(|i| { | |
| Utc::now() | |
| .checked_sub_signed(chrono::Duration::days(i)) | |
| .ok_or_else(|| PrismHomeError::Anyhow(anyhow::anyhow!("Failed to compute date")) )? | |
| }) |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes