From 7ef0e9d83bf864176c96924fa2488bad1417c91e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 17 Jun 2026 20:40:05 +0200 Subject: [PATCH] docs(auth): document userinfo path in security model; drop dead guard; pin MCP asymmetry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address claude-review round 8 on #919: - Security-model docstring: note that opaque cross-client tokens authenticate via the userinfo liveness check (not JWKS/expiry) and bypass the client allowlist, with per-user authz as the gate. - Remove the redundant `if not payload: return None` after the JWT/opaque branches (both already return None on failure) — replace with a comment. - Add test_mcp_path_does_not_use_userinfo_for_opaque_token to pin that the userinfo fallback is management-path-only (MCP path still 401s). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/auth/unified_verifier.py | 11 ++++++++--- tests/unit/test_unified_verifier.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/nextcloud_mcp_server/auth/unified_verifier.py b/nextcloud_mcp_server/auth/unified_verifier.py index 50341040..d49a7642 100644 --- a/nextcloud_mcp_server/auth/unified_verifier.py +++ b/nextcloud_mcp_server/auth/unified_verifier.py @@ -175,6 +175,12 @@ class UnifiedTokenVerifier(TokenVerifier): - Verifies token signature against Nextcloud's JWKS (cryptographic proof) - Verifies token is not expired - Extracts user identity from validated token claims + - NOTE: for opaque cross-client tokens (e.g. Astrolabe) that + introspection reports inactive, authentication falls back to the + userinfo endpoint — a live IdP liveness check (200 + ``sub``) + rather than local JWKS/expiry verification. Such tokens are stamped + ``_auth_via_userinfo`` and bypass the client allowlist (step 4); + per-user authorization (step 2) remains the security gate. 2. **Authorization layer** (management API endpoints): - EVERY endpoint verifies: token.sub == requested_resource_owner @@ -420,9 +426,8 @@ class UnifiedTokenVerifier(TokenVerifier): # a userinfo failure metric when userinfo was never attempted. return None - # Check payload is valid - if not payload: - return None + # Both branches above either set a populated payload or have already + # returned None, so payload is guaranteed truthy here. # Skip audience validation - any valid Nextcloud token is accepted logger.debug( diff --git a/tests/unit/test_unified_verifier.py b/tests/unit/test_unified_verifier.py index 95d4dd52..a290f7b0 100644 --- a/tests/unit/test_unified_verifier.py +++ b/tests/unit/test_unified_verifier.py @@ -780,6 +780,21 @@ class TestUserinfoFallback: assert result is not None assert result.resource == "testuser" + async def test_mcp_path_does_not_use_userinfo_for_opaque_token( + self, userinfo_settings + ): + """The userinfo fallback applies only to the management API path, never + the MCP-audience path — an opaque token there is still rejected.""" + verifier = UnifiedTokenVerifier(userinfo_settings) + userinfo_mock = AsyncMock(return_value={"sub": "testuser"}) + with ( + patch.object(verifier, "_introspect_token", AsyncMock(return_value=None)), + patch.object(verifier, "_validate_via_userinfo", userinfo_mock), + ): + result = await verifier.verify_token("opaque-astrolabe-token") + assert result is None + userinfo_mock.assert_not_called() + async def test_opaque_rejected_when_no_validators_configured(self, base_settings): """With neither introspection nor userinfo configured, an opaque token is rejected without recording a misleading userinfo-failure metric."""