diff --git a/README.md b/README.md index 43bee07..f52fac5 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ The plugin has no HTTP routes of its own: core calls the internal-only `POST /_g Migration `0002_git_provider.sql` moves the previous release's data into core and drops its tables (`github_integrations`, `github_repositories`, `github_pull_requests`, `github_task_branches`, `github_task_pr_links`): - Each project's access token becomes a **project-scoped token connection** (the ciphertext is copied as is, so the same `ENCRYPTION_KEY` must be in place), with its linked repositories, branch links and pull request links. -- The token's owner login and a webhook secret are not known to the migration; they are filled in the first time the token is used (GitHub is asked who owns it, a secret is generated). Until then the connection shows no `@handle`. +- The token's owner login and a webhook secret are not known to the migration; they are filled in the first time the token is used (GitHub is asked who owns it, a secret is generated). Until then the connection shows no `@handle`. The provider implements `identity_description`: the first time Paca lists connections it asks the provider who each handle-less connection is, and the owner's login (and so the connection name and `@handle`) and the token's expiry, if it has one, appear automatically, without anyone using the token first. This needs a Paca core with `identity_description` support; with an older core the handle still appears on first use of the token. If GitHub rejects the token or cannot be reached, the connection keeps the generic name *GitHub access token* with no `@handle`, and Paca asks again 15 minutes later (or after a restart). A rejected token stays nameless until the connection is replaced with a new token. - The webhooks the old release registered on GitHub pointed at the old plugin and are left there. Open the project's Git settings and press *Try again* on each repository to register a new one (the token needs the webhook permission), and delete the old hooks on GitHub. ### Build diff --git a/backend/go.mod b/backend/go.mod index f943c7f..4ce946e 100644 --- a/backend/go.mod +++ b/backend/go.mod @@ -2,6 +2,4 @@ module github.com/Paca-AI/first-party/github go 1.24 -require github.com/Paca-AI/plugin-sdk-go v0.4.1 - -replace github.com/Paca-AI/plugin-sdk-go => github.com/kayahk/plugin-sdk-go v0.0.0-20261102180211-0a7e5c7287f9 +require github.com/Paca-AI/plugin-sdk-go v0.4.2 diff --git a/backend/go.sum b/backend/go.sum index 6d49666..481df2d 100644 --- a/backend/go.sum +++ b/backend/go.sum @@ -1,2 +1,2 @@ -github.com/kayahk/plugin-sdk-go v0.0.0-20261102180211-0a7e5c7287f9 h1:OKM/oY0vR88PGi7LnZaGId2hHYroqt9BS63hmQbRS2Y= -github.com/kayahk/plugin-sdk-go v0.0.0-20261102180211-0a7e5c7287f9/go.mod h1:5WeC6cSEf2wM1ovICZbDaVky9oi5id/Qpdfc5LDAQnw= +github.com/Paca-AI/plugin-sdk-go v0.4.2 h1:J9Jz7HorscHip2pRHNPepKjtYfKkSosJ2TmGtWWJyC8= +github.com/Paca-AI/plugin-sdk-go v0.4.2/go.mod h1:5WeC6cSEf2wM1ovICZbDaVky9oi5id/Qpdfc5LDAQnw= diff --git a/backend/spi_assert.go b/backend/spi_assert.go index 333f5bc..8f1e50b 100644 --- a/backend/spi_assert.go +++ b/backend/spi_assert.go @@ -15,6 +15,7 @@ var ( _ gp.AssigneeSetter = (*ghProvider)(nil) _ gp.Merger = (*ghProvider)(nil) _ gp.IdentityDeleter = (*ghProvider)(nil) + _ gp.IdentityDescriber = (*ghProvider)(nil) _ gp.WebhookManager = (*ghProvider)(nil) _ gp.AccessGranter = (*ghProvider)(nil) ) diff --git a/backend/spi_provider.go b/backend/spi_provider.go index 9ef8b21..b9d2e95 100644 --- a/backend/spi_provider.go +++ b/backend/spi_provider.go @@ -63,17 +63,18 @@ func (p *ghProvider) Describe() gp.Info { {Kind: gp.IdentityToken, CredentialModel: gp.CredentialStatic, WebhookScope: gp.WebhookRepo}, }, - ResolveThreads: true, - FileRead: true, - IssueList: true, - Labels: true, - ReviewerRequests: true, - Assignees: true, - Merge: true, - IdentityDeletion: true, - WebhookManagement: true, - Branches: true, - AccessGrants: true, + ResolveThreads: true, + FileRead: true, + IssueList: true, + Labels: true, + ReviewerRequests: true, + Assignees: true, + Merge: true, + IdentityDeletion: true, + IdentityDescription: true, + WebhookManagement: true, + Branches: true, + AccessGrants: true, BatchReview: true, HiddenMarkers: true, diff --git a/backend/spi_test.go b/backend/spi_test.go index 55d94cd..e712ec4 100644 --- a/backend/spi_test.go +++ b/backend/spi_test.go @@ -793,3 +793,18 @@ func TestClaimSetupCodeIsSingleUse(t *testing.T) { t.Error("second claim succeeded") } } + +// An App is described from what is stored: the slug is its handle. +func TestDescribeIdentity_App(t *testing.T) { + p, _, _ := seededProvider(t) + id, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: fakeAppRowID}}) + if err != nil { + t.Fatal(err) + } + if id.Ref.ID != fakeAppRowID || id.Ref.Handle != fakeSlug || id.Kind != gp.IdentityApp || id.Host != githubHost || id.DisplayName != "Paca" { + t.Fatalf("identity = %+v", id) + } + if !p.Describe().Capabilities.IdentityDescription { + t.Error("identity_description must be declared") + } +} diff --git a/backend/spi_token.go b/backend/spi_token.go index 7293629..1feb745 100644 --- a/backend/spi_token.go +++ b/backend/spi_token.go @@ -104,6 +104,68 @@ func (p *ghProvider) completeMigratedToken(t *tokenRecord) error { return nil } +// DescribeIdentity implements gp.IdentityDescriber: who an existing identity is, +// without a setup. Core asks for identities it holds with no handle, such as the +// token connections the 0002 migration created, and keeps the answer, so the +// expiry of a token has to come from here. +// +// An App is described from what is stored, with no API call. A token is always +// checked against GitHub (one GET /user): that gives the owner's login and name, +// the expiry GitHub reports for tokens that have one, and tells a revoked token +// (forbidden). A token migrated from the previous release is completed from the +// same response (blank login filled, webhook secret generated, both stored). The +// store only fills blanks, so a later rename on GitHub is reported here but not +// written back. Unknown ids are not_found. +func (p *ghProvider) DescribeIdentity(_ context.Context, req gp.DescribeIdentityRequest) (gp.Identity, error) { + id := req.Identity.ID + if strings.TrimSpace(id) == "" { + return gp.Identity{}, notFoundIdentity() + } + app, err := p.loadApp(id) + if err == nil { + return identityOf(app), nil + } + if gp.AsError(err).Code != gp.CodeNotFound { + return gp.Identity{}, err + } + tok, terr := p.store.loadToken(id) + switch terr { + case nil: + case errNoSuchToken: + return gp.Identity{}, err + default: + return gp.Identity{}, gp.Errorf(gp.CodeInternal, "could not load the GitHub identity") + } + var u ghAuthUser + status, text, headers, err := (&ghAPI{token: tok.Token}).doRaw("GET", "/user", nil, "") + if err != nil { + return gp.Identity{}, err + } + if status >= 400 { + return gp.Identity{}, mapGitHubError(status, text, headers) + } + if err := decodeInto(text, &u); err != nil { + return gp.Identity{}, err + } + if u.Login == "" { + return gp.Identity{}, gp.Errorf(gp.CodeInternal, "GitHub returned an incomplete user") + } + if tok.Login == "" || tok.WebhookSecret == "" { + // Hand the answer over so completing does not ask GitHub again. + if tok.Login == "" { + tok.Login, tok.Name = u.Login, u.Name + } + if err := p.completeMigratedToken(&tok); err != nil { + return gp.Identity{}, err + } + } + ident := identityOfToken(tokenRecord{ID: tok.ID, Login: u.Login, Name: u.Name}) + if exp := tokenExpiry(headers); !exp.IsZero() { + ident.ExpiresAt = exp + } + return ident, nil +} + // loadTokenIdentity is loadIdentity for operations that only a token supports. func (p *ghProvider) loadTokenIdentity(id string) (tokenRecord, error) { ident, err := p.loadIdentity(id) diff --git a/backend/spi_token_test.go b/backend/spi_token_test.go index f41e32c..a21cd7a 100644 --- a/backend/spi_token_test.go +++ b/backend/spi_token_test.go @@ -8,6 +8,7 @@ import ( "strings" "sync" "testing" + "time" plugin "github.com/Paca-AI/plugin-sdk-go" gp "github.com/Paca-AI/plugin-sdk-go/gitprovider" @@ -383,3 +384,129 @@ func TestTokenSetup_GeneratesTheWebhookSecretWhenLeftEmpty(t *testing.T) { t.Fatalf("generated secret = %q", got) } } + +// DescribeIdentity completes a migrated token (login filled and persisted) and +// asks GitHub only that once. +func TestDescribeIdentity_MigratedTokenGetsItsHandle(t *testing.T) { + p, g, st := tokenProvider(t) + st.tokens["legacy"] = tokenRecord{ID: "legacy", Token: patSecret} + + id, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: "legacy"}}) + if err != nil { + t.Fatal(err) + } + if id.Ref.ID != "legacy" || id.Ref.Handle != patLogin || id.Kind != gp.IdentityToken || id.Host != githubHost || id.DisplayName != "Paca Machine ("+patLogin+")" { + t.Fatalf("identity = %+v", id) + } + if got := st.tokens["legacy"]; got.Login != patLogin || len(got.WebhookSecret) < minWebhookSecretLen { + t.Fatalf("the completion must be stored: %+v", got) + } + if _, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: id.Ref}); err != nil || g.called("GET /user") != 2 { + t.Fatalf("each description makes one GET /user, none for completing (err %v, calls %d)", err, g.called("GET /user")) + } +} + +// A token that is already complete is still checked once: the answer carries the +// owner and the expiry, and core keeps it. +func TestDescribeIdentity_StoredTokenAsksGitHubOnce(t *testing.T) { + p, g, _ := tokenProvider(t) + id, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: patIdentity}}) + if err != nil { + t.Fatal(err) + } + if id.Ref.Handle != patLogin || id.Kind != gp.IdentityToken || id.DisplayName == "" || !id.ExpiresAt.IsZero() { + t.Fatalf("identity = %+v", id) + } + if n := g.called("GET /user"); n != 1 || len(g.calls) != 1 { + t.Errorf("calls = %v, want exactly one GET /user", g.calls) + } + if strings.Contains(mustJSON(t, id), patSecret) { + t.Error("the token leaked") + } +} + +func TestDescribeIdentity_ExpiryComesFromGitHub(t *testing.T) { + p, g, _ := tokenProvider(t) + g.expiry = "2026-11-30 10:00:00 UTC" + id, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: patIdentity}}) + if err != nil { + t.Fatal(err) + } + if want := time.Date(2026, 11, 30, 10, 0, 0, 0, time.UTC); !id.ExpiresAt.Equal(want) { + t.Errorf("ExpiresAt = %v, want %v", id.ExpiresAt, want) + } +} + +func TestDescribeIdentity_RevokedStoredTokenIsForbidden(t *testing.T) { + p, _, st := tokenProvider(t) + st.tokens["old"] = tokenRecord{ID: "old", Login: patLogin, Token: "ghp_revoked", WebhookSecret: patHookKey} + if _, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: "old"}}); !hasCode(err, gp.CodeForbidden) { + t.Fatalf("err = %v, want forbidden", err) + } +} + +func TestDescribeIdentity_UnknownIsNotFound(t *testing.T) { + p, _, _ := tokenProvider(t) + for _, id := range []string{"nope", ""} { + if _, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: id}}); !hasCode(err, gp.CodeNotFound) { + t.Errorf("id %q: err = %v, want not_found", id, err) + } + } +} + +// A migrated token GitHub rejects cannot be completed: forbidden, nothing stored. +func TestDescribeIdentity_RevokedMigratedTokenIsForbidden(t *testing.T) { + p, _, st := tokenProvider(t) + st.tokens["legacy"] = tokenRecord{ID: "legacy", Token: "ghp_revoked"} + _, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: "legacy"}}) + if !hasCode(err, gp.CodeForbidden) { + t.Fatalf("err = %v, want forbidden", err) + } + if st.tokens["legacy"].Login != "" { + t.Error("nothing must be stored for a rejected token") + } +} + +// Whatever goes wrong while describing a token, the error handed back to core +// must not carry the token, even when GitHub or the transport echoes it. +func TestDescribeIdentity_ErrorsNeverCarryTheToken(t *testing.T) { + echo := func(status int) func(string, string, map[string]string, string) (*plugin.FetchResponse, error) { + return func(_, _ string, h map[string]string, _ string) (*plugin.FetchResponse, error) { + body, _ := json.Marshal(map[string]string{"message": "bad token " + patSecret}) + return &plugin.FetchResponse{Status: status, Body: string(body), Headers: map[string]string{"x-echo": h["Authorization"]}}, nil + } + } + for name, f := range map[string]func(string, string, map[string]string, string) (*plugin.FetchResponse, error){ + "401 rejected": echo(401), + "500 upstream": echo(500), + "transport error": func(string, string, map[string]string, string) (*plugin.FetchResponse, error) { + return nil, fmt.Errorf("dial failed for %s", patSecret) + }, + "unreadable body": func(string, string, map[string]string, string) (*plugin.FetchResponse, error) { + return &plugin.FetchResponse{Status: 200, Body: "not json " + patSecret, Headers: map[string]string{}}, nil + }, + } { + t.Run(name, func(t *testing.T) { + p, _, _ := tokenProvider(t) + old := fetch + fetch = f + t.Cleanup(func() { fetch = old }) + _, err := p.DescribeIdentity(bg, gp.DescribeIdentityRequest{Identity: gp.IdentityRef{ID: patIdentity}}) + if err == nil { + t.Fatal("want an error") + } + if got := mustJSON(t, gp.AsError(err)) + err.Error(); strings.Contains(got, patSecret) || strings.Contains(got, "Bearer") { + t.Errorf("the error carries the token: %s", got) + } + }) + } +} + +func mustJSON(t *testing.T, v any) string { + t.Helper() + b, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + return string(b) +}