diff --git a/backend/.sqlx/query-5a219a2532517869578c4504ff3153c43903f929ae5d62fbba12610f89c36d55.json b/backend/.sqlx/query-5a219a2532517869578c4504ff3153c43903f929ae5d62fbba12610f89c36d55.json index 713ccb9dd3..36ddb8ab9f 100644 --- a/backend/.sqlx/query-5a219a2532517869578c4504ff3153c43903f929ae5d62fbba12610f89c36d55.json +++ b/backend/.sqlx/query-5a219a2532517869578c4504ff3153c43903f929ae5d62fbba12610f89c36d55.json @@ -15,7 +15,7 @@ ] }, "nullable": [ - null + true ] }, "hash": "5a219a2532517869578c4504ff3153c43903f929ae5d62fbba12610f89c36d55" diff --git a/backend/migrations/20260801091932_backfill_app_policy_on_behalf_of.up.sql b/backend/migrations/20260801091932_backfill_app_policy_on_behalf_of.up.sql index a081a55c6c..35e2676a8e 100644 --- a/backend/migrations/20260801091932_backfill_app_policy_on_behalf_of.up.sql +++ b/backend/migrations/20260801091932_backfill_app_policy_on_behalf_of.up.sql @@ -18,12 +18,16 @@ CREATE OR REPLACE FUNCTION pg_temp.permissioned_as_from_email(w_id VARCHAR, email VARCHAR) RETURNS VARCHAR AS $$ SELECT COALESCE( - -- `username_to_permissioned_as`: an email-shaped username is stored verbatim. - (SELECT CASE WHEN u.username LIKE '%@%' THEN u.username ELSE 'u/' || u.username END + -- `username_to_permissioned_as`: an email-shaped username is its own principal unless + -- it contains a slash, which a reader would split on. + (SELECT CASE WHEN u.username LIKE '%@%' AND u.username NOT LIKE '%/%' THEN u.username + ELSE 'u/' || u.username END FROM usr u WHERE u.workspace_id = $1 AND u.email = $2), -- A superadmin acting outside their workspaces has no usr row. - (SELECT CASE WHEN COALESCE(p.username, p.email) LIKE '%@%' THEN COALESCE(p.username, p.email) - ELSE 'u/' || p.username END + (SELECT CASE WHEN COALESCE(p.username, p.email) LIKE '%@%' + AND COALESCE(p.username, p.email) NOT LIKE '%/%' + THEN COALESCE(p.username, p.email) + ELSE 'u/' || COALESCE(p.username, p.email) END FROM password p WHERE p.email = $2 AND p.super_admin), (SELECT 'g/' || g.name FROM group_ g WHERE g.workspace_id = $1 @@ -32,16 +36,21 @@ RETURNS VARCHAR AS $$ $$ LANGUAGE SQL STABLE; -- A policy naming only the address predates the principal being written to it. +-- A principal wider than `v2_job.permissioned_as` could not be enqueued, so it is not recorded +-- at all — the app falls back to erroring on anonymous execution until someone picks an identity +-- the deploy path accepts. Same cap and reason as the sibling migration 20260801043001. UPDATE app SET policy = jsonb_set(policy, ARRAY['on_behalf_of'], to_jsonb(pg_temp.permissioned_as_from_email(workspace_id, policy->>'on_behalf_of_email'))) WHERE policy->>'on_behalf_of' IS NULL - AND pg_temp.permissioned_as_from_email(workspace_id, policy->>'on_behalf_of_email') IS NOT NULL; + AND pg_temp.permissioned_as_from_email(workspace_id, policy->>'on_behalf_of_email') IS NOT NULL + AND length(pg_temp.permissioned_as_from_email(workspace_id, policy->>'on_behalf_of_email')) <= 55; -- App drafts carry a copy of the policy and are deployed from it, so they need the same. UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb(pg_temp.permissioned_as_from_email(workspace_id, value->'policy'->>'on_behalf_of_email')))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of' IS NULL - AND pg_temp.permissioned_as_from_email(workspace_id, value->'policy'->>'on_behalf_of_email') IS NOT NULL; + AND pg_temp.permissioned_as_from_email(workspace_id, value->'policy'->>'on_behalf_of_email') IS NOT NULL + AND length(pg_temp.permissioned_as_from_email(workspace_id, value->'policy'->>'on_behalf_of_email')) <= 55; DROP FUNCTION pg_temp.permissioned_as_from_email(VARCHAR, VARCHAR); diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index d42da064f6..85fa5e34d8 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -2189,7 +2189,6 @@ async fn create_app_internal<'a>( &db, ) .await?; - preserved_on_behalf_of = audited_on_behalf_of(&app.policy, &authed, w_id, &db).await?; } else { let folder_default = if windmill_common::can_preserve_on_behalf_of(&authed) { windmill_common::folders::resolve_folder_default_permissioned_as(&db, w_id, &app.path) @@ -2201,6 +2200,9 @@ async fn create_app_internal<'a>( 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); + } let mut tx = user_db.clone().begin(&authed).await?; let path = app.path.clone(); @@ -2765,11 +2767,13 @@ async fn update_app_internal<'a>( &db, ) .await?; - preserved_on_behalf_of = audited_on_behalf_of(npolicy, &authed, w_id, &db).await?; } 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); + } } let mut tx = user_db.clone().begin(&authed).await?; @@ -4725,18 +4729,16 @@ async fn stored_on_behalf_of_email( /// 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. -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(permissioned_as, w_id, db).await?; - Ok((email != authed.email).then_some(email)) +/// +/// Reads the address `stored_on_behalf_of_email` just resolved rather than looking it up again: +/// that one is uncached, and a second cached read could write a since-changed address into a +/// durable audit row. +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) } async fn get_on_behalf_of(policy: &Policy, w_id: &str, db: &DB) -> Result<(String, String)> { diff --git a/cli/test/deploy_on_behalf_of_unit.test.ts b/cli/test/deploy_on_behalf_of_unit.test.ts index 02eb25684d..7e21b24832 100644 --- a/cli/test/deploy_on_behalf_of_unit.test.ts +++ b/cli/test/deploy_on_behalf_of_unit.test.ts @@ -96,13 +96,12 @@ test("deployItem: never sends the source workspace's on_behalf_of", async () => ]); for (const [name, body] of captured) { expect(body.preserve_on_behalf_of).toBe(true); - // An app carries its identity inside the policy; the others carry it at the top level. + // Both surfaces spell it `on_behalf_of`; only its nesting differs — an app carries the + // identity inside its policy, the others at the top level. const identity = name === "createApp" ? body.policy : body; - // Both surfaces spell it the same; only its nesting differs. - const principalKey = "on_behalf_of"; // The email is still overridden with the caller's choice... expect(identity.on_behalf_of_email).toBe("alice@corp"); // ...while the principal is dropped, so the backend derives the target's own. - expect(principalKey in JSON.parse(JSON.stringify(identity))).toBe(false); + expect("on_behalf_of" in JSON.parse(JSON.stringify(identity))).toBe(false); } });