From 183b2bce21b3c6015832e8c850bf3203581995fa Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Sat, 10 Oct 2026 18:52:53 +0300 Subject: [PATCH 1/2] fix(oauth): revoke replaced sessions and preserve token metadata Clean up replaced OAuth sessions without forcing a re-login when the required scopes change. - Insufficient scopes prevent refresh but keep the stored access token, so normal 401 recovery applies after expiry. Rename the check to `canRefreshOAuthSession`. - Save a successful replacement first, then revoke the overwritten OAuth pair in the background with the client registration captured before login. Failed or canceled logins, same-token reuse, and stored-session adoption revoke nothing. - Keep refresh credentials and scopes when the same token is reused on the same origin, instead of turning it into a manual-token session. If a refresh rotates the session while its stored token is checked, keep the rotated session. - Share revocation with logout through `withOAuthMetadata`, which token refresh also uses, and request `user:update_personal` to refresh expired external-auth links. - If the server refuses dynamic client registration (off by default from Coder 2.38), name the setting and offer session-token sign-in. - Start every CLI invocation's global flags with an empty `--token=`, so `CODER_SESSION_TOKEN` in the extension host (in any case) no longer overrides the stored session. This covers the SSH ProxyCommand and terminals too. `coder login --use-token-as-session` omits it, since it reads the token from the environment, and gets an environment without the host's case variants of the variable. Closes #1140 --- CHANGELOG.md | 15 +- src/api/authInterceptor.ts | 2 +- src/cli/cliCredentialManager.ts | 25 ++- src/login/loginCoordinator.ts | 125 ++++++++++----- src/oauth/authorizer.ts | 29 +++- src/oauth/constants.ts | 5 +- src/oauth/metadataClient.ts | 28 ++++ src/oauth/revocation.ts | 66 ++++++++ src/oauth/sessionManager.ts | 122 ++++----------- src/settings/cli.ts | 7 +- test/mocks/testHelpers.ts | 2 +- test/unit/api/authInterceptor.test.ts | 8 +- test/unit/api/workspace.test.ts | 15 +- test/unit/cli/cliCredentialManager.test.ts | 11 +- test/unit/cli/cliExec.test.ts | 15 +- test/unit/login/loginCoordinator.test.ts | 173 ++++++++++++++++++++- test/unit/oauth/authorizer.test.ts | 21 ++- test/unit/oauth/sessionManager.test.ts | 22 +-- test/unit/remote/remote.test.ts | 2 +- test/unit/settings/cli.test.ts | 31 +++- 20 files changed, 538 insertions(+), 186 deletions(-) create mode 100644 src/oauth/revocation.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index acac656635..f2083826de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,9 +20,18 @@ ### 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. +- 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..040e688c6f 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, which the empty flag would hide. + 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 the only session token, since the CLI reads the name 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/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 98263562cf..929e49ac4d 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 rotated the stored session while this login checked 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,23 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, tokenSignInConfirmed = false, ): Promise { + // Read before an OAuth 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 +498,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,12 +662,10 @@ 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"); @@ -666,6 +690,21 @@ export class LoginCoordinator implements vscode.Disposable { oauth: buildOAuthTokenData(tokenResponse), }; } 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..b51ed572f3 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,14 @@ import type { SecretsManager } from "../storage/secretsManager"; import type { OAuthCallback } from "./oauthCallback"; +/** 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. @@ -171,10 +180,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..dee54fe768 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,7 @@ export const DEFAULT_OAUTH_SCOPES = [ "workspace:create", "user:read", "user:read_personal", + "user:update_personal", ].join(" "); /** diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index b6b5b4e24f..bb2d5fb31a 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,33 @@ export class OAuthMetadataClient { } } +/** + * Run `fn` with a short-lived client for `url` and the server's OAuth + * metadata. The client is disposed on exit so its config-change + * subscriptions never outlive the operation. + */ +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..db177bb6d8 --- /dev/null +++ b/src/oauth/revocation.ts @@ -0,0 +1,66 @@ +import { withOAuthMetadata } from "./metadataClient"; +import { toUrlSearchParams } from "./tokens"; + +import type { + OAuth2ClientRegistrationResponse, + 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. */ +export async function revokeOAuthTokens( + auth: SessionAuth, + registration: OAuth2ClientRegistrationResponse | undefined, + logger: Logger, +): Promise { + if (!auth.oauth) { + return; + } + if (!registration) { + 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: 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" }, + }); + 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..b26095b8c4 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,11 +361,18 @@ export class OAuthSessionManager implements vscode.Disposable { } const refreshToken = storedTokens.refresh_token; - const accessToken = storedTokens.access_token; + const registration = await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + ); + if (!registration) { + 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 = { @@ -484,64 +452,32 @@ 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. + * Returns true if the current deployment has OAuth tokens that may be + * refreshed, i.e. they carry every required scope. Always reads fresh from + * secrets to ensure cross-window synchronization. * * @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/settings/cli.ts b/src/settings/cli.ts index 3d42bd81db..164472206c 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -53,6 +53,9 @@ export function getGlobalFlags( const identity = (s: string) => s; +/** Overrides `CODER_SESSION_TOKEN` in any case variant, so the CLI reads 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); + // The last `--token` wins, so a user's own token still applies. + const flags = [EMPTY_TOKEN_FLAG, ...userFlags]; if (auth.store === "extension") { flags.push("--global-config", escAuth(auth.configDir)); } 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/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index ab413003ce..9645259b28 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,166 @@ 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, + }); + 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..58e1d8a3c7 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -136,14 +136,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 +177,7 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, auth); } - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(expected); }); }); @@ -264,7 +264,7 @@ describe("OAuthSessionManager", () => { }, }); - expect(await manager.isLoggedInWithOAuth()).toBe(false); + expect(await manager.canRefreshOAuthSession()).toBe(false); }); }); @@ -279,7 +279,7 @@ describe("OAuthSessionManager", () => { await manager.setDeployment(newDeployment); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -290,7 +290,7 @@ describe("OAuthSessionManager", () => { manager.clearDeployment(); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -518,7 +518,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 +531,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); @@ -550,7 +550,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(true); }); @@ -558,7 +558,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/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, + ]); }); }); }); From fdfe25b434a2fa42e6881c69bd9c53333a6544bf Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 12 Oct 2026 17:46:42 +0300 Subject: [PATCH 2/2] fix(oauth): keep credentials out of logs and revoke as the issuing client - Log Axios errors as their request line and the server's reply. Their config and socket hold the session token, OAuth tokens, and client secret, which the buffered connection logs replayed into the output channel. - Record the client that issued each OAuth session, and refresh and revoke as that client, so another window re-registering the client no longer breaks either. Sessions without a recorded client use the stored registration. - Explain why the extension requests `user:update_personal`. - In the changelog, note that OAuth becomes the default sign-in method and link the OAuth2 provider docs. - Shorten comments. --- CHANGELOG.md | 5 ++ src/cli/cliCredentialManager.ts | 4 +- src/logging/formatters.ts | 18 ++++- src/login/loginCoordinator.ts | 34 ++++----- src/oauth/authorizer.ts | 19 ++--- src/oauth/constants.ts | 2 + src/oauth/metadataClient.ts | 6 +- src/oauth/revocation.ts | 21 +++--- src/oauth/sessionManager.ts | 21 +++--- src/oauth/tokens.ts | 18 ++++- src/settings/cli.ts | 4 +- src/storage/secretsManager.ts | 4 ++ test/unit/logging/formatters.test.ts | 33 +++++++++ test/unit/login/loginCoordinator.test.ts | 3 + test/unit/oauth/sessionManager.test.ts | 90 ++++++++++++++++++------ test/unit/oauth/tokens.test.ts | 26 ++++++- 16 files changed, 228 insertions(+), 80 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2083826de..8dd9c7b30b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,11 @@ 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. diff --git a/src/cli/cliCredentialManager.ts b/src/cli/cliCredentialManager.ts index 040e688c6f..989ea2692a 100644 --- a/src/cli/cliCredentialManager.ts +++ b/src/cli/cliCredentialManager.ts @@ -76,7 +76,7 @@ export class CliCredentialManager { return; } span.setProperty("store", cli.auth.store); - // `login` reads the token from the environment, which the empty flag would hide. + // `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 }, @@ -276,7 +276,7 @@ export class CliCredentialManager { } } -/** `process.env` with `token` as the only session token, since the CLI reads the name in any case. */ +/** `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( 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 929e49ac4d..877b8a95ba 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -204,7 +204,7 @@ export class LoginCoordinator implements vscode.Disposable { const { safeHostname, url } = deployment; const previous = await this.secretsManager.getSessionAuth(safeHostname); const replaced = previous?.token === result.token ? undefined : previous; - // A refresh rotated the stored session while this login checked it. + // A refresh replaced the stored session during this login; keep it. if (result.method === "stored_token" && replaced) { return; } @@ -299,7 +299,8 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, tokenSignInConfirmed = false, ): Promise { - // Read before an OAuth login can re-register the client. + // 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, ); @@ -669,25 +670,26 @@ export class LoginCoordinator implements vscode.Disposable { 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) { diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index b51ed572f3..ec97220b70 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -38,6 +38,13 @@ 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() { @@ -60,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( @@ -88,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"); @@ -133,7 +136,7 @@ export class OAuthAuthorizer implements vscode.Disposable { this.logger.debug("OAuth login flow completed successfully"); - return { tokenResponse, user }; + return { tokenResponse, user, registration }; } /** diff --git a/src/oauth/constants.ts b/src/oauth/constants.ts index dee54fe768..7d48893f78 100644 --- a/src/oauth/constants.ts +++ b/src/oauth/constants.ts @@ -12,6 +12,8 @@ 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 bb2d5fb31a..af33bc23db 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -143,11 +143,7 @@ export class OAuthMetadataClient { } } -/** - * Run `fn` with a short-lived client for `url` and the server's OAuth - * metadata. The client is disposed on exit so its config-change - * subscriptions never outlive the operation. - */ +/** Runs `fn` with the server's OAuth metadata and a client disposed afterward. */ export async function withOAuthMetadata( url: string, token: string | undefined, diff --git a/src/oauth/revocation.ts b/src/oauth/revocation.ts index db177bb6d8..ffa9c36acf 100644 --- a/src/oauth/revocation.ts +++ b/src/oauth/revocation.ts @@ -1,24 +1,25 @@ import { withOAuthMetadata } from "./metadataClient"; -import { toUrlSearchParams } from "./tokens"; +import { type OAuthClient, toUrlSearchParams } from "./tokens"; -import type { - OAuth2ClientRegistrationResponse, - OAuth2TokenRevocationRequest, -} from "coder/site/src/api/typesGenerated"; +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. */ +/** + * 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, - registration: OAuth2ClientRegistrationResponse | undefined, + fallbackClient: OAuthClient | undefined, logger: Logger, ): Promise { if (!auth.oauth) { return; } - if (!registration) { + const client = auth.oauth.client ?? fallbackClient; + if (!client) { logger.warn("Skipped token revocation: no client registration"); return; } @@ -45,8 +46,8 @@ export async function revokeOAuthTokens( } const params: OAuth2TokenRevocationRequest = { token, - client_id: registration.client_id, - client_secret: registration.client_secret, + client_id: client.client_id, + client_secret: client.client_secret, token_type_hint, }; try { diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index b26095b8c4..de3bbfbfb8 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -361,10 +361,12 @@ export class OAuthSessionManager implements vscode.Disposable { } const refreshToken = storedTokens.refresh_token; - const registration = await this.secretsManager.getOAuthClientRegistration( - deployment.safeHostname, - ); - if (!registration) { + const client = + storedTokens.client ?? + (await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + )); + if (!client) { throw new Error("No client registration found"); } @@ -378,8 +380,8 @@ export class OAuthSessionManager implements vscode.Disposable { 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( @@ -412,7 +414,7 @@ export class OAuthSessionManager implements vscode.Disposable { deployment, tokenResponse.access_token, ), - oauth: buildOAuthTokenData(tokenResponse), + oauth: buildOAuthTokenData(tokenResponse, client), }); return tokenResponse; @@ -470,9 +472,8 @@ export class OAuthSessionManager implements vscode.Disposable { } /** - * Returns true if the current deployment has OAuth tokens that may be - * refreshed, i.e. they carry every required scope. 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). 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 164472206c..94a5aa2e5d 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -53,7 +53,7 @@ export function getGlobalFlags( const identity = (s: string) => s; -/** Overrides `CODER_SESSION_TOKEN` in any case variant, so the CLI reads its stored session. */ +/** Hides `CODER_SESSION_TOKEN` (in any case) so the CLI uses its stored session. */ export const EMPTY_TOKEN_FLAG = "--token="; function buildGlobalFlags( @@ -67,7 +67,7 @@ function buildGlobalFlags( getExpandedUserGlobalFlags(configs), auth.store === "extension", ).map(escAuth); - // The last `--token` wins, so a user's own token still applies. + // 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/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 9645259b28..bfe90948cb 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -940,6 +940,9 @@ describe("LoginCoordinator", () => { 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(() => { diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 58e1d8a3c7..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, }, }); }; @@ -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(); @@ -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(); 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}`); },