Skip to content

[DX-547] Add OAuth2 PKCE login flow as default authentication method #144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
umair-ably wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[DX-547] Add OAuth2 PKCE login flow as default authentication method #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a7fc9df
Add OAuth2 PKCE login flow as default authentication method
umair-ably Mar 3, 2026
3fda271
Switch OAuth login to Device Authorization Grant (RFC 8628)
umair-ably Mar 4, 2026
05b8ae1
Remove legacy token-paste login flow
umair-ably Mar 4, 2026
9e8402d
Add multi-account selection and switching for OAuth login
umair-ably Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
325 changes: 160 additions & 165 deletions src/commands/accounts/login.ts
View file
Open in desktop

Large diffs are not rendered by default.

16 changes: 16 additions & 0 deletions src/commands/accounts/logout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Args, Flags } from "@oclif/core";

import { ControlBaseCommand } from "../../control-base-command.js";
import { OAuthClient } from "../../services/oauth-client.js";
import { promptForConfirmation } from "../../utils/prompt-confirmation.js";

export default class AccountsLogout extends ControlBaseCommand {
Expand Down Expand Up @@ -88,6 +89,21 @@ 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 oauthClient = new OAuthClient({
controlHost: flags["control-host"],
});
Comment on lines +96 to +98
Copy link

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use configured host fallback when creating OAuthClient.

At Line [97], revocation host is sourced only from flags["control-host"]. If users rely on configured host values, revocation can target the wrong endpoint and silently not revoke tokens.

As per coding guidelines, "Use configuration for endpoint URLs (controlHost) instead of hardcoding endpoint URLs, following the pattern: flags.controlHost || config.controlHost || 'control.ably.net'."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/commands/accounts/logout.ts` around lines 96 - 98, The OAuthClient is
being constructed using only flags["control-host"], which ignores configured
fallbacks; update the OAuthClient instantiation in logout.ts to derive
controlHost using the configured fallback chain (flags["control-host"] ||
config.controlHost || 'control.ably.net') and pass that value to new
OAuthClient(...) so revocation targets the configured endpoint; reference the
OAuthClient construction and the flags["control-host"] and config.controlHost
symbols when making the change.

// Best-effort revocation -- don't block on failure
await Promise.all([
oauthClient.revokeToken(oauthTokens.accessToken),
oauthClient.revokeToken(oauthTokens.refreshToken),
]).catch(() => {});
Comment on lines +96 to +103
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also uses flags["control-host"] to choose the OAuth revocation endpoint. Since --control-host is meant for the Control API host (see src/base-command.ts flag description), revocation may be sent to the wrong domain when users override control host.

Consider using --dashboard-host (or a dedicated OAuth host flag) to configure OAuth revocation, keeping host overrides aligned with their documented purpose.

Copilot uses AI. Check for mistakes.
Comment on lines +100 to +103
Copy link

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Best-effort revocation should not block logout indefinitely.

At Line [100], logout still awaits both remote revocation calls. A slow/unresponsive endpoint can stall logout before local account removal.

Suggested timeout guard 
-        // Best-effort revocation -- don't block on failure
-        await Promise.all([
-          oauthClient.revokeToken(oauthTokens.accessToken),
-          oauthClient.revokeToken(oauthTokens.refreshToken),
-        ]).catch(() => {});
+        // Best-effort revocation -- don't block on failure or long hangs
+        const revokeWithTimeout = (token: string, timeoutMs = 3000) =>
+          Promise.race([
+            oauthClient.revokeToken(token),
+            new Promise<void>((resolve) => setTimeout(resolve, timeoutMs)),
+          ]);
+
+        await Promise.all([
+          revokeWithTimeout(oauthTokens.accessToken),
+          revokeWithTimeout(oauthTokens.refreshToken),
+        ]).catch(() => {});
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/commands/accounts/logout.ts` around lines 100 - 103, The current
Promise.all awaiting oauthClient.revokeToken(oauthTokens.accessToken) and
oauthClient.revokeToken(oauthTokens.refreshToken) can hang logout if the remote
endpoint is slow; wrap each revoke call with a timeout guard (e.g., Promise.race
between revokeToken(...) and a short timeout-resolving promise) or run them
fire-and-forget (start revoke calls without awaiting) and ensure errors are
caught, then proceed immediately to local account removal. Update the logout
flow around the Promise.all call so revokeToken calls are bounded by the timeout
wrapper or not awaited, still catching/logging any errors from
oauthClient.revokeToken to avoid unhandled rejections.

}
}

// Remove the account
const success = this.configManager.removeAccount(targetAlias);

Expand Down
197 changes: 163 additions & 34 deletions src/commands/accounts/switch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { Args } from "@oclif/core";
import chalk from "chalk";
import inquirer from "inquirer";

import { ControlBaseCommand } from "../../control-base-command.js";
import { ControlApi } from "../../services/control-api.js";
import { ControlApi, type AccountSummary } from "../../services/control-api.js";
import { slugifyAccountName } from "../../utils/slugify.js";

export default class AccountsSwitch extends ControlBaseCommand {
static override args = {
Expand All @@ -27,74 +30,210 @@ export default class AccountsSwitch extends ControlBaseCommand {
public async run(): Promise<void> {
const { args, flags } = await this.parse(AccountsSwitch);

// Get available accounts
const accounts = this.configManager.listAccounts();
const localAccounts = this.configManager.listAccounts();

if (accounts.length === 0) {
// No accounts configured, proxy to login command
if (localAccounts.length === 0) {
if (this.shouldOutputJson(flags)) {
const error =
'No accounts configured. Use "ably accounts login" to add an account.';
this.jsonError(
{
error,
error:
'No accounts configured. Use "ably accounts login" to add an account.',
success: false,
},
flags,
);
return;
}

// In interactive mode, proxy to login
this.log("No accounts configured. Redirecting to login...");
await this.config.runCommand("accounts:login");
return;
}

// If alias is provided, switch directly
if (args.alias) {
await this.switchToAccount(args.alias, accounts, flags);
await this.switchToLocalAccount(args.alias, localAccounts, flags);
return;
}

// Otherwise, show interactive selection if not in JSON mode
// JSON mode requires an explicit alias
if (this.shouldOutputJson(flags)) {
const error =
"No account alias provided. Please specify an account alias to switch to.";
this.jsonError(
{
availableAccounts: accounts.map(({ account, alias }) => ({
availableAccounts: localAccounts.map(({ account, alias }) => ({
alias,
id: account.accountId || "Unknown",
name: account.accountName || "Unknown",
})),
error,
error:
"No account alias provided. Please specify an account alias to switch to.",
success: false,
},
flags,
);
return;
}

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.log("Account switch cancelled.");
}
}

private async switchToAccount(
private async interactiveSwitch(
localAccounts: Array<{
account: {
accountId?: string;
accountName?: string;
userEmail?: string;
};
alias: string;
}>,
flags: Record<string, unknown>,
): Promise<boolean> {
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<Choice | inquirer.Separator> = [];

// 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",
},
]);

if (selected.type === "local") {
await this.switchToLocalAccount(selected.alias, localAccounts, flags);
return true;
}

// Remote account — create a local alias using the current token
await this.addAndSwitchToRemoteAccount(selected.account);
return true;
}

private async addAndSwitchToRemoteAccount(
remoteAccount: AccountSummary,
): Promise<void> {
const currentAlias = this.configManager.getCurrentAccountAlias();
if (!currentAlias) {
this.error("No current account to copy credentials from.");
return;
}

const oauthTokens = this.configManager.getOAuthTokens(currentAlias);
if (!oauthTokens) {
this.error(
"Current account does not use OAuth. Please log in with the target account directly.",
);
return;
}

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: ${chalk.cyan(remoteAccount.name)} (${remoteAccount.id})`,
);
this.log(`Saved as alias: ${chalk.cyan(newAlias)}`);
}

private async switchToLocalAccount(
alias: string,
accounts: Array<{
account: { accountId?: string; accountName?: string };
alias: string;
}>,
flags: Record<string, unknown>,
): Promise<void> {
// Check if account exists
const accountExists = accounts.some((account) => account.alias === alias);

if (!accountExists) {
Expand All @@ -120,23 +259,15 @@ export default class AccountsSwitch extends ControlBaseCommand {
return;
}

// Switch to the account
this.configManager.switchAccount(alias);

// Verify the account is valid by making an API call
try {
const accessToken = this.configManager.getAccessToken();
if (!accessToken) {
const error =
"No access token found for this account. Please log in again.";
if (this.shouldOutputJson(flags)) {
this.jsonError(
{
error,
success: false,
},
flags,
);
this.jsonError({ error, success: false }, flags);
return;
} else {
this.error(error);
Expand All @@ -160,9 +291,7 @@ export default class AccountsSwitch extends ControlBaseCommand {
alias,
id: account.id,
name: account.name,
user: {
email: user.email,
},
user: { email: user.email },
},
success: true,
},
Expand Down
15 changes: 15 additions & 0 deletions src/control-base-command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import chalk from "chalk";

import { AblyBaseCommand } from "./base-command.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, ErrorDetails } from "./types/cli.js";

export abstract class ControlBaseCommand extends AblyBaseCommand {
Expand All @@ -16,6 +18,7 @@ export abstract class ControlBaseCommand extends AblyBaseCommand {
*/
protected createControlApi(flags: BaseFlags): ControlApi {
let accessToken = flags["access-token"] || process.env.ABLY_ACCESS_TOKEN;
let tokenRefreshMiddleware: TokenRefreshMiddleware | undefined;

if (!accessToken) {
const account = this.configManager.getCurrentAccount();
Expand All @@ -26,6 +29,17 @@ export abstract class ControlBaseCommand extends AblyBaseCommand {
}

accessToken = account.accessToken;

// Set up token refresh middleware for OAuth accounts
if (this.configManager.getAuthMethod() === "oauth") {
const oauthClient = new OAuthClient({
controlHost: flags["control-host"],
});
tokenRefreshMiddleware = new TokenRefreshMiddleware(
this.configManager,
oauthClient,
);
Comment on lines +34 to +41
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OAuthClient is instantiated with flags["control-host"] here, but --control-host is documented as the Control API host override. If the user overrides control host to point to another environment, token refresh/revocation may be attempted against the wrong OAuth host.

Consider configuring OAuthClient from flags["dashboard-host"] (or a dedicated OAuth host flag) instead, so host overrides remain consistent with existing flag semantics.

Copilot uses AI. Check for mistakes.
Copy link

@cursor cursor bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token refresh ignores stored custom control host

Medium Severity

The OAuthClient for token refresh is created using only flags["control-host"], ignoring the controlHost stored in the account config. If a user logged in with --control-host localhost:3000, subsequent commands without that flag will send refresh requests to the default ably.com instead of the stored host. The same issue exists in the logout command for token revocation.

Additional Locations (1)

Fix in Cursor Fix in Web

}
}

if (!accessToken) {
Expand All @@ -37,6 +51,7 @@ export abstract class ControlBaseCommand extends AblyBaseCommand {
return new ControlApi({
accessToken,
controlHost: flags["control-host"],
tokenRefreshMiddleware,
});
}

Expand Down
Loading
Loading