From 12649031f29207fb1b16ee7440c271c756d26fb5 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Tue, 14 Jul 2026 17:19:59 +0200 Subject: [PATCH] refactor: simplify on_behalf_of deploy guard and prune redundant job-token denylist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token-authority cap (require_super_admin/is_super_admin/require_devops_role deny job tokens) is the actual fix, so: - validate_on_behalf_of is now sync and sentinel-only. Dropped the belong-to check (superadmin on_behalf_of_email must match on_behalf_of): it false-rejected legitimate deploys (a superadmin who is not a member of the deploying workspace with automate_username_creation off resolves to `…@unknown.windmill.dev`; pre-existing inconsistent apps failed on redeploy) and the cap already closes the escalation at execution. - Removed forbid_superadmin_job_token from the 9 routes that call require_super_admin first (now redundant with the cap). Kept it on the self-service routes with no require_super_admin: create_token, set_password, create_user. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/tests/preserve_on_behalf_of.rs | 13 +++--- backend/windmill-api-flows/src/flows.rs | 10 +---- backend/windmill-api-schedule/src/lib.rs | 10 +---- backend/windmill-api-scripts/src/scripts.rs | 5 +-- backend/windmill-api-users/src/users.rs | 12 ----- .../windmill-api-workspaces/src/workspaces.rs | 1 - backend/windmill-api/src/apps.rs | 10 +---- backend/windmill-api/src/offboarding.rs | 6 +-- backend/windmill-api/src/users.rs | 4 -- backend/windmill-common/src/auth.rs | 44 ++++--------------- backend/windmill-trigger/src/handler.rs | 10 +---- 11 files changed, 28 insertions(+), 97 deletions(-) diff --git a/backend/tests/preserve_on_behalf_of.rs b/backend/tests/preserve_on_behalf_of.rs index 6b27b4300b..c15c516775 100644 --- a/backend/tests/preserve_on_behalf_of.rs +++ b/backend/tests/preserve_on_behalf_of.rs @@ -2856,13 +2856,16 @@ async fn test_reject_forged_superadmin_on_behalf_of(db: Pool) -> anyho resp.text().await? ); - // App: deployer cannot pin a real superadmin's email onto an unrelated principal. + // App: a real superadmin on_behalf_of is *allowed* at deploy (deployers may + // deploy on behalf of any real user). The escalation is closed at execution + // by the job-token cap, not by restricting what can be stored, so even a + // superadmin email pinned onto an unrelated principal deploys fine here. let resp = authed( client().post(format!("{base}/apps/create")), "DEPLOYER_TOKEN", ) .json(&new_app_with_on_behalf_of( - "u/deployer-user/app_mismatch_sa", + "u/deployer-user/app_real_sa", Some("u/original-user"), Some(REAL_SA), true, @@ -2871,12 +2874,12 @@ async fn test_reject_forged_superadmin_on_behalf_of(db: Pool) -> anyho .await?; assert_eq!( resp.status(), - 400, - "deployer must not pin a superadmin email onto an unrelated principal: {}", + 201, + "a real superadmin on_behalf_of is allowed at deploy (capped at execution): {}", resp.text().await? ); - // App: a consistently named real superadmin identity is allowed (deployer feature). + // App: a consistently named real superadmin identity is likewise allowed. let resp = authed( client().post(format!("{base}/apps/create")), "DEPLOYER_TOKEN", diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index eca878ee8e..691b8b02a0 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -599,12 +599,9 @@ async fn create_flow( && windmill_common::can_preserve_on_behalf_of(&authed) { windmill_common::auth::validate_on_behalf_of( - &db, - &w_id, None, nf.on_behalf_of_email.as_deref(), - ) - .await?; + )?; } let mut tx = user_db.clone().begin(&authed).await?; @@ -1053,12 +1050,9 @@ async fn update_flow( && windmill_common::can_preserve_on_behalf_of(&authed) { windmill_common::auth::validate_on_behalf_of( - &db, - &w_id, None, nf.on_behalf_of_email.as_deref(), - ) - .await?; + )?; } let authed = maybe_refresh_folders(&flow_path, &w_id, authed, &db).await; diff --git a/backend/windmill-api-schedule/src/lib.rs b/backend/windmill-api-schedule/src/lib.rs index 5b3a8f2308..fb92acff83 100644 --- a/backend/windmill-api-schedule/src/lib.rs +++ b/backend/windmill-api-schedule/src/lib.rs @@ -309,12 +309,9 @@ async fn create_schedule( // Reject a forged superadmin run identity in a preserved permissioned_as // (the sentinel guard; the email is derived from it so it always belongs). windmill_common::auth::validate_on_behalf_of( - &db, - &w_id, Some(&resolved_permissioned_as), Some(&resolved_email), - ) - .await?; + )?; let schedule = sqlx::query_as!( Schedule, @@ -531,12 +528,9 @@ async fn edit_schedule( // Reject a forged superadmin run identity in a preserved permissioned_as // (the sentinel guard; the email is derived from it so it always belongs). windmill_common::auth::validate_on_behalf_of( - &db, - &w_id, Some(&resolved_permissioned_as), Some(&resolved_email), - ) - .await?; + )?; let schedule = sqlx::query_as!( Schedule, diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 099d799fea..fa1e63f853 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -985,12 +985,9 @@ async fn create_script_internal<'c>( && windmill_common::can_preserve_on_behalf_of(&authed) { windmill_common::auth::validate_on_behalf_of( - &db, - &w_id, None, ns.on_behalf_of_email.as_deref(), - ) - .await?; + )?; } if sqlx::query_scalar!( "SELECT 1 FROM script WHERE hash = $1 AND workspace_id = $2", diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 37e756ba2a..0b48e346cb 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -1426,13 +1426,11 @@ async fn convert_user_to_group( async fn update_user( authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Path(email_to_update): Path, Extension(db): Extension, Json(eu): Json, ) -> Result { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; let mut new_super_admin: Option = None; @@ -1603,12 +1601,10 @@ async fn update_user( async fn delete_user( authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Path(email_to_delete): Path, Extension(db): Extension, ) -> Result { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; sqlx::query!("DELETE FROM token WHERE email = $1", &email_to_delete) @@ -1901,11 +1897,9 @@ async fn set_login_type( Extension(db): Extension, Path(email): Path, authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Json(et): Json, ) -> Result { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; sqlx::query!( @@ -2206,7 +2200,6 @@ async fn create_token( async fn impersonate( Extension(db): Extension, authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Json(new_token): Json, ) -> Result<(StatusCode, String)> { use windmill_common::min_version::MIN_VERSION_SUPPORTS_TOKEN_HASH; @@ -2220,7 +2213,6 @@ async fn impersonate( Some(&token) }; require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; if new_token.impersonate_email.is_none() { return Err(Error::BadRequest( @@ -2745,10 +2737,8 @@ struct ExportedGlobalUser { async fn export_global_users( Extension(db): Extension, authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, ) -> JsonResult> { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; let users = sqlx::query_as!( ExportedGlobalUser, @@ -2784,11 +2774,9 @@ async fn export_global_users() -> JsonResult { async fn overwrite_global_users( Extension(db): Extension, authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Json(users): Json>, ) -> Result { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; sqlx::query!("DELETE FROM password") .execute(&mut *tx) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index bf79b03de8..8ff038d469 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3768,7 +3768,6 @@ async fn list_workspaces_as_super_admin( Extension(db): Extension, Extension(user_db): Extension, Query(pagination): Query, - ApiAuthed { email, .. }: ApiAuthed, ) -> JsonResult> { require_devops_role(&db, &authed).await?; let (per_page, offset) = paginate(pagination); diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index ae3a4a52c7..1a4d334de8 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -1938,12 +1938,9 @@ async fn create_app_internal<'a>( // Done on the non-RLS pool before the transaction below, like the resolution // above, to avoid holding a second connection while `tx` is checked out. windmill_common::auth::validate_on_behalf_of( - &db, - w_id, app.policy.on_behalf_of.as_deref(), app.policy.on_behalf_of_email.as_deref(), - ) - .await?; + )?; let mut tx = user_db.clone().begin(&authed).await?; let path = app.path.clone(); @@ -2466,12 +2463,9 @@ async fn update_app_internal<'a>( && npolicy.on_behalf_of.is_some(); if should_preserve { windmill_common::auth::validate_on_behalf_of( - &db, - w_id, npolicy.on_behalf_of.as_deref(), npolicy.on_behalf_of_email.as_deref(), - ) - .await?; + )?; } } diff --git a/backend/windmill-api/src/offboarding.rs b/backend/windmill-api/src/offboarding.rs index 7192576c65..0b7bd7e3da 100644 --- a/backend/windmill-api/src/offboarding.rs +++ b/backend/windmill-api/src/offboarding.rs @@ -1,13 +1,13 @@ use std::collections::HashMap; -use crate::db::{ApiAuthed, OptJobAuthed}; +use crate::db::ApiAuthed; use crate::secret_backend_ext::rename_vault_secrets_with_prefix; use axum::{ extract::{Extension, Path}, Json, }; use serde::{Deserialize, Serialize}; -use windmill_api_auth::{forbid_superadmin_job_token, require_super_admin}; +use windmill_api_auth::require_super_admin; use windmill_api_users::users::delete_workspace_user_internal; use windmill_audit::audit_oss::audit_log; use windmill_audit::ActionKind; @@ -483,13 +483,11 @@ pub(crate) async fn global_offboard_preview( pub(crate) async fn offboard_global_user( authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Extension(db): Extension, Path(email): Path, Json(req): Json, ) -> Result> { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let workspaces = sqlx::query!( "SELECT workspace_id, username FROM usr WHERE email = $1", diff --git a/backend/windmill-api/src/users.rs b/backend/windmill-api/src/users.rs index 4c3e578307..0bb6a3087e 100644 --- a/backend/windmill-api/src/users.rs +++ b/backend/windmill-api/src/users.rs @@ -156,11 +156,9 @@ async fn set_password_of_user( Extension(argon2): Extension>>, Path(email): Path, authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Json(ep): Json, ) -> Result { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; crate::users_oss::set_password(db, argon2, authed, &email, ep).await } @@ -171,13 +169,11 @@ struct RenameUser { async fn rename_user( authed: ApiAuthed, - OptJobAuthed { job_id, .. }: OptJobAuthed, Path(user_email): Path, Extension(db): Extension, Json(ru): Json, ) -> Result { require_super_admin(&db, &authed).await?; - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; diff --git a/backend/windmill-common/src/auth.rs b/backend/windmill-common/src/auth.rs index 8ed8bab848..ea77f16454 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -324,26 +324,15 @@ pub fn is_reserved_on_behalf_of_identity( } /// Guard a caller-supplied `on_behalf_of` before it is persisted on a deployable -/// object. The stored identity is trusted verbatim at execution and decides -/// `is_super_admin` (see [`fetch_authed_from_permissioned_as_inner`]), so a -/// deploy must not be able to forge a superadmin run identity. -/// -/// Enforces two invariants that no legitimate deploy can violate, so this does -/// *not* restrict the intended `wm_deployers` capability of deploying on behalf -/// of a real user — including a real superadmin, e.g. git-sync of -/// superadmin-authored content where `permissioned_as`/`email` name that user -/// consistently: -/// 1. The identity is not a reserved internal sentinel (above). -/// 2. If `on_behalf_of_email` resolves to a superadmin, it must genuinely belong -/// to `permissioned_as` — a superadmin's email cannot be pinned onto an -/// unrelated principal. Only apps store both fields; flows/scripts store just -/// the email (deriving `permissioned_as` from the deployer at execution) and -/// schedules/triggers store just `permissioned_as` (deriving the email from -/// it), so for those this second check is a no-op and the sentinel guard is -/// what applies. -pub async fn validate_on_behalf_of( - db: &DB, - w_id: &str, +/// object (app policy, flow/script, schedule, trigger): reject the reserved +/// internal sentinels, which no legitimate deploy ever carries. The actual +/// escalation is closed at execution by the job-token cap in +/// [`require_super_admin`] — even a superadmin `on_behalf_of` yields a token +/// capped at workspace admin — so this is a cheap, non-breaking early guard, not +/// the primary defense. It deliberately does *not* restrict deploying on behalf +/// of a real user (including a real superadmin, e.g. git-sync of +/// superadmin-authored content), which is the intended `wm_deployers` capability. +pub fn validate_on_behalf_of( permissioned_as: Option<&str>, on_behalf_of_email: Option<&str>, ) -> Result<()> { @@ -352,21 +341,6 @@ pub async fn validate_on_behalf_of( "on_behalf_of cannot be a reserved internal identity".to_string(), )); } - - if let (Some(permissioned_as), Some(email)) = (permissioned_as, on_behalf_of_email) { - // `email` is already non-sentinel here, so this only matches a real - // `password.super_admin` user. - if is_super_admin_email(db, email).await? { - let resolved = - crate::users::get_email_from_permissioned_as(permissioned_as, w_id, db).await?; - if resolved != email { - return Err(Error::BadRequest(format!( - "on_behalf_of_email '{email}' does not belong to on_behalf_of '{permissioned_as}'" - ))); - } - } - } - Ok(()) } diff --git a/backend/windmill-trigger/src/handler.rs b/backend/windmill-trigger/src/handler.rs index 8697c562b1..d8f5ebca55 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -524,12 +524,9 @@ async fn create_trigger( // (the sentinel guard; a trigger's email is derived from it at execution). let resolved_permissioned_as = new_trigger.base.resolve_permissioned_as(&authed); windmill_common::auth::validate_on_behalf_of( - &db, - &workspace_id, Some(&resolved_permissioned_as), None, - ) - .await?; + )?; let on_behalf_of_info = windmill_common::check_on_behalf_of_preservation( new_trigger.base.permissioned_as.as_deref(), @@ -770,12 +767,9 @@ async fn update_trigger( // (the sentinel guard; a trigger's email is derived from it at execution). let resolved_permissioned_as = edit_trigger.base.resolve_permissioned_as(&authed); windmill_common::auth::validate_on_behalf_of( - &db, - &workspace_id, Some(&resolved_permissioned_as), None, - ) - .await?; + )?; let on_behalf_of_info = windmill_common::check_on_behalf_of_preservation( edit_trigger.base.permissioned_as.as_deref(),