fix(client): give 401, 403 and 409 their own error kinds (GHY-4738)

packages/cli/src/core/auth/verify.test.ts

@@ -93,6 +93,38 @@ describe("verifyAndProbe", () => {
 
   // A transport failure names no route, and a login that reports only "verification failed
   // (current user)" leaves a user unable to tell a blocked route from an unreachable host.
+  it("reads a 401 on the user as a credential problem", async () => {
+    // The version probe reaches fetch first.
+    const capture = captureFetch([
+      jsonResponse(SESSION_PROPERTIES),
+      jsonResponse({ message: "Unauthenticated" }, 401),
+    ]);
+    vi.stubGlobal("fetch", capture.fetch);
+
+    expect(await verifyAndProbe(BASE_URL, CREDENTIAL)).toMatchObject({
+      ok: false,
+      which: "user",
+      kind: "auth",
+      status: 401,
+    });
+  });
+
+  it("reads a 403 on the user as a server problem, since Metabase serves every identified user", async () => {
+    // The version probe reaches fetch first.
+    const capture = captureFetch([
+      jsonResponse(SESSION_PROPERTIES),
+      jsonResponse({ message: "Forbidden" }, 403),
+    ]);
+    vi.stubGlobal("fetch", capture.fetch);
+
+    expect(await verifyAndProbe(BASE_URL, CREDENTIAL)).toMatchObject({
+      ok: false,
+      which: "user",
+      kind: "server",
+      status: 403,
+    });
+  });
+
   it("names the request a transport failure never reached", async () => {
     const unreachable = [new TypeError("fetch failed"), new TypeError("fetch failed")];
     const capture = captureFetch(unreachable);

packages/cli/src/core/auth/verify.ts

@@ -73,7 +73,9 @@ export async function verifyAndProbe(
 
 function failure(error: unknown, which: VerifyWhich): VerifyFailure {
   if (error instanceof HttpError) {
-    const kind = error.status === 401 || error.status === 403 ? "auth" : "server";
+    // Only a 401 is a credential Metabase did not take; a 403 here comes from whatever answered
+    // instead of Metabase, which serves every identified user their own record.
+    const kind = error.kind === "auth" ? "auth" : "server";
     return {
       ok: false,
       which,

packages/client/README.md

@@ -389,7 +389,7 @@ carries the request context and the Zod issues, and a hand-decoded body or heade
 `ConfigError` and `InternalError` split blame: the first is input a caller could correct, the second
 is a caller that violated a function's contract, which is a bug in the calling code. `HttpError`
 carries the status and response body, redacted of known secrets at construction, plus a `kind`
-(`HttpErrorKind`) separating a route Metabase does not serve from a row that is gone;
+(`HttpErrorKind`) separating a route Metabase does not serve from a row that is gone, and a credential Metabase did not accept (`auth`, 401) from a request it refused for an identified user (`forbidden`, 403) or that conflicts with existing state (`conflict`, 409);
 `isHttpNotFound(value)` answers the coarser question of whether a thrown value is an `HttpError` with
 status 404. `ChainedRequestError` wraps a cause and delegates its category and retryability to it.
 `toMetabaseError(unknown)` normalizes a thrown value into the taxonomy.

packages/client/src/http/errors.test.ts

@@ -151,9 +151,42 @@ describe("HttpError message extraction", () => {
     );
   });
 
-  it("emits an auth message with the host for 403 with no body", () => {
+  it("calls a 403 with no body a refusal, not a bad key, since Metabase identified the user", () => {
     expect(buildHttpError({ status: 403, rawBody: null }).message).toBe(
-      "Invalid or unauthorized API key (host: example.invalid).",
+      "The request was refused (403): the signed-in user is not allowed to do this.",
+    );
+  });
+
+  it("names Metabase's reason for a 403 with a text/plain body instead of blaming the key", () => {
+    const refused = buildHttpError({
+      status: 403,
+      responseHeaders: textHeaders(),
+      rawBody: "A table with that name already exists.",
+    });
+    expect(refused.message).toBe("A table with that name already exists.");
+    expect(refused.kind).toBe("forbidden");
+  });
+
+  it("keeps the key message for a 401 whose text/plain body is only Unauthenticated", () => {
+    expect(
+      buildHttpError({ status: 401, responseHeaders: textHeaders(), rawBody: "Unauthenticated" })
+        .message,
+    ).toBe("Invalid or unauthorized API key (host: example.invalid).");
+  });
+
+  it("names Metabase's reason for a 409 with a text/plain body", () => {
+    expect(
+      buildHttpError({
+        status: 409,
+        responseHeaders: textHeaders(),
+        rawBody: "A table with that name already exists.",
+      }).message,
+    ).toBe("A table with that name already exists.");
+  });
+
+  it("calls a 409 with no body a conflict", () => {
+    expect(buildHttpError({ status: 409, rawBody: null }).message).toBe(
+      "The request was refused (409): it conflicts with what already exists.",
     );
   });
 
@@ -309,9 +342,16 @@ describe("HttpError field errors", () => {
 });
 
 describe("HttpError kind classification", () => {
-  it("classifies 401 and 403 as auth", () => {
+  it("classifies 401 as auth: the credential was not accepted", () => {
     expect(buildHttpError({ status: 401 }).kind).toBe("auth");
-    expect(buildHttpError({ status: 403 }).kind).toBe("auth");
+  });
+
+  it("classifies 403 as forbidden: the user was identified and refused", () => {
+    expect(buildHttpError({ status: 403 }).kind).toBe("forbidden");
+  });
+
+  it("classifies 409 as conflict", () => {
+    expect(buildHttpError({ status: 409 }).kind).toBe("conflict");
   });
 
   it("classifies 429 as rate-limit", () => {

packages/client/src/http/errors.ts

@@ -10,6 +10,8 @@ export type HttpErrorKind =
   | "route-missing"
   | "resource-missing"
   | "auth"
+  | "forbidden"
+  | "conflict"
   | "rate-limit"
   | "server-error"
   | "generic";
@@ -27,8 +29,15 @@ const RESOURCE_MISSING_LITERAL = "Not found.";
 
 const STATUS_CLASSIFICATIONS: Record<number, StatusClassification> = {
   401: { retryable: false },
-  403: { retryable: false },
+  403: {
+    retryable: false,
+    message: "The request was refused (403): the signed-in user is not allowed to do this.",
+  },
   404: { retryable: false },
+  409: {
+    retryable: false,
+    message: "The request was refused (409): it conflicts with what already exists.",
+  },
   408: { retryable: true, message: "Metabase timed out responding." },
   425: { retryable: true },
   429: { retryable: true, message: "Metabase rate-limited the request." },
@@ -198,9 +207,17 @@ function classifyKind(
   sanitizedBody: string | null,
   redactedHeaders: Record<string, string>,
 ): HttpErrorKind {
-  if (status === 401 || status === 403) {
+  // 401: Metabase did not accept the credential. 403: it identified the user and refused the
+  // request. 409: the request conflicts with existing state. None of the last two is about the key.
+  if (status === 401) {
     return "auth";
   }
+  if (status === 403) {
+    return "forbidden";
+  }
+  if (status === 409) {
+    return "conflict";
+  }
   if (status === NOT_FOUND_STATUS) {
     return isRouteMissingResponse(sanitizedBody, redactedHeaders)
       ? "route-missing"

packages/client/src/index.test.ts

@@ -190,6 +190,12 @@ function describeKind(kind: HttpErrorKind): string {
     case "auth": {
       return "the credential was rejected";
     }
+    case "forbidden": {
+      return "the user was identified and the request refused";
+    }
+    case "conflict": {
+      return "the request conflicts with existing state";
+    }
     case "rate-limit": {
       return "the caller is sending too fast";
     }

packages/client/src/resources/dashboard.ts

@@ -237,7 +237,7 @@ export function dashboardResource(transport: Transport) {
       if (error.kind === "resource-missing") {
         return { reason: "missing" };
       }
-      if (error.kind === "auth") {
+      if (error.kind === "auth" || error.kind === "forbidden") {
         return { reason: "unreadable", detail: error.userMessage };
       }
       throw error;

tests/e2e/profiles.e2e.test.ts

@@ -199,7 +199,7 @@ describe("profiles e2e", () => {
       configHome,
     });
     expect(limitedQuery.exitCode).toBe(1);
-    expect(limitedQuery.stderr).toContain("Invalid or unauthorized API key");
+    expect(limitedQuery.stderr).toContain("You don't have permissions to do that.");
     expect(limitedQuery.stdout).toBe("");
   });