From fa3e4279da68069891a239ff2f172581248d0cda Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:34:08 -0700 Subject: [PATCH] fix(authz): createdBy stops counting as membership, and the creator cannot leave (TASK-166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wren's ruling: `leavePod` refuses a creator's leave, `createdBy` stays as the record of who made the pod, and the permissive sites move to the strict predicate. Both sides of the activity pair move — the read rule and the write gate that mirrored it. Thirteen predicate terms across five files stop reading `createdBy` as membership, and one guard is added: - `controllers/podController.ts` — `leavePod` refuses the creator with 409 `creator_cannot_leave`. `createdBy` is written only at creation and nothing transfers it, while `removeMember` is gated on it with no admin fallback, so a creator who left would strand the pod with nobody able to remove a member. - `routes/podInvites.ts` (3 sites), `routes/activity.ts` (2) and `services/decisionRequestService.ts` (1) — the strict predicate, imported from `utils/isPodMember` rather than the module's permissive default. All four files bound the default, which is the permissive rule. - `services/activityService.ts` (7 terms) — the `createdBy` arm is dropped from `getRecap`, `getUserFeed`, `getDecisionHistory` (query and filter), `getPodFeed`, the legacy approval gate, and `getPendingApprovals`, so the read rule and the write gate that mirrors it are the same rule. One live defect surfaced while doing it, and it is the reason the `getPodFeed` control arm exists: that function carried a THIRD hand-rolled copy of the rule, `String(member.userId) || String(m)`. `String(undefined)` is the non-empty string 'undefined', so the fallback was unreachable and no member listed as a plain ObjectId ever matched. In production only the `createdBy` term beside it ever matched, which made `GET /api/activity/pods/:podId` creator-only. Narrowing that term without the fix would have refused every caller; the arm caught it, and the copy now calls `isListedPodMember`. Fourteen mutations, one per term, each alone: every one reddens exactly its named arm and nothing else. Baseline 9 suites / 95 tests, and the wider run is green — routes 139 / 1065, services 160 / 1737, controllers 14 / 175. --- .../unit/controllers/podController.test.js | 45 ++++++++++++++ .../routes/activity.write-membership.test.js | 24 +++++++- .../unit/routes/podInvites.management.test.js | 47 +++++++++++++++ .../activityService.attentionQueue.test.js | 30 ++++++++++ .../activityService.pendingApprovals.test.js | 12 ++-- .../activityService.podFeedMembership.test.js | 57 ++++++++++++++++++ .../services/activityService.recap.test.js | 35 +++++++++++ ...activityService.userFeedMembership.test.js | 58 +++++++++++++++++++ .../services/decisionRequestService.test.js | 15 +++++ backend/controllers/podController.ts | 13 +++++ backend/routes/activity.ts | 10 +++- backend/routes/podInvites.ts | 12 ++-- backend/services/activityService.ts | 52 +++++++++-------- backend/services/decisionRequestService.ts | 6 +- 14 files changed, 376 insertions(+), 40 deletions(-) create mode 100644 backend/__tests__/unit/services/activityService.podFeedMembership.test.js create mode 100644 backend/__tests__/unit/services/activityService.userFeedMembership.test.js diff --git a/backend/__tests__/unit/controllers/podController.test.js b/backend/__tests__/unit/controllers/podController.test.js index cdf959a5a..08b7a9873 100644 --- a/backend/__tests__/unit/controllers/podController.test.js +++ b/backend/__tests__/unit/controllers/podController.test.js @@ -282,6 +282,51 @@ describe('podController', () => { expect(res.json).toHaveBeenCalledWith(pod); }); + // ── TASK-166: the creator cannot leave, everyone else still can ───────── + + it('leavePod refuses the creator with 409 creator_cannot_leave and keeps them listed', async () => { + const pod = { + _id: 'p1', + createdBy: 'creator', + members: ['creator', 'member'], + save: jest.fn(), + populate: jest.fn(), + }; + Pod.findById.mockResolvedValue(pod); + const req = { params: { id: 'p1' }, userId: 'creator' }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + + await podController.leavePod(req, res); + + expect(res.status).toHaveBeenCalledWith(409); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ code: 'creator_cannot_leave' })); + // The refusal is a non-event, not a silent success: nothing was unlisted + // and nothing was saved. + expect(pod.members).toEqual(['creator', 'member']); + expect(pod.save).not.toHaveBeenCalled(); + }); + + // Positive control. The arm above is satisfied by a route that refuses + // everyone, which is exactly the shape the guard must not be. + it('leavePod still removes a non-creator member (control)', async () => { + const pod = { + _id: 'p1', + createdBy: 'creator', + members: ['creator', 'member'], + save: jest.fn(), + populate: jest.fn().mockResolvedValue(), + }; + Pod.findById.mockResolvedValue(pod); + const req = { params: { id: 'p1' }, userId: 'member' }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + + await podController.leavePod(req, res); + + expect(pod.members).toEqual(['creator']); + expect(pod.save).toHaveBeenCalled(); + expect(res.json).toHaveBeenCalledWith(pod); + }); + // ── ADR-001 §3.10: agent-rooms are 1:1 DMs ────────────────────────────── it('joinPod rejects a third-person join on agent-room with 403', async () => { diff --git a/backend/__tests__/unit/routes/activity.write-membership.test.js b/backend/__tests__/unit/routes/activity.write-membership.test.js index 5e6572431..265125d7e 100644 --- a/backend/__tests__/unit/routes/activity.write-membership.test.js +++ b/backend/__tests__/unit/routes/activity.write-membership.test.js @@ -66,8 +66,22 @@ describe('POST /api/activity/create — pod membership', () => { expect(create).toHaveBeenCalledTimes(1); }); - it('allows the creator, who is not always listed in members', async () => { - const { app, create } = setup({ _id: 'pod-1', createdBy: CALLER, members: [] }); + // TASK-166 inverted this arm. It used to assert that the creator was admitted + // with an empty `members` — i.e. that `createdBy` counted as membership. It + // now asserts the opposite: `createdBy` is who made the pod, and a creator + // `leavePod` has unlisted is subject to the same membership rule as everyone + // else (it is the rule `createMessage` and the relay have always applied). + it('refuses a creator who is no longer listed, and writes nothing', async () => { + const { app, create } = setup({ _id: 'pod-1', createdBy: CALLER, members: [OTHER] }); + await request(app).post('/api/activity/create').send(body()).expect(403); + expect(create).not.toHaveBeenCalled(); + }); + + // The control for the inversion: a creator who IS listed still writes. Without + // it, the arm above passes for a route that refuses every pod naming a + // creator. + it('still admits a creator who is also listed', async () => { + const { app, create } = setup({ _id: 'pod-1', createdBy: CALLER, members: [CALLER] }); await request(app).post('/api/activity/create').send(body()).expect(200); expect(create).toHaveBeenCalledTimes(1); }); @@ -155,4 +169,10 @@ describe('POST /api/activity/seed/:podId — pod membership', () => { await request(app).post('/api/activity/seed/pod-1').send({}).expect(404); expect(seedPodActivities).not.toHaveBeenCalled(); }); + + it('refuses a creator who is no longer listed, and never reaches the seeder', async () => { + const { app, seedPodActivities } = setup({ _id: 'pod-1', createdBy: CALLER, members: [OTHER] }); + await request(app).post('/api/activity/seed/pod-1').send({}).expect(403); + expect(seedPodActivities).not.toHaveBeenCalled(); + }); }); diff --git a/backend/__tests__/unit/routes/podInvites.management.test.js b/backend/__tests__/unit/routes/podInvites.management.test.js index ee2651017..891e54bdb 100644 --- a/backend/__tests__/unit/routes/podInvites.management.test.js +++ b/backend/__tests__/unit/routes/podInvites.management.test.js @@ -127,6 +127,53 @@ describe('pod invite management routes', () => { expect(invite.save).not.toHaveBeenCalled(); }); + // ── TASK-166: `createdBy` is not membership ───────────────────────────── + // The three site arms below each name one route. A departed creator is + // `createdBy: user1` with `members` no longer listing them — the shape + // `leavePod` leaves behind, and the only shape the permissive predicate + // admitted that the strict one refuses. + + it('refuses to create an invite for a creator who left', async () => { + Pod.findById.mockResolvedValue(memberPod({ createdBy: 'user1', members: ['someone-else'] })); + + await request(app).post(`/api/pods/${POD_ID}/invites`).send({}).expect(403); + + const { PodInvite } = require('../../../models/PodInvite'); + expect(PodInvite.create).not.toHaveBeenCalled(); + }); + + it('refuses to list invites for a creator who left', async () => { + Pod.findById.mockResolvedValue(memberPod({ createdBy: 'user1', members: ['someone-else'] })); + + await request(app).get(`/api/pods/${POD_ID}/invites`).expect(403); + + expect(mockFind).not.toHaveBeenCalled(); + }); + + it('refuses to revoke an invite for a creator who left', async () => { + const invite = { + token: TOKEN, podId: POD_ID, revokedAt: null, save: jest.fn(), + }; + mockFindOne.mockResolvedValue(invite); + Pod.findById.mockResolvedValue(memberPod({ createdBy: 'user1', members: ['someone-else'] })); + + await request(app).delete(`/api/invites/${TOKEN}`).expect(403); + + expect(invite.save).not.toHaveBeenCalled(); + }); + + // Positive control: a creator who is ALSO listed still manages invites, so + // the three arms above cannot pass by refusing every pod that names a + // creator. + it('still lets a creator who is also listed manage invites', async () => { + Pod.findById.mockResolvedValue(memberPod({ createdBy: 'user1', members: ['user1'] })); + mockFind.mockReturnValue(listChain([])); + + await request(app).get(`/api/pods/${POD_ID}/invites`).expect(200); + + expect(mockFind).toHaveBeenCalledWith({ podId: POD_ID, revokedAt: null }); + }); + it('rejects a revoked invite on redemption', async () => { mockFindOne.mockResolvedValue({ token: TOKEN, diff --git a/backend/__tests__/unit/services/activityService.attentionQueue.test.js b/backend/__tests__/unit/services/activityService.attentionQueue.test.js index 181750ed5..1c71b74a5 100644 --- a/backend/__tests__/unit/services/activityService.attentionQueue.test.js +++ b/backend/__tests__/unit/services/activityService.attentionQueue.test.js @@ -89,6 +89,36 @@ describe('ActivityService.getDecisionQueue', () => { }); }); + // TASK-166. `createdBy` is who made the pod, not a standing membership, and + // `leavePod` leaves it behind. Two witnesses: the query term, and the + // in-process filter that re-checks the returned rows. + it('reads settled history by membership only', async () => { + mockPodFind.mockReturnValue(chain([{ + _id: 'pod-1', name: 'Current', createdBy: 'owner', members: ['member-1'], + }])); + + await ActivityService.getDecisionHistory('member-1', { podId: 'pod-1' }); + + expect(mockPodFind).toHaveBeenCalledWith({ + _id: 'pod-1', + $or: [ + { 'members.userId': 'member-1' }, + { members: 'member-1' }, + { 'members._id': 'member-1' }, + ], + }); + }); + + it('refuses a creator who left the pod, although createdBy still names them', async () => { + mockPodFind.mockReturnValue(chain([{ + _id: 'pod-1', name: 'Former', createdBy: 'creator-1', members: ['member-1'], + }])); + + await expect(ActivityService.getDecisionHistory('creator-1', { podId: 'pod-1' })) + .rejects.toThrow('Access denied'); + expect(mockDecisionFind).not.toHaveBeenCalled(); + }); + it('rejects settled history for a viewer outside the requested pod', async () => { mockPodFind.mockReturnValue(chain([])); diff --git a/backend/__tests__/unit/services/activityService.pendingApprovals.test.js b/backend/__tests__/unit/services/activityService.pendingApprovals.test.js index aa7e73995..43b8a71e5 100644 --- a/backend/__tests__/unit/services/activityService.pendingApprovals.test.js +++ b/backend/__tests__/unit/services/activityService.pendingApprovals.test.js @@ -23,9 +23,14 @@ describe('ActivityService.getPendingApprovals', () => { jest.clearAllMocks(); }); - it('queries approvals from both creator and membership pods', async () => { + // TASK-166 inverted this arm: it used to assert the query read `createdBy` + // as membership ("both creator and membership pods"). `createdBy` records who + // made the pod and `leavePod` keeps it, so that term put a departed creator's + // queue back in front of them. The read rule and the write gate in + // `requireActivityApprovalMember` are now the same rule. + it('queries approvals from listed membership only', async () => { const memberUserId = 'queue-member'; - Pod.find.mockReturnValue(podFindResult([{ _id: 'owner-pod' }])); + Pod.find.mockReturnValue(podFindResult([{ _id: 'member-pod' }])); Activity.getPendingApprovals.mockResolvedValue([{ _id: 'approval-1' }]); await expect(ActivityService.getPendingApprovals(memberUserId)).resolves.toEqual([ @@ -34,10 +39,9 @@ describe('ActivityService.getPendingApprovals', () => { expect(Pod.find).toHaveBeenCalledWith({ $or: [ - { createdBy: memberUserId }, { members: memberUserId }, ], }); - expect(Activity.getPendingApprovals).toHaveBeenCalledWith(['owner-pod']); + expect(Activity.getPendingApprovals).toHaveBeenCalledWith(['member-pod']); }); }); diff --git a/backend/__tests__/unit/services/activityService.podFeedMembership.test.js b/backend/__tests__/unit/services/activityService.podFeedMembership.test.js new file mode 100644 index 000000000..c1b63fa96 --- /dev/null +++ b/backend/__tests__/unit/services/activityService.podFeedMembership.test.js @@ -0,0 +1,57 @@ +// TASK-166: the pod feed is pod-scoped, and `getPodFeed` checked membership as +// `createdBy === userId || members.includes(userId)`. `leavePod` filters +// `members` and keeps `createdBy`, so a departed creator could still read the +// pod's whole activity feed — including activities written after they left — +// while every chat and connector write into that pod refused them. +// +// Route-level tests mock this method out (`routes/activity.read.test.js`, +// `routes/activity.identity.test.js`), so the arm lives here. +jest.mock('../../../models/Pod', () => ({ findById: jest.fn() })); +jest.mock('../../../models/User', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Activity', () => ({})); +jest.mock('../../../models/Summary', () => ({})); +jest.mock('../../../models/Post', () => ({})); +jest.mock('../../../models/Task', () => ({ find: jest.fn() })); + +const Pod = require('../../../models/Pod'); +const User = require('../../../models/User'); +const ActivityService = require('../../../services/activityService'); + +const userChain = (value) => ({ select: () => ({ lean: async () => value }) }); +const podChain = (value) => ({ lean: async () => value }); + +describe('ActivityService.getPodFeed — pod membership', () => { + let aggregateSpy; + let readStateSpy; + + beforeEach(() => { + jest.clearAllMocks(); + User.findById.mockReturnValue(userChain({ _id: 'caller-1', username: 'Caller' })); + aggregateSpy = jest.spyOn(ActivityService, 'aggregateActivities').mockResolvedValue([]); + readStateSpy = jest.spyOn(ActivityService, 'annotateReadState').mockReturnValue([]); + }); + + afterEach(() => { + aggregateSpy.mockRestore(); + readStateSpy.mockRestore(); + }); + + it('refuses a creator who left the pod', async () => { + Pod.findById.mockReturnValue(podChain({ + _id: 'pod-1', name: 'Former', createdBy: 'creator-1', members: ['member-1'], + })); + + await expect(ActivityService.getPodFeed('pod-1', 'creator-1', {})) + .rejects.toThrow('Access denied'); + expect(aggregateSpy).not.toHaveBeenCalled(); + }); + + it('still serves a listed member (control)', async () => { + Pod.findById.mockReturnValue(podChain({ + _id: 'pod-1', name: 'Current', createdBy: 'creator-1', members: ['member-1'], + })); + + await expect(ActivityService.getPodFeed('pod-1', 'member-1', {})) + .resolves.toMatchObject({ activities: [], hasMore: false }); + }); +}); diff --git a/backend/__tests__/unit/services/activityService.recap.test.js b/backend/__tests__/unit/services/activityService.recap.test.js index 76e045694..1289377c3 100644 --- a/backend/__tests__/unit/services/activityService.recap.test.js +++ b/backend/__tests__/unit/services/activityService.recap.test.js @@ -50,6 +50,22 @@ describe('ActivityService recap and legacy approval authorization', () => { findByIdSpy?.mockRestore(); }); + test('builds the viewer\'s pod list from membership, not from createdBy', async () => { + await ActivityService.getRecap(ownerId, { window: 'today' }); + + // The membership decision for this reader is in the query, so the arm + // asserts the term the change removes rather than a row — the Pod mock + // returns whichever fixture it is handed. `createdBy` is written once at + // creation and survives `leavePod`, so reading it here handed a departed + // creator the recap of a pod they are no longer in. + expect(Pod.find.mock.calls[0][0]).toEqual({ + $or: [ + { 'members.userId': ownerId }, + { members: ownerId }, + ], + }); + }); + test('rejects a requested pod that is outside the viewer membership', async () => { await expect(ActivityService.getRecap(ownerId, { podId: 'not-a-member-pod' })) .rejects.toThrow('Access denied'); @@ -112,6 +128,25 @@ describe('ActivityService recap and legacy approval authorization', () => { expect(mockResolve).toHaveBeenCalledWith('approval', storedApproval._id); }); + // TASK-166: the read rule and this gate move together. A non-member arm + // cannot see the creator clause at all — only a creator can — so this is the + // arm that distinguishes the two predicates. + test('fails closed when the pod\'s creator has left it', async () => { + const storedApproval = new Activity({ type: 'approval_needed', action: 'approval_needed', podId: pod._id }); + const approve = jest.fn().mockResolvedValue(); + storedApproval.approve = approve; + findByIdSpy = jest.spyOn(Activity, 'findById').mockResolvedValue(storedApproval); + Pod.findById.mockReturnValue({ + select: jest.fn(() => ({ + lean: jest.fn().mockResolvedValue({ _id: 'pod-1', createdBy: ownerId, members: ['member-1'] }), + })), + }); + + await expect(ActivityService.approveActivity(String(storedApproval._id), ownerId, 'Approved')) + .resolves.toEqual({ success: false, status: 403, error: 'Only pod members can decide this' }); + expect(approve).not.toHaveBeenCalled(); + }); + test('fails closed when a non-member attempts a legacy Activity approval', async () => { const storedApproval = new Activity({ type: 'approval_needed', action: 'approval_needed', podId: pod._id }); const approve = jest.fn().mockResolvedValue(); diff --git a/backend/__tests__/unit/services/activityService.userFeedMembership.test.js b/backend/__tests__/unit/services/activityService.userFeedMembership.test.js new file mode 100644 index 000000000..c3008d00c --- /dev/null +++ b/backend/__tests__/unit/services/activityService.userFeedMembership.test.js @@ -0,0 +1,58 @@ +// TASK-166: `getUserFeed` builds the viewer's pod set from membership. It used +// to `$or` in `{ createdBy: userId }`, and `leavePod` keeps that field after +// unlisting the creator — so a departed creator's ambient feed still contained +// the pod they had left, while every chat and connector write into it refused +// them. +// +// Covered here rather than through `getRecap`, which spies this method out: the +// membership decision is in the query, so the arm asserts the query rather than +// a row (the Pod mock returns whichever fixture it is handed). +jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); +jest.mock('../../../models/User', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Activity', () => ({})); +jest.mock('../../../models/Summary', () => ({})); +jest.mock('../../../models/Post', () => ({})); +jest.mock('../../../models/Task', () => ({ find: jest.fn() })); + +const Pod = require('../../../models/Pod'); +const User = require('../../../models/User'); +const ActivityService = require('../../../services/activityService'); + +const chain = (value) => ({ select: () => ({ lean: async () => value }) }); + +describe('ActivityService.getUserFeed — pod membership', () => { + let rankSpy; + let aggregateSpy; + + beforeEach(() => { + jest.clearAllMocks(); + User.findById.mockReturnValue(chain({ _id: 'caller-1', username: 'Caller' })); + Pod.find.mockReturnValue(chain([])); + rankSpy = jest.spyOn(ActivityService, 'rankPodsByRecentActivity').mockResolvedValue([]); + aggregateSpy = jest.spyOn(ActivityService, 'aggregateActivities').mockResolvedValue([]); + }); + + afterEach(() => { + rankSpy.mockRestore(); + aggregateSpy.mockRestore(); + }); + + it('selects the viewer\'s pods by membership, not by createdBy', async () => { + await ActivityService.getUserFeed('caller-1', {}); + + expect(Pod.find).toHaveBeenCalledWith({ + $or: [ + { 'members.userId': 'caller-1' }, + { members: 'caller-1' }, + ], + }); + }); + + it('still reaches the aggregator for a member (control)', async () => { + await expect(ActivityService.getUserFeed('caller-1', {})).resolves.toMatchObject({ + activities: [], + hasMore: false, + }); + expect(aggregateSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/backend/__tests__/unit/services/decisionRequestService.test.js b/backend/__tests__/unit/services/decisionRequestService.test.js index 0f3007669..80eeb629a 100644 --- a/backend/__tests__/unit/services/decisionRequestService.test.js +++ b/backend/__tests__/unit/services/decisionRequestService.test.js @@ -269,4 +269,19 @@ describe('DecisionRequestService', () => { .resolves.toEqual({ status: 403, body: { error: 'Only human pod members can rule on this decision' } }); expect(mockDecision.findOneAndUpdate).not.toHaveBeenCalled(); }); + + // TASK-166: `createdBy` is who made the pod, not a standing membership. A + // creator who has left the room must not rule on its decision cards — the + // gate is reached from `routes/activity.ts` under plain `auth`, which bot + // and human tokens both satisfy, so this is the reachable population and not + // a hypothetical one. + test('refuses a creator who left the pod before claiming the decision', async () => { + mockDecision.findById.mockResolvedValue(pending()); + mockUser.findById.mockReturnValue(userChain({ _id: 'creator-1', username: 'Former owner', isBot: false })); + mockPod.findById.mockReturnValue(podChain({ createdBy: 'creator-1', members: ['member'], type: 'team' })); + await expect(chooseDecision({ decisionId: 'decision-1', callerUserId: 'creator-1', value: 'Canary' })) + .resolves.toEqual({ status: 403, body: { error: 'Only human pod members can rule on this decision' } }); + expect(mockDecision.findOneAndUpdate).not.toHaveBeenCalled(); + expect(mockPGMessage.create).not.toHaveBeenCalled(); + }); }); diff --git a/backend/controllers/podController.ts b/backend/controllers/podController.ts index eab030a5a..4c90af4ac 100644 --- a/backend/controllers/podController.ts +++ b/backend/controllers/podController.ts @@ -589,6 +589,19 @@ exports.leavePod = async (req: any, res: any) => { return res.status(404).json({ msg: 'Pod not found' }); } + // TASK-166: the creator cannot leave. `createdBy` is written only at + // creation and nothing transfers it, while `removeMember` is gated on it + // with no admin fallback — so a creator who left would strand the pod with + // nobody able to remove a member. Refused rather than stripped: the field + // records who made the pod, and the membership readers no longer read it as + // membership. + if (String(pod.createdBy) === String(req.userId)) { + return res.status(409).json({ + msg: 'A pod creator cannot leave their own pod.', + code: 'creator_cannot_leave', + }); + } + // Check if user is a member if (!pod.members.includes(req.userId)) { return res.status(400).json({ msg: 'Not a member of this pod' }); diff --git a/backend/routes/activity.ts b/backend/routes/activity.ts index 6442471fd..dc5fc00ca 100644 --- a/backend/routes/activity.ts +++ b/backend/routes/activity.ts @@ -21,7 +21,9 @@ const getAuthenticatedUserId = require('../utils/getAuthenticatedUserId'); // eslint-disable-next-line global-require const Pod = require('../models/Pod'); // eslint-disable-next-line global-require -const isPodMember = require('../utils/isPodMember'); +// TASK-166: the strict predicate. These two routes used the permissive default, +// which admits a pod's creator after `leavePod` has unlisted them. +const { isListedPodMember } = require('../utils/isPodMember'); interface Req { query?: Record; @@ -333,7 +335,7 @@ router.post('/seed/:podId', auth, async (req: Req, res: Res) => { const userId = getAuthenticatedUserId(req); const pod = await Pod.findById(String(podId)).select('members createdBy').lean(); if (!pod) return res.status(404).json({ error: 'Pod not found' }); - if (!isPodMember(pod, userId)) return res.status(403).json({ error: 'Only pod members can seed activities' }); + if (!isListedPodMember(pod, userId)) return res.status(403).json({ error: 'Only pod members can seed activities' }); const result = await ActivityService.seedPodActivities(podId, userId) as { success?: boolean; error?: string }; if (!result.success) return res.status(400).json({ error: result.error }); return res.json(result); @@ -360,7 +362,9 @@ router.post('/create', auth, async (req: Req, res: Res) => { // operators rather than an id. const pod = await Pod.findById(String(podId)).select('members createdBy').lean(); if (!pod) return res.status(404).json({ error: 'Pod not found' }); - if (!isPodMember(pod, userId)) return res.status(403).json({ error: 'Only pod members can create activities in a pod' }); + if (!isListedPodMember(pod, userId)) { + return res.status(403).json({ error: 'Only pod members can create activities in a pod' }); + } const user = await User.findById(userId).select('username').lean() as { username?: string } | null; // Store the id of the pod that was actually resolved and authorised, not // the body's copy of it. diff --git a/backend/routes/podInvites.ts b/backend/routes/podInvites.ts index 9d4c56703..a5cc01004 100644 --- a/backend/routes/podInvites.ts +++ b/backend/routes/podInvites.ts @@ -46,10 +46,12 @@ const inviteWriteRateLimit = rateLimit({ }); // eslint-disable-next-line global-require -const isPodMember = require('../utils/isPodMember'); +// TASK-166: the strict predicate — `createdBy` records who made the pod, not who +// may manage its invites once they have left it. +const { isListedPodMember } = require('../utils/isPodMember'); // POST /api/pods/:podId/invites — issue a fresh invite token. Caller must -// be a member or creator. Body: { expiresInHours?, maxUses? } — both +// be a listed member. Body: { expiresInHours?, maxUses? } — both // optional; null = unlimited. router.post('/pods/:podId/invites', inviteWriteRateLimit, auth, async (req: any, res: any) => { try { @@ -57,7 +59,7 @@ router.post('/pods/:podId/invites', inviteWriteRateLimit, auth, async (req: any, if (!userId) return res.status(401).json({ msg: 'Unauthorized' }); const pod = await Pod.findById(req.params.podId); if (!pod) return res.status(404).json({ msg: 'Pod not found' }); - if (!isPodMember(pod, userId)) { + if (!isListedPodMember(pod, userId)) { return res.status(403).json({ msg: 'Only pod members can create invites' }); } const { expiresInHours, maxUses } = req.body || {}; @@ -99,7 +101,7 @@ router.get('/pods/:podId/invites', inviteReadRateLimit, auth, async (req: any, r const podId = new mongoose.Types.ObjectId(rawPodId); const pod = await Pod.findById(podId); if (!pod) return res.status(404).json({ msg: 'Pod not found' }); - if (!isPodMember(pod, userId)) { + if (!isListedPodMember(pod, userId)) { return res.status(403).json({ msg: 'Only pod members can manage invites' }); } const invites = await PodInvite.find({ podId: pod._id, revokedAt: null }) @@ -139,7 +141,7 @@ router.delete('/invites/:token', inviteWriteRateLimit, auth, async (req: any, re if (!invite) return res.status(404).json({ msg: 'Invite not found' }); const pod = await Pod.findById(invite.podId); if (!pod) return res.status(404).json({ msg: 'Pod not found' }); - if (!isPodMember(pod, userId)) { + if (!isListedPodMember(pod, userId)) { return res.status(403).json({ msg: 'Only pod members can manage invites' }); } if (!invite.revokedAt) { diff --git a/backend/services/activityService.ts b/backend/services/activityService.ts index 47341f7f3..4e774eb6f 100644 --- a/backend/services/activityService.ts +++ b/backend/services/activityService.ts @@ -11,7 +11,7 @@ const Post = require('../models/Post'); // eslint-disable-next-line global-require const Task = require('../models/Task'); // eslint-disable-next-line global-require -const isPodMember = require('../utils/isPodMember'); +const { isListedPodMember } = require('../utils/isPodMember'); let PGMessage: unknown = null; try { @@ -125,9 +125,11 @@ class ActivityService { ): Promise> { const window = options.window === '7d' ? '7d' : 'today'; const since = new Date(Date.now() - (window === '7d' ? 7 : 1) * 24 * 60 * 60 * 1000); + // TASK-166: the pod list is built from membership only. `createdBy` says who + // made the pod, and `leavePod` keeps it after unlisting them, so reading it + // as membership handed a departed creator this pod's recap. const pods: PodDoc[] = await Pod.find({ $or: [ - { createdBy: userId }, { 'members.userId': userId }, { members: userId }, ], @@ -315,7 +317,6 @@ class ActivityService { const pods: PodDoc[] = await Pod.find({ $or: [ - { createdBy: userId }, { 'members.userId': userId }, { members: userId }, ], @@ -411,18 +412,16 @@ class ActivityService { const offset = Number.isInteger(options.offset) ? Math.max(options.offset as number, 0) : 0; const membership = { $or: [ - { createdBy: userId }, { 'members.userId': userId }, { members: userId }, { 'members._id': userId }, ], }; const podQuery = requestedPodId ? { _id: requestedPodId, ...membership } : membership; - const pods: PodDoc[] = await Pod.find(podQuery).select('_id name createdBy members').lean(); + const pods: PodDoc[] = await Pod.find(podQuery).select('_id name members').lean(); if (requestedPodId && pods.length === 0) throw new Error('Access denied'); const allowedPods = pods.filter((pod) => ( - String(pod.createdBy || '') === String(userId) - || (pod.members || []).some((member: any) => String(member?.userId || member?._id || member) === String(userId)) + (pod.members || []).some((member: any) => String(member?.userId || member?._id || member) === String(userId)) )); if (requestedPodId && allowedPods.length === 0) throw new Error('Access denied'); const podIds = allowedPods.map((pod) => String(pod._id)); @@ -501,15 +500,16 @@ class ActivityService { throw new Error('Pod not found'); } - const isMember = String(pod.createdBy) === String(userId) - || (pod.members as unknown[])?.some( - (m: unknown) => { - const member = m as { userId?: unknown }; - return (String(member.userId) || String(m)) === String(userId); - }, - ); - - if (!isMember) { + // The shared rule, not a third hand-rolled copy of it. The local form + // this replaces read `String(member.userId) || String(m)` — and + // `String(undefined)` is the non-empty string 'undefined', so the + // fallback to `String(m)` was unreachable and the predicate admitted + // nobody whose membership is a plain ObjectId. In production only the + // `createdBy` term it sat beside ever matched (0 of 424 pods carry a + // `members.userId` entry), so narrowing that term without this fix would + // have refused every caller, members included. The control arm below is + // what caught it. + if (!isListedPodMember(pod, userId)) { throw new Error('Access denied'); } @@ -1281,8 +1281,11 @@ class ActivityService { /** * Legacy Activity approval rows predate ApprovalAction.ownerUserId. Their - * read rule is pod membership (with the creator fallback for old rows), so - * write authorization must use the identical predicate. Do this at write + * read rule is pod membership, so write authorization uses the identical + * predicate — `isListedPodMember`. TASK-166 removed the creator fallback on + * both sides at once: it stood in for rows created before creators were added + * to `members` on save, but a creator who has left is not a member, and + * `leavePod` keeps `createdBy` while removing the listing. Do this at write * time: a stale or forged client must not turn an Activity id into broad * authenticated approval authority. */ @@ -1292,9 +1295,9 @@ class ActivityService { ): Promise | null> { const rawPodId = activity.podId as { _id?: unknown } | undefined; const podId = rawPodId?._id || activity.podId; - const pod = await Pod.findById(podId).select('createdBy members').lean(); + const pod = await Pod.findById(podId).select('members').lean(); if (!pod) return { success: false, status: 404, error: 'Approval pod not found' }; - if (!isPodMember(pod, userId)) { + if (!isListedPodMember(pod, userId)) { return { success: false, status: 403, error: 'Only pod members can decide this' }; } return null; @@ -1306,10 +1309,13 @@ class ActivityService { // Pending approvals belong to every pod member. `members` is an // ObjectId[] (not a role-bearing object), so querying // `members.userId` or `members.role` silently excludes members. - // Keep createdBy for legacy rows created before creators were added - // to members on save. + // + // TASK-166: no `createdBy` term. It used to stand in for legacy rows + // created before creators were added to `members` on save, but a + // creator who leaves is not a member — and `leavePod` keeps the field. + // A pod whose creator is unlisted is now absent from their queue here, + // which is the same rule the write gate above applies. $or: [ - { createdBy: userId }, { members: userId }, ], }) diff --git a/backend/services/decisionRequestService.ts b/backend/services/decisionRequestService.ts index 88e922b93..d42ae1b5c 100644 --- a/backend/services/decisionRequestService.ts +++ b/backend/services/decisionRequestService.ts @@ -20,7 +20,7 @@ const { deliverMessageToAgents } = require('./messageAgentDeliveryService'); // eslint-disable-next-line global-require const AgentEventService = require('./agentEventService'); // eslint-disable-next-line global-require -const isPodMember = require('../utils/isPodMember'); +const { isListedPodMember } = require('../utils/isPodMember'); // eslint-disable-next-line global-require const socketConfig = require('../config/socket'); @@ -368,9 +368,9 @@ export const chooseDecision = async ( const caller = await User.findById(callerUserId).select('username isBot').lean(); if (!caller || caller.isBot) return { status: 403, body: { error: 'Only a human can rule on this decision' } }; - const pod = await Pod.findById(String(row.podId)).select('members createdBy type').lean(); + const pod = await Pod.findById(String(row.podId)).select('members type').lean(); if (!pod) return { status: 404, body: { error: 'Pod not found' } }; - if (!isPodMember(pod, callerUserId)) { + if (!isListedPodMember(pod, callerUserId)) { return { status: 403, body: { error: 'Only human pod members can rule on this decision' } }; }