From d0659723f26a84fbffa35ef90d4e48871a8f8df0 Mon Sep 17 00:00:00 2001 From: dal Date: Tue, 16 Sep 2025 14:32:36 -0600 Subject: [PATCH] Enhance access control by adding report permission checks and updating cascading permission logic. Refactor metric access functions to include workspace sharing information for chats, dashboards, and collections. --- .../api/v2/metric_files/get-metric-data.ts | 10 +-- .../src/assets/cascading-permissions.ts | 85 ++++++++++++++++--- packages/access-controls/src/index.ts | 1 + .../check-chats-containing-asset.ts | 11 ++- .../check-collections-containing-asset.ts | 12 ++- .../check-dashboards-containing-metric.ts | 13 ++- .../check-reports-containing-metric.ts | 30 +++++++ .../queries/cascading-permissions/index.ts | 1 + 8 files changed, 142 insertions(+), 21 deletions(-) create mode 100644 packages/database/src/queries/cascading-permissions/check-reports-containing-metric.ts diff --git a/apps/server/src/api/v2/metric_files/get-metric-data.ts b/apps/server/src/api/v2/metric_files/get-metric-data.ts index cb5f4f8e6..3cb00f404 100644 --- a/apps/server/src/api/v2/metric_files/get-metric-data.ts +++ b/apps/server/src/api/v2/metric_files/get-metric-data.ts @@ -66,11 +66,11 @@ export async function getMetricDataHandler( } // Check if user has permission to view this metric file - // This follows the same pattern as report-files.ts - hasAssetPermission handles all the logic including: - // - Direct permissions - // - Workspace sharing permissions - // - Cascading permissions (dashboard, chat, collection) - // - Admin permissions + // hasAssetPermission internally handles: + // 1. Direct permissions + // 2. Admin permissions + // 3. Workspace sharing permissions (if provided) + // 4. Cascading permissions (dashboard, chat, collection) const hasAccess = await hasAssetPermission({ userId: user.id, assetId: metricId, diff --git a/packages/access-controls/src/assets/cascading-permissions.ts b/packages/access-controls/src/assets/cascading-permissions.ts index 631c066ba..ae090e690 100644 --- a/packages/access-controls/src/assets/cascading-permissions.ts +++ b/packages/access-controls/src/assets/cascading-permissions.ts @@ -3,8 +3,9 @@ import { checkChatsContainingAsset, checkCollectionsContainingAsset, checkDashboardsContainingMetric, + checkReportsContainingMetric, } from '@buster/database'; -import type { AssetPermissionRole, AssetType } from '../types/asset-permissions'; +import type { AssetPermissionRole, AssetType, WorkspaceSharing } from '../types/asset-permissions'; import { AccessControlError } from '../types/errors'; import { getCachedCascadingPermission, setCachedCascadingPermission } from './cache'; import { hasAssetPermission } from './permissions'; @@ -15,7 +16,7 @@ import { hasAssetPermission } from './permissions'; */ export async function checkMetricDashboardAccess(metricId: string, user: User): Promise { try { - // Get all dashboards containing this metric + // Get all dashboards containing this metric with their workspace sharing info const dashboards = await checkDashboardsContainingMetric(metricId); if (!dashboards || dashboards.length === 0) { @@ -26,9 +27,11 @@ export async function checkMetricDashboardAccess(metricId: string, user: User): for (const dashboard of dashboards) { const hasAccess = await hasAssetPermission({ assetId: dashboard.id, - assetType: 'dashboard' as AssetType, + assetType: 'dashboard_file' as AssetType, userId: user.id, requiredRole: 'can_view' as AssetPermissionRole, + organizationId: dashboard.organizationId, + workspaceSharing: (dashboard.workspaceSharing as WorkspaceSharing) ?? 'none', }); if (hasAccess) { @@ -52,7 +55,7 @@ export async function checkMetricDashboardAccess(metricId: string, user: User): */ export async function checkMetricChatAccess(metricId: string, user: User): Promise { try { - // Get all chats containing this metric + // Get all chats containing this metric with their workspace sharing info const chats = await checkChatsContainingAsset(metricId, 'metric'); if (!chats || chats.length === 0) { @@ -66,6 +69,8 @@ export async function checkMetricChatAccess(metricId: string, user: User): Promi assetType: 'chat' as AssetType, userId: user.id, requiredRole: 'can_view' as AssetPermissionRole, + organizationId: chat.organizationId, + workspaceSharing: (chat.workspaceSharing as WorkspaceSharing) ?? 'none', }); if (hasAccess) { @@ -83,13 +88,52 @@ export async function checkMetricChatAccess(metricId: string, user: User): Promi } } +/** + * Check if a user has access to a metric through any report that contains it. + * If a user has access to a report (direct, public, or workspace), they can view the metrics in it. + */ +export async function checkMetricReportAccess(metricId: string, user: User): Promise { + try { + // Get all reports containing this metric with their workspace sharing info + const reports = await checkReportsContainingMetric(metricId); + + if (!reports || reports.length === 0) { + return false; + } + + // Check if user has access to any of these reports + for (const report of reports) { + const hasAccess = await hasAssetPermission({ + assetId: report.id, + assetType: 'report_file' as AssetType, + userId: user.id, + requiredRole: 'can_view' as AssetPermissionRole, + organizationId: report.organizationId, + workspaceSharing: (report.workspaceSharing as WorkspaceSharing) ?? 'none', + }); + + if (hasAccess) { + return true; + } + } + + return false; + } catch (error) { + throw new AccessControlError( + 'cascading_permission_error', + 'Failed to check metric report access', + { error } + ); + } +} + /** * Check if a user has access to a dashboard through any chat that contains it. * If a user has access to a chat (direct, public, or workspace), they can view the dashboards in it. */ export async function checkDashboardChatAccess(dashboardId: string, user: User): Promise { try { - // Get all chats containing this dashboard + // Get all chats containing this dashboard with their workspace sharing info const chats = await checkChatsContainingAsset(dashboardId, 'dashboard'); if (!chats || chats.length === 0) { @@ -103,6 +147,8 @@ export async function checkDashboardChatAccess(dashboardId: string, user: User): assetType: 'chat' as AssetType, userId: user.id, requiredRole: 'can_view' as AssetPermissionRole, + organizationId: chat.organizationId, + workspaceSharing: (chat.workspaceSharing as WorkspaceSharing) ?? 'none', }); if (hasAccess) { @@ -126,7 +172,7 @@ export async function checkDashboardChatAccess(dashboardId: string, user: User): */ export async function checkMetricCollectionAccess(metricId: string, user: User): Promise { try { - // Get all collections containing this metric + // Get all collections containing this metric with their workspace sharing info const collections = await checkCollectionsContainingAsset(metricId, 'metric'); if (!collections || collections.length === 0) { @@ -140,6 +186,8 @@ export async function checkMetricCollectionAccess(metricId: string, user: User): assetType: 'collection' as AssetType, userId: user.id, requiredRole: 'can_view' as AssetPermissionRole, + organizationId: collection.organizationId, + workspaceSharing: (collection.workspaceSharing as WorkspaceSharing) ?? 'none', }); if (hasAccess) { @@ -166,7 +214,7 @@ export async function checkDashboardCollectionAccess( user: User ): Promise { try { - // Get all collections containing this dashboard + // Get all collections containing this dashboard with their workspace sharing info const collections = await checkCollectionsContainingAsset(dashboardId, 'dashboard'); if (!collections || collections.length === 0) { @@ -180,6 +228,8 @@ export async function checkDashboardCollectionAccess( assetType: 'collection' as AssetType, userId: user.id, requiredRole: 'can_view' as AssetPermissionRole, + organizationId: collection.organizationId, + workspaceSharing: (collection.workspaceSharing as WorkspaceSharing) ?? 'none', }); if (hasAccess) { @@ -203,7 +253,7 @@ export async function checkDashboardCollectionAccess( */ export async function checkChatCollectionAccess(chatId: string, user: User): Promise { try { - // Get all collections containing this chat + // Get all collections containing this chat with their workspace sharing info const collections = await checkCollectionsContainingAsset(chatId, 'chat'); if (!collections || collections.length === 0) { @@ -217,6 +267,8 @@ export async function checkChatCollectionAccess(chatId: string, user: User): Pro assetType: 'collection' as AssetType, userId: user.id, requiredRole: 'can_view' as AssetPermissionRole, + organizationId: collection.organizationId, + workspaceSharing: (collection.workspaceSharing as WorkspaceSharing) ?? 'none', }); if (hasAccess) { @@ -253,8 +305,9 @@ export async function checkCascadingPermissions( let hasAccess = false; switch (assetType) { - case 'metric': { - // Check access through dashboards, chats, and collections + case 'metric': + case 'metric_file': { + // Check access through dashboards, chats, collections, and reports const dashboardAccess = await checkMetricDashboardAccess(assetId, user); if (dashboardAccess) { hasAccess = true; @@ -272,10 +325,17 @@ export async function checkCascadingPermissions( hasAccess = true; break; } + + const reportAccess = await checkMetricReportAccess(assetId, user); + if (reportAccess) { + hasAccess = true; + break; + } break; } - case 'dashboard': { + case 'dashboard': + case 'dashboard_file': { // Check access through chats and collections const dashboardChatAccess = await checkDashboardChatAccess(assetId, user); if (dashboardChatAccess) { @@ -302,7 +362,8 @@ export async function checkCascadingPermissions( } case 'collection': - // Collections don't have cascading permissions + case 'report_file': + // Collections and reports don't have cascading permissions (they're top-level) hasAccess = false; break; diff --git a/packages/access-controls/src/index.ts b/packages/access-controls/src/index.ts index 34dcac339..8ae2d943c 100644 --- a/packages/access-controls/src/index.ts +++ b/packages/access-controls/src/index.ts @@ -20,6 +20,7 @@ export { checkMetricDashboardAccess, checkMetricChatAccess, checkMetricCollectionAccess, + checkMetricReportAccess, } from './assets'; // Export dataset permissions diff --git a/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts b/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts index 7dfe802eb..56eaf0d06 100644 --- a/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts +++ b/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts @@ -1,14 +1,23 @@ import { and, eq, isNull } from 'drizzle-orm'; import { db } from '../../connection'; import { chats, messages, messagesToFiles } from '../../schema'; +import type { WorkspaceSharing } from '../../schema-types'; + +export interface ChatWithSharing { + id: string; + organizationId: string; + workspaceSharing: WorkspaceSharing | null; +} export async function checkChatsContainingAsset( assetId: string, _assetType: 'metric' | 'dashboard' -): Promise<{ id: string }[]> { +): Promise { const result = await db .selectDistinct({ id: chats.id, + organizationId: chats.organizationId, + workspaceSharing: chats.workspaceSharing, }) .from(messagesToFiles) .innerJoin(messages, eq(messages.id, messagesToFiles.messageId)) diff --git a/packages/database/src/queries/cascading-permissions/check-collections-containing-asset.ts b/packages/database/src/queries/cascading-permissions/check-collections-containing-asset.ts index 0c55f6f35..0216f94a6 100644 --- a/packages/database/src/queries/cascading-permissions/check-collections-containing-asset.ts +++ b/packages/database/src/queries/cascading-permissions/check-collections-containing-asset.ts @@ -1,12 +1,18 @@ import { and, eq, isNull } from 'drizzle-orm'; import { db } from '../../connection'; import { collections, collectionsToAssets } from '../../schema'; -import type { AssetType } from '../../schema-types'; +import type { AssetType, WorkspaceSharing } from '../../schema-types'; + +export interface CollectionWithSharing { + id: string; + organizationId: string; + workspaceSharing: WorkspaceSharing | null; +} export async function checkCollectionsContainingAsset( assetId: string, assetType: 'metric' | 'dashboard' | 'chat' -): Promise<{ id: string }[]> { +): Promise { // Map our asset type strings to the enum values expected by the database const assetTypeMap: Record = { metric: 'metric_file', @@ -22,6 +28,8 @@ export async function checkCollectionsContainingAsset( const result = await db .select({ id: collections.id, + organizationId: collections.organizationId, + workspaceSharing: collections.workspaceSharing, }) .from(collectionsToAssets) .innerJoin(collections, eq(collections.id, collectionsToAssets.collectionId)) diff --git a/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.ts b/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.ts index 73fe9132d..1a3d292fc 100644 --- a/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.ts +++ b/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.ts @@ -1,11 +1,22 @@ import { and, eq, isNull } from 'drizzle-orm'; import { db } from '../../connection'; import { dashboardFiles, metricFilesToDashboardFiles } from '../../schema'; +import type { WorkspaceSharing } from '../../schema-types'; -export async function checkDashboardsContainingMetric(metricId: string): Promise<{ id: string }[]> { +export interface DashboardWithSharing { + id: string; + organizationId: string; + workspaceSharing: WorkspaceSharing | null; +} + +export async function checkDashboardsContainingMetric( + metricId: string +): Promise { const result = await db .select({ id: dashboardFiles.id, + organizationId: dashboardFiles.organizationId, + workspaceSharing: dashboardFiles.workspaceSharing, }) .from(metricFilesToDashboardFiles) .innerJoin(dashboardFiles, eq(dashboardFiles.id, metricFilesToDashboardFiles.dashboardFileId)) diff --git a/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.ts b/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.ts new file mode 100644 index 000000000..e624d6e4b --- /dev/null +++ b/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.ts @@ -0,0 +1,30 @@ +import { and, eq, isNull } from 'drizzle-orm'; +import { db } from '../../connection'; +import { metricFilesToReportFiles, reportFiles } from '../../schema'; +import type { WorkspaceSharing } from '../../schema-types'; + +export interface ReportWithSharing { + id: string; + organizationId: string; + workspaceSharing: WorkspaceSharing | null; +} + +export async function checkReportsContainingMetric(metricId: string): Promise { + const result = await db + .select({ + id: reportFiles.id, + organizationId: reportFiles.organizationId, + workspaceSharing: reportFiles.workspaceSharing, + }) + .from(metricFilesToReportFiles) + .innerJoin(reportFiles, eq(reportFiles.id, metricFilesToReportFiles.reportFileId)) + .where( + and( + eq(metricFilesToReportFiles.metricFileId, metricId), + isNull(metricFilesToReportFiles.deletedAt), + isNull(reportFiles.deletedAt) + ) + ); + + return result; +} diff --git a/packages/database/src/queries/cascading-permissions/index.ts b/packages/database/src/queries/cascading-permissions/index.ts index 36fc54d8b..bcdffc5b4 100644 --- a/packages/database/src/queries/cascading-permissions/index.ts +++ b/packages/database/src/queries/cascading-permissions/index.ts @@ -1,3 +1,4 @@ export * from './check-dashboards-containing-metric'; export * from './check-chats-containing-asset'; export * from './check-collections-containing-asset'; +export * from './check-reports-containing-metric';