[CXH-2123] - Add provisioning for the Enterprise Owner role - GitHub Connector - #194
mateoHernandez123 wants to merge 22 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
8738a07 to
04872b2
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
…the path Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Automated review — draft by Opus, cross-verified by Codex (gpt-5.3-codex-high).
Adds opt-in (--enable-enterprise-owner-provisioning) Grant/Revoke for the built-in Enterprise Owner role under GitHub App auth, via GraphQL (inviteEnterpriseAdmin with fallback to updateEnterpriseAdministratorRole, demote to UNAFFILIATED / cancelEnterpriseAdminInvitation on revoke), plus App-path sync of owners and pending invitations. Linked ticket CXH-2123 found; both itemized work items (GraphQL client calls, Grant/Revoke on the enterprise role builder) are covered, and the ticket's implementation notes match the code. Idempotency is handled well — state is read before and after each mutation, GrantAlreadyExists/GrantAlreadyRevoked are returned correctly, NOT_FOUND on revoke counts as success, paging is bounded, errors are wrapped, rate limits annotated, CI is green.
The concerns are about deployability and blast radius, not the mutations themselves:
- High: turning the feature on requires
--enterprisesunder App auth, which also registers the license syncer — and that syncer hard-fails under App auth, so the documented setup for this feature doesn't actually sync. - Medium: Grant/Revoke also gets advertised on
enterprise_rolefor existing PAT deployments that can never provision it — every request would fail withFailedPrecondition. - Medium, low-confidence: the pending-invitation pass scans the whole enterprise membership (not just the configured org) and can emit grants for principals the sync never created.
- Low: a rejected
InviteOwnererror is only Debug-logged before falling through toUpdateRole, losing the real rejection reason from the operator-visible error. - Low: a code comment claims a resource-type failure doesn't fail the whole sync, but the fail-closed behavior this PR depends on for safety requires exactly that — the comment contradicts the actual (correct) guarantee and invites a future regression.
Verification: 4 confirmed, 1 uncertain (kept, tagged low-confidence) — no new majors from the verifier.
… be used Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… are opted into Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… it removed Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Connector PR Review: [CXH-2123] - Add provisioning for the Enterprise Owner role - GitHub ConnectorBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryWhen Security IssuesNone found. The aliased invitation query passes logins only as GraphQL variables, and the Correctness IssuesNone found. Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
6 participants
Description
Adds Grant and Revoke for the built-in Enterprise Owner role under GitHub App authentication, which CXH-2123 asks for on behalf of DoorDash: they want that role requestable and time-bound from C1.
The ticket's implementation note pointed at a GraphQL mutation called
updateEnterpriseOwnerMembership. That mutation does not exist, and the real model is more involved: GitHub has no single operation that assigns Owner. A member becomes one by accepting an invitation, and GitHub rejects that invitation outright when the user already holds an enterprise administrator role such as Billing manager. An installation token cannot read that prior role —Enterprise.ownerInforesolves tonull— so the connector refuses that case withFailedPreconditionrather than promoting in place: Revoke could only demote toUNAFFILIATED, discarding a role the grant never gave. I corrected the ticket description with what the API actually offers.Contrary to the original support thread, this does not require a personal access token. It works with a GitHub App installed on both the enterprise account and the organization.
Sync:
enterprise_role) — under GitHub App authentication the connector now lists the built-in Owner role and emits its grants. Owners are read fromOrganization.enterpriseOwnerswith the organization installation token;Enterprise.members(role: OWNER)looks like the right field but returns owners of organizations inside the enterprise, not owners of the enterprise account. Under PAT authentication the resource type is unchanged.assignedentitlement as accepted owners. C1 has no pending state for a grant, so an invitee is indistinguishable from a real owner until they accept or the invitation lapses. That is a deliberate trade: emitting nothing would leave the request invisible in C1 for up to seven days with no record that it was made. An access review or offboarding sweep will count an invitee as holding Owner — documented indocs/docs-info.mdand in the enterprise connector docs.Provisioning:
UNAFFILIATED, which keeps their enterprise membership instead of evicting them, and cancels an unaccepted invitation.GrantAlreadyExistswhen the user already holds the role or already has a pending invitation,GrantAlreadyRevokedwhen neither is present. ANOT_FOUNDfrom either mutation is treated as success, because it means the requested state is already in place.What C1 shows for each state:
C1 has no pending state for a grant — it either exists or it does not — so an invitation and an accepted owner map onto the same grant:
The grant ID being stable across the second row matters: if it changed on acceptance, C1 would read the transition as a revoke followed by a new grant and would corrupt the history exactly where it is most useful. Nothing tracks an expiry — GitHub stops resolving an invitation once it is accepted, cancelled or expired, so it simply stops being emitted.
Every row was exercised against a live GitHub Enterprise Cloud account with the app installed on both levels, driven end to end through a full ConductorOne stack: granting to an org member created the invitation and C1 kept the grant across the following sync, a teammate accepting it turned them into an active owner under the same grant ID, and revoking cleared each state through its own mutation. Also covered live: re-granting while an invitation is pending returns
GrantAlreadyExistswithout sending a second invitation, revoking with nothing to revoke reports success rather than an error, and revoking an active owner leaves their enterprise membership intact.Auth:
Enterprise Owner provisioning is opt-in:
--enable-enterprise-owner-provisioning, off by default. While it is off the connector behaves exactly as it did before this change, so upgrading cannot break an existing deployment. Turning it on requires GitHub App authentication with the app installed on both the enterprise account and the organization, and the Enterprise → People: Read and write permission.Once enabled, a missing enterprise installation fails the sync rather than emitting no owners. That is deliberate: C1 deletes every resource of a type that a completed sync did not report, so finishing the sync while reading nothing would silently drop the Owner role and every grant on it — and GitHub answers
404for an uninstalled app, a revoked permission and a slug typo alike, so the connector cannot tell them apart. Failing keeps the sync from completing, so nothing is deleted. The flag is what confines that failure to operators who asked for the capability.Only one enterprise can be served per connector under App authentication, because owners are read through the single configured organization and an organization belongs to exactly one enterprise. A configuration naming several is rejected with an explanatory error. The PAT path still accepts a list.
Architecture highlights:
Enterprise.ownerInfo.pendingAdminInvitationsis the only connection GitHub offers and it isnullfor installation tokens, so the sync resolves invitations by asking about the enterprise members, aliasing up to 100 logins into one request — measured at a single rate-limit point. An invitation addressed to somebody outside the enterprise is therefore invisible to the sync; invitations created from C1 are always visible, because C1 grants to a user it has already synced.Grants()walks owners and invitations as two phases of one page token viapagination.Bag.Unavailable. It is layered only on the enterprise clients: the shared GraphQL client is untouched becauseuser.godetects enterprise SAML by matching that error's text. The aliased batch deliberately bypasses it, since it always carriesNOT_FOUNDentries, and filters those out before classifying the rest — leaving them in would let them claim the code for the whole response, andNOT_FOUNDis the one code the SDK downgrades to a warning.customclientnow resolves URLs against the go-github client'sBaseURLand escapes each path segment, so these endpoints follow--instance-urlinstead of hardcodingapi.github.com.licenseresource type still cannot sync under GitHub App authentication, because GitHub does not offer theenterprise_administrationpermission to Apps. Pre-existing and unrelated to this change, but it means that type has to stay disabled when running the App path with enterprises configured.Useful links:
inviteEnterpriseAdminupdateEnterpriseAdministratorRolecancelEnterpriseAdminInvitation