chore: allow user to be created without role #1428
Conversation
this is helpful when an admin wants to create a user and assign group to it
WalkthroughAllows creating users without roles by treating missing bodies as empty sets, updates role validation to collect non-existent roles, conditionally assigns roles post-creation, and changes RBACError::UserExists to carry the username. Updates comments to reflect POST semantics in modal server and query server. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant HTTP Server as HTTP Server (/user/{username})
participant RBAC as RBAC Handler
participant Store as Querier/Storage
Note over Client,HTTP Server: POST /user/{username} (body may be absent)
Client->>HTTP Server: POST username [+ optional roles]
HTTP Server->>RBAC: post_user(username, body)
alt Body missing
RBAC->>RBAC: Treat roles = {}
end
RBAC->>Store: Check user exists(username)
alt User exists
Store-->>RBAC: Exists
RBAC-->>HTTP Server: Err UserExists(username)
HTTP Server-->>Client: 4xx RBACError::UserExists(username)
else Not exists
Store-->>RBAC: Not found
RBAC->>Store: Validate roles (lookup each)
alt Any role missing
Store-->>RBAC: Missing role(s)
RBAC-->>HTTP Server: Err RolesDoNotExist([...])
HTTP Server-->>Client: 4xx error
else All roles valid (or none provided)
RBAC->>Store: Create user(username)
alt roles not empty
RBAC->>Store: add_roles_to_user(username, roles)
else roles empty
Note right of RBAC: Skip role assignment
end
RBAC-->>HTTP Server: Ok
HTTP Server-->>Client: 2xx Created
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/handlers/http/modal/query_server.rs (1)
197-206: Doc fix matches actual route; minor comment inconsistency nearbyPOST annotation now matches
web::post(). In the same resource block, the delete comment says{userid}while the path segment is{username}; consider harmonizing to avoid confusion.- // DELETE /user/{userid} => Delete a user + // DELETE /user/{username} => Delete a usersrc/handlers/http/modal/query/querier_rbac.rs (4)
51-55: Prefer typed JSON extractor overserde_json::ValueUsing
Option<web::Json<HashSet<String>>>avoids manual deserialization and yields consistent 400s on invalid payloads.-pub async fn post_user( - username: web::Path<String>, - body: Option<web::Json<serde_json::Value>>, -) -> Result<impl Responder, RBACError> { +pub async fn post_user( + username: web::Path<String>, + body: Option<web::Json<HashSet<String>>>, +) -> Result<impl Responder, RBACError> { @@ - let user_roles: HashSet<String> = if let Some(body) = body { - serde_json::from_value(body.into_inner())? - } else { - HashSet::new() - }; + let user_roles: HashSet<String> = body.map(|b| b.into_inner()).unwrap_or_default();
57-66: Avoid repeated lock acquisitions; take the roles map once
roles()returns a read guard; grabbing it once is cheaper and clearer.- let mut non_existent_roles = Vec::new(); - for role in &user_roles { - if !roles().contains_key(role) { - non_existent_roles.push(role.clone()); - } - } + let mut non_existent_roles = Vec::new(); + let roles_map = roles(); + for role in &user_roles { + if !roles_map.contains_key(role) { + non_existent_roles.push(role.clone()); + } + }Note: elsewhere in this file,
non_existant_rolesis misspelled; consider standardizing tonon_existent_rolesfor consistency.
66-86: Don’t holdUPDATE_LOCKacross network/sync callsThe lock is held while calling
sync_user_creation_with_ingestors(and the subsequent role add), which can block unrelated updates. Scope the lock strictly to metadata/in-memory mutations.- let _guard = UPDATE_LOCK.lock().await; - if Users.contains(&username) + // critical section: check & persist + { + 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(username)); - } - - 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()); + { + return Err(RBACError::UserExists(username)); + } + metadata.users.push(user.clone()); + put_metadata(&metadata).await?; + Users.put_user(user.clone()); + } // lock released before outbound callsOptional: to avoid double-sync, consider populating initial roles in
metadatabefore the first sync (or introduce an internal helper that updates roles without emitting a secondary “add” sync when invoked from user creation).
85-91: Initial role assignment path duplicates sync workYou sync creation with roles, then call
add_roles_to_user(which syncs again). If idempotency isn’t guaranteed, this can be noisy. Either:
- include roles in the created
userbefore persisting and skip the add path, or- add a non-syncing internal role-attach helper for creation.
src/handlers/http/rbac.rs (5)
114-123: Avoid repeated role-map lock acquisitions.
roles()returns a read guard; calling it inside the loop re-acquires the lock repeatedly. Cache it once and compute missing roles idiomatically.- let mut non_existent_roles = Vec::new(); - for role in &user_roles { - if !roles().contains_key(role) { - non_existent_roles.push(role.clone()); - } - } + let roles_map = roles(); + let non_existent_roles: Vec<String> = user_roles + .iter() + .filter(|r| !roles_map.contains_key(*r)) + .cloned() + .collect();
124-129: Redundant condition; simplify for clarity.
Users.contains(&username) && Users.contains(&username)is equivalent to a single call and needlessly duplicates the check.- if Users.contains(&username) && 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 })
140-146: Don’t call an HTTP handler from another handler; extract a shared service.This works but couples handler-to-handler and forces Path/Json wrapping. Prefer a small internal function (e.g.,
assign_roles_to_user(userid: &str, roles: &HashSet<String>) -> Result<(), RBACError>) that both endpoints call.
422-423: Consider HTTP 409 Conflict for “user exists”.409 is more semantically accurate than 400 for duplicates.
- Self::UserExists(_) => StatusCode::BAD_REQUEST, + Self::UserExists(_) => StatusCode::CONFLICT,
466-472: serde attribute likely incorrect; userename_all.
#[serde(rename = "camelCase")]won’t camelCase fields; it renames the type. Userename_all.-#[serde(rename = "camelCase")] +#[serde(rename_all = "camelCase")] pub struct RolesResponse {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/handlers/http/modal/query/querier_rbac.rs(2 hunks)src/handlers/http/modal/query_server.rs(1 hunks)src/handlers/http/modal/server.rs(1 hunks)src/handlers/http/rbac.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/handlers/http/rbac.rs (3)
src/rbac/map.rs (1)
roles(74-80)src/handlers/http/modal/query/querier_rbac.rs (1)
add_roles_to_user(158-209)src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
add_roles_to_user(78-118)
src/handlers/http/modal/query/querier_rbac.rs (3)
src/rbac/map.rs (1)
roles(74-80)src/handlers/http/rbac.rs (2)
Users(71-71)add_roles_to_user(255-304)src/handlers/http/modal/ingest/ingestor_rbac.rs (1)
add_roles_to_user(78-118)
⏰ 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-apple-darwin
- 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 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 (5)
src/handlers/http/modal/server.rs (1)
643-649: Doc fix matches actual route; LGTMThe POST annotation now aligns with the implemented
web::post()route.src/handlers/http/modal/query/querier_rbac.rs (1)
73-74: No further updates required forUserExistserror variant
TheUserExists(String)variant is already covered in theResponseErrorimpl (Self::UserExists(_) => StatusCode::BAD_REQUEST), and no other pattern matches rely on a payload-less form.src/handlers/http/rbac.rs (3)
111-112: Allowing empty body = zero roles is correct; document the new POST contract.Treating a missing body as an empty role set aligns with the PR goal. Please update API docs/examples to show:
- POST with no body creates a user with zero roles.
- POST with
[]behaves the same.
383-385: Public error shape change — call out in release notes.
RBACError::UserExists(String)changes the public error text/JSON. Flag this in changelog to help clients parsing errors.
130-131: AllUserExistsvariants now carry the payload correctly
Both constructor sites (rbac.rsandquerier_rbac.rs) returnErr(RBACError::UserExists(username)), and no unit-style match arms remain.
nikhilsinhaparseable
merged commit 5e89972
into
parseablehq:main
this is helpful when an admin wants to create a user and assign group to it
Summary by CodeRabbit
New Features
Improvements
Documentation