feat(auth): store a browser session and renew it on expiry

Author: chajac

src/commands/auth/logout.test.ts

@@ -0,0 +1,152 @@
+import { afterEach, describe, expect, it, mock } from "bun:test";
+
+import type { CommandContext } from "~/shell/commandContext.js";
+import type { UI } from "~/shell/ui/types.js";
+import { handleLogout } from "./logout.js";
+
+afterEach(() => {
+  mock.restore();
+});
+
+type Task<T> = { message: string; task: () => T | Promise<T> };
+
+function makeCtx(
+  ui: Partial<UI> & { mode: UI["mode"] },
+): CommandContext & { ui: UI } {
+  return {
+    ui: {
+      gap: mock(),
+      intro: mock(),
+      info: mock(),
+      warn: mock(),
+      cancel: mock(),
+      outro: mock(),
+      output: mock(),
+      // Run every task so the test observes the real deletion calls.
+      withProgress: mock(
+        async (tasks: Task<unknown>[], summarise?: unknown) => {
+          const results = [];
+          for (const t of tasks) results.push(await t.task());
+          if (typeof summarise === "function") summarise(results);
+          return results;
+        },
+      ),
+      ...ui,
+    } as unknown as UI,
+    configDir: "/config",
+  } as unknown as CommandContext & { ui: UI };
+}
+
+function makeDeps(args: {
+  stored: boolean;
+  env?: Record<string, string | undefined>;
+}) {
+  return {
+    hasStoredCredentials: mock(async () => args.stored),
+    deleteApiKey: mock(async () => ({
+      keychain: "deleted" as const,
+      file: "deleted" as const,
+    })),
+    deleteTokens: mock(async () => ({
+      keychain: "deleted" as const,
+      file: "deleted" as const,
+    })),
+    env: args.env ?? {},
+  };
+}
+
+// A factory, not a shared constant: a module-level mock would carry its call
+// record from one test into the next.
+function confirmed() {
+  return {
+    mode: "human" as const,
+    confirm: mock(async () => ({ ok: true as const, value: true })),
+  };
+}
+
+describe("handleLogout", () => {
+  it("clears browser tokens as well as the stored API key", async () => {
+    const ctx = makeCtx(confirmed());
+    const deps = makeDeps({ stored: true });
+
+    await handleLogout(ctx, deps);
+
+    expect(deps.deleteApiKey).toHaveBeenCalledTimes(1);
+    expect(deps.deleteTokens).toHaveBeenCalledTimes(1);
+  });
+
+  // The bug this replaces: deletion used to sit behind resolveApiKey, which
+  // refreshes a browser session over the network. Offline, or once WorkOS had
+  // rotated the refresh token away, logout reported "not authenticated" and
+  // left the credentials on disk.
+  it("clears credentials that can no longer be resolved", async () => {
+    const ctx = makeCtx(confirmed());
+    const deps = makeDeps({ stored: true });
+
+    await handleLogout(ctx, deps);
+
+    expect(ctx.ui.info).not.toHaveBeenCalled();
+    expect(deps.deleteApiKey).toHaveBeenCalledTimes(1);
+    expect(deps.deleteTokens).toHaveBeenCalledTimes(1);
+  });
+
+  it("does not consult the network to decide whether to delete", async () => {
+    const ctx = makeCtx(confirmed());
+    const deps = makeDeps({ stored: true });
+
+    await handleLogout(ctx, deps);
+
+    expect(deps.hasStoredCredentials).toHaveBeenCalledTimes(1);
+    expect(deps.hasStoredCredentials).toHaveBeenCalledWith(
+      "/config",
+      undefined,
+    );
+  });
+
+  it("deletes nothing when there is nothing stored", async () => {
+    const ctx = makeCtx({ mode: "human" });
+    const deps = makeDeps({ stored: false });
+
+    await handleLogout(ctx, deps);
+
+    expect(deps.deleteApiKey).not.toHaveBeenCalled();
+    expect(deps.deleteTokens).not.toHaveBeenCalled();
+  });
+
+  it("warns that an environment variable cannot be removed", async () => {
+    const ctx = makeCtx(confirmed());
+    const deps = makeDeps({
+      stored: false,
+      env: { QAWOLF_API_KEY: "qaw_env" },
+    });
+
+    await handleLogout(ctx, deps);
+
+    expect(ctx.ui.warn).toHaveBeenCalled();
+  });
+
+  it("still clears storage when only an environment key is set", async () => {
+    const ctx = makeCtx(confirmed());
+    const deps = makeDeps({
+      stored: false,
+      env: { QAWOLF_API_KEY: "qaw_env" },
+    });
+
+    await handleLogout(ctx, deps);
+
+    expect(deps.deleteTokens).toHaveBeenCalledTimes(1);
+  });
+
+  it("deletes nothing when the confirmation is declined", async () => {
+    const ctx = makeCtx({
+      mode: "human",
+      confirm: mock(async () => ({ ok: true as const, value: false })),
+    });
+    const deps = makeDeps({ stored: true });
+
+    await handleLogout(ctx, deps);
+
+    expect(deps.deleteApiKey).not.toHaveBeenCalled();
+    expect(deps.deleteTokens).not.toHaveBeenCalled();
+  });
+});

src/commands/auth/logout.ts

@@ -1,21 +1,49 @@
-import { deleteApiKey, resolveApiKey } from "~/domains/auth/index.js";
-import {
-  type CommandContext,
-  type CommandResult,
-} from "~/shell/commandContext.js";
+import { deleteApiKey as realDeleteApiKey } from "~/domains/auth/index.js";
+import { deleteTokens as realDeleteTokens } from "~/domains/auth/store/deleteTokens.js";
+import { hasStoredCredentials as realHasStoredCredentials } from "~/domains/auth/store/index.js";
+import type { CommandContext, CommandResult } from "~/shell/commandContext.js";
 import { authMessages } from "~/core/messages/index.js";
 
+type LogoutDeps = {
+  hasStoredCredentials?: (
+    configDir: string,
+    fs: CommandContext["fs"],
+  ) => Promise<boolean>;
+  deleteApiKey?: (
+    configDir: string,
+    fs: CommandContext["fs"],
+  ) => Promise<unknown>;
+  deleteTokens?: (
+    configDir: string,
+    fs: CommandContext["fs"],
+  ) => Promise<unknown>;
+  env?: Record<string, string | undefined>;
+};
+
 export async function handleLogout(
   ctx: CommandContext,
+  deps: LogoutDeps = {},
 ): Promise<CommandResult> {
-  const resolved = await resolveApiKey(ctx.configDir, ctx.fs);
+  const hasStoredCredentials =
+    deps.hasStoredCredentials ?? realHasStoredCredentials;
+  const deleteApiKey = deps.deleteApiKey ?? realDeleteApiKey;
+  const deleteTokens = deps.deleteTokens ?? realDeleteTokens;
+  const env = deps.env ?? process.env;
+
+  // Storage is asked directly rather than through resolveApiKey. Resolving a
+  // browser session refreshes it over the network, so being offline or holding
+  // a refresh token WorkOS has already rotated away would report "not
+  // authenticated" and leave the credentials in place — the one case where
+  // clearing them matters most.
+  const envKey = env["QAWOLF_API_KEY"]?.trim();
+  const stored = await hasStoredCredentials(ctx.configDir, ctx.fs);
 
-  if (!resolved) {
+  if (!envKey && !stored) {
     ctx.ui.info(authMessages.logout.notAuthenticated);
     return;
   }
 
-  if (resolved.source === "env") {
+  if (envKey) {
     ctx.ui.warn(authMessages.logout.envVarWarning);
   }
 
@@ -34,7 +62,15 @@ export async function handleLogout(
     [
       {
         message: authMessages.logout.deleting,
-        task: () => deleteApiKey(ctx.configDir, ctx.fs),
+        // Both credential kinds go, whichever one is present. Clearing only the
+        // one in use would leave the other to take over on the next command,
+        // so "logged out" would not be true.
+        task: async () => {
+          await Promise.all([
+            deleteApiKey(ctx.configDir, ctx.fs),
+            deleteTokens(ctx.configDir, ctx.fs),
+          ]);
+        },
       },
     ],
     () => authMessages.logout.credentialsRemoved,