diff --git a/backend/__tests__/service/pgMessages.test.js b/backend/__tests__/service/pgMessages.test.js index d28aacdfb..1f7a6d8b8 100644 --- a/backend/__tests__/service/pgMessages.test.js +++ b/backend/__tests__/service/pgMessages.test.js @@ -13,6 +13,8 @@ jest.mock('../../models/User', () => ({ })), })); +jest.mock('../../models/Pod', () => ({ findById: jest.fn() })); + // Mock PG models jest.mock('../../models/pg/Pod', () => ({ findById: jest.fn(), @@ -35,10 +37,24 @@ jest.mock('../../services/agentMentionService', () => { }; }); +const MongoPod = require('../../models/Pod'); const PGPod = require('../../models/pg/Pod'); const PGMessage = require('../../models/pg/Message'); const AgentMentionService = require('../../services/agentMentionService'); +// TASK-162: Mongo `members` decides access, so an arm that expects 200 names +// the caller in the pod Mongo returns. `PGPod.isMember` is still mocked in these +// arms on purpose — a live PG row that says "member" must not be enough. +const podListing = (...memberIds) => { + MongoPod.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue(memberIds === null || memberIds[0] === null + ? null + : { members: memberIds }), + }), + }); +}; + let app; beforeAll(() => { @@ -53,9 +69,10 @@ afterEach(() => { }); describe('PostgreSQL Message Routes', () => { - it('retrieves messages when user is member', async () => { + it('retrieves messages for a member of the pod, decided by Mongo membership', async () => { PGPod.findById.mockResolvedValue({ id: 'pod1' }); - PGPod.isMember.mockResolvedValue(true); + PGPod.isMember.mockResolvedValue(true); // a live PG row is not the reason + podListing('user1'); PGMessage.findByPodId.mockResolvedValue([{ id: 1, content: 'Hello' }]); const token = generateTestToken('user1'); @@ -64,13 +81,35 @@ describe('PostgreSQL Message Routes', () => { .set('Authorization', `Bearer ${token}`) .expect(200); - expect(PGPod.isMember).toHaveBeenCalledWith('pod1', 'user1'); + expect(MongoPod.findById).toHaveBeenCalledWith('pod1'); + expect(PGPod.isMember).not.toHaveBeenCalled(); expect(res.body[0].content).toBe('Hello'); }); + // TASK-162's witness at the route tier: a live PG row that says "member" for + // a caller Mongo no longer lists. `PGPod.isMember` returns true in this arm, + // so an arm that only leaves the pod cannot see the defect — the SURVIVOR row + // is what discriminates the read-time check from a mirror-on-leave fix. + it('refuses a post whose PG pod_members row survived a leave', async () => { + PGPod.findById.mockResolvedValue({ id: 'pod1' }); + PGPod.isMember.mockResolvedValue(true); + podListing(); // Mongo membership is gone + const token = generateTestToken('user1'); + + const res = await request(app) + .post('/api/pg/messages/pod1') + .set('Authorization', `Bearer ${token}`) + .send({ content: 'still here?' }) + .expect(401); + + expect(res.body.msg).toMatch(/Not authorized/); + expect(PGMessage.create).not.toHaveBeenCalled(); + }); + it('returns 401 if user is not a member', async () => { PGPod.findById.mockResolvedValue({ id: 'pod1' }); PGPod.isMember.mockResolvedValue(false); + podListing(); const token = generateTestToken('user1'); const res = await request(app) @@ -84,6 +123,7 @@ describe('PostgreSQL Message Routes', () => { it('creates a message successfully', async () => { PGPod.findById.mockResolvedValue({ id: 'pod1' }); PGPod.isMember.mockResolvedValue(true); + podListing('user1'); PGMessage.create.mockResolvedValue({ id: 1 }); PGMessage.findById.mockResolvedValue({ id: 1, content: 'Hi there' }); const token = generateTestToken('user1'); @@ -112,6 +152,7 @@ describe('PostgreSQL Message Routes', () => { }; PGPod.findById.mockResolvedValue({ id: 'pod1', type: 'chat' }); PGPod.isMember.mockResolvedValue(true); + podListing('user1'); PGMessage.create.mockResolvedValue({ id: message.id }); PGMessage.findById.mockResolvedValue(message); const token = generateTestToken('user1'); @@ -136,6 +177,7 @@ describe('PostgreSQL Message Routes', () => { }; PGPod.findById.mockResolvedValue({ id: 'pod1', type: 'chat' }); PGPod.isMember.mockResolvedValue(true); + podListing('user1'); PGMessage.create.mockResolvedValue(persistedMessage); PGMessage.findById.mockResolvedValue(null); const token = generateTestToken('user1'); @@ -168,6 +210,7 @@ describe('PostgreSQL Message Routes', () => { }; PGPod.findById.mockResolvedValue({ id: 'pod1', type: 'agent-room' }); PGPod.isMember.mockResolvedValue(true); + podListing('user1'); PGMessage.create.mockResolvedValue({ id: message.id }); PGMessage.findById.mockResolvedValue(message); const token = generateTestToken('user1'); @@ -187,6 +230,7 @@ describe('PostgreSQL Message Routes', () => { it('rejects message creation for non-members', async () => { PGPod.findById.mockResolvedValue({ id: 'pod1' }); PGPod.isMember.mockResolvedValue(false); + podListing(); const token = generateTestToken('user1'); const res = await request(app) diff --git a/backend/__tests__/unit/controllers/pgMessageController.test.js b/backend/__tests__/unit/controllers/pgMessageController.test.js index 6e75f2266..d83301536 100644 --- a/backend/__tests__/unit/controllers/pgMessageController.test.js +++ b/backend/__tests__/unit/controllers/pgMessageController.test.js @@ -1,14 +1,29 @@ const controller = require('../../../controllers/pgMessageController'); const PGPod = require('../../../models/pg/Pod'); const PGMessage = require('../../../models/pg/Message'); +const MongoPod = require('../../../models/Pod'); const AgentMentionService = require('../../../services/agentMentionService'); const { AgentInstallation } = require('../../../models/AgentRegistry'); jest.mock('../../../models/pg/Pod'); jest.mock('../../../models/pg/Message'); +jest.mock('../../../models/Pod'); jest.mock('../../../services/agentMentionService'); jest.mock('../../../models/AgentRegistry'); +// Mongo membership is the decision since TASK-162, so every arm that expects a +// status other than 404 has to say what the pod's `members` holds. `null` is a +// pod Mongo does not have (the orphan-row class), not an empty member list. +const mongoPod = (members) => { + MongoPod.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue(members === null ? null : { members }), + }), + }); +}; + +const jsonRes = () => ({ status: jest.fn().mockReturnThis(), json: jest.fn() }); + describe('pgMessageController', () => { afterEach(() => jest.clearAllMocks()); @@ -21,13 +36,14 @@ describe('pgMessageController', () => { it('getMessages returns 404 if pod not found', async () => { PGPod.findById.mockResolvedValue(null); + mongoPod(null); const req = { params: { podId: 'p1' }, query: {}, userId: 'u1', user: { id: 'u1' }, }; - const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + const res = jsonRes(); await controller.getMessages(req, res); expect(res.status).toHaveBeenCalledWith(404); }); @@ -35,7 +51,7 @@ describe('pgMessageController', () => { it('returns mention delivery feedback for a legacy PG message post', async () => { const message = { id: 'm1', content: 'hello @recorder', userId: { username: 'sam' } }; PGPod.findById.mockResolvedValue({ type: 'chat' }); - PGPod.isMember.mockResolvedValue(true); + mongoPod(['u1']); PGMessage.create.mockResolvedValue({ id: 'm1', content: 'hello @recorder' }); PGMessage.findById.mockResolvedValue(message); AgentMentionService.isAutoRoutedDmPod.mockReturnValue(false); @@ -51,7 +67,7 @@ describe('pgMessageController', () => { userId: 'u1', user: { id: 'u1', username: 'sam' }, }; - const res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + const res = jsonRes(); await controller.createMessage(req, res); @@ -68,4 +84,119 @@ describe('pgMessageController', () => { }, }); }); + + // TASK-162. The defect was a stale positive: the PG `pod_members` row was + // checked first and concluded membership, so a row that outlived the + // membership granted write access. A "leave then post is 401" arm cannot see + // that — it passes while the row is absent. The arm that discriminates is the + // SURVIVOR: the ghost row still present, Mongo membership gone. + it('refuses a post from a member whose PG row survived their departure', async () => { + PGPod.findById.mockResolvedValue({ type: 'chat' }); // the ghost row is there + PGPod.isMember.mockResolvedValue(true); // and would still say yes + mongoPod([]); // Mongo is the truth, and this caller is not in it + const req = { + params: { podId: 'p1' }, + body: { content: 'still here?' }, + userId: 'u1', + user: { id: 'u1', username: 'sam' }, + }; + const res = jsonRes(); + + await controller.createMessage(req, res); + + expect(res.status).toHaveBeenCalledWith(401); + expect(PGMessage.create).not.toHaveBeenCalled(); + expect(PGPod.isMember).not.toHaveBeenCalled(); + }); + + it('refuses a read from the same stale row, so the ghost does not leak history', async () => { + PGPod.findById.mockResolvedValue({ type: 'chat' }); + PGPod.isMember.mockResolvedValue(true); + mongoPod([]); + const req = { + params: { podId: 'p1' }, + query: {}, + userId: 'u1', + user: { id: 'u1' }, + }; + const res = jsonRes(); + + await controller.getMessages(req, res); + + expect(res.status).toHaveBeenCalledWith(401); + expect(PGMessage.findByPodId).not.toHaveBeenCalled(); + }); + + it('refuses a departed CREATOR whose PG row is present', async () => { + // 36 of the 77 ghost rows are the pod's own creator (Vera 74648), which is + // the population where the creator clause and the stale row reinforce each + // other. `createdBy` is not membership: it says who made the pod, not who is + // in it. + PGPod.findById.mockResolvedValue({ type: 'chat' }); + PGPod.isMember.mockResolvedValue(true); + mongoPod([]); + MongoPod.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue({ createdBy: 'u1', members: [] }), + }), + }); + const req = { + params: { podId: 'p1' }, + body: { content: 'hello' }, + userId: 'u1', + user: { id: 'u1', username: 'sam' }, + }; + const res = jsonRes(); + + await controller.createMessage(req, res); + + expect(res.status).toHaveBeenCalledWith(401); + expect(PGMessage.create).not.toHaveBeenCalled(); + }); + + it('refuses a post into a pod Mongo no longer has, however old its PG row is', async () => { + // The 140-row orphan class: the PG pod row exists, Mongo's does not, so + // there is no membership list left to be in. + PGPod.findById.mockResolvedValue({ type: 'chat' }); + mongoPod(null); + const req = { + params: { podId: 'p1' }, + body: { content: 'hello' }, + userId: 'u1', + user: { id: 'u1', username: 'sam' }, + }; + const res = jsonRes(); + + await controller.createMessage(req, res); + + expect(res.status).toHaveBeenCalledWith(401); + expect(PGMessage.create).not.toHaveBeenCalled(); + }); + + it('admits a listed member who has no PG row at all — the lazily-synced mirror must not refuse', async () => { + // The inverse direction, and the reason Mongo decides rather than the + // mirror: community auto-join and other join paths write Mongo only, so an + // absent PG row is not evidence against membership. + PGPod.findById.mockResolvedValue({ type: 'chat' }); + PGMessage.create.mockResolvedValue({ id: 'm2', content: 'hi' }); + PGMessage.findById.mockResolvedValue({ id: 'm2', content: 'hi' }); + AgentMentionService.isAutoRoutedDmPod.mockReturnValue(false); + AgentMentionService.enqueueMentions.mockResolvedValue({ enqueued: [], implicit: [], woken: [] }); + AgentInstallation.countDocuments.mockResolvedValue(0); + mongoPod(['u1']); + const req = { + params: { podId: 'p1' }, + body: { content: 'hi' }, + userId: 'u1', + user: { id: 'u1', username: 'sam' }, + }; + const res = jsonRes(); + + await controller.createMessage(req, res); + + expect(PGMessage.create).toHaveBeenCalledWith('p1', 'u1', 'hi'); + // The mirror is warmed for the PG listing surfaces, and that write decides + // nothing. + expect(PGPod.addMember).toHaveBeenCalledWith('p1', 'u1'); + }); }); diff --git a/backend/__tests__/unit/controllers/reactionController.test.js b/backend/__tests__/unit/controllers/reactionController.test.js index 7d9b3c669..99d8a6466 100644 --- a/backend/__tests__/unit/controllers/reactionController.test.js +++ b/backend/__tests__/unit/controllers/reactionController.test.js @@ -74,7 +74,15 @@ const buildRes = () => { const messageLookup = (podId, authorUserId = 'message-author') => ({ rows: [{ pod_id: podId, user_id: authorUserId }], rowCount: 1, }); -const memberLookup = (hits) => ({ rows: [], rowCount: hits }); +// TASK-162: Mongo `members` decides write access, and the PG `pod_members` +// mirror no longer grants anything. An arm that expects access therefore names +// the caller in the pod Mongo returns; the PG membership hits these arms used +// to queue are gone, because queueing them was queueing a grant nobody read. +const podListing = (...memberIds) => { + Pod.findById.mockReturnValue({ + select: () => ({ lean: () => Promise.resolve({ members: memberIds }) }), + }); +}; describe('reactionController.addReaction — agent runtime path', () => { let emitMock; @@ -128,14 +136,10 @@ describe('reactionController.addReaction — agent runtime path', () => { AgentInstallation.findOne.mockReturnValue({ lean: () => Promise.resolve(null), }); - Pod.findById.mockReturnValue({ - select: () => ({ - lean: () => - Promise.resolve({ - members: [{ userId: { toString: () => 'bot-user-2' } }], - }), - }), - }); + // `Pod.members` holds ObjectIds (`models/Pod.ts:157`), the same shape + // `createMessage` compares against — not `{ userId }` objects, which that + // comparison would refuse. + podListing('bot-user-2'); const req = { params: { messageId: '7' }, @@ -174,12 +178,11 @@ describe('reactionController.addReaction — agent runtime path', () => { expect(MessageReaction.add).not.toHaveBeenCalled(); }); - test('human caller hits the pg pod_members path (not the AgentInstallation path)', async () => { + test('human caller is admitted from Mongo members, and no PG pod_members row is read (TASK-162)', async () => { pool.query // loadMessageContext - .mockResolvedValueOnce(messageLookup('pod-h')) - // pod_members lookup - .mockResolvedValueOnce(memberLookup(1)); + .mockResolvedValueOnce(messageLookup('pod-h')); + podListing('human-1'); const req = { params: { messageId: '11' }, @@ -192,15 +195,13 @@ describe('reactionController.addReaction — agent runtime path', () => { expect(AgentInstallation.findOne).not.toHaveBeenCalled(); expect(MessageReaction.add).toHaveBeenCalledWith('11', 'human-1', '👀'); + // The mirror is not a fast path any more: one query, the message context. + expect(pool.query).toHaveBeenCalledTimes(1); }); - test('human NOT in pg pod_members but IN mongo pod.members is still allowed (dual-DB drift, 2026-07-24)', async () => { - pool.query - .mockResolvedValueOnce(messageLookup('pod-drift')) // loadMessageContext - .mockResolvedValueOnce(memberLookup(0)); // pg pod_members MISS → must fall back to Mongo - Pod.findById.mockReturnValue({ - select: () => ({ lean: () => Promise.resolve({ members: [{ toString: () => 'human-2' }] }) }), - }); + test('human IN mongo pod.members with no PG row is still allowed (the mirror may lag, 2026-07-24)', async () => { + pool.query.mockResolvedValueOnce(messageLookup('pod-drift')); // loadMessageContext + podListing('human-2'); const req = { params: { messageId: '12' }, body: { emoji: '👍' }, user: { _id: 'human-2' } }; const res = buildRes(); @@ -211,11 +212,9 @@ describe('reactionController.addReaction — agent runtime path', () => { expect(MessageReaction.add).toHaveBeenCalledWith('12', 'human-2', '👍'); }); - test('human in neither pg pod_members nor mongo members → 403', async () => { - pool.query - .mockResolvedValueOnce(messageLookup('pod-x')) - .mockResolvedValueOnce(memberLookup(0)); - Pod.findById.mockReturnValue({ select: () => ({ lean: () => Promise.resolve({ members: [] }) }) }); + test('human in no Mongo membership list → 403', async () => { + pool.query.mockResolvedValueOnce(messageLookup('pod-x')); + podListing(); const req = { params: { messageId: '13' }, body: { emoji: '👍' }, user: { _id: 'stranger' } }; const res = buildRes(); @@ -245,9 +244,8 @@ describe('reactionController.addReaction — agent runtime path', () => { }); test('new human reaction to an agent message queues one unclaimable acknowledgement for its author', async () => { - pool.query - .mockResolvedValueOnce(messageLookup('pod-receipt', 'author-bot')) - .mockResolvedValueOnce(memberLookup(1)); + pool.query.mockResolvedValueOnce(messageLookup('pod-receipt', 'author-bot')); + podListing('human-reactor'); MessageReaction.add.mockResolvedValueOnce(true); User.findById.mockReturnValue({ select: () => ({ @@ -342,8 +340,8 @@ describe('reactionController.addReaction — agent runtime path', () => { test('derives the same instance suffix that a token-authenticated recipient polls', async () => { pool.query - .mockResolvedValueOnce(messageLookup('pod-suffix', 'suffix-bot')) - .mockResolvedValueOnce(memberLookup(1)); + .mockResolvedValueOnce(messageLookup('pod-suffix', 'suffix-bot')); + podListing('human-reactor'); MessageReaction.add.mockResolvedValueOnce(true); User.findById.mockReturnValue({ select: () => ({ @@ -370,8 +368,8 @@ describe('reactionController.addReaction — agent runtime path', () => { test('logs and skips an acknowledgement when a legacy bot has no routable agentName', async () => { pool.query - .mockResolvedValueOnce(messageLookup('pod-legacy', 'legacy-bot')) - .mockResolvedValueOnce(memberLookup(1)); + .mockResolvedValueOnce(messageLookup('pod-legacy', 'legacy-bot')); + podListing('human-reactor'); MessageReaction.add.mockResolvedValueOnce(true); User.findById.mockReturnValue({ select: () => ({ @@ -401,8 +399,8 @@ describe('reactionController.addReaction — agent runtime path', () => { test('an idempotent duplicate reaction does not wake the agent a second time', async () => { pool.query - .mockResolvedValueOnce(messageLookup('pod-idempotent', 'author-bot')) - .mockResolvedValueOnce(memberLookup(1)); + .mockResolvedValueOnce(messageLookup('pod-idempotent', 'author-bot')); + podListing('human-reactor'); MessageReaction.add.mockResolvedValueOnce(false); const req = { @@ -421,8 +419,8 @@ describe('reactionController.addReaction — agent runtime path', () => { test('a reaction to a human message does not enqueue an agent event', async () => { pool.query - .mockResolvedValueOnce(messageLookup('pod-human-author', 'human-author')) - .mockResolvedValueOnce(memberLookup(1)); + .mockResolvedValueOnce(messageLookup('pod-human-author', 'human-author')); + podListing('human-reactor'); MessageReaction.add.mockResolvedValueOnce(true); User.findById.mockReturnValue({ select: () => ({ lean: () => Promise.resolve({ isBot: false, username: 'human-author' }) }), diff --git a/backend/__tests__/unit/services/podWriteAccessService.membership.test.js b/backend/__tests__/unit/services/podWriteAccessService.membership.test.js new file mode 100644 index 000000000..fbd578e98 --- /dev/null +++ b/backend/__tests__/unit/services/podWriteAccessService.membership.test.js @@ -0,0 +1,146 @@ +// TASK-162, second site. `callerHasPodWriteAccess` is the write gate for the +// dual-auth endpoints (reactions, thread state) and it used to read the PG +// `pod_members` mirror FIRST, returning true on a row alone. The mirror is not +// authoritative: `PGPod.create` inserts the owner unconditionally and +// `syncPodFromMongo` backfills Mongo's `createdBy`, so a row outlives the +// membership. Production had 77 such rows, 36 of them the pod's own creator +// (Vera 74648). +// +// The arm that discriminates is the SURVIVOR — the row still there, Mongo +// membership gone — because "leave, then write is refused" also passes while +// the row is absent, which is the state a mirror-only fix would produce. +// +// The PG pool is a real pg-mem instance holding a real row, so the ghost is a +// fixture rather than a mock's opinion; if someone reinstates the PG fast path +// this suite grants with it. + +const { newDb } = require('pg-mem'); + +const mockDb = newDb(); +const mockPool = new (mockDb.adapters.createPg().Pool)(); +jest.mock('../../../config/db-pg', () => ({ pool: mockPool })); + +const mongoose = require('mongoose'); + +const Pod = require('../../../models/Pod'); +const { AgentInstallation } = require('../../../models/AgentRegistry'); +const { callerHasPodWriteAccess, getCallerId } = require('../../../services/podWriteAccessService'); + +jest.mock('../../../models/Pod'); +jest.mock('../../../models/AgentRegistry'); + +const podId = new mongoose.Types.ObjectId().toString(); +const memberId = new mongoose.Types.ObjectId().toString(); +const departedId = new mongoose.Types.ObjectId().toString(); +const outsiderId = new mongoose.Types.ObjectId().toString(); + +const humanReq = (id) => ({ userId: id, user: { _id: id } }); + +const mongoPod = (doc) => { + Pod.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue(doc) }), + }); +}; + +const seedPgRow = async (userId) => { + await mockPool.query('INSERT INTO pod_members (pod_id, user_id) VALUES ($1, $2)', [podId, userId]); +}; + +const pgRowCount = async (userId) => { + const r = await mockPool.query('SELECT 1 FROM pod_members WHERE pod_id = $1 AND user_id = $2', [podId, userId]); + return r.rows.length; +}; + +const installationFound = (found) => { + AgentInstallation.findOne.mockReturnValue({ lean: jest.fn().mockResolvedValue(found ? { _id: 'i1' } : null) }); +}; + +describe('pod write access reads Mongo membership, never the PG mirror (TASK-162)', () => { + beforeAll(async () => { + await mockPool.query('CREATE TABLE pod_members (pod_id text, user_id text, PRIMARY KEY (pod_id, user_id))'); + }); + + beforeEach(async () => { + await mockPool.query('DELETE FROM pod_members'); + jest.clearAllMocks(); + installationFound(false); + }); + + test('refuses a caller whose PG row outlived their membership', async () => { + // The survivor, and the whole point: the mirror says yes, Mongo says no. + await seedPgRow(departedId); + mongoPod({ members: [memberId] }); + + await expect(callerHasPodWriteAccess(podId, departedId, humanReq(departedId))).resolves.toBe(false); + // Control on the fixture itself: the row really is present, so a false here + // is the membership rule and not an empty table. + await expect(pgRowCount(departedId)).resolves.toBe(1); + }); + + test('refuses a departed CREATOR whose PG row is present', async () => { + // Two clauses at once: the stale row, and the permissive creator bypass that + // TASK-161 removed from the connector path. Neither may reach this gate. + await seedPgRow(departedId); + mongoPod({ createdBy: departedId, members: [memberId] }); + + await expect(callerHasPodWriteAccess(podId, departedId, humanReq(departedId))).resolves.toBe(false); + }); + + test('refuses a caller for a pod Mongo no longer has, whatever PG holds', async () => { + await seedPgRow(outsiderId); + mongoPod(null); + + await expect(callerHasPodWriteAccess(podId, outsiderId, humanReq(outsiderId))).resolves.toBe(false); + }); + + test('admits a listed member, and leaves the mirror alone rather than needing it', async () => { + mongoPod({ members: [memberId] }); + + await expect(callerHasPodWriteAccess(podId, memberId, humanReq(memberId))).resolves.toBe(true); + await expect(pgRowCount(memberId)).resolves.toBe(0); + }); + + test('admits an agent caller through its active installation, before any membership read', async () => { + installationFound(true); + mongoPod({ members: [] }); + + await expect(callerHasPodWriteAccess(podId, departedId, { agentUser: { _id: departedId } })).resolves.toBe(true); + expect(Pod.findById).not.toHaveBeenCalled(); + }); + + test('refuses an agent caller with no installation and no Mongo membership', async () => { + installationFound(false); + mongoPod({ members: [memberId] }); + + await expect(callerHasPodWriteAccess(podId, departedId, { agentUser: { _id: departedId } })).resolves.toBe(false); + }); + + test('refuses a member entry in a shape `createMessage` would also refuse', async () => { + // The narrowing this row makes visible. `models/Pod.ts:157` stores + // `members` as ObjectIds and `createMessage` compares + // `memberId.toString() === userIdStr`, so a `{ userId }` entry is not a + // membership the pod's own write path accepts — and this gate read it as + // one (`m?.userId?.toString?.() || m`). The arm states the divergence + // rather than leaving it implied, so if production turns out to hold such + // entries the arm gets inverted with the census that says so. + mongoPod({ members: [{ userId: { toString: () => memberId } }] }); + + await expect(callerHasPodWriteAccess(podId, memberId, humanReq(memberId))).resolves.toBe(false); + }); + + test('refuses an agent fallback in that same shape, so both branches hold one rule', async () => { + // The agent branch had its own inline copy of the membership test, and the + // copy — not the rule — was what honoured `{ userId }`. Both branches now + // read the same predicate, so this arm is the second half of the narrowing + // above: a shape `createMessage` refuses is refused here whichever auth + // path the caller used. + installationFound(false); + mongoPod({ members: [{ userId: { toString: () => departedId } }] }); + + await expect(callerHasPodWriteAccess(podId, departedId, { agentUser: { _id: departedId } })).resolves.toBe(false); + }); + + test('getCallerId reads the agent shape too, not just req.user/req.userId', async () => { + expect(getCallerId({ agentUser: { _id: 'a1' } })).toBe('a1'); + }); +}); diff --git a/backend/controllers/pgMessageController.ts b/backend/controllers/pgMessageController.ts index 5a8325b90..2a4fcb151 100644 --- a/backend/controllers/pgMessageController.ts +++ b/backend/controllers/pgMessageController.ts @@ -5,6 +5,8 @@ const PGPod = require('../models/pg/Pod'); // eslint-disable-next-line global-require const PGMessage = require('../models/pg/Message'); // eslint-disable-next-line global-require +const { isListedPodMember } = require('../utils/isPodMember'); +// eslint-disable-next-line global-require const MongoPod = require('../models/Pod'); // eslint-disable-next-line global-require const { deliverMessageToAgents } = require('../services/messageAgentDeliveryService'); @@ -30,22 +32,37 @@ type CreatedMessage = { userId?: { username?: string } | string; }; -// Check if user is a member via PG, falling back to MongoDB as source of truth -async function isMemberWithFallback(podId: string, userId: string): Promise { - const pgMember = await PGPod.isMember(podId, userId); - if (pgMember) return true; - // Fall back to MongoDB (may throw CastError for invalid ObjectId — treat as not found) +// Mongo `members` is the membership truth for the PG chat path (TASK-162). The +// PG `pod_members` row is a lazily-synced mirror, and it used to be trusted as +// proof: `PGPod.create` inserts the owner unconditionally and `syncPodFromMongo` +// backfills Mongo's `createdBy`, so a leave plus any later backfill re-created a +// row for someone no longer in the pod. Measured read-only on production +// (Vera 74648, 727 rows): 77 rows present in PG and absent from their pod's +// Mongo `members` — 36 of them the pod's own creator — plus 140 rows pointing at +// a pod id Mongo does not have, which passed membership because the fallback +// never consulted Mongo. Reading Mongo at request time settles both classes, +// and the stored-row cleanup is a separate script. +// +// The rule is the one `createMessage` runs (controllers/messageController.ts): +// `pod.members` alone. A PG write must not admit anyone the pod's own write path +// would refuse. +async function isPodMemberInMongo(podId: string, userId: string): Promise { try { - const mongoPod = await MongoPod.findById(podId).lean() as { - members?: Array<{ toString(): string }>; - } | null; - if (!mongoPod) return false; - const inMongo = (mongoPod.members || []).some((m) => m.toString() === userId.toString()); - if (inMongo) { - // Sync this member to PG for future requests - await PGPod.addMember(podId, userId).catch(() => {}); - } - return inMongo; + // CastError for a malformed id and a null row for a pod Mongo no longer has + // both mean "not a member" rather than an error. + const mongoPod = await MongoPod.findById(podId).select('members').lean(); + if (!isListedPodMember(mongoPod, userId)) return false; + // Warm the mirror the PG listings still read. This write grants nothing: + // the row was the defect, not the fix. Failure to warm a cache must not deny + // a member either, so it is swallowed rather than failing the request — and + // it is awaited, not detached, so a rejection cannot surface as an + // unhandled one. + try { + await PGPod.addMember(podId, userId); + } catch { + // cache write only + } + return true; } catch { return false; } @@ -83,7 +100,7 @@ exports.getMessages = async (req: AuthRequest, res: Response): Promise => } } - const isMember = await isMemberWithFallback(podId, userId); + const isMember = await isPodMemberInMongo(podId, userId); if (!isMember) { res.status(401).json({ msg: 'Not authorized to view messages in this pod' }); return; @@ -132,7 +149,7 @@ exports.createMessage = async (req: AuthRequest, res: Response): Promise = return; } - const isMember = await isMemberWithFallback(podId, userId); + const isMember = await isPodMemberInMongo(podId, userId); if (!isMember) { res.status(401).json({ msg: 'Not authorized to post in this pod' }); return; diff --git a/backend/services/pgPodSyncService.ts b/backend/services/pgPodSyncService.ts index 881b77c41..d162d5686 100644 --- a/backend/services/pgPodSyncService.ts +++ b/backend/services/pgPodSyncService.ts @@ -41,9 +41,14 @@ export async function syncPodFromMongo( // `created_by` MUST mirror Mongo's real owner, never whoever happened to // trigger the backfill. PGPod.create also inserts created_by into // pod_members, so attributing it to the requester manufactured a membership - // row for a non-member — which `isMemberWithFallback` and - // `reactionController.callerHasPodAccess` then trusted as proof of access, - // and which the now-removed pg deletePod trusted as proof of ownership. + // row for a non-member — which the PG-first membership checks then trusted as + // proof of access, and which the now-removed pg deletePod trusted as proof of + // ownership. Those checks read Mongo membership at request time since + // TASK-162 (pgMessageController.isPodMemberInMongo, + // podWriteAccessService.callerHasPodWriteAccess), so the row this creates is + // a mirror for the PG listing surfaces and no longer decides access — but it + // is still written from Mongo's owner, because the mirror should say what the + // pod says. const ownerId = mongoPod.createdBy ? String(mongoPod.createdBy) : requestingUserId; const pod = await PGPod.create( mongoPod.name, diff --git a/backend/services/podWriteAccessService.ts b/backend/services/podWriteAccessService.ts index cb08782cf..2093196cd 100644 --- a/backend/services/podWriteAccessService.ts +++ b/backend/services/podWriteAccessService.ts @@ -32,15 +32,23 @@ export function getCallerId(req: PodAccessReq): string { * Returns true when the caller may write into the pod. For agent callers we * check AgentInstallation first (per "AgentInstallation required for posting"), * then fall back to Pod.members for agents installed via the runtime/room - * handoff. For human callers we check the PG pod_members mirror first because - * it is the fast path, then Mongo — the source of truth — because community - * auto-join and several other join paths write Mongo only. + * handoff. For human callers Mongo `members` decides, and only Mongo: community + * auto-join and several other join paths write Mongo only, and the PG + * `pod_members` mirror can outlive the membership it mirrors — `PGPod.create` + * inserts the owner unconditionally and `syncPodFromMongo` backfills Mongo's + * `createdBy`, so 77 rows on production belong to pods whose Mongo `members` no + * longer carry them, 36 of those the pod's own creator (Vera 74648, TASK-162). + * A mirror is a fast path only while it cannot be wrong in the direction that + * grants access. */ export async function callerHasPodWriteAccess( podId: string, userId: string, req: PodAccessReq, ): Promise { + const { isListedPodMember } = require('../utils/isPodMember'); + const Pod = require('../models/Pod'); + if (req.agentUser?._id) { const { AgentInstallation } = require('../models/AgentRegistry'); const installation = await AgentInstallation.findOne({ @@ -49,20 +57,15 @@ export async function callerHasPodWriteAccess( status: 'active', }).lean(); if (installation) return true; - const Pod = require('../models/Pod'); const pod = await Pod.findById(podId).select('members').lean(); - return Boolean(pod?.members?.some((m: any) => String(m?.userId?.toString?.() || m) === userId)); + return isListedPodMember(pod, userId); } - const { pool } = require('../config/db-pg'); - const result = await pool.query( - 'SELECT 1 FROM pod_members WHERE pod_id = $1 AND user_id = $2 LIMIT 1', - [podId, userId], - ); - if ((result.rowCount || 0) > 0) return true; - const Pod = require('../models/Pod'); + // No PG read here on purpose. The mirror used to be checked first, and a + // stale positive is the whole defect: a row that survives a leave concluded + // membership for a caller the pod's own write path refuses. const pod = await Pod.findById(podId).select('members').lean(); - return Boolean(pod?.members?.some((mem: any) => String(mem?.userId?.toString?.() || mem) === userId)); + return isListedPodMember(pod, userId); } module.exports = { getCallerId, callerHasPodWriteAccess };