From 11b2c55485ec2a75d10408d0882d939e59fb7535 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 17:48:08 +0300 Subject: [PATCH] =?UTF-8?q?feat(audit):=20DB-backed=20audit=20trail=20?= =?UTF-8?q?=D0=B2=D0=BC=D0=B5=D1=81=D1=82=D0=BE=20Noop=20(#496)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Событие comment.suggestion_applied/dismissed раньше улетало в NoopAuditService и молча терялось; при childless-ветке apply/dismiss комментарий hard-delete'ится, поэтому восстановить, кто и что решил, было невозможно. - DatabaseAuditService пишет в уже существующую таблицу `audit` (миграция 20260228T223532); actor/workspace/ip берутся из CLS AuditContext, вне HTTP — из явного контекста (logWithContext). Аудит — побочная запись: сбой записи не ломает исходный запрос, EXCLUDED_AUDIT_EVENTS отбрасываются. - Биндинг AUDIT_SERVICE переключён с Noop на DatabaseAuditService. - payload apply/dismiss дополнен suggestedText/selection/commentAuthor/ decidedBy — на childless-ветке это единственная уцелевшая запись. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/server/src/app.module.ts | 4 +- .../comment.service.apply-suggestion.spec.ts | 8 + ...comment.service.dismiss-suggestion.spec.ts | 10 + .../src/core/comment/comment.service.ts | 29 ++- .../src/integrations/audit/audit.module.ts | 11 +- .../audit/database-audit.service.spec.ts | 171 ++++++++++++++++++ .../audit/database-audit.service.ts | 160 ++++++++++++++++ 7 files changed, 384 insertions(+), 9 deletions(-) create mode 100644 apps/server/src/integrations/audit/database-audit.service.spec.ts create mode 100644 apps/server/src/integrations/audit/database-audit.service.ts diff --git a/apps/server/src/app.module.ts b/apps/server/src/app.module.ts index 527035b9..c46310f6 100644 --- a/apps/server/src/app.module.ts +++ b/apps/server/src/app.module.ts @@ -25,7 +25,7 @@ import { CacheModule } from '@nestjs/cache-manager'; import KeyvRedis from '@keyv/redis'; import { LoggerModule } from './common/logger/logger.module'; import { ClsModule } from 'nestjs-cls'; -import { NoopAuditModule } from './integrations/audit/audit.module'; +import { AuditModule } from './integrations/audit/audit.module'; import { ThrottleModule } from './integrations/throttle/throttle.module'; import { McpModule } from './integrations/mcp/mcp.module'; import { SandboxModule } from './integrations/sandbox/sandbox.module'; @@ -55,7 +55,7 @@ try { middleware: { mount: true }, }), LoggerModule, - NoopAuditModule, + AuditModule, CoreModule, DatabaseModule, EnvironmentModule, diff --git a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts index 486f89bb..bcfc2f2c 100644 --- a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts @@ -146,11 +146,19 @@ describe('CommentService — applySuggestion', () => { 'page-1', expect.objectContaining({ operation: 'commentDeleted', commentId: 'c-1' }), ); + // #496: hard-deleted row → the audit payload is the only surviving record. expect(auditService.log).toHaveBeenCalledWith( expect.objectContaining({ event: AuditEvent.COMMENT_SUGGESTION_APPLIED, resourceType: AuditResource.COMMENT, resourceId: 'c-1', + metadata: expect.objectContaining({ + pageId: 'page-1', + suggestedText: 'new text', + selection: 'old text', + commentAuthor: 'user-1', + decidedBy: 'user-1', + }), }), ); expect(result.outcome).toBe('deleted'); diff --git a/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts index 819071ff..1899377a 100644 --- a/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.dismiss-suggestion.spec.ts @@ -107,11 +107,21 @@ describe('CommentService — dismissSuggestion', () => { 'page-1', expect.objectContaining({ operation: 'commentDeleted', commentId: 'c-1' }), ); + // #496: the row is hard-deleted, so the audit payload must carry the + // decision's substance (what was suggested, the anchored text, who authored + // it, who decided) — it is the only surviving record. expect(auditService.log).toHaveBeenCalledWith( expect.objectContaining({ event: AuditEvent.COMMENT_SUGGESTION_DISMISSED, resourceType: AuditResource.COMMENT, resourceId: 'c-1', + metadata: expect.objectContaining({ + pageId: 'page-1', + suggestedText: 'new text', + selection: 'old text', + commentAuthor: 'user-1', + decidedBy: 'user-1', + }), }), ); expect(result.outcome).toBe('deleted'); diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index ef97c108..c053ccf8 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -524,7 +524,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return { ...updatedComment, outcome: 'resolved' }; } @@ -538,7 +538,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return settled; } @@ -597,7 +597,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return { ...updatedComment, outcome: 'resolved' }; @@ -616,7 +616,7 @@ export class CommentService { resourceType: AuditResource.COMMENT, resourceId: comment.id, spaceId: comment.spaceId, - metadata: { pageId: comment.pageId }, + metadata: this.suggestionAuditMetadata(comment, user), }); return settled; @@ -732,6 +732,27 @@ export class CommentService { return this.generalQueue.add(QueueJob.COMMENT_MARK_UPDATE, jobData); } + /** + * Build the audit metadata for a suggestion apply/dismiss decision (#496). + * The subject comment is HARD-DELETED on the childless path, so the audit row + * is the only surviving record — capture the decision's substance (what was + * suggested, the anchored text it replaced, who authored it, who decided) + * before the row can vanish. `decidedBy` is the acting user; `commentAuthor` + * is the suggestion's creator. + */ + private suggestionAuditMetadata( + comment: Comment, + user: User, + ): Record { + return { + pageId: comment.pageId, + suggestedText: comment.suggestedText ?? null, + selection: comment.selection ?? null, + commentAuthor: comment.creatorId ?? null, + decidedBy: user.id, + }; + } + private async queueCommentNotification( content: any, oldMentionIds: string[], diff --git a/apps/server/src/integrations/audit/audit.module.ts b/apps/server/src/integrations/audit/audit.module.ts index eafffdd7..07ef04d3 100644 --- a/apps/server/src/integrations/audit/audit.module.ts +++ b/apps/server/src/integrations/audit/audit.module.ts @@ -1,14 +1,19 @@ import { Global, Module } from '@nestjs/common'; -import { AUDIT_SERVICE, NoopAuditService } from './audit.service'; +import { AUDIT_SERVICE } from './audit.service'; +import { DatabaseAuditService } from './database-audit.service'; +// #496: bind the audit token to a real DB-backed trail (was NoopAuditService, +// which silently dropped every event). Kysely (@Global DatabaseModule) and +// ClsService (@Global ClsModule) are both globally available, so this module +// needs no extra imports. @Global() @Module({ providers: [ { provide: AUDIT_SERVICE, - useClass: NoopAuditService, + useClass: DatabaseAuditService, }, ], exports: [AUDIT_SERVICE], }) -export class NoopAuditModule {} +export class AuditModule {} diff --git a/apps/server/src/integrations/audit/database-audit.service.spec.ts b/apps/server/src/integrations/audit/database-audit.service.spec.ts new file mode 100644 index 00000000..90456c72 --- /dev/null +++ b/apps/server/src/integrations/audit/database-audit.service.spec.ts @@ -0,0 +1,171 @@ +import { DatabaseAuditService } from './database-audit.service'; +import { AUDIT_CONTEXT_KEY } from '../../common/middlewares/audit-context.middleware'; +import { AuditEvent, AuditResource } from '../../common/events/audit-events'; + +/** + * Observable-property coverage for the DB-backed audit trail (#496): every + * assertion pins what actually reaches the `audit` table (or that nothing does), + * driven through a chainable Kysely mock that captures the inserted rows. + */ +describe('DatabaseAuditService', () => { + function makeService(clsContext: any) { + const inserted: any[] = []; + const updated: any[] = []; + let failNextInsert = false; + + const db: any = { + insertInto: jest.fn(() => ({ + values: jest.fn((rows: any) => ({ + execute: jest.fn(async () => { + if (failNextInsert) { + failNextInsert = false; + throw new Error('boom'); + } + inserted.push(rows); + }), + })), + })), + updateTable: jest.fn(() => ({ + set: jest.fn((patch: any) => ({ + where: jest.fn(() => ({ + execute: jest.fn(async () => { + updated.push(patch); + }), + })), + })), + })), + }; + + const store: Record = { [AUDIT_CONTEXT_KEY]: clsContext }; + const cls: any = { + get: jest.fn((key: string) => store[key]), + set: jest.fn((key: string, val: any) => { + store[key] = val; + }), + }; + + const service = new DatabaseAuditService(db, cls); + return { + service, + inserted, + updated, + cls, + store, + failInsert: () => { + failNextInsert = true; + }, + }; + } + + const applyPayload = () => ({ + event: AuditEvent.COMMENT_SUGGESTION_APPLIED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-1', + spaceId: 'space-1', + metadata: { pageId: 'p-1', suggestedText: 'new', decidedBy: 'u-2' }, + }); + + const ctx = (over?: any) => ({ + workspaceId: 'ws-1', + actorId: 'u-2', + actorType: 'user', + ipAddress: '10.0.0.1', + userAgent: 'jest', + ...over, + }); + + it('log() persists a row with the CLS context merged onto the payload', async () => { + const { service, inserted } = makeService(ctx()); + service.log(applyPayload()); + // log() is fire-and-forget; flush the microtask queue. + await Promise.resolve(); + await Promise.resolve(); + + expect(inserted).toHaveLength(1); + expect(inserted[0]).toMatchObject({ + workspaceId: 'ws-1', + actorId: 'u-2', + actorType: 'user', + event: AuditEvent.COMMENT_SUGGESTION_APPLIED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-1', + spaceId: 'space-1', + ipAddress: '10.0.0.1', + metadata: { pageId: 'p-1', suggestedText: 'new', decidedBy: 'u-2' }, + }); + }); + + it('log() is a no-op when there is no workspace in scope', async () => { + const { service, inserted } = makeService(ctx({ workspaceId: null })); + service.log(applyPayload()); + await Promise.resolve(); + expect(inserted).toHaveLength(0); + }); + + it('log() drops EXCLUDED_AUDIT_EVENTS (e.g. comment.created)', async () => { + const { service, inserted } = makeService(ctx()); + service.log({ + event: AuditEvent.COMMENT_CREATED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-1', + spaceId: 'space-1', + }); + await Promise.resolve(); + await Promise.resolve(); + expect(inserted).toHaveLength(0); + }); + + it('a failed insert never throws to the caller (audit is a side-record)', async () => { + const { service, failInsert } = makeService(ctx()); + failInsert(); + // Must not reject / throw. + expect(() => service.log(applyPayload())).not.toThrow(); + await Promise.resolve(); + await Promise.resolve(); + }); + + it('logWithContext() persists with an explicit (non-request) context', async () => { + const { service, inserted } = makeService(undefined); + service.logWithContext(applyPayload(), ctx({ actorType: 'system' }) as any); + await Promise.resolve(); + await Promise.resolve(); + expect(inserted).toHaveLength(1); + expect(inserted[0].actorType).toBe('system'); + expect(inserted[0].workspaceId).toBe('ws-1'); + }); + + it('logBatchWithContext() inserts only non-excluded events', async () => { + const { service, inserted } = makeService(undefined); + service.logBatchWithContext( + [ + applyPayload(), + { + event: AuditEvent.COMMENT_CREATED, + resourceType: AuditResource.COMMENT, + resourceId: 'c-2', + }, + ], + ctx() as any, + ); + await Promise.resolve(); + await Promise.resolve(); + // Batch is a single insert call carrying only the applied event. + expect(inserted).toHaveLength(1); + expect(inserted[0]).toHaveLength(1); + expect(inserted[0][0].event).toBe(AuditEvent.COMMENT_SUGGESTION_APPLIED); + }); + + it('setActorId / setActorType mutate the ambient CLS context', () => { + const { service, store } = makeService(ctx({ actorId: null })); + service.setActorId('u-9'); + service.setActorType('api_key'); + expect(store[AUDIT_CONTEXT_KEY].actorId).toBe('u-9'); + expect(store[AUDIT_CONTEXT_KEY].actorType).toBe('api_key'); + }); + + it('updateRetention() writes the workspace retention window', async () => { + const { service, updated } = makeService(ctx()); + await service.updateRetention('ws-1', 30); + expect(updated).toEqual([{ auditRetentionDays: 30 }]); + }); +}); diff --git a/apps/server/src/integrations/audit/database-audit.service.ts b/apps/server/src/integrations/audit/database-audit.service.ts new file mode 100644 index 00000000..cfa3f321 --- /dev/null +++ b/apps/server/src/integrations/audit/database-audit.service.ts @@ -0,0 +1,160 @@ +import { Injectable, Logger } from '@nestjs/common'; +import { InjectKysely } from 'nestjs-kysely'; +import { ClsService } from 'nestjs-cls'; +import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { + AuditContext, + AUDIT_CONTEXT_KEY, +} from '../../common/middlewares/audit-context.middleware'; +import { + AuditLogPayload, + ActorType, + EXCLUDED_AUDIT_EVENTS, +} from '../../common/events/audit-events'; +import { AuditLogContext, IAuditService } from './audit.service'; + +/** + * Minimal DB-backed audit trail (#496). Replaces NoopAuditService so that + * decision-bearing events — notably comment.suggestion_applied / + * comment.suggestion_dismissed, whose subject comment is HARD-DELETED on the + * childless path — leave a durable record of who decided what. Without this the + * events were emitted (comment.service / *.controller) but swallowed, so an + * applied/dismissed suggestion was unrecoverable once the row was gone. + * + * Rows land in the pre-existing `audit` table (migration 20260228T223532). The + * per-request actor/workspace/ip come from the CLS AuditContext populated by + * AuditContextMiddleware + AuditActorInterceptor; callers that run OUTSIDE a + * request (queue workers, imports) pass an explicit context via + * logWithContext / logBatchWithContext. + * + * Audit is a side-record: a write failure MUST NOT break the originating + * request, so every persistence path swallows its error with a warn. Events in + * EXCLUDED_AUDIT_EVENTS (high-volume/low-signal) are dropped. + */ +@Injectable() +export class DatabaseAuditService implements IAuditService { + private readonly logger = new Logger(DatabaseAuditService.name); + + constructor( + @InjectKysely() private readonly db: KyselyDB, + private readonly cls: ClsService, + ) {} + + /** + * Persist a single event using the ambient request-scoped AuditContext. A + * no-op when there is no workspace in scope (the table's workspace_id is NOT + * NULL) or the event is excluded. Fire-and-forget: the returned promise is not + * awaited by hot callers, and its rejection is swallowed here. + */ + log(payload: AuditLogPayload): void { + const context = this.cls?.get(AUDIT_CONTEXT_KEY); + if (!context?.workspaceId) { + // No workspace in scope — nothing we can attribute the row to. This is + // expected for events emitted outside an HTTP request; those callers must + // use logWithContext instead. + return; + } + void this.persist(payload, { + workspaceId: context.workspaceId, + actorId: context.actorId ?? undefined, + actorType: context.actorType, + ipAddress: context.ipAddress ?? undefined, + userAgent: context.userAgent ?? undefined, + }); + } + + /** Persist a single event with an explicit (non-request) context. */ + logWithContext(payload: AuditLogPayload, context: AuditLogContext): void { + if (!context?.workspaceId) return; + void this.persist(payload, context); + } + + /** Persist a batch of events sharing one explicit context (imports). */ + logBatchWithContext( + payloads: AuditLogPayload[], + context: AuditLogContext, + ): void { + if (!context?.workspaceId || payloads.length === 0) return; + const rows = payloads + .filter((p) => !EXCLUDED_AUDIT_EVENTS.has(p.event)) + .map((p) => this.toRow(p, context)); + if (rows.length === 0) return; + this.db + .insertInto('audit') + .values(rows) + .execute() + .catch((err: any) => + this.logger.warn(`Failed to persist ${rows.length} audit events: ${err?.message}`), + ); + } + + /** Update the ambient request actor (e.g. after login resolves the user). */ + setActorId(actorId: string): void { + const context = this.cls?.get(AUDIT_CONTEXT_KEY); + if (context) { + context.actorId = actorId; + this.cls.set(AUDIT_CONTEXT_KEY, context); + } + } + + /** Update the ambient request actor type (user | system | api_key). */ + setActorType(actorType: ActorType): void { + const context = this.cls?.get(AUDIT_CONTEXT_KEY); + if (context) { + context.actorType = actorType; + this.cls.set(AUDIT_CONTEXT_KEY, context); + } + } + + /** Persist a workspace's audit-log retention window (days). */ + async updateRetention( + workspaceId: string, + retentionDays: number, + ): Promise { + try { + await this.db + .updateTable('workspaces') + .set({ auditRetentionDays: retentionDays }) + .where('id', '=', workspaceId) + .execute(); + } catch (err: any) { + this.logger.warn( + `Failed to update audit retention for workspace ${workspaceId}: ${err?.message}`, + ); + } + } + + private async persist( + payload: AuditLogPayload, + context: AuditLogContext, + ): Promise { + if (EXCLUDED_AUDIT_EVENTS.has(payload.event)) return; + try { + await this.db + .insertInto('audit') + .values(this.toRow(payload, context)) + .execute(); + } catch (err: any) { + // Audit is a side-record; never let a failed write surface to the caller. + this.logger.warn( + `Failed to persist audit event ${payload.event}: ${err?.message}`, + ); + } + } + + private toRow(payload: AuditLogPayload, context: AuditLogContext) { + return { + workspaceId: context.workspaceId, + actorId: context.actorId ?? null, + actorType: context.actorType ?? 'user', + event: payload.event, + resourceType: payload.resourceType, + resourceId: payload.resourceId ?? null, + spaceId: payload.spaceId ?? null, + // jsonb columns: node-postgres serializes plain objects to JSON. + changes: payload.changes ? (payload.changes as any) : null, + metadata: payload.metadata ? (payload.metadata as any) : null, + ipAddress: context.ipAddress ?? null, + }; + } +}