diff --git a/backend/__tests__/services/projectOverviewEmailCanAct.test.js b/backend/__tests__/services/projectOverviewEmailCanAct.test.js new file mode 100644 index 00000000..7ba08825 --- /dev/null +++ b/backend/__tests__/services/projectOverviewEmailCanAct.test.js @@ -0,0 +1,149 @@ +/** + * getProjectOverview stamps each email with `canAct` — whether the queued-mail + * routes (requireOwnedQueuedEmail) would actually accept an action on it. + * + * The cockpit used to derive this client-side from `event_id != null`, which is + * weaker than the backend rule in a way that still produced dead controls: + * requireOwnedQueuedEmail ALSO requires ownership of that event, while + * getProjectOverview lists the project's events by project_id alone. Project + * ownership does not imply event ownership — ownedProjectsSubquery's + * `projects.created_by = admin.id` branch places no constraint on the linked + * events' owners, so a super_admin can attach admin B's event to admin A's + * project. See #969 / codex review round 1. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-canact-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'canact-test-secret'; + +const bcrypt = require('bcrypt'); +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('getProjectOverview email canAct (#969)', () => { + let db; let cleanup; let projectService; + let adminA; let adminB; let superAdmin; + let projectId; let ownEventId; let foreignEventId; let ownerlessEventId; + + const mkAdmin = async (username, roleName) => { + const role = await db('roles').where({ name: roleName }).first(); + const r = await db('admin_users').insert({ + username, email: `${username}@example.com`, + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, is_active: 1, + created_at: new Date().toISOString(), updated_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + const mkEvent = async (slug, createdBy, project) => { + const r = await db('events').insert({ + slug, event_type: 'wedding', event_name: slug, event_date: '2026-08-01', + host_email: 'h@e.com', admin_email: 'a@e.com', password_hash: 'x', + share_token: `t-${slug}`, share_link: `/g/${slug}/t-${slug}`, + created_by: createdBy, project_id: project, + expires_at: new Date(Date.now() + 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + const mkMail = async (eventId, type) => { + const r = await db('email_queue').insert({ + recipient_email: 'kunde@example.com', email_type: type, status: 'sent', + event_id: eventId, + created_at: new Date().toISOString(), sent_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + projectService = require('../../src/services/projectService'); + + adminA = await mkAdmin('canact-a', 'editor'); + adminB = await mkAdmin('canact-b', 'editor'); + superAdmin = await mkAdmin('canact-root', 'super_admin'); + + const p = await db('projects').insert({ + name: 'Cockpit canAct', status: 'active', created_by: adminA, + created_at: new Date().toISOString(), updated_at: new Date().toISOString(), + }).returning('id'); + projectId = p[0]?.id ?? p[0]; + + // All three hang off adminA's project. Only the first is adminA's; the + // third is an ownerless legacy row, which filterOwnedEventIds treats as + // owned by whoever asks — but only once we know who is asking. + ownEventId = await mkEvent('canact-own', adminA, projectId); + foreignEventId = await mkEvent('canact-foreign', adminB, projectId); + ownerlessEventId = await mkEvent('canact-legacy', null, projectId); + + await mkMail(ownEventId, 'gallery_ready'); + await mkMail(foreignEventId, 'gallery_ready'); + await mkMail(ownerlessEventId, 'gallery_ready'); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + const byEvent = (overview) => { + const m = new Map(); + for (const e of overview.emails) m.set(e.eventId, e); + return m; + }; + + it('clears mail on an event the caller owns', async () => { + const overview = await projectService.getProjectOverview( + projectId, {}, { id: adminA, roleName: 'editor' }, + ); + expect(byEvent(overview).get(ownEventId).canAct).toBe(true); + }); + + it('denies mail on a foreign admin\'s event inside the caller\'s own project', async () => { + const overview = await projectService.getProjectOverview( + projectId, {}, { id: adminA, roleName: 'editor' }, + ); + // event_id is non-null here — the old client-side rule would have offered + // controls, and requireOwnedQueuedEmail would have 404'd them. + const row = byEvent(overview).get(foreignEventId); + expect(row.eventId).not.toBeNull(); + expect(row.canAct).toBe(false); + }); + + it('clears everything for a super_admin', async () => { + const overview = await projectService.getProjectOverview( + projectId, {}, { id: superAdmin, roleName: 'super_admin' }, + ); + expect(overview.emails.every((e) => e.canAct === true)).toBe(true); + }); + + it('clears mail on an ownerless legacy event for an identified caller', async () => { + // Parity with filterOwnedEventIds, which allows created_by IS NULL. + const overview = await projectService.getProjectOverview( + projectId, {}, { id: adminA, roleName: 'editor' }, + ); + expect(byEvent(overview).get(ownerlessEventId).canAct).toBe(true); + }); + + it('denies everything when no admin context is supplied', async () => { + // Including the ownerless event: `created_by == null` must not read as + // "owned" when we do not know who is asking (codex review round 2). + const overview = await projectService.getProjectOverview(projectId, {}); + expect(overview.emails.length).toBe(3); + expect(overview.emails.every((e) => e.canAct === false)).toBe(true); + }); + + it('does not leak event ownership to the client', async () => { + const overview = await projectService.getProjectOverview( + projectId, {}, { id: adminA, roleName: 'editor' }, + ); + expect(overview.events.length).toBe(3); + for (const e of overview.events) expect(e).not.toHaveProperty('created_by'); + }); +}); diff --git a/backend/src/routes/adminProjects.js b/backend/src/routes/adminProjects.js index 2c4abcc4..eded2c81 100644 --- a/backend/src/routes/adminProjects.js +++ b/backend/src/routes/adminProjects.js @@ -183,7 +183,7 @@ router.get('/:id/overview', requirePermission('events.view'), requireProjectOwne quotes: await userHasAnyPermission(req.admin.id, ['quotes.view']), contracts: await userHasAnyPermission(req.admin.id, ['contracts.view']), }; - const overview = await projectService.getProjectOverview(parseInt(req.params.id, 10), perms); + const overview = await projectService.getProjectOverview(parseInt(req.params.id, 10), perms, req.admin); return successResponse(res, overview); })); diff --git a/backend/src/services/projectService.js b/backend/src/services/projectService.js index f470f5e0..a0334dc2 100644 --- a/backend/src/services/projectService.js +++ b/backend/src/services/projectService.js @@ -454,16 +454,48 @@ function computeValuation(invoices = [], quotes = []) { * Full overview aggregation for the cockpit. Returns the project, its events, * and the rolled-up emails / quotes / contracts / invoices / hours + a * timeline of milestones. `perms` gates which doc types are included. + * + * `admin` (optional) is used only to stamp each email with `canAct` — whether + * the queued-mail routes would actually accept an action on it. See below. */ -async function getProjectOverview(id, perms = {}) { +async function getProjectOverview(id, perms = {}, admin = null) { const project = await getProjectById(id); if (!project) throw new AppError('Project not found', 404); - const events = await db('events') + // `created_by` is selected for the ownership check below and stripped again + // before the response — the cockpit has no business learning who owns a + // sibling event. + const eventRows = await db('events') .where({ project_id: id }) - .select('id', 'event_name', 'event_date', 'slug', 'is_active', 'is_draft', 'expires_at', 'is_archived'); + .select('id', 'event_name', 'event_date', 'slug', 'is_active', 'is_draft', 'expires_at', 'is_archived', 'created_by'); + const events = eventRows.map(({ created_by: _ignored, ...e }) => e); const eventIds = events.map((e) => e.id); + // Which of this project's events would filterOwnedEventIds() let `admin` + // act on. Mirrors that predicate exactly (ownership.js): super_admin gets + // everything, otherwise created_by IS NULL OR created_by = admin.id. + // + // Project ownership does NOT imply event ownership — ownedProjectsSubquery's + // `projects.created_by = admin.id` branch places no constraint on who owns + // the linked events, so a super_admin can attach admin B's event to admin + // A's project. Deriving actionability from `event_id != null` alone (as the + // UI first did) would then still render controls that requireOwnedQueuedEmail + // rejects with a 404. + // No admin context → nothing is actionable. Without this, an ownerless + // (legacy/system) event would satisfy `created_by == null` and be marked + // actionable for a caller we know nothing about. + const isSuperAdmin = admin?.roleName === 'super_admin'; + let actionableEventIds = new Set(); + if (isSuperAdmin) { + actionableEventIds = new Set(eventIds); + } else if (admin?.id != null) { + actionableEventIds = new Set( + eventRows + .filter((e) => e.created_by == null || Number(e.created_by) === Number(admin.id)) + .map((e) => e.id), + ); + } + const out = { project, events, emails: [], quotes: [], contracts: [], invoices: [], hours: { entries: [], totalMinutes: 0 } }; // Invoices (by event) incl. storno. @@ -518,6 +550,11 @@ async function getProjectOverview(id, perms = {}) { queuedAt: e.created_at, sentAt: e.sent_at, error: e.error_message, eventId: e.event_id, // false → the cockpit preview will re-render from the current template. stored: !!Number(e.has_rendered), + // Would requireOwnedQueuedEmail accept preview/resend/cancel/retry/send-now + // on this row? Authoritative here because the client cannot derive it: CRM + // document mail has no event to own, and event mail additionally requires + // ownership of THAT event, which the response deliberately does not expose. + canAct: isSuperAdmin || (e.event_id != null && actionableEventIds.has(e.event_id)), }); const emailRows = []; diff --git a/frontend/src/pages/admin/projects/ProjectCockpitPage.tsx b/frontend/src/pages/admin/projects/ProjectCockpitPage.tsx index 1ad7ee73..bfe8c480 100644 --- a/frontend/src/pages/admin/projects/ProjectCockpitPage.tsx +++ b/frontend/src/pages/admin/projects/ProjectCockpitPage.tsx @@ -27,6 +27,7 @@ import { useLocalizedDate } from '../../../hooks/useLocalizedDate'; import { useMutationWithToast } from '../../../hooks'; import { formatMoneyMinor } from '../../../utils/money'; import { useFeatureFlags, type FeatureKey } from '../../../contexts/FeatureFlagsContext'; +import { usePermissions } from '../../../contexts/PermissionsContext'; type FeedKind = 'email' | 'quote' | 'contract' | 'invoice' | 'gallery' | 'hours'; @@ -42,6 +43,9 @@ interface FeedItem { emailId?: number; emailStatus?: string; reRendered?: boolean; + /** Server's verdict on whether the queued-mail routes would accept an action + * on this row. Not derivable client-side — see canActOnEmail. */ + emailCanAct?: boolean; } /** The feature flag that gates each document's detail ROUTE (RequireFeature @@ -109,8 +113,23 @@ export const ProjectCockpitPage: React.FC = () => { const navigate = useNavigate(); const qc = useQueryClient(); const { flags } = useFeatureFlags(); + const { hasPermission } = usePermissions(); const { format, formatTime } = useLocalizedDate(); + // Which email controls are actually reachable for THIS admin, so the feed + // stops offering buttons the API will reject (#969): + // 404 — requireOwnedQueuedEmail (adminProjects.js) scopes queued mail + // through email_queue.event_id AND ownership of that event. CRM + // document mail carries no event_id at all. The server decides this + // for us (`canAct`) because neither half is derivable here: the + // overview deliberately does not expose who owns a sibling event. + // Absent (older backend) → treat as not actionable, so we fail to + // hiding a live control rather than offering a dead one. + // 403 — preview needs `events.view`, but resend/cancel/retry/send-now + // need `email.send`; the feed used to render all four regardless. + const canActOnEmail = (item: FeedItem) => item.emailCanAct === true; + const canSendEmail = hasPermission('email.send'); + const [editName, setEditName] = useState(null); const [preview, setPreview] = useState(null); const [previewLoading, setPreviewLoading] = useState(false); @@ -189,6 +208,7 @@ export const ProjectCockpitPage: React.FC = () => { title: t(`projects.feed.email`, 'Email') + ` · ${e.type}`, subtitle: e.recipient + (e.error ? ` — ${e.error}` : ''), status: e.status, emailId: e.id, emailStatus: e.status, reRendered: !e.stored, + emailCanAct: e.canAct, }); } for (const q of data.quotes) { @@ -376,7 +396,9 @@ export const ProjectCockpitPage: React.FC = () => { // not a dead strip next to them). Hours have neither → static. const onRowClick = item.href ? () => navigate(item.href as string) - : (item.kind === 'email' && item.emailId != null ? () => openPreview(item.emailId as number) : undefined); + : (item.kind === 'email' && item.emailId != null && canActOnEmail(item) + ? () => openPreview(item.emailId as number) + : undefined); return (
  • { {item.status} )} {item.amount && {item.amount}} - {item.kind === 'email' && item.emailId != null && ( + {item.kind === 'email' && item.emailId != null && canActOnEmail(item) && (
    e.stopPropagation()}> )} - {item.emailStatus === 'pending' && ( + {canSendEmail && item.emailStatus === 'pending' && ( <> )} - {item.emailStatus === 'failed' && ( + {canSendEmail && item.emailStatus === 'failed' && ( diff --git a/frontend/src/pages/admin/projects/__tests__/ProjectCockpitEmailControls.test.tsx b/frontend/src/pages/admin/projects/__tests__/ProjectCockpitEmailControls.test.tsx new file mode 100644 index 00000000..9fcc4617 --- /dev/null +++ b/frontend/src/pages/admin/projects/__tests__/ProjectCockpitEmailControls.test.tsx @@ -0,0 +1,158 @@ +/** + * The cockpit's email feed used to render preview/resend/cancel/retry/send-now + * for every mail, consulting neither the caller's role nor their permissions. + * Two ways that produced dead controls (#969): + * + * 404 — requireOwnedQueuedEmail scopes queued mail through + * email_queue.event_id. CRM document mail (quote/contract/invoice/ + * storno) is queued with event_id = null, so for a non-super_admin it + * has no ownable parent and always 404s. Introduced by the GHSA-93x4 + * fix in #960/#966, which added the ownership middleware. + * 403 — preview requires `events.view` but the four write actions require + * `email.send`; the feed rendered all of them regardless. + */ + +import { render, screen, waitFor } from '@testing-library/react'; +import { vi, describe, it, expect, beforeEach } from 'vitest'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + t: (k: string, fb?: unknown) => (typeof fb === 'string' ? fb : k), + i18n: { language: 'en' }, + }), + }; +}); + +vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +// An event mail (actionable by anyone who owns the event) and a CRM document +// mail (event_id null → only a super_admin can act on it). Both 'sent', so the +// Resend button is the one under test in each row. +const OVERVIEW = { + project: { + id: 1, name: 'Hochzeit Müller', customerAccountId: 1, + customerEmail: 'kunde@example.com', status: 'active', + createdAt: '2026-07-01T00:00:00Z', updatedAt: '2026-08-01T00:00:00Z', + }, + events: [], quotes: [], contracts: [], invoices: [], + hours: { entries: [], totalMinutes: 0 }, + milestones: [], + valuation: { byCurrency: [] }, + emails: [ + // Event mail the caller owns → the server says actionable. + { id: 11, recipient: 'kunde@example.com', type: 'gallery_ready', status: 'sent', + queuedAt: '2026-08-01T10:00:00Z', sentAt: '2026-08-01T10:00:05Z', error: null, + eventId: 42, stored: true, canAct: true }, + // CRM document mail — no event to own, so 404 for a non-super_admin. + { id: 12, recipient: 'kunde@example.com', type: 'invoice_sent', status: 'sent', + queuedAt: '2026-08-01T11:00:00Z', sentAt: '2026-08-01T11:00:05Z', error: null, + eventId: null, stored: true, canAct: false }, + // Event mail for a FOREIGN event: a super_admin can attach admin B's event + // to admin A's project, and project ownership does not imply event + // ownership — so eventId is non-null yet the action still 404s. + { id: 13, recipient: 'kunde@example.com', type: 'gallery_ready', status: 'sent', + queuedAt: '2026-08-01T12:00:00Z', sentAt: '2026-08-01T12:00:05Z', error: null, + eventId: 99, stored: true, canAct: false }, + ], +}; + +/** The same payload as a super_admin sees it — the server stamps every row. */ +const OVERVIEW_SUPER = { + ...OVERVIEW, + emails: OVERVIEW.emails.map((e) => ({ ...e, canAct: true })), +}; + +let currentOverview: typeof OVERVIEW = OVERVIEW; + +vi.mock('../../../../services/projects.service', () => ({ + projectsService: { + overview: vi.fn(async () => currentOverview), + emailPreview: vi.fn(async () => ({ html: '

    x

    ', subject: 's' })), + resendEmail: vi.fn(), cancelEmail: vi.fn(), retryEmail: vi.fn(), sendEmailNow: vi.fn(), + update: vi.fn(), assignEvent: vi.fn(), + }, +})); + +vi.mock('../../../../services/events.service', () => ({ + eventsService: { getEvents: vi.fn(async () => []) }, +})); + +vi.mock('../../../../contexts/FeatureFlagsContext', () => ({ + useFeatureFlags: () => ({ flags: { projects: true, quotes: true, contracts: true, bills: true } }), +})); + +let mockPerms = { isSuperAdmin: false, permissions: ['events.view'] }; +vi.mock('../../../../contexts/PermissionsContext', () => ({ + usePermissions: () => ({ + isSuperAdmin: mockPerms.isSuperAdmin, + hasPermission: (p: string) => mockPerms.isSuperAdmin || mockPerms.permissions.includes(p), + }), +})); + +import { ProjectCockpitPage } from '../ProjectCockpitPage'; + +const renderPage = async () => { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + + } /> + + + , + ); + await waitFor(() => expect(screen.getAllByText(/Email/).length).toBeGreaterThan(0)); +}; + +describe('ProjectCockpitPage email controls (#969)', () => { + beforeEach(() => { + mockPerms = { isSuperAdmin: false, permissions: ['events.view'] }; + currentOverview = OVERVIEW; + }); + + it('super_admin sees controls on every row', async () => { + mockPerms = { isSuperAdmin: true, permissions: [] }; + currentOverview = OVERVIEW_SUPER; + await renderPage(); + // All three rows, Resend on all (super_admin implies email.send). + expect(screen.getAllByText('Preview')).toHaveLength(3); + expect(screen.getAllByText('Resend')).toHaveLength(3); + }); + + it('a scoped admin gets controls only on mail the server says it can act on', async () => { + mockPerms = { isSuperAdmin: false, permissions: ['events.view', 'email.send'] }; + await renderPage(); + // Only the owned event mail. The CRM row (no event) and the foreign-event + // row both 404, so neither offers anything. + expect(screen.getAllByText('Preview')).toHaveLength(1); + expect(screen.getAllByText('Resend')).toHaveLength(1); + }); + + it('an admin without email.send sees preview but no write actions (would 403)', async () => { + mockPerms = { isSuperAdmin: false, permissions: ['events.view'] }; + await renderPage(); + expect(screen.getAllByText('Preview')).toHaveLength(1); + expect(screen.queryByText('Resend')).toBeNull(); + expect(screen.queryByText('Cancel')).toBeNull(); + expect(screen.queryByText('Retry')).toBeNull(); + expect(screen.queryByText('Send now')).toBeNull(); + }); + + it('offers nothing when the server omits canAct (older backend)', async () => { + mockPerms = { isSuperAdmin: false, permissions: ['events.view', 'email.send'] }; + currentOverview = { + ...OVERVIEW, + emails: OVERVIEW.emails.map(({ canAct: _drop, ...e }) => e as typeof OVERVIEW.emails[number]), + }; + await renderPage(); + // Fail to hiding a live control rather than offering a dead one. + expect(screen.queryByText('Preview')).toBeNull(); + expect(screen.queryByText('Resend')).toBeNull(); + }); +}); diff --git a/frontend/src/services/projects.service.ts b/frontend/src/services/projects.service.ts index 1760d8ea..29a6dd54 100644 --- a/frontend/src/services/projects.service.ts +++ b/frontend/src/services/projects.service.ts @@ -49,6 +49,10 @@ export interface ProjectEmail { eventId: number | null; /** true = exact HTML stored at send time; false = preview re-rendered. */ stored: boolean; + /** Authoritative: would the queued-mail routes accept an action on this row? + * False for CRM document mail (no event to own) and for event mail the + * caller does not own. Absent on older backends → treated as false. */ + canAct?: boolean; } export interface ProjectInvoice {