diff --git a/nextcloud_mcp_server/auth/elicitation.py b/nextcloud_mcp_server/auth/elicitation.py index f8c0e1a8..a5d1abfd 100644 --- a/nextcloud_mcp_server/auth/elicitation.py +++ b/nextcloud_mcp_server/auth/elicitation.py @@ -115,8 +115,9 @@ async def present_login_url( return "message_only" except Exception as e: logger.warning( - f"Elicitation failed unexpectedly ({type(e).__name__}: {e}), " - "falling back to message" + "Elicitation failed unexpectedly (%s: %s), falling back to message", + type(e).__name__, + e, ) return "message_only" diff --git a/nextcloud_mcp_server/auth/scope_authorization.py b/nextcloud_mcp_server/auth/scope_authorization.py index cacdbd29..25d02ab2 100644 --- a/nextcloud_mcp_server/auth/scope_authorization.py +++ b/nextcloud_mcp_server/auth/scope_authorization.py @@ -159,13 +159,27 @@ def require_scopes(*required_scopes: str): present_provisioning_required, ) - await present_provisioning_required(ctx) + elicit_result = await present_provisioning_required(ctx) - error_msg = ( - f"Access denied to {func_name}: " - f"Nextcloud access not provisioned. " - f"Please call 'nc_auth_provision_access' first." - ) + # Always raise — the decorator can't safely re-check + # stored scopes mid-call (TTL cache, plus the LFv2 + # poller may still be running). Only the message + # changes so an LLM that just acknowledged the + # elicitation isn't told to call the auth tool + # again (which would loop). + if elicit_result == "accepted": + error_msg = ( + f"Access denied to {func_name}: Nextcloud " + f"access was not provisioned at the time of " + f"this call. If you just completed " + f"provisioning, please retry the request." + ) + else: + error_msg = ( + f"Access denied to {func_name}: " + f"Nextcloud access not provisioned. " + f"Please call 'nc_auth_provision_access' first." + ) logger.warning(error_msg) raise ProvisioningRequiredError(error_msg) diff --git a/tests/unit/test_elicitation.py b/tests/unit/test_elicitation.py index b7487de3..9785fca4 100644 --- a/tests/unit/test_elicitation.py +++ b/tests/unit/test_elicitation.py @@ -31,6 +31,15 @@ def test_astrolabe_settings_url_prefers_public_issuer(monkeypatch): ) +def test_astrolabe_settings_url_strips_trailing_slash_from_public_issuer(monkeypatch): + """Trailing slash on NEXTCLOUD_PUBLIC_ISSUER_URL is normalized.""" + monkeypatch.setenv("NEXTCLOUD_PUBLIC_ISSUER_URL", "https://nc.example.com/") + + assert ( + _astrolabe_settings_url() == f"https://nc.example.com{ASTROLABE_SETTINGS_PATH}" + ) + + def test_astrolabe_settings_url_falls_back_to_host(monkeypatch): """When only NEXTCLOUD_HOST is set, use it (and strip a trailing slash).""" monkeypatch.setenv("NEXTCLOUD_HOST", "https://only-host.example.com/") diff --git a/tests/unit/test_scope_authorization_stored.py b/tests/unit/test_scope_authorization_stored.py index 1a071d84..432290ef 100644 --- a/tests/unit/test_scope_authorization_stored.py +++ b/tests/unit/test_scope_authorization_stored.py @@ -112,14 +112,12 @@ def _make_login_flow_ctx() -> MagicMock: return ctx -async def test_decorator_elicits_before_raising_when_app_password_missing(): - """When no app password is stored, the decorator must elicit a clickable - Astrolabe / Login-Flow-v2 prompt to the client *before* raising - ProvisioningRequiredError. +async def test_decorator_elicits_and_uses_retry_message_when_user_accepts(): + """When the elicit returns "accepted" the raised error must tell the user + to retry — *not* "call nc_auth_provision_access". The latter would loop + an LLM that just acknowledged the elicitation prompt. - Why: an LLM-only error message ("call nc_auth_provision_access") is - unfriendly to humans whose MCP client supports elicitation. See - cbcoutinho/nextcloud-mcp-server#752. + See PR #757 review feedback (cbcoutinho/nextcloud-mcp-server#757). """ ctx = _make_login_flow_ctx() @@ -147,11 +145,55 @@ async def test_decorator_elicits_before_raising_when_app_password_missing(): "nextcloud_mcp_server.auth.elicitation.present_provisioning_required", elicit_mock, ), - pytest.raises(ProvisioningRequiredError), + pytest.raises(ProvisioningRequiredError) as exc_info, ): await fake_tool_missing_pwd(ctx=ctx) elicit_mock.assert_awaited_once_with(ctx) + msg = str(exc_info.value) + assert "retry the request" in msg + assert "nc_auth_provision_access" not in msg + + +async def test_decorator_uses_legacy_message_when_elicitation_unsupported(): + """When the elicit helper returns "message_only" (client lacks elicit + support), the raised error must keep the existing + "call nc_auth_provision_access" instruction so an agent has something + actionable. Mirrors the "accepted" case but for the fallback branch.""" + ctx = _make_login_flow_ctx() + + @require_scopes("notes.read") + async def fake_tool_missing_pwd_no_elicit(ctx: Context): # noqa: ARG001 + return "ok" + + fake_settings = SimpleNamespace(enable_login_flow=True) + elicit_mock = AsyncMock(return_value="message_only") + + with ( + patch( + "nextcloud_mcp_server.auth.scope_authorization.get_settings", + return_value=fake_settings, + ), + patch( + "nextcloud_mcp_server.auth.scope_authorization._get_stored_scopes", + return_value=None, + ), + patch( + "nextcloud_mcp_server.auth.token_utils.extract_user_id_from_token", + return_value="alice", + ), + patch( + "nextcloud_mcp_server.auth.elicitation.present_provisioning_required", + elicit_mock, + ), + pytest.raises(ProvisioningRequiredError) as exc_info, + ): + await fake_tool_missing_pwd_no_elicit(ctx=ctx) + + elicit_mock.assert_awaited_once_with(ctx) + msg = str(exc_info.value) + assert "nc_auth_provision_access" in msg + assert "retry the request" not in msg async def test_decorator_does_not_elicit_when_scopes_only_partially_missing():