From d9a716080aeace3a8e3561883dcc0519176181d0 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 28 May 2026 23:22:40 +0200 Subject: [PATCH] fix(auth): make provision/revoke consistent with the app-password store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OAuth provisioning tools (check_provisioning_status, revoke_nextcloud_ access) only consulted the refresh-token store + Astrolabe status, ignoring the app_passwords store that Login Flow v2 (nc_auth_provision_access) and the management API write to — the same store require_provisioning / get_client use to grant tool access. Result: status reported "not provisioned" while tools worked, and revoke said "nothing to revoke" while the credential persisted. - _get_provisioning_status: also check storage.get_app_password_with_scopes, reporting is_provisioned with credential_type=app_password, flow_type=login_flow_v2. - _revoke_nextcloud_access: when the credential is an app password, delete it from storage + invalidate the scope cache (no IdP token to revoke); refresh-token revocation via the Token Broker is unchanged. - tests/unit/test_oauth_tools_app_password_provisioning.py: cover status + revoke for the app-password path. - bump astrolabe submodule (deprovision MCP on disable); fix a stale assertion in the migrated bg-sync test (one-click flow has no separate app-password generation step). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/server/oauth_tools.py | 44 ++++++++- ...st_astrolabe_multi_user_background_sync.py | 3 - ...t_oauth_tools_app_password_provisioning.py | 92 +++++++++++++++++++ third_party/astrolabe | 2 +- 4 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_oauth_tools_app_password_provisioning.py diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 3d18e5e3..7e0ee70f 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -17,6 +17,7 @@ from pydantic import BaseModel, Field from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.auth.astrolabe_client import AstrolabeClient +from nextcloud_mcp_server.auth.scope_authorization import invalidate_scope_cache from nextcloud_mcp_server.auth.storage import get_shared_storage from nextcloud_mcp_server.auth.token_broker import TokenBrokerService @@ -132,6 +133,26 @@ async def _get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSt ) storage = await get_shared_storage() + # Login Flow v2 app password stored directly in this server's storage — + # written by nc_auth_provision_access and the management app-password API, + # and the credential that require_provisioning / get_client actually use. + # Checked here so check_provisioning_status and revoke_nextcloud_access stay + # consistent with what actually grants tool access (the dual-store drift in + # the original code reported "not provisioned" while tools still worked). + app_pw = await storage.get_app_password_with_scopes(user_id) + if app_pw: + logger.debug( + " get_provisioning_status: app password (login-flow store) FOUND " + "for user_id=%s", + user_id, + ) + return ProvisioningStatus( + is_provisioned=True, + credential_type="app_password", + scopes=app_pw.get("scopes"), + flow_type="login_flow_v2", + ) + token_data = await storage.get_refresh_token(user_id) if not token_data: @@ -297,9 +318,30 @@ async def _revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResu message="No Nextcloud access to revoke.", ) - # Initialize Token Broker to handle revocation storage = await get_shared_storage() + # App-password credential (Login Flow v2 / management API): there is no + # IdP token to revoke — removing it from this server's storage drops the + # server's access. Without this, revoke previously only handled refresh + # tokens and left the app password in place (tools kept working). + if status.credential_type == "app_password": + deleted = await storage.delete_app_password(user_id) + invalidate_scope_cache(user_id) + if deleted: + return RevocationResult( + success=True, + message=( + "Successfully revoked Nextcloud access (app password " + "removed). You can run provisioning again if needed." + ), + ) + return RevocationResult( + success=True, + message="No Nextcloud access to revoke.", + ) + + # Refresh-token credential: revoke via the Token Broker (IdP revocation). + # Get OAuth client credentials from storage client_creds = await storage.get_oauth_client() if not client_creds: diff --git a/tests/integration/test_astrolabe_multi_user_background_sync.py b/tests/integration/test_astrolabe_multi_user_background_sync.py index 70482e64..af355ad6 100644 --- a/tests/integration/test_astrolabe_multi_user_background_sync.py +++ b/tests/integration/test_astrolabe_multi_user_background_sync.py @@ -446,9 +446,6 @@ async def test_multi_user_astrolabe_background_sync_enablement( assert result["settings_accessed"], ( f"{username} could not access Astrolabe settings" ) - assert result["app_password_generated"], ( - f"{username} app password was not generated" - ) assert result["sync_enabled"], ( f"{username} background sync enablement did not complete successfully" ) diff --git a/tests/unit/test_oauth_tools_app_password_provisioning.py b/tests/unit/test_oauth_tools_app_password_provisioning.py new file mode 100644 index 00000000..c190d41d --- /dev/null +++ b/tests/unit/test_oauth_tools_app_password_provisioning.py @@ -0,0 +1,92 @@ +"""Unit tests for app-password-store awareness in the provisioning tools. + +Login Flow v2 (nc_auth_provision_access) and the management app-password API +write the credential to this server's ``app_passwords`` store — the same store +``require_provisioning``/``get_client`` use to grant tool access. The OAuth +provisioning tools (check_provisioning_status / revoke_nextcloud_access) must +read and clear that store too, otherwise they report "not provisioned" while +tools still work, and "nothing to revoke" while the credential persists. +""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from nextcloud_mcp_server.server import oauth_tools +from nextcloud_mcp_server.server.oauth_tools import ( + _get_provisioning_status, + _revoke_nextcloud_access, +) + +pytestmark = pytest.mark.unit + + +@pytest.fixture +def _no_astrolabe_settings(mocker): + """Disable the astrolabe-status branch so the app_passwords store is hit.""" + mocker.patch.object( + oauth_tools, + "get_settings", + return_value=SimpleNamespace(oidc_client_id=None, oidc_client_secret=None), + ) + + +async def test_status_reports_provisioned_for_app_password_store( + mocker, _no_astrolabe_settings +): + """A Login Flow v2 app password in storage => is_provisioned with the + app_password credential type (was previously reported as not provisioned).""" + storage = MagicMock() + storage.get_app_password_with_scopes = AsyncMock( + return_value={"app_password": "tok", "scopes": ["notes.read"]} + ) + storage.get_refresh_token = AsyncMock(return_value=None) + mocker.patch.object( + oauth_tools, "get_shared_storage", AsyncMock(return_value=storage) + ) + + status = await _get_provisioning_status(MagicMock(), "tester") + + assert status.is_provisioned is True + assert status.credential_type == "app_password" + assert status.flow_type == "login_flow_v2" + assert status.scopes == ["notes.read"] + storage.get_refresh_token.assert_not_awaited() # app password short-circuits + + +async def test_revoke_deletes_app_password(mocker, _no_astrolabe_settings): + """Revoke must delete the app password from storage (not just refresh tokens).""" + storage = MagicMock() + storage.get_app_password_with_scopes = AsyncMock( + return_value={"app_password": "tok", "scopes": None} + ) + storage.get_refresh_token = AsyncMock(return_value=None) + storage.delete_app_password = AsyncMock(return_value=True) + mocker.patch.object( + oauth_tools, "get_shared_storage", AsyncMock(return_value=storage) + ) + mocker.patch.object(oauth_tools, "invalidate_scope_cache") + + result = await _revoke_nextcloud_access(MagicMock(), "tester") + + assert result.success is True + storage.delete_app_password.assert_awaited_once_with("tester") + oauth_tools.invalidate_scope_cache.assert_called_once_with("tester") + + +async def test_revoke_noop_when_nothing_provisioned(mocker, _no_astrolabe_settings): + """No credential of any kind => graceful no-op, no deletion attempted.""" + storage = MagicMock() + storage.get_app_password_with_scopes = AsyncMock(return_value=None) + storage.get_refresh_token = AsyncMock(return_value=None) + storage.delete_app_password = AsyncMock() + mocker.patch.object( + oauth_tools, "get_shared_storage", AsyncMock(return_value=storage) + ) + + result = await _revoke_nextcloud_access(MagicMock(), "tester") + + assert result.success is True + assert "No Nextcloud access to revoke" in result.message + storage.delete_app_password.assert_not_awaited() diff --git a/third_party/astrolabe b/third_party/astrolabe index 71a18287..e81d961f 160000 --- a/third_party/astrolabe +++ b/third_party/astrolabe @@ -1 +1 @@ -Subproject commit 71a18287d47159aac5668a684c415b03254ea6fd +Subproject commit e81d961fb126c23eeed8adb93c7ac2fcf969c2f2