From 4529414fb80f07894a7de95b2958e2cbd6e42a03 Mon Sep 17 00:00:00 2001 From: umair Date: Wed, 15 Apr 2026 20:17:00 +0100 Subject: [PATCH 01/19] Add OAuth Device Authorization Grant login flow --- src/base-command.ts | 8 +- src/control-base-command.ts | 18 +- src/services/config-manager.ts | 202 ++++++++++++++++- src/services/control-api.ts | 41 ++++ src/services/oauth-client.ts | 275 +++++++++++++++++++++++ src/services/token-refresh-middleware.ts | 66 ++++++ 6 files changed, 596 insertions(+), 14 deletions(-) create mode 100644 src/services/oauth-client.ts create mode 100644 src/services/token-refresh-middleware.ts diff --git a/src/base-command.ts b/src/base-command.ts index 193ca708..e87c4fb5 100644 --- a/src/base-command.ts +++ b/src/base-command.ts @@ -166,7 +166,9 @@ export abstract class AblyBaseCommand extends InteractiveBaseCommand { constructor(argv: string[], config: CommandConfig) { super(argv, config); this.configManager = createConfigManager(); - this.interactiveHelper = new InteractiveHelper(this.configManager); + this.interactiveHelper = new InteractiveHelper(this.configManager, { + log: this.log.bind(this), + }); // Check if we're running in web CLI mode this.isWebCliMode = isWebCliMode(); } @@ -614,9 +616,9 @@ export abstract class AblyBaseCommand extends InteractiveBaseCommand { if (!appName) { try { // Get access token for control API - const currentAccount = this.configManager.getCurrentAccount(); const accessToken = - process.env.ABLY_ACCESS_TOKEN || currentAccount?.accessToken; + process.env.ABLY_ACCESS_TOKEN || + this.configManager.getAccessToken(); if (accessToken) { const controlApi = new ControlApi({ diff --git a/src/control-base-command.ts b/src/control-base-command.ts index 8b422b14..dddd81b1 100644 --- a/src/control-base-command.ts +++ b/src/control-base-command.ts @@ -1,6 +1,8 @@ import { AblyBaseCommand } from "./base-command.js"; import { controlApiFlags } from "./flags.js"; import { ControlApi, App } from "./services/control-api.js"; +import { OAuthClient } from "./services/oauth-client.js"; +import { TokenRefreshMiddleware } from "./services/token-refresh-middleware.js"; import { BaseFlags } from "./types/cli.js"; import { errorMessage } from "./utils/errors.js"; import isWebCliMode from "./utils/web-mode.js"; @@ -14,6 +16,7 @@ export abstract class ControlBaseCommand extends AblyBaseCommand { */ protected createControlApi(flags: BaseFlags): ControlApi { let accessToken = process.env.ABLY_ACCESS_TOKEN; + let tokenRefreshMiddleware: TokenRefreshMiddleware | undefined; if (!accessToken) { const account = this.configManager.getCurrentAccount(); @@ -25,7 +28,19 @@ export abstract class ControlBaseCommand extends AblyBaseCommand { ); } - accessToken = account.accessToken; + accessToken = this.configManager.getAccessToken(); + + // Set up token refresh middleware for OAuth accounts + if (this.configManager.getAuthMethod() === "oauth") { + const oauthHost = flags["control-host"] || account.controlHost; + const oauthClient = new OAuthClient({ + controlHost: oauthHost, + }); + tokenRefreshMiddleware = new TokenRefreshMiddleware( + this.configManager, + oauthClient, + ); + } } if (!accessToken) { @@ -47,6 +62,7 @@ export abstract class ControlBaseCommand extends AblyBaseCommand { return new ControlApi({ accessToken, controlHost: flags["control-host"], + tokenRefreshMiddleware, }); } diff --git a/src/services/config-manager.ts b/src/services/config-manager.ts index 49197d7d..555b852a 100644 --- a/src/services/config-manager.ts +++ b/src/services/config-manager.ts @@ -14,18 +14,32 @@ export interface AppConfig { } export interface AccountConfig { - accessToken: string; + // Legacy: pre-OAuth configs store the access token directly on the account. + // OAuth accounts use oauthSessionKey to reference a shared OAuthSession instead. + // Do not remove — needed for backward compatibility with existing configs. + accessToken?: string; + accessTokenExpiresAt?: number; accountId: string; accountName: string; apps?: { [appId: string]: AppConfig; }; + authMethod?: "oauth"; + controlHost?: string; currentAppId?: string; endpoint?: string; + oauthSessionKey?: string; tokenId?: string; userEmail: string; } +export interface OAuthSession { + accessToken: string; + accessTokenExpiresAt: number; + oauthScope?: string; + refreshToken: string; +} + export interface AblyConfig { accounts: Record; current?: { @@ -39,11 +53,12 @@ export interface AblyConfig { }[]; }; }; + oauthSessions?: Record; } export interface ConfigManager { // Account management - getAccessToken(alias?: string): string | undefined; + getAccessToken(): string | undefined; getCurrentAccount(): AccountConfig | undefined; getCurrentAccountAlias(): string | undefined; listAccounts(): { account: AccountConfig; alias: string }[]; @@ -59,6 +74,34 @@ export interface ConfigManager { ): void; switchAccount(alias: string): boolean; removeAccount(alias: string): boolean; + setAccountControlHost(alias: string, controlHost: string): void; + + // OAuth management + storeOAuthTokens( + alias: string, + tokens: { + accessToken: string; + refreshToken: string; + expiresAt: number; + scope?: string; + userId?: string; + userEmail?: string; + }, + accountInfo?: { + accountId?: string; + accountName?: string; + }, + ): void; + getOAuthTokens(alias?: string): + | { + accessToken: string; + refreshToken: string; + expiresAt: number; + } + | undefined; + isAccessTokenExpired(): boolean; + getAuthMethod(alias?: string): "oauth" | undefined; + getAliasesForOAuthSession(alias: string): string[]; // App management getApiKey(appId?: string): string | undefined; @@ -154,14 +197,18 @@ export class TomlConfigManager implements ConfigManager { this.saveConfig(); } - // Get access token for the current account or specific alias - public getAccessToken(alias?: string): string | undefined { - if (alias) { - return this.config.accounts[alias]?.accessToken; - } + public getAccessToken(): string | undefined { + const account = this.getCurrentAccount(); + if (!account) return undefined; - const currentAccount = this.getCurrentAccount(); - return currentAccount?.accessToken; + // OAuth accounts read from the shared session + const session = account.oauthSessionKey + ? this.config.oauthSessions?.[account.oauthSessionKey] + : undefined; + if (session) return session.accessToken; + + // Fallback: pre-OAuth configs store the token directly on the account + return account.accessToken; } // Get API key for current app or specific app ID @@ -293,12 +340,27 @@ export class TomlConfigManager implements ConfigManager { // Remove an account public removeAccount(alias: string): boolean { - if (!this.config.accounts[alias]) { + const account = this.config.accounts[alias]; + if (!account) { return false; } + const sessionKey = account.oauthSessionKey; delete this.config.accounts[alias]; + // Clean up orphaned OAuth session entry + if (sessionKey && this.config.oauthSessions?.[sessionKey]) { + const stillReferenced = Object.values(this.config.accounts).some( + (a) => a.oauthSessionKey === sessionKey, + ); + if (!stillReferenced) { + delete this.config.oauthSessions[sessionKey]; + if (Object.keys(this.config.oauthSessions).length === 0) { + delete this.config.oauthSessions; + } + } + } + // If the removed account was the current one, clear the current account selection if (this.config.current?.account === alias) { delete this.config.current.account; @@ -308,6 +370,16 @@ export class TomlConfigManager implements ConfigManager { return true; } + public getAliasesForOAuthSession(alias: string): string[] { + const account = this.config.accounts[alias]; + if (!account?.oauthSessionKey) return [alias]; + + const sessionKey = account.oauthSessionKey; + return Object.entries(this.config.accounts) + .filter(([, acc]) => acc.oauthSessionKey === sessionKey) + .map(([a]) => a); + } + // Remove API key for an app public removeApiKey(appId: string): boolean { const currentAccount = this.getCurrentAccount(); @@ -477,6 +549,110 @@ export class TomlConfigManager implements ConfigManager { this.saveConfig(); } + // Store OAuth tokens, shared across aliases with the same userEmail + public storeOAuthTokens( + alias: string, + tokens: { + accessToken: string; + refreshToken: string; + expiresAt: number; + scope?: string; + userId?: string; + userEmail?: string; + }, + accountInfo?: { + accountId?: string; + accountName?: string; + }, + ): void { + const userEmail = + tokens.userEmail ?? this.config.accounts[alias]?.userEmail ?? ""; + const sessionKey = userEmail.toLowerCase() || alias; + + // Create/update the shared OAuth session + if (!this.config.oauthSessions) { + this.config.oauthSessions = {}; + } + + this.config.oauthSessions[sessionKey] = { + accessToken: tokens.accessToken, + accessTokenExpiresAt: tokens.expiresAt, + oauthScope: tokens.scope, + refreshToken: tokens.refreshToken, + }; + + // Store account metadata and reference the OAuth session + this.config.accounts[alias] = { + ...this.config.accounts[alias], + accountId: + accountInfo?.accountId ?? this.config.accounts[alias]?.accountId ?? "", + accountName: + accountInfo?.accountName ?? + this.config.accounts[alias]?.accountName ?? + "", + apps: this.config.accounts[alias]?.apps || {}, + authMethod: "oauth", + currentAppId: this.config.accounts[alias]?.currentAppId, + oauthSessionKey: sessionKey, + userEmail, + }; + + if (!this.config.current || !this.config.current.account) { + this.config.current = { account: alias }; + } + + this.saveConfig(); + } + + // Get OAuth tokens for the current account or specific alias + public getOAuthTokens(alias?: string): + | { + accessToken: string; + refreshToken: string; + expiresAt: number; + } + | undefined { + const account = alias + ? this.config.accounts[alias] + : this.getCurrentAccount(); + if (!account || account.authMethod !== "oauth") return undefined; + + const session = account.oauthSessionKey + ? this.config.oauthSessions?.[account.oauthSessionKey] + : undefined; + if (!session) return undefined; + + return { + accessToken: session.accessToken, + expiresAt: session.accessTokenExpiresAt, + refreshToken: session.refreshToken, + }; + } + + public isAccessTokenExpired(): boolean { + const account = this.getCurrentAccount(); + if (!account) return false; + + // OAuth accounts read expiry from the shared session; + // falls back to account-level field for pre-OAuth configs + const session = account.oauthSessionKey + ? this.config.oauthSessions?.[account.oauthSessionKey] + : undefined; + const expiresAt = + session?.accessTokenExpiresAt ?? account.accessTokenExpiresAt; + if (!expiresAt) return false; + + return Date.now() >= expiresAt - 60_000; + } + + // Get the auth method for the current account or specific alias + public getAuthMethod(alias?: string): "oauth" | undefined { + const account = alias + ? this.config.accounts[alias] + : this.getCurrentAccount(); + return account?.authMethod; + } + // Switch to a different account public switchAccount(alias: string): boolean { if (!this.config.accounts[alias]) { @@ -492,6 +668,12 @@ export class TomlConfigManager implements ConfigManager { return true; } + public setAccountControlHost(alias: string, controlHost: string): void { + if (!this.config.accounts[alias]) return; + this.config.accounts[alias].controlHost = controlHost; + this.saveConfig(); + } + private ensureConfigDirExists(): void { if (!fs.existsSync(this.configDir)) { fs.mkdirSync(this.configDir, { mode: 0o700 }); // Secure permissions diff --git a/src/services/control-api.ts b/src/services/control-api.ts index 9880d38f..11c50c34 100644 --- a/src/services/control-api.ts +++ b/src/services/control-api.ts @@ -1,11 +1,14 @@ import fetch, { type RequestInit } from "node-fetch"; import { CommandError } from "../errors/command-error.js"; import { getAgentName, getCliVersion } from "../utils/version.js"; +import isTestMode from "../utils/test-mode.js"; +import type { TokenRefreshMiddleware } from "./token-refresh-middleware.js"; export interface ControlApiOptions { accessToken: string; controlHost?: string; logErrors?: boolean; + tokenRefreshMiddleware?: TokenRefreshMiddleware; } export interface App { @@ -177,13 +180,31 @@ export interface MeResponse { user: { email: string }; } +export interface AccountSummary { + id: string; + name: string; +} + export class ControlApi { private accessToken: string; private controlHost: string; + private logErrors: boolean; + private tokenRefreshMiddleware?: TokenRefreshMiddleware; constructor(options: ControlApiOptions) { this.accessToken = options.accessToken; this.controlHost = options.controlHost || "control.ably.net"; + this.tokenRefreshMiddleware = options.tokenRefreshMiddleware; + // Explicit options.logErrors overrides env var; otherwise suppress in CI/test + if (options.logErrors === undefined) { + const suppressErrors = + process.env.SUPPRESS_CONTROL_API_ERRORS === "true" || + process.env.CI === "true" || + isTestMode(); + this.logErrors = !suppressErrors; + } else { + this.logErrors = options.logErrors; + } } // Ask a question to the Ably AI agent @@ -378,6 +399,20 @@ export class ControlApi { return matchingKey; } + // Get all accounts for the authenticated user + async getAccounts(): Promise { + try { + return await this.request("/me/accounts"); + } catch (error: unknown) { + // Graceful degradation: fall back to single account from /me if endpoint not available + if (error instanceof CommandError && error.statusCode === 404) { + const me = await this.getMe(); + return [{ id: me.account.id, name: me.account.name }]; + } + throw error; + } + } + // Get user and account info async getMe(): Promise { return this.request("/me"); @@ -525,6 +560,12 @@ export class ControlApi { method = "GET", body?: unknown, ): Promise { + // If we have a token refresh middleware, get a valid token before each request + if (this.tokenRefreshMiddleware) { + this.accessToken = + await this.tokenRefreshMiddleware.getValidAccessToken(); + } + const url = this.controlHost.includes("local") ? `http://${this.controlHost}/api/v1${path}` : `https://${this.controlHost}/v1${path}`; diff --git a/src/services/oauth-client.ts b/src/services/oauth-client.ts new file mode 100644 index 00000000..8d1b2c33 --- /dev/null +++ b/src/services/oauth-client.ts @@ -0,0 +1,275 @@ +import fetch from "node-fetch"; + +export interface OAuthTokens { + accessToken: string; + expiresAt: number; + refreshToken: string; + scope?: string; + tokenType: string; + userEmail?: string; + userId?: string; +} + +export interface OAuthConfig { + clientId: string; + deviceCodeEndpoint: string; + revocationEndpoint: string; + scopes: string[]; + tokenEndpoint: string; +} + +export interface OAuthClientOptions { + controlHost?: string; +} + +export interface DeviceCodeResponse { + deviceCode: string; + expiresIn: number; + interval: number; + userCode: string; + verificationUri: string; + verificationUriComplete: string; +} + +export class OAuthClient { + private config: OAuthConfig; + + constructor(options: OAuthClientOptions = {}) { + this.config = this.getOAuthConfig(options.controlHost); + } + + /** + * Request a device code from the OAuth server (RFC 8628 step 1). + */ + async requestDeviceCode(): Promise { + const params = new URLSearchParams({ + client_id: this.config.clientId, + scope: this.config.scopes.join(" "), + }); + + const response = await fetch(this.config.deviceCodeEndpoint, { + body: params.toString(), + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + method: "POST", + }); + + if (!response.ok) { + const errorBody = await response.text(); + throw new Error( + `Device code request failed (${response.status}): ${errorBody}`, + ); + } + + const data = (await response.json()) as Record; + return { + deviceCode: data.device_code as string, + expiresIn: (data.expires_in as number) || 300, + interval: (data.interval as number) || 5, + userCode: data.user_code as string, + verificationUri: data.verification_uri as string, + verificationUriComplete: data.verification_uri_complete as string, + }; + } + + /** + * Poll for token completion (RFC 8628 step 2). + * Sleeps between requests, respects slow_down, and throws on expiry/denial. + */ + async pollForToken( + deviceCode: string, + interval: number, + expiresIn: number, + ): Promise { + const deadline = Date.now() + expiresIn * 1000; + let currentInterval = interval; + let networkRetries = 0; + const maxNetworkRetries = 3; + + while (Date.now() < deadline) { + await this.sleep(currentInterval * 1000); + + if (Date.now() >= deadline) { + throw new Error("Device code expired"); + } + + let response; + try { + const params = new URLSearchParams({ + client_id: this.config.clientId, + device_code: deviceCode, + grant_type: "urn:ietf:params:oauth:grant-type:device_code", + }); + + response = await fetch(this.config.tokenEndpoint, { + body: params.toString(), + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + method: "POST", + }); + networkRetries = 0; + } catch { + networkRetries++; + if (networkRetries >= maxNetworkRetries) { + throw new Error( + "Network error: failed to reach token endpoint after multiple retries", + ); + } + continue; + } + + if (response.ok) { + const data = (await response.json()) as Record; + return this.parseTokenResponse(data); + } + + let error: string; + try { + const errorData = (await response.json()) as Record; + error = errorData.error as string; + } catch { + throw new Error(`Token polling failed with status ${response.status}`); + } + + if (error === "authorization_pending") { + continue; + } + + if (error === "slow_down") { + currentInterval += 5; + continue; + } + + if (error === "expired_token") { + throw new Error("Device code expired"); + } + + if (error === "access_denied") { + throw new Error("Authorization denied"); + } + + throw new Error(`Token polling failed: ${error}`); + } + + throw new Error("Device code expired"); + } + + /** + * Refresh an access token using a refresh token + */ + async refreshAccessToken(refreshToken: string): Promise { + return this.postForTokens( + { + client_id: this.config.clientId, + grant_type: "refresh_token", + refresh_token: refreshToken, + }, + "Token refresh", + refreshToken, + ); + } + + /** + * Revoke a token (access or refresh) + */ + async revokeToken(token: string): Promise { + const params = new URLSearchParams({ + client_id: this.config.clientId, + token, + }); + + try { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); + await fetch(this.config.revocationEndpoint, { + body: params.toString(), + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + method: "POST", + signal: controller.signal, + }); + clearTimeout(timeout); + } catch { + // Best-effort revocation -- don't block on failure + } + } + + getClientId(): string { + return this.config.clientId; + } + + // --- Private helpers --- + + private getOAuthConfig(controlHost = "ably.com"): OAuthConfig { + const host = controlHost; + const scheme = host.includes("local") ? "http" : "https"; + return { + clientId: "gb-I8-bZRnXs-gF83jOWKQrUxPPWp_ldTfQtgGP0EFg", + deviceCodeEndpoint: `${scheme}://${host}/oauth/authorize_device`, + revocationEndpoint: `${scheme}://${host}/oauth/revoke`, + scopes: ["full_access"], + tokenEndpoint: `${scheme}://${host}/oauth/token`, + }; + } + + private async postForTokens( + params: Record, + operationName: string, + fallbackRefreshToken?: string, + ): Promise { + const body = new URLSearchParams(params); + + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 15_000); + const response = await fetch(this.config.tokenEndpoint, { + body: body.toString(), + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + method: "POST", + signal: controller.signal, + }); + clearTimeout(timeout); + + if (!response.ok) { + const errorBody = await response.text(); + throw new Error( + `${operationName} failed (${response.status}): ${errorBody}`, + ); + } + + const data = (await response.json()) as Record; + return this.parseTokenResponse(data, fallbackRefreshToken); + } + + private parseTokenResponse( + data: Record, + fallbackRefreshToken?: string, + ): OAuthTokens { + const accessToken = + typeof data.access_token === "string" ? data.access_token : undefined; + const refreshToken = + typeof data.refresh_token === "string" + ? data.refresh_token + : fallbackRefreshToken; + + if (!accessToken || !refreshToken) { + throw new Error("Token response missing required fields"); + } + + const expiresIn = (data.expires_in as number) || 3600; + return { + accessToken, + expiresAt: Date.now() + expiresIn * 1000, + refreshToken, + scope: data.scope as string | undefined, + tokenType: (data.token_type as string) || "Bearer", + userEmail: data.user_email as string | undefined, + userId: + typeof data.user_id === "string" + ? data.user_id + : typeof data.user_id === "number" + ? String(data.user_id) + : undefined, + }; + } + + private sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); + } +} diff --git a/src/services/token-refresh-middleware.ts b/src/services/token-refresh-middleware.ts new file mode 100644 index 00000000..f424cecf --- /dev/null +++ b/src/services/token-refresh-middleware.ts @@ -0,0 +1,66 @@ +import type { ConfigManager } from "./config-manager.js"; +import type { OAuthClient } from "./oauth-client.js"; + +export class TokenExpiredError extends Error { + constructor(message: string) { + super(message); + this.name = "TokenExpiredError"; + } +} + +export class TokenRefreshMiddleware { + private pendingRefresh: Promise | undefined; + + constructor( + private configManager: ConfigManager, + private oauthClient: OAuthClient, + ) {} + + async getValidAccessToken(): Promise { + const authMethod = this.configManager.getAuthMethod(); + + // Non-OAuth tokens: return as-is + if (authMethod !== "oauth") { + const token = this.configManager.getAccessToken(); + if (!token) + throw new TokenExpiredError( + "No access token found. Please run 'ably login'.", + ); + return token; + } + + // Not expired: return current token + if (!this.configManager.isAccessTokenExpired()) { + const token = this.configManager.getAccessToken(); + if (token) return token; + } + + // Expired: refresh using refresh token (deduplicate concurrent calls) + if (this.pendingRefresh) { + return this.pendingRefresh; + } + + this.pendingRefresh = this.refreshToken(); + try { + return await this.pendingRefresh; + } finally { + this.pendingRefresh = undefined; + } + } + + private async refreshToken(): Promise { + const tokens = this.configManager.getOAuthTokens(); + if (!tokens?.refreshToken) { + throw new TokenExpiredError( + "OAuth session expired. Please run 'ably login' again.", + ); + } + + const newTokens = await this.oauthClient.refreshAccessToken( + tokens.refreshToken, + ); + const alias = this.configManager.getCurrentAccountAlias() || "default"; + this.configManager.storeOAuthTokens(alias, newTokens); + return newTokens.accessToken; + } +} From debb05b3a93a2f9d5136f0b291fdb3bac7cbda09 Mon Sep 17 00:00:00 2001 From: umair Date: Wed, 15 Apr 2026 20:17:04 +0100 Subject: [PATCH 02/19] Add OAuth login and logout commands --- src/commands/accounts/login.ts | 327 ++++++++++++++++---------------- src/commands/accounts/logout.ts | 30 +++ src/commands/login.ts | 3 +- 3 files changed, 198 insertions(+), 162 deletions(-) diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index f89c53a6..bee5513b 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -1,17 +1,20 @@ -import { Args, Flags } from "@oclif/core"; +import { Flags } from "@oclif/core"; import chalk from "chalk"; import * as readline from "node:readline"; -import open from "open"; +import ora from "ora"; import { ControlBaseCommand } from "../../control-base-command.js"; import { endpointFlag } from "../../flags.js"; import { ControlApi } from "../../services/control-api.js"; +import { OAuthClient, type OAuthTokens } from "../../services/oauth-client.js"; +import { BaseFlags } from "../../types/cli.js"; import { errorMessage } from "../../utils/errors.js"; import { displayLogo } from "../../utils/logo.js"; +import openUrl from "../../utils/open-url.js"; import { formatResource } from "../../utils/output.js"; import { promptForConfirmation } from "../../utils/prompt-confirmation.js"; +import { slugifyAccountName } from "../../utils/slugify.js"; -// Moved function definition outside the class function validateAndGetAlias( input: string, logFn: (msg: string) => void, @@ -42,18 +45,12 @@ function validateAndGetAlias( } export default class AccountsLogin extends ControlBaseCommand { - static override args = { - token: Args.string({ - description: "Access token (if not provided, will prompt for it)", - required: false, - }), - }; - static override description = "Log in to your Ably account"; static override examples = [ "<%= config.bin %> <%= command.id %>", "<%= config.bin %> <%= command.id %> --alias mycompany", + "<%= config.bin %> <%= command.id %> --no-browser", "<%= config.bin %> <%= command.id %> --json", "<%= config.bin %> <%= command.id %> --pretty-json", ]; @@ -72,96 +69,21 @@ export default class AccountsLogin extends ControlBaseCommand { }; public async run(): Promise { - const { args, flags } = await this.parse(AccountsLogin); + const { flags } = await this.parse(AccountsLogin); // Display ASCII art logo if not in JSON mode if (!this.shouldOutputJson(flags)) { displayLogo(this.log.bind(this)); } - let accessToken: string; - if (args.token) { - accessToken = args.token; - } else { - let obtainTokenPath = "https://ably.com/users/access_tokens"; - if (flags["control-host"]) { - if (!this.shouldOutputJson(flags)) { - this.log("Using control host:", flags["control-host"]); - } - - obtainTokenPath = flags["control-host"].includes("local") - ? `http://${flags["control-host"]}/users/access_tokens` - : `https://${flags["control-host"]}/users/access_tokens`; - } - - // Prompt the user to get a token - if (!flags["no-browser"]) { - this.logProgress("Opening browser to get an access token", flags); - - await this.openBrowser(obtainTokenPath); - } else if (!this.shouldOutputJson(flags)) { - this.log(`Please visit ${obtainTokenPath} to create an access token`); - } - - accessToken = await this.promptForToken(); + let oauthTokens: OAuthTokens; + try { + oauthTokens = await this.oauthLogin(flags); + } catch (error) { + this.fail(error, flags, "accountLogin"); } - // If no alias flag provided, prompt the user if they want to provide one - let { alias } = flags; - if (!alias && !this.shouldOutputJson(flags)) { - // Check if the default account already exists - const accounts = this.configManager.listAccounts(); - const hasDefaultAccount = accounts.some( - (account) => account.alias === "default", - ); - - if (hasDefaultAccount) { - // Explain to the user the implications of not providing an alias - this.log("\nYou have not specified an alias for this account."); - this.log( - "If you continue without an alias, your existing default account configuration will be overwritten.", - ); - this.log( - "To maintain multiple account profiles, please provide an alias.", - ); - - // Ask if they want to provide an alias - const shouldProvideAlias = await promptForConfirmation( - "Would you like to provide an alias for this account?", - ); - - if (shouldProvideAlias) { - alias = await this.promptForAlias(); - } else { - alias = "default"; - this.log( - "No alias provided. The default account configuration will be overwritten.", - ); - } - } else { - // No default account exists yet, but still offer to set an alias - this.log("\nYou have not specified an alias for this account."); - this.log( - "Using an alias allows you to maintain multiple account profiles that you can switch between.", - ); - - // Ask if they want to provide an alias - const shouldProvideAlias = await promptForConfirmation( - "Would you like to provide an alias for this account?", - ); - - if (shouldProvideAlias) { - alias = await this.promptForAlias(); - } else { - alias = "default"; - this.log( - "No alias provided. This will be set as your default account.", - ); - } - } - } else if (!alias) { - alias = "default"; - } + const accessToken = oauthTokens.accessToken; try { // Fetch account information @@ -170,15 +92,49 @@ export default class AccountsLogin extends ControlBaseCommand { controlHost: flags["control-host"], }); - const { account, user } = await controlApi.getMe(); + const [{ account: meAccount, user }, accounts] = await Promise.all([ + controlApi.getMe(), + controlApi.getAccounts(), + ]); + + let selectedAccountInfo: { id: string; name: string }; + + if (accounts.length === 0) { + // Fallback to /me response if accounts list is empty + selectedAccountInfo = { id: meAccount.id, name: meAccount.name }; + } else if (accounts.length === 1) { + selectedAccountInfo = accounts[0]!; + } else if (accounts.length > 1 && !this.shouldOutputJson(flags)) { + const picked = + await this.interactiveHelper.selectAccountFromApi(accounts); + selectedAccountInfo = picked ?? accounts[0]!; + } else { + // Multiple accounts in JSON mode: use first + selectedAccountInfo = accounts[0]!; + } + + // Resolve alias AFTER account selection so we can default to account name + let { alias } = flags; + if (!alias && !this.shouldOutputJson(flags)) { + alias = await this.resolveAlias(selectedAccountInfo.name); + } else if (!alias) { + alias = slugifyAccountName(selectedAccountInfo.name); + } - // Store the account information - this.configManager.storeAccount(accessToken, alias, { - accountId: account.id, - accountName: account.name, - tokenId: "unknown", // Token ID is not returned by getMe(), would need additional API if needed - userEmail: user.email, - }); + // Store OAuth tokens (include user email from /me response) + this.configManager.storeOAuthTokens( + alias, + { ...oauthTokens, userEmail: user.email }, + { + accountId: selectedAccountInfo.id, + accountName: selectedAccountInfo.name, + }, + ); + + // Persist control host so other commands (like switch) can use it + if (flags["control-host"]) { + this.configManager.setAccountControlHost(alias, flags["control-host"]); + } // Switch to this account this.configManager.switchAccount(alias); @@ -234,13 +190,12 @@ export default class AccountsLogin extends ControlBaseCommand { const app = await controlApi.createApp({ name: appName, - tlsOnly: true, // Default to true for security + tlsOnly: true, }); selectedApp = app; - isAutoSelected = true; // Consider this auto-selected since it's the only one + isAutoSelected = true; - // Set as current app this.configManager.setCurrentApp(app.id); this.configManager.storeAppInfo(app.id, { appName: app.name }); @@ -249,13 +204,10 @@ export default class AccountsLogin extends ControlBaseCommand { this.warn( `Failed to create app: ${createError instanceof Error ? createError.message : String(createError)}`, ); - // Continue with login even if app creation fails } } } - // If apps.length === 0 and JSON mode, or user declined to create app, do nothing } catch (error) { - // Don't fail login if app fetching fails, just log for debugging if (!this.shouldOutputJson(flags)) { this.warn(`Could not fetch apps: ${errorMessage(error)}`); } @@ -277,7 +229,6 @@ export default class AccountsLogin extends ControlBaseCommand { keyName: selectedKey.name || "Unnamed key", }); } else if (keys.length > 1) { - // Prompt user to select a key when multiple exist this.log("\nSelect an API key to use:"); selectedKey = await this.interactiveHelper.selectKey( @@ -292,9 +243,7 @@ export default class AccountsLogin extends ControlBaseCommand { }); } } - // If keys.length === 0, continue without key (should be rare for newly created apps) } catch (keyError) { - // Don't fail login if key fetching fails this.warn( `Could not fetch API keys: ${keyError instanceof Error ? keyError.message : String(keyError)}`, ); @@ -304,6 +253,7 @@ export default class AccountsLogin extends ControlBaseCommand { if (this.shouldOutputJson(flags)) { const accountData: { alias: string; + authMethod: "oauth"; id: string; name: string; user: { email: string }; @@ -319,25 +269,24 @@ export default class AccountsLogin extends ControlBaseCommand { }; } = { alias, - id: account.id, - name: account.name, + authMethod: "oauth", + id: selectedAccountInfo.id, + name: selectedAccountInfo.name, user: { email: user.email, }, }; - if (selectedApp) { accountData.app = { + autoSelected: isAutoSelected, id: selectedApp.id, name: selectedApp.name, - autoSelected: isAutoSelected, }; - if (selectedKey) { accountData.key = { + autoSelected: isKeyAutoSelected, id: selectedKey.id, name: selectedKey.name || "Unnamed key", - autoSelected: isKeyAutoSelected, }; } } @@ -351,62 +300,134 @@ export default class AccountsLogin extends ControlBaseCommand { this.log(`Account ${formatResource(alias)} is now the current account`); } - this.logSuccessMessage( - `Successfully logged in to ${formatResource(account.name)} (account ID: ${chalk.greenBright(account.id)}).`, - flags, - ); + if (this.shouldOutputJson(flags)) { + // logJsonResult already emitted above + } else { + this.logSuccessMessage( + `Successfully logged in to ${formatResource(selectedAccountInfo.name)} (account ID: ${formatResource(selectedAccountInfo.id)}).`, + flags, + ); - if (selectedApp) { - const message = isAutoSelected - ? `Automatically selected app: ${formatResource(selectedApp.name)} (${selectedApp.id}).` - : `Selected app: ${formatResource(selectedApp.name)} (${selectedApp.id}).`; - this.logSuccessMessage(message, flags); - } + this.logToStderr("Authenticated via OAuth (token auto-refreshes)."); + + if (selectedApp) { + const message = isAutoSelected + ? `Automatically selected app: ${formatResource(selectedApp.name)} (${selectedApp.id}).` + : `Selected app: ${formatResource(selectedApp.name)} (${selectedApp.id}).`; + this.logSuccessMessage(message, flags); + } - if (selectedKey) { - const keyMessage = isKeyAutoSelected - ? `Automatically selected API key: ${formatResource(selectedKey.name || "Unnamed key")} (${selectedKey.id}).` - : `Selected API key: ${formatResource(selectedKey.name || "Unnamed key")} (${selectedKey.id}).`; - this.logSuccessMessage(keyMessage, flags); + if (selectedKey) { + const keyMessage = isKeyAutoSelected + ? `Automatically selected API key: ${formatResource(selectedKey.name || "Unnamed key")} (${selectedKey.id}).` + : `Selected API key: ${formatResource(selectedKey.name || "Unnamed key")} (${selectedKey.id}).`; + this.logSuccessMessage(keyMessage, flags); + } } } catch (error) { this.fail(error, flags, "accountLogin"); } } - private async openBrowser(url: string): Promise { + private async oauthLogin(flags: BaseFlags): Promise { + const oauthClient = new OAuthClient({ + controlHost: flags["control-host"], + }); + + const deviceResponse = await oauthClient.requestDeviceCode(); + + if (this.shouldOutputJson(flags)) { + this.logJsonEvent( + { + status: "awaiting_authorization", + userCode: deviceResponse.userCode, + verificationUri: deviceResponse.verificationUri, + verificationUriComplete: deviceResponse.verificationUriComplete, + }, + flags, + ); + } else { + this.log(""); + this.log( + ` Your authorization code: ${chalk.bold.cyan(deviceResponse.userCode)}`, + ); + this.log(""); + this.log( + ` Visit: ${chalk.underline(deviceResponse.verificationUriComplete)}`, + ); + this.log(""); + } + + if (!flags["no-browser"]) { + await openUrl(deviceResponse.verificationUriComplete, this); + } else if (!this.shouldOutputJson(flags)) { + this.log("Open the URL above in your browser to authorize."); + } + + const spinner = this.shouldOutputJson(flags) + ? undefined + : ora("Waiting for authorization...").start(); + try { - // Use the 'open' package for cross-platform browser opening - // This handles platform differences safely and avoids shell injection - await open(url); + const tokens = await oauthClient.pollForToken( + deviceResponse.deviceCode, + deviceResponse.interval, + deviceResponse.expiresIn, + ); + + spinner?.succeed("Authentication successful!"); + return tokens; } catch (error) { - this.warn(`Failed to open browser: ${String(error)}`); - this.log(`Please visit ${url} manually to create an access token`); + spinner?.fail("Authentication failed"); + throw error; + } + } + + private async resolveAlias(accountName: string): Promise { + const defaultAlias = slugifyAccountName(accountName); + const existingAccounts = this.configManager.listAccounts(); + const aliasExists = existingAccounts.some((a) => a.alias === defaultAlias); + + if (aliasExists) { + this.log( + `\nAn account with alias "${defaultAlias}" already exists and will be overwritten.`, + ); + const shouldCustomize = await promptForConfirmation( + "Would you like to use a different alias?", + ); + if (shouldCustomize) { + return this.promptForAlias(defaultAlias); + } + return defaultAlias; } + + // New account — use the derived alias without prompting + return defaultAlias; } - private promptForAlias(): Promise { + private promptForAlias(defaultAlias: string): Promise { const rl = readline.createInterface({ input: process.stdin, output: process.stdout, }); - // Pass this.log as the logging function to the external validator const logFn = this.log.bind(this); return new Promise((resolve) => { const askForAlias = () => { rl.question( - 'Enter an alias for this account (e.g. "dev", "production", "personal"): ', - (alias) => { - // Use the external validator function, passing the log function - const validatedAlias = validateAndGetAlias(alias, logFn); + `Enter an alias for this account [${defaultAlias}]: `, + (input) => { + // Accept default on empty input + if (!input.trim()) { + rl.close(); + resolve(defaultAlias); + return; + } - if (validatedAlias === null) { - if (!alias.trim()) { - logFn("Error: Alias cannot be empty"); // Use logFn here too - } + const validatedAlias = validateAndGetAlias(input, logFn); + if (validatedAlias === null) { askForAlias(); } else { rl.close(); @@ -444,18 +465,4 @@ export default class AccountsLogin extends ControlBaseCommand { askForAppName(); }); } - - private promptForToken(): Promise { - const rl = readline.createInterface({ - input: process.stdin, - output: process.stdout, - }); - - return new Promise((resolve) => { - rl.question("\nEnter your access token: ", (token) => { - rl.close(); - resolve(token.trim()); - }); - }); - } } diff --git a/src/commands/accounts/logout.ts b/src/commands/accounts/logout.ts index a9b03a7a..a267f2a6 100644 --- a/src/commands/accounts/logout.ts +++ b/src/commands/accounts/logout.ts @@ -2,6 +2,7 @@ import { Args } from "@oclif/core"; import { ControlBaseCommand } from "../../control-base-command.js"; import { forceFlag } from "../../flags.js"; +import { OAuthClient } from "../../services/oauth-client.js"; import { promptForConfirmation } from "../../utils/prompt-confirmation.js"; export default class AccountsLogout extends ControlBaseCommand { @@ -73,6 +74,35 @@ export default class AccountsLogout extends ControlBaseCommand { } } + // Revoke OAuth tokens if this is an OAuth account + if (this.configManager.getAuthMethod(targetAlias) === "oauth") { + const oauthTokens = this.configManager.getOAuthTokens(targetAlias); + if (oauthTokens) { + const targetAccount = this.configManager.getCurrentAccount(); + const oauthHost = flags["control-host"] || targetAccount?.controlHost; + const oauthClient = new OAuthClient({ + controlHost: oauthHost, + }); + // Best-effort revocation with timeout -- don't block logout + const revokeWithTimeout = (token: string, timeoutMs = 5000) => + Promise.race([ + oauthClient.revokeToken(token), + new Promise((resolve) => setTimeout(resolve, timeoutMs)), + ]); + await Promise.all([ + revokeWithTimeout(oauthTokens.accessToken), + revokeWithTimeout(oauthTokens.refreshToken), + ]).catch((error) => { + this.logCliEvent( + flags, + "accountLogout", + "revocationFailed", + `OAuth token revocation failed: ${error instanceof Error ? error.message : String(error)}`, + ); + }); + } + } + // Remove the account const success = this.configManager.removeAccount(targetAlias); diff --git a/src/commands/login.ts b/src/commands/login.ts index 8a885583..559308da 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -3,14 +3,13 @@ import { Command } from "@oclif/core"; import AccountsLogin from "./accounts/login.js"; export default class Login extends Command { - static override args = AccountsLogin.args; - static override description = 'Log in to your Ably account (alias for "ably accounts login")'; static override examples = [ "<%= config.bin %> <%= command.id %>", "<%= config.bin %> <%= command.id %> --alias mycompany", + "<%= config.bin %> <%= command.id %> --json", ]; static override flags = AccountsLogin.flags; From f972132b060c0c08b81b423d66e580ec414061a9 Mon Sep 17 00:00:00 2001 From: umair Date: Wed, 15 Apr 2026 20:17:07 +0100 Subject: [PATCH 03/19] Add multi-account selection and switching --- src/commands/accounts/switch.ts | 192 ++++++++++++++++++++++++++--- src/services/interactive-helper.ts | 59 +++++++-- src/utils/slugify.ts | 17 +++ 3 files changed, 243 insertions(+), 25 deletions(-) create mode 100644 src/utils/slugify.ts diff --git a/src/commands/accounts/switch.ts b/src/commands/accounts/switch.ts index 73cff5b3..18da5cea 100644 --- a/src/commands/accounts/switch.ts +++ b/src/commands/accounts/switch.ts @@ -1,9 +1,12 @@ import { Args } from "@oclif/core"; +import chalk from "chalk"; +import inquirer from "inquirer"; import { ControlBaseCommand } from "../../control-base-command.js"; import { endpointFlag } from "../../flags.js"; -import { ControlApi } from "../../services/control-api.js"; +import { ControlApi, type AccountSummary } from "../../services/control-api.js"; import { formatResource } from "../../utils/output.js"; +import { slugifyAccountName } from "../../utils/slugify.js"; export default class AccountsSwitch extends ControlBaseCommand { static override args = { @@ -30,10 +33,9 @@ export default class AccountsSwitch extends ControlBaseCommand { public async run(): Promise { const { args, flags } = await this.parse(AccountsSwitch); - // Get available accounts - const accounts = this.configManager.listAccounts(); + const localAccounts = this.configManager.listAccounts(); - if (accounts.length === 0) { + if (localAccounts.length === 0) { if (this.shouldOutputJson(flags)) { this.fail( 'No accounts configured. Use "ably accounts login" to add an account.', @@ -50,18 +52,18 @@ export default class AccountsSwitch extends ControlBaseCommand { // If alias is provided, switch directly if (args.accountAlias) { - await this.switchToAccount(args.accountAlias, accounts, flags); + await this.switchToLocalAccount(args.accountAlias, localAccounts, flags); return; } - // Otherwise, show interactive selection if not in JSON mode + // JSON mode requires an explicit alias if (this.shouldOutputJson(flags)) { this.fail( "No account alias provided. Please specify an account alias to switch to.", flags, "accountSwitch", { - availableAccounts: accounts.map(({ account, alias }) => ({ + availableAccounts: localAccounts.map(({ account, alias }) => ({ alias, id: account.accountId, name: account.accountName, @@ -70,17 +72,177 @@ export default class AccountsSwitch extends ControlBaseCommand { ); } - this.log("Select an account to switch to:"); - const selectedAccount = await this.interactiveHelper.selectAccount(); + // Interactive mode: show local aliases + remote accounts + const selected = await this.interactiveSwitch(localAccounts, flags); - if (selectedAccount) { - await this.switchToAccount(selectedAccount.alias, accounts, flags); - } else { + if (!selected) { this.logWarning("Account switch cancelled.", flags); } } - private async switchToAccount( + private async interactiveSwitch( + localAccounts: Array<{ + account: { + accountId?: string; + accountName?: string; + userEmail?: string; + }; + alias: string; + }>, + flags: Record, + ): Promise { + const currentAlias = this.configManager.getCurrentAccountAlias(); + + // Try to fetch remote accounts using the current token + let remoteAccounts: AccountSummary[] = []; + const accessToken = this.configManager.getAccessToken(); + if (accessToken) { + try { + const currentAccount = this.configManager.getCurrentAccount(); + const controlHost = + (flags["control-host"] as string | undefined) || + currentAccount?.controlHost; + const controlApi = new ControlApi({ + accessToken, + controlHost, + }); + remoteAccounts = await controlApi.getAccounts(); + } catch { + // Couldn't fetch remote accounts — fall back to local only + } + } + + // Build local account IDs set for deduplication + const localAccountIds = new Set( + localAccounts.map((a) => a.account.accountId).filter(Boolean), + ); + + // Remote accounts not already configured locally + const remoteOnly = remoteAccounts.filter((r) => !localAccountIds.has(r.id)); + + type Choice = { + name: string; + value: + | { type: "local"; alias: string } + | { type: "remote"; account: AccountSummary }; + }; + + const choices: Array = []; + + // Local accounts section + if (localAccounts.length > 0) { + choices.push(new inquirer.Separator("── Local accounts ──")); + for (const { account, alias } of localAccounts) { + const isCurrent = alias === currentAlias; + const name = account.accountName || account.accountId || "Unknown"; + const label = `${isCurrent ? "* " : " "}${alias} ${chalk.dim(`(${name})`)}`; + choices.push({ name: label, value: { type: "local", alias } }); + } + } + + // Remote-only accounts section + if (remoteOnly.length > 0) { + choices.push( + new inquirer.Separator("── Other accounts (no login required) ──"), + ); + for (const account of remoteOnly) { + const label = ` ${account.name} ${chalk.dim(`(${account.id})`)}`; + choices.push({ name: label, value: { type: "remote", account } }); + } + } + + if (choices.length === 0) { + this.log("No accounts available."); + return false; + } + + const { selected } = (await inquirer.prompt([ + { + choices, + message: "Select an account:", + name: "selected", + type: "list", + }, + ])) as { + selected: + | { type: "local"; alias: string } + | { type: "remote"; account: AccountSummary }; + }; + + if (selected.type === "local") { + const validAccounts = localAccounts + .filter((a) => a.account.accountId && a.account.accountName) + .map((a) => ({ + account: { + accountId: a.account.accountId!, + accountName: a.account.accountName!, + }, + alias: a.alias, + })); + await this.switchToLocalAccount(selected.alias, validAccounts, flags); + return true; + } + + // Remote account — create a local alias using the current token + this.addAndSwitchToRemoteAccount(selected.account, flags); + return true; + } + + private addAndSwitchToRemoteAccount( + remoteAccount: AccountSummary, + flags: Record, + ): void { + const currentAlias = this.configManager.getCurrentAccountAlias(); + if (!currentAlias) { + this.fail( + "No current account to copy credentials from.", + flags, + "accountSwitch", + ); + } + + const oauthTokens = this.configManager.getOAuthTokens(currentAlias); + if (!oauthTokens) { + this.fail( + "Current account does not use OAuth. Please log in with the target account directly.", + flags, + "accountSwitch", + ); + } + + const currentAccount = this.configManager.getCurrentAccount(); + const newAlias = slugifyAccountName(remoteAccount.name); + + // Store the new alias with the same OAuth tokens but different account info + this.configManager.storeOAuthTokens( + newAlias, + { + ...oauthTokens, + userEmail: currentAccount?.userEmail, + }, + { + accountId: remoteAccount.id, + accountName: remoteAccount.name, + }, + ); + + // Carry over control host from the source account + if (currentAccount?.controlHost) { + this.configManager.setAccountControlHost( + newAlias, + currentAccount.controlHost, + ); + } + + this.configManager.switchAccount(newAlias); + + this.log( + `Switched to account: ${formatResource(remoteAccount.name)} (${remoteAccount.id})`, + ); + this.log(`Saved as alias: ${formatResource(newAlias)}`); + } + + private async switchToLocalAccount( alias: string, accounts: Array<{ account: { accountId: string; accountName: string }; @@ -88,7 +250,6 @@ export default class AccountsSwitch extends ControlBaseCommand { }>, flags: Record, ): Promise { - // Check if account exists const accountExists = accounts.some((account) => account.alias === alias); if (!accountExists) { @@ -106,7 +267,6 @@ export default class AccountsSwitch extends ControlBaseCommand { ); } - // Switch to the account this.configManager.switchAccount(alias); // Store custom endpoint if provided @@ -154,7 +314,7 @@ export default class AccountsSwitch extends ControlBaseCommand { this.log(`User: ${user.email}`); } } catch { - // The account switch already happened above (line 109), so this is non-fatal. + // The account switch already happened above, so this is non-fatal. // Warn the user but still report success with a warning field. const warningMessage = "Access token may have expired or is invalid. The account was switched, but token verification failed."; diff --git a/src/services/interactive-helper.ts b/src/services/interactive-helper.ts index daf153ee..2b5e36f0 100644 --- a/src/services/interactive-helper.ts +++ b/src/services/interactive-helper.ts @@ -1,13 +1,15 @@ import inquirer from "inquirer"; import type { ConfigManager, AccountConfig } from "./config-manager.js"; -import type { App, ControlApi, Key } from "./control-api.js"; +import type { AccountSummary, App, ControlApi, Key } from "./control-api.js"; export interface InteractiveHelperOptions { + log?: (msg: string) => void; logErrors?: boolean; } export class InteractiveHelper { private configManager: ConfigManager; + private log: (msg: string) => void; private logErrors: boolean; constructor( @@ -15,6 +17,7 @@ export class InteractiveHelper { options: InteractiveHelperOptions = {}, ) { this.configManager = configManager; + this.log = options.log ?? console.log; this.logErrors = options.logErrors !== false; // Default to true } @@ -46,7 +49,7 @@ export class InteractiveHelper { const currentAlias = this.configManager.getCurrentAccountAlias(); if (accounts.length === 0) { - console.log( + this.log( 'No accounts configured. Use "ably accounts login" to add an account.', ); return null; @@ -74,7 +77,43 @@ export class InteractiveHelper { return selectedAccount; } catch (error) { if (this.logErrors) { - console.error("Error selecting account:", error); + this.log( + `Error selecting account: ${error instanceof Error ? error.message : String(error)}`, + ); + } + return null; + } + } + + /** + * Interactively select an account from API results (multi-account OAuth flow) + */ + async selectAccountFromApi( + accounts: AccountSummary[], + ): Promise { + try { + if (accounts.length === 0) { + return null; + } + + const { selectedAccount } = (await inquirer.prompt([ + { + choices: accounts.map((account) => ({ + name: `${account.name} (${account.id})`, + value: account, + })), + message: "Select an account:", + name: "selectedAccount", + type: "list", + }, + ])) as { selectedAccount: AccountSummary }; + + return selectedAccount; + } catch (error) { + if (this.logErrors) { + this.log( + `Error selecting account: ${error instanceof Error ? error.message : String(error)}`, + ); } return null; } @@ -88,9 +127,7 @@ export class InteractiveHelper { const apps = await controlApi.listApps(); if (apps.length === 0) { - console.log( - 'No apps found. Create an app with "ably apps create" first.', - ); + this.log('No apps found. Create an app with "ably apps create" first.'); return null; } @@ -109,7 +146,9 @@ export class InteractiveHelper { return selectedApp; } catch (error) { if (this.logErrors) { - console.error("Error fetching apps:", error); + this.log( + `Error fetching apps: ${error instanceof Error ? error.message : String(error)}`, + ); } return null; } @@ -123,7 +162,7 @@ export class InteractiveHelper { const keys = await controlApi.listKeys(appId); if (keys.length === 0) { - console.log("No keys found for this app."); + this.log("No keys found for this app."); return null; } @@ -142,7 +181,9 @@ export class InteractiveHelper { return selectedKey; } catch (error) { if (this.logErrors) { - console.error("Error fetching keys:", error); + this.log( + `Error fetching keys: ${error instanceof Error ? error.message : String(error)}`, + ); } return null; } diff --git a/src/utils/slugify.ts b/src/utils/slugify.ts new file mode 100644 index 00000000..fd85a96d --- /dev/null +++ b/src/utils/slugify.ts @@ -0,0 +1,17 @@ +/** + * Convert an account name to a valid alias slug. + * Rules: lowercase, alphanumeric + dashes, must start with a letter. + */ +export function slugifyAccountName(name: string): string { + const slug = name + .toLowerCase() + .replaceAll(/[^a-z\d]+/g, "-") + .replaceAll(/^-+|-+$/g, ""); + if (!slug) { + return "default"; + } + if (!/^[a-z]/.test(slug)) { + return `account-${slug}`; + } + return slug; +} From 2cde4ba607be7bc755420710c2609cb2c92efa3a Mon Sep 17 00:00:00 2001 From: umair Date: Wed, 15 Apr 2026 20:17:11 +0100 Subject: [PATCH 04/19] Add tests for OAuth login flow --- test/helpers/mock-config-manager.ts | 148 +++++++- test/unit/commands/accounts/login.test.ts | 296 ++++++++++++--- test/unit/commands/accounts/logout.test.ts | 173 ++++++++- test/unit/commands/accounts/switch.test.ts | 72 ++++ test/unit/services/oauth-client.test.ts | 346 ++++++++++++++++++ .../services/shared-oauth-session.test.ts | 215 +++++++++++ .../services/token-refresh-middleware.test.ts | 187 ++++++++++ test/unit/utils/slugify.test.ts | 44 +++ 8 files changed, 1431 insertions(+), 50 deletions(-) create mode 100644 test/unit/services/oauth-client.test.ts create mode 100644 test/unit/services/shared-oauth-session.test.ts create mode 100644 test/unit/services/token-refresh-middleware.test.ts create mode 100644 test/unit/utils/slugify.test.ts diff --git a/test/helpers/mock-config-manager.ts b/test/helpers/mock-config-manager.ts index a7a4aee9..14f81438 100644 --- a/test/helpers/mock-config-manager.ts +++ b/test/helpers/mock-config-manager.ts @@ -204,6 +204,7 @@ export class MockConfigManager implements ConfigManager { */ public clearAccounts(): void { this.config.accounts = {}; + delete this.config.oauthSessions; if (this.config.current) { delete this.config.current.account; } @@ -215,12 +216,18 @@ export class MockConfigManager implements ConfigManager { delete this.config.helpContext; } - public getAccessToken(alias?: string): string | undefined { - if (alias) { - return this.config.accounts[alias]?.accessToken; - } - const currentAccount = this.getCurrentAccount(); - return currentAccount?.accessToken; + public getAccessToken(): string | undefined { + const account = this.getCurrentAccount(); + if (!account) return undefined; + + // OAuth accounts read from the shared session + const session = account.oauthSessionKey + ? this.config.oauthSessions?.[account.oauthSessionKey] + : undefined; + if (session) return session.accessToken; + + // Fallback: pre-OAuth configs store the token directly on the account + return account.accessToken; } public getApiKey(appId?: string): string | undefined { @@ -331,12 +338,27 @@ export class MockConfigManager implements ConfigManager { } public removeAccount(alias: string): boolean { - if (!this.config.accounts[alias]) { + const account = this.config.accounts[alias]; + if (!account) { return false; } + const sessionKey = account.oauthSessionKey; delete this.config.accounts[alias]; + // Clean up orphaned OAuth session entry + if (sessionKey && this.config.oauthSessions?.[sessionKey]) { + const stillReferenced = Object.values(this.config.accounts).some( + (a) => a.oauthSessionKey === sessionKey, + ); + if (!stillReferenced) { + delete this.config.oauthSessions[sessionKey]; + if (Object.keys(this.config.oauthSessions).length === 0) { + delete this.config.oauthSessions; + } + } + } + if (this.config.current?.account === alias) { delete this.config.current.account; } @@ -482,6 +504,113 @@ export class MockConfigManager implements ConfigManager { ); } + public storeOAuthTokens( + alias: string, + tokens: { + accessToken: string; + refreshToken: string; + expiresAt: number; + scope?: string; + userId?: string; + userEmail?: string; + }, + accountInfo?: { + accountId?: string; + accountName?: string; + }, + ): void { + const userEmail = + tokens.userEmail ?? this.config.accounts[alias]?.userEmail ?? ""; + const sessionKey = userEmail.toLowerCase() || alias; + + // Create/update the shared OAuth session + if (!this.config.oauthSessions) { + this.config.oauthSessions = {}; + } + + this.config.oauthSessions[sessionKey] = { + accessToken: tokens.accessToken, + accessTokenExpiresAt: tokens.expiresAt, + oauthScope: tokens.scope, + refreshToken: tokens.refreshToken, + }; + + // Store account metadata and reference the OAuth session + this.config.accounts[alias] = { + ...this.config.accounts[alias], + accountId: + accountInfo?.accountId ?? this.config.accounts[alias]?.accountId, + accountName: + accountInfo?.accountName ?? this.config.accounts[alias]?.accountName, + apps: this.config.accounts[alias]?.apps || {}, + authMethod: "oauth", + currentAppId: this.config.accounts[alias]?.currentAppId, + oauthSessionKey: sessionKey, + userEmail, + }; + + if (!this.config.current || !this.config.current.account) { + this.config.current = { account: alias }; + } + } + + public getOAuthTokens(alias?: string): + | { + accessToken: string; + refreshToken: string; + expiresAt: number; + } + | undefined { + const account = alias + ? this.config.accounts[alias] + : this.getCurrentAccount(); + if (!account || account.authMethod !== "oauth") return undefined; + + const session = account.oauthSessionKey + ? this.config.oauthSessions?.[account.oauthSessionKey] + : undefined; + if (!session) return undefined; + + return { + accessToken: session.accessToken, + expiresAt: session.accessTokenExpiresAt, + refreshToken: session.refreshToken, + }; + } + + public isAccessTokenExpired(): boolean { + const account = this.getCurrentAccount(); + if (!account) return false; + + // OAuth accounts read expiry from the shared session; + // falls back to account-level field for pre-OAuth configs + const session = account.oauthSessionKey + ? this.config.oauthSessions?.[account.oauthSessionKey] + : undefined; + const expiresAt = + session?.accessTokenExpiresAt ?? account.accessTokenExpiresAt; + if (!expiresAt) return false; + + return Date.now() >= expiresAt - 60_000; + } + + public getAuthMethod(alias?: string): "oauth" | undefined { + const account = alias + ? this.config.accounts[alias] + : this.getCurrentAccount(); + return account?.authMethod; + } + + public getAliasesForOAuthSession(alias: string): string[] { + const account = this.config.accounts[alias]; + if (!account?.oauthSessionKey) return [alias]; + + const sessionKey = account.oauthSessionKey; + return Object.entries(this.config.accounts) + .filter(([, acc]) => acc.oauthSessionKey === sessionKey) + .map(([a]) => a); + } + public switchAccount(alias: string): boolean { if (!this.config.accounts[alias]) { return false; @@ -494,6 +623,11 @@ export class MockConfigManager implements ConfigManager { this.config.current.account = alias; return true; } + + public setAccountControlHost(alias: string, controlHost: string): void { + if (!this.config.accounts[alias]) return; + this.config.accounts[alias].controlHost = controlHost; + } } /** diff --git a/test/unit/commands/accounts/login.test.ts b/test/unit/commands/accounts/login.test.ts index 096c87c9..98e52a01 100644 --- a/test/unit/commands/accounts/login.test.ts +++ b/test/unit/commands/accounts/login.test.ts @@ -13,8 +13,38 @@ import { } from "../../../helpers/standard-tests.js"; import { parseNdjsonLines } from "../../../helpers/ndjson.js"; +const mockOAuthAccessToken = "oauth_access_token_12345"; +const mockRefreshToken = "oauth_refresh_token_67890"; + +/** + * Mock the OAuth device flow endpoints (device code + token polling). + * The device code endpoint returns immediately, and the token endpoint + * returns a successful token on the first poll. + */ +function mockOAuthDeviceFlow(host = "ably.com") { + const scheme = host.includes("local") ? "http" : "https"; + + nock(`${scheme}://${host}`) + .post("/oauth/authorize_device") + .reply(200, { + device_code: "dc_test", + expires_in: 300, + interval: 0.01, + user_code: "TEST-CODE", + verification_uri: `${scheme}://${host}/device`, + verification_uri_complete: `${scheme}://${host}/device?user_code=TEST-CODE`, + }); + + nock(`${scheme}://${host}`).post("/oauth/token").reply(200, { + access_token: mockOAuthAccessToken, + expires_in: 3600, + refresh_token: mockRefreshToken, + scope: "full_access", + token_type: "Bearer", + }); +} + describe("accounts:login command", () => { - const mockAccessToken = "test_access_token_12345"; const mockAccountId = "test-account-id"; beforeEach(() => { @@ -33,6 +63,8 @@ describe("accounts:login command", () => { describe("functionality", () => { it("should output JSON format when --json flag is used", async () => { + mockOAuthDeviceFlow(); + // Mock the /me endpoint nockControl() .get("/v1/me") @@ -41,11 +73,16 @@ describe("accounts:login command", () => { user: { email: "test@example.com" }, }); + // Mock the /me/accounts endpoint + nock("https://control.ably.net") + .get("/v1/me/accounts") + .reply(200, [{ id: mockAccountId, name: "Test Account" }]); + // Mock the apps list endpoint nockControl().get(`/v1/accounts/${mockAccountId}/apps`).reply(200, []); const { stdout } = await runCommand( - ["accounts:login", mockAccessToken, "--json"], + ["accounts:login", "--json"], import.meta.url, ); @@ -57,22 +94,31 @@ describe("accounts:login command", () => { const account = result.account as Record; expect(account).toHaveProperty("id", mockAccountId); expect(account).toHaveProperty("name", "Test Account"); + expect(account).toHaveProperty("authMethod", "oauth"); const user = account.user as Record; expect(user).toHaveProperty("email", "test@example.com"); // Verify config was updated correctly via mock const mock = getMockConfigManager(); const config = mock.getConfig(); - expect(config.current?.account).toBe("default"); - expect(config.accounts["default"]).toBeDefined(); - expect(config.accounts["default"].accessToken).toBe(mockAccessToken); - expect(config.accounts["default"].accountId).toBe(mockAccountId); - expect(config.accounts["default"].accountName).toBe("Test Account"); - expect(config.accounts["default"].userEmail).toBe("test@example.com"); + expect(config.current?.account).toBe("test-account"); + expect(config.accounts["test-account"]).toBeDefined(); + expect(config.accounts["test-account"].accountId).toBe(mockAccountId); + expect(config.accounts["test-account"].accountName).toBe("Test Account"); + expect(config.accounts["test-account"].authMethod).toBe("oauth"); + expect(config.accounts["test-account"].oauthSessionKey).toBeDefined(); + const session = + config.oauthSessions?.[ + config.accounts["test-account"].oauthSessionKey! + ]; + expect(session?.accessToken).toBe(mockOAuthAccessToken); + expect(session?.refreshToken).toBe(mockRefreshToken); + expect(session?.accessTokenExpiresAt).toBeDefined(); }); it("should include alias in JSON response when --alias flag is provided", async () => { const customAlias = "mycompany"; + mockOAuthDeviceFlow(); // Mock the /me endpoint nockControl() @@ -82,11 +128,16 @@ describe("accounts:login command", () => { user: { email: "test@example.com" }, }); + // Mock the /me/accounts endpoint + nock("https://control.ably.net") + .get("/v1/me/accounts") + .reply(200, [{ id: mockAccountId, name: "Test Account" }]); + // Mock the apps list endpoint nockControl().get(`/v1/accounts/${mockAccountId}/apps`).reply(200, []); const { stdout } = await runCommand( - ["accounts:login", mockAccessToken, "--alias", customAlias, "--json"], + ["accounts:login", "--alias", customAlias, "--json"], import.meta.url, ); @@ -102,15 +153,18 @@ describe("accounts:login command", () => { const config = mock.getConfig(); expect(config.current?.account).toBe(customAlias); expect(config.accounts[customAlias]).toBeDefined(); - expect(config.accounts[customAlias].accessToken).toBe(mockAccessToken); + expect(config.accounts[customAlias].oauthSessionKey).toBeDefined(); + const session = + config.oauthSessions?.[config.accounts[customAlias].oauthSessionKey!]; + expect(session?.accessToken).toBe(mockOAuthAccessToken); expect(config.accounts[customAlias].accountId).toBe(mockAccountId); expect(config.accounts[customAlias].accountName).toBe("Test Account"); - expect(config.accounts[customAlias].userEmail).toBe("test@example.com"); }); it("should include app info when single app is auto-selected", async () => { const mockAppId = "app-123"; const mockAppName = "My Only App"; + mockOAuthDeviceFlow(); // Mock the /me endpoint twice - once for initial login, once for listApps nockControl() @@ -121,6 +175,11 @@ describe("accounts:login command", () => { user: { email: "test@example.com" }, }); + // Mock the /me/accounts endpoint + nock("https://control.ably.net") + .get("/v1/me/accounts") + .reply(200, [{ id: mockAccountId, name: "Test Account" }]); + // Mock the apps list endpoint with single app nockControl() .get(`/v1/accounts/${mockAccountId}/apps`) @@ -129,7 +188,7 @@ describe("accounts:login command", () => { ]); const { stdout } = await runCommand( - ["accounts:login", mockAccessToken, "--json"], + ["accounts:login", "--json"], import.meta.url, ); @@ -145,18 +204,25 @@ describe("accounts:login command", () => { // Verify config was written with app info via mock const mock = getMockConfigManager(); const config = mock.getConfig(); - expect(config.current?.account).toBe("default"); - expect(config.accounts["default"]).toBeDefined(); - expect(config.accounts["default"].accessToken).toBe(mockAccessToken); - expect(config.accounts["default"].accountId).toBe(mockAccountId); - expect(config.accounts["default"].currentAppId).toBe(mockAppId); - expect(config.accounts["default"].apps?.[mockAppId]).toBeDefined(); - expect(config.accounts["default"].apps?.[mockAppId]?.appName).toBe( + expect(config.current?.account).toBe("test-account"); + expect(config.accounts["test-account"]).toBeDefined(); + expect(config.accounts["test-account"].oauthSessionKey).toBeDefined(); + const session = + config.oauthSessions?.[ + config.accounts["test-account"].oauthSessionKey! + ]; + expect(session?.accessToken).toBe(mockOAuthAccessToken); + expect(config.accounts["test-account"].accountId).toBe(mockAccountId); + expect(config.accounts["test-account"].currentAppId).toBe(mockAppId); + expect(config.accounts["test-account"].apps?.[mockAppId]).toBeDefined(); + expect(config.accounts["test-account"].apps?.[mockAppId]?.appName).toBe( mockAppName, ); }); it("should not include app info when multiple apps exist (no interactive selection in JSON mode)", async () => { + mockOAuthDeviceFlow(); + // Mock the /me endpoint twice - once for initial login, once for listApps nockControl() .get("/v1/me") @@ -166,6 +232,11 @@ describe("accounts:login command", () => { user: { email: "test@example.com" }, }); + // Mock the /me/accounts endpoint + nock("https://control.ably.net") + .get("/v1/me/accounts") + .reply(200, [{ id: mockAccountId, name: "Test Account" }]); + // Mock the apps list endpoint with multiple apps nockControl() .get(`/v1/accounts/${mockAccountId}/apps`) @@ -175,7 +246,7 @@ describe("accounts:login command", () => { ]); const { stdout } = await runCommand( - ["accounts:login", mockAccessToken, "--json"], + ["accounts:login", "--json"], import.meta.url, ); @@ -188,22 +259,29 @@ describe("accounts:login command", () => { // Verify config was written without app selection via mock const mock = getMockConfigManager(); const config = mock.getConfig(); - expect(config.current?.account).toBe("default"); - expect(config.accounts["default"]).toBeDefined(); - expect(config.accounts["default"].accessToken).toBe(mockAccessToken); - expect(config.accounts["default"].accountId).toBe(mockAccountId); + expect(config.current?.account).toBe("test-account"); + expect(config.accounts["test-account"]).toBeDefined(); + expect(config.accounts["test-account"].oauthSessionKey).toBeDefined(); + const session = + config.oauthSessions?.[ + config.accounts["test-account"].oauthSessionKey! + ]; + expect(session?.accessToken).toBe(mockOAuthAccessToken); + expect(config.accounts["test-account"].accountId).toBe(mockAccountId); // Should NOT have currentAppId when multiple apps exist - expect(config.accounts["default"].currentAppId).toBeUndefined(); + expect(config.accounts["test-account"].currentAppId).toBeUndefined(); }); }); describe("error handling", () => { it("should output error in JSON format when authentication fails", async () => { + mockOAuthDeviceFlow(); + // Mock authentication failure nockControl().get("/v1/me").reply(401, { error: "Unauthorized" }); const { stdout } = await runCommand( - ["accounts:login", "invalid_token", "--json"], + ["accounts:login", "--json"], import.meta.url, ); @@ -215,11 +293,16 @@ describe("accounts:login command", () => { }); it("should output error in JSON format when network fails", async () => { - // Mock network error + mockOAuthDeviceFlow(); + + // Mock network error for both endpoints called in parallel nockControl().get("/v1/me").replyWithError("Network error"); + nock("https://control.ably.net") + .get("/v1/me/accounts") + .replyWithError("Network error"); const { stdout } = await runCommand( - ["accounts:login", mockAccessToken, "--json"], + ["accounts:login", "--json"], import.meta.url, ); @@ -232,13 +315,15 @@ describe("accounts:login command", () => { }); it("should handle 500 server error", async () => { + mockOAuthDeviceFlow(); + // Mock server error nockControl() .get("/v1/me") .reply(500, { error: "Internal Server Error" }); const { stdout } = await runCommand( - ["accounts:login", mockAccessToken, "--json"], + ["accounts:login", "--json"], import.meta.url, ); @@ -248,6 +333,52 @@ describe("accounts:login command", () => { expect(result).toHaveProperty("success", false); expect(result).toHaveProperty("error"); }); + + it("should output error when OAuth device flow fails", async () => { + // Mock device code request failure + nock("https://ably.com") + .post("/oauth/authorize_device") + .reply(400, "invalid_client"); + + const { stdout } = await runCommand( + ["accounts:login", "--json"], + import.meta.url, + ); + + const result = parseNdjsonLines(stdout).find((r) => r.type === "error")!; + expect(result).toHaveProperty("type", "error"); + expect(result).toHaveProperty("command", "accounts:login"); + expect(result).toHaveProperty("success", false); + expect(result).toHaveProperty("error"); + }); + + it("should output error when authorization is denied", async () => { + // Device code succeeds + nock("https://ably.com").post("/oauth/authorize_device").reply(200, { + device_code: "dc_denied", + expires_in: 300, + interval: 0.01, + user_code: "DENY-CODE", + verification_uri: "https://ably.com/device", + verification_uri_complete: + "https://ably.com/device?user_code=DENY-CODE", + }); + + // Token polling returns access_denied + nock("https://ably.com") + .post("/oauth/token") + .reply(400, { error: "access_denied" }); + + const { stdout } = await runCommand( + ["accounts:login", "--json"], + import.meta.url, + ); + + const result = parseNdjsonLines(stdout).find((r) => r.type === "error")!; + expect(result).toHaveProperty("type", "error"); + expect(result).toHaveProperty("success", false); + expect(result.error.message).toContain("Authorization denied"); + }); }); standardFlagTests("accounts:login", import.meta.url, ["--json"]); @@ -255,6 +386,7 @@ describe("accounts:login command", () => { describe("custom control host", () => { it("should use custom control host when --control-host flag is provided", async () => { const customHost = "custom.ably.net"; + mockOAuthDeviceFlow(customHost); // Mock the /me endpoint on custom host nock(`https://${customHost}`) @@ -264,19 +396,18 @@ describe("accounts:login command", () => { user: { email: "test@example.com" }, }); + // Mock the /me/accounts endpoint on custom host + nock(`https://${customHost}`) + .get("/v1/me/accounts") + .reply(200, [{ id: mockAccountId, name: "Test Account" }]); + // Mock the apps list endpoint on custom host nock(`https://${customHost}`) .get(`/v1/accounts/${mockAccountId}/apps`) .reply(200, []); const { stdout } = await runCommand( - [ - "accounts:login", - mockAccessToken, - "--control-host", - customHost, - "--json", - ], + ["accounts:login", "--control-host", customHost, "--json"], import.meta.url, ); @@ -290,12 +421,93 @@ describe("accounts:login command", () => { // Verify config was written correctly via mock const mock = getMockConfigManager(); const config = mock.getConfig(); - expect(config.current?.account).toBe("default"); - expect(config.accounts["default"]).toBeDefined(); - expect(config.accounts["default"].accessToken).toBe(mockAccessToken); - expect(config.accounts["default"].accountId).toBe(mockAccountId); - expect(config.accounts["default"].accountName).toBe("Test Account"); - expect(config.accounts["default"].userEmail).toBe("test@example.com"); + expect(config.current?.account).toBe("test-account"); + expect(config.accounts["test-account"]).toBeDefined(); + expect(config.accounts["test-account"].oauthSessionKey).toBeDefined(); + const session = + config.oauthSessions?.[ + config.accounts["test-account"].oauthSessionKey! + ]; + expect(session?.accessToken).toBe(mockOAuthAccessToken); + expect(config.accounts["test-account"].accountId).toBe(mockAccountId); + expect(config.accounts["test-account"].accountName).toBe("Test Account"); + }); + }); + + describe("OAuth device flow", () => { + it("should show --no-browser flag in help output", async () => { + const { stdout } = await runCommand( + ["accounts:login", "--help"], + import.meta.url, + ); + + expect(stdout).toContain("--no-browser"); + expect(stdout).toContain("Do not open a browser"); + }); + + it("should store OAuth tokens with authMethod, refreshToken, and expiresAt", async () => { + mockOAuthDeviceFlow(); + + nock("https://control.ably.net") + .get("/v1/me") + .reply(200, { + account: { id: mockAccountId, name: "Test Account" }, + user: { email: "test@example.com" }, + }); + + nock("https://control.ably.net") + .get("/v1/me/accounts") + .reply(200, [{ id: mockAccountId, name: "Test Account" }]); + + nock("https://control.ably.net") + .get(`/v1/accounts/${mockAccountId}/apps`) + .reply(200, []); + + await runCommand(["accounts:login", "--json"], import.meta.url); + + const mock = getMockConfigManager(); + const config = mock.getConfig(); + expect(config.accounts["test-account"].authMethod).toBe("oauth"); + expect(config.accounts["test-account"].oauthSessionKey).toBeDefined(); + const session = + config.oauthSessions?.[ + config.accounts["test-account"].oauthSessionKey! + ]; + expect(session?.refreshToken).toBe(mockRefreshToken); + expect(session?.accessTokenExpiresAt).toBeDefined(); + expect(session?.accessTokenExpiresAt).toBeGreaterThan(Date.now()); + }); + + it("should emit awaiting_authorization event in JSON mode", async () => { + mockOAuthDeviceFlow(); + + nock("https://control.ably.net") + .get("/v1/me") + .reply(200, { + account: { id: mockAccountId, name: "Test Account" }, + user: { email: "test@example.com" }, + }); + + nock("https://control.ably.net") + .get("/v1/me/accounts") + .reply(200, [{ id: mockAccountId, name: "Test Account" }]); + + nock("https://control.ably.net") + .get(`/v1/accounts/${mockAccountId}/apps`) + .reply(200, []); + + const { stdout } = await runCommand( + ["accounts:login", "--json"], + import.meta.url, + ); + + const events = parseNdjsonLines(stdout); + const authEvent = events.find( + (e) => e.status === "awaiting_authorization", + ); + expect(authEvent).toBeDefined(); + expect(authEvent).toHaveProperty("userCode", "TEST-CODE"); + expect(authEvent).toHaveProperty("verificationUri"); }); }); }); diff --git a/test/unit/commands/accounts/logout.test.ts b/test/unit/commands/accounts/logout.test.ts index 2f728665..1addf03e 100644 --- a/test/unit/commands/accounts/logout.test.ts +++ b/test/unit/commands/accounts/logout.test.ts @@ -1,5 +1,6 @@ -import { describe, it, expect, beforeEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach } from "vitest"; import { runCommand } from "@oclif/test"; +import nock from "nock"; import { getMockConfigManager } from "../../../helpers/mock-config-manager.js"; import { standardHelpTests, @@ -194,5 +195,175 @@ describe("accounts:logout command", () => { }); }); + describe("OAuth token revocation", () => { + afterEach(() => { + nock.cleanAll(); + }); + + it("should call revocation endpoint when logging out an OAuth account", async () => { + const mock = getMockConfigManager(); + mock.setConfig({ + current: { account: "testaccount" }, + accounts: { + testaccount: { + accessToken: "oauth_access_token", + accountId: "acc-123", + accountName: "Test Account", + userEmail: "test@example.com", + authMethod: "oauth", + oauthSessionKey: "test@example.com", + accessTokenExpiresAt: Date.now() + 3600000, + }, + }, + oauthSessions: { + "test@example.com": { + accessToken: "oauth_access_token", + refreshToken: "oauth_refresh_token", + accessTokenExpiresAt: Date.now() + 3600000, + }, + }, + }); + + // Expect two revocation calls (one for access token, one for refresh token) + const revokeScope = nock("https://ably.com") + .post("/oauth/revoke") + .twice() + .reply(200); + + const { stdout } = await runCommand( + ["accounts:logout", "--force", "--json"], + import.meta.url, + ); + + const result = parseNdjsonLines(stdout).find( + (r) => r.type === "result" || r.type === "error", + )!; + expect(result).toHaveProperty("success", true); + expect(revokeScope.isDone()).toBe(true); + }); + + it("should succeed even if revocation endpoint fails", async () => { + const mock = getMockConfigManager(); + mock.setConfig({ + current: { account: "testaccount" }, + accounts: { + testaccount: { + accessToken: "oauth_access_token", + accountId: "acc-123", + accountName: "Test Account", + userEmail: "test@example.com", + authMethod: "oauth", + oauthSessionKey: "test@example.com", + accessTokenExpiresAt: Date.now() + 3600000, + }, + }, + oauthSessions: { + "test@example.com": { + accessToken: "oauth_access_token", + refreshToken: "oauth_refresh_token", + accessTokenExpiresAt: Date.now() + 3600000, + }, + }, + }); + + // Revocation endpoint returns errors + nock("https://ably.com") + .post("/oauth/revoke") + .twice() + .replyWithError("Connection refused"); + + const { stdout } = await runCommand( + ["accounts:logout", "--force", "--json"], + import.meta.url, + ); + + const result = parseNdjsonLines(stdout).find( + (r) => r.type === "result" || r.type === "error", + )!; + expect(result).toHaveProperty("success", true); + + // Verify the account was still removed + const config = mock.getConfig(); + expect(config.accounts["testaccount"]).toBeUndefined(); + }); + + it("should not call revocation endpoint for non-OAuth account logout", async () => { + const mock = getMockConfigManager(); + mock.setConfig({ + current: { account: "testaccount" }, + accounts: { + testaccount: { + accessToken: "regular_token", + accountId: "acc-123", + accountName: "Test Account", + userEmail: "test@example.com", + }, + }, + }); + + // Set up nock interceptor that should NOT be called + const revokeScope = nock("https://ably.com") + .post("/oauth/revoke") + .reply(200); + + const { stdout } = await runCommand( + ["accounts:logout", "--force", "--json"], + import.meta.url, + ); + + const result = parseNdjsonLines(stdout).find( + (r) => r.type === "result" || r.type === "error", + )!; + expect(result).toHaveProperty("success", true); + + // The revocation endpoint should not have been called + expect(revokeScope.isDone()).toBe(false); + }); + + it("should not revoke tokens when other aliases share the same OAuth session", async () => { + const mock = getMockConfigManager(); + // Store two aliases sharing the same session via storeOAuthTokens + mock.storeOAuthTokens( + "alias-a", + { + accessToken: "shared_access_token", + refreshToken: "shared_refresh_token", + expiresAt: Date.now() + 3600000, + userEmail: "shared@example.com", + }, + { accountId: "acc-a", accountName: "Account A" }, + ); + mock.storeOAuthTokens( + "alias-b", + { + accessToken: "shared_access_token", + refreshToken: "shared_refresh_token", + expiresAt: Date.now() + 3600000, + userEmail: "shared@example.com", + }, + { accountId: "acc-b", accountName: "Account B" }, + ); + mock.setCurrentAccountAlias("alias-a"); + + const revokeScope = nock("https://ably.com") + .post("/oauth/revoke") + .twice() + .reply(200); + + await runCommand( + ["accounts:logout", "--force", "--json"], + import.meta.url, + ); + + // Revocation should NOT have been called — alias-b still uses the session + expect(revokeScope.isDone()).toBe(false); + // alias-b should still have valid tokens + expect(mock.getOAuthTokens("alias-b")).toBeDefined(); + expect(mock.getOAuthTokens("alias-b")?.refreshToken).toBe( + "shared_refresh_token", + ); + }); + }); + standardFlagTests("accounts:logout", import.meta.url, ["--json"]); }); diff --git a/test/unit/commands/accounts/switch.test.ts b/test/unit/commands/accounts/switch.test.ts index 89ac20cb..3c7fb221 100644 --- a/test/unit/commands/accounts/switch.test.ts +++ b/test/unit/commands/accounts/switch.test.ts @@ -137,6 +137,78 @@ describe("accounts:switch command", () => { }); }); + describe("OAuth account switching", () => { + it("should switch between local OAuth accounts", async () => { + const mock = getMockConfigManager(); + + // Store an OAuth account + mock.storeOAuthTokens( + "oauth-acct", + { + accessToken: "oauth_token_123", + refreshToken: "refresh_token_123", + expiresAt: Date.now() + 3600000, + userEmail: "oauth@example.com", + }, + { + accountId: "oauth-account-id", + accountName: "OAuth Account", + }, + ); + + nockControl() + .get("/v1/me") + .reply(200, { + account: { id: "oauth-account-id", name: "OAuth Account" }, + user: { email: "oauth@example.com" }, + }); + + const { stderr } = await runCommand( + ["accounts:switch", "oauth-acct"], + import.meta.url, + ); + + expect(stderr).toContain("Switched to account"); + expect(mock.getCurrentAccountAlias()).toBe("oauth-acct"); + expect(mock.getAuthMethod("oauth-acct")).toBe("oauth"); + }); + + it("should return JSON with account info when switching OAuth account with --json", async () => { + const mock = getMockConfigManager(); + + mock.storeOAuthTokens( + "oauth-json", + { + accessToken: "oauth_token_json", + refreshToken: "refresh_token_json", + expiresAt: Date.now() + 3600000, + userEmail: "json@example.com", + }, + { + accountId: "json-account-id", + accountName: "JSON Account", + }, + ); + + nockControl() + .get("/v1/me") + .reply(200, { + account: { id: "json-account-id", name: "JSON Account" }, + user: { email: "json@example.com" }, + }); + + const { stdout } = await runCommand( + ["accounts:switch", "oauth-json", "--json"], + import.meta.url, + ); + + const result = parseJsonOutput(stdout); + expect(result).toHaveProperty("success", true); + expect(result.account.alias).toBe("oauth-json"); + expect(result.account.id).toBe("json-account-id"); + }); + }); + standardHelpTests("accounts:switch", import.meta.url); standardArgValidationTests("accounts:switch", import.meta.url); standardFlagTests("accounts:switch", import.meta.url, ["--json"]); diff --git a/test/unit/services/oauth-client.test.ts b/test/unit/services/oauth-client.test.ts new file mode 100644 index 00000000..81df9970 --- /dev/null +++ b/test/unit/services/oauth-client.test.ts @@ -0,0 +1,346 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import nock from "nock"; +import { OAuthClient } from "../../../src/services/oauth-client.js"; + +describe("OAuthClient", () => { + beforeEach(() => { + nock.cleanAll(); + }); + + afterEach(() => { + nock.cleanAll(); + }); + + describe("OAuth config derivation", () => { + it("default host uses https scheme", async () => { + const client = new OAuthClient(); + + const scope = nock("https://ably.com") + .post("/oauth/authorize_device") + .reply(200, { + device_code: "dc_test", + expires_in: 300, + interval: 5, + user_code: "ABCD-1234", + verification_uri: "https://ably.com/device", + verification_uri_complete: + "https://ably.com/device?user_code=ABCD-1234", + }); + + await client.requestDeviceCode(); + + expect(scope.isDone()).toBe(true); + }); + + it("host containing 'local' uses http scheme", async () => { + const client = new OAuthClient({ controlHost: "localhost:3000" }); + + const scope = nock("http://localhost:3000") + .post("/oauth/authorize_device") + .reply(200, { + device_code: "dc_test", + expires_in: 300, + interval: 5, + user_code: "ABCD-1234", + verification_uri: "http://localhost:3000/device", + verification_uri_complete: + "http://localhost:3000/device?user_code=ABCD-1234", + }); + + await client.requestDeviceCode(); + + expect(scope.isDone()).toBe(true); + }); + + it("client ID matches the expected value", () => { + const client = new OAuthClient(); + expect(client.getClientId()).toBe( + "gb-I8-bZRnXs-gF83jOWKQrUxPPWp_ldTfQtgGP0EFg", + ); + }); + }); + + describe("requestDeviceCode", () => { + it("returns all fields from server response", async () => { + const client = new OAuthClient(); + + nock("https://ably.com").post("/oauth/authorize_device").reply(200, { + device_code: "dc_abc123", + expires_in: 600, + interval: 10, + user_code: "WXYZ-5678", + verification_uri: "https://ably.com/device", + verification_uri_complete: + "https://ably.com/device?user_code=WXYZ-5678", + }); + + const result = await client.requestDeviceCode(); + + expect(result.deviceCode).toBe("dc_abc123"); + expect(result.expiresIn).toBe(600); + expect(result.interval).toBe(10); + expect(result.userCode).toBe("WXYZ-5678"); + expect(result.verificationUri).toBe("https://ably.com/device"); + expect(result.verificationUriComplete).toBe( + "https://ably.com/device?user_code=WXYZ-5678", + ); + }); + + it("sends correct client_id and scope in body", async () => { + const client = new OAuthClient(); + + const scope = nock("https://ably.com") + .post( + "/oauth/authorize_device", + (body: Record) => + body.client_id === "gb-I8-bZRnXs-gF83jOWKQrUxPPWp_ldTfQtgGP0EFg" && + body.scope === "full_access", + ) + .reply(200, { + device_code: "dc_test", + expires_in: 300, + interval: 5, + user_code: "TEST-CODE", + verification_uri: "https://ably.com/device", + verification_uri_complete: + "https://ably.com/device?user_code=TEST-CODE", + }); + + await client.requestDeviceCode(); + + expect(scope.isDone()).toBe(true); + }); + + it("throws on non-200 response", async () => { + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/authorize_device") + .reply(400, "invalid_client"); + + await expect(client.requestDeviceCode()).rejects.toThrow( + "Device code request failed (400): invalid_client", + ); + }); + }); + + describe("pollForToken", () => { + it("returns tokens after authorization_pending then success", async () => { + const client = new OAuthClient(); + + // First call: authorization_pending + nock("https://ably.com") + .post("/oauth/token") + .reply(400, { error: "authorization_pending" }); + + // Second call: success + nock("https://ably.com").post("/oauth/token").reply(200, { + access_token: "at_device_flow", + expires_in: 3600, + refresh_token: "rt_device_flow", + scope: "full_access", + token_type: "Bearer", + }); + + const tokens = await client.pollForToken("dc_test", 0.01, 10); + + expect(tokens.accessToken).toBe("at_device_flow"); + expect(tokens.refreshToken).toBe("rt_device_flow"); + expect(tokens.tokenType).toBe("Bearer"); + }); + + it("increases interval on slow_down", async () => { + const client = new OAuthClient(); + + // First call: slow_down (interval increases by 5s per RFC 8628) + nock("https://ably.com") + .post("/oauth/token") + .reply(400, { error: "slow_down" }); + + // Second call: success + nock("https://ably.com").post("/oauth/token").reply(200, { + access_token: "at_slow", + expires_in: 3600, + refresh_token: "rt_slow", + token_type: "Bearer", + }); + + const tokens = await client.pollForToken("dc_test", 0.01, 30); + + expect(tokens.accessToken).toBe("at_slow"); + }, 15_000); + + it("throws on expired_token", async () => { + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/token") + .reply(400, { error: "expired_token" }); + + await expect(client.pollForToken("dc_test", 0.01, 10)).rejects.toThrow( + "Device code expired", + ); + }); + + it("throws on access_denied", async () => { + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/token") + .reply(400, { error: "access_denied" }); + + await expect(client.pollForToken("dc_test", 0.01, 10)).rejects.toThrow( + "Authorization denied", + ); + }); + + it("sends correct grant_type and device_code", async () => { + const client = new OAuthClient(); + + const scope = nock("https://ably.com") + .post("/oauth/token", (body: Record) => { + return ( + body.grant_type === + "urn:ietf:params:oauth:grant-type:device_code" && + body.device_code === "dc_verify" && + body.client_id === "gb-I8-bZRnXs-gF83jOWKQrUxPPWp_ldTfQtgGP0EFg" + ); + }) + .reply(200, { + access_token: "at_verify", + expires_in: 3600, + refresh_token: "rt_verify", + token_type: "Bearer", + }); + + await client.pollForToken("dc_verify", 0.01, 10); + + expect(scope.isDone()).toBe(true); + }); + + it("throws after deadline exceeded", async () => { + const client = new OAuthClient(); + + // Keep returning authorization_pending; the deadline will expire + nock("https://ably.com") + .post("/oauth/token") + .times(10) + .reply(400, { error: "authorization_pending" }); + + // Use a very short expiresIn so the deadline passes quickly + await expect( + client.pollForToken("dc_expired", 0.01, 0.05), + ).rejects.toThrow("Device code expired"); + }); + }); + + describe("refreshAccessToken", () => { + it("returns OAuthTokens on successful refresh", async () => { + const client = new OAuthClient(); + + nock("https://ably.com").post("/oauth/token").reply(200, { + access_token: "refreshed_access_token", + expires_in: 7200, + refresh_token: "refreshed_refresh_token", + scope: "full_access", + token_type: "Bearer", + }); + + const tokens = await client.refreshAccessToken("old_refresh_token"); + + expect(tokens.accessToken).toBe("refreshed_access_token"); + expect(tokens.refreshToken).toBe("refreshed_refresh_token"); + expect(tokens.tokenType).toBe("Bearer"); + expect(tokens.scope).toBe("full_access"); + expect(tokens.expiresAt).toBeGreaterThan(Date.now()); + }); + + it("throws on non-200 response with status and body", async () => { + const client = new OAuthClient(); + + nock("https://ably.com").post("/oauth/token").reply(401, "invalid_grant"); + + await expect( + client.refreshAccessToken("bad_refresh_token"), + ).rejects.toThrow("Token refresh failed (401): invalid_grant"); + }); + + it("sends correct form-encoded body", async () => { + const client = new OAuthClient(); + const refreshToken = "my_refresh_token"; + + const scope = nock("https://ably.com") + .post("/oauth/token", (body: Record) => { + return ( + body.grant_type === "refresh_token" && + body.client_id === "gb-I8-bZRnXs-gF83jOWKQrUxPPWp_ldTfQtgGP0EFg" && + body.refresh_token === refreshToken + ); + }) + .reply(200, { + access_token: "new_access", + expires_in: 3600, + refresh_token: "new_refresh", + token_type: "Bearer", + }); + + await client.refreshAccessToken(refreshToken); + + expect(scope.isDone()).toBe(true); + }); + + it("preserves existing refresh token when response omits refresh_token", async () => { + const client = new OAuthClient(); + + // Response omits refresh_token (allowed by RFC 6749 §5.1) + nock("https://ably.com").post("/oauth/token").reply(200, { + access_token: "new_access_token", + expires_in: 3600, + token_type: "Bearer", + }); + + const tokens = await client.refreshAccessToken("existing_refresh_token"); + + expect(tokens.accessToken).toBe("new_access_token"); + expect(tokens.refreshToken).toBe("existing_refresh_token"); + }); + + it("throws when response missing both access_token and refresh_token", async () => { + const client = new OAuthClient(); + + nock("https://ably.com").post("/oauth/token").reply(200, { + token_type: "Bearer", + expires_in: 3600, + }); + + await expect( + client.refreshAccessToken("some_refresh_token"), + ).rejects.toThrow("Token response missing required fields"); + }); + }); + + describe("revokeToken", () => { + it("sends POST to revocation endpoint", async () => { + const client = new OAuthClient(); + + const scope = nock("https://ably.com").post("/oauth/revoke").reply(200); + + await client.revokeToken("token_to_revoke"); + + expect(scope.isDone()).toBe(true); + }); + + it("does not throw on network error", async () => { + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/revoke") + .replyWithError("Connection refused"); + + // Should not throw + await expect( + client.revokeToken("token_to_revoke"), + ).resolves.toBeUndefined(); + }); + }); +}); diff --git a/test/unit/services/shared-oauth-session.test.ts b/test/unit/services/shared-oauth-session.test.ts new file mode 100644 index 00000000..5cdf3e1e --- /dev/null +++ b/test/unit/services/shared-oauth-session.test.ts @@ -0,0 +1,215 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { getMockConfigManager } from "../../helpers/mock-config-manager.js"; + +describe("Shared OAuth session", () => { + let mock: ReturnType; + + beforeEach(() => { + mock = getMockConfigManager(); + mock.clearAccounts(); + }); + + it("two aliases with the same userEmail share one session", () => { + mock.storeOAuthTokens( + "alias-a", + { + accessToken: "at_1", + refreshToken: "rt_1", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-a", accountName: "Account A" }, + ); + mock.storeOAuthTokens( + "alias-b", + { + accessToken: "at_1", + refreshToken: "rt_1", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-b", accountName: "Account B" }, + ); + + const config = mock.getConfig(); + expect(config.accounts["alias-a"].oauthSessionKey).toBe("user@example.com"); + expect(config.accounts["alias-b"].oauthSessionKey).toBe("user@example.com"); + // Only one session entry + expect(Object.keys(config.oauthSessions!)).toHaveLength(1); + }); + + it("refreshing one alias propagates tokens to all sharing aliases", () => { + // Initial store for both aliases + mock.storeOAuthTokens( + "alias-a", + { + accessToken: "at_old", + refreshToken: "rt_old", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-a", accountName: "Account A" }, + ); + mock.storeOAuthTokens( + "alias-b", + { + accessToken: "at_old", + refreshToken: "rt_old", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-b", accountName: "Account B" }, + ); + + // Simulate token refresh while on alias-a + mock.switchAccount("alias-a"); + mock.storeOAuthTokens("alias-a", { + accessToken: "at_new", + refreshToken: "rt_new", + expiresAt: Date.now() + 7200000, + userEmail: "user@example.com", + }); + + // Switch to alias-b — should see the refreshed tokens + mock.switchAccount("alias-b"); + expect(mock.getAccessToken()).toBe("at_new"); + expect(mock.getOAuthTokens()?.refreshToken).toBe("rt_new"); + }); + + it("different userEmails get separate sessions", () => { + mock.storeOAuthTokens( + "alias-a", + { + accessToken: "at_a", + refreshToken: "rt_a", + expiresAt: Date.now() + 3600000, + userEmail: "user1@example.com", + }, + { accountId: "acc-a", accountName: "Account A" }, + ); + mock.storeOAuthTokens( + "alias-b", + { + accessToken: "at_b", + refreshToken: "rt_b", + expiresAt: Date.now() + 3600000, + userEmail: "user2@example.com", + }, + { accountId: "acc-b", accountName: "Account B" }, + ); + + const config = mock.getConfig(); + expect(Object.keys(config.oauthSessions!)).toHaveLength(2); + expect(config.accounts["alias-a"].oauthSessionKey).toBe( + "user1@example.com", + ); + expect(config.accounts["alias-b"].oauthSessionKey).toBe( + "user2@example.com", + ); + + // Refreshing alias-a should NOT affect alias-b + mock.switchAccount("alias-a"); + mock.storeOAuthTokens("alias-a", { + accessToken: "at_a_new", + refreshToken: "rt_a_new", + expiresAt: Date.now() + 7200000, + userEmail: "user1@example.com", + }); + + mock.switchAccount("alias-b"); + expect(mock.getOAuthTokens()?.accessToken).toBe("at_b"); + expect(mock.getOAuthTokens()?.refreshToken).toBe("rt_b"); + }); + + it("getAliasesForOAuthSession returns all sharing aliases", () => { + mock.storeOAuthTokens( + "work", + { + accessToken: "at", + refreshToken: "rt", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-1", accountName: "Work" }, + ); + mock.storeOAuthTokens( + "personal", + { + accessToken: "at", + refreshToken: "rt", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-2", accountName: "Personal" }, + ); + + const aliases = mock.getAliasesForOAuthSession("work"); + expect(aliases).toContain("work"); + expect(aliases).toContain("personal"); + expect(aliases).toHaveLength(2); + }); + + it("removing last alias for a session cleans up the session entry", () => { + mock.storeOAuthTokens( + "only-alias", + { + accessToken: "at", + refreshToken: "rt", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-1", accountName: "Only" }, + ); + + expect(mock.getConfig().oauthSessions).toBeDefined(); + mock.removeAccount("only-alias"); + expect(mock.getConfig().oauthSessions).toBeUndefined(); + }); + + it("removing one alias keeps session when other aliases still reference it", () => { + mock.storeOAuthTokens( + "alias-a", + { + accessToken: "at", + refreshToken: "rt", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-a", accountName: "A" }, + ); + mock.storeOAuthTokens( + "alias-b", + { + accessToken: "at", + refreshToken: "rt", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-b", accountName: "B" }, + ); + + mock.removeAccount("alias-a"); + + // Session should still exist for alias-b + const config = mock.getConfig(); + expect(config.oauthSessions?.["user@example.com"]).toBeDefined(); + expect(mock.getOAuthTokens("alias-b")?.refreshToken).toBe("rt"); + }); + + it("refreshToken is stored only in the session, not the account", () => { + mock.storeOAuthTokens( + "test", + { + accessToken: "at", + refreshToken: "rt", + expiresAt: Date.now() + 3600000, + userEmail: "user@example.com", + }, + { accountId: "acc-1", accountName: "Test" }, + ); + + mock.switchAccount("test"); + expect(mock.getOAuthTokens()?.refreshToken).toBe("rt"); + expect(mock.getAccessToken()).toBe("at"); + }); +}); diff --git a/test/unit/services/token-refresh-middleware.test.ts b/test/unit/services/token-refresh-middleware.test.ts new file mode 100644 index 00000000..56f1fbee --- /dev/null +++ b/test/unit/services/token-refresh-middleware.test.ts @@ -0,0 +1,187 @@ +import { describe, it, expect, vi } from "vitest"; +import { + TokenRefreshMiddleware, + TokenExpiredError, +} from "../../../src/services/token-refresh-middleware.js"; + +function createMockConfigManager(overrides: Record = {}) { + return { + getAccessToken: vi.fn().mockReturnValue("current_access_token"), + getAuthMethod: vi.fn(), + getCurrentAccountAlias: vi.fn().mockReturnValue("default"), + getOAuthTokens: vi.fn(), + isAccessTokenExpired: vi.fn().mockReturnValue(false), + storeOAuthTokens: vi.fn(), + ...overrides, + }; +} + +function createMockOAuthClient(overrides: Record = {}) { + return { + refreshAccessToken: vi.fn(), + ...overrides, + }; +} + +describe("TokenRefreshMiddleware", () => { + describe("non-OAuth passthrough", () => { + it("returns access token as-is when authMethod is not oauth", async () => { + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn().mockReturnValue("token"), + getAccessToken: vi.fn().mockReturnValue("legacy_token_value"), + }); + const oauthClient = createMockOAuthClient(); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + const token = await middleware.getValidAccessToken(); + + expect(token).toBe("legacy_token_value"); + expect(oauthClient.refreshAccessToken).not.toHaveBeenCalled(); + }); + + it("throws TokenExpiredError when no access token is found", async () => { + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn(), + getAccessToken: vi.fn(), + }); + const oauthClient = createMockOAuthClient(); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + await expect(middleware.getValidAccessToken()).rejects.toThrow( + TokenExpiredError, + ); + await expect(middleware.getValidAccessToken()).rejects.toThrow( + "No access token found", + ); + }); + }); + + describe("non-expired OAuth token", () => { + it("returns current token without calling refresh when not expired", async () => { + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn().mockReturnValue("oauth"), + getAccessToken: vi.fn().mockReturnValue("valid_oauth_token"), + isAccessTokenExpired: vi.fn().mockReturnValue(false), + }); + const oauthClient = createMockOAuthClient(); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + const token = await middleware.getValidAccessToken(); + + expect(token).toBe("valid_oauth_token"); + expect(oauthClient.refreshAccessToken).not.toHaveBeenCalled(); + expect(configManager.storeOAuthTokens).not.toHaveBeenCalled(); + }); + }); + + describe("expired OAuth token", () => { + it("refreshes token, stores new tokens, and returns new access token", async () => { + const newTokens = { + accessToken: "refreshed_access_token", + expiresAt: Date.now() + 7200000, + refreshToken: "refreshed_refresh_token", + scope: "full_access", + tokenType: "Bearer", + }; + + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn().mockReturnValue("oauth"), + getAccessToken: vi.fn().mockReturnValue("expired_oauth_token"), + isAccessTokenExpired: vi.fn().mockReturnValue(true), + getOAuthTokens: vi.fn().mockReturnValue({ + accessToken: "expired_oauth_token", + refreshToken: "valid_refresh_token", + expiresAt: Date.now() - 1000, + }), + getCurrentAccountAlias: vi.fn().mockReturnValue("default"), + }); + const oauthClient = createMockOAuthClient({ + refreshAccessToken: vi.fn().mockResolvedValue(newTokens), + }); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + const token = await middleware.getValidAccessToken(); + + expect(token).toBe("refreshed_access_token"); + expect(oauthClient.refreshAccessToken).toHaveBeenCalledWith( + "valid_refresh_token", + ); + expect(configManager.storeOAuthTokens).toHaveBeenCalledWith( + "default", + newTokens, + ); + }); + }); + + describe("missing refresh token", () => { + it("throws TokenExpiredError when token is expired but no refresh token available", async () => { + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn().mockReturnValue("oauth"), + getAccessToken: vi.fn().mockReturnValue("expired_token"), + isAccessTokenExpired: vi.fn().mockReturnValue(true), + getOAuthTokens: vi.fn(), + }); + const oauthClient = createMockOAuthClient(); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + await expect(middleware.getValidAccessToken()).rejects.toThrow( + TokenExpiredError, + ); + await expect(middleware.getValidAccessToken()).rejects.toThrow( + "run 'ably login' again", + ); + }); + }); + + describe("refresh failure", () => { + it("propagates error when refreshAccessToken throws", async () => { + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn().mockReturnValue("oauth"), + getAccessToken: vi.fn().mockReturnValue("expired_token"), + isAccessTokenExpired: vi.fn().mockReturnValue(true), + getOAuthTokens: vi.fn().mockReturnValue({ + accessToken: "expired_token", + refreshToken: "bad_refresh_token", + expiresAt: Date.now() - 1000, + }), + getCurrentAccountAlias: vi.fn().mockReturnValue("default"), + }); + const oauthClient = createMockOAuthClient({ + refreshAccessToken: vi + .fn() + .mockRejectedValue( + new Error("Token refresh failed (401): invalid_grant"), + ), + }); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + await expect(middleware.getValidAccessToken()).rejects.toThrow( + "Token refresh failed (401): invalid_grant", + ); + }); + }); +}); diff --git a/test/unit/utils/slugify.test.ts b/test/unit/utils/slugify.test.ts new file mode 100644 index 00000000..c8b2bc02 --- /dev/null +++ b/test/unit/utils/slugify.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect } from "vitest"; +import { slugifyAccountName } from "../../../src/utils/slugify.js"; + +describe("slugifyAccountName", () => { + it("lowercases and replaces spaces with dashes", () => { + expect(slugifyAccountName("My Account")).toBe("my-account"); + }); + + it("strips leading and trailing dashes", () => { + expect(slugifyAccountName(" My Account ")).toBe("my-account"); + }); + + it("replaces non-alphanumeric characters with dashes", () => { + expect(slugifyAccountName("Ably (Production)")).toBe("ably-production"); + }); + + it("collapses consecutive non-alphanumeric to single dash", () => { + expect(slugifyAccountName("foo---bar")).toBe("foo-bar"); + }); + + it("returns 'default' for empty string", () => { + expect(slugifyAccountName("")).toBe("default"); + }); + + it("returns 'default' for whitespace-only string", () => { + expect(slugifyAccountName(" ")).toBe("default"); + }); + + it("prefixes with 'account-' when name starts with a number", () => { + expect(slugifyAccountName("123 Corp")).toBe("account-123-corp"); + }); + + it("prefixes with 'account-' when name starts with a dash after cleanup", () => { + expect(slugifyAccountName("--test")).toBe("test"); + }); + + it("handles single letter name", () => { + expect(slugifyAccountName("a")).toBe("a"); + }); + + it("handles purely numeric name", () => { + expect(slugifyAccountName("12345")).toBe("account-12345"); + }); +}); From 8c6d82856356ad484357cf6e07e67489f4d82014 Mon Sep 17 00:00:00 2001 From: umair Date: Thu, 16 Apr 2026 09:51:05 +0100 Subject: [PATCH 05/19] Fix logout host resolution and mock config fallbacks - Logout now resolves controlHost from the target account (not the current account), fixing revocation against the wrong host when logging out a non-current alias. - MockConfigManager.storeOAuthTokens now uses ?? "" fallbacks for accountId, accountName, and userEmail, matching TomlConfigManager. --- src/commands/accounts/logout.ts | 54 +++++++++++++++++------------ test/helpers/mock-config-manager.ts | 6 ++-- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/commands/accounts/logout.ts b/src/commands/accounts/logout.ts index a267f2a6..bbd72719 100644 --- a/src/commands/accounts/logout.ts +++ b/src/commands/accounts/logout.ts @@ -74,32 +74,40 @@ export default class AccountsLogout extends ControlBaseCommand { } } - // Revoke OAuth tokens if this is an OAuth account + // Revoke OAuth tokens if this is an OAuth account and no other aliases share the session if (this.configManager.getAuthMethod(targetAlias) === "oauth") { const oauthTokens = this.configManager.getOAuthTokens(targetAlias); if (oauthTokens) { - const targetAccount = this.configManager.getCurrentAccount(); - const oauthHost = flags["control-host"] || targetAccount?.controlHost; - const oauthClient = new OAuthClient({ - controlHost: oauthHost, - }); - // Best-effort revocation with timeout -- don't block logout - const revokeWithTimeout = (token: string, timeoutMs = 5000) => - Promise.race([ - oauthClient.revokeToken(token), - new Promise((resolve) => setTimeout(resolve, timeoutMs)), - ]); - await Promise.all([ - revokeWithTimeout(oauthTokens.accessToken), - revokeWithTimeout(oauthTokens.refreshToken), - ]).catch((error) => { - this.logCliEvent( - flags, - "accountLogout", - "revocationFailed", - `OAuth token revocation failed: ${error instanceof Error ? error.message : String(error)}`, - ); - }); + const sharingAliases = + this.configManager.getAliasesForOAuthSession(targetAlias); + const isLastAlias = sharingAliases.length <= 1; + + if (isLastAlias) { + const targetAccount = this.configManager + .listAccounts() + .find((a) => a.alias === targetAlias)?.account; + const oauthHost = flags["control-host"] || targetAccount?.controlHost; + const oauthClient = new OAuthClient({ + controlHost: oauthHost, + }); + // Best-effort revocation with timeout -- don't block logout + const revokeWithTimeout = (token: string, timeoutMs = 5000) => + Promise.race([ + oauthClient.revokeToken(token), + new Promise((resolve) => setTimeout(resolve, timeoutMs)), + ]); + await Promise.all([ + revokeWithTimeout(oauthTokens.accessToken), + revokeWithTimeout(oauthTokens.refreshToken), + ]).catch((error) => { + this.logCliEvent( + flags, + "accountLogout", + "revocationFailed", + `OAuth token revocation failed: ${error instanceof Error ? error.message : String(error)}`, + ); + }); + } } } diff --git a/test/helpers/mock-config-manager.ts b/test/helpers/mock-config-manager.ts index 14f81438..1ac76993 100644 --- a/test/helpers/mock-config-manager.ts +++ b/test/helpers/mock-config-manager.ts @@ -539,9 +539,11 @@ export class MockConfigManager implements ConfigManager { this.config.accounts[alias] = { ...this.config.accounts[alias], accountId: - accountInfo?.accountId ?? this.config.accounts[alias]?.accountId, + accountInfo?.accountId ?? this.config.accounts[alias]?.accountId ?? "", accountName: - accountInfo?.accountName ?? this.config.accounts[alias]?.accountName, + accountInfo?.accountName ?? + this.config.accounts[alias]?.accountName ?? + "", apps: this.config.accounts[alias]?.apps || {}, authMethod: "oauth", currentAppId: this.config.accounts[alias]?.currentAppId, From 3c3ee069711533507987b1f90161e7a5e6520386 Mon Sep 17 00:00:00 2001 From: umair Date: Thu, 16 Apr 2026 11:16:47 +0100 Subject: [PATCH 06/19] Fix accounts current to use getAccessToken for OAuth compatibility Use configManager.getAccessToken() instead of destructuring account.accessToken directly, which may be undefined for OAuth accounts that store tokens in the shared session. --- src/commands/accounts/current.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/accounts/current.ts b/src/commands/accounts/current.ts index 8d0f07f3..9fb96fb6 100644 --- a/src/commands/accounts/current.ts +++ b/src/commands/accounts/current.ts @@ -44,10 +44,10 @@ export default class AccountsCurrent extends ControlBaseCommand { // Verify the account by making an API call to get up-to-date information try { - const { accessToken } = currentAccount; + const accessToken = this.configManager.getAccessToken(); const controlApi = new ControlApi({ - accessToken, + accessToken: accessToken ?? "", controlHost: flags["control-host"], }); From 19820d9899bd572c6d88d4a8d5fef2239a0e6092 Mon Sep 17 00:00:00 2001 From: umair Date: Thu, 16 Apr 2026 17:24:05 +0100 Subject: [PATCH 07/19] Harden OAuth session scoping, error handling, and test mock fidelity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the OAuth login flow branch. Addresses silent session collisions across control hosts, unhelpful refresh-token error messages, and mock/production drift that masked real behaviour in tests. Changes: - Scope OAuth session keys by control host (config-manager.ts): sessionKey is now "${email}::${controlHost}" (was just email). This prevents the same email on prod and a staging deployment from silently overwriting each other's refresh tokens in shared sessions. storeOAuthTokens now accepts controlHost in accountInfo and writes it to the account record. Adds migration cleanup: when an alias's session key changes (e.g. pre-controlHost config), the old session entry is removed if no other alias still references it. - Fold controlHost assignment into storeOAuthTokens (login.ts, switch.ts): both call sites previously stored tokens and then made a second setAccountControlHost() call. This consolidated into one storeOAuthTokens call with controlHost passed in accountInfo. setAccountControlHost is now unused and removed from the ConfigManager interface, TomlConfigManager, and MockConfigManager. - Add OAuthRefreshExpiredError and handle invalid_grant cleanly (oauth-client.ts, token-refresh-middleware.ts): when the token endpoint rejects a refresh with OAuth error "invalid_grant" (revoked, rotated by a concurrent refresh, or session expired server-side), refreshAccessToken now throws a typed OAuthRefreshExpiredError instead of a raw HTTP Error. The middleware catches it, clears the dead session via the new clearOAuthSession(), and throws TokenExpiredError with a clean "please run ably login" message. Subsequent commands short-circuit instead of re-attempting refresh against a dead token. - Add ConfigManager.clearOAuthSession(alias?) (config-manager.ts, mock-config-manager.ts): new method that removes the shared OAuth session entry for an alias (if no other alias still references it) and clears accessToken/accessTokenExpiresAt/oauthSessionKey on the account, without removing the account itself. - Route login warnings through logWarning in JSON mode (login.ts): previously "Multiple apps found", "No apps found", "Could not fetch apps/keys" were all suppressed in JSON mode. They now emit structured JSON warnings so agents get actionable signals instead of silent no-ops. Human mode output is unchanged. - Sync MockConfigManager.storeOAuthTokens with production (test/helpers/mock-config-manager.ts): accepts controlHost in accountInfo, produces email::host session keys, cleans up the previous session key on rekey, persists controlHost on the account record. Previously the mock's behaviour diverged from production — tests passed against the mock but didn't reflect real session-key behaviour. The DEFAULT_OAUTH_CONTROL_HOST constant is duplicated locally (with a pinning comment) because importing it from oauth-client.js pulls node-fetch into the vitest setup graph (via test/unit/setup.ts -> mock-config-manager) and installs node-fetch's global agent before nock is set up per test, breaking HTTPS interception for commands like auth:revoke-token. - Update session-key test assertions (shared-oauth-session.test.ts, logout.test.ts): expected keys changed from "user@example.com" to "user@example.com::ably.com". logout.test.ts fixture configs updated to the new key format too (arbitrary string, but kept consistent with production for readability). - Logout revocation no longer double-catches (logout.ts): revokeToken is best-effort and swallows its own errors, so the outer .catch() on Promise.all was dead code. Removed; the contract is pinned with a comment in OAuthClient.revokeToken stating it must never reject. - Typed refresh result in middleware (token-refresh-middleware.ts): let newTokens: OAuthTokens instead of untyped. - Extracted DEFAULT_OAUTH_CONTROL_HOST constant (oauth-client.ts): replaces a magic "ably.com" string; config-manager imports it (safe: production loads config-manager lazily at command execution, after nock is set up — unlike the test mock). All 2319 unit tests pass; 0 eslint errors. --- src/commands/accounts/login.ts | 147 ++++++++++-------- src/commands/accounts/logout.ts | 13 +- src/commands/accounts/switch.ts | 13 +- src/services/config-manager.ts | 66 +++++++- src/services/oauth-client.ts | 49 +++++- src/services/token-refresh-middleware.ts | 24 ++- test/helpers/mock-config-manager.ts | 63 +++++++- test/unit/commands/accounts/logout.test.ts | 8 +- .../services/shared-oauth-session.test.ts | 14 +- 9 files changed, 289 insertions(+), 108 deletions(-) diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index bee5513b..f23be105 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -121,21 +121,19 @@ export default class AccountsLogin extends ControlBaseCommand { alias = slugifyAccountName(selectedAccountInfo.name); } - // Store OAuth tokens (include user email from /me response) + // Store OAuth tokens (include user email from /me response). + // Pass controlHost so the session key is scoped per endpoint — otherwise + // the same email on prod and a staging deployment would collide. this.configManager.storeOAuthTokens( alias, { ...oauthTokens, userEmail: user.email }, { accountId: selectedAccountInfo.id, accountName: selectedAccountInfo.name, + controlHost: flags["control-host"], }, ); - // Persist control host so other commands (like switch) can use it - if (flags["control-host"]) { - this.configManager.setAccountControlHost(alias, flags["control-host"]); - } - // Switch to this account this.configManager.switchAccount(alias); @@ -158,70 +156,81 @@ export default class AccountsLogin extends ControlBaseCommand { this.configManager.storeAppInfo(selectedApp.id, { appName: selectedApp.name, }); - } else if (apps.length > 1 && !this.shouldOutputJson(flags)) { - // Prompt user to select an app when multiple exist - this.log("\nSelect an app to use:"); - - selectedApp = await this.interactiveHelper.selectApp(controlApi); - - if (selectedApp) { - this.configManager.setCurrentApp(selectedApp.id); - this.configManager.storeAppInfo(selectedApp.id, { - appName: selectedApp.name, - }); - } - } else if (apps.length === 0 && !this.shouldOutputJson(flags)) { - // No apps exist - offer to create one - this.log("\nNo apps found in your account."); - - const shouldCreateApp = await promptForConfirmation( - "Would you like to create your first app now?", - ); + } else if (apps.length > 1) { + if (this.shouldOutputJson(flags)) { + this.logWarning( + "Multiple apps found; cannot auto-select in JSON mode. Run 'ably apps switch' in a terminal to choose one.", + flags, + ); + } else { + this.log("\nSelect an app to use:"); - if (shouldCreateApp) { - const appName = await this.promptForAppName(); + selectedApp = await this.interactiveHelper.selectApp(controlApi); - try { - this.log(""); // blank line before progress - this.logProgress( - `Creating app ${formatResource(appName)}`, - flags, - ); - - const app = await controlApi.createApp({ - name: appName, - tlsOnly: true, + if (selectedApp) { + this.configManager.setCurrentApp(selectedApp.id); + this.configManager.storeAppInfo(selectedApp.id, { + appName: selectedApp.name, }); + } + } + } else if (apps.length === 0) { + if (this.shouldOutputJson(flags)) { + this.logWarning( + "No apps found in this account. Run 'ably apps create' to create one.", + flags, + ); + } else { + this.log("\nNo apps found in your account."); - selectedApp = app; - isAutoSelected = true; - - this.configManager.setCurrentApp(app.id); - this.configManager.storeAppInfo(app.id, { appName: app.name }); + const shouldCreateApp = await promptForConfirmation( + "Would you like to create your first app now?", + ); - this.logSuccessMessage("App created successfully.", flags); - } catch (createError) { - this.warn( - `Failed to create app: ${createError instanceof Error ? createError.message : String(createError)}`, - ); + if (shouldCreateApp) { + const appName = await this.promptForAppName(); + + try { + this.log(""); // blank line before progress + this.logProgress( + `Creating app ${formatResource(appName)}`, + flags, + ); + + const app = await controlApi.createApp({ + name: appName, + tlsOnly: true, + }); + + selectedApp = app; + isAutoSelected = true; + + this.configManager.setCurrentApp(app.id); + this.configManager.storeAppInfo(app.id, { appName: app.name }); + + this.logSuccessMessage("App created successfully.", flags); + } catch (createError) { + this.logWarning( + `Failed to create app: ${createError instanceof Error ? createError.message : String(createError)}`, + flags, + ); + } } } } } catch (error) { - if (!this.shouldOutputJson(flags)) { - this.warn(`Could not fetch apps: ${errorMessage(error)}`); - } + this.logWarning(`Could not fetch apps: ${errorMessage(error)}`, flags); } // If we have a selected app, also handle API key selection let selectedKey = null; let isKeyAutoSelected = false; - if (selectedApp && !this.shouldOutputJson(flags)) { + if (selectedApp) { try { const keys = await controlApi.listKeys(selectedApp.id); if (keys.length === 1) { - // Auto-select the only key + // Auto-select the only key (safe in both modes) selectedKey = keys[0]!; isKeyAutoSelected = true; this.configManager.storeAppKey(selectedApp.id, selectedKey.key, { @@ -229,23 +238,35 @@ export default class AccountsLogin extends ControlBaseCommand { keyName: selectedKey.name || "Unnamed key", }); } else if (keys.length > 1) { - this.log("\nSelect an API key to use:"); + if (this.shouldOutputJson(flags)) { + this.logWarning( + "Multiple API keys found; cannot auto-select in JSON mode. Run 'ably auth keys switch' in a terminal to choose one.", + flags, + ); + } else { + this.log("\nSelect an API key to use:"); - selectedKey = await this.interactiveHelper.selectKey( - controlApi, - selectedApp.id, - ); + selectedKey = await this.interactiveHelper.selectKey( + controlApi, + selectedApp.id, + ); - if (selectedKey) { - this.configManager.storeAppKey(selectedApp.id, selectedKey.key, { - keyId: selectedKey.id, - keyName: selectedKey.name || "Unnamed key", - }); + if (selectedKey) { + this.configManager.storeAppKey( + selectedApp.id, + selectedKey.key, + { + keyId: selectedKey.id, + keyName: selectedKey.name || "Unnamed key", + }, + ); + } } } } catch (keyError) { - this.warn( + this.logWarning( `Could not fetch API keys: ${keyError instanceof Error ? keyError.message : String(keyError)}`, + flags, ); } } diff --git a/src/commands/accounts/logout.ts b/src/commands/accounts/logout.ts index bbd72719..91a4f543 100644 --- a/src/commands/accounts/logout.ts +++ b/src/commands/accounts/logout.ts @@ -90,7 +90,9 @@ export default class AccountsLogout extends ControlBaseCommand { const oauthClient = new OAuthClient({ controlHost: oauthHost, }); - // Best-effort revocation with timeout -- don't block logout + // Best-effort revocation with timeout -- don't block logout. + // revokeToken() swallows its own errors and always resolves, so we + // don't need a .catch() here. const revokeWithTimeout = (token: string, timeoutMs = 5000) => Promise.race([ oauthClient.revokeToken(token), @@ -99,14 +101,7 @@ export default class AccountsLogout extends ControlBaseCommand { await Promise.all([ revokeWithTimeout(oauthTokens.accessToken), revokeWithTimeout(oauthTokens.refreshToken), - ]).catch((error) => { - this.logCliEvent( - flags, - "accountLogout", - "revocationFailed", - `OAuth token revocation failed: ${error instanceof Error ? error.message : String(error)}`, - ); - }); + ]); } } } diff --git a/src/commands/accounts/switch.ts b/src/commands/accounts/switch.ts index 18da5cea..296e059c 100644 --- a/src/commands/accounts/switch.ts +++ b/src/commands/accounts/switch.ts @@ -213,7 +213,9 @@ export default class AccountsSwitch extends ControlBaseCommand { const currentAccount = this.configManager.getCurrentAccount(); const newAlias = slugifyAccountName(remoteAccount.name); - // Store the new alias with the same OAuth tokens but different account info + // Store the new alias with the same OAuth tokens but different account info. + // Carry over the source account's controlHost so the shared session key + // resolves correctly (email + host scope). this.configManager.storeOAuthTokens( newAlias, { @@ -223,17 +225,10 @@ export default class AccountsSwitch extends ControlBaseCommand { { accountId: remoteAccount.id, accountName: remoteAccount.name, + controlHost: currentAccount?.controlHost, }, ); - // Carry over control host from the source account - if (currentAccount?.controlHost) { - this.configManager.setAccountControlHost( - newAlias, - currentAccount.controlHost, - ); - } - this.configManager.switchAccount(newAlias); this.log( diff --git a/src/services/config-manager.ts b/src/services/config-manager.ts index 555b852a..f95c81f2 100644 --- a/src/services/config-manager.ts +++ b/src/services/config-manager.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { parse, stringify } from "smol-toml"; import { extractKeyNameFromApiKey } from "../utils/api-key.js"; import isTestMode from "../utils/test-mode.js"; +import { DEFAULT_OAUTH_CONTROL_HOST } from "./oauth-client.js"; // Updated to include key and app metadata export interface AppConfig { @@ -74,7 +75,6 @@ export interface ConfigManager { ): void; switchAccount(alias: string): boolean; removeAccount(alias: string): boolean; - setAccountControlHost(alias: string, controlHost: string): void; // OAuth management storeOAuthTokens( @@ -90,6 +90,7 @@ export interface ConfigManager { accountInfo?: { accountId?: string; accountName?: string; + controlHost?: string; }, ): void; getOAuthTokens(alias?: string): @@ -102,6 +103,7 @@ export interface ConfigManager { isAccessTokenExpired(): boolean; getAuthMethod(alias?: string): "oauth" | undefined; getAliasesForOAuthSession(alias: string): string[]; + clearOAuthSession(alias?: string): void; // App management getApiKey(appId?: string): string | undefined; @@ -549,7 +551,7 @@ export class TomlConfigManager implements ConfigManager { this.saveConfig(); } - // Store OAuth tokens, shared across aliases with the same userEmail + // Store OAuth tokens, shared across aliases with the same userEmail + controlHost public storeOAuthTokens( alias: string, tokens: { @@ -563,17 +565,42 @@ export class TomlConfigManager implements ConfigManager { accountInfo?: { accountId?: string; accountName?: string; + controlHost?: string; }, ): void { const userEmail = tokens.userEmail ?? this.config.accounts[alias]?.userEmail ?? ""; - const sessionKey = userEmail.toLowerCase() || alias; + const controlHost = + accountInfo?.controlHost ?? + this.config.accounts[alias]?.controlHost ?? + DEFAULT_OAUTH_CONTROL_HOST; + const emailPart = userEmail.toLowerCase() || alias; + // Scope the session key by control host so the same email on prod and a + // staging deployment don't silently overwrite each other's refresh tokens. + const sessionKey = `${emailPart}::${controlHost.toLowerCase()}`; // Create/update the shared OAuth session if (!this.config.oauthSessions) { this.config.oauthSessions = {}; } + // Clean up the previous session entry if this account's key is changing + // (e.g. migration from a pre-controlHost key format). + const previousSessionKey = this.config.accounts[alias]?.oauthSessionKey; + if ( + previousSessionKey && + previousSessionKey !== sessionKey && + this.config.oauthSessions[previousSessionKey] + ) { + const stillReferenced = Object.entries(this.config.accounts).some( + ([otherAlias, acc]) => + otherAlias !== alias && acc.oauthSessionKey === previousSessionKey, + ); + if (!stillReferenced) { + delete this.config.oauthSessions[previousSessionKey]; + } + } + this.config.oauthSessions[sessionKey] = { accessToken: tokens.accessToken, accessTokenExpiresAt: tokens.expiresAt, @@ -592,6 +619,8 @@ export class TomlConfigManager implements ConfigManager { "", apps: this.config.accounts[alias]?.apps || {}, authMethod: "oauth", + controlHost: + accountInfo?.controlHost ?? this.config.accounts[alias]?.controlHost, currentAppId: this.config.accounts[alias]?.currentAppId, oauthSessionKey: sessionKey, userEmail, @@ -668,9 +697,34 @@ export class TomlConfigManager implements ConfigManager { return true; } - public setAccountControlHost(alias: string, controlHost: string): void { - if (!this.config.accounts[alias]) return; - this.config.accounts[alias].controlHost = controlHost; + // Clear OAuth session(s) for an alias without removing the account itself. + // Used when a refresh token has been invalidated server-side — subsequent + // commands should surface "please re-login" immediately rather than + // re-attempting refresh against a dead token. + public clearOAuthSession(alias?: string): void { + const targetAlias = alias ?? this.config.current?.account; + if (!targetAlias) return; + const account = this.config.accounts[targetAlias]; + if (!account) return; + + const sessionKey = account.oauthSessionKey; + if (sessionKey && this.config.oauthSessions?.[sessionKey]) { + const stillReferenced = Object.entries(this.config.accounts).some( + ([otherAlias, acc]) => + otherAlias !== targetAlias && acc.oauthSessionKey === sessionKey, + ); + if (!stillReferenced) { + delete this.config.oauthSessions[sessionKey]; + if (Object.keys(this.config.oauthSessions).length === 0) { + delete this.config.oauthSessions; + } + } + } + + delete account.oauthSessionKey; + delete account.accessToken; + delete account.accessTokenExpiresAt; + this.saveConfig(); } diff --git a/src/services/oauth-client.ts b/src/services/oauth-client.ts index 8d1b2c33..aaf0f6c5 100644 --- a/src/services/oauth-client.ts +++ b/src/services/oauth-client.ts @@ -1,5 +1,27 @@ import fetch from "node-fetch"; +/** + * Default OAuth control host. Shared with config-manager so the session-key + * scope matches the host actually used for token exchange. + */ +export const DEFAULT_OAUTH_CONTROL_HOST = "ably.com"; + +/** + * Thrown by refreshAccessToken when the server rejects the refresh token + * (OAuth error "invalid_grant"). This happens when: + * - the refresh token was revoked (e.g. by logout) + * - it was rotated by a concurrent refresh (single-use refresh tokens) + * - the session has otherwise expired server-side + * Callers should treat this as "session ended, re-login required" rather + * than a transient network failure. + */ +export class OAuthRefreshExpiredError extends Error { + constructor(message: string) { + super(message); + this.name = "OAuthRefreshExpiredError"; + } +} + export interface OAuthTokens { accessToken: string; expiresAt: number; @@ -168,7 +190,11 @@ export class OAuthClient { } /** - * Revoke a token (access or refresh) + * Revoke a token (access or refresh). + * + * Contract: must never reject. Callers (e.g. accounts logout) rely on this + * being best-effort so logout cannot be blocked by a network failure. + * Any errors are swallowed inside the try/catch below. */ async revokeToken(token: string): Promise { const params = new URLSearchParams({ @@ -197,7 +223,9 @@ export class OAuthClient { // --- Private helpers --- - private getOAuthConfig(controlHost = "ably.com"): OAuthConfig { + private getOAuthConfig( + controlHost = DEFAULT_OAUTH_CONTROL_HOST, + ): OAuthConfig { const host = controlHost; const scheme = host.includes("local") ? "http" : "https"; return { @@ -228,6 +256,23 @@ export class OAuthClient { if (!response.ok) { const errorBody = await response.text(); + // Detect invalid_grant on refresh: the refresh token is gone (revoked, + // rotated by a concurrent refresh, or otherwise expired). Surface this + // as a typed error so callers can prompt the user to re-login instead + // of showing a raw HTTP error body. + if (params.grant_type === "refresh_token") { + let oauthError: string | undefined; + try { + oauthError = (JSON.parse(errorBody) as { error?: string }).error; + } catch { + // Non-JSON body — fall through to generic error. + } + if (oauthError === "invalid_grant") { + throw new OAuthRefreshExpiredError( + "OAuth refresh token is no longer valid. Please run 'ably login' again.", + ); + } + } throw new Error( `${operationName} failed (${response.status}): ${errorBody}`, ); diff --git a/src/services/token-refresh-middleware.ts b/src/services/token-refresh-middleware.ts index f424cecf..d8071cb3 100644 --- a/src/services/token-refresh-middleware.ts +++ b/src/services/token-refresh-middleware.ts @@ -1,5 +1,6 @@ import type { ConfigManager } from "./config-manager.js"; -import type { OAuthClient } from "./oauth-client.js"; +import type { OAuthClient, OAuthTokens } from "./oauth-client.js"; +import { OAuthRefreshExpiredError } from "./oauth-client.js"; export class TokenExpiredError extends Error { constructor(message: string) { @@ -56,9 +57,24 @@ export class TokenRefreshMiddleware { ); } - const newTokens = await this.oauthClient.refreshAccessToken( - tokens.refreshToken, - ); + let newTokens: OAuthTokens; + try { + newTokens = await this.oauthClient.refreshAccessToken( + tokens.refreshToken, + ); + } catch (error) { + // invalid_grant typically means another CLI invocation already rotated + // this refresh token, or the session was revoked server-side. Clear + // the dead session so subsequent commands short-circuit to "please + // re-login" immediately instead of re-attempting refresh. + if (error instanceof OAuthRefreshExpiredError) { + this.configManager.clearOAuthSession(); + throw new TokenExpiredError( + "OAuth session expired (the refresh token is no longer valid, possibly because another CLI invocation refreshed concurrently). Please run 'ably login' again.", + ); + } + throw error; + } const alias = this.configManager.getCurrentAccountAlias() || "default"; this.configManager.storeOAuthTokens(alias, newTokens); return newTokens.accessToken; diff --git a/test/helpers/mock-config-manager.ts b/test/helpers/mock-config-manager.ts index 1ac76993..1edb40c9 100644 --- a/test/helpers/mock-config-manager.ts +++ b/test/helpers/mock-config-manager.ts @@ -26,6 +26,13 @@ import type { ConfigManager, } from "../../src/services/config-manager.js"; +// Kept in sync with DEFAULT_OAUTH_CONTROL_HOST in src/services/oauth-client.ts. +// Duplicated here because importing from oauth-client pulls node-fetch into +// the vitest setup graph (via test/unit/setup.ts → mock-config-manager), +// which installs node-fetch's global agent *before* nock is set up per test +// and breaks interception for commands that make HTTPS requests. +const DEFAULT_OAUTH_CONTROL_HOST = "ably.com"; + /** * Type for test configuration values. */ @@ -517,17 +524,39 @@ export class MockConfigManager implements ConfigManager { accountInfo?: { accountId?: string; accountName?: string; + controlHost?: string; }, ): void { const userEmail = tokens.userEmail ?? this.config.accounts[alias]?.userEmail ?? ""; - const sessionKey = userEmail.toLowerCase() || alias; + const controlHost = + accountInfo?.controlHost ?? + this.config.accounts[alias]?.controlHost ?? + DEFAULT_OAUTH_CONTROL_HOST; + const emailPart = userEmail.toLowerCase() || alias; + const sessionKey = `${emailPart}::${controlHost.toLowerCase()}`; // Create/update the shared OAuth session if (!this.config.oauthSessions) { this.config.oauthSessions = {}; } + // Clean up the previous session entry if this account's key is changing + const previousSessionKey = this.config.accounts[alias]?.oauthSessionKey; + if ( + previousSessionKey && + previousSessionKey !== sessionKey && + this.config.oauthSessions[previousSessionKey] + ) { + const stillReferenced = Object.entries(this.config.accounts).some( + ([otherAlias, acc]) => + otherAlias !== alias && acc.oauthSessionKey === previousSessionKey, + ); + if (!stillReferenced) { + delete this.config.oauthSessions[previousSessionKey]; + } + } + this.config.oauthSessions[sessionKey] = { accessToken: tokens.accessToken, accessTokenExpiresAt: tokens.expiresAt, @@ -546,6 +575,8 @@ export class MockConfigManager implements ConfigManager { "", apps: this.config.accounts[alias]?.apps || {}, authMethod: "oauth", + controlHost: + accountInfo?.controlHost ?? this.config.accounts[alias]?.controlHost, currentAppId: this.config.accounts[alias]?.currentAppId, oauthSessionKey: sessionKey, userEmail, @@ -613,6 +644,31 @@ export class MockConfigManager implements ConfigManager { .map(([a]) => a); } + public clearOAuthSession(alias?: string): void { + const targetAlias = alias ?? this.config.current?.account; + if (!targetAlias) return; + const account = this.config.accounts[targetAlias]; + if (!account) return; + + const sessionKey = account.oauthSessionKey; + if (sessionKey && this.config.oauthSessions?.[sessionKey]) { + const stillReferenced = Object.entries(this.config.accounts).some( + ([otherAlias, acc]) => + otherAlias !== targetAlias && acc.oauthSessionKey === sessionKey, + ); + if (!stillReferenced) { + delete this.config.oauthSessions[sessionKey]; + if (Object.keys(this.config.oauthSessions).length === 0) { + delete this.config.oauthSessions; + } + } + } + + delete account.oauthSessionKey; + delete account.accessToken; + delete account.accessTokenExpiresAt; + } + public switchAccount(alias: string): boolean { if (!this.config.accounts[alias]) { return false; @@ -625,11 +681,6 @@ export class MockConfigManager implements ConfigManager { this.config.current.account = alias; return true; } - - public setAccountControlHost(alias: string, controlHost: string): void { - if (!this.config.accounts[alias]) return; - this.config.accounts[alias].controlHost = controlHost; - } } /** diff --git a/test/unit/commands/accounts/logout.test.ts b/test/unit/commands/accounts/logout.test.ts index 1addf03e..84785e7c 100644 --- a/test/unit/commands/accounts/logout.test.ts +++ b/test/unit/commands/accounts/logout.test.ts @@ -211,12 +211,12 @@ describe("accounts:logout command", () => { accountName: "Test Account", userEmail: "test@example.com", authMethod: "oauth", - oauthSessionKey: "test@example.com", + oauthSessionKey: "test@example.com::ably.com", accessTokenExpiresAt: Date.now() + 3600000, }, }, oauthSessions: { - "test@example.com": { + "test@example.com::ably.com": { accessToken: "oauth_access_token", refreshToken: "oauth_refresh_token", accessTokenExpiresAt: Date.now() + 3600000, @@ -253,12 +253,12 @@ describe("accounts:logout command", () => { accountName: "Test Account", userEmail: "test@example.com", authMethod: "oauth", - oauthSessionKey: "test@example.com", + oauthSessionKey: "test@example.com::ably.com", accessTokenExpiresAt: Date.now() + 3600000, }, }, oauthSessions: { - "test@example.com": { + "test@example.com::ably.com": { accessToken: "oauth_access_token", refreshToken: "oauth_refresh_token", accessTokenExpiresAt: Date.now() + 3600000, diff --git a/test/unit/services/shared-oauth-session.test.ts b/test/unit/services/shared-oauth-session.test.ts index 5cdf3e1e..d4761ae6 100644 --- a/test/unit/services/shared-oauth-session.test.ts +++ b/test/unit/services/shared-oauth-session.test.ts @@ -32,8 +32,12 @@ describe("Shared OAuth session", () => { ); const config = mock.getConfig(); - expect(config.accounts["alias-a"].oauthSessionKey).toBe("user@example.com"); - expect(config.accounts["alias-b"].oauthSessionKey).toBe("user@example.com"); + expect(config.accounts["alias-a"].oauthSessionKey).toBe( + "user@example.com::ably.com", + ); + expect(config.accounts["alias-b"].oauthSessionKey).toBe( + "user@example.com::ably.com", + ); // Only one session entry expect(Object.keys(config.oauthSessions!)).toHaveLength(1); }); @@ -101,10 +105,10 @@ describe("Shared OAuth session", () => { const config = mock.getConfig(); expect(Object.keys(config.oauthSessions!)).toHaveLength(2); expect(config.accounts["alias-a"].oauthSessionKey).toBe( - "user1@example.com", + "user1@example.com::ably.com", ); expect(config.accounts["alias-b"].oauthSessionKey).toBe( - "user2@example.com", + "user2@example.com::ably.com", ); // Refreshing alias-a should NOT affect alias-b @@ -192,7 +196,7 @@ describe("Shared OAuth session", () => { // Session should still exist for alias-b const config = mock.getConfig(); - expect(config.oauthSessions?.["user@example.com"]).toBeDefined(); + expect(config.oauthSessions?.["user@example.com::ably.com"]).toBeDefined(); expect(mock.getOAuthTokens("alias-b")?.refreshToken).toBe("rt"); }); From 0819a5e890fda67ce7ce31399be9d6537140484b Mon Sep 17 00:00:00 2001 From: umair Date: Fri, 17 Apr 2026 12:18:37 +0100 Subject: [PATCH 08/19] Address OAuth review findings from ultrareview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remote ultrareview of the OAuth login flow surfaced nine issues. All non-pre-existing findings are fixed here: 1. accounts switch: persist --endpoint on the remote-account picker path so the flag is not silently dropped when selecting a remote account. 2. oauth-client: wrap requestDeviceCode and the per-poll fetch in AbortController with 15s timeouts — a hung socket used to bypass the outer deadline and spin forever. 3. accounts current: route through createControlApi so OAuth accounts get the same TokenRefreshMiddleware every other command uses (previously built ControlApi directly, skipping refresh). 4. Timer leaks: try/finally around clearTimeout in postForTokens and revokeToken; hold the revokeWithTimeout handle and clear it on settle — removes the ~5–15s process hang after logout/refresh. 5. OAuth host precedence: prefer account.controlHost over --control-host for refresh and revoke. The flag is for control-plane routing, not the OAuth issuer — a mismatch used to return invalid_grant and wipe a valid session. 6. Concurrent-refresh race: add ConfigManager.reloadConfig() and guard clearOAuthSession() with an on-disk compare. A peer that successfully rotated the refresh token no longer gets clobbered by our stale in-memory view. Includes two new middleware tests. 7. Slug collision: new pickUniqueAlias() helper auto-suffixes when a different accountId already owns the base alias. Applied to both login (JSON branch) and switch (remote-account picker); re-logging into the same account still reuses the alias. 8. login --json: warn when auto-selecting among multiple accounts, for parity with the apps/keys JSON branches. 9. config-manager: purge legacy accessToken/accessTokenExpiresAt/tokenId in storeOAuthTokens so an alias upgraded from the pre-OAuth flow does not leave a stale plaintext token at rest. Finding #10 (host.includes('local') HTTP downgrade) is pre-existing in control-api.ts and intentionally left out of scope. --- src/commands/accounts/current.ts | 12 +-- src/commands/accounts/login.ts | 60 +++++++++---- src/commands/accounts/logout.ts | 24 ++++-- src/commands/accounts/switch.ts | 24 +++++- src/control-base-command.ts | 9 +- src/services/config-manager.ts | 16 ++++ src/services/oauth-client.ts | 58 +++++++++---- src/services/token-refresh-middleware.ts | 25 ++++-- src/utils/slugify.ts | 35 ++++++++ test/helpers/mock-config-manager.ts | 11 +++ .../services/token-refresh-middleware.test.ts | 84 +++++++++++++++++++ 11 files changed, 302 insertions(+), 56 deletions(-) diff --git a/src/commands/accounts/current.ts b/src/commands/accounts/current.ts index 9fb96fb6..0d33c98e 100644 --- a/src/commands/accounts/current.ts +++ b/src/commands/accounts/current.ts @@ -6,7 +6,6 @@ import { extractKeyNameFromApiKey, } from "../../utils/api-key.js"; import { errorMessage } from "../../utils/errors.js"; -import { ControlApi } from "../../services/control-api.js"; import { formatLabel } from "../../utils/output.js"; export default class AccountsCurrent extends ControlBaseCommand { @@ -42,14 +41,11 @@ export default class AccountsCurrent extends ControlBaseCommand { ); } - // Verify the account by making an API call to get up-to-date information + // Verify the account by making an API call to get up-to-date information. + // Route through createControlApi so OAuth accounts get the same + // TokenRefreshMiddleware used by every other control command. try { - const accessToken = this.configManager.getAccessToken(); - - const controlApi = new ControlApi({ - accessToken: accessToken ?? "", - controlHost: flags["control-host"], - }); + const controlApi = this.createControlApi(flags); const { account, user } = await controlApi.getMe(); diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index f23be105..d513c85d 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -13,7 +13,7 @@ import { displayLogo } from "../../utils/logo.js"; import openUrl from "../../utils/open-url.js"; import { formatResource } from "../../utils/output.js"; import { promptForConfirmation } from "../../utils/prompt-confirmation.js"; -import { slugifyAccountName } from "../../utils/slugify.js"; +import { pickUniqueAlias, slugifyAccountName } from "../../utils/slugify.js"; function validateAndGetAlias( input: string, @@ -109,16 +109,38 @@ export default class AccountsLogin extends ControlBaseCommand { await this.interactiveHelper.selectAccountFromApi(accounts); selectedAccountInfo = picked ?? accounts[0]!; } else { - // Multiple accounts in JSON mode: use first + // Multiple accounts in JSON mode: auto-select the first but surface a + // warning so scripted callers can detect drift and choose explicitly. selectedAccountInfo = accounts[0]!; + this.logWarning( + `Multiple accounts found (${accounts.length}); auto-selected ${selectedAccountInfo.name} (${selectedAccountInfo.id}). Run 'ably accounts login' in a terminal to choose, or use --alias to pin a specific account.`, + flags, + ); } // Resolve alias AFTER account selection so we can default to account name let { alias } = flags; if (!alias && !this.shouldOutputJson(flags)) { - alias = await this.resolveAlias(selectedAccountInfo.name); + alias = await this.resolveAlias( + selectedAccountInfo.name, + selectedAccountInfo.id, + ); } else if (!alias) { - alias = slugifyAccountName(selectedAccountInfo.name); + // JSON / non-interactive: auto-suffix on collision with a different + // account so we never silently rebind an alias that already points + // somewhere else. + const picked = pickUniqueAlias( + slugifyAccountName(selectedAccountInfo.name), + selectedAccountInfo.id, + this.configManager.listAccounts(), + ); + alias = picked.alias; + if (picked.collidedWith) { + this.logWarning( + `Alias "${picked.collidedWith.alias}" is already used by a different account (${picked.collidedWith.accountId}); storing this login as "${alias}" instead.`, + flags, + ); + } } // Store OAuth tokens (include user email from /me response). @@ -404,25 +426,29 @@ export default class AccountsLogin extends ControlBaseCommand { } } - private async resolveAlias(accountName: string): Promise { + private async resolveAlias( + accountName: string, + accountId: string, + ): Promise { const defaultAlias = slugifyAccountName(accountName); const existingAccounts = this.configManager.listAccounts(); - const aliasExists = existingAccounts.some((a) => a.alias === defaultAlias); + const existing = existingAccounts.find((a) => a.alias === defaultAlias); - if (aliasExists) { - this.log( - `\nAn account with alias "${defaultAlias}" already exists and will be overwritten.`, - ); - const shouldCustomize = await promptForConfirmation( - "Would you like to use a different alias?", - ); - if (shouldCustomize) { - return this.promptForAlias(defaultAlias); - } + // No collision, or the alias already points at the same account + // (legitimate re-login) — reuse without prompting. + if (!existing || existing.account.accountId === accountId) { return defaultAlias; } - // New account — use the derived alias without prompting + this.log( + `\nAn account with alias "${defaultAlias}" already exists (account ID: ${existing.account.accountId}) and would be overwritten.`, + ); + const shouldCustomize = await promptForConfirmation( + "Would you like to use a different alias?", + ); + if (shouldCustomize) { + return this.promptForAlias(defaultAlias); + } return defaultAlias; } diff --git a/src/commands/accounts/logout.ts b/src/commands/accounts/logout.ts index 91a4f543..f0dff712 100644 --- a/src/commands/accounts/logout.ts +++ b/src/commands/accounts/logout.ts @@ -86,18 +86,32 @@ export default class AccountsLogout extends ControlBaseCommand { const targetAccount = this.configManager .listAccounts() .find((a) => a.alias === targetAlias)?.account; - const oauthHost = flags["control-host"] || targetAccount?.controlHost; + // Revoke against the host that minted the token. A --control-host + // override here would send the token to a server that never issued + // it — a no-op revocation that silently leaves the real token live. + const oauthHost = targetAccount?.controlHost ?? flags["control-host"]; const oauthClient = new OAuthClient({ controlHost: oauthHost, }); // Best-effort revocation with timeout -- don't block logout. // revokeToken() swallows its own errors and always resolves, so we // don't need a .catch() here. - const revokeWithTimeout = (token: string, timeoutMs = 5000) => - Promise.race([ + // + // We keep the timer handle and clear it once the race settles, + // otherwise a fast revocation leaves the timeout pending and the + // process hangs ~5s after printing "Successfully logged out". + const revokeWithTimeout = (token: string, timeoutMs = 5000) => { + let timer: NodeJS.Timeout | undefined; + const timeoutPromise = new Promise((resolve) => { + timer = setTimeout(resolve, timeoutMs); + }); + return Promise.race([ oauthClient.revokeToken(token), - new Promise((resolve) => setTimeout(resolve, timeoutMs)), - ]); + timeoutPromise, + ]).finally(() => { + if (timer) clearTimeout(timer); + }); + }; await Promise.all([ revokeWithTimeout(oauthTokens.accessToken), revokeWithTimeout(oauthTokens.refreshToken), diff --git a/src/commands/accounts/switch.ts b/src/commands/accounts/switch.ts index 296e059c..3e31d497 100644 --- a/src/commands/accounts/switch.ts +++ b/src/commands/accounts/switch.ts @@ -6,7 +6,7 @@ import { ControlBaseCommand } from "../../control-base-command.js"; import { endpointFlag } from "../../flags.js"; import { ControlApi, type AccountSummary } from "../../services/control-api.js"; import { formatResource } from "../../utils/output.js"; -import { slugifyAccountName } from "../../utils/slugify.js"; +import { pickUniqueAlias, slugifyAccountName } from "../../utils/slugify.js"; export default class AccountsSwitch extends ControlBaseCommand { static override args = { @@ -211,7 +211,21 @@ export default class AccountsSwitch extends ControlBaseCommand { } const currentAccount = this.configManager.getCurrentAccount(); - const newAlias = slugifyAccountName(remoteAccount.name); + // Pick a non-colliding alias — two different remote accounts whose names + // slugify identically (e.g. "Acme Prod" / "Acme-Prod") must not silently + // rebind an existing alias to a different accountId. + const picked = pickUniqueAlias( + slugifyAccountName(remoteAccount.name), + remoteAccount.id, + this.configManager.listAccounts(), + ); + const newAlias = picked.alias; + if (picked.collidedWith) { + this.logWarning( + `Alias "${picked.collidedWith.alias}" is already used by a different account (${picked.collidedWith.accountId}); storing this account as "${newAlias}" instead.`, + flags, + ); + } // Store the new alias with the same OAuth tokens but different account info. // Carry over the source account's controlHost so the shared session key @@ -231,6 +245,12 @@ export default class AccountsSwitch extends ControlBaseCommand { this.configManager.switchAccount(newAlias); + // Store custom endpoint if provided — parity with switchToLocalAccount so + // the flag is not silently dropped when the user picks a remote account. + if (flags.endpoint) { + this.configManager.storeEndpoint(flags.endpoint as string); + } + this.log( `Switched to account: ${formatResource(remoteAccount.name)} (${remoteAccount.id})`, ); diff --git a/src/control-base-command.ts b/src/control-base-command.ts index dddd81b1..0f145fe1 100644 --- a/src/control-base-command.ts +++ b/src/control-base-command.ts @@ -30,9 +30,14 @@ export abstract class ControlBaseCommand extends AblyBaseCommand { accessToken = this.configManager.getAccessToken(); - // Set up token refresh middleware for OAuth accounts + // Set up token refresh middleware for OAuth accounts. + // The OAuth issuer is an immutable property of the token — only the host + // that minted it can refresh it. Prefer the stored controlHost so a + // --control-host override (intended for control-plane routing) does not + // silently direct refresh traffic at the wrong authorization server, + // which would return invalid_grant and wipe a valid session. if (this.configManager.getAuthMethod() === "oauth") { - const oauthHost = flags["control-host"] || account.controlHost; + const oauthHost = account.controlHost ?? flags["control-host"]; const oauthClient = new OAuthClient({ controlHost: oauthHost, }); diff --git a/src/services/config-manager.ts b/src/services/config-manager.ts index f95c81f2..389685d5 100644 --- a/src/services/config-manager.ts +++ b/src/services/config-manager.ts @@ -147,6 +147,7 @@ export interface ConfigManager { // Config file getConfigPath(): string; saveConfig(): void; + reloadConfig(): void; } // Type declaration for test mocks available on globalThis @@ -410,6 +411,14 @@ export class TomlConfigManager implements ConfigManager { } } + // Re-read config from disk, discarding in-memory state. Used by the token + // refresh path to detect whether a concurrent CLI invocation has rotated + // tokens since we loaded them — otherwise we could clobber valid peer + // tokens with our stale snapshot. + public reloadConfig(): void { + this.loadConfig(); + } + // Set current app for the current account public setCurrentApp(appId: string): void { const currentAccount = this.getCurrentAccount(); @@ -626,6 +635,13 @@ export class TomlConfigManager implements ConfigManager { userEmail, }; + // Purge legacy pre-OAuth fields that the spread above may have carried + // over. They are inert for OAuth accounts but leave a stale plaintext + // token in the on-disk config. + delete this.config.accounts[alias].accessToken; + delete this.config.accounts[alias].accessTokenExpiresAt; + delete this.config.accounts[alias].tokenId; + if (!this.config.current || !this.config.current.account) { this.config.current = { account: alias }; } diff --git a/src/services/oauth-client.ts b/src/services/oauth-client.ts index aaf0f6c5..6c07299f 100644 --- a/src/services/oauth-client.ts +++ b/src/services/oauth-client.ts @@ -62,6 +62,8 @@ export class OAuthClient { /** * Request a device code from the OAuth server (RFC 8628 step 1). + * A 15s abort timeout prevents a silently hung endpoint from blocking + * `ably login` indefinitely. */ async requestDeviceCode(): Promise { const params = new URLSearchParams({ @@ -69,11 +71,19 @@ export class OAuthClient { scope: this.config.scopes.join(" "), }); - const response = await fetch(this.config.deviceCodeEndpoint, { - body: params.toString(), - headers: { "Content-Type": "application/x-www-form-urlencoded" }, - method: "POST", - }); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 15_000); + let response; + try { + response = await fetch(this.config.deviceCodeEndpoint, { + body: params.toString(), + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + method: "POST", + signal: controller.signal, + }); + } finally { + clearTimeout(timeout); + } if (!response.ok) { const errorBody = await response.text(); @@ -115,6 +125,11 @@ export class OAuthClient { } let response; + const controller = new AbortController(); + // Per-poll 15s timeout — without this a single hung fetch would block + // the outer `while (Date.now() < deadline)` guard and spin the spinner + // forever with no error. + const timeout = setTimeout(() => controller.abort(), 15_000); try { const params = new URLSearchParams({ client_id: this.config.clientId, @@ -126,6 +141,7 @@ export class OAuthClient { body: params.toString(), headers: { "Content-Type": "application/x-www-form-urlencoded" }, method: "POST", + signal: controller.signal, }); networkRetries = 0; } catch { @@ -136,6 +152,8 @@ export class OAuthClient { ); } continue; + } finally { + clearTimeout(timeout); } if (response.ok) { @@ -202,18 +220,22 @@ export class OAuthClient { token, }); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); try { - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), 10_000); await fetch(this.config.revocationEndpoint, { body: params.toString(), headers: { "Content-Type": "application/x-www-form-urlencoded" }, method: "POST", signal: controller.signal, }); - clearTimeout(timeout); } catch { // Best-effort revocation -- don't block on failure + } finally { + // Always clear the timer, even on fetch rejection, or the pending + // setTimeout keeps the event loop alive for up to 10s after the + // command has otherwise finished. + clearTimeout(timeout); } } @@ -246,13 +268,19 @@ export class OAuthClient { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 15_000); - const response = await fetch(this.config.tokenEndpoint, { - body: body.toString(), - headers: { "Content-Type": "application/x-www-form-urlencoded" }, - method: "POST", - signal: controller.signal, - }); - clearTimeout(timeout); + let response; + try { + response = await fetch(this.config.tokenEndpoint, { + body: body.toString(), + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + method: "POST", + signal: controller.signal, + }); + } finally { + // Always clear the timer — on network/TLS failure the exception + // propagates past clearTimeout and the setTimeout stays pending. + clearTimeout(timeout); + } if (!response.ok) { const errorBody = await response.text(); diff --git a/src/services/token-refresh-middleware.ts b/src/services/token-refresh-middleware.ts index d8071cb3..dcce1117 100644 --- a/src/services/token-refresh-middleware.ts +++ b/src/services/token-refresh-middleware.ts @@ -57,18 +57,29 @@ export class TokenRefreshMiddleware { ); } + const consumedRefreshToken = tokens.refreshToken; let newTokens: OAuthTokens; try { - newTokens = await this.oauthClient.refreshAccessToken( - tokens.refreshToken, - ); + newTokens = + await this.oauthClient.refreshAccessToken(consumedRefreshToken); } catch (error) { // invalid_grant typically means another CLI invocation already rotated - // this refresh token, or the session was revoked server-side. Clear - // the dead session so subsequent commands short-circuit to "please - // re-login" immediately instead of re-attempting refresh. + // this refresh token, or the session was revoked server-side. Before + // clearing the session, reload the on-disk config: a concurrent peer + // may have just rotated the token and persisted a fresh refresh token. + // Clobbering that by saving our stale in-memory view would destroy a + // valid session, turning a single-process failure into a fleet-wide + // "please re-login" for every invocation. if (error instanceof OAuthRefreshExpiredError) { - this.configManager.clearOAuthSession(); + this.configManager.reloadConfig(); + const onDisk = this.configManager.getOAuthTokens(); + // Only clear when disk still holds the refresh token we just tried + // (or the session is already gone). A different refresh token on + // disk means a peer rotated it successfully — their session is + // valid and must be preserved. + if (!onDisk || onDisk.refreshToken === consumedRefreshToken) { + this.configManager.clearOAuthSession(); + } throw new TokenExpiredError( "OAuth session expired (the refresh token is no longer valid, possibly because another CLI invocation refreshed concurrently). Please run 'ably login' again.", ); diff --git a/src/utils/slugify.ts b/src/utils/slugify.ts index fd85a96d..b473eaab 100644 --- a/src/utils/slugify.ts +++ b/src/utils/slugify.ts @@ -15,3 +15,38 @@ export function slugifyAccountName(name: string): string { } return slug; } + +/** + * Resolve a non-colliding alias for an OAuth account. + * + * Two distinct accounts whose names slugify to the same string would + * otherwise overwrite each other silently (e.g. "Acme Prod" and "Acme-Prod" + * both slugify to "acme-prod"). Re-logging into the *same* account (same + * accountId) is treated as a legitimate refresh and reuses the base alias. + * + * Returns the base alias when safe, or an auto-suffixed variant + * (`base-2`, `base-3`, ...) when a different account already owns the base. + */ +export function pickUniqueAlias( + desiredBase: string, + targetAccountId: string, + existingAccounts: { account: { accountId?: string }; alias: string }[], +): { alias: string; collidedWith?: { accountId?: string; alias: string } } { + const existing = existingAccounts.find((a) => a.alias === desiredBase); + if (!existing || existing.account.accountId === targetAccountId) { + return { alias: desiredBase }; + } + + const aliases = new Set(existingAccounts.map((a) => a.alias)); + let suffix = 2; + while (aliases.has(`${desiredBase}-${suffix}`)) { + suffix++; + } + return { + alias: `${desiredBase}-${suffix}`, + collidedWith: { + accountId: existing.account.accountId, + alias: desiredBase, + }, + }; +} diff --git a/test/helpers/mock-config-manager.ts b/test/helpers/mock-config-manager.ts index 1edb40c9..59828dc6 100644 --- a/test/helpers/mock-config-manager.ts +++ b/test/helpers/mock-config-manager.ts @@ -389,6 +389,10 @@ export class MockConfigManager implements ConfigManager { // No-op for in-memory implementation } + public reloadConfig(): void { + // No-op: in-memory mock has no on-disk state to reload. + } + public setCurrentApp(appId: string): void { const currentAccount = this.getCurrentAccount(); const currentAlias = this.getCurrentAccountAlias(); @@ -582,6 +586,13 @@ export class MockConfigManager implements ConfigManager { userEmail, }; + // Purge legacy pre-OAuth fields that the spread above may have carried + // over. They are inert for OAuth accounts but leave a stale plaintext + // token in the on-disk config. + delete this.config.accounts[alias].accessToken; + delete this.config.accounts[alias].accessTokenExpiresAt; + delete this.config.accounts[alias].tokenId; + if (!this.config.current || !this.config.current.account) { this.config.current = { account: alias }; } diff --git a/test/unit/services/token-refresh-middleware.test.ts b/test/unit/services/token-refresh-middleware.test.ts index 56f1fbee..ecff439e 100644 --- a/test/unit/services/token-refresh-middleware.test.ts +++ b/test/unit/services/token-refresh-middleware.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi } from "vitest"; +import { OAuthRefreshExpiredError } from "../../../src/services/oauth-client.js"; import { TokenRefreshMiddleware, TokenExpiredError, @@ -6,11 +7,13 @@ import { function createMockConfigManager(overrides: Record = {}) { return { + clearOAuthSession: vi.fn(), getAccessToken: vi.fn().mockReturnValue("current_access_token"), getAuthMethod: vi.fn(), getCurrentAccountAlias: vi.fn().mockReturnValue("default"), getOAuthTokens: vi.fn(), isAccessTokenExpired: vi.fn().mockReturnValue(false), + reloadConfig: vi.fn(), storeOAuthTokens: vi.fn(), ...overrides, }; @@ -184,4 +187,85 @@ describe("TokenRefreshMiddleware", () => { ); }); }); + + describe("invalid_grant race with concurrent refresh", () => { + it("clears session when on-disk refresh token still matches the consumed one", async () => { + const consumed = "consumed_refresh_token"; + const getOAuthTokens = vi.fn().mockReturnValue({ + accessToken: "expired_access", + refreshToken: consumed, + expiresAt: Date.now() - 1000, + }); + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn().mockReturnValue("oauth"), + getAccessToken: vi.fn().mockReturnValue("expired_access"), + isAccessTokenExpired: vi.fn().mockReturnValue(true), + getOAuthTokens, + getCurrentAccountAlias: vi.fn().mockReturnValue("default"), + }); + const oauthClient = createMockOAuthClient({ + refreshAccessToken: vi + .fn() + .mockRejectedValue( + new OAuthRefreshExpiredError("refresh token invalid"), + ), + }); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + await expect(middleware.getValidAccessToken()).rejects.toThrow( + TokenExpiredError, + ); + expect(configManager.reloadConfig).toHaveBeenCalled(); + expect(configManager.clearOAuthSession).toHaveBeenCalled(); + }); + + it("does NOT clear session when a peer rotated the refresh token on disk", async () => { + const consumed = "stale_refresh_token"; + const peerRotated = "fresh_refresh_token_from_peer"; + // First call (before refresh): returns the consumed token. + // Second call (after reload): returns the peer's fresh token. + const getOAuthTokens = vi + .fn() + .mockReturnValueOnce({ + accessToken: "expired_access", + refreshToken: consumed, + expiresAt: Date.now() - 1000, + }) + .mockReturnValueOnce({ + accessToken: "peer_fresh_access", + refreshToken: peerRotated, + expiresAt: Date.now() + 3600_000, + }); + const configManager = createMockConfigManager({ + getAuthMethod: vi.fn().mockReturnValue("oauth"), + getAccessToken: vi.fn().mockReturnValue("expired_access"), + isAccessTokenExpired: vi.fn().mockReturnValue(true), + getOAuthTokens, + getCurrentAccountAlias: vi.fn().mockReturnValue("default"), + }); + const oauthClient = createMockOAuthClient({ + refreshAccessToken: vi + .fn() + .mockRejectedValue( + new OAuthRefreshExpiredError("refresh token invalid"), + ), + }); + + const middleware = new TokenRefreshMiddleware( + configManager as never, + oauthClient as never, + ); + + await expect(middleware.getValidAccessToken()).rejects.toThrow( + TokenExpiredError, + ); + expect(configManager.reloadConfig).toHaveBeenCalled(); + // Critical: must NOT clear — peer's session is valid. + expect(configManager.clearOAuthSession).not.toHaveBeenCalled(); + }); + }); }); From 2cf30d29dd859894d7c6bcf02c14ef9d427bebb9 Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 20 Apr 2026 17:21:25 +0100 Subject: [PATCH 09/19] Separate OAuth authorization host from Control API host Addresses AndyTWF review feedback on PR #144. The OAuth issuer (ably.com) and the Control API (control.ably.net) are distinct services, and conflating them under --control-host broke login (the flag was being sent to the OAuth server even though the /me call afterward lives on the control API) and caused refresh traffic to be misrouted when --control-host was used for legitimate control-plane overrides. Fixes by review comment: - New --oauth-host flag (hidden, ABLY_OAUTH_HOST) separate from --control-host. login, logout, switch, and refresh all target the OAuth server via oauth-host and the Control API via control-host, independently. Session-key scope is now (userEmail :: oauthHost) since the issuer is what actually mints the tokens. Account config gains oauthHost alongside controlHost; both are preserved on login and carried over on switch. - ControlApi URL routing no longer hard-codes "local"/"control.ably.net". It now routes /v1/ for hosts whose first label starts with "control" (matches control. and control-* variants, case-insensitive) and /api/v1/ otherwise, so Heroku review apps and website-proxied environments work. - pollForToken handles HTTP 429 with exponential backoff (capped at 30s) before attempting to parse the error body, since the website returns the nested Control API error envelope for rate limits rather than an OAuth-compliant slow_down. New parsePollingError helper accepts both the OAuth-compliant string shape and the nested Control-API envelope, so error output no longer renders as "[object Object]". Probed the real /oauth/token endpoint and confirmed the website emits `{error: {message, statusCode}}` with `statusCode` as a string and no `code` field. parsePollingError reads `statusCode` as a fallback for `code` via a shared stringify helper (string or number). Comment and tests reflect the real observed shape, with a separate defensive test for the documented `{code, message, statusCode}` form so the fallback path stays covered. - Device-code polling applies +/-20% jitter so concurrent clients do not synchronize into lockstep bursts against the shared rate limit. - revokeToken now rejects on network failure, timeout, or non-2xx response, and accepts an external AbortSignal. logout links an AbortController to the timer so a timed-out revocation actually cancels the in-flight fetch (previously the fetch leaked in the background), and surfaces a logWarning so users know the token may remain valid until its natural expiry. - Added an RFC 8628 / RFC 6749 note on the embedded client_id explaining that the device flow uses a public client with no secret and the client_id is not confidential. - Spinner success/failure messages end with a full stop for consistency with the rest of the CLI. Post-review tidy-ups: - login.ts no longer re-spreads oauthHostFlag (already inherited via ControlBaseCommand.globalFlags). - Test coverage added: oauth-host vs control-host routing, 429 backoff (real envelope shape), non-2xx revocation, external AbortSignal cancellation, nested error envelope parsing, no "[object Object]" regression. --- src/commands/accounts/login.ts | 13 +- src/commands/accounts/logout.ts | 56 +++++--- src/commands/accounts/switch.ts | 6 +- src/control-base-command.ts | 14 +- src/flags.ts | 15 ++ src/services/config-manager.ts | 29 ++-- src/services/control-api.ts | 12 +- src/services/oauth-client.ts | 165 +++++++++++++++++----- src/types/cli.ts | 1 + test/helpers/mock-config-manager.ts | 17 ++- test/unit/commands/accounts/login.test.ts | 36 +++-- test/unit/services/control-api.test.ts | 4 +- test/unit/services/oauth-client.test.ts | 112 ++++++++++++++- 13 files changed, 372 insertions(+), 108 deletions(-) diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index d513c85d..2b6ce5f0 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -144,8 +144,10 @@ export default class AccountsLogin extends ControlBaseCommand { } // Store OAuth tokens (include user email from /me response). - // Pass controlHost so the session key is scoped per endpoint — otherwise - // the same email on prod and a staging deployment would collide. + // Pass oauthHost so the session key is scoped per authorization server + // — otherwise the same email on prod and a review deployment would + // collide. Also preserve controlHost so later commands talk to the same + // Control API deployment the user picked at login. this.configManager.storeOAuthTokens( alias, { ...oauthTokens, userEmail: user.email }, @@ -153,6 +155,7 @@ export default class AccountsLogin extends ControlBaseCommand { accountId: selectedAccountInfo.id, accountName: selectedAccountInfo.name, controlHost: flags["control-host"], + oauthHost: flags["oauth-host"], }, ); @@ -374,7 +377,7 @@ export default class AccountsLogin extends ControlBaseCommand { private async oauthLogin(flags: BaseFlags): Promise { const oauthClient = new OAuthClient({ - controlHost: flags["control-host"], + oauthHost: flags["oauth-host"], }); const deviceResponse = await oauthClient.requestDeviceCode(); @@ -418,10 +421,10 @@ export default class AccountsLogin extends ControlBaseCommand { deviceResponse.expiresIn, ); - spinner?.succeed("Authentication successful!"); + spinner?.succeed("Authentication successful."); return tokens; } catch (error) { - spinner?.fail("Authentication failed"); + spinner?.fail("Authentication failed."); throw error; } } diff --git a/src/commands/accounts/logout.ts b/src/commands/accounts/logout.ts index f0dff712..53fb0cb8 100644 --- a/src/commands/accounts/logout.ts +++ b/src/commands/accounts/logout.ts @@ -89,32 +89,44 @@ export default class AccountsLogout extends ControlBaseCommand { // Revoke against the host that minted the token. A --control-host // override here would send the token to a server that never issued // it — a no-op revocation that silently leaves the real token live. - const oauthHost = targetAccount?.controlHost ?? flags["control-host"]; + const oauthHost = targetAccount?.oauthHost ?? flags["oauth-host"]; const oauthClient = new OAuthClient({ - controlHost: oauthHost, + oauthHost, }); - // Best-effort revocation with timeout -- don't block logout. - // revokeToken() swallows its own errors and always resolves, so we - // don't need a .catch() here. - // - // We keep the timer handle and clear it once the race settles, - // otherwise a fast revocation leaves the timeout pending and the - // process hangs ~5s after printing "Successfully logged out". - const revokeWithTimeout = (token: string, timeoutMs = 5000) => { - let timer: NodeJS.Timeout | undefined; - const timeoutPromise = new Promise((resolve) => { - timer = setTimeout(resolve, timeoutMs); - }); - return Promise.race([ - oauthClient.revokeToken(token), - timeoutPromise, - ]).finally(() => { - if (timer) clearTimeout(timer); - }); + // Abortable best-effort revocation. The external AbortController + // cancels the in-flight fetch when the timer fires (unlike the old + // Promise.race which let the request complete in the background), + // and surfacing the reason lets us warn the user that revocation + // did not complete so they know the token may still be live until + // its natural expiry. + const revokeWithTimeout = async ( + token: string, + label: string, + timeoutMs = 5000, + ): Promise => { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { + await oauthClient.revokeToken(token, { + signal: controller.signal, + }); + } catch (error) { + const reason = controller.signal.aborted + ? `timed out after ${timeoutMs}ms` + : error instanceof Error + ? error.message + : String(error); + this.logWarning( + `Failed to revoke ${label} token (${reason}); it may remain valid until its natural expiry.`, + flags, + ); + } finally { + clearTimeout(timer); + } }; await Promise.all([ - revokeWithTimeout(oauthTokens.accessToken), - revokeWithTimeout(oauthTokens.refreshToken), + revokeWithTimeout(oauthTokens.accessToken, "access"), + revokeWithTimeout(oauthTokens.refreshToken, "refresh"), ]); } } diff --git a/src/commands/accounts/switch.ts b/src/commands/accounts/switch.ts index 3e31d497..041dd66c 100644 --- a/src/commands/accounts/switch.ts +++ b/src/commands/accounts/switch.ts @@ -228,8 +228,9 @@ export default class AccountsSwitch extends ControlBaseCommand { } // Store the new alias with the same OAuth tokens but different account info. - // Carry over the source account's controlHost so the shared session key - // resolves correctly (email + host scope). + // Carry over the source account's oauthHost (so the shared session key + // resolves correctly) and controlHost (so later Control API calls keep + // targeting the same deployment). this.configManager.storeOAuthTokens( newAlias, { @@ -240,6 +241,7 @@ export default class AccountsSwitch extends ControlBaseCommand { accountId: remoteAccount.id, accountName: remoteAccount.name, controlHost: currentAccount?.controlHost, + oauthHost: currentAccount?.oauthHost, }, ); diff --git a/src/control-base-command.ts b/src/control-base-command.ts index 0f145fe1..c496a442 100644 --- a/src/control-base-command.ts +++ b/src/control-base-command.ts @@ -1,5 +1,5 @@ import { AblyBaseCommand } from "./base-command.js"; -import { controlApiFlags } from "./flags.js"; +import { controlApiFlags, oauthHostFlag } from "./flags.js"; import { ControlApi, App } from "./services/control-api.js"; import { OAuthClient } from "./services/oauth-client.js"; import { TokenRefreshMiddleware } from "./services/token-refresh-middleware.js"; @@ -8,8 +8,10 @@ import { errorMessage } from "./utils/errors.js"; import isWebCliMode from "./utils/web-mode.js"; export abstract class ControlBaseCommand extends AblyBaseCommand { - // Control API commands get core + hidden control API flags - static globalFlags = { ...controlApiFlags }; + // Control API commands get core + hidden control API flags + oauth-host + // (so token refresh can target the authorization server that minted the + // token even when --control-host points elsewhere). + static globalFlags = { ...controlApiFlags, ...oauthHostFlag }; /** * Create a Control API instance for making requests @@ -32,14 +34,14 @@ export abstract class ControlBaseCommand extends AblyBaseCommand { // Set up token refresh middleware for OAuth accounts. // The OAuth issuer is an immutable property of the token — only the host - // that minted it can refresh it. Prefer the stored controlHost so a + // that minted it can refresh it. Prefer the stored oauthHost so a // --control-host override (intended for control-plane routing) does not // silently direct refresh traffic at the wrong authorization server, // which would return invalid_grant and wipe a valid session. if (this.configManager.getAuthMethod() === "oauth") { - const oauthHost = account.controlHost ?? flags["control-host"]; + const oauthHost = account.oauthHost ?? flags["oauth-host"]; const oauthClient = new OAuthClient({ - controlHost: oauthHost, + oauthHost, }); tokenRefreshMiddleware = new TokenRefreshMiddleware( this.configManager, diff --git a/src/flags.ts b/src/flags.ts index 21bfed1a..f88a681f 100644 --- a/src/flags.ts +++ b/src/flags.ts @@ -71,6 +71,21 @@ export const clientIdFlag = { }), }; +/** + * Hidden oauth-host flag for overriding the OAuth authorization server host. + * Kept separate from --control-host because the OAuth server (ably.com) and + * the Control API (control.ably.net) are different services and may be + * overridden independently when targeting review/staging environments. + */ +export const oauthHostFlag = { + "oauth-host": Flags.string({ + description: + "Override the host for the OAuth authorization server, which defaults to ably.com", + hidden: process.env.ABLY_SHOW_DEV_FLAGS !== "true", + env: "ABLY_OAUTH_HOST", + }), +}; + /** * endpoint flag for login / accounts switch commands only. */ diff --git a/src/services/config-manager.ts b/src/services/config-manager.ts index 389685d5..4c7b45f4 100644 --- a/src/services/config-manager.ts +++ b/src/services/config-manager.ts @@ -4,7 +4,7 @@ import path from "node:path"; import { parse, stringify } from "smol-toml"; import { extractKeyNameFromApiKey } from "../utils/api-key.js"; import isTestMode from "../utils/test-mode.js"; -import { DEFAULT_OAUTH_CONTROL_HOST } from "./oauth-client.js"; +import { DEFAULT_OAUTH_HOST } from "./oauth-client.js"; // Updated to include key and app metadata export interface AppConfig { @@ -29,6 +29,10 @@ export interface AccountConfig { controlHost?: string; currentAppId?: string; endpoint?: string; + // OAuth authorization server host (ably.com or a review-app override). + // Kept separate from controlHost so the session key and token-refresh + // traffic always targets the host that actually minted the tokens. + oauthHost?: string; oauthSessionKey?: string; tokenId?: string; userEmail: string; @@ -91,6 +95,7 @@ export interface ConfigManager { accountId?: string; accountName?: string; controlHost?: string; + oauthHost?: string; }, ): void; getOAuthTokens(alias?: string): @@ -560,7 +565,7 @@ export class TomlConfigManager implements ConfigManager { this.saveConfig(); } - // Store OAuth tokens, shared across aliases with the same userEmail + controlHost + // Store OAuth tokens, shared across aliases with the same userEmail + oauthHost public storeOAuthTokens( alias: string, tokens: { @@ -575,18 +580,20 @@ export class TomlConfigManager implements ConfigManager { accountId?: string; accountName?: string; controlHost?: string; + oauthHost?: string; }, ): void { const userEmail = tokens.userEmail ?? this.config.accounts[alias]?.userEmail ?? ""; - const controlHost = - accountInfo?.controlHost ?? - this.config.accounts[alias]?.controlHost ?? - DEFAULT_OAUTH_CONTROL_HOST; + const oauthHost = + accountInfo?.oauthHost ?? + this.config.accounts[alias]?.oauthHost ?? + DEFAULT_OAUTH_HOST; const emailPart = userEmail.toLowerCase() || alias; - // Scope the session key by control host so the same email on prod and a - // staging deployment don't silently overwrite each other's refresh tokens. - const sessionKey = `${emailPart}::${controlHost.toLowerCase()}`; + // Scope the session key by OAuth host so the same email on prod and a + // review app don't silently overwrite each other's refresh tokens — the + // issuer is the host that actually minted them. + const sessionKey = `${emailPart}::${oauthHost.toLowerCase()}`; // Create/update the shared OAuth session if (!this.config.oauthSessions) { @@ -594,7 +601,7 @@ export class TomlConfigManager implements ConfigManager { } // Clean up the previous session entry if this account's key is changing - // (e.g. migration from a pre-controlHost key format). + // (e.g. migration from a pre-oauthHost key format). const previousSessionKey = this.config.accounts[alias]?.oauthSessionKey; if ( previousSessionKey && @@ -631,6 +638,8 @@ export class TomlConfigManager implements ConfigManager { controlHost: accountInfo?.controlHost ?? this.config.accounts[alias]?.controlHost, currentAppId: this.config.accounts[alias]?.currentAppId, + oauthHost: + accountInfo?.oauthHost ?? this.config.accounts[alias]?.oauthHost, oauthSessionKey: sessionKey, userEmail, }; diff --git a/src/services/control-api.ts b/src/services/control-api.ts index 11c50c34..ea2442f0 100644 --- a/src/services/control-api.ts +++ b/src/services/control-api.ts @@ -566,9 +566,15 @@ export class ControlApi { await this.tokenRefreshMiddleware.getValidAccessToken(); } - const url = this.controlHost.includes("local") - ? `http://${this.controlHost}/api/v1${path}` - : `https://${this.controlHost}/v1${path}`; + // The dedicated Control API service (control.ably.net) serves at `/v1/`. + // The website itself (ably.com and Heroku review apps) proxies the + // Control API at `/api/v1/`. Match hosts whose first label starts with + // "control" so both `control.` and `control-*.` variants route correctly, + // case insensitively so ABLY_CONTROL_HOST values aren't locale-sensitive. + const isControlService = /^control[-.]/i.test(this.controlHost); + const scheme = this.controlHost.includes("local") ? "http" : "https"; + const prefix = isControlService ? "/v1" : "/api/v1"; + const url = `${scheme}://${this.controlHost}${prefix}${path}`; const isFormData = body instanceof FormData; const options: RequestInit = { diff --git a/src/services/oauth-client.ts b/src/services/oauth-client.ts index 6c07299f..01f0780b 100644 --- a/src/services/oauth-client.ts +++ b/src/services/oauth-client.ts @@ -1,10 +1,12 @@ -import fetch from "node-fetch"; +import fetch, { type Response as FetchResponse } from "node-fetch"; /** - * Default OAuth control host. Shared with config-manager so the session-key - * scope matches the host actually used for token exchange. + * Default OAuth authorization-server host. Shared with config-manager so the + * session-key scope matches the host that actually minted the tokens. Kept + * distinct from the Control API host (control.ably.net) — they are separate + * services that happen to share the ably.com brand. */ -export const DEFAULT_OAUTH_CONTROL_HOST = "ably.com"; +export const DEFAULT_OAUTH_HOST = "ably.com"; /** * Thrown by refreshAccessToken when the server rejects the refresh token @@ -41,7 +43,7 @@ export interface OAuthConfig { } export interface OAuthClientOptions { - controlHost?: string; + oauthHost?: string; } export interface DeviceCodeResponse { @@ -57,7 +59,7 @@ export class OAuthClient { private config: OAuthConfig; constructor(options: OAuthClientOptions = {}) { - this.config = this.getOAuthConfig(options.controlHost); + this.config = this.getOAuthConfig(options.oauthHost); } /** @@ -118,7 +120,11 @@ export class OAuthClient { const maxNetworkRetries = 3; while (Date.now() < deadline) { - await this.sleep(currentInterval * 1000); + // Apply ±20% jitter so concurrent clients don't fall into lockstep and + // hit the authorization server in synchronized bursts, which trips the + // shared rate limit long before any individual client is misbehaving. + const jitterFactor = 0.9 + Math.random() * 0.2; + await this.sleep(currentInterval * 1000 * jitterFactor); if (Date.now() >= deadline) { throw new Error("Device code expired"); @@ -161,32 +167,44 @@ export class OAuthClient { return this.parseTokenResponse(data); } - let error: string; - try { - const errorData = (await response.json()) as Record; - error = errorData.error as string; - } catch { - throw new Error(`Token polling failed with status ${response.status}`); + // Handle rate limiting before attempting to parse the error body. An + // OAuth-compliant server would return error=slow_down, but the current + // website reuses the Control API error envelope for 429s, so we bail + // out to backoff on status alone and don't depend on a specific shape. + if (response.status === 429) { + currentInterval = Math.min(currentInterval * 2, 30); + // Best-effort consume body so the socket can be released even if the + // server wrote one; failures here are irrelevant. + try { + await response.text(); + } catch { + // ignore + } + continue; } - if (error === "authorization_pending") { + const { code: errorCode, message: errorMessage } = + await parsePollingError(response); + + if (errorCode === "authorization_pending") { continue; } - if (error === "slow_down") { + if (errorCode === "slow_down") { currentInterval += 5; continue; } - if (error === "expired_token") { + if (errorCode === "expired_token") { throw new Error("Device code expired"); } - if (error === "access_denied") { + if (errorCode === "access_denied") { throw new Error("Authorization denied"); } - throw new Error(`Token polling failed: ${error}`); + const reason = errorMessage ?? errorCode ?? `HTTP ${response.status}`; + throw new Error(`Token polling failed: ${reason}`); } throw new Error("Device code expired"); @@ -210,32 +228,58 @@ export class OAuthClient { /** * Revoke a token (access or refresh). * - * Contract: must never reject. Callers (e.g. accounts logout) rely on this - * being best-effort so logout cannot be blocked by a network failure. - * Any errors are swallowed inside the try/catch below. + * Rejects on network failure, timeout, or non-2xx response. Callers that + * want best-effort behaviour (e.g. accounts logout) must catch the rejection + * themselves — surfacing it lets the caller distinguish a successful revoke + * from a timed-out one so it can warn the user. + * + * Pass an external `signal` to abort the in-flight fetch from the outside + * (the internal 10s timeout is still applied as a safety net). */ - async revokeToken(token: string): Promise { + async revokeToken( + token: string, + options: { signal?: AbortSignal; timeoutMs?: number } = {}, + ): Promise { const params = new URLSearchParams({ client_id: this.config.clientId, token, }); const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), 10_000); + const timeoutMs = options.timeoutMs ?? 10_000; + const timeout = setTimeout(() => controller.abort(), timeoutMs); + + // Link the caller-supplied signal to our internal controller so aborting + // the outer signal also cancels the fetch. + const externalSignal = options.signal; + const onExternalAbort = () => controller.abort(); + if (externalSignal) { + if (externalSignal.aborted) { + controller.abort(); + } else { + externalSignal.addEventListener("abort", onExternalAbort); + } + } + try { - await fetch(this.config.revocationEndpoint, { + const response = await fetch(this.config.revocationEndpoint, { body: params.toString(), headers: { "Content-Type": "application/x-www-form-urlencoded" }, method: "POST", signal: controller.signal, }); - } catch { - // Best-effort revocation -- don't block on failure + if (!response.ok) { + const errorBody = await response.text().catch(() => ""); + throw new Error( + `Token revocation failed (${response.status})${errorBody ? `: ${errorBody}` : ""}`, + ); + } } finally { // Always clear the timer, even on fetch rejection, or the pending // setTimeout keeps the event loop alive for up to 10s after the // command has otherwise finished. clearTimeout(timeout); + externalSignal?.removeEventListener("abort", onExternalAbort); } } @@ -245,17 +289,18 @@ export class OAuthClient { // --- Private helpers --- - private getOAuthConfig( - controlHost = DEFAULT_OAUTH_CONTROL_HOST, - ): OAuthConfig { - const host = controlHost; - const scheme = host.includes("local") ? "http" : "https"; + private getOAuthConfig(oauthHost = DEFAULT_OAUTH_HOST): OAuthConfig { + const scheme = oauthHost.includes("local") ? "http" : "https"; return { + // Per RFC 8628 §3.1 and RFC 6749 §2.1, the device flow uses a public + // client — there is no client secret, and the client_id is not + // confidential. It is intentionally embedded in the binary; distributing + // it publicly does not weaken the security of the flow. clientId: "gb-I8-bZRnXs-gF83jOWKQrUxPPWp_ldTfQtgGP0EFg", - deviceCodeEndpoint: `${scheme}://${host}/oauth/authorize_device`, - revocationEndpoint: `${scheme}://${host}/oauth/revoke`, + deviceCodeEndpoint: `${scheme}://${oauthHost}/oauth/authorize_device`, + revocationEndpoint: `${scheme}://${oauthHost}/oauth/revoke`, scopes: ["full_access"], - tokenEndpoint: `${scheme}://${host}/oauth/token`, + tokenEndpoint: `${scheme}://${oauthHost}/oauth/token`, }; } @@ -346,3 +391,55 @@ export class OAuthClient { return new Promise((resolve) => setTimeout(resolve, ms)); } } + +/** + * Extract an error code + human message from a polling-endpoint error body. + * + * The OAuth-compliant shape per RFC 6749 §5.2 is `{error: "slow_down"}` with + * an optional `error_description`. The website currently does NOT use this + * shape for non-OAuth-specific failures (rate limits, 404s, etc.); it reuses + * the Control API envelope `{error: {message, statusCode}}`. Observed in + * practice: `statusCode` is a string (e.g. "429") and there is no `code` + * field — but we still read `code` defensively in case the website is later + * brought into line with the Control API's documented shape + * `{error: {code, message, statusCode}}`. + * + * Never throws — parse failures degrade to undefined. + */ +async function parsePollingError( + response: FetchResponse, +): Promise<{ code?: string; message?: string }> { + let errorData: Record; + try { + errorData = (await response.json()) as Record; + } catch { + return {}; + } + + const raw = errorData.error; + if (typeof raw === "string") { + return { + code: raw, + message: + typeof errorData.error_description === "string" + ? errorData.error_description + : undefined, + }; + } + + if (raw && typeof raw === "object") { + const nested = raw as Record; + const stringify = (v: unknown) => + typeof v === "string" ? v : typeof v === "number" ? String(v) : undefined; + // Prefer `code` if the website ever emits it, then fall back to + // `statusCode` (what it actually emits today). + const code = stringify(nested.code) ?? stringify(nested.statusCode); + const message = + typeof nested.message === "string" ? nested.message : undefined; + return { code, message }; + } + + const topLevelMessage = + typeof errorData.message === "string" ? errorData.message : undefined; + return { message: topLevelMessage }; +} diff --git a/src/types/cli.ts b/src/types/cli.ts index 82238f82..03fec39a 100644 --- a/src/types/cli.ts +++ b/src/types/cli.ts @@ -8,6 +8,7 @@ export interface BaseFlags { "client-id"?: string; "control-host"?: string; "dashboard-host"?: string; + "oauth-host"?: string; endpoint?: string; port?: number; tls?: string; diff --git a/test/helpers/mock-config-manager.ts b/test/helpers/mock-config-manager.ts index 59828dc6..7d566c75 100644 --- a/test/helpers/mock-config-manager.ts +++ b/test/helpers/mock-config-manager.ts @@ -26,12 +26,12 @@ import type { ConfigManager, } from "../../src/services/config-manager.js"; -// Kept in sync with DEFAULT_OAUTH_CONTROL_HOST in src/services/oauth-client.ts. +// Kept in sync with DEFAULT_OAUTH_HOST in src/services/oauth-client.ts. // Duplicated here because importing from oauth-client pulls node-fetch into // the vitest setup graph (via test/unit/setup.ts → mock-config-manager), // which installs node-fetch's global agent *before* nock is set up per test // and breaks interception for commands that make HTTPS requests. -const DEFAULT_OAUTH_CONTROL_HOST = "ably.com"; +const DEFAULT_OAUTH_HOST = "ably.com"; /** * Type for test configuration values. @@ -529,16 +529,17 @@ export class MockConfigManager implements ConfigManager { accountId?: string; accountName?: string; controlHost?: string; + oauthHost?: string; }, ): void { const userEmail = tokens.userEmail ?? this.config.accounts[alias]?.userEmail ?? ""; - const controlHost = - accountInfo?.controlHost ?? - this.config.accounts[alias]?.controlHost ?? - DEFAULT_OAUTH_CONTROL_HOST; + const oauthHost = + accountInfo?.oauthHost ?? + this.config.accounts[alias]?.oauthHost ?? + DEFAULT_OAUTH_HOST; const emailPart = userEmail.toLowerCase() || alias; - const sessionKey = `${emailPart}::${controlHost.toLowerCase()}`; + const sessionKey = `${emailPart}::${oauthHost.toLowerCase()}`; // Create/update the shared OAuth session if (!this.config.oauthSessions) { @@ -582,6 +583,8 @@ export class MockConfigManager implements ConfigManager { controlHost: accountInfo?.controlHost ?? this.config.accounts[alias]?.controlHost, currentAppId: this.config.accounts[alias]?.currentAppId, + oauthHost: + accountInfo?.oauthHost ?? this.config.accounts[alias]?.oauthHost, oauthSessionKey: sessionKey, userEmail, }; diff --git a/test/unit/commands/accounts/login.test.ts b/test/unit/commands/accounts/login.test.ts index 98e52a01..1c2444f3 100644 --- a/test/unit/commands/accounts/login.test.ts +++ b/test/unit/commands/accounts/login.test.ts @@ -383,31 +383,43 @@ describe("accounts:login command", () => { standardFlagTests("accounts:login", import.meta.url, ["--json"]); - describe("custom control host", () => { - it("should use custom control host when --control-host flag is provided", async () => { - const customHost = "custom.ably.net"; - mockOAuthDeviceFlow(customHost); - - // Mock the /me endpoint on custom host - nock(`https://${customHost}`) + describe("custom hosts", () => { + it("routes OAuth to --oauth-host and Control API to --control-host independently", async () => { + // OAuth and Control API are distinct services; the CLI must target + // them separately. Using a `control.` prefix on the control host + // forces the /v1/ path; everything else uses /api/v1/. + const oauthHost = "oauth.custom.ably.net"; + const controlHost = "control.custom.ably.net"; + + mockOAuthDeviceFlow(oauthHost); + + // Mock the /me endpoint on the control host + nock(`https://${controlHost}`) .get("/v1/me") .reply(200, { account: { id: mockAccountId, name: "Test Account" }, user: { email: "test@example.com" }, }); - // Mock the /me/accounts endpoint on custom host - nock(`https://${customHost}`) + // Mock the /me/accounts endpoint on the control host + nock(`https://${controlHost}`) .get("/v1/me/accounts") .reply(200, [{ id: mockAccountId, name: "Test Account" }]); - // Mock the apps list endpoint on custom host - nock(`https://${customHost}`) + // Mock the apps list endpoint on the control host + nock(`https://${controlHost}`) .get(`/v1/accounts/${mockAccountId}/apps`) .reply(200, []); const { stdout } = await runCommand( - ["accounts:login", "--control-host", customHost, "--json"], + [ + "accounts:login", + "--oauth-host", + oauthHost, + "--control-host", + controlHost, + "--json", + ], import.meta.url, ); diff --git a/test/unit/services/control-api.test.ts b/test/unit/services/control-api.test.ts index d4985543..e1037d03 100644 --- a/test/unit/services/control-api.test.ts +++ b/test/unit/services/control-api.test.ts @@ -29,12 +29,12 @@ describe("ControlApi", function () { it("should use provided control host", function () { const customApi = new ControlApi({ accessToken, - controlHost: "custom.control.host", + controlHost: "control.custom.host", logErrors: false, }); // Set up nock to intercept request to custom host - const scope = nock("https://custom.control.host") + const scope = nock("https://control.custom.host") .get("/v1/me") .reply(200, { account: { id: "test-account-id" } }); diff --git a/test/unit/services/oauth-client.test.ts b/test/unit/services/oauth-client.test.ts index 81df9970..cccbd77c 100644 --- a/test/unit/services/oauth-client.test.ts +++ b/test/unit/services/oauth-client.test.ts @@ -33,7 +33,7 @@ describe("OAuthClient", () => { }); it("host containing 'local' uses http scheme", async () => { - const client = new OAuthClient({ controlHost: "localhost:3000" }); + const client = new OAuthClient({ oauthHost: "localhost:3000" }); const scope = nock("http://localhost:3000") .post("/oauth/authorize_device") @@ -330,17 +330,119 @@ describe("OAuthClient", () => { expect(scope.isDone()).toBe(true); }); - it("does not throw on network error", async () => { + it("throws on network error so the caller can report it", async () => { const client = new OAuthClient(); nock("https://ably.com") .post("/oauth/revoke") .replyWithError("Connection refused"); - // Should not throw + await expect(client.revokeToken("token_to_revoke")).rejects.toThrow(); + }); + + it("throws on non-2xx response", async () => { + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/revoke") + .reply(500, "internal_error"); + + await expect(client.revokeToken("token_to_revoke")).rejects.toThrow( + /Token revocation failed \(500\)/, + ); + }); + + it("rejects when external AbortSignal is aborted", async () => { + const client = new OAuthClient(); + + // Never responds — ensures the abort is what ends the request. + nock("https://ably.com").post("/oauth/revoke").delay(10_000).reply(200); + + const controller = new AbortController(); + const promise = client.revokeToken("token_to_revoke", { + signal: controller.signal, + }); + // Abort after the fetch is in flight. + setTimeout(() => controller.abort(), 20); + + await expect(promise).rejects.toThrow(); + }); + }); + + describe("pollForToken error handling", () => { + it("backs off on HTTP 429 and eventually succeeds", async () => { + const client = new OAuthClient(); + + // First call: 429 with the Control-API envelope the website actually + // returns (no `code`, `statusCode` is a string). + nock("https://ably.com") + .post("/oauth/token") + .reply(429, { + error: { message: "Too many requests.", statusCode: "429" }, + }); + + // Second call: success + nock("https://ably.com").post("/oauth/token").reply(200, { + access_token: "at_after_429", + expires_in: 3600, + refresh_token: "rt_after_429", + token_type: "Bearer", + }); + + const tokens = await client.pollForToken("dc_429", 0.01, 30); + expect(tokens.accessToken).toBe("at_after_429"); + }, 15_000); + + it("surfaces the message from the Control-API-style envelope", async () => { + // Mirrors the real shape observed at https://ably.com/oauth/token: + // {error: {message, statusCode: ""}}. No `code` field. + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/token") + .reply(404, { + error: { + message: "This resource could not be found.", + statusCode: "404", + }, + }); + + await expect(client.pollForToken("dc_obj", 0.01, 10)).rejects.toThrow( + /Token polling failed: This resource could not be found\./, + ); + }); + + it("surfaces a message when the envelope includes a `code` field", async () => { + // Defensive: if the website is ever brought in line with the documented + // Control API shape (`{error: {code, message, statusCode}}`), we should + // still extract the message. + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/token") + .reply(400, { + error: { + code: "40000", + message: "Invalid device code", + statusCode: 400, + }, + }); + + await expect(client.pollForToken("dc_obj", 0.01, 10)).rejects.toThrow( + /Token polling failed: Invalid device code/, + ); + }); + + it("does not render [object Object] when error is an unknown object", async () => { + const client = new OAuthClient(); + + nock("https://ably.com") + .post("/oauth/token") + .reply(400, { error: { something: "weird" } }); + await expect( - client.revokeToken("token_to_revoke"), - ).resolves.toBeUndefined(); + client.pollForToken("dc_obj2", 0.01, 10), + ).rejects.not.toThrow(/\[object Object\]/); }); }); }); From 79d6cb0f344567be849ba0391368409e58211fa2 Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 20 Apr 2026 18:12:35 +0100 Subject: [PATCH 10/19] Simplify OAuth error handling now that server is RFC 6749 compliant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ably/website#7962 makes /oauth/token, /oauth/authorize_device, /oauth/revoke, and /oauth/introspect return RFC 6749 §5.2 flat error bodies ({"error": "", "error_description": ""}) and attaches Retry-After / RateLimit-* headers on 429s, with a new "rate_limited" error code for throttled requests. This CLI change is designed to land together with the website PR. With the server emitting a single well-defined shape, the defensive shape-sniffing in parsePollingError is no longer needed: - parsePollingError -> parseOAuthError: a trivial flat-shape reader that extracts `error` + `error_description`. No more top-level string vs nested-object vs bare-message branching. - pollForToken's special-case 429 handler is gone. The new "rate_limited" error code joins the existing switch alongside authorization_pending / slow_down / expired_token / access_denied, and honours the server-supplied Retry-After header (clamped to 60s) with a fallback to exponential backoff capped at 30s if the header is missing or unparseable. - postForTokens uses the same parseOAuthError instead of its own ad-hoc JSON.parse. OAuthRefreshExpiredError still fires when the server returns {error: "invalid_grant"} on refresh. - Token polling / refresh error messages now read "Token polling failed: " rather than embedding the raw response body. - Tests rewritten to match the new contract: rate_limited with Retry-After (real server behaviour), error_description surfacing, bare-code fallback, unparseable-body fallback, and invalid_grant -> OAuthRefreshExpiredError. Net: -32 LOC in oauth-client.ts. One parser, one error-handling topology, no dual-world defensive code. --- src/services/oauth-client.ts | 145 ++++++++---------------- test/unit/services/oauth-client.test.ts | 97 +++++++++------- 2 files changed, 105 insertions(+), 137 deletions(-) diff --git a/src/services/oauth-client.ts b/src/services/oauth-client.ts index 01f0780b..09cdf08b 100644 --- a/src/services/oauth-client.ts +++ b/src/services/oauth-client.ts @@ -167,44 +167,37 @@ export class OAuthClient { return this.parseTokenResponse(data); } - // Handle rate limiting before attempting to parse the error body. An - // OAuth-compliant server would return error=slow_down, but the current - // website reuses the Control API error envelope for 429s, so we bail - // out to backoff on status alone and don't depend on a specific shape. - if (response.status === 429) { - currentInterval = Math.min(currentInterval * 2, 30); - // Best-effort consume body so the socket can be released even if the - // server wrote one; failures here are irrelevant. - try { - await response.text(); - } catch { - // ignore - } - continue; - } - - const { code: errorCode, message: errorMessage } = - await parsePollingError(response); - - if (errorCode === "authorization_pending") { - continue; - } - - if (errorCode === "slow_down") { - currentInterval += 5; - continue; - } + const { code, description } = await parseOAuthError(response); - if (errorCode === "expired_token") { - throw new Error("Device code expired"); - } - - if (errorCode === "access_denied") { - throw new Error("Authorization denied"); + switch (code) { + case "authorization_pending": { + continue; + } + case "slow_down": { + currentInterval += 5; + continue; + } + case "rate_limited": { + // Honour server-supplied Retry-After; fall back to exponential + // backoff capped at 30s if the header is missing or unparseable. + const retryAfter = Number(response.headers.get("retry-after")); + currentInterval = + Number.isFinite(retryAfter) && retryAfter > 0 + ? Math.min(retryAfter, 60) + : Math.min(currentInterval * 2, 30); + continue; + } + case "expired_token": { + throw new Error("Device code expired"); + } + case "access_denied": { + throw new Error("Authorization denied"); + } + default: { + const reason = description ?? code ?? `HTTP ${response.status}`; + throw new Error(`Token polling failed: ${reason}`); + } } - - const reason = errorMessage ?? errorCode ?? `HTTP ${response.status}`; - throw new Error(`Token polling failed: ${reason}`); } throw new Error("Device code expired"); @@ -328,27 +321,17 @@ export class OAuthClient { } if (!response.ok) { - const errorBody = await response.text(); + const { code, description } = await parseOAuthError(response); // Detect invalid_grant on refresh: the refresh token is gone (revoked, // rotated by a concurrent refresh, or otherwise expired). Surface this - // as a typed error so callers can prompt the user to re-login instead - // of showing a raw HTTP error body. - if (params.grant_type === "refresh_token") { - let oauthError: string | undefined; - try { - oauthError = (JSON.parse(errorBody) as { error?: string }).error; - } catch { - // Non-JSON body — fall through to generic error. - } - if (oauthError === "invalid_grant") { - throw new OAuthRefreshExpiredError( - "OAuth refresh token is no longer valid. Please run 'ably login' again.", - ); - } + // as a typed error so callers can prompt the user to re-login. + if (params.grant_type === "refresh_token" && code === "invalid_grant") { + throw new OAuthRefreshExpiredError( + "OAuth refresh token is no longer valid. Please run 'ably login' again.", + ); } - throw new Error( - `${operationName} failed (${response.status}): ${errorBody}`, - ); + const reason = description ?? code ?? `HTTP ${response.status}`; + throw new Error(`${operationName} failed: ${reason}`); } const data = (await response.json()) as Record; @@ -393,53 +376,23 @@ export class OAuthClient { } /** - * Extract an error code + human message from a polling-endpoint error body. - * - * The OAuth-compliant shape per RFC 6749 §5.2 is `{error: "slow_down"}` with - * an optional `error_description`. The website currently does NOT use this - * shape for non-OAuth-specific failures (rate limits, 404s, etc.); it reuses - * the Control API envelope `{error: {message, statusCode}}`. Observed in - * practice: `statusCode` is a string (e.g. "429") and there is no `code` - * field — but we still read `code` defensively in case the website is later - * brought into line with the Control API's documented shape - * `{error: {code, message, statusCode}}`. - * - * Never throws — parse failures degrade to undefined. + * Parse an RFC 6749 §5.2 OAuth error body: `{error: "", + * error_description?: ""}`. Never throws — a non-JSON or unexpected + * body degrades to `{}` so callers fall through to their HTTP-status fallback. */ -async function parsePollingError( +async function parseOAuthError( response: FetchResponse, -): Promise<{ code?: string; message?: string }> { - let errorData: Record; +): Promise<{ code?: string; description?: string }> { try { - errorData = (await response.json()) as Record; - } catch { - return {}; - } - - const raw = errorData.error; - if (typeof raw === "string") { + const data = (await response.json()) as Record; return { - code: raw, - message: - typeof errorData.error_description === "string" - ? errorData.error_description + code: typeof data.error === "string" ? data.error : undefined, + description: + typeof data.error_description === "string" + ? data.error_description : undefined, }; + } catch { + return {}; } - - if (raw && typeof raw === "object") { - const nested = raw as Record; - const stringify = (v: unknown) => - typeof v === "string" ? v : typeof v === "number" ? String(v) : undefined; - // Prefer `code` if the website ever emits it, then fall back to - // `statusCode` (what it actually emits today). - const code = stringify(nested.code) ?? stringify(nested.statusCode); - const message = - typeof nested.message === "string" ? nested.message : undefined; - return { code, message }; - } - - const topLevelMessage = - typeof errorData.message === "string" ? errorData.message : undefined; - return { message: topLevelMessage }; } diff --git a/test/unit/services/oauth-client.test.ts b/test/unit/services/oauth-client.test.ts index cccbd77c..cea333be 100644 --- a/test/unit/services/oauth-client.test.ts +++ b/test/unit/services/oauth-client.test.ts @@ -255,14 +255,42 @@ describe("OAuthClient", () => { expect(tokens.expiresAt).toBeGreaterThan(Date.now()); }); - it("throws on non-200 response with status and body", async () => { + it("throws OAuthRefreshExpiredError on invalid_grant", async () => { const client = new OAuthClient(); - nock("https://ably.com").post("/oauth/token").reply(401, "invalid_grant"); + nock("https://ably.com").post("/oauth/token").reply(400, { + error: "invalid_grant", + error_description: "The refresh token is invalid.", + }); + + await expect( + client.refreshAccessToken("bad_refresh_token"), + ).rejects.toThrow(/OAuth refresh token is no longer valid/); + }); + + it("surfaces error_description on other OAuth errors", async () => { + const client = new OAuthClient(); + + nock("https://ably.com").post("/oauth/token").reply(400, { + error: "invalid_request", + error_description: "The refresh_token parameter is missing.", + }); + + await expect( + client.refreshAccessToken("bad_refresh_token"), + ).rejects.toThrow( + /Token refresh failed: The refresh_token parameter is missing\./, + ); + }); + + it("falls back to HTTP status when body is unparseable", async () => { + const client = new OAuthClient(); + + nock("https://ably.com").post("/oauth/token").reply(500, "upstream down"); await expect( client.refreshAccessToken("bad_refresh_token"), - ).rejects.toThrow("Token refresh failed (401): invalid_grant"); + ).rejects.toThrow(/Token refresh failed: HTTP 500/); }); it("sends correct form-encoded body", async () => { @@ -370,18 +398,20 @@ describe("OAuthClient", () => { }); describe("pollForToken error handling", () => { - it("backs off on HTTP 429 and eventually succeeds", async () => { + it("backs off on rate_limited (429) and eventually succeeds", async () => { const client = new OAuthClient(); - // First call: 429 with the Control-API envelope the website actually - // returns (no `code`, `statusCode` is a string). - nock("https://ably.com") - .post("/oauth/token") - .reply(429, { - error: { message: "Too many requests.", statusCode: "429" }, - }); + // First call: 429 with RFC 6749 §5.2 flat body + Retry-After header, + // matching what the website now emits (see website PR #7962). + nock("https://ably.com").post("/oauth/token").reply( + 429, + { + error: "rate_limited", + error_description: "Rate limit exceeded. Retry after 1 second.", + }, + { "Retry-After": "1" }, + ); - // Second call: success nock("https://ably.com").post("/oauth/token").reply(200, { access_token: "at_after_429", expires_in: 3600, @@ -393,56 +423,41 @@ describe("OAuthClient", () => { expect(tokens.accessToken).toBe("at_after_429"); }, 15_000); - it("surfaces the message from the Control-API-style envelope", async () => { - // Mirrors the real shape observed at https://ably.com/oauth/token: - // {error: {message, statusCode: ""}}. No `code` field. + it("surfaces error_description on unknown error codes", async () => { const client = new OAuthClient(); - nock("https://ably.com") - .post("/oauth/token") - .reply(404, { - error: { - message: "This resource could not be found.", - statusCode: "404", - }, - }); + nock("https://ably.com").post("/oauth/token").reply(400, { + error: "invalid_request", + error_description: "The device_code parameter is malformed.", + }); await expect(client.pollForToken("dc_obj", 0.01, 10)).rejects.toThrow( - /Token polling failed: This resource could not be found\./, + /Token polling failed: The device_code parameter is malformed\./, ); }); - it("surfaces a message when the envelope includes a `code` field", async () => { - // Defensive: if the website is ever brought in line with the documented - // Control API shape (`{error: {code, message, statusCode}}`), we should - // still extract the message. + it("falls back to the error code when no description is provided", async () => { const client = new OAuthClient(); nock("https://ably.com") .post("/oauth/token") - .reply(400, { - error: { - code: "40000", - message: "Invalid device code", - statusCode: 400, - }, - }); + .reply(400, { error: "invalid_request" }); await expect(client.pollForToken("dc_obj", 0.01, 10)).rejects.toThrow( - /Token polling failed: Invalid device code/, + /Token polling failed: invalid_request/, ); }); - it("does not render [object Object] when error is an unknown object", async () => { + it("falls back to HTTP status when the body is not parseable", async () => { const client = new OAuthClient(); nock("https://ably.com") .post("/oauth/token") - .reply(400, { error: { something: "weird" } }); + .reply(500, "upstream error"); - await expect( - client.pollForToken("dc_obj2", 0.01, 10), - ).rejects.not.toThrow(/\[object Object\]/); + await expect(client.pollForToken("dc_obj", 0.01, 10)).rejects.toThrow( + /Token polling failed: HTTP 500/, + ); }); }); }); From 5fdf32af648ea841609bd5860e4dc1bb981f209f Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 20 Apr 2026 19:30:25 +0100 Subject: [PATCH 11/19] Surface the selected account before app/key prompts in login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AndyTWF's review on PR #144 (login.ts:102) noted that when the user has exactly one account — or zero, falling back to /me — the login flow silently proceeds to the app selection prompt with no indication of which account is active. The multi-account interactive path is self-evident because the user just picked an account; the 0/1 paths are not. Log "Using account ()" right after account selection so the user knows which account any subsequent app/key prompts apply to. Guarded on !shouldOutputJson because the JSON result record already carries the account identity. --- src/commands/accounts/login.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index 2b6ce5f0..930055f9 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -118,6 +118,12 @@ export default class AccountsLogin extends ControlBaseCommand { ); } + if (!this.shouldOutputJson(flags)) { + this.log( + `\nUsing account ${formatResource(selectedAccountInfo.name)} (${selectedAccountInfo.id}).`, + ); + } + // Resolve alias AFTER account selection so we can default to account name let { alias } = flags; if (!alias && !this.shouldOutputJson(flags)) { From 51af4223684673089b97fc16c535b6e66c96831e Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 20 Apr 2026 19:34:41 +0100 Subject: [PATCH 12/19] Clean Ctrl+C handling during OAuth device-code polling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AndyTWF's review on PR #144 (login.ts:81) reported that hitting Ctrl+C while the spinner waits for authorization produces a confusing Node warning: Warning: Detected unsettled top-level await at …bin/run.js:37 await execute({ Root cause: pollForToken had no abort plumbing, so SIGINT killed the process while the await was still pending. bin/run.js's top-level await then surfaced the unsettled-await warning. Fix: - oauth-client.ts: pollForToken now accepts an optional AbortSignal. The signal is checked at the top of each loop iteration, threaded into the per-poll fetch's AbortController (mirroring the existing pattern in revokeToken), and also threaded into the jitter-sleep so the cancel is prompt (<50ms) even mid-sleep. The sleep helper gains a signal parameter that clears its setTimeout on abort. - accounts/login.ts:oauthLogin installs a scoped SIGINT handler that aborts the controller, fails the spinner with "Authentication cancelled.", and calls this.exit(130) — oclif's clean exit, which suppresses the unsettled-top-level-await warning. Handler is removed in `finally` so it doesn't leak across commands. - Added three oauth-client tests covering: abort mid-sleep (long interval), abort mid-fetch (delayed response), and pre-aborted signal rejects immediately. --- src/commands/accounts/login.ts | 16 +++++++++ src/services/oauth-client.ts | 37 +++++++++++++++++-- test/unit/services/oauth-client.test.ts | 48 +++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index 930055f9..85f01fa1 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -420,11 +420,25 @@ export default class AccountsLogin extends ControlBaseCommand { ? undefined : ora("Waiting for authorization...").start(); + // Wire up SIGINT so Ctrl+C during polling aborts the fetch and exits + // cleanly. Without this, the unsettled top-level await in bin/run.js + // triggers a confusing "Detected unsettled top-level await" warning + // from Node when the user hits Ctrl+C. + const abortController = new AbortController(); + const onSigint = () => { + abortController.abort(); + spinner?.fail("Authentication cancelled."); + // Match the conventional exit code for SIGINT (128 + signal number). + this.exit(130); + }; + process.once("SIGINT", onSigint); + try { const tokens = await oauthClient.pollForToken( deviceResponse.deviceCode, deviceResponse.interval, deviceResponse.expiresIn, + abortController.signal, ); spinner?.succeed("Authentication successful."); @@ -432,6 +446,8 @@ export default class AccountsLogin extends ControlBaseCommand { } catch (error) { spinner?.fail("Authentication failed."); throw error; + } finally { + process.removeListener("SIGINT", onSigint); } } diff --git a/src/services/oauth-client.ts b/src/services/oauth-client.ts index 09cdf08b..41863e4e 100644 --- a/src/services/oauth-client.ts +++ b/src/services/oauth-client.ts @@ -108,11 +108,13 @@ export class OAuthClient { /** * Poll for token completion (RFC 8628 step 2). * Sleeps between requests, respects slow_down, and throws on expiry/denial. + * Accepts an optional AbortSignal for prompt cancellation. */ async pollForToken( deviceCode: string, interval: number, expiresIn: number, + signal?: AbortSignal, ): Promise { const deadline = Date.now() + expiresIn * 1000; let currentInterval = interval; @@ -120,11 +122,14 @@ export class OAuthClient { const maxNetworkRetries = 3; while (Date.now() < deadline) { + if (signal?.aborted) { + throw new Error("Polling aborted"); + } // Apply ±20% jitter so concurrent clients don't fall into lockstep and // hit the authorization server in synchronized bursts, which trips the // shared rate limit long before any individual client is misbehaving. const jitterFactor = 0.9 + Math.random() * 0.2; - await this.sleep(currentInterval * 1000 * jitterFactor); + await this.sleep(currentInterval * 1000 * jitterFactor, signal); if (Date.now() >= deadline) { throw new Error("Device code expired"); @@ -136,6 +141,14 @@ export class OAuthClient { // the outer `while (Date.now() < deadline)` guard and spin the spinner // forever with no error. const timeout = setTimeout(() => controller.abort(), 15_000); + const onExternalAbort = () => controller.abort(); + if (signal) { + if (signal.aborted) { + controller.abort(); + } else { + signal.addEventListener("abort", onExternalAbort); + } + } try { const params = new URLSearchParams({ client_id: this.config.clientId, @@ -151,6 +164,9 @@ export class OAuthClient { }); networkRetries = 0; } catch { + if (signal?.aborted) { + throw new Error("Polling aborted"); + } networkRetries++; if (networkRetries >= maxNetworkRetries) { throw new Error( @@ -160,6 +176,7 @@ export class OAuthClient { continue; } finally { clearTimeout(timeout); + signal?.removeEventListener("abort", onExternalAbort); } if (response.ok) { @@ -370,8 +387,22 @@ export class OAuthClient { }; } - private sleep(ms: number): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)); + private sleep(ms: number, signal?: AbortSignal): Promise { + return new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(new Error("Sleep aborted")); + return; + } + const timer = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(); + }, ms); + const onAbort = () => { + clearTimeout(timer); + reject(new Error("Sleep aborted")); + }; + signal?.addEventListener("abort", onAbort, { once: true }); + }); } } diff --git a/test/unit/services/oauth-client.test.ts b/test/unit/services/oauth-client.test.ts index cea333be..a76a69d9 100644 --- a/test/unit/services/oauth-client.test.ts +++ b/test/unit/services/oauth-client.test.ts @@ -460,4 +460,52 @@ describe("OAuthClient", () => { ); }); }); + + describe("pollForToken abort signal", () => { + it("rejects promptly when the signal is aborted mid-sleep", async () => { + const client = new OAuthClient(); + const controller = new AbortController(); + + // A long interval (10s) guarantees we're in the sleep, not the fetch, + // when the signal fires. + const promise = client.pollForToken( + "dc_abort_sleep", + 10, + 60, + controller.signal, + ); + setTimeout(() => controller.abort(), 20); + + await expect(promise).rejects.toThrow(/aborted/i); + }, 2_000); + + it("rejects promptly when the signal is aborted during a fetch", async () => { + const client = new OAuthClient(); + const controller = new AbortController(); + + // The endpoint never responds, so without the abort the request would + // hit pollForToken's internal 15s fetch timeout instead. + nock("https://ably.com").post("/oauth/token").delay(10_000).reply(200); + + const promise = client.pollForToken( + "dc_abort_fetch", + 0.01, + 60, + controller.signal, + ); + setTimeout(() => controller.abort(), 100); + + await expect(promise).rejects.toThrow(/aborted/i); + }, 5_000); + + it("rejects immediately when the signal is already aborted", async () => { + const client = new OAuthClient(); + const controller = new AbortController(); + controller.abort(); + + await expect( + client.pollForToken("dc_pre_aborted", 1, 60, controller.signal), + ).rejects.toThrow(/aborted/i); + }); + }); }); From 9038e21c3ed667f8f01f4cd330ee65646654d561 Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 20 Apr 2026 19:33:13 +0100 Subject: [PATCH 13/19] Honour stored account hosts so commands don't need --control-host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AndyTWF's review on PR #144 flagged that `ably accounts switch` fails with "Access token may have expired or is invalid. The account was switched, but token verification failed." when no host flags are re-passed, and asked whether users should need to pass flags at all. Root cause was broader than the switch command: createControlApi (control-base-command.ts:71) used `flags["control-host"]` directly with no `account.controlHost` fallback — even though the oauthHost block six lines above already had the right pattern (`account.oauthHost ?? flags["oauth-host"]`). So after logging in against a review app (which stores controlHost on the account), every subsequent Control API command silently targeted the default control.ably.net unless the flag was re-passed every time. switch.ts compounded the problem by constructing ControlApi directly for its verification step, so even accounts whose controlHost was correctly stored never got the chance to have it read. Fixes: - control-base-command.ts:createControlApi now prefers `flags["control-host"] ?? account?.controlHost` (flag wins for one-off overrides; stored host wins for the default case). account lookup is hoisted out of the `if (!accessToken)` branch so it's available to the final ControlApi construction. - switch.ts:switchToLocalAccount now routes through this.createControlApi(flags) instead of `new ControlApi({...})`, which picks up BOTH host fallbacks AND the token refresh middleware — so an expired access token is transparently refreshed on switch instead of producing the misleading "token may have expired" warning. - switch.ts:interactiveSwitch likewise migrated to createControlApi; the existing manual fallback at that call site already handled controlHost but lacked oauthHost and the refresh middleware. - test/helpers/mock-config-manager.ts: storeAccount now accepts optional controlHost/oauthHost so tests can exercise the stored host path. - test/unit/commands/accounts/switch.test.ts: new test covering the bug — an OAuth account stored with controlHost= "review-abc.herokuapp.com" is verified against that host (routed to /api/v1/ via the non-"control." prefix heuristic), no "Access token may have expired" warning appears. --- src/commands/accounts/switch.ts | 32 ++++-------------- src/control-base-command.ts | 12 +++++-- test/helpers/mock-config-manager.ts | 4 +++ test/unit/commands/accounts/switch.test.ts | 38 ++++++++++++++++++++++ 4 files changed, 58 insertions(+), 28 deletions(-) diff --git a/src/commands/accounts/switch.ts b/src/commands/accounts/switch.ts index 041dd66c..90aac9d1 100644 --- a/src/commands/accounts/switch.ts +++ b/src/commands/accounts/switch.ts @@ -4,7 +4,7 @@ import inquirer from "inquirer"; import { ControlBaseCommand } from "../../control-base-command.js"; import { endpointFlag } from "../../flags.js"; -import { ControlApi, type AccountSummary } from "../../services/control-api.js"; +import { type AccountSummary } from "../../services/control-api.js"; import { formatResource } from "../../utils/output.js"; import { pickUniqueAlias, slugifyAccountName } from "../../utils/slugify.js"; @@ -93,19 +93,11 @@ export default class AccountsSwitch extends ControlBaseCommand { ): Promise { const currentAlias = this.configManager.getCurrentAccountAlias(); - // Try to fetch remote accounts using the current token + // Try to fetch remote accounts using the current token. let remoteAccounts: AccountSummary[] = []; - const accessToken = this.configManager.getAccessToken(); - if (accessToken) { + if (this.configManager.getAccessToken()) { try { - const currentAccount = this.configManager.getCurrentAccount(); - const controlHost = - (flags["control-host"] as string | undefined) || - currentAccount?.controlHost; - const controlApi = new ControlApi({ - accessToken, - controlHost, - }); + const controlApi = this.createControlApi(flags); remoteAccounts = await controlApi.getAccounts(); } catch { // Couldn't fetch remote accounts — fall back to local only @@ -291,21 +283,9 @@ export default class AccountsSwitch extends ControlBaseCommand { this.configManager.storeEndpoint(flags.endpoint as string); } - // Verify the account is valid by making an API call + // Verify the account is valid by making an API call. try { - const accessToken = this.configManager.getAccessToken(); - if (!accessToken) { - this.fail( - "No access token found for this account. Please log in again.", - flags, - "accountSwitch", - ); - } - - const controlApi = new ControlApi({ - accessToken, - controlHost: flags["control-host"] as string | undefined, - }); + const controlApi = this.createControlApi(flags); const { account, user } = await controlApi.getMe(); diff --git a/src/control-base-command.ts b/src/control-base-command.ts index c496a442..b5049743 100644 --- a/src/control-base-command.ts +++ b/src/control-base-command.ts @@ -19,9 +19,11 @@ export abstract class ControlBaseCommand extends AblyBaseCommand { protected createControlApi(flags: BaseFlags): ControlApi { let accessToken = process.env.ABLY_ACCESS_TOKEN; let tokenRefreshMiddleware: TokenRefreshMiddleware | undefined; + const account = accessToken + ? undefined + : this.configManager.getCurrentAccount(); if (!accessToken) { - const account = this.configManager.getCurrentAccount(); if (!account) { this.fail( `No access token provided. Please set the ABLY_ACCESS_TOKEN environment variable or configure an account with "ably accounts login".`, @@ -66,9 +68,15 @@ export abstract class ControlBaseCommand extends AblyBaseCommand { ); } + // Prefer the stored account host (what the user picked at login time) so + // later commands don't silently target the default control plane when the + // user originally logged in against a review / staging deployment. The + // flag still wins if explicitly passed, to allow one-off overrides. + const controlHost = flags["control-host"] ?? account?.controlHost; + return new ControlApi({ accessToken, - controlHost: flags["control-host"], + controlHost, tokenRefreshMiddleware, }); } diff --git a/test/helpers/mock-config-manager.ts b/test/helpers/mock-config-manager.ts index 7d566c75..a9723a87 100644 --- a/test/helpers/mock-config-manager.ts +++ b/test/helpers/mock-config-manager.ts @@ -410,6 +410,8 @@ export class MockConfigManager implements ConfigManager { accountInfo: { accountId: string; accountName: string; + controlHost?: string; + oauthHost?: string; tokenId?: string; userEmail: string; } = { @@ -423,6 +425,8 @@ export class MockConfigManager implements ConfigManager { accessToken, accountId: accountInfo.accountId, accountName: accountInfo.accountName, + controlHost: accountInfo.controlHost, + oauthHost: accountInfo.oauthHost, userEmail: accountInfo.userEmail, tokenId: accountInfo.tokenId, apps: existing?.apps || {}, diff --git a/test/unit/commands/accounts/switch.test.ts b/test/unit/commands/accounts/switch.test.ts index 3c7fb221..dd3d0960 100644 --- a/test/unit/commands/accounts/switch.test.ts +++ b/test/unit/commands/accounts/switch.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; import { runCommand } from "@oclif/test"; +import nock from "nock"; import { nockControl, controlApiCleanup, @@ -173,6 +174,43 @@ describe("accounts:switch command", () => { expect(mock.getAuthMethod("oauth-acct")).toBe("oauth"); }); + it("hits the stored controlHost when the account was logged in against a custom host", async () => { + const mock = getMockConfigManager(); + const customHost = "review-abc.herokuapp.com"; + + mock.storeOAuthTokens( + "custom-host-acct", + { + accessToken: "oauth_token_custom", + refreshToken: "refresh_token_custom", + expiresAt: Date.now() + 3_600_000, + userEmail: "custom@example.com", + }, + { + accountId: "custom-account-id", + accountName: "Custom Host Account", + controlHost: customHost, + oauthHost: customHost, + }, + ); + + const scope = nock(`https://${customHost}`) + .get("/api/v1/me") + .reply(200, { + account: { id: "custom-account-id", name: "Custom Host Account" }, + user: { email: "custom@example.com" }, + }); + + const { stderr } = await runCommand( + ["accounts:switch", "custom-host-acct"], + import.meta.url, + ); + + expect(stderr).toContain("Switched to account"); + expect(stderr).not.toContain("Access token may have expired"); + expect(scope.isDone()).toBe(true); + }); + it("should return JSON with account info when switching OAuth account with --json", async () => { const mock = getMockConfigManager(); From 2eb9a4bc2e458320abe67d64886e21923c527a9f Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 20 Apr 2026 19:37:36 +0100 Subject: [PATCH 14/19] Use the `slugify` npm package instead of a hand-rolled regex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AndyTWF's review on PR #144 (slugify.ts:1) asked why we're reinventing the wheel when a maintained slug library exists. Agreed — the regex-based implementation didn't handle unicode (e.g. "André" → "andr-") and we were writing the same loop everyone else has already written. Swapped to the `slugify` npm package (1.6.9, MIT, zero deps, 16 kB) with a thin wrapper that preserves the alias-specific rules we actually need: collapse consecutive dashes, trim edge dashes, fall back to "default" for empty, and prefix "account-" when the result doesn't start with a letter. Options used: `{lower: true, strict: true, trim: true}`. Side effect — better unicode handling: "André" now maps to "andre" via the library's charmap instead of producing "andr-". Apostrophes are also dropped cleanly in strict mode rather than leaving a stray dash. All 10 existing ASCII tests still pass as-is; added one unicode smoke test to lock in the new behaviour. pickUniqueAlias (collision-avoiding alias picker) in the same file is untouched — that's domain-specific logic, not a slug. --- package.json | 1 + pnpm-lock.yaml | 9 +++++++++ src/utils/slugify.ts | 9 +++++---- test/unit/utils/slugify.test.ts | 4 ++++ 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index ee8eacba..1183ce49 100644 --- a/package.json +++ b/package.json @@ -132,6 +132,7 @@ "ora": "^9.3.0", "react": "^19.2.5", "react-dom": "^19.2.5", + "slugify": "^1.6.9", "smol-toml": "^1.6.1", "ws": "^8.20.0", "zod": "^3.25.76" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 331f26b6..c522e165 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -96,6 +96,9 @@ importers: react-dom: specifier: ^19.2.5 version: 19.2.5(react@19.2.5) + slugify: + specifier: ^1.6.9 + version: 1.6.9 smol-toml: specifier: ^1.6.1 version: 1.6.1 @@ -4839,6 +4842,10 @@ packages: resolution: {integrity: sha512-2wcC/oGxHis/BoHkkPwldgiPSYcpZK3JU28WoMVv55yHJgcZ8rlXvuG9iZggz+sU1d4bRgIGASwyWqjxu3FM0g==} engines: {node: '>=18'} + slugify@1.6.9: + resolution: {integrity: sha512-vZ7rfeehZui7wQs438JXBckYLkIIdfHOXsaVEUMyS5fHo1483l1bMdo0EDSWYclY0yZKFOipDy4KHuKs6ssvdg==} + engines: {node: '>=8.0.0'} + smol-toml@1.6.1: resolution: {integrity: sha512-dWUG8F5sIIARXih1DTaQAX4SsiTXhInKf1buxdY9DIg4ZYPZK5nGM1VRIYmEbDbsHt7USo99xSLFu5Q1IqTmsg==} engines: {node: '>= 18'} @@ -10648,6 +10655,8 @@ snapshots: mrmime: 2.0.1 totalist: 3.0.1 + slugify@1.6.9: {} + smol-toml@1.6.1: {} snake-case@3.0.4: diff --git a/src/utils/slugify.ts b/src/utils/slugify.ts index b473eaab..b79401cb 100644 --- a/src/utils/slugify.ts +++ b/src/utils/slugify.ts @@ -1,12 +1,13 @@ +import slugify from "slugify"; + /** * Convert an account name to a valid alias slug. * Rules: lowercase, alphanumeric + dashes, must start with a letter. */ export function slugifyAccountName(name: string): string { - const slug = name - .toLowerCase() - .replaceAll(/[^a-z\d]+/g, "-") - .replaceAll(/^-+|-+$/g, ""); + const slug = slugify(name, { lower: true, strict: true, trim: true }) + .replaceAll(/-+/g, "-") + .replaceAll(/^-|-$/g, ""); if (!slug) { return "default"; } diff --git a/test/unit/utils/slugify.test.ts b/test/unit/utils/slugify.test.ts index c8b2bc02..13908b4f 100644 --- a/test/unit/utils/slugify.test.ts +++ b/test/unit/utils/slugify.test.ts @@ -41,4 +41,8 @@ describe("slugifyAccountName", () => { it("handles purely numeric name", () => { expect(slugifyAccountName("12345")).toBe("account-12345"); }); + + it("maps unicode characters via the slugify charmap", () => { + expect(slugifyAccountName("André's Co")).toBe("andres-co"); + }); }); From dba7e9bc63cd02124ed0f9d338e3ca331b8b4f11 Mon Sep 17 00:00:00 2001 From: umair Date: Tue, 21 Apr 2026 21:10:38 +0100 Subject: [PATCH 15/19] Login now extends ControlBaseCommand --- src/commands/login.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/commands/login.ts b/src/commands/login.ts index 559308da..397c7209 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -1,8 +1,7 @@ -import { Command } from "@oclif/core"; - +import { ControlBaseCommand } from "../control-base-command.js"; import AccountsLogin from "./accounts/login.js"; -export default class Login extends Command { +export default class Login extends ControlBaseCommand { static override description = 'Log in to your Ably account (alias for "ably accounts login")'; @@ -15,7 +14,10 @@ export default class Login extends Command { static override flags = AccountsLogin.flags; public async run(): Promise { - // Run the accounts login command with the same args and flags + // Login's own init/finally (from ControlBaseCommand) handle the lifecycle: + // web-CLI restriction check, ABLY_CURRENT_COMMAND, JSON completed signal, + // cached-client cleanup. AccountsLogin is run manually so its finally + // doesn't emit a duplicate completed signal. const accountsLogin = new AccountsLogin(this.argv, this.config); await accountsLogin.run(); } From 5692ab555aaf2003fea1dba963c1221237d4d91d Mon Sep 17 00:00:00 2001 From: umair Date: Tue, 21 Apr 2026 21:13:46 +0100 Subject: [PATCH 16/19] use formatResource helper for login auth code --- src/commands/accounts/login.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts index 85f01fa1..4cc7dc25 100644 --- a/src/commands/accounts/login.ts +++ b/src/commands/accounts/login.ts @@ -401,7 +401,7 @@ export default class AccountsLogin extends ControlBaseCommand { } else { this.log(""); this.log( - ` Your authorization code: ${chalk.bold.cyan(deviceResponse.userCode)}`, + ` Your authorization code: ${formatResource(deviceResponse.userCode)}`, ); this.log(""); this.log( From 3d13ab44005657d029dbfaea6d3df0ab7865ea03 Mon Sep 17 00:00:00 2001 From: umair Date: Tue, 21 Apr 2026 21:46:08 +0100 Subject: [PATCH 17/19] Stop reading user identity from the OAuth token response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server is moving to an RFC 6749-plain token response, so user_email and user_id will no longer be there. User identity is already fetched via the Control API /me call on login, so the token-response parsing was always redundant — this just removes it. --- src/services/config-manager.ts | 7 ++++--- src/services/oauth-client.ts | 9 --------- test/helpers/mock-config-manager.ts | 1 - 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/src/services/config-manager.ts b/src/services/config-manager.ts index 4c7b45f4..11d30100 100644 --- a/src/services/config-manager.ts +++ b/src/services/config-manager.ts @@ -88,7 +88,6 @@ export interface ConfigManager { refreshToken: string; expiresAt: number; scope?: string; - userId?: string; userEmail?: string; }, accountInfo?: { @@ -565,7 +564,10 @@ export class TomlConfigManager implements ConfigManager { this.saveConfig(); } - // Store OAuth tokens, shared across aliases with the same userEmail + oauthHost + // Store OAuth tokens, shared across aliases with the same userEmail + oauthHost. + // userEmail is supplied by callers from the /me response (fresh login) or from + // the previously-stored account metadata (token refresh, account switch) — it + // is never populated from the OAuth token response, which is RFC 6749 plain. public storeOAuthTokens( alias: string, tokens: { @@ -573,7 +575,6 @@ export class TomlConfigManager implements ConfigManager { refreshToken: string; expiresAt: number; scope?: string; - userId?: string; userEmail?: string; }, accountInfo?: { diff --git a/src/services/oauth-client.ts b/src/services/oauth-client.ts index 41863e4e..19ae69fe 100644 --- a/src/services/oauth-client.ts +++ b/src/services/oauth-client.ts @@ -30,8 +30,6 @@ export interface OAuthTokens { refreshToken: string; scope?: string; tokenType: string; - userEmail?: string; - userId?: string; } export interface OAuthConfig { @@ -377,13 +375,6 @@ export class OAuthClient { refreshToken, scope: data.scope as string | undefined, tokenType: (data.token_type as string) || "Bearer", - userEmail: data.user_email as string | undefined, - userId: - typeof data.user_id === "string" - ? data.user_id - : typeof data.user_id === "number" - ? String(data.user_id) - : undefined, }; } diff --git a/test/helpers/mock-config-manager.ts b/test/helpers/mock-config-manager.ts index a9723a87..d89129d7 100644 --- a/test/helpers/mock-config-manager.ts +++ b/test/helpers/mock-config-manager.ts @@ -526,7 +526,6 @@ export class MockConfigManager implements ConfigManager { refreshToken: string; expiresAt: number; scope?: string; - userId?: string; userEmail?: string; }, accountInfo?: { From b2131b36499707028c96215f60da9a29fdf5614d Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 27 Apr 2026 13:38:03 +0100 Subject: [PATCH 18/19] Honour stored controlHost in the auth-info banner lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same class of bug 9e42acbf fixed for createControlApi, in a path it didn't touch. displayAuthInfo (base-command.ts) constructs ControlApi directly to fetch a missing app name for the "Using: …" banner, but its host resolution stopped at flag → env. So after logging in against a review/staging deployment, every data-plane command's banner silently targeted control.ably.net, the lookup 404'd, and the banner downgraded to "App=Unknown App ()". Mirror createControlApi's precedence (flag → env → account.controlHost) by reading account?.controlHost as the final fallback. Use ?? rather than || so an empty-string flag value would still fall through cleanly in the same way. Test exercises the displayAuthInfo path with a stored review controlHost: nocks the review host, traps any traffic to control.ably.net, and asserts the banner reads the real app name and the resolved name is persisted via storeAppInfo. Note: this only fixes the cosmetic "Unknown App" banner. The 40400 "No application found" from realtime commands against a review deployment is a separate problem — the realtime cluster lives behind a different host and isn't redirected by --control-host. --- src/base-command.ts | 11 +++- test/unit/base/auth-info-display.test.ts | 64 ++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/base-command.ts b/src/base-command.ts index e87c4fb5..ea97bfad 100644 --- a/src/base-command.ts +++ b/src/base-command.ts @@ -621,10 +621,19 @@ export abstract class AblyBaseCommand extends InteractiveBaseCommand { this.configManager.getAccessToken(); if (accessToken) { + // Mirror createControlApi's host precedence (flag → env → stored + // account host) so the banner's app-name lookup honours the host + // the user picked at login. Without the account fallback this + // call silently targets control.ably.net even when the user + // logged in against a review/staging deployment, the lookup + // 404s, and the banner downgrades to "Unknown App". + const account = this.configManager.getCurrentAccount(); const controlApi = new ControlApi({ accessToken, controlHost: - flags["control-host"] || process.env.ABLY_CONTROL_HOST, + flags["control-host"] ?? + process.env.ABLY_CONTROL_HOST ?? + account?.controlHost, }); const app = await controlApi.getApp(appId); appName = app.name; diff --git a/test/unit/base/auth-info-display.test.ts b/test/unit/base/auth-info-display.test.ts index f9489714..fb029833 100644 --- a/test/unit/base/auth-info-display.test.ts +++ b/test/unit/base/auth-info-display.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { Config } from "@oclif/core"; +import nock from "nock"; import { AblyBaseCommand } from "../../../src/base-command.js"; import { BaseFlags } from "../../../src/types/cli.js"; @@ -63,6 +64,11 @@ describe("Auth Info Display", function () { getApiKey: ReturnType; getEndpoint: ReturnType; getKeyName: ReturnType; + getAccessToken: ReturnType; + getAppConfig: ReturnType; + storeAppInfo: ReturnType; + storeAppKey: ReturnType; + getAuthMethod: ReturnType; }; let logStub: ReturnType; let debugStub: ReturnType; @@ -76,6 +82,11 @@ describe("Auth Info Display", function () { getApiKey: vi.fn(), getEndpoint: vi.fn(), getKeyName: vi.fn(), + getAccessToken: vi.fn(), + getAppConfig: vi.fn(), + storeAppInfo: vi.fn(), + storeAppKey: vi.fn(), + getAuthMethod: vi.fn(), }; // Initialize command with stubs @@ -243,6 +254,59 @@ describe("Auth Info Display", function () { expect(outputWithUsingPrefix).toContain("App="); expect(outputWithUsingPrefix).toContain("Key="); }); + + it("uses the stored account controlHost when fetching a missing app name", async function () { + const customHost = "review-abc.herokuapp.com"; + const appId = "review-app-id"; + const accountId = "review-account-id"; + + shouldHideAccountInfoStub.mockReturnValue(false); + configManagerStub.getCurrentAccount.mockReturnValue({ + accountId, + accountName: "Review Account", + controlHost: customHost, + }); + configManagerStub.getCurrentAppId.mockReturnValue(appId); + configManagerStub.getAppName.mockReturnValue(); + configManagerStub.getApiKey.mockReturnValue(); + configManagerStub.getAccessToken.mockReturnValue("review-access-token"); + configManagerStub.getAppConfig.mockReturnValue(); + + const reviewScope = nock(`https://${customHost}`) + .get("/api/v1/me") + .reply(200, { + account: { id: accountId, name: "Review Account" }, + user: { email: "review@example.com" }, + }) + .get(`/api/v1/accounts/${accountId}/apps`) + .reply(200, [{ id: appId, name: "Review App", accountId }]); + + // Fail loud if anything reaches the production control plane. + const productionTrap = nock("https://control.ably.net") + .get(/.*/) + .reply(404); + + try { + await command.testDisplayAuthInfo({}); + + const banner = logStub.mock.calls + .map((call) => call[0]) + .find( + (output) => typeof output === "string" && output.includes("Using:"), + ) as string | undefined; + + expect(reviewScope.isDone()).toBe(true); + expect(productionTrap.isDone()).toBe(false); + expect(banner).toBeDefined(); + expect(banner).toContain("Review App"); + expect(banner).not.toContain("Unknown App"); + expect(configManagerStub.storeAppInfo).toHaveBeenCalledWith(appId, { + appName: "Review App", + }); + } finally { + nock.cleanAll(); + } + }); }); describe("showAuthInfoIfNeeded", function () { From a881b1de938352501eccc53474c8d27233d1bf53 Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 27 Apr 2026 14:12:16 +0100 Subject: [PATCH 19/19] Refine the controlHost regression test's mock reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace mockReturnValue() (which the lint auto-fix stripped to a no-arg form that TS rejects) with mockReset() — same effect, satisfies both the unicorn/no-useless-undefined rule and TS's mockReturnValue arity. --- test/unit/base/auth-info-display.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/unit/base/auth-info-display.test.ts b/test/unit/base/auth-info-display.test.ts index fb029833..3334c980 100644 --- a/test/unit/base/auth-info-display.test.ts +++ b/test/unit/base/auth-info-display.test.ts @@ -267,10 +267,12 @@ describe("Auth Info Display", function () { controlHost: customHost, }); configManagerStub.getCurrentAppId.mockReturnValue(appId); - configManagerStub.getAppName.mockReturnValue(); - configManagerStub.getApiKey.mockReturnValue(); + // Reset the parent beforeEach's "Test App" defaults so we exercise the + // missing-app-name branch (where ControlApi is consulted). + configManagerStub.getAppName.mockReset(); + configManagerStub.getApiKey.mockReset(); + configManagerStub.getAppConfig.mockReset(); configManagerStub.getAccessToken.mockReturnValue("review-access-token"); - configManagerStub.getAppConfig.mockReturnValue(); const reviewScope = nock(`https://${customHost}`) .get("/api/v1/me")