diff --git a/nextcloud_mcp_server/api/passwords.py b/nextcloud_mcp_server/api/passwords.py index 19ae1bdf..6f29892f 100644 --- a/nextcloud_mcp_server/api/passwords.py +++ b/nextcloud_mcp_server/api/passwords.py @@ -241,7 +241,7 @@ async def provision_app_password(request: Request) -> JSONResponse: # Parse optional scopes and the Nextcloud loginName from the request body # up front. Nextcloud authenticates app passwords against the *loginName*, # which can differ from the UID — e.g. OIDC-provisioned users whose UID is - # their display name (UID "Chris Coutinho", loginName "chris@coutinho.io"). + # their display name (UID "Ada Lovelace", loginName "ada@example.com"). # Use the loginName for the BasicAuth validation below, falling back to the # path user_id for legacy callers that don't send one (where UID == # loginName). diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 4d58069d..e6b95bac 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -2130,14 +2130,22 @@ class RefreshTokenStorage: removed: list[str] = [] async def _validate_user(user_id: str) -> None: - app_password = await self.get_app_password(user_id) - if not app_password: - return - try: + app_data = await self.get_app_password_with_scopes(user_id) + if not app_data: + return + + app_password = app_data["app_password"] + # Authenticate as the stored loginName, not the UID: Nextcloud + # keys app-password auth on the loginName, which differs from + # the UID for OIDC-provisioned users. Using the UID here would + # 401 a *valid* password and wrongly delete it. Falls back to + # the UID for legacy rows without a stored loginName. + login_name = app_data.get("username") or user_id + async with httpx.AsyncClient( base_url=nextcloud_host, - auth=httpx.BasicAuth(user_id, app_password), + auth=httpx.BasicAuth(login_name, app_password), timeout=10.0, ) as client: response = await client.get( diff --git a/nextcloud_mcp_server/client/__init__.py b/nextcloud_mcp_server/client/__init__.py index ce41f47d..e76d324e 100644 --- a/nextcloud_mcp_server/client/__init__.py +++ b/nextcloud_mcp_server/client/__init__.py @@ -102,10 +102,21 @@ class NextcloudClient: username: str, auth: Auth | None = None, *, + auth_username: str | None = None, password: str | None = None, token: str | None = None, ): + # ``username`` is the Nextcloud UID — it drives DAV/API path + # construction (e.g. ``/remote.php/dav/files//``). ``auth_username`` + # is the credential identity Nextcloud authenticates the app password + # against (the loginName), which differs from the UID for + # OIDC-provisioned users. Defaults to ``username`` so single-user and + # OAuth modes (where UID == loginName) are unchanged. Callers pass the + # matching ``auth=BasicAuth(auth_username, ...)`` for the httpx leg; + # ``auth_username`` is threaded to the CalDAV client, which builds its + # own auth object from the raw credential. self.username = username + auth_username = auth_username or username self._client = AsyncClient( base_url=base_url, auth=auth, @@ -122,7 +133,11 @@ class NextcloudClient: # its preferred backend in v3.x) builds a backend-compatible auth object # itself — passing httpx.BasicAuth here breaks under niquests (#731). self.calendar = CalendarClient( - base_url, username, password=password, token=token + base_url, + username, + auth_username=auth_username, + password=password, + token=token, ) self.contacts = ContactsClient(self._client, username) self.cookbook = CookbookClient(self._client, username) diff --git a/nextcloud_mcp_server/client/calendar.py b/nextcloud_mcp_server/client/calendar.py index 54723646..11c3f1c1 100644 --- a/nextcloud_mcp_server/client/calendar.py +++ b/nextcloud_mcp_server/client/calendar.py @@ -42,6 +42,7 @@ class CalendarClient: base_url: str, username: str, *, + auth_username: str | None = None, password: str | None = None, token: str | None = None, ): @@ -55,7 +56,10 @@ class CalendarClient: Args: base_url: Nextcloud base URL - username: Nextcloud username + username: Nextcloud username (UID) — used for DAV path construction + auth_username: Credential identity (loginName) the app password + authenticates against; defaults to ``username``. Differs from + the UID for OIDC-provisioned users. password: App password / login password — selects ``auth_type="basic"`` token: OAuth bearer token — selects ``auth_type="bearer"`` @@ -64,6 +68,11 @@ class CalendarClient: """ self.username = username self.base_url = base_url + # The UID (``username``) drives DAV path construction; the loginName + # (``auth_username``) is the credential the app password authenticates + # against. They differ for OIDC-provisioned users. Defaults to the UID + # so existing single-user / OAuth callers are unchanged. + auth_username = auth_username or username auth_kwargs: dict[str, Any] = {} if password is not None: @@ -74,7 +83,7 @@ class CalendarClient: # AsyncDAVClient needs the full base URL for proper URL construction self._dav_client = AsyncDAVClient( url=f"{base_url}/remote.php/dav/", - username=username, + username=auth_username, ssl_verify_cert=get_nextcloud_ssl_verify(), # type: ignore[arg-type] # caldav types say bool|str but passes through to niquests which accepts SSLContext **auth_kwargs, ) diff --git a/nextcloud_mcp_server/context.py b/nextcloud_mcp_server/context.py index d4b4cae3..2d8d0ec4 100644 --- a/nextcloud_mcp_server/context.py +++ b/nextcloud_mcp_server/context.py @@ -191,13 +191,27 @@ async def _get_client_from_login_flow( "Call nc_auth_provision_access to complete Login Flow." ) - username = app_data.get("username") or user_id + # Authenticate with the stored Nextcloud loginName, but build DAV/API paths + # with ``user_id`` — the identity the rest of the system is keyed on (the + # app-password store, vector payloads, scopes, and the background-sync + # ``get_user_client_basic_auth`` all use it). For OIDC-provisioned users the + # loginName (e.g. an email) differs from this identity, and Nextcloud + # authenticates app passwords against the loginName. Falls back to + # ``user_id`` for legacy rows stored without a loginName. + login_name = app_data.get("username") or user_id + app_password = app_data["app_password"] - logger.debug("Creating Login Flow v2 client for %s as %s", nextcloud_host, username) + logger.debug( + "Creating Login Flow v2 client for %s (id=%s, login=%s)", + nextcloud_host, + user_id, + login_name, + ) return NextcloudClient( base_url=nextcloud_host, - username=username, - auth=BasicAuth(username, app_data["app_password"]), - password=app_data["app_password"], + username=user_id, + auth_username=login_name, + auth=BasicAuth(login_name, app_password), + password=app_password, ) diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index ad37556d..fe6f7a4f 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -114,20 +114,28 @@ async def get_user_client_basic_auth( if storage is None: storage = await _get_initialized_basic_auth_storage() - # Retrieve app password from local storage - app_password = await storage.get_app_password(user_id) + # Retrieve app password (and the stored Nextcloud loginName) from local + # storage. Nextcloud authenticates app passwords against the loginName, + # which differs from the UID for OIDC-provisioned users; authenticate as + # the loginName while keeping the UID for DAV/API path construction. Falls + # back to the UID for legacy rows stored without a loginName. + app_data = await storage.get_app_password_with_scopes(user_id) - if not app_password: + if not app_data: raise NotProvisionedError( f"User {user_id} has not provisioned an app password. " f"User must configure background sync in Astrolabe personal settings." ) + app_password = app_data["app_password"] + login_name = app_data.get("username") or user_id + logger.info("Using app password for background sync: %s", user_id) return NextcloudClient( base_url=nextcloud_host, username=user_id, - auth=BasicAuth(user_id, app_password), + auth_username=login_name, + auth=BasicAuth(login_name, app_password), password=app_password, ) diff --git a/tests/unit/client/test_calendar.py b/tests/unit/client/test_calendar.py index a2e0e92a..bb7b15ce 100644 --- a/tests/unit/client/test_calendar.py +++ b/tests/unit/client/test_calendar.py @@ -97,3 +97,48 @@ def test_password_takes_precedence_over_token(mocker): call_kwargs = mock_dav_client.call_args.kwargs assert call_kwargs["password"] == "app-pw" assert call_kwargs["auth_type"] == "basic" + + +def test_auth_username_used_for_credential_uid_for_path(mocker): + """OIDC users: the loginName authenticates, the UID builds the DAV path. + + Nextcloud keys app-password auth on the loginName (which can differ from + the UID), but ``/remote.php/dav/calendars//`` must use the UID. The + two must not be conflated. + """ + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + + from nextcloud_mcp_server.client.calendar import CalendarClient + + client = CalendarClient( + "https://cloud.example.org", + "Ada Lovelace", # UID + auth_username="ada@example.com", # loginName + password="app-pw-1234", + ) + + # Credential identity → loginName + assert mock_dav_client.call_args.kwargs["username"] == "ada@example.com" + # Path identity → UID + assert client.username == "Ada Lovelace" + assert ( + client._calendar_home_url + == "https://cloud.example.org/remote.php/dav/calendars/Ada Lovelace/" + ) + + +def test_auth_username_defaults_to_username(mocker): + """Backwards compat: without ``auth_username`` the UID is used for both, + so single-user / OAuth callers (UID == loginName) are unchanged. + """ + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + + from nextcloud_mcp_server.client.calendar import CalendarClient + + CalendarClient("https://cloud.example.org", "alice", password="app-pw") + + assert mock_dav_client.call_args.kwargs["username"] == "alice" diff --git a/tests/unit/test_app_password_loginname_auth.py b/tests/unit/test_app_password_loginname_auth.py new file mode 100644 index 00000000..0f57bb4b --- /dev/null +++ b/tests/unit/test_app_password_loginname_auth.py @@ -0,0 +1,216 @@ +"""Unit tests: stored-app-password client builders authenticate with the +Nextcloud loginName, not the UID. + +Regression for the OIDC-provisioned-user case where the Nextcloud UID differs +from the loginName (e.g. UID "Ada Lovelace", loginName "ada@example.com"). +Nextcloud authenticates app passwords against the loginName, so binding the UID +as the BasicAuth username returns HTTP 401 for every Notes/Files/Shares/CalDAV +call — which previously stopped the background sync scan loop and degraded +ACL-aware search to a self-only owner filter. + +The builders must split the two identities: +- credential username (httpx + CalDAV auth) → loginName +- DAV/URL path identity (``client.username``) → UID +""" + +import tempfile +from pathlib import Path + +import httpx +import pytest +from cryptography.fernet import Fernet + +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage + +pytestmark = pytest.mark.unit + +_APP_PW = "aaaaa-bbbbb-ccccc-ddddd-eeeee" + + +def _basic_auth_header(username: str, password: str) -> str: + return httpx.BasicAuth(username, password)._auth_header + + +@pytest.fixture +async def temp_storage(): + """Encrypted storage backed by a throwaway SQLite file.""" + with tempfile.TemporaryDirectory() as tmpdir: + storage = RefreshTokenStorage( + db_path=str(Path(tmpdir) / "test.db"), + encryption_key=Fernet.generate_key().decode(), + ) + await storage.initialize() + yield storage + + +class _FakeAsyncClient: + """Minimal stand-in for ``httpx.AsyncClient`` that records the ``auth=`` + kwarg and returns a canned status code.""" + + def __init__(self, status_code: int): + self._status_code = status_code + self.captured_auth: httpx.Auth | None = None + + def __call__(self, *args, **kwargs): + self.captured_auth = kwargs.get("auth") + return self + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def get(self, *args, **kwargs): + return httpx.Response(self._status_code) + + +async def test_basic_auth_builder_splits_uid_and_loginname(mocker): + """multi_user_basic background-sync builder: loginName authenticates, + UID builds paths.""" + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + from nextcloud_mcp_server.vector.oauth_sync import get_user_client_basic_auth + + storage = mocker.MagicMock() + storage.get_app_password_with_scopes = mocker.AsyncMock( + return_value={ + "app_password": "app-pw-1234", + "username": "ada@example.com", # loginName + "scopes": None, + } + ) + + client = await get_user_client_basic_auth( + "Ada Lovelace", # UID + "https://cloud.example.org", + storage=storage, + ) + + # Path identity → UID + assert client.username == "Ada Lovelace" + # httpx credential (notes/webdav/sharing) → loginName + assert client._client.auth._auth_header == _basic_auth_header( + "ada@example.com", "app-pw-1234" + ) + # CalDAV credential → loginName, but calendar-home path → UID + assert mock_dav_client.call_args.kwargs["username"] == "ada@example.com" + assert client.calendar.username == "Ada Lovelace" + + +async def test_basic_auth_builder_legacy_row_falls_back_to_uid(mocker): + """Legacy rows stored before the loginName column was populated have + ``username = None`` → fall back to the UID for the credential (preserves + the previous behaviour where UID == loginName).""" + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + from nextcloud_mcp_server.vector.oauth_sync import get_user_client_basic_auth + + storage = mocker.MagicMock() + storage.get_app_password_with_scopes = mocker.AsyncMock( + return_value={"app_password": "pw", "username": None, "scopes": None} + ) + + client = await get_user_client_basic_auth( + "alice", "https://cloud.example.org", storage=storage + ) + + assert client.username == "alice" + assert client._client.auth._auth_header == _basic_auth_header("alice", "pw") + assert mock_dav_client.call_args.kwargs["username"] == "alice" + + +async def test_basic_auth_builder_unprovisioned_raises(mocker): + """No stored app password → NotProvisionedError (unchanged contract).""" + from nextcloud_mcp_server.vector.oauth_sync import ( + NotProvisionedError, + get_user_client_basic_auth, + ) + + storage = mocker.MagicMock() + storage.get_app_password_with_scopes = mocker.AsyncMock(return_value=None) + + with pytest.raises(NotProvisionedError): + await get_user_client_basic_auth( + "alice", "https://cloud.example.org", storage=storage + ) + + +async def test_login_flow_builder_splits_id_and_loginname(mocker): + """Login Flow v2 per-request builder (MCP tool path): same split as the + background-sync builder. ``user_id`` (the identity the system is keyed on, + == NC UID for NC-as-OIDC-IdP) builds DAV paths; the stored loginName + authenticates. Previously the loginName was used for the DAV path too, + which is wrong for OIDC users (UID != loginName).""" + mock_dav_client = mocker.patch( + "nextcloud_mcp_server.client.calendar.AsyncDAVClient" + ) + from nextcloud_mcp_server import context + + mocker.patch( + "nextcloud_mcp_server.auth.token_utils.extract_user_id_from_token", + mocker.AsyncMock(return_value="Ada Lovelace"), # token sub == NC UID + ) + storage = mocker.MagicMock() + storage.get_app_password_with_scopes = mocker.AsyncMock( + return_value={ + "app_password": "app-pw-9999", + "username": "ada@example.com", # loginName + "scopes": None, + } + ) + mocker.patch.object( + context, "get_shared_storage", mocker.AsyncMock(return_value=storage) + ) + + client = await context._get_client_from_login_flow( + mocker.MagicMock(), "https://cloud.example.org" + ) + + # Path identity → user_id (== UID); credential → loginName + assert client.username == "Ada Lovelace" + assert client._client.auth._auth_header == _basic_auth_header( + "ada@example.com", "app-pw-9999" + ) + assert mock_dav_client.call_args.kwargs["username"] == "ada@example.com" + + +async def test_cleanup_authenticates_with_loginname_not_uid(temp_storage, mocker): + """cleanup_invalid_app_passwords must validate with the stored loginName. + + Validating as the UID would 401 a *valid* OIDC password and wrongly delete + it — the exact failure observed on a login_flow tenant in production. + """ + await temp_storage.store_app_password_with_scopes( + "Ada Lovelace", _APP_PW, username="ada@example.com" + ) + fake = _FakeAsyncClient(status_code=200) # valid credential + mocker.patch("nextcloud_mcp_server.auth.storage.httpx.AsyncClient", fake) + + removed = await temp_storage.cleanup_invalid_app_passwords( + "https://cloud.example.org" + ) + + assert removed == [] # valid password preserved + assert fake.captured_auth._auth_header == _basic_auth_header( + "ada@example.com", _APP_PW + ) + assert await temp_storage.get_app_password("Ada Lovelace") == _APP_PW + + +async def test_cleanup_removes_genuinely_invalid_password(temp_storage, mocker): + """A real 401 still removes the stored password (unchanged contract).""" + await temp_storage.store_app_password_with_scopes( + "alice", _APP_PW, username="alice" + ) + fake = _FakeAsyncClient(status_code=401) + mocker.patch("nextcloud_mcp_server.auth.storage.httpx.AsyncClient", fake) + + removed = await temp_storage.cleanup_invalid_app_passwords( + "https://cloud.example.org" + ) + + assert removed == ["alice"] + assert await temp_storage.get_app_password("alice") is None diff --git a/tests/unit/test_management_app_password_endpoints.py b/tests/unit/test_management_app_password_endpoints.py index 0a6fcb55..7b62ac3e 100644 --- a/tests/unit/test_management_app_password_endpoints.py +++ b/tests/unit/test_management_app_password_endpoints.py @@ -257,7 +257,7 @@ async def test_provision_app_password_success(temp_storage, mocker): async def test_provision_app_password_uses_loginname_not_uid(temp_storage, mocker): """Regression: when the Nextcloud UID differs from the loginName (e.g. OIDC-provisioned users whose UID is their display name — UID - "Chris Coutinho", loginName "chris@coutinho.io"), the OCS BasicAuth + "Ada Lovelace", loginName "ada@example.com"), the OCS BasicAuth validation must authenticate as the loginName from the request body, not the UID. Authenticating as the UID is rejected by Nextcloud with HTTP 401. """ @@ -273,7 +273,7 @@ async def test_provision_app_password_uses_loginname_not_uid(temp_storage, mocke # OCS validation succeeds and reports the UID as the account id. mock_response = MagicMock() mock_response.status_code = 200 - mock_response.json.return_value = {"ocs": {"data": {"id": "Chris Coutinho"}}} + mock_response.json.return_value = {"ocs": {"data": {"id": "Ada Lovelace"}}} mock_client = AsyncMock() mock_client.get = AsyncMock(return_value=mock_response) @@ -291,9 +291,9 @@ async def test_provision_app_password_uses_loginname_not_uid(temp_storage, mocke # A literal space in the path is encoded by the client and decoded back to # the UID; the BasicAuth username matches that UID. response = client.post( - "/api/v1/users/Chris Coutinho/app-password", - headers={"Authorization": create_basic_auth_header("Chris Coutinho", pw)}, - json={"username": "chris@coutinho.io"}, + "/api/v1/users/Ada Lovelace/app-password", + headers={"Authorization": create_basic_auth_header("Ada Lovelace", pw)}, + json={"username": "ada@example.com"}, ) assert response.status_code == 200 @@ -301,10 +301,10 @@ async def test_provision_app_password_uses_loginname_not_uid(temp_storage, mocke # The OCS BasicAuth used the loginName from the body, not the UID. _, get_kwargs = mock_client.get.call_args - assert get_kwargs["auth"] == ("chris@coutinho.io", pw) + assert get_kwargs["auth"] == ("ada@example.com", pw) # Stored under the UID (the identity key). - assert await temp_storage.get_app_password("Chris Coutinho") == pw + assert await temp_storage.get_app_password("Ada Lovelace") == pw async def test_provision_app_password_nextcloud_validation_fails(mocker):