fix(auth): propagate per-group fetch errors in GetUserPermissions

internal/auth/service_group.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/LeanerCloud/CUDly/pkg/logging"
 	"github.com/google/uuid"
 	"github.com/jackc/pgx/v5"
 )
@@ -50,6 +49,11 @@ func (s *Service) ListGroups(ctx context.Context) ([]Group, error) {
 // derived purely from the union of the user's groups' permissions: there is
 // no role-based fallback. A user with no groups therefore has no permissions
 // and is denied everything (fail closed).
+//
+// Any transient store error fetching a group is propagated immediately so
+// callers fail closed with an error rather than silently receiving a partial
+// permission set. A nil group (the store returns nil, nil for a deleted/
+// missing group) is skipped without error.
 func (s *Service) GetUserPermissions(ctx context.Context, userID string) ([]Permission, error) {
 	user, err := s.store.GetUserByID(ctx, userID)
 	if err != nil {
@@ -68,10 +72,14 @@ func (s *Service) GetUserPermissions(ctx context.Context, userID string) ([]Perm
 	for _, groupID := range user.GroupIDs {
 		group, err := s.store.GetGroup(ctx, groupID)
 		if err != nil {
-			logging.Warnf("Failed to fetch group %s: %v", groupID, err)
-			continue
+			if errors.Is(err, pgx.ErrNoRows) {
+				// Group was deleted; skip it rather than failing the entire request.
+				continue
+			}
+			return nil, fmt.Errorf("fetching group %s: %w", groupID, err)
 		}
 		if group == nil {
+			// Group was deleted; skip it rather than failing the entire request.
 			continue
 		}
 		permissions = append(permissions, group.Permissions...)
@@ -103,21 +111,27 @@ func (s *Service) BuildAuthContext(ctx context.Context, userID string) (*AuthCon
 		Permissions:     make([]Permission, 0),
 	}
 
-	s.collectGroupsAndAccounts(ctx, authCtx, user.GroupIDs)
+	if err := s.collectGroupsAndAccounts(ctx, authCtx, user.GroupIDs); err != nil {
+		return nil, err
+	}
 
 	return authCtx, nil
 }
 
-func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthContext, groupIDs []string) {
+func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthContext, groupIDs []string) error {
 	accountSet := make(map[string]bool)
 
 	for _, groupID := range groupIDs {
 		group, err := s.store.GetGroup(ctx, groupID)
 		if err != nil {
-			logging.Warnf("Failed to fetch group %s: %v", groupID, err)
-			continue
+			if errors.Is(err, pgx.ErrNoRows) {
+				// Group was deleted; skip it rather than failing the entire request.
+				continue
+			}
+			return fmt.Errorf("fetching group %s: %w", groupID, err)
 		}
 		if group == nil {
+			// Group was deleted; skip it rather than failing the entire request.
 			continue
 		}
 
@@ -132,6 +146,7 @@ func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthCon
 	for accountID := range accountSet {
 		authCtx.AllowedAccounts = append(authCtx.AllowedAccounts, accountID)
 	}
+	return nil
 }
 
 // GetAuthContext is an alias for BuildAuthContext for backward compatibility

internal/auth/service_group_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"testing"
 
+	"github.com/jackc/pgx/v5"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -405,6 +406,63 @@ func TestService_GetUserPermissions(t *testing.T) {
 
 		mockStore.AssertExpectations(t)
 	})
+
+	t.Run("propagates per-group fetch error instead of returning partial permissions", func(t *testing.T) {
+		// Regression test for issue #918: a transient store error on one group
+		// must propagate as an error, not silently compute a partial union.
+		mockStore := new(MockStore)
+		mockEmail := new(MockEmailSender)
+		service := createTestService(mockStore, mockEmail)
+
+		user := &User{
+			ID:       "user-123",
+			GroupIDs: []string{"group-ok", "group-err"},
+		}
+		okGroup := &Group{
+			ID:          "group-ok",
+			Name:        "OK Group",
+			Permissions: DefaultUserPermissions(),
+		}
+
+		mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil).Once()
+		mockStore.On("GetGroup", ctx, "group-ok").Return(okGroup, nil).Once()
+		mockStore.On("GetGroup", ctx, "group-err").Return(nil, assert.AnError).Once()
+
+		permissions, err := service.GetUserPermissions(ctx, "user-123")
+		require.Error(t, err, "a per-group fetch error must propagate")
+		assert.Nil(t, permissions, "no partial permission set must be returned")
+
+		mockStore.AssertExpectations(t)
+	})
+
+	t.Run("skips deleted group (pgx.ErrNoRows) without error", func(t *testing.T) {
+		// pgx.ErrNoRows from the store means the group row was deleted between
+		// the user-load and the group-load; treat it as "missing" and skip.
+		mockStore := new(MockStore)
+		mockEmail := new(MockEmailSender)
+		service := createTestService(mockStore, mockEmail)
+
+		user := &User{
+			ID:       "user-123",
+			GroupIDs: []string{"standard-group", "deleted-group"},
+		}
+		standardGrp := &Group{
+			ID:          "standard-group",
+			Name:        "Standard Users",
+			Permissions: DefaultUserPermissions(),
+		}
+
+		mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil).Once()
+		mockStore.On("GetGroup", ctx, "standard-group").Return(standardGrp, nil).Once()
+		mockStore.On("GetGroup", ctx, "deleted-group").Return(nil, pgx.ErrNoRows).Once()
+
+		permissions, err := service.GetUserPermissions(ctx, "user-123")
+		require.NoError(t, err, "a pgx.ErrNoRows group must be skipped, not propagated")
+		// Only the resolvable group's permissions; deleted group excluded.
+		assert.Len(t, permissions, len(DefaultUserPermissions()))
+
+		mockStore.AssertExpectations(t)
+	})
 }
 
 func TestService_BuildAuthContext(t *testing.T) {
@@ -556,6 +614,65 @@ func TestService_BuildAuthContext(t *testing.T) {
 
 		mockStore.AssertExpectations(t)
 	})
+
+	t.Run("propagates per-group fetch error instead of returning partial context", func(t *testing.T) {
+		// Regression test for issue #918: a transient store error on one group
+		// must propagate, not silently compute a partial auth context.
+		mockStore := new(MockStore)
+		mockEmail := new(MockEmailSender)
+		service := createTestService(mockStore, mockEmail)
+
+		user := &User{
+			ID:       "user-123",
+			GroupIDs: []string{"group-ok", "group-err"},
+		}
+		okGroup := &Group{
+			ID:              "group-ok",
+			Name:            "OK Group",
+			AllowedAccounts: []string{"111111111111"},
+		}
+
+		mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil).Once()
+		mockStore.On("GetGroup", ctx, "group-ok").Return(okGroup, nil).Once()
+		mockStore.On("GetGroup", ctx, "group-err").Return(nil, assert.AnError).Once()
+
+		authCtx, err := service.BuildAuthContext(ctx, "user-123")
+		require.Error(t, err, "a per-group fetch error must propagate")
+		assert.Nil(t, authCtx, "no partial auth context must be returned")
+
+		mockStore.AssertExpectations(t)
+	})
+
+	t.Run("skips deleted group (pgx.ErrNoRows) without error", func(t *testing.T) {
+		// pgx.ErrNoRows from the store means the group row was deleted between
+		// the user-load and the group-load; treat it as "missing" and skip.
+		mockStore := new(MockStore)
+		mockEmail := new(MockEmailSender)
+		service := createTestService(mockStore, mockEmail)
+
+		user := &User{
+			ID:       "user-123",
+			GroupIDs: []string{"valid-group", "deleted-group"},
+		}
+		validGroup := &Group{
+			ID:              "valid-group",
+			Name:            "Valid Group",
+			AllowedAccounts: []string{"111111111111"},
+		}
+
+		mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil).Once()
+		mockStore.On("GetGroup", ctx, "valid-group").Return(validGroup, nil).Once()
+		mockStore.On("GetGroup", ctx, "deleted-group").Return(nil, pgx.ErrNoRows).Once()
+
+		authCtx, err := service.BuildAuthContext(ctx, "user-123")
+		require.NoError(t, err, "a pgx.ErrNoRows group must be skipped, not propagated")
+		assert.NotNil(t, authCtx)
+		assert.Len(t, authCtx.Groups, 1, "only the present group appears")
+		assert.Len(t, authCtx.AllowedAccounts, 1)
+		assert.Contains(t, authCtx.AllowedAccounts, "111111111111")
+
+		mockStore.AssertExpectations(t)
+	})
 }
 
 func TestAuthContext_HasPermission(t *testing.T) {

internal/auth/service_test.go

@@ -644,6 +644,9 @@ func TestService_ErrorPaths(t *testing.T) {
 	})
 
 	t.Run("GetUserPermissions with store error on group", func(t *testing.T) {
+		// Regression test for issue #918: a transient group-fetch error must
+		// propagate so callers fail closed with an error rather than silently
+		// receiving a partial (or empty) permission set.
 		mockStore := new(MockStore)
 		mockEmail := new(MockEmailSender)
 		service := createTestService(mockStore, mockEmail)
@@ -657,12 +660,8 @@ func TestService_ErrorPaths(t *testing.T) {
 		mockStore.On("GetGroup", ctx, "group-1").Return(nil, fmt.Errorf("database error")).Once()
 
 		permissions, err := service.GetUserPermissions(ctx, "user-123")
-		require.NoError(t, err)
-		// A failing group fetch is logged and skipped rather than aborting the
-		// whole resolution (so a partially broken group set still yields the
-		// other groups' permissions). Here the sole group errored and there is
-		// no role fallback, so the effective permission set is empty.
-		assert.Empty(t, permissions)
+		require.Error(t, err, "a per-group fetch error must propagate")
+		assert.Nil(t, permissions)
 
 		mockStore.AssertExpectations(t)
 	})