diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index 4044d0d..2c679f2 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -902,17 +902,55 @@ class TestServiceTestEndpoint: assert before == after def test_secrets_not_logged(self, test_client: TestClient, caplog) -> None: - """No log line contains the secret value.""" - secret_value = "super-secret-hunter2" - with caplog.at_level(logging.INFO): - test_client.post( + """Secret values in the request body never reach any log line. + + Unlike the trivial empty-secrets case, this drives the full endpoint + path (validate -> dispatch to the real test_callable -> success log) + with real-looking secret payloads. The per-type test_callables are + mocked at the network boundary so they succeed, proving the endpoint + does not log the secret values even though they are in the request body. + """ + api_key_secret = "glc_somethingverysecret" + password_secret = "SUPER-SECRET-PW-12345" + + prom_response = SimpleNamespace(raise_for_status=lambda: None, json=lambda: {"results": {}}) + qbit_client = MagicMock() + qbit_client.maindata.return_value = {"server_state": {"qbittorrent_version": "v4.6.0"}} + + with ( + caplog.at_level(logging.DEBUG), + patch("media_library_viewer_api.integrations.prometheus.requests.post", return_value=prom_response), + patch("media_library_viewer_api.integrations.qbittorrent.QbittorrentClient", return_value=qbit_client), + ): + prom_resp = test_client.post( "/api/services/test", json={ - "service_type": "backups", + "service_type": "prometheus", "name": "test", - "config": {"ingestion_label": "default"}, - "secrets": {}, + "config": {"grafana_url": "http://grafana:3000"}, + "secrets": {"grafana_api_key": api_key_secret}, "enabled": True, }, ) - assert secret_value not in caplog.text + qbit_resp = test_client.post( + "/api/services/test", + json={ + "service_type": "qbittorrent", + "name": "test", + "config": {"base_url": "http://qb:8080"}, + "secrets": {"username": "u", "password": password_secret}, + "enabled": True, + }, + ) + + # Both requests must run the endpoint fully (validate + dispatch + success). + assert prom_resp.status_code == 200 + assert prom_resp.json()["ok"] is True + assert qbit_resp.status_code == 200 + assert qbit_resp.json()["ok"] is True + + # Neither the full secret values nor meaningful fragments may leak into logs. + leaked = [ + fragment for fragment in (api_key_secret, password_secret, "verysecret", "SUPER") if fragment in caplog.text + ] + assert not leaked, f"secret fragments leaked into logs: {leaked!r}" diff --git a/openspec/changes/service-credential-tester/apply-progress.md b/openspec/changes/service-credential-tester/apply-progress.md new file mode 100644 index 0000000..f0c0283 --- /dev/null +++ b/openspec/changes/service-credential-tester/apply-progress.md @@ -0,0 +1,53 @@ +# Apply Progress: Service Credential Tester + +**Change:** `service-credential-tester` +**Phase:** apply-progress +**Date:** 2026-07-09 +**Status:** complete — all 29 tasks done, all gates green, verified (see `verify-report.md`) + +## Slices delivered + +Two slices, each its own commit, each leaving `pytest` / `npm run build` / `npm run lint` / `ruff` green. + +### Slice 1 — Backend test endpoint + per-type routines (commit `3391fbc`) + +- `integrations/base.py` — added frozen `TestResult` dataclass (`ok`, `detail`, `evidence`) + optional `test_callable` field on `ServiceDefinition` (placed last for dataclass ordering). Signature: `(store, config, secrets) -> TestResult` (CT-101). +- Shared error-translation helper `translate_connection_error` — extracts the `test_machine_ssh` patterns (ConnectionError/Timeout/SSL/HTTP 401-403/5xx → friendly strings) (CT-112). +- Per-type `test_connection` routines alongside each `DEFINITION`: + - **qbittorrent** — `QbittorrentClient` login + `maindata()`; `"Fails."` → "Authentication failed (qBittorrent rejected credentials)" — resolves the #3 log-only pain at the API layer (CT-104). + - **prometheus** — `POST {grafana_url}/api/ds/query` with `grafana_api_key` + `datasource_uid`, `expr "up"`; evidence "Gateway reachable" (CT-105; gateway path per `grafana-metric-gateway`). + - **alertmanager** — `GET /api/v2/alerts` (+ optional bearer); evidence cluster version (CT-106). + - **jellyfin** — `JellyfinClient.users()`; evidence " users" (CT-107). + - **authentik** — directory endpoint GET; evidence slug/"connected" (CT-108). + - **ssh_tasks** — reuses `build_ssh_client(store, service)` + `connect()`; banner/auth translation; no duplication of `test_machine_ssh` internals (CT-109). + - **nextcloud** — `GET /status.php` (unauth); evidence version (CT-110). + - **backups** — `test_callable=None` → `{ok: True, detail: "No test needed"}` (CT-111). +- `routers/services.py` — `POST /api/services/test`: validation-first (422 on malformed config, no network call), dispatch, **zero persistence** (no `upsert`/`update_setting`), INFO log only type+ok (sanitized; no secrets) (CT-102, CT-103, CT-113). +- Tests: `test_credential_tester.py` (per-routine success + failure, dispatch, validation-before-test, **no-persistence assertion** `test_no_persistence_after_test`, qBit "Fails." → auth message). + +### Slice 2 — Frontend Test button + gating (commit `9972514`, amended) + +- `types/index.ts` — `TestResult` interface (CT-113). +- `api/services.ts` + `hooks/useServices.ts` — `testServiceInstance` + `useTestServiceInstance` mutation (no cache invalidation; test is side-effect-free). +- NEW `components/ServiceTestPanel.tsx` — **presentational** shared component (cleaner than the design's stateful version — deviation N-6). Props: `{ input, onResult, disabled }`. Renders Test button + `Testing…` state + result pill (✓ green evidence / ✗ red detail) + Save-anyway checkbox (only on failure). Parent owns `testResult` + `saveAnyway` state; store-previous pattern clears on input change (CT-114, CT-115, CT-116, CT-117). +- Wired into BOTH surfaces: `CreateServiceDialog` (`ServicesPage.tsx`) AND `ServiceConfigEditor` (`Settings.tsx` — the correct edit surface per design source-finding, not `ServicePage.tsx`). Create/Save confirm gated on `testPassed || saveAnyway` (CT-118). +- Tests: 7 `ServiceTestPanel.test.tsx` cases (button states, success/failure pills, checkbox toggle). + +## Deviations from tasks.md / design + +- **N-6 (intentional improvement):** `ServiceTestPanel` is presentational; the design's stateful version was simplified. Parent owns result + saveAnyway state. Cleaner; works identically in both surfaces. +- **Edit surface correction:** spec CT-115 said `ServicePage.tsx`; the actual edit dialog is `Settings.tsx::ServiceConfigEditor` (design source-finding). Tasks targeted the right file. + +## Final gate results + +| Gate | Result | +|---|---| +| `backend && PYTHONPATH=src python3 -m pytest -q` | **362 passed** (+31 new), 2 warnings (pre-existing pythonjsonlogger) | +| `backend && PYTHONPATH=src python3 -m ruff check src tests` | **All checks passed** | +| `frontend && npm run build` | **exit 0** (pre-existing chunk-size warning) | +| `frontend && npm run lint` | **0 errors**, 1 pre-existing warning (`WidgetConfigDialog.tsx`, untouched) | +| `frontend && npx vitest run` | **158 passed** (+7 ServiceTestPanel) | + +## Verification + +See `verify-report.md` — adversarial fresh-context review: **21/21 PASS**, all gates green. No blocking code findings. Archive blocker is doc-only (this file + ticked tasks clear it). Non-blocking notes: N-2 (no-secret-logs test sends empty secrets — weak coverage, not a defect), N-4 (edit flow requires re-typing secrets — inherent to no-persistence), N-5 (gating proven by source, not page-level test). diff --git a/openspec/changes/service-credential-tester/tasks.md b/openspec/changes/service-credential-tester/tasks.md index 3a8a3ca..fdc750a 100644 --- a/openspec/changes/service-credential-tester/tasks.md +++ b/openspec/changes/service-credential-tester/tasks.md @@ -63,79 +63,79 @@ No proposal/spec scope change is required — the intent (per-type credential te **Satisfies:** CT-101, CT-102, CT-103, CT-104, CT-105, CT-106, CT-107, CT-108, CT-109, CT-110, CT-111, CT-112, CT-113, CT-119. -- [ ] **1.1 Add `TestResult` dataclass + `TestCallable` type alias + `test_callable` field on `ServiceDefinition` (CT-101, CT-103)** +- [x] **1.1 Add `TestResult` dataclass + `TestCallable` type alias + `test_callable` field on `ServiceDefinition` (CT-101, CT-103)** - Files: `backend/src/media_library_viewer_api/integrations/base.py` (modify) - Lines: ~30 - Dependencies: none - Details: Add `@dataclass(frozen=True) class TestResult` with fields `ok: bool`, `detail: str`, `evidence: str | None = None`. Add a type alias `TestCallable = Callable[[dict[str, Any], dict[str, str], "SettingsStore"], TestResult]` (forward-reference SettingsStore to avoid circular import at module level; import it inside the test routines or use a string annotation). Add `test_callable: TestCallable | None = None` to `ServiceDefinition` — placed LAST (after `widget_kinds`) so the dataclass field-ordering with a default works. Import `Callable` from `typing`. Update `TYPE_CHECKING` guard if needed for the SettingsStore forward reference. -- [ ] **1.2 Add `translate_connection_error` shared helper (CT-104..CT-110)** +- [x] **1.2 Add `translate_connection_error` shared helper (CT-104..CT-110)** - Files: `backend/src/media_library_viewer_api/integrations/base.py` (modify, same file) - Lines: ~40 - Dependencies: 1.1 - Details: Add `def translate_connection_error(exc: Exception, *, context: str = "") -> TestResult:` that maps common exception types to human-friendly messages. Handle: `requests.HTTPError` with status 401/403 → auth message; `requests.ConnectionError` / `ConnectionRefusedError` / `OSError` with DNS keywords (`getaddrinfo`, `name or service not known`) → "Host not found"; generic connection error → "Connection refused"; SSL/certificate keywords → "SSL/TLS error"; `requests.Timeout` / `TimeoutError` / `asyncio.TimeoutError` → "Connection timed out"; SSH "protocol banner" → "SSH banner not received". Fallback: truncate message to 200 chars, prefix with context. The helper returns `TestResult(ok=False, detail=...)` always. Import `requests` and `asyncio` at the top of `base.py` (or inside the function to avoid circular imports — prefer top-level since `base.py` already may need `requests` for the type checks). -- [ ] **1.3 Add `test_connection` for qbittorrent (CT-104)** +- [x] **1.3 Add `test_connection` for qbittorrent (CT-104)** - Files: `backend/src/media_library_viewer_api/integrations/qbittorrent.py` (modify) - Lines: ~25 - Dependencies: 1.1, 1.2 - Details: Add `def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:` that constructs `QbittorrentClient(config["base_url"], secrets["username"], secrets["password"], timeout=config.get("timeout_seconds", 10))`, calls `client.maindata()`, extracts `server_state` version (fallback `"connected"`), returns `TestResult(ok=True, detail="Connected to qBittorrent.", evidence=version)`. Catch `RuntimeError` with "login failed" in the message → `TestResult(ok=False, detail="Authentication failed — qBittorrent rejected the credentials.")`. All other exceptions → `translate_connection_error(exc, context="qBittorrent")`. Import `QbittorrentClient` from `clients.qbittorrent`, `TestResult` + `translate_connection_error` from `integrations.base`. Wire `test_callable=test_connection` into the `DEFINITION`. -- [ ] **1.4 Add `test_connection` for prometheus via Grafana gateway (CT-105)** +- [x] **1.4 Add `test_connection` for prometheus via Grafana gateway (CT-105)** - Files: `backend/src/media_library_viewer_api/integrations/prometheus.py` (modify) - Lines: ~30 - Dependencies: 1.1, 1.2 - Details: Add `def test_connection(config, secrets, store) -> TestResult:` that issues `POST {grafana_url}/api/ds/query` with `Authorization: Bearer {grafana_api_key}`, body `{"queries": [{"datasource": {"uid": datasource_uid, "type": "prometheus"}, "expr": "up", "format": "time_series", "intervalMs": 15000, "maxDataPoints": 1, "refId": "A"}], "from": "now-1m", "to": "now"}`. Extract `grafana_url`, `datasource_uid`, `timeout_seconds` from config; `grafana_api_key` from secrets. Missing URL/key → `TestResult(ok=False, detail="...")` (no network). Success → `TestResult(ok=True, detail="Grafana gateway reachable.", evidence="Gateway reachable; datasource responded.")`. Errors → `translate_connection_error(exc, context="Prometheus via Grafana")`. Mirror `MetricSource._gateway_query` request shape exactly. Wire `test_callable=test_connection` into the `DEFINITION`. -- [ ] **1.5 Add `test_connection` for alertmanager (CT-106)** +- [x] **1.5 Add `test_connection` for alertmanager (CT-106)** - Files: `backend/src/media_library_viewer_api/integrations/alertmanager.py` (modify) - Lines: ~20 - Dependencies: 1.1, 1.2 - Details: Add `def test_connection(config, secrets, store) -> TestResult:` that GETs `{base_url}/api/v2/status` with optional `Authorization: Bearer {api_key}` (when secret present). Extract version from `versionInfo.version` (fallback `"connected"`). Success → `TestResult(ok=True, detail="Connected to Alertmanager.", evidence=version)`. Errors → `translate_connection_error(exc, context="Alertmanager")`. Wire `test_callable=test_connection` into the `DEFINITION`. -- [ ] **1.6 Add `test_connection` for jellyfin (CT-107)** +- [x] **1.6 Add `test_connection` for jellyfin (CT-107)** - Files: `backend/src/media_library_viewer_api/integrations/jellyfin.py` (modify) - Lines: ~20 - Dependencies: 1.1, 1.2 - Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs `JellyfinClient(config["base_url"], secrets["api_key"], timeout=config.get("timeout_seconds", 10))` and calls `.users()`. Success → `TestResult(ok=True, detail="Connected to Jellyfin.", evidence=f"{len(users)} users")`. Errors → `translate_connection_error(exc, context="Jellyfin")`. Import `JellyfinClient` from `clients.jellyfin`. Wire `test_callable=test_connection` into the `DEFINITION`. -- [ ] **1.7 Add `test_connection` for authentik (CT-108)** +- [x] **1.7 Add `test_connection` for authentik (CT-108)** - Files: `backend/src/media_library_viewer_api/integrations/authentik.py` (modify) - Lines: ~20 - Dependencies: 1.1, 1.2 - Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs `AuthentikClient(base_url=config["base_url"], api_token=secrets["api_token"], timeout=config.get("timeout_seconds", 10))` and calls `.users(page=1, page_size=1)` — the lightest directory probe. Success → `TestResult(ok=True, detail="Connected to Authentik.", evidence=...)`. Errors → `translate_connection_error(exc, context="Authentik")`. Import `AuthentikClient` from its module (check `routers/authentik_users.py::_build_client` for the exact import path + constructor signature). Wire `test_callable=test_connection` into the `DEFINITION`. -- [ ] **1.8 Add `test_connection` for ssh_tasks reusing `build_ssh_client` (CT-109)** +- [x] **1.8 Add `test_connection` for ssh_tasks reusing `build_ssh_client` (CT-109)** - Files: `backend/src/media_library_viewer_api/integrations/ssh_tasks.py` (modify) - Lines: ~30 - Dependencies: 1.1, 1.2 - Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs a `ServiceRecord(id="", service_type="ssh_tasks", name="test", config=config, secrets=secrets, enabled=True)`, calls `build_ssh_client(store, service)` (from `services.task_runner`), then `.connect()` inside try/finally with `.close()`. Translate errors inline for SSH-specific patterns: "protocol banner" → `"SSH banner not received from {host}:{port}; confirm the SSH service is running."`; "no authentication methods available" / "authentication failed" → `"SSH authentication failed for {host}:{port}; check the SSH key, passphrase, or username."`. Other exceptions → `translate_connection_error(exc, context=f"SSH {host}:{port}")`. Success → `TestResult(ok=True, detail=f"SSH connection succeeded for {host}:{port}.", evidence=f"Connected to {host}:{port}")`. Import `build_ssh_client` from `media_library_viewer_api.services.task_runner` and `ServiceRecord` from `media_library_viewer_api.widgets.sources` (or define locally to avoid circular import — check). **Do NOT import or call `test_machine_ssh`** — it's a router endpoint, not a reusable function. Known-host recording is preserved automatically by `RemoteSSHClient.connect()`. Wire `test_callable=test_connection` into the `DEFINITION`. -- [ ] **1.9 Add `test_connection` for nextcloud (CT-110)** +- [x] **1.9 Add `test_connection` for nextcloud (CT-110)** - Files: `backend/src/media_library_viewer_api/integrations/nextcloud.py` (modify) - Lines: ~15 - Dependencies: 1.1, 1.2 - Details: Add `def test_connection(config, secrets, store) -> TestResult:` that GETs `{base_url}/status.php` (unauthenticated — `/status.php` is public). Extract `version` from the JSON response (fallback `"connected"`). Success → `TestResult(ok=True, detail="Connected to Nextcloud.", evidence=version)`. Errors → `translate_connection_error(exc, context="Nextcloud")`. Wire `test_callable=test_connection` into the `DEFINITION`. -- [ ] **1.10 Confirm backups DEFINITION has `test_callable = None` (CT-111)** +- [x] **1.10 Confirm backups DEFINITION has `test_callable = None` (CT-111)** - Files: `backend/src/media_library_viewer_api/integrations/backups.py` (verify, modify only if the field isn't defaulted) - Lines: ~0 (default applies) - Dependencies: 1.1 - Details: `backups` should NOT gain a `test_connection` function. Its `DEFINITION` relies on the default `test_callable=None` from `ServiceDefinition`. Verify by reading the file; no code change expected unless the DEFINITION is constructed with explicit keyword args that omit `test_callable` (the default handles it). The endpoint returns `{ok: true, detail: "No connection test for this service type"}` automatically. -- [ ] **1.11 Add `POST /api/services/test` endpoint (CT-101, CT-102, CT-103, CT-112, CT-113)** +- [x] **1.11 Add `POST /api/services/test` endpoint (CT-101, CT-102, CT-103, CT-112, CT-113)** - Files: `backend/src/media_library_viewer_api/routers/services.py` (modify) - Lines: ~30 - Dependencies: 1.1–1.10 - Details: Add `@router.post("/test") def test_instance(body: ServiceInstanceInput, store: SettingsStore = Depends(get_settings_store)) -> dict[str, Any]:` that: (a) calls `_validate_input(body)` — reuses the existing helper, raises HTTPException(422) on malformed config/type/secrets BEFORE any network call (CT-102); (b) resolves `definition = require_service_definition(body.service_type)`; (c) if `definition.test_callable is None` → log `test requested type=%s ok=true` and return `{"ok": True, "detail": "No connection test for this service type", "evidence": None}` (CT-103/111); (d) else call `result = definition.test_callable(body.config, body.secrets, store)` inside a try/except (defensive — routines catch internally, but if one raises, return `TestResult(ok=False, detail=f"Test failed unexpectedly: {exc}")`); (e) log `test requested type=%s ok=%s` at INFO — NEVER log body/config/secrets (CT-113); (f) return `{"ok": result.ok, "detail": result.detail, "evidence": result.evidence}`. NO call to `store.upsert_service`, `store.update_setting`, or any persistence method (CT-112). Import `require_service_definition` from `integrations.registry` (already imported or adjacent). -- [ ] **1.12 Add backend tests for `translate_connection_error` (CT-119)** +- [x] **1.12 Add backend tests for `translate_connection_error` (CT-119)** - Files: `backend/tests/test_services.py` (modify) or a new `backend/tests/test_credential_tester.py` (create) - Lines: ~40 - Dependencies: 1.2 - Details: Test the shared helper directly: (a) `requests.HTTPError` with a mock response `.status_code = 401` → auth message; (b) `requests.ConnectionError("getaddrinfo failed")` → "Host not found"; (c) `requests.Timeout()` → "timed out"; (d) generic `ValueError("something")` → truncated fallback with context prefix. Assert `ok=False` and the detail string contains expected keywords. -- [ ] **1.13 Add backend tests for per-type `test_connection` routines (CT-119)** +- [x] **1.13 Add backend tests for per-type `test_connection` routines (CT-119)** - Files: `backend/tests/test_services.py` (modify) or `backend/tests/test_credential_tester.py` (create/modify) - Lines: ~100 - Dependencies: 1.3–1.9 @@ -149,13 +149,13 @@ No proposal/spec scope change is required — the intent (per-type credential te - **nextcloud**: success returns version; 404 → connection error. - Use `unittest.mock.patch` to mock at the right boundary (the client constructor or `requests.get/post`). -- [ ] **1.14 Add backend tests for the endpoint: dispatch, validation-first, no-persistence (CT-119)** +- [x] **1.14 Add backend tests for the endpoint: dispatch, validation-first, no-persistence (CT-119)** - Files: `backend/tests/test_api.py` (modify) or `backend/tests/test_credential_tester.py` (create/modify) - Lines: ~50 - Dependencies: 1.11 - Details: (a) DISPATCH: `POST /api/services/test` with a `backups` body → `{ok: True, detail: "No connection test..."}` and no network call. (b) VALIDATION-FIRST: body with schema-less `base_url` for qbittorrent → HTTP 422 (not 200); assert the test_callable was NOT called (mock it, assert call count 0). (c) NO-PERSISTENCE: call `/test` with a valid qbittorrent body (test_callable mocked to return ok), assert `store.list_services()` count is unchanged before/after. (d) NO-SECRET-LOGS: use `caplog` at INFO level, call `/test` with `secrets: {"password": "hunter2"}`, assert no log line contains "hunter2". -- [ ] **1.15 Verify Slice 1 (pytest + ruff + frontend still builds)** +- [x] **1.15 Verify Slice 1 (pytest + ruff + frontend still builds)** - Run: `cd backend && PYTHONPATH=src python3 -m pytest -q && PYTHONPATH=src python3 -m ruff check src tests` - Run: `cd frontend && npm run build && npm run lint` - Verify: all backend tests pass (per-type routines, endpoint dispatch, validation, no-persistence, no-secret-logs); ruff clean; frontend still builds + lints (no frontend change in S1, so this is a regression check only). @@ -171,56 +171,56 @@ No proposal/spec scope change is required — the intent (per-type credential te **Satisfies:** CT-114, CT-115, CT-116, CT-117, CT-118, CT-120, CT-121. -- [ ] **2.1 Add `ServiceTestResult` type (CT-114)** +- [x] **2.1 Add `ServiceTestResult` type (CT-114)** - Files: `frontend/src/types/index.ts` (modify) - Lines: ~5 - Dependencies: none - Details: Add `export interface ServiceTestResult { ok: boolean; detail: string; evidence: string | null; }`. -- [ ] **2.2 Add `testServiceInstance` API client function (CT-114)** +- [x] **2.2 Add `testServiceInstance` API client function (CT-114)** - Files: `frontend/src/api/services.ts` (modify) - Lines: ~5 - Dependencies: 2.1 - Details: Add `export async function testServiceInstance(input: ServiceInstanceInput): Promise { return post("/api/services/test", input); }`. Import `ServiceTestResult` from `../types`. The existing `post` helper (from `./shared`) handles auth headers + error extraction. -- [ ] **2.3 Add `useTestServiceInstance` hook (CT-114)** +- [x] **2.3 Add `useTestServiceInstance` hook (CT-114)** - Files: `frontend/src/hooks/useServices.ts` (modify) - Lines: ~8 - Dependencies: 2.2 - Details: Add `export function useTestServiceInstance() { return useMutation({ mutationFn: (input: ServiceInstanceInput) => testServiceInstance(input) }); }`. Import `useMutation` from `@tanstack/react-query` (already imported in the file). Import `testServiceInstance` from `../api/services`. No cache invalidation needed — the test is a one-shot mutation. -- [ ] **2.4 Create shared `ServiceTestPanel` component (CT-115, CT-116, CT-117, CT-118)** +- [x] **2.4 Create shared `ServiceTestPanel` component (CT-115, CT-116, CT-117, CT-118)** - Files: `frontend/src/components/ServiceTestPanel.tsx` (NEW) - Lines: ~70 - Dependencies: 2.3 - Details: Create a component with props `{ input: ServiceInstanceInput | null; onTestResult: (passed: boolean) => void; }`. Internal state: `result: ServiceTestResult | null`, `saveAnyway: boolean`. `useEffect([input])` clears `result` and calls `onTestResult(false)` whenever `input` changes (new object reference on every field edit — CT-118). A "Test credentials" `Button` (variant outline, size sm) fires `testService.mutateAsync(input)`; while pending shows "Testing…" and is disabled. On result: render an `Alert` — green/default variant with `✓ Connected — {evidence}` on success, destructive variant with `✗ {detail}` on failure. A "Save anyway (skip test)" checkbox toggles `saveAnyway`; when checked, calls `onTestResult(true)` regardless of test outcome (CT-117). The parent reads the gating signal via the `onTestResult` callback. Import `useTestServiceInstance`, `Alert`/`AlertDescription` from `@/components/ui/alert`, `Button` from `@/components/ui/button`. -- [ ] **2.5 Wire `ServiceTestPanel` into `CreateServiceDialog` (CT-115, CT-117)** +- [x] **2.5 Wire `ServiceTestPanel` into `CreateServiceDialog` (CT-115, CT-117)** - Files: `frontend/src/pages/ServicesPage.tsx` (modify) - Lines: ~20 - Dependencies: 2.4 - Details: In `CreateServiceDialog`: add `const [testPassed, setTestPassed] = useState(false)`. Reset it in `reset()`. Build `testInput` from the current draft (null if no draft). Render `` below the config/secret fields and above the footer. Update `DialogFooter`'s `confirmDisabled` to include `!testPassed` (i.e. `confirmDisabled={!draft.name.trim() || saveService.isPending || !testPassed}`). Import `ServiceTestPanel` from `../components/ServiceTestPanel`. -- [ ] **2.6 Wire `ServiceTestPanel` into `ServiceConfigEditor` in `Settings.tsx` (CT-115, CT-117)** +- [x] **2.6 Wire `ServiceTestPanel` into `ServiceConfigEditor` in `Settings.tsx` (CT-115, CT-117)** - Files: `frontend/src/pages/Settings.tsx` (modify) - Lines: ~20 - Dependencies: 2.4 - Details: In `ServiceConfigEditor` (NOT `ServicePage.tsx` — per design source finding §0, service editing lives in `Settings.tsx`): add `const [testPassed, setTestPassed] = useState(false)`. Build `testInput` from the editor's current draft state (name, config, secrets, enabled). Render `` below the form fields. Gate the Save button on `!testPassed` (disabled until test passes or Save anyway is checked). Reset `testPassed` when switching instances (if the editor has an instance-switch effect). Import `ServiceTestPanel`. - **Risk flag:** the spec CT-115 says "ServicePage.tsx" — that is textual drift. The actual edit surface is `Settings.tsx::ServiceConfigEditor`. This task targets the correct file. -- [ ] **2.7 Add frontend tests for `ServiceTestPanel` (CT-120)** +- [x] **2.7 Add frontend tests for `ServiceTestPanel` (CT-120)** - Files: `frontend/src/components/__tests__/ServiceTestPanel.test.tsx` (NEW) - Lines: ~80 - Dependencies: 2.4 - Details: Test cases: (a) renders Test button; (b) click fires the mocked `useTestServiceInstance` mutation; (c) success result → green `✓ Connected` pill with evidence text; (d) failure result → red `✗` pill with detail text; (e) "Save anyway" checkbox checked → `onTestResult(true)` called regardless of test state; (f) editing `input` (passing a new object) clears the result and calls `onTestResult(false)`. Mock `useTestServiceInstance` via `vi.mock("../../hooks/useServices", ...)`. Use `@testing-library/react` + `@testing-library/user-event`. -- [ ] **2.8 Update existing dialog tests for gating (CT-120)** +- [x] **2.8 Update existing dialog tests for gating (CT-120)** - Files: `frontend/src/pages/__tests__/ServicesPage.test.tsx` (modify, if it exists) or confirm coverage via the panel test - Lines: ~20 - Dependencies: 2.5 - Details: If `ServicesPage.test.tsx` tests the create dialog, add assertions that the Create button is disabled until the test passes. Mock the test mutation to return `{ok: true}` and verify the button enables. If there's no existing ServicesPage test covering the dialog, the panel test (2.7) covers the gating behavior adequately — note the coverage decision. -- [ ] **2.9 Verify Slice 2 (build + lint + test)** +- [x] **2.9 Verify Slice 2 (build + lint + test)** - Run: `cd frontend && npm run build && npm run lint && npx vitest run` - Run: `cd backend && PYTHONPATH=src python3 -m pytest -q` (regression: Slice 1 tests still pass) - Verify: frontend typechecks + builds; lint 0 errors; vitest passes (new panel test + existing tests); backend still green. @@ -232,21 +232,21 @@ No proposal/spec scope change is required — the intent (per-type credential te ## Integration verification (post-slice) -- [ ] **3.1 Full backend test run** +- [x] **3.1 Full backend test run** - Run: `cd backend && PYTHONPATH=src python3 -m pytest -q` - Verify: all tests pass (per-type routines, endpoint, validation, no-persistence, no-secret-logs). -- [ ] **3.2 Full frontend build + lint + test** +- [x] **3.2 Full frontend build + lint + test** - Run: `cd frontend && npm run build && npm run lint && npx vitest run` - Verify: 0 errors; panel test covers button/pill/gating/field-clear. -- [ ] **3.3 End-to-end dispatch check** +- [x] **3.3 End-to-end dispatch check** - Verify (by reading source or running a manual API call): `POST /api/services/test` with a `backups` body returns `{ok: true, detail: "No connection test for this service type"}`; with a `qbittorrent` body (mocked client) dispatches to the qbittorrent routine; with an unknown type returns 422. -- [ ] **3.4 No-persistence check** +- [x] **3.4 No-persistence check** - Verify (by test in 1.14): calling `/test` does not create a service row. -- [ ] **3.5 No-secret-logs check** +- [x] **3.5 No-secret-logs check** - Verify (by test in 1.14): no log line contains a secret value. --- diff --git a/openspec/changes/service-credential-tester/verify-report.md b/openspec/changes/service-credential-tester/verify-report.md new file mode 100644 index 0000000..405b76b --- /dev/null +++ b/openspec/changes/service-credential-tester/verify-report.md @@ -0,0 +1,259 @@ +# Verify Report — service-credential-tester + +> Phase: **verify** · Change: `service-credential-tester` · Repo: `/home/user/manage` +> FRESH-CONTEXT adversarial read-only verification of the change against +> `proposal.md`, `spec.md`, `design.md`, and `tasks.md`. **No source edits.** +> This verify report is the only file written. + +**Head commit verified:** `f6c67bd` (`feat(service-credential-tester): slice 2 — Test button + gating (shared ServiceTestPanel)`). + +Two implementation slices are committed underneath it: + +- `3391fbc` slice 1 — backend test endpoint + per-type routines +- `f6c67bd` slice 2 — Test button + gating (shared `ServiceTestPanel`) *(the task brief cited + `9972514` for slice 2; the actual landed commit is `f6c67bd`. Content matches the spec/design; + informational, not a defect.)* + +There is one uncommitted working-tree change: `frontend/src/pages/ServicesPage.tsx` — purely a +cosmetic JSX reflow (indentation/prettier), no functional diff. See finding N-1. + +--- + +## 0. Executive summary / verdict + +**VERDICT: PASS (functionally) — every requirement CT-101 … CT-121 is met in source and all +gates are green. ARCHIVE IS BLOCKED on a task-hygiene / missing-`apply-progress` issue +(reconcilable without code changes).** + +The credential tester is implemented end-to-end: `POST /api/services/test` validates first +(reusing `_validate_input`), dispatches through the closed `test_callable` registry, returns +`{ok, detail, evidence}`, persists nothing, and logs no secrets. All 7 remote types have a +`test_connection` routine wired into their `DEFINITION`; `backups` correctly has +`test_callable = None`. The shared `ServiceTestPanel` (Test button + result pill + Save-anyway +override) is wired into **both** `CreateServiceDialog` (ServicesPage.tsx) **and** +`ServiceConfigEditor` (Settings.tsx — the correct edit surface per the design source-finding, +**not** the read-only `ServicePage.tsx`). Create/Save are gated on `testPassed`, with a +documented store-previous pattern that clears the result on any connectivity-field edit. + +All five gates are green: backend `pytest` (362 passed), `ruff` (clean), frontend `npm run build`, +`npm run lint` (0 errors; 1 pre-existing unrelated warning), and `npx vitest run` (45 files / 158 +tests). + +**Blocking issues** are purely lifecycle: `apply-progress.md` is absent and **all 29 tasks in +`tasks.md` remain unchecked (`- [ ]`)** — the apply phase never reconciled the checklist. The +implementation is proven complete by source reading + passing gates, so this is stale-checkbox +reconciliation, but the contract requires no unchecked implementation tasks and a present +`apply-progress.md` before archive. + +**Per the strict verify contract, this cannot return a clean archive-ready PASS while unchecked +implementation tasks and the missing apply-progress persist.** Functional verification is PASS; +archive readiness is BLOCKED. + +--- + +## 1. Per-requirement verdict table (CT-101 … CT-121) + +| Req | Description | Verdict | Evidence | +|-----|-------------|---------|----------| +| CT-101 | `POST /api/services/test` accepts `ServiceInstanceInput`, returns `{ok,detail,evidence}`, auth-gated | ✅ PASS | `routers/services.py:189` `test_instance`; returns 200 dict; auth-gated via app middleware (`path.startswith("/api")`, not in `EXEMPT_PATHS`) | +| CT-102 | Validation before test → 422 on malformed config | ✅ PASS | `_validate_input(body)` called first (`services.py:196`); `test_validation_first_rejects_malformed_config` asserts 422 for schema-less `base_url` | +| CT-103 | Closed per-type dispatch via `test_callable`; `None` → default ok | ✅ PASS | `base.py:125` field `test_callable: TestCallable \| None = None`; `None` branch returns `{ok:true, "No connection test…"}` | +| CT-104 | qBittorrent: login + maindata; `"Fails."` → auth message | ✅ PASS | `qbittorrent.py:test_connection`; `RuntimeError("login failed")` → `"Authentication failed — qBittorrent rejected the credentials."`; `test_login_failed_translates_to_auth_message` confirms | +| CT-105 | Prometheus via Grafana gateway `POST /api/ds/query`, `expr:"up"` | ✅ PASS | `prometheus.py:test_connection` POSTs `{grafana_url}/api/ds/query` with `Bearer` + `queries[0].expr="up"`; **no direct Prom call** | +| CT-106 | Alertmanager probes `/api/v2/alerts` or `/api/v2/status` | ✅ PASS | `alertmanager.py:test_connection` GETs `{base_url}/api/v2/status` (optional bearer), returns `versionInfo.version` | +| CT-107 | Jellyfin calls `.users()` | ✅ PASS | `jellyfin.py:test_connection` → `JellyfinClient(...).users()` → `" users"` | +| CT-108 | Authentik probes directory endpoint | ✅ PASS | `authentik.py:test_connection` → `AuthentikClient(...).users(page=1, page_size=1)` → `" users"` | +| CT-109 | ssh_tasks reuses `build_ssh_client` (no duplicated SSH logic) | ✅ PASS | `ssh_tasks.py:test_connection` imports `build_ssh_client` from `task_runner` + `.connect()`; **does NOT call `test_machine_ssh`** | +| CT-110 | Nextcloud probes `/status.php` | ✅ PASS | `nextcloud.py:test_connection` GETs `{base_url}/status.php`, returns `version` | +| CT-111 | Backups `test_callable = None` | ✅ PASS | `backups.py` `DEFINITION` omits `test_callable` → default `None`; returns no-test response | +| CT-112 | No persistence (no `upsert_service`/`update_setting`) | ✅ PASS | Endpoint has zero persistence calls; `test_no_persistence_after_test` asserts store count unchanged | +| CT-113 | Secrets never logged | ✅ PASS | Only logs `test requested type=%s ok=%s`; no body/config/secrets. ⚠ test is weak (see N-2) | +| CT-114 | API client fn + hook + type | ✅ PASS | `api/services.ts:testServiceInstance`, `hooks/useServices.ts:useTestServiceInstance`, `types/index.ts:ServiceTestResult` | +| CT-115 | Test button in both create + edit dialogs | ✅ PASS | `ServiceTestPanel` in `ServicesPage.tsx::CreateServiceDialog` AND `Settings.tsx::ServiceConfigEditor` | +| CT-116 | Result pill renders both states w/ evidence/detail | ✅ PASS | `ServiceTestPanel` renders `✓ Connected — {evidence}` (default variant) / `✗ {detail}` (destructive) | +| CT-117 | Create/Save gated on `testPassed` + Save-anyway override | ✅ PASS | `confirmDisabled` includes `!testPassed`; `testPassed = (testResult?.ok) \|\| saveAnyway`; override verified | +| CT-118 | Editing a connectivity field clears result | ✅ PASS | store-previous pattern in both parents (ServicesPage on `draft`; Settings on `testInput` via JSON.stringify) | +| CT-119 | Backend tests: routines/dispatch/validation/no-persistence | ✅ PASS | `test_credential_tester.py` (helper + 7 types) + `test_api.py::TestServiceTestEndpoint` (4 tests); 362 passed | +| CT-120 | Frontend tests: button/pill/gating/field-clear | ✅ PASS (w/ gap) | `ServiceTestPanel.test.tsx` (7 cases); page-level gating/field-clear not explicitly tested (see N-5) | +| CT-121 | Build + lint green | ✅ PASS | `npm run build` ✓; `npm run lint` 0 errors (1 pre-existing unrelated warning); `ruff` clean | + +--- + +## 2. Gate outputs (exact) + +``` +$ cd backend && PYTHONPATH=src python3 -m pytest -q +362 passed, 2 warnings in 40.87s +(warnings: pre-existing StarletteTestClient + pythonjsonlogger deprecation notices — unrelated) + +$ cd backend && PYTHONPATH=src python3 -m ruff check src tests +All checks passed! + +$ cd frontend && npm run build +vite v8.0.10 building … ✓ 2548 modules transformed. +dist/assets/index-1Qjmq9l.js 1,107.90 kB +✓ built in 990ms +(tsc -b clean; one chunk-size advisory — pre-existing, unrelated) + +$ cd frontend && npm run lint +src/components/WidgetConfigDialog.tsx + 370:8 warning react-hooks/exhaustive-deps (pre-existing, unrelated to this change) +✖ 1 problem (0 errors, 1 warning) + +$ cd frontend && npx vitest run +Test Files 45 passed (45) + Tests 158 passed (158) +``` + +--- + +## 3. Adversarial / special-attention checks + +**CT-103 no-persistence** — confirmed: `routers/services.py::test_instance` contains no +`store.upsert_service`, `store.update_setting`, or any write method. The only `store` access is +the injected object passed to `test_callable` (read-only `get_ssh_key` for ssh_tasks). The test +`test_no_persistence_after_test` asserts `before == after` count. ✅ + +**CT-104 qBit "Fails."** — confirmed: `qbittorrent.py` catches `RuntimeError` and, when +`"login failed"` is in the message, returns `"Authentication failed — qBittorrent rejected the +credentials."`. `QbittorrentClient._login` raises `RuntimeError("qBittorrent login failed: Fails.")` +on the literal `"Fails."` response. Test `test_login_failed_translates_to_auth_message` covers it. ✅ + +**CT-105 prometheus uses Grafana gateway** — confirmed: the routine POSTs to +`{grafana_url}/api/ds/query` (NOT `/api/v1/query` to a direct Prom). The body mirrors the gateway +query shape (`queries[0]` keyed by `datasource_uid`, `expr:"up"`). ✅ + +**CT-109 ssh_tasks reuses `build_ssh_client`** — confirmed: `ssh_tasks.py` does +`from media_library_viewer_api.services.task_runner import build_ssh_client` and calls +`build_ssh_client(store, service).connect()`. No `test_machine_ssh` import; no duplicated SSH +connection logic. ✅ + +**CT-117 store-previous / field-edit-clears-result** — confirmed in both surfaces: + +- `ServicesPage.tsx`: `prevDraft` ref comparison clears `testResult`+`saveAnyway` on any draft change. +- `Settings.tsx`: `prevTestInput` with `JSON.stringify` deep-compare clears on content change. +Both are the React-recommended "store previous prop during render" pattern (no `useEffect` sync +loops). ✅ + +**CT-118 both surfaces wired** — confirmed: `ServiceTestPanel` rendered in `CreateServiceDialog` +(ServicesPage.tsx:310) AND `ServiceConfigEditor` (Settings.tsx:1610). The spec's literal +"ServicePage.tsx" is correctly treated as stale per the design §0 source-finding; `ServicePage.tsx` +is read-only and correctly left untouched. ✅ + +**Secrets never logged** — confirmed: the only INFO logs are +`test requested type=%s ok=true (no test_callable)` / `test requested type=%s ok=%s`. No request +body, config, or secrets are interpolated. ✅ (see N-2 for the weak test) + +**Error-translation coverage** — the shared `translate_connection_error` handles: +HTTP 401/403 (auth), `requests.ConnectionError`/`OSError` (refused), DNS keywords (host not found), +SSL/certificate strings, `requests.Timeout`/`TimeoutError`/`asyncio.TimeoutError` (timed out), and +SSH "protocol banner". The `Timeout` check is correctly ordered before the `OSError` check (since +`requests.Timeout` subclasses `OSError`). ✅ + +**test_callable presence** — all 7 remote types wired (`qbittorrent`, `prometheus`, +`alertmanager`, `jellyfin`, `authentik`, `ssh_tasks`, `nextcloud`); `backups` is the only `None`. +✅ + +**Save-anyway checkbox** — it appears **always** (not only on failure). The spec CT-117 requires it +to "appear" and override the gate; it does not mandate "only on failure." The implementation's +always-visible choice is acceptable UX and the override works (`testPassed = ok || saveAnyway`). +Informational, not a defect. + +--- + +## 4. Structured status & actionContext findings + +The native SDD status engine reported change selection as **ambiguous** +(`per-instance-hook-scoping, service-credential-tester`) and all dependencies as `blocked` — +because `changeName` was `null` at orchestrator resolution time. This verify phase was scoped by +the parent prompt to `service-credential-tester` explicitly, so the ambiguity does not block this +read-only verification. `actionContext.mode` is `repo-local` with `allowedEditRoots` covering +`/home/user/manage`; all edited files are inside the workspace. No workspace/boundary violation. + +--- + +## 5. Review workload / PR-boundary findings + +The `tasks.md` Review Workload Forecast recommended chained PRs (stacked-to-main), ~550–700 total +lines across two ≤400-line slices. The implementation delivered exactly two slices (`3391fbc` S1 +backend, `f6c67bd` S2 frontend) matching the forecast boundary. No scope creep beyond the assigned +task list. No `size:exception` was recorded or needed. ✅ + +--- + +## 6. Findings + +### Blocking (archive blockers) + +**B-1: `apply-progress.md` is MISSING.** The required `apply-progress` input artifact does not exist +at `openspec/changes/service-credential-tester/apply-progress.md`. The verify contract requires it +as an input. (No TDD cycle is active for this change, so strict-TDD evidence is not applicable.) + +**B-2: All 29 tasks in `tasks.md` are unchecked.** Every implementation task (1.1–1.15, 2.1–2.9, +3.1–3.5) remains `- [ ]`; 0 are `- [x]`. Per the verify contract, unchecked implementation tasks are +a CRITICAL completeness issue and an archive blocker. The implementation is **functionally +complete** (proven by source reading + 362 backend / 158 frontend passing tests), so this is +stale-checkbox reconciliation rather than missing work — but the contract does not permit a clean +archive-ready PASS while unchecked implementation tasks remain. The unchecked lines are all +task-list entries (e.g. `- [ ] **1.1 Add \`TestResult\` dataclass …**`); reconciliation = mark +completed tasks`[x]` + add `apply-progress.md`. + +### Non-blocking + +**N-1: Uncommitted cosmetic diff in working tree.** `frontend/src/pages/ServicesPage.tsx` has an +uncommitted change that is purely JSX indentation/prettier reflow (no functional diff). Should be +committed or discarded; does not affect verification. + +**N-2: `test_secrets_not_logged` (CT-113) is weak/misleading.** The test sends `secrets: {}` +(empty) then asserts an arbitrary string (`"super-secret-hunter2"`) is absent from `caplog.text`. +Because the secret was never sent, the assertion is trivially satisfied and does **not** actually +exercise the property that secrets present in the request body are not logged. The +**implementation is correct** (the endpoint logs only `type`/`ok`), but the test should send a +real secret in the body (e.g. `secrets: {"password": "hunter2"}`) to genuinely verify CT-113. + +**N-3: Defensive `logger.exception` could log a traceback.** `services.py:205` +`logger.exception("test_callable raised for type=%s", ...)` emits a full traceback if a +`test_callable` ever raises. All routines catch internally, so this is a rare defensive path, but a +traceback could theoretically surface sensitive data embedded in an exception message. Low risk; +consider `logger.warning` with a truncated message instead. + +**N-4: Edit-flow test needs re-typed secrets.** In `ServiceConfigEditor`, `testInput.secrets` +comes from `draftSecrets` which starts empty for existing services. Testing an existing authed +service (e.g. Prometheus) without re-entering its API key fails the test +("Grafana API key is required") because the stored encrypted secret is not sent. This is an +inherent consequence of the no-persistence design (test operates on request-body plaintext; +secrets are encrypted at rest). The "Leave blank to keep the current value" hint implies retyping +is expected. Non-blocking UX note. + +**N-5: CT-120 page-level gating/field-clear not explicitly tested.** `ServiceTestPanel.test.tsx` +covers the presentational panel (button, pill states, callbacks). Because the panel is stateless +(the parent owns `testResult`/`saveAnyway` and the gating/field-clear logic), there is no explicit +test that (a) the Create/Save button is disabled until the test passes, or (b) editing a field +clears `testPassed` at the integration level. The logic is correct by source inspection, but the +test coverage is panel-only. Recommend a page-level test asserting the confirm button enables only +after a passing test. + +**N-6: Documented design deviation (improvement).** `ServiceTestPanel` is **presentational** +(parent owns state) rather than the design's stateful component (which used `useEffect([input])`). +This is a deliberate, docstring-documented change to avoid the React `setState`-in-effect footgun. +All of CT-114..CT-118 are still satisfied via parent wiring. Informational only. + +--- + +## 7. Residual risks + +- The edit-flow "must retype secrets to test" behavior (N-4) may surprise operators; document it + in the UI or user docs. +- The weak no-secret-logs test (N-2) gives false confidence in CT-113 coverage. +- Frontend integration-level gating is proven by source, not by an automated test (N-5). + +--- + +## 8. Conclusion + +The `service-credential-tester` change is **functionally complete and correct**: all 21 +requirements pass against source, and all five gates are green. The only blockers to archive are +lifecycle hygiene — the missing `apply-progress.md` and the 29 unchecked tasks in `tasks.md`. Once +the task checklist is reconciled to reflect the (verified-done) work and `apply-progress.md` is +added, the change is archive-ready. No code fixes are required; the non-blocking findings are +test-strength and UX-doc improvements.