Split connector extra settings per credential path
Registration.ExtraSettings was a single flat list, but the API-key and client-credentials connect dialogs need different fields whenever a provider offers both paths, because a different create resolver and a different driver sits behind each. Replace it with APIKeyExtraSettings and ClientCredentialsExtraSettings, and split the GraphQL surface to match so a client cannot render one path's settings on the other. This fixes two connectors that could not be connected at all. 1Password declared accountId and region only, which are the client-credentials shape. The API-key dialog therefore rendered those two fields, mapAPIKeyExtraSettingToField returned nil for both so buildExtraFields discarded them, and the SCIM-bridge driver failed on an empty SCIMBridgeURL. The console already mapped scimBridgeUrl, but no registration declared that key, so the branch was dead. It now declares scimBridgeUrl on the API-key path and accountId + region on client credentials. Langfuse declared baseUrl as required, but mapAPIKeyExtraSettingToField had no LANGFUSE case, so buildExtraFields dropped the value the customer typed and the mutation failed with "langfuseBaseUrl is required". Every other extra-settings provider had a case. The GraphQL input field, the settings struct, the probe builder and the driver were all already correct; only the console mapping was missing. buildExtraFields now takes the settings list explicitly instead of reading it off the provider, so each dialog passes its own path's list and cannot silently iterate the other one. Register rejects a settings list for a path the provider does not offer, and an empty or duplicate setting key within one list. A key repeated across the two lists is allowed: that is how a dual-path provider declares a setting both dialogs need. The new resolver tests walk the whole chain the console walks, from the key a Registration declares through the mutation input field to the persisted settings struct, so a key renamed on one side and not the other fails in CI instead of at connect time. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
This commit is contained in:
@@ -54,6 +54,43 @@ func TestEveryProviderRegistered(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestEveryProviderSettingsReachADialog asserts that every builtin
|
||||
// registration declares its extra settings on a connect path it actually
|
||||
// offers. A list on an unoffered path is a dead declaration: no dialog reads
|
||||
// it, so the settings never reach the create mutation and the connect attempt
|
||||
// fails on a field the customer was never asked for. Register rejects the same
|
||||
// condition at startup; this pins it per provider so the failure names the
|
||||
// offender rather than panicking inside NewBuiltinRegistry.
|
||||
func TestEveryProviderSettingsReachADialog(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewBuiltinRegistry()
|
||||
|
||||
for _, reg := range r.All() {
|
||||
t.Run(string(reg.Provider), func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
if len(reg.APIKeyExtraSettings) > 0 {
|
||||
assert.Truef(
|
||||
t,
|
||||
reg.SupportsAPIKey || reg.ManagedAPIKey,
|
||||
"provider %q declares APIKeyExtraSettings but offers no API-key path",
|
||||
reg.Provider,
|
||||
)
|
||||
}
|
||||
|
||||
if len(reg.ClientCredentialsExtraSettings) > 0 {
|
||||
assert.Truef(
|
||||
t,
|
||||
reg.SupportsClientCredentials,
|
||||
"provider %q declares ClientCredentialsExtraSettings but offers no client-credentials path",
|
||||
reg.Provider,
|
||||
)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestRegistry_Register exercises the validation and duplicate-detection
|
||||
// paths on Register. Programmer errors at NewBuiltinRegistry time —
|
||||
// nil, empty Provider, empty DisplayName, duplicate — must all surface
|
||||
@@ -144,6 +181,110 @@ func TestRegistry_Register(t *testing.T) {
|
||||
assert.Contains(t, err.Error(), "mutually exclusive")
|
||||
})
|
||||
|
||||
t.Run("APIKeyExtraSettings requires an API-key path", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewRegistry()
|
||||
err := r.Register(&provider.Registration{
|
||||
Provider: coredata.ConnectorProviderSlack,
|
||||
DisplayName: "Slack",
|
||||
APIKeyExtraSettings: []provider.ExtraSetting{{Key: "baseUrl", Label: "Base URL"}},
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "APIKeyExtraSettings requires SupportsAPIKey or ManagedAPIKey")
|
||||
})
|
||||
|
||||
// A ManagedAPIKey provider (Crisp) collects settings without a customer
|
||||
// key, so its API-key list is legitimate even with SupportsAPIKey false.
|
||||
t.Run("APIKeyExtraSettings accepted on a ManagedAPIKey provider", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewRegistry()
|
||||
err := r.Register(&provider.Registration{
|
||||
Provider: coredata.ConnectorProviderSlack,
|
||||
DisplayName: "Slack",
|
||||
ManagedAPIKey: true,
|
||||
APIKeyExtraSettings: []provider.ExtraSetting{{Key: "websiteId", Label: "Website ID"}},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
})
|
||||
|
||||
t.Run("ClientCredentialsExtraSettings requires SupportsClientCredentials", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewRegistry()
|
||||
err := r.Register(&provider.Registration{
|
||||
Provider: coredata.ConnectorProviderSlack,
|
||||
DisplayName: "Slack",
|
||||
SupportsAPIKey: true,
|
||||
ClientCredentialsExtraSettings: []provider.ExtraSetting{{Key: "region", Label: "Region"}},
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "ClientCredentialsExtraSettings requires SupportsClientCredentials")
|
||||
})
|
||||
|
||||
t.Run("duplicate setting key within one list", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewRegistry()
|
||||
err := r.Register(&provider.Registration{
|
||||
Provider: coredata.ConnectorProviderSlack,
|
||||
DisplayName: "Slack",
|
||||
SupportsAPIKey: true,
|
||||
APIKeyExtraSettings: []provider.ExtraSetting{
|
||||
{Key: "region", Label: "Region"},
|
||||
{Key: "region", Label: "Region (again)"},
|
||||
},
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), `APIKeyExtraSettings declares duplicate setting key "region"`)
|
||||
})
|
||||
|
||||
// One setting both dialogs need is declared in both lists; that is not a
|
||||
// duplicate, because each list keys a separate form.
|
||||
t.Run("setting key repeated across the two lists", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewRegistry()
|
||||
err := r.Register(&provider.Registration{
|
||||
Provider: coredata.ConnectorProviderSlack,
|
||||
DisplayName: "Slack",
|
||||
SupportsAPIKey: true,
|
||||
SupportsClientCredentials: true,
|
||||
APIKeyExtraSettings: []provider.ExtraSetting{{Key: "region", Label: "Region"}},
|
||||
ClientCredentialsExtraSettings: []provider.ExtraSetting{{Key: "region", Label: "Region"}},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
})
|
||||
|
||||
t.Run("setting with an empty Key", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewRegistry()
|
||||
err := r.Register(&provider.Registration{
|
||||
Provider: coredata.ConnectorProviderSlack,
|
||||
DisplayName: "Slack",
|
||||
SupportsAPIKey: true,
|
||||
APIKeyExtraSettings: []provider.ExtraSetting{{Label: "Region"}},
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "APIKeyExtraSettings declares a setting with an empty Key or Label")
|
||||
})
|
||||
|
||||
t.Run("setting with an empty Label", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := provider.NewRegistry()
|
||||
err := r.Register(&provider.Registration{
|
||||
Provider: coredata.ConnectorProviderSlack,
|
||||
DisplayName: "Slack",
|
||||
SupportsClientCredentials: true,
|
||||
ClientCredentialsExtraSettings: []provider.ExtraSetting{{Key: "region"}},
|
||||
})
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "ClientCredentialsExtraSettings declares a setting with an empty Key or Label")
|
||||
})
|
||||
|
||||
t.Run("RequiresManagedResourceID requires ManagedAPIKey", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user