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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JY4bBCR1q2c5XB8s2r7Ysc

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-09-12 09:10:15 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent e877b5f2e8
commit c90d1d95c2
40 changed files with 1212 additions and 222 deletions
@@ -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"
}
@@ -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"
}
@@ -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"
}
@@ -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"
}
@@ -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"
}
@@ -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"
}
@@ -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"
}
@@ -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"
}
@@ -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;
@@ -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);
@@ -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();
@@ -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();
+11
View File
@@ -1913,6 +1913,17 @@ async fn process_notify_event(
);
windmill_api::auth::invalidate_token_from_cache(payload);
}
"notify_user_email_change" => {
// `<workspace_id>:<username>`, or `*:<username>` 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_id>:<path>`; workspace ids can't contain ':'.
if server_mode {
@@ -630,7 +630,7 @@ async fn test_folder_default_permissioned_as(db: Pool<Postgres>) -> 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
+98 -30
View File
@@ -405,11 +405,12 @@ async fn test_preserve_on_behalf_of(db: Pool<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>,
) -> 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<Postgres>) -> 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<Postgres>) -> 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",
+1 -1
View File
@@ -5483,7 +5483,7 @@ async fn test_flow_substep_tag_availability_check(db: Pool<Postgres>) -> 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;
+8 -1
View File
@@ -1053,6 +1053,10 @@ pub async fn fetch_api_authed_from_permissioned_as(
db: &DB,
username_override: Option<String>,
) -> error::Result<ApiAuthed> {
// 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,
+1 -1
View File
@@ -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?,
@@ -599,3 +599,59 @@ async fn test_offboard_invalid_target(db: Pool<Postgres>) -> 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<Postgres>,
) -> 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<String> = 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(())
}
+11 -8
View File
@@ -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!(
+25 -3
View File
@@ -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)
+2
View File
@@ -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: >
+137 -75
View File
@@ -491,6 +491,13 @@ pub struct S3Key {
#[derive(Serialize, Deserialize, Debug, Clone, Default)]
pub struct Policy {
pub on_behalf_of: Option<String>,
/// 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<String>,
//paths:
// - script/<path>
@@ -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<String> = 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<String, Box<serde_json::value::RawValue>> = 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<String> = 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<String> = 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<ApiAuthed>,
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<Option<String>> {
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<String> {
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",
+34 -13
View File
@@ -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<String> {
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<OffboardPreview> {
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<OffboardSummary> {
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
)
+47
View File
@@ -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,
@@ -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?;
+41 -15
View File
@@ -452,6 +452,42 @@ async fn fetch_authed_from_permissioned_as_inner(
w_id: &str,
conn: &mut sqlx::PgConnection,
) -> Result<Authed> {
// 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?;
-2
View File
@@ -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)))
+24 -12
View File
@@ -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(),
-3
View File
@@ -91,9 +91,6 @@ async fn prefetch_cached_script_inner(
derive_email: bool,
) -> crate::error::Result<Script<ScriptRunnableSettingsInline>> {
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,
+58 -9
View File
@@ -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,
@@ -360,6 +360,77 @@ async fn test_trigger_notify_workspace_key_change(db: Pool<Postgres>) {
);
}
/// 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<Postgres>) {
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<Postgres>) {
// First insert a session token with token_hash and token_prefix
@@ -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<Postgres>) {
// `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<Postgres>) {
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))
);
}
}
+9 -2
View File
@@ -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<ScriptLang>,
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 {
+10 -4
View File
@@ -158,6 +158,7 @@ pub struct RunJob {
pub payload: JobPayload,
pub args: serde_json::Map<String, serde_json::Value>,
pub scheduled_for_o: Option<chrono::DateTime<chrono::Utc>>,
pub username: String,
pub email: String,
pub job_id: Option<Uuid>,
pub workspace_id: String,
@@ -169,6 +170,7 @@ impl From<JobPayload> 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<String>) -> 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<String>, email: impl Into<String>) -> Self {
self.username = username.into();
self.email = email.into();
self
}
@@ -206,7 +212,7 @@ impl RunJob {
}
pub async fn push(self, db: &Pool<Postgres>) -> 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,
@@ -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,
+35 -11
View File
@@ -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);
}
});
+14 -1
View File
@@ -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({
+47
View File
@@ -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.
+41 -27
View File
@@ -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 = <T extends Record<string, any>>(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,