From 58c1e3f598c3e72a1f9441d7aae35c921601548d Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 4 Mar 2026 16:41:29 +0300 Subject: [PATCH 1/6] feat: store session tokens in the OS keyring (#808) On macOS and Windows with CLI >= 2.29.0, write session tokens to the OS keyring (Keychain / Credential Manager) instead of plaintext files. The CLI reads from the keyring when invoked with --url instead of --global-config. Falls back to file storage on Linux, or older CLIs. Key changes: - Harden file fallback with mode 0o600 - Add shouldUseKeyring() as single source of truth gating on CLI version, platform, and coder.useKeyring setting - Restructure remote.ts setup() to call configure() after featureSet is known, so the keyring decision can be made - Add keyring read fallback in LoginCoordinator for tokens written by `coder login` from the terminal - Add vendor-keyring.mjs build script to copy native binaries into dist/node_modules/ for VSIX packaging (vsce can't follow pnpm symlinks) - Add KeyringStore wrapping @napi-rs/keyring with the CLI's credential format (JSON map keyed by host, base64 on macOS, raw bytes on Windows) --- CHANGELOG.md | 6 + esbuild.mjs | 2 +- eslint.config.mjs | 2 +- package.json | 8 +- pnpm-lock.yaml | 135 +++++++++++ pnpm-workspace.yaml | 13 ++ scripts/vendor-keyring.mjs | 61 +++++ src/api/workspace.ts | 6 +- src/cliConfig.ts | 59 ++++- src/commands.ts | 27 ++- src/core/cliManager.ts | 116 +++++++--- src/core/container.ts | 14 +- src/featureSet.ts | 6 + src/keyringStore.ts | 194 ++++++++++++++++ src/login/loginCoordinator.ts | 32 +++ src/remote/remote.ts | 94 +++++--- src/remote/workspaceStateMachine.ts | 9 +- test/mocks/testHelpers.ts | 9 + test/unit/cliConfig.test.ts | 226 +++++++++++++++--- test/unit/core/cliManager.concurrent.test.ts | 2 + test/unit/core/cliManager.test.ts | 169 +++++++++++--- test/unit/featureSet.test.ts | 12 + test/unit/keyringStore.test.ts | 231 +++++++++++++++++++ test/unit/login/loginCoordinator.test.ts | 3 + 24 files changed, 1281 insertions(+), 155 deletions(-) create mode 100644 scripts/vendor-keyring.mjs create mode 100644 src/keyringStore.ts create mode 100644 test/unit/keyringStore.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index c3c5db37..0f7cabc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,12 @@ - Fixed CLI binary downloads failing when servers or proxies compress responses unexpectedly. - Clarified CLI download progress notification wording. +### Added + +- Session tokens are now stored in the OS keyring (Keychain on macOS, Credential Manager on + Windows) instead of plaintext files, when using CLI >= 2.29.0. Falls back to file storage on + Linux, older CLIs, or if the keyring write fails. Controlled via the `coder.useKeyring` setting. + ## [v1.13.0](https://github.com/coder/vscode-coder/releases/tag/v1.13.0) 2026-03-03 ### Added diff --git a/esbuild.mjs b/esbuild.mjs index 54fb4a1d..9a0e89f3 100644 --- a/esbuild.mjs +++ b/esbuild.mjs @@ -32,7 +32,7 @@ const buildOptions = { // undefined when bundled to CJS, causing runtime errors. openpgp: "./node_modules/openpgp/dist/node/openpgp.min.cjs", }, - external: ["vscode"], + external: ["vscode", "@napi-rs/keyring"], sourcemap: production ? "external" : true, minify: production, plugins: watch ? [logRebuildPlugin] : [], diff --git a/eslint.config.mjs b/eslint.config.mjs index 513b6326..f21458b2 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -154,7 +154,7 @@ export default defineConfig( // Build config - ESM with Node globals { - files: ["esbuild.mjs"], + files: ["esbuild.mjs", "scripts/*.mjs"], languageOptions: { globals: { ...globals.node, diff --git a/package.json b/package.json index 52bb6135..d6f39205 100644 --- a/package.json +++ b/package.json @@ -32,7 +32,7 @@ "test:integration": "tsc -p test --outDir out --noCheck && node esbuild.mjs && vscode-test", "test:webview": "vitest --project webview", "typecheck": "concurrently -g \"tsc --noEmit\" \"tsc --noEmit -p test\"", - "vscode:prepublish": "pnpm build:production", + "vscode:prepublish": "pnpm build:production && node scripts/vendor-keyring.mjs", "watch": "concurrently -n extension,webviews \"pnpm watch:extension\" \"pnpm watch:webviews\"", "watch:extension": "node esbuild.mjs --watch", "watch:webviews": "pnpm -r --filter \"./packages/*\" --parallel dev" @@ -156,6 +156,11 @@ "type": "string" } }, + "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.", + "type": "boolean", + "default": true + }, "coder.httpClientLogLevel": { "markdownDescription": "Controls the verbosity of HTTP client logging. This affects what details are logged for each HTTP request and response.", "type": "string", @@ -463,6 +468,7 @@ "word-wrap": "1.2.5" }, "dependencies": { + "@napi-rs/keyring": "^1.2.0", "@peculiar/x509": "^1.14.3", "@repo/shared": "workspace:*", "axios": "1.13.6", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9f685f62..77808dae 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -58,6 +58,9 @@ importers: .: dependencies: + '@napi-rs/keyring': + specifier: ^1.2.0 + version: 1.2.0 '@peculiar/x509': specifier: ^1.14.3 version: 1.14.3 @@ -986,6 +989,87 @@ packages: '@lit/reactive-element@2.1.2': resolution: {integrity: sha512-pbCDiVMnne1lYUIaYNN5wrwQXDtHaYtg7YEFPeW+hws6U47WeFvISGUWekPGKWOP1ygrs0ef0o1VJMk1exos5A==} + '@napi-rs/keyring-darwin-arm64@1.2.0': + resolution: {integrity: sha512-CA83rDeyONDADO25JLZsh3eHY8yTEtm/RS6ecPsY+1v+dSawzT9GywBMu2r6uOp1IEhQs/xAfxgybGAFr17lSA==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [darwin] + + '@napi-rs/keyring-darwin-x64@1.2.0': + resolution: {integrity: sha512-dBHjtKRCj4ByfnfqIKIJLo3wueQNJhLRyuxtX/rR4K/XtcS7VLlRD01XXizjpre54vpmObj63w+ZpHG+mGM8uA==} + engines: {node: '>= 10'} + cpu: [x64] + os: [darwin] + + '@napi-rs/keyring-freebsd-x64@1.2.0': + resolution: {integrity: sha512-DPZFr11pNJSnaoh0dzSUNF+T6ORhy3CkzUT3uGixbA71cAOPJ24iG8e8QrLOkuC/StWrAku3gBnth2XMWOcR3Q==} + engines: {node: '>= 10'} + cpu: [x64] + os: [freebsd] + + '@napi-rs/keyring-linux-arm-gnueabihf@1.2.0': + resolution: {integrity: sha512-8xv6DyEMlvRdqJzp4F39RLUmmTQsLcGYYv/3eIfZNZN1O5257tHxTrFYqAsny659rJJK2EKeSa7PhrSibQqRWQ==} + engines: {node: '>= 10'} + cpu: [arm] + os: [linux] + + '@napi-rs/keyring-linux-arm64-gnu@1.2.0': + resolution: {integrity: sha512-Pu2V6Py+PBt7inryEecirl+t+ti8bhZphjP+W68iVaXHUxLdWmkgL9KI1VkbRHbx5k8K5Tew9OP218YfmVguIA==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [linux] + libc: [glibc] + + '@napi-rs/keyring-linux-arm64-musl@1.2.0': + resolution: {integrity: sha512-8TDymrpC4P1a9iDEaegT7RnrkmrJN5eNZh3Im3UEV5PPYGtrb82CRxsuFohthCWQW81O483u1bu+25+XA4nKUw==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [linux] + libc: [musl] + + '@napi-rs/keyring-linux-riscv64-gnu@1.2.0': + resolution: {integrity: sha512-awsB5XI1MYL7fwfjMDGmKOWvNgJEO7mM7iVEMS0fO39f0kVJnOSjlu7RHcXAF0LOx+0VfF3oxbWqJmZbvRCRHw==} + engines: {node: '>= 10'} + cpu: [riscv64] + os: [linux] + libc: [glibc] + + '@napi-rs/keyring-linux-x64-gnu@1.2.0': + resolution: {integrity: sha512-8E+7z4tbxSJXxIBqA+vfB1CGajpCDRyTyqXkBig5NtASrv4YXcntSo96Iah2QDR5zD3dSTsmbqJudcj9rKKuHQ==} + engines: {node: '>= 10'} + cpu: [x64] + os: [linux] + libc: [glibc] + + '@napi-rs/keyring-linux-x64-musl@1.2.0': + resolution: {integrity: sha512-8RZ8yVEnmWr/3BxKgBSzmgntI7lNEsY7xouNfOsQkuVAiCNmxzJwETspzK3PQ2FHtDxgz5vHQDEBVGMyM4hUHA==} + engines: {node: '>= 10'} + cpu: [x64] + os: [linux] + libc: [musl] + + '@napi-rs/keyring-win32-arm64-msvc@1.2.0': + resolution: {integrity: sha512-AoqaDZpQ6KPE19VBLpxyORcp+yWmHI9Xs9Oo0PJ4mfHma4nFSLVdhAubJCxdlNptHe5va7ghGCHj3L9Akiv4cQ==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [win32] + + '@napi-rs/keyring-win32-ia32-msvc@1.2.0': + resolution: {integrity: sha512-EYL+EEI6bCsYi3LfwcQdnX3P/R76ENKNn+3PmpGheBsUFLuh0gQuP7aMVHM4rTw6UVe+L3vCLZSptq/oeacz0A==} + engines: {node: '>= 10'} + cpu: [ia32] + os: [win32] + + '@napi-rs/keyring-win32-x64-msvc@1.2.0': + resolution: {integrity: sha512-xFlx/TsmqmCwNU9v+AVnEJgoEAlBYgzFF5Ihz1rMpPAt4qQWWkMd4sCyM1gMJ1A/GnRqRegDiQpwaxGUHFtFbA==} + engines: {node: '>= 10'} + cpu: [x64] + os: [win32] + + '@napi-rs/keyring@1.2.0': + resolution: {integrity: sha512-d0d4Oyxm+v980PEq1ZH2PmS6cvpMIRc17eYpiU47KgW+lzxklMu6+HOEOPmxrpnF/XQZ0+Q78I2mgMhbIIo/dg==} + engines: {node: '>= 10'} + '@napi-rs/wasm-runtime@0.2.12': resolution: {integrity: sha512-ZVWUcfwY4E/yPitQJl481FjFo3K22D6qF0DuFH6Y/nbnE11GY5uguDxZMGXPQ8WQ0128MXQD7TnfHyK4oWoIJQ==} @@ -4922,6 +5006,57 @@ snapshots: dependencies: '@lit-labs/ssr-dom-shim': 1.5.1 + '@napi-rs/keyring-darwin-arm64@1.2.0': + optional: true + + '@napi-rs/keyring-darwin-x64@1.2.0': + optional: true + + '@napi-rs/keyring-freebsd-x64@1.2.0': + optional: true + + '@napi-rs/keyring-linux-arm-gnueabihf@1.2.0': + optional: true + + '@napi-rs/keyring-linux-arm64-gnu@1.2.0': + optional: true + + '@napi-rs/keyring-linux-arm64-musl@1.2.0': + optional: true + + '@napi-rs/keyring-linux-riscv64-gnu@1.2.0': + optional: true + + '@napi-rs/keyring-linux-x64-gnu@1.2.0': + optional: true + + '@napi-rs/keyring-linux-x64-musl@1.2.0': + optional: true + + '@napi-rs/keyring-win32-arm64-msvc@1.2.0': + optional: true + + '@napi-rs/keyring-win32-ia32-msvc@1.2.0': + optional: true + + '@napi-rs/keyring-win32-x64-msvc@1.2.0': + optional: true + + '@napi-rs/keyring@1.2.0': + optionalDependencies: + '@napi-rs/keyring-darwin-arm64': 1.2.0 + '@napi-rs/keyring-darwin-x64': 1.2.0 + '@napi-rs/keyring-freebsd-x64': 1.2.0 + '@napi-rs/keyring-linux-arm-gnueabihf': 1.2.0 + '@napi-rs/keyring-linux-arm64-gnu': 1.2.0 + '@napi-rs/keyring-linux-arm64-musl': 1.2.0 + '@napi-rs/keyring-linux-riscv64-gnu': 1.2.0 + '@napi-rs/keyring-linux-x64-gnu': 1.2.0 + '@napi-rs/keyring-linux-x64-musl': 1.2.0 + '@napi-rs/keyring-win32-arm64-msvc': 1.2.0 + '@napi-rs/keyring-win32-ia32-msvc': 1.2.0 + '@napi-rs/keyring-win32-x64-msvc': 1.2.0 + '@napi-rs/wasm-runtime@0.2.12': dependencies: '@emnapi/core': 1.7.1 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 70f3d76e..613661d2 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -27,3 +27,16 @@ onlyBuiltDependencies: - keytar - unrs-resolver - utf-8-validate + +# Install @napi-rs/keyring native binaries for macOS and Windows so they're +# available when building the universal VSIX (even on Linux CI). +# Only macOS and Windows use the keyring; Linux falls back to file storage. +supportedArchitectures: + os: + - current + - darwin + - win32 + cpu: + - current + - x64 + - arm64 diff --git a/scripts/vendor-keyring.mjs b/scripts/vendor-keyring.mjs new file mode 100644 index 00000000..5cf8cd31 --- /dev/null +++ b/scripts/vendor-keyring.mjs @@ -0,0 +1,61 @@ +/** + * Vendor @napi-rs/keyring into dist/node_modules/ for VSIX packaging. + * + * pnpm uses symlinks that vsce can't follow. This script resolves them and + * copies the JS wrapper plus macOS/Windows .node binaries into dist/, where + * Node's require() resolution finds them from dist/extension.js. + */ +import { + cpSync, + existsSync, + mkdirSync, + readdirSync, + realpathSync, + rmSync, +} from "node:fs"; +import { join, resolve } from "node:path"; + +const keyringPkg = resolve("node_modules/@napi-rs/keyring"); +const outputDir = resolve("dist/node_modules/@napi-rs/keyring"); + +if (!existsSync(keyringPkg)) { + console.error("@napi-rs/keyring not found — run pnpm install first"); + process.exit(1); +} + +// Copy the JS wrapper package (resolving pnpm symlinks) +const resolvedPkg = realpathSync(keyringPkg); +rmSync(outputDir, { recursive: true, force: true }); +mkdirSync(outputDir, { recursive: true }); +cpSync(resolvedPkg, outputDir, { recursive: true }); + +// Native binary packages live as siblings of the resolved keyring package in +// pnpm's content-addressable store (they aren't hoisted to node_modules). +const siblingsDir = resolve(resolvedPkg, ".."); +const nativePackages = [ + "keyring-darwin-arm64", + "keyring-darwin-x64", + "keyring-win32-arm64-msvc", + "keyring-win32-x64-msvc", +]; + +for (const pkg of nativePackages) { + const pkgDir = join(siblingsDir, pkg); + if (!existsSync(pkgDir)) { + console.error( + `Missing native package: ${pkg}\n` + + "Ensure supportedArchitectures in pnpm-workspace.yaml includes all target platforms.", + ); + process.exit(1); + } + const nodeFile = readdirSync(pkgDir).find((f) => f.endsWith(".node")); + if (!nodeFile) { + console.error(`No .node binary found in ${pkg}`); + process.exit(1); + } + cpSync(join(pkgDir, nodeFile), join(outputDir, nodeFile)); +} + +console.log( + `Vendored @napi-rs/keyring with ${nativePackages.length} platform binaries into dist/`, +); diff --git a/src/api/workspace.ts b/src/api/workspace.ts index fdedfcb8..e7d38327 100644 --- a/src/api/workspace.ts +++ b/src/api/workspace.ts @@ -7,7 +7,7 @@ import { import { spawn } from "node:child_process"; import * as vscode from "vscode"; -import { getGlobalFlags } from "../cliConfig"; +import { type CliAuth, getGlobalFlags } from "../cliConfig"; import { type FeatureSet } from "../featureSet"; import { escapeCommandArg } from "../util"; import { type UnidirectionalStream } from "../websocket/eventStreamConnection"; @@ -50,7 +50,7 @@ export class LazyStream { */ export async function startWorkspaceIfStoppedOrFailed( restClient: Api, - globalConfigDir: string, + auth: CliAuth, binPath: string, workspace: Workspace, writeEmitter: vscode.EventEmitter, @@ -65,7 +65,7 @@ export async function startWorkspaceIfStoppedOrFailed( return new Promise((resolve, reject) => { const startArgs = [ - ...getGlobalFlags(vscode.workspace.getConfiguration(), globalConfigDir), + ...getGlobalFlags(vscode.workspace.getConfiguration(), auth), "start", "--yes", createWorkspaceIdentifier(workspace), diff --git a/src/cliConfig.ts b/src/cliConfig.ts index 1f23949d..6831117e 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -1,8 +1,15 @@ -import { type WorkspaceConfiguration } from "vscode"; - import { getHeaderArgs } from "./headers"; +import { isKeyringSupported } from "./keyringStore"; import { escapeCommandArg } from "./util"; +import type { WorkspaceConfiguration } from "vscode"; + +import type { FeatureSet } from "./featureSet"; + +export type CliAuth = + | { mode: "global-config"; configDir: string } + | { mode: "url"; url: string }; + /** * Returns the raw global flags from user configuration. */ @@ -14,21 +21,61 @@ export function getGlobalFlagsRaw( /** * Returns global configuration flags for Coder CLI commands. - * Always includes the `--global-config` argument with the specified config directory. + * Includes either `--global-config` or `--url` depending on the auth mode. */ export function getGlobalFlags( configs: Pick, - configDir: string, + auth: CliAuth, ): string[] { + const authFlags = + auth.mode === "url" + ? ["--url", escapeCommandArg(auth.url)] + : ["--global-config", escapeCommandArg(auth.configDir)]; + // Last takes precedence/overrides previous ones return [ ...getGlobalFlagsRaw(configs), - "--global-config", - escapeCommandArg(configDir), + ...authFlags, ...getHeaderArgs(configs), ]; } +/** + * Returns true when the user has keyring enabled and the platform supports it. + */ +export function isKeyringEnabled( + configs: Pick, +): boolean { + return isKeyringSupported() && configs.get("coder.useKeyring", true); +} + +/** + * Single source of truth: should the extension use the OS keyring for this session? + * Requires CLI >= 2.29.0, macOS or Windows, and the coder.useKeyring setting enabled. + */ +export function shouldUseKeyring( + configs: Pick, + featureSet: FeatureSet, +): boolean { + return isKeyringEnabled(configs) && featureSet.keyringAuth; +} + +/** + * Resolves how the CLI should authenticate: via the keyring (`--url`) or via + * the global config directory (`--global-config`). + */ +export function resolveCliAuth( + configs: Pick, + featureSet: FeatureSet, + deploymentUrl: string, + configDir: string, +): CliAuth { + if (shouldUseKeyring(configs, featureSet)) { + return { mode: "url", url: deploymentUrl }; + } + return { mode: "global-config", configDir }; +} + /** * Returns SSH flags for the `coder ssh` command from user configuration. */ diff --git a/src/commands.ts b/src/commands.ts index 107f9556..366e9f49 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -4,12 +4,14 @@ import { } from "coder/site/src/api/typesGenerated"; import * as fs from "node:fs/promises"; import * as path from "node:path"; +import * as semver from "semver"; import * as vscode from "vscode"; import { createWorkspaceIdentifier, extractAgents } from "./api/api-helper"; import { type CoderApi } from "./api/coderApi"; -import { getGlobalFlags } from "./cliConfig"; +import { getGlobalFlags, resolveCliAuth } from "./cliConfig"; import { type CliManager } from "./core/cliManager"; +import * as cliUtils from "./core/cliUtils"; import { type ServiceContainer } from "./core/container"; import { type MementoManager } from "./core/mementoManager"; import { type PathResolver } from "./core/pathResolver"; @@ -17,6 +19,7 @@ import { type SecretsManager } from "./core/secretsManager"; import { type DeploymentManager } from "./deployment/deploymentManager"; import { CertificateError } from "./error/certificateError"; import { toError } from "./error/errorUtils"; +import { featureSetForVersion } from "./featureSet"; import { type Logger } from "./logging/logger"; import { type LoginCoordinator } from "./login/loginCoordinator"; import { maybeAskAgent, maybeAskUrl } from "./promptUtils"; @@ -210,12 +213,13 @@ export class Commands { this.logger.debug("Logging out"); - const safeHostname = - this.deploymentManager.getCurrentDeployment()?.safeHostname; + const deployment = this.deploymentManager.getCurrentDeployment(); + const safeHostname = deployment?.safeHostname; await this.deploymentManager.clearDeployment(); if (safeHostname) { + await this.cliManager.clearCredentials(safeHostname); await this.secretsManager.clearAllAuthData(safeHostname); } @@ -283,6 +287,7 @@ export class Commands { if (selected.hostnames.length === 1) { const selectedHostname = selected.hostnames[0]; + await this.cliManager.clearCredentials(selectedHostname); await this.secretsManager.clearAllAuthData(selectedHostname); this.logger.info("Removed credentials for", selectedHostname); vscode.window.showInformationMessage( @@ -300,9 +305,10 @@ export class Commands { ); if (confirm === "Remove All") { await Promise.all( - selected.hostnames.map((h) => - this.secretsManager.clearAllAuthData(h), - ), + selected.hostnames.map(async (h) => { + await this.cliManager.clearCredentials(h); + await this.secretsManager.clearAllAuthData(h); + }), ); this.logger.info( "Removed credentials for all deployments:", @@ -452,11 +458,12 @@ export class Commands { safeHost, ); + const version = semver.parse(await cliUtils.version(binary)); + const featureSet = featureSetForVersion(version); const configDir = this.pathResolver.getGlobalConfigDir(safeHost); - const globalFlags = getGlobalFlags( - vscode.workspace.getConfiguration(), - configDir, - ); + const configs = vscode.workspace.getConfiguration(); + const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir); + const globalFlags = getGlobalFlags(configs, auth); terminal.sendText( `${escapeCommandArg(binary)} ${globalFlags.join(" ")} ssh ${app.workspace_name}`, ); diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 57d0dd76..6f918eb0 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -2,24 +2,30 @@ import globalAxios, { type AxiosInstance, type AxiosRequestConfig, } from "axios"; -import { type Api } from "coder/site/src/api/api"; import { createWriteStream, type WriteStream } from "node:fs"; import fs from "node:fs/promises"; -import { type IncomingMessage } from "node:http"; import path from "node:path"; import prettyBytes from "pretty-bytes"; import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { type Logger } from "../logging/logger"; +import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; import { vscodeProposed } from "../vscodeProposed"; import { BinaryLock } from "./binaryLock"; import * as cliUtils from "./cliUtils"; import * as downloadProgress from "./downloadProgress"; -import { type PathResolver } from "./pathResolver"; + +import type { Api } from "coder/site/src/api/api"; +import type { IncomingMessage } from "node:http"; + +import type { FeatureSet } from "../featureSet"; +import type { KeyringStore } from "../keyringStore"; +import type { Logger } from "../logging/logger"; + +import type { PathResolver } from "./pathResolver"; export class CliManager { private readonly binaryLock: BinaryLock; @@ -27,6 +33,7 @@ export class CliManager { constructor( private readonly output: Logger, private readonly pathResolver: PathResolver, + private readonly keyringStore: KeyringStore, ) { this.binaryLock = new BinaryLock(output); } @@ -705,48 +712,95 @@ export class CliManager { /** * Configure the CLI for the deployment with the provided hostname. * - * Falsey URLs and null tokens are a no-op; we avoid unconfiguring the CLI to - * avoid breaking existing connections. + * Stores credentials in the OS keyring when available, otherwise falls back + * to writing plaintext files under --global-config for the CLI. + * + * Both URL and token are required. Empty tokens are allowed (e.g. mTLS + * authentication) but the URL must be a non-empty string. */ public async configure( safeHostname: string, - url: string | undefined, - token: string | null, + url: string, + token: string, + featureSet: FeatureSet, ) { + if (!url) { + throw new Error("URL is required to configure the CLI"); + } + + const configs = vscode.workspace.getConfiguration(); + if (shouldUseKeyring(configs, featureSet)) { + try { + this.keyringStore.setToken(url, token); + this.output.info("Stored token in OS keyring for", url); + } catch (error) { + this.output.error("Failed to store token in OS keyring:", error); + vscode.window + .showErrorMessage( + `Failed to store session token in OS keyring: ${errToStr(error)}. ` + + "Disable keyring storage in settings to use plaintext files instead.", + "Open Settings", + ) + .then((action) => { + if (action === "Open Settings") { + vscode.commands.executeCommand( + "workbench.action.openSettings", + "coder.useKeyring", + ); + } + }); + throw error; + } + } else { + await Promise.all([ + this.writeUrlToGlobalConfig(safeHostname, url), + this.writeTokenToGlobalConfig(safeHostname, token), + ]); + } + } + + /** + * Remove credentials for a deployment from both keyring and file storage. + */ + public async clearCredentials(safeHostname: string): Promise { + if (isKeyringEnabled(vscode.workspace.getConfiguration())) { + try { + this.keyringStore.deleteToken(safeHostname); + this.output.info("Removed keyring token for", safeHostname); + } catch (error) { + this.output.warn("Failed to remove keyring token", error); + } + } + + const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); + const urlPath = this.pathResolver.getUrlPath(safeHostname); await Promise.all([ - this.updateUrlForCli(safeHostname, url), - this.updateTokenForCli(safeHostname, token), + fs.rm(tokenPath, { force: true }).catch((error) => { + this.output.warn("Failed to remove token file", tokenPath, error); + }), + fs.rm(urlPath, { force: true }).catch((error) => { + this.output.warn("Failed to remove URL file", urlPath, error); + }), ]); } /** - * Update the URL for the deployment with the provided hostname on disk which - * can be used by the CLI via --url-file. If the URL is falsey, do nothing. - * - * If the hostname is empty, read the old deployment-unaware config instead. + * Write the URL to the --global-config directory for the CLI. */ - private async updateUrlForCli( + private async writeUrlToGlobalConfig( safeHostname: string, - url: string | undefined, + url: string, ): Promise { - if (url) { - const urlPath = this.pathResolver.getUrlPath(safeHostname); - await this.atomicWriteFile(urlPath, url); - } + const urlPath = this.pathResolver.getUrlPath(safeHostname); + await this.atomicWriteFile(urlPath, url); } /** - * Update the session token for a deployment with the provided hostname on - * disk which can be used by the CLI via --session-token-file. If the token - * is null, do nothing. - * - * If the hostname is empty, read the old deployment-unaware config instead. + * Write the session token to the --global-config directory for the CLI. */ - private async updateTokenForCli(safeHostname: string, token: string | null) { - if (token !== null) { - const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); - await this.atomicWriteFile(tokenPath, token); - } + private async writeTokenToGlobalConfig(safeHostname: string, token: string) { + const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); + await this.atomicWriteFile(tokenPath, token); } /** @@ -761,7 +815,7 @@ export class CliManager { const tempPath = filePath + ".temp-" + Math.random().toString(36).substring(8); try { - await fs.writeFile(tempPath, content); + await fs.writeFile(tempPath, content, { mode: 0o600 }); await fs.rename(tempPath, filePath); } catch (err) { await fs.rm(tempPath, { force: true }).catch((rmErr) => { diff --git a/src/core/container.ts b/src/core/container.ts index 7ea0b76e..51413c2e 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -1,5 +1,6 @@ import * as vscode from "vscode"; +import { KeyringStore } from "../keyringStore"; import { type Logger } from "../logging/logger"; import { LoginCoordinator } from "../login/loginCoordinator"; @@ -18,6 +19,7 @@ export class ServiceContainer implements vscode.Disposable { private readonly pathResolver: PathResolver; private readonly mementoManager: MementoManager; private readonly secretsManager: SecretsManager; + private readonly keyringStore: KeyringStore; private readonly cliManager: CliManager; private readonly contextManager: ContextManager; private readonly loginCoordinator: LoginCoordinator; @@ -34,12 +36,18 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); - this.cliManager = new CliManager(this.logger, this.pathResolver); + this.keyringStore = new KeyringStore(this.logger); + this.cliManager = new CliManager( + this.logger, + this.pathResolver, + this.keyringStore, + ); this.contextManager = new ContextManager(context); this.loginCoordinator = new LoginCoordinator( this.secretsManager, this.mementoManager, this.logger, + this.keyringStore, context.extension.id, ); } @@ -68,6 +76,10 @@ export class ServiceContainer implements vscode.Disposable { return this.contextManager; } + getKeyringStore(): KeyringStore { + return this.keyringStore; + } + getLoginCoordinator(): LoginCoordinator { return this.loginCoordinator; } diff --git a/src/featureSet.ts b/src/featureSet.ts index d5f54452..7f66d846 100644 --- a/src/featureSet.ts +++ b/src/featureSet.ts @@ -5,6 +5,7 @@ export interface FeatureSet { proxyLogDirectory: boolean; wildcardSSH: boolean; buildReason: boolean; + keyringAuth: boolean; } /** @@ -35,5 +36,10 @@ export function featureSetForVersion( buildReason: (version?.compare("2.25.0") ?? 0) >= 0 || version?.prerelease[0] === "devel", + + // Keyring-backed token storage was added in CLI 2.29.0 + keyringAuth: + (version?.compare("2.29.0") ?? 0) >= 0 || + version?.prerelease[0] === "devel", }; } diff --git a/src/keyringStore.ts b/src/keyringStore.ts new file mode 100644 index 00000000..4d1942d0 --- /dev/null +++ b/src/keyringStore.ts @@ -0,0 +1,194 @@ +import type { Logger } from "./logging/logger"; + +/** A single entry in the CLI's keyring credential map. */ +interface CredentialEntry { + coder_url: string; + api_token: string; +} + +type CredentialMap = Record; + +/** Subset of @napi-rs/keyring Entry used for credential storage. */ +export interface KeyringEntry { + getPassword(): string | null; + setPassword(password: string): void; + getSecret(): Uint8Array | null; + setSecret(secret: Uint8Array): void; + deleteCredential(): void; +} + +const KEYRING_SERVICE = "coder-v2-credentials"; +const KEYRING_ACCOUNT = "coder-login-credentials"; + +function createDefaultEntry(): KeyringEntry { + // Lazy require so Linux never loads the native binary. + // eslint-disable-next-line @typescript-eslint/no-require-imports + const { Entry } = require("@napi-rs/keyring") as { + Entry: { + new (service: string, username: string): KeyringEntry; + withTarget( + target: string, + service: string, + username: string, + ): KeyringEntry; + }; + }; + + if (process.platform === "darwin") { + // On macOS, withTarget selects a keychain domain, not an attribute — using + // it creates a separate entry the CLI can't find. The two-arg constructor + // matches the CLI's kSecAttrService + kSecAttrAccount lookup. + return new Entry(KEYRING_SERVICE, KEYRING_ACCOUNT); + } + + // On Windows, withTarget sets the credential's lookup key, matching the CLI. + return Entry.withTarget(KEYRING_SERVICE, KEYRING_SERVICE, KEYRING_ACCOUNT); +} + +/** Extracts the host from a URL for use as credential map key (matches CLI format). */ +function toHost(deploymentUrl: string): string { + try { + return new URL(deploymentUrl).host; + } catch { + return deploymentUrl; + } +} + +/** + * Finds the map key matching a safeHostname. VS Code identifies deployments by + * safeHostname (port stripped), while the CLI stores map keys via `toHost` + * which preserves ports. The fallback strips ports from map keys so VS Code's + * port-less hostname still matches a CLI-written entry with a port. + */ +function findMapKey( + map: CredentialMap, + safeHostname: string, +): string | undefined { + if (safeHostname in map) { + return safeHostname; + } + for (const key of Object.keys(map)) { + const hostWithoutPort = key.replace(/:\d+$/, ""); + if (hostWithoutPort === safeHostname) { + return key; + } + } + return undefined; +} + +/** + * Returns true on platforms where the OS keyring is supported (macOS, Windows). + */ +export function isKeyringSupported(): boolean { + return process.platform === "darwin" || process.platform === "win32"; +} + +/** + * Wraps @napi-rs/keyring with the credential encoding the Coder CLI expects. + * The native module is loaded lazily so Linux never touches it. + * + * Encoding (must match CLI): + * macOS: base64-encoded JSON via setPassword/getPassword + * Windows: raw UTF-8 JSON bytes via setSecret/getSecret + * + * Concurrency: setToken does read-modify-write on a shared entry, so concurrent + * writes can clobber each other. Callers recover by re-writing on reconnection. + */ +export class KeyringStore { + public constructor( + private readonly logger: Logger, + private readonly entryFactory: () => KeyringEntry = createDefaultEntry, + ) {} + + /** + * Store a token under the host extracted from deploymentUrl (includes port). + * The CLI stores map keys as host+port, so we must write in the same format. + */ + public setToken(deploymentUrl: string, token: string): void { + this.assertSupported(); + const entry = this.entryFactory(); + const map = this.readMap(entry); + const host = toHost(deploymentUrl); + map[host] = { coder_url: host, api_token: token }; + this.writeMap(entry, map); + } + + /** + * Look up a token by safeHostname (hostname without port). VS Code identifies + * deployments by safeHostname, so findMapKey bridges to the CLI's host+port keys. + */ + public getToken(safeHostname: string): string | undefined { + this.assertSupported(); + const entry = this.entryFactory(); + const map = this.readMap(entry); + const key = findMapKey(map, safeHostname); + return key !== undefined ? map[key].api_token : undefined; + } + + /** Remove a token by safeHostname, matching the same way as getToken. */ + public deleteToken(safeHostname: string): void { + this.assertSupported(); + const entry = this.entryFactory(); + const map = this.readMap(entry); + const key = findMapKey(map, safeHostname); + if (key === undefined) { + return; + } + delete map[key]; + if (Object.keys(map).length === 0) { + try { + entry.deleteCredential(); + } catch { + // NoEntry is fine — nothing to delete + } + } else { + this.writeMap(entry, map); + } + } + + private assertSupported(): void { + if (!isKeyringSupported()) { + throw new Error(`Keyring is not supported on ${process.platform}`); + } + } + + private readMap(entry: KeyringEntry): CredentialMap { + try { + const raw = this.readRaw(entry); + if (!raw) { + return {}; + } + return JSON.parse(raw) as CredentialMap; + } catch (error) { + this.logger.warn("Failed to read keyring credential map", error); + return {}; + } + } + + private readRaw(entry: KeyringEntry): string | null { + if (process.platform === "darwin") { + const password = entry.getPassword(); + return password !== null + ? Buffer.from(password, "base64").toString("utf-8") + : null; + } + if (process.platform === "win32") { + const secret = entry.getSecret(); + return secret !== null ? Buffer.from(secret).toString("utf-8") : null; + } + throw new Error(`Keyring is not supported on ${process.platform}`); + } + + private writeMap(entry: KeyringEntry, map: CredentialMap): void { + const json = JSON.stringify(map); + if (process.platform === "darwin") { + entry.setPassword(Buffer.from(json).toString("base64")); + return; + } + if (process.platform === "win32") { + entry.setSecret(Buffer.from(json)); + return; + } + throw new Error(`Keyring is not supported on ${process.platform}`); + } +} diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index cc746df6..8f7b3213 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -4,6 +4,7 @@ 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"; @@ -15,6 +16,7 @@ import type { User } from "coder/site/src/api/typesGenerated"; import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; +import type { KeyringStore } from "../keyringStore"; import type { Logger } from "../logging/logger"; type LoginResult = @@ -39,6 +41,7 @@ export class LoginCoordinator implements vscode.Disposable { private readonly secretsManager: SecretsManager, private readonly mementoManager: MementoManager, private readonly logger: Logger, + private readonly keyringStore: KeyringStore, extensionId: string, ) { this.oauthAuthorizer = new OAuthAuthorizer( @@ -211,11 +214,13 @@ export class LoginCoordinator implements vscode.Disposable { // mTLS authentication (no token needed) if (!needToken(vscode.workspace.getConfiguration())) { + this.logger.debug("Attempting mTLS authentication (no token required)"); return this.tryMtlsAuth(client, isAutoLogin); } // Try provided token first if (providedToken) { + this.logger.debug("Trying provided token"); const result = await this.tryTokenAuth( client, providedToken, @@ -231,12 +236,27 @@ export class LoginCoordinator implements vscode.Disposable { deployment.safeHostname, ); if (auth?.token && auth.token !== providedToken) { + this.logger.debug("Trying stored session token"); const result = await this.tryTokenAuth(client, auth.token, isAutoLogin); if (result !== "unauthorized") { return result; } } + // Try keyring token (picks up tokens written by `coder login` in the terminal) + const keyringToken = this.getKeyringToken(deployment.safeHostname); + if ( + keyringToken && + keyringToken !== providedToken && + keyringToken !== auth?.token + ) { + this.logger.debug("Trying token from OS keyring"); + const result = await this.tryTokenAuth(client, keyringToken, isAutoLogin); + if (result !== "unauthorized") { + return result; + } + } + // Prompt user for token const authMethod = await maybeAskAuthMethod(client); switch (authMethod) { @@ -283,6 +303,18 @@ export class LoginCoordinator implements vscode.Disposable { } } + private getKeyringToken(safeHostname: string): string | undefined { + if (!isKeyringEnabled(vscode.workspace.getConfiguration())) { + return undefined; + } + try { + return this.keyringStore.getToken(safeHostname); + } catch (error) { + this.logger.warn("Failed to read token from keyring", error); + return undefined; + } + } + /** * Shows auth error via dialog or logs it for autoLogin. */ diff --git a/src/remote/remote.ts b/src/remote/remote.ts index c51f53db..0a659a19 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -21,7 +21,13 @@ import { extractAgents } from "../api/api-helper"; import { AuthInterceptor } from "../api/authInterceptor"; import { CoderApi } from "../api/coderApi"; import { needToken } from "../api/utils"; -import { getGlobalFlags, getGlobalFlagsRaw, getSshFlags } from "../cliConfig"; +import { + type CliAuth, + getGlobalFlags, + getGlobalFlagsRaw, + getSshFlags, + resolveCliAuth, +} from "../cliConfig"; import { type Commands } from "../commands"; import { watchConfigurationChanges } from "../configWatcher"; import { type CliManager } from "../core/cliManager"; @@ -119,11 +125,6 @@ export class Remote { hasUrl: Boolean(baseUrlRaw), hasToken: token !== undefined, }); - // Empty token is valid for mTLS - if (baseUrlRaw && token !== undefined) { - await this.cliManager.configure(parts.safeHostname, baseUrlRaw, token); - } - const disposables: vscode.Disposable[] = []; try { @@ -175,7 +176,7 @@ export class Remote { } }; - // It could be that the cli config was deleted. If so, ask for the url. + // It could be that the cli config was deleted. If so, ask for the url. if ( !baseUrlRaw || (!token && needToken(vscode.workspace.getConfiguration())) @@ -210,26 +211,6 @@ export class Remote { // Store for use in commands. this.commands.remoteWorkspaceClient = workspaceClient; - // Listen for token changes for this deployment - disposables.push( - this.secretsManager.onDidChangeSessionAuth( - parts.safeHostname, - async (auth) => { - workspaceClient.setCredentials(auth?.url, auth?.token); - if (auth?.url) { - await this.cliManager.configure( - parts.safeHostname, - auth.url, - auth.token, - ); - this.logger.info( - "Updated CLI config with new token for remote deployment", - ); - } - }, - ), - ); - let binaryPath: string | undefined; if ( this.extensionContext.extensionMode === vscode.ExtensionMode.Production @@ -264,6 +245,54 @@ export class Remote { const featureSet = featureSetForVersion(version); + // Write token to keyring or file (after CLI version is known) + if (baseUrlRaw && token !== undefined) { + await this.cliManager.configure( + parts.safeHostname, + baseUrlRaw, + token, + featureSet, + ); + } + + // Listen for token changes for this deployment + disposables.push( + this.secretsManager.onDidChangeSessionAuth( + parts.safeHostname, + async (auth) => { + workspaceClient.setCredentials(auth?.url, auth?.token); + if (auth?.url) { + try { + await this.cliManager.configure( + parts.safeHostname, + auth.url, + auth.token, + featureSet, + ); + this.logger.info( + "Updated CLI config with new token for remote deployment", + ); + } catch (error) { + this.logger.error( + "Failed to update CLI config for remote deployment", + error, + ); + } + } + }, + ), + ); + + const configDir = this.pathResolver.getGlobalConfigDir( + parts.safeHostname, + ); + const cliAuth = resolveCliAuth( + vscode.workspace.getConfiguration(), + featureSet, + baseUrlRaw, + configDir, + ); + // Server versions before v0.14.1 don't support the vscodessh command! if (!featureSet.vscodessh) { await vscodeProposed.window.showErrorMessage( @@ -361,7 +390,7 @@ export class Remote { binaryPath, featureSet, this.logger, - this.pathResolver, + cliAuth, ); disposables.push(stateMachine); @@ -541,6 +570,7 @@ export class Remote { binaryPath, logDir, featureSet, + cliAuth, ); } catch (error) { this.logger.warn("Failed to configure SSH", error); @@ -715,14 +745,12 @@ export class Remote { hostPrefix: string, logDir: string, useWildcardSSH: boolean, + cliAuth: CliAuth, ): Promise { const vscodeConfig = vscode.workspace.getConfiguration(); const escapedBinaryPath = escapeCommandArg(binaryPath); - const globalConfig = getGlobalFlags( - vscodeConfig, - this.pathResolver.getGlobalConfigDir(label), - ); + const globalConfig = getGlobalFlags(vscodeConfig, cliAuth); const logArgs = await this.getLogArgs(logDir); if (useWildcardSSH) { @@ -789,6 +817,7 @@ export class Remote { binaryPath: string, logDir: string, featureSet: FeatureSet, + cliAuth: CliAuth, ) { let deploymentSSHConfig = {}; try { @@ -845,6 +874,7 @@ export class Remote { hostPrefix, logDir, featureSet.wildcardSSH, + cliAuth, ); const sshValues: SSHValues = { diff --git a/src/remote/workspaceStateMachine.ts b/src/remote/workspaceStateMachine.ts index e188132d..26b8ffba 100644 --- a/src/remote/workspaceStateMachine.ts +++ b/src/remote/workspaceStateMachine.ts @@ -19,7 +19,7 @@ import type { import type * as vscode from "vscode"; import type { CoderApi } from "../api/coderApi"; -import type { PathResolver } from "../core/pathResolver"; +import type { CliAuth } from "../cliConfig"; import type { FeatureSet } from "../featureSet"; import type { Logger } from "../logging/logger"; @@ -41,7 +41,7 @@ export class WorkspaceStateMachine implements vscode.Disposable { private readonly binaryPath: string, private readonly featureSet: FeatureSet, private readonly logger: Logger, - private readonly pathResolver: PathResolver, + private readonly cliAuth: CliAuth, ) { this.terminal = new TerminalSession("Workspace Build"); } @@ -71,12 +71,9 @@ export class WorkspaceStateMachine implements vscode.Disposable { progress.report({ message: `starting ${workspaceName}...` }); this.logger.info(`Starting ${workspaceName}`); - const globalConfigDir = this.pathResolver.getGlobalConfigDir( - this.parts.safeHostname, - ); await startWorkspaceIfStoppedOrFailed( this.workspaceClient, - globalConfigDir, + this.cliAuth, this.binaryPath, workspace, this.terminal.writeEmitter, diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 6d1af77a..e8f97f42 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -12,6 +12,7 @@ import type { User } from "coder/site/src/api/typesGenerated"; import type { IncomingMessage } from "node:http"; import type { CoderApi } from "@/api/coderApi"; +import type { KeyringStore } from "@/keyringStore"; import type { Logger } from "@/logging/logger"; /** @@ -375,6 +376,14 @@ export class InMemorySecretStorage implements vscode.SecretStorage { } } +export function createMockKeyringStore(): KeyringStore { + return { + setToken: vi.fn(), + getToken: vi.fn().mockReturnValue(undefined), + deleteToken: vi.fn(), + } as unknown as KeyringStore; +} + export function createMockLogger(): Logger { return { trace: vi.fn(), diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index e35fa687..aaeb0fd5 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -1,29 +1,66 @@ -import { it, expect, describe } from "vitest"; +import * as semver from "semver"; +import { afterEach, it, expect, describe, vi } from "vitest"; -import { getGlobalFlags, getGlobalFlagsRaw, getSshFlags } from "@/cliConfig"; +import { + type CliAuth, + getGlobalFlags, + getGlobalFlagsRaw, + getSshFlags, + isKeyringEnabled, + resolveCliAuth, + shouldUseKeyring, +} from "@/cliConfig"; +import { featureSetForVersion } from "@/featureSet"; import { MockConfigurationProvider } from "../mocks/testHelpers"; import { isWindows } from "../utils/platform"; +const globalConfigAuth: CliAuth = { + mode: "global-config", + configDir: "/config/dir", +}; + describe("cliConfig", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + describe("getGlobalFlags", () => { - it("should return global-config and header args when no global flags configured", () => { - const config = new MockConfigurationProvider(); + const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; - expect(getGlobalFlags(config, "/config/dir")).toStrictEqual([ - "--global-config", - '"/config/dir"', - ]); - }); + interface AuthFlagsCase { + scenario: string; + auth: CliAuth; + expectedAuthFlags: string[]; + } - it("should return global flags from config with global-config appended", () => { + it.each([ + { + scenario: "global-config mode", + auth: globalConfigAuth, + expectedAuthFlags: ["--global-config", '"/config/dir"'], + }, + { + scenario: "url mode", + auth: urlAuth, + expectedAuthFlags: ["--url", '"https://dev.coder.com"'], + }, + ])( + "should return auth flags for $scenario", + ({ auth, expectedAuthFlags }) => { + const config = new MockConfigurationProvider(); + expect(getGlobalFlags(config, auth)).toStrictEqual(expectedAuthFlags); + }, + ); + + it("should return global flags from config with auth flags appended", () => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", [ "--verbose", "--disable-direct-connections", ]); - expect(getGlobalFlags(config, "/config/dir")).toStrictEqual([ + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ "--verbose", "--disable-direct-connections", "--global-config", @@ -39,7 +76,7 @@ describe("cliConfig", () => { "--disable-direct-connections", ]); - expect(getGlobalFlags(config, "/config/dir")).toStrictEqual([ + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ "-v", "--global-config /path/to/ignored", "--disable-direct-connections", @@ -48,27 +85,39 @@ describe("cliConfig", () => { ]); }); - it("should not filter header-command flags, header args appended at end", () => { - const headerCommand = "echo test"; - const config = new MockConfigurationProvider(); - config.set("coder.headerCommand", headerCommand); - config.set("coder.globalFlags", [ - "-v", - "--header-command custom", - "--no-feature-warning", - ]); + it.each([ + { + scenario: "global-config mode", + auth: globalConfigAuth, + expectedAuthFlags: ["--global-config", '"/config/dir"'], + }, + { + scenario: "url mode", + auth: urlAuth, + expectedAuthFlags: ["--url", '"https://dev.coder.com"'], + }, + ])( + "should not filter header-command flags ($scenario)", + ({ auth, expectedAuthFlags }) => { + const headerCommand = "echo test"; + const config = new MockConfigurationProvider(); + config.set("coder.headerCommand", headerCommand); + config.set("coder.globalFlags", [ + "-v", + "--header-command custom", + "--no-feature-warning", + ]); - const result = getGlobalFlags(config, "/config/dir"); - expect(result).toStrictEqual([ - "-v", - "--header-command custom", // ignored by CLI - "--no-feature-warning", - "--global-config", - '"/config/dir"', - "--header-command", - quoteCommand(headerCommand), - ]); - }); + expect(getGlobalFlags(config, auth)).toStrictEqual([ + "-v", + "--header-command custom", // ignored by CLI + "--no-feature-warning", + ...expectedAuthFlags, + "--header-command", + quoteCommand(headerCommand), + ]); + }, + ); }); describe("getGlobalFlagsRaw", () => { @@ -115,6 +164,119 @@ describe("cliConfig", () => { ]); }); }); + + describe("isKeyringEnabled", () => { + interface KeyringEnabledCase { + platform: NodeJS.Platform; + useKeyring: boolean; + expected: boolean; + } + it.each([ + { platform: "darwin", useKeyring: true, expected: true }, + { platform: "win32", useKeyring: true, expected: true }, + { platform: "linux", useKeyring: true, expected: false }, + { platform: "darwin", useKeyring: false, expected: false }, + ])( + "returns $expected on $platform with useKeyring=$useKeyring", + ({ platform, useKeyring, expected }) => { + vi.stubGlobal("process", { ...process, platform }); + const config = new MockConfigurationProvider(); + config.set("coder.useKeyring", useKeyring); + expect(isKeyringEnabled(config)).toBe(expected); + }, + ); + }); + + describe("shouldUseKeyring", () => { + interface ShouldUseKeyringCase { + platform: NodeJS.Platform; + useKeyring: boolean; + version: string; + expected: boolean; + } + it.each([ + { + platform: "darwin", + useKeyring: true, + version: "2.29.0", + expected: true, + }, + { + platform: "win32", + useKeyring: true, + version: "2.29.0", + expected: true, + }, + { + platform: "linux", + useKeyring: true, + version: "2.29.0", + expected: false, + }, + { + platform: "darwin", + useKeyring: true, + version: "2.28.0", + expected: false, + }, + { + platform: "darwin", + useKeyring: false, + version: "2.29.0", + expected: false, + }, + { + platform: "darwin", + useKeyring: true, + version: "0.0.0-devel+abc123", + expected: true, + }, + ])( + "returns $expected on $platform with useKeyring=$useKeyring and version $version", + ({ platform, useKeyring, version, expected }) => { + vi.stubGlobal("process", { ...process, platform }); + const config = new MockConfigurationProvider(); + config.set("coder.useKeyring", useKeyring); + const featureSet = featureSetForVersion(semver.parse(version)); + expect(shouldUseKeyring(config, featureSet)).toBe(expected); + }, + ); + }); + + describe("resolveCliAuth", () => { + it("returns url mode when keyring should be used", () => { + vi.stubGlobal("process", { ...process, platform: "darwin" }); + const config = new MockConfigurationProvider(); + config.set("coder.useKeyring", true); + const featureSet = featureSetForVersion(semver.parse("2.29.0")); + const auth = resolveCliAuth( + config, + featureSet, + "https://dev.coder.com", + "/config/dir", + ); + expect(auth).toEqual({ + mode: "url", + url: "https://dev.coder.com", + }); + }); + + it("returns global-config mode when keyring should not be used", () => { + vi.stubGlobal("process", { ...process, platform: "linux" }); + const config = new MockConfigurationProvider(); + const featureSet = featureSetForVersion(semver.parse("2.29.0")); + const auth = resolveCliAuth( + config, + featureSet, + "https://dev.coder.com", + "/config/dir", + ); + expect(auth).toEqual({ + mode: "global-config", + configDir: "/config/dir", + }); + }); + }); }); function quoteCommand(value: string): string { diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 96ca3529..5d32f0a2 100644 --- a/test/unit/core/cliManager.concurrent.test.ts +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -18,6 +18,7 @@ import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { + createMockKeyringStore, createMockLogger, createMockStream, MockConfigurationProvider, @@ -82,6 +83,7 @@ function setupManager(testDir: string): CliManager { return new CliManager( createMockLogger(), new PathResolver(testDir, "/code/log"), + createMockKeyringStore(), ); } diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 20151032..2d386244 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -9,12 +9,14 @@ import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; +import * as cliConfig from "@/cliConfig"; import { CliManager } from "@/core/cliManager"; import * as cliUtils from "@/core/cliUtils"; import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { + createMockKeyringStore, createMockLogger, createMockStream, MockConfigurationProvider, @@ -23,6 +25,9 @@ import { } from "../../mocks/testHelpers"; import { expectPathsEqual } from "../../utils/platform"; +import type { FeatureSet } from "@/featureSet"; +import type { KeyringStore } from "@/keyringStore"; + vi.mock("os"); vi.mock("axios"); @@ -48,6 +53,16 @@ vi.mock("proper-lockfile", () => ({ check: () => Promise.resolve(false), })); +vi.mock("@/cliConfig", async () => { + const actual = + await vi.importActual("@/cliConfig"); + return { + ...actual, + shouldUseKeyring: vi.fn(), + isKeyringEnabled: vi.fn(), + }; +}); + vi.mock("@/pgp"); vi.mock("@/vscodeProposed", () => ({ @@ -71,6 +86,7 @@ describe("CliManager", () => { let mockUI: MockUserInteraction; let mockApi: Api; let mockAxios: AxiosInstance; + let mockKeyring: KeyringStore; const TEST_VERSION = "1.2.3"; const TEST_URL = "https://test.coder.com"; @@ -80,6 +96,13 @@ describe("CliManager", () => { const ARCH = "amd64"; const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; const BINARY_PATH = `${BINARY_DIR}/${BINARY_NAME}`; + const MOCK_FEATURE_SET: FeatureSet = { + vscodessh: true, + proxyLogDirectory: true, + wildcardSSH: true, + buildReason: true, + keyringAuth: true, + }; beforeEach(() => { vi.resetAllMocks(); @@ -92,15 +115,18 @@ describe("CliManager", () => { mockConfig = new MockConfigurationProvider(); mockProgress = new MockProgressReporter(); mockUI = new MockUserInteraction(); + mockKeyring = createMockKeyringStore(); manager = new CliManager( createMockLogger(), new PathResolver(BASE_PATH, "/code/log"), + mockKeyring, ); // Mock only what's necessary vi.mocked(os.platform).mockReturnValue(PLATFORM); vi.mocked(os.arch).mockReturnValue(ARCH); vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); + vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(true); }); afterEach(async () => { @@ -112,59 +138,136 @@ describe("CliManager", () => { }); describe("Configure CLI", () => { - it("should write both url and token to correct paths", async () => { - await manager.configure( - "deployment", + function configure(token = "test-token") { + return manager.configure( + "dev.coder.com", "https://coder.example.com", - "test-token", + token, + MOCK_FEATURE_SET, ); + } + + it("should write both url and token to correct paths", async () => { + await configure("test-token"); - expect(memfs.readFileSync("/path/base/deployment/url", "utf8")).toBe( + expect(memfs.readFileSync("/path/base/dev.coder.com/url", "utf8")).toBe( "https://coder.example.com", ); - expect(memfs.readFileSync("/path/base/deployment/session", "utf8")).toBe( - "test-token", - ); + expect( + memfs.readFileSync("/path/base/dev.coder.com/session", "utf8"), + ).toBe("test-token"); }); - it("should skip URL when undefined but write token", async () => { - await manager.configure("deployment", undefined, "test-token"); + it("should throw when URL is empty", async () => { + await expect( + manager.configure("dev.coder.com", "", "test-token", MOCK_FEATURE_SET), + ).rejects.toThrow("URL is required to configure the CLI"); + }); - // No entry for the url - expect(memfs.existsSync("/path/base/deployment/url")).toBe(false); - expect(memfs.readFileSync("/path/base/deployment/session", "utf8")).toBe( - "test-token", + it("should write empty string for token when provided", async () => { + await configure(""); + + expect(memfs.readFileSync("/path/base/dev.coder.com/url", "utf8")).toBe( + "https://coder.example.com", ); + expect( + memfs.readFileSync("/path/base/dev.coder.com/session", "utf8"), + ).toBe(""); }); - it("should skip token when null but write URL", async () => { - await manager.configure("deployment", "https://coder.example.com", null); + it("should use base path directly when label is empty", async () => { + await manager.configure( + "", + "https://coder.example.com", + "token", + MOCK_FEATURE_SET, + ); - // No entry for the session - expect(memfs.readFileSync("/path/base/deployment/url", "utf8")).toBe( + expect(memfs.readFileSync("/path/base/url", "utf8")).toBe( "https://coder.example.com", ); - expect(memfs.existsSync("/path/base/deployment/session")).toBe(false); + expect(memfs.readFileSync("/path/base/session", "utf8")).toBe("token"); }); - it("should write empty string for token when provided", async () => { - await manager.configure("deployment", "https://coder.example.com", ""); + it("should write to keyring and skip files when featureSet enables keyring", async () => { + vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); + + await configure("test-token"); - expect(memfs.readFileSync("/path/base/deployment/url", "utf8")).toBe( + expect(mockKeyring.setToken).toHaveBeenCalledWith( "https://coder.example.com", + "test-token", ); - expect(memfs.readFileSync("/path/base/deployment/session", "utf8")).toBe( - "", - ); + expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); + expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); }); - it("should use base path directly when label is empty", async () => { - await manager.configure("", "https://coder.example.com", "token"); + it("should throw and show error when keyring write fails", async () => { + vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); + vi.mocked(mockKeyring.setToken).mockImplementation(() => { + throw new Error("keyring unavailable"); + }); - expect(memfs.readFileSync("/path/base/url", "utf8")).toBe( - "https://coder.example.com", + await expect(configure("test-token")).rejects.toThrow( + "keyring unavailable", ); - expect(memfs.readFileSync("/path/base/session", "utf8")).toBe("token"); + + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + expect.stringContaining("keyring unavailable"), + "Open Settings", + ); + expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); + expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + }); + }); + + describe("Clear Credentials", () => { + function seedCredentialFiles() { + memfs.mkdirSync("/path/base/dev.coder.com", { recursive: true }); + memfs.writeFileSync("/path/base/dev.coder.com/session", "old-token"); + memfs.writeFileSync( + "/path/base/dev.coder.com/url", + "https://example.com", + ); + } + + interface RemoveCredentialsCase { + scenario: string; + setup: () => void; + } + it.each([ + { scenario: "normally", setup: () => {} }, + { + scenario: "when keyring delete fails", + setup: () => + vi.mocked(mockKeyring.deleteToken).mockImplementation(() => { + throw new Error("keyring unavailable"); + }), + }, + { + scenario: "when keyring is disabled", + setup: () => + vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false), + }, + ])("should remove credential files $scenario", async ({ setup }) => { + seedCredentialFiles(); + setup(); + await manager.clearCredentials("dev.coder.com"); + expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); + }); + + it("should not throw when credential files don't exist", async () => { + await expect( + manager.clearCredentials("dev.coder.com"), + ).resolves.not.toThrow(); + expect(mockKeyring.deleteToken).toHaveBeenCalledWith("dev.coder.com"); + }); + + it("should skip keyring delete when keyring is disabled", async () => { + vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false); + await manager.clearCredentials("dev.coder.com"); + expect(mockKeyring.deleteToken).not.toHaveBeenCalled(); }); }); @@ -603,7 +706,11 @@ describe("CliManager", () => { it("handles binary with spaces in path", async () => { const pathWithSpaces = "/path with spaces/bin"; const resolver = new PathResolver(pathWithSpaces, "/log"); - const manager = new CliManager(createMockLogger(), resolver); + const manager = new CliManager( + createMockLogger(), + resolver, + createMockKeyringStore(), + ); withSuccessfulDownload(); const result = await manager.fetchBinary(mockApi, "test label"); diff --git a/test/unit/featureSet.test.ts b/test/unit/featureSet.test.ts index 919f7089..b8dfad08 100644 --- a/test/unit/featureSet.test.ts +++ b/test/unit/featureSet.test.ts @@ -28,4 +28,16 @@ describe("check version support", () => { }, ); }); + it("keyring auth", () => { + ["v2.28.0", "v2.28.9", "v1.0.0", "v2.3.3+e491217"].forEach((v: string) => { + expect(featureSetForVersion(semver.parse(v)).keyringAuth).toBeFalsy(); + }); + ["v2.29.0", "v2.29.1", "v2.30.0", "v3.0.0"].forEach((v: string) => { + expect(featureSetForVersion(semver.parse(v)).keyringAuth).toBeTruthy(); + }); + // devel prerelease should enable keyring + expect( + featureSetForVersion(semver.parse("0.0.0-devel+abc123")).keyringAuth, + ).toBeTruthy(); + }); }); diff --git a/test/unit/keyringStore.test.ts b/test/unit/keyringStore.test.ts new file mode 100644 index 00000000..f273fc00 --- /dev/null +++ b/test/unit/keyringStore.test.ts @@ -0,0 +1,231 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + type KeyringEntry, + KeyringStore, + isKeyringSupported, +} from "@/keyringStore"; + +import { createMockLogger } from "../mocks/testHelpers"; + +/** + * In-memory backing store that simulates the OS keyring. + * Each call to `factory()` returns a fresh handle pointing to the same + * shared state, matching real @napi-rs/keyring behavior where + * `Entry.withTarget()` returns a new handle to the same credential. + */ +function createMockEntryFactory() { + let password: string | null = null; + let secret: Uint8Array | null = null; + + return { + factory: (): KeyringEntry => ({ + getPassword: () => password, + setPassword: (p: string) => { + password = p; + }, + getSecret: () => secret, + setSecret: (s: Uint8Array) => { + secret = s; + }, + deleteCredential: () => { + password = null; + secret = null; + }, + }), + getRawPassword: () => password, + getRawSecret: () => secret, + hasCredential: () => password !== null || secret !== null, + }; +} + +function stubPlatform(platform: string) { + vi.stubGlobal("process", { ...process, platform }); +} + +function createTestContext() { + const mockEntry = createMockEntryFactory(); + const store = new KeyringStore(createMockLogger(), mockEntry.factory); + return { store, mockEntry }; +} + +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 }) => { + stubPlatform(platform); + expect(isKeyringSupported()).toBe(expected); + }); +}); + +describe("KeyringStore", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + // CRUD behavior is platform-independent; darwin is used as the test platform. + describe("token operations", () => { + it("sets and gets a token", () => { + const { store } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "my-token"); + expect(store.getToken("dev.coder.com")).toBe("my-token"); + }); + + it("overwrites token for same deployment", () => { + const { store } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "old-token"); + store.setToken("https://dev.coder.com", "new-token"); + expect(store.getToken("dev.coder.com")).toBe("new-token"); + }); + + it("preserves other deployments on set", () => { + const { store } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "token-1"); + store.setToken("https://staging.coder.com", "token-2"); + + expect(store.getToken("dev.coder.com")).toBe("token-1"); + expect(store.getToken("staging.coder.com")).toBe("token-2"); + }); + + it("returns undefined for missing deployment", () => { + const { store } = createTestContext(); + stubPlatform("darwin"); + expect(store.getToken("unknown.coder.com")).toBeUndefined(); + }); + + it("deletes token while preserving others", () => { + const { store } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "token-1"); + store.setToken("https://staging.coder.com", "token-2"); + + store.deleteToken("dev.coder.com"); + + expect(store.getToken("dev.coder.com")).toBeUndefined(); + expect(store.getToken("staging.coder.com")).toBe("token-2"); + }); + + it("deletes entire credential when last token is removed", () => { + const { store, mockEntry } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "token-1"); + store.deleteToken("dev.coder.com"); + expect(store.getToken("dev.coder.com")).toBeUndefined(); + // OS keyring entry itself should be cleaned up, not left as empty JSON + expect(mockEntry.hasCredential()).toBe(false); + }); + + it("handles delete of non-existent deployment gracefully", () => { + const { store } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "token-1"); + store.deleteToken("unknown.coder.com"); + expect(store.getToken("dev.coder.com")).toBe("token-1"); + }); + + it("strips URL path and protocol, keeping only host", () => { + const { store } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com/some/path", "my-token"); + expect(store.getToken("dev.coder.com")).toBe("my-token"); + }); + + it("matches safeHostname to map key with port", () => { + const { store, mockEntry } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com:3000", "my-token"); + // safeHostname (port stripped) finds the entry stored with port + expect(store.getToken("dev.coder.com")).toBe("my-token"); + // Used the hostname + port should also work + expect(store.getToken("dev.coder.com:3000")).toBe("my-token"); + + store.deleteToken("dev.coder.com"); + expect(store.getToken("dev.coder.com")).toBeUndefined(); + expect(mockEntry.hasCredential()).toBe(false); + }); + }); + + describe("platform encoding", () => { + it("macOS: base64-encoded JSON via password", () => { + const { store, mockEntry } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "token"); + const decoded = JSON.parse( + Buffer.from(mockEntry.getRawPassword()!, "base64").toString("utf-8"), + ); + expect(decoded).toEqual({ + "dev.coder.com": { coder_url: "dev.coder.com", api_token: "token" }, + }); + // Secret must be untouched; reading uses getPassword, not getSecret. + expect(mockEntry.getRawSecret()).toBeNull(); + expect(store.getToken("dev.coder.com")).toBe("token"); + }); + + it("macOS: returns undefined for corrupted credential", () => { + const { store, mockEntry } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com", "token"); + mockEntry + .factory() + .setPassword(Buffer.from("not-valid-json").toString("base64")); + expect(store.getToken("dev.coder.com")).toBeUndefined(); + }); + + it("Windows: raw UTF-8 JSON via secret", () => { + const { store, mockEntry } = createTestContext(); + stubPlatform("win32"); + store.setToken("https://dev.coder.com", "token"); + const decoded = JSON.parse( + Buffer.from(mockEntry.getRawSecret()!).toString("utf-8"), + ); + expect(decoded).toEqual({ + "dev.coder.com": { coder_url: "dev.coder.com", api_token: "token" }, + }); + // Password must be untouched; reading uses getSecret, not getPassword. + expect(mockEntry.getRawPassword()).toBeNull(); + expect(store.getToken("dev.coder.com")).toBe("token"); + }); + + it("Windows: returns undefined for corrupted credential", () => { + const { store, mockEntry } = createTestContext(); + stubPlatform("win32"); + store.setToken("https://dev.coder.com", "token"); + mockEntry.factory().setSecret(Buffer.from("not-valid-json")); + expect(store.getToken("dev.coder.com")).toBeUndefined(); + }); + + it("preserves port in map key for CLI compatibility", () => { + const { store, mockEntry } = createTestContext(); + stubPlatform("darwin"); + store.setToken("https://dev.coder.com:8080", "my-token"); + const decoded = JSON.parse( + Buffer.from(mockEntry.getRawPassword()!, "base64").toString("utf-8"), + ); + expect(decoded).toEqual({ + "dev.coder.com:8080": { + coder_url: "dev.coder.com:8080", + api_token: "my-token", + }, + }); + }); + + it("throws on unsupported platform", () => { + const { store } = createTestContext(); + stubPlatform("linux"); + const msg = "Keyring is not supported on linux"; + expect(() => store.setToken("https://dev.coder.com", "t")).toThrow(msg); + expect(() => store.getToken("dev.coder.com")).toThrow(msg); + expect(() => store.deleteToken("dev.coder.com")).toThrow(msg); + }); + }); +}); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 8b850a3b..7d7a345f 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -10,6 +10,7 @@ import { maybeAskAuthMethod } from "@/promptUtils"; import { createAxiosError, + createMockKeyringStore, createMockLogger, createMockUser, InMemoryMemento, @@ -121,6 +122,7 @@ function createTestContext() { secretsManager, mementoManager, logger, + createMockKeyringStore(), "coder.coder-remote", ); @@ -309,6 +311,7 @@ describe("LoginCoordinator", () => { secretsManager, mementoManager, logger, + createMockKeyringStore(), "coder.coder-remote", ); From ae781df34c57454613721f562526bfc4a2480c09 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 3 Mar 2026 19:43:30 +0300 Subject: [PATCH 2/6] feat: delegate keyring operations to CLI instead of native binaries Replace @napi-rs/keyring (4 native .node binaries) with CLI subprocess calls. The extension now runs `coder login --use-token-as-session` to store tokens and `coder login token --url` to read them, keeping the credential format in sync with the CLI automatically. - Add CliCredentialManager that shells out to the Coder CLI - Update CliManager.configure() to accept binPath and delegate to CLI - Update LoginCoordinator to fetch CLI binary for keyring reads - Remove clearCredentials keyring cleanup (stale entries are harmless) - Remove @napi-rs/keyring dep, vendor script, supportedArchitectures - Delete KeyringStore and its tests --- esbuild.mjs | 2 +- package.json | 3 +- pnpm-lock.yaml | 135 ----------- pnpm-workspace.yaml | 13 -- scripts/vendor-keyring.mjs | 61 ----- src/cliConfig.ts | 2 +- src/core/cliCredentialManager.ts | 76 ++++++ src/core/cliManager.ts | 31 ++- src/core/container.ts | 15 +- src/keyringStore.ts | 194 ---------------- src/login/loginCoordinator.ts | 51 +++- src/remote/remote.ts | 2 + test/mocks/testHelpers.ts | 11 +- test/unit/core/cliCredentialManager.test.ts | 211 +++++++++++++++++ test/unit/core/cliManager.concurrent.test.ts | 4 +- test/unit/core/cliManager.test.ts | 68 +++--- test/unit/keyringStore.test.ts | 231 ------------------- test/unit/login/loginCoordinator.test.ts | 14 +- 18 files changed, 404 insertions(+), 720 deletions(-) delete mode 100644 scripts/vendor-keyring.mjs create mode 100644 src/core/cliCredentialManager.ts delete mode 100644 src/keyringStore.ts create mode 100644 test/unit/core/cliCredentialManager.test.ts delete mode 100644 test/unit/keyringStore.test.ts diff --git a/esbuild.mjs b/esbuild.mjs index 9a0e89f3..54fb4a1d 100644 --- a/esbuild.mjs +++ b/esbuild.mjs @@ -32,7 +32,7 @@ const buildOptions = { // undefined when bundled to CJS, causing runtime errors. openpgp: "./node_modules/openpgp/dist/node/openpgp.min.cjs", }, - external: ["vscode", "@napi-rs/keyring"], + external: ["vscode"], sourcemap: production ? "external" : true, minify: production, plugins: watch ? [logRebuildPlugin] : [], diff --git a/package.json b/package.json index d6f39205..183722e7 100644 --- a/package.json +++ b/package.json @@ -32,7 +32,7 @@ "test:integration": "tsc -p test --outDir out --noCheck && node esbuild.mjs && vscode-test", "test:webview": "vitest --project webview", "typecheck": "concurrently -g \"tsc --noEmit\" \"tsc --noEmit -p test\"", - "vscode:prepublish": "pnpm build:production && node scripts/vendor-keyring.mjs", + "vscode:prepublish": "pnpm build:production", "watch": "concurrently -n extension,webviews \"pnpm watch:extension\" \"pnpm watch:webviews\"", "watch:extension": "node esbuild.mjs --watch", "watch:webviews": "pnpm -r --filter \"./packages/*\" --parallel dev" @@ -468,7 +468,6 @@ "word-wrap": "1.2.5" }, "dependencies": { - "@napi-rs/keyring": "^1.2.0", "@peculiar/x509": "^1.14.3", "@repo/shared": "workspace:*", "axios": "1.13.6", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 77808dae..9f685f62 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -58,9 +58,6 @@ importers: .: dependencies: - '@napi-rs/keyring': - specifier: ^1.2.0 - version: 1.2.0 '@peculiar/x509': specifier: ^1.14.3 version: 1.14.3 @@ -989,87 +986,6 @@ packages: '@lit/reactive-element@2.1.2': resolution: {integrity: sha512-pbCDiVMnne1lYUIaYNN5wrwQXDtHaYtg7YEFPeW+hws6U47WeFvISGUWekPGKWOP1ygrs0ef0o1VJMk1exos5A==} - '@napi-rs/keyring-darwin-arm64@1.2.0': - resolution: {integrity: sha512-CA83rDeyONDADO25JLZsh3eHY8yTEtm/RS6ecPsY+1v+dSawzT9GywBMu2r6uOp1IEhQs/xAfxgybGAFr17lSA==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [darwin] - - '@napi-rs/keyring-darwin-x64@1.2.0': - resolution: {integrity: sha512-dBHjtKRCj4ByfnfqIKIJLo3wueQNJhLRyuxtX/rR4K/XtcS7VLlRD01XXizjpre54vpmObj63w+ZpHG+mGM8uA==} - engines: {node: '>= 10'} - cpu: [x64] - os: [darwin] - - '@napi-rs/keyring-freebsd-x64@1.2.0': - resolution: {integrity: sha512-DPZFr11pNJSnaoh0dzSUNF+T6ORhy3CkzUT3uGixbA71cAOPJ24iG8e8QrLOkuC/StWrAku3gBnth2XMWOcR3Q==} - engines: {node: '>= 10'} - cpu: [x64] - os: [freebsd] - - '@napi-rs/keyring-linux-arm-gnueabihf@1.2.0': - resolution: {integrity: sha512-8xv6DyEMlvRdqJzp4F39RLUmmTQsLcGYYv/3eIfZNZN1O5257tHxTrFYqAsny659rJJK2EKeSa7PhrSibQqRWQ==} - engines: {node: '>= 10'} - cpu: [arm] - os: [linux] - - '@napi-rs/keyring-linux-arm64-gnu@1.2.0': - resolution: {integrity: sha512-Pu2V6Py+PBt7inryEecirl+t+ti8bhZphjP+W68iVaXHUxLdWmkgL9KI1VkbRHbx5k8K5Tew9OP218YfmVguIA==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [linux] - libc: [glibc] - - '@napi-rs/keyring-linux-arm64-musl@1.2.0': - resolution: {integrity: sha512-8TDymrpC4P1a9iDEaegT7RnrkmrJN5eNZh3Im3UEV5PPYGtrb82CRxsuFohthCWQW81O483u1bu+25+XA4nKUw==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [linux] - libc: [musl] - - '@napi-rs/keyring-linux-riscv64-gnu@1.2.0': - resolution: {integrity: sha512-awsB5XI1MYL7fwfjMDGmKOWvNgJEO7mM7iVEMS0fO39f0kVJnOSjlu7RHcXAF0LOx+0VfF3oxbWqJmZbvRCRHw==} - engines: {node: '>= 10'} - cpu: [riscv64] - os: [linux] - libc: [glibc] - - '@napi-rs/keyring-linux-x64-gnu@1.2.0': - resolution: {integrity: sha512-8E+7z4tbxSJXxIBqA+vfB1CGajpCDRyTyqXkBig5NtASrv4YXcntSo96Iah2QDR5zD3dSTsmbqJudcj9rKKuHQ==} - engines: {node: '>= 10'} - cpu: [x64] - os: [linux] - libc: [glibc] - - '@napi-rs/keyring-linux-x64-musl@1.2.0': - resolution: {integrity: sha512-8RZ8yVEnmWr/3BxKgBSzmgntI7lNEsY7xouNfOsQkuVAiCNmxzJwETspzK3PQ2FHtDxgz5vHQDEBVGMyM4hUHA==} - engines: {node: '>= 10'} - cpu: [x64] - os: [linux] - libc: [musl] - - '@napi-rs/keyring-win32-arm64-msvc@1.2.0': - resolution: {integrity: sha512-AoqaDZpQ6KPE19VBLpxyORcp+yWmHI9Xs9Oo0PJ4mfHma4nFSLVdhAubJCxdlNptHe5va7ghGCHj3L9Akiv4cQ==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [win32] - - '@napi-rs/keyring-win32-ia32-msvc@1.2.0': - resolution: {integrity: sha512-EYL+EEI6bCsYi3LfwcQdnX3P/R76ENKNn+3PmpGheBsUFLuh0gQuP7aMVHM4rTw6UVe+L3vCLZSptq/oeacz0A==} - engines: {node: '>= 10'} - cpu: [ia32] - os: [win32] - - '@napi-rs/keyring-win32-x64-msvc@1.2.0': - resolution: {integrity: sha512-xFlx/TsmqmCwNU9v+AVnEJgoEAlBYgzFF5Ihz1rMpPAt4qQWWkMd4sCyM1gMJ1A/GnRqRegDiQpwaxGUHFtFbA==} - engines: {node: '>= 10'} - cpu: [x64] - os: [win32] - - '@napi-rs/keyring@1.2.0': - resolution: {integrity: sha512-d0d4Oyxm+v980PEq1ZH2PmS6cvpMIRc17eYpiU47KgW+lzxklMu6+HOEOPmxrpnF/XQZ0+Q78I2mgMhbIIo/dg==} - engines: {node: '>= 10'} - '@napi-rs/wasm-runtime@0.2.12': resolution: {integrity: sha512-ZVWUcfwY4E/yPitQJl481FjFo3K22D6qF0DuFH6Y/nbnE11GY5uguDxZMGXPQ8WQ0128MXQD7TnfHyK4oWoIJQ==} @@ -5006,57 +4922,6 @@ snapshots: dependencies: '@lit-labs/ssr-dom-shim': 1.5.1 - '@napi-rs/keyring-darwin-arm64@1.2.0': - optional: true - - '@napi-rs/keyring-darwin-x64@1.2.0': - optional: true - - '@napi-rs/keyring-freebsd-x64@1.2.0': - optional: true - - '@napi-rs/keyring-linux-arm-gnueabihf@1.2.0': - optional: true - - '@napi-rs/keyring-linux-arm64-gnu@1.2.0': - optional: true - - '@napi-rs/keyring-linux-arm64-musl@1.2.0': - optional: true - - '@napi-rs/keyring-linux-riscv64-gnu@1.2.0': - optional: true - - '@napi-rs/keyring-linux-x64-gnu@1.2.0': - optional: true - - '@napi-rs/keyring-linux-x64-musl@1.2.0': - optional: true - - '@napi-rs/keyring-win32-arm64-msvc@1.2.0': - optional: true - - '@napi-rs/keyring-win32-ia32-msvc@1.2.0': - optional: true - - '@napi-rs/keyring-win32-x64-msvc@1.2.0': - optional: true - - '@napi-rs/keyring@1.2.0': - optionalDependencies: - '@napi-rs/keyring-darwin-arm64': 1.2.0 - '@napi-rs/keyring-darwin-x64': 1.2.0 - '@napi-rs/keyring-freebsd-x64': 1.2.0 - '@napi-rs/keyring-linux-arm-gnueabihf': 1.2.0 - '@napi-rs/keyring-linux-arm64-gnu': 1.2.0 - '@napi-rs/keyring-linux-arm64-musl': 1.2.0 - '@napi-rs/keyring-linux-riscv64-gnu': 1.2.0 - '@napi-rs/keyring-linux-x64-gnu': 1.2.0 - '@napi-rs/keyring-linux-x64-musl': 1.2.0 - '@napi-rs/keyring-win32-arm64-msvc': 1.2.0 - '@napi-rs/keyring-win32-ia32-msvc': 1.2.0 - '@napi-rs/keyring-win32-x64-msvc': 1.2.0 - '@napi-rs/wasm-runtime@0.2.12': dependencies: '@emnapi/core': 1.7.1 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 613661d2..70f3d76e 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -27,16 +27,3 @@ onlyBuiltDependencies: - keytar - unrs-resolver - utf-8-validate - -# Install @napi-rs/keyring native binaries for macOS and Windows so they're -# available when building the universal VSIX (even on Linux CI). -# Only macOS and Windows use the keyring; Linux falls back to file storage. -supportedArchitectures: - os: - - current - - darwin - - win32 - cpu: - - current - - x64 - - arm64 diff --git a/scripts/vendor-keyring.mjs b/scripts/vendor-keyring.mjs deleted file mode 100644 index 5cf8cd31..00000000 --- a/scripts/vendor-keyring.mjs +++ /dev/null @@ -1,61 +0,0 @@ -/** - * Vendor @napi-rs/keyring into dist/node_modules/ for VSIX packaging. - * - * pnpm uses symlinks that vsce can't follow. This script resolves them and - * copies the JS wrapper plus macOS/Windows .node binaries into dist/, where - * Node's require() resolution finds them from dist/extension.js. - */ -import { - cpSync, - existsSync, - mkdirSync, - readdirSync, - realpathSync, - rmSync, -} from "node:fs"; -import { join, resolve } from "node:path"; - -const keyringPkg = resolve("node_modules/@napi-rs/keyring"); -const outputDir = resolve("dist/node_modules/@napi-rs/keyring"); - -if (!existsSync(keyringPkg)) { - console.error("@napi-rs/keyring not found — run pnpm install first"); - process.exit(1); -} - -// Copy the JS wrapper package (resolving pnpm symlinks) -const resolvedPkg = realpathSync(keyringPkg); -rmSync(outputDir, { recursive: true, force: true }); -mkdirSync(outputDir, { recursive: true }); -cpSync(resolvedPkg, outputDir, { recursive: true }); - -// Native binary packages live as siblings of the resolved keyring package in -// pnpm's content-addressable store (they aren't hoisted to node_modules). -const siblingsDir = resolve(resolvedPkg, ".."); -const nativePackages = [ - "keyring-darwin-arm64", - "keyring-darwin-x64", - "keyring-win32-arm64-msvc", - "keyring-win32-x64-msvc", -]; - -for (const pkg of nativePackages) { - const pkgDir = join(siblingsDir, pkg); - if (!existsSync(pkgDir)) { - console.error( - `Missing native package: ${pkg}\n` + - "Ensure supportedArchitectures in pnpm-workspace.yaml includes all target platforms.", - ); - process.exit(1); - } - const nodeFile = readdirSync(pkgDir).find((f) => f.endsWith(".node")); - if (!nodeFile) { - console.error(`No .node binary found in ${pkg}`); - process.exit(1); - } - cpSync(join(pkgDir, nodeFile), join(outputDir, nodeFile)); -} - -console.log( - `Vendored @napi-rs/keyring with ${nativePackages.length} platform binaries into dist/`, -); diff --git a/src/cliConfig.ts b/src/cliConfig.ts index 6831117e..7461651e 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -1,5 +1,5 @@ +import { isKeyringSupported } from "./core/cliCredentialManager"; import { getHeaderArgs } from "./headers"; -import { isKeyringSupported } from "./keyringStore"; import { escapeCommandArg } from "./util"; import type { WorkspaceConfiguration } from "vscode"; diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts new file mode 100644 index 00000000..dcd8f3b5 --- /dev/null +++ b/src/core/cliCredentialManager.ts @@ -0,0 +1,76 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +import { getHeaderArgs } from "../headers"; + +import type { WorkspaceConfiguration } from "vscode"; + +import type { Logger } from "../logging/logger"; + +const execFileAsync = promisify(execFile); + +/** + * Returns true on platforms where the OS keyring is supported (macOS, Windows). + */ +export function isKeyringSupported(): boolean { + return process.platform === "darwin" || process.platform === "win32"; +} + +/** + * Delegates credential storage to the Coder CLI to keep the credentials in sync. + */ +export class CliCredentialManager { + constructor(private readonly logger: Logger) {} + + /** + * Store a token by running: + * CODER_SESSION_TOKEN= login --use-token-as-session + * + * The token is passed via environment variable so it never appears in + * process argument lists. + */ + async storeToken( + binPath: string, + url: string, + token: string, + configs: Pick, + ): Promise { + const args = [ + ...getHeaderArgs(configs), + "login", + "--use-token-as-session", + url, + ]; + try { + await execFileAsync(binPath, args, { + env: { ...process.env, CODER_SESSION_TOKEN: token }, + }); + this.logger.info("Stored token via CLI for", url); + } catch (error) { + this.logger.error("Failed to store token via CLI:", error); + throw error; + } + } + + /** + * Read a token by running: + * login token --url + * + * Returns trimmed stdout, or undefined on any failure. + */ + async readToken( + binPath: string, + url: string, + configs: Pick, + ): Promise { + const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; + try { + const { stdout } = await execFileAsync(binPath, args); + const token = stdout.trim(); + return token || undefined; + } catch (error) { + this.logger.warn("Failed to read token via CLI:", error); + return undefined; + } + } +} diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 6f918eb0..474702b3 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,7 +10,7 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig"; +import { shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; import { vscodeProposed } from "../vscodeProposed"; @@ -22,9 +22,9 @@ import type { Api } from "coder/site/src/api/api"; import type { IncomingMessage } from "node:http"; import type { FeatureSet } from "../featureSet"; -import type { KeyringStore } from "../keyringStore"; import type { Logger } from "../logging/logger"; +import type { CliCredentialManager } from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; export class CliManager { @@ -33,7 +33,7 @@ export class CliManager { constructor( private readonly output: Logger, private readonly pathResolver: PathResolver, - private readonly keyringStore: KeyringStore, + private readonly cliCredentialManager: CliCredentialManager, ) { this.binaryLock = new BinaryLock(output); } @@ -723,6 +723,7 @@ export class CliManager { url: string, token: string, featureSet: FeatureSet, + binPath: string, ) { if (!url) { throw new Error("URL is required to configure the CLI"); @@ -731,10 +732,14 @@ export class CliManager { const configs = vscode.workspace.getConfiguration(); if (shouldUseKeyring(configs, featureSet)) { try { - this.keyringStore.setToken(url, token); - this.output.info("Stored token in OS keyring for", url); + await this.cliCredentialManager.storeToken( + binPath, + url, + token, + configs, + ); } catch (error) { - this.output.error("Failed to store token in OS keyring:", error); + this.output.error("Failed to store token via CLI keyring:", error); vscode.window .showErrorMessage( `Failed to store session token in OS keyring: ${errToStr(error)}. ` + @@ -760,18 +765,12 @@ export class CliManager { } /** - * Remove credentials for a deployment from both keyring and file storage. + * Remove file-based credentials for a deployment. Keyring entries are not + * removed here because deleting requires the CLI binary, which may not be + * available at logout time. This is fine: stale keyring entries are harmless + * since the CLI overwrites them on next `coder login`. */ public async clearCredentials(safeHostname: string): Promise { - if (isKeyringEnabled(vscode.workspace.getConfiguration())) { - try { - this.keyringStore.deleteToken(safeHostname); - this.output.info("Removed keyring token for", safeHostname); - } catch (error) { - this.output.warn("Failed to remove keyring token", error); - } - } - const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); const urlPath = this.pathResolver.getUrlPath(safeHostname); await Promise.all([ diff --git a/src/core/container.ts b/src/core/container.ts index 51413c2e..5b7c5953 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -1,9 +1,9 @@ import * as vscode from "vscode"; -import { KeyringStore } from "../keyringStore"; import { type Logger } from "../logging/logger"; import { LoginCoordinator } from "../login/loginCoordinator"; +import { CliCredentialManager } from "./cliCredentialManager"; import { CliManager } from "./cliManager"; import { ContextManager } from "./contextManager"; import { MementoManager } from "./mementoManager"; @@ -19,7 +19,7 @@ export class ServiceContainer implements vscode.Disposable { private readonly pathResolver: PathResolver; private readonly mementoManager: MementoManager; private readonly secretsManager: SecretsManager; - private readonly keyringStore: KeyringStore; + private readonly cliCredentialManager: CliCredentialManager; private readonly cliManager: CliManager; private readonly contextManager: ContextManager; private readonly loginCoordinator: LoginCoordinator; @@ -36,18 +36,19 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); - this.keyringStore = new KeyringStore(this.logger); + this.cliCredentialManager = new CliCredentialManager(this.logger); this.cliManager = new CliManager( this.logger, this.pathResolver, - this.keyringStore, + this.cliCredentialManager, ); this.contextManager = new ContextManager(context); this.loginCoordinator = new LoginCoordinator( this.secretsManager, this.mementoManager, this.logger, - this.keyringStore, + this.cliCredentialManager, + this.cliManager, context.extension.id, ); } @@ -76,8 +77,8 @@ export class ServiceContainer implements vscode.Disposable { return this.contextManager; } - getKeyringStore(): KeyringStore { - return this.keyringStore; + getCliCredentialManager(): CliCredentialManager { + return this.cliCredentialManager; } getLoginCoordinator(): LoginCoordinator { diff --git a/src/keyringStore.ts b/src/keyringStore.ts deleted file mode 100644 index 4d1942d0..00000000 --- a/src/keyringStore.ts +++ /dev/null @@ -1,194 +0,0 @@ -import type { Logger } from "./logging/logger"; - -/** A single entry in the CLI's keyring credential map. */ -interface CredentialEntry { - coder_url: string; - api_token: string; -} - -type CredentialMap = Record; - -/** Subset of @napi-rs/keyring Entry used for credential storage. */ -export interface KeyringEntry { - getPassword(): string | null; - setPassword(password: string): void; - getSecret(): Uint8Array | null; - setSecret(secret: Uint8Array): void; - deleteCredential(): void; -} - -const KEYRING_SERVICE = "coder-v2-credentials"; -const KEYRING_ACCOUNT = "coder-login-credentials"; - -function createDefaultEntry(): KeyringEntry { - // Lazy require so Linux never loads the native binary. - // eslint-disable-next-line @typescript-eslint/no-require-imports - const { Entry } = require("@napi-rs/keyring") as { - Entry: { - new (service: string, username: string): KeyringEntry; - withTarget( - target: string, - service: string, - username: string, - ): KeyringEntry; - }; - }; - - if (process.platform === "darwin") { - // On macOS, withTarget selects a keychain domain, not an attribute — using - // it creates a separate entry the CLI can't find. The two-arg constructor - // matches the CLI's kSecAttrService + kSecAttrAccount lookup. - return new Entry(KEYRING_SERVICE, KEYRING_ACCOUNT); - } - - // On Windows, withTarget sets the credential's lookup key, matching the CLI. - return Entry.withTarget(KEYRING_SERVICE, KEYRING_SERVICE, KEYRING_ACCOUNT); -} - -/** Extracts the host from a URL for use as credential map key (matches CLI format). */ -function toHost(deploymentUrl: string): string { - try { - return new URL(deploymentUrl).host; - } catch { - return deploymentUrl; - } -} - -/** - * Finds the map key matching a safeHostname. VS Code identifies deployments by - * safeHostname (port stripped), while the CLI stores map keys via `toHost` - * which preserves ports. The fallback strips ports from map keys so VS Code's - * port-less hostname still matches a CLI-written entry with a port. - */ -function findMapKey( - map: CredentialMap, - safeHostname: string, -): string | undefined { - if (safeHostname in map) { - return safeHostname; - } - for (const key of Object.keys(map)) { - const hostWithoutPort = key.replace(/:\d+$/, ""); - if (hostWithoutPort === safeHostname) { - return key; - } - } - return undefined; -} - -/** - * Returns true on platforms where the OS keyring is supported (macOS, Windows). - */ -export function isKeyringSupported(): boolean { - return process.platform === "darwin" || process.platform === "win32"; -} - -/** - * Wraps @napi-rs/keyring with the credential encoding the Coder CLI expects. - * The native module is loaded lazily so Linux never touches it. - * - * Encoding (must match CLI): - * macOS: base64-encoded JSON via setPassword/getPassword - * Windows: raw UTF-8 JSON bytes via setSecret/getSecret - * - * Concurrency: setToken does read-modify-write on a shared entry, so concurrent - * writes can clobber each other. Callers recover by re-writing on reconnection. - */ -export class KeyringStore { - public constructor( - private readonly logger: Logger, - private readonly entryFactory: () => KeyringEntry = createDefaultEntry, - ) {} - - /** - * Store a token under the host extracted from deploymentUrl (includes port). - * The CLI stores map keys as host+port, so we must write in the same format. - */ - public setToken(deploymentUrl: string, token: string): void { - this.assertSupported(); - const entry = this.entryFactory(); - const map = this.readMap(entry); - const host = toHost(deploymentUrl); - map[host] = { coder_url: host, api_token: token }; - this.writeMap(entry, map); - } - - /** - * Look up a token by safeHostname (hostname without port). VS Code identifies - * deployments by safeHostname, so findMapKey bridges to the CLI's host+port keys. - */ - public getToken(safeHostname: string): string | undefined { - this.assertSupported(); - const entry = this.entryFactory(); - const map = this.readMap(entry); - const key = findMapKey(map, safeHostname); - return key !== undefined ? map[key].api_token : undefined; - } - - /** Remove a token by safeHostname, matching the same way as getToken. */ - public deleteToken(safeHostname: string): void { - this.assertSupported(); - const entry = this.entryFactory(); - const map = this.readMap(entry); - const key = findMapKey(map, safeHostname); - if (key === undefined) { - return; - } - delete map[key]; - if (Object.keys(map).length === 0) { - try { - entry.deleteCredential(); - } catch { - // NoEntry is fine — nothing to delete - } - } else { - this.writeMap(entry, map); - } - } - - private assertSupported(): void { - if (!isKeyringSupported()) { - throw new Error(`Keyring is not supported on ${process.platform}`); - } - } - - private readMap(entry: KeyringEntry): CredentialMap { - try { - const raw = this.readRaw(entry); - if (!raw) { - return {}; - } - return JSON.parse(raw) as CredentialMap; - } catch (error) { - this.logger.warn("Failed to read keyring credential map", error); - return {}; - } - } - - private readRaw(entry: KeyringEntry): string | null { - if (process.platform === "darwin") { - const password = entry.getPassword(); - return password !== null - ? Buffer.from(password, "base64").toString("utf-8") - : null; - } - if (process.platform === "win32") { - const secret = entry.getSecret(); - return secret !== null ? Buffer.from(secret).toString("utf-8") : null; - } - throw new Error(`Keyring is not supported on ${process.platform}`); - } - - private writeMap(entry: KeyringEntry, map: CredentialMap): void { - const json = JSON.stringify(map); - if (process.platform === "darwin") { - entry.setPassword(Buffer.from(json).toString("base64")); - return; - } - if (process.platform === "win32") { - entry.setSecret(Buffer.from(json)); - return; - } - throw new Error(`Keyring is not supported on ${process.platform}`); - } -} diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 8f7b3213..1a1a7e38 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -13,10 +13,11 @@ import { vscodeProposed } from "../vscodeProposed"; import type { User } from "coder/site/src/api/typesGenerated"; +import type { CliCredentialManager } from "../core/cliCredentialManager"; +import type { CliManager } from "../core/cliManager"; import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; -import type { KeyringStore } from "../keyringStore"; import type { Logger } from "../logging/logger"; type LoginResult = @@ -41,7 +42,8 @@ export class LoginCoordinator implements vscode.Disposable { private readonly secretsManager: SecretsManager, private readonly mementoManager: MementoManager, private readonly logger: Logger, - private readonly keyringStore: KeyringStore, + private readonly cliCredentialManager: CliCredentialManager, + private readonly cliManager: CliManager, extensionId: string, ) { this.oauthAuthorizer = new OAuthAuthorizer( @@ -244,7 +246,7 @@ export class LoginCoordinator implements vscode.Disposable { } // Try keyring token (picks up tokens written by `coder login` in the terminal) - const keyringToken = this.getKeyringToken(deployment.safeHostname); + const keyringToken = await this.getCliKeyringToken(deployment); if ( keyringToken && keyringToken !== providedToken && @@ -303,14 +305,51 @@ export class LoginCoordinator implements vscode.Disposable { } } - private getKeyringToken(safeHostname: string): string | undefined { + /** + * Read a token from the CLI keyring. Fetches the CLI binary first (using + * an unauthenticated client) so the binary is available for keyring reads. + * Returns undefined if the keyring is disabled, the binary can't be fetched, + * or the CLI returns no token. + */ + private async getCliKeyringToken( + deployment: Deployment, + ): Promise { if (!isKeyringEnabled(vscode.workspace.getConfiguration())) { return undefined; } try { - return this.keyringStore.getToken(safeHostname); + const binPath = await this.ensureBinaryForKeyring( + deployment.url, + deployment.safeHostname, + ); + if (!binPath) { + return undefined; + } + return await this.cliCredentialManager.readToken( + binPath, + deployment.url, + vscode.workspace.getConfiguration(), + ); + } catch (error) { + this.logger.warn("Failed to read token from CLI keyring", error); + return undefined; + } + } + + /** + * Fetch or locate a CLI binary for the given deployment. Uses an + * unauthenticated client since getBuildInfo and binary downloads + * don't require auth. + */ + private async ensureBinaryForKeyring( + url: string, + safeHostname: string, + ): Promise { + try { + const client = CoderApi.create(url, "", this.logger); + return await this.cliManager.fetchBinary(client, safeHostname); } catch (error) { - this.logger.warn("Failed to read token from keyring", error); + this.logger.warn("Could not fetch CLI binary for keyring read:", error); return undefined; } } diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 0a659a19..87fe724d 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -252,6 +252,7 @@ export class Remote { baseUrlRaw, token, featureSet, + binaryPath, ); } @@ -268,6 +269,7 @@ export class Remote { auth.url, auth.token, featureSet, + binaryPath, ); this.logger.info( "Updated CLI config with new token for remote deployment", diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index e8f97f42..efe649cb 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -12,7 +12,7 @@ import type { User } from "coder/site/src/api/typesGenerated"; import type { IncomingMessage } from "node:http"; import type { CoderApi } from "@/api/coderApi"; -import type { KeyringStore } from "@/keyringStore"; +import type { CliCredentialManager } from "@/core/cliCredentialManager"; import type { Logger } from "@/logging/logger"; /** @@ -376,12 +376,11 @@ export class InMemorySecretStorage implements vscode.SecretStorage { } } -export function createMockKeyringStore(): KeyringStore { +export function createMockCliCredentialManager(): CliCredentialManager { return { - setToken: vi.fn(), - getToken: vi.fn().mockReturnValue(undefined), - deleteToken: vi.fn(), - } as unknown as KeyringStore; + storeToken: vi.fn().mockResolvedValue(undefined), + readToken: vi.fn().mockResolvedValue(undefined), + } as unknown as CliCredentialManager; } export function createMockLogger(): Logger { diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts new file mode 100644 index 00000000..cecfa97b --- /dev/null +++ b/test/unit/core/cliCredentialManager.test.ts @@ -0,0 +1,211 @@ +import { execFile } from "node:child_process"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + CliCredentialManager, + isKeyringSupported, +} from "@/core/cliCredentialManager"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +vi.mock("node:child_process", () => ({ + execFile: vi.fn(), +})); + +function stubPlatform(platform: string) { + vi.stubGlobal("process", { ...process, platform }); +} + +function mockExecFileSuccess(stdout = "") { + vi.mocked(execFile).mockImplementation( + (_cmd, _args, _opts, callback?: unknown) => { + // promisify(execFile) calls execFile with a callback as last argument + const cb = + typeof _opts === "function" + ? (_opts as (err: Error | null, result: { stdout: string }) => void) + : (callback as + | ((err: Error | null, result: { stdout: string }) => void) + | undefined); + if (cb) { + cb(null, { stdout }); + } + return {} as ReturnType; + }, + ); +} + +function mockExecFileFailure(message: string) { + vi.mocked(execFile).mockImplementation( + (_cmd, _args, _opts, callback?: unknown) => { + const cb = + typeof _opts === "function" + ? (_opts as (err: Error | null) => void) + : (callback as ((err: Error | null) => void) | undefined); + if (cb) { + cb(new Error(message)); + } + return {} as ReturnType; + }, + ); +} + +const mockConfigs = { + get: vi.fn().mockReturnValue(undefined), +}; + +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 }) => { + stubPlatform(platform); + expect(isKeyringSupported()).toBe(expected); + }); +}); + +describe("CliCredentialManager", () => { + afterEach(() => { + vi.resetAllMocks(); + }); + + describe("storeToken", () => { + it("passes token via CODER_SESSION_TOKEN env var", async () => { + mockExecFileSuccess(); + const manager = new CliCredentialManager(createMockLogger()); + + await manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "my-secret-token", + mockConfigs, + ); + + expect(execFile).toHaveBeenCalledWith( + "/usr/bin/coder", + ["login", "--use-token-as-session", "https://dev.coder.com"], + expect.objectContaining({ + env: expect.objectContaining({ + CODER_SESSION_TOKEN: "my-secret-token", + }), + }), + expect.any(Function), + ); + }); + + it("never passes token in args", async () => { + mockExecFileSuccess(); + const manager = new CliCredentialManager(createMockLogger()); + + await manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "my-secret-token", + mockConfigs, + ); + + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).not.toContain("my-secret-token"); + }); + + it("throws when CLI fails", async () => { + mockExecFileFailure("login failed"); + const manager = new CliCredentialManager(createMockLogger()); + + await expect( + manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "token", + mockConfigs, + ), + ).rejects.toThrow("login failed"); + }); + + it("includes header args when header command is set", async () => { + mockExecFileSuccess(); + const configWithHeaders = { + get: vi.fn((key: string) => + key === "coder.headerCommand" ? "my-header-cmd" : undefined, + ), + }; + const manager = new CliCredentialManager(createMockLogger()); + + await manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "token", + configWithHeaders, + ); + + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).toContain("--header-command"); + }); + }); + + describe("readToken", () => { + it("returns trimmed stdout", async () => { + mockExecFileSuccess(" my-token\n"); + const manager = new CliCredentialManager(createMockLogger()); + + const token = await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + expect(token).toBe("my-token"); + }); + + it("returns undefined on empty stdout", async () => { + mockExecFileSuccess(" \n"); + const manager = new CliCredentialManager(createMockLogger()); + + const token = await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + expect(token).toBeUndefined(); + }); + + it("returns undefined on error", async () => { + mockExecFileFailure("no token found"); + const manager = new CliCredentialManager(createMockLogger()); + + const token = await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + expect(token).toBeUndefined(); + }); + + it("passes correct args", async () => { + mockExecFileSuccess("token"); + const manager = new CliCredentialManager(createMockLogger()); + + await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + const call = vi.mocked(execFile).mock.calls[0]; + expect(call[0]).toBe("/usr/bin/coder"); + expect(call[1]).toEqual([ + "login", + "token", + "--url", + "https://dev.coder.com", + ]); + }); + }); +}); diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 5d32f0a2..0a6cd804 100644 --- a/test/unit/core/cliManager.concurrent.test.ts +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -18,7 +18,7 @@ import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { - createMockKeyringStore, + createMockCliCredentialManager, createMockLogger, createMockStream, MockConfigurationProvider, @@ -83,7 +83,7 @@ function setupManager(testDir: string): CliManager { return new CliManager( createMockLogger(), new PathResolver(testDir, "/code/log"), - createMockKeyringStore(), + createMockCliCredentialManager(), ); } diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 2d386244..1d9dac2f 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -16,7 +16,7 @@ import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { - createMockKeyringStore, + createMockCliCredentialManager, createMockLogger, createMockStream, MockConfigurationProvider, @@ -25,8 +25,8 @@ import { } from "../../mocks/testHelpers"; import { expectPathsEqual } from "../../utils/platform"; +import type { CliCredentialManager } from "@/core/cliCredentialManager"; import type { FeatureSet } from "@/featureSet"; -import type { KeyringStore } from "@/keyringStore"; vi.mock("os"); vi.mock("axios"); @@ -59,7 +59,6 @@ vi.mock("@/cliConfig", async () => { return { ...actual, shouldUseKeyring: vi.fn(), - isKeyringEnabled: vi.fn(), }; }); @@ -86,7 +85,7 @@ describe("CliManager", () => { let mockUI: MockUserInteraction; let mockApi: Api; let mockAxios: AxiosInstance; - let mockKeyring: KeyringStore; + let mockCredManager: CliCredentialManager; const TEST_VERSION = "1.2.3"; const TEST_URL = "https://test.coder.com"; @@ -115,18 +114,17 @@ describe("CliManager", () => { mockConfig = new MockConfigurationProvider(); mockProgress = new MockProgressReporter(); mockUI = new MockUserInteraction(); - mockKeyring = createMockKeyringStore(); + mockCredManager = createMockCliCredentialManager(); manager = new CliManager( createMockLogger(), new PathResolver(BASE_PATH, "/code/log"), - mockKeyring, + mockCredManager, ); // Mock only what's necessary vi.mocked(os.platform).mockReturnValue(PLATFORM); vi.mocked(os.arch).mockReturnValue(ARCH); vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(true); }); afterEach(async () => { @@ -138,12 +136,15 @@ describe("CliManager", () => { }); describe("Configure CLI", () => { + const TEST_BIN = "/usr/bin/coder"; + function configure(token = "test-token") { return manager.configure( "dev.coder.com", "https://coder.example.com", token, MOCK_FEATURE_SET, + TEST_BIN, ); } @@ -160,7 +161,13 @@ describe("CliManager", () => { it("should throw when URL is empty", async () => { await expect( - manager.configure("dev.coder.com", "", "test-token", MOCK_FEATURE_SET), + manager.configure( + "dev.coder.com", + "", + "test-token", + MOCK_FEATURE_SET, + TEST_BIN, + ), ).rejects.toThrow("URL is required to configure the CLI"); }); @@ -181,6 +188,7 @@ describe("CliManager", () => { "https://coder.example.com", "token", MOCK_FEATURE_SET, + TEST_BIN, ); expect(memfs.readFileSync("/path/base/url", "utf8")).toBe( @@ -189,24 +197,26 @@ describe("CliManager", () => { expect(memfs.readFileSync("/path/base/session", "utf8")).toBe("token"); }); - it("should write to keyring and skip files when featureSet enables keyring", async () => { + it("should store via CLI credential manager when keyring enabled", async () => { vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); await configure("test-token"); - expect(mockKeyring.setToken).toHaveBeenCalledWith( + expect(mockCredManager.storeToken).toHaveBeenCalledWith( + TEST_BIN, "https://coder.example.com", "test-token", + expect.anything(), ); expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); }); - it("should throw and show error when keyring write fails", async () => { + it("should throw and show error when CLI keyring store fails", async () => { vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); - vi.mocked(mockKeyring.setToken).mockImplementation(() => { - throw new Error("keyring unavailable"); - }); + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + new Error("keyring unavailable"), + ); await expect(configure("test-token")).rejects.toThrow( "keyring unavailable", @@ -231,27 +241,8 @@ describe("CliManager", () => { ); } - interface RemoveCredentialsCase { - scenario: string; - setup: () => void; - } - it.each([ - { scenario: "normally", setup: () => {} }, - { - scenario: "when keyring delete fails", - setup: () => - vi.mocked(mockKeyring.deleteToken).mockImplementation(() => { - throw new Error("keyring unavailable"); - }), - }, - { - scenario: "when keyring is disabled", - setup: () => - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false), - }, - ])("should remove credential files $scenario", async ({ setup }) => { + it("should remove credential files", async () => { seedCredentialFiles(); - setup(); await manager.clearCredentials("dev.coder.com"); expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); @@ -261,13 +252,6 @@ describe("CliManager", () => { await expect( manager.clearCredentials("dev.coder.com"), ).resolves.not.toThrow(); - expect(mockKeyring.deleteToken).toHaveBeenCalledWith("dev.coder.com"); - }); - - it("should skip keyring delete when keyring is disabled", async () => { - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false); - await manager.clearCredentials("dev.coder.com"); - expect(mockKeyring.deleteToken).not.toHaveBeenCalled(); }); }); @@ -709,7 +693,7 @@ describe("CliManager", () => { const manager = new CliManager( createMockLogger(), resolver, - createMockKeyringStore(), + createMockCliCredentialManager(), ); withSuccessfulDownload(); diff --git a/test/unit/keyringStore.test.ts b/test/unit/keyringStore.test.ts deleted file mode 100644 index f273fc00..00000000 --- a/test/unit/keyringStore.test.ts +++ /dev/null @@ -1,231 +0,0 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; - -import { - type KeyringEntry, - KeyringStore, - isKeyringSupported, -} from "@/keyringStore"; - -import { createMockLogger } from "../mocks/testHelpers"; - -/** - * In-memory backing store that simulates the OS keyring. - * Each call to `factory()` returns a fresh handle pointing to the same - * shared state, matching real @napi-rs/keyring behavior where - * `Entry.withTarget()` returns a new handle to the same credential. - */ -function createMockEntryFactory() { - let password: string | null = null; - let secret: Uint8Array | null = null; - - return { - factory: (): KeyringEntry => ({ - getPassword: () => password, - setPassword: (p: string) => { - password = p; - }, - getSecret: () => secret, - setSecret: (s: Uint8Array) => { - secret = s; - }, - deleteCredential: () => { - password = null; - secret = null; - }, - }), - getRawPassword: () => password, - getRawSecret: () => secret, - hasCredential: () => password !== null || secret !== null, - }; -} - -function stubPlatform(platform: string) { - vi.stubGlobal("process", { ...process, platform }); -} - -function createTestContext() { - const mockEntry = createMockEntryFactory(); - const store = new KeyringStore(createMockLogger(), mockEntry.factory); - return { store, mockEntry }; -} - -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 }) => { - stubPlatform(platform); - expect(isKeyringSupported()).toBe(expected); - }); -}); - -describe("KeyringStore", () => { - afterEach(() => { - vi.unstubAllGlobals(); - }); - - // CRUD behavior is platform-independent; darwin is used as the test platform. - describe("token operations", () => { - it("sets and gets a token", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "my-token"); - expect(store.getToken("dev.coder.com")).toBe("my-token"); - }); - - it("overwrites token for same deployment", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "old-token"); - store.setToken("https://dev.coder.com", "new-token"); - expect(store.getToken("dev.coder.com")).toBe("new-token"); - }); - - it("preserves other deployments on set", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.setToken("https://staging.coder.com", "token-2"); - - expect(store.getToken("dev.coder.com")).toBe("token-1"); - expect(store.getToken("staging.coder.com")).toBe("token-2"); - }); - - it("returns undefined for missing deployment", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - expect(store.getToken("unknown.coder.com")).toBeUndefined(); - }); - - it("deletes token while preserving others", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.setToken("https://staging.coder.com", "token-2"); - - store.deleteToken("dev.coder.com"); - - expect(store.getToken("dev.coder.com")).toBeUndefined(); - expect(store.getToken("staging.coder.com")).toBe("token-2"); - }); - - it("deletes entire credential when last token is removed", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.deleteToken("dev.coder.com"); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - // OS keyring entry itself should be cleaned up, not left as empty JSON - expect(mockEntry.hasCredential()).toBe(false); - }); - - it("handles delete of non-existent deployment gracefully", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.deleteToken("unknown.coder.com"); - expect(store.getToken("dev.coder.com")).toBe("token-1"); - }); - - it("strips URL path and protocol, keeping only host", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com/some/path", "my-token"); - expect(store.getToken("dev.coder.com")).toBe("my-token"); - }); - - it("matches safeHostname to map key with port", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com:3000", "my-token"); - // safeHostname (port stripped) finds the entry stored with port - expect(store.getToken("dev.coder.com")).toBe("my-token"); - // Used the hostname + port should also work - expect(store.getToken("dev.coder.com:3000")).toBe("my-token"); - - store.deleteToken("dev.coder.com"); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - expect(mockEntry.hasCredential()).toBe(false); - }); - }); - - describe("platform encoding", () => { - it("macOS: base64-encoded JSON via password", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token"); - const decoded = JSON.parse( - Buffer.from(mockEntry.getRawPassword()!, "base64").toString("utf-8"), - ); - expect(decoded).toEqual({ - "dev.coder.com": { coder_url: "dev.coder.com", api_token: "token" }, - }); - // Secret must be untouched; reading uses getPassword, not getSecret. - expect(mockEntry.getRawSecret()).toBeNull(); - expect(store.getToken("dev.coder.com")).toBe("token"); - }); - - it("macOS: returns undefined for corrupted credential", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token"); - mockEntry - .factory() - .setPassword(Buffer.from("not-valid-json").toString("base64")); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - }); - - it("Windows: raw UTF-8 JSON via secret", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("win32"); - store.setToken("https://dev.coder.com", "token"); - const decoded = JSON.parse( - Buffer.from(mockEntry.getRawSecret()!).toString("utf-8"), - ); - expect(decoded).toEqual({ - "dev.coder.com": { coder_url: "dev.coder.com", api_token: "token" }, - }); - // Password must be untouched; reading uses getSecret, not getPassword. - expect(mockEntry.getRawPassword()).toBeNull(); - expect(store.getToken("dev.coder.com")).toBe("token"); - }); - - it("Windows: returns undefined for corrupted credential", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("win32"); - store.setToken("https://dev.coder.com", "token"); - mockEntry.factory().setSecret(Buffer.from("not-valid-json")); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - }); - - it("preserves port in map key for CLI compatibility", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com:8080", "my-token"); - const decoded = JSON.parse( - Buffer.from(mockEntry.getRawPassword()!, "base64").toString("utf-8"), - ); - expect(decoded).toEqual({ - "dev.coder.com:8080": { - coder_url: "dev.coder.com:8080", - api_token: "my-token", - }, - }); - }); - - it("throws on unsupported platform", () => { - const { store } = createTestContext(); - stubPlatform("linux"); - const msg = "Keyring is not supported on linux"; - expect(() => store.setToken("https://dev.coder.com", "t")).toThrow(msg); - expect(() => store.getToken("dev.coder.com")).toThrow(msg); - expect(() => store.deleteToken("dev.coder.com")).toThrow(msg); - }); - }); -}); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 7d7a345f..3a6db752 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -10,7 +10,7 @@ import { maybeAskAuthMethod } from "@/promptUtils"; import { createAxiosError, - createMockKeyringStore, + createMockCliCredentialManager, createMockLogger, createMockUser, InMemoryMemento, @@ -118,11 +118,15 @@ function createTestContext() { const secretsManager = new SecretsManager(secretStorage, memento, logger); const mementoManager = new MementoManager(memento); + const mockCliManager = { + fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), + }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, - createMockKeyringStore(), + createMockCliCredentialManager(), + mockCliManager as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); @@ -307,11 +311,15 @@ describe("LoginCoordinator", () => { mockConfig.set("coder.tlsKeyFile", "/path/to/key.pem"); const logger = createMockLogger(); + const mockCliManager2 = { + fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), + }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, - createMockKeyringStore(), + createMockCliCredentialManager(), + mockCliManager2 as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); From 5856dcd07577195a777a59acbc75ffb185956707 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 4 Mar 2026 01:46:29 +0300 Subject: [PATCH 3/6] refactor: derive safeHostname internally from url instead of passing both Make url the single source of truth for deployment identity. fetchBinary, configure, and clearCredentials now derive safeHostname via toSafeHost() internally, eliminating redundant parameters and preventing inconsistency. BinaryResolver and CliCredentialManager methods take url string instead of Deployment object. --- src/commands.ts | 18 +- src/core/cliCredentialManager.ts | 58 +++++- src/core/cliManager.ts | 30 +-- src/core/container.ts | 10 +- src/login/loginCoordinator.ts | 50 +---- src/remote/remote.ts | 12 +- test/mocks/testHelpers.ts | 1 + test/unit/core/cliCredentialManager.test.ts | 183 +++++++++++++------ test/unit/core/cliManager.concurrent.test.ts | 14 +- test/unit/core/cliManager.test.ts | 152 ++++++++------- test/unit/login/loginCoordinator.test.ts | 8 - 11 files changed, 316 insertions(+), 220 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 366e9f49..b350773d 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -214,13 +214,12 @@ export class Commands { this.logger.debug("Logging out"); const deployment = this.deploymentManager.getCurrentDeployment(); - const safeHostname = deployment?.safeHostname; await this.deploymentManager.clearDeployment(); - if (safeHostname) { - await this.cliManager.clearCredentials(safeHostname); - await this.secretsManager.clearAllAuthData(safeHostname); + if (deployment) { + await this.cliManager.clearCredentials(deployment.url); + await this.secretsManager.clearAllAuthData(deployment.safeHostname); } vscode.window @@ -287,7 +286,10 @@ export class Commands { if (selected.hostnames.length === 1) { const selectedHostname = selected.hostnames[0]; - await this.cliManager.clearCredentials(selectedHostname); + const auth = await this.secretsManager.getSessionAuth(selectedHostname); + if (auth?.url) { + await this.cliManager.clearCredentials(auth.url); + } await this.secretsManager.clearAllAuthData(selectedHostname); this.logger.info("Removed credentials for", selectedHostname); vscode.window.showInformationMessage( @@ -306,7 +308,10 @@ export class Commands { if (confirm === "Remove All") { await Promise.all( selected.hostnames.map(async (h) => { - await this.cliManager.clearCredentials(h); + const auth = await this.secretsManager.getSessionAuth(h); + if (auth?.url) { + await this.cliManager.clearCredentials(auth.url); + } await this.secretsManager.clearAllAuthData(h); }), ); @@ -455,7 +460,6 @@ export class Commands { const safeHost = toSafeHost(baseUrl); const binary = await this.cliManager.fetchBinary( this.extensionClient, - safeHost, ); const version = semver.parse(await cliUtils.version(binary)); diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index dcd8f3b5..ae051b72 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -9,6 +9,12 @@ import type { Logger } from "../logging/logger"; const execFileAsync = promisify(execFile); +/** + * Resolves a CLI binary path for a given deployment URL, fetching/downloading + * if needed. Returns the path or throws if unavailable. + */ +export type BinaryResolver = (url: string) => Promise; + /** * Returns true on platforms where the OS keyring is supported (macOS, Windows). */ @@ -18,9 +24,15 @@ export function isKeyringSupported(): boolean { /** * Delegates credential storage to the Coder CLI to keep the credentials in sync. + * + * For operations that don't have a binary path available (readToken, deleteToken), + * uses the injected BinaryResolver to fetch/locate the CLI binary. */ export class CliCredentialManager { - constructor(private readonly logger: Logger) {} + constructor( + private readonly logger: Logger, + private readonly resolveBinary: BinaryResolver, + ) {} /** * Store a token by running: @@ -29,7 +41,7 @@ export class CliCredentialManager { * The token is passed via environment variable so it never appears in * process argument lists. */ - async storeToken( + public async storeToken( binPath: string, url: string, token: string, @@ -56,13 +68,21 @@ export class CliCredentialManager { * Read a token by running: * login token --url * - * Returns trimmed stdout, or undefined on any failure. + * Resolves the CLI binary automatically. Returns trimmed stdout, + * or undefined if the binary can't be resolved or the CLI returns no token. */ - async readToken( - binPath: string, + public async readToken( url: string, configs: Pick, ): Promise { + let binPath: string; + try { + binPath = await this.resolveBinary(url); + } catch (error) { + this.logger.warn("Could not resolve CLI binary for token read:", error); + return undefined; + } + const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; try { const { stdout } = await execFileAsync(binPath, args); @@ -73,4 +93,32 @@ export class CliCredentialManager { return undefined; } } + + /** + * Delete a token by running: + * logout --url + * + * Resolves the CLI binary automatically. Best-effort: logs warnings + * on failure but never throws. + */ + async deleteToken( + url: string, + configs: Pick, + ): Promise { + let binPath: string; + try { + binPath = await this.resolveBinary(url); + } catch (error) { + this.logger.warn("Could not resolve CLI binary for token delete:", error); + return; + } + + const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"]; + try { + await execFileAsync(binPath, args); + this.logger.info("Deleted token via CLI for", url); + } catch (error) { + this.logger.warn("Failed to delete token via CLI:", error); + } + } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 474702b3..9007397a 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 { shouldUseKeyring } from "../cliConfig"; +import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; +import { toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; import { BinaryLock } from "./binaryLock"; @@ -49,10 +50,12 @@ export class CliManager { * unable to download a working binary, whether because of network issues or * downloads being disabled. */ - public async fetchBinary( - restClient: Api, - safeHostname: string, - ): Promise { + public async fetchBinary(restClient: Api): Promise { + const baseUrl = restClient.getAxiosInstance().defaults.baseURL; + if (!baseUrl) { + throw new Error("REST client has no base URL configured"); + } + const safeHostname = toSafeHost(baseUrl); const cfg = vscode.workspace.getConfiguration("coder"); // Settings can be undefined when set to their defaults (true in this case), // so explicitly check against false. @@ -719,7 +722,6 @@ export class CliManager { * authentication) but the URL must be a non-empty string. */ public async configure( - safeHostname: string, url: string, token: string, featureSet: FeatureSet, @@ -728,6 +730,7 @@ export class CliManager { if (!url) { throw new Error("URL is required to configure the CLI"); } + const safeHostname = toSafeHost(url); const configs = vscode.workspace.getConfiguration(); if (shouldUseKeyring(configs, featureSet)) { @@ -765,12 +768,17 @@ export class CliManager { } /** - * Remove file-based credentials for a deployment. Keyring entries are not - * removed here because deleting requires the CLI binary, which may not be - * available at logout time. This is fine: stale keyring entries are harmless - * since the CLI overwrites them on next `coder login`. + * Remove credentials for a deployment. Clears both file-based credentials + * and keyring entries (via `coder logout`). Keyring deletion is best-effort: + * if it fails, file cleanup still runs. */ - public async clearCredentials(safeHostname: string): Promise { + public async clearCredentials(url: string): Promise { + const safeHostname = toSafeHost(url); + const configs = vscode.workspace.getConfiguration(); + if (isKeyringEnabled(configs)) { + await this.cliCredentialManager.deleteToken(url, configs); + } + const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); const urlPath = this.pathResolver.getUrlPath(safeHostname); await Promise.all([ diff --git a/src/core/container.ts b/src/core/container.ts index 5b7c5953..143687a3 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -1,5 +1,6 @@ import * as vscode from "vscode"; +import { CoderApi } from "../api/coderApi"; import { type Logger } from "../logging/logger"; import { LoginCoordinator } from "../login/loginCoordinator"; @@ -36,7 +37,13 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); - this.cliCredentialManager = new CliCredentialManager(this.logger); + this.cliCredentialManager = new CliCredentialManager( + this.logger, + async (url) => { + const client = CoderApi.create(url, "", this.logger); + return this.cliManager.fetchBinary(client); + }, + ); this.cliManager = new CliManager( this.logger, this.pathResolver, @@ -48,7 +55,6 @@ export class ServiceContainer implements vscode.Disposable { this.mementoManager, this.logger, this.cliCredentialManager, - this.cliManager, context.extension.id, ); } diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 1a1a7e38..cdaa8db6 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -4,7 +4,6 @@ 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"; @@ -14,7 +13,6 @@ import { vscodeProposed } from "../vscodeProposed"; import type { User } from "coder/site/src/api/typesGenerated"; import type { CliCredentialManager } from "../core/cliCredentialManager"; -import type { CliManager } from "../core/cliManager"; import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; @@ -43,7 +41,6 @@ export class LoginCoordinator implements vscode.Disposable { private readonly mementoManager: MementoManager, private readonly logger: Logger, private readonly cliCredentialManager: CliCredentialManager, - private readonly cliManager: CliManager, extensionId: string, ) { this.oauthAuthorizer = new OAuthAuthorizer( @@ -306,52 +303,15 @@ export class LoginCoordinator implements vscode.Disposable { } /** - * Read a token from the CLI keyring. Fetches the CLI binary first (using - * an unauthenticated client) so the binary is available for keyring reads. - * Returns undefined if the keyring is disabled, the binary can't be fetched, - * or the CLI returns no token. + * Read a token from the CLI keyring. */ private async getCliKeyringToken( deployment: Deployment, ): Promise { - if (!isKeyringEnabled(vscode.workspace.getConfiguration())) { - return undefined; - } - try { - const binPath = await this.ensureBinaryForKeyring( - deployment.url, - deployment.safeHostname, - ); - if (!binPath) { - return undefined; - } - return await this.cliCredentialManager.readToken( - binPath, - deployment.url, - vscode.workspace.getConfiguration(), - ); - } catch (error) { - this.logger.warn("Failed to read token from CLI keyring", error); - return undefined; - } - } - - /** - * Fetch or locate a CLI binary for the given deployment. Uses an - * unauthenticated client since getBuildInfo and binary downloads - * don't require auth. - */ - private async ensureBinaryForKeyring( - url: string, - safeHostname: string, - ): Promise { - try { - const client = CoderApi.create(url, "", this.logger); - return await this.cliManager.fetchBinary(client, safeHostname); - } catch (error) { - this.logger.warn("Could not fetch CLI binary for keyring read:", error); - return undefined; - } + return this.cliCredentialManager.readToken( + deployment.url, + vscode.workspace.getConfiguration(), + ); } /** diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 87fe724d..12b94bd8 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -215,10 +215,7 @@ export class Remote { if ( this.extensionContext.extensionMode === vscode.ExtensionMode.Production ) { - binaryPath = await this.cliManager.fetchBinary( - workspaceClient, - parts.safeHostname, - ); + binaryPath = await this.cliManager.fetchBinary(workspaceClient); } else { try { // In development, try to use `/tmp/coder` as the binary path. @@ -226,10 +223,7 @@ export class Remote { binaryPath = path.join(os.tmpdir(), "coder"); await fs.stat(binaryPath); } catch { - binaryPath = await this.cliManager.fetchBinary( - workspaceClient, - parts.safeHostname, - ); + binaryPath = await this.cliManager.fetchBinary(workspaceClient); } } @@ -248,7 +242,6 @@ export class Remote { // Write token to keyring or file (after CLI version is known) if (baseUrlRaw && token !== undefined) { await this.cliManager.configure( - parts.safeHostname, baseUrlRaw, token, featureSet, @@ -265,7 +258,6 @@ export class Remote { if (auth?.url) { try { await this.cliManager.configure( - parts.safeHostname, auth.url, auth.token, featureSet, diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index efe649cb..631a1282 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -380,6 +380,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { return { storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), + deleteToken: vi.fn().mockResolvedValue(undefined), } as unknown as CliCredentialManager; } diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index cecfa97b..e06987f7 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -4,6 +4,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { CliCredentialManager, isKeyringSupported, + type BinaryResolver, } from "@/core/cliCredentialManager"; import { createMockLogger } from "../../mocks/testHelpers"; @@ -53,6 +54,19 @@ const mockConfigs = { get: vi.fn().mockReturnValue(undefined), }; +const TEST_BIN = "/usr/bin/coder"; +const TEST_URL = "https://dev.coder.com"; + +function createSuccessResolver(): BinaryResolver { + return vi.fn().mockResolvedValue(TEST_BIN) as unknown as BinaryResolver; +} + +function createFailingResolver(): BinaryResolver { + return vi + .fn() + .mockRejectedValue(new Error("no binary")) as unknown as BinaryResolver; +} + describe("isKeyringSupported", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -77,18 +91,21 @@ describe("CliCredentialManager", () => { describe("storeToken", () => { it("passes token via CODER_SESSION_TOKEN env var", async () => { mockExecFileSuccess(); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); await manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", + TEST_BIN, + TEST_URL, "my-secret-token", mockConfigs, ); expect(execFile).toHaveBeenCalledWith( - "/usr/bin/coder", - ["login", "--use-token-as-session", "https://dev.coder.com"], + TEST_BIN, + ["login", "--use-token-as-session", TEST_URL], expect.objectContaining({ env: expect.objectContaining({ CODER_SESSION_TOKEN: "my-secret-token", @@ -100,11 +117,14 @@ describe("CliCredentialManager", () => { it("never passes token in args", async () => { mockExecFileSuccess(); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); await manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", + TEST_BIN, + TEST_URL, "my-secret-token", mockConfigs, ); @@ -115,15 +135,13 @@ describe("CliCredentialManager", () => { it("throws when CLI fails", async () => { mockExecFileFailure("login failed"); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); await expect( - manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", - "token", - mockConfigs, - ), + manager.storeToken(TEST_BIN, TEST_URL, "token", mockConfigs), ).rejects.toThrow("login failed"); }); @@ -134,78 +152,135 @@ describe("CliCredentialManager", () => { key === "coder.headerCommand" ? "my-header-cmd" : undefined, ), }; - const manager = new CliCredentialManager(createMockLogger()); - - await manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", - "token", - configWithHeaders, + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), ); + await manager.storeToken(TEST_BIN, TEST_URL, "token", configWithHeaders); + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; expect(args).toContain("--header-command"); }); }); describe("readToken", () => { - it("returns trimmed stdout", async () => { + it("resolves binary and returns trimmed stdout", async () => { mockExecFileSuccess(" my-token\n"); - const manager = new CliCredentialManager(createMockLogger()); + const resolver = createSuccessResolver(); + const manager = new CliCredentialManager(createMockLogger(), resolver); - const token = await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, - ); + const token = await manager.readToken(TEST_URL, mockConfigs); + expect(resolver).toHaveBeenCalledWith(TEST_URL); expect(token).toBe("my-token"); }); it("returns undefined on empty stdout", async () => { mockExecFileSuccess(" \n"); - const manager = new CliCredentialManager(createMockLogger()); - - const token = await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), ); + const token = await manager.readToken(TEST_URL, mockConfigs); + expect(token).toBeUndefined(); }); - it("returns undefined on error", async () => { + it("returns undefined on CLI error", async () => { mockExecFileFailure("no token found"); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); - const token = await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, + const token = await manager.readToken(TEST_URL, mockConfigs); + + expect(token).toBeUndefined(); + }); + + it("returns undefined when binary resolver fails", async () => { + const manager = new CliCredentialManager( + createMockLogger(), + createFailingResolver(), ); + const token = await manager.readToken(TEST_URL, mockConfigs); + expect(token).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); }); it("passes correct args", async () => { mockExecFileSuccess("token"); - const manager = new CliCredentialManager(createMockLogger()); - - await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), ); + await manager.readToken(TEST_URL, mockConfigs); + + const call = vi.mocked(execFile).mock.calls[0]; + expect(call[0]).toBe(TEST_BIN); + expect(call[1]).toEqual(["login", "token", "--url", TEST_URL]); + }); + }); + + describe("deleteToken", () => { + it("resolves binary and runs coder logout", async () => { + mockExecFileSuccess(); + const resolver = createSuccessResolver(); + const manager = new CliCredentialManager(createMockLogger(), resolver); + + await manager.deleteToken(TEST_URL, mockConfigs); + + expect(resolver).toHaveBeenCalledWith(TEST_URL); const call = vi.mocked(execFile).mock.calls[0]; - expect(call[0]).toBe("/usr/bin/coder"); - expect(call[1]).toEqual([ - "login", - "token", - "--url", - "https://dev.coder.com", - ]); + expect(call[0]).toBe(TEST_BIN); + expect(call[1]).toEqual(["logout", "--url", TEST_URL, "--yes"]); + }); + + it("does not throw when CLI fails", async () => { + mockExecFileFailure("logout failed"); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); + + await expect( + manager.deleteToken(TEST_URL, mockConfigs), + ).resolves.not.toThrow(); + }); + + it("does not throw when binary resolver fails", async () => { + const manager = new CliCredentialManager( + createMockLogger(), + createFailingResolver(), + ); + + await expect( + manager.deleteToken(TEST_URL, mockConfigs), + ).resolves.not.toThrow(); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("includes header args when header command is set", async () => { + mockExecFileSuccess(); + const configWithHeaders = { + get: vi.fn((key: string) => + key === "coder.headerCommand" ? "my-header-cmd" : undefined, + ), + }; + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); + + await manager.deleteToken(TEST_URL, configWithHeaders); + + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).toContain("--header-command"); }); }); }); diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 0a6cd804..fe0b0fae 100644 --- a/test/unit/core/cliManager.concurrent.test.ts +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -109,9 +109,9 @@ describe("CliManager Concurrent Downloads", () => { const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); const downloads = await Promise.all([ - manager.fetchBinary(mockApi, label), - manager.fetchBinary(mockApi, label), - manager.fetchBinary(mockApi, label), + manager.fetchBinary(mockApi), + manager.fetchBinary(mockApi), + manager.fetchBinary(mockApi), ]); expect(downloads).toHaveLength(3); @@ -143,14 +143,14 @@ describe("CliManager Concurrent Downloads", () => { const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); // Start first call and give it time to acquire the lock - const firstDownload = manager.fetchBinary(mockApi1, label); + const firstDownload = manager.fetchBinary(mockApi1); // Wait for the lock to be acquired before starting concurrent calls await new Promise((resolve) => setTimeout(resolve, 50)); const downloads = await Promise.all([ firstDownload, - manager.fetchBinary(mockApi2, label), - manager.fetchBinary(mockApi2, label), + manager.fetchBinary(mockApi2), + manager.fetchBinary(mockApi2), ]); expect(downloads).toHaveLength(3); @@ -188,7 +188,7 @@ describe("CliManager Concurrent Downloads", () => { const label = "test.coder.com"; const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); - await expect(manager.fetchBinary(mockApi, label)).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( `Unable to download binary: ${code}: ${message}`, ); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 1d9dac2f..845275aa 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -59,6 +59,7 @@ vi.mock("@/cliConfig", async () => { return { ...actual, shouldUseKeyring: vi.fn(), + isKeyringEnabled: vi.fn(), }; }); @@ -90,7 +91,7 @@ describe("CliManager", () => { const TEST_VERSION = "1.2.3"; const TEST_URL = "https://test.coder.com"; const BASE_PATH = "/path/base"; - const BINARY_DIR = `${BASE_PATH}/test/bin`; + const BINARY_DIR = `${BASE_PATH}/test.coder.com/bin`; const PLATFORM = "linux"; const ARCH = "amd64"; const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; @@ -140,7 +141,6 @@ describe("CliManager", () => { function configure(token = "test-token") { return manager.configure( - "dev.coder.com", "https://coder.example.com", token, MOCK_FEATURE_SET, @@ -151,50 +151,45 @@ describe("CliManager", () => { it("should write both url and token to correct paths", async () => { await configure("test-token"); - expect(memfs.readFileSync("/path/base/dev.coder.com/url", "utf8")).toBe( - "https://coder.example.com", - ); expect( - memfs.readFileSync("/path/base/dev.coder.com/session", "utf8"), + memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), + ).toBe("https://coder.example.com"); + expect( + memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), ).toBe("test-token"); }); it("should throw when URL is empty", async () => { await expect( - manager.configure( - "dev.coder.com", - "", - "test-token", - MOCK_FEATURE_SET, - TEST_BIN, - ), + manager.configure("", "test-token", MOCK_FEATURE_SET, TEST_BIN), ).rejects.toThrow("URL is required to configure the CLI"); }); it("should write empty string for token when provided", async () => { await configure(""); - expect(memfs.readFileSync("/path/base/dev.coder.com/url", "utf8")).toBe( - "https://coder.example.com", - ); expect( - memfs.readFileSync("/path/base/dev.coder.com/session", "utf8"), + memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), + ).toBe("https://coder.example.com"); + expect( + memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), ).toBe(""); }); - it("should use base path directly when label is empty", async () => { + it("should use hostname-specific path for URL", async () => { await manager.configure( - "", "https://coder.example.com", "token", MOCK_FEATURE_SET, TEST_BIN, ); - expect(memfs.readFileSync("/path/base/url", "utf8")).toBe( - "https://coder.example.com", - ); - expect(memfs.readFileSync("/path/base/session", "utf8")).toBe("token"); + expect( + memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), + ).toBe("https://coder.example.com"); + expect( + memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), + ).toBe("token"); }); it("should store via CLI credential manager when keyring enabled", async () => { @@ -208,8 +203,10 @@ describe("CliManager", () => { "test-token", expect.anything(), ); - expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); - expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/url")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/session")).toBe( + false, + ); }); it("should throw and show error when CLI keyring store fails", async () => { @@ -226,8 +223,10 @@ describe("CliManager", () => { expect.stringContaining("keyring unavailable"), "Open Settings", ); - expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); - expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/url")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/session")).toBe( + false, + ); }); }); @@ -243,29 +242,48 @@ describe("CliManager", () => { it("should remove credential files", async () => { seedCredentialFiles(); - await manager.clearCredentials("dev.coder.com"); + await manager.clearCredentials("https://dev.coder.com"); expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); }); it("should not throw when credential files don't exist", async () => { await expect( - manager.clearCredentials("dev.coder.com"), + manager.clearCredentials("https://dev.coder.com"), ).resolves.not.toThrow(); }); + + it("should call deleteToken when keyring is enabled", async () => { + vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(true); + seedCredentialFiles(); + await manager.clearCredentials("https://dev.coder.com"); + expect(mockCredManager.deleteToken).toHaveBeenCalledWith( + "https://dev.coder.com", + expect.anything(), + ); + // File cleanup still runs + expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + }); + + it("should skip deleteToken when keyring is disabled", async () => { + vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false); + seedCredentialFiles(); + await manager.clearCredentials("https://dev.coder.com"); + expect(mockCredManager.deleteToken).not.toHaveBeenCalled(); + }); }); describe("Binary Version Validation", () => { it("rejects invalid server versions", async () => { mockApi.getBuildInfo = vi.fn().mockResolvedValue({ version: "invalid" }); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Got invalid version from deployment", ); }); it("accepts valid semver versions", async () => { withExistingBinary(TEST_VERSION); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); }); }); @@ -278,7 +296,7 @@ describe("CliManager", () => { it("reuses matching binary without downloading", async () => { withExistingBinary(TEST_VERSION); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).not.toHaveBeenCalled(); // Verify binary still exists @@ -288,7 +306,7 @@ describe("CliManager", () => { it("downloads when versions differ", async () => { withExistingBinary("1.0.0"); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalled(); // Verify new binary exists @@ -301,7 +319,7 @@ describe("CliManager", () => { it("keeps mismatched binary when downloads disabled", async () => { mockConfig.set("coder.enableDownloads", false); withExistingBinary("1.0.0"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).not.toHaveBeenCalled(); // Should still have the old version @@ -314,7 +332,7 @@ describe("CliManager", () => { it("downloads fresh binary when corrupted", async () => { withCorruptedBinary(); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalled(); expect(memfs.existsSync(BINARY_PATH)).toBe(true); @@ -328,7 +346,7 @@ describe("CliManager", () => { expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalled(); @@ -342,7 +360,7 @@ describe("CliManager", () => { it("fails when downloads disabled and no binary", async () => { mockConfig.set("coder.enableDownloads", false); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download CLI because downloads are disabled", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -356,7 +374,7 @@ describe("CliManager", () => { it("downloads with correct headers", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(mockAxios.get).toHaveBeenCalledWith( `/bin/${BINARY_NAME}`, expect.objectContaining({ @@ -372,7 +390,7 @@ describe("CliManager", () => { it("uses custom binary source", async () => { mockConfig.set("coder.binarySource", "/custom/path"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(mockAxios.get).toHaveBeenCalledWith( "/custom/path", expect.objectContaining({ @@ -386,7 +404,7 @@ describe("CliManager", () => { it("uses ETag for existing binaries", async () => { withExistingBinary("1.0.0"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); // Verify ETag was computed from actual file content expect(mockAxios.get).toHaveBeenCalledWith( @@ -408,7 +426,7 @@ describe("CliManager", () => { memfs.writeFileSync(path.join(BINARY_DIR, "keeper.txt"), "keep"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); // Verify old files were actually removed but other files kept expect(memfs.existsSync(path.join(BINARY_DIR, "coder.old-xyz"))).toBe( @@ -425,7 +443,7 @@ describe("CliManager", () => { withExistingBinary("1.0.0"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); // Verify the old binary was backed up const files = readdir(BINARY_DIR); @@ -444,7 +462,7 @@ describe("CliManager", () => { it("handles 304 Not Modified", async () => { withExistingBinary("1.0.0"); withHttpResponse(304); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); // No change expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( @@ -458,7 +476,7 @@ describe("CliManager", () => { "Coder isn't supported for your platform. Please open an issue, we'd love to support it!", "Open an Issue", ); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Platform not supported", ); expect(vscode.env.openExternal).toHaveBeenCalledWith( @@ -476,7 +494,7 @@ describe("CliManager", () => { "Failed to download binary. Please open an issue.", "Open an Issue", ); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Failed to download binary", ); expect(vscode.env.openExternal).toHaveBeenCalledWith( @@ -496,7 +514,7 @@ describe("CliManager", () => { it("handles write stream errors", async () => { withStreamError("write", "disk full"); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download binary: disk full", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -504,7 +522,7 @@ describe("CliManager", () => { it("handles read stream errors", async () => { withStreamError("read", "network timeout"); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download binary: network timeout", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -512,8 +530,7 @@ describe("CliManager", () => { it("handles missing content-length", async () => { withSuccessfulDownload({ headers: {} }); - mockProgress.clearProgressReports(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(memfs.existsSync(BINARY_PATH)).toBe(true); // Without any content-length header, increment should be undefined. @@ -528,8 +545,7 @@ describe("CliManager", () => { "reports progress with %s header", async (header) => { withSuccessfulDownload({ headers: { [header]: "1024" } }); - mockProgress.clearProgressReports(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(memfs.existsSync(BINARY_PATH)).toBe(true); const reports = mockProgress.getProgressReports(); @@ -548,7 +564,7 @@ describe("CliManager", () => { it("shows download progress", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ title: `Downloading Coder CLI for ${TEST_URL}`, @@ -560,7 +576,7 @@ describe("CliManager", () => { it("handles user cancellation", async () => { mockProgress.setCancellation(true); withSuccessfulDownload(); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Download aborted", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -571,7 +587,7 @@ describe("CliManager", () => { it("verifies valid signatures", async () => { withSuccessfulDownload(); withSignatureResponses([200]); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(pgp.verifySignature).toHaveBeenCalled(); const sigFile = expectFileInDir(BINARY_DIR, ".asc"); @@ -582,7 +598,7 @@ describe("CliManager", () => { withSuccessfulDownload(); withSignatureResponses([404, 200]); mockUI.setResponse("Signature not found", "Download signature"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalledTimes(3); const sigFile = expectFileInDir(BINARY_DIR, ".asc"); @@ -596,7 +612,7 @@ describe("CliManager", () => { createVerificationError("Invalid signature"), ); mockUI.setResponse("Signature does not match", "Run anyway"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(memfs.existsSync(BINARY_PATH)).toBe(true); }); @@ -608,7 +624,7 @@ describe("CliManager", () => { createVerificationError("Invalid signature"), ); mockUI.setResponse("Signature does not match", undefined); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Signature verification aborted", ); }); @@ -616,7 +632,7 @@ describe("CliManager", () => { it("skips verification when disabled", async () => { mockConfig.set("coder.disableSignatureVerification", true); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(pgp.verifySignature).not.toHaveBeenCalled(); const files = readdir(BINARY_DIR); @@ -631,7 +647,7 @@ describe("CliManager", () => { withSuccessfulDownload(); withHttpResponse(status); mockUI.setResponse(message, "Run without verification"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(pgp.verifySignature).not.toHaveBeenCalled(); }); @@ -645,7 +661,7 @@ describe("CliManager", () => { withSuccessfulDownload(); withHttpResponse(status); mockUI.setResponse(message, undefined); // User cancels - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Signature download aborted", ); }, @@ -660,7 +676,7 @@ describe("CliManager", () => { it("creates binary directory", async () => { expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(memfs.existsSync(BINARY_DIR)).toBe(true); const stats = memfs.statSync(BINARY_DIR); expect(stats.isDirectory()).toBe(true); @@ -668,7 +684,7 @@ describe("CliManager", () => { it("validates downloaded binary version", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( mockBinaryContent(TEST_VERSION), ); @@ -676,7 +692,7 @@ describe("CliManager", () => { it("sets correct file permissions", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); const stats = memfs.statSync(BINARY_PATH); expect(stats.mode & 0o777).toBe(0o755); }); @@ -697,18 +713,12 @@ describe("CliManager", () => { ); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test label"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual( result, - `${pathWithSpaces}/test label/bin/${BINARY_NAME}`, + `${pathWithSpaces}/test.coder.com/bin/${BINARY_NAME}`, ); }); - - it("handles empty deployment label", async () => { - withExistingBinary(TEST_VERSION, "/path/base/bin"); - const result = await manager.fetchBinary(mockApi, ""); - expectPathsEqual(result, path.join(BASE_PATH, "bin", BINARY_NAME)); - }); }); function createMockApi(version: string, url: string): Api { diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 3a6db752..fb0b3e08 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -118,15 +118,11 @@ function createTestContext() { const secretsManager = new SecretsManager(secretStorage, memento, logger); const mementoManager = new MementoManager(memento); - const mockCliManager = { - fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), - }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, createMockCliCredentialManager(), - mockCliManager as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); @@ -311,15 +307,11 @@ describe("LoginCoordinator", () => { mockConfig.set("coder.tlsKeyFile", "/path/to/key.pem"); const logger = createMockLogger(); - const mockCliManager2 = { - fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), - }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, createMockCliCredentialManager(), - mockCliManager2 as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); From c2ee595eee7759ad424adb37b3d06ff41b144af2 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 5 Mar 2026 01:50:18 +0300 Subject: [PATCH 4/6] refactor: consistent credential API, lightweight binary lookup, quieter logging Make storeToken resolve its own binary internally via the injected BinaryResolver, matching readToken and deleteToken. Remove the binPath parameter from both storeToken and configure since callers no longer need to supply it. Add locateBinary to CliManager as a cheap stat-only lookup that the container resolver tries before falling back to fetchBinary. Downgrade implementation-detail log messages from info/warn to debug, keeping info for significant events (server version, download start, stored/deleted token). --- src/core/cliCredentialManager.ts | 59 ++-- src/core/cliManager.ts | 58 ++-- src/core/container.ts | 10 +- src/login/loginCoordinator.ts | 17 +- src/remote/remote.ts | 8 +- test/unit/core/cliCredentialManager.test.ts | 305 +++++++++----------- test/unit/core/cliManager.test.ts | 32 +- 7 files changed, 230 insertions(+), 259 deletions(-) diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index ae051b72..e0464ddd 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -1,6 +1,7 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; +import { isKeyringEnabled } from "../cliConfig"; import { getHeaderArgs } from "../headers"; import type { WorkspaceConfiguration } from "vscode"; @@ -23,10 +24,8 @@ export function isKeyringSupported(): boolean { } /** - * Delegates credential storage to the Coder CLI to keep the credentials in sync. - * - * For operations that don't have a binary path available (readToken, deleteToken), - * uses the injected BinaryResolver to fetch/locate the CLI binary. + * Delegates credential storage to the Coder CLI. All operations resolve the + * binary via the injected BinaryResolver before invoking it. */ export class CliCredentialManager { constructor( @@ -35,18 +34,22 @@ export class CliCredentialManager { ) {} /** - * Store a token by running: - * CODER_SESSION_TOKEN= login --use-token-as-session - * - * The token is passed via environment variable so it never appears in - * process argument lists. + * Store a token via `coder login --use-token-as-session`. + * Token is passed via CODER_SESSION_TOKEN env var, never in args. */ public async storeToken( - binPath: string, url: string, token: string, configs: Pick, ): Promise { + let binPath: string; + try { + binPath = await this.resolveBinary(url); + } catch (error) { + this.logger.debug("Could not resolve CLI binary for token store:", error); + throw error; + } + const args = [ ...getHeaderArgs(configs), "login", @@ -59,27 +62,28 @@ export class CliCredentialManager { }); this.logger.info("Stored token via CLI for", url); } catch (error) { - this.logger.error("Failed to store token via CLI:", error); + this.logger.debug("Failed to store token via CLI:", error); throw error; } } /** - * Read a token by running: - * login token --url - * - * Resolves the CLI binary automatically. Returns trimmed stdout, - * or undefined if the binary can't be resolved or the CLI returns no token. + * Read a token via `coder login token --url`. Returns trimmed stdout, + * or undefined on any failure (resolver, CLI, empty output). */ public async readToken( url: string, configs: Pick, ): Promise { + if (!isKeyringEnabled(configs)) { + return undefined; + } + let binPath: string; try { binPath = await this.resolveBinary(url); } catch (error) { - this.logger.warn("Could not resolve CLI binary for token read:", error); + this.logger.debug("Could not resolve CLI binary for token read:", error); return undefined; } @@ -89,27 +93,30 @@ export class CliCredentialManager { const token = stdout.trim(); return token || undefined; } catch (error) { - this.logger.warn("Failed to read token via CLI:", error); + this.logger.debug("Failed to read token via CLI:", error); return undefined; } } /** - * Delete a token by running: - * logout --url - * - * Resolves the CLI binary automatically. Best-effort: logs warnings - * on failure but never throws. + * Delete a token via `coder logout --url`. Best-effort: never throws. */ - async deleteToken( + public async deleteToken( url: string, configs: Pick, ): Promise { + if (!isKeyringEnabled(configs)) { + return; + } + let binPath: string; try { binPath = await this.resolveBinary(url); } catch (error) { - this.logger.warn("Could not resolve CLI binary for token delete:", error); + this.logger.debug( + "Could not resolve CLI binary for token delete:", + error, + ); return; } @@ -118,7 +125,7 @@ export class CliCredentialManager { await execFileAsync(binPath, args); this.logger.info("Deleted token via CLI for", url); } catch (error) { - this.logger.warn("Failed to delete token via CLI:", error); + this.logger.debug("Failed to delete token via CLI:", error); } } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 9007397a..9b07cade 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,7 +10,7 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig"; +import { shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; import { toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; @@ -39,6 +39,23 @@ export class CliManager { this.binaryLock = new BinaryLock(output); } + /** + * Return the path to a cached CLI binary for a deployment URL. + * Stat check only — no network, no subprocess. Throws if absent. + */ + public async locateBinary(url: string): Promise { + const safeHostname = toSafeHost(url); + const binPath = path.join( + this.pathResolver.getBinaryCachePath(safeHostname), + cliUtils.name(), + ); + const stat = await cliUtils.stat(binPath); + if (!stat) { + throw new Error(`No CLI binary found at ${binPath}`); + } + return binPath; + } + /** * Download and return the path to a working binary for the deployment with * the provided hostname using the provided client. If the hostname is empty, @@ -60,7 +77,10 @@ export class CliManager { // Settings can be undefined when set to their defaults (true in this case), // so explicitly check against false. const enableDownloads = cfg.get("enableDownloads") !== false; - this.output.info("Downloads are", enableDownloads ? "enabled" : "disabled"); + this.output.debug( + "Downloads are", + enableDownloads ? "enabled" : "disabled", + ); // Get the build info to compare with the existing binary version, if any, // and to log for debugging. @@ -80,18 +100,18 @@ export class CliManager { this.pathResolver.getBinaryCachePath(safeHostname), cliUtils.name(), ); - this.output.info("Using binary path", binPath); + this.output.debug("Using binary path", binPath); const stat = await cliUtils.stat(binPath); if (stat === undefined) { this.output.info("No existing binary found, starting download"); } else { - this.output.info("Existing binary size is", prettyBytes(stat.size)); + this.output.debug("Existing binary size is", prettyBytes(stat.size)); try { const version = await cliUtils.version(binPath); - this.output.info("Existing binary version is", version); + this.output.debug("Existing binary version is", version); // If we have the right version we can avoid the request entirely. if (version === buildInfo.version) { - this.output.info( + this.output.debug( "Using existing binary since it matches the server version", ); return binPath; @@ -130,19 +150,19 @@ export class CliManager { binPath, progressLogPath, ); - this.output.info("Acquired download lock"); + this.output.debug("Acquired download lock"); // If we waited for another process, re-check if binary is now ready if (lockResult.waited) { const latestBuildInfo = await restClient.getBuildInfo(); - this.output.info("Got latest server version", latestBuildInfo.version); + this.output.debug("Got latest server version", latestBuildInfo.version); const recheckAfterWait = await this.checkBinaryVersion( binPath, latestBuildInfo.version, ); if (recheckAfterWait.matches) { - this.output.info( + this.output.debug( "Using existing binary since it matches the latest server version", ); return binPath; @@ -174,7 +194,7 @@ export class CliManager { } finally { if (lockResult) { await lockResult.release(); - this.output.info("Released download lock"); + this.output.debug("Released download lock"); } } } @@ -721,12 +741,7 @@ export class CliManager { * Both URL and token are required. Empty tokens are allowed (e.g. mTLS * authentication) but the URL must be a non-empty string. */ - public async configure( - url: string, - token: string, - featureSet: FeatureSet, - binPath: string, - ) { + public async configure(url: string, token: string, featureSet: FeatureSet) { if (!url) { throw new Error("URL is required to configure the CLI"); } @@ -735,12 +750,7 @@ export class CliManager { const configs = vscode.workspace.getConfiguration(); if (shouldUseKeyring(configs, featureSet)) { try { - await this.cliCredentialManager.storeToken( - binPath, - url, - token, - configs, - ); + await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { this.output.error("Failed to store token via CLI keyring:", error); vscode.window @@ -775,9 +785,7 @@ export class CliManager { public async clearCredentials(url: string): Promise { const safeHostname = toSafeHost(url); const configs = vscode.workspace.getConfiguration(); - if (isKeyringEnabled(configs)) { - await this.cliCredentialManager.deleteToken(url, configs); - } + await this.cliCredentialManager.deleteToken(url, configs); const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); const urlPath = this.pathResolver.getUrlPath(safeHostname); diff --git a/src/core/container.ts b/src/core/container.ts index 143687a3..f3ab4cd5 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -37,11 +37,17 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); + // Circular ref: cliCredentialManager ↔ cliManager. Safe because + // the resolver is only called after construction. this.cliCredentialManager = new CliCredentialManager( this.logger, async (url) => { - const client = CoderApi.create(url, "", this.logger); - return this.cliManager.fetchBinary(client); + try { + return await this.cliManager.locateBinary(url); + } catch { + const client = CoderApi.create(url, "", this.logger); + return this.cliManager.fetchBinary(client); + } }, ); this.cliManager = new CliManager( diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index cdaa8db6..5dfb4677 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -243,7 +243,10 @@ export class LoginCoordinator implements vscode.Disposable { } // Try keyring token (picks up tokens written by `coder login` in the terminal) - const keyringToken = await this.getCliKeyringToken(deployment); + const keyringToken = await this.cliCredentialManager.readToken( + deployment.url, + vscode.workspace.getConfiguration(), + ); if ( keyringToken && keyringToken !== providedToken && @@ -302,18 +305,6 @@ export class LoginCoordinator implements vscode.Disposable { } } - /** - * Read a token from the CLI keyring. - */ - private async getCliKeyringToken( - deployment: Deployment, - ): Promise { - return this.cliCredentialManager.readToken( - deployment.url, - vscode.workspace.getConfiguration(), - ); - } - /** * Shows auth error via dialog or logs it for autoLogin. */ diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 12b94bd8..c676b522 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -241,12 +241,7 @@ export class Remote { // Write token to keyring or file (after CLI version is known) if (baseUrlRaw && token !== undefined) { - await this.cliManager.configure( - baseUrlRaw, - token, - featureSet, - binaryPath, - ); + await this.cliManager.configure(baseUrlRaw, token, featureSet); } // Listen for token changes for this deployment @@ -261,7 +256,6 @@ export class Remote { auth.url, auth.token, featureSet, - binaryPath, ); this.logger.info( "Updated CLI config with new token for remote deployment", diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index e06987f7..28bb94dc 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -1,6 +1,7 @@ import { execFile } from "node:child_process"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { isKeyringEnabled } from "@/cliConfig"; import { CliCredentialManager, isKeyringSupported, @@ -13,60 +14,73 @@ vi.mock("node:child_process", () => ({ execFile: vi.fn(), })); -function stubPlatform(platform: string) { - vi.stubGlobal("process", { ...process, platform }); -} +vi.mock("@/cliConfig", () => ({ + isKeyringEnabled: vi.fn().mockReturnValue(false), +})); -function mockExecFileSuccess(stdout = "") { +const TEST_BIN = "/usr/bin/coder"; +const TEST_URL = "https://dev.coder.com"; + +function stubExecFile(result: { stdout?: string } | { error: string }) { vi.mocked(execFile).mockImplementation( (_cmd, _args, _opts, callback?: unknown) => { - // promisify(execFile) calls execFile with a callback as last argument const cb = typeof _opts === "function" - ? (_opts as (err: Error | null, result: { stdout: string }) => void) + ? (_opts as (err: Error | null, result?: { stdout: string }) => void) : (callback as - | ((err: Error | null, result: { stdout: string }) => void) + | ((err: Error | null, result?: { stdout: string }) => void) | undefined); if (cb) { - cb(null, { stdout }); + if ("error" in result) { + cb(new Error(result.error)); + } else { + cb(null, { stdout: result.stdout ?? "" }); + } } return {} as ReturnType; }, ); } -function mockExecFileFailure(message: string) { - vi.mocked(execFile).mockImplementation( - (_cmd, _args, _opts, callback?: unknown) => { - const cb = - typeof _opts === "function" - ? (_opts as (err: Error | null) => void) - : (callback as ((err: Error | null) => void) | undefined); - if (cb) { - cb(new Error(message)); - } - return {} as ReturnType; - }, - ); +function lastExecArgs(): { + bin: string; + args: string[]; + env: NodeJS.ProcessEnv; +} { + const call = vi.mocked(execFile).mock.calls[0]; + return { + bin: call[0], + args: call[1] as string[], + env: (call[2] as { env: NodeJS.ProcessEnv }).env, + }; } -const mockConfigs = { - get: vi.fn().mockReturnValue(undefined), -}; - -const TEST_BIN = "/usr/bin/coder"; -const TEST_URL = "https://dev.coder.com"; - -function createSuccessResolver(): BinaryResolver { +function successResolver(): BinaryResolver { return vi.fn().mockResolvedValue(TEST_BIN) as unknown as BinaryResolver; } -function createFailingResolver(): BinaryResolver { +function failingResolver(): BinaryResolver { return vi .fn() .mockRejectedValue(new Error("no binary")) as unknown as BinaryResolver; } +const configs = { get: vi.fn().mockReturnValue(undefined) }; + +const configWithHeaders = { + get: vi.fn((key: string) => + key === "coder.headerCommand" ? "my-header-cmd" : undefined, + ), +}; + +function setup(resolver?: BinaryResolver) { + const r = resolver ?? successResolver(); + return { + resolver: r, + manager: new CliCredentialManager(createMockLogger(), r), + }; +} + describe("isKeyringSupported", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -78,7 +92,7 @@ describe("isKeyringSupported", () => { { platform: "linux", expected: false }, { platform: "freebsd", expected: false }, ])("returns $expected for $platform", ({ platform, expected }) => { - stubPlatform(platform); + vi.stubGlobal("process", { ...process, platform }); expect(isKeyringSupported()).toBe(expected); }); }); @@ -89,198 +103,149 @@ describe("CliCredentialManager", () => { }); describe("storeToken", () => { - it("passes token via CODER_SESSION_TOKEN env var", async () => { - mockExecFileSuccess(); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - await manager.storeToken( - TEST_BIN, - TEST_URL, - "my-secret-token", - mockConfigs, - ); - - expect(execFile).toHaveBeenCalledWith( - TEST_BIN, - ["login", "--use-token-as-session", TEST_URL], - expect.objectContaining({ - env: expect.objectContaining({ - CODER_SESSION_TOKEN: "my-secret-token", - }), - }), - expect.any(Function), - ); - }); - - it("never passes token in args", async () => { - mockExecFileSuccess(); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("resolves binary and invokes coder login", async () => { + stubExecFile({ stdout: "" }); + const { manager, resolver } = setup(); - await manager.storeToken( - TEST_BIN, - TEST_URL, - "my-secret-token", - mockConfigs, - ); + await manager.storeToken(TEST_URL, "my-secret-token", configs); - const args = vi.mocked(execFile).mock.calls[0][1] as string[]; - expect(args).not.toContain("my-secret-token"); + expect(resolver).toHaveBeenCalledWith(TEST_URL); + const exec = lastExecArgs(); + expect(exec.bin).toBe(TEST_BIN); + expect(exec.args).toEqual(["login", "--use-token-as-session", TEST_URL]); + // Token must only appear in env, never in args + expect(exec.env.CODER_SESSION_TOKEN).toBe("my-secret-token"); + expect(exec.args).not.toContain("my-secret-token"); }); - it("throws when CLI fails", async () => { - mockExecFileFailure("login failed"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("throws when CLI exec fails", async () => { + stubExecFile({ error: "login failed" }); + const { manager } = setup(); await expect( - manager.storeToken(TEST_BIN, TEST_URL, "token", mockConfigs), + manager.storeToken(TEST_URL, "token", configs), ).rejects.toThrow("login failed"); }); - it("includes header args when header command is set", async () => { - mockExecFileSuccess(); - const configWithHeaders = { - get: vi.fn((key: string) => - key === "coder.headerCommand" ? "my-header-cmd" : undefined, - ), - }; - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - await manager.storeToken(TEST_BIN, TEST_URL, "token", configWithHeaders); - - const args = vi.mocked(execFile).mock.calls[0][1] as string[]; - expect(args).toContain("--header-command"); + it("throws when binary resolver fails", async () => { + const { manager } = setup(failingResolver()); + + await expect( + manager.storeToken(TEST_URL, "token", configs), + ).rejects.toThrow("no binary"); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("forwards header command args", async () => { + stubExecFile({ stdout: "" }); + const { manager } = setup(); + + await manager.storeToken(TEST_URL, "token", configWithHeaders); + + expect(lastExecArgs().args).toContain("--header-command"); }); }); describe("readToken", () => { - it("resolves binary and returns trimmed stdout", async () => { - mockExecFileSuccess(" my-token\n"); - const resolver = createSuccessResolver(); - const manager = new CliCredentialManager(createMockLogger(), resolver); + it("returns trimmed token from CLI stdout", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: " my-token\n" }); + const { manager, resolver } = setup(); - const token = await manager.readToken(TEST_URL, mockConfigs); + const token = await manager.readToken(TEST_URL, configs); expect(resolver).toHaveBeenCalledWith(TEST_URL); expect(token).toBe("my-token"); + expect(lastExecArgs().args).toEqual([ + "login", + "token", + "--url", + TEST_URL, + ]); }); - it("returns undefined on empty stdout", async () => { - mockExecFileSuccess(" \n"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - const token = await manager.readToken(TEST_URL, mockConfigs); - - expect(token).toBeUndefined(); + it("returns undefined on whitespace-only stdout", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: " \n" }); + const { manager } = setup(); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); }); it("returns undefined on CLI error", async () => { - mockExecFileFailure("no token found"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - const token = await manager.readToken(TEST_URL, mockConfigs); - - expect(token).toBeUndefined(); + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ error: "no token found" }); + const { manager } = setup(); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); }); it("returns undefined when binary resolver fails", async () => { - const manager = new CliCredentialManager( - createMockLogger(), - createFailingResolver(), - ); - - const token = await manager.readToken(TEST_URL, mockConfigs); + vi.mocked(isKeyringEnabled).mockReturnValue(true); + const { manager } = setup(failingResolver()); - expect(token).toBeUndefined(); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); }); - it("passes correct args", async () => { - mockExecFileSuccess("token"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - await manager.readToken(TEST_URL, mockConfigs); + it("skips CLI when keyring is disabled", async () => { + stubExecFile({ stdout: "my-token" }); + const { manager } = setup(); - const call = vi.mocked(execFile).mock.calls[0]; - expect(call[0]).toBe(TEST_BIN); - expect(call[1]).toEqual(["login", "token", "--url", TEST_URL]); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); }); }); describe("deleteToken", () => { - it("resolves binary and runs coder logout", async () => { - mockExecFileSuccess(); - const resolver = createSuccessResolver(); - const manager = new CliCredentialManager(createMockLogger(), resolver); + it("resolves binary and invokes coder logout", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager, resolver } = setup(); - await manager.deleteToken(TEST_URL, mockConfigs); + await manager.deleteToken(TEST_URL, configs); expect(resolver).toHaveBeenCalledWith(TEST_URL); - const call = vi.mocked(execFile).mock.calls[0]; - expect(call[0]).toBe(TEST_BIN); - expect(call[1]).toEqual(["logout", "--url", TEST_URL, "--yes"]); + const exec = lastExecArgs(); + expect(exec.bin).toBe(TEST_BIN); + expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); }); - it("does not throw when CLI fails", async () => { - mockExecFileFailure("logout failed"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("never throws on CLI error", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ error: "logout failed" }); + const { manager } = setup(); await expect( - manager.deleteToken(TEST_URL, mockConfigs), + manager.deleteToken(TEST_URL, configs), ).resolves.not.toThrow(); }); - it("does not throw when binary resolver fails", async () => { - const manager = new CliCredentialManager( - createMockLogger(), - createFailingResolver(), - ); + it("never throws when binary resolver fails", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + const { manager } = setup(failingResolver()); await expect( - manager.deleteToken(TEST_URL, mockConfigs), + manager.deleteToken(TEST_URL, configs), ).resolves.not.toThrow(); expect(execFile).not.toHaveBeenCalled(); }); - it("includes header args when header command is set", async () => { - mockExecFileSuccess(); - const configWithHeaders = { - get: vi.fn((key: string) => - key === "coder.headerCommand" ? "my-header-cmd" : undefined, - ), - }; - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("forwards header command args", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager } = setup(); await manager.deleteToken(TEST_URL, configWithHeaders); - const args = vi.mocked(execFile).mock.calls[0][1] as string[]; - expect(args).toContain("--header-command"); + expect(lastExecArgs().args).toContain("--header-command"); + }); + + it("skips CLI when keyring is disabled", async () => { + stubExecFile({ stdout: "" }); + const { manager } = setup(); + + await manager.deleteToken(TEST_URL, configs); + + expect(execFile).not.toHaveBeenCalled(); }); }); }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 845275aa..3f986d3a 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -59,7 +59,6 @@ vi.mock("@/cliConfig", async () => { return { ...actual, shouldUseKeyring: vi.fn(), - isKeyringEnabled: vi.fn(), }; }); @@ -137,14 +136,11 @@ describe("CliManager", () => { }); describe("Configure CLI", () => { - const TEST_BIN = "/usr/bin/coder"; - function configure(token = "test-token") { return manager.configure( "https://coder.example.com", token, MOCK_FEATURE_SET, - TEST_BIN, ); } @@ -161,7 +157,7 @@ describe("CliManager", () => { it("should throw when URL is empty", async () => { await expect( - manager.configure("", "test-token", MOCK_FEATURE_SET, TEST_BIN), + manager.configure("", "test-token", MOCK_FEATURE_SET), ).rejects.toThrow("URL is required to configure the CLI"); }); @@ -181,7 +177,6 @@ describe("CliManager", () => { "https://coder.example.com", "token", MOCK_FEATURE_SET, - TEST_BIN, ); expect( @@ -198,7 +193,6 @@ describe("CliManager", () => { await configure("test-token"); expect(mockCredManager.storeToken).toHaveBeenCalledWith( - TEST_BIN, "https://coder.example.com", "test-token", expect.anything(), @@ -230,6 +224,20 @@ describe("CliManager", () => { }); }); + describe("Locate Binary", () => { + it("returns path when binary exists", async () => { + withExistingBinary(TEST_VERSION); + const result = await manager.locateBinary(TEST_URL); + expectPathsEqual(result, BINARY_PATH); + }); + + it("throws when binary does not exist", async () => { + await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( + "No CLI binary found at", + ); + }); + }); + describe("Clear Credentials", () => { function seedCredentialFiles() { memfs.mkdirSync("/path/base/dev.coder.com", { recursive: true }); @@ -253,8 +261,7 @@ describe("CliManager", () => { ).resolves.not.toThrow(); }); - it("should call deleteToken when keyring is enabled", async () => { - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(true); + it("should always call deleteToken (gating is internal)", async () => { seedCredentialFiles(); await manager.clearCredentials("https://dev.coder.com"); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( @@ -264,13 +271,6 @@ describe("CliManager", () => { // File cleanup still runs expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); }); - - it("should skip deleteToken when keyring is disabled", async () => { - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false); - seedCredentialFiles(); - await manager.clearCredentials("https://dev.coder.com"); - expect(mockCredManager.deleteToken).not.toHaveBeenCalled(); - }); }); describe("Binary Version Validation", () => { From ac64e3ee715f4851bde0316eea104902c06e35ee Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 5 Mar 2026 19:35:09 +0300 Subject: [PATCH 5/6] fix: filter --use-keyring and --global-config from user global flags These flags are managed by the extension and conflict with the auth mode it chooses. Passing --use-keyring=false with --url breaks keyring auth, and --global-config with --url causes the CLI to ignore the keyring entirely. --- package.json | 2 +- src/cliConfig.ts | 29 +++++++++++---- test/unit/cliConfig.test.ts | 72 ++++++++++++++++++++++++++++++++----- 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/package.json b/package.json index 183722e7..838ed3c9 100644 --- a/package.json +++ b/package.json @@ -150,7 +150,7 @@ ] }, "coder.globalFlags": { - "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` flag is explicitly ignored.", + "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.", "type": "array", "items": { "type": "string" diff --git a/src/cliConfig.ts b/src/cliConfig.ts index 7461651e..d947b1b7 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -32,12 +32,29 @@ export function getGlobalFlags( ? ["--url", escapeCommandArg(auth.url)] : ["--global-config", escapeCommandArg(auth.configDir)]; - // Last takes precedence/overrides previous ones - return [ - ...getGlobalFlagsRaw(configs), - ...authFlags, - ...getHeaderArgs(configs), - ]; + const raw = getGlobalFlagsRaw(configs); + const filtered: string[] = []; + for (let i = 0; i < raw.length; i++) { + if (isFlag(raw[i], "--use-keyring")) { + continue; + } + if (isFlag(raw[i], "--global-config")) { + // Skip the next item too when the value is a separate entry. + if (raw[i] === "--global-config") { + i++; + } + continue; + } + filtered.push(raw[i]); + } + + return [...filtered, ...authFlags, ...getHeaderArgs(configs)]; +} + +function isFlag(item: string, name: string): boolean { + return ( + item === name || item.startsWith(`${name}=`) || item.startsWith(`${name} `) + ); } /** diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index aaeb0fd5..5a14b844 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -68,18 +68,72 @@ describe("cliConfig", () => { ]); }); - it("should not filter duplicate global-config flags, last takes precedence", () => { + it.each(["--use-keyring", "--use-keyring=false", "--use-keyring=true"])( + "should filter %s from global flags", + (managedFlag) => { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", [ + "--verbose", + managedFlag, + "--disable-direct-connections", + ]); + + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ + "--verbose", + "--disable-direct-connections", + "--global-config", + '"/config/dir"', + ]); + }, + ); + + interface GlobalConfigCase { + scenario: string; + flags: string[]; + } + it.each([ + { + scenario: "space-separated in one item", + flags: ["-v", "--global-config /path/to/ignored"], + }, + { + scenario: "equals form", + flags: ["-v", "--global-config=/path/to/ignored"], + }, + { + scenario: "separate items", + flags: ["-v", "--global-config", "/path/to/ignored"], + }, + ])( + "should filter --global-config ($scenario) in both auth modes", + ({ flags }) => { + const urlAuth: CliAuth = { + mode: "url", + url: "https://dev.coder.com", + }; + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", flags); + + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ + "-v", + "--global-config", + '"/config/dir"', + ]); + expect(getGlobalFlags(config, urlAuth)).toStrictEqual([ + "-v", + "--url", + '"https://dev.coder.com"', + ]); + }, + ); + + it("should not filter flags with similar prefixes", () => { const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", [ - "-v", - "--global-config /path/to/ignored", - "--disable-direct-connections", - ]); + config.set("coder.globalFlags", ["--global-configs", "--use-keyrings"]); expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ - "-v", - "--global-config /path/to/ignored", - "--disable-direct-connections", + "--global-configs", + "--use-keyrings", "--global-config", '"/config/dir"', ]); From 37266b8570e330868b10859bb613f451fa47871d Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Fri, 6 Mar 2026 17:55:16 +0300 Subject: [PATCH 6/6] feat: add cancellable progress monitors for credential operations Show VS Code progress notifications with Cancel support during storeToken/deleteToken CLI calls. Thread AbortSignal through the credential manager and add a reusable withCancellableProgress util. --- src/cliConfig.ts | 32 ++- src/core/cliCredentialManager.ts | 179 ++++++++++++--- src/core/cliManager.ts | 144 +++++------- src/core/container.ts | 1 + src/progress.ts | 41 ++++ src/remote/remote.ts | 35 ++- test/unit/cliConfig.test.ts | 57 ----- test/unit/core/cliCredentialManager.test.ts | 241 +++++++++++++++++--- test/unit/core/cliManager.test.ts | 193 +++++++--------- test/unit/progress.test.ts | 119 ++++++++++ 10 files changed, 691 insertions(+), 351 deletions(-) create mode 100644 src/progress.ts create mode 100644 test/unit/progress.test.ts diff --git a/src/cliConfig.ts b/src/cliConfig.ts index d947b1b7..54f9bfb4 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -33,22 +33,27 @@ export function getGlobalFlags( : ["--global-config", escapeCommandArg(auth.configDir)]; const raw = getGlobalFlagsRaw(configs); + const filtered = stripManagedFlags(raw); + + return [...filtered, ...authFlags, ...getHeaderArgs(configs)]; +} + +function stripManagedFlags(rawFlags: string[]): string[] { const filtered: string[] = []; - for (let i = 0; i < raw.length; i++) { - if (isFlag(raw[i], "--use-keyring")) { + for (let i = 0; i < rawFlags.length; i++) { + if (isFlag(rawFlags[i], "--use-keyring")) { continue; } - if (isFlag(raw[i], "--global-config")) { + if (isFlag(rawFlags[i], "--global-config")) { // Skip the next item too when the value is a separate entry. - if (raw[i] === "--global-config") { + if (rawFlags[i] === "--global-config") { i++; } continue; } - filtered.push(raw[i]); + filtered.push(rawFlags[i]); } - - return [...filtered, ...authFlags, ...getHeaderArgs(configs)]; + return filtered; } function isFlag(item: string, name: string): boolean { @@ -66,17 +71,6 @@ export function isKeyringEnabled( return isKeyringSupported() && configs.get("coder.useKeyring", true); } -/** - * Single source of truth: should the extension use the OS keyring for this session? - * Requires CLI >= 2.29.0, macOS or Windows, and the coder.useKeyring setting enabled. - */ -export function shouldUseKeyring( - configs: Pick, - featureSet: FeatureSet, -): boolean { - return isKeyringEnabled(configs) && featureSet.keyringAuth; -} - /** * Resolves how the CLI should authenticate: via the keyring (`--url`) or via * the global config directory (`--global-config`). @@ -87,7 +81,7 @@ export function resolveCliAuth( deploymentUrl: string, configDir: string, ): CliAuth { - if (shouldUseKeyring(configs, featureSet)) { + if (isKeyringEnabled(configs) && featureSet.keyringAuth) { return { mode: "url", url: deploymentUrl }; } return { mode: "global-config", configDir }; diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index e0464ddd..a7722362 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -1,20 +1,32 @@ import { execFile } from "node:child_process"; +import fs from "node:fs/promises"; +import path from "node:path"; import { promisify } from "node:util"; +import * as semver from "semver"; import { isKeyringEnabled } from "../cliConfig"; +import { featureSetForVersion } from "../featureSet"; import { getHeaderArgs } from "../headers"; +import { toSafeHost } from "../util"; + +import * as cliUtils from "./cliUtils"; import type { WorkspaceConfiguration } from "vscode"; import type { Logger } from "../logging/logger"; +import type { PathResolver } from "./pathResolver"; + const execFileAsync = promisify(execFile); +const EXEC_TIMEOUT_MS = 60_000; +const EXEC_LOG_INTERVAL_MS = 5_000; + /** * Resolves a CLI binary path for a given deployment URL, fetching/downloading * if needed. Returns the path or throws if unavailable. */ -export type BinaryResolver = (url: string) => Promise; +export type BinaryResolver = (deploymentUrl: string) => Promise; /** * Returns true on platforms where the OS keyring is supported (macOS, Windows). @@ -24,30 +36,33 @@ export function isKeyringSupported(): boolean { } /** - * Delegates credential storage to the Coder CLI. All operations resolve the - * binary via the injected BinaryResolver before invoking it. + * Delegates credential storage to the Coder CLI. Owns all credential + * persistence: keyring-backed (via CLI) and file-based (plaintext). */ export class CliCredentialManager { constructor( private readonly logger: Logger, private readonly resolveBinary: BinaryResolver, + private readonly pathResolver: PathResolver, ) {} /** - * Store a token via `coder login --use-token-as-session`. - * Token is passed via CODER_SESSION_TOKEN env var, never in args. + * Store credentials for a deployment URL. Uses the OS keyring when the + * setting is enabled and the CLI supports it; otherwise writes plaintext + * files under --global-config. + * + * Keyring and files are mutually exclusive — never both. */ public async storeToken( url: string, token: string, configs: Pick, + signal?: AbortSignal, ): Promise { - let binPath: string; - try { - binPath = await this.resolveBinary(url); - } catch (error) { - this.logger.debug("Could not resolve CLI binary for token store:", error); - throw error; + const binPath = await this.resolveKeyringBinary(url, configs); + if (!binPath) { + await this.writeCredentialFiles(url, token); + return; } const args = [ @@ -57,12 +72,13 @@ export class CliCredentialManager { url, ]; try { - await execFileAsync(binPath, args, { + await this.execWithTimeout(binPath, args, { env: { ...process.env, CODER_SESSION_TOKEN: token }, + signal, }); this.logger.info("Stored token via CLI for", url); } catch (error) { - this.logger.debug("Failed to store token via CLI:", error); + this.logger.warn("Failed to store token via CLI:", error); throw error; } } @@ -83,49 +99,160 @@ export class CliCredentialManager { try { binPath = await this.resolveBinary(url); } catch (error) { - this.logger.debug("Could not resolve CLI binary for token read:", error); + this.logger.warn("Could not resolve CLI binary for token read:", error); return undefined; } const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; try { - const { stdout } = await execFileAsync(binPath, args); + const { stdout } = await this.execWithTimeout(binPath, args); const token = stdout.trim(); return token || undefined; } catch (error) { - this.logger.debug("Failed to read token via CLI:", error); + this.logger.warn("Failed to read token via CLI:", error); return undefined; } } /** - * Delete a token via `coder logout --url`. Best-effort: never throws. + * Delete credentials for a deployment. Runs file deletion and keyring + * deletion in parallel, both best-effort (never throws). */ public async deleteToken( url: string, configs: Pick, + signal?: AbortSignal, ): Promise { + await Promise.all([ + this.deleteCredentialFiles(url), + this.deleteKeyringToken(url, configs, signal), + ]); + } + + /** + * Resolve a CLI binary for keyring operations. Returns the binary path + * when keyring is enabled in settings and the CLI version supports it, + * or undefined to fall back to file-based storage. + * + * Throws on binary resolution or version-check failure (caller decides + * whether to catch or propagate). + */ + private async resolveKeyringBinary( + url: string, + configs: Pick, + ): Promise { if (!isKeyringEnabled(configs)) { - return; + return undefined; } + const binPath = await this.resolveBinary(url); + const version = semver.parse(await cliUtils.version(binPath)); + return featureSetForVersion(version).keyringAuth ? binPath : undefined; + } - let binPath: string; + /** + * Wrap execFileAsync with a 60s timeout and periodic debug logging. + */ + private async execWithTimeout( + binPath: string, + args: string[], + options: { env?: NodeJS.ProcessEnv; signal?: AbortSignal } = {}, + ): Promise<{ stdout: string; stderr: string }> { + const { signal, ...execOptions } = options; + const timer = setInterval(() => { + this.logger.debug(`CLI command still running: coder ${args[0]} ...`); + }, EXEC_LOG_INTERVAL_MS); try { - binPath = await this.resolveBinary(url); + return await execFileAsync(binPath, args, { + ...execOptions, + timeout: EXEC_TIMEOUT_MS, + signal, + }); + } finally { + clearInterval(timer); + } + } + + /** + * Write URL and token files under --global-config. + */ + private async writeCredentialFiles( + url: string, + token: string, + ): Promise { + const safeHostname = toSafeHost(url); + await Promise.all([ + this.atomicWriteFile(this.pathResolver.getUrlPath(safeHostname), url), + this.atomicWriteFile( + this.pathResolver.getSessionTokenPath(safeHostname), + token, + ), + ]); + } + + /** + * Delete URL and token files. Best-effort: never throws. + */ + private async deleteCredentialFiles(url: string): Promise { + const safeHostname = toSafeHost(url); + const paths = [ + this.pathResolver.getSessionTokenPath(safeHostname), + this.pathResolver.getUrlPath(safeHostname), + ]; + await Promise.all( + paths.map((p) => + fs.rm(p, { force: true }).catch((error) => { + this.logger.warn("Failed to remove credential file", p, error); + }), + ), + ); + } + + /** + * Delete keyring token via `coder logout`. Best-effort: never throws. + */ + private async deleteKeyringToken( + url: string, + configs: Pick, + signal?: AbortSignal, + ): Promise { + let binPath: string | undefined; + try { + binPath = await this.resolveKeyringBinary(url, configs); } catch (error) { - this.logger.debug( - "Could not resolve CLI binary for token delete:", - error, - ); + this.logger.warn("Could not resolve keyring binary for delete:", error); + return; + } + if (!binPath) { return; } const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"]; try { - await execFileAsync(binPath, args); + await this.execWithTimeout(binPath, args, { signal }); this.logger.info("Deleted token via CLI for", url); } catch (error) { - this.logger.debug("Failed to delete token via CLI:", error); + this.logger.warn("Failed to delete token via CLI:", error); + } + } + + /** + * Atomically write content to a file via temp-file + rename. + */ + private async atomicWriteFile( + filePath: string, + content: string, + ): Promise { + await fs.mkdir(path.dirname(filePath), { recursive: true }); + const tempPath = + filePath + ".temp-" + Math.random().toString(36).substring(8); + try { + await fs.writeFile(tempPath, content, { mode: 0o600 }); + await fs.rename(tempPath, filePath); + } catch (err) { + await fs.rm(tempPath, { force: true }).catch((rmErr) => { + this.logger.warn("Failed to delete temp file", tempPath, rmErr); + }); + throw err; } } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 9b07cade..0ffc5a42 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,8 +10,8 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; +import { withCancellableProgress } from "../progress"; import { toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; @@ -22,7 +22,6 @@ import * as downloadProgress from "./downloadProgress"; import type { Api } from "coder/site/src/api/api"; import type { IncomingMessage } from "node:http"; -import type { FeatureSet } from "../featureSet"; import type { Logger } from "../logging/logger"; import type { CliCredentialManager } from "./cliCredentialManager"; @@ -735,108 +734,91 @@ export class CliManager { /** * Configure the CLI for the deployment with the provided hostname. * - * Stores credentials in the OS keyring when available, otherwise falls back - * to writing plaintext files under --global-config for the CLI. + * Stores credentials in the OS keyring when the setting is enabled and the + * CLI supports it, otherwise writes plaintext files under --global-config. * * Both URL and token are required. Empty tokens are allowed (e.g. mTLS * authentication) but the URL must be a non-empty string. */ - public async configure(url: string, token: string, featureSet: FeatureSet) { + public async configure( + url: string, + token: string, + options?: { silent?: boolean }, + ): Promise { if (!url) { throw new Error("URL is required to configure the CLI"); } - const safeHostname = toSafeHost(url); const configs = vscode.workspace.getConfiguration(); - if (shouldUseKeyring(configs, featureSet)) { + + if (options?.silent) { try { await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { - this.output.error("Failed to store token via CLI keyring:", error); - vscode.window - .showErrorMessage( - `Failed to store session token in OS keyring: ${errToStr(error)}. ` + - "Disable keyring storage in settings to use plaintext files instead.", - "Open Settings", - ) - .then((action) => { - if (action === "Open Settings") { - vscode.commands.executeCommand( - "workbench.action.openSettings", - "coder.useKeyring", - ); - } - }); - throw error; + this.handleStoreError(error); } - } else { - await Promise.all([ - this.writeUrlToGlobalConfig(safeHostname, url), - this.writeTokenToGlobalConfig(safeHostname, token), - ]); + return; + } + + const result = await withCancellableProgress( + { + location: vscode.ProgressLocation.Notification, + title: `Storing credentials for ${url}`, + cancellable: true, + }, + ({ signal }) => + this.cliCredentialManager.storeToken(url, token, configs, signal), + ); + if (result.ok) { + return; + } + if (result.cancelled) { + this.output.info("Credential storage cancelled by user"); + return; } + this.handleStoreError(result.error); } /** * Remove credentials for a deployment. Clears both file-based credentials - * and keyring entries (via `coder logout`). Keyring deletion is best-effort: - * if it fails, file cleanup still runs. + * and keyring entries (via `coder logout`). All cleanup is best-effort. */ public async clearCredentials(url: string): Promise { - const safeHostname = toSafeHost(url); const configs = vscode.workspace.getConfiguration(); - await this.cliCredentialManager.deleteToken(url, configs); - - const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); - const urlPath = this.pathResolver.getUrlPath(safeHostname); - await Promise.all([ - fs.rm(tokenPath, { force: true }).catch((error) => { - this.output.warn("Failed to remove token file", tokenPath, error); - }), - fs.rm(urlPath, { force: true }).catch((error) => { - this.output.warn("Failed to remove URL file", urlPath, error); - }), - ]); - } - - /** - * Write the URL to the --global-config directory for the CLI. - */ - private async writeUrlToGlobalConfig( - safeHostname: string, - url: string, - ): Promise { - const urlPath = this.pathResolver.getUrlPath(safeHostname); - await this.atomicWriteFile(urlPath, url); - } - - /** - * Write the session token to the --global-config directory for the CLI. - */ - private async writeTokenToGlobalConfig(safeHostname: string, token: string) { - const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); - await this.atomicWriteFile(tokenPath, token); + const result = await withCancellableProgress( + { + location: vscode.ProgressLocation.Notification, + title: `Removing credentials for ${url}`, + cancellable: true, + }, + ({ signal }) => + this.cliCredentialManager.deleteToken(url, configs, signal), + ); + if (result.ok) { + return; + } + if (result.cancelled) { + this.output.info("Credential removal cancelled by user"); + } else { + this.output.warn("Failed to remove credentials:", result.error); + } } - /** - * Atomically write content to a file by writing to a temporary file first, - * then renaming it. - */ - private async atomicWriteFile( - filePath: string, - content: string, - ): Promise { - await fs.mkdir(path.dirname(filePath), { recursive: true }); - const tempPath = - filePath + ".temp-" + Math.random().toString(36).substring(8); - try { - await fs.writeFile(tempPath, content, { mode: 0o600 }); - await fs.rename(tempPath, filePath); - } catch (err) { - await fs.rm(tempPath, { force: true }).catch((rmErr) => { - this.output.warn("Failed to delete temp file", tempPath, rmErr); + private handleStoreError(error: unknown): void { + this.output.error("Failed to store credentials:", error); + vscode.window + .showErrorMessage( + `Failed to store credentials: ${errToStr(error)}.`, + "Open Settings", + ) + .then((action) => { + if (action === "Open Settings") { + vscode.commands.executeCommand( + "workbench.action.openSettings", + "coder.useKeyring", + ); + } }); - throw err; - } + throw error; } } diff --git a/src/core/container.ts b/src/core/container.ts index f3ab4cd5..d34df35d 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -49,6 +49,7 @@ export class ServiceContainer implements vscode.Disposable { return this.cliManager.fetchBinary(client); } }, + this.pathResolver, ); this.cliManager = new CliManager( this.logger, diff --git a/src/progress.ts b/src/progress.ts new file mode 100644 index 00000000..d24f017f --- /dev/null +++ b/src/progress.ts @@ -0,0 +1,41 @@ +import * as vscode from "vscode"; + +export type ProgressResult = + | { ok: true; value: T } + | { ok: false; cancelled: true } + | { ok: false; cancelled: false; error: unknown }; + +export interface ProgressContext { + progress: vscode.Progress<{ message?: string; increment?: number }>; + signal: AbortSignal; +} + +/** + * Run a task inside a VS Code progress notification with cancellation support. + * Errors thrown by the task are captured in the result rather than propagated + * through `withProgress`, and AbortErrors from cancellation are surfaced as + * `{ cancelled: true }`. + */ +export function withCancellableProgress( + options: vscode.ProgressOptions & { cancellable: true }, + fn: (ctx: ProgressContext) => Promise, +): Thenable> { + return vscode.window.withProgress( + options, + async (progress, ct): Promise> => { + const ac = new AbortController(); + const listener = ct.onCancellationRequested(() => ac.abort()); + try { + const value = await fn({ progress, signal: ac.signal }); + return { ok: true, value }; + } catch (error) { + if ((error as Error).name === "AbortError") { + return { ok: false, cancelled: true }; + } + return { ok: false, cancelled: false, error }; + } finally { + listener.dispose(); + } + }, + ); +} diff --git a/src/remote/remote.ts b/src/remote/remote.ts index c676b522..e51001ad 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -227,21 +227,9 @@ export class Remote { } } - // First thing is to check the version. - const buildInfo = await workspaceClient.getBuildInfo(); - - let version: semver.SemVer | null = null; - try { - version = semver.parse(await cliUtils.version(binaryPath)); - } catch { - version = semver.parse(buildInfo.version); - } - - const featureSet = featureSetForVersion(version); - - // Write token to keyring or file (after CLI version is known) + // Write token to keyring or file if (baseUrlRaw && token !== undefined) { - await this.cliManager.configure(baseUrlRaw, token, featureSet); + await this.cliManager.configure(baseUrlRaw, token); } // Listen for token changes for this deployment @@ -252,11 +240,9 @@ export class Remote { workspaceClient.setCredentials(auth?.url, auth?.token); if (auth?.url) { try { - await this.cliManager.configure( - auth.url, - auth.token, - featureSet, - ); + await this.cliManager.configure(auth.url, auth.token, { + silent: true, + }); this.logger.info( "Updated CLI config with new token for remote deployment", ); @@ -271,6 +257,17 @@ export class Remote { ), ); + // First thing is to check the version. + const buildInfo = await workspaceClient.getBuildInfo(); + + let version: semver.SemVer | null = null; + try { + version = semver.parse(await cliUtils.version(binaryPath)); + } catch { + version = semver.parse(buildInfo.version); + } + + const featureSet = featureSetForVersion(version); const configDir = this.pathResolver.getGlobalConfigDir( parts.safeHostname, ); diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 5a14b844..f627b4bb 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -8,7 +8,6 @@ import { getSshFlags, isKeyringEnabled, resolveCliAuth, - shouldUseKeyring, } from "@/cliConfig"; import { featureSetForVersion } from "@/featureSet"; @@ -241,62 +240,6 @@ describe("cliConfig", () => { ); }); - describe("shouldUseKeyring", () => { - interface ShouldUseKeyringCase { - platform: NodeJS.Platform; - useKeyring: boolean; - version: string; - expected: boolean; - } - it.each([ - { - platform: "darwin", - useKeyring: true, - version: "2.29.0", - expected: true, - }, - { - platform: "win32", - useKeyring: true, - version: "2.29.0", - expected: true, - }, - { - platform: "linux", - useKeyring: true, - version: "2.29.0", - expected: false, - }, - { - platform: "darwin", - useKeyring: true, - version: "2.28.0", - expected: false, - }, - { - platform: "darwin", - useKeyring: false, - version: "2.29.0", - expected: false, - }, - { - platform: "darwin", - useKeyring: true, - version: "0.0.0-devel+abc123", - expected: true, - }, - ])( - "returns $expected on $platform with useKeyring=$useKeyring and version $version", - ({ platform, useKeyring, version, expected }) => { - vi.stubGlobal("process", { ...process, platform }); - const config = new MockConfigurationProvider(); - config.set("coder.useKeyring", useKeyring); - const featureSet = featureSetForVersion(semver.parse(version)); - expect(shouldUseKeyring(config, featureSet)).toBe(expected); - }, - ); - }); - describe("resolveCliAuth", () => { it("returns url mode when keyring should be used", () => { vi.stubGlobal("process", { ...process, platform: "darwin" }); diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 28bb94dc..218fa1c5 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -1,5 +1,6 @@ +import { fs as memfs, vol } from "memfs"; import { execFile } from "node:child_process"; -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { isKeyringEnabled } from "@/cliConfig"; import { @@ -7,9 +8,13 @@ import { isKeyringSupported, type BinaryResolver, } from "@/core/cliCredentialManager"; +import * as cliUtils from "@/core/cliUtils"; +import { PathResolver } from "@/core/pathResolver"; import { createMockLogger } from "../../mocks/testHelpers"; +import type * as nodeFs from "node:fs"; + vi.mock("node:child_process", () => ({ execFile: vi.fn(), })); @@ -18,40 +23,81 @@ vi.mock("@/cliConfig", () => ({ isKeyringEnabled: vi.fn().mockReturnValue(false), })); +vi.mock("@/core/cliUtils", async () => { + const actual = + await vi.importActual("@/core/cliUtils"); + return { + ...actual, + version: vi.fn().mockResolvedValue("2.29.0"), + }; +}); + +vi.mock("fs/promises", async () => { + const memfs: { fs: typeof nodeFs } = await vi.importActual("memfs"); + return { + ...memfs.fs.promises, + default: memfs.fs.promises, + }; +}); + const TEST_BIN = "/usr/bin/coder"; const TEST_URL = "https://dev.coder.com"; +// promisify(execFile) always calls execFile(bin, args, opts, callback). +// We extract the options from the third positional argument. +interface ExecFileOptions { + env?: NodeJS.ProcessEnv; + timeout?: number; + signal?: AbortSignal; +} + +type ExecFileCallback = ( + err: Error | null, + result?: { stdout: string }, +) => void; + function stubExecFile(result: { stdout?: string } | { error: string }) { - vi.mocked(execFile).mockImplementation( - (_cmd, _args, _opts, callback?: unknown) => { - const cb = - typeof _opts === "function" - ? (_opts as (err: Error | null, result?: { stdout: string }) => void) - : (callback as - | ((err: Error | null, result?: { stdout: string }) => void) - | undefined); - if (cb) { - if ("error" in result) { - cb(new Error(result.error)); - } else { - cb(null, { stdout: result.stdout ?? "" }); - } - } - return {} as ReturnType; - }, - ); + vi.mocked(execFile).mockImplementation((( + _bin: string, + _args: string[], + _opts: ExecFileOptions, + cb: ExecFileCallback, + ) => { + if ("error" in result) { + cb(new Error(result.error)); + } else { + cb(null, { stdout: result.stdout ?? "" }); + } + }) as unknown as typeof execFile); +} + +function stubExecFileAbortable() { + vi.mocked(execFile).mockImplementation((( + _bin: string, + _args: string[], + opts: ExecFileOptions, + ) => { + if (opts.signal?.aborted) { + const err = new Error("The operation was aborted"); + err.name = "AbortError"; + throw err; + } + }) as unknown as typeof execFile); } -function lastExecArgs(): { - bin: string; - args: string[]; - env: NodeJS.ProcessEnv; -} { - const call = vi.mocked(execFile).mock.calls[0]; +function lastExecArgs() { + const [bin, args, opts] = vi.mocked(execFile).mock.calls[0] as [ + string, + readonly string[], + ExecFileOptions, + ...unknown[], + ]; return { - bin: call[0], - args: call[1] as string[], - env: (call[2] as { env: NodeJS.ProcessEnv }).env, + bin, + args, + env: opts.env ?? process.env, + timeout: opts.timeout, + signal: opts.signal, }; } @@ -73,11 +119,26 @@ const configWithHeaders = { ), }; +const TEST_PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); +const CRED_DIR = "/mock/base/dev.coder.com"; +const URL_FILE = `${CRED_DIR}/url`; +const SESSION_FILE = `${CRED_DIR}/session`; + +function writeCredentialFiles(url: string, token: string) { + vol.mkdirSync(CRED_DIR, { recursive: true }); + memfs.writeFileSync(URL_FILE, url); + memfs.writeFileSync(SESSION_FILE, token); +} + function setup(resolver?: BinaryResolver) { const r = resolver ?? successResolver(); return { resolver: r, - manager: new CliCredentialManager(createMockLogger(), r), + manager: new CliCredentialManager( + createMockLogger(), + r, + TEST_PATH_RESOLVER, + ), }; } @@ -98,12 +159,26 @@ describe("isKeyringSupported", () => { }); describe("CliCredentialManager", () => { - afterEach(() => { - vi.resetAllMocks(); + beforeEach(() => { + vi.clearAllMocks(); + vol.reset(); + vi.mocked(isKeyringEnabled).mockReturnValue(false); + vi.mocked(cliUtils.version).mockResolvedValue("2.29.0"); }); describe("storeToken", () => { - it("resolves binary and invokes coder login", async () => { + it("writes files when keyring is disabled", async () => { + const { manager } = setup(); + + await manager.storeToken(TEST_URL, "my-token", configs); + + expect(execFile).not.toHaveBeenCalled(); + expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); + expect(memfs.readFileSync(SESSION_FILE, "utf8")).toBe("my-token"); + }); + + it("resolves binary and invokes coder login when keyring enabled", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ stdout: "" }); const { manager, resolver } = setup(); @@ -118,7 +193,20 @@ describe("CliCredentialManager", () => { expect(exec.args).not.toContain("my-secret-token"); }); + it("falls back to files when CLI version too old", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + vi.mocked(cliUtils.version).mockResolvedValueOnce("2.28.0"); + const { manager } = setup(); + + await manager.storeToken(TEST_URL, "token", configs); + + expect(execFile).not.toHaveBeenCalled(); + expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); + expect(memfs.readFileSync(SESSION_FILE, "utf8")).toBe("token"); + }); + it("throws when CLI exec fails", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ error: "login failed" }); const { manager } = setup(); @@ -127,7 +215,8 @@ describe("CliCredentialManager", () => { ).rejects.toThrow("login failed"); }); - it("throws when binary resolver fails", async () => { + it("throws when binary resolver fails and keyring enabled", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); const { manager } = setup(failingResolver()); await expect( @@ -137,6 +226,7 @@ describe("CliCredentialManager", () => { }); it("forwards header command args", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ stdout: "" }); const { manager } = setup(); @@ -144,6 +234,37 @@ describe("CliCredentialManager", () => { expect(lastExecArgs().args).toContain("--header-command"); }); + + it("passes timeout to execFile", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager } = setup(); + + await manager.storeToken(TEST_URL, "token", configs); + + expect(lastExecArgs().timeout).toBe(60_000); + }); + + it("passes signal through to execFile", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager } = setup(); + const ac = new AbortController(); + + await manager.storeToken(TEST_URL, "token", configs, ac.signal); + + expect(lastExecArgs().signal).toBe(ac.signal); + }); + + it("rejects with AbortError when signal is pre-aborted", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFileAbortable(); + const { manager } = setup(); + + await expect( + manager.storeToken(TEST_URL, "token", configs, AbortSignal.abort()), + ).rejects.toThrow("The operation was aborted"); + }); }); describe("readToken", () => { @@ -193,12 +314,23 @@ describe("CliCredentialManager", () => { expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); }); + + it("passes timeout to execFile", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "token" }); + const { manager } = setup(); + + await manager.readToken(TEST_URL, configs); + + expect(lastExecArgs().timeout).toBe(60_000); + }); }); describe("deleteToken", () => { - it("resolves binary and invokes coder logout", async () => { + it("deletes files and invokes coder logout when keyring enabled", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ stdout: "" }); + writeCredentialFiles(TEST_URL, "old-token"); const { manager, resolver } = setup(); await manager.deleteToken(TEST_URL, configs); @@ -207,6 +339,19 @@ describe("CliCredentialManager", () => { const exec = lastExecArgs(); expect(exec.bin).toBe(TEST_BIN); expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); + expect(memfs.existsSync(URL_FILE)).toBe(false); + expect(memfs.existsSync(SESSION_FILE)).toBe(false); + }); + + it("deletes files even when keyring is disabled", async () => { + writeCredentialFiles(TEST_URL, "old-token"); + const { manager } = setup(); + + await manager.deleteToken(TEST_URL, configs); + + expect(execFile).not.toHaveBeenCalled(); + expect(memfs.existsSync(URL_FILE)).toBe(false); + expect(memfs.existsSync(SESSION_FILE)).toBe(false); }); it("never throws on CLI error", async () => { @@ -239,13 +384,39 @@ describe("CliCredentialManager", () => { expect(lastExecArgs().args).toContain("--header-command"); }); - it("skips CLI when keyring is disabled", async () => { + it("skips keyring when CLI version too old", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + vi.mocked(cliUtils.version).mockResolvedValueOnce("2.28.0"); stubExecFile({ stdout: "" }); + writeCredentialFiles(TEST_URL, "old-token"); const { manager } = setup(); await manager.deleteToken(TEST_URL, configs); expect(execFile).not.toHaveBeenCalled(); + expect(memfs.existsSync(URL_FILE)).toBe(false); + expect(memfs.existsSync(SESSION_FILE)).toBe(false); + }); + + it("passes signal through to execFile", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager } = setup(); + const ac = new AbortController(); + + await manager.deleteToken(TEST_URL, configs, ac.signal); + + expect(lastExecArgs().signal).toBe(ac.signal); + }); + + it("does not throw when signal is aborted", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFileAbortable(); + const { manager } = setup(); + + await expect( + manager.deleteToken(TEST_URL, configs, AbortSignal.abort()), + ).resolves.not.toThrow(); }); }); }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 3f986d3a..cd177ad6 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -9,7 +9,6 @@ import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; -import * as cliConfig from "@/cliConfig"; import { CliManager } from "@/core/cliManager"; import * as cliUtils from "@/core/cliUtils"; import { PathResolver } from "@/core/pathResolver"; @@ -26,7 +25,6 @@ import { import { expectPathsEqual } from "../../utils/platform"; import type { CliCredentialManager } from "@/core/cliCredentialManager"; -import type { FeatureSet } from "@/featureSet"; vi.mock("os"); vi.mock("axios"); @@ -53,15 +51,6 @@ vi.mock("proper-lockfile", () => ({ check: () => Promise.resolve(false), })); -vi.mock("@/cliConfig", async () => { - const actual = - await vi.importActual("@/cliConfig"); - return { - ...actual, - shouldUseKeyring: vi.fn(), - }; -}); - vi.mock("@/pgp"); vi.mock("@/vscodeProposed", () => ({ @@ -95,14 +84,6 @@ describe("CliManager", () => { const ARCH = "amd64"; const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; const BINARY_PATH = `${BINARY_DIR}/${BINARY_NAME}`; - const MOCK_FEATURE_SET: FeatureSet = { - vscodessh: true, - proxyLogDirectory: true, - wildcardSSH: true, - buildReason: true, - keyringAuth: true, - }; - beforeEach(() => { vi.resetAllMocks(); vol.reset(); @@ -136,91 +117,71 @@ describe("CliManager", () => { }); describe("Configure CLI", () => { - function configure(token = "test-token") { - return manager.configure( - "https://coder.example.com", - token, - MOCK_FEATURE_SET, - ); - } + const CONFIGURE_URL = "https://coder.example.com"; + const TOKEN = "test-token"; - it("should write both url and token to correct paths", async () => { - await configure("test-token"); - - expect( - memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), - ).toBe("https://coder.example.com"); - expect( - memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), - ).toBe("test-token"); - }); - - it("should throw when URL is empty", async () => { - await expect( - manager.configure("", "test-token", MOCK_FEATURE_SET), - ).rejects.toThrow("URL is required to configure the CLI"); - }); - - it("should write empty string for token when provided", async () => { - await configure(""); + function configure(options?: { silent?: boolean }) { + return manager.configure(CONFIGURE_URL, TOKEN, options); + } - expect( - memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), - ).toBe("https://coder.example.com"); - expect( - memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), - ).toBe(""); - }); + it("should store credentials with progress notification", async () => { + await configure(); - it("should use hostname-specific path for URL", async () => { - await manager.configure( - "https://coder.example.com", - "token", - MOCK_FEATURE_SET, + expect(vscode.window.withProgress).toHaveBeenCalledWith( + expect.objectContaining({ + location: vscode.ProgressLocation.Notification, + title: `Storing credentials for ${CONFIGURE_URL}`, + cancellable: true, + }), + expect.any(Function), + ); + expect(mockCredManager.storeToken).toHaveBeenCalledWith( + CONFIGURE_URL, + TOKEN, + expect.anything(), + expect.any(AbortSignal), ); - - expect( - memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), - ).toBe("https://coder.example.com"); - expect( - memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), - ).toBe("token"); }); - it("should store via CLI credential manager when keyring enabled", async () => { - vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); - - await configure("test-token"); + it("should skip progress when silent", async () => { + await configure({ silent: true }); + expect(vscode.window.withProgress).not.toHaveBeenCalled(); expect(mockCredManager.storeToken).toHaveBeenCalledWith( - "https://coder.example.com", - "test-token", + CONFIGURE_URL, + TOKEN, expect.anything(), ); - expect(memfs.existsSync("/path/base/coder.example.com/url")).toBe(false); - expect(memfs.existsSync("/path/base/coder.example.com/session")).toBe( - false, - ); }); - it("should throw and show error when CLI keyring store fails", async () => { - vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); - vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( - new Error("keyring unavailable"), + it("should throw when URL is empty", async () => { + await expect(manager.configure("", TOKEN)).rejects.toThrow( + "URL is required to configure the CLI", ); + }); - await expect(configure("test-token")).rejects.toThrow( - "keyring unavailable", - ); + it.each([{ silent: false }, { silent: true }])( + "should throw and show error on failure (silent=$silent)", + async (options) => { + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + new Error("keyring unavailable"), + ); - expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( - expect.stringContaining("keyring unavailable"), - "Open Settings", - ); - expect(memfs.existsSync("/path/base/coder.example.com/url")).toBe(false); - expect(memfs.existsSync("/path/base/coder.example.com/session")).toBe( - false, + await expect(configure(options)).rejects.toThrow("keyring unavailable"); + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + expect.stringContaining("keyring unavailable"), + "Open Settings", + ); + }, + ); + + it("should swallow AbortError when user cancels", async () => { + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + makeAbortError(), ); + + await expect(configure()).resolves.not.toThrow(); + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); }); }); @@ -239,37 +200,35 @@ describe("CliManager", () => { }); describe("Clear Credentials", () => { - function seedCredentialFiles() { - memfs.mkdirSync("/path/base/dev.coder.com", { recursive: true }); - memfs.writeFileSync("/path/base/dev.coder.com/session", "old-token"); - memfs.writeFileSync( - "/path/base/dev.coder.com/url", - "https://example.com", - ); - } + const CLEAR_URL = "https://dev.coder.com"; - it("should remove credential files", async () => { - seedCredentialFiles(); - await manager.clearCredentials("https://dev.coder.com"); - expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); - expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); - }); - - it("should not throw when credential files don't exist", async () => { - await expect( - manager.clearCredentials("https://dev.coder.com"), - ).resolves.not.toThrow(); - }); + it("should delete credentials with progress notification", async () => { + await manager.clearCredentials(CLEAR_URL); - it("should always call deleteToken (gating is internal)", async () => { - seedCredentialFiles(); - await manager.clearCredentials("https://dev.coder.com"); + expect(vscode.window.withProgress).toHaveBeenCalledWith( + expect.objectContaining({ + location: vscode.ProgressLocation.Notification, + title: `Removing credentials for ${CLEAR_URL}`, + cancellable: true, + }), + expect.any(Function), + ); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( - "https://dev.coder.com", + CLEAR_URL, expect.anything(), + expect.any(AbortSignal), ); - // File cleanup still runs - expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + }); + + it.each([ + { scenario: "succeeds", error: undefined }, + { scenario: "fails", error: new Error("unexpected failure") }, + { scenario: "is cancelled", error: makeAbortError() }, + ])("should not throw when deleteToken $scenario", async ({ error }) => { + if (error) { + vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); + } + await expect(manager.clearCredentials(CLEAR_URL)).resolves.not.toThrow(); }); }); @@ -823,6 +782,12 @@ describe("CliManager", () => { } }); +function makeAbortError(): Error { + const error = new Error("The operation was aborted"); + error.name = "AbortError"; + return error; +} + function createVerificationError(msg: string): pgp.VerificationError { const error = new pgp.VerificationError( pgp.VerificationErrorCode.Invalid, diff --git a/test/unit/progress.test.ts b/test/unit/progress.test.ts new file mode 100644 index 00000000..79bc255c --- /dev/null +++ b/test/unit/progress.test.ts @@ -0,0 +1,119 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +import { withCancellableProgress, type ProgressContext } from "@/progress"; + +function mockWithProgress(opts?: { cancelImmediately?: boolean }) { + const dispose = vi.fn(); + + vi.mocked(vscode.window.withProgress).mockImplementation( + async (_opts, task) => { + const progress = { report: vi.fn() }; + const token: vscode.CancellationToken = { + isCancellationRequested: false, + onCancellationRequested: vi.fn((listener: (e: unknown) => void) => { + if (opts?.cancelImmediately) { + listener(undefined); + } + return { dispose }; + }), + }; + return task(progress, token); + }, + ); + + return { dispose }; +} + +describe("withCancellableProgress", () => { + const options = { + location: vscode.ProgressLocation.Notification, + title: "Test operation", + cancellable: true as const, + }; + + beforeEach(() => { + mockWithProgress(); + }); + + it("returns ok with value on success", async () => { + const result = await withCancellableProgress(options, () => + Promise.resolve(42), + ); + + expect(result).toEqual({ ok: true, value: 42 }); + }); + + it("returns cancelled on AbortError", async () => { + const result = await withCancellableProgress(options, () => { + const err = new Error("aborted"); + err.name = "AbortError"; + return Promise.reject(err); + }); + + expect(result).toEqual({ ok: false, cancelled: true }); + }); + + it("returns error on non-abort failure", async () => { + const error = new Error("something broke"); + + const result = await withCancellableProgress(options, () => + Promise.reject(error), + ); + + expect(result).toEqual({ ok: false, cancelled: false, error }); + }); + + it("provides progress and signal to callback", async () => { + let captured: ProgressContext | undefined; + + await withCancellableProgress(options, (ctx) => { + captured = ctx; + return Promise.resolve(); + }); + + expect(captured).toBeDefined(); + expect(captured!.progress.report).toBeDefined(); + expect(captured!.signal).toBeInstanceOf(AbortSignal); + expect(captured!.signal.aborted).toBe(false); + }); + + it("aborts signal when cancellation fires", async () => { + mockWithProgress({ cancelImmediately: true }); + + let signalAborted: boolean | undefined; + await withCancellableProgress(options, ({ signal }) => { + signalAborted = signal.aborted; + return Promise.resolve(); + }); + + expect(signalAborted).toBe(true); + }); + + it("disposes cancellation listener after completion", async () => { + const { dispose } = mockWithProgress(); + + await withCancellableProgress(options, () => Promise.resolve("done")); + + expect(dispose).toHaveBeenCalled(); + }); + + it("disposes cancellation listener on error", async () => { + const { dispose } = mockWithProgress(); + + await withCancellableProgress(options, () => + Promise.reject(new Error("fail")), + ); + + expect(dispose).toHaveBeenCalled(); + }); + + it("passes options through to withProgress", async () => { + await withCancellableProgress(options, () => Promise.resolve()); + + expect(vscode.window.withProgress).toHaveBeenCalledWith( + options, + expect.any(Function), + ); + }); +});