From f7f8741b367ac2c689d92c77cfc9eea507c6d9b3 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 12 Oct 2026 22:21:29 +0200 Subject: [PATCH] feat: drop data table role set logins unused for 30 days Co-Authored-By: Claude Opus 5.5 --- backend/ee-repo-ref.txt | 2 +- .../20261012180101_datatable_role_set.up.sql | 3 +++ backend/src/monitor.rs | 19 ++++++++++++++++ .../tests/datatable_roles.rs | 22 +++++++++++++++++++ .../windmill-common/src/datatable_roles.rs | 7 ++++++ .../src/datatable_roles_oss.rs | 8 ++++++- docs/datatable-viewer-roles.md | 11 ++++++---- 7 files changed, 66 insertions(+), 6 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 699243fc55..7f79b26021 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -6dedb4687f591a88aba1503d55a001fcb0ab128a +de85414a7dc5ccddbe9c2c7bb390396b9bf1742f diff --git a/backend/migrations/20261012180101_datatable_role_set.up.sql b/backend/migrations/20261012180101_datatable_role_set.up.sql index 15642cc0ca..0093837d3b 100644 --- a/backend/migrations/20261012180101_datatable_role_set.up.sql +++ b/backend/migrations/20261012180101_datatable_role_set.up.sql @@ -11,6 +11,9 @@ CREATE TABLE datatable_role_set ( -- Generated by Windmill, never entered by anyone, and never leaves the server. pwd TEXT NOT NULL, created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + -- Recorded at most once a day. A login unused for long enough is dropped, and made again if + -- its set of roles is asked for later. + last_used_at TIMESTAMPTZ NOT NULL DEFAULT now(), PRIMARY KEY (cluster, name) ); diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index 8de85baa24..b7cbbfdd5e 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -4449,6 +4449,24 @@ pub async fn monitor_db( } }; + // Drop the data table role set logins nobody has connected with for a month. Every ~40 min, + // spawned for the same reason as the credential maintenance above: it opens a connection per + // managed database for each login it drops. + let datatable_role_set_sweep_f = async { + if server_mode && iteration.is_some() && iteration.as_ref().unwrap().should_run(240) { + if let Some(db) = conn.as_sql() { + let db = db.clone(); + tokio::spawn(async move { + if let Err(e) = + windmill_common::datatable_roles::sweep_unused_role_set_logins(&db).await + { + tracing::error!("Error sweeping unused data table role set logins: {e}"); + } + }); + } + } + }; + // Delete the AI session backups older than their workspace's retention. Every ~40 min // (240 iterations at the default 10 s, the most a u8 `should_run` counts): the retention // counts in days. Spawned for the same reason as the credential maintenance above, a @@ -4520,6 +4538,7 @@ pub async fn monitor_db( cleanup_scheduled_job_deletions_f, git_auto_pull_f, git_credential_maintenance_f, + datatable_role_set_sweep_f, ai_session_retention_f, pipeline_freshness_watchdog_f, reconcile_unarmed_schedules_f, diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index e14fe61986..627e99fb1a 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -1947,6 +1947,28 @@ async fn connecting_as_the_viewer_takes_the_roles_that_name_them_and_no_other( // No recorded viewer must not fall back to the job's owner. assert!(as_viewer(None).await.is_err()); + // A set login nobody connects with any more is dropped, a recent one is kept, and a + // dropped one is made again for the next viewer who needs it. + let exists = || async { + sqlx::query_scalar::<_, bool>( + "SELECT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = $1)", + ) + .bind(&login) + .fetch_one(&db) + .await + }; + windmill_common::datatable_roles::sweep_unused_role_set_logins(&db).await?; + assert!(exists().await?, "a set login in use was swept"); + sqlx::query("UPDATE datatable_role_set SET last_used_at = now() - interval '31 days'") + .execute(&db) + .await?; + windmill_common::datatable_roles::sweep_unused_role_set_logins(&db).await?; + assert!(!exists().await?, "an unused set login was kept"); + assert_eq!( + as_viewer(Some("test2@windmill.dev")).await?["user"], + login.as_str() + ); + // A disabled role must not stay reachable through a login that is a member of it. authed( client().post(format!("{roles_url}/{}", ids[1])), diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 9488d7846a..1434f27583 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -419,6 +419,13 @@ pub async fn drop_datatable_role( crate::datatable_roles_oss::drop_datatable_role(db, tx, cluster, name).await } +/// Drop the logins made for a connection as several roles at once that nobody has connected with +/// for a month, on every cluster. They are made again on demand, so this only bounds how many +/// exist. Safe to call from any server at any time: it serializes on the role catalog lock. +pub async fn sweep_unused_role_set_logins(db: &DB) -> Result<()> { + crate::datatable_roles_oss::sweep_unused_role_set_logins(db).await +} + #[cfg(test)] mod tests { use super::*; diff --git a/backend/windmill-common/src/datatable_roles_oss.rs b/backend/windmill-common/src/datatable_roles_oss.rs index 33d711e7f9..739f08d60b 100644 --- a/backend/windmill-common/src/datatable_roles_oss.rs +++ b/backend/windmill-common/src/datatable_roles_oss.rs @@ -29,7 +29,8 @@ pub(crate) use crate::datatable_roles_ee::{ ensure_instance_db_grant_options_unchecked, forget_datatable_role_everywhere, insert_role_catalog_entry, read_role_catalog, read_role_catalog_tx, registered_instance_databases, rename_datatable_role, resolve_datatable_role_connection, - role_cluster, set_datatable_role_login, update_role_catalog_entry, + role_cluster, set_datatable_role_login, sweep_unused_role_set_logins, + update_role_catalog_entry, }; #[cfg(not(all(feature = "private", feature = "enterprise")))] @@ -235,6 +236,11 @@ mod ce { } } + /// No roles, so no set login to sweep. + pub(crate) async fn sweep_unused_role_set_logins(_db: &DB) -> Result<()> { + Ok(()) + } + pub(crate) async fn forget_datatable_role_everywhere( _tx: &mut Tx<'_>, _role_id: &str, diff --git a/docs/datatable-viewer-roles.md b/docs/datatable-viewer-roles.md index 206a13f8f5..fcfa1c59db 100644 --- a/docs/datatable-viewer-roles.md +++ b/docs/datatable-viewer-roles.md @@ -27,7 +27,7 @@ A policy names its role as text, so renaming the role breaks it. | Where | What | |---|---| | `windmill-common/src/workspaces.rs` | `DATATABLE_VIEWER_ROLE`, `parse_datatable_ref`, `DatatableAccess` | -| `windmill-common/src/datatable_roles_ee.rs` | `resolve_viewer_login`, `ensure_role_set_login`, `retire_role_set_logins_of` | +| `windmill-common/src/datatable_roles_ee.rs` | `resolve_viewer_login`, `ensure_role_set_login`, `retire_role_set_logins_of`, `sweep_unused_role_set_logins` | | `windmill-api-jobs/src/execution.rs` | `run_end_user_email`, where a run's viewer is recorded | ## What holds it together @@ -52,6 +52,12 @@ A policy names its role as text, so renaming the role breaks it. - **A `datatable_role_set` row always names a login that can authenticate.** The row is written after the login exists and removed, in its own commit, before the login is dropped or made `NOLOGIN`. A login without a row is taken back with a new password on next use. +- **Set logins are bounded by use, not by the number of role combinations.** One exists only for + a set some viewer actually connected with, so there are at most as many as (viewer, data table) + pairs. `last_used_at` is recorded at most once a day, and `sweep_unused_role_set_logins`, run + from the monitor loop, drops those unused for 30 days; a dropped one is made again on demand. + The sweep goes by this instance's rows, never by the cluster's `wm_set_*` roles: a cluster can + serve several Windmill instances. - **Disabling or dropping a role retires every set login holding it.** `NOLOGIN` on a role does not reach a login that is a member of it. - **It travels in the reference, not in an annotation.** A worker that predates `?as=` refuses the @@ -66,6 +72,3 @@ table, the same trust as an app in viewer execution mode or a script someone els run. Whoever can deploy a publisher-mode app, or a script that others run, can therefore read through the non-`admin` roles of the people who use it; nothing on the data table opts in to that. - -A set login is dropped only with one of its roles. One that no viewer resolves to any more stays -on the cluster, holding only roles that still exist.