From 9927808704757c25857d56904b0ca44da21143ab Mon Sep 17 00:00:00 2001 From: Wells Bunker Date: Wed, 24 Sep 2025 13:17:08 -0600 Subject: [PATCH] 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