From 51c068ee0850b80e3fe4c68de7b492e0c98a0591 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 8 Jun 2026 10:39:07 +0000 Subject: [PATCH 1/3] 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/3] 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/3] 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 {