From a431bf27e4f6f3f3774b0f53c9eb2aeeba716123 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Mon, 7 Sep 2026 11:06:42 +0200 Subject: [PATCH] fix(datatables): the ACL editor and the import answer to the owning workspace, not the calling one --- ...f2cd218e8ea441193fb55b6ccf916cf92477.json} | 4 +- ...521837f4db8f653e1e353edb2d6c9ddb7a781.json | 22 +++++ .../tests/datatable_tenants.rs | 26 ++++-- .../src/datatable_acl.rs | 31 +++++-- .../src/datatable_permissions.rs | 83 +++++++++++++++---- backend/windmill-api/openapi.yaml | 9 +- backend/windmill-api/src/workspaces_export.rs | 34 ++++++-- backend/windmill-common/src/workspaces.rs | 13 ++- 8 files changed, 180 insertions(+), 42 deletions(-) rename backend/.sqlx/{query-11f66d88f9374cab8dd59132ffb63f3fef248f52d7518389ac52eb3f515d68ac.json => query-5e63f9b708d669234c600ffcef19f2cd218e8ea441193fb55b6ccf916cf92477.json} (80%) create mode 100644 backend/.sqlx/query-e5810ac68c61dd1219ac247e845521837f4db8f653e1e353edb2d6c9ddb7a781.json diff --git a/backend/.sqlx/query-11f66d88f9374cab8dd59132ffb63f3fef248f52d7518389ac52eb3f515d68ac.json b/backend/.sqlx/query-5e63f9b708d669234c600ffcef19f2cd218e8ea441193fb55b6ccf916cf92477.json similarity index 80% rename from backend/.sqlx/query-11f66d88f9374cab8dd59132ffb63f3fef248f52d7518389ac52eb3f515d68ac.json rename to backend/.sqlx/query-5e63f9b708d669234c600ffcef19f2cd218e8ea441193fb55b6ccf916cf92477.json index fa232ca3b4..9602571650 100644 --- a/backend/.sqlx/query-11f66d88f9374cab8dd59132ffb63f3fef248f52d7518389ac52eb3f515d68ac.json +++ b/backend/.sqlx/query-5e63f9b708d669234c600ffcef19f2cd218e8ea441193fb55b6ccf916cf92477.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT workspace_id AS \"workspace_id!\", datatable AS \"datatable!\", name AS \"name!\",\n code_up AS \"code_up!\", code_down\n FROM datatable_migrations\n WHERE code_up LIKE '%-- role %' OR code_down LIKE '%-- role %'\n ORDER BY workspace_id, datatable, timestamp", + "query": "SELECT workspace_id AS \"workspace_id!\", datatable AS \"datatable!\", name AS \"name!\",\n code_up AS \"code_up!\", code_down\n FROM datatable_migrations\n WHERE code_up LIKE '%--%role%' OR code_down LIKE '%--%role%'\n ORDER BY workspace_id, datatable, timestamp", "describe": { "columns": [ { @@ -40,5 +40,5 @@ true ] }, - "hash": "11f66d88f9374cab8dd59132ffb63f3fef248f52d7518389ac52eb3f515d68ac" + "hash": "5e63f9b708d669234c600ffcef19f2cd218e8ea441193fb55b6ccf916cf92477" } diff --git a/backend/.sqlx/query-e5810ac68c61dd1219ac247e845521837f4db8f653e1e353edb2d6c9ddb7a781.json b/backend/.sqlx/query-e5810ac68c61dd1219ac247e845521837f4db8f653e1e353edb2d6c9ddb7a781.json new file mode 100644 index 0000000000..000bfdab35 --- /dev/null +++ b/backend/.sqlx/query-e5810ac68c61dd1219ac247e845521837f4db8f653e1e353edb2d6c9ddb7a781.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT jsonb_object_keys(datatable->'datatables') FROM workspace_settings WHERE workspace_id = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "jsonb_object_keys", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "e5810ac68c61dd1219ac247e845521837f4db8f653e1e353edb2d6c9ddb7a781" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs index b2aa1d5482..f04027adfe 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs @@ -471,9 +471,10 @@ async fn a_save_names_only_what_exists(db: Pool) -> anyhow::Result<()> plant_main(&db, "test-workspace").await; plant_permissions(&db, MAIN_KEY, "test-workspace", &["u/test-user"]).await; + // Spelled the way the parser accepts and a `-- role ` search would miss. sqlx::query( "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up, code_down) - VALUES ('test-workspace', 'main', 1, 'add_orders', '-- role analyst\nCREATE TABLE orders ()', NULL)", + VALUES ('test-workspace', 'main', 1, 'add_orders', '--role analyst\nCREATE TABLE orders ()', NULL)", ) .execute(&db) .await?; @@ -688,7 +689,16 @@ async fn imported_permissions_govern_without_logins(db: Pool) -> anyho let port = server.addr.port(); let ws = format!("http://localhost:{port}/api/w/test-workspace"); - plant_main(&db, "test-workspace").await; + sqlx::query( + "INSERT INTO workspace_settings (workspace_id, datatable) VALUES ('test-workspace', $1) + ON CONFLICT (workspace_id) DO UPDATE SET datatable = EXCLUDED.datatable", + ) + .bind(json!({ "datatables": { + "main": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }, + "other": { "database": { "resource_type": "instance", "resource_path": "dt_other" } } + }})) + .execute(&db) + .await?; plant_permissions(&db, "instance:dt_other", "test-workspace", &["*"]).await; let import = |rows: serde_json::Value| { let ws = ws.clone(); @@ -707,15 +717,21 @@ async fn imported_permissions_govern_without_logins(db: Pool) -> anyho } }; let exported = json!([ - { "database_key": MAIN_KEY, "permissions": { "enabled": true, "roles": { + { "datatable": "main", "permissions": { "enabled": true, "roles": { "admin": { "tenants": [] }, "analyst": { "tenants": ["u/test-user-3"], "pg_rolename": "wm_analyst_x", "pg_password": "leaked?" } }}}, - { "database_key": "instance:dt_other", "permissions": { "enabled": true, "roles": { "admin": { "tenants": [] } } } } + { "datatable": "other", "permissions": { "enabled": true, "roles": { "admin": { "tenants": [] } } } } ]); let (status, text) = import(exported).await; assert_eq!(status, 200, "{text}"); - assert_eq!(text, "[\"instance:dt_other\"]"); + assert_eq!(text, "[\"other\"]"); + // A database is named through a data table of this workspace, never by key. + let (status, text) = import(json!([ + { "datatable": "nope", "permissions": { "enabled": true, "roles": { "admin": { "tenants": [] } } } } + ])) + .await; + assert_eq!(status, 404, "{text}"); let row: (Option, serde_json::Value) = sqlx::query_as( "SELECT owner_workspace_id, permissions FROM datatable_database_permissions WHERE database_key = $1", diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 075d82e1ed..9191f33845 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -245,6 +245,11 @@ struct CallerConnection { dbname: String, /// The Postgres role this connection authenticated as. current_user: String, + /// Whether the caller administers the database's permissions: an admin of + /// the workspace that owns them — the calling one, while the database is + /// unpermissioned — or a superadmin. Being admin of a workspace that merely + /// reaches the database, a fork's say, counts for nothing. + administers: bool, } async fn connect_as_caller( @@ -258,7 +263,13 @@ async fn connect_as_caller( // lists are what say who reaches which. Without permissions every member // resolves to the data table's own connection, which owns everything — so // there it is the workspace admins' to change, as the roles themselves are. - if !authed.is_admin && database_record(db, w_id, datatable_name).await?.is_none() { + let record = database_record(db, w_id, datatable_name).await?; + let administers = match &record { + None => authed.is_admin, + Some(r) if r.owner_workspace_id.as_deref() == Some(w_id) => authed.is_admin, + Some(_) => windmill_common::auth::is_super_admin_email(db, &authed.email).await?, + }; + if record.is_none() && !administers { return Err(Error::NotAuthorized(format!( "Only an admin can manage access on data table '{datatable_name}', which has no roles" ))); @@ -290,7 +301,10 @@ async fn connect_as_caller( )) })? .get(0); - Ok((client, CallerConnection { dbname, current_user })) + Ok(( + client, + CallerConnection { dbname, current_user, administers }, + )) } /// The classes of object a change reaches beyond the target itself: `relkind`s, @@ -742,10 +756,11 @@ async fn get_datatable_acl( let owner: String = owner_row.get(0); // Membership in the owning role is what Postgres asks for before an ALTER // ... OWNER or a GRANT on something you do not own; `admin` holds every role - // this feature creates, so it passes everywhere. A workspace admin manages - // the data table itself and is never shut out of it — a schema Windmill did - // not create, `public` above all, is owned by neither. - let can_manage: bool = authed.is_admin || owner_row.get::<_, bool>(1); + // this feature creates, so it passes everywhere. An admin of the workspace + // that owns the permissions manages the database itself and is never shut + // out of it — a schema Windmill did not create, `public` above all, is owned + // by neither. + let can_manage: bool = conn.administers || owner_row.get::<_, bool>(1); let mut grants = read_grants(&client, &target, &roles).await?; grants.sort_by(|a, b| { @@ -961,7 +976,7 @@ async fn build_acl_plan( // want on its own — handing an object to a role you are not a member of is // refused outright, and granting on one you own needs the grant option — so // this is the check, and the statements run as the data table's admin below. - if !authed.is_admin { + if !conn.administers { if !can_manage_target(&client, &req.target).await? { return Err(Error::NotAuthorized(format!( "{} is owned by a role you are not a member of", @@ -1012,7 +1027,7 @@ async fn build_acl_plan( .iter() .filter(|(name, pg)| { pg.as_str() != pg_role.as_str() - && (authed.is_admin || usable.iter().any(|u| u == name.as_str())) + && (conn.administers || usable.iter().any(|u| u == name.as_str())) }) .map(|(_, pg)| pg.clone()) .collect(); diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 8a4f1d81ee..dd41004597 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -89,11 +89,13 @@ pub struct DatatablePermissionsInfo { pub editable: bool, } -/// A database's permissions as a workspace export carries them: the roles and -/// their tenants, the login names, never the passwords. +/// A database's permissions as a workspace export carries them: named by a data +/// table of the workspace that reaches the database — the key is the server's +/// to derive, never the client's to choose — with the roles and their tenants +/// and the login names, never the passwords. #[derive(Deserialize, Debug)] pub struct ImportedDatabasePermissions { - pub database_key: String, + pub datatable: String, pub permissions: DataTablePermissions, } @@ -687,6 +689,9 @@ pub(crate) async fn run_planned_drop_keeping_record( } /// Forget a database's permissions: for a database that is gone, roles and all. +/// +/// Authorization: performs none. Callers MUST be acting on a database they have +/// just dropped, on behalf of a caller authorized to drop it. pub(crate) async fn forget_database_permissions(db: &DB, database_key: &str) { let forgotten = async { let mut tx = db.begin().await?; @@ -1001,9 +1006,10 @@ async fn ensure_save_names_what_exists( /// /// Roles are the database's, so a migration of another entry reaching it — in /// this workspace or a fork's copy — is stranded by a removal exactly as this -/// entry's own would be. Only migrations carrying an annotation are read, and -/// only the entries those name are resolved; an entry that does not resolve -/// reaches nothing. +/// entry's own would be. Only migrations that could carry an annotation are +/// read — the filter is looser than the parser, never tighter, so a spelling the +/// executor honours is never missed — and only the entries those name are +/// resolved; an entry that does not resolve reaches nothing. async fn migrations_naming( db: &DB, database_key: &str, @@ -1016,7 +1022,7 @@ async fn migrations_naming( r#"SELECT workspace_id AS "workspace_id!", datatable AS "datatable!", name AS "name!", code_up AS "code_up!", code_down FROM datatable_migrations - WHERE code_up LIKE '%-- role %' OR code_down LIKE '%-- role %' + WHERE code_up LIKE '%--%role%' OR code_down LIKE '%--%role%' ORDER BY workspace_id, datatable, timestamp"#, ) .fetch_all(db) @@ -1237,10 +1243,11 @@ async fn set_datatable_permissions( )) } -/// Restore exported permissions on databases nobody governs yet, owned by this -/// workspace. The export carries no passwords, so every role is refused until an -/// admin saves the drawer again, which recreates the logins; a database that is -/// already governed is left as it is and reported. +/// Restore exported permissions on the databases this workspace's data tables +/// reach and nobody governs yet, owned by this workspace. The export carries no +/// passwords, so every role is refused until an admin saves the drawer again, +/// which recreates the logins; a database that is already governed is left as +/// it is and reported, by the data table that reaches it. async fn import_datatable_permissions( authed: ApiAuthed, Extension(db): Extension, @@ -1256,16 +1263,19 @@ async fn import_datatable_permissions( } let mut skipped = Vec::new(); for row in rows { + let (_, _, database_key) = + resolve_datatable_database_unchecked(&db, &w_id, &row.datatable).await?; let mut tx = db.begin().await?; - lock_database_permissions_key(&mut tx, &row.database_key).await?; - if database_permissions_by_key(&mut *tx, &row.database_key) + lock_database_permissions_key(&mut tx, &database_key).await?; + if database_permissions_by_key(&mut *tx, &database_key) .await? .is_some() { - skipped.push(row.database_key); + skipped.push(row.datatable); continue; } let mut permissions = row.permissions; + validate_imported_permissions(&permissions)?; for role in permissions.roles.values_mut() { role.pg_password = None; } @@ -1275,7 +1285,7 @@ async fn import_datatable_permissions( .insert(ADMIN_DATATABLE_ROLE.to_string(), Default::default()); } lock_datatable_permissions_unchecked(&mut tx, &w_id).await?; - upsert_database_permissions(&mut tx, &row.database_key, &w_id, &permissions).await?; + upsert_database_permissions(&mut tx, &database_key, &w_id, &permissions).await?; audit_log( &mut *tx, &authed, @@ -1283,10 +1293,51 @@ async fn import_datatable_permissions( ActionKind::Create, &w_id, Some(&authed.email), - Some([("database", row.database_key.as_str())].into()), + Some( + [ + ("datatable", row.datatable.as_str()), + ("database", database_key.as_str()), + ] + .into(), + ), ) .await?; tx.commit().await?; } Ok(Json(skipped)) } + +/// The shape a save would have refused: role and tenant names as the planner +/// and the tenant matcher read them. +fn validate_imported_permissions(permissions: &DataTablePermissions) -> Result<()> { + for (name, role) in permissions.roles.iter() { + if name.is_empty() + || name.len() > 63 + || !name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return Err(Error::BadRequest(format!("Invalid role name '{name}'"))); + } + for tenant in role.tenants.iter() { + let valid = tenant == windmill_common::workspaces::DATATABLE_TENANT_WILDCARD + || matches!( + tenant.split_once('/'), + Some(("u" | "g" | "f", rest)) if !rest.is_empty() + ); + if !valid { + return Err(Error::BadRequest(format!( + "Invalid tenant '{tenant}' on role '{name}'" + ))); + } + } + } + if let Some(default_role) = permissions.default_role.as_deref() { + if !permissions.roles.contains_key(default_role) { + return Err(Error::BadRequest(format!( + "Default role '{default_role}' is not one of the roles" + ))); + } + } + Ok(()) +} diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index e4956fed62..f02d863d1e 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -5320,7 +5320,7 @@ paths: /w/{workspace}/workspaces/datatable_permissions_import: post: - summary: restore exported datatable permissions on databases nobody governs yet (admins only); passwords are not restored, so every role is refused until saved again + summary: restore exported datatable permissions on the databases the named data tables reach and nobody governs yet (admins only); passwords are not restored, so every role is refused until saved again operationId: importDatatablePermissions tags: - workspace @@ -5334,15 +5334,16 @@ paths: type: array items: type: object - required: [database_key, permissions] + required: [datatable, permissions] properties: - database_key: + datatable: + description: a data table of this workspace reaching the database type: string permissions: type: object responses: "200": - description: the database keys that were already governed and left as they were + description: the data tables whose database was already governed and left as it was content: application/json: schema: diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index cc72c662be..27a201c725 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -1760,21 +1760,43 @@ pub(crate) async fn tarball_workspace( } // The permissions of the databases this workspace governs, as the import - // endpoint takes them back: roles, tenants and login names, never passwords — - // those are direct database logins, and a re-save recreates them. + // endpoint takes them back: named by a data table of this workspace reaching + // the database, with roles, tenants and login names, never passwords — those + // are direct database logins, and a re-save recreates them. A database no + // entry of the workspace reaches any more cannot be named, and is left out. let permissions = windmill_common::workspaces::database_permissions_owned_by(&mut *tx, &w_id).await?; if !permissions.is_empty() { + let mut reaching: std::collections::HashMap = + std::collections::HashMap::new(); + let datatables: Vec = sqlx::query_scalar!( + "SELECT jsonb_object_keys(datatable->'datatables') FROM workspace_settings WHERE workspace_id = $1", + &w_id + ) + .fetch_all(&mut *tx) + .await? + .into_iter() + .flatten() + .collect(); + for name in datatables { + if let Ok((_, _, key)) = + windmill_common::workspaces::resolve_datatable_database_unchecked(&db, &w_id, &name) + .await + { + reaching.entry(key).or_insert(name); + } + } let exported: Vec = permissions .into_iter() - .map(|mut row| { + .filter_map(|mut row| { + let datatable = reaching.get(&row.database_key)?; for role in row.permissions.roles.values_mut() { role.pg_password = None; } - serde_json::json!({ - "database_key": row.database_key, + Some(serde_json::json!({ + "datatable": datatable, "permissions": row.permissions, - }) + })) }) .collect(); let json = serde_json::to_string_pretty(&exported) diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 5048fb8df4..a1cdf9660e 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1408,6 +1408,12 @@ pub async fn lock_database_permissions( /// Write the database's permissions, creating the row for `owner_workspace_id` /// when none exists. An existing row keeps its owner, unless it lost it. +/// +/// Authorization: performs none. Callers MUST have authorized the caller as an +/// admin of the row's owning workspace (or a superadmin), and MUST hold the +/// key ([`lock_database_permissions_key`]) and the owning workspace's lock +/// ([`lock_datatable_permissions_unchecked`]) in `tx`, or the write lands over +/// a save or a principal deletion that is still reading. pub async fn upsert_database_permissions( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, database_key: &str, @@ -1433,7 +1439,12 @@ pub async fn upsert_database_permissions( } /// Forget the database's permissions: what the opt-out does once the roles are -/// dropped. +/// dropped, and a deletion once the database itself is gone. +/// +/// Authorization: performs none. Callers MUST have authorized the caller as an +/// admin of the row's owning workspace (or a superadmin), or be acting on a +/// database that no longer exists, and MUST hold the key lock +/// ([`lock_database_permissions_key`]) in `tx`. pub async fn delete_database_permissions( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, database_key: &str,