mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
refactor(datatables): put the role catalog in its own table, not in global_settings
Five findings across three rounds were all the same choice. A set of live Postgres credentials
was living in `global_settings`, which has generic read, list, write, config-export and CLI
round-trip paths that know nothing about what they carry: the passwords reached the instance
config and its YAML editor, a full-row upsert of a neighbouring key erased the catalog,
`GET /settings/global/{key}` and the settings listing returned them raw, and this round the
redaction that fixed the last two turned `wmill instance push` into something that wipes every
password — a fix breaking the assumption the previous fix made. `POST /settings/global/datatable_roles`
could also empty it outside the lock.
The approved plan offered a table or `global_settings`, so this is the other option it already
allowed rather than a new design. `datatable_role` is a table: no generic settings path can read
it, list it, export it, write it or round-trip it, so none of the five needs a guard. The
redaction, the hidden/protected/agent-denylist entries and the JSON document all go with it.
One row per role also removes the read-modify-write the concurrency work was about: two
concurrent creates are two inserts, and the unique index on `name` is what settles a collision.
The advisory lock stays for the one window rows do not cover — `CREATE ROLE` is invisible to
another transaction until commit, so without it both creates pass their `pg_roles` check.
Also from this round: rename mappings are checked against the configuration they claim to
describe, since fork pointers are rewritten from them — a caller could otherwise submit
`main -> missing` against an unchanged config and repoint every fork of `main` at a name nothing
has, and `A -> B` plus `B -> C` moved what pointed at `A` all the way to `C`. And the warning
naming forks a delete stranded reached the response but not the screen: both the data table
settings save and the workspace delete now show it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR
This commit is contained in:
co-authored by
Claude Opus 5
parent
d0ea1ce598
commit
1db77dda87
@@ -3626,6 +3626,43 @@ async fn edit_datatable_config(
|
||||
for r in &new_config.renames {
|
||||
crate::datatable_migrations::validate_datatable_path_segment(&r.from)?;
|
||||
crate::datatable_migrations::validate_new_datatable_name(&r.to)?;
|
||||
// A rename is a claim about what this save is doing, and other workspaces' pointers are
|
||||
// rewritten from it. Unchecked, a caller could submit an unchanged configuration with
|
||||
// `main -> missing` and repoint every fork of `main` at a name nothing has.
|
||||
if !old_datatables.contains_key(&r.from) {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"Cannot rename data table '{}': this workspace has no such data table",
|
||||
r.from
|
||||
)));
|
||||
}
|
||||
if !new_config.settings.datatables.contains_key(&r.to) {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"Cannot rename data table '{}' to '{}': the save does not contain '{}'",
|
||||
r.from, r.to, r.to
|
||||
)));
|
||||
}
|
||||
}
|
||||
// `A -> B` and `B -> C` applied one after another would move what pointed at `A` all the way
|
||||
// to `C`. Each pointer moves once, from what it named before this save.
|
||||
if new_config.renames.len() > 1 {
|
||||
let mut seen = std::collections::HashSet::new();
|
||||
for r in &new_config.renames {
|
||||
if !seen.insert(r.from.as_str()) {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"Data table '{}' is renamed twice in one save",
|
||||
r.from
|
||||
)));
|
||||
}
|
||||
}
|
||||
for r in &new_config.renames {
|
||||
if seen.contains(r.to.as_str()) && r.to != r.from {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"Data table '{}' is both renamed and the target of another rename in one \
|
||||
save; do them one at a time",
|
||||
r.to
|
||||
)));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Map new name -> old name so a renamed data table inherits the previous
|
||||
|
||||
Reference in New Issue
Block a user