mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix(datatables): warn when a settings sync strands fork pointers
A settings save reported the fork pointers left resolving to nothing only for the names in `deleted_datatables`, which `wmill sync push` never sends. The save now works out what it removed from the locked entries, and the CLI prints the stranded pointers it returns. Also correct the replication helper's contract: no role or admin check makes a replication connection safe, so a data table under roles is refused outright rather than gated as an admin operation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb
This commit is contained in:
co-authored by
Claude Opus 5
parent
a01051b624
commit
0c497714b0
@@ -904,3 +904,33 @@ async fn a_stored_name_containing_a_question_mark_resolves_as_itself(
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))]
|
||||
async fn a_settings_save_dropping_a_governing_entry_names_the_forks_it_strands(
|
||||
db: Pool<Postgres>,
|
||||
) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
// The whole map and no `deleted_datatables`, as a settings sync sends it.
|
||||
let resp = authed(
|
||||
client().post(format!(
|
||||
"http://localhost:{}/api/w/test-workspace/workspaces/edit_datatable_config",
|
||||
server.addr.port()
|
||||
)),
|
||||
"SECRET_TOKEN",
|
||||
)
|
||||
.json(&json!({ "settings": { "datatables": {} } }))
|
||||
.send()
|
||||
.await?;
|
||||
let status = resp.status();
|
||||
let body = resp.text().await?;
|
||||
assert_eq!(status, 200, "{body}");
|
||||
let result: Value = serde_json::from_str(&body)?;
|
||||
assert!(
|
||||
result["stranded_references"]
|
||||
.as_array()
|
||||
.is_some_and(|refs| refs.iter().any(|r| r["workspace_id"] == "wm-fork-dt")),
|
||||
"the fork left pointing at nothing was not named: {body}"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -3820,6 +3820,18 @@ async fn edit_datatable_config(
|
||||
}
|
||||
}
|
||||
|
||||
// Worked out from the locked entries rather than taken from `deleted_datatables`: a settings
|
||||
// sync sends the whole map without that list, and dropping a governing entry strands every
|
||||
// fork pointing at it all the same.
|
||||
let removed: Vec<String> = old_datatables
|
||||
.keys()
|
||||
.filter(|name| {
|
||||
!new_config.settings.datatables.contains_key(*name)
|
||||
&& !new_config.renames.iter().any(|r| &r.from == *name)
|
||||
})
|
||||
.cloned()
|
||||
.collect();
|
||||
|
||||
let config: serde_json::Value = serde_json::to_value(new_config.settings)
|
||||
.map_err(|err| Error::internal_err(err.to_string()))?;
|
||||
|
||||
@@ -3859,7 +3871,7 @@ async fn edit_datatable_config(
|
||||
// A deletion cannot be followed the same way — there is nothing to point at any more. Read who
|
||||
// is left stranded so the caller is told, the way deleting a workspace does.
|
||||
let mut stranded: Vec<StrandedReference> = Vec::new();
|
||||
for name in &new_config.deleted_datatables {
|
||||
for name in &removed {
|
||||
let rows = sqlx::query!(
|
||||
r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!"
|
||||
FROM workspace_settings ws
|
||||
|
||||
@@ -1575,8 +1575,9 @@ pub async fn get_datatable_resource_from_db_unchecked(
|
||||
/// datatables resolve to the user's own resource unchanged; configuring it for
|
||||
/// replication there is the user's responsibility.
|
||||
///
|
||||
/// Authorization: a replication connection reads every row whatever the roles grant, so callers
|
||||
/// must gate it with [`ensure_datatable_admin_access`] rather than a role check.
|
||||
/// Authorization: a replication connection reads every row whatever the roles grant, so no role or
|
||||
/// admin check makes it safe. Callers MUST refuse a data table under roles outright — the Postgres
|
||||
/// trigger crate's `ensure_not_under_roles` — and turning roles on is refused while one streams.
|
||||
pub async fn get_datatable_replication_resource_from_db_unchecked(
|
||||
db: &DB,
|
||||
w_id: &str,
|
||||
@@ -1880,8 +1881,9 @@ pub async fn ensure_can_use_datatable_role(
|
||||
)))
|
||||
}
|
||||
|
||||
/// Gate the operations that see the whole database whatever the roles grant: replication streams,
|
||||
/// a migration that declares no role, exports, and editing the permissions themselves. Passing
|
||||
/// Gate the operations that see the whole database whatever the roles grant: a migration that
|
||||
/// declares no role, exports, and editing the permissions themselves. Not replication, which a
|
||||
/// data table under roles refuses whoever asks (see `ensure_not_under_roles`). Passing
|
||||
/// means the caller could have connected as `admin` anyway.
|
||||
pub async fn ensure_datatable_admin_access(
|
||||
db: &DB,
|
||||
|
||||
@@ -386,10 +386,17 @@ export async function pushWorkspaceSettings(
|
||||
|
||||
if (!deepEqual(localSettings.datatable, settings.datatable)) {
|
||||
log.debug(`Updating datatable config...`);
|
||||
await wmill.editDataTableConfig({
|
||||
const { stranded_references } = await wmill.editDataTableConfig({
|
||||
workspace,
|
||||
requestBody: { settings: localSettings.datatable ?? { datatables: {} } },
|
||||
});
|
||||
if (stranded_references?.length) {
|
||||
log.warn(
|
||||
`Removed data tables governed data tables in other workspaces, which no longer resolve: ${stranded_references
|
||||
.map((r) => `${r.workspace_id}/${r.datatable}`)
|
||||
.join(", ")}. A superadmin can point them somewhere else.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if (localSettings.slack_command_script != settings.slack_command_script) {
|
||||
|
||||
Reference in New Issue
Block a user