diff --git a/CHANGELOG.md b/CHANGELOG.md index 79ab6c08a..c65870ed4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,15 @@ raw output. When a slow connection is detected, the network status bar tooltip also links to the network check alongside the latency test, for a deployment-wide view of the network path. +- Share login with the terminal `coder` CLI by setting `--global-config=` + in `coder.globalFlags`; the extension reads file-based CLI credentials from + that directory (requires a deployment on 2.31.0+). + +### Changed + +- File-based credentials are now written and cleared through `coder login` and + `coder logout` rather than by the extension directly. The minimum supported + Coder version is now v0.25.0. ### Fixed diff --git a/package.json b/package.json index 5bb9524f0..62775a3be 100644 --- a/package.json +++ b/package.json @@ -182,7 +182,7 @@ "scope": "machine" }, "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\nFor `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.", + "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.", "type": "array", "items": { "type": "string" diff --git a/src/api/authInterceptor.ts b/src/api/authInterceptor.ts index 4341c6d3b..ea533f6e4 100644 --- a/src/api/authInterceptor.ts +++ b/src/api/authInterceptor.ts @@ -2,7 +2,7 @@ import { type AxiosError, isAxiosError } from "axios"; import { AuthTelemetry } from "../instrumentation/auth"; import { OAuthError } from "../oauth/errors"; -import { toSafeHost } from "../util"; +import { toSafeHost } from "../util/uri"; import type * as vscode from "vscode"; diff --git a/src/commands.ts b/src/commands.ts index 6431b80c8..d85f59df0 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -46,7 +46,8 @@ import { import { resolveCliAuth } from "./settings/cli"; import { appendVsCodeLogs } from "./supportBundle/appendVsCodeLogs"; import { runExportTelemetryCommand } from "./telemetry/export/command"; -import { openInBrowser, toRemoteAuthority, toSafeHost } from "./util"; +import { toRemoteAuthority } from "./util/authority"; +import { openInBrowser, toSafeHost } from "./util/uri"; import { vscodeProposed } from "./vscodeProposed"; import { parseNetcheckReport } from "./webviews/netcheck/types"; import { parseSpeedtestResult } from "./webviews/speedtest/types"; diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 245f1a5e1..763d12d65 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -1,22 +1,19 @@ import { execFile } from "node:child_process"; import fs from "node:fs/promises"; import os from "node:os"; -import path from "node:path"; import { promisify } from "node:util"; import * as semver from "semver"; import { isAbortError } from "../error/errorUtils"; -import { featureSetForVersion } from "../featureSet"; +import { featureSetForVersion, type FeatureSet } from "../featureSet"; import { CredentialCliError, - CredentialFileError, CredentialTelemetry, } from "../instrumentation/credentials"; -import { isKeyringEnabled } from "../settings/cli"; +import { getGlobalFlags, isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; import { type TelemetryReporter } from "../telemetry/reporter"; -import { toSafeHost } from "../util"; -import { writeAtomically } from "../util/fs"; +import { toSafeHost } from "../util/uri"; import { version } from "./cliExec"; @@ -29,7 +26,17 @@ import type { PathResolver } from "./pathResolver"; const execFileAsync = promisify(execFile); -type KeyringFeature = "keyringAuth" | "keyringTokenRead"; +// 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; @@ -49,8 +56,8 @@ export function isKeyringSupported(): boolean { } /** - * Delegates credential storage to the Coder CLI. Owns all credential - * persistence: keyring-backed (via CLI) and file-based (plaintext). + * Delegates credential storage to the Coder CLI, both keyring-backed and + * file-based, via `coder login`/`coder logout`. */ export class CliCredentialManager { private readonly credentialTelemetry: CredentialTelemetry; @@ -65,11 +72,8 @@ export class CliCredentialManager { } /** - * Store credentials for a deployment URL. Uses the OS keyring when the - * setting is enabled and the CLI supports it; otherwise writes plaintext - * files under --global-config. - * - * Keyring and files are mutually exclusive, never both. + * Store credentials via `coder login` (keyring or file-backed). Throws if the + * CLI binary cannot be resolved. */ public storeToken( url: string, @@ -78,36 +82,30 @@ export class CliCredentialManager { options?: { signal?: AbortSignal }, ): Promise { return this.credentialTelemetry.traceStore(configs, async (span) => { - const binPath = await this.resolveKeyringBinary( - url, - configs, - "keyringAuth", + const transport = await this.resolveWriteTransport(url, configs); + span.setProperty( + "category", + transport.kind === "keyring" ? "keyring" : "file", ); - if (!binPath) { - span.setProperty("category", "file"); - await this.writeCredentialFiles(url, token); - return; - } - span.setProperty("category", "keyring"); - await this.storeKeyringToken(binPath, url, token, configs, options); + await this.cliLogin(transport, url, token, configs, options); }); } - private async storeKeyringToken( - binPath: string, + private async cliLogin( + transport: CliTransport, url: string, token: string, configs: Pick, options?: { signal?: AbortSignal }, ): Promise { const args = [ - ...getHeaderArgs(configs), + ...this.credentialGlobalFlags(transport, url, configs), "login", "--use-token-as-session", url, ]; try { - await this.execWithTimeout(binPath, args, { + await this.execWithTimeout(transport.binPath, args, { env: { ...process.env, CODER_SESSION_TOKEN: token }, signal: options?.signal, }); @@ -122,37 +120,46 @@ export class CliCredentialManager { } /** - * Read a token via `coder login token --url`. Returns trimmed stdout, - * or undefined on any failure (resolver, CLI, empty output). - * Throws AbortError when the signal is aborted. + * 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 { - let binPath: string | undefined; - try { - binPath = await this.resolveKeyringBinary( - url, - configs, - "keyringTokenRead", - ); - } catch (error) { - this.logger.warn("Could not resolve CLI binary for token read:", error); + ): Promise { + const transport = await this.resolveReadTransport(url, configs); + if (transport.kind === "none") { return undefined; } - if (!binPath) { + const args = [ + ...this.credentialGlobalFlags(transport, url, configs), + "login", + "token", + "--url", + url, + ]; + const token = await this.runTokenRead(transport.binPath, args, options); + if (!token) { return undefined; } + return { + token, + source: transport.kind === "keyring" ? "keyring" : "files", + }; + } - const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; + private async runTokenRead( + binPath: string, + args: string[], + options?: { signal?: AbortSignal }, + ): Promise { try { const { stdout } = await this.execWithTimeout(binPath, args, { signal: options?.signal, }); - const token = stdout.trim(); - return token || undefined; + return nonEmpty(stdout); } catch (error) { if (isAbortError(error)) { throw error; @@ -163,9 +170,9 @@ export class CliCredentialManager { } /** - * Delete credentials for a deployment. Runs file deletion and keyring - * deletion in parallel, both best-effort. Throws AbortError when the - * signal is aborted. + * Delete credentials for a deployment. Removes the default-dir files and + * logs out of the active store (keyring or file via --global-config), both + * best-effort. Throws AbortError when the signal is aborted. */ public deleteToken( url: string, @@ -175,33 +182,109 @@ export class CliCredentialManager { return this.credentialTelemetry.traceClear(configs, async (span) => { await Promise.all([ this.deleteCredentialFiles(url), - this.deleteKeyringToken(url, configs, { - signal: options?.signal, - span, - }), + this.cliLogout(url, configs, { signal: options?.signal, span }), ]); }); } /** - * Resolve a CLI binary for keyring operations. Returns the binary path - * when keyring is enabled in settings and the CLI version supports the - * requested feature, or undefined to fall back to file-based storage. - * - * Throws on binary resolution or version-check failure (caller decides - * whether to catch or propagate). + * Log out via `coder logout`, keyring or file (--global-config). Records + * failures on the span instead of throwing (except on abort). */ - private async resolveKeyringBinary( + private async cliLogout( url: string, configs: Pick, - feature: KeyringFeature, - ): Promise { - if (!isKeyringEnabled(configs)) { - return undefined; + { signal, span }: { signal?: AbortSignal; span: Span }, + ): Promise { + let transport: CliTransport; + try { + transport = await this.resolveWriteTransport(url, configs); + } catch (error) { + this.logger.warn("Could not resolve CLI binary for logout:", error); + span.setProperty("error.type", "binary"); + span.markError(); + return; } + const args = [ + ...this.credentialGlobalFlags(transport, url, configs), + "logout", + "--url", + url, + "--yes", + ]; + try { + await this.execWithTimeout(transport.binPath, args, { signal }); + this.logger.info("Deleted token via CLI for", url); + } catch (error) { + if (isAbortError(error)) { + throw error; + } + this.logger.warn("Failed to delete token via CLI:", error); + span.setProperty("error.type", "cli"); + span.markError(); + } + } + + /** 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); - const cliVersion = semver.parse(await version(binPath)); - return featureSetForVersion(cliVersion)[feature] ? binPath : undefined; + 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" }; + } + if (isKeyringEnabled(configs) && cli.featureSet.keyringAuth) { + return cli.featureSet.tokenRead + ? { kind: "keyring", binPath: cli.binPath } + : { kind: "none" }; + } + if (cli.featureSet.tokenRead) { + return cliFileTransport(cli); + } + return { kind: "none" }; + } + + /** Keyring uses the default store; file mode passes --global-config. */ + private credentialGlobalFlags( + transport: CliTransport, + url: string, + configs: Pick, + ): string[] { + if (transport.kind === "keyring") { + return getHeaderArgs(configs); + } + return getGlobalFlags(configs, { + mode: "global-config", + configDir: this.pathResolver.getGlobalConfigDir(toSafeHost(url)), + allowOverride: transport.allowOverride, + }); + } + + private async getFeatureSet(binPath: string): Promise { + return featureSetForVersion(semver.parse(await version(binPath))); } /** @@ -227,27 +310,6 @@ export class CliCredentialManager { } } - /** - * Write URL and token files under --global-config. - */ - private async writeCredentialFiles( - url: string, - token: string, - ): Promise { - try { - const safeHostname = toSafeHost(url); - await Promise.all([ - this.atomicWriteFile(this.pathResolver.getUrlPath(safeHostname), url), - this.atomicWriteFile( - this.pathResolver.getSessionTokenPath(safeHostname), - token, - ), - ]); - } catch (error) { - throw new CredentialFileError(error); - } - } - /** * Delete URL and token files. Best-effort: never throws. */ @@ -265,55 +327,22 @@ export class CliCredentialManager { ), ); } +} - /** - * Delete keyring token via `coder logout`. Best-effort: records the failure - * on the span instead of throwing (except on abort), so it is tagged where - * it occurs. - */ - private async deleteKeyringToken( - url: string, - configs: Pick, - { signal, span }: { signal?: AbortSignal; span: Span }, - ): Promise { - let binPath: string | undefined; - try { - binPath = await this.resolveKeyringBinary(url, configs, "keyringAuth"); - } catch (error) { - this.logger.warn("Could not resolve keyring binary for delete:", error); - span.setProperty("error.type", "binary"); - span.markError(); - return; - } - if (!binPath) { - return; - } - - const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"]; - try { - await this.execWithTimeout(binPath, args, { signal }); - this.logger.info("Deleted token via CLI for", url); - } catch (error) { - if (isAbortError(error)) { - throw error; - } - this.logger.warn("Failed to delete token via CLI:", error); - span.setProperty("error.type", "cli"); - span.markError(); - } - } +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, + }; +} - /** Atomically write content to a file. */ - private async atomicWriteFile( - filePath: string, - content: string, - ): Promise { - await fs.mkdir(path.dirname(filePath), { recursive: true }); - await writeAtomically( - filePath, - (tempPath) => fs.writeFile(tempPath, content, { mode: 0o600 }), - (rmErr, tempPath) => - this.logger.warn("Failed to delete temp file", tempPath, rmErr), - ); - } +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 7154640b7..465e1a9d0 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -23,8 +23,8 @@ import { import * as pgp from "../pgp"; import { withCancellableProgress, withOptionalProgress } from "../progress"; import { isKeyringEnabled } from "../settings/cli"; -import { toSafeHost } from "../util"; import { tempFilePath } from "../util/fs"; +import { toSafeHost } from "../util/uri"; import { vscodeProposed } from "../vscodeProposed"; import { BinaryLock } from "./binaryLock"; @@ -994,8 +994,8 @@ export class CliManager { /** * Configure the CLI for the deployment with the provided hostname. * - * Stores credentials in the OS keyring when the setting is enabled and the - * CLI supports it, otherwise writes plaintext files under --global-config. + * Stores credentials via `coder login`: in the OS keyring when the setting is + * enabled and the CLI supports it, otherwise file-based under --global-config. * * Both URL and token are required. Empty tokens are allowed (e.g. mTLS * authentication) but the URL must be a non-empty string. diff --git a/src/core/container.ts b/src/core/container.ts index 289cf428a..b448a1bdd 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -1,6 +1,5 @@ import * as vscode from "vscode"; -import { CoderApi } from "../api/coderApi"; import { AuthTelemetry } from "../instrumentation/auth"; import { LoginCoordinator } from "../login/loginCoordinator"; import { OAuthCallback } from "../oauth/oauthCallback"; @@ -83,12 +82,9 @@ export class ServiceContainer implements vscode.Disposable { "BinaryResolver called before CliManager was initialised", ); } - try { - return await this.cliManager.locateBinary(url); - } catch { - const client = CoderApi.create(url, "", this.logger); - return this.cliManager.fetchBinary(client); - } + // Locate-only: never download from credential ops; the connect and + // CLI-command flows fetch the binary first. + return this.cliManager.locateBinary(url); }, this.pathResolver, this.telemetryService, diff --git a/src/core/pathResolver.ts b/src/core/pathResolver.ts index f4b337584..191534ba6 100644 --- a/src/core/pathResolver.ts +++ b/src/core/pathResolver.ts @@ -10,10 +10,9 @@ export class PathResolver { ) {} /** - * Return the directory for the deployment with the provided hostname to - * where the global Coder configs are stored. - * - * The caller must ensure this directory exists before use. + * Per-deployment directory for the extension's Coder configs. A user + * `--global-config` in `coder.globalFlags` redirects the CLI; this stays the + * default. Caller must ensure it exists. */ public getGlobalConfigDir(safeHostname: string): string { return path.join(this.basePath, safeHostname); diff --git a/src/core/secretsManager.ts b/src/core/secretsManager.ts index 1d9c22be9..5255c122c 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -1,7 +1,7 @@ import { z } from "zod"; import { DeploymentSchema, type Deployment } from "../deployment/types"; -import { toSafeHost } from "../util"; +import { toSafeHost } from "../util/uri"; import type { OAuth2ClientRegistrationResponse } from "coder/site/src/api/typesGenerated"; import type { Memento, SecretStorage, Disposable } from "vscode"; diff --git a/src/featureSet.ts b/src/featureSet.ts index 8cc17b8cb..57a3b1d68 100644 --- a/src/featureSet.ts +++ b/src/featureSet.ts @@ -1,13 +1,13 @@ import type * as semver from "semver"; export interface FeatureSet { - vscodessh: boolean; + cliLogin: boolean; proxyLogDirectory: boolean; wildcardSSH: boolean; buildReason: boolean; cliUpdate: boolean; keyringAuth: boolean; - keyringTokenRead: boolean; + tokenRead: boolean; supportBundle: boolean; } @@ -32,13 +32,9 @@ export function featureSetForVersion( version: semver.SemVer | null, ): FeatureSet { return { - vscodessh: !( - version?.major === 0 && - version?.minor <= 14 && - version?.patch < 1 && - version?.prerelease.length === 0 - ), - + // `coder login --use-token-as-session` to write a token (file or keyring). + // The extension relies on this, so 0.25.0 is the minimum supported version. + cliLogin: versionAtLeast(version, "0.25.0"), // --log-dir flag for proxy logs; vscodessh fails if unsupported proxyLogDirectory: versionAtLeast(version, "2.4.0"), // Wildcard SSH host matching @@ -49,8 +45,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 from the keyring - keyringTokenRead: versionAtLeast(version, "2.31.0"), + // `coder login token` for reading tokens (keyring or file) + tokenRead: versionAtLeast(version, "2.31.0"), // `coder support bundle` (officially released/unhidden in 2.10.0) supportBundle: versionAtLeast(version, "2.10.0"), }; diff --git a/src/instrumentation/EVENTS.md b/src/instrumentation/EVENTS.md index b4171cf69..a3186665d 100644 --- a/src/instrumentation/EVENTS.md +++ b/src/instrumentation/EVENTS.md @@ -210,7 +210,7 @@ Secret-storage session read during remote setup. No custom attributes. | ----------------- | ------------------------------------------------- | | `keyring_enabled` | `true`, `false` (from settings) | | `category` | `keyring`, `file` (the storage actually involved) | -| `error.type` | `binary`, `cli`, `file` | +| `error.type` | `binary`, `cli` | ### Logs @@ -265,12 +265,12 @@ Phase `cli.download.verify` covers binary signature verification: #### `cli.configure` -| Attribute | Values | -| ------------------- | ------------------------------------------- | -| `silent` | `true`, `false` | -| `credential_source` | `session_token`, `empty_token` | -| `abort_stage` | `credential_store` | -| `error.type` | `filesystem`, `credential_store`, `unknown` | +| Attribute | Values | +| ------------------- | ------------------------------ | +| `silent` | `true`, `false` | +| `credential_source` | `session_token`, `empty_token` | +| `abort_stage` | `credential_store` | +| `error.type` | `credential_store`, `unknown` | ## Commands @@ -337,8 +337,8 @@ Emitted by `RemoteSetupTelemetry` around connecting to a remote workspace. | --------- | -------------------------------------------------------------------------------------------- | | `outcome` | `workspace_not_found`, `incompatible_server` (non-throwing early exits; the span is aborted) | -Phases (`remote.setup.`): `cli_resolve`, `cli_configure`, -`compatibility_check`, `workspace_lookup`, `workspace_monitor_setup`, +Phases (`remote.setup.`): `cli_resolve`, `compatibility_check`, +`cli_configure`, `workspace_lookup`, `workspace_monitor_setup`, `workspace_ready`, `agent_resolve`, `ssh_config_write`, `ssh_monitor_setup`, `connection_handoff`. diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts index d819c7e6d..2057e2a65 100644 --- a/src/instrumentation/cli.ts +++ b/src/instrumentation/cli.ts @@ -1,4 +1,3 @@ -import { CredentialFileError } from "./credentials"; import { recordAborted, recordError } from "./outcomes"; import type { CallerPropertyValue } from "../telemetry/event"; @@ -20,10 +19,7 @@ export type CliVersionCheckOutcome = | "match" | "mismatch" | "unreadable"; -export type CliConfigureErrorType = - | "filesystem" - | "credential_store" - | "unknown"; +export type CliConfigureErrorType = "credential_store" | "unknown"; export type CliResolveErrorType = | "downloads_disabled" | "download" @@ -192,10 +188,6 @@ export class CliConfigureTrace { } function categorizeConfigureError(error: unknown): CliConfigureErrorType { - // A CredentialFileError is a file-write failure; anything else is keyring/CLI. - if (error instanceof CredentialFileError) { - return "filesystem"; - } return error instanceof Error ? "credential_store" : "unknown"; } diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts index 5d6096d41..c921e3837 100644 --- a/src/instrumentation/credentials.ts +++ b/src/instrumentation/credentials.ts @@ -6,7 +6,7 @@ import type { WorkspaceConfiguration } from "vscode"; import type { TelemetryReporter } from "../telemetry/reporter"; import type { Span } from "../telemetry/span"; -export type CredentialErrorCategory = "binary" | "cli" | "file"; +export type CredentialErrorCategory = "binary" | "cli"; type CredentialEvent = "auth.credential.store" | "auth.credential.clear"; @@ -68,9 +68,6 @@ export class CredentialTelemetry { } function categorizeCredentialError(error: unknown): CredentialErrorCategory { - if (error instanceof CredentialFileError) { - return "file"; - } if (error instanceof CredentialCliError) { return "cli"; } @@ -83,10 +80,3 @@ export class CredentialCliError extends Error { this.name = "CredentialCliError"; } } - -export class CredentialFileError extends Error { - public constructor(cause: unknown) { - super("Credential file operation failed", { cause }); - this.name = "CredentialFileError"; - } -} diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index cb02627a9..0023b95db 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -10,7 +10,7 @@ import { buildOAuthTokenData } from "../oauth/utils"; import { withOptionalProgress } from "../progress"; import { maybeAskAuthMethod, maybeAskUrl } from "../promptUtils"; import { isKeyringEnabled } from "../settings/cli"; -import { openInBrowser } from "../util"; +import { openInBrowser } from "../util/uri"; import { vscodeProposed } from "../vscodeProposed"; import type { User } from "coder/site/src/api/typesGenerated"; @@ -314,30 +314,41 @@ export class LoginCoordinator implements vscode.Disposable { } } - // Try keyring token (picks up tokens written by `coder login` in the terminal) + // Try CLI-managed credentials. This reads from the OS keyring when + // enabled, otherwise from the resolved global config directory. const configs = vscode.workspace.getConfiguration(); - const keyringResult = await withOptionalProgress( + const keyringEnabled = isKeyringEnabled(configs); + const cliCredentialResult = await withOptionalProgress( ({ signal }) => this.cliCredentialManager.readToken(deployment.url, configs, { signal, }), { - enabled: isKeyringEnabled(configs), + enabled: keyringEnabled, location: vscode.ProgressLocation.Notification, title: "Reading token from OS keyring...", cancellable: true, }, ); - const keyringToken = keyringResult.ok ? keyringResult.value : undefined; + const cliCredential = cliCredentialResult.ok + ? cliCredentialResult.value + : undefined; if ( - keyringToken && - keyringToken !== providedToken && - keyringToken !== auth?.token + cliCredential && + cliCredential.token !== providedToken && + cliCredential.token !== auth?.token ) { - this.logger.debug("Trying token from OS keyring"); - const result = await this.tryTokenAuth(client, keyringToken, isAutoLogin); + this.logger.debug("Trying token from CLI credentials"); + const result = await this.tryTokenAuth( + client, + cliCredential.token, + isAutoLogin, + ); if (result !== "unauthorized") { - return withLoginMethod("keyring_token", result); + return withLoginMethod( + cliCredential.source === "keyring" ? "keyring_token" : "cli_token", + result, + ); } } diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index 809ccfaed..864ca08a4 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -1,7 +1,7 @@ import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; -import { resolveUiUrl } from "../util"; +import { resolveCoderDashboardUrl } from "../util/uri"; import { AUTH_GRANT_TYPE, @@ -382,7 +382,7 @@ function toBrowserAuthorizationUrl( ): URL { const endpoint = new URL(authorizationEndpoint); const connectionBase = new URL(connectionUrl); - const browserBase = new URL(resolveUiUrl(connectionUrl)); + const browserBase = new URL(resolveCoderDashboardUrl(connectionUrl)); const connectionPrefix = connectionBase.pathname.replace(/\/$/, ""); const browserPrefix = browserBase.pathname.replace(/\/$/, ""); const underConnection = diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 61b0216ef..14f7b4609 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -37,13 +37,12 @@ import { resolveCliAuth, } from "../settings/cli"; import { getHeaderCommand } from "../settings/headers"; +import { escapeCommandArg, expandPath } from "../util"; import { AuthorityPrefix, type AuthorityParts, - escapeCommandArg, - expandPath, parseRemoteAuthority, -} from "../util"; +} from "../util/authority"; import { vscodeProposed } from "../vscodeProposed"; import { WorkspaceMonitor } from "../workspace/workspaceMonitor"; @@ -265,16 +264,6 @@ export class Remote { this.resolveRemoteBinary(workspaceClient), ); - // Write token to keyring or file - if (baseUrl && token !== undefined) { - await tracer.phase("cli_configure", () => - this.cliManager.configure(baseUrl, token), - ); - } - - // Listen for token changes for this deployment - disposables.push(this.watchRemoteSessionAuth(context, workspaceClient)); - const { featureSet, cliAuth } = await tracer.phase( "compatibility_check", () => @@ -286,14 +275,15 @@ export class Remote { }), ); - // Server versions before v0.14.1 don't support the vscodessh command! - if (!featureSet.vscodessh) { + // Reject deployments below our minimum supported version (v0.25.0) + // before configuring credentials, so they get a clear message. + if (!featureSet.cliLogin) { tracer.markAborted("incompatible_server"); await vscodeProposed.window.showErrorMessage( "Incompatible Server", { detail: - "Your Coder server is too old to support the Coder extension! Please upgrade to v0.14.1 or newer.", + "Your Coder server is too old to support the Coder extension! Please upgrade to v0.25.0 or newer.", modal: true, useCustom: true, }, @@ -306,6 +296,16 @@ export class Remote { return; } + // Write token to keyring or file + if (baseUrl && token !== undefined) { + await tracer.phase("cli_configure", () => + this.cliManager.configure(baseUrl, token), + ); + } + + // Listen for token changes for this deployment + disposables.push(this.watchRemoteSessionAuth(context, workspaceClient)); + // Next is to find the workspace from the URI scheme provided. const foundWorkspace = await tracer.phase("workspace_lookup", () => this.lookupWorkspace(context, workspaceClient), diff --git a/src/remote/workspaceStateMachine.ts b/src/remote/workspaceStateMachine.ts index c459aa502..f0915da4c 100644 --- a/src/remote/workspaceStateMachine.ts +++ b/src/remote/workspaceStateMachine.ts @@ -37,7 +37,7 @@ import type { StartupMode } from "../core/mementoManager"; import type { FeatureSet } from "../featureSet"; import type { Logger } from "../logging/logger"; import type { CliAuth } from "../settings/cli"; -import type { AuthorityParts } from "../util"; +import type { AuthorityParts } from "../util/authority"; /** * Manages workspace and agent state transitions until ready for SSH connection. diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 5086e6d62..4827ec75d 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -8,7 +8,7 @@ import type { WorkspaceConfiguration } from "vscode"; import type { FeatureSet } from "../featureSet"; export type CliAuth = - | { mode: "global-config"; configDir: string } + | { mode: "global-config"; configDir: string; allowOverride: boolean } | { mode: "url"; url: string }; /** @@ -51,27 +51,38 @@ function buildGlobalFlags( escAuth: (s: string) => string, escHeader: (s: string) => string, ): string[] { - const authFlags = - auth.mode === "url" - ? ["--url", escAuth(auth.url)] - : ["--global-config", escAuth(auth.configDir)]; - - // Escape each user flag so expansion-introduced whitespace stays inside - // one shell token. `escAuth` is `identity` on the array path. - const filtered = stripManagedFlags(getExpandedUserGlobalFlags(configs)).map( - escAuth, - ); + 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); - return [...filtered, ...authFlags, ...getHeaderArgs(configs, escHeader)]; + // Keyring mode: --url auth; drop user --global-config (would force file storage). + if (auth.mode === "url") { + return [...cleanUserFlags(true), "--url", escAuth(auth.url), ...headers]; + } + + // 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]; } -function stripManagedFlags(flags: string[]): string[] { +function stripManagedFlags( + flags: string[], + stripGlobalConfig: boolean, +): string[] { const filtered: string[] = []; for (let i = 0; i < flags.length; i++) { if (isFlag(flags[i], "--use-keyring")) { continue; } - if (isFlag(flags[i], "--global-config")) { + if (stripGlobalConfig && isFlag(flags[i], "--global-config")) { // Skip the next item too when the value is a separate entry. if (flags[i] === "--global-config") { i++; @@ -113,7 +124,12 @@ export function resolveCliAuth( if (isKeyringEnabled(configs) && featureSet.keyringAuth) { return { mode: "url", url: deploymentUrl }; } - return { mode: "global-config", configDir }; + // Honored only on 2.31.0+, where CLI-mediated read/write share the directory. + return { + mode: "global-config", + configDir, + allowOverride: featureSet.tokenRead, + }; } /** diff --git a/src/uri/uriHandler.ts b/src/uri/uriHandler.ts index 1e5765264..efd8f1b7d 100644 --- a/src/uri/uriHandler.ts +++ b/src/uri/uriHandler.ts @@ -4,7 +4,7 @@ import { errToStr } from "../api/api-helper"; import { AuthTelemetry } from "../instrumentation/auth"; import { CALLBACK_PATH } from "../oauth/utils"; import { maybeAskUrl } from "../promptUtils"; -import { toSafeHost } from "../util"; +import { toSafeHost } from "../util/uri"; import { vscodeProposed } from "../vscodeProposed"; import type { Commands } from "../commands"; diff --git a/src/util.ts b/src/util.ts index 5ea37b8c1..33510981c 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1,22 +1,4 @@ import os from "node:os"; -import url from "node:url"; -import * as vscode from "vscode"; - -export interface AuthorityParts { - agent: string | undefined; - sshHost: string; - safeHostname: string; - username: string; - workspace: string; -} - -// Prefix is a magic string that is prepended to SSH hosts to indicate that -// they should be handled by this extension. -export const AuthorityPrefix = "coder-vscode"; - -const authorityHostPrefix = `${AuthorityPrefix}.`; -const invalidAuthorityMessage = - "Invalid Coder SSH authority. Must be: ----(.)"; // Regex patterns to find the SSH port from Remote SSH extension logs. // `ms-vscode-remote.remote-ssh`: `-> socksPort ->` or `between local port ` @@ -51,86 +33,6 @@ export function findPort(text: string): number | null { return Number.parseInt(portStr); } -/** - * Given an authority, parse into the expected parts. - * - * The authority looks like `://ssh-remote+`, where the - * SSH host names created by this extension match the format: - * coder-vscode.----(.) - * - * If this is not a Coder authority, return null. - * - * Throw an error if a Coder authority is invalid. - */ -export function parseRemoteAuthority(authority: string): AuthorityParts | null { - const authorityParts = authority.split("+"); - const sshHost = authorityParts[1]; - if (!sshHost) { - return null; - } - - const parts = sshHost.split("--"); - if (!parts[0].startsWith(authorityHostPrefix)) { - return null; - } - - if (parts.length < 3) { - throw new Error(invalidAuthorityMessage); - } - - // Parse from the right because safe hostnames can contain "--". - const hostPrefix = parts.slice(0, -2).join("--"); - const safeHostname = hostPrefix.slice(authorityHostPrefix.length); - const username = parts[parts.length - 2]; - const workspaceAndAgent = parts[parts.length - 1]; - if (!safeHostname || !username || !workspaceAndAgent) { - throw new Error(invalidAuthorityMessage); - } - - let workspace = workspaceAndAgent; - let agent = ""; - const workspaceParts = workspaceAndAgent.split("."); - // Multiple dots are ambiguous because workspace and agent share this separator. - if (workspaceParts.length === 2) { - workspace = workspaceParts[0]; - agent = workspaceParts[1]; - if (!workspace || !agent) { - throw new Error(invalidAuthorityMessage); - } - } - - return { - agent, - sshHost, - safeHostname, - username, - workspace, - }; -} - -export function toRemoteAuthority( - baseUrl: string, - workspaceOwner: string, - workspaceName: string, - workspaceAgent: string | undefined, -): string { - let remoteAuthority = `ssh-remote+${AuthorityPrefix}.${toSafeHost(baseUrl)}--${workspaceOwner}--${workspaceName}`; - if (workspaceAgent) { - remoteAuthority += `.${workspaceAgent}`; - } - return remoteAuthority; -} - -/** - * Given a URL, return the host in a format that is safe to write. - */ -export function toSafeHost(rawUrl: string): string { - const u = new URL(rawUrl); - // If the host is invalid, an empty string is returned. Although, `new URL` - // should already have thrown in that case. - return url.domainToASCII(u.hostname) || u.hostname; -} - /** * Substitute `${env:VAR}` with `process.env.VAR` (unset → empty string), * `${userHome}` (anywhere) with `os.homedir()`, and a leading `~` with @@ -196,29 +98,3 @@ export function escapeShellArg(arg: string): string { } return `'${arg.replace(/'/g, "'\\''")}'`; } - -/** - * Return the URL for opening Coder pages in the browser. Uses the - * `coder.alternativeWebUrl` setting when configured, otherwise returns - * the connection URL unchanged. - */ -export function resolveUiUrl(connectionUrl: string): string { - const alt = vscode.workspace - .getConfiguration("coder") - .get("alternativeWebUrl") - ?.trim() - .replace(/\/+$/, ""); - return alt || connectionUrl; -} - -/** - * Open a path on the Coder deployment in the user's browser, applying - * `coder.alternativeWebUrl` when configured. - */ -export function openInBrowser( - connectionUrl: string, - path: string, -): Thenable { - const base = vscode.Uri.parse(resolveUiUrl(connectionUrl)); - return vscode.env.openExternal(vscode.Uri.joinPath(base, path)); -} diff --git a/src/util/authority.ts b/src/util/authority.ts new file mode 100644 index 000000000..b99200636 --- /dev/null +++ b/src/util/authority.ts @@ -0,0 +1,87 @@ +import { toSafeHost } from "./uri"; + +export interface AuthorityParts { + agent: string | undefined; + sshHost: string; + safeHostname: string; + username: string; + workspace: string; +} + +// Prefix is a magic string that is prepended to SSH hosts to indicate that +// they should be handled by this extension. +export const AuthorityPrefix = "coder-vscode"; + +const authorityHostPrefix = `${AuthorityPrefix}.`; +const invalidAuthorityMessage = + "Invalid Coder SSH authority. Must be: ----(.)"; + +/** + * Given an authority, parse into the expected parts. + * + * The authority looks like `://ssh-remote+`, where the + * SSH host names created by this extension match the format: + * coder-vscode.----(.) + * + * If this is not a Coder authority, return null. + * + * Throw an error if a Coder authority is invalid. + */ +export function parseRemoteAuthority(authority: string): AuthorityParts | null { + const authorityParts = authority.split("+"); + const sshHost = authorityParts[1]; + if (!sshHost) { + return null; + } + + const parts = sshHost.split("--"); + if (!parts[0].startsWith(authorityHostPrefix)) { + return null; + } + + if (parts.length < 3) { + throw new Error(invalidAuthorityMessage); + } + + // Parse from the right because safe hostnames can contain "--". + const hostPrefix = parts.slice(0, -2).join("--"); + const safeHostname = hostPrefix.slice(authorityHostPrefix.length); + const username = parts[parts.length - 2]; + const workspaceAndAgent = parts[parts.length - 1]; + if (!safeHostname || !username || !workspaceAndAgent) { + throw new Error(invalidAuthorityMessage); + } + + let workspace = workspaceAndAgent; + let agent = ""; + const workspaceParts = workspaceAndAgent.split("."); + // Multiple dots are ambiguous because workspace and agent share this separator. + if (workspaceParts.length === 2) { + workspace = workspaceParts[0]; + agent = workspaceParts[1]; + if (!workspace || !agent) { + throw new Error(invalidAuthorityMessage); + } + } + + return { + agent, + sshHost, + safeHostname, + username, + workspace, + }; +} + +export function toRemoteAuthority( + baseUrl: string, + workspaceOwner: string, + workspaceName: string, + workspaceAgent: string | undefined, +): string { + let remoteAuthority = `ssh-remote+${AuthorityPrefix}.${toSafeHost(baseUrl)}--${workspaceOwner}--${workspaceName}`; + if (workspaceAgent) { + remoteAuthority += `.${workspaceAgent}`; + } + return remoteAuthority; +} diff --git a/src/util/uri.ts b/src/util/uri.ts new file mode 100644 index 000000000..669590f5b --- /dev/null +++ b/src/util/uri.ts @@ -0,0 +1,47 @@ +import url from "node:url"; +import * as vscode from "vscode"; + +/** + * Given a URL, return the host in a format that is safe to write. + */ +export function toSafeHost(rawUrl: string): string { + const u = new URL(rawUrl); + // If the host is invalid, an empty string is returned. Although, `new URL` + // should already have thrown in that case. + return url.domainToASCII(u.hostname) || u.hostname; +} + +export function removeTrailingSlashes(value: string): string { + return value.replace(/\/+$/, ""); +} + +/** Trim surrounding whitespace and strip trailing slashes from a URL. */ +export function normalizeUrl(value: string): string { + return removeTrailingSlashes(value.trim()); +} + +/** + * Return the URL for opening Coder pages in the browser. Uses the + * `coder.alternativeWebUrl` setting when configured, otherwise returns + * the connection URL unchanged. + */ +export function resolveCoderDashboardUrl(connectionUrl: string): string { + const alt = normalizeUrl( + vscode.workspace + .getConfiguration("coder") + .get("alternativeWebUrl") ?? "", + ); + return alt || connectionUrl; +} + +/** + * Open a path in the user's browser, resolved against `coder.alternativeWebUrl` + * when set, otherwise against `connectionUrl`. + */ +export function openInBrowser( + connectionUrl: string, + path: string, +): Thenable { + const base = vscode.Uri.parse(resolveCoderDashboardUrl(connectionUrl)); + return vscode.env.openExternal(vscode.Uri.joinPath(base, path)); +} diff --git a/src/webviews/tasks/tasksPanelProvider.ts b/src/webviews/tasks/tasksPanelProvider.ts index 7aad4ef36..9c465583d 100644 --- a/src/webviews/tasks/tasksPanelProvider.ts +++ b/src/webviews/tasks/tasksPanelProvider.ts @@ -25,7 +25,7 @@ import { streamBuildLogs, } from "../../api/workspace"; import { type Logger } from "../../logging/logger"; -import { openInBrowser } from "../../util"; +import { openInBrowser } from "../../util/uri"; import { vscodeProposed } from "../../vscodeProposed"; import { dispatchCommand, diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 3e2e38c48..fea30aeef 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -22,13 +22,13 @@ vi.mock(import("node:child_process"), async (importOriginal) => ({ const { spawn } = await import("node:child_process"); const featureSet: FeatureSet = { - vscodessh: true, + cliLogin: true, proxyLogDirectory: true, wildcardSSH: true, buildReason: true, cliUpdate: true, keyringAuth: true, - keyringTokenRead: true, + tokenRead: true, supportBundle: true, }; diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 2d6be51c4..48c897a2d 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -21,6 +21,7 @@ vi.mock("node:os"); const globalConfigAuth: CliAuth = { mode: "global-config", configDir: "/config/dir", + allowOverride: true, }; describe("cliConfig", () => { @@ -91,35 +92,41 @@ describe("cliConfig", () => { interface GlobalConfigCase { scenario: string; flags: string[]; + expected: string[]; } it.each([ - { - scenario: "space-separated in one item", - flags: ["-v", "--global-config /path/to/ignored"], - }, { scenario: "equals form", - flags: ["-v", "--global-config=/path/to/ignored"], + flags: ["-v", "--global-config=/custom/coderv2"], + expected: ["-v", "--global-config=/custom/coderv2"], }, { scenario: "separate items", - flags: ["-v", "--global-config", "/path/to/ignored"], + flags: ["-v", "--global-config", "/custom/coderv2"], + expected: ["-v", "--global-config", "/custom/coderv2"], }, ])( - "should filter --global-config ($scenario) in both auth modes", - ({ flags }) => { - const urlAuth: CliAuth = { - mode: "url", - url: "https://dev.coder.com", - }; + "passes user --global-config through in file mode and drops our default ($scenario)", + ({ flags, expected }) => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", flags); - expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual([ - "-v", - "--global-config", - "/config/dir", - ]); + expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual( + expected, + ); + }, + ); + + it.each([ + { scenario: "space-separated in one item", flag: "--global-config /x" }, + { scenario: "equals form", flag: "--global-config=/x" }, + ])( + "strips user --global-config in keyring (url) mode ($scenario)", + ({ flag }) => { + const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["-v", flag]); + expect(getGlobalShellFlags(config, urlAuth)).toStrictEqual([ "-v", "--url", @@ -128,6 +135,18 @@ describe("cliConfig", () => { }, ); + it("strips user --global-config (separate items) in keyring (url) mode", () => { + const urlAuth: CliAuth = { mode: "url", url: "https://dev.coder.com" }; + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["-v", "--global-config", "/x"]); + + expect(getGlobalShellFlags(config, urlAuth)).toStrictEqual([ + "-v", + "--url", + "https://dev.coder.com", + ]); + }); + it("should not filter flags with similar prefixes", () => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", ["--global-configs", "--use-keyrings"]); @@ -373,7 +392,89 @@ describe("cliConfig", () => { 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", + ); + + expect(getGlobalFlags(config, auth)).toStrictEqual([ + "--global-config", + "/custom/coderv2", + ]); + }); + + 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", + ]); + }); + + 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", + ]); + }); + + it("ignores globalFlags --global-config on deployments older than 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.30.0")); + const auth = resolveCliAuth( + config, + featureSet, + "https://dev.coder.com", + "/default/coderv2", + ); + + // 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.netcheck.test.ts b/test/unit/commands.netcheck.test.ts index 901b2091f..9704f5da0 100644 --- a/test/unit/commands.netcheck.test.ts +++ b/test/unit/commands.netcheck.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; import { Commands } from "@/commands"; -import { toSafeHost } from "@/util"; +import { toSafeHost } from "@/util/uri"; import { createTelemetryHarness } from "../mocks/telemetry"; import { createMockLogger, MockUserInteraction } from "../mocks/testHelpers"; diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 9fc2ccfbc..15b90bc31 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -1,6 +1,7 @@ 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 { @@ -26,9 +27,11 @@ vi.mock("node:child_process", () => ({ vi.mock("node:os"); -vi.mock("@/settings/cli", () => ({ - isKeyringEnabled: vi.fn().mockReturnValue(false), -})); +vi.mock("@/settings/cli", async () => { + const actual = + await vi.importActual("@/settings/cli"); + return { ...actual, isKeyringEnabled: vi.fn().mockReturnValue(false) }; +}); vi.mock("@/core/cliExec", async () => { const actual = @@ -119,23 +122,52 @@ function failingResolver(): BinaryResolver { return vi.fn().mockRejectedValue(new Error("no binary")); } -const configs = { get: vi.fn().mockReturnValue(undefined) }; +// 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) => - key === "coder.headerCommand" ? "my-header-cmd" : undefined, + 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) { + return { + get: vi.fn((key: string, defaultValue?: unknown) => + key === "coder.globalFlags" ? [`--global-config=${dir}`] : defaultValue, + ), + }; +} + const TEST_PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); -const CRED_DIR = "/mock/base/dev.coder.com"; -const URL_FILE = `${CRED_DIR}/url`; -const SESSION_FILE = `${CRED_DIR}/session`; - -function writeCredentialFiles(url: string, token: string) { - vol.mkdirSync(CRED_DIR, { recursive: true }); - memfs.writeFileSync(URL_FILE, url); - memfs.writeFileSync(SESSION_FILE, token); +// Built with path.join so it matches getGlobalConfigDir on Windows too. +const CRED_DIR = path.join("/mock/base", "dev.coder.com"); +const CUSTOM_CRED_DIR = "/custom/coderv2"; + +function credentialPaths(dir = CRED_DIR) { + return { + url: `${dir}/url`, + session: `${dir}/session`, + }; +} + +function writeCredentialFiles( + url: string, + token: string, + dir = CRED_DIR, +): void { + const paths = credentialPaths(dir); + vol.mkdirSync(dir, { recursive: true }); + memfs.writeFileSync(paths.url, url); + memfs.writeFileSync(paths.session, token); +} + +function credentialFilesExist(dir = CRED_DIR): boolean { + const paths = credentialPaths(dir); + return memfs.existsSync(paths.url) || memfs.existsSync(paths.session); } function setup(resolver?: BinaryResolver) { @@ -175,16 +207,25 @@ describe("CliCredentialManager", () => { }); describe("storeToken", () => { - it("writes files when keyring is disabled", async () => { - const { manager, sink } = setup(); + it("writes via coder login (file mode) when keyring is disabled", async () => { + stubExecFile({ stdout: "" }); + const { manager, resolver, sink } = setup(); await expect( manager.storeToken(TEST_URL, "my-token", configs), ).resolves.toBeUndefined(); - expect(execFile).not.toHaveBeenCalled(); - expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); - expect(memfs.readFileSync(SESSION_FILE, "utf8")).toBe("my-token"); + 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", @@ -219,18 +260,49 @@ describe("CliCredentialManager", () => { }); }); - it("falls back to files when CLI version too old", async () => { + 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: "" }); const { manager } = setup(); - await expect( - manager.storeToken(TEST_URL, "token", configs), - ).resolves.toBeUndefined(); + await manager.storeToken(TEST_URL, "token", configs); - expect(execFile).not.toHaveBeenCalled(); - expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); - expect(memfs.readFileSync(SESSION_FILE, "utf8")).toBe("token"); + expect(lastExecArgs().args).toEqual([ + "--global-config", + CRED_DIR, + "login", + "--use-token-as-session", + TEST_URL, + ]); }); it("throws when CLI exec fails", async () => { @@ -319,7 +391,7 @@ describe("CliCredentialManager", () => { const token = await manager.readToken(TEST_URL, configs); expect(resolver).toHaveBeenCalledWith(TEST_URL); - expect(token).toBe("my-token"); + expect(token).toEqual({ token: "my-token", source: "keyring" }); expect(lastExecArgs().args).toEqual([ "login", "token", @@ -350,17 +422,76 @@ describe("CliCredentialManager", () => { expect(execFile).not.toHaveBeenCalled(); }); - it("skips CLI when keyring is disabled", async () => { - stubExecFile({ stdout: "my-token" }); + 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" }); const { manager } = setup(); + expect( + await manager.readToken( + TEST_URL, + configWithGlobalConfig(CUSTOM_CRED_DIR), + ), + ).toEqual({ token: "custom-file-token", source: "files" }); + expect(lastExecArgs().args).toEqual([ + `--global-config=${CUSTOM_CRED_DIR}`, + "login", + "token", + "--url", + TEST_URL, + ]); + }); + + it("returns undefined for file mode on deployments older than 2.31", async () => { + vi.mocked(cliExec.version).mockResolvedValue("2.30.0"); + const { manager, resolver } = setup(); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(resolver).toHaveBeenCalledWith(TEST_URL); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("does not read when keyring token read is unsupported", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + vi.mocked(cliExec.version).mockResolvedValueOnce("2.30.0"); + const { manager, resolver } = setup(); + + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(resolver).toHaveBeenCalledWith(TEST_URL); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("returns undefined when keyring is enabled but unsupported by the CLI", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); + const { manager, resolver } = setup(); + + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(resolver).toHaveBeenCalledWith(TEST_URL); expect(execFile).not.toHaveBeenCalled(); }); it("returns undefined when CLI version too old for token read", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); - // 2.30 supports keyringAuth but not keyringTokenRead (requires 2.31+) + // 2.30 supports keyringAuth but not tokenRead (requires 2.31+) vi.mocked(cliExec.version).mockResolvedValueOnce("2.30.0"); stubExecFile({ stdout: "my-token" }); const { manager } = setup(); @@ -416,8 +547,7 @@ describe("CliCredentialManager", () => { const exec = lastExecArgs(); expect(exec.bin).toBe(TEST_BIN); expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); - expect(memfs.existsSync(URL_FILE)).toBe(false); - expect(memfs.existsSync(SESSION_FILE)).toBe(false); + expect(credentialFilesExist()).toBe(false); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { category: "keyring", @@ -427,15 +557,22 @@ describe("CliCredentialManager", () => { }); }); - it("deletes files even when keyring is disabled", async () => { + 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(execFile).not.toHaveBeenCalled(); - expect(memfs.existsSync(URL_FILE)).toBe(false); - expect(memfs.existsSync(SESSION_FILE)).toBe(false); + expect(lastExecArgs().args).toEqual([ + "--global-config", + CRED_DIR, + "logout", + "--url", + TEST_URL, + "--yes", + ]); + expect(credentialFilesExist()).toBe(false); }); it("never throws on CLI error", async () => { @@ -481,7 +618,7 @@ describe("CliCredentialManager", () => { expect(lastExecArgs().args).toContain("--header-command"); }); - it("skips keyring when CLI version too old", async () => { + 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: "" }); @@ -490,9 +627,15 @@ describe("CliCredentialManager", () => { await manager.deleteToken(TEST_URL, configs); - expect(execFile).not.toHaveBeenCalled(); - expect(memfs.existsSync(URL_FILE)).toBe(false); - expect(memfs.existsSync(SESSION_FILE)).toBe(false); + expect(lastExecArgs().args).toEqual([ + "--global-config", + CRED_DIR, + "logout", + "--url", + TEST_URL, + "--yes", + ]); + expect(credentialFilesExist()).toBe(false); }); it("passes signal through to execFile", async () => { diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index 774f6e9de..27548c417 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -182,7 +182,10 @@ describe("cliExec", () => { `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "speedtest-err", code); - const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin); + const { env } = setup( + { mode: "global-config", configDir: "/tmp", allowOverride: true }, + bin, + ); await expect( cliExec.speedtest(env, "owner/workspace", "bad"), ).rejects.toThrow("invalid argument for -t flag"); @@ -192,7 +195,10 @@ 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" }, bin); + const { env } = setup( + { mode: "global-config", configDir: "/tmp", allowOverride: true }, + bin, + ); const ac = new AbortController(); ac.abort(); await expect( @@ -224,7 +230,10 @@ describe("cliExec", () => { `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "netcheck-err", code); - const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin); + const { env } = setup( + { mode: "global-config", configDir: "/tmp", allowOverride: true }, + bin, + ); await expect(cliExec.netcheck(env)).rejects.toThrow( "You are not logged in", ); @@ -234,7 +243,10 @@ 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" }, bin); + const { env } = setup( + { mode: "global-config", configDir: "/tmp", allowOverride: true }, + bin, + ); const ac = new AbortController(); ac.abort(); await expect(cliExec.netcheck(env, ac.signal)).rejects.toMatchObject({ @@ -281,7 +293,10 @@ describe("cliExec", () => { `process.exit(1);`, ].join("\n"); const bin = await writeExecutable(tmp, "sb-err", code); - const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin); + const { env } = setup( + { mode: "global-config", configDir: "/tmp", allowOverride: true }, + bin, + ); await expect( cliExec.supportBundle(env, "owner/workspace", "/tmp/bundle.zip"), ).rejects.toThrow("workspace not found"); @@ -338,6 +353,7 @@ describe("cliExec", () => { const { configs, env } = setup({ mode: "global-config", configDir: "/cfg", + allowOverride: true, }); configs.set("coder.globalFlags", ["--verbose"]); diff --git a/test/unit/core/pathResolver.test.ts b/test/unit/core/pathResolver.test.ts index 4a602382f..b3191e6d3 100644 --- a/test/unit/core/pathResolver.test.ts +++ b/test/unit/core/pathResolver.test.ts @@ -19,6 +19,25 @@ describe("PathResolver", () => { mockConfig = new MockConfigurationProvider(); }); + describe("getGlobalConfigDir", () => { + it("uses the per-deployment global storage directory", () => { + expectPathsEqual( + pathResolver.getGlobalConfigDir("deployment"), + path.join(basePath, "deployment"), + ); + }); + + it("ignores coder.globalConfig and CODER_CONFIG_DIR (override lives in globalFlags)", () => { + vi.stubEnv("CODER_CONFIG_DIR", "/env/coderv2"); + mockConfig.set("coder.globalConfig", "/custom/coderv2"); + + expectPathsEqual( + pathResolver.getGlobalConfigDir("deployment"), + path.join(basePath, "deployment"), + ); + }); + }); + describe("getProxyLogPath", () => { const defaultLogPath = path.join(basePath, "log"); diff --git a/test/unit/featureSet.test.ts b/test/unit/featureSet.test.ts index c7bee26cb..4b6bf6b09 100644 --- a/test/unit/featureSet.test.ts +++ b/test/unit/featureSet.test.ts @@ -34,6 +34,13 @@ describe("check version support", () => { ["v2.19.0", "v2.19.1", "v2.20.0+e491217", "v5.0.4+e491217"], ); }); + it("cli login", () => { + expectFlag( + "cliLogin", + ["v0.24.0", "v0.14.0", "v0.0.1"], + ["v0.25.0", "v0.25.1", "v2.31.0", "v3.0.0"], + ); + }); it("keyring auth", () => { expectFlag( "keyringAuth", @@ -41,9 +48,9 @@ describe("check version support", () => { ["v2.29.0", "v2.29.1", "v2.30.0", "v3.0.0"], ); }); - it("keyring token read", () => { + it("token read", () => { expectFlag( - "keyringTokenRead", + "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"], ); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index b5925b94d..0a5224142 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -202,6 +202,58 @@ describe("LoginCoordinator", () => { expect(auth?.token).toBe("stored-token"); }); + it("authenticates with CLI credential token on success", async () => { + const { + mockCredentialManager, + secretsManager, + coordinator, + mockSuccessfulAuth, + } = createTestContext(); + const user = mockSuccessfulAuth(); + vi.mocked(mockCredentialManager.readToken).mockResolvedValueOnce({ + token: "cli-credential-token", + source: "files", + }); + + const result = await coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }); + + expect(result).toEqual({ + success: true, + method: "cli_token", + user, + token: "cli-credential-token", + }); + 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", + }); + }); + it("prompts for token when no stored auth exists", async () => { const { userInteraction, diff --git a/test/unit/remote/workspaceStateMachine.test.ts b/test/unit/remote/workspaceStateMachine.test.ts index fab7d1472..cf9b55538 100644 --- a/test/unit/remote/workspaceStateMachine.test.ts +++ b/test/unit/remote/workspaceStateMachine.test.ts @@ -42,7 +42,7 @@ import type { CoderApi } from "@/api/coderApi"; import type { StartupMode } from "@/core/mementoManager"; import type { FeatureSet } from "@/featureSet"; import type { TelemetryService } from "@/telemetry/service"; -import type { AuthorityParts } from "@/util"; +import type { AuthorityParts } from "@/util/authority"; vi.mock("@/api/workspace", async (importActual) => { const { LazyStream } = await importActual(); diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 952ee35af..d9e912816 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -1,190 +1,14 @@ import os from "node:os"; import { afterEach, beforeEach, describe, it, expect, vi } from "vitest"; -import * as vscode from "vscode"; import { - type AuthorityParts, countSubstring, escapeCommandArg, escapeShellArg, expandPath, findPort, - openInBrowser, - parseRemoteAuthority, - resolveUiUrl, - toRemoteAuthority, - toSafeHost, } from "@/util"; -import { MockConfigurationProvider } from "../mocks/testHelpers"; - -describe("parseRemoteAuthority", () => { - const remoteAuthority = (sshHost: string) => `vscode://ssh-remote+${sshHost}`; - - it.each([ - { label: "missing SSH host", input: "vscode://ssh-remote" }, - { label: "empty SSH host", input: "vscode://ssh-remote+" }, - { - label: "non-Coder host", - input: remoteAuthority("some-unrelated-host.com"), - }, - { - label: "prefix without safeHostname separator", - input: remoteAuthority("coder-vscode--foo--bar"), - }, - { - label: "similar prefix", - input: remoteAuthority("coder-vscode-test--foo--bar"), - }, - { label: "wrong prefix", input: remoteAuthority("coder--foo--bar") }, - ])("ignores unrelated authority: $label", ({ input }) => { - expect(parseRemoteAuthority(input)).toBe(null); - }); - - it.each([ - { - label: "missing username and workspace", - sshHost: "coder-vscode.dev.coder.com", - }, - { - label: "missing workspace", - sshHost: "coder-vscode.dev.coder.com--foo", - }, - { - label: "manual host using Coder prefix", - sshHost: "coder-vscode.personal-host", - }, - { - label: "empty username", - sshHost: "coder-vscode.dev.coder.com----bar", - }, - { - label: "empty workspace", - sshHost: "coder-vscode.dev.coder.com--foo--", - }, - { - label: "empty hostname", - sshHost: "coder-vscode.--foo--bar", - }, - { - label: "empty trailing segment", - sshHost: "coder-vscode.dev.coder.com--foo--bar--", - }, - { - label: "empty workspace before agent separator", - sshHost: "coder-vscode.dev.coder.com--foo--.agent", - }, - { - label: "empty agent after separator", - sshHost: "coder-vscode.dev.coder.com--foo--bar.", - }, - ])("rejects invalid authority: $label", ({ sshHost }) => { - expect(() => parseRemoteAuthority(remoteAuthority(sshHost))).toThrow( - "Invalid Coder SSH authority", - ); - }); - - interface ParseCase { - label: string; - sshHost: string; - safeHostname: string; - workspace: string; - agent?: string; - username?: string; - } - - it("round trips generated remote authorities", () => { - const authority = toRemoteAuthority( - "https://ほげ", - "alice", - "workspace", - "main", - ); - - expect(authority).toBe( - "ssh-remote+coder-vscode.xn--18j4d--alice--workspace.main", - ); - expect(parseRemoteAuthority(authority)).toStrictEqual({ - agent: "main", - sshHost: "coder-vscode.xn--18j4d--alice--workspace.main", - safeHostname: "xn--18j4d", - username: "alice", - workspace: "workspace", - } satisfies AuthorityParts); - }); - - it.each([ - { - label: "hostname without agent", - sshHost: "coder-vscode.dev.coder.com--foo--bar", - safeHostname: "dev.coder.com", - workspace: "bar", - }, - { - label: "hostname with agent", - sshHost: "coder-vscode.dev.coder.com--foo--bar.baz", - safeHostname: "dev.coder.com", - workspace: "bar", - agent: "baz", - }, - { - label: "hostname containing delimiter", - sshHost: "coder-vscode.test--domain.com--foo--bar", - safeHostname: "test--domain.com", - workspace: "bar", - }, - { - label: "Punycode hostname containing delimiter", - sshHost: "coder-vscode.xn--test---8o4.example--foo--bar", - safeHostname: "xn--test---8o4.example", - workspace: "bar", - }, - { - label: "hostname with repeated delimiters and agent", - sshHost: "coder-vscode.first--middle--last.example--foo--bar.baz", - safeHostname: "first--middle--last.example", - workspace: "bar", - agent: "baz", - }, - { - label: "hostname with many consecutive dashes", - sshHost: "coder-vscode.foo---------------bar.com--foo--bar", - safeHostname: "foo---------------bar.com", - workspace: "bar", - }, - { - label: "ambiguous workspace/agent separator", - sshHost: "coder-vscode.dev.coder.com--foo--bar.baz.qux", - safeHostname: "dev.coder.com", - workspace: "bar.baz.qux", - }, - ])( - "parses $label", - ({ sshHost, safeHostname, workspace, agent, username }) => { - expect(parseRemoteAuthority(remoteAuthority(sshHost))).toStrictEqual({ - agent: agent ?? "", - sshHost, - safeHostname, - username: username ?? "foo", - workspace, - } satisfies AuthorityParts); - }, - ); -}); - -describe("toSafeHost", () => { - it("escapes url host", () => { - expect(toSafeHost("https://foobar:8080")).toBe("foobar"); - expect(toSafeHost("https://ほげ")).toBe("xn--18j4d"); - expect(toSafeHost("https://test.😉.invalid")).toBe("test.xn--n28h.invalid"); - expect(toSafeHost("https://dev.😉-coder.com")).toBe( - "dev.xn---coder-vx74e.com", - ); - expect(() => toSafeHost("invalid url")).toThrow("Invalid URL"); - expect(toSafeHost("http://ignore-port.com:8080")).toBe("ignore-port.com"); - }); -}); - describe("countSubstring", () => { it("handles empty strings", () => { expect(countSubstring("", "")).toBe(0); @@ -427,93 +251,3 @@ describe("findPort", () => { expect(findPort(log)).toBe(3333); }); }); - -describe("resolveUiUrl", () => { - let configurationProvider: MockConfigurationProvider; - - beforeEach(() => { - configurationProvider = new MockConfigurationProvider(); - }); - - it("returns the connection URL when no alternative is configured", () => { - expect(resolveUiUrl("https://coder.example.com:7004")).toBe( - "https://coder.example.com:7004", - ); - }); - - it.each([ - { name: "empty", value: "" }, - { name: "whitespace", value: " " }, - ])( - "returns the connection URL when the alternative is $name", - ({ value }) => { - configurationProvider.set("coder.alternativeWebUrl", value); - expect(resolveUiUrl("https://coder.example.com:7004")).toBe( - "https://coder.example.com:7004", - ); - }, - ); - - it.each([ - { - name: "uses the alternative URL when configured", - value: "https://coder.example.com", - }, - { name: "strips trailing slashes", value: "https://coder.example.com/" }, - { - name: "strips multiple trailing slashes", - value: "https://coder.example.com///", - }, - { name: "trims whitespace", value: " https://coder.example.com " }, - ])("$name", ({ value }) => { - configurationProvider.set("coder.alternativeWebUrl", value); - expect(resolveUiUrl("https://coder.example.com:7004")).toBe( - "https://coder.example.com", - ); - }); -}); - -describe("openInBrowser", () => { - let configurationProvider: MockConfigurationProvider; - - beforeEach(() => { - configurationProvider = new MockConfigurationProvider(); - vi.mocked(vscode.env.openExternal).mockClear(); - }); - - it("opens the connection URL joined with the path when no alt URL is set", () => { - openInBrowser("https://coder.example.com:7004", "/templates"); - expect(vscode.env.openExternal).toHaveBeenCalledWith( - vscode.Uri.parse("https://coder.example.com:7004/templates"), - ); - }); - - it("opens the alternative URL when configured", () => { - configurationProvider.set( - "coder.alternativeWebUrl", - "https://coder.example.com", - ); - openInBrowser("https://coder.example.com:7004", "/templates"); - expect(vscode.env.openExternal).toHaveBeenCalledWith( - vscode.Uri.parse("https://coder.example.com/templates"), - ); - }); - - it("preserves a path prefix on the alternative URL", () => { - configurationProvider.set( - "coder.alternativeWebUrl", - "https://proxy.example.com/coder", - ); - openInBrowser("https://coder.example.com:7004", "/templates"); - expect(vscode.env.openExternal).toHaveBeenCalledWith( - vscode.Uri.parse("https://proxy.example.com/coder/templates"), - ); - }); - - it("joins paths without a leading slash", () => { - openInBrowser("https://coder.example.com", "templates"); - expect(vscode.env.openExternal).toHaveBeenCalledWith( - vscode.Uri.parse("https://coder.example.com/templates"), - ); - }); -}); diff --git a/test/unit/util/authority.test.ts b/test/unit/util/authority.test.ts new file mode 100644 index 000000000..8b0dd5f2d --- /dev/null +++ b/test/unit/util/authority.test.ts @@ -0,0 +1,234 @@ +import { describe, expect, it } from "vitest"; + +import { + type AuthorityParts, + parseRemoteAuthority, + toRemoteAuthority, +} from "@/util/authority"; + +describe("parseRemoteAuthority", () => { + const remoteAuthority = (sshHost: string) => `vscode://ssh-remote+${sshHost}`; + + it.each([ + { label: "missing SSH host", input: "vscode://ssh-remote" }, + { label: "empty SSH host", input: "vscode://ssh-remote+" }, + { + label: "non-Coder host", + input: remoteAuthority("some-unrelated-host.com"), + }, + { + label: "prefix without safeHostname separator", + input: remoteAuthority("coder-vscode--foo--bar"), + }, + { + label: "similar prefix", + input: remoteAuthority("coder-vscode-test--foo--bar"), + }, + { label: "wrong prefix", input: remoteAuthority("coder--foo--bar") }, + ])("ignores unrelated authority: $label", ({ input }) => { + expect(parseRemoteAuthority(input)).toBe(null); + }); + + it.each([ + { + label: "missing username and workspace", + sshHost: "coder-vscode.dev.coder.com", + }, + { + label: "missing workspace", + sshHost: "coder-vscode.dev.coder.com--foo", + }, + { + label: "manual host using Coder prefix", + sshHost: "coder-vscode.personal-host", + }, + { + label: "empty username", + sshHost: "coder-vscode.dev.coder.com----bar", + }, + { + label: "empty workspace", + sshHost: "coder-vscode.dev.coder.com--foo--", + }, + { + label: "empty hostname", + sshHost: "coder-vscode.--foo--bar", + }, + { + label: "empty trailing segment", + sshHost: "coder-vscode.dev.coder.com--foo--bar--", + }, + { + label: "empty workspace before agent separator", + sshHost: "coder-vscode.dev.coder.com--foo--.agent", + }, + { + label: "empty agent after separator", + sshHost: "coder-vscode.dev.coder.com--foo--bar.", + }, + ])("rejects invalid authority: $label", ({ sshHost }) => { + expect(() => parseRemoteAuthority(remoteAuthority(sshHost))).toThrow( + "Invalid Coder SSH authority", + ); + }); + + interface ParseCase { + label: string; + sshHost: string; + safeHostname: string; + workspace: string; + agent?: string; + username?: string; + } + + it("round trips generated remote authorities", () => { + const authority = toRemoteAuthority( + "https://ほげ", + "alice", + "workspace", + "main", + ); + + expect(authority).toBe( + "ssh-remote+coder-vscode.xn--18j4d--alice--workspace.main", + ); + expect(parseRemoteAuthority(authority)).toStrictEqual({ + agent: "main", + sshHost: "coder-vscode.xn--18j4d--alice--workspace.main", + safeHostname: "xn--18j4d", + username: "alice", + workspace: "workspace", + } satisfies AuthorityParts); + }); + + it.each([ + { + label: "hostname without agent", + sshHost: "coder-vscode.dev.coder.com--foo--bar", + safeHostname: "dev.coder.com", + workspace: "bar", + }, + { + label: "hostname with agent", + sshHost: "coder-vscode.dev.coder.com--foo--bar.baz", + safeHostname: "dev.coder.com", + workspace: "bar", + agent: "baz", + }, + { + label: "hostname containing delimiter", + sshHost: "coder-vscode.test--domain.com--foo--bar", + safeHostname: "test--domain.com", + workspace: "bar", + }, + { + label: "Punycode hostname containing delimiter", + sshHost: "coder-vscode.xn--test---8o4.example--foo--bar", + safeHostname: "xn--test---8o4.example", + workspace: "bar", + }, + { + label: "hostname with repeated delimiters and agent", + sshHost: "coder-vscode.first--middle--last.example--foo--bar.baz", + safeHostname: "first--middle--last.example", + workspace: "bar", + agent: "baz", + }, + { + label: "hostname with many consecutive dashes", + sshHost: "coder-vscode.foo---------------bar.com--foo--bar", + safeHostname: "foo---------------bar.com", + workspace: "bar", + }, + { + label: "ambiguous workspace/agent separator", + sshHost: "coder-vscode.dev.coder.com--foo--bar.baz.qux", + safeHostname: "dev.coder.com", + workspace: "bar.baz.qux", + }, + ])( + "parses $label", + ({ sshHost, safeHostname, workspace, agent, username }) => { + expect(parseRemoteAuthority(remoteAuthority(sshHost))).toStrictEqual({ + agent: agent ?? "", + sshHost, + safeHostname, + username: username ?? "foo", + workspace, + } satisfies AuthorityParts); + }, + ); +}); + +describe("toRemoteAuthority", () => { + interface ToRemoteAuthorityCase { + url: string; + owner: string; + workspace: string; + agent: string | undefined; + expected: string; + } + it.each([ + { + url: "https://dev.coder.com", + owner: "foo", + workspace: "bar", + agent: undefined, + expected: "ssh-remote+coder-vscode.dev.coder.com--foo--bar", + }, + { + url: "http://dev.coder.com:3000", + owner: "foo", + workspace: "bar", + agent: "baz", + expected: "ssh-remote+coder-vscode.dev.coder.com--foo--bar.baz", + }, + { + url: "https://coder.example.com/some/path?q=1", + owner: "alice", + workspace: "web", + agent: "", + expected: "ssh-remote+coder-vscode.coder.example.com--alice--web", + }, + { + url: "http://192.168.1.5:8080", + owner: "foo", + workspace: "bar", + agent: undefined, + expected: "ssh-remote+coder-vscode.192.168.1.5--foo--bar", + }, + { + url: "http://localhost:3000", + owner: "dev", + workspace: "ws", + agent: "main", + expected: "ssh-remote+coder-vscode.localhost--dev--ws.main", + }, + { + url: "https://sub.DOMAIN.Example.COM", + owner: "foo", + workspace: "bar", + agent: undefined, + expected: "ssh-remote+coder-vscode.sub.domain.example.com--foo--bar", + }, + { + url: "https://ほげ:8080", + owner: "foo", + workspace: "bar", + agent: undefined, + expected: "ssh-remote+coder-vscode.xn--18j4d--foo--bar", + }, + { + url: "https://عربي", + owner: "foo", + workspace: "bar", + agent: undefined, + expected: "ssh-remote+coder-vscode.xn--ngbrx4e--foo--bar", + }, + ])( + "builds authority for $url", + ({ url, owner, workspace, agent, expected }) => { + expect(toRemoteAuthority(url, owner, workspace, agent)).toBe(expected); + }, + ); +}); diff --git a/test/unit/util/uri.test.ts b/test/unit/util/uri.test.ts new file mode 100644 index 000000000..6c326b487 --- /dev/null +++ b/test/unit/util/uri.test.ts @@ -0,0 +1,142 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +import { + normalizeUrl, + openInBrowser, + removeTrailingSlashes, + resolveCoderDashboardUrl, + toSafeHost, +} from "@/util/uri"; + +import { MockConfigurationProvider } from "../../mocks/testHelpers"; + +describe("toSafeHost", () => { + it.each([ + ["https://foobar:8080", "foobar"], + ["https://ほげ", "xn--18j4d"], + ["https://عربي", "xn--ngbrx4e"], + ["https://test.😉.invalid", "test.xn--n28h.invalid"], + ["https://dev.😉-coder.com", "dev.xn---coder-vx74e.com"], + ["http://ignore-port.com:8080", "ignore-port.com"], + ])("returns %s for %s", (input, expected) => { + expect(toSafeHost(input)).toBe(expected); + }); + + it("throws for invalid URLs", () => { + expect(() => toSafeHost("invalid url")).toThrow("Invalid URL"); + }); +}); + +describe("removeTrailingSlashes", () => { + it.each([ + ["https://coder.example.com", "https://coder.example.com"], + ["https://coder.example.com/", "https://coder.example.com"], + ["https://coder.example.com///", "https://coder.example.com"], + ["///", ""], + ])("returns %j for %j", (input, expected) => { + expect(removeTrailingSlashes(input)).toBe(expected); + }); +}); + +describe("normalizeUrl", () => { + it.each([ + ["https://coder.example.com", "https://coder.example.com"], + [" https://coder.example.com ", "https://coder.example.com"], + ["https://coder.example.com///", "https://coder.example.com"], + [" https://coder.example.com/ ", "https://coder.example.com"], + ["", ""], + ])("returns %j for %j", (input, expected) => { + expect(normalizeUrl(input)).toBe(expected); + }); +}); + +describe("resolveCoderDashboardUrl", () => { + let configurationProvider: MockConfigurationProvider; + + beforeEach(() => { + configurationProvider = new MockConfigurationProvider(); + }); + + it("returns the connection URL when no alternative is configured", () => { + expect(resolveCoderDashboardUrl("https://coder.example.com:7004")).toBe( + "https://coder.example.com:7004", + ); + }); + + it.each([ + { name: "empty", value: "" }, + { name: "whitespace", value: " " }, + ])( + "returns the connection URL when the alternative is $name", + ({ value }) => { + configurationProvider.set("coder.alternativeWebUrl", value); + expect(resolveCoderDashboardUrl("https://coder.example.com:7004")).toBe( + "https://coder.example.com:7004", + ); + }, + ); + + it.each([ + { + name: "uses the alternative URL when configured", + value: "https://coder.example.com", + }, + { name: "strips trailing slashes", value: "https://coder.example.com/" }, + { + name: "strips multiple trailing slashes", + value: "https://coder.example.com///", + }, + { name: "trims whitespace", value: " https://coder.example.com " }, + ])("$name", ({ value }) => { + configurationProvider.set("coder.alternativeWebUrl", value); + expect(resolveCoderDashboardUrl("https://coder.example.com:7004")).toBe( + "https://coder.example.com", + ); + }); +}); + +describe("openInBrowser", () => { + let configurationProvider: MockConfigurationProvider; + + beforeEach(() => { + configurationProvider = new MockConfigurationProvider(); + vi.mocked(vscode.env.openExternal).mockClear(); + }); + + it("opens the connection URL joined with the path when no alt URL is set", () => { + openInBrowser("https://coder.example.com:7004", "/templates"); + expect(vscode.env.openExternal).toHaveBeenCalledWith( + vscode.Uri.parse("https://coder.example.com:7004/templates"), + ); + }); + + it("opens the alternative URL when configured", () => { + configurationProvider.set( + "coder.alternativeWebUrl", + "https://coder.example.com", + ); + openInBrowser("https://coder.example.com:7004", "/templates"); + expect(vscode.env.openExternal).toHaveBeenCalledWith( + vscode.Uri.parse("https://coder.example.com/templates"), + ); + }); + + it("preserves a path prefix on the alternative URL", () => { + configurationProvider.set( + "coder.alternativeWebUrl", + "https://proxy.example.com/coder", + ); + openInBrowser("https://coder.example.com:7004", "/templates"); + expect(vscode.env.openExternal).toHaveBeenCalledWith( + vscode.Uri.parse("https://proxy.example.com/coder/templates"), + ); + }); + + it("joins paths without a leading slash", () => { + openInBrowser("https://coder.example.com", "templates"); + expect(vscode.env.openExternal).toHaveBeenCalledWith( + vscode.Uri.parse("https://coder.example.com/templates"), + ); + }); +});