feat: make webhooks and documents editable instead of delete-and-recreate
Both could only be created and deleted. For a webhook that is actively harmful: recreating it issues a new signing secret, so the receiver that verifies the X-ApiCore-Signature header against the old one starts rejecting every delivery without anyone changing it on purpose. Backend: - New PATCH /webhooks/:id behind a new webhooks:update permission, covering the target URL, the event selection and the active flag - the last of which had no way to be set after creation at all, so a misbehaving receiver could only be deleted. The signing secret is deliberately left alone and stays out of the audit log. - A changed webhook URL runs through the same assertPublicWebhookUrl SSRF check as a new one; without that the edit would have been a way around the guard create() applies. Covered by a test. - New PATCH /documents/:id behind a new documents:update permission, for re-linking a document filed against the wrong record and correcting its display name. Re-linking validates the new target with the existing assertEntityExists; the stored file is untouched. Frontend: - New webhook edit dialog. The event labels and the toggle helper moved out of the create dialog into lib/webhooks/labels.ts so both use one copy. - Documents can be renamed from the attachment card. Re-linking a document to a different record has no UI yet: it needs a two-step picker (colony, then inspection) that is not worth building unprompted. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
1 parent
42ceb33844
commit
9a147ee123
18 files changed
+604
-26
No files matched your search
@@ -223,6 +223,7 @@ export const PERMISSION_CATALOG = [
|
||||
[
|
||||
['read', 'Ansehen'],
|
||||
['upload', 'Hochladen'],
|
||||
['update', 'Bearbeiten'],
|
||||
['delete', 'Löschen'],
|
||||
],
|
||||
1800,
|
||||
@@ -233,6 +234,7 @@ export const PERMISSION_CATALOG = [
|
||||
[
|
||||
['read', 'Ansehen'],
|
||||
['create', 'Anlegen'],
|
||||
['update', 'Bearbeiten'],
|
||||
['delete', 'Löschen'],
|
||||
],
|
||||
1900,
|
||||
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
Get,
|
||||
HttpCode,
|
||||
Param,
|
||||
Patch,
|
||||
Post,
|
||||
Query,
|
||||
Res,
|
||||
@@ -21,6 +22,7 @@ import { CurrentUser } from '../auth/decorators/current-user.decorator';
|
||||
import type { AuthenticatedUser } from '../auth/types/authenticated-user';
|
||||
import { isAllowedDocumentMimeType } from './document-mime-types';
|
||||
import { CreateDocumentDto } from './dto/create-document.dto';
|
||||
import { UpdateDocumentDto } from './dto/update-document.dto';
|
||||
import { DocumentsService } from './documents.service';
|
||||
|
||||
const MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024;
|
||||
@@ -105,6 +107,16 @@ export class DocumentsController {
|
||||
}
|
||||
|
||||
@RequirePermission('documents:delete')
|
||||
@RequirePermission('documents:update')
|
||||
@Patch(':id')
|
||||
update(
|
||||
@Param('id') id: string,
|
||||
@Body() dto: UpdateDocumentDto,
|
||||
@CurrentUser() actor: AuthenticatedUser,
|
||||
) {
|
||||
return this.documentsService.update(id, dto, actor.id);
|
||||
}
|
||||
|
||||
@Delete(':id')
|
||||
@HttpCode(204)
|
||||
delete(@Param('id') id: string, @CurrentUser() actor: AuthenticatedUser) {
|
||||
|
||||
@@ -13,6 +13,7 @@ describe('DocumentsService', () => {
|
||||
findMany: jest.Mock;
|
||||
findUnique: jest.Mock;
|
||||
create: jest.Mock;
|
||||
update: jest.Mock;
|
||||
delete: jest.Mock;
|
||||
};
|
||||
apiary: { findUnique: jest.Mock };
|
||||
@@ -41,6 +42,7 @@ describe('DocumentsService', () => {
|
||||
findMany: jest.fn().mockResolvedValue([]),
|
||||
findUnique: jest.fn(),
|
||||
create: jest.fn(),
|
||||
update: jest.fn(),
|
||||
delete: jest.fn(),
|
||||
},
|
||||
apiary: { findUnique: jest.fn() },
|
||||
@@ -175,4 +177,63 @@ describe('DocumentsService', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('update', () => {
|
||||
const existing = {
|
||||
id: 'doc-1',
|
||||
entityType: 'COLONY',
|
||||
entityId: 'colony-1',
|
||||
fileName: 'Rechnung.pdf',
|
||||
mimeType: 'application/pdf',
|
||||
sizeBytes: 1024,
|
||||
storagePath: '/data/doc-1',
|
||||
uploadedByUserId: 'user-1',
|
||||
createdAt: new Date(),
|
||||
};
|
||||
|
||||
it('rejects re-linking to a record that does not exist', async () => {
|
||||
prisma.document.findUnique.mockResolvedValue(existing);
|
||||
prisma.colony.findUnique.mockResolvedValue(null);
|
||||
|
||||
await expect(
|
||||
service.update('doc-1', { entityId: 'missing-colony' }, 'user-2'),
|
||||
).rejects.toBeInstanceOf(NotFoundException);
|
||||
expect(prisma.document.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('re-links a document and logs before and after', async () => {
|
||||
prisma.document.findUnique.mockResolvedValue(existing);
|
||||
prisma.colony.findUnique.mockResolvedValue({ id: 'colony-2' });
|
||||
prisma.document.update.mockResolvedValue({
|
||||
...existing,
|
||||
entityId: 'colony-2',
|
||||
});
|
||||
|
||||
await service.update('doc-1', { entityId: 'colony-2' }, 'user-2');
|
||||
|
||||
expect(auditLog.record).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
entityType: 'Document',
|
||||
entityId: 'doc-1',
|
||||
action: 'UPDATE',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('does not re-check the target when only the file name changes', async () => {
|
||||
prisma.document.findUnique.mockResolvedValue(existing);
|
||||
prisma.document.update.mockResolvedValue({
|
||||
...existing,
|
||||
fileName: 'Rechnung 2026.pdf',
|
||||
});
|
||||
|
||||
await service.update(
|
||||
'doc-1',
|
||||
{ fileName: 'Rechnung 2026.pdf' },
|
||||
'user-2',
|
||||
);
|
||||
|
||||
expect(prisma.colony.findUnique).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -11,6 +11,7 @@ import { AuditLogService } from '../audit/audit-log.service';
|
||||
import { matchesFileSignature } from '../common/file-signature';
|
||||
import { Document, DocumentEntityType } from '../../generated/prisma';
|
||||
import { ALLOWED_DOCUMENT_MIME_TYPES } from './document-mime-types';
|
||||
import { UpdateDocumentDto } from './dto/update-document.dto';
|
||||
|
||||
export type DocumentSummary = Omit<Document, 'storagePath'>;
|
||||
|
||||
@@ -139,6 +140,40 @@ export class DocumentsService {
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-links a document to a different record or corrects its display name.
|
||||
* The stored file is untouched - replacing the content means uploading a
|
||||
* new document.
|
||||
*/
|
||||
async update(
|
||||
id: string,
|
||||
dto: UpdateDocumentDto,
|
||||
actingUserId: string,
|
||||
): Promise<DocumentSummary> {
|
||||
const before = await this.prisma.document.findUnique({ where: { id } });
|
||||
if (!before) {
|
||||
throw new NotFoundException('Document not found');
|
||||
}
|
||||
const entityType = dto.entityType ?? before.entityType;
|
||||
const entityId = dto.entityId ?? before.entityId;
|
||||
if (entityType !== before.entityType || entityId !== before.entityId) {
|
||||
await this.assertEntityExists(entityType, entityId);
|
||||
}
|
||||
const document = await this.prisma.document.update({
|
||||
where: { id },
|
||||
data: { entityType, entityId, fileName: dto.fileName },
|
||||
});
|
||||
await this.auditLog.record({
|
||||
entityType: 'Document',
|
||||
entityId: id,
|
||||
action: 'UPDATE',
|
||||
userId: actingUserId,
|
||||
beforeData: toSummary(before),
|
||||
afterData: toSummary(document),
|
||||
});
|
||||
return toSummary(document);
|
||||
}
|
||||
|
||||
async delete(id: string, actingUserId: string): Promise<void> {
|
||||
const document = await this.prisma.document.findUnique({
|
||||
where: { id },
|
||||
|
||||
@@ -0,0 +1,27 @@
|
||||
import { ApiPropertyOptional } from '@nestjs/swagger';
|
||||
import { IsEnum, IsNotEmpty, IsOptional, IsString } from 'class-validator';
|
||||
import { DocumentEntityType } from '../../../generated/prisma';
|
||||
|
||||
/**
|
||||
* A document filed against the wrong record can be re-linked instead of
|
||||
* deleted and uploaded again, and its display name corrected. The stored file
|
||||
* itself is not touched: replacing the content means uploading a new document.
|
||||
*/
|
||||
export class UpdateDocumentDto {
|
||||
@ApiPropertyOptional({ enum: DocumentEntityType })
|
||||
@IsOptional()
|
||||
@IsEnum(DocumentEntityType)
|
||||
entityType?: DocumentEntityType;
|
||||
|
||||
@ApiPropertyOptional()
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
@IsNotEmpty()
|
||||
entityId?: string;
|
||||
|
||||
@ApiPropertyOptional()
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
@IsNotEmpty()
|
||||
fileName?: string;
|
||||
}
|
||||
@@ -0,0 +1,22 @@
|
||||
import { ApiPropertyOptional, PartialType } from '@nestjs/swagger';
|
||||
import { IsBoolean, IsOptional } from 'class-validator';
|
||||
import { CreateWebhookDto } from './create-webhook.dto';
|
||||
|
||||
/**
|
||||
* A mistyped endpoint or a wrong event selection is correctable instead of
|
||||
* forcing a delete-and-recreate, which would issue a new signing secret and
|
||||
* silently break the receiver that already verifies against the old one.
|
||||
*
|
||||
* A changed URL goes through the same SSRF check as a new one - see
|
||||
* WebhooksService.update.
|
||||
*/
|
||||
export class UpdateWebhookDto extends PartialType(CreateWebhookDto) {
|
||||
/**
|
||||
* Pausing a subscription without losing it. There was no way to set this
|
||||
* after creation, so a misbehaving receiver could only be deleted.
|
||||
*/
|
||||
@ApiPropertyOptional()
|
||||
@IsOptional()
|
||||
@IsBoolean()
|
||||
active?: boolean;
|
||||
}
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
Get,
|
||||
HttpCode,
|
||||
Param,
|
||||
Patch,
|
||||
Post,
|
||||
} from '@nestjs/common';
|
||||
import { ApiBearerAuth, ApiTags } from '@nestjs/swagger';
|
||||
@@ -12,6 +13,7 @@ import { RequirePermission } from '../auth/decorators/require-permission.decorat
|
||||
import { CurrentUser } from '../auth/decorators/current-user.decorator';
|
||||
import type { AuthenticatedUser } from '../auth/types/authenticated-user';
|
||||
import { CreateWebhookDto } from './dto/create-webhook.dto';
|
||||
import { UpdateWebhookDto } from './dto/update-webhook.dto';
|
||||
import { WebhooksService } from './webhooks.service';
|
||||
|
||||
@ApiTags('webhooks')
|
||||
@@ -35,6 +37,16 @@ export class WebhooksController {
|
||||
return this.webhooksService.create(dto, actor.id);
|
||||
}
|
||||
|
||||
@RequirePermission('webhooks:update')
|
||||
@Patch(':id')
|
||||
update(
|
||||
@Param('id') id: string,
|
||||
@Body() dto: UpdateWebhookDto,
|
||||
@CurrentUser() actor: AuthenticatedUser,
|
||||
) {
|
||||
return this.webhooksService.update(id, dto, actor.id);
|
||||
}
|
||||
|
||||
@RequirePermission('webhooks:delete')
|
||||
@Delete(':id')
|
||||
@HttpCode(204)
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import * as crypto from 'node:crypto';
|
||||
import { NotFoundException } from '@nestjs/common';
|
||||
import { BadRequestException, NotFoundException } from '@nestjs/common';
|
||||
import { WebhooksService } from './webhooks.service';
|
||||
import { PrismaService } from '../prisma/prisma.service';
|
||||
import { AuditLogService } from '../audit/audit-log.service';
|
||||
@@ -11,6 +11,7 @@ describe('WebhooksService', () => {
|
||||
findMany: jest.Mock;
|
||||
findUnique: jest.Mock;
|
||||
create: jest.Mock;
|
||||
update: jest.Mock;
|
||||
delete: jest.Mock;
|
||||
};
|
||||
};
|
||||
@@ -23,6 +24,7 @@ describe('WebhooksService', () => {
|
||||
findMany: jest.fn().mockResolvedValue([]),
|
||||
findUnique: jest.fn(),
|
||||
create: jest.fn(),
|
||||
update: jest.fn(),
|
||||
delete: jest.fn(),
|
||||
},
|
||||
};
|
||||
@@ -117,4 +119,68 @@ describe('WebhooksService', () => {
|
||||
service.dispatch('colony.created', { id: 'colony-1' }),
|
||||
).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
describe('update', () => {
|
||||
const existing = {
|
||||
id: 'wh-1',
|
||||
url: 'https://203.0.113.10/hook',
|
||||
events: ['colony.created'],
|
||||
secret: 'a'.repeat(64),
|
||||
createdByUserId: 'user-1',
|
||||
};
|
||||
|
||||
it('applies the SSRF guard to a changed URL, so an edit is no way around it', async () => {
|
||||
prisma.webhookSubscription.findUnique.mockResolvedValue(existing);
|
||||
|
||||
await expect(
|
||||
service.update('wh-1', { url: 'http://localhost/hook' }, 'user-1'),
|
||||
).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(prisma.webhookSubscription.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('keeps the signing secret so a working receiver does not break', async () => {
|
||||
prisma.webhookSubscription.findUnique.mockResolvedValue(existing);
|
||||
prisma.webhookSubscription.update.mockResolvedValue({
|
||||
...existing,
|
||||
events: ['colony.created', 'harvest.created'],
|
||||
});
|
||||
|
||||
await service.update(
|
||||
'wh-1',
|
||||
{ events: ['colony.created', 'harvest.created'] },
|
||||
'user-1',
|
||||
);
|
||||
|
||||
const [[updateArgs]] = prisma.webhookSubscription.update.mock.calls as [
|
||||
[{ data: Record<string, unknown> }],
|
||||
];
|
||||
expect(updateArgs.data).not.toHaveProperty('secret');
|
||||
});
|
||||
|
||||
it('keeps the secret out of the audit log', async () => {
|
||||
prisma.webhookSubscription.findUnique.mockResolvedValue(existing);
|
||||
prisma.webhookSubscription.update.mockResolvedValue(existing);
|
||||
|
||||
await service.update('wh-1', { events: ['colony.created'] }, 'user-1');
|
||||
|
||||
const [[auditArgs]] = auditLog.record.mock.calls as [
|
||||
[
|
||||
{
|
||||
beforeData: Record<string, unknown>;
|
||||
afterData: Record<string, unknown>;
|
||||
},
|
||||
],
|
||||
];
|
||||
expect(auditArgs.beforeData).not.toHaveProperty('secret');
|
||||
expect(auditArgs.afterData).not.toHaveProperty('secret');
|
||||
});
|
||||
|
||||
it('throws NotFoundException for an unknown subscription', async () => {
|
||||
prisma.webhookSubscription.findUnique.mockResolvedValue(null);
|
||||
|
||||
await expect(
|
||||
service.update('missing', { events: ['colony.created'] }, 'user-1'),
|
||||
).rejects.toBeInstanceOf(NotFoundException);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -4,6 +4,7 @@ import { PrismaService } from '../prisma/prisma.service';
|
||||
import { AuditLogService } from '../audit/audit-log.service';
|
||||
import { WebhookSubscription } from '../../generated/prisma';
|
||||
import { CreateWebhookDto } from './dto/create-webhook.dto';
|
||||
import { UpdateWebhookDto } from './dto/update-webhook.dto';
|
||||
import { WebhookEvent } from './webhook-events';
|
||||
import { assertPublicWebhookUrl } from './webhook-url-guard';
|
||||
|
||||
@@ -57,6 +58,42 @@ export class WebhooksService {
|
||||
return subscription;
|
||||
}
|
||||
|
||||
/**
|
||||
* Corrects an existing subscription. The signing secret deliberately stays
|
||||
* the same - it identifies this subscription to the receiver, and rotating
|
||||
* it on an unrelated edit would break a working integration.
|
||||
*/
|
||||
async update(
|
||||
id: string,
|
||||
dto: UpdateWebhookDto,
|
||||
actingUserId: string,
|
||||
): Promise<WebhookSubscription> {
|
||||
const before = await this.prisma.webhookSubscription.findUnique({
|
||||
where: { id },
|
||||
});
|
||||
if (!before) {
|
||||
throw new NotFoundException('Webhook subscription not found');
|
||||
}
|
||||
// A changed target is as dangerous as a new one: without this check an
|
||||
// edit would be a way around the SSRF guard that create() applies.
|
||||
if (dto.url) {
|
||||
await assertPublicWebhookUrl(dto.url);
|
||||
}
|
||||
const subscription = await this.prisma.webhookSubscription.update({
|
||||
where: { id },
|
||||
data: { url: dto.url, events: dto.events, active: dto.active },
|
||||
});
|
||||
await this.auditLog.record({
|
||||
entityType: 'WebhookSubscription',
|
||||
entityId: id,
|
||||
action: 'UPDATE',
|
||||
userId: actingUserId,
|
||||
beforeData: this.omitSecret(before),
|
||||
afterData: this.omitSecret(subscription),
|
||||
});
|
||||
return subscription;
|
||||
}
|
||||
|
||||
async delete(id: string, actingUserId: string): Promise<void> {
|
||||
const subscription = await this.prisma.webhookSubscription.findUnique({
|
||||
where: { id },
|
||||
|
||||
Reference in new issue
Block a user