diff --git a/CHANGELOG.md b/CHANGELOG.md index d384fe2a..9c979225 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,62 @@ All notable changes to the Fovea project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.5.5] - 2026-06-29 + +The 0.5.5 patch is an audit-driven hardening release ([#186](https://github.com/parafovea/fovea/pull/186)). It closes two higher-severity defects — a former group member kept the group's permissions until the server restarted, and concurrent edits to the world graph silently overwrote one another — together with a set of data-scope, idempotency, and cache-staleness gaps. Nothing is breaking; the API additively gains explicit DELETE endpoints for world collections and relations, an optional client-supplied `id` on claim creation, and a `409` on a duplicate video assignment. + +### Fixed + +#### Deleting a Group Clears Its Former Members' Cached Abilities + +- Both group-delete handlers (`server/src/routes/groups.ts`) removed the group without invalidating its former members' in-memory ability cache. A group-scope, non-own-only permission compiles into a globally unconditioned CASL rule, so a former member kept the group's system-wide access until the process restarted or they logged in again. Each handler now snapshots the membership before the cascade and calls `invalidateUserAbilities` for every former member, mirroring `ProjectService.delete`. + +#### Concurrent World-Graph Edits No Longer Overwrite One Another + +- Every world add and update went through a whole-blob read-modify-write `PUT /api/world` with no serialization, so two edits in quick succession — or a Wikidata import racing a manual edit — read the same stale base and the last writer silently dropped the other's entities, events, or relations. Writes are now safe on two fronts: the client (`annotation-tool/src/store/queries/useWorld.ts`) funnels mutations through a single-flight chain that threads the latest server state into the next write, and the server (`server/src/services/world-state-service.ts`, `server/src/repositories/WorldStateRepository.ts`) merges each of the seven object arrays by `id` under an optimistic-concurrency guard rather than replacing them wholesale. Because merge-by-id makes removal-by-omission a no-op, every object removal — entities, events, times, the three collection kinds, and relations — now goes through an explicit DELETE endpoint; the client delete hooks call the graceful `DELETE /api/world/...` routes, which also clean up dependent relations, collection memberships, and gloss references. + +#### Personaless Object Annotations Inherit the Video's Project Scope + +- Object annotations carry no persona, so the create path (`server/src/routes/annotations.ts`) had no project to stamp and persisted them with `projectId = NULL`, invisible to every project reviewer but the creator. When the caller is a member of exactly one project the video is assigned to, the annotation now inherits that project; an ambiguous (multiple) or absent assignment stays personal, and the existing CASL pre-authorization still validates the resolved scope. + +#### Claim Creation Is Idempotent on a Client-Supplied ID + +- Neither claim-create route accepted a client `id`, so a network retry or programmatic resend minted a duplicate claim. Both routes (`server/src/routes/claims.ts`, `server/src/services/claim-service.ts`) now accept an optional `id`; a resend carrying an existing id re-authorizes against the stored row and returns it instead of creating a duplicate, with a `P2002` race fallback. This mirrors the annotation idempotent-create hardening from 0.5.4. + +#### Duplicate Video Assignment Returns 409 Instead of 500 + +- Re-assigning a video already assigned to a project violated the `@@unique([projectId, videoId])` constraint and surfaced as an unhandled `500`. The route (`server/src/routes/video-assignments.ts`) now catches the constraint violation and returns a `409 Conflict`. + +#### The Summary Editor Autosaves Comment-Only Edits + +- The video summary editor's autosave watched only the summary body, so editing the comment without touching the summary never triggered a save and the edit was lost on navigation. Autosave now compares a snapshot that includes the comment (`annotation-tool/src/components/video/VideoSummaryEditor.tsx`). + +#### Relating Claims Refreshes Both Endpoints' Relation Panels + +- Creating or deleting a claim relation invalidated only the source claim's relation query, leaving the target claim's relation panel stale until a manual refetch. Both mutations (`annotation-tool/src/store/queries/useClaims.ts`) now invalidate the target as well. + +#### Membership and Role Changes Refresh the Client Ability Mirror + +- The client ability cache carried a five-minute stale time and no mutation invalidated it, so a user's own role or membership change took up to five minutes to reflect in the UI even though the server always enforced it correctly. The six self-affecting project- and group-membership mutations (`annotation-tool/src/store/queries/useProjects.ts`, `useGroups.ts`) now invalidate the ability mirror on success. + +#### Admin isAdmin Changes Stay in Sync With systemRole + +- The admin user-update endpoint (`server/src/routes/users.ts`) wrote `isAdmin` but never `systemRole`, which is what CASL's `manage all` keys on, so a promotion or demotion left the two fields divergent and the cached abilities stale. The endpoint now sets `systemRole` to match and invalidates the affected user's abilities. + +#### Removed a Dead Auto-Save Hook + +- `useAutoSaveAnnotations` had no callers and re-armed the exact save-loop footgun the live autosave was hardened against; it has been deleted. + +### Added + +#### Explicit DELETE Endpoints for World Collections and Relations + +- `server/src/routes/world.ts` gains `DELETE` routes for entity collections, event collections, time collections, and relations, so these objects are removed explicitly rather than by omission from a whole-blob `PUT`. This is what lets the non-clobbering merge-by-id persistence be safe (see Fixed). + +#### Optional Client-Supplied ID on Claim Creation + +- Both claim-create endpoints accept an optional `id`, enabling idempotent retries; the `Claim` response already echoes it. + ## [0.5.4] - 2026-06-25 The 0.5.4 patch fixes project-scope and ownership stamping on video summaries and claims ([#181](https://github.com/parafovea/fovea/pull/181)). Project collaborators could not see a teammate's summary or add claims under it because summaries were persisted without their persona's project, and model-generated summaries and extracted claims were left unowned. Nothing is breaking; the API additively gains a `projectId` field on summary and claim responses. diff --git a/annotation-tool/package.json b/annotation-tool/package.json index a9f5c9cc..2cc4c4c3 100644 --- a/annotation-tool/package.json +++ b/annotation-tool/package.json @@ -1,7 +1,7 @@ { "name": "@fovea/annotation-tool", "private": true, - "version": "0.5.4", + "version": "0.5.5", "type": "module", "scripts": { "dev": "vite", diff --git a/annotation-tool/src/api/generated/openapi.ts b/annotation-tool/src/api/generated/openapi.ts index 13281501..168591ad 100644 --- a/annotation-tool/src/api/generated/openapi.ts +++ b/annotation-tool/src/api/generated/openapi.ts @@ -3774,6 +3774,254 @@ export interface paths { patch?: never; trace?: never; }; + "/api/world/entity-collections/{objectId}": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + post?: never; + /** @description Delete a world entity collection from the caller's personal world */ + delete: { + parameters: { + query?: never; + header?: never; + path: { + objectId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Default Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + success: boolean; + }; + }; + }; + /** @description Default Response */ + 403: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + /** @description Default Response */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + }; + }; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; + "/api/world/event-collections/{objectId}": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + post?: never; + /** @description Delete a world event collection from the caller's personal world */ + delete: { + parameters: { + query?: never; + header?: never; + path: { + objectId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Default Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + success: boolean; + }; + }; + }; + /** @description Default Response */ + 403: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + /** @description Default Response */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + }; + }; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; + "/api/world/time-collections/{objectId}": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + post?: never; + /** @description Delete a world time collection from the caller's personal world */ + delete: { + parameters: { + query?: never; + header?: never; + path: { + objectId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Default Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + success: boolean; + }; + }; + }; + /** @description Default Response */ + 403: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + /** @description Default Response */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + }; + }; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; + "/api/world/relations/{objectId}": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + post?: never; + /** @description Delete a world relation from the caller's personal world */ + delete: { + parameters: { + query?: never; + header?: never; + path: { + objectId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Default Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + success: boolean; + }; + }; + }; + /** @description Default Response */ + 403: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + /** @description Default Response */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + error: string; + }; + }; + }; + }; + }; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/world/events/{eventId}/deletion-preview": { parameters: { query?: never; @@ -4901,6 +5149,8 @@ export interface paths { requestBody: { content: { "application/json": { + /** Format: uuid */ + id?: string; summaryType: "video" | "collection"; text: string; gloss?: { @@ -5937,6 +6187,8 @@ export interface paths { requestBody: { content: { "application/json": { + /** Format: uuid */ + id?: string; text: string; gloss?: { type: "text" | "typeRef" | "objectRef" | "annotationRef" | "claimRef"; @@ -10996,6 +11248,22 @@ export interface paths { }; }; }; + /** @description Default Response */ + 409: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + /** @description Machine-readable error code */ + error: string; + /** @description Human-readable error message */ + message: string; + /** @description Additional error context */ + details?: unknown; + }; + }; + }; }; }; delete?: never; diff --git a/annotation-tool/src/components/video/VideoSummaryEditor.test.tsx b/annotation-tool/src/components/video/VideoSummaryEditor.test.tsx index cacbc18f..334f487c 100644 --- a/annotation-tool/src/components/video/VideoSummaryEditor.test.tsx +++ b/annotation-tool/src/components/video/VideoSummaryEditor.test.tsx @@ -378,6 +378,62 @@ describe('VideoSummaryEditor', () => { { timeout: 5000 } ) }) + + it('autosaves a comment-only edit (no summary change)', async () => { + const user = userEvent.setup() + const { useVideoSummary, useSaveSummary } = await import('@store/queries') + const mockMutateAsync = vi.fn().mockResolvedValue({}) + const existingSummary = { + id: 'summary-1', + videoId: 'test-video', + personaId: 'test-persona', + summary: [], + comment: null, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } + + vi.mocked(useVideoSummary).mockReturnValue({ + data: existingSummary, + isLoading: false, + error: null, + isError: false, + refetch: vi.fn(), + } as any) + + vi.mocked(useSaveSummary).mockReturnValue({ + mutate: vi.fn(), + mutateAsync: mockMutateAsync, + isPending: false, + error: null, + } as any) + + render( + , + { wrapper: createWrapper() } + ) + + await user.click(screen.getByRole('tab', { name: /Summary/i })) + + const commentField = await screen.findByPlaceholderText(/Enter comment/i) + // Only the comment changes; the summary body is left untouched. The + // getComparisonSnapshot wiring makes autosave notice the comment delta. + await user.type(commentField, 'Comment only') + + await waitFor( + () => { + expect(mockMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ + comment: expect.stringContaining('Comment only'), + }) + ) + }, + { timeout: 5000 } + ) + }) }) describe('Default Tab', () => { diff --git a/annotation-tool/src/components/video/VideoSummaryEditor.tsx b/annotation-tool/src/components/video/VideoSummaryEditor.tsx index a63cad27..c0b28bbd 100644 --- a/annotation-tool/src/components/video/VideoSummaryEditor.tsx +++ b/annotation-tool/src/components/video/VideoSummaryEditor.tsx @@ -164,6 +164,11 @@ const VideoSummaryEditor = forwardRef ({ summary, comment: localComment }), // Autosave is enabled as soon as videoId + personaId resolve. The // previous gate (&& !!summaryId) meant the dialog could only // autosave AFTER a row already existed for this video/persona — diff --git a/annotation-tool/src/hooks/data/index.ts b/annotation-tool/src/hooks/data/index.ts index fb63b0c2..3fcbd631 100644 --- a/annotation-tool/src/hooks/data/index.ts +++ b/annotation-tool/src/hooks/data/index.ts @@ -10,7 +10,6 @@ export type { UseAutoSaveOptions, UseAutoSaveReturn, } from './useAutoSave' -export { useAutoSaveAnnotations } from './useAutoSaveAnnotations' export { useUnsavedChangesPrompt } from './useUnsavedChangesPrompt' export type { UseUnsavedChangesPromptOptions, diff --git a/annotation-tool/src/hooks/data/useAutoSaveAnnotations.ts b/annotation-tool/src/hooks/data/useAutoSaveAnnotations.ts deleted file mode 100644 index de354411..00000000 --- a/annotation-tool/src/hooks/data/useAutoSaveAnnotations.ts +++ /dev/null @@ -1,110 +0,0 @@ -/** - * @module useAutoSaveAnnotations - * @description Hook for automatically saving annotations to the database. - * Follows the same pattern as ontology and world object auto-save: - * - Simple debounced save using TanStack Query mutation - * - Tracks which IDs were initially loaded to distinguish create vs update - * - Prevents saving on initial load (annotations just fetched from database) - */ - -import { useEffect, useRef } from 'react' -import { useSaveAnnotations } from '@store/queries' -import { Annotation } from '@models/types' - -/** - * @interface UseAutoSaveAnnotationsParams - * @description Parameters for useAutoSaveAnnotations hook. - */ -interface UseAutoSaveAnnotationsParams { - /** Video ID for annotations */ - videoId: string | undefined - /** Persona ID for filtering annotations */ - personaId: string | null - /** Annotations to auto-save */ - annotations: Annotation[] - /** Debounce delay in milliseconds (default: 1000ms to match ontology/world) */ - debounceMs?: number -} - -/** - * @hook useAutoSaveAnnotations - * @description Automatically saves annotations to the database with debouncing. - * Simplified to match the ontology/world object auto-save pattern. - * - * Key improvements: - * - Uses Redux async thunk instead of direct API calls - * - Consistent with other auto-save implementations - * - Simpler logic, easier to maintain - * - Correctly distinguishes between initial load and user-created annotations - * - * @param params - Hook parameters - * - * @example - * ```tsx - * useAutoSaveAnnotations({ - * videoId, - * personaId, - * annotations, - * debounceMs: 1000 - * }) - * ``` - */ -export function useAutoSaveAnnotations({ - videoId, - personaId, - annotations, - debounceMs = 1000, -}: UseAutoSaveAnnotationsParams): void { - const { mutate: saveAnnotations } = useSaveAnnotations() - const previousAnnotationsRef = useRef([]) - const loadedAnnotationIdsRef = useRef>(new Set()) - - // One-time initialization: capture which annotation IDs exist when hook first runs - // This happens immediately on mount, so we know which annotations came from database - useEffect(() => { - // Store IDs of any initially loaded annotations - annotations.forEach(ann => { - if (ann.id) { - loadedAnnotationIdsRef.current.add(ann.id) - } - }) - previousAnnotationsRef.current = annotations - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []) // Empty array means this only runs once on mount - - // Auto-save annotations on changes (debounced) - // This matches the pattern used by OntologyWorkspace and ObjectWorkspace - useEffect(() => { - if (!videoId) { - return - } - - // Check if annotations actually changed - const currentStr = JSON.stringify(annotations) - const previousStr = JSON.stringify(previousAnnotationsRef.current) - if (currentStr === previousStr) { - return - } - - const timeoutId = setTimeout(() => { - if (annotations.length === 0) { - return - } - - // Filter annotations by persona if specified - const annotationsToSave = personaId - ? annotations.filter(a => 'personaId' in a && a.personaId === personaId) - : annotations - - saveAnnotations({ - videoId, - annotations: annotationsToSave - }) - }, debounceMs) - - // Update ref immediately so we can detect future changes - previousAnnotationsRef.current = annotations - - return () => clearTimeout(timeoutId) - }, [videoId, personaId, annotations, debounceMs, saveAnnotations]) -} diff --git a/annotation-tool/src/store/queries/useClaims.test.tsx b/annotation-tool/src/store/queries/useClaims.test.tsx index fa0378f7..eca07563 100644 --- a/annotation-tool/src/store/queries/useClaims.test.tsx +++ b/annotation-tool/src/store/queries/useClaims.test.tsx @@ -13,6 +13,7 @@ import { useDeleteClaim, useCreateClaimRelation, useDeleteClaimRelation, + claimsQueryKeys, } from './useClaims' import { server } from '@test/setup' import { http, HttpResponse } from 'msw' @@ -257,6 +258,32 @@ describe('useClaims hooks', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)) }) + it('invalidates both the source and target claim relation queries', async () => { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }) + const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries') + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ) + const { result } = renderHook(() => useCreateClaimRelation(), { wrapper }) + + result.current.mutate({ + summaryId: 'summary-1', + sourceClaimId: 'claim-1', + relation: { targetClaimId: 'claim-2', relationTypeId: 'supports' }, + }) + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + // A relation shows in both endpoints' panels, so both keys must refresh. + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: claimsQueryKeys.relations('summary-1', 'claim-1'), + }) + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: claimsQueryKeys.relations('summary-1', 'claim-2'), + }) + }) + it('handles errors', async () => { server.use( http.post('/api/summaries/:summaryId/claims/:sourceClaimId/relations', () => { @@ -300,6 +327,26 @@ describe('useClaims hooks', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)) }) + it('invalidates all relation queries for the summary (source and target)', async () => { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }) + const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries') + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ) + const { result } = renderHook(() => useDeleteClaimRelation(), { wrapper }) + + result.current.mutate({ summaryId: 'summary-1', relationId: 'relation-1', sourceClaimId: 'claim-1' }) + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + // The delete vars lack the target id, so it invalidates the summary-wide + // relations prefix (which covers both endpoints). + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: claimsQueryKeys.relationsBySummary('summary-1'), + }) + }) + it('handles errors', async () => { server.use( http.delete('/api/summaries/:summaryId/claims/relations/:relationId', () => { diff --git a/annotation-tool/src/store/queries/useClaims.ts b/annotation-tool/src/store/queries/useClaims.ts index 2ad27452..6b414aa8 100644 --- a/annotation-tool/src/store/queries/useClaims.ts +++ b/annotation-tool/src/store/queries/useClaims.ts @@ -37,6 +37,9 @@ export const claimsQueryKeys = { bySummary: (summaryId: string) => [...claimsQueryKeys.all, 'summary', summaryId] as const, relations: (summaryId: string, claimId: string) => [...claimsQueryKeys.all, 'relations', summaryId, claimId] as const, + /** Prefix matching every claim's relations within a summary (both endpoints). */ + relationsBySummary: (summaryId: string) => + [...claimsQueryKeys.all, 'relations', summaryId] as const, extractionJob: (jobId: string) => [...claimsQueryKeys.all, 'job', jobId] as const, } @@ -403,11 +406,15 @@ export function useCreateClaimRelation() { notes?: string } }) => createClaimRelation(summaryId, sourceClaimId, relation), - onSuccess: (_, { summaryId, sourceClaimId }) => { - // Invalidate relations cache for this claim + onSuccess: (_, { summaryId, sourceClaimId, relation }) => { + // A relation appears in BOTH endpoints' panels (asSource / asTarget), so + // invalidate the source and the target claim's relation queries. queryClient.invalidateQueries({ queryKey: claimsQueryKeys.relations(summaryId, sourceClaimId), }) + queryClient.invalidateQueries({ + queryKey: claimsQueryKeys.relations(summaryId, relation.targetClaimId), + }) }, }) } @@ -427,9 +434,12 @@ export function useDeleteClaimRelation() { relationId: string sourceClaimId: string // For cache invalidation }) => deleteClaimRelation(summaryId, relationId), - onSuccess: (_, { summaryId, sourceClaimId }) => { + onSuccess: (_, { summaryId }) => { + // The delete variables don't carry the target claim id, so invalidate + // every relation query for this summary (prefix match) — this clears both + // the source and target endpoints' stale incoming/outgoing panels. queryClient.invalidateQueries({ - queryKey: claimsQueryKeys.relations(summaryId, sourceClaimId), + queryKey: claimsQueryKeys.relationsBySummary(summaryId), }) }, }) diff --git a/annotation-tool/src/store/queries/useGroups.ts b/annotation-tool/src/store/queries/useGroups.ts index 34176e98..fd7aa381 100644 --- a/annotation-tool/src/store/queries/useGroups.ts +++ b/annotation-tool/src/store/queries/useGroups.ts @@ -6,6 +6,7 @@ */ import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query' +import { abilityKeys } from './useAbilities' /** Summary of a group as returned by the list endpoint. */ export interface GroupSummary { @@ -234,6 +235,9 @@ export function useAddGroupMember() { onSuccess: (_, { groupId }) => { queryClient.invalidateQueries({ queryKey: groupKeys.detail(groupId) }) queryClient.invalidateQueries({ queryKey: groupKeys.members(groupId) }) + // Membership/role changes alter the caller's own permissions; refresh the + // client ability mirror so the UI reflects them without a staleTime lag. + queryClient.invalidateQueries({ queryKey: abilityKeys.all }) queryClient.invalidateQueries({ queryKey: groupKeys.lists() }) }, }) @@ -263,6 +267,9 @@ export function useUpdateGroupMember() { onSuccess: (_, { groupId }) => { queryClient.invalidateQueries({ queryKey: groupKeys.detail(groupId) }) queryClient.invalidateQueries({ queryKey: groupKeys.members(groupId) }) + // Membership/role changes alter the caller's own permissions; refresh the + // client ability mirror so the UI reflects them without a staleTime lag. + queryClient.invalidateQueries({ queryKey: abilityKeys.all }) }, }) } @@ -288,6 +295,9 @@ export function useRemoveGroupMember() { onSuccess: (_, { groupId }) => { queryClient.invalidateQueries({ queryKey: groupKeys.detail(groupId) }) queryClient.invalidateQueries({ queryKey: groupKeys.members(groupId) }) + // Membership/role changes alter the caller's own permissions; refresh the + // client ability mirror so the UI reflects them without a staleTime lag. + queryClient.invalidateQueries({ queryKey: abilityKeys.all }) queryClient.invalidateQueries({ queryKey: groupKeys.lists() }) }, }) diff --git a/annotation-tool/src/store/queries/useProjects.test.tsx b/annotation-tool/src/store/queries/useProjects.test.tsx index e12c85af..b1713a40 100644 --- a/annotation-tool/src/store/queries/useProjects.test.tsx +++ b/annotation-tool/src/store/queries/useProjects.test.tsx @@ -2,7 +2,7 @@ * Tests for useProjects TanStack Query hooks. */ -import { describe, it, expect, beforeEach } from 'vitest' +import { describe, it, expect, beforeEach, vi } from 'vitest' import { renderHook, waitFor, act } from '@testing-library/react' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import React from 'react' @@ -11,8 +11,10 @@ import { useProject, useProjectMembers, useCreateProject, + useAddProjectMember, projectKeys, } from './useProjects' +import { abilityKeys } from './useAbilities' import { server } from '@test/setup' import { http, HttpResponse } from 'msw' @@ -268,4 +270,28 @@ describe('useProjects hooks', () => { }) }) }) + + describe('useAddProjectMember', () => { + it('invalidates the client ability mirror so permission changes are not stale', async () => { + server.use( + http.post('*/api/projects/:projectId/members', () => + HttpResponse.json({ id: 'pm-new', userId: 'user-3', role: 'annotator' }, { status: 201 }) + ) + ) + + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }) + const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries') + const wrapper = ({ children }: { children: React.ReactNode }) => ( + {children} + ) + const { result } = renderHook(() => useAddProjectMember(), { wrapper }) + + result.current.mutate({ projectId: 'proj-1', userId: 'user-3', role: 'annotator' }) + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: abilityKeys.all }) + }) + }) }) diff --git a/annotation-tool/src/store/queries/useProjects.ts b/annotation-tool/src/store/queries/useProjects.ts index 4ed44523..fb8045cd 100644 --- a/annotation-tool/src/store/queries/useProjects.ts +++ b/annotation-tool/src/store/queries/useProjects.ts @@ -6,6 +6,7 @@ */ import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query' +import { abilityKeys } from './useAbilities' /** Query key factory for projects. */ export const projectKeys = { @@ -313,6 +314,9 @@ export function useAddProjectMember() { onSuccess: (_, { projectId }) => { queryClient.invalidateQueries({ queryKey: projectKeys.detail(projectId) }) queryClient.invalidateQueries({ queryKey: projectKeys.members(projectId) }) + // Membership/role changes alter the caller's own permissions; refresh the + // client ability mirror so the UI reflects them without a staleTime lag. + queryClient.invalidateQueries({ queryKey: abilityKeys.all }) queryClient.invalidateQueries({ queryKey: projectKeys.assignableUsers(projectId) }) queryClient.invalidateQueries({ queryKey: projectKeys.lists() }) }, @@ -343,6 +347,9 @@ export function useUpdateProjectMember() { onSuccess: (_, { projectId }) => { queryClient.invalidateQueries({ queryKey: projectKeys.detail(projectId) }) queryClient.invalidateQueries({ queryKey: projectKeys.members(projectId) }) + // Membership/role changes alter the caller's own permissions; refresh the + // client ability mirror so the UI reflects them without a staleTime lag. + queryClient.invalidateQueries({ queryKey: abilityKeys.all }) }, }) } @@ -368,6 +375,9 @@ export function useRemoveProjectMember() { onSuccess: (_, { projectId }) => { queryClient.invalidateQueries({ queryKey: projectKeys.detail(projectId) }) queryClient.invalidateQueries({ queryKey: projectKeys.members(projectId) }) + // Membership/role changes alter the caller's own permissions; refresh the + // client ability mirror so the UI reflects them without a staleTime lag. + queryClient.invalidateQueries({ queryKey: abilityKeys.all }) queryClient.invalidateQueries({ queryKey: projectKeys.assignableUsers(projectId) }) queryClient.invalidateQueries({ queryKey: projectKeys.lists() }) }, diff --git a/annotation-tool/src/store/queries/useWorld.test.tsx b/annotation-tool/src/store/queries/useWorld.test.tsx index a32bccb0..3b040ca3 100644 --- a/annotation-tool/src/store/queries/useWorld.test.tsx +++ b/annotation-tool/src/store/queries/useWorld.test.tsx @@ -178,6 +178,31 @@ describe('useWorld hooks', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)) }) + + // Regression guard: deletion must hit the graceful DELETE endpoint, not a + // whole-blob PUT. Removing by omission from a PUT is silently undone by the + // server's merge-by-id, so a deleted object would reappear. + it('calls the DELETE endpoint and not a whole-blob PUT', async () => { + let deleteHit = false + let putHit = false + server.use( + http.delete('*/api/world/entities/:entityId', () => { + deleteHit = true + return HttpResponse.json({ message: 'Entity deleted successfully' }) + }), + http.put('*/api/world', () => { + putHit = true + return HttpResponse.json({}) + }) + ) + + const { result } = renderHook(() => useDeleteEntity(), { wrapper: createWrapper() }) + result.current.mutate('entity-1') + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + expect(deleteHit).toBe(true) + expect(putHit).toBe(false) + }) }) }) @@ -226,6 +251,28 @@ describe('useWorld hooks', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)) }) + + it('calls the DELETE endpoint and not a whole-blob PUT', async () => { + let deleteHit = false + let putHit = false + server.use( + http.delete('*/api/world/events/:eventId', () => { + deleteHit = true + return HttpResponse.json({ message: 'Event deleted successfully' }) + }), + http.put('*/api/world', () => { + putHit = true + return HttpResponse.json({}) + }) + ) + + const { result } = renderHook(() => useDeleteEvent(), { wrapper: createWrapper() }) + result.current.mutate('event-1') + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + expect(deleteHit).toBe(true) + expect(putHit).toBe(false) + }) }) }) @@ -274,6 +321,28 @@ describe('useWorld hooks', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)) }) + + it('calls the DELETE endpoint and not a whole-blob PUT', async () => { + let deleteHit = false + let putHit = false + server.use( + http.delete('*/api/world/times/:timeId', () => { + deleteHit = true + return HttpResponse.json({ message: 'Time deleted successfully' }) + }), + http.put('*/api/world', () => { + putHit = true + return HttpResponse.json({}) + }) + ) + + const { result } = renderHook(() => useDeleteTime(), { wrapper: createWrapper() }) + result.current.mutate('time-1') + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + expect(deleteHit).toBe(true) + expect(putHit).toBe(false) + }) }) }) diff --git a/annotation-tool/src/store/queries/useWorld.ts b/annotation-tool/src/store/queries/useWorld.ts index d8626ecc..574a72c5 100644 --- a/annotation-tool/src/store/queries/useWorld.ts +++ b/annotation-tool/src/store/queries/useWorld.ts @@ -84,7 +84,7 @@ export async function fetchWorldState(): Promise { * Save world state to the API. * Exported for use in non-component contexts (e.g., seedTestData). */ -export async function saveWorldState(worldState: Partial): Promise { +async function putWorldState(worldState: Partial): Promise { const response = await fetch('/api/world', { method: 'PUT', headers: { @@ -99,6 +99,41 @@ export async function saveWorldState(worldState: Partial): Promise = Promise.resolve() + +/** + * Save (a partial of) the world state. Serialized: the actual PUT runs only + * after the previous one settles. Exported for non-component contexts. + */ +export async function saveWorldState(worldState: Partial): Promise { + const run = worldWriteChain.then(() => putWorldState(worldState)) + // Keep the chain alive even if a write rejects, so one failure doesn't wedge + // every subsequent world write. + worldWriteChain = run.catch(() => undefined) + return run +} + +/** + * DELETE a single world object (collection or relation) from the caller's + * personal world. Used instead of removing it via the whole-blob PUT, which the + * server now merges by id — so omitting an object from the PUT no longer + * removes it; removal must be an explicit DELETE. + */ +async function deleteWorldObject(path: string, label: string): Promise { + const response = await fetch(path, { method: 'DELETE', credentials: 'include' }) + if (!response.ok) { + const error = await response.json().catch(() => ({})) + throw new Error(error.error || `Failed to delete ${label}`) + } +} + // ============= Query Hooks ============= /** @@ -258,22 +293,10 @@ export function useDeleteEntity() { const queryClient = useQueryClient() return useMutation({ - mutationFn: async (entityId: string) => { - const currentState = queryClient.getQueryData(worldKeys.state()) - const newState = { - ...currentState, - entities: currentState?.entities.filter((e) => e.id !== entityId) ?? [], - // Clean up relations involving this entity - relations: currentState?.relations.filter( - (r) => - !(r.sourceType === 'entity' && r.sourceId === entityId) && - !(r.targetType === 'entity' && r.targetId === entityId) - ) ?? [], - } - return saveWorldState(newState) - }, - onSuccess: (data) => { - queryClient.setQueryData(worldKeys.state(), data) + mutationFn: (entityId: string) => + deleteWorldObject(`/api/world/entities/${entityId}`, 'entity'), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: worldKeys.state() }) }, }) } @@ -341,21 +364,10 @@ export function useDeleteEvent() { const queryClient = useQueryClient() return useMutation({ - mutationFn: async (eventId: string) => { - const currentState = queryClient.getQueryData(worldKeys.state()) - const newState = { - ...currentState, - events: currentState?.events.filter((e) => e.id !== eventId) ?? [], - relations: currentState?.relations.filter( - (r) => - !(r.sourceType === 'event' && r.sourceId === eventId) && - !(r.targetType === 'event' && r.targetId === eventId) - ) ?? [], - } - return saveWorldState(newState) - }, - onSuccess: (data) => { - queryClient.setQueryData(worldKeys.state(), data) + mutationFn: (eventId: string) => + deleteWorldObject(`/api/world/events/${eventId}`, 'event'), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: worldKeys.state() }) }, }) } @@ -417,21 +429,10 @@ export function useDeleteTime() { const queryClient = useQueryClient() return useMutation({ - mutationFn: async (timeId: string) => { - const currentState = queryClient.getQueryData(worldKeys.state()) - const newState = { - ...currentState, - times: currentState?.times.filter((t) => t.id !== timeId) ?? [], - relations: currentState?.relations.filter( - (r) => - !(r.sourceType === 'time' && r.sourceId === timeId) && - !(r.targetType === 'time' && r.targetId === timeId) - ) ?? [], - } - return saveWorldState(newState) - }, - onSuccess: (data) => { - queryClient.setQueryData(worldKeys.state(), data) + mutationFn: (timeId: string) => + deleteWorldObject(`/api/world/times/${timeId}`, 'time'), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: worldKeys.state() }) }, }) } @@ -499,16 +500,10 @@ export function useDeleteEntityCollection() { const queryClient = useQueryClient() return useMutation({ - mutationFn: async (collectionId: string) => { - const currentState = queryClient.getQueryData(worldKeys.state()) - const newState = { - ...currentState, - entityCollections: currentState?.entityCollections.filter((c) => c.id !== collectionId) ?? [], - } - return saveWorldState(newState) - }, - onSuccess: (data) => { - queryClient.setQueryData(worldKeys.state(), data) + mutationFn: (collectionId: string) => + deleteWorldObject(`/api/world/entity-collections/${collectionId}`, 'entity collection'), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: worldKeys.state() }) }, }) } @@ -574,16 +569,10 @@ export function useDeleteEventCollection() { const queryClient = useQueryClient() return useMutation({ - mutationFn: async (collectionId: string) => { - const currentState = queryClient.getQueryData(worldKeys.state()) - const newState = { - ...currentState, - eventCollections: currentState?.eventCollections.filter((c) => c.id !== collectionId) ?? [], - } - return saveWorldState(newState) - }, - onSuccess: (data) => { - queryClient.setQueryData(worldKeys.state(), data) + mutationFn: (collectionId: string) => + deleteWorldObject(`/api/world/event-collections/${collectionId}`, 'event collection'), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: worldKeys.state() }) }, }) } @@ -643,16 +632,10 @@ export function useDeleteTimeCollection() { const queryClient = useQueryClient() return useMutation({ - mutationFn: async (collectionId: string) => { - const currentState = queryClient.getQueryData(worldKeys.state()) - const newState = { - ...currentState, - timeCollections: currentState?.timeCollections.filter((c) => c.id !== collectionId) ?? [], - } - return saveWorldState(newState) - }, - onSuccess: (data) => { - queryClient.setQueryData(worldKeys.state(), data) + mutationFn: (collectionId: string) => + deleteWorldObject(`/api/world/time-collections/${collectionId}`, 'time collection'), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: worldKeys.state() }) }, }) } @@ -693,16 +676,10 @@ export function useDeleteRelation() { const queryClient = useQueryClient() return useMutation({ - mutationFn: async (relationId: string) => { - const currentState = queryClient.getQueryData(worldKeys.state()) - const newState = { - ...currentState, - relations: currentState?.relations.filter((r) => r.id !== relationId) ?? [], - } - return saveWorldState(newState) - }, - onSuccess: (data) => { - queryClient.setQueryData(worldKeys.state(), data) + mutationFn: (relationId: string) => + deleteWorldObject(`/api/world/relations/${relationId}`, 'relation'), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: worldKeys.state() }) }, }) } diff --git a/annotation-tool/test/mocks/handlers.ts b/annotation-tool/test/mocks/handlers.ts index 42674f22..d026f4a1 100644 --- a/annotation-tool/test/mocks/handlers.ts +++ b/annotation-tool/test/mocks/handlers.ts @@ -2058,6 +2058,26 @@ export const handlers = [ }) }), + /** + * World collection / relation removals (explicit DELETE endpoints that drop + * the object from its array, replacing the old remove-by-blob-omission PUT). + */ + http.delete('*/api/world/entity-collections/:collectionId', () => { + return HttpResponse.json({ success: true }) + }), + + http.delete('*/api/world/event-collections/:collectionId', () => { + return HttpResponse.json({ success: true }) + }), + + http.delete('*/api/world/time-collections/:collectionId', () => { + return HttpResponse.json({ success: true }) + }), + + http.delete('*/api/world/relations/:relationId', () => { + return HttpResponse.json({ success: true }) + }), + // ============================================================================= // GROUPS ENDPOINTS // ============================================================================= diff --git a/docs/docs/project/changelog.md b/docs/docs/project/changelog.md index c6dfe0cf..17aa7552 100644 --- a/docs/docs/project/changelog.md +++ b/docs/docs/project/changelog.md @@ -11,6 +11,62 @@ All notable changes to the Fovea project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.5.5] - 2026-06-29 + +The 0.5.5 patch is an audit-driven hardening release ([#186](https://github.com/parafovea/fovea/pull/186)). It closes two higher-severity defects — a former group member kept the group's permissions until the server restarted, and concurrent edits to the world graph silently overwrote one another — together with a set of data-scope, idempotency, and cache-staleness gaps. Nothing is breaking; the API additively gains explicit DELETE endpoints for world collections and relations, an optional client-supplied `id` on claim creation, and a `409` on a duplicate video assignment. + +### Fixed + +#### Deleting a Group Clears Its Former Members' Cached Abilities + +- Both group-delete handlers (`server/src/routes/groups.ts`) removed the group without invalidating its former members' in-memory ability cache. A group-scope, non-own-only permission compiles into a globally unconditioned CASL rule, so a former member kept the group's system-wide access until the process restarted or they logged in again. Each handler now snapshots the membership before the cascade and calls `invalidateUserAbilities` for every former member, mirroring `ProjectService.delete`. + +#### Concurrent World-Graph Edits No Longer Overwrite One Another + +- Every world add and update went through a whole-blob read-modify-write `PUT /api/world` with no serialization, so two edits in quick succession — or a Wikidata import racing a manual edit — read the same stale base and the last writer silently dropped the other's entities, events, or relations. Writes are now safe on two fronts: the client (`annotation-tool/src/store/queries/useWorld.ts`) funnels mutations through a single-flight chain that threads the latest server state into the next write, and the server (`server/src/services/world-state-service.ts`, `server/src/repositories/WorldStateRepository.ts`) merges each of the seven object arrays by `id` under an optimistic-concurrency guard rather than replacing them wholesale. Because merge-by-id makes removal-by-omission a no-op, every object removal — entities, events, times, the three collection kinds, and relations — now goes through an explicit DELETE endpoint; the client delete hooks call the graceful `DELETE /api/world/...` routes, which also clean up dependent relations, collection memberships, and gloss references. + +#### Personaless Object Annotations Inherit the Video's Project Scope + +- Object annotations carry no persona, so the create path (`server/src/routes/annotations.ts`) had no project to stamp and persisted them with `projectId = NULL`, invisible to every project reviewer but the creator. When the caller is a member of exactly one project the video is assigned to, the annotation now inherits that project; an ambiguous (multiple) or absent assignment stays personal, and the existing CASL pre-authorization still validates the resolved scope. + +#### Claim Creation Is Idempotent on a Client-Supplied ID + +- Neither claim-create route accepted a client `id`, so a network retry or programmatic resend minted a duplicate claim. Both routes (`server/src/routes/claims.ts`, `server/src/services/claim-service.ts`) now accept an optional `id`; a resend carrying an existing id re-authorizes against the stored row and returns it instead of creating a duplicate, with a `P2002` race fallback. This mirrors the annotation idempotent-create hardening from 0.5.4. + +#### Duplicate Video Assignment Returns 409 Instead of 500 + +- Re-assigning a video already assigned to a project violated the `@@unique([projectId, videoId])` constraint and surfaced as an unhandled `500`. The route (`server/src/routes/video-assignments.ts`) now catches the constraint violation and returns a `409 Conflict`. + +#### The Summary Editor Autosaves Comment-Only Edits + +- The video summary editor's autosave watched only the summary body, so editing the comment without touching the summary never triggered a save and the edit was lost on navigation. Autosave now compares a snapshot that includes the comment (`annotation-tool/src/components/video/VideoSummaryEditor.tsx`). + +#### Relating Claims Refreshes Both Endpoints' Relation Panels + +- Creating or deleting a claim relation invalidated only the source claim's relation query, leaving the target claim's relation panel stale until a manual refetch. Both mutations (`annotation-tool/src/store/queries/useClaims.ts`) now invalidate the target as well. + +#### Membership and Role Changes Refresh the Client Ability Mirror + +- The client ability cache carried a five-minute stale time and no mutation invalidated it, so a user's own role or membership change took up to five minutes to reflect in the UI even though the server always enforced it correctly. The six self-affecting project- and group-membership mutations (`annotation-tool/src/store/queries/useProjects.ts`, `useGroups.ts`) now invalidate the ability mirror on success. + +#### Admin isAdmin Changes Stay in Sync With systemRole + +- The admin user-update endpoint (`server/src/routes/users.ts`) wrote `isAdmin` but never `systemRole`, which is what CASL's `manage all` keys on, so a promotion or demotion left the two fields divergent and the cached abilities stale. The endpoint now sets `systemRole` to match and invalidates the affected user's abilities. + +#### Removed a Dead Auto-Save Hook + +- `useAutoSaveAnnotations` had no callers and re-armed the exact save-loop footgun the live autosave was hardened against; it has been deleted. + +### Added + +#### Explicit DELETE Endpoints for World Collections and Relations + +- `server/src/routes/world.ts` gains `DELETE` routes for entity collections, event collections, time collections, and relations, so these objects are removed explicitly rather than by omission from a whole-blob `PUT`. This is what lets the non-clobbering merge-by-id persistence be safe (see Fixed). + +#### Optional Client-Supplied ID on Claim Creation + +- Both claim-create endpoints accept an optional `id`, enabling idempotent retries; the `Claim` response already echoes it. + ## [0.5.4] - 2026-06-25 The 0.5.4 patch fixes project-scope and ownership stamping on video summaries and claims ([#181](https://github.com/parafovea/fovea/pull/181)). Project collaborators could not see a teammate's summary or add claims under it because summaries were persisted without their persona's project, and model-generated summaries and extracted claims were left unowned. Nothing is breaking; the API additively gains a `projectId` field on summary and claim responses. diff --git a/docs/package.json b/docs/package.json index 3c899092..b1195492 100644 --- a/docs/package.json +++ b/docs/package.json @@ -1,6 +1,6 @@ { "name": "docs", - "version": "0.5.4", + "version": "0.5.5", "private": true, "scripts": { "docusaurus": "docusaurus", diff --git a/model-service/package.json b/model-service/package.json index a61d358e..c19ff71d 100644 --- a/model-service/package.json +++ b/model-service/package.json @@ -1,6 +1,6 @@ { "name": "fovea-model-service", - "version": "0.5.4", + "version": "0.5.5", "description": "AI model inference service for video annotation", "scripts": { "dev": "source venv/bin/activate && uvicorn src.main:app --reload --host 0.0.0.0 --port 8000", diff --git a/model-service/pyproject.toml b/model-service/pyproject.toml index da2057c4..3add2ef2 100644 --- a/model-service/pyproject.toml +++ b/model-service/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "fovea-model-service" -version = "0.5.4" +version = "0.5.5" description = "Model service for fovea video annotation tool" requires-python = ">=3.12" dependencies = [ diff --git a/package.json b/package.json index 1da26ab5..4b1daa84 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "fovea", "private": true, - "version": "0.5.4", + "version": "0.5.5", "description": "FOVEA - Flexible Ontology Visual Event Analyzer", "packageManager": "pnpm@10.15.0", "engines": { diff --git a/server/openapi.json b/server/openapi.json index a8de1fd1..28ed2b48 100644 --- a/server/openapi.json +++ b/server/openapi.json @@ -7582,6 +7582,302 @@ } } }, + "/api/world/entity-collections/{objectId}": { + "delete": { + "tags": [ + "world" + ], + "description": "Delete a world entity collection from the caller's personal world", + "parameters": [ + { + "schema": { + "type": "string" + }, + "in": "path", + "name": "objectId", + "required": true + } + ], + "responses": { + "200": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "success" + ], + "properties": { + "success": { + "type": "boolean" + } + } + } + } + } + }, + "403": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + }, + "404": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + } + } + } + }, + "/api/world/event-collections/{objectId}": { + "delete": { + "tags": [ + "world" + ], + "description": "Delete a world event collection from the caller's personal world", + "parameters": [ + { + "schema": { + "type": "string" + }, + "in": "path", + "name": "objectId", + "required": true + } + ], + "responses": { + "200": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "success" + ], + "properties": { + "success": { + "type": "boolean" + } + } + } + } + } + }, + "403": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + }, + "404": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + } + } + } + }, + "/api/world/time-collections/{objectId}": { + "delete": { + "tags": [ + "world" + ], + "description": "Delete a world time collection from the caller's personal world", + "parameters": [ + { + "schema": { + "type": "string" + }, + "in": "path", + "name": "objectId", + "required": true + } + ], + "responses": { + "200": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "success" + ], + "properties": { + "success": { + "type": "boolean" + } + } + } + } + } + }, + "403": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + }, + "404": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + } + } + } + }, + "/api/world/relations/{objectId}": { + "delete": { + "tags": [ + "world" + ], + "description": "Delete a world relation from the caller's personal world", + "parameters": [ + { + "schema": { + "type": "string" + }, + "in": "path", + "name": "objectId", + "required": true + } + ], + "responses": { + "200": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "success" + ], + "properties": { + "success": { + "type": "boolean" + } + } + } + } + } + }, + "403": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + }, + "404": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error" + ], + "properties": { + "error": { + "type": "string" + } + } + } + } + } + } + } + } + }, "/api/world/events/{eventId}/deletion-preview": { "get": { "tags": [ @@ -10623,6 +10919,10 @@ "text" ], "properties": { + "id": { + "format": "uuid", + "type": "string" + }, "summaryType": { "anyOf": [ { @@ -14234,6 +14534,10 @@ "text" ], "properties": { + "id": { + "format": "uuid", + "type": "string" + }, "text": { "minLength": 1, "type": "string" @@ -23571,6 +23875,33 @@ } } } + }, + "409": { + "description": "Default Response", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "error", + "message" + ], + "properties": { + "error": { + "description": "Machine-readable error code", + "type": "string" + }, + "message": { + "description": "Human-readable error message", + "type": "string" + }, + "details": { + "description": "Additional error context" + } + } + } + } + } } } } diff --git a/server/package.json b/server/package.json index df975bf9..18e7c6f0 100644 --- a/server/package.json +++ b/server/package.json @@ -1,6 +1,6 @@ { "name": "@fovea/server", - "version": "0.5.4", + "version": "0.5.5", "private": true, "type": "module", "scripts": { diff --git a/server/src/repositories/WorldStateRepository.ts b/server/src/repositories/WorldStateRepository.ts index df9481d5..fe928d7f 100644 --- a/server/src/repositories/WorldStateRepository.ts +++ b/server/src/repositories/WorldStateRepository.ts @@ -128,6 +128,43 @@ export class WorldStateRepository { }) } + /** + * Applies an optimistic-concurrency update to a user's personal world state. + * + * Reads the current row, lets `transform` compute the new column values from + * it, then writes them guarded by the row's `updatedAt`. If a concurrent + * writer committed first the guard misses (count 0) and we retry against the + * freshly-read row, so both writes land instead of one silently clobbering + * the other. This is what makes the whole-blob PUT a safe per-id merge under + * concurrent writers (the transform re-runs on fresh state each attempt). + * + * @param userId - owning user ID + * @param transform - computes the Prisma update input from the current row + * @returns the updated world state + * @throws when no personal row exists or the write keeps conflicting + */ + async updatePersonalWorldStateOptimistic( + userId: string, + transform: (current: WorldState) => Prisma.WorldStateUpdateInput, + ): Promise { + for (let attempt = 0; attempt < 5; attempt++) { + const current = await this.prisma.worldState.findFirst({ + where: { userId, projectId: null }, + }) + if (!current) { + throw new Error('No personal world state to update') + } + const result = await this.prisma.worldState.updateMany({ + where: { id: current.id, updatedAt: current.updatedAt }, + data: { ...transform(current), updatedAt: new Date() }, + }) + if (result.count === 1) { + return this.prisma.worldState.findUniqueOrThrow({ where: { id: current.id } }) + } + } + throw new Error('Personal world state update conflicted after retries') + } + /** * Finds all of a user's personas with their ontologies included. * diff --git a/server/src/routes/annotations.ts b/server/src/routes/annotations.ts index 77bf860e..9af24347 100644 --- a/server/src/routes/annotations.ts +++ b/server/src/routes/annotations.ts @@ -256,6 +256,30 @@ const annotationsRoute: FastifyPluginAsync = async (fastify) => { throw new ForbiddenError('Cannot create an annotation under this Persona') } projectId = persona.projectId + } else { + // Personaless object annotations have no persona to inherit scope from. + // Fall back to the video's project so the annotation lands in the project + // its reviewers share — without this it is born projectId = NULL and + // project members (read rule { projectId: { in: [...] } }) cannot see it. + // A video can be assigned to several projects, so disambiguate by the + // caller's membership and only adopt a project when exactly one of the + // caller's projects has this video assigned (zero or ambiguous -> null, + // i.e. personal scope). + const callerProjectIds = ( + await fastify.prisma.projectMembership.findMany({ + where: { userId }, + select: { projectId: true }, + }) + ).map((m) => m.projectId) + if (callerProjectIds.length > 0) { + const assignments = await fastify.prisma.projectVideoAssignment.findMany({ + where: { videoId: data.videoId, projectId: { in: callerProjectIds } }, + select: { projectId: true }, + }) + if (assignments.length === 1) { + projectId = assignments[0].projectId + } + } } const toResponse = (a: Annotation) => ({ diff --git a/server/src/routes/claims.ts b/server/src/routes/claims.ts index 647d9b11..b7638560 100644 --- a/server/src/routes/claims.ts +++ b/server/src/routes/claims.ts @@ -118,6 +118,8 @@ const ClaimSchema: any = Type.Recursive(This => Type.Object({ * Create claim request schema */ const CreateClaimSchema = Type.Object({ + // Optional client-supplied id so a retried create is idempotent (no duplicate). + id: Type.Optional(Type.String({ format: 'uuid' })), summaryType: Type.Union([Type.Literal('video'), Type.Literal('collection')]), text: Type.String({ minLength: 1 }), gloss: Type.Optional(Type.Array(GlossItemSchema)), @@ -752,6 +754,8 @@ const claimsRoute: FastifyPluginAsync = async (fastify) => { personaId: Type.String({ format: 'uuid' }) }), body: Type.Object({ + // Optional client-supplied id so a retried create is idempotent. + id: Type.Optional(Type.String({ format: 'uuid' })), text: Type.String({ minLength: 1 }), gloss: Type.Optional(Type.Array(GlossItemSchema)), parentClaimId: Type.Optional(Type.String({ format: 'uuid' })), diff --git a/server/src/routes/groups.ts b/server/src/routes/groups.ts index 2b88a73a..7041edd7 100644 --- a/server/src/routes/groups.ts +++ b/server/src/routes/groups.ts @@ -463,8 +463,18 @@ const groupsRoute: FastifyPluginAsync = async (fastify) => { throw new NotFoundError('UserGroup', groupId) } + // Snapshot members before the cascade so their cached abilities can be + // cleared afterward. Group-scope non-ownOnly rules are emitted globally + // unconditioned (lib/abilities.ts), so without this a former member keeps + // the group's permissions until restart/re-login. Mirrors + // ProjectService.delete (snapshot ids -> delete -> invalidate each). + const memberRows = await fastify.prisma.groupMembership.findMany({ + where: { groupId }, + select: { userId: true }, + }) // Memberships cascade-delete via onDelete: Cascade in the schema await fastify.prisma.userGroup.delete({ where: { id: groupId } }) + for (const { userId } of memberRows) invalidateUserAbilities(userId) groupOperationCounter.add(1, { operation: 'delete', status: 'success' }) return reply.send({ success: true }) @@ -957,7 +967,14 @@ const groupsRoute: FastifyPluginAsync = async (fastify) => { throw new NotFoundError('UserGroup', groupId) } + // Snapshot members before the cascade so their cached abilities can be + // cleared afterward (see the non-admin delete handler above for why). + const memberRows = await fastify.prisma.groupMembership.findMany({ + where: { groupId }, + select: { userId: true }, + }) await fastify.prisma.userGroup.delete({ where: { id: groupId } }) + for (const { userId } of memberRows) invalidateUserAbilities(userId) groupOperationCounter.add(1, { operation: 'delete', status: 'success' }) return reply.send({ success: true }) diff --git a/server/src/routes/users.ts b/server/src/routes/users.ts index aa32588d..51ae0aae 100644 --- a/server/src/routes/users.ts +++ b/server/src/routes/users.ts @@ -3,6 +3,7 @@ import { FastifyPluginAsync } from 'fastify' import { z } from 'zod' import bcrypt from 'bcrypt' import { requireAuth, requireAdmin } from '../middleware/auth.js' +import { invalidateUserAbilities } from '../middleware/abilities.js' import { NotFoundError, UnauthorizedError, ForbiddenError, ConflictError } from '../lib/errors.js' /** @@ -375,6 +376,7 @@ const usersRoute: FastifyPluginAsync = async (fastify) => { email?: string | null displayName?: string isAdmin?: boolean + systemRole?: string passwordHash?: string } = {} @@ -386,6 +388,10 @@ const usersRoute: FastifyPluginAsync = async (fastify) => { } if (validatedData.isAdmin !== undefined) { updateData.isAdmin = validatedData.isAdmin + // Keep systemRole (which drives CASL `manage all`) in sync with isAdmin + // so the two admin signals cannot diverge: requireAdmin gates on isAdmin + // while CASL super-powers gate on systemRole. + updateData.systemRole = validatedData.isAdmin ? 'system_admin' : 'user' } if (validatedData.password) { updateData.passwordHash = await bcrypt.hash(validatedData.password, 12) @@ -405,6 +411,12 @@ const usersRoute: FastifyPluginAsync = async (fastify) => { updatedAt: true } }) + // The ability cache keys on systemRole; clear this user's cached + // abilities so an admin promotion/demotion takes effect immediately + // rather than lingering until restart/re-login. + if (validatedData.isAdmin !== undefined) { + invalidateUserAbilities(userId) + } return reply.send(user) } catch (error: unknown) { if (error && typeof error === 'object' && 'code' in error && error.code === 'P2025') { diff --git a/server/src/routes/video-assignments.ts b/server/src/routes/video-assignments.ts index 138cb8c6..7f7983dc 100644 --- a/server/src/routes/video-assignments.ts +++ b/server/src/routes/video-assignments.ts @@ -9,7 +9,8 @@ import { Type, Static } from '@sinclair/typebox' import { FastifyPluginAsync } from 'fastify' -import { NotFoundError, ValidationError, ForbiddenError } from '../lib/errors.js' +import { Prisma } from '@prisma/client' +import { NotFoundError, ValidationError, ForbiddenError, ConflictError, ErrorResponseSchema } from '../lib/errors.js' import { videoAssignmentCounter } from '../metrics.js' import { requireAuth, requireAdmin } from '../middleware/auth.js' import { buildAbilities } from '../middleware/abilities.js' @@ -285,6 +286,7 @@ const videoAssignmentsRoute: FastifyPluginAsync = async (fastify) => { body: AssignVideoBody, response: { 201: AssignmentResponseSchema, + 409: ErrorResponseSchema, }, }, }, @@ -334,16 +336,26 @@ const videoAssignmentsRoute: FastifyPluginAsync = async (fastify) => { } } - // Create assignment (unique constraint will prevent duplicates) - const assignment = await fastify.prisma.projectVideoAssignment.create({ - data: { - projectId, - videoId, - assignedUserId: assignedUserId ?? null, - source: 'manual', - assignedBy: request.user!.id, - }, - }) + // Create assignment. The @@unique([projectId, videoId]) raises P2002 on a + // duplicate; surface it as a 409 rather than letting it become an + // unhandled 500 (re-assigning an already-assigned video is a user error, + // not a server fault). + const assignment = await fastify.prisma.projectVideoAssignment + .create({ + data: { + projectId, + videoId, + assignedUserId: assignedUserId ?? null, + source: 'manual', + assignedBy: request.user!.id, + }, + }) + .catch((err) => { + if (err instanceof Prisma.PrismaClientKnownRequestError && err.code === 'P2002') { + throw new ConflictError('This video is already assigned to the project') + } + throw err + }) videoAssignmentCounter.add(1, { operation: 'assign', source: 'manual' }) return reply.status(201).send(assignment) diff --git a/server/src/routes/world.ts b/server/src/routes/world.ts index ed76990a..60253e02 100644 --- a/server/src/routes/world.ts +++ b/server/src/routes/world.ts @@ -232,6 +232,41 @@ const worldRoute: FastifyPluginAsync = async (fastify) => { } ) + // Explicit single-object removal for the collection and relation arrays. + // Unlike entities/events/times (above), these have no graceful-delete route + // and used to be removed by omission through the whole-blob PUT. Now that the + // PUT merges by id, removal must be explicit so the merge cannot resurrect a + // deleted object. + const objectDeleteRoutes = [ + { path: '/api/world/entity-collections/:objectId', field: 'entityCollections' as const, desc: 'entity collection' }, + { path: '/api/world/event-collections/:objectId', field: 'eventCollections' as const, desc: 'event collection' }, + { path: '/api/world/time-collections/:objectId', field: 'timeCollections' as const, desc: 'time collection' }, + { path: '/api/world/relations/:objectId', field: 'relations' as const, desc: 'relation' }, + ] + for (const { path, field, desc } of objectDeleteRoutes) { + fastify.delete<{ Params: { objectId: string } }>( + path, + { + onRequest: [requireAuth, buildAbilities], + schema: { + description: `Delete a world ${desc} from the caller's personal world`, + tags: ['world'], + params: Type.Object({ objectId: Type.String() }), + response: { + 200: Type.Object({ success: Type.Boolean() }), + 403: Type.Object({ error: Type.String() }), + 404: Type.Object({ error: Type.String() }), + }, + }, + }, + async (request, reply) => { + const service = serviceFor(request) + await service.removeWorldObject(field, request.params.objectId) + return reply.send({ success: true }) + }, + ) + } + /** * Get deletion preview for a world event. * diff --git a/server/src/services/claim-service.ts b/server/src/services/claim-service.ts index 762a09fa..dd8a67a5 100644 --- a/server/src/services/claim-service.ts +++ b/server/src/services/claim-service.ts @@ -67,6 +67,8 @@ export type MetadataModality = ('text' | 'non-text')[] /** Validated fields for creating a claim under a summary. */ export interface CreateClaimInput { + /** Optional client-supplied id; makes create idempotent on retry. */ + id?: string summaryType: 'video' | 'collection' text: string gloss?: GlossItemInput[] @@ -133,6 +135,8 @@ export interface ClaimSynthesisConfigInput { /** Validated fields for creating a claim from a video + persona pair. */ export interface CreateVideoPersonaClaimInput { + /** Optional client-supplied id; makes create idempotent on retry. */ + id?: string text: string gloss?: GlossItemInput[] parentClaimId?: string @@ -389,7 +393,7 @@ export class ClaimService { * @throws {ValidationError} when the parent claim is invalid */ async createClaim(summaryId: string, input: CreateClaimInput): Promise { - const { text, gloss, parentClaimId, summaryType, audio, video, metadata, comment, ...rest } = input + const { id, text, gloss, parentClaimId, summaryType, audio, video, metadata, comment, ...rest } = input const ability = this.requireAbility() const userId = this.userId! @@ -434,9 +438,24 @@ export class ClaimService { } } + // Idempotent create on a client-supplied id: a network retry / resend + // carrying the same id must not mint a duplicate. If the claim already + // exists, re-authorize against it (so a caller cannot hijack another + // user's claim by supplying its id) and return the current tree. + if (id) { + const existing = await this.repository.findClaimById(id) + if (existing) { + if (!ability.can('update', subject('Claim', existing))) { + throw new ForbiddenError('Cannot create this Claim') + } + return this.repository.findClaimTree(summaryId, summaryType) + } + } + // Convert null JSON fields to Prisma.JsonNull. Never trust request body // for createdBy; always stamp from authenticated session. const claimData: Prisma.ClaimUncheckedCreateInput = { + id: id || undefined, summaryId, summaryType, text, @@ -452,8 +471,19 @@ export class ClaimService { createdBy: userId, } - // Create claim - await this.repository.createClaim(claimData) + // Create claim. On a concurrent same-id insert (P2002), collapse to the + // idempotent path instead of surfacing a 500. + try { + await this.repository.createClaim(claimData) + } catch (err) { + if (id && err instanceof Prisma.PrismaClientKnownRequestError && err.code === 'P2002') { + const existing = await this.repository.findClaimById(id) + if (existing && ability.can('update', subject('Claim', existing))) { + return this.repository.findClaimTree(summaryId, summaryType) + } + } + throw err + } // Update denormalized claimsJson await this.updateSummaryClaimsJson(summaryId, summaryType) @@ -906,7 +936,7 @@ export class ClaimService { personaId: string, input: CreateVideoPersonaClaimInput ): Promise { - const { text, gloss, parentClaimId, audio, video: videoModality, metadata, comment, ...rest } = input + const { id, text, gloss, parentClaimId, audio, video: videoModality, metadata, comment, ...rest } = input const ability = this.requireAbility() const userId = this.userId! @@ -932,6 +962,20 @@ export class ClaimService { throw new ForbiddenError('Cannot create this Claim') } + // Idempotent create on a client-supplied id: a retry carrying the same id + // must not mint a duplicate. Re-authorize against the existing row (so a + // caller can't hijack another user's claim by supplying its id) and return + // it, skipping the summary upsert. + if (id) { + const existing = await this.repository.findClaimById(id) + if (existing) { + if (!ability.can('update', subject('Claim', existing))) { + throw new ForbiddenError('Cannot create this Claim') + } + return { claim: existing, summaryId: existing.summaryId } + } + } + // If an existing summary is present, the caller must also be able to // update it (we attach claims to it and auto-create it if missing). const existingSummary = await this.repository.findVideoSummaryByVideoPersona(videoId, personaId) @@ -965,6 +1009,7 @@ export class ClaimService { // Convert null JSON fields to Prisma.JsonNull. Never trust request // body for createdBy; always stamp from the authenticated session. const claimData: Prisma.ClaimUncheckedCreateInput = { + id: id || undefined, summaryId: summary.id, summaryType: 'video', text, @@ -980,8 +1025,20 @@ export class ClaimService { createdBy: userId, } - // Create claim - const claim = await this.repository.createClaim(claimData) + // Create claim. On a concurrent same-id insert (P2002), collapse to the + // idempotent path instead of surfacing a 500. + let claim + try { + claim = await this.repository.createClaim(claimData) + } catch (err) { + if (id && err instanceof Prisma.PrismaClientKnownRequestError && err.code === 'P2002') { + const existing = await this.repository.findClaimById(id) + if (existing && ability.can('update', subject('Claim', existing))) { + return { claim: existing, summaryId: existing.summaryId } + } + } + throw err + } return { claim, diff --git a/server/src/services/world-state-service.ts b/server/src/services/world-state-service.ts index 35d90325..d9099be2 100644 --- a/server/src/services/world-state-service.ts +++ b/server/src/services/world-state-service.ts @@ -29,6 +29,28 @@ function toJson(value: unknown): Prisma.InputJsonValue { return JSON.parse(JSON.stringify(value)) } +/** + * Merge an incoming array of `{ id }` objects into an existing one by id: + * existing items keep their position, a matching id is overwritten, and new + * ids are appended. This turns the whole-blob PUT into an upsert so a writer + * carrying a stale view (it never saw a concurrently-added item) no longer + * drops it — the merge re-runs against the freshly-read row via optimistic + * concurrency. Removals go through the explicit DELETE routes, never omission. + */ +function mergeById(existing: Prisma.JsonValue | null | undefined, incoming: unknown[]): Prisma.InputJsonValue { + const byId = new Map() + const order: string[] = [] + const add = (item: unknown) => { + const id = (item as { id?: string } | null)?.id + if (!id) return + if (!byId.has(id)) order.push(id) + byId.set(id, item) + } + if (Array.isArray(existing)) existing.forEach(add) + incoming.forEach(add) + return order.map((id) => byId.get(id)) as Prisma.InputJsonValue +} + /** * Partial world state update fields. All fields are optional; only provided * fields are written. @@ -238,15 +260,18 @@ export class WorldStateService { if (this.ability && !this.ability.can('update', subject('WorldState', existing))) { throw new ForbiddenError('Cannot update this WorldState') } - worldState = await this.repository.updateWorldState(existing.id, { - entities: input.entities !== undefined ? toJson(input.entities) : undefined, - events: input.events !== undefined ? toJson(input.events) : undefined, - times: input.times !== undefined ? toJson(input.times) : undefined, - entityCollections: input.entityCollections !== undefined ? toJson(input.entityCollections) : undefined, - eventCollections: input.eventCollections !== undefined ? toJson(input.eventCollections) : undefined, - timeCollections: input.timeCollections !== undefined ? toJson(input.timeCollections) : undefined, - relations: input.relations !== undefined ? toJson(input.relations) : undefined - }) + // Merge each provided array by id (upsert) instead of replacing it, under + // optimistic concurrency, so concurrent writers (rapid edits, a Wikidata + // import, or a second tab) cannot clobber each other's additions. + worldState = await this.repository.updatePersonalWorldStateOptimistic(userId, (current) => ({ + entities: input.entities !== undefined ? mergeById(current.entities, input.entities) : undefined, + events: input.events !== undefined ? mergeById(current.events, input.events) : undefined, + times: input.times !== undefined ? mergeById(current.times, input.times) : undefined, + entityCollections: input.entityCollections !== undefined ? mergeById(current.entityCollections, input.entityCollections) : undefined, + eventCollections: input.eventCollections !== undefined ? mergeById(current.eventCollections, input.eventCollections) : undefined, + timeCollections: input.timeCollections !== undefined ? mergeById(current.timeCollections, input.timeCollections) : undefined, + relations: input.relations !== undefined ? mergeById(current.relations, input.relations) : undefined, + })) } else { if (this.ability) { const candidate = subject('WorldState', { userId, projectId: null }) @@ -269,6 +294,37 @@ export class WorldStateService { return this.mapResponse(worldState) } + /** + * Removes a single object (by id) from one of the personal world state's + * collection or relation arrays. These types have no graceful-delete route of + * their own and previously relied on removal-by-omission through the whole- + * blob PUT; now that the PUT merges by id, removal must be explicit so a merge + * cannot resurrect a deleted object. Uses optimistic concurrency so it is + * safe against concurrent writers. + * + * @param field - the array to remove from + * @param objectId - the id of the object to remove + * @throws {NotFoundError} when the user has no personal world state + * @throws {ForbiddenError} when update access is denied + */ + async removeWorldObject( + field: 'entityCollections' | 'eventCollections' | 'timeCollections' | 'relations', + objectId: string, + ): Promise { + const userId = await this.resolveUserId() + const existing = await this.repository.findPersonalWorldState(userId) + if (!existing) { + throw new NotFoundError('WorldState', userId) + } + if (this.ability && !this.ability.can('update', subject('WorldState', existing))) { + throw new ForbiddenError('Cannot update this WorldState') + } + await this.repository.updatePersonalWorldStateOptimistic(userId, (current) => { + const arr = Array.isArray(current[field]) ? (current[field] as Array<{ id?: string }>) : [] + return { [field]: toJson(arr.filter((o) => o?.id !== objectId)) } + }) + } + /** * Clears a specific user's personal world state by overwriting it with empty * arrays, creating an empty row if none exists. Used by the admin endpoint; diff --git a/server/test/integration/annotation-personaless-scope.test.ts b/server/test/integration/annotation-personaless-scope.test.ts new file mode 100644 index 00000000..ac6a23f5 --- /dev/null +++ b/server/test/integration/annotation-personaless-scope.test.ts @@ -0,0 +1,83 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest' +import { buildApp } from '../../src/app.js' +import { FastifyInstance } from 'fastify' +import { PrismaClient } from '@prisma/client' +import { seedBaselinePermissions, createRegularTestUser } from '../helpers/rbac-test-setup.js' + +/** + * A personaless object annotation has no persona to inherit project scope from. + * It should fall back to the video's project (scoped to the caller's + * membership), so project reviewers can see it; without that it is born + * projectId = NULL and is invisible to project-scoped reads. + */ +describe('Personaless annotation project scope', () => { + let app: FastifyInstance + let prisma: PrismaClient + let session: string + let userId: string + let videoId: string + let projectId: string + + const frames = { + boxes: [{ x: 10, y: 20, width: 100, height: 80, frameNumber: 0, isKeyframe: true }], + interpolationSegments: [], + visibilityRanges: [{ startFrame: 0, endFrame: 0, visible: true }], + totalFrames: 1, + keyframeCount: 1, + interpolatedFrameCount: 0, + } + + beforeAll(async () => { + app = await buildApp() + prisma = app.prisma + }) + afterAll(async () => { + await app.close() + }) + + beforeEach(async () => { + await prisma.loginAttempt.deleteMany() + await prisma.annotation.deleteMany() + await prisma.projectVideoAssignment.deleteMany() + await prisma.projectMembership.deleteMany() + await prisma.project.deleteMany() + await prisma.video.deleteMany() + await prisma.session.deleteMany() + await prisma.rolePermission.deleteMany() + await prisma.user.deleteMany() + await seedBaselinePermissions(prisma) + const user = await createRegularTestUser(prisma, { username: 'annotator', email: 'ann@example.com' }) + session = user.sessionToken + userId = user.id + projectId = (await prisma.project.create({ + data: { name: 'Ann Project', slug: 'ann-project', createdBy: userId, ownerUserId: userId }, + })).id + videoId = (await prisma.video.create({ data: { filename: 'a.mp4', path: '/v/a.mp4', duration: 60 } })).id + }) + + const postObjectAnnotation = () => + app.inject({ + method: 'POST', + url: '/api/annotations', + cookies: { session_token: session }, + payload: { videoId, type: 'object', label: 'box', frames }, + }) + + it('inherits the project when the caller is a member of the one project the video is assigned to', async () => { + await prisma.projectVideoAssignment.create({ data: { projectId, videoId, assignedBy: userId } }) + await prisma.projectMembership.create({ data: { userId, projectId, role: 'annotator' } }) + + expect((await postObjectAnnotation()).statusCode).toBe(201) + const ann = await prisma.annotation.findFirst({ where: { videoId, createdByUserId: userId } }) + expect(ann?.projectId).toBe(projectId) + }) + + it('stays null when the video is assigned to a project the caller is not a member of', async () => { + await prisma.projectVideoAssignment.create({ data: { projectId, videoId, assignedBy: userId } }) + // No membership for the caller in that project. + + expect((await postObjectAnnotation()).statusCode).toBe(201) + const ann = await prisma.annotation.findFirst({ where: { videoId, createdByUserId: userId } }) + expect(ann?.projectId).toBeNull() + }) +}) diff --git a/server/test/integration/claim-idempotency.test.ts b/server/test/integration/claim-idempotency.test.ts new file mode 100644 index 00000000..cdd147e1 --- /dev/null +++ b/server/test/integration/claim-idempotency.test.ts @@ -0,0 +1,84 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest' +import { buildApp } from '../../src/app.js' +import { FastifyInstance } from 'fastify' +import { PrismaClient } from '@prisma/client' +import { seedBaselinePermissions, createRegularTestUser } from '../helpers/rbac-test-setup.js' + +/** + * A network retry / programmatic resend of a claim-create carrying the same + * client-supplied id must not mint a duplicate claim row (it returns the + * existing one). Mirrors the annotation idempotent-create hardening. + */ +describe('Claim creation idempotency', () => { + let app: FastifyInstance + let prisma: PrismaClient + let session: string + let summaryId: string + let videoId: string + let personaId: string + + beforeAll(async () => { + app = await buildApp() + prisma = app.prisma + }) + afterAll(async () => { + await app.close() + }) + + beforeEach(async () => { + await prisma.loginAttempt.deleteMany() + await prisma.claim.deleteMany() + await prisma.videoSummary.deleteMany() + await prisma.persona.deleteMany() + await prisma.video.deleteMany() + await prisma.session.deleteMany() + await prisma.rolePermission.deleteMany() + await prisma.user.deleteMany() + await seedBaselinePermissions(prisma) + const user = await createRegularTestUser(prisma, { username: 'claimer', email: 'claimer@example.com' }) + session = user.sessionToken + const persona = await prisma.persona.create({ + data: { userId: user.id, name: 'P', role: 'Analyst', informationNeed: 'n' }, + }) + personaId = persona.id + videoId = (await prisma.video.create({ data: { filename: 'a.mp4', path: '/v/a.mp4', duration: 60 } })).id + summaryId = (await prisma.videoSummary.create({ + data: { videoId, personaId, summary: [], createdBy: user.id }, + })).id + }) + + it('re-POST with the same id returns the existing claim and creates no duplicate', async () => { + const id = '11111111-1111-1111-1111-111111111111' + const post = () => + app.inject({ + method: 'POST', + url: `/api/summaries/${summaryId}/claims`, + cookies: { session_token: session }, + payload: { id, summaryType: 'video', text: 'The car is red.', audio: ['speech'] }, + }) + + expect((await post()).statusCode).toBe(201) + expect((await post()).statusCode).toBe(201) + + const rows = await prisma.claim.findMany({ where: { summaryId } }) + expect(rows).toHaveLength(1) + expect(rows[0].id).toBe(id) + }) + + it('the video-persona claim route is likewise idempotent on id', async () => { + const id = '22222222-2222-2222-2222-222222222222' + const post = () => + app.inject({ + method: 'POST', + url: `/api/videos/${videoId}/personas/${personaId}/claims`, + cookies: { session_token: session }, + payload: { id, text: 'A claim.' }, + }) + + expect((await post()).statusCode).toBe(201) + expect((await post()).statusCode).toBe(201) + + const rows = await prisma.claim.findMany({ where: { id } }) + expect(rows).toHaveLength(1) + }) +}) diff --git a/server/test/integration/group-delete-invalidation.test.ts b/server/test/integration/group-delete-invalidation.test.ts new file mode 100644 index 00000000..33b92122 --- /dev/null +++ b/server/test/integration/group-delete-invalidation.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest' +import { buildApp } from '../../src/app.js' +import { FastifyInstance } from 'fastify' +import { PrismaClient } from '@prisma/client' +import { seedBaselinePermissions, createAdminTestUser, createRegularTestUser } from '../helpers/rbac-test-setup.js' + +/** + * Deleting a group must clear its former members' cached abilities. Group-scope + * non-ownOnly rules are emitted globally unconditioned, so without invalidation + * a former member keeps the group's permissions until restart/re-login. We + * detect that through GET /api/auth/abilities (which warms and reads the cache). + * + * Against the unfixed code the post-delete read still shows the group-granted + * rule (stale cache). + */ +describe('Group deletion invalidates former members abilities', () => { + let app: FastifyInstance + let prisma: PrismaClient + let adminSession: string + let memberSession: string + let groupId: string + + beforeAll(async () => { + app = await buildApp() + prisma = app.prisma + }) + afterAll(async () => { + await app.close() + }) + + beforeEach(async () => { + await prisma.loginAttempt.deleteMany() + await prisma.groupMembership.deleteMany() + await prisma.userGroup.deleteMany() + await prisma.session.deleteMany() + await prisma.rolePermission.deleteMany() + await prisma.user.deleteMany() + await seedBaselinePermissions(prisma) + // A group-scope, non-ownOnly permission emits a globally-unconditioned rule, + // distinguishable from the own-only baseline video-delete rule. + await prisma.rolePermission.create({ + data: { scope: 'group', role: 'group_admin', resourceType: 'video', action: 'delete', ownOnly: false }, + }) + const admin = await createAdminTestUser(prisma, { username: 'groupadmin', email: 'ga@example.com' }) + adminSession = admin.sessionToken + const member = await createRegularTestUser(prisma, { username: 'groupmember', email: 'gm@example.com' }) + memberSession = member.sessionToken + const group = await prisma.userGroup.create({ data: { name: 'G', slug: 'g', createdBy: admin.id } }) + groupId = group.id + await prisma.groupMembership.create({ data: { userId: member.id, groupId, role: 'group_admin' } }) + }) + + const memberRules = async () => { + const res = await app.inject({ + method: 'GET', + url: '/api/auth/abilities', + cookies: { session_token: memberSession }, + }) + expect(res.statusCode).toBe(200) + return res.json().rules as Array<{ action?: string; subject?: string; conditions?: unknown }> + } + + // The group-granted rule is an unconditioned delete on Video (the own-only + // baseline rule carries a createdByUserId condition). + const hasGroupGrantedVideoDelete = (rules: Awaited>) => + rules.some((r) => r.action === 'delete' && r.subject === 'Video' && r.conditions == null) + + it('a former member loses the group-granted ability immediately after group delete', async () => { + // Warm the cache: the member has the group-granted global video-delete rule. + expect(hasGroupGrantedVideoDelete(await memberRules())).toBe(true) + + const del = await app.inject({ + method: 'DELETE', + url: `/api/admin/groups/${groupId}`, + cookies: { session_token: adminSession }, + }) + expect(del.statusCode).toBe(200) + + // The member's abilities are rebuilt without the group rule. + expect(hasGroupGrantedVideoDelete(await memberRules())).toBe(false) + }) +}) diff --git a/server/test/integration/world-merge-delete.test.ts b/server/test/integration/world-merge-delete.test.ts new file mode 100644 index 00000000..1c66eb32 --- /dev/null +++ b/server/test/integration/world-merge-delete.test.ts @@ -0,0 +1,94 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest' +import { buildApp } from '../../src/app.js' +import { FastifyInstance } from 'fastify' +import { PrismaClient } from '@prisma/client' +import { seedBaselinePermissions, createRegularTestUser } from '../helpers/rbac-test-setup.js' + +/** + * World-state writes must merge by id, not replace: a PUT carrying a stale view + * (e.g. a rapid second edit or a Wikidata import that never saw a concurrent + * add) must not drop previously-added objects. Removal is therefore explicit — + * the new per-object DELETE routes — since omission no longer removes. + * + * Against the unfixed (whole-array replace) code the merge assertions fail: + * the second PUT would drop the first entity. + */ +describe('World-state merge-by-id and explicit deletes', () => { + let app: FastifyInstance + let prisma: PrismaClient + let session: string + + beforeAll(async () => { + app = await buildApp() + prisma = app.prisma + }) + afterAll(async () => { + await app.close() + }) + + beforeEach(async () => { + await prisma.loginAttempt.deleteMany() + await prisma.worldState.deleteMany() + await prisma.session.deleteMany() + await prisma.rolePermission.deleteMany() + await prisma.user.deleteMany() + await seedBaselinePermissions(prisma) + const user = await createRegularTestUser(prisma, { username: 'worlduser', email: 'world@example.com' }) + session = user.sessionToken + }) + + const putWorld = (body: object) => + app.inject({ method: 'PUT', url: '/api/world', cookies: { session_token: session }, payload: body }) + + const getWorld = () => + app.inject({ method: 'GET', url: '/api/world', cookies: { session_token: session } }) + + it('merges entities by id across sequential partial PUTs (no last-writer-wins)', async () => { + expect((await putWorld({ entities: [{ id: 'e1', name: 'First' }] })).statusCode).toBe(200) + // A second PUT that only carries e2 (a stale client that never saw e1) must + // not drop e1 — the server merges by id. + expect((await putWorld({ entities: [{ id: 'e2', name: 'Second' }] })).statusCode).toBe(200) + + const entities = (await getWorld()).json().entities as Array<{ id: string }> + expect(entities.map((e) => e.id).sort()).toEqual(['e1', 'e2']) + }) + + it('updates an existing entity in place on merge (same id overwrites)', async () => { + await putWorld({ entities: [{ id: 'e1', name: 'Original' }] }) + await putWorld({ entities: [{ id: 'e1', name: 'Renamed' }] }) + const entities = (await getWorld()).json().entities as Array<{ id: string; name: string }> + expect(entities).toHaveLength(1) + expect(entities[0].name).toBe('Renamed') + }) + + it('removes a relation via DELETE (and a merge PUT does not resurrect it)', async () => { + await putWorld({ relations: [{ id: 'r1', type: 'related' }, { id: 'r2', type: 'related' }] }) + + // A PUT omitting r1 must NOT remove it (merge keeps it)... + await putWorld({ relations: [{ id: 'r2', type: 'related' }] }) + let relations = (await getWorld()).json().relations as Array<{ id: string }> + expect(relations.map((r) => r.id).sort()).toEqual(['r1', 'r2']) + + // ...explicit DELETE removes it. + const del = await app.inject({ + method: 'DELETE', + url: '/api/world/relations/r1', + cookies: { session_token: session }, + }) + expect(del.statusCode).toBe(200) + relations = (await getWorld()).json().relations as Array<{ id: string }> + expect(relations.map((r) => r.id)).toEqual(['r2']) + }) + + it('removes an entity collection via DELETE', async () => { + await putWorld({ entityCollections: [{ id: 'c1', name: 'Coll' }] }) + const del = await app.inject({ + method: 'DELETE', + url: '/api/world/entity-collections/c1', + cookies: { session_token: session }, + }) + expect(del.statusCode).toBe(200) + const collections = (await getWorld()).json().entityCollections as Array<{ id: string }> + expect(collections).toHaveLength(0) + }) +}) diff --git a/server/test/routes/admin-user-systemrole.test.ts b/server/test/routes/admin-user-systemrole.test.ts new file mode 100644 index 00000000..9f5a17ce --- /dev/null +++ b/server/test/routes/admin-user-systemrole.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest' +import { buildApp } from '../../src/app.js' +import { FastifyInstance } from 'fastify' +import { PrismaClient } from '@prisma/client' +import { seedBaselinePermissions, createAdminTestUser, createRegularTestUser } from '../helpers/rbac-test-setup.js' + +/** + * The admin user-update endpoint writes `isAdmin`, but CASL `manage all` keys on + * `systemRole`. They must stay in sync, otherwise an "admin" passes requireAdmin + * yet has no CASL super-powers (or a demotion leaves stale super-powers). + */ +describe('Admin user update keeps systemRole in sync with isAdmin', () => { + let app: FastifyInstance + let prisma: PrismaClient + let adminSession: string + + beforeAll(async () => { + app = await buildApp() + prisma = app.prisma + }) + afterAll(async () => { + await app.close() + }) + + beforeEach(async () => { + await prisma.loginAttempt.deleteMany() + await prisma.session.deleteMany() + await prisma.rolePermission.deleteMany() + await prisma.user.deleteMany() + await seedBaselinePermissions(prisma) + const admin = await createAdminTestUser(prisma, { username: 'superadmin', email: 'super@example.com' }) + adminSession = admin.sessionToken + }) + + const updateUser = (userId: string, body: object) => + app.inject({ + method: 'PUT', + url: `/api/admin/users/${userId}`, + cookies: { session_token: adminSession }, + payload: body, + }) + + it('promoting to isAdmin sets systemRole to system_admin', async () => { + const target = await createRegularTestUser(prisma, { username: 'promotee', email: 'promo@example.com' }) + expect((await updateUser(target.id, { isAdmin: true })).statusCode).toBe(200) + const row = await prisma.user.findUnique({ where: { id: target.id } }) + expect(row?.isAdmin).toBe(true) + expect(row?.systemRole).toBe('system_admin') + }) + + it('demoting from isAdmin resets systemRole to user', async () => { + const target = await createRegularTestUser(prisma, { username: 'demotee', email: 'demo@example.com' }) + await prisma.user.update({ where: { id: target.id }, data: { isAdmin: true, systemRole: 'system_admin' } }) + expect((await updateUser(target.id, { isAdmin: false })).statusCode).toBe(200) + const row = await prisma.user.findUnique({ where: { id: target.id } }) + expect(row?.isAdmin).toBe(false) + expect(row?.systemRole).toBe('user') + }) +}) diff --git a/server/test/routes/video-assignment-conflict.test.ts b/server/test/routes/video-assignment-conflict.test.ts new file mode 100644 index 00000000..b62e63a2 --- /dev/null +++ b/server/test/routes/video-assignment-conflict.test.ts @@ -0,0 +1,58 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest' +import { buildApp } from '../../src/app.js' +import { FastifyInstance } from 'fastify' +import { PrismaClient } from '@prisma/client' +import { seedBaselinePermissions, createAdminTestUser } from '../helpers/rbac-test-setup.js' + +/** + * Re-assigning an already-assigned video hits the @@unique([projectId, videoId]) + * constraint. That must surface as a 409 Conflict, not an unhandled 500. + */ +describe('Duplicate video assignment', () => { + let app: FastifyInstance + let prisma: PrismaClient + let session: string + let projectId: string + let videoId: string + + beforeAll(async () => { + app = await buildApp() + prisma = app.prisma + }) + afterAll(async () => { + await app.close() + }) + + beforeEach(async () => { + await prisma.loginAttempt.deleteMany() + await prisma.projectVideoAssignment.deleteMany() + await prisma.projectMembership.deleteMany() + await prisma.project.deleteMany() + await prisma.video.deleteMany() + await prisma.session.deleteMany() + await prisma.rolePermission.deleteMany() + await prisma.user.deleteMany() + await seedBaselinePermissions(prisma) + const admin = await createAdminTestUser(prisma, { username: 'assignadmin', email: 'assign@example.com' }) + session = admin.sessionToken + projectId = (await prisma.project.create({ + data: { name: 'Assign Project', slug: 'assign-project', createdBy: admin.id, ownerUserId: admin.id }, + })).id + videoId = (await prisma.video.create({ data: { filename: 'a.mp4', path: '/v/a.mp4', duration: 60 } })).id + }) + + it('returns 409 (not 500) when the video is already assigned', async () => { + const assign = () => + app.inject({ + method: 'POST', + url: `/api/projects/${projectId}/videos`, + cookies: { session_token: session }, + payload: { videoId }, + }) + + expect((await assign()).statusCode).toBe(201) + const second = await assign() + expect(second.statusCode).toBe(409) + expect(second.json().error).toBe('CONFLICT') + }) +}) diff --git a/server/test/routes/world.test.ts b/server/test/routes/world.test.ts index 137f6e70..1686dffc 100644 --- a/server/test/routes/world.test.ts +++ b/server/test/routes/world.test.ts @@ -289,7 +289,7 @@ describe('World State API', () => { expect(worldState.entities[1].name).toBe('New Entity') }) - it('allows partial updates (only entities)', async () => { + it('merges the provided field by id and leaves other fields unchanged', async () => { // Create initial world state await prisma.worldState.create({ data: { @@ -304,7 +304,7 @@ describe('World State API', () => { } }) - // Update only entities + // Update only entities, adding a new one const updateData = { entities: [{ id: 'entity-2', name: 'Entity 2' }] } @@ -318,9 +318,12 @@ describe('World State API', () => { expect(response.statusCode).toBe(200) const worldState = response.json() - expect(worldState.entities).toHaveLength(1) - expect(worldState.entities[0].name).toBe('Entity 2') - // Events should remain unchanged + // Entities merge by id: the pre-existing entity-1 is kept and entity-2 is + // added (the PUT no longer replaces the whole array). Removal is done via + // the per-object DELETE routes, not by omission. + expect(worldState.entities).toHaveLength(2) + expect(worldState.entities.map((e: { id: string }) => e.id).sort()).toEqual(['entity-1', 'entity-2']) + // Events (a field not provided in the PUT) remain unchanged. expect(worldState.events).toHaveLength(1) expect(worldState.events[0].name).toBe('Event 1') })