From 8609572bc1b9b230209af72e98634dc8023a852b Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 15:18:32 -0300 Subject: [PATCH 01/22] feat(github): provision the enterprise owner role under github app auth Co-authored-by: Cursor --- README.md | 3 + docs/docs-info.md | 334 ++++++ pkg/connector/connector.go | 160 ++- .../enterprise_administrator_client.go | 657 +++++++++++ .../enterprise_installations_test.go | 181 +++ pkg/connector/enterprise_role.go | 481 +++++++- pkg/connector/enterprise_role_test.go | 1021 +++++++++++++++++ pkg/connector/graphql_transport.go | 131 +++ pkg/connector/graphql_transport_test.go | 72 ++ pkg/connector/helpers.go | 44 + pkg/connector/token_refresh.go | 17 +- pkg/customclient/client.go | 103 +- pkg/customclient/models.go | 10 + 13 files changed, 3166 insertions(+), 48 deletions(-) create mode 100644 docs/docs-info.md create mode 100644 pkg/connector/enterprise_administrator_client.go create mode 100644 pkg/connector/enterprise_installations_test.go create mode 100644 pkg/connector/enterprise_role_test.go diff --git a/README.md b/README.md index 98ce535b..be6a6768 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,10 @@ baton resources - Users - Teams - Repositories +- 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 and licenses, only when `--enterprises` is set 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 new file mode 100644 index 00000000..b0658ab0 --- /dev/null +++ b/docs/docs-info.md @@ -0,0 +1,334 @@ +# GitHub Connector Setup Guide + +--- + +## Requirements + +- A **GitHub** organization, and for enterprise features a **GitHub Enterprise Cloud** account +- Either a **personal access token (classic)** or a **GitHub App** owned by the organization or the enterprise +- For the built-in Enterprise Owner role: a GitHub App installed on **both** the enterprise account and the organization + +--- + +## Connector capabilities + +1. **What resources does the connector sync?** + This connector syncs: + - Organizations (the orgs the credential can administer, or the ones named in `--orgs` / `--org`) + - Users (organization members, with SAML identity emails when SAML is configured) + - Invitations (users invited to an organization who have not accepted, and invitations that expired) + - Teams (including nested teams, with their parent team as the parent resource) + - Repositories (optionally excluding archived ones) + - Organization roles (GitHub's built-in and custom org roles, also called "enterprise licenses" in GitHub's docs) + - Enterprise roles (only when `--enterprises` is set) + - Licenses (enterprise seat consumption, only when `--enterprises` is set) + - GitHub Apps installed on the organization + - API keys (fine-grained personal access tokens with access to the org, only when `--sync-secrets` is set) + +2. **Can the connector provision any resources? If so, which ones?** + The connector can provision: + - Organization membership and admin role via Grant and Revoke. Granting to a user who is not yet a member sends an org invitation + - Team membership (`member`, `maintainer`) via Grant and Revoke + - Repository access (`pull`, `triage`, `push`, `maintain`, `admin`) via Grant and Revoke, for users and for teams + - Organization role assignment via Grant and Revoke + - The built-in Enterprise **Owner** role via Grant and Revoke, GitHub App authentication only + - Accounts, via the invitation resource type: `CreateAccount` sends an org invitation, and `Delete` cancels it + - User removal from the organization via `Delete` on the user resource type + +3. **Does the connector emit any event feeds?** + Yes, when `--sync-last-activity` is set. `github_usage_event_feed` streams member activity from each organization's audit log as usage events, which is what drives last-login and activity reporting. The flag also registers a synthetic app resource that exists only to carry those events. + +4. **Does the connector support grant expansion?** + Yes, in three places: + - Repository permissions implied by the organization's `default_repository_permission` are emitted against the organization and expanded through the org's `member` and `admin` entitlements, so every member's baseline repository access surfaces without enumerating collaborators + - An organization role assigned to a team is expanded through that team's `member` and `maintainer` entitlements + - An enterprise license held by a role is expanded through that role's `assigned` entitlement + + All three are `Shallow`, so the SDK does not recurse further. `--direct-collaborators-only` leans on this expansion instead of fetching per-team repository detail, which cuts API calls on large organizations. + +--- + +## Connector credentials + +1. **What credentials or information are needed to set up the connector?** + This connector accepts one of two authentication methods. + + **Personal access token (classic)** + + **Args**: + `--token` — the GitHub personal access token + `--instance-url` — the GitHub instance URL, defaults to `https://github.com` + `--orgs` — optional, limits syncing to specific organizations + + **GitHub App** + + **Args**: + `--app-id` — the GitHub App ID + `--app-privatekey-path` — path to the App's private key `.pem` + `--org` — required, the single organization the App is installed on + `--instance-url` — the GitHub instance URL, defaults to `https://github.com` + + Common to both: + `--enterprises` — enterprises to sync enterprise roles and licenses for + `--sync-secrets` — sync fine-grained personal access tokens as API keys + `--sync-last-activity` — emit the audit-log usage event feed. Hidden from `--help` and from the GUI config here, because it only applies to GitHub Enterprise audit-log access; `baton-github-enterprise` sets it directly instead of going through this CLI layer + `--omit-archived-repositories` — skip archived repositories + `--direct-collaborators-only` — reduce API calls on large organizations + +2. **For each item in the list above:** + - **How does a user create or look up that credential or info?** + + **Personal access token (classic):** + 1. In GitHub, click your profile photo, then **Settings** + 2. Go to **Developer settings** > **Personal access tokens** > **Tokens (classic)** + 3. Click **Generate new token** > **Generate new token (classic)** + 4. Name the token, optionally set an expiration, and select the scopes below + 5. Click **Generate token** and copy it — it is shown only once + + **GitHub App:** + 1. For enterprise features, create the App under the enterprise account: **Settings** > **GitHub Apps** > **New GitHub App**. Otherwise create it under the organization + 2. Give it a globally unique name, and use a placeholder URL for Homepage and Callback + 3. Uncheck **Active** under Webhook + 4. Select the permissions below + 5. Under **Where can this app be installed?** choose **Only on this account** + 6. Create the App, copy the **App ID**, then generate and save a **private key** + 7. Install the App. For enterprise features install it twice: once on the **enterprise account**, once on the **organization** the connector syncs + + - **Does the credential need any specific scopes or permissions?** + + **Personal access token (classic)** scopes: + - `repo` — all + - `admin:org` — all for organization-level provisioning, otherwise `read:org` + - `user` — all + - `admin:enterprise` — `read:enterprise`, for enterprise roles and licenses + + If the organization uses SAML single sign-on, the token must also be authorized for that organization. + + **GitHub App** permissions: + - Repository: **Administration** read and write (implies **Metadata** read) + - Organization: **Administration** read-only (detects SAML/SSO configuration), **Members** read and write, **Custom organization roles** read and write + - Enterprise: **Enterprise people** read and write, required to sync and provision the built-in Owner role + + - **Is the list of scopes or permissions different to sync (read) versus provision (read-write)?** + Yes. Read-only syncing needs `read:org` rather than `admin:org` on a PAT, and read-only equivalents of the App's organization permissions. Provisioning the built-in Enterprise Owner role requires **Enterprise people: read and write**; read-only is not enough because the connector issues invitations and role mutations. + + - **What level of access or permissions does the user need in order to create the credentials?** + A personal access token must be created by a user with **Enterprise Owner** access when enterprise features are used, and organization admin access otherwise. Creating an enterprise-owned GitHub App requires someone who can manage GitHub Apps for the enterprise; installing it on an organization requires **Org Owner** on that organization. + +--- + +## Resource Details + +### Organizations + +- **Resource type ID**: `org` +- **Description**: The GitHub organizations the credential can administer, or the ones named in `--orgs` / `--org` +- **Traits**: None +- **Entitlements**: `member` (assignment) and `admin` (permission) +- **Grants**: One grant per organization member for their role. Members are read from the members list; the connector distinguishes admins from plain members +- **Children**: Users, Invitations, Teams, Repositories, Organization roles, GitHub Apps, API keys +- **Provisioning**: Grant adds the member or promotes them to admin. A user who is not yet a member is sent an organization invitation instead, so the membership only exists once they accept. Revoke removes the organization membership + +### Users + +- **Resource type ID**: `user` +- **Description**: Members of the synced organizations +- **Traits**: User trait with login, email and profile +- **Parent**: Organization +- **Entitlements**: None +- **Grants**: None. Access is emitted by the organization, team, repository and role builders +- **Provisioning**: `Delete` removes the user from the organization +- **Note**: When the organization has SAML single sign-on, emails are read from the SAML identity rather than the public profile. Enterprise-level SAML is read from the enterprise consumed-licenses API, which is PAT-only; when that is unavailable the connector falls back to the REST email + +### Invitations + +- **Resource type ID**: `invitation` +- **Description**: Users invited to an organization who have not accepted, and invitations GitHub expired +- **Traits**: User trait with `RESOURCE_STATUS_PENDING`, plus `invitation_status` and `invitation_expires_at` profile fields +- **Parent**: Organization +- **Entitlements**: None +- **Grants**: None +- **Provisioning**: `CreateAccount` sends an organization invitation; `Delete` cancels it +- **Note**: Organization invitations expire seven days after creation. Because a pending invitation disappears once accepted, this resource type opts out of sync anomaly detection + +### Teams + +- **Resource type ID**: `team` +- **Description**: GitHub teams, including nested teams +- **Traits**: Group trait +- **Parent**: Organization, or the parent team for a nested team +- **Entitlements**: `member`, `maintainer` (permission) +- **Grants**: One grant per team member for their role +- **Provisioning**: Grant and Revoke add or remove team membership + +### Repositories + +- **Resource type ID**: `repository` +- **Description**: Repositories of the synced organizations +- **Traits**: None +- **Parent**: Organization +- **Entitlements**: `pull`, `triage`, `push`, `maintain`, `admin` (permission), grantable to users and teams, and declared as an exclusion group because a principal holds one level at a time +- **Grants**: One grant per collaborator for their permission level, and one per team with repository access. The organization's `default_repository_permission` is expanded into the cumulative levels it implies and emitted against the organization, annotated as expandable through the org's `member` and `admin` entitlements +- **Provisioning**: Grant and Revoke add or remove a collaborator, or a team's repository access +- **Note**: `--omit-archived-repositories` skips archived repositories. `--direct-collaborators-only` relies on grant expansion for team access instead of fetching per-team detail + +### Organization roles + +- **Resource type ID**: `org_role` +- **Description**: GitHub's built-in and custom organization roles. GitHub's documentation also calls these "enterprise licenses" +- **Traits**: Role trait +- **Parent**: Organization +- **Entitlements**: `assigned` (assignment) +- **Grants**: One grant per user and per team assigned to the role. A team's grant is expandable through that team's `member` and `maintainer` entitlements +- **Provisioning**: Grant and Revoke assign or unassign the role + +### Enterprise roles + +- **Resource type ID**: `enterprise_role` +- **Description**: Roles of an enterprise account. Only synced when `--enterprises` is set +- **Traits**: Role trait +- **Entitlements**: `assigned` (assignment) +- **Grants**: Under PAT authentication, one grant per user holding each role, read from the enterprise consumed-licenses API. Under GitHub App authentication, only the built-in **Owner** role is visible, and its grants are the users who hold it plus the users who have been invited and have not accepted. The two are emitted against the same entitlement and C1 cannot tell them apart +- **Provisioning**: Only the built-in **Owner** role, and only with GitHub App authentication. See [Enterprise Owner provisioning](#enterprise-owner-provisioning) +- **Limitation**: A GitHub App cannot read `Enterprise.ownerInfo`, so under App authentication the connector sees only the Owner role, not billing managers or custom enterprise roles + +### Licenses + +- **Resource type ID**: `license` +- **Description**: Enterprise seat consumption. Only synced when `--enterprises` is set +- **Traits**: License profile trait +- **Entitlements**: `assigned` (assignment) +- **Grants**: One grant for the enterprise member role holding the license, expandable through that role's `assigned` entitlement +- **Limitation**: Requires a personal access token. GitHub does not offer the enterprise administration permission to GitHub Apps, so this resource type cannot sync with an App installation token, and its failure fails the whole sync. Customers using a GitHub App with `--enterprises` set must disable this resource type in the connector's resource capabilities in C1 + +### GitHub Apps + +- **Resource type ID**: `app` +- **Description**: GitHub Apps installed on the organization +- **Traits**: App trait, annotated as a non-human identity of type app registration +- **Parent**: Organization +- **Entitlements**: None +- **Grants**: None + +### API keys + +- **Resource type ID**: `api-key` +- **Description**: Fine-grained personal access tokens with access to the organization. Only synced when `--sync-secrets` is set +- **Traits**: Secret trait +- **Parent**: Organization +- **Entitlements**: None +- **Grants**: None + +--- + +## Enterprise Owner provisioning + +Only the built-in **Owner** role of an enterprise is provisionable, and only under GitHub App authentication. The design is shaped by three GitHub constraints: + +**`Enterprise.ownerInfo` is invisible to an App.** It holds `admins` and `pendingAdminInvitations`, and it resolves to `null` for an installation token regardless of which permissions the App declares. + +**`Enterprise.members(role: OWNER)` is the wrong list.** That argument is an `EnterpriseUserAccountMembershipRole`, whose `OWNER` means "owner of an *organization* in the enterprise" — a different enum from the `EnterpriseAdministratorRole` the mutations take. Owners are read from `Organization.enterpriseOwners` instead, which returns every owner of the organization's enterprise account annotated with their role in that organization. The query must not pass `organizationRole`, because that would drop owners who are not owners of the organization. + +**There is no single operation that assigns Owner.** Someone who already administers the enterprise, such as a billing manager, is promoted in place. Anyone else can only be invited, and the role lands when they accept. Which case applies is only readable through `ownerInfo`, so Grant attempts the invitation first and promotes as the fallback, keyed on the `FailedPrecondition` that GitHub's `UNPROCESSABLE` error maps to. + +Consequences worth knowing: + +- Grant returns the grant in both cases: when it promoted an administrator, and when it only created an invitation. `Grants()` matches that and emits pending invitations alongside accepted Owners, so C1 keeps a record of the request from the moment it is made +- Revoke clears both states rather than treating them as alternatives: it demotes an active Owner to `UNAFFILIATED`, which keeps them as a member of the enterprise rather than evicting them, and cancels an unaccepted invitation. A `NOT_FOUND` on either is success, because it means the state being asked for is already in place +- Reading owners uses the **organization** installation token and every mutation uses the **enterprise** installation token; the enterprise token is rejected on organization fields. Startup verifies that the configured organization belongs to the configured enterprise, and a failure there fails only this resource type +- Only one enterprise can be served under App authentication, because the owners are read through the single configured organization and an organization belongs to exactly one enterprise. A configuration naming several is rejected while the clients are built, which fails this resource type with an explanatory error rather than failing later on a check the operator cannot satisfy. The PAT path does accept a list + +### Pending invitations look the same as real access + +C1 has no pending state for a grant, so an invitation nobody has accepted and an accepted Owner are emitted as the same grant on the same entitlement, and nothing distinguishes them. That is a deliberate trade: emitting nothing until the invitee accepts would leave the request invisible for up to seven days and leave reviewers no record that it was made. + +- An access review or an offboarding sweep counts an invitee as holding Owner. They do not hold it — GitHub assigns the role only on acceptance +- Nothing tracks an expiry. GitHub stops resolving an invitation once it is accepted, cancelled, or expired, so it simply stops being emitted and C1 drops the grant on that sync. `EnterpriseAdministratorInvitation` exposes no `expiresAt`, so there is nothing to compute from either + +### The list of pending invitations cannot be complete + +`Enterprise.ownerInfo.pendingAdminInvitations` is the only connection of invitations GitHub offers, and it resolves to `null` for an installation token. The root `enterpriseAdministratorInvitation` field answers for one login at a time, so the sync resolves invitations by asking about the enterprise members, batching up to 100 logins into one aliased request — GitHub charges that whole request a single rate-limit point. + +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. Invitations that C1 itself creates are always visible, because C1 grants to a user it has already synced. + +--- + +## Authentication + +The connector supports two methods, selected by which credentials are supplied. + +1. **Personal access token (classic)**: a single bearer token used for REST and GraphQL. + +2. **GitHub App**: the App's private key signs a JWT, which is exchanged for installation access tokens. Tokens are refreshed automatically when they expire. A connector using enterprise features holds two installation tokens at once, one for the organization and one for the enterprise account, because GitHub splits the data between them. + +GraphQL is used for SAML identity lookups, the audit log, and all enterprise owner reads and mutations. Everything else is REST. + +--- + +## API Endpoints Used + +**REST** (via `go-github`): + +- `GET /user`, `GET /users/{username}`, `GET /user/{id}` — resolve users +- `GET /organizations`, `GET /orgs/{org}`, `GET /organizations/{id}` — list and resolve organizations +- `GET /orgs/{org}/members` — organization members +- `GET /orgs/{org}/memberships/{username}` — a member's role +- `PUT /orgs/{org}/memberships/{username}` — promote to admin (Grant) +- `DELETE /orgs/{org}/memberships/{username}` — remove membership (Revoke, user Delete) +- `POST /orgs/{org}/invitations` — invite a user (Grant, CreateAccount) +- `GET /orgs/{org}/invitations`, `GET /orgs/{org}/failed_invitations` — pending and expired invitations +- `DELETE /orgs/{org}/invitations/{invitation_id}` — cancel an invitation (invitation Delete) +- `GET /orgs/{org}/teams`, `GET /teams/{team_id}` — teams +- `GET /teams/{team_id}/members` — team members +- `PUT /teams/{team_id}/memberships/{username}`, `DELETE /teams/{team_id}/memberships/{username}` — team membership (Grant, Revoke) +- `GET /orgs/{org}/repos`, `GET /repositories/{id}` — repositories +- `GET /repos/{owner}/{repo}/collaborators`, `GET /repos/{owner}/{repo}/collaborators/{username}/permission` — repository access +- `PUT /repos/{owner}/{repo}/collaborators/{username}`, `DELETE /repos/{owner}/{repo}/collaborators/{username}` — repository access (Grant, Revoke) +- `GET /repos/{owner}/{repo}/teams` — teams with repository access +- `PUT /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}`, `DELETE /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}` — team repository access (Grant, Revoke) +- `GET /orgs/{org}/organization-roles` — organization roles +- `GET /orgs/{org}/organization-roles/{role_id}/users`, `.../teams` — role assignments +- `GET /orgs/{org}/installations` — installed GitHub Apps, requires `organization_administration=read` +- `GET /orgs/{org}/personal-access-tokens` — fine-grained PATs, only with `--sync-secrets` +- `GET /orgs/{org}/audit-log` — organization audit log +- `GET /app/installations` — the App's installations, authenticated with the App JWT, used to find the enterprise installation +- `GET /enterprises/{enterprise}/consumed-licenses` — enterprise license consumption and enterprise SAML identities. **PAT only** + +**GraphQL**: + +- `organization(login:) { samlIdentityProvider { externalIdentities } }` — SAML identity emails +- `organization(login:) { enterpriseOwners }` — the enterprise account's owners, read with the organization token +- `enterprise(slug:) { id }`, `enterprise(slug:) { organizations }` — enterprise node ID, and the organization-belongs-to-enterprise check +- `enterpriseAdministratorInvitation(enterpriseSlug:, userLogin:, role:)` — a pending Owner invitation, for one login; the sync aliases up to 100 of these into a single request +- `enterprise(slug:).members` — the candidate logins the pending-invitation lookup asks about +- `inviteEnterpriseAdmin`, `updateEnterpriseAdministratorRole`, `cancelEnterpriseAdminInvitation` — Owner Grant and Revoke + +--- + +## Pagination + +- REST endpoints use GitHub's `page` and `per_page` parameters, 100 per page, driven one page per SDK call +- `GET /enterprises/{enterprise}/consumed-licenses` is 1-indexed; page 0 is undocumented and can repeat page 1, producing duplicates +- GraphQL connections use cursor pagination, 100 per page, with the `endCursor` passed through as the SDK page token +- The audit log event feed keeps its own cursor, which carries both the current organization index and GitHub's `after` token, so the feed resumes mid-organization + +--- + +## Rate Limits + +- REST: 5,000 requests per hour for a PAT. A GitHub App installation gets a larger budget that scales with the account; installations on this connector's test enterprise reported 15,000 +- GraphQL: a separate points-based budget, reported per query in the `rateLimit` field; App installations reported 10,000 +- The connector returns GitHub's rate limit headers and the GraphQL `rateLimit` values to the SDK as rate limit annotations, so it backs off rather than failing the sync. GraphQL errors arriving inside an HTTP 200 body are classified, so a rate limit surfaces as retryable rather than as an opaque failure + +--- + +## API Documentation + +**Official GitHub API references:** + +- **REST**: https://docs.github.com/en/rest +- **GraphQL**: https://docs.github.com/en/graphql +- **Enterprise administration (GraphQL)**: https://docs.github.com/en/graphql/reference/enterprise-admin +- **Permissions required for GitHub Apps**: https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps +- **Inviting people to manage your enterprise**: https://docs.github.com/en/enterprise-cloud@latest/admin/managing-accounts-and-repositories/managing-users-in-your-enterprise/inviting-people-to-manage-your-enterprise +- **Enterprise licensing (REST)**: https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/license diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index b1bd1017..105cd795 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -9,7 +9,6 @@ import ( "fmt" "io" "net/http" - "net/url" "strings" "time" @@ -127,6 +126,7 @@ type GitHub struct { omitArchivedRepositories bool directCollaboratorsOnly bool enterprises []string + newEnterpriseRoleClients enterpriseClientProvider syncLastActivity bool } @@ -156,7 +156,10 @@ func (gh *GitHub) ResourceSyncers(ctx context.Context) []connectorbuilder.Resour if len(gh.enterprises) > 0 { resourceSyncers = append(resourceSyncers, - EnterpriseRoleBuilder(gh.client, gh.appClient, gh.customClient, gh.enterprises), + EnterpriseRoleBuilder( + gh.client, gh.appClient, gh.customClient, gh.enterprises, + gh.newEnterpriseRoleClients, + ), LicenseBuilder(gh.customClient, gh.enterprises), ) } @@ -288,9 +291,9 @@ func (gh *GitHub) validateAppCredentials(ctx context.Context) (annotations.Annot l := ctxzap.Extract(ctx) _, _, err := gh.customClient.ListEnterpriseConsumedLicenses(ctx, gh.enterprises[0], 1) if err != nil { - l.Debug("baton-github: enterprise features (--enterprises) require a Personal Access Token. "+ - "GitHub App authentication cannot access the consumed-licenses API. "+ - "Either switch to PAT auth or remove the --enterprises flag.", + l.Debug("baton-github: enterprise license data requires a Personal Access Token. "+ + "GitHub App authentication cannot access the consumed-licenses API, "+ + "so the license resource type cannot sync.", zap.Error(err)) } } @@ -457,6 +460,19 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { return nil, err } + // Enterprise administration needs its own installation token: the org + // installation token above carries no enterprise permissions. Reading the + // owners needs the org token, so both are handed to the client. + // + // Built on first use rather than here, so a failure fails the + // enterprise_role sync instead of the whole connector: the other resource + // types keep syncing and C1 holds its previous owner state rather than + // reading an empty list as a revoke of every owner. + newEnterpriseRoleClientsFn := func(ctx context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + return newEnterpriseRoleClients( + ctx, ghc.InstanceUrl, appClient, jwtts, ghc.Enterprises, appHTTPClient, ghc.Org) + } + gh := &GitHub{ client: ghClient, appClient: appClient, @@ -464,6 +480,7 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { instanceURL: ghc.InstanceUrl, orgs: []string{ghc.Org}, enterprises: ghc.Enterprises, + newEnterpriseRoleClients: newEnterpriseRoleClientsFn, graphqlClient: graphqlClient, orgCache: newOrgNameCache(ghClient), syncSecrets: ghc.SyncSecrets, @@ -474,17 +491,131 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { return gh, nil } -func newGitHubGraphqlClient(ctx context.Context, instanceURL string, ts oauth2.TokenSource) (*githubv4.Client, error) { - instanceURL = strings.TrimSuffix(instanceURL, "/") +// enterpriseInstallationTargetType is the target_type that +// GET /app/installations reports for an enterprise-level installation. +const enterpriseInstallationTargetType = "Enterprise" + +// newEnterpriseRoleClients builds one client per configured enterprise, each +// authenticated with that enterprise's own installation access token. +// +// Enterprise installations are separate from organization installations and +// carry only enterprise permissions, so the enterprise installation is +// discovered through the app JWT (GET /app/installations) and gets its own +// token source. Sending the organization token to an enterprise mutation +// fails, and there is no way to widen the org token's scope. +// +// Fails closed when the app is not installed on a configured enterprise, or +// when the organization does not belong to it: without a trustworthy owner +// list the connector would emit an empty or foreign enterprise_role set, which +// reads to C1 as a revoke of every owner assignment. +func newEnterpriseRoleClients( + ctx context.Context, + instanceURL string, + appClient *github.Client, + jwtTokenSource oauth2.TokenSource, + enterprises []string, + orgHTTPClient *http.Client, + org string, +) (map[string]*githubEnterpriseAdministratorClient, error) { + if len(enterprises) == 0 { + return nil, nil + } + // The owners are read through the single configured organization, and an + // organization belongs to exactly one enterprise, so only that enterprise + // can be served. Saying so here beats failing later on a verification the + // operator cannot satisfy. The PAT path does support a list. + if len(enterprises) > 1 { + return nil, fmt.Errorf( + "github-connector: GitHub App authentication serves one enterprise at a time, "+ + "because the owners are read through organization %q, which belongs to a single enterprise; "+ + "%d were configured", org, len(enterprises)) + } + + installations, err := listEnterpriseInstallations(ctx, customclient.New(appClient)) + if err != nil { + return nil, err + } - var enterpriseGqlURL string - if instanceURL != "" && instanceURL != githubDotCom { - parsed, err := url.Parse(instanceURL) + clients := make(map[string]*githubEnterpriseAdministratorClient, len(enterprises)) + for _, enterprise := range enterprises { + installationID, ok := installations[strings.ToLower(enterprise)] + if !ok { + return nil, fmt.Errorf( + "github-connector: GitHub App is not installed on enterprise %q; install it on the enterprise account "+ + "with the Enterprise people read and write permission", enterprise) + } + + token, err := getInstallationToken(ctx, appClient, installationID) + if err != nil { + return nil, err + } + + ts := newRefreshableTokenSource( + &oauth2.Token{ + AccessToken: token.GetToken(), + Expiry: token.GetExpiresAt().Time, + }, + &appTokenRefresher{ + ctx: ctx, + instanceURL: instanceURL, + installationID: installationID, + jwtTokenSource: jwtTokenSource, + }, + ) + + httpClient, err := newGitHubAppHTTPClient(ctx, ts) + if err != nil { + return nil, err + } + + client, err := newEnterpriseAdministratorClient(instanceURL, httpClient, orgHTTPClient, org) if err != nil { return nil, err } - parsed.Path = "/api/graphql" - enterpriseGqlURL = parsed.String() + if err := client.verifyOrganization(ctx, enterprise); err != nil { + return nil, err + } + if err := client.resolveEnterpriseNodeID(ctx, enterprise); err != nil { + return nil, err + } + clients[enterprise] = client + } + + return clients, nil +} + +// listEnterpriseInstallations maps enterprise slug (lowercased) to installation +// ID for every enterprise installation of this app. go-github models the +// installation account as *User, which has no enterprise slug, so the response +// is decoded through customclient's own model. +func listEnterpriseInstallations(ctx context.Context, client *customclient.Client) (map[string]int64, error) { + installations := make(map[string]int64) + page := 1 + for { + pageInstallations, _, err := client.ListAppInstallations(ctx, page) + if err != nil { + return nil, fmt.Errorf("github-connector: failed to list app installations: %w", err) + } + + for _, installation := range pageInstallations { + if installation.TargetType != enterpriseInstallationTargetType || installation.Account.Slug == "" { + continue + } + installations[strings.ToLower(installation.Account.Slug)] = installation.ID + } + + // A page shorter than the one requested is the last one. + if len(pageInstallations) < customclient.AppInstallationsPageSize { + return installations, nil + } + page++ + } +} + +func newGitHubGraphqlClient(ctx context.Context, instanceURL string, ts oauth2.TokenSource) (*githubv4.Client, error) { + endpoint, err := enterpriseGraphQLEndpoint(instanceURL) + if err != nil { + return nil, err } httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, ctxzap.Extract(ctx))) @@ -496,10 +627,7 @@ func newGitHubGraphqlClient(ctx context.Context, instanceURL string, ts oauth2.T ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient) tc := oauth2.NewClient(ctx, ts) - if enterpriseGqlURL != "" { - return githubv4.NewEnterpriseClient(enterpriseGqlURL, tc), nil - } - return githubv4.NewClient(tc), nil + return githubv4.NewEnterpriseClient(endpoint.String(), tc), nil } // escapedLineBreaks unescapes LF-, CRLF-, and CR-escaped line breaks (`\r\n`, diff --git a/pkg/connector/enterprise_administrator_client.go b/pkg/connector/enterprise_administrator_client.go new file mode 100644 index 00000000..b7f504c8 --- /dev/null +++ b/pkg/connector/enterprise_administrator_client.go @@ -0,0 +1,657 @@ +package connector + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/url" + "strings" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/shurcooL/githubv4" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/timestamppb" +) + +const ( + // GraphQL variable names, shared by the query text and the variable map so + // the two cannot drift: a typo'd map key reaches the API as a missing + // variable rather than failing to compile. + enterpriseSlugVariable = "slug" + enterpriseLoginVariable = "login" + enterpriseOrgVariable = "org" + enterpriseRoleVariable = "role" + enterpriseQueryVariable = "query" + enterpriseFirstVariable = "first" + enterpriseAfterVariable = "after" + + enterpriseGraphQLPath = "/api/graphql" + githubDotComGraphQL = "https://api.github.com/graphql" + enterpriseRateLimitField = "rateLimit" + + // GitHub caps every connection used here at 100 per page. + enterpriseOwnerPageSize = 100 + enterpriseOrganizationPageSize = 100 + enterpriseMemberPageSize = 100 + // One aliased invitation lookup per member of a page. + enterpriseInvitationBatchSize = enterpriseMemberPageSize + // Bounds the walks that happen inside one call rather than across page + // tokens, so a mispaginating API cannot pin a provisioning task. + enterpriseMaxPages = 1000 +) + +// enterpriseOwnerState is what the connector can observe about a user's Owner +// access: whether they hold the role today, and the invitation that would give +// it to them once they accept it. +type enterpriseOwnerState struct { + enterpriseID string + isOwner bool + pendingInvitationID string +} + +// githubEnterpriseAdministratorClient reads and writes the built-in Owner role +// of one enterprise. It needs both of the app's installations, because GitHub +// splits the data across them: +// +// - Enterprise.ownerInfo, which holds admins and pendingAdminInvitations, +// resolves to null for an installation token, so the owners are read from +// Organization.enterpriseOwners with the organization token. The +// enterprise token gets FORBIDDEN on any organization field. +// - The invitation lookup and every mutation are enterprise fields, so they +// go through the enterprise token. +// +// Enterprise.members(role: OWNER) is deliberately not used: that role is +// EnterpriseUserAccountMembershipRole, whose OWNER means "owner of an +// organization in the enterprise", not owner of the enterprise account. +type githubEnterpriseAdministratorClient struct { + enterpriseClient *githubv4.Client + orgClient *githubv4.Client + org string + // Immutable for a given slug, so it is resolved once at construction + // rather than on each Grant and Revoke. + enterpriseNodeID string + // batchClient serves the aliased invitation lookup, and omits + // enterpriseGraphQLTransport because that batch always reports NOT_FOUND + // entries the classifier would read as a failure. + endpoint *url.URL + batchClient *uhttp.BaseHttpClient +} + +// newEnterpriseAdministratorClient returns the client for one enterprise +// installation, with a GraphQL client per token because the two installations +// are separate credentials. +// +// The HTTP clients arrive unwrapped because that is what both consumers take: +// githubv4 builds its own client over one, uhttp wraps one. They already carry +// the installation token, its 401 refresh, and uhttp's transport underneath. +func newEnterpriseAdministratorClient( + instanceURL string, + enterpriseHTTPClient *http.Client, + orgHTTPClient *http.Client, + org string, +) (*githubEnterpriseAdministratorClient, error) { + endpoint, err := enterpriseGraphQLEndpoint(instanceURL) + if err != nil { + return nil, err + } + + // NewBaseHttpClient reports a failed cache setup by returning nil, which + // only panics later inside Do. + batchClient := uhttp.NewBaseHttpClient(enterpriseHTTPClient) + if batchClient == nil { + return nil, fmt.Errorf("baton-github: error building the enterprise GraphQL batch client") + } + + return &githubEnterpriseAdministratorClient{ + enterpriseClient: newEnterpriseGraphQLClient(endpoint.String(), enterpriseHTTPClient), + orgClient: newEnterpriseGraphQLClient(endpoint.String(), orgHTTPClient), + org: org, + endpoint: endpoint, + batchClient: batchClient, + }, nil +} + +// newEnterpriseGraphQLClient returns a GraphQL client that classifies both the +// HTTP status and the errors[] GitHub returns alongside an HTTP 200. Both +// layers are needed, because the SDK only backs off on a typed Unavailable and +// a rate limit arrives as a successful status. +// +// The connector's shared GraphQL client keeps only the status classifier: +// user.go detects enterprise SAML by matching the text of its error. +func newEnterpriseGraphQLClient(endpoint string, httpClient *http.Client) *githubv4.Client { + base := httpClient.Transport + if base == nil { + base = http.DefaultTransport + } + + return githubv4.NewEnterpriseClient(endpoint, &http.Client{ + Timeout: httpClient.Timeout, + Transport: &enterpriseGraphQLTransport{ + base: &statusClassifyingTransport{base: base}, + }, + }) +} + +// enterpriseGraphQLEndpoint returns the GraphQL URL of the instance, which is +// api.github.com for GitHub.com and /api/graphql on a self-hosted host. +// +// The trailing slash is trimmed before the comparison, so "https://github.com/" +// resolves to GitHub.com rather than to a /api/graphql path on that host. +func enterpriseGraphQLEndpoint(instanceURL string) (*url.URL, error) { + instanceURL = strings.TrimSuffix(instanceURL, "/") + if instanceURL == "" || instanceURL == githubDotCom { + return url.Parse(githubDotComGraphQL) + } + + gqlURL, err := url.Parse(instanceURL) + if err != nil { + return nil, err + } + gqlURL.Path = enterpriseGraphQLPath + + return gqlURL, nil +} + +type graphQLRateLimit struct { + Limit githubv4.Int + Remaining githubv4.Int + ResetAt githubv4.DateTime +} + +// annotations returns the remaining GraphQL budget, or nil when the response +// carried no rateLimit block: the zero value would otherwise be reported as an +// exhausted budget with no reset time. +func (r graphQLRateLimit) annotations() annotations.Annotations { + if r.Limit == 0 && r.ResetAt.IsZero() { + return nil + } + + rateLimit := &v2.RateLimitDescription{ + Status: v2.RateLimitDescription_STATUS_OK, + Limit: int64(r.Limit), + Remaining: int64(r.Remaining), + } + if r.Remaining <= 0 { + rateLimit.Status = v2.RateLimitDescription_STATUS_OVERLIMIT + } + if !r.ResetAt.IsZero() { + rateLimit.ResetAt = timestamppb.New(r.ResetAt.Time) + } + + return annotations.New(rateLimit) +} + +// enterpriseOwnersQuery reads one page of the enterprise account's owners +// through the organization the app is installed on. +type enterpriseOwnersQuery struct { + Organization struct { + EnterpriseOwners struct { + Nodes []struct { + DatabaseID githubv4.Int + Login githubv4.String + } + PageInfo struct { + HasNextPage githubv4.Boolean + EndCursor githubv4.String + } + } `graphql:"enterpriseOwners(first: $first, after: $after)"` + } `graphql:"organization(login: $org)"` + RateLimit graphQLRateLimit +} + +// enterpriseUser is a user account as the enterprise reports it, whether as an +// owner or as a plain member. +type enterpriseUser struct { + databaseID int64 + login string +} + +// owners returns one page of the users who currently own the enterprise +// account. +func (c *githubEnterpriseAdministratorClient) owners( + ctx context.Context, + after *githubv4.String, +) ([]enterpriseUser, string, annotations.Annotations, error) { + var query enterpriseOwnersQuery + err := c.orgClient.Query(ctx, &query, map[string]any{ + enterpriseOrgVariable: githubv4.String(c.org), + enterpriseFirstVariable: githubv4.Int(enterpriseOwnerPageSize), + enterpriseAfterVariable: after, + }) + if err != nil { + return nil, "", nil, fmt.Errorf("baton-github: error listing enterprise owners of org %s: %w", c.org, err) + } + + owners := make([]enterpriseUser, 0, len(query.Organization.EnterpriseOwners.Nodes)) + for _, node := range query.Organization.EnterpriseOwners.Nodes { + owners = append(owners, enterpriseUser{ + databaseID: int64(node.DatabaseID), + login: string(node.Login), + }) + } + + nextCursor := "" + if query.Organization.EnterpriseOwners.PageInfo.HasNextPage { + nextCursor = string(query.Organization.EnterpriseOwners.PageInfo.EndCursor) + } + + return owners, nextCursor, query.RateLimit.annotations(), nil +} + +// resolveEnterpriseNodeID looks up the node ID every enterprise mutation takes +// and stores it on the client. It is called once at construction: the ID is +// immutable for a given slug, and resolving it per operation would add two +// requests to every Grant and Revoke, which each read OwnerState twice. +// +// It doubles as the check that the enterprise is visible to this installation. +func (c *githubEnterpriseAdministratorClient) resolveEnterpriseNodeID(ctx context.Context, enterprise string) error { + var query struct { + Enterprise struct { + ID githubv4.String + } `graphql:"enterprise(slug: $slug)"` + } + err := c.enterpriseClient.Query(ctx, &query, map[string]any{ + enterpriseSlugVariable: githubv4.String(enterprise), + }) + if err != nil { + return fmt.Errorf("baton-github: error getting enterprise %s: %w", enterprise, err) + } + if query.Enterprise.ID == "" { + return fmt.Errorf("baton-github: enterprise %s is not visible to the GitHub App", enterprise) + } + c.enterpriseNodeID = string(query.Enterprise.ID) + + return nil +} + +// verifyOrganization checks that the organization the owners are read from +// belongs to this enterprise. Otherwise the connector would report another +// enterprise's owners as owners of this one. +// +// organizations(query:) is a substring search, so an enterprise with many +// similarly named organizations can push the exact match past the first page. +// Every page is read before concluding that the organization is not there. +func (c *githubEnterpriseAdministratorClient) verifyOrganization(ctx context.Context, enterprise string) error { + var after *githubv4.String + for page := 0; page < enterpriseMaxPages; page++ { + var query struct { + Enterprise struct { + Organizations struct { + Nodes []struct { + Login githubv4.String + } + PageInfo struct { + HasNextPage githubv4.Boolean + EndCursor githubv4.String + } + } `graphql:"organizations(first: $first, after: $after, query: $query)"` + } `graphql:"enterprise(slug: $slug)"` + } + err := c.enterpriseClient.Query(ctx, &query, map[string]any{ + enterpriseSlugVariable: githubv4.String(enterprise), + enterpriseFirstVariable: githubv4.Int(enterpriseOrganizationPageSize), + enterpriseAfterVariable: after, + enterpriseQueryVariable: githubv4.String(c.org), + }) + if err != nil { + return fmt.Errorf("baton-github: error listing organizations of enterprise %s: %w", enterprise, err) + } + + for _, node := range query.Enterprise.Organizations.Nodes { + if strings.EqualFold(string(node.Login), c.org) { + return nil + } + } + + if !query.Enterprise.Organizations.PageInfo.HasNextPage { + return fmt.Errorf( + "baton-github: organization %s does not belong to enterprise %s, so its owners cannot be synced", + c.org, enterprise) + } + after = githubv4.NewString(query.Enterprise.Organizations.PageInfo.EndCursor) + } + + return fmt.Errorf( + "baton-github: gave up looking for organization %s in enterprise %s after %d pages", + c.org, enterprise, enterpriseMaxPages) +} + +// enterpriseMembersQuery reads one page of the enterprise's member accounts. +// members is a union: an enterprise with Enterprise Managed Users returns +// EnterpriseUserAccount, a regular enterprise can also return User, so both +// shapes are selected and each field falls back to the other branch. +type enterpriseMembersQuery struct { + Enterprise struct { + Members struct { + Nodes []struct { + EnterpriseUserAccount struct { + Login githubv4.String + User struct { + DatabaseID githubv4.Int + Login githubv4.String + } + } `graphql:"... on EnterpriseUserAccount"` + User struct { + DatabaseID githubv4.Int + Login githubv4.String + } `graphql:"... on User"` + } + PageInfo struct { + HasNextPage githubv4.Boolean + EndCursor githubv4.String + } + } `graphql:"members(first: $first, after: $after)"` + } `graphql:"enterprise(slug: $slug)"` + RateLimit graphQLRateLimit +} + +// members returns one page of the enterprise's member accounts and the cursor +// of the next page, skipping any node without a database ID because guessing +// one would re-key the identity on the next sync. +func (c *githubEnterpriseAdministratorClient) members( + ctx context.Context, + enterprise string, + after *githubv4.String, +) ([]enterpriseUser, string, annotations.Annotations, error) { + var query enterpriseMembersQuery + err := c.enterpriseClient.Query(ctx, &query, map[string]any{ + enterpriseSlugVariable: githubv4.String(enterprise), + enterpriseFirstVariable: githubv4.Int(enterpriseMemberPageSize), + enterpriseAfterVariable: after, + }) + if err != nil { + return nil, "", nil, fmt.Errorf("baton-github: error listing members of enterprise %s: %w", enterprise, err) + } + + members := make([]enterpriseUser, 0, len(query.Enterprise.Members.Nodes)) + for _, node := range query.Enterprise.Members.Nodes { + member := enterpriseUser{ + databaseID: int64(node.EnterpriseUserAccount.User.DatabaseID), + login: string(node.EnterpriseUserAccount.Login), + } + if member.databaseID == 0 { + member.databaseID = int64(node.User.DatabaseID) + } + if member.login == "" { + member.login = string(node.User.Login) + } + if member.databaseID == 0 || member.login == "" { + continue + } + members = append(members, member) + } + + nextCursor := "" + if query.Enterprise.Members.PageInfo.HasNextPage { + nextCursor = string(query.Enterprise.Members.PageInfo.EndCursor) + } + + return members, nextCursor, query.RateLimit.annotations(), nil +} + +// pendingOwnerInvitations returns the pending Owner invitation of each given +// login that has one, keyed by login, resolved in a single request. +// +// Invitations are not enumerable for an installation token, because +// Enterprise.ownerInfo resolves to null, so they can only be asked for one +// login at a time. Aliasing collapses that into one request, which GitHub +// charges a single rate-limit point. +// +// Logins travel as variables and only generated alias names reach the query +// text, so a login cannot alter the query. A login with no invitation answers +// with a NOT_FOUND entry, so those are dropped before the remaining errors are +// classified: left in, they would claim the code for the whole response, and +// NOT_FOUND is the one code the SDK downgrades to a warning. The rate limit is +// returned on the failure paths too, since that is when C1 needs it. +func (c *githubEnterpriseAdministratorClient) pendingOwnerInvitations( + ctx context.Context, + enterprise string, + logins []string, +) (map[string]string, annotations.Annotations, error) { + if len(logins) == 0 { + return map[string]string{}, nil, nil + } + if len(logins) > enterpriseInvitationBatchSize { + return nil, nil, fmt.Errorf( + "baton-github: pending owner invitation lookup takes at most %d logins, got %d", + enterpriseInvitationBatchSize, len(logins)) + } + + declarations := []string{"$" + enterpriseSlugVariable + ":String!", "$" + enterpriseRoleVariable + ":EnterpriseAdministratorRole!"} + selections := make([]string, 0, len(logins)) + variables := map[string]any{ + enterpriseSlugVariable: enterprise, + enterpriseRoleVariable: string(githubv4.EnterpriseAdministratorRoleOwner), + } + aliasLogin := make(map[string]string, len(logins)) + for i, login := range logins { + alias := fmt.Sprintf("i%d", i) + variable := fmt.Sprintf("l%d", i) + aliasLogin[alias] = login + variables[variable] = login + declarations = append(declarations, "$"+variable+":String!") + selections = append(selections, fmt.Sprintf( + "%s: enterpriseAdministratorInvitation(enterpriseSlug: $%s, userLogin: $%s, role: $%s){id}", + alias, enterpriseSlugVariable, variable, enterpriseRoleVariable)) + } + + query := fmt.Sprintf("query(%s){%s %s{limit remaining resetAt}}", + strings.Join(declarations, ","), strings.Join(selections, " "), enterpriseRateLimitField) + + aliases, graphQLErrors, err := c.doGraphQL(ctx, query, variables) + + var rateLimit graphQLRateLimit + if raw, ok := aliases[enterpriseRateLimitField]; ok { + if unmarshalErr := json.Unmarshal(raw, &rateLimit); unmarshalErr != nil { + return nil, nil, fmt.Errorf("baton-github: error decoding the rate limit of enterprise %s: %w", enterprise, unmarshalErr) + } + } + annos := rateLimit.annotations() + if err != nil { + return nil, annos, fmt.Errorf("baton-github: error listing pending owner invitations of enterprise %s: %w", enterprise, err) + } + unexpected := make([]graphQLError, 0, len(graphQLErrors)) + for _, graphQLErr := range graphQLErrors { + if graphQLErrorType(graphQLErr) != graphQLErrorNotFound { + unexpected = append(unexpected, graphQLErr) + } + } + if len(unexpected) > 0 { + return nil, annos, status.Errorf(graphQLErrorsCode(unexpected), + "baton-github: error listing pending owner invitations of enterprise %s: %s", + enterprise, unexpected[0].Message) + } + + invitations := make(map[string]string, len(aliases)) + for alias, raw := range aliases { + login, ok := aliasLogin[alias] + if !ok { + continue + } + var node struct { + ID string `json:"id"` + } + if err := json.Unmarshal(raw, &node); err != nil || node.ID == "" { + continue + } + invitations[login] = node.ID + } + + return invitations, annos, nil +} + +// doGraphQL runs a query whose selection set is built at runtime, which the +// typed client cannot express. It returns the top-level fields undecoded plus +// the errors array, so the caller decides which fields to read and which +// errors are expected. A non-2xx arrives already mapped onto a gRPC code by +// uhttp, so its error is returned unwrapped. +func (c *githubEnterpriseAdministratorClient) doGraphQL( + ctx context.Context, + query string, + variables map[string]any, +) (map[string]json.RawMessage, []graphQLError, error) { + req, err := c.batchClient.NewRequest(ctx, http.MethodPost, c.endpoint, + uhttp.WithContentTypeJSONHeader(), + uhttp.WithAcceptJSONHeader(), + uhttp.WithJSONBody(map[string]any{"query": query, "variables": variables}), + ) + if err != nil { + return nil, nil, fmt.Errorf("creating GraphQL request: %w", err) + } + + var envelope graphQLEnvelope + resp, err := c.batchClient.Do(req, uhttp.WithJSONResponse(&envelope)) + if err != nil { + if resp != nil { + _ = resp.Body.Close() + } + return nil, nil, err + } + defer resp.Body.Close() + + return envelope.Data, envelope.Errors, nil +} + +// pendingOwnerInvitation returns the ID of the pending Owner invitation for a +// login, or an empty string when there is none. GitHub reports a login without +// an invitation as NOT_FOUND rather than as a null field. +func (c *githubEnterpriseAdministratorClient) pendingOwnerInvitation( + ctx context.Context, + enterprise string, + login string, +) (string, error) { + var query struct { + EnterpriseAdministratorInvitation struct { + ID githubv4.String + } `graphql:"enterpriseAdministratorInvitation(enterpriseSlug: $slug, userLogin: $login, role: $role)"` + } + err := c.enterpriseClient.Query(ctx, &query, map[string]any{ + enterpriseSlugVariable: githubv4.String(enterprise), + enterpriseLoginVariable: githubv4.String(login), + enterpriseRoleVariable: githubv4.EnterpriseAdministratorRoleOwner, + }) + if err != nil { + if status.Code(err) == codes.NotFound { + return "", nil + } + return "", fmt.Errorf("baton-github: error getting owner invitation for %s: %w", login, err) + } + + return string(query.EnterpriseAdministratorInvitation.ID), nil +} + +// OwnerState reports whether a login owns the enterprise account and whether +// an invitation for it is still pending, together with the remaining budget. +// +// Both facts are resolved even for an active owner, because Revoke acts on +// each separately: stopping at the role would let a stale invitation survive +// the demotion and then fail the verification that follows it. +// +// The owners connection does take a query argument, but it is a search rather +// than an exact-login filter, so an empty result cannot be trusted to mean +// "not an owner" — on Revoke that reading would report GrantAlreadyRevoked +// while the user keeps the role. The pages are walked and matched on the login +// instead, which in practice is one request: owners are a small set. +func (c *githubEnterpriseAdministratorClient) OwnerState( + ctx context.Context, + enterprise string, + login string, +) (enterpriseOwnerState, annotations.Annotations, error) { + state := enterpriseOwnerState{enterpriseID: c.enterpriseNodeID} + + var annos annotations.Annotations + var after *githubv4.String + for page := 0; page < enterpriseMaxPages; page++ { + owners, nextCursor, pageAnnos, err := c.owners(ctx, after) + if err != nil { + return state, annos, err + } + // The freshest budget, not one descriptor per page. + if len(pageAnnos) > 0 { + annos = pageAnnos + } + for _, owner := range owners { + if strings.EqualFold(owner.login, login) { + state.isOwner = true + break + } + } + if state.isOwner || nextCursor == "" { + break + } + after = githubv4.NewString(githubv4.String(nextCursor)) + } + + invitationID, err := c.pendingOwnerInvitation(ctx, enterprise, login) + if err != nil { + return state, annos, err + } + state.pendingInvitationID = invitationID + + return state, annos, nil +} + +// UpdateRole changes the role of someone who already administers the +// enterprise. It cannot promote a plain member. +func (c *githubEnterpriseAdministratorClient) UpdateRole( + ctx context.Context, + enterpriseID string, + login string, + role githubv4.EnterpriseAdministratorRole, +) error { + // GraphQL rejects a mutation without a selection set, and rateLimit exists + // only on Query, so every mutation selects clientMutationId. + var mutation struct { + UpdateEnterpriseAdministratorRole struct { + ClientMutationID githubv4.String + } `graphql:"updateEnterpriseAdministratorRole(input: $input)"` + } + input := githubv4.UpdateEnterpriseAdministratorRoleInput{ + EnterpriseID: githubv4.ID(enterpriseID), + Login: githubv4.String(login), + Role: role, + } + if err := c.enterpriseClient.Mutate(ctx, &mutation, input, nil); err != nil { + return fmt.Errorf("baton-github: error setting enterprise role of %s to %s: %w", login, role, err) + } + + return nil +} + +// InviteOwner sends the Owner invitation a member has to accept. +func (c *githubEnterpriseAdministratorClient) InviteOwner(ctx context.Context, enterpriseID string, login string) error { + var mutation struct { + InviteEnterpriseAdmin struct { + ClientMutationID githubv4.String + } `graphql:"inviteEnterpriseAdmin(input: $input)"` + } + role := githubv4.EnterpriseAdministratorRoleOwner + input := githubv4.InviteEnterpriseAdminInput{ + EnterpriseID: githubv4.ID(enterpriseID), + Invitee: githubv4.NewString(githubv4.String(login)), + Role: &role, + } + if err := c.enterpriseClient.Mutate(ctx, &mutation, input, nil); err != nil { + return fmt.Errorf("baton-github: error inviting %s as enterprise owner: %w", login, err) + } + + return nil +} + +func (c *githubEnterpriseAdministratorClient) CancelInvitation(ctx context.Context, invitationID string) error { + var mutation struct { + CancelEnterpriseAdminInvitation struct { + ClientMutationID githubv4.String + } `graphql:"cancelEnterpriseAdminInvitation(input: $input)"` + } + input := githubv4.CancelEnterpriseAdminInvitationInput{InvitationID: githubv4.ID(invitationID)} + if err := c.enterpriseClient.Mutate(ctx, &mutation, input, nil); err != nil { + return fmt.Errorf("baton-github: error cancelling enterprise owner invitation: %w", err) + } + + return nil +} diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go new file mode 100644 index 00000000..fb0f04a3 --- /dev/null +++ b/pkg/connector/enterprise_installations_test.go @@ -0,0 +1,181 @@ +package connector + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "testing" + + "github.com/google/go-github/v69/github" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "github.com/conductorone/baton-github/pkg/customclient" +) + +// newGitHubAPITestClient points the client at a test server through BaseURL +// alone. Nothing rewrites the host, so a customclient endpoint that ignored +// BaseURL would leave the test reaching for api.github.com. +func newGitHubAPITestClient(t *testing.T, handler http.Handler) *github.Client { + t.Helper() + + return newGitHubAPITestClientAt(t, handler, "/") +} + +func newGitHubAPITestClientAt(t *testing.T, handler http.Handler, basePath string) *github.Client { + t.Helper() + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + baseURL, err := url.Parse(srv.URL + basePath) + require.NoError(t, err) + + client := github.NewClient(srv.Client()) + client.BaseURL = baseURL + + return client +} + +func TestListEnterpriseInstallations(t *testing.T) { + t.Parallel() + + ctx := context.Background() + payload := []map[string]any{ + { + "id": int64(11), + "target_type": "Organization", + "account": map[string]any{"login": "example-org"}, + }, + { + "id": int64(22), + "target_type": "Enterprise", + "account": map[string]any{"slug": "Example-Enterprise"}, + }, + { + "id": int64(33), + "target_type": "Enterprise", + "account": map[string]any{"login": "missing-slug-enterprise"}, + }, + } + + client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/app/installations", r.URL.Path) + require.Equal(t, "1", r.URL.Query().Get("page")) + require.Equal(t, "100", r.URL.Query().Get("per_page")) + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(payload)) + })) + + installations, err := listEnterpriseInstallations(ctx, customclient.New(client)) + require.NoError(t, err) + require.Equal(t, map[string]int64{"example-enterprise": 22}, installations) +} + +// On GitHub Enterprise Server, WithEnterpriseURLs puts the REST API under +// /api/v3. Asking api.github.com instead would report the app as uninstalled +// on an enterprise that does have it. +func TestListEnterpriseInstallationsUsesTheInstanceBaseURL(t *testing.T) { + t.Parallel() + + ctx := context.Background() + client := newGitHubAPITestClientAt(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v3/app/installations", r.URL.Path) + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode([]map[string]any{ + { + "id": int64(22), + "target_type": "Enterprise", + "account": map[string]any{"slug": "ghes-enterprise"}, + }, + })) + }), "/api/v3/") + + installations, err := listEnterpriseInstallations(ctx, customclient.New(client)) + require.NoError(t, err) + require.Equal(t, map[string]int64{"ghes-enterprise": 22}, installations) +} + +// The endpoint reports no total, so a page shorter than the requested size is +// what ends the walk. +func TestListEnterpriseInstallationsPagination(t *testing.T) { + t.Parallel() + + ctx := context.Background() + var requests atomic.Int32 + + client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/app/installations", r.URL.Path) + requests.Add(1) + + page := r.URL.Query().Get("page") + installations := make([]map[string]any, 0, customclient.AppInstallationsPageSize) + switch page { + case "1": + // A full page: one enterprise plus filler, so the caller must ask + // for the next one. + installations = append(installations, map[string]any{ + "id": int64(22), + "target_type": "Enterprise", + "account": map[string]any{"slug": "first-enterprise"}, + }) + for i := 1; i < customclient.AppInstallationsPageSize; i++ { + installations = append(installations, map[string]any{ + "id": int64(1000 + i), + "target_type": "Organization", + "account": map[string]any{"login": fmt.Sprintf("org-%d", i)}, + }) + } + case "2": + installations = append(installations, map[string]any{ + "id": int64(44), + "target_type": "Enterprise", + "account": map[string]any{"slug": "second-enterprise"}, + }) + default: + t.Fatalf("unexpected page %q: the walk must stop on a short page", page) + } + + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(installations)) + })) + + installations, err := listEnterpriseInstallations(ctx, customclient.New(client)) + require.NoError(t, err) + require.Equal(t, map[string]int64{"first-enterprise": 22, "second-enterprise": 44}, installations) + require.Equal(t, int32(2), requests.Load()) +} + +func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { + t.Parallel() + + ctx := context.Background() + client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/app/installations", r.URL.Path) + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode([]map[string]any{ + { + "id": int64(11), + "target_type": "Organization", + "account": map[string]any{"login": "example-org"}, + }, + })) + })) + + _, err := newEnterpriseRoleClients( + ctx, + "https://github.com", + client, + oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "unused"}), + []string{"example-enterprise"}, + nil, + "example-org", + ) + require.Error(t, err) + require.Contains(t, err.Error(), `not installed on enterprise "example-enterprise"`) + require.NotContains(t, err.Error(), "personal access token") +} diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index a6e79aab..9b69cf58 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -2,23 +2,43 @@ package connector import ( "context" - "errors" "fmt" + "strconv" "strings" "sync" "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/entitlement" "github.com/conductorone/baton-sdk/pkg/types/grant" resourceSdk "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" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) +const ( + enterpriseRoleAssigned = "assigned" + enterpriseRoleOwner = "Owner" + + // Sync phases of the Owner grants. The invitations cannot be listed, so + // they are resolved from the members in a second pass over the same token. + enterpriseOwnersPhase = "enterprise-owners" + enterprisePendingPhase = "enterprise-pending-invitations" + + // UNAFFILIATED demotes an administrator while keeping their enterprise + // membership; removeEnterpriseAdmin would evict them from the enterprise. + enterpriseAdministratorRoleUnaffiliated githubv4.EnterpriseAdministratorRole = "UNAFFILIATED" +) + +// enterpriseClientProvider builds the per-enterprise administration clients. +type enterpriseClientProvider func(ctx context.Context) (map[string]*githubEnterpriseAdministratorClient, error) + type enterpriseRoleResourceType struct { resourceType *v2.ResourceType client *github.Client @@ -27,12 +47,48 @@ type enterpriseRoleResourceType struct { enterprises []string roleUsersCache map[string][]string mu *sync.Mutex + // newEnterpriseClients builds the per-enterprise administration clients. + // It is nil under PAT auth. + newEnterpriseClients enterpriseClientProvider + // enterpriseClients is keyed by enterprise slug and memoized after the + // first successful build; enterpriseClientsErr is why it could not be + // built, which fails this resource type only so the rest still syncs. + enterpriseClients map[string]*githubEnterpriseAdministratorClient + enterpriseClientsErr error + enterpriseClientsSet bool } func (o *enterpriseRoleResourceType) ResourceType(_ context.Context) *v2.ResourceType { return o.resourceType } +// clients returns the per-enterprise administration clients, building them on +// first use and memoizing the outcome. +// +// A retryable failure is not memoized: discovering the installations is four +// network calls, and freezing a startup blip would disable this resource type +// for the lifetime of the process. A misconfiguration is memoized, because it +// will fail the same way every time. +func (o *enterpriseRoleResourceType) clients( + ctx context.Context, +) (map[string]*githubEnterpriseAdministratorClient, error) { + if o.newEnterpriseClients == nil { + return nil, nil + } + + o.mu.Lock() + defer o.mu.Unlock() + + if o.enterpriseClientsSet && !isRetryableError(o.enterpriseClientsErr) { + return o.enterpriseClients, o.enterpriseClientsErr + } + + o.enterpriseClients, o.enterpriseClientsErr = o.newEnterpriseClients(ctx) + o.enterpriseClientsSet = true + + return o.enterpriseClients, o.enterpriseClientsErr +} + func (o *enterpriseRoleResourceType) cacheRole(roleId string, userLogin string) { o.mu.Lock() defer o.mu.Unlock() @@ -97,6 +153,17 @@ func (o *enterpriseRoleResourceType) List( parentID *v2.ResourceId, opts resourceSdk.SyncOpAttrs, ) ([]*v2.Resource, *resourceSdk.SyncOpResults, error) { + enterpriseClients, err := o.clients(ctx) + if err != nil { + return nil, nil, err + } + + // The consumed-licenses API that backs the cache is PAT-only, so a GitHub + // App can only see the built-in Owner role it is able to read and mutate. + if len(enterpriseClients) > 0 { + return appList(o.enterprises, enterpriseClients) + } + var ret []*v2.Resource cache, err := o.getRoleUsersCache(ctx) if err != nil { @@ -135,7 +202,7 @@ func (o *enterpriseRoleResourceType) StaticEntitlements( _ resourceSdk.SyncOpAttrs, ) ([]*v2.Entitlement, *resourceSdk.SyncOpResults, error) { rv := []*v2.Entitlement{} - rv = append(rv, entitlement.NewAssignmentEntitlement(nil, "assigned", + rv = append(rv, entitlement.NewAssignmentEntitlement(nil, enterpriseRoleAssigned, entitlement.WithDisplayName("Role Assigned"), entitlement.WithDescription("Assignment to enterprise role in GitHub"), entitlement.WithGrantableTo(resourceTypeUser), @@ -149,6 +216,14 @@ func (o *enterpriseRoleResourceType) Grants( resource *v2.Resource, opts resourceSdk.SyncOpAttrs, ) ([]*v2.Grant, *resourceSdk.SyncOpResults, error) { + enterpriseClients, err := o.clients(ctx) + if err != nil { + return nil, nil, err + } + if len(enterpriseClients) > 0 { + return o.appGrants(ctx, enterpriseClients, resource, opts) + } + cache, err := o.getRoleUsersCache(ctx) if err != nil { return nil, nil, fmt.Errorf("baton-github: error getting user roles cache: %w", err) @@ -168,7 +243,7 @@ func (o *enterpriseRoleResourceType) Grants( ret = append(ret, grant.NewGrant( resource, - "assigned", + enterpriseRoleAssigned, principalId, )) } @@ -176,22 +251,396 @@ func (o *enterpriseRoleResourceType) Grants( return ret, &resourceSdk.SyncOpResults{}, nil } -func EnterpriseRoleBuilder(client *github.Client, appClient *github.Client, customClient *customclient.Client, enterprises []string) *enterpriseRoleResourceType { +// EnterpriseRoleBuilder returns the enterprise role syncer. newEnterpriseClients +// is nil under PAT authentication, where only the read path is available. +func EnterpriseRoleBuilder( + client *github.Client, + appClient *github.Client, + customClient *customclient.Client, + enterprises []string, + newEnterpriseClients enterpriseClientProvider, +) *enterpriseRoleResourceType { return &enterpriseRoleResourceType{ - resourceType: resourceTypeEnterpriseRole, - client: client, - appClient: appClient, - customClient: customClient, - enterprises: enterprises, - roleUsersCache: make(map[string][]string), - mu: &sync.Mutex{}, + resourceType: resourceTypeEnterpriseRole, + client: client, + appClient: appClient, + customClient: customClient, + enterprises: enterprises, + roleUsersCache: make(map[string][]string), + mu: &sync.Mutex{}, + newEnterpriseClients: newEnterpriseClients, + } +} + +// appList emits only the built-in Owner role. The consumed-licenses API that +// discovers the other roles is PAT-only. +func appList( + enterprises []string, + enterpriseClients map[string]*githubEnterpriseAdministratorClient, +) ([]*v2.Resource, *resourceSdk.SyncOpResults, error) { + var ret []*v2.Resource + for _, enterprise := range enterprises { + if _, ok := enterpriseClients[enterprise]; !ok { + continue + } + + roleResource, err := resourceSdk.NewRoleResource( + enterpriseRoleOwner, + resourceTypeEnterpriseRole, + fmt.Sprintf("%s:%s", enterprise, enterpriseRoleOwner), + []resourceSdk.RoleTraitOption{}, + ) + if err != nil { + return nil, nil, fmt.Errorf("baton-github: error creating role resource for %s in enterprise %s: %w", + enterpriseRoleOwner, enterprise, err) + } + ret = append(ret, roleResource) } + + return ret, &resourceSdk.SyncOpResults{}, nil } -func isPermissionDenied(err error) bool { - var grpcErr interface{ GRPCStatus() *status.Status } - if errors.As(err, &grpcErr) { - return grpcErr.GRPCStatus().Code() == codes.PermissionDenied +// appGrants emits both the users who hold the Owner role today and the ones +// GitHub has invited but who have not accepted yet, against the same +// entitlement. The two are indistinguishable to C1, which has no pending grant +// state; the connector still tells them apart internally, because revoking an +// accepted owner and cancelling an unaccepted invitation are different +// mutations. +// +// An invitation that nobody accepts stops resolving on GitHub's side, so it +// simply stops being emitted and C1 removes the grant on that sync. Nothing +// here tracks an expiry. +// +// The owners and the invitations are walked as two phases of one page token, +// because the invitations are not enumerable and have to be resolved by asking +// about the enterprise members in batches. An empty cursor drops the current +// phase, so the next call moves on and the token empties once both are done. +func (o *enterpriseRoleResourceType) appGrants( + ctx context.Context, + enterpriseClients map[string]*githubEnterpriseAdministratorClient, + resource *v2.Resource, + opts resourceSdk.SyncOpAttrs, +) ([]*v2.Grant, *resourceSdk.SyncOpResults, error) { + enterprise, ok := provisionableEnterpriseOwner(resource.Id.Resource) + if !ok { + return nil, &resourceSdk.SyncOpResults{}, nil + } + client, ok := enterpriseClients[enterprise] + if !ok { + return nil, &resourceSdk.SyncOpResults{}, nil + } + + bag := &pagination.Bag{} + if err := bag.Unmarshal(opts.PageToken.Token); err != nil { + return nil, nil, fmt.Errorf("baton-github: error parsing enterprise owner page token: %w", err) + } + if bag.Current() == nil { + // Reverse order: the owners are walked first. + bag.Push(pagination.PageState{ResourceTypeID: enterprisePendingPhase}) + bag.Push(pagination.PageState{ResourceTypeID: enterpriseOwnersPhase}) + } + + var after *githubv4.String + if cursor := bag.PageToken(); cursor != "" { + after = githubv4.NewString(githubv4.String(cursor)) + } + + var ( + ret []*v2.Grant + nextCursor string + annos annotations.Annotations + err error + ) + switch phase := bag.ResourceTypeID(); phase { + case enterpriseOwnersPhase: + ret, nextCursor, annos, err = o.ownerGrants(ctx, client, resource, after) + case enterprisePendingPhase: + ret, nextCursor, annos, err = o.pendingInvitationGrants(ctx, client, resource, enterprise, after) + default: + return nil, nil, fmt.Errorf("baton-github: unexpected enterprise owner sync phase %q", phase) + } + if err != nil { + return nil, &resourceSdk.SyncOpResults{Annotations: annos}, err + } + + if err := bag.Next(nextCursor); err != nil { + return nil, &resourceSdk.SyncOpResults{Annotations: annos}, + fmt.Errorf("baton-github: error advancing the enterprise owner page token: %w", err) + } + pageToken, err := bag.Marshal() + if err != nil { + return nil, &resourceSdk.SyncOpResults{Annotations: annos}, + fmt.Errorf("baton-github: error building the enterprise owner page token: %w", err) + } + + return ret, &resourceSdk.SyncOpResults{Annotations: annos, NextPageToken: pageToken}, nil +} + +// ownerGrants emits one page of the users who hold the role today. +func (o *enterpriseRoleResourceType) ownerGrants( + ctx context.Context, + client *githubEnterpriseAdministratorClient, + resource *v2.Resource, + after *githubv4.String, +) ([]*v2.Grant, string, annotations.Annotations, error) { + owners, nextCursor, annos, err := client.owners(ctx, after) + if err != nil { + return nil, "", annos, err + } + + ret := make([]*v2.Grant, 0, len(owners)) + for _, owner := range owners { + principalId, err := enterpriseOwnerPrincipalID(owner) + if err != nil { + return nil, "", annos, err + } + ret = append(ret, grant.NewGrant(resource, enterpriseRoleAssigned, principalId)) + } + + return ret, nextCursor, annos, nil +} + +// pendingInvitationGrants emits one page worth of users who have been invited +// to the role and have not accepted, in member order so a page emits the same +// grants in the same sequence on every sync. +// +// GitHub exposes no connection of pending administrator invitations to an +// installation token, so they cannot be listed: they are resolved by asking +// about known logins. The enterprise members are that candidate set, which +// means an invitation sent to someone who is not a member of the enterprise is +// invisible to the sync. +func (o *enterpriseRoleResourceType) pendingInvitationGrants( + ctx context.Context, + client *githubEnterpriseAdministratorClient, + resource *v2.Resource, + enterprise string, + after *githubv4.String, +) ([]*v2.Grant, string, annotations.Annotations, error) { + members, nextCursor, annos, err := client.members(ctx, enterprise, after) + if err != nil { + return nil, "", annos, err + } + if len(members) == 0 { + return nil, nextCursor, annos, nil } - return false + + 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 + } + + ret := make([]*v2.Grant, 0, len(invitations)) + for _, member := range members { + if _, invited := invitations[member.login]; !invited { + continue + } + principalId, err := enterpriseOwnerPrincipalID(member) + if err != nil { + return nil, "", annos, err + } + ret = append(ret, grant.NewGrant(resource, enterpriseRoleAssigned, principalId)) + } + + return ret, nextCursor, annos, nil +} + +// Grant gives a user the built-in Owner role and returns the resulting grant. +// +// A member can only become an owner by accepting an invitation, while someone +// who already administers the enterprise is promoted in place. Which case +// applies is unreadable for an installation token, so the invitation is tried +// first and the promotion is the fallback, keyed on the FailedPrecondition +// that GitHub's UNPROCESSABLE maps to. +// +// An unaccepted invitation counts as held and returns a grant, the same way +// Grants() emits it: returning nothing would make C1 drop an overlay that the +// next sync puts straight back. +func (o *enterpriseRoleResourceType) Grant( + ctx context.Context, + principal *v2.Resource, + ent *v2.Entitlement, +) ([]*v2.Grant, annotations.Annotations, error) { + enterprise, client, err := o.provisioningTarget(ctx, principal, ent) + if err != nil { + return nil, nil, err + } + login, err := o.userLogin(ctx, principal.Id.Resource) + if err != nil { + return nil, nil, err + } + + result := []*v2.Grant{grant.NewGrant(ent.GetResource(), ent.GetSlug(), principal.Id)} + annos := annotations.New() + state, stateAnnos, err := client.OwnerState(ctx, enterprise, login) + annos = freshestRateLimit(annos, stateAnnos) + if err != nil { + return nil, annos, err + } + if state.isOwner || state.pendingInvitationID != "" { + annos.Append(&v2.GrantAlreadyExists{}) + return result, annos, nil + } + + if inviteErr := client.InviteOwner(ctx, state.enterpriseID, login); inviteErr != nil { + if status.Code(inviteErr) != codes.FailedPrecondition { + return nil, annos, inviteErr + } + if promoteErr := client.UpdateRole( + ctx, state.enterpriseID, login, githubv4.EnterpriseAdministratorRoleOwner, + ); promoteErr != nil { + // The promotion's status code is what tells C1 whether to retry. + return nil, annos, fmt.Errorf( + "promoting %s after the invitation was rejected: %w", login, promoteErr) + } + } + + state, stateAnnos, err = client.OwnerState(ctx, enterprise, login) + annos = freshestRateLimit(annos, stateAnnos) + if err != nil { + return nil, annos, err + } + switch { + case state.isOwner, state.pendingInvitationID != "": + return result, annos, nil + default: + return nil, annos, status.Errorf(codes.Unavailable, + "baton-github: enterprise owner grant for %s is not visible in GitHub", login) + } +} + +// Revoke takes the built-in Owner role away from a user. +// +// Holding the role and carrying an invitation are not alternatives, so both +// are cleared: either one left behind would fail the verification that +// follows. Demotion uses UNAFFILIATED, which keeps the user as a member of the +// enterprise instead of evicting them. A NOT_FOUND from either mutation is +// success, because it means the state being asked for is already in place. +func (o *enterpriseRoleResourceType) Revoke( + ctx context.Context, + grantObj *v2.Grant, +) (annotations.Annotations, error) { + enterprise, client, err := o.provisioningTarget(ctx, grantObj.GetPrincipal(), grantObj.GetEntitlement()) + if err != nil { + return nil, err + } + login, err := o.userLogin(ctx, grantObj.GetPrincipal().GetId().GetResource()) + if err != nil { + return nil, err + } + + annos := annotations.New() + state, stateAnnos, err := client.OwnerState(ctx, enterprise, login) + annos = freshestRateLimit(annos, stateAnnos) + if err != nil { + return annos, err + } + if !state.isOwner && state.pendingInvitationID == "" { + annos.Append(&v2.GrantAlreadyRevoked{}) + return annos, nil + } + + if state.isOwner { + if err := client.UpdateRole( + ctx, state.enterpriseID, login, enterpriseAdministratorRoleUnaffiliated, + ); err != nil && status.Code(err) != codes.NotFound { + return annos, err + } + } + if state.pendingInvitationID != "" { + if err := client.CancelInvitation(ctx, state.pendingInvitationID); err != nil && status.Code(err) != codes.NotFound { + return annos, err + } + } + + state, stateAnnos, err = client.OwnerState(ctx, enterprise, login) + annos = freshestRateLimit(annos, stateAnnos) + if err != nil { + return annos, err + } + if state.isOwner || state.pendingInvitationID != "" { + return annos, status.Errorf(codes.Unavailable, + "baton-github: enterprise owner revoke for %s is not visible in GitHub", login) + } + + return annos, nil +} + +func (o *enterpriseRoleResourceType) provisioningTarget( + ctx context.Context, + principal *v2.Resource, + ent *v2.Entitlement, +) (string, *githubEnterpriseAdministratorClient, error) { + if principal.GetId().GetResourceType() != resourceTypeUser.Id { + return "", nil, status.Error(codes.InvalidArgument, + "baton-github: enterprise role can only be granted to a user") + } + enterprise, ok := provisionableEnterpriseOwner(ent.GetResource().GetId().GetResource()) + if !ok { + return "", nil, status.Error(codes.InvalidArgument, + "baton-github: only the built-in enterprise Owner role can be provisioned") + } + // The construction error comes first: without it the operator is told the + // app is not installed even when the real cause was a transient failure + // or a mismatched organization, and FailedPrecondition reads to C1 as + // non-retryable. + enterpriseClients, err := o.clients(ctx) + if err != nil { + return "", nil, err + } + client, ok := enterpriseClients[enterprise] + if !ok { + return "", nil, status.Errorf(codes.FailedPrecondition, + "baton-github: provisioning enterprise %s requires a GitHub App installed on the enterprise account", enterprise) + } + return enterprise, client, nil +} + +func (o *enterpriseRoleResourceType) userLogin(ctx context.Context, userID string) (string, error) { + id, err := strconv.ParseInt(userID, 10, 64) + if err != nil { + return "", status.Errorf(codes.InvalidArgument, "baton-github: invalid GitHub user ID %q", userID) + } + user, resp, err := o.client.Users.GetByID(ctx, id) + if err != nil { + return "", wrapGitHubError(err, resp, fmt.Sprintf("baton-github: failed to get user %d", id)) + } + if user.GetLogin() == "" { + return "", fmt.Errorf("baton-github: GitHub user %d has no login", id) + } + return user.GetLogin(), nil +} + +func enterpriseOwnerPrincipalID(owner enterpriseUser) (*v2.ResourceId, error) { + if owner.databaseID <= 0 { + return nil, fmt.Errorf("baton-github: enterprise owner %q has no database ID", owner.login) + } + principalId, err := resourceSdk.NewResourceID(resourceTypeUser, owner.databaseID) + if err != nil { + return nil, fmt.Errorf("baton-github: error creating resource ID for user %s: %w", owner.login, err) + } + return principalId, nil +} + +// provisionableEnterpriseOwner returns the enterprise of a resource ID when it +// names the one role this connector can read and write through the GitHub App. +// Sync and provisioning share it so they cannot disagree on which roles are in +// the catalog. +func provisionableEnterpriseOwner(resourceID string) (string, bool) { + enterprise, role, ok := parseEnterpriseRoleID(resourceID) + if !ok || !strings.EqualFold(role, enterpriseRoleOwner) { + return "", false + } + + return enterprise, true +} + +func parseEnterpriseRoleID(resourceID string) (string, string, bool) { + enterprise, role, ok := strings.Cut(resourceID, ":") + return enterprise, role, ok && enterprise != "" && role != "" } diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go new file mode 100644 index 00000000..ed614e71 --- /dev/null +++ b/pkg/connector/enterprise_role_test.go @@ -0,0 +1,1021 @@ +package connector + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/pagination" + entitlementSdk "github.com/conductorone/baton-sdk/pkg/types/entitlement" + 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/test/mocks" +) + +const ( + testEnterprise = "example-enterprise" + testEnterpriseID = "E_example" + // The seeded mock user has ID 56 and login "56". + testLogin = "56" + testLoginID = 56 + testOrg = "example-org" +) + +// enterpriseStub is an in-memory GitHub GraphQL enterprise: it answers the +// owner and invitation queries from its own state and applies the mutations to +// it, so the tests exercise the documents the connector actually sends. +type enterpriseStub struct { + // ownerPages holds the users that currently hold the Owner role, split + // into the pages the members connection returns. + ownerPages [][]enterpriseStubOwner + // invitations maps a login to its pending Owner invitation ID. + invitations map[string]string + // memberPages holds the enterprise member accounts, split into the pages + // the members connection returns. They are the candidate set the pending + // invitation lookup asks about. + memberPages [][]enterpriseStubOwner + + // inviteErrorType makes inviteEnterpriseAdmin fail with that GraphQL error + // type. UNPROCESSABLE is what GitHub returns for someone who already + // administers the enterprise. + inviteErrorType string + inviteErrorMessage string + updateFails bool + cancelNotFound bool + // ownersRateLimited makes the owners read answer the way GitHub reports a + // GraphQL budget error: HTTP 200 carrying errors[]. + ownersRateLimited bool + // batchErrorType adds one entry of that type to the invitation batch, + // alongside the NOT_FOUND entries the batch always produces. + batchErrorType string + // silentMutations make the mutations report success without changing any + // state, which is how a phantom grant or revoke would look. + silentMutations bool + + ownerQueries int + invitationQueries int + invitationBatches int + memberQueries int + organizationChecks int + + updatedRole githubv4.EnterpriseAdministratorRole + invitedLogin string + cancelledID string +} + +type enterpriseStubOwner struct { + id int64 + login string +} + +func (s *enterpriseStub) handle(t *testing.T, w http.ResponseWriter, r *http.Request) { + t.Helper() + + var body struct { + Query string `json:"query"` + Variables map[string]any `json:"variables"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + + w.Header().Set("Content-Type", "application/json") + switch { + case strings.Contains(body.Query, "enterpriseAdministratorInvitation("): + // The sync resolves many logins in one aliased request; Grant and + // Revoke ask about a single login through the typed client. + if _, single := body.Variables["login"]; single { + s.writeInvitation(t, w, body.Variables) + } else { + s.writeInvitationBatch(t, w, body.Query, body.Variables) + } + + case strings.Contains(body.Query, "enterpriseOwners("): + require.Contains(t, body.Query, "databaseId") + require.NotContains(t, body.Query, "members(", + "owners must come from Organization.enterpriseOwners, not Enterprise.members") + // An organizationRole filter would drop every enterprise owner whose + // role in this organization is not OWNER, which is the bug this read + // path replaced. + require.NotContains(t, body.Query, "organizationRole", + "the owners query must not filter by the owner's role in the organization") + s.writeOwners(t, w, body.Variables) + + case strings.Contains(body.Query, "members("): + s.writeMembers(t, w, body.Variables) + + case strings.Contains(body.Query, "organizations("): + s.organizationChecks++ + _, _ = fmt.Fprintf(w, `{"data":{"enterprise":{"organizations":{"nodes":[{"login":%q}]}}}}`, testOrg) + + case strings.Contains(body.Query, "updateEnterpriseAdministratorRole("): + requireMutationShape(t, body.Query, "updateEnterpriseAdministratorRole") + input := mutationInput(t, body.Variables) + login, _ := input["login"].(string) + role, _ := input["role"].(string) + if s.updateFails { + _, _ = w.Write([]byte(`{"data":{"updateEnterpriseAdministratorRole":null},` + + `"errors":[{"type":"RATE_LIMITED","message":"rate limit exceeded"}]}`)) + return + } + s.updatedRole = githubv4.EnterpriseAdministratorRole(role) + if !s.silentMutations { + if role == string(enterpriseAdministratorRoleUnaffiliated) { + s.removeOwner(login) + } else { + s.ownerPages = [][]enterpriseStubOwner{{{id: testLoginID, login: login}}} + } + } + _, _ = w.Write([]byte(`{"data":{"updateEnterpriseAdministratorRole":{"clientMutationId":null}}}`)) + + case strings.Contains(body.Query, "inviteEnterpriseAdmin("): + requireMutationShape(t, body.Query, "inviteEnterpriseAdmin") + input := mutationInput(t, body.Variables) + login, _ := input["invitee"].(string) + if s.inviteErrorType != "" { + message := s.inviteErrorMessage + if message == "" { + // Observed live against GitHub for an existing administrator. + message = "Invitee is already an owner of this enterprise" + } + _, _ = fmt.Fprintf(w, + `{"data":{"inviteEnterpriseAdmin":null},"errors":[{"type":%q,"message":%q}]}`, + s.inviteErrorType, message) + return + } + s.invitedLogin = login + if !s.silentMutations { + s.invitations[login] = "EAI_" + login + } + _, _ = w.Write([]byte(`{"data":{"inviteEnterpriseAdmin":{"clientMutationId":null}}}`)) + + case strings.Contains(body.Query, "cancelEnterpriseAdminInvitation("): + requireMutationShape(t, body.Query, "cancelEnterpriseAdminInvitation") + input := mutationInput(t, body.Variables) + id, _ := input["invitationId"].(string) + s.cancelledID = id + for login, invitation := range s.invitations { + if invitation == id { + delete(s.invitations, login) + } + } + if s.cancelNotFound { + _, _ = w.Write([]byte(`{"data":{"cancelEnterpriseAdminInvitation":null},` + + `"errors":[{"type":"NOT_FOUND","message":"invitation not found"}]}`)) + return + } + _, _ = w.Write([]byte(`{"data":{"cancelEnterpriseAdminInvitation":{"clientMutationId":null}}}`)) + + case strings.Contains(body.Query, "enterprise(slug: $slug){id}"): + _, _ = fmt.Fprintf(w, `{"data":{"enterprise":{"id":%q}}}`, testEnterpriseID) + + default: + t.Fatalf("unexpected GraphQL operation: %s", body.Query) + } +} + +func (s *enterpriseStub) removeOwner(login string) { + for pageIndex, page := range s.ownerPages { + remaining := make([]enterpriseStubOwner, 0, len(page)) + for _, owner := range page { + if !strings.EqualFold(owner.login, login) { + remaining = append(remaining, owner) + } + } + s.ownerPages[pageIndex] = remaining + } +} + +func (s *enterpriseStub) writeOwners(t *testing.T, w http.ResponseWriter, variables map[string]any) { + t.Helper() + s.ownerQueries++ + + if s.ownersRateLimited { + _, err := w.Write([]byte(`{"data":{"organization":null},` + + `"errors":[{"type":"RATE_LIMITED","message":"API rate limit exceeded"}]}`)) + require.NoError(t, err) + return + } + + pageIndex := 0 + if after, ok := variables["after"].(string); ok && after != "" { + _, err := fmt.Sscanf(after, "cursor-%d", &pageIndex) + require.NoError(t, err) + } + + owners := []enterpriseStubOwner{} + if pageIndex < len(s.ownerPages) { + owners = s.ownerPages[pageIndex] + } + hasNextPage := pageIndex+1 < len(s.ownerPages) + + nodes := make([]string, 0, len(owners)) + for _, owner := range owners { + nodes = append(nodes, fmt.Sprintf(`{"databaseId":%d,"login":%q}`, owner.id, owner.login)) + } + + _, err := fmt.Fprintf( + w, + `{"data":{"organization":{"enterpriseOwners":{"nodes":[%s],`+ + `"pageInfo":{"hasNextPage":%t,"endCursor":"cursor-%d"}}},`+ + `"rateLimit":{"limit":5000,"remaining":4999,"resetAt":"2026-09-18T23:00:00Z"}}}`, + strings.Join(nodes, ","), hasNextPage, pageIndex+1, + ) + require.NoError(t, err) +} + +// writeInvitation answers the way GitHub does: the invitation when there is +// one, and a NOT_FOUND error when there is not. +// writeMembers answers one page of the enterprise member accounts, in the +// EnterpriseUserAccount shape GitHub returns for this connection. +func (s *enterpriseStub) writeMembers(t *testing.T, w http.ResponseWriter, variables map[string]any) { + t.Helper() + s.memberQueries++ + + pageIndex := 0 + if after, ok := variables["after"].(string); ok && after != "" { + _, err := fmt.Sscanf(after, "member-cursor-%d", &pageIndex) + require.NoError(t, err) + } + + members := []enterpriseStubOwner{} + if pageIndex < len(s.memberPages) { + members = s.memberPages[pageIndex] + } + hasNextPage := pageIndex+1 < len(s.memberPages) + + nodes := make([]string, 0, len(members)) + for _, member := range members { + // Only the selected fields come back, so no __typename here: the query + // resolves the union with inline fragments instead. + nodes = append(nodes, fmt.Sprintf( + `{"login":%q,"user":{"databaseId":%d,"login":%q}}`, + member.login, member.id, member.login)) + } + + _, err := fmt.Fprintf(w, + `{"data":{"enterprise":{"members":{"nodes":[%s],`+ + `"pageInfo":{"hasNextPage":%t,"endCursor":"member-cursor-%d"}}},`+ + `"rateLimit":{"limit":5000,"remaining":4998,"resetAt":"2026-09-18T23:00:00Z"}}}`, + strings.Join(nodes, ","), hasNextPage, pageIndex+1) + require.NoError(t, err) +} + +// writeInvitationBatch answers the aliased lookup the sync uses. Every login +// without an invitation contributes a NOT_FOUND entry to errors[] next to the +// aliases that did resolve, which is how GitHub answers a partial batch. +func (s *enterpriseStub) writeInvitationBatch(t *testing.T, w http.ResponseWriter, query string, variables map[string]any) { + t.Helper() + s.invitationBatches++ + + require.NotContains(t, query, `userLogin: "`, "logins must travel as variables, not in the query text") + require.Equal(t, string(githubv4.EnterpriseAdministratorRoleOwner), variables["role"]) + + aliases, failures := []string{}, []string{} + for i := 0; ; i++ { + login, ok := variables[fmt.Sprintf("l%d", i)].(string) + if !ok { + break + } + alias := fmt.Sprintf("i%d", i) + require.Contains(t, query, alias+": enterpriseAdministratorInvitation") + if invitationID, invited := s.invitations[login]; invited { + aliases = append(aliases, fmt.Sprintf(`%q:{"id":%q}`, alias, invitationID)) + continue + } + aliases = append(aliases, fmt.Sprintf(`%q:null`, alias)) + failures = append(failures, fmt.Sprintf( + `{"type":"NOT_FOUND","message":"Could not resolve to a pending invitation for %s."}`, login)) + } + + if s.batchErrorType != "" { + failures = append(failures, fmt.Sprintf( + `{"type":%q,"message":"the app lost access to the enterprise"}`, s.batchErrorType)) + } + + aliases = append(aliases, `"rateLimit":{"limit":5000,"remaining":4997,"resetAt":"2026-09-18T23:00:00Z"}`) + body := fmt.Sprintf(`{"data":{%s}`, strings.Join(aliases, ",")) + if len(failures) > 0 { + body += fmt.Sprintf(`,"errors":[%s]`, strings.Join(failures, ",")) + } + _, err := w.Write([]byte(body + "}")) + require.NoError(t, err) +} + +func (s *enterpriseStub) writeInvitation(t *testing.T, w http.ResponseWriter, variables map[string]any) { + t.Helper() + s.invitationQueries++ + + login, ok := variables["login"].(string) + require.True(t, ok, "invitation lookup must send the login as a variable") + require.Equal(t, string(githubv4.EnterpriseAdministratorRoleOwner), variables["role"]) + + invitationID, invited := s.invitations[login] + if !invited { + _, _ = fmt.Fprintf(w, + `{"data":{"enterpriseAdministratorInvitation":null},`+ + `"errors":[{"type":"NOT_FOUND","message":"Could not resolve to an invitation for %s."}]}`, login) + return + } + _, _ = fmt.Fprintf(w, `{"data":{"enterpriseAdministratorInvitation":{"id":%q}}}`, invitationID) +} + +// requireMutationShape rejects the two mutation bodies GitHub answers with a +// 200 plus an errors array: a payload without a selection set, and a +// Query-only rateLimit field selected on Mutation. +func requireMutationShape(t *testing.T, query string, field string) { + t.Helper() + + _, payload, ok := strings.Cut(query, field+"(input: $input)") + require.True(t, ok, "mutation %s must take its input as a variable", field) + require.True(t, strings.HasPrefix(payload, "{"), "mutation %s must select payload fields", field) + require.NotEqual(t, "{}", strings.TrimSuffix(payload, "}"), "mutation %s selection set is empty", field) + require.NotContains(t, query, "rateLimit", "rateLimit does not exist on type Mutation") +} + +func mutationInput(t *testing.T, variables map[string]any) map[string]any { + t.Helper() + + input, ok := variables["input"].(map[string]any) + require.True(t, ok, "mutation must send its input as a variable") + // The enterprise node ID is resolved once at construction. An empty one + // here means that step was skipped, which GitHub would reject at runtime. + if enterpriseID, present := input["enterpriseId"]; present { + require.Equal(t, testEnterpriseID, enterpriseID, + "mutation must carry the enterprise node ID resolved at construction") + } + return input +} + +// requireNoIdempotencyClaim asserts the operation actually acted instead of +// reporting the state as already correct. Emptiness is not the assertion: +// every path also carries the GraphQL budget left after reading the owners. +func requireNoIdempotencyClaim(t *testing.T, annos annotations.Annotations) { + t.Helper() + + var alreadyExists v2.GrantAlreadyExists + var alreadyRevoked v2.GrantAlreadyRevoked + require.False(t, annos.Contains(&alreadyExists)) + require.False(t, annos.Contains(&alreadyRevoked)) +} + +func newTestEnterpriseRoleBuilder( + t *testing.T, + stub *enterpriseStub, +) (*enterpriseRoleResourceType, *v2.Resource, *v2.Entitlement) { + t.Helper() + + if stub.invitations == nil { + stub.invitations = make(map[string]string) + } + + graphqlSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + stub.handle(t, w, r) + })) + t.Cleanup(graphqlSrv.Close) + + // Both installations point at the same stub: the test exercises the + // queries, not the two-token split. + enterpriseClient, err := newEnterpriseAdministratorClient( + graphqlSrv.URL, graphqlSrv.Client(), graphqlSrv.Client(), testOrg) + require.NoError(t, err) + // Mirrors construction: the node ID is resolved once, not per operation. + require.NoError(t, enterpriseClient.resolveEnterpriseNodeID(context.Background(), testEnterprise)) + + mgh := mocks.NewMockGitHub() + _, _, _, githubUser, _, err := mgh.Seed() + require.NoError(t, err) + + builder := EnterpriseRoleBuilder( + github.NewClient(mgh.Server()), + nil, + nil, + []string{testEnterprise}, + func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + return map[string]*githubEnterpriseAdministratorClient{testEnterprise: enterpriseClient}, nil + }, + ) + + principalID, err := resourceSdk.NewResourceID(resourceTypeUser, githubUser.GetID()) + require.NoError(t, err) + + roleResource, err := resourceSdk.NewRoleResource( + enterpriseRoleOwner, + resourceTypeEnterpriseRole, + testEnterprise+":"+enterpriseRoleOwner, + []resourceSdk.RoleTraitOption{}, + ) + require.NoError(t, err) + + ent := &v2.Entitlement{ + Id: entitlementSdk.NewEntitlementID(roleResource, enterpriseRoleAssigned), + Slug: enterpriseRoleAssigned, + Resource: roleResource, + } + + return builder, &v2.Resource{Id: principalID}, ent +} + +func TestEnterpriseRoleGrant(t *testing.T) { + t.Parallel() + ctx := context.Background() + + // updateEnterpriseAdministratorRole rejects anyone who is not already an + // administrator, so a plain member can only be invited. + // The invitation is reported as the grant it will become, which is what + // the sync emits too: returning nothing here would make C1 drop an overlay + // that the next sync puts straight back. + t.Run("invites a member and reports the grant", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + grants, annos, err := builder.Grant(ctx, principal, ent) + require.NoError(t, err) + requireNoIdempotencyClaim(t, annos) + require.Len(t, grants, 1) + require.Equal(t, testLogin, grants[0].GetPrincipal().GetId().GetResource()) + require.Equal(t, testLogin, stub.invitedLogin) + require.Empty(t, stub.updatedRole) + }) + + // A billing manager cannot be invited and is promoted in place instead. + // Which case applies is unreadable for a GitHub App, so the connector + // falls back once the invitation is rejected for that reason. + t.Run("promotes an administrator the invitation rejected", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{inviteErrorType: "UNPROCESSABLE"} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + grants, annos, err := builder.Grant(ctx, principal, ent) + require.NoError(t, err) + requireNoIdempotencyClaim(t, annos) + require.Len(t, grants, 1) + require.Equal(t, githubv4.EnterpriseAdministratorRoleOwner, stub.updatedRole) + require.Empty(t, stub.invitedLogin) + }) + + // Any other invitation failure must surface instead of triggering a second + // mutation the user never asked for. + t.Run("does not promote after an unrelated invitation failure", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + inviteErrorType: "RATE_LIMITED", + inviteErrorMessage: "rate limit exceeded", + } + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + _, _, err := builder.Grant(ctx, principal, ent) + require.Equal(t, codes.Unavailable, status.Code(err)) + require.Empty(t, stub.updatedRole) + }) + + // When both mutations fail, the promotion's status code is the one that + // reaches C1: it decides whether the task is worth retrying. + t.Run("surfaces the promotion status when both mutations fail", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{inviteErrorType: "UNPROCESSABLE", updateFails: true} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + _, _, err := builder.Grant(ctx, principal, ent) + require.Equal(t, codes.Unavailable, status.Code(err)) + require.ErrorContains(t, err, "promoting") + // The invitation error is not interpolated into the message: it is + // always the expected FailedPrecondition, and both errors carry the + // connector prefix, so repeating it makes the message unreadable. + require.Equal(t, 1, strings.Count(err.Error(), "baton-github:")) + }) + + t.Run("reports an owner as already granted", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{{id: testLoginID, login: testLogin}}}, + } + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + grants, annos, err := builder.Grant(ctx, principal, ent) + require.NoError(t, err) + require.Len(t, grants, 1) + + var alreadyExists v2.GrantAlreadyExists + require.True(t, annos.Contains(&alreadyExists)) + require.Empty(t, stub.updatedRole) + require.Empty(t, stub.invitedLogin) + + // Reading the owners spends GraphQL budget, so the remaining budget + // travels with the idempotency annotation instead of being dropped. + var rateLimit v2.RateLimitDescription + require.True(t, annos.Contains(&rateLimit)) + }) + + // The owner check has to page: the owners connection has no login filter. + t.Run("finds an owner on a later page", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{ + {{id: 99, login: "another-owner"}}, + {{id: testLoginID, login: testLogin}}, + }, + } + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + grants, annos, err := builder.Grant(ctx, principal, ent) + require.NoError(t, err) + require.Len(t, grants, 1) + + var alreadyExists v2.GrantAlreadyExists + require.True(t, annos.Contains(&alreadyExists)) + require.Equal(t, 2, stub.ownerQueries) + require.Empty(t, stub.invitedLogin) + }) + + // Re-inviting returns the same invitation, so a repeat grant must not send + // a second one. + t.Run("does not resend a pending invitation", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{invitations: map[string]string{testLogin: "EAI_existing"}} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + grants, annos, err := builder.Grant(ctx, principal, ent) + require.NoError(t, err) + require.Len(t, grants, 1) + + var alreadyExists v2.GrantAlreadyExists + require.True(t, annos.Contains(&alreadyExists)) + require.Empty(t, stub.invitedLogin, "an existing invitation must not be sent again") + }) + + // The mutation reporting success is not evidence that GitHub applied it. + // Without this guard C1 would record access that does not exist. + t.Run("rejects a grant GitHub did not apply", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{silentMutations: true} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + grants, _, err := builder.Grant(ctx, principal, ent) + require.Equal(t, codes.Unavailable, status.Code(err)) + require.Empty(t, grants) + require.Equal(t, testLogin, stub.invitedLogin) + }) + + t.Run("rejects a role other than owner", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + ent.Resource.Id.Resource = testEnterprise + ":Member" + + _, _, err := builder.Grant(ctx, principal, ent) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Empty(t, stub.invitedLogin) + }) +} + +func TestEnterpriseRoleRevoke(t *testing.T) { + t.Parallel() + ctx := context.Background() + + // UNAFFILIATED demotes the administrator but keeps enterprise membership; + // removeEnterpriseAdmin would evict them from the enterprise. + t.Run("demotes an active owner", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{{id: testLoginID, login: testLogin}}}, + } + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + annos, err := builder.Revoke(ctx, &v2.Grant{Principal: principal, Entitlement: ent}) + require.NoError(t, err) + requireNoIdempotencyClaim(t, annos) + require.Equal(t, enterpriseAdministratorRoleUnaffiliated, stub.updatedRole) + }) + + // Holding the role and carrying an invitation are not alternatives. If the + // revoke only demoted, the invitation would survive and the verification + // that follows would report a retryable failure for a demotion that had + // already gone through. + t.Run("clears both the role and a leftover invitation", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{{id: testLoginID, login: testLogin}}}, + invitations: map[string]string{testLogin: "EAI_leftover"}, + } + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + annos, err := builder.Revoke(ctx, &v2.Grant{Principal: principal, Entitlement: ent}) + require.NoError(t, err) + requireNoIdempotencyClaim(t, annos) + require.Equal(t, enterpriseAdministratorRoleUnaffiliated, stub.updatedRole) + require.Equal(t, "EAI_leftover", stub.cancelledID) + }) + + t.Run("cancels an invitation that was never accepted", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{invitations: map[string]string{testLogin: "EAI_existing"}} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + annos, err := builder.Revoke(ctx, &v2.Grant{Principal: principal, Entitlement: ent}) + require.NoError(t, err) + requireNoIdempotencyClaim(t, annos) + require.Equal(t, "EAI_existing", stub.cancelledID) + require.Empty(t, stub.updatedRole) + }) + + // GitHub expires an invitation after seven days, so a time-bound revoke can + // arrive once it is already gone. + t.Run("tolerates an invitation that expired mid-revoke", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + invitations: map[string]string{testLogin: "EAI_existing"}, + cancelNotFound: true, + } + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + annos, err := builder.Revoke(ctx, &v2.Grant{Principal: principal, Entitlement: ent}) + require.NoError(t, err) + requireNoIdempotencyClaim(t, annos) + require.Equal(t, "EAI_existing", stub.cancelledID) + }) + + // The mirror of the grant guard: reporting a revoke that GitHub did not + // apply would let C1 believe the access is gone while it is still there. + t.Run("rejects a revoke GitHub did not apply", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{{id: testLoginID, login: testLogin}}}, + silentMutations: true, + } + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + _, err := builder.Revoke(ctx, &v2.Grant{Principal: principal, Entitlement: ent}) + require.Equal(t, codes.Unavailable, status.Code(err)) + require.Equal(t, enterpriseAdministratorRoleUnaffiliated, stub.updatedRole) + }) + + t.Run("reports no owner access as already revoked", func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + annos, err := builder.Revoke(ctx, &v2.Grant{Principal: principal, Entitlement: ent}) + require.NoError(t, err) + require.Empty(t, stub.updatedRole) + require.Empty(t, stub.cancelledID) + + var alreadyRevoked v2.GrantAlreadyRevoked + require.True(t, annos.Contains(&alreadyRevoked)) + }) +} + +// drainGrants runs the sync the way the SDK does, following the page token +// until it empties, and returns every grant across both phases. +func drainGrants( + t *testing.T, + builder *enterpriseRoleResourceType, + resource *v2.Resource, +) []*v2.Grant { + t.Helper() + + var all []*v2.Grant + token := "" + for calls := 0; ; calls++ { + require.Less(t, calls, 20, "the page token never emptied") + grants, result, err := builder.Grants(context.Background(), resource, + resourceSdk.SyncOpAttrs{PageToken: pagination.Token{Token: token}}) + require.NoError(t, err) + all = append(all, grants...) + if result.NextPageToken == "" { + return all + } + token = result.NextPageToken + } +} + +// principals reduces grants to the principal IDs, which is what C1 keys access +// on and therefore what these tests care about. +func principals(grants []*v2.Grant) []string { + out := make([]string, 0, len(grants)) + for _, g := range grants { + out = append(out, g.GetPrincipal().GetId().GetResource()) + } + return out +} + +// An invitation nobody has accepted is emitted as a grant so C1 keeps a record +// of the request. C1 has no pending state, so it is the same entitlement an +// accepted owner gets; the connector tells them apart only when revoking. +func TestEnterpriseRoleGrantsIncludePendingInvitations(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{{id: 101, login: "accepted-owner"}}}, + memberPages: [][]enterpriseStubOwner{{{id: 202, login: "invited-member"}, {id: 303, login: "plain-member"}}}, + invitations: map[string]string{"invited-member": "EAI_invited"}, + } + builder, _, ent := newTestEnterpriseRoleBuilder(t, stub) + + require.ElementsMatch(t, []string{"101", "202"}, principals(drainGrants(t, builder, ent.Resource))) + // One request answered both members, rather than one lookup per login. + require.Equal(t, 1, stub.invitationBatches) + require.Zero(t, stub.invitationQueries, "the sync must not use the single-login lookup") +} + +// GitHub stops resolving an invitation that expires or is cancelled, so it +// simply stops being emitted and C1 drops the grant. Nothing tracks an expiry. +func TestEnterpriseRoleGrantsDropInvitationsThatStoppedResolving(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + memberPages: [][]enterpriseStubOwner{{{id: 202, login: "invited-member"}}}, + invitations: map[string]string{"invited-member": "EAI_invited"}, + } + builder, _, ent := newTestEnterpriseRoleBuilder(t, stub) + require.Equal(t, []string{"202"}, principals(drainGrants(t, builder, ent.Resource))) + + // The invitation lapses on GitHub's side. + delete(stub.invitations, "invited-member") + require.Empty(t, drainGrants(t, builder, ent.Resource)) + + // Had the invitee accepted instead, they would surface as an owner. + stub.ownerPages = [][]enterpriseStubOwner{{{id: 202, login: "invited-member"}}} + require.Equal(t, []string{"202"}, principals(drainGrants(t, builder, ent.Resource))) +} + +// A member who was never invited must not produce a grant, even though the +// batch reports them as NOT_FOUND alongside the invitation that did resolve. +func TestEnterpriseRoleGrantsIgnoreMembersWithoutAnInvitation(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + memberPages: [][]enterpriseStubOwner{{ + {id: 202, login: "invited-member"}, + {id: 303, login: "plain-member"}, + {id: 404, login: "another-plain-member"}, + }}, + invitations: map[string]string{"invited-member": "EAI_invited"}, + } + builder, _, ent := newTestEnterpriseRoleBuilder(t, stub) + + require.Equal(t, []string{"202"}, principals(drainGrants(t, builder, ent.Resource))) +} + +// The invitation batch always reports NOT_FOUND for the members with no +// invitation, so those entries must not decide the code for the whole +// response. NOT_FOUND is the one code the SDK downgrades to a warning: if a +// real failure were reported as NOT_FOUND, the sync would finish green having +// emitted no pending invitations, and C1 would read that as a revoke. +func TestEnterpriseRoleGrantsFailOnARealErrorInsideTheInvitationBatch(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + errorType string + want codes.Code + }{ + {errorType: "FORBIDDEN", want: codes.PermissionDenied}, + {errorType: "UNAUTHENTICATED", want: codes.Unauthenticated}, + {errorType: "RATE_LIMITED", want: codes.Unavailable}, + // An error type the classifier has no rule for must still fail the + // batch rather than inherit NOT_FOUND from its neighbours. + {errorType: "SERVICE_UNAVAILABLE", want: codes.Internal}, + } { + t.Run(tc.errorType, func(t *testing.T) { + t.Parallel() + stub := &enterpriseStub{ + memberPages: [][]enterpriseStubOwner{{{id: 202, login: "plain-member"}}}, + batchErrorType: tc.errorType, + } + builder, _, ent := newTestEnterpriseRoleBuilder(t, stub) + + // Phase one reports no owners, then the invitation phase fails. + _, result, err := builder.Grants(context.Background(), ent.Resource, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + _, _, err = builder.Grants(context.Background(), ent.Resource, + resourceSdk.SyncOpAttrs{PageToken: pagination.Token{Token: result.NextPageToken}}) + require.Equal(t, tc.want, status.Code(err)) + }) + } +} + +// The members are paged, and every page gets its own batched lookup. +func TestEnterpriseRoleGrantsPageThroughMembers(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + memberPages: [][]enterpriseStubOwner{ + {{id: 202, login: "invited-member"}}, + {{id: 303, login: "second-page-invitee"}}, + }, + invitations: map[string]string{ + "invited-member": "EAI_one", + "second-page-invitee": "EAI_two", + }, + } + builder, _, ent := newTestEnterpriseRoleBuilder(t, stub) + + require.ElementsMatch(t, []string{"202", "303"}, principals(drainGrants(t, builder, ent.Resource))) + require.Equal(t, 2, stub.memberQueries) + require.Equal(t, 2, stub.invitationBatches) +} + +// The owners are read through the organization, so an organization that +// belongs to a different enterprise would report the wrong owners. +func TestEnterpriseRoleVerifyOrganization(t *testing.T) { + t.Parallel() + ctx := context.Background() + + stub := &enterpriseStub{} + builder, _, _ := newTestEnterpriseRoleBuilder(t, stub) + enterpriseClients, err := builder.clients(ctx) + require.NoError(t, err) + client := enterpriseClients[testEnterprise] + + require.NoError(t, client.verifyOrganization(ctx, testEnterprise)) + require.Equal(t, 1, stub.organizationChecks) + + client.org = "org-of-another-enterprise" + err = client.verifyOrganization(ctx, testEnterprise) + require.ErrorContains(t, err, "does not belong to enterprise") +} + +// A missing enterprise installation must fail this resource type and nothing +// else, so the rest of the connector still syncs. Returning no resources would +// read to C1 as a revoke of every owner assignment. +func TestEnterpriseRoleFailsClosedWithoutEnterpriseClients(t *testing.T) { + t.Parallel() + ctx := context.Background() + + clientsErr := errors.New("github-connector: GitHub App is not installed on enterprise") + builds := 0 + builder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, + func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + builds++ + return nil, clientsErr + }, + ) + + _, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.ErrorIs(t, err, clientsErr) + + roleResource, err := resourceSdk.NewRoleResource( + enterpriseRoleOwner, + resourceTypeEnterpriseRole, + testEnterprise+":"+enterpriseRoleOwner, + []resourceSdk.RoleTraitOption{}, + ) + require.NoError(t, err) + + _, _, err = builder.Grants(ctx, roleResource, resourceSdk.SyncOpAttrs{}) + require.ErrorIs(t, err, clientsErr) + + // A misconfiguration answers the same way every time, so it is built once + // and remembered rather than re-probed on each call. + require.Equal(t, 1, builds) +} + +// A transient failure must not be remembered: freezing a blip at startup would +// disable the resource type until the process restarts. +func TestEnterpriseRoleRetriesARetryableClientFailure(t *testing.T) { + t.Parallel() + ctx := context.Background() + + builds := 0 + builder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, + func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + builds++ + if builds == 1 { + return nil, status.Error(codes.Unavailable, "rate limited") + } + // List only checks the enterprise is present, so the client + // itself is never dereferenced here. + return map[string]*githubEnterpriseAdministratorClient{testEnterprise: nil}, nil + }, + ) + + _, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.Equal(t, codes.Unavailable, status.Code(err)) + + resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + require.Equal(t, 2, builds) + + // Once it succeeds the result is memoized. + _, _, err = builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Equal(t, 2, builds) +} + +// Provisioning is only possible through an enterprise installation, so the PAT +// path must reject it rather than attempt a mutation it cannot make. +func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { + t.Parallel() + ctx := context.Background() + + stub := &enterpriseStub{} + builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) + + t.Run("rejects a principal that is not a user", func(t *testing.T) { + t.Parallel() + notAUser := &v2.Resource{Id: &v2.ResourceId{ + ResourceType: resourceTypeTeam.Id, + Resource: "1", + }} + _, _, err := builder.Grant(ctx, notAUser, ent) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + }) + + t.Run("rejects an enterprise without an app installation", func(t *testing.T) { + t.Parallel() + patBuilder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, nil) + _, _, err := patBuilder.Grant(ctx, principal, ent) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + }) +} + +// An enterprise owner does not have to be an owner of the organization the app +// reads them through: observed live with a user whose organizationRole was +// DIRECT_MEMBER. Organization.enterpriseOwners returns them regardless, and the +// connector must emit their grant. +// GitHub reports a GraphQL budget error as an HTTP 200 carrying errors[], so +// the status of the response says nothing. Reading the owners is the hottest +// GraphQL path in this role, and an unclassified budget error reaches the SDK +// as Unknown, which it does not retry: the sync aborts instead of backing off. +func TestEnterpriseRoleGrantsClassifyARateLimitedOwnersRead(t *testing.T) { + t.Parallel() + + builder, _, ent := newTestEnterpriseRoleBuilder(t, &enterpriseStub{ownersRateLimited: true}) + + _, _, err := builder.Grants(context.Background(), ent.Resource, resourceSdk.SyncOpAttrs{}) + require.Equal(t, codes.Unavailable, status.Code(err)) +} + +func TestEnterpriseRoleGrantsIncludeOwnerWhoIsNotAnOrgOwner(t *testing.T) { + t.Parallel() + + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{{ + {id: 158784853, login: "org-owner"}, + {id: 162376288, login: "enterprise-owner-only"}, + }}, + } + builder, _, ent := newTestEnterpriseRoleBuilder(t, stub) + + require.ElementsMatch(t, []string{"158784853", "162376288"}, + principals(drainGrants(t, builder, ent.Resource))) +} + +func TestEnterpriseRoleGrantsPagination(t *testing.T) { + t.Parallel() + ctx := context.Background() + + stub := &enterpriseStub{ + ownerPages: [][]enterpriseStubOwner{ + {{id: 101, login: "owner-page-one"}}, + {{id: 102, login: "owner-page-two"}}, + }, + } + builder, _, ent := newTestEnterpriseRoleBuilder(t, stub) + + resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + require.Equal(t, testEnterprise+":"+enterpriseRoleOwner, resources[0].GetId().GetResource()) + require.Zero(t, stub.ownerQueries) + + grants, result, err := builder.Grants(ctx, ent.Resource, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, grants, 1) + require.Equal(t, "101", grants[0].GetPrincipal().GetId().GetResource()) + require.NotEmpty(t, result.NextPageToken) + require.NotEmpty(t, result.Annotations) + + // The SDK drives the second page with the cursor from the first. + grants, result, err = builder.Grants(ctx, ent.Resource, resourceSdk.SyncOpAttrs{ + PageToken: pagination.Token{Token: result.NextPageToken}, + }) + require.NoError(t, err) + require.Len(t, grants, 1) + require.Equal(t, "102", grants[0].GetPrincipal().GetId().GetResource()) + require.Equal(t, 2, stub.ownerQueries) + + // The owners are exhausted, so the token moves on to the invitations + // rather than ending the sync. + require.NotEmpty(t, result.NextPageToken) + grants, result, err = builder.Grants(ctx, ent.Resource, resourceSdk.SyncOpAttrs{ + PageToken: pagination.Token{Token: result.NextPageToken}, + }) + require.NoError(t, err) + require.Empty(t, grants) + require.Empty(t, result.NextPageToken) + require.Equal(t, 2, stub.ownerQueries, "the invitation phase must not re-read the owners") + require.Equal(t, 1, stub.memberQueries) +} diff --git a/pkg/connector/graphql_transport.go b/pkg/connector/graphql_transport.go index d18fbf0c..b4508186 100644 --- a/pkg/connector/graphql_transport.go +++ b/pkg/connector/graphql_transport.go @@ -1,12 +1,16 @@ package connector import ( + "bytes" + "encoding/json" "fmt" "io" "net/http" + "strings" "github.com/conductorone/baton-sdk/pkg/ratelimit" "github.com/conductorone/baton-sdk/pkg/uhttp" + "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -55,3 +59,130 @@ func (t *statusClassifyingTransport) RoundTrip(req *http.Request) (*http.Respons } return nil, st.Err() } + +// graphQLEnvelope is the body GraphQL answers with. Data and Errors can both +// be populated at once, which is how a partially resolved request comes back: +// the fields that resolved sit in Data and the rest report in Errors. +type graphQLEnvelope struct { + Data map[string]json.RawMessage `json:"data"` + Errors []graphQLError `json:"errors"` +} + +// graphQLError is one entry of the errors array GraphQL returns in the body. +type graphQLError struct { + Message string `json:"message"` + Type string `json:"type"` + Extensions struct { + Code string `json:"code"` + } `json:"extensions"` +} + +// The error types GitHub puts on a GraphQL error. They live here, next to the +// classifier, because callers that tolerate a specific type have to agree with +// it: the invitation batch drops NOT_FOUND entries before classifying, and a +// drifting spelling would silently stop dropping them. +const ( + graphQLErrorNotFound = "NOT_FOUND" + graphQLErrorForbidden = "FORBIDDEN" + graphQLErrorUnauthenticated = "UNAUTHENTICATED" + graphQLErrorUnprocessable = "UNPROCESSABLE" +) + +// graphQLErrorType normalizes where GitHub puts the classification, which +// differs between the legacy top-level field and the extensions object. +func graphQLErrorType(graphQLErr graphQLError) string { + if code := strings.ToUpper(graphQLErr.Extensions.Code); code != "" { + return code + } + + return strings.ToUpper(graphQLErr.Type) +} + +// enterpriseGraphQLTransport classifies the errors GitHub reports inside an +// HTTP 200 GraphQL body, so callers can branch on a gRPC code instead of +// matching error text. +// +// It wraps only the enterprise administration client, because the enterprise +// owner mutations rely on telling apart "already an administrator" (which has a +// documented fallback), a rate limit (retryable) and a missing invitation +// (already revoked). The shared GraphQL client keeps returning the library's +// own error untouched, because userResourceType.checkOrgSAML detects +// enterprise-level SAML by matching the text of that error. +type enterpriseGraphQLTransport struct { + base http.RoundTripper +} + +func (t *enterpriseGraphQLTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.base.RoundTrip(req) + if err != nil || resp == nil { + return resp, err + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return resp, nil + } + + body, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("read GraphQL response: %w", readErr) + } + resp.Body = io.NopCloser(bytes.NewReader(body)) + + var envelope graphQLEnvelope + if unmarshalErr := json.Unmarshal(body, &envelope); unmarshalErr != nil { + // A body this transport cannot parse is not a transport failure: hand + // it to the GraphQL library, which owns decoding the payload. + return resp, nil //nolint:nilerr // decoding the payload is the library's job + } + if len(envelope.Errors) == 0 { + return resp, nil + } + + messages := make([]string, 0, len(envelope.Errors)) + for _, graphQLErr := range envelope.Errors { + messages = append(messages, graphQLErr.Message) + } + st := status.New(graphQLErrorsCode(envelope.Errors), strings.Join(messages, "; ")) + if rlDesc, _ := ratelimit.ExtractRateLimitData(resp.StatusCode, &resp.Header); rlDesc != nil { + if withDetails, detailsErr := st.WithDetails(rlDesc); detailsErr == nil { + st = withDetails + } + } + + return nil, st.Err() +} + +// graphQLErrorsCode maps a GraphQL errors array onto a gRPC code. A rate limit +// wins over everything else so the SDK retries instead of failing the sync, and +// a credential error wins over NOT_FOUND because callers treat NOT_FOUND as a +// benign absence. Past that the first classified entry wins, so a later error +// cannot mask the code an earlier one already established. +func graphQLErrorsCode(graphQLErrors []graphQLError) codes.Code { + code := codes.Internal + for _, graphQLErr := range graphQLErrors { + errorType := graphQLErrorType(graphQLErr) + + switch { + case strings.Contains(errorType, "RATE_LIMIT"), strings.Contains(errorType, "RATELIMIT"): + return codes.Unavailable + // A credential problem outranks NOT_FOUND specifically. Callers read + // NOT_FOUND as "the thing is already gone" and report success, so a + // FORBIDDEN hidden behind one would turn a permission failure into a + // silent no-op. It does not outrank the rest, which are answers about + // the request rather than about the credential. + case errorType == graphQLErrorForbidden && (code == codes.Internal || code == codes.NotFound): + code = codes.PermissionDenied + case errorType == graphQLErrorUnauthenticated && (code == codes.Internal || code == codes.NotFound): + code = codes.Unauthenticated + // UNPROCESSABLE is how GitHub rejects a well-formed mutation that the + // current state does not allow, e.g. inviting someone who already + // administers the enterprise. + case errorType == graphQLErrorUnprocessable && (code == codes.Internal || code == codes.NotFound): + code = codes.FailedPrecondition + case errorType == graphQLErrorNotFound && code == codes.Internal: + code = codes.NotFound + } + } + + return code +} diff --git a/pkg/connector/graphql_transport_test.go b/pkg/connector/graphql_transport_test.go index f5384efc..a971ad5f 100644 --- a/pkg/connector/graphql_transport_test.go +++ b/pkg/connector/graphql_transport_test.go @@ -2,6 +2,7 @@ package connector import ( "context" + "io" "net/http" "net/http/httptest" "strconv" @@ -70,6 +71,77 @@ func TestStatusClassifyingTransport_PassesThrough2xx(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode) } +func TestGraphQLErrorsCode(t *testing.T) { + cases := []struct { + name string + types []string + want codes.Code + }{ + {name: "unclassified", types: []string{"SOMETHING_NEW"}, want: codes.Internal}, + {name: "forbidden", types: []string{"FORBIDDEN"}, want: codes.PermissionDenied}, + {name: "not found", types: []string{"NOT_FOUND"}, want: codes.NotFound}, + {name: "unauthenticated", types: []string{"UNAUTHENTICATED"}, want: codes.Unauthenticated}, + {name: "unprocessable", types: []string{"UNPROCESSABLE"}, want: codes.FailedPrecondition}, + // A rate limit outranks everything: the SDK has to retry rather than + // fail the sync. + {name: "rate limit wins", types: []string{"FORBIDDEN", "RATE_LIMITED"}, want: codes.Unavailable}, + // Past a rate limit the first classified entry wins, so a later error + // cannot mask it. UNPROCESSABLE is what the invite-to-promote fallback + // in Grant branches on, and a trailing FORBIDDEN used to overwrite it. + {name: "first classified wins", types: []string{"UNPROCESSABLE", "FORBIDDEN"}, want: codes.FailedPrecondition}, + {name: "order independent", types: []string{"FORBIDDEN", "UNPROCESSABLE"}, want: codes.PermissionDenied}, + // NOT_FOUND is the one code a credential error may override. Callers + // read it as "already gone" and report success, so a batch whose + // missing invitations hide a FORBIDDEN must not look benign. + {name: "credential error beats not found", types: []string{"NOT_FOUND", "FORBIDDEN"}, want: codes.PermissionDenied}, + {name: "credential error beats not found, unauthenticated", types: []string{"NOT_FOUND", "UNAUTHENTICATED"}, want: codes.Unauthenticated}, + {name: "not found alone still maps to not found", types: []string{"NOT_FOUND", "NOT_FOUND"}, want: codes.NotFound}, + // Order must not decide whether Grant's invite-to-promote fallback + // fires, so UNPROCESSABLE outranks NOT_FOUND from either position. + {name: "unprocessable beats not found", types: []string{"NOT_FOUND", "UNPROCESSABLE"}, want: codes.FailedPrecondition}, + {name: "unprocessable beats not found, reversed", types: []string{"UNPROCESSABLE", "NOT_FOUND"}, want: codes.FailedPrecondition}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + graphQLErrors := make([]graphQLError, 0, len(tc.types)) + for _, errorType := range tc.types { + graphQLErrors = append(graphQLErrors, graphQLError{Type: errorType}) + } + require.Equal(t, tc.want, graphQLErrorsCode(graphQLErrors)) + }) + } +} + +// extensions.code is preferred over the top-level type, because GitHub sets it +// on the errors that carry a machine-readable classification. +func TestGraphQLErrorsCodePrefersExtensionsCode(t *testing.T) { + graphQLErr := graphQLError{Type: "FORBIDDEN"} + graphQLErr.Extensions.Code = "RATE_LIMITED" + + require.Equal(t, codes.Unavailable, graphQLErrorsCode([]graphQLError{graphQLErr})) +} + +func TestEnterpriseGraphQLTransport_PassesThroughUnparseableBody(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`not json`)) + })) + t.Cleanup(srv.Close) + + client := &http.Client{ + Transport: &enterpriseGraphQLTransport{base: http.DefaultTransport}, + } + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL+"/graphql", nil) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, err) + require.NotNil(t, resp) + t.Cleanup(func() { _ = resp.Body.Close() }) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "not json", string(body)) +} + func TestStatusClassifyingTransport_ClassifiesClient4xx(t *testing.T) { cases := []struct { httpStatus int diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index ece84762..b43b15e1 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -324,6 +324,50 @@ func isAuthError(resp *github.Response) bool { return resp.StatusCode == http.StatusUnauthorized } +// isRetryableError reports whether an error is worth attempting again, rather +// than a misconfiguration that will fail identically every time. +func isRetryableError(err error) bool { + if err == nil { + return false + } + + switch status.Code(err) { + case codes.Unavailable, codes.DeadlineExceeded, codes.ResourceExhausted: + return true + default: + return false + } +} + +// freshestRateLimit returns current with the rate limit of latest applied, +// keeping the earlier budget when latest reports none. +// +// It replaces rather than appends because Annotations.Pick returns the first +// match: a second descriptor added by Merge is never read, so the stale one +// would win over the fresh one. +func freshestRateLimit(current, latest annotations.Annotations) annotations.Annotations { + var rateLimit v2.RateLimitDescription + found, err := latest.Pick(&rateLimit) + if err != nil || !found { + return current + } + current.Update(&rateLimit) + + return current +} + +// isPermissionDenied reports the same condition as isPermissionError, for the +// paths that only have a typed error: the GraphQL clients return a gRPC status +// rather than a *github.Response. +func isPermissionDenied(err error) bool { + var grpcErr interface{ GRPCStatus() *status.Status } + if errors.As(err, &grpcErr) { + return grpcErr.GRPCStatus().Code() == codes.PermissionDenied + } + + return false +} + func isPermissionError(resp *github.Response) bool { if resp == nil { return false diff --git a/pkg/connector/token_refresh.go b/pkg/connector/token_refresh.go index e34db985..5827c528 100644 --- a/pkg/connector/token_refresh.go +++ b/pkg/connector/token_refresh.go @@ -4,7 +4,6 @@ import ( "context" "io" "net/http" - "net/url" "strings" "sync" @@ -158,16 +157,10 @@ func newGitHubAppClients(instanceURL string, httpClient *http.Client) (*github.C Transport: &statusClassifyingTransport{base: httpClient.Transport}, } - var gqlClient *githubv4.Client - if instanceURL != "" && instanceURL != githubDotCom { - gqlURL, err := url.Parse(instanceURL) - if err != nil { - return nil, nil, err - } - gqlURL.Path = "/api/graphql" - gqlClient = githubv4.NewEnterpriseClient(gqlURL.String(), gqlHTTPClient) - } else { - gqlClient = githubv4.NewClient(gqlHTTPClient) + endpoint, err := enterpriseGraphQLEndpoint(instanceURL) + if err != nil { + return nil, nil, err } - return gc, gqlClient, nil + + return gc, githubv4.NewEnterpriseClient(endpoint.String(), gqlHTTPClient), nil } diff --git a/pkg/customclient/client.go b/pkg/customclient/client.go index 85485447..8f2e3be7 100644 --- a/pkg/customclient/client.go +++ b/pkg/customclient/client.go @@ -4,35 +4,130 @@ import ( "context" "fmt" "net/http" + "net/url" + "strconv" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/uhttp" "github.com/google/go-github/v69/github" ) +// githubMaxPageSize is the largest per_page the GitHub REST API accepts; the +// default is 30. +const ( + githubMaxPageSize = 100 + + // AppInstallationsPageSize is the per_page this package requests for app + // installations. A shorter page tells the caller it reached the last one. + AppInstallationsPageSize = githubMaxPageSize +) + +// Endpoint paths, one element per path segment, because endpoint() escapes +// each element it is given. +var ( + appInstallationsPath = []string{"app", "installations"} + consumedLicensesPathParts = func(enterprise string) []string { + return []string{"enterprises", enterprise, "consumed-licenses"} + } +) + // used for endpoints not in the go-github library // example: https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/license?apiVersion=2022-11-28#list-enterprise-consumed-licenses type Client struct { *uhttp.BaseHttpClient + // baseURL is the go-github client's own base. Keeping it is what makes + // these endpoints follow --instance-url: on GitHub Enterprise Server + // WithEnterpriseURLs sets it to https://host/api/v3/, while a literal + // api.github.com would query GitHub.com instead of the instance. + baseURL *url.URL } func New(client *github.Client) *Client { return &Client{ BaseHttpClient: uhttp.NewBaseHttpClient(client.Client()), + baseURL: client.BaseURL, + } +} + +// endpoint resolves path segments against the base URL. Each element is one +// segment and is escaped: url.JoinPath treats its arguments as already-escaped +// path, so an unescaped value containing a slash would add segments and one +// containing ".." would climb out of the path. +// +// github.NewClient always sets a base URL, so the error only fires on a +// hand-built Client. +func (c *Client) endpoint(segments ...string) (string, error) { + if c.baseURL == nil { + return "", fmt.Errorf("github client has no base URL") + } + + escaped := make([]string, 0, len(segments)) + for _, segment := range segments { + escaped = append(escaped, url.PathEscape(segment)) + } + + return url.JoinPath(c.baseURL.String(), escaped...) +} + +// Authenticates with the app's JWT, not with an installation token. +// The go-github installation account is a *User and cannot carry the +// enterprise slug, so the response is decoded through this package's model. +// https://docs.github.com/en/rest/apps/apps#list-installations-for-the-authenticated-app +func (c *Client) ListAppInstallations(ctx context.Context, page int) ([]*AppInstallation, *v2.RateLimitDescription, error) { + endpoint, err := c.endpoint(appInstallationsPath...) + if err != nil { + return nil, nil, fmt.Errorf("error building the app installations URL: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + if err != nil { + return nil, nil, fmt.Errorf("error creating request to list app installations: %w", err) + } + + q := req.URL.Query() + q.Add("page", strconv.Itoa(page)) + q.Add("per_page", strconv.Itoa(AppInstallationsPageSize)) + req.URL.RawQuery = q.Encode() + + var target []*AppInstallation + var rateLimitData v2.RateLimitDescription + res, err := c.Do(req, + uhttp.WithJSONResponse(&target), + uhttp.WithRatelimitData(&rateLimitData), + ) + + if err != nil { + if res != nil { + logBody(ctx, res.Body) + } + return nil, &rateLimitData, fmt.Errorf("error listing app installations: %w", err) } + + defer res.Body.Close() + + if res.StatusCode < http.StatusOK || res.StatusCode >= http.StatusMultipleChoices { + logBody(ctx, res.Body) + return nil, &rateLimitData, fmt.Errorf("error listing app installations: %s", res.Status) + } + + return target, &rateLimitData, nil } // https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/license?apiVersion=2022-11-28#list-enterprise-consumed-licenses func (c *Client) ListEnterpriseConsumedLicenses(ctx context.Context, enterprise string, page int) (*EnterpriseConsumedLicense, *v2.RateLimitDescription, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("https://api.github.com/enterprises/%s/consumed-licenses", enterprise), nil) + endpoint, err := c.endpoint(consumedLicensesPathParts(enterprise)...) + if err != nil { + return nil, nil, fmt.Errorf("error building the consumed licenses URL: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return nil, nil, fmt.Errorf("error creating request to list enterprise consumed licenses: %w", err) } q := req.URL.Query() - q.Add("page", fmt.Sprintf("%d", page)) - // GitHub REST API max per_page is 100, default is 30. - q.Add("per_page", "100") + q.Add("page", strconv.Itoa(page)) + q.Add("per_page", strconv.Itoa(githubMaxPageSize)) req.URL.RawQuery = q.Encode() var target EnterpriseConsumedLicense diff --git a/pkg/customclient/models.go b/pkg/customclient/models.go index c77da78d..b55797a2 100644 --- a/pkg/customclient/models.go +++ b/pkg/customclient/models.go @@ -1,5 +1,15 @@ package customclient +type AppInstallation struct { + ID int64 `json:"id"` + TargetType string `json:"target_type"` + Account AppInstallationAccount `json:"account"` +} + +type AppInstallationAccount struct { + Slug string `json:"slug"` +} + // https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/license?apiVersion=2022-11-28#list-enterprise-consumed-licenses type EnterpriseConsumedLicense struct { TotalSeatsConsumed int `json:"total_seats_consumed"` From 04872b2bf6d79e8407a96c706ed7e09f21897be4 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 15:37:28 -0300 Subject: [PATCH 02/22] chore(github): extract the repeated GET literal in the endpoint mocks Co-authored-by: Cursor --- test/mocks/endpointpattern.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/test/mocks/endpointpattern.go b/test/mocks/endpointpattern.go index b7506972..368b08fa 100644 --- a/test/mocks/endpointpattern.go +++ b/test/mocks/endpointpattern.go @@ -2,9 +2,13 @@ package mocks import "github.com/migueleliasweb/go-github-mock/src/mock" +// methodGet is extracted so goconst stops flagging the repeated literal across +// the endpoint patterns below. +const methodGet = "GET" + var GetUserById = mock.EndpointPattern{ Pattern: "/user/{id}", - Method: "GET", + Method: methodGet, } var PutOrganizationsTeamsMembershipsByOrganizationByTeamIdByUsername = mock.EndpointPattern{ @@ -19,48 +23,48 @@ var DeleteOrganizationsTeamsMembershipsByOrganizationByTeamIdByUsername = mock.E var GetOrganizationById = mock.EndpointPattern{ Pattern: "/organizations/{org_id}", - Method: "GET", + Method: methodGet, } var GetOrgsByOrg = mock.EndpointPattern{ Pattern: "/orgs/{org}", - Method: "GET", + Method: methodGet, } var GetRepositoryById = mock.EndpointPattern{ Pattern: "/repositories/{repository_id}", - Method: "GET", + Method: methodGet, } var GetOrganizationsTeamByTeamId = mock.EndpointPattern{ Pattern: "/organizations/{org_id}/team/{team_id}", - Method: "GET", + Method: methodGet, } var GetOrganizationsTeamsMembersByTeamId = mock.EndpointPattern{ Pattern: "/organizations/{org_id}/team/{team_id}/members", - Method: "GET", + Method: methodGet, } var GetOrganizationsTeamsMembershipsByTeamIdByUsername = mock.EndpointPattern{ Pattern: "/organizations/{org_id}/team/{team_id}/memberships/{username}", - Method: "GET", + Method: methodGet, } // Organization role endpoints. var GetOrgsRolesByOrg = mock.EndpointPattern{ Pattern: "/orgs/{org}/organization-roles", - Method: "GET", + Method: methodGet, } var GetOrgsRolesTeamsByOrgByRoleId = mock.EndpointPattern{ Pattern: "/orgs/{org}/organization-roles/{role_id}/teams", - Method: "GET", + Method: methodGet, } var GetOrgsRolesUsersByOrgByRoleId = mock.EndpointPattern{ Pattern: "/orgs/{org}/organization-roles/{role_id}/users", - Method: "GET", + Method: methodGet, } var PutOrgsRolesUsersByOrgByRoleIdByUsername = mock.EndpointPattern{ From 212f4455ce592050260eb5fa11fd86cd3ecc7bfb Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 15:39:38 -0300 Subject: [PATCH 03/22] fix(github): bound the installations walk and reject traversal segments Co-authored-by: Cursor --- docs/docs-info.md | 8 ++++++++ pkg/connector/connector.go | 7 ++++--- pkg/customclient/client.go | 14 ++++++++++---- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/docs/docs-info.md b/docs/docs-info.md index b0658ab0..ebf95279 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -251,6 +251,14 @@ 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. Invitations that C1 itself creates are always visible, because C1 grants to a user it has already synced. +### Owner grants can reference a user the sync did not emit + +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. + +### PAT deployments advertise a provisioning capability they cannot use + +`CAPABILITY_PROVISION` is derived from the builder implementing the provisioner interface, not from the auth mode, so adding Grant and Revoke turns the capability on for `enterprise_role` under PAT authentication too. There, every request is rejected with `FailedPrecondition` naming the missing enterprise installation, including for the Owner role the consumed-licenses sync does emit. The alternative — advertising the capability only under App auth — is not expressible through that interface. + --- ## Authentication diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 105cd795..8495a501 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -590,8 +590,7 @@ func newEnterpriseRoleClients( // is decoded through customclient's own model. func listEnterpriseInstallations(ctx context.Context, client *customclient.Client) (map[string]int64, error) { installations := make(map[string]int64) - page := 1 - for { + for page := 1; page <= enterpriseMaxPages; page++ { pageInstallations, _, err := client.ListAppInstallations(ctx, page) if err != nil { return nil, fmt.Errorf("github-connector: failed to list app installations: %w", err) @@ -608,8 +607,10 @@ func listEnterpriseInstallations(ctx context.Context, client *customclient.Clien if len(pageInstallations) < customclient.AppInstallationsPageSize { return installations, nil } - page++ } + + return nil, fmt.Errorf( + "github-connector: gave up listing app installations after %d pages", enterpriseMaxPages) } func newGitHubGraphqlClient(ctx context.Context, instanceURL string, ts oauth2.TokenSource) (*githubv4.Client, error) { diff --git a/pkg/customclient/client.go b/pkg/customclient/client.go index 8f2e3be7..fd9fefde 100644 --- a/pkg/customclient/client.go +++ b/pkg/customclient/client.go @@ -50,11 +50,14 @@ func New(client *github.Client) *Client { } // endpoint resolves path segments against the base URL. Each element is one -// segment and is escaped: url.JoinPath treats its arguments as already-escaped -// path, so an unescaped value containing a slash would add segments and one -// containing ".." would climb out of the path. +// segment: url.JoinPath treats its arguments as already-escaped path, so a raw +// value containing a slash would silently add segments. // -// github.NewClient always sets a base URL, so the error only fires on a +// Escaping alone is not enough for traversal, because "." and ".." are +// unreserved and survive url.PathEscape, and JoinPath then resolves them — so +// they are rejected outright rather than escaped. +// +// github.NewClient always sets a base URL, so that error only fires on a // hand-built Client. func (c *Client) endpoint(segments ...string) (string, error) { if c.baseURL == nil { @@ -63,6 +66,9 @@ func (c *Client) endpoint(segments ...string) (string, error) { escaped := make([]string, 0, len(segments)) for _, segment := range segments { + if segment == "." || segment == ".." { + return "", fmt.Errorf("path segment %q would traverse the base URL", segment) + } escaped = append(escaped, url.PathEscape(segment)) } From 4203291c0a27ef5a1df065a9a03bc05830b16939 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 15:51:04 -0300 Subject: [PATCH 04/22] fix(github): keep the connector context for the memoized token refresher Co-authored-by: Cursor --- pkg/connector/connector.go | 15 ++++++++++++--- pkg/connector/enterprise_administrator_client.go | 10 ++++++++++ pkg/connector/enterprise_installations_test.go | 1 + pkg/customclient/client.go | 1 + 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 8495a501..b1753cbb 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -468,9 +468,14 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { // enterprise_role sync instead of the whole connector: the other resource // types keep syncing and C1 holds its previous owner state rather than // reading an empty list as a revoke of every owner. + // + // The construction context is captured separately: the clients are + // memoized for the process lifetime, so the token refresher inside them + // must not hold the context of whichever RPC happened to build them. + connectorCtx := ctx newEnterpriseRoleClientsFn := func(ctx context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { return newEnterpriseRoleClients( - ctx, ghc.InstanceUrl, appClient, jwtts, ghc.Enterprises, appHTTPClient, ghc.Org) + ctx, connectorCtx, ghc.InstanceUrl, appClient, jwtts, ghc.Enterprises, appHTTPClient, ghc.Org) } gh := &GitHub{ @@ -508,8 +513,12 @@ const enterpriseInstallationTargetType = "Enterprise" // when the organization does not belong to it: without a trustworthy owner // list the connector would emit an empty or foreign enterprise_role set, which // reads to C1 as a revoke of every owner assignment. +// ctx scopes the discovery requests to the caller; connectorCtx outlives them +// and is what the memoized clients keep for refreshing their installation +// token, which expires after an hour or on the first 401. func newEnterpriseRoleClients( ctx context.Context, + connectorCtx context.Context, instanceURL string, appClient *github.Client, jwtTokenSource oauth2.TokenSource, @@ -556,14 +565,14 @@ func newEnterpriseRoleClients( Expiry: token.GetExpiresAt().Time, }, &appTokenRefresher{ - ctx: ctx, + ctx: connectorCtx, instanceURL: instanceURL, installationID: installationID, jwtTokenSource: jwtTokenSource, }, ) - httpClient, err := newGitHubAppHTTPClient(ctx, ts) + httpClient, err := newGitHubAppHTTPClient(connectorCtx, ts) if err != nil { return nil, err } diff --git a/pkg/connector/enterprise_administrator_client.go b/pkg/connector/enterprise_administrator_client.go index b7f504c8..d1fa6f83 100644 --- a/pkg/connector/enterprise_administrator_client.go +++ b/pkg/connector/enterprise_administrator_client.go @@ -565,6 +565,7 @@ func (c *githubEnterpriseAdministratorClient) OwnerState( var annos annotations.Annotations var after *githubv4.String + walked := false for page := 0; page < enterpriseMaxPages; page++ { owners, nextCursor, pageAnnos, err := c.owners(ctx, after) if err != nil { @@ -581,10 +582,19 @@ func (c *githubEnterpriseAdministratorClient) OwnerState( } } if state.isOwner || nextCursor == "" { + walked = true break } after = githubv4.NewString(githubv4.String(nextCursor)) } + // Falling through the bound would report "not an owner" for someone the + // walk never finished reading, which Revoke would answer with + // GrantAlreadyRevoked while they still hold the role. + if !walked { + return state, annos, fmt.Errorf( + "baton-github: gave up reading the owners of enterprise %s after %d pages", + enterprise, enterpriseMaxPages) + } invitationID, err := c.pendingOwnerInvitation(ctx, enterprise, login) if err != nil { diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index fb0f04a3..84cce54c 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -167,6 +167,7 @@ func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { })) _, err := newEnterpriseRoleClients( + ctx, ctx, "https://github.com", client, diff --git a/pkg/customclient/client.go b/pkg/customclient/client.go index fd9fefde..5f13527d 100644 --- a/pkg/customclient/client.go +++ b/pkg/customclient/client.go @@ -104,6 +104,7 @@ func (c *Client) ListAppInstallations(ctx context.Context, page int) ([]*AppInst if err != nil { if res != nil { + defer res.Body.Close() logBody(ctx, res.Body) } return nil, &rateLimitData, fmt.Errorf("error listing app installations: %w", err) From 118b0b758c131a7429dddcd508c0b1a502d40841 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 16:02:28 -0300 Subject: [PATCH 05/22] refactor(github): drop the traversal guard and state what escaping covers Co-authored-by: Cursor --- pkg/connector/connector.go | 21 ++++++++------------- pkg/connector/enterprise_role.go | 21 +++++++++------------ pkg/customclient/client.go | 14 ++++++-------- 3 files changed, 23 insertions(+), 33 deletions(-) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index b1753cbb..140d779d 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -501,21 +501,16 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { const enterpriseInstallationTargetType = "Enterprise" // newEnterpriseRoleClients builds one client per configured enterprise, each -// authenticated with that enterprise's own installation access token. +// with that enterprise's own installation token: enterprise and organization +// installations are separate, and an enterprise mutation rejects the org token. // -// Enterprise installations are separate from organization installations and -// carry only enterprise permissions, so the enterprise installation is -// discovered through the app JWT (GET /app/installations) and gets its own -// token source. Sending the organization token to an enterprise mutation -// fails, and there is no way to widen the org token's scope. +// Fails closed when the app is not installed on an enterprise, or when the +// organization does not belong to it — an empty or foreign owner list reads to +// C1 as a revoke of every owner assignment. // -// Fails closed when the app is not installed on a configured enterprise, or -// when the organization does not belong to it: without a trustworthy owner -// list the connector would emit an empty or foreign enterprise_role set, which -// reads to C1 as a revoke of every owner assignment. -// ctx scopes the discovery requests to the caller; connectorCtx outlives them -// and is what the memoized clients keep for refreshing their installation -// token, which expires after an hour or on the first 401. +// ctx scopes the discovery requests to the caller. connectorCtx outlives them +// and is what the memoized clients keep for refreshing the installation token, +// which expires after an hour or on the first 401. func newEnterpriseRoleClients( ctx context.Context, connectorCtx context.Context, diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 9b69cf58..51faf261 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -300,21 +300,18 @@ func appList( return ret, &resourceSdk.SyncOpResults{}, nil } -// appGrants emits both the users who hold the Owner role today and the ones -// GitHub has invited but who have not accepted yet, against the same -// entitlement. The two are indistinguishable to C1, which has no pending grant -// state; the connector still tells them apart internally, because revoking an -// accepted owner and cancelling an unaccepted invitation are different +// appGrants emits the users who hold the Owner role today and the ones invited +// but not yet accepted, against the same entitlement. C1 has no pending grant +// state, so the two are indistinguishable there; the connector still tells them +// apart, because revoking an owner and cancelling an invitation are different // mutations. // -// An invitation that nobody accepts stops resolving on GitHub's side, so it -// simply stops being emitted and C1 removes the grant on that sync. Nothing -// here tracks an expiry. +// Nothing tracks an expiry: an invitation nobody accepts stops resolving on +// GitHub's side, so it stops being emitted and C1 drops the grant that sync. // -// The owners and the invitations are walked as two phases of one page token, -// because the invitations are not enumerable and have to be resolved by asking -// about the enterprise members in batches. An empty cursor drops the current -// phase, so the next call moves on and the token empties once both are done. +// Owners and invitations are two phases of one page token, because invitations +// are not enumerable and have to be resolved from the enterprise members. An +// empty cursor drops the current phase, emptying the token once both are done. func (o *enterpriseRoleResourceType) appGrants( ctx context.Context, enterpriseClients map[string]*githubEnterpriseAdministratorClient, diff --git a/pkg/customclient/client.go b/pkg/customclient/client.go index 5f13527d..df16c5a6 100644 --- a/pkg/customclient/client.go +++ b/pkg/customclient/client.go @@ -50,12 +50,13 @@ func New(client *github.Client) *Client { } // endpoint resolves path segments against the base URL. Each element is one -// segment: url.JoinPath treats its arguments as already-escaped path, so a raw -// value containing a slash would silently add segments. +// segment and is escaped, because url.JoinPath treats its arguments as +// already-escaped path: an unescaped value containing a slash would silently +// add segments. // -// Escaping alone is not enough for traversal, because "." and ".." are -// unreserved and survive url.PathEscape, and JoinPath then resolves them — so -// they are rejected outright rather than escaped. +// Escaping does not stop "." or ".." from being resolved away, which is fine +// here — every segment is either a constant or an operator-supplied config +// value, not caller input. // // github.NewClient always sets a base URL, so that error only fires on a // hand-built Client. @@ -66,9 +67,6 @@ func (c *Client) endpoint(segments ...string) (string, error) { escaped := make([]string, 0, len(segments)) for _, segment := range segments { - if segment == "." || segment == ".." { - return "", fmt.Errorf("path segment %q would traverse the base URL", segment) - } escaped = append(escaped, url.PathEscape(segment)) } From 3ae0c2ba31d03d59362f9d726b129468efd1a05a Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 16:56:16 -0300 Subject: [PATCH 06/22] refactor(github): look up the enterprise installation directly instead of paging every app installation Co-authored-by: Cursor --- docs/docs-info.md | 2 +- pkg/connector/connector.go | 54 ++------ .../enterprise_installations_test.go | 129 +++++------------- pkg/customclient/client.go | 56 +++----- pkg/customclient/models.go | 8 +- 5 files changed, 69 insertions(+), 180 deletions(-) diff --git a/docs/docs-info.md b/docs/docs-info.md index ebf95279..24e25bf4 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -299,7 +299,7 @@ GraphQL is used for SAML identity lookups, the audit log, and all enterprise own - `GET /orgs/{org}/installations` — installed GitHub Apps, requires `organization_administration=read` - `GET /orgs/{org}/personal-access-tokens` — fine-grained PATs, only with `--sync-secrets` - `GET /orgs/{org}/audit-log` — organization audit log -- `GET /app/installations` — the App's installations, authenticated with the App JWT, used to find the enterprise installation +- `GET /enterprises/{enterprise}/installation` — this App's installation on one enterprise, authenticated with the App JWT - `GET /enterprises/{enterprise}/consumed-licenses` — enterprise license consumption and enterprise SAML identities. **PAT only** **GraphQL**: diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 140d779d..167743bf 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -496,10 +496,6 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { return gh, nil } -// enterpriseInstallationTargetType is the target_type that -// GET /app/installations reports for an enterprise-level installation. -const enterpriseInstallationTargetType = "Enterprise" - // newEnterpriseRoleClients builds one client per configured enterprise, each // with that enterprise's own installation token: enterprise and organization // installations are separate, and an enterprise mutation rejects the org token. @@ -535,19 +531,22 @@ func newEnterpriseRoleClients( "%d were configured", org, len(enterprises)) } - installations, err := listEnterpriseInstallations(ctx, customclient.New(appClient)) - if err != nil { - return nil, err - } + installationClient := customclient.New(appClient) clients := make(map[string]*githubEnterpriseAdministratorClient, len(enterprises)) for _, enterprise := range enterprises { - installationID, ok := installations[strings.ToLower(enterprise)] - if !ok { - return nil, fmt.Errorf( - "github-connector: GitHub App is not installed on enterprise %q; install it on the enterprise account "+ - "with the Enterprise people read and write permission", enterprise) + installation, _, err := installationClient.GetEnterpriseInstallation(ctx, enterprise) + if err != nil { + // A 404 is the app not being installed on the enterprise, which is + // the misconfiguration worth naming. + if status.Code(err) == codes.NotFound { + return nil, fmt.Errorf( + "github-connector: GitHub App is not installed on enterprise %q; install it on the enterprise account "+ + "with the Enterprise people read and write permission", enterprise) + } + return nil, err } + installationID := installation.ID token, err := getInstallationToken(ctx, appClient, installationID) if err != nil { @@ -588,35 +587,6 @@ func newEnterpriseRoleClients( return clients, nil } -// listEnterpriseInstallations maps enterprise slug (lowercased) to installation -// ID for every enterprise installation of this app. go-github models the -// installation account as *User, which has no enterprise slug, so the response -// is decoded through customclient's own model. -func listEnterpriseInstallations(ctx context.Context, client *customclient.Client) (map[string]int64, error) { - installations := make(map[string]int64) - for page := 1; page <= enterpriseMaxPages; page++ { - pageInstallations, _, err := client.ListAppInstallations(ctx, page) - if err != nil { - return nil, fmt.Errorf("github-connector: failed to list app installations: %w", err) - } - - for _, installation := range pageInstallations { - if installation.TargetType != enterpriseInstallationTargetType || installation.Account.Slug == "" { - continue - } - installations[strings.ToLower(installation.Account.Slug)] = installation.ID - } - - // A page shorter than the one requested is the last one. - if len(pageInstallations) < customclient.AppInstallationsPageSize { - return installations, nil - } - } - - return nil, fmt.Errorf( - "github-connector: gave up listing app installations after %d pages", enterpriseMaxPages) -} - func newGitHubGraphqlClient(ctx context.Context, instanceURL string, ts oauth2.TokenSource) (*githubv4.Client, error) { endpoint, err := enterpriseGraphQLEndpoint(instanceURL) if err != nil { diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index 84cce54c..85fbf326 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -3,11 +3,9 @@ package connector import ( "context" "encoding/json" - "fmt" "net/http" "net/http/httptest" "net/url" - "sync/atomic" "testing" "github.com/google/go-github/v69/github" @@ -41,129 +39,70 @@ func newGitHubAPITestClientAt(t *testing.T, handler http.Handler, basePath strin return client } -func TestListEnterpriseInstallations(t *testing.T) { +func TestGetEnterpriseInstallation(t *testing.T) { t.Parallel() ctx := context.Background() - payload := []map[string]any{ - { - "id": int64(11), - "target_type": "Organization", - "account": map[string]any{"login": "example-org"}, - }, - { - "id": int64(22), - "target_type": "Enterprise", - "account": map[string]any{"slug": "Example-Enterprise"}, - }, - { - "id": int64(33), - "target_type": "Enterprise", - "account": map[string]any{"login": "missing-slug-enterprise"}, - }, - } - client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "/app/installations", r.URL.Path) - require.Equal(t, "1", r.URL.Query().Get("page")) - require.Equal(t, "100", r.URL.Query().Get("per_page")) + require.Equal(t, "/enterprises/example-enterprise/installation", r.URL.Path) w.Header().Set("Content-Type", "application/json") - require.NoError(t, json.NewEncoder(w).Encode(payload)) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "id": int64(22), + })) })) - installations, err := listEnterpriseInstallations(ctx, customclient.New(client)) + installation, _, err := customclient.New(client).GetEnterpriseInstallation(ctx, "example-enterprise") require.NoError(t, err) - require.Equal(t, map[string]int64{"example-enterprise": 22}, installations) + require.Equal(t, int64(22), installation.ID) } -// On GitHub Enterprise Server, WithEnterpriseURLs puts the REST API under -// /api/v3. Asking api.github.com instead would report the app as uninstalled -// on an enterprise that does have it. -func TestListEnterpriseInstallationsUsesTheInstanceBaseURL(t *testing.T) { +// A slug is operator-supplied, so it has to survive as one path segment +// instead of being pasted into the URL. +func TestGetEnterpriseInstallationEscapesTheSlug(t *testing.T) { t.Parallel() ctx := context.Background() - client := newGitHubAPITestClientAt(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "/api/v3/app/installations", r.URL.Path) - w.Header().Set("Content-Type", "application/json") - require.NoError(t, json.NewEncoder(w).Encode([]map[string]any{ - { - "id": int64(22), - "target_type": "Enterprise", - "account": map[string]any{"slug": "ghes-enterprise"}, - }, - })) - }), "/api/v3/") + client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/enterprises/a%2Fb/installation", r.URL.EscapedPath()) + w.WriteHeader(http.StatusNotFound) + })) - installations, err := listEnterpriseInstallations(ctx, customclient.New(client)) - require.NoError(t, err) - require.Equal(t, map[string]int64{"ghes-enterprise": 22}, installations) + _, _, err := customclient.New(client).GetEnterpriseInstallation(ctx, "a/b") + require.Error(t, err) } -// The endpoint reports no total, so a page shorter than the requested size is -// what ends the walk. -func TestListEnterpriseInstallationsPagination(t *testing.T) { +// On GitHub Enterprise Server, WithEnterpriseURLs puts the REST API under +// /api/v3. Asking api.github.com instead would report the app as uninstalled +// on an enterprise that does have it. +func TestGetEnterpriseInstallationUsesTheInstanceBaseURL(t *testing.T) { t.Parallel() ctx := context.Background() - var requests atomic.Int32 - - client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "/app/installations", r.URL.Path) - requests.Add(1) - - page := r.URL.Query().Get("page") - installations := make([]map[string]any, 0, customclient.AppInstallationsPageSize) - switch page { - case "1": - // A full page: one enterprise plus filler, so the caller must ask - // for the next one. - installations = append(installations, map[string]any{ - "id": int64(22), - "target_type": "Enterprise", - "account": map[string]any{"slug": "first-enterprise"}, - }) - for i := 1; i < customclient.AppInstallationsPageSize; i++ { - installations = append(installations, map[string]any{ - "id": int64(1000 + i), - "target_type": "Organization", - "account": map[string]any{"login": fmt.Sprintf("org-%d", i)}, - }) - } - case "2": - installations = append(installations, map[string]any{ - "id": int64(44), - "target_type": "Enterprise", - "account": map[string]any{"slug": "second-enterprise"}, - }) - default: - t.Fatalf("unexpected page %q: the walk must stop on a short page", page) - } - + client := newGitHubAPITestClientAt(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v3/enterprises/ghes-enterprise/installation", r.URL.Path) w.Header().Set("Content-Type", "application/json") - require.NoError(t, json.NewEncoder(w).Encode(installations)) - })) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "id": int64(33), + })) + }), "/api/v3/") - installations, err := listEnterpriseInstallations(ctx, customclient.New(client)) + installation, _, err := customclient.New(client).GetEnterpriseInstallation(ctx, "ghes-enterprise") require.NoError(t, err) - require.Equal(t, map[string]int64{"first-enterprise": 22, "second-enterprise": 44}, installations) - require.Equal(t, int32(2), requests.Load()) + require.Equal(t, int64(33), installation.ID) } +// GitHub answers 404 when the app is not installed on the enterprise. That has +// to fail this resource type with an actionable message rather than leave the +// connector reporting an empty owner set, which C1 reads as a revoke. func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { t.Parallel() ctx := context.Background() client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "/app/installations", r.URL.Path) + require.Equal(t, "/enterprises/example-enterprise/installation", r.URL.Path) w.Header().Set("Content-Type", "application/json") - require.NoError(t, json.NewEncoder(w).Encode([]map[string]any{ - { - "id": int64(11), - "target_type": "Organization", - "account": map[string]any{"login": "example-org"}, - }, - })) + w.WriteHeader(http.StatusNotFound) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"})) })) _, err := newEnterpriseRoleClients( diff --git a/pkg/customclient/client.go b/pkg/customclient/client.go index df16c5a6..e123a183 100644 --- a/pkg/customclient/client.go +++ b/pkg/customclient/client.go @@ -14,22 +14,17 @@ import ( // githubMaxPageSize is the largest per_page the GitHub REST API accepts; the // default is 30. -const ( - githubMaxPageSize = 100 - - // AppInstallationsPageSize is the per_page this package requests for app - // installations. A shorter page tells the caller it reached the last one. - AppInstallationsPageSize = githubMaxPageSize -) +const githubMaxPageSize = 100 // Endpoint paths, one element per path segment, because endpoint() escapes // each element it is given. -var ( - appInstallationsPath = []string{"app", "installations"} - consumedLicensesPathParts = func(enterprise string) []string { - return []string{"enterprises", enterprise, "consumed-licenses"} - } -) +func enterpriseInstallationPath(enterprise string) []string { + return []string{"enterprises", enterprise, "installation"} +} + +func consumedLicensesPathParts(enterprise string) []string { + return []string{"enterprises", enterprise, "consumed-licenses"} +} // used for endpoints not in the go-github library // example: https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/license?apiVersion=2022-11-28#list-enterprise-consumed-licenses @@ -73,49 +68,40 @@ func (c *Client) endpoint(segments ...string) (string, error) { return url.JoinPath(c.baseURL.String(), escaped...) } -// Authenticates with the app's JWT, not with an installation token. -// The go-github installation account is a *User and cannot carry the -// enterprise slug, so the response is decoded through this package's model. -// https://docs.github.com/en/rest/apps/apps#list-installations-for-the-authenticated-app -func (c *Client) ListAppInstallations(ctx context.Context, page int) ([]*AppInstallation, *v2.RateLimitDescription, error) { - endpoint, err := c.endpoint(appInstallationsPath...) +// GetEnterpriseInstallation returns this app's installation on one enterprise. +// +// Authenticates with the app's JWT, not with an installation token, and is +// decoded through this package's model because go-github's installation +// account is a *User, which carries no enterprise slug. +// https://docs.github.com/en/rest/apps/apps#get-an-enterprise-installation-for-the-authenticated-app +func (c *Client) GetEnterpriseInstallation(ctx context.Context, enterprise string) (*AppInstallation, *v2.RateLimitDescription, error) { + endpoint, err := c.endpoint(enterpriseInstallationPath(enterprise)...) if err != nil { - return nil, nil, fmt.Errorf("error building the app installations URL: %w", err) + return nil, nil, fmt.Errorf("error building the enterprise installation URL: %w", err) } req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { - return nil, nil, fmt.Errorf("error creating request to list app installations: %w", err) + return nil, nil, fmt.Errorf("error creating request to get the enterprise installation: %w", err) } - q := req.URL.Query() - q.Add("page", strconv.Itoa(page)) - q.Add("per_page", strconv.Itoa(AppInstallationsPageSize)) - req.URL.RawQuery = q.Encode() - - var target []*AppInstallation + var target AppInstallation var rateLimitData v2.RateLimitDescription res, err := c.Do(req, uhttp.WithJSONResponse(&target), uhttp.WithRatelimitData(&rateLimitData), ) - if err != nil { if res != nil { defer res.Body.Close() logBody(ctx, res.Body) } - return nil, &rateLimitData, fmt.Errorf("error listing app installations: %w", err) + return nil, &rateLimitData, fmt.Errorf("error getting the installation of enterprise %s: %w", enterprise, err) } defer res.Body.Close() - if res.StatusCode < http.StatusOK || res.StatusCode >= http.StatusMultipleChoices { - logBody(ctx, res.Body) - return nil, &rateLimitData, fmt.Errorf("error listing app installations: %s", res.Status) - } - - return target, &rateLimitData, nil + return &target, &rateLimitData, nil } // https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/license?apiVersion=2022-11-28#list-enterprise-consumed-licenses diff --git a/pkg/customclient/models.go b/pkg/customclient/models.go index b55797a2..ecf80e54 100644 --- a/pkg/customclient/models.go +++ b/pkg/customclient/models.go @@ -1,13 +1,7 @@ package customclient type AppInstallation struct { - ID int64 `json:"id"` - TargetType string `json:"target_type"` - Account AppInstallationAccount `json:"account"` -} - -type AppInstallationAccount struct { - Slug string `json:"slug"` + ID int64 `json:"id"` } // https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/license?apiVersion=2022-11-28#list-enterprise-consumed-licenses From c9de9ce960be32de451706300cab13a119ddbfe4 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 16:56:30 -0300 Subject: [PATCH 07/22] fix(github): report an exhausted page budget as itself and log the skipped member and rejected invitation Co-authored-by: Cursor --- docs/docs-info.md | 2 +- .../enterprise_administrator_client.go | 81 ++++++++++++------- pkg/connector/enterprise_role.go | 6 ++ pkg/connector/enterprise_role_test.go | 8 +- pkg/connector/graphql_transport.go | 6 +- 5 files changed, 67 insertions(+), 36 deletions(-) diff --git a/docs/docs-info.md b/docs/docs-info.md index 24e25bf4..878e7810 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -326,7 +326,7 @@ GraphQL is used for SAML identity lookups, the audit log, and all enterprise own - REST: 5,000 requests per hour for a PAT. A GitHub App installation gets a larger budget that scales with the account; installations on this connector's test enterprise reported 15,000 - GraphQL: a separate points-based budget, reported per query in the `rateLimit` field; App installations reported 10,000 -- The connector returns GitHub's rate limit headers and the GraphQL `rateLimit` values to the SDK as rate limit annotations, so it backs off rather than failing the sync. GraphQL errors arriving inside an HTTP 200 body are classified, so a rate limit surfaces as retryable rather than as an opaque failure +- The connector returns GitHub's rate limit headers to the SDK as rate limit annotations, so it backs off rather than failing the sync. The enterprise owner queries additionally report the GraphQL `rateLimit` field, and are the ones that classify the errors GitHub returns inside an HTTP 200 body, so a rate limit there surfaces as retryable rather than as an opaque failure --- diff --git a/pkg/connector/enterprise_administrator_client.go b/pkg/connector/enterprise_administrator_client.go index d1fa6f83..c0709c28 100644 --- a/pkg/connector/enterprise_administrator_client.go +++ b/pkg/connector/enterprise_administrator_client.go @@ -11,7 +11,9 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "github.com/shurcooL/githubv4" + "go.uber.org/zap" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" @@ -268,29 +270,34 @@ func (c *githubEnterpriseAdministratorClient) resolveEnterpriseNodeID(ctx contex return nil } -// verifyOrganization checks that the organization the owners are read from -// belongs to this enterprise. Otherwise the connector would report another +// enterpriseOrganizationsQuery reads one page of an enterprise's +// organizations, narrowed by a search term. +type enterpriseOrganizationsQuery struct { + Enterprise struct { + Organizations struct { + Nodes []struct { + Login githubv4.String + } + PageInfo struct { + HasNextPage githubv4.Boolean + EndCursor githubv4.String + } + } `graphql:"organizations(first: $first, after: $after, query: $query)"` + } `graphql:"enterprise(slug: $slug)"` +} + +// verifyOrganization checks that the organization the owners are read through +// belongs to this enterprise; otherwise the connector would report another // enterprise's owners as owners of this one. // -// organizations(query:) is a substring search, so an enterprise with many -// similarly named organizations can push the exact match past the first page. -// Every page is read before concluding that the organization is not there. +// organizations(query:) is a substring search rather than an exact filter, so +// every page is read before concluding the organization is not there. GitHub +// offers no direct way to ask an organization which enterprise owns it. func (c *githubEnterpriseAdministratorClient) verifyOrganization(ctx context.Context, enterprise string) error { var after *githubv4.String + walked := false for page := 0; page < enterpriseMaxPages; page++ { - var query struct { - Enterprise struct { - Organizations struct { - Nodes []struct { - Login githubv4.String - } - PageInfo struct { - HasNextPage githubv4.Boolean - EndCursor githubv4.String - } - } `graphql:"organizations(first: $first, after: $after, query: $query)"` - } `graphql:"enterprise(slug: $slug)"` - } + var query enterpriseOrganizationsQuery err := c.enterpriseClient.Query(ctx, &query, map[string]any{ enterpriseSlugVariable: githubv4.String(enterprise), enterpriseFirstVariable: githubv4.Int(enterpriseOrganizationPageSize), @@ -308,16 +315,23 @@ func (c *githubEnterpriseAdministratorClient) verifyOrganization(ctx context.Con } if !query.Enterprise.Organizations.PageInfo.HasNextPage { - return fmt.Errorf( - "baton-github: organization %s does not belong to enterprise %s, so its owners cannot be synced", - c.org, enterprise) + walked = true + break } after = githubv4.NewString(query.Enterprise.Organizations.PageInfo.EndCursor) } + // Running out of budget is not the same answer as reading every page and + // not finding the organization: reporting the membership error there would + // send the operator to fix a membership that is already correct. + if !walked { + return fmt.Errorf( + "baton-github: gave up looking for organization %s in enterprise %s after %d pages", + c.org, enterprise, enterpriseMaxPages) + } return fmt.Errorf( - "baton-github: gave up looking for organization %s in enterprise %s after %d pages", - c.org, enterprise, enterpriseMaxPages) + "baton-github: organization %s does not belong to enterprise %s, so its owners cannot be synced", + c.org, enterprise) } // enterpriseMembersQuery reads one page of the enterprise's member accounts. @@ -380,6 +394,14 @@ func (c *githubEnterpriseAdministratorClient) members( member.login = string(node.User.Login) } if member.databaseID == 0 || member.login == "" { + // Skipping costs this member their pending invitation lookup, so + // their Owner grant would never appear. Say so rather than drop + // them silently. + ctxzap.Extract(ctx).Debug("baton-github: skipping an enterprise member with no database ID or login", + zap.String("enterprise", enterprise), + zap.Int64("database_id", member.databaseID), + zap.String("login", member.login), + ) continue } members = append(members, member) @@ -551,11 +573,14 @@ func (c *githubEnterpriseAdministratorClient) pendingOwnerInvitation( // each separately: stopping at the role would let a stale invitation survive // the demotion and then fail the verification that follows it. // -// The owners connection does take a query argument, but it is a search rather -// than an exact-login filter, so an empty result cannot be trusted to mean -// "not an owner" — on Revoke that reading would report GrantAlreadyRevoked -// while the user keeps the role. The pages are walked and matched on the login -// instead, which in practice is one request: owners are a small set. +// The owners connection does take a query argument, but it searches profile +// text rather than filtering on the login: it also matches the display name. +// A search index carries no freshness guarantee, and Grant and Revoke call +// this again right after their mutation to confirm it landed, so an empty +// result cannot be trusted to mean "not an owner" — on Revoke that reading +// would report GrantAlreadyRevoked while the user keeps the role. The pages +// are walked and matched on the login instead, which in practice is one +// request: owners are a small set. func (c *githubEnterpriseAdministratorClient) OwnerState( ctx context.Context, enterprise string, diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 51faf261..db127b4d 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -488,6 +488,12 @@ func (o *enterpriseRoleResourceType) Grant( if status.Code(inviteErr) != codes.FailedPrecondition { return nil, annos, inviteErr } + // Only the promotion's error is returned, so GitHub's reason for + // refusing the invitation would otherwise be lost. + ctxzap.Extract(ctx).Debug("baton-github: invitation rejected, promoting in place instead", + zap.String("login", login), + zap.Error(inviteErr), + ) if promoteErr := client.UpdateRole( ctx, state.enterpriseID, login, githubv4.EnterpriseAdministratorRoleOwner, ); promoteErr != nil { diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index ed614e71..bf40241c 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -941,10 +941,6 @@ func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { }) } -// An enterprise owner does not have to be an owner of the organization the app -// reads them through: observed live with a user whose organizationRole was -// DIRECT_MEMBER. Organization.enterpriseOwners returns them regardless, and the -// connector must emit their grant. // GitHub reports a GraphQL budget error as an HTTP 200 carrying errors[], so // the status of the response says nothing. Reading the owners is the hottest // GraphQL path in this role, and an unclassified budget error reaches the SDK @@ -958,6 +954,10 @@ func TestEnterpriseRoleGrantsClassifyARateLimitedOwnersRead(t *testing.T) { require.Equal(t, codes.Unavailable, status.Code(err)) } +// An enterprise owner does not have to be an owner of the organization the app +// reads them through: observed live with a user whose organizationRole was +// DIRECT_MEMBER. Organization.enterpriseOwners returns them regardless, and the +// connector must emit their grant. func TestEnterpriseRoleGrantsIncludeOwnerWhoIsNotAnOrgOwner(t *testing.T) { t.Parallel() diff --git a/pkg/connector/graphql_transport.go b/pkg/connector/graphql_transport.go index b4508186..a1fb5b9c 100644 --- a/pkg/connector/graphql_transport.go +++ b/pkg/connector/graphql_transport.go @@ -154,9 +154,9 @@ func (t *enterpriseGraphQLTransport) RoundTrip(req *http.Request) (*http.Respons // graphQLErrorsCode maps a GraphQL errors array onto a gRPC code. A rate limit // wins over everything else so the SDK retries instead of failing the sync, and -// a credential error wins over NOT_FOUND because callers treat NOT_FOUND as a -// benign absence. Past that the first classified entry wins, so a later error -// cannot mask the code an earlier one already established. +// a credential or state error wins over NOT_FOUND because callers treat +// NOT_FOUND as a benign absence. Past that the first classified entry wins, so +// a later error cannot mask the code an earlier one already established. func graphQLErrorsCode(graphQLErrors []graphQLError) codes.Code { code := codes.Internal for _, graphQLErr := range graphQLErrors { From 83015610eea3b81c0838bdc8f28ac849668720b0 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 22 Sep 2026 17:24:42 -0300 Subject: [PATCH 08/22] fix(github): retry an unclassified client build failure instead of remembering it Co-authored-by: Cursor --- pkg/connector/connector.go | 11 +++- .../enterprise_administrator_client.go | 4 +- pkg/connector/enterprise_role.go | 10 ++-- pkg/connector/enterprise_role_test.go | 50 ++++++++++++++++++- pkg/connector/helpers.go | 15 ++++-- 5 files changed, 76 insertions(+), 14 deletions(-) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 167743bf..414f66d7 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -524,14 +524,21 @@ func newEnterpriseRoleClients( // organization belongs to exactly one enterprise, so only that enterprise // can be served. Saying so here beats failing later on a verification the // operator cannot satisfy. The PAT path does support a list. + // FailedPrecondition on the answers an operator has to change: it is what + // marks them safe to remember instead of rediscovering every call. if len(enterprises) > 1 { - return nil, fmt.Errorf( + return nil, status.Errorf(codes.FailedPrecondition, "github-connector: GitHub App authentication serves one enterprise at a time, "+ "because the owners are read through organization %q, which belongs to a single enterprise; "+ "%d were configured", org, len(enterprises)) } + // NewBaseHttpClient reports a failed cache setup by returning nil, which + // only panics later inside Do. installationClient := customclient.New(appClient) + if installationClient.BaseHttpClient == nil { + return nil, fmt.Errorf("github-connector: error building the enterprise installation client") + } clients := make(map[string]*githubEnterpriseAdministratorClient, len(enterprises)) for _, enterprise := range enterprises { @@ -540,7 +547,7 @@ func newEnterpriseRoleClients( // A 404 is the app not being installed on the enterprise, which is // the misconfiguration worth naming. if status.Code(err) == codes.NotFound { - return nil, fmt.Errorf( + return nil, status.Errorf(codes.FailedPrecondition, "github-connector: GitHub App is not installed on enterprise %q; install it on the enterprise account "+ "with the Enterprise people read and write permission", enterprise) } diff --git a/pkg/connector/enterprise_administrator_client.go b/pkg/connector/enterprise_administrator_client.go index c0709c28..7a0a5fc5 100644 --- a/pkg/connector/enterprise_administrator_client.go +++ b/pkg/connector/enterprise_administrator_client.go @@ -329,7 +329,9 @@ func (c *githubEnterpriseAdministratorClient) verifyOrganization(ctx context.Con c.org, enterprise, enterpriseMaxPages) } - return fmt.Errorf( + // FailedPrecondition so the caller remembers it: only a configuration + // change can make this organization belong to that enterprise. + return status.Errorf(codes.FailedPrecondition, "baton-github: organization %s does not belong to enterprise %s, so its owners cannot be synced", c.org, enterprise) } diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index db127b4d..88bb700a 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -65,10 +65,10 @@ func (o *enterpriseRoleResourceType) ResourceType(_ context.Context) *v2.Resourc // clients returns the per-enterprise administration clients, building them on // first use and memoizing the outcome. // -// A retryable failure is not memoized: discovering the installations is four -// network calls, and freezing a startup blip would disable this resource type -// for the lifetime of the process. A misconfiguration is memoized, because it -// will fail the same way every time. +// Only a misconfiguration is memoized, because it will fail the same way every +// time. Anything else is built again on the next call: discovering the +// installations is four network calls, and freezing a startup blip would +// disable this resource type for the lifetime of the process. func (o *enterpriseRoleResourceType) clients( ctx context.Context, ) (map[string]*githubEnterpriseAdministratorClient, error) { @@ -79,7 +79,7 @@ func (o *enterpriseRoleResourceType) clients( o.mu.Lock() defer o.mu.Unlock() - if o.enterpriseClientsSet && !isRetryableError(o.enterpriseClientsErr) { + if o.enterpriseClientsSet && (o.enterpriseClientsErr == nil || isPermanentError(o.enterpriseClientsErr)) { return o.enterpriseClients, o.enterpriseClientsErr } diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index bf40241c..2a92c2ff 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -3,7 +3,6 @@ package connector import ( "context" "encoding/json" - "errors" "fmt" "net/http" "net/http/httptest" @@ -853,7 +852,10 @@ func TestEnterpriseRoleFailsClosedWithoutEnterpriseClients(t *testing.T) { t.Parallel() ctx := context.Background() - clientsErr := errors.New("github-connector: GitHub App is not installed on enterprise") + // The code that produces this answer marks it FailedPrecondition, which is + // what makes it safe to remember. + clientsErr := status.Error(codes.FailedPrecondition, + "github-connector: GitHub App is not installed on enterprise") builds := 0 builder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { @@ -881,6 +883,50 @@ func TestEnterpriseRoleFailsClosedWithoutEnterpriseClients(t *testing.T) { require.Equal(t, 1, builds) } +// Building the clients calls go-github directly, whose errors arrive wrapped +// with %w and carry no gRPC status, so a 502 and a cancelled sync both read as +// Unknown. Remembering those would disable the resource type for the lifetime +// of the process on a blip the operator cannot see or fix. +func TestEnterpriseRoleRetriesAnUnclassifiedClientFailure(t *testing.T) { + t.Parallel() + ctx := context.Background() + + for _, tc := range []struct { + name string + err error + }{ + {"a go-github failure", fmt.Errorf("github-connector: failed to create installation token: %w", + &github.ErrorResponse{ + Response: &http.Response{StatusCode: http.StatusBadGateway, Request: &http.Request{}}, + Message: "Bad gateway", + })}, + {"a cancelled sync", fmt.Errorf("github-connector: discovering installations: %w", context.Canceled)}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + builds := 0 + builder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, + func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + builds++ + if builds == 1 { + return nil, tc.err + } + return map[string]*githubEnterpriseAdministratorClient{testEnterprise: nil}, nil + }, + ) + + _, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.ErrorIs(t, err, tc.err) + + resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + require.Equal(t, 2, builds) + }) + } +} + // A transient failure must not be remembered: freezing a blip at startup would // disable the resource type until the process restarts. func TestEnterpriseRoleRetriesARetryableClientFailure(t *testing.T) { diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index b43b15e1..830e22a3 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -324,15 +324,22 @@ func isAuthError(resp *github.Response) bool { return resp.StatusCode == http.StatusUnauthorized } -// isRetryableError reports whether an error is worth attempting again, rather -// than a misconfiguration that will fail identically every time. -func isRetryableError(err error) bool { +// isPermanentError reports whether an error will keep failing until an +// operator changes something, which is what makes it safe to remember. +// +// The test is which codes are permanent, not which are transient, because an +// unclassified error has to be retried: a go-github failure or a cancelled +// context reaches here wrapped with %w and carries no gRPC status, so +// status.Code reports Unknown for a 502, a rate limit and a cancelled sync +// alike. Treating Unknown as permanent would remember a blip forever. +func isPermanentError(err error) bool { if err == nil { return false } switch status.Code(err) { - case codes.Unavailable, codes.DeadlineExceeded, codes.ResourceExhausted: + case codes.PermissionDenied, codes.Unauthenticated, codes.NotFound, + codes.InvalidArgument, codes.FailedPrecondition: return true default: return false From 445cff9d5f3b22b0b556da924559d216d34c3e9b Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 09:20:32 -0300 Subject: [PATCH 09/22] fix(github): skip an enterprise without an app installation instead of failing the sync Co-authored-by: Cursor --- pkg/connector/connector.go | 40 ++++++------ .../enterprise_installations_test.go | 61 ++++++++++++++++--- pkg/connector/enterprise_role.go | 8 +++ pkg/connector/enterprise_role_test.go | 20 +++++- 4 files changed, 102 insertions(+), 27 deletions(-) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 414f66d7..05276cda 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -384,6 +384,17 @@ func appPrivateKeyPEM(ghc *cfg.Github) (string, error) { } func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { + // Rejected at startup rather than when the enterprise clients are built: + // there the error would reach List, and the SDK aborts the whole sync on + // it. This needs no network call, so the operator hears about it before + // anything runs. + if len(ghc.Enterprises) > 1 { + return nil, status.Errorf(codes.InvalidArgument, + "github-connector: GitHub App authentication serves one enterprise at a time, "+ + "because the owners are read through organization %q, which belongs to a single enterprise; "+ + "%d were configured", ghc.Org, len(ghc.Enterprises)) + } + privateKey, err := appPrivateKeyPEM(ghc) if err != nil { return nil, err @@ -520,19 +531,6 @@ func newEnterpriseRoleClients( if len(enterprises) == 0 { return nil, nil } - // The owners are read through the single configured organization, and an - // organization belongs to exactly one enterprise, so only that enterprise - // can be served. Saying so here beats failing later on a verification the - // operator cannot satisfy. The PAT path does support a list. - // FailedPrecondition on the answers an operator has to change: it is what - // marks them safe to remember instead of rediscovering every call. - if len(enterprises) > 1 { - return nil, status.Errorf(codes.FailedPrecondition, - "github-connector: GitHub App authentication serves one enterprise at a time, "+ - "because the owners are read through organization %q, which belongs to a single enterprise; "+ - "%d were configured", org, len(enterprises)) - } - // NewBaseHttpClient reports a failed cache setup by returning nil, which // only panics later inside Do. installationClient := customclient.New(appClient) @@ -544,12 +542,18 @@ func newEnterpriseRoleClients( for _, enterprise := range enterprises { installation, _, err := installationClient.GetEnterpriseInstallation(ctx, enterprise) if err != nil { - // A 404 is the app not being installed on the enterprise, which is - // the misconfiguration worth naming. + // A 404 is the app not being installed on the enterprise. That + // enterprise is skipped rather than failing: an error here aborts + // the whole sync, and before enterprise installations were read at + // all this configuration synced everything else fine. Provisioning + // still reports it, because there the operator asked for the role. if status.Code(err) == codes.NotFound { - return nil, status.Errorf(codes.FailedPrecondition, - "github-connector: GitHub App is not installed on enterprise %q; install it on the enterprise account "+ - "with the Enterprise people read and write permission", enterprise) + ctxzap.Extract(ctx).Debug( + "baton-github: GitHub App is not installed on the enterprise account, "+ + "so its owners cannot be synced or provisioned", + zap.String("enterprise", enterprise), + ) + continue } return nil, err } diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index 85fbf326..c20e74c5 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -8,11 +8,15 @@ import ( "net/url" "testing" + resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/google/go-github/v69/github" "github.com/stretchr/testify/require" "golang.org/x/oauth2" + cfg "github.com/conductorone/baton-github/pkg/config" "github.com/conductorone/baton-github/pkg/customclient" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // newGitHubAPITestClient points the client at a test server through BaseURL @@ -91,10 +95,12 @@ func TestGetEnterpriseInstallationUsesTheInstanceBaseURL(t *testing.T) { require.Equal(t, int64(33), installation.ID) } -// GitHub answers 404 when the app is not installed on the enterprise. That has -// to fail this resource type with an actionable message rather than leave the -// connector reporting an empty owner set, which C1 reads as a revoke. -func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { +// GitHub answers 404 when the app is not installed on the enterprise. That +// enterprise is skipped instead of erroring: an error out of the build reaches +// List, and the SDK aborts the entire sync on anything but NotFound, so a +// configuration that synced users and orgs fine would stop syncing at all. +// Skipping leaves no client, which is what makes Grant and Revoke report it. +func TestNewEnterpriseRoleClientsSkipsAnEnterpriseWithoutAnInstall(t *testing.T) { t.Parallel() ctx := context.Background() @@ -105,7 +111,7 @@ func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"})) })) - _, err := newEnterpriseRoleClients( + clients, err := newEnterpriseRoleClients( ctx, ctx, "https://github.com", @@ -115,7 +121,46 @@ func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { nil, "example-org", ) - require.Error(t, err) - require.Contains(t, err.Error(), `not installed on enterprise "example-enterprise"`) - require.NotContains(t, err.Error(), "personal access token") + require.NoError(t, err) + require.Empty(t, clients) +} + +// The owners are read through one organization, which belongs to one +// enterprise, so app auth cannot serve a list. Rejecting it while the +// connector is being built keeps it out of the sync, where the SDK would turn +// it into an aborted run instead of a message about the configuration. +func TestNewWithGithubAppRejectsSeveralEnterprises(t *testing.T) { + t.Parallel() + + _, err := newWithGithubApp(context.Background(), &cfg.Github{ + Org: "example-org", + Enterprises: []string{"one-enterprise", "another-enterprise"}, + }) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Contains(t, err.Error(), "one enterprise at a time") +} + +// An error out of List aborts the whole sync: the SDK downgrades only +// NotFound, and anything else cancels the batch. So an app that is not +// installed on the enterprise has to leave List reporting what it did before +// enterprise installations were read at all, which under app auth is nothing: +// the consumed-licenses API it falls back to is PAT-only and answers 403. +func TestEnterpriseRoleListSurvivesWithoutEnterpriseClients(t *testing.T) { + t.Parallel() + + ctx := context.Background() + apiClient := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"message": "Resource not accessible by integration"})) + })) + + builder := EnterpriseRoleBuilder(apiClient, apiClient, customclient.New(apiClient), []string{"example-enterprise"}, + func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + return map[string]*githubEnterpriseAdministratorClient{}, nil + }, + ) + + resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Empty(t, resources) } diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 88bb700a..3588bf30 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -598,6 +598,14 @@ func (o *enterpriseRoleResourceType) provisioningTarget( } client, ok := enterpriseClients[enterprise] if !ok { + // Naming the credential matters: under a PAT there is no App to + // install, so the other message sends the operator after a fix that + // cannot apply. + if o.newEnterpriseClients == nil { + return "", nil, status.Errorf(codes.FailedPrecondition, + "baton-github: provisioning enterprise %s requires GitHub App authentication; "+ + "a personal access token can sync enterprise roles but cannot provision them", enterprise) + } return "", nil, status.Errorf(codes.FailedPrecondition, "baton-github: provisioning enterprise %s requires a GitHub App installed on the enterprise account", enterprise) } diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index 2a92c2ff..6407ff78 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -979,11 +979,29 @@ func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { require.Equal(t, codes.InvalidArgument, status.Code(err)) }) - t.Run("rejects an enterprise without an app installation", func(t *testing.T) { + // Under a PAT there is no app to install, so naming one sends the operator + // after a fix that cannot apply. + t.Run("names the credential when the token cannot provision", func(t *testing.T) { t.Parallel() patBuilder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, nil) _, _, err := patBuilder.Grant(ctx, principal, ent) require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.Contains(t, err.Error(), "requires GitHub App authentication") + }) + + // With app auth the enterprise is skipped during sync when the app is not + // installed on it, so the client is missing here for a reason the operator + // can act on. + t.Run("names the missing installation under app auth", func(t *testing.T) { + t.Parallel() + appBuilder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, + func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + return map[string]*githubEnterpriseAdministratorClient{}, nil + }, + ) + _, _, err := appBuilder.Grant(ctx, principal, ent) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.Contains(t, err.Error(), "installed on the enterprise account") }) } From 32d2e0030870efc95a8ae6ef155e08ad823061f0 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 09:39:40 -0300 Subject: [PATCH 10/22] fix(github): stand down enterprise roles on an ambiguous config and re-probe an incomplete build Co-authored-by: Cursor --- pkg/connector/connector.go | 28 ++++++++----- .../enterprise_installations_test.go | 34 +++++++++------ pkg/connector/enterprise_role.go | 18 +++++--- pkg/connector/enterprise_role_test.go | 41 +++++++++++++++++++ 4 files changed, 92 insertions(+), 29 deletions(-) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 05276cda..2576e3c8 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -384,17 +384,6 @@ func appPrivateKeyPEM(ghc *cfg.Github) (string, error) { } func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { - // Rejected at startup rather than when the enterprise clients are built: - // there the error would reach List, and the SDK aborts the whole sync on - // it. This needs no network call, so the operator hears about it before - // anything runs. - if len(ghc.Enterprises) > 1 { - return nil, status.Errorf(codes.InvalidArgument, - "github-connector: GitHub App authentication serves one enterprise at a time, "+ - "because the owners are read through organization %q, which belongs to a single enterprise; "+ - "%d were configured", ghc.Org, len(ghc.Enterprises)) - } - privateKey, err := appPrivateKeyPEM(ghc) if err != nil { return nil, err @@ -531,6 +520,23 @@ func newEnterpriseRoleClients( if len(enterprises) == 0 { return nil, nil } + // The owners are read through the single configured organization, and an + // organization belongs to exactly one enterprise, so app auth cannot serve + // a list and picking one of them would be a guess. Only this resource type + // stands down: returning an error instead would reach List, where the SDK + // aborts the entire sync over a configuration that syncs everything else + // today. The PAT path does support a list. + if len(enterprises) > 1 { + ctxzap.Extract(ctx).Debug( + "baton-github: GitHub App authentication serves one enterprise at a time, "+ + "because the owners are read through one organization, which belongs to a single enterprise; "+ + "enterprise roles will not be synced or provisioned", + zap.String("org", org), + zap.Strings("enterprises", enterprises), + ) + return nil, nil + } + // NewBaseHttpClient reports a failed cache setup by returning nil, which // only panics later inside Do. installationClient := customclient.New(appClient) diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index c20e74c5..e3a057e5 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -13,10 +13,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/oauth2" - cfg "github.com/conductorone/baton-github/pkg/config" "github.com/conductorone/baton-github/pkg/customclient" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) // newGitHubAPITestClient points the client at a test server through BaseURL @@ -126,18 +123,29 @@ func TestNewEnterpriseRoleClientsSkipsAnEnterpriseWithoutAnInstall(t *testing.T) } // The owners are read through one organization, which belongs to one -// enterprise, so app auth cannot serve a list. Rejecting it while the -// connector is being built keeps it out of the sync, where the SDK would turn -// it into an aborted run instead of a message about the configuration. -func TestNewWithGithubAppRejectsSeveralEnterprises(t *testing.T) { +// enterprise, so app auth cannot serve a list. Standing down is what keeps it +// to this resource type: an error would reach List, where the SDK aborts a +// sync that delivers users, orgs, teams and repositories today. +func TestNewEnterpriseRoleClientsStandsDownForSeveralEnterprises(t *testing.T) { t.Parallel() - _, err := newWithGithubApp(context.Background(), &cfg.Github{ - Org: "example-org", - Enterprises: []string{"one-enterprise", "another-enterprise"}, - }) - require.Equal(t, codes.InvalidArgument, status.Code(err)) - require.Contains(t, err.Error(), "one enterprise at a time") + ctx := context.Background() + client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("no request expected, got %s", r.URL.Path) + })) + + clients, err := newEnterpriseRoleClients( + ctx, + ctx, + "https://github.com", + client, + oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "unused"}), + []string{"one-enterprise", "another-enterprise"}, + nil, + "example-org", + ) + require.NoError(t, err) + require.Empty(t, clients) } // An error out of List aborts the whole sync: the SDK downgrades only diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 3588bf30..7861d628 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -65,10 +65,10 @@ func (o *enterpriseRoleResourceType) ResourceType(_ context.Context) *v2.Resourc // clients returns the per-enterprise administration clients, building them on // first use and memoizing the outcome. // -// Only a misconfiguration is memoized, because it will fail the same way every -// time. Anything else is built again on the next call: discovering the -// installations is four network calls, and freezing a startup blip would -// disable this resource type for the lifetime of the process. +// Only a complete build, or a misconfiguration that will fail the same way +// every time, is memoized. Anything else is built again on the next call: +// discovering the installations is four network calls, and freezing a startup +// blip would disable this resource type for the lifetime of the process. func (o *enterpriseRoleResourceType) clients( ctx context.Context, ) (map[string]*githubEnterpriseAdministratorClient, error) { @@ -79,7 +79,7 @@ func (o *enterpriseRoleResourceType) clients( o.mu.Lock() defer o.mu.Unlock() - if o.enterpriseClientsSet && (o.enterpriseClientsErr == nil || isPermanentError(o.enterpriseClientsErr)) { + if o.enterpriseClientsSet && (o.clientsAreComplete() || isPermanentError(o.enterpriseClientsErr)) { return o.enterpriseClients, o.enterpriseClientsErr } @@ -89,6 +89,14 @@ func (o *enterpriseRoleResourceType) clients( return o.enterpriseClients, o.enterpriseClientsErr } +// clientsAreComplete reports whether every configured enterprise resolved to a +// client. A build that skipped one succeeded without an error, so remembering +// it would keep an operator who installs the app on that enterprise reading +// the old answer until the process restarts. +func (o *enterpriseRoleResourceType) clientsAreComplete() bool { + return o.enterpriseClientsErr == nil && len(o.enterpriseClients) == len(o.enterprises) +} + func (o *enterpriseRoleResourceType) cacheRole(roleId string, userLogin string) { o.mu.Lock() defer o.mu.Unlock() diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index 6407ff78..1cdb8106 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -20,6 +20,7 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "github.com/conductorone/baton-github/pkg/customclient" "github.com/conductorone/baton-github/test/mocks" ) @@ -927,6 +928,46 @@ func TestEnterpriseRoleRetriesAnUnclassifiedClientFailure(t *testing.T) { } } +// An enterprise the app is not installed on is skipped, so the build succeeds +// while resolving nothing. Remembering that would leave an operator who reads +// the error from Grant, installs the app and retries still reading the old +// answer until the process restarts. +func TestEnterpriseRoleReprobesAnIncompleteClientBuild(t *testing.T) { + t.Parallel() + ctx := context.Background() + + // Without a client the list falls back to consumed-licenses, which is + // PAT-only and answers 403 to an app. + apiClient := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + + builds := 0 + builder := EnterpriseRoleBuilder(apiClient, apiClient, customclient.New(apiClient), []string{testEnterprise}, + func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + builds++ + if builds == 1 { + // The app is not installed yet: nothing resolved, no error. + return map[string]*githubEnterpriseAdministratorClient{}, nil + } + return map[string]*githubEnterpriseAdministratorClient{testEnterprise: nil}, nil + }, + ) + + _, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + + resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + require.Equal(t, 2, builds) + + // Complete now, so it is remembered. + _, _, err = builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Equal(t, 2, builds) +} + // A transient failure must not be remembered: freezing a blip at startup would // disable the resource type until the process restarts. func TestEnterpriseRoleRetriesARetryableClientFailure(t *testing.T) { From 7c843cb490cc442f263fb45d8a0019a94c091ece Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 10:44:40 -0300 Subject: [PATCH 11/22] fix(github): fail the sync when no enterprise client can be built instead of reporting no owners Co-authored-by: Cursor --- docs/docs-info.md | 12 +++- pkg/connector/connector.go | 42 ++++++------- .../enterprise_installations_test.go | 61 ++++++------------- pkg/connector/enterprise_role.go | 39 ++++++------ pkg/connector/enterprise_role_test.go | 60 +++--------------- pkg/connector/graphql_transport_test.go | 2 +- 6 files changed, 75 insertions(+), 141 deletions(-) diff --git a/docs/docs-info.md b/docs/docs-info.md index 878e7810..608be3f9 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -235,8 +235,16 @@ Consequences worth knowing: - Grant returns the grant in both cases: when it promoted an administrator, and when it only created an invitation. `Grants()` matches that and emits pending invitations alongside accepted Owners, so C1 keeps a record of the request from the moment it is made - Revoke clears both states rather than treating them as alternatives: it demotes an active Owner to `UNAFFILIATED`, which keeps them as a member of the enterprise rather than evicting them, and cancels an unaccepted invitation. A `NOT_FOUND` on either is success, because it means the state being asked for is already in place -- Reading owners uses the **organization** installation token and every mutation uses the **enterprise** installation token; the enterprise token is rejected on organization fields. Startup verifies that the configured organization belongs to the configured enterprise, and a failure there fails only this resource type -- Only one enterprise can be served under App authentication, because the owners are read through the single configured organization and an organization belongs to exactly one enterprise. A configuration naming several is rejected while the clients are built, which fails this resource type with an explanatory error rather than failing later on a check the operator cannot satisfy. The PAT path does accept a list +- Reading owners uses the **organization** installation token and every mutation uses the **enterprise** installation token; the enterprise token is rejected on organization fields. Startup verifies that the configured organization belongs to the configured enterprise, and a failure there fails the sync +- Only one enterprise can be served under App authentication, because the owners are read through the single configured organization and an organization belongs to exactly one enterprise. A configuration naming several is rejected while the clients are built, with an explanatory error rather than a later failure on a check the operator cannot satisfy. The PAT path does accept a list + +### A missing enterprise installation fails the sync rather than emitting nothing + +Under App authentication the connector refuses to sync when it cannot build an enterprise administration client: the app is not installed on the enterprise account, several enterprises are configured, or the organization does not belong to the configured one. The whole sync fails, not just this resource type. + +That is deliberate, and the alternative is worse. C1 deletes every resource of a type that a completed sync did not report, and it applies that to a resource type whose list came back empty for any reason. Letting the sync finish while reading no owners would therefore delete the Owner role and every grant on it, silently, and GitHub answers `404` for an uninstalled app, a revoked permission and a slug typo alike — the connector cannot tell a genuine uninstall from a blip. An error keeps the sync from completing, so nothing is deleted. + +The cost falls on a deployment that sets `--enterprises` under App authentication without installing the app on the enterprise account. That combination produced no enterprise data before this capability existed, and now stops the sync until the app is installed or the flag is removed. ### Pending invitations look the same as real access diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 2576e3c8..bc35d17c 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -501,8 +501,12 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { // installations are separate, and an enterprise mutation rejects the org token. // // Fails closed when the app is not installed on an enterprise, or when the -// organization does not belong to it — an empty or foreign owner list reads to -// C1 as a revoke of every owner assignment. +// organization does not belong to it. Returning no clients instead would let +// the sync complete while reading no owners, and C1 deletes every resource of +// a type that a completed sync did not report — the Owner role and every +// grant on it. Failing is what stops a sync from being completed at all, so +// an operator who has not installed the app on the enterprise account, or who +// configured several, hears about it instead of losing the assignments. // // ctx scopes the discovery requests to the caller. connectorCtx outlives them // and is what the memoized clients keep for refreshing the installation token, @@ -522,19 +526,13 @@ func newEnterpriseRoleClients( } // The owners are read through the single configured organization, and an // organization belongs to exactly one enterprise, so app auth cannot serve - // a list and picking one of them would be a guess. Only this resource type - // stands down: returning an error instead would reach List, where the SDK - // aborts the entire sync over a configuration that syncs everything else - // today. The PAT path does support a list. + // a list and picking one of them would be a guess. The PAT path does + // support a list. if len(enterprises) > 1 { - ctxzap.Extract(ctx).Debug( - "baton-github: GitHub App authentication serves one enterprise at a time, "+ - "because the owners are read through one organization, which belongs to a single enterprise; "+ - "enterprise roles will not be synced or provisioned", - zap.String("org", org), - zap.Strings("enterprises", enterprises), - ) - return nil, nil + return nil, status.Errorf(codes.FailedPrecondition, + "github-connector: GitHub App authentication serves one enterprise at a time, "+ + "because the owners are read through organization %q, which belongs to a single enterprise; "+ + "%d were configured", org, len(enterprises)) } // NewBaseHttpClient reports a failed cache setup by returning nil, which @@ -548,18 +546,12 @@ func newEnterpriseRoleClients( for _, enterprise := range enterprises { installation, _, err := installationClient.GetEnterpriseInstallation(ctx, enterprise) if err != nil { - // A 404 is the app not being installed on the enterprise. That - // enterprise is skipped rather than failing: an error here aborts - // the whole sync, and before enterprise installations were read at - // all this configuration synced everything else fine. Provisioning - // still reports it, because there the operator asked for the role. + // A 404 is the app not being installed on the enterprise, which is + // the misconfiguration worth naming. if status.Code(err) == codes.NotFound { - ctxzap.Extract(ctx).Debug( - "baton-github: GitHub App is not installed on the enterprise account, "+ - "so its owners cannot be synced or provisioned", - zap.String("enterprise", enterprise), - ) - continue + return nil, status.Errorf(codes.FailedPrecondition, + "github-connector: GitHub App is not installed on enterprise %q; install it on the enterprise account "+ + "with the Enterprise people read and write permission", enterprise) } return nil, err } diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index e3a057e5..14389987 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -8,10 +8,11 @@ import ( "net/url" "testing" - resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/google/go-github/v69/github" "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/conductorone/baton-github/pkg/customclient" ) @@ -92,12 +93,12 @@ func TestGetEnterpriseInstallationUsesTheInstanceBaseURL(t *testing.T) { require.Equal(t, int64(33), installation.ID) } -// GitHub answers 404 when the app is not installed on the enterprise. That -// enterprise is skipped instead of erroring: an error out of the build reaches -// List, and the SDK aborts the entire sync on anything but NotFound, so a -// configuration that synced users and orgs fine would stop syncing at all. -// Skipping leaves no client, which is what makes Grant and Revoke report it. -func TestNewEnterpriseRoleClientsSkipsAnEnterpriseWithoutAnInstall(t *testing.T) { +// GitHub answers 404 when the app is not installed on the enterprise. Failing +// is what protects the data: C1 deletes every resource of a type that a +// completed sync did not report, so letting the sync finish while reading no +// owners would drop the Owner role and every grant on it. An error keeps the +// sync from completing at all. +func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { t.Parallel() ctx := context.Background() @@ -108,7 +109,7 @@ func TestNewEnterpriseRoleClientsSkipsAnEnterpriseWithoutAnInstall(t *testing.T) require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"})) })) - clients, err := newEnterpriseRoleClients( + _, err := newEnterpriseRoleClients( ctx, ctx, "https://github.com", @@ -118,15 +119,16 @@ func TestNewEnterpriseRoleClientsSkipsAnEnterpriseWithoutAnInstall(t *testing.T) nil, "example-org", ) - require.NoError(t, err) - require.Empty(t, clients) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.Contains(t, err.Error(), `not installed on enterprise "example-enterprise"`) + require.NotContains(t, err.Error(), "personal access token") } // The owners are read through one organization, which belongs to one -// enterprise, so app auth cannot serve a list. Standing down is what keeps it -// to this resource type: an error would reach List, where the SDK aborts a -// sync that delivers users, orgs, teams and repositories today. -func TestNewEnterpriseRoleClientsStandsDownForSeveralEnterprises(t *testing.T) { +// enterprise, so app auth cannot serve a list and picking one would be a +// guess. This fails for the same reason as a missing installation: a sync that +// completes without owners costs the operator every owner grant. +func TestNewEnterpriseRoleClientsRejectsSeveralEnterprises(t *testing.T) { t.Parallel() ctx := context.Background() @@ -134,7 +136,7 @@ func TestNewEnterpriseRoleClientsStandsDownForSeveralEnterprises(t *testing.T) { t.Errorf("no request expected, got %s", r.URL.Path) })) - clients, err := newEnterpriseRoleClients( + _, err := newEnterpriseRoleClients( ctx, ctx, "https://github.com", @@ -144,31 +146,6 @@ func TestNewEnterpriseRoleClientsStandsDownForSeveralEnterprises(t *testing.T) { nil, "example-org", ) - require.NoError(t, err) - require.Empty(t, clients) -} - -// An error out of List aborts the whole sync: the SDK downgrades only -// NotFound, and anything else cancels the batch. So an app that is not -// installed on the enterprise has to leave List reporting what it did before -// enterprise installations were read at all, which under app auth is nothing: -// the consumed-licenses API it falls back to is PAT-only and answers 403. -func TestEnterpriseRoleListSurvivesWithoutEnterpriseClients(t *testing.T) { - t.Parallel() - - ctx := context.Background() - apiClient := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusForbidden) - require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"message": "Resource not accessible by integration"})) - })) - - builder := EnterpriseRoleBuilder(apiClient, apiClient, customclient.New(apiClient), []string{"example-enterprise"}, - func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { - return map[string]*githubEnterpriseAdministratorClient{}, nil - }, - ) - - resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) - require.NoError(t, err) - require.Empty(t, resources) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.Contains(t, err.Error(), "one enterprise at a time") } diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 7861d628..92377d16 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -52,7 +52,8 @@ type enterpriseRoleResourceType struct { newEnterpriseClients enterpriseClientProvider // enterpriseClients is keyed by enterprise slug and memoized after the // first successful build; enterpriseClientsErr is why it could not be - // built, which fails this resource type only so the rest still syncs. + // built, and is returned rather than swallowed so the sync fails instead + // of completing without owners. enterpriseClients map[string]*githubEnterpriseAdministratorClient enterpriseClientsErr error enterpriseClientsSet bool @@ -65,10 +66,10 @@ func (o *enterpriseRoleResourceType) ResourceType(_ context.Context) *v2.Resourc // clients returns the per-enterprise administration clients, building them on // first use and memoizing the outcome. // -// Only a complete build, or a misconfiguration that will fail the same way -// every time, is memoized. Anything else is built again on the next call: -// discovering the installations is four network calls, and freezing a startup -// blip would disable this resource type for the lifetime of the process. +// Only a misconfiguration is memoized, because it will fail the same way every +// time. Anything else is built again on the next call: discovering the +// installations is four network calls, and freezing a startup blip would +// disable this resource type for the lifetime of the process. func (o *enterpriseRoleResourceType) clients( ctx context.Context, ) (map[string]*githubEnterpriseAdministratorClient, error) { @@ -79,7 +80,7 @@ func (o *enterpriseRoleResourceType) clients( o.mu.Lock() defer o.mu.Unlock() - if o.enterpriseClientsSet && (o.clientsAreComplete() || isPermanentError(o.enterpriseClientsErr)) { + if o.enterpriseClientsSet && (o.enterpriseClientsErr == nil || isPermanentError(o.enterpriseClientsErr)) { return o.enterpriseClients, o.enterpriseClientsErr } @@ -89,12 +90,16 @@ func (o *enterpriseRoleResourceType) clients( return o.enterpriseClients, o.enterpriseClientsErr } -// clientsAreComplete reports whether every configured enterprise resolved to a -// client. A build that skipped one succeeded without an error, so remembering -// it would keep an operator who installs the app on that enterprise reading -// the old answer until the process restarts. -func (o *enterpriseRoleResourceType) clientsAreComplete() bool { - return o.enterpriseClientsErr == nil && len(o.enterpriseClients) == len(o.enterprises) +// noClientReason explains why no administration client exists, in the terms of +// the fix the operator has to make. Under app auth every reason already +// surfaced as a build error, so what is left is the credential and an +// enterprise this connector was not configured for. +func (o *enterpriseRoleResourceType) noClientReason() string { + if o.newEnterpriseClients == nil { + return "a personal access token can sync enterprise roles but cannot provision them, " + + "which needs GitHub App authentication" + } + return "it is not one of the configured enterprises" } func (o *enterpriseRoleResourceType) cacheRole(roleId string, userLogin string) { @@ -606,16 +611,8 @@ func (o *enterpriseRoleResourceType) provisioningTarget( } client, ok := enterpriseClients[enterprise] if !ok { - // Naming the credential matters: under a PAT there is no App to - // install, so the other message sends the operator after a fix that - // cannot apply. - if o.newEnterpriseClients == nil { - return "", nil, status.Errorf(codes.FailedPrecondition, - "baton-github: provisioning enterprise %s requires GitHub App authentication; "+ - "a personal access token can sync enterprise roles but cannot provision them", enterprise) - } return "", nil, status.Errorf(codes.FailedPrecondition, - "baton-github: provisioning enterprise %s requires a GitHub App installed on the enterprise account", enterprise) + "baton-github: cannot provision enterprise %s: %s", enterprise, o.noClientReason()) } return enterprise, client, nil } diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index 1cdb8106..e7ddfaac 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -20,7 +20,6 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "github.com/conductorone/baton-github/pkg/customclient" "github.com/conductorone/baton-github/test/mocks" ) @@ -846,9 +845,10 @@ func TestEnterpriseRoleVerifyOrganization(t *testing.T) { require.ErrorContains(t, err, "does not belong to enterprise") } -// A missing enterprise installation must fail this resource type and nothing -// else, so the rest of the connector still syncs. Returning no resources would -// read to C1 as a revoke of every owner assignment. +// A missing enterprise installation must reach the syncer as an error. The +// SDK fails the run on it, which is the point: C1 deletes every resource of a +// type that a completed sync did not report, so finishing the sync while +// reading no owners would drop the Owner role and every grant on it. func TestEnterpriseRoleFailsClosedWithoutEnterpriseClients(t *testing.T) { t.Parallel() ctx := context.Background() @@ -928,46 +928,6 @@ func TestEnterpriseRoleRetriesAnUnclassifiedClientFailure(t *testing.T) { } } -// An enterprise the app is not installed on is skipped, so the build succeeds -// while resolving nothing. Remembering that would leave an operator who reads -// the error from Grant, installs the app and retries still reading the old -// answer until the process restarts. -func TestEnterpriseRoleReprobesAnIncompleteClientBuild(t *testing.T) { - t.Parallel() - ctx := context.Background() - - // Without a client the list falls back to consumed-licenses, which is - // PAT-only and answers 403 to an app. - apiClient := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusForbidden) - })) - - builds := 0 - builder := EnterpriseRoleBuilder(apiClient, apiClient, customclient.New(apiClient), []string{testEnterprise}, - func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { - builds++ - if builds == 1 { - // The app is not installed yet: nothing resolved, no error. - return map[string]*githubEnterpriseAdministratorClient{}, nil - } - return map[string]*githubEnterpriseAdministratorClient{testEnterprise: nil}, nil - }, - ) - - _, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) - require.NoError(t, err) - - resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) - require.NoError(t, err) - require.Len(t, resources, 1) - require.Equal(t, 2, builds) - - // Complete now, so it is remembered. - _, _, err = builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) - require.NoError(t, err) - require.Equal(t, 2, builds) -} - // A transient failure must not be remembered: freezing a blip at startup would // disable the resource type until the process restarts. func TestEnterpriseRoleRetriesARetryableClientFailure(t *testing.T) { @@ -1027,13 +987,13 @@ func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { patBuilder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, nil) _, _, err := patBuilder.Grant(ctx, principal, ent) require.Equal(t, codes.FailedPrecondition, status.Code(err)) - require.Contains(t, err.Error(), "requires GitHub App authentication") + require.Contains(t, err.Error(), "needs GitHub App authentication") }) - // With app auth the enterprise is skipped during sync when the app is not - // installed on it, so the client is missing here for a reason the operator - // can act on. - t.Run("names the missing installation under app auth", func(t *testing.T) { + // Under app auth every other reason already failed the client build, so + // what reaches here is an entitlement naming an enterprise this connector + // was never configured for. + t.Run("names an enterprise that is not configured", func(t *testing.T) { t.Parallel() appBuilder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { @@ -1042,7 +1002,7 @@ func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { ) _, _, err := appBuilder.Grant(ctx, principal, ent) require.Equal(t, codes.FailedPrecondition, status.Code(err)) - require.Contains(t, err.Error(), "installed on the enterprise account") + require.Contains(t, err.Error(), "not one of the configured enterprises") }) } diff --git a/pkg/connector/graphql_transport_test.go b/pkg/connector/graphql_transport_test.go index a971ad5f..5dcfdb04 100644 --- a/pkg/connector/graphql_transport_test.go +++ b/pkg/connector/graphql_transport_test.go @@ -89,7 +89,7 @@ func TestGraphQLErrorsCode(t *testing.T) { // cannot mask it. UNPROCESSABLE is what the invite-to-promote fallback // in Grant branches on, and a trailing FORBIDDEN used to overwrite it. {name: "first classified wins", types: []string{"UNPROCESSABLE", "FORBIDDEN"}, want: codes.FailedPrecondition}, - {name: "order independent", types: []string{"FORBIDDEN", "UNPROCESSABLE"}, want: codes.PermissionDenied}, + {name: "first classified entry wins", types: []string{"FORBIDDEN", "UNPROCESSABLE"}, want: codes.PermissionDenied}, // NOT_FOUND is the one code a credential error may override. Callers // read it as "already gone" and report success, so a batch whose // missing invitations hide a FORBIDDEN must not look benign. From fcd304aab57cad8c8346a8ea79d64a0833c6eb0a Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 11:19:47 -0300 Subject: [PATCH 12/22] feat(github): put enterprise owner provisioning behind an opt-in flag Co-authored-by: Cursor --- config_schema.json | 7 +++++ docs/docs-info.md | 10 +++++-- pkg/config/conf.gen.go | 1 + pkg/config/config.go | 20 +++++++++++-- pkg/connector/connector.go | 16 +++++++++-- .../enterprise_installations_test.go | 28 +++++++++++++++++++ 6 files changed, 75 insertions(+), 7 deletions(-) diff --git a/config_schema.json b/config_schema.json index 6b0dce9b..e7f3f901 100644 --- a/config_schema.json +++ b/config_schema.json @@ -169,6 +169,12 @@ "description": "Whether to sync secrets or not", "boolField": {} }, + { + "name": "enable-enterprise-owner-provisioning", + "displayName": "Enable enterprise owner provisioning", + "description": "Sync and provision the built-in Enterprise Owner role. Requires the GitHub App to be installed on the enterprise account as well as on the organization, with the \"Enterprise people: read and write\" permission, and requires --enterprises to name that enterprise. Not available with a personal access token.", + "boolField": {} + }, { "name": "omit-archived-repositories", "displayName": "Omit syncing archived repositories", @@ -208,6 +214,7 @@ "app-privatekey", "org", "sync-secrets", + "enable-enterprise-owner-provisioning", "omit-archived-repositories", "direct-collaborators-only" ] diff --git a/docs/docs-info.md b/docs/docs-info.md index 608be3f9..886f5f3d 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -238,13 +238,19 @@ Consequences worth knowing: - Reading owners uses the **organization** installation token and every mutation uses the **enterprise** installation token; the enterprise token is rejected on organization fields. Startup verifies that the configured organization belongs to the configured enterprise, and a failure there fails the sync - Only one enterprise can be served under App authentication, because the owners are read through the single configured organization and an organization belongs to exactly one enterprise. A configuration naming several is rejected while the clients are built, with an explanatory error rather than a later failure on a check the operator cannot satisfy. The PAT path does accept a list +### Enterprise owner provisioning is opt-in + +`--enable-enterprise-owner-provisioning` is off by default, and while it is off the connector behaves exactly as it did before this capability existed: enterprise roles come from the consumed-licenses cache, which is PAT-only, so App deployments report none and nothing fails. + +The flag exists because turning the capability on requires setup nobody has done yet — a second installation of the App, on the enterprise account. Without the flag that requirement would reach every deployment that already passes `--enterprises` under App authentication, and the failure below would turn their working sync into a failing one on upgrade. With it, the failure is only reachable by an operator who asked for the capability. + ### A missing enterprise installation fails the sync rather than emitting nothing -Under App authentication the connector refuses to sync when it cannot build an enterprise administration client: the app is not installed on the enterprise account, several enterprises are configured, or the organization does not belong to the configured one. The whole sync fails, not just this resource type. +Once the capability is enabled, the connector refuses to sync when it cannot build an enterprise administration client: the app is not installed on the enterprise account, several enterprises are configured, or the organization does not belong to the configured one. The whole sync fails, not just this resource type. That is deliberate, and the alternative is worse. C1 deletes every resource of a type that a completed sync did not report, and it applies that to a resource type whose list came back empty for any reason. Letting the sync finish while reading no owners would therefore delete the Owner role and every grant on it, silently, and GitHub answers `404` for an uninstalled app, a revoked permission and a slug typo alike — the connector cannot tell a genuine uninstall from a blip. An error keeps the sync from completing, so nothing is deleted. -The cost falls on a deployment that sets `--enterprises` under App authentication without installing the app on the enterprise account. That combination produced no enterprise data before this capability existed, and now stops the sync until the app is installed or the flag is removed. +The cost falls on a deployment that enabled the capability without installing the app on the enterprise account, and the sync stops until the app is installed or the flag is turned back off. ### Pending invitations look the same as real access diff --git a/pkg/config/conf.gen.go b/pkg/config/conf.gen.go index 6541f0f2..0c4cf034 100644 --- a/pkg/config/conf.gen.go +++ b/pkg/config/conf.gen.go @@ -13,6 +13,7 @@ type Github struct { AppPrivatekey string `mapstructure:"app-privatekey"` Org string `mapstructure:"org"` SyncSecrets bool `mapstructure:"sync-secrets"` + EnableEnterpriseOwnerProvisioning bool `mapstructure:"enable-enterprise-owner-provisioning"` OmitArchivedRepositories bool `mapstructure:"omit-archived-repositories"` DirectCollaboratorsOnly bool `mapstructure:"direct-collaborators-only"` SyncLastActivity bool `mapstructure:"sync-last-activity"` diff --git a/pkg/config/config.go b/pkg/config/config.go index 60856c56..498735fe 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -69,6 +69,18 @@ var ( field.WithDisplayName("Sync secrets"), field.WithDescription(`Whether to sync secrets or not`), ) + // Off by default because it needs setup nobody has done yet: turning it on + // without the enterprise installation fails the sync, which is only a fair + // trade for someone who asked for the capability. + enableEnterpriseOwnerProvisioning = field.BoolField( + "enable-enterprise-owner-provisioning", + field.WithDisplayName("Enable enterprise owner provisioning"), + field.WithDescription( + "Sync and provision the built-in Enterprise Owner role. Requires the GitHub App to be installed on "+ + "the enterprise account as well as on the organization, with the \"Enterprise people: read and write\" "+ + "permission, and requires --enterprises to name that enterprise. Not available with a personal access token.", + ), + ) omitArchivedRepositories = field.BoolField( "omit-archived-repositories", field.WithDisplayName("Omit syncing archived repositories"), @@ -120,6 +132,7 @@ var Config = field.NewConfiguration( appPrivateKey, orgField, syncSecrets, + enableEnterpriseOwnerProvisioning, omitArchivedRepositories, directCollaboratorsOnly, syncLastActivity, @@ -139,8 +152,11 @@ var Config = field.NewConfiguration( Name: GithubAppGroup, DisplayName: "GitHub app", HelpText: "Use a github app for authentication", - Fields: []field.SchemaField{appIDField, appPrivateKeyPath, appPrivateKey, orgField, syncSecrets, omitArchivedRepositories, directCollaboratorsOnly}, - Default: false, + Fields: []field.SchemaField{ + appIDField, appPrivateKeyPath, appPrivateKey, orgField, syncSecrets, + enableEnterpriseOwnerProvisioning, omitArchivedRepositories, directCollaboratorsOnly, + }, + Default: false, }, }), ) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index bc35d17c..2a710cd7 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -473,9 +473,19 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { // memoized for the process lifetime, so the token refresher inside them // must not hold the context of whichever RPC happened to build them. connectorCtx := ctx - newEnterpriseRoleClientsFn := func(ctx context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { - return newEnterpriseRoleClients( - ctx, connectorCtx, ghc.InstanceUrl, appClient, jwtts, ghc.Enterprises, appHTTPClient, ghc.Org) + // Left nil unless the operator opted in. Reading enterprise owners needs + // the app installed on the enterprise account too, which no existing + // deployment has done, and the connector fails the sync when it cannot + // read them. Nil keeps that path inert: enterprise roles then come from + // the consumed-licenses cache exactly as they did before this capability, + // which under app auth means nothing, so an upgrade changes no behaviour + // until it is asked for. + var newEnterpriseRoleClientsFn enterpriseClientProvider + if ghc.EnableEnterpriseOwnerProvisioning { + newEnterpriseRoleClientsFn = func(ctx context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + return newEnterpriseRoleClients( + ctx, connectorCtx, ghc.InstanceUrl, appClient, jwtts, ghc.Enterprises, appHTTPClient, ghc.Org) + } } gh := &GitHub{ diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index 14389987..2972329e 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -15,6 +15,7 @@ import ( "google.golang.org/grpc/status" "github.com/conductorone/baton-github/pkg/customclient" + resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" ) // newGitHubAPITestClient points the client at a test server through BaseURL @@ -93,6 +94,33 @@ func TestGetEnterpriseInstallationUsesTheInstanceBaseURL(t *testing.T) { require.Equal(t, int64(33), installation.ID) } +// Without the opt-in the enterprise owner path is left unwired, and this is +// what that has to mean: the same answer the connector gave before the +// capability existed. Reading owners needs the app installed on the enterprise +// account, which no existing deployment has done, and the connector fails the +// sync when it cannot read them — so if the unwired path did anything else, +// upgrading would turn a working sync into a failing one for everyone already +// passing --enterprises under app auth. Under app auth that answer is no +// resources, because the consumed-licenses API it falls back to is PAT-only +// and answers 403. +func TestEnterpriseRoleListIsInertWithoutTheOptIn(t *testing.T) { + t.Parallel() + + ctx := context.Background() + apiClient := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"message": "Resource not accessible by integration"})) + })) + + // nil provider is how newWithGithubApp leaves it when the flag is off. + builder := EnterpriseRoleBuilder(apiClient, apiClient, customclient.New(apiClient), + []string{"example-enterprise"}, nil) + + resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Empty(t, resources) +} + // GitHub answers 404 when the app is not installed on the enterprise. Failing // is what protects the data: C1 deletes every resource of a type that a // completed sync did not report, so letting the sync finish while reading no From 0c939212d018e6c639b3748b528b7ef8b3b1e4f1 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 11:25:29 -0300 Subject: [PATCH 13/22] refactor(github): memoize only a successful client build so a fixed config recovers Co-authored-by: Cursor --- pkg/connector/enterprise_role.go | 36 ++++++++------- pkg/connector/enterprise_role_test.go | 63 +++++++++------------------ pkg/connector/helpers.go | 22 ---------- 3 files changed, 40 insertions(+), 81 deletions(-) diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 92377d16..96f9f3a7 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -51,12 +51,10 @@ type enterpriseRoleResourceType struct { // It is nil under PAT auth. newEnterpriseClients enterpriseClientProvider // enterpriseClients is keyed by enterprise slug and memoized after the - // first successful build; enterpriseClientsErr is why it could not be - // built, and is returned rather than swallowed so the sync fails instead - // of completing without owners. - enterpriseClients map[string]*githubEnterpriseAdministratorClient - enterpriseClientsErr error - enterpriseClientsSet bool + // first successful build. A build error is returned rather than stored, + // so the sync fails instead of completing without owners and the next + // call tries again. + enterpriseClients map[string]*githubEnterpriseAdministratorClient } func (o *enterpriseRoleResourceType) ResourceType(_ context.Context) *v2.ResourceType { @@ -64,12 +62,15 @@ func (o *enterpriseRoleResourceType) ResourceType(_ context.Context) *v2.Resourc } // clients returns the per-enterprise administration clients, building them on -// first use and memoizing the outcome. +// first use and memoizing them once they are built. // -// Only a misconfiguration is memoized, because it will fail the same way every -// time. Anything else is built again on the next call: discovering the -// installations is four network calls, and freezing a startup blip would -// disable this resource type for the lifetime of the process. +// Only success is remembered. A failure is retried on the next call, which +// costs one discovery attempt on a path that is already failing the sync, and +// buys two things: a startup blip does not disable this resource type for the +// lifetime of the process, and an operator who installs the app or restores +// the permission is picked up by the next sync instead of needing a restart. +// GitHub answers 404 for an uninstalled app, a revoked permission and a typo +// alike, so no error here can be trusted to be permanent. func (o *enterpriseRoleResourceType) clients( ctx context.Context, ) (map[string]*githubEnterpriseAdministratorClient, error) { @@ -80,14 +81,17 @@ func (o *enterpriseRoleResourceType) clients( o.mu.Lock() defer o.mu.Unlock() - if o.enterpriseClientsSet && (o.enterpriseClientsErr == nil || isPermanentError(o.enterpriseClientsErr)) { - return o.enterpriseClients, o.enterpriseClientsErr + if o.enterpriseClients != nil { + return o.enterpriseClients, nil } - o.enterpriseClients, o.enterpriseClientsErr = o.newEnterpriseClients(ctx) - o.enterpriseClientsSet = true + clients, err := o.newEnterpriseClients(ctx) + if err != nil { + return nil, err + } + o.enterpriseClients = clients - return o.enterpriseClients, o.enterpriseClientsErr + return o.enterpriseClients, nil } // noClientReason explains why no administration client exists, in the terms of diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index e7ddfaac..b404c633 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -853,8 +853,6 @@ func TestEnterpriseRoleFailsClosedWithoutEnterpriseClients(t *testing.T) { t.Parallel() ctx := context.Background() - // The code that produces this answer marks it FailedPrecondition, which is - // what makes it safe to remember. clientsErr := status.Error(codes.FailedPrecondition, "github-connector: GitHub App is not installed on enterprise") builds := 0 @@ -879,16 +877,19 @@ func TestEnterpriseRoleFailsClosedWithoutEnterpriseClients(t *testing.T) { _, _, err = builder.Grants(ctx, roleResource, resourceSdk.SyncOpAttrs{}) require.ErrorIs(t, err, clientsErr) - // A misconfiguration answers the same way every time, so it is built once - // and remembered rather than re-probed on each call. - require.Equal(t, 1, builds) + // Nothing is remembered from a failure: GitHub answers 404 for an + // uninstalled app, a revoked permission and a typo alike, so an operator + // who fixes it is picked up by the next call rather than by a restart. + require.Equal(t, 2, builds) } -// Building the clients calls go-github directly, whose errors arrive wrapped -// with %w and carry no gRPC status, so a 502 and a cancelled sync both read as -// Unknown. Remembering those would disable the resource type for the lifetime -// of the process on a blip the operator cannot see or fix. -func TestEnterpriseRoleRetriesAnUnclassifiedClientFailure(t *testing.T) { +// No build failure is remembered. GitHub answers 404 for an uninstalled app, +// a revoked permission and a typo alike, and errors from go-github or a +// cancelled context arrive wrapped with %w carrying no gRPC status at all, so +// nothing here can be classified as permanent. Remembering any of them would +// disable the resource type until the process restarts, and would leave an +// operator who fixed the configuration still reading the stale answer. +func TestEnterpriseRoleRetriesAClientBuildFailure(t *testing.T) { t.Parallel() ctx := context.Background() @@ -896,6 +897,8 @@ func TestEnterpriseRoleRetriesAnUnclassifiedClientFailure(t *testing.T) { name string err error }{ + {"a misconfiguration", status.Error(codes.FailedPrecondition, "not installed on the enterprise")}, + {"a rate limit", status.Error(codes.Unavailable, "rate limited")}, {"a go-github failure", fmt.Errorf("github-connector: failed to create installation token: %w", &github.ErrorResponse{ Response: &http.Response{StatusCode: http.StatusBadGateway, Request: &http.Request{}}, @@ -913,6 +916,8 @@ func TestEnterpriseRoleRetriesAnUnclassifiedClientFailure(t *testing.T) { if builds == 1 { return nil, tc.err } + // List only checks the enterprise is present, so the + // client itself is never dereferenced here. return map[string]*githubEnterpriseAdministratorClient{testEnterprise: nil}, nil }, ) @@ -924,43 +929,15 @@ func TestEnterpriseRoleRetriesAnUnclassifiedClientFailure(t *testing.T) { require.NoError(t, err) require.Len(t, resources, 1) require.Equal(t, 2, builds) + + // Once it succeeds the result is memoized. + _, _, err = builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Equal(t, 2, builds) }) } } -// A transient failure must not be remembered: freezing a blip at startup would -// disable the resource type until the process restarts. -func TestEnterpriseRoleRetriesARetryableClientFailure(t *testing.T) { - t.Parallel() - ctx := context.Background() - - builds := 0 - builder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, - func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { - builds++ - if builds == 1 { - return nil, status.Error(codes.Unavailable, "rate limited") - } - // List only checks the enterprise is present, so the client - // itself is never dereferenced here. - return map[string]*githubEnterpriseAdministratorClient{testEnterprise: nil}, nil - }, - ) - - _, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) - require.Equal(t, codes.Unavailable, status.Code(err)) - - resources, _, err := builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) - require.NoError(t, err) - require.Len(t, resources, 1) - require.Equal(t, 2, builds) - - // Once it succeeds the result is memoized. - _, _, err = builder.List(ctx, nil, resourceSdk.SyncOpAttrs{}) - require.NoError(t, err) - require.Equal(t, 2, builds) -} - // Provisioning is only possible through an enterprise installation, so the PAT // path must reject it rather than attempt a mutation it cannot make. func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 830e22a3..5d34d3a4 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -324,28 +324,6 @@ func isAuthError(resp *github.Response) bool { return resp.StatusCode == http.StatusUnauthorized } -// isPermanentError reports whether an error will keep failing until an -// operator changes something, which is what makes it safe to remember. -// -// The test is which codes are permanent, not which are transient, because an -// unclassified error has to be retried: a go-github failure or a cancelled -// context reaches here wrapped with %w and carries no gRPC status, so -// status.Code reports Unknown for a 502, a rate limit and a cancelled sync -// alike. Treating Unknown as permanent would remember a blip forever. -func isPermanentError(err error) bool { - if err == nil { - return false - } - - switch status.Code(err) { - case codes.PermissionDenied, codes.Unauthenticated, codes.NotFound, - codes.InvalidArgument, codes.FailedPrecondition: - return true - default: - return false - } -} - // freshestRateLimit returns current with the rate limit of latest applied, // keeping the earlier budget when latest reports none. // From 764bdece9addfb56b3ef85fb65c69d76e64562a8 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 11:43:22 -0300 Subject: [PATCH 14/22] docs(github): document the enterprise owner provisioning flag where the docs describe the capability Co-authored-by: Cursor --- README.md | 3 ++- docs/docs-info.md | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index be6a6768..3ad9facd 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 and licenses, only when `--enterprises` is set +- Enterprise roles and licenses, only when `--enterprises` is set. Provisioning the built-in Enterprise Owner role additionally requires `--enable-enterprise-owner-provisioning` and a GitHub App installed on the enterprise account 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. @@ -81,6 +81,7 @@ Flags: --app-privatekey-path string Path to private key that is used to connect to the GitHub App. Ignored when app-privatekey is set. ($BATON_APP_PRIVATEKEY_PATH) --client-id string The client ID used to authenticate with ConductorOne ($BATON_CLIENT_ID) --client-secret string The client secret used to authenticate with ConductorOne ($BATON_CLIENT_SECRET) + --enable-enterprise-owner-provisioning Sync and provision the built-in Enterprise Owner role. Requires the GitHub App to be installed on the enterprise account as well as on the organization, with the "Enterprise people: read and write" permission, and requires --enterprises to name that enterprise. Not available with a personal access token. ($BATON_ENABLE_ENTERPRISE_OWNER_PROVISIONING) --enterprises strings Sync enterprise roles, must be an admin of the enterprise. ($BATON_ENTERPRISES) --external-resource-c1z string The path to the c1z file to sync external baton resources with ($BATON_EXTERNAL_RESOURCE_C1Z) --external-resource-entitlement-id-filter string The entitlement that external users, groups must have access to sync external baton resources ($BATON_EXTERNAL_RESOURCE_ENTITLEMENT_ID_FILTER) diff --git a/docs/docs-info.md b/docs/docs-info.md index 886f5f3d..dd488158 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -20,7 +20,7 @@ - Teams (including nested teams, with their parent team as the parent resource) - Repositories (optionally excluding archived ones) - Organization roles (GitHub's built-in and custom org roles, also called "enterprise licenses" in GitHub's docs) - - Enterprise roles (only when `--enterprises` is set) + - Enterprise roles (only when `--enterprises` is set; under App authentication the built-in Owner role also needs `--enable-enterprise-owner-provisioning`) - Licenses (enterprise seat consumption, only when `--enterprises` is set) - GitHub Apps installed on the organization - API keys (fine-grained personal access tokens with access to the org, only when `--sync-secrets` is set) @@ -70,6 +70,7 @@ Common to both: `--enterprises` — enterprises to sync enterprise roles and licenses for + `--enable-enterprise-owner-provisioning` — sync and provision the built-in Enterprise Owner role. Off by default, App authentication only, and requires the App installed on the enterprise account as well as the organization `--sync-secrets` — sync fine-grained personal access tokens as API keys `--sync-last-activity` — emit the audit-log usage event feed. Hidden from `--help` and from the GUI config here, because it only applies to GitHub Enterprise audit-log access; `baton-github-enterprise` sets it directly instead of going through this CLI layer `--omit-archived-repositories` — skip archived repositories @@ -185,11 +186,11 @@ ### Enterprise roles - **Resource type ID**: `enterprise_role` -- **Description**: Roles of an enterprise account. Only synced when `--enterprises` is set +- **Description**: Roles of an enterprise account. Only synced when `--enterprises` is set. Under App authentication the built-in Owner role is only synced when `--enable-enterprise-owner-provisioning` is set as well - **Traits**: Role trait - **Entitlements**: `assigned` (assignment) - **Grants**: Under PAT authentication, one grant per user holding each role, read from the enterprise consumed-licenses API. Under GitHub App authentication, only the built-in **Owner** role is visible, and its grants are the users who hold it plus the users who have been invited and have not accepted. The two are emitted against the same entitlement and C1 cannot tell them apart -- **Provisioning**: Only the built-in **Owner** role, and only with GitHub App authentication. See [Enterprise Owner provisioning](#enterprise-owner-provisioning) +- **Provisioning**: Only the built-in **Owner** role, only with GitHub App authentication, and only when `--enable-enterprise-owner-provisioning` is set. See [Enterprise Owner provisioning](#enterprise-owner-provisioning) - **Limitation**: A GitHub App cannot read `Enterprise.ownerInfo`, so under App authentication the connector sees only the Owner role, not billing managers or custom enterprise roles ### Licenses From a5b130412604977966571d31786ec72bbc4ba224 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 11:50:28 -0300 Subject: [PATCH 15/22] fix(github): give the page-limit errors a grpc code like the rest of the path Co-authored-by: Cursor --- pkg/connector/enterprise_administrator_client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/connector/enterprise_administrator_client.go b/pkg/connector/enterprise_administrator_client.go index 7a0a5fc5..128aa170 100644 --- a/pkg/connector/enterprise_administrator_client.go +++ b/pkg/connector/enterprise_administrator_client.go @@ -324,7 +324,7 @@ func (c *githubEnterpriseAdministratorClient) verifyOrganization(ctx context.Con // not finding the organization: reporting the membership error there would // send the operator to fix a membership that is already correct. if !walked { - return fmt.Errorf( + return status.Errorf(codes.Internal, "baton-github: gave up looking for organization %s in enterprise %s after %d pages", c.org, enterprise, enterpriseMaxPages) } @@ -618,7 +618,7 @@ func (c *githubEnterpriseAdministratorClient) OwnerState( // walk never finished reading, which Revoke would answer with // GrantAlreadyRevoked while they still hold the role. if !walked { - return state, annos, fmt.Errorf( + return state, annos, status.Errorf(codes.Internal, "baton-github: gave up reading the owners of enterprise %s after %d pages", enterprise, enterpriseMaxPages) } From 4e7fa1d0881d75775441c565fb186e92e9cffbb6 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 15:23:45 -0300 Subject: [PATCH 16/22] fix(github): advertise enterprise role provisioning only where it can be used Co-authored-by: Cursor --- docs/docs-info.md | 1 + pkg/connector/connector.go | 18 +++++++++---- pkg/connector/enterprise_role.go | 37 +++++++++++++++++++++++---- pkg/connector/enterprise_role_test.go | 10 ++++---- 4 files changed, 51 insertions(+), 15 deletions(-) diff --git a/docs/docs-info.md b/docs/docs-info.md index dd488158..dde1a0dc 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -259,6 +259,7 @@ C1 has no pending state for a grant, so an invitation nobody has accepted and an - An access review or an offboarding sweep counts an invitee as holding Owner. They do not hold it — GitHub assigns the role only on acceptance - Nothing tracks an expiry. GitHub stops resolving an invitation once it is accepted, cancelled, or expired, so it simply stops being emitted and C1 drops the grant on that sync. `EnterpriseAdministratorInvitation` exposes no `expiresAt`, so there is nothing to compute from either +- **Time-bound access is measured from the invitation, not from acceptance.** C1 starts the clock when Grant reports success, which is when the invitation is sent. Someone who accepts three hours into a four-hour window holds the role for one hour, and the record still reads four. If the window closes first, Revoke cancels the unaccepted invitation — the right action, since the request expired, but C1 logs a completed Owner grant for someone who never held the role. Prefer windows comfortably longer than the invitee takes to accept, or grant to people who already administer the enterprise, who are promoted immediately ### The list of pending invitations cannot be complete diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 2a710cd7..b74decec 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -155,13 +155,21 @@ func (gh *GitHub) ResourceSyncers(ctx context.Context) []connectorbuilder.Resour } if len(gh.enterprises) > 0 { - resourceSyncers = append(resourceSyncers, - EnterpriseRoleBuilder( + // The provisioning-capable syncer is registered only where the role can + // actually be provisioned. The SDK reads CAPABILITY_PROVISION off the + // methods a syncer implements, so registering it everywhere would offer + // the role as requestable to PAT deployments, where every request fails. + if gh.newEnterpriseRoleClients != nil { + resourceSyncers = append(resourceSyncers, EnterpriseRoleProvisioningBuilder( gh.client, gh.appClient, gh.customClient, gh.enterprises, gh.newEnterpriseRoleClients, - ), - LicenseBuilder(gh.customClient, gh.enterprises), - ) + )) + } else { + resourceSyncers = append(resourceSyncers, EnterpriseRoleBuilder( + gh.client, gh.appClient, gh.customClient, gh.enterprises, nil, + )) + } + resourceSyncers = append(resourceSyncers, LicenseBuilder(gh.customClient, gh.enterprises)) } return resourceSyncers } diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 96f9f3a7..e212eeef 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -268,8 +268,35 @@ func (o *enterpriseRoleResourceType) Grants( return ret, &resourceSdk.SyncOpResults{}, nil } -// EnterpriseRoleBuilder returns the enterprise role syncer. newEnterpriseClients -// is nil under PAT authentication, where only the read path is available. +// enterpriseRoleProvisioner adds Grant and Revoke to the read-only syncer. +// +// It is a separate type because the SDK derives CAPABILITY_PROVISION from the +// methods a syncer implements, with no way to vary it at runtime. Registering +// it only when the enterprise clients can be built keeps C1 from offering the +// role as requestable on a deployment that could never satisfy the request: +// under a PAT there is no App to install, and every request would fail. +type enterpriseRoleProvisioner struct { + *enterpriseRoleResourceType +} + +// EnterpriseRoleProvisioningBuilder returns the syncer with Grant and Revoke. +// Only for deployments whose enterprise administration clients can be built. +func EnterpriseRoleProvisioningBuilder( + client *github.Client, + appClient *github.Client, + customClient *customclient.Client, + enterprises []string, + newEnterpriseClients enterpriseClientProvider, +) *enterpriseRoleProvisioner { + return &enterpriseRoleProvisioner{ + enterpriseRoleResourceType: EnterpriseRoleBuilder( + client, appClient, customClient, enterprises, newEnterpriseClients), + } +} + +// EnterpriseRoleBuilder returns the read-only enterprise role syncer. +// newEnterpriseClients is nil under PAT authentication, where only the read +// path is available. func EnterpriseRoleBuilder( client *github.Client, appClient *github.Client, @@ -475,7 +502,7 @@ func (o *enterpriseRoleResourceType) pendingInvitationGrants( // An unaccepted invitation counts as held and returns a grant, the same way // Grants() emits it: returning nothing would make C1 drop an overlay that the // next sync puts straight back. -func (o *enterpriseRoleResourceType) Grant( +func (o *enterpriseRoleProvisioner) Grant( ctx context.Context, principal *v2.Resource, ent *v2.Entitlement, @@ -541,7 +568,7 @@ func (o *enterpriseRoleResourceType) Grant( // follows. Demotion uses UNAFFILIATED, which keeps the user as a member of the // enterprise instead of evicting them. A NOT_FOUND from either mutation is // success, because it means the state being asked for is already in place. -func (o *enterpriseRoleResourceType) Revoke( +func (o *enterpriseRoleProvisioner) Revoke( ctx context.Context, grantObj *v2.Grant, ) (annotations.Annotations, error) { @@ -591,7 +618,7 @@ func (o *enterpriseRoleResourceType) Revoke( return annos, nil } -func (o *enterpriseRoleResourceType) provisioningTarget( +func (o *enterpriseRoleProvisioner) provisioningTarget( ctx context.Context, principal *v2.Resource, ent *v2.Entitlement, diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index b404c633..4eaacc63 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -371,7 +371,7 @@ func requireNoIdempotencyClaim(t *testing.T, annos annotations.Annotations) { func newTestEnterpriseRoleBuilder( t *testing.T, stub *enterpriseStub, -) (*enterpriseRoleResourceType, *v2.Resource, *v2.Entitlement) { +) (*enterpriseRoleProvisioner, *v2.Resource, *v2.Entitlement) { t.Helper() if stub.invitations == nil { @@ -395,7 +395,7 @@ func newTestEnterpriseRoleBuilder( _, _, _, githubUser, _, err := mgh.Seed() require.NoError(t, err) - builder := EnterpriseRoleBuilder( + builder := EnterpriseRoleProvisioningBuilder( github.NewClient(mgh.Server()), nil, nil, @@ -679,7 +679,7 @@ func TestEnterpriseRoleRevoke(t *testing.T) { // until it empties, and returns every grant across both phases. func drainGrants( t *testing.T, - builder *enterpriseRoleResourceType, + builder *enterpriseRoleProvisioner, resource *v2.Resource, ) []*v2.Grant { t.Helper() @@ -961,7 +961,7 @@ func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { // after a fix that cannot apply. t.Run("names the credential when the token cannot provision", func(t *testing.T) { t.Parallel() - patBuilder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, nil) + patBuilder := EnterpriseRoleProvisioningBuilder(nil, nil, nil, []string{testEnterprise}, nil) _, _, err := patBuilder.Grant(ctx, principal, ent) require.Equal(t, codes.FailedPrecondition, status.Code(err)) require.Contains(t, err.Error(), "needs GitHub App authentication") @@ -972,7 +972,7 @@ func TestEnterpriseRoleProvisioningTargetGuards(t *testing.T) { // was never configured for. t.Run("names an enterprise that is not configured", func(t *testing.T) { t.Parallel() - appBuilder := EnterpriseRoleBuilder(nil, nil, nil, []string{testEnterprise}, + appBuilder := EnterpriseRoleProvisioningBuilder(nil, nil, nil, []string{testEnterprise}, func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { return map[string]*githubEnterpriseAdministratorClient{}, nil }, From ec0f9465297404e632ed4277f33ba0885c488d0e Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 15:30:25 -0300 Subject: [PATCH 17/22] fix(github): stop registering the pat-only license type under app auth Co-authored-by: Cursor --- pkg/connector/connector.go | 19 ++++++++++++++----- pkg/connector/enterprise_role.go | 12 ++++++++---- pkg/connector/enterprise_role_test.go | 21 +++++++++++++++------ 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index b74decec..2452655b 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -169,7 +169,14 @@ func (gh *GitHub) ResourceSyncers(ctx context.Context) []connectorbuilder.Resour gh.client, gh.appClient, gh.customClient, gh.enterprises, nil, )) } - resourceSyncers = append(resourceSyncers, LicenseBuilder(gh.customClient, gh.enterprises)) + // The consumed-licenses API behind this type is PAT-only and its 403 + // fails the whole sync, so under app auth it has never emitted a + // resource and can only break the run. Not registering it deletes + // nothing, and spares the operator having to disable the type in C1 + // to make the documented enterprise setup sync at all. + if gh.appClient == nil { + resourceSyncers = append(resourceSyncers, LicenseBuilder(gh.customClient, gh.enterprises)) + } } return resourceSyncers } @@ -472,10 +479,12 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { // installation token above carries no enterprise permissions. Reading the // owners needs the org token, so both are handed to the client. // - // Built on first use rather than here, so a failure fails the - // enterprise_role sync instead of the whole connector: the other resource - // types keep syncing and C1 holds its previous owner state rather than - // reading an empty list as a revoke of every owner. + // Built on first use rather than here, so connector construction and + // Validate do not depend on the enterprise installation and a later sync + // retries the build. It does not narrow the blast radius of a failure: + // the error surfaces from List, which fails the whole sync, and that is + // deliberate — a sync that completed without owners would read to C1 as a + // revoke of every owner assignment. // // The construction context is captured separately: the clients are // memoized for the process lifetime, so the token refresher inside them diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index e212eeef..e9ef3ec9 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -532,8 +532,6 @@ func (o *enterpriseRoleProvisioner) Grant( if status.Code(inviteErr) != codes.FailedPrecondition { return nil, annos, inviteErr } - // Only the promotion's error is returned, so GitHub's reason for - // refusing the invitation would otherwise be lost. ctxzap.Extract(ctx).Debug("baton-github: invitation rejected, promoting in place instead", zap.String("login", login), zap.Error(inviteErr), @@ -541,9 +539,15 @@ func (o *enterpriseRoleProvisioner) Grant( if promoteErr := client.UpdateRole( ctx, state.enterpriseID, login, githubv4.EnterpriseAdministratorRoleOwner, ); promoteErr != nil { - // The promotion's status code is what tells C1 whether to retry. + // UNPROCESSABLE covers more than "already an administrator" — + // seat limits and SSO or EMU restrictions land here too — and + // that reason is the actionable one when the promotion also + // fails. It is carried as text so the promotion keeps %w: its + // status code is what tells C1 whether to retry, and a second + // %w would put the invitation's code first instead. return nil, annos, fmt.Errorf( - "promoting %s after the invitation was rejected: %w", login, promoteErr) + "inviting %s was rejected (%s); promoting in place also failed: %w", + login, inviteErr.Error(), promoteErr) } } diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index 4eaacc63..ae22dfa1 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -483,16 +483,25 @@ func TestEnterpriseRoleGrant(t *testing.T) { // reaches C1: it decides whether the task is worth retrying. t.Run("surfaces the promotion status when both mutations fail", func(t *testing.T) { t.Parallel() - stub := &enterpriseStub{inviteErrorType: "UNPROCESSABLE", updateFails: true} + // A reason other than "already an administrator", which is the case + // that makes carrying it worthwhile. + stub := &enterpriseStub{ + inviteErrorType: "UNPROCESSABLE", + inviteErrorMessage: "enterprise owner seat limit reached", + updateFails: true, + } builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) _, _, err := builder.Grant(ctx, principal, ent) require.Equal(t, codes.Unavailable, status.Code(err)) - require.ErrorContains(t, err, "promoting") - // The invitation error is not interpolated into the message: it is - // always the expected FailedPrecondition, and both errors carry the - // connector prefix, so repeating it makes the message unreadable. - require.Equal(t, 1, strings.Count(err.Error(), "baton-github:")) + require.ErrorContains(t, err, "promoting in place also failed") + // GitHub's reason for refusing the invitation is carried too, because + // UNPROCESSABLE is not only "already an administrator" — a seat limit + // or an SSO restriction lands there as well, and then it is the + // actionable half. It is text, not a second %w: wrapping both would + // hand C1 the invitation's code instead of the promotion's. + require.ErrorContains(t, err, "was rejected") + require.ErrorContains(t, err, stub.inviteErrorMessage) }) t.Run("reports an owner as already granted", func(t *testing.T) { From adc027c7f6652f5e78b29876101b2b74b8f6273b Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 15:49:59 -0300 Subject: [PATCH 18/22] fix(github): keep the license type registered until enterprise owners are opted into Co-authored-by: Cursor --- README.md | 3 +- pkg/connector/connector.go | 36 +++++++++++++++---- .../enterprise_installations_test.go | 32 +++++++++++++++++ 3 files changed, 64 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 3ad9facd..1455d3d6 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,8 @@ 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 and licenses, only when `--enterprises` is set. Provisioning the built-in Enterprise Owner role additionally requires `--enable-enterprise-owner-provisioning` and a GitHub App installed on the enterprise account +- Enterprise roles, only when `--enterprises` is set. Provisioning the built-in Enterprise Owner role additionally requires `--enable-enterprise-owner-provisioning` and a GitHub App installed on the enterprise account +- Enterprise licenses, only when `--enterprises` is set and the connector uses a personal access token. The API behind them is not available to GitHub Apps 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/pkg/connector/connector.go b/pkg/connector/connector.go index 2452655b..170b1302 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -169,12 +169,17 @@ func (gh *GitHub) ResourceSyncers(ctx context.Context) []connectorbuilder.Resour gh.client, gh.appClient, gh.customClient, gh.enterprises, nil, )) } - // The consumed-licenses API behind this type is PAT-only and its 403 - // fails the whole sync, so under app auth it has never emitted a - // resource and can only break the run. Not registering it deletes - // nothing, and spares the operator having to disable the type in C1 - // to make the documented enterprise setup sync at all. - if gh.appClient == nil { + // The consumed-licenses API behind this type is PAT-only, so under app + // auth its 403 fails the whole sync and it has never emitted a + // resource. It is dropped only once the operator opts into enterprise + // owner provisioning, which is the setup whose sync it would break. + // + // Keyed on the provider rather than on the credential on purpose: + // with the opt-in off this type has to stay registered, because its + // failure is what stops the run. Dropping it there would let the sync + // complete while reporting no enterprise roles, and C1 deletes the + // stored resources of a type a completed sync did not report. + if gh.newEnterpriseRoleClients == nil { resourceSyncers = append(resourceSyncers, LicenseBuilder(gh.customClient, gh.enterprises)) } } @@ -538,6 +543,22 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { // ctx scopes the discovery requests to the caller. connectorCtx outlives them // and is what the memoized clients keep for refreshing the installation token, // which expires after an hour or on the first 401. +// distinctEnterprises folds the slugs, which GitHub matches case-insensitively, +// so a repeated value does not read as several enterprises. +func distinctEnterprises(enterprises []string) []string { + seen := make(map[string]struct{}, len(enterprises)) + distinct := make([]string, 0, len(enterprises)) + for _, enterprise := range enterprises { + key := strings.ToLower(enterprise) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + distinct = append(distinct, enterprise) + } + return distinct +} + func newEnterpriseRoleClients( ctx context.Context, connectorCtx context.Context, @@ -548,6 +569,9 @@ func newEnterpriseRoleClients( orgHTTPClient *http.Client, org string, ) (map[string]*githubEnterpriseAdministratorClient, error) { + // Naming the same enterprise twice, which happens when the flag is set in + // both the environment and the command line, is one enterprise. + enterprises = distinctEnterprises(enterprises) if len(enterprises) == 0 { return nil, nil } diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index 2972329e..1b10d12b 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -152,6 +152,38 @@ func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { require.NotContains(t, err.Error(), "personal access token") } +// Setting the flag in both the environment and the command line lands the +// same enterprise twice, which is still one enterprise. Counting raw entries +// would reject a configuration the operator wrote correctly, with a message +// naming a number they never chose. +func TestNewEnterpriseRoleClientsFoldsRepeatedEnterprises(t *testing.T) { + t.Parallel() + + ctx := context.Background() + client := newGitHubAPITestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/enterprises/example-enterprise/installation", r.URL.Path) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"})) + })) + + _, err := newEnterpriseRoleClients( + ctx, + ctx, + "https://github.com", + client, + oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "unused"}), + []string{"example-enterprise", "Example-Enterprise"}, + nil, + "example-org", + ) + // Reaches the installation lookup rather than the several-enterprises + // guard, which is what proves the fold happened. + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.Contains(t, err.Error(), "not installed on enterprise") + require.NotContains(t, err.Error(), "one enterprise at a time") +} + // The owners are read through one organization, which belongs to one // enterprise, so app auth cannot serve a list and picking one would be a // guess. This fails for the same reason as a missing installation: a sync that From 5a033f5f949dff04acaefc2add928e915eda43ac Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 15:52:30 -0300 Subject: [PATCH 19/22] test(github): lock provisioning to the deployments that can use it Co-authored-by: Cursor --- .../enterprise_installations_test.go | 42 +++++++++++++++++++ pkg/connector/enterprise_role.go | 6 +++ 2 files changed, 48 insertions(+) diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index 1b10d12b..9a8e5804 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -15,6 +15,7 @@ import ( "google.golang.org/grpc/status" "github.com/conductorone/baton-github/pkg/customclient" + "github.com/conductorone/baton-sdk/pkg/connectorbuilder" resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" ) @@ -152,6 +153,47 @@ func TestNewEnterpriseRoleClientsRequiresEnterpriseInstall(t *testing.T) { require.NotContains(t, err.Error(), "personal access token") } +// The SDK derives CAPABILITY_PROVISION by type-asserting each registered +// syncer, so the split between the two types is the whole mechanism keeping +// provisioning off PAT deployments. Nothing else fails if it is undone: moving +// Grant and Revoke back onto the read-only type, or dropping the check in +// ResourceSyncers, would re-advertise provisioning to PAT with every other +// test still green. +func TestResourceSyncersOfferProvisioningOnlyWhenItWorks(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + provider enterpriseClientProvider + provisioned bool + }{ + {"pat or opt-in off", nil, false}, + {"app auth opted in", func(context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { + return nil, nil + }, true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + gh := &GitHub{ + enterprises: []string{testEnterprise}, + newEnterpriseRoleClients: tc.provider, + } + + var found bool + for _, syncer := range gh.ResourceSyncers(context.Background()) { + if syncer.ResourceType(context.Background()).GetId() != resourceTypeEnterpriseRole.Id { + continue + } + found = true + _, provisions := syncer.(connectorbuilder.ResourceProvisionerV2Limited) + require.Equal(t, tc.provisioned, provisions) + } + require.True(t, found, "enterprise_role must be synced either way") + }) + } +} + // Setting the flag in both the environment and the command line lands the // same enterprise twice, which is still one enterprise. Counting raw entries // would reject a configuration the operator wrote correctly, with a message diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index e9ef3ec9..1078438e 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.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/connectorbuilder" "github.com/conductorone/baton-sdk/pkg/pagination" "github.com/conductorone/baton-sdk/pkg/types/entitlement" "github.com/conductorone/baton-sdk/pkg/types/grant" @@ -268,6 +269,11 @@ func (o *enterpriseRoleResourceType) Grants( return ret, &resourceSdk.SyncOpResults{}, nil } +// Asserted here because the SDK reads the capability off these types: if the +// provisioner ever stops satisfying the interface the build breaks, rather +// than the capability quietly disappearing. +var _ connectorbuilder.ResourceProvisionerV2 = (*enterpriseRoleProvisioner)(nil) + // enterpriseRoleProvisioner adds Grant and Revoke to the read-only syncer. // // It is a separate type because the SDK derives CAPABILITY_PROVISION from the From e28eb7b4a4d0b4c8480568ea389448ddf597bbee Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 23 Sep 2026 15:55:34 -0300 Subject: [PATCH 20/22] docs(github): correct what the opt-in changes and drop the limitation it removed Co-authored-by: Cursor --- docs/docs-info.md | 6 +----- pkg/connector/connector.go | 32 ++++++++++++++++---------------- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/docs/docs-info.md b/docs/docs-info.md index dde1a0dc..dd4ab6f5 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -241,7 +241,7 @@ Consequences worth knowing: ### Enterprise owner provisioning is opt-in -`--enable-enterprise-owner-provisioning` is off by default, and while it is off the connector behaves exactly as it did before this capability existed: enterprise roles come from the consumed-licenses cache, which is PAT-only, so App deployments report none and nothing fails. +`--enable-enterprise-owner-provisioning` is off by default, and while it is off the connector behaves exactly as it did before this capability existed. Enterprise roles come from the consumed-licenses cache, which is PAT-only, so an App deployment reports none of them, and the `license` resource type still fails the sync on that same 403 — as it did before this change. Nothing about an upgrade is different until the capability is asked for. The flag exists because turning the capability on requires setup nobody has done yet — a second installation of the App, on the enterprise account. Without the flag that requirement would reach every deployment that already passes `--enterprises` under App authentication, and the failure below would turn their working sync into a failing one on upgrade. With it, the failure is only reachable by an operator who asked for the capability. @@ -271,10 +271,6 @@ An invitation sent to someone who is not a member of the enterprise is therefore 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. -### PAT deployments advertise a provisioning capability they cannot use - -`CAPABILITY_PROVISION` is derived from the builder implementing the provisioner interface, not from the auth mode, so adding Grant and Revoke turns the capability on for `enterprise_role` under PAT authentication too. There, every request is rejected with `FailedPrecondition` naming the missing enterprise installation, including for the Owner role the consumed-licenses sync does emit. The alternative — advertising the capability only under App auth — is not expressible through that interface. - --- ## Authentication diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 170b1302..7bc5511d 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -543,22 +543,6 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { // ctx scopes the discovery requests to the caller. connectorCtx outlives them // and is what the memoized clients keep for refreshing the installation token, // which expires after an hour or on the first 401. -// distinctEnterprises folds the slugs, which GitHub matches case-insensitively, -// so a repeated value does not read as several enterprises. -func distinctEnterprises(enterprises []string) []string { - seen := make(map[string]struct{}, len(enterprises)) - distinct := make([]string, 0, len(enterprises)) - for _, enterprise := range enterprises { - key := strings.ToLower(enterprise) - if _, ok := seen[key]; ok { - continue - } - seen[key] = struct{}{} - distinct = append(distinct, enterprise) - } - return distinct -} - func newEnterpriseRoleClients( ctx context.Context, connectorCtx context.Context, @@ -647,6 +631,22 @@ func newEnterpriseRoleClients( return clients, nil } +// distinctEnterprises folds the slugs, which GitHub matches case-insensitively, +// so a repeated value does not read as several enterprises. +func distinctEnterprises(enterprises []string) []string { + seen := make(map[string]struct{}, len(enterprises)) + distinct := make([]string, 0, len(enterprises)) + for _, enterprise := range enterprises { + key := strings.ToLower(enterprise) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + distinct = append(distinct, enterprise) + } + return distinct +} + func newGitHubGraphqlClient(ctx context.Context, instanceURL string, ts oauth2.TokenSource) (*githubv4.Client, error) { endpoint, err := enterpriseGraphQLEndpoint(instanceURL) if err != nil { From 2a8e8901f6062a245181439c1c1fd7fa28b07519 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Thu, 24 Sep 2026 14:42:44 -0300 Subject: [PATCH 21/22] fix(github): address enterprise owner review findings Co-authored-by: Cursor --- config_schema.json | 1 + docs/docs-info.md | 6 +-- pkg/config/config.go | 2 +- pkg/connector/connector.go | 8 ++-- .../enterprise_installations_test.go | 20 ++++++++++ pkg/connector/enterprise_role.go | 32 +++++++--------- pkg/connector/enterprise_role_test.go | 38 ++++++++----------- pkg/connector/graphql_transport_test.go | 8 ++-- 8 files changed, 62 insertions(+), 53 deletions(-) diff --git a/config_schema.json b/config_schema.json index e7f3f901..dec08605 100644 --- a/config_schema.json +++ b/config_schema.json @@ -213,6 +213,7 @@ "app-privatekey-path", "app-privatekey", "org", + "enterprises", "sync-secrets", "enable-enterprise-owner-provisioning", "omit-archived-repositories", diff --git a/docs/docs-info.md b/docs/docs-info.md index dd4ab6f5..d6cc1707 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -230,11 +230,11 @@ Only the built-in **Owner** role of an enterprise is provisionable, and only und **`Enterprise.members(role: OWNER)` is the wrong list.** That argument is an `EnterpriseUserAccountMembershipRole`, whose `OWNER` means "owner of an *organization* in the enterprise" — a different enum from the `EnterpriseAdministratorRole` the mutations take. Owners are read from `Organization.enterpriseOwners` instead, which returns every owner of the organization's enterprise account annotated with their role in that organization. The query must not pass `organizationRole`, because that would drop owners who are not owners of the organization. -**There is no single operation that assigns Owner.** Someone who already administers the enterprise, such as a billing manager, is promoted in place. Anyone else can only be invited, and the role lands when they accept. Which case applies is only readable through `ownerInfo`, so Grant attempts the invitation first and promotes as the fallback, keyed on the `FailedPrecondition` that GitHub's `UNPROCESSABLE` error maps to. +**There is no single operation that safely assigns Owner in every case.** A member becomes an Owner by accepting an invitation, but GitHub rejects that invitation when the user already has an enterprise administrator role such as Billing Manager. A GitHub App cannot read that prior role. The connector therefore returns `FailedPrecondition` instead of promoting the administrator in place: otherwise Revoke could only demote them to `UNAFFILIATED`, permanently discarding the role they held before the grant. Consequences worth knowing: -- Grant returns the grant in both cases: when it promoted an administrator, and when it only created an invitation. `Grants()` matches that and emits pending invitations alongside accepted Owners, so C1 keeps a record of the request from the moment it is made +- Grant returns the grant when it creates an invitation. `Grants()` emits pending invitations alongside accepted Owners, so C1 keeps a record of the request from the moment it is made - Revoke clears both states rather than treating them as alternatives: it demotes an active Owner to `UNAFFILIATED`, which keeps them as a member of the enterprise rather than evicting them, and cancels an unaccepted invitation. A `NOT_FOUND` on either is success, because it means the state being asked for is already in place - Reading owners uses the **organization** installation token and every mutation uses the **enterprise** installation token; the enterprise token is rejected on organization fields. Startup verifies that the configured organization belongs to the configured enterprise, and a failure there fails the sync - Only one enterprise can be served under App authentication, because the owners are read through the single configured organization and an organization belongs to exactly one enterprise. A configuration naming several is rejected while the clients are built, with an explanatory error rather than a later failure on a check the operator cannot satisfy. The PAT path does accept a list @@ -259,7 +259,7 @@ C1 has no pending state for a grant, so an invitation nobody has accepted and an - An access review or an offboarding sweep counts an invitee as holding Owner. They do not hold it — GitHub assigns the role only on acceptance - Nothing tracks an expiry. GitHub stops resolving an invitation once it is accepted, cancelled, or expired, so it simply stops being emitted and C1 drops the grant on that sync. `EnterpriseAdministratorInvitation` exposes no `expiresAt`, so there is nothing to compute from either -- **Time-bound access is measured from the invitation, not from acceptance.** C1 starts the clock when Grant reports success, which is when the invitation is sent. Someone who accepts three hours into a four-hour window holds the role for one hour, and the record still reads four. If the window closes first, Revoke cancels the unaccepted invitation — the right action, since the request expired, but C1 logs a completed Owner grant for someone who never held the role. Prefer windows comfortably longer than the invitee takes to accept, or grant to people who already administer the enterprise, who are promoted immediately +- **Time-bound access is measured from the invitation, not from acceptance.** C1 starts the clock when Grant reports success, which is when the invitation is sent. Someone who accepts three hours into a four-hour window holds the role for one hour, and the record still reads four. If the window closes first, Revoke cancels the unaccepted invitation — the right action, since the request expired, but C1 logs a completed Owner grant for someone who never held the role. Prefer windows comfortably longer than the invitee takes to accept ### The list of pending invitations cannot be complete diff --git a/pkg/config/config.go b/pkg/config/config.go index 498735fe..3b10b1d3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -153,7 +153,7 @@ var Config = field.NewConfiguration( DisplayName: "GitHub app", HelpText: "Use a github app for authentication", Fields: []field.SchemaField{ - appIDField, appPrivateKeyPath, appPrivateKey, orgField, syncSecrets, + appIDField, appPrivateKeyPath, appPrivateKey, orgField, EnterprisesField, syncSecrets, enableEnterpriseOwnerProvisioning, omitArchivedRepositories, directCollaboratorsOnly, }, Default: false, diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 7bc5511d..8bd88fbf 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -363,6 +363,7 @@ func NewLambdaConnector(ctx context.Context, ghc *cfg.Github, cliOpts *cli.Conne } func newWithGithubPAT(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { + enterprises := distinctEnterprises(ghc.Enterprises) ts := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: ghc.Token}, ) @@ -379,7 +380,7 @@ func newWithGithubPAT(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { customClient: customclient.New(ghClient), instanceURL: ghc.InstanceUrl, orgs: ghc.Orgs, - enterprises: ghc.Enterprises, + enterprises: enterprises, graphqlClient: graphqlClient, orgCache: newOrgNameCache(ghClient), syncSecrets: ghc.SyncSecrets, @@ -404,6 +405,7 @@ func appPrivateKeyPEM(ghc *cfg.Github) (string, error) { } func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { + enterprises := distinctEnterprises(ghc.Enterprises) privateKey, err := appPrivateKeyPEM(ghc) if err != nil { return nil, err @@ -506,7 +508,7 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { if ghc.EnableEnterpriseOwnerProvisioning { newEnterpriseRoleClientsFn = func(ctx context.Context) (map[string]*githubEnterpriseAdministratorClient, error) { return newEnterpriseRoleClients( - ctx, connectorCtx, ghc.InstanceUrl, appClient, jwtts, ghc.Enterprises, appHTTPClient, ghc.Org) + ctx, connectorCtx, ghc.InstanceUrl, appClient, jwtts, enterprises, appHTTPClient, ghc.Org) } } @@ -516,7 +518,7 @@ func newWithGithubApp(ctx context.Context, ghc *cfg.Github) (*GitHub, error) { customClient: customclient.New(ghClient), instanceURL: ghc.InstanceUrl, orgs: []string{ghc.Org}, - enterprises: ghc.Enterprises, + enterprises: enterprises, newEnterpriseRoleClients: newEnterpriseRoleClientsFn, graphqlClient: graphqlClient, orgCache: newOrgNameCache(ghClient), diff --git a/pkg/connector/enterprise_installations_test.go b/pkg/connector/enterprise_installations_test.go index 9a8e5804..c872bc26 100644 --- a/pkg/connector/enterprise_installations_test.go +++ b/pkg/connector/enterprise_installations_test.go @@ -14,6 +14,7 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + cfg "github.com/conductorone/baton-github/pkg/config" "github.com/conductorone/baton-github/pkg/customclient" "github.com/conductorone/baton-sdk/pkg/connectorbuilder" resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" @@ -226,6 +227,25 @@ func TestNewEnterpriseRoleClientsFoldsRepeatedEnterprises(t *testing.T) { require.NotContains(t, err.Error(), "one enterprise at a time") } +func TestConnectorFoldsRepeatedEnterprisesBeforeBuildingSyncers(t *testing.T) { + t.Parallel() + + gh, err := newWithGithubPAT(context.Background(), &cfg.Github{ + Token: "test-token", + InstanceUrl: githubDotCom, + Enterprises: []string{"example-enterprise", "example-enterprise"}, + }) + require.NoError(t, err) + require.Equal(t, []string{"example-enterprise"}, gh.enterprises) + + resources, _, err := appList( + gh.enterprises, + map[string]*githubEnterpriseAdministratorClient{"example-enterprise": nil}, + ) + require.NoError(t, err) + require.Len(t, resources, 1) +} + // The owners are read through one organization, which belongs to one // enterprise, so app auth cannot serve a list and picking one would be a // guess. This fails for the same reason as a missing installation: a sync that diff --git a/pkg/connector/enterprise_role.go b/pkg/connector/enterprise_role.go index 1078438e..74289bac 100644 --- a/pkg/connector/enterprise_role.go +++ b/pkg/connector/enterprise_role.go @@ -499,11 +499,11 @@ func (o *enterpriseRoleResourceType) pendingInvitationGrants( // Grant gives a user the built-in Owner role and returns the resulting grant. // -// A member can only become an owner by accepting an invitation, while someone -// who already administers the enterprise is promoted in place. Which case -// applies is unreadable for an installation token, so the invitation is tried -// first and the promotion is the fallback, keyed on the FailedPrecondition -// that GitHub's UNPROCESSABLE maps to. +// A member can only become an owner by accepting an invitation. GitHub rejects +// that invitation for existing administrators, but an installation token +// cannot read their current role. Promoting one in place would therefore make +// Revoke destructive: it could only demote them to UNAFFILIATED rather than +// restore the role they held before the grant. // // An unaccepted invitation counts as held and returns a grant, the same way // Grants() emits it: returning nothing would make C1 drop an overlay that the @@ -538,23 +538,17 @@ func (o *enterpriseRoleProvisioner) Grant( if status.Code(inviteErr) != codes.FailedPrecondition { return nil, annos, inviteErr } - ctxzap.Extract(ctx).Debug("baton-github: invitation rejected, promoting in place instead", + ctxzap.Extract(ctx).Debug("baton-github: invitation rejected; refusing unsafe in-place promotion", zap.String("login", login), zap.Error(inviteErr), ) - if promoteErr := client.UpdateRole( - ctx, state.enterpriseID, login, githubv4.EnterpriseAdministratorRoleOwner, - ); promoteErr != nil { - // UNPROCESSABLE covers more than "already an administrator" — - // seat limits and SSO or EMU restrictions land here too — and - // that reason is the actionable one when the promotion also - // fails. It is carried as text so the promotion keeps %w: its - // status code is what tells C1 whether to retry, and a second - // %w would put the invitation's code first instead. - return nil, annos, fmt.Errorf( - "inviting %s was rejected (%s); promoting in place also failed: %w", - login, inviteErr.Error(), promoteErr) - } + // The rejection is not always "already an administrator" — a seat + // limit or an SSO restriction lands here too — so the message states + // the policy and lets GitHub give the reason. + return nil, annos, fmt.Errorf( + "baton-github: cannot grant enterprise Owner to %s, and promoting in place is not attempted "+ + "because a prior administrator role cannot be read back and revoke would discard it: %w", + login, inviteErr) } state, stateAnnos, err = client.OwnerState(ctx, enterprise, login) diff --git a/pkg/connector/enterprise_role_test.go b/pkg/connector/enterprise_role_test.go index ae22dfa1..a6d03c25 100644 --- a/pkg/connector/enterprise_role_test.go +++ b/pkg/connector/enterprise_role_test.go @@ -448,19 +448,19 @@ func TestEnterpriseRoleGrant(t *testing.T) { require.Empty(t, stub.updatedRole) }) - // A billing manager cannot be invited and is promoted in place instead. - // Which case applies is unreadable for a GitHub App, so the connector - // falls back once the invitation is rejected for that reason. - t.Run("promotes an administrator the invitation rejected", func(t *testing.T) { + // A billing manager cannot be invited. Which existing role caused the + // rejection is unreadable for a GitHub App, so promoting in place would + // make Revoke unable to restore the user's previous role. + t.Run("rejects an unsafe promotion after the invitation is rejected", func(t *testing.T) { t.Parallel() stub := &enterpriseStub{inviteErrorType: "UNPROCESSABLE"} builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) - grants, annos, err := builder.Grant(ctx, principal, ent) - require.NoError(t, err) - requireNoIdempotencyClaim(t, annos) - require.Len(t, grants, 1) - require.Equal(t, githubv4.EnterpriseAdministratorRoleOwner, stub.updatedRole) + grants, _, err := builder.Grant(ctx, principal, ent) + require.Equal(t, codes.FailedPrecondition, status.Code(err)) + require.ErrorContains(t, err, "promoting in place is not attempted") + require.Empty(t, grants) + require.Empty(t, stub.updatedRole) require.Empty(t, stub.invitedLogin) }) @@ -479,29 +479,21 @@ func TestEnterpriseRoleGrant(t *testing.T) { require.Empty(t, stub.updatedRole) }) - // When both mutations fail, the promotion's status code is the one that - // reaches C1: it decides whether the task is worth retrying. - t.Run("surfaces the promotion status when both mutations fail", func(t *testing.T) { + // UNPROCESSABLE covers more than "already an administrator": seat limits + // and SSO or EMU restrictions land here too. Preserve GitHub's reason so + // the operator can distinguish those cases. + t.Run("preserves the invitation rejection reason", func(t *testing.T) { t.Parallel() - // A reason other than "already an administrator", which is the case - // that makes carrying it worthwhile. stub := &enterpriseStub{ inviteErrorType: "UNPROCESSABLE", inviteErrorMessage: "enterprise owner seat limit reached", - updateFails: true, } builder, principal, ent := newTestEnterpriseRoleBuilder(t, stub) _, _, err := builder.Grant(ctx, principal, ent) - require.Equal(t, codes.Unavailable, status.Code(err)) - require.ErrorContains(t, err, "promoting in place also failed") - // GitHub's reason for refusing the invitation is carried too, because - // UNPROCESSABLE is not only "already an administrator" — a seat limit - // or an SSO restriction lands there as well, and then it is the - // actionable half. It is text, not a second %w: wrapping both would - // hand C1 the invitation's code instead of the promotion's. - require.ErrorContains(t, err, "was rejected") + require.Equal(t, codes.FailedPrecondition, status.Code(err)) require.ErrorContains(t, err, stub.inviteErrorMessage) + require.Empty(t, stub.updatedRole) }) t.Run("reports an owner as already granted", func(t *testing.T) { diff --git a/pkg/connector/graphql_transport_test.go b/pkg/connector/graphql_transport_test.go index 5dcfdb04..f89e5b70 100644 --- a/pkg/connector/graphql_transport_test.go +++ b/pkg/connector/graphql_transport_test.go @@ -86,8 +86,8 @@ func TestGraphQLErrorsCode(t *testing.T) { // fail the sync. {name: "rate limit wins", types: []string{"FORBIDDEN", "RATE_LIMITED"}, want: codes.Unavailable}, // Past a rate limit the first classified entry wins, so a later error - // cannot mask it. UNPROCESSABLE is what the invite-to-promote fallback - // in Grant branches on, and a trailing FORBIDDEN used to overwrite it. + // cannot mask it. Grant must preserve UNPROCESSABLE as the actionable + // FailedPrecondition, and a trailing FORBIDDEN used to overwrite it. {name: "first classified wins", types: []string{"UNPROCESSABLE", "FORBIDDEN"}, want: codes.FailedPrecondition}, {name: "first classified entry wins", types: []string{"FORBIDDEN", "UNPROCESSABLE"}, want: codes.PermissionDenied}, // NOT_FOUND is the one code a credential error may override. Callers @@ -96,8 +96,8 @@ func TestGraphQLErrorsCode(t *testing.T) { {name: "credential error beats not found", types: []string{"NOT_FOUND", "FORBIDDEN"}, want: codes.PermissionDenied}, {name: "credential error beats not found, unauthenticated", types: []string{"NOT_FOUND", "UNAUTHENTICATED"}, want: codes.Unauthenticated}, {name: "not found alone still maps to not found", types: []string{"NOT_FOUND", "NOT_FOUND"}, want: codes.NotFound}, - // Order must not decide whether Grant's invite-to-promote fallback - // fires, so UNPROCESSABLE outranks NOT_FOUND from either position. + // Order must not hide Grant's unsafe-promotion rejection, so + // UNPROCESSABLE outranks NOT_FOUND from either position. {name: "unprocessable beats not found", types: []string{"NOT_FOUND", "UNPROCESSABLE"}, want: codes.FailedPrecondition}, {name: "unprocessable beats not found, reversed", types: []string{"UNPROCESSABLE", "NOT_FOUND"}, want: codes.FailedPrecondition}, } From 8943329e11c52bc42c9db03c200eaeda1de1a5a2 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Thu, 24 Sep 2026 14:51:48 -0300 Subject: [PATCH 22/22] docs(github): drop the last mention of the removed promote fallback Co-authored-by: Cursor --- pkg/connector/graphql_transport.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/connector/graphql_transport.go b/pkg/connector/graphql_transport.go index a1fb5b9c..0078de9c 100644 --- a/pkg/connector/graphql_transport.go +++ b/pkg/connector/graphql_transport.go @@ -103,9 +103,9 @@ func graphQLErrorType(graphQLErr graphQLError) string { // matching error text. // // It wraps only the enterprise administration client, because the enterprise -// owner mutations rely on telling apart "already an administrator" (which has a -// documented fallback), a rate limit (retryable) and a missing invitation -// (already revoked). The shared GraphQL client keeps returning the library's +// owner mutations rely on telling apart a rejected invitation (which Grant +// refuses rather than working around), a rate limit (retryable) and a missing +// invitation (already revoked). The shared GraphQL client keeps returning the library's // own error untouched, because userResourceType.checkOrgSAML detects // enterprise-level SAML by matching the text of that error. type enterpriseGraphQLTransport struct {