diff --git a/codeframe/ui/models.py b/codeframe/ui/models.py index 19d34892..4964f8a6 100644 --- a/codeframe/ui/models.py +++ b/codeframe/ui/models.py @@ -948,3 +948,52 @@ class AgentSettingsResponse(AgentSettings): class UpdateAgentSettingsRequest(AgentSettings): """Request shape for PUT /api/v2/settings.""" + + +# ============================================================================ +# API Key Management (issue #555) +# ============================================================================ + + +KeyProvider = Literal["LLM_ANTHROPIC", "LLM_OPENAI", "GIT_GITHUB"] +KEY_PROVIDERS: tuple[KeyProvider, ...] = ("LLM_ANTHROPIC", "LLM_OPENAI", "GIT_GITHUB") + +KeySource = Literal["environment", "stored", "none"] + + +class StoreKeyRequest(BaseModel): + """Request shape for PUT /api/v2/settings/keys/{provider}.""" + + value: str = Field(..., min_length=1, description="The credential value to store") + + +class KeyStatusResponse(BaseModel): + """Status of a single API key. Never includes the plaintext value.""" + + provider: KeyProvider = Field(..., description="One of: LLM_ANTHROPIC, LLM_OPENAI, GIT_GITHUB") + stored: bool = Field(..., description="True if a key is available (env or storage)") + source: KeySource = Field( + ..., description="Where the key comes from: environment, stored, or none" + ) + last_four: Optional[str] = Field( + default=None, + description="Last 4 characters of the key for display (None if no key available)", + ) + + +class VerifyKeyRequest(BaseModel): + """Request shape for POST /api/v2/settings/verify-key.""" + + provider: KeyProvider = Field(..., description="Which provider's key to verify") + value: Optional[str] = Field( + default=None, + description="The key value to verify; if None, the stored/env key is used", + ) + + +class VerifyKeyResponse(BaseModel): + """Result of a live verification attempt against a provider.""" + + provider: KeyProvider + valid: bool = Field(..., description="True if the provider accepted the key") + message: str = Field(..., description="Human-readable result message") diff --git a/codeframe/ui/routers/settings_v2.py b/codeframe/ui/routers/settings_v2.py index 5da52920..8286a785 100644 --- a/codeframe/ui/routers/settings_v2.py +++ b/codeframe/ui/routers/settings_v2.py @@ -1,16 +1,27 @@ -"""V2 Settings router — agent settings managed via the web UI. - -Reads/writes a flat AgentSettings shape persisted in -.codeframe/config.yaml via load_environment_config / save_environment_config. +"""V2 Settings router — agent settings + API key management. Routes: - GET /api/v2/settings - Load agent settings (returns defaults if missing) - PUT /api/v2/settings - Save agent settings (merges into existing config) + GET /api/v2/settings - Load agent settings (defaults if missing) + PUT /api/v2/settings - Save agent settings (merge into config) + GET /api/v2/settings/keys - List API key status for all providers + PUT /api/v2/settings/keys/{p} - Store an API key for provider p + DELETE /api/v2/settings/keys/{p} - Delete an API key for provider p + POST /api/v2/settings/verify-key - Live-verify a key against its provider + +Key management is machine-wide (CredentialManager / keyring) and does not +require a workspace. Env vars take precedence at read time. """ import logging -from fastapi import APIRouter, Depends, HTTPException, Request +from typing import cast + +from anthropic import Anthropic as _AnthropicClient +from anthropic import AuthenticationError as _AnthropicAuthError +from fastapi import APIRouter, Depends, HTTPException, Request, Response +from fastapi.concurrency import run_in_threadpool +from openai import AuthenticationError as _OpenAIAuthError +from openai import OpenAI as _OpenAIClient from codeframe.core.config import ( AgentBudgetConfig, @@ -18,14 +29,26 @@ load_environment_config, save_environment_config, ) +from codeframe.core.credentials import ( + CredentialManager, + CredentialProvider, + CredentialSource, + validate_credential_format, +) from codeframe.core.workspace import Workspace -from codeframe.lib.rate_limiter import rate_limit_standard +from codeframe.lib.rate_limiter import rate_limit_ai, rate_limit_standard from codeframe.ui.dependencies import get_v2_workspace from codeframe.ui.models import ( AGENT_TYPES, + KEY_PROVIDERS, AgentSettingsResponse, AgentTypeModelConfig, + KeyProvider, + KeyStatusResponse, + StoreKeyRequest, UpdateAgentSettingsRequest, + VerifyKeyRequest, + VerifyKeyResponse, ) from codeframe.ui.response_models import ErrorCodes, api_error @@ -34,6 +57,14 @@ router = APIRouter(prefix="/api/v2/settings", tags=["settings"]) +def get_credential_manager() -> CredentialManager: + """Dependency: machine-wide CredentialManager. + + Overridden in tests to point at an isolated temp directory. + """ + return CredentialManager() + + def _config_to_response(config: EnvironmentConfig) -> AgentSettingsResponse: """Map an EnvironmentConfig to the flat AgentSettings response shape.""" saved_models = config.agent_type_models or {} @@ -114,3 +145,244 @@ async def update_settings( "Failed to save settings", ErrorCodes.EXECUTION_FAILED, str(e) ), ) + + +# ============================================================================ +# API Key Management (issue #555) +# +# These endpoints are machine-wide: they do not require a workspace. Keys are +# stored via CredentialManager (platform keyring or encrypted file fallback). +# Plaintext key values are NEVER returned in any response. +# ============================================================================ + +# Map the public provider name to the internal CredentialProvider enum. +_PROVIDER_MAP: dict[str, CredentialProvider] = { + "LLM_ANTHROPIC": CredentialProvider.LLM_ANTHROPIC, + "LLM_OPENAI": CredentialProvider.LLM_OPENAI, + "GIT_GITHUB": CredentialProvider.GIT_GITHUB, +} + + +def _resolve_provider(provider: str) -> CredentialProvider: + """Resolve a provider name from path/body, raising 400 for unknown values.""" + cp = _PROVIDER_MAP.get(provider) + if cp is None: + raise HTTPException( + status_code=400, + detail=api_error( + f"Unknown provider: {provider}. Allowed: {', '.join(KEY_PROVIDERS)}", + ErrorCodes.VALIDATION_ERROR, + ), + ) + return cp + + +def _last_four(value: str | None) -> str | None: + """Return the last 4 chars of a key, or None if no value. + + For values shorter than 4 chars (which format validation should + already reject), returns the full value. + """ + if not value: + return None + return value[-4:] if len(value) >= 4 else value + + +def _build_status( + provider_key: KeyProvider, + manager: CredentialManager, +) -> KeyStatusResponse: + cp = _PROVIDER_MAP[provider_key] + source = manager.get_credential_source(cp) + if source == CredentialSource.NOT_FOUND: + return KeyStatusResponse( + provider=provider_key, stored=False, source="none", last_four=None + ) + + value = manager.get_credential(cp) + source_str = "environment" if source == CredentialSource.ENVIRONMENT else "stored" + return KeyStatusResponse( + provider=provider_key, + stored=value is not None, + source=source_str, + last_four=_last_four(value), + ) + + +@router.get("/keys", response_model=list[KeyStatusResponse]) +@rate_limit_standard() +async def list_key_status( + request: Request, + manager: CredentialManager = Depends(get_credential_manager), +) -> list[KeyStatusResponse]: + """Return status of each known API key without exposing plaintext.""" + return [_build_status(p, manager) for p in KEY_PROVIDERS] + + +@router.put("/keys/{provider}", response_model=KeyStatusResponse) +@rate_limit_standard() +async def store_key( + provider: str, + body: StoreKeyRequest, + request: Request, + manager: CredentialManager = Depends(get_credential_manager), +) -> KeyStatusResponse: + """Store an API key for the given provider after validating its format.""" + cp = _resolve_provider(provider) + if not validate_credential_format(cp, body.value): + raise HTTPException( + status_code=400, + detail=api_error( + f"Invalid {provider} key format", + ErrorCodes.VALIDATION_ERROR, + ), + ) + try: + manager.set_credential(cp, body.value) + except Exception as e: + logger.error(f"Failed to store credential: {e}", exc_info=True) + raise HTTPException( + status_code=500, + detail=api_error( + "Failed to store credential", ErrorCodes.EXECUTION_FAILED, str(e) + ), + ) + # _resolve_provider validated `provider` against KEY_PROVIDERS, so the + # cast is safe — clearer than a type: ignore. + return _build_status(cast(KeyProvider, provider), manager) + + +@router.delete("/keys/{provider}", status_code=204) +@rate_limit_standard() +async def delete_key( + provider: str, + request: Request, + manager: CredentialManager = Depends(get_credential_manager), +) -> Response: + """Delete a stored credential. Idempotent — non-existent keys are a no-op.""" + cp = _resolve_provider(provider) + try: + manager.delete_credential(cp) + except Exception as e: + # Underlying store can raise on a hard keyring error; treat absence as success. + msg = str(e).lower() + if "no such" in msg or "not found" in msg: + return Response(status_code=204) + logger.error(f"Failed to delete credential: {e}", exc_info=True) + raise HTTPException( + status_code=500, + detail=api_error( + "Failed to delete credential", ErrorCodes.EXECUTION_FAILED, str(e) + ), + ) + return Response(status_code=204) + + +# ---------------------------------------------------------------------------- +# Live verification helpers +# +# These are module-level so tests can monkeypatch them without touching the +# real network. The Anthropic / OpenAI clients are sync, so we wrap their +# calls in run_in_threadpool to avoid blocking the event loop. +# ---------------------------------------------------------------------------- + + +async def _check_github_token(token: str) -> tuple[bool, str]: + """Validate a GitHub token via GET https://api.github.com/user. + + Exception messages are NOT echoed back to the client because aiohttp + errors can include the request URL or headers (which carry the token). + The detailed error is logged server-side instead. + """ + import aiohttp + + headers = { + "Authorization": f"token {token}", + "Accept": "application/vnd.github+json", + "User-Agent": "codeframe-key-verify", + } + try: + timeout = aiohttp.ClientTimeout(total=10) + async with aiohttp.ClientSession(timeout=timeout) as session: + async with session.get( + "https://api.github.com/user", headers=headers + ) as resp: + if resp.status == 200: + data = await resp.json() + login = data.get("login", "") + return True, f"Authenticated as {login}" if login else "Valid token" + if resp.status == 401: + return False, "401 Unauthorized: invalid GitHub token" + return False, f"GitHub API returned status {resp.status}" + except Exception as e: + logger.warning(f"GitHub token verification raised: {e}", exc_info=True) + return False, "GitHub verification failed: network or server error" + + +def _verify_anthropic_sync(key: str) -> tuple[bool, str]: + """Verify an Anthropic key by issuing a minimal messages.create() call. + + Uses messages.create rather than models.list because messages is the + stable, always-present API surface across all supported SDK versions + (>=0.18). max_tokens=1 keeps the cost trivial. Non-auth exceptions + are logged but not echoed to the client to avoid leaking provider + internals. + """ + try: + client = _AnthropicClient(api_key=key) + client.messages.create( + model="claude-haiku-4-5-20251001", + max_tokens=1, + messages=[{"role": "user", "content": "ping"}], + ) + return True, "Anthropic key accepted" + except _AnthropicAuthError: + return False, "Anthropic key rejected: authentication failed" + except Exception as e: + logger.warning(f"Anthropic verification raised: {e}", exc_info=True) + return False, "Anthropic verification failed: network or server error" + + +def _verify_openai_sync(key: str) -> tuple[bool, str]: + """Verify an OpenAI key via models.list (small, cheap, auth-required).""" + try: + client = _OpenAIClient(api_key=key) + client.models.list() + return True, "OpenAI key accepted" + except _OpenAIAuthError: + return False, "OpenAI key rejected: authentication failed" + except Exception as e: + logger.warning(f"OpenAI verification raised: {e}", exc_info=True) + return False, "OpenAI verification failed: network or server error" + + +@router.post("/verify-key", response_model=VerifyKeyResponse) +@rate_limit_ai() +async def verify_key( + body: VerifyKeyRequest, + request: Request, + manager: CredentialManager = Depends(get_credential_manager), +) -> VerifyKeyResponse: + """Live-verify a key against its provider. + + If body.value is None, the stored or env-var key is used. Failed + verifications return 200 with valid=false; only unexpected errors + (programmer bugs) raise 5xx. + """ + cp = _resolve_provider(body.provider) + key = body.value if body.value else manager.get_credential(cp) + if not key: + return VerifyKeyResponse( + provider=body.provider, valid=False, message="No key provided or stored" + ) + + if cp == CredentialProvider.LLM_ANTHROPIC: + valid, message = await run_in_threadpool(_verify_anthropic_sync, key) + elif cp == CredentialProvider.LLM_OPENAI: + valid, message = await run_in_threadpool(_verify_openai_sync, key) + elif cp == CredentialProvider.GIT_GITHUB: + valid, message = await _check_github_token(key) + else: # pragma: no cover -- guarded by _resolve_provider + valid, message = False, "Verification not supported for this provider" + + return VerifyKeyResponse(provider=body.provider, valid=valid, message=message) diff --git a/docs/PRODUCT_ROADMAP.md b/docs/PRODUCT_ROADMAP.md index 078c6a78..b9e36fac 100644 --- a/docs/PRODUCT_ROADMAP.md +++ b/docs/PRODUCT_ROADMAP.md @@ -192,7 +192,7 @@ These are items that were considered and excluded because they do not serve the | 3.5C | Glitch capture UI | ✅ Complete | #568, #569 | | 4A | PR status + PROOF9 merge gate | ❌ Not started | — | | 4B | Post-merge glitch capture loop | ❌ Not started | — | -| 5.1 | Settings page (skeleton + agent config) | 🟡 In progress (#554 done; #555–556 next) | #554–556 | +| 5.1 | Settings page (skeleton + agent config) | 🟡 In progress (#554, #555 done; #556 next) | #554–556 | | 5.2 | Cost analytics | ❌ Not started | #557–558 | | 5.3 | Async notifications | ❌ Not started | #559–560 | | 5.4 | PRD stress-test web UI | ❌ Not started | #561–562 | diff --git a/tests/ui/test_settings_v2.py b/tests/ui/test_settings_v2.py index e1cf3452..9db86fa8 100644 --- a/tests/ui/test_settings_v2.py +++ b/tests/ui/test_settings_v2.py @@ -53,6 +53,43 @@ def get_test_workspace(): yield client +@pytest.fixture +def temp_credentials_dir(tmp_path, monkeypatch): + """Provide an isolated CredentialManager backed by a temp dir. + + Patches the settings_v2 dependency so endpoints use a clean store + that does not touch the developer's real keyring or ~/.codeframe. + Also clears the three target env vars so source detection is deterministic. + """ + from codeframe.core.credentials import CredentialManager, CredentialStore + + # Force file-backed store so tests are independent of any system keyring. + store = CredentialStore(storage_dir=tmp_path) + store._keyring_available = False + manager = CredentialManager.__new__(CredentialManager) + manager._store = store + + # Clear env vars so stored/none sources are detected reliably. + for var in ("ANTHROPIC_API_KEY", "OPENAI_API_KEY", "GITHUB_TOKEN"): + monkeypatch.delenv(var, raising=False) + + yield manager + + +@pytest.fixture +def keys_client(temp_credentials_dir): + """TestClient where the credential manager dependency is overridden.""" + from codeframe.ui.routers import settings_v2 + + app = FastAPI() + app.include_router(settings_v2.router) + + app.dependency_overrides[settings_v2.get_credential_manager] = ( + lambda: temp_credentials_dir + ) + yield TestClient(app) + + class TestSettingsV2Get: """Tests for GET /api/v2/settings.""" @@ -255,3 +292,284 @@ def test_get_handles_null_agent_budget(self, test_client, test_workspace): response = test_client.get("/api/v2/settings") assert response.status_code == 200 assert response.json()["max_turns"] == 100 # AgentBudgetConfig default + + +# ============================================================================ +# API Key Management (issue #555) +# ============================================================================ + + +VALID_ANTHROPIC = "sk-ant-api03-" + "x" * 32 +VALID_OPENAI = "sk-proj-" + "x" * 32 +VALID_GITHUB = "ghp_" + "x" * 36 + + +class TestSettingsV2KeysStatus: + """Tests for GET /api/v2/settings/keys.""" + + def test_returns_three_providers_when_empty(self, keys_client): + response = keys_client.get("/api/v2/settings/keys") + assert response.status_code == 200 + data = response.json() + providers = {entry["provider"] for entry in data} + assert providers == {"LLM_ANTHROPIC", "LLM_OPENAI", "GIT_GITHUB"} + for entry in data: + assert entry["stored"] is False + assert entry["source"] == "none" + assert entry["last_four"] is None + + def test_status_reports_stored_source(self, keys_client): + keys_client.put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": VALID_ANTHROPIC} + ) + response = keys_client.get("/api/v2/settings/keys") + anth = next(e for e in response.json() if e["provider"] == "LLM_ANTHROPIC") + assert anth["stored"] is True + assert anth["source"] == "stored" + assert anth["last_four"] == VALID_ANTHROPIC[-4:] + + def test_status_reports_environment_source(self, keys_client, monkeypatch): + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-from-env-12345678") + response = keys_client.get("/api/v2/settings/keys") + anth = next(e for e in response.json() if e["provider"] == "LLM_ANTHROPIC") + assert anth["stored"] is True + assert anth["source"] == "environment" + assert anth["last_four"] == "5678" + + def test_status_never_returns_plaintext(self, keys_client): + keys_client.put( + "/api/v2/settings/keys/LLM_OPENAI", json={"value": VALID_OPENAI} + ) + response = keys_client.get("/api/v2/settings/keys") + body_text = response.text + assert VALID_OPENAI not in body_text + assert VALID_OPENAI[5:] not in body_text + + +class TestSettingsV2KeysStore: + """Tests for PUT /api/v2/settings/keys/{provider}.""" + + def test_store_persists_credential(self, keys_client, temp_credentials_dir): + response = keys_client.put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": VALID_ANTHROPIC} + ) + assert response.status_code == 200 + data = response.json() + assert data["stored"] is True + assert data["source"] == "stored" + assert data["last_four"] == VALID_ANTHROPIC[-4:] + + from codeframe.core.credentials import CredentialProvider + + assert ( + temp_credentials_dir.get_credential(CredentialProvider.LLM_ANTHROPIC) + == VALID_ANTHROPIC + ) + + def test_store_rejects_invalid_format(self, keys_client): + response = keys_client.put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": "not-a-real-key"} + ) + assert response.status_code == 400 + + def test_store_rejects_unknown_provider(self, keys_client): + response = keys_client.put( + "/api/v2/settings/keys/EVIL_PROVIDER", json={"value": "x" * 30} + ) + assert response.status_code in (400, 422) + + def test_store_rejects_empty_value(self, keys_client): + response = keys_client.put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": ""} + ) + assert response.status_code == 422 + + def test_store_response_excludes_plaintext(self, keys_client): + response = keys_client.put( + "/api/v2/settings/keys/GIT_GITHUB", json={"value": VALID_GITHUB} + ) + assert VALID_GITHUB not in response.text + + +class TestSettingsV2KeysDelete: + """Tests for DELETE /api/v2/settings/keys/{provider}.""" + + def test_delete_removes_credential(self, keys_client, temp_credentials_dir): + keys_client.put( + "/api/v2/settings/keys/LLM_OPENAI", json={"value": VALID_OPENAI} + ) + response = keys_client.delete("/api/v2/settings/keys/LLM_OPENAI") + assert response.status_code == 204 + + from codeframe.core.credentials import CredentialProvider + + assert ( + temp_credentials_dir.get_credential(CredentialProvider.LLM_OPENAI) is None + ) + + def test_delete_is_idempotent(self, keys_client): + # Deleting a non-existent credential should not raise. + response = keys_client.delete("/api/v2/settings/keys/LLM_ANTHROPIC") + assert response.status_code == 204 + + def test_delete_rejects_unknown_provider(self, keys_client): + response = keys_client.delete("/api/v2/settings/keys/EVIL_PROVIDER") + assert response.status_code in (400, 422) + + +class TestSettingsV2VerifyKey: + """Tests for POST /api/v2/settings/verify-key. + + Verification calls patch the underlying SDK / HTTP client so tests + don't make real network requests. + """ + + def test_verify_returns_invalid_for_missing_key(self, keys_client): + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "LLM_ANTHROPIC", "value": None}, + ) + assert response.status_code == 200 + data = response.json() + assert data["valid"] is False + assert "no" in data["message"].lower() or "missing" in data["message"].lower() + + def test_verify_anthropic_calls_messages_create(self, keys_client, monkeypatch): + from codeframe.ui.routers import settings_v2 + + called_with = {} + + class FakeMessages: + def create(self_, **kwargs): + called_with["kwargs"] = kwargs + return {"id": "msg_test"} + + class FakeAnthropicClient: + def __init__(self, api_key): + called_with["key"] = api_key + self.messages = FakeMessages() + + monkeypatch.setattr(settings_v2, "_AnthropicClient", FakeAnthropicClient) + + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "LLM_ANTHROPIC", "value": VALID_ANTHROPIC}, + ) + assert response.status_code == 200 + data = response.json() + assert data["valid"] is True + assert called_with["key"] == VALID_ANTHROPIC + assert called_with["kwargs"]["max_tokens"] == 1 + + def test_verify_anthropic_handles_auth_error(self, keys_client, monkeypatch): + from anthropic import AuthenticationError + + from codeframe.ui.routers import settings_v2 + + class FakeMessages: + def create(self_, **kwargs): + # Construct the AuthenticationError without going through the + # SDK's response machinery; only the message is asserted on. + err = AuthenticationError.__new__(AuthenticationError) + Exception.__init__(err, "401 unauthorized") + raise err + + class FakeAnthropicClient: + def __init__(self, api_key): + self.messages = FakeMessages() + + monkeypatch.setattr(settings_v2, "_AnthropicClient", FakeAnthropicClient) + + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "LLM_ANTHROPIC", "value": "sk-ant-bad-key-1234567890"}, + ) + assert response.status_code == 200 + data = response.json() + assert data["valid"] is False + # Sanitised message: never echoes the raw exception detail. + assert "rejected" in data["message"].lower() + assert "sk-ant-bad-key" not in data["message"] + + def test_verify_uses_stored_when_value_omitted(self, keys_client, monkeypatch): + from codeframe.ui.routers import settings_v2 + + captured = {} + + class FakeMessages: + def create(self_, **kwargs): + return {"id": "msg_ok"} + + class FakeAnthropicClient: + def __init__(self, api_key): + captured["key"] = api_key + self.messages = FakeMessages() + + monkeypatch.setattr(settings_v2, "_AnthropicClient", FakeAnthropicClient) + + keys_client.put( + "/api/v2/settings/keys/LLM_ANTHROPIC", json={"value": VALID_ANTHROPIC} + ) + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "LLM_ANTHROPIC", "value": None}, + ) + assert response.status_code == 200 + assert response.json()["valid"] is True + assert captured["key"] == VALID_ANTHROPIC + + def test_verify_openai_calls_models_list(self, keys_client, monkeypatch): + from codeframe.ui.routers import settings_v2 + + class FakeOpenAIClient: + def __init__(self, api_key): + self.models = type("M", (), {"list": lambda self_: ["gpt-4"]})() + + monkeypatch.setattr(settings_v2, "_OpenAIClient", FakeOpenAIClient) + + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "LLM_OPENAI", "value": VALID_OPENAI}, + ) + assert response.status_code == 200 + assert response.json()["valid"] is True + + def test_verify_github_uses_http(self, keys_client, monkeypatch): + from codeframe.ui.routers import settings_v2 + + async def fake_check_github(token: str) -> tuple[bool, str]: + assert token == VALID_GITHUB + return True, "ok" + + monkeypatch.setattr(settings_v2, "_check_github_token", fake_check_github) + + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "GIT_GITHUB", "value": VALID_GITHUB}, + ) + assert response.status_code == 200 + assert response.json()["valid"] is True + + def test_verify_github_handles_failure(self, keys_client, monkeypatch): + from codeframe.ui.routers import settings_v2 + + async def fake_check_github(token: str) -> tuple[bool, str]: + return False, "401 Unauthorized" + + monkeypatch.setattr(settings_v2, "_check_github_token", fake_check_github) + + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "GIT_GITHUB", "value": VALID_GITHUB}, + ) + assert response.status_code == 200 + data = response.json() + assert data["valid"] is False + assert "401" in data["message"] + + def test_verify_rejects_unknown_provider(self, keys_client): + response = keys_client.post( + "/api/v2/settings/verify-key", + json={"provider": "EVIL", "value": "x"}, + ) + assert response.status_code == 422 diff --git a/web-ui/src/__tests__/components/settings/ApiKeysTab.test.tsx b/web-ui/src/__tests__/components/settings/ApiKeysTab.test.tsx new file mode 100644 index 00000000..71fd2b82 --- /dev/null +++ b/web-ui/src/__tests__/components/settings/ApiKeysTab.test.tsx @@ -0,0 +1,219 @@ +import React from 'react'; +import { render, screen, fireEvent, waitFor, act } from '@testing-library/react'; +import useSWR from 'swr'; +import { toast } from 'sonner'; + +import { ApiKeysTab } from '@/components/settings/ApiKeysTab'; +import { settingsApi } from '@/lib/api'; +import type { KeyStatusResponse } from '@/types'; + +jest.mock('swr'); +jest.mock('@/lib/api', () => ({ + settingsApi: { + getKeys: jest.fn(), + storeKey: jest.fn(), + removeKey: jest.fn(), + verifyKey: jest.fn(), + }, +})); +jest.mock('sonner', () => ({ + toast: { + success: jest.fn(), + info: jest.fn(), + error: jest.fn(), + }, +})); + +const mockUseSWR = useSWR as jest.MockedFunction; +const mockStore = settingsApi.storeKey as jest.MockedFunction; +const mockRemove = settingsApi.removeKey as jest.MockedFunction; +const mockVerify = settingsApi.verifyKey as jest.MockedFunction; + +function mockSWR(data: KeyStatusResponse[] | undefined, mutate = jest.fn()) { + mockUseSWR.mockReturnValue({ + data, + error: undefined, + isLoading: data === undefined, + mutate, + } as unknown as ReturnType); + return mutate; +} + +const NONE_STATUS: KeyStatusResponse[] = [ + { provider: 'LLM_ANTHROPIC', stored: false, source: 'none', last_four: null }, + { provider: 'LLM_OPENAI', stored: false, source: 'none', last_four: null }, + { provider: 'GIT_GITHUB', stored: false, source: 'none', last_four: null }, +]; + +describe('ApiKeysTab', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('renders three key slots when no keys are stored', () => { + mockSWR(NONE_STATUS); + render(); + expect(screen.getByText('Anthropic API Key')).toBeInTheDocument(); + expect(screen.getByText('OpenAI API Key')).toBeInTheDocument(); + expect(screen.getByText('GitHub Personal Access Token')).toBeInTheDocument(); + }); + + it('shows loading state while data is undefined', () => { + mockSWR(undefined); + render(); + expect(screen.getByText(/Loading/i)).toBeInTheDocument(); + }); + + it('shows error state when load fails', () => { + mockUseSWR.mockReturnValue({ + data: undefined, + error: new Error('boom'), + isLoading: false, + mutate: jest.fn(), + } as unknown as ReturnType); + render(); + expect(screen.getByText(/Failed to load API key status/i)).toBeInTheDocument(); + }); + + it('shows last 4 of stored key without exposing plaintext', () => { + const stored: KeyStatusResponse[] = [ + { provider: 'LLM_ANTHROPIC', stored: true, source: 'stored', last_four: 'aaaa' }, + { provider: 'LLM_OPENAI', stored: false, source: 'none', last_four: null }, + { provider: 'GIT_GITHUB', stored: false, source: 'none', last_four: null }, + ]; + mockSWR(stored); + render(); + + const anthropicInput = screen.getByLabelText(/Anthropic API Key/i); + expect(anthropicInput).toHaveAttribute('type', 'password'); + expect((anthropicInput as HTMLInputElement).placeholder).toContain('aaaa'); + // Plaintext value should never appear in the DOM. + expect(screen.queryByText(/sk-ant-/i)).not.toBeInTheDocument(); + }); + + it('saves a key and refreshes status', async () => { + const mutate = mockSWR(NONE_STATUS); + mockStore.mockResolvedValue({ + provider: 'LLM_ANTHROPIC', + stored: true, + source: 'stored', + last_four: '1234', + }); + + render(); + const input = screen.getByLabelText(/Anthropic API Key/i); + fireEvent.change(input, { target: { value: 'sk-ant-test-key-1234567890123456' } }); + + const saveButtons = screen.getAllByRole('button', { name: /Save/i }); + await act(async () => { + fireEvent.click(saveButtons[0]); + }); + + expect(mockStore).toHaveBeenCalledWith( + 'LLM_ANTHROPIC', + 'sk-ant-test-key-1234567890123456' + ); + expect(toast.success).toHaveBeenCalledWith( + expect.stringContaining('Anthropic') + ); + expect(mutate).toHaveBeenCalled(); + }); + + it('verifies a key and surfaces the result', async () => { + mockSWR(NONE_STATUS); + mockVerify.mockResolvedValue({ + provider: 'LLM_OPENAI', + valid: true, + message: 'OpenAI key accepted', + }); + + render(); + const input = screen.getByLabelText(/OpenAI API Key/i); + fireEvent.change(input, { target: { value: 'sk-proj-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx' } }); + + // Find the Verify button on the OpenAI slot + const slot = screen.getByText('OpenAI API Key').closest('[data-provider="LLM_OPENAI"]'); + expect(slot).not.toBeNull(); + const verifyButton = slot!.querySelector('button')!; + expect(verifyButton).toHaveTextContent(/Verify/); + + await act(async () => { + fireEvent.click(verifyButton); + }); + + await waitFor(() => { + expect(mockVerify).toHaveBeenCalledWith( + 'LLM_OPENAI', + 'sk-proj-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx' + ); + }); + expect(screen.getByText(/Valid/)).toBeInTheDocument(); + }); + + it('reports invalid verification with a toast and inline message', async () => { + mockSWR(NONE_STATUS); + mockVerify.mockResolvedValue({ + provider: 'GIT_GITHUB', + valid: false, + message: '401 Unauthorized: invalid GitHub token', + }); + + render(); + const input = screen.getByLabelText(/GitHub Personal Access Token/i); + fireEvent.change(input, { target: { value: 'ghp_invalidtoken_xxxxxxxxxxxxxxxxxxxxxx' } }); + + const slot = screen.getByText('GitHub Personal Access Token').closest('[data-provider="GIT_GITHUB"]'); + const verifyButton = slot!.querySelector('button')!; + + await act(async () => { + fireEvent.click(verifyButton); + }); + + await waitFor(() => { + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('401') + ); + }); + expect(screen.getByText(/Invalid/)).toBeInTheDocument(); + }); + + it('removes a stored key and refreshes status', async () => { + const stored: KeyStatusResponse[] = [ + { provider: 'LLM_ANTHROPIC', stored: true, source: 'stored', last_four: 'aaaa' }, + { provider: 'LLM_OPENAI', stored: false, source: 'none', last_four: null }, + { provider: 'GIT_GITHUB', stored: false, source: 'none', last_four: null }, + ]; + const mutate = mockSWR(stored); + mockRemove.mockResolvedValue(undefined); + + render(); + const removeButton = screen.getByRole('button', { name: /Remove/i }); + await act(async () => { + fireEvent.click(removeButton); + }); + + expect(mockRemove).toHaveBeenCalledWith('LLM_ANTHROPIC'); + expect(toast.success).toHaveBeenCalledWith(expect.stringContaining('removed')); + expect(mutate).toHaveBeenCalled(); + }); + + it('disables save and remove for env-source keys', () => { + const stored: KeyStatusResponse[] = [ + { provider: 'LLM_ANTHROPIC', stored: true, source: 'environment', last_four: '5678' }, + { provider: 'LLM_OPENAI', stored: false, source: 'none', last_four: null }, + { provider: 'GIT_GITHUB', stored: false, source: 'none', last_four: null }, + ]; + mockSWR(stored); + render(); + + const slot = screen.getByText('Anthropic API Key').closest('[data-provider="LLM_ANTHROPIC"]')!; + const input = slot.querySelector('input')!; + expect(input).toBeDisabled(); + expect(screen.getByText(/Loaded from environment variable/i)).toBeInTheDocument(); + // No remove button rendered for environment-source. + const removeButton = Array.from(slot.querySelectorAll('button')).find((btn) => + /remove/i.test(btn.textContent ?? '') + ); + expect(removeButton).toBeUndefined(); + }); +}); diff --git a/web-ui/src/app/settings/page.tsx b/web-ui/src/app/settings/page.tsx index 95e828da..2a6adc4e 100644 --- a/web-ui/src/app/settings/page.tsx +++ b/web-ui/src/app/settings/page.tsx @@ -8,6 +8,7 @@ import { toast } from 'sonner'; import { settingsApi } from '@/lib/api'; import { getSelectedWorkspacePath } from '@/lib/workspace-storage'; import type { AgentSettings, AgentTypeKey, ApiError } from '@/types'; +import { ApiKeysTab } from '@/components/settings/ApiKeysTab'; import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; import { @@ -69,25 +70,6 @@ export default function SettingsPage() { if (!workspaceReady) return null; - if (!workspacePath) { - return ( -
-
-

Settings

-
-

- No workspace selected. Use the sidebar to return to{' '} - - Workspace - {' '} - and select a project. -

-
-
-
- ); - } - const updateAgentModel = (agentType: AgentTypeKey, model: string) => { if (!draft) return; setDraft({ @@ -144,7 +126,9 @@ export default function SettingsPage() { Default model per agent type, plus per-task limits.

- {isLoading && !draft ? ( + {!workspacePath ? ( + + ) : isLoading && !draft ? (

Loading…

) : error ? (

@@ -173,7 +157,7 @@ export default function SettingsPage() {

API Keys

- +
@@ -200,6 +184,20 @@ function ComingSoon() { return

Coming soon

; } +function NoWorkspaceMessage() { + return ( +
+

+ No workspace selected. Use the sidebar to return to{' '} + + Workspace + {' '} + and select a project to manage agent settings. +

+
+ ); +} + interface AgentSettingsFormProps { draft: AgentSettings; onModelChange: (agentType: AgentTypeKey, model: string) => void; diff --git a/web-ui/src/components/settings/ApiKeysTab.tsx b/web-ui/src/components/settings/ApiKeysTab.tsx new file mode 100644 index 00000000..23ed0772 --- /dev/null +++ b/web-ui/src/components/settings/ApiKeysTab.tsx @@ -0,0 +1,64 @@ +'use client'; + +import useSWR from 'swr'; + +import { settingsApi } from '@/lib/api'; +import type { KeyProvider, KeyStatusResponse } from '@/types'; +import { KeySlot } from './KeySlot'; + +const PROVIDER_DISPLAY: Array<{ provider: KeyProvider; displayName: string }> = [ + { provider: 'LLM_ANTHROPIC', displayName: 'Anthropic API Key' }, + { provider: 'LLM_OPENAI', displayName: 'OpenAI API Key' }, + { provider: 'GIT_GITHUB', displayName: 'GitHub Personal Access Token' }, +]; + +export function ApiKeysTab() { + const { data, error, isLoading, mutate } = useSWR( + 'settings:keys', + () => settingsApi.getKeys() + ); + + if (isLoading && !data) { + return

Loading…

; + } + if (error || !data) { + return ( +

+ Failed to load API key status. Check the server logs. +

+ ); + } + + const statusByProvider = new Map( + data.map((entry) => [entry.provider, entry]) + ); + + return ( +
+

+ Keys are stored encrypted on this machine. Environment variables take + precedence at runtime. +

+
+ {PROVIDER_DISPLAY.map(({ provider, displayName }) => { + const status = + statusByProvider.get(provider) ?? { + provider, + stored: false, + source: 'none' as const, + last_four: null, + }; + return ( + mutate().then(() => undefined)} + /> + ); + })} +
+
+ ); +} diff --git a/web-ui/src/components/settings/KeySlot.tsx b/web-ui/src/components/settings/KeySlot.tsx new file mode 100644 index 00000000..a33c7241 --- /dev/null +++ b/web-ui/src/components/settings/KeySlot.tsx @@ -0,0 +1,164 @@ +'use client'; + +import { useState } from 'react'; +import { toast } from 'sonner'; + +import { settingsApi } from '@/lib/api'; +import type { ApiError, KeyProvider, KeyStatusResponse } from '@/types'; +import { Badge } from '@/components/ui/badge'; +import { Button } from '@/components/ui/button'; +import { Input } from '@/components/ui/input'; + +interface KeySlotProps { + provider: KeyProvider; + displayName: string; + status: KeyStatusResponse; + onChanged: () => void | Promise; +} + +type VerifyResult = { valid: boolean; message: string } | null; + +export function KeySlot({ + provider, + displayName, + status, + onChanged, +}: KeySlotProps) { + const [value, setValue] = useState(''); + const [verifyResult, setVerifyResult] = useState(null); + const [working, setWorking] = useState(false); + + const handleSave = async () => { + if (!value) return; + setWorking(true); + setVerifyResult(null); + try { + await settingsApi.storeKey(provider, value); + setValue(''); + toast.success(`${displayName} key saved`); + await onChanged(); + } catch (err) { + toast.error((err as ApiError).detail || 'Failed to save key'); + } finally { + setWorking(false); + } + }; + + const handleVerify = async () => { + setWorking(true); + setVerifyResult(null); + try { + const result = await settingsApi.verifyKey(provider, value || undefined); + setVerifyResult({ valid: result.valid, message: result.message }); + if (!result.valid) { + toast.error(result.message); + } + } catch (err) { + const detail = (err as ApiError).detail || 'Verification failed'; + setVerifyResult({ valid: false, message: detail }); + toast.error(detail); + } finally { + setWorking(false); + } + }; + + const handleRemove = async () => { + setWorking(true); + setVerifyResult(null); + try { + await settingsApi.removeKey(provider); + setValue(''); + toast.success(`${displayName} key removed`); + await onChanged(); + } catch (err) { + toast.error((err as ApiError).detail || 'Failed to remove key'); + } finally { + setWorking(false); + } + }; + + const placeholder = status.stored + ? `•••••••• (last 4: ${status.last_four ?? '????'})` + : `Enter ${displayName} key`; + + const sourceLabel = + status.source === 'environment' + ? 'env var' + : status.source === 'stored' + ? 'stored' + : 'not set'; + + const sourceVariant: 'default' | 'secondary' | 'outline' = + status.source === 'environment' + ? 'secondary' + : status.source === 'stored' + ? 'default' + : 'outline'; + + // Env-source keys are read-only — env vars take precedence at runtime, + // so storing/removing wouldn't change the effective key. + const fromEnv = status.source === 'environment'; + + return ( +
+
+

{displayName}

+ {sourceLabel} +
+ +
+ setValue(e.target.value)} + placeholder={placeholder} + disabled={working || fromEnv} + autoComplete="off" + spellCheck={false} + className="min-w-0 flex-1" + aria-label={displayName} + /> + + {value && ( + + )} + {status.stored && status.source === 'stored' && ( + + )} +
+ + {fromEnv && ( +

+ Loaded from environment variable. Unset the env var to manage this key here. +

+ )} + + {verifyResult && ( +

+ {verifyResult.valid ? '✓ Valid' : '✗ Invalid'} — {verifyResult.message} +

+ )} +
+ ); +} diff --git a/web-ui/src/lib/api.ts b/web-ui/src/lib/api.ts index 5ceba938..6d91e0f4 100644 --- a/web-ui/src/lib/api.ts +++ b/web-ui/src/lib/api.ts @@ -60,6 +60,9 @@ import type { SessionCreateRequest, ChatMessage, AgentSettings, + KeyProvider, + KeyStatusResponse, + VerifyKeyResponse, } from '@/types'; // FastAPI validation error format @@ -821,6 +824,38 @@ export const settingsApi = { }); return response.data; }, + + // API key management (issue #555). Machine-wide — no workspace_path needed. + getKeys: async (): Promise => { + const response = await api.get('/api/v2/settings/keys'); + return response.data; + }, + + storeKey: async ( + provider: KeyProvider, + value: string + ): Promise => { + const response = await api.put( + `/api/v2/settings/keys/${provider}`, + { value } + ); + return response.data; + }, + + removeKey: async (provider: KeyProvider): Promise => { + await api.delete(`/api/v2/settings/keys/${provider}`); + }, + + verifyKey: async ( + provider: KeyProvider, + value?: string + ): Promise => { + const response = await api.post( + '/api/v2/settings/verify-key', + { provider, value: value ?? null } + ); + return response.data; + }, }; export default api; diff --git a/web-ui/src/types/index.ts b/web-ui/src/types/index.ts index 75885667..7fb6888f 100644 --- a/web-ui/src/types/index.ts +++ b/web-ui/src/types/index.ts @@ -574,3 +574,21 @@ export interface AgentSettings { max_turns: number; max_cost_usd: number | null; } + +// API key types — mirrors codeframe/ui/models.py KeyProvider et al. +export type KeyProvider = 'LLM_ANTHROPIC' | 'LLM_OPENAI' | 'GIT_GITHUB'; + +export type KeySource = 'environment' | 'stored' | 'none'; + +export interface KeyStatusResponse { + provider: KeyProvider; + stored: boolean; + source: KeySource; + last_four: string | null; +} + +export interface VerifyKeyResponse { + provider: KeyProvider; + valid: boolean; + message: string; +}