diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index d8a44d7a55..e3269c8138 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -934,3 +934,39 @@ async fn a_settings_save_dropping_a_governing_entry_names_the_forks_it_strands( ); Ok(()) } + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_save_replacing_an_entry_under_roles_on_its_database_is_refused( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + // A rename as a settings sync sends it: the whole map, no `renames`. + 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": { + "main_renamed": { "database": { "resource_type": "instance", "resource_path": "dt_main" } } + } } })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 400, + "a save dropped the roles of the database it kept: {body}" + ); + + let still_governed: bool = sqlx::query_scalar( + "SELECT (datatable->'datatables'->'main') ? 'permissions' FROM workspace_settings + WHERE workspace_id = 'test-workspace'", + ) + .fetch_one(&db) + .await?; + assert!(still_governed, "the refused save still took effect"); + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index cfd3c47593..1e591e742e 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3883,6 +3883,40 @@ async fn edit_datatable_config( .cloned() .collect(); + // Roles follow an entry only through a declared rename. A save that drops an entry under roles + // and adds another on the same database without one — which is how a settings sync sends a + // rename — would leave that database answering everyone as `admin`. + for name in &removed { + let Some(old_db) = old_datatables + .get(name) + .filter(|old| old.permissions.is_some()) + .and_then(|old| old.database.as_ref()) + else { + continue; + }; + let added_on_same_database = + new_config + .settings + .datatables + .iter() + .find_map(|(added, dt)| { + let db = dt.database.as_ref()?; + (!old_datatables.contains_key(added) + && !new_config.renames.iter().any(|r| &r.to == added) + && db.resource_type == old_db.resource_type + && db.resource_path == old_db.resource_path) + .then_some(added) + }); + if let Some(added) = added_on_same_database { + return Err(Error::BadRequest(format!( + "Data table '{name}' is under roles, and this save removes it while adding '{added}' \ + on the same database. Its roles would not carry over, leaving that database open to \ + everyone as `admin`. Rename it from the data table settings, which carries its \ + roles, or turn its roles off first." + ))); + } + } + let config: serde_json::Value = serde_json::to_value(new_config.settings) .map_err(|err| Error::internal_err(err.to_string()))?;