diff --git a/backend/.sqlx/query-3c76304619fba1222eec7fcfa7057e488a4ba601be7062d3c79097513cb93723.json b/backend/.sqlx/query-3c76304619fba1222eec7fcfa7057e488a4ba601be7062d3c79097513cb93723.json new file mode 100644 index 0000000000..ff202c0d43 --- /dev/null +++ b/backend/.sqlx/query-3c76304619fba1222eec7fcfa7057e488a4ba601be7062d3c79097513cb93723.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "\n WITH RECURSIVE tree AS (\n SELECT id, deleted, 0 AS depth FROM workspace WHERE id = $1\n UNION ALL\n SELECT w.id, w.deleted, tree.depth + 1 FROM workspace w\n JOIN tree ON w.parent_workspace_id = tree.id\n WHERE tree.depth < 20\n )\n SELECT id AS \"id!\" FROM tree WHERE id != $1 AND NOT deleted ORDER BY id\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id!", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "3c76304619fba1222eec7fcfa7057e488a4ba601be7062d3c79097513cb93723" +} diff --git a/backend/.sqlx/query-f117e4fd909fe5f2fe821e176011e838bd797c023123735ed52eb0507455165a.json b/backend/.sqlx/query-f117e4fd909fe5f2fe821e176011e838bd797c023123735ed52eb0507455165a.json new file mode 100644 index 0000000000..7a1279bbcd --- /dev/null +++ b/backend/.sqlx/query-f117e4fd909fe5f2fe821e176011e838bd797c023123735ed52eb0507455165a.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(datatable, '{datatables}', COALESCE((\n SELECT jsonb_object_agg(key, value - 'permissions')\n FROM jsonb_each(datatable->'datatables')\n WHERE COALESCE((value->'permissions'->>'enabled')::boolean, false) = false\n ), '{}'::jsonb))\n WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [] + }, + "hash": "f117e4fd909fe5f2fe821e176011e838bd797c023123735ed52eb0507455165a" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs index 1c55ace13d..45526aee08 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs @@ -152,6 +152,56 @@ async fn enabling_permissions_is_refused_while_a_fork_exists( assert!(text.contains("wm-fork-t"), "{endpoint}: {text}"); } + // Only the opt-in is gated: once permissions are on, forks made afterwards + // never receive the data table, and editing the roles — revoking a tenant + // above all — has to keep working while they exist. The preview then gets as + // far as the database, which this test does not have. + sqlx::query( + r#"UPDATE workspace_settings + SET datatable = jsonb_set(datatable, '{datatables,main,permissions}', + '{"enabled": true, "roles": {"admin": {"tenants": []}}}') + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + let edit = json!({ "enabled": true, "roles": [ + { "name": "admin", "tenants": [] }, { "name": "analyst", "tenants": ["u/test-user"] } + ]}); + let resp = authed( + client().post(format!( + "{ws}/workspaces/datatable_permissions/main/preview" + )), + "SECRET_TOKEN", + ) + .json(&edit) + .send() + .await?; + let text = resp.text().await?; + assert!(!text.contains("cannot be enabled"), "{text}"); + sqlx::query( + r#"UPDATE workspace_settings + SET datatable = datatable #- '{datatables,main,permissions}' + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + + // An archived fork has no members left to reach anything. + sqlx::query("UPDATE workspace SET deleted = true WHERE id = 'wm-fork-t'") + .execute(&db) + .await?; + let resp = authed( + client().post(format!( + "{ws}/workspaces/datatable_permissions/main/preview" + )), + "SECRET_TOKEN", + ) + .json(&body) + .send() + .await?; + let text = resp.text().await?; + assert!(!text.contains("has forks"), "{text}"); + // A workspace that is no longer a fork — a detached dev workspace — keeps its // copy of the data table, pointing at the same instance database. sqlx::query("DELETE FROM workspace WHERE id = 'wm-fork-t'") @@ -202,3 +252,72 @@ async fn enabling_permissions_is_refused_while_a_fork_exists( Ok(()) } + +/// The rename keeps a copy of the settings under the archived id, and commits it +/// before the old id is archived. A permissioned data table must not be in that +/// copy: without its `permissions` block it would resolve, for anyone still +/// using the old id, to the owner connection. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn a_rename_leaves_no_permissioned_datatable_under_the_old_id( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + sqlx::query( + r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#, + ) + .bind(json!({ + "datatables": { + "open": { "database": { "resource_type": "instance", "resource_path": "dt_open" } }, + "main": { + "database": { "resource_type": "instance", "resource_path": "dt_main" }, + "permissions": { "enabled": true, "roles": { + "admin": { "tenants": [] }, + "analyst": { "tenants": ["*"], "pg_rolename": "wm_x", "pg_password": "s3cret" } + }} + } + } + })) + .execute(&db) + .await?; + + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/test-workspace/workspaces/change_workspace_id" + )), + "SECRET_TOKEN", + ) + .json(&json!({ "new_id": "renamed-ws", "new_name": "Renamed" })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + let names = |w: &'static str| { + let db = db.clone(); + async move { + let value: serde_json::Value = sqlx::query_scalar( + "SELECT datatable->'datatables' FROM workspace_settings WHERE workspace_id = $1", + ) + .bind(w) + .fetch_one(&db) + .await + .unwrap(); + let mut keys: Vec = value.as_object().unwrap().keys().cloned().collect(); + keys.sort(); + (keys, value) + } + }; + let (old_names, _) = names("test-workspace").await; + assert_eq!(old_names, vec!["open".to_string()]); + let (new_names, new_value) = names("renamed-ws").await; + assert_eq!(new_names, vec!["main".to_string(), "open".to_string()]); + assert_eq!(new_value["main"]["permissions"]["enabled"], json!(true)); + assert_eq!( + new_value["main"]["permissions"]["roles"]["analyst"]["pg_password"], + json!("s3cret") + ); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index c792d441c6..6e2a2bc4ce 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -734,17 +734,17 @@ pub(crate) async fn ensure_can_use_datatable_role( /// the data table was unpermissioned carries a verbatim copy of it, and every /// member of that fork — including members this workspace does not have — would /// keep reaching the database through the copy's own connection, which owns -/// everything in it. Forks made after the opt-in never receive a permissioned -/// data table (see the strip in the fork creation), so refusing while any exist -/// is what closes the gap. A dev workspace detached from this one keeps such a -/// copy without being a fork any more, which is what the last check is for; it -/// is exact for an instance database, whose only credential holders are the -/// data tables naming it, and meaningless for a resource-backed one, where -/// whoever holds the resource's credentials reaches the database regardless. +/// everything in it. A dev workspace detached from this one keeps such a copy +/// without being a fork any more, which is what the last check is for; it is +/// exact for an instance database, whose only credential holders are the data +/// tables naming it, and meaningless for a resource-backed one, where whoever +/// holds the resource's credentials reaches the database regardless. /// -/// Turning them off is always allowed, or a workspace carrying permissions from -/// before this rule could never be rid of them, and the roles behind them never -/// dropped. +/// All three are properties of the opt-in, so only the save that turns +/// permissions on is checked. Forks made afterwards never receive a permissioned +/// data table (see the strip in the fork creation), and a save that edits the +/// roles of a live config must keep working while they exist — revoking a tenant +/// above all. Turning permissions off is never refused. /// /// The save calls this under the settings row lock, which fork creation takes on /// the parent before copying its settings: a fork mid-creation has either @@ -758,6 +758,10 @@ async fn refuse_enabling_permissions_over_shared_access( if !enabled { return Ok(()); } + let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; + if datatable.permissions.as_ref().is_some_and(|p| p.enabled) { + return Ok(()); + } if crate::workspaces_extra::workspace_is_fork(db, w_id).await? { return Err(Error::BadRequest( "Data table permissions cannot be enabled from a fork workspace: a fork's data \ @@ -768,7 +772,7 @@ async fn refuse_enabling_permissions_over_shared_access( .to_string(), )); } - let forks = windmill_common::workspaces::list_fork_descendants(db, w_id).await?; + let forks = windmill_common::workspaces::list_live_fork_descendants(db, w_id).await?; if !forks.is_empty() { return Err(Error::BadRequest(format!( "Data table permissions cannot be enabled while this workspace has forks ({}): a \ @@ -778,7 +782,6 @@ async fn refuse_enabling_permissions_over_shared_access( forks.join(", ") ))); } - let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?; if datatable.database.resource_type == DataTableCatalogResourceType::Instance { let others = sqlx::query!( r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!" diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 3dc1f2a89c..f76fd1bdc3 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3126,15 +3126,14 @@ pub(crate) async fn is_instance_datatable(db: &DB, w_id: &str, name: &str) -> Re .unwrap_or(false)) } -/// Same, for the `datatable://` / `$res:` form the import endpoints take. /// Refuse to clone a data table whose role permissions are enabled. /// /// A clone lands in a brand-new database where none of the roles exist, and the /// fork's copy of the config is stripped of its permissions — so every member of /// the fork resolves to the copy's own owner connection and reads, in full, the /// data the roles existed to divide. Reproducing the roles in the copy is a -/// separate piece of work; until it exists, a fork shares the original, which -/// keeps the parent's restrictions, or goes without. +/// separate piece of work; until it exists, a fork goes without the data table +/// (the fork creation leaves a permissioned one out of the fork's config). pub(crate) async fn refuse_clone_of_permissioned_datatable( db: &DB, w_id: &str, @@ -3157,12 +3156,14 @@ pub(crate) async fn refuse_clone_of_permissioned_datatable( return Err(Error::BadRequest(format!( "Data table '{name}' has role permissions enabled and cannot be cloned into a fork: \ the copy cannot carry its roles, so it would be readable in full by every member of \ - the fork. Keep the original instead — the fork shares it with the same restrictions." + the fork. The fork goes without it; disable its permissions first to clone it." ))); } Ok(()) } +/// Same as [`is_instance_datatable`], for the `datatable://` / `$res:` form the +/// import endpoints take. async fn is_instance_datatable_source(db: &DB, w_id: &str, source: &str) -> Result { match source.strip_prefix("datatable://") { Some(name) => is_instance_datatable(db, w_id, name).await, diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index d870890f50..18da27d677 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -62,6 +62,12 @@ pub(crate) async fn change_workspace_id( // rename moves the chain from one to the other. crate::workspaces::lock_dev_pairing(&mut tx, &[&old_id, &rw.new_id]).await?; + // The settings are copied below, and a permissions save holds this row while it changes + // the roles in the database and then the config: copied without it, the new workspace + // could carry the config from before that save while the database has the roles from + // after it. Same order as fork creation: pairing lock, then the settings row. + windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &old_id).await?; + check_w_id_conflict(&mut tx, &rw.new_id).await?; info!( @@ -120,21 +126,25 @@ pub(crate) async fn change_workspace_id( .execute(&mut *tx) .await?; - // Two configs now name the same Postgres logins, and only one of them owns - // them: deleting the archived id would plan drops for logins the renamed - // workspace is still using. The archived copy stops naming them — it is kept - // for reference, and a reference does not need credentials. + // Two configs now name the same Postgres logins, and only one of them owns them: + // deleting the archived id would plan drops for logins the renamed workspace is still + // using. A permissioned data table leaves the archived copy entirely rather than losing + // its `permissions` block: this transaction commits before the old id is archived, and + // a copy that still named the database without the block would hand every caller of the + // old id the owner connection in between — and again if the shell were ever unarchived. + // A missing data table fails closed. The unpermissioned ones stay, for reference. // - // The renamed workspace keeps them and keeps working: a role's `pg_rolename` - // is what resolution uses, and the generated name only decides what a *new* - // role is called. Its next permissions save finds the stored name no longer - // matches the one this workspace id generates and renames the login to match, - // under the ownership proof that rename already carries. + // The renamed workspace keeps the roles and keeps working: a role's `pg_rolename` is + // what resolution uses, and the generated name only decides what a *new* role is called. + // Its next permissions save finds the stored name no longer matches the one this + // workspace id generates and renames the login to match, under the ownership proof that + // rename already carries. sqlx::query!( "UPDATE workspace_settings SET datatable = jsonb_set(datatable, '{datatables}', COALESCE(( SELECT jsonb_object_agg(key, value - 'permissions') FROM jsonb_each(datatable->'datatables') + WHERE COALESCE((value->'permissions'->>'enabled')::boolean, false) = false ), '{}'::jsonb)) WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", &old_id, diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 7681f3d07b..75d5b484d0 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -737,6 +737,31 @@ pub async fn list_fork_descendants(db: &crate::DB, w_id: &str) -> Result Result> { + let ids = sqlx::query_scalar!( + r#" + WITH RECURSIVE tree AS ( + SELECT id, deleted, 0 AS depth FROM workspace WHERE id = $1 + UNION ALL + SELECT w.id, w.deleted, tree.depth + 1 FROM workspace w + JOIN tree ON w.parent_workspace_id = tree.id + WHERE tree.depth < 20 + ) + SELECT id AS "id!" FROM tree WHERE id != $1 AND NOT deleted ORDER BY id + "#, + w_id + ) + .fetch_all(db) + .await + .map_err(|e| Error::internal_err(format!("listing live fork descendants of {w_id}: {e:#}")))?; + Ok(ids) +} + /// Count non-deleted fork/dev workspaces anywhere under `root` (excludes `root` itself). /// /// Unauthenticated metering helper: it reads workspace hierarchy for any `root` id, so callers must @@ -3476,7 +3501,8 @@ mod tests { // Repointed — however the resource got there, including through a `$var:` // no guard on the resource itself would see. assert!( - ensure_datatable_database_unchanged("main", &dt, &resolved("elsewhere", "one")).is_err() + ensure_datatable_database_unchanged("main", &dt, &resolved("elsewhere", "one")) + .is_err() ); // A config that never recorded one cannot claim to match: the roles it // names were created against a database nobody wrote down.