From 673e3f26e7ba3c20daf643ba0dbbfc0bc382e58b Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 12 Sep 2026 11:46:15 +0200 Subject: [PATCH] refactor: derive an app's address from its principal instead of storing it Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EYWxvwmteVyzcWenJfbnzs --- .../tests/folder_default_permissioned_as.rs | 4 - backend/tests/preserve_on_behalf_of.rs | 80 ++++------ backend/windmill-api-auth/src/lib.rs | 7 +- .../tests/offboarding.rs | 8 +- backend/windmill-api/openapi.yaml | 3 +- backend/windmill-api/src/apps.rs | 146 ++++++++++-------- backend/windmill-common/src/lib.rs | 17 +- cli/src/commands/app/app.ts | 12 +- docs/app-policy-email-removal.md | 58 +++---- .../apps/editor/AppEditorHeaderDeploy.svelte | 6 +- .../raw_apps/RawAppTemplatePicker.svelte | 1 - .../apps_raw/edit/[...path]/+page.svelte | 1 - 12 files changed, 158 insertions(+), 185 deletions(-) diff --git a/backend/tests/folder_default_permissioned_as.rs b/backend/tests/folder_default_permissioned_as.rs index 238d7cdb32..db70019d02 100644 --- a/backend/tests/folder_default_permissioned_as.rs +++ b/backend/tests/folder_default_permissioned_as.rs @@ -628,10 +628,6 @@ async fn test_folder_default_permissioned_as(db: Pool) -> anyhow::Resu policy["on_behalf_of"], "g/wm_deployers", "app policy.on_behalf_of gets folder default" ); - assert_eq!( - policy["on_behalf_of_email"], "group-wm_deployers@windmill.dev", - "the stored address is derived from that principal" - ); // 7b. Admin, non-matching path — acting user let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN") diff --git a/backend/tests/preserve_on_behalf_of.rs b/backend/tests/preserve_on_behalf_of.rs index 0c8c0b76a3..75f42fe58c 100644 --- a/backend/tests/preserve_on_behalf_of.rs +++ b/backend/tests/preserve_on_behalf_of.rs @@ -405,7 +405,6 @@ async fn test_preserve_on_behalf_of(db: Pool) -> anyhow::Result<()> { // 7. App: Admin preserves on_behalf_of // ======================================== - // Principal only, so the stored address can only have come from deriving it. let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN") .json(&new_app_with_on_behalf_of( "u/test-user/app_admin_preserve", @@ -435,12 +434,10 @@ async fn test_preserve_on_behalf_of(db: Pool) -> anyhow::Result<()> { Some("u/original-user"), "Admin should preserve app on_behalf_of" ); - // The address is written through from the principal, never taken from the request, so the - // stored copy can only agree with it. assert_eq!( - policy.get("on_behalf_of_email").and_then(|v| v.as_str()), - Some("original@windmill.dev"), - "the stored address is derived from the principal, not the one the client sent" + policy.get("on_behalf_of_email"), + None, + "the address is not stored beside the principal" ); let resp = authed( client().get(format!("{base}/apps/get/p/u/test-user/app_admin_preserve")), @@ -452,7 +449,7 @@ async fn test_preserve_on_behalf_of(db: Pool) -> anyhow::Result<()> { assert_eq!( returned["policy"]["on_behalf_of_email"].as_str(), Some("original@windmill.dev"), - "the response returns the address written through from the principal" + "the response derives the address from the principal" ); // ======================================== @@ -530,24 +527,33 @@ async fn test_preserve_on_behalf_of(db: Pool) -> anyhow::Result<()> { ); // ======================================== - // 9b. App: a policy naming two different accounts is rejected + // 9b. App: an address in the request is ignored, the principal decides // ======================================== - // This is the shape a workspace deploy produces when it carries the source - // workspace's principal beside the target's address. + // The shape a client written before the principal existed still sends. It names another + // account than the principal does, and the principal is the one that counts. let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN") .json(&new_app_with_on_behalf_of( - "u/test-user/app_mismatched_pair", + "u/test-user/app_address_ignored", Some("u/original-user"), Some("test2@windmill.dev"), true, )) .send() .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + + let app = sqlx::query!( + "SELECT policy FROM app WHERE path = $1 AND workspace_id = $2", + "u/test-user/app_address_ignored", + "test-workspace" + ) + .fetch_one(&db) + .await?; assert_eq!( - resp.status(), - 400, - "a policy whose two halves name different accounts must be rejected" + app.policy.get("on_behalf_of").and_then(|v| v.as_str()), + Some("u/original-user"), + "the principal is stored, not the account the address named" ); // ======================================== @@ -2885,9 +2891,9 @@ async fn test_schedule_permissions_superadmin_not_in_workspace( /// Reserved internal sentinel identities are rejected by name at deploy time on /// every entity that stores a preserved on_behalf_of. Real identities stay -/// deployable by a `wm_deployers` member — including a real superadmin, and even -/// their email pinned onto an unrelated principal — because that escalation is -/// closed at execution by the job-token cap, not by restricting what is stored. +/// deployable by a `wm_deployers` member — a real superadmin included — because +/// that escalation is closed at execution by the job-token cap, not by +/// restricting what is stored. #[sqlx::test(fixtures("preserve_on_behalf_of"))] async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; @@ -2900,18 +2906,17 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool) -> anyho const SENTINEL: &str = "superadmin_secret@windmill.dev"; // Reserved sentinel matched on permissioned_as. const SYNC_SENTINEL: &str = "superadmin_sync@windmill.dev"; - // Real instance superadmin, present only in `password` (not a workspace member). - const REAL_SA: &str = "superadmin-external@windmill.dev"; - // App: deployer cannot pin the sentinel email. + // App: deployer cannot pin the sentinel. The policy stores only a principal, and the + // sentinels are address-shaped, so this is the bare-address form of one. let resp = authed( client().post(format!("{base}/apps/create")), "DEPLOYER_TOKEN", ) .json(&new_app_with_on_behalf_of( "u/deployer-user/app_sentinel", - Some("u/original-user"), Some(SENTINEL), + None, true, )) .send() @@ -2919,34 +2924,13 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool) -> anyho assert_eq!( resp.status(), 400, - "deployer must not pin the sentinel as an app on_behalf_of_email: {}", + "deployer must not pin the sentinel as an app on_behalf_of: {}", resp.text().await? ); - // App: a real superadmin's address pinned onto an unrelated principal is a pair naming two - // accounts, which the principal-authoritative policy refuses. - let resp = authed( - client().post(format!("{base}/apps/create")), - "DEPLOYER_TOKEN", - ) - .json(&new_app_with_on_behalf_of( - "u/deployer-user/app_real_sa", - Some("u/original-user"), - Some(REAL_SA), - true, - )) - .send() - .await?; - assert_eq!( - resp.status(), - 400, - "a superadmin address beside an unrelated principal must be refused: {}", - resp.text().await? - ); - - // App: a real superadmin on_behalf_of is allowed at deploy when consistently named (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. + // 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. let resp = authed( client().post(format!("{base}/apps/create")), "DEPLOYER_TOKEN", @@ -2954,7 +2938,7 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool) -> anyho .json(&new_app_with_on_behalf_of( "u/deployer-user/app_consistent_sa", Some("u/superadmin-external"), - Some(REAL_SA), + None, true, )) .send() @@ -2962,7 +2946,7 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool) -> anyho assert_eq!( resp.status(), 201, - "deployer may preserve a consistently named superadmin identity: {}", + "deployer may preserve a superadmin identity: {}", resp.text().await? ); diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 04513b4c05..6c548c29ad 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -1054,9 +1054,10 @@ pub async fn fetch_api_authed_from_permissioned_as( username_override: Option, ) -> error::Result { // Keyed by the supplied address, so an entry built for a principal's previous holder is reused - // while that address is still supplied, until its 120s expiry: a cached dispatch address is - // evicted sooner, an app's stored one (a username deleted then reused) may not be. Accepted; - // the rebuild after expiry is the current holder's. + // while that address is still supplied. Every caller now derives that address rather than + // reading a stored one, so it goes stale only for as long as `get_email_from_permissioned_as` + // serves the old answer, and the 120s expiry here caps whatever survives that. Accepted; the + // rebuild after expiry is the current holder's. let key = (w_id.to_string(), permissioned_as.clone(), email.clone()); let mut api_authed = match API_AUTHED_CACHE.get(&key) { diff --git a/backend/windmill-api-integration-tests/tests/offboarding.rs b/backend/windmill-api-integration-tests/tests/offboarding.rs index 7bdeab924b..3b427663aa 100644 --- a/backend/windmill-api-integration-tests/tests/offboarding.rs +++ b/backend/windmill-api-integration-tests/tests/offboarding.rs @@ -117,7 +117,10 @@ async fn test_offboard_to_user(db: Pool) -> anyhow::Result<()> { .fetch_one(&db) .await?; assert_eq!( - (obo.on_behalf_of.as_deref(), obo.on_behalf_of_email.as_deref()), + ( + obo.on_behalf_of.as_deref(), + obo.on_behalf_of_email.as_deref() + ), (Some("u/test-user"), Some("test@windmill.dev")), "the reassignment moves the whole identity, not just the half this release reads" ); @@ -618,8 +621,7 @@ async fn test_offboard_group_prefixed_member_keeps_group_identities( INSERT INTO group_(workspace_id, name, summary) VALUES ('test-workspace', 'ops', ''); INSERT INTO app(workspace_id, path, summary, policy, versions, extra_perms) VALUES ('test-workspace', 'f/shared/ops_app', '', - '{\"execution_mode\": \"publisher\", \"on_behalf_of\": \"g/ops\", - \"on_behalf_of_email\": \"group-ops@windmill.dev\"}', '{}', '{}');", + '{\"execution_mode\": \"publisher\", \"on_behalf_of\": \"g/ops\"}', '{}', '{}');", ) .execute(&db) .await?; diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 0fd6688571..ec3c855a57 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -33949,7 +33949,8 @@ components: description: The user or group the app runs as in anonymous or publisher mode (e.g. 'u/admin' or 'g/mygroup'). The authority for the app's identity. on_behalf_of_email: type: string - description: Address of `on_behalf_of`, written through from it on every save and returned as stored. Optional; when absent it is derived from `on_behalf_of`. Sending it is optional too; it must name the same account as `on_behalf_of`, and a pair that disagrees is rejected. + readOnly: true + description: Address of `on_behalf_of`, derived from it on every read and never stored. Absent when the policy names no identity. Read-only - sending it is ignored, so a deploy that means to keep an identity has to name it in `on_behalf_of`. sandbox: type: boolean description: > diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index daa7fee59b..c5a43a71dd 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -491,13 +491,9 @@ pub struct S3Key { #[derive(Serialize, Deserialize, Debug, Clone, Default)] pub struct Policy { pub on_behalf_of: Option, - /// The address `on_behalf_of` resolves to. Every write stores what the principal resolves - /// to, so it is not taken from the request except when a client names only the address — - /// which is how a cross-workspace deploy carries an identity — and it is rejected when the - /// two disagree. Optional: a policy without it executes by deriving from the principal, so - /// removing it is a change of default rather than of behavior — see - /// `docs/app-policy-email-removal.md`. - #[serde(skip_serializing_if = "Option::is_none")] + /// The address `on_behalf_of` resolves to. Response-only: never stored and never read from + /// the request. [`attach_on_behalf_of_email`] puts it here, and states which reads may. + #[serde(skip_deserializing, skip_serializing_if = "Option::is_none")] pub on_behalf_of_email: Option, //paths: // - script/ @@ -1130,7 +1126,7 @@ async fn get_app( check_scopes(&authed, || format!("apps:read:{}", path))?; let mut tx = user_db.begin(&authed).await?; - let app_o = if query.with_starred_info.unwrap_or(false) { + let mut app_o = if query.with_starred_info.unwrap_or(false) { sqlx::query_as::<_, AppWithLastVersionAndStarred>( "SELECT app.id, app.path, app.summary, app.versions, app.policy, app.custom_path, app.extra_perms, app_version.value, @@ -1165,6 +1161,11 @@ async fn get_app( }; tx.commit().await?; + // The deployed policy only. A draft overlaid below keeps whatever its editor last saved. + if let Some(app) = app_o.as_mut() { + attach_on_behalf_of_email(&mut app.app.policy, &w_id, &db).await?; + } + // No deployed row + `get_draft`: fall back to the draft table; see scripts.rs. // Draft kind comes from the deployed row's `raw_app` flag, or for a draft-only // path from the caller's `raw_app` query param. @@ -1191,6 +1192,7 @@ async fn get_app( async fn get_app_lite( authed: ApiAuthed, Extension(user_db): Extension, + Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, ) -> JsonResult { let path = path.to_path(); @@ -1212,7 +1214,8 @@ async fn get_app_lite( tx.commit().await?; - let app = not_found_if_none(app_o, "App", path)?; + let mut app = not_found_if_none(app_o, "App", path)?; + attach_on_behalf_of_email(&mut app.policy, &w_id, &db).await?; Ok(Json(app)) } @@ -1329,6 +1332,7 @@ async fn custom_path_exists( async fn get_app_by_id( authed: ApiAuthed, Extension(user_db): Extension, + Extension(db): Extension, Path((w_id, id)): Path<(String, i64)>, ) -> JsonResult { let mut tx = user_db.begin(&authed).await?; @@ -1346,10 +1350,11 @@ async fn get_app_by_id( .await?; tx.commit().await?; - let app = not_found_if_none(app_o, "App", id.to_string())?; + let mut app = not_found_if_none(app_o, "App", id.to_string())?; check_scopes(&authed, || format!("apps:read:{}", &app.path))?; + attach_on_behalf_of_email(&mut app.policy, &w_id, &db).await?; Ok(Json(app)) } @@ -1403,6 +1408,7 @@ async fn get_public_app_by_secret( app.bundle_secret = Some(compute_bundle_secret(&db, &w_id, &app.versions).await?); } + attach_on_behalf_of_email(&mut app.policy, &w_id, &db).await?; Ok(Json(app)) } @@ -2473,12 +2479,12 @@ async fn create_app_internal<'a>( // rebinding the stored principal, is known and accepted: see `resolve_on_behalf_of`. let should_preserve = app.preserve_on_behalf_of.unwrap_or(false) && windmill_common::can_preserve_on_behalf_of(&authed) - && (app.policy.on_behalf_of.is_some() || app.policy.on_behalf_of_email.is_some()); + && app.policy.on_behalf_of.is_some(); let mut preserved_on_behalf_of: Option = None; if should_preserve { app.policy.on_behalf_of = windmill_common::resolve_on_behalf_of( - app.policy.on_behalf_of_email.as_deref(), + None, app.policy.on_behalf_of.as_deref(), true, &authed, @@ -2496,18 +2502,14 @@ async fn create_app_internal<'a>( app.policy.on_behalf_of = Some(folder_default.unwrap_or_else(|| username_to_permissioned_as(&authed.username))); } - app.policy.on_behalf_of_email = stored_on_behalf_of_email(&app.policy, w_id, &db).await?; if should_preserve { - preserved_on_behalf_of = audited_on_behalf_of(&app.policy, &authed); + preserved_on_behalf_of = audited_on_behalf_of(&app.policy, &authed, w_id, &db).await?; } // Reject a forged superadmin run identity in the (possibly preserved) policy. // 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( - app.policy.on_behalf_of.as_deref(), - app.policy.on_behalf_of_email.as_deref(), - )?; + windmill_common::auth::validate_on_behalf_of(app.policy.on_behalf_of.as_deref(), None)?; let mut tx = user_db.clone().begin(&authed).await?; let path = app.path.clone(); @@ -3410,11 +3412,11 @@ async fn update_app_internal<'a>( if let Some(npolicy) = ns.policy.as_mut() { let should_preserve = ns.preserve_on_behalf_of.unwrap_or(false) && windmill_common::can_preserve_on_behalf_of(&authed) - && (npolicy.on_behalf_of.is_some() || npolicy.on_behalf_of_email.is_some()); + && npolicy.on_behalf_of.is_some(); if should_preserve { npolicy.on_behalf_of = windmill_common::resolve_on_behalf_of( - npolicy.on_behalf_of_email.as_deref(), + None, npolicy.on_behalf_of.as_deref(), true, &authed, @@ -3425,9 +3427,8 @@ async fn update_app_internal<'a>( } else { npolicy.on_behalf_of = Some(username_to_permissioned_as(&authed.username)); } - npolicy.on_behalf_of_email = stored_on_behalf_of_email(npolicy, w_id, &db).await?; if should_preserve { - preserved_on_behalf_of = audited_on_behalf_of(npolicy, &authed); + preserved_on_behalf_of = audited_on_behalf_of(npolicy, &authed, w_id, &db).await?; } } @@ -5542,44 +5543,65 @@ async fn app_load_csv_preview() -> Result<()> { )) } -/// The address to store beside the principal. Derived from it, never taken from the request, so -/// the stored copy can only ever agree with the principal — the drift it used to allow is what -/// this replaces. +/// Attach to a policy on its way out the address its principal resolves to. /// -/// Written unconditionally, including for the versions that could derive it instead: a replica -/// predating that fallback fails outright when the key is absent, which would 400 every -/// anonymous, publisher and guest app for the length of a rolling deploy. The write is what -/// holds the key in place — see `docs/app-policy-email-removal.md`. -async fn stored_on_behalf_of_email(policy: &Policy, w_id: &str, db: &DB) -> Result> { - let Some(permissioned_as) = policy.on_behalf_of.as_deref() else { - return Ok(None); +/// Derived here rather than left to the caller because the app bundle reads it as `ctx.author` +/// in the browser, where an anonymous viewer has no session to resolve `u/alice` with. Cached, +/// like every other derivation of a run identity that is not being persisted: nothing reads this +/// back as authority, so the accepted dispatch staleness `get_email_from_permissioned_as` +/// documents applies. +/// +/// Only ever for a policy the server itself wrote. A draft's principal is whatever its editor +/// last typed, so resolving one would answer "which address is `u/x`?" for any principal a +/// caller cares to name — including a superadmin outside their own workspaces, whom +/// `resolve_username_to_email` reaches instance-wide. +pub async fn attach_on_behalf_of_email( + policy: &mut sqlx::types::Json>, + w_id: &str, + db: &DB, +) -> Result<()> { + // Field-level, not a `Policy` round-trip: a legacy policy carries keys this struct has + // never had, and returning it shorn of them is not this function's job. + let mut value = serde_json::from_str::(policy.0.get()).map_err(to_anyhow)?; + let Some(object) = value.as_object_mut() else { + return Ok(()); }; - Ok(Some( - windmill_common::users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db) - .await?, - )) + let Some(permissioned_as) = object + .get("on_behalf_of") + .and_then(|v| v.as_str()) + .map(str::to_string) + else { + return Ok(()); + }; + let email = + windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db).await?; + object.insert("on_behalf_of_email".to_string(), json!(email)); + *policy = sqlx::types::Json(serde_json::value::to_raw_value(&value).map_err(to_anyhow)?); + Ok(()) } /// The address to record in the `apps.on_behalf_of` audit entry: the one the app will run as, /// when it is not the deployer's own. `None` when they match — a deployer handing an app their /// own identity is not an on-behalf-of deploy. /// -/// Reads the address `stored_on_behalf_of_email` just resolved rather than looking it up again, -/// so the audit row and the policy row can only ever name the same account. -fn audited_on_behalf_of(policy: &Policy, authed: &ApiAuthed) -> Option { - policy - .on_behalf_of_email - .as_deref() - .filter(|email| *email != authed.email) - .map(str::to_string) +/// Uncached, unlike the read path above: this one is written to a row that is never revisited, +/// so a stale answer here is a permanent misattribution of the deploy. +async fn audited_on_behalf_of( + policy: &Policy, + authed: &ApiAuthed, + w_id: &str, + db: &DB, +) -> Result> { + let Some(permissioned_as) = policy.on_behalf_of.as_deref() else { + return Ok(None); + }; + let email = + windmill_common::users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db) + .await?; + Ok((email != authed.email).then_some(email)) } /// The identity an anonymous, publisher or guest execution runs as. -/// -/// `on_behalf_of_email` is optional: every write stores it, so it is present on anything this -/// release deployed, and it is only derived for a policy that predates that. Deriving is the -/// fallback rather than the rule so that removing the key later is a change of default, not a -/// change of behavior — see `docs/app-policy-email-removal.md`. async fn get_on_behalf_of(policy: &Policy, w_id: &str, db: &DB) -> Result<(String, String)> { let permissioned_as = policy .on_behalf_of @@ -5591,15 +5613,10 @@ async fn get_on_behalf_of(policy: &Policy, w_id: &str, db: &DB) -> Result<(Strin ) })? .to_string(); - let email = match policy.on_behalf_of_email.as_deref() { - Some(email) => email.to_string(), - // Cached on purpose, up to one notify poll stale: the accepted dispatch case - // `get_email_from_permissioned_as` documents. - None => { - windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db) - .await? - } - }; + // Cached on purpose, up to one notify poll stale: the accepted dispatch case + // `get_email_from_permissioned_as` documents. + let email = + windmill_common::users::get_email_from_permissioned_as(&permissioned_as, w_id, db).await?; // Defence in depth against a policy that already carries a forged superadmin // sentinel (deployed before validation existed, or copied verbatim by a // workspace fork): the sentinels are internal-only and never a legitimate app @@ -5746,14 +5763,11 @@ async fn build_args( "email" => authed.as_ref().map(|a| serde_json::to_value(&a.email)), "workspace" => Some(serde_json::to_value(&w_id)), "groups" => authed.as_ref().map(|a| serde_json::to_value(&a.groups)), - // Same rule as `get_on_behalf_of`: the stored address, derived only when absent. + // Derived from the principal, cached, exactly as `get_on_behalf_of` does: this + // is the same address the run it is an argument to executes under. "author" => { - let author = match ( - policy.on_behalf_of_email.as_deref(), - policy.on_behalf_of.as_deref(), - ) { - (Some(email), _) => Some(email.to_string()), - (None, Some(permissioned_as)) => Some( + let author = match policy.on_behalf_of.as_deref() { + Some(permissioned_as) => Some( windmill_common::users::get_email_from_permissioned_as( permissioned_as, w_id, @@ -5761,7 +5775,7 @@ async fn build_args( ) .await?, ), - (None, None) => None, + None => None, }; Some(serde_json::to_value(&author)) } diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index dce40048f5..0958cd4c74 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -285,13 +285,11 @@ pub fn check_on_behalf_of_preservation( /// Resolves the identity to store when creating/updating a flow, script or app. /// /// The permissioned_as is the identity: it decides what the job may access, and the address is -/// a function of it, so the two can never name different accounts. For a script or flow the -/// address is derived at read time; an app still stores it, as a compatibility copy written -/// through from the principal on every save and returned verbatim by the app reads (see -/// `docs/app-policy-email-removal.md`). Callers may supply either: a bare email (every client -/// written before the principal existed) is resolved to the principal it names, and an email -/// that names nobody is rejected rather than recorded, since it could only produce a runnable -/// that cannot authenticate. +/// a function of it, derived at read time rather than stored, so the two can never name +/// different accounts. Callers may still supply either for a script or flow: a bare email +/// (every client written before the principal existed) is resolved to the principal it names, +/// and an email that names nobody is rejected rather than recorded, since it could only produce +/// a runnable that cannot authenticate. An app policy has no address to supply. /// /// Returns `None` when the runnable has no on-behalf-of identity, and the caller's own /// identity when they are not allowed to preserve someone else's. @@ -303,9 +301,8 @@ pub fn check_on_behalf_of_preservation( /// Known, accepted race. The lookup runs on the pool, outside the caller's write transaction, so /// an account renamed or removed between the two has its sweep run before the write is visible, /// and the write stores the old principal. The runnable then fails to authenticate until it is -/// deployed with a current identity, with two exceptions: an app naming an external superadmin -/// keeps running as that account through its stored address, and if the freed username is later -/// given to another account, the stale principal binds to that account and runs as it. Every +/// deployed with a current identity, with one exception: if the freed username is later given to +/// another account, the stale principal binds to that account and runs as it. Every /// caller shares this (scripts, flows and apps, address-only inputs included), and it needs a /// rename or removal of the exact account inside the lookup-to-commit gap. Closing it means /// serializing every identity write against every identity mutation, across all runnable kinds diff --git a/cli/src/commands/app/app.ts b/cli/src/commands/app/app.ts index 0d4ad634c8..9cba4e8409 100644 --- a/cli/src/commands/app/app.ts +++ b/cli/src/commands/app/app.ts @@ -572,7 +572,7 @@ const command = new Command() }) .command( "set-permissioned-as", - "Set the on_behalf_of_email for an app (requires admin or wm_deployers group)" + "Set the user an app runs on behalf of (requires admin or wm_deployers group)" ) .arguments(" ") .action((async (opts: any, appPath: string, email: string) => { @@ -592,15 +592,15 @@ const command = new Command() // Only spread remote.policy — spreading the full remote object would include // `value` and trigger a new app version on every call. EditApp has all-Option // fields, so a minimal body only updates the policy column. + // The read carries a derived `on_behalf_of_email`; a server old enough to still read it + // would see the previous holder's address beside the new principal and reject the pair. + const policy = { ...(remote.policy as any), on_behalf_of: `u/${username}` }; + delete policy.on_behalf_of_email; await wmill.updateApp({ workspace: workspace.workspaceId, path: appPath, requestBody: { - policy: { - ...(remote.policy as any), - on_behalf_of: `u/${username}`, - on_behalf_of_email: email, - } as any, + policy: policy as any, preserve_on_behalf_of: true, // Preserve any user draft at this path (see backend skip_draft_deletion). skip_draft_deletion: true, diff --git a/docs/app-policy-email-removal.md b/docs/app-policy-email-removal.md index fb4cf100fd..a908671e88 100644 --- a/docs/app-policy-email-removal.md +++ b/docs/app-policy-email-removal.md @@ -1,47 +1,31 @@ -# Removing `policy.on_behalf_of_email` +# `policy.on_behalf_of_email` An app's identity is `policy.on_behalf_of`; the address beside it is a function of that -principal. `on_behalf_of_email` is no longer required — a policy carrying only a principal -executes, deriving the address — but it is still written on every save, and that is the only -thing holding it in place. +principal. The policy stores only the principal. The address is derived wherever it is needed: +at execution (`get_on_behalf_of`, `$ctx:author`) and on the reads that return a deployed policy +(`attach_on_behalf_of_email`), where it is a response-only field the write path ignores. ## The gate +**This cannot ship until `MIN_KEEP_ALIVE_VERSION` (`windmill-common/src/min_version.rs`) has +passed 1.810.** + `get_on_behalf_of` gained its derive-when-absent fallback in **1.810**. Every replica before that -*requires* the key and errors outright without it, so it would 400 every anonymous, publisher and -guest app saved by a newer one. A rolling deploy runs both versions at once, which is why the -write stays until no replica older than 1.810 can be live — in practice, once -`MIN_KEEP_ALIVE_VERSION` (`windmill-common/src/min_version.rs`) has passed it. +*requires* the key and errors outright without it, so a rolling deploy that runs both would 400 +every anonymous, publisher and guest app the moment one of them saves. The write is what holds +the key in place, so stopping it is only safe once no replica older than 1.810 can be live. -There is no `MIN_VERSION_*` constant for this and it does not need one: those exist to gate -behavior at runtime or to trip the build when a constraint expires, and nothing here does either. -The key is written unconditionally, so no replica ever meets its absence until someone follows -the steps below. +`MIN_KEEP_ALIVE_VERSION` is 1.420.0, and a compile-time rule keeps it at least 50 minor versions +behind current, so it cannot reach 1.810 until releases pass ~1.860. -## Step 1 — stop writing the key +There is no `MIN_VERSION_*` constant for this and it does not need one: those gate behavior at +runtime or trip the build when a constraint expires, and nothing here does either. -- `stored_on_behalf_of_email` in `windmill-api/src/apps.rs`, and the `create_app` / - `update_app_internal` call sites that store what it returns. -- The CLI and frontend workspace-deploy paths (`cli/windmill-utils-internal/src/deploy.ts`, - `frontend/src/lib/utils_workspace_deploy.ts`). These send the address *instead of* a principal - for a cross-workspace deploy, which is the one case where it is the only identity available — - so this is "stop sending it once the target resolves a principal itself", not a deletion. +## Why the address is not derived on a draft read -Policies written before this keep their key and keep being read from it; they agree with their -principal, so nothing has to strip them. - -## Step 2 — drop the field - -Remove `on_behalf_of_email` from `Policy` and the fallback in `get_on_behalf_of`, which then -always derives. Optionally strip the key from stored policies. - -This can ship with step 1. It is written separately because step 1 alone is revertible without -touching stored data or the response schema, and because the gate above is what makes either -step safe — nothing about step 2 needs its own waiting period. - -## Why the address is not derived on read - -Read paths return the stored address verbatim rather than recomputing it. Deriving on read means -resolving a principal that, for a draft, is caller-controlled — which turns the read into an -oracle for addresses the caller cannot otherwise see, and leaves a principal that resolves to -nobody with no address at all. Both were live defects while the read paths did derive. +A deployed policy's principal was written by the server, from an identity `resolve_on_behalf_of` +validated. A draft's is whatever its editor last typed. Deriving one would answer "which address +is `u/x`?" for any principal a caller cares to name — including a superadmin outside their own +workspaces, whom `resolve_username_to_email` reaches instance-wide. That was a live defect while +every read derived, and is why the derivation is attached per read path rather than to `Policy` +itself. diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte index 836af68db6..f128da0c24 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte @@ -144,7 +144,6 @@ } } let canPreserve = $derived(!!$userStore?.is_admin || !!$userStore?.is_super_admin || isDeployer) - let savedOnBehalfOfEmail = $derived(savedApp?.policy?.on_behalf_of_email) let savedOnBehalfOf = $derived(savedApp?.policy?.on_behalf_of) let onBehalfOfChoice: OnBehalfOfChoice = $state(undefined) let customOnBehalfOfEmail: string = $state('') @@ -315,22 +314,19 @@ user to run this app on behalf of. Once deployed the app will be run on behalf of { onBehalfOfChoice = choice if (choice === 'me') { - policy.on_behalf_of_email = $userStore?.email policy.on_behalf_of = `u/${$userStore?.username}` customOnBehalfOfEmail = '' preserveOnBehalfOf = false } else if (choice === 'target') { - policy.on_behalf_of_email = savedOnBehalfOfEmail policy.on_behalf_of = savedOnBehalfOf customOnBehalfOfEmail = '' preserveOnBehalfOf = true } else if (choice === 'custom' && details) { - policy.on_behalf_of_email = details.email policy.on_behalf_of = details.permissionedAs customOnBehalfOfEmail = details.email preserveOnBehalfOf = true diff --git a/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte b/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte index 7ead1f6fb0..9a6d93ca4a 100644 --- a/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte @@ -172,7 +172,6 @@ on_behalf_of: $userStore?.username.includes('@') ? $userStore?.username : `u/${$userStore?.username}`, - on_behalf_of_email: $userStore?.email, execution_mode: 'publisher' } diff --git a/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte index 78483c327a..2a6a354e5a 100644 --- a/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/apps_raw/edit/[...path]/+page.svelte @@ -211,7 +211,6 @@ on_behalf_of: $userStore?.username.includes('@') ? $userStore?.username : `u/${$userStore?.username}`, - on_behalf_of_email: $userStore?.email, execution_mode: 'publisher' } as any // Explicit path seed: the fork-a-draft handoff re-homes the source