From 30794d92852bbacf20a2bc58a207e26a490cf522 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 12 Mar 2026 17:10:28 +0300 Subject: [PATCH 1/2] feat: make keyring opt-in and store token at login - Default `coder.useKeyring` to false (opt-in) due to shared logout side effects between CLI and IDE - Update setting description to document sync behavior and version requirements - Store token to OS keyring at login time so the CLI picks it up immediately - Show cancellable progress notification for all CLI keyring invocations during login - Migrate CliCredentialManager public API to options bag pattern with signal and keyringOnly support --- CHANGELOG.md | 17 +++- package.json | 4 +- src/cliConfig.ts | 4 +- src/core/cliCredentialManager.ts | 26 +++-- src/core/cliManager.ts | 14 +-- src/login/loginCoordinator.ts | 31 +++++- src/progress.ts | 28 +++++- test/unit/cliConfig.test.ts | 21 ++-- test/unit/core/cliCredentialManager.test.ts | 84 ++++++++++++++-- test/unit/core/cliManager.test.ts | 28 ++++-- test/unit/login/loginCoordinator.test.ts | 73 +++++++++++++- test/unit/progress.test.ts | 103 +++++++++++++++++--- 12 files changed, 369 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fea55164..b4154886 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,6 @@ ## Unreleased -### Fixed - -- Fixed SSH config writes failing on Windows when antivirus, cloud sync software, - or another process briefly locks the file. - ### Added - Automatically set `reconnectionGraceTime`, `serverShutdownTimeout`, and `maxReconnectionAttempts` @@ -18,6 +13,18 @@ - SSH options from `coder config-ssh --ssh-option` are now applied to VS Code connections, with priority order: VS Code setting > `coder config-ssh` options > deployment config. +### Fixed + +- Fixed SSH config writes failing on Windows when antivirus, cloud sync software, + or another process briefly locks the file. + +### Changed + +- `coder.useKeyring` is now opt-in (default: false). Keyring storage requires CLI >= 2.29.0 for + storage and logout sync, and >= 2.31.0 for syncing login from CLI to VS Code. +- Session tokens are now saved to the OS keyring at login time (when enabled and CLI >= 2.29.0), + not only when connecting to a workspace. + ## [v1.14.0-pre](https://github.com/coder/vscode-coder/releases/tag/v1.14.0-pre) 2026-03-06 ### Added diff --git a/package.json b/package.json index 02c49b40..a07093c4 100644 --- a/package.json +++ b/package.json @@ -157,9 +157,9 @@ } }, "coder.useKeyring": { - "markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of plaintext files. Requires CLI >= 2.29.0. Has no effect on Linux.", + "markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of plaintext files. Requires CLI >= 2.29.0 (>= 2.31.0 to sync login from CLI to VS Code). This will attempt to sync between the CLI and VS Code since they share the same keyring entry. It will log you out of the CLI if you log out of the IDE, and vice versa. Has no effect on Linux.", "type": "boolean", - "default": true + "default": false }, "coder.httpClientLogLevel": { "markdownDescription": "Controls the verbosity of HTTP client logging. This affects what details are logged for each HTTP request and response.", diff --git a/src/cliConfig.ts b/src/cliConfig.ts index 54f9bfb4..be1326c1 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -68,7 +68,9 @@ function isFlag(item: string, name: string): boolean { export function isKeyringEnabled( configs: Pick, ): boolean { - return isKeyringSupported() && configs.get("coder.useKeyring", true); + return ( + isKeyringSupported() && configs.get("coder.useKeyring", false) + ); } /** diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index cd7bbe63..a1c58e6b 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -1,5 +1,6 @@ import { execFile } from "node:child_process"; import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; import * as semver from "semver"; @@ -34,7 +35,8 @@ export type BinaryResolver = (deploymentUrl: string) => Promise; * Returns true on platforms where the OS keyring is supported (macOS, Windows). */ export function isKeyringSupported(): boolean { - return process.platform === "darwin" || process.platform === "win32"; + const platform = os.platform(); + return platform === "darwin" || platform === "win32"; } /** @@ -54,12 +56,15 @@ export class CliCredentialManager { * files under --global-config. * * Keyring and files are mutually exclusive — never both. + * + * When `keyringOnly` is set, silently returns if the keyring is unavailable + * instead of falling back to file storage. */ public async storeToken( url: string, token: string, configs: Pick, - signal?: AbortSignal, + options?: { signal?: AbortSignal; keyringOnly?: boolean }, ): Promise { const binPath = await this.resolveKeyringBinary( url, @@ -67,6 +72,9 @@ export class CliCredentialManager { "keyringAuth", ); if (!binPath) { + if (options?.keyringOnly) { + return; + } await this.writeCredentialFiles(url, token); return; } @@ -80,7 +88,7 @@ export class CliCredentialManager { try { await this.execWithTimeout(binPath, args, { env: { ...process.env, CODER_SESSION_TOKEN: token }, - signal, + signal: options?.signal, }); this.logger.info("Stored token via CLI for", url); } catch (error) { @@ -96,6 +104,7 @@ export class CliCredentialManager { public async readToken( url: string, configs: Pick, + options?: { signal?: AbortSignal }, ): Promise { let binPath: string | undefined; try { @@ -114,10 +123,15 @@ export class CliCredentialManager { const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; try { - const { stdout } = await this.execWithTimeout(binPath, args); + const { stdout } = await this.execWithTimeout(binPath, args, { + signal: options?.signal, + }); const token = stdout.trim(); return token || undefined; } catch (error) { + if ((error as Error).name === "AbortError") { + throw error; + } this.logger.warn("Failed to read token via CLI:", error); return undefined; } @@ -130,11 +144,11 @@ export class CliCredentialManager { public async deleteToken( url: string, configs: Pick, - signal?: AbortSignal, + options?: { signal?: AbortSignal }, ): Promise { await Promise.all([ this.deleteCredentialFiles(url), - this.deleteKeyringToken(url, configs, signal), + this.deleteKeyringToken(url, configs, options?.signal), ]); } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 282c28ab..060f001e 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,8 +10,9 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; +import { isKeyringEnabled } from "../cliConfig"; import * as pgp from "../pgp"; -import { withCancellableProgress } from "../progress"; +import { withCancellableProgress, withOptionalProgress } from "../progress"; import { tempFilePath, toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; @@ -759,13 +760,13 @@ export class CliManager { } const result = await withCancellableProgress( + ({ signal }) => + this.cliCredentialManager.storeToken(url, token, configs, { signal }), { location: vscode.ProgressLocation.Notification, title: `Storing credentials for ${url}`, cancellable: true, }, - ({ signal }) => - this.cliCredentialManager.storeToken(url, token, configs, signal), ); if (result.ok) { return; @@ -783,14 +784,15 @@ export class CliManager { */ public async clearCredentials(url: string): Promise { const configs = vscode.workspace.getConfiguration(); - const result = await withCancellableProgress( + const result = await withOptionalProgress( + ({ signal }) => + this.cliCredentialManager.deleteToken(url, configs, { signal }), { + enabled: isKeyringEnabled(configs), location: vscode.ProgressLocation.Notification, title: `Removing credentials for ${url}`, cancellable: true, }, - ({ signal }) => - this.cliCredentialManager.deleteToken(url, configs, signal), ); if (result.ok) { return; diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 5dfb4677..bbb79f5d 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -4,9 +4,11 @@ import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; import { needToken } from "../api/utils"; +import { isKeyringEnabled } from "../cliConfig"; import { CertificateError } from "../error/certificateError"; import { OAuthAuthorizer } from "../oauth/authorizer"; import { buildOAuthTokenData } from "../oauth/utils"; +import { withOptionalProgress } from "../progress"; import { maybeAskAuthMethod, maybeAskUrl } from "../promptUtils"; import { vscodeProposed } from "../vscodeProposed"; @@ -147,6 +149,19 @@ export class LoginCoordinator implements vscode.Disposable { oauth: result.oauth, // undefined for non-OAuth logins }); await this.mementoManager.addToUrlHistory(url); + + if (result.token) { + this.cliCredentialManager + .storeToken(url, result.token, vscode.workspace.getConfiguration(), { + keyringOnly: true, + }) + .catch((error) => { + this.logger.warn( + "Failed to store token in keyring at login:", + error, + ); + }); + } } } @@ -243,10 +258,20 @@ export class LoginCoordinator implements vscode.Disposable { } // Try keyring token (picks up tokens written by `coder login` in the terminal) - const keyringToken = await this.cliCredentialManager.readToken( - deployment.url, - vscode.workspace.getConfiguration(), + const configs = vscode.workspace.getConfiguration(); + const keyringResult = await withOptionalProgress( + ({ signal }) => + this.cliCredentialManager.readToken(deployment.url, configs, { + signal, + }), + { + enabled: isKeyringEnabled(configs), + location: vscode.ProgressLocation.Notification, + title: "Reading token from OS keyring...", + cancellable: true, + }, ); + const keyringToken = keyringResult.ok ? keyringResult.value : undefined; if ( keyringToken && keyringToken !== providedToken && diff --git a/src/progress.ts b/src/progress.ts index 9de6c600..89963fca 100644 --- a/src/progress.ts +++ b/src/progress.ts @@ -17,8 +17,8 @@ export interface ProgressContext { * `{ cancelled: true }`. */ export function withCancellableProgress( - options: vscode.ProgressOptions & { cancellable: true }, fn: (ctx: ProgressContext) => Promise, + options: vscode.ProgressOptions & { cancellable: true }, ): Thenable> { return vscode.window.withProgress( options, @@ -40,6 +40,32 @@ export function withCancellableProgress( ); } +/** + * Like withCancellableProgress, but only shows the progress notification when + * `enabled` is true. When false, runs the function directly without UI. + * Returns ProgressResult in both cases for uniform call-site handling. + */ +export async function withOptionalProgress( + fn: (ctx: ProgressContext) => Promise, + options: vscode.ProgressOptions & { cancellable: true; enabled: boolean }, +): Promise> { + if (options.enabled) { + return withCancellableProgress(fn, options); + } + try { + const noop = () => { + // No-op: progress reporting is disabled. + }; + const value = await fn({ + progress: { report: noop }, + signal: new AbortController().signal, + }); + return { ok: true, value }; + } catch (error) { + return { ok: false, cancelled: false, error }; + } +} + /** * Run a task inside a VS Code progress notification (no cancellation). * A thin wrapper over `vscode.window.withProgress` that passes only the diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index f627b4bb..14a02697 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -1,5 +1,6 @@ +import * as os from "node:os"; import * as semver from "semver"; -import { afterEach, it, expect, describe, vi } from "vitest"; +import { it, expect, describe, vi } from "vitest"; import { type CliAuth, @@ -14,16 +15,14 @@ import { featureSetForVersion } from "@/featureSet"; import { MockConfigurationProvider } from "../mocks/testHelpers"; import { isWindows } from "../utils/platform"; +vi.mock("node:os"); + const globalConfigAuth: CliAuth = { mode: "global-config", configDir: "/config/dir", }; describe("cliConfig", () => { - afterEach(() => { - vi.unstubAllGlobals(); - }); - describe("getGlobalFlags", () => { const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; @@ -224,6 +223,12 @@ describe("cliConfig", () => { useKeyring: boolean; expected: boolean; } + it("returns false on darwin when setting is unset (default)", () => { + vi.mocked(os.platform).mockReturnValue("darwin"); + const config = new MockConfigurationProvider(); + expect(isKeyringEnabled(config)).toBe(false); + }); + it.each([ { platform: "darwin", useKeyring: true, expected: true }, { platform: "win32", useKeyring: true, expected: true }, @@ -232,7 +237,7 @@ describe("cliConfig", () => { ])( "returns $expected on $platform with useKeyring=$useKeyring", ({ platform, useKeyring, expected }) => { - vi.stubGlobal("process", { ...process, platform }); + vi.mocked(os.platform).mockReturnValue(platform); const config = new MockConfigurationProvider(); config.set("coder.useKeyring", useKeyring); expect(isKeyringEnabled(config)).toBe(expected); @@ -242,7 +247,7 @@ describe("cliConfig", () => { describe("resolveCliAuth", () => { it("returns url mode when keyring should be used", () => { - vi.stubGlobal("process", { ...process, platform: "darwin" }); + vi.mocked(os.platform).mockReturnValue("darwin"); const config = new MockConfigurationProvider(); config.set("coder.useKeyring", true); const featureSet = featureSetForVersion(semver.parse("2.29.0")); @@ -259,7 +264,7 @@ describe("cliConfig", () => { }); it("returns global-config mode when keyring should not be used", () => { - vi.stubGlobal("process", { ...process, platform: "linux" }); + vi.mocked(os.platform).mockReturnValue("linux"); const config = new MockConfigurationProvider(); const featureSet = featureSetForVersion(semver.parse("2.29.0")); const auth = resolveCliAuth( diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index ac0bc998..f8d10773 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -1,6 +1,7 @@ import { fs as memfs, vol } from "memfs"; import { execFile } from "node:child_process"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as os from "node:os"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { isKeyringEnabled } from "@/cliConfig"; import { @@ -19,6 +20,8 @@ vi.mock("node:child_process", () => ({ execFile: vi.fn(), })); +vi.mock("node:os"); + vi.mock("@/cliConfig", () => ({ isKeyringEnabled: vi.fn().mockReturnValue(false), })); @@ -146,17 +149,13 @@ function setup(resolver?: BinaryResolver) { } describe("isKeyringSupported", () => { - afterEach(() => { - vi.unstubAllGlobals(); - }); - it.each([ { platform: "darwin", expected: true }, { platform: "win32", expected: true }, { platform: "linux", expected: false }, { platform: "freebsd", expected: false }, ])("returns $expected for $platform", ({ platform, expected }) => { - vi.stubGlobal("process", { ...process, platform }); + vi.mocked(os.platform).mockReturnValue(platform as NodeJS.Platform); expect(isKeyringSupported()).toBe(expected); }); }); @@ -254,7 +253,9 @@ describe("CliCredentialManager", () => { const { manager } = setup(); const ac = new AbortController(); - await manager.storeToken(TEST_URL, "token", configs, ac.signal); + await manager.storeToken(TEST_URL, "token", configs, { + signal: ac.signal, + }); expect(lastExecArgs().signal).toBe(ac.signal); }); @@ -265,9 +266,47 @@ describe("CliCredentialManager", () => { const { manager } = setup(); await expect( - manager.storeToken(TEST_URL, "token", configs, AbortSignal.abort()), + manager.storeToken(TEST_URL, "token", configs, { + signal: AbortSignal.abort(), + }), ).rejects.toThrow("The operation was aborted"); }); + + it.each([ + { scenario: "keyring disabled", keyringEnabled: false }, + { scenario: "CLI version too old", keyringEnabled: true }, + ])( + "never writes files when keyringOnly and $scenario", + async ({ keyringEnabled }) => { + vi.mocked(isKeyringEnabled).mockReturnValue(keyringEnabled); + if (keyringEnabled) { + vi.mocked(cliUtils.version).mockResolvedValueOnce("2.28.0"); + } + const { manager } = setup(); + + await manager.storeToken(TEST_URL, "token", configs, { + keyringOnly: true, + }); + + expect(execFile).not.toHaveBeenCalled(); + expect(memfs.existsSync(URL_FILE)).toBe(false); + expect(memfs.existsSync(SESSION_FILE)).toBe(false); + }, + ); + + it("uses keyring without writing files when keyringOnly and keyring available", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager } = setup(); + + await manager.storeToken(TEST_URL, "my-token", configs, { + keyringOnly: true, + }); + + expect(execFile).toHaveBeenCalled(); + expect(memfs.existsSync(URL_FILE)).toBe(false); + expect(memfs.existsSync(SESSION_FILE)).toBe(false); + }); }); describe("readToken", () => { @@ -338,6 +377,29 @@ describe("CliCredentialManager", () => { expect(lastExecArgs().timeout).toBe(60_000); }); + + it("passes signal through to execFile", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "token" }); + const { manager } = setup(); + const ac = new AbortController(); + + await manager.readToken(TEST_URL, configs, { signal: ac.signal }); + + expect(lastExecArgs().signal).toBe(ac.signal); + }); + + it("throws AbortError when signal is aborted", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFileAbortable(); + const { manager } = setup(); + + await expect( + manager.readToken(TEST_URL, configs, { + signal: AbortSignal.abort(), + }), + ).rejects.toThrow("The operation was aborted"); + }); }); describe("deleteToken", () => { @@ -418,7 +480,7 @@ describe("CliCredentialManager", () => { const { manager } = setup(); const ac = new AbortController(); - await manager.deleteToken(TEST_URL, configs, ac.signal); + await manager.deleteToken(TEST_URL, configs, { signal: ac.signal }); expect(lastExecArgs().signal).toBe(ac.signal); }); @@ -429,7 +491,9 @@ describe("CliCredentialManager", () => { const { manager } = setup(); await expect( - manager.deleteToken(TEST_URL, configs, AbortSignal.abort()), + manager.deleteToken(TEST_URL, configs, { + signal: AbortSignal.abort(), + }), ).resolves.not.toThrow(); }); }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index cd177ad6..cfd99974 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -9,6 +9,7 @@ import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; +import { isKeyringEnabled } from "@/cliConfig"; import { CliManager } from "@/core/cliManager"; import * as cliUtils from "@/core/cliUtils"; import { PathResolver } from "@/core/pathResolver"; @@ -28,6 +29,11 @@ import type { CliCredentialManager } from "@/core/cliCredentialManager"; vi.mock("os"); vi.mock("axios"); +vi.mock("@/cliConfig", async () => { + const actual = + await vi.importActual("@/cliConfig"); + return { ...actual, isKeyringEnabled: vi.fn().mockReturnValue(false) }; +}); vi.mock("fs", async () => { const memfs: { fs: typeof fs } = await vi.importActual("memfs"); @@ -139,7 +145,7 @@ describe("CliManager", () => { CONFIGURE_URL, TOKEN, expect.anything(), - expect.any(AbortSignal), + { signal: expect.any(AbortSignal) }, ); }); @@ -202,7 +208,20 @@ describe("CliManager", () => { describe("Clear Credentials", () => { const CLEAR_URL = "https://dev.coder.com"; - it("should delete credentials with progress notification", async () => { + it("should skip progress notification when keyring is disabled", async () => { + await manager.clearCredentials(CLEAR_URL); + + expect(vscode.window.withProgress).not.toHaveBeenCalled(); + expect(mockCredManager.deleteToken).toHaveBeenCalledWith( + CLEAR_URL, + expect.anything(), + { signal: expect.any(AbortSignal) }, + ); + }); + + it("should show progress notification when keyring is enabled", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + await manager.clearCredentials(CLEAR_URL); expect(vscode.window.withProgress).toHaveBeenCalledWith( @@ -213,11 +232,6 @@ describe("CliManager", () => { }), expect.any(Function), ); - expect(mockCredManager.deleteToken).toHaveBeenCalledWith( - CLEAR_URL, - expect.anything(), - expect.any(AbortSignal), - ); }); it.each([ diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index fb0b3e08..3ee05ad3 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -16,6 +16,7 @@ import { InMemoryMemento, InMemorySecretStorage, MockConfigurationProvider, + MockProgressReporter, MockUserInteraction, } from "../../mocks/testHelpers"; @@ -111,6 +112,8 @@ function createTestContext() { const mockConfig = new MockConfigurationProvider(); // MockUserInteraction sets up vscode.window dialogs and input boxes const userInteraction = new MockUserInteraction(); + // MockProgressReporter sets up vscode.window.withProgress to execute callbacks + new MockProgressReporter(); const secretStorage = new InMemorySecretStorage(); const memento = new InMemoryMemento(); @@ -118,11 +121,12 @@ function createTestContext() { const secretsManager = new SecretsManager(secretStorage, memento, logger); const mementoManager = new MementoManager(memento); + const mockCredentialManager = createMockCliCredentialManager(); const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, - createMockCliCredentialManager(), + mockCredentialManager, "coder.coder-remote", ); @@ -152,6 +156,7 @@ function createTestContext() { userInteraction, secretsManager, mementoManager, + mockCredentialManager, coordinator, mockSuccessfulAuth, mockAuthFailure, @@ -468,4 +473,70 @@ describe("LoginCoordinator", () => { expect(mockGetAuthenticatedUser).toHaveBeenCalledTimes(2); }); }); + + describe("keyring storage at login", () => { + async function loginWithStoredToken() { + const ctx = createTestContext(); + const user = ctx.mockSuccessfulAuth(); + await ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, { + url: TEST_URL, + token: "stored-token", + }); + const login = async () => { + const result = await ctx.coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }); + // Flush the fire-and-forget storeToken promise + await Promise.resolve(); + return result; + }; + return { ...ctx, user, login }; + } + + it("calls storeToken with keyringOnly after successful login", async () => { + const { mockCredentialManager, login } = await loginWithStoredToken(); + + await login(); + + expect(mockCredentialManager.storeToken).toHaveBeenCalledWith( + TEST_URL, + "stored-token", + expect.anything(), + { keyringOnly: true }, + ); + }); + + it("does not call storeToken for mTLS (empty token)", async () => { + const { + mockConfig, + coordinator, + mockCredentialManager, + mockSuccessfulAuth, + } = createTestContext(); + mockConfig.set("coder.tlsCertFile", "/path/to/cert.pem"); + mockConfig.set("coder.tlsKeyFile", "/path/to/key.pem"); + mockSuccessfulAuth(); + + await coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }); + + expect(mockCredentialManager.storeToken).not.toHaveBeenCalled(); + }); + + it("login succeeds even when keyring storage throws", async () => { + const { mockCredentialManager, user, login } = + await loginWithStoredToken(); + + vi.mocked(mockCredentialManager.storeToken).mockRejectedValueOnce( + new Error("keyring unavailable"), + ); + + const result = await login(); + + expect(result).toEqual({ success: true, user, token: "stored-token" }); + }); + }); }); diff --git a/test/unit/progress.test.ts b/test/unit/progress.test.ts index fbe8b8dd..51e882ab 100644 --- a/test/unit/progress.test.ts +++ b/test/unit/progress.test.ts @@ -3,6 +3,7 @@ import * as vscode from "vscode"; import { withCancellableProgress, + withOptionalProgress, withProgress, type ProgressContext, } from "@/progress"; @@ -41,19 +42,20 @@ describe("withCancellableProgress", () => { }); it("returns ok with value on success", async () => { - const result = await withCancellableProgress(options, () => - Promise.resolve(42), + const result = await withCancellableProgress( + () => Promise.resolve(42), + options, ); expect(result).toEqual({ ok: true, value: 42 }); }); it("returns cancelled on AbortError", async () => { - const result = await withCancellableProgress(options, () => { + const result = await withCancellableProgress(() => { const err = new Error("aborted"); err.name = "AbortError"; return Promise.reject(err); - }); + }, options); expect(result).toEqual({ ok: false, cancelled: true }); }); @@ -61,8 +63,9 @@ describe("withCancellableProgress", () => { it("returns error on non-abort failure", async () => { const error = new Error("something broke"); - const result = await withCancellableProgress(options, () => - Promise.reject(error), + const result = await withCancellableProgress( + () => Promise.reject(error), + options, ); expect(result).toEqual({ ok: false, cancelled: false, error }); @@ -71,10 +74,10 @@ describe("withCancellableProgress", () => { it("provides progress and signal to callback", async () => { let captured: ProgressContext | undefined; - await withCancellableProgress(options, (ctx) => { + await withCancellableProgress((ctx) => { captured = ctx; return Promise.resolve(); - }); + }, options); expect(captured).toBeDefined(); expect(captured!.progress.report).toBeDefined(); @@ -86,10 +89,10 @@ describe("withCancellableProgress", () => { mockWithProgress({ cancelImmediately: true }); let signalAborted: boolean | undefined; - await withCancellableProgress(options, ({ signal }) => { + await withCancellableProgress(({ signal }) => { signalAborted = signal.aborted; return Promise.resolve(); - }); + }, options); expect(signalAborted).toBe(true); }); @@ -97,7 +100,7 @@ describe("withCancellableProgress", () => { it("disposes cancellation listener after completion", async () => { const { dispose } = mockWithProgress(); - await withCancellableProgress(options, () => Promise.resolve("done")); + await withCancellableProgress(() => Promise.resolve("done"), options); expect(dispose).toHaveBeenCalled(); }); @@ -105,15 +108,16 @@ describe("withCancellableProgress", () => { it("disposes cancellation listener on error", async () => { const { dispose } = mockWithProgress(); - await withCancellableProgress(options, () => - Promise.reject(new Error("fail")), + await withCancellableProgress( + () => Promise.reject(new Error("fail")), + options, ); expect(dispose).toHaveBeenCalled(); }); it("passes options through to withProgress", async () => { - await withCancellableProgress(options, () => Promise.resolve()); + await withCancellableProgress(() => Promise.resolve(), options); expect(vscode.window.withProgress).toHaveBeenCalledWith( options, @@ -122,6 +126,77 @@ describe("withCancellableProgress", () => { }); }); +describe("withOptionalProgress", () => { + const options = { + location: vscode.ProgressLocation.Notification, + title: "Test operation", + cancellable: true as const, + enabled: true, + }; + + describe("when enabled", () => { + beforeEach(() => { + mockWithProgress(); + }); + + it("delegates to withCancellableProgress", async () => { + const result = await withOptionalProgress( + () => Promise.resolve(42), + options, + ); + + expect(result).toEqual({ ok: true, value: 42 }); + expect(vscode.window.withProgress).toHaveBeenCalledWith( + expect.objectContaining({ title: "Test operation" }), + expect.any(Function), + ); + }); + }); + + describe("when disabled", () => { + const disabledOptions = { ...options, enabled: false }; + + beforeEach(() => { + vi.mocked(vscode.window.withProgress).mockClear(); + }); + + it("runs directly and returns ok result", async () => { + const result = await withOptionalProgress( + () => Promise.resolve(42), + disabledOptions, + ); + + expect(result).toEqual({ ok: true, value: 42 }); + expect(vscode.window.withProgress).not.toHaveBeenCalled(); + }); + + it("provides progress and signal to callback", async () => { + let captured: ProgressContext | undefined; + + await withOptionalProgress((ctx) => { + captured = ctx; + return Promise.resolve(); + }, disabledOptions); + + expect(captured).toBeDefined(); + expect(typeof captured!.progress.report).toBe("function"); + expect(captured!.signal).toBeInstanceOf(AbortSignal); + expect(captured!.signal.aborted).toBe(false); + }); + + it("returns error on failure", async () => { + const error = new Error("something broke"); + + const result = await withOptionalProgress( + () => Promise.reject(error), + disabledOptions, + ); + + expect(result).toEqual({ ok: false, cancelled: false, error }); + }); + }); +}); + describe("withProgress", () => { beforeEach(() => { mockWithProgress(); From 1db02d7a3611895457b4f914cdda9da0ba5f7d38 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 16 Mar 2026 20:41:36 +0300 Subject: [PATCH 2/2] Review comments --- src/core/cliCredentialManager.ts | 10 ++++++++-- src/error/errorUtils.ts | 7 +++++++ src/login/loginCoordinator.ts | 1 + src/progress.ts | 7 ++++++- test/unit/core/cliCredentialManager.test.ts | 4 ++-- 5 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index a1c58e6b..5debc588 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -6,6 +6,7 @@ import { promisify } from "node:util"; import * as semver from "semver"; import { isKeyringEnabled } from "../cliConfig"; +import { isAbortError } from "../error/errorUtils"; import { featureSetForVersion } from "../featureSet"; import { getHeaderArgs } from "../headers"; import { renameWithRetry, tempFilePath, toSafeHost } from "../util"; @@ -100,6 +101,7 @@ export class CliCredentialManager { /** * Read a token via `coder login token --url`. Returns trimmed stdout, * or undefined on any failure (resolver, CLI, empty output). + * Throws AbortError when the signal is aborted. */ public async readToken( url: string, @@ -129,7 +131,7 @@ export class CliCredentialManager { const token = stdout.trim(); return token || undefined; } catch (error) { - if ((error as Error).name === "AbortError") { + if (isAbortError(error)) { throw error; } this.logger.warn("Failed to read token via CLI:", error); @@ -139,7 +141,8 @@ export class CliCredentialManager { /** * Delete credentials for a deployment. Runs file deletion and keyring - * deletion in parallel, both best-effort (never throws). + * deletion in parallel, both best-effort. Throws AbortError when the + * signal is aborted. */ public async deleteToken( url: string, @@ -255,6 +258,9 @@ export class CliCredentialManager { await this.execWithTimeout(binPath, args, { signal }); this.logger.info("Deleted token via CLI for", url); } catch (error) { + if (isAbortError(error)) { + throw error; + } this.logger.warn("Failed to delete token via CLI:", error); } } diff --git a/src/error/errorUtils.ts b/src/error/errorUtils.ts index 3919d689..c61109d1 100644 --- a/src/error/errorUtils.ts +++ b/src/error/errorUtils.ts @@ -1,6 +1,13 @@ import { isApiError, isApiErrorResponse } from "coder/site/src/api/errors"; import util from "node:util"; +/** + * Check whether an unknown thrown value is an AbortError (signal cancellation). + */ +export function isAbortError(error: unknown): boolean { + return error instanceof Error && error.name === "AbortError"; +} + // getErrorDetail is copied from coder/site, but changes the default return. export const getErrorDetail = (error: unknown): string | undefined | null => { if (isApiError(error)) { diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index bbb79f5d..f3b2740e 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -150,6 +150,7 @@ export class LoginCoordinator implements vscode.Disposable { }); await this.mementoManager.addToUrlHistory(url); + // Fire-and-forget: sync token to OS keyring for the CLI. if (result.token) { this.cliCredentialManager .storeToken(url, result.token, vscode.workspace.getConfiguration(), { diff --git a/src/progress.ts b/src/progress.ts index 89963fca..c91b9b41 100644 --- a/src/progress.ts +++ b/src/progress.ts @@ -1,5 +1,7 @@ import * as vscode from "vscode"; +import { isAbortError } from "./error/errorUtils"; + export type ProgressResult = | { ok: true; value: T } | { ok: false; cancelled: true } @@ -29,7 +31,7 @@ export function withCancellableProgress( const value = await fn({ progress, signal: ac.signal }); return { ok: true, value }; } catch (error) { - if ((error as Error).name === "AbortError") { + if (isAbortError(error)) { return { ok: false, cancelled: true }; } return { ok: false, cancelled: false, error }; @@ -62,6 +64,9 @@ export async function withOptionalProgress( }); return { ok: true, value }; } catch (error) { + if (isAbortError(error)) { + return { ok: false, cancelled: true }; + } return { ok: false, cancelled: false, error }; } } diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index f8d10773..13a2a279 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -485,7 +485,7 @@ describe("CliCredentialManager", () => { expect(lastExecArgs().signal).toBe(ac.signal); }); - it("does not throw when signal is aborted", async () => { + it("throws AbortError when signal is aborted", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFileAbortable(); const { manager } = setup(); @@ -494,7 +494,7 @@ describe("CliCredentialManager", () => { manager.deleteToken(TEST_URL, configs, { signal: AbortSignal.abort(), }), - ).resolves.not.toThrow(); + ).rejects.toThrow("The operation was aborted"); }); }); });