diff --git a/backend/migrations/20260912072924_drop_app_policy_on_behalf_of_email.up.sql b/backend/migrations/20260912072924_drop_app_policy_on_behalf_of_email.up.sql index 41377eb2c5..ca00504a04 100644 --- a/backend/migrations/20260912072924_drop_app_policy_on_behalf_of_email.up.sql +++ b/backend/migrations/20260912072924_drop_app_policy_on_behalf_of_email.up.sql @@ -1,6 +1,14 @@ -- The app policy's address is derived from its principal on every read and no longer written, -- so a stored copy is inert and, once an account is renamed or its address changes, wrong. -UPDATE app SET policy = policy - 'on_behalf_of_email' WHERE policy ? 'on_behalf_of_email'; +-- +-- Only where there is a principal to derive from. `20260911085221` left `on_behalf_of` unset +-- wherever the address named nobody or resolved to something too wide to enqueue; for those rows +-- the address is the sole surviving record of the intended identity and nothing can recompute it. +-- They already fail `get_on_behalf_of` on the missing principal, so keeping it costs nothing. +UPDATE app SET policy = policy - 'on_behalf_of_email' +WHERE policy ? 'on_behalf_of_email' AND policy ? 'on_behalf_of'; UPDATE draft SET value = to_json(to_jsonb(value) #- '{policy,on_behalf_of_email}') -WHERE typ IN ('app', 'raw_app') AND to_jsonb(value) #> '{policy}' ? 'on_behalf_of_email'; +WHERE typ IN ('app', 'raw_app') + AND to_jsonb(value) #> '{policy}' ? 'on_behalf_of_email' + AND to_jsonb(value) #> '{policy}' ? 'on_behalf_of'; diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index ec3c855a57..d48b99cec3 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -33950,7 +33950,7 @@ components: on_behalf_of_email: type: string 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`. + 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 beside `on_behalf_of` is ignored, and sending it alone with `preserve_on_behalf_of` is rejected, so a deploy that means to keep an identity has to name it in `on_behalf_of`. sandbox: type: boolean description: > diff --git a/cli/src/commands/workspace/merge.ts b/cli/src/commands/workspace/merge.ts index 60a45c7895..0548e49282 100644 --- a/cli/src/commands/workspace/merge.ts +++ b/cli/src/commands/workspace/merge.ts @@ -188,14 +188,22 @@ function triggerService(kind: TriggerDeployKind) { * * A flow or script sends an address, which the backend resolves against the target, so the * source's answer carries over untouched. An app sends a principal — its policy stores nothing - * else — and `u/` names are per-workspace, so the source's is mapped through the address it - * resolves to. That mapping is what the backend used to do for as long as the address itself - * travelled; without it the app would land on whoever holds that username in the target, or on - * nobody. A group carries over by name: `g/ops` means the target's ops group, which is the - * identity a fork and its parent are meant to share. + * else — so a `u/` name, which is per-workspace, is re-read as the source member it names and + * looked up again in the target. That is the mapping the backend did for as long as the address + * itself travelled; without it the app lands on whoever holds that username in the target, or on + * nobody. * - * Throws when the source's account has no counterpart in the target, which is the honest answer - * — deploying as the pusher instead would silently widen what the app runs as. + * Two principals are *not* workspace-scoped and carry over untouched: `g/ops` means the target's + * ops group, which is the identity a fork and its parent are meant to share, and a `u/` name with + * no member behind it is an instance-level superadmin, valid in every workspace through the same + * `password` fallback the backend resolves it by. + * + * The member's address comes from `usr` via `listUsers`, never from the app read's derived + * `on_behalf_of_email`: that one is served from a cache that may be a notify poll stale, and this + * answer is about to be persisted as the target's identity. + * + * Throws when a source member has no counterpart in the target, which is the honest answer — + * deploying as the pusher instead would silently widen what the app runs as. */ export async function preservedIdentity( deployProvider: DeployProvider, @@ -203,25 +211,34 @@ export async function preservedIdentity( path: string, workspaceFrom: string, workspaceTo: string, - usernames: Map + usernamesInSource: Map, + usernamesInTarget: Map ): Promise { if (kind !== "app" && kind !== "raw_app") { return await getOnBehalfOf(deployProvider, kind, path, workspaceFrom); } - const { lookupUsernameByEmail } = await import("../../core/permissioned_as.ts"); + const { lookupEmailByUsername, lookupUsernameByEmail } = await import( + "../../core/permissioned_as.ts" + ); const policy = ( await deployProvider.getAppByPath({ workspace: workspaceFrom, path }) - )?.policy as - | { on_behalf_of?: string; on_behalf_of_email?: string } - | undefined; + )?.policy as { on_behalf_of?: string } | undefined; const principal = policy?.on_behalf_of; - if (!principal?.startsWith("u/") || !policy?.on_behalf_of_email) { + if (!principal?.startsWith("u/")) { + return principal; + } + const email = await lookupEmailByUsername( + workspaceFrom, + principal.slice(2), + usernamesInSource + ); + if (!email) { return principal; } return `u/${await lookupUsernameByEmail( workspaceTo, - policy.on_behalf_of_email, - usernames + email, + usernamesInTarget )}`; } @@ -585,8 +602,9 @@ async function mergeWorkspaces( } let successCount = 0; let failCount = 0; - // One `listUsers` of the target for the whole run, shared by every app that maps an identity - // into it (`preservedIdentity`). + // One `listUsers` per workspace for the whole run, shared by every app that maps an identity + // across (`preservedIdentity`). + const usernamesInSource = new Map(); const usernamesInTarget = new Map(); // Datatable migrations deployed (not deleted) into the target. Deploying a // migration only upserts its definition — the target schema is unchanged until @@ -626,6 +644,7 @@ async function mergeWorkspaces( diff.path, workspaceFrom, workspaceTo, + usernamesInSource, usernamesInTarget ); } catch (e: any) { diff --git a/cli/src/core/permissioned_as.ts b/cli/src/core/permissioned_as.ts index d0301373da..b4b4a2c3d2 100644 --- a/cli/src/core/permissioned_as.ts +++ b/cli/src/core/permissioned_as.ts @@ -58,6 +58,23 @@ async function ensureUserCache( } } +/** + * The address a workspace member's username belongs to, or `undefined` when the workspace has no + * such member. Absence is an answer, not a failure: a principal naming no `usr` row is an + * instance-level identity (a superadmin acting outside their workspaces), which is not + * workspace-scoped and so means the same thing in every workspace. + * + * Reads `usr` through `listUsers`, so it is never the cached address a derived read would give. + */ +export async function lookupEmailByUsername( + workspace: string, + username: string, + cache: Map +): Promise { + await ensureUserCache(workspace, cache); + return cache.get(username)?.email; +} + export async function lookupUsernameByEmail( workspace: string, email: string, diff --git a/cli/test/preserved_identity_unit.test.ts b/cli/test/preserved_identity_unit.test.ts new file mode 100644 index 0000000000..db1eb3b2fc --- /dev/null +++ b/cli/test/preserved_identity_unit.test.ts @@ -0,0 +1,83 @@ +import { expect, test } from "bun:test"; +import { preservedIdentity } from "../src/commands/workspace/merge.ts"; + +// A merge reads an app's identity out of the source workspace, but `u/` names are per-workspace, +// so what it sends has to be the target's name for the same account. Getting this wrong hands the +// app to whoever holds that username in the target, silently — so pin each branch. +// +// Both caches are pre-seeded, which is also what keeps this off the network: `ensureUserCache` +// short-circuits on a non-empty map. +function seeded(...users: { username: string; email: string }[]) { + const cache = new Map(); + for (const u of users) { + cache.set(u.username, u); + cache.set(u.email, u); + } + return cache; +} + +function providerFor(on_behalf_of: string | undefined) { + return { getAppByPath: async () => ({ policy: { on_behalf_of } }) } as any; +} + +const source = () => seeded({ username: "alice", email: "alice@corp" }); +const target = () => seeded({ username: "alice_dst", email: "alice@corp" }); + +test("preservedIdentity: a member's principal is remapped to the target's name for them", async () => { + expect( + await preservedIdentity( + providerFor("u/alice"), + "app" as any, + "f/x/a", + "src", + "dst", + source(), + target(), + ), + ).toBe("u/alice_dst"); +}); + +test("preservedIdentity: a group carries over by name", async () => { + expect( + await preservedIdentity( + providerFor("g/ops"), + "app" as any, + "f/x/a", + "src", + "dst", + source(), + target(), + ), + ).toBe("g/ops"); +}); + +// An instance superadmin has no `usr` row in either workspace, so there is nothing to map — and +// nothing to map *to*: the backend resolves that principal through the instance-wide `password` +// fallback, which means the same account in every workspace. +test("preservedIdentity: an identity with no member behind it carries over untouched", async () => { + expect( + await preservedIdentity( + providerFor("u/superadmin-external"), + "app" as any, + "f/x/a", + "src", + "dst", + source(), + target(), + ), + ).toBe("u/superadmin-external"); +}); + +test("preservedIdentity: a member absent from the target fails rather than falling back", async () => { + expect( + preservedIdentity( + providerFor("u/alice"), + "app" as any, + "f/x/a", + "src", + "dst", + source(), + seeded({ username: "bob", email: "bob@corp" }), + ), + ).rejects.toThrow(/Could not find username for email/); +}); diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte index f128da0c24..08e6aeb501 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte @@ -318,6 +318,11 @@ selected={onBehalfOfChoice} onSelect={(choice, details) => { onBehalfOfChoice = choice + // `policy` came from a read, which derives the address from the principal being + // replaced here. It is inert on this release, but a peer still on the one before + // reads the pair and refuses it as naming two accounts, so the deploy fails outright + // mid-rollover. Every other writer of this policy clears it for the same reason. + delete policy.on_behalf_of_email if (choice === 'me') { policy.on_behalf_of = `u/${$userStore?.username}` customOnBehalfOfEmail = ''