From 0f13bbab145bd103c36e5b37738ef1f7164b6d2b Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sun, 2 Aug 2026 04:21:00 +0000 Subject: [PATCH] [management] Align agent-network provider updates with full-state PUT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update handlers across the API build the domain object from the request alone and let the manager preserve what must survive: secrets when the value is empty (reverse-proxy auth hashes, the provider api_key) and server-managed state (session keys). The agent-network provider handler briefly deviated by merging the request onto the stored row, giving operator-editable fields omit-preserves semantics no other endpoint has. Restore the fresh-object flow so a PUT replaces the provider's mutable state, and drop the omit-preserves claims from the request schema — they never matched the shipped behavior, since the nil-gates always ran against a zero-valued struct. The api_key keeps its documented omit-to-keep contract via the manager. --- .../handlers/providers_handler.go | 15 ++----- .../handlers/providers_handler_test.go | 43 ++++++++----------- .../modules/agentnetwork/types/provider.go | 4 +- 3 files changed, 22 insertions(+), 40 deletions(-) diff --git a/management/internals/modules/agentnetwork/handlers/providers_handler.go b/management/internals/modules/agentnetwork/handlers/providers_handler.go index e0cd8b7c4..c05363101 100644 --- a/management/internals/modules/agentnetwork/handlers/providers_handler.go +++ b/management/internals/modules/agentnetwork/handlers/providers_handler.go @@ -193,19 +193,10 @@ func (h *handler) updateProvider(w http.ResponseWriter, r *http.Request) { return } - // Overlay the request onto the stored row so omitted optional fields - // (models, extra_values, toggles, identity headers) keep their persisted - // values instead of silently resetting to zero — FromAPIRequest's - // nil-gating only works against the existing state. - existing, err := h.manager.GetProvider(r.Context(), userAuth.AccountId, userAuth.UserId, providerID) - if err != nil { - util.WriteError(r.Context(), err, w) - return + provider := &types.Provider{ + ID: providerID, + AccountID: userAuth.AccountId, } - provider := existing.Copy() - // Blank the key so it is set only when the caller rotates it; the manager - // preserves the stored key for an empty value. - provider.APIKey = "" provider.FromAPIRequest(&req) updated, err := h.manager.UpdateProvider(r.Context(), userAuth.UserId, provider) diff --git a/management/internals/modules/agentnetwork/handlers/providers_handler_test.go b/management/internals/modules/agentnetwork/handlers/providers_handler_test.go index a1e90d298..05024cde9 100644 --- a/management/internals/modules/agentnetwork/handlers/providers_handler_test.go +++ b/management/internals/modules/agentnetwork/handlers/providers_handler_test.go @@ -54,16 +54,14 @@ func TestValidate_ModelRates(t *testing.T) { } } -// TestProviderHandler_UpdatePreservesOmittedFields is the anti-clobber guard -// for PUT /agent-network/providers/{id}: the fields the OpenAPI schema -// documents as omit-preserves — extra_values, metadata_disabled, identity -// headers, enabled, skip_tls_verification, api_key — must keep their stored -// values when absent from the request. Before the handler merged onto the -// existing row, such an update silently wiped dashboard-configured -// extra_values and reset the toggles account-wide. Models carry no such -// contract: like the rest of the PUT payload they are replaced with what the -// request says, so an omitted list clears. -func TestProviderHandler_UpdatePreservesOmittedFields(t *testing.T) { +// TestProviderHandler_UpdateReplacesFullState pins the update contract shared +// with the other PUT endpoints: the request replaces the provider's mutable +// state, so optional fields absent from the JSON land as their zero values. +// The two exceptions are server-side: the api_key (a secret — omitted means +// "not rotated") and the session keypair, both preserved by the manager. The +// identity headers stay on the wire as explicit empty strings so a cleared +// value round-trips. +func TestProviderHandler_UpdateReplacesFullState(t *testing.T) { f := newAgentNetworkHandlerFixture(t) create := `{ @@ -84,26 +82,21 @@ func TestProviderHandler_UpdatePreservesOmittedFields(t *testing.T) { var created api.AgentNetworkProvider require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &created)) - // Minimal update: only the required fields (a rename), nothing optional. - update := `{"provider_id": "openai_api", "name": "openai-renamed", "upstream_url": "https://api.openai.com"}` + // Minimal update: only the required fields, no api_key. Everything + // optional must land as its zero value. + update := `{"provider_id": "openai_api", "name": "openai-renamed", "upstream_url": "https://api.openai.com", "enabled": true}` rec = f.do(t, nethttp.MethodPut, "/agent-network/providers/"+created.Id, update) - require.Equal(t, nethttp.StatusOK, rec.Code, "update must succeed: %s", rec.Body.String()) + require.Equal(t, nethttp.StatusOK, rec.Code, "update without api_key must succeed (key is preserved): %s", rec.Body.String()) var updated api.AgentNetworkProvider require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &updated)) assert.Equal(t, "openai-renamed", updated.Name, "sent field must apply") - assert.True(t, updated.MetadataDisabled, "omitted metadata_disabled must be preserved") - assert.True(t, updated.SkipTlsVerification, "omitted skip_tls_verification must be preserved") - assert.True(t, updated.Enabled, "omitted enabled must be preserved") - require.NotNil(t, updated.ExtraValues, "omitted extra_values must be preserved") - assert.Equal(t, "pc-prod-3f2a", (*updated.ExtraValues)["x-portkey-config"], "stored extra value must survive the update") - assert.Equal(t, "x-bf-dim-netbird_user_id", updated.IdentityHeaderUserId, "omitted identity header must be preserved") - assert.Empty(t, updated.Models, "models carry no omit-preserves contract: an omitted list is replaced with empty") - - // An explicit "" clears the identity header and stays on the wire. - clearHeader := `{"provider_id": "openai_api", "name": "openai-renamed", "upstream_url": "https://api.openai.com", "identity_header_user_id": ""}` - rec = f.do(t, nethttp.MethodPut, "/agent-network/providers/"+created.Id, clearHeader) - require.Equal(t, nethttp.StatusOK, rec.Code, "clearing update must succeed: %s", rec.Body.String()) + assert.True(t, updated.Enabled, "sent field must apply") + assert.False(t, updated.MetadataDisabled, "omitted metadata_disabled must land as false — PUT replaces the full state") + assert.False(t, updated.SkipTlsVerification, "omitted skip_tls_verification must land as false") + assert.Nil(t, updated.ExtraValues, "omitted extra_values must be cleared") + assert.Equal(t, "", updated.IdentityHeaderUserId, "omitted identity header must be cleared yet stay on the wire") + assert.Empty(t, updated.Models, "omitted models must be cleared") assert.Contains(t, rec.Body.String(), `"identity_header_user_id":""`, "cleared identity header must round-trip as an explicit empty string") } diff --git a/management/internals/modules/agentnetwork/types/provider.go b/management/internals/modules/agentnetwork/types/provider.go index cc0bece7a..b9a194bf6 100644 --- a/management/internals/modules/agentnetwork/types/provider.go +++ b/management/internals/modules/agentnetwork/types/provider.go @@ -164,9 +164,7 @@ func (p *Provider) FromAPIRequest(req *api.AgentNetworkProviderRequest) { p.MetadataDisabled = *req.MetadataDisabled } // Identity-header overrides for catalogs flagged Customizable. - // nil pointer = "field omitted on the wire" → leave the stored - // value untouched (per the openapi description). Empty string is - // an explicit clear that disables stamping for this dimension. + // Empty or omitted disables stamping for this dimension. if req.IdentityHeaderUserId != nil { p.IdentityHeaderUserID = strings.TrimSpace(*req.IdentityHeaderUserId) }