diff --git a/backend/__tests__/unit/routes/slackOAuth.installables.test.js b/backend/__tests__/unit/routes/slackOAuth.installables.test.js index 8f82105ad9..5151c2d2d7 100644 --- a/backend/__tests__/unit/routes/slackOAuth.installables.test.js +++ b/backend/__tests__/unit/routes/slackOAuth.installables.test.js @@ -295,3 +295,69 @@ describe('Slack installable OAuth routes', () => { expect(connectorSecrets.revoke).toHaveBeenCalledWith('secret-ref'); }); }); + +// The suite above hands the routes plain objects, where an absent pendingBind +// is absent. Production hands them a Mongoose document, and `config.pendingBind` +// is a nested schema path: it hydrates as `{}` on a row that has no bind. Read +// by truthiness, that refused every new Slack install from #1537 on. These +// tests go through the real model so the route sees what `findOne` returns. +describe('Slack routes on a hydrated Integration document', () => { + const RealIntegration = jest.requireActual('../../../models/Integration'); + const hydrated = (config) => RealIntegration.hydrate({ + _id: '64b64c48c4f37a6b2f34c333', + installationId: 'install-1', + type: 'slack', + status: 'pending', + isActive: true, + config, + }); + const freshInstall = () => hydrated({ + connectCode: 'c'.repeat(32), + connectCodeExpiresAt: new Date(Date.now() + 60_000), + }); + + beforeEach(() => { + jest.clearAllMocks(); + InstallableInstallation.findOne.mockResolvedValue({ _id: 'install-1', targetId: ownerId, status: 'active' }); + }); + + test('the hydrated shape this guards: a row with no bind still carries a truthy pendingBind', () => { + // If a schema change ever stops this, the tests below stop proving anything. + expect(freshInstall().config.pendingBind).toBeTruthy(); + }); + + test('a fresh install with no bind gets an authorize URL', async () => { + const doc = freshInstall(); + Integration.findOne.mockResolvedValue(doc); + Integration.findOneAndUpdate.mockResolvedValue(doc); + + const response = await request(app).post('/api/installables/slack/authorize-url'); + + expect(response.status).toBe(200); + expect(response.body.authorizeUrl).toContain(`state=${'c'.repeat(32)}`); + }); + + test('a row that holds a bind is still refused as already authorized', async () => { + Integration.findOne.mockResolvedValue(hydrated({ + pendingBind: { + chatId: 'D1', botTokenRef: 'secret-ref', expiresAt: new Date(Date.now() + 60_000), + }, + })); + + const response = await request(app).post('/api/installables/slack/authorize-url'); + + expect(response.status).toBe(409); + expect(response.body.code).toBe('slack_already_authorized'); + }); + + test.each(['confirm', 'reject'])('%s with no bind is named as missing and writes nothing', async (verb) => { + Integration.findOne.mockResolvedValue(freshInstall()); + + const response = await request(app).post(`/api/installables/slack/${verb}`); + + expect(response.status).toBe(409); + expect(response.body.code).toBe('slack_bind_missing'); + expect(Integration.findOneAndUpdate).not.toHaveBeenCalled(); + expect(connectorSecrets.revoke).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/routes/installables.ts b/backend/routes/installables.ts index 16e689e433..98a93a6a51 100644 --- a/backend/routes/installables.ts +++ b/backend/routes/installables.ts @@ -106,6 +106,17 @@ const ownedSlackIntegration = async (userId: string): Promise<{ installation: an return integration ? { installation, integration } : null; }; +/** + * Whether the OAuth callback has stored a Slack bind on this row. + * + * `config.pendingBind` is a nested schema path, so a hydrated document carries + * it as `{}` even when the stored row has none: its truthiness says nothing. + * Judge by the secret reference the callback always writes into a bind. The + * `$exists` filters in the CAS updates below read the stored row and are right + * as they are. + */ +const hasPendingBind = (config: any): boolean => Boolean(config?.pendingBind?.botTokenRef); + const slackError = (res: Res, status: number, code: string, error: string): void => { res.status(status).json({ code, error }); }; @@ -166,7 +177,7 @@ router.post('/slack/authorize-url', writeIntegrationsRateLimit, auth, async (req if (!owned.integration.isActive) { return slackError(res, 409, 'slack_authorization_unavailable', 'Slack authorization is no longer available.'); } - if (owned.integration.config?.chatId || owned.integration.config?.pendingBind) { + if (owned.integration.config?.chatId || hasPendingBind(owned.integration.config)) { return slackError(res, 409, 'slack_already_authorized', 'Slack is already awaiting confirmation or connected.'); } // OAuth state expires after ten minutes. Re-mint it here, rather than @@ -372,7 +383,9 @@ router.post('/slack/confirm', writeIntegrationsRateLimit, auth, async (req: Auth const owned = await ownedSlackIntegration(userId); if (!owned) return slackError(res, 404, 'slack_installation_not_found', 'Slack installation not found.'); const pending = owned.integration.config?.pendingBind; - if (!pending) return slackError(res, 409, 'slack_bind_missing', 'There is no Slack authorization to confirm.'); + if (!hasPendingBind(owned.integration.config)) { + return slackError(res, 409, 'slack_bind_missing', 'There is no Slack authorization to confirm.'); + } if (new Date(pending.expiresAt) <= new Date()) { const cleared = await Integration.findOneAndUpdate( { _id: owned.integration._id, 'config.pendingBind.botTokenRef': pending.botTokenRef }, @@ -436,7 +449,9 @@ router.post('/slack/reject', writeIntegrationsRateLimit, auth, async (req: AuthR const owned = await ownedSlackIntegration(userId); if (!owned) return slackError(res, 404, 'slack_installation_not_found', 'Slack installation not found.'); const pending = owned.integration.config?.pendingBind; - if (!pending) return slackError(res, 409, 'slack_bind_missing', 'There is no Slack authorization to reject.'); + if (!hasPendingBind(owned.integration.config)) { + return slackError(res, 409, 'slack_bind_missing', 'There is no Slack authorization to reject.'); + } const rejected = await Integration.findOneAndUpdate( { _id: owned.integration._id, 'config.pendingBind.botTokenRef': pending.botTokenRef }, { $unset: { 'config.pendingBind': 1 } }, diff --git a/docs/design/evidence/slack-authorize-409/slack-authorize-409-1200.png b/docs/design/evidence/slack-authorize-409/slack-authorize-409-1200.png new file mode 100644 index 0000000000..ce2c2005da Binary files /dev/null and b/docs/design/evidence/slack-authorize-409/slack-authorize-409-1200.png differ diff --git a/docs/design/evidence/slack-authorize-409/slack-authorize-409-390.png b/docs/design/evidence/slack-authorize-409/slack-authorize-409-390.png new file mode 100644 index 0000000000..04156b21c4 Binary files /dev/null and b/docs/design/evidence/slack-authorize-409/slack-authorize-409-390.png differ