Repository navigation
[CXH-2710] - Sync the users that enterprise role grants name - GitHub Connector - #195
mateoHernandez123 wants to merge 2 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
…ations Co-authored-by: Cursor <cursoragent@cursor.com>
Superseded — see the current review report for commit
|
Connector PR Review: [CXH-2710] - Sync the users that enterprise role grants name - GitHub ConnectorBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit ( I scanned the full PR diff again for security and correctness. It covers the user-syncer enterprise phases, the shared I applied the repo-local criteria, with these results:
The incremental artifact was complete (not partial, no dropped paths). Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
|
Thanks for the thorough work on this. We've decided not to fix CXH-2710, so I'm asking to close this PR without merging. C1 already drops a grant whose principal has no resource row. Uplift builds AppUsers only from user resources in the c1z, and Emitting the users would make those Owners visible in C1, but it costs API calls on every sync, and on the PAT path that means the whole licensed enterprise. It also widens the user population beyond the configured organization. We're accepting that an enterprise Owner outside the configured org is not visible in C1. The limitation is already documented in Full reasoning is on CXH-2710. Could you close this one? |
|
The mechanism you describe is what we measured, and the two calls that produce it are different. What GitHub returns
On flowchart TD
gql["POST https://api.github.com/graphql<br/>enterpriseOwners"] --> owners[mateoHernandez123 and manuel-ts-14]
members["GET https://api.github.com/orgs/mateo-baton-test/members"] --> org[org members: me and Felipe]
owners --> check{GET https://api.github.com/orgs/mateo-baton-test/members/login}
org --> check
check -->|204 me and Felipe| row[user row already exists]
check -->|404 Manuel| gap[Owner grant, no user row]
row --> shown[C1 shows the Owner]
gap --> main[main: C1 drops Manuel]
gap --> pr[this PR: emit Manuel, C1 shows all three]
What
This PR leaves What we ran Before this change, C1 on the test tenant showed one Owner, me. Manuel's grant was in the c1z and did not become a binding. We then invited FeliLucero1 (database id 162376288), who is a member of He would have shown up on The case I want on the thread is the one where I am not an Owner. App auth on this tenant is two membership checks. The token path is the expensive one, and we have not run it live: List enterprise consumed licenses walks every licensed account, because |
| 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)), |
There was a problem hiding this comment.
[low-confidence] The enterprise_role gate is resolved once, at construction, from cli.ConnectorOpts.SyncResourceTypeIDs. That field only carries the local --sync-resource-types flag (or the lambda effective config). In self-hosted daemon mode (--client-id), the per-task selection from C1 is applied later, inside the SDK syncer (c1api/full_sync.go: c.task.GetSyncFull().GetSyncResourceTypeIds() -> sdkSync.WithSyncResourceTypes), and never reaches ConnectorOpts. So SyncFilterIsExplicit() is false, WillSyncResourceType("enterprise_role") returns true, and with --enterprises set the user syncer walks the enterprise on every sync even when the admin has left enterprise_role deselected. On the PAT path that walk covers the whole licensed enterprise: one IsMember per org plus one Users.Get per seat, which the new docs put at six figures of REST calls on a large enterprise. It also adds users that no grant references. This contradicts the PR description's claim that the walk runs only when C1 selects the type. Please either derive the decision from something that reflects the actual sync, or make the walk opt-in explicitly (for example a config flag). At minimum, document that daemon-mode deployments always run the walk. Also confirm that hosted lambda deployments actually populate sync-resource-types from the UI selection; otherwise the same applies there.
| ctx context.Context, | ||
| enterprise string, | ||
| ) (*customclient.EnterpriseAdminClient, error) { | ||
| clients, err := u.newEnterpriseClients(ctx, []string{enterprise}) |
There was a problem hiding this comment.
adminClient calls u.newEnterpriseClients on every Owners and pending-invitee page, and continueWithEnterprisePrincipals calls it again at the end of each org's members. newEnterpriseRoleClients is not memoized. Each call runs GetEnterpriseInstallation, mints a new installation access token, builds a new uhttp client, and runs VerifyOrganization plus ResolveEnterpriseNodeID (two GraphQL queries). The pending phase pages through the whole Enterprise.members list 100 at a time, so a 50k-member enterprise pays roughly 500 token mints and about 1,000 extra GraphQL verification queries per sync on top of the walk itself. Any transient failure in that setup path also fails the user page. The role syncer avoids this with enterpriseRoleResourceType.clients(), which builds once and caches. Please cache the built clients on the user syncer the same way, for example a mutex-guarded map keyed by enterprise, or share the role syncer's cache. A per-process cache stays correct under the multi-process model the PR cites, because a missing entry is simply rebuilt.
|
Closing this without merging. I agree with keeping GitHub separates the two roles. Abilities of roles in an enterprise gives the enterprise Owner control of the enterprise: administrators, adding and removing organizations, removing members from all organizations, enterprise settings, policies across organizations, billing and security. The same page says they "do not have access to organization settings or content by default, but they can gain access by joining any organization." Managing your role in an organization owned by your enterprise states it per organization. With no organization role, an Owner "cannot access organization content or repositories" but can manage the enterprise settings and policies that affect it. Joining as a member gives access to repositories. Joining as an organization owner gives the organization's settings and access management. That is exactly the test tenant:
Manuel is a real enterprise Owner with no access to This PR would add a second user phase, a membership check per Owner and, on the token path, a walk of every consumed license in the enterprise. All of that is to show people who cannot see the organization this connector is configured for. #194 already chose to leave the user population as the configured organization, and What stays true for QA: an enterprise Owner outside the configured organization does not show up on the Enterprise Owner entitlement in C1, and if they are the only Owner the entitlement is empty. That is expected for an organization-scoped connector, not a defect in how Owners are listed. flowchart TD
owner[Enterprise Owner] --> member{member of the configured org?}
member -->|yes| visible[user row from the org member list, C1 shows the Owner]
member -->|no| outside[no org content or repositories]
outside --> policy[still manages enterprise policy and can join the org]
outside --> c1[C1 drops the grant: expected for an org-scoped sync]
|
6 participants
Description
CXH-2710 reports a grant that points at a user the sync never emits. With
--enterprisesset, the connector emits an Owner grant for every owner of the enterprise. It emits user resources only for members of the configured organization. An owner outside that organization therefore gets a grant whose principal has no resource row.The SDK reports ingest invariant I9 on every sync. A local grant against that principal exits 5 with
pebble: not found, because the bundle does not contain the principal. QA found this onbaton-github-enterprisev0.1.1, which vendors baton-github v0.4.8.This change makes the user syncer emit the principals that enterprise-role grants name. It adds no configuration and no new credential scope.
Sync:
user) — after an organization's member pages finish, the syncer walks the enterprise and emits the principals its grants name. Under GitHub App authentication these are the owners of the enterprise and the enterprise members that hold a pending Owner invitation. Under a personal access token these are the consumed-licenses users that hold an enterprise role. The resource ID is the GitHub database ID the grant already uses. These users get no organization membership.enterprise_role) — unchanged surface. The walk runs only when C1 selects this type, because without it no grant names these principals.org), Teams (team), Repositories (repository), Organization roles (org_role), Invitations (invitation), GitHub Apps (app), Licenses (license) — unchanged.Provisioning:
Unchanged. The fix removes the cause of a failure on the existing path: a grant or a revoke against an enterprise owner outside the configured organization failed before, because the SDK could not resolve the principal in the bundle.
Auth:
Unchanged. No new flag and no new scope. The membership check calls
GET /orgs/{org}/members/{login}, which needs the same Members read permission that the organization member list already needs.flowchart TD members[organization member pages] --> last{last page?} last -->|no| members last -->|yes| gate{enterprise_role selected?} gate -->|no| done[user list ends] gate -->|setup error| skip[skip the phases] gate -->|yes| cred{credential?} cred -->|GitHub App| owners[Owners, then pending invitees] cred -->|token| licenses[licensed role holders] owners --> check{already an organization member?} licenses --> check check -->|yes| keep[keep the member-page user] check -->|no| emit[emit the user]The walk runs only when C1 selects
enterprise_role. A setup error skips the phases, and the role type skips its grants for the same reason, so no grant is left without a user. A principal an organization already emits is left alone, because a second write would drop the SAML identity the member page resolved.Architecture highlights:
GET /orgs/{org}/members/{login}per principal. Member pages resolve the user's SAML identity, and the resource store keeps the last write, so a second emission would drop that identity. The cost follows the number of enterprise administrators, not the size of the organization.administersOrghelper now serves both. Counting an organization this sync leaves out would drop the user and restore the defect.NotFoundis converted at the boundary. The SDK treatsNotFoundas a warning and completes the sync, which would drop the members the page already fetched and make C1 delete them.ConnectorOpts.WillSyncResourceType("enterprise_role"), threaded as a resolved bool.enterprise_roleis 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.Memberis the role GitHub reports for every license-consuming account and the role sync emits a grant for each one.docs/docs-info.mdstates the cost and asks the operator to budget for it.Useful links: