From 539480a713c621917fd1de348ada8cc0ffe41e8f Mon Sep 17 00:00:00 2001
From: Bethuel Mmbaga <bethuelmbaga12@gmail.com>
Date: Mon, 12 Aug 2024 13:48:05 +0300
Subject: [PATCH] [management] Prevent removal of All group from peers during
 user groups propagation  (#2410)

* Prevent removal of "All" group from peers

* Prevent adding "All" group to users and setup keys

* Refactor setup key group validation
---
 management/server/account.go       |  2 +-
 management/server/setupkey.go      | 23 +++++++++++++++---
 management/server/setupkey_test.go | 39 ++++++++++++++++++++++++++----
 management/server/user.go          |  6 ++++-
 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/management/server/account.go b/management/server/account.go
index ca53ebad0..972272746 100644
--- a/management/server/account.go
+++ b/management/server/account.go
@@ -922,7 +922,7 @@ func (a *Account) UserGroupsAddToPeers(userID string, groups ...string) {
 func (a *Account) UserGroupsRemoveFromPeers(userID string, groups ...string) {
 	for _, gid := range groups {
 		group, ok := a.Groups[gid]
-		if !ok {
+		if !ok || group.Name == "All" {
 			continue
 		}
 		update := make([]string, 0, len(group.Peers))
diff --git a/management/server/setupkey.go b/management/server/setupkey.go
index 8ef91755c..859f1b0b9 100644
--- a/management/server/setupkey.go
+++ b/management/server/setupkey.go
@@ -223,10 +223,8 @@ func (am *DefaultAccountManager) CreateSetupKey(ctx context.Context, accountID s
 		return nil, err
 	}
 
-	for _, group := range autoGroups {
-		if _, ok := account.Groups[group]; !ok {
-			return nil, status.Errorf(status.NotFound, "group %s doesn't exist", group)
-		}
+	if err := validateSetupKeyAutoGroups(account, autoGroups); err != nil {
+		return nil, err
 	}
 
 	setupKey := GenerateSetupKey(keyName, keyType, keyDuration, autoGroups, usageLimit, ephemeral)
@@ -279,6 +277,10 @@ func (am *DefaultAccountManager) SaveSetupKey(ctx context.Context, accountID str
 		return nil, status.Errorf(status.NotFound, "setup key not found")
 	}
 
+	if err := validateSetupKeyAutoGroups(account, keyToSave.AutoGroups); err != nil {
+		return nil, err
+	}
+
 	// only auto groups, revoked status, and name can be updated for now
 	newKey := oldKey.Copy()
 	newKey.Name = keyToSave.Name
@@ -399,3 +401,16 @@ func (am *DefaultAccountManager) GetSetupKey(ctx context.Context, accountID, use
 
 	return foundKey, nil
 }
+
+func validateSetupKeyAutoGroups(account *Account, autoGroups []string) error {
+	for _, group := range autoGroups {
+		g, ok := account.Groups[group]
+		if !ok {
+			return status.Errorf(status.NotFound, "group %s doesn't exist", group)
+		}
+		if g.Name == "All" {
+			return status.Errorf(status.InvalidArgument, "can't add All group to the setup key")
+		}
+	}
+	return nil
+}
diff --git a/management/server/setupkey_test.go b/management/server/setupkey_test.go
index 034f4e2d6..aa5075b02 100644
--- a/management/server/setupkey_test.go
+++ b/management/server/setupkey_test.go
@@ -26,10 +26,17 @@ func TestDefaultAccountManager_SaveSetupKey(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	err = manager.SaveGroup(context.Background(), account.Id, userID, &nbgroup.Group{
-		ID:    "group_1",
-		Name:  "group_name_1",
-		Peers: []string{},
+	err = manager.SaveGroups(context.Background(), account.Id, userID, []*nbgroup.Group{
+		{
+			ID:    "group_1",
+			Name:  "group_name_1",
+			Peers: []string{},
+		},
+		{
+			ID:    "group_2",
+			Name:  "group_name_2",
+			Peers: []string{},
+		},
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -70,6 +77,19 @@ func TestDefaultAccountManager_SaveSetupKey(t *testing.T) {
 	assert.NotEmpty(t, ev.Meta["key"])
 	assert.Equal(t, userID, ev.InitiatorID)
 	assert.Equal(t, key.Id, ev.TargetID)
+
+	groupAll, err := account.GetGroupAll()
+	assert.NoError(t, err)
+
+	// saving setup key with All group assigned to auto groups should return error
+	autoGroups = append(autoGroups, groupAll.ID)
+	_, err = manager.SaveSetupKey(context.Background(), account.Id, &SetupKey{
+		Id:         key.Id,
+		Name:       newKeyName,
+		Revoked:    revoked,
+		AutoGroups: autoGroups,
+	}, userID)
+	assert.Error(t, err, "should not save setup key with All group assigned in auto groups")
 }
 
 func TestDefaultAccountManager_CreateSetupKey(t *testing.T) {
@@ -102,6 +122,9 @@ func TestDefaultAccountManager_CreateSetupKey(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	groupAll, err := account.GetGroupAll()
+	assert.NoError(t, err)
+
 	type testCase struct {
 		name string
 
@@ -134,8 +157,14 @@ func TestDefaultAccountManager_CreateSetupKey(t *testing.T) {
 		expectedGroups:  []string{"FAKE"},
 		expectedFailure: true,
 	}
+	testCase3 := testCase{
+		name:            "Create Setup Key should fail because of All group",
+		expectedKeyName: "my-test-key",
+		expectedGroups:  []string{groupAll.ID},
+		expectedFailure: true,
+	}
 
-	for _, tCase := range []testCase{testCase1, testCase2} {
+	for _, tCase := range []testCase{testCase1, testCase2, testCase3} {
 		t.Run(tCase.name, func(t *testing.T) {
 			key, err := manager.CreateSetupKey(context.Background(), account.Id, tCase.expectedKeyName, SetupKeyReusable, expiresIn,
 				tCase.expectedGroups, SetupKeyUnlimitedUsage, userID, false)
diff --git a/management/server/user.go b/management/server/user.go
index 8fd0a1627..727bc5c6b 100644
--- a/management/server/user.go
+++ b/management/server/user.go
@@ -944,10 +944,14 @@ func validateUserUpdate(account *Account, initiatorUser, oldUser, update *User)
 	}
 
 	for _, newGroupID := range update.AutoGroups {
-		if _, ok := account.Groups[newGroupID]; !ok {
+		group, ok := account.Groups[newGroupID]
+		if !ok {
 			return status.Errorf(status.InvalidArgument, "provided group ID %s in the user %s update doesn't exist",
 				newGroupID, update.Id)
 		}
+		if group.Name == "All" {
+			return status.Errorf(status.InvalidArgument, "can't add All group to the user")
+		}
 	}
 
 	return nil