From c90d1d95c2777f8d1864fce68901e2e221ff90fc Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 12 Sep 2026 09:10:15 +0200 Subject: [PATCH] refactor: make the app policy's principal the authority for its identity (#10440) * refactor: make the app policy's principal the authority for its identity * fix: align the app backfill with the sibling migration and audit the uncached address * chore: refresh the sqlx cache after rebasing onto the merged base * fix: resolve the app execution address uncached, it decides the job's authorization * chore: cache the EE queries at the ref this branch pins * chore: cache the EE queries at the ref this branch pins * fix: derive the app draft's on-behalf-of address on read * chore: cache the query the draft derivation test added * fix: derive the app identity on the draft-table and version reads too * docs: state the draft resolver's authorization contract * fix: resolve a draft's principal against workspace membership only * chore: cache the membership lookup the draft resolver added * fix: drop an unresolvable draft's address instead of leaving it stale * perf: evict the address cache on change so app dispatch can read it * fix: evict on superadmin role changes, not only address changes * refactor: make the app policy's address optional instead of derived on read * fix: follow an external superadmin's rename into the apps that name them * docs: state the removal gate once, and correctly * refactor: drop the app-policy version constant that gated nothing * docs: drop the last reference to the removed constant * perf: read the address cache everywhere now that eviction reaches every replica * fix: keep persisted addresses off the cache the poller evicts asynchronously * docs: state where the cached address is accepted and where it is not * docs: keep the cache rule in one place and drop the stale premise * docs: sort the two lookups by how long a wrong answer lives * fix: resolve the schedule address uncached where it is written to the row * docs: name the release this actually ships in * perf: evict a superadmin's key per workspace instead of the whole cache * fix: evict every alias a superadmin principal can be spelled as * docs: describe the trigger as it is * docs: cover the round-tripped read in the cache rule * docs: record why a stale dispatch address cannot escalate * fix: validate a dispatch address against the principal's live binding * fix: carry the validated address through to the job row and token * fix: record the validated address on the job row, not the one handed in * test: run the substep tag check as the non-superadmin it means to test Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * fix: rewrite a stored app address that disagrees with its principal Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * docs: record the accepted staleness window of the cached dispatch address Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * fix: record the validated address on the job's audit row Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * docs: record the accepted rename race of pre-transaction identity resolution Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * docs: separate the app's stored address from the derived one in the resolver doc Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * docs: describe the job identity fast path the push comments skipped Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * fix: backfill a legacy group-prefixed username as the group it names Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * fix: resolve a schedule edit's identity before opening its transaction Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * fix: never resolve a disabled member to a same-named superadmin Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * docs: state what the email-change notify buys, and rewrap two comment lines Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * fix: keep a group's runnables when offboarding a legacy group-prefixed member Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * fix: read the app author from the stored address, as execution does Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * docs: record the rename race's full consequence as a known, accepted limitation Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc * docs: record the keep-target group address case as a known, accepted limitation Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc --------- Co-authored-by: Claude Opus 5 (1M context) --- ...2b0cb7770950e5c7fa6128bc9d2b2abcc7f2f.json | 15 ++ ...c1aeca1b9fe784336fabb71ce13bdb58839da.json | 15 ++ ...52898bb83047f3563f789753e8fec030d301f.json | 16 ++ ...c26f3dd6ff9aa87293ca671be32fd1305a73a.json | 16 ++ ...ed693e011a89aefb3a0e239630e6950dca5cd.json | 17 ++ ...1dede87b6793f92af884a023253a0ffc3a905.json | 17 ++ ...2ea3d912ec703687f6ce52d2419a410b1599e.json | 16 ++ ...6df004f8cb7091588309a1f484b91fe7183fc.json | 35 +++ ..._backfill_app_policy_on_behalf_of.down.sql | 6 + ...21_backfill_app_policy_on_behalf_of.up.sql | 100 +++++++++ ...11085230_notify_user_email_change.down.sql | 8 + ...0911085230_notify_user_email_change.up.sql | 85 +++++++ backend/src/main.rs | 11 + .../tests/folder_default_permissioned_as.rs | 2 +- backend/tests/preserve_on_behalf_of.rs | 128 ++++++++--- backend/tests/worker.rs | 2 +- backend/windmill-api-auth/src/lib.rs | 9 +- backend/windmill-api-flows/src/flows.rs | 2 +- .../tests/offboarding.rs | 56 +++++ backend/windmill-api-schedule/src/lib.rs | 19 +- backend/windmill-api-users/src/users.rs | 28 ++- backend/windmill-api/openapi.yaml | 2 + backend/windmill-api/src/apps.rs | 212 +++++++++++------- backend/windmill-api/src/offboarding.rs | 47 ++-- backend/windmill-api/src/users.rs | 47 ++++ backend/windmill-api/src/workspaces_export.rs | 4 +- backend/windmill-common/src/auth.rs | 56 +++-- backend/windmill-common/src/folders.rs | 2 - backend/windmill-common/src/lib.rs | 36 ++- backend/windmill-common/src/scripts.rs | 3 - backend/windmill-common/src/users.rs | 67 +++++- .../windmill-common/tests/notify_events.rs | 71 ++++++ .../tests/permissioned_as_authz.rs | 81 +++++++ backend/windmill-queue/src/jobs.rs | 11 +- backend/windmill-test-utils/src/lib.rs | 14 +- backend/windmill-worker/src/worker_flow.rs | 2 + cli/test/deploy_on_behalf_of_unit.test.ts | 46 +++- cli/windmill-utils-internal/src/deploy.ts | 15 +- docs/app-policy-email-removal.md | 47 ++++ frontend/src/lib/utils_workspace_deploy.ts | 68 +++--- 40 files changed, 1212 insertions(+), 222 deletions(-) create mode 100644 backend/.sqlx/query-32b4d1fe69fd219a5931fdd63cb2b0cb7770950e5c7fa6128bc9d2b2abcc7f2f.json create mode 100644 backend/.sqlx/query-3721bd6524ea48a1068ee8013bcc1aeca1b9fe784336fabb71ce13bdb58839da.json create mode 100644 backend/.sqlx/query-79d3bbd278c34734ce5babc3be852898bb83047f3563f789753e8fec030d301f.json create mode 100644 backend/.sqlx/query-98cb765a480dac8a27ecad5df26c26f3dd6ff9aa87293ca671be32fd1305a73a.json create mode 100644 backend/.sqlx/query-99194b850cb30d174c8d99303f6ed693e011a89aefb3a0e239630e6950dca5cd.json create mode 100644 backend/.sqlx/query-bff72874d1fdee7d572e2677aea1dede87b6793f92af884a023253a0ffc3a905.json create mode 100644 backend/.sqlx/query-d599e8058e96f3708cf6af2cc1a2ea3d912ec703687f6ce52d2419a410b1599e.json create mode 100644 backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json create mode 100644 backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.down.sql create mode 100644 backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.up.sql create mode 100644 backend/migrations/20260911085230_notify_user_email_change.down.sql create mode 100644 backend/migrations/20260911085230_notify_user_email_change.up.sql create mode 100644 backend/windmill-common/tests/permissioned_as_authz.rs create mode 100644 docs/app-policy-email-removal.md diff --git a/backend/.sqlx/query-32b4d1fe69fd219a5931fdd63cb2b0cb7770950e5c7fa6128bc9d2b2abcc7f2f.json b/backend/.sqlx/query-32b4d1fe69fd219a5931fdd63cb2b0cb7770950e5c7fa6128bc9d2b2abcc7f2f.json new file mode 100644 index 0000000000..941dc122d0 --- /dev/null +++ b/backend/.sqlx/query-32b4d1fe69fd219a5931fdd63cb2b0cb7770950e5c7fa6128bc9d2b2abcc7f2f.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb($1::text))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of' = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "32b4d1fe69fd219a5931fdd63cb2b0cb7770950e5c7fa6128bc9d2b2abcc7f2f" +} diff --git a/backend/.sqlx/query-3721bd6524ea48a1068ee8013bcc1aeca1b9fe784336fabb71ce13bdb58839da.json b/backend/.sqlx/query-3721bd6524ea48a1068ee8013bcc1aeca1b9fe784336fabb71ce13bdb58839da.json new file mode 100644 index 0000000000..ebaaf0b755 --- /dev/null +++ b/backend/.sqlx/query-3721bd6524ea48a1068ee8013bcc1aeca1b9fe784336fabb71ce13bdb58839da.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of_email'], to_jsonb($1::text))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of_email' = $2 AND (value->'policy'->>'on_behalf_of' IS NULL OR value->'policy'->>'on_behalf_of' NOT LIKE 'g/%')", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "3721bd6524ea48a1068ee8013bcc1aeca1b9fe784336fabb71ce13bdb58839da" +} diff --git a/backend/.sqlx/query-79d3bbd278c34734ce5babc3be852898bb83047f3563f789753e8fec030d301f.json b/backend/.sqlx/query-79d3bbd278c34734ce5babc3be852898bb83047f3563f789753e8fec030d301f.json new file mode 100644 index 0000000000..98aa0d9367 --- /dev/null +++ b/backend/.sqlx/query-79d3bbd278c34734ce5babc3be852898bb83047f3563f789753e8fec030d301f.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE app SET policy = jsonb_set(policy, ARRAY['on_behalf_of'], to_jsonb($1::text))\n WHERE policy->>'on_behalf_of' = $2 AND policy->>'on_behalf_of_email' = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "79d3bbd278c34734ce5babc3be852898bb83047f3563f789753e8fec030d301f" +} diff --git a/backend/.sqlx/query-98cb765a480dac8a27ecad5df26c26f3dd6ff9aa87293ca671be32fd1305a73a.json b/backend/.sqlx/query-98cb765a480dac8a27ecad5df26c26f3dd6ff9aa87293ca671be32fd1305a73a.json new file mode 100644 index 0000000000..a4a692fd4b --- /dev/null +++ b/backend/.sqlx/query-98cb765a480dac8a27ecad5df26c26f3dd6ff9aa87293ca671be32fd1305a73a.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb('u/' || $1))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of' = ('u/' || $2) AND workspace_id = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "98cb765a480dac8a27ecad5df26c26f3dd6ff9aa87293ca671be32fd1305a73a" +} diff --git a/backend/.sqlx/query-99194b850cb30d174c8d99303f6ed693e011a89aefb3a0e239630e6950dca5cd.json b/backend/.sqlx/query-99194b850cb30d174c8d99303f6ed693e011a89aefb3a0e239630e6950dca5cd.json new file mode 100644 index 0000000000..9960ba85c9 --- /dev/null +++ b/backend/.sqlx/query-99194b850cb30d174c8d99303f6ed693e011a89aefb3a0e239630e6950dca5cd.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft SET value = to_json(jsonb_set(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb($1::text)), ARRAY['policy', 'on_behalf_of_email'], to_jsonb($4::text))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of' = $2 AND workspace_id = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "99194b850cb30d174c8d99303f6ed693e011a89aefb3a0e239630e6950dca5cd" +} diff --git a/backend/.sqlx/query-bff72874d1fdee7d572e2677aea1dede87b6793f92af884a023253a0ffc3a905.json b/backend/.sqlx/query-bff72874d1fdee7d572e2677aea1dede87b6793f92af884a023253a0ffc3a905.json new file mode 100644 index 0000000000..d64cf09687 --- /dev/null +++ b/backend/.sqlx/query-bff72874d1fdee7d572e2677aea1dede87b6793f92af884a023253a0ffc3a905.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE app SET policy = jsonb_set(\n jsonb_set(policy, ARRAY['on_behalf_of'], to_jsonb($1::text)),\n ARRAY['on_behalf_of_email'], to_jsonb($4::text)\n ) WHERE policy->>'on_behalf_of' = $2 AND workspace_id = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "bff72874d1fdee7d572e2677aea1dede87b6793f92af884a023253a0ffc3a905" +} diff --git a/backend/.sqlx/query-d599e8058e96f3708cf6af2cc1a2ea3d912ec703687f6ce52d2419a410b1599e.json b/backend/.sqlx/query-d599e8058e96f3708cf6af2cc1a2ea3d912ec703687f6ce52d2419a410b1599e.json new file mode 100644 index 0000000000..30cc0e7092 --- /dev/null +++ b/backend/.sqlx/query-d599e8058e96f3708cf6af2cc1a2ea3d912ec703687f6ce52d2419a410b1599e.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb($1::text)))\n WHERE typ IN ('app', 'raw_app')\n AND value->'policy'->>'on_behalf_of' = $2\n AND value->'policy'->>'on_behalf_of_email' = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "d599e8058e96f3708cf6af2cc1a2ea3d912ec703687f6ce52d2419a410b1599e" +} diff --git a/backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json b/backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json new file mode 100644 index 0000000000..669e78201d --- /dev/null +++ b/backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json @@ -0,0 +1,35 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT is_admin, operator, email FROM usr where username = $1 AND workspace_id = $2 AND disabled = false", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "is_admin", + "type_info": "Bool" + }, + { + "ordinal": 1, + "name": "operator", + "type_info": "Bool" + }, + { + "ordinal": 2, + "name": "email", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + false, + false + ] + }, + "hash": "dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc" +} diff --git a/backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.down.sql b/backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.down.sql new file mode 100644 index 0000000000..f615c55463 --- /dev/null +++ b/backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.down.sql @@ -0,0 +1,6 @@ +-- Add down migration script here +-- Nothing to undo. The up migration gives a policy that only ever carried the address the +-- principal it runs as, and rewrites an address that disagreed with its principal. The previous +-- version reads both halves, so both results are correct for it too, and the addresses replaced +-- named an account other than the one the app runs as. +SELECT 1; diff --git a/backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.up.sql b/backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.up.sql new file mode 100644 index 0000000000..fe97d7529c --- /dev/null +++ b/backend/migrations/20260911085221_backfill_app_policy_on_behalf_of.up.sql @@ -0,0 +1,100 @@ +-- Add up migration script here +-- `policy.on_behalf_of` becomes the authority for an app's identity: the address beside it is +-- written through from it on every save, so the two can no longer name different accounts. +-- +-- The address key is deliberately NOT removed here, and is still written: a replica predating +-- the derive-when-absent fallback errors outright when it is missing, which would 400 every +-- anonymous, publisher and guest app served by one that has not yet rolled over. Removing the +-- key is a follow-up, per docs/app-policy-email-removal.md. +-- +-- What is left is the data written before that rule. A policy that only ever had the address has +-- no principal to run as, so give it one. A policy whose halves disagree was stored as a client +-- sent it; reads return that pair and a redeploy that keeps the identity sends it back, where the +-- pair check rejects it. So once every policy has a principal, rewrite its address from it. + +-- Mirrors `users::username_to_permissioned_as`: an email-shaped username is its own principal +-- unless it contains a slash, which a reader would split on, and a legacy `group-*` username is +-- the group it names. +CREATE OR REPLACE FUNCTION pg_temp.username_to_permissioned_as(name VARCHAR) +RETURNS VARCHAR AS $$ + SELECT CASE + WHEN $1 LIKE '%@%' AND $1 LIKE '%/%' THEN 'u/' || $1 + WHEN $1 LIKE '%@%' THEN $1 + WHEN $1 LIKE 'group-%' THEN 'g/' || substr($1, 7) + ELSE 'u/' || $1 + END; +$$ LANGUAGE SQL IMMUTABLE; + +-- Mirrors `users::permissioned_as_from_email`: a real account wins over the synthetic group +-- namespace, which is not reserved and may be a user's own address. `pg_temp` lives for the +-- whole session and migrations share one connection, so an identically-named helper from an +-- earlier migration is still in scope: replace it, and drop this one at the end. +CREATE OR REPLACE FUNCTION pg_temp.permissioned_as_from_email(w_id VARCHAR, email VARCHAR) +RETURNS VARCHAR AS $$ + SELECT COALESCE( + (SELECT pg_temp.username_to_permissioned_as(u.username) + FROM usr u WHERE u.workspace_id = $1 AND u.email = $2), + -- A superadmin acting outside their workspaces has no usr row. + (SELECT pg_temp.username_to_permissioned_as(COALESCE(p.username, p.email)) + FROM password p WHERE p.email = $2 AND p.super_admin), + (SELECT 'g/' || g.name FROM group_ g + WHERE g.workspace_id = $1 + AND $2 = 'group-' || g.name || '@windmill.dev') + ); +$$ LANGUAGE SQL STABLE; + +-- Mirrors `users::get_email_from_permissioned_as`, except that a `u/` principal naming nobody +-- yields NULL rather than the synthetic `@unknown.windmill.dev` address, so that row is left as +-- it is instead of losing the one address it had. +CREATE OR REPLACE FUNCTION pg_temp.email_from_permissioned_as(w_id VARCHAR, principal VARCHAR) +RETURNS VARCHAR AS $$ + SELECT CASE + WHEN $2 LIKE 'u/%' THEN COALESCE( + (SELECT u.email FROM usr u WHERE u.workspace_id = $1 AND u.username = substr($2, 3)), + (SELECT p.email FROM password p + WHERE (p.username = substr($2, 3) OR p.email = substr($2, 3)) AND p.super_admin + ORDER BY p.email LIMIT 1)) + WHEN $2 LIKE 'g/%' THEN 'group-' || substr($2, 3) || '@windmill.dev' + ELSE $2 + END; +$$ 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 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 length(pg_temp.permissioned_as_from_email(workspace_id, value->'policy'->>'on_behalf_of_email')) <= 55; + +-- The address a save now writes, applied to the rows saved before. Execution already takes a `u/` +-- principal's own address, so this only changes what runs for a `g/` or bare principal, whose +-- stored address decided the superadmin flag and instance groups: those now follow the principal. +UPDATE app SET policy = jsonb_set(policy, ARRAY['on_behalf_of_email'], + to_jsonb(pg_temp.email_from_permissioned_as(workspace_id, policy->>'on_behalf_of'))) + WHERE policy->>'on_behalf_of' IS NOT NULL + AND pg_temp.email_from_permissioned_as(workspace_id, policy->>'on_behalf_of') IS NOT NULL + AND policy->>'on_behalf_of_email' + IS DISTINCT FROM pg_temp.email_from_permissioned_as(workspace_id, policy->>'on_behalf_of'); + +UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of_email'], + to_jsonb(pg_temp.email_from_permissioned_as(workspace_id, value->'policy'->>'on_behalf_of')))) + WHERE typ IN ('app', 'raw_app') + AND value->'policy'->>'on_behalf_of' IS NOT NULL + AND pg_temp.email_from_permissioned_as(workspace_id, value->'policy'->>'on_behalf_of') IS NOT NULL + AND value->'policy'->>'on_behalf_of_email' + IS DISTINCT FROM pg_temp.email_from_permissioned_as(workspace_id, value->'policy'->>'on_behalf_of'); + +DROP FUNCTION pg_temp.permissioned_as_from_email(VARCHAR, VARCHAR); +DROP FUNCTION pg_temp.email_from_permissioned_as(VARCHAR, VARCHAR); +DROP FUNCTION pg_temp.username_to_permissioned_as(VARCHAR); diff --git a/backend/migrations/20260911085230_notify_user_email_change.down.sql b/backend/migrations/20260911085230_notify_user_email_change.down.sql new file mode 100644 index 0000000000..7f6f494115 --- /dev/null +++ b/backend/migrations/20260911085230_notify_user_email_change.down.sql @@ -0,0 +1,8 @@ +-- Add down migration script here +DROP TRIGGER IF EXISTS password_superadmin_delete_trigger ON password; +DROP TRIGGER IF EXISTS password_superadmin_insert_trigger ON password; +DROP TRIGGER IF EXISTS password_superadmin_update_trigger ON password; +DROP TRIGGER IF EXISTS usr_email_update_trigger ON usr; +DROP TRIGGER IF EXISTS usr_email_change_trigger ON usr; +DROP FUNCTION IF EXISTS notify_superadmin_identity_change(); +DROP FUNCTION IF EXISTS notify_usr_email_change(); diff --git a/backend/migrations/20260911085230_notify_user_email_change.up.sql b/backend/migrations/20260911085230_notify_user_email_change.up.sql new file mode 100644 index 0000000000..f4e4a818f9 --- /dev/null +++ b/backend/migrations/20260911085230_notify_user_email_change.up.sql @@ -0,0 +1,85 @@ +-- Add up migration script here +-- Emit a notify_event so every process evicts its cached `permissioned_as` -> address mapping +-- (windmill-common EMAIL_CACHE) at its next notify-event poll, rather than serving the old +-- address for the rest of the TTL. Authorization does not rest on this: +-- `fetch_authed_from_permissioned_as` re-resolves the address from the principal's live binding. +-- SECURITY DEFINER so the INSERT runs as the function owner rather than the invoking +-- windmill_user/windmill_admin role, matching the other notify_* triggers. +CREATE OR REPLACE FUNCTION notify_usr_email_change() +RETURNS TRIGGER AS $$ +BEGIN + INSERT INTO notify_event (channel, payload) + VALUES ( + 'notify_user_email_change', + COALESCE(NEW.workspace_id, OLD.workspace_id) || ':' || COALESCE(NEW.username, OLD.username) + ); + -- A rename leaves the OLD username cached against this account's address; evict both keys. + IF TG_OP = 'UPDATE' AND NEW.username IS DISTINCT FROM OLD.username THEN + INSERT INTO notify_event (channel, payload) + VALUES ('notify_user_email_change', OLD.workspace_id || ':' || OLD.username); + END IF; + RETURN COALESCE(NEW, OLD); +END; +$$ LANGUAGE plpgsql SECURITY DEFINER; + +-- INSERT matters too: a lookup that resolved to nobody is cached as the synthetic +-- `{username}@unknown.windmill.dev`, so creating the row has to drop that entry. +CREATE TRIGGER usr_email_change_trigger +AFTER INSERT OR DELETE ON usr +FOR EACH ROW +EXECUTE FUNCTION notify_usr_email_change(); + +CREATE TRIGGER usr_email_update_trigger +AFTER UPDATE OF email, username ON usr +FOR EACH ROW +WHEN (OLD.email IS DISTINCT FROM NEW.email OR OLD.username IS DISTINCT FROM NEW.username) +EXECUTE FUNCTION notify_usr_email_change(); + +-- A superadmin acting outside their workspaces resolves through `password` instead, and that row +-- names no workspace of its own. The `*:` payload says so: the reader drops that name's entry in +-- every workspace rather than the whole cache, which would undo the caching on an instance that +-- rewrites these rows in bulk. Confined to superadmins because they are the only accounts the +-- `usr` triggers above cannot cover. +CREATE OR REPLACE FUNCTION notify_superadmin_identity_change() +RETURNS TRIGGER AS $$ +DECLARE + names TEXT[] := '{}'; +BEGIN + -- Every alias the principal can be spelled as: `resolve_username_to_email` matches a `u/` + -- principal against `username` OR `email`, and whichever string the caller passed is the key + -- it cached under, so one account can hold a live entry under either. Old and new of each, + -- because a change to one leaves the other's entry behind. + IF TG_OP <> 'DELETE' THEN names := names || ARRAY[NEW.username, NEW.email]; END IF; + IF TG_OP <> 'INSERT' THEN names := names || ARRAY[OLD.username, OLD.email]; END IF; + INSERT INTO notify_event (channel, payload) + SELECT DISTINCT 'notify_user_email_change', '*:' || n + FROM unnest(names) AS n + WHERE n IS NOT NULL; + RETURN COALESCE(NEW, OLD); +END; +$$ LANGUAGE plpgsql SECURITY DEFINER; + +-- `super_admin` is half of what the fallback matches on, so gaining or losing it moves the +-- mapping as surely as the address does: a demotion leaves the real address cached where the +-- truth is now synthetic, and a promotion leaves that synthetic one cached in place of a real +-- account. `OLD.super_admin OR NEW.super_admin` is what catches both directions. +CREATE TRIGGER password_superadmin_update_trigger +AFTER UPDATE OF email, username, super_admin ON password +FOR EACH ROW +WHEN ((OLD.super_admin OR NEW.super_admin) + AND (OLD.email IS DISTINCT FROM NEW.email + OR OLD.username IS DISTINCT FROM NEW.username + OR OLD.super_admin IS DISTINCT FROM NEW.super_admin)) +EXECUTE FUNCTION notify_superadmin_identity_change(); + +CREATE TRIGGER password_superadmin_insert_trigger +AFTER INSERT ON password +FOR EACH ROW +WHEN (NEW.super_admin) +EXECUTE FUNCTION notify_superadmin_identity_change(); + +CREATE TRIGGER password_superadmin_delete_trigger +AFTER DELETE ON password +FOR EACH ROW +WHEN (OLD.super_admin) +EXECUTE FUNCTION notify_superadmin_identity_change(); diff --git a/backend/src/main.rs b/backend/src/main.rs index 118795bd17..ac1d286d6e 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -1913,6 +1913,17 @@ async fn process_notify_event( ); windmill_api::auth::invalidate_token_from_cache(payload); } + "notify_user_email_change" => { + // `:`, or `*:` from a `password` change, which + // knows the name but no workspace. Workspace ids can't contain ':'. + if let Some(username) = payload.strip_prefix("*:") { + tracing::info!("Superadmin identity change detected, invalidating: {username}"); + windmill_common::users::invalidate_email_cache_for_username(username); + } else if let Some((workspace_id, username)) = payload.split_once(':') { + tracing::info!("User email change detected, invalidating cache: {payload}"); + windmill_common::users::invalidate_email_cache(workspace_id, username); + } + } "notify_app_policy_change" => { // payload is `:`; workspace ids can't contain ':'. if server_mode { diff --git a/backend/tests/folder_default_permissioned_as.rs b/backend/tests/folder_default_permissioned_as.rs index 7aa4f0a003..238d7cdb32 100644 --- a/backend/tests/folder_default_permissioned_as.rs +++ b/backend/tests/folder_default_permissioned_as.rs @@ -630,7 +630,7 @@ async fn test_folder_default_permissioned_as(db: Pool) -> anyhow::Resu ); assert_eq!( policy["on_behalf_of_email"], "group-wm_deployers@windmill.dev", - "app policy.on_behalf_of_email gets folder default email" + "the stored address is derived from that principal" ); // 7b. Admin, non-matching path — acting user diff --git a/backend/tests/preserve_on_behalf_of.rs b/backend/tests/preserve_on_behalf_of.rs index 2a1f3895ca..0c8c0b76a3 100644 --- a/backend/tests/preserve_on_behalf_of.rs +++ b/backend/tests/preserve_on_behalf_of.rs @@ -405,11 +405,12 @@ 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", Some("u/original-user"), - Some("original@windmill.dev"), + None, true, )) .send() @@ -434,10 +435,24 @@ 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"), - "Admin should preserve app on_behalf_of_email" + "the stored address is derived from the principal, not the one the client sent" + ); + let resp = authed( + client().get(format!("{base}/apps/get/p/u/test-user/app_admin_preserve")), + "SECRET_TOKEN", + ) + .send() + .await?; + let returned: serde_json::Value = resp.json().await?; + assert_eq!( + returned["policy"]["on_behalf_of_email"].as_str(), + Some("original@windmill.dev"), + "the response returns the address written through from the principal" ); // ======================================== @@ -476,11 +491,6 @@ async fn test_preserve_on_behalf_of(db: Pool) -> anyhow::Result<()> { Some("u/original-user"), "Deployer should preserve app on_behalf_of" ); - assert_eq!( - policy.get("on_behalf_of_email").and_then(|v| v.as_str()), - Some("original@windmill.dev"), - "Deployer should preserve app on_behalf_of_email" - ); // ======================================== // 9. App: Non-admin cannot preserve @@ -518,10 +528,26 @@ async fn test_preserve_on_behalf_of(db: Pool) -> anyhow::Result<()> { Some("u/test-user-2"), "Non-admin should have their own permissioned_as as app on_behalf_of" ); + + // ======================================== + // 9b. App: a policy naming two different accounts is rejected + // ======================================== + + // This is the shape a workspace deploy produces when it carries the source + // workspace's principal beside the target's address. + let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN") + .json(&new_app_with_on_behalf_of( + "u/test-user/app_mismatched_pair", + Some("u/original-user"), + Some("test2@windmill.dev"), + true, + )) + .send() + .await?; assert_eq!( - policy.get("on_behalf_of_email").and_then(|v| v.as_str()), - Some("test2@windmill.dev"), - "Non-admin should have their own email as app on_behalf_of_email" + resp.status(), + 400, + "a policy whose two halves name different accounts must be rejected" ); // ======================================== @@ -1238,11 +1264,6 @@ async fn test_app_update_preserves_on_behalf_of(db: Pool) -> anyhow::R Some("u/original-user"), "Admin update should preserve app on_behalf_of" ); - assert_eq!( - policy.get("on_behalf_of_email").and_then(|v| v.as_str()), - Some("original@windmill.dev"), - "Admin update should preserve app on_behalf_of_email" - ); // ======================================== // Deployer updates with preserve flag @@ -1304,11 +1325,6 @@ async fn test_app_update_preserves_on_behalf_of(db: Pool) -> anyhow::R Some("u/original-user"), "Deployer update should preserve app on_behalf_of" ); - assert_eq!( - policy.get("on_behalf_of_email").and_then(|v| v.as_str()), - Some("original@windmill.dev"), - "Deployer update should preserve app on_behalf_of_email" - ); // ======================================== // Non-admin cannot preserve on update @@ -1370,10 +1386,62 @@ async fn test_app_update_preserves_on_behalf_of(db: Pool) -> anyhow::R Some("u/test-user-2"), "Non-admin update should overwrite app on_behalf_of with their own" ); + + Ok(()) +} + +/// A superadmin acting outside their workspaces has no `usr` row, so the per-workspace rename +/// sweep never reaches the apps that name them. Their principal is their instance username, so +/// without a global sweep a rename leaves those apps naming an account that resolves to nobody. +#[sqlx::test(fixtures("preserve_on_behalf_of"))] +async fn test_rename_sweeps_external_superadmin_app_identity( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + let path = "u/original-user/app_run_by_external_superadmin"; + + let resp = authed(client().post(format!("{base}/apps/create")), "SECRET_TOKEN") + .json(&new_app_with_on_behalf_of( + path, + Some("u/superadmin-external"), + Some("superadmin-external@windmill.dev"), + true, + )) + .send() + .await?; assert_eq!( - policy.get("on_behalf_of_email").and_then(|v| v.as_str()), - Some("test2@windmill.dev"), - "Non-admin update should overwrite app on_behalf_of_email with their own" + resp.status(), + 201, + "Should create app: {}", + resp.text().await? + ); + + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/users/rename/superadmin-external@windmill.dev" + )), + "SECRET_TOKEN", + ) + .json(&json!({ "new_username": "superadmin_renamed" })) + .send() + .await?; + assert_eq!(resp.status(), 200, "Should rename: {}", resp.text().await?); + + let app = sqlx::query!( + "SELECT policy FROM app WHERE path = $1 AND workspace_id = $2", + path, + "test-workspace" + ) + .fetch_one(&db) + .await?; + assert_eq!( + app.policy.get("on_behalf_of").and_then(|v| v.as_str()), + Some("u/superadmin_renamed"), + "the rename should follow the principal an app names" ); Ok(()) @@ -2855,10 +2923,8 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool) -> anyho resp.text().await? ); - // 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, so even a - // superadmin email pinned onto an unrelated principal deploys fine here. + // 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", @@ -2873,12 +2939,14 @@ async fn test_reject_reserved_sentinel_on_behalf_of(db: Pool) -> anyho .await?; assert_eq!( resp.status(), - 201, - "a real superadmin on_behalf_of is allowed at deploy (capped at execution): {}", + 400, + "a superadmin address beside an unrelated principal must be refused: {}", resp.text().await? ); - // App: a consistently named real superadmin identity is likewise allowed. + // 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. let resp = authed( client().post(format!("{base}/apps/create")), "DEPLOYER_TOKEN", diff --git a/backend/tests/worker.rs b/backend/tests/worker.rs index 22f95aefb8..3e2bae5092 100644 --- a/backend/tests/worker.rs +++ b/backend/tests/worker.rs @@ -5483,7 +5483,7 @@ async fn test_flow_substep_tag_availability_check(db: Pool) -> anyhow: let result = RunJob::from(JobPayload::RawFlow { value: flow.clone(), path: None, restarted_from: None }) - .email("test2@windmill.dev") + .as_user("test-user-2", "test2@windmill.dev") .run_until_complete(&db, false, server.addr.port()) .await; diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index b9edde3c6d..04513b4c05 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -1053,6 +1053,10 @@ pub async fn fetch_api_authed_from_permissioned_as( db: &DB, 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. let key = (w_id.to_string(), permissioned_as.clone(), email.clone()); let mut api_authed = match API_AUTHED_CACHE.get(&key) { @@ -1068,7 +1072,10 @@ pub async fn fetch_api_authed_from_permissioned_as( let api_authed = ApiAuthed { username: authed.username, - email, + // The resolved one, not the address we were handed: that is the point of + // `fetch_authed_from_permissioned_as` validating it against the principal's live + // binding, and this value goes on to the job row, `job_perms` and the JWT. + email: authed.email, is_admin: authed.is_admin, is_operator: authed.is_operator, groups: authed.groups, diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index ecb7ba405b..d985078e71 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -922,7 +922,7 @@ async fn derived_on_behalf_of_email( let Some(permissioned_as) = flow.on_behalf_of.as_deref() else { return Ok(None); }; - // Uncached, for the reason given on `prefetch_cached_script`: this pair is round-tripped. + // Uncached: this pair is round-tripped by the client and stored again on redeploy. Ok(Some( windmill_common::users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db) .await?, diff --git a/backend/windmill-api-integration-tests/tests/offboarding.rs b/backend/windmill-api-integration-tests/tests/offboarding.rs index 4434e6a2b5..7bdeab924b 100644 --- a/backend/windmill-api-integration-tests/tests/offboarding.rs +++ b/backend/windmill-api-integration-tests/tests/offboarding.rs @@ -599,3 +599,59 @@ async fn test_offboard_invalid_target(db: Pool) -> anyhow::Result<()> Ok(()) } + +/// A legacy member named `group-ops` canonicalizes to `g/ops`, the principal the real `ops` group +/// runs as. Offboarding the member must not hand the group's runnables to the replacement. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_offboard_group_prefixed_member_keeps_group_identities( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + sqlx::raw_sql( + "INSERT INTO password(email, password_hash, login_type, super_admin, verified, name, username) + VALUES ('ops-bot@windmill.dev', 'x', 'password', false, true, 'Ops bot', 'group-ops'); + INSERT INTO usr(workspace_id, email, username, is_admin, role) + VALUES ('test-workspace', 'ops-bot@windmill.dev', 'group-ops', false, 'User'); + 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\"}', '{}', '{}');", + ) + .execute(&db) + .await?; + + let preview: serde_json::Value = + authed(client().get(ws_url(port, "offboard_preview/group-ops"))) + .send() + .await? + .json() + .await?; + assert!( + preview["executing_on_behalf"]["apps"].is_null(), + "the group's apps are not the member's to reassign: {preview}" + ); + + let resp = authed(client().post(ws_url(port, "offboard/group-ops"))) + .json(&json!({ + "reassign_to": "u/test-user", + "new_on_behalf_of_user": "test-user", + "delete_user": false + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + let principal: Option = sqlx::query_scalar( + "SELECT policy->>'on_behalf_of' FROM app + WHERE workspace_id = 'test-workspace' AND path = 'f/shared/ops_app'", + ) + .fetch_one(&db) + .await?; + assert_eq!(principal.as_deref(), Some("g/ops")); + + Ok(()) +} diff --git a/backend/windmill-api-schedule/src/lib.rs b/backend/windmill-api-schedule/src/lib.rs index 1b6b49a136..d581608c10 100644 --- a/backend/windmill-api-schedule/src/lib.rs +++ b/backend/windmill-api-schedule/src/lib.rs @@ -332,7 +332,7 @@ async fn create_schedule( ) .await?; // email is still written for backwards compat with old workers that don't know about permissioned_as - let resolved_email = windmill_common::users::get_email_from_permissioned_as( + let resolved_email = windmill_common::users::get_email_from_permissioned_as_uncached( &resolved_permissioned_as, &w_id, &db, @@ -545,18 +545,14 @@ async fn edit_schedule( reject_reserved_schedule_path(path)?; let authed = maybe_refresh_folders(&path, &w_id, authed, &db).await; - let mut tx = user_db.begin(&authed).await?; // Check schedule for error ScheduleType::from_str(&es.schedule, es.cron_version.as_deref(), true)?; - // Validate dynamic_skip if provided - if let Some(handler_path) = &es.dynamic_skip { - validate_dynamic_skip(&mut tx, &w_id, handler_path).await?; - } - let resolved_edited_by = resolve_edited_by(&authed); + // Resolved on the (non-RLS) pool before the RLS transaction opens: the lookup mid-transaction + // would hold a second connection while `tx` is checked out. let resolved_permissioned_as = resolve_permissioned_as( es.permissioned_as.as_ref(), es.preserve_permissioned_as, @@ -568,7 +564,7 @@ async fn edit_schedule( let resolved_email = if resolved_permissioned_as != windmill_common::users::username_to_permissioned_as(&authed.username) { - windmill_common::users::get_email_from_permissioned_as( + windmill_common::users::get_email_from_permissioned_as_uncached( &resolved_permissioned_as, &w_id, &db, @@ -585,6 +581,13 @@ async fn edit_schedule( Some(&resolved_email), )?; + let mut tx = user_db.begin(&authed).await?; + + // Validate dynamic_skip if provided + if let Some(handler_path) = &es.dynamic_skip { + validate_dynamic_skip(&mut tx, &w_id, handler_path).await?; + } + let before = trigger_history::snapshot_row(&mut *tx, "schedule", &w_id, path).await?; let schedule = sqlx::query_as!( diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index afe7580055..260df2f969 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2149,6 +2149,28 @@ async fn change_user_email( .execute(&mut *tx) .await?; + // An app draft carries a copy of the deployed policy, principal included. + sqlx::query!( + r#"UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb($1::text))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of' = $2"#, + &new_principal, + &old_principal + ) + .execute(&mut *tx) + .await?; + + // A raw-app draft persists the address the client read back too. The deploy sends it beside + // the principal, where an address naming somebody else is rejected — and unlike a live read + // it never refreshes on its own. Same group guard as the deployed policy above, plus the + // `IS NULL` arm: without it the predicate is `NULL` for a draft with no principal, which is + // neither true nor false, so those rows would be skipped. + sqlx::query!( + r#"UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of_email'], to_jsonb($1::text))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of_email' = $2 AND (value->'policy'->>'on_behalf_of' IS NULL OR value->'policy'->>'on_behalf_of' NOT LIKE 'g/%')"#, + &new_email, + &old_email + ) + .execute(&mut *tx) + .await?; + // A folder's default rules are an ordered array, first match wins, so the rewrite has to // preserve their order. A rule left on the old address makes `ensure_permissioned_as_exists` // reject the creation of every runnable the rule matches. @@ -2326,9 +2348,9 @@ async fn change_user_email( ) .await?; - // Read back inside the transaction: the address is derived at dispatch through a cache - // that nothing else evicts, so without this a job pushed in the next 60s would resolve - // the old address and with it the wrong superadmin flag and instance groups. + // Read back inside the transaction so this process can evict its own keys immediately. + // `notify_user_email_change` reaches every replica for the same change, but asynchronously, + // and this one is the replica that just served the request. let memberships = sqlx::query_scalar!("SELECT workspace_id FROM usr WHERE email = $1", &new_email) .fetch_all(&mut *tx) diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index f1c67efd26..0fd6688571 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -33946,8 +33946,10 @@ components: checked when the session is minted and again on every guest request on_behalf_of: type: string + 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. sandbox: type: boolean description: > diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 25178ebd80..daa7fee59b 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -491,6 +491,13 @@ 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")] pub on_behalf_of_email: Option, //paths: // - script/ @@ -2461,31 +2468,37 @@ async fn create_app_internal<'a>( } // Resolve the on-behalf-of defaults on the (non-RLS) pool *before* opening // the RLS transaction below: doing these lookups mid-transaction would hold - // a second simultaneous connection while `tx` is still checked out. + // a second simultaneous connection while `tx` is still checked out. The race this + // leaves with a concurrent rename or removal, including a freed username later + // 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.is_some() || app.policy.on_behalf_of_email.is_some()); - if !should_preserve { + 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(), + app.policy.on_behalf_of.as_deref(), + true, + &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) .await? } else { None }; - if let Some(default_permissioned_as) = folder_default { - let default_email = windmill_common::users::get_email_from_permissioned_as( - &default_permissioned_as, - w_id, - &db, - ) - .await?; - app.policy.on_behalf_of = Some(default_permissioned_as); - app.policy.on_behalf_of_email = Some(default_email); - } else { - app.policy.on_behalf_of = Some(username_to_permissioned_as(&authed.username)); - app.policy.on_behalf_of_email = Some(authed.email.clone()); - } + 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); } // Reject a forged superadmin run identity in the (possibly preserved) policy. @@ -2621,21 +2634,17 @@ async fn create_app_internal<'a>( None, ) .await?; - if should_preserve { - if let Some(ref obo_email) = app.policy.on_behalf_of_email { - if obo_email != &authed.email { - audit_log( - &mut *tx, - &authed, - "apps.on_behalf_of", - ActionKind::Create, - w_id, - Some(&app.path), - Some([("on_behalf_of", obo_email.as_str()), ("action", "create")].into()), - ) - .await?; - } - } + if let Some(ref obo_email) = preserved_on_behalf_of { + audit_log( + &mut *tx, + &authed, + "apps.on_behalf_of", + ActionKind::Create, + w_id, + Some(&app.path), + Some([("on_behalf_of", obo_email.as_str()), ("action", "create")].into()), + ) + .await?; } let mut args: HashMap> = HashMap::new(); if let Some(dm) = &app.deployment_message { @@ -3392,19 +3401,33 @@ async fn update_app_internal<'a>( } } - // Reject a forged superadmin run identity in a preserved policy. Mirror the - // `should_preserve` gate below (only a preserved value is caller-controlled; - // otherwise the policy is rewritten to the deployer's own identity) and run - // it on the non-RLS pool before the transaction to avoid a second connection. - if let Some(npolicy) = ns.policy.as_ref() { + // Resolved on the (non-RLS) pool before the RLS transaction opens, for the reason + // `create_app` states, with the same known, accepted rename race (see + // `resolve_on_behalf_of`). Submitting a policy is how a deployer claims the app's execution + // identity; a source deploy that sent none claims nothing, so whoever the app already runs as + // stays. + let mut preserved_on_behalf_of: Option = None; + 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.is_some() || npolicy.on_behalf_of_email.is_some()); + if should_preserve { - windmill_common::auth::validate_on_behalf_of( - npolicy.on_behalf_of.as_deref(), + npolicy.on_behalf_of = windmill_common::resolve_on_behalf_of( npolicy.on_behalf_of_email.as_deref(), - )?; + npolicy.on_behalf_of.as_deref(), + true, + &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); } } @@ -3437,7 +3460,6 @@ async fn update_app_internal<'a>( reject_kind_change(path, raw_app, deployed_raw_app)?; } - let mut preserved_on_behalf_of: Option = None; let npath = if ns.policy.is_some() || ns.path.is_some() || ns.summary.is_some() @@ -3623,23 +3645,6 @@ async fn update_app_internal<'a>( } } } - 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(); - - if should_preserve { - if let Some(ref obo_email) = npolicy.on_behalf_of_email { - if obo_email != &authed.email { - preserved_on_behalf_of = Some(obo_email.clone()); - } - } - } else if caller_sent_policy { - // Submitting a policy is how a deployer claims the app's - // execution identity. A source deploy that sent none is not - // claiming anything, so whoever the app already runs as stays. - npolicy.on_behalf_of = Some(username_to_permissioned_as(&authed.username)); - npolicy.on_behalf_of_email = Some(authed.email.clone()); - } sqlb.set( "policy", quote(serde_json::to_string(&json!(npolicy)).map_err(|e| { @@ -3849,6 +3854,8 @@ fn digest(code: &str) -> String { async fn get_on_behalf_details_from_policy_and_authed( policy: &Policy, opt_authed: &Option, + w_id: &str, + db: &DB, ) -> Result<(String, String, String)> { // A guest acts only through an app open to guests — or to everyone. A members-only // mode means the policy changed after the session was issued. Decided here, in the @@ -3871,7 +3878,7 @@ async fn get_on_behalf_details_from_policy_and_authed( .as_ref() .map(|a| a.username.clone()) .unwrap_or_else(|| "anonymous".to_string()); - let (permissioned_as, email) = get_on_behalf_of(&policy)?; + let (permissioned_as, email) = get_on_behalf_of(&policy, w_id, db).await?; (username, permissioned_as, email) } // Guest runs as the publisher exactly as Publisher does; the two differ only @@ -3885,7 +3892,7 @@ async fn get_on_behalf_details_from_policy_and_authed( "publisher execution mode requires authentication".to_string(), ) })?; - let (permissioned_as, email) = get_on_behalf_of(&policy)?; + let (permissioned_as, email) = get_on_behalf_of(&policy, w_id, db).await?; (username, permissioned_as, email) } ExecutionMode::Viewer => { @@ -4243,7 +4250,7 @@ async fn execute_component( } let (username, permissioned_as, email) = - get_on_behalf_details_from_policy_and_authed(&policy, &opt_authed).await?; + get_on_behalf_details_from_policy_and_authed(&policy, &opt_authed, &w_id, &db).await?; let resolved_delete_secs = resolve_delete_after_secs(None, policy_triggerables.delete_after_secs); @@ -4613,7 +4620,7 @@ async fn upload_s3_file_from_app( let s3_inputs = policy.s3_inputs.as_ref().unwrap(); let (username, permissioned_as, email) = - get_on_behalf_details_from_policy_and_authed(&policy, &opt_authed).await?; + get_on_behalf_details_from_policy_and_authed(&policy, &opt_authed, &w_id, &db).await?; let on_behalf_authed = fetch_api_authed_from_permissioned_as( permissioned_as.clone(), @@ -5024,7 +5031,7 @@ async fn get_on_behalf_authed_from_app( let opt_authed = guest_caller_for_mode(opt_authed.clone(), policy.execution_mode(), path)?; let (username, permissioned_as, email) = - get_on_behalf_details_from_policy_and_authed(&policy, &opt_authed).await?; + get_on_behalf_details_from_policy_and_authed(&policy, &opt_authed, &w_id, &db).await?; let on_behalf_authed = fetch_api_authed_from_permissioned_as(permissioned_as, email, &w_id, &db, Some(username)) @@ -5535,7 +5542,45 @@ async fn app_load_csv_preview() -> Result<()> { )) } -fn get_on_behalf_of(policy: &Policy) -> Result<(String, String)> { +/// 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. +/// +/// 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); + }; + Ok(Some( + windmill_common::users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db) + .await?, + )) +} + +/// 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) +} + +/// 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 .as_ref() @@ -5546,16 +5591,15 @@ fn get_on_behalf_of(policy: &Policy) -> Result<(String, String)> { ) })? .to_string(); - let email = policy - .on_behalf_of_email - .as_ref() - .ok_or_else(|| { - Error::BadRequest( - "on_behalf_of_email is missing in the app policy and is required for anonymous execution" - .to_string(), - ) - })? - .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? + } + }; // 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 @@ -5702,7 +5746,25 @@ 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)), - "author" => Some(serde_json::to_value(&policy.on_behalf_of_email)), + // Same rule as `get_on_behalf_of`: the stored address, derived only when absent. + "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( + windmill_common::users::get_email_from_permissioned_as( + permissioned_as, + w_id, + db, + ) + .await?, + ), + (None, None) => None, + }; + Some(serde_json::to_value(&author)) + } _ => { return Err(Error::BadRequest(format!( "context variable {} not allowed", diff --git a/backend/windmill-api/src/offboarding.rs b/backend/windmill-api/src/offboarding.rs index 2998faa7f3..26eb06c8e8 100644 --- a/backend/windmill-api/src/offboarding.rs +++ b/backend/windmill-api/src/offboarding.rs @@ -128,6 +128,17 @@ struct WorkspaceReassignment { // ---- Preview helpers ---- +/// The principal a departing member's runnables run as, or `None` when none of them are theirs +/// to hand over. `usr.username` is constrained to `[\w-]+`, so a member is `u/{username}`, except +/// a legacy `group-*` username, which canonicalizes to the group it names: what runs under that +/// principal runs as the group, which outlives the member. `None` binds NULL, which the +/// `on_behalf_of = $n` queries then match nowhere. +fn departing_principal(username: &str) -> Option { + use windmill_common::users::{username_to_permissioned_as, PERMISSIONED_AS_GROUP_PREFIX}; + let principal = username_to_permissioned_as(username); + (!principal.starts_with(PERMISSIONED_AS_GROUP_PREFIX)).then_some(principal) +} + async fn get_offboard_preview( db: impl sqlx::PgExecutor<'_> + Copy, w_id: &str, @@ -136,10 +147,8 @@ async fn get_offboard_preview( ) -> Result { let user_prefix = format!("u/{}/%", username); let user_owner = format!("u/{}", username); - // Same form the mutation reassigns, so preview and execution cannot disagree. `usr.username` - // is constrained to `[\w-]+`, so a member is always named `u/{username}` — the address form a - // principal can also take names an account with no `usr` row, which is nobody offboardable. - let departing = windmill_common::users::username_to_permissioned_as(username); + // Same form the mutation reassigns, so preview and execution cannot disagree. + let departing = departing_principal(username); // ---- Owned objects (under u/{username}/) ---- let scripts = sqlx::query_scalar!( @@ -244,17 +253,17 @@ async fn get_offboard_preview( // ---- Operator references (not under user's path) ---- let obo_scripts = sqlx::query_scalar!( "SELECT path FROM script WHERE on_behalf_of = $1 AND NOT path LIKE $2 AND workspace_id = $3 AND NOT archived AND NOT deleted", - &departing, &user_prefix, w_id + departing.as_deref(), &user_prefix, w_id ).fetch_all(db).await?; let obo_flows = sqlx::query_scalar!( "SELECT path FROM flow WHERE on_behalf_of = $1 AND NOT path LIKE $2 AND workspace_id = $3 AND NOT archived", - &departing, &user_prefix, w_id + departing.as_deref(), &user_prefix, w_id ).fetch_all(db).await?; let obo_apps = sqlx::query_scalar!( "SELECT path FROM app WHERE policy->>'on_behalf_of' = $1 AND NOT path LIKE $2 AND workspace_id = $3", - &user_owner, &user_prefix, w_id + departing.as_deref(), &user_prefix, w_id ).fetch_all(db).await?; let obo_schedules = sqlx::query_scalar!( @@ -831,7 +840,7 @@ async fn offboard_user_from_workspace<'c>( new_permissioned_as: &str, ) -> Result { let new_prefix = reassign_to.to_string(); - let departing = windmill_common::users::username_to_permissioned_as(username); + let departing = departing_principal(username); // The app policy stores an address beside its principal, and script/flow keep one for the // workers that still read it, so the replacement's is resolved here. @@ -871,7 +880,7 @@ async fn offboard_user_from_workspace<'c>( sqlx::query!( "UPDATE script SET on_behalf_of = $1, on_behalf_of_email = $4 WHERE on_behalf_of = $2 AND workspace_id = $3", new_permissioned_as, - &departing, + departing.as_deref(), w_id, new_on_behalf_of_user_email ) @@ -912,7 +921,7 @@ async fn offboard_user_from_workspace<'c>( sqlx::query!( "UPDATE flow SET on_behalf_of = $1, on_behalf_of_email = $4 WHERE on_behalf_of = $2 AND workspace_id = $3", new_permissioned_as, - &departing, + departing.as_deref(), w_id, new_on_behalf_of_user_email ) @@ -925,7 +934,7 @@ async fn offboard_user_from_workspace<'c>( sqlx::query!( r#"UPDATE draft SET value = to_json(jsonb_set(jsonb_set(to_jsonb(value), ARRAY['on_behalf_of'], to_jsonb($1::text)), ARRAY['on_behalf_of_email'], to_jsonb($4::text))) WHERE typ IN ('script', 'flow') AND value->>'on_behalf_of' = $2 AND workspace_id = $3"#, new_permissioned_as, - &departing, + departing.as_deref(), w_id, new_on_behalf_of_user_email ) @@ -951,9 +960,21 @@ async fn offboard_user_from_workspace<'c>( "UPDATE app SET policy = jsonb_set( jsonb_set(policy, ARRAY['on_behalf_of'], to_jsonb($1::text)), ARRAY['on_behalf_of_email'], to_jsonb($4::text) - ) WHERE policy->>'on_behalf_of' = ('u/' || $2) AND workspace_id = $3", + ) WHERE policy->>'on_behalf_of' = $2 AND workspace_id = $3", &new_permissioned_as, - username, + departing.as_deref(), + w_id, + new_on_behalf_of_user_email + ) + .execute(&mut **tx) + .await?; + + // An app draft carries a copy of the deployed policy and is deployed from it, so it needs + // the same pair rewritten — the draft sweep above only covers scripts and flows. + sqlx::query!( + r#"UPDATE draft SET value = to_json(jsonb_set(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb($1::text)), ARRAY['policy', 'on_behalf_of_email'], to_jsonb($4::text))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of' = $2 AND workspace_id = $3"#, + new_permissioned_as, + departing.as_deref(), w_id, new_on_behalf_of_user_email ) diff --git a/backend/windmill-api/src/users.rs b/backend/windmill-api/src/users.rs index ed66eb5e74..4dd073b3af 100644 --- a/backend/windmill-api/src/users.rs +++ b/backend/windmill-api/src/users.rs @@ -261,6 +261,12 @@ async fn rename_user( ))); } + let old_instance_username = + sqlx::query_scalar!("SELECT username FROM password WHERE email = $1", user_email) + .fetch_optional(&mut *tx) + .await? + .flatten(); + sqlx::query!( "UPDATE password SET username = $1 WHERE email = $2", ru.new_username, @@ -269,6 +275,36 @@ async fn rename_user( .execute(&mut *tx) .await?; + // The per-workspace sweep below only reaches accounts with a `usr` row. A superadmin acting + // outside their workspaces has none, yet an app can name them: their principal is + // `u/{password.username}`, which this rename just moved. Matching on the address as well + // keeps a like-named member of some other workspace out of it. + if let Some(old_username) = old_instance_username.filter(|u| *u != ru.new_username) { + let old_principal = windmill_common::users::username_to_permissioned_as(&old_username); + let new_principal = + windmill_common::users::username_to_permissioned_as(&ru.new_username); + sqlx::query!( + "UPDATE app SET policy = jsonb_set(policy, ARRAY['on_behalf_of'], to_jsonb($1::text)) + WHERE policy->>'on_behalf_of' = $2 AND policy->>'on_behalf_of_email' = $3", + &new_principal, + &old_principal, + user_email + ) + .execute(&mut *tx) + .await?; + sqlx::query!( + r#"UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb($1::text))) + WHERE typ IN ('app', 'raw_app') + AND value->'policy'->>'on_behalf_of' = $2 + AND value->'policy'->>'on_behalf_of_email' = $3"#, + &new_principal, + &old_principal, + user_email + ) + .execute(&mut *tx) + .await?; + } + let workspace_usernames = sqlx::query!( "SELECT workspace_id, username FROM usr WHERE email = $1", &user_email @@ -754,6 +790,17 @@ async fn update_username_in_workpsace<'c>( .execute(&mut **tx) .await?; + // An app draft carries a copy of the deployed policy, so the rename must reach it + // there too — same reason as the script/flow draft sweep above. + sqlx::query!( + r#"UPDATE draft SET value = to_json(jsonb_set(to_jsonb(value), ARRAY['policy', 'on_behalf_of'], to_jsonb('u/' || $1))) WHERE typ IN ('app', 'raw_app') AND value->'policy'->>'on_behalf_of' = ('u/' || $2) AND workspace_id = $3"#, + new_username, + old_username, + w_id + ) + .execute(&mut **tx) + .await?; + sqlx::query!( "UPDATE app SET extra_perms = extra_perms - ('u/' || $2) || jsonb_build_object(('u/' || $1), extra_perms->('u/' || $2)) WHERE extra_perms ? ('u/' || $2) AND workspace_id = $3", new_username, diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index e0128e9e7a..828509519e 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -149,9 +149,7 @@ async fn derive_email( if let Some(hit) = cache.get(permissioned_as) { return Ok(Some(hit.clone())); } - // Uncached: the address goes into an archive a client redeploys from, and the write path - // validates the pair it sends back against an uncached lookup. The memo above still holds - // this to one query per distinct principal per export. + // The memo above holds this to one query per distinct principal per export. let email = windmill_common::users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db) .await?; diff --git a/backend/windmill-common/src/auth.rs b/backend/windmill-common/src/auth.rs index b51186f464..73cdfba35d 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -452,6 +452,42 @@ async fn fetch_authed_from_permissioned_as_inner( w_id: &str, conn: &mut sqlx::PgConnection, ) -> Result { + // The `usr` row is the live binding between a `u/` principal and an address, and it is read + // here anyway for the workspace role. Callers may hand us a cached address, so read it before + // anything is granted: `super_admin` and `email_to_igroup` below are keyed on the address + // while the role is keyed on the principal, and an address that no longer belongs to this + // principal — a username freed and reassigned while its previous holder keeps a privileged + // account — would mix one account's role with another's instance privileges. + let member = match permissioned_as.split_once('/') { + Some(("u", name)) => sqlx::query!( + "SELECT is_admin, operator, email FROM usr where username = $1 AND \ + workspace_id = $2 AND disabled = false", + name, + &w_id + ) + .fetch_optional(&mut *conn) + .await?, + _ => None, + }; + let resolved_email; + let email = match member.as_ref() { + Some(m) => m.email.as_str(), + // No enabled `usr` row. Resolve as `resolve_username_to_email` does: a disabled member's + // own row still wins over the `password` superadmin fallback, so it can never resolve to + // an unrelated superadmin who shares the username (workspace usernames are only unique per + // workspace). Off the member path, which is why it is worth a query that path skips. + None => match permissioned_as.split_once('/') { + Some(("u", name)) => { + resolved_email = + crate::users::resolve_username_to_email(w_id, name, &mut *conn).await?; + // No live binding at all: the supplied address stands. A cached one is at most one + // notify poll stale; accepted, see `users::get_email_from_permissioned_as`. + resolved_email.as_deref().unwrap_or(email) + } + _ => email, + }, + }; + let is_super_admin = permissioned_as == SUPERADMIN_SYNC_EMAIL || email == SUPERADMIN_SECRET_EMAIL || email == SUPERADMIN_NOTIFICATION_EMAIL @@ -465,22 +501,12 @@ async fn fetch_authed_from_permissioned_as_inner( if prefix == "u" { let (is_admin, is_operator) = if is_super_admin { (true, false) + } else if let Some(m) = member.as_ref() { + (m.is_admin, m.operator) } else { - let r = sqlx::query!( - "SELECT is_admin, operator FROM usr where username = $1 AND \ - workspace_id = $2 AND disabled = false", - name, - &w_id - ) - .fetch_optional(&mut *conn) - .await?; - if let Some(r) = r { - (r.is_admin, r.operator) - } else { - return Err(Error::NotFound(format!( - "user {name} not found in workspace {w_id}" - ))); - } + return Err(Error::NotFound(format!( + "user {name} not found in workspace {w_id}" + ))); }; let groups = get_groups_for_user(w_id, &name, email, &mut *conn).await?; diff --git a/backend/windmill-common/src/folders.rs b/backend/windmill-common/src/folders.rs index 2a6ba935b6..fa4dfdf3b7 100644 --- a/backend/windmill-common/src/folders.rs +++ b/backend/windmill-common/src/folders.rs @@ -78,8 +78,6 @@ pub async fn resolve_folder_default_on_behalf_of( else { return Ok(None); }; - // Uncached: this pair is written straight onto the runnable, where a stale address would - // contradict the principal it is stored beside. let email = crate::users::get_email_from_permissioned_as_uncached(&permissioned_as, w_id, db).await?; Ok(Some((email, permissioned_as))) diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 1639b4605d..dce40048f5 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -282,14 +282,16 @@ pub fn check_on_behalf_of_preservation( None } -/// Resolves the identity to store when creating/updating a flow or script. +/// Resolves the identity to store when creating/updating a flow, script or app. /// -/// The permissioned_as is the only stored identity — it decides what the job may access, -/// and the address is derived from it at read time — so the two can never name different -/// accounts. 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. +/// 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. /// /// 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. @@ -297,6 +299,18 @@ pub fn check_on_behalf_of_preservation( /// Resolves through the non-RLS pool and authorizes nothing itself — `authed` decides only /// whether preservation is allowed, and its role flags are not re-checked against `w_id`. /// Callers must already be authorized for the workspace they pass. +/// +/// 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 +/// 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 +/// (a `usr` row lock in each write, with each sweep ordered after the account change), which no +/// single caller can do on its own; it is left open deliberately. pub async fn resolve_on_behalf_of( on_behalf_of_email: Option<&str>, on_behalf_of: Option<&str>, @@ -1869,11 +1883,9 @@ pub async fn on_behalf_of_from_permissioned_as( let Some(permissioned_as) = permissioned_as else { return Ok(None); }; - // Uncached: the address is copied onto the job row, where it stays for the life of the run - // and decides the superadmin flag and the instance groups. Nothing evicts the cache across - // processes, so a cached read would keep minting jobs under an address the account no longer - // holds for up to a minute after it moves. - let email = users::get_email_from_permissioned_as_uncached(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 = users::get_email_from_permissioned_as(permissioned_as, w_id, db).await?; Ok(Some(jobs::OnBehalfOf { email, permissioned_as: permissioned_as.to_string(), diff --git a/backend/windmill-common/src/scripts.rs b/backend/windmill-common/src/scripts.rs index 00aae89b43..628ae2a709 100644 --- a/backend/windmill-common/src/scripts.rs +++ b/backend/windmill-common/src/scripts.rs @@ -91,9 +91,6 @@ async fn prefetch_cached_script_inner( derive_email: bool, ) -> crate::error::Result> { let derived_email = match script.on_behalf_of.as_deref().filter(|_| derive_email) { - // Uncached: the client preserves this pair and sends it back, where the write path - // validates it against an uncached lookup. A cached address would pair a live principal - // with an address the account no longer holds, and the redeploy would be rejected. Some(permissioned_as) => Some( crate::users::get_email_from_permissioned_as_uncached( permissioned_as, diff --git a/backend/windmill-common/src/users.rs b/backend/windmill-common/src/users.rs index 3d00c5d312..1a49605e59 100644 --- a/backend/windmill-common/src/users.rs +++ b/backend/windmill-common/src/users.rs @@ -175,13 +175,19 @@ pub async fn permissioned_as_exists( /// Drop a cached address so a transactional email change is visible immediately. /// -/// The address is derived at dispatch and feeds the instance-superadmin check and -/// `email_to_igroup`, so serving a stale one would run jobs with the wrong authorization -/// for up to the cache TTL. +/// Not the thing that keeps authorization correct — `fetch_authed_from_permissioned_as` +/// re-resolves the address before granting anything. This keeps the cache from serving an +/// address that is merely wrong for the TTL, on reads and on what is shown. pub fn invalidate_email_cache(workspace_id: &str, username: &str) { EMAIL_CACHE.remove(&(workspace_id.to_string(), username.to_string())); } +/// Drop this name's entry in every workspace, for the changes that know the name but not the +/// workspace: a superadmin resolves through `password`, whose row names no workspace of its own. +pub fn invalidate_email_cache_for_username(username: &str) { + EMAIL_CACHE.retain(|(_workspace_id, cached_username), _| cached_username != username); +} + /// Inverse of [`get_email_from_permissioned_as`]: the principal an on-behalf-of email /// names in this workspace, for callers that supply the email alone. /// @@ -194,6 +200,14 @@ pub fn invalidate_email_cache(workspace_id: &str, username: &str) { /// not a superadmin's, or a group that no longer exists. Callers then leave the identity /// unrecorded rather than storing a principal that cannot authenticate. /// +/// 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 +/// 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. +/// /// Reads through the non-RLS pool and authorizes nothing: callers must already be authorized /// for `workspace_id`. pub async fn permissioned_as_from_email( @@ -242,6 +256,33 @@ pub async fn permissioned_as_from_email( /// - "u/{username}" → resolve via [`resolve_username_to_email`] (cached) /// - "g/{group}" → "group-{group}@windmill.dev" /// - raw email → return as-is +/// +/// `notify_user_email_change` evicts the key on every process for each change that can move it, +/// at that process's next notify-event poll (`LISTEN_NEW_EVENTS_INTERVAL_SEC`, 10s by default), +/// so a hit can still be the old address for up to one poll. The TTL caps it if an eviction is +/// 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 +/// 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. +/// +/// 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 +/// corrected address is what the job row and its token carry. Route an address into an `Authed`, +/// a job row or a token without going through that function, and this cache stops being safe to +/// read at dispatch. +/// +/// What reads the dispatch address before that re-resolution (the quota and superadmin-exemption +/// checks at the top of `push_inner`, a flow step's tag check) or when the principal has no live +/// binding can act on the old address for up to one poll after a username reuse, an email change +/// or a superadmin change. That window is accepted as the cost of keeping dispatch off the +/// database; a consumer that cannot tolerate it must re-resolve first. +/// +/// Reads through the non-RLS pool and authorizes nothing — callers must already be authorized +/// for `workspace_id`. pub async fn get_email_from_permissioned_as<'c>( permissioned_as: &str, workspace_id: &str, @@ -250,13 +291,21 @@ pub async fn get_email_from_permissioned_as<'c>( get_email_from_permissioned_as_inner(permissioned_as, workspace_id, db, true).await } -/// [`get_email_from_permissioned_as`] without the address cache. Nothing evicts that cache -/// across processes, so for a minute after an email change it still serves the old address — -/// fine where the address only labels something on screen, wrong where it decides whether a -/// write is accepted or is copied onto a job row that outlives the window. +/// [`get_email_from_permissioned_as`] for a value about to be **persisted**. /// -/// Reads through the non-RLS pool and authorizes nothing, like the cached one: callers must -/// already be authorized for `workspace_id`. +/// The eviction is delivered by the `notify_event` poller, not synchronously, so for a few +/// seconds after a change a replica can still serve the old address. In a config row that is +/// permanent: the row outlives the eviction, every later run trusts it, and nothing re-derives +/// it, so a principal and an address that name different accounts stay that way. +/// +/// Use this for three cases, all of which end in a stored pair: +/// - writing the address into a row; +/// - the lookup that validates a pair before it is stored; +/// - **reads whose result the client sends back** — a script or a workspace export hands over a +/// principal and address together, and a redeploy validates that pair against a fresh +/// resolution, so a stale one comes back as a rejected deploy rather than a stale display. +/// +/// See [`get_email_from_permissioned_as`] for the dispatch case that deliberately does not. pub async fn get_email_from_permissioned_as_uncached<'c>( permissioned_as: &str, workspace_id: &str, diff --git a/backend/windmill-common/tests/notify_events.rs b/backend/windmill-common/tests/notify_events.rs index 8161130875..285086308e 100644 --- a/backend/windmill-common/tests/notify_events.rs +++ b/backend/windmill-common/tests/notify_events.rs @@ -360,6 +360,77 @@ async fn test_trigger_notify_workspace_key_change(db: Pool) { ); } +/// The address a job runs as is served from a process-local cache, so every change that can move +/// a `(workspace, username)` -> email mapping has to reach the other replicas as an eviction. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_trigger_notify_user_email_change(db: Pool) { + let before_id = get_latest_event_id(&db).await.unwrap(); + + sqlx::query("UPDATE usr SET email = 'renamed@windmill.dev' WHERE workspace_id = 'test-workspace' AND username = 'test-user'") + .execute(&db) + .await + .expect("Failed to change email"); + + let events = poll_notify_events(&db, before_id) + .await + .expect("Should poll events"); + assert!( + events.iter().any(|e| e.channel == "notify_user_email_change" + && e.payload == "test-workspace:test-user"), + "email change should evict the key it moved" + ); + + // A superadmin outside their workspaces resolves through `password`, which names no + // workspace: the wildcard is the only way to reach that key. `super_admin` is half of what + // that fallback matches on, so losing it moves the mapping just as the address does. + for (label, stmt, expected_aliases) in [ + ( + "email change", + "UPDATE password SET email = 'sa2@windmill.dev' WHERE email = 'test@windmill.dev'", + // old address, new address, and the username that outlives both + vec!["test@windmill.dev", "sa2@windmill.dev", "test-user"], + ), + ( + "demotion", + "UPDATE password SET super_admin = false WHERE email = 'sa2@windmill.dev'", + vec!["sa2@windmill.dev", "test-user"], + ), + ( + "promotion", + "UPDATE password SET super_admin = true WHERE email = 'sa2@windmill.dev'", + vec!["sa2@windmill.dev", "test-user"], + ), + ( + "deletion", + "DELETE FROM password WHERE email = 'sa2@windmill.dev'", + vec!["sa2@windmill.dev", "test-user"], + ), + ] { + let before_id = get_latest_event_id(&db).await.unwrap(); + sqlx::query(stmt) + .execute(&db) + .await + .unwrap_or_else(|e| panic!("Failed to apply superadmin {label}: {e}")); + + let events = poll_notify_events(&db, before_id) + .await + .expect("Should poll events"); + // Every alias the principal can be spelled as, since `resolve_username_to_email` + // matches a `u/` principal against the username or the address. + let evicted: Vec<&str> = events + .iter() + .filter(|e| e.channel == "notify_user_email_change") + .filter_map(|e| e.payload.strip_prefix("*:")) + .collect(); + for alias in expected_aliases { + assert!( + evicted.contains(&alias), + "superadmin {label} should evict {alias}, got {evicted:?}" + ); + } + } +} + #[sqlx::test(migrations = "../migrations", fixtures("base"))] async fn test_trigger_notify_token_invalidation(db: Pool) { // First insert a session token with token_hash and token_prefix diff --git a/backend/windmill-common/tests/permissioned_as_authz.rs b/backend/windmill-common/tests/permissioned_as_authz.rs new file mode 100644 index 0000000000..25c219845f --- /dev/null +++ b/backend/windmill-common/tests/permissioned_as_authz.rs @@ -0,0 +1,81 @@ +use sqlx::{Pool, Postgres}; +use windmill_common::auth::fetch_authed_from_permissioned_as; + +/// The address handed to `fetch_authed_from_permissioned_as` may come from a cache that a +/// username reassignment has outrun. It must not be believed: the workspace role is keyed on the +/// principal while `super_admin` and `email_to_igroup` are keyed on the address, so trusting a +/// stale one would run the new holder's job with the previous holder's instance privileges. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_stale_address_cannot_carry_the_previous_holders_privileges(db: Pool) { + // `test-user` in the fixture is a superadmin with the address `test@windmill.dev`. Free the + // username and hand it to somebody who is not, exactly as an offboard-then-onboard would. + sqlx::query("DELETE FROM usr WHERE workspace_id = 'test-workspace' AND username = 'test-user'") + .execute(&db) + .await + .expect("free the username"); + sqlx::query( + "INSERT INTO password(email, password_hash, login_type, super_admin, verified, name) + VALUES ('newcomer@windmill.dev', 'x', 'password', false, true, 'Newcomer')", + ) + .execute(&db) + .await + .expect("create the new account"); + sqlx::query( + "INSERT INTO usr(workspace_id, email, username, is_admin, role) + VALUES ('test-workspace', 'newcomer@windmill.dev', 'test-user', false, 'User')", + ) + .execute(&db) + .await + .expect("reassign the username"); + + // What a replica that has not yet consumed the eviction would pass: the principal is the + // reassigned username, the address is the one it cached for the previous holder. + let authed = fetch_authed_from_permissioned_as( + "u/test-user", + "test@windmill.dev", + "test-workspace", + &db, + ) + .await + .expect("should authenticate the current holder"); + + assert_eq!( + authed.email, "newcomer@windmill.dev", + "the principal's live address must win over the one supplied" + ); + assert!( + !authed.is_admin, + "the new holder must not inherit the previous holder's superadmin" + ); +} + +/// A disabled member still holds its username in the workspace. Workspace usernames are only +/// unique per workspace, so an unrelated instance superadmin can share it, and falling through to +/// the `password` fallback would run the disabled member's jobs as that superadmin. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_disabled_member_never_resolves_to_a_same_named_superadmin(db: Pool) { + sqlx::query( + "UPDATE usr SET disabled = true WHERE workspace_id = 'test-workspace' AND username = 'test-user-2'", + ) + .execute(&db) + .await + .expect("disable the member"); + sqlx::query( + "INSERT INTO password(email, password_hash, login_type, super_admin, verified, name, username) + VALUES ('other-superadmin@windmill.dev', 'x', 'password', true, true, 'Other', 'test-user-2')", + ) + .execute(&db) + .await + .expect("create the same-named superadmin"); + + for supplied in ["test2@windmill.dev", "other-superadmin@windmill.dev"] { + let authed = + fetch_authed_from_permissioned_as("u/test-user-2", supplied, "test-workspace", &db) + .await; + assert!( + authed.is_err(), + "a disabled member must not authenticate (supplied {supplied}): {:?}", + authed.map(|a| (a.email, a.is_admin)) + ); + } +} diff --git a/backend/windmill-queue/src/jobs.rs b/backend/windmill-queue/src/jobs.rs index 5bb9a1120f..b6380c02d0 100644 --- a/backend/windmill-queue/src/jobs.rs +++ b/backend/windmill-queue/src/jobs.rs @@ -5505,6 +5505,8 @@ async fn push_inner<'c, 'd>( ) { // Check current usage with SELECT (fast, no row locks) // Only check user usage for non-premium workspaces + // `email` here and in the per-user checks below can be a cached dispatch address, up + // to one notify poll stale; accepted, see `get_email_from_permissioned_as`. let (current_workspace_usage, current_user_usage) = check_usage_limits(db, &billing_w_id, email, !team_plan_status.premium).await?; @@ -6894,7 +6896,11 @@ async fn push_inner<'c, 'd>( language as Option, same_worker, pre_run_error.map(|e| e.to_string()), - email, + // `job_authed`'s, not the handed-in `email`: unless the caller's own authed already names + // this identity, it came through `fetch_authed_from_permissioned_as`, which re-resolves the + // address from the principal's live binding. The same statement writes it to + // `job_perms.email`, and the two columns naming different accounts is what this prevents. + job_authed.email, visible_to_owner, flow_innermost_root_job, guarded_concurrent_limit, @@ -7011,7 +7017,8 @@ async fn push_inner<'c, 'd>( hm.insert("created_by", user); } let audit_author = AuditAuthor { - email: email.to_string(), + // `job_authed`'s address, matching `v2_job` and `job_perms` above. + email: job_authed.email.clone(), username: if runs_on_behalf { windmill_common::auth::permissioned_as_to_username(&permissioned_as) } else { diff --git a/backend/windmill-test-utils/src/lib.rs b/backend/windmill-test-utils/src/lib.rs index 3766a6050d..773616a807 100644 --- a/backend/windmill-test-utils/src/lib.rs +++ b/backend/windmill-test-utils/src/lib.rs @@ -158,6 +158,7 @@ pub struct RunJob { pub payload: JobPayload, pub args: serde_json::Map, pub scheduled_for_o: Option>, + pub username: String, pub email: String, pub job_id: Option, pub workspace_id: String, @@ -169,6 +170,7 @@ impl From for RunJob { payload, args: Default::default(), scheduled_for_o: None, + username: "test-user".to_string(), email: "test@windmill.dev".to_string(), job_id: None, workspace_id: "test-workspace".to_string(), @@ -190,7 +192,11 @@ impl RunJob { self } - pub fn email(mut self, email: impl Into) -> Self { + /// Run as this workspace member. Both halves together, because the job's identity is the + /// principal: an address paired with another member's username is re-resolved at push to + /// the address that username holds. + pub fn as_user(mut self, username: impl Into, email: impl Into) -> Self { + self.username = username.into(); self.email = email.into(); self } @@ -206,7 +212,7 @@ impl RunJob { } pub async fn push(self, db: &Pool) -> Uuid { - let RunJob { payload, args, scheduled_for_o, email, job_id, workspace_id } = self; + let RunJob { payload, args, scheduled_for_o, username, email, job_id, workspace_id } = self; let mut hm_args = std::collections::HashMap::new(); for (k, v) in args { hm_args.insert(k, windmill_common::worker::to_raw_value(&v)); @@ -219,9 +225,9 @@ impl RunJob { &workspace_id, payload, windmill_queue::PushArgs::from(&hm_args), - /* user */ "test-user", + /* user */ &username, /* email */ &email, - /* permissioned_as */ "u/test-user".to_string(), + /* permissioned_as */ format!("u/{username}"), /* token_prefix */ None, /* audit_end_user */ None, scheduled_for_o, diff --git a/backend/windmill-worker/src/worker_flow.rs b/backend/windmill-worker/src/worker_flow.rs index 34f46307cf..31dddcaf5f 100644 --- a/backend/windmill-worker/src/worker_flow.rs +++ b/backend/windmill-worker/src/worker_flow.rs @@ -4438,6 +4438,8 @@ async fn push_next_flow_job( .as_deref() .filter(|t| !t.is_empty() && *t != flow_job.tag.as_str()) { + // A step with its own on-behalf-of carries a cached dispatch address, up to one + // notify poll stale; accepted, see `get_email_from_permissioned_as`. let is_super_admin = windmill_common::auth::is_super_admin_email(db, email).await?; check_tag_available_for_workspace_internal( db, diff --git a/cli/test/deploy_on_behalf_of_unit.test.ts b/cli/test/deploy_on_behalf_of_unit.test.ts index 2214c0dc37..7e21b24832 100644 --- a/cli/test/deploy_on_behalf_of_unit.test.ts +++ b/cli/test/deploy_on_behalf_of_unit.test.ts @@ -1,11 +1,12 @@ import { expect, test } from "bun:test"; import { deployItem } from "../windmill-utils-internal/src/deploy.ts"; -// `deployItem` spreads the source item into the request body, and a script's/flow's -// on_behalf_of 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 key never reaches the wire. +// `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. function recordingProvider(captured: [string, any][], flowExists: boolean) { const source = { on_behalf_of_email: "alice@corp", @@ -32,6 +33,19 @@ function recordingProvider(captured: [string, any][], flowExists: boolean) { }), createScript: async (p: any) => void captured.push(["createScript", p.requestBody]), + existsApp: async () => false, + getAppByPath: async () => ({ + path: "f/x/a", + summary: "", + value: {}, + raw_app: false, + 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]), } as any; } @@ -65,19 +79,29 @@ test("deployItem: never sends the source workspace's on_behalf_of", async () => "dst", "alice@corp", ); + await deployItem( + recordingProvider(captured, false), + "app" as any, + "f/x/a", + "src", + "dst", + "alice@corp", + ); expect(captured.map(([fn]) => fn)).toEqual([ "createFlow", "updateFlow", "createScript", + "createApp", ]); - for (const [, body] of captured) { - // The email is still overridden with the caller's choice... - expect(body.on_behalf_of_email).toBe("alice@corp"); + 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(body)), - ).toBe(false); + expect("on_behalf_of" in JSON.parse(JSON.stringify(identity))).toBe(false); } }); diff --git a/cli/windmill-utils-internal/src/deploy.ts b/cli/windmill-utils-internal/src/deploy.ts index 047d50ee78..ac0b775e86 100644 --- a/cli/windmill-utils-internal/src/deploy.ts +++ b/cli/windmill-utils-internal/src/deploy.ts @@ -506,10 +506,23 @@ export async function deployItem( }, }); } else if (kind === "app" || kind === "raw_app") { - const app = await provider.getAppByPath({ + const rawApp = await provider.getAppByPath({ 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. + const app = { + ...rawApp, + policy: { + ...rawApp.policy, + on_behalf_of: undefined, + on_behalf_of_email: onBehalfOf, + }, + }; if (alreadyExists) { if (app.raw_app) { const secret = await provider.getPublicSecretOfLatestVersionOfApp({ diff --git a/docs/app-policy-email-removal.md b/docs/app-policy-email-removal.md new file mode 100644 index 0000000000..fb4cf100fd --- /dev/null +++ b/docs/app-policy-email-removal.md @@ -0,0 +1,47 @@ +# Removing `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. + +## The gate + +`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. + +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. + +## Step 1 — stop writing the key + +- `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. + +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. diff --git a/frontend/src/lib/utils_workspace_deploy.ts b/frontend/src/lib/utils_workspace_deploy.ts index 382bf94aea..228e32fc06 100644 --- a/frontend/src/lib/utils_workspace_deploy.ts +++ b/frontend/src/lib/utils_workspace_deploy.ts @@ -186,12 +186,20 @@ export type DeployConflict = { hit: boolean } * nowhere else since usernames are per-workspace. The key is therefore always overwritten: * with the picked user's principal for a custom choice, and cleared otherwise so the * backend derives the target's own from the email it is given. The shared `deployItem` - * clears it too, but this app consumes the published package, so the clear has to exist + * 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`. */ function makeProvider( onBehalfOfPrincipal?: string, - appIdentity?: AppIdentity, + onBehalfOf?: string, /** * Refuse the writes the shared `deployItem` reaches for only when the item already exists in * the target, turning its silent switch to an update into a failure the caller can act on. @@ -205,6 +213,17 @@ function makeProvider( ...requestBody, on_behalf_of: onBehalfOfPrincipal }) + const withPolicyIdentity = >(app: T): T => + app.policy + ? { + ...app, + policy: { + ...app.policy, + on_behalf_of: onBehalfOfPrincipal, + on_behalf_of_email: onBehalfOf + } + } + : app const refuseUpdate = (): never => { if (conflict) conflict.hit = true throw new Error('item already exists in the target workspace') @@ -231,26 +250,25 @@ function makeProvider( ? refuseUpdate() : ScriptService.createScript({ ...p, requestBody: withPermissionedAs(p.requestBody) }), archiveScriptByPath: (p) => ScriptService.archiveScriptByPath(p), - // An app's identity lives in its policy, and the shared deploy forwards the source policy - // untouched — it only turns `onBehalfOf` into `preserve_on_behalf_of: true`. Rewriting the - // policy on the way out is therefore the only way a chosen identity reaches the target; the - // backend honours it (`should_preserve` requires `policy.on_behalf_of.is_some()`). - getAppByPath: async (p) => { - const app = await AppService.getAppByPath(p) - if (!appIdentity) return app - return { - ...app, - policy: { - ...app.policy, - on_behalf_of: appIdentity.permissionedAs, - on_behalf_of_email: appIdentity.email - } - } - }, - createApp: (p) => AppService.createApp(p), - updateApp: (p) => (conflict ? refuseUpdate() : AppService.updateApp(p)), - createAppRaw: (p) => AppService.createAppRaw(p), - updateAppRaw: (p) => (conflict ? refuseUpdate() : AppService.updateAppRaw(p)), + getAppByPath: (p) => AppService.getAppByPath(p), + createApp: (p) => + AppService.createApp({ ...p, requestBody: withPolicyIdentity(p.requestBody) }), + updateApp: (p) => + conflict + ? refuseUpdate() + : AppService.updateApp({ ...p, requestBody: withPolicyIdentity(p.requestBody) }), + createAppRaw: (p) => + AppService.createAppRaw({ + ...p, + formData: { ...p.formData, app: withPolicyIdentity(p.formData.app) } + }), + updateAppRaw: (p) => + conflict + ? refuseUpdate() + : AppService.updateAppRaw({ + ...p, + formData: { ...p.formData, app: withPolicyIdentity(p.formData.app) } + }), getPublicSecretOfLatestVersionOfApp: (p) => AppService.getPublicSecretOfLatestVersionOfApp(p), getRawAppData: (p) => AppService.getRawAppData(p), deleteApp: (p) => AppService.deleteApp(p), @@ -401,13 +419,9 @@ export async function deployItem( } } - const appIdentity = - (kind === 'app' || kind === 'raw_app') && onBehalfOf && onBehalfOfPrincipal - ? { email: onBehalfOf, permissionedAs: onBehalfOfPrincipal } - : undefined const conflict: DeployConflict | undefined = createOnly ? { hit: false } : undefined const result = await sharedDeployItem( - makeProvider(onBehalfOfPrincipal, appIdentity, conflict), + makeProvider(onBehalfOfPrincipal, onBehalfOf, conflict), kind as DeployKind, path, workspaceFrom,