Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion pkg/connector/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
114 changes: 114 additions & 0 deletions pkg/connector/repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Comment thread
kans marked this conversation as resolved.
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))
})
}
}
5 changes: 5 additions & 0 deletions test/mocks/endpointpattern.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
33 changes: 33 additions & 0 deletions test/mocks/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
Loading