From 3536740d06a8ccc292a9b313110f0dce9ecb5d7f Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sun, 2 Aug 2026 17:49:04 +0000 Subject: [PATCH] fix: evict on superadmin role changes, not only address changes --- ...02172300_notify_user_email_change.down.sql | 6 ++- ...0802172300_notify_user_email_change.up.sql | 36 +++++++++---- backend/windmill-common/src/lib.rs | 7 +-- backend/windmill-common/src/users.rs | 9 ++-- .../windmill-common/tests/notify_events.rs | 50 +++++++++++++------ 5 files changed, 75 insertions(+), 33 deletions(-) diff --git a/backend/migrations/20260802172300_notify_user_email_change.down.sql b/backend/migrations/20260802172300_notify_user_email_change.down.sql index c8e7efacc2..7f6f494115 100644 --- a/backend/migrations/20260802172300_notify_user_email_change.down.sql +++ b/backend/migrations/20260802172300_notify_user_email_change.down.sql @@ -1,6 +1,8 @@ -- Add down migration script here -DROP TRIGGER IF EXISTS password_superadmin_email_change_trigger ON password; +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_email_change(); +DROP FUNCTION IF EXISTS notify_superadmin_identity_change(); DROP FUNCTION IF EXISTS notify_usr_email_change(); diff --git a/backend/migrations/20260802172300_notify_user_email_change.up.sql b/backend/migrations/20260802172300_notify_user_email_change.up.sql index 2fc05c583c..5133129d9f 100644 --- a/backend/migrations/20260802172300_notify_user_email_change.up.sql +++ b/backend/migrations/20260802172300_notify_user_email_change.up.sql @@ -35,11 +35,11 @@ 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, so there is no key to target: clear the whole cache. The empty payload --- is the wildcard. Confined to superadmins because they are the only accounts the `usr` triggers +-- A superadmin acting outside their workspaces resolves through `password` instead, and that row +-- names no workspace, so there is no key to target: clear the whole cache. The empty payload is +-- the wildcard. Confined to superadmins because they are the only accounts the `usr` triggers -- above cannot cover. -CREATE OR REPLACE FUNCTION notify_superadmin_email_change() +CREATE OR REPLACE FUNCTION notify_superadmin_identity_change() RETURNS TRIGGER AS $$ BEGIN INSERT INTO notify_event (channel, payload) VALUES ('notify_user_email_change', ''); @@ -47,9 +47,27 @@ BEGIN END; $$ LANGUAGE plpgsql SECURITY DEFINER; -CREATE TRIGGER password_superadmin_email_change_trigger -AFTER UPDATE OF email, username ON password +-- `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 (NEW.super_admin - AND (OLD.email IS DISTINCT FROM NEW.email OR OLD.username IS DISTINCT FROM NEW.username)) -EXECUTE FUNCTION notify_superadmin_email_change(); +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/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index dbb80d7dc9..36915b1cc8 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -1702,9 +1702,10 @@ pub async fn on_behalf_of_from_permissioned_as( 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. + // and decides the superadmin flag and the instance groups. `notify_user_email_change` now + // evicts the cache on every replica, which is what lets app dispatch read it cached; this + // site can follow once it is exercised the same way, and stays uncached until then rather + // than for a reason of its own. let email = users::get_email_from_permissioned_as_uncached(permissioned_as, w_id, db).await?; Ok(Some(jobs::OnBehalfOf { email, permissioned_as: permissioned_as.to_string() })) } diff --git a/backend/windmill-common/src/users.rs b/backend/windmill-common/src/users.rs index 5d7c7f8022..94f757cd2e 100644 --- a/backend/windmill-common/src/users.rs +++ b/backend/windmill-common/src/users.rs @@ -233,10 +233,11 @@ 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`] without the address cache. `notify_user_email_change` +/// evicts that cache on every replica, so the cached read is the right default; this variant is +/// for the narrower case where even the gap between the change committing and the eviction +/// arriving is too much — a value about to be compared against a freshly resolved one, or +/// written somewhere that outlives the request. /// /// Reads through the non-RLS pool and authorizes nothing, like the cached one: callers must /// already be authorized for `workspace_id`. diff --git a/backend/windmill-common/tests/notify_events.rs b/backend/windmill-common/tests/notify_events.rs index f77ff57d33..821e791256 100644 --- a/backend/windmill-common/tests/notify_events.rs +++ b/backend/windmill-common/tests/notify_events.rs @@ -381,22 +381,42 @@ async fn test_trigger_notify_user_email_change(db: Pool) { ); // A superadmin outside their workspaces resolves through `password`, which names no - // workspace: the wildcard is the only way to reach that key. - let before_id = get_latest_event_id(&db).await.unwrap(); - sqlx::query("UPDATE password SET email = 'sa2@windmill.dev' WHERE email = 'test@windmill.dev'") - .execute(&db) - .await - .expect("Failed to change superadmin email"); + // 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) in [ + ( + "email change", + "UPDATE password SET email = 'sa2@windmill.dev' WHERE email = 'test@windmill.dev'", + ), + ( + "demotion", + "UPDATE password SET super_admin = false WHERE email = 'sa2@windmill.dev'", + ), + ( + "promotion", + "UPDATE password SET super_admin = true WHERE email = 'sa2@windmill.dev'", + ), + ( + "deletion", + "DELETE FROM password WHERE email = 'sa2@windmill.dev'", + ), + ] { + 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"); - assert!( - events - .iter() - .any(|e| e.channel == "notify_user_email_change" && e.payload.is_empty()), - "superadmin email change should emit the wildcard" - ); + 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.is_empty()), + "superadmin {label} should emit the wildcard" + ); + } } #[sqlx::test(migrations = "../migrations", fixtures("base"))]