diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 23efd478bd..41230fa3c8 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -1531,15 +1531,64 @@ mod tests { } } - /// A kind of object the list misses stays with its old owner while the schema changes hands, - /// which only a real catalog shows. - #[sqlx::test(migrations = false)] - async fn a_schemas_owner_change_takes_every_object_in_it(pool: sqlx::PgPool) { + /// A connection to the test's own database, the way the handlers reach a data table's. + async fn catalog_client(pool: &sqlx::PgPool) -> tokio_postgres::Client { let mut config: tokio_postgres::Config = std::env::var("DATABASE_URL").unwrap().parse().unwrap(); config.dbname(pool.connect_options().get_database().unwrap()); let (client, connection) = config.connect(tokio_postgres::NoTls).await.unwrap(); tokio::spawn(connection); + client + } + + /// What a revoke takes back is read from the catalog, per object and source, and only the + /// privileges it asks for: the planner renders exactly this, and refuses an empty read. + #[sqlx::test(migrations = false)] + async fn a_revoke_reads_back_only_what_it_asks_for(pool: sqlx::PgPool) { + let client = catalog_client(&pool).await; + // A predefined role, so that nothing is granted outside the test's own database. + client + .batch_execute( + "CREATE SCHEMA granted; + CREATE TABLE granted.g (id int); + GRANT SELECT, INSERT ON granted.g TO pg_read_all_data;", + ) + .await + .unwrap(); + let target = AclTarget::Table { schema: "granted".to_string(), table: "g".to_string() }; + let revoked = read_revoked_grants( + &client, + "db", + &target, + GrantScope::Target, + &[], + &["select".to_string()], + "pg_read_all_data", + ) + .await + .unwrap(); + assert_eq!(revoked.len(), 1, "{revoked:?}"); + assert_eq!(revoked[0].object, None); + assert_eq!(revoked[0].privileges, ["SELECT"]); + let held_none = read_revoked_grants( + &client, + "db", + &target, + GrantScope::Target, + &[], + &["update".to_string()], + "pg_read_all_data", + ) + .await + .unwrap(); + assert!(held_none.is_empty(), "{held_none:?}"); + } + + /// A kind of object the list misses stays with its old owner while the schema changes hands, + /// which only a real catalog shows. + #[sqlx::test(migrations = false)] + async fn a_schemas_owner_change_takes_every_object_in_it(pool: sqlx::PgPool) { + let client = catalog_client(&pool).await; client .batch_execute( "CREATE SCHEMA moved; diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts index 29e8d00e2d..0bf722e0fc 100644 --- a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts +++ b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts @@ -121,6 +121,30 @@ describe('revoke of a row', () => { expect(revokeScopeOf({ ...row(), objects: [{ name: 'mood', kind: 'TYPE' }] })).toBeUndefined() }) + // Whether a grant can be taken back depends on its object, so a row folding several objects is + // only revocable if each of its grants is. + it('offers none for a folded row with a source out of reach on any of its objects', () => { + const grants: AclGrant[] = [ + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('orders'), + sources: from('admin') + }, + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('salaries'), + sources: [{ role: 'admin', reachable: false }] + } + ] + const [folded] = groupGrants(grants) + expect(folded.objects).toHaveLength(2) + expect(revokeScopeOf(folded)).toBeUndefined() + // Folding reads the grants, never rewrites them. + expect(grants[0].sources[0].reachable).toBe(true) + }) + // Postgres takes a grant back only through its source: offering the revoke would promise what // the plan then refuses. it('offers none for a row with a source out of reach', () => { diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.ts b/frontend/src/lib/components/datatableAcl/aclScopes.ts index 75dbeaf88c..5a2a5f236a 100644 --- a/frontend/src/lib/components/datatableAcl/aclScopes.ts +++ b/frontend/src/lib/components/datatableAcl/aclScopes.ts @@ -121,9 +121,11 @@ export function groupGrants(grants: AclGrant[]): GroupedGrant[] { if (existing) { existing.objects.push(grant.object!) for (const source of grant.sources) { - if (!existing.sources.some((s) => s.role === source.role)) { - existing.sources.push(source) - } + const known = existing.sources.find((s) => s.role === source.role) + // Whether a role's grant can be taken back depends on the object it is on, so a row + // holds a source as reachable only if it is on every object the row folds. + if (known) known.reachable &&= source.reachable + else existing.sources.push({ ...source }) } } else { rows.push({ @@ -131,7 +133,7 @@ export function groupGrants(grants: AclGrant[]): GroupedGrant[] { privileges: grant.privileges, objects: grant.object ? [grant.object] : [], future: grant.future, - sources: [...grant.sources] + sources: grant.sources.map((s) => ({ ...s })) }) } }