From 04871245bc9843d5e053357446f642e5627a4baf Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Fri, 22 May 2026 18:01:16 +0000 Subject: [PATCH] fix: recover mTLS auth after settings races --- src/api/authInterceptor.ts | 45 ++++- src/api/coderApi.ts | 76 ++++++-- src/configWatcher.ts | 53 +++-- src/deployment/deploymentManager.ts | 74 ++++++- src/login/loginCoordinator.ts | 20 +- src/oauth/authorizer.ts | 26 ++- src/oauth/sessionManager.ts | 182 +++++++++--------- src/remote/remote.ts | 43 +++-- src/settings/authConfig.ts | 58 ++++++ test/mocks/testHelpers.ts | 4 + test/unit/api/authInterceptor.test.ts | 81 ++++++++ test/unit/api/coderApi.test.ts | 25 ++- test/unit/configWatcher.test.ts | 68 ++++++- .../unit/deployment/deploymentManager.test.ts | 81 +++++++- test/unit/login/loginCoordinator.test.ts | 1 + 15 files changed, 679 insertions(+), 158 deletions(-) create mode 100644 src/settings/authConfig.ts diff --git a/src/api/authInterceptor.ts b/src/api/authInterceptor.ts index f943aa6a8b..3a22153a5e 100644 --- a/src/api/authInterceptor.ts +++ b/src/api/authInterceptor.ts @@ -58,6 +58,14 @@ export class AuthInterceptor implements vscode.Disposable { throw error; } + if (error.response?.status !== 401) { + throw error; + } + + if (this.shouldRetryAfterAuthConfigChange(error)) { + return this.retryAfterAuthConfigChange(error); + } + if (error.config) { const config = error.config as { _retryAttempted?: boolean }; if (config._retryAttempted) { @@ -65,10 +73,6 @@ export class AuthInterceptor implements vscode.Disposable { } } - if (error.response?.status !== 401) { - throw error; - } - const baseUrl = this.client.getHost(); if (!baseUrl) { throw error; @@ -78,6 +82,39 @@ export class AuthInterceptor implements vscode.Disposable { return this.recoverFromUnauthorized(error, hostname); } + private shouldRetryAfterAuthConfigChange(error: AxiosError): boolean { + if (!error.config) { + return false; + } + + const config = error.config as RequestConfigWithMeta & { + _retryAttempted?: boolean; + _authConfigRetryAttempted?: boolean; + authConfigVersion?: number; + }; + // Skip if any retry already happened: caps total attempts at 2. + if (config._retryAttempted || config._authConfigRetryAttempted) { + return false; + } + + return this.client.hasAuthConfigChangedSince(config.authConfigVersion); + } + + private retryAfterAuthConfigChange(error: AxiosError): Promise { + if (!error.config) { + throw error; + } + + const config = error.config as RequestConfigWithMeta & { + _authConfigRetryAttempted?: boolean; + }; + config._authConfigRetryAttempted = true; + this.logger.debug( + "Authentication settings changed during request, retrying once", + ); + return this.client.getAxiosInstance().request(config); + } + private recoverFromUnauthorized( error: AxiosError, hostname: string, diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index c113a7cc18..ff784d5a71 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -8,7 +8,10 @@ import { import { Api } from "coder/site/src/api/api"; import * as vscode from "vscode"; -import { watchConfigurationChanges } from "../configWatcher"; +import { + CONFIG_CHANGE_DEBOUNCE_MS, + watchConfigurationChanges, +} from "../configWatcher"; import { ClientCertificateError } from "../error/clientCertificateError"; import { toError } from "../error/errorUtils"; import { ServerCertificateError } from "../error/serverCertificateError"; @@ -26,6 +29,7 @@ import { type RequestConfigWithMeta, } from "../logging/types"; import { sizeOf } from "../logging/utils"; +import { AuthConfigTracker } from "../settings/authConfig"; import { getHeaderCommand } from "../settings/headers"; import { NOOP_TELEMETRY_REPORTER, @@ -98,6 +102,7 @@ export class CoderApi extends Api implements vscode.Disposable { private readonly output: Logger, private readonly telemetry: TelemetryReporter, private readonly httpRequestsTelemetry: HttpRequestsTelemetry, + private readonly authConfigTracker: AuthConfigTracker, ) { super(); this.configWatcher = this.watchConfigChanges(); @@ -117,10 +122,16 @@ export class CoderApi extends Api implements vscode.Disposable { telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, ): CoderApi { const httpRequestsTelemetry = new HttpRequestsTelemetry(telemetry); - const client = new CoderApi(output, telemetry, httpRequestsTelemetry); + const authConfigTracker = new AuthConfigTracker(); + const client = new CoderApi( + output, + telemetry, + httpRequestsTelemetry, + authConfigTracker, + ); client.setCredentials(baseUrl, token); - setupInterceptors(client, output, httpRequestsTelemetry); + setupInterceptors(client, output, httpRequestsTelemetry, authConfigTracker); return client; } @@ -128,6 +139,10 @@ export class CoderApi extends Api implements vscode.Disposable { return this.getAxiosInstance().defaults.baseURL; } + hasAuthConfigChangedSince(version: number | undefined): boolean { + return this.authConfigTracker.hasChangedSince(version); + } + /** * Set both host and token together. Useful for login/logout/switch to * avoid triggering multiple reconnection events. @@ -172,6 +187,7 @@ export class CoderApi extends Api implements vscode.Disposable { */ dispose(): void { this.configWatcher.dispose(); + this.authConfigTracker.dispose(); this.httpRequestsTelemetry.dispose(); for (const socket of this.reconnectingSockets) { socket.close(); @@ -189,20 +205,24 @@ export class CoderApi extends Api implements vscode.Disposable { setting, getValue: () => vscode.workspace.getConfiguration().get(setting), })); - return watchConfigurationChanges(settings, () => { - const socketsToReconnect = [...this.reconnectingSockets].filter( - (socket) => socket.state === ConnectionState.DISCONNECTED, - ); - if (socketsToReconnect.length) { - this.output.debug( - `Configuration changed, ${socketsToReconnect.length}/${this.reconnectingSockets.size} socket(s) in DISCONNECTED state`, + return watchConfigurationChanges( + settings, + () => { + const socketsToReconnect = [...this.reconnectingSockets].filter( + (socket) => socket.state === ConnectionState.DISCONNECTED, ); - for (const socket of socketsToReconnect) { - this.output.debug(`Reconnecting WebSocket: ${socket.url}`); - socket.reconnect(); + if (socketsToReconnect.length) { + this.output.debug( + `Configuration changed, ${socketsToReconnect.length}/${this.reconnectingSockets.size} socket(s) in DISCONNECTED state`, + ); + for (const socket of socketsToReconnect) { + this.output.debug(`Reconnecting WebSocket: ${socket.url}`); + socket.reconnect(); + } } - } - }); + }, + { debounceMs: CONFIG_CHANGE_DEBOUNCE_MS }, + ); } watchInboxNotifications = async ( @@ -491,10 +511,16 @@ export class CoderApi extends Api implements vscode.Disposable { } } +type RequestConfigWithAuthMeta = RequestConfigWithMeta & { + authConfigVersion?: number; + headerCommandKeys?: string[]; +}; + function setupInterceptors( client: CoderApi, output: Logger, httpRequestsTelemetry: HttpRequestsTelemetry, + authConfigTracker: AuthConfigTracker, ): void { addRequestInterceptors( client.getAxiosInstance(), @@ -503,25 +529,37 @@ function setupInterceptors( ); client.getAxiosInstance().interceptors.request.use(async (config) => { + const configWithAuthMeta = config as RequestConfigWithAuthMeta; + + for (const key of configWithAuthMeta.headerCommandKeys ?? []) { + config.headers.delete(key); + } + const baseUrl = client.getAxiosInstance().defaults.baseURL; const headers = await getHeaders( baseUrl, getHeaderCommand(vscode.workspace.getConfiguration()), output, ); - // Add headers from the header command. for (const [key, value] of Object.entries(headers)) { config.headers[key] = value; } + // Skip the session-token header: retryRequest writes it on the same + // config and the retry cleanup would clobber the refreshed token. + configWithAuthMeta.headerCommandKeys = Object.keys(headers).filter( + (k) => k.toLowerCase() !== coderSessionTokenHeader.toLowerCase(), + ); - // Configure proxy and TLS. - // Note that by default VS Code overrides the agent. To prevent this, set - // `http.proxySupport` to `on` or `off`. + // VS Code overrides the agent by default; set `http.proxySupport` to + // `on` or `off` to keep ours. const agent = await createHttpAgent(vscode.workspace.getConfiguration()); config.httpsAgent = agent; config.httpAgent = agent; config.proxy = false; + // Stamp last so the version matches the config actually used. + configWithAuthMeta.authConfigVersion = authConfigTracker.version; + return config; }); diff --git a/src/configWatcher.ts b/src/configWatcher.ts index cc026c9087..9a094c64e5 100644 --- a/src/configWatcher.ts +++ b/src/configWatcher.ts @@ -1,43 +1,74 @@ import { isDeepStrictEqual } from "node:util"; import * as vscode from "vscode"; +/** + * Debounce window for config-change reactions that fan out (recovery, + * reconnect, reload prompts). Keeps rapid edits in settings.json from + * flushing each side-effect per keystroke. + */ +export const CONFIG_CHANGE_DEBOUNCE_MS = 250; + export interface WatchedSetting { setting: string; getValue: () => unknown; } +export interface WatchConfigurationChangesOptions { + debounceMs?: number; +} + /** * Watch for configuration changes and invoke a callback when values change. - * The callback receives a map of changed settings to their new values, so - * consumers can act on the new value without re-reading the configuration. - * Only fires when actual values change, not just when settings are touched. + * Fires only when actual values change. With `debounceMs`, the first event + * opens a fixed collection window; subsequent events during the window are + * coalesced. This bounds latency even when events arrive faster than the + * window length (a reset-style debounce would starve). */ export function watchConfigurationChanges( settings: WatchedSetting[], onChange: (changes: ReadonlyMap) => void, + options: WatchConfigurationChangesOptions = {}, ): vscode.Disposable { const appliedValues = new Map(settings.map((s) => [s.setting, s.getValue()])); - return vscode.workspace.onDidChangeConfiguration((e) => { + const detectAndFire = () => { const changes = new Map(); - for (const { setting, getValue } of settings) { - if (!e.affectsConfiguration(setting)) { - continue; - } - const newValue = getValue(); - if (!configValuesEqual(newValue, appliedValues.get(setting))) { changes.set(setting, newValue); appliedValues.set(setting, newValue); } } - if (changes.size > 0) { onChange(changes); } + }; + + let windowTimer: ReturnType | undefined; + const listener = vscode.workspace.onDidChangeConfiguration((e) => { + if (!settings.some((s) => e.affectsConfiguration(s.setting))) { + return; + } + if (!options.debounceMs) { + detectAndFire(); + return; + } + if (windowTimer) { + return; // already collecting in the open window + } + windowTimer = setTimeout(() => { + windowTimer = undefined; + detectAndFire(); + }, options.debounceMs); }); + + return { + dispose: () => { + clearTimeout(windowTimer); + listener.dispose(); + }, + }; } function configValuesEqual(a: unknown, b: unknown): boolean { diff --git a/src/deployment/deploymentManager.ts b/src/deployment/deploymentManager.ts index 738f87b776..88cb2b52c6 100644 --- a/src/deployment/deploymentManager.ts +++ b/src/deployment/deploymentManager.ts @@ -1,10 +1,15 @@ import { CoderApi } from "../api/coderApi"; +import { + CONFIG_CHANGE_DEBOUNCE_MS, + watchConfigurationChanges, +} from "../configWatcher"; 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 { type Logger } from "../logging/logger"; import { type OAuthSessionManager } from "../oauth/sessionManager"; +import { getAuthConfigWatchSettings } from "../settings/authConfig"; import { type TelemetryService } from "../telemetry/service"; import { type WorkspaceProvider } from "../workspace/workspacesProvider"; @@ -37,7 +42,11 @@ export class DeploymentManager implements vscode.Disposable { private readonly telemetryService: TelemetryService; #deployment: Deployment | null = null; + #disposed = false; #authListenerDisposable: vscode.Disposable | undefined; + #authConfigDisposable: vscode.Disposable | undefined; + #recoveryPromise: Promise | null = null; + #recoveryPending = false; #crossWindowSyncDisposable: vscode.Disposable | undefined; private constructor( @@ -65,6 +74,7 @@ export class DeploymentManager implements vscode.Disposable { oauthSessionManager, workspaceProviders, ); + manager.subscribeToAuthConfigChanges(); manager.subscribeToCrossWindowChanges(); return manager; } @@ -84,12 +94,13 @@ export class DeploymentManager implements vscode.Disposable { } /** - * Attempt to change to a deployment after validating authentication. - * Only changes deployment if authentication succeeds. - * Returns true if deployment was changed, false otherwise. + * Validate authentication and switch on success. `shouldApply`, if given, + * is checked after validation; the write is skipped when it returns false + * (used to bail out if state mutated during the validation await). */ public async setDeploymentIfValid( deployment: Deployment & { token?: string }, + shouldApply: () => boolean = () => true, ): Promise { const token = deployment.token ?? @@ -99,13 +110,10 @@ export class DeploymentManager implements vscode.Disposable { try { const user = await tempClient.getAuthenticatedUser(); - - // Authentication succeeded - now change the deployment - await this.setDeployment({ - ...deployment, - token, - user, - }); + if (!shouldApply()) { + return false; + } + await this.setDeployment({ ...deployment, token, user }); return true; } catch (e) { this.logger.warn("Failed to authenticate with deployment:", e); @@ -186,7 +194,9 @@ export class DeploymentManager implements vscode.Disposable { } public dispose(): void { + this.#disposed = true; this.#authListenerDisposable?.dispose(); + this.#authConfigDisposable?.dispose(); this.#crossWindowSyncDisposable?.dispose(); } @@ -232,6 +242,50 @@ export class DeploymentManager implements vscode.Disposable { ); } + private subscribeToAuthConfigChanges(): void { + this.#authConfigDisposable = watchConfigurationChanges( + getAuthConfigWatchSettings(), + () => this.onAuthConfigChange(), + { debounceMs: CONFIG_CHANGE_DEBOUNCE_MS }, + ); + } + + private onAuthConfigChange(): void { + // One recovery at a time; mark pending so a settings change during the + // current pass triggers a fresh attempt once it settles. + if (this.#recoveryPromise) { + this.#recoveryPending = true; + return; + } + this.#recoveryPromise = this.runRecovery(); + } + + private async runRecovery(): Promise { + try { + do { + this.#recoveryPending = false; + const snapshot = this.#deployment; + if (this.#disposed || !snapshot || this.isAuthenticated()) { + return; + } + this.logger.debug( + "Authentication settings changed after session suspended, recovering", + ); + await this.setDeploymentIfValid( + snapshot, + () => !this.#disposed && this.#deployment === snapshot, + ); + } while (this.#recoveryPending); + } catch (err) { + this.logger.warn( + "Failed to recover session after authentication settings changed", + err, + ); + } finally { + this.#recoveryPromise = null; + } + } + private subscribeToCrossWindowChanges(): void { this.#crossWindowSyncDisposable = this.secretsManager.onDidChangeCurrentDeployment( diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 67dc5693ed..d1e5e7234f 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -214,7 +214,8 @@ export class LoginCoordinator implements vscode.Disposable { resolve({ success: true, token: auth.token, user }); } catch { // Token from other window was invalid, ignore and keep waiting - // (or user can click Login/Cancel in the dialog) + } finally { + client.dispose(); } } }, @@ -240,7 +241,24 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, ): Promise { const client = CoderApi.create(deployment.url, "", this.logger); + try { + return await this.runLoginAttempts( + client, + deployment, + isAutoLogin, + providedToken, + ); + } finally { + client.dispose(); + } + } + private async runLoginAttempts( + client: CoderApi, + deployment: Deployment, + isAutoLogin: boolean, + providedToken: string | undefined, + ): Promise { // mTLS authentication (no token needed) if (!needToken(vscode.workspace.getConfiguration())) { this.logger.debug("Attempting mTLS authentication (no token required)"); diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index 7280e74202..3d45bd61d9 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -56,6 +56,25 @@ export class OAuthAuthorizer implements vscode.Disposable { deployment: Deployment, progress: vscode.Progress<{ message?: string; increment?: number }>, cancellationToken: vscode.CancellationToken, + ): Promise<{ tokenResponse: OAuth2TokenResponse; user: User }> { + const client = CoderApi.create(deployment.url, undefined, this.logger); + try { + return await this.runLoginFlow( + client, + deployment, + progress, + cancellationToken, + ); + } finally { + client.dispose(); + } + } + + private async runLoginFlow( + client: CoderApi, + deployment: Deployment, + progress: vscode.Progress<{ message?: string; increment?: number }>, + cancellationToken: vscode.CancellationToken, ): Promise<{ tokenResponse: OAuth2TokenResponse; user: User }> { const reportProgress = (message?: string, increment?: number): void => { if (cancellationToken.isCancellationRequested) { @@ -64,7 +83,6 @@ export class OAuthAuthorizer implements vscode.Disposable { progress.report({ message, increment }); }; - const client = CoderApi.create(deployment.url, undefined, this.logger); const axiosInstance = client.getAxiosInstance(); reportProgress("fetching metadata...", 10); @@ -94,7 +112,6 @@ export class OAuthAuthorizer implements vscode.Disposable { registration, ); - // Set token on client to fetch user client.setSessionToken(tokenResponse.access_token); reportProgress("fetching user...", 20); @@ -102,10 +119,7 @@ export class OAuthAuthorizer implements vscode.Disposable { this.logger.debug("OAuth login flow completed successfully"); - return { - tokenResponse, - user, - }; + return { tokenResponse, user }; } /** diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 2357fb0ae7..b8ef0ee003 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -285,29 +285,39 @@ export class OAuthSessionManager implements vscode.Disposable { } /** - * Prepare common OAuth operation setup: client, metadata, and registration. - * Used by refresh and revoke operations to reduce duplication. + * Run `fn` with a per-call CoderApi configured for the current + * deployment. The client is disposed on exit so its config-change + * subscriptions never outlive the operation. */ - private async prepareOAuthOperation(token?: string): Promise<{ - axiosInstance: AxiosInstance; - metadata: OAuth2AuthorizationServerMetadata; - registration: OAuth2ClientRegistrationResponse; - }> { + private async withOAuthOperation( + token: string | undefined, + fn: (ctx: { + axiosInstance: AxiosInstance; + metadata: OAuth2AuthorizationServerMetadata; + registration: OAuth2ClientRegistrationResponse; + }) => Promise, + ): Promise { const deployment = this.requireDeployment(); const client = CoderApi.create(deployment.url, token, this.logger); - const axiosInstance = client.getAxiosInstance(); + try { + const axiosInstance = client.getAxiosInstance(); + const metadataClient = new OAuthMetadataClient( + axiosInstance, + this.logger, + ); + const metadata = await metadataClient.getMetadata(); - const metadataClient = new OAuthMetadataClient(axiosInstance, this.logger); - const metadata = await metadataClient.getMetadata(); + const registration = await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + ); + if (!registration) { + throw new Error("No client registration found"); + } - const registration = await this.secretsManager.getOAuthClientRegistration( - deployment.safeHostname, - ); - if (!registration) { - throw new Error("No client registration found"); + return await fn({ axiosInstance, metadata, registration }); + } finally { + client.dispose(); } - - return { axiosInstance, metadata, registration }; } public async setDeployment(deployment: Deployment): Promise { @@ -382,46 +392,45 @@ export class OAuthSessionManager implements vscode.Disposable { const refreshToken = storedTokens.refresh_token; const accessToken = storedTokens.access_token; - const { axiosInstance, metadata, registration } = - await this.prepareOAuthOperation(accessToken); + return await this.withOAuthOperation( + accessToken, + async ({ axiosInstance, metadata, registration }) => { + this.logger.debug("Refreshing access token"); + + const params: OAuth2TokenRequest = { + grant_type: REFRESH_GRANT_TYPE, + refresh_token: refreshToken, + client_id: registration.client_id, + client_secret: registration.client_secret, + }; + + const response = await axiosInstance.post( + metadata.token_endpoint, + toUrlSearchParams(params), + { + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + signal: abortController.signal, + }, + ); - this.logger.debug("Refreshing access token"); + if (abortController.signal.aborted) { + throw new Error("Token refresh aborted"); + } - const params: OAuth2TokenRequest = { - grant_type: REFRESH_GRANT_TYPE, - refresh_token: refreshToken, - client_id: registration.client_id, - client_secret: registration.client_secret, - }; + this.logger.debug("Token refresh successful"); - const tokenRequest = toUrlSearchParams(params); + const oauthData = buildOAuthTokenData(response.data); + await this.secretsManager.setSessionAuth(deployment.safeHostname, { + url: deployment.url, + token: response.data.access_token, + oauth: oauthData, + }); - const response = await axiosInstance.post( - metadata.token_endpoint, - tokenRequest, - { - headers: { - "Content-Type": "application/x-www-form-urlencoded", - }, - signal: abortController.signal, + return response.data; }, ); - - // Check if aborted between response and save - if (abortController.signal.aborted) { - throw new Error("Token refresh aborted"); - } - - this.logger.debug("Token refresh successful"); - - const oauthData = buildOAuthTokenData(response.data); - await this.secretsManager.setSessionAuth(deployment.safeHostname, { - url: deployment.url, - token: response.data.access_token, - oauth: oauthData, - }); - - return response.data; } catch (error) { throw parseOAuthError(error) ?? error; } finally { @@ -458,43 +467,42 @@ export class OAuthSessionManager implements vscode.Disposable { tokenToRevoke: string, tokenTypeHint: "access_token" | "refresh_token" = "refresh_token", ): Promise { - const { axiosInstance, metadata, registration } = - await this.prepareOAuthOperation(authToken); - - if (!metadata.revocation_endpoint) { - this.logger.debug( - "No revocation endpoint available, skipping revocation", - ); - return; - } - - this.logger.debug("Revoking refresh token"); - - const params: OAuth2TokenRevocationRequest = { - token: tokenToRevoke, - client_id: registration.client_id, - client_secret: registration.client_secret, - token_type_hint: tokenTypeHint, - }; - - const revocationRequest = toUrlSearchParams(params); - - try { - await axiosInstance.post( - metadata.revocation_endpoint, - revocationRequest, - { - headers: { - "Content-Type": "application/x-www-form-urlencoded", - }, - }, - ); + await this.withOAuthOperation( + authToken, + async ({ axiosInstance, metadata, registration }) => { + if (!metadata.revocation_endpoint) { + this.logger.debug( + "No revocation endpoint available, skipping revocation", + ); + return; + } - this.logger.debug("Token revocation successful"); - } catch (error) { - this.logger.error("Token revocation failed:", error); - throw error; - } + this.logger.debug("Revoking refresh token"); + + const params: OAuth2TokenRevocationRequest = { + token: tokenToRevoke, + client_id: registration.client_id, + client_secret: registration.client_secret, + token_type_hint: tokenTypeHint, + }; + + try { + await axiosInstance.post( + metadata.revocation_endpoint, + toUrlSearchParams(params), + { + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + }, + ); + this.logger.debug("Token revocation successful"); + } catch (error) { + this.logger.error("Token revocation failed:", error); + throw error; + } + }, + ); } /** diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 71fbbe5a2d..1e8012f247 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -21,7 +21,10 @@ import { AuthInterceptor } from "../api/authInterceptor"; import { CoderApi } from "../api/coderApi"; import { needToken } from "../api/utils"; import { type Commands } from "../commands"; -import { watchConfigurationChanges } from "../configWatcher"; +import { + CONFIG_CHANGE_DEBOUNCE_MS, + watchConfigurationChanges, +} from "../configWatcher"; import { version as cliVersion } from "../core/cliExec"; import { type CliManager } from "../core/cliManager"; import { type ServiceContainer } from "../core/container"; @@ -985,22 +988,28 @@ export class Remote { ): vscode.Disposable { const titleMap = new Map(settings.map((s) => [s.setting, s.title])); - return watchConfigurationChanges(settings, (changes) => { - const changedTitles = [...changes.keys()] - .map((s) => titleMap.get(s)) - .filter((t) => t !== undefined); - - const message = - changedTitles.length === 1 - ? `${changedTitles[0]} setting changed. Reload window to apply.` - : `${changedTitles.join(", ")} settings changed. Reload window to apply.`; - - vscode.window.showInformationMessage(message, "Reload").then((action) => { - if (action === "Reload") { - vscode.commands.executeCommand("workbench.action.reloadWindow"); - } - }); - }); + return watchConfigurationChanges( + settings, + (changes) => { + const changedTitles = [...changes.keys()] + .map((s) => titleMap.get(s)) + .filter((t) => t !== undefined); + + const message = + changedTitles.length === 1 + ? `${changedTitles[0]} setting changed. Reload window to apply.` + : `${changedTitles.join(", ")} settings changed. Reload window to apply.`; + + vscode.window + .showInformationMessage(message, "Reload") + .then((action) => { + if (action === "Reload") { + vscode.commands.executeCommand("workbench.action.reloadWindow"); + } + }); + }, + { debounceMs: CONFIG_CHANGE_DEBOUNCE_MS }, + ); } /** diff --git a/src/settings/authConfig.ts b/src/settings/authConfig.ts new file mode 100644 index 0000000000..f714445392 --- /dev/null +++ b/src/settings/authConfig.ts @@ -0,0 +1,58 @@ +import * as vscode from "vscode"; + +import { + watchConfigurationChanges, + type WatchedSetting, +} from "../configWatcher"; + +/** + * Settings that affect how a request is authenticated (header command output + * and mTLS material). A change to any of these invalidates an in-flight + * session. + */ +const AUTH_CONFIG_SETTINGS = [ + "coder.headerCommand", + "coder.tlsCertFile", + "coder.tlsKeyFile", + "coder.tlsCaFile", + "coder.tlsAltHost", +] as const; + +/** {@link AUTH_CONFIG_SETTINGS} packaged for `watchConfigurationChanges`. */ +export function getAuthConfigWatchSettings(): WatchedSetting[] { + return AUTH_CONFIG_SETTINGS.map((setting) => ({ + setting, + getValue: () => vscode.workspace.getConfiguration().get(setting), + })); +} + +/** + * Monotonic counter that ticks when an auth setting changes. The request + * interceptor stamps each request with the current version; on a 401 the + * auth interceptor retries once if the counter advanced in the meantime. + */ +export class AuthConfigTracker implements vscode.Disposable { + #version = 0; + readonly #disposable: vscode.Disposable; + + public constructor() { + this.#disposable = watchConfigurationChanges( + getAuthConfigWatchSettings(), + () => { + this.#version++; + }, + ); + } + + public get version(): number { + return this.#version; + } + + public hasChangedSince(version: number | undefined): boolean { + return version !== undefined && this.#version !== version; + } + + public dispose(): void { + this.#disposable.dispose(); + } +} diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index b9a8aa9399..f49f1fdb4f 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -686,6 +686,7 @@ export class MockCoderApi implements Pick< | "setCredentials" | "getHost" | "getSessionToken" + | "hasAuthConfigChangedSince" | "getAuthenticatedUser" | "dispose" | "getExperiments" @@ -712,6 +713,9 @@ export class MockCoderApi implements Pick< readonly getHost = vi.fn(() => this._host); readonly getSessionToken = vi.fn(() => this._token); + readonly hasAuthConfigChangedSince = vi.fn( + (version: number | undefined) => version !== undefined && version !== 0, + ); readonly getAuthenticatedUser = vi.fn((): Promise => { if (this.authenticatedUser instanceof Error) { diff --git a/test/unit/api/authInterceptor.test.ts b/test/unit/api/authInterceptor.test.ts index 7192912096..cd9c0dd75e 100644 --- a/test/unit/api/authInterceptor.test.ts +++ b/test/unit/api/authInterceptor.test.ts @@ -75,6 +75,7 @@ function createMockAxiosInstance(): AxiosInstance & { function createMockCoderApi(axiosInstance: AxiosInstance): CoderApi { let sessionToken: string | undefined; let host: string | undefined = TEST_URL; + let authConfigVersion = 0; return { getAxiosInstance: () => axiosInstance, setSessionToken: vi.fn((token: string) => { @@ -85,6 +86,11 @@ function createMockCoderApi(axiosInstance: AxiosInstance): CoderApi { setHost: (newHost: string | undefined) => { host = newHost; }, + hasAuthConfigChangedSince: (version: number | undefined) => + version !== undefined && version !== authConfigVersion, + setAuthConfigVersion: (version: number) => { + authConfigVersion = version; + }, } as unknown as CoderApi; } @@ -388,6 +394,81 @@ describe("AuthInterceptor", () => { }); describe("race condition safety", () => { + it("silently retries when auth settings changed since the request started", async () => { + const { mockCoderApi, axiosInstance, createInterceptor } = + createTestContext(); + const retryResponse = { data: "success", status: 200 }; + vi.spyOn(axiosInstance, "request").mockResolvedValue(retryResponse); + const onAuthRequired = vi.fn().mockResolvedValue(false); + createInterceptor(onAuthRequired); + + const error = createAxiosError(401, "Unauthorized", { + authConfigVersion: 0, + }); + ( + mockCoderApi as unknown as { + setAuthConfigVersion: (version: number) => void; + } + ).setAuthConfigVersion(1); + + const result = await axiosInstance.triggerResponseError(error); + + expect(result).toBe(retryResponse); + expect(axiosInstance.request).toHaveBeenCalledWith( + expect.objectContaining({ + _authConfigRetryAttempted: true, + }), + ); + expect(onAuthRequired).not.toHaveBeenCalled(); + }); + + interface RetryGuardCase { + when: string; + flag: "_authConfigRetryAttempted" | "_retryAttempted"; + expectInteractive: boolean; + } + it.each([ + { + when: "auth-config retry already ran", + flag: "_authConfigRetryAttempted", + expectInteractive: true, + }, + { + when: "OAuth/interactive retry already ran", + flag: "_retryAttempted", + expectInteractive: false, + }, + ])( + "does not auth-config-retry when $when", + async ({ flag, expectInteractive }) => { + const { mockCoderApi, axiosInstance, createInterceptor } = + createTestContext(); + vi.spyOn(axiosInstance, "request"); + const onAuthRequired = vi.fn().mockResolvedValue(false); + createInterceptor(onAuthRequired); + ( + mockCoderApi as unknown as { + setAuthConfigVersion: (version: number) => void; + } + ).setAuthConfigVersion(1); + + const error = createAxiosError(401, "Unauthorized", { + authConfigVersion: 0, + [flag]: true, + }); + + await expect( + axiosInstance.triggerResponseError(error), + ).rejects.toThrow(); + expect(axiosInstance.request).not.toHaveBeenCalled(); + if (expectInteractive) { + expect(onAuthRequired).toHaveBeenCalledWith(TEST_HOSTNAME); + } else { + expect(onAuthRequired).not.toHaveBeenCalled(); + } + }, + ); + it("skips OAuth refresh if hostname changed", async () => { const { mockOAuthManager, diff --git a/test/unit/api/coderApi.test.ts b/test/unit/api/coderApi.test.ts index f7170d11ac..692a78eddc 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -23,6 +23,7 @@ import { } from "@/api/certificateRefresh"; import { CoderApi } from "@/api/coderApi"; import { createHttpAgent } from "@/api/utils"; +import { CONFIG_CHANGE_DEBOUNCE_MS } from "@/configWatcher"; import { ClientCertificateError } from "@/error/clientCertificateError"; import { ServerCertificateError } from "@/error/serverCertificateError"; import { getHeaders } from "@/headers"; @@ -214,6 +215,26 @@ describe("CoderApi", () => { ).toBe("from-header-command"); }); + it("preserves Coder-Session-Token on retry when prior command emitted it", async () => { + const api = createApi(CODER_URL, "default-token"); + + vi.mocked(getHeaders).mockResolvedValueOnce({ + "Coder-Session-Token": "from-cmd", + }); + const first = await api.getAxiosInstance().get("/api/v2/users/me"); + + // Retry: command no longer emits the token; retryRequest writes a + // fresh OAuth token onto the same config and re-issues. + vi.mocked(getHeaders).mockResolvedValueOnce({}); + (first.config.headers as AxiosHeaders).set( + "Coder-Session-Token", + "fresh", + ); + const retry = await api.getAxiosInstance().request(first.config); + + expect(retry.config.headers["Coder-Session-Token"]).toBe("fresh"); + }); + it("logs requests and responses", async () => { const api = createApi(); @@ -917,7 +938,9 @@ describe("CoderApi", () => { await tick(); mockConfig.set("coder.insecure", true); - await tick(); + await new Promise((resolve) => + setTimeout(resolve, CONFIG_CHANGE_DEBOUNCE_MS + 50), + ); // Only DISCONNECTED sockets get reconnected by config changes expect(sockets).toHaveLength(2); diff --git a/test/unit/configWatcher.test.ts b/test/unit/configWatcher.test.ts index 7d552b8d47..48b700a994 100644 --- a/test/unit/configWatcher.test.ts +++ b/test/unit/configWatcher.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; import * as vscode from "vscode"; import { @@ -135,4 +135,70 @@ describe("watchConfigurationChanges", () => { expect(changes[0].has("test.setting")).toBe(true); dispose(); }); + + describe("debounced (fixed window)", () => { + const setup = () => { + vi.useFakeTimers(); + const config = new MockConfigurationProvider(); + config.set("test.setting", "initial"); + const changes: Array> = []; + const watcher = watchConfigurationChanges( + [ + { + setting: "test.setting", + getValue: () => + vscode.workspace.getConfiguration().get("test.setting"), + }, + ], + (c) => changes.push(c), + { debounceMs: 250 }, + ); + return { config, changes, watcher }; + }; + + it("coalesces changes within the window and emits the final value", () => { + const { config, changes, watcher } = setup(); + try { + config.set("test.setting", "changed1"); + config.set("test.setting", "changed2"); + vi.advanceTimersByTime(249); + expect(changes).toEqual([]); + vi.advanceTimersByTime(1); + expect(changes.map((c) => c.get("test.setting"))).toEqual(["changed2"]); + } finally { + watcher.dispose(); + vi.useRealTimers(); + } + }); + + it("suppresses emission when the final value matches the applied value", () => { + const { config, changes, watcher } = setup(); + try { + config.set("test.setting", ""); + config.set("test.setting", "initial"); + vi.advanceTimersByTime(250); + expect(changes).toEqual([]); + } finally { + watcher.dispose(); + vi.useRealTimers(); + } + }); + + it("does not starve when events arrive faster than the window", () => { + const { config, changes, watcher } = setup(); + try { + for (let i = 0; i < 5; i++) { + config.set("test.setting", `tick-${i}`); + vi.advanceTimersByTime(200); + } + vi.advanceTimersByTime(250); + expect(changes.length).toBeGreaterThan(0); + const last = changes[changes.length - 1]; + expect(last.get("test.setting")).toBe("tick-4"); + } finally { + watcher.dispose(); + vi.useRealTimers(); + } + }); + }); }); diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 25c087ecab..00a4a18598 100644 --- a/test/unit/deployment/deploymentManager.test.ts +++ b/test/unit/deployment/deploymentManager.test.ts @@ -1,6 +1,7 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { CoderApi } from "@/api/coderApi"; +import { CONFIG_CHANGE_DEBOUNCE_MS } from "@/configWatcher"; import { MementoManager } from "@/core/mementoManager"; import { SecretsManager } from "@/core/secretsManager"; import { DeploymentManager } from "@/deployment/deploymentManager"; @@ -43,6 +44,14 @@ class MockWorkspaceProvider { const TEST_URL = "https://coder.example.com"; const TEST_HOSTNAME = "coder.example.com"; +const managers: DeploymentManager[] = []; + +afterEach(() => { + for (const manager of managers) { + manager.dispose(); + } + managers.length = 0; +}); /** * Creates a fresh test context with all dependencies. @@ -83,6 +92,7 @@ function createTestContext() { mockOAuthSessionManager as unknown as OAuthSessionManager, [mockWorkspaceProvider as unknown as WorkspaceProvider], ); + managers.push(manager); return { mockClient, @@ -456,6 +466,75 @@ describe("DeploymentManager", () => { }); describe("auth listener recovery", () => { + it("recovers from suspended state when auth settings change", async () => { + vi.useFakeTimers(); + try { + const { mockClient, validationMockClient, manager } = + createTestContext(); + const config = new MockConfigurationProvider(); + const user = createMockUser(); + validationMockClient.setAuthenticatedUserResponse(user); + + await manager.setDeployment({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token: "", + user, + }); + manager.suspendSession(); + expect(manager.isAuthenticated()).toBe(false); + + config.set("coder.tlsCertFile", "/path/to/cert.pem"); + config.set("coder.tlsKeyFile", "/path/to/key.pem"); + await vi.advanceTimersByTimeAsync(CONFIG_CHANGE_DEBOUNCE_MS); + + expect(mockClient.host).toBe(TEST_URL); + expect(mockClient.token).toBe(""); + expect(manager.isAuthenticated()).toBe(true); + expect(validationMockClient.getAuthenticatedUser).toHaveBeenCalledTimes( + 1, + ); + } finally { + vi.useRealTimers(); + } + }); + + it("does not resurrect a concurrent clearDeployment during recovery", async () => { + vi.useFakeTimers(); + try { + const { validationMockClient, manager } = createTestContext(); + const config = new MockConfigurationProvider(); + const user = createMockUser(); + + // Pause validation so a clearDeployment can race in. + let resolveAuth!: (u: typeof user) => void; + validationMockClient.getAuthenticatedUser.mockReturnValue( + new Promise((resolve) => { + resolveAuth = resolve; + }), + ); + + await manager.setDeployment({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token: "", + user, + }); + manager.suspendSession(); + config.set("coder.tlsCertFile", "/path/to/cert.pem"); + await vi.advanceTimersByTimeAsync(CONFIG_CHANGE_DEBOUNCE_MS); + + await manager.clearDeployment(); + resolveAuth(user); + await vi.runAllTimersAsync(); + + expect(manager.getCurrentDeployment()).toBeNull(); + expect(manager.isAuthenticated()).toBe(false); + } finally { + vi.useRealTimers(); + } + }); + it("recovers from suspended state when tokens update", async () => { const { mockClient, validationMockClient, secretsManager, manager } = createTestContext(); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 92d99c1a4a..280b127648 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -86,6 +86,7 @@ vi.mock("@/api/coderApi", async (importOriginal) => { }), setSessionToken: vi.fn(), getAuthenticatedUser: mockGetAuthenticatedUser, + dispose: vi.fn(), })), }, };