chore: add validation to user and role creation#1435
chore: add validation to user and role creation#1435nitisht merged 5 commits intoparseablehq:mainfrom
Conversation
WalkthroughHandlers now validate user/role path names earlier using Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as HTTP Handler (POST /user or PUT /role)
participant V as validator::user_role_name
participant M as Metadata Store
participant S as Sync/Downstream
C->>H: Request with {name}
H->>V: validator::user_role_name(name)
alt invalid
V-->>H: UsernameValidationError
H-->>C: 400 Bad Request (validation error)
else valid
V-->>H: Ok
H->>M: get_metadata()
H->>H: parse body, role checks, updates (add/remove roles)
H->>S: persist / invalidate sessions / sync
H-->>C: 2xx Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 3
🧹 Nitpick comments (3)
src/handlers/http/rbac.rs (1)
123-130: Remove duplicate condition in existence check.
Users.contains(&username) && Users.contains(&username)is redundant. Simplify.Apply this diff:
- if Users.contains(&username) && Users.contains(&username) - || metadata.users.iter().any(|user| match &user.ty { + 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(username)); }src/handlers/http/modal/query/querier_rbac.rs (1)
175-186: Correct spelling: non_existent_roles.Minor naming nit; adjust both occurrences for consistency with other files.
Apply this diff:
- 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));- 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: 231-242
src/validator.rs (1)
191-198: Stale error text; mentions “lowercase” and “underscore-only”.This variant’s message no longer reflects the actual rules and may confuse users. Update or deprecate.
Apply this diff:
- #[error( - "Username contains invalid characters. Please use lowercase, alphanumeric or underscore" - )] - SpecialChar, + #[error( + "Username contains invalid characters. Only alphanumeric characters and _, -, ., , are allowed" + )] + SpecialChar,Optionally remove this variant in a follow-up if unused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/http/modal/query/querier_rbac.rs(1 hunks)src/handlers/http/modal/query/querier_role.rs(2 hunks)src/handlers/http/rbac.rs(1 hunks)src/handlers/http/role.rs(2 hunks)src/validator.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/handlers/http/role.rs (1)
src/validator.rs (1)
user_role_name(89-142)
src/handlers/http/modal/query/querier_rbac.rs (1)
src/validator.rs (1)
user_role_name(89-142)
src/handlers/http/modal/query/querier_role.rs (1)
src/validator.rs (1)
user_role_name(89-142)
src/handlers/http/rbac.rs (1)
src/validator.rs (1)
user_role_name(89-142)
⏰ 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: coverage
- GitHub Check: Build Default aarch64-apple-darwin
- 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: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (4)
src/handlers/http/rbac.rs (1)
104-105: Good move: early, unified validation on username.Validating before touching metadata is correct and consistent with the new rules.
src/handlers/http/modal/query/querier_rbac.rs (1)
47-48: LGTM: unified username validation placed early.Consistent with the new validator and error mapping.
src/validator.rs (2)
73-87: Reserved-name set looks good.Coverage matches the PR’s list and is case-insensitive via normalization.
Would you also want to reserve “superuser”, “root”, or environment-like values (e.g., “system”)? If yes, I can add them.
90-92: Clarify normalization intent (validation vs storage).validator::user_role_name lowercases only for validation (src/validator.rs:90–91) but handlers insert the original-cased name into metadata (src/handlers/http/role.rs:48; src/handlers/http/modal/query/querier_role.rs:49), so "Alice" and "alice" will be distinct. Confirm whether names should be case-insensitive; if so, normalize consistently before storing/using as keys (and for lookups).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/validator.rs (1)
119-139: Comma now allowed; consecutive-specials logic—LGTM.This resolves the earlier review about rejecting commas; consecutive-special detection is correct.
🧹 Nitpick comments (2)
src/validator.rs (2)
89-101: Lowercasing + length bounds—OK; note on byte-length semantics.len() counts bytes; given ASCII-only allowed chars, this is fine. If you ever broaden the charset, switch to chars().count().
192-209: Cleanup/error-text consistency: deprecate stale SpecialChar; clarify InvalidCharacter message.SpecialChar is no longer emitted by user_role_name and its text contradicts the current rules. Deprecate it and make the message consistent. Also rephrase InvalidCharacter to list allowed chars clearly.
Apply this diff:
@@ #[error("Username should be between 3 and 64 chars long")] InvalidLength, - #[error( - "Username contains invalid characters. Please use lowercase, alphanumeric or underscore" - )] - SpecialChar, + #[deprecated(note = "Use InvalidCharacter/InvalidStartChar/InvalidEndChar/ConsecutiveSpecialChars")] + #[error("Username contains invalid characters; allowed: letters, digits, underscore (_), hyphen (-), comma (,), and period (.)")] + SpecialChar, @@ - #[error( - "Username contains invalid characters. Only alphanumeric characters and _, -, ., , are allowed" - )] + #[error("Username contains invalid characters; allowed: letters, digits, underscore (_), hyphen (-), comma (,), and period (.)")] InvalidCharacter,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/modal/query/querier_role.rs(2 hunks)src/handlers/http/role.rs(4 hunks)src/validator.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/modal/query/querier_role.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/handlers/http/role.rs (1)
src/validator.rs (1)
user_role_name(89-142)
⏰ 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 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 aarch64-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 Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (4)
src/validator.rs (2)
73-87: Reserved names set looks good; case-insensitive check confirmed by pre-lowercasing.Implementation matches PR spec; using Lazy is fine.
Confirm that case-insensitive blocking of reserved names is intended (because input is lowercased before checking).
105-117: Start/end alphanumeric checks—LGTM.Accurately enforces the “first/last must be alphanumeric” rule.
src/handlers/http/role.rs (2)
35-36: Map validation errors to 400 and validate early—LGTM.Importing UsernameValidationError and mapping via RoleError::ValidationError correctly surfaces client issues as Bad Request.
Also applies to: 45-47
174-176: New RoleError::ValidationError + BAD_REQUEST mapping—LGTM.Error surfacing is now accurate and user-friendly.
Also applies to: 186-187
nikhilsinhaparseable
dismissed
coderabbitai[bot]’s stale review
via
096c48f
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 (1)
src/handlers/http/modal/query/querier_rbac.rs (1)
156-206: Missing UPDATE_LOCK in add_roles_to_user causes races (write vs. other user/role ops).remove_roles_from_user takes UPDATE_LOCK; add_roles_to_user does not. Acquire the same lock around metadata/mem updates to avoid TOCTOU and inconsistent state.
@@ pub async fn add_roles_to_user( @@ - // update parseable.json first - let mut metadata = get_metadata().await?; + // update parseable.json first (guard updates similar to `remove_roles_from_user`) + let _guard = UPDATE_LOCK.lock().await; + let mut metadata = get_metadata().await?; @@ put_metadata(&metadata).await?; // update in mem table Users.add_roles(&userid.clone(), roles_to_add.clone());
🧹 Nitpick comments (4)
src/validator.rs (2)
73-87: Reserved-name set: good; consider compile-time set.Current Lazy is fine. If you want zero runtime init and faster lookup, consider phf::phf_set! (if adding a dep is acceptable).
89-143: Validator matches the spec; minor micro-optimization and clarity nits.
- Avoid allocating Vec; use iterators for first/last checks and the scan.
- Keeping allowed specials in one place helps keep the code and error text in sync (optional).
Apply this diff to drop the allocation:
@@ pub fn user_role_name(name: &str) -> Result<(), UsernameValidationError> { // Normalize username to lowercase for validation let name = name.to_lowercase(); @@ - let chars: Vec<char> = name.chars().collect(); - // Check first character (must be alphanumeric) - if let Some(first_char) = chars.first() + if let Some(first_char) = name.chars().next() && !first_char.is_ascii_alphanumeric() { return Err(UsernameValidationError::InvalidStartChar); } // Check last character (must be alphanumeric) - if let Some(last_char) = chars.last() + if let Some(last_char) = name.chars().rev().next() && !last_char.is_ascii_alphanumeric() { return Err(UsernameValidationError::InvalidEndChar); } // Check all characters and consecutive special chars let mut prev_was_special = false; - for &ch in &chars { + for ch in name.chars() { match ch { // Allow alphanumeric c if c.is_ascii_alphanumeric() => { prev_was_special = false; } // Allow specific special characters '_' | '-' | '.' | ',' => { if prev_was_special { return Err(UsernameValidationError::ConsecutiveSpecialChars); } prev_was_special = true; } // Reject any other character _ => { return Err(UsernameValidationError::InvalidCharacter); } } }src/handlers/http/modal/query/querier_rbac.rs (2)
175-186: Good rename to non_existent_roles; make error deterministic.Sort/dedup before returning so clients get stable error payloads irrespective of HashSet iteration order.
- if !non_existent_roles.is_empty() { + if !non_existent_roles.is_empty() { + non_existent_roles.sort(); + non_existent_roles.dedup(); return Err(RBACError::RolesDoNotExist(non_existent_roles)); }
231-242: Mirror deterministic ordering in remove_roles_from_user.Same suggestion as above for stable error output.
- if !non_existent_roles.is_empty() { + if !non_existent_roles.is_empty() { + non_existent_roles.sort(); + non_existent_roles.dedup(); return Err(RBACError::RolesDoNotExist(non_existent_roles)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/modal/query/querier_rbac.rs(3 hunks)src/handlers/http/rbac.rs(2 hunks)src/validator.rs(3 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/handlers/http/modal/query/querier_rbac.rs (2)
src/validator.rs (1)
user_role_name(89-142)src/rbac/map.rs (1)
roles(74-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-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 Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- 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
🔇 Additional comments (3)
src/validator.rs (2)
19-23: Imports look correct and scoped.once_cell::Lazy and HashSet are appropriate for the reserved-name set.
190-210: Harmonize error text to “Name”, deprecate SpecialChar, and verify no remaining referencesFile: src/validator.rs (lines 190-210)
- Replace user-facing "Username" with "Name".
- Mark SpecialChar as deprecated (keep variant for ABI).
Apply this diff:
@@ pub enum UsernameValidationError { - #[error("Username should be between 3 and 64 chars long")] + #[error("Name should be between 3 and 64 characters long")] InvalidLength, - #[error( - "Username contains invalid characters. Only alphanumeric characters and _, -, ., , are allowed" - )] - SpecialChar, - #[error("Username should start with an alphanumeric character")] + #[deprecated(note = "Use InvalidCharacter or ConsecutiveSpecialChars instead")] + #[error("Name contains invalid characters. Only alphanumeric characters and the following special characters are allowed: _, -, ,, .")] + SpecialChar, + #[error("Name should start with an alphanumeric character")] InvalidStartChar, - #[error("Username should end with an alphanumeric character")] + #[error("Name should end with an alphanumeric character")] InvalidEndChar, - #[error( - "Username contains invalid characters. Only alphanumeric characters and _, -, ., , are allowed" - )] + #[error("Name contains invalid characters. Only alphanumeric characters and the following special characters are allowed: _, -, ,, .")] InvalidCharacter, - #[error("Username contains consecutive special characters")] + #[error("Name contains consecutive special characters")] ConsecutiveSpecialChars, - #[error("Username is reserved")] + #[error("Name is reserved")] ReservedName, }Confirm no code still matches on UsernameValidationError::SpecialChar:
#!/bin/bash rg -n "UsernameValidationError::SpecialChar"src/handlers/http/modal/query/querier_rbac.rs (1)
47-47: Confirm RBACError maps validation failure to HTTP 400Early call validator::user_role_name at src/handlers/http/modal/query/querier_rbac.rs:47 is correct. Repository search returned no matches for
impl From<validator::error::UsernameValidationError> for RBACErrororimpl ResponseError for RBACError— add/confirm animpl From<validator::error::UsernameValidationError> for RBACError(or implementResponseErrorforRBACError) that maps the validation variant to HTTP 400.
below validations are added to both user and role names at creation - 1. the length should be between 3-64 characters 2. the name should not be any of these - admin, user, role, null, undefined, none, empty, password, username 3. first and the last character must be alphanumeric 4. allow any of these special characters - `_, -,.` 5. no two consecutive characters should be special character
nikhilsinhaparseable
force-pushed
the
add-user-role-validation
branch
from
26938bf to
180266e
Compare
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 (1)
src/handlers/http/rbac.rs (1)
122-146: Avoid holding UPDATE_LOCK across awaits; inline role assignment instead of calling an async handler.The lock is held while awaiting
add_roles_to_user(...), lengthening the critical section and risking future deadlocks if that path ever acquires the same lock. Do the role assignment inline using the in-scopemetadataandUsers, then persist once.- put_metadata(&metadata).await?; - let created_role = user_roles.clone(); - Users.put_user(user.clone()); - if !created_role.is_empty() { - add_roles_to_user( - web::Path::<String>::from(username.clone()), - web::Json(created_role), - ) - .await?; - } + let created_role = user_roles.clone(); + Users.put_user(user.clone()); + if !created_role.is_empty() { + if let Some(u) = metadata + .users + .iter_mut() + .find(|u| u.userid() == user.userid()) + { + u.roles.extend(created_role.clone()); + } + Users.add_roles(&username, created_role); + } + put_metadata(&metadata).await?;Optional follow-up: re-check role existence after taking the lock to avoid TOCTOU if roles can change concurrently.
♻️ Duplicate comments (1)
src/validator.rs (1)
128-138: Blocker: Comma ',' is still rejected but PR spec allows it.Align validator with spec by allowing ',' and preserving the consecutive‑specials check.
- '_' | '-' | '.' => { + '_' | '-' | '.' | ',' => { if prev_was_special { return Err(UsernameValidationError::ConsecutiveSpecialChars); } prev_was_special = true; }
🧹 Nitpick comments (1)
src/validator.rs (1)
195-205: Remove unused SpecialChar variant and fix error-message punctuation
- Change both error strings in src/validator.rs (around lines 195 and 203) to use an Oxford comma: "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen, and dot) are allowed"
- Remove the unused enum variant SpecialChar (src/validator.rs:197); a repo search shows no references to UsernameValidationError::SpecialChar — keep and use InvalidCharacter for invalid-character errors instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/http/modal/query/querier_rbac.rs(3 hunks)src/handlers/http/modal/query/querier_role.rs(2 hunks)src/handlers/http/rbac.rs(2 hunks)src/handlers/http/role.rs(4 hunks)src/validator.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/handlers/http/modal/query/querier_role.rs
- src/handlers/http/role.rs
- src/handlers/http/modal/query/querier_rbac.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/handlers/http/rbac.rs (1)
src/validator.rs (1)
user_role_name(89-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: coverage
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build 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 Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (3)
src/validator.rs (2)
105-111: Start/end alphanumeric checks look good.Matches the “first/last must be alphanumeric” rule.
Also applies to: 112-117
90-92: Lowercasing for validation only is fine.Case-insensitive checks for reserved names without mutating stored value.
src/handlers/http/rbac.rs (1)
104-104: Good: early validation before metadata I/O.Validating path param upfront reduces wasted work and yields clear 400s.
Consider applying the same early validation to other user/role path handlers for consistency.
nikhilsinhaparseable
dismissed
coderabbitai[bot]’s stale review
via
d7a8b7f
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/validator.rs (1)
121-129: PR/spec mismatch: comma ',' must be allowed; update error text accordingly.The validator currently rejects ',' and the messages omit it. Spec allows underscore, hyphen, comma, and period.
- '_' | '-' | '.' => { + '_' | '-' | '.' | ',' => { if prev_was_special { return Err(UsernameValidationError::ConsecutiveSpecialChars); } prev_was_special = true; }- #[error( - "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen and dot) are allowed" - )] + #[error( + "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen, comma and dot) are allowed" + )] SpecialChar, @@ - #[error( - "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen and dot) are allowed" - )] + #[error( + "Username contains invalid characters. Only alphanumeric characters and special characters (underscore, hyphen, comma and dot) are allowed" + )] InvalidCharacter,Also applies to: 188-199
🧹 Nitpick comments (3)
src/validator.rs (3)
183-203: Error wording may confuse for roles; consider neutral “name”.These errors bubble up for both users and roles but say “Username”. Consider “Name …” to avoid confusing role APIs.
112-127: Minor maintainability: centralize allowed specials.Define a small const (e.g., ALLOWED_USER_ROLE_SPECIALS: [char; 4]) or reuse one place to reduce future drift between code and messages.
89-135: Add unit tests for edge cases.Please add tests for: min/max length (2/3/64/65), start/end special chars, consecutive specials (e.g., "a__b", "a,.-b"), allowed comma cases ("a,b"), reserved names (case-insensitive), and mixed-case valid names.
Do you want me to draft a test module exercising these cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validator.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/validator.rs (2)
src/handlers/http/modal/mod.rs (1)
new(288-310)src/rbac/user.rs (1)
new(339-341)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
🔇 Additional comments (1)
src/validator.rs (1)
73-88: Reserved names set looks good.Covers the PR list and normalizes via lowercase first. LGTM.
2 participants
below validations are added to both user and role names at creation -
_, -,.Summary by CodeRabbit
New Features
Bug Fixes