From 0c60447dbf4f5de36ae3b3196954f904cac90c8e Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Wed, 19 Aug 2026 18:47:09 +0000 Subject: [PATCH] fix(auth): resolve OIDC sub claim to the Nextcloud canonical UID user_oidc maps an external IdP's sub to its own account UID; the two are equal only under the non-default --mapping-uid=sub --unique-uid=0. UnifiedTokenVerifier stored the raw claim directly as AccessToken.resource, so every downstream consumer keyed on it (Qdrant filters, api/management.py's user_id, app-password lookups) silently used the wrong identity: a lookup under the wrong key returns zero results, not an error. Add _resolve_canonical_uid, mirroring _validate_nextcloud_credentials's OCS v2 canonical-UID lookup in api/passwords.py but authenticated with the bearer token itself. Runs in both _verify_mcp_audience and _verify_without_audience_check, right before the claim is cached; the cache-read paths inherit the resolved value without a separate change. Falls back to the raw claim on any lookup failure. Address review from cbcoutinho: - Blocking: the OCS lookup now opens a dedicated short-lived client via nextcloud_httpx_client instead of the shared self.http_client. Nextcloud sets a session cookie on every OCS response, including failures, and the shared client would replay one user's authenticated session onto the next user's lookup, letting a rejected bearer token answer with a stale session's identity instead. - Widen the except clause from httpx.RequestError to Exception: the fallback to the claimed UID is safe by construction, so an unexpected error (a malformed NEXTCLOUD_HOST raising httpx.InvalidURL, a closed client) should degrade the same way a network error does rather than reject a token JWT/introspection already validated. - Exclude preferred_username from the cached payload alongside sub: the entry previously kept the raw preferred_username claim next to the resolved sub, two identity fields disagreeing about who the token belongs to. - Note in docs/keycloak-multi-client-validation.md that canonical-UID resolution is a silent no-op without user_oidc's --check-bearer=1. - Add regression tests for the widened except clause and the cache consistency fix; rework the existing OCS-lookup tests to mock nextcloud_httpx_client (matching api/passwords.py's own test pattern) instead of self.http_client.get, since the fix no longer calls that shared client for this lookup. Fixes #1326 BREAKING CHANGE: AccessToken.resource (and any app password stored under it) now key on the Nextcloud canonical UID instead of the raw OIDC sub/preferred_username claim on external-IdP deployments where they differ. Rows already provisioned under the raw claim (server/ auth_tools.py, auth/provision_routes.py) become unreachable under the new key; affected users see NotProvisionedError until they re-provision their app password. --- docs/keycloak-multi-client-validation.md | 7 + nextcloud_mcp_server/auth/unified_verifier.py | 149 +++++++++- tests/unit/test_unified_verifier.py | 277 ++++++++++++++++++ 3 files changed, 428 insertions(+), 5 deletions(-) diff --git a/docs/keycloak-multi-client-validation.md b/docs/keycloak-multi-client-validation.md index b43595bba..60f6bbf9f 100644 --- a/docs/keycloak-multi-client-validation.md +++ b/docs/keycloak-multi-client-validation.md @@ -192,6 +192,13 @@ php occ user_oidc:provider keycloak-realm \ --bearer-provisioning=1 ``` +`--check-bearer=1` is not just a Keycloak-side setting: the MCP server's own canonical-UID +resolution (#1326, `UnifiedTokenVerifier._resolve_canonical_uid`) depends on `user_oidc` +actually validating the bearer token against this provider. Without it, the OCS `/cloud/user` +lookup this server makes still runs, but Nextcloud has nothing to map the token to, and the +resolution silently falls back to the raw OIDC claim, i.e. the exact bug #1326 fixes stays +unfixed with no error anywhere. + **Step 2: MCP Server Registers with Keycloak (DCR)** ```python # MCP server startup diff --git a/nextcloud_mcp_server/auth/unified_verifier.py b/nextcloud_mcp_server/auth/unified_verifier.py index 93eccf720..9181a8a88 100644 --- a/nextcloud_mcp_server/auth/unified_verifier.py +++ b/nextcloud_mcp_server/auth/unified_verifier.py @@ -361,13 +361,25 @@ async def _verify_mcp_audience(self, token: str) -> AccessToken | None: "(Nextcloud will validate its own audience)" ) + # Resolve the raw identity claim to Nextcloud's own account UID + # before it becomes AccessToken.resource (#1326): everything + # downstream keys storage on that field. + raw_username = payload.get("sub") or payload.get("preferred_username") + resolved_username = ( + await self._resolve_canonical_uid(token, raw_username) + if raw_username + else None + ) + # Recorded only once the AccessToken actually exists — not at each # validation stage, and not merely once the audience check passes. # `_create_access_token` still returns None (without raising) for a # payload carrying no `sub`/`preferred_username`, so recording any # earlier means a token that is about to be refused is counted as # accepted. "valid" has to mean the caller got a token. - access_token = self._create_access_token(token, payload) + access_token = self._create_access_token( + token, payload, resolved_username=resolved_username + ) if access_token is None: return self._reject( validation_method, @@ -478,6 +490,16 @@ async def _verify_without_audience_check( payload.get("sub"), ) + # Resolve the raw identity claim to Nextcloud's own account UID + # before it becomes AccessToken.resource (#1326), same as the + # MCP-audience path. + raw_username = payload.get("sub") or payload.get("preferred_username") + resolved_username = ( + await self._resolve_canonical_uid(token, raw_username) + if raw_username + else None + ) + # Cache and return the token. via_userinfo is derived from how we # actually validated — never from a payload claim (see # _create_access_token_with_cache_key). @@ -486,6 +508,7 @@ async def _verify_without_audience_check( payload, cache_key, via_userinfo=(validation_method == "userinfo"), + resolved_username=resolved_username, ) # Creation still returns None (without raising) for a payload with # no `sub`/`preferred_username`, and a stage passing is not the same @@ -1016,8 +1039,111 @@ async def _validate_via_userinfo( logger.debug("Token validated via userinfo for user: %s", data.get("sub")) return data + async def _resolve_canonical_uid(self, token: str, claimed_uid: str) -> str: + """Resolve an OIDC identity claim to the Nextcloud account UID. + + ``user_oidc`` maps an external IdP's ``sub`` to its own account UID; + the two are equal only under the non-default + ``--mapping-uid=sub --unique-uid=0``. Trusting the raw claim keys + every downstream store (Qdrant filters, app-password lookups) on an + identity Nextcloud itself does not use, and the failure is silent: a + lookup under the wrong key returns zero results, not an error (#1326). + + Mirrors ``_validate_nextcloud_credentials``'s OCS v2 canonical-UID + lookup in ``api/passwords.py``, but authenticates with the bearer + token itself rather than a login/password pair: this module's own + docstring establishes that token reuse against Nextcloud is safe + (RFC 8707), and ``context_helper.py`` already reuses this exact + token the same way once verification succeeds. + + Falls back to ``claimed_uid`` on any lookup failure, so an OCS + outage degrades to the behavior before this method existed rather + than rejecting an otherwise-valid token. + + Args: + token: The bearer token to present to Nextcloud. + claimed_uid: The unverified ``sub``/``preferred_username`` claim. + + Returns: + The canonical Nextcloud UID, or ``claimed_uid`` if it could not + be resolved. + """ + nextcloud_host = getattr(self.settings, "nextcloud_host", None) + if not nextcloud_host: + return claimed_uid + + try: + async with nextcloud_httpx_client(timeout=10.0) as client: + response = await client.get( + f"{nextcloud_host.rstrip('/')}/ocs/v2.php/cloud/user", + headers={ + "Authorization": f"Bearer {token}", + "OCS-APIRequest": "true", + }, + params={"format": "json"}, + ) + except Exception as e: + # A dedicated, short-lived client (never self.http_client, which + # is shared across every user's verification): Nextcloud sets a + # session cookie on every OCS response, including failures, and a + # long-lived client would replay user A's session on user B's + # lookup. Broad except (not just httpx.RequestError): the + # fallback to claimed_uid is safe by construction, so an + # unexpected error here (a malformed NEXTCLOUD_HOST, a client + # already closed, ...) should degrade the same way a network + # error does rather than reject a token JWT/introspection + # already validated. + logger.warning( + "Canonical-UID lookup failed for %s, using claimed value: %s", + claimed_uid, + e, + ) + return claimed_uid + + if response.status_code != 200: + logger.warning( + "Canonical-UID lookup returned HTTP %s for %s, using claimed value", + response.status_code, + claimed_uid, + ) + return claimed_uid + + try: + payload = response.json() + except ValueError: + logger.warning( + "Canonical-UID lookup returned a non-JSON body for %s, using " + "claimed value", + claimed_uid, + ) + return claimed_uid + + # Parsed defensively, same shape as _validate_nextcloud_credentials: + # a malformed OCS body must degrade, never raise. + ocs = payload.get("ocs") if isinstance(payload, dict) else None + ocs_data = ocs.get("data") if isinstance(ocs, dict) else None + canonical_uid = ocs_data.get("id") if isinstance(ocs_data, dict) else None + if not canonical_uid: + logger.warning( + "Canonical-UID lookup returned no id for %s, using claimed value", + claimed_uid, + ) + return claimed_uid + + if canonical_uid != claimed_uid: + logger.info( + "Resolved OIDC claim %s to canonical Nextcloud UID %s", + claimed_uid, + canonical_uid, + ) + return canonical_uid + def _create_access_token( - self, token: str, payload: dict[str, Any] + self, + token: str, + payload: dict[str, Any], + *, + resolved_username: str | None = None, ) -> AccessToken | None: """ Create AccessToken object from validated token payload. @@ -1025,13 +1151,18 @@ def _create_access_token( Args: token: The bearer token payload: Validated token payload + resolved_username: Canonical Nextcloud UID from + :meth:`_resolve_canonical_uid`, when the caller already ran + that (async) lookup. Falls back to the raw claim when omitted. Returns: AccessToken object or None if required fields missing """ # Use default cache key (hash of token) cache_key = hashlib.sha256(token.encode()).hexdigest() - return self._create_access_token_with_cache_key(token, payload, cache_key) + return self._create_access_token_with_cache_key( + token, payload, cache_key, resolved_username=resolved_username + ) def _create_access_token_with_cache_key( self, @@ -1040,6 +1171,7 @@ def _create_access_token_with_cache_key( cache_key: str, *, via_userinfo: bool = False, + resolved_username: str | None = None, ) -> AccessToken | None: """ Create AccessToken object from validated token payload with custom cache key. @@ -1052,12 +1184,19 @@ def _create_access_token_with_cache_key( fallback. Sourced from the caller (how validation happened), never from a payload claim — it gates the allowlist relaxation and the short cache TTL, so it must not be forgeable by the IdP response. + resolved_username: Canonical Nextcloud UID from + :meth:`_resolve_canonical_uid`, when the caller already ran + that (async) lookup. Falls back to the raw claim when omitted, + so this stays sync and callers that never resolve (tests, + cache reconstruction) are unaffected. Returns: AccessToken object or None if required fields missing """ # Extract username (sub claim, with fallback to preferred_username) - username = payload.get("sub") or payload.get("preferred_username") + username = ( + resolved_username or payload.get("sub") or payload.get("preferred_username") + ) if not username: # Deliberately silent: every caller routes a None result through # _reject(), whose WARNING carries the client_id and reason this @@ -1105,7 +1244,7 @@ def _create_access_token_with_cache_key( **{ k: v for k, v in payload.items() - if k not in ("sub", "scope", "_auth_via_userinfo") + if k not in ("sub", "scope", "_auth_via_userinfo", "preferred_username") }, } if via_userinfo: diff --git a/tests/unit/test_unified_verifier.py b/tests/unit/test_unified_verifier.py index 169700372..f166442dc 100644 --- a/tests/unit/test_unified_verifier.py +++ b/tests/unit/test_unified_verifier.py @@ -540,6 +540,283 @@ def test_client_id_from_claims_falls_through_empty_client_id(self, base_settings ) +class TestCanonicalUidResolution: + """Regression tests for #1326. + + An external IdP's `sub` need not equal the Nextcloud UID `user_oidc` + assigns it. `_resolve_canonical_uid` looks up the real UID via OCS + `/cloud/user`, authenticated with the bearer token itself, through a + dedicated short-lived client (never `self.http_client`, which is shared + across every user's verification and would replay one user's Nextcloud + session cookie onto another's lookup). + """ + + def _ocs_response(self, uid: str): + response = MagicMock() + response.status_code = 200 + response.json.return_value = { + "ocs": {"meta": {"statuscode": 200}, "data": {"id": uid}} + } + return response + + def _mock_ocs_client(self, response=None, *, side_effect=None): + """A mocked `nextcloud_httpx_client` context manager, matching the + pattern `api/passwords.py`'s own OCS-call tests already use.""" + mock_get = AsyncMock() + if side_effect is not None: + mock_get.side_effect = side_effect + else: + mock_get.return_value = response + mock_client = AsyncMock() + mock_client.get = mock_get + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + # Must resolve falsy: a truthy __aexit__ return SUPPRESSES an + # exception raised inside the `async with` block (real + # httpx.AsyncClient.__aexit__ returns None), so a naive AsyncMock() + # here would swallow the network-error/unexpected-error cases before + # they ever reach the method's own except clause. + mock_client.__aexit__ = AsyncMock(return_value=False) + return mock_client, mock_get + + async def test_resolves_to_canonical_uid(self, base_settings): + verifier = UnifiedTokenVerifier(base_settings) + mock_client, mock_get = self._mock_ocs_client(self._ocs_response("alice")) + + with patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ): + result = await verifier._resolve_canonical_uid( + "bearer-token", "f47ac10b-58cc-4372-a567-0e02b2c3d479" + ) + + assert result == "alice" + mock_get.assert_awaited_once() + _, kwargs = mock_get.call_args + assert kwargs["headers"]["Authorization"] == "Bearer bearer-token" + + async def test_falls_back_to_claimed_uid_on_network_error(self, base_settings): + verifier = UnifiedTokenVerifier(base_settings) + mock_client, _ = self._mock_ocs_client( + side_effect=httpx.ConnectError("connection refused") + ) + + with patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ): + result = await verifier._resolve_canonical_uid( + "bearer-token", "f47ac10b-58cc-4372-a567-0e02b2c3d479" + ) + + assert result == "f47ac10b-58cc-4372-a567-0e02b2c3d479" + + async def test_falls_back_to_claimed_uid_on_unexpected_error(self, base_settings): + """Not just `httpx.RequestError`: any failure from the lookup must + degrade to the claimed value, since the fallback is safe by + construction and the caller's own `except Exception` would otherwise + reject a token JWT/introspection already validated (e.g. a malformed + `NEXTCLOUD_HOST` raising `httpx.InvalidURL`, which is not a + `RequestError`).""" + verifier = UnifiedTokenVerifier(base_settings) + mock_client, _ = self._mock_ocs_client(side_effect=httpx.InvalidURL("bad host")) + + with patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ): + result = await verifier._resolve_canonical_uid( + "bearer-token", "f47ac10b-58cc-4372-a567-0e02b2c3d479" + ) + + assert result == "f47ac10b-58cc-4372-a567-0e02b2c3d479" + + async def test_falls_back_to_claimed_uid_on_non_200(self, base_settings): + verifier = UnifiedTokenVerifier(base_settings) + response = MagicMock() + response.status_code = 401 + mock_client, _ = self._mock_ocs_client(response) + + with patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ): + result = await verifier._resolve_canonical_uid("bearer-token", "some-sub") + + assert result == "some-sub" + + async def test_falls_back_to_claimed_uid_on_malformed_body(self, base_settings): + verifier = UnifiedTokenVerifier(base_settings) + response = MagicMock() + response.status_code = 200 + response.json.return_value = {"ocs": {"data": []}} # not a dict + mock_client, _ = self._mock_ocs_client(response) + + with patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ): + result = await verifier._resolve_canonical_uid("bearer-token", "some-sub") + + assert result == "some-sub" + + async def test_falls_back_to_claimed_uid_on_non_json_body(self, base_settings): + verifier = UnifiedTokenVerifier(base_settings) + response = MagicMock() + response.status_code = 200 + response.json.side_effect = ValueError("not JSON") + mock_client, _ = self._mock_ocs_client(response) + + with patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ): + result = await verifier._resolve_canonical_uid("bearer-token", "some-sub") + + assert result == "some-sub" + + async def test_skips_lookup_without_nextcloud_host(self, base_settings): + base_settings.nextcloud_host = None + verifier = UnifiedTokenVerifier(base_settings) + mock_client, mock_get = self._mock_ocs_client(self._ocs_response("alice")) + + with patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ): + result = await verifier._resolve_canonical_uid("bearer-token", "some-sub") + + assert result == "some-sub" + mock_get.assert_not_awaited() + + async def test_verify_mcp_audience_stores_canonical_uid_not_raw_sub( + self, base_settings + ): + """End-to-end: a token whose `sub` is a Keycloak UUID must resolve to + the Nextcloud UID in AccessToken.resource, not the raw claim. + + This reproduces #1326: on `main`, `access_token.resource` here comes + back as the raw UUID, and every downstream consumer (Qdrant filters, + `api/management.py`'s `user_id`) is keyed on an identity Nextcloud + itself does not use. + """ + verifier = UnifiedTokenVerifier(base_settings) + raw_sub = "f47ac10b-58cc-4372-a567-0e02b2c3d479" + introspection_response = { + "active": True, + "sub": raw_sub, + "aud": ["test-client-id"], + "scope": "openid profile", + "exp": int(time.time() + 3600), + "client_id": "test-client-id", + } + mock_client, _ = self._mock_ocs_client(self._ocs_response("alice")) + + with ( + patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ), + patch.object( + verifier, "_introspect_token", return_value=introspection_response + ), + ): + result = await verifier._verify_mcp_audience("opaque-token-12345") + + assert result is not None + assert result.resource == "alice" + assert result.resource != raw_sub + + async def test_canonical_uid_replaces_preferred_username_in_cache_too( + self, base_settings + ): + """The cached entry must not carry the stale raw `preferred_username` + claim alongside the resolved `sub`: two identity fields disagreeing + about who the token belongs to is a live trap for any future reader + that prefers `preferred_username`.""" + verifier = UnifiedTokenVerifier(base_settings) + raw_sub = "f47ac10b-58cc-4372-a567-0e02b2c3d479" + introspection_response = { + "active": True, + "sub": raw_sub, + "preferred_username": raw_sub, + "aud": ["test-client-id"], + "scope": "openid profile", + "exp": int(time.time() + 3600), + "client_id": "test-client-id", + } + mock_client, _ = self._mock_ocs_client(self._ocs_response("alice")) + + with ( + patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ), + patch.object( + verifier, "_introspect_token", return_value=introspection_response + ), + ): + await verifier._verify_mcp_audience("opaque-token-12345") + + cache_key = hashlib.sha256(b"opaque-token-12345").hexdigest() + cached_payload, _ = verifier._token_cache[cache_key] + assert cached_payload["sub"] == "alice" + assert "preferred_username" not in cached_payload + + async def test_management_api_path_also_stores_canonical_uid( + self, monkeypatch, base_settings + ): + """The management-API verification path shares the same fix.""" + monkeypatch.setenv("ALLOWED_MGMT_CLIENT", "test-client-id") + from nextcloud_mcp_server.config import _reload_config + + _reload_config() + verifier = UnifiedTokenVerifier(base_settings) + raw_sub = "f47ac10b-58cc-4372-a567-0e02b2c3d479" + introspection_response = { + "active": True, + "sub": raw_sub, + "client_id": "test-client-id", + "scope": "openid profile", + "exp": int(time.time() + 3600), + } + mock_client, mock_get = self._mock_ocs_client(self._ocs_response("alice")) + + with ( + patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ), + patch.object( + verifier, "_introspect_token", return_value=introspection_response + ), + ): + result = await verifier.verify_token_for_management_api( + "opaque-token-12345" + ) + + assert result is not None + assert result.resource == "alice" + + # A second call must hit the cache, not the OCS endpoint again, and + # must still carry the resolved UID (not the raw claim). + mock_get.reset_mock() + with ( + patch( + "nextcloud_mcp_server.auth.unified_verifier.nextcloud_httpx_client", + return_value=mock_client, + ), + patch.object( + verifier, "_introspect_token", return_value=introspection_response + ), + ): + cached_result = await verifier.verify_token_for_management_api( + "opaque-token-12345" + ) + assert cached_result.resource == "alice" + mock_get.assert_not_awaited() + + class TestVerifyTokenFlow: """Test complete verify_token flow."""