From 8dcdaeb7993fa652debb48912d569afeaba83446 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Wed, 26 Aug 2026 07:05:44 +0000 Subject: [PATCH] [management] Let a discovery request name a record and a new upstream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Editing a provider's upstream URL meant retyping its API key. The key is the one field the API never returns, so there was nothing to retype from, and the dashboard had to demand it because a provider_id request resolved the upstream from the stored row — listing the old endpoint while the form showed the new one. The request now reads as a partial edit of the record it names, the same way PUT on a provider already does: an upstream in the body overrides the stored one, an omitted upstream keeps it. The credential and the catalog entry still come from the record only, so a caller cannot aim a stored key at a vendor of their choosing. --- .../agentnetwork/credentialcheck_test.go | 47 +++++++++++++++++++ .../internals/modules/agentnetwork/manager.go | 17 +++++-- shared/management/http/api/openapi.yml | 4 +- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/management/internals/modules/agentnetwork/credentialcheck_test.go b/management/internals/modules/agentnetwork/credentialcheck_test.go index dc0cf133c..1d35c5858 100644 --- a/management/internals/modules/agentnetwork/credentialcheck_test.go +++ b/management/internals/modules/agentnetwork/credentialcheck_test.go @@ -456,3 +456,50 @@ func TestDiscoverProviderModels_SurfacesTheVendorRefusal(t *testing.T) { require.EqualError(t, err, "the provider rejected the credential") } + +// TestDiscoverProviderModels_ListsAgainstTheUrlOnTheForm covers the edit the +// operator cannot otherwise make: the upstream has been retyped and the +// credential has not, because the API never returned it to be retyped. Naming +// the record supplies the key; the request supplies the URL under test. +func TestDiscoverProviderModels_ListsAgainstTheUrlOnTheForm(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + // Twice: the create, and the listing, which is gated on Create too. + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + f.vendor.requests = nil + + _, err = f.manager.DiscoverProviderModels(ctx, "account1", "user1", modeldiscovery.Request{ + CatalogID: "openai_api", + UpstreamURL: "https://gateway.example.com", + }, created.ID) + require.NoError(t, err) + + asked := f.vendor.only(t) + require.Equal(t, "https://gateway.example.com", asked.UpstreamURL, "the typed url must be the one listed against") + require.Equal(t, "sk-good", asked.APIKey, "and the stored key must be what lists it") +} + +// TestDiscoverProviderModels_FallsBackToTheStoredUrl keeps the plain refresh +// working: a request naming only the record still reaches the saved upstream. +func TestDiscoverProviderModels_FallsBackToTheStoredUrl(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + stored := f.vendor.only(t).UpstreamURL + f.vendor.requests = nil + + _, err = f.manager.DiscoverProviderModels(ctx, "account1", "user1", modeldiscovery.Request{ + CatalogID: "openai_api", + }, created.ID) + require.NoError(t, err) + + require.Equal(t, stored, f.vendor.only(t).UpstreamURL) +} diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index fe646d555..9cf5a54e1 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -204,9 +204,11 @@ func (m *managerImpl) GetProvider(ctx context.Context, accountID, userID, provid // DiscoverProviderModels asks the vendor which models a credential can reach. // -// recordID, when set, names an existing provider whose stored credential and -// upstream are used instead of the ones in req — so the dashboard can refresh -// the list without ever holding the key. +// recordID, when set, names an existing provider whose stored credential is +// used instead of the one in req — so the dashboard can refresh the list +// without ever holding the key. An upstream in req overrides the stored one, +// which is what lets a form list against a URL the operator has typed but not +// saved yet, using the credential they cannot retype. // // Gated on Create rather than Read: this spends the operator's credential // against a third party, which is not something a read-only role should be @@ -227,8 +229,15 @@ func (m *managerImpl) DiscoverProviderModels(ctx context.Context, accountID, use // name a different one would run a provider's credential against // whichever vendor endpoint they picked. req.CatalogID = record.ProviderID - req.UpstreamURL = record.UpstreamURL req.APIKey = record.APIKey + // The upstream is the one field the caller may override, and only for + // entries that serve their listing from it. Those are the operator's + // own endpoints, reached with their own credential, so an unsaved URL + // is no more reachable here than a saved one — and the SSRF guard + // treats both alike. + if strings.TrimSpace(req.UpstreamURL) == "" { + req.UpstreamURL = record.UpstreamURL + } } models, err := m.modelDiscovery.Fetch(ctx, req) diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index c318c8b04..281bb11ea 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -5345,7 +5345,7 @@ components: upstream_url: type: string description: | - The upstream being configured. Used to reach vendors that serve their listing from the same host as inference, and to read back the region for those whose host embeds one. Ignored when provider_id is supplied. + The upstream being configured. Used to reach vendors that serve their listing from the same host as inference, and to read back the region for those whose host embeds one. Sent alongside provider_id, it overrides the stored upstream, so an edit can be listed against the URL on the form before it is saved. example: "https://bedrock-runtime.eu-central-1.amazonaws.com" api_key: type: string @@ -5353,7 +5353,7 @@ components: example: "sk-..." provider_id: type: string - description: Existing Agent Network provider record whose stored credential and upstream should be used. Lets the form refresh the list without the client holding the key. + description: Existing Agent Network provider record to query with. Its stored credential is used, and its upstream unless upstream_url overrides it, so the form can refresh the list without the client holding the key. example: "ch8i4ug6lnn4g9hqv7m0" required: - catalog_provider_id