diff --git a/README.md b/README.md index 8ee3daa5..1868d18a 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ baton resources - Organization roles - Invitations (users invited to an organization who have not accepted yet) - GitHub Apps (installed in organizations, synced as non-human identities) -- Enterprise roles, only when `--enterprises` is set, and only on GitHub Enterprise Cloud — `github.com` or a `ghe.com` data-residency host. A personal access token syncs every role; a GitHub App syncs the built-in Enterprise Owner role and can also grant and revoke it, with the app installed on both the enterprise account and the organization +- Enterprise roles, only when `--enterprises` is set, and only on GitHub Enterprise Cloud — `github.com` or a `ghe.com` data-residency host. A personal access token syncs every role; a GitHub App syncs the built-in Enterprise Owner role and can also grant and revoke it, with the app installed on both the enterprise account and the organization. When this type is being synced, Owners, pending Owner invitees, and — with a token — anyone who holds an enterprise role are also synced as users, even when they are not members of the configured organization - Enterprise licenses, only when `--enterprises` is set. The API behind them requires a personal access token; under GitHub App authentication the type answers 403, so leave it disabled on the app path By default, `baton-github` will sync information from any organizations that the provided credential has Administrator permissions on. You can specify exactly which organizations you would like to sync using the `--orgs` flag. diff --git a/docs/docs-info.md b/docs/docs-info.md index 05b4d086..f4c1fe1b 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -288,11 +288,23 @@ C1 has no pending state for a grant, so an invitation nobody has accepted and an An invitation sent to someone who is not a member of the enterprise is therefore invisible to the sync, and that is not hypothetical: an owner invitation can be addressed to any GitHub user. It is not enough that C1 has already synced the principal either — outside collaborators reach C1 through repository access without being enterprise members, so a grant to one of them would create an invitation on GitHub that no later sync could read, and C1 would drop the grant while it stayed live. Grant therefore checks membership first and rejects a non-member with `InvalidArgument` rather than creating state it cannot read back. The check is one filtered `Enterprise.members` lookup, and it compares the returned logins because that argument is a search rather than an exact match. -The cost of resolving invitations scales with the enterprise, not with the organization: every sync walks the whole enterprise membership, one page of 100 per request, and asks one aliased batch per page. An enterprise of N members therefore adds roughly N/50 GraphQL requests per sync — about 1,000 for 50,000 members. Memory is unaffected, since the walk streams through the page token, and none of it happens unless the capability is enabled. +The cost of resolving invitations scales with the enterprise, not with the organization: every sync walks the whole enterprise membership, one page of 100 per request, and asks one aliased batch per page. An enterprise of N members therefore adds roughly N/50 GraphQL requests per sync — about 1,000 for 50,000 members. The user sync walks those same owner and member pages once, so the principals exist as users; the grant sync walks them again. Memory is unaffected, since the walk streams through the page token, and none of it happens unless `--enterprises` is set. -### Owner grants can reference a user the sync did not emit +### Users named by an enterprise-role grant are synced -The `user` resource type is populated from the members of the **configured organization**, while `Organization.enterpriseOwners` returns owners of the whole **enterprise account** and the invitation candidates come from `Enterprise.members`, which spans every organization in it. An owner who belongs to a different organization in the same enterprise therefore produces a grant whose principal this sync never created. Widening the user sync is out of scope here — it would change the connector's user population for every deployment — so the grant is emitted and this limitation is recorded instead. +Organization members still come from the configured organization. When `--enterprises` is set **and the `enterprise_role` type is being synced**, the user sync also emits every principal an enterprise-role grant names who is not in that member list. The second condition is not automatic on a hosted tenant: `enterprise_role` is absent from the published capability set, so C1 leaves it out of a new connector's default selection and an admin enables it by hand, as described above. Until then there are no enterprise-role grants to resolve and the user population is unchanged. + +Under GitHub App authentication the principals are the enterprise's Owners and the enterprise members with a pending Owner invitation; an enterprise member with neither is named by no grant and is left out. + +Under a personal access token the set is far larger, and deliberately so. That path reads every enterprise role out of consumed licenses, and `Member` is the role GitHub reports for every license-consuming account, so the role sync emits a grant for each of them and each one needs a user. The principal set is therefore the whole licensed enterprise, not its administrators. + +The resource ID is the GitHub database ID the grant already uses. These users gain no organization membership; like every other user this connector emits, they carry no parent resource. On the App path the enterprise query supplies the database ID, so an account whose lookup answers 404 is still emitted from that ID and its login. The token path has only a login to go on, so it cannot name the account and the sync fails instead, which is what the enterprise-role grant sync already does with the same lookup. + +Each principal costs one `GET /orgs/{org}/members/{login}` per organization this sync emits members for, until one answers that it holds the account. That check is what keeps the walk from overwriting a member page: those pages resolve a user's SAML identity, the resource store keeps the last write, and emitting the account again from the enterprise would drop it. Which organizations count is decided by the same rule the organization sync applies — the ones the credential administers — because treating a principal as covered by an organization this sync leaves out would drop the user and put the grant back in the state this walk repairs. A configured list costs nothing to resolve. Without `--orgs` the connector reads the organizations once per page of enterprise principals, which is one walk of `/user/orgs` plus one membership read per organization. + +The connector keeps no state between pages. Every configured organization runs the walk at the end of its members and repeating it emits the same records, so the result does not depend on which organization ran it or on state surviving a resumed sync. The cost of that choice is that a deployment naming several organizations walks the enterprise once per organization; GitHub App authentication serves one, so it walks once. + +Budget for the token path before enabling `enterprise_role` on a large enterprise. Its principal set is the whole licensed enterprise, and each member costs the membership checks above plus one `GET /users/{login}`, on top of the one the role sync already spends per login. On an enterprise of tens of thousands of seats that is six figures of REST calls against a 5,000 per hour limit. The App path does not have this shape: its principal set is the Owners and the pending Owner invitees. --- diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 22e16bfb..1b0a3d4f 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -129,13 +129,26 @@ type GitHub struct { enterprises []string newEnterpriseRoleClients enterpriseClientProvider syncLastActivity bool + connectorOpts *cli.ConnectorOpts +} + +// willSyncResourceType is true when the type is in this sync. Nil opts, which +// is how the capabilities prototype is built, means no filter, so every +// advertised type is in scope. +func (gh *GitHub) willSyncResourceType(resourceTypeID string) bool { + if gh.connectorOpts == nil { + return true + } + + return gh.connectorOpts.WillSyncResourceType(resourceTypeID) } func (gh *GitHub) ResourceSyncers(ctx context.Context) []connectorbuilder.ResourceSyncerV2 { resourceSyncers := []connectorbuilder.ResourceSyncerV2{ OrgBuilder(gh.client, gh.appClient, gh.orgCache, gh.orgs, gh.syncSecrets), TeamBuilder(gh.client, gh.orgCache, gh.directCollaboratorsOnly), - UserBuilder(gh.client, gh.graphqlClient, gh.orgCache, gh.orgs, gh.customClient, gh.enterprises), + UserBuilder(gh.client, gh.graphqlClient, gh.orgCache, gh.orgs, gh.customClient, gh.enterprises, + gh.newEnterpriseRoleClients, gh.willSyncResourceType(resourceTypeEnterpriseRole.Id)), RepositoryBuilder(gh.client, gh.orgCache, gh.omitArchivedRepositories, gh.directCollaboratorsOnly), OrgRoleBuilder(gh.client, gh.orgCache), InvitationBuilder(InvitationBuilderParams{ @@ -329,21 +342,21 @@ func NewLambdaConnector(ctx context.Context, ghc *cfg.Github, cliOpts *cli.Conne err error ) if group == cfg.GithubAppGroup { - cb, err = newWithGithubApp(ctx, ghc) + cb, err = newWithGithubApp(ctx, ghc, cliOpts) if err != nil { return nil, nil, err } return cb, nil, nil } - cb, err = newWithGithubPAT(ctx, ghc) + cb, err = newWithGithubPAT(ctx, ghc, cliOpts) if err != nil { return nil, nil, err } return cb, nil, nil } -func newWithGithubPAT(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { +func newWithGithubPAT(ctx context.Context, ghc *cfg.Github, connectorOpts *cli.ConnectorOpts) (*GitHub, error) { ts := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: ghc.Token}, ) @@ -367,6 +380,7 @@ func newWithGithubPAT(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { omitArchivedRepositories: ghc.OmitArchivedRepositories, directCollaboratorsOnly: ghc.DirectCollaboratorsOnly, syncLastActivity: ghc.SyncLastActivity, + connectorOpts: connectorOpts, }, nil } @@ -384,7 +398,7 @@ func appPrivateKeyPEM(ghc *cfg.Github) (string, error) { return "", errors.New("github app authentication requires either --app-privatekey or --app-privatekey-path") } -func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { +func newWithGithubApp(ctx context.Context, ghc *cfg.Github, connectorOpts *cli.ConnectorOpts) (*GitHub, error) { enterprises := distinctEnterprises(ghc.Enterprises) privateKey, err := appPrivateKeyPEM(ghc) if err != nil { @@ -485,6 +499,7 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { omitArchivedRepositories: ghc.OmitArchivedRepositories, directCollaboratorsOnly: ghc.DirectCollaboratorsOnly, syncLastActivity: ghc.SyncLastActivity, + connectorOpts: connectorOpts, } return gh, nil } diff --git a/pkg/connector/default_capabilities.go b/pkg/connector/default_capabilities.go index f0dbe55d..ab9e141b 100644 --- a/pkg/connector/default_capabilities.go +++ b/pkg/connector/default_capabilities.go @@ -30,7 +30,7 @@ func (d *DefaultCapabilitiesBuilder) ResourceSyncers(_ context.Context) []connec return []connectorbuilder.ResourceSyncerV2{ OrgBuilder(nil, nil, nil, nil, false), TeamBuilder(nil, nil, false), - UserBuilder(nil, nil, nil, nil, nil, nil), + UserBuilder(nil, nil, nil, nil, nil, nil, nil, false), RepositoryBuilder(nil, nil, false, false), OrgRoleBuilder(nil, nil), InvitationBuilder(InvitationBuilderParams{}), diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 54d49556..2a3fe0e2 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -421,10 +421,7 @@ func (o *enterpriseRoleResourceType) appGrants( bag.Push(pagination.PageState{ResourceTypeID: enterpriseOwnersPhase}) } - var after *githubv4.String - if cursor := bag.PageToken(); cursor != "" { - after = githubv4.NewString(githubv4.String(cursor)) - } + after := graphqlCursor(bag.PageToken()) var ( ret []*v2.Grant diff --git a/pkg/connector/enterprise_users.go b/pkg/connector/enterprise_users.go new file mode 100644 index 00000000..446b809f --- /dev/null +++ b/pkg/connector/enterprise_users.go @@ -0,0 +1,488 @@ +package connector + +import ( + "context" + "fmt" + "slices" + "strconv" + "strings" + + "github.com/conductorone/baton-github/pkg/customclient" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/pagination" + "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/google/go-github/v69/github" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "github.com/shurcooL/githubv4" + "go.uber.org/zap" +) + +const ( + // Phases of the user list that run after an organization's members, one + // page per call. They emit the principals enterprise-role grants name. + enterpriseUserOwnersPhase = "enterprise-user-owners" + enterpriseUserPendingPhase = "enterprise-user-pending" + enterpriseUserLicensesPhase = "enterprise-user-licenses" +) + +// tracksEnterprisePrincipals reports whether this sync resolves the principals +// of enterprise-role grants. Without the role type there are no such grants. +func (u *userResourceType) tracksEnterprisePrincipals() bool { + return len(u.enterprises) > 0 && u.syncsEnterpriseRoles +} + +func isEnterpriseUserPhase(phase string) bool { + switch phase { + case enterpriseUserOwnersPhase, enterpriseUserPendingPhase, enterpriseUserLicensesPhase: + return true + default: + return false + } +} + +// nextUserPageToken advances the organization's member pages, and on the last +// one hands the page token to the enterprise-principal phases. +// +// Queuing those phases reads the enterprise, so this page can fail on one that +// is unreadable. NotFound has to be converted: the SDK treats it as a warning +// and completes the sync, dropping the members this page already fetched. +func (u *userResourceType) nextUserPageToken( + ctx context.Context, + bag *pagination.Bag, + nextPage string, +) (string, error) { + if nextPage == "" { + token, queued, err := u.continueWithEnterprisePrincipals(ctx, bag) + if err != nil { + return "", failClosedOnUnreadableEnterprise(err, strings.Join(u.enterprises, ", ")) + } + if queued { + return token, nil + } + } + + return bag.NextToken(nextPage) +} + +// continueWithEnterprisePrincipals appends the enterprise-principal phases to +// bag. It returns the next page token and true when the member page should +// continue into those phases. +// +// App authentication walks Owners and pending Owner invitees, the two sets +// Grants emits; a personal access token walks the consumed-licenses users that +// hold an enterprise role. A setup error skips the phases, because the role +// type skips its grants for the same reason. +func (u *userResourceType) continueWithEnterprisePrincipals( + ctx context.Context, + bag *pagination.Bag, +) (string, bool, error) { + if !u.tracksEnterprisePrincipals() { + return "", false, nil + } + + enterprises := distinctEnterprises(u.enterprises) + phases := []string{enterpriseUserLicensesPhase} + slugs := enterprises + if u.newEnterpriseClients != nil { + clients, err := u.newEnterpriseClients(ctx, enterprises) + if err != nil { + if isEnterpriseSetupError(err) { + ctxzap.Extract(ctx).Debug( + "baton-github: not syncing enterprise principals as users, the GitHub App is not set up to read them", + zap.Error(err), + ) + return "", false, nil + } + return "", false, err + } + phases = []string{enterpriseUserOwnersPhase, enterpriseUserPendingPhase} + slugs = enterprisesWithClients(slugs, clients) + } + plan := enterpriseUserPhasePlan(slugs, phases) + if len(plan) == 0 { + return "", false, nil + } + + queueEnterpriseUserPhases(bag, plan) + token, err := bag.Marshal() + if err != nil { + return "", false, fmt.Errorf("baton-github: error encoding the enterprise user page token: %w", err) + } + + return token, true, nil +} + +// enterpriseUserPhasePlan lists the pages the walk visits, in the order it +// visits them: every phase of one enterprise before moving to the next. +func enterpriseUserPhasePlan(slugs []string, phases []string) []pagination.PageState { + plan := make([]pagination.PageState, 0, len(slugs)*len(phases)) + for _, slug := range slugs { + for _, phase := range phases { + plan = append(plan, pagination.PageState{ + ResourceTypeID: phase, + ResourceID: slug, + }) + } + } + + return plan +} + +// queueEnterpriseUserPhases replaces the finished member state with the plan. +// The bag is a stack, so the plan is pushed back to front and its first page +// ends up on top. +func queueEnterpriseUserPhases(bag *pagination.Bag, plan []pagination.PageState) { + bag.Pop() + for _, state := range slices.Backward(plan) { + bag.Push(state) + } +} + +func enterprisesWithClients(slugs []string, clients map[string]*customclient.EnterpriseAdminClient) []string { + var withClients []string + for _, enterprise := range slugs { + if _, ok := clients[enterprise]; ok { + withClients = append(withClients, enterprise) + } + } + + return withClients +} + +// listEnterprisePrincipalUsers emits one page of enterprise principals whose +// user resource the organization member list does not produce. +func (u *userResourceType) listEnterprisePrincipalUsers( + ctx context.Context, + bag *pagination.Bag, +) ([]*v2.Resource, *resource.SyncOpResults, error) { + // Resolved once for the page: every principal on it is checked against the + // same organizations. + orgs, annos, err := u.syncedOrgs(ctx) + if err != nil { + return nil, &resource.SyncOpResults{Annotations: annos}, err + } + + var ( + ret []*v2.Resource + next string + pageAnnos annotations.Annotations + ) + switch phase := bag.ResourceTypeID(); phase { + case enterpriseUserOwnersPhase: + ret, next, pageAnnos, err = u.listOwnerUsers(ctx, orgs, bag.ResourceID(), bag.PageToken()) + case enterpriseUserPendingPhase: + ret, next, pageAnnos, err = u.listPendingOwnerUsers(ctx, orgs, bag.ResourceID(), bag.PageToken()) + case enterpriseUserLicensesPhase: + ret, next, pageAnnos, err = u.listLicensedRoleUsers(ctx, orgs, bag.ResourceID(), bag.PageToken()) + default: + return nil, nil, fmt.Errorf("baton-github: unexpected user sync phase %q", phase) + } + annos = freshestRateLimit(annos, pageAnnos) + // As in nextUserPageToken: a NotFound would be a warning, and the sync would + // report no enterprise principals while the role syncer emits their grants. + if err != nil { + return nil, &resource.SyncOpResults{Annotations: annos}, + failClosedOnUnreadableEnterprise(err, bag.ResourceID()) + } + + token, err := bag.NextToken(next) + if err != nil { + return nil, &resource.SyncOpResults{Annotations: annos}, + fmt.Errorf("baton-github: error advancing the enterprise user page token: %w", err) + } + + return ret, &resource.SyncOpResults{ + NextPageToken: token, + Annotations: annos, + }, nil +} + +func (u *userResourceType) listOwnerUsers( + ctx context.Context, + orgs []string, + enterprise string, + cursor string, +) ([]*v2.Resource, string, annotations.Annotations, error) { + client, err := u.adminClient(ctx, enterprise) + if err != nil { + return nil, "", nil, err + } + + owners, next, annos, err := client.Owners(ctx, graphqlCursor(cursor)) + if err != nil { + return nil, "", annos, err + } + + ret, annos, err := u.resourcesForEnterpriseUsers(ctx, orgs, owners, annos) + if err != nil { + return nil, "", annos, err + } + + return ret, next, annos, nil +} + +func (u *userResourceType) listPendingOwnerUsers( + ctx context.Context, + orgs []string, + enterprise string, + cursor string, +) ([]*v2.Resource, string, annotations.Annotations, error) { + client, err := u.adminClient(ctx, enterprise) + if err != nil { + return nil, "", nil, err + } + + members, next, annos, err := client.Members(ctx, enterprise, graphqlCursor(cursor)) + if err != nil { + return nil, "", annos, err + } + if len(members) == 0 { + return nil, next, annos, nil + } + + logins := make([]string, 0, len(members)) + for _, member := range members { + logins = append(logins, member.Login) + } + invitations, invitationAnnos, err := client.PendingOwnerInvitations(ctx, enterprise, logins) + annos = freshestRateLimit(annos, invitationAnnos) + if err != nil { + return nil, "", annos, err + } + + invited := make([]customclient.EnterpriseUser, 0, len(invitations)) + for _, member := range members { + if _, ok := invitations[member.Login]; ok { + invited = append(invited, member) + } + } + + ret, annos, err := u.resourcesForEnterpriseUsers(ctx, orgs, invited, annos) + if err != nil { + return nil, "", annos, err + } + + return ret, next, annos, nil +} + +// listLicensedRoleUsers emits the consumed-licenses users who hold an +// enterprise role, which is the set the token path's Grants emits. +func (u *userResourceType) listLicensedRoleUsers( + ctx context.Context, + orgs []string, + enterprise string, + cursor string, +) ([]*v2.Resource, string, annotations.Annotations, error) { + page := 1 + if cursor != "" { + parsed, err := strconv.Atoi(cursor) + if err != nil { + return nil, "", nil, fmt.Errorf("baton-github: invalid consumed-licenses page %q: %w", cursor, err) + } + page = parsed + } + + var annos annotations.Annotations + consumed, rateLimit, err := u.customClient.ListEnterpriseConsumedLicenses(ctx, enterprise, page) + annos.WithRateLimiting(rateLimit) + if err != nil { + return nil, "", annos, fmt.Errorf( + "baton-github: error listing enterprise consumed licenses for %s: %w", enterprise, err) + } + if len(consumed.Users) == 0 { + return nil, "", annos, nil + } + + ret := make([]*v2.Resource, 0, len(consumed.Users)) + for _, licensed := range consumed.Users { + if licensed.GitHubComLogin == "" || len(licensed.GitHubComEnterpriseRoles) == 0 { + continue + } + emitted, memberAnnos, err := u.emittedByOrgMembers(ctx, orgs, licensed.GitHubComLogin) + annos = freshestRateLimit(annos, memberAnnos) + if err != nil { + return nil, "", annos, err + } + if emitted { + continue + } + + ghUser, resp, err := u.client.Users.Get(ctx, licensed.GitHubComLogin) + if restRateLimit, rateLimitErr := extractRateLimitData(resp); rateLimitErr == nil { + annos.WithRateLimiting(restRateLimit) + } + if err != nil { + return nil, "", annos, wrapGitHubError(err, resp, + fmt.Sprintf("baton-github: failed to get user %s", licensed.GitHubComLogin)) + } + ur, err := userResource(ctx, ghUser, ghUser.GetEmail(), nil) + if err != nil { + return nil, "", annos, err + } + ret = append(ret, ur) + } + + return ret, strconv.Itoa(page + 1), annos, nil +} + +// adminClient builds the administration client of one enterprise. Each page +// builds its own, since this syncer keeps no state between pages. +func (u *userResourceType) adminClient( + ctx context.Context, + enterprise string, +) (*customclient.EnterpriseAdminClient, error) { + clients, err := u.newEnterpriseClients(ctx, []string{enterprise}) + if err != nil { + return nil, err + } + client, ok := clients[enterprise] + if !ok { + return nil, fmt.Errorf("baton-github: enterprise %s has no administration client", enterprise) + } + + return client, nil +} + +// resourcesForEnterpriseUsers builds a user resource for each principal no +// organization in this sync emits. +func (u *userResourceType) resourcesForEnterpriseUsers( + ctx context.Context, + orgs []string, + users []customclient.EnterpriseUser, + annos annotations.Annotations, +) ([]*v2.Resource, annotations.Annotations, error) { + ret := make([]*v2.Resource, 0, len(users)) + for _, user := range users { + if user.DatabaseID <= 0 { + ctxzap.Extract(ctx).Debug("baton-github: skipping an enterprise principal with no database ID", + zap.String("login", user.Login), + ) + continue + } + + emitted, memberAnnos, err := u.emittedByOrgMembers(ctx, orgs, user.Login) + annos = freshestRateLimit(annos, memberAnnos) + if err != nil { + return nil, annos, err + } + if emitted { + continue + } + + ur, userAnnos, err := u.userFromDatabaseID(ctx, user.DatabaseID, user.Login) + annos = freshestRateLimit(annos, userAnnos) + if err != nil { + return nil, annos, err + } + ret = append(ret, ur) + } + + return ret, annos, nil +} + +// emittedByOrgMembers reports whether an organization this sync walks already +// emits the login. Those member pages carry the user's SAML identity, and the +// resource store keeps the last write, so emitting the account again here +// would drop it. +func (u *userResourceType) emittedByOrgMembers( + ctx context.Context, + orgs []string, + login string, +) (bool, annotations.Annotations, error) { + var annos annotations.Annotations + for _, org := range orgs { + member, resp, err := u.client.Organizations.IsMember(ctx, org, login) + if rateLimit, rateLimitErr := extractRateLimitData(resp); rateLimitErr == nil { + annos.WithRateLimiting(rateLimit) + } + if err != nil { + return false, annos, wrapGitHubError(err, resp, + fmt.Sprintf("baton-github: failed to check whether %s belongs to organization %s", login, org)) + } + if member { + return true, annos, nil + } + } + + return false, annos, nil +} + +// syncedOrgs returns the organizations whose member pages this sync emits. The +// set has to match what the organization syncer produces: counting a principal +// as emitted by an organization this sync leaves out would drop the user and +// leave the grant pointing at nothing. +// +// A named organization is taken as given, because the organization syncer does +// the same: under a GitHub App the installation decides, and under a token +// validation already refused a credential that does not administer one. Naming +// none syncs every organization the credential administers, so that filter has +// to run here too. +func (u *userResourceType) syncedOrgs(ctx context.Context) ([]string, annotations.Annotations, error) { + var annos annotations.Annotations + if len(u.orgs) > 0 { + return u.orgs, annos, nil + } + + candidates, err := getOrgs(ctx, u.client, nil) + if err != nil { + return nil, annos, err + } + + administered := make([]string, 0, len(candidates)) + for _, org := range candidates { + isAdmin, resp, err := administersOrg(ctx, u.client, org) + if rateLimit, rateLimitErr := extractRateLimitData(resp); rateLimitErr == nil { + annos.WithRateLimiting(rateLimit) + } + if err != nil { + return nil, annos, wrapGitHubError(err, resp, + fmt.Sprintf("baton-github: failed to read the membership of organization %s", org)) + } + if isAdmin { + administered = append(administered, org) + } + } + + return administered, annos, nil +} + +// userFromDatabaseID reads the account behind an enterprise principal. A 404 +// still emits the principal from the database ID and login the enterprise query +// returned: dropping it would leave the grant pointing at nothing. +func (u *userResourceType) userFromDatabaseID( + ctx context.Context, + id int64, + login string, +) (*v2.Resource, annotations.Annotations, error) { + var annos annotations.Annotations + ghUser, resp, err := u.client.Users.GetByID(ctx, id) + if rateLimit, rateLimitErr := extractRateLimitData(resp); rateLimitErr == nil { + annos.WithRateLimiting(rateLimit) + } + if err != nil { + if !isNotFoundError(resp) { + return nil, annos, wrapGitHubError(err, resp, fmt.Sprintf("baton-github: failed to get user %d", id)) + } + ctxzap.Extract(ctx).Debug("baton-github: emitting an enterprise principal whose account did not resolve", + zap.Int64("user_id", id), + zap.String("login", login), + ) + ghUser = &github.User{ID: &id, Login: &login} + } + + ur, err := userResource(ctx, ghUser, ghUser.GetEmail(), nil) + if err != nil { + return nil, annos, err + } + + return ur, annos, nil +} + +func graphqlCursor(cursor string) *githubv4.String { + if cursor == "" { + return nil + } + + return githubv4.NewString(githubv4.String(cursor)) +} diff --git a/pkg/connector/enterprise_users_test.go b/pkg/connector/enterprise_users_test.go new file mode 100644 index 00000000..9e78c960 --- /dev/null +++ b/pkg/connector/enterprise_users_test.go @@ -0,0 +1,482 @@ +package connector + +import ( + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/cli" + "github.com/conductorone/baton-sdk/pkg/pagination" + resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/google/go-github/v69/github" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/conductorone/baton-github/pkg/customclient" + "github.com/conductorone/baton-github/test/mocks" +) + +// orgSAMLGraphQL answers an organization that has its own SAML provider, so the +// user list does not fall through to the consumed-licenses enrichment. +func orgSAMLGraphQL(t *testing.T) *githubv4.Client { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + if strings.Contains(string(body), "externalIdentities") { + _, _ = w.Write([]byte(`{"data":{"organization":{"samlIdentityProvider":{"externalIdentities":{"edges":[]}}},` + + `"rateLimit":{"limit":5000,"remaining":4999,"resetAt":"2026-09-18T23:00:00Z"}}}`)) + return + } + _, _ = w.Write([]byte(`{"data":{"organization":{"samlIdentityProvider":{"id":"saml","ssoUrl":"https://example.com/sso"}}}}`)) + })) + t.Cleanup(srv.Close) + + return githubv4.NewEnterpriseClient(srv.URL, srv.Client()) +} + +// newTestEnterpriseUserBuilder returns a user syncer whose organization holds +// the seeded member and whose enterprise is served by stub. +func newTestEnterpriseUserBuilder( + t *testing.T, + stub *enterpriseStub, + syncsEnterpriseRoles bool, +) (*userResourceType, *v2.Resource) { + t.Helper() + + if stub.invitations == nil { + stub.invitations = make(map[string]string) + } + enterpriseClient := newTestEnterpriseAdminClient(t, stub, testOrg) + + mgh := mocks.NewMockGitHub() + githubOrganization, _, _, _, _, err := mgh.Seed() + require.NoError(t, err) + organization, err := organizationResource(context.Background(), githubOrganization, nil, false) + require.NoError(t, err) + + ghClient := github.NewClient(mgh.Server()) + builder := UserBuilder( + ghClient, + orgSAMLGraphQL(t), + newOrgNameCache(ghClient), + []string{githubOrganization.GetLogin()}, + nil, + []string{testEnterprise}, + func(context.Context, []string) (map[string]*customclient.EnterpriseAdminClient, error) { + return map[string]*customclient.EnterpriseAdminClient{testEnterprise: enterpriseClient}, nil + }, + syncsEnterpriseRoles, + ) + + return builder, organization +} + +// drainUserList walks every page of the user list and returns how many times +// each resource ID was emitted. +func drainUserList(t *testing.T, builder *userResourceType, organization *v2.Resource) map[string]int { + t.Helper() + + counts := map[string]int{} + token := "" + for page := 0; page < 8; page++ { + users, results, err := builder.List(context.Background(), organization.Id, resourceSdk.SyncOpAttrs{ + PageToken: pagination.Token{Token: token}, + Session: &noOpSessionStore{}, + }) + require.NoError(t, err) + for _, user := range users { + counts[user.GetId().GetResource()]++ + } + token = results.NextPageToken + if token == "" { + return counts + } + } + t.Fatal("the user list did not finish") + + return nil +} + +func TestUserListEmitsEnterprisePrincipalsMissingFromTheOrg(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{ + {{id: testLoginID, login: testLogin}}, + {{id: 999, login: "outsider"}}, + }, + memberPages: [][]enterpriseStubOwner{ + {{id: 1001, login: "invitee"}, {id: 1002, login: "bystander"}}, + }, + invitations: map[string]string{"invitee": "EAI_invitee"}, + } + builder, organization := newTestEnterpriseUserBuilder(t, stub, true) + + counts := drainUserList(t, builder, organization) + + // An Owner the organization already emits is left to the member page, so + // the account keeps the identity that page resolved for it. + require.Equal(t, 1, counts[testLogin]) + // An Owner outside the organization is the dangling principal. + require.Equal(t, 1, counts["999"]) + // A pending Owner invitee holds the same grant, so it needs the same user. + require.Equal(t, 1, counts["1001"]) + // An enterprise member with no role and no invitation is named by no grant. + require.Zero(t, counts["1002"]) +} + +// The plan is the order the walk runs in, and the bag is a stack, so queuing +// it has to come back out in that same order. +func TestEnterpriseUserPhasePlanRunsInOrder(t *testing.T) { + t.Parallel() + + plan := enterpriseUserPhasePlan( + []string{"first", "second"}, + []string{enterpriseUserOwnersPhase, enterpriseUserPendingPhase}, + ) + require.Equal(t, []pagination.PageState{ + {ResourceTypeID: enterpriseUserOwnersPhase, ResourceID: "first"}, + {ResourceTypeID: enterpriseUserPendingPhase, ResourceID: "first"}, + {ResourceTypeID: enterpriseUserOwnersPhase, ResourceID: "second"}, + {ResourceTypeID: enterpriseUserPendingPhase, ResourceID: "second"}, + }, plan) + + bag := &pagination.Bag{} + bag.Push(pagination.PageState{ResourceTypeID: resourceTypeUser.Id}) + queueEnterpriseUserPhases(bag, plan) + + var drained []pagination.PageState + for bag.Current() != nil { + drained = append(drained, pagination.PageState{ + ResourceTypeID: bag.ResourceTypeID(), + ResourceID: bag.ResourceID(), + }) + require.NoError(t, bag.Next("")) + } + require.Equal(t, plan, drained) +} + +// Each configured organization runs the walk at the end of its members, so it +// can run more than once in one sync. Repeating it has to be a no-op: an +// account an organization emits is never emitted from the enterprise, and one +// it does not emit comes back identical. +func TestRepeatingTheEnterpriseWalkIsANoOp(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{ + {id: testLoginID, login: testLogin}, + {id: 999, login: "outsider"}, + }}, + } + builder, organization := newTestEnterpriseUserBuilder(t, stub, true) + + first := drainUserList(t, builder, organization) + ownerQueries := stub.ownerQueries + require.Positive(t, ownerQueries) + + second := drainUserList(t, builder, organization) + + require.Equal(t, first, second) + require.Equal(t, 1, second[testLogin]) + require.Equal(t, 1, second["999"]) + // The walk really did run again rather than being skipped by remembered state. + require.Greater(t, stub.ownerQueries, ownerQueries) +} + +func TestUserListSkipsEnterprisePrincipalsWhenTheAppIsNotSetUp(t *testing.T) { + t.Parallel() + ctx := context.Background() + + mgh := mocks.NewMockGitHub() + githubOrganization, _, _, _, _, err := mgh.Seed() + require.NoError(t, err) + organization, err := organizationResource(ctx, githubOrganization, nil, false) + require.NoError(t, err) + + ghClient := github.NewClient(mgh.Server()) + builder := UserBuilder( + ghClient, + orgSAMLGraphQL(t), + newOrgNameCache(ghClient), + []string{githubOrganization.GetLogin()}, + nil, + []string{testEnterprise}, + func(context.Context, []string) (map[string]*customclient.EnterpriseAdminClient, error) { + return nil, enterpriseSetupError{fmt.Errorf("app is not installed")} + }, + true, + ) + + users, results, err := builder.List(ctx, organization.Id, resourceSdk.SyncOpAttrs{ + Session: &noOpSessionStore{}, + }) + require.NoError(t, err) + require.Empty(t, results.NextPageToken) + require.Len(t, users, 1) + require.Equal(t, testLogin, users[0].GetId().GetResource()) +} + +// Queuing the phases reads the enterprise, so this page can fail on one that +// is unreadable. The SDK answers NotFound by finishing the action with a +// warning and completing the sync, which would drop the members this page +// already fetched and make C1 delete them, so the page has to fail instead. +func TestAnUnreadableEnterpriseFailsTheUserPageRatherThanWarning(t *testing.T) { + t.Parallel() + ctx := context.Background() + + mgh := mocks.NewMockGitHub() + githubOrganization, _, _, _, _, err := mgh.Seed() + require.NoError(t, err) + organization, err := organizationResource(ctx, githubOrganization, nil, false) + require.NoError(t, err) + + ghClient := github.NewClient(mgh.Server()) + builder := UserBuilder( + ghClient, + orgSAMLGraphQL(t), + newOrgNameCache(ghClient), + []string{githubOrganization.GetLogin()}, + nil, + []string{testEnterprise}, + func(context.Context, []string) (map[string]*customclient.EnterpriseAdminClient, error) { + return nil, status.Error(codes.NotFound, "enterprise not found") + }, + true, + ) + + _, _, err = builder.List(ctx, organization.Id, resourceSdk.SyncOpAttrs{ + Session: &noOpSessionStore{}, + }) + require.Error(t, err) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) +} + +// C1 can leave enterprise_role out of the selection. Without that type there +// are no enterprise grants to resolve, so the organization's members stay the +// whole user population and the enterprise APIs are never called. +func TestUserListLeavesEnterprisePrincipalsOutWhenTheRoleTypeIsNotSynced(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{{id: 999, login: "outsider"}}}, + } + builder, organization := newTestEnterpriseUserBuilder(t, stub, false) + + users, results, err := builder.List(context.Background(), organization.Id, resourceSdk.SyncOpAttrs{ + Session: &noOpSessionStore{}, + }) + require.NoError(t, err) + require.Empty(t, results.NextPageToken) + require.Len(t, users, 1) + require.Equal(t, testLogin, users[0].GetId().GetResource()) + require.Zero(t, stub.ownerQueries) +} + +// The connector translates C1's resource type selection for the builders. The +// capabilities prototype carries no options at all, and the SDK reads that as +// every advertised type rather than none. +func TestWillSyncResourceType(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + opts *cli.ConnectorOpts + want bool + }{ + { + name: "no options at all", + want: true, + }, + { + name: "no selection", + opts: &cli.ConnectorOpts{}, + want: true, + }, + { + name: "selected", + opts: &cli.ConnectorOpts{SyncResourceTypeIDs: []string{"user", "enterprise_role"}}, + want: true, + }, + { + name: "not selected", + opts: &cli.ConnectorOpts{SyncResourceTypeIDs: []string{"user", "org"}}, + want: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + gh := &GitHub{connectorOpts: tc.opts} + require.Equal(t, tc.want, gh.willSyncResourceType(resourceTypeEnterpriseRole.Id)) + }) + } +} + +// A personal access token reads the role holders from consumed licenses, so it +// queues that phase rather than the two the enterprise administration API +// serves. +func TestTokenAuthQueuesTheConsumedLicensesPhase(t *testing.T) { + t.Parallel() + + builder := UserBuilder(nil, nil, nil, nil, nil, []string{testEnterprise}, nil, true) + bag := &pagination.Bag{} + bag.Push(pagination.PageState{ResourceTypeID: resourceTypeUser.Id}) + + token, queued, err := builder.continueWithEnterprisePrincipals(context.Background(), bag) + require.NoError(t, err) + require.True(t, queued) + + next := &pagination.Bag{} + require.NoError(t, next.Unmarshal(token)) + require.Equal(t, enterpriseUserLicensesPhase, next.ResourceTypeID()) + require.Equal(t, testEnterprise, next.ResourceID()) +} + +// The token path emits a licensed user who holds an enterprise role, and only +// that user: a seat without a role is named by no grant, and a role holder the +// organization already emits keeps the identity that page resolved. +func TestLicensedRoleUsersCoverOnlyTheRoleHoldersTheOrgDoesNotEmit(t *testing.T) { + t.Parallel() + ctx := context.Background() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Ratelimit-Limit", "5000") + w.Header().Set("X-Ratelimit-Remaining", "4999") + w.Header().Set("X-Ratelimit-Reset", "1900000000") + switch r.URL.Path { + case "/enterprises/" + testEnterprise + "/consumed-licenses": + if r.URL.Query().Get("page") == "1" { + _, _ = w.Write([]byte(`{"users":[` + + `{"github_com_login":"outsider","github_com_enterprise_roles":["Owner"]},` + + `{"github_com_login":"insider","github_com_enterprise_roles":["Owner"]},` + + `{"github_com_login":"seat-only","github_com_enterprise_roles":[]}` + + `]}`)) + return + } + _, _ = w.Write([]byte(`{"users":[]}`)) + case "/user/memberships/orgs/acme": + _, _ = w.Write([]byte(`{"role":"admin","state":"active"}`)) + // The organization already emits this one, so it is left alone. + case "/orgs/acme/members/insider": + w.WriteHeader(http.StatusNoContent) + case "/orgs/acme/members/outsider", "/orgs/acme/members/seat-only": + w.WriteHeader(http.StatusNotFound) + case "/users/outsider": + _, _ = w.Write([]byte(`{"id":999,"login":"outsider"}`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + + base, err := url.Parse(srv.URL + "/") + require.NoError(t, err) + ghClient := github.NewClient(srv.Client()) + ghClient.BaseURL = base + + builder := UserBuilder(ghClient, nil, nil, []string{"acme"}, customclient.New(ghClient), + []string{testEnterprise}, nil, true) + + users, next, _, err := builder.listLicensedRoleUsers(ctx, []string{"acme"}, testEnterprise, "") + require.NoError(t, err) + require.Equal(t, "2", next) + require.Len(t, users, 1) + require.Equal(t, "999", users[0].GetId().GetResource()) + + done, next, _, err := builder.listLicensedRoleUsers(ctx, []string{"acme"}, testEnterprise, "2") + require.NoError(t, err) + require.Empty(t, next) + require.Empty(t, done) +} + +// A deployment that names no organization syncs the ones it administers and +// silently leaves out the rest. Treating a principal as emitted by one of +// those would drop the user and leave the grant pointing at nothing, so only +// the administered organizations decide it. +func TestOnlyAdministeredOrgsDecideWhetherAPrincipalIsAlreadyEmitted(t *testing.T) { + t.Parallel() + + var asked []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + asked = append(asked, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/user/orgs": + _, _ = w.Write([]byte(`[{"login":"administered"},{"login":"joined"},{"login":"opaque"}]`)) + case "/user/memberships/orgs/administered": + _, _ = w.Write([]byte(`{"role":"admin","state":"active"}`)) + // A member-only organization: the sync never emits its members. + case "/user/memberships/orgs/joined": + _, _ = w.Write([]byte(`{"role":"member","state":"active"}`)) + // Membership that is not visible reads the same way as not administered. + case "/user/memberships/orgs/opaque": + w.WriteHeader(http.StatusForbidden) + case "/orgs/administered/members/outsider": + w.WriteHeader(http.StatusNotFound) + case "/enterprises/" + testEnterprise + "/consumed-licenses": + if r.URL.Query().Get("page") == "1" { + _, _ = w.Write([]byte( + `{"users":[{"github_com_login":"outsider","github_com_enterprise_roles":["Owner"]}]}`)) + return + } + _, _ = w.Write([]byte(`{"users":[]}`)) + case "/users/outsider": + _, _ = w.Write([]byte(`{"id":999,"login":"outsider"}`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + + base, err := url.Parse(srv.URL + "/") + require.NoError(t, err) + ghClient := github.NewClient(srv.Client()) + ghClient.BaseURL = base + + builder := UserBuilder(ghClient, nil, nil, nil, customclient.New(ghClient), + []string{testEnterprise}, nil, true) + + ctx := context.Background() + orgs, _, err := builder.syncedOrgs(ctx) + require.NoError(t, err) + require.Equal(t, []string{"administered"}, orgs) + + users, _, _, err := builder.listLicensedRoleUsers(ctx, orgs, testEnterprise, "") + require.NoError(t, err) + require.Len(t, users, 1) + require.Equal(t, "999", users[0].GetId().GetResource()) + require.NotContains(t, asked, "/orgs/joined/members/outsider") + require.NotContains(t, asked, "/orgs/opaque/members/outsider") +} + +// The enterprise query already supplied a stable ID, so an account that no +// longer resolves is still emitted rather than left as a dangling principal. +func TestEnterprisePrincipalIsEmittedWhenTheAccountDoesNotResolve(t *testing.T) { + t.Parallel() + + mgh := mocks.NewMockGitHub() + _, _, _, _, _, err := mgh.Seed() + require.NoError(t, err) + + builder := UserBuilder(github.NewClient(mgh.Server()), nil, nil, nil, nil, + []string{testEnterprise}, nil, true) + + ur, _, err := builder.userFromDatabaseID(context.Background(), 999, "ghost") + require.NoError(t, err) + require.Equal(t, "999", ur.GetId().GetResource()) + require.Equal(t, "ghost", ur.GetDisplayName()) +} diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index b4bc5f41..6b50642a 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -16,7 +16,9 @@ import ( "github.com/conductorone/baton-sdk/pkg/types/sessions" "github.com/conductorone/baton-sdk/pkg/uhttp" "github.com/google/go-github/v69/github" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "github.com/shurcooL/githubv4" + "go.uber.org/zap" "golang.org/x/text/cases" "golang.org/x/text/language" "google.golang.org/grpc/codes" @@ -70,6 +72,30 @@ func (o *orgNameCache) GetOrgNameFromRemoteServer(ctx context.Context, rID strin return org.GetLogin(), nil } +// administersOrg reports whether the credential administers the organization, +// which is what decides that this sync emits its members. A 403 means the +// membership is not visible, which reads the same way: not administered. +// +// Shared by the organization syncer, which filters on it, and by the user +// syncer, which uses it to tell whether an organization in this sync already +// emits an account. The two must agree, or the user syncer drops a principal +// whose member page never runs. +func administersOrg(ctx context.Context, client *github.Client, org string) (bool, *github.Response, error) { + membership, resp, err := client.Organizations.GetOrgMembership(ctx, "", org) + if err != nil { + if resp != nil && resp.StatusCode == http.StatusForbidden { + ctxzap.Extract(ctx).Debug("baton-github: no access to the membership of this organization, skipping it", + zap.String("org", org), + zap.String("github_error", gitHubErrorMessage(err)), + ) + return false, resp, nil + } + return false, resp, err + } + + return strings.EqualFold(membership.GetRole(), orgRoleAdmin), resp, nil +} + func newOrgNameCache(c *github.Client) *orgNameCache { return &orgNameCache{ c: c, diff --git a/pkg/connector/org.go b/pkg/connector/org.go index 4d17748d..d6013e1d 100644 --- a/pkg/connector/org.go +++ b/pkg/connector/org.go @@ -3,9 +3,7 @@ package connector import ( "context" "fmt" - "net/http" "strconv" - "strings" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" @@ -92,8 +90,6 @@ func (o *orgResourceType) List( }, nil } - l := ctxzap.Extract(ctx) - bag, page, err := parsePageToken(opts.PageToken.Token, &v2.ResourceId{ResourceType: resourceTypeOrg.Id}) if err != nil { return nil, nil, err @@ -119,20 +115,12 @@ func (o *orgResourceType) List( if _, ok := o.orgs[org.GetLogin()]; !ok && len(o.orgs) > 0 { continue } - membership, resp, err := o.client.Organizations.GetOrgMembership(ctx, "", org.GetLogin()) + // Only sync orgs that we are an admin for + isAdmin, resp, err := administersOrg(ctx, o.client, org.GetLogin()) if err != nil { - if resp != nil && resp.StatusCode == http.StatusForbidden { - l.Debug("insufficient access to list org membership, skipping org", - zap.String("org", org.GetLogin()), - zap.String("github_error", gitHubErrorMessage(err)), - ) - continue - } return nil, nil, wrapGitHubError(err, resp, "github-connector: failed to get org membership") } - - // Only sync orgs that we are an admin for - if strings.ToLower(membership.GetRole()) != orgRoleAdmin { + if !isAdmin { continue } diff --git a/pkg/connector/user.go b/pkg/connector/user.go index 705507a5..ad515044 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -10,6 +10,7 @@ import ( "github.com/conductorone/baton-github/pkg/customclient" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/pagination" "github.com/conductorone/baton-sdk/pkg/session" "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/conductorone/baton-sdk/pkg/types/sessions" @@ -116,14 +117,16 @@ const ( ) type userResourceType struct { - resourceType *v2.ResourceType - client *github.Client - graphqlClient *githubv4.Client - samlStates map[string]samlState // per-org SAML state, keyed by org name - orgCache *orgNameCache - orgs []string - customClient *customclient.Client - enterprises []string + resourceType *v2.ResourceType + client *github.Client + graphqlClient *githubv4.Client + samlStates map[string]samlState // per-org SAML state, keyed by org name + orgCache *orgNameCache + orgs []string + customClient *customclient.Client + enterprises []string + newEnterpriseClients enterpriseClientProvider + syncsEnterpriseRoles bool } func (u *userResourceType) ResourceType(_ context.Context) *v2.ResourceType { @@ -137,9 +140,19 @@ func (u *userResourceType) List(ctx context.Context, parentID *v2.ResourceId, op return nil, &resource.SyncOpResults{}, nil } - bag, page, err := parsePageToken(opts.PageToken.Token, &v2.ResourceId{ResourceType: resourceTypeUser.Id}) + bag := &pagination.Bag{} + if err := bag.Unmarshal(opts.PageToken.Token); err != nil { + return nil, nil, fmt.Errorf("baton-github: error parsing the user page token: %w", err) + } + if isEnterpriseUserPhase(bag.ResourceTypeID()) { + return u.listEnterprisePrincipalUsers(ctx, bag) + } + if bag.Current() == nil { + bag.Push(pagination.PageState{ResourceTypeID: resourceTypeUser.Id}) + } + page, err := convertPageToken(bag.PageToken()) if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("baton-github: error reading the user page number: %w", err) } orgName, err := u.orgCache.GetOrgName(ctx, opts.Session, parentID) @@ -205,11 +218,6 @@ func (u *userResourceType) List(ctx context.Context, parentID *v2.ResourceId, op return nil, nil, err } - pageToken, err := bag.NextToken(nextPage) - if err != nil { - return nil, nil, err - } - var lastGraphQLRateLimit *struct { Limit int Remaining int @@ -303,6 +311,11 @@ func (u *userResourceType) List(ctx context.Context, parentID *v2.ResourceId, op annotations.WithRateLimiting(graphqlRateLimit) } + pageToken, err := u.nextUserPageToken(ctx, bag, nextPage) + if err != nil { + return nil, &resource.SyncOpResults{Annotations: annotations}, err + } + return rv, &resource.SyncOpResults{ NextPageToken: pageToken, Annotations: annotations, @@ -366,16 +379,27 @@ func (u *userResourceType) Delete(ctx context.Context, resourceId *v2.ResourceId return annotations, nil } -func UserBuilder(client *github.Client, graphqlClient *githubv4.Client, orgCache *orgNameCache, orgs []string, customClient *customclient.Client, enterprises []string) *userResourceType { +func UserBuilder( + client *github.Client, + graphqlClient *githubv4.Client, + orgCache *orgNameCache, + orgs []string, + customClient *customclient.Client, + enterprises []string, + newEnterpriseClients enterpriseClientProvider, + syncsEnterpriseRoles bool, +) *userResourceType { return &userResourceType{ - resourceType: resourceTypeUser, - client: client, - graphqlClient: graphqlClient, - samlStates: make(map[string]samlState), - orgCache: orgCache, - orgs: orgs, - customClient: customClient, - enterprises: enterprises, + resourceType: resourceTypeUser, + client: client, + graphqlClient: graphqlClient, + samlStates: make(map[string]samlState), + orgCache: orgCache, + orgs: orgs, + customClient: customClient, + enterprises: enterprises, + newEnterpriseClients: newEnterpriseClients, + syncsEnterpriseRoles: syncsEnterpriseRoles, } } diff --git a/pkg/connector/user_test.go b/pkg/connector/user_test.go index e2e4d0e5..12552fdc 100644 --- a/pkg/connector/user_test.go +++ b/pkg/connector/user_test.go @@ -40,6 +40,8 @@ func TestUsersList(t *testing.T) { []string{organization.DisplayName}, nil, nil, + nil, + false, ) users, results, err := client.List( diff --git a/test/mocks/endpointpattern.go b/test/mocks/endpointpattern.go index 2c293942..08283600 100644 --- a/test/mocks/endpointpattern.go +++ b/test/mocks/endpointpattern.go @@ -9,6 +9,16 @@ var GetUserById = mock.EndpointPattern{ Method: methodGet, } +var GetUserMembershipsOrgsByOrg = mock.EndpointPattern{ + Pattern: "/user/memberships/orgs/{org}", + Method: methodGet, +} + +var GetOrgsMembersByOrgByUsername = mock.EndpointPattern{ + Pattern: "/orgs/{org}/members/{username}", + Method: methodGet, +} + var PutOrganizationsTeamsMembershipsByOrganizationByTeamIdByUsername = mock.EndpointPattern{ Pattern: "/organizations/{org_id}/team/{team_id}/memberships/{username}", Method: "PUT", diff --git a/test/mocks/github.go b/test/mocks/github.go index 129e119f..ca38ba81 100644 --- a/test/mocks/github.go +++ b/test/mocks/github.go @@ -423,6 +423,58 @@ func userToMembership(user *github.User) github.Membership { } } +// isOrgMember handles GET /orgs/{org}/members/{username}, which answers 204 +// for a member and 404 for anyone else. The real endpoint takes a login; this +// mock follows its usual convention that a login is the user's ID as a string, +// so a login it cannot read that way is simply not a member. +func (mgh MockGitHub) isOrgMember( + w http.ResponseWriter, + variables map[string]string, +) { + orgId, err := getCrossTableId(w, variables, "org") + if err != nil { + return + } + username, ok := getVariable(variables, "username") + if !ok { + w.WriteHeader(http.StatusNotFound) + return + } + userId, err := strconv.ParseInt(username, 10, 64) + if err != nil { + w.WriteHeader(http.StatusNotFound) + return + } + + members, ok := mgh.organizationMemberships[orgId] + if !ok || !members.Contains(userId) { + w.WriteHeader(http.StatusNotFound) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// getAuthenticatedOrgMembership handles GET /user/memberships/orgs/{org}, +// which decides whether the credential administers the organization and so +// whether the sync emits its members. +func (mgh MockGitHub) getAuthenticatedOrgMembership( + w http.ResponseWriter, + variables map[string]string, +) { + orgId, err := getCrossTableId(w, variables, "org") + if err != nil { + return + } + if _, ok := mgh.organizations[orgId]; !ok { + w.WriteHeader(http.StatusNotFound) + return + } + _, _ = w.Write(mock.MustMarshal(github.Membership{ + Role: github.Ptr("admin"), + State: github.Ptr("active"), + })) +} + func (mgh MockGitHub) getMembership( w http.ResponseWriter, variables map[string]string, @@ -748,6 +800,8 @@ func (mgh MockGitHub) Server() *http.Client { mock.DeleteOrgsMembershipsByOrgByUsername: mgh.removeUser, mock.DeleteReposCollaboratorsByOwnerByRepoByUsername: mgh.removeRepositoryCollaborator, mock.GetOrgsMembersByOrg: mgh.getUsers, + GetOrgsMembersByOrgByUsername: mgh.isOrgMember, + GetUserMembershipsOrgsByOrg: mgh.getAuthenticatedOrgMembership, mock.GetOrgsMembershipsByOrgByUsername: mgh.getMembership, mock.GetReposCollaboratorsByOwnerByRepo: mgh.getRepositoryCollaborators, mock.GetReposCollaboratorsByOwnerByRepoByUsername: mgh.getRepositoryCollaborator,