Implement auto-approval for MCP audit records and enhance data actor …#4875
Implement auto-approval for MCP audit records and enhance data actor …#4875harshithb3304 wants to merge 1 commit intofeature/mini-runtime-releasefrom
Conversation
…methods for MCP audit info management
There was a problem hiding this comment.
Pull request overview
This PR adds MCP audit-record management plumbing to the DataActor abstraction and introduces an auto-approval step in the mini-runtime MCP tools sync job based on an “approved MCP servers” allowlist.
Changes:
- Add
DataActorAPIs for fetching approved MCP servers and fetching/updatingMcpAuditInfo. - Implement these APIs in
ClientActor(DB-abstractor calls) and add stub implementations inDbActor. - Add an auto-approval pass in
McpToolsSyncJobExecutorand store anapprovedAttimestamp onMcpAuditInfo.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/utils/src/main/java/com/akto/mcp/McpRequestResponseUtils.java | Adds allowlist check helper used by auto-approval logic. |
| libs/utils/src/main/java/com/akto/data_actor/DbActor.java | Adds new DataActor method overrides (currently stubbed). |
| libs/utils/src/main/java/com/akto/data_actor/DataActor.java | Extends DataActor contract with MCP allowlist/audit-info methods. |
| libs/utils/src/main/java/com/akto/data_actor/ClientActor.java | Implements new MCP allowlist + audit-info fetch/update via DB-abstractor endpoints. |
| libs/dao/src/main/java/com/akto/dto/McpAuditInfo.java | Adds approvedAt field for audit records. |
| apps/mini-runtime/src/main/java/com/akto/hybrid_runtime/McpToolsSyncJobExecutor.java | Adds auto-approval pass for existing pending MCP audit records. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| List<McpAuditInfo> pendingRecords = DataActorFactory.fetchInstance() | ||
| .fetchMcpAuditInfo(null, Collections.emptyList()); | ||
| if (pendingRecords.isEmpty()) { | ||
| logger.debug("No pending MCP audit records found for auto-approval."); | ||
| return; | ||
| } | ||
| logger.info("Found {} MCP audit records to check for auto-approval.", pendingRecords.size()); | ||
| int approvedCount = 0; |
There was a problem hiding this comment.
autoApproveExistingPendingRecords() calls fetchMcpAuditInfo(null, Collections.emptyList()), but ClientActor.fetchMcpAuditInfo only includes remarksList in the request when it’s non-empty. As written, this call won’t send any remarks filter and can end up fetching all MCP audit records, which is risky for runtime performance/DB-abstractor load. Consider adding an explicit “pending only” filter/parameter (or passing a non-empty remarks filter) so the server can filter before returning results.
| String mcpHost = record.getMcpHost(); | ||
| if (StringUtils.isBlank(mcpHost)) { | ||
| continue; | ||
| } | ||
| if ("Approved".equals(McpRequestResponseUtils.checkIfServerIsApproved(mcpHost))) { | ||
| record.setRemarks("Approved"); | ||
| record.setMarkedBy("System (Auto-approved)"); | ||
| record.setUpdatedTimestamp(currentTime); | ||
| record.setApprovedAt(currentTime); | ||
| DataActorFactory.fetchInstance().updateMcpAuditInfo(record); |
There was a problem hiding this comment.
Inside the loop you call McpRequestResponseUtils.checkIfServerIsApproved(mcpHost), which (currently) fetches the approved servers list each time. This creates an N+1 remote-call pattern and can significantly slow down the job when there are many pending records. Fetch the approved servers list once (e.g., build a Set<String> of approved hosts) before the loop and do in-memory membership checks per record.
| BasicDBList objList = (BasicDBList) payloadObj.get("mcpAuditInfoList"); | ||
| for (Object obj2 : objList) { |
There was a problem hiding this comment.
fetchMcpAuditInfo assumes the response always contains a non-null mcpAuditInfoList. If the server returns {} or omits the key, objList will be null and the enhanced for-loop will throw an NPE. Add a null/type check for mcpAuditInfoList before iterating (and treat missing/invalid payload as an empty list).
| BasicDBList objList = (BasicDBList) payloadObj.get("mcpAuditInfoList"); | |
| for (Object obj2 : objList) { | |
| Object objListValue = payloadObj.get("mcpAuditInfoList"); | |
| if (!(objListValue instanceof BasicDBList)) { | |
| return mcpAuditInfoList; | |
| } | |
| BasicDBList objList = (BasicDBList) objListValue; | |
| for (Object obj2 : objList) { | |
| if (!(obj2 instanceof BasicDBObject)) { | |
| continue; | |
| } |
| return new ArrayList<>(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<McpAuditInfo> fetchMcpAuditInfo(Integer updatedAfter, List<String> remarksList) { | ||
| return new ArrayList<>(); | ||
| } | ||
|
|
||
| @Override | ||
| public void updateMcpAuditInfo(McpAuditInfo auditInfo) { |
There was a problem hiding this comment.
These new DataActor overrides in DbActor currently return empty lists / no-op. In NORMAL (non-hybrid) runtime mode, this will silently disable MCP auto-approval and any future callers of these APIs, which can be very hard to debug. Either implement these methods via DbLayer/DAOs, or explicitly throw an UnsupportedOperationException with a clear message if the functionality is intended to be hybrid-only.
| return new ArrayList<>(); | |
| } | |
| @Override | |
| public List<McpAuditInfo> fetchMcpAuditInfo(Integer updatedAfter, List<String> remarksList) { | |
| return new ArrayList<>(); | |
| } | |
| @Override | |
| public void updateMcpAuditInfo(McpAuditInfo auditInfo) { | |
| throw new UnsupportedOperationException( | |
| "fetchAllowedMcpServers is not supported in DbActor for NORMAL (non-hybrid) runtime mode."); | |
| } | |
| @Override | |
| public List<McpAuditInfo> fetchMcpAuditInfo(Integer updatedAfter, List<String> remarksList) { | |
| throw new UnsupportedOperationException( | |
| "fetchMcpAuditInfo is not supported in DbActor for NORMAL (non-hybrid) runtime mode."); | |
| } | |
| @Override | |
| public void updateMcpAuditInfo(McpAuditInfo auditInfo) { | |
| throw new UnsupportedOperationException( | |
| "updateMcpAuditInfo is not supported in DbActor for NORMAL (non-hybrid) runtime mode."); |
| public static String checkIfServerIsApproved(String serverName) { | ||
| if (StringUtils.isBlank(serverName)) { | ||
| return null; | ||
| } | ||
| try { | ||
| List<Map<String, Object>> approvedServers = dataActor.fetchAllowedMcpServers(); | ||
| for (Map<String, Object> server : approvedServers) { | ||
| Object nameObj = server.get("name"); | ||
| if (nameObj != null && serverName.equalsIgnoreCase(nameObj.toString())) { | ||
| return "Approved"; | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| logger.error("Error checking approved servers list", e); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
checkIfServerIsApproved returns the literal string "Approved" or null. This makes the API contract unclear (method name suggests boolean) and forces callers to depend on a magic string. Prefer returning boolean (or an enum/status) and have callers set remarks separately.
| public static String checkIfServerIsApproved(String serverName) { | |
| if (StringUtils.isBlank(serverName)) { | |
| return null; | |
| } | |
| try { | |
| List<Map<String, Object>> approvedServers = dataActor.fetchAllowedMcpServers(); | |
| for (Map<String, Object> server : approvedServers) { | |
| Object nameObj = server.get("name"); | |
| if (nameObj != null && serverName.equalsIgnoreCase(nameObj.toString())) { | |
| return "Approved"; | |
| } | |
| } | |
| } catch (Exception e) { | |
| logger.error("Error checking approved servers list", e); | |
| } | |
| return null; | |
| public static boolean checkIfServerIsApproved(String serverName) { | |
| if (StringUtils.isBlank(serverName)) { | |
| return false; | |
| } | |
| try { | |
| List<Map<String, Object>> approvedServers = dataActor.fetchAllowedMcpServers(); | |
| for (Map<String, Object> server : approvedServers) { | |
| Object nameObj = server.get("name"); | |
| if (nameObj != null && serverName.equalsIgnoreCase(nameObj.toString())) { | |
| return true; | |
| } | |
| } | |
| } catch (Exception e) { | |
| logger.error("Error checking approved servers list", e); | |
| } | |
| return false; |
…methods for MCP audit info management