release: v2.5.0

frappe_assistant_core/api/fac_endpoint.py

@@ -51,35 +51,112 @@ def _check_assistant_enabled(user: str) -> bool:
         return False
 
 
-def _import_tools():
-    """Import and register all enabled tools from plugin manager."""
+def _build_tool_registry():
+    """
+    Build a per-request tool registry for the current user.
+
+    Returns a fresh ``OrderedDict`` (name -> tool_dict) built on the call stack
+    rather than mutating the module-level ``mcp`` instance. This keeps
+    concurrent MCP requests isolated from each other: one in-flight request can
+    no longer clear or overwrite the tool set another request is validating or
+    executing against (issue #197). The set is also genuinely per-user, since
+    ``get_available_tools`` filters by the requesting user's permissions.
+
+    Each tool dict carries MCP annotation hints derived from its FAC tool
+    category, so MCP clients (e.g. Claude Desktop) can group tools into
+    Read-only vs Write/delete instead of an undifferentiated "Other tools"
+    bucket. The category is the same one shown/overridable on the FAC admin
+    page (FAC Tool Configuration.tool_category) — single source of truth.
+
+    Returns:
+        OrderedDict mapping tool name to its MCP tool dict.
+    """
+    from collections import OrderedDict
+
+    registry_dict = OrderedDict()
     try:
         from frappe_assistant_core.core.tool_registry import get_tool_registry
-        from frappe_assistant_core.mcp.tool_adapter import register_base_tool
-
-        # Clear existing tools to avoid duplicates on subsequent requests
-        # This is necessary because mcp is a module-level global instance
-        mcp._tool_registry.clear()
+        from frappe_assistant_core.mcp.tool_adapter import build_tool_dict
+        from frappe_assistant_core.utils.tool_category_detector import category_to_annotations
 
         # Get available tools (respects enabled/disabled state and permissions)
         registry = get_tool_registry()
         available_tools = registry.get_available_tools(user=frappe.session.user)
 
-        # Register each enabled tool with MCP server
-        tool_count = 0
+        # Resolve each tool's category once (honors admin overrides stored on
+        # FAC Tool Configuration; falls back to auto-detection).
+        categories = _resolve_tool_categories(
+            [t.get("name") for t in available_tools if t.get("name")], registry
+        )
+
         for tool_metadata in available_tools:
             tool_name = tool_metadata.get("name")
             if tool_name:
                 tool_instance = registry.get_tool(tool_name)
                 if tool_instance:
-                    register_base_tool(mcp, tool_instance)
-                    tool_count += 1
+                    tool_dict = build_tool_dict(tool_instance)
+                    annotations = category_to_annotations(categories.get(tool_name, "read_write"))
+                    if annotations:
+                        # Merge with any annotations the tool already declared.
+                        tool_dict["annotations"] = {**(tool_dict.get("annotations") or {}), **annotations}
+                    registry_dict[tool_name] = tool_dict
 
-        frappe.logger().info(f"Registered {tool_count} enabled tools for user {frappe.session.user}")
+        frappe.logger().info(f"Built {len(registry_dict)} enabled tools for user {frappe.session.user}")
 
     except Exception as e:
         frappe.log_error(title="Tool Import Error", message=f"Error importing tools: {str(e)}")
 
+    return registry_dict
+
+
+def _resolve_tool_categories(tool_names: list, registry) -> dict:
+    """
+    Resolve the FAC tool category for each tool name.
+
+    Resolution order per tool:
+      1. Stored ``FAC Tool Configuration.tool_category`` (honors admin override).
+      2. Auto-detected category via ``detect_tool_category`` (no config row yet).
+      3. ``"read_write"`` fallback (maps to no annotation hints — safe default).
+
+    Stored categories are batch-fetched in one query to avoid a DB read per tool.
+
+    Args:
+        tool_names: Tool names to resolve.
+        registry: The tool registry (used to fetch instances for auto-detection).
+
+    Returns:
+        Dict mapping tool name -> category string.
+    """
+    from frappe_assistant_core.utils.tool_category_detector import detect_tool_category
+
+    categories = {}
+
+    # 1. Batch-fetch stored categories.
+    try:
+        rows = frappe.get_all(
+            "FAC Tool Configuration",
+            filters={"tool_name": ["in", tool_names]} if tool_names else {},
+            fields=["tool_name", "tool_category"],
+            ignore_permissions=True,
+        )
+        for row in rows:
+            if row.get("tool_category"):
+                categories[row["tool_name"]] = row["tool_category"]
+    except Exception as e:
+        frappe.logger().warning(f"Could not batch-fetch tool categories: {e}")
+
+    # 2 & 3. Fill gaps via auto-detection, defaulting to read_write.
+    for tool_name in tool_names:
+        if tool_name in categories:
+            continue
+        try:
+            tool_instance = registry.get_tool(tool_name)
+            categories[tool_name] = detect_tool_category(tool_instance) if tool_instance else "read_write"
+        except Exception:
+            categories[tool_name] = "read_write"
+
+    return categories
+
 
 def _authenticate_mcp_request():
     """
@@ -93,9 +170,10 @@ def _authenticate_mcp_request():
         str: Authenticated username
         None: Authentication failed (returns 401 response directly)
     """
-    from frappe.oauth import get_server_url
     from werkzeug.wrappers import Response
 
+    from frappe_assistant_core.api.oauth_discovery import get_public_base_url
+
     auth_header = frappe.request.headers.get("Authorization", "")
 
     # Try OAuth Bearer token authentication first
@@ -130,7 +208,7 @@ def _authenticate_mcp_request():
         except frappe.DoesNotExistError:
             frappe.logger().error("OAuth Bearer Token not found")
             # Token not found - return 401
-            frappe_url = get_server_url()
+            frappe_url = get_public_base_url()
             metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource"
 
             response = Response()
@@ -151,7 +229,7 @@ def _authenticate_mcp_request():
             frappe.log_error(title="OAuth Token Validation Error", message=f"{type(e).__name__}: {str(e)}")
 
             # Return 401 for invalid/expired tokens
-            frappe_url = get_server_url()
+            frappe_url = get_public_base_url()
             metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource"
 
             response = Response()
@@ -205,7 +283,7 @@ def _authenticate_mcp_request():
 
         except frappe.AuthenticationError as e:
             # Return 401 for invalid API credentials
-            frappe_url = get_server_url()
+            frappe_url = get_public_base_url()
             metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource"
 
             response = Response()
@@ -222,7 +300,7 @@ def _authenticate_mcp_request():
             frappe.log_error(title="API Key Authentication Error", message=f"{type(e).__name__}: {str(e)}")
 
             # Return 401 for other errors
-            frappe_url = get_server_url()
+            frappe_url = get_public_base_url()
             metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource"
 
             response = Response()
@@ -236,7 +314,7 @@ def _authenticate_mcp_request():
 
     # No valid authentication method found
     frappe.logger().warning("No valid authentication method found in request")
-    frappe_url = get_server_url()
+    frappe_url = get_public_base_url()
     metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource"
 
     response = Response()
@@ -264,13 +342,14 @@ def handle_mcp():
     1. OAuth 2.0 Bearer tokens: "Authorization: Bearer <token>" (for web clients)
     2. API Key/Secret: "Authorization: token <api_key>:<api_secret>" (for STDIO clients)
     """
-    from frappe.oauth import get_server_url
     from werkzeug.wrappers import Response
 
+    from frappe_assistant_core.api.oauth_discovery import get_public_base_url
+
     # Handle HEAD request for connectivity check (Claude Web uses this)
     if frappe.request.method == "HEAD":
         # Return 401 with WWW-Authenticate header to indicate auth is required
-        frappe_url = get_server_url()
+        frappe_url = get_public_base_url()
         metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource"
 
         response = Response()
@@ -296,8 +375,6 @@ def handle_mcp():
             _("Assistant access is disabled for user {0}").format(authenticated_user), frappe.PermissionError
         )
 
-    # Import tools (they auto-register via decorators)
-    _import_tools()
-
-    # Return None to let the decorator continue with MCP handling
-    return None
+    # Build a per-request tool registry (isolated from concurrent requests) and
+    # hand it back to the MCP server wrapper, which passes it into handle().
+    return _build_tool_registry()

frappe_assistant_core/api/oauth_discovery.py

@@ -27,6 +27,47 @@
 from frappe.oauth import get_server_url
 
 
+def _get_public_base_url() -> str:
+    """
+    Resolve the canonical public base URL for OAuth/MCP discovery metadata.
+
+    Resolution order:
+      1. site_config host_name — explicit operator override; used verbatim
+         if it includes a scheme (https://example.com).
+      2. frappe.oauth.get_server_url() — existing behavior (Social Login Key
+         "frappe" base_url, then frappe.request.url).
+      3. If force_https is set in site_config and the result still starts
+         with http://, upgrade to https://. Gated so localhost dev still works.
+
+    Trailing slash is stripped so callers can append /api/method/... safely.
+    """
+    host_name = frappe.conf.get("host_name")
+    if host_name and "://" in host_name:
+        base = host_name
+    else:
+        base = get_server_url()
+
+    if frappe.conf.get("force_https") and base.startswith("http://"):
+        base = "https://" + base[len("http://") :]
+
+    return base.rstrip("/")
+
+
+def get_public_base_url() -> str:
+    """
+    Public accessor for the canonical public base URL.
+
+    Other modules (e.g. the MCP endpoint's WWW-Authenticate header) must build
+    OAuth metadata URLs from the same host_name-aware base as the discovery
+    endpoints, so that a configured host_name including a non-standard port is
+    honored verbatim. Frappe's get_server_url() reconstructs host/port from the
+    request or the Social Login Key base_url and drops the configured port
+    (issue #196), which breaks the OAuth handshake behind port-restricted
+    networks.
+    """
+    return _get_public_base_url()
+
+
 # nosemgrep: frappe-semgrep-rules.rules.security.guest-whitelisted-method — OpenID Connect Discovery 1.0 mandates unauthenticated access to this endpoint
 @frappe.whitelist(allow_guest=True, methods=["GET"])
 def openid_configuration():
@@ -52,7 +93,16 @@ def openid_configuration():
     metadata = frappe.local.response
 
     # Add MCP-required fields that are missing
-    frappe_url = get_server_url()
+    frappe_url = _get_public_base_url()
+    # Override Frappe-inherited URLs with canonical public base URL
+    metadata["issuer"] = frappe_url
+    metadata["authorization_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.authorize"
+    metadata["token_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.get_token"
+    metadata["revocation_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.revoke_token"
+    metadata["introspection_endpoint"] = (
+        f"{frappe_url}/api/method/frappe.integrations.oauth2.introspect_token"
+    )
+    metadata["userinfo_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.openid_profile"
 
     # Add jwks_uri (required by MCP Inspector)
     metadata["jwks_uri"] = f"{frappe_url}/api/method/frappe_assistant_core.api.oauth_discovery.jwks"
@@ -107,7 +157,7 @@ def mcp_discovery():
     """
     from frappe_assistant_core import hooks
 
-    frappe_url = get_server_url()
+    frappe_url = _get_public_base_url()
 
     # Get MCP configuration from settings
     mcp_protocol_version = "2025-06-18"
@@ -151,7 +201,7 @@ def _get_frappe_authorization_server_metadata():
         return _get_authorization_server_metadata()
     except ImportError:
         # Fallback for Frappe V15 - build metadata manually
-        frappe_url = get_server_url()
+        frappe_url = _get_public_base_url()
 
         # Base metadata following RFC 8414
         metadata = {
@@ -201,8 +251,18 @@ def authorization_server_metadata():
     # Get base metadata from Frappe (V16 built-in or V15 fallback)
     metadata = _get_frappe_authorization_server_metadata()
 
+    # Normalize all URLs using canonical public base URL (fixes http -> https issue #156)
+    frappe_url = _get_public_base_url()
+    metadata["issuer"] = frappe_url
+    metadata["authorization_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.authorize"
+    metadata["token_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.get_token"
+    metadata["revocation_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.revoke_token"
+    metadata["introspection_endpoint"] = (
+        f"{frappe_url}/api/method/frappe.integrations.oauth2.introspect_token"
+    )
+    metadata["userinfo_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.openid_profile"
+
     # Add/override custom service documentation
-    frappe_url = get_server_url()
     metadata["service_documentation"] = "https://github.com/buildswithpaul/Frappe_Assistant_Core"
 
     # Add client_secret_post as an additional auth method (Frappe V16 only has client_secret_basic)
@@ -261,7 +321,7 @@ def protected_resource_metadata():
 
         raise NotFound("Protected resource metadata is not enabled")
 
-    frappe_url = get_server_url()
+    frappe_url = _get_public_base_url()
 
     # Build list of authorization servers
     authorization_servers = [frappe_url]

frappe_assistant_core/change_log/v2/v2_5_0.md

@@ -0,0 +1,21 @@
+## Version 2.5.0
+
+### New Features
+
+- **MCP tool annotations from FAC tool category (#202)** — tools advertised over MCP now carry `readOnlyHint` / `destructiveHint` annotations derived from each tool's category in FAC Tool Configuration. Clients like Claude Desktop use these to group tools meaningfully instead of bucketing every FAC tool under "Other tools".
+
+### Security
+
+- **`list_documents` permission leak fixed (#189)** — `search_documents` now goes through permission-aware `get_list(ignore_permissions=False)` instead of `get_all`, so results respect the requesting user's document-level permissions.
+
+### Fixes
+
+- **`report_requirements` filter discovery is robust and diagnosable (#203)** — Script Report filters defined only in the `.js` file are now discovered reliably: the `Report.filters` child table is consulted first, the JS path is resolved via Frappe's `get_module_path`, and the parser tolerates JSON-style quoted keys and template-literal labels. Genuinely unparseable cases (e.g. filters built programmatically) now return a `discovery_diagnostics` payload instead of a silent empty result.
+- **Concurrent MCP tool calls isolated (#197)** — each request builds its own tool registry, preventing cross-request state bleed under concurrency.
+- **OAuth/MCP metadata URLs honor configured port (#196)** — the `WWW-Authenticate` metadata URL is built from `host_name` including its port, so discovery works behind non-standard ports and proxies.
+- **`get_doctype_info` includes child table metadata (#192)** — child table fields are now surfaced in the doctype description.
+- **beautifulsoup4 pin relaxed for Frappe v16 (#198)** — constraint widened to `>=4.12,<5` so it spans Frappe v15 (`~=4.12.2`) and v16 (`~=4.13.5`) without forcing a downgrade.
+
+### Improvements
+
+- **OAuth URL resolution centralized** — discovery URLs are normalized through a single `_get_public_base_url()` helper that honors `host_name` configuration.