fix(api): distinguish Nextcloud 5xx from auth failure; tighten body parse (#824)

Address PR #825 review round 2:
- _validate_nextcloud_credentials now only maps OCS HTTP 401/403 to a 401
  "invalid credential"; any other non-200 (5xx, 503 maintenance mode) surfaces
  as 502 "Nextcloud returned a server error" so ops don't chase a phantom bad
  password when Nextcloud is actually down.
- The client-facing 401 message is now a parameter, so delete_app_password keeps
  its "Invalid credentials" wording without unwrapping/rebuilding the helper's
  JSONResponse.
- Body parsing catches (ValueError, UnicodeDecodeError) instead of bare
  Exception, and guards body.get behind isinstance(body, dict) — no longer
  swallows RuntimeError/AttributeError or a non-object JSON body.
- Add a unit test asserting 500/503 -> 502.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/api/passwords.py

@@ -188,7 +188,11 @@ async def _get_app_password_storage(request: Request) -> RefreshTokenStorage:
 
 
 async def _validate_nextcloud_credentials(
-    nextcloud_host: str, login_name: str, password: str
+    nextcloud_host: str,
+    login_name: str,
+    password: str,
+    *,
+    invalid_credential_error: str = "Invalid app password",
 ) -> tuple[str | None, JSONResponse | None]:
     """Validate a credential against Nextcloud and return the account UID.
 
@@ -206,11 +210,15 @@ async def _validate_nextcloud_credentials(
     crash behind issue #824 (``AttributeError: 'list' object has no attribute
     'get'`` escaping as an unhandled 500).
 
+    Args:
+        invalid_credential_error: client-facing error string for the 401 path,
+            so callers can keep their own wording.
+
     Returns:
         ``(ocs_user_id, None)`` on success, otherwise ``(None, error_response)``
-        with a ready-to-return :class:`JSONResponse`: 401 for an invalid
-        credential, 502 when Nextcloud is unreachable or returns something we
-        cannot parse.
+        with a ready-to-return :class:`JSONResponse`: **401** for an invalid
+        credential, **502** when Nextcloud is unreachable, errors out (5xx /
+        maintenance mode), or returns something we cannot parse.
     """
     try:
         async with nextcloud_httpx_client(
@@ -229,13 +237,22 @@ async def _validate_nextcloud_credentials(
             status_code=502,
         )
 
-    # v2.php maps an OCS auth failure onto a real HTTP status (e.g. 401).
-    if response.status_code != 200:
+    # v2.php maps an OCS auth failure onto a real HTTP status. Only 401/403 mean
+    # "bad credential" — anything else non-200 (5xx, 503 maintenance mode) is a
+    # Nextcloud-side problem and must surface as 502, not a misleading "invalid
+    # password" that sends ops chasing the wrong cause.
+    if response.status_code in (401, 403):
         logger.warning("Credential validation failed: HTTP %s", response.status_code)
         return None, JSONResponse(
-            {"success": False, "error": "Invalid app password"},
+            {"success": False, "error": invalid_credential_error},
             status_code=401,
         )
+    if response.status_code != 200:
+        logger.error("Nextcloud OCS returned HTTP %s", response.status_code)
+        return None, JSONResponse(
+            {"success": False, "error": "Nextcloud returned a server error"},
+            status_code=502,
+        )
 
     # Parse defensively: even on HTTP 200 the body may be malformed or carry a
     # non-dict ``ocs.data`` (the v1.php auth-failure shape, kept as a guard
@@ -268,7 +285,7 @@ async def _validate_nextcloud_credentials(
             ocs_user_id is not None,
         )
         return None, JSONResponse(
-            {"success": False, "error": "Invalid app password"},
+            {"success": False, "error": invalid_credential_error},
             status_code=401,
         )
 
@@ -342,10 +359,11 @@ async def provision_app_password(request: Request) -> JSONResponse:
     nc_username = None
     try:
         body = await request.json()
+    except (ValueError, UnicodeDecodeError):
+        body = None  # No / malformed JSON body = legacy call without extras
+    if isinstance(body, dict):
         scopes = body.get("scopes")  # list[str] | None
         nc_username = body.get("username")  # Nextcloud loginName
-    except Exception:
-        pass  # No JSON body = legacy call without scopes / loginName
 
     login_name = nc_username or username
 
@@ -476,9 +494,10 @@ async def delete_app_password(request: Request) -> JSONResponse:
     nc_username = None
     try:
         body = await request.json()
+    except (ValueError, UnicodeDecodeError):
+        body = None  # No / malformed JSON body = legacy call without a loginName
+    if isinstance(body, dict):
         nc_username = body.get("username")
-    except Exception:
-        pass  # No JSON body = legacy call without an explicit loginName
     login_name = nc_username or username
 
     # Validate credentials against Nextcloud. Shares the OCS v2 + defensive
@@ -495,16 +514,15 @@ async def delete_app_password(request: Request) -> JSONResponse:
             status_code=500,
         )
 
+    # Keep this route's historical "Invalid credentials" wording via the helper
+    # rather than unwrapping and rebuilding its response.
     ocs_user_id, error_response = await _validate_nextcloud_credentials(
-        nextcloud_host, login_name, password
+        nextcloud_host,
+        login_name,
+        password,
+        invalid_credential_error="Invalid credentials",
     )
     if error_response is not None:
-        # Preserve the historical "Invalid credentials" wording for this route.
-        if error_response.status_code == 401:
-            return JSONResponse(
-                {"success": False, "error": "Invalid credentials"},
-                status_code=401,
-            )
         return error_response
 
     # The authenticated account must be the UID whose password is being deleted.

tests/unit/test_management_app_password_endpoints.py

@@ -429,6 +429,33 @@ async def test_provision_app_password_ocs_v1_failure_payload_returns_401(mocker)
     assert "Invalid app password" in response.json()["error"]
 
 
+async def test_provision_app_password_nextcloud_5xx_returns_502(mocker):
+    """A Nextcloud server error / maintenance mode (5xx) surfaces as 502, not a
+    misleading 401 that blames the user's password."""
+    mocker.patch(
+        "nextcloud_mcp_server.api.passwords.get_settings",
+        return_value=MagicMock(
+            nextcloud_host="http://localhost:8080",
+            nextcloud_verify_ssl=True,
+            nextcloud_ca_bundle=None,
+        ),
+    )
+    for status_code in (500, 503):
+        passwords._rate_limit_attempts.clear()
+        _mock_ocs_client(mocker, status_code=status_code, json_payload={})
+        client = TestClient(_provision_only_app())
+        response = client.post(
+            "/api/v1/users/testuser/app-password",
+            headers={
+                "Authorization": create_basic_auth_header(
+                    "testuser", "aaaaa-bbbbb-ccccc-ddddd-eeeee"
+                )
+            },
+        )
+        assert response.status_code == 502, f"HTTP {status_code} should map to 502"
+        assert "server error" in response.json()["error"].lower()
+
+
 async def test_provision_app_password_nondict_data_does_not_500(mocker):
     """Regression for #824: a non-dict ``ocs.data`` under HTTP 200 (list, or
     ``null``) is handled gracefully (401), not as an unhandled 500/