From 8012f5aa22d12fc140cee28bedba750c6e6611a1 Mon Sep 17 00:00:00 2001 From: Anant Vindal Date: Wed, 4 Mar 2026 17:15:08 +0530 Subject: [PATCH 01/19] feat: Updates for OAUTH implementation --- src/handlers/http/middleware.rs | 1 - src/handlers/http/modal/mod.rs | 35 ++----- src/handlers/http/modal/server.rs | 1 + src/handlers/http/oidc.rs | 146 +++++++++--------------------- src/lib.rs | 1 + src/oauth/mod.rs | 5 + src/oauth/oidc_client.rs | 138 ++++++++++++++++++++++++++++ src/oauth/provider.rs | 75 +++++++++++++++ src/rbac/user.rs | 14 +++ 9 files changed, 281 insertions(+), 135 deletions(-) create mode 100644 src/oauth/mod.rs create mode 100644 src/oauth/oidc_client.rs create mode 100644 src/oauth/provider.rs diff --git a/src/handlers/http/middleware.rs b/src/handlers/http/middleware.rs index f0d777879..9f8dba69a 100644 --- a/src/handlers/http/middleware.rs +++ b/src/handlers/http/middleware.rs @@ -320,7 +320,6 @@ pub async fn refresh_token( let refreshed_token = match client .read() .await - .client() .refresh_token(&oauth_data, Some(PARSEABLE.options.scope.as_str())) .await { diff --git a/src/handlers/http/modal/mod.rs b/src/handlers/http/modal/mod.rs index 471483d6d..115f8b643 100644 --- a/src/handlers/http/modal/mod.rs +++ b/src/handlers/http/modal/mod.rs @@ -26,7 +26,6 @@ use base64::{Engine, prelude::BASE64_STANDARD}; use bytes::Bytes; use futures::future; use once_cell::sync::OnceCell; -use openid::Discovered; use relative_path::RelativePathBuf; use serde::{Deserialize, Serialize}; use serde_json::{Map, Value}; @@ -40,7 +39,7 @@ use crate::{ correlation::CORRELATIONS, hottier::{HotTierManager, StreamHotTier}, metastore::metastore_traits::MetastoreObject, - oidc::{Claims, DiscoveredClient}, + oauth::{OAuthProvider, connect_oidc}, option::Mode, parseable::{DEFAULT_TENANT, PARSEABLE}, storage::{ObjectStorageProvider, PARSEABLE_ROOT_DIRECTORY}, @@ -48,7 +47,7 @@ use crate::{ utils::get_node_id, }; -use super::{API_BASE_PATH, API_VERSION, cross_origin_config, health_check, resource_check}; +use super::{cross_origin_config, health_check, resource_check}; pub mod ingest; pub mod ingest_server; @@ -58,28 +57,7 @@ pub mod server; pub mod ssl_acceptor; pub mod utils; -pub type OpenIdClient = Arc>; - -pub static OIDC_CLIENT: OnceCell>> = OnceCell::new(); - -#[derive(Debug)] -pub struct GlobalClient { - client: DiscoveredClient, -} - -impl GlobalClient { - pub fn set(&mut self, client: DiscoveredClient) { - self.client = client; - } - - pub fn client(&self) -> &DiscoveredClient { - &self.client - } - - pub fn new(client: DiscoveredClient) -> Self { - Self { client } - } -} +pub static OIDC_CLIENT: OnceCell>>> = OnceCell::new(); // to be decided on what the Default version should be pub const DEFAULT_VERSION: &str = "v4"; @@ -114,10 +92,9 @@ pub trait ParseableServer { Self: Sized, { if let Some(config) = oidc_client { - let client = config - .connect(&format!("{API_BASE_PATH}/{API_VERSION}/o/code")) - .await?; - OIDC_CLIENT.get_or_init(|| Arc::new(RwLock::new(GlobalClient::new(client)))); + let gc = connect_oidc(config).await?; + OIDC_CLIENT + .get_or_init(|| Arc::new(RwLock::new(Box::new(gc) as Box))); } // get the ssl stuff diff --git a/src/handlers/http/modal/server.rs b/src/handlers/http/modal/server.rs index 9b7aa4aea..29a977c3d 100644 --- a/src/handlers/http/modal/server.rs +++ b/src/handlers/http/modal/server.rs @@ -95,6 +95,7 @@ impl ParseableServer for Server { .service(Self::get_llm_webscope()) .service(Self::get_oauth_webscope()) .service(Self::get_user_role_webscope()) + .service(Self::get_roles_webscope()) .service(Self::get_counts_webscope().wrap(from_fn( resource_check::check_resource_utilization_middleware, ))) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 49bbb2e8c..b10c4b810 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -16,7 +16,7 @@ * */ -use std::{collections::HashSet, sync::Arc}; +use std::collections::HashSet; use actix_web::http::StatusCode; use actix_web::{ @@ -26,22 +26,18 @@ use actix_web::{ web, }; use chrono::{Duration, TimeDelta}; -use openid::{Bearer, Options, Token, Userinfo}; +use openid::Bearer; use regex::Regex; use serde::Deserialize; -use tokio::sync::RwLock; use ulid::Ulid; use url::Url; use crate::{ handlers::{ COOKIE_AGE_DAYS, SESSION_COOKIE_NAME, USER_COOKIE_NAME, USER_ID_COOKIE_NAME, - http::{ - API_BASE_PATH, API_VERSION, - modal::{GlobalClient, OIDC_CLIENT}, - }, + http::modal::OIDC_CLIENT, }, - oidc::{Claims, DiscoveredClient}, + oauth::OAuthSession, parseable::{DEFAULT_TENANT, PARSEABLE}, rbac::{ self, EXPIRY_DURATION, Users, @@ -87,11 +83,15 @@ pub async fn login( let (session_key, oidc_client) = match (session_key, oidc_client) { (None, None) => return Ok(redirect_no_oauth_setup(query.redirect.clone())), (None, Some(client)) => { - return Ok(redirect_to_oidc( - query, - client.read().await.client(), - PARSEABLE.options.scope.to_string().as_str(), - )); + let redirect = query.into_inner().redirect.to_string(); + + let scope = PARSEABLE.options.scope.to_string(); + let mut auth_url: String = client.read().await.auth_url(&scope, Some(redirect)).into(); + + auth_url.push_str("&access_type=offline&prompt=consent"); + return Ok(HttpResponse::TemporaryRedirect() + .insert_header((actix_web::http::header::LOCATION, auth_url)) + .finish()); } (Some(session_key), client) => (session_key, client), }; @@ -138,11 +138,17 @@ pub async fn login( } else { Users.remove_session(&key); if let Some(oidc_client) = oidc_client { - redirect_to_oidc( - query, - oidc_client.read().await.client(), - PARSEABLE.options.scope.to_string().as_str(), - ) + let redirect = query.into_inner().redirect.to_string(); + let scope = PARSEABLE.options.scope.to_string(); + let mut auth_url: String = oidc_client + .read() + .await + .auth_url(&scope, Some(redirect)) + .into(); + auth_url.push_str("&access_type=offline&prompt=consent"); + HttpResponse::TemporaryRedirect() + .insert_header((actix_web::http::header::LOCATION, auth_url)) + .finish() } else { redirect_to_client(query.redirect.as_str(), None) } @@ -161,13 +167,7 @@ pub async fn logout(req: HttpRequest, query: web::Query) -> let tenant_id = get_tenant_id_from_key(&session); let user = Users.remove_session(&session); let logout_endpoint = if let Some(client) = oidc_client { - client - .read() - .await - .client() - .config() - .end_session_endpoint - .clone() + client.read().await.logout_url() } else { None }; @@ -195,37 +195,38 @@ pub async fn reply_login( }; let tenant_id = get_tenant_id_from_request(&req); - let (mut claims, user_info, bearer) = match request_token(oidc_client, &login_query).await { - Ok(v) => v, + let OAuthSession { + bearer, + claims, + userinfo: user_info, + } = match oidc_client + .write() + .await + .exchange_code(&login_query.code) + .await + { + Ok(session) => session, Err(e) => { - tracing::error!("reply_login call failed- {e}"); + tracing::error!("reply_login exchange_code failed: {e}"); return Ok(HttpResponse::Unauthorized().finish()); } }; + 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)"); + .expect("OAuth 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"); + tracing::error!("OAuth provider did not return a sub"); return Err(OIDCError::Unauthorized); } }; let user_info: user::UserInfo = user_info.into(); - - // if provider has group A, and parseable as has role A - // then user will automatically get assigned role A - // else, the default oidc role (inside parseable) will get assigned - let group: HashSet = claims - .other - .remove("groups") - .map(serde_json::from_value) - .transpose()? - .unwrap_or_default(); + let group = claims.groups.clone(); let metadata = get_metadata(&tenant_id).await?; // Find which OIDC groups match existing roles in Parseable @@ -293,14 +294,12 @@ pub async fn reply_login( (None, roles) => put_user(&user_id, roles, user_info, bearer, None).await?, }; let id = Ulid::new(); - Users.new_session(&user, SessionKey::SessionId(id), expires_in); let redirect_url = login_query .state .clone() .unwrap_or_else(|| PARSEABLE.options.address.to_string()); - Ok(redirect_to_client( &redirect_url, [ @@ -347,24 +346,6 @@ fn exchange_basic_for_cookie( cookie_session(id) } -fn redirect_to_oidc( - query: web::Query, - oidc_client: &DiscoveredClient, - scope: &str, -) -> HttpResponse { - let redirect = query.into_inner().redirect.to_string(); - let auth_url = oidc_client.auth_url(&Options { - scope: Some(scope.to_string()), - state: Some(redirect), - ..Default::default() - }); - let mut url: String = auth_url.into(); - url.push_str("&access_type=offline&prompt=consent"); - HttpResponse::TemporaryRedirect() - .insert_header((actix_web::http::header::LOCATION, url)) - .finish() -} - fn redirect_to_oidc_logout(mut logout_endpoint: Url, redirect: &Url) -> HttpResponse { logout_endpoint.set_query(Some(&format!("post_logout_redirect_uri={redirect}"))); HttpResponse::TemporaryRedirect() @@ -422,51 +403,6 @@ pub fn cookie_userid(user_id: &str) -> Cookie<'static> { .finish() } -pub async fn request_token( - oidc_client: &Arc>, - login_query: &Login, -) -> anyhow::Result<(Claims, Userinfo, Bearer)> { - let old_client = oidc_client.read().await.client().clone(); - let mut token: Token = old_client.request_token(&login_query.code).await?.into(); - - let id_token = if let Some(token) = token.id_token.as_mut() { - token - } else { - return Err(anyhow::anyhow!("No id_token provided")); - }; - - if let Err(e) = old_client.decode_token(id_token) { - tracing::error!("error while decoding the id_token- {e}"); - let new_client = PARSEABLE - .options - .openid() - .unwrap() - .connect(&format!("{API_BASE_PATH}/{API_VERSION}/o/code")) - .await?; - - // Reuse the already-obtained token, just decode with new client's JWKS - new_client.decode_token(id_token)?; - new_client.validate_token(id_token, None, None)?; - let claims = id_token.payload().expect("payload is decoded").clone(); - - let userinfo = new_client.request_userinfo(&token).await?; - let bearer = token.bearer; - - // replace old client with new one - drop(old_client); - - oidc_client.write().await.set(new_client); - return Ok((claims, userinfo, bearer)); - } - - old_client.validate_token(id_token, None, None)?; - let claims = id_token.payload().expect("payload is decoded").clone(); - - let userinfo = old_client.request_userinfo(&token).await?; - let bearer = token.bearer; - Ok((claims, userinfo, bearer)) -} - // put new user in metadata if does not exit // update local cache pub async fn put_user( diff --git a/src/lib.rs b/src/lib.rs index b6d15205e..75b4254be 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -34,6 +34,7 @@ mod metadata; pub mod metastore; pub mod metrics; pub mod migration; +pub mod oauth; pub mod oidc; pub mod option; pub mod otel; diff --git a/src/oauth/mod.rs b/src/oauth/mod.rs new file mode 100644 index 000000000..9eff358ea --- /dev/null +++ b/src/oauth/mod.rs @@ -0,0 +1,5 @@ +pub mod oidc_client; +pub mod provider; + +pub use oidc_client::{GlobalClient, connect_oidc}; +pub use provider::{OAuthProvider, OAuthSession, ProviderClaims, ProviderUserInfo}; diff --git a/src/oauth/oidc_client.rs b/src/oauth/oidc_client.rs new file mode 100644 index 000000000..222dca5e0 --- /dev/null +++ b/src/oauth/oidc_client.rs @@ -0,0 +1,138 @@ +use async_trait::async_trait; +use openid::{Bearer, Options, Token}; +use url::Url; + +use crate::{ + handlers::http::{API_BASE_PATH, API_VERSION}, + oauth::provider::{OAuthProvider, OAuthSession, ProviderClaims, ProviderUserInfo}, + oidc::{Claims, DiscoveredClient, OpenidConfig}, + rbac::user::OAuth, +}; + +/// Wraps the OpenID Connect `DiscoveredClient`. +/// +/// Stores the original `OpenidConfig` and the redirect suffix so that it can +/// reconnect (rotating the JWKS) inside `exchange_code` without any outside +/// help. +#[derive(Debug)] +pub struct GlobalClient { + client: DiscoveredClient, + /// Original config – cloned and used to reconnect on JWKS rotation. + config: OpenidConfig, + /// `"api/v1/o/code"` – the path appended to the base URL for the + /// redirect URI when re-discovering. + redirect_suffix: String, +} + +impl GlobalClient { + pub fn new(client: DiscoveredClient, config: OpenidConfig, redirect_suffix: String) -> Self { + Self { + client, + config, + redirect_suffix, + } + } +} + +#[async_trait] +impl OAuthProvider for GlobalClient { + fn auth_url(&self, scope: &str, state: Option) -> Url { + self.client.auth_url(&Options { + scope: Some(scope.to_string()), + state, + ..Default::default() + }) + } + + /// Exchange an authorization code for the full session, handling JWKS + /// rotation transparently: if `decode_token` fails with the cached client, + /// a fresh discovery is performed and decoding is retried once. + async fn exchange_code(&mut self, code: &str) -> Result { + let mut token: Token = self.client.request_token(code).await?.into(); + + let id_token = token + .id_token + .as_mut() + .ok_or_else(|| anyhow::anyhow!("OIDC provider did not return an id_token"))?; + + if let Err(e) = self.client.decode_token(id_token) { + // Stale JWKS – reconnect and retry once. + tracing::warn!("id_token decode failed ({e}), rotating JWKS and retrying"); + self.client = self.config.clone().connect(&self.redirect_suffix).await?; + self.client.decode_token(id_token)?; + } + + self.client.validate_token(id_token, None, None)?; + + let raw_claims = id_token + .payload() + .expect("token is decoded at this point") + .clone(); + + // `sub` is a required non-optional String in StandardClaims. + // `email` and `name` are not in StandardClaims (they're userinfo fields) + // but providers sometimes include them as additional claims in the ID + // token; extract them from `other` if present. + let groups: std::collections::HashSet = raw_claims + .other + .get("groups") + .and_then(|v| serde_json::from_value(v.clone()).ok()) + .unwrap_or_default(); + + let claims = ProviderClaims { + sub: Some(raw_claims.standard.sub.clone()), + email: raw_claims + .other + .get("email") + .and_then(|v| v.as_str()) + .map(String::from), + name: raw_claims + .other + .get("name") + .and_then(|v| v.as_str()) + .map(String::from), + groups, + other: raw_claims.other.clone(), + }; + + let userinfo_raw = self.client.request_userinfo(&token).await?; + let userinfo = ProviderUserInfo { + sub: userinfo_raw.sub.clone(), + email: userinfo_raw.email.clone(), + name: userinfo_raw.name.clone(), + preferred_username: userinfo_raw.preferred_username.clone(), + + picture: userinfo_raw + .picture + .as_ref() + .map(|p| p.as_str().to_string()), + }; + + Ok(OAuthSession { + bearer: token.bearer, + claims, + userinfo, + }) + } + + async fn refresh_token( + &self, + oauth: &OAuth, + scope: Option<&str>, + ) -> Result { + // Box the clone so we can pass it to the openid client. + let boxed: Box = Box::new(oauth.clone()); + Ok(self.client.refresh_token(boxed, scope).await?) + } + + fn logout_url(&self) -> Option { + self.client.config().end_session_endpoint.clone() + } +} + +/// Runs OIDC discovery and wraps the result in a `GlobalClient`. +pub async fn connect_oidc(config: OpenidConfig) -> Result { + let redirect_suffix = format!("{API_BASE_PATH}/{API_VERSION}/o/code"); + let client = config.clone().connect(&redirect_suffix).await?; + Ok(GlobalClient::new(client, config, redirect_suffix)) +} diff --git a/src/oauth/provider.rs b/src/oauth/provider.rs new file mode 100644 index 000000000..77b31ed36 --- /dev/null +++ b/src/oauth/provider.rs @@ -0,0 +1,75 @@ +use std::{ + any::Any, + collections::{HashMap, HashSet}, +}; + +use async_trait::async_trait; +use openid::Bearer; +use url::Url; + +use crate::rbac::user::OAuth; + +/// Trait implemented by every OAuth provider. +/// +#[async_trait] +pub trait OAuthProvider: Send + Sync + Any { + /// Build the redirect-to-provider authorization URL. + fn auth_url(&self, scope: &str, state: Option) -> Url; + + /// Exchange an authorization code for the complete session data. + /// + /// Implementors are responsible for all internal steps: + /// - token request + /// - ID-token decoding and validation + /// - JWKS key rotation / reconnection (OIDC-specific) + /// - userinfo fetch + /// + /// Requires `&mut self` so OIDC can swap out its client on JWKS rotation + /// while holding the `write()` lock that the caller already holds. + async fn exchange_code(&mut self, code: &str) -> Result; + + /// Refresh an existing access token using the credentials stored in the + /// user's `OAuth` record. + async fn refresh_token( + &self, + oauth: &OAuth, + scope: Option<&str>, + ) -> Result; + + /// Return the provider's logout / end-session URL, if one exists. + fn logout_url(&self) -> Option; +} + +// ── Output types ──────────────────────────────────────────────────────────── + +/// Everything produced by a successful code exchange. +#[derive(Debug)] +pub struct OAuthSession { + /// Stored verbatim in `OAuth::bearer` in the user model. + pub bearer: Bearer, + pub claims: ProviderClaims, + pub userinfo: ProviderUserInfo, +} + +/// Identity claims extracted from the ID token / JWT. +#[derive(Debug, Clone)] +pub struct ProviderClaims { + /// `sub` – stable unique identifier for the user at this provider. + pub sub: Option, + pub email: Option, + pub name: Option, + /// Group memberships / roles declared by the provider. + pub groups: HashSet, + /// Any additional claims not captured by the fields above. + pub other: HashMap, +} + +/// Data returned by the provider's userinfo endpoint. +#[derive(Debug, Clone)] +pub struct ProviderUserInfo { + pub sub: Option, + pub email: Option, + pub name: Option, + pub preferred_username: Option, + pub picture: Option, +} diff --git a/src/rbac/user.rs b/src/rbac/user.rs index c438012e6..5e1a6a81f 100644 --- a/src/rbac/user.rs +++ b/src/rbac/user.rs @@ -253,6 +253,20 @@ impl From for UserInfo { } } +impl From for UserInfo { + fn from(info: crate::oauth::ProviderUserInfo) -> Self { + UserInfo { + sub: info.sub, + name: info.name, + preferred_username: info.preferred_username, + picture: info.picture.as_deref().and_then(|s| s.parse().ok()), + email: info.email, + gender: None, + updated_at: None, + } + } +} + /// Represents a user in a UserGroup - simplified structure for both user types #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] pub struct GroupUser { From fabb3491c329de44bb5b0abe40dff4e513307ef5 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Fri, 6 Mar 2026 04:41:15 +1100 Subject: [PATCH 02/19] conditional redirect code --- src/handlers/http/oidc.rs | 41 +++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index b10c4b810..f865df0b7 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -296,18 +296,35 @@ pub async fn reply_login( let id = Ulid::new(); Users.new_session(&user, SessionKey::SessionId(id), expires_in); - let redirect_url = login_query - .state - .clone() - .unwrap_or_else(|| PARSEABLE.options.address.to_string()); - Ok(redirect_to_client( - &redirect_url, - [ - cookie_session(id), - cookie_username(&username), - cookie_userid(&user_id), - ], - )) + let cookies = [ + cookie_session(id), + cookie_username(&username), + cookie_userid(&user_id), + ]; + + // If the request is an XHR/fetch call (e.g. from the SPA frontend), + // return 200 with cookies instead of a 301 redirect to avoid CORS issues. + let is_xhr = req.headers().contains_key("x-p-tenant") + || req + .headers() + .get("accept") + .and_then(|v| v.to_str().ok()) + .is_some_and(|v| v.contains("application/json")); + + if is_xhr { + let mut response = HttpResponse::Ok(); + for cookie in cookies { + response.cookie(cookie); + } + Ok(response.finish()) + } else { + let redirect_url = login_query + .state + .clone() + .unwrap_or_else(|| PARSEABLE.options.address.to_string()); + + Ok(redirect_to_client(&redirect_url, cookies)) + } } fn find_existing_user(user_info: &user::UserInfo, tenant_id: Option) -> Option { From aa8194b627498eefeb28c3498540217bc2e63177 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Fri, 6 Mar 2026 05:08:55 +1100 Subject: [PATCH 03/19] session, username and id in response body --- src/handlers/http/oidc.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index f865df0b7..3a1393d0b 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -316,7 +316,11 @@ pub async fn reply_login( for cookie in cookies { response.cookie(cookie); } - Ok(response.finish()) + Ok(response.json(serde_json::json!({ + "session": id.to_string(), + "username": username, + "user_id": user_id, + }))) } else { let redirect_url = login_query .state @@ -399,7 +403,8 @@ fn redirect_no_oauth_setup(mut url: Url) -> HttpResponse { 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)) - .same_site(SameSite::Strict) + .same_site(SameSite::None) + .secure(true) .path("/") .finish() } @@ -407,7 +412,8 @@ pub fn cookie_session(id: Ulid) -> Cookie<'static> { 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)) - .same_site(SameSite::Strict) + .same_site(SameSite::None) + .secure(true) .path("/") .finish() } @@ -415,7 +421,8 @@ pub fn cookie_username(username: &str) -> Cookie<'static> { 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) + .same_site(SameSite::None) + .secure(true) .path("/") .finish() } From 851ae0aedd96675c31338a813f8e0853eb195287 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Fri, 6 Mar 2026 05:25:11 +1100 Subject: [PATCH 04/19] default admin priviledge to tenant owner --- src/handlers/http/oidc.rs | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 3a1393d0b..56db5ef1c 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -25,7 +25,7 @@ use actix_web::{ http::header::ContentType, web, }; -use chrono::{Duration, TimeDelta}; +use chrono::{Duration, TimeDelta, Utc}; use openid::Bearer; use regex::Regex; use serde::Deserialize; @@ -275,7 +275,6 @@ pub async fn reply_login( // If no roles were found, use the default role final_roles.clone_from(&default_role); } - let expires_in = if let Some(expires_in) = bearer.expires_in.as_ref() { // need an i64 somehow if *expires_in > u32::MAX.into() { @@ -294,7 +293,27 @@ pub async fn reply_login( (None, roles) => put_user(&user_id, roles, user_info, bearer, None).await?, }; let id = Ulid::new(); - Users.new_session(&user, SessionKey::SessionId(id), expires_in); + + // Create session: try normal role resolution first. + // If roles resolve to empty permissions (e.g. tenant roles not in ROLES map), + // grant admin permissions so the tenant owner can access their workspace. + let perms = rbac::roles_to_permission( + user.roles(), + tenant_id.as_deref().unwrap_or(DEFAULT_TENANT), + ); + if perms.is_empty() { + use crate::rbac::role::{self, RoleBuilder}; + let admin_perms = RoleBuilder::from(&role::model::DefaultPrivilege::Admin).build(); + rbac::map::mut_sessions().track_new( + user.userid().to_owned(), + SessionKey::SessionId(id), + Utc::now() + expires_in, + admin_perms.into_iter().collect(), + &user.tenant, + ); + } else { + Users.new_session(&user, SessionKey::SessionId(id), expires_in); + } let cookies = [ cookie_session(id), From afb4400d891209a7225c8f69b9c1d42a37b3b8c7 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Fri, 6 Mar 2026 05:35:00 +1100 Subject: [PATCH 05/19] fix error --- src/handlers/http/oidc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 56db5ef1c..71f7270bd 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -254,7 +254,7 @@ pub async fn reply_login( // HashSet::new() // }; - let existing_user = find_existing_user(&user_info, tenant_id); + let existing_user = find_existing_user(&user_info, tenant_id.clone()); let mut final_roles = match existing_user { Some(ref user) => { // For existing users: keep existing roles + add new valid OIDC roles From 05a4b43f3badbb95f091e51f8d7f432f532aecbc Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Fri, 6 Mar 2026 13:20:24 +1100 Subject: [PATCH 06/19] match basic user email from user_info.email --- src/handlers/http/oidc.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 71f7270bd..91870623c 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -275,6 +275,21 @@ pub async fn reply_login( // If no roles were found, use the default role final_roles.clone_from(&default_role); } + // If still no roles, look for a native user with the same email + // and inherit their roles (e.g. tenant owner logging in via OAuth) + if final_roles.is_empty() { + if let Some(email) = &user_info.email { + for u in &metadata.users { + if matches!(u.ty, UserType::Native(_)) + && u.userid() == email.as_str() + && !u.roles.is_empty() + { + final_roles.clone_from(&u.roles); + break; + } + } + } + } let expires_in = if let Some(expires_in) = bearer.expires_in.as_ref() { // need an i64 somehow if *expires_in > u32::MAX.into() { @@ -289,8 +304,7 @@ pub async fn reply_login( let user = match (existing_user, final_roles) { (Some(user), roles) => update_user_if_changed(user, roles, user_info, bearer).await?, - // LET TENANT BE NONE FOR NOW!!! - (None, roles) => put_user(&user_id, roles, user_info, bearer, None).await?, + (None, roles) => put_user(&user_id, roles, user_info, bearer, tenant_id.clone()).await?, }; let id = Ulid::new(); From 709bbff99e064bd45c5052dca482e37ac2e76a15 Mon Sep 17 00:00:00 2001 From: Anant Vindal Date: Fri, 6 Mar 2026 11:41:17 +0530 Subject: [PATCH 07/19] fix: dataset_stats headers, oauth user update --- src/handlers/http/oidc.rs | 54 +++++++------------------------------- src/storage/field_stats.rs | 30 ++++++++++++++++----- 2 files changed, 33 insertions(+), 51 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 91870623c..cf6433f23 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -25,7 +25,7 @@ use actix_web::{ http::header::ContentType, web, }; -use chrono::{Duration, TimeDelta, Utc}; +use chrono::TimeDelta; use openid::Bearer; use regex::Regex; use serde::Deserialize; @@ -212,12 +212,15 @@ pub async fn reply_login( } }; - let username = user_info + let Some(username) = user_info .name .clone() .or_else(|| user_info.email.clone()) .or_else(|| user_info.sub.clone()) - .expect("OAuth provider did not return a usable identifier (name, email or sub)"); + else { + tracing::error!("OAuth provider did not return a usable identifier (name, email or sub)"); + return Err(OIDCError::Unauthorized); + }; let user_id = match user_info.sub.clone() { Some(id) => id, None => { @@ -240,7 +243,6 @@ pub async fn reply_login( let default_role = if let Some(role) = DEFAULT_ROLE .read() - // .unwrap() .get(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) && let Some(role) = role { @@ -248,11 +250,6 @@ pub async fn reply_login( } else { HashSet::new() }; - // let default_role = if let Some(default_role) = DEFAULT_ROLE.lock().unwrap().clone() { - // HashSet::from([default_role]) - // } else { - // HashSet::new() - // }; let existing_user = find_existing_user(&user_info, tenant_id.clone()); let mut final_roles = match existing_user { @@ -277,8 +274,8 @@ pub async fn reply_login( } // If still no roles, look for a native user with the same email // and inherit their roles (e.g. tenant owner logging in via OAuth) - if final_roles.is_empty() { - if let Some(email) = &user_info.email { + if final_roles.is_empty() + && let Some(email) = &user_info.email { for u in &metadata.users { if matches!(u.ty, UserType::Native(_)) && u.userid() == email.as_str() @@ -289,46 +286,13 @@ pub async fn reply_login( } } } - } - let expires_in = if let Some(expires_in) = bearer.expires_in.as_ref() { - // need an i64 somehow - if *expires_in > u32::MAX.into() { - EXPIRY_DURATION - } else { - let v = i64::from(*expires_in as u32); - Duration::seconds(v) - } - } else { - EXPIRY_DURATION - }; - let user = match (existing_user, final_roles) { + match (existing_user, final_roles) { (Some(user), roles) => update_user_if_changed(user, roles, user_info, bearer).await?, (None, roles) => put_user(&user_id, roles, user_info, bearer, tenant_id.clone()).await?, }; let id = Ulid::new(); - // Create session: try normal role resolution first. - // If roles resolve to empty permissions (e.g. tenant roles not in ROLES map), - // grant admin permissions so the tenant owner can access their workspace. - let perms = rbac::roles_to_permission( - user.roles(), - tenant_id.as_deref().unwrap_or(DEFAULT_TENANT), - ); - if perms.is_empty() { - use crate::rbac::role::{self, RoleBuilder}; - let admin_perms = RoleBuilder::from(&role::model::DefaultPrivilege::Admin).build(); - rbac::map::mut_sessions().track_new( - user.userid().to_owned(), - SessionKey::SessionId(id), - Utc::now() + expires_in, - admin_perms.into_iter().collect(), - &user.tenant, - ); - } else { - Users.new_session(&user, SessionKey::SessionId(id), expires_in); - } - let cookies = [ cookie_session(id), cookie_username(&username), diff --git a/src/storage/field_stats.rs b/src/storage/field_stats.rs index 78e6106e2..5b3cc1b66 100644 --- a/src/storage/field_stats.rs +++ b/src/storage/field_stats.rs @@ -24,11 +24,14 @@ use crate::event::format::json; use crate::handlers::TelemetryType; use crate::handlers::http::cluster::send_query_request; use crate::handlers::http::ingest::PostError; +use crate::handlers::http::middleware::CLUSTER_SECRET; +use crate::handlers::http::middleware::CLUSTER_SECRET_HEADER; use crate::handlers::http::query::Query; use crate::handlers::http::query::QueryError; use crate::handlers::http::query::query; use crate::metadata::SchemaVersion; use crate::option::Mode; +use crate::parseable::DEFAULT_TENANT; use crate::parseable::PARSEABLE; use crate::query::QUERY_SESSION_STATE; use crate::storage::ObjectStorageError; @@ -563,13 +566,27 @@ pub async fn get_dataset_stats( serde_json::from_str(body_str).map_err(|e| QueryError::CustomError(e.to_string()))? } Mode::Prism => { - let auth = if let Some(tenant) = tenant_id.as_ref() - && let Some(header) = TENANT_METADATA.get_global_query_auth(tenant) - { - let mut map = HeaderMap::new(); + let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + let auth = if let Some((_, hash)) = CLUSTER_SECRET.get() { + let mut map = actix_web::http::header::HeaderMap::new(); + if let Some(header) = TENANT_METADATA.get_global_query_auth(tenant) { + map.insert( + HeaderName::from_static("authorization"), + HeaderValue::from_str(&header).unwrap(), + ); + } + let userid = get_user_from_request(&req).unwrap(); + map.insert( + HeaderName::from_static(CLUSTER_SECRET_HEADER), + HeaderValue::from_str(hash).unwrap(), + ); map.insert( - HeaderName::from_static("authorization"), - HeaderValue::from_str(&header).unwrap(), + HeaderName::from_static("intra-cluster-tenant"), + HeaderValue::from_str(tenant).unwrap(), + ); + map.insert( + HeaderName::from_static("intra-cluster-userid"), + HeaderValue::from_str(&userid).unwrap(), ); Some(map) } else { @@ -589,6 +606,7 @@ pub async fn get_dataset_stats( } Some(map) }; + let response = match send_query_request(auth, &query_request, &tenant_id).await { Ok((query_response, _)) => query_response, Err(err) => { From 79b12149827d07f5cefd32f048f79340575cfb93 Mon Sep 17 00:00:00 2001 From: Anant Vindal Date: Fri, 6 Mar 2026 13:04:09 +0530 Subject: [PATCH 08/19] fix: create new user session --- src/handlers/http/oidc.rs | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index cf6433f23..df89e5972 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -25,7 +25,7 @@ use actix_web::{ http::header::ContentType, web, }; -use chrono::TimeDelta; +use chrono::{Duration, TimeDelta}; use openid::Bearer; use regex::Regex; use serde::Deserialize; @@ -275,23 +275,38 @@ pub async fn reply_login( // If still no roles, look for a native user with the same email // and inherit their roles (e.g. tenant owner logging in via OAuth) if final_roles.is_empty() - && let Some(email) = &user_info.email { - for u in &metadata.users { - if matches!(u.ty, UserType::Native(_)) - && u.userid() == email.as_str() - && !u.roles.is_empty() - { - final_roles.clone_from(&u.roles); - break; - } + && let Some(email) = &user_info.email + { + for u in &metadata.users { + if matches!(u.ty, UserType::Native(_)) + && u.userid() == email.as_str() + && !u.roles.is_empty() + { + final_roles.clone_from(&u.roles); + break; } } + } + + let expires_in = if let Some(expires_in) = bearer.expires_in.as_ref() { + // need an i64 somehow + if *expires_in > u32::MAX.into() { + EXPIRY_DURATION + } else { + let v = i64::from(*expires_in as u32); + Duration::seconds(v) + } + } else { + EXPIRY_DURATION + }; - match (existing_user, final_roles) { + let user = match (existing_user, final_roles) { (Some(user), roles) => update_user_if_changed(user, roles, user_info, bearer).await?, (None, roles) => put_user(&user_id, roles, user_info, bearer, tenant_id.clone()).await?, }; + let id = Ulid::new(); + Users.new_session(&user, SessionKey::SessionId(id), expires_in); let cookies = [ cookie_session(id), From ac3a294f4533112d70ab8c48f75ab7919a3b4c26 Mon Sep 17 00:00:00 2001 From: Anant Vindal Date: Sat, 7 Mar 2026 18:47:52 +0530 Subject: [PATCH 09/19] updates --- src/handlers/http/middleware.rs | 13 +++++++++---- src/oauth/oidc_client.rs | 2 ++ src/oauth/provider.rs | 2 ++ src/parseable/mod.rs | 22 +++++++++++----------- src/tenants/mod.rs | 8 ++++---- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/handlers/http/middleware.rs b/src/handlers/http/middleware.rs index 9f8dba69a..ed2ecb838 100644 --- a/src/handlers/http/middleware.rs +++ b/src/handlers/http/middleware.rs @@ -23,7 +23,7 @@ use actix_web::{ Error, HttpMessage, HttpRequest, Route, dev::{Service, ServiceRequest, ServiceResponse, Transform, forward_ready}, error::{ErrorBadRequest, ErrorForbidden, ErrorUnauthorized}, - http::header::{self, HeaderName, HeaderValue}, + http::header::{self, HeaderMap, HeaderName, HeaderValue}, }; use argon2::{Argon2, PasswordHash, PasswordVerifier}; use chrono::{Duration, TimeDelta, Utc}; @@ -194,7 +194,7 @@ where } let auth_result: Result<_, Error> = (self.auth_method)(&mut req, self.action); - + let headers = req.headers().clone(); let fut = self.service.call(req); Box::pin(async move { let Ok(key) = key else { @@ -209,7 +209,7 @@ where // if session is expired, refresh token if sessions().is_session_expired(&key) { - refresh_token(user_and_tenant_id, &key).await?; + refresh_token(user_and_tenant_id, &key, headers).await?; } match auth_result? { @@ -296,6 +296,7 @@ fn get_user_and_tenant( pub async fn refresh_token( user_and_tenant_id: Result<(Result, Option), RBACError>, key: &SessionKey, + headers: HeaderMap, ) -> Result<(), Error> { let oidc_client = OIDC_CLIENT.get(); @@ -320,7 +321,7 @@ pub async fn refresh_token( let refreshed_token = match client .read() .await - .refresh_token(&oauth_data, Some(PARSEABLE.options.scope.as_str())) + .refresh_token(&oauth_data, Some(PARSEABLE.options.scope.as_str()), headers) .await { Ok(bearer) => bearer, @@ -570,6 +571,10 @@ where header::COOKIE, HeaderValue::from_str(&format!("session={}", id)).unwrap(), ); + + // remove basic auth header + req.headers_mut().remove(header::AUTHORIZATION); + let session = SessionKey::SessionId(id); req.extensions_mut().insert(session.clone()); Users.new_session(&user, session, TimeDelta::seconds(20)); diff --git a/src/oauth/oidc_client.rs b/src/oauth/oidc_client.rs index 222dca5e0..a2e492eaa 100644 --- a/src/oauth/oidc_client.rs +++ b/src/oauth/oidc_client.rs @@ -1,3 +1,4 @@ +use actix_web::http::header::HeaderMap; use async_trait::async_trait; use openid::{Bearer, Options, Token}; use url::Url; @@ -119,6 +120,7 @@ impl OAuthProvider for GlobalClient { &self, oauth: &OAuth, scope: Option<&str>, + _headers: HeaderMap, ) -> Result { // Box the clone so we can pass it to the openid client. let boxed: Box = Box::new(oauth.clone()); diff --git a/src/oauth/provider.rs b/src/oauth/provider.rs index 77b31ed36..668d8880c 100644 --- a/src/oauth/provider.rs +++ b/src/oauth/provider.rs @@ -3,6 +3,7 @@ use std::{ collections::{HashMap, HashSet}, }; +use actix_web::http::header::HeaderMap; use async_trait::async_trait; use openid::Bearer; use url::Url; @@ -34,6 +35,7 @@ pub trait OAuthProvider: Send + Sync + Any { &self, oauth: &OAuth, scope: Option<&str>, + headers: HeaderMap, ) -> Result; /// Return the provider's logout / end-session URL, if one exists. diff --git a/src/parseable/mod.rs b/src/parseable/mod.rs index c6072798d..a35791a5c 100644 --- a/src/parseable/mod.rs +++ b/src/parseable/mod.rs @@ -1073,18 +1073,18 @@ impl Parseable { pub async fn suspend_tenant_service( &self, - tenant_id: String, - service: Service, + tenant_id: &str, + service: &Service, ) -> Result<(), anyhow::Error> { - TENANT_METADATA.suspend_service(&tenant_id, service.clone()); + TENANT_METADATA.suspend_service(tenant_id, service); // write to disk - let tenant_id = &Some(tenant_id); + let tenant_id = &Some(tenant_id.to_owned()); let mut meta = get_metadata(tenant_id).await?; if let Some(sus) = meta.suspended_services.as_mut() { - sus.insert(service); + sus.insert(service.clone()); } else { - meta.suspended_services = Some(HashSet::from_iter([service])); + meta.suspended_services = Some(HashSet::from_iter([service.clone()])); } put_remote_metadata(&meta, tenant_id).await?; @@ -1093,16 +1093,16 @@ impl Parseable { pub async fn resume_tenant_service( &self, - tenant_id: String, - service: Service, + tenant_id: &str, + service: &Service, ) -> Result<(), anyhow::Error> { - TENANT_METADATA.resume_service(&tenant_id, service.clone()); + TENANT_METADATA.resume_service(tenant_id, service); // write to disk - let tenant_id = &Some(tenant_id); + let tenant_id = &Some(tenant_id.to_owned()); let mut meta = get_metadata(tenant_id).await?; if let Some(sus) = meta.suspended_services.as_mut() { - sus.remove(&service); + sus.remove(service); } put_remote_metadata(&meta, tenant_id).await?; diff --git a/src/tenants/mod.rs b/src/tenants/mod.rs index 62926cb30..80197278f 100644 --- a/src/tenants/mod.rs +++ b/src/tenants/mod.rs @@ -90,16 +90,16 @@ impl TenantMetadata { } } - pub fn suspend_service(&self, tenant_id: &str, service: Service) { + pub fn suspend_service(&self, tenant_id: &str, service: &Service) { if let Some(mut tenant) = self.tenants.get_mut(tenant_id) { - tenant.suspended_services.insert(service); + tenant.suspended_services.insert(service.clone()); tenant.meta.suspended_services = Some(tenant.suspended_services.clone()); } } - pub fn resume_service(&self, tenant_id: &str, service: Service) { + pub fn resume_service(&self, tenant_id: &str, service: &Service) { if let Some(mut tenant) = self.tenants.get_mut(tenant_id) { - tenant.suspended_services.remove(&service); + tenant.suspended_services.remove(service); tenant.meta.suspended_services = if tenant.suspended_services.is_empty() { None } else { From 6578800072e4f74046edb9ea127b97bae2b918f9 Mon Sep 17 00:00:00 2001 From: Anant Vindal Date: Wed, 11 Mar 2026 17:55:45 +0530 Subject: [PATCH 10/19] fix: oauth issues --- src/handlers/http/oidc.rs | 43 +++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index df89e5972..04bd0c836 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -17,6 +17,8 @@ */ use std::collections::HashSet; +use std::sync::LazyLock; +use std::time::Instant; use actix_web::http::StatusCode; use actix_web::{ @@ -26,7 +28,9 @@ use actix_web::{ web, }; use chrono::{Duration, TimeDelta}; +use dashmap::DashMap; use openid::Bearer; +use rand::distributions::{Alphanumeric, DistString}; use regex::Regex; use serde::Deserialize; use ulid::Ulid; @@ -50,6 +54,31 @@ use crate::{ }, }; +/// In-memory store mapping OAuth state nonces to redirect URLs. +/// Entries expire after 10 minutes to prevent stale nonces. +const OAUTH_NONCE_TTL: std::time::Duration = std::time::Duration::from_secs(600); +static OAUTH_NONCE_STORE: LazyLock> = + LazyLock::new(DashMap::new); + +/// Generate a cryptographic nonce, store it with the redirect URL, and return the nonce. +fn store_oauth_nonce(redirect: &str) -> String { + // Evict expired entries opportunistically + OAUTH_NONCE_STORE.retain(|_, (_, created)| created.elapsed() < OAUTH_NONCE_TTL); + let nonce = Alphanumeric.sample_string(&mut rand::thread_rng(), 32); + OAUTH_NONCE_STORE.insert(nonce.clone(), (redirect.to_string(), Instant::now())); + nonce +} + +/// Look up and consume a nonce, returning the associated redirect URL if valid. +fn consume_oauth_nonce(nonce: &str) -> Option { + let (_, (redirect, created)) = OAUTH_NONCE_STORE.remove(nonce)?; + if created.elapsed() < OAUTH_NONCE_TTL { + Some(redirect) + } else { + None + } +} + /// Struct representing query params returned from oidc provider #[derive(Deserialize, Debug)] pub struct Login { @@ -84,9 +113,11 @@ pub async fn login( (None, None) => return Ok(redirect_no_oauth_setup(query.redirect.clone())), (None, Some(client)) => { let redirect = query.into_inner().redirect.to_string(); + let nonce = store_oauth_nonce(&redirect); let scope = PARSEABLE.options.scope.to_string(); - let mut auth_url: String = client.read().await.auth_url(&scope, Some(redirect)).into(); + let mut auth_url: String = + client.read().await.auth_url(&scope, Some(nonce)).into(); auth_url.push_str("&access_type=offline&prompt=consent"); return Ok(HttpResponse::TemporaryRedirect() @@ -139,11 +170,12 @@ pub async fn login( Users.remove_session(&key); if let Some(oidc_client) = oidc_client { let redirect = query.into_inner().redirect.to_string(); + let nonce = store_oauth_nonce(&redirect); let scope = PARSEABLE.options.scope.to_string(); let mut auth_url: String = oidc_client .read() .await - .auth_url(&scope, Some(redirect)) + .auth_url(&scope, Some(nonce)) .into(); auth_url.push_str("&access_type=offline&prompt=consent"); HttpResponse::TemporaryRedirect() @@ -325,6 +357,7 @@ pub async fn reply_login( if is_xhr { let mut response = HttpResponse::Ok(); + response.insert_header((actix_web::http::header::CACHE_CONTROL, "no-store")); for cookie in cookies { response.cookie(cookie); } @@ -334,9 +367,11 @@ pub async fn reply_login( "user_id": user_id, }))) } else { + // The state parameter is a nonce; resolve it to the original redirect URL. let redirect_url = login_query .state - .clone() + .as_deref() + .and_then(consume_oauth_nonce) .unwrap_or_else(|| PARSEABLE.options.address.to_string()); Ok(redirect_to_client(&redirect_url, cookies)) @@ -595,4 +630,4 @@ fn is_valid_redirect_url(base_url_without_scheme: &str, redirect_url: &str) -> b let redirect_url_without_scheme = http_scheme_match_regex.replace(redirect_url, ""); base_url_without_scheme == redirect_url_without_scheme -} +} \ No newline at end of file From 68e3fabaad4c6b38752c67a8fcb495bd1122bf93 Mon Sep 17 00:00:00 2001 From: Anant Vindal Date: Wed, 11 Mar 2026 18:16:45 +0530 Subject: [PATCH 11/19] fix: deepsourcex --- src/handlers/http/oidc.rs | 232 ++++++++++++++++++++------------------ 1 file changed, 125 insertions(+), 107 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 04bd0c836..0d7982a46 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -116,8 +116,7 @@ pub async fn login( let nonce = store_oauth_nonce(&redirect); let scope = PARSEABLE.options.scope.to_string(); - let mut auth_url: String = - client.read().await.auth_url(&scope, Some(nonce)).into(); + let mut auth_url: String = client.read().await.auth_url(&scope, Some(nonce)).into(); auth_url.push_str("&access_type=offline&prompt=consent"); return Ok(HttpResponse::TemporaryRedirect() @@ -220,134 +219,154 @@ pub async fn reply_login( req: HttpRequest, login_query: web::Query, ) -> Result { - let oidc_client = if let Some(oidc_client) = OIDC_CLIENT.get() { - oidc_client - } else { - return Err(OIDCError::Unauthorized); - }; + let oidc_client = OIDC_CLIENT.get().ok_or(OIDCError::Unauthorized)?; let tenant_id = get_tenant_id_from_request(&req); - let OAuthSession { - bearer, - claims, - userinfo: user_info, - } = match oidc_client + let session = oidc_client .write() .await .exchange_code(&login_query.code) .await - { - Ok(session) => session, - Err(e) => { + .map_err(|e| { tracing::error!("reply_login exchange_code failed: {e}"); - return Ok(HttpResponse::Unauthorized().finish()); + OIDCError::Unauthorized + })?; + + let (username, user_id, user_info) = extract_identity(&session)?; + let metadata = get_metadata(&tenant_id).await?; + let existing_user = find_existing_user(&user_info, tenant_id.clone()); + let final_roles = resolve_roles( + &session.claims.groups, + &metadata, + &user_info, + &tenant_id, + existing_user.as_ref(), + ); + + let expires_in = bearer_expiry(&session.bearer); + let user = match (existing_user, final_roles) { + (Some(user), roles) => { + update_user_if_changed(user, roles, user_info, session.bearer).await? + } + (None, roles) => { + put_user( + &user_id, + roles, + user_info, + session.bearer, + tenant_id.clone(), + ) + .await? } }; - let Some(username) = user_info + let id = Ulid::new(); + Users.new_session(&user, SessionKey::SessionId(id), expires_in); + + let cookies = [ + cookie_session(id), + cookie_username(&username), + cookie_userid(&user_id), + ]; + + Ok(build_login_response( + &req, + &login_query, + cookies, + id, + &username, + &user_id, + )) +} + +/// Extract username, user_id, and UserInfo from the OAuth session. +fn extract_identity(session: &OAuthSession) -> Result<(String, String, user::UserInfo), OIDCError> { + let user_info = &session.userinfo; + let username = user_info .name .clone() .or_else(|| user_info.email.clone()) .or_else(|| user_info.sub.clone()) - else { - tracing::error!("OAuth provider did not return a usable identifier (name, email or sub)"); - return Err(OIDCError::Unauthorized); - }; - let user_id = match user_info.sub.clone() { - Some(id) => id, - None => { - tracing::error!("OAuth provider did not return a sub"); - return Err(OIDCError::Unauthorized); - } - }; - let user_info: user::UserInfo = user_info.into(); - let group = claims.groups.clone(); - let metadata = get_metadata(&tenant_id).await?; + .ok_or_else(|| { + tracing::error!( + "OAuth provider did not return a usable identifier (name, email or sub)" + ); + OIDCError::Unauthorized + })?; + let user_id = user_info.sub.clone().ok_or_else(|| { + tracing::error!("OAuth provider did not return a sub"); + OIDCError::Unauthorized + })?; + Ok((username, user_id, user_info.clone().into())) +} - // Find which OIDC groups match existing roles in Parseable - let mut valid_oidc_roles = HashSet::new(); - for role in metadata.roles.iter() { - let role_name = role.0; - if group.contains(role_name) { - valid_oidc_roles.insert(role_name.clone()); - } - } +/// Determine the final set of roles for the user. +fn resolve_roles( + groups: &HashSet, + metadata: &StorageMetadata, + user_info: &user::UserInfo, + tenant_id: &Option, + existing_user: Option<&User>, +) -> HashSet { + let valid_oidc_roles: HashSet = metadata + .roles + .keys() + .filter(|role_name| groups.contains(*role_name)) + .cloned() + .collect(); - let default_role = if let Some(role) = DEFAULT_ROLE + let default_role = DEFAULT_ROLE .read() .get(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) - && let Some(role) = role - { - HashSet::from([role.to_owned()]) - } else { - HashSet::new() - }; + .and_then(|r| r.clone()) + .map(|r| HashSet::from([r])) + .unwrap_or_default(); - let existing_user = find_existing_user(&user_info, tenant_id.clone()); - let mut final_roles = match existing_user { - Some(ref user) => { - // For existing users: keep existing roles + add new valid OIDC roles + let mut roles = match existing_user { + Some(user) => { let mut roles = user.roles.clone(); - roles.extend(valid_oidc_roles); // Add new matching roles + roles.extend(valid_oidc_roles); roles } - None => { - // For new users: use valid OIDC roles, fallback to default if none - if valid_oidc_roles.is_empty() { - default_role.clone() - } else { - valid_oidc_roles - } - } + None if !valid_oidc_roles.is_empty() => valid_oidc_roles, + None => default_role.clone(), }; - if final_roles.is_empty() { - // If no roles were found, use the default role - final_roles.clone_from(&default_role); + + if roles.is_empty() { + roles.clone_from(&default_role); } - // If still no roles, look for a native user with the same email - // and inherit their roles (e.g. tenant owner logging in via OAuth) - if final_roles.is_empty() + + // Inherit roles from a native user with the same email (e.g. tenant owner via OAuth) + if roles.is_empty() && let Some(email) = &user_info.email - { - for u in &metadata.users { - if matches!(u.ty, UserType::Native(_)) - && u.userid() == email.as_str() - && !u.roles.is_empty() - { - final_roles.clone_from(&u.roles); - break; + && let Some(native) = metadata.users.iter().find(|u| { + matches!(u.ty, UserType::Native(_)) + && u.userid() == email.as_str() + && !u.roles.is_empty() + }) { + roles.clone_from(&native.roles); } - } - } - - let expires_in = if let Some(expires_in) = bearer.expires_in.as_ref() { - // need an i64 somehow - if *expires_in > u32::MAX.into() { - EXPIRY_DURATION - } else { - let v = i64::from(*expires_in as u32); - Duration::seconds(v) - } - } else { - EXPIRY_DURATION - }; - let user = match (existing_user, final_roles) { - (Some(user), roles) => update_user_if_changed(user, roles, user_info, bearer).await?, - (None, roles) => put_user(&user_id, roles, user_info, bearer, tenant_id.clone()).await?, - }; - - let id = Ulid::new(); - Users.new_session(&user, SessionKey::SessionId(id), expires_in); + roles +} - let cookies = [ - cookie_session(id), - cookie_username(&username), - cookie_userid(&user_id), - ]; +/// Compute session expiry from the bearer token. +fn bearer_expiry(bearer: &Bearer) -> TimeDelta { + match bearer.expires_in.as_ref() { + Some(&exp) if exp <= u32::MAX.into() => Duration::seconds(i64::from(exp as u32)), + _ => EXPIRY_DURATION, + } +} - // If the request is an XHR/fetch call (e.g. from the SPA frontend), - // return 200 with cookies instead of a 301 redirect to avoid CORS issues. +/// Build the HTTP response for the login callback (XHR JSON or redirect). +fn build_login_response( + req: &HttpRequest, + login_query: &web::Query, + cookies: [Cookie<'static>; 3], + session_id: Ulid, + username: &str, + user_id: &str, +) -> HttpResponse { let is_xhr = req.headers().contains_key("x-p-tenant") || req .headers() @@ -361,20 +380,19 @@ pub async fn reply_login( for cookie in cookies { response.cookie(cookie); } - Ok(response.json(serde_json::json!({ - "session": id.to_string(), + response.json(serde_json::json!({ + "session": session_id.to_string(), "username": username, "user_id": user_id, - }))) + })) } else { - // The state parameter is a nonce; resolve it to the original redirect URL. let redirect_url = login_query .state .as_deref() .and_then(consume_oauth_nonce) .unwrap_or_else(|| PARSEABLE.options.address.to_string()); - Ok(redirect_to_client(&redirect_url, cookies)) + redirect_to_client(&redirect_url, cookies) } } @@ -630,4 +648,4 @@ fn is_valid_redirect_url(base_url_without_scheme: &str, redirect_url: &str) -> b let redirect_url_without_scheme = http_scheme_match_regex.replace(redirect_url, ""); base_url_without_scheme == redirect_url_without_scheme -} \ No newline at end of file +} From 95f718e74f73caf3179b2dac3c9f7247fe7dd23b Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Thu, 12 Mar 2026 17:41:58 +1100 Subject: [PATCH 12/19] remove unwraps, fix put role issue, rever oidc changes --- src/handlers/http/cluster/mod.rs | 12 +++-- .../http/modal/ingest/ingestor_role.rs | 2 +- src/handlers/http/modal/query/querier_role.rs | 8 ++- src/handlers/http/oidc.rs | 54 ++++--------------- 4 files changed, 25 insertions(+), 51 deletions(-) diff --git a/src/handlers/http/cluster/mod.rs b/src/handlers/http/cluster/mod.rs index fac662ba1..c8454dbfb 100644 --- a/src/handlers/http/cluster/mod.rs +++ b/src/handlers/http/cluster/mod.rs @@ -537,7 +537,7 @@ pub async fn sync_users_with_roles_with_ingestors( let userid = userid.to_owned(); let headers = req.headers().clone(); let op = operation.to_string(); - let caller_userid = get_user_from_request(req).unwrap(); + let caller_userid = get_user_from_request(req)?; for_each_live_node(tenant_id, move |ingestor| { let url = format!( "{}{}/user/{}/role/sync/{}", @@ -588,7 +588,7 @@ pub async fn sync_user_deletion_with_ingestors( tenant_id: &Option, ) -> Result<(), RBACError> { let userid = userid.to_owned(); - let caller_userid = get_user_from_request(req).unwrap(); + let caller_userid = get_user_from_request(req)?; let headers = req.headers().clone(); for_each_live_node(tenant_id, move |ingestor| { let url = format!( @@ -697,7 +697,7 @@ pub async fn sync_password_reset_with_ingestors( ) -> Result<(), RBACError> { let userid = username.to_owned(); let tenant_id = get_tenant_id_from_request(&req); - let caller_userid = get_user_from_request(&req).unwrap(); + let caller_userid = get_user_from_request(&req)?; let headers = req.headers().clone(); for_each_live_node(&tenant_id, move |ingestor| { let url = format!( @@ -745,7 +745,8 @@ pub async fn sync_role_update( tenant_id: &Option, ) -> Result<(), RoleError> { let tenant = tenant_id.to_owned(); - let userid = get_user_from_request(req).unwrap(); + let userid = + get_user_from_request(req).map_err(|e| RoleError::Anyhow(anyhow::anyhow!("{e}")))?; let headers = req.headers().clone(); for_each_live_node(tenant_id, move |node| { let url = format!( @@ -794,7 +795,8 @@ pub async fn sync_role_delete( name: String, tenant_id: &Option, ) -> Result<(), RoleError> { - let userid = get_user_from_request(req).unwrap(); + let userid = + get_user_from_request(req).map_err(|e| RoleError::Anyhow(anyhow::anyhow!("{e}")))?; let headers = req.headers().clone(); for_each_live_node(tenant_id, move |node| { let url = format!( diff --git a/src/handlers/http/modal/ingest/ingestor_role.rs b/src/handlers/http/modal/ingest/ingestor_role.rs index dba9528e0..43c947250 100644 --- a/src/handlers/http/modal/ingest/ingestor_role.rs +++ b/src/handlers/http/modal/ingest/ingestor_role.rs @@ -44,7 +44,7 @@ pub async fn put( let name = name.into_inner(); let req_tenant_id = get_tenant_id_from_request(&req); let req_tenant = req_tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); - if req_tenant.ne(DEFAULT_TENANT) && (req_tenant_id.eq(&sync_req.tenant_id)) { + if req_tenant.ne(DEFAULT_TENANT) && (req_tenant_id.ne(&sync_req.tenant_id)) { return Err(RoleError::Anyhow(anyhow::Error::msg( "non super-admin user trying to create role for another tenant", ))); diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 5c1a9a770..1e8cef17a 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -104,7 +104,9 @@ pub async fn put( mut_sessions().remove_user(&userid, tenant); } - sync_role_update(&req, name.clone(), role, &tenant_id).await?; + if let Err(e) = sync_role_update(&req, name.clone(), role, &tenant_id).await { + tracing::error!("Failed to sync role update to cluster nodes: {e}"); + } Ok(HttpResponse::Ok().finish()) } @@ -171,7 +173,9 @@ pub async fn delete( mut_sessions().remove_user(&userid, tenant); } - sync_role_delete(&req, name.clone(), &tenant_id).await?; + if let Err(e) = sync_role_delete(&req, name.clone(), &tenant_id).await { + tracing::error!("Failed to sync role deletion to cluster nodes: {e}"); + } Ok(HttpResponse::Ok().finish()) } diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 0d7982a46..e00e5347b 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -17,8 +17,6 @@ */ use std::collections::HashSet; -use std::sync::LazyLock; -use std::time::Instant; use actix_web::http::StatusCode; use actix_web::{ @@ -28,9 +26,7 @@ use actix_web::{ web, }; use chrono::{Duration, TimeDelta}; -use dashmap::DashMap; use openid::Bearer; -use rand::distributions::{Alphanumeric, DistString}; use regex::Regex; use serde::Deserialize; use ulid::Ulid; @@ -54,31 +50,6 @@ use crate::{ }, }; -/// In-memory store mapping OAuth state nonces to redirect URLs. -/// Entries expire after 10 minutes to prevent stale nonces. -const OAUTH_NONCE_TTL: std::time::Duration = std::time::Duration::from_secs(600); -static OAUTH_NONCE_STORE: LazyLock> = - LazyLock::new(DashMap::new); - -/// Generate a cryptographic nonce, store it with the redirect URL, and return the nonce. -fn store_oauth_nonce(redirect: &str) -> String { - // Evict expired entries opportunistically - OAUTH_NONCE_STORE.retain(|_, (_, created)| created.elapsed() < OAUTH_NONCE_TTL); - let nonce = Alphanumeric.sample_string(&mut rand::thread_rng(), 32); - OAUTH_NONCE_STORE.insert(nonce.clone(), (redirect.to_string(), Instant::now())); - nonce -} - -/// Look up and consume a nonce, returning the associated redirect URL if valid. -fn consume_oauth_nonce(nonce: &str) -> Option { - let (_, (redirect, created)) = OAUTH_NONCE_STORE.remove(nonce)?; - if created.elapsed() < OAUTH_NONCE_TTL { - Some(redirect) - } else { - None - } -} - /// Struct representing query params returned from oidc provider #[derive(Deserialize, Debug)] pub struct Login { @@ -113,10 +84,9 @@ pub async fn login( (None, None) => return Ok(redirect_no_oauth_setup(query.redirect.clone())), (None, Some(client)) => { let redirect = query.into_inner().redirect.to_string(); - let nonce = store_oauth_nonce(&redirect); let scope = PARSEABLE.options.scope.to_string(); - let mut auth_url: String = client.read().await.auth_url(&scope, Some(nonce)).into(); + let mut auth_url: String = client.read().await.auth_url(&scope, Some(redirect)).into(); auth_url.push_str("&access_type=offline&prompt=consent"); return Ok(HttpResponse::TemporaryRedirect() @@ -169,12 +139,11 @@ pub async fn login( Users.remove_session(&key); if let Some(oidc_client) = oidc_client { let redirect = query.into_inner().redirect.to_string(); - let nonce = store_oauth_nonce(&redirect); let scope = PARSEABLE.options.scope.to_string(); let mut auth_url: String = oidc_client .read() .await - .auth_url(&scope, Some(nonce)) + .auth_url(&scope, Some(redirect)) .into(); auth_url.push_str("&access_type=offline&prompt=consent"); HttpResponse::TemporaryRedirect() @@ -339,13 +308,14 @@ fn resolve_roles( // Inherit roles from a native user with the same email (e.g. tenant owner via OAuth) if roles.is_empty() && let Some(email) = &user_info.email - && let Some(native) = metadata.users.iter().find(|u| { - matches!(u.ty, UserType::Native(_)) - && u.userid() == email.as_str() - && !u.roles.is_empty() - }) { - roles.clone_from(&native.roles); - } + && let Some(native) = metadata.users.iter().find(|u| { + matches!(u.ty, UserType::Native(_)) + && u.userid() == email.as_str() + && !u.roles.is_empty() + }) + { + roles.clone_from(&native.roles); + } roles } @@ -376,7 +346,6 @@ fn build_login_response( if is_xhr { let mut response = HttpResponse::Ok(); - response.insert_header((actix_web::http::header::CACHE_CONTROL, "no-store")); for cookie in cookies { response.cookie(cookie); } @@ -388,8 +357,7 @@ fn build_login_response( } else { let redirect_url = login_query .state - .as_deref() - .and_then(consume_oauth_nonce) + .clone() .unwrap_or_else(|| PARSEABLE.options.address.to_string()); redirect_to_client(&redirect_url, cookies) From 90aa303596a8467a46830cb110e0e83d5b79cf1b Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Thu, 12 Mar 2026 19:14:52 +1100 Subject: [PATCH 13/19] add caller user id in all sync calls --- src/handlers/http/cluster/mod.rs | 24 +++++---- src/handlers/http/modal/query/querier_rbac.rs | 52 +++++++++++++++---- src/handlers/http/modal/query/querier_role.rs | 12 +++-- 3 files changed, 63 insertions(+), 25 deletions(-) diff --git a/src/handlers/http/cluster/mod.rs b/src/handlers/http/cluster/mod.rs index c8454dbfb..247f86325 100644 --- a/src/handlers/http/cluster/mod.rs +++ b/src/handlers/http/cluster/mod.rs @@ -52,9 +52,7 @@ use crate::rbac::role::model::Role; use crate::rbac::user::User; use crate::stats::Stats; use crate::storage::{ObjectStorageError, ObjectStoreFormat}; -use crate::utils::{ - create_intracluster_auth_headermap, get_tenant_id_from_request, get_user_from_request, -}; +use crate::utils::{create_intracluster_auth_headermap, get_tenant_id_from_request}; use super::base_path_without_preceding_slash; use super::ingest::PostError; @@ -523,6 +521,7 @@ pub async fn sync_users_with_roles_with_ingestors( role: &HashSet, operation: &str, tenant_id: &Option, + caller_userid: &str, ) -> Result<(), RBACError> { match operation { "add" | "remove" => {} @@ -537,7 +536,7 @@ pub async fn sync_users_with_roles_with_ingestors( let userid = userid.to_owned(); let headers = req.headers().clone(); let op = operation.to_string(); - let caller_userid = get_user_from_request(req)?; + let caller_userid = caller_userid.to_owned(); for_each_live_node(tenant_id, move |ingestor| { let url = format!( "{}{}/user/{}/role/sync/{}", @@ -586,9 +585,10 @@ pub async fn sync_user_deletion_with_ingestors( req: &HttpRequest, userid: &str, tenant_id: &Option, + caller_userid: &str, ) -> Result<(), RBACError> { let userid = userid.to_owned(); - let caller_userid = get_user_from_request(req)?; + let caller_userid = caller_userid.to_owned(); let headers = req.headers().clone(); for_each_live_node(tenant_id, move |ingestor| { let url = format!( @@ -634,6 +634,7 @@ pub async fn sync_user_creation( user: User, role: &Option>, tenant_id: &Option, + caller_userid: &str, ) -> Result<(), RBACError> { let mut user = user.clone(); @@ -647,7 +648,7 @@ pub async fn sync_user_creation( RBACError::SerdeError(err) })?; - let caller_userid = get_user_from_request(req)?; + let caller_userid = caller_userid.to_owned(); let userid = userid.to_string(); let headers = req.headers().clone(); for_each_live_node(tenant_id, move |node| { @@ -694,10 +695,11 @@ pub async fn sync_user_creation( pub async fn sync_password_reset_with_ingestors( req: HttpRequest, username: &str, + caller_userid: &str, ) -> Result<(), RBACError> { let userid = username.to_owned(); let tenant_id = get_tenant_id_from_request(&req); - let caller_userid = get_user_from_request(&req)?; + let caller_userid = caller_userid.to_owned(); let headers = req.headers().clone(); for_each_live_node(&tenant_id, move |ingestor| { let url = format!( @@ -743,10 +745,10 @@ pub async fn sync_role_update( name: String, role: Role, tenant_id: &Option, + caller_userid: &str, ) -> Result<(), RoleError> { let tenant = tenant_id.to_owned(); - let userid = - get_user_from_request(req).map_err(|e| RoleError::Anyhow(anyhow::anyhow!("{e}")))?; + let userid = caller_userid.to_owned(); let headers = req.headers().clone(); for_each_live_node(tenant_id, move |node| { let url = format!( @@ -794,9 +796,9 @@ pub async fn sync_role_delete( req: &HttpRequest, name: String, tenant_id: &Option, + caller_userid: &str, ) -> Result<(), RoleError> { - let userid = - get_user_from_request(req).map_err(|e| RoleError::Anyhow(anyhow::anyhow!("{e}")))?; + let userid = caller_userid.to_owned(); let headers = req.headers().clone(); for_each_live_node(tenant_id, move |node| { let url = format!( diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index fc396fa06..77fb81430 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -35,7 +35,7 @@ use crate::{ map::{roles, users, write_user_groups}, user::{self, UserType}, }, - utils::get_tenant_id_from_request, + utils::{get_tenant_id_from_request, get_user_from_request}, validator, }; @@ -48,6 +48,7 @@ pub async fn post_user( ) -> Result { let username = userid.into_inner(); let tenant_id = get_tenant_id_from_request(&req); + let caller_userid = get_user_from_request(&req)?; validator::user_role_name(&username)?; let mut metadata = get_metadata(&tenant_id).await?; @@ -90,7 +91,7 @@ pub async fn post_user( // let created_role = user_roles.clone(); Users.put_user(user.clone()); - sync_user_creation(&req, user, &None, &tenant_id).await?; + sync_user_creation(&req, user, &None, &tenant_id, &caller_userid).await?; Ok(password) } @@ -102,6 +103,7 @@ pub async fn delete_user( ) -> Result { let userid = userid.into_inner(); let tenant_id = get_tenant_id_from_request(&req); + let caller_userid = get_user_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); let _guard = UPDATE_LOCK.lock().await; // fail this request if the user does not exist @@ -158,7 +160,7 @@ pub async fn delete_user( } put_metadata(&metadata, &tenant_id).await?; - sync_user_deletion_with_ingestors(&req, &userid, &tenant_id).await?; + sync_user_deletion_with_ingestors(&req, &userid, &tenant_id, &caller_userid).await?; // update in mem table Users.delete_user(&userid, &tenant_id); @@ -174,6 +176,7 @@ pub async fn add_roles_to_user( let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); let tenant_id = get_tenant_id_from_request(&req); + let caller_userid = get_user_from_request(&req)?; if !Users.contains(&userid, &tenant_id) { return Err(RBACError::UserDoesNotExist); }; @@ -217,10 +220,23 @@ pub async fn add_roles_to_user( } put_metadata(&metadata, &tenant_id).await?; - // update in mem table - Users.add_roles(&userid.clone(), roles_to_add.clone(), &tenant_id); - sync_users_with_roles_with_ingestors(&req, &userid, &roles_to_add, "add", &tenant_id).await?; + // sync to other nodes before updating in-memory (which invalidates sessions) + if let Err(e) = sync_users_with_roles_with_ingestors( + &req, + &userid, + &roles_to_add, + "add", + &tenant_id, + &caller_userid, + ) + .await + { + tracing::error!("Failed to sync role addition to cluster nodes: {e}"); + } + + // update in mem table (this invalidates the user's session) + Users.add_roles(&userid.clone(), roles_to_add.clone(), &tenant_id); Ok(format!("Roles updated successfully for {username}")) } @@ -234,6 +250,7 @@ pub async fn remove_roles_from_user( let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); let tenant_id = get_tenant_id_from_request(&req); + let caller_userid = get_user_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); let _guard = UPDATE_LOCK.lock().await; @@ -291,11 +308,23 @@ pub async fn remove_roles_from_user( } put_metadata(&metadata, &tenant_id).await?; - // update in mem table - Users.remove_roles(&userid.clone(), roles_to_remove.clone(), &tenant_id); - sync_users_with_roles_with_ingestors(&req, &userid, &roles_to_remove, "remove", &tenant_id) - .await?; + // sync to other nodes before updating in-memory (which invalidates sessions) + if let Err(e) = sync_users_with_roles_with_ingestors( + &req, + &userid, + &roles_to_remove, + "remove", + &tenant_id, + &caller_userid, + ) + .await + { + tracing::error!("Failed to sync role removal to cluster nodes: {e}"); + } + + // update in mem table (this invalidates the user's session) + Users.remove_roles(&userid.clone(), roles_to_remove.clone(), &tenant_id); Ok(HttpResponse::Ok().json(format!("Roles updated successfully for {username}"))) } @@ -310,6 +339,7 @@ pub async fn post_gen_password( let mut new_password = String::default(); let mut new_hash = String::default(); let tenant_id = get_tenant_id_from_request(&req); + let caller_userid = get_user_from_request(&req)?; let mut metadata = get_metadata(&tenant_id).await?; let _guard = UPDATE_LOCK.lock().await; @@ -332,7 +362,7 @@ pub async fn post_gen_password( put_metadata(&metadata, &tenant_id).await?; Users.change_password_hash(&username, &new_hash, &tenant_id); - sync_password_reset_with_ingestors(req, &username).await?; + sync_password_reset_with_ingestors(req, &username, &caller_userid).await?; Ok(new_password) } diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 1e8cef17a..25eef5ddd 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -34,7 +34,7 @@ use crate::{ map::{mut_roles, mut_sessions, read_user_groups, roles, users}, role::model::{Role, RoleType}, }, - utils::get_tenant_id_from_request, + utils::{get_tenant_id_from_request, get_user_from_request}, validator, }; @@ -48,6 +48,9 @@ pub async fn put( let name = name.into_inner(); let tenant_id = get_tenant_id_from_request(&req); let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + // extract caller userid early, before any session mutations + let caller_userid = + get_user_from_request(&req).map_err(|e| RoleError::Anyhow(anyhow::anyhow!("{e}")))?; // validate the role name validator::user_role_name(&name).map_err(RoleError::ValidationError)?; @@ -104,7 +107,7 @@ pub async fn put( mut_sessions().remove_user(&userid, tenant); } - if let Err(e) = sync_role_update(&req, name.clone(), role, &tenant_id).await { + if let Err(e) = sync_role_update(&req, name.clone(), role, &tenant_id, &caller_userid).await { tracing::error!("Failed to sync role update to cluster nodes: {e}"); } @@ -120,6 +123,9 @@ pub async fn delete( let name = name.into_inner(); let tenant_id = get_tenant_id_from_request(&req); let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + // extract caller userid early, before any session mutations + let caller_userid = + get_user_from_request(&req).map_err(|e| RoleError::Anyhow(anyhow::anyhow!("{e}")))?; if let Some(tenant_roles) = roles().get(tenant) && let Some(role) = tenant_roles.get(&name) && role.role_type().eq(&RoleType::Internal) @@ -173,7 +179,7 @@ pub async fn delete( mut_sessions().remove_user(&userid, tenant); } - if let Err(e) = sync_role_delete(&req, name.clone(), &tenant_id).await { + if let Err(e) = sync_role_delete(&req, name.clone(), &tenant_id, &caller_userid).await { tracing::error!("Failed to sync role deletion to cluster nodes: {e}"); } From 6714901afc25f1f6981446a858553e951235b775 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Thu, 12 Mar 2026 19:46:14 +1100 Subject: [PATCH 14/19] refresh user permissions instead of removing the session --- src/handlers/http/modal/query/querier_role.rs | 25 +++++++-- src/rbac/map.rs | 18 ++++++ src/rbac/mod.rs | 6 +- src/rbac/user.rs | 55 +++++++++++++------ 4 files changed, 81 insertions(+), 23 deletions(-) diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 25eef5ddd..82cfcf371 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -33,6 +33,7 @@ use crate::{ rbac::{ map::{mut_roles, mut_sessions, read_user_groups, roles, users}, role::model::{Role, RoleType}, + roles_to_permission, }, utils::{get_tenant_id_from_request, get_user_from_request}, validator, @@ -103,8 +104,16 @@ pub async fn put( } } - for userid in session_refresh_users { - mut_sessions().remove_user(&userid, tenant); + { + let mut sessions = mut_sessions(); + for userid in &session_refresh_users { + if let Some(tenant_users) = users().get(tenant) + && let Some(user) = tenant_users.get(userid) + { + let new_perms = roles_to_permission(user.roles(), tenant); + sessions.refresh_user_permissions(userid, tenant, new_perms); + } + } } if let Err(e) = sync_role_update(&req, name.clone(), role, &tenant_id, &caller_userid).await { @@ -175,8 +184,16 @@ pub async fn delete( } } - for userid in session_refresh_users { - mut_sessions().remove_user(&userid, tenant); + { + let mut sessions = mut_sessions(); + for userid in &session_refresh_users { + if let Some(tenant_users) = users().get(tenant) + && let Some(user) = tenant_users.get(userid) + { + let new_perms = roles_to_permission(user.roles(), tenant); + sessions.refresh_user_permissions(userid, tenant, new_perms); + } + } } if let Err(e) = sync_role_delete(&req, name.clone(), &tenant_id, &caller_userid).await { diff --git a/src/rbac/map.rs b/src/rbac/map.rs index ab158f11a..025abfa82 100644 --- a/src/rbac/map.rs +++ b/src/rbac/map.rs @@ -277,6 +277,24 @@ impl Sessions { } } + // refresh permissions for all sessions belonging to a user + pub fn refresh_user_permissions( + &mut self, + username: &str, + tenant_id: &str, + new_permissions: Vec, + ) { + if let Some(tenant_sessions) = self.user_sessions.get(tenant_id) + && let Some(sessions) = tenant_sessions.get(username) + { + for (key, _) in sessions { + if let Some((_, _, perms)) = self.active_sessions.get_mut(key) { + *perms = new_permissions.clone(); + } + } + } + } + // remove sessions related to a user pub fn remove_user(&mut self, username: &str, tenant_id: &str) { let sessions = if let Some(tenant_sessions) = self.user_sessions.get_mut(tenant_id) { diff --git a/src/rbac/mod.rs b/src/rbac/mod.rs index 25dea5a6a..6d9219f60 100644 --- a/src/rbac/mod.rs +++ b/src/rbac/mod.rs @@ -163,7 +163,8 @@ impl Users { && let Some(user) = users.get_mut(userid) { user.roles.extend(roles); - mut_sessions().remove_user(userid, tenant_id) + let new_perms = roles_to_permission(user.roles(), tenant_id); + mut_sessions().refresh_user_permissions(userid, tenant_id, new_perms); }; } @@ -174,7 +175,8 @@ impl Users { { let diff = HashSet::from_iter(user.roles.difference(&roles).cloned()); user.roles = diff; - mut_sessions().remove_user(userid, tenant_id) + let new_perms = roles_to_permission(user.roles(), tenant_id); + mut_sessions().refresh_user_permissions(userid, tenant_id, new_perms); }; } diff --git a/src/rbac/user.rs b/src/rbac/user.rs index 5e1a6a81f..12373b4e0 100644 --- a/src/rbac/user.rs +++ b/src/rbac/user.rs @@ -35,6 +35,7 @@ use crate::{ rbac::{ map::{mut_sessions, read_user_groups, roles, users}, role::model::RoleType, + roles_to_permission, }, }; @@ -464,9 +465,15 @@ impl UserGroup { } } self.roles.extend(roles_to_add); - // also refresh all user sessions + // refresh permissions for all user sessions in this group + let mut sessions = mut_sessions(); for group_user in &self.users { - mut_sessions().remove_user(group_user.userid(), tenant_id); + if let Some(tenant_users) = users().get(tenant_id) + && let Some(user) = tenant_users.get(group_user.userid()) + { + let new_perms = roles_to_permission(user.roles(), tenant_id); + sessions.refresh_user_permissions(group_user.userid(), tenant_id, new_perms); + } } Ok(()) } @@ -476,12 +483,17 @@ impl UserGroup { return Ok(()); } self.users.extend(users.clone()); - // also refresh all user sessions + // refresh permissions for newly added user sessions + let mut sessions = mut_sessions(); + let all_users = super::map::users(); for group_user in &users { - mut_sessions().remove_user( - group_user.userid(), - group_user.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT), - ); + let tid = group_user.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + if let Some(tenant_users) = all_users.get(tid) + && let Some(user) = tenant_users.get(group_user.userid()) + { + let new_perms = roles_to_permission(user.roles(), tid); + sessions.refresh_user_permissions(group_user.userid(), tid, new_perms); + } } Ok(()) } @@ -498,12 +510,16 @@ impl UserGroup { } self.roles.clone_from(&new_roles); - // also refresh all user sessions + // refresh permissions for all user sessions in this group + let mut sessions = mut_sessions(); for group_user in &self.users { - mut_sessions().remove_user( - group_user.userid(), - group_user.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT), - ); + let tid = group_user.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + if let Some(tenant_users) = users().get(tid) + && let Some(user) = tenant_users.get(group_user.userid()) + { + let new_perms = roles_to_permission(user.roles(), tid); + sessions.refresh_user_permissions(group_user.userid(), tid, new_perms); + } } Ok(()) } @@ -517,12 +533,17 @@ impl UserGroup { if removed_users.is_empty() { return Ok(()); } - // also refresh all user sessions + // refresh permissions for removed user sessions + let mut sessions = mut_sessions(); + let all_users = super::map::users(); for group_user in &removed_users { - mut_sessions().remove_user( - group_user.userid(), - group_user.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT), - ); + let tid = group_user.tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + if let Some(tenant_users) = all_users.get(tid) + && let Some(user) = tenant_users.get(group_user.userid()) + { + let new_perms = roles_to_permission(user.roles(), tid); + sessions.refresh_user_permissions(group_user.userid(), tid, new_perms); + } } self.users.clone_from(&new_users); From c755e3c42b3e0ff9f925fa2491fdee5e182e25f3 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Thu, 12 Mar 2026 20:39:35 +1100 Subject: [PATCH 15/19] deepsource fix --- src/handlers/http/modal/query/querier_role.rs | 4 ++-- src/rbac/map.rs | 9 +++++---- src/rbac/mod.rs | 4 ++-- src/rbac/user.rs | 8 ++++---- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 82cfcf371..321efa9b0 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -111,7 +111,7 @@ pub async fn put( && let Some(user) = tenant_users.get(userid) { let new_perms = roles_to_permission(user.roles(), tenant); - sessions.refresh_user_permissions(userid, tenant, new_perms); + sessions.refresh_user_permissions(userid, tenant, &new_perms); } } } @@ -191,7 +191,7 @@ pub async fn delete( && let Some(user) = tenant_users.get(userid) { let new_perms = roles_to_permission(user.roles(), tenant); - sessions.refresh_user_permissions(userid, tenant, new_perms); + sessions.refresh_user_permissions(userid, tenant, &new_perms); } } } diff --git a/src/rbac/map.rs b/src/rbac/map.rs index 025abfa82..b8f805441 100644 --- a/src/rbac/map.rs +++ b/src/rbac/map.rs @@ -282,14 +282,15 @@ impl Sessions { &mut self, username: &str, tenant_id: &str, - new_permissions: Vec, + new_permissions: &[Permission], ) { if let Some(tenant_sessions) = self.user_sessions.get(tenant_id) && let Some(sessions) = tenant_sessions.get(username) { - for (key, _) in sessions { - if let Some((_, _, perms)) = self.active_sessions.get_mut(key) { - *perms = new_permissions.clone(); + let keys: Vec<_> = sessions.iter().map(|(key, _)| key.clone()).collect(); + for key in keys { + if let Some((_, _, perms)) = self.active_sessions.get_mut(&key) { + *perms = new_permissions.to_vec(); } } } diff --git a/src/rbac/mod.rs b/src/rbac/mod.rs index 6d9219f60..20d702357 100644 --- a/src/rbac/mod.rs +++ b/src/rbac/mod.rs @@ -164,7 +164,7 @@ impl Users { { user.roles.extend(roles); let new_perms = roles_to_permission(user.roles(), tenant_id); - mut_sessions().refresh_user_permissions(userid, tenant_id, new_perms); + mut_sessions().refresh_user_permissions(userid, tenant_id, &new_perms); }; } @@ -176,7 +176,7 @@ impl Users { let diff = HashSet::from_iter(user.roles.difference(&roles).cloned()); user.roles = diff; let new_perms = roles_to_permission(user.roles(), tenant_id); - mut_sessions().refresh_user_permissions(userid, tenant_id, new_perms); + mut_sessions().refresh_user_permissions(userid, tenant_id, &new_perms); }; } diff --git a/src/rbac/user.rs b/src/rbac/user.rs index 12373b4e0..f661a64cb 100644 --- a/src/rbac/user.rs +++ b/src/rbac/user.rs @@ -472,7 +472,7 @@ impl UserGroup { && let Some(user) = tenant_users.get(group_user.userid()) { let new_perms = roles_to_permission(user.roles(), tenant_id); - sessions.refresh_user_permissions(group_user.userid(), tenant_id, new_perms); + sessions.refresh_user_permissions(group_user.userid(), tenant_id, &new_perms); } } Ok(()) @@ -492,7 +492,7 @@ impl UserGroup { && let Some(user) = tenant_users.get(group_user.userid()) { let new_perms = roles_to_permission(user.roles(), tid); - sessions.refresh_user_permissions(group_user.userid(), tid, new_perms); + sessions.refresh_user_permissions(group_user.userid(), tid, &new_perms); } } Ok(()) @@ -518,7 +518,7 @@ impl UserGroup { && let Some(user) = tenant_users.get(group_user.userid()) { let new_perms = roles_to_permission(user.roles(), tid); - sessions.refresh_user_permissions(group_user.userid(), tid, new_perms); + sessions.refresh_user_permissions(group_user.userid(), tid, &new_perms); } } Ok(()) @@ -542,7 +542,7 @@ impl UserGroup { && let Some(user) = tenant_users.get(group_user.userid()) { let new_perms = roles_to_permission(user.roles(), tid); - sessions.refresh_user_permissions(group_user.userid(), tid, new_perms); + sessions.refresh_user_permissions(group_user.userid(), tid, &new_perms); } } self.users.clone_from(&new_users); From 87e401561c1d1568b2acfd6ca5fe6f3e5b5861db Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Thu, 12 Mar 2026 21:50:19 +1100 Subject: [PATCH 16/19] super-admin oauth login --- src/handlers/http/oidc.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index e00e5347b..d892f781f 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -469,6 +469,15 @@ pub async fn put_user( bearer: Bearer, tenant: Option, ) -> Result { + // If the userid matches the super admin (P_USERNAME), return the existing + // Native user as-is. This prevents overwriting the super admin with an + // OAuth user while still allowing OAuth login to create a session. + if userid == PARSEABLE.options.username { + if let Some(user) = Users.get_user(userid, &tenant) { + return Ok(user); + } + } + let mut metadata = get_metadata(&tenant).await?; let mut user = metadata From 08cdb38617afe515ae747e33d0fe27d8b8e07be6 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Thu, 12 Mar 2026 21:56:05 +1100 Subject: [PATCH 17/19] clippy fix --- src/handlers/http/oidc.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index d892f781f..6cda5dc9e 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -472,10 +472,10 @@ pub async fn put_user( // If the userid matches the super admin (P_USERNAME), return the existing // Native user as-is. This prevents overwriting the super admin with an // OAuth user while still allowing OAuth login to create a session. - if userid == PARSEABLE.options.username { - if let Some(user) = Users.get_user(userid, &tenant) { - return Ok(user); - } + if userid == PARSEABLE.options.username + && let Some(user) = Users.get_user(userid, &tenant) + { + return Ok(user); } let mut metadata = get_metadata(&tenant).await?; From e2dc5d73380eb4fdec5cf47c6501ca7120eed334 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Thu, 12 Mar 2026 22:24:55 +1100 Subject: [PATCH 18/19] add owner to tenant metadata --- src/handlers/http/modal/query/querier_rbac.rs | 20 ++++++++++++++++++- src/storage/store_metadata.rs | 3 +++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 77fb81430..a3eebc89c 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -29,12 +29,13 @@ use crate::{ modal::utils::rbac_utils::{get_metadata, put_metadata}, rbac::{RBACError, UPDATE_LOCK}, }, - parseable::DEFAULT_TENANT, + parseable::{DEFAULT_TENANT, PARSEABLE}, rbac::{ Users, map::{roles, users, write_user_groups}, user::{self, UserType}, }, + tenants::TENANT_METADATA, utils::{get_tenant_id_from_request, get_user_from_request}, validator, }; @@ -292,6 +293,23 @@ pub async fn remove_roles_from_user( ))); } + // In multi-tenant, prevent removing the admin role from the tenant owner + if PARSEABLE.options.is_multi_tenant() + && roles_to_remove.contains("admin") + && let Some(tid) = tenant_id.as_deref() + { + let is_owner = TENANT_METADATA + .get_tenant_meta(tid) + .and_then(|meta| meta.owner) + .map(|owner| owner == userid) + .unwrap_or(false); + if is_owner { + return Err(RBACError::InvalidDeletionRequest( + "Cannot remove the admin role from the tenant owner".to_string(), + )); + } + } + // update parseable.json first let mut metadata = get_metadata(&tenant_id).await?; if let Some(user) = metadata diff --git a/src/storage/store_metadata.rs b/src/storage/store_metadata.rs index 73b6615d3..fbb157072 100644 --- a/src/storage/store_metadata.rs +++ b/src/storage/store_metadata.rs @@ -78,6 +78,8 @@ pub struct StorageMetadata { pub end_date: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub plan: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub owner: Option, } impl Default for StorageMetadata { @@ -100,6 +102,7 @@ impl Default for StorageMetadata { start_date: None, end_date: None, plan: None, + owner: None, } } } From b4e025806fd596a5246722f98b8fbfe5fd0c09c4 Mon Sep 17 00:00:00 2001 From: Nikhil Sinha Date: Fri, 13 Mar 2026 01:37:59 +1100 Subject: [PATCH 19/19] SameSite::None only for clerk setup --- src/handlers/http/oidc.rs | 44 +++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 6cda5dc9e..51d1355cd 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -17,6 +17,7 @@ */ use std::collections::HashSet; +use std::sync::atomic::{AtomicBool, Ordering}; use actix_web::http::StatusCode; use actix_web::{ @@ -25,6 +26,14 @@ use actix_web::{ http::header::ContentType, web, }; + +/// When set to true, cookies use SameSite::None + Secure (required for Clerk OAuth). +/// Enterprise sets this when P_CLERK_SECRET is configured. +static COOKIE_REQUIRE_CROSS_SITE: AtomicBool = AtomicBool::new(false); + +pub fn set_cookie_cross_site(enabled: bool) { + COOKIE_REQUIRE_CROSS_SITE.store(enabled, Ordering::Relaxed); +} use chrono::{Duration, TimeDelta}; use openid::Bearer; use regex::Regex; @@ -433,31 +442,30 @@ fn redirect_no_oauth_setup(mut url: Url) -> HttpResponse { response.finish() } -pub fn cookie_session(id: Ulid) -> Cookie<'static> { - Cookie::build(SESSION_COOKIE_NAME, id.to_string()) +fn build_cookie(name: &str, value: String) -> Cookie<'static> { + let mut cookie = Cookie::build(name.to_string(), value) .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64)) - .same_site(SameSite::None) - .secure(true) - .path("/") - .finish() + .path("/".to_string()); + + if COOKIE_REQUIRE_CROSS_SITE.load(Ordering::Relaxed) { + cookie = cookie.same_site(SameSite::None).secure(true); + } else { + cookie = cookie.same_site(SameSite::Lax); + } + + cookie.finish() +} + +pub fn cookie_session(id: Ulid) -> Cookie<'static> { + build_cookie(SESSION_COOKIE_NAME, id.to_string()) } 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)) - .same_site(SameSite::None) - .secure(true) - .path("/") - .finish() + build_cookie(USER_COOKIE_NAME, username.to_string()) } 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::None) - .secure(true) - .path("/") - .finish() + build_cookie(USER_ID_COOKIE_NAME, user_id.to_string()) } // put new user in metadata if does not exit