-
-
Notifications
You must be signed in to change notification settings - Fork 145
feat: Introduce User Groups to Parseable #1366
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis update introduces user group support to the RBAC system, enabling users to inherit roles through group memberships. Role assignment endpoints are refactored to separate add/remove operations, and metadata migration is extended to support user groups. Permission checks, API responses, and storage structures are updated to reflect group-based role inheritance and improved validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant RBAC
participant Metadata
participant UserGroups
Client->>API: PATCH /user/{username}/role/add (roles)
API->>RBAC: add_roles_to_user(username, roles)
RBAC->>Metadata: Update user roles in metadata
RBAC->>UserGroups: Update user group memberships if needed
RBAC-->>API: Success/Error
API-->>Client: Response
Client->>API: PATCH /user/{username}/role/remove (roles)
API->>RBAC: remove_roles_from_user(username, roles)
RBAC->>Metadata: Update user roles in metadata
RBAC->>UserGroups: Update user group memberships if needed
RBAC-->>API: Success/Error
API-->>Client: Response
Client->>API: GET /user/{username}/role
API->>RBAC: get_role(username)
RBAC->>UserGroups: Fetch group roles for user
RBAC-->>API: RolesResponse (direct + group roles)
API-->>Client: RolesResponse
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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)
✨ Finishing Touches
🪧 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 (
|
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: 8
🔭 Outside diff range comments (1)
src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
109-137
: Add role existence validation for consistency.The
add_roles_to_user
handler is missing role existence validation that's present in the main RBAC handlers (src/handlers/http/rbac.rs
lines 321-328). This could allow non-existent roles to be added to users.Add role validation before processing:
pub async fn add_roles_to_user( username: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let username = username.into_inner(); let roles_to_add = roles_to_add.into_inner(); if !Users.contains(&username) { return Err(RBACError::UserDoesNotExist); }; + + // check if all roles exist + let mut non_existent_roles = Vec::new(); + roles_to_add.iter().for_each(|r| { + if roles().get(r).is_none() { + non_existent_roles.push(r.clone()); + } + }); + + if !non_existent_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existent_roles)); + } // update parseable.json first
🧹 Nitpick comments (6)
src/migration/mod.rs (1)
107-107
: Fix comment capitalization for consistency.- //remove querier endpoint and token from storage metadata + // Remove querier endpoint and token from storage metadatasrc/rbac/mod.rs (1)
119-137
: Correctly implements group-based permission inheritance.The method properly aggregates permissions from both direct roles and roles inherited through user groups.
Consider extracting the group permission aggregation logic into a helper method for better readability:
fn get_group_permissions(&self, username: &str) -> Vec<Permission> { let user_groups = self.get_user_groups(username); let mut permissions = Vec::new(); for group in user_groups { if let Some(group) = read_user_groups().get(&group) { for role in &group.roles { if let Some(privilege_list) = roles().get(role) { for privilege in privilege_list { permissions.extend(RoleBuilder::from(privilege).build()); } } } } } permissions }src/handlers/http/modal/query/querier_rbac.rs (2)
113-120
: Simplify role validation logic.The
map().for_each(drop)
pattern is unnecessarily complex for this use case.- user_roles - .iter() - .map(|r| { - if !roles().contains_key(r) { - non_existant_roles.push(r.clone()); - } - }) - .for_each(drop); + for r in user_roles.iter() { + if !roles().contains_key(r) { + non_existant_roles.push(r.clone()); + } + }
181-192
: Simplify user group removal logic.The
map().for_each(drop)
pattern is unnecessarily complex and reduces readability.- [&username] - .iter() - .map(|user| { - if let Some(user) = mut_users().get_mut(*user) { - for group in groups_to_update.iter() { - user.user_groups.remove(&group.name); - } - - metadata.users.retain(|u| u.username() != user.username()); - metadata.users.push(user.clone()); - } - }) - .for_each(drop); + if let Some(user) = mut_users().get_mut(&username) { + for group in groups_to_update.iter() { + user.user_groups.remove(&group.name); + } + + metadata.users.retain(|u| u.username() != user.username()); + metadata.users.push(user.clone()); + }src/handlers/http/rbac.rs (2)
119-126
: Simplify role validation logic.Use a standard for loop instead of the complex
map().for_each(drop)
pattern for better readability.- user_roles - .iter() - .map(|r| { - if !roles().contains_key(r) { - non_existant_roles.push(r.clone()); - } - }) - .for_each(drop); + for r in user_roles.iter() { + if !roles().contains_key(r) { + non_existant_roles.push(r.clone()); + } + }
254-266
: Simplify user metadata update logic.The
map().for_each(drop)
pattern is unnecessarily complex for updating a single user.- [&username] - .iter() - .map(|user| { - if let Some(user) = mut_users().get_mut(*user) { - for group in groups_to_update.iter() { - user.user_groups.remove(&group.name); - } - - metadata.users.retain(|u| u.username() != user.username()); - metadata.users.push(user.clone()); - } - }) - .for_each(drop); + if let Some(user) = mut_users().get_mut(&username) { + for group in groups_to_update.iter() { + user.user_groups.remove(&group.name); + } + + metadata.users.retain(|u| u.username() != user.username()); + metadata.users.push(user.clone()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/handlers/http/cluster/mod.rs
(1 hunks)src/handlers/http/modal/ingest/ingestor_rbac.rs
(3 hunks)src/handlers/http/modal/ingest_server.rs
(1 hunks)src/handlers/http/modal/query/querier_rbac.rs
(5 hunks)src/handlers/http/modal/query_server.rs
(2 hunks)src/handlers/http/modal/server.rs
(2 hunks)src/handlers/http/oidc.rs
(1 hunks)src/handlers/http/rbac.rs
(9 hunks)src/handlers/http/role.rs
(3 hunks)src/migration/metadata_migration.rs
(1 hunks)src/migration/mod.rs
(4 hunks)src/rbac/map.rs
(6 hunks)src/rbac/mod.rs
(6 hunks)src/rbac/role.rs
(16 hunks)src/rbac/user.rs
(6 hunks)src/rbac/utils.rs
(3 hunks)src/storage/store_metadata.rs
(5 hunks)src/utils/mod.rs
(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
src/utils/mod.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
src/handlers/http/cluster/mod.rs (2)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/handlers/http/health_check.rs:81-90
Timestamp: 2025-06-16T02:04:58.990Z
Learning: In the shutdown function in src/handlers/http/health_check.rs, the design approach is to log errors from sync operations rather than propagate them. This is intentional because the shutdown function is called on SIGTERM/SIGINT signals, and the goal is to perform best-effort cleanup (syncing pending files to object storage) while allowing the shutdown to proceed regardless of sync failures. Logging provides debugging information without blocking the shutdown process.
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.
src/handlers/http/modal/query_server.rs (3)
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.
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#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.
src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
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.
src/rbac/map.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
src/handlers/http/modal/query/querier_rbac.rs (1)
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.
src/handlers/http/rbac.rs (1)
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.
🧬 Code Graph Analysis (3)
src/utils/mod.rs (1)
src/handlers/http/query.rs (1)
query
(121-171)
src/handlers/http/modal/ingest/ingestor_rbac.rs (4)
src/handlers/http/rbac.rs (3)
Users
(72-72)add_roles_to_user
(314-356)remove_roles_from_user
(359-413)src/rbac/user.rs (1)
username
(80-88)src/storage/store_metadata.rs (1)
put_staging_metadata
(286-301)src/handlers/http/oidc.rs (1)
get_metadata
(368-376)
src/rbac/mod.rs (3)
src/rbac/map.rs (8)
mut_sessions
(97-103)mut_users
(65-71)read_user_groups
(41-47)roles
(73-79)sessions
(89-95)users
(57-63)from
(299-307)from
(322-330)src/rbac/user.rs (5)
roles
(94-96)username
(80-88)add_roles
(267-279)remove_roles
(300-310)from
(188-197)src/rbac/role.rs (1)
from
(220-243)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (27)
src/handlers/http/oidc.rs (1)
336-340
: LGTM! Correct pattern matching update for user groups.The destructuring pattern correctly includes the new
user_groups
field with an ignore pattern, ensuring the match remains exhaustive while not affecting the existing logic.src/utils/mod.rs (1)
96-96
: LGTM! Correct extraction of tables from visitor.The
.into_inner()
call correctly extracts the tables vector from theTableScanVisitor
, matching the expected type foruser_auth_for_datasets
.src/migration/metadata_migration.rs (1)
164-185
: LGTM! Well-structured migration for user groups.The migration correctly:
- Updates version from v5 to v6
- Adds top-level
user_groups
field if missing- Adds
user_groups
field to each user object if missing- Uses appropriate default values (empty arrays)
The defensive checks ensure idempotency and prevent errors.
src/storage/store_metadata.rs (5)
35-35
: LGTM! Correct import for user groups support.Adding
UserGroup
import is necessary for the newuser_groups
field in theStorageMetadata
struct.
45-45
: LGTM! Version bump aligns with migration.Updating to "v6" is consistent with the new
v5_v6
migration function that adds user groups support.
63-63
: LGTM! User groups field addition is correct.The new
user_groups
field follows the same pattern as the existingusers
field and supports the user groups feature.
82-82
: LGTM! Consistent default initialization.Initializing
user_groups
to an emptyVec
is consistent with how other collection fields are handled in theDefault
implementation.
94-94
: Nice typo fix!Correcting "gloabal" to "global" improves code quality.
src/handlers/http/modal/ingest_server.rs (1)
201-219
: LGTM! API endpoint refactoring aligns with PR objectives.The splitting of role management into separate add and remove endpoints is well-implemented:
- Appropriate use of PATCH method for partial updates
- Consistent authorization (
Action::PutUserRoles
) and middleware (DisAllowRootUser
)- Clear, descriptive handler function names
- Proper route structure following RESTful conventions
This provides more granular control over role operations compared to the previous monolithic PUT endpoint.
src/handlers/http/role.rs (1)
91-104
: Fix logic error in role usage validation.The current logic checks if any group contains the role being deleted and returns
RoleInUse
error, but then continues to collect group names regardless. This creates inconsistent behavior where the function should either fail early or continue processing.Apply this diff to fix the logic:
- // also delete from user groups - let groups = read_user_groups().keys().cloned().collect_vec(); - let mut group_names = Vec::new(); - - for user_group in groups { - if let Some(ug) = read_user_groups().get(&user_group) { - if ug.roles.contains(&name) { - return Err(RoleError::RoleInUse); - } - group_names.push(ug.name.clone()); - } else { - continue; - }; - } + // check if any user group is using this role + let groups = read_user_groups(); + for group in groups.values() { + if group.roles.contains(&name) { + return Err(RoleError::RoleInUse); + } + }Likely an incorrect or invalid review comment.
src/handlers/http/cluster/mod.rs (1)
163-186
: LGTM: Clean implementation of operation-specific role synchronization.The addition of the
operation
parameter with proper validation and URL path construction correctly supports the new split role synchronization endpoints. The error handling follows the existing pattern and the validation ensures only valid operations are processed.src/handlers/http/modal/query_server.rs (2)
97-98
: LGTM: Correct mutation for metadata migration.Making
parseable_json
mutable before passing to the migration function allows for in-place updates, which aligns with the updated migration logic that adds user group support.
211-236
: LGTM: Well-structured role endpoint refactoring.The refactoring from a single PUT endpoint to separate GET/PATCH add/remove endpoints follows REST principles and provides better granular control over role operations. The authorization configuration is appropriate for each endpoint type.
src/rbac/utils.rs (1)
41-78
: LGTM: Comprehensive user group role integration.The enhancement to include user group roles in the
UsersPrism
response is well-implemented. The code properly:
- Collects roles from user groups the user belongs to
- Handles missing group references gracefully
- Organizes data into logical structures (direct roles vs group roles)
- Maintains consistency with existing role collection patterns
src/migration/mod.rs (2)
46-49
: LGTM!The parameter change to mutable reference enables in-place metadata updates, allowing the migration results to be propagated back to the caller.
75-79
: Well-structured migration flow.The consistent addition of the v5_v6 migration step across all version paths and the pattern of serializing results back to the mutable reference ensures proper metadata updates.
Also applies to: 87-91, 98-102, 110-115, 118-122
src/handlers/http/modal/server.rs (2)
117-118
: LGTM!The change correctly propagates the mutable reference requirement from the migration module.
611-636
: Role endpoints updated; old PUT handler is commented out
Verified that the previousput_role
handler is no longer active and has been commented out in:
- src/handlers/http/rbac.rs
- src/handlers/http/modal/query/querier_rbac.rs
- src/handlers/http/modal/ingest/ingestor_rbac.rs
No further changes are required.
src/rbac/user.rs (3)
28-35
: LGTM!The User struct is properly extended with the
user_groups
field, and all constructors correctly initialize it as an empty set.Also applies to: 48-50, 63-64, 76-77, 160-161
200-262
: Well-designed UserGroup struct with comprehensive validation.The validation method properly checks for:
- Valid group name format using regex
- Existence of the group (preventing duplicates)
- Existence of all referenced roles and users
- Provides detailed error information
300-331
: LGTM!The remove methods correctly use set operations and optimize for no-op cases. The
update_in_metadata
method properly replaces the group entry.src/rbac/mod.rs (3)
32-32
: LGTM!The
get_user_groups
method correctly retrieves user group memberships with proper error handling for missing users.Also applies to: 57-63
100-113
: Clean separation of role operations.The split into
add_roles
andremove_roles
provides clearer semantics than a single update method. Session invalidation correctly ensures permission changes take immediate effect.
207-224
: LGTM!The
UsersPrism
struct comprehensively represents users with:
- Direct roles
- Group-inherited roles
- Group memberships
The camelCase serialization maintains API consistency.
src/rbac/map.rs (2)
39-55
: LGTM!The
USER_GROUPS
map follows the established pattern for global state management with proper initialization and thread-safe accessors.Also applies to: 111-111, 144-146
310-331
: LGTM!The
UserGroups
struct implementation is clean and follows the established pattern from theUsers
struct.src/rbac/role.rs (1)
23-26
: Well-structured RBAC model extensions.The additions properly extend the RBAC system to support:
- User group management actions
- Resource-scoped permissions with optional resource type and ID
- Consistent builder pattern implementation
The integration with LLM actions and the DefaultPrivilege enum is clean and follows existing patterns.
Also applies to: 85-85, 95-96, 111-115, 167-175, 213-216
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: 2
♻️ Duplicate comments (1)
src/rbac/map.rs (1)
228-254
: Address potential panic and improve readability.The permission aggregation logic has issues that need attention:
- Potential panic: Line 234 uses
unwrap()
which could panic if the user group is not found- Redundant lookups: The user group is looked up multiple times
- Complex nesting: The logic is hard to follow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/modal/query/querier_rbac.rs
(5 hunks)src/handlers/http/rbac.rs
(9 hunks)src/rbac/map.rs
(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
src/handlers/http/modal/query/querier_rbac.rs (1)
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.
src/rbac/map.rs (2)
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.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
src/handlers/http/rbac.rs (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-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: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (11)
src/rbac/map.rs (3)
39-55
: LGTM: User groups integration follows established patterns.The new
USER_GROUPS
static and accessor functions follow the same safe pattern used for other RBAC maps in this module. The initialization and read/write guards are consistent with existing code.
111-111
: LGTM: Proper initialization of user groups from metadata.The integration of user groups initialization in the
init
function is well-placed and follows the same pattern as other RBAC components.Also applies to: 144-146
310-331
: LGTM: Clean UserGroups implementation.The
UserGroups
struct implementation follows the same pattern as the existingUsers
struct, providing consistent API design across the RBAC module.src/handlers/http/modal/query/querier_rbac.rs (3)
62-75
: LGTM: Improved role validation prevents invalid user creation.The addition of role existence validation before user creation is a valuable improvement that prevents creating users with non-existent roles. The error handling appropriately returns a list of invalid roles.
96-100
: LGTM: Consistent with new role management API.The switch from
put_role
toadd_roles_to_user
aligns with the new granular role management approach and maintains consistency across the codebase.
160-205
: LGTM: Well-implemented granular role management.Both
add_roles_to_user
andremove_roles_from_user
handlers include comprehensive validation:
- Role existence validation
- User existence validation
- Role assignment validation (for removal)
- Proper metadata synchronization with ingestors
The separation of add/remove operations provides better granular control and clearer API semantics.
Also applies to: 207-264
src/handlers/http/rbac.rs (5)
117-130
: LGTM: Consistent role validation across handlers.The role existence validation in
post_user
matches the implementation in the querier module, ensuring consistent behavior across the RBAC API.
195-226
: Excellent enhancement: Comprehensive role information.The enhanced
get_role
handler now provides both direct user roles and inherited group roles in a well-structuredRolesResponse
. This gives clients complete visibility into a user's effective permissions, which is crucial for RBAC systems.
383-444
: LGTM: Comprehensive error handling for user groups.The new error variants and
InvalidUserGroupError
struct provide detailed error information that will help clients understand and correct validation issues. The structured approach to error reporting is well-designed.
467-489
: LGTM: Enhanced error responses with structured JSON.The improved error response handling returns structured JSON for role-related errors, making the API more client-friendly and easier to parse programmatically.
491-497
: LGTM: Well-designed response structure.The
RolesResponse
struct clearly separates direct roles from group roles while maintaining the role-to-privileges mapping, providing comprehensive role information in a logical structure.
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
♻️ Duplicate comments (2)
src/rbac/map.rs (1)
228-255
: Address potential panic and improve readability.The permission aggregation logic has issues that need attention as previously identified.
src/handlers/http/rbac.rs (1)
240-257
: Similar complexity issue as in querier module.The user group cleanup logic here has the same complexity issues as identified in the querier module.
🧹 Nitpick comments (4)
src/rbac/role.rs (2)
93-94
: Remove commented-out code.These commented lines represent the old implementation and should be removed to maintain code clarity:
- Lines 93-94: Old Permission variants
- Lines 103-105: Old RoleBuilder fields
- Lines 121-135: Old RoleBuilder methods
pub enum Permission { Unit(Action), - // Stream(Action, String), - // StreamWithTag(Action, String, Option<String>), Resource(Action, ParseableResourceType), SelfUser, } pub struct RoleBuilder { actions: Vec<Action>, - // stream: Option<String>, - // tag: Option<String>, - // resource_id: Option<String>, resource_type: Option<ParseableResourceType>, } - // pub fn with_stream(mut self, stream: String) -> Self { - // self.stream = Some(stream); - // self - // } - - // pub fn with_tag(mut self, tag: String) -> Self { - // self.tag = Some(tag); - // self - // } - - // pub fn with_resource(mut self, resource_id: String, resource_type: ParseableResourceType) -> Self { - // self.resource_id = Some(resource_id); - // self.resource_type = Some(resource_type); - // self - // }Also applies to: 103-105, 121-135
258-261
: Remove more commented-out code.Additional commented lines from the old implementation should be removed:
RoleBuilder { actions: vec![Action::All], - // stream: Some("*".to_string()), - // tag: None, resource_type: Some(ParseableResourceType::All), - // resource_id: Some("*".to_string()), } ], - // stream: Some("*".to_string()), - // tag: None, - // resource_id: Some("*".to_string()), resource_type: Some(ParseableResourceType::All), ], - // stream: None, - // tag: None, - // resource_id: None, resource_type: None, ], - // stream: None, - // tag: None, - // resource_id: None, resource_type: None, actions: vec![Action::Ingest], - // stream: None, - // tag: None, - // resource_id: None, resource_type: None,Also applies to: 307-310, 350-353, 386-389, 396-399
src/handlers/http/rbac.rs (2)
119-127
: Use more idiomatic code for role validation.The current implementation uses
.map()
with.for_each(drop)
which is unconventional. Consider a more idiomatic approach:- let mut non_existant_roles = Vec::new(); - user_roles - .iter() - .map(|r| { - if !roles().contains_key(r) { - non_existant_roles.push(r.clone()); - } - }) - .for_each(drop); + let non_existant_roles: Vec<String> = user_roles + .iter() + .filter(|r| !roles().contains_key(*r)) + .cloned() + .collect();
277-288
: Extract role validation to reduce duplication.The role existence validation is duplicated between
add_roles_to_user
andremove_roles_from_user
. Consider extracting to a helper function:fn validate_roles_exist(roles: &HashSet<String>) -> Result<(), RBACError> { let non_existant_roles: Vec<String> = roles .iter() .filter(|r| roles().get(*r).is_none()) .cloned() .collect(); if !non_existant_roles.is_empty() { return Err(RBACError::RolesDoNotExist(non_existant_roles)); } Ok(()) }Then use it in both handlers:
validate_roles_exist(&roles_to_add)?;Also applies to: 322-333
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/handlers/http/modal/query/querier_rbac.rs
(5 hunks)src/handlers/http/rbac.rs
(9 hunks)src/migration/mod.rs
(2 hunks)src/rbac/map.rs
(6 hunks)src/rbac/role.rs
(10 hunks)src/utils/mod.rs
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/mod.rs
- src/migration/mod.rs
- src/handlers/http/modal/query/querier_rbac.rs
🧰 Additional context used
🧠 Learnings (2)
src/rbac/map.rs (2)
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.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
src/handlers/http/rbac.rs (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (5)
src/rbac/map.rs (3)
40-56
: LGTM!The USER_GROUPS static and accessor functions follow the established patterns in the codebase.
268-269
: Clarify the permission logic when context_resource is None.The comment "WHEN IS THIS VALID??" indicates uncertainty about this code path. When
context_resource
is None, the code grants permission regardless of the resource_id. This could be a security concern if not intentional.Please verify:
- Under what conditions is
context_resource
expected to be None?- Is it correct to grant permission when no resource context is provided?
- Should this case log a warning or require explicit handling?
319-340
: LGTM!The UserGroups struct implementation follows the established pattern used for Users.
src/handlers/http/rbac.rs (2)
195-225
: LGTM!The implementation correctly returns both direct roles and roles inherited from user groups, providing a comprehensive view of user permissions.
452-471
: LGTM!The structured JSON error responses provide clear feedback to API consumers, improving the developer experience.
376dbda
to
1838f89
Compare
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: 0
♻️ Duplicate comments (1)
src/handlers/http/rbac.rs (1)
240-257
: Complexity issue persists from previous reviews.The user group cleanup logic still has the same complexity issues identified in previous reviews. The nested loops, multiple
retain
operations, and repeated metadata updates make this code hard to follow and maintain.
🧹 Nitpick comments (3)
src/handlers/http/rbac.rs (3)
115-130
: Simplify role validation logic for better readability.The current approach using
map
+for_each(drop)
is unconventional. Consider a more direct approach:- } else { - let mut non_existant_roles = Vec::new(); - user_roles - .iter() - .map(|r| { - if !roles().contains_key(r) { - non_existant_roles.push(r.clone()); - } - }) - .for_each(drop); - if !non_existant_roles.is_empty() { - return Err(RBACError::RolesDoNotExist(non_existant_roles)); - } + } else { + let non_existant_roles: Vec<String> = user_roles + .iter() + .filter(|r| !roles().contains_key(*r)) + .cloned() + .collect(); + if !non_existant_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existant_roles)); + } }
265-308
: Add roles handler is well-implemented.The handler correctly validates role existence and updates both persistent and in-memory state atomically.
Consider extracting the role validation logic to a helper function to reduce duplication:
fn validate_roles_exist(roles: &HashSet<String>) -> Result<(), RBACError> { let non_existant_roles: Vec<String> = roles .iter() .filter(|r| !roles().contains_key(*r)) .cloned() .collect(); if !non_existant_roles.is_empty() { return Err(RBACError::RolesDoNotExist(non_existant_roles)); } Ok(()) }
367-430
: Enhanced error types provide good validation feedback.The new error variants and InvalidUserGroupError struct provide comprehensive validation information for user group operations.
Consider removing the commented-out Display implementation or implementing it if needed:
-// impl Display for InvalidUserGroupRequestStruct { -// fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { -// if !self.invalid_name { -// write!( -// f, -// "Invalid user group request- {{invalidName: {}\nnonExistantRoles: {:?}\nnonExistantUsers: {:?}\nThe name should follow this regex- ^[A-Za-z0-9_-]+$}}", -// self.invalid_name, self.non_existant_roles, self.non_existant_users -// ) -// } else { -// write!( -// f, -// "Invalid user group request- {{nonExistantRoles: {:?}\nnonExistantUsers: {:?}}}", -// self.non_existant_roles, self.non_existant_users -// ) -// } -// } -// }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/handlers/http/middleware.rs
(3 hunks)src/handlers/http/modal/ingest_server.rs
(3 hunks)src/handlers/http/modal/query_server.rs
(6 hunks)src/handlers/http/modal/server.rs
(13 hunks)src/handlers/http/rbac.rs
(9 hunks)src/rbac/map.rs
(6 hunks)src/rbac/role.rs
(11 hunks)src/rbac/user.rs
(6 hunks)src/utils/mod.rs
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/utils/mod.rs
- src/handlers/http/modal/query_server.rs
- src/handlers/http/modal/ingest_server.rs
- src/handlers/http/modal/server.rs
- src/rbac/user.rs
- src/rbac/map.rs
- src/handlers/http/middleware.rs
- src/rbac/role.rs
🧰 Additional context used
🧠 Learnings (1)
src/handlers/http/rbac.rs (1)
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.
⏰ 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 Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-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
🔇 Additional comments (4)
src/handlers/http/rbac.rs (4)
19-44
: Import changes look appropriate for user group functionality.The new imports correctly support the enhanced RBAC system with user groups and improved role management.
195-226
: Role retrieval logic correctly handles group inheritance.The implementation properly fetches both direct user roles and roles inherited from user groups. The nested structure in the response provides clear visibility into role sources.
310-365
: Remove roles handler correctly validates role assignment.The handler properly validates both role existence and current assignment before removal. The additional validation prevents attempts to remove roles that aren't assigned to the user.
432-485
: Enhanced error handling provides better API responses.The structured JSON error responses and the RolesResponse struct provide clear, actionable feedback to API clients. The separation of direct roles from group roles in the response is well-designed.
- Added support for user groups - Migrated `PUT /user/{username}/role` to `PATCH /user/{username}/role/add` and `PATCH /user/{username}/role/remove`
- Introduce resource types (stream, llm, all) for privileges
- roles don't need any migration - auth flow modified to account for resource type
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: 2
🔭 Outside diff range comments (1)
src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
77-88
: Add role existence validation before adding roles.The
add_roles_to_user
handler is missing validation to check if all roles exist before adding them, unlike theremove_roles_from_user
handler which has comprehensive validation.Add validation similar to the remove handler:
pub async fn add_roles_to_user( username: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let username = username.into_inner(); let roles_to_add = roles_to_add.into_inner(); if !Users.contains(&username) { return Err(RBACError::UserDoesNotExist); }; + + // check if all roles exist + let mut non_existent_roles = Vec::new(); + roles_to_add.iter().for_each(|r| { + if roles().get(r).is_none() { + non_existent_roles.push(r.clone()); + } + }); + + if !non_existent_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existent_roles)); + } + // update parseable.json first
♻️ Duplicate comments (2)
src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
109-141
: Comprehensive validation implementation looks good.The validation logic correctly checks for user existence, role existence, and that the user actually has the roles before removal. This prevents edge cases and ensures data consistency.
src/handlers/http/rbac.rs (1)
240-258
: Simplify user group cleanup logic.The user group cleanup has high complexity with nested loops and multiple metadata updates.
Consider collecting all changes first:
- // also delete from user groups - let user_groups = Users.get_user_groups(&username); - let mut groups_to_update = Vec::new(); - for user_group in user_groups { - if let Some(ug) = write_user_groups().get_mut(&user_group) { - ug.remove_users(HashSet::from_iter([username.clone()]))?; - groups_to_update.push(ug.clone()); - // ug.update_in_metadata().await?; - } else { - continue; - }; - } - - // update in metadata user groups - metadata - .user_groups - .retain(|x| !groups_to_update.contains(x)); - metadata.user_groups.extend(groups_to_update); + // Remove user from all groups + let user_groups = Users.get_user_groups(&username); + { + let mut groups = write_user_groups(); + for group_name in &user_groups { + if let Some(group) = groups.get_mut(group_name) { + group.remove_users(HashSet::from_iter([username.clone()]))?; + } + } + } + + // Update metadata with modified groups + for group in metadata.user_groups.iter_mut() { + if user_groups.contains(&group.name) { + group.users.retain(|u| u != &username); + } + }
🧹 Nitpick comments (1)
src/handlers/http/rbac.rs (1)
119-127
: Simplify role validation logic.The pattern
map().for_each(drop)
is unusual and reduces readability. A more idiomatic approach would be clearer.- let mut non_existant_roles = Vec::new(); - user_roles - .iter() - .map(|r| { - if !roles().contains_key(r) { - non_existant_roles.push(r.clone()); - } - }) - .for_each(drop); + let non_existant_roles: Vec<String> = user_roles + .iter() + .filter(|r| !roles().contains_key(r)) + .cloned() + .collect();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/handlers/http/cluster/mod.rs
(1 hunks)src/handlers/http/middleware.rs
(3 hunks)src/handlers/http/modal/ingest/ingestor_rbac.rs
(4 hunks)src/handlers/http/modal/ingest_server.rs
(3 hunks)src/handlers/http/modal/query/querier_rbac.rs
(5 hunks)src/handlers/http/modal/query_server.rs
(6 hunks)src/handlers/http/modal/server.rs
(13 hunks)src/handlers/http/oidc.rs
(1 hunks)src/handlers/http/rbac.rs
(9 hunks)src/handlers/http/role.rs
(3 hunks)src/migration/metadata_migration.rs
(1 hunks)src/migration/mod.rs
(3 hunks)src/rbac/map.rs
(6 hunks)src/rbac/mod.rs
(6 hunks)src/rbac/role.rs
(11 hunks)src/rbac/user.rs
(6 hunks)src/rbac/utils.rs
(3 hunks)src/storage/store_metadata.rs
(5 hunks)src/utils/mod.rs
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- src/storage/store_metadata.rs
- src/handlers/http/oidc.rs
- src/handlers/http/modal/query_server.rs
- src/utils/mod.rs
- src/handlers/http/modal/ingest_server.rs
- src/handlers/http/cluster/mod.rs
- src/handlers/http/role.rs
- src/rbac/utils.rs
- src/migration/metadata_migration.rs
- src/handlers/http/middleware.rs
- src/rbac/user.rs
- src/handlers/http/modal/query/querier_rbac.rs
- src/rbac/role.rs
- src/migration/mod.rs
- src/handlers/http/modal/server.rs
🧰 Additional context used
🧠 Learnings (3)
src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
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.
src/rbac/map.rs (2)
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.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
src/handlers/http/rbac.rs (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-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-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (6)
src/rbac/mod.rs (2)
57-63
: Safe implementation of user group retrieval.The method properly handles missing users by returning an empty set as default, preventing potential panics.
100-113
: Correct implementation of role management methods.Both
add_roles
andremove_roles
methods properly update the user's roles and invalidate sessions to ensure permission changes take effect immediately.src/rbac/map.rs (2)
40-56
: Consistent implementation of user groups global state.The user groups state management follows the established pattern used for other global maps (USERS, ROLES, SESSIONS), maintaining consistency across the codebase.
296-324
: Well-structured permission aggregation with proper error handling.The
aggregate_group_permissions
function safely handles missing users and groups, using pattern matching to avoid panics. The implementation correctly aggregates permissions from all user groups.src/handlers/http/rbac.rs (2)
195-225
: Comprehensive role retrieval including group roles.The enhanced
get_role
handler properly returns both direct roles and roles inherited from user groups, providing complete visibility into a user's effective permissions.
367-430
: Well-designed error types with detailed JSON responses.The new error variants and JSON response formatting provide clear, structured error information for API consumers, particularly for validation errors.
Also applies to: 454-475
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: 0
♻️ Duplicate comments (1)
src/rbac/map.rs (1)
236-254
: Replace confusing comment with clear explanationThe comment "WHEN IS THIS VALID??" at lines 244-246 should be replaced with a clear explanation of when
context_resource
is None.Based on the past review analysis, this occurs during login operations, global/list operations, and middleware calls with no specific resource context. The comment should explain this intended behavior.
- // if no resource to match then resource check is not needed - // WHEN IS THIS VALID?? + // no resource context provided (e.g. login, global operations), + // so skip resource matching and only check the action
🧹 Nitpick comments (1)
src/rbac/map.rs (1)
296-324
: Consider performance optimization for permission aggregationThe
aggregate_group_permissions
function performs multiple cloning operations that could impact performance with large user bases or frequent permission checks.While the current implementation is safe and correct, consider optimizing the cloning pattern if performance becomes a concern:
fn aggregate_group_permissions(username: &str) -> HashSet<Permission> { let mut group_perms = HashSet::new(); // Get user groups without cloning the entire user let user_groups = { let users_lock = users(); users_lock.get(username)?.user_groups.clone() }; if user_groups.is_empty() { return group_perms; } // Process groups with minimal lock holding for group_name in &user_groups { let group_roles = { let groups_lock = read_user_groups(); groups_lock.get(group_name)?.roles.clone() }; for role_name in &group_roles { let privileges = { let roles_lock = roles(); roles_lock.get(role_name)?.clone() }; for privilege in privileges { group_perms.extend(RoleBuilder::from(&privilege).build()); } } } group_perms }However, the current implementation is perfectly acceptable for most use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/http/modal/ingest/ingestor_rbac.rs
(4 hunks)src/handlers/http/role.rs
(3 hunks)src/migration/mod.rs
(3 hunks)src/rbac/map.rs
(6 hunks)src/rbac/role.rs
(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/handlers/http/role.rs
- src/handlers/http/modal/ingest/ingestor_rbac.rs
- src/rbac/role.rs
- src/migration/mod.rs
🧰 Additional context used
🧠 Learnings (1)
src/rbac/map.rs (2)
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.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
🧬 Code Graph Analysis (1)
src/rbac/map.rs (3)
src/rbac/user.rs (4)
new
(263-265)from
(188-197)username
(80-88)roles
(94-96)src/rbac/role.rs (1)
from
(197-211)src/storage/store_metadata.rs (1)
default
(73-87)
⏰ 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: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (6)
src/rbac/map.rs (6)
19-22
: LGTM: Import additions are well-structuredThe new imports support the user group functionality and resource-based permissions. The addition of
HashSet
import aligns with the permission aggregation changes.
40-56
: LGTM: User groups static and accessors follow established patternsThe
USER_GROUPS
static and its accessor functionsread_user_groups()
andwrite_user_groups()
follow the same thread-safe pattern as existingUSERS
andROLES
statics.
112-112
: LGTM: User groups initialization is properly integratedThe initialization of user groups from storage metadata follows the same pattern as other components and is properly integrated into the
init
function.Also applies to: 145-147
225-225
: LGTM: Parameter rename reflects the shift to resource-based permissionsRenaming
context_stream
tocontext_resource
appropriately reflects the broader scope of resource-based permission checking.
229-230
: LGTM: Permission aggregation properly includes group permissionsThe conversion to
HashSet
and extension with group permissions correctly implements the group-based permission inheritance model.
325-346
: LGTM: UserGroups structure follows established patternsThe
UserGroups
struct and its implementation mirror the existingUsers
structure, maintaining consistency in the codebase architecture.
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: 4
♻️ Duplicate comments (1)
src/handlers/http/rbac.rs (1)
248-264
: Simplify group cleanup complexity.This group cleanup logic exhibits the same complexity issues mentioned in previous reviews. The nested loops and multiple metadata updates make it hard to maintain and potentially error-prone.
Consider collecting all necessary changes first, then performing a single metadata update to reduce complexity and improve atomicity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/rbac.rs
(8 hunks)src/handlers/http/role.rs
(1 hunks)src/rbac/role.rs
(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handlers/http/role.rs
- src/rbac/role.rs
🧰 Additional context used
🧠 Learnings (1)
src/handlers/http/rbac.rs (1)
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.
⏰ 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 x86_64-pc-windows-msvc
- GitHub Check: Build Default 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-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
🔇 Additional comments (3)
src/handlers/http/rbac.rs (3)
374-383
: Well-structured error type for user group validation.The
InvalidUserGroupError
struct provides comprehensive validation feedback, which improves API usability by giving clients detailed information about validation failures.
447-467
: Enhanced error response handling provides better API feedback.The JSON-formatted error responses for role-related and user group errors improve API usability by providing structured error information that clients can parse and handle appropriately.
470-476
: RolesResponse structure supports the new group-based role model.The separation of direct roles and group roles in the response structure aligns well with the enhanced permission model that includes user group role inheritance.
user_roles | ||
.iter() | ||
.map(|r| { | ||
if !roles().contains_key(r) { | ||
non_existent_roles.push(r.clone()); | ||
} | ||
}) | ||
.for_each(drop); |
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.
🛠️ Refactor suggestion
Simplify role validation logic for better efficiency.
The current pattern using map().for_each(drop)
is inefficient and hard to read. Consider using a more direct approach.
- user_roles
- .iter()
- .map(|r| {
- if !roles().contains_key(r) {
- non_existent_roles.push(r.clone());
- }
- })
- .for_each(drop);
+ for role in &user_roles {
+ if !roles().contains_key(role) {
+ non_existent_roles.push(role.clone());
+ }
+ }
📝 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.
user_roles | |
.iter() | |
.map(|r| { | |
if !roles().contains_key(r) { | |
non_existent_roles.push(r.clone()); | |
} | |
}) | |
.for_each(drop); | |
for role in &user_roles { | |
if !roles().contains_key(role) { | |
non_existent_roles.push(role.clone()); | |
} | |
} |
🤖 Prompt for AI Agents
In src/handlers/http/rbac.rs around lines 119 to 126, the current code uses
map().for_each(drop) to iterate over user_roles for validation, which is
inefficient and unclear. Replace this with a simple for loop or an iterator
method like for_each that directly checks if each role exists in roles() and
pushes non-existent roles to non_existent_roles, improving readability and
efficiency.
roles_to_remove.iter().for_each(|r| { | ||
if roles().get(r).is_none() { | ||
non_existent_roles.push(r.clone()); | ||
} | ||
}); |
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.
🛠️ Refactor suggestion
Use more efficient iteration pattern.
Same inefficiency pattern as in other handlers.
- // check if the role exists
- roles_to_remove.iter().for_each(|r| {
- if roles().get(r).is_none() {
- non_existent_roles.push(r.clone());
- }
- });
+ // check if the role exists
+ for role in &roles_to_remove {
+ if !roles().contains_key(role) {
+ non_existent_roles.push(role.clone());
+ }
+ }
📝 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.
roles_to_remove.iter().for_each(|r| { | |
if roles().get(r).is_none() { | |
non_existent_roles.push(r.clone()); | |
} | |
}); | |
// check if the role exists | |
for role in &roles_to_remove { | |
if !roles().contains_key(role) { | |
non_existent_roles.push(role.clone()); | |
} | |
} |
🤖 Prompt for AI Agents
In src/handlers/http/rbac.rs around lines 332 to 336, the iteration over
roles_to_remove uses for_each with a closure that pushes to non_existent_roles,
which is less efficient. Replace this with a more efficient iterator method such
as filter_map or filter followed by collect to directly build non_existent_roles
without explicit push calls, improving clarity and performance.
roles_to_add.iter().for_each(|r| { | ||
if roles().get(r).is_none() { | ||
non_existent_roles.push(r.clone()); | ||
} | ||
}); |
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.
🛠️ Refactor suggestion
Use more efficient iteration pattern.
Similar to the post_user handler, this validation pattern is inefficient.
- // check if the role exists
- roles_to_add.iter().for_each(|r| {
- if roles().get(r).is_none() {
- non_existent_roles.push(r.clone());
- }
- });
+ // check if the role exists
+ for role in &roles_to_add {
+ if !roles().contains_key(role) {
+ non_existent_roles.push(role.clone());
+ }
+ }
📝 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.
roles_to_add.iter().for_each(|r| { | |
if roles().get(r).is_none() { | |
non_existent_roles.push(r.clone()); | |
} | |
}); | |
// check if the role exists | |
for role in &roles_to_add { | |
if !roles().contains_key(role) { | |
non_existent_roles.push(role.clone()); | |
} | |
} |
🤖 Prompt for AI Agents
In src/handlers/http/rbac.rs around lines 287 to 291, the current iteration uses
for_each with a closure to check for non-existent roles, which is inefficient.
Replace this with a more efficient iterator method such as filter_map or filter
to directly collect non-existent roles into the vector without explicit push
calls, improving readability and performance.
// if user is a part of any groups then don't allow deletion | ||
if !Users.get_user_groups(&username).is_empty() { | ||
return Err(RBACError::InvalidDeletionRequest(format!( | ||
"User: {username} should not be a part of any groups" | ||
))); | ||
} | ||
// fail this request if the user does not exists | ||
if !Users.contains(&username) { | ||
return Err(RBACError::UserDoesNotExist); | ||
}; | ||
let _ = UPDATE_LOCK.lock().await; | ||
|
||
// delete from parseable.json first | ||
let mut metadata = get_metadata().await?; | ||
metadata.users.retain(|user| user.username() != username); | ||
|
||
// Remove user from all groups | ||
let user_groups = Users.get_user_groups(&username); | ||
{ | ||
let mut groups = write_user_groups(); | ||
for group_name in &user_groups { | ||
if let Some(group) = groups.get_mut(group_name) { | ||
group.remove_users(HashSet::from_iter([username.clone()]))?; | ||
} | ||
} | ||
} | ||
|
||
// Update metadata with modified groups | ||
for group in metadata.user_groups.iter_mut() { | ||
if user_groups.contains(&group.name) { | ||
group.users.retain(|u| u != &username); | ||
} | ||
} |
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.
Fix contradictory logic in user deletion and group cleanup.
The handler first prevents deletion if the user is in any groups (lines 232-237), but then has code to remove the user from groups (lines 248-264). This is contradictory logic that will never execute the cleanup code.
Either:
- Remove the group membership check and allow deletion with cleanup, or
- Remove the cleanup code since deletion is prevented for users in groups
- // if user is a part of any groups then don't allow deletion
- if !Users.get_user_groups(&username).is_empty() {
- return Err(RBACError::InvalidDeletionRequest(format!(
- "User: {username} should not be a part of any groups"
- )));
- }
If you want to allow deletion with cleanup, remove the above check. If you want to prevent deletion, remove the cleanup code (lines 248-264).
📝 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.
// if user is a part of any groups then don't allow deletion | |
if !Users.get_user_groups(&username).is_empty() { | |
return Err(RBACError::InvalidDeletionRequest(format!( | |
"User: {username} should not be a part of any groups" | |
))); | |
} | |
// fail this request if the user does not exists | |
if !Users.contains(&username) { | |
return Err(RBACError::UserDoesNotExist); | |
}; | |
let _ = UPDATE_LOCK.lock().await; | |
// delete from parseable.json first | |
let mut metadata = get_metadata().await?; | |
metadata.users.retain(|user| user.username() != username); | |
// Remove user from all groups | |
let user_groups = Users.get_user_groups(&username); | |
{ | |
let mut groups = write_user_groups(); | |
for group_name in &user_groups { | |
if let Some(group) = groups.get_mut(group_name) { | |
group.remove_users(HashSet::from_iter([username.clone()]))?; | |
} | |
} | |
} | |
// Update metadata with modified groups | |
for group in metadata.user_groups.iter_mut() { | |
if user_groups.contains(&group.name) { | |
group.users.retain(|u| u != &username); | |
} | |
} | |
// fail this request if the user does not exists | |
if !Users.contains(&username) { | |
return Err(RBACError::UserDoesNotExist); | |
}; | |
let _ = UPDATE_LOCK.lock().await; | |
// delete from parseable.json first | |
let mut metadata = get_metadata().await?; | |
metadata.users.retain(|user| user.username() != username); | |
// Remove user from all groups | |
let user_groups = Users.get_user_groups(&username); | |
{ | |
let mut groups = write_user_groups(); | |
for group_name in &user_groups { | |
if let Some(group) = groups.get_mut(group_name) { | |
group.remove_users(HashSet::from_iter([username.clone()]))?; | |
} | |
} | |
} | |
// Update metadata with modified groups | |
for group in metadata.user_groups.iter_mut() { | |
if user_groups.contains(&group.name) { | |
group.users.retain(|u| u != &username); | |
} | |
} |
🤖 Prompt for AI Agents
In src/handlers/http/rbac.rs between lines 232 and 264, there is contradictory
logic where user deletion is blocked if the user belongs to any groups, but
later code attempts to remove the user from those groups. To fix this, decide
whether to allow deletion with cleanup or prevent deletion if the user is in
groups. If allowing deletion, remove the initial group membership check (lines
232-237) so the cleanup code (lines 248-264) runs. If preventing deletion,
remove the cleanup code (lines 248-264) since it will never execute.
PUT /user/{username}/role
toPATCH /user/{username}/role/add
andPATCH /user/{username}/role/remove
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores