From e536c035b62e6922bf0334a21ae4770ab2b559ac Mon Sep 17 00:00:00 2001 From: Artem Niehrieiev Date: Mon, 12 Oct 2026 13:25:04 +0000 Subject: [PATCH] refactor: optimize CedarPermissionsService for batched access level checks - Introduced methods for evaluating connection and group access levels in a batched manner to reduce database queries. - Replaced individual access level checks with batched counterparts in various use cases, improving performance. - Updated the `getReadableTables` method to cache results and minimize repeated group lookups. - Enhanced tests to cover new batched functionality and ensure consistent permission checks across different scenarios. --- ...est-info-from-table-with-ai-v7.use.case.ts | 74 +++--- .../cedar-permissions.service.ts | 251 ++++++++++-------- .../use-cases/find-one-connection.use.case.ts | 6 +- .../get-user-groups-in-connection.use.case.ts | 12 +- .../group-custom-repository-extension.ts | 1 + .../find-all-user-groups.use.case.ts | 14 +- .../filter-foreign-keys-by-permission.util.ts | 12 +- .../utils/process-referenced-tables.util.ts | 25 +- .../user-access.repository.interface.ts | 5 + .../panel/use-cases/execute-panel.use.case.ts | 13 +- .../panel/use-cases/test-db-query.use.case.ts | 13 +- .../assert-query-tables-readable.util.ts | 20 +- .../execute-ai-raw-query.use.case.ts | 4 +- .../get-ai-connection-tables.use.case.ts | 8 +- .../utils/ai-data-access.helpers.ts | 66 +++-- .../cedar-permissions-batched-checks.test.ts | 249 +++++++++++++++++ 16 files changed, 505 insertions(+), 268 deletions(-) create mode 100644 backend/test/ava-tests/unit-tests/cedar-permissions-batched-checks.test.ts diff --git a/backend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.ts b/backend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.ts index 18cdbb6a5..4285f14fa 100644 --- a/backend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.ts +++ b/backend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.ts @@ -276,8 +276,8 @@ export class RequestInfoFromTableWithAIUseCaseV7 query, connectionType: foundConnection.type as ConnectionTypesEnum, connectionId: foundConnection.id, - validateTableRead: (referencedTableName) => - this.cedarPermissions.improvedCheckTableRead(userId, foundConnection.id, referencedTableName), + getReadableTables: (tableNames) => + this.cedarPermissions.getReadableTables(userId, foundConnection.id, tableNames), listAllTableNames: async () => (await dataAccessObject.getTablesFromDB()).map((table) => table.tableName), }); const wrappedQuery = wrapQueryWithLimit(query, foundConnection.type as ConnectionTypesEnum); @@ -354,39 +354,26 @@ export class RequestInfoFromTableWithAIUseCaseV7 // Only expose the structure of related tables the user is permitted to // read — otherwise foreign-key traversal would leak the schema of tables - // the user has no access to. - const referencedTablesStructures = []; - const structurePromises = referencedTableNamesAndColumns.flatMap((referencedTable) => - referencedTable.referenced_by.map(async (table) => { - const canRead = await this.cedarPermissions.improvedCheckTableRead( - userId, - foundConnection.id, - table.table_name, - ); - if (!canRead) { - return null; - } - const structure = await dao.getTableStructure(table.table_name, userEmail); - return { tableName: table.table_name, structure }; - }), + // the user has no access to. One batched permission check covers both + // directions of the relation. + const referencedByTableNames = referencedTableNamesAndColumns.flatMap((referencedTable) => + referencedTable.referenced_by.map((table) => table.table_name), ); - referencedTablesStructures.push(...(await Promise.all(structurePromises)).filter((item) => item !== null)); - - const foreignTablesStructures = []; - const foreignTablesStructurePromises = tableForeignKeys.map(async (foreignKey) => { - const canRead = await this.cedarPermissions.improvedCheckTableRead( - userId, - foundConnection.id, - foreignKey.referenced_table_name, - ); - if (!canRead) { - return null; - } - const structure = await dao.getTableStructure(foreignKey.referenced_table_name, userEmail); - return { tableName: foreignKey.referenced_table_name, structure }; - }); - foreignTablesStructures.push( - ...(await Promise.all(foreignTablesStructurePromises)).filter((item) => item !== null), + const foreignTableNames = tableForeignKeys.map((foreignKey) => foreignKey.referenced_table_name); + const readableTables = await this.cedarPermissions.getReadableTables(userId, foundConnection.id, [ + ...referencedByTableNames, + ...foreignTableNames, + ]); + + const referencedTablesStructures = await Promise.all( + referencedByTableNames + .filter((name) => readableTables.has(name)) + .map(async (name) => ({ tableName: name, structure: await dao.getTableStructure(name, userEmail) })), + ); + const foreignTablesStructures = await Promise.all( + foreignTableNames + .filter((name) => readableTables.has(name)) + .map(async (name) => ({ tableName: name, structure: await dao.getTableStructure(name, userEmail) })), ); return { @@ -416,15 +403,16 @@ export class RequestInfoFromTableWithAIUseCaseV7 new Set(tableNames.map((name) => name?.trim()).filter((name): name is string => Boolean(name))), ); - for (const tableName of uniqueTableNames) { - const canRead = await this.cedarPermissions.improvedCheckTableRead(userId, connectionId, tableName); - if (!canRead) { - this.logger.warn( - `AI request blocked for user ${userId} on connection ${connectionId}: ` + - `no read permission for table "${tableName}"`, - ); - throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(tableName)); - } + if (uniqueTableNames.length === 0) return; + + const readableTables = await this.cedarPermissions.getReadableTables(userId, connectionId, uniqueTableNames); + const blockedTable = uniqueTableNames.find((tableName) => !readableTables.has(tableName)); + if (blockedTable !== undefined) { + this.logger.warn( + `AI request blocked for user ${userId} on connection ${connectionId}: ` + + `no read permission for table "${blockedTable}"`, + ); + throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(blockedTable)); } } diff --git a/backend/src/entities/cedar-authorization/cedar-permissions.service.ts b/backend/src/entities/cedar-authorization/cedar-permissions.service.ts index 626a73e44..c9b8b6a5b 100644 --- a/backend/src/entities/cedar-authorization/cedar-permissions.service.ts +++ b/backend/src/entities/cedar-authorization/cedar-permissions.service.ts @@ -37,33 +37,7 @@ export class CedarPermissionsService implements IUserAccessRepository { async getUserConnectionAccessLevel(cognitoUserName: string, connectionId: string): Promise { const ctx = await this.loadContext(connectionId, cognitoUserName); if (!ctx) return AccessLevelEnum.none; - - const entities = buildCedarEntities(cognitoUserName, ctx.userGroups, connectionId); - if ( - this.evaluatePolicies( - cognitoUserName, - CedarAction.ConnectionEdit, - CedarResourceType.Connection, - connectionId, - ctx.policies, - entities, - ) - ) { - return AccessLevelEnum.edit; - } - if ( - this.evaluatePolicies( - cognitoUserName, - CedarAction.ConnectionRead, - CedarResourceType.Connection, - connectionId, - ctx.policies, - entities, - ) - ) { - return AccessLevelEnum.readonly; - } - return AccessLevelEnum.none; + return this.evaluateConnectionAccessLevel(cognitoUserName, connectionId, ctx); } async getUserConnectionAccessLevelsForMultipleConnections( @@ -73,61 +47,14 @@ export class CedarPermissionsService implements IUserAccessRepository { const result = new Map(); if (connectionIds.length === 0) return result; - const allGroups = await this.globalDbContext.groupRepository.findAllUserGroupsInConnections(connectionIds, userId); - - const groupsByConnection = new Map>(); - for (const group of allGroups) { - const connId = group.connection?.id; - if (!connId) continue; - let connectionGroups = groupsByConnection.get(connId); - if (!connectionGroups) { - connectionGroups = []; - groupsByConnection.set(connId, connectionGroups); - } - connectionGroups.push(group); - } - + const contexts = await this.loadContexts(connectionIds, userId); for (const connectionId of connectionIds) { - const userGroups = groupsByConnection.get(connectionId); - if (!userGroups || userGroups.length === 0) { - result.set(connectionId, AccessLevelEnum.none); - continue; - } - - const policies = userGroups.map((g) => g.cedarPolicy).filter((policy): policy is string => Boolean(policy)); - if (policies.length === 0) { - result.set(connectionId, AccessLevelEnum.none); - continue; - } - - const entities = buildCedarEntities(userId, userGroups, connectionId); - if ( - this.evaluatePolicies( - userId, - CedarAction.ConnectionEdit, - CedarResourceType.Connection, - connectionId, - policies, - entities, - ) - ) { - result.set(connectionId, AccessLevelEnum.edit); - } else if ( - this.evaluatePolicies( - userId, - CedarAction.ConnectionRead, - CedarResourceType.Connection, - connectionId, - policies, - entities, - ) - ) { - result.set(connectionId, AccessLevelEnum.readonly); - } else { - result.set(connectionId, AccessLevelEnum.none); - } + const ctx = contexts.get(connectionId); + result.set( + connectionId, + ctx ? this.evaluateConnectionAccessLevel(userId, connectionId, ctx) : AccessLevelEnum.none, + ); } - return result; } @@ -175,34 +102,36 @@ export class CedarPermissionsService implements IUserAccessRepository { const connectionId = await this.getConnectionId(groupId); const ctx = await this.loadContext(connectionId, cognitoUserName); if (!ctx) return AccessLevelEnum.none; + return this.evaluateGroupAccessLevel(cognitoUserName, connectionId, groupId, ctx); + } - const entities = buildCedarEntities(cognitoUserName, ctx.userGroups, connectionId); - const resourceId = groupId; - if ( - this.evaluatePolicies( - cognitoUserName, - CedarAction.GroupEdit, - CedarResourceType.Group, - resourceId, - ctx.policies, - entities, - ) - ) { - return AccessLevelEnum.edit; - } - if ( - this.evaluatePolicies( - cognitoUserName, - CedarAction.GroupRead, - CedarResourceType.Group, - resourceId, - ctx.policies, - entities, - ) - ) { - return AccessLevelEnum.readonly; + // Batched counterpart of getGroupAccessLevel: one group lookup for every connection the given + // groups belong to, instead of two queries per group. Each group must carry its `connection` + // relation (the repository finders that return groups for a user or a connection all join it). + async getGroupAccessLevels(userId: string, groups: Array): Promise> { + const result = new Map(); + if (groups.length === 0) return result; + + const groupConnections = groups.map((group) => { + const connectionId = group.connection?.id; + if (!connectionId) { + throw new Error(`getGroupAccessLevels: group ${group.id} was loaded without its connection`); + } + return { groupId: group.id, connectionId }; + }); + + const contexts = await this.loadContexts( + Array.from(new Set(groupConnections.map(({ connectionId }) => connectionId))), + userId, + ); + for (const { groupId, connectionId } of groupConnections) { + const ctx = contexts.get(connectionId); + result.set( + groupId, + ctx ? this.evaluateGroupAccessLevel(userId, connectionId, groupId, ctx) : AccessLevelEnum.none, + ); } - return AccessLevelEnum.none; + return result; } async checkUserGroupRead(cognitoUserName: string, groupId: string): Promise { @@ -450,6 +379,31 @@ export class CedarPermissionsService implements IUserAccessRepository { return canRead; } + // Batched counterpart of improvedCheckTableRead: the user's groups are loaded once for the whole + // list instead of once per table. Cached verdicts are honoured and fresh ones are cached the same + // way, so the two stay interchangeable. Returns the subset of `tableNames` the user may query. + async getReadableTables(userId: string, connectionId: string, tableNames: Array): Promise> { + const readable = new Set(); + const uncached: Array = []; + for (const tableName of new Set(tableNames)) { + const cached = Cacher.getUserTableReadPermissionCache(userId, connectionId, tableName); + if (cached === null) { + uncached.push(tableName); + } else if (cached) { + readable.add(tableName); + } + } + if (uncached.length === 0) return readable; + + const ctx = await this.loadContext(connectionId, userId); + for (const tableName of uncached) { + const canRead = ctx ? this.evaluateTableQuery(userId, connectionId, tableName, ctx) : false; + Cacher.setUserTableReadPermissionCache(userId, connectionId, tableName, canRead); + if (canRead) readable.add(tableName); + } + return readable; + } + private evaluateTablePermissions( userId: string, connectionId: string, @@ -614,16 +568,93 @@ export class CedarPermissionsService implements IUserAccessRepository { return false; } - private async loadContext(connectionId: string, userId: string): Promise { - const userGroups = await this.globalDbContext.groupRepository.findAllUserGroupsInConnection(connectionId, userId); - if (userGroups.length === 0) return null; + private evaluateConnectionAccessLevel(userId: string, connectionId: string, ctx: EvalContext): AccessLevelEnum { + const entities = buildCedarEntities(userId, ctx.userGroups, connectionId); + if ( + this.evaluatePolicies( + userId, + CedarAction.ConnectionEdit, + CedarResourceType.Connection, + connectionId, + ctx.policies, + entities, + ) + ) { + return AccessLevelEnum.edit; + } + if ( + this.evaluatePolicies( + userId, + CedarAction.ConnectionRead, + CedarResourceType.Connection, + connectionId, + ctx.policies, + entities, + ) + ) { + return AccessLevelEnum.readonly; + } + return AccessLevelEnum.none; + } + private evaluateGroupAccessLevel( + userId: string, + connectionId: string, + groupId: string, + ctx: EvalContext, + ): AccessLevelEnum { + const entities = buildCedarEntities(userId, ctx.userGroups, connectionId); + if ( + this.evaluatePolicies(userId, CedarAction.GroupEdit, CedarResourceType.Group, groupId, ctx.policies, entities) + ) { + return AccessLevelEnum.edit; + } + if ( + this.evaluatePolicies(userId, CedarAction.GroupRead, CedarResourceType.Group, groupId, ctx.policies, entities) + ) { + return AccessLevelEnum.readonly; + } + return AccessLevelEnum.none; + } + + private buildContext(userGroups: Array): EvalContext | null { + if (userGroups.length === 0) return null; const policies = userGroups.map((g) => g.cedarPolicy).filter((policy): policy is string => Boolean(policy)); if (policies.length === 0) return null; - return { userGroups, policies }; } + private async loadContext(connectionId: string, userId: string): Promise { + const userGroups = await this.globalDbContext.groupRepository.findAllUserGroupsInConnection(connectionId, userId); + return this.buildContext(userGroups); + } + + // One query for several connections; connections where the user has no group (or no policy) + // are simply absent from the result. + private async loadContexts(connectionIds: Array, userId: string): Promise> { + const contexts = new Map(); + if (connectionIds.length === 0) return contexts; + + const allGroups = await this.globalDbContext.groupRepository.findAllUserGroupsInConnections(connectionIds, userId); + const groupsByConnection = new Map>(); + for (const group of allGroups) { + const connId = group.connection?.id; + if (!connId) continue; + let connectionGroups = groupsByConnection.get(connId); + if (!connectionGroups) { + connectionGroups = []; + groupsByConnection.set(connId, connectionGroups); + } + connectionGroups.push(group); + } + + for (const [connectionId, userGroups] of groupsByConnection) { + const ctx = this.buildContext(userGroups); + if (ctx) contexts.set(connectionId, ctx); + } + return contexts; + } + // Public-access context: no groups, a single policy taken from the connection's // public_cedar_policy. The synthetic principal is irrelevant since generated policies leave the // principal unconstrained. diff --git a/backend/src/entities/connection/use-cases/find-one-connection.use.case.ts b/backend/src/entities/connection/use-cases/find-one-connection.use.case.ts index 198079a8e..44ff65b84 100644 --- a/backend/src/entities/connection/use-cases/find-one-connection.use.case.ts +++ b/backend/src/entities/connection/use-cases/find-one-connection.use.case.ts @@ -112,9 +112,9 @@ export class FindOneConnectionUseCase return true; } const groupsInConnection = await this._dbContext.groupRepository.findAllGroupsInConnection(connectionId); - for (const group of groupsInConnection) { - const groupRead = await this.cedarPermissions.checkUserGroupRead(cognitoUserName, group.id); - if (groupRead) { + const accessLevels = await this.cedarPermissions.getGroupAccessLevels(cognitoUserName, groupsInConnection); + for (const level of accessLevels.values()) { + if (level === AccessLevelEnum.edit || level === AccessLevelEnum.readonly) { return true; } } diff --git a/backend/src/entities/connection/use-cases/get-user-groups-in-connection.use.case.ts b/backend/src/entities/connection/use-cases/get-user-groups-in-connection.use.case.ts index eb9f8ccfc..fc5ba3b1e 100644 --- a/backend/src/entities/connection/use-cases/get-user-groups-in-connection.use.case.ts +++ b/backend/src/entities/connection/use-cases/get-user-groups-in-connection.use.case.ts @@ -2,6 +2,7 @@ import { Inject, Injectable } from '@nestjs/common'; import AbstractUseCase from '../../../common/abstract-use.case.js'; import { IGlobalDatabaseContext } from '../../../common/application/global-database-context.interface.js'; import { BaseType } from '../../../common/data-injection.tokens.js'; +import { AccessLevelEnum } from '../../../enums/access-level.enum.js'; import { CedarPermissionsService } from '../../cedar-authorization/cedar-permissions.service.js'; import { GroupEntity } from '../../group/group.entity.js'; import { GetGroupsInConnectionDs } from '../application/data-structures/get-groups-in-connection.ds.js'; @@ -27,14 +28,9 @@ export class GetUserGroupsInConnectionUseCase inputData.connectionId, inputData.cognitoUserName, ); - return await Promise.all( - userGroups.map(async (group: GroupEntity) => { - const userAccessLevel = await this.cedarPermissions.getGroupAccessLevel( - inputData.cognitoUserName, - group.id, - ); - return buildFoundUserGroupInConnectionDto(group, userAccessLevel); - }), + const accessLevels = await this.cedarPermissions.getGroupAccessLevels(inputData.cognitoUserName, userGroups); + return userGroups.map((group: GroupEntity) => + buildFoundUserGroupInConnectionDto(group, accessLevels.get(group.id) ?? AccessLevelEnum.none), ); } } diff --git a/backend/src/entities/group/repository/group-custom-repository-extension.ts b/backend/src/entities/group/repository/group-custom-repository-extension.ts index c50152830..6335dce81 100644 --- a/backend/src/entities/group/repository/group-custom-repository-extension.ts +++ b/backend/src/entities/group/repository/group-custom-repository-extension.ts @@ -55,6 +55,7 @@ export const groupCustomRepositoryExtension: IGroupRepository = { async findAllUserGroups(userId: string): Promise> { const qb = this.createQueryBuilder('group') + .leftJoinAndSelect('group.connection', 'connection') .leftJoinAndSelect('group.users', 'user') .andWhere('user.id = :userId', { userId: userId }); return await qb.getMany(); diff --git a/backend/src/entities/group/use-cases/find-all-user-groups.use.case.ts b/backend/src/entities/group/use-cases/find-all-user-groups.use.case.ts index e08d4abb3..14035b519 100644 --- a/backend/src/entities/group/use-cases/find-all-user-groups.use.case.ts +++ b/backend/src/entities/group/use-cases/find-all-user-groups.use.case.ts @@ -20,18 +20,14 @@ export class FindAllUserGroupsUseCase extends AbstractUseCase { const foundUserGroups: Array = await this._dbContext.groupRepository.findAllUserGroups(userId); + const accessLevels = await this.cedarPermissions.getGroupAccessLevels(userId, foundUserGroups); const groupsWithAccessLevels: Array<{ group: GroupEntity; accessLevel: AccessLevelEnum; - }> = await Promise.all( - foundUserGroups.map(async (group: GroupEntity) => { - const accessLevel = await this.cedarPermissions.getGroupAccessLevel(userId, group.id); - return { - group: group, - accessLevel: accessLevel, - }; - }), - ); + }> = foundUserGroups.map((group: GroupEntity) => ({ + group: group, + accessLevel: accessLevels.get(group.id) ?? AccessLevelEnum.none, + })); return { groups: groupsWithAccessLevels.map((g) => { diff --git a/backend/src/entities/table/utils/filter-foreign-keys-by-permission.util.ts b/backend/src/entities/table/utils/filter-foreign-keys-by-permission.util.ts index 03e7259c7..94e8c2a4a 100644 --- a/backend/src/entities/table/utils/filter-foreign-keys-by-permission.util.ts +++ b/backend/src/entities/table/utils/filter-foreign-keys-by-permission.util.ts @@ -5,15 +5,13 @@ export async function filterForeignKeysByReadPermission( foreignKeys: Array, userId: string, connectionId: string, - masterPwd: string, + _masterPwd: string, cedarPermissions: CedarPermissionsService, ): Promise> { - const canReadResults = await Promise.all( - foreignKeys.map(async (fk) => ({ - tableName: fk.referenced_table_name, - canRead: await cedarPermissions.improvedCheckTableRead(userId, connectionId, fk.referenced_table_name, masterPwd), - })), + const readableSet = await cedarPermissions.getReadableTables( + userId, + connectionId, + foreignKeys.map((fk) => fk.referenced_table_name), ); - const readableSet = new Set(canReadResults.filter((r) => r.canRead).map((r) => r.tableName)); return foreignKeys.filter((fk) => readableSet.has(fk.referenced_table_name)); } diff --git a/backend/src/entities/table/utils/process-referenced-tables.util.ts b/backend/src/entities/table/utils/process-referenced-tables.util.ts index a38dcf95e..aee64bad5 100644 --- a/backend/src/entities/table/utils/process-referenced-tables.util.ts +++ b/backend/src/entities/table/utils/process-referenced-tables.util.ts @@ -6,26 +6,17 @@ export async function filterReferencedTablesByPermission( referencedTables: Array, userId: string, connectionId: string, - masterPwd: string, + _masterPwd: string, cedarPermissions: CedarPermissionsService, ): Promise { - await Promise.all( - referencedTables.map(async (referencedTable) => { - referencedTable.referenced_by = ( - await Promise.all( - referencedTable.referenced_by.map(async (ref) => { - const canRead = await cedarPermissions.improvedCheckTableRead( - userId, - connectionId, - ref.table_name, - masterPwd, - ); - return canRead ? ref : null; - }), - ) - ).filter((ref): ref is { table_name: string; column_name: string } => ref !== null); - }), + const readableTables = await cedarPermissions.getReadableTables( + userId, + connectionId, + referencedTables.flatMap((referencedTable) => referencedTable.referenced_by.map((ref) => ref.table_name)), ); + for (const referencedTable of referencedTables) { + referencedTable.referenced_by = referencedTable.referenced_by.filter((ref) => readableTables.has(ref.table_name)); + } } export async function enrichReferencedTablesWithDisplayNames( diff --git a/backend/src/entities/user-access/repository/user-access.repository.interface.ts b/backend/src/entities/user-access/repository/user-access.repository.interface.ts index 22764be99..9119b78a4 100644 --- a/backend/src/entities/user-access/repository/user-access.repository.interface.ts +++ b/backend/src/entities/user-access/repository/user-access.repository.interface.ts @@ -1,4 +1,5 @@ import { AccessLevelEnum } from '../../../enums/access-level.enum.js'; +import { GroupEntity } from '../../group/group.entity.js'; import { ITablePermissionData } from '../../permission/permission.interface.js'; export interface IUserAccessRepository { @@ -10,6 +11,8 @@ export interface IUserAccessRepository { getGroupAccessLevel(cognitoUserName: string, groupId: string): Promise; + getGroupAccessLevels(userId: string, groups: Array): Promise>; + checkUserGroupRead(cognitoUserName: string, groupId: string): Promise; checkUserGroupEdit(cognitoUserName: string, groupId: string): Promise; @@ -43,4 +46,6 @@ export interface IUserAccessRepository { getConnectionId(groupId: string): Promise; improvedCheckTableRead(userId: string, connectionId: string, tableName: string, masterPwd?: string): Promise; + + getReadableTables(userId: string, connectionId: string, tableNames: Array): Promise>; } diff --git a/backend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.ts b/backend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.ts index f51be9bcd..aa06f9d24 100644 --- a/backend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.ts +++ b/backend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.ts @@ -7,8 +7,7 @@ import { BaseType } from '../../../../common/data-injection.tokens.js'; import { ConnectionNotFoundException } from '../../../../exceptions/custom-exceptions/connection-not-found-exception.js'; import { Messages } from '../../../../exceptions/text/messages.js'; import { isConnectionTypeAgent } from '../../../../helpers/is-connection-entity-agent.js'; -import { CedarAction } from '../../../cedar-authorization/cedar-action-map.js'; -import { CedarAuthorizationService } from '../../../cedar-authorization/cedar-authorization.service.js'; +import { CedarPermissionsService } from '../../../cedar-authorization/cedar-permissions.service.js'; import { ExecuteSavedDbQueryDs } from '../data-structures/execute-saved-db-query.ds.js'; import { ExecuteSavedDbQueryResultDto } from '../dto/execute-saved-db-query-result.dto.js'; import { assertUserCanReadQueryTables } from '../utils/assert-query-tables-readable.util.js'; @@ -23,7 +22,7 @@ export class ExecuteSavedDbQueryUseCase constructor( @Inject(BaseType.GLOBAL_DB_CONTEXT) protected _dbContext: IGlobalDatabaseContext, - private readonly cedarAuthService: CedarAuthorizationService, + private readonly cedarPermissions: CedarPermissionsService, ) { super(); } @@ -54,13 +53,7 @@ export class ExecuteSavedDbQueryUseCase query: foundQuery.query_text, connectionType: foundConnection.type as ConnectionTypesEnum, connectionId, - validateTableRead: (referencedTableName) => - this.cedarAuthService.validate({ - userId, - action: CedarAction.TableQuery, - connectionId, - tableName: referencedTableName, - }), + getReadableTables: (tableNames) => this.cedarPermissions.getReadableTables(userId, connectionId, tableNames), listAllTableNames: async () => (await dao.getTablesFromDB()).map((table) => table.tableName), }); diff --git a/backend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.ts b/backend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.ts index c33250aca..834d2f046 100644 --- a/backend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.ts +++ b/backend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.ts @@ -6,8 +6,7 @@ import { IGlobalDatabaseContext } from '../../../../common/application/global-da import { BaseType } from '../../../../common/data-injection.tokens.js'; import { ConnectionNotFoundException } from '../../../../exceptions/custom-exceptions/connection-not-found-exception.js'; import { isConnectionTypeAgent } from '../../../../helpers/is-connection-entity-agent.js'; -import { CedarAction } from '../../../cedar-authorization/cedar-action-map.js'; -import { CedarAuthorizationService } from '../../../cedar-authorization/cedar-authorization.service.js'; +import { CedarPermissionsService } from '../../../cedar-authorization/cedar-permissions.service.js'; import { TestDbQueryDs } from '../data-structures/test-db-query.ds.js'; import { TestDbQueryResultDto } from '../dto/test-db-query-result.dto.js'; import { assertUserCanReadQueryTables } from '../utils/assert-query-tables-readable.util.js'; @@ -19,7 +18,7 @@ export class TestDbQueryUseCase extends AbstractUseCase - this.cedarAuthService.validate({ - userId, - action: CedarAction.TableQuery, - connectionId, - tableName: referencedTableName, - }), + getReadableTables: (tableNames) => this.cedarPermissions.getReadableTables(userId, connectionId, tableNames), listAllTableNames: async () => (await dao.getTablesFromDB()).map((table) => table.tableName), }); diff --git a/backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts b/backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts index 0488e196c..ee3519d40 100644 --- a/backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts +++ b/backend/src/entities/visualizations/panel/utils/assert-query-tables-readable.util.ts @@ -8,8 +8,8 @@ interface AssertQueryTablesReadableParams { query: string; connectionType: ConnectionTypesEnum; connectionId: string; - /** Resolves to `true` when the user is allowed to read the given table (table:read). */ - validateTableRead: (tableName: string) => Promise; + /** Resolves to the subset of the given tables the user is allowed to read (table:read). */ + getReadableTables: (tableNames: Array) => Promise>; /** Lists every table in the connection; used for the all-tables fallback check. */ listAllTableNames: () => Promise>; } @@ -25,7 +25,7 @@ interface AssertQueryTablesReadableParams { * full read access run such queries. */ export async function assertUserCanReadQueryTables(params: AssertQueryTablesReadableParams): Promise { - const { query, connectionType, connectionId, validateTableRead, listAllTableNames } = params; + const { query, connectionType, connectionId, getReadableTables, listAllTableNames } = params; const collected = collectQueryTables(query, connectionType); @@ -40,10 +40,14 @@ export async function assertUserCanReadQueryTables(params: AssertQueryTablesRead tablesToCheck = await listAllTableNames(); } - for (const tableName of tablesToCheck) { - const allowed = await validateTableRead(tableName); - if (!allowed) { - throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(tableName)); - } + // One batched permission check: the all-tables fallback can cover dozens of tables, and a + // per-table check would load the user's groups once for each of them. + const uniqueTables = Array.from(new Set(tablesToCheck)); + if (uniqueTables.length === 0) return; + + const readableTables = await getReadableTables(uniqueTables); + const blockedTable = uniqueTables.find((tableName) => !readableTables.has(tableName)); + if (blockedTable !== undefined) { + throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(blockedTable)); } } diff --git a/backend/src/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.ts b/backend/src/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.ts index d5f90bf57..a59c8bc08 100644 --- a/backend/src/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.ts +++ b/backend/src/microservices/agents-microservice/use-cases/execute-ai-raw-query.use.case.ts @@ -44,8 +44,8 @@ export class ExecuteAiRawQueryUseCase query, connectionType: foundConnection.type as ConnectionTypesEnum, connectionId: foundConnection.id, - validateTableRead: (referencedTableName) => - this.cedarPermissions.improvedCheckTableRead(userId, foundConnection.id, referencedTableName), + getReadableTables: (tableNames) => + this.cedarPermissions.getReadableTables(userId, foundConnection.id, tableNames), listAllTableNames: async () => (await dataAccessObject.getTablesFromDB()).map((table) => table.tableName), }); diff --git a/backend/src/microservices/agents-microservice/use-cases/get-ai-connection-tables.use.case.ts b/backend/src/microservices/agents-microservice/use-cases/get-ai-connection-tables.use.case.ts index dba854e90..6cf9173fd 100644 --- a/backend/src/microservices/agents-microservice/use-cases/get-ai-connection-tables.use.case.ts +++ b/backend/src/microservices/agents-microservice/use-cases/get-ai-connection-tables.use.case.ts @@ -34,12 +34,8 @@ export class GetAiConnectionTablesUseCase const tables = await dataAccessObject.getTablesFromDB(); const tableNames = tables.map((table) => table.tableName?.trim()).filter((name): name is string => Boolean(name)); - const readableFlags = await Promise.all( - tableNames.map((tableName) => - this.cedarPermissions.improvedCheckTableRead(userId, foundConnection.id, tableName), - ), - ); - const readableTableNames = tableNames.filter((_name, index) => readableFlags[index]); + const readableTables = await this.cedarPermissions.getReadableTables(userId, foundConnection.id, tableNames); + const readableTableNames = tableNames.filter((tableName) => readableTables.has(tableName)); return { tables: readableTableNames }; } diff --git a/backend/src/microservices/agents-microservice/utils/ai-data-access.helpers.ts b/backend/src/microservices/agents-microservice/utils/ai-data-access.helpers.ts index bb623b0bd..e785bfe5b 100644 --- a/backend/src/microservices/agents-microservice/utils/ai-data-access.helpers.ts +++ b/backend/src/microservices/agents-microservice/utils/ai-data-access.helpers.ts @@ -63,15 +63,16 @@ export async function assertUserCanReadTables( new Set(tableNames.map((name) => name?.trim()).filter((name): name is string => Boolean(name))), ); - for (const tableName of uniqueTableNames) { - const canRead = await cedarPermissions.improvedCheckTableRead(userId, connectionId, tableName); - if (!canRead) { - logger.warn( - `AI request blocked for user ${userId} on connection ${connectionId}: ` + - `no read permission for table "${tableName}"`, - ); - throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(tableName)); - } + if (uniqueTableNames.length === 0) return; + + const readableTables = await cedarPermissions.getReadableTables(userId, connectionId, uniqueTableNames); + const blockedTable = uniqueTableNames.find((tableName) => !readableTables.has(tableName)); + if (blockedTable !== undefined) { + logger.warn( + `AI request blocked for user ${userId} on connection ${connectionId}: ` + + `no read permission for table "${blockedTable}"`, + ); + throw new ForbiddenException(Messages.NO_READ_PERMISSION_FOR_TABLE(blockedTable)); } } @@ -113,33 +114,28 @@ export async function getTableStructureInfo( dao.getReferencedTableNamesAndColumns(tableName, userEmail), ]); - const referencedTablesStructures = []; - const structurePromises = referencedTableNamesAndColumns.flatMap((referencedTable) => - referencedTable.referenced_by.map(async (table) => { - const canRead = await cedarPermissions.improvedCheckTableRead(userId, foundConnection.id, table.table_name); - if (!canRead) { - return null; - } - const structure = await dao.getTableStructure(table.table_name, userEmail); - return { tableName: table.table_name, structure }; - }), + // Only expose the structure of related tables the user is permitted to read — otherwise + // foreign-key traversal would leak the schema of tables the user has no access to. One batched + // permission check covers both directions of the relation. + const referencedByTableNames = referencedTableNamesAndColumns.flatMap((referencedTable) => + referencedTable.referenced_by.map((table) => table.table_name), + ); + const foreignTableNames = tableForeignKeys.map((foreignKey) => foreignKey.referenced_table_name); + const readableTables = await cedarPermissions.getReadableTables(userId, foundConnection.id, [ + ...referencedByTableNames, + ...foreignTableNames, + ]); + + const referencedTablesStructures = await Promise.all( + referencedByTableNames + .filter((name) => readableTables.has(name)) + .map(async (name) => ({ tableName: name, structure: await dao.getTableStructure(name, userEmail) })), + ); + const foreignTablesStructures = await Promise.all( + foreignTableNames + .filter((name) => readableTables.has(name)) + .map(async (name) => ({ tableName: name, structure: await dao.getTableStructure(name, userEmail) })), ); - referencedTablesStructures.push(...(await Promise.all(structurePromises)).filter((item) => item !== null)); - - const foreignTablesStructures = []; - const foreignTablesStructurePromises = tableForeignKeys.map(async (foreignKey) => { - const canRead = await cedarPermissions.improvedCheckTableRead( - userId, - foundConnection.id, - foreignKey.referenced_table_name, - ); - if (!canRead) { - return null; - } - const structure = await dao.getTableStructure(foreignKey.referenced_table_name, userEmail); - return { tableName: foreignKey.referenced_table_name, structure }; - }); - foreignTablesStructures.push(...(await Promise.all(foreignTablesStructurePromises)).filter((item) => item !== null)); return { tableStructure, diff --git a/backend/test/ava-tests/unit-tests/cedar-permissions-batched-checks.test.ts b/backend/test/ava-tests/unit-tests/cedar-permissions-batched-checks.test.ts new file mode 100644 index 000000000..f75c19438 --- /dev/null +++ b/backend/test/ava-tests/unit-tests/cedar-permissions-batched-checks.test.ts @@ -0,0 +1,249 @@ +import { ForbiddenException } from '@nestjs/common'; +import { ConnectionTypesEnum } from '@rocketadmin/shared-code/dist/src/shared/enums/connection-types-enum.js'; +import test from 'ava'; +import { IGlobalDatabaseContext } from '../../../src/common/application/global-database-context.interface.js'; +import { CedarPermissionsService } from '../../../src/entities/cedar-authorization/cedar-permissions.service.js'; +import { generateCedarPolicyForGroup } from '../../../src/entities/cedar-authorization/cedar-policy-generator.js'; +import { GroupEntity } from '../../../src/entities/group/group.entity.js'; +import { assertUserCanReadQueryTables } from '../../../src/entities/visualizations/panel/utils/assert-query-tables-readable.util.js'; +import { AccessLevelEnum } from '../../../src/enums/access-level.enum.js'; +import { Cacher } from '../../../src/helpers/cache/cacher.js'; + +// Every Cedar permission check loads the caller's groups in the connection (one SQL query joining +// group + connection + user). Listing endpoints used to run that check once per table or per group +// inside Promise.all, which surfaced as an "N+1 query with repeating spans" on the backend. The +// batched entry points below must answer the same verdicts as the per-item ones while loading the +// groups exactly once. + +const ADMIN_USER = 'user-admin'; +const LIMITED_USER = 'user-limited'; +const OUTSIDER = 'user-outsider'; + +interface FakeGroup { + id: string; + connectionId: string; + isMain: boolean; + cedarPolicy: string | null; + userIds: Array; +} + +function toEntity(group: FakeGroup): GroupEntity { + return { + id: group.id, + title: group.id, + isMain: group.isMain, + cedarPolicy: group.cedarPolicy, + connection: { id: group.connectionId }, + users: group.userIds.map((id) => ({ id })), + } as unknown as GroupEntity; +} + +function limitedPolicy(connectionId: string, groupId: string, tables: Array): string { + return generateCedarPolicyForGroup(connectionId, false, { + connection: { connectionId, accessLevel: AccessLevelEnum.readonly }, + group: { groupId, accessLevel: AccessLevelEnum.readonly }, + tables: tables.map((tableName) => ({ + tableName, + accessLevel: { visibility: true, readonly: true, add: false, delete: false, edit: false }, + })), + }); +} + +function makeFixture(connectionId: string) { + const otherConnectionId = `${connectionId}-other`; + const groups: Array = [ + { + id: `${connectionId}-admin`, + connectionId, + isMain: true, + cedarPolicy: generateCedarPolicyForGroup(connectionId, true, { + connection: { connectionId, accessLevel: AccessLevelEnum.edit }, + group: { groupId: `${connectionId}-admin`, accessLevel: AccessLevelEnum.edit }, + tables: [], + }), + userIds: [ADMIN_USER], + }, + { + id: `${connectionId}-limited`, + connectionId, + isMain: false, + cedarPolicy: limitedPolicy(connectionId, `${connectionId}-limited`, ['orders', 'customers']), + userIds: [LIMITED_USER], + }, + { + id: `${otherConnectionId}-limited`, + connectionId: otherConnectionId, + isMain: false, + cedarPolicy: limitedPolicy(otherConnectionId, `${otherConnectionId}-limited`, ['invoices']), + userIds: [LIMITED_USER], + }, + ]; + + const calls = { single: 0, multi: 0, byId: 0 }; + const groupRepository = { + findAllUserGroupsInConnection: async (connId: string, userId: string) => { + calls.single++; + return groups.filter((g) => g.connectionId === connId && g.userIds.includes(userId)).map(toEntity); + }, + findAllUserGroupsInConnections: async (connIds: Array, userId: string) => { + calls.multi++; + return groups.filter((g) => connIds.includes(g.connectionId) && g.userIds.includes(userId)).map(toEntity); + }, + findGroupByIdWithConnectionAndUsers: async (groupId: string) => { + calls.byId++; + const found = groups.find((g) => g.id === groupId); + return found ? toEntity(found) : null; + }, + }; + const service = new CedarPermissionsService({ groupRepository } as unknown as IGlobalDatabaseContext); + return { service, calls, groups, otherConnectionId }; +} + +test.beforeEach(async () => { + await Cacher.clearAllCache(); +}); + +test.serial('getReadableTables loads the groups once for the whole table list', async (t) => { + const connectionId = 'conn-readable-batch'; + const { service, calls } = makeFixture(connectionId); + const tableNames = Array.from({ length: 74 }, (_v, i) => `table_${i}`); + + const readable = await service.getReadableTables(ADMIN_USER, connectionId, tableNames); + + t.is(readable.size, 74); + t.is(calls.single, 1); + t.is(calls.multi, 0); +}); + +test.serial('getReadableTables answers exactly what improvedCheckTableRead answers per table', async (t) => { + const connectionId = 'conn-readable-parity'; + const { service } = makeFixture(connectionId); + const tableNames = ['orders', 'customers', 'secrets', 'orders']; + + const batched = await service.getReadableTables(LIMITED_USER, connectionId, tableNames); + await Cacher.clearAllCache(); + const perTable = new Set(); + for (const tableName of tableNames) { + if (await service.improvedCheckTableRead(LIMITED_USER, connectionId, tableName)) perTable.add(tableName); + } + + t.deepEqual([...batched].sort(), ['customers', 'orders']); + t.deepEqual([...batched].sort(), [...perTable].sort()); +}); + +test.serial('getReadableTables shares the table-read cache with improvedCheckTableRead', async (t) => { + const connectionId = 'conn-readable-cache'; + const { service, calls } = makeFixture(connectionId); + + await service.getReadableTables(LIMITED_USER, connectionId, ['orders', 'secrets']); + t.is(calls.single, 1); + + // Verdicts (both allow and deny) were cached: no further group lookup for the same tables. + t.true(await service.improvedCheckTableRead(LIMITED_USER, connectionId, 'orders')); + t.false(await service.improvedCheckTableRead(LIMITED_USER, connectionId, 'secrets')); + t.deepEqual(await service.getReadableTables(LIMITED_USER, connectionId, ['orders', 'secrets']), new Set(['orders'])); + t.is(calls.single, 1); + + // A table never seen before costs one more lookup, not one per table. + await service.getReadableTables(LIMITED_USER, connectionId, ['orders', 'customers', 'audit']); + t.is(calls.single, 2); +}); + +test.serial('getReadableTables denies everything (and caches it) for a user with no groups', async (t) => { + const connectionId = 'conn-readable-outsider'; + const { service, calls } = makeFixture(connectionId); + + const readable = await service.getReadableTables(OUTSIDER, connectionId, ['orders', 'customers']); + + t.is(readable.size, 0); + t.false(await service.improvedCheckTableRead(OUTSIDER, connectionId, 'orders')); + t.is(calls.single, 1); +}); + +test.serial('getGroupAccessLevels resolves every group with one lookup across connections', async (t) => { + const connectionId = 'conn-groups-batch'; + const { service, calls, groups } = makeFixture(connectionId); + const entities = groups.map(toEntity); + + const levels = await service.getGroupAccessLevels(LIMITED_USER, entities); + + t.is(calls.multi, 1); + t.is(calls.single, 0); + t.is(calls.byId, 0); + t.is(levels.get(`${connectionId}-admin`), AccessLevelEnum.none); + t.is(levels.get(`${connectionId}-limited`), AccessLevelEnum.readonly); + t.is(levels.get(`${connectionId}-other-limited`), AccessLevelEnum.readonly); +}); + +test.serial('getGroupAccessLevels matches getGroupAccessLevel for every user and group', async (t) => { + const connectionId = 'conn-groups-parity'; + const { service, groups } = makeFixture(connectionId); + const entities = groups.map(toEntity); + + for (const userId of [ADMIN_USER, LIMITED_USER, OUTSIDER]) { + const batched = await service.getGroupAccessLevels(userId, entities); + for (const group of entities) { + t.is(batched.get(group.id), await service.getGroupAccessLevel(userId, group.id), `${userId} / ${group.id}`); + } + } +}); + +test.serial('getGroupAccessLevels refuses groups loaded without their connection', async (t) => { + const { service } = makeFixture('conn-groups-no-relation'); + const bare = { id: 'g1', title: 'g1', isMain: false, cedarPolicy: null } as unknown as GroupEntity; + + await t.throwsAsync(service.getGroupAccessLevels(ADMIN_USER, [bare]), { message: /without its connection/ }); +}); + +test('assertUserCanReadQueryTables checks all referenced tables in one batch', async (t) => { + const seen: Array> = []; + await t.notThrowsAsync( + assertUserCanReadQueryTables({ + query: 'SELECT o.id FROM orders o JOIN customers c ON c.id = o.customer_id JOIN orders o2 ON o2.id = o.id', + connectionType: ConnectionTypesEnum.postgres, + connectionId: 'conn-query', + getReadableTables: async (tableNames) => { + seen.push(tableNames); + return new Set(tableNames); + }, + listAllTableNames: async () => { + throw new Error('not expected: the query names its tables'); + }, + }), + ); + t.is(seen.length, 1); + t.deepEqual([...seen[0]].sort(), ['customers', 'orders']); +}); + +test('assertUserCanReadQueryTables rejects on the first unreadable table and falls back to all tables', async (t) => { + await t.throwsAsync( + assertUserCanReadQueryTables({ + query: 'SELECT * FROM orders JOIN secrets ON secrets.id = orders.id', + connectionType: ConnectionTypesEnum.postgres, + connectionId: 'conn-query-denied', + getReadableTables: async (tableNames) => new Set(tableNames.filter((name) => name !== 'secrets')), + listAllTableNames: async () => [], + }), + { instanceOf: ForbiddenException, message: /secrets/ }, + ); + + let listed = 0; + const seen: Array> = []; + await t.notThrowsAsync( + assertUserCanReadQueryTables({ + query: 'db.orders.find({})', + connectionType: ConnectionTypesEnum.mongodb, + connectionId: 'conn-query-fallback', + getReadableTables: async (tableNames) => { + seen.push(tableNames); + return new Set(tableNames); + }, + listAllTableNames: async () => { + listed++; + return ['a', 'b', 'a', 'c']; + }, + }), + ); + t.is(listed, 1); + t.deepEqual(seen, [['a', 'b', 'c']]); +});