From afe6d9fca4904bb57e053e21065e77c75ca4e9e2 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Tue, 24 Feb 2026 19:19:43 +0100 Subject: [PATCH] [management] Prevent deletion of groups linked to flow groups (#5439) --- management/server/group.go | 13 +++++-- management/server/group_test.go | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/management/server/group.go b/management/server/group.go index 9fc8db120..326b167cf 100644 --- a/management/server/group.go +++ b/management/server/group.go @@ -425,6 +425,11 @@ func (am *DefaultAccountManager) DeleteGroups(ctx context.Context, accountID, us var groupIDsToDelete []string var deletedGroups []*types.Group + extraSettings, err := am.settingsManager.GetExtraSettings(ctx, accountID) + if err != nil { + return err + } + err = am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { for _, groupID := range groupIDs { group, err := transaction.GetGroupByID(ctx, store.LockingStrengthNone, accountID, groupID) @@ -433,7 +438,7 @@ func (am *DefaultAccountManager) DeleteGroups(ctx context.Context, accountID, us continue } - if err := validateDeleteGroup(ctx, transaction, group, userID); err != nil { + if err = validateDeleteGroup(ctx, transaction, group, userID, extraSettings.FlowGroups); err != nil { allErrors = errors.Join(allErrors, err) continue } @@ -621,7 +626,7 @@ func validateNewGroup(ctx context.Context, transaction store.Store, accountID st return nil } -func validateDeleteGroup(ctx context.Context, transaction store.Store, group *types.Group, userID string) error { +func validateDeleteGroup(ctx context.Context, transaction store.Store, group *types.Group, userID string, flowGroups []string) error { // disable a deleting integration group if the initiator is not an admin service user if group.Issued == types.GroupIssuedIntegration { executingUser, err := transaction.GetUserByUserID(ctx, store.LockingStrengthNone, userID) @@ -641,6 +646,10 @@ func validateDeleteGroup(ctx context.Context, transaction store.Store, group *ty return &GroupLinkError{"network resource", group.Resources[0].ID} } + if slices.Contains(flowGroups, group.ID) { + return &GroupLinkError{"settings", "traffic event logging"} + } + if isLinked, linkedRoute := isGroupLinkedToRoute(ctx, transaction, group.AccountID, group.ID); isLinked { return &GroupLinkError{"route", string(linkedRoute.NetID)} } diff --git a/management/server/group_test.go b/management/server/group_test.go index dba917dbb..dd6869d50 100644 --- a/management/server/group_test.go +++ b/management/server/group_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/golang/mock/gomock" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -26,6 +27,7 @@ import ( networkTypes "github.com/netbirdio/netbird/management/server/networks/types" peer2 "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/permissions" + "github.com/netbirdio/netbird/management/server/settings" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/route" @@ -284,6 +286,67 @@ func TestDefaultAccountManager_DeleteGroups(t *testing.T) { } } +func TestDefaultAccountManager_DeleteGroupLinkedToFlowGroup(t *testing.T) { + am, _, err := createManager(t) + require.NoError(t, err) + + ctrl := gomock.NewController(t) + settingsMock := settings.NewMockManager(ctrl) + settingsMock.EXPECT(). + GetExtraSettings(gomock.Any(), gomock.Any()). + Return(&types.ExtraSettings{FlowGroups: []string{"grp-for-flow"}}, nil). + AnyTimes() + settingsMock.EXPECT(). + UpdateExtraSettings(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(false, nil). + AnyTimes() + am.settingsManager = settingsMock + + _, account, err := initTestGroupAccount(am) + require.NoError(t, err) + + grp := &types.Group{ + ID: "grp-for-flow", + AccountID: account.Id, + Name: "Group for flow", + Issued: types.GroupIssuedAPI, + Peers: make([]string, 0), + } + require.NoError(t, am.CreateGroup(context.Background(), account.Id, groupAdminUserID, grp)) + + err = am.DeleteGroup(context.Background(), account.Id, groupAdminUserID, "grp-for-flow") + require.Error(t, err) + + var gErr *GroupLinkError + require.ErrorAs(t, err, &gErr) + assert.Equal(t, "settings", gErr.Resource) + assert.Equal(t, "traffic event logging", gErr.Name) + + group, err := am.GetGroup(context.Background(), account.Id, "grp-for-flow", groupAdminUserID) + require.NoError(t, err) + assert.NotNil(t, group) + + regularGrp := &types.Group{ + ID: "grp-regular", + AccountID: account.Id, + Name: "Regular group", + Issued: types.GroupIssuedAPI, + Peers: make([]string, 0), + } + err = am.CreateGroup(context.Background(), account.Id, groupAdminUserID, regularGrp) + require.NoError(t, err) + + err = am.DeleteGroups(context.Background(), account.Id, groupAdminUserID, []string{"grp-for-flow", "grp-regular"}) + require.Error(t, err) + + group, err = am.GetGroup(context.Background(), account.Id, "grp-for-flow", groupAdminUserID) + require.NoError(t, err) + assert.NotNil(t, group) + + _, err = am.GetGroup(context.Background(), account.Id, "grp-regular", groupAdminUserID) + assert.Error(t, err) +} + func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *types.Account, error) { accountID := "testingAcc" domain := "example.com"