mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-05 08:02:18 +00:00
fix(datatables): opting in reaches the forks made before it
A fork made while a data table was unpermissioned carries a verbatim copy of the config, still pointing at this workspace's database. Opting in never reached that copy, so the fork went on resolving through the connection that owns everything there — for every member of the fork, including members this workspace does not have. The fork strip only covers forks made after the opt-in. Enabling permissions now converts those copies into pointers, over the whole descendant tree. Conservative by construction: a copy is taken only when it still names this workspace's database, was not cloned into one of its own, and the fork has not opted in on it itself — the other three are data tables the fork owns. It runs after the save commits, one fork at a time under that fork's own settings lock, so it cannot deadlock against a save there; it is idempotent, and the next save retries whatever failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5arH3G2Sa1Qqm32veJQ1n
This commit is contained in:
co-authored by
Claude Opus 5
parent
0faedba495
commit
c63bfa86c2
+16
@@ -0,0 +1,16 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "UPDATE workspace_settings\n SET datatable = jsonb_set(\n datatable #- ARRAY['datatables', $2],\n '{shared_datatables}',\n COALESCE(CASE WHEN jsonb_typeof(datatable->'shared_datatables') = 'object'\n THEN datatable->'shared_datatables' ELSE '{}'::jsonb END, '{}'::jsonb)\n || jsonb_build_object($2::text, jsonb_build_object('from', $3::text)))\n WHERE workspace_id = $1",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "be9524e1876ccd3dcf5dab5dfc5fd7187d6a678cbdb6dee545b468504285f50d"
|
||||
}
|
||||
+22
@@ -0,0 +1,22 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "\n WITH RECURSIVE descendants AS (\n SELECT id, 0 AS depth FROM workspace WHERE id = $1\n UNION ALL\n SELECT w.id, descendants.depth + 1\n FROM workspace w JOIN descendants ON w.parent_workspace_id = descendants.id\n WHERE descendants.depth < 20\n )\n SELECT id AS \"id!\" FROM descendants WHERE depth > 0\n ",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "id!",
|
||||
"type_info": "Varchar"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "d4605c688b499dafa266075701b7dab50857f2a8b2f4339043c396dda0f53d14"
|
||||
}
|
||||
@@ -737,6 +737,117 @@ pub(crate) async fn ensure_can_use_datatable_role(
|
||||
|
||||
/// List the roles `authed` may run this data table as. An unpermissioned data
|
||||
/// table reports `enabled: false` and no roles, so a picker can hide itself.
|
||||
/// Whether a fork's entry is a copy of this workspace's data table and nothing
|
||||
/// more — the only shape [`point_forks_at_this_datatable`] may take away.
|
||||
///
|
||||
/// A clone points at a database of its own (`forked_from`), a repointed copy at
|
||||
/// someone else's, and a copy the fork has opted in on has roles in a database
|
||||
/// this workspace does not own. None of those are this workspace's to convert.
|
||||
fn is_inherited_copy(entry: &serde_json::Value, database: &serde_json::Value) -> bool {
|
||||
entry.get("forked_from").is_none()
|
||||
&& entry.get("permissions").is_none()
|
||||
&& entry.get("database") == Some(database)
|
||||
}
|
||||
|
||||
/// Turn every fork's inherited copy of a data table into a pointer at this one.
|
||||
///
|
||||
/// A fork made while the data table was unpermissioned carries a verbatim copy
|
||||
/// of the config, pointing at this workspace's database. Opting in never reached
|
||||
/// that copy, so the fork went on resolving the database through its own
|
||||
/// connection — the one that owns everything in it — for every member of the
|
||||
/// fork, including members this workspace does not have. The fork strip covers
|
||||
/// forks made after the opt-in; this covers the ones made before it.
|
||||
///
|
||||
/// Deliberately conservative. A copy is converted only when it still names this
|
||||
/// workspace's database and was not cloned into one of its own (`forked_from`),
|
||||
/// and never when the fork has opted in on it itself: those roles live in a
|
||||
/// database this does not own, and a pointer would orphan them.
|
||||
///
|
||||
/// Runs after the save has committed, one fork at a time under that fork's own
|
||||
/// settings lock, so it can neither deadlock against a save there nor hold this
|
||||
/// workspace's row while it waits. Idempotent, and the next save retries what
|
||||
/// failed — until then a fork that was not converted keeps the access it already
|
||||
/// had.
|
||||
async fn point_forks_at_this_datatable(db: &DB, w_id: &str, datatable_name: &str) {
|
||||
let database = match read_datatable_unchecked(db, w_id, datatable_name).await {
|
||||
Ok(datatable) => match serde_json::to_value(&datatable.database) {
|
||||
Ok(database) => database,
|
||||
Err(e) => {
|
||||
tracing::error!("reading the database of {datatable_name} in {w_id}: {e:#}");
|
||||
return;
|
||||
}
|
||||
},
|
||||
Err(e) => {
|
||||
tracing::error!("reading {datatable_name} of {w_id}: {e:#}");
|
||||
return;
|
||||
}
|
||||
};
|
||||
let forks = sqlx::query_scalar!(
|
||||
r#"
|
||||
WITH RECURSIVE descendants AS (
|
||||
SELECT id, 0 AS depth FROM workspace WHERE id = $1
|
||||
UNION ALL
|
||||
SELECT w.id, descendants.depth + 1
|
||||
FROM workspace w JOIN descendants ON w.parent_workspace_id = descendants.id
|
||||
WHERE descendants.depth < 20
|
||||
)
|
||||
SELECT id AS "id!" FROM descendants WHERE depth > 0
|
||||
"#,
|
||||
w_id,
|
||||
)
|
||||
.fetch_all(db)
|
||||
.await;
|
||||
let forks = match forks {
|
||||
Ok(forks) => forks,
|
||||
Err(e) => {
|
||||
tracing::error!("listing the forks of {w_id}: {e:#}");
|
||||
return;
|
||||
}
|
||||
};
|
||||
for fork in forks {
|
||||
let converted = async {
|
||||
let mut tx = db.begin().await?;
|
||||
let inherited = windmill_common::workspaces::lock_workspace_settings_unchecked(
|
||||
&mut tx, &fork,
|
||||
)
|
||||
.await?
|
||||
.and_then(|s| s.get("datatables")?.get(datatable_name).cloned())
|
||||
.filter(|entry| is_inherited_copy(entry, &database));
|
||||
if inherited.is_none() {
|
||||
return Ok::<_, Error>(false);
|
||||
}
|
||||
sqlx::query!(
|
||||
"UPDATE workspace_settings
|
||||
SET datatable = jsonb_set(
|
||||
datatable #- ARRAY['datatables', $2],
|
||||
'{shared_datatables}',
|
||||
COALESCE(CASE WHEN jsonb_typeof(datatable->'shared_datatables') = 'object'
|
||||
THEN datatable->'shared_datatables' ELSE '{}'::jsonb END, '{}'::jsonb)
|
||||
|| jsonb_build_object($2::text, jsonb_build_object('from', $3::text)))
|
||||
WHERE workspace_id = $1",
|
||||
&fork,
|
||||
datatable_name,
|
||||
w_id,
|
||||
)
|
||||
.execute(&mut *tx)
|
||||
.await?;
|
||||
tx.commit().await?;
|
||||
Ok(true)
|
||||
}
|
||||
.await;
|
||||
match converted {
|
||||
Ok(true) => tracing::info!(
|
||||
"fork {fork} now points at data table {datatable_name} of {w_id} \
|
||||
instead of holding a copy of its config"
|
||||
),
|
||||
Ok(false) => {}
|
||||
Err(e) => tracing::error!(
|
||||
"pointing fork {fork} at data table {datatable_name} of {w_id}: {e:#}"
|
||||
),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn list_usable_datatable_roles(
|
||||
authed: ApiAuthed,
|
||||
Extension(db): Extension<DB>,
|
||||
@@ -886,7 +997,41 @@ async fn set_datatable_permissions(
|
||||
})?;
|
||||
}
|
||||
|
||||
// A fork that predates this opt-in still holds a copy of the config, and a
|
||||
// copy says nothing about roles.
|
||||
if req.enabled {
|
||||
point_forks_at_this_datatable(&db, &w_id, &datatable_name).await;
|
||||
}
|
||||
|
||||
Ok(format!(
|
||||
"Updated permissions of data table {datatable_name}"
|
||||
))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod fork_sweep_tests {
|
||||
use super::is_inherited_copy;
|
||||
use serde_json::json;
|
||||
|
||||
/// Opting in takes a fork's copy of the config away, so it must recognise
|
||||
/// only a copy. Anything else it takes is a data table the fork owns, and
|
||||
/// the fork loses it.
|
||||
#[test]
|
||||
fn only_a_plain_copy_of_this_database_is_taken() {
|
||||
let database = json!({ "resource_type": "postgresql", "resource_path": "u/a/db" });
|
||||
assert!(is_inherited_copy(
|
||||
&json!({ "database": database, "migrations_enabled": true }),
|
||||
&database
|
||||
));
|
||||
for kept in [
|
||||
// cloned into a database of its own
|
||||
json!({ "database": database, "forked_from": { "schema": {} } }),
|
||||
// repointed at another database
|
||||
json!({ "database": { "resource_type": "postgresql", "resource_path": "u/a/other" } }),
|
||||
// the fork opted in itself: those roles are not in this database
|
||||
json!({ "database": database, "permissions": { "enabled": true } }),
|
||||
] {
|
||||
assert!(!is_inherited_copy(&kept, &database), "{kept}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user