feat: add baton-appstoreconnect connector source under contrib/ - #1119
feat: add baton-appstoreconnect connector source under contrib/#1119c1-squire-dev[bot] wants to merge 1 commit into
Conversation
Adds a complete Apple App Store Connect connector (CXH-2377). It syncs team members, the fixed App Store Connect role enum, and apps, and it provisions role and per-app access back. Why it is here rather than in its own repository: connectors live in ConductorOne/baton-appstoreconnect, created from baton-starter-pack, and that repository does not exist yet. `contrib/` is a staging area — the directory is a self-contained Go module, so the SDK's build, tests and linters do not descend into it and the SDK's dependency graph is untouched. contrib/README.md has the `git subtree split` command that moves it out once the repository exists, after which the directory should be deleted. What the connector does: - Auth: ES256 JWTs signed with the .p8 key, minted with a 15-minute lifetime (Apple's ceiling is 20) and rolled over two minutes before expiry so a long sync never carries a dead token. Signing is stdlib-only; the JWS fixed-width R||S encoding is covered by a test that would catch the classic short- coordinate bug. - Sync: users (roles inline, one paginated pass, no per-user fan-out), pending userInvitations as PENDING users, the static role enum, and apps with visibleApps entitlements. A user with allAppsVisible holds every app's entitlement, since they really can see every app. - Provisioning: role grant/revoke and app-visibility grant/revoke via PATCH /v1/users (full-replace, so current state is read first), account creation via POST /v1/userInvitations, and deletion that falls back to cancelling an invitation when the id is not a user. - Rate limiting: Apple's non-standard x-rate-limit header is parsed into rate-limit annotations, and 429s carry Retry-After when present. Guardrails worth calling out: ACCOUNT_HOLDER is synced but refuses grant and revoke, because Apple does not allow it through the API; revoking a single app from an allAppsVisible user is refused rather than silently narrowing them to a hand-built list; and a visibleApps relationship truncated by Apple's 50-app inline cap is re-read from the relationship endpoint, since trusting it would drop grants on sync and revoke apps on provisioning. Testing: 39 unit tests drive a stand-in API over httptest, covering the JWT signing path, pagination, rate-limit parsing, error mapping and every provisioning payload. A full sync was also run end-to-end against a fake App Store Connect API and the resulting .c1z inspected with the baton CLI. golangci-lint reports no issues. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for i := range invitations { | ||
| if !invitations[i].Attributes.AllAppsVisible && !contains(invitations[i].VisibleAppIDs(), appID) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🟠 Bug: the invitation branch trusts the inlined visibleApps relationship, while the user branch above deliberately does not (userSeesApp → VisibleAppsComplete() → relationship endpoint). ListUserInvitations requests limit[visibleApps]=50 (client.go:209), so an invitation with more than 50 visible apps returns a truncated data array plus meta.paging.total, and every app past the cap silently loses its grant — the exact failure mode the user path guards against. Add a VisibleAppsComplete() to UserInvitation and fall back to GET /v1/userInvitations/{id}/visibleApps when the relationship is truncated or absent.
|
|
||
| switch bag.Current().ResourceTypeID { | ||
| case pageStateUsers: | ||
| users, nextURL, annos, err := a.client.ListUsers(ctx, bag.PageToken()) |
There was a problem hiding this comment.
🟡 Suggestion: request cost scales as (13 roles + N apps) × full user+invitation pagination, because Grants is invoked per role and per app and each invocation re-walks /v1/users and /v1/userInvitations. It compounds at line 172: a user whose inlined relationship is truncated gets a fresh paginated visibleApps fetch once per app. With ~200 apps and ~2000 users that is a few thousand calls against Apple's ~3600/hour budget. connectorbuilder.TypeScopedGrantsSyncer (GrantsForResourceType) lets one pass over users emit all app/role grants; worth doing before this hits a large team.
| user, annos, err := r.client.GetUser(ctx, userID) | ||
| if err != nil { | ||
| if appstoreconnect.IsNotFound(err) { | ||
| return append(annos, annotations.New(&v2.ResourceDoesNotExist{})...), nil | ||
| } | ||
| return annos, fmt.Errorf("baton-appstoreconnect: failed to read user before revoking role: %w", err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Grants emits role grants for pending invitations (principal id = invitation id), but revoke resolves the principal through GET /v1/users/{id}, which 404s for an invitation id. That returns ResourceDoesNotExist, telling C1 the principal is gone when the invitee is in fact still an outstanding invitation holding the role. app.go's Revoke has the same shape. Consider checking GET /v1/userInvitations/{id} on 404 and returning an explicit FailedPrecondition ("cancel the invitation instead") rather than ResourceDoesNotExist.
| func resetHint(response *http.Response, overLimit bool) *timestamppb.Timestamp { | ||
| if retryAfter := response.Header.Get("Retry-After"); retryAfter != "" { | ||
| if seconds, err := strconv.ParseInt(retryAfter, 10, 64); err == nil { | ||
| return timestamppb.New(time.Now().Add(time.Duration(seconds) * time.Second)) | ||
| } | ||
| if at, err := http.ParseTime(retryAfter); err == nil { | ||
| return timestamppb.New(at) | ||
| } | ||
| } | ||
|
|
||
| if overLimit { | ||
| return timestamppb.New(time.Now().Add(rateLimitWindow)) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🟡 Suggestion: on a successful response there is no Retry-After and overLimit is false, so ResetAt is never set. ratelimit.MemRateLimiter.Report bails out on !desc.HasResetAt(), so the parsed user-hour-lim/user-hour-rem budget never paces anything — the annotation is decorative except on 429s. Setting ResetAt to now + rateLimitWindow on every response with the header would let a sliding limiter actually use the budget. Relatedly, a header carrying user-hour-lim but not user-hour-rem reports Remaining: 0 with STATUS_OK, which is misleading (harmless today only because Report also short-circuits on Remaining == 0).
| func (c *Client) url(path string, query url.Values) string { | ||
| resolved := *c.baseURL | ||
| resolved.Path = strings.TrimSuffix(resolved.Path, "/") + path | ||
| if len(query) > 0 { | ||
| resolved.RawQuery = query.Encode() | ||
| } | ||
| return resolved.String() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: callers pass already-escaped path segments (url.PathEscape(userID)), but this assigns into URL.Path, which is the decoded field. With RawPath empty, String() re-escapes, so a % from PathEscape becomes %25 — double-encoding. Harmless for Apple's opaque numeric/UUID ids, but it makes the PathEscape calls actively wrong rather than defensive. Either drop the PathEscape and build Path from raw segments, or set RawPath alongside Path.
| FirstName: firstName, | ||
| LastName: lastName, | ||
| Roles: roles, | ||
| AllAppsVisible: boolFromProfileField(fields, profileFieldAllAppsVisible, true), |
There was a problem hiding this comment.
🟡 Suggestion: all_apps_visible defaults to true, so a JML create that omits the field grants the new member visibility of every app in the team — including anything the requester never asked for. The schema default (connector.go:51) agrees, so this is deliberate, but defaulting to false (app access then arrives via the app:<id>:visible grants C1 already models) is the least-privilege shape and matches how the rest of the connector treats per-app access.
|
|
||
| Nothing here is part of the `github.com/conductorone/baton-sdk` Go module. Each subdirectory is a | ||
| self-contained module with its own `go.mod`, so the SDK's `go build ./...`, `go test ./...` and | ||
| `golangci-lint run` do not descend into it, and the SDK's own dependency graph is unaffected. |
There was a problem hiding this comment.
🟡 Suggestion: the isolation cuts both ways — because contrib/baton-appstoreconnect/.github/workflows/ is nested rather than at the repo root, none of those workflows run here, and the root .github/workflows/ never descends into contrib/. So while this code lives in baton-sdk nothing builds, tests, or lints it in CI, and go.mod pins the published baton-sdk v0.26.0 rather than replace-ing the local tree, so it is not verified against this repo's SDK either. Worth either a small root job that runs go build ./... && go test ./... per contrib/*/go.mod, or a line here stating plainly that the staged copy is CI-unverified.
General PR Review: feat: add baton-appstoreconnect connector source under contrib/Blocking Issues: 1 | Suggestions: 6 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. This adds 5,931 lines under Risk triage (per Security IssuesNone found. JWT signing is stdlib ES256 over Correctness Issues
Suggestions
Prompt for AI agents |
Summary
Adds a complete Apple App Store Connect connector (CXH-2377) — sync and provisioning — as a
self-contained Go module under
contrib/baton-appstoreconnect/.App Store Connect users are personal Apple IDs that sit outside SSO and SCIM, so today there is no
central view of who holds Admin or App Manager on the account that controls code signing, releases
and financial reports. This brings that under access review and JML like any other app.
Why the code is in this repo
Connectors live in their own repository (
ConductorOne/baton-appstoreconnect), created frombaton-starter-pack. That repository does not
exist yet, and I could not create it: the GitHub App available in this environment mints only
repo-scoped tokens and
POST /orgs/ConductorOne/reposreturns403 … X-Accepted-GitHub-Permissions: administration=write. Rather than throw away working code,it is staged here.
contrib/is deliberately inert with respect to the SDK. Each subdirectory has its owngo.mod, sogo build ./...,go test ./...,golangci-lint runandmake race-shard-auditat the repo rootdo not descend into it and the SDK's dependency graph is untouched (verified — see Testing).
contrib/README.mdcarries thegit subtree splitcommand that moves the connector out once therepository exists, after which this directory should be deleted. If a reviewer would rather this
never touch
baton-sdkat all, closing the PR loses nothing: create the repo, run the subtree splitfrom this branch, and push.
What the connector does
Auth. ES256 JWTs signed with the
.p8key, minted with a 15-minute lifetime (Apple's ceiling is20) and rolled over two minutes before expiry, so a sync longer than a token's life keeps working.
Signing is stdlib-only — no new JWT dependency — and the JWS fixed-width
R||Sencoding has a testthat would catch the classic short-coordinate bug that makes Apple reject roughly 1 signature in 256.
Sync.
GET /v1/usersGET /v1/userInvitationsPENDINGstatus.GET /v1/appsvisibleApps) access.Entitlements are
role:<ROLE>:assignedandapp:<id>:visible. A user withallAppsVisibleholdsthe entitlement on every app, because they genuinely can see every app; omitting them would
understate access at review time.
Provisioning. Role grant/revoke and app-visibility grant/revoke via
PATCH /v1/users/{id},account creation via
POST /v1/userInvitations, and deletion that falls back to cancelling aninvitation when the id turns out to be a pending invitee rather than a user.
Rate limiting. Apple's non-standard
x-rate-limitheader (user-hour-lim/user-hour-rem) isparsed into rate-limit annotations on every response, and 429s carry
Retry-Afterwhen present.Guardrails worth a reviewer's attention
These are the places where the obvious implementation is wrong:
ACCOUNT_HOLDERis synced so it appears in reviews, but grant and revoke refuse with a clearmessage — Apple does not allow it through the API.
allAppsVisibleuser is refused, not silently honoured. The grantexists because the user sees everything, so "removing one app" would mean turning
allAppsVisibleoff and rebuilding the list of everything they should keep — much larger than therequest asked for.
visibleAppsrelationship at 50 per user. The truncation is detectedfrom
meta.paging.totaland re-read fromGET /v1/users/{id}/visibleApps. Trusting the truncatedlist would drop grants on sync and revoke apps during provisioning, since updates are
full-replace.
VisibleAppsComplete()returns false when therelationship was never requested, so no code path can mistake "unknown" for "no access".
is not atomic; it is documented in the README, and ConductorOne serializes provisioning per user,
so the realistic exposure is a concurrent edit in the Apple UI.
uhttpmaps every 4xx ontocodes.InvalidArgument, sothe client wraps failures in an
APIErrorcarrying the HTTP status and Apple's error document.Testing
httptest,covering the JWT signing path (signature verified against the public key, 200 iterations for the
fixed-width encoding), two-phase pagination,
next-link following, rate-limit parsing including429, error mapping, and the exact JSON body of every provisioning request — including that an
omitted
visibleAppsrelationship stays omitted and an intentional revoke-to-zero sends anexplicit empty array.
resulting
.c1zwas read back with thebatonCLI. Users, the pending invitation, all 13 roles,both apps, role grants and app-visibility grants all came out correct.
golangci-lint run(v2.9.0, the connector's own config): 0 issues.go list ./...returns nocontribpackages,go build ./...passes, andmake race-shard-auditstill reports every package covered exactlyonce.
Follow-ups
ConductorOne/baton-appstoreconnectfrombaton-starter-pack, extract with thegit subtree splitincontrib/README.md, then deletecontrib/from this repo.make update-depsto vendor (connector repos vendor; this staged copy does not,to keep the diff reviewable), and let
generate-baton-metadataproducebaton_capabilities.jsonand
config_schema.json.is written but skipped until
CI_ROLE_ENTITLEMENTis set.provisioningAllowedas its own entitlement. It issynced onto the user profile but is a per-user flag rather than access to a resource.
Live Preview
Open preview — the end-to-end sync
output, read back from the
.c1zwith thebatonCLI. The connector is a CLI rather than aservice, so this page is the reviewable artifact instead of a running app.