Check links of groups before delete it (#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,15 +215,80 @@ 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()
@@ -219,6 +296,8 @@ func (am *DefaultAccountManager) DeleteGroup(accountID, groupID string) error {
 		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
 	}

management/server/http/groups_handler_test.go

@@ -11,17 +11,15 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/netbirdio/netbird/management/server/http/api"
-	"github.com/netbirdio/netbird/management/server/status"
-
 	"github.com/gorilla/mux"
-
-	"github.com/netbirdio/netbird/management/server/jwtclaims"
-
 	"github.com/magiconair/properties/assert"
 
 	"github.com/netbirdio/netbird/management/server"
+	"github.com/netbirdio/netbird/management/server/http/api"
+	"github.com/netbirdio/netbird/management/server/http/util"
+	"github.com/netbirdio/netbird/management/server/jwtclaims"
 	"github.com/netbirdio/netbird/management/server/mock_server"
+	"github.com/netbirdio/netbird/management/server/status"
 )
 
 var TestPeers = map[string]*server.Peer{
@@ -94,6 +92,18 @@ func initGroupTestData(user *server.User, groups ...*server.Group) *GroupsHandle
 					},
 				}, user, nil
 			},
+			DeleteGroupFunc: func(accountID, userId, groupID string) error {
+				if groupID == "linked-grp" {
+					return &server.GroupLinkError{
+						Resource: "something",
+						Name:     "linked-grp",
+					}
+				}
+				if groupID == "invalid-grp" {
+					return fmt.Errorf("internal error")
+				}
+				return nil
+			},
 		},
 		claimsExtractor: jwtclaims.NewClaimsExtractor(
 			jwtclaims.WithFromRequestContext(func(r *http.Request) jwtclaims.AuthorizationClaims {
@@ -297,3 +307,79 @@ func TestWriteGroup(t *testing.T) {
 		})
 	}
 }
+
+func TestDeleteGroup(t *testing.T) {
+	tt := []struct {
+		name           string
+		expectedStatus int
+		expectedBody   bool
+		requestType    string
+		requestPath    string
+	}{
+		{
+			name:           "Try to delete linked group",
+			requestType:    http.MethodDelete,
+			requestPath:    "/api/groups/linked-grp",
+			expectedStatus: http.StatusBadRequest,
+			expectedBody:   true,
+		},
+		{
+			name:           "Try to cause internal error",
+			requestType:    http.MethodDelete,
+			requestPath:    "/api/groups/invalid-grp",
+			expectedStatus: http.StatusInternalServerError,
+			expectedBody:   true,
+		},
+		{
+			name:           "Try to cause internal error",
+			requestType:    http.MethodDelete,
+			requestPath:    "/api/groups/invalid-grp",
+			expectedStatus: http.StatusInternalServerError,
+			expectedBody:   true,
+		},
+		{
+			name:           "Delete group",
+			requestType:    http.MethodDelete,
+			requestPath:    "/api/groups/any-grp",
+			expectedStatus: http.StatusOK,
+			expectedBody:   false,
+		},
+	}
+
+	adminUser := server.NewAdminUser("test_user")
+	p := initGroupTestData(adminUser)
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			recorder := httptest.NewRecorder()
+			req := httptest.NewRequest(tc.requestType, tc.requestPath, nil)
+
+			router := mux.NewRouter()
+			router.HandleFunc("/api/groups/{groupId}", p.DeleteGroup).Methods("DELETE")
+			router.ServeHTTP(recorder, req)
+
+			res := recorder.Result()
+			defer res.Body.Close()
+
+			content, err := io.ReadAll(res.Body)
+			if err != nil {
+				t.Fatalf("I don't know what I expected; %v", err)
+			}
+
+			if status := recorder.Code; status != tc.expectedStatus {
+				t.Errorf("handler returned wrong status code: got %v want %v, content: %s",
+					status, tc.expectedStatus, string(content))
+				return
+			}
+
+			if tc.expectedBody {
+				got := &util.ErrorResponse{}
+
+				if err = json.Unmarshal(content, &got); err != nil {
+					t.Fatalf("Sent content is not in correct json format; %v", err)
+				}
+				assert.Equal(t, got.Code, tc.expectedStatus)
+			}
+		})
+	}
+}

management/server/http/util/util.go

@@ -13,6 +13,11 @@ import (
 	"github.com/netbirdio/netbird/management/server/status"
 )
 
+type ErrorResponse struct {
+	Message string `json:"message"`
+	Code    int    `json:"code"`
+}
+
 // WriteJSONObject simply writes object to the HTTP reponse in JSON format
 func WriteJSONObject(w http.ResponseWriter, obj interface{}) {
 	w.WriteHeader(http.StatusOK)
@@ -58,14 +63,9 @@ func (d *Duration) UnmarshalJSON(b []byte) error {
 
 // WriteErrorResponse prepares and writes an error response i nJSON
 func WriteErrorResponse(errMsg string, httpStatus int, w http.ResponseWriter) {
-	type errorResponse struct {
-		Message string `json:"message"`
-		Code    int    `json:"code"`
-	}
-
 	w.WriteHeader(httpStatus)
 	w.Header().Set("Content-Type", "application/json; charset=UTF-8")
-	err := json.NewEncoder(w).Encode(&errorResponse{
+	err := json.NewEncoder(w).Encode(&ErrorResponse{
 		Message: errMsg,
 		Code:    httpStatus,
 	})

management/server/mock_server/account_mock.go

@@ -32,7 +32,7 @@ type MockAccountManager struct {
 	GetGroupFunc                    func(accountID, groupID string) (*server.Group, error)
 	SaveGroupFunc                   func(accountID, userID string, group *server.Group) error
 	UpdateGroupFunc                 func(accountID string, groupID string, operations []server.GroupUpdateOperation) (*server.Group, error)
-	DeleteGroupFunc                 func(accountID, groupID string) error
+	DeleteGroupFunc                 func(accountID, userId, groupID string) error
 	ListGroupsFunc                  func(accountID string) ([]*server.Group, error)
 	GroupAddPeerFunc                func(accountID, groupID, peerKey string) error
 	GroupDeletePeerFunc             func(accountID, groupID, peerKey string) error
@@ -275,9 +275,9 @@ func (am *MockAccountManager) UpdateGroup(accountID string, groupID string, oper
 }
 
 // DeleteGroup mock implementation of DeleteGroup from server.AccountManager interface
-func (am *MockAccountManager) DeleteGroup(accountID, groupID string) error {
+func (am *MockAccountManager) DeleteGroup(accountId, userId, groupID string) error {
 	if am.DeleteGroupFunc != nil {
-		return am.DeleteGroupFunc(accountID, groupID)
+		return am.DeleteGroupFunc(accountId, userId, groupID)
 	}
 	return status.Errorf(codes.Unimplemented, "method DeleteGroup is not implemented")
 }