test(ldap): add login-flow × LDAP integration lane (cross-mode #980 guard)

[cbcoutinho]

What

The ldap CI lane only exercised multi-user-BasicAuth mode (port 8003) as the divergent LDAP user alice (loginName ≠ UID, GH #980 regression guard). This adds a second lane that exercises the same principal-resolution behaviour in 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.

It proves BaseNextcloudClient._ensure_principal_id resolves the divergent LDAP loginName→UID regardless of deployment mode.

Changes

• New login_flow_ldap pytest marker — dedicated so the existing login_flow and ldap lanes stay zero-touch (verified via --collect-only: -m login_flow and -m ldap both exclude the new test).
• New tests/server/login_flow/test_ldap_dav_principal.py mirroring the multi-user-basic WebDAV create→write→read→list→delete 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 configuration steps extended to the new mode.
• CLAUDE.md: document both LDAP lanes and correct the stale xfail note (fix(client): resolve DAV paths via current-user-principal discovery #980's fix has landed → both are passing guards).
• Incidental: assert ... is not None narrowing after the 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 — CI's ty check -- nextcloud_mcp_server never covered them).

Verified locally (end-to-end, not just static)

Ran the full stack (--profile ldap --profile login-flow) and the new lane against it:

tests/server/login_flow/test_ldap_dav_principal.py::...[chromium] PASSED
1 passed, 2647 deselected in 36.42s

Confirmed the test is meaningful: LDAP alice logs in as alice but her canonical UID is 57da83ba-0fda-1041-... (entryUUID-derived, her real home dir on disk). The round-trip succeeds only because principal discovery rewrites /files/alice/ → /files/57da83ba-.../. Investigation result: login-flow mode works correctly with the LDAP backend.

Test coverage

This PR is test-tier + CI config + docs — no product API surface changes, so the e2e+contract review gate is not triggered. It adds integration coverage (a new full-stack lane).

Tracking: Deck card #617 (board 12 — nextcloud-mcp-server Product).

---

This PR was generated with the help of AI, and reviewed by a Human

🤖 Generated with Claude Code

[claude]

Claude finished @cbcoutinho's task in 2m 28s —— View job

---

Review: PR #1053 — login-flow × LDAP integration lane

• Gather context (diff, CLAUDE.md, changed files)
• Review CI workflow changes (.github/workflows/test.yml)
• Review new test file (tests/server/login_flow/test_ldap_dav_principal.py)
• Review fixture changes (tests/server/login_flow/conftest.py)
• Review pyproject.toml marker addition
• Review CLAUDE.md doc updates
• Post review feedback

Summary

This is a well-scoped, low-risk test-infrastructure PR. It closely mirrors the existing tests/server/ldap/test_ldap_dav_principal.py pattern for the login-flow deployment mode, and the marker-leak bug (new test getting picked up by the single-user lane) was already caught and fixed in the second commit (4950dcb) — good self-review loop.

What I checked

• New CI lane (.github/workflows/test.yml): login-flow-ldap mode correctly reuses profile: login-flow + extra-compose-profiles: "--profile ldap", matching the exact pattern the existing ldap lane uses for multi-user-basic. wait-port/mcp-internal-url (8004) match the plain login-flow lane. Verify LDAP backend and Verify OIDC configuration steps are correctly extended with || matrix.mode == 'login-flow-ldap'.
• Marker isolation: Confirmed the new login_flow_ldap marker is disjoint from login_flow/ldap in every lane. The lanes that select by exact marker name (multi_user_basic, login_flow, keycloak, ldap) are naturally unaffected since pytest -m matches marker names exactly, not substrings. Only the single-user lane's compound "... and not X ..." expression needed the explicit and not login_flow_ldap addition — and that's already in the diff. I didn't find any other compound marker expression in the repo that would need the same fix (grepped for not keycloak/not multi_user_basic patterns).
• New fixture nc_mcp_login_flow_ldap_alice_client (tests/server/login_flow/conftest.py:973): Correctly composed from the existing _get_login_flow_token_for_user + _provision_login_flow_mcp_client helpers, with matching call signatures/argument order to the sibling per-user fixtures (alice_login_flow_mcp_client et al.). Session-scoped, doesn't depend on test_users_setup, as intended since LDAP alice is provisioned on first login rather than pre-seeded.
• New test (tests/server/login_flow/test_ldap_dav_principal.py): Structurally a faithful port of the multi-user-basic version (create→write→read→list→delete round-trip, same cleanup-in-finally pattern, same assertion style). Correctly scoped with pytestmark = [pytest.mark.integration, pytest.mark.login_flow_ldap].
• assert ... is not None narrowing in conftest.py (3 call sites after pytest.skip guards): reasonable, minimal fix for ty check, consistent with the stated rationale (ty doesn't treat pytest.skip as NoReturn).
• CLAUDE.md doc updates accurately describe both lanes and correctly retire the stale xfail note now that fix(client): resolve DAV paths via current-user-principal discovery #980's fix has landed on both.
• Credential handling: LDAP_LOGIN_FLOW_PASSWORD reuses the existing dev-only LDAP fixture credential with the same NOSONAR(S2068) suppression convention already used elsewhere in the repo — consistent, not a new pattern.

Minor observations (non-blocking)

• The new LDAP_LOGIN_FLOW_USERNAME = "alice" reuses the same literal username as the Nextcloud-native alice from test_users_setup used by alice_login_flow_mcp_client. Since the login_flow_ldap marker is CI-isolated from login_flow, this is fine in practice, but worth knowing if someone ever runs pytest -m "login_flow or login_flow_ldap" locally against a stack with both the ldap and login-flow profiles enabled simultaneously — behavior there is untested by this PR (not a regression, just an edge case outside current scope).
• Couldn't execute ruff/ruff format/ty check/pytest locally in this sandbox (Bash networked/tooling commands required approval that wasn't grantable in this session) — the PR body's "Verified locally" section and the passing SonarCloud quality gate comment on this PR give good independent confidence, and CI (Tests, Pact contract tests) is currently still running for this push.
• Per CLAUDE.md's test-coverage gate: this PR is test-tier + CI config + docs only, no product API surface change, so the e2e/contract gate correctly doesn't apply — the PR body already states this explicitly.

No blocking issues found.

[cbcoutinho]

Round-1 findings addressed in 4950dcb5:

• 🔴 Blocking — new marker leaked into the single-user lane: correct, and it's exactly why single-user / nc32 + nc33 failed on the first run. Added and not login_flow_ldap to both disjuncts of the single-user markers expression (.github/workflows/test.yml:86). Verified with --collect-only: the single-user expression now selects 0 of the new test, while -m login_flow_ldap still selects exactly it. Good catch — my pre-push --collect-only checks tested -m login_flow and -m ldap but not the single-user compound integration and … expression, which is where the exact-match gap bit.

Left as-is:

• Minor — duplicated LDAP_LOGIN_FLOW_USERNAME/_PASSWORD constants: deferring. They're two test-only literals; sharing them would couple login_flow/conftest.py to ldap/conftest.py (a cross-lane conftest import) for little gain. Worth revisiting only if a third cross-mode lane appears, as you noted.

New CI run triggered by the push (the login-flow-ldap lanes already passed on the prior run; this re-runs everything including the previously-failing single-user lane).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud