From 3ed5949f75153d14965b4a033e0ff86f99c292d2 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Tue, 21 Jul 2026 11:58:16 -0600 Subject: [PATCH] org base permission is none, not read --- pkg/connector/repository.go | 15 +++- pkg/connector/repository_test.go | 114 +++++++++++++++++++++++++++++++ test/mocks/endpointpattern.go | 5 ++ test/mocks/github.go | 33 +++++++++ 4 files changed, 166 insertions(+), 1 deletion(-) diff --git a/pkg/connector/repository.go b/pkg/connector/repository.go index 841aedd2..85762ef7 100644 --- a/pkg/connector/repository.go +++ b/pkg/connector/repository.go @@ -548,7 +548,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 +// 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. 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 { @@ -565,7 +572,13 @@ func (o *repositoryResourceType) getOrgBasePermission(ctx context.Context, ss se perm := org.GetDefaultRepoPermission() if perm == "" { - perm = readConst // GitHub default + 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), + ) + perm = "none" } 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 61009d0c..88be6002 100644 --- a/pkg/connector/repository_test.go +++ b/pkg/connector/repository_test.go @@ -2,9 +2,11 @@ package connector import ( "context" + "slices" "testing" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/pagination" entitlement2 "github.com/conductorone/baton-sdk/pkg/types/entitlement" resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" @@ -80,3 +82,115 @@ func TestRepository(t *testing.T) { require.Empty(t, revokeAnnotations) }) } + +// TestRepositoryGrantsOrgBasePermissionExpansion is a regression test for the +// 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 +// direct-collaborators-only enabled. +// +// Seeded mock IDs: org 12, repo 34, user 56. +func TestRepositoryGrantsOrgBasePermissionExpansion(t *testing.T) { + ctx := context.Background() + + memberEntitlementID := "org:12:member" + + // listAllGrants pages through Grants() until the page token is exhausted. + listAllGrants := func(t *testing.T, builder *repositoryResourceType, repository *v2.Resource) []*v2.Grant { + t.Helper() + var grants []*v2.Grant + pToken := pagination.Token{} + for { + nextGrants, results, err := builder.Grants(ctx, repository, resourceSdk.SyncOpAttrs{ + PageToken: pToken, + Session: &noOpSessionStore{}, + }) + require.NoError(t, err) + grants = append(grants, nextGrants...) + if results.NextPageToken == "" { + return grants + } + pToken.Token = results.NextPageToken + } + } + + // memberExpansionGrants returns grants whose GrantExpandable annotation + // expands the org:12:member entitlement — i.e. repo access inferred from + // org membership via the base permission. + memberExpansionGrants := func(t *testing.T, grants []*v2.Grant) []*v2.Grant { + t.Helper() + var rv []*v2.Grant + for _, g := range grants { + expandable := &v2.GrantExpandable{} + annos := annotations.Annotations(g.GetAnnotations()) + found, err := annos.Pick(expandable) + require.NoError(t, err) + if found && slices.Contains(expandable.GetEntitlementIds(), memberEntitlementID) { + rv = append(rv, g) + } + } + return rv + } + + setup := func(t *testing.T) (*mocks.MockGitHub, *repositoryResourceType, *v2.Resource) { + t.Helper() + mgh := mocks.NewMockGitHub() + githubOrganization, githubRepository, _, githubUser, _, _ := mgh.Seed() + // Seed a direct collaborator so the collaborator page succeeds. + mgh.AddRepositoryCollaborator(githubRepository.GetID(), githubUser.GetID()) + + githubClient := github.NewClient(mgh.Server()) + builder := RepositoryBuilder(githubClient, newOrgNameCache(githubClient), false, true /* directCollaboratorsOnly */) + + organization, err := organizationResource(ctx, githubOrganization, nil, false) + require.NoError(t, err) + repository, err := repositoryResource(ctx, githubRepository, organization.Id) + require.NoError(t, err) + return mgh, builder, repository + } + + t.Run("missing default_repository_permission fails closed", 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. + grants := listAllGrants(t, builder, repository) + + require.NotEmpty(t, grants, "expected admin-expansion and direct-collaborator grants") + require.Empty(t, memberExpansionGrants(t, grants), + "an omitted default_repository_permission must not invent org-member repo grants") + }) + + t.Run("read default_repository_permission still expands members to pull", func(t *testing.T) { + mgh, builder, repository := setup(t) + mgh.SetOrgDefaultRepoPermission(12, "read") + grants := listAllGrants(t, builder, repository) + + memberGrants := memberExpansionGrants(t, grants) + require.Len(t, memberGrants, 1, "base permission read should expand org members to exactly pull") + require.Equal(t, "repository:34:pull:org:12", memberGrants[0].GetId()) + }) +} + +func TestOrgBasePermissionToRepoPermissions(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + basePerm string + want []string + }{ + {name: "none skips member expansion", basePerm: "none", want: nil}, + {name: "empty skips member expansion", basePerm: "", want: nil}, + {name: "read grants pull", basePerm: "read", want: []string{repoPermissionPull}}, + {name: "write grants pull triage push", basePerm: "write", want: []string{repoPermissionPull, repoPermissionTriage, repoPermissionPush}}, + {name: "admin grants all levels", basePerm: "admin", want: []string{repoPermissionPull, repoPermissionTriage, repoPermissionPush, repoPermissionMaintain, repoPermissionAdmin}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tc.want, orgBasePermissionToRepoPermissions(tc.basePerm)) + }) + } +} diff --git a/test/mocks/endpointpattern.go b/test/mocks/endpointpattern.go index c57cefff..b7506972 100644 --- a/test/mocks/endpointpattern.go +++ b/test/mocks/endpointpattern.go @@ -22,6 +22,11 @@ var GetOrganizationById = mock.EndpointPattern{ Method: "GET", } +var GetOrgsByOrg = mock.EndpointPattern{ + Pattern: "/orgs/{org}", + Method: "GET", +} + var GetRepositoryById = mock.EndpointPattern{ Pattern: "/repositories/{repository_id}", Method: "GET", diff --git a/test/mocks/github.go b/test/mocks/github.go index 20adf375..129e119f 100644 --- a/test/mocks/github.go +++ b/test/mocks/github.go @@ -234,6 +234,20 @@ func (mgh MockGitHub) getOrganization( } } +// getOrganizationByName handles GET /orgs/{org}. The mock pretends slugs and +// IDs are interchangeable (see getCrossTableId), so "organization-12" resolves +// to org ID 12. +func (mgh MockGitHub) getOrganizationByName( + w http.ResponseWriter, + variables map[string]string, +) { + id, err := getCrossTableId(w, variables, "org") + if err != nil { + return + } + writeResource(w, strconv.FormatInt(id, 10), mgh.organizations) +} + func (mgh MockGitHub) getRepository( w http.ResponseWriter, variables map[string]string, @@ -725,6 +739,7 @@ func addEndpointHandler( func (mgh MockGitHub) Server() *http.Client { routesMap := map[mock.EndpointPattern]handler{ GetOrganizationById: mgh.getOrganization, + GetOrgsByOrg: mgh.getOrganizationByName, GetOrganizationsTeamsMembersByTeamId: mgh.getMembers, GetOrganizationsTeamByTeamId: mgh.getTeam, GetOrganizationsTeamsMembershipsByTeamIdByUsername: mgh.getTeamMembership, @@ -773,6 +788,24 @@ func (mgh *MockGitHub) AddUserToOrgRole(roleID int64, userID int64) { mgh.orgRoles[roleID].Add(userID) } +// SetOrgDefaultRepoPermission sets default_repository_permission on a mock org +// for testing purposes. When unset, the org payload omits the field, matching +// what GitHub returns to credentials without org-owner visibility. +func (mgh *MockGitHub) SetOrgDefaultRepoPermission(orgID int64, permission string) { + org := mgh.organizations[orgID] + org.DefaultRepoPermission = github.Ptr(permission) + mgh.organizations[orgID] = org +} + +// AddRepositoryCollaborator adds a user as a direct repository collaborator +// for testing purposes. +func (mgh *MockGitHub) AddRepositoryCollaborator(repoID int64, userID int64) { + if _, ok := mgh.repositoryMemberships[repoID]; !ok { + mgh.repositoryMemberships[repoID] = mapset.NewSet[int64]() + } + mgh.repositoryMemberships[repoID].Add(userID) +} + // AddMembership adds a user to a team for testing purposes. func (mgh *MockGitHub) AddMembership(teamID int64, userID int64) { if _, ok := mgh.teamMemberships[teamID]; !ok {