@W-23757363: clarify Tableau auth/permission errors across all auth modes - #894
@W-23757363: clarify Tableau auth/permission errors across all auth modes#894Alon-ST-DATA wants to merge 8 commits into
Conversation
A bad or expired credential surfaced as a bare "Request failed with status code 401" (or crashed the server at startup while fetching site settings), which models paraphrased into a misleading "feature not configured" message — indistinguishable from Admin Insights genuinely not being provisioned. Classify raw thrown 401/403 REST errors in the shared web-tool error path into clear, self-explanatory guidance naming the targeted site and pod. This lives in WebTool.logAndExecute, so it covers every auth mode (PAT, OAuth, direct-trust, UAT, passthrough). Typed McpToolErrors (e.g. AdminOnlyError) bypass reclassification and keep their curated text. - src/utils/authErrorMessage.ts (new): single source for 401/403 wording - src/tools/web/tool.ts: reclassify raw thrown 401/403 in the catch - src/utils/mcpSiteSettings.ts: a startup 401 no longer crashes the server — it logs guidance and continues with defaults (matches OAuth behavior) - src/server/oauth/authMiddleware.ts: enrich the OAuth 401 challenge error_description with the same guidance (WWW-Authenticate unchanged) - src/tools/web/adminInsights/resolver.ts: de-conflate the not-provisioned 404 message from the site-admin-role hint - docs: new multiple-servers.md decoder table + auth/oauth/admin-insights/intro - version 4.8.2 -> 4.8.3 (patch) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Akash-Rastogi
left a comment
There was a problem hiding this comment.
[Akash's agent] Not approving yet — two merge blockers:
build (24.x)is red (see inline onauthMiddleware.ts): the enriched OAutherror_descriptionbroke two exact-match assertions intests/oauth/embedded-authz/oauth.test.ts(lines 71 and 761) that this PR didn't update. Deterministic, not a flake.check-titleis failing — the title isW-23757363:, but the repo's check requires the@W-23757363:form. Please retitle.
The classification approach and authErrorMessage.ts wording look sound, and the reclassification in tool.ts is correctly scoped to raw, non-McpToolError, exactly-401/403 errors — so other tools' error paths are unchanged. Happy to approve once the build is green and the title is fixed.
…_description
The PR enriched the OAuth 401 error_description strings (no-token and
invalid_token) in authMiddleware.ts with the shared multi-server guidance,
but the embedded-authz suite still pinned the old text byte-for-byte and
failed in CI. That suite runs under test:oauth:embedded (not npm test), so a
unit-only run stayed green while build (24.x) went red.
Replace the two toEqual assertions with toMatchObject({ error }) plus
error_description.toContain(<stable substring>) so the description text is no
longer pinned byte-for-byte. Test-only: no production change, no version bump.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…auth-error-clarity
….8.3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…clarity' into asimantov/W-23757363-auth-error-clarity
Akash-Rastogi
left a comment
There was a problem hiding this comment.
[Akash's agent] LGTM — approving. Both blockers from my prior review are cleared:
- Build green — the two embedded-authz assertions now use
toMatchObject({ error })+error_description.toContain(...)(commit1cb67409), the robust fix that no longer pins the enriched description byte-for-byte.build (24.x)andbuild (>=22.7.5 <23)both pass. - Title fixed to
@W-23757363:—check-titlegreen.
The classification logic I reviewed last round is unchanged and sound: authErrorMessage.ts wording is clear (403 correctly hedges role/permission-or-capability and says "don't re-authenticate"), and the raw-401/403 reclassification in tool.ts is scoped to non-McpToolError errors only, so other tools' error paths are untouched. Full CI matrix green. Nice work.
…auth-error-clarity # Conflicts: # package-lock.json # package.json
The allData 403/404 test asserted the raw Axios message for both statuses. After the W-23757363 auth-error-clarity change, logAndExecute rewrites 403 into the shared 'Permission denied (403)' guidance while leaving other statuses (404) raw. Assert the per-status expected text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
||
| **Issue 3: Multiple configured servers — a tool call may target the wrong or unauthenticated server** | ||
|
|
||
| - **Problem:** When more than one Tableau MCP server is configured, the AI client may send a tool call to a different server than you intended — including one that is not authenticated. The resulting `401` can be summarized by the AI as a vague "feature not configured" message. |
There was a problem hiding this comment.
Why is it that a 401 Unauthorized expected with this issue? When there are multiple servers connected, a client does not user credentials from one server when calling the other right? It uses the credentials that pertains to the connection. So it shouldn't be 401 Unauthorized, it would be more like 404 Not Found when you specify a target. For example, if I ask it to get me metadata for a Datasource belonging to MCP server A, but instead it calls MCP server B. The request will be authorized right, just fail with 404 because the datasource was not found.
There was a problem hiding this comment.
I hear your point @stephendeoca, but this can be debatable. You can look at it as we have an authenticated user (for a session on server A) that cannot see resources because no authorization on target server B... catching this error with proper logging.
For me, having 404 error hide the real auth error.
Problem (W-23757363)
When a Tableau MCP server holds a bad/expired credential, or the request reaches the wrong server in a multi-server setup, the failure surfaced as a bare
Request failed with status code 401— or the server crashed at startup while fetching site settings. Models paraphrased both into a misleading "feature not configured" message, making a 401 auth failure indistinguishable from Admin Insights genuinely not being provisioned.Fix
Classify raw thrown
401/403REST errors in the shared web-tool error path (WebTool.logAndExecute) into clear, self-explanatory guidance that names the targeted site and pod. Because it lives in the shared path, it covers every auth mode — PAT, OAuth, direct-trust/connected-app, UAT, passthrough. TypedMcpToolErrors (e.g.AdminOnlyError) bypass reclassification and keep their curated text.src/utils/authErrorMessage.ts(new)buildAuthenticationErrorMessage/buildPermissionErrorMessage)src/tools/web/tool.tssrc/utils/mcpSiteSettings.tssrc/server/oauth/authMiddleware.tserror_descriptionwith the same guidance (WWW-Authenticateunchanged)src/tools/web/adminInsights/resolver.tsdocs/multiple-servers.mddecoder table + edits to auth/oauth/admin-insights/introVersion: 4.8.2 → 4.8.3 (patch).
Scope & risk
resolver.tsis admin-insights-specific.Result.Err), is not anMcpToolError, and status is exactly 401/403. Success paths,ZodiosValidationError, and all typed errors are unchanged.Tests
authErrorMessage.test.ts,authMiddleware.test.ts; extendedtool.test.ts,mcpSiteSettings.test.ts,resolver.test.ts(raw 401, raw 403, curated-403 passthrough, non-auth 503 passthrough, telemetry error_code, startup-401-degrades, 404 de-conflation).kiwimoussetdp2: 401-at-call, 401-at-startup (no crash), 404 not-provisioned, OAuth transport 401 — all verified.Known follow-up (out of scope, noted intentionally)
The OAuth
invalid_token401 path inauthMiddleware.tsreturns 401 + enrichederror_descriptionbut does not set aWWW-Authenticateheader (only the no-token 401 and insufficient-scope 403 do). Pre-existing; RFC 6750 suggests it should. Left for a separate change to keep this PR scoped to W-23757363.🤖 Generated with Claude Code