diff --git a/rest/admin_api.go b/rest/admin_api.go index de1965676e..ce528960cc 100644 --- a/rest/admin_api.go +++ b/rest/admin_api.go @@ -917,25 +917,21 @@ func mutateConfigFromDbAuditConfigBody(isReplace bool, existingAuditConfig *DbAu // initialize to non-nil set of defaults before modifying from request existingAuditConfig.EnabledEvents = &base.DefaultDbAuditEventIDs } - if existingAuditConfig.EnabledEvents != nil { - for i, event := range *existingAuditConfig.EnabledEvents { - if shouldEnable, ok := eventsToChange[base.AuditID(event)]; ok { - if shouldEnable { - // already enabled - } else { - // disable by removing - *existingAuditConfig.EnabledEvents = append((*existingAuditConfig.EnabledEvents)[:i], (*existingAuditConfig.EnabledEvents)[i+1:]...) - } - // drop from toChange so we don't duplicate IDs - delete(eventsToChange, base.AuditID(event)) - } + // build EnabledEvents back up in temp based on request - avoids mutating slice in-place during iteration + // slice[:0] reuses underlying array to avoid alloc of a new slice + newEnabledEvents := (*existingAuditConfig.EnabledEvents)[:0] + for _, event := range *existingAuditConfig.EnabledEvents { + if _, ok := eventsToChange[base.AuditID(event)]; !ok { + // existing enabled event and not in request - don't change + newEnabledEvents = append(newEnabledEvents, event) } - for id, enabled := range eventsToChange { - if enabled { - *existingAuditConfig.EnabledEvents = append(*existingAuditConfig.EnabledEvents, uint(id)) - } + } + for id, enabled := range eventsToChange { + if enabled { + newEnabledEvents = append(newEnabledEvents, uint(id)) } } + *existingAuditConfig.EnabledEvents = newEnabledEvents } } } diff --git a/rest/adminapitest/admin_api_test.go b/rest/adminapitest/admin_api_test.go index 44b28ce56b..63fffe8339 100644 --- a/rest/adminapitest/admin_api_test.go +++ b/rest/adminapitest/admin_api_test.go @@ -4298,19 +4298,33 @@ func TestDatabaseConfigAuditAPI(t *testing.T) { responseBody = nil require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &responseBody)) assert.Equal(t, true, responseBody["enabled"].(bool)) - assert.False(t, responseBody["events"].(map[string]interface{})[base.AuditIDISGRStatus.String()].(bool), "audit enabled event should be disabled by default") - assert.True(t, responseBody["events"].(map[string]interface{})[base.AuditIDPublicUserAuthenticated.String()].(bool), "public user authenticated event should be enabled by default") + eventsMap, ok := responseBody["events"].(map[string]interface{}) + require.True(t, ok) + assert.False(t, eventsMap[base.AuditIDISGRStatus.String()].(bool), "audit enabled event should be disabled by default") + assert.True(t, eventsMap[base.AuditIDPublicUserAuthenticated.String()].(bool), "public user authenticated event should be enabled by default") + + // use event IDs returned from GET response to disable all of them + for id := range eventsMap { + eventsMap[id] = false + } + eventsJSON, err := json.Marshal(eventsMap) + require.NoError(t, err) // CBG-4111: Try to disable events on top of the default (nil) set... either PUT or POST where *all* of the given IDs are set to false. Bug results in a no-op. - resp = rt.SendAdminRequest(http.MethodPost, "/db/_config/audit", fmt.Sprintf(`{"enabled":true,"events":{"%s":false}}`, base.AuditIDPublicUserAuthenticated)) + // CBG-4157: Ensure ALL specified events were actually disabled. Bug results in some events remaining 'true', and sometimes panicking by going out-of-bounds in a slice. + resp = rt.SendAdminRequest(http.MethodPost, "/db/_config/audit", fmt.Sprintf(`{"enabled":true,"events":%s}`, eventsJSON)) rest.RequireStatus(t, resp, http.StatusOK) - // check event we just tried to disable + // check all events were actually disabled resp = rt.SendAdminRequest(http.MethodGet, "/db/_config/audit", "") rest.RequireStatus(t, resp, http.StatusOK) resp.DumpBody() responseBody = nil require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &responseBody)) - assert.False(t, responseBody["events"].(map[string]interface{})[base.AuditIDPublicUserAuthenticated.String()].(bool), "public user authenticated event should be disabled") + eventsMap, ok = responseBody["events"].(map[string]interface{}) + require.True(t, ok) + for id, val := range eventsMap { + assert.False(t, val.(bool), "event %s should be disabled", id) + } // do a PUT to completely replace the full config (events not declared here will be disabled) // enable AuditEnabled event, but implicitly others