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/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..838ed3c9 100644 --- a/package.json +++ b/package.json @@ -150,12 +150,17 @@ ] }, "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" } }, + "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", 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..d947b1b7 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -1,8 +1,15 @@ -import { type WorkspaceConfiguration } from "vscode"; - +import { isKeyringSupported } from "./core/cliCredentialManager"; import { getHeaderArgs } from "./headers"; 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,19 +21,76 @@ 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[] { - // Last takes precedence/overrides previous ones - return [ - ...getGlobalFlagsRaw(configs), - "--global-config", - escapeCommandArg(configDir), - ...getHeaderArgs(configs), - ]; + const authFlags = + auth.mode === "url" + ? ["--url", escapeCommandArg(auth.url)] + : ["--global-config", escapeCommandArg(auth.configDir)]; + + 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} `) + ); +} + +/** + * 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 }; } /** diff --git a/src/commands.ts b/src/commands.ts index 107f9556..b350773d 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,13 +213,13 @@ export class Commands { this.logger.debug("Logging out"); - const safeHostname = - this.deploymentManager.getCurrentDeployment()?.safeHostname; + const deployment = this.deploymentManager.getCurrentDeployment(); await this.deploymentManager.clearDeployment(); - if (safeHostname) { - await this.secretsManager.clearAllAuthData(safeHostname); + if (deployment) { + await this.cliManager.clearCredentials(deployment.url); + await this.secretsManager.clearAllAuthData(deployment.safeHostname); } vscode.window @@ -283,6 +286,10 @@ export class Commands { if (selected.hostnames.length === 1) { const selectedHostname = selected.hostnames[0]; + 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( @@ -300,9 +307,13 @@ export class Commands { ); if (confirm === "Remove All") { await Promise.all( - selected.hostnames.map((h) => - this.secretsManager.clearAllAuthData(h), - ), + selected.hostnames.map(async (h) => { + const auth = await this.secretsManager.getSessionAuth(h); + if (auth?.url) { + await this.cliManager.clearCredentials(auth.url); + } + await this.secretsManager.clearAllAuthData(h); + }), ); this.logger.info( "Removed credentials for all deployments:", @@ -449,14 +460,14 @@ export class Commands { const safeHost = toSafeHost(baseUrl); const binary = await this.cliManager.fetchBinary( this.extensionClient, - 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/cliCredentialManager.ts b/src/core/cliCredentialManager.ts new file mode 100644 index 00000000..e0464ddd --- /dev/null +++ b/src/core/cliCredentialManager.ts @@ -0,0 +1,131 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +import { isKeyringEnabled } from "../cliConfig"; +import { getHeaderArgs } from "../headers"; + +import type { WorkspaceConfiguration } from "vscode"; + +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). + */ +export function isKeyringSupported(): boolean { + return process.platform === "darwin" || process.platform === "win32"; +} + +/** + * Delegates credential storage to the Coder CLI. All operations resolve the + * binary via the injected BinaryResolver before invoking it. + */ +export class CliCredentialManager { + constructor( + private readonly logger: Logger, + private readonly resolveBinary: BinaryResolver, + ) {} + + /** + * 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( + 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", + "--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.debug("Failed to store token via CLI:", error); + throw error; + } + } + + /** + * 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.debug("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 token = stdout.trim(); + return token || undefined; + } catch (error) { + this.logger.debug("Failed to read token via CLI:", error); + return undefined; + } + } + + /** + * Delete a token via `coder logout --url`. Best-effort: never throws. + */ + 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.debug( + "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.debug("Failed to delete token via CLI:", error); + } + } +} diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 57d0dd76..9b07cade 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -2,24 +2,31 @@ 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 { shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; +import { toSafeHost } from "../util"; 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 { Logger } from "../logging/logger"; + +import type { CliCredentialManager } from "./cliCredentialManager"; +import type { PathResolver } from "./pathResolver"; export class CliManager { private readonly binaryLock: BinaryLock; @@ -27,10 +34,28 @@ export class CliManager { constructor( private readonly output: Logger, private readonly pathResolver: PathResolver, + private readonly cliCredentialManager: CliCredentialManager, ) { 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, @@ -42,15 +67,20 @@ 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. 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. @@ -70,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; @@ -120,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; @@ -164,7 +194,7 @@ export class CliManager { } finally { if (lockResult) { await lockResult.release(); - this.output.info("Released download lock"); + this.output.debug("Released download lock"); } } } @@ -705,48 +735,87 @@ 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, - ) { + public async configure(url: string, token: string, featureSet: FeatureSet) { + 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)) { + 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; + } + } else { + await Promise.all([ + this.writeUrlToGlobalConfig(safeHostname, url), + this.writeTokenToGlobalConfig(safeHostname, token), + ]); + } + } + + /** + * 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(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([ - 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 +830,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..f3ab4cd5 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -1,8 +1,10 @@ import * as vscode from "vscode"; +import { CoderApi } from "../api/coderApi"; 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"; @@ -18,6 +20,7 @@ export class ServiceContainer implements vscode.Disposable { private readonly pathResolver: PathResolver; private readonly mementoManager: MementoManager; private readonly secretsManager: SecretsManager; + private readonly cliCredentialManager: CliCredentialManager; private readonly cliManager: CliManager; private readonly contextManager: ContextManager; private readonly loginCoordinator: LoginCoordinator; @@ -34,12 +37,30 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); - this.cliManager = new CliManager(this.logger, this.pathResolver); + // Circular ref: cliCredentialManager ↔ cliManager. Safe because + // the resolver is only called after construction. + this.cliCredentialManager = new CliCredentialManager( + this.logger, + async (url) => { + try { + return await this.cliManager.locateBinary(url); + } catch { + const client = CoderApi.create(url, "", this.logger); + return this.cliManager.fetchBinary(client); + } + }, + ); + this.cliManager = new CliManager( + this.logger, + this.pathResolver, + this.cliCredentialManager, + ); this.contextManager = new ContextManager(context); this.loginCoordinator = new LoginCoordinator( this.secretsManager, this.mementoManager, this.logger, + this.cliCredentialManager, context.extension.id, ); } @@ -68,6 +89,10 @@ export class ServiceContainer implements vscode.Disposable { return this.contextManager; } + getCliCredentialManager(): CliCredentialManager { + return this.cliCredentialManager; + } + 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/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index cc746df6..5dfb4677 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -12,6 +12,7 @@ import { vscodeProposed } from "../vscodeProposed"; import type { User } from "coder/site/src/api/typesGenerated"; +import type { CliCredentialManager } from "../core/cliCredentialManager"; import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; @@ -39,6 +40,7 @@ export class LoginCoordinator implements vscode.Disposable { private readonly secretsManager: SecretsManager, private readonly mementoManager: MementoManager, private readonly logger: Logger, + private readonly cliCredentialManager: CliCredentialManager, extensionId: string, ) { this.oauthAuthorizer = new OAuthAuthorizer( @@ -211,11 +213,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 +235,30 @@ 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 = await this.cliCredentialManager.readToken( + deployment.url, + vscode.workspace.getConfiguration(), + ); + 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) { diff --git a/src/remote/remote.ts b/src/remote/remote.ts index c51f53db..c676b522 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,34 +211,11 @@ 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 ) { - 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. @@ -245,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); } } @@ -264,6 +239,48 @@ 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(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( + 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 +378,7 @@ export class Remote { binaryPath, featureSet, this.logger, - this.pathResolver, + cliAuth, ); disposables.push(stateMachine); @@ -541,6 +558,7 @@ export class Remote { binaryPath, logDir, featureSet, + cliAuth, ); } catch (error) { this.logger.warn("Failed to configure SSH", error); @@ -715,14 +733,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 +805,7 @@ export class Remote { binaryPath: string, logDir: string, featureSet: FeatureSet, + cliAuth: CliAuth, ) { let deploymentSSHConfig = {}; try { @@ -845,6 +862,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..631a1282 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 { CliCredentialManager } from "@/core/cliCredentialManager"; import type { Logger } from "@/logging/logger"; /** @@ -375,6 +376,14 @@ export class InMemorySecretStorage implements vscode.SecretStorage { } } +export function createMockCliCredentialManager(): CliCredentialManager { + return { + storeToken: vi.fn().mockResolvedValue(undefined), + readToken: vi.fn().mockResolvedValue(undefined), + deleteToken: vi.fn().mockResolvedValue(undefined), + } as unknown as CliCredentialManager; +} + export function createMockLogger(): Logger { return { trace: vi.fn(), diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index e35fa687..5a14b844 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.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 global-config appended", () => { + 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", @@ -31,44 +68,110 @@ describe("cliConfig", () => { ]); }); - it("should not filter duplicate global-config flags, last takes precedence", () => { - const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", [ - "-v", - "--global-config /path/to/ignored", - "--disable-direct-connections", - ]); + 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, "/config/dir")).toStrictEqual([ - "-v", - "--global-config /path/to/ignored", - "--disable-direct-connections", - "--global-config", - '"/config/dir"', - ]); - }); + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ + "--verbose", + "--disable-direct-connections", + "--global-config", + '"/config/dir"', + ]); + }, + ); - it("should not filter header-command flags, header args appended at end", () => { - const headerCommand = "echo test"; + 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.headerCommand", headerCommand); - config.set("coder.globalFlags", [ - "-v", - "--header-command custom", - "--no-feature-warning", - ]); + config.set("coder.globalFlags", ["--global-configs", "--use-keyrings"]); - const result = getGlobalFlags(config, "/config/dir"); - expect(result).toStrictEqual([ - "-v", - "--header-command custom", // ignored by CLI - "--no-feature-warning", + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ + "--global-configs", + "--use-keyrings", "--global-config", '"/config/dir"', - "--header-command", - quoteCommand(headerCommand), ]); }); + + 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", + ]); + + expect(getGlobalFlags(config, auth)).toStrictEqual([ + "-v", + "--header-command custom", // ignored by CLI + "--no-feature-warning", + ...expectedAuthFlags, + "--header-command", + quoteCommand(headerCommand), + ]); + }, + ); }); describe("getGlobalFlagsRaw", () => { @@ -115,6 +218,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/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts new file mode 100644 index 00000000..28bb94dc --- /dev/null +++ b/test/unit/core/cliCredentialManager.test.ts @@ -0,0 +1,251 @@ +import { execFile } from "node:child_process"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { isKeyringEnabled } from "@/cliConfig"; +import { + CliCredentialManager, + isKeyringSupported, + type BinaryResolver, +} from "@/core/cliCredentialManager"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +vi.mock("node:child_process", () => ({ + execFile: vi.fn(), +})); + +vi.mock("@/cliConfig", () => ({ + isKeyringEnabled: vi.fn().mockReturnValue(false), +})); + +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) => { + 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; + }, + ); +} + +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, + }; +} + +function successResolver(): BinaryResolver { + return vi.fn().mockResolvedValue(TEST_BIN) as unknown as 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(); + }); + + it.each([ + { platform: "darwin", expected: true }, + { platform: "win32", expected: true }, + { platform: "linux", expected: false }, + { platform: "freebsd", expected: false }, + ])("returns $expected for $platform", ({ platform, expected }) => { + vi.stubGlobal("process", { ...process, platform }); + expect(isKeyringSupported()).toBe(expected); + }); +}); + +describe("CliCredentialManager", () => { + afterEach(() => { + vi.resetAllMocks(); + }); + + describe("storeToken", () => { + it("resolves binary and invokes coder login", async () => { + stubExecFile({ stdout: "" }); + const { manager, resolver } = setup(); + + await manager.storeToken(TEST_URL, "my-secret-token", configs); + + 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 exec fails", async () => { + stubExecFile({ error: "login failed" }); + const { manager } = setup(); + + await expect( + manager.storeToken(TEST_URL, "token", configs), + ).rejects.toThrow("login failed"); + }); + + 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("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, configs); + + expect(resolver).toHaveBeenCalledWith(TEST_URL); + expect(token).toBe("my-token"); + expect(lastExecArgs().args).toEqual([ + "login", + "token", + "--url", + TEST_URL, + ]); + }); + + 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 () => { + 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 () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + const { manager } = setup(failingResolver()); + + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("skips CLI when keyring is disabled", async () => { + stubExecFile({ stdout: "my-token" }); + const { manager } = setup(); + + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); + }); + }); + + describe("deleteToken", () => { + it("resolves binary and invokes coder logout", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager, resolver } = setup(); + + await manager.deleteToken(TEST_URL, configs); + + expect(resolver).toHaveBeenCalledWith(TEST_URL); + const exec = lastExecArgs(); + expect(exec.bin).toBe(TEST_BIN); + expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); + }); + + 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, configs), + ).resolves.not.toThrow(); + }); + + it("never throws when binary resolver fails", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + const { manager } = setup(failingResolver()); + + await expect( + manager.deleteToken(TEST_URL, configs), + ).resolves.not.toThrow(); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("forwards header command args", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager } = setup(); + + await manager.deleteToken(TEST_URL, configWithHeaders); + + 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.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 96ca3529..fe0b0fae 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 { + createMockCliCredentialManager, createMockLogger, createMockStream, MockConfigurationProvider, @@ -82,6 +83,7 @@ function setupManager(testDir: string): CliManager { return new CliManager( createMockLogger(), new PathResolver(testDir, "/code/log"), + createMockCliCredentialManager(), ); } @@ -107,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); @@ -141,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); @@ -186,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 20151032..3f986d3a 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 { + createMockCliCredentialManager, createMockLogger, createMockStream, MockConfigurationProvider, @@ -23,6 +25,9 @@ import { } from "../../mocks/testHelpers"; import { expectPathsEqual } from "../../utils/platform"; +import type { CliCredentialManager } from "@/core/cliCredentialManager"; +import type { FeatureSet } from "@/featureSet"; + vi.mock("os"); vi.mock("axios"); @@ -48,6 +53,15 @@ 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", () => ({ @@ -71,15 +85,23 @@ describe("CliManager", () => { let mockUI: MockUserInteraction; let mockApi: Api; let mockAxios: AxiosInstance; + let mockCredManager: CliCredentialManager; 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}`; 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,9 +114,11 @@ describe("CliManager", () => { mockConfig = new MockConfigurationProvider(); mockProgress = new MockProgressReporter(); mockUI = new MockUserInteraction(); + mockCredManager = createMockCliCredentialManager(); manager = new CliManager( createMockLogger(), new PathResolver(BASE_PATH, "/code/log"), + mockCredManager, ); // Mock only what's necessary @@ -112,73 +136,154 @@ 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( "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/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(""); - expect(memfs.readFileSync("/path/base/deployment/url", "utf8")).toBe( + 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 use hostname-specific path for URL", async () => { + await manager.configure( "https://coder.example.com", + "token", + MOCK_FEATURE_SET, ); - expect(memfs.readFileSync("/path/base/deployment/session", "utf8")).toBe( - "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("token"); }); - it("should skip URL when undefined but write token", async () => { - await manager.configure("deployment", undefined, "test-token"); + it("should store via CLI credential manager when keyring enabled", async () => { + vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); - // No entry for the url - expect(memfs.existsSync("/path/base/deployment/url")).toBe(false); - expect(memfs.readFileSync("/path/base/deployment/session", "utf8")).toBe( + await configure("test-token"); + + expect(mockCredManager.storeToken).toHaveBeenCalledWith( + "https://coder.example.com", "test-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 skip token when null but write URL", async () => { - await manager.configure("deployment", "https://coder.example.com", null); + 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"), + ); - // No entry for the session - expect(memfs.readFileSync("/path/base/deployment/url", "utf8")).toBe( - "https://coder.example.com", + await expect(configure("test-token")).rejects.toThrow( + "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, ); - expect(memfs.existsSync("/path/base/deployment/session")).toBe(false); }); + }); - it("should write empty string for token when provided", async () => { - await manager.configure("deployment", "https://coder.example.com", ""); + describe("Locate Binary", () => { + it("returns path when binary exists", async () => { + withExistingBinary(TEST_VERSION); + const result = await manager.locateBinary(TEST_URL); + expectPathsEqual(result, BINARY_PATH); + }); - expect(memfs.readFileSync("/path/base/deployment/url", "utf8")).toBe( - "https://coder.example.com", + it("throws when binary does not exist", async () => { + await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( + "No CLI binary found at", ); - expect(memfs.readFileSync("/path/base/deployment/session", "utf8")).toBe( - "", + }); + }); + + 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", ); + } + + 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 use base path directly when label is empty", async () => { - await manager.configure("", "https://coder.example.com", "token"); + it("should not throw when credential files don't exist", async () => { + await expect( + manager.clearCredentials("https://dev.coder.com"), + ).resolves.not.toThrow(); + }); - expect(memfs.readFileSync("/path/base/url", "utf8")).toBe( - "https://coder.example.com", + it("should always call deleteToken (gating is internal)", async () => { + seedCredentialFiles(); + await manager.clearCredentials("https://dev.coder.com"); + expect(mockCredManager.deleteToken).toHaveBeenCalledWith( + "https://dev.coder.com", + expect.anything(), ); - expect(memfs.readFileSync("/path/base/session", "utf8")).toBe("token"); + // File cleanup still runs + expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); }); }); 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); }); }); @@ -191,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 @@ -201,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 @@ -214,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 @@ -227,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); @@ -241,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(); @@ -255,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); @@ -269,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({ @@ -285,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({ @@ -299,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( @@ -321,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( @@ -338,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); @@ -357,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( @@ -371,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( @@ -389,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( @@ -409,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); @@ -417,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); @@ -425,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. @@ -441,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(); @@ -461,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}`, @@ -473,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); @@ -484,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"); @@ -495,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"); @@ -509,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); }); @@ -521,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", ); }); @@ -529,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); @@ -544,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(); }); @@ -558,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", ); }, @@ -573,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); @@ -581,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), ); @@ -589,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); }); @@ -603,21 +706,19 @@ 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, + createMockCliCredentialManager(), + ); 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/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/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 8b850a3b..fb0b3e08 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, + createMockCliCredentialManager, createMockLogger, createMockUser, InMemoryMemento, @@ -121,6 +122,7 @@ function createTestContext() { secretsManager, mementoManager, logger, + createMockCliCredentialManager(), "coder.coder-remote", ); @@ -309,6 +311,7 @@ describe("LoginCoordinator", () => { secretsManager, mementoManager, logger, + createMockCliCredentialManager(), "coder.coder-remote", );