diff --git a/pkg/server/api/console/v1/access_source_provider_config.go b/pkg/accessreview/access_source_provider_config.go similarity index 93% rename from pkg/server/api/console/v1/access_source_provider_config.go rename to pkg/accessreview/access_source_provider_config.go index 8c90aea72..30f8444e3 100644 --- a/pkg/server/api/console/v1/access_source_provider_config.go +++ b/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 -// AccessReviewSource picker resolvers (ProviderOrganizations, -// SelectedOrganization, NeedsConfiguration) dispatch through. Adding a -// provider takes one entry here, not three switch arms. +// providerOrgConfigs is the single source of truth that the access-source +// picker paths dispatch through: the console/MCP picker resolvers +// (ProviderOrganizations, SelectedOrganization, NeedsConfiguration) and the +// 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 { diff --git a/pkg/accessreview/source_service.go b/pkg/accessreview/source_service.go index e147d2312..aa41056c1 100644 --- a/pkg/accessreview/source_service.go +++ b/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 diff --git a/pkg/server/api/console/v1/access_review_campaign_resolvers.go b/pkg/server/api/console/v1/access_review_campaign_resolvers.go index 5c786e01a..9f1411bce 100644 --- a/pkg/server/api/console/v1/access_review_campaign_resolvers.go +++ b/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) - 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) + orgs, err := r.accessReview.ProviderOrganizations(ctx, scope, *obj.ConnectorID) 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] - if !ok || !cfg.NeedsPicker { - return false, nil - } - - return cfg.SelectedSlug(dbConnector) == "", nil + return needsConfiguration, 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) diff --git a/pkg/server/api/mcp/v1/schema.resolvers.go b/pkg/server/api/mcp/v1/schema.resolvers.go index 16aadf7a3..6efcde65a 100644 --- a/pkg/server/api/mcp/v1/schema.resolvers.go +++ b/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