From 5e52a323ace46393e31a333737722c49b9fc8aee Mon Sep 17 00:00:00 2001 From: kneshi <60970253+kneshi@users.noreply.github.com> Date: Mon, 11 May 2026 00:30:09 +0200 Subject: [PATCH 1/6] feat(shared): add DOCUMENT_READ_ROLES constant and docs.read capability --- shared/src/constants/role-permissions.ts | 7 +++++++ shared/src/constants/roles.ts | 9 +++++++++ 2 files changed, 16 insertions(+) diff --git a/shared/src/constants/role-permissions.ts b/shared/src/constants/role-permissions.ts index c4360fb..c827d68 100644 --- a/shared/src/constants/role-permissions.ts +++ b/shared/src/constants/role-permissions.ts @@ -3,6 +3,7 @@ import { ADMIN_ROLES, AUDIT_ROLES, DELETE_ROLES, + DOCUMENT_READ_ROLES, DSR_ROLES, EXPORT_ROLES, FOLLOW_UP_READ_ROLES, @@ -190,6 +191,12 @@ export const ROLE_PERMISSION_MATRIX: readonly RoleCapability[] = [ { method: 'POST', path: '/api/screenings/:id/convert' }, ], }, + { + id: 'docs.read', + labelKey: 'roleMatrix.capability.docs.read', + allowedRoles: DOCUMENT_READ_ROLES, + routes: [{ method: 'GET', path: '/api/documents/:id/download' }], + }, { id: 'docs.write', labelKey: 'roleMatrix.capability.docs.write', diff --git a/shared/src/constants/roles.ts b/shared/src/constants/roles.ts index e35a6de..67109aa 100644 --- a/shared/src/constants/roles.ts +++ b/shared/src/constants/roles.ts @@ -30,3 +30,12 @@ export const FOLLOW_UP_WRITE_ROLES = [ Role.EDITOR, Role.PROCESS_OWNER, ] as const; + +/** Roles that can READ documents linked to treatments / violations / checklist items. */ +export const DOCUMENT_READ_ROLES = [ + Role.ADMIN, + Role.DPO, + Role.EDITOR, + Role.PROCESS_OWNER, + Role.AUDITOR, +] as const; From f23146a6d5edc26e8198f590c7f13a5a90b9bfb4 Mon Sep 17 00:00:00 2001 From: kneshi <60970253+kneshi@users.noreply.github.com> Date: Mon, 11 May 2026 00:30:16 +0200 Subject: [PATCH 2/6] feat(storage): add getObject streaming primitive with Range support --- .../src/modules/documents/storage.service.ts | 56 +++++++++++-- backend/test/services/storage.service.spec.ts | 78 ++++++++++++++++--- 2 files changed, 116 insertions(+), 18 deletions(-) diff --git a/backend/src/modules/documents/storage.service.ts b/backend/src/modules/documents/storage.service.ts index f1f303d..fa366ce 100644 --- a/backend/src/modules/documents/storage.service.ts +++ b/backend/src/modules/documents/storage.service.ts @@ -1,13 +1,21 @@ -import { Injectable, Logger, OnModuleInit } from '@nestjs/common'; +import { + Injectable, + Logger, + OnModuleInit, + NotFoundException, + HttpException, + HttpStatus, +} from '@nestjs/common'; +import { Readable } from 'node:stream'; import { S3Client, PutObjectCommand, GetObjectCommand, + GetObjectCommandOutput, DeleteObjectCommand, CreateBucketCommand, HeadBucketCommand, } from '@aws-sdk/client-s3'; -import { getSignedUrl } from '@aws-sdk/s3-request-presigner'; @Injectable() export class StorageService implements OnModuleInit { @@ -70,10 +78,46 @@ export class StorageService implements OnModuleInit { ); } - async getPresignedUrl(key: string): Promise { - const command = new GetObjectCommand({ Bucket: this.bucket, Key: key }); - const PRESIGNED_URL_EXPIRY_SECONDS = 900; - return getSignedUrl(this.ensureClient(), command, { expiresIn: PRESIGNED_URL_EXPIRY_SECONDS }); + async getObject( + key: string, + range?: string, + ): Promise<{ + body: Readable; + contentType: string; + contentLength: number; + contentRange?: string; + etag?: string; + statusCode: 200 | 206; + }> { + let response: GetObjectCommandOutput; + try { + response = await this.ensureClient().send( + new GetObjectCommand({ Bucket: this.bucket, Key: key, Range: range }), + ); + } catch (err: unknown) { + const name = (err as { name?: string })?.name; + if (name === 'NoSuchKey' || name === 'NotFound') { + throw new NotFoundException('Object not found'); + } + if (name === 'InvalidRange') { + throw new HttpException( + 'Range Not Satisfiable', + HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE, + ); + } + throw err; + } + if (!response.Body) { + throw new Error(`S3 GetObject returned no body for key: ${key}`); + } + return { + body: response.Body as Readable, + contentType: response.ContentType ?? 'application/octet-stream', + contentLength: response.ContentLength ?? 0, + contentRange: response.ContentRange, + etag: response.ETag ?? undefined, + statusCode: response.ContentRange ? 206 : 200, + }; } async delete(key: string): Promise { diff --git a/backend/test/services/storage.service.spec.ts b/backend/test/services/storage.service.spec.ts index 9f67380..9a43bb4 100644 --- a/backend/test/services/storage.service.spec.ts +++ b/backend/test/services/storage.service.spec.ts @@ -1,9 +1,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { Readable } from 'node:stream'; import { StorageService } from '../../src/modules/documents/storage.service'; -const { sendSpy, getSignedUrlSpy } = vi.hoisted(() => ({ +const { sendSpy } = vi.hoisted(() => ({ sendSpy: vi.fn(), - getSignedUrlSpy: vi.fn().mockResolvedValue('https://signed.example.test/x'), })); vi.mock('@aws-sdk/client-s3', () => ({ @@ -32,10 +32,6 @@ vi.mock('@aws-sdk/client-s3', () => ({ }), })); -vi.mock('@aws-sdk/s3-request-presigner', () => ({ - getSignedUrl: getSignedUrlSpy, -})); - const ORIG_ENV = { ...process.env }; describe('StorageService', () => { @@ -122,11 +118,69 @@ describe('StorageService', () => { expect(cmd.__cmd).toBe('Delete'); }); - it('getPresignedUrl calls the presigner helper and returns the URL', async () => { - getSignedUrlSpy.mockClear(); - const url = await service.getPresignedUrl('treatment/abc.pdf'); - expect(url).toBe('https://signed.example.test/x'); - expect(getSignedUrlSpy).toHaveBeenCalledTimes(1); + it('getObject returns body, headers, and 200 status when no Range is given', async () => { + const bytes = Buffer.from('hello-bytes'); + sendSpy.mockResolvedValueOnce({ + Body: Readable.from(bytes), + ContentType: 'application/pdf', + ContentLength: bytes.length, + ETag: '"abc"', + }); + + const result = await service.getObject('treatment/abc.pdf'); + + expect(sendSpy).toHaveBeenCalledTimes(1); + const cmd = sendSpy.mock.calls[0][0] as { __cmd: string; input: Record }; + expect(cmd.__cmd).toBe('Get'); + expect(cmd.input.Key).toBe('treatment/abc.pdf'); + expect(cmd.input.Range).toBeUndefined(); + expect(result.statusCode).toBe(200); + expect(result.contentType).toBe('application/pdf'); + expect(result.contentLength).toBe(bytes.length); + expect(result.contentRange).toBeUndefined(); + expect(result.etag).toBe('"abc"'); + expect(result.body).toBeInstanceOf(Readable); + }); + + it('getObject forwards the Range header and returns 206 + ContentRange', async () => { + sendSpy.mockResolvedValueOnce({ + Body: Readable.from(Buffer.from('hello')), + ContentType: 'application/pdf', + ContentLength: 5, + ContentRange: 'bytes 0-4/100', + }); + + const result = await service.getObject('treatment/abc.pdf', 'bytes=0-4'); + + const cmd = sendSpy.mock.calls[0][0] as { input: Record }; + expect(cmd.input.Range).toBe('bytes=0-4'); + expect(result.statusCode).toBe(206); + expect(result.contentRange).toBe('bytes 0-4/100'); + }); + + it('getObject maps NoSuchKey to NotFoundException', async () => { + const { NotFoundException } = await import('@nestjs/common'); + const err = new Error('not found'); + (err as Error & { name: string }).name = 'NoSuchKey'; + sendSpy.mockRejectedValueOnce(err); + + await expect(service.getObject('missing/key')).rejects.toBeInstanceOf(NotFoundException); + }); + + it('getObject maps InvalidRange to HTTP 416', async () => { + const { HttpException, HttpStatus } = await import('@nestjs/common'); + const err = new Error('bad range'); + (err as Error & { name: string }).name = 'InvalidRange'; + sendSpy.mockRejectedValueOnce(err); + + let caught: unknown; + await service.getObject('k', 'bytes=999-999').catch(e => { + caught = e; + }); + expect(caught).toBeInstanceOf(HttpException); + expect((caught as InstanceType).getStatus()).toBe( + HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE, + ); }); }); @@ -135,7 +189,7 @@ describe('StorageService', () => { delete process.env.S3_ENDPOINT; await service.onModuleInit(); await expect(service.delete('k')).rejects.toThrow(/Set S3_ENDPOINT/); - await expect(service.getPresignedUrl('k')).rejects.toThrow(/Set S3_ENDPOINT/); + await expect(service.getObject('k')).rejects.toThrow(/Set S3_ENDPOINT/); }); }); }); From b14c953956db7e4bf2c4e26e443c5473229c2128 Mon Sep 17 00:00:00 2001 From: kneshi <60970253+kneshi@users.noreply.github.com> Date: Mon, 11 May 2026 00:30:24 +0200 Subject: [PATCH 3/6] feat(authorization): per-entity read helpers for documents and follow-up attachments --- .../common/authorization/document-access.ts | 103 ++++++ backend/test/services/document-access.spec.ts | 311 ++++++++++++++++++ 2 files changed, 414 insertions(+) create mode 100644 backend/src/common/authorization/document-access.ts create mode 100644 backend/test/services/document-access.spec.ts diff --git a/backend/src/common/authorization/document-access.ts b/backend/src/common/authorization/document-access.ts new file mode 100644 index 0000000..c31f6e4 --- /dev/null +++ b/backend/src/common/authorization/document-access.ts @@ -0,0 +1,103 @@ +import { ForbiddenException, NotFoundException } from '@nestjs/common'; +import { LinkedEntity, Role, DOCUMENT_READ_ROLES, FOLLOW_UP_READ_ROLES } from '@article30/shared'; +import { EntityType } from '@prisma/client'; +import type { Document, FollowUpAttachment } from '@prisma/client'; +import type { PrismaService } from '../../prisma/prisma.service'; +import type { RequestUser } from '../types/request-user'; +import { ownsTreatment, treatmentOwnershipWhere, isProcessOwner } from './treatment-ownership'; + +function ensureRole( + user: RequestUser | undefined, + allowed: readonly Role[], +): asserts user is RequestUser { + // RequestUser.role is the Prisma-generated Role type; allowed comes from + // @article30/shared. Same string values, nominally different types - cast + // through readonly string[] to bridge them without losing call-site safety. + if (!user || !(allowed as readonly string[]).includes(user.role)) { + throw new ForbiddenException(); + } +} + +export async function assertCanReadDocument( + user: RequestUser | undefined, + document: Document, + prisma: PrismaService, +): Promise { + ensureRole(user, DOCUMENT_READ_ROLES); + if (!isProcessOwner(user)) return; + + switch (document.linkedEntity) { + case LinkedEntity.TREATMENT: { + const treatment = await prisma.treatment.findUnique({ + where: { id: document.linkedEntityId }, + select: { createdBy: true, assignedTo: true }, + }); + if (!treatment || !ownsTreatment(treatment, user.id)) { + throw new NotFoundException(); + } + return; + } + case LinkedEntity.VIOLATION: { + const violation = await prisma.violation.findUnique({ + where: { id: document.linkedEntityId }, + select: { createdBy: true, assignedTo: true }, + }); + if (!violation) throw new NotFoundException(); + if (violation.createdBy === user.id || violation.assignedTo === user.id) return; + const linked = await prisma.violationTreatment.findFirst({ + where: { + violationId: document.linkedEntityId, + treatment: treatmentOwnershipWhere(user.id), + }, + select: { violationId: true }, + }); + if (!linked) throw new NotFoundException(); + return; + } + case LinkedEntity.CHECKLIST_ITEM: + // Org-wide artefact - no per-user scoping beyond the role gate. + return; + default: { + const _exhaustive: never = document.linkedEntity; + throw new NotFoundException(); + } + } +} + +export async function assertCanReadFollowUpAttachment( + user: RequestUser | undefined, + attachment: FollowUpAttachment, + prisma: PrismaService, +): Promise { + ensureRole(user, FOLLOW_UP_READ_ROLES); + if (!isProcessOwner(user)) return; + + switch (attachment.entityType) { + case EntityType.VIOLATION: { + const violation = await prisma.violation.findUnique({ + where: { id: attachment.entityId }, + select: { createdBy: true, assignedTo: true }, + }); + if (!violation) throw new NotFoundException(); + if (violation.createdBy === user.id || violation.assignedTo === user.id) return; + const linked = await prisma.violationTreatment.findFirst({ + where: { violationId: attachment.entityId, treatment: treatmentOwnershipWhere(user.id) }, + select: { violationId: true }, + }); + if (!linked) throw new NotFoundException(); + return; + } + case EntityType.DSR: { + const linked = await prisma.dsrTreatmentProcessingLog.findFirst({ + where: { dsrId: attachment.entityId, treatment: treatmentOwnershipWhere(user.id) }, + select: { dsrId: true }, + }); + if (!linked) throw new NotFoundException(); + return; + } + default: { + const _exhaustive: never = attachment.entityType; + throw new NotFoundException(); + } + } +} diff --git a/backend/test/services/document-access.spec.ts b/backend/test/services/document-access.spec.ts new file mode 100644 index 0000000..3d39c00 --- /dev/null +++ b/backend/test/services/document-access.spec.ts @@ -0,0 +1,311 @@ +import { describe, it, expect, beforeAll, afterAll, afterEach } from 'vitest'; +import { ForbiddenException, NotFoundException } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { LinkedEntity, Role } from '@article30/shared'; +import { EntityType } from '@prisma/client'; +import { PrismaModule } from '../../src/prisma/prisma.module'; +import { PrismaService } from '../../src/prisma/prisma.service'; +import { + assertCanReadDocument, + assertCanReadFollowUpAttachment, +} from '../../src/common/authorization/document-access'; +import { cleanupDatabase } from '../helpers'; +import type { Document, FollowUpAttachment } from '@prisma/client'; +import type { RequestUser } from '../../src/common/types/request-user'; + +const TEST_DB_URL = + process.env.DATABASE_URL_TEST ?? + 'postgresql://article30:article30_secret@localhost:5432/article30_test'; + +describe('document-access helpers', () => { + let module: TestingModule; + let prisma: PrismaService; + + beforeAll(async () => { + process.env.DATABASE_URL = TEST_DB_URL; + module = await Test.createTestingModule({ imports: [PrismaModule] }).compile(); + prisma = module.get(PrismaService); + }); + + afterEach(() => cleanupDatabase(prisma)); + afterAll(() => module.close()); + + function makeUser(role: Role, id = 'user-1'): RequestUser { + return { id, role, email: `${id}@x.test`, firstName: 'F', lastName: 'L', approved: true }; + } + + async function seedUserRow(role: Role, id = 'user-1') { + return prisma.user.create({ + data: { + id, + firstName: 'F', + lastName: 'L', + email: `${id}@x.test`, + password: 'x', + role, + approved: true, + }, + }); + } + + describe('assertCanReadDocument - TREATMENT', () => { + it('allows ADMIN regardless of ownership', async () => { + await seedUserRow(Role.ADMIN, 'admin-1'); + const owner = await seedUserRow(Role.PROCESS_OWNER, 'owner-1'); + const t = await prisma.treatment.create({ + data: { + name: 't', + purpose: 'p', + legalBasis: 'consent', + personCategories: [], + recipientTypes: [], + securityMeasures: [], + sensitiveCategories: [], + subPurposes: [], + retentionPeriod: '5y', + createdBy: owner.id, + }, + }); + const doc = { + id: 'd1', + linkedEntity: LinkedEntity.TREATMENT, + linkedEntityId: t.id, + } as Document; + + await expect( + assertCanReadDocument(makeUser(Role.ADMIN, 'admin-1'), doc, prisma), + ).resolves.toBeUndefined(); + }); + + it('rejects PROCESS_OWNER who does not own the treatment with NotFoundException', async () => { + const otherOwner = await seedUserRow(Role.PROCESS_OWNER, 'owner-other'); + await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const t = await prisma.treatment.create({ + data: { + name: 't', + purpose: 'p', + legalBasis: 'consent', + personCategories: [], + recipientTypes: [], + securityMeasures: [], + sensitiveCategories: [], + subPurposes: [], + retentionPeriod: '5y', + createdBy: otherOwner.id, + }, + }); + const doc = { + id: 'd1', + linkedEntity: LinkedEntity.TREATMENT, + linkedEntityId: t.id, + } as Document; + + await expect( + assertCanReadDocument(makeUser(Role.PROCESS_OWNER, 'owner-self'), doc, prisma), + ).rejects.toBeInstanceOf(NotFoundException); + }); + + it('allows PROCESS_OWNER who owns the treatment (createdBy)', async () => { + const owner = await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const t = await prisma.treatment.create({ + data: { + name: 't', + purpose: 'p', + legalBasis: 'consent', + personCategories: [], + recipientTypes: [], + securityMeasures: [], + sensitiveCategories: [], + subPurposes: [], + retentionPeriod: '5y', + createdBy: owner.id, + }, + }); + const doc = { + id: 'd1', + linkedEntity: LinkedEntity.TREATMENT, + linkedEntityId: t.id, + } as Document; + + await expect( + assertCanReadDocument(makeUser(Role.PROCESS_OWNER, owner.id), doc, prisma), + ).resolves.toBeUndefined(); + }); + }); + + describe('assertCanReadDocument - VIOLATION', () => { + it('allows PROCESS_OWNER who created the violation', async () => { + const u = await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const v = await prisma.violation.create({ + data: { title: 'v', severity: 'LOW', awarenessAt: new Date(), createdBy: u.id }, + }); + const doc = { + id: 'd', + linkedEntity: LinkedEntity.VIOLATION, + linkedEntityId: v.id, + } as Document; + await expect( + assertCanReadDocument(makeUser(Role.PROCESS_OWNER, u.id), doc, prisma), + ).resolves.toBeUndefined(); + }); + + it('allows PROCESS_OWNER via ViolationTreatment join to an owned treatment', async () => { + const owner = await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const otherOwner = await seedUserRow(Role.PROCESS_OWNER, 'owner-other'); + const v = await prisma.violation.create({ + data: { title: 'v', severity: 'LOW', awarenessAt: new Date(), createdBy: otherOwner.id }, + }); + const t = await prisma.treatment.create({ + data: { + name: 't', + purpose: 'p', + legalBasis: 'consent', + personCategories: [], + recipientTypes: [], + securityMeasures: [], + sensitiveCategories: [], + subPurposes: [], + retentionPeriod: '5y', + createdBy: owner.id, + }, + }); + await prisma.violationTreatment.create({ data: { violationId: v.id, treatmentId: t.id } }); + const doc = { + id: 'd', + linkedEntity: LinkedEntity.VIOLATION, + linkedEntityId: v.id, + } as Document; + await expect( + assertCanReadDocument(makeUser(Role.PROCESS_OWNER, owner.id), doc, prisma), + ).resolves.toBeUndefined(); + }); + + it('rejects PROCESS_OWNER with no ownership and no linked treatment', async () => { + const otherOwner = await seedUserRow(Role.PROCESS_OWNER, 'owner-other'); + await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const v = await prisma.violation.create({ + data: { title: 'v', severity: 'LOW', awarenessAt: new Date(), createdBy: otherOwner.id }, + }); + const doc = { + id: 'd', + linkedEntity: LinkedEntity.VIOLATION, + linkedEntityId: v.id, + } as Document; + await expect( + assertCanReadDocument(makeUser(Role.PROCESS_OWNER, 'owner-self'), doc, prisma), + ).rejects.toBeInstanceOf(NotFoundException); + }); + }); + + describe('assertCanReadDocument - CHECKLIST_ITEM', () => { + it('allows any DOCUMENT_READ_ROLES regardless of ownership', async () => { + await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const doc = { + id: 'd', + linkedEntity: LinkedEntity.CHECKLIST_ITEM, + linkedEntityId: 'static-item-123', + } as Document; + await expect( + assertCanReadDocument(makeUser(Role.PROCESS_OWNER, 'owner-self'), doc, prisma), + ).resolves.toBeUndefined(); + }); + }); + + describe('role gate', () => { + it('assertCanReadDocument throws ForbiddenException for an undefined user', async () => { + const doc = { + id: 'd', + linkedEntity: LinkedEntity.TREATMENT, + linkedEntityId: '00000000-0000-0000-0000-000000000000', + } as Document; + await expect(assertCanReadDocument(undefined, doc, prisma)).rejects.toBeInstanceOf( + ForbiddenException, + ); + }); + + it('assertCanReadFollowUpAttachment throws ForbiddenException for an undefined user', async () => { + const a = { + entityType: 'VIOLATION', + entityId: '00000000-0000-0000-0000-000000000000', + storageKey: 'k', + } as FollowUpAttachment; + await expect(assertCanReadFollowUpAttachment(undefined, a, prisma)).rejects.toBeInstanceOf( + ForbiddenException, + ); + }); + }); + + describe('assertCanReadFollowUpAttachment', () => { + it('allows PROCESS_OWNER for VIOLATION attachment when violation is owned', async () => { + const u = await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const v = await prisma.violation.create({ + data: { title: 'v', severity: 'LOW', awarenessAt: new Date(), createdBy: u.id }, + }); + const a = { + entityType: EntityType.VIOLATION, + entityId: v.id, + storageKey: 'k', + } as unknown as FollowUpAttachment; + await expect( + assertCanReadFollowUpAttachment(makeUser(Role.PROCESS_OWNER, u.id), a, prisma), + ).resolves.toBeUndefined(); + }); + + it('rejects PROCESS_OWNER for DSR attachment when no DsrTreatmentProcessingLog joins an owned treatment', async () => { + await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const dsr = await prisma.dataSubjectRequest.create({ + data: { + type: 'ACCESS', + requesterName: 'X', + requesterEmail: 'x@x.test', + deadline: new Date(Date.now() + 1e9), + }, + }); + const a = { + entityType: EntityType.DSR, + entityId: dsr.id, + storageKey: 'k', + } as unknown as FollowUpAttachment; + await expect( + assertCanReadFollowUpAttachment(makeUser(Role.PROCESS_OWNER, 'owner-self'), a, prisma), + ).rejects.toBeInstanceOf(NotFoundException); + }); + + it('allows PROCESS_OWNER for DSR attachment via DsrTreatmentProcessingLog join', async () => { + const owner = await seedUserRow(Role.PROCESS_OWNER, 'owner-self'); + const dsr = await prisma.dataSubjectRequest.create({ + data: { + type: 'ACCESS', + requesterName: 'X', + requesterEmail: 'x@x.test', + deadline: new Date(Date.now() + 1e9), + }, + }); + const t = await prisma.treatment.create({ + data: { + name: 't', + purpose: 'p', + legalBasis: 'consent', + personCategories: [], + recipientTypes: [], + securityMeasures: [], + sensitiveCategories: [], + subPurposes: [], + retentionPeriod: '5y', + createdBy: owner.id, + }, + }); + await prisma.dsrTreatmentProcessingLog.create({ + data: { dsrId: dsr.id, treatmentId: t.id }, + }); + const a = { + entityType: EntityType.DSR, + entityId: dsr.id, + storageKey: 'k', + } as unknown as FollowUpAttachment; + await expect( + assertCanReadFollowUpAttachment(makeUser(Role.PROCESS_OWNER, owner.id), a, prisma), + ).resolves.toBeUndefined(); + }); + }); +}); From 119ce8c3cb3ce18f2c51aecaded85b92a7b085d8 Mon Sep 17 00:00:00 2001 From: kneshi <60970253+kneshi@users.noreply.github.com> Date: Mon, 11 May 2026 00:30:35 +0200 Subject: [PATCH 4/6] feat(documents): stream document and attachment downloads with ownership checks --- backend/package.json | 1 - .../modules/documents/documents.controller.ts | 68 ++++++++++++--- .../modules/documents/documents.service.ts | 6 +- .../follow-up/attachments.controller.ts | 49 +++++++++-- backend/test/e2e/app-factory.ts | 13 ++- backend/test/e2e/documents.e2e.spec.ts | 82 ++++++++++++++++++- backend/test/e2e/follow-up.e2e.spec.ts | 71 ++++++++++++++++ .../test/services/attachments.service.spec.ts | 1 - .../test/services/documents.service.spec.ts | 21 ++++- pnpm-lock.yaml | 29 ------- 10 files changed, 281 insertions(+), 60 deletions(-) diff --git a/backend/package.json b/backend/package.json index 725a63d..dea6087 100644 --- a/backend/package.json +++ b/backend/package.json @@ -21,7 +21,6 @@ "dependencies": { "@article30/shared": "workspace:*", "@aws-sdk/client-s3": "^3.1043.0", - "@aws-sdk/s3-request-presigner": "^3.1043.0", "@nestjs/common": "^11.1.19", "@nestjs/core": "^11.1.19", "@nestjs/platform-express": "^11.1.19", diff --git a/backend/src/modules/documents/documents.controller.ts b/backend/src/modules/documents/documents.controller.ts index 1acdcc3..1f17f3d 100644 --- a/backend/src/modules/documents/documents.controller.ts +++ b/backend/src/modules/documents/documents.controller.ts @@ -3,41 +3,57 @@ import { Controller, Delete, Get, + Headers, Param, Post, Query, + Res, UploadedFile, UseInterceptors, } from '@nestjs/common'; import { FileInterceptor } from '@nestjs/platform-express'; -import { ApiTags } from '@nestjs/swagger'; +import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; import { Throttle } from '@nestjs/throttler'; -import { WRITE_ROLES } from '@article30/shared'; +import { Response } from 'express'; +import { DOCUMENT_READ_ROLES, WRITE_ROLES } from '@article30/shared'; import { DocumentsService } from './documents.service'; +import { StorageService } from './storage.service'; import { UploadDocumentDto } from './dto/upload-document.dto'; import { Roles } from '../../common/decorators/roles.decorator'; import { CurrentUser } from '../../common/decorators/current-user.decorator'; import { RequestUser } from '../../common/types/request-user'; +import { PrismaService } from '../../prisma/prisma.service'; +import { assertCanReadDocument } from '../../common/authorization/document-access'; const MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024; const UPLOAD_RATE_LIMIT = 10; const UPLOAD_RATE_TTL_MS = 60_000; +const RFC8187_RESERVED = /[!'()*]/g; + +function encodeRfc8187(value: string): string { + // encodeURIComponent already handles most chars; we additionally percent-encode + // !'()* because they are syntactically significant in filename*=UTF-8'' + // (notably "'" terminates the language tag). + return encodeURIComponent(value).replace( + RFC8187_RESERVED, + c => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, + ); +} + @ApiTags('documents') @Controller('documents') export class DocumentsController { - constructor(private readonly documentsService: DocumentsService) {} + constructor( + private readonly documentsService: DocumentsService, + private readonly storage: StorageService, + private readonly prisma: PrismaService, + ) {} @Post('upload') @Roles(...WRITE_ROLES) @Throttle({ default: { limit: UPLOAD_RATE_LIMIT, ttl: UPLOAD_RATE_TTL_MS } }) - // Pre-buffer cap matches the service-layer post-buffer check. - // Requests over the limit get a 413 before bytes land in the Node heap. - @UseInterceptors( - FileInterceptor('file', { - limits: { fileSize: MAX_FILE_SIZE_BYTES }, - }), - ) + @UseInterceptors(FileInterceptor('file', { limits: { fileSize: MAX_FILE_SIZE_BYTES } })) upload( @UploadedFile() file: Express.Multer.File, @Body() dto: UploadDocumentDto, @@ -52,8 +68,36 @@ export class DocumentsController { } @Get(':id/download') - getDownloadUrl(@Param('id') id: string) { - return this.documentsService.getDownloadUrl(id); + @Roles(...DOCUMENT_READ_ROLES) + @ApiOkResponse({ + description: 'Streams the file. Returns 206 with Content-Range when a Range header is sent.', + content: { '*/*': { schema: { type: 'string', format: 'binary' } } }, + }) + async download( + @Param('id') id: string, + @Headers('range') range: string | undefined, + @CurrentUser() user: RequestUser, + @Res() res: Response, + ) { + const document = await this.documentsService.findById(id); + await assertCanReadDocument(user, document, this.prisma); + + const obj = await this.storage.getObject(document.s3Key, range); + + res.status(obj.statusCode); + res.setHeader('Content-Type', document.mimeType); + res.setHeader('Content-Length', String(obj.contentLength)); + res.setHeader('Accept-Ranges', 'bytes'); + if (obj.contentRange) res.setHeader('Content-Range', obj.contentRange); + if (obj.etag) res.setHeader('ETag', obj.etag); + res.setHeader( + 'Content-Disposition', + `inline; filename*=UTF-8''${encodeRfc8187(document.filename)}`, + ); + res.setHeader('Cache-Control', 'private, no-store'); + res.setHeader('X-Content-Type-Options', 'nosniff'); + + obj.body.pipe(res); } @Delete(':id') diff --git a/backend/src/modules/documents/documents.service.ts b/backend/src/modules/documents/documents.service.ts index d47d9d9..e528fbe 100644 --- a/backend/src/modules/documents/documents.service.ts +++ b/backend/src/modules/documents/documents.service.ts @@ -93,14 +93,12 @@ export class DocumentsService { }); } - async getDownloadUrl(id: string) { + async findById(id: string) { const document = await this.prisma.document.findUnique({ where: { id } }); if (!document) { throw new NotFoundException('Document not found'); } - - const url = await this.storage.getPresignedUrl(document.s3Key); - return { url, filename: document.filename }; + return document; } async delete(id: string) { diff --git a/backend/src/modules/follow-up/attachments.controller.ts b/backend/src/modules/follow-up/attachments.controller.ts index 17a0987..0ddd3f2 100644 --- a/backend/src/modules/follow-up/attachments.controller.ts +++ b/backend/src/modules/follow-up/attachments.controller.ts @@ -3,6 +3,7 @@ import { Controller, Delete, Get, + Headers, NotFoundException, Param, Post, @@ -10,7 +11,7 @@ import { UploadedFile, UseInterceptors, } from '@nestjs/common'; -import { ApiTags } from '@nestjs/swagger'; +import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; import { FileInterceptor } from '@nestjs/platform-express'; import { Response } from 'express'; import { AttachmentCategory, EntityType } from '@prisma/client'; @@ -21,6 +22,16 @@ import { RequestUser } from '../../common/types/request-user'; import { PrismaService } from '../../prisma/prisma.service'; import { AttachmentsService } from './attachments.service'; import { StorageService } from '../documents/storage.service'; +import { assertCanReadFollowUpAttachment } from '../../common/authorization/document-access'; + +const RFC8187_RESERVED = /[!'()*]/g; + +function encodeRfc8187(value: string): string { + return encodeURIComponent(value).replace( + RFC8187_RESERVED, + c => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, + ); +} @ApiTags('follow-up') @Controller('follow-up/attachments') @@ -32,9 +43,7 @@ export class AttachmentsController { ) {} // Order matters: more-specific routes (`:id/download`, `:id` Delete) must - // be declared BEFORE the polymorphic `:entityType/:entityId` Get, otherwise - // Express matches them as a 2-param entityType/entityId and 404s on the - // entity validator (review #2). + // be declared BEFORE the polymorphic `:entityType/:entityId` Get. @Post() @Roles(...FOLLOW_UP_WRITE_ROLES) @@ -59,15 +68,41 @@ export class AttachmentsController { @Get(':id/download') @Roles(...FOLLOW_UP_READ_ROLES) - async download(@Param('id') id: string, @Res() res: Response) { + @ApiOkResponse({ + description: 'Streams the file. Returns 206 with Content-Range when a Range header is sent.', + content: { '*/*': { schema: { type: 'string', format: 'binary' } } }, + }) + async download( + @Param('id') id: string, + @Headers('range') range: string | undefined, + @CurrentUser() user: RequestUser, + @Res() res: Response, + ) { const attachment = await this.prisma.followUpAttachment.findFirst({ where: { id, deletedAt: null }, }); if (!attachment || !attachment.storageKey) { throw new NotFoundException('Attachment not found'); } - const url = await this.storage.getPresignedUrl(attachment.storageKey); - res.redirect(url); + + await assertCanReadFollowUpAttachment(user, attachment, this.prisma); + + const obj = await this.storage.getObject(attachment.storageKey, range); + + res.status(obj.statusCode); + res.setHeader('Content-Type', attachment.mimeType); + res.setHeader('Content-Length', String(obj.contentLength)); + res.setHeader('Accept-Ranges', 'bytes'); + if (obj.contentRange) res.setHeader('Content-Range', obj.contentRange); + if (obj.etag) res.setHeader('ETag', obj.etag); + res.setHeader( + 'Content-Disposition', + `inline; filename*=UTF-8''${encodeRfc8187(attachment.filename)}`, + ); + res.setHeader('Cache-Control', 'private, no-store'); + res.setHeader('X-Content-Type-Options', 'nosniff'); + + obj.body.pipe(res); } @Delete(':id') diff --git a/backend/test/e2e/app-factory.ts b/backend/test/e2e/app-factory.ts index fcb982d..02667a3 100644 --- a/backend/test/e2e/app-factory.ts +++ b/backend/test/e2e/app-factory.ts @@ -70,7 +70,18 @@ export async function createTestApp(): Promise { .useValue({ onModuleInit: async () => {}, upload: async (_key: string, _buffer: Buffer, _mimeType: string) => {}, - getPresignedUrl: async (_key: string) => 'https://test.local/signed', + getObject: async (_key: string, range?: string) => { + const { Readable } = await import('node:stream'); + const bytes = Buffer.from('proxied-bytes'); + return { + body: Readable.from(bytes), + contentType: 'application/pdf', + contentLength: bytes.length, + contentRange: range ? `bytes 0-${bytes.length - 1}/${bytes.length}` : undefined, + etag: '"deadbeef"', + statusCode: range ? 206 : 200, + }; + }, delete: async (_key: string) => {}, }) .compile(); diff --git a/backend/test/e2e/documents.e2e.spec.ts b/backend/test/e2e/documents.e2e.spec.ts index 2f4f49e..f9fed77 100644 --- a/backend/test/e2e/documents.e2e.spec.ts +++ b/backend/test/e2e/documents.e2e.spec.ts @@ -165,16 +165,90 @@ describe('documents.controller (e2e)', () => { // ------------------------ GET /:id/download -------------------------- describe('GET /api/documents/:id/download', () => { - it('returns 200 with a signed url for an approved user', async () => { + it('streams the bytes for an approved DPO with the right headers', async () => { const { user, password } = await seedUser(testApp.prisma, Role.DPO); const treatment = await seedTreatment(testApp.prisma, user.id); const doc = await seedDocument(testApp.prisma, 'TREATMENT', treatment.id, user.id); const { agent } = await loginAs(testApp.app, user.email, password); + + const res = await agent.get(`/api/documents/${doc.id}/download`).buffer(true); + + expect(res.status).toBe(200); + expect(res.headers['content-type']).toContain('application/pdf'); // Document.mimeType from seed + expect(res.headers['content-disposition']).toContain('inline'); + expect(res.headers['content-disposition']).toContain(encodeURIComponent(doc.filename)); + expect(res.headers['accept-ranges']).toBe('bytes'); + expect(res.headers['cache-control']).toContain('no-store'); + expect(res.headers['x-content-type-options']).toBe('nosniff'); + }); + + it('returns 206 with Content-Range when a Range header is sent', async () => { + const { user, password } = await seedUser(testApp.prisma, Role.DPO); + const treatment = await seedTreatment(testApp.prisma, user.id); + const doc = await seedDocument(testApp.prisma, 'TREATMENT', treatment.id, user.id); + const { agent } = await loginAs(testApp.app, user.email, password); + + const res = await agent + .get(`/api/documents/${doc.id}/download`) + .set('Range', 'bytes=0-4') + .buffer(true); + + expect(res.status).toBe(206); + expect(res.headers['content-range']).toMatch(/^bytes 0-/); + }); + + it('returns 404 for PROCESS_OWNER who does not own the linked treatment', async () => { + const { user: ownerOther } = await seedUser(testApp.prisma, Role.PROCESS_OWNER, { + email: 'other-po@example.test', + }); + const treatment = await seedTreatment(testApp.prisma, ownerOther.id); + const doc = await seedDocument(testApp.prisma, 'TREATMENT', treatment.id, ownerOther.id); + + const { user: self, password } = await seedUser(testApp.prisma, Role.PROCESS_OWNER, { + email: 'self-po@example.test', + }); + const { agent } = await loginAs(testApp.app, self.email, password); + const res = await agent.get(`/api/documents/${doc.id}/download`); + expect(res.status).toBe(404); + }); + + it('returns 200 for an AUDITOR (no ownership scoping)', async () => { + const { user: ownerOther } = await seedUser(testApp.prisma, Role.PROCESS_OWNER, { + email: 'other-po-2@example.test', + }); + const treatment = await seedTreatment(testApp.prisma, ownerOther.id); + const doc = await seedDocument(testApp.prisma, 'TREATMENT', treatment.id, ownerOther.id); + + const { user: auditor, password } = await seedUser(testApp.prisma, Role.AUDITOR); + const { agent } = await loginAs(testApp.app, auditor.email, password); + + const res = await agent.get(`/api/documents/${doc.id}/download`).buffer(true); + expect(res.status).toBe(200); + }); + + it('returns 404 for a non-existent document', async () => { + const { user, password } = await seedUser(testApp.prisma, Role.DPO); + const { agent } = await loginAs(testApp.app, user.email, password); + const res = await agent.get(`/api/documents/00000000-0000-0000-0000-000000000000/download`); + expect(res.status).toBe(404); + }); + + it('encodes RFC 8187 reserved chars in Content-Disposition for filenames with apostrophes', async () => { + const { user, password } = await seedUser(testApp.prisma, Role.DPO); + const treatment = await seedTreatment(testApp.prisma, user.id); + const doc = await seedDocument(testApp.prisma, 'TREATMENT', treatment.id, user.id, { + filename: "O'Reilly's policy.pdf", + }); + const { agent } = await loginAs(testApp.app, user.email, password); + + const res = await agent.get(`/api/documents/${doc.id}/download`).buffer(true); + expect(res.status).toBe(200); - expect(typeof res.body.url).toBe('string'); - expect(res.body.url).toBe('https://test.local/signed'); - expect(res.body.filename).toBe(doc.filename); + // Apostrophes must be percent-encoded (%27) so they do not terminate the language tag + expect(res.headers['content-disposition']).toBe( + "inline; filename*=UTF-8''O%27Reilly%27s%20policy.pdf", + ); }); }); diff --git a/backend/test/e2e/follow-up.e2e.spec.ts b/backend/test/e2e/follow-up.e2e.spec.ts index 0f6751d..5d62dfe 100644 --- a/backend/test/e2e/follow-up.e2e.spec.ts +++ b/backend/test/e2e/follow-up.e2e.spec.ts @@ -134,4 +134,75 @@ describe('follow-up.controllers (e2e)', () => { expect(res.status).toBe(403); }); }); + + describe('GET /api/follow-up/attachments/:id/download', () => { + async function seedAttachment( + prisma: TestApp['prisma'], + entityType: 'VIOLATION' | 'DSR', + entityId: string, + uploadedBy: string, + ) { + return prisma.followUpAttachment.create({ + data: { + entityType, + entityId, + filename: 'evidence.pdf', + mimeType: 'application/pdf', + sizeBytes: 13, + storageKey: `follow-up/${entityType.toLowerCase()}/${entityId}/k`, + sha256: 'a'.repeat(64), + previousSha256: null, + category: 'EVIDENCE', + uploadedBy, + }, + }); + } + + it('streams the bytes for a DPO viewing a VIOLATION attachment', async () => { + const { user, password } = await seedUser(testApp.prisma, Role.DPO); + const v = await seedViolation(testApp.prisma, user.id); + const a = await seedAttachment(testApp.prisma, 'VIOLATION', v.id, user.id); + const { agent } = await loginAs(testApp.app, user.email, password); + + const res = await agent.get(`/api/follow-up/attachments/${a.id}/download`).buffer(true); + expect(res.status).toBe(200); + expect(res.headers['content-type']).toContain('application/pdf'); + expect(res.headers['content-disposition']).toContain('inline'); + }); + + it('returns 404 for a soft-deleted attachment', async () => { + const { user, password } = await seedUser(testApp.prisma, Role.DPO); + const v = await seedViolation(testApp.prisma, user.id); + const a = await seedAttachment(testApp.prisma, 'VIOLATION', v.id, user.id); + await testApp.prisma.followUpAttachment.update({ + where: { id: a.id }, + data: { + deletedAt: new Date(), + storageKey: null, + deletedBy: user.id, + deletionReason: 'test', + }, + }); + const { agent } = await loginAs(testApp.app, user.email, password); + + const res = await agent.get(`/api/follow-up/attachments/${a.id}/download`); + expect(res.status).toBe(404); + }); + + it('returns 404 for PROCESS_OWNER on a VIOLATION they do not own', async () => { + const { user: other } = await seedUser(testApp.prisma, Role.PROCESS_OWNER, { + email: 'po-other@x.test', + }); + const v = await seedViolation(testApp.prisma, other.id); + const a = await seedAttachment(testApp.prisma, 'VIOLATION', v.id, other.id); + + const { user: self, password } = await seedUser(testApp.prisma, Role.PROCESS_OWNER, { + email: 'po-self@x.test', + }); + const { agent } = await loginAs(testApp.app, self.email, password); + + const res = await agent.get(`/api/follow-up/attachments/${a.id}/download`); + expect(res.status).toBe(404); + }); + }); }); diff --git a/backend/test/services/attachments.service.spec.ts b/backend/test/services/attachments.service.spec.ts index 1669ae8..132777d 100644 --- a/backend/test/services/attachments.service.spec.ts +++ b/backend/test/services/attachments.service.spec.ts @@ -14,7 +14,6 @@ function makeStorageStub(): StorageService { return { upload: vi.fn().mockResolvedValue(undefined), delete: vi.fn().mockResolvedValue(undefined), - getPresignedUrl: vi.fn().mockResolvedValue('https://test/url'), } as unknown as StorageService; } diff --git a/backend/test/services/documents.service.spec.ts b/backend/test/services/documents.service.spec.ts index efb6fe3..8d93d8d 100644 --- a/backend/test/services/documents.service.spec.ts +++ b/backend/test/services/documents.service.spec.ts @@ -51,7 +51,6 @@ describe('DocumentsService', () => { provide: StorageService, useValue: { upload: vi.fn().mockResolvedValue(undefined), - getPresignedUrl: vi.fn().mockResolvedValue('https://presigned-url.example.com/doc'), delete: vi.fn().mockResolvedValue(undefined), }, }, @@ -175,6 +174,26 @@ describe('DocumentsService', () => { }); }); + describe('findById()', () => { + it('returns the document', async () => { + const user = await seedUser(); + const file = createMockFile(); + const created = await service.upload( + file, + { linkedEntity: LinkedEntity.TREATMENT, linkedEntityId: 'treat-1' }, + user.id, + ); + const found = await service.findById(created.id); + expect(found.id).toBe(created.id); + }); + + it('throws NotFoundException for a missing id', async () => { + await expect(service.findById('00000000-0000-0000-0000-000000000000')).rejects.toThrow( + NotFoundException, + ); + }); + }); + describe('delete()', () => { it('deletes document from DB and S3', async () => { const user = await seedUser(); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9f366fc..6f80eea 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -44,9 +44,6 @@ importers: '@aws-sdk/client-s3': specifier: ^3.1043.0 version: 3.1043.0 - '@aws-sdk/s3-request-presigner': - specifier: ^3.1043.0 - version: 3.1043.0 '@nestjs/common': specifier: ^11.1.19 version: 11.1.19(class-transformer@0.5.1)(class-validator@0.15.1)(reflect-metadata@0.2.2)(rxjs@7.8.2) @@ -531,10 +528,6 @@ packages: resolution: {integrity: sha512-CvJ2ZIjK/jVD/lbOpowBVElJyC1YxLTIJ13yM0AEo0t2v7swOzGjSA6lJGH+DwZXQhcjUjoYwc8bVYCX5MDr1A==} engines: {node: '>=20.0.0'} - '@aws-sdk/s3-request-presigner@3.1043.0': - resolution: {integrity: sha512-IgHbDo7c2crU4aGa7FjlMAUUAPgiN4koR060ImbzcFRd2G0v0PXio599O+VDjdmIbOqZwGRQHTbfBKhnDbQZ7g==} - engines: {node: '>=20.0.0'} - '@aws-sdk/signature-v4-multi-region@3.996.25': resolution: {integrity: sha512-+CMIt3e1VzlklAECmG+DtP1sV8iKq25FuA0OKpnJ4KA0kxUtd7CgClY7/RU6VzJBQwbN4EJ9Ue6plvqx1qGadw==} engines: {node: '>=20.0.0'} @@ -555,10 +548,6 @@ packages: resolution: {integrity: sha512-oOZHcRDihk5iEe5V25NVWg45b3qEA8OpHWVdU/XQh8Zj4heVPAJqWvMphQnU7LkufmUo10EpvFPZuQMiFLJK3g==} engines: {node: '>=20.0.0'} - '@aws-sdk/util-format-url@3.972.10': - resolution: {integrity: sha512-DEKiHNJVtNxdyTeQspzY+15Po/kHm6sF0Cs4HV9Q2+lplB63+DrvdeiSoOSdWEWAoO2RcY1veoXVDz2tWxWCgQ==} - engines: {node: '>=20.0.0'} - '@aws-sdk/util-locate-window@3.965.5': resolution: {integrity: sha512-WhlJNNINQB+9qtLtZJcpQdgZw3SCDCpXdUJP7cToGwHbCWCnRckGlc6Bx/OhWwIYFNAn+FIydY8SZ0QmVu3xTQ==} engines: {node: '>=20.0.0'} @@ -6897,17 +6886,6 @@ snapshots: '@smithy/types': 4.14.1 tslib: 2.8.1 - '@aws-sdk/s3-request-presigner@3.1043.0': - dependencies: - '@aws-sdk/signature-v4-multi-region': 3.996.25 - '@aws-sdk/types': 3.973.8 - '@aws-sdk/util-format-url': 3.972.10 - '@smithy/middleware-endpoint': 4.4.32 - '@smithy/protocol-http': 5.3.14 - '@smithy/smithy-client': 4.12.13 - '@smithy/types': 4.14.1 - tslib: 2.8.1 - '@aws-sdk/signature-v4-multi-region@3.996.25': dependencies: '@aws-sdk/middleware-sdk-s3': 3.972.37 @@ -6946,13 +6924,6 @@ snapshots: '@smithy/util-endpoints': 3.4.2 tslib: 2.8.1 - '@aws-sdk/util-format-url@3.972.10': - dependencies: - '@aws-sdk/types': 3.973.8 - '@smithy/querystring-builder': 4.2.14 - '@smithy/types': 4.14.1 - tslib: 2.8.1 - '@aws-sdk/util-locate-window@3.965.5': dependencies: tslib: 2.8.1 From 8f98d2f306b51cc46240b3e81dcda658c563e4e8 Mon Sep 17 00:00:00 2001 From: kneshi <60970253+kneshi@users.noreply.github.com> Date: Mon, 11 May 2026 00:30:46 +0200 Subject: [PATCH 5/6] feat(frontend): open documents via same-origin proxy URL directly --- .../src/components/domain/document-list.tsx | 9 ++------- frontend/src/i18n/en.json | 1 + frontend/src/i18n/fr.json | 1 + .../components/domain/document-list.spec.tsx | 17 ++++++----------- 4 files changed, 10 insertions(+), 18 deletions(-) diff --git a/frontend/src/components/domain/document-list.tsx b/frontend/src/components/domain/document-list.tsx index bf71e50..b779588 100644 --- a/frontend/src/components/domain/document-list.tsx +++ b/frontend/src/components/domain/document-list.tsx @@ -161,13 +161,8 @@ export function DocumentList({ linkedEntity, linkedEntityId }: DocumentListProps } } - const handleDownload = useCallback(async (docId: string) => { - try { - const res = await api.get<{ url: string; filename: string }>(`/documents/${docId}/download`); - globalThis.open(res.url, '_blank'); - } catch { - // error handled by api client - } + const handleDownload = useCallback((docId: string) => { + globalThis.open(`/api/documents/${docId}/download`, '_blank', 'noopener'); }, []); const handleDelete = useCallback( diff --git a/frontend/src/i18n/en.json b/frontend/src/i18n/en.json index b1641fd..0a4c6c7 100644 --- a/frontend/src/i18n/en.json +++ b/frontend/src/i18n/en.json @@ -412,6 +412,7 @@ "roleMatrix.capability.organization.edit": "Edit organization settings, RSS feeds, sync regulatory updates", "roleMatrix.capability.organization.notifications": "Configure notification email toggles", "roleMatrix.capability.screening.write": "Create or convert screenings", + "roleMatrix.capability.docs.read": "Download documents", "roleMatrix.capability.docs.write": "Upload or delete documents", "roleMatrix.capability.vendor.write": "Create or edit vendors and assessments", "roleMatrix.capability.vendor.delete": "Delete vendors", diff --git a/frontend/src/i18n/fr.json b/frontend/src/i18n/fr.json index d3556b6..e6942bc 100644 --- a/frontend/src/i18n/fr.json +++ b/frontend/src/i18n/fr.json @@ -412,6 +412,7 @@ "roleMatrix.capability.organization.edit": "Modifier les paramètres de l'organisation, flux RSS, synchroniser la veille", "roleMatrix.capability.organization.notifications": "Configurer les notifications par e-mail", "roleMatrix.capability.screening.write": "Créer ou convertir des screenings", + "roleMatrix.capability.docs.read": "Télécharger des documents", "roleMatrix.capability.docs.write": "Téléverser ou supprimer des documents", "roleMatrix.capability.vendor.write": "Créer ou modifier des sous-traitants et évaluations", "roleMatrix.capability.vendor.delete": "Supprimer des sous-traitants", diff --git a/frontend/test/components/domain/document-list.spec.tsx b/frontend/test/components/domain/document-list.spec.tsx index 42a23a1..bc590c3 100644 --- a/frontend/test/components/domain/document-list.spec.tsx +++ b/frontend/test/components/domain/document-list.spec.tsx @@ -17,7 +17,7 @@ import type { DocumentDto, UserDto } from '@article30/shared'; // `credentials: 'include'`, and a manually-built `X-XSRF-TOKEN` header read from // the `XSRF-TOKEN` cookie. On success the component re-fetches the list. // - Delete: window.confirm() → api.delete(`/documents/:id`) → local state filter. -// - Download: api.get(`/documents/:id/download`) → { url, filename } → window.open(url, '_blank'). +// - Download: window.open('/api/documents/:id/download', '_blank', 'noopener') - same-origin, no JSON round-trip. const { apiMock, authMock } = vi.hoisted(() => ({ apiMock: { @@ -289,25 +289,20 @@ describe('DocumentList', () => { expect(screen.getByText('A.pdf')).toBeInTheDocument(); }); - it('opens the signed download URL in a new tab when Download is clicked', async () => { + it('opens the same-origin download URL in a new tab when Download is clicked', async () => { authMock.getMe.mockResolvedValueOnce(makeUser(Role.DPO)); apiMock.get.mockResolvedValueOnce([makeDoc({ id: 'doc-X', filename: 'X.pdf' })]); renderList(); await screen.findByText('X.pdf'); - apiMock.get.mockResolvedValueOnce({ - url: 'https://signed.test/X.pdf?sig=abc', - filename: 'X.pdf', - }); - const downloadButton = screen.getByRole('button', { name: /Télécharger|Download/i }); await userEvent.click(downloadButton); - await waitFor(() => { - expect(apiMock.get).toHaveBeenCalledWith('/documents/doc-X/download'); - }); - expect(window.open).toHaveBeenCalledWith('https://signed.test/X.pdf?sig=abc', '_blank'); + // No JSON round-trip; we open the same-origin streaming endpoint directly. + expect(apiMock.get).toHaveBeenCalledTimes(1); // only the initial list fetch + expect(apiMock.get).not.toHaveBeenCalledWith('/documents/doc-X/download'); + expect(window.open).toHaveBeenCalledWith('/api/documents/doc-X/download', '_blank', 'noopener'); }); it('threads linkedEntity and linkedEntityId into the initial list query string', async () => { From 955039af08fd332559b01077263552313a831c33 Mon Sep 17 00:00:00 2001 From: kneshi <60970253+kneshi@users.noreply.github.com> Date: Mon, 11 May 2026 09:21:46 +0200 Subject: [PATCH 6/6] fix(frontend): force download via anchor download attribute instead of new tab --- .../src/components/domain/document-list.tsx | 21 +++++++++-- .../components/domain/document-list.spec.tsx | 37 ++++++++++++++----- 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/frontend/src/components/domain/document-list.tsx b/frontend/src/components/domain/document-list.tsx index b779588..966bfd6 100644 --- a/frontend/src/components/domain/document-list.tsx +++ b/frontend/src/components/domain/document-list.tsx @@ -35,12 +35,15 @@ interface DocumentRowProps { doc: DocumentDto; t: (key: string) => string; canWrite: boolean; - onDownload: (docId: string) => void; + onDownload: (docId: string, filename: string) => void; onDelete: (docId: string) => void; } function DocumentRow({ doc, t, canWrite, onDownload, onDelete }: Readonly) { - const handleDownloadClick = useCallback(() => onDownload(doc.id), [doc.id, onDownload]); + const handleDownloadClick = useCallback( + () => onDownload(doc.id, doc.filename), + [doc.id, doc.filename, onDownload], + ); const handleDeleteClick = useCallback(() => onDelete(doc.id), [doc.id, onDelete]); return ( @@ -161,8 +164,18 @@ export function DocumentList({ linkedEntity, linkedEntityId }: DocumentListProps } } - const handleDownload = useCallback((docId: string) => { - globalThis.open(`/api/documents/${docId}/download`, '_blank', 'noopener'); + const handleDownload = useCallback((docId: string, filename: string) => { + // The backend serves files with Content-Disposition: inline so they preview + // when navigated to directly (e.g. ). The Download button should + // actually download, so we use the HTML `download` attribute - browsers + // honour it for same-origin URLs and override the inline disposition. + const a = document.createElement('a'); + a.href = `/api/documents/${docId}/download`; + a.download = filename; + a.rel = 'noopener'; + document.body.appendChild(a); + a.click(); + a.remove(); }, []); const handleDelete = useCallback( diff --git a/frontend/test/components/domain/document-list.spec.tsx b/frontend/test/components/domain/document-list.spec.tsx index bc590c3..716aaad 100644 --- a/frontend/test/components/domain/document-list.spec.tsx +++ b/frontend/test/components/domain/document-list.spec.tsx @@ -17,7 +17,9 @@ import type { DocumentDto, UserDto } from '@article30/shared'; // `credentials: 'include'`, and a manually-built `X-XSRF-TOKEN` header read from // the `XSRF-TOKEN` cookie. On success the component re-fetches the list. // - Delete: window.confirm() → api.delete(`/documents/:id`) → local state filter. -// - Download: window.open('/api/documents/:id/download', '_blank', 'noopener') - same-origin, no JSON round-trip. +// - Download: builds a programmatic +// and clicks it - same-origin, no JSON round-trip; the `download` attribute overrides +// the backend's Content-Disposition: inline so the browser saves instead of previewing. const { apiMock, authMock } = vi.hoisted(() => ({ apiMock: { @@ -289,20 +291,37 @@ describe('DocumentList', () => { expect(screen.getByText('A.pdf')).toBeInTheDocument(); }); - it('opens the same-origin download URL in a new tab when Download is clicked', async () => { + it('forces a download via a programmatic anchor click when Download is pressed', async () => { authMock.getMe.mockResolvedValueOnce(makeUser(Role.DPO)); apiMock.get.mockResolvedValueOnce([makeDoc({ id: 'doc-X', filename: 'X.pdf' })]); renderList(); await screen.findByText('X.pdf'); - const downloadButton = screen.getByRole('button', { name: /Télécharger|Download/i }); - await userEvent.click(downloadButton); - - // No JSON round-trip; we open the same-origin streaming endpoint directly. - expect(apiMock.get).toHaveBeenCalledTimes(1); // only the initial list fetch - expect(apiMock.get).not.toHaveBeenCalledWith('/documents/doc-X/download'); - expect(window.open).toHaveBeenCalledWith('/api/documents/doc-X/download', '_blank', 'noopener'); + // Spy on anchor.click so we can capture the synthesised the handler creates. + const anchorClicks: HTMLAnchorElement[] = []; + const originalClick = HTMLAnchorElement.prototype.click; + HTMLAnchorElement.prototype.click = function () { + anchorClicks.push(this); + }; + + try { + const downloadButton = screen.getByRole('button', { name: /Télécharger|Download/i }); + await userEvent.click(downloadButton); + + // No JSON round-trip; the handler builds and clicks a same-origin . + expect(apiMock.get).toHaveBeenCalledTimes(1); // only the initial list fetch + expect(apiMock.get).not.toHaveBeenCalledWith('/documents/doc-X/download'); + expect(window.open).not.toHaveBeenCalled(); + + expect(anchorClicks).toHaveLength(1); + const a = anchorClicks[0]; + expect(a.getAttribute('href')).toBe('/api/documents/doc-X/download'); + expect(a.getAttribute('download')).toBe('X.pdf'); + expect(a.getAttribute('rel')).toBe('noopener'); + } finally { + HTMLAnchorElement.prototype.click = originalClick; + } }); it('threads linkedEntity and linkedEntityId into the initial list query string', async () => {