refactor: simplify on_behalf_of deploy guard and prune redundant job-token denylist

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) <noreply@anthropic.com>
This commit is contained in:
hugocasa
2026-07-14 17:19:59 +02:00
parent 19eaf3e2cf
commit 12649031f2
11 changed files with 28 additions and 97 deletions
+8 -5
View File
@@ -2856,13 +2856,16 @@ async fn test_reject_forged_superadmin_on_behalf_of(db: Pool<Postgres>) -> 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<Postgres>) -> 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",
+2 -8
View File
@@ -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;
+2 -8
View File
@@ -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,
+1 -4
View File
@@ -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",
-12
View File
@@ -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<String>,
Extension(db): Extension<DB>,
Json(eu): Json<EditUser>,
) -> Result<String> {
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<bool> = None;
@@ -1603,12 +1601,10 @@ async fn update_user(
async fn delete_user(
authed: ApiAuthed,
OptJobAuthed { job_id, .. }: OptJobAuthed,
Path(email_to_delete): Path<String>,
Extension(db): Extension<DB>,
) -> Result<String> {
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<DB>,
Path(email): Path<String>,
authed: ApiAuthed,
OptJobAuthed { job_id, .. }: OptJobAuthed,
Json(et): Json<EditLoginType>,
) -> Result<String> {
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<DB>,
authed: ApiAuthed,
OptJobAuthed { job_id, .. }: OptJobAuthed,
Json(new_token): Json<NewToken>,
) -> 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<DB>,
authed: ApiAuthed,
OptJobAuthed { job_id, .. }: OptJobAuthed,
) -> JsonResult<Vec<ExportedGlobalUser>> {
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<String> {
async fn overwrite_global_users(
Extension(db): Extension<DB>,
authed: ApiAuthed,
OptJobAuthed { job_id, .. }: OptJobAuthed,
Json(users): Json<Vec<ExportedGlobalUser>>,
) -> Result<String> {
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)
@@ -3768,7 +3768,6 @@ async fn list_workspaces_as_super_admin(
Extension(db): Extension<DB>,
Extension(user_db): Extension<UserDB>,
Query(pagination): Query<Pagination>,
ApiAuthed { email, .. }: ApiAuthed,
) -> JsonResult<Vec<Workspace>> {
require_devops_role(&db, &authed).await?;
let (per_page, offset) = paginate(pagination);
+2 -8
View File
@@ -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?;
)?;
}
}
+2 -4
View File
@@ -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<DB>,
Path(email): Path<String>,
Json(req): Json<GlobalOffboardRequest>,
) -> Result<Json<OffboardResponse>> {
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",
-4
View File
@@ -156,11 +156,9 @@ async fn set_password_of_user(
Extension(argon2): Extension<Arc<Argon2<'_>>>,
Path(email): Path<String>,
authed: ApiAuthed,
OptJobAuthed { job_id, .. }: OptJobAuthed,
Json(ep): Json<EditPassword>,
) -> Result<String> {
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<String>,
Extension(db): Extension<DB>,
Json(ru): Json<RenameUser>,
) -> Result<String> {
require_super_admin(&db, &authed).await?;
forbid_superadmin_job_token(&db, &authed.email, job_id).await?;
let mut tx = db.begin().await?;
+9 -35
View File
@@ -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(())
}
+2 -8
View File
@@ -524,12 +524,9 @@ async fn create_trigger<T: TriggerCrud>(
// (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<T: TriggerCrud>(
// (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(),