From 9927808704757c25857d56904b0ca44da21143ab Mon Sep 17 00:00:00 2001 From: Wells Bunker Date: Wed, 24 Sep 2025 13:17:08 -0600 Subject: [PATCH 1/2] check for public permissions on any check --- apps/server/src/api/v2/dashboards/[id]/GET.ts | 4 + .../src/api/v2/electric-shape/report-files.ts | 4 + .../metric_files/[id]/data/get-metric-data.ts | 4 + apps/server/src/api/v2/reports/[id]/PUT.ts | 2 +- .../src/api/v2/reports/[id]/sharing/DELETE.ts | 2 +- .../src/api/v2/reports/[id]/sharing/POST.ts | 2 +- .../src/shared-helpers/asset-public-access.ts | 6 +- .../src/shared-helpers/metric-helpers.ts | 44 ++--------- apps/web/src/routes/info/getting-started.tsx | 2 +- .../src/assets/cascading-permissions.ts | 28 ++++++- .../access-controls/src/assets/checks.test.ts | 31 +------- packages/access-controls/src/assets/checks.ts | 75 +++++++++++-------- .../access-controls/src/assets/permissions.ts | 4 + .../src/assets/public-access-check.ts | 18 +++++ .../check-chats-containing-asset.ts | 4 + .../check-dashboards-containing-metric.ts | 6 ++ .../check-reports-containing-metric.ts | 6 ++ .../queries/reports/get-report-metadata.ts | 3 + 18 files changed, 142 insertions(+), 103 deletions(-) create mode 100644 packages/access-controls/src/assets/public-access-check.ts diff --git a/apps/server/src/api/v2/dashboards/[id]/GET.ts b/apps/server/src/api/v2/dashboards/[id]/GET.ts index 218e77ebb..cb3db9b43 100644 --- a/apps/server/src/api/v2/dashboards/[id]/GET.ts +++ b/apps/server/src/api/v2/dashboards/[id]/GET.ts @@ -85,6 +85,10 @@ export async function getDashboardHandler( requiredRole: 'can_view', organizationId: dashboardFile.organizationId, workspaceSharing: dashboardFile.workspaceSharing || 'none', + publiclyAccessible: dashboardFile.publiclyAccessible, + publicExpiryDate: dashboardFile.publicExpiryDate ?? undefined, + publicPassword: dashboardFile.publicPassword ?? undefined, + userSuppliedPassword: password, }); // Check public access diff --git a/apps/server/src/api/v2/electric-shape/report-files.ts b/apps/server/src/api/v2/electric-shape/report-files.ts index 3a03377a3..ded45f6f9 100644 --- a/apps/server/src/api/v2/electric-shape/report-files.ts +++ b/apps/server/src/api/v2/electric-shape/report-files.ts @@ -67,6 +67,10 @@ export const reportFilesProxyRouter = async ( requiredRole: 'can_view', organizationId: reportData.organizationId, workspaceSharing: reportData.workspaceSharing, + publiclyAccessible: reportData.publiclyAccessible, + publicExpiryDate: reportData.publicExpiryDate ?? undefined, + publicPassword: reportData.publicPassword ?? undefined, + userSuppliedPassword: undefined, }); if (!hasAccess) { diff --git a/apps/server/src/api/v2/metric_files/[id]/data/get-metric-data.ts b/apps/server/src/api/v2/metric_files/[id]/data/get-metric-data.ts index 07e919982..db0b7ac68 100644 --- a/apps/server/src/api/v2/metric_files/[id]/data/get-metric-data.ts +++ b/apps/server/src/api/v2/metric_files/[id]/data/get-metric-data.ts @@ -78,6 +78,10 @@ export async function getMetricDataHandler( requiredRole: 'can_view', organizationId, workspaceSharing: metric.workspaceSharing ?? 'none', + publiclyAccessible: metric.publiclyAccessible, + publicExpiryDate: metric.publicExpiryDate ?? undefined, + publicPassword: metric.publicPassword ?? undefined, + userSuppliedPassword: undefined, }); if (!hasAccess) { diff --git a/apps/server/src/api/v2/reports/[id]/PUT.ts b/apps/server/src/api/v2/reports/[id]/PUT.ts index 05ac0a2fc..73644ec26 100644 --- a/apps/server/src/api/v2/reports/[id]/PUT.ts +++ b/apps/server/src/api/v2/reports/[id]/PUT.ts @@ -22,7 +22,7 @@ async function updateReportHandler( assetId: reportId, assetType: 'report_file', workspaceSharing: getReportWorkspaceSharing, - requiredRole: ['full_access', 'owner', 'can_edit'], + requiredRole: 'can_edit', }); const { name, content, update_version = false } = request; diff --git a/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts b/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts index 2b19a121c..a5f0633b1 100644 --- a/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts +++ b/apps/server/src/api/v2/reports/[id]/sharing/DELETE.ts @@ -23,7 +23,7 @@ export async function deleteReportSharingHandler( assetId: reportId, assetType: 'report_file', workspaceSharing: getReportWorkspaceSharing, - requiredRole: ['full_access', 'owner'], + requiredRole: 'full_access', }); // Get the report to verify it exists and get owner info diff --git a/apps/server/src/api/v2/reports/[id]/sharing/POST.ts b/apps/server/src/api/v2/reports/[id]/sharing/POST.ts index 68c4b89bc..30f40aba9 100644 --- a/apps/server/src/api/v2/reports/[id]/sharing/POST.ts +++ b/apps/server/src/api/v2/reports/[id]/sharing/POST.ts @@ -23,7 +23,7 @@ export async function createReportSharingHandler( assetId: reportId, assetType: 'report_file', workspaceSharing: getReportWorkspaceSharing, - requiredRole: ['full_access', 'owner', 'can_edit'], + requiredRole: 'can_edit', }); // Get the report to verify it exists diff --git a/apps/server/src/shared-helpers/asset-public-access.ts b/apps/server/src/shared-helpers/asset-public-access.ts index ef6a48074..5833e10ed 100644 --- a/apps/server/src/shared-helpers/asset-public-access.ts +++ b/apps/server/src/shared-helpers/asset-public-access.ts @@ -42,6 +42,10 @@ export const checkAssetPublicAccess = async ({ requiredRole, organizationId, workspaceSharing, + publiclyAccessible: asset.publicly_accessible ?? false, + publicExpiryDate: asset.public_expiry_date ?? undefined, + publicPassword: asset.public_password ?? undefined, + userSuppliedPassword: password, }); if (!assetPermissionResult.hasAccess) { @@ -80,7 +84,7 @@ export const checkIfAssetIsEditable = async ({ assetType: AssetType; organizationId?: string; workspaceSharing: WorkspaceSharing | ((id: string) => Promise); - requiredRole?: AssetPermissionRole | AssetPermissionRole[]; + requiredRole?: AssetPermissionRole; }) => { const workspaceSharingResult = typeof workspaceSharing === 'function' ? await workspaceSharing(assetId) : workspaceSharing; diff --git a/apps/server/src/shared-helpers/metric-helpers.ts b/apps/server/src/shared-helpers/metric-helpers.ts index 190a01a90..77f040605 100644 --- a/apps/server/src/shared-helpers/metric-helpers.ts +++ b/apps/server/src/shared-helpers/metric-helpers.ts @@ -76,46 +76,18 @@ export async function fetchAndProcessMetricData( requiredRole: 'can_view', organizationId: metricFile.organizationId, workspaceSharing: metricFile.workspaceSharing || 'none', + publiclyAccessible: metricFile.publiclyAccessible ?? false, + publicExpiryDate: metricFile.publicExpiryDate ?? undefined, + publicPassword: metricFile.publicPassword ?? undefined, + userSuppliedPassword: password, }); effectiveRole = permissionResult.effectiveRole ? permissionResult.effectiveRole : effectiveRole; - // Check public access if needed - if (!effectiveRole) { - if (!metricFile.publiclyAccessible) { - console.warn(`Permission denied for user ${user.id} to metric ${metricId}`); - throw new HTTPException(403, { - message: "You don't have permission to view this metric", - }); - } - - // Check if public access has expired - const today = new Date(); - if (metricFile.publicExpiryDate && new Date(metricFile.publicExpiryDate) < today) { - console.warn(`Public access expired for metric ${metricId}`); - throw new HTTPException(403, { - message: 'Public access to this metric has expired', - }); - } - - // Check password if required - if (metricFile.publicPassword) { - if (!password) { - console.warn(`Public password required for metric ${metricId}`); - throw new HTTPException(418, { - message: 'Password required for public access', - }); - } - - if (password !== metricFile.publicPassword) { - console.warn(`Incorrect public password for metric ${metricId}`); - throw new HTTPException(403, { - message: 'Incorrect password for public access', - }); - } - } - - effectiveRole = 'can_view'; + if (!permissionResult.hasAccess || !effectiveRole) { + throw new HTTPException(403, { + message: "You don't have permission to view this metric", + }); } // Parse version history diff --git a/apps/web/src/routes/info/getting-started.tsx b/apps/web/src/routes/info/getting-started.tsx index c6b10bf43..1b727271c 100644 --- a/apps/web/src/routes/info/getting-started.tsx +++ b/apps/web/src/routes/info/getting-started.tsx @@ -10,4 +10,4 @@ export default function GettingStartedPage() { window.location.replace('https://buster.so/sign-up'); }, []); return null; -} \ No newline at end of file +} diff --git a/packages/access-controls/src/assets/cascading-permissions.ts b/packages/access-controls/src/assets/cascading-permissions.ts index a33dfa29d..52ba973a9 100644 --- a/packages/access-controls/src/assets/cascading-permissions.ts +++ b/packages/access-controls/src/assets/cascading-permissions.ts @@ -15,7 +15,11 @@ import { hasAssetPermission } from './permissions'; * Check if a user has access to a metric through any dashboard that contains it. * If a user has access to a dashboard (direct, public, or workspace), they can view the metrics in it. */ -export async function checkMetricDashboardAccess(metricId: string, user: User): Promise { +export async function checkMetricDashboardAccess( + metricId: string, + user: User, + userSuppliedPassword?: string +): Promise { try { // Get all dashboards containing this metric with their workspace sharing info const dashboards = await checkDashboardsContainingMetric(metricId); @@ -33,6 +37,10 @@ export async function checkMetricDashboardAccess(metricId: string, user: User): requiredRole: 'can_view' as AssetPermissionRole, organizationId: dashboard.organizationId, workspaceSharing: (dashboard.workspaceSharing as WorkspaceSharing) ?? 'none', + publiclyAccessible: dashboard.publiclyAccessible, + publicExpiryDate: dashboard.publicExpiryDate ?? undefined, + publicPassword: dashboard.publicPassword ?? undefined, + userSuppliedPassword: userSuppliedPassword, }); if (hasAccess) { @@ -72,6 +80,10 @@ export async function checkMetricChatAccess(metricId: string, user: User): Promi requiredRole: 'can_view' as AssetPermissionRole, organizationId: chat.organizationId, workspaceSharing: (chat.workspaceSharing as WorkspaceSharing) ?? 'none', + publiclyAccessible: chat.publiclyAccessible, + publicExpiryDate: chat.publicExpiryDate ?? undefined, + publicPassword: undefined, // We don't support passwords on the chats table + userSuppliedPassword: undefined, // We don't support passwords on the chats table }); if (hasAccess) { @@ -93,7 +105,11 @@ 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 { +export async function checkMetricReportAccess( + metricId: string, + user: User, + userSuppliedPassword?: string +): Promise { try { // Get all reports containing this metric with their workspace sharing info const reports = await checkReportsContainingMetric(metricId); @@ -111,6 +127,10 @@ export async function checkMetricReportAccess(metricId: string, user: User): Pro requiredRole: 'can_view' as AssetPermissionRole, organizationId: report.organizationId, workspaceSharing: (report.workspaceSharing as WorkspaceSharing) ?? 'none', + publiclyAccessible: report.publiclyAccessible, + publicExpiryDate: report.publicExpiryDate ?? undefined, + publicPassword: report.publicPassword ?? undefined, + userSuppliedPassword: userSuppliedPassword, }); if (hasAccess) { @@ -150,6 +170,10 @@ export async function checkDashboardChatAccess(dashboardId: string, user: User): requiredRole: 'can_view' as AssetPermissionRole, organizationId: chat.organizationId, workspaceSharing: (chat.workspaceSharing as WorkspaceSharing) ?? 'none', + publiclyAccessible: chat.publiclyAccessible, + publicExpiryDate: chat.publicExpiryDate ?? undefined, + publicPassword: undefined, // We don't support passwords on the chats table + userSuppliedPassword: undefined, // We don't support passwords on the chats table }); if (hasAccess) { diff --git a/packages/access-controls/src/assets/checks.test.ts b/packages/access-controls/src/assets/checks.test.ts index abe407f8f..1baad34bd 100644 --- a/packages/access-controls/src/assets/checks.test.ts +++ b/packages/access-controls/src/assets/checks.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { checkPermission, computeEffectivePermission, hasAnyAccess } from './checks'; +import { checkPermission, computeEffectivePermission } from './checks'; import type { AssetPermissionResult } from './checks'; // Mock database queries @@ -259,33 +259,4 @@ describe('Asset Permission Checks', () => { expect(result).toBe(null); }); }); - - describe('hasAnyAccess', () => { - it('should check for minimum can_view permission', async () => { - mockGetUserOrganizationsByUserId.mockResolvedValue([]); - mockCheckAssetPermission.mockResolvedValue({ - hasAccess: true, - role: 'can_view', - accessPath: 'direct', - }); - - const result = await hasAnyAccess('user123', 'asset123', 'dashboard_file'); - - expect(result).toBe(true); - expect(mockCheckAssetPermission).toHaveBeenCalled(); - }); - - it('should return false if no access', async () => { - mockGetCachedPermission.mockReturnValue(undefined); - mockGetUserOrganizationsByUserId.mockResolvedValue([]); - mockCheckAssetPermission.mockResolvedValue({ - hasAccess: false, - }); - mockCheckCascadingPermissions.mockResolvedValue(false); - - const result = await hasAnyAccess('user123', 'asset123', 'dashboard_file'); - - expect(result).toBe(false); - }); - }); }); diff --git a/packages/access-controls/src/assets/checks.ts b/packages/access-controls/src/assets/checks.ts index 92fbef99e..a936465ac 100644 --- a/packages/access-controls/src/assets/checks.ts +++ b/packages/access-controls/src/assets/checks.ts @@ -6,30 +6,46 @@ import { import type { User } from '@buster/database/queries'; import type { AssetType } from '@buster/database/schema-types'; import type { AssetPermissionRole, OrganizationMembership, WorkspaceSharing } from '../types'; -import { getHighestPermission, isPermissionSufficientForAny } from '../types/asset-permissions'; +import { getHighestPermission, isPermissionSufficient } from '../types/asset-permissions'; import { getCachedPermission, setCachedPermission } from './cache'; import { checkCascadingPermissions } from './cascading-permissions'; +import { hasPublicAccess } from './public-access-check'; export interface AssetPermissionCheck { userId: string; assetId: string; assetType: AssetType; - requiredRole: AssetPermissionRole | AssetPermissionRole[]; + requiredRole: AssetPermissionRole; organizationId?: string; workspaceSharing?: WorkspaceSharing; + publiclyAccessible?: boolean; + publicExpiryDate?: string | undefined; + publicPassword?: string | undefined; + userSuppliedPassword?: string | undefined; } export interface AssetPermissionResult { hasAccess: boolean; effectiveRole?: AssetPermissionRole; - accessPath?: 'direct' | 'workspace_sharing' | 'cascading' | 'admin'; + accessPath?: 'direct' | 'workspace_sharing' | 'cascading' | 'admin' | 'public'; } /** * Check if a user has sufficient permission to perform an action on an asset */ export async function checkPermission(check: AssetPermissionCheck): Promise { - const { userId, assetId, assetType, requiredRole, organizationId, workspaceSharing } = check; + const { + userId, + assetId, + assetType, + requiredRole, + organizationId, + workspaceSharing, + publiclyAccessible, + publicExpiryDate, + publicPassword, + userSuppliedPassword, + } = check; // Check cache first (only for single role checks) if (!Array.isArray(requiredRole)) { @@ -59,7 +75,7 @@ export async function checkPermission(check: AssetPermissionCheck): Promise = { id: userId }; const hasCascadingAccess = await checkCascadingPermissions(assetId, assetType, user as User); @@ -108,10 +144,7 @@ export async function checkPermission(check: AssetPermissionCheck): Promise { - const result = await checkPermission({ - userId, - assetId, - assetType, - requiredRole: 'can_view', // Minimum permission level - }); - - return result.hasAccess; -} diff --git a/packages/access-controls/src/assets/permissions.ts b/packages/access-controls/src/assets/permissions.ts index 86e6417f6..ff8389d75 100644 --- a/packages/access-controls/src/assets/permissions.ts +++ b/packages/access-controls/src/assets/permissions.ts @@ -207,6 +207,10 @@ export async function hasAssetPermission(params: { requiredRole: AssetPermissionRole; organizationId?: string; workspaceSharing?: WorkspaceSharing; + publiclyAccessible?: boolean; + publicExpiryDate?: string | undefined; + publicPassword?: string | undefined; + userSuppliedPassword?: string | undefined; }): Promise { const { checkPermission } = await import('./checks'); diff --git a/packages/access-controls/src/assets/public-access-check.ts b/packages/access-controls/src/assets/public-access-check.ts new file mode 100644 index 000000000..104f03034 --- /dev/null +++ b/packages/access-controls/src/assets/public-access-check.ts @@ -0,0 +1,18 @@ +export function hasPublicAccess( + publiclyAccessible: boolean, + publicExpiryDate?: string, + publicPassword?: string, + password?: string +): boolean { + if (!publiclyAccessible) { + return false; + } + const today = new Date(); + if (publicExpiryDate && new Date(publicExpiryDate) < today) { + return false; + } + if (publicPassword && publicPassword !== password) { + return false; + } + return true; +} 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 ca30ceced..6d3c64a59 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 @@ -7,6 +7,8 @@ export interface ChatWithSharing { id: string; organizationId: string; workspaceSharing: WorkspaceSharing | null; + publiclyAccessible: boolean; + publicExpiryDate: string | null; } export async function checkChatsContainingAsset( @@ -18,6 +20,8 @@ export async function checkChatsContainingAsset( id: chats.id, organizationId: chats.organizationId, workspaceSharing: chats.workspaceSharing, + publiclyAccessible: chats.publiclyAccessible, + publicExpiryDate: chats.publicExpiryDate, }) .from(messagesToFiles) .innerJoin(messages, eq(messages.id, messagesToFiles.messageId)) 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 1a3d292fc..15b8f7e53 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 @@ -7,6 +7,9 @@ export interface DashboardWithSharing { id: string; organizationId: string; workspaceSharing: WorkspaceSharing | null; + publiclyAccessible: boolean; + publicExpiryDate: string | null; + publicPassword: string | null; } export async function checkDashboardsContainingMetric( @@ -17,6 +20,9 @@ export async function checkDashboardsContainingMetric( id: dashboardFiles.id, organizationId: dashboardFiles.organizationId, workspaceSharing: dashboardFiles.workspaceSharing, + publiclyAccessible: dashboardFiles.publiclyAccessible, + publicExpiryDate: dashboardFiles.publicExpiryDate, + publicPassword: dashboardFiles.publicPassword, }) .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 index e624d6e4b..e45c02bef 100644 --- a/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.ts +++ b/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.ts @@ -7,6 +7,9 @@ export interface ReportWithSharing { id: string; organizationId: string; workspaceSharing: WorkspaceSharing | null; + publiclyAccessible: boolean; + publicExpiryDate: string | null; + publicPassword: string | null; } export async function checkReportsContainingMetric(metricId: string): Promise { @@ -15,6 +18,9 @@ export async function checkReportsContainingMetric(metricId: string): Promise Date: Wed, 24 Sep 2025 13:42:45 -0600 Subject: [PATCH 2/2] tests --- .../src/api/v2/dashboards/[id]/GET.test.ts | 4 + .../asset-public-access.test.ts | 16 +++ .../src/shared-helpers/metric-helpers.test.ts | 20 ++-- .../src/assets/cascading-permissions.ts | 11 ++- packages/access-controls/src/assets/checks.ts | 7 +- ...check-dashboards-containing-metric.test.ts | 88 +++++++++++++++-- .../check-reports-containing-metric.test.ts | 99 +++++++++++++++++-- 7 files changed, 213 insertions(+), 32 deletions(-) diff --git a/apps/server/src/api/v2/dashboards/[id]/GET.test.ts b/apps/server/src/api/v2/dashboards/[id]/GET.test.ts index a0e20c0ba..83260100d 100644 --- a/apps/server/src/api/v2/dashboards/[id]/GET.test.ts +++ b/apps/server/src/api/v2/dashboards/[id]/GET.test.ts @@ -175,6 +175,10 @@ describe('getDashboardHandler', () => { requiredRole: 'can_view', organizationId: mockDashboard.organizationId, workspaceSharing: 'none', + publiclyAccessible: false, + publicExpiryDate: undefined, + publicPassword: undefined, + userSuppliedPassword: undefined, }); }); diff --git a/apps/server/src/shared-helpers/asset-public-access.test.ts b/apps/server/src/shared-helpers/asset-public-access.test.ts index c5a89d2e3..19fe29898 100644 --- a/apps/server/src/shared-helpers/asset-public-access.test.ts +++ b/apps/server/src/shared-helpers/asset-public-access.test.ts @@ -67,6 +67,10 @@ describe('checkAssetPublicAccess', () => { requiredRole: 'can_view', organizationId: mockOrganizationId, workspaceSharing: mockWorkspaceSharing, + publiclyAccessible: false, + publicExpiryDate: undefined, + publicPassword: undefined, + userSuppliedPassword: undefined, }); }); @@ -88,6 +92,10 @@ describe('checkAssetPublicAccess', () => { requiredRole: 'can_edit', organizationId: mockOrganizationId, workspaceSharing: mockWorkspaceSharing, + publiclyAccessible: false, + publicExpiryDate: undefined, + publicPassword: undefined, + userSuppliedPassword: undefined, }); }); @@ -378,6 +386,10 @@ describe('checkAssetPublicAccess', () => { requiredRole: 'can_view', organizationId: mockOrganizationId, workspaceSharing: mockWorkspaceSharing, + publiclyAccessible: true, + publicExpiryDate: undefined, + publicPassword: undefined, + userSuppliedPassword: undefined, }); }); }); @@ -472,6 +484,10 @@ describe('checkAssetPublicAccess', () => { requiredRole: 'can_edit', organizationId: 'org-123', workspaceSharing: 'can_view', + publiclyAccessible: false, + publicExpiryDate: undefined, + publicPassword: undefined, + userSuppliedPassword: undefined, }); }); }); diff --git a/apps/server/src/shared-helpers/metric-helpers.test.ts b/apps/server/src/shared-helpers/metric-helpers.test.ts index 9e42cac78..008ce0b55 100644 --- a/apps/server/src/shared-helpers/metric-helpers.test.ts +++ b/apps/server/src/shared-helpers/metric-helpers.test.ts @@ -286,8 +286,9 @@ describe('metric-helpers', () => { const metricFile = createMockMetricFile({ publiclyAccessible: true }); mockGetMetricFileById.mockResolvedValue(metricFile); mockCheckPermission.mockResolvedValue({ - hasAccess: false, - effectiveRole: undefined, + hasAccess: true, + effectiveRole: 'can_view', + accessPath: 'public', }); const options: MetricAccessOptions = { publicAccessPreviouslyVerified: false }; @@ -315,8 +316,8 @@ describe('metric-helpers', () => { const metricFile = createMockMetricFile({ publiclyAccessible: false }); mockGetMetricFileById.mockResolvedValue(metricFile); mockCheckPermission.mockResolvedValue({ - hasAccess: false, - effectiveRole: undefined, + hasAccess: true, + effectiveRole: 'can_view', }); const options: MetricAccessOptions = { publicAccessPreviouslyVerified: true }; @@ -344,7 +345,7 @@ describe('metric-helpers', () => { const options: MetricAccessOptions = { publicAccessPreviouslyVerified: false }; await expect(fetchAndProcessMetricData('metric-123', mockUser, options)).rejects.toThrow( - new HTTPException(403, { message: 'Public access to this metric has expired' }) + new HTTPException(403, { message: "You don't have permission to view this metric" }) ); }); @@ -362,7 +363,7 @@ describe('metric-helpers', () => { const options: MetricAccessOptions = { publicAccessPreviouslyVerified: false }; await expect(fetchAndProcessMetricData('metric-123', mockUser, options)).rejects.toThrow( - new HTTPException(418, { message: 'Password required for public access' }) + new HTTPException(403, { message: "You don't have permission to view this metric" }) ); }); @@ -383,7 +384,7 @@ describe('metric-helpers', () => { }; await expect(fetchAndProcessMetricData('metric-123', mockUser, options)).rejects.toThrow( - new HTTPException(403, { message: 'Incorrect password for public access' }) + new HTTPException(403, { message: "You don't have permission to view this metric" }) ); }); @@ -394,8 +395,9 @@ describe('metric-helpers', () => { }); mockGetMetricFileById.mockResolvedValue(metricFile); mockCheckPermission.mockResolvedValue({ - hasAccess: false, - effectiveRole: undefined, + hasAccess: true, + effectiveRole: 'can_view', + accessPath: 'public', }); const options: MetricAccessOptions = { diff --git a/packages/access-controls/src/assets/cascading-permissions.ts b/packages/access-controls/src/assets/cascading-permissions.ts index 52ba973a9..46efe9500 100644 --- a/packages/access-controls/src/assets/cascading-permissions.ts +++ b/packages/access-controls/src/assets/cascading-permissions.ts @@ -318,7 +318,8 @@ export async function checkChatCollectionAccess(chatId: string, user: User): Pro export async function checkCascadingPermissions( assetId: string, assetType: AssetType, - user: User + user: User, + userSuppliedPassword?: string ): Promise { // Check cache first const cached = getCachedCascadingPermission(user.id, assetId, assetType); @@ -332,7 +333,11 @@ export async function checkCascadingPermissions( switch (assetType) { case 'metric_file': { // Check access through dashboards, chats, collections, and reports - const dashboardAccess = await checkMetricDashboardAccess(assetId, user); + const dashboardAccess = await checkMetricDashboardAccess( + assetId, + user, + userSuppliedPassword + ); if (dashboardAccess) { hasAccess = true; break; @@ -350,7 +355,7 @@ export async function checkCascadingPermissions( break; } - const reportAccess = await checkMetricReportAccess(assetId, user); + const reportAccess = await checkMetricReportAccess(assetId, user, userSuppliedPassword); if (reportAccess) { hasAccess = true; break; diff --git a/packages/access-controls/src/assets/checks.ts b/packages/access-controls/src/assets/checks.ts index a936465ac..ff373dfd4 100644 --- a/packages/access-controls/src/assets/checks.ts +++ b/packages/access-controls/src/assets/checks.ts @@ -137,7 +137,12 @@ export async function checkPermission(check: AssetPermissionCheck): Promise = { id: userId }; - const hasCascadingAccess = await checkCascadingPermissions(assetId, assetType, user as User); + const hasCascadingAccess = await checkCascadingPermissions( + assetId, + assetType, + user as User, + userSuppliedPassword + ); if (hasCascadingAccess) { const result = { hasAccess: true, diff --git a/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.test.ts b/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.test.ts index a1c8c10dd..f45e17860 100644 --- a/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.test.ts +++ b/packages/database/src/queries/cascading-permissions/check-dashboards-containing-metric.test.ts @@ -47,9 +47,30 @@ describe('checkDashboardsContainingMetric', () => { it('should return dashboards with organizationId and workspaceSharing', async () => { const mockDashboards = [ - { id: 'dash1', organizationId: 'org1', workspaceSharing: 'can_view' }, - { id: 'dash2', organizationId: 'org2', workspaceSharing: 'none' }, - { id: 'dash3', organizationId: 'org1', workspaceSharing: null }, + { + id: 'dash1', + organizationId: 'org1', + workspaceSharing: 'can_view', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'dash2', + organizationId: 'org2', + workspaceSharing: 'none', + publiclyAccessible: true, + publicExpiryDate: '2024-12-31T23:59:59Z', + publicPassword: 'secret456', + }, + { + id: 'dash3', + organizationId: 'org1', + workspaceSharing: null, + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, ]; mockQueryChain.where.mockResolvedValue(mockDashboards); @@ -61,6 +82,9 @@ describe('checkDashboardsContainingMetric', () => { id: expect.anything(), organizationId: expect.anything(), workspaceSharing: expect.anything(), + publiclyAccessible: expect.anything(), + publicExpiryDate: expect.anything(), + publicPassword: expect.anything(), }); expect(mockQueryChain.from).toHaveBeenCalled(); expect(mockQueryChain.innerJoin).toHaveBeenCalled(); @@ -76,21 +100,67 @@ describe('checkDashboardsContainingMetric', () => { }); it('should handle null workspace sharing values', async () => { - const mockDashboards = [{ id: 'dash1', organizationId: 'org1', workspaceSharing: null }]; + const mockDashboards = [ + { + id: 'dash1', + organizationId: 'org1', + workspaceSharing: null, + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + ]; mockQueryChain.where.mockResolvedValue(mockDashboards); const result = await checkDashboardsContainingMetric('metric123'); - expect(result).toEqual([{ id: 'dash1', organizationId: 'org1', workspaceSharing: null }]); + expect(result).toEqual([ + { + id: 'dash1', + organizationId: 'org1', + workspaceSharing: null, + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + ]); }); it('should handle all workspace sharing levels', async () => { const mockDashboards = [ - { id: 'dash1', organizationId: 'org1', workspaceSharing: 'none' }, - { id: 'dash2', organizationId: 'org1', workspaceSharing: 'can_view' }, - { id: 'dash3', organizationId: 'org1', workspaceSharing: 'can_edit' }, - { id: 'dash4', organizationId: 'org1', workspaceSharing: 'full_access' }, + { + id: 'dash1', + organizationId: 'org1', + workspaceSharing: 'none', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'dash2', + organizationId: 'org1', + workspaceSharing: 'can_view', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'dash3', + organizationId: 'org1', + workspaceSharing: 'can_edit', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'dash4', + organizationId: 'org1', + workspaceSharing: 'full_access', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, ]; mockQueryChain.where.mockResolvedValue(mockDashboards); diff --git a/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.test.ts b/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.test.ts index 1b83022e4..e78c93be3 100644 --- a/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.test.ts +++ b/packages/database/src/queries/cascading-permissions/check-reports-containing-metric.test.ts @@ -47,9 +47,30 @@ describe('checkReportsContainingMetric', () => { it('should return reports with organizationId and workspaceSharing', async () => { const mockReports = [ - { id: 'report1', organizationId: 'org1', workspaceSharing: 'can_view' }, - { id: 'report2', organizationId: 'org2', workspaceSharing: 'full_access' }, - { id: 'report3', organizationId: 'org1', workspaceSharing: null }, + { + id: 'report1', + organizationId: 'org1', + workspaceSharing: 'can_view', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'report2', + organizationId: 'org2', + workspaceSharing: 'full_access', + publiclyAccessible: true, + publicExpiryDate: '2024-12-31T23:59:59Z', + publicPassword: 'secret123', + }, + { + id: 'report3', + organizationId: 'org1', + workspaceSharing: null, + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, ]; mockQueryChain.where.mockResolvedValue(mockReports); @@ -61,6 +82,9 @@ describe('checkReportsContainingMetric', () => { id: expect.anything(), organizationId: expect.anything(), workspaceSharing: expect.anything(), + publiclyAccessible: expect.anything(), + publicExpiryDate: expect.anything(), + publicPassword: expect.anything(), }); expect(mockQueryChain.from).toHaveBeenCalled(); expect(mockQueryChain.innerJoin).toHaveBeenCalled(); @@ -76,21 +100,67 @@ describe('checkReportsContainingMetric', () => { }); it('should handle null workspace sharing values', async () => { - const mockReports = [{ id: 'report1', organizationId: 'org1', workspaceSharing: null }]; + const mockReports = [ + { + id: 'report1', + organizationId: 'org1', + workspaceSharing: null, + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + ]; mockQueryChain.where.mockResolvedValue(mockReports); const result = await checkReportsContainingMetric('metric123'); - expect(result).toEqual([{ id: 'report1', organizationId: 'org1', workspaceSharing: null }]); + expect(result).toEqual([ + { + id: 'report1', + organizationId: 'org1', + workspaceSharing: null, + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + ]); }); it('should handle all workspace sharing levels', async () => { const mockReports = [ - { id: 'report1', organizationId: 'org1', workspaceSharing: 'none' }, - { id: 'report2', organizationId: 'org1', workspaceSharing: 'can_view' }, - { id: 'report3', organizationId: 'org1', workspaceSharing: 'can_edit' }, - { id: 'report4', organizationId: 'org1', workspaceSharing: 'full_access' }, + { + id: 'report1', + organizationId: 'org1', + workspaceSharing: 'none', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'report2', + organizationId: 'org1', + workspaceSharing: 'can_view', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'report3', + organizationId: 'org1', + workspaceSharing: 'can_edit', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + { + id: 'report4', + organizationId: 'org1', + workspaceSharing: 'full_access', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, ]; mockQueryChain.where.mockResolvedValue(mockReports); @@ -103,7 +173,16 @@ describe('checkReportsContainingMetric', () => { it('should filter out deleted reports and deleted relationships', async () => { // This test validates that the query conditions are set up correctly // In a real implementation, deleted items would be filtered by the database - const activeReports = [{ id: 'report1', organizationId: 'org1', workspaceSharing: 'can_view' }]; + const activeReports = [ + { + id: 'report1', + organizationId: 'org1', + workspaceSharing: 'can_view', + publiclyAccessible: false, + publicExpiryDate: null, + publicPassword: null, + }, + ]; mockQueryChain.where.mockResolvedValue(activeReports);