Fix/mcp conversation ownership

src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java

@@ -13,7 +13,6 @@
 import ai.labs.eddi.engine.hitl.tools.IHitlToolJournalStore;
 import ai.labs.eddi.engine.api.IRestAgentEngine;
 import ai.labs.eddi.engine.lifecycle.model.HitlDecision;
-import ai.labs.eddi.engine.memory.descriptor.IConversationDescriptorStore;
 import ai.labs.eddi.engine.model.PendingApprovalSummary;
 import ai.labs.eddi.engine.memory.model.ConversationMemorySnapshot;
 import ai.labs.eddi.engine.memory.model.ConversationMemorySnapshot.ConversationStepSnapshot;
@@ -26,8 +25,8 @@
 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.ConversationAccessGuard;
 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;
@@ -63,9 +62,9 @@
 public class RestAgentEngine implements IRestAgentEngine {
 
     private final IConversationService conversationService;
-    private final IConversationDescriptorStore conversationDescriptorStore;
     private final SecurityIdentity identity;
     private final OwnershipValidator ownershipValidator;
+    private final ConversationAccessGuard conversationAccessGuard;
     private final HitlAccessGuard hitlAccessGuard;
     private final IHitlToolJournalStore hitlToolJournalStore;
     private final int agentTimeout;
@@ -74,16 +73,16 @@ public class RestAgentEngine implements IRestAgentEngine {
 
     @Inject
     public RestAgentEngine(IConversationService conversationService,
-            IConversationDescriptorStore conversationDescriptorStore,
             SecurityIdentity identity,
             OwnershipValidator ownershipValidator,
+            ConversationAccessGuard conversationAccessGuard,
             HitlAccessGuard hitlAccessGuard,
             IHitlToolJournalStore hitlToolJournalStore,
             @ConfigProperty(name = "systemRuntime.agentTimeoutInSeconds") int agentTimeout) {
         this.conversationService = conversationService;
-        this.conversationDescriptorStore = conversationDescriptorStore;
         this.identity = identity;
         this.ownershipValidator = ownershipValidator;
+        this.conversationAccessGuard = conversationAccessGuard;
         this.hitlAccessGuard = hitlAccessGuard;
         this.hitlToolJournalStore = hitlToolJournalStore;
         this.agentTimeout = agentTimeout;
@@ -554,20 +553,9 @@ private String validateConversationOwnership(String conversationId, boolean hitl
             // descriptor) is shared with the MCP surface via HitlAccessGuard.
             return hitlAccessGuard.requireConversationHitlAccess(conversationId);
         }
-        try {
-            var descriptor = conversationDescriptorStore.readDescriptor(conversationId, 0);
-            ownershipValidator.requireOwnerOrAdmin(identity, descriptor.getUserId(), "conversation");
-            return descriptor.getUserId();
-        } catch (ForbiddenException e) {
-            throw e;
-        } catch (ResourceNotFoundException e) {
-            // Descriptor not found — let the actual operation handle it
-            LOGGER.debugf("Conversation descriptor not found for %s", sanitize(conversationId));
-            return null;
-        } catch (ResourceStoreException e) {
-            // 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");
-        }
+        // Plain owner-or-admin access is shared with the MCP surface via
+        // ConversationAccessGuard, so REST and MCP cannot drift apart on who may
+        // read or drive a conversation.
+        return conversationAccessGuard.requireConversationOwner(conversationId);
     }
 }

src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java

@@ -19,6 +19,7 @@
 import ai.labs.eddi.engine.memory.model.ConversationStatus;
 import ai.labs.eddi.engine.runtime.IRuntime;
 import ai.labs.eddi.engine.runtime.ThreadContext;
+import ai.labs.eddi.engine.security.ConversationAccessGuard;
 import io.quarkus.scheduler.Scheduled;
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.enterprise.inject.Instance;
@@ -51,12 +52,22 @@
 public class RestConversationStore implements IRestConversationStore {
     public static final String DESCRIPTOR_TYPE = "ai.labs.conversation";
 
+    /**
+     * Upper bound on how many descriptors an owner-filtered listing will scan
+     * before giving up, so a caller who owns few/none of a large shared store
+     * cannot turn one list request into a full-collection scan. Mirrors the MCP
+     * conversation tools' owner-scan cap. Admins / auth-disabled callers are not
+     * filtered and never reach this bound.
+     */
+    private static final int MAX_OWNER_SCAN = 500;
+
     private final IDocumentDescriptorStore documentDescriptorStore;
     private final IConversationDescriptorStore conversationDescriptorStore;
     private final IConversationMemoryStore conversationMemoryStore;
     private final IConversationService conversationService;
     private final IUserMemoryStore userMemoryStore;
     private final IRuntime runtime;
+    private final ConversationAccessGuard conversationAccessGuard;
     private final Integer deleteEndedConversationsOnceOlderThanDays;
     private final Integer deleteMemoriesOlderThanDays;
     private final Instance<IAttachmentStore> attachmentStorageInstance;
@@ -72,6 +83,7 @@ public RestConversationStore(
             IConversationService conversationService,
             IUserMemoryStore userMemoryStore,
             IRuntime runtime,
+            ConversationAccessGuard conversationAccessGuard,
             @ConfigProperty(name = "eddi.conversations.deleteEndedConversationsOnceOlderThanDays")
             Integer deleteEndedConversationsOnceOlderThanDays,
             @ConfigProperty(name = "eddi.usermemories.deleteOlderThanDays")
@@ -85,6 +97,7 @@ public RestConversationStore(
         this.conversationService = conversationService;
         this.userMemoryStore = userMemoryStore;
         this.runtime = runtime;
+        this.conversationAccessGuard = conversationAccessGuard;
         this.deleteEndedConversationsOnceOlderThanDays = deleteEndedConversationsOnceOlderThanDays;
         this.deleteMemoriesOlderThanDays = deleteMemoriesOlderThanDays;
         this.attachmentStorageInstance = attachmentStorageInstance;
@@ -105,9 +118,22 @@ public List<ConversationDescriptor> readConversationDescriptors(Integer index, I
             limit = 100;
         }
 
+        // Owner-scoping: a non-admin caller may only enumerate their own
+        // conversations. The descriptor store has no owner-scoped query, so we
+        // post-filter each descriptor by its resolved owner. Admins (and any caller
+        // when authorization is disabled) see all — resolved once, up front, so the
+        // per-row check is skipped entirely on that path. The existing do-while
+        // back-fills across pages so a filtered-out row does not starve a personal
+        // list, but that back-fill is bounded by MAX_OWNER_SCAN so a caller who owns
+        // few/none of a large shared store cannot force a full-collection scan
+        // (descriptors come back most-recent-first, so a typical caller's own
+        // conversations fall well within the budget).
+        final boolean seesAllConversations = conversationAccessGuard.seesAllConversations();
+
         try {
             List<ConversationDescriptor> conversationDescriptors;
             List<ConversationDescriptor> retConversationDescriptors = new LinkedList<>();
+            int scannedDescriptors = 0;
 
             do {
                 conversationDescriptors = readConversationDescriptors(index, limit, filter);
@@ -116,6 +142,7 @@ public List<ConversationDescriptor> readConversationDescriptors(Integer index, I
                 }
 
                 for (var conversationDescriptor : conversationDescriptors) {
+                    scannedDescriptors++;
                     try {
                         URI resourceUri = conversationDescriptor.getResource();
                         var conversationResourceId = extractResourceId(resourceUri);
@@ -124,8 +151,31 @@ public List<ConversationDescriptor> readConversationDescriptors(Integer index, I
                             continue;
                         }
 
+                        // Ownership gate — split around the expensive snapshot load so a
+                        // foreign conversation is skipped WITHOUT loading its memory
+                        // document. Every conversation since v5.1.6 records its owner on
+                        // the descriptor, so decide here for the common case; only a
+                        // legacy row with no recorded owner falls through to the
+                        // post-populate re-check below (populate resolves its owner from
+                        // the snapshot). A fully unowned (null both ways) conversation
+                        // stays visible, matching OwnershipValidator.requireOwnerOrAdmin.
+                        String recordedOwner = conversationDescriptor.getUserId();
+                        boolean ownerRecorded = !isNullOrEmpty(recordedOwner);
+                        if (!seesAllConversations && ownerRecorded
+                                && !conversationAccessGuard.canAccessConversation(recordedOwner)) {
+                            continue;
+                        }
+
                         populateDataToDescriptor(conversationDescriptor, conversationResourceId);
 
+                        // Legacy safety net: the descriptor recorded no owner; populate
+                        // has now resolved it from the snapshot (pre-v5.1.6 fallback), so
+                        // re-check before returning it.
+                        if (!seesAllConversations && !ownerRecorded
+                                && !conversationAccessGuard.canAccessConversation(conversationDescriptor.getUserId())) {
+                            continue;
+                        }
+
                         // Agent filtering uses the agentResource URI (which contains
                         // the agent's ID), NOT the conversation's resource URI.
                         if (!isNullOrEmpty(agentId)) {
@@ -164,7 +214,11 @@ public List<ConversationDescriptor> readConversationDescriptors(Integer index, I
                 } else {
                     break; // prevent integer overflow
                 }
-            } while (!conversationDescriptors.isEmpty() && retConversationDescriptors.size() < limit);
+                // Bound the owner-filtered back-fill: stop once the scan budget is spent
+                // (admins/auth-disabled are never filtered, so they page only as far as
+                // filling `limit` requires and never hit this).
+            } while (!conversationDescriptors.isEmpty() && retConversationDescriptors.size() < limit
+                    && (seesAllConversations || scannedDescriptors < MAX_OWNER_SCAN));
 
             return retConversationDescriptors;
 

src/main/java/ai/labs/eddi/engine/security/ConversationAccessGuard.java

@@ -0,0 +1,134 @@
+/*
+ * Copyright EDDI contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package ai.labs.eddi.engine.security;
+
+import ai.labs.eddi.datastore.IResourceStore.ResourceNotFoundException;
+import ai.labs.eddi.datastore.IResourceStore.ResourceStoreException;
+import ai.labs.eddi.engine.memory.descriptor.IConversationDescriptorStore;
+import io.quarkus.security.ForbiddenException;
+import io.quarkus.security.identity.SecurityIdentity;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+import org.jboss.logging.Logger;
+
+import static ai.labs.eddi.utils.LogSanitizer.sanitize;
+
+/**
+ * Single source of truth for <em>who may access a (1:1) conversation</em>,
+ * shared by the REST layer ({@code RestAgentEngine}) and the MCP layer
+ * ({@code McpConversationTools}).
+ * <p>
+ * It is the non-HITL sibling of
+ * {@link ai.labs.eddi.engine.hitl.HitlAccessGuard}: same composition (delegate
+ * role/identity decisions to {@link OwnershipValidator}, resolve the owner
+ * through the conversation descriptor), but plain owner-or-admin semantics —
+ * the {@code eddi-approver} role grants nothing here, because reading or
+ * driving a conversation is not deciding a pending approval.
+ * <p>
+ * Extracting the check keeps "who may read this conversation" from drifting
+ * between the surfaces: a role check alone (e.g. {@code eddi-viewer}) is a
+ * coarse gate that says nothing about <em>whose</em> conversation is being
+ * read.
+ *
+ * @author ginccc
+ */
+@ApplicationScoped
+public class ConversationAccessGuard {
+
+    private static final Logger LOGGER = Logger.getLogger(ConversationAccessGuard.class);
+    private static final String RESOURCE_TYPE = "conversation";
+
+    private final SecurityIdentity identity;
+    private final OwnershipValidator ownershipValidator;
+    private final IConversationDescriptorStore conversationDescriptorStore;
+
+    @Inject
+    public ConversationAccessGuard(SecurityIdentity identity,
+            OwnershipValidator ownershipValidator,
+            IConversationDescriptorStore conversationDescriptorStore) {
+        this.identity = identity;
+        this.ownershipValidator = ownershipValidator;
+        this.conversationDescriptorStore = conversationDescriptorStore;
+    }
+
+    /**
+     * Asserts that the caller owns the conversation, or holds {@code eddi-admin}. A
+     * conversation whose descriptor records no owner (legacy data) is left
+     * accessible — {@link OwnershipValidator#requireOwnerOrAdmin} decides that.
+     *
+     * @return the conversation owner's userId, or {@code null} when the descriptor
+     *         was not found (the caller's actual operation then produces the 404).
+     * @throws ForbiddenException
+     *             if the caller is neither the owner nor an admin, or if the
+     *             descriptor cannot be loaded at all (fail-closed: an unverifiable
+     *             owner is not an absent owner).
+     */
+    public String requireConversationOwner(String conversationId) {
+        try {
+            var descriptor = conversationDescriptorStore.readDescriptor(conversationId, 0);
+            if (descriptor == null) {
+                LOGGER.debugf("Conversation descriptor not found for %s", sanitize(conversationId));
+                return null;
+            }
+            ownershipValidator.requireOwnerOrAdmin(identity, descriptor.getUserId(), RESOURCE_TYPE);
+            return descriptor.getUserId();
+        } catch (ForbiddenException e) {
+            throw e;
+        } catch (ResourceNotFoundException e) {
+            LOGGER.debugf("Conversation descriptor not found for %s", sanitize(conversationId));
+            return null;
+        } catch (ResourceStoreException e) {
+            LOGGER.warnf("Could not load conversation descriptor for ownership check: %s", sanitize(conversationId));
+            throw new ForbiddenException("Access denied: unable to verify conversation ownership");
+        }
+    }
+
+    /**
+     * Non-throwing counterpart of {@link #requireConversationOwner} for filtering
+     * listings, where a denied entry must be omitted rather than raise. It admits
+     * exactly what {@code requireConversationOwner} admits — admin, owner, or an
+     * unowned (legacy) conversation — so a caller never lists a conversation they
+     * could not read, nor reads one they could not list.
+     *
+     * @param conversationOwnerId
+     *            the owner recorded on the conversation descriptor (may be null)
+     */
+    public boolean canAccessConversation(String conversationOwnerId) {
+        if (ownershipValidator.isAdmin(identity)) {
+            // Also true when authorization is disabled — every caller sees everything.
+            return true;
+        }
+        if (conversationOwnerId == null || conversationOwnerId.isBlank()) {
+            return true; // legacy data without ownership — same as requireOwnerOrAdmin
+        }
+        return ownershipValidator.isOwner(identity, conversationOwnerId);
+    }
+
+    /**
+     * Whether the caller sees every conversation regardless of owner — an admin, or
+     * anyone at all when authorization is disabled. Lets a listing skip owner
+     * filtering (and the over-fetching it needs) entirely.
+     */
+    public boolean seesAllConversations() {
+        return ownershipValidator.isAdmin(identity);
+    }
+
+    /**
+     * Resolves the owner to stamp on a <em>new</em> conversation: the caller's own
+     * identity, unless an admin explicitly starts one on another user's behalf.
+     * <p>
+     * Without this, a conversation started with a {@code null} userId is persisted
+     * under a generated {@code anonymous-*} id that matches no principal — it would
+     * belong to nobody and {@link #requireConversationOwner} could never admit its
+     * own creator.
+     *
+     * @throws ForbiddenException
+     *             if a non-admin caller tries to start a conversation as another
+     *             user
+     */
+    public String resolveOwnerUserId(String requestedUserId) {
+        return ownershipValidator.validateAndResolveUserId(identity, requestedUserId);
+    }
+}

src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.java

@@ -16,6 +16,7 @@
 import ai.labs.eddi.engine.lifecycle.model.HitlDecision;
 import ai.labs.eddi.engine.lifecycle.model.HitlDecision.HitlVerdict;
 import ai.labs.eddi.engine.hitl.HitlAccessGuard;
+import ai.labs.eddi.engine.security.ConversationAccessGuard;
 import ai.labs.eddi.engine.security.OwnershipValidator;
 import io.quarkus.security.ForbiddenException;
 import io.quarkus.security.identity.SecurityIdentity;
@@ -79,9 +80,11 @@ void setUp() throws Exception {
         var hitlAccessGuard = new HitlAccessGuard(
                 identity, ownershipValidator, conversationDescriptorStore, conversationService,
                 mock(ai.labs.eddi.engine.api.IGroupConversationService.class));
+        var conversationAccessGuard = new ConversationAccessGuard(
+                identity, ownershipValidator, conversationDescriptorStore);
         restAgentEngine = new RestAgentEngine(
-                conversationService, conversationDescriptorStore,
-                identity, ownershipValidator, hitlAccessGuard, hitlToolJournalStore, AGENT_TIMEOUT);
+                conversationService, identity, ownershipValidator, conversationAccessGuard,
+                hitlAccessGuard, hitlToolJournalStore, AGENT_TIMEOUT);
     }
 
     // =========================================================================

src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java

@@ -16,6 +16,7 @@
 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.ConversationAccessGuard;
 import ai.labs.eddi.engine.security.OwnershipValidator;
 import io.quarkus.security.ForbiddenException;
 import io.quarkus.security.identity.SecurityIdentity;
@@ -58,9 +59,10 @@ void setUp() throws Exception {
                 .thenThrow(new ResourceNotFoundException("test default"));
         var hitlAccessGuard = new HitlAccessGuard(identity, ownershipValidator, descriptorStore, conversationService,
                 mock(ai.labs.eddi.engine.api.IGroupConversationService.class));
+        var conversationAccessGuard = new ConversationAccessGuard(identity, ownershipValidator, descriptorStore);
         var hitlToolJournalStore = mock(ai.labs.eddi.engine.hitl.tools.IHitlToolJournalStore.class);
-        restAgentEngine = new RestAgentEngine(conversationService, descriptorStore, identity, ownershipValidator,
-                hitlAccessGuard, hitlToolJournalStore, 30);
+        restAgentEngine = new RestAgentEngine(conversationService, identity, ownershipValidator,
+                conversationAccessGuard, hitlAccessGuard, hitlToolJournalStore, 30);
     }
 
     @Nested