diff --git a/backend/__tests__/unit/routes/installables.test.js b/backend/__tests__/unit/routes/installables.test.js index 1c4845e42..78e1e7e2a 100644 --- a/backend/__tests__/unit/routes/installables.test.js +++ b/backend/__tests__/unit/routes/installables.test.js @@ -125,6 +125,26 @@ describe('installable connector routes', () => { expect(installationService.install).not.toHaveBeenCalled(); }); + it('rejects a departed CREATOR before any install row is claimed', async () => { + // TASK-161: `leavePod` filters `members` and never clears `createdBy`, so + // this pod has no members and still names the caller as its creator. The + // install route used to admit them through the creator clause; installing + // seeds the pod's gate ON, so it must take the pod's write path instead. + Pod.findById.mockResolvedValue({ + _id: podId, + createdBy: { toString: () => '64b64c48c4f37a6b2f34c111' }, + members: [], + }); + + const res = await request(app) + .post('/api/installables/telegram/install') + .set(auth) + .send({ podId }); + + expect(res.status).toBe(403); + expect(installationService.install).not.toHaveBeenCalled(); + }); + it('rejects an invalid podId without querying a pod or claiming an install', async () => { const response = await request(app) .post('/api/installables/telegram/install') diff --git a/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js b/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js index e8ad1f12c..12c8ca5a8 100644 --- a/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js +++ b/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js @@ -29,7 +29,6 @@ jest.mock('../../../middleware/integrationRateLimit', () => ({ listIntegrationsRateLimit: (_req, _res, next) => next(), })); jest.mock('../../../services/dmService', () => ({ canViewPod: jest.fn(() => true) })); -jest.mock('../../../utils/isPodMember', () => jest.fn(() => true)); jest.mock('jsonwebtoken', () => ({ sign: jest.fn(() => 't'), verify: jest.fn(), decode: jest.fn() })); // The create path initializes and connects the provider; only the row matters. jest.mock('../../../services/discordService', () => jest.fn().mockImplementation(() => ({ diff --git a/backend/__tests__/unit/routes/integrations.linkedUserId.test.js b/backend/__tests__/unit/routes/integrations.linkedUserId.test.js index f578e8e3e..b804df98e 100644 --- a/backend/__tests__/unit/routes/integrations.linkedUserId.test.js +++ b/backend/__tests__/unit/routes/integrations.linkedUserId.test.js @@ -220,7 +220,7 @@ describe('PATCH /api/integrations/:id — user-scoped connector gates', () => { it('checks every requested gate before writing any of them', async () => { Pod.findById - .mockResolvedValueOnce({ _id: allowedPodId, createdBy: 'user-1', members: [] }) + .mockResolvedValueOnce({ _id: allowedPodId, createdBy: 'someone-else', members: ['user-1'] }) .mockResolvedValueOnce({ _id: forbiddenPodId, createdBy: 'someone-else', members: [] }); const res = await request(app) @@ -239,7 +239,9 @@ describe('PATCH /api/integrations/:id — user-scoped connector gates', () => { }); it('allows the linked owner to write gates for pods they belong to', async () => { - Pod.findById.mockResolvedValue({ _id: allowedPodId, createdBy: 'user-1', members: [] }); + // Listed in `members`, which is what "belong to" means — the fixture used to + // leave `members` empty and lean on `createdBy` (TASK-161). + Pod.findById.mockResolvedValue({ _id: allowedPodId, createdBy: 'someone-else', members: ['user-1'] }); const res = await request(app) .patch(`/api/integrations/${userScopedIntegrationId}`) @@ -251,7 +253,7 @@ describe('PATCH /api/integrations/:id — user-scoped connector gates', () => { }); it('allows the linked owner to select a member pod as the active inbound destination', async () => { - Pod.findById.mockResolvedValue({ _id: allowedPodId, createdBy: 'user-1', members: [] }); + Pod.findById.mockResolvedValue({ _id: allowedPodId, createdBy: 'someone-else', members: ['user-1'] }); const res = await request(app) .patch(`/api/integrations/${userScopedIntegrationId}`) @@ -262,6 +264,37 @@ describe('PATCH /api/integrations/:id — user-scoped connector gates', () => { expect(update.podId).toBe(allowedPodId); }); + it.each([ + ['created and then left', { _id: 'pod', createdBy: 'user-1', members: [] }], + ['never a member', { _id: 'pod', createdBy: 'someone-else', members: [] }], + ])('refuses a gate for a pod the linked owner %s', async (_label, podDoc) => { + // TASK-161: `leavePod` filters `members` and never clears `createdBy`, so + // the first cell is a creator who left. Both must refuse, or a connector + // could aim inbound messages at a pod its owner cannot post in. + Pod.findById.mockResolvedValue({ ...podDoc, _id: allowedPodId }); + + const res = await request(app) + .patch(`/api/integrations/${userScopedIntegrationId}`) + .send({ config: { gates: { [allowedPodId]: { enabled: true } } } }); + + expect(res.status).toBe(403); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + }); + + it.each([ + ['created and then left', { _id: 'pod', createdBy: 'user-1', members: [] }], + ['never a member', { _id: 'pod', createdBy: 'someone-else', members: [] }], + ])('refuses selecting a pod the linked owner %s as the active destination', async (_label, podDoc) => { + Pod.findById.mockResolvedValue({ ...podDoc, _id: allowedPodId }); + + const res = await request(app) + .patch(`/api/integrations/${userScopedIntegrationId}`) + .send({ podId: allowedPodId }); + + expect(res.status).toBe(403); + expect(Integration.findByIdAndUpdate).not.toHaveBeenCalled(); + }); + it('refuses selecting an active pod the linked owner is no longer a member of', async () => { Pod.findById.mockResolvedValue({ _id: forbiddenPodId, createdBy: 'someone-else', members: [] }); diff --git a/backend/__tests__/unit/routes/integrations.manifestStatus.test.js b/backend/__tests__/unit/routes/integrations.manifestStatus.test.js index 0194c20b6..2ab5bf596 100644 --- a/backend/__tests__/unit/routes/integrations.manifestStatus.test.js +++ b/backend/__tests__/unit/routes/integrations.manifestStatus.test.js @@ -21,7 +21,6 @@ jest.mock('../../../middleware/integrationRateLimit', () => ({ listIntegrationsRateLimit: (_req, _res, next) => next(), })); jest.mock('../../../services/dmService', () => ({ canViewPod: jest.fn() })); -jest.mock('../../../utils/isPodMember', () => jest.fn(() => true)); jest.mock('jsonwebtoken', () => ({ sign: jest.fn(() => 't'), verify: jest.fn(), decode: jest.fn() })); const { MongoMemoryServer } = require('mongodb-memory-server'); diff --git a/backend/__tests__/unit/routes/integrations.routingState.test.js b/backend/__tests__/unit/routes/integrations.routingState.test.js index 57ea7c749..3b3ad552c 100644 --- a/backend/__tests__/unit/routes/integrations.routingState.test.js +++ b/backend/__tests__/unit/routes/integrations.routingState.test.js @@ -24,7 +24,6 @@ jest.mock('../../../middleware/integrationRateLimit', () => ({ listIntegrationsRateLimit: (_req, _res, next) => next(), })); jest.mock('../../../services/dmService', () => ({ canViewPod: jest.fn() })); -jest.mock('../../../utils/isPodMember', () => jest.fn(() => true)); jest.mock('jsonwebtoken', () => ({ sign: jest.fn(() => 't'), verify: jest.fn(), decode: jest.fn() })); const { MongoMemoryServer } = require('mongodb-memory-server'); diff --git a/backend/__tests__/unit/routes/integrations.validation.test.js b/backend/__tests__/unit/routes/integrations.validation.test.js index c88faa82e..9804a0586 100644 --- a/backend/__tests__/unit/routes/integrations.validation.test.js +++ b/backend/__tests__/unit/routes/integrations.validation.test.js @@ -64,7 +64,13 @@ describe('integration manifest validation', () => { beforeEach(() => { jest.clearAllMocks(); - Pod.findById.mockResolvedValue({ _id: 'pod-1', createdBy: { toString: () => 'user-1' } }); + // The pod lists the caller: a real pod lists its creator, and the + // connector sites read `pod.members` alone (TASK-161). + Pod.findById.mockResolvedValue({ + _id: 'pod-1', + createdBy: { toString: () => 'user-1' }, + members: [{ toString: () => 'user-1' }], + }); User.findById.mockResolvedValue({ _id: 'user-1' }); consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); Integration.findOne.mockResolvedValue(null); @@ -209,4 +215,30 @@ describe('integration manifest validation', () => { expect(res.status).toBe(400); expect(res.body.missing).toEqual(expect.arrayContaining(['channelId'])); }); + + it('refuses to create a connection for a pod the caller created and then left', async () => { + // TASK-161, from Vera 74671: `:393` is the write gate, and the population it + // exists to refuse is exactly the departed creator — the one person the + // permissive predicate admitted. Every other create arm here lists the + // caller in `members`, so before this arm the site had no witness at all: + // `createdBy` merely identifies the owner and must not stand in for it. + Pod.findById.mockResolvedValue({ + _id: 'pod-1', + createdBy: { toString: () => 'user-1' }, + members: [], + }); + const before = Integration.__getLastInstance(); + + const res = await request(app) + .post('/api/integrations') + .send({ + podId: 'pod-1', + type: 'groupme', + config: { webhookListenerEnabled: true }, + }); + + expect(res.status).toBe(403); + expect(res.body.message).toBe('Access denied'); + expect(Integration.__getLastInstance()).toBe(before); + }); }); diff --git a/backend/__tests__/unit/routes/slackOAuth.installables.test.js b/backend/__tests__/unit/routes/slackOAuth.installables.test.js index cd1e4d4b5..8e12b5be1 100644 --- a/backend/__tests__/unit/routes/slackOAuth.installables.test.js +++ b/backend/__tests__/unit/routes/slackOAuth.installables.test.js @@ -271,6 +271,25 @@ describe('Slack installable OAuth routes', () => { expect(SlackApi.mock.results[0].value.postMessage).toHaveBeenCalledWith('D1', '[<Evil|pod>] connected'); }); + test('refuses to confirm a bind whose pod the caller created and left', async () => { + // TASK-161. `leavePod` filters `members` and never clears `createdBy`; the + // permissive predicate counted that as membership, so the confirm step — + // which stores the chat a connector will speak into — admitted a person the + // pod's own write path refuses. `slack_pod_access_denied` had no arm at all. + const pending = { + teamId: 'T1', slackUserId: 'U1', chatId: 'D1', botTokenRef: 'secret-ref', + expiresAt: new Date(Date.now() + 60_000), + }; + Integration.findOne.mockResolvedValue({ ...integration, config: { pendingBind: pending } }); + Pod.findById.mockResolvedValueOnce({ createdBy: String(ownerId), members: [] }); + + const response = await request(app).post('/api/installables/slack/confirm'); + + expect(response.status).toBe(403); + expect(response.body.code).toBe('slack_pod_access_denied'); + expect(Integration.findOneAndUpdate).not.toHaveBeenCalled(); + }); + test('a reconnect clears the reason an earlier flip left on the row (wren 73838)', async () => { const pending = { teamId: 'T1', slackUserId: 'U1', chatId: 'D1', botTokenRef: 'secret-ref', diff --git a/backend/__tests__/unit/services/connectorRelayPolicy.test.js b/backend/__tests__/unit/services/connectorRelayPolicy.test.js index e04a9d50a..cb7ea0177 100644 --- a/backend/__tests__/unit/services/connectorRelayPolicy.test.js +++ b/backend/__tests__/unit/services/connectorRelayPolicy.test.js @@ -72,8 +72,21 @@ describe('connectorRelayPolicy — the routed-target conjunction', () => { expect(isRoutedPodTarget(target({ pod: undefined }))).toBe(false); }); - it('admits a pod whose creator is not listed in members', () => { - expect(isRoutedPodTarget(target({ pod: pod({ createdBy: 'user-1' }) }))).toBe(true); + it('refuses a pod whose creator is not listed in members', () => { + // TASK-161. `leavePod` filters `members` and never clears `createdBy`, so + // this cell IS "a creator who has left" — and the pod's own write path + // (`createMessage`) 401s them. The permissive `isPodMember` counted them as + // a member so that a pod whose `members` forgot its creator still + // authorises them; a connector must reach the same verdict as the write. + expect(isRoutedPodTarget(target({ pod: pod({ createdBy: 'user-1' }) }))).toBe(false); + }); + + it('still admits a creator who is also listed — the clause is gone, not the creator', () => { + // The complement, so the arm above cannot pass by refusing every pod that + // names a creator at all. + expect(isRoutedPodTarget(target({ + pod: pod({ createdBy: 'user-1', members: ['user-1'] }), + }))).toBe(true); }); it('refuses a missing user id rather than stringifying it into a match', () => { diff --git a/backend/__tests__/unit/services/decisionCardReconcileService.test.js b/backend/__tests__/unit/services/decisionCardReconcileService.test.js index 75eb27823..6bc349c68 100644 --- a/backend/__tests__/unit/services/decisionCardReconcileService.test.js +++ b/backend/__tests__/unit/services/decisionCardReconcileService.test.js @@ -301,6 +301,53 @@ describe('decision card closure fan-out', () => { expect((await Integration.findById(missingLink._id)).config.cards[0].closedAt).toEqual(expect.any(Date)); }); + test('sends no closing line to a creator who left, while a listed sibling still receives one', async () => { + // TASK-161, from Vera 74671: `canSendClosingLine` had no witness for the + // population the permissive predicate admitted. `createdBy` identifies the + // pod's creator; it is not membership, and this arm is the one that says so. + // The listed sibling is the control — without it a `not.toHaveBeenCalled` + // would be indistinguishable from an arm that never ran. + const thirdId = new mongoose.Types.ObjectId(); + Pod.findById.mockImplementation(() => chain({ createdBy: memberId, members: [ownerId, thirdId] })); + const origin = await Integration.create({ + podId, scope: 'user', type: 'telegram', createdBy: ownerId, isActive: true, status: 'connected', + config: { + liveRelay: true, linkedUserId: String(ownerId), chatType: 'private', chatId: 'origin', + gates: { [String(podId)]: { enabled: true, since: new Date() } }, + cards: [{ podMessageId: cardId, tgMessageId: '11', sentAt: new Date() }], + }, + }); + const leftCreator = await Integration.create({ + podId, scope: 'user', type: 'telegram', createdBy: memberId, isActive: true, status: 'connected', + config: { + liveRelay: true, linkedUserId: String(memberId), chatType: 'private', chatId: 'left-creator', + gates: { [String(podId)]: { enabled: true, since: new Date() } }, + cards: [{ podMessageId: cardId, tgMessageId: '12', sentAt: new Date() }], + }, + }); + const listed = await Integration.create({ + podId, scope: 'user', type: 'telegram', createdBy: thirdId, isActive: true, status: 'connected', + config: { + liveRelay: true, linkedUserId: String(thirdId), chatType: 'private', chatId: 'listed', + gates: { [String(podId)]: { enabled: true, since: new Date() } }, + cards: [{ podMessageId: cardId, tgMessageId: '13', sentAt: new Date() }], + }, + }); + + await fanoutDecisionClosure( + { _id: new mongoose.Types.ObjectId(), podId, messageId: cardId, ruling: { value: 'Now', byUsername: 'Sam' } }, + { via: 'workspace', integrationId: origin._id }, + ); + + expect(telegramSend.sendMessage).toHaveBeenCalledTimes(1); + expect(telegramSend.sendMessage).toHaveBeenCalledWith( + 'telegram-token', 'listed', '✓ Ruled by Sam: Now', { replyToMessageId: '13', plainText: true }, + ); + // Closure is unconditional (it is a receipt, not a delivery), so the + // departure shows up in the send and not in `closedAt`. + expect((await Integration.findById(leftCreator._id)).config.cards[0].closedAt).toEqual(expect.any(Date)); + }); + test('returns after durable closure while a sibling provider send is still pending', async () => { let releaseSend; const pendingSend = new Promise((resolve) => { releaseSend = resolve; }); diff --git a/backend/__tests__/unit/services/decisionCardReply.bridges.test.js b/backend/__tests__/unit/services/decisionCardReply.bridges.test.js index c86783881..79889b5c8 100644 --- a/backend/__tests__/unit/services/decisionCardReply.bridges.test.js +++ b/backend/__tests__/unit/services/decisionCardReply.bridges.test.js @@ -177,6 +177,20 @@ describe.each(['telegram', 'slack'])('%s decision reply', (provider) => { expect(confirmation()).not.toContain('secret'); }); + test('refuses a card reply from a pod creator who left, recording nothing', async () => { + // TASK-161, from Vera 74671: wren's ruling named this as witness (v) and the + // arm did not exist. The arm above departs through `members`; the population + // the permissive predicate admitted is the CREATOR who left — present in + // `createdBy`, absent from `members`. `assertOpen` is the other half of the + // claim: the refusal has to leave the standing ruling and the ledger alone. + Pod.findById.mockImplementation(() => chain({ name: 'Launch', members: [], createdBy: ownerId })); + await receive(); + expect(choose).not.toHaveBeenCalled(); + expect(messages).toHaveLength(0); + await assertOpen(); + expect(confirmation()).toContain("You're no longer in Launch"); + }); + test('409-ruled: loser text goes under ask, not winner; standing ruling and ledger stay unchanged', async () => { await receive(); const original = await ledger(); diff --git a/backend/__tests__/unit/services/installableEventHandlers.test.js b/backend/__tests__/unit/services/installableEventHandlers.test.js index 2d396057a..e19bb2514 100644 --- a/backend/__tests__/unit/services/installableEventHandlers.test.js +++ b/backend/__tests__/unit/services/installableEventHandlers.test.js @@ -455,24 +455,36 @@ describe('installable event dispatcher', () => { // reddens as well. describe('the outbound selector agrees with connectorRelayPolicy', () => { // The matrix the row names: member / non-member × gate on / off / absent. + // + // TASK-161 adds the CREATOR cell — a subject who is the pod's `createdBy` and + // is not listed in `members`, i.e. a creator who has left. It is the cell + // where the two sides used to agree on "yes": the selector unioned + // `pod.createdBy` into memberIds exactly as the permissive `isPodMember` + // did, so green meant the wrong answer rather than an untested one. Both + // sides now exclude it, and this asserts the exclusion rather than the + // agreement alone. const CELLS = [ - { label: 'member, gate on', member: true, gate: true }, - { label: 'member, gate off', member: true, gate: false }, - { label: 'member, gate absent', member: true, gate: null }, - { label: 'non-member, gate on', member: false, gate: true }, - { label: 'non-member, gate off', member: false, gate: false }, - { label: 'non-member, gate absent', member: false, gate: null }, + { label: 'member, gate on', membership: 'listed', gate: true }, + { label: 'member, gate off', membership: 'listed', gate: false }, + { label: 'member, gate absent', membership: 'listed', gate: null }, + { label: 'non-member, gate on', membership: 'absent', gate: true }, + { label: 'non-member, gate off', membership: 'absent', gate: false }, + { label: 'non-member, gate absent', membership: 'absent', gate: null }, + { label: 'creator not listed, gate on', membership: 'creator', gate: true }, + { label: 'creator not listed, gate off', membership: 'creator', gate: false }, + { label: 'creator not listed, gate absent', membership: 'creator', gate: null }, ]; it.each(CELLS)('$label: the selector and the predicate reach the same verdict', async ({ - member, gate, + membership, gate, }) => { const podId = freshId(); - const creatorId = freshId(); const ownerId = freshId(); // Installed while the owner IS a member, then removed for the non-member - // cells — the same shape the membership arm above uses, and the only order - // that leaves the row itself unchanged between cells. + // and creator cells — the only order that leaves the row itself unchanged + // between cells. `creatorId === ownerId` is the whole difference between + // the creator cell and the plain non-member one. + const creatorId = membership === 'creator' ? ownerId : freshId(); await createPod(podId, creatorId, [ownerId]); const installed = await install({ installableId: 'telegram', installedBy: ownerId, podId }); // `install` seeds the installed pod's gate ON, so the absent cell has to @@ -489,7 +501,9 @@ describe('installable event dispatcher', () => { { $set: { [`config.gates.${podId}.enabled`]: gate } }, ); } - if (!member) await Pod.updateOne({ _id: podId }, { $pull: { members: ownerId } }); + if (membership !== 'listed') { + await Pod.updateOne({ _id: podId }, { $pull: { members: ownerId } }); + } const row = await Integration.findById(installed.integration._id).lean(); const pod = await Pod.findById(podId).lean(); @@ -507,7 +521,7 @@ describe('installable event dispatcher', () => { // Both halves, then their conjunction, then the verdict all three must // reach — so agreement-by-both-saying-no cannot pass as agreement. - const expected = member && gate === true; + const expected = membership === 'listed' && gate === true; expect(isGatedPodTarget(row, podId)).toBe(gate === true); expect(selectorSays).toBe(expected); expect(predicateSays).toBe(expected); diff --git a/backend/__tests__/unit/services/installableInstallationService.test.js b/backend/__tests__/unit/services/installableInstallationService.test.js index 740223bbf..806831db2 100644 --- a/backend/__tests__/unit/services/installableInstallationService.test.js +++ b/backend/__tests__/unit/services/installableInstallationService.test.js @@ -690,7 +690,11 @@ describe('installable connector projection', () => { it('reconciles pause projections and prunes gates when the owner leaves a pod', async () => { const { userId, podId } = ids(); - await Pod.create({ _id: podId, name: 'Current pod', type: 'team', createdBy: userId, members: [] }); + // The owner is LISTED: this arm is about the stale pod's gate, and a pod + // created-but-unlisted is a different cell with its own arm below (TASK-161). + await Pod.create({ + _id: podId, name: 'Current pod', type: 'team', createdBy: 'someone-else', members: [userId], + }); const installed = await install({ installableId: 'telegram', installedBy: userId, podId }); const stalePodId = new mongoose.Types.ObjectId().toString(); const pausedAt = new Date(); @@ -722,6 +726,49 @@ describe('installable connector projection', () => { expect((await Integration.findById(installed.integration._id)).config.adminPause).toBeUndefined(); }); + it('prunes a NON-ACTIVE gate for a pod the owner created and then left', async () => { + // TASK-161, from Vera's 74663. The connecting gate is not the active pod: + // only `sweepOrphanedGates` prunes a secondary gate, so this cell cannot be + // satisfied by the active-pod sweep — which is why the first draft of this + // arm survived the reconciler being reverted. A pod the owner CREATED and + // left still names them in `createdBy`, which the permissive predicate read + // as membership, leaving the ON switch the Connectors page shows for a pod + // whose relay now refuses every message. + // + // Two halves make this site strict, and the mutation that witnesses it has to + // restore BOTH: the predicate, and the `.select('members')` beside it. Put the + // permissive predicate back on its own and it reads `pod.createdBy` off a + // query that no longer selects it, so the clause is inert and this arm stays + // green — measured, which is why the ledger's M6 is a two-edit mutation. + const { userId, podId } = ids(); + await Pod.create({ + _id: podId, name: 'Active pod', type: 'team', createdBy: new mongoose.Types.ObjectId(), members: [userId], + }); + const installed = await install({ installableId: 'telegram', installedBy: userId, podId }); + await Integration.updateOne( + { _id: installed.integration._id }, + { $set: { 'config.chatId': 'chat-1', 'config.chatType': 'private' } }, + ); + const leftPodId = new mongoose.Types.ObjectId().toString(); + await Pod.create({ + _id: leftPodId, name: 'Created then left', type: 'team', createdBy: userId, members: [userId], + }); + await Integration.updateOne( + { _id: installed.integration._id }, + { $set: { [`config.gates.${leftPodId}`]: { enabled: true, since: new Date() } } }, + ); + await Pod.updateOne({ _id: leftPodId }, { $pull: { members: userId } }); + expect(String((await Pod.findById(leftPodId).lean()).createdBy)).toBe(String(userId)); + + const reconciled = await sweep(new Date()); + const projection = await Integration.findById(installed.integration._id).lean(); + + expect(reconciled.prunedGates).toBe(1); + expect(projection.config.gates?.[leftPodId]).toBeUndefined(); + // The active pod's own gate stays: its owner is still listed there. + expect(projection.config.gates?.[podId]).toBeDefined(); + }); + it('clears the active pod with its orphaned gate and tells the linked chat why', async () => { const { userId, podId } = ids(); await Pod.create({ diff --git a/backend/__tests__/unit/services/slackBridgeService.test.js b/backend/__tests__/unit/services/slackBridgeService.test.js index 5c288396c..ea065358c 100644 --- a/backend/__tests__/unit/services/slackBridgeService.test.js +++ b/backend/__tests__/unit/services/slackBridgeService.test.js @@ -365,4 +365,32 @@ describe('Slack installable bridge', () => { 'D1', 'This connector has no active pod. Choose one in Commonly first.', ); }); + + test('refuses inbound authorship when the linked user created the active pod and left', async () => { + // TASK-161: same refusal, different cell. The arm above has no `createdBy`; + // here the linked user IS the pod's creator, which `leavePod` leaves behind + // — so the permissive predicate admitted exactly this message. + Pod.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue({ type: 'team', createdBy: 'user-1', members: [] }), + }), + }); + + await expect(relaySlackMessageToPod({ + integration: { + ...integration, + scope: 'user', + config: { ...integration.config, linkedUserId: 'user-1', slackUserId: 'U1' }, + }, + event: { text: 'hello', user: 'U1' }, + })).resolves.toEqual({ relayed: false }); + + expect(User.findById).not.toHaveBeenCalled(); + expect(connectorSecrets.get).toHaveBeenCalledWith('secret-ref'); + const api = SlackApi.mock.results[0].value; + expect(api.postMessage).toHaveBeenCalledWith( + 'D1', + 'This connector has no active pod. Choose one in Commonly first.', + ); + }); }); diff --git a/backend/__tests__/unit/services/telegramBridgeService.test.js b/backend/__tests__/unit/services/telegramBridgeService.test.js index eb9d97e37..8af8206b7 100644 --- a/backend/__tests__/unit/services/telegramBridgeService.test.js +++ b/backend/__tests__/unit/services/telegramBridgeService.test.js @@ -345,6 +345,48 @@ describe('telegramBridgeService — multi-pod routing', () => { expect(telegramSend.sendMessage.mock.calls[0][2]).toContain('Launch'); }); + it('refuses a quote-reply into a pod the linked user created and left', async () => { + // TASK-161. The arm above is a pod whose `members` no longer name the user; + // this is the same person as the pod's `createdBy`, which `leavePod` never + // clears. The permissive predicate treated that as membership, so this is + // the cell the row measured: relayed in, and the pod's own write path 401s. + Pod.findById.mockImplementation((id) => ({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue( + String(id) === GATED_POD + ? podDoc({ name: 'Launch', createdBy: 'user-1', members: [] }) + : podDoc(), + ), + }), + })); + const result = await inbound(userScoped(), { reply_to_message: { message_id: 101 } }); + + expect(result).toEqual({ relayed: false }); + expect(PGMessage.create).not.toHaveBeenCalled(); + expect(deliverMessageToAgents).not.toHaveBeenCalled(); + expect(telegramSend.sendMessage.mock.calls[0][2]).toContain('Launch'); + }); + + it('refuses an unquoted inbound message when the ACTIVE pod\'s creator left it', async () => { + // The routed-reply arm above exercises the policy conjunction; this one is + // the bridge's own membership read on the active pod, so a future edit that + // restores the permissive predicate at either site reddens something. + Pod.findById.mockImplementation(() => ({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue(podDoc({ createdBy: 'user-1', members: [] })), + }), + })); + const result = await inbound(userScoped(), {}); + + expect(result).toEqual({ relayed: false }); + expect(PGMessage.create).not.toHaveBeenCalled(); + expect(telegramSend.sendMessage).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + 'This connector has no active pod. Choose one in Commonly first.', + ); + }); + it('routes an entry with no podId as it always has — the active pod', async () => { await inbound(userScoped(), { reply_to_message: { message_id: 103 } }); diff --git a/backend/routes/installables.ts b/backend/routes/installables.ts index 65749772d..66565bdea 100644 --- a/backend/routes/installables.ts +++ b/backend/routes/installables.ts @@ -10,7 +10,7 @@ const Pod = require('../models/Pod'); const Integration = require('../models/Integration'); const InstallableInstallation = require('../models/InstallableInstallation'); // eslint-disable-next-line global-require -const isPodMember = require('../utils/isPodMember'); +const { isListedPodMember } = require('../services/connectorRelayPolicy'); // eslint-disable-next-line global-require const { randomSecret } = require('../utils/secret'); // eslint-disable-next-line global-require @@ -403,7 +403,7 @@ router.post('/slack/confirm', writeIntegrationsRateLimit, auth, async (req: Auth return slackError(res, 409, 'slack_bind_expired', 'Slack authorization expired. Start again.'); } const pod = await Pod.findById(owned.integration.podId); - if (!pod || !isPodMember(pod, userId)) { + if (!pod || !isListedPodMember(pod, userId)) { return slackError(res, 403, 'slack_pod_access_denied', 'You no longer have access to this pod.'); } const confirmed = await Integration.findOneAndUpdate( @@ -568,7 +568,10 @@ router.post('/:installableId/install', writeIntegrationsRateLimit, auth, async ( try { const pod = await Pod.findById(podId); - if (!pod || !isPodMember(pod, userId)) { + // The pod's write path, not the permissive predicate: installing a connector + // seeds that pod's gate ON, so a creator who left must not be able to write + // install state into a pod that would refuse their messages (TASK-161). + if (!pod || !isListedPodMember(pod, userId)) { return res.status(403).json({ error: 'Access denied' }); } const result = await install({ diff --git a/backend/routes/integrations.ts b/backend/routes/integrations.ts index 5fc1b4fe4..25a55b302 100644 --- a/backend/routes/integrations.ts +++ b/backend/routes/integrations.ts @@ -35,7 +35,7 @@ const { hash, randomSecret } = require('../utils/secret'); // eslint-disable-next-line global-require const { mintConnectCode } = require('../services/telegramConnectCode'); // eslint-disable-next-line global-require -const isPodMember = require('../utils/isPodMember'); +const { isListedPodMember } = require('../services/connectorRelayPolicy'); // eslint-disable-next-line global-require const { projectIntegrationForViewer, withoutConnectCode } = require('../models/integrationPublicConfig'); // eslint-disable-next-line global-require @@ -384,11 +384,13 @@ router.post('/', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res return res.status(400).json({ message: 'linkedUserId is derived from the authenticated caller and cannot be set' }); } // Membership gate: an integration relays a pod's content outward and - // authors content into it — a WRITE, so it takes the strict predicate - // (members + creator; no admin read-bypass — #1302's isPodMember, not - // DMService.canViewPod). Plain findById: unit mocks resolve a bare doc. + // authors content into it — a WRITE, so it takes the strict predicate the + // pod's own write path runs: `pod.members` alone, no creator bypass and no + // admin read-bypass (TASK-161; DMService.canViewPod's admin clause exists + // for read observability, and would make "only members can write here" + // untrue). Plain findById: unit mocks resolve a bare doc. const targetPod = await Pod.findById(String(podId)); - if (!targetPod || !isPodMember(targetPod, req.user?.id)) { + if (!targetPod || !isListedPodMember(targetPod, req.user?.id)) { return res.status(403).json({ message: 'Access denied' }); } const relay = readRelayFlags(stripServerOwnedConfig(config)); @@ -636,15 +638,15 @@ router.patch('/:id', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: return res.status(400).json({ message: 'adminPause is managed by an administrator' }); } // A user connector has one active inbound destination. Selecting it is - // an owner action, and it must be a pod that owner can still write to; - // otherwise a browser could redirect private inbound messages into a - // pod it merely knows the id of. + // an owner action, and it must be a pod that owner can still write to — + // the same strict membership `createMessage` checks, so a creator who + // left cannot aim private inbound messages at a pod that refuses them. if (podId !== undefined) { if (typeof podId !== 'string' || !isObjectIdKey(podId)) { return res.status(400).json({ message: 'podId must be a valid pod id' }); } const activePod = await Pod.findById(podId); - if (!activePod || !isPodMember(activePod, requesterId)) { + if (!activePod || !isListedPodMember(activePod, requesterId)) { return res.status(403).json({ message: 'Access denied' }); } } @@ -667,7 +669,7 @@ router.patch('/:id', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: // a mixed valid/invalid PATCH atomic: no valid gate is written first. for (const gatePodId of gatePodIds) { const gatePod = await Pod.findById(gatePodId); - if (!gatePod || !isPodMember(gatePod, requesterId)) { + if (!gatePod || !isListedPodMember(gatePod, requesterId)) { return res.status(403).json({ message: 'Access denied' }); } } diff --git a/backend/services/connectorRelayPolicy.ts b/backend/services/connectorRelayPolicy.ts index 96521eb5a..598d3533e 100644 --- a/backend/services/connectorRelayPolicy.ts +++ b/backend/services/connectorRelayPolicy.ts @@ -3,7 +3,13 @@ // about which pod messages are allowed to interrupt a person's attention surface, // or about which pods a connector may address at all. -const isPodMember = require('../utils/isPodMember'); +// The strict membership rule, re-exported here so every connector site reads one +// definition through this module. The permissive `isPodMember` is deliberately +// NOT imported any more: its creator clause is what let a departed creator keep +// relaying (TASK-161). Defined beside `isPodMember` in utils so the platform +// readers that need the same rule (PG chat, reactions) share this one home. +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { isListedPodMember } = require('../utils/isPodMember'); export interface RelayPolicyIntegration { scope?: string; @@ -69,6 +75,11 @@ export const isGatedPodTarget = ( // failure this exists for: the gate says the connector is still subscribed, the // membership says the person it speaks for is still in the room. // +// The membership half is `isListedPodMember` — `pod.members` only, the check the +// pod's own write path runs — so a connector can never write where its owner +// would be refused. Before TASK-161 this read the permissive `isPodMember`, and a +// pod's creator who had left the pod still relayed in both directions. +// // KNOWN WINDOW, accepted: this is check-then-act. A gate switched off between // this call and the write still lets that one message through. Closing it means // making the write itself carry the condition (a conditional update or a @@ -88,11 +99,16 @@ export const isRoutedPodTarget = (opts: { const { integration, pod, podId, userId, } = opts; - // No separate user-id guard: `isPodMember` fails closed on a falsy id itself + // No separate user-id guard: the predicate fails closed on a falsy id itself // (measured — a guard here changed no arm, so it was removed rather than kept // unwitnessed). return isGatedPodTarget(integration, podId) - && isPodMember(pod, userId); + && isListedPodMember(pod, userId); }; -module.exports = { shouldEscalate, isGatedPodTarget, isRoutedPodTarget }; +module.exports = { + shouldEscalate, + isGatedPodTarget, + isRoutedPodTarget, + isListedPodMember, +}; diff --git a/backend/services/decisionCardReconcileService.ts b/backend/services/decisionCardReconcileService.ts index ad003a1f5..fd62f69c7 100644 --- a/backend/services/decisionCardReconcileService.ts +++ b/backend/services/decisionCardReconcileService.ts @@ -7,9 +7,8 @@ const DecisionRequest = require('../models/DecisionRequest'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const Pod = require('../models/Pod'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const isPodMember = require('../utils/isPodMember'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const { isGatedPodTarget } = require('./connectorRelayPolicy'); +const { isGatedPodTarget, isListedPodMember } = require('./connectorRelayPolicy'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const telegramSend = require('./telegramService'); const deliveryFailures = require('./connectorDeliveryFailureService'); @@ -92,7 +91,7 @@ const canSendClosingLine = ( const mutedUntil = integration.config.relayMutedUntil; if (mutedUntil && new Date(mutedUntil).getTime() > now.getTime()) return false; const linkedUserId = memberIdFor(integration); - if (!linkedUserId || !isPodMember(pod, linkedUserId)) return false; + if (!linkedUserId || !isListedPodMember(pod, linkedUserId)) return false; if (!integration.config.chatId) return false; if (integration.type === 'telegram') { return integration.config.chatType === 'private' && Boolean(process.env.TELEGRAM_BOT_TOKEN); diff --git a/backend/services/decisionCardReply.ts b/backend/services/decisionCardReply.ts index 6f5413b1c..acb30fd17 100644 --- a/backend/services/decisionCardReply.ts +++ b/backend/services/decisionCardReply.ts @@ -9,7 +9,7 @@ const Pod = require('../models/Pod'); // eslint-disable-next-line @typescript-eslint/no-require-imports const User = require('../models/User'); // eslint-disable-next-line @typescript-eslint/no-require-imports -const isPodMember = require('../utils/isPodMember'); +const { isListedPodMember } = require('./connectorRelayPolicy'); // eslint-disable-next-line @typescript-eslint/no-require-imports const verdicts = require('./channelVerdictService'); @@ -81,7 +81,7 @@ export const resolveDecisionCardReply = async (input: { + ` Open it in Commonly: ${link}`; // chooseDecision's already-ruled response precedes its membership guard. // Protect the late-reply writer and standing ruling from ex-members too. - if (!pod || !caller || caller.isBot || !isPodMember(pod, input.linkedUserId)) return answer(denied); + if (!pod || !caller || caller.isBot || !isListedPodMember(pod, input.linkedUserId)) return answer(denied); const close = async (): Promise => { try { diff --git a/backend/services/installable/eventHandlers.ts b/backend/services/installable/eventHandlers.ts index 2c4437a19..1b1991332 100644 --- a/backend/services/installable/eventHandlers.ts +++ b/backend/services/installable/eventHandlers.ts @@ -45,16 +45,20 @@ const activeHandlersForPod = async (podId: string, includeCardHolds = false): Pr // connector is private to its owner, so a stale gate can never relay after // that owner leaves even before the reconciler prunes the key. Legacy rows // retain their pod-scoped selection until they are explicitly migrated. + // + // `pod.members` ALONE: identity with the predicate the bridges apply + // (connectorRelayPolicy.isListedPodMember). This selector used to union + // `pod.createdBy` in, so the two agreed on admitting a departed creator — + // agreement on the wrong answer, which is why that cell is in the suite's + // matrix rather than left to this comment (TASK-161, from TASK-160's ruling). if (!Types.ObjectId.isValid(podId)) return []; - const pod = await Pod.findById(podId).select('createdBy members').lean() as { - createdBy?: unknown; + const pod = await Pod.findById(podId).select('members').lean() as { members?: unknown[]; } | null; if (!pod) return []; - const memberIds = Array.from(new Set([ - pod.createdBy, - ...(pod.members || []), - ].filter(Boolean).map((id) => String(id)))).map((id) => new Types.ObjectId(id)); + const memberIds = Array.from(new Set( + (pod.members || []).filter(Boolean).map((id) => String(id)), + )).map((id) => new Types.ObjectId(id)); if (!memberIds.length) return []; const gateEnabledPath = `config.gates.${String(podId)}.enabled`; // A decision card must record why it was deliberately withheld. Ordinary diff --git a/backend/services/installable/installableReconciler.ts b/backend/services/installable/installableReconciler.ts index c71c10f44..38dc11f60 100644 --- a/backend/services/installable/installableReconciler.ts +++ b/backend/services/installable/installableReconciler.ts @@ -25,7 +25,7 @@ const { allRefPaths, kindSpec, rowReferencesSecret } = require('../connectorSecr // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const connectorDeliveryFailures = require('../connectorDeliveryFailureService'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const isPodMember = require('../../utils/isPodMember'); +const { isListedPodMember } = require('../connectorRelayPolicy'); const ORPHAN_SECRET_GRACE_MS = 10 * 60_000; @@ -142,7 +142,11 @@ const sweepPausedInstallations = async (): Promise => { }; // Membership is authoritative on outbound selection, but prune obsolete keys -// here as well so the owner's gate list does not promise a pod they left. +// here as well so the owner's gate list does not promise a pod they left. That +// promise is exactly what a departed CREATOR breaks: `leavePod` filters `members` +// and keeps `createdBy`, so the permissive predicate kept this sweep blind to +// them, and after TASK-161 made relay strict the ON switch they were shown could +// never deliver. Same predicate as the relay, so the two cannot disagree here. const sweepOrphanedGates = async (): Promise => { const rows = await Integration.find({ scope: 'user', @@ -159,8 +163,8 @@ const sweepOrphanedGates = async (): Promise => { if (!gates || typeof gates !== 'object') continue; const unset: Record = {}; for (const podId of Object.keys(gates)) { - const pod = await Pod.findById(podId).select('createdBy members').lean(); - if (!pod || !isPodMember(pod, row.createdBy)) { + const pod = await Pod.findById(podId).select('members').lean(); + if (!pod || !isListedPodMember(pod, row.createdBy)) { unset[`config.gates.${podId}`] = 1; if (String(row.podId) === podId) unset.podId = 1; } diff --git a/backend/services/slackBridgeService.ts b/backend/services/slackBridgeService.ts index dbe107b09..e1b2a082f 100644 --- a/backend/services/slackBridgeService.ts +++ b/backend/services/slackBridgeService.ts @@ -5,12 +5,13 @@ const Integration = require('../models/Integration'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const Pod = require('../models/Pod'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const isPodMember = require('../utils/isPodMember'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const connectorSecrets = require('./connectorSecrets'); const deliveryFailures = require('./connectorDeliveryFailureService'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const { shouldEscalate, isGatedPodTarget, isRoutedPodTarget } = require('./connectorRelayPolicy'); +const { + shouldEscalate, isGatedPodTarget, isRoutedPodTarget, isListedPodMember, +} = require('./connectorRelayPolicy'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const channelVerdictService = require('./channelVerdictService'); import type { DecisionRelayCard } from './decisionCardRelay'; @@ -462,7 +463,7 @@ export const relaySlackMessageToPod = async (opts: { const socketConfig = require('../config/socket'); const pod = await PodModel.findById(podId).select('type createdBy members').lean(); - if (!pod || !isPodMember(pod, linkedUserId)) { + if (!pod || !isListedPodMember(pod, linkedUserId)) { console.warn('[slack-bridge] inbound dropped — linked user is no longer a pod member'); await replyNoActivePod(integration); return { relayed: false }; diff --git a/backend/services/telegramBridgeService.ts b/backend/services/telegramBridgeService.ts index 7c25f9de4..db728ef3a 100644 --- a/backend/services/telegramBridgeService.ts +++ b/backend/services/telegramBridgeService.ts @@ -21,12 +21,13 @@ // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const IntegrationModel = require('../models/Integration'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const isPodMember = require('../utils/isPodMember'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const telegramSend = require('./telegramService'); const deliveryFailures = require('./connectorDeliveryFailureService'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const { shouldEscalate, isGatedPodTarget, isRoutedPodTarget } = require('./connectorRelayPolicy'); +const { + shouldEscalate, isGatedPodTarget, isRoutedPodTarget, isListedPodMember, +} = require('./connectorRelayPolicy'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const channelVerdictService = require('./channelVerdictService'); import type { DecisionRelayCard } from './decisionCardRelay'; @@ -553,7 +554,7 @@ export const relayTelegramMessageToPod = async (opts: { const socketConfig = require('../config/socket'); const pod = await Pod.findById(podId).select('type createdBy members').lean(); - if (!pod || !isPodMember(pod, linkedUserId)) { + if (!pod || !isListedPodMember(pod, linkedUserId)) { console.warn('[tg-bridge] inbound dropped — linked user is no longer a pod member'); await replyNoActivePod(integration); return { relayed: false }; diff --git a/backend/utils/isPodMember.ts b/backend/utils/isPodMember.ts index 5e5b66c96..4ba4beab7 100644 --- a/backend/utils/isPodMember.ts +++ b/backend/utils/isPodMember.ts @@ -13,6 +13,26 @@ const isPodMember = (pod: any, userId: unknown): boolean => { )); }; +// The membership rule the pod's own write paths implement, and nothing else: +// `createMessage` (controllers/messageController.ts) and the socket post path +// (server.ts) both check `pod.members` alone. A connector that admits a writer +// those two refuse is more permissive than the pod it writes into, which is what +// TASK-161 measured: `createdBy` keeps the creator's id after `leavePod` filters +// `members`, so a departed creator was refused by the app and relayed anyway. +// +// Named for the mechanism, not for emphasis. The creator clause above answers +// "is this someone `Pod.members` forgot to list" — a false negative at creation +// time — and was never a ruling that leaving leaves membership intact. +const isListedPodMember = (pod: any, userId: unknown): boolean => { + if (!pod || !userId) return false; + const id = String(userId); + return (pod.members || []).some((m: any) => ( + (m?._id?.toString?.() || m?.toString?.() || '') === id + )); +}; + module.exports = isPodMember; +module.exports.isPodMember = isPodMember; +module.exports.isListedPodMember = isListedPodMember; export {};