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 fea55164c5..b4154886ac 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 02c49b40e5..a07093c4e2 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 54f9bfb495..be1326c1d0 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 cd7bbe6328..a1c58e6b54 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 282c28ab4b..060f001e31 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 5dfb4677a1..bbb79f5de6 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 9de6c600f6..89963fca50 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 f627b4bb2a..14a026971a 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 ac0bc998ed..f8d1077365 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 cd177ad60e..cfd99974cf 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 fb0b3e08ea..3ee05ad388 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 fbe8b8dd0a..51e882ab65 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 a1c58e6b54..5debc58857 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 3919d68988..c61109d125 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 bbb79f5de6..f3b2740ea0 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 89963fca50..c91b9b41ed 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 f8d1077365..13a2a279dd 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"); }); }); });