[management] Make the live rotation test prove what it claims

Two gaps a review found in the live suite.

The rotation test finished by renaming the provider and expecting that to
succeed. A rename touches none of the fields the check looks at, so it is
stored without asking the vendor anything — it would have passed just as well
against a key the rotation had already replaced. It now moves the upstream by a
trailing slash, which reaches the same host but differs as a string, so the
check runs and the stored key is what has to satisfy it.

And the refused-url test only asserted the error. An error is not the same fact
as an absent record, so it now lists the providers and looks for the one that
must not be there — which the refused-key test beside it already did.

The discovery upstream override is logged. It sends the stored credential to a
host the caller named, which the same permission set can already do by pointing
the record there and letting the write check it — but that leaves an activity
event behind, and this would otherwise leave nothing.
This commit is contained in:
mlsmaycon
2026-08-26 15:33:16 +00:00
parent b23969db59
commit 540c551f19
2 changed files with 29 additions and 7 deletions

View File

@@ -134,6 +134,14 @@ func TestLiveProviderUrlCheck(t *testing.T) {
require.Equal(t, http.StatusUnprocessableEntity, apiErr.StatusCode)
require.Contains(t, strings.ToLower(apiErr.Message), "could not be reached",
"the message must name the url rather than the credential")
// An error is not the same fact as an absent record: a handler that saved
// first and reported afterwards would satisfy everything above.
all, listErr := srv.ListProviders(ctx)
require.NoError(t, listErr)
for _, p := range all {
require.NotEqual(t, "e2e-cred-badurl", p.Name, "a refused provider must not be stored")
}
}
// TestLiveProviderUpdateKeepsTheWorkingKey is the state the check exists to
@@ -164,8 +172,15 @@ func TestLiveProviderUpdateKeepsTheWorkingKey(t *testing.T) {
// The stored key is never returned by the API, so the proof that it
// survived is that an edit which reuses it still passes its check. A
// replaced key would fail here exactly as the rotation just did.
renamed := credentialProviderRequest(tc, "e2e-cred-rotate-renamed", "")
updated, err := srv.UpdateProvider(ctx, prov.Id, renamed)
//
// The trailing slash is what makes that an actual check: an edit touching
// neither the url, the key nor the catalog entry is stored without asking
// the vendor anything, so a rename alone would pass whatever is on the
// record. Only the host is read out of the upstream, so the same vendor is
// reached — but the string differs, and the check runs.
recheck := credentialProviderRequest(tc, "e2e-cred-rotate-renamed", "")
recheck.UpstreamUrl = tc.upstream + "/"
updated, err := srv.UpdateProvider(ctx, prov.Id, recheck)
require.NoError(t, err, "the working key must still be the stored one")
require.Equal(t, "e2e-cred-rotate-renamed", updated.Name)
}

View File

@@ -230,13 +230,20 @@ func (m *managerImpl) DiscoverProviderModels(ctx context.Context, accountID, use
// whichever vendor endpoint they picked.
req.CatalogID = record.ProviderID
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.
// The upstream is the one field the caller may override, so that a URL
// typed into the form can be listed against before it is saved.
//
// It sends the stored credential to a host the caller named, which is
// a capability they already have: the same permission set updates the
// record's upstream, and that write runs this same check against
// whatever it is pointed at. What it would not otherwise be is silent,
// since the write leaves an activity event behind — so the override is
// recorded here.
if strings.TrimSpace(req.UpstreamURL) == "" {
req.UpstreamURL = record.UpstreamURL
} else if req.UpstreamURL != record.UpstreamURL {
log.WithContext(ctx).Infof("agent network provider %s listed against caller-supplied upstream %s by user %s",
recordID, req.UpstreamURL, userID)
}
}