Fix: code review (#17442)

internal/handler/memory.go

@@ -264,10 +264,17 @@ func (h *MemoryHandler) DeleteMemory(c *gin.Context) {
 		common.ResponseWithCodeData(c, common.CodeArgumentError, nil, "memory ID is required")
 		return
 	}
+
+	// Get current logged-in user for access control
+	user, errorCode, errorMessage := GetUser(c)
+	if errorCode != common.CodeSuccess {
+		common.ErrorWithCode(c, errorCode, errorMessage)
+		return
+	}
 	ctx := c.Request.Context()
 
-	// Call service layer to delete memory
-	err := h.memoryService.DeleteMemory(ctx, memoryID)
+	// Call service layer to delete memory (with access control)
+	err := h.memoryService.DeleteMemory(ctx, user.ID, memoryID)
 	if err != nil {
 		errMsg := err.Error()
 		// Check if it's a "not found" error
@@ -386,10 +393,17 @@ func (h *MemoryHandler) GetMemoryConfig(c *gin.Context) {
 		common.ResponseWithCodeData(c, common.CodeArgumentError, nil, "memory ID is required")
 		return
 	}
+
+	// Get current logged-in user for access control
+	user, errorCode, errorMessage := GetUser(c)
+	if errorCode != common.CodeSuccess {
+		common.ErrorWithCode(c, errorCode, errorMessage)
+		return
+	}
 	ctx := c.Request.Context()
 
-	// Call service layer to get memory configuration
-	result, err := h.memoryService.GetMemoryConfig(ctx, memoryID)
+	// Call service layer to get memory configuration (with access control)
+	result, err := h.memoryService.GetMemoryConfig(ctx, user.ID, memoryID)
 	if err != nil {
 		errMsg := err.Error()
 		// Check if it's a "not found" error

internal/service/memory.go

@@ -776,22 +776,23 @@ func sameStringSet(a, b []string) bool {
 	return true
 }
 
-// DeleteMemory deletes a memory by ID
-// It also deletes associated message indexes before removing the memory record
+// DeleteMemory deletes a memory by ID after verifying the caller's access.
+// It also deletes associated message indexes before removing the memory record.
 //
 // Parameters:
+//   - userID: The ID of the user requesting the deletion (access control)
 //   - memoryID: The ID of the memory to delete
 //
 // Returns:
-//   - error: Error if memory not found or deletion fails
+//   - error: Error if memory not found, access denied, or deletion fails
 //
 // Example:
 //
-//	err := service.DeleteMemory("memory456")
-func (s *MemoryService) DeleteMemory(ctx context.Context, memoryID string) error {
-	_, err := s.memoryDAO.GetByID(ctx, dao.DB, memoryID)
-	if err != nil {
-		return fmt.Errorf("memory '%s' not found", memoryID)
+//	err := service.DeleteMemory(ctx, "user123", "memory456")
+func (s *MemoryService) DeleteMemory(ctx context.Context, userID, memoryID string) error {
+	// Verify the caller has access to this memory
+	if _, err := s.requireMemoryAccess(ctx, userID, memoryID); err != nil {
+		return err
 	}
 
 	// TODO: Delete associated message index - Implementation pending MessageService
@@ -802,7 +803,7 @@ func (s *MemoryService) DeleteMemory(ctx context.Context, memoryID string) error
 	// }
 
 	// Delete memory record
-	if err = s.memoryDAO.DeleteByID(ctx, dao.DB, memoryID); err != nil {
+	if err := s.memoryDAO.DeleteByID(ctx, dao.DB, memoryID); err != nil {
 		return errors.New("failed to delete memory")
 	}
 
@@ -1529,19 +1530,30 @@ func ResolveTenantModelDisplayName(ctx context.Context, db *gorm.DB, tenantModel
 	return displayName
 }
 
-// GetMemoryConfig retrieves the full configuration of a memory by ID
+// GetMemoryConfig retrieves the full configuration of a memory by ID after
+// verifying the caller's access.
 //
 // Parameters:
+//   - userID: The ID of the user requesting the configuration (access control)
 //   - memoryID: The ID of the memory to retrieve
 //
 // Returns:
 //   - *CreateMemoryResponse: The memory configuration details
-//   - error: Error if memory not found
+//   - error: Error if memory not found or access denied
 //
 // Example:
 //
-//	resp, err := service.GetMemoryConfig("memory456")
-func (s *MemoryService) GetMemoryConfig(ctx context.Context, memoryID string) (*CreateMemoryResponse, error) {
+//	resp, err := service.GetMemoryConfig(ctx, "user123", "memory456")
+func (s *MemoryService) GetMemoryConfig(ctx context.Context, userID, memoryID string) (*CreateMemoryResponse, error) {
+	if _, err := s.requireMemoryAccess(ctx, userID, memoryID); err != nil {
+		return nil, err
+	}
+	return s.getMemoryConfig(ctx, memoryID)
+}
+
+// getMemoryConfig retrieves the full configuration of a memory without access
+// control checks. This is for trusted internal callers such as queue processing.
+func (s *MemoryService) getMemoryConfig(ctx context.Context, memoryID string) (*CreateMemoryResponse, error) {
 	memory, err := s.memoryDAO.GetWithOwnerNameByID(ctx, dao.DB, memoryID)
 	if err != nil {
 		return nil, fmt.Errorf("memory '%s' not found", memoryID)

internal/service/memory_message_service.go

@@ -153,8 +153,8 @@ func (s *MemoryMessageService) QueueSaveToMemoryTask(
 
 	res := &QueueSaveResult{}
 	for _, memoryID := range memoryIDs {
-		// (1) Look up the memory.
-		mem, err := s.memories.GetMemoryConfig(ctx, memoryID)
+		// (1) Look up the memory (no access control — trusted internal queue processing).
+		mem, err := s.memories.getMemoryConfig(ctx, memoryID)
 		if err != nil {
 			res.NotFound = append(res.NotFound, memoryID)
 			continue

internal/service/model_service.go

@@ -1020,6 +1020,10 @@ func verifyProviderModel(ctx context.Context, driver modelModule.ModelDriver, pr
 					ModelTypes: modelTypes,
 				})
 			}
+		} else {
+			zap.L().Warn("failed to list remote models from provider",
+				zap.Error(listErr),
+			)
 		}
 
 		if len(modelsToVerify) == 0 {