diff --git a/backend/.sqlx/query-a1b9097dc53acafe21256f093633b89f944ddd5ac9d0bafe4bcbbbd934518535.json b/backend/.sqlx/query-a1b9097dc53acafe21256f093633b89f944ddd5ac9d0bafe4bcbbbd934518535.json new file mode 100644 index 0000000000..d6fdf2099b --- /dev/null +++ b/backend/.sqlx/query-a1b9097dc53acafe21256f093633b89f944ddd5ac9d0bafe4bcbbbd934518535.json @@ -0,0 +1,29 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id AND NOT w.deleted,\n jsonb_each(ws.datatable->'datatables') dt\n WHERE ws.workspace_id <> $1\n AND dt.value->'database'->>'resource_type' = 'instance'\n AND dt.value->'database'->>'resource_path' = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id!", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "name!", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + null + ] + }, + "hash": "a1b9097dc53acafe21256f093633b89f944ddd5ac9d0bafe4bcbbbd934518535" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs index 2dd476afd0..1c55ace13d 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs @@ -106,3 +106,99 @@ async fn freeing_a_principal_takes_its_datatable_tenant(db: Pool) -> a Ok(()) } + +/// A fork made while the data table was unpermissioned carries a copy of it that +/// points at the same database; opting in would leave every member of the fork +/// reaching that database through the copy's own connection. Pinned on the save +/// and on the preview, since a plan the save refuses to run must not be offered. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn enabling_permissions_is_refused_while_a_fork_exists( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = format!("http://localhost:{port}/api/w/test-workspace"); + + sqlx::query( + r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#, + ) + .bind(json!({ + "datatables": { + "main": { "database": { "resource_type": "instance", "resource_path": "dt_main" } } + } + })) + .execute(&db) + .await?; + sqlx::query( + "INSERT INTO workspace (id, name, owner, parent_workspace_id) + VALUES ('wm-fork-t', 'wm-fork-t', 'test-user', 'test-workspace')", + ) + .execute(&db) + .await?; + + let body = json!({ "enabled": true, "roles": [] }); + for endpoint in [ + "workspaces/datatable_permissions/main/preview", + "workspaces/datatable_permissions/main", + ] { + let resp = authed(client().post(format!("{ws}/{endpoint}")), "SECRET_TOKEN") + .json(&body) + .send() + .await?; + let status = resp.status(); + let text = resp.text().await?; + assert_eq!(status, 400, "{endpoint}: {text}"); + assert!(text.contains("wm-fork-t"), "{endpoint}: {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'") + .execute(&db) + .await?; + sqlx::query( + "INSERT INTO workspace (id, name, owner) VALUES ('detached', 'detached', 'test-user')", + ) + .execute(&db) + .await?; + sqlx::query("INSERT INTO workspace_settings (workspace_id, datatable) VALUES ('detached', $1)") + .bind(json!({ + "datatables": { + "copy": { "database": { "resource_type": "instance", "resource_path": "dt_main" } } + } + })) + .execute(&db) + .await?; + let resp = authed( + client().post(format!( + "{ws}/workspaces/datatable_permissions/main/preview" + )), + "SECRET_TOKEN", + ) + .json(&body) + .send() + .await?; + assert_eq!(resp.status(), 400); + let text = resp.text().await?; + assert!(text.contains("detached (data table 'copy')"), "{text}"); + + // With both gone the refusal lifts: the preview then gets as far as the + // database, which this test does not have. + sqlx::query("DELETE FROM workspace_settings WHERE workspace_id = 'detached'") + .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("cannot be enabled"), "{text}"); + + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 3d9a8e409e..c792d441c6 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -723,18 +723,42 @@ pub(crate) async fn ensure_can_use_datatable_role( Ok(()) } -/// A fork may not turn a data table's permissions on. +/// Permissions are turned on where this workspace is the only one reaching the +/// database: no fork above it, none below it, and no other workspace's data +/// table naming the same instance database. /// -/// Its data table is either a copy pointing at the database of the workspace it -/// was forked from, where roles created here would hold grants that workspace's -/// own config does not name, or a clone whose whole database the fork can drop, -/// taking the roles with it. Neither is a place to build an access model; it is -/// built in the workspace that owns the data table. +/// A fork's data table is either a copy pointing at the database of the workspace +/// it was forked from, where roles created in the fork would hold grants that +/// workspace's own config does not name, or a clone whose whole database the fork +/// can drop, taking the roles with it. In the other direction, a fork made while +/// 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. /// -/// Turning them off is always allowed, or a fork carrying permissions from before -/// this rule could never be rid of them, and the roles behind them never dropped. -async fn refuse_enabling_permissions_from_a_fork(db: &DB, w_id: &str, enabled: bool) -> Result<()> { - if enabled && crate::workspaces_extra::workspace_is_fork(db, w_id).await? { +/// 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. +/// +/// 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 +/// committed, and is listed here, or copies the config after the opt-in landed. +async fn refuse_enabling_permissions_over_shared_access( + db: &DB, + w_id: &str, + datatable_name: &str, + enabled: bool, +) -> Result<()> { + if !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 \ table points either at the database of the workspace it was forked from, where \ @@ -744,6 +768,44 @@ async fn refuse_enabling_permissions_from_a_fork(db: &DB, w_id: &str, enabled: b .to_string(), )); } + let forks = windmill_common::workspaces::list_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 \ + fork holds a copy of the data table pointing at the same database, and its members \ + would keep reaching it through the data table's own connection, as every role at \ + once. Delete the forks first.", + 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!" + FROM workspace_settings ws + JOIN workspace w ON w.id = ws.workspace_id AND NOT w.deleted, + jsonb_each(ws.datatable->'datatables') dt + WHERE ws.workspace_id <> $1 + AND dt.value->'database'->>'resource_type' = 'instance' + AND dt.value->'database'->>'resource_path' = $2"#, + w_id, + &datatable.database.resource_path, + ) + .fetch_all(db) + .await?; + if !others.is_empty() { + let others: Vec = others + .into_iter() + .map(|o| format!("{} (data table '{}')", o.workspace_id, o.name)) + .collect(); + return Err(Error::BadRequest(format!( + "Data table permissions cannot be enabled while another workspace reaches the \ + same database: {}. Its members would keep reaching it through that data \ + table's own connection, as every role at once. Remove that data table first.", + others.join(", ") + ))); + } + } Ok(()) } @@ -784,7 +846,8 @@ async fn preview_datatable_permissions( require_admin(authed.is_admin, &authed.username)?; // Refused here too: the preview connects to the database and reads its roles, // and offering a plan that the save will not run is its own kind of wrong. - refuse_enabling_permissions_from_a_fork(&db, &w_id, req.enabled).await?; + refuse_enabling_permissions_over_shared_access(&db, &w_id, &datatable_name, req.enabled) + .await?; let (_client, plan) = build_plan(&db, &w_id, &datatable_name, &req).await?; Ok(Json(DatatablePermissionsPreview { statements: plan.statements.into_iter().map(|s| s.display).collect(), @@ -799,7 +862,6 @@ async fn set_datatable_permissions( Json(req): Json, ) -> Result { require_admin(authed.is_admin, &authed.username)?; - refuse_enabling_permissions_from_a_fork(&db, &w_id, req.enabled).await?; // Reading the config, planning against it, running the plan and persisting // it are one operation: interleaved with another save, or with the removal @@ -808,6 +870,8 @@ async fn set_datatable_permissions( // touching that config takes, so taking it here is what serializes them. let mut tx = db.begin().await?; windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &w_id).await?; + refuse_enabling_permissions_over_shared_access(&db, &w_id, &datatable_name, req.enabled) + .await?; // The roles about to be created are handed privileges by this connection, // which cannot pass on what it holds without the grant option. diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 1b0cd55dd0..3dc1f2a89c 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -8243,6 +8243,14 @@ async fn create_workspace_fork( .await?; } + // Enabling a data table's permissions is refused while the workspace has forks, and it + // checks for them under this same row: a fork still being created has either committed, + // and is found, or copies the parent's settings only after the opt-in landed, so the + // permissioned data table is stripped from it below. After the pairing lock, which is + // the order the workspace rename takes the two in. + windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &parent_workspace_id) + .await?; + let forked_id = nw.id; sqlx::query!( diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index 20e7d6ad5c..bb68c9fad4 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -275,7 +275,7 @@ A fork's data table points either at that workspace's database, where roles created here would be invisible to its own configuration, or at a copy this fork can drop. - Enable permissions there instead — a fork shares them, with the same restrictions. + Enable permissions there instead, once this fork is deleted. {/if}