fix(auth): resolve OIDC sub claim to the Nextcloud canonical UID - #1357
fix(auth): resolve OIDC sub claim to the Nextcloud canonical UID#1357AmirF194 wants to merge 1 commit into
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes external-IdP deployments where the OIDC sub (or preferred_username) claim is incorrectly treated as the Nextcloud canonical UID, causing downstream lookups (e.g., Qdrant filtering, management API user_id, app-password storage keys) to silently miss and return empty results.
Changes:
- Add canonical UID resolution in
UnifiedTokenVerifierby calling OCS v2/cloud/userwith the bearer token and storing the returned UID intoAccessToken.resource. - Apply the resolution consistently for both MCP tool verification (
_verify_mcp_audience) and the management API verification path (_verify_without_audience_check). - Add unit regression tests covering success, fallback behavior, and cache-hit behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
nextcloud_mcp_server/auth/unified_verifier.py |
Resolves identity claims to Nextcloud’s canonical UID via OCS v2 before caching/issuing AccessToken, ensuring downstream components key on the correct UID. |
tests/unit/test_unified_verifier.py |
Adds regression tests for canonical UID resolution, including fallback cases and management-API cache-hit behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # 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 |
cbcoutinho
left a comment
cbcoutinho
left a comment
There was a problem hiding this comment.
Thanks for this — the approach is right, it's what the issue itself prescribes, and it lands in both verification paths. I checked the claim about cache reconstruction rather than taking it on trust: because _create_access_token_with_cache_key rewrites sub in the cache entry, both _get_cached_token and the management-API cache-hit branch do inherit the resolved value without a separate change. Good.
Ran it locally on the branch: ruff check, ruff format --check, ty check and the full unit suite (3610 passed) are green. Worth noting CI gave this almost nothing — only SonarCloud (pass) and license/cla (pending, unsigned) ran; no test lane triggered, so my local run is the only test signal on it.
One blocking item, then a few smaller ones.
🔴 Blocking
Nextcloud session cookies accumulate on the shared client — see the inline comment on the OCS call. This one is a cross-user identity-assignment risk, and it's introduced by this PR: before it, self.http_client never talked to Nextcloud in an external-IdP deployment.
🟡 Worth a round
The docstring's fallback contract isn't actually held — inline on the except clause.
Silent re-keying of already-provisioned app passwords. AccessToken.resource is the write key too, not just the read key: server/auth_tools.py:314 and auth/provision_routes.py:114 both store app passwords under a user_id that traces back to it (extract_user_id_from_token → auth/token_utils.py:282, and validate_token_and_get_user → api/management.py:208). On an existing external-IdP deployment, rows written under the raw sub become unreachable the moment this ships, and the user gets NotProvisionedError until they re-provision.
That's a defensible outcome — the old key was wrong — but it's currently silent. Per CLAUDE.md's breaking-change convention this wants a BREAKING CHANGE: footer on the commit naming the version and the re-provision step, so it lands in CHANGELOG.md instead of living only in a PR description.
No coverage above the unit tier. The unit tests are genuinely good — I confirmed the mocked-OCS assertions fail on master, and each fallback branch (network error / non-200 / malformed / non-JSON / no host) is covered. But changed auth behaviour on the /api/v1/* provider surface is exactly what the repo's e2e + contract gate is aimed at. The real reproduction lane is external-idp over in astrolabe; a follow-up card on Deck board 11 referencing it would close this honestly rather than leaving the gap implicit. Your PR body already discloses "not run", which is the right instinct — this is just one step further.
🟢 Smaller
ocs.meta.statuscodeunchecked, as Copilot noted. Low value on v2, which maps the OCS status onto HTTP — but it's oneisinstanceline if you want the mirror of_validate_nextcloud_credentialsto be exact.- INFO log volume, also raised by Copilot — I'd push back on that one and keep it at INFO. It fires per token validation, not per request, so the rate is bounded by the cache TTL, and a one-time-per-token identity remap is precisely what an operator wants to see when diagnosing this class of bug.
- Docs: this is a silent no-op unless
user_oidcruns with--check-bearer=1(docs/keycloak-multi-client-validation.md:191). One line saying so saves someone an afternoon of "the fix doesn't do anything". - A cache-entry consistency nit inline.
| return claimed_uid | ||
|
|
||
| try: | ||
| response = await self.http_client.get( |
There was a problem hiding this comment.
🔴 Blocking: OCS session cookies accumulate on this shared client.
self.http_client is a long-lived httpx.AsyncClient built once in __init__ and shared across every token verification for every user. Nextcloud sets a session cookie on every OCS response, including failures — verified against the local stack:
$ curl -D- -H 'OCS-APIRequest: true' -H 'Authorization: Bearer bogus' \
'http://localhost:8080/ocs/v2.php/cloud/user?format=json'
HTTP/1.1 401 Unauthorized
Set-Cookie: ocj699c4ri4i=b03aaeb8e4b5ac9148dfc9cdc37a1c4e; path=/; HttpOnly; SameSite=Lax
Set-Cookie: oc_sessionPassphrase=b0BGsgLiUIsplIp3P3V6WL6BFKtahUtkdAmjZOf1TCZ0XG8bDHqm...
httpx extracts those into client.cookies and replays them on the next request. So a successful lookup for user A leaves an authenticated Nextcloud session in the jar, and user B's lookup arrives carrying it. If B's bearer isn't accepted by OCS — a token minted for a different OIDC client, or a provider without --check-bearer=1 — the stale session can answer instead: HTTP 200 with A's ocs.data.id, which this method then treats as authoritative and writes into B's AccessToken.resource. That's cross-user identity assignment for Qdrant filtering and app-password lookup, arriving through the code path meant to make identity correct.
This isn't hypothetical repo-lore — client/__init__.py:104 (AsyncDisableCookieTransport) exists solely to stop this, and both siblings this method says it mirrors avoid it structurally by opening a fresh client per call: api/passwords.py:229 and auth/storage.py:2050. Matching them is the smallest fix:
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"},
)Before this PR, self.http_client only ever talked to the IdP, so in the external-IdP deployment this change targets, the jar never held Nextcloud cookies at all. The PR is what introduces them.
| headers={"Authorization": f"Bearer {token}", "OCS-APIRequest": "true"}, | ||
| params={"format": "json"}, | ||
| ) | ||
| except httpx.RequestError as e: |
There was a problem hiding this comment.
🟡 The docstring promises "falls back to claimed_uid on any lookup failure ... rather than rejecting an otherwise-valid token", but only httpx.RequestError is caught here.
Anything else escapes into the caller's except Exception in _verify_mcp_audience / _verify_without_audience_check and becomes _reject(...) — so a token that already passed JWT verification or introspection gets refused because a best-effort UID lookup misbehaved. Reachable cases: httpx.InvalidURL from a malformed NEXTCLOUD_HOST (not a RequestError), a RuntimeError if the client has been closed, or anything unexpected from the response object.
Widen it to except Exception — the fallback value is safe by construction, so there's no reason to be selective about what triggers it.
| # 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") |
There was a problem hiding this comment.
🟢 Nit: the cache entry built just below writes "sub": username (now the canonical UID) but spreads the rest of payload through unchanged, so preferred_username stays as the raw claim. The entry ends up internally inconsistent — two identity fields disagreeing about who the token belongs to.
Harmless today, since both readers do sub or preferred_username and sub wins. But it's a live trap for whoever next reads a cache entry, or adds a third reader that prefers preferred_username. Adding it to the exclusion set alongside sub and scope costs nothing.
3fb5b5f to
de1d464
Compare
|
Thanks for the thorough pass, especially catching the cookie-jar issue: I hadn't thought through what a shared client means once it starts talking to Nextcloud instead of just the IdP. Addressed in the new commit:
Left alone:
Full suite (3359 unit tests), ruff check/format, and ty check all clean on the branch in a fresh container. Re-requesting review. |
|
No rush, just checking in: I addressed the cross-user session-cookie issue and the three other points from your review a week ago (dedicated client per call, widened except clause, preferred_username cache fix, breaking-change note). Let me know if there's anything else you'd like changed. |
|
Hi @AmirF194 thanks for your contribution. Please submit a response to the cla. I'll run the unit/integration tests and provide another review shortly |
|
Thanks, will get that signed. The test/review round whenever you get to it is appreciated, no rush on my end. |
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 cbcoutinho#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.
de1d464 to
0c60447
Compare
|
Problem
External-IdP deployments (Keycloak, Authentik, Entra) leave
user_oidcon its default UID mapping, which is a Nextcloud-generated account UID, not the IdP'ssubclaim.UnifiedTokenVerifierstores the rawsub/preferred_usernameclaim directly asAccessToken.resource, and every consumer downstream (Qdrant document filtering,api/management.py'suser_id, app-password lookups) treats that as the Nextcloud UID. When the two differ, the failure is silent: a lookup keyed on the raw claim returns zero results with HTTP 200, not an error.api/passwords.py's_validate_nextcloud_credentialsalready solves this on the app-password path, since Nextcloud'sloginNamecan also differ from its UID: it resolves the canonical UID via OCSGET /ocs/v2.php/cloud/user(v2, not v1, since v1 returns 200 even on auth failure).unified_verifier.pynever applied that pattern to the OIDC path.Fix
_resolve_canonical_uidcalls the same OCS endpoint, authenticated with the bearer token itself rather than a login/password pair (the module's own docstring already establishes that token reuse against Nextcloud is safe: RFC 8707, andcontext_helper.pyreuses this exact token the same way once verification succeeds). It runs in both_verify_mcp_audience(MCP tool calls) and_verify_without_audience_check(management API), right before the claim is written into_token_cache/AccessToken.resource._get_cached_tokenand the management-API cache hit reconstruct the token from that same cache entry, so they inherit the resolved value without a separate change.On any lookup failure (Nextcloud unreachable, non-200, malformed body) it falls back to the raw claim, so an OCS outage degrades to the current behavior rather than rejecting an otherwise-valid token.
Verification
subresolves to the mocked canonical UID inAccessToken.resource, not the raw claim. Onmainthis assertion fails (resourcecomes back as the raw UUID).tests/unit(3610 cases) passes;ruff format/ruff check/ty check/deptryare clean. Not run: the live integration suite against a real Nextcloud + external IdP, so this verifies the mechanism against a mocked OCS response, not an end-to-end deployment.Fixes #1326