diff --git a/management/server/account.go b/management/server/account.go index 417c32c09..68302d2ba 100644 --- a/management/server/account.go +++ b/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 diff --git a/management/server/account_test.go b/management/server/account_test.go index f90abc41b..1609abafc 100644 --- a/management/server/account_test.go +++ b/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 } diff --git a/management/server/activity/codes.go b/management/server/activity/codes.go index 03ec709e5..e571c3a0c 100644 --- a/management/server/activity/codes.go +++ b/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: diff --git a/management/server/group.go b/management/server/group.go index dd1229c86..53571e099 100644 --- a/management/server/group.go +++ b/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) } diff --git a/management/server/group_test.go b/management/server/group_test.go new file mode 100644 index 000000000..3e2d6d3cc --- /dev/null +++ b/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) +} diff --git a/management/server/http/groups_handler.go b/management/server/http/groups_handler.go index 966c3f678..30c78e21b 100644 --- a/management/server/http/groups_handler.go +++ b/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 } diff --git a/management/server/http/groups_handler_test.go b/management/server/http/groups_handler_test.go index 73ca8db6a..44603059a 100644 --- a/management/server/http/groups_handler_test.go +++ b/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) + } + }) + } +} diff --git a/management/server/http/util/util.go b/management/server/http/util/util.go index 407443251..44f4919f5 100644 --- a/management/server/http/util/util.go +++ b/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, }) diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 9482c5ec6..eb31d2a79 100644 --- a/management/server/mock_server/account_mock.go +++ b/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") }