stackrox · dhaus67 · Nov 11, 2022 · Nov 9, 2022 · Nov 11, 2022
diff --git a/central/group/datastore/datastore.go b/central/group/datastore/datastore.go
@@ -21,6 +21,7 @@ type DataStore interface {
 	Mutate(ctx context.Context, remove, update, add []*storage.Group, force bool) error
 	Remove(ctx context.Context, props *storage.GroupProperties, force bool) error
 	RemoveAllWithAuthProviderID(ctx context.Context, authProviderID string, force bool) error
+	RemoveAllWithEmptyProperties(ctx context.Context) error
 }
 
 // New returns a new DataStore instance.

diff --git a/central/group/datastore/datastore_impl.go b/central/group/datastore/datastore_impl.go
@@ -3,6 +3,8 @@ package datastore
 import (
 	"context"
 
+	"github.com/gogo/protobuf/proto"
+	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
 	"github.com/stackrox/rox/central/group/datastore/internal/store"
 	"github.com/stackrox/rox/central/role/resources"
@@ -218,6 +220,36 @@ func (ds *dataStoreImpl) RemoveAllWithAuthProviderID(ctx context.Context, authPr
 	return ds.Mutate(ctx, groups, nil, nil, force)
 }
 
+func (ds *dataStoreImpl) RemoveAllWithEmptyProperties(ctx context.Context) error {
+	// Search through all groups and verify whether any group exists with empty properties and attempt to delete them.
+	isEmptyGroupPropertiesF := func(props *storage.GroupProperties) bool {
+		if props.GetAuthProviderId() == "" && props.GetKey() == "" && props.GetValue() == "" {
+			return true
+		}
+		return false
+	}
+	groups, err := ds.GetFiltered(ctx, isEmptyGroupPropertiesF)
+	if err != nil {
+		return err
+	}
+
+	var removeGroupErrs *multierror.Error
+	for _, group := range groups {
+		// Since we are dealing with empty properties, we only require the ID to be set.
+		// In case the ID is not set, add the error to the error list.
+		id := group.GetProps().GetId()
+		if id == "" {
+			removeGroupErrs = multierror.Append(removeGroupErrs, errox.InvalidArgs.Newf("group %s has no ID"+
+				" set and cannot be deleted", proto.MarshalTextString(group)))
+			continue
+		}
+		if err := ds.storage.Delete(ctx, id); err != nil {
+			removeGroupErrs = multierror.Append(removeGroupErrs, err)
+		}
+	}
+	return removeGroupErrs.ErrorOrNil()
+}
+
 // Helpers
 //////////
 
@@ -370,13 +402,10 @@ func (ds *dataStoreImpl) getDefaultGroupForProps(ctx context.Context, props *sto
 // validateMutableGroupIDNoLock validates whether a group allows changes or not based on the mutability mode set.
 // NOTE: This function assumes that the call to this function is already behind a lock.
 func (ds *dataStoreImpl) validateMutableGroupIDNoLock(ctx context.Context, id string, force bool) error {
-	group, exists, err := ds.storage.Get(ctx, id)
+	group, err := ds.validateGroupExists(ctx, id)
 	if err != nil {
 		return err
 	}
-	if !exists {
-		return errox.NotFound.Newf("group with id %q was not found", id)
-	}
 
 	switch group.GetProps().GetTraits().GetMutabilityMode() {
 	case storage.Traits_ALLOW_MUTATE:
@@ -393,3 +422,14 @@ func (ds *dataStoreImpl) validateMutableGroupIDNoLock(ctx context.Context, id st
 	}
 	return errox.InvalidArgs.Newf("group %q is immutable", id)
 }
+
+func (ds *dataStoreImpl) validateGroupExists(ctx context.Context, id string) (*storage.Group, error) {
+	group, exists, err := ds.storage.Get(ctx, id)
+	if err != nil {
+		return nil, err
+	}
+	if !exists {
+		return nil, errox.NotFound.Newf("group with id %q was not found", id)
+	}
+	return group, nil
+}
diff --git a/central/group/datastore/datastore_test.go b/central/group/datastore/datastore_test.go
@@ -738,3 +738,59 @@ func (s *groupDataStoreTestSuite) TestMutateGroupForce() {
 	err = s.dataStore.Mutate(s.hasWriteCtx, []*storage.Group{mutableGroup}, []*storage.Group{immutableGroup}, nil, true)
 	s.NoError(err)
 }
+
+func (s *groupDataStoreTestSuite) TestRemoveAllWithEmptyProperties() {
+	// 1. Try and remove groups without properties without running into any issues.
+	groupsWithoutProperties := []*storage.Group{
+		{
+			Props: &storage.GroupProperties{
+				Id: "id1",
+			},
+			RoleName: "i don't",
+		},
+
+		{
+			Props: &storage.GroupProperties{
+				Id: "id2",
+			},
+			RoleName: "know anything",
+		},
+	}
+	gomock.InOrder(
+		s.storage.EXPECT().Walk(gomock.Any(), gomock.Any()).DoAndReturn(walkMockFunc(groupsWithoutProperties)),
+		s.storage.EXPECT().Delete(gomock.Any(), groupsWithoutProperties[0].GetProps().GetId()).Return(nil),
+		s.storage.EXPECT().Delete(gomock.Any(), groupsWithoutProperties[1].GetProps().GetId()).Return(nil),
+	)
+
+	err := s.dataStore.RemoveAllWithEmptyProperties(s.hasWriteCtx)
+	s.NoError(err)
+
+	// 2. Try and remove groups without properties with some groups not having an ID.
+	groupsWithoutProperties = []*storage.Group{
+		{
+			Props: &storage.GroupProperties{
+				Id: "id1",
+			},
+			RoleName: "i don't",
+		},
+		{
+			Props:    &storage.GroupProperties{},
+			RoleName: "this is",
+		},
+		{
+			Props: &storage.GroupProperties{
+				Id: "id2",
+			},
+			RoleName: "know anything",
+		},
+	}
+	gomock.InOrder(
+		s.storage.EXPECT().Walk(gomock.Any(), gomock.Any()).DoAndReturn(walkMockFunc(groupsWithoutProperties)),
+		s.storage.EXPECT().Delete(gomock.Any(), groupsWithoutProperties[0].GetProps().GetId()).Return(nil),
+		s.storage.EXPECT().Delete(gomock.Any(), groupsWithoutProperties[2].GetProps().GetId()).Return(nil),
+	)
+
+	err = s.dataStore.RemoveAllWithEmptyProperties(s.hasWriteCtx)
+	s.Error(err)
+	s.ErrorIs(err, errox.InvalidArgs)
+}
diff --git a/central/group/datastore/mocks/datastore.go b/central/group/datastore/mocks/datastore.go
diff --git a/central/group/datastore/singleton.go b/central/group/datastore/singleton.go
@@ -19,13 +19,6 @@ var (
 	once sync.Once
 )
 
-var isEmptyGroupPropertiesF = func(props *storage.GroupProperties) bool {
-	if props.GetAuthProviderId() == "" && props.GetKey() == "" && props.GetValue() == "" {
-		return true
-	}
-	return false
-}
-
 func initialize() {
 	if env.PostgresDatastoreEnabled.BooleanSetting() {
 		ds = New(postgres.New(globaldb.GetPostgres()))
@@ -39,12 +32,7 @@ func initialize() {
 			sac.AccessModeScopeKeys(storage.Access_READ_ACCESS, storage.Access_READ_WRITE_ACCESS),
 			sac.ResourceScopeKeys(resources.Access)))
 
-	grps, err := ds.GetFiltered(ctx, isEmptyGroupPropertiesF)
-	utils.Should(err)
-	for _, grp := range grps {
-		err = ds.Remove(ctx, grp.GetProps(), true)
-		utils.Should(err)
-	}
+	utils.Should(ds.RemoveAllWithEmptyProperties(ctx))
 }
 
 // Singleton returns the singleton providing access to the roles store.