From ba5c74a676ec2f0fd52bfc9331f825d5fbd8d7a1 Mon Sep 17 00:00:00 2001 From: jmgasper Date: Fri, 4 Sep 2026 08:47:34 +1000 Subject: [PATCH] fix(PM-6169): correct forum command and deletion policy --- README.md | 2 +- .../forums-access-policy.service.spec.ts | 26 ++++++++++- src/forums/forums-access-policy.service.ts | 18 +++++++- src/forums/forums-command.service.spec.ts | 43 +++++++++++++++++++ src/forums/forums-command.service.ts | 11 +++-- .../forums-moderation.integration.spec.ts | 4 +- 6 files changed, 96 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index ce151ab..bd9f144 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ Topic reads are exposed under `read:forums-topics`. `GET /v6/forums/topics` retu Human members set or switch their one-per-post reaction with `PUT /v6/forums/posts/:postId/reaction` and `{ "reaction": "THUMBS_UP" }` or `{ "reaction": "THUMBS_DOWN" }`. `DELETE` on the same route idempotently removes the member's reaction. Both commands return the resulting `viewerReaction` and current shared counts, enforce runtime bans and inherited post visibility, and reject deleted posts. They remain available on locked topics because reactions do not change discussion content. The reaction routes are human-member-only; M2M callers cannot own reaction state. -Top-level non-challenge topics may be created by human admins and scoped M2M callers. Top-level challenge topics may be created by eligible challenge members, challenge copilots, and admins; M2M callers cannot create challenge roots. Regular authenticated members may create child topics only under parents they can see and only when the resolved effective child context remains non-challenge; requests that inherit or introduce a non-null `challengeId` under a parent are rejected before writes. Allowed child topics must keep monotonic role restrictions: inherited roles cannot be cleared or replaced. Challenge-scoped visibility is verified through the configured challenge and resource adapters, including challenge-resource membership and challenge-copilot elevation. Challenge copilots may access or moderate challenge-scoped forums only when any effective `roleName` forum restriction is also satisfied; admin and scoped M2M bypass behavior is unchanged. Active member bans and trusted exact-IP bans return 403 for human writes before content, watch, or read-state changes. M2M on-behalf watch and read-state commands enforce active bans on the resolved target member and do not evaluate IP bans. Locked topics reject child-topic creation under the locked parent, replies, topic updates/deletes, and post updates/deletes unless the actor is an administrator or a human challenge copilot acting on a challenge-scoped topic. +Top-level non-challenge topics may be created by human admins and scoped M2M callers. Top-level challenge topics may be created by eligible challenge members, challenge copilots, and admins; M2M callers cannot create challenge roots. Regular authenticated members may create child topics only under parents they can see and only when the resolved effective child context remains non-challenge; requests that inherit or introduce a non-null `challengeId` under a parent are rejected before writes. Allowed child topics must keep monotonic role restrictions: inherited roles cannot be cleared or replaced. Challenge-scoped visibility is verified through the configured challenge and resource adapters, including challenge-resource membership and challenge-copilot elevation. Challenge copilots may access or moderate challenge-scoped forums only when any effective `roleName` forum restriction is also satisfied; admin and scoped M2M bypass behavior is unchanged. Active member bans and trusted exact-IP bans return 403 for human writes before content, watch, or read-state changes. M2M on-behalf watch and read-state commands enforce active bans on the resolved target member and do not evaluate IP bans. Locked topics reject child-topic creation under the locked parent, replies, topic updates/deletes, and post updates/deletes unless the actor is an administrator or a human challenge copilot acting on a challenge-scoped topic. Topic authors may edit their own topics, but deletion remains limited to administrators and scoped M2M callers; challenge-copilot elevation does not grant deletion. Optional command DTO fields whose transformed runtime value is `undefined` are treated as omitted, including post parent fields and announcement state. Moderation management is exposed under `/v6/forums/moderation`. Human callers must have the case-insensitive `administrator` role; M2M callers must have `moderate:forums`. Human tokens do not gain moderation-route access from scopes alone, and M2M tokens do not gain access from roles alone. Challenge copilots do not gain moderation-endpoint access unless they are also administrators. Topic lock/unlock endpoints return `topicId`, `locked`, `lockedBy`, `lockedAt`, and `updatedAt`. Member and IP ban endpoints return the persisted ban row plus an `active` flag. Ban audit columns store the human administrator member id when available; M2M moderation stores null audit member ids. IP moderation accepts only exact bare IPv4 or IPv6 host values and rejects CIDR, wildcards, comma-delimited values, bracketed IPv6, host:port, quoted values, and invalid text. diff --git a/src/forums/forums-access-policy.service.spec.ts b/src/forums/forums-access-policy.service.spec.ts index 155907a..ae6aeeb 100644 --- a/src/forums/forums-access-policy.service.spec.ts +++ b/src/forums/forums-access-policy.service.spec.ts @@ -233,7 +233,10 @@ describe('ForumsAccessPolicyService', () => { expect(topicDecisions.canView).toEqual({ allowed: true }); expect(topicDecisions.canUpdateTopic).toEqual({ allowed: true }); - expect(topicDecisions.canDeleteTopic).toEqual({ allowed: true }); + expect(topicDecisions.canDeleteTopic).toEqual({ + allowed: false, + reason: 'Only an administrator may delete a topic.', + }); expect(topicDecisions.canControlAnnouncement).toEqual({ allowed: true }); expect(postDecisions.canUpdatePost).toEqual({ allowed: true }); expect(postDecisions.canDeletePost).toEqual({ allowed: true }); @@ -319,9 +322,28 @@ describe('ForumsAccessPolicyService', () => { expect(createDecisions.canControlAnnouncement).toEqual({ allowed: true }); expect(topicDecisions.canView).toEqual({ allowed: true }); expect(topicDecisions.canUpdateTopic).toEqual({ allowed: true }); + expect(topicDecisions.canDeleteTopic.allowed).toBe(false); expect(topicDecisions.canControlAnnouncement).toEqual({ allowed: true }); }); + it('lets an author edit but not delete their own topic', async () => { + const { service } = createPolicyHarness(); + const decisions = await service.decideForTopic( + makePrincipal({ memberId: 'author-1' }), + makeTopicContext({ + effectiveChallengeId: null, + effectiveRoleName: null, + isTopicAuthor: true, + }), + ); + + expect(decisions.canUpdateTopic).toEqual({ allowed: true }); + expect(decisions.canDeleteTopic).toEqual({ + allowed: false, + reason: 'Only an administrator may delete a topic.', + }); + }); + it('allows general public child-topic creation', async () => { const { service } = createPolicyHarness(); const principal = makePrincipal(); @@ -490,6 +512,8 @@ describe('ForumsAccessPolicyService', () => { expect(adminTopicDecisions.canUpdateTopic).toEqual({ allowed: true }); expect(machineTopicDecisions.canUpdateTopic).toEqual({ allowed: true }); + expect(adminTopicDecisions.canDeleteTopic).toEqual({ allowed: true }); + expect(machineTopicDecisions.canDeleteTopic).toEqual({ allowed: true }); expect( challengeAccessService.getChallengeAccessFacts, ).not.toHaveBeenCalled(); diff --git a/src/forums/forums-access-policy.service.ts b/src/forums/forums-access-policy.service.ts index 88e430e..cdd4712 100644 --- a/src/forums/forums-access-policy.service.ts +++ b/src/forums/forums-access-policy.service.ts @@ -85,6 +85,7 @@ export class ForumsAccessPolicyService { : this.deny( 'Only the author or an elevated forums actor may modify it.', ); + const canDeleteTopic = this.evaluateTopicDeletion(evaluation); const canControlAnnouncement = this.evaluateAnnouncementControl(evaluation); return { @@ -93,7 +94,7 @@ export class ForumsAccessPolicyService { canCreateChildTopic: evaluation.visibility, canCreatePost: evaluation.visibility, canUpdateTopic: canMutateContent, - canDeleteTopic: canMutateContent, + canDeleteTopic, canAddWatch: evaluation.visibility, canRemoveWatch: evaluation.visibility, canMarkRead: evaluation.visibility, @@ -496,6 +497,21 @@ export class ForumsAccessPolicyService { : this.deny('Announcement control requires elevated forums access.'); } + /** + * Applies legacy-compatible topic deletion rules independently of editing. + * + * @param evaluation Restriction evaluation for the persisted topic. + * @returns Decision allowing administrators and scoped M2M callers, but not authors or challenge copilots. + * @throws Does not throw. + */ + private evaluateTopicDeletion( + evaluation: RestrictionEvaluation, + ): ForumsAccessDecision { + return evaluation.isElevated && !evaluation.isChallengeCopilot + ? this.allow() + : this.deny('Only an administrator may delete a topic.'); + } + /** * Applies the approved root-topic creation rules. * diff --git a/src/forums/forums-command.service.spec.ts b/src/forums/forums-command.service.spec.ts index 0b22530..22c21af 100644 --- a/src/forums/forums-command.service.spec.ts +++ b/src/forums/forums-command.service.spec.ts @@ -1,6 +1,7 @@ import { Logger } from '@nestjs/common'; import { JwtUser } from '../auth/jwt.service'; import { Post, PostReactionType, Topic } from '../../prisma/generated/client'; +import { CreatePostDto, UpdateTopicDto } from './dto/forums-command.dto'; import { ForumsCommandService } from './forums-command.service'; const createdAt = new Date('2026-06-04T00:00:00.000Z'); @@ -277,6 +278,22 @@ describe('ForumsCommandService notification boundary', () => { ); }); + it('treats transformed undefined post parent fields as omitted', async () => { + const { service, tx } = createCommandService(); + const dto = new CreatePostDto(); + dto.content = 'Persisted content'; + + await expect(service.createPost('topic-1', dto, user)).resolves.toEqual( + expect.objectContaining({ id: 'post-1' }), + ); + expect(tx.post.create).toHaveBeenCalledWith({ + data: expect.objectContaining({ + parentId: 'topic-1', + parentType: 'TOPIC', + }), + }); + }); + it('attempts starter-post notification for a child createTopic command', async () => { const order: string[] = []; const { notificationService, service, tx } = createCommandService(order); @@ -498,6 +515,32 @@ describe('ForumsCommandService notification boundary', () => { expect(tx.topic.update).not.toHaveBeenCalled(); }); + it('ignores transformed undefined announcement state during a title-only update', async () => { + const { accessPolicyService, service, tx } = createCommandService(); + accessPolicyService.decideForTopic.mockResolvedValue({ + canAddWatch: { allowed: true }, + canCreatePost: { allowed: true }, + canControlAnnouncement: { + allowed: false, + reason: 'Announcement control requires elevated forums access.', + }, + canDeleteTopic: { allowed: false }, + canMarkRead: { allowed: true }, + canRemoveWatch: { allowed: true }, + canUpdateTopic: { allowed: true }, + }); + const dto = new UpdateTopicDto(); + dto.title = 'Updated title'; + + await expect(service.updateTopic('topic-1', dto, user)).resolves.toEqual( + expect.objectContaining({ id: 'topic-1' }), + ); + expect(tx.topic.update).toHaveBeenCalledWith({ + data: { title: 'Updated title' }, + where: { id: 'topic-1' }, + }); + }); + it('rejects locked role updates before restriction or descendant checks', async () => { const { accessPolicyService, db, moderationService, service } = createCommandService(); diff --git a/src/forums/forums-command.service.ts b/src/forums/forums-command.service.ts index 3c972b9..fea1b48 100644 --- a/src/forums/forums-command.service.ts +++ b/src/forums/forums-command.service.ts @@ -1595,17 +1595,22 @@ export class ForumsCommandService { } /** - * Checks whether an object owns a property supplied by the request body. + * Checks whether an object owns a defined property supplied by the request body. + * Class-transformer DTO instances can own optional fields whose runtime value + * is `undefined`; those fields remain omitted for command semantics. * * @param value Object to inspect. * @param key Property key to check. - * @returns `true` when the property exists on the object. + * @returns `true` when the property exists and is not `undefined`. * @throws Does not throw. */ private hasOwn( value: T, key: K, ): value is T & Record { - return Object.prototype.hasOwnProperty.call(value, key); + return ( + Object.prototype.hasOwnProperty.call(value, key) && + (value as T & Record)[key] !== undefined + ); } } diff --git a/src/forums/forums-moderation.integration.spec.ts b/src/forums/forums-moderation.integration.spec.ts index fada418..f8d6dc0 100644 --- a/src/forums/forums-moderation.integration.spec.ts +++ b/src/forums/forums-moderation.integration.spec.ts @@ -195,7 +195,7 @@ function createModerationDb(seed: ModerationSeedData) { memberBanCounter += 1; seed.memberBans.push(ban); - return ban; + return Promise.resolve(ban); }, update: (args: { where: { id: string }; @@ -243,7 +243,7 @@ function createModerationDb(seed: ModerationSeedData) { ipBanCounter += 1; seed.ipBans.push(ban); - return ban; + return Promise.resolve(ban); }, update: (args: { where: { id: string };