fix(slack): preserve users.info failures instead of laundering them - #629
Open
yzxcj797 wants to merge 2 commits into
Open
fix(slack): preserve users.info failures instead of laundering them#629yzxcj797 wants to merge 2 commits into
yzxcj797 wants to merge 2 commits into
Conversation
In email identity mode (the default posture of a standard deployment — the CLI scaffold hardcodes SLACK_IDENTITY_EMAIL=1 for Fly and AWS), an own-team member whose email the directory can't currently see was classified isExternalGuest — an org owner with a just-set email masqueraded as an external guest for the ~39-minute Slack propagation window, and the refusal copy, logs, and audit all said "external" when the truth was "own-team, principal unresolved" (yc-software#626). classifyUser now keeps the third state distinct: email-less members with no Slack-side external evidence carry principalUnresolved (native flags still decide isExternalGuest, and email keying is unchanged — a restricted member with a visible email remains an email-keyed guest). The turn-handler gate refuses principalUnresolved members ahead of the external-guest gate — equally fail-closed, never a fallback Slack-ID principal — but with its own user-facing copy (names the directory refresh, tells the sender to retry) and its own log line. The external-guest refusal path now logs on success too: the incident in yc-software#626 was undiagnosable precisely because a successful refusal left no trace, leaving H1/H2/H3 indistinguishable from the record. Not included: preserving users.info ok:false at callers (defect 1b) and user_change/team_join event-driven refresh (defect 3) — separate follow-ups per the issue's fix ladder. Fixes defects 1 and 2 of yc-software#626
A transient users.info failure returned
{ actor: { isExternalGuest: true }, ok: false } — the failure flag
existed, but classifyActor stripped it (returning only .actor) and the
direct-intake path read classifyUserCached and ignored .ok too, so a
single failed lookup read to every consumer as a confident "this user
is an external guest" and refused the member outright. In the yc-software#626
incident an org owner was refused on exactly that shape.
- classifyUserCached's failure arm now returns a lookupFailed actor
(the id-based placeholder, named for what it is) instead of
asserting external guest — the failure state is distinguishable from
every checked state (guest, internal, principal-unresolved).
- classifyActor returns ActorAssertion & { ok } — the flag rides along
instead of being discarded; consumers decide what a failed lookup
means. The roster path already consumed ok (resolveChannelMembership
sets complete=false); the two remaining discard sites no longer can.
- The direct-intake path (turn-handler) acts on it: fall back to the
directory's last-known classification when the cache holds one
(time-bounded by the cache TTL — failures are never cached, so the
fallback is always a prior success), log the fallback, and with no
prior state refuse as principalUnresolved — the third-state path
from the stacked commit — never a laundered "external".
Stacked on the third-state commit (principalUnresolved + refusal
gates); together they cover yc-software#626's defect 1 including its 1b
sub-defect. The event-driven refresh (defect 3) remains a follow-up.
Fixes the ok-propagation half of yc-software#626 (defect 1b)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
ok:false-propagation half of #626 (defect 1b). Stacked on #628 (the third-state commit —principalUnresolvedand the refusal gates come from there); merge that first.The laundering
A transient
users.infofailure returned{ actor: { isExternalGuest: true }, ok: false }— the flag existed, but both consumers discarded it:classifyActorreturned only.actor, so the assertion arrived as a confident "external guest" with no trace of the failure;turn-handler) readclassifyUserCached(...)and ignored.okidentically.A single failed lookup therefore refused a member outright — in the incident, an org owner, with no signal that the classification was a placeholder rather than a verdict.
The fix
classifyUserCached's catch returns alookupFailedactor (id-based placeholder, named for what it is). Distinguishable from every checked state — guest, internal, and (from feat(slack): refuse unresolved own-team members as their own state #628) principal-unresolved.classifyActorreturnsActorAssertion & { ok }— the flag rides along; consumers decide what a failed lookup means. The roster path already consumedok(resolveChannelMembershipsetscomplete=false); the two discard sites no longer can.lastKnownClassification(userId)— the LRU entry, which failures never populate, so the fallback is always a prior success, time-bounded by the cache TTL — logging the fallback; with no prior state, refuse asprincipalUnresolved(the third-state gate: fail closed, visibly, never a laundered "external").Tests
a failed users.info asserts lookupFailed, not external guest— drives a failing client throughcreateDirectory:ok:false,lookupFailed:true,isExternalGuestunset. Fails on the base (guest).classifyActor preserves ok instead of discarding it— the interface contract with a working client:ok:truerides on the returned assertion. Fails on the base (nookon the return).slack-identity29/29 on this stack; the four identity suites 80/80 both with and without the patch's base state.tsc --noEmitclean.Remaining from #626
Defect 3 (event-driven refresh:
user_change/team_joinsubscriptions + cache eviction) — needs the manifest change and the app-reinstall note the issue describes.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.