diff --git a/CHANGELOG.md b/CHANGELOG.md index 135c3fe27d..e0fda5ff8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,34 @@ from published versions since it shows up in the VS Code extension changelog tab and is confusing to users. Add it back between releases if needed. --> +## Unreleased + +### Changed + +- Store session tokens in the OS keyring by default on macOS and Windows. 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`. +- Read the `coder` CLI's session only on Coder CLI 2.32.0 or later, up from + 2.31.0, where the CLI checks the stored URL against the one you connect to. +- Ask before signing in with the `coder` CLI's session when it belongs to a + different user than your previous session. +- Show an error with **Open Settings** when the CLI cannot store the token at + login, and a **Show Output** button when logout cannot remove every + credential. + +### 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 ae19cefde0..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\nSet `--global-config` here to point the CLI at a shared config directory (e.g. `--global-config=~/.config/coderv2` to share login/auth with the Coder CLI); requires a deployment on 2.31.0+ and is ignored when `#coder.useKeyring#` is active. The `--use-keyring` flag is ignored; use `#coder.useKeyring#` instead.\n\nFor `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here.", + "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item, in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`. For `--flag=value` items the expansion applies to the value half, so `--cfg=~/coder` works.\n\nTo share a config directory with the `coder` CLI, add `--global-config` here (for example `--global-config=~/.config/coderv2`) or set `CODER_CONFIG_DIR`. Requires Coder CLI 2.32.0 or later. A `--use-keyring` item is ignored; use `#coder.useKeyring#` instead.\n\nFor `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here.", "type": "array", "items": { "type": "string" @@ -204,9 +204,9 @@ "ignoreSync": true }, "coder.useKeyring": { - "markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of plaintext files. Requires CLI >= 2.29.0 (>= 2.31.0 to sync login from CLI to VS Code). This will attempt to sync between the CLI and VS Code since they share the same keyring entry. It will log you out of the CLI if you log out of the IDE, and vice versa. Has no effect on Linux.", + "markdownDescription": "Store session tokens in the OS keyring (macOS Keychain, Windows Credential Manager) instead of a file. Requires Coder CLI 2.29.0 or later; 2.32.0 or later to sign in with the CLI's existing session. Has no effect on Linux.\n\nThe keyring entry is shared with the `coder` CLI: signing in here also signs in the CLI, and signing out 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..b59b7ce50f 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,55 @@ 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.32+). Undefined on any failure. */ + public async readToken( url: string, - token: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - const args = [ - ...this.credentialGlobalFlags(transport, url, configs), - "login", - "--use-token-as-session", - url, - ]; + ): Promise { + 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); + this.logger.warn("Could not resolve CLI binary:", error); + return undefined; } - } - - /** - * Read a token via `coder login token` (keyring or file-backed). Requires - * 2.31.0+; older deployments return undefined. Returns the token and its - * source, or undefined on any failure. Throws AbortError on abort. - */ - public async readToken( - url: string, - configs: Pick, - options?: { signal?: AbortSignal }, - ): Promise { - const transport = await this.resolveReadTransport(url, configs); - if (transport.kind === "none") { + if (!cli.featureSet.tokenRead) { return undefined; } - const args = [ - ...this.credentialGlobalFlags(transport, url, configs), - "login", - "token", - "--url", - url, - ]; - const token = await this.runTokenRead(transport.binPath, args, options); - if (!token) { + // 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 { - 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 +121,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.32 the token is not read back; 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 +251,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/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/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..8b7e1c1d32 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.32+, where the CLI reports 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..6f9e5e0fa2 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.32+", + platform: "darwin", + override: "flag", + version: "2.32.0", + expected: [ + "--verbose", + `--global-config=${USER_DIR}`, + ...SHARED_FLAGS, + "--use-keyring=true", + ], + }, + { + scenario: "honors CODER_CONFIG_DIR on 2.32+ by emitting no directory", + platform: "darwin", + override: "env", + version: "2.32.0", + expected: ["--verbose", ...SHARED_FLAGS, "--use-keyring=true"], + }, + { + scenario: "honors a globalFlags --global-config with keyring disabled", + platform: "linux", + override: "flag", + version: "2.32.0", + expected: [ + "--verbose", + `--global-config=${USER_DIR}`, + ...SHARED_FLAGS, + "--use-keyring=false", + ], + }, + { + scenario: + "keeps the extension directory over a user directory below 2.32", + platform: "linux", + override: "flag", + version: "2.31.0", + expected: ["--verbose", ...PRIVATE_FLAGS, "--use-keyring=false"], + }, + { + scenario: + "keeps the extension directory over CODER_CONFIG_DIR below 2.32", + platform: "linux", + override: "env", + version: "2.31.0", + expected: ["--verbose", ...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..a1d1d7a68f 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,471 @@ 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); -} +const execCalls = () => + vi.mocked(execFile).mock.calls.map((call) => call[1] as string[]); +const execOptions = () => vi.mocked(execFile).mock.calls[0][2] as ExecOptions; -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, - ), -}; - -// 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.mocked(cliExec.version).mockResolvedValue("2.31.0"); + vi.stubEnv("CODER_CONFIG_DIR", undefined); + // Linux: keyring unsupported, so the extension directory is used. + vi.mocked(os.platform).mockReturnValue("linux"); + vi.mocked(cliExec.version).mockResolvedValue("2.32.0"); }); - describe("storeToken", () => { - it("writes via coder login (file mode) when keyring is disabled", async () => { - stubExecFile({ stdout: "" }); - const { manager, resolver, sink } = setup(); + 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", - }, - }); - }); - - 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" }, + properties: { "error.type": "cli", result: "error" }, }); - 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" }); + it("returns the trimmed token from the CLI store", async () => { + vi.mocked(os.platform).mockReturnValue("darwin"); + stubExecFile({ token: " my-token\n" }); const { manager } = setup(); - expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); - }); - it("returns undefined on CLI error", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ error: "no token found" }); - const { manager } = setup(); - expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(await manager.readToken(TEST_URL, configs)).toBe("my-token"); + expect(execCalls()).toEqual([[...KEYRING_FLAGS, "login", "token"]]); }); - it("returns undefined when binary resolver fails", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - const { manager } = setup(failingResolver()); + 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, 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, - ]); }); - it("reads via coder login token under a user --global-config override", async () => { - stubExecFile({ stdout: "custom-file-token" }); + 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( - 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); + await manager.readToken("http://dev.coder.com", configs), + ).toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); }); - it("returns undefined when keyring is enabled but unsupported by the CLI", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); - const { manager, resolver } = setup(); - - expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); - expect(resolver).toHaveBeenCalledWith(TEST_URL); - expect(execFile).not.toHaveBeenCalled(); - }); - - it("returns undefined when CLI version too old for token read", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - // 2.30 supports keyringAuth but not tokenRead (requires 2.31+) - vi.mocked(cliExec.version).mockResolvedValueOnce("2.30.0"); - stubExecFile({ stdout: "my-token" }); + it("returns undefined below CLI 2.32 without running the CLI", async () => { + vi.mocked(cliExec.version).mockResolvedValue("2.31.0"); const { manager } = setup(); expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); }); - - it("passes timeout to execFile", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "token" }); - const { manager } = setup(); - - await manager.readToken(TEST_URL, configs); - - expect(lastExecArgs().timeout).toBe(60_000); - }); - - it("passes signal through to execFile", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "token" }); - const { manager } = setup(); - const ac = new AbortController(); - - await manager.readToken(TEST_URL, configs, { signal: ac.signal }); - - expect(lastExecArgs().signal).toBe(ac.signal); - }); - - it("throws AbortError when signal is aborted", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFileAbortable(); - const { manager } = setup(); - - await expect( - manager.readToken(TEST_URL, configs, { - signal: AbortSignal.abort(), - }), - ).rejects.toThrow("The operation was aborted"); - }); }); describe("deleteToken", () => { - it("deletes files and invokes coder logout when keyring enabled", async () => { - vi.mocked(isKeyringEnabled).mockReturnValue(true); - stubExecFile({ stdout: "" }); - writeCredentialFiles(TEST_URL, "old-token"); - const { manager, resolver, sink } = setup(); - - const result = await manager.deleteToken(TEST_URL, configs); - - expect(result).toBe(true); - expect(resolver).toHaveBeenCalledWith(TEST_URL); - const exec = lastExecArgs(); - expect(exec.bin).toBe(TEST_BIN); - expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); - expect(credentialFilesExist()).toBe(false); - expect(sink.expectOne("auth.credential.clear")).toMatchObject({ - properties: { - category: "keyring", - keyring_enabled: "true", - result: "success", - }, - }); - }); - - 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.32", async () => { + vi.mocked(cliExec.version).mockResolvedValue("2.31.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/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", () => { 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", + ); + }); +});