diff --git a/CHANGELOG.md b/CHANGELOG.md index 135c3fe27d..2a5f4e06bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,34 @@ from published versions since it shows up in the VS Code extension changelog tab and is confusing to users. Add it back between releases if needed. --> +## Unreleased + +### Changed + +- Store session tokens in the OS keyring by default on macOS and Windows, in + addition to the extension's own storage. The `coder` CLI reads the same + entry, so once the extension has downloaded the CLI, signing in here also + signs in the CLI. Requires Coder CLI 2.29.0 or later; older CLIs and Linux + keep using a file. To opt out, set `coder.useKeyring` to `false`. +- Ask at logout whether to sign the `coder` CLI out too when it shares the + session, since anything else using that session is signed out with it. +- Pass `coder.useKeyring` to the CLI as `--use-keyring`, so the setting wins + over the `CODER_USE_KEYRING` environment variable. +- Honor `CODER_CONFIG_DIR` like `--global-config` in `coder.globalFlags`. +- Read the `coder` CLI's session only on Coder CLI 2.32.0 or later, up from + 2.31.0, where the CLI checks the stored URL against the one you connect to. +- Ask before signing in with the `coder` CLI's session when it belongs to a + different user than your previous session. +- Show an error with **Open Settings** when the CLI cannot store the token at + login, and a **Show Output** button when logout cannot remove every + credential. + +### Fixed + +- Pass `--allow-redirects` to Coder CLI 2.38.0 or later. The extension already + follows a redirected deployment URL, and without the flag that CLI fails + `coder login`, `coder logout`, and `coder ssh` for it. + ## [v1.16.2](https://github.com/coder/vscode-coder/releases/tag/v1.16.2) 2026-08-25 ### Fixed diff --git a/package.json b/package.json index ae19cefde0..59d0bdfc47 100644 --- a/package.json +++ b/package.json @@ -195,7 +195,7 @@ "ignoreSync": true }, "coder.globalFlags": { - "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item, in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`. For `--flag=value` items the expansion applies to the value half, so `--cfg=~/coder` works.\n\nSet `--global-config` here to point the CLI at a shared config directory (e.g. `--global-config=~/.config/coderv2` to share login/auth with the Coder CLI); requires a deployment on 2.31.0+ and is ignored when `#coder.useKeyring#` is active. The `--use-keyring` flag is ignored; use `#coder.useKeyring#` instead.\n\nFor `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here.", + "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item, in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`. For `--flag=value` items the expansion applies to the value half, so `--cfg=~/coder` works.\n\nTo share a config directory with the `coder` CLI, add `--global-config` here (for example `--global-config=~/.config/coderv2`) or set `CODER_CONFIG_DIR`. Requires Coder CLI 2.32.0 or later. A `--use-keyring` item is ignored; use `#coder.useKeyring#` instead.\n\nFor `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here.", "type": "array", "items": { "type": "string" @@ -204,9 +204,9 @@ "ignoreSync": true }, "coder.useKeyring": { - "markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of plaintext files. Requires CLI >= 2.29.0 (>= 2.31.0 to sync login from CLI to VS Code). This will attempt to sync between the CLI and VS Code since they share the same keyring entry. It will log you out of the CLI if you log out of the IDE, and vice versa. Has no effect on Linux.", + "markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of a file. Requires Coder CLI 2.29.0 or later; 2.32.0 or later to sign in with the CLI's existing session. Has no effect on Linux.\n\nThe keyring entry is shared with the `coder` CLI: signing in here also signs in the CLI, and signing out asks whether to sign out the CLI too.", "type": "boolean", - "default": false, + "default": true, "scope": "application" }, "coder.networkThreshold.latencyMs": { diff --git a/src/commands.ts b/src/commands.ts index 738897d545..3ce889b52e 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -43,7 +43,7 @@ import { RECOMMENDED_SSH_SETTINGS, applySettingOverrides, } from "./remote/sshOverrides"; -import { resolveCliAuth } from "./settings/cli"; +import { isKeyringEnabled, resolveCliAuth } from "./settings/cli"; import { appendVsCodeLogs } from "./supportBundle/appendVsCodeLogs"; import { getRemoteServerDataPath, @@ -82,7 +82,7 @@ import type { CliManager } from "./core/cliManager"; import type { ServiceContainer } from "./core/container"; import type { MementoManager } from "./core/mementoManager"; import type { PathResolver } from "./core/pathResolver"; -import type { SecretsManager } from "./core/secretsManager"; +import type { SecretsManager, SessionAuth } from "./core/secretsManager"; import type { DeploymentManager } from "./deployment/deploymentManager"; import type { Logger } from "./logging/logger"; import type { LoginCoordinator, LoginMethod } from "./login/loginCoordinator"; @@ -698,24 +698,41 @@ export class Commands { } private async performLogout(): Promise { - if (!this.deploymentManager.isAuthenticated()) { + const deployment = this.deploymentManager.getCurrentDeployment(); + if (!this.deploymentManager.isAuthenticated() || !deployment) { return { success: false, reason: "not_authenticated" }; } - this.logger.debug("Logging out"); + const auth = await this.secretsManager.getSessionAuth( + deployment.safeHostname, + ); + const signOutCli = await this.askSignOutCli(auth); + if (signOutCli === undefined) { + return { success: false, reason: "user_dismissed" }; + } + // Another window may have switched deployments while the prompt was open. + if (this.deploymentManager.getCurrentDeployment()?.url !== deployment.url) { + return { success: false, reason: "not_authenticated" }; + } - const deployment = this.deploymentManager.getCurrentDeployment(); + this.logger.debug("Logging out"); await this.deploymentManager.clearDeployment("logout"); - - if (deployment) { - const cleared = await this.cliManager.clearCredentials(deployment.url); - await this.secretsManager.clearAllAuthData(deployment.safeHostname); - if (!cleared) { - vscode.window.showWarningMessage( + const cleared = await this.cliManager.clearCredentials(deployment.url, { + signOutCli, + }); + await this.secretsManager.clearAllAuthData(deployment.safeHostname); + if (!cleared) { + vscode.window + .showWarningMessage( 'You\'ve been logged out of Coder, but some credentials could not be removed. Log out again to retry, or run "coder logout" in a terminal.', - ); - return { success: false, reason: "cleanup_incomplete" }; - } + "Show Output", + ) + .then((action) => { + if (action === "Show Output") { + this.logger.show(); + } + }); + return { success: false, reason: "cleanup_incomplete" }; } this.showLogoutMessage(); @@ -735,6 +752,36 @@ export class Commands { }); } + /** Whether to sign the CLI out too. Asks when it holds this session's token; undefined when dismissed. */ + private async askSignOutCli( + auth: SessionAuth | undefined, + ): Promise { + if ( + !auth?.token || + !(await this.cliManager.holdsToken(auth.url, auth.token)) + ) { + return false; + } + // The CLI cannot refresh an OAuth token and logout revokes it, so there is nothing to keep. + if (auth.oauth) { + return true; + } + const action = await vscodeProposed.window.showWarningMessage( + "Sign out of the Coder CLI too?", + { + useCustom: true, + modal: true, + detail: `${auth.url}\n\nThe Coder CLI is signed in with this session. Signing it out also signs out other tools that rely on it.`, + }, + "Sign Out", + "Keep Signed In", + ); + if (action === undefined) { + return undefined; + } + return action === "Sign Out"; + } + /** * Switch to a different deployment without clearing credentials. * If login fails or user cancels, stays on current deployment. @@ -790,7 +837,11 @@ export class Commands { const selectedHostname = selected.hostnames[0]; const auth = await this.secretsManager.getSessionAuth(selectedHostname); if (auth?.url) { - await this.cliManager.clearCredentials(auth.url); + const signOutCli = await this.askSignOutCli(auth); + if (signOutCli === undefined) { + return; + } + await this.cliManager.clearCredentials(auth.url, { signOutCli }); } await this.secretsManager.clearAllAuthData(selectedHostname); this.logger.info("Removed credentials for", selectedHostname); @@ -803,20 +854,24 @@ export class Commands { { useCustom: true, modal: true, - detail: `This will remove credentials for: ${selected.hostnames.join(", ")}\n\nYou'll need to log in again to access them.`, + detail: `This will remove credentials for: ${selected.hostnames.join(", ")}\n\nYou'll need to log in again to access them.${isKeyringEnabled(vscode.workspace.getConfiguration()) ? " This also signs the Coder CLI out where it shares a session." : ""}`, }, "Remove All", ); if (confirm === "Remove All") { - await Promise.all( - 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); - }), - ); + // One at a time: `coder logout` rewrites the whole keyring entry. + for (const h of selected.hostnames) { + const auth = await this.secretsManager.getSessionAuth(h); + if (auth?.url) { + await this.cliManager.clearCredentials(auth.url, { + signOutCli: await this.cliManager.holdsToken( + auth.url, + auth.token, + ), + }); + } + await this.secretsManager.clearAllAuthData(h); + } this.logger.info( "Removed credentials for all deployments:", selected.hostnames.join(", "), @@ -1410,12 +1465,9 @@ export class Commands { throw new Error("You are not logged in"); } const safeHost = toSafeHost(baseUrl); - let binary: string; - try { - binary = await this.cliManager.locateBinary(baseUrl); - } catch { - binary = await this.cliManager.fetchBinary(client); - } + const binary = + (await this.cliManager.locateBinary(baseUrl)) ?? + (await this.cliManager.fetchBinary(client)); const version = semver.parse(await cliExec.version(binary)); const featureSet = featureSetForVersion(version); const configDir = this.pathResolver.getGlobalConfigDir(safeHost); diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index f6299f58fc..bfcdb0d13a 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -1,17 +1,22 @@ import { execFile } from "node:child_process"; import fs from "node:fs/promises"; -import os from "node:os"; import { promisify } from "node:util"; import * as semver from "semver"; import { isAbortError } from "../error/errorUtils"; import { featureSetForVersion, type FeatureSet } from "../featureSet"; import { + categorizeCredentialError, CredentialCliError, CredentialTelemetry, } from "../instrumentation/credentials"; -import { getGlobalFlags, isKeyringEnabled } from "../settings/cli"; -import { getHeaderArgs } from "../settings/headers"; +import { recordError } from "../instrumentation/outcomes"; +import { + type CliAuth, + getGlobalFlags, + mayUseCliStore, + resolveCliAuth, +} from "../settings/cli"; import { type TelemetryReporter } from "../telemetry/reporter"; import { toSafeHost } from "../util/uri"; @@ -26,39 +31,22 @@ import type { PathResolver } from "./pathResolver"; const execFileAsync = promisify(execFile); -// keyring uses the CLI's default store; cli-file passes --global-config. -type CliTransport = - | { kind: "keyring"; binPath: string } - | { kind: "cli-file"; binPath: string; allowOverride: boolean }; - -type ReadTransport = CliTransport | { kind: "none" }; - -export interface CliCredential { - token: string; - source: "keyring" | "files"; -} - 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 = (deploymentUrl: string) => Promise; - -/** - * Returns true on platforms where the OS keyring is supported (macOS, Windows). - */ -export function isKeyringSupported(): boolean { - const platform = os.platform(); - return platform === "darwin" || platform === "win32"; +interface ResolvedCli { + binPath: string; + featureSet: FeatureSet; + auth: CliAuth; + flags: string[]; } -/** - * Delegates credential storage to the Coder CLI, both keyring-backed and - * file-based, via `coder login`/`coder logout`. - */ +/** The downloaded CLI binary for a deployment URL, or undefined when there is none. */ +export type BinaryResolver = ( + deploymentUrl: string, +) => Promise; + +/** Stores, reads, and deletes credentials through `coder login` and `coder logout`. */ export class CliCredentialManager { private readonly credentialTelemetry: CredentialTelemetry; @@ -71,10 +59,7 @@ export class CliCredentialManager { this.credentialTelemetry = new CredentialTelemetry(telemetry); } - /** - * Store credentials via `coder login` (keyring or file-backed). Throws if the - * CLI binary cannot be resolved. - */ + /** Stores a token via `coder login`. Skipped until the CLI is downloaded; throws when the CLI fails. */ public storeToken( url: string, token: string, @@ -82,243 +67,190 @@ export class CliCredentialManager { options?: { signal?: AbortSignal }, ): Promise { return this.credentialTelemetry.traceStore(configs, async (span) => { - const transport = await this.resolveWriteTransport(url, configs); - span.setProperty( - "category", - transport.kind === "keyring" ? "keyring" : "file", - ); - await this.cliLogin(transport, url, token, configs, options); + const cli = await this.resolveCli(url, configs); + if (!cli) { + span.setProperty("outcome", "no_binary"); + this.logger.info( + "Skipped storing the token in the CLI: it is not downloaded yet", + ); + return; + } + span.setProperty("store", cli.auth.store); + await this.exec(cli, ["login", "--use-token-as-session", url], { + env: { ...process.env, CODER_SESSION_TOKEN: token }, + signal: options?.signal, + }); + span.setProperty("outcome", "stored"); + this.logger.info("Stored token via CLI for", url); }); } - private async cliLogin( - transport: CliTransport, + /** Reads the CLI's token via `coder login token` (CLI 2.32+). Undefined on any failure. */ + public async readToken( url: string, - token: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - const args = [ - ...this.credentialGlobalFlags(transport, url, configs), - "login", - "--use-token-as-session", - url, - ]; + ): Promise { try { - await this.execWithTimeout(transport.binPath, args, { - env: { ...process.env, CODER_SESSION_TOKEN: token }, - signal: options?.signal, - }); - this.logger.info("Stored token via CLI for", url); + const cli = await this.resolveCli(url, configs); + if (!cli) { + return undefined; + } + if (!cli.featureSet.tokenRead) { + return undefined; + } + return await this.cliToken(cli, options?.signal); } catch (error) { - this.logger.warn("Failed to store token via CLI:", error); if (isAbortError(error)) { throw error; } - throw new CredentialCliError(error); + this.logger.info( + "Could not read the CLI session (it may not be signed in):", + error, + ); + return undefined; } } + /** True when the CLI is downloaded and settings allow its own store. Otherwise reads and token checks have nothing to do. */ + public async hasCliStore( + url: string, + configs: Pick, + ): Promise { + return ( + mayUseCliStore(configs) && (await this.resolveBinary(url)) !== undefined + ); + } + /** - * Read a token via `coder login token` (keyring or file-backed). Requires - * 2.31.0+; older deployments return undefined. Returns the token and its - * source, or undefined on any failure. Throws AbortError on abort. + * True when the CLI's own store holds `token`. Below CLI 2.32 the token + * cannot be read back, so the CLI's store counts as holding it. False without a working CLI. */ - public async readToken( + public async holdsToken( url: string, + token: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - const transport = await this.resolveReadTransport(url, configs); - if (transport.kind === "none") { - return undefined; - } - const args = [ - ...this.credentialGlobalFlags(transport, url, configs), - "login", - "token", - "--url", - url, - ]; - const token = await this.runTokenRead(transport.binPath, args, options); - if (!token) { - return undefined; + ): Promise { + try { + const cli = await this.resolveCli(url, configs); + if (cli?.auth.store !== "cli") { + return false; + } + return ( + !cli.featureSet.tokenRead || + (await this.cliToken(cli, options?.signal)) === token + ); + } catch (error) { + this.logger.warn("Could not read the CLI session:", error); + return false; } - return { - token, - source: transport.kind === "keyring" ? "keyring" : "files", - }; } - private async runTokenRead( - binPath: string, - args: string[], - options?: { signal?: AbortSignal }, + private async cliToken( + cli: ResolvedCli, + signal?: AbortSignal, ): Promise { - try { - const { stdout } = await this.execWithTimeout(binPath, args, { - signal: options?.signal, - }); - return nonEmpty(stdout); - } catch (error) { - if (isAbortError(error)) { - throw error; - } - this.logger.warn("Failed to read token via CLI:", error); - return undefined; - } + const { stdout } = await this.exec(cli, ["login", "token"], { signal }); + return stdout.trim() || undefined; } /** - * Delete credentials for a deployment. Removes the default-dir files and - * logs out of the active store (keyring or file via --global-config). - * Returns whether every store was cleared instead of throwing, except - * for AbortError when the signal is aborted. + * Deletes the extension's credential files and runs `coder logout`, which + * revokes the token. A shared CLI session is only logged out when + * `signOutCli` is set. Returns whether every store was cleared; throws only on abort. */ public deleteToken( url: string, configs: Pick, - options?: { signal?: AbortSignal }, + options: { signal?: AbortSignal; signOutCli: boolean }, ): Promise { return this.credentialTelemetry.traceClear(configs, async (span) => { const [filesCleared, cliCleared] = await Promise.all([ this.deleteCredentialFiles(url), - this.cliLogout(url, configs, { signal: options?.signal, span }), + this.cliLogout(url, configs, { ...options, span }), ]); return filesCleared && cliCleared; }); } - /** - * Log out via `coder logout`, keyring or file (--global-config). Records - * failures on the span instead of throwing (except on abort) and returns - * whether the logout succeeded. - */ private async cliLogout( url: string, configs: Pick, - { signal, span }: { signal?: AbortSignal; span: Span }, + { + signal, + signOutCli, + span, + }: { signal?: AbortSignal; signOutCli: boolean; span: Span }, ): Promise { - let transport: CliTransport; - try { - transport = await this.resolveWriteTransport(url, configs); - } catch (error) { - this.logger.warn("Could not resolve CLI binary for logout:", error); - span.setProperty("error.type", "binary"); - span.markError(); - return false; - } - const args = [ - ...this.credentialGlobalFlags(transport, url, configs), - "logout", - "--url", - url, - "--yes", - ]; try { - await this.execWithTimeout(transport.binPath, args, { signal }); - this.logger.info("Deleted token via CLI for", url); + const cli = await this.resolveCli(url, configs); + if (!cli) { + span.setProperty("outcome", "no_binary"); + this.logger.info("Skipped signing out the CLI: it is not downloaded"); + return true; + } + span.setProperty("store", cli.auth.store); + // The CLI's own session is the user's call; the extension's is always revoked. + if (cli.auth.store === "cli" && !signOutCli) { + span.setProperty("outcome", "kept"); + return true; + } + await this.exec(cli, ["logout", "--yes"], { signal }); + span.setProperty("outcome", "logged_out"); + this.logger.info("Logged out via CLI for", url); return true; } catch (error) { if (isAbortError(error)) { throw error; } - this.logger.warn("Failed to delete token via CLI:", error); - span.setProperty("error.type", "cli"); - span.markError(); + this.logger.warn("Failed to log out via CLI:", error); + recordError(span, categorizeCredentialError(error)); return false; } } - /** Resolve the CLI binary and its feature set, or throw if unavailable. */ private async resolveCli( url: string, - ): Promise<{ binPath: string; featureSet: FeatureSet }> { - const binPath = await this.resolveBinary(url); - return { binPath, featureSet: await this.getFeatureSet(binPath) }; - } - - private async resolveWriteTransport( - url: string, configs: Pick, - ): Promise { - const cli = await this.resolveCli(url); - if (isKeyringEnabled(configs) && cli.featureSet.keyringAuth) { - return { kind: "keyring", binPath: cli.binPath }; - } - return cliFileTransport(cli); - } - - private async resolveReadTransport( - url: string, - configs: Pick, - ): Promise { - // Reading is best-effort: a missing binary means no CLI credentials. - const cli = await this.resolveCli(url).catch((error) => { - this.logger.warn("Could not resolve CLI binary:", error); + ): Promise { + const binPath = await this.resolveBinary(url); + if (!binPath) { return undefined; - }); - if (!cli) { - return { kind: "none" }; } - if (isKeyringEnabled(configs) && cli.featureSet.keyringAuth) { - return cli.featureSet.tokenRead - ? { kind: "keyring", binPath: cli.binPath } - : { kind: "none" }; - } - if (cli.featureSet.tokenRead) { - return cliFileTransport(cli); - } - return { kind: "none" }; - } - - /** Keyring uses the default store; file mode passes --global-config. */ - private credentialGlobalFlags( - transport: CliTransport, - url: string, - configs: Pick, - ): string[] { - if (transport.kind === "keyring") { - return getHeaderArgs(configs); - } - return getGlobalFlags(configs, { - mode: "global-config", - configDir: this.pathResolver.getGlobalConfigDir(toSafeHost(url)), - allowOverride: transport.allowOverride, - }); - } - - private async getFeatureSet(binPath: string): Promise { - return featureSetForVersion(semver.parse(await version(binPath))); + const featureSet = featureSetForVersion( + semver.parse(await version(binPath)), + ); + const configDir = this.pathResolver.getGlobalConfigDir(toSafeHost(url)); + const auth = resolveCliAuth(configs, featureSet, url, configDir); + return { binPath, featureSet, auth, flags: getGlobalFlags(configs, auth) }; } - /** - * Wrap execFileAsync with a 60s timeout and periodic debug logging. - */ - private async execWithTimeout( - binPath: string, + /** Runs a subcommand with a 60s timeout. Failures become `CredentialCliError`; aborts pass through. */ + private async exec( + cli: ResolvedCli, args: string[], - options: { env?: NodeJS.ProcessEnv; signal?: AbortSignal } = {}, + 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 { - return await execFileAsync(binPath, args, { - ...execOptions, + return await execFileAsync(cli.binPath, [...cli.flags, ...args], { + ...options, timeout: EXEC_TIMEOUT_MS, - signal, }); + } catch (error) { + if (isAbortError(error)) { + throw error; + } + throw new CredentialCliError(error); } finally { clearInterval(timer); } } - /** - * Delete URL and token files. Returns whether all removals succeeded; - * never throws. - */ + /** Removes the url and session files. Never throws. */ private async deleteCredentialFiles(url: string): Promise { const safeHostname = toSafeHost(url); const paths = [ @@ -339,21 +271,3 @@ export class CliCredentialManager { return results.every(Boolean); } } - -function cliFileTransport(cli: { - binPath: string; - featureSet: FeatureSet; -}): CliTransport { - // Override applies only once read+write are CLI-mediated (2.31+), matching - // resolveCliAuth. - return { - kind: "cli-file", - binPath: cli.binPath, - allowOverride: cli.featureSet.tokenRead, - }; -} - -function nonEmpty(value: string): string | undefined { - const trimmed = value.trim(); - return trimmed || undefined; -} diff --git a/src/core/cliExec.ts b/src/core/cliExec.ts index 04df34ec13..0cad9fd0dd 100644 --- a/src/core/cliExec.ts +++ b/src/core/cliExec.ts @@ -1,4 +1,5 @@ import { type ExecFileException, execFile, spawn } from "node:child_process"; +import { stat } from "node:fs/promises"; import { promisify } from "node:util"; import * as vscode from "vscode"; @@ -17,11 +18,25 @@ export interface CliEnv { configs: Pick; } +const VERSION_CACHE = new Map(); + /** - * Return the version from the binary. Throw if unable to execute the binary or - * find the version for any reason. + * Return the version from the binary, cached until the file changes on disk. + * Throw if unable to execute the binary or find the version for any reason. */ export async function version(binPath: string): Promise { + const { mtimeNs, size } = await stat(binPath, { bigint: true }); + const fileKey = `${mtimeNs}:${size}`; + const cached = VERSION_CACHE.get(binPath); + if (cached?.fileKey === fileKey) { + return cached.version; + } + const result = await readVersion(binPath); + VERSION_CACHE.set(binPath, { fileKey, version: result }); + return result; +} + +async function readVersion(binPath: string): Promise { let stdout: string; try { const result = await execFileAsync(binPath, [ diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index f603910a4c..103448c42e 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -22,7 +22,7 @@ import { } from "../instrumentation/cli"; import * as pgp from "../pgp"; import { withCancellableProgress, withOptionalProgress } from "../progress"; -import { isKeyringEnabled } from "../settings/cli"; +import { showStoreCredentialsError } from "../util/credentials"; import { tempFilePath } from "../util/fs"; import { toSafeHost } from "../util/uri"; import { vscodeProposed } from "../vscodeProposed"; @@ -70,17 +70,10 @@ export class CliManager { this.cliTelemetry = new CliTelemetry(telemetry); } - /** - * 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 resolved = await this.resolveBinaryPath(safeHostname); - if (resolved.source === "not_found") { - throw new Error(`No CLI binary found at ${resolved.binPath}`); - } - return resolved.binPath; + /** The cached CLI binary for a deployment URL, or undefined when none is downloaded. Stat check only. */ + public async locateBinary(url: string): Promise { + const resolved = await this.resolveBinaryPath(toSafeHost(url)); + return resolved.source === "not_found" ? undefined : resolved.binPath; } /** @@ -1041,7 +1034,7 @@ export class CliManager { await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { trace.error(error); - this.handleStoreError(error); + this.handleStoreError(error, configs); } return; } @@ -1064,21 +1057,44 @@ export class CliManager { return; } trace.error(result.error); - this.handleStoreError(result.error); + this.handleStoreError(result.error, configs); + } + + /** True when the CLI's own store holds this token. Cancelling the check counts as false. */ + public async holdsToken(url: string, token: string): Promise { + const configs = vscode.workspace.getConfiguration(); + if (!(await this.cliCredentialManager.hasCliStore(url, configs))) { + return false; + } + const result = await withCancellableProgress( + ({ signal }) => + this.cliCredentialManager.holdsToken(url, token, configs, { signal }), + { + location: vscode.ProgressLocation.Notification, + title: "Reading credentials from the Coder CLI", + cancellable: true, + }, + ); + return result.ok && result.value; } /** - * Remove credentials for a deployment. Clears both file-based credentials - * and keyring entries (via `coder logout`). Never throws; returns whether - * every store was cleared. + * Remove credentials for a deployment. `signOutCli` also logs a shared CLI + * session out. Never throws; returns whether every store was cleared. */ - public async clearCredentials(url: string): Promise { + public async clearCredentials( + url: string, + { signOutCli }: { signOutCli: boolean }, + ): Promise { const configs = vscode.workspace.getConfiguration(); const result = await withOptionalProgress( ({ signal }) => - this.cliCredentialManager.deleteToken(url, configs, { signal }), + this.cliCredentialManager.deleteToken(url, configs, { + signal, + signOutCli, + }), { - enabled: isKeyringEnabled(configs), + enabled: signOutCli, location: vscode.ProgressLocation.Notification, title: `Removing credentials for ${url}`, cancellable: true, @@ -1095,21 +1111,11 @@ export class CliManager { return false; } - 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", - ); - } - }); + private handleStoreError( + error: unknown, + configs: Pick, + ): never { + showStoreCredentialsError(error, configs, this.output); throw error; } } diff --git a/src/featureSet.ts b/src/featureSet.ts index 1f8d53e52d..0b86f03149 100644 --- a/src/featureSet.ts +++ b/src/featureSet.ts @@ -10,6 +10,7 @@ export interface FeatureSet { tokenRead: boolean; supportBundle: boolean; supportBundleWorkspaceFiles: boolean; + allowRedirects: boolean; } /** @@ -54,11 +55,13 @@ export function featureSetForVersion( cliUpdate: versionAtLeast(version, "2.24.0"), // Keyring-backed token storage via `coder login` keyringAuth: versionAtLeast(version, "2.29.0"), - // `coder login token` for reading tokens (keyring or file) - tokenRead: versionAtLeast(version, "2.31.0"), + // `coder login token`; from 2.32 file mode also checks the URL it stored. + tokenRead: versionAtLeast(version, "2.32.0"), // `coder support bundle` (officially released/unhidden in 2.10.0) supportBundle: versionAtLeast(version, "2.10.0"), // --workspace-file flag for `coder support bundle` supportBundleWorkspaceFiles: versionAtLeast(version, "2.36.0"), + // --allow-redirects; from 2.38 the CLI otherwise errors on a redirected URL. + allowRedirects: versionAtLeast(version, "2.38.0"), }; } diff --git a/src/instrumentation/EVENTS.md b/src/instrumentation/EVENTS.md index ef0814813d..f42db9b421 100644 --- a/src/instrumentation/EVENTS.md +++ b/src/instrumentation/EVENTS.md @@ -159,19 +159,19 @@ Emitted by `AuthTelemetry`; the credential events by `CredentialTelemetry`. #### `auth.login` -| Attribute | Values | -| ------------ | ------------------------------------------------------------------------------------------------------------- | -| `source` | `auto_login`, `command`, `switch_deployment`, `uri` | -| `method` | `mtls`, `provided_token`, `stored_token`, `keyring_token`, `cli_token`, `oauth`, `unknown` (starts `unknown`) | -| `reason` | `user_dismissed`, `no_url_provided` (aborted logins only) | -| `error.type` | `auth_failed`, `exception` | +| Attribute | Values | +| ------------ | -------------------------------------------------------------------------------------------- | +| `source` | `auto_login`, `command`, `switch_deployment`, `uri` | +| `method` | `mtls`, `provided_token`, `stored_token`, `cli_token`, `oauth`, `unknown` (starts `unknown`) | +| `reason` | `user_dismissed`, `no_url_provided` (aborted logins only) | +| `error.type` | `auth_failed`, `exception` | #### `auth.logout` -| Attribute | Values | -| ------------ | ------------------------------------------ | -| `reason` | `not_authenticated` (aborted logouts only) | -| `error.type` | `exception` | +| Attribute | Values | +| ------------ | ---------------------------------------------------------------------------------- | +| `reason` | `not_authenticated`, `user_dismissed`, `cleanup_incomplete` (aborted logouts only) | +| `error.type` | `exception` | #### `auth.login_prompted` @@ -206,11 +206,12 @@ Secret-storage session read during remote setup. No custom attributes. #### `auth.credential.store` / `auth.credential.clear` -| Attribute | Values | -| ----------------- | ------------------------------------------------- | -| `keyring_enabled` | `true`, `false` (from settings) | -| `category` | `keyring`, `file` (the storage actually involved) | -| `error.type` | `binary`, `cli` | +| Attribute | Values | +| ----------------- | ------------------------------------------------------------------------ | +| `keyring_enabled` | `true`, `false` (from settings) | +| `store` | `cli` (the CLI's own store), `extension` (the extension's directory) | +| `outcome` | `stored`, `no_binary` (store); `logged_out`, `kept`, `no_binary` (clear) | +| `error.type` | `binary`, `cli` | ### Logs diff --git a/src/instrumentation/auth.ts b/src/instrumentation/auth.ts index bc21c588ff..07d4a692d4 100644 --- a/src/instrumentation/auth.ts +++ b/src/instrumentation/auth.ts @@ -18,7 +18,10 @@ export type AuthLoginOutcome = | { success: false; method?: LoginMethod; reason: LoginPromptReason }; export type AuthLogoutOutcome = | { success: true } - | { success: false; reason: "not_authenticated" | "cleanup_incomplete" }; + | { + success: false; + reason: "not_authenticated" | "user_dismissed" | "cleanup_incomplete"; + }; interface AuthLoginTrace { setMethod: (method: LoginMethod) => void; diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts index c193f0dc1f..44735397bb 100644 --- a/src/instrumentation/credentials.ts +++ b/src/instrumentation/credentials.ts @@ -11,11 +11,9 @@ export type CredentialErrorCategory = "binary" | "cli"; type CredentialEvent = "auth.credential.store" | "auth.credential.clear"; /** - * Wraps credential store/clear in a span carrying `keyring_enabled`, the - * `category` of storage involved, and an `error.type` on failure. The - * traced operation sets `category` on the span and reports failures by - * throwing a categorized error (store) or recording on the span (clear, which - * is best-effort). Aborts are recorded and re-thrown so callers still unwind. + * Wraps credential store/clear in a span with `keyring_enabled`, the `store` + * once the CLI is resolved, and `error.type` on failure. Aborts are recorded + * and re-thrown. */ export class CredentialTelemetry { public constructor(private readonly telemetry: TelemetryReporter) {} @@ -39,7 +37,6 @@ export class CredentialTelemetry { configs: Pick, fn: (span: Span) => Promise, ): Promise { - const keyringEnabled = isKeyringEnabled(configs); let aborted: Error | undefined; let result: T | undefined; await this.telemetry.trace( @@ -57,10 +54,7 @@ export class CredentialTelemetry { throw error; } }, - { - keyring_enabled: keyringEnabled, - category: keyringEnabled ? "keyring" : "file", - }, + { keyring_enabled: isKeyringEnabled(configs) }, ); if (aborted) { throw aborted; @@ -69,16 +63,22 @@ export class CredentialTelemetry { } } -function categorizeCredentialError(error: unknown): CredentialErrorCategory { +export function categorizeCredentialError( + error: unknown, +): CredentialErrorCategory { if (error instanceof CredentialCliError) { return "cli"; } return "binary"; } +/** A failed CLI command, described by its stderr when it printed any. */ export class CredentialCliError extends Error { public constructor(cause: unknown) { - super("Credential CLI operation failed", { cause }); + const stderr = (cause as { stderr?: string } | undefined)?.stderr?.trim(); + const fallback = + cause instanceof Error ? cause.message : "The Coder CLI failed"; + super(stderr || fallback, { cause }); this.name = "CredentialCliError"; } } diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 15c4fd93f2..e8057093de 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -7,9 +7,9 @@ import { needToken } from "../api/utils"; import { CertificateError } from "../error/certificateError"; import { OAuthAuthorizer } from "../oauth/authorizer"; import { buildOAuthTokenData } from "../oauth/utils"; -import { withOptionalProgress } from "../progress"; +import { withCancellableProgress } from "../progress"; import { maybeAskAuthMethod, maybeAskUrl } from "../promptUtils"; -import { isKeyringEnabled } from "../settings/cli"; +import { showStoreCredentialsError } from "../util/credentials"; import { isSameOrigin, openInBrowser } from "../util/uri"; import { vscodeProposed } from "../vscodeProposed"; @@ -32,12 +32,7 @@ import type { Logger } from "../logging/logger"; import type { OAuthCallback } from "../oauth/oauthCallback"; export type LoginMethod = - | "mtls" - | "provided_token" - | "stored_token" - | "keyring_token" - | "cli_token" - | "oauth"; + "mtls" | "provided_token" | "stored_token" | "cli_token" | "oauth"; type LoginAttemptResult = | { success: false; reason: LoginPromptReason } @@ -197,7 +192,7 @@ export class LoginCoordinator implements vscode.Disposable { } private async persistSessionAuth( - result: LoginAttemptResult, + result: LoginResult, safeHostname: string, url: string, ): Promise { @@ -212,11 +207,12 @@ export class LoginCoordinator implements vscode.Disposable { await this.mementoManager.addToUrlHistory(url); if (result.token) { + const configs = vscode.workspace.getConfiguration(); this.cliCredentialManager - .storeToken(url, result.token, vscode.workspace.getConfiguration()) - .catch((error) => { - this.logger.warn("Failed to store token at login:", error); - }); + .storeToken(url, result.token, configs) + .catch((error) => + showStoreCredentialsError(error, configs, this.logger), + ); } } } @@ -385,9 +381,12 @@ export class LoginCoordinator implements vscode.Disposable { sameOriginAuth?.token !== undefined && (await this.tryTokenAuth(client, sameOriginAuth.token, true)) === "unauthorized"; - const confirmed = await this.confirmLinkSignIn( + const confirmed = await this.confirmSignIn( deployment.url, - result.user, + { + title: "Sign in with the token from the link?", + detail: `The link contains a token that signs you in as "${result.user.username}"`, + }, auth && { username: auth.username, expired }, ); if (!confirmed) { @@ -418,43 +417,54 @@ export class LoginCoordinator implements vscode.Disposable { return withLoginMethod("stored_token", result); } - /** CLI credentials: the OS keyring when enabled, else the config dir. */ + /** The CLI's own session, adopted after confirmation if it is another user's. */ private async tryCliCredentials( ctx: LoginAttemptContext, ): Promise { - const { client, deployment, isAutoLogin, auth } = ctx; + const { client, deployment, isAutoLogin, auth, sameOriginAuth } = ctx; const configs = vscode.workspace.getConfiguration(); - const cliCredentialResult = await withOptionalProgress( + if ( + !(await this.cliCredentialManager.hasCliStore(deployment.url, configs)) + ) { + return undefined; + } + const cliCredentialResult = await withCancellableProgress( ({ signal }) => this.cliCredentialManager.readToken(deployment.url, configs, { signal, }), { - enabled: isKeyringEnabled(configs), location: vscode.ProgressLocation.Notification, - title: "Reading token from OS keyring...", + title: "Reading credentials from the Coder CLI", cancellable: true, }, ); - const cliCredential = cliCredentialResult.ok + const cliToken = cliCredentialResult.ok ? cliCredentialResult.value : undefined; - if (!cliCredential || cliCredential.token === auth?.token) { + if (!cliToken || cliToken === auth?.token) { return undefined; } this.logger.debug("Trying token from CLI credentials"); - const result = await this.tryTokenAuth( - client, - cliCredential.token, - isAutoLogin, - ); + const result = await this.tryTokenAuth(client, cliToken, isAutoLogin); if (result === "unauthorized") { return undefined; } - return withLoginMethod( - cliCredential.source === "keyring" ? "keyring_token" : "cli_token", - result, - ); + if (result.success && auth && auth.username !== result.user.username) { + const confirmed = await this.confirmSignIn( + deployment.url, + { + title: "Sign in with the Coder CLI's session?", + detail: `The Coder CLI's session signs you in as "${result.user.username}"`, + }, + // A same-origin session reached this point only because it failed. + { username: auth.username, expired: sameOriginAuth !== undefined }, + ); + if (!confirmed) { + return undefined; + } + } + return withLoginMethod("cli_token", result); } /** Last resort: ask the user how to authenticate. */ @@ -476,10 +486,10 @@ export class LoginCoordinator implements vscode.Disposable { } } - /** Ask before a token from a link signs the user in. */ - private async confirmLinkSignIn( + /** Ask before a token the user did not enter here signs them in. */ + private async confirmSignIn( url: string, - user: User, + prompt: { title: string; detail: string }, previousSession: { username: string | undefined; expired: boolean } | undefined, ): Promise { @@ -490,11 +500,11 @@ export class LoginCoordinator implements vscode.Disposable { ? `, replacing your ${previousSession.expired ? "expired" : "current"} session${previous}` : ""; const action = await vscodeProposed.window.showWarningMessage( - "Sign in with the token from the link?", + prompt.title, { useCustom: true, modal: true, - detail: `${url}\n\nThe link contains a token that signs you in as "${user.username}"${replacing}.`, + detail: `${url}\n\n${prompt.detail}${replacing}.`, }, "Sign In", ); diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 4827ec75db..36f867eb8b 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -1,4 +1,5 @@ -import { isKeyringSupported } from "../core/cliCredentialManager"; +import os from "node:os"; + import { escapeCommandArg, escapeShellArg, expandPath } from "../util"; import { getHeaderArgs } from "./headers"; @@ -7,9 +8,15 @@ import type { WorkspaceConfiguration } from "vscode"; import type { FeatureSet } from "../featureSet"; -export type CliAuth = - | { mode: "global-config"; configDir: string; allowOverride: boolean } - | { mode: "url"; url: string }; +/** The CLI's own store (its config directory or the keyring, shared with the terminal), or a directory private to the extension. */ +export type CliAuth = { + url: string; + /** The extension follows redirects itself; from 2.38 the CLI needs the flag to match. */ + allowRedirects: boolean; +} & ( + | { store: "cli"; useKeyring: boolean | undefined } + | { store: "extension"; configDir: string; useKeyring: false | undefined } +); /** * Returns the user's `coder.globalFlags` with `expandPath` applied. For @@ -51,28 +58,25 @@ function buildGlobalFlags( escAuth: (s: string) => string, escHeader: (s: string) => string, ): string[] { - const userFlags = getExpandedUserGlobalFlags(configs); - const headers = getHeaderArgs(configs, escHeader); - // Escape after stripping so expansion whitespace stays in one shell token. - const cleanUserFlags = (stripGlobalConfig: boolean) => - stripManagedFlags(userFlags, stripGlobalConfig).map(escAuth); - - // Keyring mode: --url auth; drop user --global-config (would force file storage). - if (auth.mode === "url") { - return [...cleanUserFlags(true), "--url", escAuth(auth.url), ...headers]; + const flags = stripManagedFlags( + getExpandedUserGlobalFlags(configs), + auth.store === "extension", + ).map(escAuth); + if (auth.store === "extension") { + flags.push("--global-config", escAuth(auth.configDir)); } - - // File mode: keep the user's --global-config on 2.31+, else emit our own. - const honorOverride = - auth.allowOverride && - userFlags.some((flag) => isFlag(flag, "--global-config")); - const authFlags = honorOverride - ? [] - : ["--global-config", escAuth(auth.configDir)]; - return [...cleanUserFlags(!honorOverride), ...authFlags, ...headers]; + flags.push("--url", escAuth(auth.url)); + if (auth.useKeyring !== undefined) { + flags.push(`--use-keyring=${auth.useKeyring}`); + } + if (auth.allowRedirects) { + flags.push("--allow-redirects"); + } + return [...flags, ...getHeaderArgs(configs, escHeader)]; } +/** Drops `--use-keyring`, and `--global-config` when the extension supplies its own. */ function stripManagedFlags( flags: string[], stripGlobalConfig: boolean, @@ -100,36 +104,55 @@ function isFlag(item: string, name: string): boolean { ); } -/** - * Returns true when the user has keyring enabled and the platform supports it. - */ +/** True on platforms with an OS keyring the CLI supports (macOS, Windows). */ +export function isKeyringSupported(): boolean { + const platform = os.platform(); + return platform === "darwin" || platform === "win32"; +} + +/** True when `coder.useKeyring` is on and the platform supports it. */ export function isKeyringEnabled( configs: Pick, ): boolean { - return ( - isKeyringSupported() && configs.get("coder.useKeyring", false) - ); + return isKeyringSupported() && configs.get("coder.useKeyring", true); } -/** - * Resolves how the CLI should authenticate: via the keyring (`--url`) or via - * the global config directory (`--global-config`). - */ +/** True when settings allow the CLI's own store: the keyring is on or a user config directory is set. `resolveCliAuth` adds the CLI version gates. */ +export function mayUseCliStore( + configs: Pick, +): boolean { + return isKeyringEnabled(configs) || hasUserConfigDir(configs); +} + +/** Uses the CLI's own store when the keyring is on or the user set a config directory. */ export function resolveCliAuth( configs: Pick, featureSet: FeatureSet, - deploymentUrl: string, + url: string, configDir: string, ): CliAuth { - if (isKeyringEnabled(configs) && featureSet.keyringAuth) { - return { mode: "url", url: deploymentUrl }; + // Below 2.29 the CLI lacks --use-keyring. + const useKeyring = featureSet.keyringAuth + ? isKeyringEnabled(configs) + : undefined; + // A user directory is honored on 2.32+, where the CLI reports its token. + const userDir = hasUserConfigDir(configs) && featureSet.tokenRead; + const common = { url, allowRedirects: featureSet.allowRedirects }; + if (useKeyring || userDir) { + return { ...common, store: "cli", useKeyring }; } - // Honored only on 2.31.0+, where CLI-mediated read/write share the directory. - return { - mode: "global-config", - configDir, - allowOverride: featureSet.tokenRead, - }; + return { ...common, store: "extension", configDir, useKeyring }; +} + +function hasUserConfigDir( + configs: Pick, +): boolean { + return ( + Boolean(process.env.CODER_CONFIG_DIR) || + getExpandedUserGlobalFlags(configs).some((flag) => + isFlag(flag, "--global-config"), + ) + ); } /** diff --git a/src/util/credentials.ts b/src/util/credentials.ts new file mode 100644 index 0000000000..9c53193509 --- /dev/null +++ b/src/util/credentials.ts @@ -0,0 +1,32 @@ +import * as vscode from "vscode"; + +import { errToStr } from "../api/api-helper"; +import { isKeyringEnabled } from "../settings/cli"; + +import type { WorkspaceConfiguration } from "vscode"; + +import type { Logger } from "../logging/logger"; + +/** Logs a failed credential store and shows an error that opens `coder.useKeyring`. */ +export function showStoreCredentialsError( + error: unknown, + configs: Pick, + logger: Logger, +): void { + logger.error("Failed to store credentials:", error); + let message = `Failed to store credentials: ${errToStr(error)}.`; + if (isKeyringEnabled(configs)) { + message += + ' To store the token in a file instead, set "coder.useKeyring" to false.'; + } + void vscode.window + .showErrorMessage(message, "Open Settings") + .then((action) => { + if (action === "Open Settings") { + void vscode.commands.executeCommand( + "workbench.action.openSettings", + "coder.useKeyring", + ); + } + }); +} diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 6808285630..9200a59705 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -523,6 +523,8 @@ export function createMockCliCredentialManager(): CliCredentialManager { storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), deleteToken: vi.fn().mockResolvedValue(true), + holdsToken: vi.fn().mockResolvedValue(false), + hasCliStore: vi.fn().mockResolvedValue(true), } as unknown as CliCredentialManager; } diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index df55fb48ee..c4204cb5b9 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -31,6 +31,7 @@ const featureSet: FeatureSet = { tokenRead: true, supportBundle: true, supportBundleWorkspaceFiles: true, + allowRedirects: true, }; function mockStream(): UnidirectionalStream { @@ -94,7 +95,12 @@ function createUpdateCtx( }; const ctx = { restClient: restClient as unknown as Api, - auth: { mode: "url" as const, url: "https://test.coder.com" }, + auth: { + store: "cli" as const, + url: "https://test.coder.com", + useKeyring: undefined, + allowRedirects: false, + }, binPath: "/usr/bin/coder", workspace, write: vi.fn<(data: string) => void>(), diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 48c897a2d8..feef6fb630 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -10,6 +10,7 @@ import { getGlobalShellFlags, getSshFlags, isKeyringEnabled, + mayUseCliStore, resolveCliAuth, } from "@/settings/cli"; @@ -18,228 +19,157 @@ import { quoteCommand } from "../utils/platform"; vi.mock("node:os"); -const globalConfigAuth: CliAuth = { - mode: "global-config", - configDir: "/config/dir", - allowOverride: true, +const URL = "https://dev.coder.com"; +const EXT_DIR = "/config/dir"; +const USER_DIR = "/custom/coderv2"; + +const extensionStoreAuth: CliAuth = { + store: "extension", + url: URL, + configDir: EXT_DIR, + useKeyring: undefined, + allowRedirects: false, +}; +const cliStoreAuth: CliAuth = { + store: "cli", + url: URL, + useKeyring: undefined, + allowRedirects: false, }; +const EXTENSION_FLAGS = ["--global-config", EXT_DIR, "--url", URL]; +const CLI_FLAGS = ["--url", URL]; + describe("cliConfig", () => { describe("getGlobalShellFlags", () => { - const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; - interface AuthFlagsCase { scenario: string; auth: CliAuth; - expectedAuthFlags: string[]; + expected: string[]; } it.each([ { - scenario: "global-config mode", - auth: globalConfigAuth, - expectedAuthFlags: ["--global-config", "/config/dir"], + scenario: "extension store", + auth: extensionStoreAuth, + expected: EXTENSION_FLAGS, }, + { scenario: "CLI store", auth: cliStoreAuth, expected: CLI_FLAGS }, { - scenario: "url mode", - auth: urlAuth, - expectedAuthFlags: ["--url", "https://dev.coder.com"], + scenario: "extension store with keyring off", + auth: { ...extensionStoreAuth, useKeyring: false }, + expected: [...EXTENSION_FLAGS, "--use-keyring=false"], }, - ])( - "should return auth flags for $scenario", - ({ auth, expectedAuthFlags }) => { - const config = new MockConfigurationProvider(); - expect(getGlobalShellFlags(config, auth)).toStrictEqual( - expectedAuthFlags, - ); + { + scenario: "CLI store with keyring on", + auth: { ...cliStoreAuth, useKeyring: true }, + expected: [...CLI_FLAGS, "--use-keyring=true"], }, - ); + ])("emits auth flags for a $scenario", ({ auth, expected }) => { + const config = new MockConfigurationProvider(); + expect(getGlobalShellFlags(config, auth)).toStrictEqual(expected); + }); - it("should return global flags from config with auth flags appended", () => { + it("appends auth flags after user global flags", () => { const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", [ - "--verbose", - "--disable-direct-connections", - ]); + config.set("coder.globalFlags", ["--verbose", "--global-configs"]); - expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ "--verbose", - "--disable-direct-connections", - "--global-config", - "/config/dir", + "--global-configs", // similar prefixes are not managed flags + ...EXTENSION_FLAGS, ]); }); - 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", - ]); + it("strips a user --use-keyring flag", () => { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["--verbose", "--use-keyring=false"]); - expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual([ - "--verbose", - "--disable-direct-connections", - "--global-config", - "/config/dir", - ]); - }, - ); + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + "--verbose", + ...EXTENSION_FLAGS, + ]); + }); - interface GlobalConfigCase { - scenario: string; - flags: string[]; - expected: string[]; - } - it.each([ - { - scenario: "equals form", - flags: ["-v", "--global-config=/custom/coderv2"], - expected: ["-v", "--global-config=/custom/coderv2"], - }, + const userGlobalConfigCases = [ + { scenario: "equals form", flags: ["-v", `--global-config=${USER_DIR}`] }, { scenario: "separate items", - flags: ["-v", "--global-config", "/custom/coderv2"], - expected: ["-v", "--global-config", "/custom/coderv2"], + flags: ["-v", "--global-config", USER_DIR], }, - ])( - "passes user --global-config through in file mode and drops our default ($scenario)", - ({ flags, expected }) => { + ]; + + it.each(userGlobalConfigCases)( + "passes user --global-config through in the CLI store ($scenario)", + ({ flags }) => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", flags); - expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual( - expected, - ); + expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + ...flags, + ...CLI_FLAGS, + ]); }, ); it.each([ - { scenario: "space-separated in one item", flag: "--global-config /x" }, - { scenario: "equals form", flag: "--global-config=/x" }, + ...userGlobalConfigCases, + { + scenario: "space-separated in one item", + flags: ["-v", `--global-config ${USER_DIR}`], + }, ])( - "strips user --global-config in keyring (url) mode ($scenario)", - ({ flag }) => { - const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; + "strips user --global-config in the extension store ($scenario)", + ({ flags }) => { const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", ["-v", flag]); + config.set("coder.globalFlags", flags); - expect(getGlobalShellFlags(config, urlAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ "-v", - "--url", - "https://dev.coder.com", + ...EXTENSION_FLAGS, ]); }, ); - it("strips user --global-config (separate items) in keyring (url) mode", () => { - const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; + it("keeps user header-command items and appends the setting", () => { + const headerCommand = "echo test"; const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", ["-v", "--global-config", "/x"]); + config.set("coder.headerCommand", headerCommand); + config.set("coder.globalFlags", ["-v", "--header-command custom"]); - expect(getGlobalShellFlags(config, urlAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ "-v", - "--url", - "https://dev.coder.com", - ]); - }); - - it("should not filter flags with similar prefixes", () => { - const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", ["--global-configs", "--use-keyrings"]); - - expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual([ - "--global-configs", - "--use-keyrings", - "--global-config", - "/config/dir", + '"--header-command custom"', // ignored by CLI + ...CLI_FLAGS, + "--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(getGlobalShellFlags(config, auth)).toStrictEqual([ - "-v", - '"--header-command custom"', // ignored by CLI - "--no-feature-warning", - ...expectedAuthFlags, - "--header-command", - quoteCommand(headerCommand), - ]); - }, - ); - it("quotes flags whose expanded value contains whitespace", () => { vi.mocked(os.homedir).mockReturnValue("C:\\Users\\John Doe"); const config = new MockConfigurationProvider(); config.set("coder.globalFlags", ["--cfg=${userHome}/coder"]); // Without per-entry escaping the space splits the shell command. - expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ '"--cfg=C:\\Users\\John Doe/coder"', - "--global-config", - "/config/dir", + ...EXTENSION_FLAGS, ]); }); }); describe("getGlobalFlags", () => { - const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; - - it("should not escape auth flags", () => { - const config = new MockConfigurationProvider(); - expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ - "--global-config", - "/config/dir", - ]); - expect(getGlobalFlags(config, urlAuth)).toStrictEqual([ - "--url", - "https://dev.coder.com", - ]); - }); - - it("passes header-command value through verbatim (no shell)", () => { + it("passes user flags, auth flags, and header-command verbatim", () => { const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["--verbose"]); config.set("coder.headerCommand", "echo test"); - expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ - "--global-config", - "/config/dir", - "--header-command", - "echo test", - ]); - }); - it("should include user global flags", () => { - const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", ["--verbose"]); - expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ + expect(getGlobalFlags(config, extensionStoreAuth)).toStrictEqual([ "--verbose", - "--global-config", - "/config/dir", + ...EXTENSION_FLAGS, + "--header-command", + "echo test", ]); }); }); @@ -336,18 +266,14 @@ describe("cliConfig", () => { describe("isKeyringEnabled", () => { interface KeyringEnabledCase { platform: NodeJS.Platform; - useKeyring: boolean; + useKeyring?: boolean; expected: boolean; } - it("returns false on darwin when setting is unset (default)", () => { - vi.mocked(os.platform).mockReturnValue("darwin"); - const config = new MockConfigurationProvider(); - expect(isKeyringEnabled(config)).toBe(false); - }); it.each([ - { platform: "darwin", useKeyring: true, expected: true }, - { platform: "win32", useKeyring: true, expected: true }, + { platform: "darwin", expected: true }, + { platform: "win32", expected: true }, + { platform: "linux", expected: false }, { platform: "linux", useKeyring: true, expected: false }, { platform: "darwin", useKeyring: false, expected: false }, ])( @@ -355,126 +281,159 @@ describe("cliConfig", () => { ({ platform, useKeyring, expected }) => { vi.mocked(os.platform).mockReturnValue(platform); const config = new MockConfigurationProvider(); - config.set("coder.useKeyring", useKeyring); + if (useKeyring !== undefined) { + config.set("coder.useKeyring", useKeyring); + } expect(isKeyringEnabled(config)).toBe(expected); }, ); }); - describe("resolveCliAuth", () => { - it("returns url mode when keyring should be used", () => { - vi.mocked(os.platform).mockReturnValue("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.mocked(os.platform).mockReturnValue("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", - // 2.29 < 2.31, so a user --global-config is not honored. - allowOverride: false, - }); - }); + describe("mayUseCliStore", () => { + interface MayUseCliStoreCase { + platform: NodeJS.Platform; + flags: string[]; + expected: boolean; + } - it("uses caller-provided config directory in global-config mode", () => { - vi.mocked(os.platform).mockReturnValue("linux"); - const config = new MockConfigurationProvider(); - const featureSet = featureSetForVersion(semver.parse("2.29.0")); - const auth = resolveCliAuth( - config, - featureSet, - "https://dev.coder.com", - "/custom/coderv2", - ); + it.each([ + { platform: "linux", flags: [], expected: false }, + { platform: "darwin", flags: [], expected: true }, + { + platform: "linux", + flags: [`--global-config=${USER_DIR}`], + expected: true, + }, + ])( + "is $expected on $platform with flags $flags", + ({ platform, flags, expected }) => { + vi.mocked(os.platform).mockReturnValue(platform); + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", flags); - expect(getGlobalFlags(config, auth)).toStrictEqual([ - "--global-config", - "/custom/coderv2", - ]); - }); + expect(mayUseCliStore(config)).toBe(expected); + }, + ); + }); - it("keeps keyring precedence over caller-provided config directory", () => { - vi.mocked(os.platform).mockReturnValue("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", - "/custom/coderv2", - ); + describe("resolveCliAuth", () => { + function resolve(config: MockConfigurationProvider, version: string) { + const featureSet = featureSetForVersion(semver.parse(version)); + return resolveCliAuth(config, featureSet, URL, EXT_DIR); + } - expect(getGlobalFlags(config, auth)).toStrictEqual([ - "--url", - "https://dev.coder.com", - ]); + beforeEach(() => { + vi.stubEnv("CODER_CONFIG_DIR", undefined); }); - it("lets globalFlags --global-config override the caller-provided directory on 2.31+", () => { - vi.mocked(os.platform).mockReturnValue("linux"); - const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", [ - "--verbose", - "--global-config=/custom/coderv2", - ]); - const featureSet = featureSetForVersion(semver.parse("2.31.0")); - const auth = resolveCliAuth( - config, - featureSet, - "https://dev.coder.com", - "/default/coderv2", - ); - - // User's directory passes through; our default is dropped. - expect(getGlobalFlags(config, auth)).toStrictEqual([ - "--verbose", - "--global-config=/custom/coderv2", - ]); + afterEach(() => { + vi.unstubAllEnvs(); }); - it("ignores globalFlags --global-config on deployments older than 2.31", () => { - vi.mocked(os.platform).mockReturnValue("linux"); + interface ResolveCliAuthCase { + scenario: string; + platform: NodeJS.Platform; + override: "none" | "flag" | "env"; + version: string; + expected: string[]; + } + + it.each([ + { + scenario: "uses the CLI store when keyring is enabled on 2.29+", + platform: "darwin", + override: "none", + version: "2.29.0", + expected: ["--verbose", ...CLI_FLAGS, "--use-keyring=true"], + }, + { + scenario: "follows redirects on 2.38+", + platform: "darwin", + override: "none", + version: "2.38.0", + expected: [ + "--verbose", + ...CLI_FLAGS, + "--use-keyring=true", + "--allow-redirects", + ], + }, + { + scenario: "uses the extension directory when keyring is unsupported", + platform: "linux", + override: "none", + version: "2.29.0", + expected: ["--verbose", ...EXTENSION_FLAGS, "--use-keyring=false"], + }, + { + scenario: + "omits --use-keyring below 2.29, where the CLI lacks the flag", + platform: "darwin", + override: "none", + version: "2.28.0", + expected: ["--verbose", ...EXTENSION_FLAGS], + }, + { + scenario: "honors a globalFlags --global-config on 2.32+", + platform: "darwin", + override: "flag", + version: "2.32.0", + expected: [ + "--verbose", + `--global-config=${USER_DIR}`, + ...CLI_FLAGS, + "--use-keyring=true", + ], + }, + { + scenario: "honors CODER_CONFIG_DIR on 2.32+ by emitting no directory", + platform: "darwin", + override: "env", + version: "2.32.0", + expected: ["--verbose", ...CLI_FLAGS, "--use-keyring=true"], + }, + { + scenario: "honors a globalFlags --global-config with keyring disabled", + platform: "linux", + override: "flag", + version: "2.32.0", + expected: [ + "--verbose", + `--global-config=${USER_DIR}`, + ...CLI_FLAGS, + "--use-keyring=false", + ], + }, + { + scenario: + "keeps the extension directory over a user directory below 2.32", + platform: "linux", + override: "flag", + version: "2.31.0", + expected: ["--verbose", ...EXTENSION_FLAGS, "--use-keyring=false"], + }, + { + scenario: + "keeps the extension directory over CODER_CONFIG_DIR below 2.32", + platform: "linux", + override: "env", + version: "2.31.0", + expected: ["--verbose", ...EXTENSION_FLAGS, "--use-keyring=false"], + }, + ])("$scenario", ({ platform, override, version, expected }) => { + vi.mocked(os.platform).mockReturnValue(platform); const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", [ - "--verbose", - "--global-config=/custom/coderv2", - ]); - const featureSet = featureSetForVersion(semver.parse("2.30.0")); - const auth = resolveCliAuth( - config, - featureSet, - "https://dev.coder.com", - "/default/coderv2", + const userFlags = ["--verbose"]; + if (override === "flag") { + userFlags.push(`--global-config=${USER_DIR}`); + } else if (override === "env") { + vi.stubEnv("CODER_CONFIG_DIR", USER_DIR); + } + config.set("coder.globalFlags", userFlags); + + expect(getGlobalFlags(config, resolve(config, version))).toStrictEqual( + expected, ); - - // User override stripped; our default is used so it matches where we wrote. - expect(getGlobalFlags(config, auth)).toStrictEqual([ - "--verbose", - "--global-config", - "/default/coderv2", - ]); }); }); }); diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index 17b59d6f9f..ffb0d1c892 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -15,7 +15,7 @@ import type { CliManager } from "@/core/cliManager"; import type { ServiceContainer } from "@/core/container"; import type { MementoManager } from "@/core/mementoManager"; import type { PathResolver } from "@/core/pathResolver"; -import type { SecretsManager } from "@/core/secretsManager"; +import type { SecretsManager, SessionAuth } from "@/core/secretsManager"; import type { DeploymentManager } from "@/deployment/deploymentManager"; import type { Deployment } from "@/deployment/types"; import type { LoginCoordinator, LoginResult } from "@/login/loginCoordinator"; @@ -48,6 +48,11 @@ interface SetupOptions { readonly clearCredentialsResult?: boolean; } +const TEST_SESSION: SessionAuth = { + url: TEST_URL, + token: "test-token", +}; + function setup(options: SetupOptions = {}) { vi.clearAllMocks(); const interaction = new MockUserInteraction(); @@ -83,17 +88,19 @@ function setup(options: SetupOptions = {}) { clearDeployment: vi.fn(() => Promise.resolve()), }; - const cliManager: Pick = { + const cliManager: Pick = { clearCredentials: vi.fn(() => Promise.resolve(options.clearCredentialsResult ?? true), ), + holdsToken: vi.fn(() => Promise.resolve(false)), }; const secretsManager: Pick< SecretsManager, - "getCurrentDeployment" | "clearAllAuthData" + "getCurrentDeployment" | "getSessionAuth" | "clearAllAuthData" > = { getCurrentDeployment: vi.fn(() => Promise.resolve(null)), + getSessionAuth: vi.fn(() => Promise.resolve(TEST_SESSION)), clearAllAuthData: vi.fn(() => { if (options.clearAllAuthDataError) { return Promise.reject(options.clearAllAuthDataError); @@ -258,12 +265,85 @@ describe("Commands", () => { expect(mocks.deploymentManager.clearDeployment).toHaveBeenCalledWith( "logout", ); - expect(mocks.cliManager.clearCredentials).toHaveBeenCalledWith(TEST_URL); + expect(mocks.cliManager.holdsToken).toHaveBeenCalledWith( + TEST_URL, + TEST_SESSION.token, + ); + expect(mocks.cliManager.clearCredentials).toHaveBeenCalledWith(TEST_URL, { + signOutCli: false, + }); expect(mocks.secretsManager.clearAllAuthData).toHaveBeenCalledWith( TEST_HOSTNAME, ); }); + const CLI_PROMPT = "Sign out of the Coder CLI too?"; + + interface PromptCase { + scenario: string; + oauth?: boolean; + answer?: string; + /** Undefined when the logout is aborted as user_dismissed. */ + signOutCli?: boolean; + } + + it.each([ + { + scenario: "keeps the CLI session on request", + answer: "Keep Signed In", + signOutCli: false, + }, + { + scenario: "signs out the CLI on request", + answer: "Sign Out", + signOutCli: true, + }, + { + scenario: "signs out the CLI without asking for OAuth", + oauth: true, + signOutCli: true, + }, + { scenario: "aborts when the prompt is dismissed" }, + ])( + "$scenario for a shared store", + async ({ oauth, answer, signOutCli }) => { + const { commands, mocks, interaction, sink } = setup({ + authenticated: true, + }); + vi.mocked(mocks.cliManager.holdsToken).mockResolvedValueOnce(true); + if (oauth) { + vi.mocked(mocks.secretsManager.getSessionAuth).mockResolvedValueOnce({ + ...TEST_SESSION, + oauth: { scope: "workspace:read", expiry_timestamp: 1 }, + }); + } + interaction.setResponse(CLI_PROMPT, answer); + + await commands.logout(); + + const prompted = interaction + .getMessageCalls() + .some((call) => call.message === CLI_PROMPT); + expect(prompted).toBe(!oauth); + if (signOutCli === undefined) { + expect(sink.expectOne("auth.logout").properties).toMatchObject({ + result: "aborted", + reason: "user_dismissed", + }); + expect( + mocks.deploymentManager.clearDeployment, + ).not.toHaveBeenCalled(); + return; + } + expect(mocks.cliManager.clearCredentials).toHaveBeenCalledWith( + TEST_URL, + { + signOutCli, + }, + ); + }, + ); + it("records logout exceptions", async () => { const { commands, sink } = setup({ authenticated: true, diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index e02d45f4b4..a6c5934cde 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -2,16 +2,14 @@ import { fs as memfs, vol } from "memfs"; import { execFile } from "node:child_process"; import * as os from "node:os"; import path from "node:path"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { CliCredentialManager, - isKeyringSupported, type BinaryResolver, } from "@/core/cliCredentialManager"; import * as cliExec from "@/core/cliExec"; import { PathResolver } from "@/core/pathResolver"; -import { isKeyringEnabled } from "@/settings/cli"; import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; import { @@ -21,646 +19,524 @@ import { import type * as nodeFs from "node:fs"; -vi.mock("node:child_process", () => ({ - execFile: vi.fn(), -})); +vi.mock("node:child_process", () => ({ execFile: vi.fn() })); vi.mock("node:os"); -vi.mock("@/settings/cli", async () => { - const actual = - await vi.importActual("@/settings/cli"); - return { ...actual, isKeyringEnabled: vi.fn().mockReturnValue(false) }; -}); - vi.mock("@/core/cliExec", async () => { const actual = await vi.importActual("@/core/cliExec"); - return { - ...actual, - version: vi.fn().mockResolvedValue("2.29.0"), - }; + return { ...actual, version: vi.fn() }; }); vi.mock("fs/promises", async () => { const memfs: { fs: typeof nodeFs } = await vi.importActual("memfs"); - return { - ...memfs.fs.promises, - default: memfs.fs.promises, - }; + 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 { +const PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); +// Built with path.join so it matches getGlobalConfigDir on Windows too. +const CRED_DIR = path.join("/mock/base", "dev.coder.com"); +const USER_DIR = "/custom/coderv2"; + +const EXTENSION_FLAGS = [ + "--global-config", + CRED_DIR, + "--url", + TEST_URL, + "--use-keyring=false", +]; +const KEYRING_FLAGS = ["--url", TEST_URL, "--use-keyring=true"]; +const USER_DIR_FLAGS = [ + `--global-config=${USER_DIR}`, + "--url", + TEST_URL, + "--use-keyring=false", +]; + +type ExecResult = string | Error; +type ExecCallback = (err: Error | null, result?: { stdout: string }) => void; +interface ExecOptions { env?: NodeJS.ProcessEnv; timeout?: number; signal?: AbortSignal; } -type ExecFileCallback = ( - err: Error | null, - result?: { stdout: string }, -) => void; - -function stubExecFile(result: { stdout?: string } | { error: string }) { +/** Answers each subcommand with stdout or a failure; "abort" waits for the signal. */ +function stubExecFile( + results: + | { login?: ExecResult; token?: ExecResult; logout?: ExecResult } + | "abort" = {}, +) { vi.mocked(execFile).mockImplementation((( _bin: string, - _args: string[], - _opts: ExecFileOptions, - cb: ExecFileCallback, + args: string[], + opts: ExecOptions, + cb: ExecCallback, ) => { - if ("error" in result) { - cb(new Error(result.error)); - } else { - cb(null, { stdout: result.stdout ?? "" }); + if (results === "abort") { + const err = new Error("The operation was aborted"); + err.name = "AbortError"; + if (opts.signal?.aborted) { + cb(err); + } else { + opts.signal?.addEventListener("abort", () => cb(err)); + } + return; } - }) as unknown as typeof execFile); -} - -function stubExecFileAbortable() { - vi.mocked(execFile).mockImplementation((( - _bin: string, - _args: string[], - opts: ExecFileOptions, - cb: ExecFileCallback, - ) => { - const err = new Error("The operation was aborted"); - err.name = "AbortError"; - if (opts.signal?.aborted) { - cb(err); + const result = args.includes("token") + ? results.token + : args.includes("logout") + ? results.logout + : results.login; + if (result instanceof Error) { + cb(result); } else { - opts.signal?.addEventListener("abort", () => cb(err)); + cb(null, { stdout: result ?? "" }); } }) as unknown as typeof execFile); } -function lastExecArgs() { - const [bin, args, opts] = vi.mocked(execFile).mock.calls[0] as [ - string, - readonly string[], - ExecFileOptions, - ...unknown[], - ]; - return { - bin, - args, - env: opts.env ?? process.env, - timeout: opts.timeout, - signal: opts.signal, - }; -} - -function successResolver(): BinaryResolver { - return vi.fn().mockResolvedValue(TEST_BIN); -} - -function failingResolver(): BinaryResolver { - return vi.fn().mockRejectedValue(new Error("no binary")); -} +const execCalls = () => + vi.mocked(execFile).mock.calls.map((call) => call[1] as string[]); +const execOptions = () => vi.mocked(execFile).mock.calls[0][2] as ExecOptions; -// Honor the defaultValue arg so getExpandedUserGlobalFlags sees [] when unset. -const configs = { - get: vi.fn((_key: string, defaultValue?: unknown) => defaultValue), -}; - -const configWithHeaders = { - get: vi.fn((key: string, defaultValue?: unknown) => - key === "coder.headerCommand" ? "my-header-cmd" : defaultValue, - ), -}; - -// A configs that sets a user --global-config override via coder.globalFlags. -function configWithGlobalConfig(dir: string) { +/** A configs fake that honors defaultValue for everything but `values`. */ +function configWith(values: Record) { return { get: vi.fn((key: string, defaultValue?: unknown) => - key === "coder.globalFlags" ? [`--global-config=${dir}`] : defaultValue, + key in values ? values[key] : defaultValue, ), }; } +const configs = configWith({}); +const userDirConfigs = configWith({ + "coder.globalFlags": [`--global-config=${USER_DIR}`], +}); -const TEST_PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); -// Built with path.join so it matches getGlobalConfigDir on Windows too. -const CRED_DIR = path.join("/mock/base", "dev.coder.com"); -const CUSTOM_CRED_DIR = "/custom/coderv2"; - -function credentialPaths(dir = CRED_DIR) { - return { - url: `${dir}/url`, - session: `${dir}/session`, - }; -} - -function writeCredentialFiles( - url: string, - token: string, - dir = CRED_DIR, -): void { - const paths = credentialPaths(dir); - vol.mkdirSync(dir, { recursive: true }); - memfs.writeFileSync(paths.url, url); - memfs.writeFileSync(paths.session, token); +function writeCredentialFiles(): void { + vol.mkdirSync(CRED_DIR, { recursive: true }); + memfs.writeFileSync(`${CRED_DIR}/url`, TEST_URL); + memfs.writeFileSync(`${CRED_DIR}/session`, "old-token"); } -function credentialFilesExist(dir = CRED_DIR): boolean { - const paths = credentialPaths(dir); - return memfs.existsSync(paths.url) || memfs.existsSync(paths.session); -} +const credentialFilesExist = () => + memfs.existsSync(`${CRED_DIR}/url`) || + memfs.existsSync(`${CRED_DIR}/session`); -function setup(resolver?: BinaryResolver) { - const r = resolver ?? successResolver(); +function setup(resolver: BinaryResolver = vi.fn().mockResolvedValue(TEST_BIN)) { const sink = new TestSink(); - return { - resolver: r, - sink, - manager: new CliCredentialManager( - createMockLogger(), - r, - TEST_PATH_RESOLVER, - createTestTelemetryService(sink), - ), - }; + const manager = new CliCredentialManager( + createMockLogger(), + resolver, + PATH_RESOLVER, + createTestTelemetryService(sink), + ); + return { sink, manager }; } -describe("isKeyringSupported", () => { - 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.mocked(os.platform).mockReturnValue(platform as NodeJS.Platform); - expect(isKeyringSupported()).toBe(expected); - }); -}); - describe("CliCredentialManager", () => { beforeEach(() => { new MockConfigurationProvider(); vi.clearAllMocks(); vol.reset(); - vi.mocked(isKeyringEnabled).mockReturnValue(false); - vi.mocked(cliExec.version).mockResolvedValue("2.31.0"); + vi.stubEnv("CODER_CONFIG_DIR", undefined); + // Linux: keyring unsupported, so the extension directory is used. + vi.mocked(os.platform).mockReturnValue("linux"); + vi.mocked(cliExec.version).mockResolvedValue("2.32.0"); }); - describe("storeToken", () => { - it("writes via coder login (file mode) when keyring is disabled", async () => { - stubExecFile({ stdout: "" }); - const { manager, resolver, sink } = setup(); - - await expect( - manager.storeToken(TEST_URL, "my-token", configs), - ).resolves.toBeUndefined(); - - expect(resolver).toHaveBeenCalledWith(TEST_URL); - const exec = lastExecArgs(); - expect(exec.args).toEqual([ - "--global-config", - CRED_DIR, - "login", - "--use-token-as-session", - TEST_URL, - ]); - expect(exec.env.CODER_SESSION_TOKEN).toBe("my-token"); - expect(exec.args).not.toContain("my-token"); - expect(sink.expectOne("auth.credential.store")).toMatchObject({ - properties: { - category: "file", - keyring_enabled: "false", - result: "success", - }, - }); - }); - - it("resolves binary and invokes coder login when keyring enabled", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "" }); - const { manager, resolver, sink } = setup(); - - await expect( - manager.storeToken(TEST_URL, "my-secret-token", configs), - ).resolves.toBeUndefined(); - - 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"); - expect(sink.expectOne("auth.credential.store")).toMatchObject({ - properties: { - category: "keyring", - keyring_enabled: "true", - result: "success", - }, - }); - }); - - it("writes via coder login under a user --global-config override", async () => { - stubExecFile({ stdout: "" }); - const { manager } = setup(); - - await manager.storeToken( - TEST_URL, - "my-token", - configWithGlobalConfig(CUSTOM_CRED_DIR), - ); - - expect(lastExecArgs().args).toEqual([ - `--global-config=${CUSTOM_CRED_DIR}`, - "login", - "--use-token-as-session", - TEST_URL, - ]); - }); - - it("throws and writes no files when the binary cannot be resolved", async () => { - const { manager } = setup(failingResolver()); - - await expect( - manager.storeToken(TEST_URL, "my-token", configs), - ).rejects.toThrow("no binary"); - expect(execFile).not.toHaveBeenCalled(); - expect(credentialFilesExist()).toBe(false); - }); + afterEach(() => { + vi.unstubAllEnvs(); + }); - it("writes via coder login (file) when keyring is enabled but unsupported", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); - stubExecFile({ stdout: "" }); - const { manager } = setup(); + // Store selection is covered by cliConfig.test.ts; this checks the wiring. + interface StoreCase { + scenario: string; + platform: NodeJS.Platform; + configs: typeof configs; + expected: string[]; + store: string; + } + + it.each([ + { + scenario: "extension directory when keyring is unsupported", + platform: "linux", + configs, + expected: EXTENSION_FLAGS, + store: "extension", + }, + { + scenario: "CLI default store when keyring is enabled", + platform: "darwin", + configs, + expected: KEYRING_FLAGS, + store: "cli", + }, + { + scenario: "user --global-config directory", + platform: "linux", + configs: userDirConfigs, + expected: USER_DIR_FLAGS, + store: "cli", + }, + ])( + "targets the $scenario", + async ({ platform, configs, expected, store }) => { + vi.mocked(os.platform).mockReturnValue(platform); + stubExecFile(); + const { manager, sink } = setup(); await manager.storeToken(TEST_URL, "token", configs); - expect(lastExecArgs().args).toEqual([ - "--global-config", - CRED_DIR, - "login", - "--use-token-as-session", - TEST_URL, + expect(execCalls()).toEqual([ + [...expected, "login", "--use-token-as-session", TEST_URL], ]); - }); - - it("throws when CLI exec fails", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ error: "login failed" }); - const { manager, sink } = setup(); - - await expect( - manager.storeToken(TEST_URL, "token", configs), - ).rejects.toThrow("Credential CLI operation failed"); expect(sink.expectOne("auth.credential.store")).toMatchObject({ - properties: { - "error.type": "cli", - result: "error", - }, + properties: { store, result: "success" }, }); - }); - - it("throws when binary resolver fails and keyring enabled", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - 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 () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "" }); - const { manager } = setup(); - - await manager.storeToken(TEST_URL, "token", configWithHeaders); + }, + ); - 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: "" }); + describe("storeToken", () => { + it("passes the token through the environment only", async () => { + stubExecFile(); const { manager } = setup(); - const ac = new AbortController(); - await manager.storeToken(TEST_URL, "token", configs, { - signal: ac.signal, - }); + await manager.storeToken(TEST_URL, "my-secret-token", configs); - expect(lastExecArgs().signal).toBe(ac.signal); + expect(execOptions().env?.CODER_SESSION_TOKEN).toBe("my-secret-token"); + expect(execCalls()[0]).not.toContain("my-secret-token"); }); - it("rejects with AbortError when signal is pre-aborted", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFileAbortable(); - const { manager, sink } = setup(); + interface CliErrorCase { + scenario: string; + error: Error; + message: string; + } - await expect( - manager.storeToken(TEST_URL, "token", configs, { - signal: AbortSignal.abort(), + it.each([ + { + scenario: "the CLI's stderr", + error: Object.assign(new Error("Command failed"), { + stderr: "keychain is locked\n", }), - ).rejects.toThrow("The operation was aborted"); - const event = sink.expectOne("auth.credential.store"); - expect(event).toMatchObject({ - properties: { result: "aborted" }, - }); - expect(event.properties["error.type"]).toBeUndefined(); - }); + message: "keychain is locked", + }, + { + scenario: "the error message without stderr", + error: new Error("login failed"), + message: "login failed", + }, + ])( + "throws a CredentialCliError carrying $scenario", + async ({ error, message }) => { + stubExecFile({ login: error }); + const { manager, sink } = setup(); + + await expect( + manager.storeToken(TEST_URL, "token", configs), + ).rejects.toThrow(message); + expect(sink.expectOne("auth.credential.store")).toMatchObject({ + properties: { "error.type": "cli", result: "error" }, + }); + }, + ); }); 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).toEqual({ token: "my-token", source: "keyring" }); - 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" }); + it("returns the trimmed token from the CLI store", async () => { + vi.mocked(os.platform).mockReturnValue("darwin"); + stubExecFile({ token: " my-token\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(); + expect(await manager.readToken(TEST_URL, configs)).toBe("my-token"); + expect(execCalls()).toEqual([[...KEYRING_FLAGS, "login", "token"]]); }); - 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("reads via coder login token (file mode) when keyring is disabled", async () => { - stubExecFile({ stdout: "file-token\n" }); - const { manager, resolver } = setup(); - - expect(await manager.readToken(TEST_URL, configs)).toEqual({ - token: "file-token", - source: "files", - }); - expect(resolver).toHaveBeenCalledWith(TEST_URL); - expect(lastExecArgs().args).toEqual([ - "--global-config", - CRED_DIR, - "login", - "token", - "--url", - TEST_URL, - ]); - }); + interface ReadTokenCase { + scenario: string; + token: ExecResult; + } - it("reads via coder login token under a user --global-config override", async () => { - stubExecFile({ stdout: "custom-file-token" }); + it.each([ + { scenario: "whitespace-only stdout", token: " \n" }, + { scenario: "a CLI error", token: new Error("no token found") }, + ])("returns undefined on $scenario", async ({ token }) => { + stubExecFile({ token }); const { manager } = setup(); - expect( - await manager.readToken( - TEST_URL, - configWithGlobalConfig(CUSTOM_CRED_DIR), - ), - ).toEqual({ token: "custom-file-token", source: "files" }); - expect(lastExecArgs().args).toEqual([ - `--global-config=${CUSTOM_CRED_DIR}`, - "login", - "token", - "--url", - TEST_URL, - ]); - }); - - it("returns undefined for file mode on deployments older than 2.31", async () => { - vi.mocked(cliExec.version).mockResolvedValue("2.30.0"); - const { manager, resolver } = setup(); - expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); - expect(resolver).toHaveBeenCalledWith(TEST_URL); - expect(execFile).not.toHaveBeenCalled(); }); - it("does not read when keyring token read is unsupported", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(cliExec.version).mockResolvedValueOnce("2.30.0"); - const { manager, resolver } = setup(); - - expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); - expect(resolver).toHaveBeenCalledWith(TEST_URL); - expect(execFile).not.toHaveBeenCalled(); - }); - - it("returns undefined when keyring is enabled but unsupported by the CLI", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); - const { manager, resolver } = setup(); - - expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); - expect(resolver).toHaveBeenCalledWith(TEST_URL); - expect(execFile).not.toHaveBeenCalled(); - }); - - it("returns undefined when CLI version too old for token read", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - // 2.30 supports keyringAuth but not tokenRead (requires 2.31+) - vi.mocked(cliExec.version).mockResolvedValueOnce("2.30.0"); - stubExecFile({ stdout: "my-token" }); + it("returns undefined below CLI 2.32 without running the CLI", async () => { + vi.mocked(cliExec.version).mockResolvedValue("2.31.0"); const { manager } = setup(); 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); - }); - - it("passes signal through to execFile", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "token" }); - const { manager } = setup(); - const ac = new AbortController(); - - await manager.readToken(TEST_URL, configs, { signal: ac.signal }); - - expect(lastExecArgs().signal).toBe(ac.signal); - }); - - it("throws AbortError when signal is aborted", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFileAbortable(); - const { manager } = setup(); - - await expect( - manager.readToken(TEST_URL, configs, { - signal: AbortSignal.abort(), - }), - ).rejects.toThrow("The operation was aborted"); - }); }); - describe("deleteToken", () => { - 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, sink } = setup(); - - const result = await manager.deleteToken(TEST_URL, configs); - - expect(result).toBe(true); - expect(resolver).toHaveBeenCalledWith(TEST_URL); - const exec = lastExecArgs(); - expect(exec.bin).toBe(TEST_BIN); - expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); - expect(credentialFilesExist()).toBe(false); - expect(sink.expectOne("auth.credential.clear")).toMatchObject({ - properties: { - category: "keyring", - keyring_enabled: "true", - result: "success", - }, - }); - }); + describe("holdsToken", () => { + interface HoldsTokenCase { + scenario: string; + platform: NodeJS.Platform; + version?: string; + cliToken?: string; + expected: boolean; + } - it("deletes files and invokes coder logout (file) when keyring is disabled", async () => { - stubExecFile({ stdout: "" }); - writeCredentialFiles(TEST_URL, "old-token"); - const { manager } = setup(); + it.each([ + { scenario: "the extension store", platform: "linux", expected: false }, + { + scenario: "the CLI store holding the token", + platform: "darwin", + expected: true, + }, + { + scenario: "the CLI store holding another token", + platform: "darwin", + cliToken: "other", + expected: false, + }, + { + scenario: "the CLI store below 2.32, which cannot be read", + platform: "darwin", + version: "2.31.0", + cliToken: "other", + expected: true, + }, + ])( + "is $expected for $scenario", + async ({ + platform, + version = "2.32.0", + cliToken = "my-token", + expected, + }) => { + vi.mocked(os.platform).mockReturnValue(platform); + vi.mocked(cliExec.version).mockResolvedValue(version); + stubExecFile({ token: cliToken }); + + expect( + await setup().manager.holdsToken(TEST_URL, "my-token", configs), + ).toBe(expected); + }, + ); + }); - await manager.deleteToken(TEST_URL, configs); + interface HasCliStoreCase { + scenario: string; + platform: NodeJS.Platform; + binary: string | undefined; + expected: boolean; + } + + it.each([ + { + scenario: "the extension store", + platform: "linux", + binary: TEST_BIN, + expected: false, + }, + { + scenario: "the CLI store", + platform: "darwin", + binary: TEST_BIN, + expected: true, + }, + { + scenario: "the CLI store without a binary", + platform: "darwin", + binary: undefined, + expected: false, + }, + ])( + "hasCliStore is $expected for $scenario", + async ({ platform, binary, expected }) => { + vi.mocked(os.platform).mockReturnValue(platform); + const { manager } = setup(vi.fn().mockResolvedValue(binary)); + + expect(await manager.hasCliStore(TEST_URL, configs)).toBe(expected); + }, + ); - expect(lastExecArgs().args).toEqual([ - "--global-config", - CRED_DIR, - "logout", - "--url", - TEST_URL, - "--yes", - ]); - expect(credentialFilesExist()).toBe(false); - }); + describe("deleteToken", () => { + interface DeleteTokenCase { + scenario: string; + platform: NodeJS.Platform; + signOutCli: boolean; + logout: string[] | undefined; + outcome: string; + } - it("never throws on CLI error", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ error: "logout failed" }); + it.each([ + { + scenario: "always logs out of the extension store", + platform: "linux", + signOutCli: false, + logout: EXTENSION_FLAGS, + outcome: "logged_out", + }, + { + scenario: "logs out of the CLI store when asked", + platform: "darwin", + signOutCli: true, + logout: KEYRING_FLAGS, + outcome: "logged_out", + }, + { + scenario: "keeps the CLI session unless asked", + platform: "darwin", + signOutCli: false, + logout: undefined, + outcome: "kept", + }, + ])("$scenario", async ({ platform, signOutCli, logout, outcome }) => { + vi.mocked(os.platform).mockReturnValue(platform); + stubExecFile(); + writeCredentialFiles(); const { manager, sink } = setup(); - await expect(manager.deleteToken(TEST_URL, configs)).resolves.toBe(false); - expect(sink.expectOne("auth.credential.clear")).toMatchObject({ - properties: { - "error.type": "cli", - result: "error", - }, - }); - }); - - it("never throws when binary resolver fails", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - const { manager, sink } = setup(failingResolver()); - - await expect(manager.deleteToken(TEST_URL, configs)).resolves.toBe(false); - expect(execFile).not.toHaveBeenCalled(); - expect(sink.expectOne("auth.credential.clear")).toMatchObject({ - properties: { - category: "keyring", - "error.type": "binary", - result: "error", - }, + const result = await manager.deleteToken(TEST_URL, configs, { + signOutCli, }); - }); - - 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("logs out via coder logout (file) when keyring is enabled but unsupported", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); - stubExecFile({ stdout: "" }); - writeCredentialFiles(TEST_URL, "old-token"); - const { manager } = setup(); - await manager.deleteToken(TEST_URL, configs); - - expect(lastExecArgs().args).toEqual([ - "--global-config", - CRED_DIR, - "logout", - "--url", - TEST_URL, - "--yes", - ]); + expect(result).toBe(true); + expect(execCalls()).toEqual( + logout ? [[...logout, "logout", "--yes"]] : [], + ); expect(credentialFilesExist()).toBe(false); + expect(sink.expectOne("auth.credential.clear").properties).toMatchObject({ + result: "success", + outcome, + }); }); - 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, { signal: ac.signal }); - - expect(lastExecArgs().signal).toBe(ac.signal); - }); - - it("throws AbortError when signal is aborted", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFileAbortable(); + it("reports a failed logout without throwing", async () => { + stubExecFile({ logout: new Error("logout failed") }); const { manager, sink } = setup(); await expect( - manager.deleteToken(TEST_URL, configs, { - signal: AbortSignal.abort(), - }), - ).rejects.toThrow("The operation was aborted"); - const event = sink.expectOne("auth.credential.clear"); - expect(event).toMatchObject({ - properties: { result: "aborted" }, + manager.deleteToken(TEST_URL, configs, { signOutCli: true }), + ).resolves.toBe(false); + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ + properties: { "error.type": "cli", result: "error" }, }); - expect(event.properties["error.type"]).toBeUndefined(); }); }); + + describe("every CLI call", () => { + type Run = ( + manager: CliCredentialManager, + options: { signal: AbortSignal }, + ) => Promise; + interface Operation { + name: string; + run: Run; + event?: string; + whenMissing: (result: Promise) => Promise; + whenBroken: (result: Promise) => Promise; + } + const operations: Operation[] = [ + { + name: "storeToken", + run: (m, o) => m.storeToken(TEST_URL, "token", configs, o), + event: "auth.credential.store", + whenMissing: (r) => expect(r).resolves.toBeUndefined(), + whenBroken: (r) => expect(r).rejects.toThrow("broken"), + }, + { + name: "readToken", + run: (m, o) => m.readToken(TEST_URL, configs, o), + whenMissing: (r) => expect(r).resolves.toBeUndefined(), + whenBroken: (r) => expect(r).resolves.toBeUndefined(), + }, + { + name: "deleteToken", + run: (m, o) => + m.deleteToken(TEST_URL, configs, { ...o, signOutCli: true }), + event: "auth.credential.clear", + whenMissing: (r) => expect(r).resolves.toBe(true), + whenBroken: (r) => expect(r).resolves.toBe(false), + }, + ]; + + it.each(operations)( + "$name passes the timeout and signal", + async ({ run }) => { + stubExecFile({ token: "token" }); + const { manager } = setup(); + const ac = new AbortController(); + + await run(manager, { signal: ac.signal }); + + expect(execOptions()).toMatchObject({ + timeout: 60_000, + signal: ac.signal, + }); + }, + ); + + it.each(operations)( + "$name rethrows AbortError and records the abort", + async ({ run, event }) => { + stubExecFile("abort"); + const { manager, sink } = setup(); + + await expect( + run(manager, { signal: AbortSignal.abort() }), + ).rejects.toThrow("The operation was aborted"); + if (event) { + const span = sink.expectOne(event); + expect(span.properties).toMatchObject({ result: "aborted" }); + expect(span.properties["error.type"]).toBeUndefined(); + } + }, + ); + + it.each(operations)( + "$name is skipped when no binary is downloaded", + async ({ run, event, whenMissing }) => { + const { manager, sink } = setup(vi.fn().mockResolvedValue(undefined)); + + await whenMissing( + run(manager, { signal: new AbortController().signal }), + ); + + expect(execFile).not.toHaveBeenCalled(); + if (event) { + expect(sink.expectOne(event).properties).toMatchObject({ + result: "success", + outcome: "no_binary", + }); + } + }, + ); + + it.each(operations)( + "$name reports a binary that cannot be resolved without running the CLI", + async ({ run, event, whenBroken }) => { + const { manager, sink } = setup( + vi.fn().mockRejectedValue(new Error("broken")), + ); + + await whenBroken( + run(manager, { signal: new AbortController().signal }), + ); + + expect(execFile).not.toHaveBeenCalled(); + if (event) { + expect(sink.expectOne(event).properties).toMatchObject({ + result: "error", + "error.type": "binary", + }); + } + }, + ); + }); }); diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index 7fb81a5014..43e99cb16f 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -35,6 +35,20 @@ vi.mock("node:child_process", async (importOriginal) => { const cliExec = await import("@/core/cliExec"); const { spawn } = await import("node:child_process"); +const sharedAuth = (url: string): CliEnv["auth"] => ({ + store: "cli", + url, + useKeyring: undefined, + allowRedirects: false, +}); +const privateAuth = (url: string, configDir: string): CliEnv["auth"] => ({ + store: "extension", + url, + configDir, + useKeyring: undefined, + allowRedirects: false, +}); + describe("cliExec", () => { const tmp = path.join(os.tmpdir(), "vscode-coder-tests-cliExec"); let echoArgsBin: string; @@ -97,6 +111,27 @@ describe("cliExec", () => { ); }); + it("reuses the version until the binary changes", async () => { + const at = new Date("2026-01-01T00:00:00Z"); + const versionBin = (v: string) => echoBin(JSON.stringify({ version: v })); + const bin = await writeExecutable( + tmp, + "ver-cached", + versionBin("v1.0.0"), + ); + await fs.utimes(bin, at, at); + expect(await cliExec.version(bin)).toBe("v1.0.0"); + + // Same size and mtime: served from the cache without running the file. + await writeExecutable(tmp, "ver-cached", versionBin("v2.0.0")); + await fs.utimes(bin, at, at); + expect(await cliExec.version(bin)).toBe("v1.0.0"); + + // A new mtime invalidates it. + await writeExecutable(tmp, "ver-cached", versionBin("v3.0.0")); + expect(await cliExec.version(bin)).toBe("v3.0.0"); + }); + it("parses version from JSON output", async () => { const bin = await writeExecutable( tmp, @@ -154,10 +189,7 @@ describe("cliExec", () => { describe("speedtest", () => { it("passes global, header, and command-specific flags", async () => { - const { configs, env } = setup({ - mode: "url", - url: "http://localhost:3000", - }); + const { configs, env } = setup(sharedAuth("http://localhost:3000")); configs.set("coder.headerCommand", "my-header-cmd"); const args = (await cliExec.speedtest(env, "owner/workspace", "10s")) .trim() @@ -182,10 +214,7 @@ describe("cliExec", () => { `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "speedtest-err", code); - const { env } = setup( - { mode: "global-config", configDir: "/tmp", allowOverride: true }, - bin, - ); + const { env } = setup(privateAuth("http://localhost:3000", "/tmp"), bin); await expect( cliExec.speedtest(env, "owner/workspace", "bad"), ).rejects.toThrow("invalid argument for -t flag"); @@ -195,10 +224,7 @@ describe("cliExec", () => { // Hangs forever so the only way out is the abort signal. const code = `setInterval(() => {}, 1000);`; const bin = await writeExecutable(tmp, "speedtest-hang", code); - const { env } = setup( - { mode: "global-config", configDir: "/tmp", allowOverride: true }, - bin, - ); + const { env } = setup(privateAuth("http://localhost:3000", "/tmp"), bin); const ac = new AbortController(); ac.abort(); await expect( @@ -209,10 +235,7 @@ describe("cliExec", () => { describe("netcheck", () => { it("passes global and header flags", async () => { - const { configs, env } = setup({ - mode: "url", - url: "http://localhost:3000", - }); + const { configs, env } = setup(sharedAuth("http://localhost:3000")); configs.set("coder.headerCommand", "my-header-cmd"); const args = (await cliExec.netcheck(env)).trim().split("\n"); expect(args).toEqual([ @@ -230,10 +253,7 @@ describe("cliExec", () => { `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "netcheck-err", code); - const { env } = setup( - { mode: "global-config", configDir: "/tmp", allowOverride: true }, - bin, - ); + const { env } = setup(privateAuth("http://localhost:3000", "/tmp"), bin); await expect(cliExec.netcheck(env)).rejects.toThrow( "You are not logged in", ); @@ -243,10 +263,7 @@ describe("cliExec", () => { // Hangs forever so the only way out is the abort signal. const code = `setInterval(() => {}, 1000);`; const bin = await writeExecutable(tmp, "netcheck-hang", code); - const { env } = setup( - { mode: "global-config", configDir: "/tmp", allowOverride: true }, - bin, - ); + const { env } = setup(privateAuth("http://localhost:3000", "/tmp"), bin); const ac = new AbortController(); ac.abort(); await expect(cliExec.netcheck(env, ac.signal)).rejects.toMatchObject({ @@ -266,10 +283,7 @@ describe("cliExec", () => { ].join("\n"); const bin = await writeExecutable(tmp, "sb-echo-args", code); const outputPath = path.join(tmp, "sb-args-output.zip"); - const { configs, env } = setup( - { mode: "url", url: "http://localhost:3000" }, - bin, - ); + const { configs, env } = setup(sharedAuth("http://localhost:3000"), bin); configs.set("coder.headerCommand", "my-header-cmd"); await cliExec.supportBundle(env, "owner/workspace", { outputPath, @@ -307,7 +321,7 @@ describe("cliExec", () => { ].join("\n"); const bin = await writeExecutable(tmp, "sb-echo-defaults", code); const outputPath = path.join(tmp, "sb-defaults-output.zip"); - const { env } = setup({ mode: "url", url: "http://localhost:3000" }, bin); + const { env } = setup(sharedAuth("http://localhost:3000"), bin); await cliExec.supportBundle(env, "owner/workspace", { outputPath }); const args = (await fs.readFile(outputPath, "utf-8")).trim().split("\n"); expect(args).toEqual([ @@ -328,10 +342,7 @@ describe("cliExec", () => { `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "sb-err", code); - const { env } = setup( - { mode: "global-config", configDir: "/tmp", allowOverride: true }, - bin, - ); + const { env } = setup(privateAuth("http://localhost:3000", "/tmp"), bin); await expect( cliExec.supportBundle(env, "owner/workspace", { outputPath: "/tmp/bundle.zip", @@ -376,7 +387,7 @@ describe("cliExec", () => { }); it("spawns coder ping with raw argv (no shell, unescaped workspace name)", () => { - const { env } = setup({ mode: "url", url: "https://test.coder.com" }); + const { env } = setup(sharedAuth("https://test.coder.com")); cliExec.ping(env, "owner/my workspace"); expect(spawn).toHaveBeenCalledWith( @@ -387,24 +398,30 @@ describe("cliExec", () => { }); it("includes user global flags raw in the spawn argv", () => { - const { configs, env } = setup({ - mode: "global-config", - configDir: "/cfg", - allowOverride: true, - }); + const { configs, env } = setup( + privateAuth("https://test.coder.com", "/cfg"), + ); configs.set("coder.globalFlags", ["--verbose"]); cliExec.ping(env, "owner/ws"); expect(spawn).toHaveBeenCalledWith( env.binary, - ["--verbose", "--global-config", "/cfg", "ping", "owner/ws"], + [ + "--verbose", + "--global-config", + "/cfg", + "--url", + "https://test.coder.com", + "ping", + "owner/ws", + ], expect.objectContaining({ detached: process.platform !== "win32" }), ); }); it("reports ENOENT once even when `close` fires after `error`", () => { - const { env } = setup({ mode: "url", url: "https://test.coder.com" }); + const { env } = setup(sharedAuth("https://test.coder.com")); cliExec.ping(env, "owner/ws"); // Real Node emits `error` then `close(null, null)` on missing binary. diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index c150c1649c..e38d257a15 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -4,7 +4,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; import * as pgp from "@/pgp"; -import { isKeyringEnabled } from "@/settings/cli"; import { expectPathsEqual } from "../../utils/platform"; @@ -27,12 +26,6 @@ import type * as fs from "node:fs"; vi.mock("os"); vi.mock("axios"); -vi.mock("@/settings/cli", async () => { - const actual = - await vi.importActual("@/settings/cli"); - return { ...actual, isKeyringEnabled: vi.fn().mockReturnValue(false) }; -}); - vi.mock("fs", async () => { const memfs: { fs: typeof fs } = await vi.importActual("memfs"); return { ...memfs.fs, default: memfs.fs }; @@ -141,11 +134,9 @@ describe("CliManager", () => { expectPathsEqual(await manager.locateBinary(TEST_URL), BINARY_PATH); }); - it("throws when binary does not exist", async () => { + it("returns undefined when binary does not exist", async () => { const { manager } = setupCliManager(); - await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( - "No CLI binary found at", - ); + await expect(manager.locateBinary(TEST_URL)).resolves.toBeUndefined(); }); }); @@ -183,12 +174,10 @@ describe("CliManager", () => { expectPathsEqual(await t.manager.locateBinary(TEST_URL), FILE_PATH); }); - it("locateBinary throws when file does not exist", async () => { + it("locateBinary returns undefined when file does not exist", async () => { const { manager, mockConfig } = setupCliManager(); mockConfig.set("coder.binaryDestination", "/nonexistent/coder"); - await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( - "No CLI binary found at", - ); + await expect(manager.locateBinary(TEST_URL)).resolves.toBeUndefined(); }); it("fetchBinary uses file when version matches", async () => { @@ -310,57 +299,93 @@ describe("CliManager", () => { describe("Clear Credentials", () => { const CLEAR_URL = "https://dev.coder.com"; - it("should skip progress notification when keyring is disabled", async () => { - const { manager, mockCredManager } = setupCliManager(); - - await manager.clearCredentials(CLEAR_URL); - - expect(vscode.window.withProgress).not.toHaveBeenCalled(); - expect(mockCredManager.deleteToken).toHaveBeenCalledWith( - CLEAR_URL, - expect.anything(), - { signal: expect.any(AbortSignal) }, - ); - }); + interface ProgressCase { + signOutCli: boolean; + progress: number; + } - it("should show progress notification when keyring is enabled", async () => { - const { manager } = setupCliManager(); - vi.mocked(isKeyringEnabled).mockReturnValue(true); + it.each([ + { signOutCli: false, progress: 0 }, + { signOutCli: true, progress: 1 }, + ])( + "shows progress $progress time(s) when signOutCli is $signOutCli", + async ({ signOutCli, progress }) => { + const { manager, mockCredManager } = setupCliManager(); - await manager.clearCredentials(CLEAR_URL); + await manager.clearCredentials(CLEAR_URL, { signOutCli }); - expect(vscode.window.withProgress).toHaveBeenCalledWith( - expect.objectContaining({ - location: vscode.ProgressLocation.Notification, - title: `Removing credentials for ${CLEAR_URL}`, - cancellable: true, - }), - expect.any(Function), - ); - }); + expect(vscode.window.withProgress).toHaveBeenCalledTimes(progress); + expect(mockCredManager.deleteToken).toHaveBeenCalledWith( + CLEAR_URL, + expect.anything(), + { signal: expect.any(AbortSignal), signOutCli }, + ); + }, + ); it.each([ - { scenario: "succeeds", error: undefined, cleared: true }, + { + scenario: "succeeds", + error: undefined, + expected: true, + }, { scenario: "fails", error: new Error("unexpected failure"), - cleared: false, + expected: false, + }, + { + scenario: "is cancelled", + error: makeAbortError(), + expected: false, }, - { scenario: "is cancelled", error: makeAbortError(), cleared: false }, ])( "should report cleanup state when deleteToken $scenario", - async ({ error, cleared }) => { + async ({ error, expected }) => { const { manager, mockCredManager } = setupCliManager(); if (error) { vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); + } else { + vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce( + expected, + ); } - await expect(manager.clearCredentials(CLEAR_URL)).resolves.toBe( - cleared, - ); + await expect( + manager.clearCredentials(CLEAR_URL, { signOutCli: true }), + ).resolves.toEqual(expected); }, ); }); + describe("Holds Token", () => { + const URL = "https://dev.coder.com"; + + it("asks the CLI under cancellable progress", async () => { + const { manager, mockCredManager } = setupCliManager(); + vi.mocked(mockCredManager.holdsToken).mockResolvedValueOnce(true); + + expect(await manager.holdsToken(URL, "t")).toBe(true); + expect(vscode.window.withProgress).toHaveBeenCalledTimes(1); + }); + + it("counts a cancelled check as not held", async () => { + const { manager, mockCredManager } = setupCliManager(); + vi.mocked(mockCredManager.holdsToken).mockRejectedValueOnce( + makeAbortError(), + ); + + expect(await manager.holdsToken(URL, "t")).toBe(false); + }); + + it("skips the CLI when it has no store to check", async () => { + const { manager, mockCredManager } = setupCliManager(); + vi.mocked(mockCredManager.hasCliStore).mockResolvedValueOnce(false); + + expect(await manager.holdsToken(URL, "t")).toBe(false); + expect(vscode.window.withProgress).not.toHaveBeenCalled(); + }); + }); + describe("Binary Version Validation", () => { it("rejects invalid server versions", async () => { const { manager, mockApi } = setupCliManager(); diff --git a/test/unit/core/secretsManager.test.ts b/test/unit/core/secretsManager.test.ts index c66d5c4bbe..482d71e0a9 100644 --- a/test/unit/core/secretsManager.test.ts +++ b/test/unit/core/secretsManager.test.ts @@ -502,9 +502,12 @@ describe("SecretsManager", () => { const sessionAuthCases: BackwardsCompatTestCase[] = [ { - name: "without optional oauth field", + name: "without optional fields", data: { url: "https://coder.example.com", token: "test-token" }, - expected: { url: "https://coder.example.com", token: "test-token" }, + expected: { + url: "https://coder.example.com", + token: "test-token", + }, }, { name: "with OAuth without optional fields", diff --git a/test/unit/featureSet.test.ts b/test/unit/featureSet.test.ts index ccfd508065..0a97b3bf77 100644 --- a/test/unit/featureSet.test.ts +++ b/test/unit/featureSet.test.ts @@ -52,8 +52,8 @@ describe("check version support", () => { it("token read", () => { expectFlag( "tokenRead", - ["v2.30.0", "v2.29.0", "v2.28.0", "v1.0.0"], - ["v2.31.0", "v2.31.1", "v2.32.0", "v3.0.0"], + ["v2.31.1", "v2.31.0", "v2.30.0", "v1.0.0"], + ["v2.32.0", "v2.32.1", "v2.33.0", "v3.0.0"], ); }); it("support bundle", () => { @@ -63,6 +63,13 @@ describe("check version support", () => { ["v2.10.0", "v2.10.1", "v2.11.0", "v3.0.0"], ); }); + it("allow redirects", () => { + expectFlag( + "allowRedirects", + ["v2.37.1", "v2.37.0", "v2.36.5", "v1.0.0"], + ["v2.38.0", "v2.38.1", "v2.39.0", "v3.0.0"], + ); + }); it("support bundle workspace files", () => { expectFlag( "supportBundleWorkspaceFiles", diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 44bfbb7982..eb1870a25b 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -179,14 +179,55 @@ function createTestContext(telemetry?: TelemetryService) { }; } +/** Test context plus shorthands for a sign-in that `prompt` may guard. */ +function createSignInTestContext( + prompt: string, + detail: (username: string) => string, +) { + const ctx = createTestContext(); + return { + ...ctx, + /** Queue one getAuthenticatedUser result per expected call, in order. */ + authSequence: (...results: Array) => { + for (const result of results) { + if (result === "unauthorized") { + mockGetAuthenticatedUser.mockRejectedValueOnce( + createAxiosError(401, "Unauthorized"), + ); + } else { + mockGetAuthenticatedUser.mockResolvedValueOnce(result); + } + } + }, + storeSession: (auth: { token: string; username?: string; url?: string }) => + ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, { + url: TEST_URL, + ...auth, + }), + confirmSignIn: () => ctx.userInteraction.setResponse(prompt, "Sign In"), + dismissSignIn: () => ctx.userInteraction.setResponse(prompt, undefined), + storedToken: async () => + (await ctx.secretsManager.getSessionAuth(TEST_HOSTNAME))?.token, + /** Assert the prompt named the user and the session it replaces. */ + expectSignInPrompt: (username: string, replaces?: string) => + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith( + prompt, + expect.objectContaining({ + detail: `${TEST_URL}\n\n${detail(username)}${replaces ? `, replacing your ${replaces}` : ""}.`, + }), + "Sign In", + ), + expectNoPrompt: () => + expect(vscode.window.showWarningMessage).not.toHaveBeenCalled(), + }; +} + describe("LoginCoordinator", () => { describe("token authentication", () => { - it("authenticates with stored token on success", async () => { + it("authenticates with a stored token", async () => { const { secretsManager, coordinator, mockSuccessfulAuth } = createTestContext(); const user = mockSuccessfulAuth(); - - // Pre-store a token await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "stored-token", @@ -203,9 +244,6 @@ describe("LoginCoordinator", () => { user, token: "stored-token", }); - - const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); - expect(auth?.token).toBe("stored-token"); }); it("authenticates with CLI credential token on success", async () => { @@ -216,10 +254,9 @@ describe("LoginCoordinator", () => { mockSuccessfulAuth, } = createTestContext(); const user = mockSuccessfulAuth(); - vi.mocked(mockCredentialManager.readToken).mockResolvedValueOnce({ - token: "cli-credential-token", - source: "files", - }); + vi.mocked(mockCredentialManager.readToken).mockResolvedValueOnce( + "cli-credential-token", + ); const result = await coordinator.ensureLoggedIn({ url: TEST_URL, @@ -238,28 +275,6 @@ describe("LoginCoordinator", () => { expect(auth?.token).toBe("cli-credential-token"); }); - it("reports keyring_token method when the credential comes from the keyring", async () => { - const { mockCredentialManager, coordinator, mockSuccessfulAuth } = - createTestContext(); - const user = mockSuccessfulAuth(); - vi.mocked(mockCredentialManager.readToken).mockResolvedValueOnce({ - token: "keyring-token", - source: "keyring", - }); - - const result = await coordinator.ensureLoggedIn({ - url: TEST_URL, - safeHostname: TEST_HOSTNAME, - }); - - expect(result).toEqual({ - success: true, - method: "keyring_token", - user, - token: "keyring-token", - }); - }); - it("prompts for token when no stored auth exists", async () => { const { userInteraction, @@ -447,36 +462,14 @@ describe("LoginCoordinator", () => { token, }); - /** Test context plus shorthands for the link sign-in flow. */ function createLinkTestContext() { - const ctx = createTestContext(); + const ctx = createSignInTestContext( + SIGN_IN_PROMPT, + (username) => + `The link contains a token that signs you in as "${username}"`, + ); return { ...ctx, - /** Queue one getAuthenticatedUser result per expected call, in order. */ - authSequence: (...results: Array) => { - for (const result of results) { - if (result === "unauthorized") { - mockGetAuthenticatedUser.mockRejectedValueOnce( - createAxiosError(401, "Unauthorized"), - ); - } else { - mockGetAuthenticatedUser.mockResolvedValueOnce(result); - } - } - }, - storeSession: (auth: { - token: string; - username?: string; - url?: string; - }) => - ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, { - url: TEST_URL, - ...auth, - }), - confirmSignIn: () => - ctx.userInteraction.setResponse(SIGN_IN_PROMPT, "Sign In"), - dismissSignIn: () => - ctx.userInteraction.setResponse(SIGN_IN_PROMPT, undefined), login: (options?: { token?: string; tokenSignInConfirmed?: boolean }) => ctx.coordinator.ensureLoggedIn({ url: TEST_URL, @@ -484,21 +477,6 @@ describe("LoginCoordinator", () => { token: LINK_TOKEN, ...options, }), - storedToken: async () => - (await ctx.secretsManager.getSessionAuth(TEST_HOSTNAME))?.token, - /** Assert the prompt named the user and the session it replaces. */ - expectSignInPrompt: (username: string, replaces?: string) => - expect(vscode.window.showWarningMessage).toHaveBeenCalledWith( - SIGN_IN_PROMPT, - expect.objectContaining({ - detail: - `${TEST_URL}\n\nThe link contains a token that signs you in as "${username}"` + - `${replaces ? `, replacing your ${replaces}` : ""}.`, - }), - "Sign In", - ), - expectNoPrompt: () => - expect(vscode.window.showWarningMessage).not.toHaveBeenCalled(), }; } @@ -784,6 +762,90 @@ describe("LoginCoordinator", () => { user, token: "stored-token", }); + await vi.waitFor(() => + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + expect.stringContaining("keyring unavailable"), + "Open Settings", + ), + ); + }); + }); + + describe("CLI session confirmation", () => { + const CLI_PROMPT = "Sign in with the Coder CLI's session?"; + + function createCliTestContext() { + const ctx = createSignInTestContext( + CLI_PROMPT, + (username) => `The Coder CLI's session signs you in as "${username}"`, + ); + return { + ...ctx, + cliToken: (token: string) => + vi + .mocked(ctx.mockCredentialManager.readToken) + .mockResolvedValueOnce(token), + login: () => + ctx.coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }), + }; + } + + it("adopts the CLI session without a prompt when there is no previous session", async () => { + const t = createCliTestContext(); + const user = t.mockSuccessfulAuth( + createMockUser({ username: "cli-user" }), + ); + t.cliToken("cli-token"); + + expect(await t.login()).toMatchObject({ + method: "cli_token", + user, + }); + t.expectNoPrompt(); + }); + + it("adopts the CLI session without a prompt when it belongs to the same user", async () => { + const t = createCliTestContext(); + await t.storeSession({ token: "expired-token", username: "same-user" }); + t.authSequence("unauthorized", createMockUser({ username: "same-user" })); + t.cliToken("cli-token"); + + expect(await t.login()).toMatchObject({ token: "cli-token" }); + t.expectNoPrompt(); + }); + + it("asks before adopting a CLI session for a different user, naming both", async () => { + const t = createCliTestContext(); + await t.storeSession({ token: "expired-token", username: "old-user" }); + t.authSequence("unauthorized", createMockUser({ username: "cli-user" })); + t.cliToken("cli-token"); + t.confirmSignIn(); + + expect(await t.login()).toMatchObject({ + token: "cli-token", + }); + t.expectSignInPrompt("cli-user", 'expired session for "old-user"'); + }); + + it("falls back to asking for a token when the CLI session is declined", async () => { + const t = createCliTestContext(); + await t.storeSession({ token: "expired-token", username: "old-user" }); + t.authSequence( + "unauthorized", + createMockUser({ username: "cli-user" }), + createMockUser({ username: "new-user" }), + ); + t.cliToken("cli-token"); + t.dismissSignIn(); + t.userInteraction.setInputBoxValue("new-token"); + + expect(await t.login()).toMatchObject({ + token: "new-token", + }); + expect(await t.storedToken()).toBe("new-token"); }); }); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index be11270d44..b02927fc4a 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -164,7 +164,10 @@ describe("OAuthSessionManager", () => { }, { name: "returns false when session auth has no OAuth data", - auth: { url: TEST_URL, token: "session-token" }, + auth: { + url: TEST_URL, + token: "session-token", + }, expected: false, }, ])("$name", async ({ auth, expected }) => { diff --git a/test/unit/remote/migration.test.ts b/test/unit/remote/migration.test.ts index 0eadd0c27b..f663e77b02 100644 --- a/test/unit/remote/migration.test.ts +++ b/test/unit/remote/migration.test.ts @@ -76,7 +76,10 @@ describe("Session auth migration", () => { it("does not migrate or delete files when auth already exists", async () => { const { migrate, secretsManager } = setup({ - existingAuth: { url: "https://dep.example.com", token: "current" }, + existingAuth: { + url: "https://dep.example.com", + token: "current", + }, }); writeLegacyFiles(); diff --git a/test/unit/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index e437b67467..cc1e970ecf 100644 --- a/test/unit/remote/workspaceStateMachine.test.ts +++ b/test/unit/remote/workspaceStateMachine.test.ts @@ -109,7 +109,12 @@ function setup( startupMode, "/usr/bin/coder", {} as FeatureSet, - { mode: "url", url: "https://test.coder.com" }, + { + store: "cli", + url: "https://test.coder.com", + useKeyring: undefined, + allowRedirects: false, + }, createMockServiceContainer({ telemetry, logger: createMockLogger() }), ); return { sm, progress, userInteraction }; diff --git a/test/unit/util/credentials.test.ts b/test/unit/util/credentials.test.ts new file mode 100644 index 0000000000..432721cbd7 --- /dev/null +++ b/test/unit/util/credentials.test.ts @@ -0,0 +1,65 @@ +import * as os from "node:os"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +import { showStoreCredentialsError } from "@/util/credentials"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +vi.mock("node:os"); + +const configs = { + get: vi.fn((_key: string, defaultValue?: unknown) => defaultValue), +}; + +describe("showStoreCredentialsError", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + interface StoreErrorCase { + platform: NodeJS.Platform; + message: string; + } + + it.each([ + { + platform: "darwin", + message: + 'Failed to store credentials: exit status 36. To store the token in a file instead, set "coder.useKeyring" to false.', + }, + { + platform: "linux", + message: "Failed to store credentials: exit status 36.", + }, + ])("logs and shows the failure on $platform", ({ platform, message }) => { + vi.mocked(os.platform).mockReturnValue(platform); + const logger = createMockLogger(); + + showStoreCredentialsError(new Error("exit status 36"), configs, logger); + + expect(logger.error).toHaveBeenCalledWith( + "Failed to store credentials:", + expect.any(Error), + ); + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + message, + "Open Settings", + ); + }); + + it("opens the coder.useKeyring setting from the toast", async () => { + vi.mocked(os.platform).mockReturnValue("linux"); + vi.mocked(vscode.window.showErrorMessage).mockResolvedValueOnce( + "Open Settings" as unknown as vscode.MessageItem, + ); + + showStoreCredentialsError(new Error("x"), configs, createMockLogger()); + await Promise.resolve(); + + expect(vscode.commands.executeCommand).toHaveBeenCalledWith( + "workbench.action.openSettings", + "coder.useKeyring", + ); + }); +}); diff --git a/test/utils/platform.ts b/test/utils/platform.ts index 666f5f4a4c..26320e3766 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -123,8 +123,9 @@ export function quoteCommand(value: string): string { return `${quote}${value}${quote}`; } -export function expectPathsEqual(actual: string, expected: string) { - expect(normalizePath(actual)).toBe(normalizePath(expected)); +export function expectPathsEqual(actual: string | undefined, expected: string) { + expect(actual).toBeDefined(); + expect(normalizePath(actual!)).toBe(normalizePath(expected)); } function normalizePath(p: string): string {