From 49f8a253fdbbb087a063ceaa9f7ad01bb2b561c9 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 12 Sep 2026 11:46:03 +0200 Subject: [PATCH] fix: carry an app's own principal across a workspace deploy Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EYWxvwmteVyzcWenJfbnzs --- backend/windmill-common/src/users.rs | 14 +++--- cli/test/deploy_on_behalf_of_unit.test.ts | 31 +++++++------ cli/windmill-utils-internal/src/deploy.ts | 22 ++++++---- .../lib/components/CompareWorkspaces.svelte | 9 ++-- .../src/lib/components/DeployWorkspace.svelte | 9 ++-- .../lib/components/OnBehalfOfSelector.svelte | 15 +++++++ .../components/UpdateDevWorkspaceModal.svelte | 6 ++- frontend/src/lib/utils_workspace_deploy.ts | 44 ++++++++++++------- 8 files changed, 95 insertions(+), 55 deletions(-) diff --git a/backend/windmill-common/src/users.rs b/backend/windmill-common/src/users.rs index 1a49605e59..3074b1599f 100644 --- a/backend/windmill-common/src/users.rs +++ b/backend/windmill-common/src/users.rs @@ -202,11 +202,12 @@ pub fn invalidate_email_cache_for_username(username: &str) { /// /// Known, accepted consequence of a real account winning the synthetic `group-*@windmill.dev` /// namespace: a group identity sent as its address alone, as a "keep target identity" workspace -/// deploy sends it for scripts, flows and apps, comes back as the account holding that address -/// when one exists, not as `g/*`. Such an account takes an admin to exist: a superadmin or an +/// deploy sends it for scripts and flows, comes back as the account holding that address when one +/// exists, not as `g/*`. Such an account takes an admin to exist: a superadmin or an /// admin-configured identity provider to create it (the public OAuth providers only assert a /// `@windmill.dev` address to that domain's owner) and an admin of the target workspace to admit -/// it, so no member can steer a group's runnables to themselves this way. +/// it, so no member can steer a group's runnables to themselves this way. An app is not exposed +/// to it: its policy stores only the principal, so a deploy carries `g/*` itself. /// /// Reads through the non-RLS pool and authorizes nothing: callers must already be authorized /// for `workspace_id`. @@ -263,11 +264,12 @@ pub async fn permissioned_as_from_email( /// ever missed. /// /// Which of the two to use is a question of how long a wrong answer lives, not of whether it is -/// stored — both of these get stored and read back. A config row (an app policy, a schedule, a -/// runnable) is the authority for every run that follows it, so a stale address there is +/// stored — both of these get stored and read back. A config row that persists an address (a +/// schedule, a runnable) is the authority for every run that follows it, so a stale one there is /// permanent and invisible: those use [`get_email_from_permissioned_as_uncached`]. Job dispatch /// also stores its answer, and the worker reads it back to build that run's authed, but it -/// governs one job and dies with it, so it stays here. +/// governs one job and dies with it, so it stays here. An app policy persists no address at all +/// — its reads and its dispatch both derive one, and both are in the dispatch case. /// /// The job's own authorization does not trust the address as given: /// `fetch_authed_from_permissioned_as` re-resolves it from the principal's live binding, and that diff --git a/cli/test/deploy_on_behalf_of_unit.test.ts b/cli/test/deploy_on_behalf_of_unit.test.ts index 7e21b24832..2b63e8f3ce 100644 --- a/cli/test/deploy_on_behalf_of_unit.test.ts +++ b/cli/test/deploy_on_behalf_of_unit.test.ts @@ -3,10 +3,11 @@ import { deployItem } from "../windmill-utils-internal/src/deploy.ts"; // `deployItem` spreads the source item into the request body, and the principal it carries // (`on_behalf_of`, at the top level for a script or flow and inside the policy for an app) -// names a username that only exists in the source workspace. Sending it to the target pairs -// one workspace's principal with the other's email, which the backend rejects. Deleting the -// spread is an easy regression, so pin that the principal never reaches the wire while the -// caller's chosen address does. +// names a username that only exists in the source workspace. Sending it to the target would +// run the item as whoever holds that name there, or as nobody. Deleting the overwrite is an +// easy regression, so pin that the source principal never reaches the wire — and that each +// kind sends the identity in the form it takes: an address for a flow or script, the target's +// own principal for an app. function recordingProvider(captured: [string, any][], flowExists: boolean) { const source = { on_behalf_of_email: "alice@corp", @@ -42,7 +43,6 @@ function recordingProvider(captured: [string, any][], flowExists: boolean) { policy: { execution_mode: "publisher", on_behalf_of: "u/alice", - on_behalf_of_email: "alice@corp", }, }), createApp: async (p: any) => void captured.push(["createApp", p.requestBody]), @@ -52,7 +52,7 @@ function recordingProvider(captured: [string, any][], flowExists: boolean) { test("deployItem: never sends the source workspace's on_behalf_of", async () => { const captured: [string, any][] = []; - // The clear is written out once per branch, so exercise all three: a flow that + // The overwrite is written out once per branch, so exercise all three: a flow that // does not exist in the target (create), one that does (update — the branch // `wmill workspace merge` takes for anything already deployed), and a script. await deployItem( @@ -79,13 +79,15 @@ test("deployItem: never sends the source workspace's on_behalf_of", async () => "dst", "alice@corp", ); + // An app is handed a principal, not an address: `getOnBehalfOf` read it out of the + // target workspace, where `u/` and `g/` names mean what they say. await deployItem( recordingProvider(captured, false), "app" as any, "f/x/a", "src", "dst", - "alice@corp", + "g/ops", ); expect(captured.map(([fn]) => fn)).toEqual([ @@ -96,12 +98,13 @@ 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); - // 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; - // 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("on_behalf_of" in JSON.parse(JSON.stringify(identity))).toBe(false); + if (name === "createApp") { + expect(body.policy.on_behalf_of).toBe("g/ops"); + expect(body.policy.on_behalf_of_email).toBeUndefined(); + } else { + expect(body.on_behalf_of_email).toBe("alice@corp"); + // The principal is dropped, so the backend derives the target's own from the address. + expect("on_behalf_of" in JSON.parse(JSON.stringify(body))).toBe(false); + } } }); diff --git a/cli/windmill-utils-internal/src/deploy.ts b/cli/windmill-utils-internal/src/deploy.ts index ac0b775e86..769fc7d269 100644 --- a/cli/windmill-utils-internal/src/deploy.ts +++ b/cli/windmill-utils-internal/src/deploy.ts @@ -510,17 +510,19 @@ export async function deployItem( workspace: workspaceFrom, path, }); - // See the flow branch: a source-workspace principal is never valid here, and the - // policy carries the app's in `on_behalf_of`. Clearing it lets the backend derive - // the target's own from the address. A group travels as its synthetic - // `group-*@windmill.dev` address, which an admin-created account holding it would - // win: known and accepted, see `users::permissioned_as_from_email` in the backend. + // The source policy's principal names nobody in the target, so it is always replaced: + // `getOnBehalfOf` read this one out of the *target*, where `u/alice` and `g/ops` mean + // what they say. Unlike the flow and script branches above, which still carry an + // address the backend resolves, the app's identity travels as the principal itself — + // a group has no address of its own to travel as. The address the read derived goes + // too: it is the source's, and a server old enough to still read it would take the + // pair as naming two accounts. const app = { ...rawApp, policy: { ...rawApp.policy, - on_behalf_of: undefined, - on_behalf_of_email: onBehalfOf, + on_behalf_of: onBehalfOf, + on_behalf_of_email: undefined, }, }; if (alreadyExists) { @@ -987,7 +989,9 @@ function stripTriggerOrScheduleRuntimeFields(row: unknown): unknown { /** * Fetch the on_behalf_of value for a deployable item. - * Returns an email for flows/scripts/apps, or undefined if not applicable. + * Returns an email for flows/scripts, `permissioned_as` for apps, schedules and triggers, or + * undefined if not applicable. Which of the two a kind speaks is also what `deployItem` sends + * back for it, so the pair has to stay in step. */ export async function getOnBehalfOf( provider: DeployProvider, @@ -1004,7 +1008,7 @@ export async function getOnBehalfOf( return script.on_behalf_of_email; } else if (kind === "app" || kind === "raw_app") { const app = await provider.getAppByPath({ workspace, path }); - return app.policy?.on_behalf_of_email; + return app.policy?.on_behalf_of; } else if (kind === "schedule") { const schedule = await provider.getSchedule({ workspace, path }); return schedule.permissioned_as; diff --git a/frontend/src/lib/components/CompareWorkspaces.svelte b/frontend/src/lib/components/CompareWorkspaces.svelte index 375e73480e..172a0a77e9 100644 --- a/frontend/src/lib/components/CompareWorkspaces.svelte +++ b/frontend/src/lib/components/CompareWorkspaces.svelte @@ -56,6 +56,7 @@ import { isTriggerOrScheduleKind } from 'windmill-utils-internal' import Tooltip from './Tooltip.svelte' import OnBehalfOfSelector, { + deployItemIdentityIsPrincipal, needsOnBehalfOfSelection, type OnBehalfOfChoice, type OnBehalfOfDetails @@ -609,15 +610,15 @@ /** * Get the on_behalf_of value for deployment based on user's choice. - * Returns an email for flows/scripts/apps, or permissioned_as (u/username, g/group) for triggers/schedules. + * An email for flows/scripts, permissioned_as (u/username, g/group) for the kinds + * `deployItemIdentityIsPrincipal` names. */ function getOnBehalfOfForDeploy(itemKey: string, kind: Kind): string | undefined { const choice = onBehalfOfChoice[itemKey] if (choice === 'target') return getTargetOnBehalfOf(itemKey) if (choice === 'custom') { const details = customOnBehalfOf[itemKey] - const wantsPermissionedAs = kind === 'trigger' || isTriggerOrScheduleKind(kind) - return wantsPermissionedAs ? details?.permissionedAs : details?.email + return deployItemIdentityIsPrincipal(kind) ? details?.permissionedAs : details?.email } // 'me' or undefined = don't pass, backend will use deploying user's identity return undefined @@ -629,7 +630,7 @@ * returning undefined clears the source item's value rather than keeping it. */ function getOnBehalfOfPermissionedAsForDeploy(itemKey: string, kind: Kind): string | undefined { - if (kind === 'trigger' || isTriggerOrScheduleKind(kind)) return undefined + if (deployItemIdentityIsPrincipal(kind)) return undefined if (onBehalfOfChoice[itemKey] !== 'custom') return undefined return customOnBehalfOf[itemKey]?.permissionedAs } diff --git a/frontend/src/lib/components/DeployWorkspace.svelte b/frontend/src/lib/components/DeployWorkspace.svelte index 1f938dcfdf..44eed28b40 100644 --- a/frontend/src/lib/components/DeployWorkspace.svelte +++ b/frontend/src/lib/components/DeployWorkspace.svelte @@ -39,6 +39,7 @@ import type { Runnable } from './raw_apps/utils' import WorkspaceDeployLayout from './WorkspaceDeployLayout.svelte' import OnBehalfOfSelector, { + deployItemIdentityIsPrincipal, needsOnBehalfOfSelection, type OnBehalfOfChoice, type OnBehalfOfDetails @@ -93,14 +94,15 @@ /** * Get the on_behalf_of value for deployment based on user's choice. - * Returns an email for flows/scripts/apps, or permissioned_as (u/username, g/group) for triggers/schedules. + * An email for flows/scripts, permissioned_as (u/username, g/group) for the kinds + * `deployItemIdentityIsPrincipal` names. */ function getOnBehalfOfForDeploy(statusPath: string, kind: Kind): string | undefined { const choice = onBehalfOfChoice[statusPath] if (choice === 'target') return targetOnBehalfOfInfo[statusPath] if (choice === 'custom') { const details = customOnBehalfOf[statusPath] - return kind === 'trigger' ? details?.permissionedAs : details?.email + return deployItemIdentityIsPrincipal(kind) ? details?.permissionedAs : details?.email } // 'me' or undefined = don't pass, backend will use deploying user's identity return undefined @@ -115,7 +117,8 @@ statusPath: string, kind: Kind ): string | undefined { - if (kind === 'trigger' || onBehalfOfChoice[statusPath] !== 'custom') return undefined + if (deployItemIdentityIsPrincipal(kind)) return undefined + if (onBehalfOfChoice[statusPath] !== 'custom') return undefined return customOnBehalfOf[statusPath]?.permissionedAs } diff --git a/frontend/src/lib/components/OnBehalfOfSelector.svelte b/frontend/src/lib/components/OnBehalfOfSelector.svelte index 67648936ae..11e1ab49cf 100644 --- a/frontend/src/lib/components/OnBehalfOfSelector.svelte +++ b/frontend/src/lib/components/OnBehalfOfSelector.svelte @@ -8,6 +8,21 @@ permissionedAs: string } + /** + * Whether a deploy carries this kind's identity as a principal (`u/alice`, `g/ops`) rather + * than as an address. Apps sit with triggers and schedules: an app's policy stores only the + * principal, and a group has no address of its own to travel as. Flows and scripts still send + * an address, which the backend resolves against the target workspace. + * + * Both halves of a deploy read this — what `getOnBehalfOf` fetches out of the target and what + * is sent back — so a kind moved between the two sides has to move in `deployItem` with it. + */ + export function deployItemIdentityIsPrincipal(kind: string): boolean { + return ( + kind === 'app' || kind === 'raw_app' || kind === 'trigger' || isTriggerOrScheduleKind(kind) + ) + } + /** * Check if an item needs on_behalf_of selection. * Shows the selector when the source item has an on_behalf_of value set. diff --git a/frontend/src/lib/components/UpdateDevWorkspaceModal.svelte b/frontend/src/lib/components/UpdateDevWorkspaceModal.svelte index c53b1436c8..3b0a106112 100644 --- a/frontend/src/lib/components/UpdateDevWorkspaceModal.svelte +++ b/frontend/src/lib/components/UpdateDevWorkspaceModal.svelte @@ -17,6 +17,7 @@ } from '$lib/utils_workspace_deploy' import { COMPARE_ITEMS_PARAM } from '$lib/components/sessions/modifiedItemsMask' import OnBehalfOfSelector, { + deployItemIdentityIsPrincipal, needsOnBehalfOfSelection, type OnBehalfOfChoice, type OnBehalfOfDetails @@ -230,13 +231,14 @@ ) return } + const identityIsPrincipal = deployItemIdentityIsPrincipal(req.itemType) const deployed = await deployItem({ kind: req.itemType, path: req.itemPath, workspaceFrom: req.prodWorkspaceId, workspaceTo: req.devWorkspaceId, - onBehalfOf: chosenIdentity?.email, - onBehalfOfPrincipal: chosenIdentity?.permissionedAs, + onBehalfOf: identityIsPrincipal ? chosenIdentity?.permissionedAs : chosenIdentity?.email, + onBehalfOfPrincipal: identityIsPrincipal ? undefined : chosenIdentity?.permissionedAs, createOnly: true }) if (deployed.conflict) { diff --git a/frontend/src/lib/utils_workspace_deploy.ts b/frontend/src/lib/utils_workspace_deploy.ts index 228e32fc06..d5608c5615 100644 --- a/frontend/src/lib/utils_workspace_deploy.ts +++ b/frontend/src/lib/utils_workspace_deploy.ts @@ -170,7 +170,7 @@ function legacyTriggerKind(kind: TriggerDeployKind) { return map[kind] } -/** An identity in both formats the app policy stores it in. */ +/** An identity in both formats a deploy may carry it in — see `deployItemIdentityIsPrincipal`. */ export type AppIdentity = { email: string; permissionedAs: string } /** @@ -189,13 +189,10 @@ export type DeployConflict = { hit: boolean } * does the same, but this app consumes the published package, so the rewrite has to exist * on both sides until that version ships. * - * An app carries both halves inside its `policy` instead, so it needs both stamped here: - * the published package leaves the policy untouched, and a source principal beside a - * target address is rejected as a pair naming two different accounts. - * - * A group kept as the target identity travels as its synthetic `group-*@windmill.dev` - * address, which an admin-created account holding it would win on the backend: known and - * accepted, see `users::permissioned_as_from_email`. + * An app carries its identity inside its `policy` instead, and as a principal rather than an + * address — `deployItemIdentityIsPrincipal` says which kinds do. So `onBehalfOf` is already + * the `u/`/`g/` form for one, and it is stamped into the policy here because the published + * package leaves that untouched. */ function makeProvider( onBehalfOfPrincipal?: string, @@ -219,8 +216,8 @@ function makeProvider( ...app, policy: { ...app.policy, - on_behalf_of: onBehalfOfPrincipal, - on_behalf_of_email: onBehalfOf + on_behalf_of: onBehalfOf, + on_behalf_of_email: undefined } } : app @@ -342,17 +339,18 @@ export interface DeployItemParams { /** * The value to use for on_behalf_of when deploying. * Format varies by item kind: - * - For flows/scripts/apps: an email address (on_behalf_of_email) - * - For triggers/schedules: permissioned_as format (u/username or g/group) + * - For flows/scripts: an email address (on_behalf_of_email) + * - For apps, triggers and schedules: permissioned_as format (u/username or g/group) + * `deployItemIdentityIsPrincipal` is the authority on which of the two a kind takes. * If set, preserve_on_behalf_of / preserve_permissioned_as will be true. * If undefined, the deploying user's identity is used. */ onBehalfOf?: string /** - * Authorization half of `onBehalfOf` (u/username or g/group). Must name the same identity as - * `onBehalfOf`. Set it only when the user picked a specific user; undefined clears the key, - * leaving the backend to derive the target workspace's own principal from `onBehalfOf`. Apps - * additionally need it in the policy, which holds both formats — see `makeProvider`. + * Authorization half of `onBehalfOf` for the kinds that send an address. Must name the same + * identity as `onBehalfOf`. Set it only when the user picked a specific user; undefined + * clears the key, leaving the backend to derive the target workspace's own principal from + * `onBehalfOf`. Leave it unset for a kind that already sends a principal. */ onBehalfOfPrincipal?: string /** @@ -548,6 +546,11 @@ export async function getItemValue( /** * Get the on_behalf_of value for a deployable item. + * + * The app branch is answered here rather than by the shared helper: the published package + * still reads the policy's address, which is the target's own only by coincidence — a group's + * is `group-*@windmill.dev`, and the backend hands that to whichever account holds it. Drop + * this once a package carrying the principal branch ships. */ export async function getOnBehalfOf( kind: Kind, @@ -562,6 +565,13 @@ export async function getOnBehalfOf( return undefined } } + if (kind === 'app' || kind === 'raw_app') { + try { + return (await AppService.getAppByPath({ workspace, path })).policy?.on_behalf_of + } catch { + return undefined + } + } return sharedGetOnBehalfOf(makeProvider(), kind as DeployKind, path, workspace) } @@ -579,7 +589,7 @@ export async function getOnBehalfOfOrThrow( if (kind === 'flow') return (await provider.getFlowByPath({ workspace, path })).on_behalf_of_email if (kind === 'script') return (await provider.getScriptByPath({ workspace, path })).on_behalf_of_email - return (await provider.getAppByPath({ workspace, path })).policy?.on_behalf_of_email + return (await provider.getAppByPath({ workspace, path })).policy?.on_behalf_of } /**