From 8101f2026d2529d8fc82b1d8637d389ae7984e70 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Mon, 9 Jun 2025 09:31:01 -0700 Subject: [PATCH] Admins should be able to share all tasks (#76) * Admins should be able to share all tasks * PR feedback --- src/actions/__tests__/taskSharing.test.ts | 125 ++++++++++++++ src/actions/analytics/events.ts | 20 ++- src/actions/auth.ts | 6 +- src/actions/taskSharing.ts | 169 ++++++++++++++----- src/app/(authenticated)/usage/TaskDrawer.tsx | 14 +- src/types/react-query.ts | 1 + 6 files changed, 290 insertions(+), 45 deletions(-) create mode 100644 src/actions/__tests__/taskSharing.test.ts diff --git a/src/actions/__tests__/taskSharing.test.ts b/src/actions/__tests__/taskSharing.test.ts new file mode 100644 index 0000000000..0e7e5dc1e0 --- /dev/null +++ b/src/actions/__tests__/taskSharing.test.ts @@ -0,0 +1,125 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { TaskWithUser } from '../analytics'; + +// Create a mock function for the canShareTask function +const mockCanShareTask = vi.fn(); + +// Mock the taskSharing module +vi.mock('../taskSharing', async () => { + const actual = await vi.importActual('../taskSharing'); + return { + ...actual, + canShareTask: mockCanShareTask, + }; +}); + +// Mock the analytics module +vi.mock('../analytics', () => ({ + getTasks: vi.fn(), +})); + +describe('Task Sharing Permissions', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should allow admin to share any task', async () => { + const mockTask: TaskWithUser = { + taskId: 'task-123', + userId: 'other-user', + user: { + id: 'other-user', + name: 'Other User', + email: 'other@example.com', + imageUrl: 'https://example.com/avatar.jpg', + entity: {}, + lastSyncAt: new Date(), + createdAt: new Date(), + updatedAt: new Date(), + orgId: 'org-123', + orgRole: 'org:member', + }, + title: 'Test Task', + provider: 'openai', + model: 'gpt-4', + mode: 'code', + completed: false, + tokens: 1000, + cost: 0.02, + timestamp: 1234567890, + }; + + mockCanShareTask.mockResolvedValue({ + canShare: true, + task: mockTask, + }); + + const result = await mockCanShareTask('task-123'); + + expect(result.canShare).toBe(true); + expect(result.task).toEqual(mockTask); + }); + + it('should allow member to share their own task', async () => { + const mockTask: TaskWithUser = { + taskId: 'task-123', + userId: 'member-user', + user: { + id: 'member-user', + name: 'Member User', + email: 'member@example.com', + imageUrl: 'https://example.com/avatar.jpg', + entity: {}, + lastSyncAt: new Date(), + createdAt: new Date(), + updatedAt: new Date(), + orgId: 'org-123', + orgRole: 'org:member', + }, + title: 'Test Task', + provider: 'openai', + model: 'gpt-4', + mode: 'code', + completed: false, + tokens: 1000, + cost: 0.02, + timestamp: 1234567890, + }; + + mockCanShareTask.mockResolvedValue({ + canShare: true, + task: mockTask, + }); + + const result = await mockCanShareTask('task-123'); + + expect(result.canShare).toBe(true); + expect(result.task).toEqual(mockTask); + }); + + it('should not allow member to share other users task', async () => { + mockCanShareTask.mockResolvedValue({ + canShare: false, + error: 'Task not found or you do not have permission to share this task', + }); + + const result = await mockCanShareTask('task-123'); + + expect(result.canShare).toBe(false); + expect(result.error).toBe( + 'Task not found or you do not have permission to share this task', + ); + }); + + it('should handle task not found for admin', async () => { + mockCanShareTask.mockResolvedValue({ + canShare: false, + error: 'Task not found', + }); + + const result = await mockCanShareTask('nonexistent-task'); + + expect(result.canShare).toBe(false); + expect(result.error).toBe('Task not found'); + }); +}); diff --git a/src/actions/analytics/events.ts b/src/actions/analytics/events.ts index 336ab0ab2d..b4ef423507 100644 --- a/src/actions/analytics/events.ts +++ b/src/actions/analytics/events.ts @@ -22,10 +22,12 @@ async function validateAnalyticsAccess({ requestedOrgId, requestedUserId, requireAdmin = false, + allowCrossUserAccess = false, }: { requestedOrgId?: string | null; requestedUserId?: string | null; requireAdmin?: boolean; + allowCrossUserAccess?: boolean; }): Promise<{ authOrgId: string; authUserId: string; @@ -54,8 +56,11 @@ async function validateAnalyticsAccess({ } // For non-admin users, force userId filter to their own ID + // Unless allowCrossUserAccess is true and we're checking task sharing permissions const effectiveUserId = - orgRole !== 'org:admin' ? authUserId : requestedUserId || null; + orgRole !== 'org:admin' && !allowCrossUserAccess + ? authUserId + : requestedUserId || null; return { authOrgId, @@ -347,13 +352,18 @@ export type TaskWithUser = TaskWithTitle & { user: User }; export const getTasks = async ({ orgId, userId, + taskId, + allowCrossUserAccess = false, }: { orgId?: string | null; userId?: string | null; + taskId?: string | null; + allowCrossUserAccess?: boolean; }): Promise => { const { effectiveUserId } = await validateAnalyticsAccess({ requestedOrgId: orgId, requestedUserId: userId, + allowCrossUserAccess, }); if (!orgId) { @@ -361,9 +371,12 @@ export const getTasks = async ({ } const userFilter = effectiveUserId ? 'AND e.userId = {userId: String}' : ''; + const taskFilter = taskId ? 'AND e.taskId = {taskId: String}' : ''; const messageUserFilter = effectiveUserId ? 'AND userId = {userId: String}' : ''; + const messageTaskFilter = taskId ? 'AND taskId = {taskId: String}' : ''; + const queryParams: Record = { orgId: orgId!, types: [ @@ -375,6 +388,9 @@ export const getTasks = async ({ if (effectiveUserId) { queryParams.userId = effectiveUserId; } + if (taskId) { + queryParams.taskId = taskId; + } const results = await analytics.query({ query: ` @@ -386,6 +402,7 @@ export const getTasks = async ({ FROM messages WHERE orgId = {orgId: String} ${messageUserFilter} + ${messageTaskFilter} GROUP BY taskId ) SELECT @@ -405,6 +422,7 @@ export const getTasks = async ({ e.orgId = {orgId: String} AND e.type IN ({types: Array(String)}) ${userFilter} + ${taskFilter} GROUP BY 1, 2 ORDER BY timestamp DESC `, diff --git a/src/actions/auth.ts b/src/actions/auth.ts index 096a58d980..5efeb15676 100644 --- a/src/actions/auth.ts +++ b/src/actions/auth.ts @@ -6,9 +6,9 @@ import type { ApiResponse } from '@/types'; import { Env, logger } from '@/lib/server'; export async function validateAuth(): Promise< - { userId: string; orgId: string } | ApiResponse + { userId: string; orgId: string; orgRole: string } | ApiResponse > { - const { userId, orgId } = await auth(); + const { userId, orgId, orgRole } = await auth(); if (!userId) { return { success: false, error: 'Unauthorized: User required' }; @@ -18,7 +18,7 @@ export async function validateAuth(): Promise< return { success: false, error: 'Unauthorized: Organization required' }; } - return { userId, orgId }; + return { userId, orgId, orgRole: orgRole || 'unknown' }; } // Default expiration is 30 days (2592000 seconds). diff --git a/src/actions/taskSharing.ts b/src/actions/taskSharing.ts index 208577ed6c..968770e093 100644 --- a/src/actions/taskSharing.ts +++ b/src/actions/taskSharing.ts @@ -34,18 +34,72 @@ type TaskShareResponse = ApiResponse & { }; }; +/** + * Check if the current user can share a specific task (for UI components) + */ +export async function canShareTask(taskId: string): Promise<{ + canShare: boolean; + task?: TaskWithUser; + error?: string; + userId?: string; + orgId?: string; + orgRole?: string; +}> { + try { + // Get authentication info + const { userId, orgId, orgRole } = await auth(); + + if (!userId || !orgId) { + return { canShare: false, error: 'Authentication required' }; + } + + // Admins can share any task in the organization + if (orgRole === 'org:admin') { + const tasks = await getTasks({ + taskId, + orgId, + allowCrossUserAccess: true, + }); + const task = tasks[0]; + + if (!task) { + return { canShare: false, error: 'Task not found' }; + } + + return { canShare: true, task, userId, orgId, orgRole }; + } + + // Members can only share tasks they created + const tasks = await getTasks({ taskId, orgId }); + const task = tasks[0]; + + // Additional check: ensure the task belongs to the requesting user + if (task && task.userId !== userId) { + return { + canShare: false, + error: + 'Task not found or you do not have permission to share this task', + }; + } + + if (!task) { + return { + canShare: false, + error: + 'Task not found or you do not have permission to share this task', + }; + } + + return { canShare: true, task, userId, orgId, orgRole }; + } catch (_error) { + return { canShare: false, error: 'Failed to verify task access' }; + } +} + export async function createTaskShare( data: CreateTaskShareRequest, ): Promise { try { - const authResult = await validateAuth(); - - if (!isAuthSuccess(authResult)) { - return authResult; - } - - const { userId, orgId } = authResult; - const result = createTaskShareSchema.safeParse(data); if (!result.success) { @@ -63,11 +117,19 @@ export async function createTaskShare( }; } - const tasks = await getTasks({ orgId, userId }); - const task = tasks.find((t) => t.taskId === taskId); + // Check if user can share this specific task (includes auth) + const { canShare, task, error, userId, orgId, orgRole } = + await canShareTask(taskId); - if (!task) { - return { success: false, error: 'Task not found or access denied' }; + if (!canShare) { + return { success: false, error: error || 'Access denied' }; + } + + if (!task || !userId || !orgId || !orgRole) { + return { + success: false, + error: 'Task not found or authentication failed', + }; } const expirationDaysToUse = @@ -103,8 +165,14 @@ export async function createTaskShare( action: 'created', shareId: insertedShare[0].id, expiresAt: expiresAt.toISOString(), + taskOwnerId: task.userId, + sharedByAdmin: orgRole === 'org:admin' && task.userId !== userId, }, - description: `Created task share for task ${taskId}`, + description: `Created task share for task ${taskId}${ + orgRole === 'org:admin' && task.userId !== userId + ? ` (admin sharing task created by ${task.user.name})` + : '' + }`, }); return insertedShare; @@ -159,8 +227,12 @@ export async function getTaskByShareToken( return null; } - const tasks = await getTasks({ orgId: share.orgId }); - const task = tasks.find((t) => t.taskId === share.taskId); + const tasks = await getTasks({ + taskId: share.taskId, + orgId: share.orgId, + allowCrossUserAccess: true, + }); + const task = tasks[0]; if (!task) { return null; @@ -190,7 +262,7 @@ export async function deleteTaskShare(shareId: string): Promise { return authResult; } - const { userId, orgId } = authResult; + const { userId, orgId, orgRole } = authResult; const shareIdResult = shareIdSchema.safeParse(shareId); @@ -198,20 +270,24 @@ export async function deleteTaskShare(shareId: string): Promise { return { success: false, error: 'Invalid share ID format' }; } + // First, find the share to check permissions const [share] = await db .select() .from(taskShares) - .where( - and( - eq(taskShares.id, shareId), - eq(taskShares.orgId, orgId), - eq(taskShares.createdByUserId, userId), - ), - ) + .where(and(eq(taskShares.id, shareId), eq(taskShares.orgId, orgId))) .limit(1); if (!share) { - return { success: false, error: 'Share not found or access denied' }; + return { success: false, error: 'Share not found' }; + } + + // Check if user can delete this share + // Admins can delete any share, members can only delete shares they created + if (orgRole !== 'org:admin' && share.createdByUserId !== userId) { + return { + success: false, + error: 'Access denied: You can only delete shares you created', + }; } await db.transaction(async (tx) => { @@ -222,8 +298,17 @@ export async function deleteTaskShare(shareId: string): Promise { orgId, targetType: AuditLogTargetType.TASK_SHARE, targetId: share.taskId, - newValue: { action: 'deleted', shareId: share.id }, - description: `Deleted task share for task ${share.taskId}`, + newValue: { + action: 'deleted', + shareId: share.id, + deletedByAdmin: + orgRole === 'org:admin' && share.createdByUserId !== userId, + }, + description: `Deleted task share for task ${share.taskId}${ + orgRole === 'org:admin' && share.createdByUserId !== userId + ? ' (admin deletion)' + : '' + }`, }); }); @@ -238,29 +323,33 @@ export async function deleteTaskShare(shareId: string): Promise { */ export async function getTaskShares(taskId: string): Promise { try { - const { userId, orgId } = await auth(); + // Check if user can access this task (includes auth) + const { canShare, error, userId, orgId, orgRole } = + await canShareTask(taskId); - if (!userId || !orgId) { - throw new Error('Authentication required'); + if (!canShare) { + throw new Error(error || 'Task not found or access denied'); } - const tasks = await getTasks({ orgId, userId }); - const task = tasks.find((t) => t.taskId === taskId); + if (!userId || !orgId) { + throw new Error('Authentication failed'); + } - if (!task) { - throw new Error('Task not found or access denied'); + // For admins, show all shares for the task + // For members, only show shares they created + const whereConditions = [ + eq(taskShares.taskId, taskId), + eq(taskShares.orgId, orgId), + ]; + + if (orgRole !== 'org:admin') { + whereConditions.push(eq(taskShares.createdByUserId, userId)); } const shares = await db .select() .from(taskShares) - .where( - and( - eq(taskShares.taskId, taskId), - eq(taskShares.orgId, orgId), - eq(taskShares.createdByUserId, userId), - ), - ) + .where(and(...whereConditions)) .orderBy(desc(taskShares.createdAt)); return shares.filter((share) => !isShareExpired(share.expiresAt)); diff --git a/src/app/(authenticated)/usage/TaskDrawer.tsx b/src/app/(authenticated)/usage/TaskDrawer.tsx index 926e2f77c9..f4211f0460 100644 --- a/src/app/(authenticated)/usage/TaskDrawer.tsx +++ b/src/app/(authenticated)/usage/TaskDrawer.tsx @@ -3,9 +3,11 @@ import { X } from 'lucide-react'; import type { TaskWithUser } from '@/actions/analytics'; import { getMessages } from '@/actions/analytics'; +import { canShareTask } from '@/actions/taskSharing'; import { useOrganizationSettings } from '@/hooks/useOrganizationSettings'; import { formatCurrency, formatNumber } from '@/lib/formatters'; import { generateFallbackTitle } from '@/lib/task-utils'; +import { QueryKey } from '@/types/react-query'; import { Drawer, DrawerContent, Button } from '@/components/ui'; import { ShareButton } from '@/components/task-sharing/ShareButton'; @@ -25,16 +27,26 @@ export const TaskDrawer = ({ task, onClose }: TaskDrawerProps) => { const { data: orgSettings } = useOrganizationSettings(); + const { data: sharePermission } = useQuery({ + queryKey: [QueryKey.CanShareTask, task.taskId], + queryFn: () => canShareTask(task.taskId), + enabled: !!task.taskId, + }); + const isTaskSharingEnabled = orgSettings?.cloudSettings?.enableTaskSharing ?? false; + const canUserShareThisTask = sharePermission?.canShare ?? false; + return (
- {isTaskSharingEnabled && } + {isTaskSharingEnabled && canUserShareThisTask && ( + + )} diff --git a/src/types/react-query.ts b/src/types/react-query.ts index 8f9d9c4d0f..3aecef634b 100644 --- a/src/types/react-query.ts +++ b/src/types/react-query.ts @@ -1,4 +1,5 @@ export enum QueryKey { GetOrganizationSettings = 'GetOrganizationSettings', GetDynamicRouterModels = 'GetDynamicRouterModels', + CanShareTask = 'canShareTask', }