From 51c068ee0850b80e3fe4c68de7b492e0c98a0591 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 8 Jun 2026 10:39:07 +0000 Subject: [PATCH 1/6] feat: add auth telemetry lifecycle traces --- src/commands.ts | 172 +++++---- src/core/cliCredentialManager.ts | 101 ++++-- src/core/cliManager.ts | 16 +- src/core/container.ts | 1 + src/deployment/deploymentManager.ts | 53 ++- src/extension.ts | 2 +- src/instrumentation/auth.ts | 82 +++++ src/instrumentation/credentials.ts | 132 +++++++ src/instrumentation/deployment.ts | 31 ++ src/login/loginCoordinator.ts | 49 ++- src/uri/uriHandler.ts | 1 + test/mocks/testHelpers.ts | 4 +- test/unit/commands.test.ts | 334 ++++++++++++++++++ test/unit/core/cliCredentialManager.test.ts | 87 ++++- test/unit/core/cliManager.test.ts | 53 ++- .../unit/deployment/deploymentManager.test.ts | 90 ++++- test/unit/instrumentation/auth.test.ts | 62 ++++ test/unit/login/loginCoordinator.test.ts | 82 +++++ 18 files changed, 1207 insertions(+), 145 deletions(-) create mode 100644 src/instrumentation/credentials.ts create mode 100644 src/instrumentation/deployment.ts create mode 100644 test/unit/commands.test.ts create mode 100644 test/unit/instrumentation/auth.test.ts diff --git a/src/commands.ts b/src/commands.ts index 1a219148f7..4d67c6934e 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -14,6 +14,11 @@ 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 AuthLoginMethod, + type AuthLoginSource, +} from "./instrumentation/auth"; import { reportElapsedProgress, withCancellableProgress, @@ -83,6 +88,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 +115,7 @@ export class Commands { this.loginCoordinator = serviceContainer.getLoginCoordinator(); this.duplicateWorkspaceIpc = serviceContainer.getDuplicateWorkspaceIpc(); this.speedtestPanelFactory = serviceContainer.getSpeedtestPanelFactory(); + this.authTelemetry = new AuthTelemetry(this.telemetryService); } /** @@ -144,60 +151,76 @@ export class Commands { if (this.deploymentManager.isAuthenticated()) { return; } - await this.performLogin(args); + await this.performLogin(args, args?.autoLogin ? "auto_login" : "command"); } - private async performLogin(args?: { - url?: string; - autoLogin?: boolean; - }): Promise { - this.logger.debug("Logging in"); - - const currentDeployment = await this.secretsManager.getCurrentDeployment(); - const url = await maybeAskUrl( - this.mementoManager, - args?.url, - currentDeployment?.url, - ); - if (!url) { - return; // The user aborted. - } + private async performLogin( + args?: { + url?: string; + autoLogin?: boolean; + }, + source: AuthLoginSource = args?.autoLogin ? "auto_login" : "command", + ): Promise { + let method: AuthLoginMethod = "unknown"; + await this.authTelemetry.traceLogin( + source, + () => method, + async () => { + this.logger.debug("Logging in"); + + const currentDeployment = + await this.secretsManager.getCurrentDeployment(); + const url = await maybeAskUrl( + this.mementoManager, + args?.url, + currentDeployment?.url, + ); + if (!url) { + return { success: false, reason: "no_url_provided" }; + } - const safeHostname = toSafeHost(url); - this.logger.debug("Using hostname", safeHostname); + const safeHostname = toSafeHost(url); + this.logger.debug("Using hostname", safeHostname); - const result = await this.loginCoordinator.ensureLoggedIn({ - safeHostname, - url, - autoLogin: args?.autoLogin, - }); + const result = await this.loginCoordinator.ensureLoggedIn({ + safeHostname, + url, + autoLogin: args?.autoLogin, + traceLogin: false, + onLoginMethod: (next) => { + method = next; + }, + }); - if (!result.success) { - return; - } + if (!result.success) { + return result; + } - await this.deploymentManager.setDeployment({ - url, - safeHostname, - token: result.token, - user: result.user, - }); + await this.deploymentManager.setDeployment({ + url, + safeHostname, + token: result.token, + user: result.user, + }); - vscode.window - .showInformationMessage( - `Welcome to Coder, ${result.user.username}!`, - { - detail: - "You can now use the Coder extension to manage your Coder instance.", - }, - "Open Workspace", - ) - .then((action) => { - if (action === "Open Workspace") { - vscode.commands.executeCommand("coder.open"); - } - }); - this.logger.debug("Login complete to deployment:", url); + vscode.window + .showInformationMessage( + `Welcome to Coder, ${result.user.username}!`, + { + detail: + "You can now use the Coder extension to manage your Coder instance.", + }, + "Open Workspace", + ) + .then((action) => { + if (action === "Open Workspace") { + vscode.commands.executeCommand("coder.open"); + } + }); + this.logger.debug("Login complete to deployment:", url); + return { success: true }; + }, + ); } /** @@ -418,32 +441,45 @@ export class Commands { * Log out and clear stored credentials, requiring re-authentication on next login. */ public async logout(): Promise { - if (!this.deploymentManager.isAuthenticated()) { - return; - } + await this.authTelemetry.traceLogout(async () => { + if (!this.deploymentManager.isAuthenticated()) { + return { success: false, reason: "not_authenticated" }; + } - this.logger.debug("Logging out"); + this.logger.debug("Logging out"); - const deployment = this.deploymentManager.getCurrentDeployment(); + const deployment = this.deploymentManager.getCurrentDeployment(); - await this.deploymentManager.clearDeployment(); + await this.deploymentManager.clearDeployment("logout"); - if (deployment) { - await this.cliManager.clearCredentials(deployment.url); - await this.secretsManager.clearAllAuthData(deployment.safeHostname); - } + let credentialFailureCategory: string | undefined; + if (deployment) { + const credentialResult = await this.cliManager.clearCredentials( + deployment.url, + ); + credentialFailureCategory = credentialResult.failureCategory; + await this.secretsManager.clearAllAuthData(deployment.safeHostname); + } - vscode.window - .showInformationMessage("You've been logged out of Coder!", "Login") - .then((action) => { - if (action === "Login") { - this.login().catch((error) => { - this.logger.error("Login failed", error); - }); - } - }); + vscode.window + .showInformationMessage("You've been logged out of Coder!", "Login") + .then((action) => { + if (action === "Login") { + this.login().catch((error) => { + this.logger.error("Login failed", error); + }); + } + }); - this.logger.debug("Logout complete"); + this.logger.debug("Logout complete"); + if (credentialFailureCategory === "aborted") { + return { success: false, reason: "credential_clear_cancelled" }; + } + if (credentialFailureCategory) { + return { success: false, reason: "credential_clear_failed" }; + } + return { success: true }; + }); } /** @@ -452,7 +488,7 @@ export class Commands { */ public async switchDeployment(): Promise { this.logger.debug("Switching deployment"); - await this.performLogin(); + await this.performLogin(undefined, "switch_deployment"); } /** diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index c338c6387b..ba0b12b1c8 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -7,8 +7,20 @@ import * as semver from "semver"; import { isAbortError } from "../error/errorUtils"; import { featureSetForVersion } from "../featureSet"; +import { + CredentialCliError, + CredentialFileError, + CredentialTelemetry, + type CredentialCategory, + type CredentialClearResult, + type CredentialStoreResult, +} 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 +58,16 @@ export function isKeyringSupported(): boolean { * persistence: keyring-backed (via CLI) and file-based (plaintext). */ export class CliCredentialManager { + private readonly credentialTelemetry: CredentialTelemetry; + constructor( private readonly logger: Logger, private readonly resolveBinary: BinaryResolver, private readonly pathResolver: PathResolver, - ) {} + telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, + ) { + this.credentialTelemetry = new CredentialTelemetry(telemetry); + } /** * Store credentials for a deployment URL. Uses the OS keyring when the @@ -59,12 +76,23 @@ 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 { + ): Promise { + return this.credentialTelemetry.traceStore(configs, () => + this.storeTokenInner(url, token, configs, options), + ); + } + + private async storeTokenInner( + url: string, + token: string, + configs: Pick, + options?: { signal?: AbortSignal }, + ): Promise { const binPath = await this.resolveKeyringBinary( url, configs, @@ -72,7 +100,7 @@ export class CliCredentialManager { ); if (!binPath) { await this.writeCredentialFiles(url, token); - return; + return { category: "file" }; } const args = [ @@ -87,9 +115,13 @@ export class CliCredentialManager { signal: options?.signal, }); this.logger.info("Stored token via CLI for", url); + return { category: "keyring" }; } catch (error) { this.logger.warn("Failed to store token via CLI:", error); - throw error; + if (isAbortError(error)) { + throw error; + } + throw new CredentialCliError(error); } } @@ -139,15 +171,32 @@ export class CliCredentialManager { * deletion in parallel, both best-effort. Throws AbortError when the * signal is aborted. */ - public async deleteToken( + public deleteToken( url: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - await Promise.all([ + ): Promise { + return this.credentialTelemetry.traceClear(configs, () => + this.deleteTokenInner(url, configs, options), + ); + } + + private async deleteTokenInner( + url: string, + configs: Pick, + options?: { signal?: AbortSignal }, + ): Promise { + const [filesResult, keyringResult] = await Promise.all([ this.deleteCredentialFiles(url), this.deleteKeyringToken(url, configs, options?.signal), ]); + return { + category: + keyringResult.used || keyringResult.failureCategory + ? "keyring" + : filesResult.category, + failureCategory: keyringResult.failureCategory, + }; } /** @@ -201,20 +250,26 @@ export class CliCredentialManager { 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, - ), - ]); + 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 { + private async deleteCredentialFiles( + url: string, + ): Promise<{ category: CredentialCategory }> { const safeHostname = toSafeHost(url); const paths = [ this.pathResolver.getSessionTokenPath(safeHostname), @@ -227,6 +282,7 @@ export class CliCredentialManager { }), ), ); + return { category: "file" }; } /** @@ -236,27 +292,32 @@ export class CliCredentialManager { url: string, configs: Pick, signal?: AbortSignal, - ): Promise { + ): Promise<{ + used: boolean; + failureCategory?: CredentialClearResult["failureCategory"]; + }> { 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; + return { used: false, failureCategory: "binary" }; } if (!binPath) { - return; + return { used: false }; } const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"]; try { await this.execWithTimeout(binPath, args, { signal }); this.logger.info("Deleted token via CLI for", url); + return { used: true }; } catch (error) { if (isAbortError(error)) { throw error; } this.logger.warn("Failed to delete token via CLI:", error); + return { used: true, failureCategory: "cli" }; } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index b65f0f0a6c..4ee2379eac 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,17 @@ 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 { category: "keyring", failureCategory: "aborted" }; } + this.output.warn("Failed to remove credentials:", result.error); + return { + category: isKeyringEnabled(configs) ? "keyring" : "file", + failureCategory: categorizeCredentialError(result.error), + }; } private handleStoreError(error: unknown): void { diff --git a/src/core/container.ts b/src/core/container.ts index 8586869249..b18b54d87d 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 5868ecb91c..a979348711 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 c8e7fb8f3e..5903d79cfd 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 c6cba325c1..5044e23068 100644 --- a/src/instrumentation/auth.ts +++ b/src/instrumentation/auth.ts @@ -3,15 +3,41 @@ import type { TelemetryReporter } from "../telemetry/reporter"; 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" + | "legacy_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 } + | { success: false; reason: AuthLoginReason }; +export type AuthLogoutOutcome = + | { success: true } + | { success: false; reason: AuthLogoutReason }; interface AuthRecoveryRecorder { logReceived(): void; @@ -22,6 +48,62 @@ interface AuthRecoveryRecorder { export class AuthTelemetry { public constructor(private readonly telemetry: TelemetryReporter) {} + public traceLogin( + source: AuthLoginSource, + getMethod: () => AuthLoginMethod, + fn: () => Promise, + ): Promise { + return this.telemetry.trace( + "auth.login", + async (span) => { + const setMethod = () => span.setProperty("method", getMethod()); + try { + const result = await fn(); + setMethod(); + if (!result.success) { + span.setProperty("reason", result.reason); + if (result.reason === "auth_failed") { + span.markFailure(); + } else { + span.markAborted(); + } + } + return result; + } catch (error) { + setMethod(); + 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(); + if (!result.success) { + span.setProperty("reason", result.reason); + if ( + result.reason === "not_authenticated" || + result.reason === "credential_clear_cancelled" + ) { + span.markAborted(); + } else { + span.markFailure(); + } + } + return result; + } catch (error) { + span.setProperty("reason", "exception"); + throw error; + } + }); + } + public traceTokenRefresh( trigger: AuthTokenRefreshTrigger, fn: () => Promise, diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts new file mode 100644 index 0000000000..fb489c9182 --- /dev/null +++ b/src/instrumentation/credentials.ts @@ -0,0 +1,132 @@ +import { isAbortError } from "../error/errorUtils"; +import { isKeyringEnabled } from "../settings/cli"; + +import type { WorkspaceConfiguration } from "vscode"; + +import type { TelemetryReporter } from "../telemetry/reporter"; + +export type CredentialCategory = "keyring" | "file"; +export type CredentialFailureCategory = "aborted" | "binary" | "cli" | "file"; + +export interface CredentialStoreResult { + readonly category: CredentialCategory; +} + +export interface CredentialClearResult { + readonly category: CredentialCategory; + readonly failureCategory?: CredentialFailureCategory; +} + +export class CredentialTelemetry { + public constructor(private readonly telemetry: TelemetryReporter) {} + + public async traceStore( + configs: Pick, + fn: () => Promise, + ): Promise { + const keyringEnabled = isKeyringEnabled(configs); + const defaultCategory: CredentialCategory = keyringEnabled + ? "keyring" + : "file"; + let cancellation: unknown; + const result = await this.telemetry.trace( + "auth.credential_stored", + async (span) => { + try { + const result = await fn(); + span.setProperty("category", result.category); + return result; + } catch (error) { + span.setProperty("category", defaultCategory); + span.setProperty("failureCategory", categorizeCredentialError(error)); + if (isAbortError(error)) { + span.markAborted(); + cancellation = error; + return { category: defaultCategory } as T; + } + throw error; + } + }, + { keyringEnabled, category: defaultCategory }, + ); + if (cancellation instanceof Error) { + throw cancellation; + } + return result; + } + + public async traceClear( + configs: Pick, + fn: () => Promise, + ): Promise { + const keyringEnabled = isKeyringEnabled(configs); + const defaultCategory: CredentialCategory = keyringEnabled + ? "keyring" + : "file"; + let cancellation: unknown; + const result = await this.telemetry.trace( + "auth.credential_cleared", + async (span) => { + try { + const result = await fn(); + span.setProperty("category", result.category); + if (result.failureCategory) { + span.setProperty("failureCategory", result.failureCategory); + if (result.failureCategory === "aborted") { + span.markAborted(); + } else { + span.markFailure(); + } + } + return result; + } catch (error) { + span.setProperty("category", defaultCategory); + span.setProperty("failureCategory", categorizeCredentialError(error)); + if (isAbortError(error)) { + span.markAborted(); + cancellation = error; + return { + category: defaultCategory, + failureCategory: "aborted", + } as T; + } + throw error; + } + }, + { keyringEnabled, category: defaultCategory }, + ); + if (cancellation instanceof Error) { + throw cancellation; + } + return result; + } +} + +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 0000000000..5aae289162 --- /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 baac3f28d9..df560dfc40 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -20,7 +20,9 @@ import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; import type { + AuthLoginMethod, AuthLoginPromptTrigger, + AuthLoginSource, AuthTelemetry, LoginPromptReason, } from "../instrumentation/auth"; @@ -36,6 +38,9 @@ export interface LoginOptions { url: string | undefined; autoLogin?: boolean; token?: string; + source?: AuthLoginSource; + traceLogin?: boolean; + onLoginMethod?: (method: AuthLoginMethod) => void; } /** @@ -70,17 +75,34 @@ export class LoginCoordinator implements vscode.Disposable { options: LoginOptions & { url: string }, ): Promise { const { safeHostname, url } = options; - return this.executeWithGuard(async () => { - const result = await this.attemptLogin( - { safeHostname, url }, - options.autoLogin ?? false, - options.token, - ); + let method: AuthLoginMethod = "unknown"; + const setMethod = (next: AuthLoginMethod): void => { + method = next; + options.onLoginMethod?.(next); + }; + const login = () => + this.executeWithGuard(async () => { + const result = await this.attemptLogin( + { safeHostname, url }, + options.autoLogin ?? false, + options.token, + setMethod, + ); - await this.persistSessionAuth(result, safeHostname, url); + await this.persistSessionAuth(result, safeHostname, url); - return result; - }); + return result; + }); + + if (options.traceLogin === false) { + return login(); + } + + return this.authTelemetry.traceLogin( + options.source ?? "direct", + () => method, + login, + ); } /** @@ -242,6 +264,7 @@ export class LoginCoordinator implements vscode.Disposable { deployment: Deployment, isAutoLogin: boolean, providedToken?: string, + recordMethod: (method: AuthLoginMethod) => void = () => undefined, ): Promise { const client = CoderApi.create(deployment.url, "", this.logger); try { @@ -250,6 +273,7 @@ export class LoginCoordinator implements vscode.Disposable { deployment, isAutoLogin, providedToken, + recordMethod, ); } finally { client.dispose(); @@ -261,10 +285,12 @@ export class LoginCoordinator implements vscode.Disposable { deployment: Deployment, isAutoLogin: boolean, providedToken: string | undefined, + recordMethod: (method: AuthLoginMethod) => void, ): Promise { // mTLS authentication (no token needed) if (!needToken(vscode.workspace.getConfiguration())) { this.logger.debug("Attempting mTLS authentication (no token required)"); + recordMethod("mtls"); return this.tryMtlsAuth(client, isAutoLogin); } @@ -277,6 +303,7 @@ export class LoginCoordinator implements vscode.Disposable { isAutoLogin, ); if (result !== "unauthorized") { + recordMethod("provided_token"); return result; } } @@ -289,6 +316,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") { + recordMethod("stored_token"); return result; } } @@ -316,6 +344,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") { + recordMethod("keyring_token"); return result; } } @@ -324,8 +353,10 @@ export class LoginCoordinator implements vscode.Disposable { const authMethod = await maybeAskAuthMethod(client); switch (authMethod) { case "oauth": + recordMethod("oauth"); return this.loginWithOAuth(deployment); case "legacy": + recordMethod("legacy_token"); return this.loginWithToken(client); case undefined: return { success: false, reason: "user_dismissed" }; diff --git a/src/uri/uriHandler.ts b/src/uri/uriHandler.ts index a846b4ef49..74cd9acd9e 100644 --- a/src/uri/uriHandler.ts +++ b/src/uri/uriHandler.ts @@ -155,6 +155,7 @@ async function setupDeployment( safeHostname, url, token, + source: "uri", }); if (!result.success) { diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index f715d9b4a6..590f5424f9 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -444,9 +444,9 @@ export class InMemorySecretStorage implements vscode.SecretStorage { export function createMockCliCredentialManager(): CliCredentialManager { return { - storeToken: vi.fn().mockResolvedValue(undefined), + storeToken: vi.fn().mockResolvedValue({ category: "file" }), readToken: vi.fn().mockResolvedValue(undefined), - deleteToken: vi.fn().mockResolvedValue(undefined), + deleteToken: vi.fn().mockResolvedValue({ category: "file" }), } as unknown as CliCredentialManager; } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts new file mode 100644 index 0000000000..184af3bde2 --- /dev/null +++ b/test/unit/commands.test.ts @@ -0,0 +1,334 @@ +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 { + AuthLoginMethod, + LoginPromptReason, +} from "@/instrumentation/auth"; +import type { LoginCoordinator } 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; + traceLogin?: boolean; + onLoginMethod?: (method: AuthLoginMethod) => void; +} + +type LoginResultForTest = + | { success: true; user: ReturnType; token: string } + | { success: false; reason: LoginPromptReason }; + +interface SetupOptions { + readonly authenticated?: boolean; + readonly loginMethod?: AuthLoginMethod; + readonly loginResult?: LoginResultForTest; + readonly credentialResult?: Awaited< + ReturnType + >; + readonly clearDeploymentError?: Error; + readonly clearAllAuthDataError?: Error; +} + +function setup(options: SetupOptions = {}) { + 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, + user: createMockUser(), + token: "test-token", + } satisfies LoginResultForTest); + const loginMethod = options.loginMethod ?? "stored_token"; + + const ensureLoggedIn = vi.fn( + (loginOptions: LoginOptionsForTest): Promise => { + loginOptions.onLoginMethod?.(loginMethod); + return Promise.resolve(loginResult); + }, + ); + const loginCoordinator = { + ensureLoggedIn, + } as unknown as LoginCoordinator; + + 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(); + }), + } as unknown as DeploymentManager; + + const cliManager = { + clearCredentials: vi.fn(() => + Promise.resolve(options.credentialResult ?? { category: "file" }), + ), + } as unknown as CliManager; + + const secretsManager = { + getCurrentDeployment: vi.fn(() => Promise.resolve(null)), + clearAllAuthData: vi.fn(() => { + if (options.clearAllAuthDataError) { + return Promise.reject(options.clearAllAuthDataError); + } + return Promise.resolve(); + }), + } as unknown as SecretsManager; + + const serviceContainer = { + getTelemetryService: () => telemetry, + getLogger: () => logger, + getPathResolver: () => ({}) as PathResolver, + getMementoManager: () => ({}) as MementoManager, + getSecretsManager: () => secretsManager, + getCliManager: () => cliManager, + getLoginCoordinator: () => 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, + ); + + return { + commands, + deployment, + deploymentManager, + ensureLoggedIn, + cliManager, + secretsManager, + sink, + }; +} + +describe("Commands", () => { + describe("login telemetry", () => { + it("emits one auth.login for command login success", async () => { + const { commands, deploymentManager, ensureLoggedIn, sink } = setup(); + + await commands.login(); + + const events = sink.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(ensureLoggedIn).toHaveBeenCalledWith( + expect.objectContaining({ + safeHostname: TEST_HOSTNAME, + url: TEST_URL, + traceLogin: false, + }), + ); + expect(deploymentManager.setDeployment).toHaveBeenCalled(); + }); + + it("uses auto_login source when requested", async () => { + const { commands, ensureLoggedIn, sink } = setup({ + loginMethod: "provided_token", + }); + + await commands.login({ url: TEST_URL, autoLogin: true }); + + expect(sink.expectOne("auth.login").properties).toMatchObject({ + source: "auto_login", + method: "provided_token", + result: "success", + }); + expect(ensureLoggedIn).toHaveBeenCalledWith( + expect.objectContaining({ autoLogin: true, traceLogin: false }), + ); + }); + + it("records URL cancellation without attempting login", async () => { + const { commands, ensureLoggedIn, sink } = setup(); + vi.mocked(maybeAskUrl).mockResolvedValueOnce(undefined); + + await commands.login(); + + expect(sink.expectOne("auth.login")).toMatchObject({ + properties: { + source: "command", + method: "unknown", + result: "aborted", + reason: "no_url_provided", + }, + }); + expect(ensureLoggedIn).not.toHaveBeenCalled(); + }); + + it("records auth failures as auth.login errors", async () => { + const { commands, deploymentManager, sink } = setup({ + loginMethod: "legacy_token", + loginResult: { success: false, reason: "auth_failed" }, + }); + + await commands.login(); + + expect(sink.expectOne("auth.login")).toMatchObject({ + properties: { + method: "legacy_token", + result: "error", + reason: "auth_failed", + }, + }); + expect(deploymentManager.setDeployment).not.toHaveBeenCalled(); + }); + + it("uses switch_deployment source", async () => { + const { commands, sink } = setup(); + + await commands.switchDeployment(); + + expect(sink.expectOne("auth.login").properties).toMatchObject({ + source: "switch_deployment", + result: "success", + }); + }); + }); + + describe("logout telemetry", () => { + it("records not_authenticated as aborted", async () => { + const { commands, cliManager, sink } = setup({ authenticated: false }); + + await commands.logout(); + + expect(sink.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "not_authenticated", + }, + }); + expect(cliManager.clearCredentials).not.toHaveBeenCalled(); + }); + + it("records successful logout", async () => { + const { commands, deploymentManager, cliManager, secretsManager, sink } = + setup({ authenticated: true }); + + await commands.logout(); + + const event = sink.expectOne("auth.logout"); + expect(event.properties).toMatchObject({ result: "success" }); + expect(event.properties.reason).toBeUndefined(); + expect(event.measurements.durationMs).toEqual(expect.any(Number)); + expect(deploymentManager.clearDeployment).toHaveBeenCalledWith("logout"); + expect(cliManager.clearCredentials).toHaveBeenCalledWith(TEST_URL); + expect(secretsManager.clearAllAuthData).toHaveBeenCalledWith( + TEST_HOSTNAME, + ); + }); + + it("records credential clear cancellation as aborted", async () => { + const { commands, sink } = setup({ + authenticated: true, + credentialResult: { + category: "keyring", + failureCategory: "aborted", + }, + }); + + await commands.logout(); + + expect(sink.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "credential_clear_cancelled", + }, + }); + }); + + it("records credential clear failure as an error", async () => { + const { commands, sink } = setup({ + authenticated: true, + credentialResult: { + category: "keyring", + failureCategory: "cli", + }, + }); + + await commands.logout(); + + expect(sink.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "error", + reason: "credential_clear_failed", + }, + }); + }); + + it("records logout exceptions", async () => { + const { commands, sink } = setup({ + authenticated: true, + clearAllAuthDataError: new Error("secret clear failed"), + }); + + await expect(commands.logout()).rejects.toThrow("secret clear failed"); + expect(sink.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 1167654bda..57175b640f 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_stored")).toMatchObject({ + properties: { + category: "file", + keyringEnabled: "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_stored")).toMatchObject({ + properties: { + category: "keyring", + keyringEnabled: "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_stored")).toMatchObject({ + properties: { + failureCategory: "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_stored")).toMatchObject({ + properties: { + failureCategory: "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_cleared")).toMatchObject({ + properties: { + category: "keyring", + keyringEnabled: "true", + result: "success", + }, + }); }); it("deletes files even when keyring is disabled", async () => { @@ -395,21 +436,35 @@ 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_cleared")).toMatchObject({ + properties: { + failureCategory: "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({ + category: "keyring", + failureCategory: "binary", + }); expect(execFile).not.toHaveBeenCalled(); + expect(sink.expectOne("auth.credential_cleared")).toMatchObject({ + properties: { + category: "keyring", + failureCategory: "binary", + result: "error", + }, + }); }); it("forwards header command args", async () => { @@ -450,13 +505,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_cleared")).toMatchObject({ + properties: { + failureCategory: "aborted", + result: "aborted", + }, + }); }); }); }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 4e0c185d1b..32b977f5db 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -353,7 +353,9 @@ 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({ + category: "file", + }); expect(vscode.window.withProgress).not.toHaveBeenCalled(); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( @@ -365,8 +367,13 @@ describe("CliManager", () => { it("should show progress notification when keyring is enabled", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); + vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce({ + category: "keyring", + }); - await manager.clearCredentials(CLEAR_URL); + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ + category: "keyring", + }); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ @@ -378,15 +385,39 @@ 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({ + category: "keyring", + failureCategory: "cli", + }); + + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ + category: "keyring", + 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({ + category: "file", + 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({ + category: "keyring", + failureCategory: "aborted", + }); }); }); diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 2e90287e13..53dca31eb3 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 0000000000..ebcafc8802 --- /dev/null +++ b/test/unit/instrumentation/auth.test.ts @@ -0,0 +1,62 @@ +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("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 280b127648..2845af6683 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -541,6 +541,17 @@ describe("LoginCoordinator", () => { }); describe("telemetry", () => { + function setupTelemetryContext() { + const sink = new TestSink(); + const ctx = createTestContext(createTestTelemetryService(sink)); + return { ...ctx, sink }; + } + + const loginOptions = () => ({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }); + const dialogOptions = (trigger: "auth_required" | "missing_session") => ({ url: TEST_URL, safeHostname: TEST_HOSTNAME, @@ -562,6 +573,77 @@ describe("LoginCoordinator", () => { }; } + it("records auth.login success with source, method, result, and duration", async () => { + const { mockConfig, coordinator, mockSuccessfulAuth, sink } = + setupTelemetryContext(); + enableMTLS(mockConfig); + mockSuccessfulAuth(); + + await coordinator.ensureLoggedIn({ + ...loginOptions(), + source: "uri", + }); + + const event = sink.expectOne("auth.login"); + expect(event.properties).toMatchObject({ + source: "uri", + method: "mtls", + result: "success", + }); + expect(event.properties.reason).toBeUndefined(); + expect(event.error).toBeUndefined(); + expect(event.measurements.durationMs).toEqual(expect.any(Number)); + }); + + it("records auth.login cancellation with bounded reason", async () => { + const { userInteraction, coordinator, mockAuthFailure, sink } = + setupTelemetryContext(); + mockAuthFailure(); + userInteraction.setInputBoxValue(undefined); + + await coordinator.ensureLoggedIn(loginOptions()); + + const event = sink.expectOne("auth.login"); + expect(event.properties).toMatchObject({ + source: "direct", + method: "legacy_token", + result: "aborted", + reason: "user_dismissed", + }); + expect(event.error).toBeUndefined(); + }); + + it("records auth.login auth failures as errors with bounded reason", async () => { + const { mockConfig, coordinator, mockAuthFailure, sink } = + setupTelemetryContext(); + enableMTLS(mockConfig); + mockAuthFailure("Certificate error"); + + await coordinator.ensureLoggedIn(loginOptions()); + + const event = sink.expectOne("auth.login"); + expect(event.properties).toMatchObject({ + method: "mtls", + result: "error", + reason: "auth_failed", + }); + expect(event.error).toBeUndefined(); + }); + + it("does not duplicate auth.login when tracing is disabled by caller", async () => { + const { mockConfig, coordinator, mockSuccessfulAuth, sink } = + setupTelemetryContext(); + enableMTLS(mockConfig); + mockSuccessfulAuth(); + + await coordinator.ensureLoggedIn({ + ...loginOptions(), + traceLogin: false, + }); + + expect(sink.eventsNamed("auth.login")).toHaveLength(0); + }); + it.each([ { name: "user dismisses the dialog: aborted + user_dismissed", From 20ab3f734dbe803210bde81cd34ad6d69e79a2e4 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 8 Jun 2026 11:42:10 +0000 Subject: [PATCH 2/6] refactor: isolate auth telemetry wrappers --- src/commands.ts | 238 ++++++++++++++---------- src/login/loginCoordinator.ts | 68 ++++--- test/unit/commands.test.ts | 334 ++++++++++++++++++---------------- 3 files changed, 369 insertions(+), 271 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 4d67c6934e..887f322a6c 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -17,7 +17,9 @@ import { type FeatureSet, featureSetForVersion } from "./featureSet"; import { AuthTelemetry, type AuthLoginMethod, + type AuthLoginOutcome, type AuthLoginSource, + type AuthLogoutOutcome, } from "./instrumentation/auth"; import { reportElapsedProgress, @@ -42,6 +44,7 @@ import { } from "./workspace/workspacesProvider"; import type { + User, Workspace, WorkspaceAgent, } from "coder/site/src/api/typesGenerated"; @@ -53,6 +56,8 @@ 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 { TelemetryService } from "./telemetry/service"; @@ -73,6 +78,18 @@ interface OpenOptions { useDefaultDirectory?: boolean; } +interface LoginArgs { + readonly url?: string; + readonly autoLogin?: boolean; +} + +type LoginMethodRecorder = (method: AuthLoginMethod) => void; + +interface LoginSuccess { + readonly user: User; + readonly token: string; +} + const openDefaults = { openRecent: false, useDefaultDirectory: true, @@ -144,83 +161,97 @@ 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, args?.autoLogin ? "auto_login" : "command"); + await this.traceLoginCommand( + args?.autoLogin ? "auto_login" : "command", + (recordMethod) => this.performLogin(args, recordMethod), + ); } - private async performLogin( - args?: { - url?: string; - autoLogin?: boolean; - }, - source: AuthLoginSource = args?.autoLogin ? "auto_login" : "command", + private async traceLoginCommand( + source: AuthLoginSource, + run: (recordMethod: LoginMethodRecorder) => Promise, ): Promise { let method: AuthLoginMethod = "unknown"; await this.authTelemetry.traceLogin( source, () => method, - async () => { - this.logger.debug("Logging in"); - - const currentDeployment = - await this.secretsManager.getCurrentDeployment(); - const url = await maybeAskUrl( - this.mementoManager, - args?.url, - currentDeployment?.url, - ); - if (!url) { - return { success: false, reason: "no_url_provided" }; - } + () => + run((next) => { + method = next; + }), + ); + } + + private async performLogin( + args: LoginArgs | undefined, + recordMethod: LoginMethodRecorder, + ): Promise { + this.logger.debug("Logging in"); + + const currentDeployment = await this.secretsManager.getCurrentDeployment(); + const url = await maybeAskUrl( + this.mementoManager, + args?.url, + currentDeployment?.url, + ); + if (!url) { + return { success: false, reason: "no_url_provided" }; + } - const safeHostname = toSafeHost(url); - this.logger.debug("Using hostname", safeHostname); + const safeHostname = toSafeHost(url); + this.logger.debug("Using hostname", safeHostname); - const result = await this.loginCoordinator.ensureLoggedIn({ - safeHostname, - url, - autoLogin: args?.autoLogin, - traceLogin: false, - onLoginMethod: (next) => { - method = next; - }, - }); + const result = await this.loginCoordinator.ensureLoggedIn({ + safeHostname, + url, + autoLogin: args?.autoLogin, + traceLogin: false, + onLoginMethod: recordMethod, + }); - if (!result.success) { - return result; - } + if (!result.success) { + return result; + } - await this.deploymentManager.setDeployment({ - url, - safeHostname, - token: result.token, - user: result.user, - }); + await this.completeLogin(url, safeHostname, result); + return { success: true }; + } - vscode.window - .showInformationMessage( - `Welcome to Coder, ${result.user.username}!`, - { - detail: - "You can now use the Coder extension to manage your Coder instance.", - }, - "Open Workspace", - ) - .then((action) => { - if (action === "Open Workspace") { - vscode.commands.executeCommand("coder.open"); - } - }); - this.logger.debug("Login complete to deployment:", url); - return { success: true }; - }, - ); + private async completeLogin( + url: string, + safeHostname: string, + result: LoginSuccess, + ): Promise { + await this.deploymentManager.setDeployment({ + url, + safeHostname, + token: result.token, + 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, ${username}!`, + { + detail: + "You can now use the Coder extension to manage your Coder instance.", + }, + "Open Workspace", + ) + .then((action) => { + if (action === "Open Workspace") { + vscode.commands.executeCommand("coder.open"); + } + }); } /** @@ -441,45 +472,46 @@ export class Commands { * Log out and clear stored credentials, requiring re-authentication on next login. */ public async logout(): Promise { - await this.authTelemetry.traceLogout(async () => { - if (!this.deploymentManager.isAuthenticated()) { - return { success: false, reason: "not_authenticated" }; - } + await this.authTelemetry.traceLogout(() => this.performLogout()); + } - this.logger.debug("Logging out"); + private async performLogout(): Promise { + if (!this.deploymentManager.isAuthenticated()) { + return { success: false, reason: "not_authenticated" }; + } - const deployment = this.deploymentManager.getCurrentDeployment(); + this.logger.debug("Logging out"); - await this.deploymentManager.clearDeployment("logout"); + const deployment = this.deploymentManager.getCurrentDeployment(); + await this.deploymentManager.clearDeployment("logout"); - let credentialFailureCategory: string | undefined; - if (deployment) { - const credentialResult = await this.cliManager.clearCredentials( - deployment.url, - ); - credentialFailureCategory = credentialResult.failureCategory; - await this.secretsManager.clearAllAuthData(deployment.safeHostname); - } + const credentialFailureCategory = deployment + ? await this.clearDeploymentCredentials(deployment) + : undefined; - vscode.window - .showInformationMessage("You've been logged out of Coder!", "Login") - .then((action) => { - if (action === "Login") { - this.login().catch((error) => { - this.logger.error("Login failed", error); - }); - } - }); + this.showLogoutMessage(); + this.logger.debug("Logout complete"); + return logoutResultForCredentialFailure(credentialFailureCategory); + } - this.logger.debug("Logout complete"); - if (credentialFailureCategory === "aborted") { - return { success: false, reason: "credential_clear_cancelled" }; - } - if (credentialFailureCategory) { - return { success: false, reason: "credential_clear_failed" }; - } - return { success: true }; - }); + 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) => { + if (action === "Login") { + this.login().catch((error) => { + this.logger.error("Login failed", error); + }); + } + }); } /** @@ -488,7 +520,9 @@ export class Commands { */ public async switchDeployment(): Promise { this.logger.debug("Switching deployment"); - await this.performLogin(undefined, "switch_deployment"); + await this.traceLoginCommand("switch_deployment", (recordMethod) => + this.performLogin(undefined, recordMethod), + ); } /** @@ -1254,6 +1288,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/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index df560dfc40..0b30527773 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -33,6 +33,13 @@ type LoginResult = | { success: false; reason: LoginPromptReason } | { success: true; user: User; token: string; oauth?: OAuthTokenData }; +type LoginMethodRecorder = (method: AuthLoginMethod) => void; + +interface LoginMethodTracker { + readonly record: LoginMethodRecorder; + readonly current: () => AuthLoginMethod; +} + export interface LoginOptions { safeHostname: string; url: string | undefined; @@ -71,28 +78,11 @@ export class LoginCoordinator implements vscode.Disposable { * Direct login - for user-initiated login via commands. * Stores session auth and URL history on success. */ - public async ensureLoggedIn( + public ensureLoggedIn( options: LoginOptions & { url: string }, ): Promise { - const { safeHostname, url } = options; - let method: AuthLoginMethod = "unknown"; - const setMethod = (next: AuthLoginMethod): void => { - method = next; - options.onLoginMethod?.(next); - }; - const login = () => - this.executeWithGuard(async () => { - const result = await this.attemptLogin( - { safeHostname, url }, - options.autoLogin ?? false, - options.token, - setMethod, - ); - - await this.persistSessionAuth(result, safeHostname, url); - - return result; - }); + const method = createLoginMethodTracker(options.onLoginMethod); + const login = () => this.ensureLoggedInCore(options, method.record); if (options.traceLogin === false) { return login(); @@ -100,11 +90,30 @@ export class LoginCoordinator implements vscode.Disposable { return this.authTelemetry.traceLogin( options.source ?? "direct", - () => method, + method.current, login, ); } + private ensureLoggedInCore( + options: LoginOptions & { url: string }, + recordMethod: LoginMethodRecorder, + ): Promise { + const { safeHostname, url } = options; + return this.executeWithGuard(async () => { + const result = await this.attemptLogin( + { safeHostname, url }, + options.autoLogin ?? false, + options.token, + recordMethod, + ); + + await this.persistSessionAuth(result, safeHostname, url); + + return result; + }); + } + /** * Shows dialog then login - for system-initiated auth (remote, OAuth refresh). */ @@ -264,7 +273,7 @@ export class LoginCoordinator implements vscode.Disposable { deployment: Deployment, isAutoLogin: boolean, providedToken?: string, - recordMethod: (method: AuthLoginMethod) => void = () => undefined, + recordMethod: LoginMethodRecorder = () => undefined, ): Promise { const client = CoderApi.create(deployment.url, "", this.logger); try { @@ -285,7 +294,7 @@ export class LoginCoordinator implements vscode.Disposable { deployment: Deployment, isAutoLogin: boolean, providedToken: string | undefined, - recordMethod: (method: AuthLoginMethod) => void, + recordMethod: LoginMethodRecorder, ): Promise { // mTLS authentication (no token needed) if (!needToken(vscode.workspace.getConfiguration())) { @@ -523,3 +532,16 @@ export class LoginCoordinator implements vscode.Disposable { this.oauthAuthorizer.dispose(); } } + +function createLoginMethodTracker( + onRecord?: LoginMethodRecorder, +): LoginMethodTracker { + let method: AuthLoginMethod = "unknown"; + return { + record: (next) => { + method = next; + onRecord?.(next); + }, + current: () => method, + }; +} diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 184af3bde2..1399635c79 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -23,6 +23,7 @@ import type { AuthLoginMethod, LoginPromptReason, } from "@/instrumentation/auth"; +import type { CredentialClearResult } from "@/instrumentation/credentials"; import type { LoginCoordinator } from "@/login/loginCoordinator"; import type { SpeedtestPanelFactory } from "@/webviews/speedtest/speedtestPanelFactory"; import type { DuplicateWorkspaceIpc } from "@/workspace/duplicateWorkspaceIpc"; @@ -53,18 +54,16 @@ type LoginResultForTest = | { success: true; user: ReturnType; token: string } | { success: false; reason: LoginPromptReason }; -interface SetupOptions { +interface CommandsHarnessOptions { readonly authenticated?: boolean; readonly loginMethod?: AuthLoginMethod; readonly loginResult?: LoginResultForTest; - readonly credentialResult?: Awaited< - ReturnType - >; + readonly credentialResult?: CredentialClearResult; readonly clearDeploymentError?: Error; readonly clearAllAuthDataError?: Error; } -function setup(options: SetupOptions = {}) { +function createCommandsHarness(options: CommandsHarnessOptions = {}) { vi.clearAllMocks(); new MockConfigurationProvider(); new MockUserInteraction(); @@ -86,15 +85,12 @@ function setup(options: SetupOptions = {}) { } satisfies LoginResultForTest); const loginMethod = options.loginMethod ?? "stored_token"; - const ensureLoggedIn = vi.fn( - (loginOptions: LoginOptionsForTest): Promise => { + const loginCoordinator = { + ensureLoggedIn: vi.fn((loginOptions: LoginOptionsForTest) => { loginOptions.onLoginMethod?.(loginMethod); return Promise.resolve(loginResult); - }, - ); - const loginCoordinator = { - ensureLoggedIn, - } as unknown as LoginCoordinator; + }), + }; const deploymentManager = { isAuthenticated: vi.fn(() => options.authenticated ?? false), @@ -106,13 +102,13 @@ function setup(options: SetupOptions = {}) { } return Promise.resolve(); }), - } as unknown as DeploymentManager; + }; const cliManager = { clearCredentials: vi.fn(() => Promise.resolve(options.credentialResult ?? { category: "file" }), ), - } as unknown as CliManager; + }; const secretsManager = { getCurrentDeployment: vi.fn(() => Promise.resolve(null)), @@ -122,16 +118,16 @@ function setup(options: SetupOptions = {}) { } return Promise.resolve(); }), - } as unknown as SecretsManager; + }; const serviceContainer = { getTelemetryService: () => telemetry, getLogger: () => logger, getPathResolver: () => ({}) as PathResolver, getMementoManager: () => ({}) as MementoManager, - getSecretsManager: () => secretsManager, - getCliManager: () => cliManager, - getLoginCoordinator: () => loginCoordinator, + 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; @@ -143,189 +139,223 @@ function setup(options: SetupOptions = {}) { const commands = new Commands( serviceContainer, extensionClient, - deploymentManager, + deploymentManager as unknown as DeploymentManager, ); return { commands, deployment, - deploymentManager, - ensureLoggedIn, - cliManager, - secretsManager, - sink, + telemetry: sink, + mocks: { + cliManager, + deploymentManager, + loginCoordinator, + secretsManager, + }, }; } -describe("Commands", () => { - describe("login telemetry", () => { - it("emits one auth.login for command login success", async () => { - const { commands, deploymentManager, ensureLoggedIn, sink } = setup(); +type CommandsHarness = ReturnType; - await commands.login(); +interface CommandScenario { + readonly name: string; + readonly options?: CommandsHarnessOptions; + readonly arrange?: (harness: CommandsHarness) => void; + readonly act: (harness: CommandsHarness) => Promise; + readonly assert: (harness: CommandsHarness) => void; +} - const events = sink.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(ensureLoggedIn).toHaveBeenCalledWith( - expect.objectContaining({ - safeHostname: TEST_HOSTNAME, - url: TEST_URL, - traceLogin: false, - }), - ); - expect(deploymentManager.setDeployment).toHaveBeenCalled(); - }); +function testCommandScenario(scenario: CommandScenario): void { + it(scenario.name, async () => { + const harness = createCommandsHarness(scenario.options); + scenario.arrange?.(harness); - it("uses auto_login source when requested", async () => { - const { commands, ensureLoggedIn, sink } = setup({ - loginMethod: "provided_token", - }); + await scenario.act(harness); - await commands.login({ url: TEST_URL, autoLogin: true }); + scenario.assert(harness); + }); +} - expect(sink.expectOne("auth.login").properties).toMatchObject({ - source: "auto_login", - method: "provided_token", - result: "success", - }); - expect(ensureLoggedIn).toHaveBeenCalledWith( - expect.objectContaining({ autoLogin: true, traceLogin: false }), - ); +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, + traceLogin: false, + }), + ); + expect(mocks.deploymentManager.setDeployment).toHaveBeenCalled(); + }, }); - it("records URL cancellation without attempting login", async () => { - const { commands, ensureLoggedIn, sink } = setup(); - vi.mocked(maybeAskUrl).mockResolvedValueOnce(undefined); - - await commands.login(); + testCommandScenario({ + name: "uses auto_login source when requested", + options: { loginMethod: "provided_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, traceLogin: false }), + ); + }, + }); - expect(sink.expectOne("auth.login")).toMatchObject({ - properties: { - source: "command", - method: "unknown", - result: "aborted", - reason: "no_url_provided", - }, - }); - expect(ensureLoggedIn).not.toHaveBeenCalled(); + 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(); + }, }); - it("records auth failures as auth.login errors", async () => { - const { commands, deploymentManager, sink } = setup({ + testCommandScenario({ + name: "records auth failures as auth.login errors", + options: { loginMethod: "legacy_token", loginResult: { success: false, reason: "auth_failed" }, - }); - - await commands.login(); - - expect(sink.expectOne("auth.login")).toMatchObject({ - properties: { - method: "legacy_token", - result: "error", - reason: "auth_failed", - }, - }); - expect(deploymentManager.setDeployment).not.toHaveBeenCalled(); + }, + act: ({ commands }) => commands.login(), + assert: ({ mocks, telemetry }) => { + expect(telemetry.expectOne("auth.login")).toMatchObject({ + properties: { + method: "legacy_token", + result: "error", + reason: "auth_failed", + }, + }); + expect(mocks.deploymentManager.setDeployment).not.toHaveBeenCalled(); + }, }); - it("uses switch_deployment source", async () => { - const { commands, sink } = setup(); - - await commands.switchDeployment(); - - expect(sink.expectOne("auth.login").properties).toMatchObject({ - source: "switch_deployment", - result: "success", - }); + 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", () => { - it("records not_authenticated as aborted", async () => { - const { commands, cliManager, sink } = setup({ authenticated: false }); - - await commands.logout(); - - expect(sink.expectOne("auth.logout")).toMatchObject({ - properties: { - result: "aborted", - reason: "not_authenticated", - }, - }); - expect(cliManager.clearCredentials).not.toHaveBeenCalled(); + 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(); + }, }); - it("records successful logout", async () => { - const { commands, deploymentManager, cliManager, secretsManager, sink } = - setup({ authenticated: true }); - - await commands.logout(); - - const event = sink.expectOne("auth.logout"); - expect(event.properties).toMatchObject({ result: "success" }); - expect(event.properties.reason).toBeUndefined(); - expect(event.measurements.durationMs).toEqual(expect.any(Number)); - expect(deploymentManager.clearDeployment).toHaveBeenCalledWith("logout"); - expect(cliManager.clearCredentials).toHaveBeenCalledWith(TEST_URL); - expect(secretsManager.clearAllAuthData).toHaveBeenCalledWith( - TEST_HOSTNAME, - ); + 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, + ); + }, }); - it("records credential clear cancellation as aborted", async () => { - const { commands, sink } = setup({ + testCommandScenario({ + name: "records credential clear cancellation as aborted", + options: { authenticated: true, credentialResult: { category: "keyring", failureCategory: "aborted", }, - }); - - await commands.logout(); - - expect(sink.expectOne("auth.logout")).toMatchObject({ - properties: { - result: "aborted", - reason: "credential_clear_cancelled", - }, - }); + }, + act: ({ commands }) => commands.logout(), + assert: ({ telemetry }) => { + expect(telemetry.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "credential_clear_cancelled", + }, + }); + }, }); - it("records credential clear failure as an error", async () => { - const { commands, sink } = setup({ + testCommandScenario({ + name: "records credential clear failure as an error", + options: { authenticated: true, credentialResult: { category: "keyring", failureCategory: "cli", }, - }); - - await commands.logout(); - - expect(sink.expectOne("auth.logout")).toMatchObject({ - properties: { - result: "error", - reason: "credential_clear_failed", - }, - }); + }, + 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 { commands, sink } = setup({ + const harness = createCommandsHarness({ authenticated: true, clearAllAuthDataError: new Error("secret clear failed"), }); - await expect(commands.logout()).rejects.toThrow("secret clear failed"); - expect(sink.expectOne("auth.logout")).toMatchObject({ + 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" }, }); From 23f7ba45705d38f5afaae76b3f341e833a519641 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 9 Jun 2026 09:46:09 +0000 Subject: [PATCH 3/6] refactor: align auth telemetry conventions --- src/core/cliCredentialManager.ts | 43 ++++--- src/core/cliManager.ts | 7 +- src/instrumentation/credentials.ts | 112 +++++++++++------- src/instrumentation/deployment.ts | 4 +- test/mocks/testHelpers.ts | 4 +- test/unit/commands.test.ts | 4 +- test/unit/core/cliCredentialManager.test.ts | 33 +++--- test/unit/core/cliManager.test.ts | 16 +-- .../unit/deployment/deploymentManager.test.ts | 4 +- 9 files changed, 121 insertions(+), 106 deletions(-) diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index ba0b12b1c8..0bf2cd49fb 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -11,9 +11,9 @@ import { CredentialCliError, CredentialFileError, CredentialTelemetry, - type CredentialCategory, + type CredentialClearRecorder, type CredentialClearResult, - type CredentialStoreResult, + type CredentialStoreRecorder, } from "../instrumentation/credentials"; import { isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; @@ -81,9 +81,9 @@ export class CliCredentialManager { token: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - return this.credentialTelemetry.traceStore(configs, () => - this.storeTokenInner(url, token, configs, options), + ): Promise { + return this.credentialTelemetry.traceStore(configs, (recorder) => + this.storeTokenInner(url, token, configs, recorder, options), ); } @@ -91,8 +91,9 @@ export class CliCredentialManager { url: string, token: string, configs: Pick, + recorder: CredentialStoreRecorder, options?: { signal?: AbortSignal }, - ): Promise { + ): Promise { const binPath = await this.resolveKeyringBinary( url, configs, @@ -100,7 +101,8 @@ export class CliCredentialManager { ); if (!binPath) { await this.writeCredentialFiles(url, token); - return { category: "file" }; + recorder.setCategory("file"); + return; } const args = [ @@ -114,8 +116,8 @@ export class CliCredentialManager { env: { ...process.env, CODER_SESSION_TOKEN: token }, signal: options?.signal, }); + recorder.setCategory("keyring"); this.logger.info("Stored token via CLI for", url); - return { category: "keyring" }; } catch (error) { this.logger.warn("Failed to store token via CLI:", error); if (isAbortError(error)) { @@ -176,27 +178,27 @@ export class CliCredentialManager { configs: Pick, options?: { signal?: AbortSignal }, ): Promise { - return this.credentialTelemetry.traceClear(configs, () => - this.deleteTokenInner(url, configs, options), + return this.credentialTelemetry.traceClear(configs, (recorder) => + this.deleteTokenInner(url, configs, recorder, options), ); } private async deleteTokenInner( url: string, configs: Pick, + recorder: CredentialClearRecorder, options?: { signal?: AbortSignal }, ): Promise { - const [filesResult, keyringResult] = await Promise.all([ + const [_filesResult, keyringResult] = await Promise.all([ this.deleteCredentialFiles(url), this.deleteKeyringToken(url, configs, options?.signal), ]); - return { - category: - keyringResult.used || keyringResult.failureCategory - ? "keyring" - : filesResult.category, - failureCategory: keyringResult.failureCategory, - }; + recorder.setCategory( + keyringResult.used || keyringResult.failureCategory ? "keyring" : "file", + ); + return keyringResult.failureCategory + ? { failureCategory: keyringResult.failureCategory } + : {}; } /** @@ -267,9 +269,7 @@ export class CliCredentialManager { /** * Delete URL and token files. Best-effort: never throws. */ - private async deleteCredentialFiles( - url: string, - ): Promise<{ category: CredentialCategory }> { + private async deleteCredentialFiles(url: string): Promise { const safeHostname = toSafeHost(url); const paths = [ this.pathResolver.getSessionTokenPath(safeHostname), @@ -282,7 +282,6 @@ export class CliCredentialManager { }), ), ); - return { category: "file" }; } /** diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 4ee2379eac..2031b2faf6 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -931,13 +931,10 @@ export class CliManager { } if (result.cancelled) { this.output.info("Credential removal cancelled by user"); - return { category: "keyring", failureCategory: "aborted" }; + return { failureCategory: "aborted" }; } this.output.warn("Failed to remove credentials:", result.error); - return { - category: isKeyringEnabled(configs) ? "keyring" : "file", - failureCategory: categorizeCredentialError(result.error), - }; + return { failureCategory: categorizeCredentialError(result.error) }; } private handleStoreError(error: unknown): void { diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts index fb489c9182..57ba41f02e 100644 --- a/src/instrumentation/credentials.ts +++ b/src/instrumentation/credentials.ts @@ -4,50 +4,51 @@ 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"; -export interface CredentialStoreResult { - readonly category: CredentialCategory; +export interface CredentialStoreRecorder { + setCategory(category: CredentialCategory): void; +} + +export interface CredentialClearRecorder { + setCategory(category: CredentialCategory): void; } export interface CredentialClearResult { - readonly category: CredentialCategory; readonly failureCategory?: CredentialFailureCategory; } export class CredentialTelemetry { public constructor(private readonly telemetry: TelemetryReporter) {} - public async traceStore( + public async traceStore( configs: Pick, - fn: () => Promise, + fn: (recorder: CredentialStoreRecorder) => Promise, ): Promise { - const keyringEnabled = isKeyringEnabled(configs); - const defaultCategory: CredentialCategory = keyringEnabled - ? "keyring" - : "file"; + const defaults = defaultCredentialProperties(configs); let cancellation: unknown; const result = await this.telemetry.trace( - "auth.credential_stored", + "auth.credential.store", async (span) => { try { - const result = await fn(); - span.setProperty("category", result.category); - return result; + return await fn(createCredentialRecorder(span)); } catch (error) { - span.setProperty("category", defaultCategory); - span.setProperty("failureCategory", categorizeCredentialError(error)); + recordCredentialFailure(span, defaults.category, error); if (isAbortError(error)) { span.markAborted(); cancellation = error; - return { category: defaultCategory } as T; + return undefined as T; } throw error; } }, - { keyringEnabled, category: defaultCategory }, + { + keyring_enabled: defaults.keyringEnabled, + category: defaults.category, + }, ); if (cancellation instanceof Error) { throw cancellation; @@ -57,43 +58,31 @@ export class CredentialTelemetry { public async traceClear( configs: Pick, - fn: () => Promise, + fn: (recorder: CredentialClearRecorder) => Promise, ): Promise { - const keyringEnabled = isKeyringEnabled(configs); - const defaultCategory: CredentialCategory = keyringEnabled - ? "keyring" - : "file"; + const defaults = defaultCredentialProperties(configs); let cancellation: unknown; const result = await this.telemetry.trace( - "auth.credential_cleared", + "auth.credential.clear", async (span) => { try { - const result = await fn(); - span.setProperty("category", result.category); - if (result.failureCategory) { - span.setProperty("failureCategory", result.failureCategory); - if (result.failureCategory === "aborted") { - span.markAborted(); - } else { - span.markFailure(); - } - } + const result = await fn(createCredentialRecorder(span)); + recordClearFailure(span, result.failureCategory); return result; } catch (error) { - span.setProperty("category", defaultCategory); - span.setProperty("failureCategory", categorizeCredentialError(error)); + recordCredentialFailure(span, defaults.category, error); if (isAbortError(error)) { span.markAborted(); cancellation = error; - return { - category: defaultCategory, - failureCategory: "aborted", - } as T; + return { failureCategory: "aborted" } as T; } throw error; } }, - { keyringEnabled, category: defaultCategory }, + { + keyring_enabled: defaults.keyringEnabled, + category: defaults.category, + }, ); if (cancellation instanceof Error) { throw cancellation; @@ -102,6 +91,49 @@ export class CredentialTelemetry { } } +function defaultCredentialProperties( + configs: Pick, +): { keyringEnabled: boolean; category: CredentialCategory } { + const keyringEnabled = isKeyringEnabled(configs); + return { + keyringEnabled, + category: keyringEnabled ? "keyring" : "file", + }; +} + +function createCredentialRecorder( + span: Span, +): CredentialStoreRecorder & CredentialClearRecorder { + return { + setCategory: (category) => span.setProperty("category", category), + }; +} + +function recordCredentialFailure( + span: Span, + category: CredentialCategory, + error: unknown, +): void { + span.setProperty("category", category); + span.setProperty("failure_category", categorizeCredentialError(error)); +} + +function recordClearFailure( + span: Span, + failureCategory: CredentialClearResult["failureCategory"], +): void { + if (!failureCategory) { + return; + } + + span.setProperty("failure_category", failureCategory); + if (failureCategory === "aborted") { + span.markAborted(); + return; + } + span.markFailure(); +} + export function categorizeCredentialError( error: unknown, ): CredentialFailureCategory { diff --git a/src/instrumentation/deployment.ts b/src/instrumentation/deployment.ts index 5aae289162..738b9ff918 100644 --- a/src/instrumentation/deployment.ts +++ b/src/instrumentation/deployment.ts @@ -22,10 +22,10 @@ export class DeploymentTelemetry { } public crossWindowDetected(): void { - this.telemetry.log("deployment.cross_window_detected"); + this.telemetry.log("deployment.cross_window.detected"); } public authConfigRecoveryFailed(): void { - this.telemetry.log("deployment.auth_config_recovery_failed"); + this.telemetry.log("deployment.auth_config.recovery_failed"); } } diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 590f5424f9..b135e5940f 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -444,9 +444,9 @@ export class InMemorySecretStorage implements vscode.SecretStorage { export function createMockCliCredentialManager(): CliCredentialManager { return { - storeToken: vi.fn().mockResolvedValue({ category: "file" }), + storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), - deleteToken: vi.fn().mockResolvedValue({ category: "file" }), + deleteToken: vi.fn().mockResolvedValue({}), } as unknown as CliCredentialManager; } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 1399635c79..19186cf67a 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -106,7 +106,7 @@ function createCommandsHarness(options: CommandsHarnessOptions = {}) { const cliManager = { clearCredentials: vi.fn(() => - Promise.resolve(options.credentialResult ?? { category: "file" }), + Promise.resolve(options.credentialResult ?? {}), ), }; @@ -311,7 +311,6 @@ describe("Commands", () => { options: { authenticated: true, credentialResult: { - category: "keyring", failureCategory: "aborted", }, }, @@ -331,7 +330,6 @@ describe("Commands", () => { options: { authenticated: true, credentialResult: { - category: "keyring", failureCategory: "cli", }, }, diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 57175b640f..1e2eb85818 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -183,10 +183,10 @@ describe("CliCredentialManager", () => { 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_stored")).toMatchObject({ + expect(sink.expectOne("auth.credential.store")).toMatchObject({ properties: { category: "file", - keyringEnabled: "false", + keyring_enabled: "false", result: "success", }, }); @@ -206,10 +206,10 @@ 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_stored")).toMatchObject({ + expect(sink.expectOne("auth.credential.store")).toMatchObject({ properties: { category: "keyring", - keyringEnabled: "true", + keyring_enabled: "true", result: "success", }, }); @@ -235,9 +235,9 @@ describe("CliCredentialManager", () => { await expect( manager.storeToken(TEST_URL, "token", configs), ).rejects.toThrow("Credential CLI operation failed"); - expect(sink.expectOne("auth.credential_stored")).toMatchObject({ + expect(sink.expectOne("auth.credential.store")).toMatchObject({ properties: { - failureCategory: "cli", + failure_category: "cli", result: "error", }, }); @@ -296,9 +296,9 @@ describe("CliCredentialManager", () => { signal: AbortSignal.abort(), }), ).rejects.toThrow("The operation was aborted"); - expect(sink.expectOne("auth.credential_stored")).toMatchObject({ + expect(sink.expectOne("auth.credential.store")).toMatchObject({ properties: { - failureCategory: "aborted", + failure_category: "aborted", result: "aborted", }, }); @@ -413,10 +413,10 @@ 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_cleared")).toMatchObject({ + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { category: "keyring", - keyringEnabled: "true", + keyring_enabled: "true", result: "success", }, }); @@ -441,9 +441,9 @@ describe("CliCredentialManager", () => { await expect( manager.deleteToken(TEST_URL, configs), ).resolves.not.toThrow(); - expect(sink.expectOne("auth.credential_cleared")).toMatchObject({ + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { - failureCategory: "cli", + failure_category: "cli", result: "error", }, }); @@ -454,14 +454,13 @@ describe("CliCredentialManager", () => { const { manager, sink } = setup(failingResolver()); await expect(manager.deleteToken(TEST_URL, configs)).resolves.toEqual({ - category: "keyring", failureCategory: "binary", }); expect(execFile).not.toHaveBeenCalled(); - expect(sink.expectOne("auth.credential_cleared")).toMatchObject({ + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { category: "keyring", - failureCategory: "binary", + failure_category: "binary", result: "error", }, }); @@ -512,9 +511,9 @@ describe("CliCredentialManager", () => { signal: AbortSignal.abort(), }), ).rejects.toThrow("The operation was aborted"); - expect(sink.expectOne("auth.credential_cleared")).toMatchObject({ + expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { - failureCategory: "aborted", + failure_category: "aborted", result: "aborted", }, }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 32b977f5db..f9ddf3e1c7 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -353,9 +353,7 @@ describe("CliManager", () => { const CLEAR_URL = "https://dev.coder.com"; it("should skip progress notification when keyring is disabled", async () => { - await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ - category: "file", - }); + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({}); expect(vscode.window.withProgress).not.toHaveBeenCalled(); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( @@ -367,13 +365,9 @@ describe("CliManager", () => { it("should show progress notification when keyring is enabled", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce({ - category: "keyring", - }); + vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce({}); - await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ - category: "keyring", - }); + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({}); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ @@ -387,12 +381,10 @@ describe("CliManager", () => { it("returns credential manager failure categories", async () => { vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce({ - category: "keyring", failureCategory: "cli", }); await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ - category: "keyring", failureCategory: "cli", }); }); @@ -403,7 +395,6 @@ describe("CliManager", () => { ); await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ - category: "file", failureCategory: "binary", }); }); @@ -415,7 +406,6 @@ describe("CliManager", () => { ); await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ - category: "keyring", failureCategory: "aborted", }); }); diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 53dca31eb3..3f4e157f44 100644 --- a/test/unit/deployment/deploymentManager.test.ts +++ b/test/unit/deployment/deploymentManager.test.ts @@ -345,7 +345,7 @@ describe("DeploymentManager", () => { expect(mockClient.host).toBe(TEST_URL); expect(mockClient.token).toBe("synced-token"); expect( - telemetrySink.expectOne("deployment.cross_window_detected").properties, + telemetrySink.expectOne("deployment.cross_window.detected").properties, ).toEqual({}); expect(telemetrySink.expectOne("deployment.recovered")).toMatchObject({ properties: { trigger: "cross_window" }, @@ -634,7 +634,7 @@ describe("DeploymentManager", () => { 0, ); expect( - telemetrySink.expectOne("deployment.auth_config_recovery_failed") + telemetrySink.expectOne("deployment.auth_config.recovery_failed") .properties, ).toEqual({}); } finally { From 408abd67f32ced9270b65691c7dcd562c510e5a1 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 9 Jun 2026 13:59:38 +0000 Subject: [PATCH 4/6] refactor: simplify auth telemetry flow --- src/commands.ts | 35 ++--- src/core/cliCredentialManager.ts | 180 +++++++++++------------ src/instrumentation/auth.ts | 110 +++++++++----- src/instrumentation/credentials.ts | 87 +++++------ src/login/loginCoordinator.ts | 125 +++++++--------- src/uri/uriHandler.ts | 19 ++- test/unit/commands.test.ts | 47 +++--- test/unit/instrumentation/auth.test.ts | 53 +++++++ test/unit/login/loginCoordinator.test.ts | 139 ++++++----------- 9 files changed, 402 insertions(+), 393 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 887f322a6c..8c34340b38 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -16,7 +16,6 @@ import { toError } from "./error/errorUtils"; import { type FeatureSet, featureSetForVersion } from "./featureSet"; import { AuthTelemetry, - type AuthLoginMethod, type AuthLoginOutcome, type AuthLoginSource, type AuthLogoutOutcome, @@ -59,7 +58,7 @@ 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 { @@ -83,8 +82,6 @@ interface LoginArgs { readonly autoLogin?: boolean; } -type LoginMethodRecorder = (method: AuthLoginMethod) => void; - interface LoginSuccess { readonly user: User; readonly token: string; @@ -167,28 +164,24 @@ export class Commands { } await this.traceLoginCommand( args?.autoLogin ? "auto_login" : "command", - (recordMethod) => this.performLogin(args, recordMethod), + (setMethod) => this.performLogin(args, setMethod), ); } private async traceLoginCommand( source: AuthLoginSource, - run: (recordMethod: LoginMethodRecorder) => Promise, + run: ( + setMethod: (method: LoginMethod) => void, + ) => Promise, ): Promise { - let method: AuthLoginMethod = "unknown"; - await this.authTelemetry.traceLogin( - source, - () => method, - () => - run((next) => { - method = next; - }), + await this.authTelemetry.traceLogin(source, (trace) => + run((method) => trace.setMethod(method)), ); } private async performLogin( args: LoginArgs | undefined, - recordMethod: LoginMethodRecorder, + setMethod: (method: LoginMethod) => void, ): Promise { this.logger.debug("Logging in"); @@ -209,16 +202,18 @@ export class Commands { safeHostname, url, autoLogin: args?.autoLogin, - traceLogin: false, - onLoginMethod: recordMethod, }); if (!result.success) { + if (result.method) { + setMethod(result.method); + } return result; } + setMethod(result.method); await this.completeLogin(url, safeHostname, result); - return { success: true }; + return { success: true, method: result.method }; } private async completeLogin( @@ -520,8 +515,8 @@ export class Commands { */ public async switchDeployment(): Promise { this.logger.debug("Switching deployment"); - await this.traceLoginCommand("switch_deployment", (recordMethod) => - this.performLogin(undefined, recordMethod), + await this.traceLoginCommand("switch_deployment", (setMethod) => + this.performLogin(undefined, setMethod), ); } diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 0bf2cd49fb..7553132e00 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -11,9 +11,7 @@ import { CredentialCliError, CredentialFileError, CredentialTelemetry, - type CredentialClearRecorder, type CredentialClearResult, - type CredentialStoreRecorder, } from "../instrumentation/credentials"; import { isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; @@ -58,15 +56,21 @@ export function isKeyringSupported(): boolean { * persistence: keyring-backed (via CLI) and file-based (plaintext). */ export class CliCredentialManager { - private readonly credentialTelemetry: CredentialTelemetry; + 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.credentialTelemetry = new CredentialTelemetry(telemetry); + this.#logger = logger; + this.#resolveBinary = resolveBinary; + this.#pathResolver = pathResolver; + this.#credentialTelemetry = new CredentialTelemetry(telemetry); } /** @@ -82,29 +86,30 @@ export class CliCredentialManager { configs: Pick, options?: { signal?: AbortSignal }, ): Promise { - return this.credentialTelemetry.traceStore(configs, (recorder) => - this.storeTokenInner(url, token, configs, recorder, options), - ); + 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), + ); + }); } - private async storeTokenInner( + async #storeKeyringToken( + binPath: string, url: string, token: string, configs: Pick, - recorder: CredentialStoreRecorder, options?: { signal?: AbortSignal }, ): Promise { - const binPath = await this.resolveKeyringBinary( - url, - configs, - "keyringAuth", - ); - if (!binPath) { - await this.writeCredentialFiles(url, token); - recorder.setCategory("file"); - return; - } - const args = [ ...getHeaderArgs(configs), "login", @@ -112,14 +117,13 @@ 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, }); - recorder.setCategory("keyring"); - 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); + this.#logger.warn("Failed to store token via CLI:", error); if (isAbortError(error)) { throw error; } @@ -139,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) { @@ -154,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(); @@ -163,42 +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 deleteToken( url: string, configs: Pick, options?: { signal?: AbortSignal }, ): Promise { - return this.credentialTelemetry.traceClear(configs, (recorder) => - this.deleteTokenInner(url, configs, recorder, options), - ); - } + return this.#credentialTelemetry.traceClear(configs, async (trace) => { + await trace.file(() => this.#deleteCredentialFiles(url)); - private async deleteTokenInner( - url: string, - configs: Pick, - recorder: CredentialClearRecorder, - options?: { signal?: AbortSignal }, - ): Promise { - const [_filesResult, keyringResult] = await Promise.all([ - this.deleteCredentialFiles(url), - this.deleteKeyringToken(url, configs, options?.signal), - ]); - recorder.setCategory( - keyringResult.used || keyringResult.failureCategory ? "keyring" : "file", - ); - return keyringResult.failureCategory - ? { failureCategory: keyringResult.failureCategory } - : {}; + 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 {}; + }); } /** @@ -209,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, @@ -217,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; } @@ -225,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, { @@ -248,16 +257,13 @@ export class CliCredentialManager { /** * Write URL and token files under --global-config. */ - private async writeCredentialFiles( - url: string, - token: string, - ): Promise { + 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), + this.#atomicWriteFile(this.#pathResolver.getUrlPath(safeHostname), url), + this.#atomicWriteFile( + this.#pathResolver.getSessionTokenPath(safeHostname), token, ), ]); @@ -269,16 +275,16 @@ export class CliCredentialManager { /** * 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); }), ), ); @@ -287,50 +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<{ - used: boolean; - failureCategory?: CredentialClearResult["failureCategory"]; - }> { - 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 { used: false, failureCategory: "binary" }; - } - if (!binPath) { - return { used: false }; - } - + ): 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); - return { used: true }; + 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); - return { used: true, failureCategory: "cli" }; + 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/instrumentation/auth.ts b/src/instrumentation/auth.ts index 5044e23068..0e85ba33b9 100644 --- a/src/instrumentation/auth.ts +++ b/src/instrumentation/auth.ts @@ -1,4 +1,6 @@ +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"; @@ -15,7 +17,7 @@ export type AuthLoginMethod = | "provided_token" | "stored_token" | "keyring_token" - | "legacy_token" + | "cli_token" | "oauth"; export type LoginPromptReason = @@ -33,44 +35,47 @@ export type LoginPromptOutcome = | { success: true } | { success: false; reason: LoginPromptReason }; export type AuthLoginOutcome = - | { success: true } - | { success: false; reason: AuthLoginReason }; + | { 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; setRecovery(recovery: AuthRecoveryAction): void; 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, - getMethod: () => AuthLoginMethod, - fn: () => Promise, + fn: (trace: AuthLoginTrace) => Promise, ): Promise { return this.telemetry.trace( "auth.login", async (span) => { - const setMethod = () => span.setProperty("method", getMethod()); try { - const result = await fn(); - setMethod(); - if (!result.success) { - span.setProperty("reason", result.reason); - if (result.reason === "auth_failed") { - span.markFailure(); - } else { - span.markAborted(); - } - } + const result = await fn(createLoginTrace(span)); + recordLoginResult(span, result); return result; } catch (error) { - setMethod(); span.setProperty("reason", "exception"); throw error; } @@ -85,17 +90,7 @@ export class AuthTelemetry { return this.telemetry.trace("auth.logout", async (span) => { try { const result = await fn(); - if (!result.success) { - span.setProperty("reason", result.reason); - if ( - result.reason === "not_authenticated" || - result.reason === "credential_clear_cancelled" - ) { - span.markAborted(); - } else { - span.markFailure(); - } - } + recordLogoutResult(span, result); return result; } catch (error) { span.setProperty("reason", "exception"); @@ -149,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 index 57ba41f02e..030c08b108 100644 --- a/src/instrumentation/credentials.ts +++ b/src/instrumentation/credentials.ts @@ -9,12 +9,9 @@ import type { Span } from "../telemetry/span"; export type CredentialCategory = "keyring" | "file"; export type CredentialFailureCategory = "aborted" | "binary" | "cli" | "file"; -export interface CredentialStoreRecorder { - setCategory(category: CredentialCategory): void; -} - -export interface CredentialClearRecorder { - setCategory(category: CredentialCategory): void; +interface CredentialTrace { + file(fn: () => Promise): Promise; + keyring(fn: () => Promise): Promise; } export interface CredentialClearResult { @@ -26,55 +23,44 @@ export class CredentialTelemetry { public async traceStore( configs: Pick, - fn: (recorder: CredentialStoreRecorder) => Promise, + fn: (trace: CredentialTrace) => Promise, ): Promise { - const defaults = defaultCredentialProperties(configs); - let cancellation: unknown; - const result = await this.telemetry.trace( - "auth.credential.store", - async (span) => { - try { - return await fn(createCredentialRecorder(span)); - } 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; + return this.traceCredential("auth.credential.store", configs, fn); } public async traceClear( configs: Pick, - fn: (recorder: CredentialClearRecorder) => Promise, + 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( - "auth.credential.clear", + eventName, async (span) => { try { - const result = await fn(createCredentialRecorder(span)); - recordClearFailure(span, result.failureCategory); + 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 { failureCategory: "aborted" } as T; + return undefined as T; } throw error; } @@ -101,11 +87,17 @@ function defaultCredentialProperties( }; } -function createCredentialRecorder( - span: Span, -): CredentialStoreRecorder & CredentialClearRecorder { +function createCredentialTrace(span: Span): CredentialTrace { + const run = async ( + category: CredentialCategory, + fn: () => Promise, + ): Promise => { + span.setProperty("category", category); + return await fn(); + }; return { - setCategory: (category) => span.setProperty("category", category), + file: (fn) => run("file", fn), + keyring: (fn) => run("keyring", fn), }; } @@ -118,16 +110,13 @@ function recordCredentialFailure( span.setProperty("failure_category", categorizeCredentialError(error)); } -function recordClearFailure( - span: Span, - failureCategory: CredentialClearResult["failureCategory"], -): void { - if (!failureCategory) { +function recordClearFailure(span: Span, result: CredentialClearResult): void { + if (!result.failureCategory) { return; } - span.setProperty("failure_category", failureCategory); - if (failureCategory === "aborted") { + span.setProperty("failure_category", result.failureCategory); + if (result.failureCategory === "aborted") { span.markAborted(); return; } diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 0b30527773..cb02627a96 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -20,34 +20,40 @@ import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; import type { - AuthLoginMethod, AuthLoginPromptTrigger, - AuthLoginSource, AuthTelemetry, LoginPromptReason, } from "../instrumentation/auth"; 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 }; -type LoginMethodRecorder = (method: AuthLoginMethod) => void; - -interface LoginMethodTracker { - readonly record: LoginMethodRecorder; - readonly current: () => AuthLoginMethod; -} +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; autoLogin?: boolean; token?: string; - source?: AuthLoginSource; - traceLogin?: boolean; - onLoginMethod?: (method: AuthLoginMethod) => void; } /** @@ -75,29 +81,11 @@ 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 ensureLoggedIn( options: LoginOptions & { url: string }, - ): Promise { - const method = createLoginMethodTracker(options.onLoginMethod); - const login = () => this.ensureLoggedInCore(options, method.record); - - if (options.traceLogin === false) { - return login(); - } - - return this.authTelemetry.traceLogin( - options.source ?? "direct", - method.current, - login, - ); - } - - private ensureLoggedInCore( - options: LoginOptions & { url: string }, - recordMethod: LoginMethodRecorder, ): Promise { const { safeHostname, url } = options; return this.executeWithGuard(async () => { @@ -105,7 +93,6 @@ export class LoginCoordinator implements vscode.Disposable { { safeHostname, url }, options.autoLogin ?? false, options.token, - recordMethod, ); await this.persistSessionAuth(result, safeHostname, url); @@ -123,7 +110,7 @@ export class LoginCoordinator implements vscode.Disposable { detailPrefix?: string; trigger: AuthLoginPromptTrigger; }, - ): Promise { + ): Promise { return this.authTelemetry.traceLoginPrompt(options.trigger, () => this.performLoginDialog(options), ); @@ -131,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 @@ -147,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 = @@ -189,7 +176,7 @@ export class LoginCoordinator implements vscode.Disposable { } private async persistSessionAuth( - result: LoginResult, + result: LoginAttemptResult, safeHostname: string, url: string, ): Promise { @@ -215,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 */ @@ -230,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) => { @@ -265,15 +252,14 @@ 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, isAutoLogin: boolean, providedToken?: string, - recordMethod: LoginMethodRecorder = () => undefined, ): Promise { const client = CoderApi.create(deployment.url, "", this.logger); try { @@ -282,7 +268,6 @@ export class LoginCoordinator implements vscode.Disposable { deployment, isAutoLogin, providedToken, - recordMethod, ); } finally { client.dispose(); @@ -294,13 +279,14 @@ export class LoginCoordinator implements vscode.Disposable { deployment: Deployment, isAutoLogin: boolean, providedToken: string | undefined, - recordMethod: LoginMethodRecorder, ): Promise { // mTLS authentication (no token needed) if (!needToken(vscode.workspace.getConfiguration())) { this.logger.debug("Attempting mTLS authentication (no token required)"); - recordMethod("mtls"); - return this.tryMtlsAuth(client, isAutoLogin); + return withLoginMethod( + "mtls", + await this.tryMtlsAuth(client, isAutoLogin), + ); } // Try provided token first @@ -312,8 +298,7 @@ export class LoginCoordinator implements vscode.Disposable { isAutoLogin, ); if (result !== "unauthorized") { - recordMethod("provided_token"); - return result; + return withLoginMethod("provided_token", result); } } @@ -325,8 +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") { - recordMethod("stored_token"); - return result; + return withLoginMethod("stored_token", result); } } @@ -353,8 +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") { - recordMethod("keyring_token"); - return result; + return withLoginMethod("keyring_token", result); } } @@ -362,11 +345,9 @@ export class LoginCoordinator implements vscode.Disposable { const authMethod = await maybeAskAuthMethod(client); switch (authMethod) { case "oauth": - recordMethod("oauth"); - return this.loginWithOAuth(deployment); + return withLoginMethod("oauth", await this.loginWithOAuth(deployment)); case "legacy": - recordMethod("legacy_token"); - return this.loginWithToken(client); + return withLoginMethod("cli_token", await this.loginWithToken(client)); case undefined: return { success: false, reason: "user_dismissed" }; } @@ -375,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 }; @@ -392,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(); @@ -432,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"); @@ -490,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"); @@ -533,15 +516,9 @@ export class LoginCoordinator implements vscode.Disposable { } } -function createLoginMethodTracker( - onRecord?: LoginMethodRecorder, -): LoginMethodTracker { - let method: AuthLoginMethod = "unknown"; - return { - record: (next) => { - method = next; - onRecord?.(next); - }, - current: () => method, - }; +function withLoginMethod( + method: LoginMethod, + result: LoginAttemptResult, +): LoginResult { + return { ...result, method }; } diff --git a/src/uri/uriHandler.ts b/src/uri/uriHandler.ts index 74cd9acd9e..520654fba2 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,11 +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, - source: "uri", + 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/unit/commands.test.ts b/test/unit/commands.test.ts index 19186cf67a..2a799fd8fd 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -19,12 +19,8 @@ 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 { - AuthLoginMethod, - LoginPromptReason, -} from "@/instrumentation/auth"; import type { CredentialClearResult } from "@/instrumentation/credentials"; -import type { LoginCoordinator } from "@/login/loginCoordinator"; +import type { LoginCoordinator, LoginResult } from "@/login/loginCoordinator"; import type { SpeedtestPanelFactory } from "@/webviews/speedtest/speedtestPanelFactory"; import type { DuplicateWorkspaceIpc } from "@/workspace/duplicateWorkspaceIpc"; @@ -46,17 +42,15 @@ interface LoginOptionsForTest { safeHostname: string; url: string; autoLogin?: boolean; - traceLogin?: boolean; - onLoginMethod?: (method: AuthLoginMethod) => void; } -type LoginResultForTest = - | { success: true; user: ReturnType; token: string } - | { success: false; reason: LoginPromptReason }; +type LoginResultForTest = LoginResult & { + readonly user?: ReturnType; +}; interface CommandsHarnessOptions { readonly authenticated?: boolean; - readonly loginMethod?: AuthLoginMethod; + readonly loginResult?: LoginResultForTest; readonly credentialResult?: CredentialClearResult; readonly clearDeploymentError?: Error; @@ -80,16 +74,14 @@ function createCommandsHarness(options: CommandsHarnessOptions = {}) { options.loginResult ?? ({ success: true, + method: "stored_token", user: createMockUser(), token: "test-token", } satisfies LoginResultForTest); - const loginMethod = options.loginMethod ?? "stored_token"; - const loginCoordinator = { - ensureLoggedIn: vi.fn((loginOptions: LoginOptionsForTest) => { - loginOptions.onLoginMethod?.(loginMethod); - return Promise.resolve(loginResult); - }), + ensureLoggedIn: vi.fn((_loginOptions: LoginOptionsForTest) => + Promise.resolve(loginResult), + ), }; const deploymentManager = { @@ -196,7 +188,6 @@ describe("Commands", () => { expect.objectContaining({ safeHostname: TEST_HOSTNAME, url: TEST_URL, - traceLogin: false, }), ); expect(mocks.deploymentManager.setDeployment).toHaveBeenCalled(); @@ -205,7 +196,14 @@ describe("Commands", () => { testCommandScenario({ name: "uses auto_login source when requested", - options: { loginMethod: "provided_token" }, + 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({ @@ -214,7 +212,7 @@ describe("Commands", () => { result: "success", }); expect(mocks.loginCoordinator.ensureLoggedIn).toHaveBeenCalledWith( - expect.objectContaining({ autoLogin: true, traceLogin: false }), + expect.objectContaining({ autoLogin: true }), ); }, }); @@ -241,14 +239,17 @@ describe("Commands", () => { testCommandScenario({ name: "records auth failures as auth.login errors", options: { - loginMethod: "legacy_token", - loginResult: { success: false, reason: "auth_failed" }, + loginResult: { + success: false, + method: "cli_token", + reason: "auth_failed", + }, }, act: ({ commands }) => commands.login(), assert: ({ mocks, telemetry }) => { expect(telemetry.expectOne("auth.login")).toMatchObject({ properties: { - method: "legacy_token", + method: "cli_token", result: "error", reason: "auth_failed", }, diff --git a/test/unit/instrumentation/auth.test.ts b/test/unit/instrumentation/auth.test.ts index ebcafc8802..1017fc7db5 100644 --- a/test/unit/instrumentation/auth.test.ts +++ b/test/unit/instrumentation/auth.test.ts @@ -10,6 +10,59 @@ function setup() { } 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(); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 2845af6683..03031ccb56 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,22 +570,16 @@ 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", + }); }); }); describe("telemetry", () => { - function setupTelemetryContext() { - const sink = new TestSink(); - const ctx = createTestContext(createTestTelemetryService(sink)); - return { ...ctx, sink }; - } - - const loginOptions = () => ({ - url: TEST_URL, - safeHostname: TEST_HOSTNAME, - }); - const dialogOptions = (trigger: "auth_required" | "missing_session") => ({ url: TEST_URL, safeHostname: TEST_HOSTNAME, @@ -573,77 +601,6 @@ describe("LoginCoordinator", () => { }; } - it("records auth.login success with source, method, result, and duration", async () => { - const { mockConfig, coordinator, mockSuccessfulAuth, sink } = - setupTelemetryContext(); - enableMTLS(mockConfig); - mockSuccessfulAuth(); - - await coordinator.ensureLoggedIn({ - ...loginOptions(), - source: "uri", - }); - - const event = sink.expectOne("auth.login"); - expect(event.properties).toMatchObject({ - source: "uri", - method: "mtls", - result: "success", - }); - expect(event.properties.reason).toBeUndefined(); - expect(event.error).toBeUndefined(); - expect(event.measurements.durationMs).toEqual(expect.any(Number)); - }); - - it("records auth.login cancellation with bounded reason", async () => { - const { userInteraction, coordinator, mockAuthFailure, sink } = - setupTelemetryContext(); - mockAuthFailure(); - userInteraction.setInputBoxValue(undefined); - - await coordinator.ensureLoggedIn(loginOptions()); - - const event = sink.expectOne("auth.login"); - expect(event.properties).toMatchObject({ - source: "direct", - method: "legacy_token", - result: "aborted", - reason: "user_dismissed", - }); - expect(event.error).toBeUndefined(); - }); - - it("records auth.login auth failures as errors with bounded reason", async () => { - const { mockConfig, coordinator, mockAuthFailure, sink } = - setupTelemetryContext(); - enableMTLS(mockConfig); - mockAuthFailure("Certificate error"); - - await coordinator.ensureLoggedIn(loginOptions()); - - const event = sink.expectOne("auth.login"); - expect(event.properties).toMatchObject({ - method: "mtls", - result: "error", - reason: "auth_failed", - }); - expect(event.error).toBeUndefined(); - }); - - it("does not duplicate auth.login when tracing is disabled by caller", async () => { - const { mockConfig, coordinator, mockSuccessfulAuth, sink } = - setupTelemetryContext(); - enableMTLS(mockConfig); - mockSuccessfulAuth(); - - await coordinator.ensureLoggedIn({ - ...loginOptions(), - traceLogin: false, - }); - - expect(sink.eventsNamed("auth.login")).toHaveLength(0); - }); - it.each([ { name: "user dismisses the dialog: aborted + user_dismissed", From 23a5367f6efb84cd5e28e6e851323c8016617ec0 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 9 Jun 2026 19:37:59 +0300 Subject: [PATCH 5/6] refactor: simplify auth telemetry per review - Revert cliCredentialManager from `#` fields back to `private` to cut diff - Restore parallel file/keyring deletion in deleteToken - Drop the failure-category return threaded up to logout; record credential failures on the auth.credential.clear span at the source instead - Pass span last and grouped ({ signal, span }) into deleteKeyringToken - Remove the loginMethods identity map, dead AuthLoginMethod/AuthLoginReason, unused "direct" source, and collapse the record-* helpers to one - Drop redundant setMethod calls now that traceLogin records method on return - Rename commands.test.ts -> commands.telemetry.test.ts - Fix uriHandler tests broken by the telemetry service dependency --- src/commands.ts | 72 ++------ src/core/cliCredentialManager.ts | 156 +++++++++--------- src/core/cliManager.ts | 13 +- src/instrumentation/auth.ts | 107 +++--------- src/instrumentation/credentials.ts | 125 ++++---------- src/uri/uriHandler.ts | 14 +- test/mocks/testHelpers.ts | 2 +- ...nds.test.ts => commands.telemetry.test.ts} | 44 +---- test/unit/core/cliCredentialManager.test.ts | 6 +- test/unit/core/cliManager.test.ts | 43 ++--- test/unit/instrumentation/auth.test.ts | 23 +-- test/unit/uri/uriHandler.test.ts | 2 + 12 files changed, 183 insertions(+), 424 deletions(-) rename test/unit/{commands.test.ts => commands.telemetry.test.ts} (89%) diff --git a/src/commands.ts b/src/commands.ts index 8c34340b38..e0aee69368 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -17,7 +17,6 @@ import { type FeatureSet, featureSetForVersion } from "./featureSet"; import { AuthTelemetry, type AuthLoginOutcome, - type AuthLoginSource, type AuthLogoutOutcome, } from "./instrumentation/auth"; import { @@ -43,7 +42,6 @@ import { } from "./workspace/workspacesProvider"; import type { - User, Workspace, WorkspaceAgent, } from "coder/site/src/api/typesGenerated"; @@ -55,8 +53,6 @@ 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, LoginMethod } from "./login/loginCoordinator"; import type { TelemetryService } from "./telemetry/service"; @@ -82,11 +78,6 @@ interface LoginArgs { readonly autoLogin?: boolean; } -interface LoginSuccess { - readonly user: User; - readonly token: string; -} - const openDefaults = { openRecent: false, useDefaultDirectory: true, @@ -162,20 +153,9 @@ export class Commands { if (this.deploymentManager.isAuthenticated()) { return; } - await this.traceLoginCommand( + await this.authTelemetry.traceLogin( args?.autoLogin ? "auto_login" : "command", - (setMethod) => this.performLogin(args, setMethod), - ); - } - - private async traceLoginCommand( - source: AuthLoginSource, - run: ( - setMethod: (method: LoginMethod) => void, - ) => Promise, - ): Promise { - await this.authTelemetry.traceLogin(source, (trace) => - run((method) => trace.setMethod(method)), + (trace) => this.performLogin(args, trace.setMethod), ); } @@ -205,31 +185,20 @@ export class Commands { }); if (!result.success) { - if (result.method) { - setMethod(result.method); - } return result; } + // Record the method eagerly so it is captured even if persistence throws. 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, token: result.token, user: result.user, }); - this.showWelcomeMessage(result.user.username); this.logger.debug("Login complete to deployment:", url); + return { success: true, method: result.method }; } private showWelcomeMessage(username: string): void { @@ -480,21 +449,14 @@ export class Commands { const deployment = this.deploymentManager.getCurrentDeployment(); await this.deploymentManager.clearDeployment("logout"); - 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; + return { success: true }; } private showLogoutMessage(): void { @@ -515,8 +477,8 @@ export class Commands { */ public async switchDeployment(): Promise { this.logger.debug("Switching deployment"); - await this.traceLoginCommand("switch_deployment", (setMethod) => - this.performLogin(undefined, setMethod), + await this.authTelemetry.traceLogin("switch_deployment", (trace) => + this.performLogin(undefined, trace.setMethod), ); } @@ -1283,18 +1245,6 @@ 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 7553132e00..61d4163b17 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -11,7 +11,6 @@ import { CredentialCliError, CredentialFileError, CredentialTelemetry, - type CredentialClearResult, } from "../instrumentation/credentials"; import { isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; @@ -27,6 +26,7 @@ import { version } from "./cliExec"; import type { WorkspaceConfiguration } from "vscode"; import type { Logger } from "../logging/logger"; +import type { Span } from "../telemetry/span"; import type { PathResolver } from "./pathResolver"; @@ -56,21 +56,15 @@ 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; + private readonly credentialTelemetry: CredentialTelemetry; constructor( - logger: Logger, - resolveBinary: BinaryResolver, - pathResolver: PathResolver, + private readonly logger: Logger, + private readonly resolveBinary: BinaryResolver, + private readonly pathResolver: PathResolver, telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, ) { - this.#logger = logger; - this.#resolveBinary = resolveBinary; - this.#pathResolver = pathResolver; - this.#credentialTelemetry = new CredentialTelemetry(telemetry); + this.credentialTelemetry = new CredentialTelemetry(telemetry); } /** @@ -86,24 +80,23 @@ export class CliCredentialManager { configs: Pick, options?: { signal?: AbortSignal }, ): Promise { - return this.#credentialTelemetry.traceStore(configs, async (trace) => { - const binPath = await this.#resolveKeyringBinary( + return this.credentialTelemetry.traceStore(configs, async (span) => { + const binPath = await this.resolveKeyringBinary( url, configs, "keyringAuth", ); if (!binPath) { - await trace.file(() => this.#writeCredentialFiles(url, token)); + span.setProperty("category", "file"); + await this.writeCredentialFiles(url, token); return; } - - await trace.keyring(() => - this.#storeKeyringToken(binPath, url, token, configs, options), - ); + span.setProperty("category", "keyring"); + await this.storeKeyringToken(binPath, url, token, configs, options); }); } - async #storeKeyringToken( + private async storeKeyringToken( binPath: string, url: string, token: string, @@ -117,13 +110,13 @@ 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); + this.logger.warn("Failed to store token via CLI:", error); if (isAbortError(error)) { throw error; } @@ -143,13 +136,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) { @@ -158,7 +151,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(); @@ -167,46 +160,29 @@ 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. File deletion is best-effort. - * Keyring deletion returns a bounded failure category when attempted and - * unsuccessful. Throws AbortError when the signal is aborted. + * Delete credentials for a deployment. Runs file deletion and keyring + * deletion in parallel, both best-effort. Throws AbortError when the + * signal is aborted. */ public deleteToken( url: string, configs: Pick, options?: { signal?: AbortSignal }, - ): 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 {}; + ): Promise { + return this.credentialTelemetry.traceClear(configs, async (span) => { + await Promise.all([ + this.deleteCredentialFiles(url), + this.deleteKeyringToken(url, configs, { + signal: options?.signal, + span, + }), + ]); }); } @@ -218,7 +194,7 @@ export class CliCredentialManager { * Throws on binary resolution or version-check failure (caller decides * whether to catch or propagate). */ - async #resolveKeyringBinary( + private async resolveKeyringBinary( url: string, configs: Pick, feature: KeyringFeature, @@ -226,7 +202,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; } @@ -234,14 +210,14 @@ export class CliCredentialManager { /** * Wrap execFileAsync with a 60s timeout and periodic debug logging. */ - async #execWithTimeout( + private 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, { @@ -257,13 +233,16 @@ export class CliCredentialManager { /** * Write URL and token files under --global-config. */ - async #writeCredentialFiles(url: string, token: string): Promise { + 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), + this.atomicWriteFile(this.pathResolver.getUrlPath(safeHostname), url), + this.atomicWriteFile( + this.pathResolver.getSessionTokenPath(safeHostname), token, ), ]); @@ -275,52 +254,69 @@ export class CliCredentialManager { /** * Delete URL and token files. Best-effort: never throws. */ - async #deleteCredentialFiles(url: string): Promise { + private 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); }), ), ); } /** - * Delete keyring token via `coder logout`. Best-effort: never throws. + * 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. */ - async #deleteKeyringToken( - binPath: string, + private async deleteKeyringToken( url: string, configs: Pick, - signal?: AbortSignal, - ): Promise { + { 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("failure_category", "binary"); + span.markFailure(); + 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); - return {}; + 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); - return { failureCategory: "cli" }; + this.logger.warn("Failed to delete token via CLI:", error); + span.setProperty("failure_category", "cli"); + span.markFailure(); } } /** Atomically write content to a file. */ - async #atomicWriteFile(filePath: string, content: string): Promise { + 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), + this.logger.warn("Failed to delete temp file", tempPath, rmErr), ); } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 2031b2faf6..b65f0f0a6c 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,10 +10,6 @@ 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"; @@ -914,7 +910,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 }) => @@ -927,14 +923,13 @@ export class CliManager { }, ); if (result.ok) { - return result.value; + return; } if (result.cancelled) { this.output.info("Credential removal cancelled by user"); - return { failureCategory: "aborted" }; + } else { + this.output.warn("Failed to remove credentials:", result.error); } - this.output.warn("Failed to remove credentials:", result.error); - return { failureCategory: categorizeCredentialError(result.error) }; } private handleStoreError(error: unknown): void { diff --git a/src/instrumentation/auth.ts b/src/instrumentation/auth.ts index 0e85ba33b9..c4d5f2018b 100644 --- a/src/instrumentation/auth.ts +++ b/src/instrumentation/auth.ts @@ -8,41 +8,26 @@ 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 }; + | { success: false; method?: LoginMethod; reason: LoginPromptReason }; export type AuthLogoutOutcome = | { success: true } - | { success: false; reason: AuthLogoutReason }; + | { success: false; reason: "not_authenticated" }; interface AuthLoginTrace { - setMethod(method: LoginMethod): void; + setMethod: (method: LoginMethod) => void; } interface AuthRecoveryRecorder { @@ -51,16 +36,6 @@ 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) {} @@ -72,8 +47,15 @@ export class AuthTelemetry { "auth.login", async (span) => { try { - const result = await fn(createLoginTrace(span)); - recordLoginResult(span, result); + const result = await fn({ + setMethod: (method) => span.setProperty("method", method), + }); + if (result.method) { + span.setProperty("method", result.method); + } + if (!result.success) { + recordReason(span, result.reason); + } return result; } catch (error) { span.setProperty("reason", "exception"); @@ -84,13 +66,16 @@ export class AuthTelemetry { ); } - public traceLogout( - fn: () => Promise, - ): Promise { + public traceLogout( + fn: () => Promise, + ): Promise { return this.telemetry.trace("auth.logout", async (span) => { try { const result = await fn(); - recordLogoutResult(span, result); + if (!result.success) { + span.setProperty("reason", result.reason); + span.markAborted(); + } return result; } catch (error) { span.setProperty("reason", "exception"); @@ -144,7 +129,9 @@ export class AuthTelemetry { "auth.login_prompted", async (span) => { const result = await fn(); - recordPromptResult(span, result); + if (!result.success) { + recordReason(span, result.reason); + } return result; }, { trigger }, @@ -152,52 +139,12 @@ export class AuthTelemetry { } } -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 { +/** `auth_failed` is a real failure; user/URL dismissals are intentional aborts. */ +function recordReason(span: Span, reason: LoginPromptReason): void { span.setProperty("reason", reason); - if (reason === "auth_failed" || reason === "exception") { + if (reason === "auth_failed") { span.markFailure(); - return; + } else { + span.markAborted(); } - span.markAborted(); } diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts index 030c08b108..754b26e1a5 100644 --- a/src/instrumentation/credentials.ts +++ b/src/instrumentation/credentials.ts @@ -6,126 +6,71 @@ 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; -} +type CredentialEvent = "auth.credential.store" | "auth.credential.clear"; +/** + * Wraps credential store/clear in a span carrying `keyring_enabled`, the + * `category` of storage involved, and a `failure_category` on failure. The + * traced operation sets `category` on the span and reports failures by + * throwing a categorized error (store) or recording on the span (clear, which + * is best-effort). Aborts are recorded and re-thrown so callers still unwind. + */ export class CredentialTelemetry { public constructor(private readonly telemetry: TelemetryReporter) {} - public async traceStore( + public traceStore( configs: Pick, - fn: (trace: CredentialTrace) => Promise, - ): Promise { - return this.traceCredential("auth.credential.store", configs, fn); + fn: (span: Span) => Promise, + ): Promise { + return this.trace("auth.credential.store", configs, fn); } - public async traceClear( + public traceClear( configs: Pick, - fn: (trace: CredentialTrace) => Promise, - ): Promise { - return this.traceCredential( - "auth.credential.clear", - configs, - fn, - recordClearFailure, - ); + fn: (span: Span) => Promise, + ): Promise { + return this.trace("auth.credential.clear", configs, fn); } - private async traceCredential( - eventName: "auth.credential.store" | "auth.credential.clear", + private async trace( + eventName: CredentialEvent, 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( + fn: (span: Span) => Promise, + ): Promise { + const keyringEnabled = isKeyringEnabled(configs); + let aborted: Error | undefined; + await this.telemetry.trace( eventName, async (span) => { try { - const result = await fn(createCredentialTrace(span)); - recordResult?.(span, result); - return result; + await fn(span); } catch (error) { - recordCredentialFailure(span, defaults.category, error); + span.setProperty( + "failure_category", + categorizeCredentialError(error), + ); if (isAbortError(error)) { span.markAborted(); - cancellation = error; - return undefined as T; + aborted = error; + return; } throw error; } }, { - keyring_enabled: defaults.keyringEnabled, - category: defaults.category, + keyring_enabled: keyringEnabled, + category: keyringEnabled ? "keyring" : "file", }, ); - if (cancellation instanceof Error) { - throw cancellation; + if (aborted) { + throw aborted; } - 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 { +function categorizeCredentialError(error: unknown): CredentialFailureCategory { if (isAbortError(error)) { return "aborted"; } diff --git a/src/uri/uriHandler.ts b/src/uri/uriHandler.ts index 520654fba2..861eeff244 100644 --- a/src/uri/uriHandler.ts +++ b/src/uri/uriHandler.ts @@ -155,17 +155,9 @@ async function setupDeployment( const safeHostname = toSafeHost(url); const token: string | undefined = params.get("token") ?? undefined; - 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; - }); + const result = await authTelemetry.traceLogin("uri", () => + loginCoordinator.ensureLoggedIn({ safeHostname, url, token }), + ); if (!result.success) { throw new Error("Failed to login to deployment from URI"); diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index b135e5940f..f715d9b4a6 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({}), + deleteToken: vi.fn().mockResolvedValue(undefined), } as unknown as CliCredentialManager; } diff --git a/test/unit/commands.test.ts b/test/unit/commands.telemetry.test.ts similarity index 89% rename from test/unit/commands.test.ts rename to test/unit/commands.telemetry.test.ts index 2a799fd8fd..7dc8a2b5ee 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -19,7 +19,6 @@ 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"; @@ -52,7 +51,6 @@ interface CommandsHarnessOptions { readonly authenticated?: boolean; readonly loginResult?: LoginResultForTest; - readonly credentialResult?: CredentialClearResult; readonly clearDeploymentError?: Error; readonly clearAllAuthDataError?: Error; } @@ -97,9 +95,7 @@ function createCommandsHarness(options: CommandsHarnessOptions = {}) { }; const cliManager = { - clearCredentials: vi.fn(() => - Promise.resolve(options.credentialResult ?? {}), - ), + clearCredentials: vi.fn(() => Promise.resolve()), }; const secretsManager = { @@ -307,44 +303,6 @@ describe("Commands", () => { }, }); - 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, diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 1e2eb85818..200eb8801a 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -453,9 +453,9 @@ describe("CliCredentialManager", () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); const { manager, sink } = setup(failingResolver()); - await expect(manager.deleteToken(TEST_URL, configs)).resolves.toEqual({ - failureCategory: "binary", - }); + await expect( + manager.deleteToken(TEST_URL, configs), + ).resolves.toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index f9ddf3e1c7..4e0c185d1b 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 expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({}); + await manager.clearCredentials(CLEAR_URL); expect(vscode.window.withProgress).not.toHaveBeenCalled(); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( @@ -365,9 +365,8 @@ describe("CliManager", () => { it("should show progress notification when keyring is enabled", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); - vi.mocked(mockCredManager.deleteToken).mockResolvedValueOnce({}); - await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({}); + await manager.clearCredentials(CLEAR_URL); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ @@ -379,35 +378,15 @@ describe("CliManager", () => { ); }); - 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", - }); + 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(); }); }); diff --git a/test/unit/instrumentation/auth.test.ts b/test/unit/instrumentation/auth.test.ts index 1017fc7db5..de7a37be00 100644 --- a/test/unit/instrumentation/auth.test.ts +++ b/test/unit/instrumentation/auth.test.ts @@ -76,39 +76,34 @@ describe("AuthTelemetry", () => { expect(event.measurements.durationMs).toEqual(expect.any(Number)); }); - it("marks user cancellation as aborted with a bounded reason", async () => { + it("marks not_authenticated as aborted", async () => { const { auth, sink } = setup(); await auth.traceLogout(() => - Promise.resolve({ - success: false, - reason: "credential_clear_cancelled", - }), + Promise.resolve({ success: false, reason: "not_authenticated" }), ); expect(sink.expectOne("auth.logout")).toMatchObject({ properties: { result: "aborted", - reason: "credential_clear_cancelled", + reason: "not_authenticated", }, }); }); - it("marks credential clear failures as errors with a bounded reason", async () => { + it("records exceptions as errors", async () => { const { auth, sink } = setup(); - await auth.traceLogout(() => - Promise.resolve({ - success: false, - reason: "credential_clear_failed", - }), - ); + await expect( + auth.traceLogout(() => Promise.reject(new Error("clear failed"))), + ).rejects.toThrow("clear failed"); expect(sink.expectOne("auth.logout")).toMatchObject({ properties: { result: "error", - reason: "credential_clear_failed", + reason: "exception", }, + error: { message: "clear failed" }, }); }); }); diff --git a/test/unit/uri/uriHandler.test.ts b/test/unit/uri/uriHandler.test.ts index 7e448076d6..4c87ae72f6 100644 --- a/test/unit/uri/uriHandler.test.ts +++ b/test/unit/uri/uriHandler.test.ts @@ -6,6 +6,7 @@ import { SecretsManager } from "@/core/secretsManager"; import { OAuthCallback } from "@/oauth/oauthCallback"; import { CALLBACK_PATH } from "@/oauth/utils"; import { maybeAskUrl } from "@/promptUtils"; +import { NOOP_TELEMETRY_REPORTER } from "@/telemetry/reporter"; import { registerUriHandler } from "@/uri/uriHandler"; import { @@ -81,6 +82,7 @@ function createTestContext() { getContextManager: () => new MockContextManager(), getOAuthCallback: () => oauthCallback, getLogger: () => logger, + getTelemetryService: () => NOOP_TELEMETRY_REPORTER, } as unknown as ServiceContainer; vi.mocked(maybeAskUrl).mockImplementation((_m, urlParam) => From 9f32eecde4634ee63d43631b12268c73150a5d77 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 10 Jun 2026 13:42:09 +0300 Subject: [PATCH 6/6] refactor: apply telemetry review conventions Make telemetry dimensions explicit at every call site: suspendSession and clearDeployment now require a reason instead of defaulting (the suspendSession default was never used in production), and CliCredentialManager requires its TelemetryReporter rather than silently falling back to the noop reporter. Replace the scenario DSL in commands.telemetry.test.ts with plain it() blocks on a shared setup() that reuses createTelemetryHarness, and drop the unused clearDeploymentError option. --- src/core/cliCredentialManager.ts | 7 +- src/deployment/deploymentManager.ts | 10 +- test/unit/commands.telemetry.test.ts | 314 ++++++++---------- .../unit/deployment/deploymentManager.test.ts | 18 +- 4 files changed, 148 insertions(+), 201 deletions(-) diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 61d4163b17..a6399cc1a6 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -14,10 +14,7 @@ import { } from "../instrumentation/credentials"; import { isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; -import { - NOOP_TELEMETRY_REPORTER, - type TelemetryReporter, -} from "../telemetry/reporter"; +import { type TelemetryReporter } from "../telemetry/reporter"; import { toSafeHost } from "../util"; import { writeAtomically } from "../util/fs"; @@ -62,7 +59,7 @@ export class CliCredentialManager { private readonly logger: Logger, private readonly resolveBinary: BinaryResolver, private readonly pathResolver: PathResolver, - telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, + telemetry: TelemetryReporter, ) { this.credentialTelemetry = new CredentialTelemetry(telemetry); } diff --git a/src/deployment/deploymentManager.ts b/src/deployment/deploymentManager.ts index a979348711..8ef71d56e7 100644 --- a/src/deployment/deploymentManager.ts +++ b/src/deployment/deploymentManager.ts @@ -182,9 +182,7 @@ export class DeploymentManager implements vscode.Disposable { /** * Clears the current deployment. */ - public async clearDeployment( - reason: DeploymentSuspendReason = "credentials_removed", - ): Promise { + public async clearDeployment(reason: DeploymentSuspendReason): Promise { this.logger.debug("Clearing deployment", this.#deployment?.safeHostname); this.suspendSession(reason); this.#authListenerDisposable?.dispose(); @@ -199,9 +197,7 @@ 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( - reason: DeploymentSuspendReason = "auth_config_change", - ): void { + public suspendSession(reason: DeploymentSuspendReason): void { const wasAuthenticated = this.isAuthenticated(); this.oauthSessionManager.clearDeployment(); this.client.setCredentials(undefined, undefined); @@ -267,7 +263,7 @@ export class DeploymentManager implements vscode.Disposable { ); } } else { - await this.clearDeployment(); + await this.clearDeployment("credentials_removed"); } }, ); diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index 7dc8a2b5ee..aa0a416377 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -3,11 +3,10 @@ import { describe, expect, it, vi } from "vitest"; import { Commands } from "@/commands"; import { maybeAskUrl } from "@/promptUtils"; -import { createTestTelemetryService, TestSink } from "../mocks/telemetry"; +import { createTelemetryHarness } from "../mocks/telemetry"; import { createMockLogger, createMockUser, - MockConfigurationProvider, MockUserInteraction, } from "../mocks/testHelpers"; @@ -37,33 +36,22 @@ vi.mock("@/workspace/workspacesProvider", () => { 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 { +interface SetupOptions { readonly authenticated?: boolean; - readonly loginResult?: LoginResultForTest; - readonly clearDeploymentError?: Error; readonly clearAllAuthDataError?: Error; } -function createCommandsHarness(options: CommandsHarnessOptions = {}) { +function setup(options: SetupOptions = {}) { vi.clearAllMocks(); - new MockConfigurationProvider(); new MockUserInteraction(); vi.mocked(maybeAskUrl).mockResolvedValue(TEST_URL); - const sink = new TestSink(); - const telemetry = createTestTelemetryService(sink); - const logger = createMockLogger(); + const { sink, service } = createTelemetryHarness(); const deployment: Deployment = { url: TEST_URL, safeHostname: TEST_HOSTNAME, @@ -76,29 +64,31 @@ function createCommandsHarness(options: CommandsHarnessOptions = {}) { user: createMockUser(), token: "test-token", } satisfies LoginResultForTest); - const loginCoordinator = { - ensureLoggedIn: vi.fn((_loginOptions: LoginOptionsForTest) => - Promise.resolve(loginResult), - ), + const loginCoordinator: Pick = { + ensureLoggedIn: vi.fn(() => Promise.resolve(loginResult)), }; - const deploymentManager = { + const deploymentManager: Pick< + DeploymentManager, + | "isAuthenticated" + | "getCurrentDeployment" + | "setDeployment" + | "clearDeployment" + > = { 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(); - }), + clearDeployment: vi.fn(() => Promise.resolve()), }; - const cliManager = { + const cliManager: Pick = { clearCredentials: vi.fn(() => Promise.resolve()), }; - const secretsManager = { + const secretsManager: Pick< + SecretsManager, + "getCurrentDeployment" | "clearAllAuthData" + > = { getCurrentDeployment: vi.fn(() => Promise.resolve(null)), clearAllAuthData: vi.fn(() => { if (options.clearAllAuthDataError) { @@ -109,16 +99,16 @@ function createCommandsHarness(options: CommandsHarnessOptions = {}) { }; const serviceContainer = { - getTelemetryService: () => telemetry, - getLogger: () => logger, + getTelemetryService: () => service, + getLogger: () => createMockLogger(), getPathResolver: () => ({}) as PathResolver, getMementoManager: () => ({}) as MementoManager, - getSecretsManager: () => secretsManager as unknown as SecretsManager, - getCliManager: () => cliManager as unknown as CliManager, - getLoginCoordinator: () => loginCoordinator as unknown as LoginCoordinator, + getSecretsManager: () => secretsManager, + getCliManager: () => cliManager, + getLoginCoordinator: () => loginCoordinator, getDuplicateWorkspaceIpc: () => ({}) as DuplicateWorkspaceIpc, getSpeedtestPanelFactory: () => ({}) as SpeedtestPanelFactory, - } as unknown as ServiceContainer; + } as ServiceContainer; const extensionClient = { getAxiosInstance: () => ({ defaults: { baseURL: TEST_URL } }), @@ -127,192 +117,156 @@ function createCommandsHarness(options: CommandsHarnessOptions = {}) { const commands = new Commands( serviceContainer, extensionClient, - deploymentManager as unknown as DeploymentManager, + deploymentManager as DeploymentManager, ); return { commands, - deployment, - telemetry: sink, - mocks: { - cliManager, - deploymentManager, - loginCoordinator, - secretsManager, - }, + 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(); - }, + it("emits one auth.login for command login success", async () => { + const { commands, mocks, sink } = setup(); + + await commands.login(); + + const events = sink.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: { + it("uses auto_login source when requested", async () => { + const { commands, mocks, sink } = setup({ 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 }), - ); - }, + }); + + await commands.login({ url: TEST_URL, autoLogin: true }); + + expect(sink.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(); - }, + it("records URL cancellation without attempting login", async () => { + const { commands, mocks, sink } = setup(); + vi.mocked(maybeAskUrl).mockResolvedValueOnce(undefined); + + await commands.login(); + + expect(sink.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: { + it("records auth failures as auth.login errors", async () => { + const { commands, mocks, sink } = setup({ 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(); - }, + }); + + await commands.login(); + + expect(sink.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", - }); - }, + it("uses switch_deployment source", async () => { + const { commands, sink } = setup(); + + await commands.switchDeployment(); + + expect(sink.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(); - }, + it("records not_authenticated as aborted", async () => { + const { commands, mocks, sink } = setup({ authenticated: false }); + + await commands.logout(); + + expect(sink.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, - ); - }, + it("records successful logout", async () => { + const { commands, mocks, sink } = setup({ authenticated: true }); + + await commands.logout(); + + const event = sink.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, + ); }); it("records logout exceptions", async () => { - const harness = createCommandsHarness({ + const { commands, sink } = setup({ 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({ + await expect(commands.logout()).rejects.toThrow("secret clear failed"); + + expect(sink.expectOne("auth.logout")).toMatchObject({ properties: { result: "error", reason: "exception" }, error: { message: "secret clear failed" }, }); diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 3f4e157f44..e47c75cacb 100644 --- a/test/unit/deployment/deploymentManager.test.ts +++ b/test/unit/deployment/deploymentManager.test.ts @@ -147,7 +147,7 @@ describe("DeploymentManager", () => { user, }); - await manager.clearDeployment(); + await manager.clearDeployment("credentials_removed"); expect(manager.getCurrentDeployment()).toBeNull(); expect(manager.isAuthenticated()).toBe(false); @@ -418,7 +418,7 @@ describe("DeploymentManager", () => { user, }); - await manager.clearDeployment(); + await manager.clearDeployment("credentials_removed"); expect(mockClient.host).toBeUndefined(); expect(mockClient.token).toBeUndefined(); @@ -435,7 +435,7 @@ describe("DeploymentManager", () => { token: "test-token", user: createMockUser(), }); - await manager.clearDeployment(); + await manager.clearDeployment("credentials_removed"); expect(setDeploymentUrlSpy).toHaveBeenLastCalledWith(""); }); @@ -477,7 +477,7 @@ describe("DeploymentManager", () => { }); expect(manager.isAuthenticated()).toBe(true); - manager.suspendSession(); + manager.suspendSession("auth_failure"); // Auth state is cleared expect(mockOAuthSessionManager.clearDeployment).toHaveBeenCalled(); @@ -511,7 +511,7 @@ describe("DeploymentManager", () => { token: "", user, }); - manager.suspendSession(); + manager.suspendSession("auth_failure"); expect(manager.isAuthenticated()).toBe(false); config.set("coder.tlsCertFile", "/path/to/cert.pem"); @@ -553,11 +553,11 @@ describe("DeploymentManager", () => { token: "", user, }); - manager.suspendSession(); + manager.suspendSession("auth_failure"); config.set("coder.tlsCertFile", "/path/to/cert.pem"); await vi.advanceTimersByTimeAsync(CONFIG_CHANGE_DEBOUNCE_MS); - await manager.clearDeployment(); + await manager.clearDeployment("credentials_removed"); resolveAuth(user); await vi.runAllTimersAsync(); @@ -588,7 +588,7 @@ describe("DeploymentManager", () => { }); // Suspend session (simulates session expiry) - manager.suspendSession(); + manager.suspendSession("auth_failure"); expect(manager.isAuthenticated()).toBe(false); // Simulate token update (e.g., from another window or re-login) @@ -621,7 +621,7 @@ describe("DeploymentManager", () => { token: "test-token", user, }); - manager.suspendSession(); + manager.suspendSession("auth_failure"); validationMockClient.setAuthenticatedUserResponse( new Error("Auth failed"), );