Skip to content

Commit 4eac615

Browse files
authored
Merge pull request #299 from plotday/fix/slack-dm-scope-reauth
fix(slack): prompt reconnect when a DM grant predates im:read
2 parents b95d6ec + 4b59db6 commit 4eac615

2 files changed

Lines changed: 65 additions & 3 deletions

File tree

‎connectors/slack/src/slack.test.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,50 @@ describe("listDMChannels", () => {
494494
expect(markNeedsReauth).not.toHaveBeenCalled();
495495
expect(store.map.has("dm_channels")).toBe(false);
496496
});
497+
498+
it("flags re-auth when the token has im:history but not im:read (opted into DMs on a pre-im:read grant)", async () => {
499+
// A token granted before im:read/mpim:read joined the `dms` scope group
500+
// carries im:history but not im:read. That user OPTED IN to DM sync, so
501+
// silently no-oping strands them: the DM webhook registers (it only needs
502+
// im:history), but listDMChannels never populates `dm_channels`, so
503+
// isKnownDMChannel rejects every incoming DM event forever — while the
504+
// connection still reports itself healthy. Both scopes come from the same
505+
// group, so im:history-without-im:read can only mean a stale grant, never
506+
// a decline; prompt a reconnect to re-consent.
507+
const store = makeStore({});
508+
const integrationsGet = vi.fn().mockResolvedValue({
509+
token: "xoxp-test",
510+
scopes: ["channels:history", "im:history", "im:write", "mpim:history"], // pre-fix grant
511+
});
512+
const markNeedsReauth = vi.fn().mockResolvedValue(undefined);
513+
const create = vi.fn(async () => ({ token: "cb" }) as never);
514+
const tools = {
515+
store,
516+
integrations: { get: integrationsGet, markNeedsReauth },
517+
network: { createWebhook: vi.fn() },
518+
files: {},
519+
callbacks: { create },
520+
tasks: { runTask: vi.fn(async () => "task-token"), scheduleRecurring: vi.fn(async () => {}) },
521+
};
522+
const slack = new Slack("twist-instance-1" as never, { getTools: () => tools } as never);
523+
524+
const fetchMock = vi.fn();
525+
vi.stubGlobal("fetch", fetchMock);
526+
try {
527+
await (slack as unknown as {
528+
listDMChannels: (c: string) => Promise<void>;
529+
}).listDMChannels("C1");
530+
} finally {
531+
vi.unstubAllGlobals();
532+
}
533+
534+
// Still no API call: the grant genuinely lacks the enumeration scope, so
535+
// conversations.list would only return missing_scope.
536+
expect(fetchMock).not.toHaveBeenCalled();
537+
// ...but unlike a decline, surface it so the user can reconnect.
538+
expect(markNeedsReauth).toHaveBeenCalledWith("C1");
539+
expect(store.map.has("dm_channels")).toBe(false);
540+
});
497541
});
498542

499543
describe("extractSlackMessageReactions (custom emoji)", () => {

‎connectors/slack/src/slack.ts‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1683,11 +1683,16 @@ export class Slack extends Connector<Slack> {
16831683
* immediately if proactively registered (see `createDirectMessage`).
16841684
*
16851685
* Requires the `im:read` optional scope (part of the `dms` group) to have
1686-
* been granted — no-ops otherwise (the user declined DM sync at connect
1687-
* time). Without this guard, a token missing `im:read` would hit
1686+
* been granted. Without that guard, a token missing `im:read` would hit
16881687
* `conversations.list`, get back `missing_scope` (a permanent error in
16891688
* `SLACK_AUTH_ERRORS`), and force-flag the entire connection as needing
16901689
* reconnection even though nothing is actually broken.
1690+
*
1691+
* When `im:read` is absent, `im:history` disambiguates why: both come from
1692+
* the `dms` group, so having `im:history` alone means the user opted into DM
1693+
* sync on a grant made before `im:read` joined that group. That grant can
1694+
* never enumerate DMs, so re-auth is flagged to prompt a reconnect. A user
1695+
* who declined DM sync has neither scope and is left alone.
16911696
*/
16921697
async listDMChannels(channelId: string): Promise<void> {
16931698
const now = Date.now();
@@ -1700,7 +1705,20 @@ export class Slack extends Connector<Slack> {
17001705
console.warn("listDMChannels: Slack token unavailable");
17011706
return;
17021707
}
1703-
if (!token.scopes?.includes("im:read")) return; // optional scope declined
1708+
if (!token.scopes?.includes("im:read")) {
1709+
// im:read and im:history are granted together by the `dms` group, so a
1710+
// token carrying im:history WITHOUT im:read predates im:read joining
1711+
// that group — the user opted into DM sync but their grant can't
1712+
// enumerate DMs. Returning quietly would strand them permanently: the
1713+
// DM webhook registers (im:history is enough), but `dm_channels` stays
1714+
// empty, so isKnownDMChannel drops every incoming DM while the
1715+
// connection still reports healthy. Prompt a reconnect to re-consent.
1716+
// A user who declined DMs has neither scope and is left alone.
1717+
if (token.scopes?.includes("im:history")) {
1718+
await this.tools.integrations.markNeedsReauth(channelId);
1719+
}
1720+
return;
1721+
}
17041722

17051723
const api = new SlackApi(token.token);
17061724

0 commit comments

Comments
 (0)