diff --git a/src/api/authInterceptor.ts b/src/api/authInterceptor.ts index f943aa6a8b..7f62f278c7 100644 --- a/src/api/authInterceptor.ts +++ b/src/api/authInterceptor.ts @@ -9,7 +9,6 @@ import type * as vscode from "vscode"; import type { ServiceContainer } from "../core/container"; import type { SecretsManager } from "../core/secretsManager"; import type { Logger } from "../logging/logger"; -import type { RequestConfigWithMeta } from "../logging/types"; import type { OAuthSessionManager } from "../oauth/sessionManager"; import type { CoderApi } from "./coderApi"; @@ -58,13 +57,6 @@ export class AuthInterceptor implements vscode.Disposable { throw error; } - if (error.config) { - const config = error.config as { _retryAttempted?: boolean }; - if (config._retryAttempted) { - throw error; - } - } - if (error.response?.status !== 401) { throw error; } @@ -82,6 +74,26 @@ export class AuthInterceptor implements vscode.Disposable { error: AxiosError, hostname: string, ): Promise { + const config = error.config; + + // Checked before _retryAttempted so an OAuth-retry 401 caused by a + // fresh settings change still gets one silent attempt. + if ( + config && + !config._authConfigRetryAttempted && + this.client.hasAuthConfigChangedSince(config.authConfigVersion) + ) { + config._authConfigRetryAttempted = true; + this.logger.debug( + "Authentication settings changed during request, retrying once", + ); + return this.client.getAxiosInstance().request(config); + } + + if (config?._retryAttempted) { + throw error; + } + this.logger.debug("Received 401 response, attempting recovery"); return this.authTelemetry.traceAuthRecovery(async (recorder) => { recorder.logReceived(); @@ -166,12 +178,9 @@ export class AuthInterceptor implements vscode.Disposable { throw error; } - const config = error.config as RequestConfigWithMeta & { - _retryAttempted?: boolean; - }; - config._retryAttempted = true; - config.headers[coderSessionTokenHeader] = token; - return this.client.getAxiosInstance().request(config); + error.config._retryAttempted = true; + error.config.headers[coderSessionTokenHeader] = token; + return this.client.getAxiosInstance().request(error.config); } public dispose(): void { diff --git a/src/api/axios.d.ts b/src/api/axios.d.ts new file mode 100644 index 0000000000..ac76309294 --- /dev/null +++ b/src/api/axios.d.ts @@ -0,0 +1,27 @@ +import "axios"; + +declare module "axios" { + interface InternalAxiosRequestConfig { + /** Set once the OAuth-refresh or interactive re-auth path has run. */ + _retryAttempted?: boolean; + /** + * Set once the silent auth-config-change retry has run. Separate + * from `_retryAttempted` so a follow-up 401 can still escalate to + * OAuth or interactive re-auth. + */ + _authConfigRetryAttempted?: boolean; + /** Set once a client-certificate error has been retried with refreshed certs. */ + _certRetried?: boolean; + /** + * Auth-config version snapshotted when the request was stamped. + * Compared on a 401 to detect that auth settings changed mid-flight. + */ + authConfigVersion?: number; + /** + * Headers the previous header-command run added to this request. + * Cleared at the start of the next pass so stale keys don't leak + * through when the command output changes between retries. + */ + headerCommandKeys?: string[]; + } +} diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index c113a7cc18..643152685e 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 ( @@ -495,6 +515,7 @@ function setupInterceptors( client: CoderApi, output: Logger, httpRequestsTelemetry: HttpRequestsTelemetry, + authConfigTracker: AuthConfigTracker, ): void { addRequestInterceptors( client.getAxiosInstance(), @@ -503,20 +524,43 @@ function setupInterceptors( ); client.getAxiosInstance().interceptors.request.use(async (config) => { + // Snapshot the version up front so it matches the config we're about + // to read, not whatever it bumps to during the awaits below. + config.authConfigVersion = authConfigTracker.version; + + // Drop headers from the prior header-command run so stale keys can't + // leak through if the command output changed between attempts. + for (const key of config.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. + const retrying = + config._retryAttempted === true || + config._authConfigRetryAttempted === true; for (const [key, value] of Object.entries(headers)) { + // On retry, don't let stale command output overwrite the session + // token retryRequest just wrote. + if ( + retrying && + key.toLowerCase() === coderSessionTokenHeader.toLowerCase() + ) { + continue; + } config.headers[key] = value; } + // Don't track the session token: cleanup must never touch it. + config.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; @@ -626,13 +670,10 @@ async function tryRefreshClientCertificate( } // _certRetried is per-request (Axios creates fresh config per request). - const config = err.config as RequestConfigWithMeta & { - _certRetried?: boolean; - }; - if (config._certRetried) { + if (err.config._certRetried) { throw certError; } - config._certRetried = true; + err.config._certRetried = true; output.info( `Client certificate error (alert ${certError.alertCode}), attempting refresh...`, @@ -644,12 +685,12 @@ async function tryRefreshClientCertificate( // Create new agent with refreshed certificates. const agent = await createHttpAgent(vscode.workspace.getConfiguration()); - config.httpsAgent = agent; - config.httpAgent = agent; + err.config.httpsAgent = agent; + err.config.httpAgent = agent; // Retry the request. output.info("Retrying request with refreshed certificates..."); - return axiosInstance.request(config); + return axiosInstance.request(err.config); } function wrapRequestTransform( diff --git a/src/configWatcher.ts b/src/configWatcher.ts index cc026c9087..4bd9eeef63 100644 --- a/src/configWatcher.ts +++ b/src/configWatcher.ts @@ -1,43 +1,70 @@ import { isDeepStrictEqual } from "node:util"; import * as vscode from "vscode"; +/** Idle window for config-change reactions. Coalesces rapid edits into one fire. */ +export const CONFIG_CHANGE_DEBOUNCE_MS = 200; + export interface WatchedSetting { setting: string; getValue: () => unknown; } +export interface WatchConfigurationChangesOptions { + /** + * Idle window in ms. Each new event resets the timer; the callback + * fires once settings have been quiet for this long. Unset means fire + * synchronously on every event. + */ + 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. + * Watch for configuration changes and fire when watched values change. + * With `debounceMs`, defers until settings have been quiet for that long. */ 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 idleTimer: ReturnType | undefined; + const listener = vscode.workspace.onDidChangeConfiguration((e) => { + if (!settings.some((s) => e.affectsConfiguration(s.setting))) { + return; + } + if (!options.debounceMs) { + detectAndFire(); + return; + } + clearTimeout(idleTimer); + idleTimer = setTimeout(() => { + idleTimer = undefined; + detectAndFire(); + }, options.debounceMs); }); + + return { + dispose: () => { + clearTimeout(idleTimer); + listener.dispose(); + }, + }; } function configValuesEqual(a: unknown, b: unknown): boolean { diff --git a/src/deployment/deploymentManager.ts b/src/deployment/deploymentManager.ts index 738f87b776..2ac17a6592 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; + #recoveryRunning = false; + #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,13 +94,15 @@ 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. + * Verify credentials and apply the deployment on success. Used for + * fresh logins and for un-suspending a session after auth settings or + * a token become valid again. Bails if state moved during the verify + * (logout, another login, dispose), so callers don't need a race guard. */ - public async setDeploymentIfValid( + public async verifyAndApplyDeployment( deployment: Deployment & { token?: string }, ): Promise { + const deploymentBefore = this.#deployment; const token = deployment.token ?? (await this.secretsManager.getSessionAuth(deployment.safeHostname)) @@ -99,13 +111,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 (this.#hasStateChangedSince(deploymentBefore)) { + return false; + } + await this.setDeployment({ ...deployment, token, user }); return true; } catch (e) { this.logger.warn("Failed to authenticate with deployment:", e); @@ -115,6 +124,15 @@ export class DeploymentManager implements vscode.Disposable { } } + /** True if disposal, login, or a deployment switch raced our await. */ + #hasStateChangedSince(deploymentBefore: Deployment | null): boolean { + return ( + this.#disposed || + this.isAuthenticated() || + this.#deployment !== deploymentBefore + ); + } + /** * Change to a fully authenticated deployment (with user). * Use this when you already have the user from a successful login. @@ -127,6 +145,7 @@ export class DeploymentManager implements vscode.Disposable { user: deployment.user.username, }); this.#deployment = { ...deployment }; + const ourRef = this.#deployment; this.telemetryService.setDeploymentUrl(deployment.url); // Updates client credentials @@ -147,6 +166,10 @@ export class DeploymentManager implements vscode.Disposable { const deploymentWithoutAuth: Deployment = DeploymentSchema.parse(deployment); await this.oauthSessionManager.setDeployment(deploymentWithoutAuth); + // Bail if a concurrent write took over during the await. + if (this.#deployment !== ourRef) { + return; + } await this.persistDeployment(deploymentWithoutAuth); } @@ -186,7 +209,9 @@ export class DeploymentManager implements vscode.Disposable { } public dispose(): void { + this.#disposed = true; this.#authListenerDisposable?.dispose(); + this.#authConfigDisposable?.dispose(); this.#crossWindowSyncDisposable?.dispose(); } @@ -219,7 +244,7 @@ export class DeploymentManager implements vscode.Disposable { this.logger.debug( "Token updated after session suspended, recovering", ); - await this.setDeploymentIfValid({ + await this.verifyAndApplyDeployment({ url: auth.url, safeHostname, token: auth.token, @@ -232,18 +257,59 @@ 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.#recoveryRunning) { + this.#recoveryPending = true; + return; + } + this.#recoveryRunning = true; + void 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.verifyAndApplyDeployment(snapshot); + } while (this.#recoveryPending); + } catch (err) { + this.logger.warn( + "Failed to recover session after authentication settings changed", + err, + ); + } finally { + this.#recoveryRunning = false; + } + } + private subscribeToCrossWindowChanges(): void { this.#crossWindowSyncDisposable = this.secretsManager.onDidChangeCurrentDeployment( async ({ deployment }) => { if (this.isAuthenticated()) { - // Ignore if we are already authenticated return; } if (deployment) { this.logger.info("Deployment changed from another window"); - await this.setDeploymentIfValid(deployment); + await this.verifyAndApplyDeployment(deployment); } }, ); diff --git a/src/extension.ts b/src/extension.ts index 6e4554ec38..c4a3d013f7 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -396,7 +396,7 @@ async function doActivate( if (details) { ctx.subscriptions.push(details); - const deploymentSet = await deploymentManager.setDeploymentIfValid({ + const deploymentSet = await deploymentManager.verifyAndApplyDeployment({ safeHostname: details.safeHostname, url: details.url, token: details.token, @@ -461,7 +461,7 @@ async function doActivate( output.info(`Initializing deployment: ${deployment.url}`); tracer .traceDeploymentInit(() => - deploymentManager.setDeploymentIfValid(deployment), + deploymentManager.verifyAndApplyDeployment(deployment), ) .then((success) => { if (success) { diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 67dc5693ed..e70da211a5 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -207,14 +207,17 @@ export class LoginCoordinator implements vscode.Disposable { safeHostname, async (auth) => { if (auth?.token) { - disposable?.dispose(); const client = CoderApi.create(auth.url, auth.token, this.logger); try { const user = await client.getAuthenticatedUser(); + // Stop listening only on success; a bad token shouldn't + // drop us off the bus for a follow-up valid write. + disposable?.dispose(); 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) + // Invalid token; keep listening. + } finally { + client.dispose(); } } }, @@ -240,7 +243,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..219af8a292 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,105 @@ 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(); + }); + + it("escalates to interactive after the silent auth-config retry already ran", async () => { + 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, + _authConfigRetryAttempted: true, + }); + + await expect(axiosInstance.triggerResponseError(error)).rejects.toThrow(); + expect(axiosInstance.request).not.toHaveBeenCalled(); + expect(onAuthRequired).toHaveBeenCalledWith(TEST_HOSTNAME); + }); + + it("still silent-retries when an OAuth-retry 401s under newly-changed settings", 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); + ( + mockCoderApi as unknown as { + setAuthConfigVersion: (version: number) => void; + } + ).setAuthConfigVersion(1); + + const error = createAxiosError(401, "Unauthorized", { + authConfigVersion: 0, + _retryAttempted: true, + }); + + const result = await axiosInstance.triggerResponseError(error); + expect(result).toBe(retryResponse); + expect(axiosInstance.request).toHaveBeenCalledWith( + expect.objectContaining({ _authConfigRetryAttempted: true }), + ); + expect(onAuthRequired).not.toHaveBeenCalled(); + }); + + it("gives up once both silent and OAuth/interactive retries have run", async () => { + 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, + _retryAttempted: true, + _authConfigRetryAttempted: true, + }); + + await expect(axiosInstance.triggerResponseError(error)).rejects.toThrow(); + expect(axiosInstance.request).not.toHaveBeenCalled(); + 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..77fb043463 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,72 @@ describe("watchConfigurationChanges", () => { expect(changes[0].has("test.setting")).toBe(true); dispose(); }); + + describe("debounced (idle 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("resets the idle window on each event and fires once after the burst settles", () => { + const { config, changes, watcher } = setup(); + try { + // Events spaced inside the window keep resetting the timer. + for (let i = 0; i < 5; i++) { + config.set("test.setting", `tick-${i}`); + vi.advanceTimersByTime(200); + } + expect(changes).toEqual([]); + + // Once quiet, the callback fires exactly once with the last value. + vi.advanceTimersByTime(250); + expect(changes.map((c) => c.get("test.setting"))).toEqual(["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..2e90287e13 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. @@ -52,7 +61,7 @@ function createTestContext() { new MockConfigurationProvider(); const mockClient = new MockCoderApi(); - // For setDeploymentIfValid, we use a separate mock for validation + // For verifyAndApplyDeployment, we use a separate mock for validation const validationMockClient = new MockCoderApi(); const mockWorkspaceProvider = new MockWorkspaceProvider(); const mockOAuthSessionManager = new MockOAuthSessionManager(); @@ -83,6 +92,7 @@ function createTestContext() { mockOAuthSessionManager as unknown as OAuthSessionManager, [mockWorkspaceProvider as unknown as WorkspaceProvider], ); + managers.push(manager); return { mockClient, @@ -194,13 +204,13 @@ describe("DeploymentManager", () => { }); }); - describe("setDeploymentIfValid", () => { + describe("verifyAndApplyDeployment", () => { it("returns true and sets deployment on auth success", async () => { const { mockClient, validationMockClient, manager } = createTestContext(); const user = createMockUser(); validationMockClient.setAuthenticatedUserResponse(user); - const result = await manager.setDeploymentIfValid({ + const result = await manager.verifyAndApplyDeployment({ url: TEST_URL, safeHostname: TEST_HOSTNAME, token: "test-token", @@ -218,7 +228,7 @@ describe("DeploymentManager", () => { new Error("Auth failed"), ); - const result = await manager.setDeploymentIfValid({ + const result = await manager.verifyAndApplyDeployment({ url: TEST_URL, safeHostname: TEST_HOSTNAME, token: "test-token", @@ -234,7 +244,7 @@ describe("DeploymentManager", () => { const user = createMockUser(); validationMockClient.setAuthenticatedUserResponse(user); - const result = await manager.setDeploymentIfValid({ + const result = await manager.verifyAndApplyDeployment({ url: TEST_URL, safeHostname: TEST_HOSTNAME, token: "", @@ -258,7 +268,7 @@ describe("DeploymentManager", () => { token: "stored-token", }); - const result = await manager.setDeploymentIfValid({ + const result = await manager.verifyAndApplyDeployment({ url: TEST_URL, safeHostname: TEST_HOSTNAME, }); @@ -272,7 +282,7 @@ describe("DeploymentManager", () => { const user = createMockUser(); validationMockClient.setAuthenticatedUserResponse(user); - await manager.setDeploymentIfValid({ + await manager.verifyAndApplyDeployment({ url: TEST_URL, safeHostname: TEST_HOSTNAME, token: "test-token", @@ -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(), })), }, };