test: cover _update_auth_config — remove # pragma: no cover from the auth credential writer

[SckyzO]

test: cover _update_auth_config — remove # pragma: no cover from the auth credential writer

Symptom

_update_auth_config in src/rackscope/api/routers/auth.py is the function responsible for writing the admin password hash (and other auth-sensitive fields) back to app.yaml after a POST /api/auth/change-password or POST /api/auth/change-username call.

It is currently excluded from coverage:

# src/rackscope/api/routers/auth.py
def _update_auth_config(app_config: AppConfig, updates: dict) -> None:  # pragma: no cover
    """Persist auth changes back to app.yaml."""
    ...

This is the single most security-critical function in the auth layer — a bug here either (a) silently fails to persist a password change, leaving the user with the old credentials, or (b) corrupts app.yaml and breaks login for the entire instance. Yet there is no automated test verifying that it does the right thing.

Technical Analysis

The # pragma: no cover marker was almost certainly added because writing to the actual app.yaml is awkward to test directly — you don't want the test suite to mutate the developer's local config. But Pytest provides tmp_path for exactly this situation: a fresh temporary directory per test, automatically cleaned up.

The reason for the pragma is convenience, not necessity. The audit (MAIN-02) flagged this as a clear coverage gap on a high-risk surface.

What _update_auth_config does

Reading the current code, the function:

1. Opens app.yaml for read+write (or read followed by atomic replace).
2. Patches a small set of auth.* fields based on the updates dict (typically password_hash, possibly username).
3. Serializes back to YAML.
4. Triggers apply_config() to reload state from the new file.

Each of these four steps can fail in distinct ways that should be tested.

Proposed Fix

1. Remove the pragma

-def _update_auth_config(app_config: AppConfig, updates: dict) -> None:  # pragma: no cover
+def _update_auth_config(app_config: AppConfig, updates: dict) -> None:

2. Add a test module tests/unit/api/test_update_auth_config.py

Using tmp_path for a sacrificial app.yaml:

import yaml
from pathlib import Path
import pytest

def _write_initial_config(tmp_path: Path) -> Path:
    cfg = {
        "auth": {
            "enabled": True,
            "username": "admin",
            "password_hash": "$2b$13$OLD",
            "secret_key": "fixed-for-test",
            "session_duration": "24h",
        },
        "paths": {...},
        # ... minimal valid AppConfig shape
    }
    path = tmp_path / "app.yaml"
    path.write_text(yaml.safe_dump(cfg))
    return path

Test cases:

• Happy path — password change: call with {"password_hash": "$2b$13$NEW"} → file on disk reflects the new hash, all other auth fields preserved
• Happy path — username change: call with {"username": "alice"} → file reflects new username, password_hash preserved
• Combined update: both username and password in the same call → both applied atomically (the file is never in a partially-updated state visible to another reader)
• Non-auth fields untouched: paths, telemetry, cache, features in app.yaml are bit-identical pre and post
• YAML formatting preserved: comments and key ordering in app.yaml survive the round-trip (or, if not preserved, the test asserts the documented contract — e.g., "comments are stripped" must be deliberately documented, not silently lossy)
• Invalid file: corrupt YAML on input → raises a clear exception, does not silently fall back to empty config (this would erase the user's settings)
• Read-only file: chmod 444 app.yaml → raises PermissionError with a useful message
• Atomic write: simulate a crash between read and write (mock Path.write_text to raise) → original file content is preserved on disk, not truncated
• Triggers apply_config: after a successful write, apply_config is called (mock + call count assertion) so the in-memory state matches the file

3. Document the security criticality

Add a docstring to _update_auth_config that explicitly states its security-critical nature:

def _update_auth_config(app_config: AppConfig, updates: dict) -> None:
    """Persist auth changes back to app.yaml.

    SECURITY-CRITICAL. A bug here either corrupts app.yaml (locking everyone
    out of the instance) or fails silently (leaving the user on stale
    credentials). Any change to this function MUST be accompanied by updates
    to tests/unit/api/test_update_auth_config.py.
    """
    ...

This is one of the rare cases where a multi-line comment in code is warranted (per PROFILE_DEV.md: comment when removing it would confuse a future reader).

Test Checklist

• pytest tests/unit/api/test_update_auth_config.py passes with at least the 9 cases above
• Coverage report shows _update_auth_config at ≥90% line coverage (the function is small; this should be straightforward)
• make typecheck returns 0 errors
• No regression on existing auth tests
• If Issue J (ci: tighten lint coverage, make security scans blocking, enforce coverage threshold #36-pending — CI discipline) has shipped first and added --cov-fail-under=70, this PR helps reach that bar; document the coverage delta in the PR description

Impact and Severity

• Audience affected: anyone who calls POST /api/auth/change-password or POST /api/auth/change-username. Today, a bug in this function would only be discovered in production after an actual password change failed.
• Severity: medium. The function works today (no reported bugs); the priority is preventive — protecting against regressions in a security-critical path.
• Priority: Sprint 3 of the audit roadmap. Independent of other open issues, ships any time.

Breaking Changes

None. Internal test addition + docstring + pragma removal. Behaviour identical.

Related

• Audit ref: MAIN-02 in AUDIT_ARCHITECTURAL.md
• Related to upcoming RBAC work (Issue feat(auth): local RBAC backend — multi-user accounts with viewer/editor/admin roles #27 — backend RBAC) which will introduce users.yaml and partially obsolete _update_auth_config (auth state moves out of app.yaml). This issue's tests remain valuable during the transition: they cover the legacy single-admin code path until it is removed in the RBAC migration.

Out of Scope

• Refactoring _update_auth_config itself — only adding tests and the docstring. The audit did not flag a defect, only the coverage gap.
• Adding similar coverage for other functions tagged # pragma: no cover elsewhere — grep at implementation time, if more exist, file separate issues with the same pattern.
• Coverage tooling changes (e.g., switching from coverage.py to another tool) — out of scope.