From fd69b6b3c7cd67ff6343f1035efb0a614d2e5df9 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Wed, 10 Jun 2026 14:25:57 +0200 Subject: [PATCH 1/5] =?UTF-8?q?feat(security):=20IDOR=20remediation=20?= =?UTF-8?q?=E2=80=94=20ownership=20validation=20for=20conversations,=20mem?= =?UTF-8?q?ories,=20groups?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NEW: OwnershipValidator utility (validateUserAccess, validateAndResolveUserId, requireOwnerOrAdmin) - RestAgentEngine: ownership checks on all conversation-scoped endpoints, userId resolution on startConversation - RestUserMemoryStore: ownership checks on all userId-scoped endpoints - RestGroupConversation: ownership checks on read/delete, userId validation on discuss, filtered list for non-admins - IRestGdprAdmin: moved @RolesAllowed to interface level (was implementation-only) - RestA2AEndpoint: added @PermitAll to GET discovery endpoints (documents intentional public access) - McpToolUtils: added requireOwnerOrAdmin() for MCP-level ownership validation - McpMemoryTools: added ownership checks to all read tools (userId param validated against caller) Admin role (eddi-admin) bypasses all ownership checks. All checks are no-ops when auth is disabled. --- docs/changelog.md | 44 +++++ .../properties/rest/RestUserMemoryStore.java | 18 +- .../labs/eddi/engine/a2a/RestA2AEndpoint.java | 6 + .../labs/eddi/engine/gdpr/IRestGdprAdmin.java | 2 + .../labs/eddi/engine/gdpr/RestGdprAdmin.java | 2 - .../eddi/engine/internal/RestAgentEngine.java | 47 +++++- .../internal/RestGroupConversation.java | 41 ++++- .../labs/eddi/engine/mcp/McpMemoryTools.java | 6 + .../ai/labs/eddi/engine/mcp/McpToolUtils.java | 30 ++++ .../engine/security/OwnershipValidator.java | 158 ++++++++++++++++++ 10 files changed, 343 insertions(+), 11 deletions(-) create mode 100644 src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java diff --git a/docs/changelog.md b/docs/changelog.md index e73210dd4..2459b33cf 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -4,6 +4,50 @@ --- +## 🛡️ Security Audit Remediation — IDOR Prevention & Ownership Validation (2026-06-10) + +**Repo:** EDDI (`fix/security-audit-idor-remediation`) +**What changed:** Addressed 5 findings from a comprehensive security audit. Added resource ownership validation across all conversation, user memory, and group conversation REST endpoints. Hardened GDPR, A2A, and MCP annotations. + +### Finding: IDOR — Conversations (HIGH → FIXED) +- **Problem:** Any authenticated user with `eddi-user` role could read/modify ANY conversation by guessing the conversationId. No ownership validation existed despite `ConversationDescriptor` having a `userId` field. +- **Fix:** `RestAgentEngine` now injects `SecurityIdentity`, `OwnershipValidator`, and `IConversationDescriptorStore`. All conversation-scoped endpoints (`readConversation`, `say`, `endConversation`, `undo`, `redo`, `rerun`, `readConversationLog`, `getConversationState`) validate that the caller owns the conversation. `startConversation` validates that the provided `userId` matches the caller's identity (admins can set any userId). + +### Finding: IDOR — User Memory (HIGH → FIXED) +- **Problem:** Any authenticated user could read/delete another user's persistent memories via the `/usermemorystore/memories/{userId}` endpoints. +- **Fix:** `RestUserMemoryStore` now injects `SecurityIdentity` and `OwnershipValidator`. All endpoints validate that the `{userId}` path parameter matches the authenticated caller. `upsertMemory` validates against the `userId` in the request body. + +### Finding: IDOR — Group Conversations (HIGH → FIXED) +- **Problem:** Any authenticated user could read/delete any group conversation. +- **Fix:** `RestGroupConversation` now validates ownership on `readGroupConversation` and `deleteGroupConversation`. `listGroupConversations` filters results to only the caller's conversations. `discuss`/`discussStreaming` validate the provided userId. + +### Finding: GDPR Annotation on Implementation Only (MEDIUM → FIXED) +- **Problem:** `@RolesAllowed("eddi-admin")` was only on `RestGdprAdmin` implementation, not the `IRestGdprAdmin` interface. Fragile to refactoring. +- **Fix:** Moved `@RolesAllowed("eddi-admin")` to the interface level. + +### Finding: A2A Endpoint Annotation Clarity (MEDIUM → FIXED) +- **Problem:** A2A GET discovery endpoints had no explicit security annotations, making intent unclear. +- **Fix:** Added `@PermitAll` to all 5 GET discovery endpoints to document intentional public access per A2A protocol spec. + +### Finding: MCP Memory Ownership (NEW → FIXED) +- **Problem:** MCP memory read tools (`list_user_memories`, `get_visible_memories`, etc.) accepted `userId` as a tool parameter without validating against the caller's identity. +- **Fix:** Added `requireOwnerOrAdmin()` to `McpToolUtils` and applied it in all 5 read-only MCP memory tools. + +### New Component: OwnershipValidator +- Centralized `@ApplicationScoped` utility for ownership checks +- Three methods: `validateUserAccess()`, `validateAndResolveUserId()`, `requireOwnerOrAdmin()` +- All checks are no-ops when `authorization.enabled=false` (dev mode) +- `eddi-admin` role bypasses all ownership checks +- Legacy data without ownership (null/blank userId) is allowed through gracefully +- WARN-level audit logging on all ownership check failures + +### Dropped Finding: MCP Unauthenticated by Default +- **Rationale:** When OIDC is disabled, ALL endpoints are unauthenticated — MCP is not uniquely vulnerable. `AuthStartupGuard` already prevents accidental unauthenticated production deployments. Not a finding. + +**Files:** `OwnershipValidator.java` [NEW], `RestAgentEngine.java`, `RestUserMemoryStore.java`, `RestGroupConversation.java`, `IRestGdprAdmin.java`, `RestGdprAdmin.java`, `RestA2AEndpoint.java`, `McpToolUtils.java`, `McpMemoryTools.java` + +--- + ## 🐛 Fix: Swagger UI Broken by CSP — Per-Path Filter Override (2026-06-03) **Repo:** EDDI (`fix/swagger-ui-csp`) diff --git a/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java b/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java index 0537a6803..880b58227 100644 --- a/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java +++ b/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java @@ -8,6 +8,8 @@ import ai.labs.eddi.configs.properties.IUserMemoryStore; import ai.labs.eddi.configs.properties.model.UserMemoryEntry; import ai.labs.eddi.datastore.IResourceStore; +import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.identity.SecurityIdentity; import org.jboss.logging.Logger; import jakarta.enterprise.context.ApplicationScoped; @@ -28,16 +30,23 @@ public class RestUserMemoryStore implements IRestUserMemoryStore { private final IUserMemoryStore userMemoryStore; + private final SecurityIdentity identity; + private final OwnershipValidator ownershipValidator; private static final Logger LOGGER = Logger.getLogger(RestUserMemoryStore.class); @Inject - public RestUserMemoryStore(IUserMemoryStore userMemoryStore) { + public RestUserMemoryStore(IUserMemoryStore userMemoryStore, + SecurityIdentity identity, + OwnershipValidator ownershipValidator) { this.userMemoryStore = userMemoryStore; + this.identity = identity; + this.ownershipValidator = ownershipValidator; } @Override public List getAllMemories(String userId) { + ownershipValidator.validateUserAccess(identity, userId); try { return userMemoryStore.getAllEntries(userId); } catch (IResourceStore.ResourceStoreException e) { @@ -48,6 +57,7 @@ public List getAllMemories(String userId) { @Override public List getVisibleMemories(String userId, String agentId, List groupIds, String recallOrder, int maxEntries) { + ownershipValidator.validateUserAccess(identity, userId); try { return userMemoryStore.getVisibleEntries(userId, agentId, groupIds != null ? groupIds : List.of(), recallOrder, maxEntries); } catch (IResourceStore.ResourceStoreException e) { @@ -58,6 +68,7 @@ public List getVisibleMemories(String userId, String agentId, L @Override public List searchMemories(String userId, String query) { + ownershipValidator.validateUserAccess(identity, userId); try { return userMemoryStore.filterEntries(userId, query); } catch (IResourceStore.ResourceStoreException e) { @@ -68,6 +79,7 @@ public List searchMemories(String userId, String query) { @Override public List getMemoriesByCategory(String userId, String category) { + ownershipValidator.validateUserAccess(identity, userId); try { return userMemoryStore.getEntriesByCategory(userId, category); } catch (IResourceStore.ResourceStoreException e) { @@ -78,6 +90,7 @@ public List getMemoriesByCategory(String userId, String categor @Override public Response getMemoryByKey(String userId, String key) { + ownershipValidator.validateUserAccess(identity, userId); try { var entry = userMemoryStore.getByKey(userId, key); if (entry.isPresent()) { @@ -105,6 +118,7 @@ public Response upsertMemory(UserMemoryEntry entry) { if (entry.key().length() > 255) { return Response.status(Response.Status.BAD_REQUEST).entity(Map.of("error", "key must not exceed 255 characters")).build(); } + ownershipValidator.validateUserAccess(identity, entry.userId()); try { String id = userMemoryStore.upsert(entry); return Response.ok(Map.of("id", id)).build(); @@ -127,6 +141,7 @@ public Response deleteMemory(String entryId) { @Override public Response deleteAllForUser(String userId) { + ownershipValidator.validateUserAccess(identity, userId); try { userMemoryStore.deleteAllForUser(userId); return Response.noContent().build(); @@ -138,6 +153,7 @@ public Response deleteAllForUser(String userId) { @Override public Response countMemories(String userId) { + ownershipValidator.validateUserAccess(identity, userId); try { long count = userMemoryStore.countEntries(userId); return Response.ok(Map.of("userId", userId, "count", count)).build(); diff --git a/src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java b/src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java index 7db2f336b..e5c5d0dcb 100644 --- a/src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java +++ b/src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java @@ -7,6 +7,7 @@ import ai.labs.eddi.configs.agents.CapabilityRegistryService; import ai.labs.eddi.configs.agents.CapabilityRegistryService.CapabilityMatch; import ai.labs.eddi.engine.a2a.A2AModels.*; +import jakarta.annotation.security.PermitAll; import jakarta.inject.Inject; import jakarta.ws.rs.*; import jakarta.ws.rs.core.MediaType; @@ -63,6 +64,7 @@ public RestA2AEndpoint(AgentCardService agentCardService, A2ATaskHandler taskHan /** * Default Agent Card — returns the first A2A-enabled agent's card. */ + @PermitAll @GET @Path(".well-known/agent.json") public Response getDefaultAgentCard() { @@ -81,6 +83,7 @@ public Response getDefaultAgentCard() { /** * Per-agent Agent Card. */ + @PermitAll @GET @Path("a2a/agents/{agentId}/agent.json") public Response getAgentCard(@PathParam("agentId") String agentId) { @@ -99,6 +102,7 @@ public Response getAgentCard(@PathParam("agentId") String agentId) { /** * List all A2A-enabled agents. */ + @PermitAll @GET @Path("a2a/agents") public Response listA2AAgents() { @@ -117,6 +121,7 @@ public Response listA2AAgents() { * Path follows the well-known URI convention, same auth model as * {@code /.well-known/agent.json}. */ + @PermitAll @GET @Path(".well-known/capabilities") @Tag(name = "06. Capability Registry", description = "A2A agent capability discovery") @@ -145,6 +150,7 @@ public Response searchCapabilities(@QueryParam("skill") String skill, * Public endpoint listing all registered skill names. Gated behind * {@code eddi.a2a.capabilities.public} (default {@code false}). */ + @PermitAll @GET @Path(".well-known/capabilities/skills") @Tag(name = "06. Capability Registry", description = "A2A agent capability discovery") diff --git a/src/main/java/ai/labs/eddi/engine/gdpr/IRestGdprAdmin.java b/src/main/java/ai/labs/eddi/engine/gdpr/IRestGdprAdmin.java index 15ef3d5ab..6a1fa395a 100644 --- a/src/main/java/ai/labs/eddi/engine/gdpr/IRestGdprAdmin.java +++ b/src/main/java/ai/labs/eddi/engine/gdpr/IRestGdprAdmin.java @@ -4,6 +4,7 @@ */ package ai.labs.eddi.engine.gdpr; +import jakarta.annotation.security.RolesAllowed; import jakarta.ws.rs.*; import jakarta.ws.rs.core.MediaType; import org.eclipse.microprofile.openapi.annotations.Operation; @@ -23,6 +24,7 @@ @Path("/admin/gdpr") @Tag(name = "GDPR / Privacy") @Produces(MediaType.APPLICATION_JSON) +@RolesAllowed("eddi-admin") public interface IRestGdprAdmin { @DELETE diff --git a/src/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.java b/src/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.java index 495439ccf..dadf7ac30 100644 --- a/src/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.java +++ b/src/main/java/ai/labs/eddi/engine/gdpr/RestGdprAdmin.java @@ -4,7 +4,6 @@ */ package ai.labs.eddi.engine.gdpr; -import jakarta.annotation.security.RolesAllowed; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import jakarta.ws.rs.BadRequestException; @@ -20,7 +19,6 @@ * @since 6.0.0 */ @ApplicationScoped -@RolesAllowed("eddi-admin") public class RestGdprAdmin implements IRestGdprAdmin { private static final Logger LOGGER = Logger.getLogger(RestGdprAdmin.class); diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java index b57ed869a..29a8030ab 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java @@ -10,11 +10,15 @@ import ai.labs.eddi.engine.api.IConversationService.*; import ai.labs.eddi.engine.gdpr.ProcessingRestrictedException; import ai.labs.eddi.engine.api.IRestAgentEngine; +import ai.labs.eddi.engine.memory.descriptor.IConversationDescriptorStore; import ai.labs.eddi.engine.memory.model.SimpleConversationMemorySnapshot; import ai.labs.eddi.engine.model.Context; import ai.labs.eddi.engine.memory.model.ConversationState; import ai.labs.eddi.engine.model.Deployment.Environment; import ai.labs.eddi.engine.model.InputData; +import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.ForbiddenException; +import io.quarkus.security.identity.SecurityIdentity; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import jakarta.ws.rs.InternalServerErrorException; @@ -46,13 +50,23 @@ public class RestAgentEngine implements IRestAgentEngine { private final IConversationService conversationService; + private final IConversationDescriptorStore conversationDescriptorStore; + private final SecurityIdentity identity; + private final OwnershipValidator ownershipValidator; private final int agentTimeout; private static final Logger LOGGER = Logger.getLogger(RestAgentEngine.class); @Inject - public RestAgentEngine(IConversationService conversationService, @ConfigProperty(name = "systemRuntime.agentTimeoutInSeconds") int agentTimeout) { + public RestAgentEngine(IConversationService conversationService, + IConversationDescriptorStore conversationDescriptorStore, + SecurityIdentity identity, + OwnershipValidator ownershipValidator, + @ConfigProperty(name = "systemRuntime.agentTimeoutInSeconds") int agentTimeout) { this.conversationService = conversationService; + this.conversationDescriptorStore = conversationDescriptorStore; + this.identity = identity; + this.ownershipValidator = ownershipValidator; this.agentTimeout = agentTimeout; } @@ -64,7 +78,8 @@ public Response startConversation(String agentId, Environment environment, Strin @Override public Response startConversationWithContext(String agentId, Environment environment, String userId, Map context) { try { - var result = conversationService.startConversation(environment, agentId, userId, context); + String resolvedUserId = ownershipValidator.validateAndResolveUserId(identity, userId); + var result = conversationService.startConversation(environment, agentId, resolvedUserId, context); return Response.created(result.conversationUri()).build(); } catch (ProcessingRestrictedException e) { LOGGER.warnf("GDPR processing restricted for user: %s", e.getMessage()); @@ -80,6 +95,7 @@ public Response startConversationWithContext(String agentId, Environment environ @Override public Response endConversation(String conversationId) { + validateConversationOwnership(conversationId); conversationService.endConversation(conversationId); return Response.ok().build(); } @@ -87,6 +103,7 @@ public Response endConversation(String conversationId) { @Override public SimpleConversationMemorySnapshot readConversation(String conversationId, Boolean returnDetailed, Boolean returnCurrentStepOnly, List returningFields) { + validateConversationOwnership(conversationId); try { return conversationService.readConversation(conversationId, returnDetailed, returnCurrentStepOnly, returningFields); } catch (ResourceStoreException e) { @@ -99,6 +116,7 @@ public SimpleConversationMemorySnapshot readConversation(String conversationId, @Override public Response readConversationLog(String conversationId, String outputType, Integer logSize) { + validateConversationOwnership(conversationId); try { var result = conversationService.readConversationLog(conversationId, outputType, logSize); return Response.ok(result.content(), result.mediaType()).build(); @@ -112,6 +130,7 @@ public Response readConversationLog(String conversationId, String outputType, In @Override public ConversationState getConversationState(String conversationId) { + validateConversationOwnership(conversationId); return conversationService.getConversationState(conversationId); } @@ -120,6 +139,7 @@ public void rerunLastConversationStep(String conversationId, String language, Bo List returningFields, final AsyncResponse response) { checkNotNull(conversationId, "conversationId"); checkNotEmpty(language, "language"); + validateConversationOwnership(conversationId); sayInternal(conversationId, returnDetailed, returnCurrentStepOnly, returningFields, new InputData("", Map.of(KEY_LANG, new Context(string, language))), true, response); @@ -139,6 +159,7 @@ public void sayWithinContext(final String conversationId, final Boolean returnDe checkNotNull(conversationId, "conversationId"); checkNotNull(inputData, "inputData"); checkNotNull(inputData.getInput(), "inputData.input"); + validateConversationOwnership(conversationId); sayInternal(conversationId, returnDetailed, returnCurrentStepOnly, returningFields, inputData, false, response); } @@ -172,6 +193,7 @@ private void sayInternal(String conversationId, Boolean returnDetailed, Boolean @Override public Boolean isUndoAvailable(String conversationId) { + validateConversationOwnership(conversationId); try { return conversationService.isUndoAvailable(conversationId); } catch (ResourceStoreException e) { @@ -184,6 +206,7 @@ public Boolean isUndoAvailable(String conversationId) { @Override public Response undo(final String conversationId) { + validateConversationOwnership(conversationId); try { boolean performed = conversationService.undo(conversationId); return performed ? Response.ok().build() : Response.status(Response.Status.CONFLICT).build(); @@ -197,6 +220,7 @@ public Response undo(final String conversationId) { @Override public Boolean isRedoAvailable(final String conversationId) { + validateConversationOwnership(conversationId); try { return conversationService.isRedoAvailable(conversationId); } catch (ResourceStoreException e) { @@ -209,6 +233,7 @@ public Boolean isRedoAvailable(final String conversationId) { @Override public Response redo(final String conversationId) { + validateConversationOwnership(conversationId); try { boolean performed = conversationService.redo(conversationId); return performed ? Response.ok().build() : Response.status(Response.Status.CONFLICT).build(); @@ -219,4 +244,22 @@ public Response redo(final String conversationId) { throw new InternalServerErrorException("Failed to redo"); } } + + /** + * Validates that the caller owns the conversation identified by + * {@code conversationId}. Admin role bypasses the check. If the descriptor + * cannot be loaded (e.g., not found), the check is skipped and the actual + * operation will handle the error. + */ + private void validateConversationOwnership(String conversationId) { + try { + var descriptor = conversationDescriptorStore.readDescriptor(conversationId, 0); + ownershipValidator.requireOwnerOrAdmin(identity, descriptor.getUserId(), "conversation"); + } catch (ForbiddenException e) { + throw e; // re-throw ownership errors + } catch (Exception e) { + // Descriptor not found or store error — let the actual operation handle it + LOGGER.debugf("Could not validate conversation ownership for %s: %s", conversationId, e.getMessage()); + } + } } diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java b/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java index c577308be..ab9ccd391 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java @@ -11,6 +11,9 @@ import ai.labs.eddi.engine.api.IGroupConversationService.GroupDiscussionEventListener; import ai.labs.eddi.engine.api.IRestGroupConversation; import ai.labs.eddi.engine.lifecycle.GroupConversationEventSink; +import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.ForbiddenException; +import io.quarkus.security.identity.SecurityIdentity; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import jakarta.ws.rs.core.Response; @@ -35,17 +38,25 @@ public class RestGroupConversation implements IRestGroupConversation { private final IGroupConversationService groupConversationService; private final IJsonSerialization jsonSerialization; + private final SecurityIdentity identity; + private final OwnershipValidator ownershipValidator; @Inject - public RestGroupConversation(IGroupConversationService groupConversationService, IJsonSerialization jsonSerialization) { + public RestGroupConversation(IGroupConversationService groupConversationService, + IJsonSerialization jsonSerialization, + SecurityIdentity identity, + OwnershipValidator ownershipValidator) { this.groupConversationService = groupConversationService; this.jsonSerialization = jsonSerialization; + this.identity = identity; + this.ownershipValidator = ownershipValidator; } @Override public Response discuss(String groupId, DiscussRequest request) { try { - String userId = request.userId() != null ? request.userId() : "anonymous"; + String userId = ownershipValidator.validateAndResolveUserId(identity, request.userId()); + if (userId == null || userId.isBlank()) userId = "anonymous"; GroupConversation gc = groupConversationService.discuss(groupId, request.question(), userId, 0); URI location = URI.create("/groups/" + groupId + "/conversations/" + gc.getId()); return Response.created(location).entity(gc).build(); @@ -62,7 +73,8 @@ public Response discuss(String groupId, DiscussRequest request) { @Override public void discussStreaming(String groupId, DiscussRequest request, SseEventSink eventSink, Sse sse) { try { - String userId = request.userId() != null ? request.userId() : "anonymous"; + String userId = ownershipValidator.validateAndResolveUserId(identity, request.userId()); + if (userId == null || userId.isBlank()) userId = "anonymous"; GroupDiscussionEventListener listener = new GroupDiscussionEventListener() { @Override @@ -125,7 +137,11 @@ public void onGroupError(GroupConversationEventSink.GroupErrorEvent event) { @Override public GroupConversation readGroupConversation(String groupId, String groupConversationId) { try { - return groupConversationService.readGroupConversation(groupConversationId); + GroupConversation gc = groupConversationService.readGroupConversation(groupConversationId); + ownershipValidator.requireOwnerOrAdmin(identity, gc.getUserId(), "group conversation"); + return gc; + } catch (ForbiddenException e) { + throw e; } catch (IResourceStore.ResourceNotFoundException | IResourceStore.ResourceStoreException e) { throw sneakyThrow(e); } @@ -134,9 +150,13 @@ public GroupConversation readGroupConversation(String groupId, String groupConve @Override public Response deleteGroupConversation(String groupId, String groupConversationId) { try { + GroupConversation gc = groupConversationService.readGroupConversation(groupConversationId); + ownershipValidator.requireOwnerOrAdmin(identity, gc.getUserId(), "group conversation"); groupConversationService.deleteGroupConversation(groupConversationId); return Response.ok().build(); - } catch (IResourceStore.ResourceStoreException e) { + } catch (ForbiddenException e) { + throw e; + } catch (IResourceStore.ResourceNotFoundException | IResourceStore.ResourceStoreException e) { throw sneakyThrow(e); } } @@ -144,7 +164,16 @@ public Response deleteGroupConversation(String groupId, String groupConversation @Override public List listGroupConversations(String groupId, Integer index, Integer limit) { try { - return groupConversationService.listGroupConversations(groupId, index, limit); + List conversations = groupConversationService.listGroupConversations(groupId, index, limit); + // Filter to owned conversations unless admin + if (ownershipValidator.isAuthEnabled() && identity != null && !identity.isAnonymous() + && !identity.hasRole("eddi-admin")) { + String callerId = identity.getPrincipal().getName(); + conversations = conversations.stream() + .filter(gc -> callerId.equals(gc.getUserId())) + .toList(); + } + return conversations; } catch (IResourceStore.ResourceStoreException e) { throw sneakyThrow(e); } diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java b/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java index 656f59832..894b5bbff 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java @@ -21,6 +21,7 @@ import java.util.Map; import static ai.labs.eddi.engine.mcp.McpToolUtils.errorJson; +import static ai.labs.eddi.engine.mcp.McpToolUtils.requireOwnerOrAdmin; import static ai.labs.eddi.engine.mcp.McpToolUtils.requireRole; /** @@ -57,6 +58,7 @@ public McpMemoryTools(IUserMemoryStore userMemoryStore, IJsonSerialization jsonS public String listUserMemories(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Maximum number of entries to return (default: 50)") Integer limit) { requireRole(identity, authEnabled, "eddi-viewer"); + requireOwnerOrAdmin(identity, authEnabled, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); try { @@ -86,6 +88,7 @@ public String getVisibleMemories(@ToolArg(description = "User ID (required)") St @ToolArg(description = "Recall order: 'most_recent' or 'most_accessed' (default: most_recent)") String order, @ToolArg(description = "Maximum number of entries (default: 50)") Integer limit) { requireRole(identity, authEnabled, "eddi-viewer"); + requireOwnerOrAdmin(identity, authEnabled, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); if (agentId == null || agentId.isBlank()) @@ -113,6 +116,7 @@ public String getVisibleMemories(@ToolArg(description = "User ID (required)") St public String searchUserMemories(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Search query (required)") String query) { requireRole(identity, authEnabled, "eddi-viewer"); + requireOwnerOrAdmin(identity, authEnabled, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); if (query == null || query.isBlank()) @@ -136,6 +140,7 @@ public String searchUserMemories(@ToolArg(description = "User ID (required)") St public String getMemoryByKey(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Memory key name (required)") String key) { requireRole(identity, authEnabled, "eddi-viewer"); + requireOwnerOrAdmin(identity, authEnabled, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); if (key == null || key.isBlank()) @@ -223,6 +228,7 @@ public String deleteAllUserMemories(@ToolArg(description = "User ID (required)") @Tool(name = "count_user_memories", description = "Count the number of memory entries for a user.") public String countUserMemories(@ToolArg(description = "User ID (required)") String userId) { requireRole(identity, authEnabled, "eddi-viewer"); + requireOwnerOrAdmin(identity, authEnabled, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); try { diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java b/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java index 38ed1098e..e40601aad 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java @@ -43,6 +43,36 @@ static void requireRole(SecurityIdentity identity, boolean authEnabled, String r } } + /** + * Check that the current caller owns the requested userId, or has admin role. + * When authorization is disabled, no check is performed. When the userId is + * null or blank, no check is performed (legacy data without ownership). + * + * @param identity + * the current security identity + * @param authEnabled + * whether authorization is enabled + * @param userId + * the requested userId to validate against the caller + * @throws ForbiddenException + * if the caller does not own the userId and is not admin + */ + static void requireOwnerOrAdmin(SecurityIdentity identity, boolean authEnabled, String userId) { + if (!authEnabled || userId == null || userId.isBlank()) { + return; + } + if (identity == null || identity.isAnonymous()) { + return; // Let requireRole handle authentication + } + if (identity.hasRole("eddi-admin")) { + return; + } + String callerId = identity.getPrincipal().getName(); + if (!callerId.equals(userId)) { + throw new ForbiddenException("MCP operation denied: userId '" + userId + "' does not match authenticated caller"); + } + } + /** * Get a REST interface proxy via IRestInterfaceFactory. These proxies make HTTP * calls that go through the full JAX-RS workflow, including diff --git a/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java b/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java new file mode 100644 index 000000000..fefe06e23 --- /dev/null +++ b/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java @@ -0,0 +1,158 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.security; + +import io.quarkus.security.ForbiddenException; +import io.quarkus.security.identity.SecurityIdentity; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.jboss.logging.Logger; + +/** + * Centralized ownership validation utility. Provides methods to assert that the + * authenticated caller owns the resource they are attempting to access, or + * holds the {@code eddi-admin} role. + * + *

+ * All checks are no-ops when {@code authorization.enabled=false}. + *

+ * + * @author ginccc + * @since 6.1.0 + */ +@ApplicationScoped +public class OwnershipValidator { + + private static final Logger LOGGER = Logger.getLogger(OwnershipValidator.class); + + private final boolean authEnabled; + + @Inject + public OwnershipValidator(@ConfigProperty(name = "authorization.enabled", defaultValue = "false") boolean authEnabled) { + this.authEnabled = authEnabled; + } + + /** + * Returns whether authorization enforcement is enabled. + * + * @return {@code true} if authorization checks are active + */ + public boolean isAuthEnabled() { + return authEnabled; + } + + /** + * Asserts that the caller matches the requested {@code userId} or holds the + * {@code eddi-admin} role. + * + * @param identity + * the caller's security identity (may be null/anonymous) + * @param requestedUserId + * the userId being accessed + * @throws ForbiddenException + * if the caller is not the owner and not an admin + */ + public void validateUserAccess(SecurityIdentity identity, String requestedUserId) { + if (!authEnabled) { + return; + } + if (identity == null || identity.isAnonymous()) { + return; // let @RolesAllowed handle anonymous access + } + if (identity.hasRole("eddi-admin")) { + return; + } + + String callerId = identity.getPrincipal().getName(); + if (!callerId.equals(requestedUserId)) { + LOGGER.warnf("Ownership check failed: caller '%s' attempted to access userId '%s'", callerId, requestedUserId); + throw new ForbiddenException("Access denied: you do not own this user's data"); + } + } + + /** + * Resolves the effective userId for starting a conversation. Admins may specify + * any userId; regular users must match their own identity. When the requested + * userId is {@code null}, the caller's identity is used as fallback. + * + *

+ * No-op when authorization is disabled — returns {@code requestedUserId} as-is. + *

+ * + * @param identity + * the caller's security identity (may be null/anonymous) + * @param requestedUserId + * the userId requested by the caller (may be null) + * @return the resolved userId to use + * @throws ForbiddenException + * if a non-admin caller tries to impersonate another user + */ + public String validateAndResolveUserId(SecurityIdentity identity, String requestedUserId) { + if (!authEnabled) { + return requestedUserId; + } + if (identity == null || identity.isAnonymous()) { + return requestedUserId; // let @RolesAllowed handle anonymous access + } + + String callerId = identity.getPrincipal().getName(); + + if (requestedUserId == null || requestedUserId.isBlank()) { + return callerId; + } + + if (identity.hasRole("eddi-admin")) { + return requestedUserId; + } + + if (!callerId.equals(requestedUserId)) { + LOGGER.warnf("UserId resolution rejected: caller '%s' attempted to set userId '%s'", callerId, requestedUserId); + throw new ForbiddenException("Access denied: you cannot start a conversation as another user"); + } + + return requestedUserId; + } + + /** + * Asserts that the caller owns the resource identified by + * {@code resourceOwnerId}, or holds the {@code eddi-admin} role. + * + *

+ * No-op when authorization is disabled, or when {@code resourceOwnerId} is + * null/blank (legacy data without ownership tracking). + *

+ * + * @param identity + * the caller's security identity (may be null/anonymous) + * @param resourceOwnerId + * the userId of the resource owner (may be null for legacy data) + * @param resourceType + * human-readable resource type for error messages + * @throws ForbiddenException + * if the caller is not the owner and not an admin + */ + public void requireOwnerOrAdmin(SecurityIdentity identity, String resourceOwnerId, String resourceType) { + if (!authEnabled) { + return; + } + if (resourceOwnerId == null || resourceOwnerId.isBlank()) { + return; // legacy data without ownership — allow access + } + if (identity == null || identity.isAnonymous()) { + return; // let @RolesAllowed handle anonymous access + } + if (identity.hasRole("eddi-admin")) { + return; + } + + String callerId = identity.getPrincipal().getName(); + if (!callerId.equals(resourceOwnerId)) { + LOGGER.warnf("Ownership check failed: caller '%s' denied access to %s owned by '%s'", + callerId, resourceType, resourceOwnerId); + throw new ForbiddenException("Access denied: you do not own this " + resourceType); + } + } +} From eb8ca32f394d7c87e435564a511d88c0ba01cb9b Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Wed, 10 Jun 2026 14:50:20 +0200 Subject: [PATCH 2/5] fix(security): update tests for new ownership constructor parameters Updated 4 test files to pass SecurityIdentity and OwnershipValidator mocks to the modified constructors of RestAgentEngine, RestUserMemoryStore, and RestGroupConversation. --- .../properties/rest/RestUserMemoryStoreTest.java | 8 +++++++- .../eddi/engine/internal/RestAgentEngineTest.java | 11 ++++++++++- .../internal/RestGroupConversationExtendedTest.java | 9 ++++++++- .../engine/internal/RestGroupConversationTest.java | 9 ++++++++- 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java b/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java index f7e32e699..c87bb7dd1 100644 --- a/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java +++ b/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java @@ -8,6 +8,8 @@ import ai.labs.eddi.configs.properties.model.Property.Visibility; import ai.labs.eddi.configs.properties.model.UserMemoryEntry; import ai.labs.eddi.datastore.IResourceStore; +import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.InternalServerErrorException; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.Response; @@ -28,12 +30,16 @@ class RestUserMemoryStoreTest { private IUserMemoryStore store; + private SecurityIdentity identity; + private OwnershipValidator ownershipValidator; private RestUserMemoryStore rest; @BeforeEach void setUp() { store = mock(IUserMemoryStore.class); - rest = new RestUserMemoryStore(store); + identity = mock(SecurityIdentity.class); + ownershipValidator = mock(OwnershipValidator.class); + rest = new RestUserMemoryStore(store, identity, ownershipValidator); } // === getAllMemories === diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java index 6d66db5b7..4a068fc9c 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java @@ -9,10 +9,13 @@ import ai.labs.eddi.engine.api.IConversationService; import ai.labs.eddi.engine.api.IConversationService.*; import ai.labs.eddi.engine.gdpr.ProcessingRestrictedException; +import ai.labs.eddi.engine.memory.descriptor.IConversationDescriptorStore; import ai.labs.eddi.engine.memory.model.ConversationState; import ai.labs.eddi.engine.memory.model.SimpleConversationMemorySnapshot; import ai.labs.eddi.engine.model.Deployment; import ai.labs.eddi.engine.model.InputData; +import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.InternalServerErrorException; import jakarta.ws.rs.container.AsyncResponse; import jakarta.ws.rs.core.Response; @@ -35,12 +38,18 @@ class RestAgentEngineTest { private IConversationService conversationService; + private IConversationDescriptorStore descriptorStore; + private SecurityIdentity identity; + private OwnershipValidator ownershipValidator; private RestAgentEngine restAgentEngine; @BeforeEach void setUp() { conversationService = mock(IConversationService.class); - restAgentEngine = new RestAgentEngine(conversationService, 30); + descriptorStore = mock(IConversationDescriptorStore.class); + identity = mock(SecurityIdentity.class); + ownershipValidator = mock(OwnershipValidator.class); + restAgentEngine = new RestAgentEngine(conversationService, descriptorStore, identity, ownershipValidator, 30); } @Nested diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java index 4f84809e8..ca4d5fd0b 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationExtendedTest.java @@ -11,6 +11,8 @@ import ai.labs.eddi.engine.api.IGroupConversationService.GroupDiscussionEventListener; import ai.labs.eddi.engine.api.IRestGroupConversation.DiscussRequest; import ai.labs.eddi.engine.lifecycle.GroupConversationEventSink; +import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.sse.OutboundSseEvent; import jakarta.ws.rs.sse.Sse; import jakarta.ws.rs.sse.SseEventSink; @@ -33,6 +35,8 @@ class RestGroupConversationExtendedTest { private IGroupConversationService groupService; private IJsonSerialization jsonSerialization; + private SecurityIdentity identity; + private OwnershipValidator ownershipValidator; private RestGroupConversation restGroupConversation; private SseEventSink eventSink; private Sse sse; @@ -41,7 +45,10 @@ class RestGroupConversationExtendedTest { void setUp() { groupService = mock(IGroupConversationService.class); jsonSerialization = mock(IJsonSerialization.class); - restGroupConversation = new RestGroupConversation(groupService, jsonSerialization); + identity = mock(SecurityIdentity.class); + ownershipValidator = mock(OwnershipValidator.class); + when(ownershipValidator.validateAndResolveUserId(any(), any())).thenAnswer(inv -> inv.getArgument(1)); + restGroupConversation = new RestGroupConversation(groupService, jsonSerialization, identity, ownershipValidator); eventSink = mock(SseEventSink.class); sse = mock(Sse.class); diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java index bce80df31..f182528b8 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java @@ -9,6 +9,8 @@ import ai.labs.eddi.datastore.serialization.IJsonSerialization; import ai.labs.eddi.engine.api.IGroupConversationService; import ai.labs.eddi.engine.api.IRestGroupConversation.DiscussRequest; +import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.core.Response; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; @@ -27,13 +29,18 @@ class RestGroupConversationTest { private IGroupConversationService groupService; private IJsonSerialization jsonSerialization; + private SecurityIdentity identity; + private OwnershipValidator ownershipValidator; private RestGroupConversation restGroupConversation; @BeforeEach void setUp() { groupService = mock(IGroupConversationService.class); jsonSerialization = mock(IJsonSerialization.class); - restGroupConversation = new RestGroupConversation(groupService, jsonSerialization); + identity = mock(SecurityIdentity.class); + ownershipValidator = mock(OwnershipValidator.class); + when(ownershipValidator.validateAndResolveUserId(any(), any())).thenAnswer(inv -> inv.getArgument(1)); + restGroupConversation = new RestGroupConversation(groupService, jsonSerialization, identity, ownershipValidator); } @Nested From 1623caa3753e236569e52c4ed8e5e23d288f2939 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Wed, 10 Jun 2026 15:12:39 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(security):=20code=20review=20hardening?= =?UTF-8?q?=20=E2=80=94=20PII=20logs,=20narrow=20catch,=20MCP=20consolidat?= =?UTF-8?q?ion,=20deleteMemory=20ownership?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - M1: Remove duplicate requireOwnerOrAdmin from McpToolUtils; McpMemoryTools injects OwnershipValidator directly - M3: Sanitize PII in WARN logs; full details at DEBUG level only - M4: Narrow catch in validateConversationOwnership to ResourceNotFoundException + ResourceStoreException - BUG-2: Add findEntryById to IUserMemoryStore (Mongo + Postgres); deleteMemory validates ownership before deletion --- docs/changelog.md | 26 ++++++++++++++++ .../configs/properties/IUserMemoryStore.java | 9 ++++++ .../mongo/MongoUserMemoryStore.java | 7 +++++ .../properties/rest/RestUserMemoryStore.java | 7 +++++ .../postgres/PostgresUserMemoryStore.java | 17 +++++++++++ .../eddi/engine/internal/RestAgentEngine.java | 11 ++++--- .../internal/RestGroupConversation.java | 12 ++++---- .../labs/eddi/engine/mcp/McpMemoryTools.java | 16 ++++++---- .../ai/labs/eddi/engine/mcp/McpToolUtils.java | 30 ------------------- .../engine/security/OwnershipValidator.java | 10 ++++--- 10 files changed, 96 insertions(+), 49 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 2459b33cf..2a7e56135 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -46,6 +46,32 @@ **Files:** `OwnershipValidator.java` [NEW], `RestAgentEngine.java`, `RestUserMemoryStore.java`, `RestGroupConversation.java`, `IRestGdprAdmin.java`, `RestGdprAdmin.java`, `RestA2AEndpoint.java`, `McpToolUtils.java`, `McpMemoryTools.java` +### Code Review Hardening (2026-06-10) + +**Repo:** EDDI (`fix/security-audit-idor-remediation`) +**What changed:** Addressed all findings from the post-implementation code review. + +- **M1 — MCP ownership consolidation:** Removed duplicate `requireOwnerOrAdmin` static method from `McpToolUtils`. `McpMemoryTools` now injects `OwnershipValidator` directly and calls `validateUserAccess()` — single source of truth for ownership logic. +- **M3 — PII in WARN logs:** `OwnershipValidator` WARN messages no longer include caller/user IDs. Full details are logged at DEBUG level only, reducing compliance risk. +- **M4 — Narrow catch clause:** `RestAgentEngine.validateConversationOwnership()` now catches `ResourceNotFoundException` and `ResourceStoreException` specifically instead of generic `Exception`, preventing unexpected errors from being silently swallowed. +- **BUG-2 — deleteMemory ownership:** Added `findEntryById(String entryId)` to `IUserMemoryStore` with MongoDB and PostgreSQL implementations. `RestUserMemoryStore.deleteMemory()` now looks up the entry, validates ownership via `validateUserAccess()`, and returns 404 if not found. + +**Files:** `OwnershipValidator.java`, `RestAgentEngine.java`, `RestUserMemoryStore.java`, `McpToolUtils.java`, `McpMemoryTools.java`, `IUserMemoryStore.java`, `MongoUserMemoryStore.java`, `PostgresUserMemoryStore.java` + +### Test Coverage for Security Fixes (2026-06-10) + +**Repo:** EDDI (`fix/security-audit-idor-remediation`) +**What changed:** Added 36 new tests covering all security-critical ownership validation logic. + +- **OwnershipValidatorTest [NEW]:** 24 tests across 4 nested groups — `validateUserAccess`, `validateAndResolveUserId`, `requireOwnerOrAdmin`, `isAuthEnabled`. Covers auth on/off, admin bypass, legacy null owner, caller mismatch → ForbiddenException. +- **RestAgentEngineTest — OwnershipValidation:** 5 tests — admin userId override, impersonation rejection, non-owner read/end, descriptor-not-found graceful skip. +- **RestUserMemoryStoreTest — DeleteMemory:** 3 tests — owner match → 204, not found → 404, non-owner → ForbiddenException. +- **RestGroupConversationTest — OwnershipValidation:** 4 tests — non-owner read/delete, userId resolution in discuss, list filtering for non-admin. +- **Existing test fixes:** Updated `RestAgentEngineTest`, `RestGroupConversationTest`, `McpMemoryToolsTest` stubs for new constructor parameters and ownership lookup patterns. + +**Total:** 184 security-related tests, 0 failures, 0 errors. +**Files:** `OwnershipValidatorTest.java` [NEW], `RestAgentEngineTest.java`, `RestUserMemoryStoreTest.java`, `RestGroupConversationTest.java`, `McpMemoryToolsTest.java` + --- ## 🐛 Fix: Swagger UI Broken by CSP — Per-Path Filter Override (2026-06-03) diff --git a/src/main/java/ai/labs/eddi/configs/properties/IUserMemoryStore.java b/src/main/java/ai/labs/eddi/configs/properties/IUserMemoryStore.java index 73df34285..b9904bdb7 100644 --- a/src/main/java/ai/labs/eddi/configs/properties/IUserMemoryStore.java +++ b/src/main/java/ai/labs/eddi/configs/properties/IUserMemoryStore.java @@ -53,6 +53,15 @@ public interface IUserMemoryStore { void deleteEntry(String entryId) throws IResourceStore.ResourceStoreException; + /** + * Finds a memory entry by its ID. Used for ownership validation before + * deletion. + * + * @return the entry, or empty if not found + * @since 6.1.0 + */ + Optional findEntryById(String entryId) throws IResourceStore.ResourceStoreException; + // === Queries === /** diff --git a/src/main/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStore.java b/src/main/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStore.java index e2b5d02fa..b79117eba 100644 --- a/src/main/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStore.java +++ b/src/main/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStore.java @@ -186,6 +186,13 @@ public void deleteEntry(String entryId) throws IResourceStore.ResourceStoreExcep memoriesCollection.deleteOne(eq("_id", new ObjectId(entryId))); } + @Override + public Optional findEntryById(String entryId) throws IResourceStore.ResourceStoreException { + RuntimeUtilities.checkNotNull(entryId, "entryId"); + Document doc = memoriesCollection.find(eq("_id", new ObjectId(entryId))).first(); + return doc != null ? Optional.of(documentToEntry(doc)) : Optional.empty(); + } + // === Queries === @Override diff --git a/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java b/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java index 880b58227..fdd204d55 100644 --- a/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java +++ b/src/main/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStore.java @@ -131,8 +131,15 @@ public Response upsertMemory(UserMemoryEntry entry) { @Override public Response deleteMemory(String entryId) { try { + var entry = userMemoryStore.findEntryById(entryId); + if (entry.isEmpty()) { + throw new NotFoundException("Memory entry not found: " + entryId); + } + ownershipValidator.validateUserAccess(identity, entry.get().userId()); userMemoryStore.deleteEntry(entryId); return Response.noContent().build(); + } catch (NotFoundException e) { + throw e; } catch (IResourceStore.ResourceStoreException e) { LOGGER.error("Failed to delete memory entry: " + entryId, e); throw new InternalServerErrorException(e.getLocalizedMessage()); diff --git a/src/main/java/ai/labs/eddi/datastore/postgres/PostgresUserMemoryStore.java b/src/main/java/ai/labs/eddi/datastore/postgres/PostgresUserMemoryStore.java index d1a130c9f..32a2e99f4 100644 --- a/src/main/java/ai/labs/eddi/datastore/postgres/PostgresUserMemoryStore.java +++ b/src/main/java/ai/labs/eddi/datastore/postgres/PostgresUserMemoryStore.java @@ -245,6 +245,23 @@ public void deleteEntry(String entryId) throws IResourceStore.ResourceStoreExcep } } + @Override + public Optional findEntryById(String entryId) throws IResourceStore.ResourceStoreException { + ensureSchema(); + String sql = "SELECT * FROM usermemories WHERE id = ?"; + try (Connection conn = dataSourceInstance.get().getConnection(); PreparedStatement ps = conn.prepareStatement(sql)) { + ps.setString(1, entryId); + try (ResultSet rs = ps.executeQuery()) { + if (rs.next()) { + return Optional.of(resultSetToEntry(rs)); + } + } + } catch (SQLException e) { + throw new IResourceStore.ResourceStoreException("Failed to find memory entry by id", e); + } + return Optional.empty(); + } + // === Queries === @Override diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java index 29a8030ab..60cde8792 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java @@ -256,10 +256,13 @@ private void validateConversationOwnership(String conversationId) { var descriptor = conversationDescriptorStore.readDescriptor(conversationId, 0); ownershipValidator.requireOwnerOrAdmin(identity, descriptor.getUserId(), "conversation"); } catch (ForbiddenException e) { - throw e; // re-throw ownership errors - } catch (Exception e) { - // Descriptor not found or store error — let the actual operation handle it - LOGGER.debugf("Could not validate conversation ownership for %s: %s", conversationId, e.getMessage()); + throw e; + } catch (ResourceNotFoundException e) { + // Descriptor not found — let the actual operation handle it + LOGGER.debugf("Conversation descriptor not found for %s", conversationId); + } catch (ResourceStoreException e) { + // Store error — let the actual operation handle it + LOGGER.debugf("Could not load conversation descriptor for %s: %s", conversationId, e.getMessage()); } } } diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java b/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java index ab9ccd391..4016432ad 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java @@ -43,9 +43,9 @@ public class RestGroupConversation implements IRestGroupConversation { @Inject public RestGroupConversation(IGroupConversationService groupConversationService, - IJsonSerialization jsonSerialization, - SecurityIdentity identity, - OwnershipValidator ownershipValidator) { + IJsonSerialization jsonSerialization, + SecurityIdentity identity, + OwnershipValidator ownershipValidator) { this.groupConversationService = groupConversationService; this.jsonSerialization = jsonSerialization; this.identity = identity; @@ -56,7 +56,8 @@ public RestGroupConversation(IGroupConversationService groupConversationService, public Response discuss(String groupId, DiscussRequest request) { try { String userId = ownershipValidator.validateAndResolveUserId(identity, request.userId()); - if (userId == null || userId.isBlank()) userId = "anonymous"; + if (userId == null || userId.isBlank()) + userId = "anonymous"; GroupConversation gc = groupConversationService.discuss(groupId, request.question(), userId, 0); URI location = URI.create("/groups/" + groupId + "/conversations/" + gc.getId()); return Response.created(location).entity(gc).build(); @@ -74,7 +75,8 @@ public Response discuss(String groupId, DiscussRequest request) { public void discussStreaming(String groupId, DiscussRequest request, SseEventSink eventSink, Sse sse) { try { String userId = ownershipValidator.validateAndResolveUserId(identity, request.userId()); - if (userId == null || userId.isBlank()) userId = "anonymous"; + if (userId == null || userId.isBlank()) + userId = "anonymous"; GroupDiscussionEventListener listener = new GroupDiscussionEventListener() { @Override diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java b/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java index 894b5bbff..8d7a7be89 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java @@ -20,8 +20,9 @@ import java.util.List; import java.util.Map; +import ai.labs.eddi.engine.security.OwnershipValidator; + import static ai.labs.eddi.engine.mcp.McpToolUtils.errorJson; -import static ai.labs.eddi.engine.mcp.McpToolUtils.requireOwnerOrAdmin; import static ai.labs.eddi.engine.mcp.McpToolUtils.requireRole; /** @@ -42,14 +43,17 @@ public class McpMemoryTools { private final IUserMemoryStore userMemoryStore; private final IJsonSerialization jsonSerialization; private final SecurityIdentity identity; + private final OwnershipValidator ownershipValidator; private final boolean authEnabled; @Inject public McpMemoryTools(IUserMemoryStore userMemoryStore, IJsonSerialization jsonSerialization, SecurityIdentity identity, + OwnershipValidator ownershipValidator, @ConfigProperty(name = "authorization.enabled", defaultValue = "false") boolean authEnabled) { this.userMemoryStore = userMemoryStore; this.jsonSerialization = jsonSerialization; this.identity = identity; + this.ownershipValidator = ownershipValidator; this.authEnabled = authEnabled; } @@ -58,7 +62,7 @@ public McpMemoryTools(IUserMemoryStore userMemoryStore, IJsonSerialization jsonS public String listUserMemories(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Maximum number of entries to return (default: 50)") Integer limit) { requireRole(identity, authEnabled, "eddi-viewer"); - requireOwnerOrAdmin(identity, authEnabled, userId); + ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); try { @@ -88,7 +92,7 @@ public String getVisibleMemories(@ToolArg(description = "User ID (required)") St @ToolArg(description = "Recall order: 'most_recent' or 'most_accessed' (default: most_recent)") String order, @ToolArg(description = "Maximum number of entries (default: 50)") Integer limit) { requireRole(identity, authEnabled, "eddi-viewer"); - requireOwnerOrAdmin(identity, authEnabled, userId); + ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); if (agentId == null || agentId.isBlank()) @@ -116,7 +120,7 @@ public String getVisibleMemories(@ToolArg(description = "User ID (required)") St public String searchUserMemories(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Search query (required)") String query) { requireRole(identity, authEnabled, "eddi-viewer"); - requireOwnerOrAdmin(identity, authEnabled, userId); + ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); if (query == null || query.isBlank()) @@ -140,7 +144,7 @@ public String searchUserMemories(@ToolArg(description = "User ID (required)") St public String getMemoryByKey(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Memory key name (required)") String key) { requireRole(identity, authEnabled, "eddi-viewer"); - requireOwnerOrAdmin(identity, authEnabled, userId); + ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); if (key == null || key.isBlank()) @@ -228,7 +232,7 @@ public String deleteAllUserMemories(@ToolArg(description = "User ID (required)") @Tool(name = "count_user_memories", description = "Count the number of memory entries for a user.") public String countUserMemories(@ToolArg(description = "User ID (required)") String userId) { requireRole(identity, authEnabled, "eddi-viewer"); - requireOwnerOrAdmin(identity, authEnabled, userId); + ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); try { diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java b/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java index e40601aad..38ed1098e 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java @@ -43,36 +43,6 @@ static void requireRole(SecurityIdentity identity, boolean authEnabled, String r } } - /** - * Check that the current caller owns the requested userId, or has admin role. - * When authorization is disabled, no check is performed. When the userId is - * null or blank, no check is performed (legacy data without ownership). - * - * @param identity - * the current security identity - * @param authEnabled - * whether authorization is enabled - * @param userId - * the requested userId to validate against the caller - * @throws ForbiddenException - * if the caller does not own the userId and is not admin - */ - static void requireOwnerOrAdmin(SecurityIdentity identity, boolean authEnabled, String userId) { - if (!authEnabled || userId == null || userId.isBlank()) { - return; - } - if (identity == null || identity.isAnonymous()) { - return; // Let requireRole handle authentication - } - if (identity.hasRole("eddi-admin")) { - return; - } - String callerId = identity.getPrincipal().getName(); - if (!callerId.equals(userId)) { - throw new ForbiddenException("MCP operation denied: userId '" + userId + "' does not match authenticated caller"); - } - } - /** * Get a REST interface proxy via IRestInterfaceFactory. These proxies make HTTP * calls that go through the full JAX-RS workflow, including diff --git a/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java b/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java index fefe06e23..4c391de98 100644 --- a/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java +++ b/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java @@ -68,7 +68,8 @@ public void validateUserAccess(SecurityIdentity identity, String requestedUserId String callerId = identity.getPrincipal().getName(); if (!callerId.equals(requestedUserId)) { - LOGGER.warnf("Ownership check failed: caller '%s' attempted to access userId '%s'", callerId, requestedUserId); + LOGGER.warnf("Ownership check failed: caller attempted to access another user's data"); + LOGGER.debugf("Ownership detail: caller='%s', requestedUserId='%s'", callerId, requestedUserId); throw new ForbiddenException("Access denied: you do not own this user's data"); } } @@ -109,7 +110,8 @@ public String validateAndResolveUserId(SecurityIdentity identity, String request } if (!callerId.equals(requestedUserId)) { - LOGGER.warnf("UserId resolution rejected: caller '%s' attempted to set userId '%s'", callerId, requestedUserId); + LOGGER.warnf("UserId resolution rejected: caller attempted to impersonate another user"); + LOGGER.debugf("UserId resolution detail: caller='%s', requestedUserId='%s'", callerId, requestedUserId); throw new ForbiddenException("Access denied: you cannot start a conversation as another user"); } @@ -150,8 +152,8 @@ public void requireOwnerOrAdmin(SecurityIdentity identity, String resourceOwnerI String callerId = identity.getPrincipal().getName(); if (!callerId.equals(resourceOwnerId)) { - LOGGER.warnf("Ownership check failed: caller '%s' denied access to %s owned by '%s'", - callerId, resourceType, resourceOwnerId); + LOGGER.warnf("Ownership check failed: caller denied access to %s owned by another user", resourceType); + LOGGER.debugf("Ownership detail: caller='%s', resourceType='%s', ownerId='%s'", callerId, resourceType, resourceOwnerId); throw new ForbiddenException("Access denied: you do not own this " + resourceType); } } From 320f6ceee44873865f62439a7509613acc14a418 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Wed, 10 Jun 2026 15:12:52 +0200 Subject: [PATCH 4/5] test(security): comprehensive ownership validation test coverage (36 new tests) - OwnershipValidatorTest [NEW]: 24 tests for all 4 public methods - RestAgentEngineTest: 5 ownership validation tests + default descriptor stub - RestUserMemoryStoreTest: 3 deleteMemory ownership tests - RestGroupConversationTest: 4 ownership validation tests (IDOR, list filtering) - McpMemoryToolsTest: updated constructor for OwnershipValidator injection --- .../rest/RestUserMemoryStoreTest.java | 47 ++- .../engine/internal/RestAgentEngineTest.java | 83 ++++- .../internal/RestGroupConversationTest.java | 84 +++++ .../eddi/engine/mcp/McpMemoryToolsTest.java | 4 +- .../security/OwnershipValidatorTest.java | 303 ++++++++++++++++++ 5 files changed, 512 insertions(+), 9 deletions(-) create mode 100644 src/test/java/ai/labs/eddi/engine/security/OwnershipValidatorTest.java diff --git a/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java b/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java index c87bb7dd1..07c97e1fe 100644 --- a/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java +++ b/src/test/java/ai/labs/eddi/configs/properties/rest/RestUserMemoryStoreTest.java @@ -9,11 +9,14 @@ import ai.labs.eddi.configs.properties.model.UserMemoryEntry; import ai.labs.eddi.datastore.IResourceStore; import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.ForbiddenException; import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.InternalServerErrorException; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.Response; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import java.time.Instant; @@ -161,13 +164,43 @@ void upsertMemory_shouldRejectKeyTooLong() { // === deleteMemory === - @Test - void deleteMemory_shouldReturn204() throws Exception { - doNothing().when(store).deleteEntry("entry-1"); - - Response response = rest.deleteMemory("entry-1"); - - assertEquals(204, response.getStatus()); + @Nested + @DisplayName("deleteMemory") + class DeleteMemory { + + @Test + @DisplayName("should return 204 when owner matches") + void deleteMemory_shouldReturn204WhenOwnerMatches() throws Exception { + var memEntry = new UserMemoryEntry("entry-1", "user-1", "key", "value", "fact", + Visibility.global, null, List.of(), null, false, 0, Instant.now(), Instant.now()); + when(store.findEntryById("entry-1")).thenReturn(Optional.of(memEntry)); + + Response response = rest.deleteMemory("entry-1"); + + assertEquals(204, response.getStatus()); + verify(store).deleteEntry("entry-1"); + } + + @Test + @DisplayName("should throw NotFoundException when entry not found") + void deleteMemory_shouldThrow404WhenNotFound() throws Exception { + when(store.findEntryById("entry-1")).thenReturn(Optional.empty()); + + assertThrows(NotFoundException.class, () -> rest.deleteMemory("entry-1")); + } + + @Test + @DisplayName("should throw ForbiddenException when caller does not own entry") + void deleteMemory_shouldRejectNonOwner() throws Exception { + var memEntry = new UserMemoryEntry("entry-1", "other-user", "key", "value", "fact", + Visibility.global, null, List.of(), null, false, 0, Instant.now(), Instant.now()); + when(store.findEntryById("entry-1")).thenReturn(Optional.of(memEntry)); + doThrow(new ForbiddenException("Access denied")) + .when(ownershipValidator).validateUserAccess(identity, "other-user"); + + assertThrows(ForbiddenException.class, () -> rest.deleteMemory("entry-1")); + verify(store, never()).deleteEntry(anyString()); + } } // === deleteAllForUser === diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java index 4a068fc9c..9c8724664 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java @@ -10,11 +10,13 @@ import ai.labs.eddi.engine.api.IConversationService.*; import ai.labs.eddi.engine.gdpr.ProcessingRestrictedException; import ai.labs.eddi.engine.memory.descriptor.IConversationDescriptorStore; +import ai.labs.eddi.engine.memory.descriptor.model.ConversationDescriptor; import ai.labs.eddi.engine.memory.model.ConversationState; import ai.labs.eddi.engine.memory.model.SimpleConversationMemorySnapshot; import ai.labs.eddi.engine.model.Deployment; import ai.labs.eddi.engine.model.InputData; import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.ForbiddenException; import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.InternalServerErrorException; import jakarta.ws.rs.container.AsyncResponse; @@ -44,11 +46,15 @@ class RestAgentEngineTest { private RestAgentEngine restAgentEngine; @BeforeEach - void setUp() { + void setUp() throws Exception { conversationService = mock(IConversationService.class); descriptorStore = mock(IConversationDescriptorStore.class); identity = mock(SecurityIdentity.class); ownershipValidator = mock(OwnershipValidator.class); + when(ownershipValidator.validateAndResolveUserId(any(), any())).thenAnswer(inv -> inv.getArgument(1)); + // Default: descriptor not found → ownership check skipped gracefully + when(descriptorStore.readDescriptor(anyString(), anyInt())) + .thenThrow(new ResourceNotFoundException("test default")); restAgentEngine = new RestAgentEngine(conversationService, descriptorStore, identity, ownershipValidator, 30); } @@ -527,4 +533,79 @@ void redoStoreError() throws Exception { () -> restAgentEngine.redo("conv-1")); } } + + @Nested + @DisplayName("OwnershipValidation") + class OwnershipValidation { + + @Test + @DisplayName("should pass resolved userId to startConversation") + void startConversation_resolvesUserId() throws Exception { + when(ownershipValidator.validateAndResolveUserId(identity, "user-1")) + .thenReturn("admin-resolved"); + var result = new IConversationService.ConversationResult("conv-1", URI.create("/conversations/conv-1")); + when(conversationService.startConversation(any(), anyString(), eq("admin-resolved"), any())) + .thenReturn(result); + + Response response = restAgentEngine.startConversation("agent-1", + Deployment.Environment.production, "user-1"); + + assertEquals(201, response.getStatus()); + verify(conversationService).startConversation( + Deployment.Environment.production, "agent-1", "admin-resolved", Map.of()); + } + + @Test + @DisplayName("should throw ForbiddenException when caller tries to impersonate") + void startConversation_rejectsImpersonation() { + when(ownershipValidator.validateAndResolveUserId(identity, "other-user")) + .thenThrow(new ForbiddenException("Access denied")); + + assertThrows(ForbiddenException.class, + () -> restAgentEngine.startConversation("agent-1", + Deployment.Environment.production, "other-user")); + } + + @Test + @DisplayName("should throw ForbiddenException when caller does not own conversation (read)") + void readConversation_rejectsNonOwner() throws Exception { + var descriptor = mock(ConversationDescriptor.class); + when(descriptor.getUserId()).thenReturn("other-user"); + doReturn(descriptor).when(descriptorStore).readDescriptor("conv-1", 0); + doThrow(new ForbiddenException("Access denied")) + .when(ownershipValidator).requireOwnerOrAdmin(identity, "other-user", "conversation"); + + assertThrows(ForbiddenException.class, + () -> restAgentEngine.readConversation("conv-1", false, false, List.of())); + } + + @Test + @DisplayName("should throw ForbiddenException when caller does not own conversation (end)") + void endConversation_rejectsNonOwner() throws Exception { + var descriptor = mock(ConversationDescriptor.class); + when(descriptor.getUserId()).thenReturn("other-user"); + doReturn(descriptor).when(descriptorStore).readDescriptor("conv-1", 0); + doThrow(new ForbiddenException("Access denied")) + .when(ownershipValidator).requireOwnerOrAdmin(identity, "other-user", "conversation"); + + assertThrows(ForbiddenException.class, + () -> restAgentEngine.endConversation("conv-1")); + } + + @Test + @DisplayName("should skip ownership check when descriptor not found") + void descriptorNotFound_skipsCheck() throws Exception { + // Default stub already throws ResourceNotFoundException — just verify behavior + var snapshot = new SimpleConversationMemorySnapshot(); + snapshot.setConversationState(ConversationState.READY); + when(conversationService.readConversation("conv-1", false, false, List.of())) + .thenReturn(snapshot); + + SimpleConversationMemorySnapshot result = restAgentEngine + .readConversation("conv-1", false, false, List.of()); + + assertEquals(ConversationState.READY, result.getConversationState()); + verify(ownershipValidator, never()).requireOwnerOrAdmin(any(), any(), any()); + } + } } diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java index f182528b8..f2ceea3d3 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java @@ -10,6 +10,7 @@ import ai.labs.eddi.engine.api.IGroupConversationService; import ai.labs.eddi.engine.api.IRestGroupConversation.DiscussRequest; import ai.labs.eddi.engine.security.OwnershipValidator; +import io.quarkus.security.ForbiddenException; import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.core.Response; import org.junit.jupiter.api.BeforeEach; @@ -17,6 +18,7 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import java.util.ArrayList; import java.util.List; import static org.junit.jupiter.api.Assertions.*; @@ -156,6 +158,10 @@ class DeleteGroupConversation { @Test @DisplayName("should return 200 on successful delete") void success() throws Exception { + var gc = new GroupConversation(); + gc.setId("gc-1"); + when(groupService.readGroupConversation("gc-1")).thenReturn(gc); + Response response = restGroupConversation.deleteGroupConversation("group-1", "gc-1"); assertEquals(200, response.getStatus()); @@ -165,6 +171,9 @@ void success() throws Exception { @Test @DisplayName("should propagate ResourceStoreException") void storeError() throws Exception { + var gc = new GroupConversation(); + gc.setId("gc-1"); + when(groupService.readGroupConversation("gc-1")).thenReturn(gc); doThrow(new IResourceStore.ResourceStoreException("Delete failed")) .when(groupService).deleteGroupConversation("gc-1"); @@ -213,4 +222,79 @@ void storeError() throws Exception { () -> restGroupConversation.listGroupConversations("group-1", 0, 10)); } } + + @Nested + @DisplayName("OwnershipValidation") + class OwnershipValidation { + + @Test + @DisplayName("should throw ForbiddenException when caller does not own group conversation (read)") + void readGroupConversation_rejectsNonOwner() throws Exception { + var gc = new GroupConversation(); + gc.setId("gc-1"); + gc.setUserId("other-user"); + when(groupService.readGroupConversation("gc-1")).thenReturn(gc); + doThrow(new ForbiddenException("Access denied")) + .when(ownershipValidator).requireOwnerOrAdmin(identity, "other-user", "group conversation"); + + assertThrows(ForbiddenException.class, + () -> restGroupConversation.readGroupConversation("group-1", "gc-1")); + } + + @Test + @DisplayName("should throw ForbiddenException when caller does not own group conversation (delete)") + void deleteGroupConversation_rejectsNonOwner() throws Exception { + var gc = new GroupConversation(); + gc.setId("gc-1"); + gc.setUserId("other-user"); + when(groupService.readGroupConversation("gc-1")).thenReturn(gc); + doThrow(new ForbiddenException("Access denied")) + .when(ownershipValidator).requireOwnerOrAdmin(identity, "other-user", "group conversation"); + + assertThrows(ForbiddenException.class, + () -> restGroupConversation.deleteGroupConversation("group-1", "gc-1")); + verify(groupService, never()).deleteGroupConversation(anyString()); + } + + @Test + @DisplayName("should resolve userId via validator during discuss") + void discuss_resolvesUserId() throws Exception { + when(ownershipValidator.validateAndResolveUserId(identity, "user-1")) + .thenReturn("admin-resolved"); + var gc = new GroupConversation(); + gc.setId("gc-1"); + when(groupService.discuss("group-1", "Hello", "admin-resolved", 0)).thenReturn(gc); + + Response response = restGroupConversation.discuss("group-1", + new DiscussRequest("Hello", "user-1")); + + assertEquals(201, response.getStatus()); + verify(groupService).discuss("group-1", "Hello", "admin-resolved", 0); + } + + @Test + @DisplayName("should filter list to owned conversations for non-admin users") + void listGroupConversations_filtersForNonAdmin() throws Exception { + when(ownershipValidator.isAuthEnabled()).thenReturn(true); + when(identity.isAnonymous()).thenReturn(false); + when(identity.hasRole("eddi-admin")).thenReturn(false); + var principal = mock(java.security.Principal.class); + when(principal.getName()).thenReturn("user-1"); + when(identity.getPrincipal()).thenReturn(principal); + + var gc1 = new GroupConversation(); + gc1.setId("gc-1"); + gc1.setUserId("user-1"); + var gc2 = new GroupConversation(); + gc2.setId("gc-2"); + gc2.setUserId("other-user"); + when(groupService.listGroupConversations("group-1", 0, 10)) + .thenReturn(new ArrayList<>(List.of(gc1, gc2))); + + List result = restGroupConversation.listGroupConversations("group-1", 0, 10); + + assertEquals(1, result.size()); + assertEquals("gc-1", result.get(0).getId()); + } + } } diff --git a/src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java b/src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java index 8eb12e536..83a557415 100644 --- a/src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java +++ b/src/test/java/ai/labs/eddi/engine/mcp/McpMemoryToolsTest.java @@ -7,6 +7,7 @@ import ai.labs.eddi.configs.properties.IUserMemoryStore; import ai.labs.eddi.configs.properties.model.UserMemoryEntry; import ai.labs.eddi.datastore.serialization.IJsonSerialization; +import ai.labs.eddi.engine.security.OwnershipValidator; import io.quarkus.security.identity.SecurityIdentity; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -28,8 +29,9 @@ void setUp() { userMemoryStore = mock(IUserMemoryStore.class); jsonSerialization = mock(IJsonSerialization.class); var identity = mock(SecurityIdentity.class); + var ownershipValidator = mock(OwnershipValidator.class); // authEnabled=false so no role checks - tools = new McpMemoryTools(userMemoryStore, jsonSerialization, identity, false); + tools = new McpMemoryTools(userMemoryStore, jsonSerialization, identity, ownershipValidator, false); } // ==================== listUserMemories ==================== diff --git a/src/test/java/ai/labs/eddi/engine/security/OwnershipValidatorTest.java b/src/test/java/ai/labs/eddi/engine/security/OwnershipValidatorTest.java new file mode 100644 index 000000000..be520dffb --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/security/OwnershipValidatorTest.java @@ -0,0 +1,303 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.security; + +import io.quarkus.security.ForbiddenException; +import io.quarkus.security.identity.SecurityIdentity; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.security.Principal; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link OwnershipValidator}. + * + * @author ginccc + */ +class OwnershipValidatorTest { + + private static final String CALLER_ID = "user-123"; + private static final String OTHER_USER = "user-456"; + + // -- helpers -- + + private SecurityIdentity authenticatedIdentity(String principalName, boolean isAdmin) { + var identity = mock(SecurityIdentity.class); + var principal = mock(Principal.class); + when(principal.getName()).thenReturn(principalName); + when(identity.getPrincipal()).thenReturn(principal); + when(identity.isAnonymous()).thenReturn(false); + when(identity.hasRole("eddi-admin")).thenReturn(isAdmin); + return identity; + } + + private SecurityIdentity anonymousIdentity() { + var identity = mock(SecurityIdentity.class); + when(identity.isAnonymous()).thenReturn(true); + return identity; + } + + // ==================== isAuthEnabled ==================== + + @Nested + @DisplayName("isAuthEnabled()") + class IsAuthEnabled { + + @Test + @DisplayName("returns true when constructed with authEnabled=true") + void returnsTrue_whenAuthEnabled() { + var validator = new OwnershipValidator(true); + assertTrue(validator.isAuthEnabled()); + } + + @Test + @DisplayName("returns false when constructed with authEnabled=false") + void returnsFalse_whenAuthDisabled() { + var validator = new OwnershipValidator(false); + assertFalse(validator.isAuthEnabled()); + } + } + + // ==================== validateUserAccess ==================== + + @Nested + @DisplayName("validateUserAccess()") + class ValidateUserAccess { + + @Test + @DisplayName("does not throw when auth is disabled") + void noOp_whenAuthDisabled() { + var validator = new OwnershipValidator(false); + assertDoesNotThrow(() -> validator.validateUserAccess(null, OTHER_USER)); + } + + @Test + @DisplayName("does not throw when auth is disabled, even with mismatched identity") + void noOp_whenAuthDisabled_withMismatchedIdentity() { + var validator = new OwnershipValidator(false); + var identity = authenticatedIdentity(CALLER_ID, false); + assertDoesNotThrow(() -> validator.validateUserAccess(identity, OTHER_USER)); + } + + @Test + @DisplayName("does not throw when identity is null (lets @RolesAllowed handle)") + void noOp_whenIdentityIsNull() { + var validator = new OwnershipValidator(true); + assertDoesNotThrow(() -> validator.validateUserAccess(null, OTHER_USER)); + } + + @Test + @DisplayName("does not throw when identity is anonymous") + void noOp_whenAnonymous() { + var validator = new OwnershipValidator(true); + assertDoesNotThrow(() -> validator.validateUserAccess(anonymousIdentity(), OTHER_USER)); + } + + @Test + @DisplayName("does not throw when caller is admin, even with userId mismatch") + void noOp_whenAdmin() { + var validator = new OwnershipValidator(true); + var admin = authenticatedIdentity(CALLER_ID, true); + assertDoesNotThrow(() -> validator.validateUserAccess(admin, OTHER_USER)); + } + + @Test + @DisplayName("does not throw when caller matches requestedUserId") + void noOp_whenCallerMatchesUserId() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + assertDoesNotThrow(() -> validator.validateUserAccess(identity, CALLER_ID)); + } + + @Test + @DisplayName("throws ForbiddenException when caller does not match requestedUserId") + void throws_whenCallerDoesNotMatchUserId() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + + var ex = assertThrows(ForbiddenException.class, + () -> validator.validateUserAccess(identity, OTHER_USER)); + assertTrue(ex.getMessage().contains("do not own")); + } + + @Test + @DisplayName("throws ForbiddenException when requestedUserId is null (caller doesn't match null)") + void throws_whenRequestedUserIdIsNull() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + + assertThrows(ForbiddenException.class, + () -> validator.validateUserAccess(identity, null)); + } + } + + // ==================== validateAndResolveUserId ==================== + + @Nested + @DisplayName("validateAndResolveUserId()") + class ValidateAndResolveUserId { + + @Test + @DisplayName("returns requestedUserId as-is when auth is disabled") + void returnsRequestedUserId_whenAuthDisabled() { + var validator = new OwnershipValidator(false); + assertEquals(OTHER_USER, validator.validateAndResolveUserId(null, OTHER_USER)); + } + + @Test + @DisplayName("returns null when auth is disabled and requestedUserId is null") + void returnsNull_whenAuthDisabled_andRequestedUserIdIsNull() { + var validator = new OwnershipValidator(false); + assertNull(validator.validateAndResolveUserId(null, null)); + } + + @Test + @DisplayName("returns requestedUserId as-is when identity is null") + void returnsRequestedUserId_whenIdentityIsNull() { + var validator = new OwnershipValidator(true); + assertEquals(OTHER_USER, validator.validateAndResolveUserId(null, OTHER_USER)); + } + + @Test + @DisplayName("returns requestedUserId as-is when identity is anonymous") + void returnsRequestedUserId_whenAnonymous() { + var validator = new OwnershipValidator(true); + assertEquals(OTHER_USER, validator.validateAndResolveUserId(anonymousIdentity(), OTHER_USER)); + } + + @Test + @DisplayName("returns caller's principal name when requestedUserId is null") + void returnsCallerId_whenRequestedUserIdIsNull() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + assertEquals(CALLER_ID, validator.validateAndResolveUserId(identity, null)); + } + + @Test + @DisplayName("returns caller's principal name when requestedUserId is blank") + void returnsCallerId_whenRequestedUserIdIsBlank() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + assertEquals(CALLER_ID, validator.validateAndResolveUserId(identity, " ")); + } + + @Test + @DisplayName("admin can impersonate — returns requestedUserId even if different") + void returnsRequestedUserId_whenAdminImpersonates() { + var validator = new OwnershipValidator(true); + var admin = authenticatedIdentity(CALLER_ID, true); + assertEquals(OTHER_USER, validator.validateAndResolveUserId(admin, OTHER_USER)); + } + + @Test + @DisplayName("returns requestedUserId when caller matches") + void returnsRequestedUserId_whenCallerMatches() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + assertEquals(CALLER_ID, validator.validateAndResolveUserId(identity, CALLER_ID)); + } + + @Test + @DisplayName("throws ForbiddenException when non-admin caller does not match requestedUserId") + void throws_whenNonAdminCallerMismatch() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + + var ex = assertThrows(ForbiddenException.class, + () -> validator.validateAndResolveUserId(identity, OTHER_USER)); + assertTrue(ex.getMessage().contains("cannot start a conversation as another user")); + } + } + + // ==================== requireOwnerOrAdmin ==================== + + @Nested + @DisplayName("requireOwnerOrAdmin()") + class RequireOwnerOrAdmin { + + @Test + @DisplayName("does not throw when auth is disabled") + void noOp_whenAuthDisabled() { + var validator = new OwnershipValidator(false); + assertDoesNotThrow(() -> validator.requireOwnerOrAdmin( + authenticatedIdentity(CALLER_ID, false), OTHER_USER, "agent")); + } + + @Test + @DisplayName("does not throw when resourceOwnerId is null (legacy data)") + void noOp_whenOwnerIdIsNull() { + var validator = new OwnershipValidator(true); + assertDoesNotThrow(() -> validator.requireOwnerOrAdmin( + authenticatedIdentity(CALLER_ID, false), null, "agent")); + } + + @Test + @DisplayName("does not throw when resourceOwnerId is blank (legacy data)") + void noOp_whenOwnerIdIsBlank() { + var validator = new OwnershipValidator(true); + assertDoesNotThrow(() -> validator.requireOwnerOrAdmin( + authenticatedIdentity(CALLER_ID, false), " ", "agent")); + } + + @Test + @DisplayName("does not throw when identity is null") + void noOp_whenIdentityIsNull() { + var validator = new OwnershipValidator(true); + assertDoesNotThrow(() -> validator.requireOwnerOrAdmin(null, OTHER_USER, "agent")); + } + + @Test + @DisplayName("does not throw when identity is anonymous") + void noOp_whenAnonymous() { + var validator = new OwnershipValidator(true); + assertDoesNotThrow(() -> validator.requireOwnerOrAdmin( + anonymousIdentity(), OTHER_USER, "agent")); + } + + @Test + @DisplayName("does not throw when caller is admin, even with owner mismatch") + void noOp_whenAdmin() { + var validator = new OwnershipValidator(true); + var admin = authenticatedIdentity(CALLER_ID, true); + assertDoesNotThrow(() -> validator.requireOwnerOrAdmin(admin, OTHER_USER, "agent")); + } + + @Test + @DisplayName("does not throw when caller matches resourceOwnerId") + void noOp_whenCallerMatchesOwner() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + assertDoesNotThrow(() -> validator.requireOwnerOrAdmin(identity, CALLER_ID, "agent")); + } + + @Test + @DisplayName("throws ForbiddenException when caller does not match resourceOwnerId") + void throws_whenCallerDoesNotMatchOwner() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + + var ex = assertThrows(ForbiddenException.class, + () -> validator.requireOwnerOrAdmin(identity, OTHER_USER, "agent")); + assertTrue(ex.getMessage().contains("agent")); + } + + @Test + @DisplayName("ForbiddenException message includes resourceType") + void exceptionMessage_containsResourceType() { + var validator = new OwnershipValidator(true); + var identity = authenticatedIdentity(CALLER_ID, false); + + var ex = assertThrows(ForbiddenException.class, + () -> validator.requireOwnerOrAdmin(identity, OTHER_USER, "conversation")); + assertTrue(ex.getMessage().contains("conversation"), + "Expected message to contain 'conversation', was: " + ex.getMessage()); + } + } +} From 3b348fe7c3f0c33e2f6d2ee3e1038ccf7e9313e3 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Wed, 10 Jun 2026 16:47:20 +0200 Subject: [PATCH 5/5] fix(security): remediate CodeQL log injection, fail-open ownership, and MCP validation order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - RestAgentEngine: sanitize conversationId in debug logs via LogSanitizer - OwnershipValidator: sanitize all user-provided values in debug logs - RestAgentEngine: fail-closed on ResourceStoreException in ownership check (previously silently skipped, allowing unauthorized access during DB errors) - McpMemoryTools: reorder validation — null/blank check before ownership check in all 5 read-only tools (prevents ForbiddenException on missing input) - Changelog: clarify MCP ownership fix chronology, add remediation entry --- docs/changelog.md | 15 ++++++++++++++- .../eddi/engine/internal/RestAgentEngine.java | 13 ++++++++----- .../ai/labs/eddi/engine/mcp/McpMemoryTools.java | 10 +++++----- .../eddi/engine/security/OwnershipValidator.java | 9 ++++++--- 4 files changed, 33 insertions(+), 14 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 2a7e56135..5e617bbec 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -31,7 +31,7 @@ ### Finding: MCP Memory Ownership (NEW → FIXED) - **Problem:** MCP memory read tools (`list_user_memories`, `get_visible_memories`, etc.) accepted `userId` as a tool parameter without validating against the caller's identity. -- **Fix:** Added `requireOwnerOrAdmin()` to `McpToolUtils` and applied it in all 5 read-only MCP memory tools. +- **Fix:** `McpMemoryTools` now injects `OwnershipValidator` and calls `validateUserAccess()` in all 5 read-only MCP memory tools (initially via `McpToolUtils.requireOwnerOrAdmin()`, consolidated to direct `OwnershipValidator` use in code review hardening below). ### New Component: OwnershipValidator - Centralized `@ApplicationScoped` utility for ownership checks @@ -72,6 +72,19 @@ **Total:** 184 security-related tests, 0 failures, 0 errors. **Files:** `OwnershipValidatorTest.java` [NEW], `RestAgentEngineTest.java`, `RestUserMemoryStoreTest.java`, `RestGroupConversationTest.java`, `McpMemoryToolsTest.java` +### GitHub Advanced Security / CodeQL Remediation (2026-06-10) + +**Repo:** EDDI (`fix/security-audit-idor-remediation`) +**What changed:** Addressed 12 CodeQL "Log Injection" findings and 5 Copilot validation-order findings from automated PR review. + +- **Log Injection — RestAgentEngine:** `validateConversationOwnership()` now sanitizes `conversationId` via `LogSanitizer.sanitize()` before logging. +- **Log Injection — OwnershipValidator:** All 3 debug-level log statements (`validateUserAccess`, `validateAndResolveUserId`, `requireOwnerOrAdmin`) now sanitize user-provided values (`callerId`, `requestedUserId`, `resourceOwnerId`, `resourceType`) via `LogSanitizer.sanitize()`. +- **Fail-closed ownership check:** `RestAgentEngine.validateConversationOwnership()` now throws `ForbiddenException` on `ResourceStoreException` instead of silently skipping the ownership check. Previous fail-open behavior could allow unauthorized access during transient DB errors. +- **MCP validation order:** In `McpMemoryTools`, all 5 read-only tools (`listUserMemories`, `getVisibleMemories`, `searchUserMemories`, `getMemoryByKey`, `countUserMemories`) now validate `userId` is non-null/non-blank **before** calling `ownershipValidator.validateUserAccess()`. Previously, a missing `userId` with auth enabled would throw `ForbiddenException` instead of the intended `"userId is required"` error JSON. +- **Changelog clarity:** Updated MCP ownership entry (line 32-35) to reflect final state — `OwnershipValidator.validateUserAccess()` is the sole mechanism, not `requireOwnerOrAdmin()` in `McpToolUtils`. + +**Files:** `RestAgentEngine.java`, `OwnershipValidator.java`, `McpMemoryTools.java`, `docs/changelog.md` + --- ## 🐛 Fix: Swagger UI Broken by CSP — Per-Path Filter Override (2026-06-03) diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java index 60cde8792..ef2d6e523 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java @@ -28,6 +28,7 @@ import org.eclipse.microprofile.config.inject.ConfigProperty; import org.jboss.logging.Logger; import static ai.labs.eddi.engine.exception.SneakyThrow.sneakyThrow; +import static ai.labs.eddi.utils.LogSanitizer.sanitize; import java.util.Collections; import java.util.HashMap; @@ -248,8 +249,9 @@ public Response redo(final String conversationId) { /** * Validates that the caller owns the conversation identified by * {@code conversationId}. Admin role bypasses the check. If the descriptor - * cannot be loaded (e.g., not found), the check is skipped and the actual - * operation will handle the error. + * cannot be loaded due to a store error, access is denied (fail-closed). If the + * descriptor is not found, the check is skipped and the actual operation will + * handle the 404. */ private void validateConversationOwnership(String conversationId) { try { @@ -259,10 +261,11 @@ private void validateConversationOwnership(String conversationId) { throw e; } catch (ResourceNotFoundException e) { // Descriptor not found — let the actual operation handle it - LOGGER.debugf("Conversation descriptor not found for %s", conversationId); + LOGGER.debugf("Conversation descriptor not found for %s", sanitize(conversationId)); } catch (ResourceStoreException e) { - // Store error — let the actual operation handle it - LOGGER.debugf("Could not load conversation descriptor for %s: %s", conversationId, e.getMessage()); + // Fail-closed: cannot verify ownership → deny access + LOGGER.warnf("Could not load conversation descriptor for ownership check: %s", sanitize(conversationId)); + throw new ForbiddenException("Access denied: unable to verify conversation ownership"); } } } diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java b/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java index 8d7a7be89..9ece08264 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpMemoryTools.java @@ -62,9 +62,9 @@ public McpMemoryTools(IUserMemoryStore userMemoryStore, IJsonSerialization jsonS public String listUserMemories(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Maximum number of entries to return (default: 50)") Integer limit) { requireRole(identity, authEnabled, "eddi-viewer"); - ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); + ownershipValidator.validateUserAccess(identity, userId); try { var entries = userMemoryStore.getAllEntries(userId); int maxEntries = limit != null && limit > 0 ? limit : 50; @@ -92,9 +92,9 @@ public String getVisibleMemories(@ToolArg(description = "User ID (required)") St @ToolArg(description = "Recall order: 'most_recent' or 'most_accessed' (default: most_recent)") String order, @ToolArg(description = "Maximum number of entries (default: 50)") Integer limit) { requireRole(identity, authEnabled, "eddi-viewer"); - ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); + ownershipValidator.validateUserAccess(identity, userId); if (agentId == null || agentId.isBlank()) return errorJson("agentId is required"); try { @@ -120,9 +120,9 @@ public String getVisibleMemories(@ToolArg(description = "User ID (required)") St public String searchUserMemories(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Search query (required)") String query) { requireRole(identity, authEnabled, "eddi-viewer"); - ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); + ownershipValidator.validateUserAccess(identity, userId); if (query == null || query.isBlank()) return errorJson("query is required"); try { @@ -144,9 +144,9 @@ public String searchUserMemories(@ToolArg(description = "User ID (required)") St public String getMemoryByKey(@ToolArg(description = "User ID (required)") String userId, @ToolArg(description = "Memory key name (required)") String key) { requireRole(identity, authEnabled, "eddi-viewer"); - ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); + ownershipValidator.validateUserAccess(identity, userId); if (key == null || key.isBlank()) return errorJson("key is required"); try { @@ -232,9 +232,9 @@ public String deleteAllUserMemories(@ToolArg(description = "User ID (required)") @Tool(name = "count_user_memories", description = "Count the number of memory entries for a user.") public String countUserMemories(@ToolArg(description = "User ID (required)") String userId) { requireRole(identity, authEnabled, "eddi-viewer"); - ownershipValidator.validateUserAccess(identity, userId); if (userId == null || userId.isBlank()) return errorJson("userId is required"); + ownershipValidator.validateUserAccess(identity, userId); try { long count = userMemoryStore.countEntries(userId); return jsonSerialization.serialize(Map.of("userId", userId, "count", count)); diff --git a/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java b/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java index 4c391de98..9b0928df6 100644 --- a/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java +++ b/src/main/java/ai/labs/eddi/engine/security/OwnershipValidator.java @@ -11,6 +11,8 @@ import org.eclipse.microprofile.config.inject.ConfigProperty; import org.jboss.logging.Logger; +import static ai.labs.eddi.utils.LogSanitizer.sanitize; + /** * Centralized ownership validation utility. Provides methods to assert that the * authenticated caller owns the resource they are attempting to access, or @@ -69,7 +71,7 @@ public void validateUserAccess(SecurityIdentity identity, String requestedUserId String callerId = identity.getPrincipal().getName(); if (!callerId.equals(requestedUserId)) { LOGGER.warnf("Ownership check failed: caller attempted to access another user's data"); - LOGGER.debugf("Ownership detail: caller='%s', requestedUserId='%s'", callerId, requestedUserId); + LOGGER.debugf("Ownership detail: caller='%s', requestedUserId='%s'", sanitize(callerId), sanitize(requestedUserId)); throw new ForbiddenException("Access denied: you do not own this user's data"); } } @@ -111,7 +113,7 @@ public String validateAndResolveUserId(SecurityIdentity identity, String request if (!callerId.equals(requestedUserId)) { LOGGER.warnf("UserId resolution rejected: caller attempted to impersonate another user"); - LOGGER.debugf("UserId resolution detail: caller='%s', requestedUserId='%s'", callerId, requestedUserId); + LOGGER.debugf("UserId resolution detail: caller='%s', requestedUserId='%s'", sanitize(callerId), sanitize(requestedUserId)); throw new ForbiddenException("Access denied: you cannot start a conversation as another user"); } @@ -153,7 +155,8 @@ public void requireOwnerOrAdmin(SecurityIdentity identity, String resourceOwnerI String callerId = identity.getPrincipal().getName(); if (!callerId.equals(resourceOwnerId)) { LOGGER.warnf("Ownership check failed: caller denied access to %s owned by another user", resourceType); - LOGGER.debugf("Ownership detail: caller='%s', resourceType='%s', ownerId='%s'", callerId, resourceType, resourceOwnerId); + LOGGER.debugf("Ownership detail: caller='%s', resourceType='%s', ownerId='%s'", sanitize(callerId), sanitize(resourceType), + sanitize(resourceOwnerId)); throw new ForbiddenException("Access denied: you do not own this " + resourceType); } }