From 8c965434ae67329885cc5bfc66ad580e42fbb984 Mon Sep 17 00:00:00 2001
From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com>
Date: Wed, 22 Jan 2025 19:33:20 +0100
Subject: [PATCH] [management] remove peer from group on delete (#3223)

---
 .../peers_handler_benchmark_test.go           | 16 +++---
 management/server/peer.go                     | 13 +++++
 management/server/peer_test.go                | 49 +++++++++++++++++++
 3 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go b/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go
index 2eb50e4b4..23b4edefb 100644
--- a/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go
+++ b/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go
@@ -145,14 +145,14 @@ func BenchmarkGetAllPeers(b *testing.B) {
 
 func BenchmarkDeletePeer(b *testing.B) {
 	var expectedMetrics = map[string]testing_tools.PerformanceMetrics{
-		"Peers - XS":     {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
-		"Peers - S":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
-		"Peers - M":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
-		"Peers - L":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
-		"Groups - L":     {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
-		"Users - L":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
-		"Setup Keys - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
-		"Peers - XL":     {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15},
+		"Peers - XS":     {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
+		"Peers - S":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
+		"Peers - M":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
+		"Peers - L":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
+		"Groups - L":     {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
+		"Users - L":      {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
+		"Setup Keys - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
+		"Peers - XL":     {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16},
 	}
 
 	log.SetOutput(io.Discard)
diff --git a/management/server/peer.go b/management/server/peer.go
index 5b0f12899..e5442acea 100644
--- a/management/server/peer.go
+++ b/management/server/peer.go
@@ -380,6 +380,19 @@ func (am *DefaultAccountManager) DeletePeer(ctx context.Context, accountID, peer
 			return err
 		}
 
+		groups, err := transaction.GetPeerGroups(ctx, store.LockingStrengthUpdate, accountID, peerID)
+		if err != nil {
+			return fmt.Errorf("failed to get peer groups: %w", err)
+		}
+
+		for _, group := range groups {
+			group.RemovePeer(peerID)
+			err = transaction.SaveGroup(ctx, store.LockingStrengthUpdate, group)
+			if err != nil {
+				return fmt.Errorf("failed to save group: %w", err)
+			}
+		}
+
 		eventsToStore, err = deletePeers(ctx, am, transaction, accountID, userID, []*nbpeer.Peer{peer})
 		return err
 	})
diff --git a/management/server/peer_test.go b/management/server/peer_test.go
index bf712f38a..40f8d15d5 100644
--- a/management/server/peer_test.go
+++ b/management/server/peer_test.go
@@ -1728,3 +1728,52 @@ func TestPeerAccountPeersUpdate(t *testing.T) {
 		}
 	})
 }
+
+func Test_DeletePeer(t *testing.T) {
+	manager, err := createManager(t)
+	if err != nil {
+		t.Fatal(err)
+		return
+	}
+
+	// account with an admin and a regular user
+	accountID := "test_account"
+	adminUser := "account_creator"
+	account := newAccountWithId(context.Background(), accountID, adminUser, "")
+	account.Peers = map[string]*nbpeer.Peer{
+		"peer1": {
+			ID:        "peer1",
+			AccountID: accountID,
+		},
+		"peer2": {
+			ID:        "peer2",
+			AccountID: accountID,
+		},
+	}
+	account.Groups = map[string]*types.Group{
+		"group1": {
+			ID:    "group1",
+			Name:  "Group1",
+			Peers: []string{"peer1", "peer2"},
+		},
+	}
+
+	err = manager.Store.SaveAccount(context.Background(), account)
+	if err != nil {
+		t.Fatal(err)
+		return
+	}
+
+	err = manager.DeletePeer(context.Background(), accountID, "peer1", adminUser)
+	if err != nil {
+		t.Fatalf("DeletePeer failed: %v", err)
+	}
+
+	_, err = manager.GetPeer(context.Background(), accountID, "peer1", adminUser)
+	assert.Error(t, err)
+
+	group, err := manager.GetGroup(context.Background(), accountID, "group1", adminUser)
+	assert.NoError(t, err)
+	assert.NotContains(t, group.Peers, "peer1")
+
+}