Default access source org across GraphQL and MCP

Org-defaulting for picker providers only ran in the GraphQL resolver,
so a picker-provider source created or updated through the MCP API
connected fine but resolved no users until the org was picked. Move
the defaulting into the accessreview service as
AutoSelectDefaultOrganization and call it from both surfaces, moving
the providerOrgConfigs picker dispatch alongside it (the three console
picker resolvers now dispatch through service accessors, behavior
unchanged).

Also harden the moved path: resolve the provider from cheap connector
metadata before building the authenticated HTTP client, so the ~50
non-picker providers no longer pay a decrypt/refresh/DB-write on every
create/update; bound the outbound ListOrgs call with a 10s timeout so
a hung provider cannot stall the mutation; and re-check inside the
ConfigureAccessReviewSource tx (OnlyIfUnset) so an org the user picks
while ListOrgs is in flight is not overwritten by the first listed
org.

Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>

pkg/accessreview/access_source_provider_config.go

@@ -18,7 +18,7 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.
 
-package console_v1
+package accessreview
 
 import (
 	"context"
@@ -45,10 +45,11 @@ type providerOrgConfig struct {
 	NeedsPicker  bool
 }
 
-// providerOrgConfigs is the single source of truth that the three
+// providerOrgConfigs is the single source of truth that the access-source
-// AccessReviewSource picker resolvers (ProviderOrganizations,
+// picker paths dispatch through: the console/MCP picker resolvers
-// SelectedOrganization, NeedsConfiguration) dispatch through. Adding a
+// (ProviderOrganizations, SelectedOrganization, NeedsConfiguration) and the
-// provider takes one entry here, not three switch arms.
+// AutoSelectDefaultOrganization defaulting run on create/update. Adding a
+// provider takes one entry here.
 var providerOrgConfigs = map[coredata.ConnectorProvider]providerOrgConfig{
 	coredata.ConnectorProviderGitHub: {
 		ListOrgs: drivers.ListGitHubOrganizations,
@@ -123,7 +124,7 @@ var providerOrgConfigs = map[coredata.ConnectorProvider]providerOrgConfig{
 		NeedsPicker: true,
 	},
 	// Pattern 2-auto: identifier is captured during the OAuth callback
-	// (subdomain for PagerDuty, team_id or fallback /v2/user.id for
+	// (subdomain for PagerDuty, teamId or fallback /v2/user.id for
 	// Vercel). No picker UI; NeedsPicker = false.
 	coredata.ConnectorProviderPagerDuty: {
 		SelectedSlug: func(c *coredata.Connector) string {

pkg/accessreview/source_service.go

@@ -22,11 +22,14 @@ package accessreview
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"time"
 
+	"go.gearno.de/kit/log"
 	"go.gearno.de/kit/pg"
+	"go.probo.inc/probo/pkg/accessreview/drivers"
 	"go.probo.inc/probo/pkg/connector"
 	"go.probo.inc/probo/pkg/coredata"
 	"go.probo.inc/probo/pkg/gid"
@@ -56,6 +59,12 @@ type (
 	ConfigureAccessReviewSourceRequest struct {
 		AccessReviewSourceID gid.GID
 		OrganizationSlug     string
+
+		// OnlyIfUnset makes the configure a no-op when the connector already
+		// has an org selected. AutoSelectDefaultOrganization sets it so a
+		// concurrent user pick made while ListOrgs was in flight is not
+		// silently overwritten by the first listed org.
+		OnlyIfUnset bool
 	}
 )
 
@@ -434,6 +443,15 @@ func (s *Service) ConfigureAccessReviewSource(
 				return fmt.Errorf("cannot load connector: %w", err)
 			}
 
+			// TOCTOU guard for the auto-default path: if the org was set (e.g.
+			// by a concurrent user pick) after the caller observed it as unset,
+			// leave the existing selection untouched.
+			if req.OnlyIfUnset {
+				if cfg, ok := providerOrgConfigs[dbConnector.Provider]; ok && cfg.SelectedSlug(dbConnector) != "" {
+					return nil
+				}
+			}
+
 			reg, ok := s.providerRegistry.Get(dbConnector.Provider)
 			if !ok || reg.SetOrganizationSettings == nil {
 				return fmt.Errorf("cannot configure access source: provider %s does not support organization configuration", dbConnector.Provider)
@@ -469,6 +487,196 @@ func (s *Service) ConfigureAccessReviewSource(
 	return source, nil
 }
 
+// loadConnectorMetadata loads a connector's metadata (provider, settings)
+// without decrypting the connection. The raw ErrResourceNotFound is
+// propagated so callers can decide how to treat a missing connector.
+func (s *Service) loadConnectorMetadata(
+	ctx context.Context,
+	scope coredata.Scoper,
+	connectorID gid.GID,
+) (*coredata.Connector, error) {
+	dbConnector := &coredata.Connector{}
+
+	err := s.pg.WithConn(
+		ctx,
+		func(ctx context.Context, conn pg.Querier) error {
+			return dbConnector.LoadMetadataByID(ctx, conn, scope, connectorID)
+		},
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	return dbConnector, nil
+}
+
+// ProviderOrganizations lists the orgs/workspaces the connector backing the
+// source can be scoped to, for the picker UI. Returns an empty list when the
+// connector is gone or the provider has no picker.
+func (s *Service) ProviderOrganizations(
+	ctx context.Context,
+	scope coredata.Scoper,
+	connectorID gid.GID,
+) ([]drivers.Organization, error) {
+	httpClient, dbConnector, err := s.ConnectorHTTPClient(ctx, scope, connectorID)
+	if err != nil {
+		if errors.Is(err, coredata.ErrResourceNotFound) {
+			return nil, nil
+		}
+
+		return nil, fmt.Errorf("cannot get connector HTTP client: %w", err)
+	}
+
+	cfg, ok := providerOrgConfigs[dbConnector.Provider]
+	if !ok || cfg.ListOrgs == nil {
+		return nil, nil
+	}
+
+	orgs, err := cfg.ListOrgs(ctx, httpClient)
+	if err != nil {
+		return nil, err
+	}
+
+	return orgs, nil
+}
+
+// SelectedOrganizationSlug returns the org identifier currently configured on
+// the connector backing the source, or "" when none is set or the provider
+// has no picker. ErrResourceNotFound is propagated for a missing connector.
+func (s *Service) SelectedOrganizationSlug(
+	ctx context.Context,
+	scope coredata.Scoper,
+	connectorID gid.GID,
+) (string, error) {
+	dbConnector, err := s.loadConnectorMetadata(ctx, scope, connectorID)
+	if err != nil {
+		return "", err
+	}
+
+	cfg, ok := providerOrgConfigs[dbConnector.Provider]
+	if !ok {
+		return "", nil
+	}
+
+	return cfg.SelectedSlug(dbConnector), nil
+}
+
+// SourceNeedsConfiguration reports whether the connector backing the source
+// has a picker UI and no org selected yet. ErrResourceNotFound is propagated
+// for a missing connector.
+func (s *Service) SourceNeedsConfiguration(
+	ctx context.Context,
+	scope coredata.Scoper,
+	connectorID gid.GID,
+) (bool, error) {
+	dbConnector, err := s.loadConnectorMetadata(ctx, scope, connectorID)
+	if err != nil {
+		return false, err
+	}
+
+	cfg, ok := providerOrgConfigs[dbConnector.Provider]
+	if !ok || !cfg.NeedsPicker {
+		return false, nil
+	}
+
+	return cfg.SelectedSlug(dbConnector) == "", nil
+}
+
+// AutoSelectDefaultOrganization picks the first workspace/org the connector
+// can see for a freshly linked picker-provider source that has none selected
+// yet. Without it a connected source stays "needs configuration" until the
+// user completes the picker; if they skip it, the first campaign silently
+// resolves no users (the driver requires an org). Defaulting to the first
+// available makes the source immediately usable; the picker stays available
+// to switch when several are listed.
+//
+// Best-effort: any failure (provider unreachable, nothing listed) leaves the
+// source in its existing "needs configuration" state, where the picker is the
+// fallback. It never returns an error and must not fail the create/update
+// that triggered it.
+func (s *Service) AutoSelectDefaultOrganization(
+	ctx context.Context,
+	scope coredata.Scoper,
+	source *coredata.AccessReviewSource,
+) {
+	if source == nil || source.ConnectorID == nil {
+		return
+	}
+
+	// Resolve the provider from cheap metadata first: only picker providers
+	// that still need defaulting should pay for the connector decrypt, token
+	// refresh, and HTTP-client build below (all ~50 other providers skip it).
+	dbMeta, err := s.loadConnectorMetadata(ctx, scope, *source.ConnectorID)
+	if err != nil {
+		// A missing connector is not worth logging: the picker simply never
+		// surfaces a default.
+		if !errors.Is(err, coredata.ErrResourceNotFound) {
+			s.logger.WarnCtx(ctx, "cannot load connector metadata for default organization", log.Error(err))
+		}
+
+		return
+	}
+
+	cfg, ok := providerOrgConfigs[dbMeta.Provider]
+	if !ok || !cfg.NeedsPicker || cfg.ListOrgs == nil {
+		return
+	}
+
+	// Never override an org the user (or an earlier default) already picked.
+	if cfg.SelectedSlug(dbMeta) != "" {
+		return
+	}
+
+	httpClient, dbConnector, err := s.ConnectorHTTPClient(ctx, scope, *source.ConnectorID)
+	if err != nil {
+		if !errors.Is(err, coredata.ErrResourceNotFound) {
+			s.logger.WarnCtx(ctx, "cannot load connector for default organization", log.Error(err))
+		}
+
+		return
+	}
+
+	// Bound the outbound provider call so a hung provider cannot stall the
+	// create/update mutation that triggered the defaulting.
+	listCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
+	defer cancel()
+
+	orgs, err := cfg.ListOrgs(listCtx, httpClient)
+	if err != nil {
+		s.logger.WarnCtx(
+			ctx,
+			"cannot list provider organizations for default selection",
+			log.String("provider", dbConnector.Provider.String()),
+			log.Error(err),
+		)
+
+		return
+	}
+
+	if len(orgs) == 0 {
+		return
+	}
+
+	// OnlyIfUnset guards against a user picking an org while ListOrgs was in
+	// flight: the configure re-checks inside its tx and does not overwrite.
+	if _, err := s.ConfigureAccessReviewSource(
+		ctx,
+		scope,
+		ConfigureAccessReviewSourceRequest{
+			AccessReviewSourceID: source.ID,
+			OrganizationSlug:     orgs[0].Slug,
+			OnlyIfUnset:          true,
+		},
+	); err != nil {
+		s.logger.WarnCtx(
+			ctx,
+			"cannot apply default provider organization",
+			log.String("provider", dbConnector.Provider.String()),
+			log.Error(err),
+		)
+	}
+}
+
 // ResetSourceNameSyncForConnector clears the synced-name flag on every access
 // source backed by connectorID so the source-name worker re-resolves the
 // display name. Called after a connector is reconnected — the new grant may

pkg/server/api/console/v1/access_review_campaign_resolvers.go

@@ -448,21 +448,7 @@ func (r *accessReviewSourceResolver) ProviderOrganizations(ctx context.Context,
 		return []*types.ProviderOrganization{}, nil
 	}
 
-	httpClient, dbConnector, err := r.accessReview.ConnectorHTTPClient(ctx, scope, *obj.ConnectorID)
+	orgs, err := r.accessReview.ProviderOrganizations(ctx, scope, *obj.ConnectorID)
-	if err != nil {
-		if errors.Is(err, coredata.ErrResourceNotFound) {
-			return []*types.ProviderOrganization{}, nil
-		}
-
-		return nil, fmt.Errorf("cannot get connector HTTP client: %w", err)
-	}
-
-	cfg, ok := providerOrgConfigs[dbConnector.Provider]
-	if !ok || cfg.ListOrgs == nil {
-		return []*types.ProviderOrganization{}, nil
-	}
-
-	orgs, err := cfg.ListOrgs(ctx, httpClient)
 	if err != nil {
 		return nil, err
 	}
@@ -491,23 +477,18 @@ func (r *accessReviewSourceResolver) NeedsConfiguration(ctx context.Context, obj
 		return false, nil
 	}
 
-	dbConnector, err := r.probo.Connectors.Get(ctx, scope, *obj.ConnectorID)
+	needsConfiguration, err := r.accessReview.SourceNeedsConfiguration(ctx, scope, *obj.ConnectorID)
 	if err != nil {
 		if errors.Is(err, coredata.ErrResourceNotFound) {
 			return false, nil
 		}
 
-		r.logger.ErrorCtx(ctx, "cannot get connector", log.Error(err))
+		r.logger.ErrorCtx(ctx, "cannot determine access source configuration", log.Error(err))
 
 		return false, gqlutils.Internal(ctx)
 	}
 
-	cfg, ok := providerOrgConfigs[dbConnector.Provider]
+	return needsConfiguration, nil
-	if !ok || !cfg.NeedsPicker {
-		return false, nil
-	}
-
-	return cfg.SelectedSlug(dbConnector) == "", nil
 }
 
 // ConnectionStatus is the resolver for the connectionStatus field.
@@ -552,23 +533,17 @@ func (r *accessReviewSourceResolver) SelectedOrganization(ctx context.Context, o
 		return nil, nil
 	}
 
-	dbConnector, err := r.probo.Connectors.Get(ctx, scope, *obj.ConnectorID)
+	slug, err := r.accessReview.SelectedOrganizationSlug(ctx, scope, *obj.ConnectorID)
 	if err != nil {
 		if errors.Is(err, coredata.ErrResourceNotFound) {
 			return nil, nil
 		}
 
-		r.logger.ErrorCtx(ctx, "cannot get connector", log.Error(err))
+		r.logger.ErrorCtx(ctx, "cannot get selected organization", log.Error(err))
 
 		return nil, gqlutils.Internal(ctx)
 	}
 
-	cfg, ok := providerOrgConfigs[dbConnector.Provider]
-	if !ok {
-		return nil, nil
-	}
-
-	slug := cfg.SelectedSlug(dbConnector)
 	if slug == "" {
 		return nil, nil
 	}
@@ -628,7 +603,7 @@ func (r *mutationResolver) CreateAccessReviewSource(ctx context.Context, input t
 		return nil, gqlutils.Internal(ctx)
 	}
 
-	r.autoSelectDefaultOrganization(ctx, scope, source)
+	r.accessReview.AutoSelectDefaultOrganization(ctx, scope, source)
 
 	return &types.CreateAccessReviewSourcePayload{
 		AccessReviewSourceEdge: types.NewAccessReviewSourceEdge(source, coredata.AccessReviewSourceOrderFieldCreatedAt),
@@ -666,7 +641,7 @@ func (r *mutationResolver) UpdateAccessReviewSource(ctx context.Context, input t
 	// right away. Skipped on name/CSV-only updates to avoid a needless
 	// provider round-trip.
 	if input.ConnectorID.IsSet() {
-		r.autoSelectDefaultOrganization(ctx, scope, source)
+		r.accessReview.AutoSelectDefaultOrganization(ctx, scope, source)
 	}
 
 	return &types.UpdateAccessReviewSourcePayload{
@@ -726,80 +701,6 @@ func (r *mutationResolver) ConfigureAccessReviewSource(ctx context.Context, inpu
 	}, nil
 }
 
-// autoSelectDefaultOrganization picks the first workspace/org the connector can
-// see for a freshly linked picker-provider source that has none selected yet.
-// Without it a connected source stays "needs configuration" until the user
-// completes the picker step; if they skip it, the first campaign silently
-// resolves no users (the driver requires an org). Defaulting to the first
-// available makes the source immediately usable; the picker UI stays available
-// to switch when several are listed.
-//
-// Best-effort: any failure (provider unreachable, nothing listed) leaves the
-// source in its existing "needs configuration" state, where the picker is the
-// fallback. It never fails the create/update mutation that triggered it.
-func (r *mutationResolver) autoSelectDefaultOrganization(
-	ctx context.Context,
-	scope coredata.Scoper,
-	source *coredata.AccessReviewSource,
-) {
-	if source == nil || source.ConnectorID == nil {
-		return
-	}
-
-	httpClient, dbConnector, err := r.accessReview.ConnectorHTTPClient(ctx, scope, *source.ConnectorID)
-	if err != nil {
-		// A missing connector is not an error worth logging: the picker
-		// simply never surfaces a default.
-		if !errors.Is(err, coredata.ErrResourceNotFound) {
-			r.logger.WarnCtx(ctx, "cannot load connector for default organization", log.Error(err))
-		}
-
-		return
-	}
-
-	cfg, ok := providerOrgConfigs[dbConnector.Provider]
-	if !ok || !cfg.NeedsPicker || cfg.ListOrgs == nil {
-		return
-	}
-
-	// Never override an org the user (or an earlier default) already picked.
-	if cfg.SelectedSlug(dbConnector) != "" {
-		return
-	}
-
-	orgs, err := cfg.ListOrgs(ctx, httpClient)
-	if err != nil {
-		r.logger.WarnCtx(
-			ctx,
-			"cannot list provider organizations for default selection",
-			log.String("provider", dbConnector.Provider.String()),
-			log.Error(err),
-		)
-
-		return
-	}
-
-	if len(orgs) == 0 {
-		return
-	}
-
-	if _, err := r.accessReview.ConfigureAccessReviewSource(
-		ctx,
-		scope,
-		accessreview.ConfigureAccessReviewSourceRequest{
-			AccessReviewSourceID: source.ID,
-			OrganizationSlug:     orgs[0].Slug,
-		},
-	); err != nil {
-		r.logger.WarnCtx(
-			ctx,
-			"cannot apply default provider organization",
-			log.String("provider", dbConnector.Provider.String()),
-			log.Error(err),
-		)
-	}
-}
-
 // CreateAccessReviewCampaign is the resolver for the createAccessReviewCampaign field.
 func (r *mutationResolver) CreateAccessReviewCampaign(ctx context.Context, input types.CreateAccessReviewCampaignInput) (*types.CreateAccessReviewCampaignPayload, error) {
 	scope, err := r.authorize(ctx, input.OrganizationID, accessreview.ActionCampaignCreate)

pkg/server/api/mcp/v1/schema.resolvers.go

@@ -3679,6 +3679,8 @@ func (r *Resolver) CreateAccessReviewSourceTool(ctx context.Context, req *mcp.Ca
 		return nil, types.CreateAccessReviewSourceOutput{}, fmt.Errorf("cannot create access source: %w", err)
 	}
 
+	r.accessReview.AutoSelectDefaultOrganization(ctx, scope, source)
+
 	return nil, types.CreateAccessReviewSourceOutput{
 		AccessReviewSource: types.NewAccessReviewSource(source),
 	}, nil
@@ -3700,7 +3702,11 @@ func (r *Resolver) UpdateAccessReviewSourceTool(ctx context.Context, req *mcp.Ca
 		updateReq.Name = &input.Name
 	}
 
+	connectorSet := false
+
 	if rawConnectorID := UnwrapOmittable(input.ConnectorID); rawConnectorID != nil {
+		connectorSet = true
+
 		if *rawConnectorID != nil {
 			id, err := gid.ParseGID(**rawConnectorID)
 			if err != nil {
@@ -3725,6 +3731,13 @@ func (r *Resolver) UpdateAccessReviewSourceTool(ctx context.Context, req *mcp.Ca
 		return nil, types.UpdateAccessReviewSourceOutput{}, fmt.Errorf("cannot update access source: %w", err)
 	}
 
+	// A connector was just (re)linked: default its org so the source is
+	// usable right away. Matches the GraphQL surface; skipped on name/CSV-only
+	// updates to avoid a needless provider round-trip.
+	if connectorSet {
+		r.accessReview.AutoSelectDefaultOrganization(ctx, scope, source)
+	}
+
 	return nil, types.UpdateAccessReviewSourceOutput{
 		AccessReviewSource: types.NewAccessReviewSource(source),
 	}, nil