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 {