diff --git a/pkg/config/config.go b/pkg/config/config.go index 2b9c5e63..a5fe3bae 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -70,7 +70,10 @@ var ( field.WithDisplayName("Optimize sync for large organizations"), field.WithDescription( "Reduces API calls by using grant expansion for team-based repo access "+ - "and skipping per-team detail fetches. Recommended for large orgs.", + "and skipping per-team detail fetches. Recommended for large orgs. "+ + "Requires the credential to be able to read the org's default repository permission "+ + "(admin:org scope or the Organization Administration read permission); "+ + "validation and syncs fail without it.", ), ) orgField = field.StringField( diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 2cdb5a62..c9e35524 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -217,7 +217,7 @@ func (gh *GitHub) Validate(ctx context.Context) (annotations.Annotations, error) } } - adminFound := false + firstAdminOrg := "" for _, o := range orgLogins { membership, _, err := gh.client.Organizations.GetOrgMembership(ctx, "", o) if err != nil { @@ -237,14 +237,20 @@ func (gh *GitHub) Validate(ctx context.Context) (annotations.Annotations, error) continue } - adminFound = true + if firstAdminOrg == "" { + firstAdminOrg = o + } } - if !adminFound { + if firstAdminOrg == "" { err := fmt.Errorf("access token must be an admin on at least one organization") return nil, uhttp.WrapErrors(codes.PermissionDenied, "github-connector: credentials validation failed", err) } + if err := gh.validateBasePermissionVisibility(ctx, firstAdminOrg); err != nil { + return nil, err + } + if len(gh.enterprises) > 0 { l := ctxzap.Extract(ctx) _, _, err := gh.customClient.ListEnterpriseConsumedLicenses(ctx, gh.enterprises[0], 1) @@ -268,6 +274,10 @@ func (gh *GitHub) validateAppCredentials(ctx context.Context) (annotations.Annot return nil, err } + if err := gh.validateBasePermissionVisibility(ctx, orgLogins[0]); err != nil { + return nil, err + } + if len(gh.enterprises) > 0 { l := ctxzap.Extract(ctx) _, _, err := gh.customClient.ListEnterpriseConsumedLicenses(ctx, gh.enterprises[0], 1) @@ -282,6 +292,38 @@ func (gh *GitHub) validateAppCredentials(ctx context.Context) (annotations.Annot return nil, nil } +// validateBasePermissionVisibility probes a single org to confirm the credential can +// read default_repository_permission, which direct-collaborators-only requires to sync +// org-member repo access (see getOrgBasePermission). GitHub returns the field only to +// org owners whose credential has admin:org scope / the GitHub App Organization +// Administration permission, and omits it silently otherwise. +// +// The scope/permission dimension is credential-wide, so probing one org where the +// credential is already confirmed as an admin is representative — no need to probe +// every org (there can be thousands). Per-org enforcement still happens at sync time. +func (gh *GitHub) validateBasePermissionVisibility(ctx context.Context, orgLogin string) error { + if !gh.directCollaboratorsOnly { + return nil + } + + org, resp, err := gh.client.Organizations.Get(ctx, orgLogin) + if err != nil { + return wrapGitHubError(err, resp, fmt.Sprintf("github-connector: failed to get organization %s", orgLogin)) + } + + if org.GetDefaultRepoPermission() == "" { + err := fmt.Errorf( + "org %q did not return default_repository_permission; direct-collaborators-only requires it to sync org-member repo access correctly. "+ + "Grant the credential org-owner visibility (admin:org scope, the GitHub App Organization Administration permission, "+ + "or fine-grained PAT organization Administration read access), or disable direct-collaborators-only", + orgLogin, + ) + return uhttp.WrapErrors(codes.PermissionDenied, "github-connector: credentials validation failed", err) + } + + return nil +} + // newGitHubClient returns a new GitHub API client authenticated with an access token via oauth2. func newGitHubClient(ctx context.Context, instanceURL string, ts oauth2.TokenSource) (*github.Client, error) { httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, ctxzap.Extract(ctx))) diff --git a/pkg/connector/connector_test.go b/pkg/connector/connector_test.go new file mode 100644 index 00000000..80648755 --- /dev/null +++ b/pkg/connector/connector_test.go @@ -0,0 +1,53 @@ +package connector + +import ( + "context" + "testing" + + "github.com/google/go-github/v69/github" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/conductorone/baton-github/test/mocks" +) + +// TestValidateBasePermissionVisibility covers the up-front credential probe for +// direct-collaborators-only: default_repository_permission must be readable or +// validation fails with PermissionDenied. Seeded mock org ID is 12 (login +// "organization-12"), and the seeded org omits default_repository_permission, +// matching what GitHub returns to credentials without org-owner visibility. +func TestValidateBasePermissionVisibility(t *testing.T) { + ctx := context.Background() + + setup := func(t *testing.T, directCollaboratorsOnly bool) (*mocks.MockGitHub, *GitHub) { + t.Helper() + mgh := mocks.NewMockGitHub() + _, _, _, _, _, err := mgh.Seed() + require.NoError(t, err) + return mgh, &GitHub{ + client: github.NewClient(mgh.Server()), + directCollaboratorsOnly: directCollaboratorsOnly, + } + } + + t.Run("missing default_repository_permission fails validation", func(t *testing.T) { + _, gh := setup(t, true) + err := gh.validateBasePermissionVisibility(ctx, "organization-12") + require.Error(t, err) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + require.ErrorContains(t, err, "default_repository_permission") + }) + + t.Run("visible default_repository_permission passes validation", func(t *testing.T) { + mgh, gh := setup(t, true) + mgh.SetOrgDefaultRepoPermission(12, "none") + require.NoError(t, gh.validateBasePermissionVisibility(ctx, "organization-12")) + }) + + t.Run("skipped when direct-collaborators-only is off", func(t *testing.T) { + _, gh := setup(t, false) + // No API call is made, so even the missing field is fine. + require.NoError(t, gh.validateBasePermissionVisibility(ctx, "organization-12")) + }) +} diff --git a/pkg/connector/repository.go b/pkg/connector/repository.go index 85762ef7..64957409 100644 --- a/pkg/connector/repository.go +++ b/pkg/connector/repository.go @@ -197,37 +197,40 @@ func (o *repositoryResourceType) Grants( // When direct-collaborators-only is enabled, org members whose only repo access // is via the org base permission won't appear in ListCollaborators. Add expandable // grants so the SDK resolves org membership into repo access. + // + // If the base permission can't be determined, fail the sync: guessing in + // either direction produces wrong access data (inventing grants or + // silently dropping real ones). if o.directCollaboratorsOnly { basePerm, err := o.getOrgBasePermission(ctx, opts.Session, orgName, resource.ParentResourceId) if err != nil { - l.Debug("failed to fetch org base permission, skipping org expansion", zap.Error(err)) - } else { - orgResID := resource.ParentResourceId.Resource - // Org admins always have admin on all repos - adminEntitlementID := fmt.Sprintf("%s:%s:%s", resourceTypeOrg.Id, orgResID, orgRoleAdmin) - for _, perm := range repoAccessLevels { + return nil, nil, err + } + orgResID := resource.ParentResourceId.Resource + // Org admins always have admin on all repos + adminEntitlementID := fmt.Sprintf("%s:%s:%s", resourceTypeOrg.Id, orgResID, orgRoleAdmin) + for _, perm := range repoAccessLevels { + rv = append(rv, grant.NewGrant(resource, perm, resource.ParentResourceId, + grant.WithAnnotation(&v2.GrantExpandable{ + EntitlementIds: []string{adminEntitlementID}, + Shallow: true, + ResourceTypeIds: []string{resourceTypeUser.Id}, + }), + )) + } + // Org members get access based on the org's default repo permission + memberPerms := orgBasePermissionToRepoPermissions(basePerm) + if len(memberPerms) > 0 { + memberEntitlementID := fmt.Sprintf("%s:%s:%s", resourceTypeOrg.Id, orgResID, orgRoleMember) + for _, perm := range memberPerms { rv = append(rv, grant.NewGrant(resource, perm, resource.ParentResourceId, grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{adminEntitlementID}, + EntitlementIds: []string{memberEntitlementID}, Shallow: true, ResourceTypeIds: []string{resourceTypeUser.Id}, }), )) } - // Org members get access based on the org's default repo permission - memberPerms := orgBasePermissionToRepoPermissions(basePerm) - if len(memberPerms) > 0 { - memberEntitlementID := fmt.Sprintf("%s:%s:%s", resourceTypeOrg.Id, orgResID, orgRoleMember) - for _, perm := range memberPerms { - rv = append(rv, grant.NewGrant(resource, perm, resource.ParentResourceId, - grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{memberEntitlementID}, - Shallow: true, - ResourceTypeIds: []string{resourceTypeUser.Id}, - }), - )) - } - } } } @@ -549,13 +552,14 @@ func orgBasePermissionSessionKey(orgID string) string { // getOrgBasePermission fetches the org's default_repository_permission, caching in the session. // Returns "read", "write", "admin", or "none". // -// An empty/absent field is treated as "none" (fail closed). GitHub only returns +// An empty/absent field is an error. GitHub only returns // default_repository_permission to org owners / tokens with admin:org (or the // GitHub App Organization Administration permission). An omitted field means -// "unknown", not GitHub's create-org default of "read" — assuming "read" would -// invent pull grants for every org member on every repo. +// "unknown" — guessing "read" invents pull grants for every org member on +// every repo, and guessing "none" silently drops real org-base grants. With +// direct-collaborators-only enabled we cannot produce correct grants without +// this value, so fail the sync and tell the operator how to fix the credential. func (o *repositoryResourceType) getOrgBasePermission(ctx context.Context, ss sessions.SessionStore, orgName string, orgResourceID *v2.ResourceId) (string, error) { - l := ctxzap.Extract(ctx) key := orgBasePermissionSessionKey(orgResourceID.Resource) cached, found, err := session.GetJSON[string](ctx, ss, key) if err != nil { @@ -572,13 +576,14 @@ func (o *repositoryResourceType) getOrgBasePermission(ctx context.Context, ss se perm := org.GetDefaultRepoPermission() if perm == "" { - l.Debug( - "baton-github: org default_repository_permission missing or empty; skipping org-member repo expansion (treating as none). "+ - "Grant the credential org-owner visibility (admin:org / Organization Administration) to sync base-permission grants accurately.", - zap.String("org", orgName), - zap.String("org_id", orgResourceID.Resource), + err := fmt.Errorf( + "org %q did not return default_repository_permission; "+ + "direct-collaborators-only requires it to sync org-member repo access correctly. "+ + "Grant the credential org-owner visibility (admin:org scope, the GitHub App Organization Administration permission, "+ + "or fine-grained PAT organization Administration read access), or disable direct-collaborators-only", + orgName, ) - perm = "none" + return "", uhttp.WrapErrors(codes.PermissionDenied, "baton-github: failed to determine org base permission", err) } if err := session.SetJSON(ctx, ss, key, perm); err != nil { diff --git a/pkg/connector/repository_test.go b/pkg/connector/repository_test.go index 88be6002..877baed1 100644 --- a/pkg/connector/repository_test.go +++ b/pkg/connector/repository_test.go @@ -12,6 +12,8 @@ import ( resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/google/go-github/v69/github" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/conductorone/baton-github/test" "github.com/conductorone/baton-github/test/mocks" @@ -87,7 +89,9 @@ func TestRepository(t *testing.T) { // empty default_repository_permission bug: when GitHub omits the field (the // credential lacks org-owner visibility), the connector used to assume "read" // and emit expandable org-member pull grants on every repo — inventing access -// for every org member. It drives Grants() end-to-end with +// for every org member. With direct-collaborators-only enabled the base +// permission is required to produce correct grants, so an omitted field must +// hard-fail the sync instead of guessing. It drives Grants() end-to-end with // direct-collaborators-only enabled. // // Seeded mock IDs: org 12, repo 34, user 56. @@ -150,15 +154,31 @@ func TestRepositoryGrantsOrgBasePermissionExpansion(t *testing.T) { return mgh, builder, repository } - t.Run("missing default_repository_permission fails closed", func(t *testing.T) { + t.Run("missing default_repository_permission fails the sync", func(t *testing.T) { _, builder, repository := setup(t) // The seeded org omits default_repository_permission, like GitHub does - // for credentials without admin:org / Organization Administration. + // for credentials without admin:org / Organization Administration. We + // can't know org members' repo access without it, so Grants must error + // rather than guess in either direction. + grants, _, err := builder.Grants(ctx, repository, resourceSdk.SyncOpAttrs{ + PageToken: pagination.Token{}, + Session: &noOpSessionStore{}, + }) + require.ErrorContains(t, err, "default_repository_permission") + require.Equal(t, codes.PermissionDenied, status.Code(err), + "missing default_repository_permission is a credential permission problem, not a transient failure") + require.Empty(t, grants, + "an omitted default_repository_permission must not produce grants") + }) + + t.Run("none default_repository_permission emits no member expansion grants", func(t *testing.T) { + mgh, builder, repository := setup(t) + mgh.SetOrgDefaultRepoPermission(12, "none") grants := listAllGrants(t, builder, repository) - require.NotEmpty(t, grants, "expected admin-expansion and direct-collaborator grants") + require.NotEmpty(t, grants, "admin expansion and direct collaborators are still emitted") require.Empty(t, memberExpansionGrants(t, grants), - "an omitted default_repository_permission must not invent org-member repo grants") + "base permission none must not expand org members into repo access") }) t.Run("read default_repository_permission still expands members to pull", func(t *testing.T) {