fix: use userid, username and method in a usergroup #1422
Conversation
WalkthroughIdentity handling migrated from username-based to userid-based across RBAC core, HTTP handlers, routes, sessions, ingestors, and Prism conversion. Introduced Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as HTTP RBAC API
participant RBAC as RBAC Core
participant Store as Metadata Store
participant Ing as Ingestors
rect rgb(245,248,255)
note right of API: DELETE /api/v1/user/{userid}
Client->>API: DELETE {userid}
API->>API: Acquire UPDATE_LOCK (_guard)
API->>RBAC: users().get(userid) → display name (optional)
API->>Store: put_metadata(updated without user where user.userid == userid)
API->>Ing: sync_user_deletion_with_ingestors(userid)
API->>RBAC: Users.delete_user(userid)
API-->>Client: 200 OK (display name if available)
end
sequenceDiagram
autonumber
actor Admin
participant API as Role Handler
participant RBAC as RBAC Core
participant Sess as Sessions
rect rgb(245,255,245)
note right of API: PUT /role/{role}
Admin->>API: Update role
API->>RBAC: read_user_groups() & users() (by reference)
API->>API: Collect affected userids (GroupUser.userid / User.userid)
loop for each userid
API->>Sess: remove_sessions(userid)
end
API->>RBAC: sync_role_update_with_ingestors(role, affected_userids)
API-->>Admin: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/http/modal/query/querier_rbac.rs (1)
106-149: Use the single shared UPDATE_LOCK and hold its guard
- In
src/handlers/http/modal/query/querier_rbac.rs(and similarly iningestor_rbac.rs), remove the localand instead import the one fromstatic UPDATE_LOCK: Mutex<()> = Mutex::const_new(());src/handlers/http/rbac.rs(make itpub(crate)if not already).- Change
tolet _ = UPDATE_LOCK.lock().await;so the lock stays held for the entire delete flow.let _guard = UPDATE_LOCK.lock().await;
🧹 Nitpick comments (3)
src/rbac/user.rs (2)
329-343: Avoid extra clone when extending users.You clone the entire
HashSet<GroupUser>only to iterate it again. Iterate first for session refresh, then extend by moving the set.Apply this diff:
- self.users.extend(users.clone()); - // also refresh all user sessions - for group_user in &users { + // refresh all user sessions + for group_user in &users { mut_sessions().remove_user(group_user.userid()); } + self.users.extend(users);
384-391: Expose userids to prevent misuse of display names downstream.Multiple handlers need stable IDs for session invalidation. Add a
get_userids()helper alongsideget_usernames().Apply this diff:
/// Get all usernames in this group pub fn get_usernames(&self) -> Vec<String> { self.users .iter() .map(|u| u.username().to_string()) .collect() } + + /// Get all userids in this group (stable identifier) + pub fn get_userids(&self) -> Vec<String> { + self.users + .iter() + .map(|u| u.userid().to_string()) + .collect() + }Then update callers to use
get_userids()for session invalidation.src/handlers/http/oidc.rs (1)
435-437: Group migration is correct; consider small clarity/robustness tweaks.
- Nit:
old_usernameis actually the prior userid for OAuth; consider renaming for clarity.- Optional: proactively invalidate sessions keyed by the old id to avoid stray sessions on other devices.
Example:
- // Store the old username before modifying the user object - let old_username = user.username().to_string(); + // Store the old userid before modifying the user object + let old_userid = user.username().to_string(); ... - group.users.retain(|u| u.userid() != old_username); + group.users.retain(|u| u.userid() != old_userid); group.users.insert(GroupUser::from_user(&user));And optionally after metadata write:
// optional cleanup mut_sessions().remove_user(&old_userid); mut_sessions().remove_user(user.username());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/handlers/http/modal/ingest/ingestor_role.rs(1 hunks)src/handlers/http/modal/query/querier_rbac.rs(2 hunks)src/handlers/http/modal/query/querier_role.rs(1 hunks)src/handlers/http/oidc.rs(2 hunks)src/handlers/http/rbac.rs(4 hunks)src/handlers/http/role.rs(1 hunks)src/rbac/user.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/handlers/http/role.rs (1)
src/rbac/map.rs (2)
read_user_groups(42-48)users(58-64)
src/handlers/http/modal/ingest/ingestor_role.rs (1)
src/rbac/map.rs (2)
read_user_groups(42-48)users(58-64)
src/handlers/http/modal/query/querier_role.rs (1)
src/rbac/map.rs (2)
read_user_groups(42-48)users(58-64)
src/handlers/http/oidc.rs (2)
src/rbac/user.rs (1)
from_user(231-253)src/handlers/http/modal/utils/rbac_utils.rs (1)
put_metadata(34-38)
src/rbac/user.rs (2)
src/rbac/mod.rs (1)
is_oauth(68-70)src/rbac/map.rs (2)
users(58-64)mut_sessions(98-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka 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 (14)
src/handlers/http/modal/ingest/ingestor_role.rs (1)
58-61: LGTM: direct-user scan uses stable key.
user.username()resolves to userid for OAuth, so this path invalidates sessions correctly.src/rbac/user.rs (6)
292-295: Creation-time validation expectsuserid, but PR API examples send onlyusername+method.
users().contains_key(group_user.userid())assumesuseridis present. If endpoints deserialize request bodies directly intoGroupUser, requests withoutuseridwill fail.Please verify HTTP DTOs for POST/PATCH /usergroup do NOT deserialize into
GroupUserdirectly. If they do, introduce a request type:#[derive(Deserialize)] struct GroupUserInput { username: String, method: String }and resolve to
GroupUser { userid, username, method }server-side via lookup.
319-321: Constructor change looks good.Accepts
HashSet<GroupUser>; pairs with the refactors elsewhere.
360-362: LGTM: session refresh on role removal uses userid.
366-381: LGTM: removal computes set diff and refreshes removed users by userid.
392-397: LGTM: ergonomic helper for adding from&Userrefs.
399-413: LGTM:remove_users_by_user_idsaligns with userid-as-canonical semantics.src/handlers/http/modal/query/querier_role.rs (1)
61-64: LGTM: direct-user path uses stable key.src/handlers/http/role.rs (1)
60-63: LGTM: direct-user path uses stable key.src/handlers/http/rbac.rs (3)
69-76: Docs cleanups LGTMRoute comments/readability improvements look good.
353-361: Public JSON shape change to camelCase — confirm downstream compatibilitySwitching to
#[serde(rename_all = "camelCase")]changes field names in HTTP error payloads. Ensure UI/SDKs/tests consumingInvalidUserGroupErrorare updated.
393-395: User-facing error string changed — verify callers not asserting on text“UserGroup
{0}is still in use” replaces the prior message. If any clients/tests compare strings, update them or prefer structured errors.src/handlers/http/modal/query/querier_rbac.rs (2)
130-141: In-place metadata update LGTMThe replace-or-push logic is correct and idempotent for group names.
106-106: Route doc correction LGTMUpdated DELETE route path matches the handler behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/handlers/http/rbac.rs (2)
127-152: Avoid calling unlocked helpers while holding the global lock (risk of inconsistent patterns/deadlocks later)You correctly hold the guard here, but
add_roles_to_userdoes not lock; other call sites may run it concurrently. Either inline the role update under this guard or release it before calling and have the callee lock.Apply:
- Users.put_user(user.clone()); - - add_roles_to_user( + Users.put_user(user.clone()); + // Release the global UPDATE_LOCK before calling a function that acquires it. + drop(_guard); + add_roles_to_user( web::Path::<String>::from(username.clone()), web::Json(created_role), ) .await?;And add a lock at the start of
add_roles_to_user/remove_roles_from_user(in this file) so they are always atomic:// inside add/remove role handlers let _guard = UPDATE_LOCK.lock().await;
448-454:#[serde(rename = "camelCase")]is ineffective for field casing on structsUse
rename_allto actually camelCase the fields (you already aliasdirect_roles→roles).-#[derive(Serialize)] -#[serde(rename = "camelCase")] +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] pub struct RolesResponse { #[serde(rename = "roles")] pub direct_roles: HashMap<String, Vec<DefaultPrivilege>>, pub group_roles: HashMap<String, HashMap<String, Vec<DefaultPrivilege>>>, }
♻️ Duplicate comments (3)
src/handlers/http/rbac.rs (1)
45-46: Export a single shared UPDATE_LOCK; duplicate locks across modules break atomicityMake this lock reusable across modules and remove per-file duplicates in querier/ingestor. Holding different locks provides no cross-module exclusion.
Apply:
-// async aware lock for updating storage metadata and user map atomically -static UPDATE_LOCK: Mutex<()> = Mutex::const_new(()); +// async-aware lock for updating storage metadata and user map atomically +pub(crate) static UPDATE_LOCK: Mutex<()> = Mutex::const_new(());#!/usr/bin/env bash # Find duplicate UPDATE_LOCKs and transient guard usages rg -nP 'static\s+UPDATE_LOCK' -g '!**/target/**' -C2 rg -nP 'let\s+_\s*=\s*UPDATE_LOCK\.lock\(\)\.await' -g '!**/target/**' -C2src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
34-36: Use the shared UPDATE_LOCK; fix typoDuplicate lock defeats cross-module serialization; also fix “atomicically” → “atomically”.
-// async aware lock for updating storage metadata and user map atomicically -static UPDATE_LOCK: Mutex<()> = Mutex::const_new(()); +// Use the shared lock to keep cross-module updates atomic +use crate::handlers::http::rbac::UPDATE_LOCK;If
Muteximport becomes unused:-use tokio::sync::Mutex;#!/usr/bin/env bash rg -nP 'static\s+UPDATE_LOCK' -g '!**/target/**' -C2src/handlers/http/modal/query/querier_rbac.rs (1)
41-42: Consolidate on the shared UPDATE_LOCKReplace the local static with the exported lock to ensure global serialization.
-// async aware lock for updating storage metadata and user map atomically -static UPDATE_LOCK: Mutex<()> = Mutex::const_new(()); +// Use the shared lock for cross-module atomicity +use crate::handlers::http::rbac::UPDATE_LOCK;Drop
use tokio::sync::Mutex;if unused after this change.
🧹 Nitpick comments (7)
src/handlers/http/rbac.rs (1)
54-66: Clarify field naming: id is actually userid for OAuth usersSince
user.username()resolves to OAuth userid, consider renaming the response field touserid(or document clearly) to avoid API ambiguity.src/rbac/user.rs (3)
235-241: Prefer a typed method enum over string literalsTo avoid typos and ease pattern matching, consider
enum AuthMethod { Native, OAuth }with#[serde(rename_all = "lowercase")].
243-266: from_user mapping is correct; minor nit on styleYou can shorten the iterator map in other call sites to
map(GroupUser::from_user).
404-409: Tiny iterator nitUse function pointer style.
- let group_users: HashSet<GroupUser> = - user_refs.iter().map(|u| GroupUser::from_user(u)).collect(); + let group_users: HashSet<GroupUser> = + user_refs.iter().map(GroupUser::from_user).collect();src/handlers/http/modal/query/querier_rbac.rs (3)
109-159: Delete: now resolves correct userid for group removal — good; update misleading commentCode removes by internal userid; comment mentions “display name”.
- // Look up the user by display name to get the correct userid + // Resolve the canonical internal userid for this username
173-181: Spelling nit: non_existant_roles → non_existent_roles- let mut non_existant_roles = Vec::new(); + let mut non_existent_roles = Vec::new(); @@ - if !non_existant_roles.is_empty() { - return Err(RBACError::RolesDoNotExist(non_existant_roles)); + if !non_existent_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existent_roles)); }
220-228: Spelling nit: non_existant_roles → non_existent_roles- let mut non_existant_roles = Vec::new(); + let mut non_existent_roles = Vec::new(); @@ - if !non_existant_roles.is_empty() { - return Err(RBACError::RolesDoNotExist(non_existant_roles)); + if !non_existent_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existent_roles)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/handlers/http/modal/ingest/ingestor_rbac.rs(1 hunks)src/handlers/http/modal/ingest/ingestor_role.rs(1 hunks)src/handlers/http/modal/query/querier_rbac.rs(5 hunks)src/handlers/http/modal/query/querier_role.rs(1 hunks)src/handlers/http/rbac.rs(7 hunks)src/handlers/http/role.rs(1 hunks)src/rbac/user.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/handlers/http/modal/ingest/ingestor_role.rs
- src/handlers/http/role.rs
- src/handlers/http/modal/query/querier_role.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/rbac/user.rs (2)
src/rbac/mod.rs (1)
is_oauth(68-70)src/rbac/map.rs (2)
users(58-64)mut_sessions(98-104)
src/handlers/http/modal/query/querier_rbac.rs (4)
src/rbac/user.rs (3)
roles(98-100)username(84-92)username(227-229)src/rbac/map.rs (3)
roles(74-80)users(58-64)write_user_groups(50-56)src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
delete_user(59-75)src/handlers/http/rbac.rs (1)
delete_user(225-248)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: coverage
- 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 Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (11)
src/handlers/http/rbac.rs (3)
162-183: Password reset now correctly holds the lockGuard lifetime covers metadata + in-memory update. Good.
238-248: User deletion holds the lock across metadata and in-memory updatesThis fixes lost-update races here.
353-361: serde casing fix looks good
#[serde(rename_all = "camelCase")]on InvalidUserGroupError matches the API intent.src/rbac/user.rs (6)
206-224: Eq/Hash by userid-only — prevents duplicate membership on display-name changesGood manual impls; this fixes the prior duplication risk.
304-307: Validate by userid, report by display name — good UXThis matches the new identity model.
347-357: Session refresh on add_users — LGTMRevoking sessions for newly added users is correct.
378-394: Session refresh on remove_users — LGTMMinimal and correct set math.
396-403: Helper returns display usernames — OKNaming matches behavior.
206-213: Separate persistedGroupUserfrom request DTO
Ensure your create/update endpoints accept request bodies withoutuserid. I didn’t find any handlers deserializingGroupUserdirectly—please verify none of your POST/PATCH handlers acceptGroupUseras the JSON body. If they do, introduce an input DTO:#[derive(serde::Deserialize)] pub struct GroupUserInput { pub username: String, pub method: String, }Then map
GroupUserInput→GroupUserinternally, filling inuserid. Otherwise keepGroupUserinternal-only.src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
61-61: Guard held during delete — LGTMThis aligns with the locking model.
src/handlers/http/modal/query/querier_rbac.rs (1)
275-298: Password reset: guard now held — LGTMLock covers metadata write + in-memory update + cluster sync.
new structure for POST /usergroup
```
{
"name": "parseable",
"roles": [],
"users": [
{
"username": "nikhil",
"method": "native"
}
]
}
```
new structure for add/remove user - PATCH /usergroup/{name}/add(remove)
```
{
"users": [
{
"username": "authentik Default Admin",
"method": "oauth"
}
]
}
```
nikhilsinhaparseable
force-pushed
the
oauth-userid
branch
from
3993f91 to
3a7a06e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/rbac/map.rs (1)
206-212: Expired sessions are retained (comparison inverted).retain keeps items where predicate is true; expiry < now keeps expired sessions and drops valid ones.
- sessions.retain(|(_, expiry)| expiry < &now); + // keep only non-expired sessions + sessions.retain(|(_, expiry)| expiry > &now);src/rbac/mod.rs (1)
210-229: serde attribute should berename_all, notrename.
#[serde(rename = "camelCase")]won’t camel-case the field names; it renames the type. This breaks the API contract for list_users_prism/get_prism_user.-#[serde(rename = "camelCase")] +#[serde(rename_all = "camelCase")] pub struct UsersPrism {src/handlers/http/modal/ingest/ingestor_rbac.rs (2)
84-103: Guard metadata mutation with the shared UPDATE_LOCK and avoid cloninguserid.Two tweaks:
- Acquire the lock before reading/updating metadata to avoid TOCTOU.
- Pass
&useriddirectly toUsers.add_roles.pub async fn add_roles_to_user( userid: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); + let _guard = UPDATE_LOCK.lock().await; if !Users.contains(&userid) { return Err(RBACError::UserDoesNotExist); }; @@ - let _ = storage::put_staging_metadata(&metadata); - // update in mem table - Users.add_roles(&userid.clone(), roles_to_add.clone()); + let _ = storage::put_staging_metadata(&metadata); + // update in mem table + Users.add_roles(&userid, roles_to_add.clone());
135-175: Same here: hold UPDATE_LOCK and avoid cloninguserid.Mirror the locking discipline from delete and rbac handlers; pass
&userid.pub async fn remove_roles_from_user( userid: web::Path<String>, roles_to_remove: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); + let _guard = UPDATE_LOCK.lock().await; if !Users.contains(&userid) { return Err(RBACError::UserDoesNotExist); };src/handlers/http/rbac.rs (4)
79-91: Bug: endpoint uses “username” but lookups are keyed by userid.OAuth users will 404. Make the path parameter
useridand look up by userid.-/// Function for GET /users/{username} -pub async fn get_prism_user(username: Path<String>) -> Result<impl Responder, RBACError> { - let username = username.into_inner(); +/// Function for GET /users/{userid} +pub async fn get_prism_user(userid: Path<String>) -> Result<impl Responder, RBACError> { + let userid = userid.into_inner(); // First check if the user exists let users = rbac::map::users(); - if let Some(user) = users.get(&username) { + if let Some(user) = users.get(&userid) { let prism_user = to_prism_user(user); Ok(web::Json(prism_user)) } else { Err(RBACError::UserDoesNotExist) } }
101-133: Avoid stale read: take UPDATE_LOCK before reading metadata; simplify existence check.Current flow reads metadata, then locks, risking lost updates. Also the nested
Users.containsis redundant.- let mut metadata = get_metadata().await?; + let _guard = UPDATE_LOCK.lock().await; + let mut metadata = get_metadata().await?; @@ - let _guard = UPDATE_LOCK.lock().await; - if Users.contains(&username) { - if Users.contains(&username) - || metadata.users.iter().any(|user| match &user.ty { - UserType::Native(basic) => basic.username == username, - UserType::OAuth(_) => false, - }) - { - return Err(RBACError::UserExists); - } - } + if Users.contains(&username) + || metadata + .users + .iter() + .any(|user| matches!(&user.ty, UserType::Native(basic) if basic.username == username)) + { + return Err(RBACError::UserExists); + }
158-166: Same TOCTOU: lock before reading metadata.Ensure atomic read-modify-write for password reset.
- let mut metadata = get_metadata().await?; - - let _guard = UPDATE_LOCK.lock().await; + let _guard = UPDATE_LOCK.lock().await; + let mut metadata = get_metadata().await?;
185-220: Compile bug:useridis a Path; callinto_inner()before use.Handlers elsewhere already do this.
-pub async fn get_role(userid: web::Path<String>) -> Result<impl Responder, RBACError> { - if !Users.contains(&userid) { +pub async fn get_role(userid: web::Path<String>) -> Result<impl Responder, RBACError> { + let userid = userid.into_inner(); + if !Users.contains(&userid) { return Err(RBACError::UserDoesNotExist); }; - let direct_roles: HashMap<String, Vec<DefaultPrivilege>> = Users - .get_role(&userid) + let direct_roles: HashMap<String, Vec<DefaultPrivilege>> = Users + .get_role(&userid) @@ - for user_group in Users.get_user_groups(&userid) { + for user_group in Users.get_user_groups(&userid) {
🧹 Nitpick comments (19)
src/rbac/utils.rs (2)
33-41: Clarify 'username' semantics (display name vs identifier).You now set id=userid for both auth types, but username=userid (native) and username=display_name (OAuth). Ensure all downstream code treats id as the stable key and username purely as a display field; otherwise uniqueness assumptions may break.
Would you like me to scan handlers/UI for any equality checks or map keys still using username?
103-129: Unicode-safe masking.mask_string uses .len() (bytes) while extracting the first char via .chars(), which can miscount for multi-byte names. Count chars, not bytes.
- let parts: Vec<&str> = input.split('@').collect(); + let parts: Vec<&str> = input.split('@').collect(); //mask everything if not a proper email format if parts.len() != 2 { - return Some("X".repeat(input.len())); + return Some("X".repeat(input.chars().count())); } let username = parts[0]; - let masked_username = if !username.is_empty() { - let first = username.chars().next().unwrap().to_uppercase().to_string(); - format!("{}{}", first, "X".repeat(username.len() - 1)) + let masked_username = if !username.is_empty() { + let mut it = username.chars(); + let first = it.next().unwrap().to_uppercase().to_string(); + let rest = it.count(); + format!("{}{}", first, "X".repeat(rest)) } else { - return Some("X".repeat(input.len())); + return Some("X".repeat(input.chars().count())); }; - Some(format!("{masked_username}@XXX")) + Some(format!("{masked_username}@XXX")) } else { // mask all other strings with X - Some("X".repeat(input.len())) + Some("X".repeat(input.chars().count())) }src/rbac/map.rs (3)
124-138: Use userid for admin session (good), but rename var to avoid confusion.Logic is correct; consider renaming admin_username -> admin_userid for clarity.
- let admin_username = admin.userid().to_owned(); + let admin_userid = admin.userid().to_owned(); ... - sessions.track_new( - admin_username, + sessions.track_new( + admin_userid,
196-204: Parameter naming drift after userid switch.remove_user(&mut self, username: &str) still says username, but it now represents userid. Rename for clarity; no behavior change.
- pub fn remove_user(&mut self, username: &str) { - let sessions = self.user_sessions.remove(username); + pub fn remove_user(&mut self, userid: &str) { + let sessions = self.user_sessions.remove(userid);
305-333: Make aggregate_group_permissions parameter name reflect userid.Avoid mental mismatches; the map key is now userid.
-fn aggregate_group_permissions(username: &str) -> HashSet<Permission> { +fn aggregate_group_permissions(userid: &str) -> HashSet<Permission> { let mut group_perms = HashSet::new(); - let Some(user) = users().get(username).cloned() else { + let Some(user) = users().get(userid).cloned() else { return group_perms; };src/utils/mod.rs (1)
61-66: Avoid unwrap on missing/invalid session key.extract_session_key_from_req(req).unwrap() can panic. Prefer graceful error.
- let session_key = extract_session_key_from_req(req).unwrap(); + let Some(session_key) = extract_session_key_from_req(req) else { + return Err(RBACError::UserDoesNotExist); + };src/handlers/http/modal/query_server.rs (1)
196-210: Route param mismatch and incorrect HTTP verb in docs for the same resource
- The resource path is
/{username}but the comment for DELETE says{userid}. Also, the comment says "PUT" while the code uses POST.- For clarity and consistency with userid-first APIs, prefer a single token name.
Apply:
- web::resource("/{username}") - // PUT /user/{username} => Create a new user + web::resource("/{userid}") + // POST /user/{userid} => Create a new native user (userid=username) .route( web::post() .to(querier_rbac::post_user) .authorize(Action::PutUser), ) // DELETE /user/{userid} => Delete a user .route( web::delete() .to(querier_rbac::delete_user) .authorize(Action::DeleteUser), )src/handlers/http/cluster/mod.rs (1)
334-349: Use userid all the way through (naming and docs)Functionally correct: you now sync user creation via userid in the URL. For readability, consider renaming parameters and comments of related helpers (e.g., sync_users_with_roles_with_ingestors, sync_user_deletion_with_ingestors) from “username” to “userid” to avoid confusion. No behavior change needed.
src/handlers/http/modal/ingest_server.rs (1)
186-200: Keep route token consistent with useridThe resource is
/{username}/syncbut comments now reference{userid}. Prefer consistent tokens to avoid confusion (extractors don’t care about the token name, but docs and maintainability do).- web::resource("/{username}/sync") - // PUT /user/{username}/sync => Sync creation of a new user + web::resource("/{userid}/sync") + // POST /user/{userid}/sync => Sync creation of a new user .route( web::post() .to(ingestor_rbac::post_user) .authorize(Action::PutUser), ) - // DELETE /user/{userid} => Sync deletion of a user + // DELETE /user/{userid}/sync => Sync deletion of a usersrc/handlers/http/oidc.rs (2)
370-388: Avoid unnecessary metadata writes in put_user
put_userwrites metadata even when the user already exists (no mutation). Optimize to write only on insert.-pub async fn put_user( - userid: &str, - group: HashSet<String>, - user_info: user::UserInfo, -) -> Result<User, ObjectStorageError> { - let mut metadata = get_metadata().await?; - - let user = metadata - .users - .iter() - .find(|user| user.userid() == userid) - .cloned() - .unwrap_or_else(|| { - let user = User::new_oauth(userid.to_owned(), group, user_info); - metadata.users.push(user.clone()); - user - }); - - put_metadata(&metadata).await?; - Users.put_user(user.clone()); - Ok(user) -} +pub async fn put_user( + userid: &str, + group: HashSet<String>, + user_info: user::UserInfo, +) -> Result<User, ObjectStorageError> { + let mut metadata = get_metadata().await?; + if let Some(existing) = metadata.users.iter().find(|u| u.userid() == userid).cloned() { + Users.put_user(existing.clone()); + return Ok(existing); + } + let user = User::new_oauth(userid.to_owned(), group, user_info); + metadata.users.push(user.clone()); + put_metadata(&metadata).await?; + Users.put_user(user.clone()); + Ok(user) +}
398-443: Variable naming: old_userid (not old_username) + minor robustness
let old_username = user.userid().to_string();is actually an old userid, which is confusing when reading migration logic. Rename for clarity.- Optional: If the user entry isn’t found in metadata (shouldn’t happen), consider inserting rather than silently doing nothing before writing.
- let old_username = user.userid().to_string(); + let old_userid = user.userid().to_string(); ... - .find(|x| x.userid() == old_username) + .find(|x| x.userid() == old_userid) ... - group.users.retain(|u| u.userid() != old_username); + group.users.retain(|u| u.userid() != old_userid); ... - Users.delete_user(&old_username); + Users.delete_user(&old_userid);src/handlers/http/modal/query/querier_rbac.rs (2)
73-83: Redundant existence checks — simplifyThe nested
Users.contains(&username)+ repeatedUsers.containsis redundant; return early on first check, then validate metadata separately if needed.- let _guard = UPDATE_LOCK.lock().await; - if Users.contains(&username) { - if Users.contains(&username) - || metadata.users.iter().any(|user| match &user.ty { - UserType::Native(basic) => basic.username == username, - UserType::OAuth(_) => false, // OAuth users should be created differently - }) - { - return Err(RBACError::UserExists); - } - } + let _guard = UPDATE_LOCK.lock().await; + if Users.contains(&username) + || metadata.users.iter().any(|u| matches!(&u.ty, UserType::Native(b) if b.username == username)) + { + return Err(RBACError::UserExists); + }
104-166: Release UPDATE_LOCK before network sync; avoid repeat lookups
- Hold the lock for metadata + in-memory mutations only; release before
sync_user_deletion_with_ingestorsto reduce contention.- Avoid repeated
users().get(&userid)inside the loop. Compute once.- // also delete from user groups - let user_groups = Users.get_user_groups(&userid); + // also delete from user groups + let user_groups = Users.get_user_groups(&userid); + let user_opt = users().get(&userid).cloned(); let mut groups_to_update = Vec::new(); for user_group in user_groups { if let Some(ug) = write_user_groups().get_mut(&user_group) { - // Look up the user by display name to get the correct userid - if let Some(user) = users().get(&userid) { - let userid = match &user.ty { + if let Some(user) = user_opt.as_ref() { + let canonical_userid = match &user.ty { UserType::Native(basic) => basic.username.clone(), UserType::OAuth(oauth) => oauth.userid.clone(), }; - ug.remove_users_by_user_ids(HashSet::from_iter([userid]))?; + ug.remove_users_by_user_ids(HashSet::from_iter([canonical_userid]))?; groups_to_update.push(ug.clone()); } else { // User not found, skip or log as needed continue; } } else { continue; } } @@ - put_metadata(&metadata).await?; - - sync_user_deletion_with_ingestors(&userid).await?; - - // update in mem table - Users.delete_user(&userid); + put_metadata(&metadata).await?; + // update in mem table + Users.delete_user(&userid); + // release the lock before network I/O + drop(_guard); + sync_user_deletion_with_ingestors(&userid).await?;src/rbac/mod.rs (3)
89-97: Prefer&strfor hash to avoid unnecessary coupling toString.Signature can accept
&str; callers withStringwill coerce. Minor ergonomics win.- pub fn change_password_hash(&self, userid: &str, hash: &String) { + pub fn change_password_hash(&self, userid: &str, hash: &str) { if let Some(User { ty: UserType::Native(user), .. }) = mut_users().get_mut(userid) { - user.password_hash.clone_from(hash); + user.password_hash.clear(); + user.password_hash.push_str(hash); mut_sessions().remove_user(userid); }; }
100-105: Add missing semicolon after session invalidation.Silences clippy’s “semicolon_if_nothing_returned” and clarifies intent.
- mut_sessions().remove_user(userid) + mut_sessions().remove_user(userid);
107-113: Add missing semicolon after session invalidation.Same nit as above.
- mut_sessions().remove_user(userid) + mut_sessions().remove_user(userid);src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
192-197: Avoid cloninguseridwhen updating in-memory roles.No need to allocate here.
- Users.remove_roles(&userid.clone(), roles_to_remove.clone()); + Users.remove_roles(&userid, roles_to_remove.clone());src/handlers/http/rbac.rs (2)
256-306: Avoid cloninguseridwhen mutating roles.Pass
&useriddirectly.- Users.add_roles(&userid.clone(), roles_to_add); + Users.add_roles(&userid, roles_to_add);
308-370: Same nit: drop unnecessaryuseridclone.- Users.remove_roles(&userid.clone(), roles_to_remove); + Users.remove_roles(&userid, roles_to_remove);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
src/handlers/http/cluster/mod.rs(1 hunks)src/handlers/http/modal/ingest/ingestor_rbac.rs(7 hunks)src/handlers/http/modal/ingest/ingestor_role.rs(1 hunks)src/handlers/http/modal/ingest_server.rs(3 hunks)src/handlers/http/modal/query/querier_rbac.rs(9 hunks)src/handlers/http/modal/query/querier_role.rs(1 hunks)src/handlers/http/modal/query_server.rs(3 hunks)src/handlers/http/oidc.rs(5 hunks)src/handlers/http/rbac.rs(14 hunks)src/handlers/http/role.rs(1 hunks)src/rbac/map.rs(4 hunks)src/rbac/mod.rs(3 hunks)src/rbac/user.rs(7 hunks)src/rbac/utils.rs(2 hunks)src/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/handlers/http/modal/ingest/ingestor_role.rs
- src/handlers/http/role.rs
- src/handlers/http/modal/query/querier_role.rs
- src/rbac/user.rs
🧰 Additional context used
🧬 Code graph analysis (10)
src/utils/mod.rs (1)
src/handlers/http/rbac.rs (1)
Users(67-67)
src/handlers/http/cluster/mod.rs (3)
src/rbac/user.rs (2)
userid(84-89)userid(243-245)src/handlers/http/modal/mod.rs (2)
domain_name(572-572)domain_name(579-581)src/handlers/http/mod.rs (1)
base_path_without_preceding_slash(78-80)
src/rbac/utils.rs (1)
src/rbac/user.rs (2)
userid(84-89)userid(243-245)
src/rbac/mod.rs (2)
src/rbac/map.rs (5)
mut_sessions(98-104)mut_users(66-72)users(58-64)roles(74-80)sessions(90-96)src/rbac/user.rs (8)
userid(84-89)userid(243-245)is_oauth(106-108)is_oauth(247-249)hash(233-235)add_roles(347-357)roles(110-112)remove_roles(371-388)
src/handlers/http/modal/query_server.rs (2)
src/handlers/http/rbac.rs (1)
get_role(185-220)src/rbac/mod.rs (1)
get_role(76-81)
src/rbac/map.rs (1)
src/rbac/user.rs (2)
userid(84-89)userid(243-245)
src/handlers/http/oidc.rs (2)
src/rbac/user.rs (4)
userid(84-89)userid(243-245)new_oauth(69-82)from_user(255-277)src/handlers/http/role.rs (1)
put_metadata(153-157)
src/handlers/http/modal/query/querier_rbac.rs (5)
src/rbac/map.rs (3)
roles(74-80)users(58-64)write_user_groups(50-56)src/rbac/user.rs (4)
roles(110-112)username(239-241)userid(84-89)userid(243-245)src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
delete_user(58-82)src/rbac/mod.rs (1)
delete_user(83-86)src/handlers/http/cluster/mod.rs (1)
sync_users_with_roles_with_ingestors(224-282)
src/handlers/http/rbac.rs (5)
src/rbac/map.rs (3)
read_user_groups(42-48)roles(74-80)users(58-64)src/rbac/user.rs (4)
roles(110-112)username(239-241)userid(84-89)userid(243-245)src/rbac/utils.rs (1)
to_prism_user(31-85)src/rbac/mod.rs (2)
get_role(76-81)delete_user(83-86)src/handlers/http/modal/ingest/ingestor_rbac.rs (3)
delete_user(58-82)add_roles_to_user(85-133)remove_roles_from_user(136-197)
src/handlers/http/modal/ingest/ingestor_rbac.rs (5)
src/handlers/http/rbac.rs (4)
Users(67-67)delete_user(223-254)add_roles_to_user(257-306)remove_roles_from_user(309-370)src/rbac/map.rs (2)
roles(74-80)users(58-64)src/rbac/user.rs (4)
roles(110-112)userid(84-89)userid(243-245)username(239-241)src/rbac/mod.rs (1)
delete_user(83-86)src/storage/store_metadata.rs (1)
put_staging_metadata(286-301)
⏰ 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 x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (18)
src/rbac/utils.rs (1)
59-73: Group lookups useuserid, but session/permission code still usesusername
get_user_groupsis consistently called withuserideverywhere (utils.rs, mod.rs, handlers)- However, in
src/rbac/map.rsandsrc/rbac/mod.rsthe auth/session APIs still accept and key offusername, notuserid
–users().get(username)(auth check)
–remove_user(&self, username: &str)
–active_sessionsmap entries(username, perms)and callsaggregate_group_permissions(username)- If these should now use
userid(post-refactor), update those APIs and mappings; otherwise, rename parameters to clarify they’re login namessrc/rbac/map.rs (3)
277-279: New Sessions::get_userid accessor aligns with userid-first design.LGTM. Minimal and clear.
288-291: Index Users by userid (good).Insertion path updated to userid as key; consistent with the PR objectives.
296-301: Build Users map keyed by userid (good).The From<Vec> impl matches the new indexing.
src/utils/mod.rs (1)
60-68: Switched to get_userid_from_session (good).This aligns with the userid-first refactor.
src/handlers/http/modal/query_server.rs (3)
212-217: Switched to userid for role read — looks consistentThe route
/{userid}/rolealigns with userid-based flows.
219-227: userid in role add path is correct
PATCH /user/{userid}/role/addmatches the ingest/query syncing contract.
229-237: userid in role remove path is correct
PATCH /user/{userid}/role/removeis consistent with the rest of the PR.src/handlers/http/modal/ingest_server.rs (2)
202-210: userid in role sync add path is correctMatches the querier -> ingestor sync contract.
212-220: userid in role sync remove path is correctConsistent and correct.
src/handlers/http/modal/query/querier_rbac.rs (3)
169-221: Role add flow via userid is correctValidates roles, updates parseable.json and in-memory, and syncs using userid. Looks good.
222-286: Role remove flow via userid is correctChecks presence, updates both metadata and memory, and syncs appropriately.
296-300: Lock around password reset — goodEnsures serialized writes for password updates.
src/rbac/mod.rs (2)
119-140: Good: session-derived userid authorization path.Fetching userid from session and merging group-derived permissions looks correct and aligns with the userid pivot.
150-157: Good: sessions keyed by userid.Session tracking by userid is consistent with the rest of the refactor.
src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
57-82: LGTM: deletion path holds shared lock and updates staging + in-memory tables.Consistent with the shared UPDATE_LOCK strategy.
src/handlers/http/rbac.rs (2)
41-43: LGTM: single shared async UPDATE_LOCK.This resolves prior atomicity issues across modules.
64-77: LGTM: prism list endpoint builds DTOs via to_prism_user.Consistent with userid-based roles and groups aggregation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/handlers/http/rbac.rs (4)
448-469: Fix content-type for JSON error responsesYou’re returning JSON while explicitly setting ContentType::plaintext(), which will mislead clients and break JSON parsing.
Apply:
match self { RBACError::RolesNotAssigned(obj) => actix_web::HttpResponse::build(self.status_code()) - .insert_header(ContentType::plaintext()) - .json(json!({ + .json(json!({ "roles_not_assigned": obj })), RBACError::RolesDoNotExist(obj) => actix_web::HttpResponse::build(self.status_code()) - .insert_header(ContentType::plaintext()) - .json(json!({ + .json(json!({ "non_existent_roles": obj })), RBACError::InvalidUserGroupRequest(obj) => { actix_web::HttpResponse::build(self.status_code()) - .insert_header(ContentType::plaintext()) - .json(obj) + .json(obj) }
260-310: Guard role-add mutations with UPDATE_LOCKadd_roles_to_user mutates metadata and in-memory Users without the global lock; concurrent updates can race and lose changes.
pub async fn add_roles_to_user( userid: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); if !Users.contains(&userid) { return Err(RBACError::UserDoesNotExist); }; // find username by userid, for native users, username is userid, for oauth users, we need to look up let username = if let Some(user) = users().get(&userid) { user.username_by_userid() } else { return Err(RBACError::UserDoesNotExist); }; let mut non_existent_roles = Vec::new(); // check if the role exists for role in &roles_to_add { if !roles().contains_key(role) { non_existent_roles.push(role.clone()); } } if !non_existent_roles.is_empty() { return Err(RBACError::RolesDoNotExist(non_existent_roles)); } + let _guard = UPDATE_LOCK.lock().await; // update parseable.json first let mut metadata = get_metadata().await?; if let Some(user) = metadata .users .iter_mut() .find(|user| user.userid() == userid) { user.roles.extend(roles_to_add.clone()); } else { // should be unreachable given state is always consistent return Err(RBACError::UserDoesNotExist); } put_metadata(&metadata).await?; // update in mem table - Users.add_roles(&userid.clone(), roles_to_add); + Users.add_roles(&userid, roles_to_add); Ok(format!("Roles updated successfully for {username}")) }
312-374: Guard role-remove mutations with UPDATE_LOCKSame race risk as add_roles_to_user; take the lock across metadata and Users updates.
pub async fn remove_roles_from_user( userid: web::Path<String>, roles_to_remove: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); if !Users.contains(&userid) { return Err(RBACError::UserDoesNotExist); }; // find username by userid, for native users, username is userid, for oauth users, we need to look up let username = if let Some(user) = users().get(&userid) { user.username_by_userid() } else { return Err(RBACError::UserDoesNotExist); }; let mut non_existent_roles = Vec::new(); // check if the role exists for role in &roles_to_remove { if !roles().contains_key(role) { non_existent_roles.push(role.clone()); } } if !non_existent_roles.is_empty() { return Err(RBACError::RolesDoNotExist(non_existent_roles)); } // check for role not present with user let user_roles: HashSet<String> = HashSet::from_iter(Users.get_role(&userid)); let roles_not_with_user: HashSet<String> = HashSet::from_iter(roles_to_remove.difference(&user_roles).cloned()); if !roles_not_with_user.is_empty() { return Err(RBACError::RolesNotAssigned(Vec::from_iter( roles_not_with_user, ))); } + let _guard = UPDATE_LOCK.lock().await; // update parseable.json first let mut metadata = get_metadata().await?; if let Some(user) = metadata .users .iter_mut() .find(|user| user.userid() == userid) { let diff: HashSet<String> = HashSet::from_iter(user.roles.difference(&roles_to_remove).cloned()); user.roles = diff; } else { // should be unreachable given state is always consistent return Err(RBACError::UserDoesNotExist); } put_metadata(&metadata).await?; // update in mem table - Users.remove_roles(&userid.clone(), roles_to_remove); + Users.remove_roles(&userid, roles_to_remove); Ok(format!("Roles updated successfully for {username}")) }
83-95: GET /users/{username} uses the wrong key
Theusersmap is keyed by each user’suserid(fromUser::userid()), not the display username, so OAuth users will always 404. Rename the path param to{userid}(and update docs/signature) or iterateusers.values().find(|u| u.username_by_userid()==username).
🧹 Nitpick comments (2)
src/handlers/http/rbac.rs (2)
189-224: Minor: normalize Path usage in get_roleFor consistency/readability, extract Path before use.
-pub async fn get_role(userid: web::Path<String>) -> Result<impl Responder, RBACError> { - if !Users.contains(&userid) { +pub async fn get_role(userid: web::Path<String>) -> Result<impl Responder, RBACError> { + let userid = userid.into_inner(); + if !Users.contains(&userid) { return Err(RBACError::UserDoesNotExist); }; - let direct_roles: HashMap<String, Vec<DefaultPrivilege>> = Users - .get_role(&userid) + let direct_roles: HashMap<String, Vec<DefaultPrivilege>> = Users + .get_role(&userid) .iter() .filter_map(|role_name| { roles() .get(role_name) .map(|role| (role_name.to_owned(), role.clone())) }) .collect(); - for user_group in Users.get_user_groups(&userid) { + for user_group in Users.get_user_groups(&userid) {
472-478: Serde attribute likely intended as rename_all, not renamerename = "camelCase" has no effect on field names; if you want camelCase fields (e.g., groupRoles), use rename_all.
-#[derive(Serialize)] -#[serde(rename = "camelCase")] +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] pub struct RolesResponse { #[serde(rename = "roles")] pub direct_roles: HashMap<String, Vec<DefaultPrivilege>>, pub group_roles: HashMap<String, HashMap<String, Vec<DefaultPrivilege>>>, }Note: This changes group_roles -> groupRoles in responses. Verify API consumers before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/handlers/http/rbac.rs(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/handlers/http/rbac.rs (4)
src/rbac/map.rs (3)
read_user_groups(42-48)roles(74-80)users(58-64)src/rbac/user.rs (4)
roles(110-112)username(239-241)userid(84-89)userid(243-245)src/rbac/mod.rs (2)
get_role(76-81)delete_user(83-86)src/handlers/http/modal/query/querier_rbac.rs (3)
delete_user(105-166)add_roles_to_user(169-220)remove_roles_from_user(223-286)
🪛 GitHub Actions: Lint, Test and Coverage Report
src/handlers/http/rbac.rs
[error] 128-136: Clippy: this if statement can be collapsed (collapsible_if).
⏰ 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). (4)
- 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 Kafka x86_64-unknown-linux-gnu
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/handlers/http/rbac.rs (2)
83-95: Bug:get_prism_userlooks up by userid, not username — OAuth users won’t resolve.The map is keyed by userid; this handler receives a display name. Fall back to searching by
username_by_userid().- let users = rbac::map::users(); - if let Some(user) = users.get(&username) { - // Create UsersPrism for the found user only - let prism_user = to_prism_user(user); - Ok(web::Json(prism_user)) - } else { - Err(RBACError::UserDoesNotExist) - } + let users = rbac::map::users(); + if let Some(user) = users.get(&username) { + Ok(web::Json(to_prism_user(user))) + } else if let Some(user) = users.values().find(|u| u.username_by_userid() == username) { + Ok(web::Json(to_prism_user(user))) + } else { + Err(RBACError::UserDoesNotExist) + }
470-477: Serde container attr should berename_all, notrename.
#[serde(rename = "camelCase")]has no effect on a struct; fields won’t be camelCased.-#[serde(rename = "camelCase")] +#[serde(rename_all = "camelCase")] pub struct RolesResponse {
♻️ Duplicate comments (2)
src/handlers/http/rbac.rs (2)
127-133: Collapse duplicate check inpost_user.Same logical issue as elsewhere; remove the repeated
Users.contains.- let _guard = UPDATE_LOCK.lock().await; - if Users.contains(&username) && Users.contains(&username) - || metadata.users.iter().any(|user| match &user.ty { - UserType::Native(basic) => basic.username == username, - UserType::OAuth(_) => false, // OAuth users should be created differently - }) - { + let _guard = UPDATE_LOCK.lock().await; + if Users.contains(&username) + || metadata.users.iter().any(|user| { + matches!(&user.ty, UserType::Native(basic) if basic.username == username) + }) + { return Err(RBACError::UserExists); }
224-256: Close TOCTOU window indelete_userby holding the lock across the pre-check.Acquire
UPDATE_LOCKbefore checking group membership.pub async fn delete_user(userid: web::Path<String>) -> Result<impl Responder, RBACError> { let userid = userid.into_inner(); - // if user is a part of any groups then don't allow deletion + let _guard = UPDATE_LOCK.lock().await; + // if user is a part of any groups then don't allow deletion if !Users.get_user_groups(&userid).is_empty() { return Err(RBACError::InvalidDeletionRequest(format!( "User: {userid} should not be a part of any groups" ))); } @@ - let _guard = UPDATE_LOCK.lock().await;
🧹 Nitpick comments (3)
src/handlers/http/modal/query/querier_rbac.rs (2)
60-71: Tidy role validation loop and fix “existant” typo.Readability and consistency.
- 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)); + let mut non_existent_roles = Vec::new(); + for r in &user_roles { + if !roles().contains_key(r) { + non_existent_roles.push(r.clone()); + } + } + if !non_existent_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existent_roles)); }
130-135: Avoid shadowing the outeruseridin the deletion loop.Shadowing is confusing during review/debugging.
- let userid = match &user.ty { + let target_userid = match &user.ty { UserType::Native(basic) => basic.username.clone(), UserType::OAuth(oauth) => oauth.userid.clone(), }; - ug.remove_users_by_user_ids(HashSet::from_iter([userid]))?; + ug.remove_users_by_user_ids(HashSet::from_iter([target_userid]))?;src/rbac/user.rs (1)
317-319: Validate by userid assumes handlers resolvedusername+method→ userid.Given the API now accepts
{username, method}for usergroup operations, ensure the HTTP layer resolves to auseridbefore constructingGroupUser; otherwise this validation will falsely flag existing users as non-existent.I can patch the handlers to resolve
username+methodtouseridviausers()lookup and return per-user errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/handlers/http/modal/query/querier_rbac.rs(9 hunks)src/handlers/http/rbac.rs(14 hunks)src/rbac/user.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/handlers/http/modal/query/querier_rbac.rs (5)
src/handlers/http/rbac.rs (4)
Users(71-71)delete_user(225-256)add_roles_to_user(259-308)remove_roles_from_user(311-372)src/rbac/user.rs (5)
roles(110-112)username(239-241)userid(84-89)userid(243-245)new(343-345)src/rbac/map.rs (3)
roles(74-80)users(58-64)write_user_groups(50-56)src/rbac/mod.rs (1)
delete_user(83-86)src/handlers/http/cluster/mod.rs (1)
sync_users_with_roles_with_ingestors(224-282)
src/handlers/http/rbac.rs (4)
src/rbac/map.rs (3)
read_user_groups(42-48)roles(74-80)users(58-64)src/rbac/user.rs (4)
roles(110-112)username(239-241)userid(84-89)userid(243-245)src/rbac/mod.rs (2)
get_role(76-81)delete_user(83-86)src/handlers/http/modal/query/querier_rbac.rs (3)
delete_user(103-161)add_roles_to_user(164-215)remove_roles_from_user(218-281)
src/rbac/user.rs (2)
src/rbac/map.rs (2)
users(58-64)mut_sessions(98-104)src/rbac/mod.rs (1)
is_oauth(68-70)
⏰ 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 Distributed deployments
- 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 Default x86_64-unknown-linux-gnu
- 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: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
🔇 Additional comments (1)
src/handlers/http/rbac.rs (1)
224-256: Inconsistent delete_user handlers – unify behavior
- The HTTP route in
src/handlers/http/rbac.rsblocks deletion if the user belongs to any groups, whereas the querier variant insrc/handlers/http/modal/query/querier_rbac.rscascades group removal and invokessync_user_deletion_with_ingestors.- This duplication risks divergent outcomes depending on which endpoint is called.
Please choose one of the following:
- Remove the basic HTTP handler and consolidate on the cascading variant.
- Extend the basic handler to match the querier’s behavior (remove group memberships + call
sync_user_deletion_with_ingestors).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/rbac/user.rs (2)
218-236: Eq/Hash scoped to userid only — good fix.This prevents duplicate members when display name or method changes. Nice.
69-77: Do not fall back to display name for OAuth userid — use only stable subject.Falling back to
user_info.namecan produce collisions and break identity stability. Only use OIDCsub(or the provideduseridparam as a last resort).- ty: UserType::OAuth(OAuth { - userid: user_info - .sub - .clone() - .or_else(|| user_info.name.clone()) - .unwrap_or(userid), - user_info, - }), + ty: UserType::OAuth(OAuth { + // Use only a stable identifier for userid + userid: user_info.sub.clone().unwrap_or(userid), + user_info, + }),
🧹 Nitpick comments (4)
src/rbac/user.rs (4)
91-104: Prefer clearer naming and add preferred_username fallback; avoid extra clones.
- Consider renaming to
display_username()for intent.- Include
preferred_usernamebefore- Avoid cloning
user_info.- pub fn username_by_userid(&self) -> String { - match &self.ty { - UserType::Native(basic) => basic.username.clone(), - UserType::OAuth(oauth) => { - let user_info = oauth.user_info.clone(); - user_info.name.clone().unwrap_or_else(|| { - user_info - .email - .clone() - .unwrap_or_else(|| oauth.userid.clone()) - }) - } - } - } + pub fn display_username(&self) -> String { + match &self.ty { + UserType::Native(basic) => basic.username.clone(), + UserType::OAuth(oauth) => { + let info = &oauth.user_info; + info.name + .as_deref() + .or(info.preferred_username.as_deref()) + .or(info.email.as_deref()) + .unwrap_or(&oauth.userid) + .to_owned() + } + } + }
238-254: Use typed auth method or constants to avoid stringly-typed comparisons.Replace
"oauth"/"native"literals with:
- An enum
AuthMethodwith serderename_all = "lowercase", or- File-level constants to reduce typo risk.
+const METHOD_OAUTH: &str = "oauth"; +const METHOD_NATIVE: &str = "native"; @@ - pub fn is_oauth(&self) -> bool { - self.method == "oauth" + pub fn is_oauth(&self) -> bool { + self.method == METHOD_OAUTH } @@ - pub fn user_type(&self) -> &str { - if self.is_oauth() { "oauth" } else { "native" } + pub fn user_type(&self) -> &str { + if self.is_oauth() { METHOD_OAUTH } else { METHOD_NATIVE } }
255-277: Reasonable defaults for display name; reuse method constants.Construction from
Userlooks solid; suggest using the method constants (or enum) here too.- method: "native".to_string(), + method: METHOD_NATIVE.to_string(), @@ - GroupUser { + GroupUser { userid: userid.clone(), username: display_username, - method: "oauth".to_string(), + method: METHOD_OAUTH.to_string(), }
316-321: Validation good; consider including display names in error payload.You currently return only userids in
non_existent_users. Includingusernamecould improve DX for API clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/handlers/http/rbac.rs(14 hunks)src/rbac/user.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/rbac.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/rbac/user.rs (2)
src/rbac/map.rs (2)
users(58-64)mut_sessions(98-104)src/rbac/mod.rs (1)
is_oauth(68-70)
⏰ 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 x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (8)
src/rbac/user.rs (8)
84-89: Getter reads well; return-by-ref is correct.
userid()uses a stable key across user types with zero copy. LGTM.
343-345: Constructor aligns with new model.Signature and field init are coherent. LGTM.
353-355: Session invalidation on role changes — good.Ensures permissions refresh. LGTM.
384-386: Session refresh on role removal — good.Behavior is consistent with add/remove roles. LGTM.
390-406: Set ops for removal are correct and dedupe-safe.Intersection to compute
removed_users+ session refresh before replacing set looks good.
408-412: Helper is fine.Efficient projection of userids. LGTM.
413-419: Nice convenience API.
from_userusage avoids call-site duplication. LGTM.
420-435: Removal by id is clean and leverages Eq/Hash semantics.Good use of filter + cloned set. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/handlers/http/rbac.rs (2)
262-307: Add UPDATE_LOCK to serialize role-add mutations.Align with other mutating endpoints; prevents concurrent lost updates.
pub async fn add_roles_to_user( userid: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); + let _guard = UPDATE_LOCK.lock().await;
314-371: Add UPDATE_LOCK to serialize role-remove mutations.Same rationale as add.
pub async fn remove_roles_from_user( userid: web::Path<String>, roles_to_remove: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); + let _guard = UPDATE_LOCK.lock().await;
🧹 Nitpick comments (8)
src/rbac/user.rs (3)
87-99: Preferpreferred_username->name->Small UX tweak to follow common OIDC precedence and avoid extra clone.
Apply:
- let user_info = oauth.user_info.clone(); - user_info.name.clone().unwrap_or_else(|| { - user_info - .email - .clone() - .unwrap_or_else(|| oauth.userid.clone()) - }) + let user_info = &oauth.user_info; + user_info + .preferred_username + .clone() + .or_else(|| user_info.name.clone()) + .or_else(|| user_info.email.clone()) + .unwrap_or_else(|| oauth.userid.clone())
243-249: Normalize method checks; avoid case/typo sensitivity.
is_oauth()currently relies on exact string match. Make it resilient to input variants.- pub fn is_oauth(&self) -> bool { - self.method == "oauth" - } + pub fn is_oauth(&self) -> bool { + self.method.eq_ignore_ascii_case("oauth") + }Optionally, centralize method literals (
const METHOD_OAUTH: &str = "oauth"; const METHOD_NATIVE: &str = "native";) to avoid typos across modules.
251-273: Usepreferred_usernamefirst when deriving display name for OAuth.Aligns with typical IdP semantics.
- let display_username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .unwrap_or_else(|| userid.clone()); // fallback to userid if nothing else available + let display_username = user_info + .preferred_username + .clone() + .or_else(|| user_info.name.clone()) + .or_else(|| user_info.email.clone()) + .unwrap_or_else(|| userid.clone()); // fallback to userid if nothing else availablesrc/handlers/http/modal/query/querier_rbac.rs (3)
73-81: Remove duplicate existence check (also satisfies Clippy).Condition repeats
Users.contains(&username)and obscures intent.- let _guard = UPDATE_LOCK.lock().await; - if Users.contains(&username) && Users.contains(&username) - || metadata.users.iter().any(|user| match &user.ty { - UserType::Native(basic) => basic.username == username, - UserType::OAuth(_) => false, // OAuth users should be created differently - }) - { + let _guard = UPDATE_LOCK.lock().await; + if Users.contains(&username) + || metadata.users.iter().any(|user| { + matches!(&user.ty, UserType::Native(basic) if basic.username == username) + }) + { return Err(RBACError::UserExists); }
127-153: Nit: avoid redundant user lookup and branching; use the already-known userid.You already have the canonical
userid; reuse it instead of re-deriving viamatch.- let userid = match &user.ty { - UserType::Native(basic) => basic.username.clone(), - UserType::OAuth(oauth) => oauth.userid.clone(), - }; - ug.remove_users_by_user_ids(HashSet::from_iter([userid]))?; + let uid = userid.clone(); + ug.remove_users_by_user_ids(HashSet::from_iter([uid]))?;
182-194: Spelling/consistency: usenon_existent_roles.Minor naming fix; improves readability and keeps error messages consistent across modules.
- let mut non_existant_roles = Vec::new(); + let mut non_existent_roles = Vec::new(); @@ - if !non_existant_roles.is_empty() { - return Err(RBACError::RolesDoNotExist(non_existant_roles)); + if !non_existent_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existent_roles));Apply similarly in
remove_roles_from_user.Also applies to: 236-247
src/handlers/http/rbac.rs (2)
127-135: Collapse duplicate existence check.Same issue here—double
Users.contains(&username).- let _guard = UPDATE_LOCK.lock().await; - if Users.contains(&username) && Users.contains(&username) - || metadata.users.iter().any(|user| match &user.ty { - UserType::Native(basic) => basic.username == username, - UserType::OAuth(_) => false, // OAuth users should be created differently - }) - { + let _guard = UPDATE_LOCK.lock().await; + if Users.contains(&username) + || metadata.users.iter().any(|user| { + matches!(&user.ty, UserType::Native(basic) if basic.username == username) + }) + { return Err(RBACError::UserExists); }
470-475: Incorrect serde attribute; userename_allor drop it.
#[serde(rename = "camelCase")]renames the type, not fields. If the intent is camelCase fields, switch torename_all. Otherwise, remove it.-#[serde(rename = "camelCase")] +#[serde(rename_all = "camelCase")] pub struct RolesResponse { #[serde(rename = "roles")] pub direct_roles: HashMap<String, Vec<DefaultPrivilege>>, pub group_roles: HashMap<String, HashMap<String, Vec<DefaultPrivilege>>>, }If clients expect
group_roles(snake_case), keep the current field name by removing the struct-level attribute instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/handlers/http/modal/query/querier_rbac.rs(9 hunks)src/handlers/http/rbac.rs(14 hunks)src/rbac/user.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T06:46:24.562Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1422
File: src/rbac/user.rs:359-369
Timestamp: 2025-09-05T06:46:24.562Z
Learning: In the Parseable codebase, user existence validation for UserGroup operations is handled by the caller methods, not within the UserGroup methods themselves. The add_users() method in UserGroup trusts that callers have already validated user existence.
Applied to files:
src/rbac/user.rs
🧬 Code graph analysis (3)
src/handlers/http/rbac.rs (6)
src/rbac/map.rs (3)
read_user_groups(42-48)roles(74-80)users(58-64)src/rbac/user.rs (4)
roles(106-108)username(235-237)userid(80-85)userid(239-241)src/rbac/mod.rs (2)
get_role(76-81)delete_user(83-86)src/handlers/http/modal/query/querier_rbac.rs (3)
delete_user(103-161)add_roles_to_user(164-215)remove_roles_from_user(218-281)src/handlers/http/modal/ingest/ingestor_rbac.rs (3)
delete_user(58-82)add_roles_to_user(85-133)remove_roles_from_user(136-197)src/handlers/http/role.rs (2)
get_metadata(143-151)put_metadata(153-157)
src/handlers/http/modal/query/querier_rbac.rs (4)
src/handlers/http/rbac.rs (4)
Users(71-71)delete_user(226-255)add_roles_to_user(258-307)remove_roles_from_user(310-371)src/rbac/user.rs (5)
roles(106-108)username(235-237)userid(80-85)userid(239-241)new(339-341)src/rbac/map.rs (3)
roles(74-80)users(58-64)write_user_groups(50-56)src/handlers/http/cluster/mod.rs (2)
sync_user_deletion_with_ingestors(285-322)sync_users_with_roles_with_ingestors(224-282)
src/rbac/user.rs (2)
src/rbac/map.rs (2)
users(58-64)mut_sessions(98-104)src/rbac/mod.rs (1)
is_oauth(68-70)
⏰ 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 Default 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 Kafka x86_64-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 (7)
src/rbac/user.rs (3)
69-78: Good: OAuth userid uses stable OIDCsub.Using
user_info.sub.clone().unwrap_or(userid)avoids falling back to display fields and keeps identity stable.
80-86: LGTM: unified userid accessor.The accessor correctly returns the canonical ID for both native and OAuth users.
214-233: Correct: Eq/Hash by userid only prevents duplicates on display-name change.Manual
PartialEq/Eq/Hashscoped touseridis the right call.src/handlers/http/modal/query/querier_rbac.rs (1)
30-36: Good: reusing shared UPDATE_LOCK from rbac.This removes duplicate locks and preserves atomicity across modules.
src/handlers/http/rbac.rs (3)
45-46: Good: single, shared async UPDATE_LOCK.Exposing a shared lock point is the right primitive for serializing metadata + in-memory updates.
185-193: Fix validated: path param extracted before use.
let userid = userid.into_inner();resolves the earlier type mismatch.
225-255: Good: lock held across membership check and deletion.This closes the TOCTOU window during user deletion.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/http/modal/query/querier_rbac.rs (1)
73-100: Release UPDATE_LOCK before network calls and nested handler to avoid deadlocks/lock contention.Lock is held across put_metadata (await), sync_user_creation_with_ingestors (await), and a nested call to add_roles_to_user. If add_roles_to_user ever acquires UPDATE_LOCK (it should), this can deadlock. Narrow the critical section and avoid awaiting under the lock.
Apply:
@@ - let _guard = UPDATE_LOCK.lock().await; - if Users.contains(&username) + // Prepare user before locking to allow releasing the lock prior to network calls + let (user, password) = user::User::new_basic(username.clone()); + let created_role = user_roles.clone(); + + { + let _guard = UPDATE_LOCK.lock().await; + if Users.contains(&username) || metadata .users .iter() .any(|user| matches!(&user.ty, UserType::Native(basic) if basic.username == username)) { return Err(RBACError::UserExists); } - let (user, password) = user::User::new_basic(username.clone()); - metadata.users.push(user.clone()); - put_metadata(&metadata).await?; - let created_role = user_roles.clone(); - Users.put_user(user.clone()); + metadata.users.push(user.clone()); + put_metadata(&metadata).await?; + Users.put_user(user.clone()); + } // release UPDATE_LOCK before external I/O and nested handler - sync_user_creation_with_ingestors(user, &Some(user_roles)).await?; + sync_user_creation_with_ingestors(user, &Some(user_roles)).await?; add_roles_to_user( web::Path::<String>::from(username.clone()), web::Json(created_role), ) .await?;
♻️ Duplicate comments (1)
src/handlers/http/modal/query/querier_rbac.rs (1)
168-181: Acquire UPDATE_LOCK while mutating metadata and Users in add_roles_to_user.Without the lock, concurrent add/remove can cause lost updates in parseable.json and in-memory Users.
pub async fn add_roles_to_user( userid: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); + let _guard = UPDATE_LOCK.lock().await;Note: With the change above, ensure post_user releases UPDATE_LOCK before calling add_roles_to_user (see earlier diff).
🧹 Nitpick comments (3)
src/handlers/http/modal/query/querier_rbac.rs (3)
124-140: Avoid repeated locks/lookups; compute member_id once and hold the user-group write guard once.We fetch the same user repeatedly and acquire/release the user-group write guard inside the loop. This increases contention and the else branch is unreachable while UPDATE_LOCK is held.
- let user_groups = Users.get_user_groups(&userid); - let mut groups_to_update = Vec::new(); - for user_group in user_groups { - if let Some(ug) = write_user_groups().get_mut(&user_group) - && let Some(user) = users().get(&userid) - { - let userid = match &user.ty { - UserType::Native(basic) => basic.username.clone(), - UserType::OAuth(oauth) => oauth.userid.clone(), - }; - ug.remove_users_by_user_ids(HashSet::from_iter([userid]))?; - groups_to_update.push(ug.clone()); - } else { - // User not found, skip or log as needed - continue; - } - } + let user_groups = Users.get_user_groups(&userid); + let member_id = users() + .get(&userid) + .map(|u| u.userid().to_string()) + .ok_or(RBACError::UserDoesNotExist)?; + let mut groups_to_update = Vec::new(); + { + let mut ug_guard = write_user_groups(); + for group_name in user_groups { + if let Some(ug) = ug_guard.get_mut(&group_name) { + ug.remove_users_by_user_ids(HashSet::from([member_id.clone()]))?; + groups_to_update.push(ug.clone()); + } + } + }
60-71: Spelling nit: non_existant_roles → non_existent_roles.Pure readability; no behavior change.
- let mut non_existant_roles = Vec::new(); + let mut non_existent_roles = Vec::new(); @@ - if !non_existant_roles.is_empty() { - return Err(RBACError::RolesDoNotExist(non_existant_roles)); + if !non_existent_roles.is_empty() { + return Err(RBACError::RolesDoNotExist(non_existent_roles));Also applies to: 182-189, 239-246
102-161: Optional: avoid awaiting under UPDATE_LOCK throughout handlers.Shorten lock scopes to cover only state mutations; perform network syncs after releasing the lock to improve throughput and reduce risk of future deadlocks.
Also applies to: 163-215, 217-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/handlers/http/modal/query/querier_rbac.rs(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/handlers/http/modal/query/querier_rbac.rs (5)
src/rbac/user.rs (5)
roles(106-108)username(235-237)userid(80-85)userid(239-241)new(339-341)src/rbac/map.rs (3)
roles(74-80)users(58-64)write_user_groups(50-56)src/handlers/http/modal/ingest/ingestor_rbac.rs (3)
delete_user(58-82)add_roles_to_user(85-133)remove_roles_from_user(136-197)src/rbac/mod.rs (1)
delete_user(83-86)src/handlers/http/cluster/mod.rs (2)
sync_user_deletion_with_ingestors(285-322)sync_users_with_roles_with_ingestors(224-282)
⏰ 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-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: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-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 (3)
src/handlers/http/modal/query/querier_rbac.rs (3)
30-36: Imports look correct and align with the userid-centered model.Bringing in UPDATE_LOCK, users(), and UserType is consistent with the refactor direction.
225-283: Good: Locking present during role removal.UPDATE_LOCK is taken; metadata and Users updates are consistent.
293-316: Password reset flow is consistent.Locking, metadata update, in-memory update, and cluster sync are in the right order.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/http/rbac.rs (1)
309-371: Add UPDATE_LOCK earlier in remove_roles_from_user.The UPDATE_LOCK should be acquired before any checks that could lead to mutations, not in the middle of the function logic. This ensures atomicity throughout the operation.
Apply this diff to move the lock acquisition:
pub async fn remove_roles_from_user( userid: web::Path<String>, roles_to_remove: web::Json<HashSet<String>>, ) -> Result<impl Responder, RBACError> { let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); + let _guard = UPDATE_LOCK.lock().await; if !Users.contains(&userid) {
♻️ Duplicate comments (4)
src/handlers/http/rbac.rs (1)
127-136: Fix the redundantUsers.contains(&username)check.The condition has
Users.contains(&username) && Users.contains(&username)which is redundant.Apply this diff to fix the redundancy:
- if Users.contains(&username) && Users.contains(&username) - || metadata.users.iter().any(|user| match &user.ty { - UserType::Native(basic) => basic.username == username, - UserType::OAuth(_) => false, // OAuth users should be created differently - }) + if Users.contains(&username) + || metadata.users.iter().any(|user| match &user.ty { + UserType::Native(basic) => basic.username == username, + UserType::OAuth(_) => false, // OAuth users should be created differently + })src/handlers/http/modal/query/querier_rbac.rs (3)
163-215: Hold UPDATE_LOCK and fix sync call in add_roles_to_user.Two issues:
- Missing UPDATE_LOCK for atomic operations
- Sync API expects username, not userid
Apply this diff to fix both issues:
pub async fn add_roles_to_user( userid: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<String, RBACError> { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); + let _guard = UPDATE_LOCK.lock().await; if !Users.contains(&userid) { // ... existing code ... - sync_users_with_roles_with_ingestors(&userid, &roles_to_add, "add").await?; + sync_users_with_roles_with_ingestors(&username, &roles_to_add, "add").await?;
217-283: Fix sync call in remove_roles_from_user.The sync API expects username but the code passes userid, which will fail for OAuth users.
Apply this diff to fix the sync call:
- sync_users_with_roles_with_ingestors(&userid, &roles_to_remove, "remove").await?; + sync_users_with_roles_with_ingestors(&username, &roles_to_remove, "remove").await?;
156-157: Sync with ingestors uses userid but the API expects username.Based on the codebase analysis,
sync_user_deletion_with_ingestorsexpects a username parameter and constructs the endpoint as/user/{username}/sync. Passing userid will fail for OAuth users where userid != username.Apply this diff to fix the parameter:
- sync_user_deletion_with_ingestors(&userid).await?; + sync_user_deletion_with_ingestors(&username).await?;
🧹 Nitpick comments (1)
src/handlers/http/rbac.rs (1)
257-307: Add UPDATE_LOCK for consistency in add_roles_to_user.While the current implementation may work, for consistency with other mutation operations and to prevent potential race conditions, consider acquiring the UPDATE_LOCK.
Apply this diff to add the lock:
pub async fn add_roles_to_user( userid: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<impl Responder, RBACError> { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); + let _guard = UPDATE_LOCK.lock().await; if !Users.contains(&userid) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/handlers/http/modal/ingest/ingestor_rbac.rs(8 hunks)src/handlers/http/modal/query/querier_rbac.rs(10 hunks)src/handlers/http/rbac.rs(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/handlers/http/modal/query/querier_rbac.rs (5)
src/handlers/http/rbac.rs (4)
Users(71-71)delete_user(226-255)add_roles_to_user(258-307)remove_roles_from_user(310-371)src/rbac/user.rs (5)
roles(106-108)username(235-237)userid(80-85)userid(239-241)new(339-341)src/rbac/map.rs (3)
roles(74-80)users(58-64)write_user_groups(50-56)src/rbac/mod.rs (1)
delete_user(83-86)src/handlers/http/cluster/mod.rs (2)
sync_user_deletion_with_ingestors(285-322)sync_users_with_roles_with_ingestors(224-282)
src/handlers/http/modal/ingest/ingestor_rbac.rs (6)
src/handlers/http/role.rs (1)
get_metadata(143-151)src/storage/object_storage.rs (1)
get_metadata(620-636)src/handlers/http/rbac.rs (4)
Users(71-71)delete_user(226-255)add_roles_to_user(258-307)remove_roles_from_user(310-371)src/rbac/user.rs (4)
roles(106-108)userid(80-85)userid(239-241)username(235-237)src/rbac/map.rs (2)
roles(74-80)users(58-64)src/storage/store_metadata.rs (1)
put_staging_metadata(286-301)
src/handlers/http/rbac.rs (6)
src/rbac/map.rs (3)
read_user_groups(42-48)roles(74-80)users(58-64)src/rbac/user.rs (4)
roles(106-108)username(235-237)userid(80-85)userid(239-241)src/rbac/utils.rs (1)
to_prism_user(31-85)src/rbac/mod.rs (2)
get_role(76-81)delete_user(83-86)src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
delete_user(58-82)src/handlers/http/modal/query/querier_rbac.rs (1)
delete_user(103-161)
⏰ 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 Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (18)
src/handlers/http/modal/ingest/ingestor_rbac.rs (8)
21-30: LGTM! Clean refactoring to use shared UPDATE_LOCK.The migration to use the shared UPDATE_LOCK from the rbac module aligns well with the centralized locking strategy, ensuring consistent atomic operations across RBAC handlers.
57-65: LGTM! Proper lock acquisition order prevents race conditions.The lock is acquired early (Line 60) before checking user existence, which prevents race conditions where a user could be deleted after the check but before acquiring the lock. This is the correct pattern.
66-72: LGTM! Good approach for userid-to-username resolution.The code properly handles both native users (where userid == username) and OAuth users (where they differ) by using the
username_by_userid()method. This ensures correct display names in response messages.
73-82: LGTM! Consistent userid-based operations.The changes correctly use userid for both metadata filtering and in-memory state deletion, maintaining consistency throughout the system. The response includes the human-readable username which improves user experience.
84-102: LGTM! Comprehensive role existence validation.The function properly validates all requested roles before making any changes, collecting non-existent roles to provide informative error messages. The userid-based lookups and username resolution for display are correctly implemented.
115-132: LGTM! Consistent userid-based role management.The metadata update correctly uses userid for finding users, and the in-memory update maintains consistency. Good practice to use
clone()for the userid parameter to avoid moving it.
153-196: LGTM! Thorough role removal validation.The function properly:
- Validates role existence
- Verifies the user actually has the roles to be removed
- Updates both metadata and in-memory state using userid
- Returns an appropriate message with the username for clarity
198-221: LGTM! Consistent response type change.The password generation handler correctly returns HttpResponse with JSON payload, maintaining consistency with other endpoints in the file.
src/handlers/http/rbac.rs (5)
45-46: LGTM! Well-structured centralized locking.Making UPDATE_LOCK public at the crate level (
pub(crate)) is the right approach for sharing it across RBAC modules while maintaining encapsulation.
54-66: LGTM! Consistent userid-based identity model.The User struct correctly uses
userid()for the id field, which properly handles both native users (userid == username) and OAuth users (userid != username).
83-95: LGTM! Well-implemented user lookup endpoint.The new
get_prism_userendpoint correctly validates user existence and returns enriched user information using theto_prism_userutility.
225-254: LGTM! Well-structured userid-based deletion.The delete_user handler correctly:
- Acquires the lock early to prevent TOCTOU issues
- Checks group membership before allowing deletion
- Resolves userid to username for display
- Returns a JSON response with the human-readable username
373-382: LGTM! Comprehensive InvalidUserGroupError structure.The new fields (
non_existent_users,roles_not_in_group,users_not_in_group) and camelCase serialization align well with the PR's objective to enhance usergroup handling with userid/username/method.src/handlers/http/modal/query/querier_rbac.rs (5)
73-81: LGTM! Proper lock acquisition and validation.The lock is correctly acquired before any mutations, and the user existence check properly handles both in-memory and metadata validation to ensure consistency.
102-118: LGTM! Proper userid-based deletion with username resolution.The handler correctly:
- Holds the lock throughout the operation
- Validates user existence
- Resolves userid to username for display purposes
- Properly cleans up metadata
123-140: LGTM! Proper userid extraction for group removal.The code correctly extracts the userid/username based on user type (Native vs OAuth) before calling
remove_users_by_user_ids, which resolves the issue from previous reviews where OAuth users weren't being properly removed from groups.
142-154: LGTM! Efficient group update logic.The code efficiently updates existing groups in-place or appends new ones as needed, maintaining consistency in the metadata structure.
285-316: LGTM! Proper password reset with sync.The password generation handler correctly:
- Acquires the lock before mutations
- Updates both metadata and in-memory state
- Syncs with ingestors using the correct username parameter
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/handlers/http/modal/ingest/ingestor_rbac.rs (3)
45-52: Bug: roles added under username key; must use userid.For OAuth users,
username != userid. Adding roles withusernamecorrupts in-memory state. Also consider serializing mutations withUPDATE_LOCK.- let metadata = get_metadata().await?; - if let Some(body) = body { - let user: ParseableUser = serde_json::from_value(body.into_inner())?; - let _ = storage::put_staging_metadata(&metadata); - let created_role = user.roles.clone(); - Users.put_user(user.clone()); - Users.add_roles(&username, created_role.clone()); - } + let metadata = get_metadata().await?; + if let Some(body) = body { + let user: ParseableUser = serde_json::from_value(body.into_inner())?; + let userid = user.userid().to_string(); + let _guard = UPDATE_LOCK.lock().await; + Users.put_user(user.clone()); + Users.add_roles(&userid, user.roles.clone()); + // Optional: log staging errors instead of silently discarding + if let Err(e) = storage::put_staging_metadata(&metadata) { + tracing::warn!("failed to write staging metadata: {e}"); + } + }
77-118: Serialize role-add updates and avoid clones.
- Acquire
UPDATE_LOCKto make the check/update atomic.&useridsuffices; avoid cloning.- Consider logging
put_staging_metadatafailures.pub async fn add_roles_to_user( userid: web::Path<String>, roles_to_add: web::Json<HashSet<String>>, ) -> Result<HttpResponse, RBACError> { - let userid = userid.into_inner(); + let userid = userid.into_inner(); + let _guard = UPDATE_LOCK.lock().await; @@ - let _ = storage::put_staging_metadata(&metadata); + if let Err(e) = storage::put_staging_metadata(&metadata) { + tracing::warn!("failed to write staging metadata: {e}"); + } // update in mem table - Users.add_roles(&userid.clone(), roles_to_add.clone()); + Users.add_roles(&userid, roles_to_add.clone());
121-175: Serialize role-remove updates; minor cleanups.
- Acquire
UPDATE_LOCKfor atomicity with metadata + in-memory updates.- Avoid cloning
userid.- Log staging errors rather than discarding.
pub async fn remove_roles_from_user( userid: web::Path<String>, roles_to_remove: web::Json<HashSet<String>>, ) -> Result<HttpResponse, RBACError> { - let userid = userid.into_inner(); + let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); + let _guard = UPDATE_LOCK.lock().await; @@ - let _ = storage::put_staging_metadata(&metadata); + if let Err(e) = storage::put_staging_metadata(&metadata) { + tracing::warn!("failed to write staging metadata: {e}"); + } // update in mem table - Users.remove_roles(&userid.clone(), roles_to_remove.clone()); + Users.remove_roles(&userid, roles_to_remove.clone());
♻️ Duplicate comments (1)
src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
120-120: Fix comment: this is the remove-roles handler.-// Handler PATCH /user/{userid}/role/sync/add => Add roles to a user +// Handler PATCH /user/{userid}/role/sync/remove => Remove roles from a user
🧹 Nitpick comments (3)
src/handlers/http/modal/ingest/ingestor_rbac.rs (3)
21-22: Drop redundant StatusCode usage and the import.
HttpResponse::Ok()already sets 200;.status(StatusCode::OK)is unnecessary. Removing it also lets you drop thehttp::StatusCodeimport.- use http::StatusCode; + // use http::StatusCode; // no longer needed - Ok(HttpResponse::Ok().status(StatusCode::OK).finish()) + Ok(HttpResponse::Ok().finish())Also applies to: 54-55, 74-75, 117-118, 174-175, 199-200
57-75: Deletion path looks correct; handle staging write errors.You correctly switch to
useridand holdUPDATE_LOCK. Please don’t discard errors fromput_staging_metadata—log or propagate them to avoid silent desyncs.- let _ = storage::put_staging_metadata(&metadata); + if let Err(e) = storage::put_staging_metadata(&metadata) { + tracing::warn!("failed to write staging metadata: {e}"); + }
179-200: Password sync: scope and resilience.Assuming this ingestor endpoint “pulls” the latest hash from metadata (generated by the querier), the logic is fine. Add
UPDATE_LOCKand don’t ignore staging write errors. Optional: renamenew_hashtopassword_hashto avoid confusion.pub async fn post_gen_password(username: web::Path<String>) -> Result<HttpResponse, RBACError> { let username = username.into_inner(); - let mut new_hash = String::default(); + let mut new_hash = String::default(); let mut metadata = get_metadata().await?; - let _ = storage::put_staging_metadata(&metadata); + let _guard = UPDATE_LOCK.lock().await; + if let Err(e) = storage::put_staging_metadata(&metadata) { + tracing::warn!("failed to write staging metadata: {e}"); + } @@ Users.change_password_hash(&username, &new_hash); Ok(HttpResponse::Ok().finish()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/handlers/http/modal/ingest/ingestor_rbac.rs(9 hunks)src/handlers/http/modal/ingest_server.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/modal/ingest_server.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/handlers/http/modal/ingest/ingestor_rbac.rs (7)
src/handlers/http/role.rs (1)
get_metadata(143-151)src/handlers/http/oidc.rs (1)
get_metadata(445-453)src/storage/object_storage.rs (1)
get_metadata(620-636)src/handlers/http/modal/query/querier_rbac.rs (4)
delete_user(103-161)add_roles_to_user(164-215)remove_roles_from_user(218-283)post_gen_password(287-316)src/rbac/mod.rs (1)
delete_user(83-86)src/rbac/user.rs (3)
userid(80-85)userid(239-241)username(235-237)src/storage/store_metadata.rs (1)
put_staging_metadata(286-301)
⏰ 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 x86_64-unknown-linux-gnu
- 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 x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/handlers/http/cluster/mod.rs (2)
239-251: Percent-encode userid when embedding in the URL path.OAuth/user IDs can contain reserved chars (e.g.,
|,@,+, spaces). Without percent-encoding, requests can hit the wrong route or 404. Encode the path segment before formatting the URL.Apply:
- let userid = userid.to_owned(); + let userid = urlencoding::encode(userid).into_owned(); ... let url = format!( "{}{}/user/{}/role/sync/{}", ingestor.domain_name, base_path_without_preceding_slash(), - userid, + userid, op );No import is needed if you call
urlencoding::encode(...)fully-qualified. Ensureurlencoding = "2"is in Cargo.toml.
239-251: Encodeuseridin all URL path segments
Interpolating rawuseridintoformat!risks invalid or malicious URLs—percent-encode the segment (e.g., via thepercent_encodingcrate or URL builder).
Affected occurrences in src/handlers/http/cluster/mod.rs:
- lines 243–247 (
.../user/{}/role/sync/{})- lines 288–292 (
.../user/{}/sync)- lines 343–347 (
.../user/{}/sync)- lines 387–391 (
.../user/{}/generate-new-password/sync)
🧹 Nitpick comments (1)
src/handlers/http/cluster/mod.rs (1)
234-237: Minor: avoid needless clone on role serialization.
to_vec(&role.clone())allocates unnecessarily.- let role_data = to_vec(&role.clone()).map_err(|err| { + let role_data = to_vec(role).map_err(|err| {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/handlers/http/cluster/mod.rs(5 hunks)src/handlers/http/modal/ingest/ingestor_rbac.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/modal/ingest/ingestor_rbac.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/handlers/http/cluster/mod.rs (3)
src/rbac/user.rs (2)
userid(80-85)userid(239-241)src/handlers/http/modal/mod.rs (2)
domain_name(572-572)domain_name(579-581)src/handlers/http/mod.rs (1)
base_path_without_preceding_slash(78-80)
⏰ 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 x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- 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: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (1)
src/handlers/http/cluster/mod.rs (1)
225-232: Approved: PATCH routes for role sync confirmed
Ingestor-side routes for/role/sync/addand/role/sync/removeuseweb::patch().
new structure for POST /usergroup
new structure for add/remove user - PATCH /usergroup/{name}/add(remove)
Summary by CodeRabbit
New Features
Refactor
Documentation