From b8599f634c06617092e1f2b1373f1b7d4ff6cf0d Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Wed, 11 Oct 2023 23:00:56 +0200 Subject: [PATCH] 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 --- management/server/account.go | 21 +++++-------- management/server/account_test.go | 2 +- management/server/dns.go | 30 +++++-------------- management/server/dns_test.go | 2 +- .../server/http/dns_settings_handler_test.go | 4 +-- 5 files changed, 19 insertions(+), 40 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 8e453a1fe..0e583e17f 100644 --- 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 index 331df2017..e47b3b854 100644 --- 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 index 252782aea..9707cc372 100644 --- 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 index b089949b2..8c979c2a6 100644 --- 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 index c7d135fd1..a2f65a521 100644 --- 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 {