fix: use email as fallback if name not present in oidc login #1399
Conversation
WalkthroughThe change introduces handling of a new user identifier cookie alongside the existing username cookie in OIDC login flows. It enhances user lookup by searching with multiple identifiers ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
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.
| .name | ||
| .clone() | ||
| .expect("OIDC provider did not return a sub which is currently required."); | ||
| .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 user_info: user::UserInfo = user_info.into(); |
There was a problem hiding this comment.
Avoid panics on missing claims; derive user_id robustly
Using .expect(...) will crash the handler if an OIDC provider omits fields in userinfo. The ID Token’s sub is required; fall back to it and return an error instead of panicking.
- 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 = match user_info.sub.clone().or_else(|| Some(claims.sub.clone())) {
+ Some(id) => id,
+ None => return Err(OIDCError::Unauthorized),
+ };🤖 Prompt for AI Agents
In src/handlers/http/oidc.rs around lines 168 to 177, the code uses .expect()
which causes a panic if required OIDC claims like name, email, or sub are
missing. Replace these .expect() calls with proper error handling: check if the
fields exist, and if not, return a controlled error response instead of
panicking. Ensure user_id is derived from the required sub claim safely,
returning an error if it is absent.
src/handlers/http/oidc.rs
Outdated
| // Store the old username before modifying the user object | ||
| let old_username = user.username().to_string(); | ||
| let User { ty, roles, .. } = &mut user; | ||
| let UserType::OAuth(oauth_user) = ty else { | ||
| unreachable!() | ||
| }; | ||
|
|
||
| // update user only if roles or userinfo has changed | ||
| if roles == &group && oauth_user.user_info == user_info { | ||
| // Check if userid needs migration to sub (even if nothing else changed) | ||
| let needs_userid_migration = if let Some(ref sub) = user_info.sub { | ||
| oauth_user.userid != *sub | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| // update user only if roles, userinfo has changed, or userid needs migration | ||
| if roles == &group && oauth_user.user_info == user_info && !needs_userid_migration { | ||
| return Ok(user); | ||
| } | ||
|
|
||
| oauth_user.user_info = user_info; | ||
| oauth_user.user_info = user_info.clone(); | ||
| *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 = sub.clone(); | ||
| } | ||
|
|
||
| let mut metadata = get_metadata().await?; | ||
|
|
||
| // Find the user entry using the old username (before migration) | ||
| if let Some(entry) = metadata | ||
| .users | ||
| .iter_mut() | ||
| .find(|x| x.username() == user.username()) | ||
| .find(|x| x.username() == old_username) | ||
| { |
There was a problem hiding this comment.
Migrate group membership when changing OAuth id (name/email → sub)
If userid changes, entries in metadata.user_groups.users referencing the old id will become stale and group roles won’t apply. Update group memberships during the migration.
- // Find the user entry using the old username (before migration)
+ // 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);
+ // Also 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?;
}Also consider invalidating sessions tied to the old id if any exist.
🤖 Prompt for AI Agents
In src/handlers/http/oidc.rs around lines 390 to 424, when the OAuth userid
changes (migrating from name/email to sub), the group memberships in
metadata.user_groups.users referencing the old userid become stale. To fix this,
update the group memberships to replace the old userid with the new one during
the migration. Additionally, check for and invalidate any sessions tied to the
old userid to prevent stale session issues.
| let (id, username, method, email, picture) = match &user.ty { | ||
| UserType::Native(_) => (user.username(), user.username(), "native", None, None), | ||
| UserType::OAuth(oauth) => { | ||
| let username = user.username(); | ||
| let display_name = oauth.user_info.name.as_deref().unwrap_or(username); | ||
| ( | ||
| username, | ||
| display_name, | ||
| "oauth", | ||
| oauth.user_info.email.clone(), | ||
| oauth.user_info.picture.clone(), | ||
| ) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use email as display fallback when name is missing (consistent with login cookie)
Bring to_prism_user in line with reply_login: prefer name, then email, else id (sub). This avoids showing the opaque sub when name is absent.
- 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(),
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (id, username, method, email, picture) = match &user.ty { | |
| UserType::Native(_) => (user.username(), user.username(), "native", None, None), | |
| UserType::OAuth(oauth) => { | |
| let username = user.username(); | |
| let display_name = oauth.user_info.name.as_deref().unwrap_or(username); | |
| ( | |
| username, | |
| display_name, | |
| "oauth", | |
| oauth.user_info.email.clone(), | |
| oauth.user_info.picture.clone(), | |
| ) | |
| } | |
| }; | |
| let (id, username, method, email, picture) = match &user.ty { | |
| UserType::Native(_) => (user.username(), user.username(), "native", None, None), | |
| 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(), | |
| ) | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/rbac/utils.rs around lines 32 to 45, update the logic for determining the
display name in the OAuth user match arm to prefer the user's name, then
fallback to their email if the name is missing, and finally fallback to the
username (id) if both are absent. This ensures the display name is consistent
with the login cookie behavior and avoids showing an opaque id when the name is
missing.
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.
| 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) | ||
| .path("/") | ||
| .finish() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden cookies: set secure and review http_only based on UI needs
Session/user cookies should be secure in production. Consider http_only(true) for session cookie; for username/user_id, keep readable only if the frontend must access them.
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), consider:
- In
cookie_session: add.secure(true).http_only(true) - In
cookie_username: add.secure(true)(keephttp_only(false)if UI reads it)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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) | |
| .path("/") | |
| .finish() | |
| } | |
| 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() | |
| } |
🤖 Prompt for AI Agents
In src/handlers/http/oidc.rs around lines 335 to 341, enhance cookie security by
adding .secure(true) to the cookie builder to ensure cookies are only sent over
HTTPS. Also, evaluate whether to add .http_only(true) based on whether the
frontend needs to access the user_id cookie; if the UI requires access, keep
http_only(false). For other cookies like cookie_session, add both .secure(true)
and .http_only(true), and for cookie_username, add .secure(true) while keeping
.http_only(false) if the UI reads it.
Summary by CodeRabbit