diff --git a/openspec/changes/service-credential-tester/proposal.md b/openspec/changes/service-credential-tester/proposal.md new file mode 100644 index 0000000..df1f78b --- /dev/null +++ b/openspec/changes/service-credential-tester/proposal.md @@ -0,0 +1,125 @@ +# SDD Proposal: Service Credential Tester + +**Change:** `service-credential-tester` +**Phase:** proposal +**Date:** 2026-07-09 + +## 1. Problem / Why Now + +Adding a service in Manage today is **blind**: the user fills in a URL + credentials, clicks "Create service," and the row is persisted regardless of whether the backend can actually reach or authenticate against the external service. Failures only surface later, as error states inside individual widgets (or, for qBittorrent, as a `RuntimeError: qBittorrent login failed: ...` in the backend logs — invisible to the operator staring at the add form). The operator's first signal that something is wrong is a broken dashboard widget, possibly hours later. + +Two concrete pain points triggered this: + +1. **qBittorrent login failures** are being diagnosed from backend logs rather than at entry time. The operator typed credentials, the form accepted them, the service saved, and only `RuntimeError(f"qBittorrent login failed: {resp.text.strip()}")` in logs reveals the auth failure. +2. **URL/schema mistakes** (caught by validation in #1's fix, committed separately) are a related but distinct concern: validation rejects malformed input at submission; the tester checks *live reachability + auth* before submission. Both layers are wanted. + +There is already a proven pattern in the codebase: `test_machine_ssh` in `routers/settings.py` takes unsaved SSH machine input, attempts `.connect()`, translates specific error strings into helpful messages (banner failure, auth failure, etc.), and returns a structured result. This change generalizes that pattern to every service type, surfaced in the add/edit dialog as a "Test credentials" button that gates the save. + +## 2. Target Users and Situations + +- **Primary users:** Operators onboarding a new external service (Jellyfin, qBittorrent, Prometheus, Alertmanager, Authentik, ssh_tasks, backups) who want to confirm they got the URL/credentials right before committing. +- **Operators debugging** a service that stopped working: edit the service, re-test, see the specific failure (DNS, refused, auth, SSL, CSRF), fix, re-test, save. +- **Urgency:** Medium-high. This removes a real daily friction (silently-saved-broken-service) and is the prerequisite for trusting the service registry as "configured = working." + +## 3. Product Outcome + +After this change, the add-service and edit-service dialogs have a **"Test credentials"** button that, for the current (unsaved) form values: + +- Attempts a minimal, read-only connection against the target service. +- Returns either **✓ connected** (with a small piece of proof — e.g. qBittorrent version, Prometheus build info, Jellyfin server name) **or ✗ failed** with a specific, actionable message (auth refused, host unreachable, SSL error, timeout, CSRF/Referer rejected). +- The **Create / Save button is disabled until the test passes** (configurable per-operator preference; default ON; a "save anyway" escape hatch is available for services that are temporarily down but being pre-configured). + +The same tester is reachable from the service page as a "Re-test" action for already-saved services (useful when a service goes down and comes back). + +## 4. Scope Boundaries and Non-Goals + +### In scope + +- **Backend:** a new endpoint `POST /api/services/test` that accepts `{service_type, config, secrets}` (NOT a service id — the tester works on unsaved input), dispatches to a per-type test routine, and returns `{ok: bool, detail: str, evidence?: str}`. The dispatch registry is closed and compile-time (mirrors `SERVICE_DEFINITIONS`). +- **Per-type test routines** (minimal, read-only): + - **qbittorrent:** login via `/api/v2/auth/login`, then `GET /api/v2/sync/maindata` (or `/app/version`). Evidence: qBittorrent version. + - **prometheus:** `GET /api/v1/query?query=up` (or `/api/v1/status/buildinfo`). Evidence: Prometheus version. + - **alertmanager:** `GET /api/v2/alerts` (or `/api/v2/status`). Evidence: cluster version. + - **jellyfin:** `GET /Users` (already exposed by `JellyfinClient.users()`). Evidence: user count + server name. + - **jellyseerr:** (if still present as a distinct type) `GET /user`. Evidence: count. + - **authentik:** a lightweight authenticated GET against its directory endpoint (already exists). Evidence: instance slug. + - **backups:** no remote connection (data is internal) — test always succeeds (or is omitted from the tester UI for this type). + - **ssh_tasks:** reuse the existing `test_machine_ssh` flow at the service level (connect + banner check). Evidence: connected host. + - **nextcloud:** `GET /status.php` (unauthenticated server probe). Evidence: nextcloud version. +- **Frontend:** "Test credentials" button in `CreateServiceDialog` and the edit dialog on `ServicePage`; a `useTestService` mutation hook; test-result state; a small inline status pill (✓/✗ + detail); "Create/Save" button gating on a successful test (with an override toggle). +- **Error translation:** map common failure modes to human-friendly messages — connection refused, DNS failure, SSL/cert error, HTTP 401/403 (auth), timeout, qBit "Fails." response, etc. +- **Reuse for qBittorrent login diagnosis** (#3): the tester surfaces the exact `resp.text` qBit returns, making the "login failed" logs issue debuggable from the UI. + +### Non-goals (explicitly out of scope) + +- **Write-path testing.** Tests are read-only (no add-pause-delete-torrent probes, no create-user probes). +- **Background health-checking / monitoring.** The tester is on-demand only; continuous polling stays with the existing widget data-fetch path. +- **Replacing the validation layer.** Schema validation (the #1 fix) and the tester are complementary: validation rejects malformed config before it hits the network; the tester checks live connectivity. Both ship. +- **Per-widget test.** The test is per-service-instance, not per-widget-kind. +- **Async/long-running tests.** Each test has a short timeout (≤10s) and runs synchronously from the user's perspective. +- **Secrets persistence during test.** The test endpoint accepts plaintext secrets in the request body (over TLS, like every other secret-bearing endpoint), tests, and discards them — it does NOT persist anything. + +## 5. High-Level Approach + +### 5.1 Backend + +1. **`integrations/base.py`** — add an optional `test_callable` field to `ServiceDefinition` (a callable `(config, secrets) -> TestResult` or `None` for types without a remote). `TestResult` is a small dataclass `{ok: bool, detail: str, evidence: str | None}`. Types without a test (e.g. `backups`) leave it `None`. +2. **Per-type test routines** live alongside each integration (e.g. `integrations/qbittorrent.py` gains `test_connection(config, secrets) -> TestResult`). They construct the appropriate client with a short timeout, attempt the minimal probe, catch exceptions, and translate to friendly messages. +3. **`routers/services.py`** — add `POST /api/services/test` taking `ServiceInstanceInput` (reuses the existing model: service_type + config + secrets). It resolves the definition, runs `test_callable` (or returns `{ok: True, detail: "No connection test for this service type"}` if none), and returns the result. Validation runs first (reuse `_validate_input`) so malformed configs fail fast with the same 422 as create. +4. **Error translation helper** — a small shared utility mapping common exception types/messages to human strings, modeled on the `test_machine_ssh` message translation. + +### 5.2 Frontend + +1. **`api/services.ts`** — add `testServiceInstance(input) -> Promise`. +2. **`hooks/useServices.ts`** — add `useTestServiceInstance()` mutation hook. +3. **`CreateServiceDialog`** (in `ServicesPage.tsx`) + the edit dialog on `ServicePage.tsx` — add a "Test credentials" button below the form fields; on click, fire the mutation with the current draft; render a result pill (`✓ Connected — ` green / `✗ ` red) inline; track `testPassed` state; gate the Create/Save confirm button on `testPassed` (with a "Save anyway" checkbox override for temporarily-down services). +4. The test result clears whenever the user edits any field that affects connectivity (URL, secrets) — re-test required after edits. + +### 5.3 Security note + +The test endpoint accepts plaintext secrets in the request body, exactly as the existing `POST /api/services/instances` create endpoint does (secrets are encrypted at rest only after persistence). The test endpoint does NOT persist; it decrypts nothing (the body is already plaintext); it discards the secrets after the probe. Auth gating is the same as every other service endpoint (JWT/API-key). No new attack surface. + +## 6. Success Criteria / Acceptance Criteria + +1. Every service type with a remote connection (qbittorrent, prometheus, alertmanager, jellyfin, authentik, ssh_tasks, nextcloud) has a working `test_connection` routine; types without remote data (backups) report `{ok: True, detail: "No test needed"}`. +2. The add-service dialog has a "Test credentials" button that returns a clear ✓/✗ result with evidence or a specific failure message. +3. The Create button is disabled until either the test passes OR the operator checks "Save anyway." +4. Editing a connectivity field (URL, secrets) clears the previous test result, requiring a re-test. +5. qBittorrent auth failure is surfaced as a readable message (e.g. "Authentication failed — qBittorrent returned 'Fails.'"), not just a generic error — resolving the #3 "login failed in logs" pain at the UI layer. +6. The same tester is reachable from the service edit dialog for already-saved services. +7. Backend tests cover: each type's test routine (mocked client) returns the expected shape; the endpoint dispatches correctly; validation runs before the test. +8. Frontend tests cover: the Test button fires the mutation, the result pill renders both states, the Create button gating works, field-edit clears the result. +9. `pytest`, `npm run build`, `npm run lint`, `npm run test` all stay green. +10. No secrets are persisted by the test endpoint (verified by a test that calls `/test` and asserts no new service row exists). + +## 7. Risks and Mitigations + +| Risk | Mitigation | +|------|------------| +| **Test endpoint leaks secrets in logs.** | The endpoint logs at INFO only `test requested type=%s ok=%s`; never logs the request body or secrets. Sanitize via the existing `sanitize_log_extra`. | +| **Slow tests block the UI.** Each test hits a remote. | Short timeout per type (≤10s, most ≤5s); the UI shows a "Testing…" state; the button re-enables on completion. | +| **Per-type test routines duplicate client logic.** | Reuse the existing clients (`JellyfinClient`, `QbittorrentClient`, etc.) — each test routine is a thin wrapper that constructs the client + calls one method. | +| **False negatives from transient network blips.** | The "Save anyway" escape hatch ensures an operator pre-configuring a not-yet-online service isn't blocked. | +| **Error message quality varies per type.** | Shared error-translation helper covers the common cases (refused, DNS, SSL, 401/403, timeout); type-specific messages (e.g. qBit "Fails.") handled in their routine. | +| **Review budget (>400 lines).** | Likely 2 slices: (S1) backend endpoint + per-type routines + tests; (S2) frontend Test button + gating + tests. | +| **#1 (validation surfacing) overlaps.** | No conflict — #1 ships first as a quick fix; the tester adds the live-connection layer on top. Both coexist. | + +## 8. Resolved Questions (no question round needed — settled during investigation) + +- **Q1 — Endpoint shape?** `POST /api/services/test` taking `ServiceInstanceInput`, returning `{ok, detail, evidence?}`. +- **Q2 — Gate Create on test pass?** Yes, with a "Save anyway" override (default-on gating, but escapable for offline-pre-config). +- **Q3 — Test on edit, or only on create?** Both — same dialog component, same flow. +- **Q4 — Backups type?** No remote test; returns ok-with-detail. Not shown in the UI for that type. + +## 9. Future Phases + +1. **Continuous health probes** — promote the tester into a periodic background check that flags services going down (separate from this on-demand feature). +2. **Per-widget-kind test** — for widgets that take their own config (e.g. a PromQL query), test that the specific query returns data. +3. **Test history** — record the last test result + timestamp on the service row, visible in the service page. + +--- + +## Notes for downstream phases + +- **#1 (validation surfacing) is a prerequisite quick-fix**, already committed in `493c0e1`. It ensures the tester's own validation failures (malformed config) are visible in the dialog. +- **#3 (qBittorrent login logs)** is materially resolved by this change's acceptance criterion #5 — the operator will see the auth failure in the UI at add time, not in backend logs.