diff --git a/src/slack/directory.ts b/src/slack/directory.ts index baa2624c..4f97590b 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -65,7 +65,16 @@ export interface Directory { getUserSnapshot(client: any): Promise<{ byId: Map; fetchedAt: number } | undefined>; forceDirectorySync(client: any, invalidateChannelId?: string, invalidatePrincipalId?: string): Promise; classifyUserCached(client: any, userId: string): Promise; - classifyActor(client: any, userId: string): Promise; + lastKnownClassification(userId: string): ActorAssertion | undefined; + /** Apply a `user_change`/`team_join` event payload to the caches (#626): + * re-classify from the fresh profile and overwrite BOTH the LRU entry + * and the snapshot's row, so a profile/email change propagates + * immediately instead of waiting out the TTL. */ + updateUserFromEvent(user: SlackUser): void; + classifyActor( + client: any, + userId: string, + ): Promise; getChannelInfo(client: any, channel: string): Promise; channelMembership( client: any, @@ -588,12 +597,44 @@ export function createDirectory(deps: { if (ids.ownTeamId) userCache.set(userId, classified); return { ...classified, ok: true }; } catch { - return { actor: { externalId: userId, isExternalGuest: true }, ok: false }; + // Lookup failed — say so instead of asserting the user is an external + // guest (#626): the id-based actor carries lookupFailed so consumers + // can distinguish "couldn't check" from every checked state. + return { actor: { externalId: userId, lookupFailed: true }, ok: false }; + } + } + + function updateUserFromEvent(user: SlackUser): void { + // The event carries the FULL fresh profile — classify from it directly + // (no users.info round-trip) and overwrite every copy the caches hold. + // The LRU entry is the transient-lookup cache; the snapshot row feeds + // roster/membership resolution. Both must move together or a name/email + // change shows up in one surface and not the other until the TTLs lapse + // (#626 defect 3). + const actor = classifyUser(user, ids.ownTeamId, ids.identityMode); + const timezone = slackUserTimezone(user); + const classified = { actor, ...(timezone ? { timezone } : {}) }; + if (user.id) { + userCache.set(user.id, classified); + if (userSnapshot) userSnapshot.byId.set(user.id, classified); } } - async function classifyActor(client: any, userId: string): Promise { - return (await classifyUserCached(client, userId)).actor; + function lastKnownClassification(userId: string): ActorAssertion | undefined { + // The LRU entry is the last SUCCESSFUL classification (failures are + // never cached), i.e. exactly the fallback a transient users.info + // failure should fall back to (#626). Time-bounded by the cache TTL. + return userCache.get(userId)?.actor; + } + + async function classifyActor(client: any, userId: string): Promise { + // ok preserved, not discarded: a failed users.info currently reads as a + // confident "external guest" to every caller — transiently refusing a + // member whose record is merely unreachable. Callers decide what a failed + // lookup means (skip, retry, fall back to last-known); the classification + // itself must not launder the failure into an identity claim (#626). + const { actor, ok } = await classifyUserCached(client, userId); + return { ...actor, ok }; } async function getChannelInfo(client: any, channel: string): Promise { @@ -658,6 +699,8 @@ export function createDirectory(deps: { forceDirectorySync, classifyUserCached, classifyActor, + lastKnownClassification, + updateUserFromEvent, getChannelInfo, channelMembership, allInternalRosters, diff --git a/src/slack/events.ts b/src/slack/events.ts index 7f5a4d69..48797a45 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -1,3 +1,4 @@ +import type { SlackUser } from "./identity.ts"; import { type SlackFile, channelPrivacyChange, @@ -200,6 +201,26 @@ export function registerSlackEvents( } }); + // Profile/membership changes must propagate to the identity caches + // immediately: classification updates previously waited out the 5-min + // snapshot/lookup TTLs, so a just-set email kept an own-team member + // refused (the #626 incident's ~39-min window) and a profile change + // showed stale names until the TTL lapsed. The event payload carries the + // full fresh profile, so the handler re-classifies from it directly. + app.event("user_change", async ({ body }: any) => { + const user = (body as { event?: { user?: unknown } })?.event?.user as + | undefined + | (SlackUser & { id?: string }); + if (user?.id) directory.updateUserFromEvent(user); + }); + + app.event("team_join", async ({ body }: any) => { + const user = (body as { event?: { user?: unknown } })?.event?.user as + | undefined + | (SlackUser & { id?: string }); + if (user?.id) directory.updateUserFromEvent(user); + }); + app.event("member_joined_channel", async ({ event, body, client }: any) => { const e = event as { user?: string; channel?: string; event_ts?: string }; if ( diff --git a/src/slack/identity.ts b/src/slack/identity.ts index f03aaf8d..1427e385 100644 --- a/src/slack/identity.ts +++ b/src/slack/identity.ts @@ -5,6 +5,21 @@ export interface ActorAssertion { isExternalGuest?: boolean; isBot?: boolean; displayName?: string; + /** + * Own-team member whose principal could not be resolved in email identity + * mode (email not currently visible in the directory). Refused like a + * guest — fail closed, never a fallback Slack-ID principal — but as its + * OWN state, so logs and the user-facing refusal say "unresolved member" + * instead of masquerading as "external guest" (#626). + */ + principalUnresolved?: true; + /** + * The users.info lookup itself failed — the classification below is a + * placeholder, not an identity claim. Distinct from every checked state + * (guest, internal, principal-unresolved) so callers can retry or fall + * back to last-known rather than acting on "external" (#626). + */ + lookupFailed?: true; } export interface SlackUser { @@ -53,14 +68,22 @@ export function classifyUser( ); const displayName = user.profile?.display_name || user.real_name || user.name || user.id || ""; let externalId = String(user.id ?? ""); + // Email mode keys members on their email — including guests, whose + // principal still resolves for refusal copy and audit. An email-LESS + // member with no Slack-side external evidence is own-team but + // principal-unresolved: kept distinct from an external guest (which the + // native flags above decide) so refusal copy and logs name what actually + // happened instead of masquerading as external (#626). + let principalUnresolved = false; if (identity === "email" && !user.is_bot) { const email = (user.profile?.email ?? "").trim().toLowerCase(); if (email.includes("@")) externalId = email; - else isGuest = true; + else if (!isGuest) principalUnresolved = true; } return { externalId, isExternalGuest: isGuest, + ...(principalUnresolved ? { principalUnresolved: true as const } : {}), ...(user.is_bot ? { isBot: true } : {}), ...(displayName ? { displayName } : {}), }; diff --git a/src/slack/manifest.json b/src/slack/manifest.json index 4434a964..a9fa0c04 100644 --- a/src/slack/manifest.json +++ b/src/slack/manifest.json @@ -55,6 +55,8 @@ "message.mpim", "member_joined_channel", "member_left_channel", + "user_change", + "team_join", "channel_created", "channel_rename", "channel_unarchive", diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 199d7b54..544556ab 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -199,6 +199,27 @@ export function createTurnHandler(deps: { ...(inc.prefetched.timezone ? { timezone: inc.prefetched.timezone } : {}), }; else classified = await classifyUserCached(client, inc.userId); + // A failed users.info must not read as a confident external-guest + // refusal: log it so the transient failure is visible, and fall back to + // the LAST-KNOWN classification when one exists (the incident in #626 + // showed a single failed lookup refusing an owner outright). With no + // prior state, fail closed but as "unverifiable", the third-state + // refusal path — never a laundered "external". + if ("ok" in classified && !classified.ok) { + const cachedAgain = await directory.lastKnownClassification?.(inc.userId); + if (cachedAgain) { + console.warn( + `[slack-plugin] users.info failed for ${inc.userId}; using last-known classification`, + ); + classified = { actor: cachedAgain }; + } else { + console.warn( + `[slack-plugin] users.info failed for ${inc.userId} with no prior classification; ` + + `refusing as unverifiable ch=${inc.channel} ts=${inc.ts}`, + ); + classified = { actor: { ...classified.actor, principalUnresolved: true as const } }; + } + } const actor = classified.actor; const timezone = classified.timezone; const text = stripMention(inc.rawText, ids.botUserId); @@ -296,7 +317,33 @@ export function createTurnHandler(deps: { ...(ids.botHandle ? { botHandle: ids.botHandle } : {}), }; + // A principal-unresolved own-team member (email mode, email not yet + // visible) is refused as its OWN state — fail closed like a guest, but + // named for what it is, both to the sender and in the log, so the + // incident signature is decidable (#626: the refusal used to be + // indistinguishable from external-guest in logs, and its success path + // logged nothing at all). + const unresolvedMember = audience.find((a) => a.principalUnresolved); + if (unresolvedMember) { + console.warn( + `[slack-plugin] refusing turn: own-team member principal unresolved ` + + `(email identity mode; email not visible in the directory) ` + + `user=${unresolvedMember.externalId || "?"} ch=${inc.channel} ts=${inc.ts}`, + ); + if (!inc.unprompted) { + await ephemeralOrSay( + "I can't respond here yet — I couldn't verify your team membership " + + "(your email isn't visible to me). This usually resolves within a " + + "few minutes of the directory refreshing; try again shortly.", + ); + } + return; + } + if (audience.some((a) => a.isExternalGuest) && !(await externalParticipantsEnabled())) { + console.warn( + `[slack-plugin] refusing turn: external guest ch=${inc.channel} ts=${inc.ts}`, + ); if (!inc.unprompted) { await ephemeralOrSay( "I can't respond here — this conversation isn't fully internal. Try a DM or a fully-internal channel.", diff --git a/test/slack-identity.test.ts b/test/slack-identity.test.ts index c260e770..1b5b05dd 100644 --- a/test/slack-identity.test.ts +++ b/test/slack-identity.test.ts @@ -63,7 +63,26 @@ test("classifyUser email mode keys members on their normalized work email", () = test("classifyUser email mode fails closed to guest when a member has no visible email", () => { const a = classifyUser({ id: "U1", team_id: TEAM }, TEAM, "email"); assert.equal(a.externalId, "U1"); - assert.equal(a.isExternalGuest, true); + // Own-team member, principal unresolved: its OWN state, not external + // guest (#626). The refusal is equally closed, but logs and copy say + // what actually happened. + assert.equal(a.isExternalGuest, false); + assert.equal(a.principalUnresolved, true); +}); + +test("classifyUser email mode: native external evidence still wins over unresolved", () => { + // A restricted member with no visible email is an EXTERNAL GUEST, not an + // unresolved own-team member — the third state never masks the flags + // (#626). + const g = classifyUser({ id: "U2", team_id: TEAM, is_restricted: true }, TEAM, "email"); + assert.equal(g.isExternalGuest, true); + assert.equal(g.principalUnresolved, undefined); +}); + +test("classifyUser slack-id mode never marks principalUnresolved", () => { + const a = classifyUser({ id: "U1", team_id: TEAM }, TEAM, "slack-id"); + assert.equal(a.isExternalGuest, false); + assert.equal(a.principalUnresolved, undefined); }); test("classifyUser email mode keeps bots on their Slack id and non-guest", () => { @@ -83,6 +102,82 @@ test("classifyUser email mode still flags restricted/other-workspace members as assert.equal(g.isExternalGuest, true); }); +test("a failed users.info asserts lookupFailed, not external guest (#626)", async () => { + // The directory's classifyUserCached currently launders a transient + // users.info failure into a confident "external guest" — the incident + // refused an owner outright on one failed lookup. The failure state must + // be distinguishable from every checked state. + const { createDirectory } = await import("../src/slack/directory.ts"); + const failingClient = { + users: { + info: async () => { throw new Error("transient"); }, + list: async () => ({ members: [] }), + }, + conversations: { list: async () => ({ channels: [] }) }, + auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1" }) }, + }; + const dir = createDirectory({ + core: failingClient as never, + ids: { botUserId: "UBOT", ownTeamId: "T1", identityMode: "slack-id" } as never, + }); + const result = await dir.classifyUserCached(failingClient, "U1"); + assert.equal(result.ok, false); + assert.equal(result.actor.lookupFailed, true, "the failure is named, not guest-folded"); + assert.equal(result.actor.isExternalGuest, undefined); +}); + +test("updateUserFromEvent propagates a fresh profile into both caches (#626)", async () => { + const { createDirectory } = await import("../src/slack/directory.ts"); + // Email mode: the member starts email-less (principal-unresolved — the + // #626 incident shape) and a user_change carries the just-set email. + const client = { + users: { + info: async () => ({ user: { id: "U1", team_id: "T1" } }), + list: async () => ({ members: [] }), + }, + conversations: { list: async () => ({ channels: [] }) }, + auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1", bot_id: "BBOT" }) }, + }; + const dir = createDirectory({ + core: client as never, + ids: { botUserId: "UBOT", ownTeamId: "T1", bot_id: "BBOT", identityMode: "email" } as never, + }); + + const before = await dir.classifyUserCached(client, "U1"); + assert.equal(before.actor.principalUnresolved, true, "fixture starts unresolved"); + + dir.updateUserFromEvent({ id: "U1", team_id: "T1", profile: { email: "U1@acme.com" } }); + + const after = await dir.classifyUserCached(client, "U1"); + assert.equal(after.actor.principalUnresolved, undefined, "the fresh profile resolves immediately"); + assert.equal(after.actor.externalId, "u1@acme.com", "keyed on the fresh email"); + // A users.info failure now falls back to the UPDATED classification, + // not the stale unresolved one. + const lastKnown = dir.lastKnownClassification("U1"); + assert.equal(lastKnown?.externalId, "u1@acme.com"); +}); + +test("classifyActor preserves ok instead of discarding it (#626)", async () => { + // Shape check without a live client: the interface change is the contract + // (ok rides on the assertion); the directory unit above covers behavior. + const { createDirectory } = await import("../src/slack/directory.ts"); + const okClient = { + users: { + info: async () => ({ user: { id: "U1", team_id: "T1" } }), + list: async () => ({ members: [] }), + }, + conversations: { list: async () => ({ channels: [] }) }, + auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1" }) }, + }; + const dir = createDirectory({ + core: okClient as never, + ids: { botUserId: "UBOT", ownTeamId: "T1", identityMode: "slack-id" } as never, + }); + const actor = await dir.classifyActor(okClient, "U1"); + assert.equal(actor.ok, true, "ok rides on the returned assertion"); + assert.equal(actor.isExternalGuest, false); +}); + test("slackUserTimezone extracts only valid Slack user timezones", () => { assert.equal(slackUserTimezone({ id: "U1", tz: "America/Los_Angeles" }), "America/Los_Angeles"); assert.equal(slackUserTimezone({ id: "U1", profile: { tz: "Europe/London" } }), "Europe/London");