fix(datatables): the ACL editor and the import answer to the owning workspace, not the calling one

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"
 }

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"
+}

backend/windmill-api-integration-tests/tests/datatable_tenants.rs

@@ -471,9 +471,10 @@ async fn a_save_names_only_what_exists(db: Pool<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<String>, serde_json::Value) = sqlx::query_as(
         "SELECT owner_workspace_id, permissions FROM datatable_database_permissions WHERE database_key = $1",

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();

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<DB>,
@@ -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(())
+}

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:

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<String, String> =
+            std::collections::HashMap::new();
+        let datatables: Vec<String> = 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<serde_json::Value> = 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)

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,