fix(slack): judge a Slack bind by its secret reference, not an object that is always there

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();
+  });
+});

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 } },