From 9b4035b34aaa79af323bc73596f44ee584e9f717 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 1 Jul 2026 13:52:36 +0200 Subject: [PATCH 1/7] test(keycloak): reproduce divergent-principal DAV bug (#980) 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//) instead of the real home dir. Without PR #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 #489 (board 12). Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/server/keycloak/conftest.py | 348 ++++++++++++++++++ .../keycloak/test_keycloak_dav_principal.py | 110 ++++++ 2 files changed, 458 insertions(+) create mode 100644 tests/server/keycloak/conftest.py create mode 100644 tests/server/keycloak/test_keycloak_dav_principal.py diff --git a/tests/server/keycloak/conftest.py b/tests/server/keycloak/conftest.py new file mode 100644 index 000000000..becd35090 --- /dev/null +++ b/tests/server/keycloak/conftest.py @@ -0,0 +1,348 @@ +"""Fixtures for Keycloak-service Login Flow v2 integration tests (port 8002). + +The ``mcp-keycloak`` service uses Keycloak as the external OAuth IdP but reaches +Nextcloud via Login Flow v2 **app passwords** (``MCP_DEPLOYMENT_MODE=login_flow``), +exactly like the ``mcp-login-flow`` service — the only difference is the OAuth +IdP (Keycloak vs Nextcloud's built-in ``oidc`` app). The keycloak lane currently +only has DCR/authorize tests; the Login Flow v2 app-password integration tests +are missing. + +These fixtures fill that gap AND set up the divergent-principal condition fixed +by PR #980: + +* **OAuth leg** — a Keycloak auth-code flow (browser) obtains an access token + that the ``mcp-keycloak`` session accepts. This exercises the keycloak service. +* **Login Flow v2 leg** — the browser completes Nextcloud Login Flow v2 by logging + in as a *local* Nextcloud user using its **email address**. Nextcloud keys the + resulting app password on the *loginName* (the email), which differs from the + user's canonical UID. ``context.py`` then builds DAV paths from the email + (``/remote.php/dav/files//``) instead of the UID — the exact wrong-path + bug PR #980 fixes via ``current-user-principal`` discovery. + +A plain Keycloak/user_oidc login can NOT reproduce this: ``user_oidc``'s +``LoginController`` sets ``loginName == UID`` (the sha256 hash), so its DAV paths +are already correct. Login-by-email of a local user is the reliable divergence +generator (and matches PR #980's own ``alice@example.com`` unit tests). +""" + +import base64 +import hashlib +import json +import logging +import secrets +import time +import uuid +from typing import Any, AsyncGenerator +from urllib.parse import quote + +import anyio +import httpx +import pytest +from mcp import ClientSession +from mcp.types import ElicitRequestParams, ElicitResult + +from nextcloud_mcp_server.client import NextcloudClient +from tests.conftest import ( + DEFAULT_FULL_SCOPES, + create_mcp_client_session, +) +from tests.server.login_flow.conftest import _rewrite_login_flow_url + +logger = logging.getLogger(__name__) + +KEYCLOAK_MCP_URL = "http://localhost:8002/mcp" +KEYCLOAK_MCP_BASE_URL = "http://localhost:8002" +KEYCLOAK_BASE_URL = "http://localhost:8888" +KEYCLOAK_REALM = "nextcloud-mcp" + +# Static confidential client from keycloak/realm-export.json. It permits the +# test callback (redirectUris include http://localhost:*) and carries audience +# mappers for both `nextcloud-mcp-server` (MCP validation) and `nextcloud` +# (user_oidc validation). +KEYCLOAK_CLIENT_ID = "nextcloud-mcp-server" +KEYCLOAK_CLIENT_SECRET = "mcp-secret-change-in-production" + +# Keycloak user used only for the OAuth leg (session identity key). It does not +# have to match the Nextcloud data user — the app password minted by the Login +# Flow leg is what authenticates DAV requests. +KEYCLOAK_OAUTH_USER = "admin" +KEYCLOAK_OAUTH_PASSWORD = "admin" + + +@pytest.fixture() +async def divergent_email_user( + anyio_backend, nc_client: NextcloudClient +) -> AsyncGenerator[dict[str, str], Any]: + """Create a local Nextcloud user whose loginName (email) differs from its UID. + + Yields a dict with ``uid``, ``email``, ``password`` and ``display_name``. + The user is deleted on teardown. Nextcloud login-by-email is enabled by + default, so logging in with the email during Login Flow v2 produces an app + password whose stored loginName is the email — not the UID. + """ + suffix = uuid.uuid4().hex[:8] + uid = f"divprincipal_{suffix}" + user = { + "uid": uid, + "email": f"{uid}@example.com", + "password": "DivergentPrincipalPass123!", + "display_name": f"Divergent Principal {suffix}", + } + + logger.info("Creating divergent-principal user uid=%s email=%s", uid, user["email"]) + await nc_client.users.create_user( + userid=uid, + password=user["password"], + display_name=user["display_name"], + email=user["email"], + ) + + try: + yield user + finally: + try: + await nc_client.users.delete_user(uid) + logger.info("Deleted divergent-principal user %s", uid) + except Exception as e: # noqa: BLE001 - best-effort cleanup + logger.warning("Failed to delete divergent-principal user %s: %s", uid, e) + + +def _pkce_pair() -> tuple[str, str]: + """Return (code_verifier, code_challenge) for a PKCE S256 exchange.""" + verifier = secrets.token_urlsafe(64) + digest = hashlib.sha256(verifier.encode()).digest() + challenge = base64.urlsafe_b64encode(digest).rstrip(b"=").decode() + return verifier, challenge + + +@pytest.fixture() +async def keycloak_service_oauth_token( + anyio_backend, browser, oauth_callback_server +) -> str: + """Obtain a Keycloak access token accepted by the ``mcp-keycloak`` session. + + Drives the OAuth auth-code flow (with PKCE) against Keycloak using the + static ``nextcloud-mcp-server`` client and the test OAuth callback server. + Logs into Keycloak (not Nextcloud) via its native login form. + """ + auth_states, callback_url = oauth_callback_server + + async with httpx.AsyncClient(timeout=30.0) as http: + discovery = await http.get( + f"{KEYCLOAK_BASE_URL}/realms/{KEYCLOAK_REALM}" + "/.well-known/openid-configuration" + ) + try: + discovery.raise_for_status() + except httpx.HTTPStatusError as e: + pytest.skip(f"Keycloak realm not available: {e}") + oidc = discovery.json() + + authorization_endpoint = oidc["authorization_endpoint"] + token_endpoint = oidc["token_endpoint"] + + state = secrets.token_urlsafe(32) + verifier, challenge = _pkce_pair() + auth_url = ( + f"{authorization_endpoint}?" + f"response_type=code&" + f"client_id={KEYCLOAK_CLIENT_ID}&" + f"redirect_uri={quote(callback_url, safe='')}&" + f"state={state}&" + f"scope={quote(DEFAULT_FULL_SCOPES, safe='')}&" + f"code_challenge={challenge}&" + f"code_challenge_method=S256" + ) + + context = await browser.new_context(ignore_https_errors=True) + page = await context.new_page() + try: + await page.goto(auth_url, wait_until="networkidle", timeout=60000) + + # Keycloak login form (native). Field ids are stable across KC 26.x. + await page.wait_for_selector("#username", timeout=15000) + await page.fill("#username", KEYCLOAK_OAUTH_USER) + await page.fill("#password", KEYCLOAK_OAUTH_PASSWORD) + await page.click("#kc-login") + await page.wait_for_load_state("networkidle", timeout=60000) + + start = time.time() + while state not in auth_states: + if time.time() - start > 45: + await page.screenshot(path="/tmp/keycloak_oauth_timeout.png") + raise TimeoutError( + f"Timeout waiting for Keycloak OAuth callback (url={page.url})" + ) + await anyio.sleep(0.5) + auth_code = auth_states[state] + finally: + await context.close() + + async with httpx.AsyncClient(timeout=30.0) as http: + token_resp = await http.post( + token_endpoint, + data={ + "grant_type": "authorization_code", + "code": auth_code, + "redirect_uri": callback_url, + "client_id": KEYCLOAK_CLIENT_ID, + "client_secret": KEYCLOAK_CLIENT_SECRET, + "code_verifier": verifier, + }, + ) + token_resp.raise_for_status() + access_token = token_resp.json()["access_token"] + + logger.info("Obtained Keycloak OAuth token for mcp-keycloak session") + return access_token + + +async def _complete_login_flow_v2_with_email( + browser, login_url: str, email: str, password: str +) -> None: + """Complete Nextcloud Login Flow v2 logging in as a local user via EMAIL. + + Identical to the login_flow helper, but fills the Nextcloud login form's + user field with the *email* address so the resulting app password's stored + loginName is the email (not the UID). This is what creates the divergent + principal path that PR #980 fixes. + """ + login_url = _rewrite_login_flow_url(login_url) + + context = await browser.new_context(ignore_https_errors=True) + page = await context.new_page() + try: + logger.info("Opening Login Flow v2 URL: %s...", login_url[:80]) + await page.goto(login_url, wait_until="networkidle", timeout=60000) + + # Step 1: "Connect to your account" -> click "Log in" (exact match; the + # connect page also renders "Alternative log in using app password"). + login_btn = page.get_by_role("button", name="Log in", exact=True) + try: + await login_btn.wait_for(timeout=10000) + await login_btn.click() + await page.wait_for_load_state("networkidle", timeout=30000) + except Exception: + logger.info("No 'Log in' button - may already be on login/grant page") + + # Step 2: native login form -> fill EMAIL as the user identifier. + user_field = page.locator('input[name="user"]') + if await user_field.count() > 0: + logger.info("Login form detected, logging in via email %s", email) + await user_field.fill(email) + await page.locator('input[name="password"]').fill(password) + await page.get_by_role("button", name="Log in", exact=True).click() + await page.wait_for_load_state("networkidle", timeout=60000) + else: + logger.info("No login form - already logged in via session") + + # Step 3: "Account access" grant page -> "Grant access". + grant_btn = page.get_by_role("button", name="Grant access") + try: + await grant_btn.wait_for(timeout=15000) + await grant_btn.click() + except Exception as e: + logger.warning("No Grant access button: %s", e) + await page.screenshot(path="/tmp/keycloak_login_flow_no_grant.png") + + # Step 4: password confirmation dialog. + confirm_password = page.get_by_role("dialog").get_by_role( + "textbox", name="Password" + ) + try: + await confirm_password.wait_for(timeout=10000) + await confirm_password.fill(password) + confirm_btn = page.get_by_role("dialog").get_by_role( + "button", name="Confirm" + ) + await confirm_btn.wait_for(timeout=5000) + await confirm_btn.click() + except Exception: + logger.info( + "No password confirmation dialog (may have been auto-confirmed)" + ) + + # Step 5: "Account connected" success page. + try: + await page.get_by_text("Account connected").wait_for(timeout=15000) + logger.info("Login Flow v2 completed: Account connected!") + except Exception: + await page.wait_for_load_state("networkidle", timeout=10000) + logger.info("Login Flow v2 done. Final URL: %s", page.url) + finally: + await context.close() + + +@pytest.fixture() +async def nc_mcp_keycloak_email_client( + anyio_backend, + keycloak_service_oauth_token: str, + browser, + divergent_email_user: dict[str, str], +) -> AsyncGenerator[ClientSession, Any]: + """Provisioned ``mcp-keycloak`` session whose app password loginName is an email. + + 1. Connects to mcp-keycloak (8002) with a Keycloak OAuth token. + 2. Calls ``nc_auth_provision_access`` to start Login Flow v2. + 3. Completes the browser login as the local ``divergent_email_user`` **via + its email**, minting an app password whose loginName is the email. + 4. Polls ``nc_auth_check_status`` until provisioned, then yields the session. + """ + email = divergent_email_user["email"] + password = divergent_email_user["password"] + login_url_holder: dict[str, str] = {} + + async def elicitation_callback( + context: Any, params: ElicitRequestParams + ) -> ElicitResult: + for line in params.message.split("\n"): + stripped = line.strip() + if stripped.startswith("http") and "/login/v2/" in stripped: + login_url_holder["url"] = stripped + break + if "url" in login_url_holder: + await _complete_login_flow_v2_with_email( + browser, login_url_holder["url"], email, password + ) + return ElicitResult(action="accept", content={"acknowledged": True}) + + async with create_mcp_client_session( + url=KEYCLOAK_MCP_URL, + token=keycloak_service_oauth_token, + client_name="Keycloak MCP (email login)", + elicitation_callback=elicitation_callback, + ) as session: + provision_result = await session.call_tool( + "nc_auth_provision_access", {"scopes": None} + ) + provision_data = json.loads(provision_result.content[0].text) + logger.info("Provision status: %s", provision_data.get("status")) + + if provision_data.get("status") == "login_required": + login_url = provision_data.get("login_url") + if login_url and "url" not in login_url_holder: + await _complete_login_flow_v2_with_email( + browser, login_url, email, password + ) + + for attempt in range(15): + status_result = await session.call_tool("nc_auth_check_status", {}) + status_data = json.loads(status_result.content[0].text) + status = status_data.get("status") + logger.info("Status %s/15: %s", attempt + 1, status) + if status == "provisioned": + logger.info( + "Provisioned. Stored loginName=%s (expected email=%s)", + status_data.get("username"), + email, + ) + break + if status in ("not_initiated", "error"): + raise RuntimeError( + f"Login Flow v2 failed: {status_data.get('message')}" + ) + await anyio.sleep(2) + else: + raise TimeoutError("Login Flow v2 did not complete after 15 attempts") + + yield session diff --git a/tests/server/keycloak/test_keycloak_dav_principal.py b/tests/server/keycloak/test_keycloak_dav_principal.py new file mode 100644 index 000000000..789edeec0 --- /dev/null +++ b/tests/server/keycloak/test_keycloak_dav_principal.py @@ -0,0 +1,110 @@ +"""Keycloak-service integration test for DAV current-user-principal discovery. + +Reproduces the divergent-principal bug fixed by PR #980 against the +``mcp-keycloak`` service (port 8002), through Login Flow v2 app-password auth. + +The ``divergent_email_user`` logs into Nextcloud Login Flow v2 via its **email**, +so the app password's stored loginName is the email while the canonical UID is +``divprincipal_``. ``context.py`` builds the client with +``username = loginName = ``, so every DAV path is +``/remote.php/dav/files//`` — which is NOT the user's real home dir +(``/remote.php/dav/files//``). + +* **Without PR #980** the WebDAV operations target ``/files//`` and fail + (Nextcloud has no home dir for the email) -> this test is RED. +* **With PR #980** ``BaseNextcloudClient._ensure_principal_id()`` issues a + ``PROPFIND /remote.php/dav/`` for ``current-user-principal``, discovers the + real UID, and rewrites the base path to ``/files//`` -> GREEN. + +This is the keycloak-service counterpart of the existing +``tests/server/login_flow`` WebDAV test, which was missing for the keycloak lane. +""" + +import json +import logging + +import pytest +from mcp import ClientSession + +logger = logging.getLogger(__name__) + +pytestmark = [pytest.mark.integration, pytest.mark.keycloak] + + +async def test_divergence_condition_holds( + nc_mcp_keycloak_email_client: ClientSession, + divergent_email_user: dict[str, str], +): + """Guard: the provisioned app password's loginName is the email, not the UID. + + If this ever stops holding, the WebDAV test below would be a false pass + (username == uid means paths are trivially correct even without the fix). + """ + status_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_auth_check_status", {} + ) + status_data = json.loads(status_result.content[0].text) + + assert status_data.get("status") == "provisioned" + login_name = status_data.get("username") + assert login_name == divergent_email_user["email"], ( + f"Expected loginName to be the email {divergent_email_user['email']!r}, " + f"got {login_name!r}" + ) + assert login_name != divergent_email_user["uid"], ( + "Divergence precondition failed: loginName == UID, so the wrong-path bug " + "cannot be reproduced." + ) + + +async def test_webdav_operations_resolve_divergent_principal( + nc_mcp_keycloak_email_client: ClientSession, + divergent_email_user: dict[str, str], +): + """Full WebDAV cycle succeeds only when the real UID home dir is resolved. + + Without PR #980 the paths are built from the email loginName and every + operation targets a non-existent home dir -> failures (RED). With the fix, + current-user-principal discovery resolves the UID -> success (GREEN). + """ + suffix = divergent_email_user["uid"].split("_")[-1] + dir_path = f"/KeycloakPrincipalTest_{suffix}" + file_path = f"{dir_path}/divergent_principal.txt" + content = f"principal discovery via keycloak service {suffix}" + + mkdir_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_create_directory", {"path": dir_path} + ) + assert mkdir_result.isError is False, ( + "create_directory failed — DAV path likely built from the email " + "loginName instead of the discovered UID (PR #980 not applied?)" + ) + + try: + write_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_write_file", + {"path": file_path, "content": content}, + ) + assert write_result.isError is False + + read_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_read_file", {"path": file_path} + ) + assert read_result.isError is False + read_data = json.loads(read_result.content[0].text) + assert content in read_data.get("content", "") + + list_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_list_directory", {"path": dir_path} + ) + assert list_result.isError is False + list_data = json.loads(list_result.content[0].text) + names = [f.get("name", "") for f in list_data.get("files", [])] + assert "divergent_principal.txt" in names + finally: + await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_delete_resource", {"path": file_path} + ) + await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_delete_resource", {"path": dir_path} + ) From b99516273b011e2cad98ec43857737661cdee0c1 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 1 Jul 2026 21:19:20 +0200 Subject: [PATCH 2/7] fix(login-flow): resolve Login Flow v2 login_url to Nextcloud in external-IdP mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Login Flow v2 login_url origin was rewritten to `NEXTCLOUD_PUBLIC_ISSUER_URL`, which doubles as the OAuth issuer for JWT `iss` validation. In single-IdP (login-flow) mode the issuer IS Nextcloud, so this worked. In external-IdP mode (e.g. Keycloak) the issuer is the IdP, so the browser-facing login URL was rewritten onto the IdP origin (e.g. `http://localhost:8888/login/v2/flow/...`) — which has no `/login/v2` endpoint and 404s, breaking Login Flow v2 for every external-IdP deployment. Add a dedicated `NEXTCLOUD_PUBLIC_URL` (browser-reachable Nextcloud base URL) resolved via a new `Settings.nextcloud_browser_url` property with a backward-compatible fallback chain (`nextcloud_public_url` → `nextcloud_public_issuer_url` → `nextcloud_host`). Login Flow v2 / elicitation rewrites now use it; OAuth issuer / JWT validation stays on `nextcloud_public_issuer_url`. Wire `NEXTCLOUD_PUBLIC_URL=http://localhost:8080` for the `mcp-keycloak` compose service. Existing login-flow deployments that set only `NEXTCLOUD_PUBLIC_ISSUER_URL` are unchanged (the fallback resolves to the same value). Co-Authored-By: Claude Opus 4.8 (1M context) --- docker-compose.yml | 4 ++ docs/configuration.md | 3 +- docs/login-flow-v2.md | 6 ++- nextcloud_mcp_server/auth/elicitation.py | 20 +++++----- nextcloud_mcp_server/auth/provision_routes.py | 16 ++++---- nextcloud_mcp_server/config.py | 39 +++++++++++++++++++ nextcloud_mcp_server/server/auth_tools.py | 6 +-- tests/unit/test_config.py | 31 +++++++++++++++ tests/unit/test_elicitation.py | 26 ++++++++++++- 9 files changed, 126 insertions(+), 25 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index e810d087a..ef6a107e1 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -264,6 +264,10 @@ services: - NEXTCLOUD_MCP_SERVER_URL=http://localhost:8002 - NEXTCLOUD_RESOURCE_URI=nextcloud # ADR-005: Keycloak uses client IDs as audiences, not URLs - NEXTCLOUD_PUBLIC_ISSUER_URL=http://localhost:8888/realms/nextcloud-mcp + # External-IdP mode: the OAuth issuer (above) is Keycloak, but Login Flow + # v2 must send the browser to *Nextcloud*. NEXTCLOUD_HOST is the internal + # Docker hostname, so give the browser-reachable Nextcloud URL explicitly. + - NEXTCLOUD_PUBLIC_URL=http://localhost:8080 # Refresh token storage (ADR-002 Tier 1 & 2). Source from .env. - ENABLE_BACKGROUND_OPERATIONS=true diff --git a/docs/configuration.md b/docs/configuration.md index 46c48de81..cd8eac140 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -116,7 +116,8 @@ NEXTCLOUD_PUBLIC_ISSUER_URL=https://your.nextcloud.instance.com | `TOKEN_ENCRYPTION_KEY` | ✅ Yes | Fernet key for app-password encryption — generate with `python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())"` | | `TOKEN_STORAGE_DB` | ✅ Yes | Path to SQLite DB for stored app passwords (use a persistent volume) | | `NEXTCLOUD_MCP_SERVER_URL` | ✅ Yes | Public URL of the MCP server (used as the audience claim and for browser redirects) | -| `NEXTCLOUD_PUBLIC_ISSUER_URL` | ✅ Yes | Public URL of Nextcloud (for browser redirects during Login Flow v2) | +| `NEXTCLOUD_PUBLIC_ISSUER_URL` | ✅ Yes | Public URL used as the OAuth issuer for JWT validation **and** (by default) the browser-reachable Nextcloud URL for Login Flow v2 redirects. When Nextcloud is its own IdP these coincide. | +| `NEXTCLOUD_PUBLIC_URL` | Optional (required for external IdPs) | Browser-reachable public URL of **Nextcloud** for Login Flow v2 login pages and elicitation links. Only needed when the OAuth issuer is a *separate* IdP (e.g. Keycloak/Cognito): there `NEXTCLOUD_PUBLIC_ISSUER_URL` points at the IdP, so set this to Nextcloud's own URL or the Login Flow v2 login page is built on the IdP origin and 404s. Falls back to `NEXTCLOUD_PUBLIC_ISSUER_URL` then `NEXTCLOUD_HOST` when unset. | | `NEXTCLOUD_OIDC_CLIENT_ID` | ✅ Strongly recommended | OIDC client ID for the MCP server's relying-party registration with the IdP (Nextcloud's built-in OIDC by default; Keycloak / Cognito / etc. via `OIDC_DISCOVERY_URL`). If unset and the IdP advertises a `registration_endpoint`, the server falls back to RFC 7591 Dynamic Client Registration (DCR) — **but with Nextcloud's built-in `oidc` app this fallback breaks after ~1 hour** (see warning below). Create a static client and set this instead. | | `NEXTCLOUD_OIDC_CLIENT_SECRET` | ✅ Strongly recommended | OIDC client secret paired with `NEXTCLOUD_OIDC_CLIENT_ID`. | | `OIDC_DISCOVERY_URL` | Optional | Override the IdP discovery URL. Defaults to `${NEXTCLOUD_HOST}/.well-known/openid-configuration` (Nextcloud's built-in OIDC). Set to a Keycloak realm or AWS Cognito user-pool discovery URL to use an external IdP. | diff --git a/docs/login-flow-v2.md b/docs/login-flow-v2.md index fa7042b96..481b5a1bd 100644 --- a/docs/login-flow-v2.md +++ b/docs/login-flow-v2.md @@ -391,7 +391,11 @@ The MCP server cannot reach Nextcloud at `NEXTCLOUD_HOST`. Verify network connec ### "Login URL points to localhost in browser" -`NEXTCLOUD_PUBLIC_ISSUER_URL` is missing or wrong. Set it to the public URL of Nextcloud as the user's browser sees it. The server rewrites the login URL's origin from the internal `NEXTCLOUD_HOST` to `NEXTCLOUD_PUBLIC_ISSUER_URL` before redirecting the browser. +`NEXTCLOUD_PUBLIC_ISSUER_URL` is missing or wrong. Set it to the public URL of Nextcloud as the user's browser sees it. The server rewrites the login URL's origin from the internal `NEXTCLOUD_HOST` to the browser-reachable Nextcloud URL before redirecting the browser. + +### Login page 404s / lands on the IdP (external IdP, e.g. Keycloak) + +With an **external** IdP, `NEXTCLOUD_PUBLIC_ISSUER_URL` points at the IdP (it doubles as the OAuth issuer for JWT validation), so the Login Flow v2 login URL gets rewritten onto the IdP's origin — which has no `/login/v2` endpoint and 404s. Set **`NEXTCLOUD_PUBLIC_URL`** to Nextcloud's own browser-reachable URL; it takes precedence over `NEXTCLOUD_PUBLIC_ISSUER_URL` for the login-page and elicitation-link rewrites while leaving JWT issuer validation on the IdP. Single-IdP (Nextcloud-is-the-IdP) deployments don't need it — the issuer URL already resolves to Nextcloud. ### Stored app password rejected by Nextcloud (401) diff --git a/nextcloud_mcp_server/auth/elicitation.py b/nextcloud_mcp_server/auth/elicitation.py index 57b59d4c5..67eab11a8 100644 --- a/nextcloud_mcp_server/auth/elicitation.py +++ b/nextcloud_mcp_server/auth/elicitation.py @@ -44,17 +44,16 @@ class ProvisioningRequiredConfirmation(BaseModel): def _astrolabe_settings_url() -> str | None: """Construct the Astrolabe settings page URL from settings. - Prefers ``nextcloud_public_issuer_url`` (the browser-reachable public URL) - over ``nextcloud_host`` (which may be an internal hostname in Docker - deployments). Returns None if neither is set (or set to the empty - string), or if the configured base URL is missing an http:// or - https:// scheme — in the latter case the caller renders the tool-only - fallback message instead of a broken link. + Uses ``nextcloud_browser_url`` (the browser-reachable Nextcloud base URL: + ``nextcloud_public_url`` → ``nextcloud_public_issuer_url`` → ``nextcloud_host``) + so the link points at Nextcloud even in external-IdP deployments where the + OAuth issuer URL is the IdP, not Nextcloud. Returns None if none is set (or + set to the empty string), or if the configured base URL is missing an + http:// or https:// scheme — in the latter case the caller renders the + tool-only fallback message instead of a broken link. """ settings = get_settings() - base = ( - settings.nextcloud_public_issuer_url or settings.nextcloud_host or "" - ).strip() + base = (settings.nextcloud_browser_url or "").strip() if not base: return None if not base.startswith(("http://", "https://")): @@ -182,8 +181,7 @@ async def present_provisioning_required(ctx: Context) -> str: has to translate. The Astrolabe settings URL is reconstructed from - ``settings.nextcloud_public_issuer_url`` / - ``settings.nextcloud_host``; if Astrolabe is not installed the link + ``settings.nextcloud_browser_url``; if Astrolabe is not installed the link 404s and the user falls back to the tool path suggested in the same message. diff --git a/nextcloud_mcp_server/auth/provision_routes.py b/nextcloud_mcp_server/auth/provision_routes.py index 7fa2e92f8..1e1d9d6e5 100644 --- a/nextcloud_mcp_server/auth/provision_routes.py +++ b/nextcloud_mcp_server/auth/provision_routes.py @@ -74,7 +74,7 @@ async def _poll_and_store(provision_id: str) -> None: flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), - public_host=settings.nextcloud_public_issuer_url, + public_host=settings.nextcloud_browser_url, ) poll_endpoint = session["poll_endpoint"] @@ -214,7 +214,7 @@ async def provision_page( flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), - public_host=settings.nextcloud_public_issuer_url, + public_host=settings.nextcloud_browser_url, ) init_response = await flow_client.initiate() except Exception as e: @@ -262,12 +262,14 @@ async def provision_page( # The login_url may use the internal Docker hostname (http://app/...). # Replace with the public Nextcloud URL for the browser. # Note: poll_endpoint is rewritten to NEXTCLOUD_HOST (server-side, in - # LoginFlowV2Client) while login_url is rewritten to the public issuer - # URL here because the browser needs a publicly-reachable address. + # LoginFlowV2Client) while login_url is rewritten to the public *Nextcloud* + # URL here because the browser needs a publicly-reachable address. This must + # use nextcloud_browser_url (not the OAuth issuer URL): in external-IdP mode + # the issuer is the IdP, which has no Login Flow v2 endpoint. login_url = init_response.login_url - public_issuer = settings.nextcloud_public_issuer_url or "" - if public_issuer and nextcloud_host: - login_url = rewrite_url_origin(login_url, public_issuer.rstrip("/")) + public_browser_url = settings.nextcloud_browser_url or "" + if public_browser_url and nextcloud_host: + login_url = rewrite_url_origin(login_url, public_browser_url.rstrip("/")) return RedirectResponse(login_url) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 7d75e87d7..c1b6b7070 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -35,6 +35,7 @@ "nextcloud_mcp_server_url": None, "nextcloud_resource_uri": None, "nextcloud_public_issuer_url": None, + "nextcloud_public_url": None, "cookie_secure": None, # OAuth/OIDC "oidc_discovery_url": None, @@ -734,8 +735,25 @@ class Settings: # Browser-reachable public URL for OAuth/Login-Flow-v2 redirects when # NEXTCLOUD_HOST is an internal Docker hostname. Falls back to # nextcloud_host when unset. + # + # NOTE: this doubles as the OAuth *issuer* URL used for JWT ``iss`` + # validation. In external-IdP mode (e.g. Keycloak) the issuer is the IdP, + # NOT Nextcloud — so this value points at the IdP, not the browser-reachable + # Nextcloud host. Use ``nextcloud_public_url`` / ``nextcloud_browser_url`` + # for anything that must resolve to Nextcloud itself (Login Flow v2 login + # URLs, elicitation links). nextcloud_public_issuer_url: str | None = None + # Browser-reachable public URL of the *Nextcloud* instance, used to rewrite + # Login Flow v2 login URLs and elicitation links when NEXTCLOUD_HOST is an + # internal Docker hostname. Distinct from ``nextcloud_public_issuer_url`` + # because, in external-IdP (Keycloak/OIDC) deployments, the OAuth issuer is + # the IdP while Login Flow v2 must still point the browser at Nextcloud. + # Falls back to ``nextcloud_public_issuer_url`` then ``nextcloud_host`` (see + # ``nextcloud_browser_url``) so single-IdP (login-flow) deployments that set + # only NEXTCLOUD_PUBLIC_ISSUER_URL keep working unchanged. + nextcloud_public_url: str | None = None + # Browser cookie Secure flag. None = auto-detect from nextcloud_host # scheme (https → True, else False). Set COOKIE_SECURE=true/false to # override. @@ -1006,6 +1024,26 @@ class Settings: # control plane to pull. See nextcloud_mcp_server/usage/store.py. usage_metering_enabled: bool = False + @property + def nextcloud_browser_url(self) -> str | None: + """Browser-reachable base URL of the Nextcloud instance. + + Resolves the URL the *user's browser* must use to reach Nextcloud for + Login Flow v2 login pages and elicitation links. Prefers the dedicated + ``nextcloud_public_url``; falls back to ``nextcloud_public_issuer_url`` + (correct in single-IdP / login-flow deployments where the OAuth issuer + IS Nextcloud) and finally the internal ``nextcloud_host``. + + In external-IdP mode (e.g. Keycloak) set ``NEXTCLOUD_PUBLIC_URL`` so this + does not fall back to the IdP issuer URL, which would send the browser to + the IdP instead of Nextcloud. + """ + return ( + self.nextcloud_public_url + or self.nextcloud_public_issuer_url + or self.nextcloud_host + ) + def __post_init__(self): """Validate configuration and set defaults.""" @@ -1588,6 +1626,7 @@ def get_settings() -> Settings: "nextcloud_password": "NEXTCLOUD_PASSWORD", "nextcloud_app_password": "NEXTCLOUD_APP_PASSWORD", "nextcloud_public_issuer_url": "NEXTCLOUD_PUBLIC_ISSUER_URL", + "nextcloud_public_url": "NEXTCLOUD_PUBLIC_URL", "cookie_secure": "COOKIE_SECURE", # Nextcloud SSL/TLS settings "nextcloud_verify_ssl": "NEXTCLOUD_VERIFY_SSL", diff --git a/nextcloud_mcp_server/server/auth_tools.py b/nextcloud_mcp_server/server/auth_tools.py index c9ce59b41..514a450d9 100644 --- a/nextcloud_mcp_server/server/auth_tools.py +++ b/nextcloud_mcp_server/server/auth_tools.py @@ -113,7 +113,7 @@ async def nc_auth_provision_access( flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), - public_host=settings.nextcloud_public_issuer_url, + public_host=settings.nextcloud_browser_url, ) init_response = await flow_client.initiate() except Exception as e: @@ -259,7 +259,7 @@ async def nc_auth_check_status( flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), - public_host=settings.nextcloud_public_issuer_url, + public_host=settings.nextcloud_browser_url, ) poll_result = await flow_client.poll( poll_endpoint=session["poll_endpoint"], @@ -441,7 +441,7 @@ async def nc_auth_update_scopes( flow_client = LoginFlowV2Client( nextcloud_host=nextcloud_host, verify_ssl=get_nextcloud_ssl_verify(), - public_host=settings.nextcloud_public_issuer_url, + public_host=settings.nextcloud_browser_url, ) init_response = await flow_client.initiate() except Exception as e: diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 1741ba9f8..7efea0b2b 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -630,3 +630,34 @@ def test_valid_log_format_json(self): _reload_config() settings = get_settings() assert settings.log_format == "json" + + +class TestNextcloudBrowserUrl: + """Test the ``nextcloud_browser_url`` resolver property (Login Flow v2 rewrite).""" + + def test_prefers_public_url(self): + """nextcloud_public_url wins — the external-IdP (Keycloak) case.""" + settings = Settings( + nextcloud_public_url="https://nc.example.com", + nextcloud_public_issuer_url="https://keycloak.example.com/realms/x", + nextcloud_host="http://app:80", + ) + assert settings.nextcloud_browser_url == "https://nc.example.com" + + def test_falls_back_to_public_issuer(self): + """Without public_url, the OAuth issuer URL is used (single-IdP case).""" + settings = Settings( + nextcloud_public_issuer_url="https://nc.example.com", + nextcloud_host="http://app:80", + ) + assert settings.nextcloud_browser_url == "https://nc.example.com" + + def test_falls_back_to_host(self): + """With neither public URL set, the internal host is used.""" + settings = Settings(nextcloud_host="http://app:80") + assert settings.nextcloud_browser_url == "http://app:80" + + def test_none_when_nothing_set(self): + """Returns None when no Nextcloud URL is configured at all.""" + settings = Settings() + assert settings.nextcloud_browser_url is None diff --git a/tests/unit/test_elicitation.py b/tests/unit/test_elicitation.py index 4183ebb8c..905b5ba3a 100644 --- a/tests/unit/test_elicitation.py +++ b/tests/unit/test_elicitation.py @@ -15,13 +15,35 @@ def _fake_settings( - public_issuer_url: str | None = None, host: str | None = None + public_issuer_url: str | None = None, + host: str | None = None, + public_url: str | None = None, ) -> SimpleNamespace: - """Build a Settings-shaped object exposing only the fields elicitation reads.""" + """Build a Settings-shaped object exposing only the fields elicitation reads. + + ``nextcloud_browser_url`` mirrors the real ``Settings`` property's fallback + chain (public_url → public_issuer_url → host). + """ return SimpleNamespace( + nextcloud_public_url=public_url, nextcloud_public_issuer_url=public_issuer_url, nextcloud_host=host, + nextcloud_browser_url=public_url or public_issuer_url or host, + ) + + +def test_astrolabe_settings_url_prefers_public_url(): + """nextcloud_public_url wins over the OAuth issuer URL (external-IdP mode).""" + fake = _fake_settings( + public_url="https://nc.example.com", + public_issuer_url="https://keycloak.example.com/realms/x", + host="http://internal:8080", ) + with patch("nextcloud_mcp_server.auth.elicitation.get_settings", return_value=fake): + assert ( + _astrolabe_settings_url() + == f"https://nc.example.com{ASTROLABE_SETTINGS_PATH}" + ) def test_astrolabe_settings_url_prefers_public_issuer(): From 3b63b5179500ee4dd940d6e42f8d7083babf0dc4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 1 Jul 2026 21:19:40 +0200 Subject: [PATCH 3/7] test(keycloak): rework Login Flow v2 lane as end-to-end WebDAV coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//` 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) --- tests/server/keycloak/conftest.py | 170 ++++++++---------- .../keycloak/test_keycloak_dav_principal.py | 110 ------------ .../test_keycloak_login_flow_webdav.py | 116 ++++++++++++ 3 files changed, 194 insertions(+), 202 deletions(-) delete mode 100644 tests/server/keycloak/test_keycloak_dav_principal.py create mode 100644 tests/server/keycloak/test_keycloak_login_flow_webdav.py diff --git a/tests/server/keycloak/conftest.py b/tests/server/keycloak/conftest.py index becd35090..8b69ba957 100644 --- a/tests/server/keycloak/conftest.py +++ b/tests/server/keycloak/conftest.py @@ -3,37 +3,40 @@ The ``mcp-keycloak`` service uses Keycloak as the external OAuth IdP but reaches Nextcloud via Login Flow v2 **app passwords** (``MCP_DEPLOYMENT_MODE=login_flow``), exactly like the ``mcp-login-flow`` service — the only difference is the OAuth -IdP (Keycloak vs Nextcloud's built-in ``oidc`` app). The keycloak lane currently -only has DCR/authorize tests; the Login Flow v2 app-password integration tests -are missing. - -These fixtures fill that gap AND set up the divergent-principal condition fixed -by PR #980: - -* **OAuth leg** — a Keycloak auth-code flow (browser) obtains an access token - that the ``mcp-keycloak`` session accepts. This exercises the keycloak service. +IdP (Keycloak vs Nextcloud's built-in ``oidc`` app). The keycloak lane previously +only had DCR/authorize tests; it never provisioned a Login Flow v2 app password or +issued a real Nextcloud API call. These fixtures fill that gap end-to-end: + +* **OAuth leg** — a Keycloak direct-grant (ROPC) obtains an access token that the + ``mcp-keycloak`` session accepts. This exercises the keycloak service without + driving Keycloak's browser login form (its identity is irrelevant here — the + Login Flow v2 app password is what authenticates DAV requests). * **Login Flow v2 leg** — the browser completes Nextcloud Login Flow v2 by logging in as a *local* Nextcloud user using its **email address**. Nextcloud keys the resulting app password on the *loginName* (the email), which differs from the - user's canonical UID. ``context.py`` then builds DAV paths from the email - (``/remote.php/dav/files//``) instead of the UID — the exact wrong-path - bug PR #980 fixes via ``current-user-principal`` discovery. - -A plain Keycloak/user_oidc login can NOT reproduce this: ``user_oidc``'s -``LoginController`` sets ``loginName == UID`` (the sha256 hash), so its DAV paths -are already correct. Login-by-email of a local user is the reliable divergence -generator (and matches PR #980's own ``alice@example.com`` unit tests). + user's canonical UID (loginName != UID). + +Getting these fixtures to provision at all is the regression guard for +``NEXTCLOUD_PUBLIC_URL``: in external-IdP mode the OAuth issuer URL is Keycloak, +so without a dedicated browser-reachable Nextcloud URL the Login Flow v2 +``login_url`` is rewritten to Keycloak's origin and 404s. + +Relation to PR #980: the login-by-email leg produces the same ``loginName != UID`` +identity shape as #980's client fix, but it does NOT reproduce #980's wrong-path +failure on the CI Nextcloud versions — Nextcloud resolves +``/remote.php/dav/files//`` to the user's real home (email is a valid path +alias), so the WebDAV round-trip succeeds with or without the principal-discovery +fix. Neither can a plain Keycloak/``user_oidc`` login: ``user_oidc``'s +``LoginController`` hardcodes ``loginName == UID`` (the sha256 hash), so its DAV +paths are already correct. #980's failure mode needs a backend (e.g. LDAP) where +the loginName is not a valid files-path alias; it is covered by #980's own mocked +unit tests. """ -import base64 -import hashlib import json import logging -import secrets -import time import uuid from typing import Any, AsyncGenerator -from urllib.parse import quote import anyio import httpx @@ -43,7 +46,6 @@ from nextcloud_mcp_server.client import NextcloudClient from tests.conftest import ( - DEFAULT_FULL_SCOPES, create_mcp_client_session, ) from tests.server.login_flow.conftest import _rewrite_login_flow_url @@ -64,12 +66,32 @@ # Keycloak user used only for the OAuth leg (session identity key). It does not # have to match the Nextcloud data user — the app password minted by the Login -# Flow leg is what authenticates DAV requests. +# Flow leg is what authenticates DAV requests. Direct Access Grants (ROPC) are +# enabled for the `nextcloud-mcp-server` client in realm-export.json. KEYCLOAK_OAUTH_USER = "admin" KEYCLOAK_OAUTH_PASSWORD = "admin" +# Scopes registered on the Keycloak `nextcloud-mcp-server` client +# (realm-export.json optionalClientScopes). This deliberately EXCLUDES +# ``talk.read``/``talk.write``: those are part of ``DEFAULT_FULL_SCOPES`` but are +# NOT registered on the Keycloak client, so requesting them makes Keycloak reject +# the whole token request with ``invalid_scope``. ``files.read``/``files.write`` +# are what the WebDAV reproduction test actually needs. +KEYCLOAK_SUPPORTED_SCOPES = ( + "openid profile email " + "notes.read notes.write " + "calendar.read calendar.write " + "todo.read todo.write " + "contacts.read contacts.write " + "cookbook.read cookbook.write " + "deck.read deck.write " + "tables.read tables.write " + "files.read files.write " + "sharing.read sharing.write" +) -@pytest.fixture() + +@pytest.fixture(scope="session") async def divergent_email_user( anyio_backend, nc_client: NextcloudClient ) -> AsyncGenerator[dict[str, str], Any]: @@ -79,6 +101,13 @@ async def divergent_email_user( The user is deleted on teardown. Nextcloud login-by-email is enabled by default, so logging in with the email during Login Flow v2 produces an app password whose stored loginName is the email — not the UID. + + Session-scoped: the ``mcp-keycloak`` app-password store is keyed by the + Keycloak OAuth identity (a single shared ``admin``), so all tests share one + provisioned app password. The divergent user must therefore stay alive for + the whole session — a per-test user would be deleted while its app password + is still cached server-side, turning the WebDAV reproduction into a spurious + 401-on-deleted-user instead of the #980 wrong-path failure. """ suffix = uuid.uuid4().hex[:8] uid = f"divprincipal_{suffix}" @@ -107,26 +136,18 @@ async def divergent_email_user( logger.warning("Failed to delete divergent-principal user %s: %s", uid, e) -def _pkce_pair() -> tuple[str, str]: - """Return (code_verifier, code_challenge) for a PKCE S256 exchange.""" - verifier = secrets.token_urlsafe(64) - digest = hashlib.sha256(verifier.encode()).digest() - challenge = base64.urlsafe_b64encode(digest).rstrip(b"=").decode() - return verifier, challenge - - -@pytest.fixture() -async def keycloak_service_oauth_token( - anyio_backend, browser, oauth_callback_server -) -> str: +@pytest.fixture(scope="session") +async def keycloak_service_oauth_token(anyio_backend) -> str: """Obtain a Keycloak access token accepted by the ``mcp-keycloak`` session. - Drives the OAuth auth-code flow (with PKCE) against Keycloak using the - static ``nextcloud-mcp-server`` client and the test OAuth callback server. - Logs into Keycloak (not Nextcloud) via its native login form. + Uses the OAuth 2.0 Resource Owner Password Credentials (direct access) + grant against the static ``nextcloud-mcp-server`` client. The OAuth leg's + identity is irrelevant to the reproduction — the Login Flow v2 app password + is what authenticates DAV requests — so there is no need to drive Keycloak's + browser login form here. Direct grant is faster and avoids the flakiness of + the auth-code + Playwright flow (whose native ``#username`` login page also + breaks when the request carries scopes the client does not know about). """ - auth_states, callback_url = oauth_callback_server - async with httpx.AsyncClient(timeout=30.0) as http: discovery = await http.get( f"{KEYCLOAK_BASE_URL}/realms/{KEYCLOAK_REALM}" @@ -136,64 +157,23 @@ async def keycloak_service_oauth_token( discovery.raise_for_status() except httpx.HTTPStatusError as e: pytest.skip(f"Keycloak realm not available: {e}") - oidc = discovery.json() - - authorization_endpoint = oidc["authorization_endpoint"] - token_endpoint = oidc["token_endpoint"] - - state = secrets.token_urlsafe(32) - verifier, challenge = _pkce_pair() - auth_url = ( - f"{authorization_endpoint}?" - f"response_type=code&" - f"client_id={KEYCLOAK_CLIENT_ID}&" - f"redirect_uri={quote(callback_url, safe='')}&" - f"state={state}&" - f"scope={quote(DEFAULT_FULL_SCOPES, safe='')}&" - f"code_challenge={challenge}&" - f"code_challenge_method=S256" - ) + token_endpoint = discovery.json()["token_endpoint"] - context = await browser.new_context(ignore_https_errors=True) - page = await context.new_page() - try: - await page.goto(auth_url, wait_until="networkidle", timeout=60000) - - # Keycloak login form (native). Field ids are stable across KC 26.x. - await page.wait_for_selector("#username", timeout=15000) - await page.fill("#username", KEYCLOAK_OAUTH_USER) - await page.fill("#password", KEYCLOAK_OAUTH_PASSWORD) - await page.click("#kc-login") - await page.wait_for_load_state("networkidle", timeout=60000) - - start = time.time() - while state not in auth_states: - if time.time() - start > 45: - await page.screenshot(path="/tmp/keycloak_oauth_timeout.png") - raise TimeoutError( - f"Timeout waiting for Keycloak OAuth callback (url={page.url})" - ) - await anyio.sleep(0.5) - auth_code = auth_states[state] - finally: - await context.close() - - async with httpx.AsyncClient(timeout=30.0) as http: token_resp = await http.post( token_endpoint, data={ - "grant_type": "authorization_code", - "code": auth_code, - "redirect_uri": callback_url, + "grant_type": "password", "client_id": KEYCLOAK_CLIENT_ID, "client_secret": KEYCLOAK_CLIENT_SECRET, - "code_verifier": verifier, + "username": KEYCLOAK_OAUTH_USER, + "password": KEYCLOAK_OAUTH_PASSWORD, + "scope": KEYCLOAK_SUPPORTED_SCOPES, }, ) token_resp.raise_for_status() access_token = token_resp.json()["access_token"] - logger.info("Obtained Keycloak OAuth token for mcp-keycloak session") + logger.info("Obtained Keycloak OAuth token (direct grant) for mcp-keycloak session") return access_token @@ -204,8 +184,8 @@ async def _complete_login_flow_v2_with_email( Identical to the login_flow helper, but fills the Nextcloud login form's user field with the *email* address so the resulting app password's stored - loginName is the email (not the UID). This is what creates the divergent - principal path that PR #980 fixes. + loginName is the email (not the UID) — the ``loginName != UID`` identity + shape exercised by this lane. """ login_url = _rewrite_login_flow_url(login_url) @@ -273,7 +253,7 @@ async def _complete_login_flow_v2_with_email( await context.close() -@pytest.fixture() +@pytest.fixture(scope="session") async def nc_mcp_keycloak_email_client( anyio_backend, keycloak_service_oauth_token: str, @@ -282,6 +262,12 @@ async def nc_mcp_keycloak_email_client( ) -> AsyncGenerator[ClientSession, Any]: """Provisioned ``mcp-keycloak`` session whose app password loginName is an email. + Session-scoped so a single Login Flow v2 provisioning (one browser login) + serves every test in the keycloak lane. This is both faster and correct: + the app password is keyed by the shared Keycloak ``admin`` identity, so + re-provisioning per test would just hit ``already_provisioned`` and reuse the + same stored password anyway (see ``divergent_email_user``). + 1. Connects to mcp-keycloak (8002) with a Keycloak OAuth token. 2. Calls ``nc_auth_provision_access`` to start Login Flow v2. 3. Completes the browser login as the local ``divergent_email_user`` **via diff --git a/tests/server/keycloak/test_keycloak_dav_principal.py b/tests/server/keycloak/test_keycloak_dav_principal.py deleted file mode 100644 index 789edeec0..000000000 --- a/tests/server/keycloak/test_keycloak_dav_principal.py +++ /dev/null @@ -1,110 +0,0 @@ -"""Keycloak-service integration test for DAV current-user-principal discovery. - -Reproduces the divergent-principal bug fixed by PR #980 against the -``mcp-keycloak`` service (port 8002), through Login Flow v2 app-password auth. - -The ``divergent_email_user`` logs into Nextcloud Login Flow v2 via its **email**, -so the app password's stored loginName is the email while the canonical UID is -``divprincipal_``. ``context.py`` builds the client with -``username = loginName = ``, so every DAV path is -``/remote.php/dav/files//`` — which is NOT the user's real home dir -(``/remote.php/dav/files//``). - -* **Without PR #980** the WebDAV operations target ``/files//`` and fail - (Nextcloud has no home dir for the email) -> this test is RED. -* **With PR #980** ``BaseNextcloudClient._ensure_principal_id()`` issues a - ``PROPFIND /remote.php/dav/`` for ``current-user-principal``, discovers the - real UID, and rewrites the base path to ``/files//`` -> GREEN. - -This is the keycloak-service counterpart of the existing -``tests/server/login_flow`` WebDAV test, which was missing for the keycloak lane. -""" - -import json -import logging - -import pytest -from mcp import ClientSession - -logger = logging.getLogger(__name__) - -pytestmark = [pytest.mark.integration, pytest.mark.keycloak] - - -async def test_divergence_condition_holds( - nc_mcp_keycloak_email_client: ClientSession, - divergent_email_user: dict[str, str], -): - """Guard: the provisioned app password's loginName is the email, not the UID. - - If this ever stops holding, the WebDAV test below would be a false pass - (username == uid means paths are trivially correct even without the fix). - """ - status_result = await nc_mcp_keycloak_email_client.call_tool( - "nc_auth_check_status", {} - ) - status_data = json.loads(status_result.content[0].text) - - assert status_data.get("status") == "provisioned" - login_name = status_data.get("username") - assert login_name == divergent_email_user["email"], ( - f"Expected loginName to be the email {divergent_email_user['email']!r}, " - f"got {login_name!r}" - ) - assert login_name != divergent_email_user["uid"], ( - "Divergence precondition failed: loginName == UID, so the wrong-path bug " - "cannot be reproduced." - ) - - -async def test_webdav_operations_resolve_divergent_principal( - nc_mcp_keycloak_email_client: ClientSession, - divergent_email_user: dict[str, str], -): - """Full WebDAV cycle succeeds only when the real UID home dir is resolved. - - Without PR #980 the paths are built from the email loginName and every - operation targets a non-existent home dir -> failures (RED). With the fix, - current-user-principal discovery resolves the UID -> success (GREEN). - """ - suffix = divergent_email_user["uid"].split("_")[-1] - dir_path = f"/KeycloakPrincipalTest_{suffix}" - file_path = f"{dir_path}/divergent_principal.txt" - content = f"principal discovery via keycloak service {suffix}" - - mkdir_result = await nc_mcp_keycloak_email_client.call_tool( - "nc_webdav_create_directory", {"path": dir_path} - ) - assert mkdir_result.isError is False, ( - "create_directory failed — DAV path likely built from the email " - "loginName instead of the discovered UID (PR #980 not applied?)" - ) - - try: - write_result = await nc_mcp_keycloak_email_client.call_tool( - "nc_webdav_write_file", - {"path": file_path, "content": content}, - ) - assert write_result.isError is False - - read_result = await nc_mcp_keycloak_email_client.call_tool( - "nc_webdav_read_file", {"path": file_path} - ) - assert read_result.isError is False - read_data = json.loads(read_result.content[0].text) - assert content in read_data.get("content", "") - - list_result = await nc_mcp_keycloak_email_client.call_tool( - "nc_webdav_list_directory", {"path": dir_path} - ) - assert list_result.isError is False - list_data = json.loads(list_result.content[0].text) - names = [f.get("name", "") for f in list_data.get("files", [])] - assert "divergent_principal.txt" in names - finally: - await nc_mcp_keycloak_email_client.call_tool( - "nc_webdav_delete_resource", {"path": file_path} - ) - await nc_mcp_keycloak_email_client.call_tool( - "nc_webdav_delete_resource", {"path": dir_path} - ) diff --git a/tests/server/keycloak/test_keycloak_login_flow_webdav.py b/tests/server/keycloak/test_keycloak_login_flow_webdav.py new file mode 100644 index 000000000..5fe9a7c21 --- /dev/null +++ b/tests/server/keycloak/test_keycloak_login_flow_webdav.py @@ -0,0 +1,116 @@ +"""End-to-end WebDAV coverage for the ``mcp-keycloak`` service (port 8002). + +The keycloak lane previously only covered DCR / authorize — it never provisioned +a Login Flow v2 app password or exercised a real Nextcloud API call. These tests +close that gap: they drive a full Keycloak-fronted Login Flow v2 provisioning and +then run a WebDAV round-trip through the resulting app password. + +``mcp-keycloak`` uses Keycloak as the external OAuth IdP but reaches Nextcloud via +Login Flow v2 app passwords (``MCP_DEPLOYMENT_MODE=login_flow``), exactly like +``mcp-login-flow``; only the OAuth IdP differs. Getting these tests to pass +requires the Login Flow v2 ``login_url`` to point the browser at *Nextcloud* — so +they are also the regression guard for ``NEXTCLOUD_PUBLIC_URL`` (in external-IdP +mode the OAuth issuer URL is Keycloak, not Nextcloud; without the dedicated +public-URL setting the login page 404s on Keycloak). + +The fixtures log into Nextcloud Login Flow v2 as a *local* user via its **email**, +so the app password's stored ``loginName`` is the email while the canonical UID is +``divprincipal_`` (loginName != UID). This exercises the same +identity-divergence shape as PR #980's client fix. Note: it does not reproduce +#980's wrong-path failure on the CI Nextcloud versions — Nextcloud resolves +``/remote.php/dav/files//`` to the user's real home, so the round-trip +succeeds regardless of 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 is covered by #980's own mocked unit tests. +""" + +import json +import logging + +import pytest +from mcp import ClientSession + +logger = logging.getLogger(__name__) + +pytestmark = [pytest.mark.integration, pytest.mark.keycloak] + + +async def test_login_flow_stores_email_login_name( + nc_mcp_keycloak_email_client: ClientSession, + divergent_email_user: dict[str, str], +): + """Login Flow v2 via email stores the email as the app password loginName. + + Confirms the Keycloak-fronted Login Flow v2 provisioning completed and that + logging in by email yields a stored loginName that differs from the canonical + UID (loginName != UID) — the identity shape PR #980 targets. + """ + status_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_auth_check_status", {} + ) + status_data = json.loads(status_result.content[0].text) + + assert status_data.get("status") == "provisioned" + login_name = status_data.get("username") + assert login_name == divergent_email_user["email"], ( + f"Expected loginName to be the email {divergent_email_user['email']!r}, " + f"got {login_name!r}" + ) + assert login_name != divergent_email_user["uid"], ( + "Expected loginName (email) to differ from the canonical UID." + ) + + +async def test_webdav_round_trip_via_keycloak_login_flow( + nc_mcp_keycloak_email_client: ClientSession, + divergent_email_user: dict[str, str], +): + """A full WebDAV cycle works end-to-end through the keycloak service. + + Exercises create/write/read/list/delete against Nextcloud using the app + password minted by Keycloak-fronted Login Flow v2. This is the keycloak-lane + counterpart of the existing ``tests/server/login_flow`` WebDAV coverage, and + it doubles as the regression guard for ``NEXTCLOUD_PUBLIC_URL`` — if the + Login Flow v2 ``login_url`` were rewritten to the Keycloak origin again, the + session fixture could not provision and this test would never run. + """ + suffix = divergent_email_user["uid"].split("_")[-1] + dir_path = f"/KeycloakLoginFlowTest_{suffix}" + file_path = f"{dir_path}/keycloak_login_flow.txt" + content = f"webdav round-trip via keycloak service {suffix}" + + mkdir_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_create_directory", {"path": dir_path} + ) + assert mkdir_result.isError is False, ( + "create_directory failed — Keycloak Login Flow v2 WebDAV path is broken" + ) + + try: + write_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_write_file", + {"path": file_path, "content": content}, + ) + assert write_result.isError is False + + read_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_read_file", {"path": file_path} + ) + assert read_result.isError is False + read_data = json.loads(read_result.content[0].text) + assert content in read_data.get("content", "") + + list_result = await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_list_directory", {"path": dir_path} + ) + assert list_result.isError is False + list_data = json.loads(list_result.content[0].text) + names = [f.get("name", "") for f in list_data.get("files", [])] + assert "keycloak_login_flow.txt" in names + finally: + await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_delete_resource", {"path": file_path} + ) + await nc_mcp_keycloak_email_client.call_tool( + "nc_webdav_delete_resource", {"path": dir_path} + ) From 8061a38e47a0cbed49e9826957af6b0be0bc6011 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 1 Jul 2026 22:26:21 +0200 Subject: [PATCH 4/7] fix(login-flow): use nextcloud_browser_url for userinfo app links Round-1 review finding: userinfo_routes.py built browser-facing Nextcloud app links (viz tab) from `nextcloud_public_issuer_url or nextcloud_host`, the same pattern migrated elsewhere in this PR. In external-IdP mode the issuer URL is the IdP, so those links would point at the IdP origin instead of Nextcloud. Switch to `settings.nextcloud_browser_url` for parity with the elicitation / provision / auth_tools call sites. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/auth/userinfo_routes.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/auth/userinfo_routes.py b/nextcloud_mcp_server/auth/userinfo_routes.py index 276ede124..706a622ae 100644 --- a/nextcloud_mcp_server/auth/userinfo_routes.py +++ b/nextcloud_mcp_server/auth/userinfo_routes.py @@ -482,13 +482,12 @@ async def user_info_html(request: Request) -> HTMLResponse: str(request.url_for("oauth_logout")) if oauth_ctx else "/oauth/logout" ) - # Get Nextcloud host for generating links to apps (used by viz tab) - # Use public issuer URL if available (for browser-accessible links), - # otherwise fall back to NEXTCLOUD_HOST from settings + # Get Nextcloud host for generating browser-accessible links to apps (viz + # tab). Use nextcloud_browser_url so the links point at Nextcloud even in + # external-IdP mode, where nextcloud_public_issuer_url is the IdP, not + # Nextcloud (falls back to public_issuer_url → nextcloud_host). settings = get_settings() - nextcloud_host_for_links = ( - settings.nextcloud_public_issuer_url or settings.nextcloud_host - ) + nextcloud_host_for_links = settings.nextcloud_browser_url # Build host info HTML (BasicAuth only) host_info_html = "" From 2a190730a5782055efb0a018d2f9cca547eb9c84 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 1 Jul 2026 22:30:48 +0200 Subject: [PATCH 5/7] docs(elicitation): update stale module comment to nextcloud_browser_url Round-2 nit: the module-level comment above ASTROLABE_SETTINGS_PATH still referenced nextcloud_public_issuer_url / nextcloud_host; the code (and the _astrolabe_settings_url docstring just below) now uses nextcloud_browser_url. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/auth/elicitation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/auth/elicitation.py b/nextcloud_mcp_server/auth/elicitation.py index 67eab11a8..03c2590ab 100644 --- a/nextcloud_mcp_server/auth/elicitation.py +++ b/nextcloud_mcp_server/auth/elicitation.py @@ -15,8 +15,8 @@ logger = logging.getLogger(__name__) # Path of the Astrolabe Nextcloud app's settings UI. The full URL is -# reconstructed at elicitation time from settings.nextcloud_public_issuer_url -# / settings.nextcloud_host so the user gets a browser-reachable link without +# reconstructed at elicitation time from settings.nextcloud_browser_url (the +# browser-reachable Nextcloud base URL) so the user gets a working link without # needing a separate config knob. If the Astrolabe app is not installed this # path will 404, and the user falls back to the nc_auth_provision_access tool # path mentioned in the same message. From 59f2aa7dc0af87782edc56d2c631f3968070951f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 1 Jul 2026 22:34:18 +0200 Subject: [PATCH 6/7] test(keycloak): mark dev-only credential literals with NOSONAR SonarCloud's quality gate flagged the hardcoded Keycloak client secret, bootstrap password, and ephemeral test-user password in conftest.py (Security Rating D on new code, rule S2068). These are dev-only values that mirror keycloak/realm-export.json + docker-compose.yml (not real secrets). Add the repo's established `# NOSONAR(S2068)` markers, matching the pattern in tests/server/login_flow/test_app_password_loginname_mismatch.py. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/server/keycloak/conftest.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/server/keycloak/conftest.py b/tests/server/keycloak/conftest.py index 8b69ba957..34dd86486 100644 --- a/tests/server/keycloak/conftest.py +++ b/tests/server/keycloak/conftest.py @@ -62,14 +62,16 @@ # mappers for both `nextcloud-mcp-server` (MCP validation) and `nextcloud` # (user_oidc validation). KEYCLOAK_CLIENT_ID = "nextcloud-mcp-server" -KEYCLOAK_CLIENT_SECRET = "mcp-secret-change-in-production" +# Dev-only value mirrored from keycloak/realm-export.json + docker-compose.yml, +# not a real secret. NOSONAR suppresses the hardcoded-credentials hotspot. +KEYCLOAK_CLIENT_SECRET = "mcp-secret-change-in-production" # NOSONAR(S2068) # Keycloak user used only for the OAuth leg (session identity key). It does not # have to match the Nextcloud data user — the app password minted by the Login # Flow leg is what authenticates DAV requests. Direct Access Grants (ROPC) are # enabled for the `nextcloud-mcp-server` client in realm-export.json. KEYCLOAK_OAUTH_USER = "admin" -KEYCLOAK_OAUTH_PASSWORD = "admin" +KEYCLOAK_OAUTH_PASSWORD = "admin" # NOSONAR(S2068) - dev-only Keycloak bootstrap creds # Scopes registered on the Keycloak `nextcloud-mcp-server` client # (realm-export.json optionalClientScopes). This deliberately EXCLUDES @@ -114,7 +116,7 @@ async def divergent_email_user( user = { "uid": uid, "email": f"{uid}@example.com", - "password": "DivergentPrincipalPass123!", + "password": "DivergentPrincipalPass123!", # NOSONAR(S2068) - ephemeral test user "display_name": f"Divergent Principal {suffix}", } From d2eab9db7c78fdf2c5e681b2f2dc589bfd9f4bb5 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 1 Jul 2026 22:39:57 +0200 Subject: [PATCH 7/7] test: clear SonarCloud new-code security findings (S5443, S5332) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Security Rating D gate was driven by the actual issues (not the credential literals the reviewer speculated about): - S5443 (CRITICAL) conftest.py — hardcoded /tmp screenshot path. Kept (CI uploads /tmp/*.png as debug artifacts) but hoisted to its own line with a # NOSONAR(S5443) marker so the suppression anchors to the flagged literal. - S5332 (MINOR) test_config.py / test_elicitation.py — http:// URLs in the new nextcloud_browser_url fallback-chain assertions. Switched to https:// (the scheme is irrelevant to what those unit tests assert). Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/server/keycloak/conftest.py | 5 ++++- tests/unit/test_config.py | 8 ++++---- tests/unit/test_elicitation.py | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/server/keycloak/conftest.py b/tests/server/keycloak/conftest.py index 34dd86486..bd37716d1 100644 --- a/tests/server/keycloak/conftest.py +++ b/tests/server/keycloak/conftest.py @@ -225,7 +225,10 @@ async def _complete_login_flow_v2_with_email( await grant_btn.click() except Exception as e: logger.warning("No Grant access button: %s", e) - await page.screenshot(path="/tmp/keycloak_login_flow_no_grant.png") + # Debug artifact uploaded by CI on failure (test.yml collects + # /tmp/*.png). NOSONAR suppresses the world-writable-dir hotspot. + shot_path = "/tmp/keycloak_login_flow_no_grant.png" # NOSONAR(S5443) + await page.screenshot(path=shot_path) # Step 4: password confirmation dialog. confirm_password = page.get_by_role("dialog").get_by_role( diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 7efea0b2b..ebf14dee2 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -640,7 +640,7 @@ def test_prefers_public_url(self): settings = Settings( nextcloud_public_url="https://nc.example.com", nextcloud_public_issuer_url="https://keycloak.example.com/realms/x", - nextcloud_host="http://app:80", + nextcloud_host="https://app.internal", ) assert settings.nextcloud_browser_url == "https://nc.example.com" @@ -648,14 +648,14 @@ def test_falls_back_to_public_issuer(self): """Without public_url, the OAuth issuer URL is used (single-IdP case).""" settings = Settings( nextcloud_public_issuer_url="https://nc.example.com", - nextcloud_host="http://app:80", + nextcloud_host="https://app.internal", ) assert settings.nextcloud_browser_url == "https://nc.example.com" def test_falls_back_to_host(self): """With neither public URL set, the internal host is used.""" - settings = Settings(nextcloud_host="http://app:80") - assert settings.nextcloud_browser_url == "http://app:80" + settings = Settings(nextcloud_host="https://app.internal") + assert settings.nextcloud_browser_url == "https://app.internal" def test_none_when_nothing_set(self): """Returns None when no Nextcloud URL is configured at all.""" diff --git a/tests/unit/test_elicitation.py b/tests/unit/test_elicitation.py index 905b5ba3a..c7f3ca775 100644 --- a/tests/unit/test_elicitation.py +++ b/tests/unit/test_elicitation.py @@ -37,7 +37,7 @@ def test_astrolabe_settings_url_prefers_public_url(): fake = _fake_settings( public_url="https://nc.example.com", public_issuer_url="https://keycloak.example.com/realms/x", - host="http://internal:8080", + host="https://internal.example", ) with patch("nextcloud_mcp_server.auth.elicitation.get_settings", return_value=fake): assert (