From 2718d6846c07ca343cfac4502771edd4195411e8 Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Mon, 21 Sep 2026 09:06:42 +0200 Subject: [PATCH 01/32] feat(datatables): clone a data table under roles with its owners and grants (#11120) Claude-Session: https://claude.ai/code/session_01UbrtwiYNfayrmqouBJHwGV Co-authored-by: Claude Opus 5 (1M context) --- ...522398d2a3ae99d0568b785e63749d6f3225a.json | 15 + ...df6bc09f19cf53b141fac5aefa981463d39fa.json | 29 + ...052b42aaf5c87e5be6333b2df67a81990c8a6.json | 16 + backend/ee-repo-ref.txt | 2 +- .../tests/datatable_roles.rs | 229 ++++++- .../src/datatable_acl.rs | 96 ++- .../src/datatable_clone.rs | 560 ++++++++++++++++ .../src/datatable_replay_oss.rs | 48 ++ backend/windmill-api-workspaces/src/lib.rs | 4 + .../windmill-api-workspaces/src/workspaces.rs | 619 ++++++++++++++---- .../src/workspaces_extra.rs | 30 +- backend/windmill-api/openapi.yaml | 35 +- backend/windmill-common/src/lib.rs | 136 +++- backend/windmill-common/src/workspaces.rs | 142 +++- cli/src/commands/workspace/fork.ts | 24 +- .../lib/components/InstanceSettings.svelte | 14 +- .../datatableAcl/PgAclEditor.svelte | 7 +- .../CreateWorkspaceInner.svelte | 22 +- .../DataTablePermissionsButton.svelte | 6 + .../ForkDatatableSection.svelte | 152 +---- 20 files changed, 1815 insertions(+), 371 deletions(-) create mode 100644 backend/.sqlx/query-d0548225d92e6e7d0eb9a0227a6522398d2a3ae99d0568b785e63749d6f3225a.json create mode 100644 backend/.sqlx/query-e524e89440004953cc0b1cb57b8df6bc09f19cf53b141fac5aefa981463d39fa.json create mode 100644 backend/.sqlx/query-f7ece5036ad92e485b5e15a70e5052b42aaf5c87e5be6333b2df67a81990c8a6.json create mode 100644 backend/windmill-api-workspaces/src/datatable_clone.rs create mode 100644 backend/windmill-api-workspaces/src/datatable_replay_oss.rs diff --git a/backend/.sqlx/query-d0548225d92e6e7d0eb9a0227a6522398d2a3ae99d0568b785e63749d6f3225a.json b/backend/.sqlx/query-d0548225d92e6e7d0eb9a0227a6522398d2a3ae99d0568b785e63749d6f3225a.json new file mode 100644 index 0000000000..2ff2722f77 --- /dev/null +++ b/backend/.sqlx/query-d0548225d92e6e7d0eb9a0227a6522398d2a3ae99d0568b785e63749d6f3225a.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings ws\n SET datatable = (\n SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg(\n dt.key,\n (SELECT CASE WHEN r.v->'governed_by'->>'workspace_id' = $2\n THEN jsonb_set(r.v, '{governed_by,workspace_id}', to_jsonb($1::text))\n ELSE r.v END\n FROM (SELECT CASE WHEN dt.value->'reference'->>'workspace_id' = $2\n THEN jsonb_set(dt.value, '{reference,workspace_id}', to_jsonb($1::text))\n ELSE dt.value END AS v) r)\n ))\n FROM jsonb_each(ws.datatable->'datatables') dt\n )\n WHERE jsonb_typeof(ws.datatable->'datatables') = 'object'\n AND (ws.datatable::text LIKE '%\"reference\"%'\n OR ws.datatable::text LIKE '%\"governed_by\"%')", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "d0548225d92e6e7d0eb9a0227a6522398d2a3ae99d0568b785e63749d6f3225a" +} diff --git a/backend/.sqlx/query-e524e89440004953cc0b1cb57b8df6bc09f19cf53b141fac5aefa981463d39fa.json b/backend/.sqlx/query-e524e89440004953cc0b1cb57b8df6bc09f19cf53b141fac5aefa981463d39fa.json new file mode 100644 index 0000000000..af8d0eb1ab --- /dev/null +++ b/backend/.sqlx/query-e524e89440004953cc0b1cb57b8df6bc09f19cf53b141fac5aefa981463d39fa.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 EXISTS (\n SELECT 1 FROM (VALUES ('reference'), ('governed_by')) k(link)\n WHERE dt.value->k.link->>'workspace_id' = $1\n AND dt.value->k.link->>'datatable' = $2\n )", + "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": "e524e89440004953cc0b1cb57b8df6bc09f19cf53b141fac5aefa981463d39fa" +} diff --git a/backend/.sqlx/query-f7ece5036ad92e485b5e15a70e5052b42aaf5c87e5be6333b2df67a81990c8a6.json b/backend/.sqlx/query-f7ece5036ad92e485b5e15a70e5052b42aaf5c87e5be6333b2df67a81990c8a6.json new file mode 100644 index 0000000000..1be51feb85 --- /dev/null +++ b/backend/.sqlx/query-f7ece5036ad92e485b5e15a70e5052b42aaf5c87e5be6333b2df67a81990c8a6.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings ws\n SET datatable = (\n SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg(\n dt.key,\n (SELECT CASE WHEN r.v->'governed_by'->>'workspace_id' = $1\n AND r.v->'governed_by'->>'datatable' = $2\n THEN jsonb_set(r.v, '{governed_by,datatable}', to_jsonb($3::text))\n ELSE r.v END\n FROM (SELECT CASE WHEN dt.value->'reference'->>'workspace_id' = $1\n AND dt.value->'reference'->>'datatable' = $2\n THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text))\n ELSE dt.value END AS v) r)\n ))\n FROM jsonb_each(ws.datatable->'datatables') dt\n )\n WHERE EXISTS (\n SELECT 1 FROM jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) d,\n LATERAL (VALUES ('reference'), ('governed_by')) k(link)\n WHERE d.value->k.link->>'workspace_id' = $1\n AND d.value->k.link->>'datatable' = $2\n )", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "f7ece5036ad92e485b5e15a70e5052b42aaf5c87e5be6333b2df67a81990c8a6" +} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index d5f3b9c086..d039bd8b4a 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -875dbc26f75743115a244fe22085290fc14635ff +3145e422d61d580f0a82804f075285c112879da0 diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index d2f6ca325e..d1b4501735 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -627,21 +627,234 @@ async fn a_rename_has_to_match_the_save_it_claims_to_describe( Ok(()) } +async fn database_exists(db: &Pool, name: &str) -> anyhow::Result { + Ok( + sqlx::query_scalar("SELECT EXISTS (SELECT 1 FROM pg_database WHERE datname = $1)") + .bind(name) + .fetch_one(db) + .await?, + ) +} + +async fn workspace_exists(db: &Pool, id: &str) -> anyhow::Result { + Ok( + sqlx::query_scalar("SELECT EXISTS (SELECT 1 FROM workspace WHERE id = $1)") + .bind(id) + .fetch_one(db) + .await?, + ) +} + #[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] -async fn a_data_table_under_roles_is_not_copied_into_a_fork( +async fn a_data_table_under_roles_is_cloned_only_with_its_grants( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + // The database a fork's copy of `main` goes into is named after the fork. Every request below + // is refused before it is created, so the cluster-wide name never collides with a sibling run. + let target = "wm_fork_copy__main"; + let fork = |token: &str, forked: Value| { + authed( + client().post(format!( + "http://localhost:{port}/api/w/test-workspace/workspaces/create_fork" + )), + token, + ) + .json(&json!({"id": "wm-fork-copy", "name": "copy", "forked_datatables": [forked]})) + }; + + // Rows are a workspace admin's to copy, as they were before roles. + let resp = fork( + "SECRET_TOKEN_2", + json!({"name": "main", "new_dbname": target, "fork_behavior": "schema_and_data"}), + ) + .send() + .await?; + assert_eq!(resp.status(), 403, "{}", resp.text().await?); + assert!(!database_exists(&db, target).await?); + + // Even the schema alone lists every table, which a member covered by no role cannot read in + // the parent. + let resp = fork( + "SECRET_TOKEN_3", + json!({"name": "main", "new_dbname": target, "fork_behavior": "schema_only"}), + ) + .send() + .await?; + assert_eq!(resp.status(), 401, "{}", resp.text().await?); + assert!(!database_exists(&db, target).await?); + + // Refused in the first phase too, before a git branch is created for a fork that cannot be. + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/test-workspace/workspaces/create_workspace_fork_branch" + )), + "SECRET_TOKEN_3", + ) + .json( + &json!({"id": "wm-fork-copy", "name": "copy", "forked_datatables": [ + {"name": "main", "new_dbname": target, "fork_behavior": "schema_only"} + ]}), + ) + .send() + .await?; + assert_eq!(resp.status(), 401, "{}", resp.text().await?); + + // A database the request did not create — whatever the data table, under roles or not — may be + // a copy left behind by a deleted fork, which the entry would reach without its governance. + let resp = fork( + "SECRET_TOKEN", + json!({"name": "other", "new_dbname": "wm_fork_copy__other"}), + ) + .send() + .await?; + assert_eq!(resp.status(), 400); + assert!( + resp.text().await?.contains("fork_behavior"), + "a database the fork did not create was taken" + ); + assert!(!workspace_exists(&db, "wm-fork-copy").await?); + + // The copy an admin asks for goes ahead in an edition that replays grants, and is refused + // before any database exists in one that does not. The fixture's database does not exist, so + // the dump fails either way — and leaves neither a database nor a fork behind. + let resp = fork( + "SECRET_TOKEN", + json!({"name": "main", "new_dbname": target, "fork_behavior": "schema_only"}), + ) + .send() + .await?; + assert_ne!(resp.status(), 200); + let body = resp.text().await?; + #[cfg(all(feature = "private", feature = "enterprise"))] + assert!(body.contains("pg_dump"), "{body}"); + #[cfg(not(all(feature = "private", feature = "enterprise")))] + assert!(body.contains("Enterprise Edition"), "{body}"); + assert!(!database_exists(&db, target).await?); + assert!(!workspace_exists(&db, "wm-fork-copy").await?); + Ok(()) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_clone_takes_its_roles_from_the_data_table_it_was_cloned_from( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + sqlx::query( + r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, '{datatables,copy}', '{ + "database": {"resource_type": "instance", "resource_path": "wm_fork_dt__copy"}, + "governed_by": {"workspace_id": "test-workspace", "datatable": "main"}, + "forked_from": {} + }'::jsonb) WHERE workspace_id = 'wm-fork-dt'"#, + ) + .execute(&db) + .await?; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let fork = format!("http://localhost:{port}/api/w/wm-fork-dt/workspaces"); + + // `test-user-2` administers the fork, and is only a tenant of `analytics` in the parent. + let resp = authed( + client().get(format!("{fork}/datatable_usable_roles/copy")), + "SECRET_TOKEN_2", + ) + .send() + .await?; + let body: Value = resp.json().await?; + assert_eq!(body["roles"], json!(["analytics"]), "{body}"); + + let resp = authed( + client().get(format!("{fork}/datatable_permissions/copy")), + "SECRET_TOKEN_2", + ) + .send() + .await?; + let body: Value = resp.json().await?; + assert_eq!(body["editable"], false, "{body}"); + assert_eq!(body["clone_of"]["workspace_id"], "test-workspace", "{body}"); + + // Nobody changes a clone's roles, a superadmin included: they are the source's. + for token in ["SECRET_TOKEN_2", "SECRET_TOKEN"] { + let resp = authed( + client().post(format!("{fork}/datatable_permissions/copy")), + token, + ) + .json(&json!({"permissioned": false})) + .send() + .await?; + assert_eq!(resp.status(), 400, "{token} changed a clone's roles"); + } + + // A settings save cannot clear the link. + let resp = authed( + client().post(format!("{fork}/edit_datatable_config")), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "settings": {"datatables": {"main": {}, "copy": { + "database": {"resource_type": "instance", "resource_path": "wm_fork_dt__copy"} + }}}, + "renames": [], "deleted_datatables": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let governed_by: Option = sqlx::query_scalar( + "SELECT datatable->'datatables'->'copy'->'governed_by' FROM workspace_settings + WHERE workspace_id = 'wm-fork-dt'", + ) + .fetch_one(&db) + .await?; + assert_eq!(governed_by.unwrap()["datatable"], "main"); + + // Nor move it, a superadmin included: its grants were replayed into that database alone. + let resp = authed( + client().post(format!("{fork}/edit_datatable_config")), + "SECRET_TOKEN", + ) + .json(&json!({ + "settings": {"datatables": {"main": {}, "copy": { + "database": {"resource_type": "instance", "resource_path": "wm_fork_dt__elsewhere"} + }}}, + "renames": [], "deleted_datatables": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 400, "{}", resp.text().await?); + + // Nor reach its database through a second entry without roles, which would connect as `admin`. + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/test-workspace/workspaces/edit_datatable_config" + )), + "SECRET_TOKEN", + ) + .json(&json!({ + "settings": {"datatables": { + "main": {"database": {"resource_type": "instance", "resource_path": "dt_main"}}, + "alias": {"database": {"resource_type": "instance", "resource_path": "wm_fork_dt__copy"}} + }}, + "renames": [], "deleted_datatables": [] + })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 400, "{body}"); + assert!(body.contains("without carrying those roles"), "{body}"); + Ok(()) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_data_table_under_roles_is_not_copied_without_its_grants( db: Pool, ) -> anyhow::Result<()> { initialize_tracing().await; - // `pg_dump` carries no roles and the restore drops ACLs, so a copy would arrive with the - // parent's tenants and none of the grants behind them: every role but admin denied by - // Postgres in a data table that reads as configured. Refuse the copy rather than ship that, - // and refuse it before any data moves. let server = ApiServer::start(db.clone()).await?; let port = server.addr.port(); - // Both halves of the clone: the database the copy would land in, then the copy itself. The - // first has to refuse too, or a permissioned fork leaves an empty registered database that - // no data table entry names and nothing collects. let resp = authed( client().post(format!( "http://localhost:{port}/api/w/test-workspace/workspaces/create_pg_database" diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 6bd14316fc..04dc3d4e80 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -245,6 +245,8 @@ pub struct DatatableAclInfo { /// Whether this caller may plan and apply changes: they administer the data table, on an /// edition that has the planner. pub editable: bool, + /// Whether the data table is a clone, whose grants stay as they were copied from its source. + pub clone: bool, /// Whether the server is Postgres 17 or later, which added the `MAINTAIN` table privilege. pub supports_maintain: bool, /// The database the target lives in, which no target carries itself. @@ -335,13 +337,40 @@ async fn connect_as_admin_unchecked( pg.user = Some(CUSTOM_INSTANCE_USER.to_string()); pg.password = Some(windmill_common::utils::get_custom_pg_instance_password(db).await?); let dbname = pg.dbname.clone(); - let (client, mut connection) = pg.connect(Some(db)).await?; + let (client, notices) = connect_with_notices(db, &pg).await?; + Ok((client, notices, dbname)) +} + +/// A connection to `pg`, and the notices Postgres sends on it — which is where a grant or revoke +/// that changed nothing is reported ([`execute_acl_statements`]). +pub(crate) async fn connect_with_notices( + db: &DB, + pg: &PgDatabase, +) -> Result<(tokio_postgres::Client, mpsc::UnboundedReceiver)> { + let (client, connection) = pg.connect(Some(db)).await?; + Ok(( + client, + drive_with_notices(connection, windmill_common::TokioPgConnection::poll_message), + )) +} + +type PollMessage = + fn( + &mut C, + &mut std::task::Context<'_>, + ) -> std::task::Poll>>; + +/// Drive `connection` in the background with `poll`, forwarding its notices. +pub(crate) fn drive_with_notices( + mut connection: C, + poll: PollMessage, +) -> mpsc::UnboundedReceiver { // Unbounded: the driver must never wait on the receiver, which only drains once the statement // the driver is carrying has completed. let (notices_tx, notices) = mpsc::unbounded_channel(); tokio::spawn(async move { loop { - match std::future::poll_fn(|cx| connection.poll_message(cx)).await { + match std::future::poll_fn(|cx| poll(&mut connection, cx)).await { Some(Ok(AsyncMessage::Notice(notice))) => { let _ = notices_tx.send(notice); } @@ -354,7 +383,41 @@ async fn connect_as_admin_unchecked( } } }); - Ok((client, notices, dbname)) + notices +} + +/// Run `statements` in order on `tx`, failing on the first that errors or that Postgres only warns +/// about. A privilege the connection cannot pass on is a warning to Postgres (`01007` / `01006`), +/// which then carries on having changed nothing; returning drops the transaction, rolling back +/// everything before it. +/// +/// Authorization: none. Runs `statements` as the connection `tx` is on; callers MUST have authorized +/// changing that database's access, and built the statements themselves. +pub(crate) async fn execute_acl_statements( + tx: &tokio_postgres::Transaction<'_>, + notices: &mut mpsc::UnboundedReceiver, + statements: &[String], +) -> Result<()> { + while notices.try_recv().is_ok() {} + for statement in statements { + tx.batch_execute(statement).await.map_err(|e| { + Error::ExecutionErr(format!( + "Failed to run `{statement}`: {}", + pg_error_message(&e) + )) + })?; + while let Ok(notice) = notices.try_recv() { + if *notice.code() == SqlState::WARNING_PRIVILEGE_NOT_GRANTED + || *notice.code() == SqlState::WARNING_PRIVILEGE_NOT_REVOKED + { + return Err(Error::ExecutionErr(format!( + "`{statement}` did not take effect ({}), so nothing was applied", + notice.message() + ))); + } + } + } + Ok(()) } /// An object whose ownership follows the schema's. @@ -399,10 +462,12 @@ macro_rules! schema_owned_objects { AND x.refobjsubid <> 0)))" }; } +#[allow(unused_imports)] +pub(crate) use schema_owned_objects; /// The keyword `ALTER ... OWNER TO` takes for a kind of object, as `pg_identify_object` names the /// kind. A kind missing here is refused rather than skipped, which would leave it behind. -fn owned_keyword(kind: &str) -> Option<&'static str> { +pub(crate) fn owned_keyword(kind: &str) -> Option<&'static str> { Some(match kind { "table" => "TABLE", "view" => "VIEW", @@ -1054,6 +1119,7 @@ async fn get_datatable_acl( owner: role_name_of(&owner), roles, editable, + clone: governing.governor.is_some(), supports_maintain, dbname, grants, @@ -1625,27 +1691,7 @@ async fn apply_datatable_acl( pg_error_message(&e) )) })?; - for statement in &plan.statements { - pg_tx.batch_execute(statement).await.map_err(|e| { - Error::ExecutionErr(format!( - "Failed to run `{statement}`: {}", - pg_error_message(&e) - )) - })?; - // A privilege the connection cannot pass on is only a warning to Postgres, which then - // carries on having changed nothing. Returning drops the transaction, rolling back - // everything before it. - while let Ok(notice) = notices.try_recv() { - if *notice.code() == SqlState::WARNING_PRIVILEGE_NOT_GRANTED - || *notice.code() == SqlState::WARNING_PRIVILEGE_NOT_REVOKED - { - return Err(Error::ExecutionErr(format!( - "`{statement}` did not take effect ({}), so nothing was applied", - notice.message() - ))); - } - } - } + execute_acl_statements(&pg_tx, &mut notices, &plan.statements).await?; // A schema's objects were listed before the transaction opened; one committed since would stay // with its old owner. One created while this transaction is still open can still slip past, as diff --git a/backend/windmill-api-workspaces/src/datatable_clone.rs b/backend/windmill-api-workspaces/src/datatable_clone.rs new file mode 100644 index 0000000000..be728782af --- /dev/null +++ b/backend/windmill-api-workspaces/src/datatable_clone.rs @@ -0,0 +1,560 @@ +/* + * Author: Ruben Fiszel + * Copyright: Windmill Labs, Inc 2022 + * This file and its contents are licensed under the AGPLv3 License. + * Please see the included NOTICE for copyright information and + * LICENSE-AGPL for a copy of the license. + */ + +//! Copying a data table's database for the fork being created. +//! +//! The fork request makes its copies before it writes the fork: each is created, filled and — for a +//! data table under roles — given the source's owners and grants. What each copy was made from +//! stays in the request ([`MadeCopy`]), so the fork's transaction checks it against the source as it +//! is then, and a fork that fails drops the instance copies it made ([`drop_copies_after`]) and names +//! the others, which live on servers of the workspace's own. + +use std::collections::BTreeSet; + +use windmill_api_auth::ApiAuthed; +use windmill_common::datatable_roles::{lock_role_catalog, read_role_catalog_tx}; +use windmill_common::error::{pg_error_message, Error, Result}; +use windmill_common::utils::require_admin; +use windmill_common::worker::CLOUD_HOSTED; +use windmill_common::workspaces::{ + get_datatable_resource_from_db_unchecked, DataTableDatabase, DataTableForkBehavior, + GoverningDatatable, +}; +use windmill_common::{PgDatabase, DB}; + +use crate::datatable_acl::connect_with_notices; +use crate::datatable_permissions::ensure_reaches_governing_datatable; +use crate::workspaces::{ + create_database_on_server, ensure_datatable_is_clonable, pg_dump_database, pg_import_dump, + DumpFile, PgDumpOptions, +}; + +/// A database this request created and filled for one data table of the fork. +pub(crate) struct MadeCopy { + /// The data table's name, in the parent and in the fork. + pub(crate) name: String, + pub(crate) dbname: String, + pub(crate) behavior: DataTableForkBehavior, + /// The database the source resolved to when it was copied. + pub(crate) source_database: DataTableDatabase, + /// Whether the source's owners and grants were replayed into the copy: it was under roles. + pub(crate) replayed: bool, + /// For a resource-backed copy, the resource and variables its connection was resolved from. + pub(crate) connection: Option, +} + +/// What a resource-backed data table's connection is resolved from: its resource, and every +/// resource and variable that one references, as stored — and, for a secret kept in an external +/// backend, the value that backend holds. The connection is resolved from the snapshot itself +/// ([`ConnectionSnapshot::resolve`]), so it is exactly the one these rows describe. +#[derive(PartialEq)] +pub(crate) struct ConnectionSnapshot { + root: String, + resources: std::collections::BTreeMap>, + /// Stored value and whether it is secret. + variables: std::collections::BTreeMap>, + external_secrets: std::collections::BTreeMap, +} + +/// Read the [`ConnectionSnapshot`] of resource `resource_path` in workspace `w_id`. +/// +/// Authorization: none, and it holds secret values. Callers MUST have authorized using that +/// resource, and never disclose the snapshot or what it resolves to. +pub(crate) async fn connection_snapshot( + db: &DB, + conn: &mut sqlx::PgConnection, + w_id: &str, + resource_path: &str, +) -> Result { + let root = resource_path.trim_start_matches("$res:").to_string(); + let mut snapshot = ConnectionSnapshot { + root: root.clone(), + resources: Default::default(), + variables: Default::default(), + external_secrets: Default::default(), + }; + let mut pending = vec![root]; + while let Some(path) = pending.pop() { + if snapshot.resources.contains_key(&path) { + continue; + } + let value: Option = sqlx::query_scalar( + "SELECT value FROM resource WHERE workspace_id = $1 AND path = $2", + ) + .bind(w_id) + .bind(&path) + .fetch_optional(&mut *conn) + .await? + .flatten(); + let mut strings = vec![]; + collect_strings(value.as_ref(), &mut strings); + for reference in strings { + if let Some(var) = reference.strip_prefix("$var:") { + if snapshot.variables.contains_key(var) { + continue; + } + let row: Option<(String, bool)> = sqlx::query_as( + "SELECT value, is_secret FROM variable WHERE workspace_id = $1 AND path = $2", + ) + .bind(w_id) + .bind(var) + .fetch_optional(&mut *conn) + .await?; + if let Some((stored, true)) = &row { + if windmill_common::secret_backend::is_external_stored_value(stored) { + let secret = windmill_common::secret_backend::get_secret_value( + db, w_id, var, stored, + ) + .await?; + snapshot.external_secrets.insert(var.to_string(), secret); + } + } + snapshot.variables.insert(var.to_string(), row); + } else if let Some(res) = reference.strip_prefix("$res:") { + pending.push(res.to_string()); + } + } + snapshot.resources.insert(path, value); + } + Ok(snapshot) +} + +impl ConnectionSnapshot { + /// The connection these rows resolve to, as the data table's own resolution would: references + /// substituted, secrets decrypted. + pub(crate) async fn resolve(&self, db: &DB, w_id: &str) -> Result { + let root = self.resource(&self.root)?; + self.substitute(db, w_id, root, 0).await + } + + /// Whether the resource itself holds the connection's fields, which the fork repoints by setting + /// its `dbname`, rather than being a reference to another resource. + pub(crate) fn root_holds_connection(&self) -> bool { + self.resource(&self.root).is_ok_and(|v| v.is_object()) + } + + fn resource(&self, path: &str) -> Result<&serde_json::Value> { + self.resources + .get(path) + .and_then(|v| v.as_ref()) + .ok_or_else(|| Error::NotFound(format!("resource {path} does not exist"))) + } + + fn substitute<'a>( + &'a self, + db: &'a DB, + w_id: &'a str, + value: &'a serde_json::Value, + depth: usize, + ) -> std::pin::Pin> + Send + 'a>> + { + Box::pin(async move { + if depth > 32 { + return Err(Error::BadRequest("resource references nest too deeply".to_string())); + } + Ok(match value { + serde_json::Value::Object(map) => { + let mut out = serde_json::Map::new(); + for (key, val) in map { + out.insert(key.clone(), self.substitute(db, w_id, val, depth).await?); + } + serde_json::Value::Object(out) + } + serde_json::Value::Array(items) => { + let mut out = vec![]; + for val in items { + out.push(self.substitute(db, w_id, val, depth).await?); + } + serde_json::Value::Array(out) + } + serde_json::Value::String(s) if s.starts_with("$res:") => { + let path = &s[5..]; + self.substitute(db, w_id, self.resource(path)?, depth + 1).await? + } + serde_json::Value::String(s) if s.starts_with("$var:") => { + let path = &s[5..]; + let (stored, secret) = self + .variables + .get(path) + .and_then(|v| v.as_ref()) + .ok_or_else(|| Error::NotFound(format!("variable {path} does not exist")))?; + serde_json::Value::String(match (secret, self.external_secrets.get(path)) { + (false, _) => stored.clone(), + (true, Some(external)) => external.clone(), + (true, None) => windmill_common::variables::decrypt( + &windmill_common::variables::build_crypt(db, w_id).await?, + stored.clone(), + ) + .map_err(|e| { + Error::internal_err(format!("Error decrypting variable {s}: {e}")) + })?, + }) + } + other => other.clone(), + }) + }) + } +} + +fn collect_strings<'a>(value: Option<&'a serde_json::Value>, out: &mut Vec<&'a str>) { + match value { + Some(serde_json::Value::String(s)) => out.push(s), + Some(serde_json::Value::Array(items)) => { + items.iter().for_each(|v| collect_strings(Some(v), out)) + } + Some(serde_json::Value::Object(map)) => { + map.values().for_each(|v| collect_strings(Some(v), out)) + } + _ => {} + } +} + +/// What a data table of the fork should be copied as. +pub(crate) struct CopyRequest<'a> { + pub(crate) name: &'a str, + pub(crate) dbname: &'a str, + pub(crate) behavior: DataTableForkBehavior, +} + +/// Check that `authed` may copy each data table of `parent_w_id` as asked, before anything is +/// created. +/// +/// Who may copy is what it was before data table roles — anyone for the schema, an admin of the +/// workspace for the rows — narrowed under roles to whoever may connect as one of them: even the +/// schema alone lists every table, which under roles only role holders can read in the parent. +pub(crate) async fn authorize_copies( + db: &DB, + authed: &ApiAuthed, + parent_w_id: &str, + requests: &[CopyRequest<'_>], +) -> Result<()> { + for request in requests { + match request.behavior { + DataTableForkBehavior::KeepOriginal => { + return Err(Error::BadRequest(format!( + "Data table '{}' is kept, which copies nothing", + request.name + ))) + } + DataTableForkBehavior::SchemaOnly => {} + DataTableForkBehavior::SchemaAndData => { + require_admin(authed.is_admin, &authed.username)?; + if *CLOUD_HOSTED { + return Err(Error::BadRequest( + "Cloning schema and data is not available on cloud".to_string(), + )); + } + } + } + windmill_common::validate_dbname(request.dbname)?; + if !request.dbname.starts_with("wm_fork_") { + return Err(Error::BadRequest(format!( + "Forked datatable database name '{}' must start with 'wm_fork_'", + request.dbname + ))); + } + let governing = ensure_datatable_is_clonable(db, parent_w_id, request.name).await?; + ensure_reaches_governing_datatable(db, parent_w_id, request.name, &governing, authed) + .await?; + if governing.datatable.permissions.is_some() { + crate::datatable_replay_oss::ensure_replay()?; + } + } + Ok(()) +} + +/// Make every copy in `requests`, in order, once [`authorize_copies`] allows them all. When one +/// fails, the copies already made are dropped before the error is returned. +pub(crate) async fn make_copies( + db: &DB, + authed: &ApiAuthed, + parent_w_id: &str, + requests: &[CopyRequest<'_>], +) -> Result> { + authorize_copies(db, authed, parent_w_id, requests).await?; + let mut copies = Vec::with_capacity(requests.len()); + for request in requests { + match make_copy(db, authed, parent_w_id, request).await { + Ok(copy) => copies.push(copy), + Err(e) => return Err(drop_copies_after(db, copies, e).await), + } + } + Ok(copies) +} + +/// Drop every copy, returning `error` — with what could not be dropped appended, since nothing +/// else will name those databases again. +pub(crate) async fn drop_copies_after(db: &DB, copies: Vec, error: Error) -> Error { + let mut stranded = Vec::new(); + for copy in copies { + if let Err(e) = drop_copy(db, ©.source_database, ©.dbname).await { + tracing::error!("Could not drop '{}' after a failed fork: {e}", copy.dbname); + stranded.push(format!("'{}' ({e})", copy.dbname)); + } + } + if stranded.is_empty() { + error + } else { + Error::ExecutionErr(format!( + "{error}. These databases created for the fork could not be dropped: {}", + stranded.join(", ") + )) + } +} + +async fn drop_copy(db: &DB, source_database: &DataTableDatabase, dbname: &str) -> Result<()> { + if source_database.resource_type + == windmill_common::workspaces::DataTableCatalogResourceType::Instance + { + let users = windmill_common::drop_unused_instance_database(db, dbname).await?; + if users.is_empty() { + Ok(()) + } else { + Err(Error::BadRequest(format!( + "kept, since workspaces {} now use it", + users.join(", ") + ))) + } + } else { + // On a server of the workspace's own, where a resource edited meanwhile can already name it + // and nothing locks such an edit: dropping it could take someone's data. + Err(Error::BadRequest( + "kept on its PostgreSQL server, where it may already be in use; drop it there once it \ + is not, before forking the same data table under this id again" + .to_string(), + )) + } +} + +async fn make_copy( + db: &DB, + authed: &ApiAuthed, + parent_w_id: &str, + request: &CopyRequest<'_>, +) -> Result { + let governing = ensure_datatable_is_clonable(db, parent_w_id, request.name).await?; + let source_database = governing.datatable.database.clone().ok_or_else(|| { + Error::internal_err(format!( + "Data table '{}' resolves to an entry that owns no database", + request.name + )) + })?; + let is_instance = governing.is_instance(); + // Resolved from the snapshot, not read again: the connection the copy is made on is then + // exactly what these rows describe, which the fork's own clone of them is checked against. + let (server, connection): (PgDatabase, Option) = if is_instance { + let server = serde_json::from_value( + get_datatable_resource_from_db_unchecked(db, parent_w_id, request.name).await?, + ) + .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?; + (server, None) + } else { + let snapshot = connection_snapshot( + db, + &mut *db.acquire().await?, + &governing.workspace_id, + &source_database.resource_path, + ) + .await?; + if !snapshot.root_holds_connection() { + return Err(Error::BadRequest(format!( + "Data table '{}' uses resource '{}', which only refers to another resource: the \ + fork's copy of it could not be pointed at the copied database. Point the data \ + table at the resource holding the connection, then fork again.", + request.name, source_database.resource_path + ))); + } + let server = serde_json::from_value(snapshot.resolve(db, &governing.workspace_id).await?) + .map_err(|e| { + Error::internal_err(format!("Failed to parse database credentials: {e}")) + })?; + (server, Some(snapshot)) + }; + + // Dumped before the database exists, so a source that cannot be read leaves nothing behind. + // Ownership never carries over: the restore runs as the target's connection user. Grants do, + // except on the instance, where the replay below is what reproduces them. + let dump = pg_dump_database( + &server, + PgDumpOptions { + schema_only: request.behavior == DataTableForkBehavior::SchemaOnly, + no_owner: true, + no_acl: is_instance, + ..Default::default() + }, + ) + .await?; + + if is_instance { + windmill_common::create_custom_instance_database(db, request.dbname, "datatable").await?; + } else { + create_database_on_server(db, &server, request.dbname).await?; + } + + let target = PgDatabase { dbname: request.dbname.to_string(), ..server.clone() }; + let filled = fill( + db, + authed, + parent_w_id, + request.name, + &governing, + &server, + &target, + &dump, + ) + .await; + match filled { + Ok(replayed) => Ok(MadeCopy { + name: request.name.to_string(), + dbname: request.dbname.to_string(), + behavior: request.behavior, + source_database, + replayed, + connection, + }), + Err(e) => { + if let Err(drop_err) = drop_copy(db, &source_database, request.dbname).await { + tracing::error!( + "Could not drop '{}' after a failed copy of data table '{}': {drop_err}", + request.dbname, + request.name + ); + return Err(Error::ExecutionErr(format!( + "{e}. The database '{}' created for the copy could not be dropped: {drop_err}", + request.dbname + ))); + } + Err(e) + } + } +} + +/// Restore the dump into the new database and, for a data table under roles, replay the source's +/// owners and grants into it. Returns whether it replayed. +#[allow(clippy::too_many_arguments)] +async fn fill( + db: &DB, + authed: &ApiAuthed, + parent_w_id: &str, + name: &str, + governing: &GoverningDatatable, + source: &PgDatabase, + target: &PgDatabase, + dump: &DumpFile, +) -> Result { + pg_import_dump(target, dump).await?; + + // Held until the replay commits: a role renamed or dropped meanwhile would change what the + // replay names, and a settings save could move the source onto another database or put it under + // roles. Taken in the same order as the permissions save and the ACL apply. + // On a connection of its own: the checks below take theirs from the pool, and a lock holder + // drawn from the pool too could leave a small one with nothing to give them. + let database_url = windmill_common::get_database_url().await?; + let mut lock_holder = + ::connect_with(&database_url.connect_options().await?) + .await + .map_err(|e| Error::internal_err(format!("Failed to connect to the database: {e}")))?; + let mut tx = sqlx::Connection::begin(&mut lock_holder).await?; + lock_role_catalog(&mut tx).await?; + lock_settings_rows(&mut tx, parent_w_id, name).await?; + + // Everything so far was decided before the locks. + let now = ensure_datatable_is_clonable(db, parent_w_id, name).await?; + ensure_reaches_governing_datatable(db, parent_w_id, name, &now, authed).await?; + if !same_database(&now.datatable.database, &governing.datatable.database) { + return Err(Error::BadRequest(format!( + "Data table '{name}' moved to another database while it was being copied; fork again" + ))); + } + + let replayed = now.datatable.permissions.is_some(); + if replayed { + crate::datatable_replay_oss::ensure_replay()?; + let catalog = read_role_catalog_tx(&mut tx).await?; + // The replay leaves `CONNECT` to the catalog, and creating the database only tried to set + // it: a copy `PUBLIC` could still connect to would admit logins the source turns away. + windmill_common::datatable_roles::converge_connect_grants_with( + db, + &target.dbname, + &catalog, + ) + .await?; + let catalog_roles: BTreeSet = catalog.values().map(|r| r.name.clone()).collect(); + let (source, _source_notices) = connect_with_notices(db, source).await?; + let (mut target, mut notices) = connect_with_notices(db, target).await?; + // One transaction on the copy: a replay that stops halfway leaves objects owned by one role + // and granted as another. + let pg_tx = target.transaction().await.map_err(|e| { + Error::internal_err(format!( + "Failed to open a transaction on the copy: {}", + pg_error_message(&e) + )) + })?; + crate::datatable_replay_oss::replay_owners_and_grants( + &source, + &pg_tx, + &mut notices, + &catalog_roles, + ) + .await?; + pg_tx.commit().await.map_err(|e| { + Error::internal_err(format!( + "Failed to commit the replayed grants: {}", + pg_error_message(&e) + )) + })?; + } + tx.commit().await?; + Ok(replayed) +} + +/// Lock every settings row the resolution of data table `name` of `start_w_id` passes through, in +/// the order it passes them — each pointer, the entry that owns the database, and each workspace its +/// roles come from — and return them. Deleting or repointing any of them would leave a copy made +/// for the resolution answering for another one. +pub(crate) async fn lock_settings_rows( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + start_w_id: &str, + name: &str, +) -> Result> { + let mut path: Vec<(String, String)> = vec![]; + let (mut w_id, mut datatable) = (start_w_id.to_string(), name.to_string()); + loop { + if path.contains(&(w_id.clone(), datatable.clone())) || path.len() > 32 { + return Err(Error::BadRequest(format!( + "Data table '{name}' of workspace '{start_w_id}' resolves through a cycle" + ))); + } + let entry: Option = sqlx::query_scalar( + "SELECT datatable->'datatables'->$2 FROM workspace_settings + WHERE workspace_id = $1 FOR UPDATE", + ) + .bind(&w_id) + .bind(&datatable) + .fetch_optional(&mut **tx) + .await? + .flatten(); + path.push((w_id.clone(), datatable.clone())); + let entry: Option = + entry.and_then(|e| serde_json::from_value(e).ok()); + match entry.and_then(|e| e.reference.or(e.governed_by)) { + Some(next) => (w_id, datatable) = (next.workspace_id, next.datatable), + None => break, + } + } + Ok(path.into_iter().map(|(w_id, _)| w_id).collect()) +} + +pub(crate) fn same_database(a: &Option, b: &Option) -> bool { + match (a, b) { + (Some(a), Some(b)) => { + a.resource_type == b.resource_type && a.resource_path == b.resource_path + } + _ => false, + } +} diff --git a/backend/windmill-api-workspaces/src/datatable_replay_oss.rs b/backend/windmill-api-workspaces/src/datatable_replay_oss.rs new file mode 100644 index 0000000000..edcf8e402b --- /dev/null +++ b/backend/windmill-api-workspaces/src/datatable_replay_oss.rs @@ -0,0 +1,48 @@ +/* + * Author: Ruben Fiszel + * Copyright: Windmill Labs, Inc 2022 + * This file and its contents are licensed under the AGPLv3 License. + * Please see the included NOTICE for copyright information and + * LICENSE-AGPL for a copy of the license. + */ + +//! Where the replay of a data table's owners and grants into its copy comes from: the enterprise +//! one, or a refusal. Without it a data table under roles is not copied at all: its rows would +//! arrive owned by the admin connection with no grant for any role. + +#[cfg(all(feature = "private", feature = "enterprise"))] +pub(crate) use crate::datatable_replay_ee::replay_owners_and_grants; + +#[cfg(all(feature = "private", feature = "enterprise"))] +pub(crate) fn ensure_replay() -> windmill_common::error::Result<()> { + Ok(()) +} + +#[cfg(not(all(feature = "private", feature = "enterprise")))] +use { + std::collections::BTreeSet, + tokio::sync::mpsc, + tokio_postgres::error::DbError, + windmill_common::error::{Error, Result}, +}; + +/// Checked before anything is created. +#[cfg(not(all(feature = "private", feature = "enterprise")))] +pub(crate) fn ensure_replay() -> Result<()> { + Err(Error::BadRequest( + "Cloning a data table under roles is a Windmill Enterprise Edition feature: the copy \ + needs the source's owners and grants replayed. Fork it keeping the original database \ + instead." + .to_string(), + )) +} + +#[cfg(not(all(feature = "private", feature = "enterprise")))] +pub(crate) async fn replay_owners_and_grants( + _source: &tokio_postgres::Client, + _target: &tokio_postgres::Transaction<'_>, + _notices: &mut mpsc::UnboundedReceiver, + _catalog_roles: &BTreeSet, +) -> Result<()> { + ensure_replay() +} diff --git a/backend/windmill-api-workspaces/src/lib.rs b/backend/windmill-api-workspaces/src/lib.rs index 5dfdece644..e7da2b8496 100644 --- a/backend/windmill-api-workspaces/src/lib.rs +++ b/backend/windmill-api-workspaces/src/lib.rs @@ -3,9 +3,11 @@ pub mod ai_session_backups; pub mod data_metrics; pub mod datatable_acl; pub mod datatable_acl_oss; +pub mod datatable_clone; pub mod datatable_migrations; pub mod datatable_permissions; pub mod datatable_permissions_oss; +pub mod datatable_replay_oss; pub mod deployment_requests; pub mod workspaces; pub mod workspaces_extra; @@ -16,6 +18,8 @@ pub mod workspaces_ee; #[cfg(all(feature = "private", feature = "enterprise"))] pub mod datatable_acl_ee; +#[cfg(all(feature = "private", feature = "enterprise"))] +pub mod datatable_replay_ee; #[cfg(all(feature = "private", feature = "enterprise"))] pub mod datatable_permissions_ee; diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 76105dbc24..4822c2f3b9 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -501,8 +501,8 @@ struct CreateWorkspaceFork { id: String, name: String, color: Option, - /// Datatable names that were forked. For each, the backend will update the - /// forked workspace's datatable config to point to the new database. + /// Data tables the fork gets a copy of rather than a pointer at the parent's. For each, the + /// fork's entry is pointed at the new database. #[serde(default)] forked_datatables: Vec, /// Lakes the user explicitly chose to SHARE with the parent (the fork then reads and @@ -534,6 +534,11 @@ struct CreateWorkspaceFork { struct ForkedDatatableInfo { name: String, new_dbname: String, + /// What this request copies into `new_dbname`, which it creates. Optional only so that the + /// entry older CLIs send — naming a database they created and filled themselves — is refused + /// with a message rather than a parse error. + #[serde(default)] + fork_behavior: Option, } #[derive(Deserialize)] @@ -2233,8 +2238,8 @@ async fn list_datatables( name, resource_type: database.resource_type.as_ref().to_string(), resource_path: database.resource_path.clone(), - governing_workspace_id: (governing.workspace_id != w_id) - .then(|| governing.workspace_id.clone()), + governing_workspace_id: (governing.governing_workspace_id() != w_id) + .then(|| governing.governing_workspace_id().to_string()), permissioned: governing.datatable.permissions.is_some(), }); } @@ -2916,6 +2921,18 @@ fn truncate_column_default(default: String) -> String { mod tests { use super::*; + #[test] + fn a_dev_workspace_copies_into_the_fork_database_namespace() { + assert_eq!( + forked_datatable_dbname("wm-fork-my-fork", "main"), + "wm_fork_my_fork__main" + ); + assert_eq!( + forked_datatable_dbname("my-dev", "main"), + "wm_fork_my_dev__main" + ); + } + /// The header of a pg_dump, followed by an object whose body also holds a `SET`. const DUMP: &str = "--\n\ -- PostgreSQL database dump\n\ @@ -3384,7 +3401,10 @@ async fn server_setting_names(pg_db: &PgDatabase) -> Result> { /// so a dump that breaks partway through imports partially and reads as a success. /// ON_ERROR_STOP surfaces the failure and --single-transaction makes the restore /// all-or-nothing, leaving the target as it was and the import retryable. -async fn pg_import_dump(target_db: &PgDatabase, dump_file: &DumpFile) -> Result<()> { +/// +/// Authorization: none. Writes into `target_db` with the credentials it carries; callers MUST have +/// authorized writing to that database. +pub(crate) async fn pg_import_dump(target_db: &PgDatabase, dump_file: &DumpFile) -> Result<()> { let supported_settings = server_setting_names(target_db).await?; comment_out_unsupported_settings(dump_file, &supported_settings).await?; @@ -3433,7 +3453,7 @@ async fn create_pg_database( // exists rather than leaving an empty registered `wm_fork_…` behind. if let Some(reference) = req.source.strip_prefix("datatable://") { let (name, _) = parse_datatable_ref_for(&db, &w_id, reference).await?; - ensure_datatable_is_clonable(&db, &w_id, &name).await?; + ensure_copied_without_roles(&ensure_datatable_is_clonable(&db, &w_id, &name).await?)?; } // Non-superadmin: restrict dbname to wm_fork_ prefix @@ -3452,50 +3472,62 @@ async fn create_pg_database( } else { let source_pg = resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.source).await?; - let (client, connection) = source_pg.connect(Some(&db)).await?; - let join_handle = tokio::spawn(async move { connection.await }); - - let row = client - .query_one( - "SELECT EXISTS (SELECT 1 FROM pg_catalog.pg_database WHERE datname = $1)", - &[&req.target_dbname], - ) - .await - .map_err(|e| { - Error::internal_err(format!( - "Failed to check database existence: {}", - pg_error_message(&e) - )) - })?; - let db_exists: bool = row.get(0); - - if db_exists { - drop(client); - let _ = windmill_common::shutdown_pg_connection(join_handle).await; - return Err(Error::BadRequest(format!( - "Database '{}' already exists on the resource server", - req.target_dbname - ))); - } - - client - .execute(&format!("CREATE DATABASE \"{}\"", &req.target_dbname), &[]) - .await - .map_err(|e| { - Error::internal_err(format!( - "Failed to create database '{}': {}", - req.target_dbname, - pg_error_message(&e) - )) - })?; - - drop(client); - windmill_common::shutdown_pg_connection(join_handle).await?; + create_database_on_server(&db, &source_pg, &req.target_dbname).await?; } Ok(format!("Created database '{}'", req.target_dbname)) } +/// `CREATE DATABASE` on the server `server` connects to, refusing a name already taken there. +/// +/// Authorization: none. Callers MUST have authorized creating a database on that server — resolved +/// from a data table or resource the caller may administer. +pub(crate) async fn create_database_on_server( + db: &DB, + server: &PgDatabase, + dbname: &str, +) -> Result<()> { + windmill_common::validate_dbname(dbname)?; + let (client, connection) = server.connect(Some(db)).await?; + let join_handle = tokio::spawn(async move { connection.await }); + + let row = client + .query_one( + "SELECT EXISTS (SELECT 1 FROM pg_catalog.pg_database WHERE datname = $1)", + &[&dbname], + ) + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to check database existence: {}", + pg_error_message(&e) + )) + })?; + let db_exists: bool = row.get(0); + + if db_exists { + drop(client); + let _ = windmill_common::shutdown_pg_connection(join_handle).await; + return Err(Error::BadRequest(format!( + "Database '{dbname}' already exists on the resource server" + ))); + } + + client + .execute(&format!("CREATE DATABASE \"{dbname}\""), &[]) + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to create database '{dbname}': {}", + pg_error_message(&e) + )) + })?; + + drop(client); + windmill_common::shutdown_pg_connection(join_handle).await?; + Ok(()) +} + #[derive(Deserialize)] struct ImportPgDatabaseRequest { source: String, @@ -3505,48 +3537,22 @@ struct ImportPgDatabaseRequest { fork_behavior: DataTableForkBehavior, } -/// Refuse to copy a data table that is under roles. +/// Whether data table `name` of `w_id` has a shape a copy can be made of, returning what governs +/// it. Checked before anything is created — by the fork request for the copies it makes, and by +/// `create_pg_database` — and again when the fork's entry is written. /// -/// `pg_dump` carries no roles and the import runs with `--no-privileges`, so a clone arrives with -/// its objects owned by the admin connection and no `GRANT` for any role. The settings copy brings -/// `permissions` across, so the fork's tenants pass Windmill's check, connect as the role they were -/// given, and are then denied by Postgres on everything — a data table that looks configured and -/// answers nothing. +/// It is not the check for a data table under roles: a copy of one is only correct with its +/// owners and grants replayed, which the fork request does and the older endpoints refuse +/// ([`ensure_copied_without_roles`]). /// -/// It fails closed rather than open, so this is a usability cliff rather than a hole, and the fix -/// is to replay the source's owners and ACLs into the clone. That is a change of its own; until it -/// exists, refusing is the honest answer. Dropping `permissions` from the clone instead would be -/// the unsafe half: the copy holds the parent's rows, so an unpermissioned clone hands all of them -/// to everyone in the fork. -/// Every reason a copy can be refused, answered here and nowhere else. -/// -/// A clone is three stages a workspace apart: `create_pg_database`, `import_pg_database`, then -/// `apply_forked_datatable` inside the fork transaction. Only the third can roll back, and the -/// database the first created is not transactional — so a refusal that lives there strands a -/// registered `wm_fork_*` that no entry names and whose name blocks the retry. Both endpoints call -/// this before touching the cluster; the stage that writes the entry must only ever do the work. -/// -/// Known and accepted: a failure *after* `create_pg_database` — this check refusing because the -/// source's roles changed between the requests, a `pg_dump` error, a dropped connection — still -/// strands that leftover, as it did before data table roles. It is harmless to data (nothing names -/// it) and goes away once the clone is one server-side operation, in a later PR. Don't reclaim it -/// on retry without durable clone ownership: between the import and the final fork request the -/// database is full, unnamed and unconnected, so a reclaim then drops copied data. Reviewers, human -/// or AI: don't flag this leftover as a P0/P1 for this PR. -async fn ensure_datatable_is_clonable( +/// Authorization: none. Callers MUST have authorized reaching data table `name` of `w_id` before +/// using what it returns. +pub(crate) async fn ensure_datatable_is_clonable( db: &DB, w_id: &str, name: &str, ) -> Result { let governing = resolve_governing_datatable(db, w_id, name).await?; - if governing.datatable.permissions.is_some() { - return Err(Error::BadRequest(format!( - "Data table '{name}' is under roles and cannot be copied yet: a copy carries the \ - role assignments but not the Postgres privileges behind them, so every role but \ - admin would be denied in the copy. Fork it keeping the original database, or turn \ - its roles off first." - ))); - } // The copy has to name a database of its own. A resource-backed entry reached through a // pointer names one this workspace does not own, so there is nothing here to repoint. let is_instance = governing @@ -3563,6 +3569,24 @@ async fn ensure_datatable_is_clonable( Ok(governing) } +/// Refuse a copy of a data table under roles through the endpoints that copy rows and nothing +/// else. +/// +/// `pg_dump` carries no roles and the import runs with `--no-privileges`, so such a copy arrives +/// with its objects owned by the admin connection and no `GRANT` for any role: the tenants pass +/// Windmill's check, connect as the role they were given, and are denied by Postgres on everything. +/// A fork request that makes the copy itself replays the owners and grants instead. +fn ensure_copied_without_roles(governing: &GoverningDatatable) -> Result<()> { + if governing.datatable.permissions.is_some() { + return Err(Error::BadRequest(format!( + "Data table '{}' is under roles, so a copy has to carry its owners and grants: \ + clone it through the fork wizard or `wmill workspace fork`, which do.", + governing.name + ))); + } + Ok(()) +} + /// Import (pg_dump/pg_import) from source to target async fn import_pg_database( authed: ApiAuthed, @@ -3577,7 +3601,7 @@ async fn import_pg_database( if let Some(reference) = req.source.strip_prefix("datatable://") { let (name, _) = parse_datatable_ref_for(&db, &w_id, reference).await?; - ensure_datatable_is_clonable(&db, &w_id, &name).await?; + ensure_copied_without_roles(&ensure_datatable_is_clonable(&db, &w_id, &name).await?)?; } if req.fork_behavior == DataTableForkBehavior::SchemaAndData { @@ -3698,6 +3722,15 @@ async fn edit_ducklake_config( } let mut tx = db.begin().await?; + windmill_common::lock_instance_databases( + &mut tx, + new_config.settings.ducklakes.values().filter_map(|lake| { + (lake.catalog.resource_type + == windmill_common::workspaces::DucklakeCatalogResourceType::Instance) + .then_some(lake.catalog.resource_path.as_str()) + }), + ) + .await?; let args_for_audit = format!("{:?}", new_config.settings); audit_log( @@ -3799,6 +3832,16 @@ async fn edit_datatable_config( let is_superadmin = require_super_admin(&db, &authed).await.is_ok(); let mut tx = db.begin().await?; + windmill_common::lock_instance_databases( + &mut tx, + new_config.settings.datatables.values().filter_map(|dt| { + dt.database + .as_ref() + .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance) + .map(|d| d.resource_path.as_str()) + }), + ) + .await?; // Read under the row lock this transaction will write with. `permissions`, `reference` and // `forked_from` are carried across from what this read returns, so a permissions save @@ -3924,12 +3967,28 @@ async fn edit_datatable_config( // hand the fork the database outright. `forked_from` is the clone stamp the fork flow // writes: whether an entry has one is carried the same way, since it is what marks the // database droppable, but the schema baseline inside it is the diff view's to advance. + // `governed_by` is a clone's `reference` for its roles, and clearing it would hand the + // fork the copied rows the same way. dt.permissions = old.and_then(|old| old.permissions.clone()); dt.reference = old.and_then(|old| old.reference.clone()); + dt.governed_by = old.and_then(|old| old.governed_by.clone()); dt.forked_from = match old.and_then(|old| old.forked_from.as_ref()) { Some(stored) => Some(dt.forked_from.take().unwrap_or_else(|| stored.clone())), None => None, }; + // A clone's roles and grants were replayed into the database it was copied into, and hold + // for that database alone: whoever saves, it stays where it is. + if dt.governed_by.is_some() + && !crate::datatable_clone::same_database( + &dt.database, + &old.and_then(|old| old.database.clone()), + ) + { + return Err(Error::BadRequest(format!( + "Data table '{name}' is a clone taking its roles from the data table it was copied \ + from, and its grants hold for its own database only: it cannot be moved." + ))); + } // Carrying the block onto a resource-backed entry would produce a data table the chokepoint // refuses on every job — a save that succeeds and breaks everything afterwards. Refuse it // instead: turning roles off first is one step, and it keeps discarding an access decision @@ -3968,9 +4027,18 @@ async fn edit_datatable_config( // workspace does not own. Pointing an entry at another workspace's data table is not checked // here because it cannot be requested at all: `reference` is overwritten from the stored entry // above, for every caller. + // + // Compared against the entry the carried fields came from, not the one stored under the same + // name: otherwise swapping two names keeps each database in place while moving a clone's + // `governed_by` off the copy it governs. if !is_superadmin { for (name, dt) in new_config.settings.datatables.iter() { - let old_dt = old_datatables.get(name); + let old_dt = old_datatables.get( + rename_src + .get(name.as_str()) + .copied() + .unwrap_or(name.as_str()), + ); if dt .database .as_ref() @@ -4010,7 +4078,8 @@ async fn edit_datatable_config( .settings .datatables .iter() - .filter(|(_, dt)| dt.permissions.is_none()) + // A clone carries its roles through `governed_by` rather than `permissions`. + .filter(|(_, dt)| dt.permissions.is_none() && dt.governed_by.is_none()) .filter_map(|(name, dt)| { let db = dt .database @@ -4043,7 +4112,7 @@ async fn edit_datatable_config( sqlx::query_scalar( "SELECT DISTINCT dt.value->'database'->>'resource_path' FROM workspace_settings ws CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt - WHERE ws.workspace_id <> $1 AND dt.value ? 'permissions' + WHERE ws.workspace_id <> $1 AND (dt.value ? 'permissions' OR dt.value ? 'governed_by') AND dt.value->'database'->>'resource_type' = 'instance'", ) .bind(&w_id) @@ -4052,7 +4121,7 @@ async fn edit_datatable_config( }; for (name, dbname) in newly_pointed { let governed_here = old_datatables.values().any(|old| { - old.permissions.is_some() + (old.permissions.is_some() || old.governed_by.is_some()) && old.database.as_ref().is_some_and(|d| { d.resource_type == DataTableCatalogResourceType::Instance && d.resource_path == dbname @@ -4112,8 +4181,11 @@ async fn edit_datatable_config( 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"#, + WHERE EXISTS ( + SELECT 1 FROM (VALUES ('reference'), ('governed_by')) k(link) + WHERE dt.value->k.link->>'workspace_id' = $1 + AND dt.value->k.link->>'datatable' = $2 + )"#, &w_id, name, ) @@ -8241,6 +8313,7 @@ async fn create_workspace_fork_branch( // dangling branch on the synced repos. check_fork_w_id_conflict(&db, &nw.id).await?; purge_stale_fork_diff_state(&db, &nw.id).await?; + validate_forked_datatables(&db, &authed, &w_id, &nw).await?; Ok(Json( handle_fork_branch_creation(&authed.email, &authed.username, &db, &w_id, &nw.id).await?, @@ -8280,8 +8353,9 @@ async fn snapshot_datatable_schema( /// a fork admin could then edit to widen their own access to it. A pointer has nothing local to /// edit: the parent's entry stays the only place the decision lives. /// -/// The cloned data tables are skipped: they own a fresh database of their own, and they keep the -/// copied `permissions` as their starting point, which they then govern. +/// The cloned data tables are skipped: they own a fresh database of their own, and +/// `apply_forked_datatable` has already dropped their copied `permissions` — for a clone of a data +/// table under roles, in favor of `governed_by`. async fn point_kept_datatables_at_parent( tx: &mut Transaction<'_, Postgres>, parent_w_id: &str, @@ -8336,6 +8410,7 @@ async fn point_kept_datatables_at_parent( workspace_id: parent_w_id.to_string(), datatable: name.clone(), }), + governed_by: None, forked_from: None, migrations_enabled: dt.migrations_enabled, permissions: None, @@ -8356,7 +8431,8 @@ async fn point_kept_datatables_at_parent( Ok(()) } -/// Move every pointer in any workspace that names `(w_id, from)` to `(w_id, to)`. +/// Move every pointer, and every clone's `governed_by`, in any workspace that names `(w_id, from)` +/// to `(w_id, to)`. /// /// `EXISTS` rather than a `LIKE` over the whole document: the update rewrites the row, so matching /// every workspace that holds any pointer would rewrite rows to a byte-identical value and hold an @@ -8372,17 +8448,22 @@ async fn repoint_datatable_references( SET datatable = ( SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg( dt.key, - CASE WHEN dt.value->'reference'->>'workspace_id' = $1 - AND dt.value->'reference'->>'datatable' = $2 - THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text)) - ELSE dt.value END + (SELECT CASE WHEN r.v->'governed_by'->>'workspace_id' = $1 + AND r.v->'governed_by'->>'datatable' = $2 + THEN jsonb_set(r.v, '{governed_by,datatable}', to_jsonb($3::text)) + ELSE r.v END + FROM (SELECT CASE WHEN dt.value->'reference'->>'workspace_id' = $1 + AND dt.value->'reference'->>'datatable' = $2 + THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text)) + ELSE dt.value END AS v) r) )) FROM jsonb_each(ws.datatable->'datatables') dt ) WHERE EXISTS ( - SELECT 1 FROM jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) d - WHERE d.value->'reference'->>'workspace_id' = $1 - AND d.value->'reference'->>'datatable' = $2 + SELECT 1 FROM jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) d, + LATERAL (VALUES ('reference'), ('governed_by')) k(link) + WHERE d.value->k.link->>'workspace_id' = $1 + AND d.value->k.link->>'datatable' = $2 )"#, w_id, from, @@ -8400,17 +8481,8 @@ async fn apply_forked_datatable( parent_w_id: &str, forked_w_id: &str, fdt: &ForkedDatatableInfo, + copy: &crate::datatable_clone::MadeCopy, ) -> Result<()> { - // Cloning reads the parent's whole schema as admin and hands the copy to the fork, so it is - // for the workspace that governs the data table — a fork can use one, never duplicate it. - windmill_common::workspaces::ensure_datatable_admin_access( - db, - parent_w_id, - &fdt.name, - &DatatableAccess::Authed(authed.to_authed_ref()), - ) - .await?; - let governing = ensure_datatable_is_clonable(db, parent_w_id, &fdt.name).await?; windmill_common::validate_dbname(&fdt.new_dbname)?; if !fdt.new_dbname.starts_with("wm_fork_") { return Err(Error::BadRequest(format!( @@ -8418,6 +8490,52 @@ async fn apply_forked_datatable( fdt.new_dbname ))); } + // Held until the fork commits, as the permissions save holds them: the source moved onto + // another database, or put under roles or taken off them, since its copy was made would link + // the copy to roles its grants were not replayed for. + let locked = crate::datatable_clone::lock_settings_rows(tx, parent_w_id, &fdt.name).await?; + let governing = ensure_datatable_is_clonable(db, parent_w_id, &fdt.name).await?; + let under_roles = governing.datatable.permissions.is_some(); + let unchanged = crate::datatable_clone::same_database( + &governing.datatable.database, + &Some(copy.source_database.clone()), + ) && copy.replayed == under_roles + && crate::datatable_clone::lock_settings_rows(tx, parent_w_id, &fdt.name).await? == locked; + if !unchanged { + return Err(Error::BadRequest(format!( + "Data table '{}' changed while it was being copied for this fork — its database, or \ + whether it is under roles. Create the fork again.", + fdt.name + ))); + } + // The fork keeps a snapshot of the source's schema, which under roles is only for those who may + // connect as one of them — as the copy itself was. + crate::datatable_permissions::ensure_reaches_governing_datatable( + db, + parent_w_id, + &fdt.name, + &governing, + authed, + ) + .await?; + // Under roles, the copy holds rows the source's roles decide who reaches, so that entry keeps + // deciding. Settled from the source as it resolves now: the settings clone may have handed the + // fork a pointer, or a clone of its own. A copy of a data table without roles is the fork's, + // as it was before roles existed: everyone reached all of it already. + let governed_by = governing + .datatable + .permissions + .is_some() + .then(|| { + serde_json::to_value(governing.governor.clone().unwrap_or_else(|| { + windmill_common::workspaces::DataTableReference { + workspace_id: governing.workspace_id.clone(), + datatable: governing.name.clone(), + } + })) + }) + .transpose() + .map_err(|e| Error::internal_err(format!("serializing a clone's governor: {e}")))?; // Snapshot the schema from the source (parent) datatable let schema = snapshot_datatable_schema(db, parent_w_id, &fdt.name).await?; @@ -8463,33 +8581,42 @@ async fn apply_forked_datatable( "resource_type": "instance", "resource_path": &fdt.new_dbname, }); - sqlx::query!( + sqlx::query( r#"UPDATE workspace_settings SET datatable = jsonb_set( - jsonb_set( - datatable #- ARRAY['datatables', $2, 'reference'], - ARRAY['datatables', $2, 'database'], $3::jsonb), - ARRAY['datatables', $2, 'forked_from'], $4::jsonb - ) + datatable #- ARRAY['datatables', $2, 'reference'], + ARRAY['datatables', $2, 'database'], $3::jsonb) WHERE workspace_id = $1"#, - forked_w_id, - &fdt.name, - new_database, - forked_from, ) + .bind(forked_w_id) + .bind(&fdt.name) + .bind(new_database) .execute(&mut **tx) .await?; } else { - // Resource: update the resource's dbname and mark as ws_specific + // Resource: point it at the copy and mark it ws_specific. What the settings clone carried is + // the source's resource and variables as they are now, which an edit made while the copy + // was being made can have changed, and changed back: the clone has to be what the copy's + // connection was resolved from. let resource_path = &database.resource_path; - sqlx::query!( + let cloned = + crate::datatable_clone::connection_snapshot(db, &mut **tx, forked_w_id, resource_path) + .await?; + if copy.connection.as_ref() != Some(&cloned) { + return Err(Error::BadRequest(format!( + "The resource of data table '{}' changed while it was being copied for this \ + fork. Create the fork again.", + fdt.name + ))); + } + sqlx::query( r#"UPDATE resource SET value = jsonb_set(value, '{dbname}', to_jsonb($3::text)) WHERE workspace_id = $1 AND path = $2"#, - forked_w_id, - resource_path, - &fdt.new_dbname, ) + .bind(forked_w_id) + .bind(resource_path) + .bind(&fdt.new_dbname) .execute(&mut **tx) .await?; @@ -8500,20 +8627,31 @@ async fn apply_forked_datatable( ) .execute(&mut **tx) .await?; - - // Set forked_from on the datatable config - sqlx::query!( - r#"UPDATE workspace_settings - SET datatable = jsonb_set(datatable, ARRAY['datatables', $2, 'forked_from'], $3::jsonb) - WHERE workspace_id = $1"#, - forked_w_id, - &fdt.name, - forked_from, - ) - .execute(&mut **tx) - .await?; } + // The settings clone copied the source's `permissions` along, and a clone of a clone its + // `governed_by`: a clone keeps no `permissions` of its own, and a link only under roles. + sqlx::query( + r#"UPDATE workspace_settings + SET datatable = jsonb_set( + CASE WHEN $3::jsonb IS NULL + THEN datatable #- ARRAY['datatables', $2, 'permissions'] + #- ARRAY['datatables', $2, 'governed_by'] + ELSE jsonb_set( + datatable #- ARRAY['datatables', $2, 'permissions'], + ARRAY['datatables', $2, 'governed_by'], $3::jsonb) + END, + ARRAY['datatables', $2, 'forked_from'], $4::jsonb + ) + WHERE workspace_id = $1"#, + ) + .bind(forked_w_id) + .bind(&fdt.name) + .bind(governed_by) + .bind(forked_from) + .execute(&mut **tx) + .await?; + Ok(()) } @@ -8848,7 +8986,169 @@ async fn create_workspace_fork( ensure_no_existing_dev_workspace(&db, &parent_workspace_id).await?; } + // Refused here, before any database exists; `make_copies` checks again under the locks. + validate_forked_datatables(&db, &authed, &parent_workspace_id, &nw).await?; + + // Detached from the request: a client that goes away while the copies are made must still end + // with the fork created or no copy left behind, and dropping the handler's future would skip + // that cleanup. + tokio::spawn(async move { + let fork_id = nw.id.clone(); + let copies = crate::datatable_clone::make_copies( + &db, + &authed, + &parent_workspace_id, + ©_requests(&nw.forked_datatables), + ) + .await?; + let replayed: Vec = copies + .iter() + .filter(|c| c.replayed) + .map(|c| c.behavior) + .collect(); + match write_workspace_fork( + db.clone(), + authed, + parent_workspace_id, + nw, + dev_workspace_label, + &copies, + ) + .await + { + Ok(message) => { + for behavior in replayed { + windmill_common::feature_usage::log_feature_usage( + "datatable", + "clone_replayed", + match behavior { + DataTableForkBehavior::SchemaOnly => "schema_only", + _ => "schema_and_data", + }, + ); + } + Ok(message) + } + // A commit whose acknowledgement was lost can still have committed, and a concurrent + // request for the same id can have: once the write has settled, the cleanup keeps + // whichever copies a committed fork names, and drops the rest. + Err(e) => match wait_for_fork_write(&db, &fork_id).await { + Ok(()) => Err(crate::datatable_clone::drop_copies_after(&db, copies, e).await), + Err(settle) => { + tracing::error!( + "Could not tell whether fork '{fork_id}' was created, so its copies were \ + kept: {settle}" + ); + Err(e) + } + }, + } + }) + .await + .map_err(|e| Error::internal_err(format!("Creating the fork stopped unexpectedly: {e}")))? +} + +/// The database a data table of the fork or dev workspace `fork_id` is copied into, as the wizard +/// and the CLI derive it. A dev workspace's id has no `wm-fork-` prefix, and its copies are dropped +/// by the same `wm_fork_` rule as a fork's. Dev workspace `x` and fork `wm-fork-x` share a name, as +/// their branches do: the second to copy a data table of that name is refused at `CREATE`. +fn forked_datatable_dbname(fork_id: &str, datatable: &str) -> String { + let suffix = fork_id + .strip_prefix(windmill_common::workspaces::WM_FORK_PREFIX) + .unwrap_or(fork_id); + format!("wm_fork_{}__{datatable}", suffix.replace('-', "_")) +} + +/// Wait until no transaction writing fork `fork_id` is still running: the lock +/// `write_workspace_fork` holds until its transaction ends is taken and released. +async fn wait_for_fork_write(db: &DB, fork_id: &str) -> Result<()> { + let mut tx = db.begin().await?; + sqlx::query("SELECT pg_advisory_xact_lock(hashtext('fork:' || $1))") + .bind(fork_id) + .execute(&mut *tx) + .await?; + tx.commit().await?; + Ok(()) +} + +/// Everything about a fork's data tables that can be refused before a git branch or a database is +/// created, and that the fork request re-checks. +/// +/// Every data table is copied by the fork request, into a database it creates: an entry naming a +/// database that already exists could reach a copy made for another fork — one left behind by a +/// deleted fork included — without that copy's governance. +async fn validate_forked_datatables( + db: &DB, + authed: &ApiAuthed, + parent_w_id: &str, + nw: &CreateWorkspaceFork, +) -> Result<()> { + let mut names = HashSet::new(); + for fdt in &nw.forked_datatables { + if fdt.fork_behavior.is_none() { + return Err(Error::BadRequest(format!( + "Data table '{}' names no `fork_behavior`: the fork request makes the copy of each \ + data table itself. Update the Windmill CLI.", + fdt.name + ))); + } + let expected = forked_datatable_dbname(&nw.id, &fdt.name); + if fdt.new_dbname != expected { + return Err(Error::BadRequest(format!( + "Data table '{}' of fork '{}' is copied into database '{expected}', not '{}'", + fdt.name, nw.id, fdt.new_dbname + ))); + } + if !names.insert(fdt.name.as_str()) { + return Err(Error::BadRequest(format!( + "Data table '{}' is named more than once in this fork", + fdt.name + ))); + } + } + crate::datatable_clone::authorize_copies( + db, + authed, + parent_w_id, + ©_requests(&nw.forked_datatables), + ) + .await +} + +/// The copies a fork request asks this request to make. [`validate_forked_datatables`] refuses an +/// entry without a `fork_behavior`. +fn copy_requests( + forked_datatables: &[ForkedDatatableInfo], +) -> Vec> { + forked_datatables + .iter() + .filter_map(|fdt| { + fdt.fork_behavior + .map(|behavior| crate::datatable_clone::CopyRequest { + name: &fdt.name, + dbname: &fdt.new_dbname, + behavior, + }) + }) + .collect() +} + +/// Write the fork, with its data tables pointed at `copies`. +async fn write_workspace_fork( + db: DB, + authed: ApiAuthed, + parent_workspace_id: String, + nw: CreateWorkspaceFork, + dev_workspace_label: Option, + copies: &[crate::datatable_clone::MadeCopy], +) -> Result { let mut tx: Transaction<'_, Postgres> = db.begin().await?; + // Held until this transaction ends: after an error, the copies' cleanup waits on it, so it reads + // whether the fork exists only once a commit still being resolved has settled. + sqlx::query("SELECT pg_advisory_xact_lock(hashtext('fork:' || $1))") + .bind(&nw.id) + .execute(&mut *tx) + .await?; if nw.is_dev_workspace { // The checks above ran outside a transaction, so the parent's eligibility and the chain's @@ -8974,10 +9274,41 @@ async fn create_workspace_fork( repoint_unresolvable_cloned_identities(&mut tx, &forked_id, &authed).await?; + // Held until the fork commits, as settings saves naming these databases hold it: an entry + // saved elsewhere while a copy was being made would reach it without the governance written + // below, and the alias check on that save saw no governed entry yet. + let instance_copies: Vec<&str> = copies + .iter() + .filter(|c| c.source_database.resource_type == DataTableCatalogResourceType::Instance) + .map(|c| c.dbname.as_str()) + .collect(); + windmill_common::lock_instance_databases(&mut tx, instance_copies.iter().copied()).await?; + for dbname in &instance_copies { + let users = windmill_common::instance_database_users(&mut *tx, dbname).await?; + if !users.is_empty() { + return Err(Error::BadRequest(format!( + "Database '{dbname}' copied for this fork is already used by workspaces {}; \ + fork again", + users.join(", ") + ))); + } + } + // Update forked datatable settings to point to new databases for fdt in &nw.forked_datatables { - apply_forked_datatable(&db, &mut tx, &authed, &parent_workspace_id, &forked_id, fdt) - .await?; + let copy = copies.iter().find(|c| c.name == fdt.name).ok_or_else(|| { + Error::internal_err(format!("No copy was made of data table '{}'", fdt.name)) + })?; + apply_forked_datatable( + &db, + &mut tx, + &authed, + &parent_workspace_id, + &forked_id, + fdt, + copy, + ) + .await?; } point_kept_datatables_at_parent( @@ -9036,6 +9367,20 @@ async fn create_workspace_fork( .await?; } + let copied = copies + .iter() + .map(|c| { + format!( + "{} ({})", + c.name, + match c.behavior { + DataTableForkBehavior::SchemaOnly => "schema_only", + _ => "schema_and_data", + } + ) + }) + .collect::>() + .join(", "); audit_log( &mut *tx, &authed, @@ -9043,7 +9388,7 @@ async fn create_workspace_fork( ActionKind::Create, &forked_id, Some(nw.name.as_str()), - None, + (!copied.is_empty()).then(|| [("copied_datatables", copied.as_str())].into()), ) .await?; tx.commit().await?; diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 62ab812a8f..82423480fa 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -502,21 +502,25 @@ pub(crate) async fn change_workspace_id( // A fork's data table entry names the workspace that governs it by id, so the rename has to // follow there too — anywhere, not just in the reparented children: a detached workspace can // point at this one without being its fork. Left behind, the pointer resolves to the archived - // shell and every job through it stops. + // shell and every job through it stops. A clone's `governed_by` names it the same way. info!("Re-pointing data table references to the new workspace id"); sqlx::query!( r#"UPDATE workspace_settings ws SET datatable = ( SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg( dt.key, - CASE WHEN dt.value->'reference'->>'workspace_id' = $2 - THEN jsonb_set(dt.value, '{reference,workspace_id}', to_jsonb($1::text)) - ELSE dt.value END + (SELECT CASE WHEN r.v->'governed_by'->>'workspace_id' = $2 + THEN jsonb_set(r.v, '{governed_by,workspace_id}', to_jsonb($1::text)) + ELSE r.v END + FROM (SELECT CASE WHEN dt.value->'reference'->>'workspace_id' = $2 + THEN jsonb_set(dt.value, '{reference,workspace_id}', to_jsonb($1::text)) + ELSE dt.value END AS v) r) )) FROM jsonb_each(ws.datatable->'datatables') dt ) WHERE jsonb_typeof(ws.datatable->'datatables') = 'object' - AND ws.datatable::text LIKE '%"reference"%'"#, + AND (ws.datatable::text LIKE '%"reference"%' + OR ws.datatable::text LIKE '%"governed_by"%')"#, &rw.new_id, &old_id, ) @@ -1003,17 +1007,19 @@ pub(crate) async fn delete_workspace( // fails mid-way must never leave a live workspace with its fork data destroyed and no // registry row to retry from. Read-only: nothing is dropped here. // Read before the delete: another workspace's data table entry can point at one of this - // workspace's, and deleting the workspace it names leaves that pointer resolving to nothing. - // Nothing sweeps them — turning them back into copies would hand each fork the database - // outright — so the deleter is told which data tables they just stranded. - let stranded_pointers = sqlx::query!( - r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!" + // workspace's, or be a clone taking its roles from one, and deleting the workspace it names + // leaves it resolving to nothing. Nothing sweeps them — turning them back into copies would + // hand each fork the database outright — so the deleter is told which data tables they just + // stranded. + let stranded_pointers = sqlx::query_as::<_, (String, String)>( + r#"SELECT ws.workspace_id, dt.key FROM workspace_settings ws CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt WHERE dt.value->'reference'->>'workspace_id' = $1 + OR dt.value->'governed_by'->>'workspace_id' = $1 ORDER BY ws.workspace_id, dt.key"#, - &w_id, ) + .bind(&w_id) .fetch_all(&db) .await .unwrap_or_default(); @@ -1341,7 +1347,7 @@ pub(crate) async fn delete_workspace( } else { let stranded = stranded_pointers .iter() - .map(|r| format!("{}/{}", r.workspace_id, r.datatable)) + .map(|(workspace_id, datatable)| format!("{workspace_id}/{datatable}")) .collect::>() .join(", "); Ok(format!( diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 4ddfbffa28..9c4212c234 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -33872,6 +33872,15 @@ components: $ref: "#/components/schemas/DatatableRoleTenants" governing_workspace_id: type: string + clone_of: + type: object + description: for a clone, the data table whose roles it takes + required: [workspace_id, datatable] + properties: + workspace_id: + type: string + datatable: + type: string editable: type: boolean available_roles: @@ -34110,7 +34119,7 @@ components: DatatableAclInfo: type: object - required: [owner, roles, editable, supports_maintain, dbname, grants, children] + required: [owner, roles, editable, clone, supports_maintain, dbname, grants, children] properties: owner: type: string @@ -34122,6 +34131,9 @@ components: editable: type: boolean description: whether the caller may plan and apply changes + clone: + type: boolean + description: whether this is a clone, whose grants stay as they were copied supports_maintain: type: boolean description: whether the server is Postgres 17+, which added the MAINTAIN table privilege @@ -35274,6 +35286,16 @@ components: new_dbname: type: string description: "New database name for the fork" + fork_behavior: + type: string + enum: + - schema_only + - schema_and_data + description: >- + What the fork request copies into `new_dbname`, which it creates — with the + owners and grants of a data table under roles. This server refuses an entry + without it; servers predating it expect `new_dbname` created and filled + beforehand. shared_ducklakes: type: array items: @@ -36237,6 +36259,17 @@ components: type: string datatable: type: string + governed_by: + description: >- + On a clone, the data table it was copied from, whose roles it takes. Server-owned + like `reference`. + type: object + required: [workspace_id, datatable] + properties: + workspace_id: + type: string + datatable: + type: string migrations_enabled: type: boolean description: Whether the SQL migrations feature is opted in for this data table diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index a67bb6bfa1..5feaba7800 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -1474,8 +1474,98 @@ pub fn validate_dbname(dbname: &str) -> error::Result<()> { Ok(()) } +/// Lock the instance databases among `names` until `tx` ends, in a stable order. Taken by every +/// settings save naming an instance database, and by [`drop_unused_instance_database`]: a save +/// cannot start using a database between that drop's check that nothing does and the drop. +pub async fn lock_instance_databases<'a>( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + names: impl IntoIterator, +) -> error::Result<()> { + let names: std::collections::BTreeSet<&str> = names.into_iter().collect(); + for name in names { + sqlx::query("SELECT pg_advisory_xact_lock(hashtext('instance_database:' || $1))") + .bind(name) + .execute(&mut **tx) + .await?; + } + Ok(()) +} + +/// Drop instance database `dbname`, which a request just created, unless a workspace names it as a +/// data table or a ducklake catalog — the workspaces returned, with nothing dropped. A database is +/// registered on the instance as soon as it is created, so a superadmin can point a workspace at it +/// before the request that created it gives up on it. +/// +/// Authorization: none. Callers MUST pass only a database the same request created and has not +/// handed to anything yet. +/// +/// The lock, the check and the drop share one connection: a second one taken from the pool while +/// the first is held could wait forever on a small pool. +pub async fn drop_unused_instance_database(db: &DB, dbname: &str) -> error::Result> { + let mut conn = db.acquire().await?; + let key = format!("instance_database:{dbname}"); + sqlx::query("SELECT pg_advisory_lock(hashtext($1))") + .bind(&key) + .execute(&mut *conn) + .await?; + let dropped = drop_if_unused_on(&mut conn, dbname).await; + let unlocked = sqlx::query("SELECT pg_advisory_unlock(hashtext($1))") + .bind(&key) + .execute(&mut *conn) + .await; + if unlocked.is_err() { + // Closing the connection is what releases a session lock it could not release itself. + drop(conn.detach()); + } + dropped +} + +async fn drop_if_unused_on( + conn: &mut sqlx::PgConnection, + dbname: &str, +) -> error::Result> { + let users = instance_database_users(conn, dbname).await?; + if users.is_empty() { + drop_custom_instance_database_on(conn, dbname).await?; + } + Ok(users) +} + +/// The workspaces naming instance database `dbname` as a data table or a ducklake catalog. Taken +/// under [`lock_instance_databases`] for `dbname`, the answer holds until that lock is released. +/// +/// Authorization: none, and it reads every workspace's settings. Callers MUST pass only a database +/// their own request created, and may name the workspaces returned only to a caller allowed to +/// create or drop instance databases. +pub async fn instance_database_users( + conn: &mut sqlx::PgConnection, + dbname: &str, +) -> error::Result> { + Ok(sqlx::query_scalar( + "SELECT DISTINCT ws.workspace_id FROM workspace_settings ws + WHERE EXISTS (SELECT 1 FROM jsonb_each(CASE WHEN jsonb_typeof(ws.datatable->'datatables') = 'object' + THEN ws.datatable->'datatables' ELSE '{}'::jsonb END) dt + WHERE dt.value->'database'->>'resource_type' = 'instance' + AND dt.value->'database'->>'resource_path' = $1) + OR EXISTS (SELECT 1 FROM jsonb_each(CASE WHEN jsonb_typeof(ws.ducklake->'ducklakes') = 'object' + THEN ws.ducklake->'ducklakes' ELSE '{}'::jsonb END) dl + WHERE dl.value->'catalog'->>'resource_type' = 'instance' + AND dl.value->'catalog'->>'resource_path' = $1)", + ) + .bind(dbname) + .fetch_all(conn) + .await?) +} + /// Drop a custom instance database: validate, terminate connections, DROP DATABASE, remove from global_settings. pub async fn drop_custom_instance_database(db: &DB, dbname: &str) -> error::Result<()> { + drop_custom_instance_database_on(&mut *db.acquire().await?, dbname).await +} + +async fn drop_custom_instance_database_on( + conn: &mut sqlx::PgConnection, + dbname: &str, +) -> error::Result<()> { let dbname = dbname.trim(); validate_dbname(dbname)?; @@ -1490,7 +1580,7 @@ pub async fn drop_custom_instance_database(db: &DB, dbname: &str) -> error::Resu "SELECT EXISTS (SELECT 1 FROM pg_catalog.pg_database WHERE datname = $1)", dbname ) - .fetch_one(db) + .fetch_one(&mut *conn) .await? .unwrap_or(false); @@ -1501,7 +1591,7 @@ pub async fn drop_custom_instance_database(db: &DB, dbname: &str) -> error::Resu "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = '{}' AND pid <> pg_backend_pid()", dbname.replace('\'', "''") )) - .execute(db) + .execute(&mut *conn) .await { tracing::warn!("Failed to terminate connections to '{}': {}", dbname, e); @@ -1510,7 +1600,7 @@ pub async fn drop_custom_instance_database(db: &DB, dbname: &str) -> error::Resu // Drop the database // SAFETY: `dbname` has been validated via validate_dbname() before reaching this point. sqlx::query(&format!("DROP DATABASE IF EXISTS \"{}\"", dbname)) - .execute(db) + .execute(&mut *conn) .await .map_err(|e| { error::Error::internal_err(format!("Failed to drop database '{}': {}", dbname, e)) @@ -1526,7 +1616,7 @@ pub async fn drop_custom_instance_database(db: &DB, dbname: &str) -> error::Resu r#"UPDATE global_settings SET value = value #- ARRAY['databases', $1] WHERE name = 'custom_instance_pg_databases'"#, dbname ) - .execute(db) + .execute(&mut *conn) .await?; Ok(()) @@ -1600,6 +1690,35 @@ pub async fn create_custom_instance_database( error::Error::internal_err(format!("Failed to create database '{}': {}", dbname, e)) })?; + // Nothing names a database that failed past this point, and its name blocks the retry: drop it + // rather than leave it behind. + if let Err(e) = finish_custom_instance_database(db, dbname, tag).await { + match drop_unused_instance_database(db, dbname).await { + Ok(users) if !users.is_empty() => tracing::warn!( + "Kept '{dbname}' after failing to set it up: workspaces {} use it", + users.join(", ") + ), + Ok(_) => {} + Err(drop_err) => { + tracing::error!("Could not drop '{dbname}' after failing to set it up: {drop_err}") + } + } + return Err(e); + } + + // A data table role can only reach a database it may CONNECT to, and PUBLIC's default CONNECT + // would otherwise let every role in regardless of what this instance defines. Best-effort: a + // failure here leaves the database usable as `admin`, and the next role change repairs it. + if let Err(e) = crate::datatable_roles::converge_connect_grants(db, dbname).await { + tracing::warn!("Could not set CONNECT grants on instance database '{dbname}': {e}"); + } + + tracing::info!("Created custom instance database '{}'", dbname); + Ok(()) +} + +/// Grant `custom_instance_user` its privileges on a database just created, and register it. +async fn finish_custom_instance_database(db: &DB, dbname: &str, tag: &str) -> error::Result<()> { // Grant permissions to custom_instance_user let wmill_pg_creds = PgDatabase::parse_uri(&get_database_url().await?.as_str().await)?; let new_pg_creds = PgDatabase { dbname: dbname.to_string(), ..wmill_pg_creds }; @@ -1634,15 +1753,6 @@ pub async fn create_custom_instance_database( ) .execute(db) .await?; - - // A data table role can only reach a database it may CONNECT to, and PUBLIC's default CONNECT - // would otherwise let every role in regardless of what this instance defines. Best-effort: a - // failure here leaves the database usable as `admin`, and the next role change repairs it. - if let Err(e) = crate::datatable_roles::converge_connect_grants(db, dbname).await { - tracing::warn!("Could not set CONNECT grants on instance database '{dbname}': {e}"); - } - - tracing::info!("Created custom instance database '{}'", dbname); Ok(()) } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 199b2f0906..6f82e3a261 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1304,6 +1304,12 @@ pub struct DataTable { /// nothing local for a fork admin to widen. #[serde(default, skip_serializing_if = "Option::is_none")] pub reference: Option, + /// Set on a *clone* — a terminal entry whose database was copied from the entry this names. The + /// copy holds that entry's rows, so who may connect as which role stays that entry's decision: + /// the clone carries no `permissions` of its own and is governed like a pointer, while + /// connecting to its own database. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub governed_by: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub forked_from: Option, /// Whether the SQL-migrations feature is opted in for this data table. @@ -1362,9 +1368,16 @@ pub const DATATABLE_TENANT_WILDCARD: &str = "*"; /// enough to survive a fork of a fork. const DATATABLE_REFERENCE_MAX_DEPTH: usize = 20; -/// Exactly one of `database` and `reference` must be set. Called wherever an entry is persisted, -/// so nothing downstream has to handle an entry that is both or neither. +/// Exactly one of `database` and `reference` must be set, and a clone's `governed_by` leaves no +/// `permissions` beside it. Called wherever an entry is persisted, so nothing downstream has to +/// handle an entry that is both or neither, or a clone with a decision of its own. pub fn validate_datatable_shape(name: &str, dt: &DataTable) -> Result<()> { + if dt.governed_by.is_some() && (dt.database.is_none() || dt.permissions.is_some()) { + return Err(Error::BadRequest(format!( + "Data table '{name}' is a clone, which owns a database and takes its roles from the \ + data table it was cloned from" + ))); + } match (&dt.database, &dt.reference) { (Some(_), None) | (None, Some(_)) => Ok(()), (Some(_), Some(_)) => Err(Error::BadRequest(format!( @@ -1452,23 +1465,28 @@ pub async fn read_datatable_entry(db: &DB, w_id: &str, name: &str) -> Result(datatable.clone())?) } -/// The terminal entry a reference chain lands on: the workspace that governs the data table, the -/// entry name there, and the entry itself. A terminal entry resolves to itself. +/// The terminal entry a reference chain lands on, and what governs it: the workspace and name of +/// the entry that owns the database, the entry itself, and — for a clone — the entry its +/// `governed_by` chain lands on. A terminal entry that is not a clone resolves to itself and +/// governs itself. /// -/// Every decision downstream — which database to connect to, whose `permissions` apply, whose -/// members tenants are evaluated against, who may administer it — is taken on this, never on the -/// entry the caller named. +/// Every decision downstream is taken on this, never on the entry the caller named. Which database +/// to connect to comes from `workspace_id` / `name` / `datatable.database`; whose `permissions` +/// apply (already in `datatable.permissions`), whose members tenants are evaluated against and who +/// may administer it come from [`GoverningDatatable::governing_workspace_id`]. /// /// Authorization: resolving deliberately crosses into the governing workspace, so it answers for a /// workspace the caller may not belong to and checks nothing itself. It is the input to the /// checks, not one of them: callers MUST pass what it returns to /// [`can_use_datatable_role_in_governing_workspace`] or [`ensure_datatable_admin_access`] before -/// acting on it, and MUST NOT return its `permissions` or `workspace_id` to a caller from +/// acting on it, and MUST NOT return its `permissions` or workspace ids to a caller from /// elsewhere without gating on the answer. pub struct GoverningDatatable { pub workspace_id: String, pub name: String, pub datatable: DataTable, + /// For a clone, the entry whose `permissions` govern it. `None` when the entry governs itself. + pub governor: Option, } impl GoverningDatatable { @@ -1480,6 +1498,13 @@ impl GoverningDatatable { .as_ref() .is_some_and(|d| d.resource_type == DataTableCatalogResourceType::Instance) } + + /// The workspace whose admins administer the data table and whose members its tenants are. + pub fn governing_workspace_id(&self) -> &str { + self.governor + .as_ref() + .map_or(&self.workspace_id, |g| &g.workspace_id) + } } pub async fn resolve_governing_datatable( @@ -1490,6 +1515,9 @@ pub async fn resolve_governing_datatable( let mut workspace_id = w_id.to_string(); let mut name = name.to_string(); let mut hops = 0; + // The clone a `governed_by` chain started from: it keeps its database, and takes the + // `permissions` of wherever the chain lands. + let mut clone: Option<(String, String, DataTable)> = None; for _ in 0..DATATABLE_REFERENCE_MAX_DEPTH { let datatable = read_datatable_entry(db, &workspace_id, &name) .await @@ -1500,21 +1528,48 @@ pub async fn resolve_governing_datatable( // A pointer outlives the workspace it names: deleting one only nulls the fork // lineage, it does not sweep the entries that pointed at it. Say which one is // gone rather than reporting a data table this workspace never had. - Error::NotFound(format!( - "Data table '{name}' of workspace '{workspace_id}' governs this one and no \ - longer exists. A superadmin can point this data table somewhere else." - )) + if clone.is_some() { + Error::NotFound(format!( + "Data table '{name}' of workspace '{workspace_id}', which this clone \ + takes its roles from, no longer exists, so nobody is let into the copy." + )) + } else { + Error::NotFound(format!( + "Data table '{name}' of workspace '{workspace_id}' governs this one and \ + no longer exists. A superadmin can point this data table somewhere \ + else." + )) + } } })?; hops += 1; validate_datatable_shape(&name, &datatable)?; - match &datatable.reference { - None => return Ok(GoverningDatatable { workspace_id, name, datatable }), - Some(reference) => { - workspace_id = reference.workspace_id.clone(); - name = reference.datatable.clone(); + let next = match (&datatable.reference, &datatable.governed_by) { + (Some(reference), _) => reference.clone(), + (None, Some(governed_by)) => { + let governed_by = governed_by.clone(); + if clone.is_none() { + clone = Some((workspace_id.clone(), name.clone(), datatable)); + } + governed_by } - } + (None, None) => { + return Ok(match clone { + None => GoverningDatatable { workspace_id, name, datatable, governor: None }, + Some((clone_w_id, clone_name, mut clone_datatable)) => { + clone_datatable.permissions = datatable.permissions; + GoverningDatatable { + workspace_id: clone_w_id, + name: clone_name, + datatable: clone_datatable, + governor: Some(DataTableReference { workspace_id, datatable: name }), + } + } + }); + } + }; + workspace_id = next.workspace_id; + name = next.datatable; } Err(Error::BadRequest(format!( "Data table '{name}' points at another data table through more than \ @@ -1554,17 +1609,20 @@ pub async fn resolve_workspace_governing_datatables( let mut entries = Entries::new(); let listed = load(db, &[w_id.to_string()], &mut entries).await?; - // (index into `listed`, workspace, entry name) still to be followed. - let mut cursors: Vec<(usize, String, String)> = listed + // (index into `listed`, workspace, entry name, the clone the chain started from) still to be + // followed. A clone keeps its own database and takes the `permissions` of wherever its + // `governed_by` chain lands, as the single resolution does. + type Clone = Option<(String, String, DataTable)>; + let mut cursors: Vec<(usize, String, String, Clone)> = listed .iter() .enumerate() - .map(|(i, name)| (i, w_id.to_string(), name.clone())) + .map(|(i, name)| (i, w_id.to_string(), name.clone(), None)) .collect(); let mut resolved: Vec<(usize, GoverningDatatable)> = vec![]; for _ in 0..DATATABLE_REFERENCE_MAX_DEPTH { let mut next = vec![]; - for (i, ws, name) in cursors.drain(..) { + for (i, ws, name, clone) in cursors.drain(..) { let Some(value) = entries .get(&ws) .and_then(|m| m.get(&name)) @@ -1578,14 +1636,41 @@ pub async fn resolve_workspace_governing_datatables( if validate_datatable_shape(&name, &datatable).is_err() { continue; } - match &datatable.reference { - None => { - resolved.push((i, GoverningDatatable { workspace_id: ws, name, datatable })) - } - Some(reference) => next.push(( + match (&datatable.reference, &datatable.governed_by) { + (Some(reference), _) => next.push(( i, reference.workspace_id.clone(), reference.datatable.clone(), + clone, + )), + (None, Some(governed_by)) => { + let (governor_ws, governor_name) = + (governed_by.workspace_id.clone(), governed_by.datatable.clone()); + let clone = clone.or(Some((ws, name, datatable))); + next.push((i, governor_ws, governor_name, clone)); + } + (None, None) => resolved.push(( + i, + match clone { + None => GoverningDatatable { + workspace_id: ws, + name, + datatable, + governor: None, + }, + Some((clone_ws, clone_name, mut clone_datatable)) => { + clone_datatable.permissions = datatable.permissions; + GoverningDatatable { + workspace_id: clone_ws, + name: clone_name, + datatable: clone_datatable, + governor: Some(DataTableReference { + workspace_id: ws, + datatable: name, + }), + } + } + }, )), } } @@ -1594,7 +1679,7 @@ pub async fn resolve_workspace_governing_datatables( } let to_load: Vec = next .iter() - .map(|(_, ws, _)| ws.clone()) + .map(|(_, ws, _, _)| ws.clone()) .filter(|ws| !entries.contains_key(ws)) .collect::>() .into_iter() @@ -3356,6 +3441,7 @@ mod tests { resource_path: "dt_main".to_string(), }), reference: None, + governed_by: None, forked_from: None, migrations_enabled: None, permissions: None, diff --git a/cli/src/commands/workspace/fork.ts b/cli/src/commands/workspace/fork.ts index 219252bb7a..0aca0984ff 100644 --- a/cli/src/commands/workspace/fork.ts +++ b/cli/src/commands/workspace/fork.ts @@ -239,6 +239,7 @@ async function createWorkspaceFork( interface ForkedDatatableInfo { name: string; new_dbname: string; + fork_behavior?: "schema_only" | "schema_and_data"; } const forkedDatatables: ForkedDatatableInfo[] = []; @@ -285,6 +286,23 @@ async function createWorkspaceFork( const newDbName = `${trueWorkspaceId.replace(/-/g, "_")}__${dt.name}`; + const forkBehavior = dtBehavior as "schema_only" | "schema_and_data"; + // A server that reports `permissioned` copies each data table in the fork request itself, + // and refuses a database copied beforehand. An older one only takes a database copied here. + if (typeof dt.permissioned === "boolean") { + log.info( + colors.blue( + ` Datatable "${dt.name}" will be cloned (${forkBehavior === "schema_only" ? "schema" : "schema + data"}) into "${newDbName}" when the fork is created.` + ) + ); + forkedDatatables.push({ + name: dt.name, + new_dbname: newDbName, + fork_behavior: forkBehavior, + }); + continue; + } + try { log.info( colors.blue(` Creating database "${newDbName}" for datatable "${dt.name}"...`) @@ -300,7 +318,7 @@ async function createWorkspaceFork( log.info( colors.blue( - ` Importing ${dtBehavior === "schema_only" ? "schema" : "schema + data"}...` + ` Importing ${forkBehavior === "schema_only" ? "schema" : "schema + data"}...` ) ); @@ -310,7 +328,7 @@ async function createWorkspaceFork( source: `datatable://${dt.name}`, target: `datatable://${dt.name}`, target_dbname_override: newDbName, - fork_behavior: dtBehavior as "schema_only" | "schema_and_data", + fork_behavior: forkBehavior, }, }); @@ -336,6 +354,8 @@ async function createWorkspaceFork( id: trueWorkspaceId, name: opts.createWorkspaceName ?? workspaceName ?? trueWorkspaceId, color: forkColor, + // So a clone the fork would refuse is refused before any branch is created. + forked_datatables: forkedDatatables, }, }); if (gitSyncJobIds && gitSyncJobIds.length > 0) { diff --git a/frontend/src/lib/components/InstanceSettings.svelte b/frontend/src/lib/components/InstanceSettings.svelte index abe37c3537..d87de125a9 100644 --- a/frontend/src/lib/components/InstanceSettings.svelte +++ b/frontend/src/lib/components/InstanceSettings.svelte @@ -1094,9 +1094,10 @@ the home page’s create menu and hub-project picker are opened and from which entry point, the name of any public hub project imported from the home page and how far that import got, whether a pre-approved trial offer was opened, whether data tables are put - under roles and whether callers name a role or take the default, and which kinds of - access change (grant, revoke, ownership, default privileges) are applied to data - tables, last 30 days)
  • feature adoption (counts of which flow, script, trigger, worker and data table @@ -1163,9 +1164,10 @@ the home page’s create menu and hub-project picker are opened and from which entry point, the name of any public hub project imported from the home page and how far that import got, whether a pre-approved trial offer was opened, whether data tables are put - under roles and whether callers name a role or take the default, and which kinds of - access change (grant, revoke, ownership, default privileges) are applied to data - tables, last 30 days)
  • feature adoption (counts of which flow, script, trigger, worker and data table diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index 801ec13ffb..a3a765014a 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -114,7 +114,12 @@ Loading… {:else}
    - {#if !info.editable} + {#if info.clone} + + Read only: this data table is a clone, and its owners and grants stay as they were copied + from the data table it was cloned from. + + {:else if !info.editable} Read only: access is changed by the admins of the workspace that governs this data table, on Windmill Enterprise Edition. diff --git a/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte b/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte index 2b0962f32e..d0f86b36ad 100644 --- a/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte +++ b/frontend/src/lib/components/workspaceSettings/CreateWorkspaceInner.svelte @@ -380,7 +380,7 @@ failedSyncJobs = [] forkCreationError = '' - // Clone datatables BEFORE creating the workspace fork + // Each data table copy is confirmed first; the fork request makes the copies if (forkDatatableSection) { const queue = forkDatatableSection.buildCloneQueue(prefixed_id) if (queue.length > 0) { @@ -396,6 +396,15 @@ } async function completeFork(prefixed_id: string): Promise { + // The fork request makes these copies, and drops them if the fork is not created + const forkedDatatables = forkDatatableSection + ? forkDatatableSection.getConfirmedCloneJobs().map((job) => ({ + name: job.name, + new_dbname: job._newDbName, + fork_behavior: job.behavior + })) + : [] + let gitSyncJobIds: string[] try { gitSyncJobIds = await WorkspaceService.createWorkspaceForkGitBranch({ @@ -404,6 +413,9 @@ id: prefixed_id, name, color: colorEnabled && workspaceColor ? workspaceColor : undefined, + // Sent in this first phase too, so a clone the fork would refuse is refused before + // any branch is created. + forked_datatables: forkedDatatables, is_dev_workspace: createAsDevWorkspace, dev_workspace_label: createAsDevWorkspace ? devWorkspaceLabel : undefined, // Send the lock intent in this first phase too so the backend can reject a non-admin's @@ -457,14 +469,6 @@ return } - // Build forked_datatables info from completed clone jobs - const forkedDatatables = forkDatatableSection - ? forkDatatableSection.getCompletedCloneJobs().map((job) => ({ - name: job.name, - new_dbname: job._newDbName - })) - : [] - try { await WorkspaceService.createWorkspaceFork({ workspace: baseWorkspaceId!, diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index 8a75027c68..1a08de1351 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -258,6 +258,12 @@ 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 info?.clone_of} + + This data table is a clone of {info.clone_of.datatable} + in workspace {info.clone_of.workspace_id}, so it takes + its roles from there. You are evaluated as a member of that workspace. + {:else if governing} This data table points at the one in workspace {governing} - export type ForkStep = { - label: string - status: 'pending' | 'running' | 'done' | 'error' - error?: string - } - export type DatatableCloneJob = { name: string resourceType: string behavior: 'schema_only' | 'schema_and_data' - steps: ForkStep[] _newDbName: string - _isInstance: boolean - _sourceWorkspace: string - _targetWorkspace: string - _resourcePath: string } @@ -27,7 +16,6 @@ import Label from '../Label.svelte' import Alert from '../common/alert/Alert.svelte' import ConfirmationModal from '../common/confirmationModal/ConfirmationModal.svelte' - import { Check, X, Loader2 } from 'lucide-svelte' interface Props { // Workspace whose datatables are cloned into the fork (the fork's base). Falls back to the @@ -52,111 +40,46 @@ let cloneModalOpen = $state(false) let currentCloneJob: DatatableCloneJob | undefined = $state(undefined) let cloneQueue: DatatableCloneJob[] = $state([]) - let cloneRunning = $state(false) export function hasDatatables(): boolean { return (allDatatables.current?.length ?? 0) > 0 } export function buildCloneQueue(targetWorkspaceId: string): DatatableCloneJob[] { + // A fork attempt starts here: what an earlier attempt confirmed is not this one's to send. + confirmedJobs = [] return (allDatatables.current ?? []) .filter((dt) => { const behavior = datatableBehaviors[dt.name] ?? 'keep_original' return behavior !== 'keep_original' }) - .map((dt) => { - const behavior = datatableBehaviors[dt.name] as 'schema_only' | 'schema_and_data' - const isInstance = dt.resource_type === 'instance' - const newDbName = `${targetWorkspaceId.replace(/-/g, '_')}__${dt.name}` - - const steps: ForkStep[] = [ - { - label: `CREATE DATABASE "${newDbName}"`, - status: 'pending' - }, - { - label: `pg_dump → pg_import (${behavior === 'schema_only' ? 'schema only' : 'schema + data'})`, - status: 'pending' - } - ] - - return { - name: dt.name, - resourceType: dt.resource_type, - behavior, - steps, - _newDbName: newDbName, - _isInstance: isInstance, - _sourceWorkspace: effectiveSource!, - _targetWorkspace: targetWorkspaceId, - _resourcePath: dt.resource_path - } - }) + .map((dt) => ({ + name: dt.name, + resourceType: dt.resource_type, + behavior: datatableBehaviors[dt.name] as 'schema_only' | 'schema_and_data', + // A dev workspace's id has no `wm-fork-` prefix; its copies are named like a fork's + _newDbName: `wm_fork_${targetWorkspaceId.replace(/^wm-fork-/, '').replace(/-/g, '_')}__${dt.name}` + })) } - let completedJobs: DatatableCloneJob[] = $state([]) + let confirmedJobs: DatatableCloneJob[] = $state([]) + // Each clone is only confirmed here: the fork request makes the copies, and drops them if the + // fork is not created. export function startCloning(queue: DatatableCloneJob[]) { - completedJobs = [] + confirmedJobs = [] cloneQueue = queue currentCloneJob = cloneQueue[0] cloneModalOpen = true } - export function getCompletedCloneJobs(): DatatableCloneJob[] { - return completedJobs - } - - async function executeCloneJob(job: DatatableCloneJob) { - cloneRunning = true - let stepIdx = 0 - - // Step 1: Create the database - job.steps[stepIdx].status = 'running' - try { - await WorkspaceService.createPgDatabase({ - workspace: job._sourceWorkspace, - requestBody: { - source: `datatable://${job.name}`, - target_dbname: job._newDbName - } - }) - job.steps[stepIdx].status = 'done' - } catch (e: any) { - job.steps[stepIdx].status = 'error' - job.steps[stepIdx].error = e?.body ?? e?.message ?? String(e) - cloneRunning = false - return - } - stepIdx++ - - // Step 2: Import data - job.steps[stepIdx].status = 'running' - try { - await WorkspaceService.importPgDatabase({ - workspace: job._sourceWorkspace, - requestBody: { - source: `datatable://${job.name}`, - target: `datatable://${job.name}`, - target_dbname_override: job._newDbName, - fork_behavior: job.behavior - } - }) - job.steps[stepIdx].status = 'done' - } catch (e: any) { - job.steps[stepIdx].status = 'error' - job.steps[stepIdx].error = e?.body ?? e?.message ?? String(e) - cloneRunning = false - return - } - stepIdx++ - - cloneRunning = false + export function getConfirmedCloneJobs(): DatatableCloneJob[] { + return confirmedJobs } function advanceCloneQueue() { if (currentCloneJob) { - completedJobs.push(currentCloneJob) + confirmedJobs.push(currentCloneJob) } const idx = cloneQueue.indexOf(currentCloneJob!) if (idx < cloneQueue.length - 1) { @@ -213,13 +136,9 @@ {#if cloneModalOpen && currentCloneJob} { - await executeCloneJob(currentCloneJob!) - advanceCloneQueue() - }} + onConfirmed={advanceCloneQueue} onCanceled={() => { cloneModalOpen = false currentCloneJob = undefined @@ -241,39 +160,16 @@ {#if currentCloneJob.resourceType === 'instance'}

    - This will run CREATE DATABASE {currentCloneJob.steps[0]?.label.match(/"([^"]+)"/)?.[1] ?? ''} on the Windmill PostgreSQL instance. + Creating the fork will run CREATE DATABASE {currentCloneJob._newDbName} on the Windmill + PostgreSQL instance. A data table under roles keeps its owners and grants in the copy, and its + roles stay decided where they are decided today.

    {:else}

    - This will run CREATE DATABASE on the resource's PostgreSQL server. + Creating the fork will run CREATE DATABASE {currentCloneJob._newDbName} on the resource's + PostgreSQL server.

    {/if} - -
    - {#each currentCloneJob.steps as step} -
    - {#if step.status === 'done'} - - {:else if step.status === 'running'} - - {:else if step.status === 'error'} - - {:else} -
    - {/if} - - {step.label} - -
    - {#if step.error} -

    {step.error}

    - {/if} - {/each} -
    +

    If the fork cannot be created, the copy is dropped.

    {/if} From d2480b4bbdba7aa5e78a0980919ad492534f54c4 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Mon, 21 Sep 2026 09:12:08 +0200 Subject: [PATCH 02/32] fix: open the raw app data table drawer when the workspace has none Selecting the first data table of an empty list passed undefined to the name check, which threw instead of opening the drawer on no data table. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/components/raw_apps/RawAppDataTableDrawer.svelte | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte b/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte index 8b9c31f452..0122c032b9 100644 --- a/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte @@ -210,11 +210,17 @@ ) } - function selectDatatable(datatable: string, role?: string) { + // `undefined` where the workspace has no data table at all: the drawer opens on none, and + // its tree says so, rather than on a name that does not exist. + function selectDatatable(datatable: string | undefined, role?: string) { // A row clicked under another data table has just set the selection it should open on. openSchemaKey = selectedSchemaKey openTableKey = selectedTableKey selectedDatatable = datatable + if (datatable === undefined) { + selectedRole = undefined + return + } // A data table opens as the role its picked tables were browsed as, else the one the app // already uses it through. connectAs( From eff0adc17409b48ddf11d927d0a0563b4f7bc5cb Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Mon, 21 Sep 2026 09:45:15 +0200 Subject: [PATCH 03/32] fix(datatables): offer cloning a data table under roles where its grants can be replayed The server clones such a data table and replays the source's owners and grants, which only the Enterprise Edition does, so the fork wizard hid both clone options everywhere instead of on a build that cannot replay them. Co-Authored-By: Claude Opus 5 (1M context) --- .../workspaceSettings/ForkDatatableSection.svelte | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frontend/src/lib/components/workspaceSettings/ForkDatatableSection.svelte b/frontend/src/lib/components/workspaceSettings/ForkDatatableSection.svelte index 10f4d668b8..9627e4a026 100644 --- a/frontend/src/lib/components/workspaceSettings/ForkDatatableSection.svelte +++ b/frontend/src/lib/components/workspaceSettings/ForkDatatableSection.svelte @@ -9,7 +9,7 @@ - +
    - {#if dbSelector} - {@render dbSelector()} - {/if} - {#if dbSupportsSchemas && !multiSelectMode} - e.stopPropagation()} - onchange={() => toggleSchemaSelection(schemaKey)} - /> - - {/if} - {schemaKey} - - {schemaTables.length} - - - -
    - - {#each schemaTables as tableKey} - {@const isDisabled = isTableDisabled(schemaKey, tableKey)} - {@const isChecked = isTableSelected(schemaKey, tableKey) || isDisabled} - {@const isCurrentPreview = - selected.schemaKey === schemaKey && selected.tableKey === tableKey} -
    { - selectTable(schemaKey, tableKey) - toggleTableSelection(schemaKey, tableKey) - }} - onkeydown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - selectTable(schemaKey, tableKey) - toggleTableSelection(schemaKey, tableKey) - } - }} - > - - e.stopPropagation()} - onchange={() => toggleTableSelection(schemaKey, tableKey)} - /> - - -

    {tableKey}

    - + {#if dtOpen} + {#if root.error} +

    {root.error}

    + {/if} + {#each root.schemas as sc (sc.schemaKey)} + {@const schemaOpen = isExpanded(root.datatable, sc.schemaKey)} + {@const indent = root.datatable !== undefined ? 'pl-7' : 'pl-3'} + {#if dbSupportsSchemas} -
    - {/each} - - - {/each} - {:else} - - {#each filteredTableKeys as tableKey} - - - {/each} - {/if} + ]} + btnId={'db-manager-schema-actions-' + onlyAlphaNumAndUnderscore(sc.schemaKey)} + /> + {/if} +
    + + {/if} + + + {#if schemaOpen || !dbSupportsSchemas} + {@const tableIndent = dbSupportsSchemas + ? root.datatable !== undefined + ? 'pl-11' + : 'pl-7' + : root.datatable !== undefined + ? 'pl-7' + : 'pl-3'} + {#each sc.tables as tableKey (tableKey)} + {@const entry = { + datatable: root.datatable, + schema: sc.schemaKey, + table: tableKey + }} + {@const hasMenu = !multiSelectMode} + {@const isSelected = + root.datatable === currentDatatable && + selected.schemaKey === sc.schemaKey && + selected.tableKey === tableKey} + + {/each} + {#if canCreateTableIn(root.datatable, sc.schemaKey)} + + {/if} + {/if} + + {/each} + {#if dbSupportsSchemas && search.trim() === '' && canCreateSchemaIn(root.datatable)} + + {/if} + {/if} + {/each} - {#if !multiSelectMode} - - {/if} - {#if tableKey && colDefs?.[tableKey]?.length} + {#if mainPane} + {@render mainPane()} + {:else if tableKey && colDefs?.[tableKey]?.length} {@const dbTableOps = dbTableOpsFactory({ colDefs: colDefs[tableKey], tableKey, whereClause })} + (aclDrawer = undefined)}> + (aclDrawer = undefined)} + tooltip="Who owns this, and what each role may do with it." + > + {#if aclDrawer && workspace} + {@const dt = aclDrawer.datatable ?? currentDatatable} + {#if dt} + {#key `${dt}~${JSON.stringify(aclDrawer.target)}`} + + {/key} + {/if} + {/if} + + + (askingForConfirmation = undefined)} @@ -754,20 +1164,12 @@ - { - newSchemaDialogOpen = false - newSchemaName = '' - }} -> + { - newSchemaDialogOpen = false - newSchemaName = '' - }} - title="Create a new schema" + on:close={closeSchemaDialog} + title={schemaDialog?.mode === 'rename' + ? `Rename ${schemaDialog.schema}` + : 'Create a new schema'} >
    @@ -779,27 +1181,7 @@ placeholder="Enter schema name..." autofocus on:keydown={(e) => { - if (e.key === 'Enter' && sanitizedNewSchemaName && !schemaAlreadyExists) { - askingForConfirmation = { - confirmationText: `Create ${sanitizedNewSchemaName}`, - type: 'reload', - title: `This will run 'CREATE SCHEMA ${sanitizedNewSchemaName}' on your database. Are you sure?`, - open: true, - id: 'db-create-schema-confirmation-modal', - onConfirm: async () => { - askingForConfirmation && (askingForConfirmation.loading = true) - try { - await dbSchemaOps.onCreateSchema({ schema: sanitizedNewSchemaName }) - refresh?.() - selected.schemaKey = sanitizedNewSchemaName - newSchemaDialogOpen = false - newSchemaName = '' - } finally { - askingForConfirmation = undefined - } - } - } - } + if (e.key === 'Enter') submitSchemaName() }} /> {#if schemaAlreadyExists} @@ -814,32 +1196,8 @@
    {#snippet actions()} - {/snippet}     diff --git a/frontend/src/lib/components/DBManagerContent.svelte b/frontend/src/lib/components/DBManagerContent.svelte index 3ee98538ba..d4e00ce30b 100644 --- a/frontend/src/lib/components/DBManagerContent.svelte +++ b/frontend/src/lib/components/DBManagerContent.svelte @@ -1,16 +1,21 @@ + + (open = false) }} +> + { + // The row underneath folds on click, and picking a role is not that. + e.stopPropagation() + open = !open + }} + > + + {role} + + + anchorEl!.getBoundingClientRect())} + onSelectValue={(item) => { + open = false + if (item.value !== role) onSelect(item.value) + }} + /> + diff --git a/frontend/src/lib/components/DdlMigrationGuard.svelte b/frontend/src/lib/components/DdlMigrationGuard.svelte index 3bda6aa216..0dab262fd0 100644 --- a/frontend/src/lib/components/DdlMigrationGuard.svelte +++ b/frontend/src/lib/components/DdlMigrationGuard.svelte @@ -6,8 +6,18 @@ import { joinSqlStatements, splitSqlRuns } from './sqlDdl' import { logDdlGuardChoice } from './workspaceSettings/datatableTelemetry' import { CornerDownLeft } from 'lucide-svelte' + import { withMigrationRole } from './datatableMigrationRole' - let { workspace, datatable }: { workspace: string; datatable: string } = $props() + let { + workspace, + datatable, + role + }: { + workspace: string + datatable: string + /** The role the editor runs as. The migration declares it, or it would run as admin. */ + role?: string + } = $props() type Choice = 'run' | 'migrate' | 'cancel' @@ -73,7 +83,7 @@ function openMigrationModal(sql: string): Promise { return new Promise((resolve) => { resolveMigrationClosed = (created: boolean) => resolve(created) - newMigrationModal?.open({ codeUp: sql }) + newMigrationModal?.open({ codeUp: withMigrationRole(sql, role) }) }) } @@ -145,6 +155,11 @@ migrations rather than run ad-hoc. Create a migration for it instead? {/if}

    + {#if role} +

    + It will run as role {role}. +

    + {/if} 
    {promptSql}
  • feature adoption (counts of which flow, script, trigger, worker and data table @@ -1163,9 +1164,10 @@ the home page’s create menu and hub-project picker are opened and from which entry point, the name of any public hub project imported from the home page and how far that import got, whether a pre-approved trial offer was opened, whether data tables are put - under roles and whether callers name a role or take the default, and which kinds of - access change (grant, revoke, ownership, default privileges) are applied to data - tables, last 30 days)
  • feature adoption (counts of which flow, script, trigger, worker and data table diff --git a/frontend/src/lib/components/SqlRepl.svelte b/frontend/src/lib/components/SqlRepl.svelte index d6c2d288e6..d58ac65898 100644 --- a/frontend/src/lib/components/SqlRepl.svelte +++ b/frontend/src/lib/components/SqlRepl.svelte @@ -225,5 +225,10 @@ {#if datatableName && ws} - + {/if} diff --git a/frontend/src/lib/components/Star.svelte b/frontend/src/lib/components/Star.svelte index dd91f671c8..4f0f169109 100644 --- a/frontend/src/lib/components/Star.svelte +++ b/frontend/src/lib/components/Star.svelte @@ -9,9 +9,10 @@ kind: FavoriteKind summary?: string workspaceId?: string + size?: number } - let { path, kind, workspaceId, summary }: Props = $props() + let { path, kind, workspaceId, summary, size = 16 }: Props = $props() let buttonHover = $state(false) let starred = $derived(favoriteManager.isStarred(path, kind)) @@ -31,14 +32,14 @@ > {#if starred} {#if buttonHover} - + {:else} - + {/if} {:else} {/if} diff --git a/frontend/src/lib/components/apps/components/display/dbtable/utils.ts b/frontend/src/lib/components/apps/components/display/dbtable/utils.ts index c0477e8408..f2337d699b 100644 --- a/frontend/src/lib/components/apps/components/display/dbtable/utils.ts +++ b/frontend/src/lib/components/apps/components/display/dbtable/utils.ts @@ -282,7 +282,8 @@ const scriptsV2: typeof legacyScripts = { ...legacyScripts.postgresql, code: ` SELECT table_name, column_name, udt_name, column_default, is_nullable, nsp.nspname AS table_schema FROM information_schema.columns -RIGHT JOIN pg_namespace nsp ON table_schema = nsp.nspname WHERE nsp.nspname NOT IN ('information_schema', 'pg_toast', 'pg_catalog')` +RIGHT JOIN pg_namespace nsp ON table_schema = nsp.nspname WHERE nsp.nspname NOT IN ('information_schema', 'pg_toast', 'pg_catalog') +AND NOT starts_with(nsp.nspname, 'pg_') AND has_schema_privilege(nsp.oid, 'USAGE')` } } diff --git a/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte b/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte index bfba6a6a82..524817ba34 100644 --- a/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte +++ b/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte @@ -197,6 +197,7 @@ one long unbreakable string (a path list, a URL) sizes this column by that string and pushes it out of the panel — and any `truncate` inside never engages. --> +

    {title} diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 71c06d6194..21c3d24770 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -732,12 +732,14 @@ export class AIChatManager implements ChatViewHost { /** Every mounted flow editor. */ #flowEditors = new Set() appAiChatHelpers = $state(undefined) - /** Datatable creation policy: enabled flag, datatable name, and optional schema */ + /** Datatable creation policy: enabled flag, datatable name, optional schema, and the role the + * app uses each data table through */ datatableCreationPolicy = $state<{ enabled: boolean datatable: string | undefined schema: string | undefined - }>({ enabled: false, datatable: undefined, schema: undefined }) + roles?: Record + }>({ enabled: false, datatable: undefined, schema: undefined, roles: undefined }) pendingNewCode = $state(undefined) apiTools = $state[]>([]) aiChatInput = $state(null) diff --git a/frontend/src/lib/components/copilot/chat/DatatableCreationPolicy.svelte b/frontend/src/lib/components/copilot/chat/DatatableCreationPolicy.svelte index 3aec0fa2b6..6b4cbbd710 100644 --- a/frontend/src/lib/components/copilot/chat/DatatableCreationPolicy.svelte +++ b/frontend/src/lib/components/copilot/chat/DatatableCreationPolicy.svelte @@ -69,6 +69,7 @@ diff --git a/frontend/src/lib/components/copilot/chat/app/core.ts b/frontend/src/lib/components/copilot/chat/app/core.ts index 5ab271cc6f..634341b966 100644 --- a/frontend/src/lib/components/copilot/chat/app/core.ts +++ b/frontend/src/lib/components/copilot/chat/app/core.ts @@ -18,6 +18,7 @@ import { type AppCodeSelectionElement, type AppDatatableElement } from '../context' +import { appDatatableRole, sdkDatatableCall } from '$lib/components/raw_apps/dataTableRefUtils' // Backend runnable types export type BackendRunnableType = 'script' | 'flow' | 'hubscript' | 'inline' @@ -921,9 +922,20 @@ export function prepareAppSystemMessage(customPrompt?: string): ChatCompletionSy const policy = aiChatManager.datatableCreationPolicy const datatableName = policy.datatable ?? 'main' const schemaPrefix = policy.schema ? `${policy.schema}.` : '' - // Use wmill.datatable() for 'main' (default), otherwise wmill.datatable('name') - const datatableCall = - datatableName === 'main' ? 'wmill.datatable()' : `wmill.datatable('${datatableName}')` + // A role names the privileges the app's queries run with, so it has to be in the code the + // model writes. + const datatableRole = appDatatableRole(policy.roles, datatableName) + const tsDatatableCall = sdkDatatableCall(datatableName, datatableRole, 'typescript') + const pyDatatableCall = sdkDatatableCall(datatableName, datatableRole, 'python') + const roleEntries = Object.entries(policy.roles ?? {}) + const rolesNote = + roleEntries.length > 0 + ? `\n\nThis app uses these data tables through a role: ${roleEntries + .map(([dt, role]) => `\`${dt}\` as \`${role}\``) + .join( + ', ' + )}. Always pass that role when calling \`wmill.datatable\` on them, as in the examples. The role only reaches what it was granted, so a query on a table it lacks privileges on fails with \`permission denied\`.` + : '' let content = `You are a helpful assistant that creates and edits apps on the Windmill platform. Apps are defined as a collection of files that contains both the frontend and the backend. @@ -1024,7 +1036,7 @@ Backend runnables should only perform **data operations** (SELECT, INSERT, UPDAT import * as wmill from 'windmill-client'; export async function main(user_id: string) { - const sql = ${datatableCall}; + const sql = ${tsDatatableCall}; const user = await sql\`SELECT * FROM ${schemaPrefix}users WHERE id = \${user_id}\`.fetchOne(); return user; } @@ -1035,12 +1047,12 @@ export async function main(user_id: string) { import wmill def main(user_id: str): - db = ${datatableCall} + db = ${pyDatatableCall} user = db.query('SELECT * FROM ${schemaPrefix}users WHERE id = $1', user_id).fetch_one() return user \`\`\` -Use these examples for normal datatable access. +Use these examples for normal datatable access.${rolesNote} ### Schema Modifications (DDL) - Use exec_datatable_sql tool ONLY diff --git a/frontend/src/lib/components/copilot/chat/datatableTools.ts b/frontend/src/lib/components/copilot/chat/datatableTools.ts index 7232500898..bb7976d396 100644 --- a/frontend/src/lib/components/copilot/chat/datatableTools.ts +++ b/frontend/src/lib/components/copilot/chat/datatableTools.ts @@ -2,6 +2,7 @@ import { z } from 'zod' import { WorkspaceService, type CompletedJob } from '$lib/gen' import type { DataTableTables } from '$lib/gen/types.gen' import { runScript } from '$lib/components/jobs/utils' +import { datatableReference } from '$lib/components/dbTypes' import { createToolDef, executeTestRun, @@ -15,9 +16,9 @@ import { * * Datatables are workspace-level managed PostgreSQL databases. The backend * endpoints used here (`list_datatable_tables`, `get_datatable_table_schema`) - * and SQL execution (`datatable://`) are gated only by workspace - * membership, so these tools need no app context and operate directly on the - * workspace. This is the unrestricted counterpart to the app-mode datatable + * and SQL execution (`datatable://`) need no app context: the server + * decides what the caller reaches, as the datatable role they name or its + * default. This is the unrestricted counterpart to the app-mode datatable * tools in `app/core.ts`, which additionally filter by the app's whitelist. */ @@ -31,9 +32,19 @@ const memo = (factory: () => T): (() => T) => { // ============= Pure workspace-scoped operations ============= -/** List all datatables configured in the workspace, with their schema/table names. */ -export async function listDatatables(workspace: string): Promise { - return await WorkspaceService.listDataTableTables({ workspace }) +/** List the datatables configured in the workspace, with their schema/table names: all of them as + * their default role, or only `datatableName`, as `role` when one is given. */ +export async function listDatatables( + workspace: string, + datatableName?: string, + role?: string +): Promise { + if (datatableName === undefined) return await WorkspaceService.listDataTableTables({ workspace }) + return await WorkspaceService.listDataTableTables({ + workspace, + datatableName, + ...(role !== undefined && { roleFor: datatableName, role }) + }) } /** Get the columns (column_name -> compact_type) of one datatable table. */ @@ -41,13 +52,15 @@ export async function getDatatableColumns( workspace: string, datatableName: string, schemaName: string, - tableName: string + tableName: string, + role?: string ): Promise> { const schema = await WorkspaceService.getDataTableTableSchema({ workspace, datatableName, schemaName, - tableName + tableName, + role }) return schema.columns } @@ -81,7 +94,26 @@ const NO_DATATABLES_CONFIGURED_MESSAGE = // ============= Tool definitions ============= -const getListDatatablesSchema = memo(() => z.object({})) +// The same rule the server applies to `-- role `; a name it would refuse fails here instead. +const getRoleSchema = memo(() => + z + .string() + .regex(/^[A-Za-z0-9_-]{1,63}$/) + .optional() + .describe( + "The datatable role to connect as, when the code you are working on uses one (an app's `data.roles` entry, or the `role` it passes to wmill.datatable). Omit for the datatable's default role." + ) +) + +const getListDatatablesSchema = memo(() => + z.object({ + datatable_name: z + .string() + .optional() + .describe('List only this datatable. Required with `role`.'), + role: getRoleSchema() + }) +) const getListDatatablesToolDef = memo(() => createToolDef( getListDatatablesSchema(), @@ -94,7 +126,8 @@ const getGetDatatableTableSchemaSchema = memo(() => z.object({ datatable_name: z.string().describe('The datatable name to inspect, e.g. "main".'), schema_name: z.string().describe('The schema name, e.g. "public".'), - table_name: z.string().describe('The table name to inspect.') + table_name: z.string().describe('The table name to inspect.'), + role: getRoleSchema() }) ) const getGetDatatableTableSchemaToolDef = memo(() => @@ -117,6 +150,7 @@ const getExecDatatableSqlSchema = memo(() => .describe( 'The SQL query to execute. Supports SELECT, INSERT, UPDATE, DELETE, CREATE TABLE, ALTER TABLE, DROP TABLE, etc. For SELECT queries, results are returned as an array of objects. A newly created table will appear in list_datatables automatically.' ), + role: getRoleSchema(), background: z .boolean() .optional() @@ -217,10 +251,18 @@ export function getDatatableTools(): Tool<{}>[] { { def: getListDatatablesToolDef(), planModeSafe: true, - fn: async ({ workspace, toolId, toolCallbacks }) => { + fn: async ({ args, workspace, toolId, toolCallbacks }) => { toolCallbacks.setToolStatus(toolId, { content: 'Listing datatables...' }) try { - const metadata = await listDatatables(workspace) + const parsedArgs = getListDatatablesSchema().parse(args ?? {}) + if (parsedArgs.role !== undefined && parsedArgs.datatable_name === undefined) { + throw new Error('`role` needs `datatable_name`, the datatable it is a role of') + } + const metadata = await listDatatables( + workspace, + parsedArgs.datatable_name, + parsedArgs.role + ) if (metadata.length === 0) { toolCallbacks.setToolStatus(toolId, { content: 'No datatables configured — set one up in workspace settings' @@ -236,7 +278,18 @@ export function getDatatableTools(): Tool<{}>[] { toolCallbacks.setToolStatus(toolId, { content: `Listed ${metadata.length} datatable(s) with ${totalTables} table(s)` }) - return JSON.stringify(metadata, null, 2) + // Only what the model acts on: the roles it may pass, not the creation privileges + // the manager's UI gates on. + return JSON.stringify( + metadata.map((d) => ({ + datatable_name: d.datatable_name, + schemas: d.schemas, + ...(d.error && { error: d.error }), + ...(d.permissioned && { usable_roles: d.usable_roles, default_role: d.default_role }) + })), + null, + 2 + ) } catch (e) { const errorMsg = `Error listing datatables: ${e instanceof Error ? e.message : String(e)}` toolCallbacks.setToolStatus(toolId, { content: errorMsg, error: errorMsg }) @@ -257,7 +310,8 @@ export function getDatatableTools(): Tool<{}>[] { workspace, parsedArgs.datatable_name, parsedArgs.schema_name, - parsedArgs.table_name + parsedArgs.table_name, + parsedArgs.role ) toolCallbacks.setToolStatus(toolId, { content: `Retrieved schema for ${parsedArgs.schema_name}.${parsedArgs.table_name}` @@ -300,7 +354,7 @@ export function getDatatableTools(): Tool<{}>[] { requestBody: { language: 'postgresql', content: parsedArgs.sql, - args: { database: `datatable://${name}` } + args: { database: datatableReference(name, parsedArgs.role) } } }), workspace, diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 2e68b74fe1..6c85a75a4e 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -1434,7 +1434,12 @@ Data Tables: - Datatables are workspace-scoped managed PostgreSQL databases, shared across the workspace (not owned by any single app). They must be configured by the user in their workspace settings (Workspace settings → Data Tables); they cannot be created via SQL. - Use list_datatables to discover the available datatables and their tables. Reuse an existing table rather than creating a duplicate. If list_datatables reports none, this is a blocking prerequisite — tell the user to set up a datatable in their workspace settings and stop; do not assume a "main" datatable exists or call exec_datatable_sql. - Use get_datatable_table_schema only when you need a table's column names/types; list_datatables is enough for table-list or availability summaries. -- Use exec_datatable_sql to explore data, run queries, mutate rows, or change schema (CREATE/ALTER/DROP). Creating a table is a normal CREATE TABLE statement — it appears in list_datatables afterward, with no registration step. +- Use exec_datatable_sql to explore data, run queries, mutate rows, or change schema (CREATE/ALTER/DROP). Creating a table is a normal CREATE TABLE statement — it appears in list_datatables afterward, with no registration step.${ + isCloudHosted() + ? '' + : ` +- A raw app may use a datatable through a role (\`data.roles\` in its raw_app.yaml). When working on such an app, pass that role to the datatable tools, and to wmill.datatable in its runnables, so you see and change only what the app itself can.` + } - When writing runnable code (inline app runnables, scripts, flow modules) that reads or writes datatable data at runtime, it accesses a datatable via wmill.datatable(). Default to TypeScript (bun) unless the user asked for another language. Call get_instructions with subject "datatable" and language "bun" for the TypeScript SQL SDK reference (or language "python3" for Python) — it returns only that language so you get just what you need.${ skills.length > 0 ? ` diff --git a/frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte b/frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte deleted file mode 100644 index cbfb43ad39..0000000000 --- a/frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte +++ /dev/null @@ -1,50 +0,0 @@ - - -
    - ({ value: t, label: t }))} - bind:value={table} - placeholder="The whole schema" - clearable - size="sm" - class="w-56" - /> - {/if} -
    diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index e0ae596a3d..a3a765014a 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -25,30 +25,24 @@ let { workspace, datatable, - target, - onLoaded + target }: { workspace: string datatable: string /** What owner and grants are read and written for. */ target: AclTarget - /** Each read, with the target it was made for: it also lists what the target holds. */ - onLoaded?: (target: AclTarget, info: DatatableAclInfo) => void } = $props() const acl = resource( () => [workspace, datatable, target] as const, - async ([ws, dt, t]) => { - const loaded = await WorkspaceService.getDatatableAcl({ + async ([ws, dt, t]) => + await WorkspaceService.getDatatableAcl({ workspace: ws, datatableName: dt, kind: t.kind, schema: t.kind === 'database' ? undefined : t.schema, table: t.kind === 'table' ? t.table : undefined }) - onLoaded?.(t, loaded) - return loaded - } ) // Nothing is written before its SQL has been shown, and the apply runs exactly that SQL: the @@ -120,7 +114,12 @@ Loading… {:else}
    - {#if !info.editable} + {#if info.clone} + + Read only: this data table is a clone, and its owners and grants stay as they were copied + from the data table it was cloned from. + + {:else if !info.editable} Read only: access is changed by the admins of the workspace that governs this data table, on Windmill Enterprise Edition. diff --git a/frontend/src/lib/components/datatableMigrationRole.test.ts b/frontend/src/lib/components/datatableMigrationRole.test.ts new file mode 100644 index 0000000000..6d136b8371 --- /dev/null +++ b/frontend/src/lib/components/datatableMigrationRole.test.ts @@ -0,0 +1,60 @@ +import { describe, test, expect } from 'vitest' +import { parseMigrationRole, withMigrationRole } from './datatableMigrationRole' + +describe('parseMigrationRole', () => { + test('reads every spelling the server accepts from the leading comment block', () => { + for (const line of [ + '-- role analyst', + '-- Role: analyst', + '-- role=analyst', + '-- role analyst;' + ]) { + expect(parseMigrationRole(`\n${line}\nBEGIN;\nEND;`)).toEqual({ + kind: 'role', + role: 'analyst' + }) + } + }) + + test('an annotation below BEGIN is not one', () => { + expect(parseMigrationRole('BEGIN;\n-- role analyst\nEND;')).toEqual({ kind: 'none' }) + }) + + test('a malformed attempt is an error, not the default', () => { + for (const line of [ + '-- role based access below', + '-- role', + '-- role:', + '-- role an;alytics' + ]) { + expect(parseMigrationRole(`${line}\nBEGIN;`)).toEqual({ kind: 'malformed', line }) + } + }) + + test('comments that do not start with the word role are ignored', () => { + expect(parseMigrationRole('-- roles analyst\n-- rolex\nBEGIN;')).toEqual({ kind: 'none' }) + }) +}) + +describe('withMigrationRole', () => { + test('leads above BEGIN, so the server reads it', () => { + const out = withMigrationRole('BEGIN;\nSELECT 1;\nEND;', 'analyst') + expect(out).toBe('-- role analyst\nBEGIN;\nSELECT 1;\nEND;') + }) + + test('replaces any attempt rather than stacking, malformed ones included', () => { + const out = withMigrationRole( + '-- Role: auditor\n-- role oops no\n-- keep me\nBEGIN;', + 'analyst' + ) + expect(out).toBe('-- role analyst\n-- keep me\nBEGIN;') + }) + + test('undefined strips the annotation, so it runs as admin', () => { + expect(withMigrationRole('-- role analyst\n\nBEGIN;\nEND;', undefined)).toBe('BEGIN;\nEND;') + }) + + test('refuses a name the server would refuse', () => { + expect(() => withMigrationRole('BEGIN;', 'bad;name')).toThrow() + }) +}) diff --git a/frontend/src/lib/components/datatableMigrationRole.ts b/frontend/src/lib/components/datatableMigrationRole.ts new file mode 100644 index 0000000000..5475bf2fd7 --- /dev/null +++ b/frontend/src/lib/components/datatableMigrationRole.ts @@ -0,0 +1,70 @@ +import { isDatatableRoleName } from './dbTypes' + +/** + * A migration carries the data table role it runs as in its own SQL, as a `-- role ` + * annotation. There is no separate field: the annotation is what the server reads, and keeping + * it in the SQL is what lets it survive a `wmill sync` round-trip. + * + * Mirrors `SqlAnnotations::datatable_role` on the backend. It is only read from the leading + * comment block, so an annotation below `BEGIN;` is ignored and the migration runs as admin. A + * leading comment whose first word is `role` is an annotation attempt, and a malformed one is an + * error there, so it is one here too. + */ + +export type MigrationRole = + | { kind: 'none' } + | { kind: 'role'; role: string } + | { kind: 'malformed'; line: string } + +/** The body of a leading comment line that attempts a role annotation, or undefined. */ +function roleAttempt(line: string): string | undefined { + if (!line.startsWith('--')) return undefined + const body = line.slice(2).trimStart() + if (body.slice(0, 4).toLowerCase() !== 'role') return undefined + const after = body.slice(4) + if (after !== '' && !/^[\s:=]/.test(after)) return undefined + return after +} + +function parseAttempt(after: string): string | undefined { + let rest = after.trimStart() + if (rest.startsWith(':') || rest.startsWith('=')) rest = rest.slice(1) + const tokens = rest.split(/\s+/).filter((t) => t !== '') + if (tokens.length !== 1) return undefined + const role = tokens[0].endsWith(';') ? tokens[0].slice(0, -1) : tokens[0] + return isDatatableRoleName(role) ? role : undefined +} + +export function parseMigrationRole(sql: string): MigrationRole { + for (const raw of sql.split('\n')) { + const line = raw.trim() + if (line === '') continue + if (!line.startsWith('--')) break + const after = roleAttempt(line) + if (after === undefined) continue + const role = parseAttempt(after) + return role === undefined ? { kind: 'malformed', line } : { kind: 'role', role } + } + return { kind: 'none' } +} + +/** + * `sql` declaring `role`: any role annotation attempt in the leading comment block is removed, + * and `-- role ` is prepended above everything, or nothing when `role` is undefined. + */ +export function withMigrationRole(sql: string, role: string | undefined): string { + if (role !== undefined && !isDatatableRoleName(role)) { + throw new Error(`Invalid data table role '${role}'`) + } + const lines = sql.split('\n') + const kept: string[] = [] + let i = 0 + for (; i < lines.length; i++) { + const line = lines[i].trim() + if (line !== '' && !line.startsWith('--')) break + if (roleAttempt(line) === undefined) kept.push(lines[i]) + } + const rest = [...kept, ...lines.slice(i)] + while (rest.length > 0 && rest[0].trim() === '') rest.shift() + return role === undefined ? rest.join('\n') : [`-- role ${role}`, ...rest].join('\n') +} diff --git a/frontend/src/lib/components/datatableUsableRoles.test.ts b/frontend/src/lib/components/datatableUsableRoles.test.ts new file mode 100644 index 0000000000..cca4ab7607 --- /dev/null +++ b/frontend/src/lib/components/datatableUsableRoles.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it, vi } from 'vitest' + +const request = vi.fn() +vi.mock('$lib/gen/core/request', () => ({ request: (...args: unknown[]) => request(...args) })) +vi.mock('$lib/gen', () => ({ OpenAPI: {} })) +vi.mock('$lib/cloud', () => ({ isCloudHosted: () => false })) + +import { listUsableDatatableRoles } from './datatableUsableRoles' + +const refuse = (body: string) => () => + Promise.reject(Object.assign(new Error('Bad Request'), { body })) + +describe('listUsableDatatableRoles', () => { + // The refusal is recognised by its sentence, so rewording it on one side alone would turn every + // data table on a build without the Enterprise Edition into a failed lookup rather than one + // that is not under roles. + it('reads the enterprise refusal as not under roles, and rethrows anything else', async () => { + request.mockImplementation(refuse('Data table roles are a Windmill Enterprise Edition feature')) + expect(await listUsableDatatableRoles('ws', 'main')).toEqual({ + permissioned: false, + roles: [], + default_role: 'admin' + }) + + request.mockImplementation(refuse('Data table not found')) + const rethrown = await listUsableDatatableRoles('ws', 'main').catch((e) => e) + expect(rethrown).toBeInstanceOf(Error) + }) +}) diff --git a/frontend/src/lib/components/datatableUsableRoles.ts b/frontend/src/lib/components/datatableUsableRoles.ts new file mode 100644 index 0000000000..84e52f29a8 --- /dev/null +++ b/frontend/src/lib/components/datatableUsableRoles.ts @@ -0,0 +1,44 @@ +import { OpenAPI, type ListUsableDatatableRolesResponse } from '$lib/gen' +import { request } from '$lib/gen/core/request' +import { isCloudHosted } from '$lib/cloud' +import { ADMIN_DATATABLE_ROLE } from './dbTypes' + +// `datatable_roles_unavailable` on the server, which is a plain 400: rewording it there without +// here makes every role picker on a non-Enterprise build fail instead of reading "not under roles". +const ROLES_UNAVAILABLE = 'Data table roles are a Windmill Enterprise Edition feature' + +const NOT_UNDER_ROLES: ListUsableDatatableRolesResponse = { + permissioned: false, + roles: [], + default_role: ADMIN_DATATABLE_ROLE +} + +/** + * The roles the caller may connect as on a data table. Cloud has no instance database, so no data + * table there is under roles, and none of the role pickers show. Without the Enterprise Edition + * every roles route refuses, which reads the same way: the data table is then used the way it was + * before roles, and one that is under roles is refused when something connects to it. + */ +export async function listUsableDatatableRoles( + workspace: string, + datatableName: string +): Promise { + if (isCloudHosted()) return NOT_UNDER_ROLES + try { + // The generated client encodes path params with `encodeURI`, which leaves a '?' in a + // data table name created before names were restricted to cut the path short. + return await request( + { ...OpenAPI, ENCODE_PATH: encodeURIComponent }, + { + method: 'GET', + url: '/w/{workspace}/workspaces/datatable_usable_roles/{datatable_name}', + path: { workspace, datatable_name: datatableName } + } + ) + } catch (e) { + const body = (e as { body?: unknown })?.body + const detail = `${typeof body === 'string' ? body : JSON.stringify(body ?? '')} ${(e as Error)?.message ?? e}` + if (detail.includes(ROLES_UNAVAILABLE)) return NOT_UNDER_ROLES + throw e + } +} diff --git a/frontend/src/lib/components/dbManagerDrawerModel.svelte.ts b/frontend/src/lib/components/dbManagerDrawerModel.svelte.ts index 59345c6ae7..890635dcf8 100644 --- a/frontend/src/lib/components/dbManagerDrawerModel.svelte.ts +++ b/frontend/src/lib/components/dbManagerDrawerModel.svelte.ts @@ -5,7 +5,7 @@ import { isDbType } from './dbTypes' /** * Single URL param `dbm` encodes the full DB manager state: - * firstSegment~path~schema.table + * firstSegment~path~schema.table~role=name * * firstSegment: * datatable – database with datatable:// resource (resourceType always postgresql) @@ -26,28 +26,46 @@ import { isDbType } from './dbTypes' * datatable~main~.customers (schema "public" implied) * ducklake~main~.orders (schema "main" implied) * postgresql~$res:u/user/my_pg~public.customers + * datatable~main~.customers~role=analyst + * datatable~main~role=analyst (no schema/table selected) + * + * role=name (last segment, optional, data tables only): the data table role to connect as. + * Omitted means the data table's default role. A trailing segment starting with `role=` is always + * the role, whatever follows. The name is kept as written, even when invalid (a `.` included), so + * the connection refuses it visibly instead of falling back to the default. */ const dbManagerSchema = z.object({ dbm: z.string().nullable() }) -interface ParsedDbm { +export interface ParsedDbm { type: 'database' | 'datatable' | 'ducklake' path: string resType?: string schema?: string table?: string + role?: string } -function parseDbm(raw: unknown): ParsedDbm | null { +const ROLE_SEGMENT_PREFIX = 'role=' + +function isRoleSegment(segment: string | undefined): segment is string { + return !!segment && segment.startsWith(ROLE_SEGMENT_PREFIX) +} + +export function parseDbm(raw: unknown): ParsedDbm | null { if (!raw || typeof raw !== 'string') return null const parts = raw.split('~') if (parts.length < 2 || !parts[1]) return null const firstSeg = parts[0] const path = parts[1] - const schemaTable = parts[2] ?? '' + const rest = parts.slice(2) + const role = isRoleSegment(rest.at(-1)) + ? rest.pop()!.slice(ROLE_SEGMENT_PREFIX.length) + : undefined + const schemaTable = rest[0] ?? '' let type: ParsedDbm['type'] let resType: string | undefined @@ -81,12 +99,12 @@ function parseDbm(raw: unknown): ParsedDbm | null { schema = defaultSchemas[type] } - return { type, path, resType, schema, table } + return { type, path, resType, schema, table, role: type === 'datatable' ? role : undefined } } const defaultSchemas: Record = { datatable: 'public', ducklake: 'main' } -function buildDbm(p: ParsedDbm): string { +export function buildDbm(p: ParsedDbm): string { const firstSeg = p.type === 'database' ? p.resType! : p.type const schema = p.schema === defaultSchemas[p.type] ? undefined : p.schema let schemaTable = '' @@ -97,7 +115,12 @@ function buildDbm(p: ParsedDbm): string { } else if (schema) { schemaTable = `${schema}.` } - return schemaTable ? `${firstSeg}~${p.path}~${schemaTable}` : `${firstSeg}~${p.path}` + const segments = [firstSeg, p.path] + if (schemaTable) segments.push(schemaTable) + if (p.type === 'datatable' && p.role !== undefined) { + segments.push(`${ROLE_SEGMENT_PREFIX}${p.role}`) + } + return segments.join('~') } export interface DbManagerUriState { @@ -105,6 +128,8 @@ export interface DbManagerUriState { readonly effectiveInput: DbInput | undefined readonly isDatatableInput: boolean selectedDatatable: string | undefined + /** The data table role the drawer connects as; undefined means its default. */ + selectedRole: string | undefined selectedSchema: string | undefined selectedTable: string | undefined readonly open: boolean @@ -137,6 +162,7 @@ export function useDbManagerUriState(): DbManagerUriState { type: 'database' as const, resourceType: resType as DbType, resourcePath: parsed.type === 'datatable' ? `datatable://${parsed.path}` : parsed.path, + role: parsed.role, specificSchema: parsed.schema, specificTable: parsed.table } @@ -163,6 +189,7 @@ export function useDbManagerUriState(): DbManagerUriState { type: isDatatable ? 'datatable' : 'database', path: isDatatable ? nInput.resourcePath.slice('datatable://'.length) : nInput.resourcePath, resType: isDatatable ? undefined : nInput.resourceType, + role: isDatatable ? nInput.role : undefined, schema: nInput.specificSchema, table: nInput.specificTable }) @@ -194,7 +221,14 @@ export function useDbManagerUriState(): DbManagerUriState { return parsed?.type === 'datatable' ? parsed.path : undefined }, set selectedDatatable(v: string | undefined) { - if (v) updateField({ path: v }) + // A role belongs to one data table, so it cannot carry over to another. + if (v) updateField({ path: v, role: undefined }) + }, + get selectedRole() { + return parsed?.role + }, + set selectedRole(v: string | undefined) { + updateField({ role: v }) }, get selectedSchema() { return parsed?.schema diff --git a/frontend/src/lib/components/dbManagerRole.test.ts b/frontend/src/lib/components/dbManagerRole.test.ts new file mode 100644 index 0000000000..6c45d0ed30 --- /dev/null +++ b/frontend/src/lib/components/dbManagerRole.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from 'vitest' +import { buildDbm, parseDbm } from './dbManagerDrawerModel.svelte' +import { schemaCacheKey } from './dbSchemaCache' +import { datatableReference, type DbInput } from './dbTypes' + +describe('dbm role segment', () => { + it('round-trips a role, with and without a table', () => { + for (const dbm of ['datatable~main~.orders~role=p4_analytics', 'datatable~main~role=p4-op']) { + expect(buildDbm(parseDbm(dbm)!)).toBe(dbm) + } + expect(parseDbm('datatable~main~sales.orders~role=analyst')).toMatchObject({ + path: 'main', + schema: 'sales', + table: 'orders', + role: 'analyst' + }) + }) + + it('reads a link without a role as the default role', () => { + const parsed = parseDbm('datatable~main~.orders')! + expect(parsed.role).toBeUndefined() + expect(parsed).toMatchObject({ schema: 'public', table: 'orders' }) + expect(buildDbm(parsed)).toBe('datatable~main~.orders') + }) + + it('keeps an invalid role as written, so the connection refuses it', () => { + expect(parseDbm('datatable~main~role=a;b')?.role).toBe('a;b') + // A dot does not turn it into a schema.table selection read as the default role. + expect(parseDbm('datatable~main~role=bad.name')).toMatchObject({ + role: 'bad.name', + schema: undefined, + table: undefined + }) + }) +}) + +describe('connecting as a role', () => { + const input = (role?: string): DbInput => ({ + type: 'database', + resourceType: 'postgresql', + resourcePath: 'datatable://main', + role + }) + + // What `getDatabaseArg` builds every DB manager connection from. + it('appends the role to the data table reference', () => { + expect(datatableReference('main', 'p4_analytics')).toBe('datatable://main?role=p4_analytics') + expect(datatableReference('main', undefined)).toBe('datatable://main') + }) + + it('never appends a role to a name containing ?', () => { + // The server reads such a whole reference as the stored name first, so `?role=` would + // be taken as part of the name or refused instead of picking the role. + expect(datatableReference('legacy?x', undefined)).toBe('datatable://legacy?x') + expect(() => datatableReference('sales?role=analytics', 'admin')).toThrow(/'\?' in its name/) + }) + + it('refuses a role name the server would not accept', () => { + expect(() => datatableReference('main', 'a&role=admin')).toThrow(/Invalid data table role/) + expect(() => datatableReference('main', '')).toThrow(/Invalid data table role/) + }) + + it('keys the schema cache by role', () => { + expect(schemaCacheKey('ws', input('a'))).not.toBe(schemaCacheKey('ws', input('b'))) + expect(schemaCacheKey('ws', input('a'))).not.toBe(schemaCacheKey('ws', input())) + }) +}) diff --git a/frontend/src/lib/components/dbOps.ts b/frontend/src/lib/components/dbOps.ts index 5005cc931c..e239a9e957 100644 --- a/frontend/src/lib/components/dbOps.ts +++ b/frontend/src/lib/components/dbOps.ts @@ -8,7 +8,8 @@ import { runScriptAndPollResult } from './jobs/utils' import { writingJobOptions } from './jobs/writingJob' import type { DBSchema, SQLSchema } from '$lib/stores' import { stringifySchema } from './copilot/lib' -import type { DbInput, DbType } from './dbTypes' +import { datatableReference, type DbInput, type DbType } from './dbTypes' +import { withMigrationRole } from './datatableMigrationRole' import { assert } from '$lib/utils' import { WorkspaceService } from '$lib/gen' import { pendingMigrations } from './workspaceSettings/datatableMigrationUtils' @@ -70,7 +71,9 @@ export function dbTableOpsWithPreviewScripts({ }): IDbTableOps { const dbType = getDbType(input) const language = getLanguageByResourceType(dbType) - const dbArg = getDatabaseArg(input) + // Built per call: an invalid role throws there, as that operation's error, rather than while + // the manager renders. + const dbArg = () => getDatabaseArg(input) const ducklake = input.type === 'ducklake' ? input.ducklake : undefined function makeMarker(op: string, payload: Record): string { @@ -91,7 +94,7 @@ export function dbTableOpsWithPreviewScripts({ }) const result = await runScriptAndPollResult({ workspace, - requestBody: { args: { ...dbArg, quicksearch }, language, content, tag } + requestBody: { args: { ...dbArg(), quicksearch }, language, content, tag } }) const count = result?.[0].count as number return count @@ -106,7 +109,7 @@ export function dbTableOpsWithPreviewScripts({ }) let items = (await runScriptAndPollResult({ workspace, - requestBody: { args: { ...dbArg, ...params }, language, content, tag } + requestBody: { args: { ...dbArg(), ...params }, language, content, tag } })) as unknown[] if (!items || !Array.isArray(items)) { throw 'items is not an array' @@ -123,7 +126,7 @@ export function dbTableOpsWithPreviewScripts({ { workspace, requestBody: { - args: { ...dbArg, value_to_update: newValue, ...values }, + args: { ...dbArg(), value_to_update: newValue, ...values }, language, content, tag @@ -135,14 +138,14 @@ export function dbTableOpsWithPreviewScripts({ onDelete: async ({ values }) => { const content = makeMarker('DELETE', { table: tableKey, columns: colDefs }) await runScriptAndPollResult( - { workspace, requestBody: { args: { ...dbArg, ...values }, language, content, tag } }, + { workspace, requestBody: { args: { ...dbArg(), ...values }, language, content, tag } }, writingJobOptions ) }, onInsert: async ({ values }) => { const content = makeMarker('INSERT', { table: tableKey, columns: colDefs }) await runScriptAndPollResult( - { workspace, requestBody: { args: { ...dbArg, ...values }, language, content, tag } }, + { workspace, requestBody: { args: { ...dbArg(), ...values }, language, content, tag } }, writingJobOptions ) } @@ -246,6 +249,7 @@ export type IDbSchemaOps = { previewAlterSql: (params: { values: AlterTableValues; schema?: string }) => Promise onCreateSchema: (params: { schema: string }) => Promise onDeleteSchema: (params: { schema: string }) => Promise + onRenameSchema: (params: { schema: string; newSchema: string }) => Promise onFetchTableEditorDefinition: (params: { table: string schema?: string @@ -283,7 +287,8 @@ export function dbSchemaOpsWithPreviewScripts({ tag?: string }): IDbSchemaOps { const dbType = getDbType(input) - const dbArg = getDatabaseArg(input) + // Built per call, for the same reason as in the table ops above. + const dbArg = () => getDatabaseArg(input) const language = getLanguageByResourceType(dbType) const ducklake = input.type === 'ducklake' ? input.ducklake : undefined @@ -293,6 +298,8 @@ export function dbSchemaOpsWithPreviewScripts({ input.type === 'database' && input.resourcePath.startsWith('datatable://') ? input.resourcePath.slice('datatable://'.length) : undefined + // A migration declaring no role runs as admin, whatever role the manager connects as. + const migrationRole = input.type === 'database' ? (input.role ?? input.migrationRole) : undefined function makeMarker(op: string, payload: Record): string { if (ducklake) payload.ducklake = ducklake @@ -359,7 +366,7 @@ export function dbSchemaOpsWithPreviewScripts({ : undefined if (!datatableName || !status?.enabled) { await runScriptAndPollResult( - { workspace, requestBody: { args: dbArg, content, language, tag } }, + { workspace, requestBody: { args: dbArg(), content, language, tag } }, writingJobOptions ) return @@ -373,12 +380,16 @@ export function dbSchemaOpsWithPreviewScripts({ throw new MigrationRunCancelled() } } - const codeUp = wrapMigration(await expandMarker(workspace, language, content)) + // Wrapped before annotating: the annotation must lead, above `BEGIN;`. + const codeUp = withMigrationRole( + wrapMigration(await expandMarker(workspace, language, content)), + migrationRole + ) // Down migrations are only generated for Postgres for now. let codeDown: string | undefined if (downContent && dbType === 'postgresql') { const downSql = (await expandMarker(workspace, language, downContent)).trim() - if (downSql) codeDown = wrapMigration(downSql) + if (downSql) codeDown = withMigrationRole(wrapMigration(downSql), migrationRole) } const created = await WorkspaceService.createDatatableMigration({ workspace, @@ -415,7 +426,7 @@ export function dbSchemaOpsWithPreviewScripts({ const fkContent = makeMarker('FOREIGN_KEYS', { table, schema }) const fkResult = await runScriptAndPollResult({ workspace, - requestBody: { args: dbArg, content: fkContent, language, tag } + requestBody: { args: dbArg(), content: fkContent, language, tag } }) let rawForeignKeys: RawForeignKey[] @@ -501,6 +512,11 @@ export function dbSchemaOpsWithPreviewScripts({ const downContent = makeMarker('CREATE_SCHEMA', { schema }) await applyDdl(migrationName('drop_schema', schema), content, downContent) }, + onRenameSchema: async ({ schema, newSchema }) => { + const content = makeMarker('RENAME_SCHEMA', { schema, new_schema: newSchema }) + const downContent = makeMarker('RENAME_SCHEMA', { schema: newSchema, new_schema: schema }) + await applyDdl(migrationName('rename_schema', schema), content, downContent) + }, onFetchForeignKeys: fetchForeignKeys, onFetchTableEditorDefinition: async ({ table, schema, colDefs }) => { const foreignKeys = await fetchForeignKeys({ table, schema }) @@ -512,7 +528,7 @@ export function dbSchemaOpsWithPreviewScripts({ const pkContent = makeMarker('PRIMARY_KEY_CONSTRAINT', { table, schema }) const pkResult = (await runScriptAndPollResult({ workspace, - requestBody: { args: dbArg, content: pkContent, language, tag } + requestBody: { args: dbArg(), content: pkContent, language, tag } })) as { constraint_name?: string; CONSTRAINT_NAME?: string }[] if (pkResult && Array.isArray(pkResult) && pkResult.length > 0) { @@ -611,7 +627,9 @@ export function getDefaultDbTag(input: DbInput): string { export function getDatabaseArg(input: DbInput | undefined) { if (input?.type === 'database') { if (input.resourcePath.startsWith('datatable://')) { - return { database: input.resourcePath } + return { + database: datatableReference(input.resourcePath.slice('datatable://'.length), input.role) + } } else { return { database: '$res:' + input.resourcePath } } diff --git a/frontend/src/lib/components/dbSchemaCache.ts b/frontend/src/lib/components/dbSchemaCache.ts new file mode 100644 index 0000000000..0421c0c033 --- /dev/null +++ b/frontend/src/lib/components/dbSchemaCache.ts @@ -0,0 +1,21 @@ +import type { DbInput } from './dbTypes' + +/** What identifies a database's schema, role included: two roles on one data table may reach + * different schemas, so they cannot share a cache entry. Never throws, since it keys derived + * state; the connection itself is what refuses an invalid role. */ +export function getDbSchemasPath(input: DbInput): string { + switch (input.type) { + case 'database': + return input.role !== undefined && input.resourcePath.startsWith('datatable://') + ? `${input.resourcePath}?role=${input.role}` + : input.resourcePath + case 'ducklake': + return 'ducklake://' + input.ducklake + } +} + +/** Scoped by the acting workspace: a data table of the same name can exist in both the nav and + * the acting workspace, and one's schema must not be reused for the other. */ +export function schemaCacheKey(workspace: string | undefined, input: DbInput): string { + return `${workspace}:${getDbSchemasPath(input)}` +} diff --git a/frontend/src/lib/components/dbTypes.ts b/frontend/src/lib/components/dbTypes.ts index 6a85617110..e6d3af12e8 100644 --- a/frontend/src/lib/components/dbTypes.ts +++ b/frontend/src/lib/components/dbTypes.ts @@ -3,6 +3,12 @@ export type DbInput = type: 'database' resourceType: DbType resourcePath: string + /** The data table role to connect as; the data table's default when unset. Only + * meaningful for a `datatable://` path. */ + role?: string + /** The role migrations written through this input declare when `role` is unset. A + * migration declaring none runs as admin, not as the role the manager connects as. */ + migrationRole?: string specificSchema?: string specificTable?: string } @@ -23,3 +29,47 @@ export const dbTypes = [ 'duckdb' ] as const export const isDbType = (str?: string): str is DbType => !!str && dbTypes.includes(str as DbType) + +/** The role every data table has: the one it connects as when it is not under roles. */ +export const ADMIN_DATATABLE_ROLE = 'admin' + +/** What the server accepts in `-- role ` and `?role=`. */ +export function isDatatableRoleName(name: string): boolean { + return /^[A-Za-z0-9_-]{1,63}$/.test(name) +} + +/** Whether a role can be named in a reference to this data table. A name stored before names were + * restricted may contain `?`, and the server reads such a whole reference as that name first, so + * `?role=` after it would be taken as part of the name or refused. */ +export function datatableNameTakesRole(name: string): boolean { + return !name.includes('?') +} + +/** The `migrationRole` of a data table that cannot name a role in its reference: it connects as + * its default role, which its migrations must then declare. */ +export function defaultMigrationRole( + name: string, + permissioned: boolean | undefined, + defaultRole: string | undefined +): string | undefined { + return permissioned && !datatableNameTakesRole(name) ? defaultRole : undefined +} + +/** `datatable://`, with `?role=` when a role is named. Throws rather than build a + * reference the executor would refuse, or one that would silently mean another role. */ +export function datatableReference(name: string, role: string | undefined): string { + if (role === undefined) return `datatable://${name}` + if (!isDatatableRoleName(role)) { + throw new Error( + `Invalid data table role '${role}': only letters, digits, '_' and '-' are allowed` + ) + } + if (!datatableNameTakesRole(name)) { + throw new Error( + `Data table '${name}' has a '?' in its name, so it can only be used here as its default role. Rename it to connect as role '${role}'.` + ) + } + return `datatable://${name}?role=${role}` +} + +export type DatatableRowAction = 'migrations' | 'roles' | 'export' | 'import' diff --git a/frontend/src/lib/components/raw_apps/DefaultDatabaseSelector.svelte b/frontend/src/lib/components/raw_apps/DefaultDatabaseSelector.svelte index 57f01dddc1..c4d6d47269 100644 --- a/frontend/src/lib/components/raw_apps/DefaultDatabaseSelector.svelte +++ b/frontend/src/lib/components/raw_apps/DefaultDatabaseSelector.svelte @@ -3,13 +3,14 @@ import Popover from '$lib/components/meltComponents/Popover.svelte' import Select from '$lib/components/select/Select.svelte' import { + createDatatableAccessResource, createDatatablesResource, - createSchemasResource, toDatatableItems, toSchemaItems } from './datatableUtils.svelte' import { Button } from '../common' import { useOperatingWorkspace } from '$lib/components/operatingWorkspace.svelte' + import { appDatatableRole } from './dataTableRefUtils' const operatingWorkspace = useOperatingWorkspace() let opWs = $derived($operatingWorkspace) @@ -19,6 +20,8 @@ datatable: string | undefined /** Currently selected schema */ schema: string | undefined + /** The role the app uses each data table through: schemas are listed as that role. */ + roles?: Record /** Callback when either value changes */ onChange?: (datatable: string | undefined, schema: string | undefined) => void /** Description text to show in the popover */ @@ -28,19 +31,31 @@ let { datatable, schema, + roles, onChange, description = 'Set the default datatable and schema for new tables. This is where AI will create new tables when needed.' }: Props = $props() + const role = $derived(datatable ? appDatatableRole(roles, datatable) : undefined) + // Load available datatables and schemas using shared utilities const datatables = createDatatablesResource(() => opWs) - const schemas = createSchemasResource( + const access = createDatatableAccessResource( () => datatable, + () => role, () => opWs ) const datatableItems = $derived(toDatatableItems(datatables.current)) - const schemaItems = $derived(toSchemaItems(schemas.current)) + // Until the answer is for this workspace, data table and role, the schemas in hand belong to + // another. + const schemaItems = $derived( + access.current.workspace === opWs && + access.current.datatable === datatable && + access.current.role === role + ? toSchemaItems(access.current.schemas) + : [] + ) // Track datatable changes to reset schema let previousDatatable = $state(undefined) @@ -81,6 +96,9 @@ placeholder="Select database" size="sm" /> + {#if role} + Used as role {role} + {/if}
    diff --git a/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte b/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte index 6befd85579..7ccf89a275 100644 --- a/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppDataTableDrawer.svelte @@ -1,34 +1,50 @@ - - {/if} -
    - {#if editable && row.id !== ADMIN_ROLE} - removeRole(row.id)} /> - {/if} -
    - -

    - {/each} - - - {#if editable && unusedRoles.length > 0} -
    - - ({ + value: r.id, + label: r.enabled ? r.name : `${r.name} (disabled)` + }))} + placeholder="+ Add a role" + bind:value={ + () => undefined, + (id) => { + if (id) addRole(id) + } + } + onCreateItem={addRoleByName} + class="w-64" + /> +
    + + + {/if} + + {/if} + + {#if info?.supported && !hasUnsavedChanges} +
    + +
    + {/if} {/if} + + {#snippet actions()} + {#if editable} + + {/if} + {/snippet} + +{#if $superadmin} + +{/if} diff --git a/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte b/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte index d100b1681a..b863dac06b 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte @@ -13,11 +13,22 @@ import { SettingService, type InstanceDatatableRole } from '$lib/gen' import { sendUserToast } from '$lib/toast' + let { + initialName = '', + onChanged + }: { + /** Prefills the name of the role to add. */ + initialName?: string + /** Called after every change to the catalog, whether or not it went through. */ + onChanged?: () => void + } = $props() + let roles = $state([]) let loading = $state(true) let loadError = $state(undefined) let busy = $state(false) - let newName = $state('') + // svelte-ignore state_referenced_locally + let newName = $state(initialName) /** Which role's name is being edited, and to what. */ let renaming = $state<{ id: string; name: string } | undefined>(undefined) @@ -48,6 +59,7 @@ // holds, so a failed flip has to snap back rather than sit there claiming it landed. await load() busy = false + onChanged?.() } } diff --git a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte index 53b4819689..ebc4bd8a7f 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte @@ -64,12 +64,11 @@ - +{#if !hideTrigger} + +{/if} Beta {/snippet} - + {#key openCount} + + {/key} diff --git a/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte b/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte index d16f948b88..04cf841b28 100644 --- a/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte +++ b/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte @@ -8,12 +8,17 @@ import TextInput from '../text_input/TextInput.svelte' import SimpleEditor from '../SimpleEditor.svelte' import { WorkspaceService, type DatatableMigration } from '$lib/gen' + import { listUsableDatatableRoles } from '../datatableUsableRoles' import { sendUserToast } from '$lib/toast' import { tick } from 'svelte' import ConfirmationModal from '../common/confirmationModal/ConfirmationModal.svelte' import { createAsyncConfirmationModal } from '../common/confirmationModal/asyncConfirmationModal.svelte' import Portal from '$lib/components/Portal.svelte' import { fetchPendingMigrations, outOfOrderRunMessage } from './datatableMigrationUtils' + import Select from '../select/Select.svelte' + import { parseMigrationRole, withMigrationRole } from '../datatableMigrationRole' + import { ADMIN_DATATABLE_ROLE } from '../dbTypes' + import { resource } from 'runed' let { workspace, @@ -50,6 +55,8 @@ let tab = $state('up') let name = $state('') let nameInput = $state() + let upEditor = $state() + let downEditor = $state() // A valid migration name is non-empty and limited to letters, digits, '_' and '-'. const MIGRATION_NAME_RE = /^[a-zA-Z0-9_-]+$/ let nameInvalid = $derived(!MIGRATION_NAME_RE.test(name.trim())) @@ -60,6 +67,96 @@ const confirmationModal = createAsyncConfirmationModal() + // The role lives in the SQL as its `-- role ` annotation, so the code is the single + // source of truth and the Select is a view onto it: reading parses, writing rewrites the + // annotation. + const usableRoles = resource( + () => [workspace, datatable] as const, + async ([ws, dt]) => { + try { + return await listUsableDatatableRoles(ws, dt) + } catch (e) { + console.error('Failed to load data table roles:', e) + return null + } + } + ) + // Not a valid role name, so it cannot collide with one. + const NO_ROLE = '(no role)' + let declaredUp = $derived(parseMigrationRole(codeUp)) + let declaredDown = $derived(enableDown ? parseMigrationRole(codeDown) : undefined) + let malformedLine = $derived( + declaredUp.kind === 'malformed' + ? declaredUp.line + : declaredDown?.kind === 'malformed' + ? declaredDown.line + : undefined + ) + const roleOf = (d: typeof declaredUp) => (d.kind === 'role' ? d.role : undefined) + // A rollback runs as the role its own SQL names: under another role than the up migration it + // typically cannot touch what the up created. + let sqlProblem = $derived( + malformedLine !== undefined + ? malformedMessage(malformedLine) + : declaredDown !== undefined && roleOf(declaredDown) !== roleOf(declaredUp) + ? `The down migration runs as ${roleOf(declaredDown) ?? 'admin (no role)'} but the up migration as ${roleOf(declaredUp) ?? 'admin (no role)'}: make their role annotations match` + : undefined + ) + let selectedRole = $derived(declaredUp.kind === 'role' ? declaredUp.role : NO_ROLE) + let permissioned = $derived(!!usableRoles.current?.permissioned) + // No annotation runs as admin, which the server allows exactly to those who may use `admin`. + let adminUsable = $derived(!!usableRoles.current?.roles.includes(ADMIN_DATATABLE_ROLE)) + let roleItems = $derived.by(() => { + const usable = usableRoles.current + if (!usable?.permissioned) return [] + const names = usable.roles.filter((r) => r !== ADMIN_DATATABLE_ROLE) + // A role the SQL names but the caller cannot use is still shown, or the picker would + // misreport what the migration runs as. + if (declaredUp.kind === 'role' && !names.includes(declaredUp.role)) { + names.push(declaredUp.role) + } + const items = names.map((r) => ({ + value: r, + label: r === usable.default_role ? `${r} (default)` : r + })) + if (adminUsable || declaredUp.kind === 'none') { + items.push({ + value: NO_ROLE, + label: 'No role — runs as admin with full access' + }) + } + return items + }) + + function setRole(value: string | undefined) { + const role = value === NO_ROLE ? undefined : value + codeUp = withMigrationRole(codeUp, role) + // Up and down agree: a rollback run as another role could fail on objects it does not own. + if (enableDown) codeDown = withMigrationRole(codeDown, role) + // Assigning the bound value does not repaint the editor, and its next keystroke would write + // the stale text back. + upEditor?.setCode(codeUp) + if (enableDown) downEditor?.setCode(codeDown) + } + + // Set by `open` when the SQL names no role yet: the data table's default is written once its + // roles are known. + let applyDefaultRole = $state(false) + $effect(() => { + // `undefined` until the first answer lands; `null` when it failed. + const usable = usableRoles.current + if (!applyDefaultRole || !isOpen || usableRoles.loading || usable === undefined) return + applyDefaultRole = false + if (!usable?.permissioned || declaredUp.kind !== 'none') return + const role = + usable.default_role !== ADMIN_DATATABLE_ROLE && usable.roles.includes(usable.default_role) + ? usable.default_role + : usable.default_role === ADMIN_DATATABLE_ROLE && adminUsable + ? undefined + : usable.roles.find((r) => r !== ADMIN_DATATABLE_ROLE) + if (role !== undefined) setRole(role) + }) + // Frame the migration body in an explicit transaction so it applies atomically. function wrapInTransaction(body: string): string { return `BEGIN;\n\n${body}\n\nEND;` @@ -72,15 +169,35 @@ } const PLACEHOLDER = wrapInTransaction('-- Add your migration here') + function malformedMessage(line: string): string { + return `Malformed role annotation \`${line}\`: write it as \`-- role \`, or pick the role above` + } + export function open(prefill?: { name?: string; codeUp?: string; codeDown?: string }) { + // Roles and the default can have changed since the last open (the roles drawer sits next + // to this modal), and the default role is written from this answer. + usableRoles.refetch() name = prefill?.name ?? '' // Start from the transaction template; when prefilled from detected DDL, - // wrap that DDL in the same BEGIN; ... END; frame. - codeUp = prefill?.codeUp - ? wrapInTransaction(ensureTrailingSemicolon(prefill.codeUp)) - : PLACEHOLDER + // wrap that DDL in the same BEGIN; ... END; frame. A role the prefill declares is taken + // out first and put back on top: below `BEGIN;` it would not be read. + const prefillRole = prefill?.codeUp ? parseMigrationRole(prefill.codeUp) : undefined + if (prefill?.codeUp) { + const wrapped = wrapInTransaction( + ensureTrailingSemicolon(withMigrationRole(prefill.codeUp, undefined)) + ) + codeUp = + prefillRole?.kind === 'role' + ? withMigrationRole(wrapped, prefillRole.role) + : prefillRole?.kind === 'malformed' + ? `${prefillRole.line}\n${wrapped}` + : wrapped + } else { + codeUp = PLACEHOLDER + } codeDown = prefill?.codeDown ?? PLACEHOLDER enableDown = (prefill?.codeDown ?? '') !== '' + applyDefaultRole = prefillRole === undefined || prefillRole.kind === 'none' tab = 'up' isOpen = true // Focus the name field once the modal content has rendered. @@ -96,6 +213,10 @@ sendUserToast("Invalid migration name: use only letters, digits, '_' and '-'", true) return } + if (sqlProblem !== undefined) { + sendUserToast(sqlProblem, true) + return + } if (run) { // A new migration gets the highest timestamp, so any still-pending // migration is earlier: running only this one applies it out of order. @@ -176,29 +297,65 @@ closeOnOutsideClick={false} >
    - +
    + + {#if permissioned} + { - const value = parseInt(e.currentTarget.value) - if (!isNaN(value)) { - updateTokensForModel(provider as AIProvider, model, value) - } - }} - class="w-20 px-2 py-1 text-xs text-center border border-gray-200 dark:border-gray-700 rounded bg-surface focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-transparent" - /> +
    + { + if (e.currentTarget.value === '') { + resetModel(provider as AIProvider, model) + return + } + const value = Number(e.currentTarget.value) + if (!isNaN(value)) { + updateTokens(provider as AIProvider, model, value) + } + }, + onblur: (e: Event & { currentTarget: HTMLInputElement }) => { + // Resync a value the state refused, so what is shown is what is stored. + e.currentTarget.value = String(limits[key] ?? '') + errors[key] = '' + } + }} + /> +
    tokens
    - {#if !isAtDefault} -
    - Default: {defaultTokens} tokens + {#if overridden} +
    + + {fallback.assumed ? 'Assumed' : 'Default'}: {fallback.tokens.toLocaleString()} + tokens +
    - {#if errors[getModelKey(provider as AIProvider, model)]} -
    {errors[getModelKey(provider as AIProvider, model)]}
    - {/if} + {/if} + {#if errors[key]} +
    {errors[key]}
    {/if}
    {/each} diff --git a/frontend/src/lib/components/workspaceSettings/perModelSettings.ts b/frontend/src/lib/components/workspaceSettings/perModelSettings.ts new file mode 100644 index 0000000000..a83da75540 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/perModelSettings.ts @@ -0,0 +1,24 @@ +import type { AIConfig, AIProvider } from '$lib/gen' +import { stripLegacyThinkingSuffix } from '../copilot/reasoningRegistry' + +/** + * The rows of a per-model setting, grouped by provider. Settings are keyed by the id + * the chat reads them under, and `setCopilotInfo` strips the deprecated `/thinking` + * suffix before the chat ever sees a model. A row built from the raw config would + * save a value under a key nothing reads, so it would sit in settings looking applied + * and never take effect. Stripping can collapse two configured slots onto one model, + * hence the dedupe. + */ +export function chatModelsByProvider( + aiProviders: Exclude +): Record> { + return Object.fromEntries( + Object.entries(aiProviders).map(([provider, config]) => [ + provider, + [...new Set(config.models.map(stripLegacyThinkingSuffix))].map((model) => ({ + provider: provider as AIProvider, + model + })) + ]) + ) +} From d340bec9994e12fcb652f17586ceeaec6a5bf49d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:46:03 +0000 Subject: [PATCH 27/32] chore(deps): bump actions/setup-node from 3 to 7 (#11260) Bumps [actions/setup-node](https://github.com/actions/setup-node) from 3 to 7. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](https://github.com/actions/setup-node/compare/v3...v7) --- updated-dependencies: - dependency-name: actions/setup-node dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/ai-agent-tests.yml | 2 +- .github/workflows/ai-evals-test.yml | 2 +- .github/workflows/backend-test-windows.yml | 2 +- .github/workflows/backend-test.yml | 2 +- .github/workflows/check-docs-links.yml | 2 +- .github/workflows/cli-tests.yml | 6 +++--- .github/workflows/codex-pr-review.yml | 2 +- .github/workflows/frontend-check.yml | 2 +- .github/workflows/git-sync-test.yml | 2 +- .github/workflows/npm_on_release.yml | 6 +++--- .github/workflows/pi-pr-review.yml | 2 +- .github/workflows/sdk-tests.yml | 2 +- .github/workflows/validate-openapi.yml | 2 +- .github/workflows/yaml-validator-tests.yml | 2 +- 14 files changed, 18 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ai-agent-tests.yml b/.github/workflows/ai-agent-tests.yml index cd07602109..2527339699 100644 --- a/.github/workflows/ai-agent-tests.yml +++ b/.github/workflows/ai-agent-tests.yml @@ -63,7 +63,7 @@ jobs: with: bun-version: 1.4.0 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: node-version: "20" diff --git a/.github/workflows/ai-evals-test.yml b/.github/workflows/ai-evals-test.yml index 87eefab0f3..573bc77df9 100644 --- a/.github/workflows/ai-evals-test.yml +++ b/.github/workflows/ai-evals-test.yml @@ -77,7 +77,7 @@ jobs: with: bun-version: 1.4.0 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: # Node 24 ships npm 11, which frontend/package-lock.json is authored # with; npm 10 rejects it ("Missing: picomatch@4.0.5 from lock file"). diff --git a/.github/workflows/backend-test-windows.yml b/.github/workflows/backend-test-windows.yml index f9153c0ce0..e0c731e41f 100644 --- a/.github/workflows/backend-test-windows.yml +++ b/.github/workflows/backend-test-windows.yml @@ -75,7 +75,7 @@ jobs: with: bun-version: 1.4.0 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: node-version: "20" diff --git a/.github/workflows/backend-test.yml b/.github/workflows/backend-test.yml index 9f65c2ae47..d5465d035d 100644 --- a/.github/workflows/backend-test.yml +++ b/.github/workflows/backend-test.yml @@ -59,7 +59,7 @@ jobs: - uses: oven-sh/setup-bun@v2 with: bun-version: 1.4.0 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: node-version: "24" - uses: astral-sh/setup-uv@v6.2.1 diff --git a/.github/workflows/check-docs-links.yml b/.github/workflows/check-docs-links.yml index ec17c06771..e32e976f0c 100644 --- a/.github/workflows/check-docs-links.yml +++ b/.github/workflows/check-docs-links.yml @@ -16,7 +16,7 @@ jobs: sparse-checkout: | frontend/src .github/scripts - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: node-version: "22.x" - name: Verify docs links are not 404 diff --git a/.github/workflows/cli-tests.yml b/.github/workflows/cli-tests.yml index e26231eda4..19afa777ae 100644 --- a/.github/workflows/cli-tests.yml +++ b/.github/workflows/cli-tests.yml @@ -43,7 +43,7 @@ jobs: uses: actions/checkout@v4 - name: Setup Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@v7 with: node-version: "20" @@ -89,7 +89,7 @@ jobs: cache-workspaces: backend - name: Setup Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@v7 with: node-version: "20" @@ -143,7 +143,7 @@ jobs: cache-workspaces: backend - name: Setup Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@v7 with: node-version: "20" diff --git a/.github/workflows/codex-pr-review.yml b/.github/workflows/codex-pr-review.yml index 7a3882df7c..354e8a60ca 100644 --- a/.github/workflows/codex-pr-review.yml +++ b/.github/workflows/codex-pr-review.yml @@ -213,7 +213,7 @@ jobs: - name: Set up Node.js if: steps.codex_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' - uses: actions/setup-node@v4 + uses: actions/setup-node@v7 with: node-version: 22 diff --git a/.github/workflows/frontend-check.yml b/.github/workflows/frontend-check.yml index 178ed53dc5..6347614e16 100644 --- a/.github/workflows/frontend-check.yml +++ b/.github/workflows/frontend-check.yml @@ -18,7 +18,7 @@ jobs: runs-on: ubicloud-standard-8 steps: - uses: actions/checkout@v4 - - uses: actions/setup-node@v5 + - uses: actions/setup-node@v7 with: node-version: 24 cache: "npm" diff --git a/.github/workflows/git-sync-test.yml b/.github/workflows/git-sync-test.yml index bd15ec8876..b19f899a20 100644 --- a/.github/workflows/git-sync-test.yml +++ b/.github/workflows/git-sync-test.yml @@ -141,7 +141,7 @@ jobs: with: deno-version: v2.x - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: node-version: "20" diff --git a/.github/workflows/npm_on_release.yml b/.github/workflows/npm_on_release.yml index 7d30265c74..631ab53153 100644 --- a/.github/workflows/npm_on_release.yml +++ b/.github/workflows/npm_on_release.yml @@ -10,7 +10,7 @@ jobs: runs-on: ubicloud-standard-8 steps: - uses: actions/checkout@v4 - - uses: actions/setup-node@v3 + - uses: actions/setup-node@v7 with: node-version: "20.x" registry-url: "https://registry.npmjs.org" @@ -21,7 +21,7 @@ jobs: runs-on: ubicloud-standard-8 steps: - uses: actions/checkout@v4 - - uses: actions/setup-node@v3 + - uses: actions/setup-node@v7 with: node-version: "20.x" registry-url: "https://registry.npmjs.org" @@ -32,7 +32,7 @@ jobs: runs-on: ubicloud-standard-8 steps: - uses: actions/checkout@v4 - - uses: actions/setup-node@v3 + - uses: actions/setup-node@v7 with: node-version: "20.x" registry-url: "https://registry.npmjs.org" diff --git a/.github/workflows/pi-pr-review.yml b/.github/workflows/pi-pr-review.yml index 9b536b6ed5..a89275cfb5 100644 --- a/.github/workflows/pi-pr-review.yml +++ b/.github/workflows/pi-pr-review.yml @@ -206,7 +206,7 @@ jobs: - name: Set up Node.js if: steps.pi_config.outputs.enabled == 'true' && steps.pr.outputs.skip != 'true' - uses: actions/setup-node@v4 + uses: actions/setup-node@v7 with: node-version: 22 diff --git a/.github/workflows/sdk-tests.yml b/.github/workflows/sdk-tests.yml index 6f7d79c6af..c6078a9e7f 100644 --- a/.github/workflows/sdk-tests.yml +++ b/.github/workflows/sdk-tests.yml @@ -43,7 +43,7 @@ jobs: with: bun-version: latest - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: node-version: "20.x" diff --git a/.github/workflows/validate-openapi.yml b/.github/workflows/validate-openapi.yml index 5bc59ebc4c..846621b503 100644 --- a/.github/workflows/validate-openapi.yml +++ b/.github/workflows/validate-openapi.yml @@ -15,7 +15,7 @@ jobs: uses: actions/checkout@v4 - name: Set up Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@v7 with: node-version: '20' diff --git a/.github/workflows/yaml-validator-tests.yml b/.github/workflows/yaml-validator-tests.yml index 04d31451d4..c5524312cb 100644 --- a/.github/workflows/yaml-validator-tests.yml +++ b/.github/workflows/yaml-validator-tests.yml @@ -24,7 +24,7 @@ jobs: steps: - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v7 with: node-version: "20" From 49e0974c0787c10818b7c7923e09af7234630d01 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:46:16 +0000 Subject: [PATCH 28/32] chore(deps): bump astral-sh/setup-uv from 5 to 7 (#11258) Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 5 to 7. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](https://github.com/astral-sh/setup-uv/compare/v5...v7) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/backend-test-windows.yml | 2 +- .github/workflows/backend-test.yml | 2 +- .github/workflows/sdk-tests.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/backend-test-windows.yml b/.github/workflows/backend-test-windows.yml index e0c731e41f..d7c29e5d0c 100644 --- a/.github/workflows/backend-test-windows.yml +++ b/.github/workflows/backend-test-windows.yml @@ -79,7 +79,7 @@ jobs: with: node-version: "20" - - uses: astral-sh/setup-uv@v6.2.1 + - uses: astral-sh/setup-uv@v7 with: version: "0.11.24" diff --git a/.github/workflows/backend-test.yml b/.github/workflows/backend-test.yml index d5465d035d..00659fef5f 100644 --- a/.github/workflows/backend-test.yml +++ b/.github/workflows/backend-test.yml @@ -62,7 +62,7 @@ jobs: - uses: actions/setup-node@v7 with: node-version: "24" - - uses: astral-sh/setup-uv@v6.2.1 + - uses: astral-sh/setup-uv@v7 with: version: "0.11.24" - uses: shivammathur/setup-php@v2 diff --git a/.github/workflows/sdk-tests.yml b/.github/workflows/sdk-tests.yml index c6078a9e7f..260fbb3a02 100644 --- a/.github/workflows/sdk-tests.yml +++ b/.github/workflows/sdk-tests.yml @@ -58,7 +58,7 @@ jobs: uses: actions/checkout@v4 - name: Setup uv - uses: astral-sh/setup-uv@v5 + uses: astral-sh/setup-uv@v7 # The interpreter is named explicitly: on a clean checkout uv picks the # runner's system python and stops with "not compatible with the locked From dbac7cce9921085618961d98fccc682fe01ba876 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:46:50 +0200 Subject: [PATCH 29/32] refactor(ai-evals): drop two unreachable job routes from the fetch stub (#11251) Co-authored-by: Claude Opus 5 (1M context) --- ai_evals/adapters/frontend/mockBackend.ts | 53 --------------- .../adapters/frontend/mockBackendApi.test.ts | 66 ++++--------------- 2 files changed, 12 insertions(+), 107 deletions(-) diff --git a/ai_evals/adapters/frontend/mockBackend.ts b/ai_evals/adapters/frontend/mockBackend.ts index 07ea82a58f..90df0cf870 100644 --- a/ai_evals/adapters/frontend/mockBackend.ts +++ b/ai_evals/adapters/frontend/mockBackend.ts @@ -1178,27 +1178,6 @@ const BENCHMARK_WORKERS = [ } ] -const BENCHMARK_JOB_GET_PATH = /^\/api\/w\/([^/]+)\/jobs_u\/get\/([^/]+)$/ -const BENCHMARK_RUN_BY_PATH = /^\/api\/w\/([^/]+)\/jobs\/run\/(p|f)\/([^/]+)$/ - -/** An intercepted request carries its args as a JSON string; anything else means none - * were supplied. */ -function parseBenchmarkRequestBody( - body: BodyInit | null | undefined -): Record | undefined { - if (typeof body !== 'string') { - return undefined - } - try { - const parsed = JSON.parse(body) - return typeof parsed === 'object' && parsed !== null - ? (parsed as Record) - : undefined - } catch { - return undefined - } -} - /** True when `handleBenchmarkApiFetch` has an answer for this `/api/...` url. * Any other relative fetch must keep its normal (non-benchmark) behavior — * intercepting it with a synthetic 404 sends the model into retry loops. */ @@ -1210,8 +1189,6 @@ export function hasBenchmarkApiHandler(url: string): boolean { const path = url.split('?')[0] return ( path === '/api/workers/list' || - BENCHMARK_JOB_GET_PATH.test(path) || - BENCHMARK_RUN_BY_PATH.test(path) || path === '/api/embeddings/query_hub_scripts' || path.startsWith('/api/scripts/hub/get_full/') || BENCHMARK_AI_MODELS_PATH.test(path) @@ -1235,36 +1212,6 @@ export function handleBenchmarkApiFetch(url: string, init?: RequestInit): Respon ?.aiProviders?.find((entry) => entry.path === resourcePath) return Response.json({ data: (seed?.models ?? []).map((id) => ({ id })) }) } - const jobGet = BENCHMARK_JOB_GET_PATH.exec(path) - if (jobGet) { - const id = decodeURIComponent(jobGet[2]) - const job = getBenchmarkCompletedJob(decodeURIComponent(jobGet[1]), id) - if (!job) { - return Response.json({ error: `Job not found for "${id}"` }, { status: 404 }) - } - // The real endpoint lets a caller drop the bulky fields. Ignoring that here would - // size the model's context off a payload it explicitly asked to shrink. - const query = new URLSearchParams(url.split('?')[1] ?? '') - if (query.get('no_logs') === 'true') { - delete job.logs - } - if (query.get('no_code') === 'true') { - delete job.raw_code - } - return Response.json(job) - } - const runByPath = BENCHMARK_RUN_BY_PATH.exec(path) - if (runByPath) { - const workspace = decodeURIComponent(runByPath[1]) - const runnablePath = decodeURIComponent(runByPath[3]) - const args = parseBenchmarkRequestBody(init?.body) - // The real endpoint answers with the bare job id as text, not JSON. - return new Response( - runByPath[2] === 'f' - ? runBenchmarkFlowByPath({ workspace, path: runnablePath, args }) - : runBenchmarkScriptByPath({ workspace, path: runnablePath, args }) - ) - } if (path === '/api/embeddings/query_hub_scripts') { const text = new URLSearchParams(url.split('?')[1] ?? '').get('text') ?? '' return Response.json(searchBenchmarkHubScripts(text)) diff --git a/ai_evals/adapters/frontend/mockBackendApi.test.ts b/ai_evals/adapters/frontend/mockBackendApi.test.ts index 0e489a4408..85bd1f3320 100644 --- a/ai_evals/adapters/frontend/mockBackendApi.test.ts +++ b/ai_evals/adapters/frontend/mockBackendApi.test.ts @@ -1,60 +1,18 @@ -import { afterEach, beforeEach, describe, expect, it } from 'bun:test' -import { - createBenchmarkCompletedJob, - getBenchmarkCompletedJob, - handleBenchmarkApiFetch, - hasBenchmarkApiHandler, - resetBenchmarkMockBackend, - registerBenchmarkWorkspaceRunnables -} from './mockBackend' - -const WORKSPACE = 'benchmark-api-ws' +import { describe, expect, it } from 'bun:test' +import { handleBenchmarkApiFetch, hasBenchmarkApiHandler } from './mockBackend' +// `list_workers`'s client call is not faked, so its request really does leave as a relative +// `/api/...` url that node's fetch cannot resolve. Without this route the tool throws instead +// of answering. describe('benchmark API fetch handlers', () => { - beforeEach(() => resetBenchmarkMockBackend()) - afterEach(() => resetBenchmarkMockBackend()) + it('lists workers, the way list_workers reaches them', async () => { + const url = '/api/workers/list?per_page=100' - it('runs a deployed script by path', async () => { - registerBenchmarkWorkspaceRunnables(WORKSPACE, { - scripts: [ - { - path: 'f/evals/greet', - summary: 'Greet', - language: 'bun', - content: 'export async function main() {}' - } - ] - }) + expect(hasBenchmarkApiHandler(url)).toBe(true) + const body = await handleBenchmarkApiFetch(url).json() - const res = handleBenchmarkApiFetch( - `/api/w/${WORKSPACE}/jobs/run/p/${encodeURIComponent('f/evals/greet')}`, - { method: 'POST', body: JSON.stringify({ name: 'ada' }) } - ) - - expect(res.status).toBe(200) - const job = getBenchmarkCompletedJob(WORKSPACE, (await res.text()).trim()) - expect(job).toMatchObject({ success: true, args: { name: 'ada' } }) - }) - - it('serves a recorded job so a model can check the run it just started', async () => { - const id = createBenchmarkCompletedJob({ - workspace: WORKSPACE, - jobKind: 'preview', - result: 'Hello, World!' - }) - - const res = handleBenchmarkApiFetch(`/api/w/${WORKSPACE}/jobs_u/get/${id}`) - - expect(res.status).toBe(200) - expect(await res.json()).toMatchObject({ - id, - success: true, - result: 'Hello, World!' - }) - }) - - it('404s an unknown job id instead of letting the fetch fall through', () => { - expect(hasBenchmarkApiHandler(`/api/w/${WORKSPACE}/jobs_u/get/missing`)).toBe(true) - expect(handleBenchmarkApiFetch(`/api/w/${WORKSPACE}/jobs_u/get/missing`).status).toBe(404) + // The tool counts the list and maps over it, so it needs a bare array, not an envelope. + expect(Array.isArray(body)).toBe(true) + expect(body[0]).toMatchObject({ worker: expect.any(String), worker_group: expect.any(String) }) }) }) From 6dc78049ba2a29dbb34ff8a231d5742b23308177 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:47:09 +0000 Subject: [PATCH 30/32] chore(deps): bump actions/setup-go from 2 to 7 (#11257) Bumps [actions/setup-go](https://github.com/actions/setup-go) from 2 to 7. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](https://github.com/actions/setup-go/compare/v2...v7) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/backend-test-windows.yml | 2 +- .github/workflows/backend-test.yml | 2 +- .github/workflows/go_on_release.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/backend-test-windows.yml b/.github/workflows/backend-test-windows.yml index d7c29e5d0c..54a62ec495 100644 --- a/.github/workflows/backend-test-windows.yml +++ b/.github/workflows/backend-test-windows.yml @@ -67,7 +67,7 @@ jobs: # tests what production runs, instead of floating on the latest v2.x. deno-version: 2.2.1 - - uses: actions/setup-go@v2 + - uses: actions/setup-go@v7 with: go-version: 1.21.5 diff --git a/.github/workflows/backend-test.yml b/.github/workflows/backend-test.yml index 00659fef5f..fea84e7781 100644 --- a/.github/workflows/backend-test.yml +++ b/.github/workflows/backend-test.yml @@ -53,7 +53,7 @@ jobs: # Pin to the Deno version shipped in the runtime image (Dockerfile) so CI # tests what production runs, instead of floating on the latest v2.x. deno-version: 2.2.1 - - uses: actions/setup-go@v2 + - uses: actions/setup-go@v7 with: go-version: 1.21.5 - uses: oven-sh/setup-bun@v2 diff --git a/.github/workflows/go_on_release.yml b/.github/workflows/go_on_release.yml index 78ce12c6c3..6e53db09e7 100644 --- a/.github/workflows/go_on_release.yml +++ b/.github/workflows/go_on_release.yml @@ -13,7 +13,7 @@ jobs: runs-on: ubicloud-standard-8 steps: - uses: actions/checkout@v4 - - uses: actions/setup-go@v4 + - uses: actions/setup-go@v7 - name: install_jq run: | sudo apt-get update From f39742ae324a412704f1da84a502a3d21d109203 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:47:20 +0000 Subject: [PATCH 31/32] chore(deps): bump actions/setup-python from 5 to 7 (#11256) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5 to 7. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](https://github.com/actions/setup-python/compare/v5...v7) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/ai-agent-tests.yml | 2 +- .github/workflows/check-system-prompts.yml | 2 +- .github/workflows/publish-cli-docs.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ai-agent-tests.yml b/.github/workflows/ai-agent-tests.yml index 2527339699..8a3e9e6dbc 100644 --- a/.github/workflows/ai-agent-tests.yml +++ b/.github/workflows/ai-agent-tests.yml @@ -67,7 +67,7 @@ jobs: with: node-version: "20" - - uses: actions/setup-python@v5 + - uses: actions/setup-python@v7 with: python-version: "3.11" diff --git a/.github/workflows/check-system-prompts.yml b/.github/workflows/check-system-prompts.yml index 3e684c82f9..370a177716 100644 --- a/.github/workflows/check-system-prompts.yml +++ b/.github/workflows/check-system-prompts.yml @@ -28,7 +28,7 @@ jobs: steps: - uses: actions/checkout@v4 - - uses: actions/setup-python@v5 + - uses: actions/setup-python@v7 with: python-version: "3.12" diff --git a/.github/workflows/publish-cli-docs.yml b/.github/workflows/publish-cli-docs.yml index 9e76117eb5..d6a2bdb332 100644 --- a/.github/workflows/publish-cli-docs.yml +++ b/.github/workflows/publish-cli-docs.yml @@ -38,7 +38,7 @@ jobs: ssh-key: ${{ secrets.CLI_DOCS_DEPLOY_KEY }} fetch-depth: 0 - - uses: actions/setup-python@v5 + - uses: actions/setup-python@v7 with: python-version: "3.12" From b7425443e1e08be029fc676047f727f84b646323 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 21 Sep 2026 16:47:47 +0200 Subject: [PATCH 32/32] fix: guard data table migration routes against operators and unauthorized authors (#11243) * fix: refuse operators and unauthorized authors on data table migration routes Co-Authored-By: Claude Opus 5 (1M context) * fix: check the replaced definition on migration upsert and skip unchanged writes Co-Authored-By: Claude Opus 5 (1M context) * fix: let data table admins edit migrations whose annotation no longer parses Co-Authored-By: Claude Opus 5 (1M context) * fix: keep migration delete idempotent when a concurrent delete wins Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> --- ...ce891a85daa00e037aba4b65b60f87243bd3.json} | 5 +- ...97462276930f0a83ffb458f7cb26035c8c3d.json} | 8 +- backend/tests/datatable_migration_add_down.rs | 32 +++ .../tests/datatable_roles.rs | 152 ++++++++++++ .../src/datatable_migrations.rs | 231 +++++++++++++----- 5 files changed, 363 insertions(+), 65 deletions(-) rename backend/.sqlx/{query-2b93351b9c2c91272c3f69c9c8eba192d2e4d087c7d7db8d3b257bb864c8265d.json => query-0f374865faa5bdf884345808a004ce891a85daa00e037aba4b65b60f87243bd3.json} (65%) rename backend/.sqlx/{query-3c057e0284f1219865b8f7dc6eb3c1293c4b72d0b52abeefd1e954617984a2d8.json => query-91e2456814ac29534f5a1f65a3bb97462276930f0a83ffb458f7cb26035c8c3d.json} (59%) diff --git a/backend/.sqlx/query-2b93351b9c2c91272c3f69c9c8eba192d2e4d087c7d7db8d3b257bb864c8265d.json b/backend/.sqlx/query-0f374865faa5bdf884345808a004ce891a85daa00e037aba4b65b60f87243bd3.json similarity index 65% rename from backend/.sqlx/query-2b93351b9c2c91272c3f69c9c8eba192d2e4d087c7d7db8d3b257bb864c8265d.json rename to backend/.sqlx/query-0f374865faa5bdf884345808a004ce891a85daa00e037aba4b65b60f87243bd3.json index b82fcf2e17..2ff7820d86 100644 --- a/backend/.sqlx/query-2b93351b9c2c91272c3f69c9c8eba192d2e4d087c7d7db8d3b257bb864c8265d.json +++ b/backend/.sqlx/query-0f374865faa5bdf884345808a004ce891a85daa00e037aba4b65b60f87243bd3.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up, code_down) VALUES ($1, $2, $3, $4, $5, $6) ON CONFLICT (workspace_id, datatable, timestamp) DO UPDATE SET name = EXCLUDED.name, code_up = EXCLUDED.code_up, code_down = EXCLUDED.code_down WHERE NOT $7 OR (datatable_migrations.name = $8::text AND datatable_migrations.code_up = $9::text AND datatable_migrations.code_down IS NOT DISTINCT FROM $10::text)", + "query": "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up, code_down) VALUES ($1, $2, $3, $4, $5, $6) ON CONFLICT (workspace_id, datatable, timestamp) DO UPDATE SET name = EXCLUDED.name, code_up = EXCLUDED.code_up, code_down = EXCLUDED.code_down WHERE datatable_migrations.name = $7::text AND datatable_migrations.code_up = $8::text AND datatable_migrations.code_down IS NOT DISTINCT FROM $9::text", "describe": { "columns": [], "parameters": { @@ -11,7 +11,6 @@ "Varchar", "Text", "Text", - "Bool", "Text", "Text", "Text" @@ -19,5 +18,5 @@ }, "nullable": [] }, - "hash": "2b93351b9c2c91272c3f69c9c8eba192d2e4d087c7d7db8d3b257bb864c8265d" + "hash": "0f374865faa5bdf884345808a004ce891a85daa00e037aba4b65b60f87243bd3" } diff --git a/backend/.sqlx/query-3c057e0284f1219865b8f7dc6eb3c1293c4b72d0b52abeefd1e954617984a2d8.json b/backend/.sqlx/query-91e2456814ac29534f5a1f65a3bb97462276930f0a83ffb458f7cb26035c8c3d.json similarity index 59% rename from backend/.sqlx/query-3c057e0284f1219865b8f7dc6eb3c1293c4b72d0b52abeefd1e954617984a2d8.json rename to backend/.sqlx/query-91e2456814ac29534f5a1f65a3bb97462276930f0a83ffb458f7cb26035c8c3d.json index 3206ba4433..57dbc41a9f 100644 --- a/backend/.sqlx/query-3c057e0284f1219865b8f7dc6eb3c1293c4b72d0b52abeefd1e954617984a2d8.json +++ b/backend/.sqlx/query-91e2456814ac29534f5a1f65a3bb97462276930f0a83ffb458f7cb26035c8c3d.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "DELETE FROM datatable_migrations WHERE workspace_id = $1 AND datatable = $2 AND timestamp = $3 RETURNING name", + "query": "DELETE FROM datatable_migrations WHERE workspace_id = $1 AND datatable = $2 AND timestamp = $3 AND code_up = $4 AND code_down IS NOT DISTINCT FROM $5 RETURNING name", "describe": { "columns": [ { @@ -13,12 +13,14 @@ "Left": [ "Text", "Text", - "Int8" + "Int8", + "Text", + "Text" ] }, "nullable": [ false ] }, - "hash": "3c057e0284f1219865b8f7dc6eb3c1293c4b72d0b52abeefd1e954617984a2d8" + "hash": "91e2456814ac29534f5a1f65a3bb97462276930f0a83ffb458f7cb26035c8c3d" } diff --git a/backend/tests/datatable_migration_add_down.rs b/backend/tests/datatable_migration_add_down.rs index 3cd39e0fa3..47ea4c253b 100644 --- a/backend/tests/datatable_migration_add_down.rs +++ b/backend/tests/datatable_migration_add_down.rs @@ -142,3 +142,35 @@ async fn test_add_down_to_an_applied_migration(db: Pool) -> anyhow::Re Ok(()) } + +/// A definition saved before `-- role` annotations were parsed can open with a comment that now +/// reads as a malformed one. It never gets through a role check again, so an admin must still be +/// able to finish it rather than be refused on its annotation. +#[sqlx::test(fixtures("datatable_migrations_grants"))] +async fn test_add_down_to_a_migration_whose_comment_predates_role_annotations( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + setup_applied_migration_without_down(&db).await?; + let code_up = format!("-- Role table for widgets\n{CODE_UP}"); + sqlx::query("UPDATE datatable_migrations SET code_up = $1 WHERE workspace_id = 'dtmig-ws'") + .bind(&code_up) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let resp = authed(reqwest::Client::new().post(format!( + "http://localhost:{}/api/w/dtmig-ws/workspaces/upsert_datatable_migration/main", + server.addr.port() + ))) + .json(&json!({ + "timestamp": VERSION, + "name": "create_widgets", + "code_up": code_up, + "code_down": CODE_DOWN, + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + Ok(()) +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 3d2853284d..4948a0098a 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -1414,6 +1414,158 @@ async fn a_fork_waits_for_a_rename_of_the_data_table_it_keeps( Ok(()) } +async fn enable_migrations_on_main(db: &Pool) -> anyhow::Result<()> { + sqlx::query( + "UPDATE workspace_settings + SET datatable = jsonb_set(datatable, '{datatables,main,migrations_enabled}', 'true') + WHERE workspace_id = 'test-workspace'", + ) + .execute(db) + .await?; + Ok(()) +} + +#[cfg(all(feature = "private", feature = "enterprise"))] +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_migration_is_written_only_by_who_may_run_it(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + enable_migrations_on_main(&db).await?; + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace/workspaces", + server.addr.port() + ); + let create = |token: &str, body: Value| { + authed( + client() + .post(format!("{base}/create_datatable_migration/main")) + .json(&body), + token, + ) + .send() + }; + let upsert = |token: &str, code_up: &str| { + authed( + client() + .post(format!("{base}/upsert_datatable_migration/main")) + .json(&json!({"timestamp": 20260101000000i64, "name": "m", "code_up": code_up})), + token, + ) + .send() + }; + + // `test-user-2` holds `analytics` and nothing else, so of a migration they may store only + // what they could run: SQL that declares no role runs as admin, in the up or the down. + for body in [ + json!({"name": "m", "code_up": "DROP TABLE t"}), + json!({"name": "m", "code_up": "-- role analytics\nSELECT 1", "code_down": "DROP TABLE t"}), + ] { + let resp = create("SECRET_TOKEN_2", body.clone()).await?; + assert_eq!(resp.status(), 401, "{body}: {}", resp.text().await?); + } + let resp = create( + "SECRET_TOKEN_2", + json!({"name": "m", "code_up": "-- role analytics\nSELECT 1"}), + ) + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + // An admin's migration re-pushed as stored writes nothing, so it passes. Replacing it removes + // SQL they could not run, even with SQL they could, so it is refused like deleting it. + let resp = upsert("SECRET_TOKEN", "DROP TABLE t").await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let resp = upsert("SECRET_TOKEN_2", "DROP TABLE t").await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + for code_up in ["DROP TABLE u", "-- role analytics\nSELECT 1"] { + let resp = upsert("SECRET_TOKEN_2", code_up).await?; + assert_eq!(resp.status(), 401, "{code_up}: {}", resp.text().await?); + } + let resp = authed( + client().delete(format!( + "{base}/delete_datatable_migration/main/20260101000000" + )), + "SECRET_TOKEN_2", + ) + .send() + .await?; + assert_eq!(resp.status(), 401, "{}", resp.text().await?); + + let code_up: String = sqlx::query_scalar( + "SELECT code_up FROM datatable_migrations + WHERE workspace_id = 'test-workspace' AND timestamp = 20260101000000", + ) + .fetch_one(&db) + .await?; + assert_eq!(code_up, "DROP TABLE t"); + Ok(()) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn an_operator_neither_writes_nor_runs_a_migration(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + enable_migrations_on_main(&db).await?; + // Off roles, where every role check passes everyone, so only the operator guard is left to + // refuse. + sqlx::query( + "UPDATE workspace_settings SET datatable = datatable #- '{datatables,main,permissions}' + WHERE workspace_id = 'test-workspace'", + ) + .execute(&db) + .await?; + // A token of its own: the auth cache is process-wide, and the other tests here authenticate + // `SECRET_TOKEN_3` in this workspace as a non-operator. + sqlx::raw_sql( + "UPDATE usr SET operator = true, role = 'Operator' + WHERE workspace_id = 'test-workspace' AND username = 'test-user-3'; + INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin) + VALUES (encode(sha256('DT_OPERATOR_TOKEN'::bytea), 'hex'), 'DT_OPERATO', + 'DT_OPERATOR_TOKEN', 'test3@windmill.dev', 'operator', false);", + ) + .execute(&db) + .await?; + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace/workspaces", + server.addr.port() + ); + let migration = json!({"timestamp": 20260101000000i64, "name": "m", "code_up": "SELECT 1"}); + + for request in [ + client() + .post(format!("{base}/create_datatable_migration/main")) + .json(&migration), + client() + .post(format!("{base}/upsert_datatable_migration/main")) + .json(&migration), + client().delete(format!( + "{base}/delete_datatable_migration/main/20260101000000" + )), + client().post(format!("{base}/run_datatable_migrations/main")), + client().post(format!("{base}/rollback_datatable_migrations/main")), + client().post(format!("{base}/generate_initial_datatable_migration/main")), + ] { + let resp = authed(request, "DT_OPERATOR_TOKEN").send().await?; + let status = resp.status(); + let text = resp.text().await?; + assert!( + status == 401 && text.contains("Operators cannot"), + "{status}: {text}" + ); + } + + // The same write from a developer is accepted: what refused the operator is theirs alone. + let resp = authed( + client() + .post(format!("{base}/upsert_datatable_migration/main")) + .json(&migration), + "SECRET_TOKEN_2", + ) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + Ok(()) +} + #[cfg(not(all(feature = "private", feature = "enterprise")))] const ENTERPRISE_REFUSAL: &str = "Data table roles are a Windmill Enterprise Edition feature"; diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index fa433b2cbc..df1fac203b 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -107,26 +107,63 @@ async fn ensure_migration_role_allowed( datatable_name: &str, authed: &ApiAuthed, sql: &str, - timestamp: i64, - name: &str, + migration: &str, ) -> Result<()> { - let context = format!("Migration {timestamp} ({name})"); let access = DatatableAccess::Authed(authed.to_authed_ref()); match SqlAnnotations::datatable_role(sql)? { Some(role) => { - ensure_can_use_datatable_role(db, w_id, datatable_name, Some(&role), &access, &context) + ensure_can_use_datatable_role(db, w_id, datatable_name, Some(&role), &access, migration) .await } None => ensure_datatable_admin_access(db, w_id, datatable_name, &access) .await .map_err(|e| { Error::NotAuthorized(format!( - "{context} declares no role, so it would run as admin. {e}" + "{migration} declares no role, so it would run as admin. {e}" )) }), } } +/// Refuse writing or deleting a migration definition unless this caller may run both its up and its +/// down. A definition is run later by whoever applies or rolls back pending migrations, as the role +/// it declares, so gating only the run would let anyone plant SQL for a data table's admin to run. +async fn ensure_may_edit_migration( + db: &DB, + w_id: &str, + datatable_name: &str, + authed: &ApiAuthed, + migration: &str, + code_up: &str, + code_down: Option<&str>, +) -> Result<()> { + // Admin access reaches everything a role can, so it edits any definition without reading its + // annotation, which one saved before annotations were parsed may no longer pass. A refusal + // there would leave such a definition impossible to delete or fix. + let access = DatatableAccess::Authed(authed.to_authed_ref()); + if ensure_datatable_admin_access(db, w_id, datatable_name, &access) + .await + .is_ok() + { + return Ok(()); + } + for sql in std::iter::once(code_up).chain(code_down) { + ensure_migration_role_allowed(db, w_id, datatable_name, authed, sql, migration).await?; + } + Ok(()) +} + +/// Operators run deployed code only, and a migration is SQL written or run by hand. Checked before +/// anything else, including the role checks: on a data table not under roles those pass everyone. +fn ensure_not_operator(authed: &ApiAuthed) -> Result<()> { + if authed.is_operator { + return Err(Error::NotAuthorized( + "Operators cannot manage or run data table migrations".to_string(), + )); + } + Ok(()) +} + #[derive(Serialize)] struct AppliedMigration { version: i64, @@ -436,6 +473,7 @@ async fn run_datatable_migrations( Path((w_id, datatable_name)): Path<(String, String)>, Query(query): Query, ) -> JsonResult { + ensure_not_operator(&authed)?; // Before the admin connection is opened at all: the bookkeeping below is created and read // through it, so a caller no role covers must be refused here rather than after the fact. crate::datatable_permissions::ensure_reaches_datatable(&db, &w_id, &datatable_name, &authed) @@ -503,8 +541,7 @@ async fn run_datatable_migrations( &datatable_name, &authed, &m.code_up, - m.timestamp, - &m.name, + &format!("Migration {} ({})", m.timestamp, m.name), ) .await?; run_datatable_migration_job(&db, &user_db, &authed, &w_id, &database_arg, &m.code_up) @@ -573,6 +610,7 @@ async fn rollback_datatable_migrations( Path((w_id, datatable_name)): Path<(String, String)>, Query(query): Query, ) -> JsonResult { + ensure_not_operator(&authed)?; // Before the admin connection is opened at all: the bookkeeping below is created and read // through it, so a caller no role covers must be refused here rather than after the fact. crate::datatable_permissions::ensure_reaches_datatable(&db, &w_id, &datatable_name, &authed) @@ -666,8 +704,7 @@ async fn rollback_datatable_migrations( &datatable_name, &authed, &code_down, - version, - &definition.name, + &format!("Migration {} ({})", version, definition.name), ) .await?; @@ -1213,9 +1250,20 @@ async fn create_datatable_migration( Path((w_id, datatable_name)): Path<(String, String)>, Json(payload): Json, ) -> JsonResult { + ensure_not_operator(&authed)?; validate_datatable_path_segment(&datatable_name)?; validate_migration_name(&payload.name)?; ensure_datatable_migrations_enabled(&db, &w_id, &datatable_name).await?; + ensure_may_edit_migration( + &db, + &w_id, + &datatable_name, + &authed, + &format!("Migration '{}'", payload.name), + &payload.code_up, + payload.code_down.as_deref(), + ) + .await?; let mut tx = db.begin().await?; let timestamp = insert_datatable_migration_def( @@ -1267,6 +1315,34 @@ async fn delete_datatable_migration( Extension(db): Extension, Path((w_id, datatable_name, timestamp)): Path<(String, String, i64)>, ) -> Result { + ensure_not_operator(&authed)?; + let deleted_message = format!("Deleted migration {} from {}", timestamp, datatable_name); + + // A definition already gone is not an error: `wmill sync push` deletes whatever the repo + // dropped, which may no longer exist here. + let Some(definition) = sqlx::query!( + "SELECT name, code_up, code_down FROM datatable_migrations \ + WHERE workspace_id = $1 AND datatable = $2 AND timestamp = $3", + &w_id, + &datatable_name, + timestamp, + ) + .fetch_optional(&db) + .await? + else { + return Ok(deleted_message); + }; + ensure_may_edit_migration( + &db, + &w_id, + &datatable_name, + &authed, + &format!("Migration {} ({})", timestamp, definition.name), + &definition.code_up, + definition.code_down.as_deref(), + ) + .await?; + // Hold the run-serialization lock across the applied-check and the delete: a // run snapshots a migration's SQL before recording its version, so an // unserialized delete could race it and leave `_wm_migrations` pointing at a @@ -1294,19 +1370,46 @@ async fn delete_datatable_migration( ))); } + // Delete only the SQL the role check above cleared: it read the definition without the lock, + // and a rewrite that landed since was never checked. let deleted_name = sqlx::query_scalar!( "DELETE FROM datatable_migrations \ WHERE workspace_id = $1 AND datatable = $2 AND timestamp = $3 \ + AND code_up = $4 AND code_down IS NOT DISTINCT FROM $5 \ RETURNING name", &w_id, &datatable_name, timestamp, + &definition.code_up, + definition.code_down.as_deref(), ) .fetch_optional(&db) .await?; // The definition is removed; runs may resume (audit/deploy metadata below // don't need the lock). drop(lock_client); + let Some(name) = deleted_name else { + // Nothing matched either because another delete got there first, which is this one's + // outcome too, or because the definition was rewritten. + let still_defined = sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM datatable_migrations \ + WHERE workspace_id = $1 AND datatable = $2 AND timestamp = $3)", + &w_id, + &datatable_name, + timestamp, + ) + .fetch_one(&db) + .await? + .unwrap_or(false); + if !still_defined { + return Ok(deleted_message); + } + return Err(Error::BadRequest(format!( + "Migration {} on data table '{}' changed while it was being deleted. Reload it and \ + retry.", + timestamp, datatable_name + ))); + }; audit_log( &db, @@ -1319,23 +1422,10 @@ async fn delete_datatable_migration( ) .await?; - // Only tally a change if a migration was actually deleted. - if let Some(name) = deleted_name { - record_datatable_migration_deployment( - &authed, - &db, - &w_id, - &datatable_name, - timestamp, - &name, - ) + record_datatable_migration_deployment(&authed, &db, &w_id, &datatable_name, timestamp, &name) .await?; - } - Ok(format!( - "Deleted migration {} from {}", - timestamp, datatable_name - )) + Ok(deleted_message) } #[derive(Deserialize)] @@ -1356,6 +1446,7 @@ async fn upsert_datatable_migration( Path((w_id, datatable_name)): Path<(String, String)>, Json(payload): Json, ) -> Result { + ensure_not_operator(&authed)?; validate_datatable_path_segment(&datatable_name)?; validate_migration_name(&payload.name)?; ensure_datatable_migrations_enabled(&db, &w_id, &datatable_name).await?; @@ -1381,9 +1472,9 @@ async fn upsert_datatable_migration( // run-serialization lock across the applied-check and the write below so an // in-flight run can't record a version for the SQL we're about to overwrite. // Held until the end of the handler (well past the write). The exempt - // upserts need no lock: a new or unchanged one overwrites nothing, and one - // that only adds a down leaves the `code_up` a concurrent run is recording - // a version for untouched. + // upserts need no lock: a new one overwrites nothing, and one that only adds + // a down leaves the `code_up` a concurrent run is recording a version for + // untouched. let only_adds_down = existing.as_ref().is_some_and(|existing| { existing.name == payload.name && existing.code_up == payload.code_up @@ -1395,8 +1486,40 @@ async fn upsert_datatable_migration( && existing.code_up == payload.code_up && existing.code_down == payload.code_down }); + let upserted_message = format!( + "Upserted migration {} in {}", + payload.timestamp, datatable_name + ); + // A re-push of the definition as stored changes nothing, so it is not checked, written, audited, + // synced or counted. Writing it anyway would re-create a definition deleted since the read above. + if unchanged { + return Ok(upserted_message); + } + ensure_may_edit_migration( + &db, + &w_id, + &datatable_name, + &authed, + &format!("Migration {} ({})", payload.timestamp, payload.name), + &payload.code_up, + payload.code_down.as_deref(), + ) + .await?; + // Replacing a definition removes the one stored, so it answers to the same check as a delete. + if let Some(existing) = existing.as_ref() { + ensure_may_edit_migration( + &db, + &w_id, + &datatable_name, + &authed, + &format!("Migration {} ({})", payload.timestamp, existing.name), + &existing.code_up, + existing.code_down.as_deref(), + ) + .await?; + } let _run_lock = match existing.as_ref() { - Some(_) if !only_adds_down && !unchanged => { + Some(_) if !only_adds_down => { // Fail closed: if we can't lock/read the applied set (e.g. the // data-table database is temporarily unreachable), refuse the change // rather than risk overwriting a migration that has already run. @@ -1426,31 +1549,27 @@ async fn upsert_datatable_migration( _ => None, }; - // An exempt upsert judged the row from an unlocked read and then writes - // without the lock, so that whole read is re-tested here, where `ON CONFLICT - // DO UPDATE` re-reads the row under a row lock. Otherwise a request working - // from a stale definition silently reverts whatever changed in between — a - // second addition's down, or a locked rewrite of the up whose new SQL a run - // may already have recorded a version for. Reading no row at all is part of - // the premise: the equality is NULL when `$8` is, so a version created in the - // meantime is refused rather than overwritten. - let recheck_observed = _run_lock.is_none(); + // The role checks, and the exemptions from the lock, all judged the row from the unlocked read + // above, so that whole read is re-tested here, where `ON CONFLICT DO UPDATE` re-reads the row + // under a row lock. Otherwise a request working from a stale definition silently replaces + // whatever changed in between: SQL the role checks never saw, or a locked rewrite of the up + // whose new SQL a run may already have recorded a version for. Reading no row at all is part of + // the premise: the equality is NULL when `$7` is, so a version created in the meantime is + // refused rather than overwritten. let written = sqlx::query!( "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up, code_down) \ VALUES ($1, $2, $3, $4, $5, $6) \ ON CONFLICT (workspace_id, datatable, timestamp) DO UPDATE \ SET name = EXCLUDED.name, code_up = EXCLUDED.code_up, code_down = EXCLUDED.code_down \ - WHERE NOT $7 \ - OR (datatable_migrations.name = $8::text \ - AND datatable_migrations.code_up = $9::text \ - AND datatable_migrations.code_down IS NOT DISTINCT FROM $10::text)", + WHERE datatable_migrations.name = $7::text \ + AND datatable_migrations.code_up = $8::text \ + AND datatable_migrations.code_down IS NOT DISTINCT FROM $9::text", &w_id, &datatable_name, payload.timestamp, &payload.name, &payload.code_up, payload.code_down.as_deref(), - recheck_observed, existing.as_ref().map(|e| e.name.as_str()), existing.as_ref().map(|e| e.code_up.as_str()), existing.as_ref().and_then(|e| e.code_down.as_deref()), @@ -1489,24 +1608,17 @@ async fn upsert_datatable_migration( ) .await?; - // An unchanged re-push is not counted: `wmill sync push` sends every migration - // on every sync, so counting those would swamp the definitions people write. - if !unchanged { - windmill_common::feature_usage::log_feature_usage( - "datatable", - "migration_created", - if existing.is_none() { - "synced" - } else { - "edited" - }, - ); - } + windmill_common::feature_usage::log_feature_usage( + "datatable", + "migration_created", + if existing.is_none() { + "synced" + } else { + "edited" + }, + ); - Ok(format!( - "Upserted migration {} in {}", - payload.timestamp, datatable_name - )) + Ok(upserted_message) } /// Generate the first migration for a data table by snapshotting its current @@ -1519,6 +1631,7 @@ async fn generate_initial_datatable_migration( Extension(db): Extension, Path((w_id, datatable_name)): Path<(String, String)>, ) -> JsonResult { + ensure_not_operator(&authed)?; // 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(