Fix nil pointer exception in group delete (#1211)

Fix group delete panic In case if in the db the DNSSettings is null then can cause panic in delete group function because this field is pointer and it was not checked. Because of in the future implementation this variable will be filled in any case then make no sense to keep the pointer type. Fix DNSSettings copy function
netbirdio · Oct 11, 2023 · b8599f6 · b8599f6
1 parent 659110f
commit b8599f6
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 40 deletions.
diff --git a/management/server/account.go b/management/server/account.go
@@ -180,7 +180,7 @@ type Account struct {
 	Policies               []*Policy
 	Routes                 map[string]*route.Route
 	NameServerGroups       map[string]*nbdns.NameServerGroup
-	DNSSettings            *DNSSettings
+	DNSSettings            DNSSettings
 	// Settings is a dictionary of Account settings
 	Settings *Settings
 }
@@ -513,13 +513,11 @@ func (a *Account) getUserGroups(userID string) ([]string, error) {
 func (a *Account) getPeerDNSManagementStatus(peerID string) bool {
 	peerGroups := a.getPeerGroups(peerID)
 	enabled := true
-	if a.DNSSettings != nil {
-		for _, groupID := range a.DNSSettings.DisabledManagementGroups {
-			_, found := peerGroups[groupID]
-			if found {
-				enabled = false
-				break
-			}
+	for _, groupID := range a.DNSSettings.DisabledManagementGroups {
+		_, found := peerGroups[groupID]
+		if found {
+			enabled = false
+			break
 		}
 	}
 	return enabled
@@ -606,10 +604,7 @@ func (a *Account) Copy() *Account {
 		nsGroups[id] = nsGroup.Copy()
 	}
 
-	var dnsSettings *DNSSettings
-	if a.DNSSettings != nil {
-		dnsSettings = a.DNSSettings.Copy()
-	}
+	dnsSettings := a.DNSSettings.Copy()
 
 	var settings *Settings
 	if a.Settings != nil {
@@ -1618,7 +1613,7 @@ func newAccountWithId(accountID, userID, domain string) *Account {
 	setupKeys := map[string]*SetupKey{}
 	nameServersGroups := make(map[string]*nbdns.NameServerGroup)
 	users[userID] = NewAdminUser(userID)
-	dnsSettings := &DNSSettings{
+	dnsSettings := DNSSettings{
 		DisabledManagementGroups: make([]string, 0),
 	}
 	log.Debugf("created new account %s", accountID)

diff --git a/management/server/account_test.go b/management/server/account_test.go
@@ -1374,7 +1374,7 @@ func TestAccount_Copy(t *testing.T) {
 				NameServers: []nbdns.NameServer{},
 			},
 		},
-		DNSSettings: &DNSSettings{DisabledManagementGroups: []string{}},
+		DNSSettings: DNSSettings{DisabledManagementGroups: []string{}},
 		Settings:    &Settings{},
 	}
 	err := hasNilField(account)

diff --git a/management/server/dns.go b/management/server/dns.go
@@ -24,19 +24,11 @@ type DNSSettings struct {
 }
 
 // Copy returns a copy of the DNS settings
-func (d *DNSSettings) Copy() *DNSSettings {
-	settings := &DNSSettings{
-		DisabledManagementGroups: make([]string, 0),
+func (d DNSSettings) Copy() DNSSettings {
+	settings := DNSSettings{
+		DisabledManagementGroups: make([]string, len(d.DisabledManagementGroups)),
 	}
-
-	if d == nil {
-		return settings
-	}
-
-	if d.DisabledManagementGroups != nil && len(d.DisabledManagementGroups) > 0 {
-		settings.DisabledManagementGroups = d.DisabledManagementGroups[:]
-	}
-
+	copy(settings.DisabledManagementGroups, d.DisabledManagementGroups)
 	return settings
 }
 
@@ -58,12 +50,8 @@ func (am *DefaultAccountManager) GetDNSSettings(accountID string, userID string)
 	if !user.IsAdmin() {
 		return nil, status.Errorf(status.PermissionDenied, "only admins are allowed to view DNS settings")
 	}
-
-	if account.DNSSettings == nil {
-		return &DNSSettings{}, nil
-	}
-
-	return account.DNSSettings.Copy(), nil
+	dnsSettings := account.DNSSettings.Copy()
+	return &dnsSettings, nil
 }
 
 // SaveDNSSettings validates a user role and updates the account's DNS settings
@@ -96,11 +84,7 @@ func (am *DefaultAccountManager) SaveDNSSettings(accountID string, userID string
 		}
 	}
 
-	oldSettings := &DNSSettings{}
-	if account.DNSSettings != nil {
-		oldSettings = account.DNSSettings.Copy()
-	}
-
+	oldSettings := account.DNSSettings.Copy()
 	account.DNSSettings = dnsSettingsToSave.Copy()
 
 	account.Network.IncSerial()

diff --git a/management/server/dns_test.go b/management/server/dns_test.go
@@ -42,7 +42,7 @@ func TestGetDNSSettings(t *testing.T) {
 		t.Fatal("DNS settings for new accounts shouldn't return nil")
 	}
 
-	account.DNSSettings = &DNSSettings{
+	account.DNSSettings = DNSSettings{
 		DisabledManagementGroups: []string{group1ID},
 	}
 

diff --git a/management/server/http/dns_settings_handler_test.go b/management/server/http/dns_settings_handler_test.go
@@ -26,7 +26,7 @@ const (
 	testDNSSettingsUserID        = "test_user"
 )
 
-var baseExistingDNSSettings = &server.DNSSettings{
+var baseExistingDNSSettings = server.DNSSettings{
 	DisabledManagementGroups: []string{testDNSSettingsExistingGroup},
 }
 
@@ -43,7 +43,7 @@ func initDNSSettingsTestData() *DNSSettingsHandler {
 	return &DNSSettingsHandler{
 		accountManager: &mock_server.MockAccountManager{
 			GetDNSSettingsFunc: func(accountID string, userID string) (*server.DNSSettings, error) {
-				return testingDNSSettingsAccount.DNSSettings, nil
+				return &testingDNSSettingsAccount.DNSSettings, nil
 			},
 			SaveDNSSettingsFunc: func(accountID string, userID string, dnsSettingsToSave *server.DNSSettings) error {
 				if dnsSettingsToSave != nil {