From 483fdef3842be18e2143650aaabb61f78e5fd677 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 25 Feb 2026 10:55:34 +0100 Subject: [PATCH] groupmapper: avoid mutating original group users slice when removing user --- pkg/groupmapper/groupmapper.go | 24 ++---- pkg/groupmapper/groupmapper_test.go | 117 ++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 16 deletions(-) diff --git a/pkg/groupmapper/groupmapper.go b/pkg/groupmapper/groupmapper.go index 6fc78fd46..185de2be0 100644 --- a/pkg/groupmapper/groupmapper.go +++ b/pkg/groupmapper/groupmapper.go @@ -3,6 +3,7 @@ package groupmapper import ( "context" "fmt" + "slices" "time" "k8s.io/apimachinery/pkg/api/errors" @@ -142,24 +143,15 @@ func (m *UserGroupsMapper) removeUserFromGroup(idpName, username, group string) } // find the user and remove it from the slice - userIdx := -1 - for i, groupUser := range updatedGroup.Users { - if groupUser == username { - userIdx = i - break - } - } - - var newUsers []string - switch userIdx { - case -1: + userIdx := slices.Index(updatedGroup.Users, username) + if userIdx == -1 { return nil - case 0: - newUsers = updatedGroup.Users[1:] - default: - newUsers = append(updatedGroup.Users[0:userIdx], updatedGroup.Users[userIdx+1:]...) } + newUsers := make([]string, 0, len(updatedGroup.Users)-1) + newUsers = append(newUsers, updatedGroup.Users[:userIdx]...) + newUsers = append(newUsers, updatedGroup.Users[userIdx+1:]...) + updatedGroupCopy := updatedGroup.DeepCopy() updatedGroupCopy.Users = newUsers @@ -206,7 +198,7 @@ func (m *UserGroupsMapper) addUserToGroup(idpName, username, group string) error updatedGroupCopy := updatedGroup.DeepCopy() if !onlyAddAnnotation { - updatedGroupCopy.Users = append(updatedGroup.Users, username) + updatedGroupCopy.Users = append(updatedGroupCopy.Users, username) } updatedGroupCopy.Annotations[fmt.Sprintf(groupSyncedKeyFmt, idpName)] = "synced" diff --git a/pkg/groupmapper/groupmapper_test.go b/pkg/groupmapper/groupmapper_test.go index 498b9a6f1..255858c01 100644 --- a/pkg/groupmapper/groupmapper_test.go +++ b/pkg/groupmapper/groupmapper_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "reflect" + "slices" "testing" "time" @@ -441,6 +442,122 @@ func watchForGroupEvents( } } +func TestRemoveUserFromGroup_DoesNotMutateCachedObject(t *testing.T) { + const testGroupName = "test-group" + + tests := []struct { + name string + username string + originalUsers []string + expectedUsers []string // users expected in the group after removal + }{ + { + name: "remove middle user (default case)", + username: "user2", + originalUsers: []string{"user1", "user2", "user3", "user4"}, + expectedUsers: []string{"user1", "user3", "user4"}, + }, + { + name: "remove first user (case 0)", + username: "user1", + originalUsers: []string{"user1", "user2", "user3", "user4"}, + expectedUsers: []string{"user2", "user3", "user4"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + group := createGroupWithUsers(testGroupName, tt.originalUsers...) + fakeUserClient := fakeuserclient.NewSimpleClientset(group) + + // Use a real informer so the lister and GroupCache share the + // same underlying indexed store, just like in production. + informerFactory := userinformer.NewSharedInformerFactory(fakeUserClient, 0) + groupInformer := informerFactory.User().V1().Groups() + require.NoError(t, groupInformer.Informer().AddIndexers(cache.Indexers{ + usercache.ByUserIndexName: usercache.ByUserIndexKeys, + })) + + ctx := t.Context() + informerFactory.Start(ctx.Done()) + cache.WaitForCacheSync(ctx.Done(), groupInformer.Informer().HasSynced) + + indexer := groupInformer.Informer().GetIndexer() + + m := &UserGroupsMapper{ + groupsLister: userlisterv1.NewGroupLister(indexer), + groupsClient: fakeUserClient.UserV1().Groups(), + groupsCache: usercache.NewGroupCache(groupInformer), + } + + err := m.removeUserFromGroup(testIDPName, tt.username, testGroupName) + require.NoError(t, err) + + // Simulate the watch event: the API returns the correctly updated + // group, and the informer calls indexer.Update with it. This is + // where the re-indexing happens against the (possibly corrupted) + // old cached object. + updatedGroup := createGroupWithUsers(testGroupName, tt.expectedUsers...) + require.NoError(t, indexer.Update(updatedGroup)) + + // The removed user should no longer appear in GroupsFor results. + // With the bug, the append mutation causes the re-indexer to miss + // cleaning up the removed user's index entry, creating a phantom. + groups, err := m.groupsCache.GroupsFor(tt.username) + require.NoError(t, err) + require.False(t, slices.ContainsFunc(groups, func(g *userv1.Group) bool { + return g.Name == testGroupName + }), "phantom index entry: GroupsFor(%q) still returns %q after user was removed", tt.username, testGroupName) + }) + } +} + +func TestAddUserToGroup_DoesNotMutateCachedObject(t *testing.T) { + const testGroupName = "test-group" + + // Create a Users slice with extra capacity so append can write into + // the backing array without allocating. + users := make(userv1.OptionalNames, 2, 4) + users[0] = "user1" + users[1] = "user2" + + group := &userv1.Group{ + ObjectMeta: metav1.ObjectMeta{ + Name: testGroupName, + Annotations: map[string]string{ + fmt.Sprintf(groupSyncedKeyFmt, testIDPName): "synced", + groupGeneratedKey: "true", + }, + }, + Users: users, + } + + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + require.NoError(t, indexer.Add(group)) + fakeUserClient := fakeuserclient.NewSimpleClientset(group) + + m := &UserGroupsMapper{ + groupsLister: userlisterv1.NewGroupLister(indexer), + groupsClient: fakeUserClient.UserV1().Groups(), + } + + err := m.addUserToGroup(testIDPName, "user3", testGroupName) + require.NoError(t, err) + + obj, exists, err := indexer.GetByKey(testGroupName) + require.NoError(t, err) + require.True(t, exists) + + cachedGroup := obj.(*userv1.Group) + require.Equal(t, []string{"user1", "user2"}, []string(cachedGroup.Users), + "cached object was mutated: Users changed from [user1 user2] to %v", cachedGroup.Users) + + // Also verify the backing array beyond len was not written to. + // Extend the slice to its capacity and check the spare slots. + fullSlice := cachedGroup.Users[:cap(cachedGroup.Users)] + require.Empty(t, fullSlice[2], + "backing array was mutated beyond len: slot 2 contains %q", fullSlice[2]) +} + var basicGroups = []*userv1.Group{ createGroupWithUsers("group0", "user1", "user2", "user3", "user4", "user5", "user6"), createGroupWithUsers("group1", "user2", "user3", "user4", "user5", "user6"),