Refactor MCP implementation with enhanced security and session management

- Replace MoquiMcpServlet with EnhancedMcpServlet for better SSE handling
- Add proper JSON-RPC message classes for MCP compatibility
- Implement proper permission checks in ToolsList service
- Remove temporary permission bypasses and test ping service
- Update McpFilter to use EnhancedMcpServlet
- Clean up unused dependencies and configuration files
- Fix parameter type handling and required field detection

@@ -231,7 +231,7 @@
         </actions>
         </actions>
     </service>
     </service>
     
     
-    <service verb="mcp" noun="Initialize" authenticate="false" allow-remote="true" transaction-timeout="30">
+    <service verb="mcp" noun="Initialize" authenticate="true" allow-remote="true" transaction-timeout="30">
         <description>Handle MCP initialize request using Moqui authentication</description>
         <description>Handle MCP initialize request using Moqui authentication</description>
         <in-parameters>
         <in-parameters>
             <parameter name="protocolVersion" required="true"/>
             <parameter name="protocolVersion" required="true"/>
@@ -318,14 +318,12 @@
                             continue
                             continue
                         }
                         }
                         
                         
-                        // TODO: Fix permission check - temporarily bypass for testing
-                        boolean hasPermission = true
-                        ec.logger.info("MCP ToolsList: Service ${serviceName} bypassing permission check for testing")
-                        // boolean hasPermission = ec.user.hasPermission(serviceName)
-                        // ec.logger.info("MCP ToolsList: Service ${serviceName} hasPermission=${hasPermission}")
-                        // if (!hasPermission) {
-                        //     continue
-                        // }
+                        // Check permission using Moqui's artifact authorization
+                        boolean hasPermission = ec.user.hasPermission(serviceName)
+                        ec.logger.info("MCP ToolsList: Service ${serviceName} hasPermission=${hasPermission}")
+                        if (!hasPermission) {
+                            continue
+                        }
                         
                         
                         def serviceDefinition = ec.service.getServiceDefinition(serviceName)
                         def serviceDefinition = ec.service.getServiceDefinition(serviceName)
                         if (!serviceDefinition) continue
                         if (!serviceDefinition) continue
@@ -363,6 +361,7 @@
                             }
                             }
                             
                             
                             // Convert Moqui type to JSON Schema type
                             // Convert Moqui type to JSON Schema type
+                            // Convert Moqui type to JSON Schema type
                             def typeMap = [
                             def typeMap = [
                                 "text-short": "string",
                                 "text-short": "string",
                                 "text-medium": "string", 
                                 "text-medium": "string", 
@@ -379,14 +378,14 @@
                                 "boolean": "boolean",
                                 "boolean": "boolean",
                                 "text-indicator": "boolean"
                                 "text-indicator": "boolean"
                             ]
                             ]
-                            def jsonSchemaType = typeMap[paramInfo.type] ?: "string"
+                            def jsonSchemaType = typeMap[paramType] ?: "string"
                             
                             
                             tool.inputSchema.properties[paramName] = [
                             tool.inputSchema.properties[paramName] = [
                                 type: jsonSchemaType,
                                 type: jsonSchemaType,
                                 description: paramDesc
                                 description: paramDesc
                             ]
                             ]
                             
                             
-                            if (paramInfo.required) {
+                            if (paramNode?.attribute('required') == "true") {
                                 tool.inputSchema.required << paramName
                                 tool.inputSchema.required << paramName
                             }
                             }
                         }
                         }
@@ -815,17 +814,7 @@
         </actions>
         </actions>
     </service>
     </service>
 
 
-    <service verb="mcp" noun="Ping" authenticate="false" allow-remote="true" transaction-timeout="30">
-        <description>Simple ping service for MCP testing</description>
-        <out-parameters>
-            <parameter name="message" type="String"/>
-        </out-parameters>
-        <actions>
-            <script><![CDATA[
-                result = [message: "MCP ping successful at ${new Date()}"]
-            ]]></script>
-        </actions>
-    </service>
+
 
 
     <!-- NOTE: handle#McpRequest service removed - functionality moved to screen/webapp.xml for unified handling -->
     <!-- NOTE: handle#McpRequest service removed - functionality moved to screen/webapp.xml for unified handling -->
 
 

@@ -14,6 +14,7 @@
 package org.moqui.mcp
 package org.moqui.mcp
 
 
 import groovy.json.JsonSlurper
 import groovy.json.JsonSlurper
+import groovy.json.JsonOutput
 import org.moqui.impl.context.ExecutionContextFactoryImpl
 import org.moqui.impl.context.ExecutionContextFactoryImpl
 import org.moqui.context.ArtifactAuthorizationException
 import org.moqui.context.ArtifactAuthorizationException
 import org.moqui.context.ArtifactTarpitException
 import org.moqui.context.ArtifactTarpitException
@@ -31,6 +32,47 @@ import java.util.concurrent.atomic.AtomicBoolean
 import java.util.UUID
 import java.util.UUID
 
 
 /**
 /**
+ * Simple JSON-RPC Message classes for MCP compatibility
+ */
+class JsonRpcMessage {
+    String jsonrpc = "2.0"
+}
+
+class JsonRpcResponse extends JsonRpcMessage {
+    Object id
+    Object result
+    Map error
+    
+    JsonRpcResponse(Object result, Object id) {
+        this.result = result
+        this.id = id
+    }
+    
+    JsonRpcResponse(Map error, Object id) {
+        this.error = error
+        this.id = id
+    }
+    
+    String toJson() {
+        return JsonOutput.toJson(this)
+    }
+}
+
+class JsonRpcNotification extends JsonRpcMessage {
+    String method
+    Object params
+    
+    JsonRpcNotification(String method, Object params = null) {
+        this.method = method
+        this.params = params
+    }
+    
+    String toJson() {
+        return JsonOutput.toJson(this)
+    }
+}
+
+/**
  * Enhanced MCP Servlet with proper SSE handling inspired by HttpServletSseServerTransportProvider
  * Enhanced MCP Servlet with proper SSE handling inspired by HttpServletSseServerTransportProvider
  * This implementation provides better SSE support and session management.
  * This implementation provides better SSE support and session management.
  */
  */
@@ -369,7 +411,7 @@ try {
             def result = processMcpMethod(rpcRequest.method, rpcRequest.params, ec)
             def result = processMcpMethod(rpcRequest.method, rpcRequest.params, ec)
             
             
             // Send response via MCP transport to the specific session
             // Send response via MCP transport to the specific session
-            def responseMessage = new McpSchema.JSONRPCMessage(result, rpcRequest.id)
+            def responseMessage = new JsonRpcResponse(result, rpcRequest.id)
             session.sendMessage(responseMessage)
             session.sendMessage(responseMessage)
             
             
             response.setContentType("application/json")
             response.setContentType("application/json")
@@ -593,7 +635,7 @@ try {
     /**
     /**
      * Broadcast message to all active sessions
      * Broadcast message to all active sessions
      */
      */
-    void broadcastToAllSessions(McpSchema.JSONRPCMessage message) {
+    void broadcastToAllSessions(JsonRpcMessage message) {
         sessionManager.broadcast(message)
         sessionManager.broadcast(message)
     }
     }
     
     

@@ -23,7 +23,7 @@ import javax.servlet.http.HttpServletResponse
 class McpFilter implements Filter {
 class McpFilter implements Filter {
     protected final static Logger logger = LoggerFactory.getLogger(McpFilter.class)
     protected final static Logger logger = LoggerFactory.getLogger(McpFilter.class)
     
     
-    private MoquiMcpServlet mcpServlet = new MoquiMcpServlet()
+    private EnhancedMcpServlet mcpServlet = new EnhancedMcpServlet()
     
     
     @Override
     @Override
     void init(FilterConfig filterConfig) throws ServletException {
     void init(FilterConfig filterConfig) throws ServletException {

@@ -58,12 +58,11 @@ class McpSessionManager {
         logger.info("Registered MCP session ${session.sessionId} (total: ${sessions.size()})")
         logger.info("Registered MCP session ${session.sessionId} (total: ${sessions.size()})")
         
         
         // Send welcome message to new session
         // Send welcome message to new session
-        def welcomeMessage = new McpSchema.JSONRPCMessage([
-            type: "welcome",
+        def welcomeMessage = new JsonRpcNotification("welcome", [
             sessionId: session.sessionId,
             sessionId: session.sessionId,
             totalSessions: sessions.size(),
             totalSessions: sessions.size(),
             timestamp: System.currentTimeMillis()
             timestamp: System.currentTimeMillis()
-        ], null)
+        ])
         session.sendMessage(welcomeMessage)
         session.sendMessage(welcomeMessage)
     }
     }
     
     
@@ -87,7 +86,7 @@ class McpSessionManager {
     /**
     /**
      * Broadcast message to all active sessions
      * Broadcast message to all active sessions
      */
      */
-    void broadcast(McpSchema.JSONRPCMessage message) {
+    void broadcast(JsonRpcMessage message) {
         if (isShuttingDown.get()) {
         if (isShuttingDown.get()) {
             logger.warn("Rejecting broadcast during shutdown")
             logger.warn("Rejecting broadcast during shutdown")
             return
             return
@@ -121,7 +120,7 @@ class McpSessionManager {
     /**
     /**
      * Send message to specific session
      * Send message to specific session
      */
      */
-    boolean sendToSession(String sessionId, McpSchema.JSONRPCMessage message) {
+    boolean sendToSession(String sessionId, JsonRpcMessage message) {
         def session = sessions.get(sessionId)
         def session = sessions.get(sessionId)
         if (!session) {
         if (!session) {
             return false
             return false
@@ -181,11 +180,10 @@ class McpSessionManager {
         logger.info("Initiating graceful MCP session manager shutdown")
         logger.info("Initiating graceful MCP session manager shutdown")
         
         
         // Send shutdown notification to all sessions
         // Send shutdown notification to all sessions
-        def shutdownMessage = new McpSchema.JSONRPCMessage([
-            type: "server_shutdown",
+        def shutdownMessage = new JsonRpcNotification("server_shutdown", [
             message: "Server is shutting down gracefully",
             message: "Server is shutting down gracefully",
             timestamp: System.currentTimeMillis()
             timestamp: System.currentTimeMillis()
-        ], null)
+        ])
         broadcast(shutdownMessage)
         broadcast(shutdownMessage)
         
         
         // Give sessions time to receive shutdown message
         // Give sessions time to receive shutdown message

@@ -13,93 +13,11 @@
  */
  */
 package org.moqui.mcp
 package org.moqui.mcp
 
 
-import groovy.json.JsonBuilder
-
 /**
 /**
- * MCP Transport interface compatible with Servlet 4.0 and Moqui Visit system
- * Provides SDK-style session management capabilities while maintaining compatibility
+ * Simple transport interface for MCP messages
  */
  */
 interface MoquiMcpTransport {
 interface MoquiMcpTransport {
-    /**
-     * Send a JSON-RPC message through this transport
-     * @param message The MCP JSON-RPC message to send
-     */
-    void sendMessage(McpSchema.JSONRPCMessage message)
-    
-    /**
-     * Close the transport gracefully, allowing in-flight messages to complete
-     */
-    void closeGracefully()
-    
-    /**
-     * Force close the transport immediately
-     */
-    void close()
-    
-    /**
-     * Check if the transport is still active
-     * @return true if transport is active, false otherwise
-     */
+    void sendMessage(JsonRpcMessage message)
     boolean isActive()
     boolean isActive()
-    
-    /**
-     * Get the session ID associated with this transport
-     * @return the MCP session ID
-     */
     String getSessionId()
     String getSessionId()
-    
-    /**
-     * Get the associated Moqui Visit ID
-     * @return the Visit ID if available, null otherwise
-     */
-    String getVisitId()
-}
-
-/**
- * Simple implementation of MCP JSON-RPC message schema
- * Compatible with MCP protocol specifications
- */
-class McpSchema {
-    static class JSONRPCMessage {
-        String jsonrpc = "2.0"
-        Object id
-        String method
-        Map params
-        Object result
-        Map error
-        
-        JSONRPCMessage(String method, Map params = null, Object id = null) {
-            this.method = method
-            this.params = params
-            this.id = id
-        }
-        
-        JSONRPCMessage(Object result, Object id) {
-            this.result = result
-            this.id = id
-        }
-        
-        JSONRPCMessage(Map error, Object id) {
-            this.error = error
-            this.id = id
-        }
-        
-        String toJson() {
-            return new JsonBuilder(this).toString()
-        }
-        
-        static JSONRPCMessage fromJson(String json) {
-            // Simple JSON parsing - in production would use proper JSON parser
-            def slurper = new groovy.json.JsonSlurper()
-            def data = slurper.parseText(json)
-            
-            if (data.error) {
-                return new JSONRPCMessage(data.error, data.id)
-            } else if (data.result != null) {
-                return new JSONRPCMessage(data.result, data.id)
-            } else {
-                return new JSONRPCMessage(data.method, data.params, data.id)
-            }
-        }
-    }
 }
 }
\ No newline at end of file

@@ -258,15 +258,18 @@ class ServiceBasedMcpServlet extends HttpServlet {
             
             
             logger.info("Service-Based MCP Message authenticated user: ${ec.user?.username}, userId: ${ec.user?.userId}")
             logger.info("Service-Based MCP Message authenticated user: ${ec.user?.username}, userId: ${ec.user?.userId}")
             
             
-            // If no user authenticated, try to authenticate as admin for MCP requests
+            // Require authentication - do not fallback to admin
             if (!ec.user?.userId) {
             if (!ec.user?.userId) {
-                logger.info("No user authenticated, attempting admin login for Service-Based MCP")
-                try {
-                    ec.user.loginUser("admin", "admin")
-                    logger.info("Service-Based MCP Admin login successful, user: ${ec.user?.username}")
-                } catch (Exception e) {
-                    logger.warn("Service-Based MCP Admin login failed: ${e.message}")
-                }
+                logger.warn("Service-Based MCP Request denied - no authenticated user")
+                // Handle error directly without sendError to avoid Moqui error screen interference
+                response.setStatus(HttpServletResponse.SC_UNAUTHORIZED)
+                response.setContentType("application/json")
+                response.writer.write(groovy.json.JsonOutput.toJson([
+                    jsonrpc: "2.0",
+                    error: [code: -32000, message: "Authentication required. Please provide valid credentials."],
+                    id: null
+                ]))
+                return
             }
             }
             
             
             // Handle different HTTP methods
             // Handle different HTTP methods
@@ -435,15 +438,18 @@ class ServiceBasedMcpServlet extends HttpServlet {
             // Initialize web facade for authentication
             // Initialize web facade for authentication
             ec.initWebFacade(webappName, request, response)
             ec.initWebFacade(webappName, request, response)
             
             
-            // If no user authenticated, try to authenticate as admin for MCP requests
+            // Require authentication - do not fallback to admin
             if (!ec.user?.userId) {
             if (!ec.user?.userId) {
-                logger.info("No user authenticated, attempting admin login for Legacy MCP")
-                try {
-                    ec.user.loginUser("admin", "admin")
-                    logger.info("Legacy MCP Admin login successful, user: ${ec.user?.username}")
-                } catch (Exception e) {
-                    logger.warn("Legacy MCP Admin login failed: ${e.message}")
-                }
+                logger.warn("Legacy MCP Request denied - no authenticated user")
+                // Handle error directly without sendError to avoid Moqui error screen interference
+                response.setStatus(HttpServletResponse.SC_UNAUTHORIZED)
+                response.setContentType("application/json")
+                response.writer.write(groovy.json.JsonOutput.toJson([
+                    jsonrpc: "2.0",
+                    error: [code: -32000, message: "Authentication required. Please provide valid credentials."],
+                    id: null
+                ]))
+                return
             }
             }
             
             
             // Read and parse JSON-RPC request (same as POST handling)
             // Read and parse JSON-RPC request (same as POST handling)

@@ -73,7 +73,7 @@ class VisitBasedMcpSession implements MoquiMcpTransport {
     }
     }
     
     
     @Override
     @Override
-    void sendMessage(McpSchema.JSONRPCMessage message) {
+    void sendMessage(JsonRpcMessage message) {
         if (!active.get() || closing.get()) {
         if (!active.get() || closing.get()) {
             logger.warn("Attempted to send message on inactive or closing session ${sessionId}")
             logger.warn("Attempted to send message on inactive or closing session ${sessionId}")
             return
             return
@@ -95,7 +95,6 @@ class VisitBasedMcpSession implements MoquiMcpTransport {
         }
         }
     }
     }
     
     
-    @Override
     void closeGracefully() {
     void closeGracefully() {
         if (!active.compareAndSet(true, false)) {
         if (!active.compareAndSet(true, false)) {
             return // Already closed
             return // Already closed
@@ -106,11 +105,10 @@ class VisitBasedMcpSession implements MoquiMcpTransport {
         
         
         try {
         try {
             // Send graceful shutdown notification
             // Send graceful shutdown notification
-            def shutdownMessage = new McpSchema.JSONRPCMessage([
-                type: "shutdown",
+            def shutdownMessage = new JsonRpcNotification("shutdown", [
                 sessionId: sessionId,
                 sessionId: sessionId,
                 timestamp: System.currentTimeMillis()
                 timestamp: System.currentTimeMillis()
-            ], null)
+            ])
             sendMessage(shutdownMessage)
             sendMessage(shutdownMessage)
             
             
             // Give some time for message to be sent
             // Give some time for message to be sent
@@ -123,7 +121,6 @@ class VisitBasedMcpSession implements MoquiMcpTransport {
         }
         }
     }
     }
     
     
-    @Override
     void close() {
     void close() {
         if (!active.compareAndSet(true, false)) {
         if (!active.compareAndSet(true, false)) {
             return // Already closed
             return // Already closed
@@ -160,7 +157,6 @@ class VisitBasedMcpSession implements MoquiMcpTransport {
         return sessionId
         return sessionId
     }
     }
     
     
-    @Override
     String getVisitId() {
     String getVisitId() {
         return visitId
         return visitId
     }
     }

@@ -21,18 +21,9 @@
 
 
     <!-- Service-Based MCP Servlet Configuration -->
     <!-- Service-Based MCP Servlet Configuration -->
     <servlet>
     <servlet>
-        <servlet-name>ServiceBasedMcpServlet</servlet-name>
-        <servlet-class>org.moqui.mcp.ServiceBasedMcpServlet</servlet-class>
+        <servlet-name>EnhancedMcpServlet</servlet-name>
+        <servlet-class>org.moqui.mcp.EnhancedMcpServlet</servlet-class>
         
         
-        <!-- Configuration Parameters -->
-        <init-param>
-            <param-name>sseEndpoint</param-name>
-            <param-value>/sse</param-value>
-        </init-param>
-        <init-param>
-            <param-name>messageEndpoint</param-name>
-            <param-value>/mcp/message</param-value>
-        </init-param>
         <init-param>
         <init-param>
             <param-name>keepAliveIntervalSeconds</param-name>
             <param-name>keepAliveIntervalSeconds</param-name>
             <param-value>30</param-value>
             <param-value>30</param-value>
@@ -49,20 +40,9 @@
         <load-on-startup>5</load-on-startup>
         <load-on-startup>5</load-on-startup>
     </servlet>
     </servlet>
     
     
-    <!-- Servlet Mappings -->
     <servlet-mapping>
     <servlet-mapping>
-        <servlet-name>ServiceBasedMcpServlet</servlet-name>
-        <url-pattern>/sse/*</url-pattern>
-    </servlet-mapping>
-    
-    <servlet-mapping>
-        <servlet-name>ServiceBasedMcpServlet</servlet-name>
-        <url-pattern>/mcp/message/*</url-pattern>
-    </servlet-mapping>
-    
-    <servlet-mapping>
-        <servlet-name>ServiceBasedMcpServlet</servlet-name>
-        <url-pattern>/rpc/*</url-pattern>
+        <servlet-name>EnhancedMcpServlet</servlet-name>
+        <url-pattern>/mcp/*</url-pattern>
     </servlet-mapping>
     </servlet-mapping>
     
     
     <!-- Session Configuration -->
     <!-- Session Configuration -->

-<?xml version="1.0" encoding="UTF-8"?>
-<web-app xmlns="http://xmlns.jcp.org/xml/ns/javaee"
-         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-         xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee 
-         http://xmlns.jcp.org/xml/ns/javaee/web-app_4_0.xsd"
-         version="4.0">
-    
-    <!-- MCP SSE Servlet Configuration -->
-    <servlet>
-        <servlet-name>EnhancedMcpServlet</servlet-name>
-        <servlet-class>org.moqui.mcp.EnhancedMcpServlet</servlet-class>
-        
-        <!-- Configuration parameters -->
-        <init-param>
-            <param-name>moqui-name</param-name>
-            <param-value>moqui-mcp-2</param-value>
-        </init-param>
-        
-        <init-param>
-            <param-name>sseEndpoint</param-name>
-            <param-value>/sse</param-value>
-        </init-param>
-        
-        <init-param>
-            <param-name>messageEndpoint</param-name>
-            <param-value>/mcp/message</param-value>
-        </init-param>
-        
-        <init-param>
-            <param-name>keepAliveIntervalSeconds</param-name>
-            <param-value>30</param-value>
-        </init-param>
-        
-        <init-param>
-            <param-name>maxConnections</param-name>
-            <param-value>100</param-value>
-        </init-param>
-        
-        <!-- Enable async support -->
-        <async-supported>true</async-supported>
-    </servlet>
-    
-    <!-- Servlet mappings for MCP SSE endpoints -->
-    <servlet-mapping>
-        <servlet-name>EnhancedMcpServlet</servlet-name>
-        <url-pattern>/sse/*</url-pattern>
-    </servlet-mapping>
-    
-    <servlet-mapping>
-        <servlet-name>EnhancedMcpServlet</servlet-name>
-        <url-pattern>/mcp/message/*</url-pattern>
-    </servlet-mapping>
-    
-    <!-- Session configuration -->
-    <session-config>
-        <session-timeout>30</session-timeout>
-        <cookie-config>
-            <http-only>true</http-only>
-            <secure>false</secure>
-        </cookie-config>
-    </session-config>
-    
-</web-app>
\ No newline at end of file