diff --git a/backend/.sqlx/query-5f1842cd56d68292854c938b24cebfdd57f2f412a8e33f4ac62d15ced48dc7bf.json b/backend/.sqlx/query-5f1842cd56d68292854c938b24cebfdd57f2f412a8e33f4ac62d15ced48dc7bf.json new file mode 100644 index 0000000000..3c21a6495f --- /dev/null +++ b/backend/.sqlx/query-5f1842cd56d68292854c938b24cebfdd57f2f412a8e33f4ac62d15ced48dc7bf.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft SET value = to_json(to_jsonb(value) #- '{policy,on_behalf_of_email}') WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of_email' = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [] + }, + "hash": "5f1842cd56d68292854c938b24cebfdd57f2f412a8e33f4ac62d15ced48dc7bf" +} diff --git a/backend/.sqlx/query-61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878.json b/backend/.sqlx/query-61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878.json new file mode 100644 index 0000000000..f50b4b743f --- /dev/null +++ b/backend/.sqlx/query-61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM ws_specific\n WHERE workspace_id = $1 AND item_kind = 'variable' AND path = ANY($2)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "TextArray" + ] + }, + "nullable": [] + }, + "hash": "61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878" +} diff --git a/backend/.sqlx/query-a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d.json b/backend/.sqlx/query-a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d.json new file mode 100644 index 0000000000..fa43d27ed8 --- /dev/null +++ b/backend/.sqlx/query-a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d.json @@ -0,0 +1,24 @@ +{ + "db_name": "PostgreSQL", + "query": "WITH survivors AS (\n SELECT value::text AS rendered FROM resource\n WHERE workspace_id = $1 AND NOT (path = ANY($2::text[]))\n )\n SELECT v.path FROM unnest($3::text[]) AS v(path)\n WHERE EXISTS (\n SELECT 1 FROM survivors s\n WHERE strpos(s.rendered, '\"$var:' || v.path || '\"') > 0\n OR strpos(s.rendered, '\"$jsonvar:' || v.path || '\"') > 0\n )", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "TextArray", + "TextArray" + ] + }, + "nullable": [ + null + ] + }, + "hash": "a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d" +} diff --git a/backend/.sqlx/query-d1d8b0f8a1e88a04056c245a6f28971a3f9fe209f8092f400c660814f6ee61bd.json b/backend/.sqlx/query-d1d8b0f8a1e88a04056c245a6f28971a3f9fe209f8092f400c660814f6ee61bd.json new file mode 100644 index 0000000000..4782b87140 --- /dev/null +++ b/backend/.sqlx/query-d1d8b0f8a1e88a04056c245a6f28971a3f9fe209f8092f400c660814f6ee61bd.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE app SET policy = policy - 'on_behalf_of_email' WHERE policy->>'on_behalf_of_email' = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [] + }, + "hash": "d1d8b0f8a1e88a04056c245a6f28971a3f9fe209f8092f400c660814f6ee61bd" +} diff --git a/backend/.sqlx/query-d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5.json b/backend/.sqlx/query-d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5.json new file mode 100644 index 0000000000..b0c2d95175 --- /dev/null +++ b/backend/.sqlx/query-d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM variable WHERE workspace_id = $1 AND path = ANY($2) RETURNING path", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "TextArray" + ] + }, + "nullable": [ + false + ] + }, + "hash": "d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5" +} 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 ca00504a04..f944f58402 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 @@ -5,10 +5,13 @@ -- 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. +-- +-- Tested through `->>` rather than `?`: a policy `Policy` wrote serializes an absent principal as +-- an explicit JSON null, which `?` counts as present. UPDATE app SET policy = policy - 'on_behalf_of_email' -WHERE policy ? 'on_behalf_of_email' AND policy ? 'on_behalf_of'; +WHERE policy ? 'on_behalf_of_email' AND policy->>'on_behalf_of' IS NOT NULL; 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' - AND to_jsonb(value) #> '{policy}' ? 'on_behalf_of'; + AND value->'policy'->>'on_behalf_of' IS NOT NULL; diff --git a/backend/windmill-api-integration-tests/tests/users.rs b/backend/windmill-api-integration-tests/tests/users.rs index 56ca64c832..18020b44df 100644 --- a/backend/windmill-api-integration-tests/tests/users.rs +++ b/backend/windmill-api-integration-tests/tests/users.rs @@ -626,6 +626,15 @@ async fn test_change_user_email(db: Pool) -> anyhow::Result<()> { ) .execute(&db) .await?; + // The shape a peer still on the release before this one re-adds on every save: it prefers + // that address on reads, so one left behind would answer with the address this change moves + // away from for as long as that peer is live. + sqlx::query!( + "INSERT INTO app(workspace_id, path, summary, policy, versions) + VALUES ('test-workspace', 'u/test-user-2/legacy', '', '{\"on_behalf_of\": \"u/test-user-2\", \"on_behalf_of_email\": \"test2@windmill.dev\"}'::jsonb, '{}')" + ) + .execute(&db) + .await?; sqlx::query!( "INSERT INTO folder(workspace_id, name, display_name, owners, extra_perms, default_permissioned_as) VALUES ('test-workspace', 'fold', 'fold', '{}', '{}'::jsonb, @@ -679,6 +688,17 @@ async fn test_change_user_email(db: Pool) -> anyhow::Result<()> { "app policy should carry only the new address: {policy}" ); + let legacy = sqlx::query_scalar!( + "SELECT policy::text FROM app WHERE path = 'u/test-user-2/legacy' AND workspace_id = 'test-workspace'" + ) + .fetch_one(&db) + .await? + .unwrap_or_default(); + assert!( + !legacy.contains("on_behalf_of_email"), + "an address a peer re-added is dropped, not left naming the old account: {legacy}" + ); + // The rules keep their order, since the folder resolver takes the first glob that matches. let rules = sqlx::query_scalar!( "SELECT default_permissioned_as::text FROM folder WHERE workspace_id = 'test-workspace' AND name = 'fold'" diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 8b6371f5b5..6727f10462 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2062,6 +2062,25 @@ async fn change_user_email( .execute(&mut *tx) .await?; + // An app policy carries no address, so there is none to move — but a peer still running the + // release before this one writes it on every save and prefers it on reads, so one re-added + // after the migration would answer with the address this change is moving away from, for as + // long as that peer is live. Dropped rather than rewritten: the key has no place here, and + // every reader that matters derives it. + sqlx::query!( + "UPDATE app SET policy = policy - 'on_behalf_of_email' WHERE policy->>'on_behalf_of_email' = $1", + &old_email + ) + .execute(&mut *tx) + .await?; + + sqlx::query!( + r#"UPDATE draft SET value = to_json(to_jsonb(value) #- '{policy,on_behalf_of_email}') WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of_email' = $1"#, + &old_email + ) + .execute(&mut *tx) + .await?; + // ---- permissioned_as naming the account by its address ---- // `usr.username` is constrained to `[\w-]+`, so a workspace member is always named // `u/{username}` and their principals survive an address change untouched. The address form diff --git a/cli/src/commands/workspace/merge.ts b/cli/src/commands/workspace/merge.ts index 0548e49282..10fe77da9b 100644 --- a/cli/src/commands/workspace/merge.ts +++ b/cli/src/commands/workspace/merge.ts @@ -193,17 +193,19 @@ function triggerService(kind: TriggerDeployKind) { * itself travelled; without it the app lands on whoever holds that username in the target, or on * nobody. * - * 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. + * `g/ops` carries over by name: it means the target's ops group, which is the identity a fork and + * its parent are meant to share. So does a `u/` name with no member behind it in *either* + * workspace — that is an instance-level superadmin, which the backend resolves through the same + * `password` fallback everywhere. It has to be absent from both: the target's own `usr` row wins + * over that fallback, so carrying the name into a workspace that has a member of its own by that + * name would hand the app to them. * * 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. + * Throws rather than guessing whenever the two workspaces disagree about who a name belongs to. + * Deploying as the pusher instead would silently widen what the app runs as. */ export async function preservedIdentity( deployProvider: DeployProvider, @@ -227,19 +229,27 @@ export async function preservedIdentity( if (!principal?.startsWith("u/")) { return principal; } + const username = principal.slice(2); const email = await lookupEmailByUsername( workspaceFrom, - principal.slice(2), + username, usernamesInSource ); - if (!email) { - return principal; + if (email) { + return `u/${await lookupUsernameByEmail( + workspaceTo, + email, + usernamesInTarget + )}`; } - return `u/${await lookupUsernameByEmail( - workspaceTo, - email, - usernamesInTarget - )}`; + if (await lookupEmailByUsername(workspaceTo, username, usernamesInTarget)) { + throw new Error( + `'${principal}' names no member of ${workspaceFrom} but does name one of ${workspaceTo}, ` + + `so it cannot be carried across: deploying it would run the app as that account. ` + + `Set the app's identity in ${workspaceTo} explicitly.` + ); + } + return principal; } /** diff --git a/cli/test/preserved_identity_unit.test.ts b/cli/test/preserved_identity_unit.test.ts index db1eb3b2fc..984d5723f9 100644 --- a/cli/test/preserved_identity_unit.test.ts +++ b/cli/test/preserved_identity_unit.test.ts @@ -68,6 +68,22 @@ test("preservedIdentity: an identity with no member behind it carries over untou ).toBe("u/superadmin-external"); }); +// ...but only while the target has no member of that name. `resolve_username_to_email` prefers a +// `usr` row over the `password` fallback, so carrying it across would run the app as that member. +test("preservedIdentity: a name the target has a member for is refused, not carried", async () => { + expect( + preservedIdentity( + providerFor("u/saext"), + "app" as any, + "f/x/a", + "src", + "dst", + source(), + seeded({ username: "saext", email: "someone-else@corp" }), + ), + ).rejects.toThrow(/cannot be carried across/); +}); + test("preservedIdentity: a member absent from the target fails rather than falling back", async () => { expect( preservedIdentity( diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte index ab043f81a1..4a32bfd7ae 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte @@ -64,7 +64,7 @@ import type { DiffDrawerI } from '$lib/components/diff_drawer' import AppEditorHeaderDeploy from './AppEditorHeaderDeploy.svelte' import { computeSecretUrl } from './appDeploy.svelte' - import { updatePolicy } from './appPolicy' + import { policyForDeploy, updatePolicy } from './appPolicy' import { editInForkAllowed, editInForkLabel, openEditInFork } from '$lib/utils/editInFork' import { isCloudHosted } from '$lib/cloud' @@ -253,7 +253,7 @@ value: $app, path, summary: $summary, - policy, + policy: policyForDeploy(policy), deployment_message: deploymentMsg, custom_path: customPath, preserve_on_behalf_of: preserveOnBehalfOf || undefined, @@ -357,7 +357,7 @@ requestBody: { value: $app!, summary: $summary, - policy, + policy: policyForDeploy(policy), path: npath, deployment_message: deploymentMsg, // custom_path requires admin so to accept update without it, we need to send as undefined when non-admin (when undefined, it will be ignored) @@ -410,7 +410,7 @@ await AppService.updateApp({ workspace: $workspaceStore!, path: $appPath, - requestBody: { policy } + requestBody: { policy: policyForDeploy(policy) } }) if (message) { sendUserToast(message) diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte index 08e6aeb501..f128da0c24 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte @@ -318,11 +318,6 @@ 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 = '' diff --git a/frontend/src/lib/components/apps/editor/appPolicy.ts b/frontend/src/lib/components/apps/editor/appPolicy.ts index b6e35bd65e..2984e54254 100644 --- a/frontend/src/lib/components/apps/editor/appPolicy.ts +++ b/frontend/src/lib/components/apps/editor/appPolicy.ts @@ -189,6 +189,18 @@ export async function updatePolicy(app: App, currentPolicy: Policy | undefined): return next } +/** + * The policy as a deploy should send it. + * + * A read derives `on_behalf_of_email` from the principal, and the editor keeps it to show who the + * app runs as — but sending it back beside a principal the user has just changed reads, to a peer + * that still stores the address, as a pair naming two accounts, and the deploy is refused. Built + * as a copy so the object the editor renders from keeps the address it displays. + */ +export function policyForDeploy>(policy: T): T { + return { ...policy, on_behalf_of_email: undefined } +} + export async function processRunnable( id: string, runnable: Runnable, diff --git a/frontend/src/lib/rawAppDeploy.ts b/frontend/src/lib/rawAppDeploy.ts index 2a9bea2973..65277e2628 100644 --- a/frontend/src/lib/rawAppDeploy.ts +++ b/frontend/src/lib/rawAppDeploy.ts @@ -23,6 +23,7 @@ import { normalizeRawAppData } from '$lib/components/raw_apps/rawAppDraftValue' import { stateSnapshot } from '$lib/svelte5Utils.svelte' +import { policyForDeploy } from '$lib/components/apps/editor/appPolicy' /** * Deploy an explicit raw-app value — one the user edited as JSON, or one @@ -73,7 +74,7 @@ export async function deployRawAppValue({ // version can still carry the pre-`data` datatable shapes. value: { files, runnables, data: normalizeRawAppData(plainValue) }, summary: summary ?? '', - policy, + policy: policyForDeploy(policy), deployment_message: deploymentMessage, // custom_path changes require admin (see deployRawAppDraft). custom_path: isAdmin ? (customPath ?? '') : undefined, @@ -140,7 +141,7 @@ export async function deployRawAppDraft( path: targetPath, value: rawAppValue, summary, - policy, + policy: policyForDeploy(policy), deployment_message: deploymentMessage, custom_path: isAdmin ? (value.custom_path ?? '') : undefined, // Preserve the policy's on_behalf_of: this draft-deploy path has no @@ -160,7 +161,7 @@ export async function deployRawAppDraft( path: targetPath, value: rawAppValue, summary, - policy, + policy: policyForDeploy(policy), deployment_message: deploymentMessage, custom_path: value.custom_path, // Preserve the policy's on_behalf_of (see update branch above).