diff --git a/backend/.sqlx/query-2a391cc1bfcd2f75b46144a394c01237e09c3060da88170f1f6e06468309d213.json b/backend/.sqlx/query-2a391cc1bfcd2f75b46144a394c01237e09c3060da88170f1f6e06468309d213.json new file mode 100644 index 0000000000..a893edfbd7 --- /dev/null +++ b/backend/.sqlx/query-2a391cc1bfcd2f75b46144a394c01237e09c3060da88170f1f6e06468309d213.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE capture_config SET server_id = NULL, last_server_ping = NULL\n WHERE workspace_id = $1 AND trigger_kind = 'postgres'\n AND (trigger_config->>'postgres_resource_path' = $2\n OR trigger_config->>'postgres_resource_path' LIKE $3)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "2a391cc1bfcd2f75b46144a394c01237e09c3060da88170f1f6e06468309d213" +} diff --git a/backend/.sqlx/query-5048e21546f9710697100100e1255ab103979433bc386d7c89d0e30db12bfd57.json b/backend/.sqlx/query-5048e21546f9710697100100e1255ab103979433bc386d7c89d0e30db12bfd57.json new file mode 100644 index 0000000000..804e294296 --- /dev/null +++ b/backend/.sqlx/query-5048e21546f9710697100100e1255ab103979433bc386d7c89d0e30db12bfd57.json @@ -0,0 +1,29 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"datatable!\"\n FROM workspace_settings ws\n CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt\n WHERE dt.value->'reference'->>'workspace_id' = $1\n AND dt.value->'reference'->>'datatable' = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id!", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "datatable!", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + null + ] + }, + "hash": "5048e21546f9710697100100e1255ab103979433bc386d7c89d0e30db12bfd57" +} diff --git a/backend/.sqlx/query-e159b2ff15633f85e839ee4fe1ec2ecd11caf228ea8d0f52ad66def595644250.json b/backend/.sqlx/query-e159b2ff15633f85e839ee4fe1ec2ecd11caf228ea8d0f52ad66def595644250.json new file mode 100644 index 0000000000..3f7245f35f --- /dev/null +++ b/backend/.sqlx/query-e159b2ff15633f85e839ee4fe1ec2ecd11caf228ea8d0f52ad66def595644250.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE postgres_trigger SET server_id = NULL, last_server_ping = NULL\n WHERE workspace_id = $1\n AND (postgres_resource_path = $2 OR postgres_resource_path LIKE $3)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "e159b2ff15633f85e839ee4fe1ec2ecd11caf228ea8d0f52ad66def595644250" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 620de4065e..0b0ca1f71c 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -180,3 +180,83 @@ async fn a_second_entry_on_the_same_database_is_reported_rather_than_governed( ); Ok(()) } + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_resource_backed_data_table_cannot_be_put_under_roles( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + // A role is a login on Windmill's own cluster. A resource-backed data table dials a host the + // workspace admin chose, so accepting one here would hand that host a real cluster credential. + sqlx::query( + r#"UPDATE workspace_settings SET datatable = '{"datatables": {"byo": { + "database": {"resource_type": "postgresql", "resource_path": "u/test-user/pg"}}}}'::jsonb + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + let resp = authed( + client().get(format!("{base}/datatable_permissions/byo")), + "SECRET_TOKEN", + ) + .send() + .await?; + let body: Value = resp.json().await?; + assert_eq!(body["supported"], false, "{body}"); + + let resp = authed( + client().post(format!("{base}/datatable_permissions/byo")), + "SECRET_TOKEN", + ) + .json(&json!({"permissioned": true, "default_role": "role1", + "roles": [{"id": "role1", "tenants": ["*"]}]})) + .send() + .await?; + assert_eq!(resp.status(), 400, "{}", resp.text().await?); + Ok(()) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_fork_renaming_its_own_entry_leaves_the_governing_bookkeeping_alone( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + // The parent's migration definitions. A rename or delete through the fork's settings form + // resolves through the pointer, so without a guard it would relabel or wipe these. + sqlx::query( + "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up) + VALUES ('test-workspace', 'main', 1, 'init', 'SELECT 1')", + ) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/wm-fork-dt/workspaces/edit_datatable_config" + )), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "settings": {"datatables": {}}, + "renames": [], + "deleted_datatables": ["main"] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + let left: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM datatable_migrations WHERE workspace_id = 'test-workspace'", + ) + .fetch_one(&db) + .await?; + assert_eq!(left, 1, "the fork's delete reached the parent's migrations"); + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index 24e6fa74ff..266d5fb0cc 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -42,7 +42,7 @@ use windmill_common::datatable_roles::ADMIN_DATATABLE_ROLE; use windmill_common::worker::SqlAnnotations; use windmill_common::workspaces::{ ensure_can_use_datatable_role, ensure_datatable_admin_access, - get_datatable_resource_from_db_unchecked, DatatableAccess, + get_datatable_resource_from_db_unchecked, resolve_governing_datatable, DatatableAccess, }; use windmill_common::{PgDatabase, DB}; use windmill_git_sync::{ @@ -1500,6 +1500,15 @@ async fn generate_initial_datatable_migration( Extension(db): Extension, Path((w_id, datatable_name)): Path<(String, String)>, ) -> JsonResult { + // Returns a `pg_dump` of the whole schema and writes into the data table's own bookkeeping, so + // it answers to the workspace that governs it rather than to whoever is asking. + ensure_datatable_admin_access( + &db, + &w_id, + &datatable_name, + &DatatableAccess::Authed(authed.to_authed_ref()), + ) + .await?; validate_datatable_path_segment(&datatable_name)?; ensure_datatable_migrations_enabled(&db, &w_id, &datatable_name).await?; @@ -1670,9 +1679,21 @@ pub(crate) struct DatatableRename { pub(crate) to: String, } -async fn resolve_datatable_pg(db: &DB, w_id: &str, datatable: &str) -> Result { +/// The database whose `_wm_migrations` a rename or delete of `datatable` in `w_id` should touch — +/// `None` when that is somebody else's. +/// +/// A fork's entry points at the workspace that governs the data table, so renaming or removing it +/// changes what the fork calls the data table and nothing more. Following the pointer here would +/// let a fork admin relabel or wipe the *governing* workspace's migration bookkeeping through +/// their own settings form, and the parent would then re-run every migration from zero. +async fn resolve_datatable_pg(db: &DB, w_id: &str, datatable: &str) -> Result> { + let governing = resolve_governing_datatable(db, w_id, datatable).await?; + if governing.workspace_id != w_id { + return Ok(None); + } let db_resource = get_datatable_resource_from_db_unchecked(db, w_id, datatable).await?; serde_json::from_value(db_resource) + .map(Some) .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e))) } @@ -1690,7 +1711,9 @@ fn ignore_missing_wm_migrations(e: tokio_postgres::Error) -> Result<()> { /// Drop a data table's rows from its own database's `_wm_migrations`. async fn remote_forget_datatable_migrations(db: &DB, w_id: &str, datatable: &str) -> Result<()> { - let pg_db = resolve_datatable_pg(db, w_id, datatable).await?; + let Some(pg_db) = resolve_datatable_pg(db, w_id, datatable).await? else { + return Ok(()); + }; let (client, connection) = pg_db.connect(Some(db)).await?; tokio::spawn(async move { let _ = connection.await; @@ -1715,7 +1738,9 @@ async fn remote_rename_datatable_migrations( from: &str, to: &str, ) -> Result<()> { - let pg_db = resolve_datatable_pg(db, w_id, resolve_by).await?; + let Some(pg_db) = resolve_datatable_pg(db, w_id, resolve_by).await? else { + return Ok(()); + }; let (client, connection) = pg_db.connect(Some(db)).await?; tokio::spawn(async move { let _ = connection.await; diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 519742ca36..41df4d32bc 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -62,6 +62,9 @@ pub struct DatatableRoleTenantsInfo { #[derive(Serialize)] struct DatatablePermissionsInfo { + /// Whether this data table can be put under roles at all — only one on the instance database + /// can, since a role is a login on that cluster. + supported: bool, /// Whether the data table is under roles at all. permissioned: bool, default_role: String, @@ -205,13 +208,22 @@ async fn get_datatable_permissions( } else { catalog.get(id).map(|r| r.name.clone()) }, - tenants: tenants.tenants.clone(), + // Tenants name users, groups and folders of the governing workspace, so they + // are for the people who set them. Someone reading from a fork gets the shape + // of the decision, not the parent's membership; what they may use themselves + // is what `datatable_usable_roles` answers. + tenants: if editable { + tenants.tenants.clone() + } else { + vec![] + }, }) .collect() }) .unwrap_or_default(); Ok(Json(DatatablePermissionsInfo { + supported: governing.is_instance(), permissioned: permissions.is_some(), default_role: permissions .map(|p| p.default_role().to_string()) @@ -220,14 +232,20 @@ async fn get_datatable_permissions( governing_workspace_id: (governing.workspace_id != w_id) .then(|| governing.workspace_id.clone()), editable, - available_roles: catalog - .iter() - .map(|(id, role)| AvailableRole { - id: id.clone(), - name: role.name.clone(), - enabled: role.enabled, - }) - .collect(), + // The instance's role names are only of use to someone who can pick from them, and + // enumerating them is the first step of anything that wants to name one it shouldn't. + available_roles: if editable { + catalog + .iter() + .map(|(id, role)| AvailableRole { + id: id.clone(), + name: role.name.clone(), + enabled: role.enabled, + }) + .collect() + } else { + vec![] + }, ungoverned_reachers: if editable { ungoverned_reachers(&db, &governing).await? } else { @@ -245,6 +263,17 @@ async fn set_datatable_permissions( let governing = resolve_governing_datatable(&db, &w_id, &datatable_name).await?; ensure_governs_datatable(&db, &authed, &w_id, &governing).await?; + // A data table role is a login on Windmill's own cluster; a resource-backed data table dials a + // host the workspace admin chose, so it has no business naming one. + if req.permissioned && !governing.is_instance() { + return Err(Error::BadRequest(format!( + "Data table '{}' is backed by a Postgres resource. Data table roles are logins on the \ + Windmill instance's own Postgres, so only a data table on the instance database can \ + use them.", + governing.name + ))); + } + let permissions = if req.permissioned { let catalog = read_role_catalog(&db).await?; let mut roles: BTreeMap = BTreeMap::new(); @@ -356,33 +385,54 @@ pub(crate) async fn restart_streams_reaching( db: &DB, governing: &GoverningDatatable, ) -> Result<()> { - let reference = format!("datatable://{}", governing.name); - let prefix = format!("{reference}?%"); - - sqlx::query!( - "UPDATE postgres_trigger SET server_id = NULL, last_server_ping = NULL - WHERE workspace_id = $1 - AND (postgres_resource_path = $2 OR postgres_resource_path LIKE $3)", + // Every workspace holding an entry that resolves here, under the name it calls it: the + // governing one, plus each fork pointing at it. A fork's trigger names its own local entry, so + // filtering on the governing workspace alone would leave its stream running on the connection + // it already opened under the old decision — which is the one window this function exists to + // close. + let mut reached = vec![(governing.workspace_id.clone(), governing.name.clone())]; + let pointers = sqlx::query!( + r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!" + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt + WHERE dt.value->'reference'->>'workspace_id' = $1 + AND dt.value->'reference'->>'datatable' = $2"#, &governing.workspace_id, - &reference, - &prefix, + &governing.name, ) - .execute(db) + .fetch_all(db) .await?; + reached.extend(pointers.into_iter().map(|r| (r.workspace_id, r.datatable))); - // A capture keeps the reference inside its `trigger_config` blob rather than in a column of - // its own, and only a postgres capture has one there at all. - sqlx::query!( - "UPDATE capture_config SET server_id = NULL, last_server_ping = NULL - WHERE workspace_id = $1 AND trigger_kind = 'postgres' - AND (trigger_config->>'postgres_resource_path' = $2 - OR trigger_config->>'postgres_resource_path' LIKE $3)", - &governing.workspace_id, - &reference, - &prefix, - ) - .execute(db) - .await?; + for (w_id, name) in reached { + let reference = format!("datatable://{name}"); + let prefix = format!("{reference}?%"); + + sqlx::query!( + "UPDATE postgres_trigger SET server_id = NULL, last_server_ping = NULL + WHERE workspace_id = $1 + AND (postgres_resource_path = $2 OR postgres_resource_path LIKE $3)", + &w_id, + &reference, + &prefix, + ) + .execute(db) + .await?; + + // A capture keeps the reference inside its `trigger_config` blob rather than in a column + // of its own, and only a postgres capture has one there at all. + sqlx::query!( + "UPDATE capture_config SET server_id = NULL, last_server_ping = NULL + WHERE workspace_id = $1 AND trigger_kind = 'postgres' + AND (trigger_config->>'postgres_resource_path' = $2 + OR trigger_config->>'postgres_resource_path' LIKE $3)", + &w_id, + &reference, + &prefix, + ) + .execute(db) + .await?; + } Ok(()) } diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 23f36b750c..e8c0159679 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -32142,8 +32142,13 @@ components: DatatablePermissions: type: object - required: [permissioned, default_role, roles, editable, available_roles] + required: [supported, permissioned, default_role, roles, editable, available_roles] properties: + supported: + type: boolean + description: >- + Whether this data table can be put under roles at all. Only one backed by the + instance database can: a role is a login on that cluster. permissioned: type: boolean default_role: diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index dbf33cdddc..f9828ca822 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -8145,8 +8145,9 @@ pub async fn run_wait_result_flow_by_version( /// job lives, in particular DuckDB, which runs in-process in the worker. /// /// What it does permit is any statement against the workspace's data tables, writes and DDL -/// included: the helper's body is an unrestricted SQL template and data tables carry no -/// per-user ACL. Narrowing that is a separate decision from this exemption. +/// included: the helper's body is an unrestricted SQL template. What that reaches is the +/// operator's own data table role — the preview job is permissioned as them, so the executor +/// resolves it under their tenancy like any other job. /// /// The database argument is only half the target: the executor honors a `-- database` /// directive in the SQL over it, and `-- s3` redirects the result set, so both are refused. diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 46d4bd0b37..8907c4e8eb 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1371,6 +1371,17 @@ pub struct GoverningDatatable { pub datatable: DataTable, } +impl GoverningDatatable { + /// Backed by the Windmill instance's own Postgres, which is the only substrate data table + /// roles apply to. + pub fn is_instance(&self) -> bool { + self.datatable + .database + .as_ref() + .is_some_and(|d| d.resource_type == DataTableCatalogResourceType::Instance) + } +} + pub async fn resolve_governing_datatable( db: &DB, w_id: &str, @@ -1635,6 +1646,21 @@ pub async fn get_datatable_resource_from_db( let governing = resolve_governing_datatable(db, w_id, name).await?; let mut db_resource = resolve_datatable_connection_unchecked(db, &governing, false).await?; + // A data table role is a login on Windmill's own cluster, so it is only meaningful against an + // instance database. Substituting its password into a resource-backed connection would hand a + // real cluster credential to whatever host that resource names — which a workspace admin + // chooses. Refused rather than ignored: an entry that reached this state was never a shape the + // permissions endpoint accepts, so silently resolving it as admin would hide a broken record. + if !governing.is_instance() { + return match (governing.datatable.permissions.as_ref(), role) { + (None, None | Some(ADMIN_DATATABLE_ROLE)) => Ok(db_resource), + _ => Err(Error::BadRequest(format!( + "Data table '{name}' is backed by a Postgres resource, which cannot be put under \ + data table roles" + ))), + }; + } + let catalog = crate::datatable_roles::read_role_catalog(db).await?; let Some((role_id, tenants)) = datatable_role_entry( governing.datatable.permissions.as_ref(), @@ -1712,6 +1738,9 @@ pub async fn ensure_can_use_datatable_role( context: &str, ) -> Result<()> { let governing = resolve_governing_datatable(db, w_id, name).await?; + if !governing.is_instance() { + return Ok(()); + } let catalog = crate::datatable_roles::read_role_catalog(db).await?; let Some((role_id, tenants)) = datatable_role_entry(governing.datatable.permissions.as_ref(), &catalog, name, role)? @@ -1748,6 +1777,9 @@ pub async fn ensure_datatable_admin_access( access: &DatatableAccess<'_>, ) -> Result<()> { let governing = resolve_governing_datatable(db, w_id, name).await?; + if !governing.is_instance() { + return Ok(()); + } let Some(permissions) = governing.datatable.permissions.as_ref() else { return Ok(()); }; @@ -1865,14 +1897,19 @@ pub async fn rename_datatable_tenant_in_workspace( update_datatable_permissions_in_workspace(tx, w_id, |permissions| { let mut touched = false; for role in permissions.roles.values_mut() { + let mut role_touched = false; for tenant in role.tenants.iter_mut() { if tenant == old { *tenant = new.to_string(); - touched = true; + role_touched = true; } } - if touched { - role.tenants.dedup(); + if role_touched { + // The rename can collide with a name already in the list, and the two need not be + // adjacent — `Vec::dedup` only collapses neighbours, so it would leave the pair. + let mut seen = std::collections::HashSet::new(); + role.tenants.retain(|t| seen.insert(t.clone())); + touched = true; } } touched diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index 9ff4b67d4c..17ba0edd09 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -151,7 +151,13 @@ {loadError} {:else}
- {#if governing} + {#if !info?.supported} + + A data table role is a Postgres login on the Windmill instance's own database, so only a + data table backed by that database can use one. This one is backed by a PostgreSQL + resource — grant access on that server directly. + + {:else if governing} This data table points at the one in workspace {governing}, so its roles are decided there. You are evaluated as a member of that workspace. @@ -176,7 +182,7 @@