mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-21 00:02:23 +00:00
fix: evict on superadmin role changes, not only address changes
This commit is contained in:
@@ -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();
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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() }))
|
||||
}
|
||||
|
||||
@@ -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`.
|
||||
|
||||
@@ -381,22 +381,42 @@ async fn test_trigger_notify_user_email_change(db: Pool<Postgres>) {
|
||||
);
|
||||
|
||||
// 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"))]
|
||||
|
||||
Reference in New Issue
Block a user