fix: use email as fallback if name not present in oidc login #1399
Conversation
|
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✨ 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: 3
🧹 Nitpick comments (3)
src/rbac/user.rs (1)
72-76: Align OAuth userid fallback with email as secondaryTo stay consistent with the OIDC login fallback (name → email → sub), consider including email in the userid fallback chain. While sub should be present, this makes the constructor resilient and consistent with reply_login.
- userid: user_info - .sub - .clone() - .or_else(|| user_info.name.clone()) - .unwrap_or(username), + userid: user_info + .sub + .clone() + .or_else(|| user_info.name.clone()) + .or_else(|| user_info.email.clone()) + .unwrap_or(username),src/handlers/http/oidc.rs (2)
244-264: User lookup: considerpreferred_usernameas an additional fallbackGreat migration helper. For completeness, also try preferred_username between name and email.
if let Some(name) = &user_info.name { if let Some(user) = Users.get_user(name) { return Some(user); } } + if let Some(preferred) = &user_info.preferred_username { + if let Some(user) = Users.get_user(preferred) { + return Some(user); + } + } + if let Some(email) = &user_info.email { if let Some(user) = Users.get_user(email) { return Some(user); } }
335-341: Hardenuser_idcookie: set HttpOnly (and optionally Secure)Unless the frontend must read the user_id, make it HttpOnly to reduce exposure. Consider Secure in production.
pub fn cookie_userid(user_id: &str) -> Cookie<'static> { Cookie::build(USER_ID_COOKIE_NAME, user_id.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) .same_site(SameSite::Strict) + .http_only(true) + // .secure(true) // consider enabling under HTTPS .path("/") .finish() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/http/oidc.rs(8 hunks)src/handlers/mod.rs(1 hunks)src/rbac/mod.rs(1 hunks)src/rbac/user.rs(3 hunks)src/rbac/utils.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/handlers/mod.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-01T10:27:56.858Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
Applied to files:
src/handlers/http/oidc.rs
⏰ 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 Default x86_64-unknown-linux-gnu
- 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 Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (7)
src/rbac/user.rs (2)
174-176: Good addition: capture OIDC subject (sub) in UserInfoCapturing sub is essential for stable identity. LGTM.
195-202: Populatesubfrom OIDC userinfoMapping
subthrough Fromopenid::Userinfo is correct. LGTM.src/rbac/mod.rs (1)
213-217: UsersPrism API adds a new username fieldThe
UsersPrismstruct in src/rbac/mod.rs (lines 212–217) now serializes both
id(still the JWTsubclaim) and- a new
usernamefield for display.This alters the JSON shape returned by:
to_prism_userin src/rbac/utils.rs- the
GET /api/v1/user/{username}handler in src/handlers/http/rbac.rsPlease verify all downstream consumers (UI, CLI, other services) that parse this JSON:
- Use the new
usernamefield for display or labels- Continue to treat
idpurely as the subject identifiersrc/rbac/utils.rs (1)
75-78: LGTM: propagate newusernameto UsersPrismPopulating the new field looks correct for both Native and OAuth paths.
src/handlers/http/oidc.rs (3)
105-111: LGTM: also setuser_idcookie for Basic Auth exchangeIssuing both username and user_id cookies maintains consistency across auth methods.
195-221: Default role handling: ensure at least one role or fail fastCurrent logic still allows empty roles if DEFAULT_ROLE is unset. Confirm intended behavior; otherwise, fail the login when no roles resolve.
Proposed guard (optional):
if final_roles.is_empty() { // No valid roles and no default configured – reject login explicitly return Err(OIDCError::Unauthorized); }
223-225: LGTM: new users keyed byuser_id(sub)Persisting by stable subject identifier is the right call.
nikhilsinhaparseable
force-pushed
the
oidc-fix-userinfo
branch
from
cf3f038 to
5d3c187
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/handlers/http/oidc.rs (2)
167-176: Avoid panics on missing claims; derive user_id robustly using ID tokensub
.expect(...)will panic if a provider omits optional fields in userinfo. Prefer falling back toclaims.sub(required in ID Token) and return a controlled error if ever absent.Apply:
- let username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .or_else(|| user_info.sub.clone()) - .expect("OIDC provider did not return a usable identifier (name, email or sub)"); - let user_id = user_info - .sub - .clone() - .expect("OIDC provider did not return a usable identifier (sub)"); + let username = user_info + .name + .clone() + .or_else(|| user_info.email.clone()) + // fall back to ID Token `sub` (required by spec) + .unwrap_or_else(|| claims.sub.clone()); + let user_id = user_info + .sub + .clone() + .unwrap_or_else(|| claims.sub.clone());
390-416: When migrating tosub, also update group memberships and stale sessionsYou migrate the stored user object and persist it, but references in
metadata.user_groupsstill point to the old id; stale sessions may also point to the old id. Update both during migration.Augment the metadata update:
- oauth_user.user_info.clone_from(&user_info); + oauth_user.user_info.clone_from(&user_info); *roles = group; // Update userid to use sub if available (migration from name-based to sub-based identification) if let Some(ref sub) = user_info.sub { oauth_user.userid.clone_from(sub); } @@ - if let Some(entry) = metadata + if let Some(entry) = metadata .users .iter_mut() .find(|x| x.username() == old_username) { entry.clone_from(&user); + // Also migrate user references inside user groups + for grp in metadata.user_groups.iter_mut() { + if grp.users.remove(&old_username) { + grp.users.insert(user.username().to_string()); + } + } put_metadata(&metadata).await?; }For sessions: if the in-memory session map stores the old id, either:
- Migrate session owner ids from
old_username→user.username(), or- Invalidate sessions for
old_usernameto avoid orphaned sessions.If you want, I can draft a small API addition in
Usersto migrate or purge sessions for a given user id.
🧹 Nitpick comments (1)
src/handlers/http/oidc.rs (1)
195-201: Handle poisoned DEFAULT_ROLE lock gracefully
lock().unwrap()can panic if the mutex is poisoned. Fall back safely.- let default_role = if let Some(default_role) = DEFAULT_ROLE.lock().unwrap().clone() { - HashSet::from([default_role]) - } else { - HashSet::new() - }; + let default_role = match DEFAULT_ROLE.lock() { + Ok(guard) => guard + .clone() + .map(|role| HashSet::from([role])) + .unwrap_or_default(), + Err(_) => HashSet::new(), + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/http/oidc.rs(8 hunks)src/handlers/mod.rs(1 hunks)src/rbac/mod.rs(1 hunks)src/rbac/user.rs(3 hunks)src/rbac/utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/handlers/mod.rs
- src/rbac/mod.rs
- src/rbac/user.rs
- src/rbac/utils.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-01T10:27:56.858Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
Applied to files:
src/handlers/http/oidc.rs
⏰ 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: coverage
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- 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: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (7)
src/handlers/http/oidc.rs (7)
105-111: USER_ID cookie for basic auth uses username — confirm client expectationsFor native users, setting user_id = username is reasonable; verify the UI treats user_id as “internal identifier” generically (OIDC sub for OAuth, username for native).
Would you like me to scan the UI code for reads of the user-id cookie to confirm no provider-specific assumptions?
201-222: Role resolution logic is sound; default never emptyMerging existing roles with valid OIDC roles and falling back to default avoids empty role sets. LGTM.
236-241: Include USER_ID cookie in post-login redirectCookie set sequencing looks correct. LGTM.
244-264: User lookup by sub → name → email is correct and migration-friendlyThis prevents duplicate users and enables smooth ID migration. LGTM.
419-424: Lookup by old id during persistence is correct — ensure no duplicate entriesGood that you locate by
old_username; confirm there’s no separate entry for the new id to avoid duplicates. If duplicates are possible, dedupe by removing the old entry after cloning the new data in.I can generate a quick scan to detect duplicate user records by id in metadata during startup if helpful.
35-35: ConfirmUSER_ID_COOKIE_NAMEdefinition and front-end alignment
- Verified that
USER_ID_COOKIE_NAMEis declared insrc/handlers/mod.rs(line 41) and correctly imported insrc/handlers/http/oidc.rs.- Ensure the literal
"user_id"matches any client-side (frontend) expectations or documentation so there are no mismatches in cookie handling.
224-225: No other asyncput_userinvocations detected
Verified that the only call to the freeput_user(&str, HashSet<String>, UserInfo)function is in this match arm (src/handlers/http/oidc.rs:224). All otherput_useroccurrences are calls to theUsers::put_user(User)method and remain unchanged.
nikhilsinhaparseable
force-pushed
the
oidc-fix-userinfo
branch
from
4fb1661 to
56de397
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/handlers/http/oidc.rs (3)
322-336: Harden session/username cookies in buildersSession cookie should be
secureandhttp_only. Username cookie should besecure; keephttp_only(false)only if UI reads it. See concrete diffs provided in thecookie_useridcomment.
167-176: Avoid panics on missing claims; deriveusername/user_idrobustly (fallback to ID Tokensub)Using
.expect(...)will crash the handler if an OIDC provider omits fields in userinfo. Prefer falling back to the (required) ID Tokensub, and return a controlled error if even that is missing.Apply this diff to make the derivation robust:
- let username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .or_else(|| user_info.sub.clone()) - .expect("OIDC provider did not return a usable identifier (name, email or sub)"); - let user_id = user_info - .sub - .clone() - .expect("OIDC provider did not return a usable identifier (sub)"); + let username = user_info + .name + .clone() + .or_else(|| user_info.email.clone()) + // fall back to ID Token `sub` (required by spec) + .unwrap_or_else(|| claims.sub.clone()); + let user_id = user_info + .sub + .clone() + // fall back to `claims.sub` if userinfo.sub is absent + .unwrap_or_else(|| claims.sub.clone());Additionally (outside this hunk), verify that when
userinfo.subis present, it matchesclaims.subas required by the spec, otherwise reject the login:// place before: let user_info: user::UserInfo = user_info.into(); if let Some(ref ui_sub) = user_info.sub { if ui_sub != &claims.sub { return Err(OIDCError::Unauthorized); } }
338-344: Hardenuser_idcookie: setsecure; considerhttp_onlyMark cookies as
secureso they’re only sent over HTTPS. Keephttp_only(false)only if the frontend must read this value.Apply this diff:
pub fn cookie_userid(user_id: &str) -> Cookie<'static> { Cookie::build(USER_ID_COOKIE_NAME, user_id.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) + .secure(true) .same_site(SameSite::Strict) .path("/") .finish() }Also consider:
- In
cookie_session: add.secure(true).http_only(true)- In
cookie_username: add.secure(true)(keeping it readable if the UI needs it)For reference (outside this hunk), suggested builders:
pub fn cookie_session(id: Ulid) -> Cookie<'static> { Cookie::build(SESSION_COOKIE_NAME, id.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) .secure(true) .http_only(true) .same_site(SameSite::Strict) .path("/") .finish() } pub fn cookie_username(username: &str) -> Cookie<'static> { Cookie::build(USER_COOKIE_NAME, username.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) .secure(true) .same_site(SameSite::Strict) .path("/") .finish() }
🧹 Nitpick comments (5)
src/handlers/http/oidc.rs (5)
195-200: Default role fallback: OK; minor: avoid locking on each requestThe logic to materialize a HashSet from
DEFAULT_ROLEis fine. If this path is hot, consider caching the value in anOnceCell<Option<String>>(or similar) to avoid mutex contention. Optional.
247-267: Simplifyfind_existing_userfor compatibility; avoidlet-chainsThe
if let ... && let ...pattern depends onlet_chains. For broader MSRV compatibility and readability, use nestedif let.Apply this diff:
-fn find_existing_user(user_info: &user::UserInfo) -> Option<User> { - if let Some(sub) = &user_info.sub - && let Some(user) = Users.get_user(sub) - { - return Some(user); - } - - if let Some(name) = &user_info.name - && let Some(user) = Users.get_user(name) - { - return Some(user); - } - - if let Some(email) = &user_info.email - && let Some(user) = Users.get_user(email) - { - return Some(user); - } - - None -} +fn find_existing_user(user_info: &user::UserInfo) -> Option<User> { + if let Some(sub) = &user_info.sub { + if let Some(user) = Users.get_user(sub) { + return Some(user); + } + } + if let Some(name) = &user_info.name { + if let Some(user) = Users.get_user(name) { + return Some(user); + } + } + if let Some(email) = &user_info.email { + if let Some(user) = Users.get_user(email) { + return Some(user); + } + } + None +}
481-486: Redirect URL check is very strictComparing full “host/” to the redirect (sans scheme) forces redirects only to “/”. If intentional, ignore. If the intention is to allow same-origin paths, consider checking only the host portion.
Would you like me to propose a stricter-but-flexible validator that ensures same-origin while allowing arbitrary paths and query strings?
232-245: Unit coverage: add tests for username/user_id fallback and migrationConsider adding tests that verify:
- username fallback order: name → email → claims.sub
- user_id fallback: userinfo.sub → claims.sub
- migration path updates
useridand preserves roles; when groups are present, memberships are migrated.I can draft these tests against the RBAC/Users harness if you’d like.
355-361: Consider verifyinguserinfo.subequalsclaims.subYou already decode/validate the ID Token. Add a check that
userinfo.sub(if present) matchesclaims.suband reject otherwise to prevent token mix-up.
📜 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 (5)
src/handlers/http/oidc.rs(8 hunks)src/handlers/mod.rs(1 hunks)src/rbac/mod.rs(1 hunks)src/rbac/user.rs(3 hunks)src/rbac/utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/handlers/mod.rs
- src/rbac/mod.rs
- src/rbac/user.rs
- src/rbac/utils.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/handlers/http/oidc.rs (1)
src/rbac/user.rs (1)
username(84-92)
⏰ 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). (3)
- 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/handlers/http/oidc.rs (7)
35-35: Import looks correct and aligned with new cookieBringing
USER_ID_COOKIE_NAMEinto scope matches the new helper and downstream usage.
105-111: Setuser_idcookie for BasicAuth path — OKUsing username as
user_idfor native users is consistent with the OAuthsub-as-id model. No functional concerns here.
201-222: Role resolution logic is reasonable
- Existing users: union current roles with valid OIDC groups.
- New users: use OIDC roles or fall back to default.
- Ensures non-empty final set.
Looks good.
227-228: Creating users keyed bysubis correctSwitching to
user_id(OIDCsub) as the canonical key for new OAuth users is the right move.
146-152: Logout path OKConditional provider logout for OAuth users is sound.
97-114: Basic auth flow: password verification and cookie issuance OKThe flow and checks remain correct after adding the new
user_idcookie.
481-486: Incorrect review comment – unrelated migration verification requested
The verification script targetsuser_groupsin metadata, but this snippet (is_valid_redirect_urlinsrc/handlers/http/oidc.rs) solely handles URL scheme stripping and comparison. Please remove or ignore the migration-related request here.Likely an incorrect or invalid review comment.
nikhilsinhaparseable
force-pushed
the
oidc-fix-userinfo
branch
from
56de397 to
1e0f70c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/rbac/utils.rs (1)
32-44: Make display name fallback to email when name is missing (aligns with login cookie)Currently, OAuth display_name falls back to the opaque id if name is missing. Prefer name → email → id to match the login flow and improve UX.
- UserType::OAuth(oauth) => { - let username = user.username(); - let display_name = oauth.user_info.name.as_deref().unwrap_or(username); + UserType::OAuth(oauth) => { + let username = user.username(); // id (sub) + let display_name = oauth + .user_info + .name + .as_deref() + .or_else(|| oauth.user_info.email.as_deref()) + .unwrap_or(username); ( username, display_name, "oauth", oauth.user_info.email.clone(), oauth.user_info.picture.clone(), ) }src/handlers/http/oidc.rs (3)
167-176: Avoid panics on missing claims; fall back to ID tokensubUsing
.expect(...)will crash the handler if userinfo omits fields. Use the ID token’ssub(required) as a fallback and return Unauthorized if still unavailable.- let username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .or_else(|| user_info.sub.clone()) - .expect("OIDC provider did not return a usable identifier (name, email or sub)"); - let user_id = user_info - .sub - .clone() - .expect("OIDC provider did not return a usable identifier (sub)"); + let username = user_info + .name + .clone() + .or_else(|| user_info.email.clone()) + // fall back to ID Token `sub` + .unwrap_or_else(|| claims.sub.clone()); + let user_id = match user_info.sub.clone().or_else(|| Some(claims.sub.clone())) { + Some(id) => id, + None => return Err(OIDCError::Unauthorized), + };
336-342: Hardenuser_idcookie: add.secure(true)(and consider HttpOnly based on UI needs)Mark cookies Secure so they’re sent only over HTTPS. Keep HttpOnly false only if the frontend must read
user_id.pub fn cookie_userid(user_id: &str) -> Cookie<'static> { Cookie::build(USER_ID_COOKIE_NAME, user_id.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) + .secure(true) .same_site(SameSite::Strict) .path("/") .finish() }Additionally (outside this hunk):
- In cookie_session(): add
.secure(true).http_only(true)- In cookie_username(): add
.secure(true)(keep.http_only(false)only if the UI reads it)Example (outside selected lines):
pub fn cookie_session(id: Ulid) -> Cookie<'static> { Cookie::build(SESSION_COOKIE_NAME, id.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) .secure(true) .http_only(true) .same_site(SameSite::Strict) .path("/") .finish() } pub fn cookie_username(username: &str) -> Cookie<'static> { Cookie::build(USER_COOKIE_NAME, username.to_string()) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) .secure(true) .same_site(SameSite::Strict) .path("/") .finish() }
391-429: Complete metadata migration when userid changes; tolerate missing entry; update group membershipsWhen migrating from name/email → sub, also:
- Insert a new entry if the user wasn’t present in metadata (edge case).
- Update any group memberships that reference the old id, otherwise group roles won’t apply.
- Persist metadata after updating it (move put_metadata after modifications).
- // Find the user entry using the old username (before migration) - if let Some(entry) = metadata - .users - .iter_mut() - .find(|x| x.username() == old_username) - { - entry.clone_from(&user); - put_metadata(&metadata).await?; - } - Users.delete_user(&old_username); - Users.put_user(user.clone()); + // Find the user entry using the old username (before migration) + if let Some(entry) = metadata + .users + .iter_mut() + .find(|x| x.username() == old_username) + { + entry.clone_from(&user); + } else { + // User not present in metadata; insert + metadata.users.push(user.clone()); + } + // Migrate user references inside user groups (if present) + if let Some(user_groups) = metadata.user_groups.as_mut() { + for group in user_groups.iter_mut() { + if group.users.remove(&old_username) { + group.users.insert(user.username().to_string()); + } + } + } + put_metadata(&metadata).await?; + // Refresh in-memory cache and drop old key + Users.delete_user(&old_username); + Users.put_user(user.clone());If
metadata.user_groupsisn’t optional in your model, drop theOptionguard accordingly.
🧹 Nitpick comments (3)
src/rbac/mod.rs (1)
213-217: Clarify semantics ofidvsusername; consider fixing serde rename_all
- The new
usernamefield is good. Please clarify in the comment thatidis “sub for OAuth; username for native,” otherwise the “sub” comment will mislead API consumers for native users.- Also, note that
#[serde(rename = "camelCase")]on the struct doesn’t do what you likely intend. If the goal is camelCase field names, switch to#[serde(rename_all = "camelCase")]at the container level. Not a blocker, but worth correcting.Apply this minimal doc tweak:
- // sub + // sub (OAuth) or username (native)src/rbac/utils.rs (1)
75-84: Field naming nit:usernamein UsersPrism is actually a display nameGiven OAuth maps
usernameto a display name (name/email/id), consider renaming the field todisplay_nameon the API in a future breaking change to avoid confusion. Out of scope for this PR, but worth tracking.I can open a follow-up issue and propose a backwards-compatible transition plan if helpful.
src/handlers/http/oidc.rs (1)
235-242: Cookies on redirect: OK; ensure hardened cookie flags in buildersSetting session, username, and user_id cookies here looks correct. See cookie builder hardening suggestion below to add
.secure(true)(and review.http_only(...)needs).
📜 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 (5)
src/handlers/http/oidc.rs(8 hunks)src/handlers/mod.rs(1 hunks)src/rbac/mod.rs(1 hunks)src/rbac/user.rs(3 hunks)src/rbac/utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handlers/mod.rs
- src/rbac/user.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/handlers/http/oidc.rs (2)
src/rbac/user.rs (2)
username(84-92)from(193-203)src/rbac/mod.rs (1)
put_user(52-55)
src/rbac/utils.rs (1)
src/rbac/user.rs (1)
username(84-92)
⏰ 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-apple-darwin
- 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 x86_64-pc-windows-msvc
- GitHub Check: Build Kafka 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 (6)
src/handlers/http/oidc.rs (6)
35-35: LGTM: new USER_ID cookie wiringImport looks correct and isolates the name in a single place.
105-111: LGTM: add user_id cookie for Basic Auth exchangeMirrors the OAuth path so client code can rely on both cookies consistently.
195-200: LGTM: sensible default role fallbackGood to ensure a non-empty role set with a configurable default when OIDC group mapping yields nothing.
201-221: LGTM: role merge logic for existing/new users
- Existing users keep roles and gain new valid OIDC roles.
- New users get valid OIDC roles or fall back to the default.
- Final guard ensures no-empty roles.
223-226: LGTM: store new OAuth users keyed by user_id (sub)Switching to
subas the primary key removes ambiguity when name/email change.
245-265: LGTM: robust existing-user lookup orderLookup by
subfirst, thenname, then
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/handlers/http/oidc.rs (3)
339-345: Cookie security flags: keep HTTP support, but consider conditional hardeningGood addition of
cookie_useridmirroring other cookies. For production, consider.http_only(true)and.secure(true)when served over HTTPS, but keep it off for HTTP to avoid browser rejection. Make this conditional via env/config.Example approach (outside this hunk):
fn apply_cookie_security(mut c: Cookie<'static>) -> Cookie<'static> { if std::env::var("COOKIE_SECURE").as_deref() == Ok("true") { c.set_secure(true); } if std::env::var("COOKIE_HTTP_ONLY").as_deref() != Ok("false") { c.set_http_only(true); } c }
167-179: Avoid panics; deriveusernameanduser_idrobustly using ID Tokensubas fallbackUsing
.expect(...)on Line 172 can panic ifname,userinfo.subare all absent. Also, some providers omitsubin userinfo, but ID Tokensubis required by the spec. Fall back toclaims.subfor bothusernameanduser_idto prevent handler crashes and unnecessary Unauthorized errors.Apply this diff:
- let username = user_info - .name - .clone() - .or_else(|| user_info.email.clone()) - .or_else(|| user_info.sub.clone()) - .expect("OIDC provider did not return a usable identifier (name, email or sub)"); - let user_id = match user_info.sub.clone() { - Some(id) => id, - None => { - tracing::error!("OIDC provider did not return a sub"); - return Err(OIDCError::Unauthorized); - } - }; + let username = user_info + .name + .clone() + .or_else(|| user_info.email.clone()) + // Fall back to ID Token `sub` (required by spec) + .unwrap_or_else(|| claims.sub.clone()); + // Prefer userinfo.sub, but fall back to ID Token `sub` to avoid failing when userinfo omits it + let user_id = user_info + .sub + .clone() + .unwrap_or_else(|| claims.sub.clone());
423-437: Persist user even if old entry is missing; always apply group migrations and write metadataIf the user isn’t found in metadata (Line 423), we currently skip persistence and group migration, leading to stale/absent metadata and inconsistent state after restart. Insert on miss, always migrate group refs, and write metadata once.
Apply this diff:
- // Find the user entry using the old username (before migration) - if let Some(entry) = metadata - .users - .iter_mut() - .find(|x| x.username() == old_username) - { - entry.clone_from(&user); - // migrate user references inside user groups - for group in metadata.user_groups.iter_mut() { - if group.users.remove(&old_username) { - group.users.insert(user.username().to_string()); - } - } - put_metadata(&metadata).await?; - } + // Update existing entry or insert if missing + if let Some(entry) = metadata + .users + .iter_mut() + .find(|x| x.username() == old_username) + { + entry.clone_from(&user); + } else { + metadata.users.push(user.clone()); + } + // Migrate user references inside user groups + for group in metadata.user_groups.iter_mut() { + if group.users.remove(&old_username) { + group.users.insert(user.username().to_string()); + } + } + put_metadata(&metadata).await?;
🧹 Nitpick comments (2)
src/handlers/http/oidc.rs (2)
226-229: Nit: Parameter naming mismatch input_usercan confuse maintainersAt the call site we pass
&user_id(Line 228), butput_user’s parameter is namedusername. Since OAuth users are keyed bysub, consider renaming the parameter touseridfor clarity.Example (outside this hunk):
pub async fn put_user( userid: &str, group: HashSet<String>, user_info: user::UserInfo, ) -> Result<User, ObjectStorageError> { // ... use `userid` consistently }
240-244: Consider makinguser_idcookie HttpOnly (conditionally)You’re now setting
cookie_userid(&user_id)(Lines 241–244). If the UI does not need to readuser_idfrom JavaScript, preferHttpOnlyto reduce exposure. Per our prior discussion, only setSecurein HTTPS deployments to avoid breakage on HTTP.
📜 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/oidc.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T09:44:26.115Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1399
File: src/handlers/http/oidc.rs:336-342
Timestamp: 2025-08-19T09:44:26.115Z
Learning: Secure cookies with .secure(true) cannot be set in HTTP contexts as browsers will reject them. Cookie security attributes should be conditionally applied based on whether the application is running over HTTPS or made configurable for different deployment environments.
Applied to files:
src/handlers/http/oidc.rs
🧬 Code Graph Analysis (1)
src/handlers/http/oidc.rs (5)
src/rbac/user.rs (2)
username(84-92)from(193-203)src/handlers/http/rbac.rs (1)
from(55-65)src/rbac/map.rs (2)
from(294-302)from(346-354)src/rbac/mod.rs (1)
put_user(52-55)src/handlers/http/role.rs (2)
get_metadata(143-151)put_metadata(153-157)
⏰ 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 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 aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- 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 (6)
src/handlers/http/oidc.rs (6)
105-111: LGTM: Add user_id cookie for BasicAuth flowAdding
cookie_userid(&username)(Line 105) and including it in the redirect (Line 110) keeps the contract consistent across native and OIDC logins. For native users,user_id==username, which is reasonable.
198-203: LGTM: Sensible default role fallbackComputing a default role set (Lines 198–203) and deferring to it when OIDC groups don’t map to roles is a solid defensive default.
204-225: LGTM: Role merge logic for existing/new users
- Existing user: merge new OIDC-mapped roles into existing set (Lines 205–211).
- New user: use OIDC-mapped roles or fall back to default (Lines 213–219).
- Safety net to ensure non-empty roles (Lines 221–224).
This avoids accidentally creating users with no permissions.
394-407: LGTM: Robust migration gating and user update
- Storing
old_usernamebefore mutation is necessary for metadata and cache updates (Lines 394–396).needs_userid_migrationcorrectly triggers even when only thesubchanged (Lines 401–407).- Updating
user_info, roles, and migratinguseridtosubwhen present is correct (Lines 413–420).Also applies to: 413-420
438-440: LGTM: Refresh in-memory cache and drop old keyDeleting the old key (Line 438) and re-inserting the updated user ensures the in-memory map stays coherent post-migration.
35-35: No action needed:USER_ID_COOKIE_NAMEis correctly available
The constantUSER_ID_COOKIE_NAMEis defined insrc/handlers/mod.rs(line 41). Becauseoidc.rsresides in thehandlersmodule’s descendant tree, it can import that private constant directly—no public re-export is required.
Summary by CodeRabbit
New Features
Bug Fixes