Check links of groups before delete it (netbirdio#1010)

* Check links of groups before delete it

* Add delete group handler test

* Rename dns error msg

* Add delete group test

* Remove rule check

The policy cover this scenario

* Fix test

* Check disabled management grps

* Change error message

* Add new activity for group delete event

management/server/account.go

@@ -81,7 +81,7 @@ type AccountManager interface {
 	GetGroup(accountId, groupID string) (*Group, error)
 	SaveGroup(accountID, userID string, group *Group) error
 	UpdateGroup(accountID string, groupID string, operations []GroupUpdateOperation) (*Group, error)
-	DeleteGroup(accountId, groupID string) error
+	DeleteGroup(accountId, userId, groupID string) error
 	ListGroups(accountId string) ([]*Group, error)
 	GroupAddPeer(accountId, groupID, peerID string) error
 	GroupDeletePeer(accountId, groupID, peerKey string) error

management/server/account_test.go

@@ -1083,7 +1083,10 @@ func TestAccountManager_NetworkUpdates(t *testing.T) {
 			}
 		}()
 
-		if err := manager.DeleteGroup(account.Id, group.ID); err != nil {
+		// clean policy is pre requirement for delete group
+		_ = manager.DeletePolicy(account.Id, policy.ID, userID)
+
+		if err := manager.DeleteGroup(account.Id, "", group.ID); err != nil {
 			t.Errorf("delete group: %v", err)
 			return
 		}

management/server/activity/codes.go

@@ -95,6 +95,8 @@ const (
 	UserBlocked
 	// UserUnblocked indicates that a user unblocked another user
 	UserUnblocked
+	// GroupDeleted indicates that a user deleted group
+	GroupDeleted
 )
 
 const (
@@ -192,6 +194,8 @@ const (
 	UserBlockedMessage string = "User blocked"
 	// UserUnblockedMessage is a human-readable text message of the UserUnblocked activity
 	UserUnblockedMessage string = "User unblocked"
+	// GroupDeletedMessage is a human-readable text message of the GroupDeleted activity
+	GroupDeletedMessage string = "Group deleted"
 )
 
 // Activity that triggered an Event
@@ -294,6 +298,8 @@ func (a Activity) Message() string {
 		return UserBlockedMessage
 	case UserUnblocked:
 		return UserUnblockedMessage
+	case GroupDeleted:
+		return GroupDeletedMessage
 	default:
 		return "UNKNOWN_ACTIVITY"
 	}
@@ -342,6 +348,8 @@ func (a Activity) StringCode() string {
 		return "group.add"
 	case GroupUpdated:
 		return "group.update"
+	case GroupDeleted:
+		return "group.delete"
 	case GroupRemovedFromPeer:
 		return "peer.group.delete"
 	case GroupAddedToPeer:

management/server/group.go

@@ -1,11 +1,23 @@
 package server
 
 import (
+	"fmt"
+
+	log "github.com/sirupsen/logrus"
+
 	"github.com/netbirdio/netbird/management/server/activity"
 	"github.com/netbirdio/netbird/management/server/status"
-	log "github.com/sirupsen/logrus"
 )
 
+type GroupLinkError struct {
+	Resource string
+	Name     string
+}
+
+func (e *GroupLinkError) Error() string {
+	return fmt.Sprintf("group has been linked to %s: %s", e.Resource, e.Name)
+}
+
 // Group of the peers for ACL
 type Group struct {
 	// ID of the group
@@ -203,22 +215,89 @@ func (am *DefaultAccountManager) UpdateGroup(accountID string,
 }
 
 // DeleteGroup object of the peers
-func (am *DefaultAccountManager) DeleteGroup(accountID, groupID string) error {
-	unlock := am.Store.AcquireAccountLock(accountID)
+func (am *DefaultAccountManager) DeleteGroup(accountId, userId, groupID string) error {
+	unlock := am.Store.AcquireAccountLock(accountId)
 	defer unlock()
 
-	account, err := am.Store.GetAccount(accountID)
+	account, err := am.Store.GetAccount(accountId)
 	if err != nil {
 		return err
 	}
 
+	g, ok := account.Groups[groupID]
+	if !ok {
+		return nil
+	}
+
+	// check route links
+	for _, r := range account.Routes {
+		for _, g := range r.Groups {
+			if g == groupID {
+				return &GroupLinkError{"route", r.NetID}
+			}
+		}
+	}
+
+	// check DNS links
+	for _, dns := range account.NameServerGroups {
+		for _, g := range dns.Groups {
+			if g == groupID {
+				return &GroupLinkError{"name server groups", dns.Name}
+			}
+		}
+	}
+
+	// check ACL links
+	for _, policy := range account.Policies {
+		for _, rule := range policy.Rules {
+			for _, src := range rule.Sources {
+				if src == groupID {
+					return &GroupLinkError{"policy", policy.Name}
+				}
+			}
+
+			for _, dst := range rule.Destinations {
+				if dst == groupID {
+					return &GroupLinkError{"policy", policy.Name}
+				}
+			}
+		}
+	}
+
+	// check setup key links
+	for _, setupKey := range account.SetupKeys {
+		for _, grp := range setupKey.AutoGroups {
+			if grp == groupID {
+				return &GroupLinkError{"setup key", setupKey.Name}
+			}
+		}
+	}
+
+	// check user links
+	for _, user := range account.Users {
+		for _, grp := range user.AutoGroups {
+			if grp == groupID {
+				return &GroupLinkError{"user", user.Id}
+			}
+		}
+	}
+
+	// check DisabledManagementGroups
+	for _, disabledMgmGrp := range account.DNSSettings.DisabledManagementGroups {
+		if disabledMgmGrp == groupID {
+			return &GroupLinkError{"disabled DNS management groups", g.Name}
+		}
+	}
+
 	delete(account.Groups, groupID)
 
 	account.Network.IncSerial()
 	if err = am.Store.SaveAccount(account); err != nil {
 		return err
 	}
 
+	am.storeEvent(userId, groupID, accountId, activity.GroupDeleted, g.EventMeta())
+
 	return am.updateAccountPeers(account)
 }
 

management/server/group_test.go

@@ -0,0 +1,164 @@
+package server
+
+import (
+	"testing"
+
+	nbdns "github.com/netbirdio/netbird/dns"
+	"github.com/netbirdio/netbird/route"
+)
+
+const (
+	groupAdminUserID = "testingAdminUser"
+)
+
+func TestDefaultAccountManager_DeleteGroup(t *testing.T) {
+	am, err := createManager(t)
+	if err != nil {
+		t.Error("failed to create account manager")
+	}
+
+	account, err := initTestGroupAccount(am)
+	if err != nil {
+		t.Error("failed to init testing account")
+	}
+
+	testCases := []struct {
+		name           string
+		groupID        string
+		expectedReason string
+	}{
+		{
+			"route",
+			"grp-for-route",
+			"route",
+		},
+		{
+			"name server groups",
+			"grp-for-name-server-grp",
+			"name server groups",
+		},
+		{
+			"policy",
+			"grp-for-policies",
+			"policy",
+		},
+		{
+			"setup keys",
+			"grp-for-keys",
+			"setup key",
+		},
+		{
+			"users",
+			"grp-for-users",
+			"user",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			err = am.DeleteGroup(account.Id, "", testCase.groupID)
+			if err == nil {
+				t.Errorf("delete %s group successfully", testCase.groupID)
+				return
+			}
+
+			gErr, ok := err.(*GroupLinkError)
+			if !ok {
+				t.Error("invalid error type")
+				return
+			}
+			if gErr.Resource != testCase.expectedReason {
+				t.Errorf("invalid error case: %s, expected: %s", gErr.Resource, testCase.expectedReason)
+			}
+		})
+	}
+}
+
+func initTestGroupAccount(am *DefaultAccountManager) (*Account, error) {
+	accountID := "testingAcc"
+	domain := "example.com"
+
+	groupForRoute := &Group{
+		"grp-for-route",
+		"Group for route",
+		GroupIssuedAPI,
+		make([]string, 0),
+	}
+
+	groupForNameServerGroups := &Group{
+		"grp-for-name-server-grp",
+		"Group for name server groups",
+		GroupIssuedAPI,
+		make([]string, 0),
+	}
+
+	groupForPolicies := &Group{
+		"grp-for-policies",
+		"Group for policies",
+		GroupIssuedAPI,
+		make([]string, 0),
+	}
+
+	groupForSetupKeys := &Group{
+		"grp-for-keys",
+		"Group for setup keys",
+		GroupIssuedAPI,
+		make([]string, 0),
+	}
+
+	groupForUsers := &Group{
+		"grp-for-users",
+		"Group for users",
+		GroupIssuedAPI,
+		make([]string, 0),
+	}
+
+	routeResource := &route.Route{
+		ID:     "example route",
+		Groups: []string{groupForRoute.ID},
+	}
+
+	nameServerGroup := &nbdns.NameServerGroup{
+		ID:     "example name server group",
+		Groups: []string{groupForNameServerGroups.ID},
+	}
+
+	policy := &Policy{
+		ID: "example policy",
+		Rules: []*PolicyRule{
+			{
+				ID:           "example policy rule",
+				Destinations: []string{groupForPolicies.ID},
+			},
+		},
+	}
+
+	setupKey := &SetupKey{
+		Id:         "example setup key",
+		AutoGroups: []string{groupForSetupKeys.ID},
+	}
+
+	user := &User{
+		Id:         "example user",
+		AutoGroups: []string{groupForUsers.ID},
+	}
+	account := newAccountWithId(accountID, groupAdminUserID, domain)
+	account.Routes[routeResource.ID] = routeResource
+	account.NameServerGroups[nameServerGroup.ID] = nameServerGroup
+	account.Policies = append(account.Policies, policy)
+	account.SetupKeys[setupKey.Id] = setupKey
+	account.Users[user.Id] = user
+
+	err := am.Store.SaveAccount(account)
+	if err != nil {
+		return nil, err
+	}
+
+	_ = am.SaveGroup(accountID, groupAdminUserID, groupForRoute)
+	_ = am.SaveGroup(accountID, groupAdminUserID, groupForNameServerGroups)
+	_ = am.SaveGroup(accountID, groupAdminUserID, groupForPolicies)
+	_ = am.SaveGroup(accountID, groupAdminUserID, groupForSetupKeys)
+	_ = am.SaveGroup(accountID, groupAdminUserID, groupForUsers)
+
+	return am.Store.GetAccount(account.Id)
+}

management/server/http/groups_handler.go

@@ -168,7 +168,7 @@ func (h *GroupsHandler) CreateGroup(w http.ResponseWriter, r *http.Request) {
 // DeleteGroup handles group deletion request
 func (h *GroupsHandler) DeleteGroup(w http.ResponseWriter, r *http.Request) {
 	claims := h.claimsExtractor.FromRequestContext(r)
-	account, _, err := h.accountManager.GetAccountFromToken(claims)
+	account, user, err := h.accountManager.GetAccountFromToken(claims)
 	if err != nil {
 		util.WriteError(err, w)
 		return
@@ -192,8 +192,13 @@ func (h *GroupsHandler) DeleteGroup(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	err = h.accountManager.DeleteGroup(aID, groupID)
+	err = h.accountManager.DeleteGroup(aID, user.Id, groupID)
 	if err != nil {
+		_, ok := err.(*server.GroupLinkError)
+		if ok {
+			util.WriteErrorResponse(err.Error(), http.StatusBadRequest, w)
+			return
+		}
 		util.WriteError(err, w)
 		return
 	}