From fb68e9894118c48e9816a269f7950c4e848da19d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Sibiril?= <81782+aureliensibiril@users.noreply.github.com> Date: Sun, 26 Jul 2026 15:56:10 +0200 Subject: [PATCH] Split connector extra settings per credential path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../dialogs/AddAccessReviewSourceDialog.tsx | 7 +- .../_components/APIKeyConnectorDialog.tsx | 9 +- .../ClientCredentialsConnectorDialog.tsx | 9 +- .../dialogs/_lib/connectorSettings.ts | 20 ++- e2e/console/connector_test.go | 56 +++++-- pkg/connector/provider/better_stack.go | 2 +- pkg/connector/provider/crisp.go | 4 +- pkg/connector/provider/github.go | 2 +- pkg/connector/provider/grafana.go | 2 +- pkg/connector/provider/langfuse.go | 2 +- pkg/connector/provider/langfuse_test.go | 8 +- pkg/connector/provider/metabase.go | 2 +- pkg/connector/provider/metabase_test.go | 8 +- pkg/connector/provider/neon.go | 2 +- pkg/connector/provider/neon_test.go | 8 +- pkg/connector/provider/okta.go | 2 +- pkg/connector/provider/one_password.go | 21 ++- pkg/connector/provider/one_password_test.go | 60 +++++++- pkg/connector/provider/posthog.go | 2 +- pkg/connector/provider/qovery.go | 2 +- pkg/connector/provider/qovery_test.go | 8 +- pkg/connector/provider/registry.go | 39 +++++ pkg/connector/provider/registry_test.go | 141 ++++++++++++++++++ pkg/connector/provider/render.go | 5 +- pkg/connector/provider/render_test.go | 8 +- pkg/connector/provider/scaleway.go | 6 +- pkg/connector/provider/segment.go | 2 +- pkg/connector/provider/sentry.go | 2 +- pkg/connector/provider/signoz.go | 2 +- pkg/connector/provider/signoz_test.go | 8 +- pkg/connector/provider/supabase.go | 2 +- pkg/connector/provider/tally.go | 2 +- pkg/connector/provider/types.go | 29 +++- pkg/server/api/console/v1/base_resolvers.go | 35 ++--- .../api/console/v1/connector_provider_info.go | 37 ++--- .../api/console/v1/connector_settings_test.go | 125 ++++++++++++++++ .../api/console/v1/graphql/connector.graphql | 15 +- 37 files changed, 551 insertions(+), 143 deletions(-) create mode 100644 pkg/server/api/console/v1/connector_settings_test.go diff --git a/apps/console/src/pages/organizations/access-reviews/dialogs/AddAccessReviewSourceDialog.tsx b/apps/console/src/pages/organizations/access-reviews/dialogs/AddAccessReviewSourceDialog.tsx index cddfbf1b8..e1963b203 100644 --- a/apps/console/src/pages/organizations/access-reviews/dialogs/AddAccessReviewSourceDialog.tsx +++ b/apps/console/src/pages/organizations/access-reviews/dialogs/AddAccessReviewSourceDialog.tsx @@ -58,7 +58,12 @@ export const addAccessReviewSourceDialogConnectorProviderInfoFragment = graphql` apiKeyManaged clientCredentialsSupported oauth2Scopes - extraSettings { + apiKeyExtraSettings { + key + label + required + } + clientCredentialsExtraSettings { key label required diff --git a/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx b/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx index d94a0b884..887b8ccbc 100644 --- a/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx +++ b/apps/console/src/pages/organizations/access-reviews/dialogs/_components/APIKeyConnectorDialog.tsx @@ -196,7 +196,7 @@ export function APIKeyConnectorDialog({ return; } - const requiredSettings = provider.extraSettings.filter(s => s.required); + const requiredSettings = provider.apiKeyExtraSettings.filter(s => s.required); if (!hasRequiredExtraSettings(requiredSettings, extraSettingValues)) { return; } @@ -204,7 +204,8 @@ export function APIKeyConnectorDialog({ setIsConnectingAPIKey(true); const extraFields = buildExtraFields( - provider, + provider.provider, + provider.apiKeyExtraSettings, extraSettingValues, mapAPIKeyExtraSettingToField, ); @@ -285,7 +286,7 @@ export function APIKeyConnectorDialog({ ); } - return provider.extraSettings.map((setting) => { + return provider.apiKeyExtraSettings.map((setting) => { const value = extraSettingValues[setting.key] ?? ""; return ( s.required); + const requiredSettings = provider.clientCredentialsExtraSettings.filter(s => s.required); if (!hasRequiredExtraSettings(requiredSettings, clientCredentialsExtraValues)) { return; } @@ -117,7 +117,8 @@ export function ClientCredentialsConnectorDialog({ setIsConnectingClientCredentials(true); const extraFields = buildExtraFields( - provider, + provider.provider, + provider.clientCredentialsExtraSettings, clientCredentialsExtraValues, mapClientCredentialsExtraSettingToField, ); @@ -173,7 +174,7 @@ export function ClientCredentialsConnectorDialog({ }; const clientCredentialsExtraSettingsValid = provider - ? hasRequiredExtraSettings(provider.extraSettings, clientCredentialsExtraValues) + ? hasRequiredExtraSettings(provider.clientCredentialsExtraSettings, clientCredentialsExtraValues) : true; return ( @@ -232,7 +233,7 @@ export function ClientCredentialsConnectorDialog({ value={scope} onChange={(e: React.ChangeEvent) => setScope(e.target.value)} /> - {provider?.extraSettings.map(setting => + {provider?.clientCredentialsExtraSettings.map(setting => setting.key === "region" ? (
diff --git a/apps/console/src/pages/organizations/access-reviews/dialogs/_lib/connectorSettings.ts b/apps/console/src/pages/organizations/access-reviews/dialogs/_lib/connectorSettings.ts index 74cedd97b..875377f12 100644 --- a/apps/console/src/pages/organizations/access-reviews/dialogs/_lib/connectorSettings.ts +++ b/apps/console/src/pages/organizations/access-reviews/dialogs/_lib/connectorSettings.ts @@ -55,6 +55,9 @@ export function mapAPIKeyExtraSettingToField( case "SIGNOZ": if (settingKey === "baseUrl") return "signozBaseUrl"; break; + case "LANGFUSE": + if (settingKey === "baseUrl") return "langfuseBaseUrl"; + break; case "ONE_PASSWORD": if (settingKey === "scimBridgeUrl") return "onePasswordScimBridgeUrl"; break; @@ -115,22 +118,25 @@ export function hasRequiredExtraSettings( .every(s => values[s.key]?.trim()); } -// buildExtraFields flattens a provider's extra settings into the input-field map -// the create mutations expect: each non-empty, trimmed value keyed by its -// provider-specific input field name (via mapFn), skipping settings that map to -// nothing. Shared by the API-key and client-credentials dialogs. +// buildExtraFields flattens one connect path's extra settings into the +// input-field map that path's create mutation expects: each non-empty, trimmed +// value keyed by its provider-specific input field name (via mapFn), skipping +// settings that map to nothing. Each dialog passes the settings list for its own +// path together with the matching mapFn — a provider offering both paths +// (1Password) declares different settings on each. export function buildExtraFields( - provider: ProviderInfo, + provider: string, + settings: ReadonlyArray<{ readonly key: string }>, values: Record, mapFn: (provider: string, settingKey: string) => string | null, ): Record { const extraFields: Record = {}; - for (const setting of provider.extraSettings) { + for (const setting of settings) { const value = values[setting.key]?.trim(); if (!value) { continue; } - const fieldName = mapFn(provider.provider, setting.key); + const fieldName = mapFn(provider, setting.key); if (fieldName) { extraFields[fieldName] = value; } diff --git a/e2e/console/connector_test.go b/e2e/console/connector_test.go index a9f9ae1f1..f5b2fe462 100644 --- a/e2e/console/connector_test.go +++ b/e2e/console/connector_test.go @@ -41,7 +41,12 @@ func TestAccessReviewDrivers(t *testing.T) { oauthConfigured apiKeySupported clientCredentialsSupported - extraSettings { + apiKeyExtraSettings { + key + label + required + } + clientCredentialsExtraSettings { key label required @@ -50,19 +55,22 @@ func TestAccessReviewDrivers(t *testing.T) { } ` + type settingInfo struct { + Key string `json:"key"` + Label string `json:"label"` + Required bool `json:"required"` + } + var result struct { AccessReviewDrivers []struct { - Provider string `json:"provider"` - DisplayName string `json:"displayName"` - DocumentationURL *string `json:"documentationUrl"` - OauthConfigured bool `json:"oauthConfigured"` - APIKeySupported bool `json:"apiKeySupported"` - ClientCredentialsSupported bool `json:"clientCredentialsSupported"` - ExtraSettings []struct { - Key string `json:"key"` - Label string `json:"label"` - Required bool `json:"required"` - } `json:"extraSettings"` + Provider string `json:"provider"` + DisplayName string `json:"displayName"` + DocumentationURL *string `json:"documentationUrl"` + OauthConfigured bool `json:"oauthConfigured"` + APIKeySupported bool `json:"apiKeySupported"` + ClientCredentialsSupported bool `json:"clientCredentialsSupported"` + APIKeyExtraSettings []settingInfo `json:"apiKeyExtraSettings"` + ClientCredentialsExtraSettings []settingInfo `json:"clientCredentialsExtraSettings"` } `json:"accessReviewDrivers"` } @@ -72,18 +80,40 @@ func TestAccessReviewDrivers(t *testing.T) { providerNames := make(map[string]bool) docURLByProvider := make(map[string]*string) + apiKeySettingKeys := make(map[string][]string) + clientCredentialsSettingKeys := make(map[string][]string) for _, info := range result.AccessReviewDrivers { assert.NotEmpty(t, info.Provider) assert.NotEmpty(t, info.DisplayName) - assert.NotNil(t, info.ExtraSettings) + assert.NotNil(t, info.APIKeyExtraSettings) + assert.NotNil(t, info.ClientCredentialsExtraSettings) providerNames[info.Provider] = true docURLByProvider[info.Provider] = info.DocumentationURL + + for _, s := range info.APIKeyExtraSettings { + apiKeySettingKeys[info.Provider] = append(apiKeySettingKeys[info.Provider], s.Key) + } + + for _, s := range info.ClientCredentialsExtraSettings { + clientCredentialsSettingKeys[info.Provider] = append(clientCredentialsSettingKeys[info.Provider], s.Key) + } } assert.True(t, providerNames["BREX"], "expected BREX provider to be present") assert.True(t, providerNames["HUBSPOT"], "expected HUBSPOT provider to be present") + // 1Password is the only provider offering both connect paths, and each path + // needs different settings: the SCIM-bridge driver behind the API key, the + // Users API driver behind client credentials. A client rendering one path's + // settings on the other would collect fields the create resolver rejects. + assert.Equal(t, []string{"scimBridgeUrl"}, apiKeySettingKeys["ONE_PASSWORD"]) + assert.Equal(t, []string{"accountId", "region"}, clientCredentialsSettingKeys["ONE_PASSWORD"]) + + // A single-path provider declares its settings on that path only. + assert.Equal(t, []string{"baseUrl"}, apiKeySettingKeys["LANGFUSE"]) + assert.Empty(t, clientCredentialsSettingKeys["LANGFUSE"]) + // A documented provider exposes its probo.com docs URL; an undocumented one // exposes null. See pkg/connector/provider/docs.go. require.Contains(t, docURLByProvider, "ANTHROPIC") diff --git a/pkg/connector/provider/better_stack.go b/pkg/connector/provider/better_stack.go index 5583fb0aa..54acbfb51 100644 --- a/pkg/connector/provider/better_stack.go +++ b/pkg/connector/provider/better_stack.go @@ -42,7 +42,7 @@ func betterStackRegistration() *Registration { DisplayName: "Better Stack", SupportsAPIKey: true, ProbeURL: "https://betterstack.com/api/v2/team-members", - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "teamName", Label: "Team Name", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/crisp.go b/pkg/connector/provider/crisp.go index 87d96443e..56695b3d0 100644 --- a/pkg/connector/provider/crisp.go +++ b/pkg/connector/provider/crisp.go @@ -54,11 +54,11 @@ func crispRegistration() *Registration { // APIKeyBasicAuthUserPass base64-encodes it (the empty-password // APIKeyBasicAuth cannot carry the key). A plugin token can serve // several websites, so the reviewed website is captured via - // ExtraSettings. Every request also needs the non-auth X-Crisp-Tier + // APIKeyExtraSettings. Every request also needs the non-auth X-Crisp-Tier // header (set by the driver/probe/name resolver), so the probe is a // custom closure. APIKeyBasicAuthUserPass: true, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "websiteId", Label: "Website ID", Required: true}, }, Probe: probeCrisp, diff --git a/pkg/connector/provider/github.go b/pkg/connector/provider/github.go index 64ac72fd5..444a84a04 100644 --- a/pkg/connector/provider/github.go +++ b/pkg/connector/provider/github.go @@ -39,7 +39,7 @@ func githubRegistration() *Registration { ProbeURL: "https://api.github.com/user", OAuth2Scopes: []string{"read:org"}, SupportsAPIKey: true, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "organization", Label: "Organization", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, logger *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/grafana.go b/pkg/connector/provider/grafana.go index 6f3d1b21a..94370ba7e 100644 --- a/pkg/connector/provider/grafana.go +++ b/pkg/connector/provider/grafana.go @@ -38,7 +38,7 @@ func grafanaRegistration() *Registration { DisplayName: "Grafana", SupportsAPIKey: true, BuildProbeURL: buildGrafanaProbeURL, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "baseUrl", Label: "Base URL", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/langfuse.go b/pkg/connector/provider/langfuse.go index 6b1f4ba57..6eb6881f0 100644 --- a/pkg/connector/provider/langfuse.go +++ b/pkg/connector/provider/langfuse.go @@ -45,7 +45,7 @@ func langfuseRegistration() *Registration { // there is nothing to pick; only the regional/self-hosted base URL // is per-tenant and is surfaced as an extra setting. APIKeyBasicAuthUserPass: true, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "baseUrl", Label: "Base URL", Required: true}, }, // BuildProbeURL derives the probe endpoint from the per-connection diff --git a/pkg/connector/provider/langfuse_test.go b/pkg/connector/provider/langfuse_test.go index e7e97ff02..877f7e8c3 100644 --- a/pkg/connector/provider/langfuse_test.go +++ b/pkg/connector/provider/langfuse_test.go @@ -46,10 +46,10 @@ func TestLangfuseRegistrationMetadata(t *testing.T) { assert.True(t, reg.APIKeyBasicAuthUserPass) assert.Empty(t, reg.APIKeyHeader) assert.Empty(t, reg.APIKeyAuthScheme) - require.Len(t, reg.ExtraSettings, 1) - assert.Equal(t, "baseUrl", reg.ExtraSettings[0].Key) - assert.Equal(t, "Base URL", reg.ExtraSettings[0].Label) - assert.True(t, reg.ExtraSettings[0].Required) + require.Len(t, reg.APIKeyExtraSettings, 1) + assert.Equal(t, "baseUrl", reg.APIKeyExtraSettings[0].Key) + assert.Equal(t, "Base URL", reg.APIKeyExtraSettings[0].Label) + assert.True(t, reg.APIKeyExtraSettings[0].Required) // Single-tenant API-key provider: no picker, no name resolver. assert.Nil(t, reg.NewNameResolver, "langfuse must not wire a name resolver") assert.Nil(t, reg.SetOrganizationSettings, "langfuse must not wire a picker store") diff --git a/pkg/connector/provider/metabase.go b/pkg/connector/provider/metabase.go index a5166ade0..5c3c22e66 100644 --- a/pkg/connector/provider/metabase.go +++ b/pkg/connector/provider/metabase.go @@ -39,7 +39,7 @@ func metabaseRegistration() *Registration { SupportsAPIKey: true, APIKeyHeader: "x-api-key", BuildProbeURL: buildMetabaseProbeURL, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "instanceUrl", Label: "Instance URL", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/metabase_test.go b/pkg/connector/provider/metabase_test.go index fb7b99ab2..c84517802 100644 --- a/pkg/connector/provider/metabase_test.go +++ b/pkg/connector/provider/metabase_test.go @@ -43,10 +43,10 @@ func TestMetabaseRegistrationMetadata(t *testing.T) { assert.Equal(t, "Metabase", reg.DisplayName) assert.True(t, reg.SupportsAPIKey) assert.Equal(t, "x-api-key", reg.APIKeyHeader) - require.Len(t, reg.ExtraSettings, 1) - assert.Equal(t, "instanceUrl", reg.ExtraSettings[0].Key) - assert.Equal(t, "Instance URL", reg.ExtraSettings[0].Label) - assert.True(t, reg.ExtraSettings[0].Required) + require.Len(t, reg.APIKeyExtraSettings, 1) + assert.Equal(t, "instanceUrl", reg.APIKeyExtraSettings[0].Key) + assert.Equal(t, "Instance URL", reg.APIKeyExtraSettings[0].Label) + assert.True(t, reg.APIKeyExtraSettings[0].Required) } func TestMetabaseNewDriver(t *testing.T) { diff --git a/pkg/connector/provider/neon.go b/pkg/connector/provider/neon.go index d9d93c30b..4c4380fd6 100644 --- a/pkg/connector/provider/neon.go +++ b/pkg/connector/provider/neon.go @@ -43,7 +43,7 @@ func neonRegistration() *Registration { // SupportsAPIKey: true, BuildProbeURL: buildNeonProbeURL, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "organizationId", Label: "Organization ID", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/neon_test.go b/pkg/connector/provider/neon_test.go index 1b784b0d4..eb6203277 100644 --- a/pkg/connector/provider/neon_test.go +++ b/pkg/connector/provider/neon_test.go @@ -43,10 +43,10 @@ func TestNeonRegistrationMetadata(t *testing.T) { assert.Equal(t, "Neon", reg.DisplayName) assert.True(t, reg.SupportsAPIKey) assert.Empty(t, reg.APIKeyAuthScheme, "neon API keys use the default Bearer scheme") - require.Len(t, reg.ExtraSettings, 1) - assert.Equal(t, "organizationId", reg.ExtraSettings[0].Key) - assert.Equal(t, "Organization ID", reg.ExtraSettings[0].Label) - assert.True(t, reg.ExtraSettings[0].Required) + require.Len(t, reg.APIKeyExtraSettings, 1) + assert.Equal(t, "organizationId", reg.APIKeyExtraSettings[0].Key) + assert.Equal(t, "Organization ID", reg.APIKeyExtraSettings[0].Label) + assert.True(t, reg.APIKeyExtraSettings[0].Required) } func TestNeonNewDriver(t *testing.T) { diff --git a/pkg/connector/provider/okta.go b/pkg/connector/provider/okta.go index 1f0a8ed0e..eb1ba58b6 100644 --- a/pkg/connector/provider/okta.go +++ b/pkg/connector/provider/okta.go @@ -44,7 +44,7 @@ func oktaRegistration() *Registration { SupportsAPIKey: true, APIKeyAuthScheme: "SSWS", BuildProbeURL: buildOktaProbeURL, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "domain", Label: "Okta Domain", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/one_password.go b/pkg/connector/provider/one_password.go index 1f7bb7dfc..0f93e5c2e 100644 --- a/pkg/connector/provider/one_password.go +++ b/pkg/connector/provider/one_password.go @@ -38,18 +38,23 @@ func onePasswordRegistration() *Registration { ProbeURL: "https://events.1password.com/api/v1/auditevents", SupportsAPIKey: true, SupportsClientCredentials: true, - ExtraSettings: []ExtraSetting{ + // Two settings shapes, one per connect path, because a different + // driver sits behind each: + // - API key: SCIMBridgeURL (SCIM-bridge driver). + // - Client credentials: AccountID + Region (Users API driver). + APIKeyExtraSettings: []ExtraSetting{ + {Key: "scimBridgeUrl", Label: "SCIM Bridge URL", Required: true}, + }, + ClientCredentialsExtraSettings: []ExtraSetting{ {Key: "accountId", Label: "Account ID", Required: true}, {Key: "region", Label: "Region", Required: true}, }, - // 1Password has two settings shapes selected by protocol: - // - Client-credentials: AccountID + Region (Users API driver). - // - API key: SCIMBridgeURL (SCIM-bridge driver). - // The create resolvers build the matching settings; only one - // path is possible for any given request. NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { - // Client credentials grant uses the Users API driver; the - // authorization-code grant uses the SCIM-bridge driver. + // The client-credentials grant uses the Users API driver. + // Everything else is the API-key connection, whose + // *APIKeyConnection makes GrantType() return "": it uses the + // SCIM-bridge driver. 1Password declares no AuthURL/TokenURL, so + // the authorization-code path is unreachable. if conn.GrantType() == string(connector.OAuth2GrantTypeClientCredentials) { s, err := coredata.ConnectorSettings[coredata.OnePasswordUsersAPISettings](conn) if err != nil { diff --git a/pkg/connector/provider/one_password_test.go b/pkg/connector/provider/one_password_test.go index 60e21b005..5431e56e7 100644 --- a/pkg/connector/provider/one_password_test.go +++ b/pkg/connector/provider/one_password_test.go @@ -34,11 +34,43 @@ import ( "go.probo.inc/probo/pkg/coredata" ) +// TestOnePasswordRegistrationMetadata pins the per-connect-path settings +// split. 1Password is the only registration offering both paths, and each +// needs different settings because a different driver sits behind each: a +// dialog handed the other path's list would collect fields the create +// resolver rejects, which is exactly how the API-key path was broken while +// the two shapes shared one flat list. +func TestOnePasswordRegistrationMetadata(t *testing.T) { + t.Parallel() + + r := provider.NewBuiltinRegistry() + reg, ok := r.Get(coredata.ConnectorProviderOnePassword) + require.True(t, ok, "1Password provider must be registered") + + assert.Equal(t, "1Password", reg.DisplayName) + assert.True(t, reg.SupportsAPIKey) + assert.True(t, reg.SupportsClientCredentials) + + require.Len(t, reg.APIKeyExtraSettings, 1) + assert.Equal(t, "scimBridgeUrl", reg.APIKeyExtraSettings[0].Key) + assert.Equal(t, "SCIM Bridge URL", reg.APIKeyExtraSettings[0].Label) + assert.True(t, reg.APIKeyExtraSettings[0].Required) + + require.Len(t, reg.ClientCredentialsExtraSettings, 2) + assert.Equal(t, "accountId", reg.ClientCredentialsExtraSettings[0].Key) + assert.Equal(t, "Account ID", reg.ClientCredentialsExtraSettings[0].Label) + assert.True(t, reg.ClientCredentialsExtraSettings[0].Required) + assert.Equal(t, "region", reg.ClientCredentialsExtraSettings[1].Key) + assert.Equal(t, "Region", reg.ClientCredentialsExtraSettings[1].Label) + assert.True(t, reg.ClientCredentialsExtraSettings[1].Required) +} + // TestOnePassword_NewDriver_DispatchByGrantType is the pre-merge gate // for the 1Password closure. The OnePassword registration dispatches -// between two drivers based on the connector's OAuth2 grant type — -// this test asserts both paths construct without error from a -// coredata.Connector shaped for each grant type. +// between two drivers on GrantType(), which is "" for an API-key +// connection — this test asserts every connection shape reaching the +// closure constructs the driver whose settings the matching +// per-path settings list collects. func TestOnePassword_NewDriver_DispatchByGrantType(t *testing.T) { t.Parallel() @@ -69,6 +101,28 @@ func TestOnePassword_NewDriver_DispatchByGrantType(t *testing.T) { assert.IsType(t, &drivers.OnePasswordUsersAPIDriver{}, drv) }) + // The production API-key path: an *APIKeyConnection makes GrantType() + // return "", so it falls through to the SCIM-bridge driver and reads the + // SCIMBridgeURL that APIKeyExtraSettings collects. + t.Run("api key uses SCIM-bridge driver", func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(&coredata.OnePasswordConnectorSettings{ + SCIMBridgeURL: "https://scim.example.test", + }) + require.NoError(t, err) + + conn := &coredata.Connector{ + Provider: coredata.ConnectorProviderOnePassword, + RawSettings: raw, + Connection: &connector.APIKeyConnection{APIKey: "scim-token"}, + } + + drv, err := reg.NewDriver(context.Background(), httpclient.DefaultClient(httpclient.WithSSRFProtection()), conn, nil) + require.NoError(t, err) + assert.IsType(t, &drivers.OnePasswordDriver{}, drv) + }) + t.Run("authorization_code uses SCIM-bridge driver", func(t *testing.T) { t.Parallel() diff --git a/pkg/connector/provider/posthog.go b/pkg/connector/provider/posthog.go index 2dd265711..1eb081774 100644 --- a/pkg/connector/provider/posthog.go +++ b/pkg/connector/provider/posthog.go @@ -64,7 +64,7 @@ func posthogRegistration() *Registration { // self-hosted (an instance URL). The two are mutually exclusive, so // neither is individually Required; apiKeyConnectorSettings enforces // that exactly one is supplied. - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "region", Label: "Region"}, {Key: "instanceUrl", Label: "Instance URL"}, }, diff --git a/pkg/connector/provider/qovery.go b/pkg/connector/provider/qovery.go index 2c1f87f4e..f78b1f89c 100644 --- a/pkg/connector/provider/qovery.go +++ b/pkg/connector/provider/qovery.go @@ -37,7 +37,7 @@ func qoveryRegistration() *Registration { SupportsAPIKey: true, APIKeyAuthScheme: "Token", BuildProbeURL: buildQoveryProbeURL, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "organizationId", Label: "Organization ID", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/qovery_test.go b/pkg/connector/provider/qovery_test.go index b0370360c..95a3ae09a 100644 --- a/pkg/connector/provider/qovery_test.go +++ b/pkg/connector/provider/qovery_test.go @@ -43,10 +43,10 @@ func TestQoveryRegistrationMetadata(t *testing.T) { assert.Equal(t, "Qovery", reg.DisplayName) assert.True(t, reg.SupportsAPIKey) assert.Equal(t, "Token", reg.APIKeyAuthScheme) - require.Len(t, reg.ExtraSettings, 1) - assert.Equal(t, "organizationId", reg.ExtraSettings[0].Key) - assert.Equal(t, "Organization ID", reg.ExtraSettings[0].Label) - assert.True(t, reg.ExtraSettings[0].Required) + require.Len(t, reg.APIKeyExtraSettings, 1) + assert.Equal(t, "organizationId", reg.APIKeyExtraSettings[0].Key) + assert.Equal(t, "Organization ID", reg.APIKeyExtraSettings[0].Label) + assert.True(t, reg.APIKeyExtraSettings[0].Required) } func TestQoveryNewDriver(t *testing.T) { diff --git a/pkg/connector/provider/registry.go b/pkg/connector/provider/registry.go index d78268d41..5bbcfdd3f 100644 --- a/pkg/connector/provider/registry.go +++ b/pkg/connector/provider/registry.go @@ -142,6 +142,45 @@ func (r *Registry) Register(reg *Registration) error { return fmt.Errorf("cannot register connector provider %q: BuildTokenURLForDomain and BuildTokenURLForSite are mutually exclusive", reg.Provider) } + // A per-path settings list for a path the provider cannot offer is a dead + // declaration: no dialog will ever render it. ManagedAPIKey counts as an + // API-key path — the customer supplies the settings, Probo the key. + if len(reg.APIKeyExtraSettings) > 0 && !reg.SupportsAPIKey && !reg.ManagedAPIKey { + return fmt.Errorf("cannot register connector provider %q: APIKeyExtraSettings requires SupportsAPIKey or ManagedAPIKey", reg.Provider) + } + + if len(reg.ClientCredentialsExtraSettings) > 0 && !reg.SupportsClientCredentials { + return fmt.Errorf("cannot register connector provider %q: ClientCredentialsExtraSettings requires SupportsClientCredentials", reg.Provider) + } + + // The console keys both its form state and its submitted values by setting + // key within one dialog, so a duplicate key silently collapses two fields + // into one and an empty key produces an unlabelled field bound to nothing. + // Reject both at startup. A key repeated across the two lists is fine and + // intended: that is how a dual-path provider declares one setting both + // dialogs need. + for _, list := range []struct { + field string + settings []ExtraSetting + }{ + {"APIKeyExtraSettings", reg.APIKeyExtraSettings}, + {"ClientCredentialsExtraSettings", reg.ClientCredentialsExtraSettings}, + } { + seen := make(map[string]bool, len(list.settings)) + + for _, s := range list.settings { + if s.Key == "" || s.Label == "" { + return fmt.Errorf("cannot register connector provider %q: %s declares a setting with an empty Key or Label", reg.Provider, list.field) + } + + if seen[s.Key] { + return fmt.Errorf("cannot register connector provider %q: %s declares duplicate setting key %q", reg.Provider, list.field, s.Key) + } + + seen[s.Key] = true + } + } + r.mu.Lock() defer r.mu.Unlock() diff --git a/pkg/connector/provider/registry_test.go b/pkg/connector/provider/registry_test.go index 70b52c3f5..43b4f933f 100644 --- a/pkg/connector/provider/registry_test.go +++ b/pkg/connector/provider/registry_test.go @@ -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() diff --git a/pkg/connector/provider/render.go b/pkg/connector/provider/render.go index e90733808..c01942181 100644 --- a/pkg/connector/provider/render.go +++ b/pkg/connector/provider/render.go @@ -35,14 +35,15 @@ import ( // read-scoped API key plus their Workspace ID (Render's owner ID). The key // authenticates with the default Authorization: Bearer scheme, so no // APIKeyAuthScheme override is set. There is no picker — the workspace is -// captured up front via ExtraSettings — so SetOrganizationSettings is omitted. +// captured up front via APIKeyExtraSettings — so SetOrganizationSettings is +// omitted. func renderRegistration() *Registration { return &Registration{ Provider: coredata.ConnectorProviderRender, DisplayName: "Render", SupportsAPIKey: true, BuildProbeURL: buildRenderProbeURL, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "workspaceId", Label: "Workspace ID", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/render_test.go b/pkg/connector/provider/render_test.go index 03a440f5f..8b76dad2a 100644 --- a/pkg/connector/provider/render_test.go +++ b/pkg/connector/provider/render_test.go @@ -50,10 +50,10 @@ func TestRenderRegistrationMetadata(t *testing.T) { assert.Empty(t, reg.AuthURL) assert.Nil(t, reg.SetOrganizationSettings) - require.Len(t, reg.ExtraSettings, 1) - assert.Equal(t, "workspaceId", reg.ExtraSettings[0].Key) - assert.Equal(t, "Workspace ID", reg.ExtraSettings[0].Label) - assert.True(t, reg.ExtraSettings[0].Required) + require.Len(t, reg.APIKeyExtraSettings, 1) + assert.Equal(t, "workspaceId", reg.APIKeyExtraSettings[0].Key) + assert.Equal(t, "Workspace ID", reg.APIKeyExtraSettings[0].Label) + assert.True(t, reg.APIKeyExtraSettings[0].Required) } func TestRenderNewDriver(t *testing.T) { diff --git a/pkg/connector/provider/scaleway.go b/pkg/connector/provider/scaleway.go index 48b2991f3..42d5edd1f 100644 --- a/pkg/connector/provider/scaleway.go +++ b/pkg/connector/provider/scaleway.go @@ -40,10 +40,10 @@ func scalewayRegistration() *Registration { // rather than Authorization: Bearer. APIKeyHeader makes the // APIKeyConnection send that header and omit Authorization. The key is // bound to one Organization, but GET /iam/v1alpha1/users requires the - // organization_id explicitly, so it is captured via ExtraSettings rather - // than discovered — hence no picker and a BuildProbeURL. + // organization_id explicitly, so it is captured via APIKeyExtraSettings + // rather than discovered — hence no picker and a BuildProbeURL. APIKeyHeader: "X-Auth-Token", - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "organizationId", Label: "Organization ID", Required: true}, }, BuildProbeURL: buildScalewayProbeURL, diff --git a/pkg/connector/provider/segment.go b/pkg/connector/provider/segment.go index 8d7ccccdf..6ba2e025b 100644 --- a/pkg/connector/provider/segment.go +++ b/pkg/connector/provider/segment.go @@ -43,7 +43,7 @@ func segmentRegistration() *Registration { // (US vs EU) and is not discoverable from the token, so it is captured // as an extra setting and resolved to a base URL (Pattern 3 + region); // there is nothing to pick. - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "region", Label: "Region", Required: true}, }, BuildProbeURL: buildSegmentProbeURL, diff --git a/pkg/connector/provider/sentry.go b/pkg/connector/provider/sentry.go index 199cd9bd4..b1ee046f0 100644 --- a/pkg/connector/provider/sentry.go +++ b/pkg/connector/provider/sentry.go @@ -39,7 +39,7 @@ func sentryRegistration() *Registration { ProbeURL: "https://sentry.io/api/0/organizations/", OAuth2Scopes: []string{"org:read", "member:read"}, SupportsAPIKey: true, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "organizationSlug", Label: "Organization Slug", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/signoz.go b/pkg/connector/provider/signoz.go index 5a3b7fda0..777eddf48 100644 --- a/pkg/connector/provider/signoz.go +++ b/pkg/connector/provider/signoz.go @@ -39,7 +39,7 @@ func signozRegistration() *Registration { SupportsAPIKey: true, APIKeyHeader: "SIGNOZ-API-KEY", BuildProbeURL: buildSigNozProbeURL, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "baseUrl", Label: "Base URL", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/signoz_test.go b/pkg/connector/provider/signoz_test.go index c722e2d04..6736f97dc 100644 --- a/pkg/connector/provider/signoz_test.go +++ b/pkg/connector/provider/signoz_test.go @@ -43,10 +43,10 @@ func TestSigNozRegistrationMetadata(t *testing.T) { assert.Equal(t, "SigNoz", reg.DisplayName) assert.True(t, reg.SupportsAPIKey) assert.Equal(t, "SIGNOZ-API-KEY", reg.APIKeyHeader) - require.Len(t, reg.ExtraSettings, 1) - assert.Equal(t, "baseUrl", reg.ExtraSettings[0].Key) - assert.Equal(t, "Base URL", reg.ExtraSettings[0].Label) - assert.True(t, reg.ExtraSettings[0].Required) + require.Len(t, reg.APIKeyExtraSettings, 1) + assert.Equal(t, "baseUrl", reg.APIKeyExtraSettings[0].Key) + assert.Equal(t, "Base URL", reg.APIKeyExtraSettings[0].Label) + assert.True(t, reg.APIKeyExtraSettings[0].Required) require.NotNil(t, reg.NewNameResolver, "signoz NewNameResolver closure must be wired") } diff --git a/pkg/connector/provider/supabase.go b/pkg/connector/provider/supabase.go index 0271490e2..948a86d50 100644 --- a/pkg/connector/provider/supabase.go +++ b/pkg/connector/provider/supabase.go @@ -36,7 +36,7 @@ func supabaseRegistration() *Registration { DisplayName: "Supabase", ProbeURL: "https://api.supabase.com/v1/organizations", SupportsAPIKey: true, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "organizationSlug", Label: "Organization Slug", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/tally.go b/pkg/connector/provider/tally.go index 9bb287f7c..e1ccf1197 100644 --- a/pkg/connector/provider/tally.go +++ b/pkg/connector/provider/tally.go @@ -36,7 +36,7 @@ func tallyRegistration() *Registration { DisplayName: "Tally", ProbeURL: "https://api.tally.so/me", SupportsAPIKey: true, - ExtraSettings: []ExtraSetting{ + APIKeyExtraSettings: []ExtraSetting{ {Key: "organizationId", Label: "Organization ID", Required: true}, }, NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { diff --git a/pkg/connector/provider/types.go b/pkg/connector/provider/types.go index 96371d2e4..62517b4b1 100644 --- a/pkg/connector/provider/types.go +++ b/pkg/connector/provider/types.go @@ -89,7 +89,20 @@ type Registration struct { // Protocol support / GraphQL surface. SupportsAPIKey bool SupportsClientCredentials bool - ExtraSettings []ExtraSetting + // APIKeyExtraSettings declares the per-provider settings fields the + // console's API-key connect dialog renders and submits, in render order. + // It covers a ManagedAPIKey provider too (Crisp): the customer supplies + // the settings, Probo supplies the key. Nil for a provider with no + // API-key path. + APIKeyExtraSettings []ExtraSetting + // ClientCredentialsExtraSettings declares the settings fields the + // client-credentials connect dialog renders and submits. The two lists are + // independent because a different create resolver and a different driver + // sit behind each path: 1Password needs SCIMBridgeURL on the API key + // (SCIM-bridge driver) and AccountID + Region on client credentials (Users + // API driver). A setting genuinely needed on both paths is declared in + // both lists. + ClientCredentialsExtraSettings []ExtraSetting // APIKeyHeader selects how an API-key connection presents its key // on outbound requests. Empty (the default) uses the standard // `Authorization: Bearer ` scheme; a value such as "x-api-key" @@ -125,13 +138,13 @@ type Registration struct { // ManagedAPIKey marks a provider whose API key is supplied by Probo // from bootstrap config (a single, Probo-held credential shared across // all connections) rather than pasted per-connection by the customer. - // The connection carries only the ExtraSettings (e.g. a Crisp Website - // ID); the create-connector resolver injects the managed key registered - // via (*Registry).SetManagedAPIKey. Such a provider stays hidden from - // the driver catalog until the operator configures the key, so it ships - // deactivated and activates with no code change. Orthogonal to the - // APIKey*/SupportsAPIKey auth-mode flags, which still select how the - // injected key is presented on the wire. + // The connection carries only the APIKeyExtraSettings (e.g. a Crisp + // Website ID); the create-connector resolver injects the managed key + // registered via (*Registry).SetManagedAPIKey. Such a provider stays + // hidden from the driver catalog until the operator configures the key, + // so it ships deactivated and activates with no code change. Orthogonal + // to the APIKey*/SupportsAPIKey auth-mode flags, which still select how + // the injected key is presented on the wire. ManagedAPIKey bool // RequiresManagedResourceID marks a ManagedAPIKey provider that also needs diff --git a/pkg/server/api/console/v1/base_resolvers.go b/pkg/server/api/console/v1/base_resolvers.go index 62bdfda6c..45317d19a 100644 --- a/pkg/server/api/console/v1/base_resolvers.go +++ b/pkg/server/api/console/v1/base_resolvers.go @@ -606,33 +606,26 @@ func (r *queryResolver) AccessReviewDrivers(ctx context.Context) ([]*types.Conne scopes = []string{} } - extraSettings := make([]*types.ConnectorProviderSettingInfo, 0, len(reg.ExtraSettings)) - for _, setting := range reg.ExtraSettings { - extraSettings = append( - extraSettings, - &types.ConnectorProviderSettingInfo{ - Key: setting.Key, - Label: setting.Label, - Required: setting.Required, - }, - ) - } - var documentationURL *string if reg.DocumentationURL != "" { documentationURL = new(reg.DocumentationURL) } + // The two settings lists are surfaced separately, never merged: a + // provider offering both connect paths (1Password) needs different + // fields on each, so a client that saw one flat list would render the + // wrong fields on one of the two dialogs. infos = append(infos, &types.ConnectorProviderInfo{ - Provider: provider, - DisplayName: reg.DisplayName, - DocumentationURL: documentationURL, - OauthConfigured: oauthConfigured, - APIKeySupported: apiKeySupported, - APIKeyManaged: apiKeyManaged, - ClientCredentialsSupported: clientCredentialsSupported, - Oauth2Scopes: scopes, - ExtraSettings: extraSettings, + Provider: provider, + DisplayName: reg.DisplayName, + DocumentationURL: documentationURL, + OauthConfigured: oauthConfigured, + APIKeySupported: apiKeySupported, + APIKeyManaged: apiKeyManaged, + ClientCredentialsSupported: clientCredentialsSupported, + Oauth2Scopes: scopes, + APIKeyExtraSettings: connectorProviderSettingInfos(reg.APIKeyExtraSettings), + ClientCredentialsExtraSettings: connectorProviderSettingInfos(reg.ClientCredentialsExtraSettings), }) } diff --git a/pkg/server/api/console/v1/connector_provider_info.go b/pkg/server/api/console/v1/connector_provider_info.go index 593b14271..dae3b636b 100644 --- a/pkg/server/api/console/v1/connector_provider_info.go +++ b/pkg/server/api/console/v1/connector_provider_info.go @@ -21,38 +21,19 @@ package console_v1 import ( - "go.probo.inc/probo/pkg/coredata" + "go.probo.inc/probo/pkg/connector/provider" "go.probo.inc/probo/pkg/server/api/console/v1/types" ) -func (r *Resolver) providerDisplayName(p coredata.ConnectorProvider) string { - return r.providerRegistry.ProviderDisplayName(p) -} +// connectorProviderSettingInfos projects one connect path's settings list onto +// the GraphQL type. A Registration declares one list per connect path, so +// AccessReviewDrivers calls this once per path. The result is never nil: both +// schema fields are non-null lists, and a provider with no settings on a path +// returns an empty one. +func connectorProviderSettingInfos(settings []provider.ExtraSetting) []*types.ConnectorProviderSettingInfo { + out := make([]*types.ConnectorProviderSettingInfo, 0, len(settings)) -func (r *Resolver) providerSupportsAPIKey(p coredata.ConnectorProvider) bool { - if reg, ok := r.providerRegistry.Get(p); ok { - return reg.SupportsAPIKey - } - - return false -} - -func (r *Resolver) providerSupportsClientCredentials(p coredata.ConnectorProvider) bool { - if reg, ok := r.providerRegistry.Get(p); ok { - return reg.SupportsClientCredentials - } - - return false -} - -func (r *Resolver) providerExtraSettings(p coredata.ConnectorProvider) []*types.ConnectorProviderSettingInfo { - reg, ok := r.providerRegistry.Get(p) - if !ok || len(reg.ExtraSettings) == 0 { - return []*types.ConnectorProviderSettingInfo{} - } - - out := make([]*types.ConnectorProviderSettingInfo, 0, len(reg.ExtraSettings)) - for _, s := range reg.ExtraSettings { + for _, s := range settings { out = append(out, &types.ConnectorProviderSettingInfo{ Key: s.Key, Label: s.Label, diff --git a/pkg/server/api/console/v1/connector_settings_test.go b/pkg/server/api/console/v1/connector_settings_test.go new file mode 100644 index 000000000..cdb634363 --- /dev/null +++ b/pkg/server/api/console/v1/connector_settings_test.go @@ -0,0 +1,125 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package console_v1 + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.probo.inc/probo/pkg/connector/provider" + "go.probo.inc/probo/pkg/coredata" + "go.probo.inc/probo/pkg/server/api/console/v1/types" +) + +// Langfuse and 1Password are the two providers whose declared settings failed +// to reach these resolvers: the console dropped Langfuse's Base URL, and +// 1Password's single flat list made the API-key dialog collect the +// client-credentials fields. Each test below walks the whole chain the console +// walks — the key a Registration declares, the mutation input field it is +// submitted as, the settings struct that is persisted — so a key renamed on one +// side and not the other fails here instead of at connect time. +func TestApiKeyConnectorSettings_LangfuseBaseURL(t *testing.T) { + t.Parallel() + + reg, ok := provider.NewBuiltinRegistry().Get(coredata.ConnectorProviderLangfuse) + require.True(t, ok) + require.Len(t, reg.APIKeyExtraSettings, 1) + require.Equal(t, "baseUrl", reg.APIKeyExtraSettings[0].Key) + + baseURL := "https://cloud.langfuse.com" + + raw, err := apiKeyConnectorSettings(types.CreateAPIKeyConnectorInput{ + Provider: coredata.ConnectorProviderLangfuse, + LangfuseBaseURL: &baseURL, + }) + require.NoError(t, err) + + var settings coredata.LangfuseConnectorSettings + require.NoError(t, json.Unmarshal(raw, &settings)) + assert.Equal(t, baseURL, settings.BaseURL) + + _, err = apiKeyConnectorSettings(types.CreateAPIKeyConnectorInput{ + Provider: coredata.ConnectorProviderLangfuse, + }) + require.Error(t, err) +} + +func TestApiKeyConnectorSettings_OnePasswordSCIMBridgeURL(t *testing.T) { + t.Parallel() + + reg, ok := provider.NewBuiltinRegistry().Get(coredata.ConnectorProviderOnePassword) + require.True(t, ok) + require.Len(t, reg.APIKeyExtraSettings, 1) + require.Equal(t, "scimBridgeUrl", reg.APIKeyExtraSettings[0].Key) + + scimBridgeURL := "https://scim.example.test" + + raw, err := apiKeyConnectorSettings(types.CreateAPIKeyConnectorInput{ + Provider: coredata.ConnectorProviderOnePassword, + OnePasswordScimBridgeURL: &scimBridgeURL, + }) + require.NoError(t, err) + + var settings coredata.OnePasswordConnectorSettings + require.NoError(t, json.Unmarshal(raw, &settings)) + assert.Equal(t, scimBridgeURL, settings.SCIMBridgeURL) + + // The old shared settings list made this dialog collect Account ID and + // Region instead, which CreateAPIKeyConnectorInput has no fields for at + // all: whatever the customer typed was dropped and the create failed here. + _, err = apiKeyConnectorSettings(types.CreateAPIKeyConnectorInput{ + Provider: coredata.ConnectorProviderOnePassword, + }) + require.Error(t, err) +} + +func TestClientCredentialsConnectorSettings_OnePassword(t *testing.T) { + t.Parallel() + + reg, ok := provider.NewBuiltinRegistry().Get(coredata.ConnectorProviderOnePassword) + require.True(t, ok) + require.Len(t, reg.ClientCredentialsExtraSettings, 2) + require.Equal(t, "accountId", reg.ClientCredentialsExtraSettings[0].Key) + require.Equal(t, "region", reg.ClientCredentialsExtraSettings[1].Key) + + accountID, region := "acme", "EU" + + raw, err := clientCredentialsConnectorSettings(types.CreateClientCredentialsConnectorInput{ + Provider: coredata.ConnectorProviderOnePassword, + OnePasswordAccountID: &accountID, + OnePasswordRegion: ®ion, + }) + require.NoError(t, err) + + var settings coredata.OnePasswordUsersAPISettings + require.NoError(t, json.Unmarshal(raw, &settings)) + assert.Equal(t, accountID, settings.AccountID) + assert.Equal(t, region, settings.Region) + + _, err = clientCredentialsConnectorSettings(types.CreateClientCredentialsConnectorInput{ + Provider: coredata.ConnectorProviderOnePassword, + OnePasswordAccountID: &accountID, + }) + require.Error(t, err) +} diff --git a/pkg/server/api/console/v1/graphql/connector.graphql b/pkg/server/api/console/v1/graphql/connector.graphql index 99dca7197..d8079d90c 100644 --- a/pkg/server/api/console/v1/graphql/connector.graphql +++ b/pkg/server/api/console/v1/graphql/connector.graphql @@ -130,7 +130,20 @@ type ConnectorProviderInfo { apiKeyManaged: Boolean! clientCredentialsSupported: Boolean! oauth2Scopes: [String!]! - extraSettings: [ConnectorProviderSettingInfo!]! + """ + apiKeyExtraSettings lists the per-provider settings the API-key connect + form must render and submit, in render order. Empty when the provider needs + none or has no API-key path. + """ + apiKeyExtraSettings: [ConnectorProviderSettingInfo!]! + """ + clientCredentialsExtraSettings lists the settings the client-credentials + connect form must render and submit. A provider offering both paths + (1Password) returns different fields here than in apiKeyExtraSettings, + because a different settings struct and driver sits behind each — so a + client cannot render one path's settings on the other. + """ + clientCredentialsExtraSettings: [ConnectorProviderSettingInfo!]! } type ConnectorProviderSettingInfo {