feat: grant teams, repos, and org access to pending invitations - #182
feat: grant teams, repos, and org access to pending invitations#182mstanbCO wants to merge 3 commits into
Conversation
Birthright access provisioned before someone's start date targets an app
user that is still a pending GitHub org invitation, not an accepted
member. Every provisioning path rejected that principal outright ("only
users can be granted team membership"), so the access could not be
pre-staged and only landed after the user clicked through.
Accept invitation principals in the org, team, and repository grant and
revoke paths, and emit grants for pending invitations during sync so
invitees show up next to accepted members instead of looking unfulfilled.
GitHub keys every post-hoc membership write off a username, so an
invitation created from a bare email address has nothing to write
against. For teams and org roles the only mechanism GitHub offers is
team_ids at invitation-creation time, which means cancel-and-reissue;
that is gated behind the new reinvite-pending-invitations option because
it invalidates the outstanding invite link and restarts the 7-day expiry.
CreateAccount now prefers invitee_id when github_username is supplied so
the non-destructive path is available in the first place.
Organization roles stay unsupported for invitations — GitHub requires
active org membership — but now say so explicitly.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // all. Put the original one back so a failed grant does not silently strip | ||
| // access the user already had. | ||
| restoreOpts, restoreOptsErr := reinviteOptions(ctx, client, inv, originalTeamIDs) |
There was a problem hiding this comment.
🟠 Bug: the restore rebuilds its payload from inv, but the org promote/demote callers pass a mutated copy (org.go:479-481 sets Role = admin, org.go:596-598 sets Role = direct_member). So this "put the original one back" path re-creates the invitation with the new role, not the original one. Two bad outcomes: if the create failed because of the role change, the restore fails identically and the invitee is left with no invitation at all; if it failed transiently, the restore silently applies the role change that the grant/revoke just reported as failed.
Fix: have reinviteWithTeams capture the original role (and email/invitee) before any mutation — e.g. take the untouched *github.Invitation plus an explicit newRole parameter, and build restoreOpts from the untouched original. TestReinviteRestoresOriginalOnCreateFailure only exercises a team-set change, so this gap is untested.
| return nil, invitationNotProvisionableError(operation, inv) | ||
| } | ||
|
|
||
| if _, err := reinviteWithTeams(ctx, o.client, orgName, inv, append(existingTeamIDs, teamID)); err != nil { |
There was a problem hiding this comment.
🟠 Bug: permission is dropped on this path. POST /orgs/{org}/invitations accepts only team_ids — there is no per-team role — so an invitation re-issued this way confers plain team member. A maintainer grant against an email-only (or GitHub-refused) invitation therefore returns success while granting member, and pendingTeamRole will keep reporting member at every sync, so the maintainer entitlement stays permanently unfulfilled with no error surfaced.
Fix: only take the reinvite path when permission == teamRoleMember; for teamRoleMaintainer, return a FailedPrecondition explaining that GitHub cannot pre-stage a maintainer role on a pending invitation (same treatment org_role.go already gives org roles). The slices.Contains short-circuit at line 486 also can't distinguish member from maintainer for an already-attached team, so it will report GrantAlreadyExists for a maintainer request that is only a member.
| if isNotFoundError(resp) || isPermissionError(resp) { | ||
| // Without invitation visibility we simply cannot correlate repo | ||
| // invitations; cache the empty index so every repo in this sync | ||
| // does not retry the same failing call. | ||
| ctxzap.Extract(ctx).Debug("github-connector: cannot list pending org invitations, skipping invitation-backed repo grants", | ||
| zap.String("org", orgName), | ||
| zap.String("github_error", gitHubErrorMessage(err)), | ||
| ) | ||
| break | ||
| } | ||
| return nil, wrapGitHubError(err, resp, "github-connector: failed to list pending org invitations") |
There was a problem hiding this comment.
🟠 Bug: isPermissionError(resp) is just StatusCode == 403, which is also how GitHub reports both primary rate limiting (403 + X-RateLimit-Remaining: 0) and secondary rate limiting (403 + Retry-After). A rate-limited sync therefore falls into this branch, and because the (possibly empty or partial) index is then written to the session at line 336, every remaining repository in that sync silently emits zero invitation-backed grants — C1 sees those grants disappear rather than a retryable rate-limit error.
Fix: check isRatelimited(resp) first and fall through to wrapGitHubError so the SDK can back off, and don't persist the index into the session when the walk ended early. The same 403-swallow appears in org.go:311 and team.go:307 (pendingInvitationGrants) — a rate-limited page there returns no grants and no error, which drops org-membership grants for pending invitees.
| l.Warn("github-connector: gave up looking for pending invitation", | ||
| zap.Int64("invitation_id", invitationID), | ||
| zap.String("org", orgName), | ||
| zap.Int("pages_searched", maxPendingInvitationPages), | ||
| ) | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: hitting the 25-page cap returns (nil, nil), which is indistinguishable from "not pending". Callers read that as a definitive answer: org.go:566 and team.go:583 return GrantAlreadyRevoked, and repository.go:663 does the same — i.e. a truncated search reports a revoke as successfully completed while the access is still in place. Consider returning a distinct error on truncation so revokes surface a retryable failure instead of a false success.
| if inv.GetRole() == invitationRoleAdmin { | ||
| rv = append(rv, o.invitationGrant(orgRoleAdmin, resource, principalID, inv.GetID())) | ||
| } | ||
| rv = append(rv, o.invitationGrant(orgRoleMember, resource, principalID, inv.GetID())) |
There was a problem hiding this comment.
🟡 Suggestion: every non-admin role gets an orgRoleMember grant, including billing_manager and hiring_manager. Those invitees are not org members in GitHub and do not appear in ListMembers, so the grant shows up while the invitation is pending and then vanishes on acceptance — and in the meantime an access review sees org membership they never had. Consider emitting membership only for direct_member/admin/reinstate and skipping the billing/hiring roles.
| if err != nil { | ||
| return nil, fmt.Errorf("baton-github: error reading pending invitation index from session: %w", err) | ||
| } | ||
| if found { | ||
| return cached, nil | ||
| } | ||
|
|
||
| index := make(map[string]int64) | ||
| opts := &github.ListOptions{PerPage: maxPageSize} |
There was a problem hiding this comment.
🟡 Suggestion: this walk is uncapped, unlike resolvePendingInvitation's maxPendingInvitationPages, and the whole index is JSON-serialized into the session. An org mid-onboarding with tens of thousands of pending invitations will page indefinitely on the first repository that has any invitation and store a correspondingly large session blob. Applying the same page cap (with a truncation warning) would bound both.
| // member when GitHub cannot tell us (no login to query by, or the lookup fails). | ||
| func (o *teamResourceType) pendingTeamRole(ctx context.Context, orgID, teamID int64, inv *github.Invitation) string { | ||
| login := inv.GetLogin() | ||
| if login == "" { | ||
| return teamRoleMember | ||
| } | ||
|
|
||
| membership, _, err := o.client.Teams.GetTeamMembershipByID(ctx, orgID, teamID, login) | ||
| if err != nil { | ||
| ctxzap.Extract(ctx).Debug("github-connector: could not read pending team membership role, assuming member", | ||
| zap.Int64("team_id", teamID), | ||
| zap.String("login", login), | ||
| zap.String("github_error", gitHubErrorMessage(err)), |
There was a problem hiding this comment.
🟡 Suggestion: pendingTeamRole issues one GetTeamMembershipByID per invitation, so a team with a page of 100 pending invitees costs 100 extra API calls per Grants page, and the *github.Response from each is discarded — its rate-limit headers never reach the SDK as a RateLimitDescription (C3). Returning and merging those annotations, or skipping the lookup unless the team actually has maintainers, would reduce both the call volume and the blind spot.
Connector PR Review: feat: grant teams, repos, and org access to pending invitationsBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commits are almost entirely documentation: expanded rationale comments on Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
The "User isn't a member of this organization. Please invite them first."
422 belongs to the deprecated PUT /teams/{id}/members/{username}, not the
memberships endpoint this connector calls, which is documented to invite
non-members and leave the membership pending. Narrow the comment to the
case the guard actually covers (an org invitation already outstanding) and
also match "already invited".
Note GitHub's 50-invitations-per-repo-per-24h cap on the repository path,
which applies to pending invitees because they are not org members yet.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| _, resp, err := o.client.Repositories.AddCollaborator( | ||
| ctx, | ||
| repo.GetOwner().GetLogin(), | ||
| repo.GetName(), | ||
| login, | ||
| &github.RepositoryAddCollaboratorOptions{Permission: permission}, | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: unlike the resourceTypeUser.Id case above (lines 515-566), the invitation path never checks the invitee's current repository access, so it can neither return GrantAlreadyExists nor a GrantReplaced annotation. Since repoAccessLevels are an EntitlementExclusionGroup and AddCollaborator overwrites the outstanding repository invitation's permission, re-granting admin over a pending push invitation leaves the stale push grant live in C1 until the next sync. Consider reusing the same findRepositoryInvitation lookup (added below for Revoke) to compare inv.GetPermissions() against permission and emit GrantAlreadyExists/GrantReplaced accordingly.
| if err != nil { | ||
| return nil, wrapGitHubError(err, resp, "github-connector: failed to add invited user to repository") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the comment just added at lines 585-587 documents the 50-invitations-per-repo-per-24-hours cap, but nothing here acts on it. wrapGitHubError only treats a 403 as retryable when isRatelimited matches (X-RateLimit-Remaining: 0 / a 429), and the invitation-cap 403 sets neither — so it maps to codes.PermissionDenied and a bulk backfill's grants fail permanently instead of being retried. Consider detecting that refusal (403 whose message mentions the invitation limit) and returning codes.ResourceExhausted/Unavailable via uhttp.WrapErrors so the task is retried after the window resets.
| invitee, _, err := i.client.Users.Get(ctx, params.login) | ||
| if err != nil { | ||
| l.Debug("github-connector: could not resolve github_username, inviting by email instead", | ||
| zap.String("github_username", params.login), | ||
| zap.String("github_error", gitHubErrorMessage(err)), | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: any Users.Get failure — 403 secondary rate limit, 5xx, or a real 404 — is logged at Debug and silently downgrades the account to an email-only invitation, which is exactly the case that later forces the destructive reinvite-pending-invitations path. Per the repo's log-level criteria this non-404 fallback deserves Warn, and the discarded *github.Response means the extra call's rate-limit headers never surface as a RateLimitDescription annotation. Consider capturing resp, distinguishing 404 (Debug) from transient failures (Warn), and merging the rate-limit annotation into the response.
| zap.String("github_error", gitHubErrorMessage(err)), | ||
| ) | ||
| } else { | ||
| inviteOpts = &github.CreateOrgInvitationOptions{InviteeID: github.Ptr(invitee.GetID())} |
There was a problem hiding this comment.
🟡 Suggestion: replacing the whole options struct drops params.email, and GitHub returns email: null for an invitee_id invitation — so invitationToUserResource at line 352 builds the CreateAccount response with WithEmail("", true) and no email in the profile, whereas the previous email-only path always carried one. The connector already knows the address (getCreateUserParams requires it), so downstream identity matching loses data for no reason. Consider setting InviteeID on the existing inviteOpts and backfilling params.email onto the returned resource when invitation.GetEmail() is empty.
The field now decides whether the invitation is addressed to a GitHub
account or just an email address, which in turn decides whether teams and
repository access can be pre-staged at all. The old description ("used to
look up the user if email is private") understated that to whoever
configures account provisioning.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // use. This guard therefore covers the undocumented case: a write refused | ||
| // because an org invitation for that user is already outstanding. | ||
| func isNotAnOrgMemberError(err error, resp *github.Response) bool { | ||
| return isGitHubValidationError(err, resp, "not a member", "isn't a member", "invite them first", "already invited") |
There was a problem hiding this comment.
🟡 Suggestion: "already invited" is broad enough to also catch a 422 that means "already invited to this team", not "an org invitation is outstanding". In that case the grant is effectively already fulfilled, but grantToInvitation treats it as unfulfilled: ListOrgInvitationTeams won't list a team attached via the membership endpoint, so with reinvite-pending-invitations on this triggers a destructive cancel-and-recreate of the org invitation (and with it off, a FailedPrecondition instead of GrantAlreadyExists). Consider narrowing to the org-scoped wording (e.g. "already invited to this organization"), and adding a table-driven test for isNotAnOrgMemberError — the two 422 tests in invitation_grants_test.go only exercise the "isn't a member" wording, so this new branch is uncovered. (confidence: medium)
| repo.GetName(), | ||
| login, | ||
| &github.RepositoryAddCollaboratorOptions{Permission: permission}, | ||
| ) | ||
| if err != nil { | ||
| return nil, wrapGitHubError(err, resp, "github-connector: failed to add invited user to repository") | ||
| } | ||
| default: | ||
| l.Error( |
There was a problem hiding this comment.
🟡 Suggestion: the new comment documents the 50-invites-per-repo-per-24h cap, but the code does nothing with it — wrapGitHubError maps that 403 to codes.PermissionDenied, which reads as a durable permission problem rather than a throttle, so a bulk backfill fails hard instead of being retried later. Consider detecting the cap/abuse message and wrapping with codes.ResourceExhausted so the platform backs off and retries. (confidence: medium)
1 participant
Problem
A customer uses birthright access for GitHub. Birthright fires before the employee's start date, so C1 creates the GitHub account first — which, for GitHub, means sending an organization invitation. The follow-on grant tasks then try to add that person to a team.
Until they accept (in this customer's case, by clicking their Okta tile), the C1 app user is backed by the connector's
invitationresource type, notuser. Every provisioning path here rejected that principal on the first line:So the access couldn't be pre-staged: the grants failed, and the entitlements only landed after the user clicked through. Pending invitations also produced no grants at all during sync, so a pending invitee looked like they had nothing — even org membership their invitation already carried.
What this does
Provisioning —
invitationis now an accepted principal for org membership/admin, team member/maintainer, and all five repository permission levels, and is listed in each entitlement'sgrantable_to.Sync — three new pagination states emit grants for pending invitations, so invite-backed and member-backed grants both show up in C1:
GET /orgs/{org}/invitationsGET /orgs/{org}/teams/{team}/invitationsmember)GET /repos/{o}/{r}/invitationsEach state is pushed first so it drains last, and each degrades to a debug log on 403/404 rather than failing the whole sync.
The hard constraint, and how it's handled
Every GitHub endpoint that writes membership takes a username. An invitation created from a bare email address has no login until GitHub resolves one, so there is nothing to write against. This splits into two paths:
PUT /orgs/{org}/teams/{team}/memberships/{login}andPUT /repos/{o}/{r}/collaborators/{login}both create a pending membership for non-members. Non-destructive; this is the normal path.team_idsonly when an invitation is created, and there is noPATCH /orgs/{org}/invitations/{id}. The only mechanism is cancel-then-recreate.Because cancel-and-reissue invalidates the outstanding invite link, sends a second email, restarts the 7-day expiry, and changes the invitation ID, it is opt-in behind a new
reinvite-pending-invitationsconfig field (default off). When off, the grant fails with an error naming both remedies. When on,reinviteWithTeamsreads the existing team set first, cancels, recreates with the union, and restores the original invitation on a best-effort basis if the recreate fails — otherwise a failed grant would strip an invitation the user already had.CreateAccountnow prefersinvitee_idwhengithub_usernameis supplied, so an invitation starts life with a resolvable login and the non-destructive path is available at all. This is the cheapest lever the customer has: populating GitHub usernames in Okta avoids the reissue path entirely.Organization roles remain unsupported for invitations —
PUT /orgs/{org}/organization-roles/users/{user_id}/{role_id}requires active org membership — but now return an explicitFailedPreconditionexplaining that instead of a generic type error.Things a reviewer should weigh
"User isn't a member of this organization. Please invite them first."belongs to the deprecatedPUT /teams/{id}/members/{username}, not thePUT /orgs/{org}/teams/{team}/memberships/{username}this connector calls. The memberships endpoint is documented to invite non-members and leave the membership pending, so the login path should be the normal case and the reissue fallback genuinely defensive. What remains undocumented is the behavior when an org invitation for that user is already outstanding — GitHub's docs are silent. The guard now covers that case specifically (including"already invited") and falls back rather than failing.SkipSyncAnomalyDetectionis already set on the invitation resource type.Delete).resolvePendingInvitationpages the pending list because GitHub has no get-invitation-by-ID. One call for a typical org, capped at 25 pages with a truncation warning. Same pattern the existinglookupPendingInvitationuses.golangci-linthit (G115onrepository.gouint32(i)) is pre-existing onmain; left alone rather than folded into this PR.Testing
go test ./...andgolangci-lint runpass. New table of cases ininvitation_grants_test.gocovers, against a mocked GitHub: grant sync for all three resources (including role resolution, case-insensitive login correlation, expired-invitation and unmatched-invitee skips), the login grant path, the refusal-vs-missing-login error distinction,GrantAlreadyExistson an already-attached team, the no-longer-pending failure, reissue preserving pre-existing teams, reissue byinvitee_idfor known logins, and the restore-on-failure path.Three pre-existing tests changed only their exact
NextPageTokenassertions to account for the new pagination state.