From fa5f9d1bacac4c3c652fed59d8b11c642a8cdfa7 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 10 Sep 2026 19:00:09 +0200 Subject: [PATCH 1/5] feat: default coder.useKeyring to true and harden shared credential handling Store session tokens in the OS keyring by default on macOS and Windows, passing --use-keyring explicitly to CLI 2.29 and later. Model the CLI store as shared (the CLI's own directory, or a user directory on 2.31+) or private (a file in the extension's per-deployment directory), and treat CODER_CONFIG_DIR like a user --global-config. Record who minted each stored token so logout runs coder logout against a shared store only for a token the extension created and the CLI still holds. Ask before adopting the CLI's session for a different user. 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. Closes #1106 --- CHANGELOG.md | 23 + package.json | 6 +- src/commands.ts | 25 +- src/core/cliCredentialManager.ts | 289 ++---- src/core/cliManager.ts | 41 +- src/core/secretsManager.ts | 8 + src/instrumentation/EVENTS.md | 22 +- src/instrumentation/credentials.ts | 14 +- src/login/loginCoordinator.ts | 87 +- src/oauth/sessionManager.ts | 1 + src/remote/migration.ts | 1 + src/settings/cli.ts | 92 +- src/util/credentials.ts | 32 + test/unit/api/authInterceptor.test.ts | 4 + test/unit/api/workspace.test.ts | 6 +- test/unit/cliConfig.test.ts | 463 ++++----- test/unit/commands.telemetry.test.ts | 18 +- test/unit/core/cliCredentialManager.test.ts | 886 +++++++----------- test/unit/core/cliExec.test.ts | 76 +- test/unit/core/cliManager.test.ts | 36 +- test/unit/core/secretsManager.test.ts | 43 +- .../unit/deployment/deploymentManager.test.ts | 10 + test/unit/login/loginCoordinator.test.ts | 269 ++++-- test/unit/oauth/sessionManager.test.ts | 11 +- test/unit/remote/migration.test.ts | 7 +- .../unit/remote/workspaceStateMachine.test.ts | 2 +- test/unit/uri/uriHandler.test.ts | 3 + test/unit/util/credentials.test.ts | 65 ++ 28 files changed, 1250 insertions(+), 1290 deletions(-) create mode 100644 src/util/credentials.ts create mode 100644 test/unit/util/credentials.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 135c3fe27d..29fc19781d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,29 @@ 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. The + entry is shared with the `coder` CLI, so 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`. +- 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`. +- 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. + +### Security + +- Sign out the `coder` CLI only when it still holds the token this extension + created. A session that came from the CLI is removed from the extension + without signing the CLI out. + ## [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..6f91baa8ce 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.31.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.31.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 signs out the CLI only when it still holds the token this extension created.", "type": "boolean", - "default": false, + "default": true, "scope": "application" }, "coder.networkThreshold.latencyMs": { diff --git a/src/commands.ts b/src/commands.ts index 738897d545..6a663faa92 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -708,12 +708,25 @@ export class Commands { await this.deploymentManager.clearDeployment("logout"); if (deployment) { - const cleared = await this.cliManager.clearCredentials(deployment.url); + const session = await this.secretsManager.getSessionAuth( + deployment.safeHostname, + ); + const cleared = await this.cliManager.clearCredentials( + deployment.url, + session, + ); 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.', - ); + 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.', + "Show Output", + ) + .then((action) => { + if (action === "Show Output") { + this.logger.show(); + } + }); return { success: false, reason: "cleanup_incomplete" }; } } @@ -790,7 +803,7 @@ export class Commands { const selectedHostname = selected.hostnames[0]; const auth = await this.secretsManager.getSessionAuth(selectedHostname); if (auth?.url) { - await this.cliManager.clearCredentials(auth.url); + await this.cliManager.clearCredentials(auth.url, auth); } await this.secretsManager.clearAllAuthData(selectedHostname); this.logger.info("Removed credentials for", selectedHostname); @@ -812,7 +825,7 @@ export class Commands { selected.hostnames.map(async (h) => { const auth = await this.secretsManager.getSessionAuth(h); if (auth?.url) { - await this.cliManager.clearCredentials(auth.url); + await this.cliManager.clearCredentials(auth.url, auth); } await this.secretsManager.clearAllAuthData(h); }), diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index f6299f58fc..2589981a1e 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -1,6 +1,5 @@ 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"; @@ -10,8 +9,7 @@ import { CredentialCliError, CredentialTelemetry, } from "../instrumentation/credentials"; -import { getGlobalFlags, isKeyringEnabled } from "../settings/cli"; -import { getHeaderArgs } from "../settings/headers"; +import { type CliAuth, getGlobalFlags, resolveCliAuth } from "../settings/cli"; import { type TelemetryReporter } from "../telemetry/reporter"; import { toSafeHost } from "../util/uri"; @@ -23,42 +21,27 @@ import type { Logger } from "../logging/logger"; import type { Span } from "../telemetry/span"; import type { PathResolver } from "./pathResolver"; +import type { SessionAuth } from "./secretsManager"; 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; +interface ResolvedCli { + binPath: string; + featureSet: FeatureSet; + auth: CliAuth; + flags: string[]; +} + /** * 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"; -} - -/** - * Delegates credential storage to the Coder CLI, both keyring-backed and - * file-based, via `coder login`/`coder logout`. - */ +/** Stores, reads, and deletes credentials through `coder login` and `coder logout`. */ export class CliCredentialManager { private readonly credentialTelemetry: CredentialTelemetry; @@ -71,10 +54,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`. Throws when the binary or the CLI fails. */ public storeToken( url: string, token: string, @@ -82,84 +62,50 @@ 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); + span.setProperty("store", cli.auth.store); + try { + await this.exec(cli, ["login", "--use-token-as-session", url], { + env: { ...process.env, CODER_SESSION_TOKEN: token }, + signal: options?.signal, + }); + this.logger.info("Stored token via CLI for", url); + } catch (error) { + this.logger.warn("Failed to store token via CLI:", error); + if (isAbortError(error)) { + throw error; + } + throw new CredentialCliError(error); + } }); } - private async cliLogin( - transport: CliTransport, + /** Reads the CLI's token via `coder login token` (CLI 2.31+). 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 { + let cli: ResolvedCli; 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); + cli = await this.resolveCli(url, configs); } catch (error) { - this.logger.warn("Failed to store token via CLI:", error); - if (isAbortError(error)) { - throw error; - } - throw new CredentialCliError(error); - } - } - - /** - * 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. - */ - public async readToken( - url: string, - configs: Pick, - options?: { signal?: AbortSignal }, - ): Promise { - const transport = await this.resolveReadTransport(url, configs); - if (transport.kind === "none") { + this.logger.warn("Could not resolve CLI binary:", error); return undefined; } - const args = [ - ...this.credentialGlobalFlags(transport, url, configs), - "login", - "token", - "--url", - url, - ]; - const token = await this.runTokenRead(transport.binPath, args, options); - if (!token) { + if (!cli.featureSet.tokenRead) { return undefined; } - return { - token, - source: transport.kind === "keyring" ? "keyring" : "files", - }; + return this.readCliToken(cli, options?.signal); } - private async runTokenRead( - binPath: string, - args: string[], - options?: { signal?: AbortSignal }, + private async readCliToken( + cli: ResolvedCli, + signal: AbortSignal | undefined, ): Promise { try { - const { stdout } = await this.execWithTimeout(binPath, args, { - signal: options?.signal, - }); - return nonEmpty(stdout); + const { stdout } = await this.exec(cli, ["login", "token"], { signal }); + return stdout.trim() || undefined; } catch (error) { if (isAbortError(error)) { throw error; @@ -170,155 +116,116 @@ export class CliCredentialManager { } /** - * 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` when the + * CLI session is ours (see `ownsCliSession`). Returns whether every store + * was cleared; throws only on abort. */ public deleteToken( url: string, configs: Pick, + session: SessionAuth | undefined, options?: { signal?: AbortSignal }, ): 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, session, { + signal: options?.signal, + 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, + session: SessionAuth | undefined, { signal, span }: { signal?: AbortSignal; span: Span }, ): Promise { - let transport: CliTransport; + let cli: ResolvedCli; try { - transport = await this.resolveWriteTransport(url, configs); + cli = await this.resolveCli(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", - ]; + span.setProperty("store", cli.auth.store); + if (!(await this.ownsCliSession(cli, session, signal))) { + this.logger.info("Kept the CLI session for", url); + return true; + } try { - await this.execWithTimeout(transport.binPath, args, { signal }); - this.logger.info("Deleted token via CLI for", url); + await this.exec(cli, ["logout", "--yes"], { signal }); + 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); + this.logger.warn("Failed to log out via CLI:", error); span.setProperty("error.type", "cli"); span.markError(); 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); - return undefined; - }); - if (!cli) { - return { kind: "none" }; + /** A shared store is ours only if the CLI still holds the token this extension created. */ + private async ownsCliSession( + cli: ResolvedCli, + session: SessionAuth | undefined, + signal: AbortSignal | undefined, + ): Promise { + if (cli.auth.store === "private") { + return true; } - if (isKeyringEnabled(configs) && cli.featureSet.keyringAuth) { - return cli.featureSet.tokenRead - ? { kind: "keyring", binPath: cli.binPath } - : { kind: "none" }; + if (session?.tokenSource !== "extension") { + return false; } - if (cli.featureSet.tokenRead) { - return cliFileTransport(cli); + // Below 2.31 the CLI cannot report its token; trust the provenance. + if (!cli.featureSet.tokenRead) { + return true; } - return { kind: "none" }; + const cliToken = await this.readCliToken(cli, signal); + return cliToken === session.token; } - /** Keyring uses the default store; file mode passes --global-config. */ - private credentialGlobalFlags( - transport: CliTransport, + private async resolveCli( 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))); + ): Promise { + const binPath = await this.resolveBinary(url); + 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 and periodic debug logging. */ + 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, }); } 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 +246,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/cliManager.ts b/src/core/cliManager.ts index f603910a4c..5cd786d70d 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -23,6 +23,7 @@ import { 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"; @@ -41,6 +42,7 @@ import type { Span } from "../telemetry/span"; import type { CliCredentialManager } from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; +import type { SessionAuth } from "./secretsManager"; type ResolvedBinary = | { binPath: string; stat: Stats; source: "file_path" | "directory" } @@ -1041,7 +1043,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,19 +1066,24 @@ export class CliManager { return; } trace.error(result.error); - this.handleStoreError(result.error); + this.handleStoreError(result.error, configs); } /** - * 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. A store shared with the CLI is only + * logged out of a token this extension created, so pass the stored + * `session`. Never throws; returns whether every store was cleared. */ - public async clearCredentials(url: string): Promise { + public async clearCredentials( + url: string, + session: SessionAuth | undefined, + ): Promise { const configs = vscode.workspace.getConfiguration(); const result = await withOptionalProgress( ({ signal }) => - this.cliCredentialManager.deleteToken(url, configs, { signal }), + this.cliCredentialManager.deleteToken(url, configs, session, { + signal, + }), { enabled: isKeyringEnabled(configs), location: vscode.ProgressLocation.Notification, @@ -1095,21 +1102,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/core/secretsManager.ts b/src/core/secretsManager.ts index 28fbd52e9c..db8e0880a9 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -42,6 +42,11 @@ const OAuthTokenDataSchema = z.object({ export type OAuthTokenData = z.infer; +const TokenSourceSchema = z.enum(["extension", "cli"]); + +/** Who minted a session token: this extension, or the Coder CLI. */ +export type TokenSource = z.infer; + const SessionAuthSchema = z.object({ url: z.string(), token: z.string(), @@ -49,6 +54,8 @@ const SessionAuthSchema = z.object({ username: z.string().optional(), /** If present, this session uses OAuth authentication */ oauth: OAuthTokenDataSchema.optional(), + /** Only extension tokens are revoked at logout. Older sessions predate the CLI source. */ + tokenSource: TokenSourceSchema.default("extension"), }); export type SessionAuth = z.infer; @@ -312,6 +319,7 @@ export class SecretsManager { await this.setSessionAuth(safeHostname, { url: legacyUrl, token: oldToken ?? "", + tokenSource: "extension", }); } diff --git a/src/instrumentation/EVENTS.md b/src/instrumentation/EVENTS.md index ef0814813d..82d045104e 100644 --- a/src/instrumentation/EVENTS.md +++ b/src/instrumentation/EVENTS.md @@ -159,12 +159,12 @@ 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` @@ -206,11 +206,11 @@ 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` | `shared` (the CLI's own store), `private` (the extension's directory) | +| `error.type` | `binary`, `cli` | ### Logs diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts index c193f0dc1f..81a31d82f3 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; diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 15c4fd93f2..da7178778a 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -10,6 +10,7 @@ import { buildOAuthTokenData } from "../oauth/utils"; import { withOptionalProgress } 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"; @@ -21,6 +22,7 @@ import type { OAuthTokenData, SecretsManager, SessionAuth, + TokenSource, } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; import type { @@ -32,12 +34,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 } @@ -51,6 +48,7 @@ export type LoginResult = user: User; token: string; oauth?: OAuthTokenData; + tokenSource: TokenSource; }; export interface LoginOptions { @@ -197,7 +195,7 @@ export class LoginCoordinator implements vscode.Disposable { } private async persistSessionAuth( - result: LoginAttemptResult, + result: LoginResult, safeHostname: string, url: string, ): Promise { @@ -208,15 +206,17 @@ export class LoginCoordinator implements vscode.Disposable { token: result.token, username: result.user.username, oauth: result.oauth, // undefined for non-OAuth logins + tokenSource: result.tokenSource, }); 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), + ); } } } @@ -315,6 +315,7 @@ export class LoginCoordinator implements vscode.Disposable { return withLoginMethod( "mtls", await this.tryMtlsAuth(client, isAutoLogin), + "extension", ); } @@ -385,9 +386,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) { @@ -395,7 +399,7 @@ export class LoginCoordinator implements vscode.Disposable { } } } - return withLoginMethod("provided_token", result); + return withLoginMethod("provided_token", result, "extension"); } /** Stored session for the deployment's exact origin, if it still works. */ @@ -415,14 +419,14 @@ export class LoginCoordinator implements vscode.Disposable { if (result === "unauthorized") { return undefined; } - return withLoginMethod("stored_token", result); + return withLoginMethod("stored_token", result, sameOriginAuth.tokenSource); } - /** 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( ({ signal }) => @@ -432,29 +436,36 @@ export class LoginCoordinator implements vscode.Disposable { { 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 session?", + detail: `The Coder CLI 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, "cli"); } /** Last resort: ask the user how to authenticate. */ @@ -465,21 +476,23 @@ export class LoginCoordinator implements vscode.Disposable { return withLoginMethod( "oauth", await this.loginWithOAuth(ctx.deployment), + "extension", ); case "legacy": return withLoginMethod( "cli_token", await this.loginWithToken(ctx.client), + "extension", ); case undefined: return { success: false, reason: "user_dismissed" }; } } - /** 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 +503,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", ); @@ -667,6 +680,10 @@ export class LoginCoordinator implements vscode.Disposable { function withLoginMethod( method: LoginMethod, result: LoginAttemptResult, + tokenSource: TokenSource, ): LoginResult { - return { ...result, method }; + if (!result.success) { + return { ...result, method }; + } + return { ...result, method, tokenSource }; } diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 5ccc122aa5..4fa3cf051b 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -436,6 +436,7 @@ export class OAuthSessionManager implements vscode.Disposable { tokenResponse.access_token, ), oauth: buildOAuthTokenData(tokenResponse), + tokenSource: "extension", }); return tokenResponse; diff --git a/src/remote/migration.ts b/src/remote/migration.ts index c18a68fde7..3db978b520 100644 --- a/src/remote/migration.ts +++ b/src/remote/migration.ts @@ -75,6 +75,7 @@ async function migrateSessionAuthFromFiles( await secretsManager.setSessionAuth(safeHostname, { url: url.value.trim(), token: token.value.trim(), + tokenSource: "extension", }); } catch (error) { logger.warn("Failed to migrate session auth from files:", error); diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 4827ec75db..c67385daa5 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"; +/** The CLI's own store, shared with the terminal CLI, or a file in the extension's private directory. */ export type CliAuth = - | { mode: "global-config"; configDir: string; allowOverride: boolean } - | { mode: "url"; url: string }; + | { store: "shared"; url: string; useKeyring: boolean | undefined } + | { + store: "private"; + url: string; + configDir: string; + useKeyring: false | undefined; + }; /** * Returns the user's `coder.globalFlags` with `expandPath` applied. For @@ -51,28 +58,22 @@ 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 === "private", + ).map(escAuth); + if (auth.store === "private") { + 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}`); + } + 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 +101,47 @@ 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`). - */ +/** Shares the CLI's 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.31+, where the CLI can report its token. + const userDir = hasUserConfigDir(configs) && featureSet.tokenRead; + if (useKeyring || userDir) { + return { store: "shared", url, useKeyring }; } - // Honored only on 2.31.0+, where CLI-mediated read/write share the directory. - return { - mode: "global-config", - configDir, - allowOverride: featureSet.tokenRead, - }; + return { store: "private", url, 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/unit/api/authInterceptor.test.ts b/test/unit/api/authInterceptor.test.ts index 7056778061..10b3694189 100644 --- a/test/unit/api/authInterceptor.test.ts +++ b/test/unit/api/authInterceptor.test.ts @@ -122,6 +122,7 @@ function createTestContext() { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "access-token", + tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -144,6 +145,7 @@ function createTestContext() { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "session-token", + tokenSource: "extension", }); }; @@ -152,6 +154,7 @@ function createTestContext() { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "", + tokenSource: "extension", }); }; @@ -297,6 +300,7 @@ describe("AuthInterceptor", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "new-token-after-login", + tokenSource: "extension", }); const retryResponse = { data: "success", status: 200 }; diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index df55fb48ee..c9c9ea4500 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -94,7 +94,11 @@ function createUpdateCtx( }; const ctx = { restClient: restClient as unknown as Api, - auth: { mode: "url" as const, url: "https://test.coder.com" }, + auth: { + store: "shared" as const, + url: "https://test.coder.com", + useKeyring: undefined, + }, 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..a8f0979873 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -18,228 +18,151 @@ 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 privateAuth: CliAuth = { + store: "private", + url: URL, + configDir: EXT_DIR, + useKeyring: undefined, }; +const sharedAuth: CliAuth = { + store: "shared", + url: URL, + useKeyring: undefined, +}; + +const PRIVATE_FLAGS = ["--global-config", EXT_DIR, "--url", URL]; +const SHARED_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: "private store", auth: privateAuth, expected: PRIVATE_FLAGS }, + { scenario: "shared store", auth: sharedAuth, expected: SHARED_FLAGS }, { - scenario: "global-config mode", - auth: globalConfigAuth, - expectedAuthFlags: ["--global-config", "/config/dir"], + scenario: "private store with keyring off", + auth: { ...privateAuth, useKeyring: false }, + expected: [...PRIVATE_FLAGS, "--use-keyring=false"], }, { - scenario: "url mode", - auth: urlAuth, - expectedAuthFlags: ["--url", "https://dev.coder.com"], + scenario: "shared store with keyring on", + auth: { ...sharedAuth, useKeyring: true }, + expected: [...SHARED_FLAGS, "--use-keyring=true"], }, - ])( - "should return auth flags for $scenario", - ({ auth, expectedAuthFlags }) => { - const config = new MockConfigurationProvider(); - expect(getGlobalShellFlags(config, auth)).toStrictEqual( - expectedAuthFlags, - ); - }, - ); + ])("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, privateAuth)).toStrictEqual([ "--verbose", - "--disable-direct-connections", - "--global-config", - "/config/dir", + "--global-configs", // similar prefixes are not managed flags + ...PRIVATE_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, privateAuth)).toStrictEqual([ + "--verbose", + ...PRIVATE_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 a shared store ($scenario)", + ({ flags }) => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", flags); - expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual( - expected, - ); + expect(getGlobalShellFlags(config, sharedAuth)).toStrictEqual([ + ...flags, + ...SHARED_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 a private 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, privateAuth)).toStrictEqual([ "-v", - "--url", - "https://dev.coder.com", + ...PRIVATE_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, sharedAuth)).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 + ...SHARED_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, privateAuth)).toStrictEqual([ '"--cfg=C:\\Users\\John Doe/coder"', - "--global-config", - "/config/dir", + ...PRIVATE_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, privateAuth)).toStrictEqual([ "--verbose", - "--global-config", - "/config/dir", + ...PRIVATE_FLAGS, + "--header-command", + "echo test", ]); }); }); @@ -334,20 +257,16 @@ describe("cliConfig", () => { }); describe("isKeyringEnabled", () => { - interface KeyringEnabledCase { + interface Case { 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 }, + it.each([ + { 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 +274,120 @@ 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, - }); - }); - - 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", - ); + function resolve(config: MockConfigurationProvider, version: string) { + const featureSet = featureSetForVersion(semver.parse(version)); + return resolveCliAuth(config, featureSet, URL, EXT_DIR); + } - expect(getGlobalFlags(config, auth)).toStrictEqual([ - "--global-config", - "/custom/coderv2", - ]); + beforeEach(() => { + vi.stubEnv("CODER_CONFIG_DIR", undefined); }); - 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", - ); - - expect(getGlobalFlags(config, auth)).toStrictEqual([ - "--url", - "https://dev.coder.com", - ]); + afterEach(() => { + vi.unstubAllEnvs(); }); - 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", - ]); - }); + interface Case { + scenario: string; + platform: NodeJS.Platform; + override: "none" | "flag" | "env"; + version: string; + expected: string[]; + } - it("ignores globalFlags --global-config on deployments older than 2.31", () => { - vi.mocked(os.platform).mockReturnValue("linux"); + it.each([ + { + scenario: "shares the CLI store when keyring is enabled on 2.29+", + platform: "darwin", + override: "none", + version: "2.29.0", + expected: ["--verbose", ...SHARED_FLAGS, "--use-keyring=true"], + }, + { + scenario: "uses the extension directory when keyring is unsupported", + platform: "linux", + override: "none", + version: "2.29.0", + expected: ["--verbose", ...PRIVATE_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", ...PRIVATE_FLAGS], + }, + { + scenario: "honors a globalFlags --global-config on 2.31+", + platform: "darwin", + override: "flag", + version: "2.31.0", + expected: [ + "--verbose", + `--global-config=${USER_DIR}`, + ...SHARED_FLAGS, + "--use-keyring=true", + ], + }, + { + scenario: "honors CODER_CONFIG_DIR on 2.31+ by emitting no directory", + platform: "darwin", + override: "env", + version: "2.31.0", + expected: ["--verbose", ...SHARED_FLAGS, "--use-keyring=true"], + }, + { + scenario: "honors a globalFlags --global-config with keyring disabled", + platform: "linux", + override: "flag", + version: "2.31.0", + expected: [ + "--verbose", + `--global-config=${USER_DIR}`, + ...SHARED_FLAGS, + "--use-keyring=false", + ], + }, + { + scenario: + "keeps the extension directory over a user directory below 2.31", + platform: "linux", + override: "flag", + version: "2.30.0", + expected: ["--verbose", ...PRIVATE_FLAGS, "--use-keyring=false"], + }, + { + scenario: + "keeps the extension directory over CODER_CONFIG_DIR below 2.31", + platform: "linux", + override: "env", + version: "2.30.0", + expected: ["--verbose", ...PRIVATE_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..ad736feb38 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,12 @@ interface SetupOptions { readonly clearCredentialsResult?: boolean; } +const TEST_SESSION: SessionAuth = { + url: TEST_URL, + token: "test-token", + tokenSource: "extension", +}; + function setup(options: SetupOptions = {}) { vi.clearAllMocks(); const interaction = new MockUserInteraction(); @@ -65,6 +71,7 @@ function setup(options: SetupOptions = {}) { method: "stored_token", user: createMockUser(), token: "test-token", + tokenSource: "extension", } satisfies LoginResultForTest); const loginCoordinator: Pick = { ensureLoggedIn: vi.fn(() => Promise.resolve(loginResult)), @@ -91,9 +98,10 @@ function setup(options: SetupOptions = {}) { 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); @@ -166,6 +174,7 @@ describe("Commands", () => { method: "provided_token", user: createMockUser(), token: "test-token", + tokenSource: "extension", }, }); @@ -258,7 +267,10 @@ describe("Commands", () => { expect(mocks.deploymentManager.clearDeployment).toHaveBeenCalledWith( "logout", ); - expect(mocks.cliManager.clearCredentials).toHaveBeenCalledWith(TEST_URL); + expect(mocks.cliManager.clearCredentials).toHaveBeenCalledWith( + TEST_URL, + TEST_SESSION, + ); expect(mocks.secretsManager.clearAllAuthData).toHaveBeenCalledWith( TEST_HOSTNAME, ); diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index e02d45f4b4..5414c6a9fa 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,460 @@ import { import type * as nodeFs from "node:fs"; -vi.mock("node:child_process", () => ({ - execFile: vi.fn(), -})); +import type { SessionAuth } from "@/core/secretsManager"; -vi.mock("node:os"); +vi.mock("node:child_process", () => ({ execFile: vi.fn() })); -vi.mock("@/settings/cli", async () => { - const actual = - await vi.importActual("@/settings/cli"); - return { ...actual, isKeyringEnabled: vi.fn().mockReturnValue(false) }; -}); +vi.mock("node:os"); 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"; +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 PRIVATE_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", +]; + +const EXTENSION_SESSION: SessionAuth = { + url: TEST_URL, + token: "my-token", + tokenSource: "extension", +}; +const CLI_SESSION: SessionAuth = { ...EXTENSION_SESSION, tokenSource: "cli" }; -// promisify(execFile) always calls execFile(bin, args, opts, callback). -// We extract the options from the third positional argument. -interface ExecFileOptions { +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")); -} - -// 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, - ), -}; +const execCalls = () => + vi.mocked(execFile).mock.calls.map((call) => call[1] as string[]); +const execOptions = () => vi.mocked(execFile).mock.calls[0][2] as ExecOptions; -// 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(): void { + vol.mkdirSync(CRED_DIR, { recursive: true }); + memfs.writeFileSync(`${CRED_DIR}/url`, TEST_URL); + memfs.writeFileSync(`${CRED_DIR}/session`, "old-token"); } -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); -} +const credentialFilesExist = () => + memfs.existsSync(`${CRED_DIR}/url`) || + memfs.existsSync(`${CRED_DIR}/session`); -function credentialFilesExist(dir = CRED_DIR): boolean { - const paths = credentialPaths(dir); - return memfs.existsSync(paths.url) || memfs.existsSync(paths.session); -} +const missingBinary = (): BinaryResolver => + vi.fn().mockRejectedValue(new Error("no binary")); -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.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.31.0"); }); - describe("storeToken", () => { - it("writes via coder login (file mode) when keyring is disabled", async () => { - stubExecFile({ stdout: "" }); - const { manager, resolver, sink } = setup(); + afterEach(() => { + vi.unstubAllEnvs(); + }); - 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", - }, - }); - }); + // Store selection is covered by cliConfig.test.ts; this checks the wiring. + interface Case { + 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: PRIVATE_FLAGS, + store: "private", + }, + { + scenario: "CLI default store when keyring is enabled", + platform: "darwin", + configs, + expected: KEYRING_FLAGS, + store: "shared", + }, + { + scenario: "user --global-config directory", + platform: "linux", + configs: userDirConfigs, + expected: USER_DIR_FLAGS, + store: "shared", + }, + ])( + "targets the $scenario", + async ({ platform, configs, expected, store }) => { + vi.mocked(os.platform).mockReturnValue(platform); + stubExecFile(); + const { manager, sink } = setup(); - it("resolves binary and invokes coder login when keyring enabled", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "" }); - const { manager, resolver, sink } = setup(); + await manager.storeToken(TEST_URL, "token", configs); - 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(execCalls()).toEqual([ + [...expected, "login", "--use-token-as-session", TEST_URL], + ]); expect(sink.expectOne("auth.credential.store")).toMatchObject({ - properties: { - category: "keyring", - keyring_enabled: "true", - result: "success", - }, + properties: { store, 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); - }); - - 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: "" }); + describe("storeToken", () => { + it("passes the token through the environment only", async () => { + stubExecFile(); const { manager } = setup(); - await manager.storeToken(TEST_URL, "token", configs); + await manager.storeToken(TEST_URL, "my-secret-token", configs); - expect(lastExecArgs().args).toEqual([ - "--global-config", - CRED_DIR, - "login", - "--use-token-as-session", - TEST_URL, - ]); + expect(execOptions().env?.CODER_SESSION_TOKEN).toBe("my-secret-token"); + expect(execCalls()[0]).not.toContain("my-secret-token"); }); - it("throws when CLI exec fails", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ error: "login failed" }); + it("throws a CredentialCliError when the CLI fails", async () => { + stubExecFile({ login: new 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: { "error.type": "cli", result: "error" }, }); }); - - 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: "" }); - const { manager } = setup(); - const ac = new AbortController(); - - await manager.storeToken(TEST_URL, "token", configs, { - signal: ac.signal, - }); - - expect(lastExecArgs().signal).toBe(ac.signal); - }); - - it("rejects with AbortError when signal is pre-aborted", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFileAbortable(); - const { manager, sink } = setup(); - - await expect( - manager.storeToken(TEST_URL, "token", configs, { - signal: AbortSignal.abort(), - }), - ).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(); - }); }); 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" }); - 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" }); + 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 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, - ]); + expect(await manager.readToken(TEST_URL, configs)).toBe("my-token"); + expect(execCalls()).toEqual([[...KEYRING_FLAGS, "login", "token"]]); }); - 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.31 without running the CLI", async () => { + vi.mocked(cliExec.version).mockResolvedValue("2.30.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", - }, - }); - }); - - it("deletes files and invokes coder logout (file) when keyring is disabled", async () => { - 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(credentialFilesExist()).toBe(false); - }); - - it("never throws on CLI error", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ error: "logout failed" }); + it.each([ + { scenario: "a CLI token", session: CLI_SESSION }, + { scenario: "no session", session: undefined }, + ])( + "logs out of the extension directory even for $scenario", + async ({ session }) => { + stubExecFile(); + writeCredentialFiles(); + const { manager, sink } = setup(); + + const result = await manager.deleteToken(TEST_URL, configs, session); + + expect(result).toBe(true); + expect(execCalls()).toEqual([[...PRIVATE_FLAGS, "logout", "--yes"]]); + expect(credentialFilesExist()).toBe(false); + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ + properties: { store: "private", result: "success" }, + }); + }, + ); + + 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)).resolves.toBe(false); + await expect( + manager.deleteToken(TEST_URL, configs, EXTENSION_SESSION), + ).resolves.toBe(false); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ - properties: { - "error.type": "cli", - result: "error", - }, + 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", - }, + describe("in a store shared with the CLI", () => { + beforeEach(() => { + vi.mocked(os.platform).mockReturnValue("darwin"); }); - }); - - 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(); + it("logs out when the CLI holds the extension's token", async () => { + stubExecFile({ token: "my-token\n" }); + writeCredentialFiles(); + const { manager, sink } = setup(); - await manager.deleteToken(TEST_URL, configs); + const result = await manager.deleteToken( + TEST_URL, + configs, + EXTENSION_SESSION, + ); + + expect(result).toBe(true); + expect(execCalls()).toEqual([ + [...KEYRING_FLAGS, "login", "token"], + [...KEYRING_FLAGS, "logout", "--yes"], + ]); + expect(credentialFilesExist()).toBe(false); + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ + properties: { store: "shared", result: "success" }, + }); + }); - expect(lastExecArgs().args).toEqual([ - "--global-config", - CRED_DIR, - "logout", - "--url", - TEST_URL, - "--yes", - ]); - expect(credentialFilesExist()).toBe(false); - }); + interface Case { + scenario: string; + session?: SessionAuth; + token?: ExecResult; + } + + it.each([ + { + scenario: "the CLI holds another token", + session: EXTENSION_SESSION, + token: "someone-elses-token", + }, + { + scenario: "the CLI token cannot be read", + session: EXTENSION_SESSION, + token: new Error("keychain locked"), + }, + { scenario: "the token came from the CLI", session: CLI_SESSION }, + { scenario: "there is no session", session: undefined }, + ])("keeps the CLI session when $scenario", async ({ session, token }) => { + stubExecFile({ token }); + writeCredentialFiles(); + const { manager } = setup(); + + const result = await manager.deleteToken(TEST_URL, configs, session); + + expect(result).toBe(true); + expect(execCalls().some((args) => args.includes("logout"))).toBe(false); + expect(credentialFilesExist()).toBe(false); + }); - it("passes signal through to execFile", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "" }); - const { manager } = setup(); - const ac = new AbortController(); + it("logs out without verifying below CLI 2.31", async () => { + vi.mocked(cliExec.version).mockResolvedValue("2.30.0"); + stubExecFile(); + const { manager } = setup(); - await manager.deleteToken(TEST_URL, configs, { signal: ac.signal }); + const result = await manager.deleteToken( + TEST_URL, + configs, + EXTENSION_SESSION, + ); - expect(lastExecArgs().signal).toBe(ac.signal); - }); + expect(result).toBe(true); + expect(execCalls()).toEqual([[...KEYRING_FLAGS, "logout", "--yes"]]); + }); - it("throws AbortError when signal is aborted", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFileAbortable(); - const { manager, sink } = setup(); + it("treats a user --global-config directory as shared", async () => { + vi.mocked(os.platform).mockReturnValue("linux"); + stubExecFile({ token: "my-token" }); + const { manager } = 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" }, + const result = await manager.deleteToken( + TEST_URL, + userDirConfigs, + EXTENSION_SESSION, + ); + + expect(result).toBe(true); + expect(execCalls()).toEqual([ + [...USER_DIR_FLAGS, "login", "token"], + [...USER_DIR_FLAGS, "logout", "--yes"], + ]); }); - expect(event.properties["error.type"]).toBeUndefined(); }); }); + + describe("every CLI call", () => { + type Run = ( + manager: CliCredentialManager, + options: { signal: AbortSignal }, + ) => Promise; + const operations: Array<{ + name: string; + run: Run; + event?: string; + onMissingBinary: (result: Promise) => Promise; + }> = [ + { + name: "storeToken", + run: (m, o) => m.storeToken(TEST_URL, "token", configs, o), + event: "auth.credential.store", + onMissingBinary: (r) => expect(r).rejects.toThrow("no binary"), + }, + { + name: "readToken", + run: (m, o) => m.readToken(TEST_URL, configs, o), + onMissingBinary: (r) => expect(r).resolves.toBeUndefined(), + }, + { + name: "deleteToken", + run: (m, o) => m.deleteToken(TEST_URL, configs, EXTENSION_SESSION, o), + event: "auth.credential.clear", + onMissingBinary: (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 handles a missing binary without running the CLI", + async ({ run, event, onMissingBinary }) => { + const { manager, sink } = setup(missingBinary()); + + await onMissingBinary( + 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..fbaf96b267 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -35,6 +35,18 @@ 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: "shared", + url, + useKeyring: undefined, +}); +const privateAuth = (url: string, configDir: string): CliEnv["auth"] => ({ + store: "private", + url, + configDir, + useKeyring: undefined, +}); + describe("cliExec", () => { const tmp = path.join(os.tmpdir(), "vscode-coder-tests-cliExec"); let echoArgsBin: string; @@ -154,10 +166,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 +191,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 +201,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 +212,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 +230,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 +240,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 +260,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 +298,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 +319,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 +364,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 +375,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..0e73feb5cb 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -309,16 +309,22 @@ describe("CliManager", () => { describe("Clear Credentials", () => { const CLEAR_URL = "https://dev.coder.com"; + const SESSION = { + url: CLEAR_URL, + token: "test-token", + tokenSource: "extension", + } as const; it("should skip progress notification when keyring is disabled", async () => { const { manager, mockCredManager } = setupCliManager(); - await manager.clearCredentials(CLEAR_URL); + await manager.clearCredentials(CLEAR_URL, SESSION); expect(vscode.window.withProgress).not.toHaveBeenCalled(); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( CLEAR_URL, expect.anything(), + SESSION, { signal: expect.any(AbortSignal) }, ); }); @@ -327,7 +333,7 @@ describe("CliManager", () => { const { manager } = setupCliManager(); vi.mocked(isKeyringEnabled).mockReturnValue(true); - await manager.clearCredentials(CLEAR_URL); + await manager.clearCredentials(CLEAR_URL, SESSION); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ @@ -340,23 +346,35 @@ describe("CliManager", () => { }); 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, SESSION), + ).resolves.toEqual(expected); }, ); }); diff --git a/test/unit/core/secretsManager.test.ts b/test/unit/core/secretsManager.test.ts index c66d5c4bbe..5b7fd22312 100644 --- a/test/unit/core/secretsManager.test.ts +++ b/test/unit/core/secretsManager.test.ts @@ -34,6 +34,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", + tokenSource: "extension", }); const auth = await secretsManager.getSessionAuth("example.com"); expect(auth?.token).toBe("test-token"); @@ -42,6 +43,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "new-token", + tokenSource: "extension", }); const newAuth = await secretsManager.getSessionAuth("example.com"); expect(newAuth?.token).toBe("new-token"); @@ -51,11 +53,13 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com:8443", token: "test-token", + tokenSource: "extension", }); expect(await secretsManager.getSessionAuth("example.com")).toEqual({ url: "https://example.com:8443", token: "test-token", + tokenSource: "extension", }); }); @@ -88,6 +92,7 @@ describe("SecretsManager", () => { const existingAuth = { url: "https://example.com", token: "existing-token", + tokenSource: "extension" as const, }; await secretsManager.setSessionAuth("example.com", existingAuth); @@ -95,6 +100,7 @@ describe("SecretsManager", () => { secretsManager.setSessionAuth("example.com", { url, token: "secret-token", + tokenSource: "extension", }), ).rejects.toThrow(error); @@ -129,6 +135,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", + tokenSource: "extension", }); await secretsManager.clearAllAuthData("example.com"); expect( @@ -157,6 +164,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", + tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toContain( "example.com", @@ -165,6 +173,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("other.com", { url: "https://other.com", token: "other-token", + tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toContain( "example.com", @@ -178,6 +187,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", + tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toContain( "example.com", @@ -193,6 +203,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", + tokenSource: "extension", }); secretStorage.corruptStorage(); @@ -205,16 +216,19 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("first.com", { url: "https://first.com", token: "token1", + tokenSource: "extension", }); vi.advanceTimersByTime(10); await secretsManager.setSessionAuth("second.com", { url: "https://second.com", token: "token2", + tokenSource: "extension", }); vi.advanceTimersByTime(10); await secretsManager.setSessionAuth("first.com", { url: "https://first.com", token: "token1-updated", + tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toEqual([ @@ -233,6 +247,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth(`host${i}.com`, { url: `https://host${i}.com`, token: `token${i}`, + tokenSource: "extension", }); vi.advanceTimersByTime(10); } @@ -352,6 +367,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("existing.coder.com", { url: "https://existing.coder.com", token: "existing-token", + tokenSource: "extension", }); // Set up legacy storage with same hostname @@ -387,6 +403,7 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("mtls.coder.com", { url: "https://mtls.coder.com", token: "", + tokenSource: "extension", }); const auth = await secretsManager.getSessionAuth("mtls.coder.com"); @@ -401,6 +418,7 @@ describe("SecretsManager", () => { const authWithExtra = { url: "https://coder.example.com", token: "test-token", + tokenSource: "extension" as const, extraField: "should be stripped", }; @@ -410,6 +428,7 @@ describe("SecretsManager", () => { expect(JSON.parse(raw!)).toEqual({ url: "https://coder.example.com", token: "test-token", + tokenSource: "extension", }); }); @@ -417,6 +436,7 @@ describe("SecretsManager", () => { const authWithExtra = { url: "https://coder.example.com", token: "test-token", + tokenSource: "extension" as const, oauth: { scope: "workspace:read", expiry_timestamp: 12345, @@ -430,6 +450,7 @@ describe("SecretsManager", () => { expect(JSON.parse(raw!)).toEqual({ url: "https://coder.example.com", token: "test-token", + tokenSource: "extension", oauth: { scope: "workspace:read", expiry_timestamp: 12345 }, }); }); @@ -502,9 +523,13 @@ describe("SecretsManager", () => { const sessionAuthCases: BackwardsCompatTestCase[] = [ { - name: "without optional oauth field", + name: "without optional fields, defaulting tokenSource", 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", + tokenSource: "extension", + }, }, { name: "with OAuth without optional fields", @@ -517,6 +542,20 @@ describe("SecretsManager", () => { url: "https://coder.example.com", token: "test-token", oauth: { scope: "workspace:read", expiry_timestamp: 12345 }, + tokenSource: "extension", + }, + }, + { + name: "with a CLI token source", + data: { + url: "https://coder.example.com", + token: "test-token", + tokenSource: "cli", + }, + expected: { + url: "https://coder.example.com", + token: "test-token", + tokenSource: "cli", }, }, ]; diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 1a45bfa596..7985355152 100644 --- a/test/unit/deployment/deploymentManager.test.ts +++ b/test/unit/deployment/deploymentManager.test.ts @@ -325,6 +325,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "stored-token", + tokenSource: "extension", }); const result = await manager.verifyAndApplySession({ @@ -417,6 +418,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "synced-token", + tokenSource: "extension", }); // Simulate cross-window change @@ -447,6 +449,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "", + tokenSource: "extension", }); await secretsManager.setCurrentDeployment({ @@ -487,6 +490,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "refreshed-token", + tokenSource: "extension", }); await flush(); @@ -514,6 +518,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "refreshed-token", + tokenSource: "extension", }); await flush(); await manager.clearDeployment("logout"); @@ -543,6 +548,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "rotated-token", + tokenSource: "extension", }); await flush(); @@ -571,6 +577,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "rotated-token", + tokenSource: "extension", }); await flush(); @@ -598,6 +605,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "rotated-token", + tokenSource: "extension", }); await flush(); @@ -718,6 +726,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "", + tokenSource: "extension", }); await manager.setDeployment({ url: TEST_URL, @@ -811,6 +820,7 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "recovered-token", + tokenSource: "extension", }); await flush(); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 44bfbb7982..6096e381e1 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -3,7 +3,7 @@ import { describe, expect, it, vi, type Mock } from "vitest"; import * as vscode from "vscode"; import { MementoManager } from "@/core/mementoManager"; -import { SecretsManager } from "@/core/secretsManager"; +import { SecretsManager, type TokenSource } from "@/core/secretsManager"; import { getHeaders } from "@/headers"; import { AuthTelemetry } from "@/instrumentation/auth"; import { LoginCoordinator, type LoginMethod } from "@/login/loginCoordinator"; @@ -179,34 +179,84 @@ 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, + tokenSource: "extension", + ...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 () => { - const { secretsManager, coordinator, mockSuccessfulAuth } = - createTestContext(); - const user = mockSuccessfulAuth(); - - // Pre-store a token - await secretsManager.setSessionAuth(TEST_HOSTNAME, { - url: TEST_URL, - token: "stored-token", - }); + interface Case { + tokenSource: TokenSource; + } - const result = await coordinator.ensureLoggedIn({ - url: TEST_URL, - safeHostname: TEST_HOSTNAME, - }); + it.each([{ tokenSource: "extension" }, { tokenSource: "cli" }])( + "authenticates with a stored token and keeps its $tokenSource source", + async ({ tokenSource }) => { + const { secretsManager, coordinator, mockSuccessfulAuth } = + createTestContext(); + const user = mockSuccessfulAuth(); + await secretsManager.setSessionAuth(TEST_HOSTNAME, { + url: TEST_URL, + token: "stored-token", + tokenSource, + }); - expect(result).toEqual({ - success: true, - method: "stored_token", - user, - token: "stored-token", - }); + const result = await coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }); - const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); - expect(auth?.token).toBe("stored-token"); - }); + expect(result).toEqual({ + success: true, + method: "stored_token", + user, + token: "stored-token", + tokenSource, + }); + const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); + expect(auth?.tokenSource).toBe(tokenSource); + }, + ); it("authenticates with CLI credential token on success", async () => { const { @@ -216,10 +266,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, @@ -231,33 +280,13 @@ describe("LoginCoordinator", () => { method: "cli_token", user, token: "cli-credential-token", + tokenSource: "cli", }); expect(vscode.window.showInputBox).not.toHaveBeenCalled(); const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); 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", - }); + expect(auth?.tokenSource).toBe("cli"); }); it("prompts for token when no stored auth exists", async () => { @@ -283,11 +312,13 @@ describe("LoginCoordinator", () => { method: "cli_token", user, token: "new-token", + tokenSource: "extension", }); // Verify new token was persisted const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); expect(auth?.token).toBe("new-token"); + expect(auth?.tokenSource).toBe("extension"); }); it("returns success false when user cancels input", async () => { @@ -364,6 +395,7 @@ describe("LoginCoordinator", () => { method: "mtls", user, token: "", + tokenSource: "extension", }); // Verify empty string token was persisted @@ -445,38 +477,17 @@ describe("LoginCoordinator", () => { method, user, token, + tokenSource: "extension", }); - /** 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 +495,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(), }; } @@ -724,6 +720,7 @@ describe("LoginCoordinator", () => { await ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "stored-token", + tokenSource: "extension", }); const login = async () => { const result = await ctx.coordinator.ensureLoggedIn({ @@ -783,7 +780,95 @@ describe("LoginCoordinator", () => { method: "stored_token", user, token: "stored-token", + tokenSource: "extension", + }); + 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 session?"; + + function createCliTestContext() { + const ctx = createSignInTestContext( + CLI_PROMPT, + (username) => `The Coder CLI 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, + tokenSource: "cli", + }); + 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", + tokenSource: "cli", + }); + 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", + tokenSource: "extension", }); + 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..c170817531 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -90,6 +90,7 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { await base.secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: overrides.token ?? "access-token", + tokenSource: "extension", username: overrides.username, oauth: { refresh_token: overrides.refreshToken ?? "refresh-token", @@ -149,6 +150,7 @@ describe("OAuthSessionManager", () => { auth: { url: TEST_URL, token: "access-token", + tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -164,7 +166,11 @@ 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", + tokenSource: "extension", + }, expected: false, }, ])("$name", async ({ auth, expected }) => { @@ -254,6 +260,7 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: `${TEST_URL}:8443`, token: "access-token", + tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -508,6 +515,7 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "access-token", + tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -525,6 +533,7 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "access-token", + tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, diff --git a/test/unit/remote/migration.test.ts b/test/unit/remote/migration.test.ts index 0eadd0c27b..a03f5a4e1a 100644 --- a/test/unit/remote/migration.test.ts +++ b/test/unit/remote/migration.test.ts @@ -56,6 +56,7 @@ describe("Session auth migration", () => { expect(secretsManager.setSessionAuth).toHaveBeenCalledWith(HOSTNAME, { url: "https://dep.example.com", token: "legacy-token", + tokenSource: "extension", }); expect(vol.existsSync(URL_PATH)).toBe(false); expect(vol.existsSync(TOKEN_PATH)).toBe(false); @@ -76,7 +77,11 @@ 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", + tokenSource: "extension", + }, }); writeLegacyFiles(); diff --git a/test/unit/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index e437b67467..78c1494e13 100644 --- a/test/unit/remote/workspaceStateMachine.test.ts +++ b/test/unit/remote/workspaceStateMachine.test.ts @@ -109,7 +109,7 @@ function setup( startupMode, "/usr/bin/coder", {} as FeatureSet, - { mode: "url", url: "https://test.coder.com" }, + { store: "shared", url: "https://test.coder.com", useKeyring: undefined }, createMockServiceContainer({ telemetry, logger: createMockLogger() }), ); return { sm, progress, userInteraction }; diff --git a/test/unit/uri/uriHandler.test.ts b/test/unit/uri/uriHandler.test.ts index 1d8bbce101..273d92427c 100644 --- a/test/unit/uri/uriHandler.test.ts +++ b/test/unit/uri/uriHandler.test.ts @@ -74,6 +74,7 @@ function createMockLoginCoordinator(secretsManager: SecretsManager) { await secretsManager.setSessionAuth(options.safeHostname, { url: options.url, token, + tokenSource: "extension", }); return { success: true, @@ -159,6 +160,7 @@ function createTestContext() { secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "known-token", + tokenSource: "extension", ...auth, }), @@ -531,6 +533,7 @@ describe("uriHandler", () => { expect(await t.secretsManager.getSessionAuth(TEST_HOSTNAME)).toEqual({ url: TEST_URL, token: "tok", + tokenSource: "extension", }); }); diff --git a/test/unit/util/credentials.test.ts b/test/unit/util/credentials.test.ts new file mode 100644 index 0000000000..101aea4bc4 --- /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 Case { + 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", + ); + }); +}); From f98b0b6e50b8599366a5dfb971cb3f155761f1c0 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 10 Sep 2026 19:26:22 +0200 Subject: [PATCH 2/5] fix: scope CLI credential reads to https and CLI 2.32 The CLI keys keyring entries by host without the scheme, so an http lookup returns the https deployment's token. File reads are left to the CLI, whose file mode checks the stored URL against --url from 2.32. --- CHANGELOG.md | 5 +++++ package.json | 4 ++-- src/core/cliCredentialManager.ts | 9 +++++++-- src/featureSet.ts | 4 ++-- src/settings/cli.ts | 2 +- test/unit/cliConfig.test.ts | 18 +++++++++--------- test/unit/core/cliCredentialManager.test.ts | 21 ++++++++++++++++----- test/unit/featureSet.test.ts | 4 ++-- 8 files changed, 44 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29fc19781d..e0fda5ff8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ - 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 @@ -27,6 +29,9 @@ - Sign out the `coder` CLI only when it still holds the token this extension created. A session that came from the CLI is removed from the extension without signing the CLI out. +- Read the CLI's keyring entry only for `https` deployments. The entry is keyed + by host, so an `http` address for the same host would receive the `https` + session's token. ## [v1.16.2](https://github.com/coder/vscode-coder/releases/tag/v1.16.2) 2026-08-25 diff --git a/package.json b/package.json index 6f91baa8ce..4b9f278372 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\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.31.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.", + "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,7 +204,7 @@ "ignoreSync": true }, "coder.useKeyring": { - "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.31.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 signs out the CLI only when it still holds the token this extension created.", + "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 signs out the CLI only when it still holds the token this extension created.", "type": "boolean", "default": true, "scope": "application" diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 2589981a1e..b59b7ce50f 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -80,7 +80,7 @@ export class CliCredentialManager { }); } - /** Reads the CLI's token via `coder login token` (CLI 2.31+). Undefined on any failure. */ + /** Reads the CLI's token via `coder login token` (CLI 2.32+). Undefined on any failure. */ public async readToken( url: string, configs: Pick, @@ -96,6 +96,11 @@ export class CliCredentialManager { if (!cli.featureSet.tokenRead) { return undefined; } + // Keyring entries drop the scheme, so an http lookup returns the https token. + if (cli.auth.useKeyring && !url.startsWith("https:")) { + this.logger.warn("Refusing to read keyring credentials for", url); + return undefined; + } return this.readCliToken(cli, options?.signal); } @@ -185,7 +190,7 @@ export class CliCredentialManager { if (session?.tokenSource !== "extension") { return false; } - // Below 2.31 the CLI cannot report its token; trust the provenance. + // Below 2.32 the token is not read back; trust the provenance. if (!cli.featureSet.tokenRead) { return true; } diff --git a/src/featureSet.ts b/src/featureSet.ts index 1f8d53e52d..1ac9b17e6e 100644 --- a/src/featureSet.ts +++ b/src/featureSet.ts @@ -54,8 +54,8 @@ 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` diff --git a/src/settings/cli.ts b/src/settings/cli.ts index c67385daa5..8b7e1c1d32 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -125,7 +125,7 @@ export function resolveCliAuth( const useKeyring = featureSet.keyringAuth ? isKeyringEnabled(configs) : undefined; - // A user directory is honored on 2.31+, where the CLI can report its token. + // A user directory is honored on 2.32+, where the CLI reports its token. const userDir = hasUserConfigDir(configs) && featureSet.tokenRead; if (useKeyring || userDir) { return { store: "shared", url, useKeyring }; diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index a8f0979873..6f9e5e0fa2 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -328,10 +328,10 @@ describe("cliConfig", () => { expected: ["--verbose", ...PRIVATE_FLAGS], }, { - scenario: "honors a globalFlags --global-config on 2.31+", + scenario: "honors a globalFlags --global-config on 2.32+", platform: "darwin", override: "flag", - version: "2.31.0", + version: "2.32.0", expected: [ "--verbose", `--global-config=${USER_DIR}`, @@ -340,17 +340,17 @@ describe("cliConfig", () => { ], }, { - scenario: "honors CODER_CONFIG_DIR on 2.31+ by emitting no directory", + scenario: "honors CODER_CONFIG_DIR on 2.32+ by emitting no directory", platform: "darwin", override: "env", - version: "2.31.0", + version: "2.32.0", expected: ["--verbose", ...SHARED_FLAGS, "--use-keyring=true"], }, { scenario: "honors a globalFlags --global-config with keyring disabled", platform: "linux", override: "flag", - version: "2.31.0", + version: "2.32.0", expected: [ "--verbose", `--global-config=${USER_DIR}`, @@ -360,18 +360,18 @@ describe("cliConfig", () => { }, { scenario: - "keeps the extension directory over a user directory below 2.31", + "keeps the extension directory over a user directory below 2.32", platform: "linux", override: "flag", - version: "2.30.0", + version: "2.31.0", expected: ["--verbose", ...PRIVATE_FLAGS, "--use-keyring=false"], }, { scenario: - "keeps the extension directory over CODER_CONFIG_DIR below 2.31", + "keeps the extension directory over CODER_CONFIG_DIR below 2.32", platform: "linux", override: "env", - version: "2.30.0", + version: "2.31.0", expected: ["--verbose", ...PRIVATE_FLAGS, "--use-keyring=false"], }, ])("$scenario", ({ platform, override, version, expected }) => { diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 5414c6a9fa..a1d1d7a68f 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -157,7 +157,7 @@ describe("CliCredentialManager", () => { 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.31.0"); + vi.mocked(cliExec.version).mockResolvedValue("2.32.0"); }); afterEach(() => { @@ -257,8 +257,19 @@ describe("CliCredentialManager", () => { expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); }); - it("returns undefined below CLI 2.31 without running the CLI", async () => { - vi.mocked(cliExec.version).mockResolvedValue("2.30.0"); + it("refuses the keyring for a non-HTTPS URL without running the CLI", async () => { + vi.mocked(os.platform).mockReturnValue("darwin"); + stubExecFile({ token: "my-token" }); + const { manager } = setup(); + + expect( + await manager.readToken("http://dev.coder.com", configs), + ).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); + }); + + 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(); @@ -358,8 +369,8 @@ describe("CliCredentialManager", () => { expect(credentialFilesExist()).toBe(false); }); - it("logs out without verifying below CLI 2.31", async () => { - vi.mocked(cliExec.version).mockResolvedValue("2.30.0"); + it("logs out without verifying below CLI 2.32", async () => { + vi.mocked(cliExec.version).mockResolvedValue("2.31.0"); stubExecFile(); const { manager } = setup(); diff --git a/test/unit/featureSet.test.ts b/test/unit/featureSet.test.ts index ccfd508065..702a78e3ed 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", () => { From d59211f15fdcd665f5f4501a688d3303689985a4 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 14 Sep 2026 12:14:21 +0200 Subject: [PATCH 3/5] fix: skip CLI credential steps without a binary and ask before signing out the CLI A missing CLI binary is no longer an error for credential operations. Storing and reading at login, and logging out, skip the CLI with an info log, since connecting downloads the binary and stores the token again. Only a binary that exists but cannot be resolved still reports an error. Logout asks whether to sign out the CLI too when its own store holds the same token as the extension, read with `coder login token` on 2.32+ (older CLIs cannot be read, so the CLI's store always asks). A CLI signed in with a different token is left alone. This drops the `tokenSource` field and the stored provenance it fed. The extension's own store is always logged out, since `coder logout` revokes the token. OAuth sessions skip the prompt because logout revokes their token anyway. Manage Stored Credentials asks the same question for one deployment and signs the CLI out for Remove All wherever it holds the same token. `CliAuth.store` is now `cli` or `extension` instead of `shared` or `private`, naming whose store it is. The http keyring guard is removed. The CLI keys entries by host and `coder login token` prints only the token, so the extension cannot check the scheme; coder/coder#29290 tracks verifying `--url` in the CLI. The credential spans record an `outcome` so skipped and kept operations are distinguishable from ones that ran the CLI. The store-failure error now shows the CLI's stderr instead of a generic wrapper message. --- CHANGELOG.md | 20 +- package.json | 2 +- src/commands.ts | 114 +++++-- src/core/cliCredentialManager.ts | 186 +++++----- src/core/cliManager.ts | 37 +- src/core/secretsManager.ts | 8 - src/instrumentation/EVENTS.md | 19 +- src/instrumentation/auth.ts | 5 +- src/instrumentation/credentials.ts | 10 +- src/login/loginCoordinator.ts | 24 +- src/oauth/sessionManager.ts | 1 - src/remote/migration.ts | 1 - src/settings/cli.ts | 16 +- test/mocks/testHelpers.ts | 1 + test/unit/api/authInterceptor.test.ts | 4 - test/unit/api/workspace.test.ts | 2 +- test/unit/cliConfig.test.ts | 82 ++--- test/unit/commands.telemetry.test.ts | 80 ++++- test/unit/core/cliCredentialManager.test.ts | 319 +++++++++--------- test/unit/core/cliExec.test.ts | 4 +- test/unit/core/cliManager.test.ts | 81 +++-- test/unit/core/secretsManager.test.ts | 38 +-- .../unit/deployment/deploymentManager.test.ts | 10 - test/unit/login/loginCoordinator.test.ts | 67 ++-- test/unit/oauth/sessionManager.test.ts | 6 - test/unit/remote/migration.test.ts | 2 - .../unit/remote/workspaceStateMachine.test.ts | 2 +- test/unit/uri/uriHandler.test.ts | 3 - test/utils/platform.ts | 5 +- 29 files changed, 585 insertions(+), 564 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0fda5ff8c..e93d630fd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,13 @@ ### Changed -- Store session tokens in the OS keyring by default on macOS and Windows. The - entry is shared with the `coder` CLI, so 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`. +- 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`. @@ -24,15 +27,6 @@ login, and a **Show Output** button when logout cannot remove every credential. -### Security - -- Sign out the `coder` CLI only when it still holds the token this extension - created. A session that came from the CLI is removed from the extension - without signing the CLI out. -- Read the CLI's keyring entry only for `https` deployments. The entry is keyed - by host, so an `http` address for the same host would receive the `https` - session's token. - ## [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 4b9f278372..59d0bdfc47 100644 --- a/package.json +++ b/package.json @@ -204,7 +204,7 @@ "ignoreSync": true }, "coder.useKeyring": { - "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 signs out the CLI only when it still holds the token this extension created.", + "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": true, "scope": "application" diff --git a/src/commands.ts b/src/commands.ts index 6a663faa92..9dee5decde 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,37 +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 session = await this.secretsManager.getSessionAuth( - deployment.safeHostname, - ); - const cleared = await this.cliManager.clearCredentials( - deployment.url, - session, - ); - 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.', - "Show Output", - ) - .then((action) => { - if (action === "Show Output") { - this.logger.show(); - } - }); - return { success: false, reason: "cleanup_incomplete" }; - } + 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.', + "Show Output", + ) + .then((action) => { + if (action === "Show Output") { + this.logger.show(); + } + }); + return { success: false, reason: "cleanup_incomplete" }; } this.showLogoutMessage(); @@ -748,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. @@ -803,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, auth); + 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); @@ -816,7 +854,7 @@ 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", ); @@ -825,7 +863,12 @@ export class Commands { selected.hostnames.map(async (h) => { const auth = await this.secretsManager.getSessionAuth(h); if (auth?.url) { - await this.cliManager.clearCredentials(auth.url, auth); + await this.cliManager.clearCredentials(auth.url, { + signOutCli: await this.cliManager.holdsToken( + auth.url, + auth.token, + ), + }); } await this.secretsManager.clearAllAuthData(h); }), @@ -1423,12 +1466,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 b59b7ce50f..92866db607 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -6,9 +6,11 @@ import * as semver from "semver"; import { isAbortError } from "../error/errorUtils"; import { featureSetForVersion, type FeatureSet } from "../featureSet"; import { + categorizeCredentialError, CredentialCliError, CredentialTelemetry, } from "../instrumentation/credentials"; +import { recordError } from "../instrumentation/outcomes"; import { type CliAuth, getGlobalFlags, resolveCliAuth } from "../settings/cli"; import { type TelemetryReporter } from "../telemetry/reporter"; import { toSafeHost } from "../util/uri"; @@ -21,7 +23,6 @@ import type { Logger } from "../logging/logger"; import type { Span } from "../telemetry/span"; import type { PathResolver } from "./pathResolver"; -import type { SessionAuth } from "./secretsManager"; const execFileAsync = promisify(execFile); @@ -35,11 +36,10 @@ interface ResolvedCli { flags: string[]; } -/** - * 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; +/** 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 { @@ -54,7 +54,7 @@ export class CliCredentialManager { this.credentialTelemetry = new CredentialTelemetry(telemetry); } - /** Stores a token via `coder login`. Throws when the binary or the CLI fails. */ + /** Stores a token via `coder login`. Skipped until the CLI is downloaded; throws when the CLI fails. */ public storeToken( url: string, token: string, @@ -63,20 +63,20 @@ export class CliCredentialManager { ): Promise { return this.credentialTelemetry.traceStore(configs, async (span) => { const cli = await this.resolveCli(url, configs); - span.setProperty("store", cli.auth.store); - try { - await this.exec(cli, ["login", "--use-token-as-session", url], { - env: { ...process.env, CODER_SESSION_TOKEN: token }, - signal: options?.signal, - }); - this.logger.info("Stored token via CLI for", url); - } catch (error) { - this.logger.warn("Failed to store token via CLI:", error); - if (isAbortError(error)) { - throw error; - } - throw new CredentialCliError(error); + 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); }); } @@ -86,58 +86,71 @@ export class CliCredentialManager { configs: Pick, options?: { signal?: AbortSignal }, ): Promise { - let cli: ResolvedCli; try { - cli = await this.resolveCli(url, configs); + const cli = await this.resolveCli(url, configs); + if (!cli) { + this.logger.debug("No CLI session to read: the CLI is not downloaded"); + return undefined; + } + if (!cli.featureSet.tokenRead) { + return undefined; + } + return await this.cliToken(cli, options?.signal); } catch (error) { - this.logger.warn("Could not resolve CLI binary:", error); - return undefined; - } - if (!cli.featureSet.tokenRead) { - return undefined; - } - // Keyring entries drop the scheme, so an http lookup returns the https token. - if (cli.auth.useKeyring && !url.startsWith("https:")) { - this.logger.warn("Refusing to read keyring credentials for", url); + if (isAbortError(error)) { + throw error; + } + this.logger.info( + "Could not read the CLI session (it may not be signed in):", + error, + ); return undefined; } - return this.readCliToken(cli, options?.signal); } - private async readCliToken( - cli: ResolvedCli, - signal: AbortSignal | undefined, - ): Promise { + /** + * 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 holdsToken( + url: string, + token: string, + configs: Pick, + ): Promise { try { - const { stdout } = await this.exec(cli, ["login", "token"], { signal }); - return stdout.trim() || undefined; - } catch (error) { - if (isAbortError(error)) { - throw error; + const cli = await this.resolveCli(url, configs); + if (cli?.auth.store !== "cli") { + return false; } - this.logger.warn("Failed to read token via CLI:", error); - return undefined; + return !cli.featureSet.tokenRead || (await this.cliToken(cli)) === token; + } catch (error) { + this.logger.warn("Could not read the CLI session:", error); + return false; } } + private async cliToken( + cli: ResolvedCli, + signal?: AbortSignal, + ): Promise { + const { stdout } = await this.exec(cli, ["login", "token"], { signal }); + return stdout.trim() || undefined; + } + /** - * Deletes the extension's credential files and runs `coder logout` when the - * CLI session is ours (see `ownsCliSession`). Returns whether every store - * was cleared; throws only on abort. + * 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, - session: SessionAuth | undefined, - 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, session, { - signal: options?.signal, - span, - }), + this.cliLogout(url, configs, { ...options, span }), ]); return filesCleared && cliCleared; }); @@ -146,25 +159,27 @@ export class CliCredentialManager { private async cliLogout( url: string, configs: Pick, - session: SessionAuth | undefined, - { signal, span }: { signal?: AbortSignal; span: Span }, + { + signal, + signOutCli, + span, + }: { signal?: AbortSignal; signOutCli: boolean; span: Span }, ): Promise { - let cli: ResolvedCli; - try { - cli = await this.resolveCli(url, configs); - } catch (error) { - this.logger.warn("Could not resolve CLI binary for logout:", error); - span.setProperty("error.type", "binary"); - span.markError(); - return false; - } - span.setProperty("store", cli.auth.store); - if (!(await this.ownsCliSession(cli, session, signal))) { - this.logger.info("Kept the CLI session for", url); - return true; - } try { + 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) { @@ -172,37 +187,19 @@ export class CliCredentialManager { throw error; } this.logger.warn("Failed to log out via CLI:", error); - span.setProperty("error.type", "cli"); - span.markError(); + recordError(span, categorizeCredentialError(error)); return false; } } - /** A shared store is ours only if the CLI still holds the token this extension created. */ - private async ownsCliSession( - cli: ResolvedCli, - session: SessionAuth | undefined, - signal: AbortSignal | undefined, - ): Promise { - if (cli.auth.store === "private") { - return true; - } - if (session?.tokenSource !== "extension") { - return false; - } - // Below 2.32 the token is not read back; trust the provenance. - if (!cli.featureSet.tokenRead) { - return true; - } - const cliToken = await this.readCliToken(cli, signal); - return cliToken === session.token; - } - private async resolveCli( url: string, configs: Pick, - ): Promise { + ): Promise { const binPath = await this.resolveBinary(url); + if (!binPath) { + return undefined; + } const featureSet = featureSetForVersion( semver.parse(await version(binPath)), ); @@ -211,7 +208,7 @@ export class CliCredentialManager { return { binPath, featureSet, auth, flags: getGlobalFlags(configs, auth) }; } - /** Runs a subcommand with a 60s timeout and periodic debug logging. */ + /** Runs a subcommand with a 60s timeout. Failures become `CredentialCliError`; aborts pass through. */ private async exec( cli: ResolvedCli, args: string[], @@ -225,6 +222,11 @@ export class CliCredentialManager { ...options, timeout: EXEC_TIMEOUT_MS, }); + } catch (error) { + if (isAbortError(error)) { + throw error; + } + throw new CredentialCliError(error); } finally { clearInterval(timer); } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 5cd786d70d..91470d561f 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -42,7 +42,6 @@ import type { Span } from "../telemetry/span"; import type { CliCredentialManager } from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; -import type { SessionAuth } from "./secretsManager"; type ResolvedBinary = | { binPath: string; stat: Stats; source: "file_path" | "directory" } @@ -72,17 +71,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; } /** @@ -1069,23 +1061,32 @@ export class CliManager { this.handleStoreError(result.error, configs); } + /** True when the CLI's own store holds this token. */ + public holdsToken(url: string, token: string): Promise { + return this.cliCredentialManager.holdsToken( + url, + token, + vscode.workspace.getConfiguration(), + ); + } + /** - * Remove credentials for a deployment. A store shared with the CLI is only - * logged out of a token this extension created, so pass the stored - * `session`. 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, - session: SessionAuth | undefined, + { signOutCli }: { signOutCli: boolean }, ): Promise { const configs = vscode.workspace.getConfiguration(); const result = await withOptionalProgress( ({ signal }) => - this.cliCredentialManager.deleteToken(url, configs, session, { + this.cliCredentialManager.deleteToken(url, configs, { signal, + signOutCli, }), { - enabled: isKeyringEnabled(configs), + enabled: signOutCli && isKeyringEnabled(configs), location: vscode.ProgressLocation.Notification, title: `Removing credentials for ${url}`, cancellable: true, diff --git a/src/core/secretsManager.ts b/src/core/secretsManager.ts index db8e0880a9..28fbd52e9c 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -42,11 +42,6 @@ const OAuthTokenDataSchema = z.object({ export type OAuthTokenData = z.infer; -const TokenSourceSchema = z.enum(["extension", "cli"]); - -/** Who minted a session token: this extension, or the Coder CLI. */ -export type TokenSource = z.infer; - const SessionAuthSchema = z.object({ url: z.string(), token: z.string(), @@ -54,8 +49,6 @@ const SessionAuthSchema = z.object({ username: z.string().optional(), /** If present, this session uses OAuth authentication */ oauth: OAuthTokenDataSchema.optional(), - /** Only extension tokens are revoked at logout. Older sessions predate the CLI source. */ - tokenSource: TokenSourceSchema.default("extension"), }); export type SessionAuth = z.infer; @@ -319,7 +312,6 @@ export class SecretsManager { await this.setSessionAuth(safeHostname, { url: legacyUrl, token: oldToken ?? "", - tokenSource: "extension", }); } diff --git a/src/instrumentation/EVENTS.md b/src/instrumentation/EVENTS.md index 82d045104e..f42db9b421 100644 --- a/src/instrumentation/EVENTS.md +++ b/src/instrumentation/EVENTS.md @@ -168,10 +168,10 @@ Emitted by `AuthTelemetry`; the credential events by `CredentialTelemetry`. #### `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) | -| `store` | `shared` (the CLI's own store), `private` (the extension's directory) | -| `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 81a31d82f3..44735397bb 100644 --- a/src/instrumentation/credentials.ts +++ b/src/instrumentation/credentials.ts @@ -63,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 da7178778a..92e1fdc1f5 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -22,7 +22,6 @@ import type { OAuthTokenData, SecretsManager, SessionAuth, - TokenSource, } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; import type { @@ -48,7 +47,6 @@ export type LoginResult = user: User; token: string; oauth?: OAuthTokenData; - tokenSource: TokenSource; }; export interface LoginOptions { @@ -206,7 +204,6 @@ export class LoginCoordinator implements vscode.Disposable { token: result.token, username: result.user.username, oauth: result.oauth, // undefined for non-OAuth logins - tokenSource: result.tokenSource, }); await this.mementoManager.addToUrlHistory(url); @@ -315,7 +312,6 @@ export class LoginCoordinator implements vscode.Disposable { return withLoginMethod( "mtls", await this.tryMtlsAuth(client, isAutoLogin), - "extension", ); } @@ -399,7 +395,7 @@ export class LoginCoordinator implements vscode.Disposable { } } } - return withLoginMethod("provided_token", result, "extension"); + return withLoginMethod("provided_token", result); } /** Stored session for the deployment's exact origin, if it still works. */ @@ -419,7 +415,7 @@ export class LoginCoordinator implements vscode.Disposable { if (result === "unauthorized") { return undefined; } - return withLoginMethod("stored_token", result, sameOriginAuth.tokenSource); + return withLoginMethod("stored_token", result); } /** The CLI's own session, adopted after confirmation if it is another user's. */ @@ -436,7 +432,7 @@ export class LoginCoordinator implements vscode.Disposable { { enabled: isKeyringEnabled(configs), location: vscode.ProgressLocation.Notification, - title: "Reading credentials from the Coder CLI...", + title: "Reading credentials from the Coder CLI", cancellable: true, }, ); @@ -455,8 +451,8 @@ export class LoginCoordinator implements vscode.Disposable { const confirmed = await this.confirmSignIn( deployment.url, { - title: "Sign in with the Coder CLI session?", - detail: `The Coder CLI session signs you in as "${result.user.username}"`, + 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 }, @@ -465,7 +461,7 @@ export class LoginCoordinator implements vscode.Disposable { return undefined; } } - return withLoginMethod("cli_token", result, "cli"); + return withLoginMethod("cli_token", result); } /** Last resort: ask the user how to authenticate. */ @@ -476,13 +472,11 @@ export class LoginCoordinator implements vscode.Disposable { return withLoginMethod( "oauth", await this.loginWithOAuth(ctx.deployment), - "extension", ); case "legacy": return withLoginMethod( "cli_token", await this.loginWithToken(ctx.client), - "extension", ); case undefined: return { success: false, reason: "user_dismissed" }; @@ -680,10 +674,6 @@ export class LoginCoordinator implements vscode.Disposable { function withLoginMethod( method: LoginMethod, result: LoginAttemptResult, - tokenSource: TokenSource, ): LoginResult { - if (!result.success) { - return { ...result, method }; - } - return { ...result, method, tokenSource }; + return { ...result, method }; } diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 4fa3cf051b..5ccc122aa5 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -436,7 +436,6 @@ export class OAuthSessionManager implements vscode.Disposable { tokenResponse.access_token, ), oauth: buildOAuthTokenData(tokenResponse), - tokenSource: "extension", }); return tokenResponse; diff --git a/src/remote/migration.ts b/src/remote/migration.ts index 3db978b520..c18a68fde7 100644 --- a/src/remote/migration.ts +++ b/src/remote/migration.ts @@ -75,7 +75,6 @@ async function migrateSessionAuthFromFiles( await secretsManager.setSessionAuth(safeHostname, { url: url.value.trim(), token: token.value.trim(), - tokenSource: "extension", }); } catch (error) { logger.warn("Failed to migrate session auth from files:", error); diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 8b7e1c1d32..3677475d5b 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -8,11 +8,11 @@ import type { WorkspaceConfiguration } from "vscode"; import type { FeatureSet } from "../featureSet"; -/** The CLI's own store, shared with the terminal CLI, or a file in the extension's private directory. */ +/** 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 = - | { store: "shared"; url: string; useKeyring: boolean | undefined } + | { store: "cli"; url: string; useKeyring: boolean | undefined } | { - store: "private"; + store: "extension"; url: string; configDir: string; useKeyring: false | undefined; @@ -61,9 +61,9 @@ function buildGlobalFlags( // Escape after stripping so expansion whitespace stays in one shell token. const flags = stripManagedFlags( getExpandedUserGlobalFlags(configs), - auth.store === "private", + auth.store === "extension", ).map(escAuth); - if (auth.store === "private") { + if (auth.store === "extension") { flags.push("--global-config", escAuth(auth.configDir)); } flags.push("--url", escAuth(auth.url)); @@ -114,7 +114,7 @@ export function isKeyringEnabled( return isKeyringSupported() && configs.get("coder.useKeyring", true); } -/** Shares the CLI's store when the keyring is on or the user set a config directory. */ +/** 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, @@ -128,9 +128,9 @@ export function resolveCliAuth( // A user directory is honored on 2.32+, where the CLI reports its token. const userDir = hasUserConfigDir(configs) && featureSet.tokenRead; if (useKeyring || userDir) { - return { store: "shared", url, useKeyring }; + return { store: "cli", url, useKeyring }; } - return { store: "private", url, configDir, useKeyring }; + return { store: "extension", url, configDir, useKeyring }; } function hasUserConfigDir( diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 6808285630..e01a25ac5f 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -523,6 +523,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), deleteToken: vi.fn().mockResolvedValue(true), + holdsToken: vi.fn().mockResolvedValue(false), } as unknown as CliCredentialManager; } diff --git a/test/unit/api/authInterceptor.test.ts b/test/unit/api/authInterceptor.test.ts index 10b3694189..7056778061 100644 --- a/test/unit/api/authInterceptor.test.ts +++ b/test/unit/api/authInterceptor.test.ts @@ -122,7 +122,6 @@ function createTestContext() { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "access-token", - tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -145,7 +144,6 @@ function createTestContext() { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "session-token", - tokenSource: "extension", }); }; @@ -154,7 +152,6 @@ function createTestContext() { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "", - tokenSource: "extension", }); }; @@ -300,7 +297,6 @@ describe("AuthInterceptor", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "new-token-after-login", - tokenSource: "extension", }); const retryResponse = { data: "success", status: 200 }; diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index c9c9ea4500..2e563198f1 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -95,7 +95,7 @@ function createUpdateCtx( const ctx = { restClient: restClient as unknown as Api, auth: { - store: "shared" as const, + store: "cli" as const, url: "https://test.coder.com", useKeyring: undefined, }, diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 6f9e5e0fa2..45cd385387 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -22,20 +22,20 @@ const URL = "https://dev.coder.com"; const EXT_DIR = "/config/dir"; const USER_DIR = "/custom/coderv2"; -const privateAuth: CliAuth = { - store: "private", +const extensionStoreAuth: CliAuth = { + store: "extension", url: URL, configDir: EXT_DIR, useKeyring: undefined, }; -const sharedAuth: CliAuth = { - store: "shared", +const cliStoreAuth: CliAuth = { + store: "cli", url: URL, useKeyring: undefined, }; -const PRIVATE_FLAGS = ["--global-config", EXT_DIR, "--url", URL]; -const SHARED_FLAGS = ["--url", URL]; +const EXTENSION_FLAGS = ["--global-config", EXT_DIR, "--url", URL]; +const CLI_FLAGS = ["--url", URL]; describe("cliConfig", () => { describe("getGlobalShellFlags", () => { @@ -46,17 +46,21 @@ describe("cliConfig", () => { } it.each([ - { scenario: "private store", auth: privateAuth, expected: PRIVATE_FLAGS }, - { scenario: "shared store", auth: sharedAuth, expected: SHARED_FLAGS }, { - scenario: "private store with keyring off", - auth: { ...privateAuth, useKeyring: false }, - expected: [...PRIVATE_FLAGS, "--use-keyring=false"], + scenario: "extension store", + auth: extensionStoreAuth, + expected: EXTENSION_FLAGS, }, + { scenario: "CLI store", auth: cliStoreAuth, expected: CLI_FLAGS }, { - scenario: "shared store with keyring on", - auth: { ...sharedAuth, useKeyring: true }, - expected: [...SHARED_FLAGS, "--use-keyring=true"], + scenario: "extension store with keyring off", + auth: { ...extensionStoreAuth, useKeyring: false }, + expected: [...EXTENSION_FLAGS, "--use-keyring=false"], + }, + { + 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(); @@ -67,10 +71,10 @@ describe("cliConfig", () => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", ["--verbose", "--global-configs"]); - expect(getGlobalShellFlags(config, privateAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ "--verbose", "--global-configs", // similar prefixes are not managed flags - ...PRIVATE_FLAGS, + ...EXTENSION_FLAGS, ]); }); @@ -78,9 +82,9 @@ describe("cliConfig", () => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", ["--verbose", "--use-keyring=false"]); - expect(getGlobalShellFlags(config, privateAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ "--verbose", - ...PRIVATE_FLAGS, + ...EXTENSION_FLAGS, ]); }); @@ -93,14 +97,14 @@ describe("cliConfig", () => { ]; it.each(userGlobalConfigCases)( - "passes user --global-config through in a shared store ($scenario)", + "passes user --global-config through in the CLI store ($scenario)", ({ flags }) => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", flags); - expect(getGlobalShellFlags(config, sharedAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ ...flags, - ...SHARED_FLAGS, + ...CLI_FLAGS, ]); }, ); @@ -112,14 +116,14 @@ describe("cliConfig", () => { flags: ["-v", `--global-config ${USER_DIR}`], }, ])( - "strips user --global-config in a private store ($scenario)", + "strips user --global-config in the extension store ($scenario)", ({ flags }) => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", flags); - expect(getGlobalShellFlags(config, privateAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ "-v", - ...PRIVATE_FLAGS, + ...EXTENSION_FLAGS, ]); }, ); @@ -130,10 +134,10 @@ describe("cliConfig", () => { config.set("coder.headerCommand", headerCommand); config.set("coder.globalFlags", ["-v", "--header-command custom"]); - expect(getGlobalShellFlags(config, sharedAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ "-v", '"--header-command custom"', // ignored by CLI - ...SHARED_FLAGS, + ...CLI_FLAGS, "--header-command", quoteCommand(headerCommand), ]); @@ -145,9 +149,9 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--cfg=${userHome}/coder"]); // Without per-entry escaping the space splits the shell command. - expect(getGlobalShellFlags(config, privateAuth)).toStrictEqual([ + expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ '"--cfg=C:\\Users\\John Doe/coder"', - ...PRIVATE_FLAGS, + ...EXTENSION_FLAGS, ]); }); }); @@ -158,9 +162,9 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose"]); config.set("coder.headerCommand", "echo test"); - expect(getGlobalFlags(config, privateAuth)).toStrictEqual([ + expect(getGlobalFlags(config, extensionStoreAuth)).toStrictEqual([ "--verbose", - ...PRIVATE_FLAGS, + ...EXTENSION_FLAGS, "--header-command", "echo test", ]); @@ -306,18 +310,18 @@ describe("cliConfig", () => { it.each([ { - scenario: "shares the CLI store when keyring is enabled on 2.29+", + scenario: "uses the CLI store when keyring is enabled on 2.29+", platform: "darwin", override: "none", version: "2.29.0", - expected: ["--verbose", ...SHARED_FLAGS, "--use-keyring=true"], + expected: ["--verbose", ...CLI_FLAGS, "--use-keyring=true"], }, { scenario: "uses the extension directory when keyring is unsupported", platform: "linux", override: "none", version: "2.29.0", - expected: ["--verbose", ...PRIVATE_FLAGS, "--use-keyring=false"], + expected: ["--verbose", ...EXTENSION_FLAGS, "--use-keyring=false"], }, { scenario: @@ -325,7 +329,7 @@ describe("cliConfig", () => { platform: "darwin", override: "none", version: "2.28.0", - expected: ["--verbose", ...PRIVATE_FLAGS], + expected: ["--verbose", ...EXTENSION_FLAGS], }, { scenario: "honors a globalFlags --global-config on 2.32+", @@ -335,7 +339,7 @@ describe("cliConfig", () => { expected: [ "--verbose", `--global-config=${USER_DIR}`, - ...SHARED_FLAGS, + ...CLI_FLAGS, "--use-keyring=true", ], }, @@ -344,7 +348,7 @@ describe("cliConfig", () => { platform: "darwin", override: "env", version: "2.32.0", - expected: ["--verbose", ...SHARED_FLAGS, "--use-keyring=true"], + expected: ["--verbose", ...CLI_FLAGS, "--use-keyring=true"], }, { scenario: "honors a globalFlags --global-config with keyring disabled", @@ -354,7 +358,7 @@ describe("cliConfig", () => { expected: [ "--verbose", `--global-config=${USER_DIR}`, - ...SHARED_FLAGS, + ...CLI_FLAGS, "--use-keyring=false", ], }, @@ -364,7 +368,7 @@ describe("cliConfig", () => { platform: "linux", override: "flag", version: "2.31.0", - expected: ["--verbose", ...PRIVATE_FLAGS, "--use-keyring=false"], + expected: ["--verbose", ...EXTENSION_FLAGS, "--use-keyring=false"], }, { scenario: @@ -372,7 +376,7 @@ describe("cliConfig", () => { platform: "linux", override: "env", version: "2.31.0", - expected: ["--verbose", ...PRIVATE_FLAGS, "--use-keyring=false"], + expected: ["--verbose", ...EXTENSION_FLAGS, "--use-keyring=false"], }, ])("$scenario", ({ platform, override, version, expected }) => { vi.mocked(os.platform).mockReturnValue(platform); diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index ad736feb38..ffb0d1c892 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -51,7 +51,6 @@ interface SetupOptions { const TEST_SESSION: SessionAuth = { url: TEST_URL, token: "test-token", - tokenSource: "extension", }; function setup(options: SetupOptions = {}) { @@ -71,7 +70,6 @@ function setup(options: SetupOptions = {}) { method: "stored_token", user: createMockUser(), token: "test-token", - tokenSource: "extension", } satisfies LoginResultForTest); const loginCoordinator: Pick = { ensureLoggedIn: vi.fn(() => Promise.resolve(loginResult)), @@ -90,10 +88,11 @@ 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< @@ -174,7 +173,6 @@ describe("Commands", () => { method: "provided_token", user: createMockUser(), token: "test-token", - tokenSource: "extension", }, }); @@ -267,15 +265,85 @@ describe("Commands", () => { expect(mocks.deploymentManager.clearDeployment).toHaveBeenCalledWith( "logout", ); - expect(mocks.cliManager.clearCredentials).toHaveBeenCalledWith( + expect(mocks.cliManager.holdsToken).toHaveBeenCalledWith( TEST_URL, - TEST_SESSION, + 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 a1d1d7a68f..9596085b11 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -19,8 +19,6 @@ import { import type * as nodeFs from "node:fs"; -import type { SessionAuth } from "@/core/secretsManager"; - vi.mock("node:child_process", () => ({ execFile: vi.fn() })); vi.mock("node:os"); @@ -43,7 +41,7 @@ const PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); const CRED_DIR = path.join("/mock/base", "dev.coder.com"); const USER_DIR = "/custom/coderv2"; -const PRIVATE_FLAGS = [ +const EXTENSION_FLAGS = [ "--global-config", CRED_DIR, "--url", @@ -58,13 +56,6 @@ const USER_DIR_FLAGS = [ "--use-keyring=false", ]; -const EXTENSION_SESSION: SessionAuth = { - url: TEST_URL, - token: "my-token", - tokenSource: "extension", -}; -const CLI_SESSION: SessionAuth = { ...EXTENSION_SESSION, tokenSource: "cli" }; - type ExecResult = string | Error; type ExecCallback = (err: Error | null, result?: { stdout: string }) => void; interface ExecOptions { @@ -135,9 +126,6 @@ const credentialFilesExist = () => memfs.existsSync(`${CRED_DIR}/url`) || memfs.existsSync(`${CRED_DIR}/session`); -const missingBinary = (): BinaryResolver => - vi.fn().mockRejectedValue(new Error("no binary")); - function setup(resolver: BinaryResolver = vi.fn().mockResolvedValue(TEST_BIN)) { const sink = new TestSink(); const manager = new CliCredentialManager( @@ -178,22 +166,22 @@ describe("CliCredentialManager", () => { scenario: "extension directory when keyring is unsupported", platform: "linux", configs, - expected: PRIVATE_FLAGS, - store: "private", + expected: EXTENSION_FLAGS, + store: "extension", }, { scenario: "CLI default store when keyring is enabled", platform: "darwin", configs, expected: KEYRING_FLAGS, - store: "shared", + store: "cli", }, { scenario: "user --global-config directory", platform: "linux", configs: userDirConfigs, expected: USER_DIR_FLAGS, - store: "shared", + store: "cli", }, ])( "targets the $scenario", @@ -224,17 +212,33 @@ describe("CliCredentialManager", () => { expect(execCalls()[0]).not.toContain("my-secret-token"); }); - it("throws a CredentialCliError when the CLI fails", async () => { - stubExecFile({ login: new Error("login failed") }); - const { manager, sink } = setup(); + it.each([ + { + scenario: "the CLI's stderr", + error: Object.assign(new Error("Command failed"), { + stderr: "keychain is locked\n", + }), + 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("Credential CLI operation failed"); - expect(sink.expectOne("auth.credential.store")).toMatchObject({ - properties: { "error.type": "cli", result: "error" }, - }); - }); + 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", () => { @@ -257,17 +261,6 @@ describe("CliCredentialManager", () => { expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); }); - it("refuses the keyring for a non-HTTPS URL without running the CLI", async () => { - vi.mocked(os.platform).mockReturnValue("darwin"); - stubExecFile({ token: "my-token" }); - const { manager } = setup(); - - expect( - await manager.readToken("http://dev.coder.com", configs), - ).toBeUndefined(); - expect(execFile).not.toHaveBeenCalled(); - }); - it("returns undefined below CLI 2.32 without running the CLI", async () => { vi.mocked(cliExec.version).mockResolvedValue("2.31.0"); const { manager } = setup(); @@ -277,131 +270,117 @@ describe("CliCredentialManager", () => { }); }); - describe("deleteToken", () => { - it.each([ - { scenario: "a CLI token", session: CLI_SESSION }, - { scenario: "no session", session: undefined }, + describe("holdsToken", () => { + interface Case { + scenario: string; + platform: NodeJS.Platform; + version?: string; + cliToken?: string; + expected: boolean; + } + + 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, + }, ])( - "logs out of the extension directory even for $scenario", - async ({ session }) => { - stubExecFile(); - writeCredentialFiles(); - const { manager, sink } = setup(); + "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); + }, + ); + }); - const result = await manager.deleteToken(TEST_URL, configs, session); + describe("deleteToken", () => { + interface Case { + scenario: string; + platform: NodeJS.Platform; + signOutCli: boolean; + logout: string[] | undefined; + outcome: string; + } - expect(result).toBe(true); - expect(execCalls()).toEqual([[...PRIVATE_FLAGS, "logout", "--yes"]]); - expect(credentialFilesExist()).toBe(false); - expect(sink.expectOne("auth.credential.clear")).toMatchObject({ - properties: { store: "private", result: "success" }, - }); + 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(); + + const result = await manager.deleteToken(TEST_URL, configs, { + signOutCli, + }); + + 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("reports a failed logout without throwing", async () => { stubExecFile({ logout: new Error("logout failed") }); const { manager, sink } = setup(); await expect( - manager.deleteToken(TEST_URL, configs, EXTENSION_SESSION), + manager.deleteToken(TEST_URL, configs, { signOutCli: true }), ).resolves.toBe(false); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { "error.type": "cli", result: "error" }, }); }); - - describe("in a store shared with the CLI", () => { - beforeEach(() => { - vi.mocked(os.platform).mockReturnValue("darwin"); - }); - - it("logs out when the CLI holds the extension's token", async () => { - stubExecFile({ token: "my-token\n" }); - writeCredentialFiles(); - const { manager, sink } = setup(); - - const result = await manager.deleteToken( - TEST_URL, - configs, - EXTENSION_SESSION, - ); - - expect(result).toBe(true); - expect(execCalls()).toEqual([ - [...KEYRING_FLAGS, "login", "token"], - [...KEYRING_FLAGS, "logout", "--yes"], - ]); - expect(credentialFilesExist()).toBe(false); - expect(sink.expectOne("auth.credential.clear")).toMatchObject({ - properties: { store: "shared", result: "success" }, - }); - }); - - interface Case { - scenario: string; - session?: SessionAuth; - token?: ExecResult; - } - - it.each([ - { - scenario: "the CLI holds another token", - session: EXTENSION_SESSION, - token: "someone-elses-token", - }, - { - scenario: "the CLI token cannot be read", - session: EXTENSION_SESSION, - token: new Error("keychain locked"), - }, - { scenario: "the token came from the CLI", session: CLI_SESSION }, - { scenario: "there is no session", session: undefined }, - ])("keeps the CLI session when $scenario", async ({ session, token }) => { - stubExecFile({ token }); - writeCredentialFiles(); - const { manager } = setup(); - - const result = await manager.deleteToken(TEST_URL, configs, session); - - expect(result).toBe(true); - expect(execCalls().some((args) => args.includes("logout"))).toBe(false); - expect(credentialFilesExist()).toBe(false); - }); - - it("logs out without verifying below CLI 2.32", async () => { - vi.mocked(cliExec.version).mockResolvedValue("2.31.0"); - stubExecFile(); - const { manager } = setup(); - - const result = await manager.deleteToken( - TEST_URL, - configs, - EXTENSION_SESSION, - ); - - expect(result).toBe(true); - expect(execCalls()).toEqual([[...KEYRING_FLAGS, "logout", "--yes"]]); - }); - - it("treats a user --global-config directory as shared", async () => { - vi.mocked(os.platform).mockReturnValue("linux"); - stubExecFile({ token: "my-token" }); - const { manager } = setup(); - - const result = await manager.deleteToken( - TEST_URL, - userDirConfigs, - EXTENSION_SESSION, - ); - - expect(result).toBe(true); - expect(execCalls()).toEqual([ - [...USER_DIR_FLAGS, "login", "token"], - [...USER_DIR_FLAGS, "logout", "--yes"], - ]); - }); - }); }); describe("every CLI call", () => { @@ -413,24 +392,29 @@ describe("CliCredentialManager", () => { name: string; run: Run; event?: string; - onMissingBinary: (result: Promise) => Promise; + whenMissing: (result: Promise) => Promise; + whenBroken: (result: Promise) => Promise; }> = [ { name: "storeToken", run: (m, o) => m.storeToken(TEST_URL, "token", configs, o), event: "auth.credential.store", - onMissingBinary: (r) => expect(r).rejects.toThrow("no binary"), + whenMissing: (r) => expect(r).resolves.toBeUndefined(), + whenBroken: (r) => expect(r).rejects.toThrow("broken"), }, { name: "readToken", run: (m, o) => m.readToken(TEST_URL, configs, o), - onMissingBinary: (r) => expect(r).resolves.toBeUndefined(), + whenMissing: (r) => expect(r).resolves.toBeUndefined(), + whenBroken: (r) => expect(r).resolves.toBeUndefined(), }, { name: "deleteToken", - run: (m, o) => m.deleteToken(TEST_URL, configs, EXTENSION_SESSION, o), + run: (m, o) => + m.deleteToken(TEST_URL, configs, { ...o, signOutCli: true }), event: "auth.credential.clear", - onMissingBinary: (r) => expect(r).resolves.toBe(false), + whenMissing: (r) => expect(r).resolves.toBe(true), + whenBroken: (r) => expect(r).resolves.toBe(false), }, ]; @@ -468,11 +452,32 @@ describe("CliCredentialManager", () => { ); it.each(operations)( - "$name handles a missing binary without running the CLI", - async ({ run, event, onMissingBinary }) => { - const { manager, sink } = setup(missingBinary()); + "$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 onMissingBinary( + await whenBroken( run(manager, { signal: new AbortController().signal }), ); diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index fbaf96b267..4579057b3b 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -36,12 +36,12 @@ const cliExec = await import("@/core/cliExec"); const { spawn } = await import("node:child_process"); const sharedAuth = (url: string): CliEnv["auth"] => ({ - store: "shared", + store: "cli", url, useKeyring: undefined, }); const privateAuth = (url: string, configDir: string): CliEnv["auth"] => ({ - store: "private", + store: "extension", url, configDir, useKeyring: undefined, diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 0e73feb5cb..961913f058 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -141,11 +141,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 +181,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 () => { @@ -309,41 +305,44 @@ describe("CliManager", () => { describe("Clear Credentials", () => { const CLEAR_URL = "https://dev.coder.com"; - const SESSION = { - url: CLEAR_URL, - token: "test-token", - tokenSource: "extension", - } as const; - - it("should skip progress notification when keyring is disabled", async () => { - const { manager, mockCredManager } = setupCliManager(); - - await manager.clearCredentials(CLEAR_URL, SESSION); - expect(vscode.window.withProgress).not.toHaveBeenCalled(); - expect(mockCredManager.deleteToken).toHaveBeenCalledWith( - CLEAR_URL, - expect.anything(), - SESSION, - { signal: expect.any(AbortSignal) }, - ); - }); - - it("should show progress notification when keyring is enabled", async () => { - const { manager } = setupCliManager(); - vi.mocked(isKeyringEnabled).mockReturnValue(true); + it.each([ + { + scenario: "keyring disabled", + keyring: false, + signOutCli: true, + progress: false, + }, + { + scenario: "CLI session kept", + keyring: true, + signOutCli: false, + progress: false, + }, + { + scenario: "keyring sign-out", + keyring: true, + signOutCli: true, + progress: true, + }, + ])( + "$scenario: progress notification shown is $progress", + async ({ keyring, signOutCli, progress }) => { + const { manager, mockCredManager } = setupCliManager(); + vi.mocked(isKeyringEnabled).mockReturnValue(keyring); - await manager.clearCredentials(CLEAR_URL, SESSION); + 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( + Number(progress), + ); + expect(mockCredManager.deleteToken).toHaveBeenCalledWith( + CLEAR_URL, + expect.anything(), + { signal: expect.any(AbortSignal), signOutCli }, + ); + }, + ); it.each([ { @@ -373,7 +372,7 @@ describe("CliManager", () => { ); } await expect( - manager.clearCredentials(CLEAR_URL, SESSION), + manager.clearCredentials(CLEAR_URL, { signOutCli: true }), ).resolves.toEqual(expected); }, ); diff --git a/test/unit/core/secretsManager.test.ts b/test/unit/core/secretsManager.test.ts index 5b7fd22312..482d71e0a9 100644 --- a/test/unit/core/secretsManager.test.ts +++ b/test/unit/core/secretsManager.test.ts @@ -34,7 +34,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", - tokenSource: "extension", }); const auth = await secretsManager.getSessionAuth("example.com"); expect(auth?.token).toBe("test-token"); @@ -43,7 +42,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "new-token", - tokenSource: "extension", }); const newAuth = await secretsManager.getSessionAuth("example.com"); expect(newAuth?.token).toBe("new-token"); @@ -53,13 +51,11 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com:8443", token: "test-token", - tokenSource: "extension", }); expect(await secretsManager.getSessionAuth("example.com")).toEqual({ url: "https://example.com:8443", token: "test-token", - tokenSource: "extension", }); }); @@ -92,7 +88,6 @@ describe("SecretsManager", () => { const existingAuth = { url: "https://example.com", token: "existing-token", - tokenSource: "extension" as const, }; await secretsManager.setSessionAuth("example.com", existingAuth); @@ -100,7 +95,6 @@ describe("SecretsManager", () => { secretsManager.setSessionAuth("example.com", { url, token: "secret-token", - tokenSource: "extension", }), ).rejects.toThrow(error); @@ -135,7 +129,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", - tokenSource: "extension", }); await secretsManager.clearAllAuthData("example.com"); expect( @@ -164,7 +157,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", - tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toContain( "example.com", @@ -173,7 +165,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("other.com", { url: "https://other.com", token: "other-token", - tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toContain( "example.com", @@ -187,7 +178,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", - tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toContain( "example.com", @@ -203,7 +193,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", token: "test-token", - tokenSource: "extension", }); secretStorage.corruptStorage(); @@ -216,19 +205,16 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("first.com", { url: "https://first.com", token: "token1", - tokenSource: "extension", }); vi.advanceTimersByTime(10); await secretsManager.setSessionAuth("second.com", { url: "https://second.com", token: "token2", - tokenSource: "extension", }); vi.advanceTimersByTime(10); await secretsManager.setSessionAuth("first.com", { url: "https://first.com", token: "token1-updated", - tokenSource: "extension", }); expect(await secretsManager.getKnownSafeHostnames()).toEqual([ @@ -247,7 +233,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth(`host${i}.com`, { url: `https://host${i}.com`, token: `token${i}`, - tokenSource: "extension", }); vi.advanceTimersByTime(10); } @@ -367,7 +352,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("existing.coder.com", { url: "https://existing.coder.com", token: "existing-token", - tokenSource: "extension", }); // Set up legacy storage with same hostname @@ -403,7 +387,6 @@ describe("SecretsManager", () => { await secretsManager.setSessionAuth("mtls.coder.com", { url: "https://mtls.coder.com", token: "", - tokenSource: "extension", }); const auth = await secretsManager.getSessionAuth("mtls.coder.com"); @@ -418,7 +401,6 @@ describe("SecretsManager", () => { const authWithExtra = { url: "https://coder.example.com", token: "test-token", - tokenSource: "extension" as const, extraField: "should be stripped", }; @@ -428,7 +410,6 @@ describe("SecretsManager", () => { expect(JSON.parse(raw!)).toEqual({ url: "https://coder.example.com", token: "test-token", - tokenSource: "extension", }); }); @@ -436,7 +417,6 @@ describe("SecretsManager", () => { const authWithExtra = { url: "https://coder.example.com", token: "test-token", - tokenSource: "extension" as const, oauth: { scope: "workspace:read", expiry_timestamp: 12345, @@ -450,7 +430,6 @@ describe("SecretsManager", () => { expect(JSON.parse(raw!)).toEqual({ url: "https://coder.example.com", token: "test-token", - tokenSource: "extension", oauth: { scope: "workspace:read", expiry_timestamp: 12345 }, }); }); @@ -523,12 +502,11 @@ describe("SecretsManager", () => { const sessionAuthCases: BackwardsCompatTestCase[] = [ { - name: "without optional fields, defaulting tokenSource", + name: "without optional fields", data: { url: "https://coder.example.com", token: "test-token" }, expected: { url: "https://coder.example.com", token: "test-token", - tokenSource: "extension", }, }, { @@ -542,20 +520,6 @@ describe("SecretsManager", () => { url: "https://coder.example.com", token: "test-token", oauth: { scope: "workspace:read", expiry_timestamp: 12345 }, - tokenSource: "extension", - }, - }, - { - name: "with a CLI token source", - data: { - url: "https://coder.example.com", - token: "test-token", - tokenSource: "cli", - }, - expected: { - url: "https://coder.example.com", - token: "test-token", - tokenSource: "cli", }, }, ]; diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 7985355152..1a45bfa596 100644 --- a/test/unit/deployment/deploymentManager.test.ts +++ b/test/unit/deployment/deploymentManager.test.ts @@ -325,7 +325,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "stored-token", - tokenSource: "extension", }); const result = await manager.verifyAndApplySession({ @@ -418,7 +417,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "synced-token", - tokenSource: "extension", }); // Simulate cross-window change @@ -449,7 +447,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "", - tokenSource: "extension", }); await secretsManager.setCurrentDeployment({ @@ -490,7 +487,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "refreshed-token", - tokenSource: "extension", }); await flush(); @@ -518,7 +514,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "refreshed-token", - tokenSource: "extension", }); await flush(); await manager.clearDeployment("logout"); @@ -548,7 +543,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "rotated-token", - tokenSource: "extension", }); await flush(); @@ -577,7 +571,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "rotated-token", - tokenSource: "extension", }); await flush(); @@ -605,7 +598,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "rotated-token", - tokenSource: "extension", }); await flush(); @@ -726,7 +718,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "", - tokenSource: "extension", }); await manager.setDeployment({ url: TEST_URL, @@ -820,7 +811,6 @@ describe("DeploymentManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "recovered-token", - tokenSource: "extension", }); await flush(); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 6096e381e1..eb1870a25b 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -3,7 +3,7 @@ import { describe, expect, it, vi, type Mock } from "vitest"; import * as vscode from "vscode"; import { MementoManager } from "@/core/mementoManager"; -import { SecretsManager, type TokenSource } from "@/core/secretsManager"; +import { SecretsManager } from "@/core/secretsManager"; import { getHeaders } from "@/headers"; import { AuthTelemetry } from "@/instrumentation/auth"; import { LoginCoordinator, type LoginMethod } from "@/login/loginCoordinator"; @@ -202,7 +202,6 @@ function createSignInTestContext( storeSession: (auth: { token: string; username?: string; url?: string }) => ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, - tokenSource: "extension", ...auth, }), confirmSignIn: () => ctx.userInteraction.setResponse(prompt, "Sign In"), @@ -225,38 +224,27 @@ function createSignInTestContext( describe("LoginCoordinator", () => { describe("token authentication", () => { - interface Case { - tokenSource: TokenSource; - } - - it.each([{ tokenSource: "extension" }, { tokenSource: "cli" }])( - "authenticates with a stored token and keeps its $tokenSource source", - async ({ tokenSource }) => { - const { secretsManager, coordinator, mockSuccessfulAuth } = - createTestContext(); - const user = mockSuccessfulAuth(); - await secretsManager.setSessionAuth(TEST_HOSTNAME, { - url: TEST_URL, - token: "stored-token", - tokenSource, - }); + it("authenticates with a stored token", async () => { + const { secretsManager, coordinator, mockSuccessfulAuth } = + createTestContext(); + const user = mockSuccessfulAuth(); + await secretsManager.setSessionAuth(TEST_HOSTNAME, { + url: TEST_URL, + token: "stored-token", + }); - const result = await coordinator.ensureLoggedIn({ - url: TEST_URL, - safeHostname: TEST_HOSTNAME, - }); + const result = await coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }); - expect(result).toEqual({ - success: true, - method: "stored_token", - user, - token: "stored-token", - tokenSource, - }); - const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); - expect(auth?.tokenSource).toBe(tokenSource); - }, - ); + expect(result).toEqual({ + success: true, + method: "stored_token", + user, + token: "stored-token", + }); + }); it("authenticates with CLI credential token on success", async () => { const { @@ -280,13 +268,11 @@ describe("LoginCoordinator", () => { method: "cli_token", user, token: "cli-credential-token", - tokenSource: "cli", }); expect(vscode.window.showInputBox).not.toHaveBeenCalled(); const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); expect(auth?.token).toBe("cli-credential-token"); - expect(auth?.tokenSource).toBe("cli"); }); it("prompts for token when no stored auth exists", async () => { @@ -312,13 +298,11 @@ describe("LoginCoordinator", () => { method: "cli_token", user, token: "new-token", - tokenSource: "extension", }); // Verify new token was persisted const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); expect(auth?.token).toBe("new-token"); - expect(auth?.tokenSource).toBe("extension"); }); it("returns success false when user cancels input", async () => { @@ -395,7 +379,6 @@ describe("LoginCoordinator", () => { method: "mtls", user, token: "", - tokenSource: "extension", }); // Verify empty string token was persisted @@ -477,7 +460,6 @@ describe("LoginCoordinator", () => { method, user, token, - tokenSource: "extension", }); function createLinkTestContext() { @@ -720,7 +702,6 @@ describe("LoginCoordinator", () => { await ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "stored-token", - tokenSource: "extension", }); const login = async () => { const result = await ctx.coordinator.ensureLoggedIn({ @@ -780,7 +761,6 @@ describe("LoginCoordinator", () => { method: "stored_token", user, token: "stored-token", - tokenSource: "extension", }); await vi.waitFor(() => expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( @@ -792,12 +772,12 @@ describe("LoginCoordinator", () => { }); describe("CLI session confirmation", () => { - const CLI_PROMPT = "Sign in with the Coder CLI session?"; + const CLI_PROMPT = "Sign in with the Coder CLI's session?"; function createCliTestContext() { const ctx = createSignInTestContext( CLI_PROMPT, - (username) => `The Coder CLI session signs you in as "${username}"`, + (username) => `The Coder CLI's session signs you in as "${username}"`, ); return { ...ctx, @@ -823,7 +803,6 @@ describe("LoginCoordinator", () => { expect(await t.login()).toMatchObject({ method: "cli_token", user, - tokenSource: "cli", }); t.expectNoPrompt(); }); @@ -847,7 +826,6 @@ describe("LoginCoordinator", () => { expect(await t.login()).toMatchObject({ token: "cli-token", - tokenSource: "cli", }); t.expectSignInPrompt("cli-user", 'expired session for "old-user"'); }); @@ -866,7 +844,6 @@ describe("LoginCoordinator", () => { expect(await t.login()).toMatchObject({ token: "new-token", - tokenSource: "extension", }); expect(await t.storedToken()).toBe("new-token"); }); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index c170817531..b02927fc4a 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -90,7 +90,6 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { await base.secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: overrides.token ?? "access-token", - tokenSource: "extension", username: overrides.username, oauth: { refresh_token: overrides.refreshToken ?? "refresh-token", @@ -150,7 +149,6 @@ describe("OAuthSessionManager", () => { auth: { url: TEST_URL, token: "access-token", - tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -169,7 +167,6 @@ describe("OAuthSessionManager", () => { auth: { url: TEST_URL, token: "session-token", - tokenSource: "extension", }, expected: false, }, @@ -260,7 +257,6 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: `${TEST_URL}:8443`, token: "access-token", - tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -515,7 +511,6 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "access-token", - tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, @@ -533,7 +528,6 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "access-token", - tokenSource: "extension", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, diff --git a/test/unit/remote/migration.test.ts b/test/unit/remote/migration.test.ts index a03f5a4e1a..f663e77b02 100644 --- a/test/unit/remote/migration.test.ts +++ b/test/unit/remote/migration.test.ts @@ -56,7 +56,6 @@ describe("Session auth migration", () => { expect(secretsManager.setSessionAuth).toHaveBeenCalledWith(HOSTNAME, { url: "https://dep.example.com", token: "legacy-token", - tokenSource: "extension", }); expect(vol.existsSync(URL_PATH)).toBe(false); expect(vol.existsSync(TOKEN_PATH)).toBe(false); @@ -80,7 +79,6 @@ describe("Session auth migration", () => { existingAuth: { url: "https://dep.example.com", token: "current", - tokenSource: "extension", }, }); writeLegacyFiles(); diff --git a/test/unit/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index 78c1494e13..22481d7939 100644 --- a/test/unit/remote/workspaceStateMachine.test.ts +++ b/test/unit/remote/workspaceStateMachine.test.ts @@ -109,7 +109,7 @@ function setup( startupMode, "/usr/bin/coder", {} as FeatureSet, - { store: "shared", url: "https://test.coder.com", useKeyring: undefined }, + { store: "cli", url: "https://test.coder.com", useKeyring: undefined }, createMockServiceContainer({ telemetry, logger: createMockLogger() }), ); return { sm, progress, userInteraction }; diff --git a/test/unit/uri/uriHandler.test.ts b/test/unit/uri/uriHandler.test.ts index 273d92427c..1d8bbce101 100644 --- a/test/unit/uri/uriHandler.test.ts +++ b/test/unit/uri/uriHandler.test.ts @@ -74,7 +74,6 @@ function createMockLoginCoordinator(secretsManager: SecretsManager) { await secretsManager.setSessionAuth(options.safeHostname, { url: options.url, token, - tokenSource: "extension", }); return { success: true, @@ -160,7 +159,6 @@ function createTestContext() { secretsManager.setSessionAuth(TEST_HOSTNAME, { url: TEST_URL, token: "known-token", - tokenSource: "extension", ...auth, }), @@ -533,7 +531,6 @@ describe("uriHandler", () => { expect(await t.secretsManager.getSessionAuth(TEST_HOSTNAME)).toEqual({ url: TEST_URL, token: "tok", - tokenSource: "extension", }); }); 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 { From 91c65a2d2762ea8e14cf996dcea4af93ca9aaff3 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 14 Sep 2026 17:52:46 +0200 Subject: [PATCH 4/5] fix: pass --allow-redirects to Coder CLI 2.38 and later coder/coder#29104 makes the CLI return an error on an HTTP redirect instead of following it. The extension follows redirects itself, so a deployment URL that redirects kept working for the API but made `coder login` fail at connect and `coder logout` fail at logout. The flag is global, so `coder ssh` gets it too. Older CLIs followed redirects by default and do not know the flag, so it is gated on the version. --- CHANGELOG.md | 6 +++++ src/featureSet.ts | 3 +++ src/settings/cli.ts | 24 +++++++++++-------- test/unit/api/workspace.test.ts | 2 ++ test/unit/cliConfig.test.ts | 14 +++++++++++ test/unit/core/cliExec.test.ts | 2 ++ test/unit/featureSet.test.ts | 7 ++++++ .../unit/remote/workspaceStateMachine.test.ts | 7 +++++- 8 files changed, 54 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e93d630fd8..2a5f4e06bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,12 @@ 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/src/featureSet.ts b/src/featureSet.ts index 1ac9b17e6e..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; } /** @@ -60,5 +61,7 @@ export function featureSetForVersion( 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/settings/cli.ts b/src/settings/cli.ts index 3677475d5b..26bd11ec57 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -9,14 +9,14 @@ import type { WorkspaceConfiguration } from "vscode"; import type { FeatureSet } from "../featureSet"; /** 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 = - | { store: "cli"; url: string; useKeyring: boolean | undefined } - | { - store: "extension"; - url: string; - configDir: string; - useKeyring: false | undefined; - }; +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 @@ -70,6 +70,9 @@ function buildGlobalFlags( if (auth.useKeyring !== undefined) { flags.push(`--use-keyring=${auth.useKeyring}`); } + if (auth.allowRedirects) { + flags.push("--allow-redirects"); + } return [...flags, ...getHeaderArgs(configs, escHeader)]; } @@ -127,10 +130,11 @@ export function resolveCliAuth( : 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 { store: "cli", url, useKeyring }; + return { ...common, store: "cli", useKeyring }; } - return { store: "extension", url, configDir, useKeyring }; + return { ...common, store: "extension", configDir, useKeyring }; } function hasUserConfigDir( diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 2e563198f1..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 { @@ -98,6 +99,7 @@ function createUpdateCtx( store: "cli" as const, url: "https://test.coder.com", useKeyring: undefined, + allowRedirects: false, }, binPath: "/usr/bin/coder", workspace, diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 45cd385387..b83000396e 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -27,11 +27,13 @@ const extensionStoreAuth: CliAuth = { 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]; @@ -316,6 +318,18 @@ describe("cliConfig", () => { 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", diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index 4579057b3b..a25316baec 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -39,12 +39,14 @@ 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", () => { diff --git a/test/unit/featureSet.test.ts b/test/unit/featureSet.test.ts index 702a78e3ed..0a97b3bf77 100644 --- a/test/unit/featureSet.test.ts +++ b/test/unit/featureSet.test.ts @@ -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/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index 22481d7939..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, - { store: "cli", url: "https://test.coder.com", useKeyring: undefined }, + { + store: "cli", + url: "https://test.coder.com", + useKeyring: undefined, + allowRedirects: false, + }, createMockServiceContainer({ telemetry, logger: createMockLogger() }), ); return { sm, progress, userInteraction }; From b880ac93fd8ed1ee7fd07f7b1862a174e56e530b Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 14 Sep 2026 18:45:30 +0200 Subject: [PATCH 5/5] perf: cache the CLI version and skip CLI checks the store rules out `coder version` is cached per binary while the file's mtime and size match, so the credential operations, the binary check on connect, and the CLI command flow stop spawning it for the same file. Reads and token checks run only when the CLI is downloaded and settings allow its own store, sparing Linux two process spawns per login and logout. The token check before the logout prompt runs under the same cancellable progress as the login read, so a locked keychain no longer freezes Logout until the exec timeout, and the progress only appears when a CLI call is made. Remove All logs hosts out one at a time, since `coder logout` rewrites the whole keyring entry. --- src/commands.ts | 27 ++++---- src/core/cliCredentialManager.ts | 24 ++++++- src/core/cliExec.ts | 19 +++++- src/core/cliManager.ts | 24 ++++--- src/login/loginCoordinator.ts | 11 ++-- src/settings/cli.ts | 7 ++ test/mocks/testHelpers.ts | 1 + test/unit/cliConfig.test.ts | 36 +++++++++-- test/unit/core/cliCredentialManager.test.ts | 68 ++++++++++++++++--- test/unit/core/cliExec.test.ts | 21 ++++++ test/unit/core/cliManager.test.ts | 72 ++++++++++++--------- test/unit/util/credentials.test.ts | 4 +- 12 files changed, 235 insertions(+), 79 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 9dee5decde..3ce889b52e 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -859,20 +859,19 @@ export class Commands { "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, { - signOutCli: await this.cliManager.holdsToken( - auth.url, - auth.token, - ), - }); - } - 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(", "), diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 92866db607..bfcdb0d13a 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -11,7 +11,12 @@ import { CredentialTelemetry, } from "../instrumentation/credentials"; import { recordError } from "../instrumentation/outcomes"; -import { type CliAuth, getGlobalFlags, resolveCliAuth } from "../settings/cli"; +import { + type CliAuth, + getGlobalFlags, + mayUseCliStore, + resolveCliAuth, +} from "../settings/cli"; import { type TelemetryReporter } from "../telemetry/reporter"; import { toSafeHost } from "../util/uri"; @@ -89,7 +94,6 @@ export class CliCredentialManager { try { const cli = await this.resolveCli(url, configs); if (!cli) { - this.logger.debug("No CLI session to read: the CLI is not downloaded"); return undefined; } if (!cli.featureSet.tokenRead) { @@ -108,6 +112,16 @@ export class CliCredentialManager { } } + /** 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 + ); + } + /** * 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. @@ -116,13 +130,17 @@ export class CliCredentialManager { url: string, token: string, configs: Pick, + options?: { signal?: AbortSignal }, ): Promise { try { const cli = await this.resolveCli(url, configs); if (cli?.auth.store !== "cli") { return false; } - return !cli.featureSet.tokenRead || (await this.cliToken(cli)) === token; + 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; 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 91470d561f..103448c42e 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -22,7 +22,6 @@ 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"; @@ -1061,13 +1060,22 @@ export class CliManager { this.handleStoreError(result.error, configs); } - /** True when the CLI's own store holds this token. */ - public holdsToken(url: string, token: string): Promise { - return this.cliCredentialManager.holdsToken( - url, - token, - vscode.workspace.getConfiguration(), + /** 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; } /** @@ -1086,7 +1094,7 @@ export class CliManager { signOutCli, }), { - enabled: signOutCli && isKeyringEnabled(configs), + enabled: signOutCli, location: vscode.ProgressLocation.Notification, title: `Removing credentials for ${url}`, cancellable: true, diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 92e1fdc1f5..e8057093de 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -7,9 +7,8 @@ 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"; @@ -424,13 +423,17 @@ export class LoginCoordinator implements vscode.Disposable { ): Promise { 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 credentials from the Coder CLI", cancellable: true, diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 26bd11ec57..36f867eb8b 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -117,6 +117,13 @@ export function isKeyringEnabled( return isKeyringSupported() && configs.get("coder.useKeyring", true); } +/** 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, diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index e01a25ac5f..9200a59705 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -524,6 +524,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { 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/cliConfig.test.ts b/test/unit/cliConfig.test.ts index b83000396e..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"; @@ -263,13 +264,13 @@ describe("cliConfig", () => { }); describe("isKeyringEnabled", () => { - interface Case { + interface KeyringEnabledCase { platform: NodeJS.Platform; useKeyring?: boolean; expected: boolean; } - it.each([ + it.each([ { platform: "darwin", expected: true }, { platform: "win32", expected: true }, { platform: "linux", expected: false }, @@ -288,6 +289,33 @@ describe("cliConfig", () => { ); }); + describe("mayUseCliStore", () => { + interface MayUseCliStoreCase { + platform: NodeJS.Platform; + flags: string[]; + expected: boolean; + } + + 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(mayUseCliStore(config)).toBe(expected); + }, + ); + }); + describe("resolveCliAuth", () => { function resolve(config: MockConfigurationProvider, version: string) { const featureSet = featureSetForVersion(semver.parse(version)); @@ -302,7 +330,7 @@ describe("cliConfig", () => { vi.unstubAllEnvs(); }); - interface Case { + interface ResolveCliAuthCase { scenario: string; platform: NodeJS.Platform; override: "none" | "flag" | "env"; @@ -310,7 +338,7 @@ describe("cliConfig", () => { expected: string[]; } - it.each([ + it.each([ { scenario: "uses the CLI store when keyring is enabled on 2.29+", platform: "darwin", diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 9596085b11..a6c5934cde 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -153,7 +153,7 @@ describe("CliCredentialManager", () => { }); // Store selection is covered by cliConfig.test.ts; this checks the wiring. - interface Case { + interface StoreCase { scenario: string; platform: NodeJS.Platform; configs: typeof configs; @@ -161,7 +161,7 @@ describe("CliCredentialManager", () => { store: string; } - it.each([ + it.each([ { scenario: "extension directory when keyring is unsupported", platform: "linux", @@ -212,7 +212,13 @@ describe("CliCredentialManager", () => { expect(execCalls()[0]).not.toContain("my-secret-token"); }); - it.each([ + interface CliErrorCase { + scenario: string; + error: Error; + message: string; + } + + it.each([ { scenario: "the CLI's stderr", error: Object.assign(new Error("Command failed"), { @@ -251,7 +257,12 @@ describe("CliCredentialManager", () => { expect(execCalls()).toEqual([[...KEYRING_FLAGS, "login", "token"]]); }); - it.each([ + interface ReadTokenCase { + scenario: string; + token: ExecResult; + } + + it.each([ { scenario: "whitespace-only stdout", token: " \n" }, { scenario: "a CLI error", token: new Error("no token found") }, ])("returns undefined on $scenario", async ({ token }) => { @@ -271,7 +282,7 @@ describe("CliCredentialManager", () => { }); describe("holdsToken", () => { - interface Case { + interface HoldsTokenCase { scenario: string; platform: NodeJS.Platform; version?: string; @@ -279,7 +290,7 @@ describe("CliCredentialManager", () => { expected: boolean; } - it.each([ + it.each([ { scenario: "the extension store", platform: "linux", expected: false }, { scenario: "the CLI store holding the token", @@ -318,8 +329,44 @@ describe("CliCredentialManager", () => { ); }); + 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); + }, + ); + describe("deleteToken", () => { - interface Case { + interface DeleteTokenCase { scenario: string; platform: NodeJS.Platform; signOutCli: boolean; @@ -327,7 +374,7 @@ describe("CliCredentialManager", () => { outcome: string; } - it.each([ + it.each([ { scenario: "always logs out of the extension store", platform: "linux", @@ -388,13 +435,14 @@ describe("CliCredentialManager", () => { manager: CliCredentialManager, options: { signal: AbortSignal }, ) => Promise; - const operations: Array<{ + 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), diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index a25316baec..43e99cb16f 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -111,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, diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 961913f058..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 }; @@ -306,36 +299,22 @@ describe("CliManager", () => { describe("Clear Credentials", () => { const CLEAR_URL = "https://dev.coder.com"; - it.each([ - { - scenario: "keyring disabled", - keyring: false, - signOutCli: true, - progress: false, - }, - { - scenario: "CLI session kept", - keyring: true, - signOutCli: false, - progress: false, - }, - { - scenario: "keyring sign-out", - keyring: true, - signOutCli: true, - progress: true, - }, + interface ProgressCase { + signOutCli: boolean; + progress: number; + } + + it.each([ + { signOutCli: false, progress: 0 }, + { signOutCli: true, progress: 1 }, ])( - "$scenario: progress notification shown is $progress", - async ({ keyring, signOutCli, progress }) => { + "shows progress $progress time(s) when signOutCli is $signOutCli", + async ({ signOutCli, progress }) => { const { manager, mockCredManager } = setupCliManager(); - vi.mocked(isKeyringEnabled).mockReturnValue(keyring); await manager.clearCredentials(CLEAR_URL, { signOutCli }); - expect(vscode.window.withProgress).toHaveBeenCalledTimes( - Number(progress), - ); + expect(vscode.window.withProgress).toHaveBeenCalledTimes(progress); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( CLEAR_URL, expect.anything(), @@ -378,6 +357,35 @@ describe("CliManager", () => { ); }); + 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/util/credentials.test.ts b/test/unit/util/credentials.test.ts index 101aea4bc4..432721cbd7 100644 --- a/test/unit/util/credentials.test.ts +++ b/test/unit/util/credentials.test.ts @@ -17,12 +17,12 @@ describe("showStoreCredentialsError", () => { vi.clearAllMocks(); }); - interface Case { + interface StoreErrorCase { platform: NodeJS.Platform; message: string; } - it.each([ + it.each([ { platform: "darwin", message: