starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-10 08:07:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(datatables): permissions are enabled where the workspace alone reaches the database

Browse Source
This commit is contained in:
Diego Imbert
2026-09-05 21:26:17 +02:00
parent d4c0c83e0b
commit ddae0ec609
5 changed files with 210 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/.sqlx/query-a1b9097dc53acafe21256f093633b89f944ddd5ac9d0bafe4bcbbbd934518535.json
Generated
+29
View File
@@ -0,0 +1,29 @@
{
"db_name": "PostgreSQL",
"query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id AND NOT w.deleted,\n jsonb_each(ws.datatable->'datatables') dt\n WHERE ws.workspace_id <> $1\n AND dt.value->'database'->>'resource_type' = 'instance'\n AND dt.value->'database'->>'resource_path' = $2",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "workspace_id!",
"type_info": "Varchar"
},
{
"ordinal": 1,
"name": "name!",
"type_info": "Text"
}
],
"parameters": {
"Left": [
"Text",
"Text"
]
},
"nullable": [
false,
null
]
},
"hash": "a1b9097dc53acafe21256f093633b89f944ddd5ac9d0bafe4bcbbbd934518535"
}
backend/windmill-api-integration-tests/tests/datatable_tenants.rs
+96
View File
@@ -106,3 +106,99 @@ async fn freeing_a_principal_takes_its_datatable_tenant(db: Pool<Postgres>) -> a
Ok(())
}
/// A fork made while the data table was unpermissioned carries a copy of it that
/// points at the same database; opting in would leave every member of the fork
/// reaching that database through the copy's own connection. Pinned on the save
/// and on the preview, since a plan the save refuses to run must not be offered.
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
async fn enabling_permissions_is_refused_while_a_fork_exists(
db: Pool<Postgres>,
) -> anyhow::Result<()> {
initialize_tracing().await;
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let ws = format!("http://localhost:{port}/api/w/test-workspace");
sqlx::query(
r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#,
)
.bind(json!({
"datatables": {
"main": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }
}
}))
.execute(&db)
.await?;
sqlx::query(
"INSERT INTO workspace (id, name, owner, parent_workspace_id)
VALUES ('wm-fork-t', 'wm-fork-t', 'test-user', 'test-workspace')",
)
.execute(&db)
.await?;
let body = json!({ "enabled": true, "roles": [] });
for endpoint in [
"workspaces/datatable_permissions/main/preview",
"workspaces/datatable_permissions/main",
] {
let resp = authed(client().post(format!("{ws}/{endpoint}")), "SECRET_TOKEN")
.json(&body)
.send()
.await?;
let status = resp.status();
let text = resp.text().await?;
assert_eq!(status, 400, "{endpoint}: {text}");
assert!(text.contains("wm-fork-t"), "{endpoint}: {text}");
}
// A workspace that is no longer a fork — a detached dev workspace — keeps its
// copy of the data table, pointing at the same instance database.
sqlx::query("DELETE FROM workspace WHERE id = 'wm-fork-t'")
.execute(&db)
.await?;
sqlx::query(
"INSERT INTO workspace (id, name, owner) VALUES ('detached', 'detached', 'test-user')",
)
.execute(&db)
.await?;
sqlx::query("INSERT INTO workspace_settings (workspace_id, datatable) VALUES ('detached', $1)")
.bind(json!({
"datatables": {
"copy": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }
}
}))
.execute(&db)
.await?;
let resp = authed(
client().post(format!(
"{ws}/workspaces/datatable_permissions/main/preview"
)),
"SECRET_TOKEN",
)
.json(&body)
.send()
.await?;
assert_eq!(resp.status(), 400);
let text = resp.text().await?;
assert!(text.contains("detached (data table 'copy')"), "{text}");
// With both gone the refusal lifts: the preview then gets as far as the
// database, which this test does not have.
sqlx::query("DELETE FROM workspace_settings WHERE workspace_id = 'detached'")
.execute(&db)
.await?;
let resp = authed(
client().post(format!(
"{ws}/workspaces/datatable_permissions/main/preview"
)),
"SECRET_TOKEN",
)
.json(&body)
.send()
.await?;
let text = resp.text().await?;
assert!(!text.contains("cannot be enabled"), "{text}");
Ok(())
}
backend/windmill-api-workspaces/src/datatable_permissions.rs
+76 -12
View File
@@ -723,18 +723,42 @@ pub(crate) async fn ensure_can_use_datatable_role(
Ok(())
}
/// A fork may not turn a data table's permissions on.
/// Permissions are turned on where this workspace is the only one reaching the
/// database: no fork above it, none below it, and no other workspace's data
/// table naming the same instance database.
///
/// Its data table is either a copy pointing at the database of the workspace it
/// was forked from, where roles created here would hold grants that workspace's
/// own config does not name, or a clone whose whole database the fork can drop,
/// taking the roles with it. Neither is a place to build an access model; it is
/// built in the workspace that owns the data table.
/// A fork's data table is either a copy pointing at the database of the workspace
/// it was forked from, where roles created in the fork would hold grants that
/// workspace's own config does not name, or a clone whose whole database the fork
/// can drop, taking the roles with it. In the other direction, a fork made while
/// the data table was unpermissioned carries a verbatim copy of it, and every
/// member of that fork — including members this workspace does not have — would
/// keep reaching the database through the copy's own connection, which owns
/// everything in it. Forks made after the opt-in never receive a permissioned
/// data table (see the strip in the fork creation), so refusing while any exist
/// is what closes the gap. A dev workspace detached from this one keeps such a
/// copy without being a fork any more, which is what the last check is for; it
/// is exact for an instance database, whose only credential holders are the
/// data tables naming it, and meaningless for a resource-backed one, where
/// whoever holds the resource's credentials reaches the database regardless.
///
/// Turning them off is always allowed, or a fork carrying permissions from before
/// this rule could never be rid of them, and the roles behind them never dropped.
async fn refuse_enabling_permissions_from_a_fork(db: &DB, w_id: &str, enabled: bool) -> Result<()> {
if enabled && crate::workspaces_extra::workspace_is_fork(db, w_id).await? {
/// Turning them off is always allowed, or a workspace carrying permissions from
/// before this rule could never be rid of them, and the roles behind them never
/// dropped.
///
/// The save calls this under the settings row lock, which fork creation takes on
/// the parent before copying its settings: a fork mid-creation has either
/// committed, and is listed here, or copies the config after the opt-in landed.
async fn refuse_enabling_permissions_over_shared_access(
db: &DB,
w_id: &str,
datatable_name: &str,
enabled: bool,
) -> Result<()> {
if !enabled {
return Ok(());
}
if crate::workspaces_extra::workspace_is_fork(db, w_id).await? {
return Err(Error::BadRequest(
"Data table permissions cannot be enabled from a fork workspace: a fork's data \
table points either at the database of the workspace it was forked from, where \
@@ -744,6 +768,44 @@ async fn refuse_enabling_permissions_from_a_fork(db: &DB, w_id: &str, enabled: b
.to_string(),
));
}
let forks = windmill_common::workspaces::list_fork_descendants(db, w_id).await?;
if !forks.is_empty() {
return Err(Error::BadRequest(format!(
"Data table permissions cannot be enabled while this workspace has forks ({}): a \
fork holds a copy of the data table pointing at the same database, and its members \
would keep reaching it through the data table's own connection, as every role at \
once. Delete the forks first.",
forks.join(", ")
)));
}
let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?;
if datatable.database.resource_type == DataTableCatalogResourceType::Instance {
let others = sqlx::query!(
r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!"
FROM workspace_settings ws
JOIN workspace w ON w.id = ws.workspace_id AND NOT w.deleted,
jsonb_each(ws.datatable->'datatables') dt
WHERE ws.workspace_id <> $1
AND dt.value->'database'->>'resource_type' = 'instance'
AND dt.value->'database'->>'resource_path' = $2"#,
w_id,
&datatable.database.resource_path,
)
.fetch_all(db)
.await?;
if !others.is_empty() {
let others: Vec<String> = others
.into_iter()
.map(|o| format!("{} (data table '{}')", o.workspace_id, o.name))
.collect();
return Err(Error::BadRequest(format!(
"Data table permissions cannot be enabled while another workspace reaches the \
same database: {}. Its members would keep reaching it through that data \
table's own connection, as every role at once. Remove that data table first.",
others.join(", ")
)));
}
}
Ok(())
}
@@ -784,7 +846,8 @@ async fn preview_datatable_permissions(
require_admin(authed.is_admin, &authed.username)?;
// Refused here too: the preview connects to the database and reads its roles,
// and offering a plan that the save will not run is its own kind of wrong.
refuse_enabling_permissions_from_a_fork(&db, &w_id, req.enabled).await?;
refuse_enabling_permissions_over_shared_access(&db, &w_id, &datatable_name, req.enabled)
.await?;
let (_client, plan) = build_plan(&db, &w_id, &datatable_name, &req).await?;
Ok(Json(DatatablePermissionsPreview {
statements: plan.statements.into_iter().map(|s| s.display).collect(),
@@ -799,7 +862,6 @@ async fn set_datatable_permissions(
Json(req): Json<SetDatatablePermissions>,
) -> Result<String> {
require_admin(authed.is_admin, &authed.username)?;
refuse_enabling_permissions_from_a_fork(&db, &w_id, req.enabled).await?;
// Reading the config, planning against it, running the plan and persisting
// it are one operation: interleaved with another save, or with the removal
@@ -808,6 +870,8 @@ async fn set_datatable_permissions(
// touching that config takes, so taking it here is what serializes them.
let mut tx = db.begin().await?;
windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &w_id).await?;
refuse_enabling_permissions_over_shared_access(&db, &w_id, &datatable_name, req.enabled)
.await?;
// The roles about to be created are handed privileges by this connection,
// which cannot pass on what it holds without the grant option.
backend/windmill-api-workspaces/src/workspaces.rs
+8
View File
@@ -8243,6 +8243,14 @@ async fn create_workspace_fork(
.await?;
}
// Enabling a data table's permissions is refused while the workspace has forks, and it
// checks for them under this same row: a fork still being created has either committed,
// and is found, or copies the parent's settings only after the opt-in landed, so the
// permissioned data table is stripped from it below. After the pairing lock, which is
// the order the workspace rename takes the two in.
windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &parent_workspace_id)
.await?;
let forked_id = nw.id;
sqlx::query!(
frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte
+1 -1
View File
@@ -275,7 +275,7 @@
<Alert type="info" title="Permissions belong to the workspace this was forked from" size="xs">
A fork's data table points either at that workspace's database, where roles created
here would be invisible to its own configuration, or at a copy this fork can drop.
Enable permissions there instead — a fork shares them, with the same restrictions.
Enable permissions there instead, once this fork is deleted.
</Alert>
{/if}