diff --git a/src/__tests__/slack-user/listChannelRows.test.ts b/src/__tests__/slack-user/listChannelRows.test.ts index 774e7e3..c0931f8 100644 --- a/src/__tests__/slack-user/listChannelRows.test.ts +++ b/src/__tests__/slack-user/listChannelRows.test.ts @@ -195,18 +195,26 @@ describe('renderListSummary', () => { // === diagnoseChannelAccess helpers === import { lookupChannelByName, explainDenial, describeChannel } from '../../slack-user/server.js'; -import { assertAccess, SlackAccessDenied } from '../../slack-user/accessControl.js'; +import { assertAccess, assertDmMemberAccess, SlackAccessDenied } from '../../slack-user/accessControl.js'; import type { ChannelMeta } from '../../slack-user/accessControl.js'; +/** `mine` may be a flat list (one page) or a list of member pages. */ function lookupClient(mine: any[], workspacePages: any[][] = []) { - let page = 0; + const memberPages: any[][] = Array.isArray(mine[0]) ? mine as any[][] : [mine]; + let mp = 0; + let wp = 0; return { - conversationsList: async () => ({ channels: mine }), + conversationsList: async () => { + const channels = memberPages[mp] ?? []; + const more = mp < memberPages.length - 1; + mp++; + return { channels, response_metadata: more ? { next_cursor: `m${mp}` } : {} }; + }, conversationsListAll: async () => { - const channels = workspacePages[page] ?? []; - const more = page < workspacePages.length - 1; - page++; - return { channels, response_metadata: more ? { next_cursor: `c${page}` } : {} }; + const channels = workspacePages[wp] ?? []; + const more = wp < workspacePages.length - 1; + wp++; + return { channels, response_metadata: more ? { next_cursor: `c${wp}` } : {} }; }, } as any; } @@ -239,6 +247,7 @@ describe('lookupChannelByName', () => { const res = await lookupChannelByName(client, 'missing'); assert.equal(res.kind, 'scanBounded'); assert.equal((res as any).pages, 10); + assert.equal((res as any).source, 'workspace'); }); it('reports a genuine absence when the scan reached the end', async () => { @@ -396,3 +405,132 @@ describe('renderChannelLines', () => { assert.equal(line, '#bare (C2)\n Type: public'); }); }); + +describe('lookupChannelByName pagination', () => { + it('follows the member-list cursor to find a DM on a later page', async () => { + // users.conversations is the ONLY source that returns im/mpim — the + // workspace fallback cannot see them — so not following its cursor made a + // DM past page one unfindable, not merely slower. + const client = lookupClient([ + [{ id: 'D1', name: 'someone-else', is_im: true }], + [{ id: 'D2', name: 'alice', is_im: true }], + ]); + assert.deepEqual(await lookupChannelByName(client, 'alice'), { kind: 'found', channelId: 'D2' }); + }); + + it('finds a group DM on a later member page', async () => { + const client = lookupClient([ + [{ id: 'G1', name: 'mpdm-x', is_mpim: true }], + [{ id: 'G2', name: 'mpdm-target', is_mpim: true }], + ]); + assert.deepEqual(await lookupChannelByName(client, 'mpdm-target'), { kind: 'found', channelId: 'G2' }); + }); + + it('reports a bounded member scan instead of falling through to a wrong answer', async () => { + const pages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const res = await lookupChannelByName(lookupClient(pages), 'missing'); + assert.equal(res.kind, 'scanBounded'); + assert.equal((res as any).pages, 10); + assert.equal((res as any).source, 'member'); + }); + + it('stops paginating members as soon as it matches', async () => { + let calls = 0; + const client: any = { + conversationsList: async () => { + calls++; + return { channels: [{ id: 'C1', name: 'found-it' }], response_metadata: { next_cursor: 'more' } }; + }, + conversationsListAll: async () => ({ channels: [], response_metadata: {} }), + }; + await lookupChannelByName(client, 'found-it'); + assert.equal(calls, 1); + }); + + it('still falls back to the workspace list for a channel you are not in', async () => { + const client = lookupClient([[]], [[{ id: 'C9', name: 'eng-general' }]]); + assert.deepEqual(await lookupChannelByName(client, 'eng-general'), { kind: 'found', channelId: 'C9' }); + }); +}); + +describe('explainDenial for member rules', () => { + const rulesWithOrg: SlackAccessRules = { + allowedOrgs: ['T_MINE'], blacklistUsers: ['U_BAD'], + whitelistChannels: ['*'], blacklistChannels: [], allowPublicOnly: false, + }; + + async function dmDenial(meta: any, client: any): Promise { + try { await assertDmMemberAccess(client, rulesWithOrg, meta, meta.id ?? 'D1'); } + catch (e) { return e as SlackAccessDenied; } + throw new Error('expected a denial'); + } + + it('names the DM counterpart\'s organisation and the fix', async () => { + const client: any = { usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }) }; + const d = await dmDenial({ is_im: true, user: 'U1' }, client); + assert.equal(d.reason, 'dm-org-not-allowed'); + assert.deepEqual(d.orgIds, ['T_OTHER']); + const out = explainDenial(d, rulesWithOrg, '', new Map([['T_OTHER', 'Acme Corp']])).join('\n'); + assert.match(out, /The other participant belongs to an organisation that is not allowed: Acme Corp \(T_OTHER\)/); + assert.match(out, /Access Rules → Organizations/); + }); + + it('names a group DM member\'s organisation', async () => { + const client: any = { + conversationsMembers: async () => ({ members: ['U1'] }), + usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }), + }; + const d = await dmDenial({ is_mpim: true, id: 'G1' }, client); + assert.equal(d.reason, 'group-dm-org'); + const out = explainDenial(d, rulesWithOrg, '').join('\n'); + assert.match(out, /A member of this group DM belongs to an organisation that is not allowed/); + }); + + it('points a blocked group-DM member at the blocked-users list', async () => { + const client: any = { + conversationsMembers: async () => ({ members: ['U_BAD'] }), + usersInfo: async () => ({ user: { id: 'U_BAD', team_id: 'T_MINE' } }), + }; + const d = await dmDenial({ is_mpim: true, id: 'G1' }, client); + assert.equal(d.reason, 'group-dm-blacklist'); + const out = explainDenial(d, rulesWithOrg, '').join('\n'); + assert.match(out, /blocked-users list/); + assert.match(out, /Access Rules → Blocked users/); + }); + + it('degrades to the raw ID when the org cannot be named', async () => { + const client: any = { usersInfo: async () => ({ user: { id: 'U1', team_id: 'T_OTHER' } }) }; + const d = await dmDenial({ is_im: true, user: 'U1' }, client); + assert.match(explainDenial(d, rulesWithOrg, '').join('\n'), /not allowed: T_OTHER/); + }); +}); + +describe('lookupChannelByName workspace fallback after a bounded member scan', () => { + it('still finds a workspace channel when the member scan ran out of pages', async () => { + // The regression: returning early on the member bound meant an account with + // enough member conversations could never reach the workspace list, so a + // public channel it is not a member of became undiscoverable. + const memberPages = Array.from({ length: 11 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const client = lookupClient(memberPages, [[{ id: 'C_TARGET', name: 'eng-general' }]]); + assert.deepEqual(await lookupChannelByName(client, 'eng-general'), { + kind: 'found', channelId: 'C_TARGET', + }); + }); + + it('reports both limits when neither scan finished', async () => { + const memberPages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const workspacePages = Array.from({ length: 12 }, (_, i) => [{ id: `C${i}`, name: `ch-${i}` }]); + const res = await lookupChannelByName(lookupClient(memberPages, workspacePages), 'missing'); + assert.equal(res.kind, 'scanBounded'); + assert.equal((res as any).source, 'both'); + }); + + it('reports only the member limit when the workspace list was exhausted', async () => { + const memberPages = Array.from({ length: 12 }, (_, i) => [{ id: `D${i}`, name: `dm-${i}`, is_im: true }]); + const res = await lookupChannelByName( + lookupClient(memberPages, [[{ id: 'C1', name: 'something-else' }]]), 'missing', + ); + assert.equal(res.kind, 'scanBounded'); + assert.equal((res as any).source, 'member'); + }); +}); diff --git a/src/slack-user/accessControl.ts b/src/slack-user/accessControl.ts index 551fb7f..0a93f92 100644 --- a/src/slack-user/accessControl.ts +++ b/src/slack-user/accessControl.ts @@ -22,6 +22,9 @@ import type { SlackAccessRules } from '../mcpConnectionStore.js'; export type SlackDenialReason = | 'org-not-allowed' | 'org-unverified' + | 'dm-org-not-allowed' + | 'group-dm-blacklist' + | 'group-dm-org' | 'whitelist-empty' | 'whitelist-miss' | 'blacklist-channel' @@ -242,7 +245,10 @@ export async function assertDmMemberAccess( if (meta.is_im && meta.user && rules.allowedOrgs.length > 0) { const { user } = await client.usersInfo(meta.user); if (user.team_id && !rules.allowedOrgs.includes(user.team_id)) { - throw new UserError('Access denied: this user belongs to an organisation not in your allowed list.'); + throw new SlackAccessDenied( + 'Access denied: this user belongs to an organisation not in your allowed list.', + { reason: 'dm-org-not-allowed', orgIds: [user.team_id] }, + ); } } @@ -250,13 +256,19 @@ export async function assertDmMemberAccess( if (meta.is_mpim && (rules.blacklistUsers.length > 0 || rules.allowedOrgs.length > 0)) { const { members } = await client.conversationsMembers(channelId); if (rules.blacklistUsers.length > 0 && members.some(uid => rules.blacklistUsers.includes(uid))) { - throw new UserError('Access denied: this group DM contains a blacklisted user.'); + throw new SlackAccessDenied( + 'Access denied: this group DM contains a blacklisted user.', + { reason: 'group-dm-blacklist' }, + ); } if (rules.allowedOrgs.length > 0) { for (const uid of members) { const { user } = await client.usersInfo(uid); if (user.team_id && !rules.allowedOrgs.includes(user.team_id)) { - throw new UserError('Access denied: this group DM contains a user from a non-allowed organisation.'); + throw new SlackAccessDenied( + 'Access denied: this group DM contains a user from a non-allowed organisation.', + { reason: 'group-dm-org', orgIds: [user.team_id] }, + ); } } } diff --git a/src/slack-user/server.ts b/src/slack-user/server.ts index 0ad95a7..ef57a16 100644 --- a/src/slack-user/server.ts +++ b/src/slack-user/server.ts @@ -188,6 +188,9 @@ const DENIAL_LABELS: Record = { 'blacklist-channel': 'blacklist', 'blacklist-user': 'blocked user', 'dm-rules': 'DM/group-DM rules (blocked user or organisation)', + 'dm-org-not-allowed': 'organisation of the other participant', + 'group-dm-blacklist': 'blocked user in the group DM', + 'group-dm-org': 'organisation of a group DM member', 'public-only': 'private (allowPublicOnly)', 'org-not-allowed': 'organisation', 'org-unverified': 'organisation unverified', @@ -951,7 +954,8 @@ const DIAGNOSE_MAX_PAGES = 10; export type ChannelLookup = | { kind: 'found'; channelId: string } | { kind: 'none' } - | { kind: 'scanBounded'; pages: number } + /** Which scan ran out of pages — the caller has to name the right one. */ + | { kind: 'scanBounded'; pages: number; source: 'member' | 'workspace' | 'both' } | { kind: 'ambiguous'; ids: string[] }; /** @@ -972,23 +976,50 @@ export async function lookupChannelByName( const matches = (channels: ListedChannel[]) => channels.filter(ch => ch.name?.toLowerCase() === wanted); - const mine = await client.conversationsList(undefined, 'public_channel,private_channel,mpim,im'); - let hits = matches(mine.channels as ListedChannel[]); - + // Pass 1: users.conversations, paginated. It is member-scoped, so it is both + // cheap and the *only* source that returns DMs and group DMs — the workspace + // fallback below cannot see them at all. Following its cursor therefore is + // not an optimisation: without it, a DM on page two is simply unfindable. + // No initialiser: the do-while below always assigns before anything reads it. + let hits: ListedChannel[]; let cursor: string | undefined; - let pages = 0; - while (hits.length === 0 && pages < DIAGNOSE_MAX_PAGES) { + let memberPages = 0; + do { + const page = await client.conversationsList(cursor, 'public_channel,private_channel,mpim,im'); + hits = matches(page.channels as ListedChannel[]); + cursor = page.response_metadata?.next_cursor || undefined; + memberPages++; + } while (hits.length === 0 && cursor && memberPages < DIAGNOSE_MAX_PAGES); + + // Hitting the member bound must NOT skip the workspace scan: a public channel + // the user is not in is only ever found there, and an account with enough + // member conversations would otherwise never reach it. Remember the + // uncertainty and carry on. + const memberScanBounded = hits.length === 0 && !!cursor; + + // Pass 2: the workspace-wide list, for a channel the user is not in. + let workspacePages = 0; + cursor = undefined; + while (hits.length === 0 && workspacePages < DIAGNOSE_MAX_PAGES) { const page = await client.conversationsListAll(cursor, 'public_channel,private_channel'); hits = matches(page.channels as ListedChannel[]); cursor = page.response_metadata?.next_cursor || undefined; - pages++; + workspacePages++; if (!cursor) break; } if (hits.length === 1) return { kind: 'found', channelId: hits[0].id }; if (hits.length > 1) return { kind: 'ambiguous', ids: hits.map(h => h.id) }; - // A bounded scan is a floor, not a verdict. - return cursor ? { kind: 'scanBounded', pages } : { kind: 'none' }; + + // A bounded scan is a floor, not a verdict — and the caller has to be told + // which list ran out, since the two mean different things to the user. + const workspaceScanBounded = !!cursor; + if (memberScanBounded && workspaceScanBounded) { + return { kind: 'scanBounded', pages: Math.max(memberPages, workspacePages), source: 'both' }; + } + if (workspaceScanBounded) return { kind: 'scanBounded', pages: workspacePages, source: 'workspace' }; + if (memberScanBounded) return { kind: 'scanBounded', pages: memberPages, source: 'member' }; + return { kind: 'none' }; } /** @@ -1015,6 +1046,25 @@ export function explainDenial( ); break; } + case 'dm-org-not-allowed': + case 'group-dm-org': { + const labels = (denial.orgIds ?? []).map(id => formatTeamLabel(id, orgNames)).join(', '); + const who = denial.reason === 'dm-org-not-allowed' + ? 'The other participant belongs to' + : 'A member of this group DM belongs to'; + out.push( + `${who} an organisation that is not allowed: ${labels || '(unresolved)'}`, + `Your allowed organisations: ${rules.allowedOrgs.length ? rules.allowedOrgs.join(', ') : '(none)'}`, + 'Fix: tick the organisation under Access Rules → Organizations in the dashboard, or remove the person from the conversation.', + ); + break; + } + case 'group-dm-blacklist': + out.push( + 'One of this group DM\'s members is on your blocked-users list.', + 'Fix: remove them under Access Rules → Blocked users, or leave the conversation.', + ); + break; case 'org-unverified': out.push( 'Slack returned no organisation for this shared channel, so it cannot be checked against your allowlist.', @@ -1092,11 +1142,17 @@ slackUserServer.addTool({ case 'ambiguous': return [`${found.ids.length} channels are named "${args.name}". Re-run with one of these IDs:`, ...found.ids.map(id => ` ${id}`)].join('\n'); - case 'scanBounded': + case 'scanBounded': { + const which = { + member: 'your own conversations', + workspace: 'the workspace channel list', + both: 'both your own conversations and the workspace channel list', + }[found.source]; return [ - `No channel named "${args.name}" found in the first ${found.pages} page(s) of the workspace channel list.`, + `No channel named "${args.name}" found in the first ${found.pages} page(s) of ${which}.`, 'The scan stopped at its page limit, so the channel may still exist further in. Pass channelId to check it directly.', ].join('\n'); + } default: return `No channel named "${args.name}" is visible to your Slack account. Slack itself does not return it, so this is not an access-rules problem — you are most likely not a member of it.`; } @@ -1111,7 +1167,10 @@ slackUserServer.addTool({ await assertDmMemberAccess(client, rules, meta, channelId!); } catch (err) { if (err instanceof SlackAccessDenied) denial = err; - else if (err instanceof UserError) return [header, '', `Denied by: ${err.message}`].join('\n'); + // Every access rule now throws SlackAccessDenied, so this branch is only + // for a UserError that is not a denial at all — a missing rules record, + // say. Report it rather than dressing it up as a rule verdict. + else if (err instanceof UserError) return [header, '', `Could not evaluate: ${err.message}`].join('\n'); else throw err; } @@ -1119,8 +1178,8 @@ slackUserServer.addTool({ return [header, '', 'Readable: yes. Every access rule passes for this channel.'].join('\n'); } - const orgNames = denial.reason === 'org-not-allowed' - ? (await resolveTeamNames(client, denial.orgIds ?? [], { tokenKey })).names + const orgNames = (denial.orgIds?.length ?? 0) > 0 + ? (await resolveTeamNames(client, denial.orgIds!, { tokenKey })).names : undefined; return [header, '', ...explainDenial(denial, rules, meta.name, orgNames)].join('\n'); },