docs(auth): document fine-grained PAT permission for org publishing (+ pagination-cap test) (#1435)
Follow-up to #1383, surfaced while validating that change end-to-end against staging. ## Why #1383 hardened org-namespace publishing to require Owner (`admin`) role, read via `GET /user/memberships/orgs`. Validating on staging revealed a gap: the docs only mention the classic `read:org` scope, but a **fine-grained PAT** (GitHub's recommended token type) needs a *different* permission to expose org membership. Without it, GitHub returns no org memberships for the token, so the exchange silently grants only the personal namespace — with no hint why org publishing "doesn't work." This is **not** a code bug (the behavior is fail-closed and correct given GitHub's response); it's a documentation/UX gap. ## What - **Docs** (`authentication.mdx`, `github-actions.mdx`): spell out both token types — - Classic PAT → `read:org` scope - Fine-grained PAT → **Organization permissions → Members → Read-only** (the equivalent), noting a fine-grained PAT is bound to a single resource owner. - Also clarified `read:user` is not required. - **Test** (`github_at_test.go`): cover the `maxOrgMembershipPages` pagination cap — when every page is full, the exchange **fails closed** (no token) rather than returning a possibly-truncated org grant. This path was previously untested. - **Comment** (`github_at.go`): qualify the `X-GitHub-SSO` 403 check as best-effort — we have not confirmed the memberships *list* endpoint emits that header on SSO enforcement (it may just omit the org). The check is safe either way, but the comment no longer overstates it as a guarantee. ## Verification (live, against staging) Confirmed by exchanging real tokens for `rdimitrov` on `staging.registry.modelcontextprotocol.io`: - **Classic `read:org` PAT** across 16 real org memberships → the JWT granted exactly the 8 **admin** orgs + personal namespace, and **zero** of the 8 **member** orgs (incl. `stacklok`, `modelcontextprotocol`) — the core #1383 behavior, working. - **Fine-grained PAT**: without Members:read → personal namespace only (fail-closed); after adding Members:read → the org (Owner) namespace is granted. This is what the new docs describe. No runtime behavior change here beyond the added test; the docs and comment are the substance. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
5da9b4c8bf
commit
9e762b03c7
2 changed files with 48 additions and 6 deletions
|
|
@ -255,10 +255,15 @@ func (h *GitHubHandler) fetchOrgMembershipsPage(ctx context.Context, token strin
|
|||
if resp.Header.Get("X-RateLimit-Remaining") == "0" || resp.Header.Get("Retry-After") != "" {
|
||||
return nil, fmt.Errorf("GitHub API rate limit exceeded while listing org memberships (status 403): %s", readErrorBody(resp.Body))
|
||||
}
|
||||
// A SAML/SSO-enforced org returns 403 with an X-GitHub-SSO header when the
|
||||
// token has not been SSO-authorized. That is an Owner being blocked, not a
|
||||
// missing scope, so fail closed rather than silently dropping the org grant
|
||||
// (which would degrade a legitimate Owner to personal-only with no signal).
|
||||
// Best-effort defense: GitHub uses the X-GitHub-SSO header to signal that a
|
||||
// resource is behind SAML/SSO the token hasn't been authorized for. If we see
|
||||
// it, treat that as an Owner being blocked (not a missing scope) and fail
|
||||
// closed rather than silently degrading them to personal-only. NOTE: we have
|
||||
// not confirmed that the memberships *list* endpoint emits this header on SSO
|
||||
// enforcement (it may instead just omit the SSO-protected org); this check is
|
||||
// safe either way — if the header is absent we fall through to the same
|
||||
// graceful degrade as before — but it is not a proven guarantee that every
|
||||
// SSO-blocked Owner is caught here.
|
||||
if resp.Header.Get("X-GitHub-SSO") != "" {
|
||||
return nil, fmt.Errorf("GitHub org memberships require SSO authorization for this token (status 403): %s", readErrorBody(resp.Body))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -665,6 +665,34 @@ func TestGitHubHandler_ExchangeToken_403FailClosed(t *testing.T) {
|
|||
})
|
||||
}
|
||||
|
||||
func TestGitHubHandler_ExchangeToken_PaginationCap(t *testing.T) {
|
||||
testSeed := make([]byte, ed25519.SeedSize)
|
||||
_, err := rand.Read(testSeed)
|
||||
require.NoError(t, err)
|
||||
|
||||
cfg := &config.Config{JWTPrivateKey: hex.EncodeToString(testSeed)}
|
||||
|
||||
t.Run("all-full pages past the cap fails closed", func(t *testing.T) {
|
||||
// Every page comes back full (100), so the loop never sees a short final page
|
||||
// and runs to maxOrgMembershipPages. Rather than return a possibly-truncated
|
||||
// org set, the exchange must fail closed (error, no token).
|
||||
server := newMockGitHubServer(func(w http.ResponseWriter, _ *http.Request) {
|
||||
page := make([]orgMembership, 0, 100)
|
||||
for i := 0; i < 100; i++ {
|
||||
page = append(page, orgMembership{
|
||||
State: "active", Role: "member",
|
||||
Organization: v0auth.GitHubUserOrOrg{Login: fmt.Sprintf("org-%d", i), ID: i},
|
||||
})
|
||||
}
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(page) //nolint:errcheck
|
||||
})
|
||||
defer server.Close()
|
||||
|
||||
assertExchangeFailsClosed(t, cfg, server)
|
||||
})
|
||||
}
|
||||
|
||||
func TestJWTTokenValidation(t *testing.T) {
|
||||
testSeed := make([]byte, ed25519.SeedSize)
|
||||
_, err := rand.Read(testSeed)
|
||||
|
|
|
|||
Loading…
Reference in a new issue