refactor(auth): AccessControl — required boundAuth + options-object constructor, drop dead branch#4181
Open
viktormarinho wants to merge 3 commits into
Open
Conversation
…th branch `boundAuth` is built by the context factory for EVERY request (even anonymous), so it is never absent at runtime. Type it as a required constructor dependency and delete the code that only ran when it was missing: - `check()`'s `!userId && !boundAuth` branch (the UnauthorizedError 401 + public- tool fallback) was dead in prod — an anonymous caller already gets 403 via the permission check, and public tools are gated by the MCP transport's own `isPublicTool` and the `MESH_PUBLIC_` prefix. Removed it, along with the now-unused `isToolPublic`, the `getToolMeta` constructor param, the `GetToolMetaFn` type, and the `MCP_MESH_KEY` import. - Dropped the dead `if (!boundAuth) return false` guards in `checkResource` / `checkApiKeyAccess` / `checkMemberAccess`. - `userId`/`toolName` become required-but-nullable params (may be undefined, but are always passed positionally). No behavior change: the removed branch never executed in production. Tests that constructed AccessControl without a boundAuth now pass a mock (grant/granted), or were deleted (the "no BoundAuthClient" case, which can no longer happen). Note: AccessControl no longer throws UnauthorizedError (nothing in the repo does now). The type and its 401 mappings in tools-rest/org-fs are retained pending a decision on anonymous-caller semantics (401 vs the current 403). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tructor Positional args with three leading optionals (passing `undefined` to reach `boundAuth`) were a footgun. Replace the 6-positional constructor with one `AccessControlOptions` object: `boundAuth` required, everything else named and optional, `connectionId` defaulting to "self". TypeScript flagged every call site; updated all of them (context-factory, mcp transport, automation-context, and the unit tests). No behavior change — pure call-shape refactor on top of the boundAuth-required cleanup in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
viktormarinho
changed the title
`createBoundAuthClient` is the sole real producer and always sets it, and `AccessControl.checkResource` reads it on every call. Typing it optional let an impossible "bound client without a principal kind" state exist on paper; make it required so the invariant lives in the type (the flag is read as a real boolean, no implicit undefined→false coercion). Test mocks cast `as unknown as BoundAuthClient`, so they're unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Follow-up to #4180 (the
isApiKeyPrincipalflag there raised: why isboundAuthoptional / why are these args positional at all?).Two related cleanups to
AccessControl, both stemming from "boundAuthis always present at runtime":1.
boundAuthis now required + dead branch removedboundAuthis built by the context factory for every request (even anonymous — no role/perms, then denied by the permission check). So it's never absent, yet it was typed optional, leaving dead guards and an unreachable branch.!userId && !boundAuthbranch incheck()— theUnauthorizedError401 + public-tool fallback. It only ran whenboundAuthwas missing (impossible). Anonymous callers already get 403 via the permission path; public tools are gated by the MCP transport's ownisPublicTool(auth.ts) and theMESH_PUBLIC_prefix. With it went the now-unusedisToolPublic, thegetToolMetaparam, theGetToolMetaFntype, and theMCP_MESH_KEYimport.if (!boundAuth) return falseguards incheckResource/checkApiKeyAccess/checkMemberAccess.2. Single options-object constructor
The 6-positional constructor required three leading optionals and passing
undefinedto reachboundAuth— a footgun. Replaced with oneAccessControlOptionsobject:boundAuthrequired, everything else named/optional,connectionIddefaulting to"self". TypeScript flagged every call site; all updated (context-factory, MCP transport,automation-context, unit tests).No behavior change — the removed branch never executed in prod; the rest is a pure call-shape refactor. Tests that constructed
AccessControlwithout aboundAuthnow pass a mock (grant/granted), or were deleted (the "no BoundAuthClient" case, which can no longer happen).Heads-up / follow-up decision
Removing the dead branch removed the last
throw new UnauthorizedErrorin the repo, so the type is now never thrown (itsinstanceofcatches intools-rest.ts/org-fs.tsare dead). Left as-is. Open question: dropUnauthorizedError+ those catches, or restore 401 semantics for unauthenticated callers (anon → 401 vs today's 403)?tsc+oxlintclean; auth+core unit tests green (except the pre-existing DB-gatedcontext-factory.integration.test.ts).🤖 Generated with Claude Code