[DX-547] Add OAuth2 PKCE login flow as default authentication method#144
[DX-547] Add OAuth2 PKCE login flow as default authentication method#144umair-ably wants to merge 4 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 | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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.
| private async openBrowser(url: string): Promise<void> { | ||
| private async oauthLogin(flags: BaseFlags): Promise<OAuthTokens> { | ||
| const oauthClient = new OAuthClient({ | ||
| controlHost: flags["control-host"], |
There was a problem hiding this comment.
OAuthClient is being configured with flags["control-host"], but that flag is documented as overriding the Control API host (default control.ably.net). OAuth endpoints live on the dashboard host (default ably.com), and there is already a global --dashboard-host flag.
To avoid surprising behavior when someone overrides --control-host (e.g. pointing Control API to a different env but still expecting OAuth against the dashboard), consider wiring OAuthClient to flags["dashboard-host"] (or introducing a dedicated --oauth-host), rather than reusing --control-host.
| controlHost: flags["control-host"], | |
| controlHost: flags["dashboard-host"], |
| const oauthClient = new OAuthClient({ | ||
| controlHost: flags["control-host"], | ||
| }); | ||
| // Best-effort revocation -- don't block on failure | ||
| await Promise.all([ | ||
| oauthClient.revokeToken(oauthTokens.accessToken), | ||
| oauthClient.revokeToken(oauthTokens.refreshToken), | ||
| ]).catch(() => {}); |
There was a problem hiding this comment.
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.
| if (this.configManager.getAuthMethod() === "oauth") { | ||
| const oauthClient = new OAuthClient({ | ||
| controlHost: flags["control-host"], | ||
| }); | ||
| tokenRefreshMiddleware = new TokenRefreshMiddleware( | ||
| this.configManager, | ||
| oauthClient, | ||
| ); |
There was a problem hiding this comment.
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.
| export interface OAuthClientOptions { | ||
| controlHost?: string; | ||
| } |
There was a problem hiding this comment.
OAuthClientOptions.controlHost is named like the Control API host, but it’s actually used to build OAuth authorization/token/revocation endpoints (defaulting to ably.com). Renaming this option to something like oauthHost / dashboardHost would make call sites clearer and reduce the risk of wiring the wrong flag (e.g. --control-host).
src/commands/accounts/login.ts
Outdated
| 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.login(async (url) => { | ||
| await openUrl(url, this); | ||
| }); |
There was a problem hiding this comment.
In --json / --pretty-json mode, this will still call openUrl(url, this), and openUrl always writes human-readable log lines (e.g. “Opening … in your browser…”). That will prepend non-JSON text before the final JSON output, making stdout invalid JSON for automation.
Consider skipping OAuth/interactive browser opening when shouldOutputJson(flags) is true (e.g. require a token arg / --legacy --no-browser), or call openUrl with a silent logger in JSON mode so stdout remains parseable.
src/commands/accounts/login.ts
Outdated
| if (!flags["no-browser"]) { | ||
| if (!this.shouldOutputJson(flags)) { | ||
| this.log("Opening browser to get an access token..."); | ||
| } | ||
| await openUrl(obtainTokenPath, this); | ||
| } else if (!this.shouldOutputJson(flags)) { |
There was a problem hiding this comment.
legacyTokenLogin() can also break JSON output: even though the surrounding code guards some this.log(...) calls, openUrl() itself logs unconditionally. If a user runs accounts:login --json without providing a token (or if OAuth falls back to legacy), the output will include “Opening …” lines before the JSON payload.
To keep JSON output valid, avoid calling openUrl() in JSON mode (or use a no-op logger), and ideally avoid any interactive prompting in JSON mode unless the token is provided via args/env.
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
src/commands/accounts/login.ts
Outdated
| } catch (error) { | ||
| if (!this.shouldOutputJson(flags)) { | ||
| this.log("Opening browser to get an access token..."); | ||
| this.warn( | ||
| `OAuth login failed: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| this.log("Falling back to manual token login..."); | ||
| } | ||
|
|
||
| await this.openBrowser(obtainTokenPath); | ||
| } else if (!this.shouldOutputJson(flags)) { | ||
| this.log(`Please visit ${obtainTokenPath} to create an access token`); | ||
| accessToken = await this.legacyTokenLogin(flags); | ||
| } |
There was a problem hiding this comment.
Avoid interactive fallback when --json is enabled.
Lines [100-108] fall back to legacyTokenLogin after OAuth failure, which prompts for token input and can hang non-interactive JSON workflows.
✅ Suggested patch
} catch (error) {
- if (!this.shouldOutputJson(flags)) {
- this.warn(
- `OAuth login failed: ${error instanceof Error ? error.message : String(error)}`,
- );
- this.log("Falling back to manual token login...");
- }
- accessToken = await this.legacyTokenLogin(flags);
+ if (this.shouldOutputJson(flags)) {
+ throw new Error(
+ `OAuth login failed: ${error instanceof Error ? error.message : String(error)}`,
+ );
+ }
+ this.warn(
+ `OAuth login failed: ${error instanceof Error ? error.message : String(error)}`,
+ );
+ this.log("Falling back to manual token login...");
+ accessToken = await this.legacyTokenLogin(flags);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| if (!this.shouldOutputJson(flags)) { | |
| this.log("Opening browser to get an access token..."); | |
| this.warn( | |
| `OAuth login failed: ${error instanceof Error ? error.message : String(error)}`, | |
| ); | |
| this.log("Falling back to manual token login..."); | |
| } | |
| await this.openBrowser(obtainTokenPath); | |
| } else if (!this.shouldOutputJson(flags)) { | |
| this.log(`Please visit ${obtainTokenPath} to create an access token`); | |
| accessToken = await this.legacyTokenLogin(flags); | |
| } | |
| } catch (error) { | |
| if (this.shouldOutputJson(flags)) { | |
| throw new Error( | |
| `OAuth login failed: ${error instanceof Error ? error.message : String(error)}`, | |
| ); | |
| } | |
| this.warn( | |
| `OAuth login failed: ${error instanceof Error ? error.message : String(error)}`, | |
| ); | |
| this.log("Falling back to manual token login..."); | |
| accessToken = await this.legacyTokenLogin(flags); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/accounts/login.ts` around lines 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.
| const oauthClient = new OAuthClient({ | ||
| controlHost: flags["control-host"], | ||
| }); |
There was a problem hiding this comment.
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.
| await Promise.all([ | ||
| oauthClient.revokeToken(oauthTokens.accessToken), | ||
| oauthClient.revokeToken(oauthTokens.refreshToken), | ||
| ]).catch(() => {}); |
There was a problem hiding this comment.
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.
| async refreshAccessToken(refreshToken: string): Promise<OAuthTokens> { | ||
| return this.postForTokens( | ||
| { | ||
| client_id: this.config.clientId, | ||
| grant_type: "refresh_token", | ||
| refresh_token: refreshToken, | ||
| }, | ||
| "Token refresh", | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "oauth-client.ts" --type fRepository: ably/ably-cli
Length of output: 85
🏁 Script executed:
cat -n src/services/oauth-client.ts | head -100Repository: ably/ably-cli
Length of output: 3168
🏁 Script executed:
cat -n src/services/oauth-client.ts | sed -n '280,320p'Repository: ably/ably-cli
Length of output: 1575
🌐 Web query:
OAuth 2.0 refresh token grant response specification RFC 6749
💡 Result:
In RFC 6749, the Refresh Token Grant is defined in Section 6, and its token endpoint response is the standard token response from Section 5.1 (success) or Section 5.2 (error). (datatracker.ietf.org)
Successful response (RFC 6749 §5.1; used by §6)
HTTP 200 with Content-Type: application/json and the following headers:
Cache-Control: no-storePragma: no-cache(datatracker.ietf.org)
JSON body parameters (top-level):
access_token(REQUIRED)token_type(REQUIRED)expires_in(RECOMMENDED)refresh_token(OPTIONAL; the server may issue a new one—if it does, the client must replace/discard the old one) (datatracker.ietf.org)scope(OPTIONAL if identical to what was requested; otherwise REQUIRED) (datatracker.ietf.org)
Error response (RFC 6749 §5.2; used by §6)
Typically HTTP 400 with application/json, plus the same no-store / no-cache headers, and JSON:
error(REQUIRED)error_description(OPTIONAL)error_uri(OPTIONAL) (datatracker.ietf.org)
Error codes include: invalid_request, invalid_client, invalid_grant, unauthorized_client, unsupported_grant_type, invalid_scope. (datatracker.ietf.org)
Citations:
- 1: https://datatracker.ietf.org/doc/rfc6749/?utm_source=openai
- 2: https://datatracker.ietf.org/doc/rfc6749/?utm_source=openai
- 3: https://datatracker.ietf.org/doc/rfc6749/?utm_source=openai
- 4: https://datatracker.ietf.org/doc/rfc6749/?utm_source=openai
- 5: https://datatracker.ietf.org/doc/rfc6749/?utm_source=openai
- 6: https://datatracker.ietf.org/doc/rfc6749/?utm_source=openai
🌐 Web query:
Ably OAuth 2.0 refresh token grant response includes refresh_token field
💡 Result:
Ably’s token authentication is not OAuth 2.0, so there is no OAuth “refresh_token grant” and an Ably token response does not include a refresh_token field. Instead, Ably returns a TokenDetails object whose fields are token, expires, issued, capability, and (optionally) clientId—and when a token is near expiry you obtain a new Ably token (the SDK can do this automatically via authUrl/authCallback). [1], [2]
🏁 Script executed:
pnpm exec eslint src/services/oauth-client.ts --format=compactRepository: ably/ably-cli
Length of output: 864
🏁 Script executed:
rg -A 5 -B 5 "refresh_token" src/ --type ts --type tsx -iRepository: ably/ably-cli
Length of output: 84
🏁 Script executed:
fd "test" --type f | head -20Repository: ably/ably-cli
Length of output: 863
🌐 Web query:
Ably OAuth 2.0 authorization server refresh token endpoint specification
💡 Result:
Ably is not an OAuth 2.0 / OpenID Connect authorization server, so it does not provide an OAuth 2.0 “refresh token endpoint” (i.e., no /oauth/token refresh-token grant for Ably auth).
Instead, Ably uses API keys and Ably-issued short-lived tokens (“token authentication”). When a token is near expiry, the Ably SDK automatically renews it by calling your authUrl (or authCallback) so your backend can mint a fresh Ably token (typically by using an API key to create a token request / token). Ably’s docs describe this renewal mechanism as “Token refresh” handled by the SDK. [1], [2]
🏁 Script executed:
rg "tokenEndpoint\|oauth" src/services/oauth-client.ts -A 2 -B 2Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
rg "getOAuthConfig" src/services/oauth-client.ts -A 20Repository: ably/ably-cli
Length of output: 1466
🏁 Script executed:
fd -e test.ts -e spec.ts -e test.js | xargs grep -l "oauth" -i 2>/dev/null | head -5Repository: ably/ably-cli
Length of output: 233
🏁 Script executed:
cat -n src/services/oauth-client.ts | sed -n '100,200p'Repository: ably/ably-cli
Length of output: 3956
🏁 Script executed:
cat -n test/unit/services/oauth-client.test.ts | head -150Repository: ably/ably-cli
Length of output: 6349
🏁 Script executed:
cat -n test/unit/services/oauth-client.test.ts | grep -A 10 -B 5 "refresh"Repository: ably/ably-cli
Length of output: 5383
🏁 Script executed:
cat -n test/unit/services/oauth-client.test.ts | grep -A 15 "parseTokenResponse\|data\\.refresh_token"Repository: ably/ably-cli
Length of output: 39
Preserve the existing refresh token when OAuth responses omit refresh_token.
According to OAuth 2.0 RFC 6749 §5.1, the refresh_token field in a token endpoint response is optional. If an authorization server omits it (allowed by spec), line 312 hard-casts data.refresh_token as string, resulting in undefined being assigned to the required refreshToken: string field. This breaks the next refresh cycle.
Pass the current refresh token as a fallback to parseTokenResponse() and use it when the response omits the field, preserving session continuity:
♻️ Suggested patch
- async refreshAccessToken(refreshToken: string): Promise<OAuthTokens> {
+ async refreshAccessToken(refreshToken: string): Promise<OAuthTokens> {
return this.postForTokens(
{
client_id: this.config.clientId,
grant_type: "refresh_token",
refresh_token: refreshToken,
},
"Token refresh",
+ refreshToken,
);
}
private async postForTokens(
params: Record<string, string>,
operationName: string,
+ fallbackRefreshToken?: string,
): Promise<OAuthTokens> {
@@
- return this.parseTokenResponse(data);
+ return this.parseTokenResponse(data, fallbackRefreshToken);
}
- private parseTokenResponse(data: Record<string, unknown>): OAuthTokens {
+ private parseTokenResponse(
+ data: Record<string, unknown>,
+ 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: data.access_token as string,
+ accessToken,
expiresAt: Date.now() + expiresIn * 1000,
- refreshToken: data.refresh_token as string,
+ refreshToken,
scope: data.scope as string | undefined,
tokenType: (data.token_type as string) || "Bearer",
userEmail: data.user_email as string | undefined,
userId: data.user_id === undefined ? undefined : String(data.user_id),
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/oauth-client.ts` around lines 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.
| async revokeToken(token: string): Promise<void> { | ||
| const params = new URLSearchParams({ | ||
| client_id: this.config.clientId, | ||
| token, | ||
| }); | ||
|
|
||
| try { | ||
| await fetch(this.config.revocationEndpoint, { | ||
| body: params.toString(), | ||
| headers: { "Content-Type": "application/x-www-form-urlencoded" }, | ||
| method: "POST", | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, get the file and check the specific line ranges mentioned
wc -l src/services/oauth-client.tsRepository: ably/ably-cli
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read the specific line ranges mentioned in the review
echo "=== Lines 95-106 (revokeToken) ==="
sed -n '95,106p' src/services/oauth-client.ts
echo ""
echo "=== Lines 290-294 (token exchange/refresh) ==="
sed -n '280,300p' src/services/oauth-client.tsRepository: ably/ably-cli
Length of output: 1068
🏁 Script executed:
#!/bin/bash
# Search for all fetch calls in the file to understand the scope
rg -n 'fetch\(' src/services/oauth-client.ts -A 8 -B 2Repository: ably/ably-cli
Length of output: 768
🏁 Script executed:
#!/bin/bash
# Check if any signal/AbortController is used in the file
rg -n 'AbortController|signal\s*:' src/services/oauth-client.ts -B 2 -A 2Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Run ESLint on the file to check for compliance
pnpm exec eslint src/services/oauth-client.ts 2>&1 | head -50Repository: ably/ably-cli
Length of output: 864
Add timeouts to OAuth HTTP calls to avoid indefinite hangs.
Lines 102 and 290 perform network requests without an abort/timeout mechanism. A stalled network can leave login/refresh stuck indefinitely.
Expected result: each fetch call should have a signal tied to an AbortController with a timeout. Apply this pattern to both revokeToken() (line 102) and postForTokens() (line 290).
🛡️ Suggested patch pattern
+ 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);Apply the same timeout/abort pattern to revokeToken().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/oauth-client.ts` around lines 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.
|
@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 🙂 |
Replace Authorization Code + PKCE flow with Device Authorization Grant. No localhost callback server needed, works in headless/SSH environments.
|
@lmars updated 🙂 added a new vid in the PR description |
src/commands/accounts/login.ts
Outdated
| 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!
Device flow handles both browser and no-browser cases. Direct token argument still works for CI/automation.
|
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 |
After authenticating via device flow, the CLI now fetches all accounts the user belongs to and prompts them to choose one. Account switching reuses the existing OAuth token to discover and add remote accounts without re-authenticating.
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({| } else { | ||
| // Multiple accounts in JSON mode or empty (backward compat: use first) | ||
| selectedAccountInfo = accounts[0]; | ||
| } |
There was a problem hiding this comment.
Crash when getAccounts returns empty array
Medium Severity
The else branch on line 114-116 handles both "multiple accounts in JSON mode" and "empty accounts list" (as the comment acknowledges). If getAccounts() returns a 200 with an empty array, accounts[0] is undefined, causing a crash when the code subsequently accesses selectedAccountInfo.id and selectedAccountInfo.name. The getAccounts fallback only covers a 404 error, not an empty successful response.
| this.configManager.storeOAuthTokens(alias, oauthTokens, { | ||
| accountId: selectedAccountInfo.id, | ||
| accountName: selectedAccountInfo.name, | ||
| }); |
There was a problem hiding this comment.
OAuth login doesn't store user email from API
Medium Severity
When storing OAuth tokens, user.email from the /me response is not passed to storeOAuthTokens, unlike the direct-token path which passes userEmail: user.email. The OAuth path relies on oauthTokens.userEmail from the token endpoint, which may be absent. This leaves userEmail undefined in the config for OAuth accounts, even though it's available from the /me call.
| tokenRefreshMiddleware = new TokenRefreshMiddleware( | ||
| this.configManager, | ||
| oauthClient, | ||
| ); |
There was a problem hiding this comment.
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.
REMOVED!!!
oauth.cli.mov
NEW - DEVICE FLOW:
OAuth.device.flow.mov
Summary by CodeRabbit
Release Notes
--legacyflag.Note
Cursor Bugbot is generating a summary for commit 9e8402d. Configure here.