From 3a758393bfca2ffcdc5d1f373fe66cb4dfb45129 Mon Sep 17 00:00:00 2001 From: Marcel Date: Wed, 27 May 2026 14:14:00 +0200 Subject: [PATCH] refactor(shared): extract hasWriteAll(locals) permission helper The locals.user.groups.some(...WRITE_ALL) derivation was copy-pasted across the persons directory, persons review and the two document loaders touched by this PR. Extract a single tested hasWriteAll(locals) helper in $lib/shared/server and reuse it, removing the ad-hoc casts. Refs #667 Co-Authored-By: Claude Opus 4.7 --- .../src/lib/shared/server/permissions.spec.ts | 30 +++++++++++++++++++ frontend/src/lib/shared/server/permissions.ts | 14 +++++++++ .../documents/[id]/edit/+page.server.ts | 7 ++--- .../src/routes/documents/new/+page.server.ts | 7 ++--- frontend/src/routes/persons/+page.server.ts | 6 ++-- .../src/routes/persons/review/+page.server.ts | 6 ++-- 6 files changed, 52 insertions(+), 18 deletions(-) create mode 100644 frontend/src/lib/shared/server/permissions.spec.ts create mode 100644 frontend/src/lib/shared/server/permissions.ts diff --git a/frontend/src/lib/shared/server/permissions.spec.ts b/frontend/src/lib/shared/server/permissions.spec.ts new file mode 100644 index 00000000..4e93b11f --- /dev/null +++ b/frontend/src/lib/shared/server/permissions.spec.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest'; +import { hasWriteAll } from './permissions'; + +type Locals = { user?: { groups?: { permissions: string[] }[] } }; + +const localsWith = (permissions: string[][]): Locals => ({ + user: { groups: permissions.map((p) => ({ permissions: p })) } +}); + +describe('hasWriteAll', () => { + it('returns true when a group grants WRITE_ALL', () => { + expect(hasWriteAll(localsWith([['READ_ALL', 'WRITE_ALL']]))).toBe(true); + }); + + it('returns true when WRITE_ALL is in any of several groups', () => { + expect(hasWriteAll(localsWith([['READ_ALL'], ['WRITE_ALL']]))).toBe(true); + }); + + it('returns false when no group grants WRITE_ALL', () => { + expect(hasWriteAll(localsWith([['READ_ALL'], ['ANNOTATE_ALL']]))).toBe(false); + }); + + it('returns false for an anonymous user (no locals.user)', () => { + expect(hasWriteAll({})).toBe(false); + }); + + it('returns false when the user has no groups', () => { + expect(hasWriteAll({ user: {} })).toBe(false); + }); +}); diff --git a/frontend/src/lib/shared/server/permissions.ts b/frontend/src/lib/shared/server/permissions.ts new file mode 100644 index 00000000..11053859 --- /dev/null +++ b/frontend/src/lib/shared/server/permissions.ts @@ -0,0 +1,14 @@ +/** + * Server-side permission predicates derived from the authenticated user in `locals`. + * + * The user shape is intentionally narrowed to the only field these checks read + * (`groups[].permissions`) so the helper works against `App.Locals` without importing it. + */ +type PermissionLocals = { + user?: { groups?: { permissions: string[] }[] } | null; +}; + +/** True when any of the user's groups grants WRITE_ALL. False for anonymous users. */ +export function hasWriteAll(locals: PermissionLocals): boolean { + return locals.user?.groups?.some((group) => group.permissions.includes('WRITE_ALL')) ?? false; +} diff --git a/frontend/src/routes/documents/[id]/edit/+page.server.ts b/frontend/src/routes/documents/[id]/edit/+page.server.ts index 5043e963..c012e3b9 100644 --- a/frontend/src/routes/documents/[id]/edit/+page.server.ts +++ b/frontend/src/routes/documents/[id]/edit/+page.server.ts @@ -2,6 +2,7 @@ import { error, fail, redirect } from '@sveltejs/kit'; import { env } from '$env/dynamic/private'; import { createApiClient, extractErrorCode } from '$lib/shared/api.server'; import { parseBackendError, getErrorMessage } from '$lib/shared/errors'; +import { hasWriteAll } from '$lib/shared/server/permissions'; export async function load({ params, @@ -15,11 +16,7 @@ export async function load({ depends: (dep: string) => void; }) { depends('app:document'); - const canWrite = - locals.user?.groups?.some((g: { permissions: string[] }) => - g.permissions.includes('WRITE_ALL') - ) ?? false; - if (!canWrite) throw error(403, 'Forbidden'); + if (!hasWriteAll(locals)) throw error(403, 'Forbidden'); const { id } = params; const api = createApiClient(fetch); diff --git a/frontend/src/routes/documents/new/+page.server.ts b/frontend/src/routes/documents/new/+page.server.ts index c393ca98..2d6c2fd8 100644 --- a/frontend/src/routes/documents/new/+page.server.ts +++ b/frontend/src/routes/documents/new/+page.server.ts @@ -2,6 +2,7 @@ import { fail, redirect } from '@sveltejs/kit'; import { env } from '$env/dynamic/private'; import { createApiClient } from '$lib/shared/api.server'; import { parseBackendError, getErrorMessage } from '$lib/shared/errors'; +import { hasWriteAll } from '$lib/shared/server/permissions'; export async function load({ fetch, @@ -12,11 +13,7 @@ export async function load({ locals: App.Locals; url: URL; }) { - const canWrite = - locals.user?.groups?.some((g: { permissions: string[] }) => - g.permissions.includes('WRITE_ALL') - ) ?? false; - if (!canWrite) throw redirect(303, '/'); + if (!hasWriteAll(locals)) throw redirect(303, '/'); const senderId = url.searchParams.get('senderId') || ''; const receiverId = url.searchParams.get('receiverId') || ''; diff --git a/frontend/src/routes/persons/+page.server.ts b/frontend/src/routes/persons/+page.server.ts index 071208e2..3f04ff7c 100644 --- a/frontend/src/routes/persons/+page.server.ts +++ b/frontend/src/routes/persons/+page.server.ts @@ -1,6 +1,7 @@ import { error } from '@sveltejs/kit'; import { createApiClient } from '$lib/shared/api.server'; import { getErrorMessage } from '$lib/shared/errors'; +import { hasWriteAll } from '$lib/shared/server/permissions'; const PAGE_SIZE = 50; @@ -21,10 +22,7 @@ export async function load({ url, fetch, locals }) { const api = createApiClient(fetch); - const canWrite = - (locals.user as { groups?: { permissions: string[] }[] } | undefined)?.groups?.some((g) => - g.permissions.includes('WRITE_ALL') - ) ?? false; + const canWrite = hasWriteAll(locals); const filters = { q: q || undefined, diff --git a/frontend/src/routes/persons/review/+page.server.ts b/frontend/src/routes/persons/review/+page.server.ts index cb51eab4..c52121f2 100644 --- a/frontend/src/routes/persons/review/+page.server.ts +++ b/frontend/src/routes/persons/review/+page.server.ts @@ -1,14 +1,12 @@ import { error, fail } from '@sveltejs/kit'; import { createApiClient, extractErrorCode } from '$lib/shared/api.server'; import { getErrorMessage } from '$lib/shared/errors'; +import { hasWriteAll } from '$lib/shared/server/permissions'; const PAGE_SIZE = 50; export async function load({ url, fetch, locals }) { - const canWrite = - (locals.user as { groups?: { permissions: string[] }[] } | undefined)?.groups?.some((g) => - g.permissions.includes('WRITE_ALL') - ) ?? false; + const canWrite = hasWriteAll(locals); const page = Math.max(0, Number.parseInt(url.searchParams.get('page') ?? '0', 10) || 0); const api = createApiClient(fetch);