diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index e3269c8138..0a00ed8bb6 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -936,30 +936,58 @@ async fn a_settings_save_dropping_a_governing_entry_names_the_forks_it_strands( } #[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] -async fn a_save_replacing_an_entry_under_roles_on_its_database_is_refused( +async fn an_entry_without_roles_cannot_newly_reach_a_database_under_roles( 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", + sqlx::query( + r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, '{datatables,other}', + '{"database": {"resource_type": "instance", "resource_path": "dt_other"}}') + WHERE workspace_id = 'test-workspace'"#, ) - .json(&json!({ "settings": { "datatables": { - "main_renamed": { "database": { "resource_type": "instance", "resource_path": "dt_main" } } - } } })) - .send() + .execute(&db) .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 server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + // Whole-map saves with no `renames`, as a settings sync sends them. + let dt_main = + json!({ "database": { "resource_type": "instance", "resource_path": "dt_main" } }); + let dt_other = + json!({ "database": { "resource_type": "instance", "resource_path": "dt_other" } }); + for (case, w_id, datatables) in [ + ( + "a rename to a new name", + "test-workspace", + json!({ "main_renamed": dt_main, "other": dt_other }), + ), + ( + "an existing name repointed", + "test-workspace", + json!({ "other": dt_main }), + ), + ( + "another workspace's entry", + "wm-fork-dt", + json!({ "direct": dt_main }), + ), + ] { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/{w_id}/workspaces/edit_datatable_config" + )), + "SECRET_TOKEN", + ) + .json(&json!({ "settings": { "datatables": datatables } })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert!( + status == 400 && body.contains("which a data table under roles uses"), + "{case} reached the database under roles without them ({status}): {body}" + ); + } let still_governed: bool = sqlx::query_scalar( "SELECT (datatable->'datatables'->'main') ? 'permissions' FROM workspace_settings diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 5892e71480..38b3797518 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3887,36 +3887,54 @@ 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()) + // A database under roles is reached only through an entry that carries them. Roles follow an + // entry through a declared rename alone, and a settings sync never declares one, so an entry + // without roles that newly points at such a database — a name added, or an existing one + // repointed — would answer everyone there as `admin`. That holds whichever workspace governs it. + let governed_elsewhere: Vec = sqlx::query_scalar( + "SELECT DISTINCT dt.value->'database'->>'resource_path' FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt + WHERE ws.workspace_id <> $1 AND dt.value ? 'permissions' + AND dt.value->'database'->>'resource_type' = 'instance'", + ) + .bind(&w_id) + .fetch_all(&mut *tx) + .await?; + for (name, dt) in new_config.settings.datatables.iter() { + if dt.permissions.is_some() { + continue; + } + let Some(db) = dt + .database + .as_ref() + .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance) 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 { + let lookup = rename_src + .get(name.as_str()) + .copied() + .unwrap_or(name.as_str()); + let repointed = old_datatables + .get(lookup) + .and_then(|old| old.database.as_ref()) + .is_none_or(|old_db| { + old_db.resource_type != db.resource_type || old_db.resource_path != db.resource_path + }); + let governed_here = old_datatables.values().any(|old| { + old.permissions.is_some() + && old.database.as_ref().is_some_and(|d| { + d.resource_type == DataTableCatalogResourceType::Instance + && d.resource_path == db.resource_path + }) + }); + if repointed && (governed_here || governed_elsewhere.contains(&db.resource_path)) { 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." + "Data table '{name}' would point at database '{}', which a data table under roles \ + uses, without carrying those roles: everyone reaching '{name}' would connect there \ + as `admin`. Rename the data table under roles from the data table settings, which \ + carries its roles, or turn its roles off first.", + db.resource_path ))); } }