Skip to content

nhi: add GitHub Apps syncer (APP_REGISTRATION)#171

Merged
luisina-santos merged 5 commits into
mainfrom
nhi/add-github-apps-syncer
Jun 4, 2026
Merged

nhi: add GitHub Apps syncer (APP_REGISTRATION)#171
luisina-santos merged 5 commits into
mainfrom
nhi/add-github-apps-syncer

Conversation

@c1-squire-dev
Copy link
Copy Markdown
Contributor

@c1-squire-dev c1-squire-dev Bot commented May 31, 2026

Summary

GitHub Apps and their installations are not synced today. The connector authenticates as a GitHub App — it mints the App JWT (connector.go:getJWTToken), finds the org installation (connector.go:findInstallation), and mints an installation token — but throws that identity away: the resource syncers are org/team/repository/user/invitation/api-key/org_role/enterprise_role only (connector.go ResourceSyncers).

This PR adds a new read-only app resource type that enumerates org-installed GitHub Apps and emits each as a non-human identity (NHI) app registration.

This is the Class-B (new-syncer) GitHub item from NHI Phase-1 (RFC §5.8.8, coverage matrix §6 row 6). It follows the proven new-syncer pattern (new resource type + builder + paginated read-only List + register + emit), mirroring the existing api-key secret syncer (api_token.go).

What it does

  • New resource type app (TRAIT_APP, SkipEntitlementsAndGrants), child of org.
  • Enumeration: Organizations.ListInstallationsGET /orgs/{org}/installations, paginated, per org. This is the org-wide-enumerable window onto GitHub Apps and works with the connector's existing client (installation-token in App-auth mode, PAT in PAT mode).
  • NHI emission (baton-sdk v0.11.0):
    resourceSdk.WithNHIType(v2.NonHumanIdentityTrait_NHI_TYPE_APP_REGISTRATION, "github.app")
  • App profile (dotted/lowercase keys): app_id, app_slug, installation_id, account_login, target_type, repository_selection.

Enumerability / auth scope-guard

GET /orgs/{org}/installations requires Organization administration (read).

  • PAT auth: the connector already validates that the token is an org admin (connector.go Validate), so this is generally available.
  • GitHub App auth: the installation token may not be granted Organization administration: read. In that case the endpoint returns 403 (or 404). The syncer logs a warning and skips that org instead of failing the whole sync.

Apps vs app definitions: Org-wide, what is enumerable is installations — each installation row carries the app's identity (app_id, app_slug), which is what we surface as the app registration. There is no PAT-accessible endpoint to list app definitions org-wide (GET /app returns only the single authenticated app, JWT-only), so we sync the installation view and capture the app identity from it. Within an org a given app has exactly one installation, so this enumerates the distinct installed apps.

SDK bump

Bumps baton-sdk v0.10.0 → v0.11.0 (for WithNHIType / NonHumanIdentityTrait). Repo vendors deps, so go mod tidy && go mod vendor was run; vendor changes are scoped to baton-sdk + modules.txt.

Testing

  • go build ./...
  • go vet ./pkg/connector/...
  • go test ./... ✅ (new TestAppResource asserts the NHI annotation, app trait, and profile)
  • golangci-lint run ./... → 0 issues ✅

Files

  • pkg/connector/app.go (new) — resource builder + paginated List + AppBuilder
  • pkg/connector/app_test.go (new)
  • pkg/connector/connector.goresourceTypeApp + register AppBuilder
  • pkg/connector/org.goapp child-resource-type annotation
  • go.mod / go.sum / vendor/ — SDK bump

@c1-squire-dev c1-squire-dev Bot requested a review from a team May 31, 2026 16:11
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 31, 2026

Connector PR Review: nhi: add GitHub Apps syncer (APP_REGISTRATION)

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Review mode: full
View review run

Review Summary

This PR adds a new read-only app resource type that syncs installed GitHub Apps as non-human identity (NHI) app registrations under each organization. The implementation follows established connector patterns: paginated List via the new nextPageToken helper, TRAIT_APP with SkipEntitlementsAndGrants, and a CapabilityPermissions annotation declaring the organization_administration:read scope. The refactoring of pagination boilerplate into nextPageToken across api_token.go, org.go, repository.go, and team.go is clean and consistent. CI config correctly moves hardcoded test values to repository variables.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@c1-squire-dev c1-squire-dev Bot force-pushed the nhi/add-github-apps-syncer branch from 7b56ce2 to 4ec249d Compare May 31, 2026 17:03
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

pquerna and others added 2 commits June 1, 2026 04:13
GitHub Apps and their installations were never synced — the connector
authenticates as a GitHub App (mints the App JWT, finds the org
installation) but discarded that identity. Add a read-only `app` resource
type that enumerates org-installed GitHub Apps via
`GET /orgs/{org}/installations` (Organizations.ListInstallations) and emits
each as a non-human identity:

  WithNHIType(NHI_TYPE_APP_REGISTRATION, "github.app")

Each app carries TRAIT_APP plus a profile (app_id, app_slug,
installation_id, account_login, target_type, repository_selection). The
endpoint requires Organization administration (read); when the configured
credentials lack it (e.g. a GitHub App installation token), the syncer logs
a warning and skips that org rather than failing the whole sync.

Bumps baton-sdk v0.10.0 -> v0.11.0 for WithNHIType and re-vendors.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…archived (v0.4.5) and can't resolve NonHumanIdentityTrait

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@c1-squire-dev c1-squire-dev Bot force-pushed the nhi/add-github-apps-syncer branch from 75ee386 to e1332c3 Compare June 1, 2026 04:13
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

luisina-santos and others added 2 commits June 3, 2026 16:06
- Remove V1Identifier from app resource (new resource type, no v1 history)
- Remove manual 403/404 skip; capability permissions handle access gating
- Add CapabilityPermissions annotation (organization_administration:read) to resourceTypeApp
- Extract nextPageToken() helper combining parseResp + bag.NextToken; apply to app, api_token, org, team, repository builders — also fixes latent double-extractRateLimitData bug in those builders
- Update README and connector.mdx: add GitHub Apps (NHI) to data model, capabilities table, and fine-grained token permissions

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 3, 2026

CXP-618

Comment thread pkg/connector/app.go
Comment on lines +100 to +101
if err != nil {
return nil, nil, wrapGitHubError(err, resp, "github-connector: failed to list organization app installations")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: The previous version of this code gracefully degraded on 403/404 by logging a warning and skipping the org, which was important for GitHub App auth where the installation token may lack organization_administration:read. The CapabilityPermissions annotation on the resource type is used for metadata reporting only (SDK connectorbuilder reads it in GetCapabilities), not for runtime error handling — so a missing permission will now propagate as a PermissionDenied gRPC error and fail the entire sync. If the intent is to make this permission strictly required, this is fine, but it's a behavior change for App-auth users whose installation token doesn't include Administration read access.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make permission required. if the customer doesn't want to sync apps should disable the resource

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@luisina-santos luisina-santos force-pushed the nhi/add-github-apps-syncer branch from 1cd59f8 to ea15782 Compare June 3, 2026 19:37
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@luisina-santos luisina-santos self-assigned this Jun 3, 2026
@luisina-santos luisina-santos merged commit cfcb947 into main Jun 4, 2026
9 checks passed
@luisina-santos luisina-santos deleted the nhi/add-github-apps-syncer branch June 4, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants