[DX-547] Add OAuth Device Authorization Grant login flow#144
[DX-547] Add OAuth Device Authorization Grant login flow#144umair-ably wants to merge 8 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR introduces OAuth-based authentication support, adding an OAuthClient for PKCE-based authorization flows, TokenRefreshMiddleware for automatic token refresh, OAuth token storage in ConfigManager, and updates to the login and logout commands to support both OAuth and legacy token authentication methods with proper fallback mechanisms. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Login Command
participant OAuthClient
participant LocalServer as Callback Server
participant OAuthServer
participant ConfigManager
User->>CLI: Run accounts:login
CLI->>OAuthClient: login(openBrowser)
OAuthClient->>OAuthClient: Generate PKCE verifier & challenge
OAuthClient->>LocalServer: Start callback server on localhost
OAuthClient->>OAuthServer: Open browser with auth URL
OAuthServer->>User: Display login screen
User->>OAuthServer: Authenticate and authorize
OAuthServer->>LocalServer: Redirect to /callback with code & state
LocalServer->>OAuthClient: Callback received
OAuthClient->>OAuthClient: Validate state
OAuthClient->>OAuthServer: Exchange code for tokens
OAuthServer->>OAuthClient: Return access & refresh tokens
OAuthClient->>CLI: Return OAuthTokens
CLI->>ConfigManager: storeOAuthTokens(alias, tokens)
ConfigManager->>ConfigManager: Save to config file
CLI->>User: Success message
sequenceDiagram
participant API as API Request
participant ControlApi
participant TokenRefreshMiddleware
participant ConfigManager
participant OAuthClient
participant OAuthServer
API->>ControlApi: request()
ControlApi->>TokenRefreshMiddleware: getValidAccessToken()
TokenRefreshMiddleware->>ConfigManager: getAuthMethod()
alt OAuth Account
TokenRefreshMiddleware->>ConfigManager: isAccessTokenExpired()
alt Token Expired
TokenRefreshMiddleware->>ConfigManager: getOAuthTokens()
TokenRefreshMiddleware->>OAuthClient: refreshAccessToken(refreshToken)
OAuthClient->>OAuthServer: POST token endpoint
OAuthServer->>OAuthClient: New tokens
TokenRefreshMiddleware->>ConfigManager: storeOAuthTokens()
TokenRefreshMiddleware->>ControlApi: Return new accessToken
else Token Valid
TokenRefreshMiddleware->>ControlApi: Return existing token
end
else Non-OAuth Account
TokenRefreshMiddleware->>ControlApi: Return stored token
end
ControlApi->>ControlApi: Execute API request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces an OAuth2 Authorization Code + PKCE login flow as the default authentication path for the CLI, including token persistence, automatic access-token refresh for Control API calls, and best-effort token revocation on logout.
Changes:
- Add OAuth PKCE client + localhost callback server flow, including token refresh and revocation helpers.
- Persist OAuth tokens (refresh token + expiry) in config and add a middleware to auto-refresh expired access tokens.
- Update
accounts:loginto default to OAuth (with fallback to legacy token flow) andaccounts:logoutto revoke OAuth tokens; add unit tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/services/oauth-client.ts |
Implements OAuth PKCE login, token refresh, and token revocation. |
src/services/oauth-callback-page.ts |
Adds HTML pages for success/error callback responses (with escaping). |
src/services/token-refresh-middleware.ts |
Adds middleware to return a valid access token (refreshing when needed). |
src/services/control-api.ts |
Integrates token refresh middleware to update access token before each request. |
src/services/config-manager.ts |
Extends config schema + manager interface to store/read OAuth tokens and expiry. |
src/control-base-command.ts |
Creates and wires token refresh middleware for OAuth-authenticated accounts. |
src/commands/accounts/login.ts |
Makes OAuth the default login path; adds --legacy and refactors legacy token flow. |
src/commands/accounts/logout.ts |
Adds best-effort OAuth token revocation during logout. |
test/helpers/mock-config-manager.ts |
Updates mock config manager to support new OAuth config methods. |
test/unit/services/token-refresh-middleware.test.ts |
Adds unit tests for refresh middleware behavior and error cases. |
test/unit/services/oauth-client.test.ts |
Adds unit tests for PKCE generation, refresh, revoke, callback flow, and HTML escaping. |
test/unit/commands/accounts/login.test.ts |
Adds tests asserting legacy/direct-token login doesn’t mark auth as OAuth. |
test/unit/commands/accounts/logout.test.ts |
Adds tests for OAuth token revocation behavior on logout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
umair-ably
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/control-base-command.ts (1)
34-41: Pin token refresh to a fixed account alias.Lines [34-41] create
TokenRefreshMiddlewarewithoutaccountAlias, so refresh writes depend on mutable current-account state. Capture and pass the alias when building the API client instance.♻️ Suggested patch
- if (this.configManager.getAuthMethod() === "oauth") { + const accountAlias = this.configManager.getCurrentAccountAlias(); + if (this.configManager.getAuthMethod(accountAlias) === "oauth") { const oauthClient = new OAuthClient({ controlHost: flags["control-host"], }); tokenRefreshMiddleware = new TokenRefreshMiddleware( this.configManager, oauthClient, + accountAlias, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-base-command.ts` around lines 34 - 41, The TokenRefreshMiddleware is created without an explicit account alias so refresh writes depend on mutable current-account state; capture the current account alias from this.configManager (e.g., call the existing method that returns the active account alias) before constructing OAuthClient/TokenRefreshMiddleware and pass that alias into TokenRefreshMiddleware (or into OAuthClient if it accepts accountAlias) so the middleware is pinned to a fixed account; update the construction around OAuthClient and TokenRefreshMiddleware to include the captured accountAlias parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/accounts/login.ts`:
- Around line 100-108: The catch block in src/commands/accounts/login.ts
currently always falls back to interactive legacyTokenLogin even when JSON
output is requested; change the control flow so that if
this.shouldOutputJson(flags) is true the method rethrows (or returns a failed
promise) instead of calling legacyTokenLogin, and only call legacyTokenLogin
when shouldOutputJson(flags) is false. Update the catch to reference
shouldOutputJson(flags), throw the caught error (or propagate it) when JSON is
enabled, and keep the existing warn/log + accessToken = await
legacyTokenLogin(flags) path inside the non-JSON branch.
In `@src/commands/accounts/logout.ts`:
- Around line 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.
- Around line 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.
In `@src/services/oauth-client.ts`:
- Around line 95-106: Add a timeout/abort mechanism to the OAuth HTTP calls so
fetch doesn't hang: in revokeToken() and postForTokens() create an
AbortController, start a setTimeout that calls controller.abort() after a
sensible timeout (e.g. configurable constant), pass controller.signal to fetch,
and clear the timeout on success/failure to avoid leaks; ensure fetch errors due
to abort are handled or rethrown consistently by the existing error path in
revokeToken and postForTokens so behavior remains unchanged except for enforced
timeouts.
- Around line 81-89: The refreshAccessToken flow calls postForTokens without
providing the existing refresh token as a fallback, so when the token endpoint
omits refresh_token the code later casts data.refresh_token and ends up with
undefined; update refreshAccessToken to pass the current refreshToken into
postForTokens (so postForTokens -> parseTokenResponse can accept an optional
fallbackRefreshToken) and modify parseTokenResponse to use the provided
fallbackRefreshToken when data.refresh_token is missing, ensuring the returned
OAuthTokens.refreshToken remains the existing value if the server omits a new
one; target the functions refreshAccessToken, postForTokens and
parseTokenResponse to add the fallback parameter and use it as described.
---
Nitpick comments:
In `@src/control-base-command.ts`:
- Around line 34-41: The TokenRefreshMiddleware is created without an explicit
account alias so refresh writes depend on mutable current-account state; capture
the current account alias from this.configManager (e.g., call the existing
method that returns the active account alias) before constructing
OAuthClient/TokenRefreshMiddleware and pass that alias into
TokenRefreshMiddleware (or into OAuthClient if it accepts accountAlias) so the
middleware is pinned to a fixed account; update the construction around
OAuthClient and TokenRefreshMiddleware to include the captured accountAlias
parameter.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/commands/accounts/login.tssrc/commands/accounts/logout.tssrc/control-base-command.tssrc/services/config-manager.tssrc/services/control-api.tssrc/services/oauth-callback-page.tssrc/services/oauth-client.tssrc/services/token-refresh-middleware.tstest/helpers/mock-config-manager.tstest/unit/commands/accounts/login.test.tstest/unit/commands/accounts/logout.test.tstest/unit/services/oauth-client.test.tstest/unit/services/token-refresh-middleware.test.ts
|
@umair-ably this looks great, but OOI, did you consider OAuth 2.0 Device Authorization Grant? |
@lmars I didn't initially but looking into it now... I'm on the fence... Granted, it is the solution for a headless environment but can you foresee substantial usage with that scenario? I'd expect devs are using the CLI locally, in which a 1-click authorise button in a browser is a significantly better experience than needing to enter an auth code yourself imo (which is also what we wanted to solve by getting rid of the need to copy the Control API Access Token). Unless we already know there's headless usage of the CLI (or foresee there will be a significant amount), I suggest we stick with this. We can quite easily introduce an additional login mode e.g. WDYT? |
|
@umair-ably the flow would be the same as what's in the video, but rather than a user going to the regular ably.com login page, being redirected to a random localhost port, and the local server exchanging the code for an access token, the flow would be:
It's also simpler to implement (i.e. make an auth request, print the URL, poll until an access token is granted). Up to you, though 🙂 |
@lmars Ahh of course - nice, I like it! I don't see any reason not to do this now - I'll rework this and have something up for tomorrow 🙂 |
|
@lmars updated 🙂 added a new vid in the PR description |
| char: "a", | ||
| description: "Alias for this account (default account if not specified)", | ||
| }), | ||
| legacy: Flags.boolean({ |
There was a problem hiding this comment.
Why are we keeping the legacy approach?
There was a problem hiding this comment.
Claude always thinking we need backwards compatibility... removed!
|
Thanks, looks great. You'll see I added a comment to say that I'm not sure we should continue to support legacy authentication methods. I don't really understand why we would do that. It just seems like extra maintenance and code. However, saying that, what's going to happen the moment you start using this CLI and you're already set up an OAuth, the server control access token previously? What experience will a user now have? |
If you've already auth'd with a Control API Access Token, then everything continues to work as usual. The only thing that doesn't is switching accounts. The experience here is quite nice though, it just tells you that you're not OAuth'd and to login again |
|
Logging in when a user has multiple accounts multi.account.mov |
umair-ably
force-pushed
the
oauth-login-flow
branch
from
137a97f to
9e8402d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Crash when
getAccountsreturns empty arrayaccounts:loginnow falls back to the/meaccount when/me/accountsis empty soselectedAccountInfois always defined.
- ✅ Fixed: OAuth login doesn't store user email from API
- The OAuth login path now persists
user.emailfrom/mewhen storing OAuth tokens if the token response omitsuserEmail.
- The OAuth login path now persists
- ✅ Fixed: Token refresh ignores stored custom control host
- OAuth refresh and logout revocation now use
--control-hostor fall back to the account’s storedcontrolHostwhen no flag is provided.
- OAuth refresh and logout revocation now use
Or push these changes by commenting:
@cursor push 8f8f070526
Preview (8f8f070526)
diff --git a/src/commands/accounts/login.ts b/src/commands/accounts/login.ts
--- a/src/commands/accounts/login.ts
+++ b/src/commands/accounts/login.ts
@@ -98,21 +98,23 @@
controlHost: flags["control-host"],
});
- const [{ user }, accounts] = await Promise.all([
+ const [{ account, user }, accounts] = await Promise.all([
controlApi.getMe(),
controlApi.getAccounts(),
]);
let selectedAccountInfo: { id: string; name: string };
- if (accounts.length === 1) {
+ if (accounts.length === 0) {
+ selectedAccountInfo = { id: account.id, name: account.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 or empty (backward compat: use first)
+ // Multiple accounts in JSON mode (backward compat: use first)
selectedAccountInfo = accounts[0];
}
@@ -126,10 +128,14 @@
// Store based on auth method
if (oauthTokens) {
- this.configManager.storeOAuthTokens(alias, oauthTokens, {
- accountId: selectedAccountInfo.id,
- accountName: selectedAccountInfo.name,
- });
+ this.configManager.storeOAuthTokens(
+ alias,
+ { ...oauthTokens, userEmail: oauthTokens.userEmail ?? user.email },
+ {
+ accountId: selectedAccountInfo.id,
+ accountName: selectedAccountInfo.name,
+ },
+ );
} else {
this.configManager.storeAccount(accessToken, alias, {
accountId: selectedAccountInfo.id,
diff --git a/src/commands/accounts/logout.ts b/src/commands/accounts/logout.ts
--- a/src/commands/accounts/logout.ts
+++ b/src/commands/accounts/logout.ts
@@ -58,9 +58,10 @@
}
const accounts = this.configManager.listAccounts();
- const accountExists = accounts.some(
+ const targetAccount = accounts.find(
(account) => account.alias === targetAlias,
- );
+ )?.account;
+ const accountExists = Boolean(targetAccount);
if (!accountExists) {
const error = `Account with alias "${targetAlias}" not found. Use "ably accounts list" to see available accounts.`;
@@ -94,7 +95,9 @@
const oauthTokens = this.configManager.getOAuthTokens(targetAlias);
if (oauthTokens) {
const oauthClient = new OAuthClient({
- controlHost: flags["control-host"],
+ controlHost:
+ (flags["control-host"] as string | undefined) ||
+ targetAccount?.controlHost,
});
// Best-effort revocation -- don't block on failure
await Promise.all([
diff --git a/src/control-base-command.ts b/src/control-base-command.ts
--- a/src/control-base-command.ts
+++ b/src/control-base-command.ts
@@ -19,21 +19,23 @@
protected createControlApi(flags: BaseFlags): ControlApi {
let accessToken = flags["access-token"] || process.env.ABLY_ACCESS_TOKEN;
let tokenRefreshMiddleware: TokenRefreshMiddleware | undefined;
+ let currentAccount = this.configManager.getCurrentAccount();
if (!accessToken) {
- const account = this.configManager.getCurrentAccount();
- if (!account) {
+ if (!currentAccount) {
this.error(
`No access token provided. Please specify --access-token or configure an account with "ably accounts login".`,
);
}
- accessToken = account.accessToken;
+ accessToken = currentAccount.accessToken;
// Set up token refresh middleware for OAuth accounts
if (this.configManager.getAuthMethod() === "oauth") {
const oauthClient = new OAuthClient({
- controlHost: flags["control-host"],
+ controlHost:
+ (flags["control-host"] as string | undefined) ||
+ currentAccount?.controlHost,
});
tokenRefreshMiddleware = new TokenRefreshMiddleware(
this.configManager,
diff --git a/test/unit/base/control-base-command.test.ts b/test/unit/base/control-base-command.test.ts
new file mode 100644
--- /dev/null
+++ b/test/unit/base/control-base-command.test.ts
@@ -1,0 +1,103 @@
+import { beforeEach, afterEach, describe, expect, it, vi } from "vitest";
+import fs from "node:fs";
+import nock from "nock";
+import { Config } from "@oclif/core";
+
+import { ControlBaseCommand } from "../../../src/control-base-command.js";
+import {
+ ConfigManager,
+ TomlConfigManager,
+} from "../../../src/services/config-manager.js";
+import { BaseFlags } from "../../../src/types/cli.js";
+
+class TestControlCommand extends ControlBaseCommand {
+ public testCreateControlApi(flags: BaseFlags) {
+ return this.createControlApi(flags);
+ }
+
+ public set testConfigManager(value: ConfigManager) {
+ this.configManager = value;
+ }
+
+ async run(): Promise<void> {
+ // No-op
+ }
+}
+
+describe("ControlBaseCommand", () => {
+ let command: TestControlCommand;
+ let originalEnv: NodeJS.ProcessEnv;
+
+ beforeEach(() => {
+ originalEnv = { ...process.env };
+ delete process.env.ABLY_ACCESS_TOKEN;
+
+ vi.spyOn(fs, "existsSync").mockReturnValue(true);
+ vi.spyOn(fs, "readFileSync").mockReturnValue("");
+ vi.spyOn(
+ TomlConfigManager.prototype as unknown as {
+ ensureConfigDirExists: () => void;
+ },
+ "ensureConfigDirExists",
+ ).mockImplementation(() => {});
+ vi.spyOn(
+ TomlConfigManager.prototype as unknown as {
+ saveConfig: () => void;
+ },
+ "saveConfig",
+ ).mockImplementation(() => {});
+
+ command = new TestControlCommand([], {} as Config);
+ });
+
+ afterEach(() => {
+ process.env = originalEnv;
+ nock.cleanAll();
+ vi.restoreAllMocks();
+ });
+
+ it("uses stored control host for OAuth token refresh when flag is not provided", async () => {
+ const customControlHost = "custom.ably.net";
+ const configManagerStub = {
+ getAccessToken: vi.fn().mockReturnValue("expired_access_token"),
+ getAuthMethod: vi.fn().mockReturnValue("oauth"),
+ getCurrentAccount: vi.fn().mockReturnValue({
+ accessToken: "expired_access_token",
+ controlHost: customControlHost,
+ }),
+ getCurrentAccountAlias: vi.fn().mockReturnValue("default"),
+ getOAuthTokens: vi.fn().mockReturnValue({
+ accessToken: "expired_access_token",
+ expiresAt: Date.now() - 1000,
+ refreshToken: "refresh_token",
+ }),
+ isAccessTokenExpired: vi.fn().mockReturnValue(true),
+ storeOAuthTokens: vi.fn(),
+ } as unknown as ConfigManager;
+ command.testConfigManager = configManagerStub;
+
+ const refreshScope = nock(`https://${customControlHost}`)
+ .post("/oauth/token")
+ .reply(200, {
+ access_token: "refreshed_access_token",
+ expires_in: 3600,
+ refresh_token: "refreshed_refresh_token",
+ token_type: "Bearer",
+ });
+
+ nock("https://control.ably.net")
+ .matchHeader("authorization", "Bearer refreshed_access_token")
+ .get("/v1/me")
+ .reply(200, {
+ account: { id: "acc-123", name: "Test Account" },
+ user: { email: "test@example.com" },
+ });
+
+ const controlApi = command.testCreateControlApi({});
+ const me = await controlApi.getMe();
+
+ expect(me.account.id).toBe("acc-123");
+ expect(refreshScope.isDone()).toBe(true);
+ expect(configManagerStub.storeOAuthTokens).toHaveBeenCalled();
+ });
+});
diff --git a/test/unit/commands/accounts/login.test.ts b/test/unit/commands/accounts/login.test.ts
--- a/test/unit/commands/accounts/login.test.ts
+++ b/test/unit/commands/accounts/login.test.ts
@@ -128,6 +128,48 @@
expect(config.accounts[customAlias].userEmail).toBe("test@example.com");
});
+ it("should fall back to /me account when /me/accounts is empty", async () => {
+ const fallbackAccountId = "fallback-account-id";
+ const fallbackAccountName = "Fallback Account";
+
+ // Mock the /me endpoint twice - once for initial login, once for listApps
+ nock("https://control.ably.net")
+ .get("/v1/me")
+ .twice()
+ .reply(200, {
+ account: { id: fallbackAccountId, name: fallbackAccountName },
+ user: { email: "fallback@example.com" },
+ });
+
+ // Mock an empty /me/accounts response
+ nock("https://control.ably.net").get("/v1/me/accounts").reply(200, []);
+
+ // Mock the apps list endpoint
+ nock("https://control.ably.net")
+ .get(`/v1/accounts/${fallbackAccountId}/apps`)
+ .reply(200, []);
+
+ const { stdout } = await runCommand(
+ ["accounts:login", mockAccessToken, "--json"],
+ import.meta.url,
+ );
+
+ const result = JSON.parse(stdout);
+ expect(result).toHaveProperty("success", true);
+ expect(result.account).toHaveProperty("id", fallbackAccountId);
+ expect(result.account).toHaveProperty("name", fallbackAccountName);
+
+ const mock = getMockConfigManager();
+ const config = mock.getConfig();
+ expect(config.accounts["fallback-account"]).toBeDefined();
+ expect(config.accounts["fallback-account"].accountId).toBe(
+ fallbackAccountId,
+ );
+ expect(config.accounts["fallback-account"].accountName).toBe(
+ fallbackAccountName,
+ );
+ });
+
it("should include app info when single app is auto-selected", async () => {
const mockAppId = "app-123";
const mockAppName = "My Only App";
@@ -401,5 +443,53 @@
config.accounts["test-account"].accessTokenExpiresAt,
).toBeUndefined();
});
+
+ it("should store user email from /me for OAuth login when token response omits user_email", async () => {
+ const oauthAccountId = "oauth-account-id";
+ const oauthEmail = "oauth-user@example.com";
+
+ nock("https://ably.com").post("/oauth/authorize_device").reply(200, {
+ device_code: "device-code-123",
+ expires_in: 300,
+ interval: 0.01,
+ user_code: "ABC123",
+ verification_uri: "https://ably.com/verify",
+ verification_uri_complete: "https://ably.com/verify?user_code=ABC123",
+ });
+
+ nock("https://ably.com").post("/oauth/token").reply(200, {
+ access_token: "oauth_access_token",
+ expires_in: 3600,
+ refresh_token: "oauth_refresh_token",
+ token_type: "Bearer",
+ });
+
+ // Mock the /me endpoint twice - once for initial login, once for listApps
+ nock("https://control.ably.net")
+ .get("/v1/me")
+ .twice()
+ .reply(200, {
+ account: { id: oauthAccountId, name: "OAuth Account" },
+ user: { email: oauthEmail },
+ });
+
+ nock("https://control.ably.net")
+ .get("/v1/me/accounts")
+ .reply(200, [{ id: oauthAccountId, name: "OAuth Account" }]);
+
+ nock("https://control.ably.net")
+ .get(`/v1/accounts/${oauthAccountId}/apps`)
+ .reply(200, []);
+
+ await runCommand(
+ ["accounts:login", "--no-browser", "--json"],
+ import.meta.url,
+ );
+
+ const mock = getMockConfigManager();
+ const config = mock.getConfig();
+ expect(config.accounts["oauth-account"].authMethod).toBe("oauth");
+ expect(config.accounts["oauth-account"].userEmail).toBe(oauthEmail);
+ });
});
});
diff --git a/test/unit/commands/accounts/logout.test.ts b/test/unit/commands/accounts/logout.test.ts
--- a/test/unit/commands/accounts/logout.test.ts
+++ b/test/unit/commands/accounts/logout.test.ts
@@ -270,6 +270,40 @@
expect(config.accounts["testaccount"]).toBeUndefined();
});
+ it("should use stored control host for revocation when flag is not provided", async () => {
+ const customControlHost = "custom.ably.net";
+ const mock = getMockConfigManager();
+ mock.setConfig({
+ current: { account: "testaccount" },
+ accounts: {
+ testaccount: {
+ accessToken: "oauth_access_token",
+ accessTokenExpiresAt: Date.now() + 3600000,
+ accountId: "acc-123",
+ accountName: "Test Account",
+ authMethod: "oauth",
+ controlHost: customControlHost,
+ refreshToken: "oauth_refresh_token",
+ userEmail: "test@example.com",
+ },
+ },
+ });
+
+ const revokeScope = nock(`https://${customControlHost}`)
+ .post("/oauth/revoke")
+ .twice()
+ .reply(200);
+
+ const { stdout } = await runCommand(
+ ["accounts:logout", "--force", "--json"],
+ import.meta.url,
+ );
+
+ const result = JSON.parse(stdout);
+ expect(result).toHaveProperty("success", true);
+ expect(revokeScope.isDone()).toBe(true);
+ });
+
it("should not call revocation endpoint for non-OAuth account logout", async () => {
const mock = getMockConfigManager();
mock.setConfig({
umair-ably
force-pushed
the
oauth-login-flow
branch
from
9e8402d to
571ff57
Compare
umair-ably
force-pushed
the
oauth-login-flow
branch
from
1c87b2e to
da91b3d
Compare
umair-ably
force-pushed
the
oauth-login-flow
branch
from
da91b3d to
9bf58a8
Compare
|
@claude review |
![claude-code-ably-assistant[bot]](https://avatars.githubusercontent.com/in/2109912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: OAuth2 Device Authorization Grant login flow
This is a solid implementation — clean architecture, good separation between OAuthClient / TokenRefreshMiddleware, and thorough unit tests. Three real issues to address before merge.
1. Bug: logout revokes against the wrong host for non-current accounts
src/commands/accounts/logout.ts:
const targetAccount = this.configManager.getCurrentAccount(); // <- current, not target
const oauthHost = flags["control-host"] || targetAccount?.controlHost;getCurrentAccount() returns the currently active account, but targetAlias can point to a different one. If the user runs ably accounts logout other-alias while logged in as a different alias, and other-alias was registered with a custom controlHost, the revocation POST goes to the wrong endpoint. Low security impact (token is cleared locally either way) but the server-side revocation silently fails. Fix: read controlHost from the target alias directly.
2. Shared refresh tokens across aliases will break on token rotation
src/commands/accounts/switch.ts addAndSwitchToRemoteAccount:
this.configManager.storeOAuthTokens(
newAlias,
{ ...oauthTokens, userEmail: currentAccount?.userEmail }, // copies refresh token
{ accountId: remoteAccount.id, accountName: remoteAccount.name },
);The current OAuth refresh token is cloned into the new alias. If the Ably server rotates refresh tokens on use (RFC 6749 §10.4 best practice), whichever alias refreshes first invalidates the other's stored token. TokenRefreshMiddleware then fails with invalid_grant requiring a re-login.
This needs confirmation from the server side: does the OAuth server rotate refresh tokens? If yes, this design needs rethinking (e.g. a shared token store keyed by user session). If no, document that assumption explicitly so a future server change does not silently break multi-account users.
3. MockConfigManager.storeOAuthTokens missing empty-string fallbacks
test/helpers/mock-config-manager.ts:
accountId: accountInfo?.accountId ?? this.config.accounts[alias]?.accountId, // undefined possible
accountName: accountInfo?.accountName ?? this.config.accounts[alias]?.accountName, // undefined possible
userEmail: tokens.userEmail ?? this.config.accounts[alias]?.userEmail, // undefined possibleTomlConfigManager uses ?? "" on all three to keep them as string per AccountConfig. The mock omits the fallbacks, which is a strictNullChecks violation. Add ?? "" to match — simple fix.
Minor: The PR title says "PKCE" but the implementation is Device Authorization Grant (RFC 8628), no PKCE involved. The description is accurate; the title is worth correcting before merge to avoid confusion in git history.
umair-ably
changed the title
umair-ably
force-pushed
the
oauth-login-flow
branch
2 times, most recently
from
437f835 to
9d55e1a
Compare
|
@claude all feedback has been addressed. Review again |
|
PR Review - OAuth Device Authorization Grant Login Flow Good overall. Four issues worth addressing before merge. ISSUE 1: this.warn() drops warnings in JSON mode (login.ts) ISSUE 2: OAuth session key does not include control host (config-manager.ts) ISSUE 3: Concurrent refresh from two processes can fail with single-use refresh tokens (token-refresh-middleware.ts) ISSUE 4: Dead code - revocationFailed event never fires (logout.ts) |
- 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.
Use configManager.getAccessToken() instead of destructuring account.accessToken directly, which may be undefined for OAuth accounts that store tokens in the shared session.
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.
umair-ably
force-pushed
the
oauth-login-flow
branch
from
a4d6264 to
c9c03b2
Compare
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.
Motivation
The CLI currently authenticates via manually copy-pasted access tokens — users have to go to the Ably dashboard, create a token, and paste it into the terminal. This is friction-heavy, error-prone, and doesn't support token refresh.
This PR replaces that flow with OAuth 2.0 Device Authorization Grant (RFC 8628). The user runs
ably login, gets a URL + code, approves in their browser, and the CLI receives tokens automatically. No secrets are ever pasted into a terminal. Tokens are stored locally and refreshed transparently.This is the CLI-side counterpart to ably/website#7962, which adds the OAuth provider to the website.
Summary of changes
Commit 1: Add OAuth Device Authorization Grant login flow
Core infrastructure — the building blocks everything else depends on.
src/services/oauth-client.ts— OAuth client implementing the Device Authorization Grant. Handles device code requests, token polling with exponential backoff, token refresh, and token revocation. Configurable viaOAuthConfigwith sensible defaults pointing toably.com.src/services/token-refresh-middleware.ts— Transparent token refresh middleware. Wraps Control API calls and automatically refreshes expired access tokens using the stored refresh token before retrying.src/services/config-manager.ts— Extended to store/retrieve OAuth tokens (access token, refresh token, expiry) alongside existing account config. New methods:getOAuthTokens,setOAuthTokens,clearOAuthTokens.src/services/control-api.ts— NewgetMe()andgetAccounts()methods for fetching user/account info after OAuth login.src/control-base-command.ts— Integrates the token refresh middleware so all Control API commands transparently refresh expired tokens.src/base-command.ts— Minor update to support the new auth flow.Commit 2: Add OAuth login and logout commands
The user-facing commands that use the infrastructure from commit 1.
src/commands/accounts/login.ts— Rewritten to use Device Authorization Grant. Shows a verification URL + user code, opens a browser (unless--no-browser), polls for approval, then fetches account info and stores credentials. Supports--aliasfor named accounts. Multi-account: if the user belongs to multiple Ably accounts, prompts them to choose which to log in as.src/commands/accounts/logout.ts— Enhanced to revoke OAuth tokens on the server before clearing local credentials.src/commands/login.ts— Alias updated to match.Commit 3: Add multi-account selection and switching
UX for users who belong to multiple Ably organizations/accounts.
src/commands/accounts/switch.ts— Expanded with interactive account selection. When switching, shows available accounts with their current app context. Supports switching by alias argument or interactive prompt.src/services/interactive-helper.ts— New interactive selection helpers used by login and switch flows.src/utils/slugify.ts— Utility to generate account aliases from account names (e.g., "My Company" → "my-company").Commit 4: Add tests for OAuth login flow
Comprehensive unit tests for all new code.
test/unit/services/oauth-client.test.ts— Device code request, token polling (success, pending, slow-down, expiry, errors), token refresh, token revocation, configuration.test/unit/services/token-refresh-middleware.test.ts— Transparent refresh on 401, refresh failure handling, non-401 passthrough.test/unit/commands/accounts/login.test.ts— OAuth login flow, multi-account selection, alias handling, error cases, JSON output.test/unit/commands/accounts/logout.test.ts— Token revocation on logout, graceful handling when revocation fails.test/unit/commands/accounts/switch.test.ts— Account switching by alias.test/unit/utils/slugify.test.ts— Slug generation edge cases.test/helpers/mock-config-manager.ts— Extended with OAuth token mock methods.Review guidance
Commit-by-commit review recommended — each commit is self-contained and builds on the previous.
oauth-client.ts, token refresh middleware edge casesKey design decisions
ControlBaseCommand--no-browserflag — for headless environments whereopendoesn't work; user manually copies the URLHow to manually test
ably login— should open browser with device code pageably apps list— should work with the OAuth tokenably accounts switch— should show interactive account pickerably accounts logout— should revoke token and clear local credentialsDEMO VID
OAuth.device.flow.mov