[CXH-2123] - Add provisioning for the Enterprise Owner role - GitHub Connector - #194
mateoHernandez123 wants to merge 36 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>
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
…edential decides The GHES paragraph still framed the question as whether to expose a flag there. With the app path chosen by the credential, a GHES deployment under app authentication with --enterprises inherits the Enterprise Cloud path by default and its sync fails at the invitation query. That is not a regression, since consumed-licenses already answered 404 there for either credential, but the decision now belongs to the wrapper, which is the only layer that knows which kind of instance it is pointed at. Also renames the two subtests that still described the flag. Co-authored-by: Cursor <cursoragent@cursor.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
Declining this one with the reasoning on the inline thread. The finding is accurate — an app deployment that already passes --enterprises and is not installed on the enterprise account goes from green to red on the first sync — but neither remedy it proposes is the right trade.
Degrading instead of failing would have enterprise_role report empty on a completed sync, which is what makes C1 delete the role and every grant on it, and the connector cannot tell a deployment that never had an enterprise installation from one whose installation was just removed. Restoring the opt-in protects a configuration that produces no enterprise data at all, since the consumed-licenses fallback answers 403 under app auth.
The third option is the one taken: the break is now stated in the PR description and in docs-info.md, and both failure paths return FailedPrecondition naming the fix. The inline thread stays open so this can be argued.
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
I didn't review much the main connector files since it's really hard considering the behavior should be reconsidered after my initial requests - please address my comments and I'll run a second review round
…ig and docs Enterprises is defined for baton-github-enterprise to expose, and that is the connector whose customers have an enterprise account. This one had started advertising it: the field was added to both auth groups, and docs/connector.mdx grew a capabilities row and a full setup path for a role its own audience cannot use. Both groups are now byte-identical to main again, and connector.mdx is restored to its base state -- every line this PR had added there was about enterprise. The field stays in the flat field list, so the command line and baton-github-enterprise, which declares its own copy and maps it through the shared Github struct, are unaffected. newWithGithubPAT no longer folds repeated slugs. It never did on main; the fold belongs to the app path, which serves one enterprise and would otherwise reject a count the operator never chose, and the test now covers it there. The enterprise role is also registered with Grant and Revoke on either credential again, rather than swapping in a read-only type for tokens. A token cannot reach the enterprise administrator API, so a request made with one fails naming the credential it needs -- which is more useful than hiding a role from C1 that an app in the same tenant can grant. Co-authored-by: Cursor <cursoragent@cursor.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
…er credential Both comments still said the type is registered only where the enterprise clients can be built, which stopped being true when the split was removed. Co-authored-by: Cursor <cursoragent@cursor.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
Dismissing: the blocking item is the ungated breaking change, and its own stated way to clear it is a maintainer accepting the break on the PR. That acceptance is recorded on the inline thread at pkg/connector/connector.go.
Its latest pass also narrows the impact itself: with enterprises out of this connector field groups, hosted baton-github tenants cannot reach the path. What is left is CLI users and baton-github-enterprise, where enterprises has always been exposed in both auth groups, so naming an enterprise the credential cannot reach has always been a configuration to fix. The failure returns FailedPrecondition naming the fix.
The stale-comment suggestion from the same pass was valid and is fixed in 5484b12.
The block only described the GitHub App permission, in a section that presents itself as being about a token. A CLI operator using a personal access token with --enterprises needs read:enterprise, which was documented nowhere, so the block now names what each credential requires and what it buys. The section heading still says token while listing app permission names. That predates this change and is left for its own PR. Co-authored-by: Cursor <cursoragent@cursor.com>
Superseded — see the current review report for commit
|
Connector PR Review: [CXH-2123] - Add provisioning for the Enterprise Owner role - GitHub ConnectorBlocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThe new commit ( Security IssuesNone found. Correctness Issues
Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
Blocking issues found — see the full review report
Dismissing again: same blocking item, same acceptance. The break is default-on by design — enterprises plus the credential are the configuration, and the alternative the criteria ask for is the flag that was removed in review. A maintainer accepting the break on the PR is the other way its own criteria allow to clear it, and that acceptance is on the inline thread at pkg/connector/connector.go.
No code change is pending for it. The suggestions from this pass were addressed where valid.
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review (round 5)
Resolved since last round: opt-in flag removed, Enterprises out of both UI groups, docs trimmed, failClosedOnUnreadableEnterprise now covers the clients() error, Enterprises description fixed. Still open: 1 new major below (and luisina-santos's docs thread discussion_r4107815305 is outdated and just needs her confirmation).
drafted 1 → confirmed 1, dropped 0, added 0 (verifier: gpt-5.3-codex-high)
| // the consumed-licenses API behind the token path is the only one a token | ||
| // can use. The clients are built lazily, so configuring no enterprise | ||
| // costs nothing. | ||
| newEnterpriseRoleClientsFn := func(ctx context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { |
There was a problem hiding this comment.
[major] With the opt-in gone, every GitHub App deployment that sets --enterprises now takes the Enterprise Cloud owner path. The only consumer of that field is baton-github-enterprise, and on a GHES instance that path cannot work: inviteEnterpriseAdmin, cancelEnterpriseAdminInvitation and enterpriseAdministratorInvitation don't exist there, so Grants() fails and takes the whole sync with it. On github.com or ghe.com, a deployment without the second installation on the enterprise account fails the same way. docs/docs-info.md:247 calls the GHES case "not a regression" because license already returned 404. But docs-info.md:257 says license carries OptInRequired, so on hosted tenants with selective sync it never ran, and those syncs were green. Every existing App + --enterprises tenant on the wrapper goes from green to a failing sync on the vendor bump, before anyone opts in. Before this merges, please either keep the App path behind a switch the wrapper sets (an option on cfg.Github, or a constructor option, so baton-github-enterprise can limit it to Enterprise Cloud), or link the baton-github-enterprise PR that adds that gate. Please also fix the "not a regression" sentence so it matches line 257.
8 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:
There is no separate switch.
--enterprisessays the deployment has an enterprise and the credential says which API can serve it. The role is registered with Grant and Revoke on either credential; a personal access token cannot reach the enterprise administrator API, so a request made with one fails naming the credential it needs rather than the role being hidden. Licenses are registered on the token path only, since their API answers 403 to anything else. The App path needs the app installed on both the enterprise account and the organization, with the Enterprise → People: Read and write permission.This is a behaviour change for an App deployment that already passes
--enterprises. It used to report no enterprise roles; it now reads the Owner role, and fails on the first sync if the app is not installed on the enterprise account. On a hosted tenant with selective sync that sync was green before, becauselicensecarriesOptInRequiredand its 403 never fired — green with no enterprise data in it. The error names the remedy, anddocs-info.mdstates the trade.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.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.capabilitiescommand runs without credentials, so a connector built from real config omitted every resource type its configuration did not switch on — enterprise roles and licenses need--enterprises, API keys need--sync-secrets, the usage app and its event feed need--sync-last-activity. ADefaultCapabilitiesBuilderregistered for that command alone (the pattern baton-aws uses) restores them, which is what lets the catalog advertiseenterprise_roleas provisionable at all. Each tenant still gets the truth of its own deployment:MakeGRPCServerCommandnever receives the option, and C1 overwrites the capabilities it stores from the live connector on Validate and on every sync, so a PAT deployment still reports the role as sync-only. The distinction is that the catalog describes what the connector can do once configured, while each tenant's record describes what its own deployment does. This PR also removescapabilities_and_config.yamlper review, sobaton_capabilities.jsonandconfig_schema.jsonare regenerated by hand untilbaton-adminenables the managed metadata workflow for this repo (ci_workflows.capabilitiesis currentlyfalse);docs/docs-info.mdrecords the procedure.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