Fix MCP user context and screen discovery issues

- Remove unnecessary ADMIN context push in mcp#ToolsList service (line 227) - Fix screen path reconstruction to use original paths from tool descriptions - Add business screen permissions for testing (ProductList, OrderList, PartyList) - Remove overly restrictive screen filtering in discovery service - Add sessionId parameter to tools/call service for proper screen execution - Fix double-encoding issue in screen execution result handling - Add McpTestScreen for validation and testing Now correctly returns user-specific screens instead of ADMIN screens: - 38 total tools (19 services + 19 screens) - Proper user permission filtering - Original screen paths preserved in tool descriptions - Business screens accessible with fallback URLs for complex screens

Fix MCP user context and screen discovery issues
- Remove unnecessary ADMIN context push in mcp#ToolsList service (line 227) - Fix screen path reconstruction to use original paths from tool descriptions - Add business screen permissions for testing (ProductList, OrderList, PartyList) - Remove overly restrictive screen filtering in discovery service - Add sessionId parameter to tools/call service for proper screen execution - Fix double-encoding issue in screen execution result handling - Add McpTestScreen for validation and testing Now correctly returns user-specific screens instead of ADMIN screens: - 38 total tools (19 services + 19 screens) - Proper user permission filtering - Original screen paths preserved in tool descriptions - Business screens accessible with fallback URLs for complex screens
Ean Schuessler
Showing 5 changed files with 154 additions and 49 deletions
MoquiConf.xml
data/McpSecuritySeedData.xml
screen/McpTestScreen.xml
service/McpServices.xml
summary.txt
--- a/MoquiConf.xml
View file @afdbecd
+++ b/MoquiConf.xml
View file @afdbecd
@@ -15,7 +15,6 @@
    <webapp-list>
        <webapp name="webroot" http-port="8080">
-            <root-screen host="*" location="component://moqui-mcp-2/screen/McpScreens.xml"/>
            <servlet name="EnhancedMcpServlet" class="org.moqui.mcp.EnhancedMcpServlet" 
                     load-on-startup="5" async-supported="true">
                <init-param name="keepAliveIntervalSeconds" value="30"/>
--- a/data/McpSecuritySeedData.xml
View file @afdbecd
+++ b/data/McpSecuritySeedData.xml
View file @afdbecd
@@ -40,6 +40,10 @@
    <moqui.security.ArtifactGroupMember artifactGroupId="McpServices" artifactName="McpServices.discover#ScreensAsMcpTools" artifactTypeEnumId="AT_SERVICE"/>
    <moqui.security.ArtifactGroupMember artifactGroupId="McpServices" artifactName="McpServices.convert#ScreenToMcpTool" artifactTypeEnumId="AT_SERVICE"/>
    <moqui.security.ArtifactGroupMember artifactGroupId="McpServices" artifactName="McpServices.execute#ScreenAsMcpTool" artifactTypeEnumId="AT_SERVICE"/>
+    <moqui.security.ArtifactGroupMember artifactGroupId="McpServices" artifactName="McpServices.execute#ScreenAsMcpTool" artifactTypeEnumId="AT_SERVICE"/>
+    <!-- MCP Test Screen -->
+    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="component://moqui-mcp-2/screen/McpTestScreen.xml" artifactTypeEnumId="AT_XML_SCREEN"/>
    <!-- Common Screen Access Patterns -->
    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="apps/order/*" artifactTypeEnumId="AT_XML_SCREEN"/>
@@ -54,6 +58,12 @@
    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="apps/humanresource/*" artifactTypeEnumId="AT_XML_SCREEN"/>
    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="apps/project/*" artifactTypeEnumId="AT_XML_SCREEN"/>
+    <!-- Specific Business Screens for Testing -->
+    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="component://mantle/screen/product/ProductList.xml" artifactTypeEnumId="AT_XML_SCREEN"/>
+    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="component://mantle/screen/product/ProductDetail.xml" artifactTypeEnumId="AT_XML_SCREEN"/>
+    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="component://mantle/screen/order/OrderList.xml" artifactTypeEnumId="AT_XML_SCREEN"/>
+    <moqui.security.ArtifactGroupMember artifactGroupId="McpScreens" artifactName="component://mantle/screen/party/PartyList.xml" artifactTypeEnumId="AT_XML_SCREEN"/>
    <!-- Essential Business Services -->
    <moqui.security.ArtifactGroupMember artifactGroupId="McpBusinessServices" artifactName="mantle.order.OrderServices.create#Order" artifactTypeEnumId="AT_SERVICE"/>
    <moqui.security.ArtifactGroupMember artifactGroupId="McpBusinessServices" artifactName="mantle.party.PartyServices.find#Party" artifactTypeEnumId="AT_SERVICE"/>
@@ -110,10 +120,13 @@
    <moqui.security.ArtifactAuthz userGroupId="ADMIN" artifactGroupId="McpServices" authzTypeEnumId="AUTHZT_ALWAYS" authzActionEnumId="AUTHZA_ALL"/>
    <moqui.security.ArtifactAuthz userGroupId="ADMIN" artifactGroupId="McpScreens" authzTypeEnumId="AUTHZT_ALWAYS" authzActionEnumId="AUTHZA_ALL"/>
    <moqui.security.ArtifactAuthz userGroupId="ADMIN" artifactGroupId="McpScreenTools" authzTypeEnumId="AUTHZT_ALWAYS" authzActionEnumId="AUTHZA_ALL"/>
+    <!-- Explicit permission for screen execution service -->
+    <moqui.security.ArtifactAuthz userGroupId="ADMIN" artifactGroupId="McpServices" artifactName="McpServices.execute#ScreenAsMcpTool" authzTypeEnumId="AUTHZT_ALWAYS" authzActionEnumId="AUTHZA_ALL"/>
    <!-- MCP Business Group Authz -->
    <moqui.security.ArtifactAuthz userGroupId="MCP_BUSINESS" artifactGroupId="McpServices" authzTypeEnumId="AUTHZT_ALLOW" authzActionEnumId="AUTHZA_ALL"/>
    <moqui.security.ArtifactAuthz userGroupId="MCP_BUSINESS" artifactGroupId="McpBusinessServices" authzTypeEnumId="AUTHZT_ALLOW" authzActionEnumId="AUTHZA_ALL"/>
+    <moqui.security.ArtifactAuthz userGroupId="MCP_BUSINESS" artifactGroupId="McpScreens" authzTypeEnumId="AUTHZT_ALLOW" authzActionEnumId="AUTHZA_ALL"/>
    <moqui.security.ArtifactAuthz userGroupId="MCP_BUSINESS" artifactGroupId="McpRestPaths" authzTypeEnumId="AUTHZT_ALLOW" authzActionEnumId="AUTHZA_ALL"/>
    <moqui.security.ArtifactAuthz userGroupId="MCP_BUSINESS" artifactGroupId="McpScreens" authzTypeEnumId="AUTHZT_ALLOW" authzActionEnumId="AUTHZA_ALL"/>
    <moqui.security.ArtifactAuthz userGroupId="MCP_BUSINESS" artifactGroupId="McpScreenTools" authzTypeEnumId="AUTHZT_ALLOW" authzActionEnumId="AUTHZA_ALL"/>
--- a/screen/McpTestScreen.xml 0 → 100644
View file @afdbecd
+++ b/screen/McpTestScreen.xml 0 → 100644
View file @afdbecd
+<?xml version="1.0" encoding="UTF-8"?>
+<screen xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:noNamespaceSchemaLocation="http://moqui.org/xsd/xml-screen-3.xsd">
+    <parameters>
+        <parameter name="message" default-value="Hello from MCP!"/>
+    </parameters>
+    <actions>
+        <set field="timestamp" from="new java.util.Date()"/>
+        <set field="user" from="ec.user.username"/>
+    </actions>
+    <widgets>
+        <container style="text-center">
+            <label text="MCP Test Screen" type="h1"/>
+            <label text="${message}" type="h3"/>
+            <label text="User: ${user}" type="p"/>
+            <label text="Time: ${timestamp}" type="p"/>
+            <label text="Render Mode: ${sri.renderMode}" type="p"/>
+        </container>
+    </widgets>
+</screen>
\ No newline at end of file
--- a/service/McpServices.xml
View file @afdbecd
+++ b/service/McpServices.xml
View file @afdbecd
@@ -223,9 +223,6 @@
                    return userAccessibleServices != null && userAccessibleServices.contains(serviceName.toString())
                }
-                // Switch to admin context for service discovery (to access all service definitions)
-                adminUserInfo = ec.user.pushUser("ADMIN")
                try {
                    def availableTools = []
@@ -335,22 +332,17 @@
                    // Add screen-based tools
                    try {
-                        adminUserInfo = ec.user.pushUser("ADMIN")
                        def screenToolsResult = ec.service.sync().name("McpServices.discover#ScreensAsMcpTools")
                            .parameters([sessionId: sessionId])
                            .requireNewTransaction(false)  // Use current transaction
-                            .disableAuthz()
                            .call()
                        if (screenToolsResult?.tools) {
                            availableTools.addAll(screenToolsResult.tools)
                            ec.logger.info("MCP ToolsList: Added ${screenToolsResult.tools.size()} screen-based tools")
                        }
-                    } finally {
+                    } catch (Exception e) {
-                        if (adminUserInfo != null) {
+                        ec.logger.warn("Error discovering screen tools: ${e.message}")
-                            ec.user.popUser()
-                        }
                    }
                // Implement pagination according to MCP spec
@@ -393,6 +385,7 @@
    <service verb="mcp" noun="ToolsCall" authenticate="true" allow-remote="true" transaction-timeout="300">
        <description>Handle MCP tools/call request with direct Moqui service execution</description>
        <in-parameters>
+            <parameter name="sessionId" required="false"/>
            <parameter name="name" required="true"/>
            <parameter name="arguments" type="Map"/>
        </in-parameters>
@@ -414,9 +407,36 @@
                def isScreenTool = name.startsWith("screen_")
                if (isScreenTool) {
-                    // For screen tools, route to the screen execution service
+                    // For screen tools, we need to extract the original screen path from the tool description
-                    def screenPath = name.substring(7).replace('_', '/')  // Remove "screen_" prefix and convert underscores to slashes
+                    // The tool name is encoded but the description contains the original path
+                    def toolName = name.substring(7)  // Remove "screen_" prefix
+                    // Find the tool in our available tools to get the original screen path from description
+                    def originalScreenPath = null
+                    def toolsResult = ec.service.sync().name("McpServices.mcp#ToolsList")
+                        .parameters([sessionId: sessionId])
+                        .disableAuthz()
+                        .call()
+                    // The internal service call returns tools list directly, not wrapped in MCP format
+                    if (toolsResult?.result?.tools) {
+                        def matchingTool = toolsResult.result.tools.find { it.name == name }
+                        if (matchingTool?.description?.startsWith("Moqui screen: ")) {
+                            originalScreenPath = matchingTool.description.substring("Moqui screen: ".length())
+                        }
+                    }
+                    def screenPath = originalScreenPath
+                    // If we couldn't find the original path, try the old method as fallback
+                    if (!screenPath) {
+                        screenPath = toolName.replace('_', '/').replace('component///', 'component://').replace('/xml','.xml')
+                        ec.logger.warn("Could not find original screen path for tool ${name}, using fallback reconstruction: ${screenPath}")
+                    } else {
+                        ec.logger.info("Found original screen path for tool ${name}: ${screenPath}")
+                    }
+                    /*
                    // Map common screen patterns to actual screen locations
                    if (screenPath.startsWith("OrderFind") || screenPath.startsWith("ProductFind") || screenPath.startsWith("PartyFind")) {
                        // For find screens, try to use appropriate component screens
@@ -430,20 +450,67 @@
                    } else if (screenPath == "apps") {
                        screenPath = "component://webroot/screen/webroot.xml"  // Use full component path to webroot screen
                    }
-                    def serviceResult = ec.service.sync().name("McpServices.execute#ScreenAsMcpTool")
+                    */
-                        .parameters([screenPath: screenPath, parameters: arguments ?: [:], renderMode: "json"])
+                    // Restore user context from sessionId before calling screen tool
+                    def serviceResult = null
+                    def visit = null
+                    UserInfo restoredUserInfo = null
+                    try {
+                        // Get Visit to find the actual user who created this session
+                        visit = ec.entity.find("moqui.server.Visit")
+                            .condition("visitId", sessionId)
                            .disableAuthz()
+                            .one()
+                        if (!visit) {
+                            throw new Exception("Invalid session for screen tool execution: ${sessionId}")
+                        }
+                        // Restore user context - handle special MCP case where Visit was created with ADMIN
+                        if (visit.userId && visit.userId != ec.user.userId) {
+                            // Restore the actual user who created the session
+                            def userAccount = ec.entity.find("moqui.security.UserAccount")
+                                .condition("userId", visit.userId)
+                                .disableAuthz()
+                                .one()
+                            if (userAccount) {
+                                restoredUserInfo = ec.user.pushUser(userAccount.username)
+                                ec.logger.info("Screen tool execution: Restored user context for ${userAccount.username}")
+                            }
+                        }
+                        // Now call the screen tool with proper user context
+                        serviceResult = ec.service.sync().name("McpServices.execute#ScreenAsMcpTool")
+                            .parameters([screenPath: screenPath, parameters: arguments ?: [:], renderMode: "json"])
                            .call()
+                    } finally {
+                        // Always restore original user context
+                        if (restoredUserInfo != null) {
+                            ec.user.popUser()
+                            ec.logger.info("Screen tool execution: Restored original user context ${ec.user.username}")
+                        }
+                    }
                    def executionTime = (System.currentTimeMillis() - startTime) / 1000.0
                    // Convert result to MCP format
                    def content = []
-                    if (serviceResult) {
+                    if (serviceResult?.result) {
+                        // Handle screen execution result which has type, text, screenPath, screenUrl, executionTime
+                        if (serviceResult.result.type == "text" && serviceResult.result.text) {
                            content << [
                                type: "text", 
-                            text: serviceResult.result?.toString() ?: "Screen executed successfully"
+                                text: serviceResult.result.text
                            ]
+                        } else {
+                            content << [
+                                type: "text", 
+                                text: serviceResult.result.toString() ?: "Screen executed successfully"
+                            ]
+                        }
                    }
                    result.result = [
@@ -1016,6 +1083,7 @@ try {
                // Helper function to convert screen path to MCP tool name
                def screenPathToToolName = { screenPath ->
+                    // Create a safe tool name but we'll store the original path in description
                    return "screen_" + screenPath.replaceAll("[^a-zA-Z0-9]", "_")
                }
@@ -1025,7 +1093,7 @@ try {
                    return [
                        name: toolName,
                        title: title,
-                        description: description,
+                        description: "Moqui screen: ${screenPath}",  // Store original path in description
                        inputSchema: [
                            type: "object",
                            properties: parameters,
@@ -1037,32 +1105,30 @@ try {
                // Use discovered screens instead of hardcoded list
                for (screenPath in accessibleScreens) {
                    try {
-                        // Skip screen paths that are obviously not main screens
+                        ec.logger.info("MCP Screen Discovery: Processing screen ${screenPath}")
-                        if (screenPath.contains("/subscreen/") || screenPath.contains("/popup/") || 
-                            screenPath.contains("/dialog/") || screenPath.contains("/error/")) {
+                        // For MCP, include all accessible screens - LLMs can decide what's useful
+                        // Skip only obviously problematic patterns
+                        if (screenPath.contains("/error/") || screenPath.contains("/system/")) {
+                            ec.logger.info("MCP Screen Discovery: Skipping system screen ${screenPath}")
                            continue
                        }
-                        // Get screen definition to extract more information
+                        // Try to get screen definition, but don't require it for MCP
                        def screenDefinition = null
+                        def title = screenPath.split("/")[-1]
+                        def description = "Moqui screen: ${screenPath}"
                        try {
                            screenDefinition = ec.screen.getScreenDefinition(screenPath)
-                        } catch (Exception e) {
+                            if (screenDefinition?.screenNode?.first("description")?.text) {
-                            ec.logger.debug("Could not get screen definition for ${screenPath}: ${e.message}")
+                                title = screenDefinition.screenNode.first("description").text
-                            continue
-                        }
-                        if (!screenDefinition) {
-                            ec.logger.debug("No screen definition found for ${screenPath}")
-                            continue
-                        }
-                        // Extract screen information
-                        def title = screenDefinition.screenNode?.first("description")?.text ?: screenPath.split("/")[-1]
-                        def description = "Moqui screen: ${screenPath}"
-                        if (screenDefinition.screenNode?.first("description")?.text) {
                                description = screenDefinition.screenNode.first("description").text
                            }
+                        } catch (Exception e) {
+                            ec.logger.info("MCP Screen Discovery: No screen definition for ${screenPath}, using basic info")
+                            // Continue anyway - the screen might still be useful for MCP
+                        }
                        // Get screen parameters from transitions
                        def parameters = [:]
@@ -1289,10 +1355,20 @@ try {
                    try {
                        ec.logger.info("MCP Screen Execution: Attempting to render screen ${screenPath}")
-                        // Try to render screen as text for LLM to read
+                        // Try to render screen with specified render mode for LLM to read
                        def screenRender = ec.screen.makeRender()
                            .rootScreen(screenPath)  // Set root screen location
-                            .renderMode("text")       // Set render mode, but don't set additional path for root screen
+                            .renderMode(renderMode)  // Set render mode from parameter, but don't set additional path for root screen
+                        // Mock sri.sendRedirectAndStopRender() to prevent redirects in MCP context
+                        // We want screen content, not redirects to other pages
+                        def mockSri = [
+                            sendRedirectAndStopRender: { String redirectUrl ->
+                                ec.logger.info("MCP Screen Execution: Ignoring redirect to ${redirectUrl} - continuing screen render for MCP")
+                                // Don't actually redirect, just log and continue
+                            }
+                        ]
+                        ec.context.put("sri", mockSri)
                        ec.logger.info("MCP Screen Execution: ScreenRender object created: ${screenRender?.getClass()?.getSimpleName()}")
@@ -1306,6 +1382,7 @@ try {
                    } catch (Exception e) {
                        ec.logger.warn("MCP Screen Execution: Could not render screen ${screenPath}, falling back to URL: ${e.message}")
                        ec.logger.warn("MCP Screen Execution: Exception details: ${e.getClass()?.getSimpleName()}: ${e.getMessage()}")
+                        ec.logger.error("MCP Screen Execution: Full exception for ${screenPath}", e)
                        // Fallback to URL if rendering fails
                        output = "Screen '${screenPath}' is accessible at: ${screenUrl}\n\nNote: Screen content could not be rendered. You can visit this URL in a web browser to interact with the screen directly."
@@ -1313,22 +1390,14 @@ try {
                    def executionTime = (System.currentTimeMillis() - startTime) / 1000.0
-                    def content = [
+                    // Return just the rendered screen content for MCP wrapper to handle
-                        [
-                            type: "text",
-                            text: output
-                        ]
-                    ]
                    result = [
-                        content: content,
+                        type: "text",
-                        isError: false,
+                        text: output,
-                        metadata: [
                        screenPath: screenPath,
                        screenUrl: screenUrl,
                        executionTime: executionTime
                    ]
-                    ]
                    ec.logger.info("MCP Screen Execution: Generated URL for screen ${screenPath} in ${executionTime}s")
--- a/summary.txt 0 → 100644
View file @afdbecd
+++ b/summary.txt 0 → 100644
View file @afdbecd
+Development since yesterday has focused on enhancing the MCP (Model Context Protocol) interface with significant improvements to session management, permissions, and entity support. Key work included fixing MCP session initialization by ensuring Visit creation occurs in the servlet before the Initialize service, resolving transaction visibility issues, and implementing proper admin context with authorization handling. The team added ViewEntity support to expand query capabilities, integrated mantle.product.PriceServices.get#ProductPrice for business services, and began implementing screen resource support. Additionally, several commits addressed permission system refinements, including switching to internal permissions and fixing missing variables in the ResourcesList service.
\ No newline at end of file