fix(accounts): stop stale Claude Code connections and false removal success
Accounts Connected now follows token validity instead of access-token presence. Windows removal uses unambiguous PowerShell, a clear that removes nothing is an error instead of a success toast, and the connected-row terminal control runs disconnect.
This commit is contained in:
@@ -0,0 +1,120 @@
|
||||
import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'
|
||||
import { atom } from 'nanostores'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { ConfirmHost } from '@/components/confirm-host'
|
||||
import { $confirmRequest } from '@/store/confirm'
|
||||
import type { OAuthProvider } from '@/types/hermes'
|
||||
|
||||
const listOAuthProviders = vi.fn()
|
||||
const disconnectOAuthProvider = vi.fn()
|
||||
const startManualProviderOAuth = vi.fn()
|
||||
const runInTerminal = vi.fn()
|
||||
const notify = vi.fn()
|
||||
const notifyError = vi.fn()
|
||||
|
||||
vi.mock('@/hermes', () => ({
|
||||
setApiRequestProfile: vi.fn(),
|
||||
getProfiles: async () => ({ profiles: [] }),
|
||||
disconnectOAuthProvider: (...args: unknown[]) => disconnectOAuthProvider(...args),
|
||||
getEnvVars: async () => ({}),
|
||||
listOAuthProviders: (...args: unknown[]) => listOAuthProviders(...args)
|
||||
}))
|
||||
|
||||
vi.mock('@/store/onboarding', () => ({
|
||||
$desktopOnboarding: atom({ manual: false }),
|
||||
startManualProviderOAuth: (...args: unknown[]) => startManualProviderOAuth(...args),
|
||||
startManualLocalEndpoint: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('@/app/right-sidebar/store', () => ({
|
||||
runInTerminal: (command: string) => runInTerminal(command)
|
||||
}))
|
||||
|
||||
vi.mock('@/store/notifications', () => ({
|
||||
notify: (notification: unknown) => notify(notification),
|
||||
notifyError: (...args: unknown[]) => notifyError(...args)
|
||||
}))
|
||||
|
||||
const COMMAND = 'Remove-Item -LiteralPath "$HOME/.external/credentials.json" -Force -ErrorAction Stop'
|
||||
|
||||
function connectedExternal(patch: Partial<OAuthProvider> = {}): OAuthProvider {
|
||||
return {
|
||||
cli_command: 'external login',
|
||||
disconnect_command: COMMAND,
|
||||
disconnectable: false,
|
||||
docs_url: '',
|
||||
flow: 'external',
|
||||
id: 'external-cli',
|
||||
name: 'External CLI',
|
||||
status: { logged_in: true },
|
||||
...patch
|
||||
}
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
listOAuthProviders.mockResolvedValue({ providers: [connectedExternal()] })
|
||||
disconnectOAuthProvider.mockResolvedValue({ ok: false, provider: 'nous' })
|
||||
Object.defineProperty(window, 'hermesDesktop', {
|
||||
configurable: true,
|
||||
value: { terminal: {} }
|
||||
})
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
cleanup()
|
||||
$confirmRequest.set(null)
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
async function renderAccounts() {
|
||||
const { ProvidersSettings } = await import('./providers-settings')
|
||||
|
||||
await act(async () => {
|
||||
render(
|
||||
<>
|
||||
<ProvidersSettings onClose={vi.fn()} onViewChange={vi.fn()} view="accounts" />
|
||||
<ConfirmHost />
|
||||
</>
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
describe('connected external provider row', () => {
|
||||
it('runs the terminal control as disconnect instead of starting sign-in', async () => {
|
||||
await renderAccounts()
|
||||
|
||||
fireEvent.click(await screen.findByRole('button', { name: /Disconnect External CLI in terminal/ }))
|
||||
fireEvent.click(await screen.findByRole('button', { name: 'Disconnect' }))
|
||||
|
||||
await waitFor(() => expect(runInTerminal).toHaveBeenCalledWith(COMMAND))
|
||||
expect(startManualProviderOAuth).not.toHaveBeenCalled()
|
||||
expect(notify).not.toHaveBeenCalledWith(expect.objectContaining({ kind: 'success' }))
|
||||
expect(notify).not.toHaveBeenCalledWith(expect.objectContaining({ title: 'Account removed' }))
|
||||
})
|
||||
|
||||
it('does not toast success when an API clear reports nothing was removed', async () => {
|
||||
listOAuthProviders.mockResolvedValue({
|
||||
providers: [
|
||||
{
|
||||
cli_command: 'hermes auth add nous',
|
||||
disconnectable: true,
|
||||
docs_url: '',
|
||||
flow: 'device_code',
|
||||
id: 'nous',
|
||||
name: 'Nous Portal',
|
||||
status: { logged_in: true }
|
||||
}
|
||||
]
|
||||
})
|
||||
|
||||
await renderAccounts()
|
||||
fireEvent.click(await screen.findByRole('button', { name: 'Remove Nous Portal' }))
|
||||
fireEvent.click(await screen.findByRole('button', { name: 'Disconnect' }))
|
||||
|
||||
await waitFor(() => expect(disconnectOAuthProvider).toHaveBeenCalled())
|
||||
expect(notify).not.toHaveBeenCalledWith(expect.objectContaining({ kind: 'success' }))
|
||||
expect(notify).not.toHaveBeenCalledWith(expect.objectContaining({ title: 'Account removed' }))
|
||||
expect(notifyError).toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
@@ -260,7 +260,10 @@ function ConnectedProviderRow({
|
||||
|
||||
return (
|
||||
<div className="group grid grid-cols-[minmax(0,1fr)_auto] items-center gap-1 rounded-[6px] transition-colors hover:bg-(--ui-control-hover-background)">
|
||||
<RowButton className="min-w-0 px-3 py-2.5 text-left" onClick={() => onSelect(provider)}>
|
||||
<RowButton
|
||||
className="min-w-0 px-3 py-2.5 text-left"
|
||||
onClick={() => (terminalDisconnect ? onTerminalDisconnect(provider) : onSelect(provider))}
|
||||
>
|
||||
<div className="flex min-w-0 items-center gap-2">
|
||||
<span className="truncate text-[length:var(--conversation-text-font-size)] font-semibold">{title}</span>
|
||||
<span className="inline-flex shrink-0 items-center gap-1 bg-primary/10 px-2 py-0.5 text-xs font-medium text-primary">
|
||||
@@ -276,7 +279,20 @@ function ConnectedProviderRow({
|
||||
)}
|
||||
</RowButton>
|
||||
<div className="flex items-center gap-1 pr-2">
|
||||
<Trail className="size-4 text-muted-foreground transition group-hover:text-foreground" />
|
||||
{terminalDisconnect ? (
|
||||
<Button
|
||||
aria-label={`${copy.disconnect} ${title} in terminal`}
|
||||
onClick={() => onTerminalDisconnect(provider)}
|
||||
size="icon-xs"
|
||||
title={copy.disconnectInTerminal}
|
||||
type="button"
|
||||
variant="ghost"
|
||||
>
|
||||
<Terminal className="size-4" />
|
||||
</Button>
|
||||
) : (
|
||||
<Trail className="size-4 text-muted-foreground transition group-hover:text-foreground" />
|
||||
)}
|
||||
{canDisconnect && (
|
||||
<Button
|
||||
aria-label={`${t.common.remove} ${title}`}
|
||||
@@ -423,7 +439,7 @@ export function ProvidersSettings({
|
||||
runInTerminal(command)
|
||||
notify({
|
||||
kind: 'info',
|
||||
title: t.settings.providers.removedTitle,
|
||||
title: t.settings.providers.disconnect,
|
||||
message: t.settings.providers.removeTerminalRunning(name)
|
||||
})
|
||||
}
|
||||
@@ -444,7 +460,13 @@ export function ProvidersSettings({
|
||||
setDisconnecting(provider.id)
|
||||
|
||||
try {
|
||||
await disconnectOAuthProvider(provider.id, scopeProfile)
|
||||
const result = await disconnectOAuthProvider(provider.id, scopeProfile)
|
||||
|
||||
if (!result?.ok) {
|
||||
notifyError(new Error('No stored credentials were removed'), t.settings.providers.failedRemove(name))
|
||||
return
|
||||
}
|
||||
|
||||
notify({
|
||||
durationMs: 3_000,
|
||||
kind: 'success',
|
||||
|
||||
@@ -559,20 +559,39 @@ async def _start_device_code_flow(provider_id: str, profile: Optional[str] = Non
|
||||
return await starter(profile)
|
||||
|
||||
|
||||
def _oauth_provider_disconnect_command(provider: Dict[str, Any]) -> Optional[str]:
|
||||
def _claude_code_disconnect_command(platform: str) -> str:
|
||||
"""Host-native command that removes Claude Code's borrowed credential file.
|
||||
|
||||
Windows must not emit ``rm -f``. PowerShell aliases ``rm`` to ``Remove-Item``,
|
||||
and ``-f`` binds both ``-Force`` and ``-Filter`` (AmbiguousParameter).
|
||||
"""
|
||||
if platform == "win32":
|
||||
literal = '"$HOME/.claude/.credentials.json"'
|
||||
return (
|
||||
f"if (Test-Path -LiteralPath {literal}) {{ "
|
||||
f"Remove-Item -LiteralPath {literal} -Force -ErrorAction Stop }}"
|
||||
)
|
||||
rm_file = "rm -f ~/.claude/.credentials.json"
|
||||
if platform == "darwin":
|
||||
return f'security delete-generic-password -s "Claude Code-credentials" 2>/dev/null; {rm_file}'
|
||||
return rm_file
|
||||
|
||||
|
||||
def _oauth_provider_disconnect_command(
|
||||
provider: Dict[str, Any], platform: Optional[str] = None
|
||||
) -> Optional[str]:
|
||||
"""Shell command that clears an external provider's credentials, or None.
|
||||
|
||||
The disconnect API never silently deletes files another CLI owns; the GUI runs
|
||||
this in its embedded terminal so the user sees exactly what executes. Claude Code
|
||||
has no scriptable logout, so remove what logout would: the macOS Keychain entry
|
||||
and/or ``~/.claude/.credentials.json`` (the two ``read_claude_code_credentials()`` sources).
|
||||
``platform`` is the host the command will run on (default: this process). Pass it
|
||||
explicitly in tests; do not fake ``sys.platform``.
|
||||
"""
|
||||
if provider.get("flow") != "external" or provider.get("id") != "claude-code":
|
||||
return None
|
||||
rm_file = "rm -f ~/.claude/.credentials.json"
|
||||
if sys.platform == "darwin":
|
||||
return f'security delete-generic-password -s "Claude Code-credentials" 2>/dev/null; {rm_file}'
|
||||
return rm_file
|
||||
return _claude_code_disconnect_command(platform or sys.platform)
|
||||
|
||||
|
||||
def _oauth_provider_disconnect_hint(provider: Dict[str, Any], status: Dict[str, Any]) -> Optional[str]:
|
||||
@@ -653,15 +672,25 @@ def _clear_anthropic_auth() -> bool:
|
||||
oauth_file.unlink()
|
||||
cleared = True
|
||||
except Exception:
|
||||
pass
|
||||
_log.exception("disconnect anthropic OAuth file failed")
|
||||
raise
|
||||
try:
|
||||
from hermes_cli.auth import clear_provider_auth
|
||||
cleared = clear_provider_auth("anthropic") or cleared
|
||||
except Exception:
|
||||
pass
|
||||
_log.exception("disconnect anthropic auth store failed")
|
||||
raise
|
||||
return cleared
|
||||
|
||||
|
||||
def _disconnect_http_error(status_code: int, provider_name: str) -> HTTPException:
|
||||
if status_code == 409:
|
||||
detail = f"No stored credentials were removed for {provider_name}."
|
||||
else:
|
||||
detail = f"Failed to remove stored credentials for {provider_name}."
|
||||
return HTTPException(status_code=status_code, detail=detail)
|
||||
|
||||
|
||||
@router.delete("/api/providers/oauth/{provider_id}")
|
||||
async def disconnect_oauth_provider(provider_id: str, request: Request, profile: Optional[str] = None):
|
||||
"""Disconnect an OAuth provider. Token-protected (matches /env/reveal)."""
|
||||
@@ -677,19 +706,31 @@ async def disconnect_oauth_provider(provider_id: str, request: Request, profile:
|
||||
_reject_if_not_disconnectable(provider, _resolve_provider_status(provider_id, provider.get("status_fn")))
|
||||
|
||||
if provider_id == "anthropic":
|
||||
cleared = _clear_anthropic_auth()
|
||||
try:
|
||||
cleared = _clear_anthropic_auth()
|
||||
except HTTPException:
|
||||
raise
|
||||
except Exception:
|
||||
_log.exception("disconnect %s failed", provider_id)
|
||||
raise _disconnect_http_error(500, provider["name"])
|
||||
if not cleared:
|
||||
raise _disconnect_http_error(409, provider["name"])
|
||||
_log.info("oauth/disconnect: %s", provider_id)
|
||||
return {"ok": bool(cleared), "provider": provider_id}
|
||||
return {"ok": True, "provider": provider_id}
|
||||
try:
|
||||
from hermes_cli.auth import clear_provider_auth, invalidate_nous_auth_status_cache
|
||||
cleared = clear_provider_auth(provider_id)
|
||||
if provider_id == "nous":
|
||||
invalidate_nous_auth_status_cache()
|
||||
if not cleared:
|
||||
raise _disconnect_http_error(409, provider["name"])
|
||||
_log.info("oauth/disconnect: %s (cleared=%s)", provider_id, cleared)
|
||||
return {"ok": bool(cleared), "provider": provider_id}
|
||||
except Exception as e:
|
||||
return {"ok": True, "provider": provider_id}
|
||||
except HTTPException:
|
||||
raise
|
||||
except Exception:
|
||||
_log.exception("disconnect %s failed", provider_id)
|
||||
raise HTTPException(status_code=500, detail=str(e))
|
||||
raise _disconnect_http_error(500, provider["name"])
|
||||
|
||||
return await scoped_to_thread(profile, _run)
|
||||
|
||||
|
||||
@@ -74,14 +74,21 @@ def _anthropic_oauth_status() -> Dict[str, Any]:
|
||||
|
||||
|
||||
def _claude_code_only_status() -> Dict[str, Any]:
|
||||
"""Claude Code CLI credentials as their own entry, independent of the Anthropic card."""
|
||||
"""Claude Code CLI credentials as their own entry, independent of the Anthropic card.
|
||||
|
||||
Connected follows the same local validity gate as Anthropic resolution. A
|
||||
persisted access token that has already expired is not a usable login.
|
||||
"""
|
||||
try:
|
||||
from agent.anthropic_credentials import read_claude_code_credentials
|
||||
from agent.anthropic_credentials import (
|
||||
is_claude_code_token_valid,
|
||||
read_claude_code_credentials,
|
||||
)
|
||||
creds = read_claude_code_credentials()
|
||||
if creds and is_claude_code_token_valid(creds):
|
||||
return _token_status("claude_code_cli", "~/.claude/.credentials.json", creds)
|
||||
except Exception:
|
||||
creds = None
|
||||
if creds and creds.get("accessToken"):
|
||||
return _token_status("claude_code_cli", "~/.claude/.credentials.json", creds)
|
||||
pass
|
||||
return dict(_LOGGED_OUT)
|
||||
|
||||
|
||||
|
||||
69
tests/hermes_cli/test_claude_code_account_connection.py
Normal file
69
tests/hermes_cli/test_claude_code_account_connection.py
Normal file
@@ -0,0 +1,69 @@
|
||||
"""Accounts Connected must follow Claude Code token validity, not token presence."""
|
||||
|
||||
from hermes_cli.web_server import _SESSION_TOKEN, app
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
client = TestClient(app)
|
||||
HEADERS = {"X-Hermes-Session-Token": _SESSION_TOKEN}
|
||||
|
||||
|
||||
def test_expired_claude_code_token_is_not_connected(monkeypatch):
|
||||
"""A present but expired access token must not mark the account Connected."""
|
||||
from agent import anthropic_credentials
|
||||
|
||||
monkeypatch.setattr(
|
||||
anthropic_credentials,
|
||||
"read_claude_code_credentials",
|
||||
lambda: {"accessToken": "stale-token", "expiresAt": 1},
|
||||
)
|
||||
|
||||
resp = client.get("/api/providers/oauth", headers=HEADERS)
|
||||
assert resp.status_code == 200, resp.text
|
||||
providers = {p["id"]: p for p in resp.json()["providers"]}
|
||||
|
||||
assert providers["claude-code"]["status"]["logged_in"] is False
|
||||
|
||||
|
||||
def test_windows_claude_code_removal_command_is_unambiguous_powershell():
|
||||
"""PowerShell must not see ``rm -f``: ``-f`` is an ambiguous Remove-Item parameter."""
|
||||
from hermes_cli.web_routers.oauth import _oauth_provider_disconnect_command
|
||||
|
||||
command = _oauth_provider_disconnect_command(
|
||||
{"id": "claude-code", "flow": "external"}, platform="win32"
|
||||
)
|
||||
|
||||
assert command is not None
|
||||
assert "Remove-Item" in command
|
||||
assert "-LiteralPath" in command
|
||||
assert "-Force" in command
|
||||
assert "rm -f" not in command
|
||||
assert " -f " not in command and not command.strip().endswith(" -f")
|
||||
|
||||
|
||||
def test_disconnect_is_not_success_when_nothing_was_cleared(monkeypatch):
|
||||
"""A no-op clear must not be a 200 the client can toast as removed."""
|
||||
from hermes_cli import auth as auth_mod
|
||||
|
||||
monkeypatch.setattr(auth_mod, "clear_provider_auth", lambda _provider: False)
|
||||
|
||||
resp = client.delete("/api/providers/oauth/nous", headers=HEADERS)
|
||||
|
||||
assert resp.status_code == 409, resp.text
|
||||
assert resp.json().get("ok") is not True
|
||||
|
||||
|
||||
def test_disconnect_failure_does_not_echo_the_store_error(monkeypatch):
|
||||
from hermes_cli import auth as auth_mod
|
||||
|
||||
secret = "auth store is read-only: /secret/hermes-home"
|
||||
|
||||
def fail_clear(_provider):
|
||||
raise OSError(secret)
|
||||
|
||||
monkeypatch.setattr(auth_mod, "clear_provider_auth", fail_clear)
|
||||
|
||||
resp = client.delete("/api/providers/oauth/nous", headers=HEADERS)
|
||||
|
||||
assert resp.status_code == 500, resp.text
|
||||
assert secret not in resp.text
|
||||
|
||||
Reference in New Issue
Block a user