From db6a989f708b131ac172b381cb6d2e9229e0371d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 17:57:37 +0200 Subject: [PATCH 1/2] fix(auth): propagate per-group fetch errors in GetUserPermissions Any transient store error fetching a group in GetUserPermissions or collectGroupsAndAccounts is now returned immediately rather than logged and skipped. Callers therefore fail closed with an error instead of silently receiving a partial permission union computed from the remaining groups (closes #918). A deleted/missing group (store returns nil, nil) is still skipped without error, preserving the existing behavior for that case. Remove unused logging import; update existing test that asserted the old swallow-and-continue behavior. --- internal/auth/service_group.go | 21 +++++++---- internal/auth/service_group_test.go | 56 +++++++++++++++++++++++++++++ internal/auth/service_test.go | 11 +++--- 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index b930192a1..09181bf3c 100644 --- a/internal/auth/service_group.go +++ b/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,10 @@ 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 + 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 +107,23 @@ 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 + 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 +138,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 diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index b5d9a3eed..b851379ec 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -405,6 +405,34 @@ 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) + }) } func TestService_BuildAuthContext(t *testing.T) { @@ -556,6 +584,34 @@ 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) + }) } func TestAuthContext_HasPermission(t *testing.T) { diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 107dae526..843d4418c 100644 --- a/internal/auth/service_test.go +++ b/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) }) From 99a83d7b83500a481d6b865dbe3f4a3ff727d50e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 19:50:13 +0200 Subject: [PATCH 2/2] fix(auth): skip pgx.ErrNoRows groups in GetUserPermissions and collectGroupsAndAccounts PostgresStore.GetGroup returns (nil, pgx.ErrNoRows) when a group row no longer exists. Without an explicit check, that error was treated as a transient failure and propagated, causing the entire permission lookup to fail for users whose group list includes a deleted group. Add errors.Is(err, pgx.ErrNoRows) guards in both GetUserPermissions and collectGroupsAndAccounts so a deleted group is skipped (same as the existing nil-group path), while any other store error still propagates. Add pgx.ErrNoRows test cases for both functions to cover the deleted-group skip path explicitly (CR #920 minor finding). --- internal/auth/service_group.go | 8 ++++ internal/auth/service_group_test.go | 61 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 09181bf3c..39c6de6b7 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -72,6 +72,10 @@ 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 { + 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 { @@ -120,6 +124,10 @@ func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthCon for _, groupID := range groupIDs { group, err := s.store.GetGroup(ctx, groupID) if err != nil { + 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 { diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index b851379ec..a27188734 100644 --- a/internal/auth/service_group_test.go +++ b/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" @@ -433,6 +434,35 @@ func TestService_GetUserPermissions(t *testing.T) { 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) { @@ -612,6 +642,37 @@ func TestService_BuildAuthContext(t *testing.T) { 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) {