fix(client): resolve DAV paths via current-user-principal discovery - #980
Merged
cbcoutinho merged 3 commits intoJul 1, 2026
Merged
Conversation
cbcoutinho
added a commit
that referenced
this pull request
Jul 1, 2026
The keycloak lane's Login Flow v2 tests never actually ran the reproduction they claimed: the OAuth-leg fixture requested `talk.*` scopes not registered on the Keycloak client (invalid_scope → Playwright #username timeout), and the Login Flow leg 404'd on the Keycloak-origin login_url (fixed separately via NEXTCLOUD_PUBLIC_URL). Rework: - OAuth leg now uses a Keycloak direct-grant (ROPC) with realm-supported scopes instead of the flaky browser auth-code flow — its identity is irrelevant to the reproduction (the Login Flow v2 app password authenticates DAV). - Session-scope the divergent user + provisioned client so a single live user and one browser login serve the lane (the app-password store is keyed by the shared Keycloak `admin` identity, so per-test users would be deleted while their app password is still cached). - Reframe as end-to-end Keycloak Login Flow v2 WebDAV coverage. These do NOT reproduce #980's wrong-path bug on NC32/33: Nextcloud resolves `/remote.php/dav/files/<email>/` to the real home, so the round-trip succeeds with or without the client-side principal-discovery fix. #980's failure mode needs a backend (e.g. LDAP) where the loginName is not a valid files-path alias; it stays covered by #980's own mocked unit tests. The tests remain valuable: they fill the missing keycloak Login Flow coverage and guard the NEXTCLOUD_PUBLIC_URL fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cbcoutinho
added a commit
that referenced
this pull request
Jul 1, 2026
test(ldap): reproduce GH #980 divergent-principal DAV bug via OpenLDAP
- discover and cache DAV principal ids for WebDAV and CardDAV paths - resolve CalDAV calendar homes through async-safe principal lookup - cover divergent principal, fallback, retry, caching, and encoded href cases
… path Rebased onto master (which now carries the OpenLDAP cbcoutinho#980 reproduction lane from cbcoutinho#997). With this PR's BaseNextcloudClient._ensure_principal_id fix present, the divergent-principal WebDAV round-trip lands in alice's real home, so the test passes — remove the xfail(strict=True) marker and reword the docstring from "reproduction (xfails until the fix)" to a passing regression guard. Verified locally: `pytest -m ldap` → 1 passed with the fix (was 1 xfailed on master without it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cbcoutinho
force-pushed
the
codex/dav-principal-discovery
branch
from
July 1, 2026 22:53
e4d2ea2 to
462549e
Compare
cbcoutinho
added a commit
to dismantl/nextcloud-mcp-server
that referenced
this pull request
Jul 1, 2026
… via Login Flow v2 Adds the missing Login Flow v2 app-password integration tests for the mcp-keycloak service (port 8002). The keycloak lane previously only covered DCR/authorize; it never provisioned an app password or exercised a DAV path. The new fixtures obtain a Keycloak OAuth token (browser auth-code + PKCE) and complete Nextcloud Login Flow v2, logging in as a *local* Nextcloud user via its EMAIL. Nextcloud keys the app password on the loginName (the email), which differs from the canonical UID, so context.py builds DAV paths from the email (/remote.php/dav/files/<email>/) instead of the real home dir. Without PR cbcoutinho#980 the WebDAV cycle targets the wrong home and fails (RED); with the current-user-principal discovery fix it resolves the UID and passes (GREEN). This is the keycloak-service counterpart of the existing login_flow WebDAV test. Card: Deck cbcoutinho#489 (board 12). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cbcoutinho
added a commit
to dismantl/nextcloud-mcp-server
that referenced
this pull request
Jul 1, 2026
…ia OpenLDAP Adds an `ldap` docker-compose profile + integration lane that provides the live RED→GREEN reproduction of cbcoutinho#980 (DAV paths built from the loginName instead of the canonical Nextcloud UID) that the Keycloak lane (cbcoutinho#993) could not: login-by-email resolves to the real home (email is a path alias) and user_oidc hardcodes loginName == UID. - `openldap` service (vegardit/openldap:2.6.10) seeds user `alice` from ldap/bootstrap.ldif. Nextcloud's user_ldap maps her to a UID derived from the LDAP entryUUID, so `loginName (alice) != UID` AND `/remote.php/dav/files/alice/` does not resolve to her real home. - user_ldap is configured by an app-hook (app-hooks/post-installation/15-setup-ldap-backend.sh), gated on the openldap service so it is a no-op for every other lane — mirroring 15-setup-keycloak-provider.sh. - tests/server/ldap drives the multi-user BasicAuth MCP service (port 8003) as `alice`; the round-trip is xfail(strict=True): it fails on master (bug present) and xpasses once cbcoutinho#980's BaseNextcloudClient._ensure_principal_id discovery lands, at which point the marker should be dropped. - CI matrix gains an `ldap` lane (reuses multi-user-basic + `ldap` profile, no browser); docs added to CLAUDE.md. Verified locally: fresh-install hook auto-configures user_ldap; test xfails on master and xpasses with cbcoutinho#980 cherry-picked. Relates to cbcoutinho#980. Tracked on Deck card cbcoutinho#490. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sword The quality gate fails with "C Security Rating on New Code" from three python:S2068 findings (hardcoded "app-pw" passed to CalendarClient in the DAV principal-discovery unit tests). Hoist the dev-only literal to a single `_APP_PW` constant annotated `# NOSONAR(S2068)` so the suppression anchors to the literal and ruff won't wrap it, matching the repo's convention for test-only credentials. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Owner
|
Thanks for your contribution! I was able to reproduce the issue in a separate PR, and then once that was merged rebased your work onto the latest |
This was referenced Jul 2, 2026
Merged
1 task
cbcoutinho
added a commit
that referenced
this pull request
Jul 10, 2026
test(ldap): add login-flow × LDAP integration lane (cross-mode #980 guard)
pi0n00r
pushed a commit
to pi0n00r/nextcloud-mcp-server
that referenced
this pull request
Jul 10, 2026
…inho#980 guard) The `ldap` lane only exercised multi-user-BasicAuth mode (port 8003). Extend the GH cbcoutinho#980 principal-resolution coverage to login-flow mode (port 8004), where identity comes via OIDC and Nextcloud API access via a per-user app password — a different path to the username used for DAV path construction. This proves BaseNextcloudClient._ensure_principal_id resolves the divergent LDAP loginName→UID regardless of deployment mode. - New `login_flow_ldap` pytest marker (dedicated → existing `login_flow` and `ldap` lanes stay zero-touch; verified via --collect-only). - New test tests/server/login_flow/test_ldap_dav_principal.py mirroring the multi-user-basic WebDAV round-trip, driven via Login Flow v2 as the LDAP alice. - New fixture nc_mcp_login_flow_ldap_alice_client reusing the existing _get_login_flow_token_for_user / _provision_login_flow_mcp_client helpers (no test_users_setup coupling — alice is user_ldap-provisioned on first login). - New CI matrix lane `login-flow-ldap` (profile login-flow + `--profile ldap`, playwright, port 8004); Verify-LDAP-backend and Verify-OIDC steps extended to the new mode. - CLAUDE.md: document both LDAP lanes; correct the stale xfail note (cbcoutinho#980's fix has landed → both are passing guards). - Incidental: add `assert ... is not None` narrowing after existing pytest.skip guards in login_flow/conftest.py so the ty-check pre-commit hook passes on the touched file (ty doesn't treat pytest.skip as NoReturn; these were pre-existing errors surfaced by editing the file). Refs: Deck cbcoutinho#617 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 22, 2026
This was referenced Aug 6, 2026
1 task
1 task
1 task
cbcoutinho
added a commit
that referenced
this pull request
Sep 8, 2026
…6-x82r) Login Flow v2 hands out a transferable URL: whoever opens it and clicks "Grant access" produces the app password, and the flow says nothing about who asked for it. Both completion paths stored that password under the OAuth caller who *started* the flow, so any authenticated caller could start one, send the link to a victim and — after a single click — hold that victim's Nextcloud credential for every later call. Before storing, the account that granted must now be shown to be the caller. The comparison is on canonical Nextcloud UIDs, never names: - the granter is resolved by authenticating the fresh app password against OCS /cloud/user, which maps the Login Flow loginName (an email alias, or an LDAP login that differs from the UID — GH #980) onto the account Nextcloud will act as. This mirrors what api/passwords.py already does for GHSA-x88r-fhx7-52h6; - the caller is the OAuth sub, plus the IdP's preferred_username when an external IdP (Keycloak) issues opaque UUID subjects. Both call sites — the browser route GET /app/provision and the MCP tool nc_auth_check_status — go through the one helper. A grant that does not match, or that cannot be verified at all, is refused: nothing is stored and the app password it produced is revoked. Operators upgrading should treat app passwords provisioned before this release as suspect: purge stored rows and re-provision, and revoke the device tokens in Nextcloud, since existing grants cannot be attributed retroactively. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01422hRnsXruUySa9kFz5tCm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



I use external LDAP authentication for my Nextcloud instance, and
nextcloud-mcp-serverwouldn't work for my user account because its loginName and UID differ. This PR fixes that, and I deployed it to my Nextcloud 34 instance and it successfully solved the problem for me.Problem
nextcloud-mcp-serverbuilds every DAV path from the configuredusername, assuming that segment equals the user's canonical Nextcloud UID:/remote.php/dav/files/<username>//remote.php/dav/addressbooks/users/<username>//remote.php/dav/calendars/<username>/That assumption holds on simple/local installs where
loginName == UID, but breaks on LDAP and external-OIDC deployments where they can diverge. For example:loginName = alice/cloud/userreportsid = alice_1234(LDAP-suffixed canonical UID)alicealice, missing the real data at/files/alice_1234/If a separate local user named
aliceexists,PROPFIND /files/alice/can return207with empty or stale content. That creates silent wrong-path behavior rather than a clean 404. All three DAV clients are affected: WebDAV files, CardDAV contacts, and CalDAV calendars/tasks.Fix
WebDAV + CardDAV (
BaseNextcloudClient)Add
_ensure_principal_id()toclient/base.py: a lazyPROPFIND /remote.php/dav/requesting<d:current-user-principal/>(RFC 5397). On success, the last href segment isunquote()-decoded and cached as_principal_id. On failure (transport error, parse error, missing/empty href), a warning is logged and the method returns without caching, so the next call can retry._get_webdav_base_path()and_get_carddav_base_path()now call_principal_or_username()(discovered id orself.usernamefallback). Public DAV methods inwebdav.pyandcontacts.pycallawait self._ensure_principal_id()before building paths.CalDAV (
CalendarClient)CalendarClientuses the caldav library rather than raw PROPFIND. A new_ensure_calendar_home()method discovers the calendar home viaprincipal().get_property(CalendarHomeSet)(RFC 4791, preferred), falls back to thecalendar_home_setattribute, and falls back further to deriving the principal id from the principal URL. The result is cached in_calendar_home_url. Calendar and task methods that construct URLs guard withawait self._ensure_calendar_home().Safety net
The existing
self.usernameremains the fallback at every step. When UID == loginName, discovery returns the same segment and paths are unchanged. If discovery is unavailable, existing username-based behavior is preserved.Tests
The divergent-principal case cannot be reached in the integration suite because local Nextcloud users always have matching UID and loginName. New mocked unit tests in
tests/unit/client/test_dav_principal_discovery.pycover:self.usernamepath, no exception raised_ensure_principal_id()issues at most one PROPFIND per client instancealice%40example.com) are decoded correctlyCalendarHomeSetdiscovery is preferred over deriving from the principal URLget_property(CalendarHomeSet)is awaited correctly whether the caldav version returns a coroutine or a plain valueExisting path-helper unit tests that build clients and call public methods are updated to pre-seed
_principal_discovered = True, preserving existing_make_requestcall-order assertions.The integration suite serves as the regression guard for the equal-case local-user behavior.
Relationship to PR #638
PR #638 introduced the same principal-discovery idea but bundles additional independent changes: CardDAV server-side
query/limit, RU->LAT transliteration, resilient vCardBDAYparsing, and OCS sharing path normalization. This PR extracts only the DAV path identity fix as a standalone, focused change.It also corrects a bug present in #638's implementation:
unquote()was not applied to the extracted principal id, producing wrong paths for UIDs containing percent-encoded characters, such as OIDC sub-claims with@or+.The other changes from #638 are out of scope here and can be proposed as separate PRs.
Out of scope
query/limit, transliteration, vCardBDAYparsing, and OCS sharing normalizationloginNameincontext.pyor the app-password storeThe discovery approach fixes the DAV-path issue at the client layer for all deployment modes without a schema migration.
Validation
uv run ruff checkuv run ruff format --checkuv run ty check -- nextcloud_mcp_serveruv run pytest tests/unit/ -q-> 1908 passed, 1 skipped, 2 warningsKnown xfail:
tests/integration/test_mail_greenmail.py::test_send_message_via_outbox, a Mail/GreenMail outbox environment limitation unrelated to DAV principal discovery.