rohit/probo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Refine connector provider registry per review

Browse Source 
Three follow-ups from review of the registry consolidation:

- Build Vercel's authorization URL with net/url instead of a
  hand-rolled "{integration_slug}" placeholder resolved by
  strings.ReplaceAll. The slug is escaped via url.PathEscape in a
  per-provider Registration.BuildAuthURL closure, and the unused
  AuthURLParams plumbing on Registration and OAuth2Connector is
  removed (OAuth2Connector now carries a typed IntegrationSlug).

- Drop the SettingsInput union type and the per-provider
  MarshalSettings closures. The create resolvers now build the typed
  coredata.*ConnectorSettings directly from the gqlgen input, the
  same way the OAuth callback path already does, so there is no
  shared catch-all DTO and no stringly-typed boundary.

- Restore ConnectorProviders() to a plain ordered slice literal; the
  intermediate map + slices.Sort added nondeterminism and a sort for
  no benefit.

Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
This commit is contained in:
Aurélien Sibiril
2026-05-26 15:15:43 +02:00
parent e18ecdda8b
commit a5259fc978
16 changed files with 220 additions and 226 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
pkg/connector/oauth2.go
View File

@@ -57,12 +57,13 @@ type (
// to the authorize URL; CompleteWithState replays the verifier
// on the token exchange.
RequiresPKCE bool
// AuthURLParams are operator-supplied placeholders substituted
// into the static provider AuthURL by
// (*provider.Registry).ApplyOAuth2Defaults (for example
// Vercel's "{integration_slug}"). Empty for the vast majority
// of providers.
AuthURLParams map[string]string
// IntegrationSlug is an operator-supplied identifier used by
// providers whose authorization URL embeds it as a path segment
// (Vercel-style integrations). It is consumed by the provider's
// Registration.BuildAuthURL in
// (*provider.Registry).ApplyOAuth2Defaults. Empty for the vast
// majority of providers.
IntegrationSlug string
// HTTPClient is used for the OAuth2 token-exchange request
// issued from CompleteWithState. It must be set by callers;

27
pkg/connector/provider/apply.go
View File

@@ -15,8 +15,8 @@
package provider
import (
"fmt"
"maps"
"strings"
"go.gearno.de/kit/httpclient"
"go.probo.inc/probo/pkg/connector"
@@ -30,16 +30,16 @@ import (
// pulled from r; only ClientID and ClientSecret come from deployment
// config.
//
// Operator-supplied placeholders in the static AuthURL (e.g. Vercel's
// "{integration_slug}") are substituted from c.AuthURLParams; the
// substitution is a no-op when no placeholders are configured.
func (r *Registry) ApplyOAuth2Defaults(p string, redirectURI string, c *connector.OAuth2Connector) {
// Providers whose authorization URL embeds an operator-supplied slug
// (e.g. Vercel) derive it via Registration.BuildAuthURL from
// c.IntegrationSlug; this is a no-op when no slug is configured.
func (r *Registry) ApplyOAuth2Defaults(p string, redirectURI string, c *connector.OAuth2Connector) error {
c.RedirectURI = redirectURI
c.HTTPClient = httpclient.DefaultClient(httpclient.WithSSRFProtection())
reg, ok := r.Get(coredata.ConnectorProvider(p))
if !ok {
return
return nil
}
c.AuthURL = reg.AuthURL
@@ -57,13 +57,16 @@ func (r *Registry) ApplyOAuth2Defaults(p string, redirectURI string, c *connecto
c.ExtraAuthParams = extra
}
// Resolve operator-supplied placeholders in the static AuthURL
// (for example Vercel's "{integration_slug}"). Providers without
// placeholders are unaffected; the loop is a no-op when
// AuthURLParams is empty.
for k, v := range c.AuthURLParams {
c.AuthURL = strings.ReplaceAll(c.AuthURL, "{"+k+"}", v)
if reg.BuildAuthURL != nil && c.IntegrationSlug != "" {
authURL, err := reg.BuildAuthURL(c.IntegrationSlug)
if err != nil {
return fmt.Errorf("cannot build %s auth URL: %w", p, err)
}
c.AuthURL = authURL
}
return nil
}
// ProbeURL returns the registered probe URL for provider p, or the

54
pkg/connector/provider/apply_test.go
View File

@@ -18,39 +18,51 @@ import (
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.probo.inc/probo/pkg/connector"
"go.probo.inc/probo/pkg/connector/provider"
)
// TestApplyOAuth2Defaults_AuthURLTemplating verifies that operator-supplied
// AuthURLParams (for example Vercel's "{integration_slug}") are substituted
// into the static provider AuthURL when the connector is initialized.
// Providers without placeholders are unaffected.
func TestApplyOAuth2Defaults_AuthURLTemplating(t *testing.T) {
// TestApplyOAuth2Defaults_AuthURLFromSlug verifies that providers whose
// authorization URL embeds an operator-supplied slug (Vercel) build it
// from c.IntegrationSlug via Registration.BuildAuthURL, with the slug
// percent-escaped. Providers without a BuildAuthURL are unaffected.
func TestApplyOAuth2Defaults_AuthURLFromSlug(t *testing.T) {
t.Parallel()
t.Run("placeholder is substituted when AuthURLParams is supplied", func(t *testing.T) {
t.Run("auth URL is built when an integration slug is supplied", func(t *testing.T) {
t.Parallel()
r := provider.NewBuiltinRegistry()
c := &connector.OAuth2Connector{
ClientID: "id",
ClientSecret: "secret",
AuthURLParams: map[string]string{
"integration_slug": "acme",
},
ClientID: "id",
ClientSecret: "secret",
IntegrationSlug: "acme",
}
// VERCEL uses a templated AuthURL with the
// "{integration_slug}" placeholder.
r.ApplyOAuth2Defaults("VERCEL", "https://example.com/cb", c)
require.NoError(t, r.ApplyOAuth2Defaults("VERCEL", "https://example.com/cb", c))
assert.Equal(t, "https://vercel.com/integrations/acme/new", c.AuthURL)
assert.Equal(t, "https://api.vercel.com/v2/oauth/access_token", c.TokenURL)
})
t.Run("placeholder remains literal when AuthURLParams is empty", func(t *testing.T) {
t.Run("slug with reserved characters is percent-escaped", func(t *testing.T) {
t.Parallel()
r := provider.NewBuiltinRegistry()
c := &connector.OAuth2Connector{
ClientID: "id",
ClientSecret: "secret",
IntegrationSlug: "a/b c",
}
require.NoError(t, r.ApplyOAuth2Defaults("VERCEL", "https://example.com/cb", c))
assert.Equal(t, "https://vercel.com/integrations/a%2Fb%20c/new", c.AuthURL)
})
t.Run("auth URL is empty when no integration slug is supplied", func(t *testing.T) {
t.Parallel()
r := provider.NewBuiltinRegistry()
@@ -59,12 +71,12 @@ func TestApplyOAuth2Defaults_AuthURLTemplating(t *testing.T) {
ClientSecret: "secret",
}
r.ApplyOAuth2Defaults("VERCEL", "https://example.com/cb", c)
require.NoError(t, r.ApplyOAuth2Defaults("VERCEL", "https://example.com/cb", c))
// No substitution requested; the placeholder is preserved
// verbatim so a misconfiguration is visible at the
// authorization step rather than silently masked.
assert.Equal(t, "https://vercel.com/integrations/{integration_slug}/new", c.AuthURL)
// Vercel has no static AuthURL; without a slug there is nothing
// to build, so the misconfiguration surfaces at the
// authorization step rather than being silently masked.
assert.Empty(t, c.AuthURL)
})
}
@@ -80,7 +92,7 @@ func TestApplyOAuth2Defaults_PKCEDefaults(t *testing.T) {
r := provider.NewBuiltinRegistry()
c := &connector.OAuth2Connector{ClientID: "id", ClientSecret: "secret"}
r.ApplyOAuth2Defaults(p, "https://example.com/cb", c)
require.NoError(t, r.ApplyOAuth2Defaults(p, "https://example.com/cb", c))
assert.True(t, c.RequiresPKCE,
"provider %s must enable PKCE so Initiate generates a verifier", p)
})

8
pkg/connector/provider/github.go
View File

@@ -16,7 +16,6 @@ package provider
import (
"context"
"encoding/json"
"fmt"
"net/http"
@@ -37,13 +36,6 @@ func githubRegistration() *Registration {
ExtraSettings: []ExtraSetting{
{Key: "organization", Label: "Organization", Required: true},
},
MarshalSettings: func(in *SettingsInput) (json.RawMessage, error) {
if in == nil || in.GitHubOrganization == nil || *in.GitHubOrganization == "" {
return nil, fmt.Errorf("cannot create github connector: githubOrganization is required")
}
return json.Marshal(&coredata.GitHubConnectorSettings{Organization: *in.GitHubOrganization})
},
NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, logger *log.Logger) (drivers.Driver, error) {
s, err := coredata.ConnectorSettings[coredata.GitHubConnectorSettings](conn)
if err != nil {

36
pkg/connector/provider/one_password.go
View File

@@ -16,10 +16,8 @@ package provider
import (
"context"
"encoding/json"
"fmt"
"net/http"
"net/url"
"go.gearno.de/kit/log"
"go.probo.inc/probo/pkg/accessreview/drivers"
@@ -41,38 +39,8 @@ func onePasswordRegistration() *Registration {
// 1Password has two settings shapes selected by protocol:
// - Client-credentials: AccountID + Region (Users API driver).
// - API key: SCIMBridgeURL (SCIM-bridge driver).
// MarshalSettings picks the shape based on which input fields
// are populated. The resolvers ensure that only one path is
// possible for any given request.
MarshalSettings: func(in *SettingsInput) (json.RawMessage, error) {
if in == nil {
return nil, nil
}
if in.OnePasswordAccountID != nil && in.OnePasswordRegion != nil {
if *in.OnePasswordAccountID == "" || *in.OnePasswordRegion == "" {
return nil, fmt.Errorf("cannot create 1password connector: onePasswordAccountId and onePasswordRegion must be non-empty")
}
return json.Marshal(&coredata.OnePasswordUsersAPISettings{
AccountID: *in.OnePasswordAccountID,
Region: *in.OnePasswordRegion,
})
}
if in.OnePasswordSCIMBridgeURL != nil && *in.OnePasswordSCIMBridgeURL != "" {
u, err := url.Parse(*in.OnePasswordSCIMBridgeURL)
if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
return nil, fmt.Errorf("cannot create 1password connector: onePasswordScimBridgeURL must be an http(s) URL")
}
return json.Marshal(&coredata.OnePasswordConnectorSettings{
SCIMBridgeURL: *in.OnePasswordSCIMBridgeURL,
})
}
return nil, nil
},
// 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.

8
pkg/connector/provider/sentry.go
View File

@@ -16,7 +16,6 @@ package provider
import (
"context"
"encoding/json"
"fmt"
"net/http"
@@ -37,13 +36,6 @@ func sentryRegistration() *Registration {
ExtraSettings: []ExtraSetting{
{Key: "organizationSlug", Label: "Organization Slug", Required: true},
},
MarshalSettings: func(in *SettingsInput) (json.RawMessage, error) {
if in == nil || in.SentryOrganizationSlug == nil || *in.SentryOrganizationSlug == "" {
return nil, fmt.Errorf("cannot create sentry connector: sentryOrganizationSlug is required")
}
return json.Marshal(&coredata.SentryConnectorSettings{OrganizationSlug: *in.SentryOrganizationSlug})
},
NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) {
s, err := coredata.ConnectorSettings[coredata.SentryConnectorSettings](conn)
if err != nil {

8
pkg/connector/provider/supabase.go
View File

@@ -16,7 +16,6 @@ package provider
import (
"context"
"encoding/json"
"fmt"
"net/http"
@@ -34,13 +33,6 @@ func supabaseRegistration() *Registration {
ExtraSettings: []ExtraSetting{
{Key: "organizationSlug", Label: "Organization Slug", Required: true},
},
MarshalSettings: func(in *SettingsInput) (json.RawMessage, error) {
if in == nil || in.SupabaseOrganizationSlug == nil || *in.SupabaseOrganizationSlug == "" {
return nil, fmt.Errorf("cannot create supabase connector: supabaseOrganizationSlug is required")
}
return json.Marshal(&coredata.SupabaseConnectorSettings{OrganizationSlug: *in.SupabaseOrganizationSlug})
},
NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) {
s, err := coredata.ConnectorSettings[coredata.SupabaseConnectorSettings](conn)
if err != nil {

8
pkg/connector/provider/tally.go
View File

@@ -16,7 +16,6 @@ package provider
import (
"context"
"encoding/json"
"fmt"
"net/http"
@@ -34,13 +33,6 @@ func tallyRegistration() *Registration {
ExtraSettings: []ExtraSetting{
{Key: "organizationId", Label: "Organization ID", Required: true},
},
MarshalSettings: func(in *SettingsInput) (json.RawMessage, error) {
if in == nil || in.TallyOrganizationID == nil || *in.TallyOrganizationID == "" {
return nil, fmt.Errorf("cannot create tally connector: tallyOrganizationId is required")
}
return json.Marshal(&coredata.TallyConnectorSettings{OrganizationID: *in.TallyOrganizationID})
},
NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) {
s, err := coredata.ConnectorSettings[coredata.TallyConnectorSettings](conn)
if err != nil {

39
pkg/connector/provider/types.go
View File

@@ -16,7 +16,6 @@ package provider
import (
"context"
"encoding/json"
"net/http"
"go.gearno.de/kit/log"
@@ -47,10 +46,11 @@ type Registration struct {
// request and replays the verifier on the token exchange. Default
// false; non-PKCE providers are unaffected.
RequiresPKCE bool
// AuthURLParams are operator-supplied placeholders substituted
// into the static provider AuthURL (e.g. Vercel's
// "{integration_slug}"). Empty for the vast majority of providers.
AuthURLParams map[string]string
// BuildAuthURL derives the authorization URL from an operator-supplied
// integration slug, for providers (e.g. Vercel) whose AuthURL embeds
// it as a path segment. It must construct the URL with net/url and
// escape the slug. Nil for providers with a fully static AuthURL.
BuildAuthURL func(slug string) (string, error)
// Protocol support / GraphQL surface.
SupportsAPIKey bool
@@ -61,35 +61,6 @@ type Registration struct {
NewDriver func(context.Context, *http.Client, *coredata.Connector, *log.Logger) (drivers.Driver, error)
NewNameResolver func(context.Context, *http.Client, *coredata.Connector, *log.Logger) drivers.NameResolver
SetOrganizationSettings func(*coredata.Connector, string) error
// MarshalSettings normalises the per-provider extra settings into
// the JSON blob persisted on coredata.Connector.RawSettings.
//
// SECURITY CONTRACT: returned errors are surfaced verbatim to the
// client via gqlutils.Invalid. They must contain only field names
// and structural information — never user-supplied values,
// secrets, or driver-internal details. Use a static string per
// validation failure ("sentryOrganizationSlug is required",
// "onePasswordRegion must be one of …"), never interpolate input.
MarshalSettings func(*SettingsInput) (json.RawMessage, error)
}
// SettingsInput is the union of every optional per-provider field
// available on the GraphQL CreateAPIKeyConnectorInput and
// CreateClientCredentialsConnectorInput types. The resolver populates
// it once from the gqlgen input; each provider's MarshalSettings
// reads only the fields it cares about.
//
// Adding a new provider with extra settings: add the optional field
// here + the corresponding optional field on the GraphQL input + the
// read in the per-provider MarshalSettings closure.
type SettingsInput struct {
TallyOrganizationID *string
SentryOrganizationSlug *string
SupabaseOrganizationSlug *string
GitHubOrganization *string
OnePasswordSCIMBridgeURL *string
OnePasswordAccountID *string
OnePasswordRegion *string
}
// ExtraSetting describes one extra per-provider settings field

21
pkg/connector/provider/vercel.go
View File

@@ -18,6 +18,7 @@ import (
"context"
"fmt"
"net/http"
"net/url"
"go.gearno.de/kit/log"
"go.probo.inc/probo/pkg/accessreview/drivers"
@@ -25,17 +26,25 @@ import (
)
func vercelRegistration() *Registration {
// Vercel uses a templated AuthURL: the operator supplies an
// `integration-slug` config field which is resolved into the
// "{integration_slug}" placeholder by ApplyOAuth2Defaults.
// Vercel does not use OAuth scopes — capabilities are pinned on
// the integration registration in the Vercel dashboard.
// Vercel's authorization URL embeds the operator's integration slug
// as a path segment; the operator supplies it via the
// `integration-slug` config field. BuildAuthURL constructs the URL
// with net/url so the slug is escaped. Vercel does not use OAuth
// scopes — capabilities are pinned on the integration registration
// in the Vercel dashboard.
return &Registration{
Provider: coredata.ConnectorProviderVercel,
DisplayName: "Vercel",
AuthURL: "https://vercel.com/integrations/{integration_slug}/new",
TokenURL: "https://api.vercel.com/v2/oauth/access_token",
ProbeURL: "https://api.vercel.com/v2/user",
BuildAuthURL: func(slug string) (string, error) {
u, err := url.JoinPath("https://vercel.com/integrations", url.PathEscape(slug), "new")
if err != nil {
return "", fmt.Errorf("cannot build vercel auth URL: %w", err)
}
return u, nil
},
NewDriver: func(_ context.Context, c *http.Client, conn *coredata.Connector, _ *log.Logger) (drivers.Driver, error) {
s, err := coredata.ConnectorSettings[coredata.VercelConnectorSettings](conn)
if err != nil {