-
-
Notifications
You must be signed in to change notification settings - Fork 153
add admin authorizations to dashboards and filters #1447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add admin authorizations to dashboards and filters #1447
Conversation
allow admins to get, update and delete dashboards and filters
WalkthroughThe changes introduce admin-aware authorization across handlers and data layers. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Handler Layer
participant Utils as Utils (is_admin)
participant Data as Data Layer
participant DB as Database
Client->>Handler: Request (GET/UPDATE/DELETE)
Handler->>Utils: is_admin(req)
Utils->>DB: extract session -> fetch permissions
DB-->>Utils: permissions
Utils-->>Handler: Result<bool>
alt is_admin OK
Handler->>Data: call method with is_admin
Data->>DB: verify ownership OR allow if is_admin
DB-->>Data: item / not found / error
Data-->>Handler: result
Handler-->>Client: response
else is_admin Err
Handler->>Handler: map error to Custom error
Handler-->>Client: error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/mod.rs (1)
159-159: Avoid unnecessary String allocation in permission check.The current check allocates a new String for comparison. Consider using a string literal instead.
Apply this diff:
- Ok(permissions.contains(&String::from("admin"))) + Ok(permissions.contains(&"admin".to_string()) || permissions.iter().any(|p| p == "admin"))Or even simpler, if the collection supports it:
- Ok(permissions.contains(&String::from("admin"))) + Ok(permissions.iter().any(|p| p == "admin"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/http/users/dashboards.rs(4 hunks)src/handlers/http/users/filters.rs(4 hunks)src/users/dashboards.rs(4 hunks)src/users/filters.rs(1 hunks)src/utils/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-01T10:27:56.858Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
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.
Applied to files:
src/users/dashboards.rssrc/handlers/http/users/dashboards.rs
🧬 Code graph analysis (5)
src/utils/mod.rs (3)
src/utils/actix.rs (2)
req(31-31)extract_session_key_from_req(51-71)src/rbac/user.rs (3)
userid(80-85)userid(239-241)from(201-211)src/rbac/map.rs (2)
from(294-302)from(346-354)
src/users/filters.rs (1)
src/utils/mod.rs (1)
is_admin(149-160)
src/handlers/http/users/filters.rs (2)
src/utils/actix.rs (2)
extract_session_key_from_req(51-71)req(31-31)src/utils/mod.rs (3)
get_hash(70-75)get_user_from_request(60-68)is_admin(149-160)
src/users/dashboards.rs (1)
src/utils/mod.rs (1)
is_admin(149-160)
src/handlers/http/users/dashboards.rs (2)
src/utils/mod.rs (3)
get_hash(70-75)get_user_from_request(60-68)is_admin(149-160)src/users/dashboards.rs (1)
validate_dashboard_id(167-170)
⏰ 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: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
🔇 Additional comments (10)
src/handlers/http/users/filters.rs (3)
43-60: Admin authorization integrated correctly.The handler now properly computes admin status and passes it to
get_filter, allowing admins to access any filter while maintaining owner-based access for non-admin users. The error handling is appropriate.
79-106: Admin bypass correctly implemented for filter updates.The update handler properly checks admin status before authorizing filter modifications. The implementation allows admins to update any filter while preserving ownership checks for regular users.
108-127: Deletion authorization enhanced with admin support.The delete handler correctly incorporates admin checking, allowing administrators to remove any filter while maintaining the existing authorization model for regular users.
src/users/filters.rs (1)
135-150: Admin-aware filter retrieval implemented correctly.The updated
get_filter()method signature and logic properly incorporate admin authorization. The conditionf.user_id == Some(user_id.to_string()) || is_admincorrectly allows admins to bypass ownership checks while maintaining user-based filtering for regular users.src/handlers/http/users/dashboards.rs (3)
100-187: Admin authorization correctly integrated into dashboard updates.The
update_dashboardhandler now computes admin status and passes it toget_dashboard_by_user, properly allowing administrators to update any dashboard. The error handling maintains clear messaging for authorization failures.
189-203: Dashboard deletion now supports admin authorization.The delete handler correctly computes admin status and delegates authorization to the data layer via
delete_dashboard. This maintains the separation of concerns while enabling admin access.
205-236: Admin authorization added while maintaining proper authorization checks.The
add_tilehandler now computes admin status and correctly usesget_dashboard_by_userwith theis_adminflag for authorization. This aligns with previous guidance to useget_dashboard_by_userinstead ofget_dashboardfor proper authorization checks when modifying dashboards.Based on learnings.
src/users/dashboards.rs (3)
294-316: Dashboard deletion method properly updated with admin support.The
delete_dashboardmethod signature correctly accepts theis_adminparameter and threads it through toensure_dashboard_ownership, maintaining the authorization model while enabling admin access.
335-352: Admin-aware dashboard retrieval correctly implemented.The
get_dashboard_by_usermethod now acceptsis_adminand correctly implements the bypass logic:d.author == Some(user_id.to_string()) || is_admin. This allows administrators to access any dashboard while maintaining owner-based access for regular users, consistent with the filter implementation.
410-423: Ownership verification updated to respect admin privileges.The
ensure_dashboard_ownershiphelper correctly delegates toget_dashboard_by_userwith theis_adminflag, maintaining its role as an authorization validator while incorporating admin bypass logic.
allow admins to get, update and delete dashboards and filters
Summary by CodeRabbit