diff --git a/CHANGELOG.md b/CHANGELOG.md index acac656635..8dd9c7b30b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,9 +20,23 @@ ### Fixed -- OAuth sign-in now works on deployments that enforce token scopes. Users signed - in with OAuth are asked to sign in again once. -- Revoke OAuth tokens on logout even when their scopes are outdated. +- OAuth sign-in now works on deployments that enforce token scopes. Sessions + missing required scopes stop refreshing and require sign-in when they expire. +- Revoke OAuth tokens on logout and after successful session replacement, even + when their scopes are outdated. Failed or canceled sign-ins do not revoke + the stored session. +- Preserve OAuth refresh credentials when reusing a stored token, and request + the permissions needed to refresh workspace external-auth links. +- If a deployment disables dynamic client registration (the default from Coder + 2.38), OAuth sign-in now names the setting and offers session-token sign-in. + OAuth becomes the default sign-in method in a future release, so ask your + Coder administrator to enable dynamic client registration. For instructions, + see [OAuth2 provider](https://coder.com/docs/admin/integrations/oauth2-provider). +- Keep session tokens, OAuth tokens, and client secrets from failed requests out + of the buffered logs that the Coder output channel replays. +- CLI commands and SSH connections that the extension starts ignore + `CODER_SESSION_TOKEN` from the environment, so they no longer fail or run as a + different user inside a Coder workspace. ## [v1.16.4](https://github.com/coder/vscode-coder/releases/tag/v1.16.4) 2026-09-23 diff --git a/src/api/authInterceptor.ts b/src/api/authInterceptor.ts index 7ea7bc5cab..3925c7df70 100644 --- a/src/api/authInterceptor.ts +++ b/src/api/authInterceptor.ts @@ -102,7 +102,7 @@ export class AuthInterceptor implements vscode.Disposable { // 1) OAuth refresh path. const isOAuth = - await this.oauthSessionManager.isLoggedInWithOAuth(hostname); + await this.oauthSessionManager.canRefreshOAuthSession(hostname); recorder.setRefreshAttempted(isOAuth); if (isOAuth) { const newToken = await this.tryOAuthRefresh(); diff --git a/src/cli/cliCredentialManager.ts b/src/cli/cliCredentialManager.ts index 16bc140026..989ea2692a 100644 --- a/src/cli/cliCredentialManager.ts +++ b/src/cli/cliCredentialManager.ts @@ -14,6 +14,7 @@ import { import { recordError } from "../instrumentation/outcomes"; import { type CliAuth, + EMPTY_TOKEN_FLAG, getGlobalFlags, mayUseCliStore, resolveCliAuth, @@ -75,10 +76,16 @@ export class CliCredentialManager { return; } span.setProperty("store", cli.auth.store); - await this.exec(cli, ["login", "--use-token-as-session", url], { - env: { ...process.env, CODER_SESSION_TOKEN: token }, - signal: options?.signal, - }); + // `login` reads the token from the environment, so drop the empty flag. + const flags = cli.flags.filter((flag) => flag !== EMPTY_TOKEN_FLAG); + await this.exec( + { ...cli, flags }, + ["login", "--use-token-as-session", url], + { + env: envWithSessionToken(token), + signal: options?.signal, + }, + ); span.setProperty("outcome", "stored"); this.logger.info("Stored token via CLI for", url); }); @@ -268,3 +275,13 @@ export class CliCredentialManager { return results.every(Boolean); } } + +/** `process.env` with `token` as its only `CODER_SESSION_TOKEN`, in any case. */ +function envWithSessionToken(token: string): NodeJS.ProcessEnv { + const env = Object.fromEntries( + Object.entries(process.env).filter( + ([key]) => key.toUpperCase() !== "CODER_SESSION_TOKEN", + ), + ); + return { ...env, CODER_SESSION_TOKEN: token }; +} diff --git a/src/logging/formatters.ts b/src/logging/formatters.ts index d3389b7358..4181488276 100644 --- a/src/logging/formatters.ts +++ b/src/logging/formatters.ts @@ -1,11 +1,10 @@ +import { type AxiosError, type AxiosRequestConfig, isAxiosError } from "axios"; import { Buffer } from "node:buffer"; import util from "node:util"; import prettyBytes from "pretty-bytes"; import { lowercase } from "../common/strings"; -import type { AxiosRequestConfig } from "axios"; - const SENSITIVE_HEADERS: ReadonlySet> = new Set([ "authorization", "coder-session-token", @@ -200,9 +199,22 @@ export function sizeOf(data: unknown): number | undefined { return undefined; } +/** Drops the error's config and socket, which hold the request's credentials. */ +function summarizeAxiosError(error: AxiosError) { + return { + name: error.name, + message: error.message, + code: error.code, + method: formatMethod(error.config?.method), + url: formatUri(error.config), + status: error.response?.status, + data: redactBodyFields(error.response?.data), + }; +} + export function safeStringify(data: unknown): string | null { try { - return util.inspect(data, { + return util.inspect(isAxiosError(data) ? summarizeAxiosError(data) : data, { showHidden: false, // Bounded so a single log line cannot balloon in size. depth: 8, diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 98263562cf..877b8a95ba 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -7,7 +7,11 @@ import { needToken } from "../api/httpAgent"; import { HttpStatusCode } from "../api/httpStatusCode"; import { isSameOrigin, openInBrowser } from "../common/url"; import { CertificateError } from "../error/certificateError"; -import { OAuthAuthorizer } from "../oauth/authorizer"; +import { + ClientRegistrationDisabledError, + OAuthAuthorizer, +} from "../oauth/authorizer"; +import { revokeOAuthTokens } from "../oauth/revocation"; import { buildOAuthTokenData } from "../oauth/tokens"; import { withCancellableProgress } from "../ui/progress"; import { maybeAskAuthMethod, maybeAskUrl } from "../ui/prompts"; @@ -15,7 +19,10 @@ import { vscodeProposed } from "../vscode/proposed"; import { showStoreCredentialsError } from "./credentials"; -import type { User } from "coder/site/src/api/typesGenerated"; +import type { + User, + OAuth2ClientRegistrationResponse, +} from "coder/site/src/api/typesGenerated"; import type { CliCredentialManager } from "../cli/cliCredentialManager"; import type { Deployment } from "../deployment/types"; @@ -109,18 +116,14 @@ export class LoginCoordinator implements vscode.Disposable { options: LoginOptions & { url: string }, ): Promise { const { safeHostname, url } = options; - return this.executeWithGuard(async () => { - const result = await this.attemptLogin( + return this.executeWithGuard(() => + this.attemptLogin( { safeHostname, url }, options.autoLogin ?? false, options.token, options.tokenSignInConfirmed ?? false, - ); - - await this.persistSessionAuth(result, safeHostname, url); - - return result; - }); + ), + ); } /** @@ -170,15 +173,11 @@ export class LoginCoordinator implements vscode.Disposable { return { success: false, reason: "no_url_provided" }; } - const result = await this.attemptLogin( + return this.attemptLogin( { url: newUrl, safeHostname }, false, options.token, ); - - await this.persistSessionAuth(result, safeHostname, newUrl); - - return result; } // User cancelled return { success: false, reason: "user_dismissed" }; @@ -198,28 +197,42 @@ export class LoginCoordinator implements vscode.Disposable { } private async persistSessionAuth( - result: LoginResult, - safeHostname: string, - url: string, + result: Extract, + deployment: Deployment, + registration: OAuth2ClientRegistrationResponse | undefined, ): Promise { + const { safeHostname, url } = deployment; + const previous = await this.secretsManager.getSessionAuth(safeHostname); + const replaced = previous?.token === result.token ? undefined : previous; + // A refresh replaced the stored session during this login; keep it. + if (result.method === "stored_token" && replaced) { + return; + } + const reusedOAuth = + !replaced && previous && isSameOrigin(previous.url, url) + ? previous.oauth + : undefined; + // Empty token is valid for mTLS - if (result.success) { - await this.secretsManager.setSessionAuth(safeHostname, { - url, - token: result.token, - username: result.user.username, - oauth: result.oauth, // undefined for non-OAuth logins - }); - await this.mementoManager.addToUrlHistory(url); - - if (result.token) { - const configs = vscode.workspace.getConfiguration(); - this.cliCredentialManager - .storeToken(url, result.token, configs) - .catch((error) => - showStoreCredentialsError(error, configs, this.logger), - ); - } + await this.secretsManager.setSessionAuth(safeHostname, { + url, + token: result.token, + username: result.user.username, + oauth: result.oauth ?? reusedOAuth, + }); + await this.mementoManager.addToUrlHistory(url); + + if (replaced?.oauth) { + void revokeOAuthTokens(replaced, registration, this.logger); + } + + if (result.token) { + const configs = vscode.workspace.getConfiguration(); + this.cliCredentialManager + .storeToken(url, result.token, configs) + .catch((error) => + showStoreCredentialsError(error, configs, this.logger), + ); } } @@ -286,15 +299,24 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, tokenSignInConfirmed = false, ): Promise { + // Revokes sessions that don't record their client. Read it before login can + // re-register the client. + const registration = await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + ); const client = CoderApi.create(deployment.url, "", this.logger); try { - return await this.runLoginAttempts( + const result = await this.runLoginAttempts( client, deployment, isAutoLogin, providedToken, tokenSignInConfirmed, ); + if (result.success) { + await this.persistSessionAuth(result, deployment, registration); + } + return result; } finally { client.dispose(); } @@ -477,11 +499,16 @@ export class LoginCoordinator implements vscode.Disposable { private async askAuthMethod(ctx: LoginAttemptContext): Promise { const authMethod = await maybeAskAuthMethod(ctx.client); switch (authMethod) { - case "oauth": + case "oauth": { + const result = await this.loginWithOAuth(ctx.deployment); + if (result !== "use_token") { + return withLoginMethod("oauth", result); + } return withLoginMethod( - "oauth", - await this.loginWithOAuth(ctx.deployment), + "cli_token", + await this.loginWithToken(ctx.client), ); + } case "legacy": return withLoginMethod( "cli_token", @@ -636,36 +663,50 @@ export class LoginCoordinator implements vscode.Disposable { return { success: false, reason: "user_dismissed" }; } - /** - * OAuth authentication flow. - */ + /** OAuth flow; "use_token" means the user chose session-token sign-in instead. */ private async loginWithOAuth( deployment: Deployment, - ): Promise { + ): Promise { try { this.logger.debug("Starting OAuth authentication"); - const { tokenResponse, user } = await vscode.window.withProgress( - { - location: vscode.ProgressLocation.Notification, - title: "Authenticating", - cancellable: true, - }, - async (progress, cancellationToken) => - await this.oauthAuthorizer.login( - deployment, - progress, - cancellationToken, - ), - ); + const { tokenResponse, user, registration } = + await vscode.window.withProgress( + { + location: vscode.ProgressLocation.Notification, + title: "Authenticating", + cancellable: true, + }, + async (progress, cancellationToken) => + await this.oauthAuthorizer.login( + deployment, + progress, + cancellationToken, + ), + ); return { success: true, token: tokenResponse.access_token, user, - oauth: buildOAuthTokenData(tokenResponse), + oauth: buildOAuthTokenData(tokenResponse, registration), }; } catch (error) { + if (error instanceof ClientRegistrationDisabledError) { + this.logger.warn("OAuth sign-in unavailable:", error.message); + const action = await vscodeProposed.window.showWarningMessage( + "OAuth sign-in is unavailable", + { + useCustom: true, + modal: true, + detail: `${error.message}. To use OAuth, ask an administrator to set dynamic_client_registration_enabled.`, + }, + "Sign In with Token", + ); + return action === "Sign In with Token" + ? "use_token" + : { success: false, reason: "user_dismissed" }; + } const title = "OAuth authentication failed"; this.logger.error(title, error); if (error instanceof CertificateError) { diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index c3fcb7db75..ec97220b70 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -1,7 +1,9 @@ +import { type AxiosInstance, isAxiosError } from "axios"; import { createHash, randomBytes } from "node:crypto"; import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; +import { HttpStatusCode } from "../api/httpStatusCode"; import { resolveCoderDashboardUrl } from "../common/url"; import { @@ -21,7 +23,6 @@ import { parseOAuthResponse, } from "./validation"; -import type { AxiosInstance } from "axios"; import type { OAuth2AuthorizationServerMetadata, OAuth2ClientRegistrationRequest, @@ -37,6 +38,21 @@ import type { SecretsManager } from "../storage/secretsManager"; import type { OAuthCallback } from "./oauthCallback"; +interface OAuthLoginResult { + tokenResponse: OAuth2TokenResponse; + user: User; + /** Client the tokens were issued to. */ + registration: OAuth2ClientRegistrationResponse; +} + +/** The server refuses client registration, which is off by default from Coder 2.38. */ +export class ClientRegistrationDisabledError extends Error { + constructor() { + super("Dynamic client registration is disabled on this deployment"); + this.name = "ClientRegistrationDisabledError"; + } +} + /** * Handles the OAuth authorization code flow for authenticating with Coder deployments. * Encapsulates client registration, PKCE challenge, and token exchange. @@ -51,16 +67,12 @@ export class OAuthAuthorizer implements vscode.Disposable { private readonly extensionId: string, ) {} - /** - * Perform complete OAuth login flow. - * Creates CoderApi internally from deployment. - * Returns the token response and user - does not persist tokens. - */ + /** Runs the OAuth login flow. Saves the client registration, not the tokens. */ public async login( deployment: Deployment, progress: vscode.Progress<{ message?: string; increment?: number }>, cancellationToken: vscode.CancellationToken, - ): Promise<{ tokenResponse: OAuth2TokenResponse; user: User }> { + ): Promise { const client = CoderApi.create(deployment.url, undefined, this.logger); try { return await this.runLoginFlow( @@ -79,7 +91,7 @@ export class OAuthAuthorizer implements vscode.Disposable { deployment: Deployment, progress: vscode.Progress<{ message?: string; increment?: number }>, cancellationToken: vscode.CancellationToken, - ): Promise<{ tokenResponse: OAuth2TokenResponse; user: User }> { + ): Promise { const reportProgress = (message?: string, increment?: number): void => { if (cancellationToken.isCancellationRequested) { throw new Error("OAuth login cancelled by user"); @@ -124,7 +136,7 @@ export class OAuthAuthorizer implements vscode.Disposable { this.logger.debug("OAuth login flow completed successfully"); - return { tokenResponse, user }; + return { tokenResponse, user, registration }; } /** @@ -171,10 +183,20 @@ export class OAuthAuthorizer implements vscode.Disposable { token_endpoint_auth_method: TOKEN_ENDPOINT_AUTH_METHOD, }; - const response = await axiosInstance.post( - metadata.registration_endpoint, - registrationRequest, - ); + const response = await axiosInstance + .post( + metadata.registration_endpoint, + registrationRequest, + ) + .catch((error: unknown) => { + if ( + isAxiosError(error) && + error.response?.status === HttpStatusCode.FORBIDDEN + ) { + throw new ClientRegistrationDisabledError(); + } + throw error; + }); const registrationResponse = parseOAuthResponse( OAuth2ClientRegistrationResponseSchema, diff --git a/src/oauth/constants.ts b/src/oauth/constants.ts index 32c314fa1a..7d48893f78 100644 --- a/src/oauth/constants.ts +++ b/src/oauth/constants.ts @@ -3,8 +3,8 @@ export const AUTH_GRANT_TYPE = "authorization_code"; export const REFRESH_GRANT_TYPE = "refresh_token"; /** - * Scopes the extension and the CLI need. Stored sessions must have all of - * them, so adding one signs out users of servers that enforce scopes. + * Scopes the extension and the CLI need. Sessions missing one stop refreshing, + * so on servers that enforce scopes, adding one means signing in at expiry. */ export const DEFAULT_OAUTH_SCOPES = [ "coder:workspaces.operate", @@ -12,6 +12,9 @@ export const DEFAULT_OAUTH_SCOPES = [ "workspace:create", "user:read", "user:read_personal", + // Lets `coder start` refresh expired external-auth links. No narrower scope + // exists, so it also allows profile and Git SSH key changes. + "user:update_personal", ].join(" "); /** diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index b6b5b4e24f..af33bc23db 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -1,3 +1,4 @@ +import { CoderApi } from "../api/coderApi"; import { parseApiResponse } from "../api/responseValidation"; import { @@ -142,6 +143,29 @@ export class OAuthMetadataClient { } } +/** Runs `fn` with the server's OAuth metadata and a client disposed afterward. */ +export async function withOAuthMetadata( + url: string, + token: string | undefined, + logger: Logger, + fn: (ctx: { + axiosInstance: AxiosInstance; + metadata: OAuth2AuthorizationServerMetadata; + }) => Promise, +): Promise { + const client = CoderApi.create(url, token, logger); + try { + const axiosInstance = client.getAxiosInstance(); + const metadata = await new OAuthMetadataClient( + axiosInstance, + logger, + ).getMetadata(); + return await fn({ axiosInstance, metadata }); + } finally { + client.dispose(); + } +} + /** * Check if an array includes all required types. */ diff --git a/src/oauth/revocation.ts b/src/oauth/revocation.ts new file mode 100644 index 0000000000..ffa9c36acf --- /dev/null +++ b/src/oauth/revocation.ts @@ -0,0 +1,67 @@ +import { withOAuthMetadata } from "./metadataClient"; +import { type OAuthClient, toUrlSearchParams } from "./tokens"; + +import type { OAuth2TokenRevocationRequest } from "coder/site/src/api/typesGenerated"; + +import type { Logger } from "../logging/logger"; +import type { SessionAuth } from "../storage/secretsManager"; + +/** + * Best-effort revocation of the token pair in `auth`; never throws. Signs in as + * the client that issued the tokens, else `fallbackClient`. + */ +export async function revokeOAuthTokens( + auth: SessionAuth, + fallbackClient: OAuthClient | undefined, + logger: Logger, +): Promise { + if (!auth.oauth) { + return; + } + const client = auth.oauth.client ?? fallbackClient; + if (!client) { + logger.warn("Skipped token revocation: no client registration"); + return; + } + // Refresh token first, while the access token still authenticates the call. + const targets = [ + [auth.oauth.refresh_token, "refresh_token"], + [auth.token, "access_token"], + ] as const; + + try { + await withOAuthMetadata( + auth.url, + auth.token, + logger, + async ({ axiosInstance, metadata }) => { + const endpoint = metadata.revocation_endpoint; + if (!endpoint) { + logger.debug("No revocation endpoint; skipping revocation"); + return; + } + for (const [token, token_type_hint] of targets) { + if (!token) { + continue; + } + const params: OAuth2TokenRevocationRequest = { + token, + client_id: client.client_id, + client_secret: client.client_secret, + token_type_hint, + }; + try { + await axiosInstance.post(endpoint, toUrlSearchParams(params), { + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + }); + logger.debug(`Revoked ${token_type_hint}`); + } catch (error) { + logger.warn(`Failed to revoke ${token_type_hint}:`, error); + } + } + }, + ); + } catch (error) { + logger.warn("Token revocation failed:", error); + } +} diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 843d4b3658..de3bbfbfb8 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -6,17 +6,14 @@ import { import { DEFAULT_OAUTH_SCOPES, REFRESH_GRANT_TYPE } from "./constants"; import { OAuthError, parseOAuthError } from "./errors"; -import { OAuthMetadataClient } from "./metadataClient"; +import { withOAuthMetadata } from "./metadataClient"; +import { revokeOAuthTokens } from "./revocation"; import { buildOAuthTokenData, toUrlSearchParams } from "./tokens"; import { OAuth2TokenResponseSchema, parseOAuthResponse } from "./validation"; -import type { AxiosInstance } from "axios"; import type { - OAuth2AuthorizationServerMetadata, - OAuth2ClientRegistrationResponse, OAuth2TokenRequest, OAuth2TokenResponse, - OAuth2TokenRevocationRequest, } from "coder/site/src/api/typesGenerated"; import type * as vscode from "vscode"; @@ -294,42 +291,6 @@ export class OAuthSessionManager implements vscode.Disposable { return true; } - /** - * 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 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); - try { - const axiosInstance = client.getAxiosInstance(); - 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"); - } - - return await fn({ axiosInstance, metadata, registration }); - } finally { - client.dispose(); - } - } - public async setDeployment(deployment: Deployment): Promise { if ( deployment.safeHostname === this.deployment?.safeHostname && @@ -400,18 +361,27 @@ export class OAuthSessionManager implements vscode.Disposable { } const refreshToken = storedTokens.refresh_token; - const accessToken = storedTokens.access_token; + const client = + storedTokens.client ?? + (await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + )); + if (!client) { + throw new Error("No client registration found"); + } - return await this.withOAuthOperation( - accessToken, - async ({ axiosInstance, metadata, registration }) => { + return await withOAuthMetadata( + deployment.url, + storedTokens.access_token, + this.logger, + async ({ axiosInstance, metadata }) => { 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, + client_id: client.client_id, + client_secret: client.client_secret, }; const response = await axiosInstance.post( @@ -444,7 +414,7 @@ export class OAuthSessionManager implements vscode.Disposable { deployment, tokenResponse.access_token, ), - oauth: buildOAuthTokenData(tokenResponse), + oauth: buildOAuthTokenData(tokenResponse, client), }); return tokenResponse; @@ -484,64 +454,31 @@ export class OAuthSessionManager implements vscode.Disposable { /** Best-effort server-side revocation of the stored refresh and access tokens; never throws. */ public async revokeTokens(): Promise { - // Includes tokens lacking a required scope, which still work on the server. - const storedTokens = await this.getStoredTokens().catch((error) => { - this.logger.warn("Failed to read stored tokens for revocation:", error); - return undefined; - }); - if (!storedTokens) { + const deployment = this.deployment; + if (!deployment) { return; } - - // Refresh token first, while the access token still authenticates the call. - const targets: Array<[string, "access_token" | "refresh_token"]> = []; - if (storedTokens.refresh_token) { - targets.push([storedTokens.refresh_token, "refresh_token"]); - } - targets.push([storedTokens.access_token, "access_token"]); - - try { - await this.withOAuthOperation( - storedTokens.access_token, - async ({ axiosInstance, metadata, registration }) => { - const endpoint = metadata.revocation_endpoint; - if (!endpoint) { - this.logger.debug("No revocation endpoint; skipping revocation"); - return; - } - for (const [token, token_type_hint] of targets) { - const params: OAuth2TokenRevocationRequest = { - token, - client_id: registration.client_id, - client_secret: registration.client_secret, - token_type_hint, - }; - try { - await axiosInstance.post(endpoint, toUrlSearchParams(params), { - headers: { - "Content-Type": "application/x-www-form-urlencoded", - }, - }); - this.logger.debug(`Revoked ${token_type_hint}`); - } catch (error) { - this.logger.warn(`Failed to revoke ${token_type_hint}:`, error); - } - } - }, - ); - } catch (error) { - this.logger.warn("Token revocation failed:", error); + // Includes tokens lacking a required scope, which still work on the server. + const auth = await this.secretsManager.getSessionAuth( + deployment.safeHostname, + ); + if (auth?.url !== deployment.url) { + return; } + const registration = await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + ); + await revokeOAuthTokens(auth, registration, this.logger); } /** - * Returns true if OAuth tokens exist for the current deployment. - * Always reads fresh from secrets to ensure cross-window synchronization. + * Whether the deployment's OAuth tokens carry every required scope, so they + * may refresh. Reads secrets each time to stay in sync across windows. * * @param hostname Optional hostname to validate against current deployment. * If provided and doesn't match, returns false (race-safety). */ - public async isLoggedInWithOAuth(hostname?: string): Promise { + public async canRefreshOAuthSession(hostname?: string): Promise { if (hostname && hostname !== this.deployment?.safeHostname) { return false; } diff --git a/src/oauth/tokens.ts b/src/oauth/tokens.ts index 1f0ade9165..f635125658 100644 --- a/src/oauth/tokens.ts +++ b/src/oauth/tokens.ts @@ -1,4 +1,7 @@ -import type { OAuth2TokenResponse } from "coder/site/src/api/typesGenerated"; +import type { + OAuth2ClientRegistrationResponse, + OAuth2TokenResponse, +} from "coder/site/src/api/typesGenerated"; import type { OAuthTokenData } from "../storage/secretsManager"; @@ -22,12 +25,19 @@ export function toUrlSearchParams(obj: object): URLSearchParams { return new URLSearchParams(params); } +/** Client credentials that refresh and revocation authenticate with. */ +export type OAuthClient = Pick< + OAuth2ClientRegistrationResponse, + "client_id" | "client_secret" +>; + /** - * Build OAuthTokenData from a token response. + * Build OAuthTokenData from a token response issued to `client`. * Prefers the `expiry` timestamp over calculating from `expires_in`. */ export function buildOAuthTokenData( tokenResponse: OAuth2TokenResponse, + client: OAuthClient, ): OAuthTokenData { if (tokenResponse.token_type !== "Bearer") { throw new Error( @@ -40,6 +50,10 @@ export function buildOAuthTokenData( // Servers that ignore scopes return none and grant everything scope: tokenResponse.scope || "coder:all", expiry_timestamp: getExpiryTimestamp(tokenResponse), + client: { + client_id: client.client_id, + client_secret: client.client_secret, + }, }; } diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 3d42bd81db..94a5aa2e5d 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -53,6 +53,9 @@ export function getGlobalFlags( const identity = (s: string) => s; +/** Hides `CODER_SESSION_TOKEN` (in any case) so the CLI uses its stored session. */ +export const EMPTY_TOKEN_FLAG = "--token="; + function buildGlobalFlags( configs: Pick, auth: CliAuth, @@ -60,10 +63,12 @@ function buildGlobalFlags( escHeader: (s: string) => string, ): string[] { // Escape after stripping so expansion whitespace stays in one shell token. - const flags = stripManagedFlags( + const userFlags = stripManagedFlags( getExpandedUserGlobalFlags(configs), auth.store === "extension", ).map(escAuth); + // A user's own `--token` comes later, so it wins. + const flags = [EMPTY_TOKEN_FLAG, ...userFlags]; if (auth.store === "extension") { flags.push("--global-config", escAuth(auth.configDir)); } diff --git a/src/storage/secretsManager.ts b/src/storage/secretsManager.ts index 783005e6fb..d141f6a8ce 100644 --- a/src/storage/secretsManager.ts +++ b/src/storage/secretsManager.ts @@ -38,6 +38,10 @@ const OAuthTokenDataSchema = z.object({ refresh_token: z.string().optional(), scope: z.string(), expiry_timestamp: z.number(), + /** Client that issued the tokens; if absent, the stored registration is used. */ + client: z + .object({ client_id: z.string(), client_secret: z.string().optional() }) + .optional(), }); export type OAuthTokenData = z.infer; diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 3e6cef9be5..66dff94346 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -1066,7 +1066,7 @@ export class MockOAuthSessionManager { .fn() .mockResolvedValue({ access_token: "test-token" }); readonly revokeTokens = vi.fn().mockResolvedValue(undefined); - readonly isLoggedInWithOAuth = vi.fn().mockResolvedValue(false); + readonly canRefreshOAuthSession = vi.fn().mockResolvedValue(false); readonly clearOAuthState = vi.fn().mockResolvedValue(undefined); readonly dispose = vi.fn(); } diff --git a/test/unit/api/authInterceptor.test.ts b/test/unit/api/authInterceptor.test.ts index 94888450f9..4259e6561a 100644 --- a/test/unit/api/authInterceptor.test.ts +++ b/test/unit/api/authInterceptor.test.ts @@ -116,7 +116,7 @@ function createTestContext() { const mockOAuthManager = new MockOAuthSessionManager(); // Default: not logged in with OAuth - mockOAuthManager.isLoggedInWithOAuth.mockResolvedValue(false); + mockOAuthManager.canRefreshOAuthSession.mockResolvedValue(false); /** Sets up OAuth tokens in storage and configures mock */ const setupOAuthTokens = async () => { @@ -129,7 +129,7 @@ function createTestContext() { scope: "workspace:read", }, }); - mockOAuthManager.isLoggedInWithOAuth.mockImplementation( + mockOAuthManager.canRefreshOAuthSession.mockImplementation( async (hostname?: string) => { if (hostname && hostname !== TEST_HOSTNAME) { return false; @@ -556,8 +556,8 @@ describe("AuthInterceptor", () => { await setupOAuthTokens(); - // Make isLoggedInWithOAuth return false for different hostname - mockOAuthManager.isLoggedInWithOAuth.mockImplementation( + // Make canRefreshOAuthSession return false for different hostname + mockOAuthManager.canRefreshOAuthSession.mockImplementation( (hostname?: string) => { // Simulate hostname mismatch (deployment changed) if (hostname === TEST_HOSTNAME) { diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index ed1b857a16..f6b2f5e873 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace"; +import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { workspace as createWorkspace } from "@repo/mocks"; @@ -21,6 +22,8 @@ vi.mock(import("node:child_process"), async (importOriginal) => ({ })); const { spawn } = await import("node:child_process"); +const GLOBAL_FLAGS = [EMPTY_TOKEN_FLAG, "--url", "https://test.coder.com"]; + const CLI_FEATURES: CliFeatureSet = { cliLogin: true, proxyLogDirectory: true, @@ -224,8 +227,7 @@ describe("updateWorkspace", () => { await expect(result).resolves.toBe(finalWorkspace); expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "update", "testuser/test-workspace", ]); @@ -245,8 +247,7 @@ describe("updateWorkspace", () => { await result; expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "update", "--parameter", "region=us-east", @@ -421,8 +422,7 @@ describe("startWorkspace", () => { await expect(result).resolves.toBe(finalWorkspace); expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "start", "--yes", "--reason", @@ -451,8 +451,7 @@ describe("startWorkspace", () => { await result; expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "start", "--yes", "testuser/test-workspace", diff --git a/test/unit/cli/cliCredentialManager.test.ts b/test/unit/cli/cliCredentialManager.test.ts index 10b6ba4f03..1e9367e86b 100644 --- a/test/unit/cli/cliCredentialManager.test.ts +++ b/test/unit/cli/cliCredentialManager.test.ts @@ -9,6 +9,7 @@ import { type BinaryResolver, } from "@/cli/cliCredentialManager"; import * as cliExec from "@/cli/cliExec"; +import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { PathResolver } from "@/storage/pathResolver"; import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; @@ -41,6 +42,7 @@ const PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); const CRED_DIR = path.join("/mock/base", "dev.coder.com"); const USER_DIR = "/custom/coderv2"; +// Flags for `login --use-token-as-session`; every other command gets EMPTY_TOKEN_FLAG first. const EXTENSION_FLAGS = [ "--global-config", CRED_DIR, @@ -204,12 +206,15 @@ describe("CliCredentialManager", () => { describe("storeToken", () => { it("passes the token through the environment only", async () => { stubExecFile(); + vi.stubEnv("coder_session_token", "host-token"); const { manager } = setup(); await manager.storeToken(TEST_URL, "my-secret-token", configs); expect(execOptions().env?.CODER_SESSION_TOKEN).toBe("my-secret-token"); + expect(execOptions().env).not.toHaveProperty("coder_session_token"); expect(execCalls()[0]).not.toContain("my-secret-token"); + expect(execCalls()[0]).not.toContain(EMPTY_TOKEN_FLAG); }); interface CliErrorCase { @@ -254,7 +259,9 @@ describe("CliCredentialManager", () => { const { manager } = setup(); expect(await manager.readToken(TEST_URL, configs)).toBe("my-token"); - expect(execCalls()).toEqual([[...KEYRING_FLAGS, "login", "token"]]); + expect(execCalls()).toEqual([ + [EMPTY_TOKEN_FLAG, ...KEYRING_FLAGS, "login", "token"], + ]); }); interface ReadTokenCase { @@ -408,7 +415,7 @@ describe("CliCredentialManager", () => { expect(result).toBe(true); expect(execCalls()).toEqual( - logout ? [[...logout, "logout", "--yes"]] : [], + logout ? [[EMPTY_TOKEN_FLAG, ...logout, "logout", "--yes"]] : [], ); expect(credentialFilesExist()).toBe(false); expect(sink.expectOne("auth.credential.clear").properties).toMatchObject({ diff --git a/test/unit/cli/cliExec.test.ts b/test/unit/cli/cliExec.test.ts index 83d9b3a603..a09cb5c23e 100644 --- a/test/unit/cli/cliExec.test.ts +++ b/test/unit/cli/cliExec.test.ts @@ -13,6 +13,8 @@ import { } from "vitest"; import * as vscode from "vscode"; +import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; + import { MockConfigurationProvider } from "../../mocks/testHelpers"; import { isWindows, @@ -195,6 +197,7 @@ describe("cliExec", () => { .trim() .split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -239,6 +242,7 @@ describe("cliExec", () => { configs.set("coder.headerCommand", "my-header-cmd"); const args = (await cliExec.netcheck(env)).trim().split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -295,6 +299,7 @@ describe("cliExec", () => { }); const args = (await fs.readFile(outputPath, "utf-8")).trim().split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -325,6 +330,7 @@ describe("cliExec", () => { await cliExec.supportBundle(env, "owner/workspace", { outputPath }); const args = (await fs.readFile(outputPath, "utf-8")).trim().split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "support", @@ -392,7 +398,13 @@ describe("cliExec", () => { expect(spawn).toHaveBeenCalledWith( env.binary, - ["--url", "https://test.coder.com", "ping", "owner/my workspace"], + [ + EMPTY_TOKEN_FLAG, + "--url", + "https://test.coder.com", + "ping", + "owner/my workspace", + ], expect.objectContaining({ detached: process.platform !== "win32" }), ); }); @@ -408,6 +420,7 @@ describe("cliExec", () => { expect(spawn).toHaveBeenCalledWith( env.binary, [ + EMPTY_TOKEN_FLAG, "--verbose", "--global-config", "/cfg", diff --git a/test/unit/logging/formatters.test.ts b/test/unit/logging/formatters.test.ts index ded822f45e..4fe97cf25b 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -1,3 +1,4 @@ +import { AxiosError, AxiosHeaders } from "axios"; import { describe, expect, it } from "vitest"; import { @@ -291,4 +292,36 @@ describe("safeStringify", () => { } expect(safeStringify(nested)).not.toContain("bottom"); }); + + it("keeps request credentials out of Axios errors", () => { + const config = { + method: "post", + url: "https://coder.example.com/oauth2/revoke", + headers: new AxiosHeaders({ "Coder-Session-Token": "session-secret" }), + data: "token=token-secret&client_secret=client-secret", + }; + const error = new AxiosError( + "Bad request", + "ERR_BAD_REQUEST", + config, + {}, + { + status: 400, + statusText: "Bad Request", + headers: {}, + config, + data: { error: "invalid_client", refresh_token: "reply-secret" }, + }, + ); + + const result = safeStringify(error); + + expect(result).toContain("POST"); + expect(result).toContain("https://coder.example.com/oauth2/revoke"); + expect(result).toContain("status: 400"); + expect(result).toContain("invalid_client"); + expect(result).not.toMatch( + /session-secret|token-secret|client-secret|reply-secret/, + ); + }); }); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index ab413003ce..bfe90948cb 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -6,9 +6,14 @@ import { HttpStatusCode } from "@/api/httpStatusCode"; import { getHeaders } from "@/headers"; import { AuthTelemetry } from "@/instrumentation/auth"; import { LoginCoordinator, type LoginMethod } from "@/login/loginCoordinator"; +import { + ClientRegistrationDisabledError, + OAuthAuthorizer, +} from "@/oauth/authorizer"; import { OAuthCallback } from "@/oauth/oauthCallback"; +import { revokeOAuthTokens } from "@/oauth/revocation"; import { MementoManager } from "@/storage/mementoManager"; -import { SecretsManager } from "@/storage/secretsManager"; +import { SecretsManager, type SessionAuth } from "@/storage/secretsManager"; import { maybeAskAuthMethod, maybeAskUrl } from "@/ui/prompts"; import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; @@ -23,6 +28,10 @@ import { MockProgressReporter, MockUserInteraction, } from "../../mocks/testHelpers"; +import { + createMockClientRegistration, + createMockTokenResponse, +} from "../oauth/testUtils"; import type { User } from "coder/site/src/api/typesGenerated"; @@ -75,6 +84,8 @@ vi.mock("@/ui/prompts", () => ({ maybeAskUrl: vi.fn(), })); +vi.mock("@/oauth/revocation", () => ({ revokeOAuthTokens: vi.fn() })); + // Mock CoderApi to control getAuthenticatedUser behavior const mockGetAuthenticatedUser = vi.hoisted(() => vi.fn()); vi.mock("@/api/coderApi", async (importOriginal) => { @@ -853,6 +864,169 @@ describe("LoginCoordinator", () => { }); }); + describe("replacing an OAuth session", () => { + const registration = createMockClientRegistration(); + const oldSession: SessionAuth = { + url: TEST_URL, + token: "old-token", + username: createMockUser().username, + oauth: { + refresh_token: "old-refresh-token", + scope: "coder:all", + expiry_timestamp: Date.now() + 60 * 60 * 1000, + }, + }; + + async function setup() { + const ctx = createTestContext(); + const user = ctx.mockSuccessfulAuth(); + await ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, oldSession); + await ctx.secretsManager.setOAuthClientRegistration( + TEST_HOSTNAME, + registration, + ); + return { + ...ctx, + user, + login: (token?: string) => + ctx.coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token, + }), + stored: () => ctx.secretsManager.getSessionAuth(TEST_HOSTNAME), + }; + } + + it.each([undefined, "old-token"])( + "keeps the OAuth data of a reused session (provided token: %s)", + async (token) => { + const t = await setup(); + + expect(await t.login(token)).toMatchObject({ success: true }); + expect(await t.stored()).toEqual(oldSession); + expect(revokeOAuthTokens).not.toHaveBeenCalled(); + }, + ); + + it.each<{ + name: string; + token: string | undefined; + method: "legacy" | "oauth"; + }>([ + { name: "a manual token", token: undefined, method: "legacy" }, + { name: "a provided token", token: "new-token", method: "legacy" }, + { name: "an OAuth login", token: undefined, method: "oauth" }, + ])( + "revokes the session replaced by $name, after saving it", + async ({ token, method }) => { + const t = await setup(); + const rotated = { ...oldSession, token: "rotated-token" }; + // While the first token is checked, a refresh rotates the session + // and a different client registers; the stored token has expired. + t.mockGetAuthenticatedUser.mockImplementationOnce(async () => { + await t.secretsManager.setSessionAuth(TEST_HOSTNAME, rotated); + await t.secretsManager.setOAuthClientRegistration( + TEST_HOSTNAME, + createMockClientRegistration({ client_id: "other-client" }), + ); + if (!token) { + throw createAxiosError(HttpStatusCode.UNAUTHORIZED, "Expired"); + } + return t.user; + }); + vi.mocked(maybeAskAuthMethod).mockResolvedValue(method); + t.userInteraction.setInputBoxValue("new-token"); + vi.spyOn(OAuthAuthorizer.prototype, "login").mockResolvedValue({ + tokenResponse: createMockTokenResponse({ access_token: "new-token" }), + user: t.user, + registration: createMockClientRegistration({ + client_id: "new-client", + }), + }); + let storedAtRevoke: Promise | undefined; + vi.mocked(revokeOAuthTokens).mockImplementation(() => { + storedAtRevoke = t.stored(); + return Promise.resolve(); + }); + + expect(await t.login(token)).toMatchObject({ token: "new-token" }); + expect((await storedAtRevoke)?.token).toBe("new-token"); + expect(revokeOAuthTokens).toHaveBeenCalledExactlyOnceWith( + rotated, + registration, + t.logger, + ); + }, + ); + + it("keeps a session a refresh rotates while its stored token is checked", async () => { + const t = await setup(); + const rotated = { ...oldSession, token: "rotated-token" }; + t.mockGetAuthenticatedUser.mockImplementationOnce(async () => { + await t.secretsManager.setSessionAuth(TEST_HOSTNAME, rotated); + return t.user; + }); + + expect(await t.login()).toMatchObject({ method: "stored_token" }); + expect(await t.stored()).toEqual(rotated); + expect(revokeOAuthTokens).not.toHaveBeenCalled(); + }); + + it.each([ + { name: "the provided token is invalid", token: "bad-token" }, + { name: "the manual token prompt is cancelled", token: undefined }, + ])("keeps the old session when $name", async ({ token }) => { + const t = await setup(); + t.mockAuthFailure(); + + expect(await t.login(token)).toMatchObject({ success: false }); + expect(await t.stored()).toEqual(oldSession); + expect(revokeOAuthTokens).not.toHaveBeenCalled(); + }); + }); + + describe("when client registration is disabled", () => { + function setup() { + const ctx = createTestContext(); + ctx.mockSuccessfulAuth(); + vi.mocked(maybeAskAuthMethod).mockResolvedValue("oauth"); + vi.spyOn(OAuthAuthorizer.prototype, "login").mockRejectedValue( + new ClientRegistrationDisabledError(), + ); + ctx.userInteraction.setInputBoxValue("session-token"); + return { + ...ctx, + login: () => + ctx.coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }), + }; + } + + it("signs in with a session token when the user chooses it", async () => { + const t = setup(); + t.userInteraction.setResponse( + "OAuth sign-in is unavailable", + "Sign In with Token", + ); + + expect(await t.login()).toMatchObject({ + method: "cli_token", + token: "session-token", + }); + }); + + it("cancels sign-in when the user dismisses the prompt", async () => { + const t = setup(); + t.userInteraction.setResponse("OAuth sign-in is unavailable", undefined); + + expect(await t.login()).toMatchObject({ reason: "user_dismissed" }); + expect(vscode.window.showInputBox).not.toHaveBeenCalled(); + }); + }); + describe("telemetry", () => { const dialogOptions = (trigger: "auth_required" | "missing_session") => ({ url: TEST_URL, diff --git a/test/unit/oauth/authorizer.test.ts b/test/unit/oauth/authorizer.test.ts index 2780ed6a68..3e464d4a4c 100644 --- a/test/unit/oauth/authorizer.test.ts +++ b/test/unit/oauth/authorizer.test.ts @@ -3,13 +3,17 @@ import * as vscode from "vscode"; import { InvalidApiResponseError } from "@/api/responseValidation"; import { getHeaders } from "@/headers"; -import { OAuthAuthorizer } from "@/oauth/authorizer"; +import { + ClientRegistrationDisabledError, + OAuthAuthorizer, +} from "@/oauth/authorizer"; import { DEFAULT_OAUTH_SCOPES, IF_SUPPORTED_OAUTH_SCOPES, } from "@/oauth/constants"; import { + createAxiosError, createMockUser, MockCancellationToken, MockProgress, @@ -525,6 +529,21 @@ describe("OAuthAuthorizer", () => { ).rejects.toThrow("Server does not support dynamic client registration"); }); + it("throws ClientRegistrationDisabledError when the server refuses registration", async () => { + const { setupOAuthRoutes, authorizer } = createTestContext(); + setupOAuthRoutes(undefined, { + "/oauth2/register": createAxiosError(403, "Forbidden"), + }); + + await expect( + authorizer.login( + createTestDeployment(), + new MockProgress(), + new MockCancellationToken(), + ), + ).rejects.toThrow(ClientRegistrationDisabledError); + }); + it("rejects a token response without an access token", async () => { const { setupOAuthRoutes, startLogin, completeLogin } = createTestContext(); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 71d38f1d50..3848fc20d4 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -33,6 +33,7 @@ import { } from "./testUtils"; import type { Deployment } from "@/deployment/types"; +import type { OAuthClient } from "@/oauth/tokens"; import type { TelemetryService } from "@/telemetry/service"; vi.mock("axios", async () => { @@ -85,6 +86,7 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { refreshToken?: string; expiryMs?: number; scope?: string; + client?: OAuthClient; } = {}, ) => { await base.secretsManager.setSessionAuth(TEST_HOSTNAME, { @@ -95,6 +97,7 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { refresh_token: overrides.refreshToken ?? "refresh-token", expiry_timestamp: Date.now() + (overrides.expiryMs ?? ONE_HOUR_MS), scope: overrides.scope ?? DEFAULT_OAUTH_SCOPES, + client: overrides.client, }, }); }; @@ -136,14 +139,14 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { } describe("OAuthSessionManager", () => { - describe("isLoggedInWithOAuth", () => { - interface IsLoggedInTestCase { + describe("canRefreshOAuthSession", () => { + interface CanRefreshTestCase { name: string; auth: SessionAuth | null; expected: boolean; } - it.each([ + it.each([ { name: "returns true when OAuth tokens exist", auth: { @@ -177,7 +180,7 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, auth); } - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(expected); }); }); @@ -213,6 +216,31 @@ describe("OAuthSessionManager", () => { }); }); + it("authenticates as the client that issued the tokens and keeps it", async () => { + const { secretsManager, manager, setupForOAuthOperation } = + createTestContext(); + const client = { client_id: "issuer", client_secret: "issuer-secret" }; + let params: URLSearchParams | undefined; + await setupForOAuthOperation( + { + "/oauth2/token": (config: InternalAxiosRequestConfig) => { + params = new URLSearchParams(config.data as string); + return createMockTokenResponse(); + }, + "/api/v2/users/me": createMockUser(), + }, + { client }, + ); + + await manager.refreshToken(); + + expect(params?.get("client_id")).toBe("issuer"); + expect(params?.get("client_secret")).toBe("issuer-secret"); + expect( + (await secretsManager.getSessionAuth(TEST_HOSTNAME))?.oauth, + ).toMatchObject({ client }); + }); + it("keeps the stored username when the user lookup fails", async () => { const { secretsManager, manager, setupForOAuthOperation } = createTestContext(); @@ -264,7 +292,7 @@ describe("OAuthSessionManager", () => { }, }); - expect(await manager.isLoggedInWithOAuth()).toBe(false); + expect(await manager.canRefreshOAuthSession()).toBe(false); }); }); @@ -279,7 +307,7 @@ describe("OAuthSessionManager", () => { await manager.setDeployment(newDeployment); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -290,7 +318,7 @@ describe("OAuthSessionManager", () => { manager.clearDeployment(); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -461,28 +489,50 @@ describe("OAuthSessionManager", () => { }); describe("revokeTokens", () => { - it("revokes the refresh and access tokens", async () => { - const { manager, setupForOAuthOperation } = createTestContext(); - - const revoked: Array<{ token: string | null; hint: string | null }> = []; - await setupForOAuthOperation({ - "/oauth2/revoke": (config: InternalAxiosRequestConfig) => { - const params = new URLSearchParams(config.data as string); - revoked.push({ - token: params.get("token"), - hint: params.get("token_type_hint"), - }); - return {}; - }, - }); + interface RevokeCase { + name: string; + client?: OAuthClient; + clientId: string; + } + it.each([ + { + name: "the client that issued the tokens", + client: { client_id: "issuer", client_secret: "issuer-secret" }, + clientId: "issuer", + }, + { + name: "the stored registration without one", + clientId: "test-client-id", + }, + ])( + "revokes the refresh and access tokens as $name", + async ({ client, clientId }) => { + const { manager, setupForOAuthOperation } = createTestContext(); + + const revoked: Array> = []; + await setupForOAuthOperation( + { + "/oauth2/revoke": (config: InternalAxiosRequestConfig) => { + const params = new URLSearchParams(config.data as string); + revoked.push({ + token: params.get("token"), + hint: params.get("token_type_hint"), + client: params.get("client_id"), + }); + return {}; + }, + }, + { client }, + ); - await manager.revokeTokens(); + await manager.revokeTokens(); - expect(revoked).toEqual([ - { token: "refresh-token", hint: "refresh_token" }, - { token: "access-token", hint: "access_token" }, - ]); - }); + expect(revoked).toEqual([ + { token: "refresh-token", hint: "refresh_token", client: clientId }, + { token: "access-token", hint: "access_token", client: clientId }, + ]); + }, + ); it("revokes tokens with outdated scopes", async () => { const { manager, setupForOAuthOperation } = createTestContext(); @@ -518,7 +568,7 @@ describe("OAuthSessionManager", () => { }); describe("scope validation", () => { - it("rejects tokens with insufficient scopes", async () => { + it("cannot refresh tokens with insufficient scopes", async () => { const { secretsManager, manager } = createTestContext(); await secretsManager.setSessionAuth(TEST_HOSTNAME, { @@ -531,7 +581,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); @@ -550,7 +600,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(true); }); @@ -558,7 +608,7 @@ describe("OAuthSessionManager", () => { const { manager, setupOAuthSession } = createTestContext(); await setupOAuthSession({ scope: "coder:all" }); - expect(await manager.isLoggedInWithOAuth()).toBe(true); + expect(await manager.canRefreshOAuthSession()).toBe(true); }); }); diff --git a/test/unit/oauth/tokens.test.ts b/test/unit/oauth/tokens.test.ts index 34092685a3..85074d8dd3 100644 --- a/test/unit/oauth/tokens.test.ts +++ b/test/unit/oauth/tokens.test.ts @@ -5,6 +5,7 @@ import { buildOAuthTokenData } from "@/oauth/tokens"; import type { OAuth2TokenResponse } from "coder/site/src/api/typesGenerated"; const ACCESS_TOKEN_DEFAULT_EXPIRY_MS = 60 * 60 * 1000; +const CLIENT = { client_id: "client-id", client_secret: "client-secret" }; function createTokenResponse( overrides: Partial = {}, @@ -24,6 +25,7 @@ describe("buildOAuthTokenData", () => { it("uses expires_in when valid", () => { const result = buildOAuthTokenData( createTokenResponse({ expires_in: 7200 }), + CLIENT, ); const expectedExpiry = Date.now() + 7200 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -36,6 +38,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: 0 }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -46,6 +49,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: -100 }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -56,6 +60,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: undefined }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -66,6 +71,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: Infinity }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -81,6 +87,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: futureExpiry.toISOString(), }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( futureExpiry.getTime() - 100, @@ -97,6 +104,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: pastExpiry.toISOString(), }), + CLIENT, ); const expectedExpiry = Date.now() + 3600 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -111,6 +119,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: "not-a-valid-date", }), + CLIENT, ); const expectedExpiry = Date.now() + 3600 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -126,6 +135,7 @@ describe("buildOAuthTokenData", () => { expires_in: undefined, expiry: "not-a-valid-date", }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -138,6 +148,7 @@ describe("buildOAuthTokenData", () => { expires_in: 7200, expiry: undefined, }), + CLIENT, ); const expectedExpiry = Date.now() + 7200 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -147,16 +158,26 @@ describe("buildOAuthTokenData", () => { }); }); + it("records the client the tokens were issued to", () => { + expect(buildOAuthTokenData(createTokenResponse(), CLIENT).client).toEqual( + CLIENT, + ); + }); + it("stores coder:all when the server returns no scope", () => { expect( - buildOAuthTokenData(createTokenResponse({ scope: undefined })).scope, + buildOAuthTokenData(createTokenResponse({ scope: undefined }), CLIENT) + .scope, ).toBe("coder:all"); }); describe("token_type validation", () => { it("accepts Bearer tokens", () => { expect(() => - buildOAuthTokenData(createTokenResponse({ token_type: "Bearer" })), + buildOAuthTokenData( + createTokenResponse({ token_type: "Bearer" }), + CLIENT, + ), ).not.toThrow(); }); @@ -166,6 +187,7 @@ describe("buildOAuthTokenData", () => { expect(() => buildOAuthTokenData( createTokenResponse({ token_type: tokenType as "Bearer" }), + CLIENT, ), ).toThrow(`Unsupported token type: ${tokenType}`); }, diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index eb8c6a38ec..60d63d49dd 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -394,7 +394,7 @@ describe("Remote", () => { }); expect(elideNetworkInfoDir(proxyCommand)).toBe( - `/mock/coder --global-config /mock/global --url https://coder.example.com ${expected}`, + `/mock/coder --token= --global-config /mock/global --url https://coder.example.com ${expected}`, ); }); }); diff --git a/test/unit/settings/cli.test.ts b/test/unit/settings/cli.test.ts index 52c894547b..f11c38ee41 100644 --- a/test/unit/settings/cli.test.ts +++ b/test/unit/settings/cli.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, it, expect, describe, vi } from "vitest"; import { cliFeatureSet } from "@/featureSet"; import { type CliAuth, + EMPTY_TOKEN_FLAG, getExpandedUserGlobalFlags, getGlobalFlags, getGlobalShellFlags, @@ -67,7 +68,10 @@ describe("cliConfig", () => { }, ])("emits auth flags for a $scenario", ({ auth, expected }) => { const config = new MockConfigurationProvider(); - expect(getGlobalShellFlags(config, auth)).toStrictEqual(expected); + expect(getGlobalShellFlags(config, auth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, + ...expected, + ]); }); it("appends auth flags after user global flags", () => { @@ -75,6 +79,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--global-configs"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "--verbose", "--global-configs", // similar prefixes are not managed flags ...EXTENSION_FLAGS, @@ -86,6 +91,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--use-keyring=false"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "--verbose", ...EXTENSION_FLAGS, ]); @@ -106,6 +112,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, ...flags, ...CLI_FLAGS, ]); @@ -125,6 +132,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "-v", ...EXTENSION_FLAGS, ]); @@ -138,6 +146,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["-v", "--header-command custom"]); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "-v", '"--header-command custom"', // ignored by CLI ...CLI_FLAGS, @@ -153,6 +162,7 @@ describe("cliConfig", () => { // Without per-entry escaping the space splits the shell command. expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, '"--cfg=C:\\Users\\John Doe/coder"', ...EXTENSION_FLAGS, ]); @@ -166,12 +176,24 @@ describe("cliConfig", () => { config.set("coder.headerCommand", "echo test"); expect(getGlobalFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "--verbose", ...EXTENSION_FLAGS, "--header-command", "echo test", ]); }); + + it("places a user --token after the empty token flag so it wins", () => { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["--token=abc"]); + + expect(getGlobalFlags(config, cliStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, + "--token=abc", + ...CLI_FLAGS, + ]); + }); }); describe("getExpandedUserGlobalFlags", () => { @@ -431,9 +453,10 @@ describe("cliConfig", () => { } config.set("coder.globalFlags", userFlags); - expect(getGlobalFlags(config, resolve(config, version))).toStrictEqual( - expected, - ); + expect(getGlobalFlags(config, resolve(config, version))).toStrictEqual([ + EMPTY_TOKEN_FLAG, + ...expected, + ]); }); }); });