diff --git a/src/commands.ts b/src/commands.ts index 1a219148f..8c34340b3 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -14,6 +14,12 @@ import * as cliExec from "./core/cliExec"; import { CertificateError } from "./error/certificateError"; import { toError } from "./error/errorUtils"; import { type FeatureSet, featureSetForVersion } from "./featureSet"; +import { + AuthTelemetry, + type AuthLoginOutcome, + type AuthLoginSource, + type AuthLogoutOutcome, +} from "./instrumentation/auth"; import { reportElapsedProgress, withCancellableProgress, @@ -37,6 +43,7 @@ import { } from "./workspace/workspacesProvider"; import type { + User, Workspace, WorkspaceAgent, } from "coder/site/src/api/typesGenerated"; @@ -48,8 +55,10 @@ import type { MementoManager } from "./core/mementoManager"; import type { PathResolver } from "./core/pathResolver"; import type { SecretsManager } from "./core/secretsManager"; import type { DeploymentManager } from "./deployment/deploymentManager"; +import type { Deployment } from "./deployment/types"; +import type { CredentialFailureCategory } from "./instrumentation/credentials"; import type { Logger } from "./logging/logger"; -import type { LoginCoordinator } from "./login/loginCoordinator"; +import type { LoginCoordinator, LoginMethod } from "./login/loginCoordinator"; import type { TelemetryService } from "./telemetry/service"; import type { SpeedtestPanelFactory } from "./webviews/speedtest/speedtestPanelFactory"; import type { @@ -68,6 +77,16 @@ interface OpenOptions { useDefaultDirectory?: boolean; } +interface LoginArgs { + readonly url?: string; + readonly autoLogin?: boolean; +} + +interface LoginSuccess { + readonly user: User; + readonly token: string; +} + const openDefaults = { openRecent: false, useDefaultDirectory: true, @@ -83,6 +102,7 @@ export class Commands { private readonly duplicateWorkspaceIpc: DuplicateWorkspaceIpc; private readonly speedtestPanelFactory: SpeedtestPanelFactory; private readonly telemetryService: TelemetryService; + private readonly authTelemetry: AuthTelemetry; // These will only be populated when actively connected to a workspace and are // used in commands. Because commands can be executed by the user, it is not @@ -109,6 +129,7 @@ export class Commands { this.loginCoordinator = serviceContainer.getLoginCoordinator(); this.duplicateWorkspaceIpc = serviceContainer.getDuplicateWorkspaceIpc(); this.speedtestPanelFactory = serviceContainer.getSpeedtestPanelFactory(); + this.authTelemetry = new AuthTelemetry(this.telemetryService); } /** @@ -137,20 +158,31 @@ export class Commands { * Log into a deployment. If already authenticated, this is a no-op. * If no URL is provided, shows a menu of recent URLs plus defaults. */ - public async login(args?: { - url?: string; - autoLogin?: boolean; - }): Promise { + public async login(args?: LoginArgs): Promise { if (this.deploymentManager.isAuthenticated()) { return; } - await this.performLogin(args); + await this.traceLoginCommand( + args?.autoLogin ? "auto_login" : "command", + (setMethod) => this.performLogin(args, setMethod), + ); } - private async performLogin(args?: { - url?: string; - autoLogin?: boolean; - }): Promise { + private async traceLoginCommand( + source: AuthLoginSource, + run: ( + setMethod: (method: LoginMethod) => void, + ) => Promise, + ): Promise { + await this.authTelemetry.traceLogin(source, (trace) => + run((method) => trace.setMethod(method)), + ); + } + + private async performLogin( + args: LoginArgs | undefined, + setMethod: (method: LoginMethod) => void, + ): Promise { this.logger.debug("Logging in"); const currentDeployment = await this.secretsManager.getCurrentDeployment(); @@ -160,7 +192,7 @@ export class Commands { currentDeployment?.url, ); if (!url) { - return; // The user aborted. + return { success: false, reason: "no_url_provided" }; } const safeHostname = toSafeHost(url); @@ -173,9 +205,22 @@ export class Commands { }); if (!result.success) { - return; + if (result.method) { + setMethod(result.method); + } + return result; } + setMethod(result.method); + await this.completeLogin(url, safeHostname, result); + return { success: true, method: result.method }; + } + + private async completeLogin( + url: string, + safeHostname: string, + result: LoginSuccess, + ): Promise { await this.deploymentManager.setDeployment({ url, safeHostname, @@ -183,9 +228,14 @@ export class Commands { user: result.user, }); + this.showWelcomeMessage(result.user.username); + this.logger.debug("Login complete to deployment:", url); + } + + private showWelcomeMessage(username: string): void { vscode.window .showInformationMessage( - `Welcome to Coder, ${result.user.username}!`, + `Welcome to Coder, ${username}!`, { detail: "You can now use the Coder extension to manage your Coder instance.", @@ -197,7 +247,6 @@ export class Commands { vscode.commands.executeCommand("coder.open"); } }); - this.logger.debug("Login complete to deployment:", url); } /** @@ -418,21 +467,37 @@ export class Commands { * Log out and clear stored credentials, requiring re-authentication on next login. */ public async logout(): Promise { + await this.authTelemetry.traceLogout(() => this.performLogout()); + } + + private async performLogout(): Promise { if (!this.deploymentManager.isAuthenticated()) { - return; + return { success: false, reason: "not_authenticated" }; } this.logger.debug("Logging out"); const deployment = this.deploymentManager.getCurrentDeployment(); + await this.deploymentManager.clearDeployment("logout"); - await this.deploymentManager.clearDeployment(); + const credentialFailureCategory = deployment + ? await this.clearDeploymentCredentials(deployment) + : undefined; - if (deployment) { - await this.cliManager.clearCredentials(deployment.url); - await this.secretsManager.clearAllAuthData(deployment.safeHostname); - } + this.showLogoutMessage(); + this.logger.debug("Logout complete"); + return logoutResultForCredentialFailure(credentialFailureCategory); + } + private async clearDeploymentCredentials( + deployment: Deployment, + ): Promise { + const result = await this.cliManager.clearCredentials(deployment.url); + await this.secretsManager.clearAllAuthData(deployment.safeHostname); + return result.failureCategory; + } + + private showLogoutMessage(): void { vscode.window .showInformationMessage("You've been logged out of Coder!", "Login") .then((action) => { @@ -442,8 +507,6 @@ export class Commands { }); } }); - - this.logger.debug("Logout complete"); } /** @@ -452,7 +515,9 @@ export class Commands { */ public async switchDeployment(): Promise { this.logger.debug("Switching deployment"); - await this.performLogin(); + await this.traceLoginCommand("switch_deployment", (setMethod) => + this.performLogin(undefined, setMethod), + ); } /** @@ -1218,6 +1283,18 @@ export class Commands { } } +function logoutResultForCredentialFailure( + failureCategory: CredentialFailureCategory | undefined, +): AuthLogoutOutcome { + if (failureCategory === "aborted") { + return { success: false, reason: "credential_clear_cancelled" }; + } + if (failureCategory) { + return { success: false, reason: "credential_clear_failed" }; + } + return { success: true }; +} + async function openFile(filePath: string): Promise { const uri = vscode.Uri.file(filePath); await vscode.window.showTextDocument(uri); diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index c338c6387..7553132e0 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -7,8 +7,18 @@ import * as semver from "semver"; import { isAbortError } from "../error/errorUtils"; import { featureSetForVersion } from "../featureSet"; +import { + CredentialCliError, + CredentialFileError, + CredentialTelemetry, + type CredentialClearResult, +} from "../instrumentation/credentials"; import { isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; +import { + NOOP_TELEMETRY_REPORTER, + type TelemetryReporter, +} from "../telemetry/reporter"; import { toSafeHost } from "../util"; import { writeAtomically } from "../util/fs"; @@ -46,11 +56,22 @@ export function isKeyringSupported(): boolean { * persistence: keyring-backed (via CLI) and file-based (plaintext). */ export class CliCredentialManager { + readonly #logger: Logger; + readonly #resolveBinary: BinaryResolver; + readonly #pathResolver: PathResolver; + readonly #credentialTelemetry: CredentialTelemetry; + constructor( - private readonly logger: Logger, - private readonly resolveBinary: BinaryResolver, - private readonly pathResolver: PathResolver, - ) {} + logger: Logger, + resolveBinary: BinaryResolver, + pathResolver: PathResolver, + telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, + ) { + this.#logger = logger; + this.#resolveBinary = resolveBinary; + this.#pathResolver = pathResolver; + this.#credentialTelemetry = new CredentialTelemetry(telemetry); + } /** * Store credentials for a deployment URL. Uses the OS keyring when the @@ -59,22 +80,36 @@ export class CliCredentialManager { * * Keyring and files are mutually exclusive, never both. */ - public async storeToken( + public storeToken( url: string, token: string, configs: Pick, options?: { signal?: AbortSignal }, ): Promise { - const binPath = await this.resolveKeyringBinary( - url, - configs, - "keyringAuth", - ); - if (!binPath) { - await this.writeCredentialFiles(url, token); - return; - } + return this.#credentialTelemetry.traceStore(configs, async (trace) => { + const binPath = await this.#resolveKeyringBinary( + url, + configs, + "keyringAuth", + ); + if (!binPath) { + await trace.file(() => this.#writeCredentialFiles(url, token)); + return; + } + + await trace.keyring(() => + this.#storeKeyringToken(binPath, url, token, configs, options), + ); + }); + } + async #storeKeyringToken( + binPath: string, + url: string, + token: string, + configs: Pick, + options?: { signal?: AbortSignal }, + ): Promise { const args = [ ...getHeaderArgs(configs), "login", @@ -82,14 +117,17 @@ export class CliCredentialManager { url, ]; try { - await this.execWithTimeout(binPath, args, { + await this.#execWithTimeout(binPath, args, { env: { ...process.env, CODER_SESSION_TOKEN: token }, signal: options?.signal, }); - this.logger.info("Stored token via CLI for", url); + this.#logger.info("Stored token via CLI for", url); } catch (error) { - this.logger.warn("Failed to store token via CLI:", error); - throw error; + this.#logger.warn("Failed to store token via CLI:", error); + if (isAbortError(error)) { + throw error; + } + throw new CredentialCliError(error); } } @@ -105,13 +143,13 @@ export class CliCredentialManager { ): Promise { let binPath: string | undefined; try { - binPath = await this.resolveKeyringBinary( + binPath = await this.#resolveKeyringBinary( url, configs, "keyringTokenRead", ); } catch (error) { - this.logger.warn("Could not resolve CLI binary for token read:", error); + this.#logger.warn("Could not resolve CLI binary for token read:", error); return undefined; } if (!binPath) { @@ -120,7 +158,7 @@ export class CliCredentialManager { const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; try { - const { stdout } = await this.execWithTimeout(binPath, args, { + const { stdout } = await this.#execWithTimeout(binPath, args, { signal: options?.signal, }); const token = stdout.trim(); @@ -129,25 +167,47 @@ export class CliCredentialManager { if (isAbortError(error)) { throw error; } - this.logger.warn("Failed to read token via CLI:", error); + this.#logger.warn("Failed to read token via CLI:", error); return undefined; } } /** - * 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. File deletion is best-effort. + * Keyring deletion returns a bounded failure category when attempted and + * unsuccessful. Throws AbortError when the signal is aborted. */ - public async deleteToken( + public deleteToken( url: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - await Promise.all([ - this.deleteCredentialFiles(url), - this.deleteKeyringToken(url, configs, options?.signal), - ]); + ): Promise { + return this.#credentialTelemetry.traceClear(configs, async (trace) => { + await trace.file(() => this.#deleteCredentialFiles(url)); + + 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, + ); + await trace.keyring(() => Promise.resolve()); + return { failureCategory: "binary" }; + } + if (!binPath) { + return {}; + } + + const result = await trace.keyring(() => + this.#deleteKeyringToken(binPath, url, configs, options?.signal), + ); + if (result.failureCategory) { + return { failureCategory: result.failureCategory }; + } + return {}; + }); } /** @@ -158,7 +218,7 @@ export class CliCredentialManager { * Throws on binary resolution or version-check failure (caller decides * whether to catch or propagate). */ - private async resolveKeyringBinary( + async #resolveKeyringBinary( url: string, configs: Pick, feature: KeyringFeature, @@ -166,7 +226,7 @@ export class CliCredentialManager { if (!isKeyringEnabled(configs)) { return undefined; } - const binPath = await this.resolveBinary(url); + const binPath = await this.#resolveBinary(url); const cliVersion = semver.parse(await version(binPath)); return featureSetForVersion(cliVersion)[feature] ? binPath : undefined; } @@ -174,14 +234,14 @@ export class CliCredentialManager { /** * Wrap execFileAsync with a 60s timeout and periodic debug logging. */ - private async execWithTimeout( + async #execWithTimeout( binPath: string, args: string[], 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]} ...`); + this.#logger.debug(`CLI command still running: coder ${args[0]} ...`); }, EXEC_LOG_INTERVAL_MS); try { return await execFileAsync(binPath, args, { @@ -197,33 +257,34 @@ export class CliCredentialManager { /** * Write URL and token files under --global-config. */ - private async writeCredentialFiles( - url: string, - token: string, - ): Promise { - const safeHostname = toSafeHost(url); - await Promise.all([ - this.atomicWriteFile(this.pathResolver.getUrlPath(safeHostname), url), - this.atomicWriteFile( - this.pathResolver.getSessionTokenPath(safeHostname), - token, - ), - ]); + 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. */ - private async deleteCredentialFiles(url: string): Promise { + async #deleteCredentialFiles(url: string): Promise { const safeHostname = toSafeHost(url); const paths = [ - this.pathResolver.getSessionTokenPath(safeHostname), - this.pathResolver.getUrlPath(safeHostname), + this.#pathResolver.getSessionTokenPath(safeHostname), + this.#pathResolver.getUrlPath(safeHostname), ]; await Promise.all( paths.map((p) => fs.rm(p, { force: true }).catch((error) => { - this.logger.warn("Failed to remove credential file", p, error); + this.#logger.warn("Failed to remove credential file", p, error); }), ), ); @@ -232,45 +293,34 @@ export class CliCredentialManager { /** * Delete keyring token via `coder logout`. Best-effort: never throws. */ - private async deleteKeyringToken( + async #deleteKeyringToken( + binPath: string, url: string, configs: Pick, signal?: AbortSignal, - ): 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); - return; - } - if (!binPath) { - return; - } - + ): Promise { const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"]; try { - await this.execWithTimeout(binPath, args, { signal }); - this.logger.info("Deleted token via CLI for", url); + await this.#execWithTimeout(binPath, args, { signal }); + this.#logger.info("Deleted token via CLI for", url); + return {}; } catch (error) { if (isAbortError(error)) { throw error; } - this.logger.warn("Failed to delete token via CLI:", error); + this.#logger.warn("Failed to delete token via CLI:", error); + return { failureCategory: "cli" }; } } /** Atomically write content to a file. */ - private async atomicWriteFile( - filePath: string, - content: string, - ): Promise { + 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), + this.#logger.warn("Failed to delete temp file", tempPath, rmErr), ); } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index b65f0f0a6..2031b2faf 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,6 +10,10 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; +import { + categorizeCredentialError, + type CredentialClearResult, +} from "../instrumentation/credentials"; import * as pgp from "../pgp"; import { withCancellableProgress, withOptionalProgress } from "../progress"; import { isKeyringEnabled } from "../settings/cli"; @@ -910,7 +914,7 @@ export class CliManager { * Remove credentials for a deployment. Clears both file-based credentials * and keyring entries (via `coder logout`). All cleanup is best-effort. */ - public async clearCredentials(url: string): Promise { + public async clearCredentials(url: string): Promise { const configs = vscode.workspace.getConfiguration(); const result = await withOptionalProgress( ({ signal }) => @@ -923,13 +927,14 @@ export class CliManager { }, ); if (result.ok) { - return; + return result.value; } if (result.cancelled) { this.output.info("Credential removal cancelled by user"); - } else { - this.output.warn("Failed to remove credentials:", result.error); + return { failureCategory: "aborted" }; } + this.output.warn("Failed to remove credentials:", result.error); + return { failureCategory: categorizeCredentialError(result.error) }; } private handleStoreError(error: unknown): void { diff --git a/src/core/container.ts b/src/core/container.ts index 858686924..b18b54d87 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -90,6 +90,7 @@ export class ServiceContainer implements vscode.Disposable { } }, this.pathResolver, + this.telemetryService, ); this.cliManager = new CliManager( this.logger, diff --git a/src/deployment/deploymentManager.ts b/src/deployment/deploymentManager.ts index 5868ecb91..a97934871 100644 --- a/src/deployment/deploymentManager.ts +++ b/src/deployment/deploymentManager.ts @@ -7,6 +7,11 @@ import { type ServiceContainer } from "../core/container"; import { type ContextManager } from "../core/contextManager"; import { type MementoManager } from "../core/mementoManager"; import { type SecretsManager } from "../core/secretsManager"; +import { + DeploymentTelemetry, + type DeploymentRecoveryTrigger, + type DeploymentSuspendReason, +} from "../instrumentation/deployment"; import { type Logger } from "../logging/logger"; import { type OAuthSessionManager } from "../oauth/sessionManager"; import { getAuthConfigWatchSettings } from "../settings/authConfig"; @@ -40,6 +45,7 @@ export class DeploymentManager implements vscode.Disposable { private readonly contextManager: ContextManager; private readonly logger: Logger; private readonly telemetryService: TelemetryService; + private readonly deploymentTelemetry: DeploymentTelemetry; #deployment: Deployment | null = null; #disposed = false; @@ -60,6 +66,7 @@ export class DeploymentManager implements vscode.Disposable { this.contextManager = serviceContainer.getContextManager(); this.logger = serviceContainer.getLogger(); this.telemetryService = serviceContainer.getTelemetryService(); + this.deploymentTelemetry = new DeploymentTelemetry(this.telemetryService); } public static create( @@ -175,9 +182,11 @@ export class DeploymentManager implements vscode.Disposable { /** * Clears the current deployment. */ - public async clearDeployment(): Promise { + public async clearDeployment( + reason: DeploymentSuspendReason = "credentials_removed", + ): Promise { this.logger.debug("Clearing deployment", this.#deployment?.safeHostname); - this.suspendSession(); + this.suspendSession(reason); this.#authListenerDisposable?.dispose(); this.#authListenerDisposable = undefined; this.#deployment = null; @@ -190,11 +199,17 @@ export class DeploymentManager implements vscode.Disposable { * Suspend session: shows logged-out state but keeps deployment for easy re-login. * Auth listener remains active so recovery can happen automatically if tokens update. */ - public suspendSession(): void { + public suspendSession( + reason: DeploymentSuspendReason = "auth_config_change", + ): void { + const wasAuthenticated = this.isAuthenticated(); this.oauthSessionManager.clearDeployment(); this.client.setCredentials(undefined, undefined); this.updateAuthContexts(undefined); this.clearWorkspaces(); + if (wasAuthenticated) { + this.deploymentTelemetry.suspended(reason); + } } /** @@ -242,11 +257,14 @@ export class DeploymentManager implements vscode.Disposable { this.logger.debug( "Token updated after session suspended, recovering", ); - await this.verifyAndApplyDeployment({ - url: auth.url, - safeHostname, - token: auth.token, - }); + await this.recoverDeployment( + { + url: auth.url, + safeHostname, + token: auth.token, + }, + "token_update", + ); } } else { await this.clearDeployment(); @@ -285,7 +303,10 @@ export class DeploymentManager implements vscode.Disposable { this.logger.debug( "Authentication settings changed after session suspended, recovering", ); - await this.verifyAndApplyDeployment(snapshot); + const recovered = await this.recoverDeployment(snapshot, "auth_config"); + if (!recovered) { + this.deploymentTelemetry.authConfigRecoveryFailed(); + } } while (this.#recoveryPending); } catch (err) { this.logger.warn( @@ -307,12 +328,24 @@ export class DeploymentManager implements vscode.Disposable { if (deployment) { this.logger.info("Deployment changed from another window"); - await this.verifyAndApplyDeployment(deployment); + this.deploymentTelemetry.crossWindowDetected(); + await this.recoverDeployment(deployment, "cross_window"); } }, ); } + private async recoverDeployment( + deployment: Deployment & { token?: string }, + trigger: DeploymentRecoveryTrigger, + ): Promise { + const recovered = await this.verifyAndApplyDeployment(deployment); + if (recovered) { + this.deploymentTelemetry.recovered(trigger); + } + return recovered; + } + /** * Update authentication-related contexts. */ diff --git a/src/extension.ts b/src/extension.ts index c8e7fb8f3..5903d79cf 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -103,7 +103,7 @@ async function doActivate( // Shared handler for auth failures (used by interceptor + session manager) const handleAuthFailure = (): Promise => { - deploymentManager.suspendSession(); + deploymentManager.suspendSession("auth_failure"); vscode.window .showWarningMessage( "Session expired. You have been signed out.", diff --git a/src/instrumentation/auth.ts b/src/instrumentation/auth.ts index c6cba325c..0e85ba33b 100644 --- a/src/instrumentation/auth.ts +++ b/src/instrumentation/auth.ts @@ -1,17 +1,49 @@ +import type { LoginMethod } from "../login/loginCoordinator"; import type { TelemetryReporter } from "../telemetry/reporter"; +import type { Span } from "../telemetry/span"; export type AuthTokenRefreshTrigger = "background" | "reactive"; export type AuthRecoveryAction = "refresh_success" | "login_required" | "none"; export type AuthLoginPromptTrigger = "auth_required" | "missing_session"; +export type AuthLoginSource = + | "auto_login" + | "command" + | "direct" + | "switch_deployment" + | "uri"; +export type AuthLoginMethod = + | "unknown" + | "mtls" + | "provided_token" + | "stored_token" + | "keyring_token" + | "cli_token" + | "oauth"; export type LoginPromptReason = | "user_dismissed" | "no_url_provided" | "auth_failed"; +export type AuthLoginReason = LoginPromptReason | "exception"; +export type AuthLogoutReason = + | "not_authenticated" + | "credential_clear_cancelled" + | "credential_clear_failed" + | "exception"; export type LoginPromptOutcome = | { success: true } | { success: false; reason: LoginPromptReason }; +export type AuthLoginOutcome = + | { success: true; method: LoginMethod } + | { success: false; method?: LoginMethod; reason: AuthLoginReason }; +export type AuthLogoutOutcome = + | { success: true } + | { success: false; reason: AuthLogoutReason }; + +interface AuthLoginTrace { + setMethod(method: LoginMethod): void; +} interface AuthRecoveryRecorder { logReceived(): void; @@ -19,9 +51,54 @@ interface AuthRecoveryRecorder { setRefreshAttempted(attempted: boolean): void; } +const loginMethods = { + unknown: "unknown", + mtls: "mtls", + provided_token: "provided_token", + stored_token: "stored_token", + keyring_token: "keyring_token", + cli_token: "cli_token", + oauth: "oauth", +} as const satisfies Record; + export class AuthTelemetry { public constructor(private readonly telemetry: TelemetryReporter) {} + public traceLogin( + source: AuthLoginSource, + fn: (trace: AuthLoginTrace) => Promise, + ): Promise { + return this.telemetry.trace( + "auth.login", + async (span) => { + try { + const result = await fn(createLoginTrace(span)); + recordLoginResult(span, result); + return result; + } catch (error) { + span.setProperty("reason", "exception"); + throw error; + } + }, + { source, method: "unknown" }, + ); + } + + public traceLogout( + fn: () => Promise, + ): Promise { + return this.telemetry.trace("auth.logout", async (span) => { + try { + const result = await fn(); + recordLogoutResult(span, result); + return result; + } catch (error) { + span.setProperty("reason", "exception"); + throw error; + } + }); + } + public traceTokenRefresh( trigger: AuthTokenRefreshTrigger, fn: () => Promise, @@ -67,17 +144,60 @@ export class AuthTelemetry { "auth.login_prompted", async (span) => { const result = await fn(); - if (!result.success) { - span.setProperty("reason", result.reason); - if (result.reason === "auth_failed") { - span.markFailure(); - } else { - span.markAborted(); - } - } + recordPromptResult(span, result); return result; }, { trigger }, ); } } + +function createLoginTrace(span: Span): AuthLoginTrace { + return { + setMethod: (method) => span.setProperty("method", loginMethods[method]), + }; +} + +function recordLoginResult(span: Span, result: AuthLoginOutcome): void { + if (result.method) { + span.setProperty("method", loginMethods[result.method]); + } + if (result.success) { + return; + } + + recordReason(span, result.reason); +} + +function recordLogoutResult(span: Span, result: AuthLogoutOutcome): void { + if (result.success) { + return; + } + + span.setProperty("reason", result.reason); + if ( + result.reason === "not_authenticated" || + result.reason === "credential_clear_cancelled" + ) { + span.markAborted(); + return; + } + span.markFailure(); +} + +function recordPromptResult(span: Span, result: LoginPromptOutcome): void { + if (result.success) { + return; + } + + recordReason(span, result.reason); +} + +function recordReason(span: Span, reason: AuthLoginReason): void { + span.setProperty("reason", reason); + if (reason === "auth_failed" || reason === "exception") { + span.markFailure(); + return; + } + span.markAborted(); +} diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts new file mode 100644 index 000000000..030c08b10 --- /dev/null +++ b/src/instrumentation/credentials.ts @@ -0,0 +1,153 @@ +import { isAbortError } from "../error/errorUtils"; +import { isKeyringEnabled } from "../settings/cli"; + +import type { WorkspaceConfiguration } from "vscode"; + +import type { TelemetryReporter } from "../telemetry/reporter"; +import type { Span } from "../telemetry/span"; + +export type CredentialCategory = "keyring" | "file"; +export type CredentialFailureCategory = "aborted" | "binary" | "cli" | "file"; + +interface CredentialTrace { + file(fn: () => Promise): Promise; + keyring(fn: () => Promise): Promise; +} + +export interface CredentialClearResult { + readonly failureCategory?: CredentialFailureCategory; +} + +export class CredentialTelemetry { + public constructor(private readonly telemetry: TelemetryReporter) {} + + public async traceStore( + configs: Pick, + fn: (trace: CredentialTrace) => Promise, + ): Promise { + return this.traceCredential("auth.credential.store", configs, fn); + } + + public async traceClear( + configs: Pick, + fn: (trace: CredentialTrace) => Promise, + ): Promise { + return this.traceCredential( + "auth.credential.clear", + configs, + fn, + recordClearFailure, + ); + } + + private async traceCredential( + eventName: "auth.credential.store" | "auth.credential.clear", + configs: Pick, + fn: (trace: CredentialTrace) => Promise, + recordResult?: (span: Span, result: T) => void, + ): Promise { + const defaults = defaultCredentialProperties(configs); + let cancellation: unknown; + const result = await this.telemetry.trace( + eventName, + async (span) => { + try { + const result = await fn(createCredentialTrace(span)); + recordResult?.(span, result); + return result; + } catch (error) { + recordCredentialFailure(span, defaults.category, error); + if (isAbortError(error)) { + span.markAborted(); + cancellation = error; + return undefined as T; + } + throw error; + } + }, + { + keyring_enabled: defaults.keyringEnabled, + category: defaults.category, + }, + ); + if (cancellation instanceof Error) { + throw cancellation; + } + return result; + } +} + +function defaultCredentialProperties( + configs: Pick, +): { keyringEnabled: boolean; category: CredentialCategory } { + const keyringEnabled = isKeyringEnabled(configs); + return { + keyringEnabled, + category: keyringEnabled ? "keyring" : "file", + }; +} + +function createCredentialTrace(span: Span): CredentialTrace { + const run = async ( + category: CredentialCategory, + fn: () => Promise, + ): Promise => { + span.setProperty("category", category); + return await fn(); + }; + return { + file: (fn) => run("file", fn), + keyring: (fn) => run("keyring", fn), + }; +} + +function recordCredentialFailure( + span: Span, + category: CredentialCategory, + error: unknown, +): void { + span.setProperty("category", category); + span.setProperty("failure_category", categorizeCredentialError(error)); +} + +function recordClearFailure(span: Span, result: CredentialClearResult): void { + if (!result.failureCategory) { + return; + } + + span.setProperty("failure_category", result.failureCategory); + if (result.failureCategory === "aborted") { + span.markAborted(); + return; + } + span.markFailure(); +} + +export function categorizeCredentialError( + error: unknown, +): CredentialFailureCategory { + if (isAbortError(error)) { + return "aborted"; + } + if (error instanceof CredentialFileError) { + return "file"; + } + if (error instanceof CredentialCliError) { + return "cli"; + } + return "binary"; +} + +export class CredentialCliError extends Error { + public constructor(cause: unknown) { + super("Credential CLI operation failed", { cause }); + 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/instrumentation/deployment.ts b/src/instrumentation/deployment.ts new file mode 100644 index 000000000..738b9ff91 --- /dev/null +++ b/src/instrumentation/deployment.ts @@ -0,0 +1,31 @@ +import type { TelemetryReporter } from "../telemetry/reporter"; + +export type DeploymentSuspendReason = + | "auth_config_change" + | "auth_failure" + | "credentials_removed" + | "logout"; +export type DeploymentRecoveryTrigger = + | "auth_config" + | "cross_window" + | "token_update"; + +export class DeploymentTelemetry { + public constructor(private readonly telemetry: TelemetryReporter) {} + + public suspended(reason: DeploymentSuspendReason): void { + this.telemetry.log("deployment.suspended", { reason }); + } + + public recovered(trigger: DeploymentRecoveryTrigger): void { + this.telemetry.log("deployment.recovered", { trigger }); + } + + public crossWindowDetected(): void { + this.telemetry.log("deployment.cross_window.detected"); + } + + public authConfigRecoveryFailed(): void { + this.telemetry.log("deployment.auth_config.recovery_failed"); + } +} diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index baac3f28d..cb02627a9 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -27,10 +27,28 @@ import type { import type { Logger } from "../logging/logger"; import type { OAuthCallback } from "../oauth/oauthCallback"; -type LoginResult = +export type LoginMethod = + | "mtls" + | "provided_token" + | "stored_token" + | "keyring_token" + | "cli_token" + | "oauth"; + +type LoginAttemptResult = | { success: false; reason: LoginPromptReason } | { success: true; user: User; token: string; oauth?: OAuthTokenData }; +export type LoginResult = + | { success: false; method?: LoginMethod; reason: LoginPromptReason } + | { + success: true; + method: LoginMethod; + user: User; + token: string; + oauth?: OAuthTokenData; + }; + export interface LoginOptions { safeHostname: string; url: string | undefined; @@ -63,10 +81,10 @@ export class LoginCoordinator implements vscode.Disposable { } /** - * Direct login - for user-initiated login via commands. + * Direct login for callers that already have a deployment URL. * Stores session auth and URL history on success. */ - public async ensureLoggedIn( + public ensureLoggedIn( options: LoginOptions & { url: string }, ): Promise { const { safeHostname, url } = options; @@ -92,7 +110,7 @@ export class LoginCoordinator implements vscode.Disposable { detailPrefix?: string; trigger: AuthLoginPromptTrigger; }, - ): Promise { + ): Promise { return this.authTelemetry.traceLoginPrompt(options.trigger, () => this.performLoginDialog(options), ); @@ -100,7 +118,7 @@ export class LoginCoordinator implements vscode.Disposable { private async performLoginDialog( options: LoginOptions & { message?: string; detailPrefix?: string }, - ): Promise { + ): Promise { const { safeHostname, url, detailPrefix, message } = options; return this.executeWithGuard(async () => { // Show dialog promise @@ -116,7 +134,7 @@ export class LoginCoordinator implements vscode.Disposable { }, "Login", ) - .then(async (action): Promise => { + .then(async (action): Promise => { if (action === "Login") { // Proceed with the login flow, handling logging in from another window const storedAuth = @@ -158,7 +176,7 @@ export class LoginCoordinator implements vscode.Disposable { } private async persistSessionAuth( - result: LoginResult, + result: LoginAttemptResult, safeHostname: string, url: string, ): Promise { @@ -184,9 +202,9 @@ export class LoginCoordinator implements vscode.Disposable { /** * Chains login attempts to prevent overlapping UI. */ - private executeWithGuard( - executeFn: () => Promise, - ): Promise { + private executeWithGuard( + executeFn: () => Promise, + ): Promise { const result = this.loginQueue.then(executeFn); this.loginQueue = result.catch(() => { /* Keep chain going on error */ @@ -199,11 +217,11 @@ export class LoginCoordinator implements vscode.Disposable { * Returns a promise and a dispose function to clean up the listener. */ private waitForCrossWindowLogin(safeHostname: string): { - promise: Promise; + promise: Promise; dispose: () => void; } { let disposable: vscode.Disposable | undefined; - const promise = new Promise((resolve) => { + const promise = new Promise((resolve) => { disposable = this.secretsManager.onDidChangeSessionAuth( safeHostname, async (auth) => { @@ -234,9 +252,9 @@ export class LoginCoordinator implements vscode.Disposable { /** * Attempt to authenticate using OAuth, token, or mTLS. If necessary, prompts - * for authentication method and credentials. Returns the token and user upon - * successful authentication. Null means the user aborted or authentication - * failed (in which case an error notification will have been displayed). + * for authentication method and credentials. Returns the token, user, and + * method on success, or a bounded reason when the user aborts or + * authentication fails. */ private async attemptLogin( deployment: Deployment, @@ -265,7 +283,10 @@ export class LoginCoordinator implements vscode.Disposable { // mTLS authentication (no token needed) if (!needToken(vscode.workspace.getConfiguration())) { this.logger.debug("Attempting mTLS authentication (no token required)"); - return this.tryMtlsAuth(client, isAutoLogin); + return withLoginMethod( + "mtls", + await this.tryMtlsAuth(client, isAutoLogin), + ); } // Try provided token first @@ -277,7 +298,7 @@ export class LoginCoordinator implements vscode.Disposable { isAutoLogin, ); if (result !== "unauthorized") { - return result; + return withLoginMethod("provided_token", result); } } @@ -289,7 +310,7 @@ export class LoginCoordinator implements vscode.Disposable { this.logger.debug("Trying stored session token"); const result = await this.tryTokenAuth(client, auth.token, isAutoLogin); if (result !== "unauthorized") { - return result; + return withLoginMethod("stored_token", result); } } @@ -316,7 +337,7 @@ export class LoginCoordinator implements vscode.Disposable { this.logger.debug("Trying token from OS keyring"); const result = await this.tryTokenAuth(client, keyringToken, isAutoLogin); if (result !== "unauthorized") { - return result; + return withLoginMethod("keyring_token", result); } } @@ -324,9 +345,9 @@ export class LoginCoordinator implements vscode.Disposable { const authMethod = await maybeAskAuthMethod(client); switch (authMethod) { case "oauth": - return this.loginWithOAuth(deployment); + return withLoginMethod("oauth", await this.loginWithOAuth(deployment)); case "legacy": - return this.loginWithToken(client); + return withLoginMethod("cli_token", await this.loginWithToken(client)); case undefined: return { success: false, reason: "user_dismissed" }; } @@ -335,7 +356,7 @@ export class LoginCoordinator implements vscode.Disposable { private async tryMtlsAuth( client: CoderApi, isAutoLogin: boolean, - ): Promise { + ): Promise { try { const user = await client.getAuthenticatedUser(); return { success: true, token: "", user }; @@ -352,7 +373,7 @@ export class LoginCoordinator implements vscode.Disposable { client: CoderApi, token: string, isAutoLogin: boolean, - ): Promise { + ): Promise { client.setSessionToken(token); try { const user = await client.getAuthenticatedUser(); @@ -392,7 +413,7 @@ export class LoginCoordinator implements vscode.Disposable { /** * Session token authentication flow. */ - private async loginWithToken(client: CoderApi): Promise { + private async loginWithToken(client: CoderApi): Promise { const url = client.getAxiosInstance().defaults.baseURL; if (!url) { throw new Error("No base URL set on REST client"); @@ -450,7 +471,9 @@ export class LoginCoordinator implements vscode.Disposable { /** * OAuth authentication flow. */ - private async loginWithOAuth(deployment: Deployment): Promise { + private async loginWithOAuth( + deployment: Deployment, + ): Promise { try { this.logger.debug("Starting OAuth authentication"); @@ -492,3 +515,10 @@ export class LoginCoordinator implements vscode.Disposable { this.oauthAuthorizer.dispose(); } } + +function withLoginMethod( + method: LoginMethod, + result: LoginAttemptResult, +): LoginResult { + return { ...result, method }; +} diff --git a/src/uri/uriHandler.ts b/src/uri/uriHandler.ts index a846b4ef4..520654fba 100644 --- a/src/uri/uriHandler.ts +++ b/src/uri/uriHandler.ts @@ -1,6 +1,7 @@ import * as vscode from "vscode"; 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"; @@ -131,6 +132,9 @@ async function setupDeployment( const secretsManager = serviceContainer.getSecretsManager(); const mementoManager = serviceContainer.getMementoManager(); const loginCoordinator = serviceContainer.getLoginCoordinator(); + const authTelemetry = new AuthTelemetry( + serviceContainer.getTelemetryService(), + ); const currentDeployment = await secretsManager.getCurrentDeployment(); @@ -151,10 +155,16 @@ async function setupDeployment( const safeHostname = toSafeHost(url); const token: string | undefined = params.get("token") ?? undefined; - const result = await loginCoordinator.ensureLoggedIn({ - safeHostname, - url, - token, + const result = await authTelemetry.traceLogin("uri", async (trace) => { + const result = await loginCoordinator.ensureLoggedIn({ + safeHostname, + url, + token, + }); + if (result.method) { + trace.setMethod(result.method); + } + return result; }); if (!result.success) { diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index f715d9b4a..b135e5940 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -446,7 +446,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { return { storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), - deleteToken: vi.fn().mockResolvedValue(undefined), + deleteToken: vi.fn().mockResolvedValue({}), } as unknown as CliCredentialManager; } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts new file mode 100644 index 000000000..2a799fd8f --- /dev/null +++ b/test/unit/commands.test.ts @@ -0,0 +1,363 @@ +import { describe, expect, it, vi } from "vitest"; + +import { Commands } from "@/commands"; +import { maybeAskUrl } from "@/promptUtils"; + +import { createTestTelemetryService, TestSink } from "../mocks/telemetry"; +import { + createMockLogger, + createMockUser, + MockConfigurationProvider, + MockUserInteraction, +} from "../mocks/testHelpers"; + +import type { CoderApi } from "@/api/coderApi"; +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 { DeploymentManager } from "@/deployment/deploymentManager"; +import type { Deployment } from "@/deployment/types"; +import type { CredentialClearResult } from "@/instrumentation/credentials"; +import type { LoginCoordinator, LoginResult } from "@/login/loginCoordinator"; +import type { SpeedtestPanelFactory } from "@/webviews/speedtest/speedtestPanelFactory"; +import type { DuplicateWorkspaceIpc } from "@/workspace/duplicateWorkspaceIpc"; + +vi.mock("@/promptUtils", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, maybeAskUrl: vi.fn() }; +}); + +vi.mock("@/workspace/workspacesProvider", () => { + class AgentTreeItem {} + class WorkspaceTreeItem {} + return { AgentTreeItem, WorkspaceTreeItem }; +}); + +const TEST_URL = "https://coder.example.com"; +const TEST_HOSTNAME = "coder.example.com"; + +interface LoginOptionsForTest { + safeHostname: string; + url: string; + autoLogin?: boolean; +} + +type LoginResultForTest = LoginResult & { + readonly user?: ReturnType; +}; + +interface CommandsHarnessOptions { + readonly authenticated?: boolean; + + readonly loginResult?: LoginResultForTest; + readonly credentialResult?: CredentialClearResult; + readonly clearDeploymentError?: Error; + readonly clearAllAuthDataError?: Error; +} + +function createCommandsHarness(options: CommandsHarnessOptions = {}) { + vi.clearAllMocks(); + new MockConfigurationProvider(); + new MockUserInteraction(); + vi.mocked(maybeAskUrl).mockResolvedValue(TEST_URL); + + const sink = new TestSink(); + const telemetry = createTestTelemetryService(sink); + const logger = createMockLogger(); + const deployment: Deployment = { + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }; + const loginResult = + options.loginResult ?? + ({ + success: true, + method: "stored_token", + user: createMockUser(), + token: "test-token", + } satisfies LoginResultForTest); + const loginCoordinator = { + ensureLoggedIn: vi.fn((_loginOptions: LoginOptionsForTest) => + Promise.resolve(loginResult), + ), + }; + + const deploymentManager = { + isAuthenticated: vi.fn(() => options.authenticated ?? false), + getCurrentDeployment: vi.fn(() => deployment), + setDeployment: vi.fn(() => Promise.resolve()), + clearDeployment: vi.fn(() => { + if (options.clearDeploymentError) { + return Promise.reject(options.clearDeploymentError); + } + return Promise.resolve(); + }), + }; + + const cliManager = { + clearCredentials: vi.fn(() => + Promise.resolve(options.credentialResult ?? {}), + ), + }; + + const secretsManager = { + getCurrentDeployment: vi.fn(() => Promise.resolve(null)), + clearAllAuthData: vi.fn(() => { + if (options.clearAllAuthDataError) { + return Promise.reject(options.clearAllAuthDataError); + } + return Promise.resolve(); + }), + }; + + const serviceContainer = { + getTelemetryService: () => telemetry, + getLogger: () => logger, + getPathResolver: () => ({}) as PathResolver, + getMementoManager: () => ({}) as MementoManager, + getSecretsManager: () => secretsManager as unknown as SecretsManager, + getCliManager: () => cliManager as unknown as CliManager, + getLoginCoordinator: () => loginCoordinator as unknown as LoginCoordinator, + getDuplicateWorkspaceIpc: () => ({}) as DuplicateWorkspaceIpc, + getSpeedtestPanelFactory: () => ({}) as SpeedtestPanelFactory, + } as unknown as ServiceContainer; + + const extensionClient = { + getAxiosInstance: () => ({ defaults: { baseURL: TEST_URL } }), + } as unknown as CoderApi; + + const commands = new Commands( + serviceContainer, + extensionClient, + deploymentManager as unknown as DeploymentManager, + ); + + return { + commands, + deployment, + telemetry: sink, + mocks: { + cliManager, + deploymentManager, + loginCoordinator, + secretsManager, + }, + }; +} + +type CommandsHarness = ReturnType; + +interface CommandScenario { + readonly name: string; + readonly options?: CommandsHarnessOptions; + readonly arrange?: (harness: CommandsHarness) => void; + readonly act: (harness: CommandsHarness) => Promise; + readonly assert: (harness: CommandsHarness) => void; +} + +function testCommandScenario(scenario: CommandScenario): void { + it(scenario.name, async () => { + const harness = createCommandsHarness(scenario.options); + scenario.arrange?.(harness); + + await scenario.act(harness); + + scenario.assert(harness); + }); +} + +describe("Commands", () => { + describe("login telemetry", () => { + testCommandScenario({ + name: "emits one auth.login for command login success", + act: ({ commands }) => commands.login(), + assert: ({ mocks, telemetry }) => { + const events = telemetry.eventsNamed("auth.login"); + expect(events).toHaveLength(1); + expect(events[0]).toMatchObject({ + properties: { + source: "command", + method: "stored_token", + result: "success", + }, + }); + expect(events[0].measurements.durationMs).toEqual(expect.any(Number)); + expect(mocks.loginCoordinator.ensureLoggedIn).toHaveBeenCalledWith( + expect.objectContaining({ + safeHostname: TEST_HOSTNAME, + url: TEST_URL, + }), + ); + expect(mocks.deploymentManager.setDeployment).toHaveBeenCalled(); + }, + }); + + testCommandScenario({ + name: "uses auto_login source when requested", + options: { + loginResult: { + success: true, + method: "provided_token", + user: createMockUser(), + token: "test-token", + }, + }, + act: ({ commands }) => commands.login({ url: TEST_URL, autoLogin: true }), + assert: ({ mocks, telemetry }) => { + expect(telemetry.expectOne("auth.login").properties).toMatchObject({ + source: "auto_login", + method: "provided_token", + result: "success", + }); + expect(mocks.loginCoordinator.ensureLoggedIn).toHaveBeenCalledWith( + expect.objectContaining({ autoLogin: true }), + ); + }, + }); + + testCommandScenario({ + name: "records URL cancellation without attempting login", + arrange: () => { + vi.mocked(maybeAskUrl).mockResolvedValueOnce(undefined); + }, + act: ({ commands }) => commands.login(), + assert: ({ mocks, telemetry }) => { + expect(telemetry.expectOne("auth.login")).toMatchObject({ + properties: { + source: "command", + method: "unknown", + result: "aborted", + reason: "no_url_provided", + }, + }); + expect(mocks.loginCoordinator.ensureLoggedIn).not.toHaveBeenCalled(); + }, + }); + + testCommandScenario({ + name: "records auth failures as auth.login errors", + options: { + loginResult: { + success: false, + method: "cli_token", + reason: "auth_failed", + }, + }, + act: ({ commands }) => commands.login(), + assert: ({ mocks, telemetry }) => { + expect(telemetry.expectOne("auth.login")).toMatchObject({ + properties: { + method: "cli_token", + result: "error", + reason: "auth_failed", + }, + }); + expect(mocks.deploymentManager.setDeployment).not.toHaveBeenCalled(); + }, + }); + + testCommandScenario({ + name: "uses switch_deployment source", + act: ({ commands }) => commands.switchDeployment(), + assert: ({ telemetry }) => { + expect(telemetry.expectOne("auth.login").properties).toMatchObject({ + source: "switch_deployment", + result: "success", + }); + }, + }); + }); + + describe("logout telemetry", () => { + testCommandScenario({ + name: "records not_authenticated as aborted", + options: { authenticated: false }, + act: ({ commands }) => commands.logout(), + assert: ({ mocks, telemetry }) => { + expect(telemetry.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "not_authenticated", + }, + }); + expect(mocks.cliManager.clearCredentials).not.toHaveBeenCalled(); + }, + }); + + testCommandScenario({ + name: "records successful logout", + options: { authenticated: true }, + act: ({ commands }) => commands.logout(), + assert: ({ mocks, telemetry }) => { + const event = telemetry.expectOne("auth.logout"); + expect(event.properties).toMatchObject({ result: "success" }); + expect(event.properties.reason).toBeUndefined(); + expect(event.measurements.durationMs).toEqual(expect.any(Number)); + expect(mocks.deploymentManager.clearDeployment).toHaveBeenCalledWith( + "logout", + ); + expect(mocks.cliManager.clearCredentials).toHaveBeenCalledWith( + TEST_URL, + ); + expect(mocks.secretsManager.clearAllAuthData).toHaveBeenCalledWith( + TEST_HOSTNAME, + ); + }, + }); + + testCommandScenario({ + name: "records credential clear cancellation as aborted", + options: { + authenticated: true, + credentialResult: { + failureCategory: "aborted", + }, + }, + act: ({ commands }) => commands.logout(), + assert: ({ telemetry }) => { + expect(telemetry.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "credential_clear_cancelled", + }, + }); + }, + }); + + testCommandScenario({ + name: "records credential clear failure as an error", + options: { + authenticated: true, + credentialResult: { + failureCategory: "cli", + }, + }, + act: ({ commands }) => commands.logout(), + assert: ({ telemetry }) => { + expect(telemetry.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "error", + reason: "credential_clear_failed", + }, + }); + }, + }); + + it("records logout exceptions", async () => { + const harness = createCommandsHarness({ + authenticated: true, + clearAllAuthDataError: new Error("secret clear failed"), + }); + + await expect(harness.commands.logout()).rejects.toThrow( + "secret clear failed", + ); + expect(harness.telemetry.expectOne("auth.logout")).toMatchObject({ + properties: { result: "error", reason: "exception" }, + error: { message: "secret clear failed" }, + }); + }); + }); +}); diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 1167654bd..1e2eb8581 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -12,7 +12,11 @@ import * as cliExec from "@/core/cliExec"; import { PathResolver } from "@/core/pathResolver"; import { isKeyringEnabled } from "@/settings/cli"; -import { createMockLogger } from "../../mocks/testHelpers"; +import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; +import { + createMockLogger, + MockConfigurationProvider, +} from "../../mocks/testHelpers"; import type * as nodeFs from "node:fs"; @@ -136,12 +140,15 @@ function writeCredentialFiles(url: string, token: string) { function setup(resolver?: BinaryResolver) { const r = resolver ?? successResolver(); + const sink = new TestSink(); return { resolver: r, + sink, manager: new CliCredentialManager( createMockLogger(), r, TEST_PATH_RESOLVER, + createTestTelemetryService(sink), ), }; } @@ -160,6 +167,7 @@ describe("isKeyringSupported", () => { describe("CliCredentialManager", () => { beforeEach(() => { + new MockConfigurationProvider(); vi.clearAllMocks(); vol.reset(); vi.mocked(isKeyringEnabled).mockReturnValue(false); @@ -168,19 +176,26 @@ describe("CliCredentialManager", () => { describe("storeToken", () => { it("writes files when keyring is disabled", async () => { - const { manager } = setup(); + const { manager, sink } = setup(); await manager.storeToken(TEST_URL, "my-token", configs); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); expect(memfs.readFileSync(SESSION_FILE, "utf8")).toBe("my-token"); + expect(sink.expectOne("auth.credential.store")).toMatchObject({ + properties: { + category: "file", + keyring_enabled: "false", + result: "success", + }, + }); }); it("resolves binary and invokes coder login when keyring enabled", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ stdout: "" }); - const { manager, resolver } = setup(); + const { manager, resolver, sink } = setup(); await manager.storeToken(TEST_URL, "my-secret-token", configs); @@ -191,6 +206,13 @@ describe("CliCredentialManager", () => { // Token must only appear in env, never in args expect(exec.env.CODER_SESSION_TOKEN).toBe("my-secret-token"); expect(exec.args).not.toContain("my-secret-token"); + expect(sink.expectOne("auth.credential.store")).toMatchObject({ + properties: { + category: "keyring", + keyring_enabled: "true", + result: "success", + }, + }); }); it("falls back to files when CLI version too old", async () => { @@ -208,11 +230,17 @@ describe("CliCredentialManager", () => { it("throws when CLI exec fails", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ error: "login failed" }); - const { manager } = setup(); + const { manager, sink } = setup(); await expect( manager.storeToken(TEST_URL, "token", configs), - ).rejects.toThrow("login failed"); + ).rejects.toThrow("Credential CLI operation failed"); + expect(sink.expectOne("auth.credential.store")).toMatchObject({ + properties: { + failure_category: "cli", + result: "error", + }, + }); }); it("throws when binary resolver fails and keyring enabled", async () => { @@ -261,13 +289,19 @@ describe("CliCredentialManager", () => { it("rejects with AbortError when signal is pre-aborted", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFileAbortable(); - const { manager } = setup(); + const { manager, sink } = setup(); await expect( manager.storeToken(TEST_URL, "token", configs, { signal: AbortSignal.abort(), }), ).rejects.toThrow("The operation was aborted"); + expect(sink.expectOne("auth.credential.store")).toMatchObject({ + properties: { + failure_category: "aborted", + result: "aborted", + }, + }); }); }); @@ -369,7 +403,7 @@ describe("CliCredentialManager", () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ stdout: "" }); writeCredentialFiles(TEST_URL, "old-token"); - const { manager, resolver } = setup(); + const { manager, resolver, sink } = setup(); await manager.deleteToken(TEST_URL, configs); @@ -379,6 +413,13 @@ describe("CliCredentialManager", () => { expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); expect(memfs.existsSync(URL_FILE)).toBe(false); expect(memfs.existsSync(SESSION_FILE)).toBe(false); + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ + properties: { + category: "keyring", + keyring_enabled: "true", + result: "success", + }, + }); }); it("deletes files even when keyring is disabled", async () => { @@ -395,21 +436,34 @@ describe("CliCredentialManager", () => { it("never throws on CLI error", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ error: "logout failed" }); - const { manager } = setup(); + const { manager, sink } = setup(); await expect( manager.deleteToken(TEST_URL, configs), ).resolves.not.toThrow(); + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ + properties: { + failure_category: "cli", + result: "error", + }, + }); }); it("never throws when binary resolver fails", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); - const { manager } = setup(failingResolver()); + const { manager, sink } = setup(failingResolver()); - await expect( - manager.deleteToken(TEST_URL, configs), - ).resolves.not.toThrow(); + await expect(manager.deleteToken(TEST_URL, configs)).resolves.toEqual({ + failureCategory: "binary", + }); expect(execFile).not.toHaveBeenCalled(); + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ + properties: { + category: "keyring", + failure_category: "binary", + result: "error", + }, + }); }); it("forwards header command args", async () => { @@ -450,13 +504,19 @@ describe("CliCredentialManager", () => { it("throws AbortError when signal is aborted", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFileAbortable(); - const { manager } = setup(); + const { manager, sink } = setup(); await expect( manager.deleteToken(TEST_URL, configs, { signal: AbortSignal.abort(), }), ).rejects.toThrow("The operation was aborted"); + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ + properties: { + failure_category: "aborted", + result: "aborted", + }, + }); }); }); }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 4e0c185d1..f9ddf3e1c 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -353,7 +353,7 @@ describe("CliManager", () => { const CLEAR_URL = "https://dev.coder.com"; it("should skip progress notification when keyring is disabled", async () => { - await manager.clearCredentials(CLEAR_URL); + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({}); expect(vscode.window.withProgress).not.toHaveBeenCalled(); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( @@ -365,8 +365,9 @@ describe("CliManager", () => { it("should show progress notification when keyring is enabled", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); + vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce({}); - await manager.clearCredentials(CLEAR_URL); + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({}); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ @@ -378,15 +379,35 @@ describe("CliManager", () => { ); }); - it.each([ - { scenario: "succeeds", error: undefined }, - { scenario: "fails", error: new Error("unexpected failure") }, - { scenario: "is cancelled", error: makeAbortError() }, - ])("should not throw when deleteToken $scenario", async ({ error }) => { - if (error) { - vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); - } - await expect(manager.clearCredentials(CLEAR_URL)).resolves.not.toThrow(); + it("returns credential manager failure categories", async () => { + vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce({ + failureCategory: "cli", + }); + + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ + failureCategory: "cli", + }); + }); + + it("categorizes unexpected deleteToken failures", async () => { + vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce( + new Error("unexpected failure"), + ); + + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ + failureCategory: "binary", + }); + }); + + it("categorizes deleteToken cancellation", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce( + makeAbortError(), + ); + + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ + failureCategory: "aborted", + }); }); }); diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 2e90287e1..3f4e157f4 100644 --- a/test/unit/deployment/deploymentManager.test.ts +++ b/test/unit/deployment/deploymentManager.test.ts @@ -6,7 +6,7 @@ import { MementoManager } from "@/core/mementoManager"; import { SecretsManager } from "@/core/secretsManager"; import { DeploymentManager } from "@/deployment/deploymentManager"; -import { createTestTelemetryService } from "../../mocks/telemetry"; +import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; import { createMockLogger, createMockServiceContainer, @@ -77,7 +77,8 @@ function createTestContext() { validationMockClient as unknown as CoderApi, ); - const telemetryService = createTestTelemetryService(); + const telemetrySink = new TestSink(); + const telemetryService = createTestTelemetryService(telemetrySink); const setDeploymentUrlSpy = vi.spyOn(telemetryService, "setDeploymentUrl"); const manager = DeploymentManager.create( @@ -101,6 +102,7 @@ function createTestContext() { contextManager, mockOAuthSessionManager, mockWorkspaceProvider, + telemetrySink, telemetryService, setDeploymentUrlSpy, manager, @@ -316,8 +318,12 @@ describe("DeploymentManager", () => { }); it("picks up deployment when not authenticated", async () => { - const { mockClient, validationMockClient, secretsManager } = - createTestContext(); + const { + mockClient, + validationMockClient, + secretsManager, + telemetrySink, + } = createTestContext(); const user = createMockUser(); validationMockClient.setAuthenticatedUserResponse(user); @@ -338,6 +344,12 @@ describe("DeploymentManager", () => { expect(mockClient.host).toBe(TEST_URL); expect(mockClient.token).toBe("synced-token"); + expect( + telemetrySink.expectOne("deployment.cross_window.detected").properties, + ).toEqual({}); + expect(telemetrySink.expectOne("deployment.recovered")).toMatchObject({ + properties: { trigger: "cross_window" }, + }); }); it("handles mTLS deployment (empty token) from other window", async () => { @@ -430,6 +442,24 @@ describe("DeploymentManager", () => { }); describe("suspendSession", () => { + it("emits deployment.suspended once for an authenticated session", async () => { + const { manager, telemetrySink } = createTestContext(); + + await manager.setDeployment({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token: "test-token", + user: createMockUser(), + }); + + manager.suspendSession("auth_failure"); + manager.suspendSession("auth_failure"); + + const events = telemetrySink.eventsNamed("deployment.suspended"); + expect(events).toHaveLength(1); + expect(events[0].properties).toEqual({ reason: "auth_failure" }); + }); + it("clears auth state but keeps deployment for re-login", async () => { const { mockClient, @@ -469,7 +499,7 @@ describe("DeploymentManager", () => { it("recovers from suspended state when auth settings change", async () => { vi.useFakeTimers(); try { - const { mockClient, validationMockClient, manager } = + const { mockClient, validationMockClient, telemetrySink, manager } = createTestContext(); const config = new MockConfigurationProvider(); const user = createMockUser(); @@ -494,6 +524,9 @@ describe("DeploymentManager", () => { expect(validationMockClient.getAuthenticatedUser).toHaveBeenCalledTimes( 1, ); + expect(telemetrySink.expectOne("deployment.recovered")).toMatchObject({ + properties: { trigger: "auth_config" }, + }); } finally { vi.useRealTimers(); } @@ -536,8 +569,13 @@ describe("DeploymentManager", () => { }); it("recovers from suspended state when tokens update", async () => { - const { mockClient, validationMockClient, secretsManager, manager } = - createTestContext(); + const { + mockClient, + validationMockClient, + secretsManager, + telemetrySink, + manager, + } = createTestContext(); const user = createMockUser(); validationMockClient.setAuthenticatedUserResponse(user); @@ -564,6 +602,44 @@ describe("DeploymentManager", () => { // Should recover and be authenticated again expect(mockClient.token).toBe("recovered-token"); expect(manager.isAuthenticated()).toBe(true); + expect(telemetrySink.expectOne("deployment.recovered")).toMatchObject({ + properties: { trigger: "token_update" }, + }); + }); + + it("logs failed auth-config recovery", async () => { + vi.useFakeTimers(); + try { + const { validationMockClient, telemetrySink, manager } = + createTestContext(); + const config = new MockConfigurationProvider(); + const user = createMockUser(); + + await manager.setDeployment({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token: "test-token", + user, + }); + manager.suspendSession(); + validationMockClient.setAuthenticatedUserResponse( + new Error("Auth failed"), + ); + + config.set("coder.tlsCertFile", "/path/to/cert.pem"); + await vi.advanceTimersByTimeAsync(CONFIG_CHANGE_DEBOUNCE_MS); + + expect(manager.isAuthenticated()).toBe(false); + expect(telemetrySink.eventsNamed("deployment.recovered")).toHaveLength( + 0, + ); + expect( + telemetrySink.expectOne("deployment.auth_config.recovery_failed") + .properties, + ).toEqual({}); + } finally { + vi.useRealTimers(); + } }); }); }); diff --git a/test/unit/instrumentation/auth.test.ts b/test/unit/instrumentation/auth.test.ts new file mode 100644 index 000000000..1017fc7db --- /dev/null +++ b/test/unit/instrumentation/auth.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from "vitest"; + +import { AuthTelemetry } from "@/instrumentation/auth"; + +import { createTelemetryHarness } from "../../mocks/telemetry"; + +function setup() { + const { sink, service } = createTelemetryHarness(); + return { auth: new AuthTelemetry(service), sink }; +} + +describe("AuthTelemetry", () => { + describe("traceLogin", () => { + it.each([ + { + name: "records success with the returned method", + outcome: { success: true, method: "mtls" } as const, + properties: { + source: "uri", + method: "mtls", + result: "success", + }, + }, + { + name: "records cancellation with the latest traced method", + outcome: { + success: false, + reason: "user_dismissed", + } as const, + traceMethod: "cli_token" as const, + properties: { + source: "uri", + method: "cli_token", + result: "aborted", + reason: "user_dismissed", + }, + }, + { + name: "records auth failures as errors", + outcome: { + success: false, + method: "oauth", + reason: "auth_failed", + } as const, + properties: { + source: "uri", + method: "oauth", + result: "error", + reason: "auth_failed", + }, + }, + ])("$name", async ({ outcome, traceMethod, properties }) => { + const { auth, sink } = setup(); + + await auth.traceLogin("uri", (trace) => { + if (traceMethod) { + trace.setMethod(traceMethod); + } + return Promise.resolve(outcome); + }); + + expect(sink.expectOne("auth.login")).toMatchObject({ properties }); + }); + }); + + describe("traceLogout", () => { + it("emits auth.logout success with duration", async () => { + const { auth, sink } = setup(); + + await auth.traceLogout(() => Promise.resolve({ success: true })); + + const event = sink.expectOne("auth.logout"); + expect(event.properties).toMatchObject({ result: "success" }); + expect(event.properties.reason).toBeUndefined(); + expect(event.error).toBeUndefined(); + expect(event.measurements.durationMs).toEqual(expect.any(Number)); + }); + + it("marks user cancellation as aborted with a bounded reason", async () => { + const { auth, sink } = setup(); + + await auth.traceLogout(() => + Promise.resolve({ + success: false, + reason: "credential_clear_cancelled", + }), + ); + + expect(sink.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "credential_clear_cancelled", + }, + }); + }); + + it("marks credential clear failures as errors with a bounded reason", async () => { + const { auth, sink } = setup(); + + await auth.traceLogout(() => + Promise.resolve({ + success: false, + reason: "credential_clear_failed", + }), + ); + + expect(sink.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "error", + reason: "credential_clear_failed", + }, + }); + }); + }); +}); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 280b12764..03031ccb5 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -191,7 +191,12 @@ describe("LoginCoordinator", () => { safeHostname: TEST_HOSTNAME, }); - expect(result).toEqual({ success: true, user, token: "stored-token" }); + expect(result).toEqual({ + success: true, + method: "stored_token", + user, + token: "stored-token", + }); const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); expect(auth?.token).toBe("stored-token"); @@ -215,7 +220,12 @@ describe("LoginCoordinator", () => { safeHostname: TEST_HOSTNAME, }); - expect(result).toEqual({ success: true, user, token: "new-token" }); + expect(result).toEqual({ + success: true, + method: "cli_token", + user, + token: "new-token", + }); // Verify new token was persisted const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); @@ -259,10 +269,17 @@ describe("LoginCoordinator", () => { safeHostname: TEST_HOSTNAME, }); - // Both should complete with the same result const [result1, result2] = await Promise.all([login1, login2]); - expect(result1.success).toBe(true); - expect(result1).toEqual(result2); + expect(result1).toMatchObject({ + success: true, + method: "cli_token", + token: "new-token", + }); + expect(result2).toMatchObject({ + success: true, + method: "stored_token", + token: "new-token", + }); // Input box should only be shown once (guard prevents duplicate prompts) expect(vscode.window.showInputBox).toHaveBeenCalledTimes(1); @@ -284,7 +301,12 @@ describe("LoginCoordinator", () => { safeHostname: TEST_HOSTNAME, }); - expect(result).toEqual({ success: true, user, token: "" }); + expect(result).toEqual({ + success: true, + method: "mtls", + user, + token: "", + }); // Verify empty string token was persisted const auth = await secretsManager.getSessionAuth(TEST_HOSTNAME); @@ -372,7 +394,12 @@ describe("LoginCoordinator", () => { token: "provided-token", }); - expect(result).toEqual({ success: true, user, token: "provided-token" }); + expect(result).toEqual({ + success: true, + method: "provided_token", + user, + token: "provided-token", + }); }); it("falls back to stored token when provided token is invalid", async () => { @@ -396,7 +423,12 @@ describe("LoginCoordinator", () => { token: "invalid-provided-token", }); - expect(result).toEqual({ success: true, user, token: "stored-token" }); + expect(result).toEqual({ + success: true, + method: "stored_token", + user, + token: "stored-token", + }); }); it("prompts user when both provided and stored tokens are invalid", async () => { @@ -430,6 +462,7 @@ describe("LoginCoordinator", () => { expect(result).toEqual({ success: true, + method: "cli_token", user, token: "user-entered-token", }); @@ -467,6 +500,7 @@ describe("LoginCoordinator", () => { expect(result).toEqual({ success: true, + method: "cli_token", user, token: "user-entered-token", }); @@ -536,7 +570,12 @@ describe("LoginCoordinator", () => { const result = await login(); - expect(result).toEqual({ success: true, user, token: "stored-token" }); + expect(result).toEqual({ + success: true, + method: "stored_token", + user, + token: "stored-token", + }); }); });