From 15f8ef74099ed994b45bfd5af703ce1342b9145e Mon Sep 17 00:00:00 2001 From: Lynn Date: Tue, 28 Jul 2026 09:47:22 +0800 Subject: [PATCH] Fix: code review (#17442) --- internal/handler/memory.go | 22 ++++++++++--- internal/service/memory.go | 38 ++++++++++++++-------- internal/service/memory_message_service.go | 4 +-- internal/service/model_service.go | 4 +++ 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/internal/handler/memory.go b/internal/handler/memory.go index 6e54bec5a3..c890e93ccc 100644 --- a/internal/handler/memory.go +++ b/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 diff --git a/internal/service/memory.go b/internal/service/memory.go index 7ae1c27276..119d1c0efd 100644 --- a/internal/service/memory.go +++ b/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) diff --git a/internal/service/memory_message_service.go b/internal/service/memory_message_service.go index ac171ea89c..3ef75851ff 100644 --- a/internal/service/memory_message_service.go +++ b/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 diff --git a/internal/service/model_service.go b/internal/service/model_service.go index 13a93b9014..0eaf31bcf2 100644 --- a/internal/service/model_service.go +++ b/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 {