diff --git a/docs/ai-agent/git-agent-workflow.md b/docs/ai-agent/git-agent-workflow.md index f167d298c..2f7f54fcd 100644 --- a/docs/ai-agent/git-agent-workflow.md +++ b/docs/ai-agent/git-agent-workflow.md @@ -176,7 +176,7 @@ Declared in `Describe()`. Core's UI, tools and router read them instead of hard- | `webhook_scope` | `app`, `group`, `repo` | Whether core asks the provider to `EnsureWebhook` per linked repository. | | `triggers` | subset of `mention, label, review_request, assignment, ci_failure, own_cr_feedback, auto_review` | Which trigger switches are offered on an agent. | | `status_kind` | `check_run`, `commit_status`, `comment` | How run status is shown. | -| `branches`, `resolve_threads`, `file_read`, `code_search`, `labels`, `reviewer_requests`, `assignees`, `merge`, `rerun_checks`, `reactions`, `webhook_management`, `identity_deletion`, `access_grants` | bool | The matching optional operation exists; tools and screens that need it appear only then. | +| `branches`, `resolve_threads`, `file_read`, `code_search`, `labels`, `reviewer_requests`, `assignees`, `merge`, `rerun_checks`, `reactions`, `webhook_management`, `identity_deletion`, `identity_description`, `access_grants` | bool | The matching optional operation exists; tools and screens that need it appear only then. | | `batch_review`, `idempotent_writes`, `hidden_markers`, `fork_change_requests`, `draft_change_requests`, `nested_paths` | bool | Behaviour of required operations. | | `ci_config_globs` | paths | Files agents must not modify (§12.3). | diff --git a/docs/plugins/git-provider-spi.md b/docs/plugins/git-provider-spi.md index 831c88362..fe65bb153 100644 --- a/docs/plugins/git-provider-spi.md +++ b/docs/plugins/git-provider-spi.md @@ -42,6 +42,7 @@ func main() { plugin.Run(&myPlugin{}) } | `Merger`, `CheckRerunner`, `Reactor` | `merge`, `rerun_checks`, `reactions` | | `WebhookManager` | `webhook_management` (repo/group-scoped webhooks) | | `IdentityDeleter` | `identity_deletion` | +| `IdentityDescriber` (`DescribeIdentity`) | `identity_description` (see below) | | `BranchManager` (`GetBranch`, `CreateBranch`) | `branches` | | `AccessGranter` | `access_grants` (see below) | @@ -77,6 +78,16 @@ The SDK ships a conformance suite (`plugin-sdk-go/gitprovider/conformance`) and Trigger configuration, routing, actor policy, run lifecycle, debounce, comment/thread ownership, status comments, audit, MCP tools, workspace protocol, credential-helper plumbing, task linking and every screen except your identity setup component. If you find yourself writing one of those, the SPI is missing something — raise it. +## Identity description (optional) + +`DescribeIdentity(ctx, DescribeIdentityRequest{Identity})` says who an existing identity is, without a setup flow. It exists for identities core holds with no handle: a plugin's data migration (for example from an older plugin version or a community plugin) creates `git_identities` rows without calling the platform, so it cannot know the token's user. Such a row would stay nameless in Settings → Git and `@mentions` would not match it. + +* **When core calls it**: while listing connections (a project's Settings → Git) or the workspace's shared identities, for each identity whose stored handle is empty and whose provider declares `identity_description`. Once per identity, because the answer is stored; an identity that already has a handle is never described. +* **What you return**: an `Identity` whose `Ref.ID` equals the request's, with `Ref.Handle` set (the login users `@mention`), `DisplayName`, and `ExpiresAt` for credentials that expire. `Next` is ignored. Unknown identity: `not_found`; credentials the platform no longer accepts: `forbidden`. +* **What core stores**: the handle, and the display name and expiry when non-empty. An answer with another `Ref.ID` or an empty handle is discarded. +* **Failure**: best effort. Core bounds all the calls of one listing to 5 s in total (not per call), logs the error without secrets and serves the listing with the old values. After a failure, or an answer it cannot use (another `Ref.ID`, empty handle), core does not ask about that identity again for 15 minutes (in memory, reset on restart), then retries. A request the client cancels is not counted as a failure. +* **For migrations**: leave `handle` empty (and a generic `display_name`) in the SQL and implement `DescribeIdentity` by calling the platform's "current user" endpoint with the stored credential. + ## Access grants (optional) Some platforms only let an identity see the repositories of accounts it has been granted access to, and a *user* grants that on the platform. GitHub is the example: a GitHub App sees only the accounts and repositories it is installed on. Because each project can use a repository owned by a different user or organization, this cannot happen once, when the identity is created. diff --git a/services/api/internal/domain/git/repository.go b/services/api/internal/domain/git/repository.go index e10856f80..9352e3ec6 100644 --- a/services/api/internal/domain/git/repository.go +++ b/services/api/internal/domain/git/repository.go @@ -21,6 +21,12 @@ type Store interface { SetAgentIdentity(ctx context.Context, agentID uuid.UUID, providerID string, identityID uuid.UUID) error FindAgentIdentity(ctx context.Context, agentID uuid.UUID, providerID string) (*Identity, error) DeleteIdentity(ctx context.Context, id uuid.UUID) error + // UpdateIdentityProfile fills in who an identity is on the platform. An + // empty displayName and a nil expiresAt leave the stored values as they are. + // It only touches an identity that has no handle yet, so a handle set in + // the meantime (a reconnect) is never overwritten; an identity that is gone + // or already named is a silent no-op. + UpdateIdentityProfile(ctx context.Context, id uuid.UUID, handle, displayName string, expiresAt *time.Time) error // DeleteAgentIdentity removes an agent's mapping (not the identity itself). DeleteAgentIdentity(ctx context.Context, agentID uuid.UUID, providerID string) error diff --git a/services/api/internal/platform/gitprovider/client.go b/services/api/internal/platform/gitprovider/client.go index a42524905..d038e8a2f 100644 --- a/services/api/internal/platform/gitprovider/client.go +++ b/services/api/internal/platform/gitprovider/client.go @@ -124,6 +124,11 @@ func (c *Client) RemoveWebhook(ctx context.Context, req RemoveWebhookRequest) er return err } +// DescribeIdentity says who an existing identity is (capability identity_description). +func (c *Client) DescribeIdentity(ctx context.Context, req DescribeIdentityRequest) (Identity, error) { + return call[DescribeIdentityRequest, Identity](ctx, c, "describe_identity", req) +} + // DeleteIdentity asks the provider to forget an identity remotely. func (c *Client) DeleteIdentity(ctx context.Context, req DeleteIdentityRequest) error { _, err := call[DeleteIdentityRequest, Empty](ctx, c, "delete_identity", req) diff --git a/services/api/internal/platform/gitprovider/client_test.go b/services/api/internal/platform/gitprovider/client_test.go index 3bb02fcca..3279df65d 100644 --- a/services/api/internal/platform/gitprovider/client_test.go +++ b/services/api/internal/platform/gitprovider/client_test.go @@ -142,3 +142,23 @@ func TestClient_AccessGrantOperations(t *testing.T) { t.Errorf("a provider without grants answers unsupported: %v", err) } } + +func TestClient_DescribeIdentity(t *testing.T) { + d := &fakeDoer{status: 200, resp: `{"ok":true,"result":{"ref":{"id":"i1","handle":"bot"},"display_name":"Bot"}}`} + c := NewClient(d, "com.paca.gitlab") + id, err := c.DescribeIdentity(context.Background(), DescribeIdentityRequest{Identity: IdentityRef{ID: "i1"}}) + if err != nil || id.Ref.ID != "i1" || id.Ref.Handle != "bot" || id.DisplayName != "Bot" { + t.Fatalf("describe: %+v %v", id, err) + } + if d.gotURL != "com.paca.gitlab POST /_git/describe_identity" { + t.Errorf("called %q", d.gotURL) + } + var sent DescribeIdentityRequest + if json.Unmarshal(d.gotReq, &sent) != nil || sent.Identity.ID != "i1" { + t.Errorf("request body %s", d.gotReq) + } + d.status, d.resp = 404, `{"ok":false,"error":{"code":"not_found","message":"gone"}}` + if _, err := c.DescribeIdentity(context.Background(), DescribeIdentityRequest{}); !IsCode(err, CodeNotFound) { + t.Errorf("unknown identity is not_found: %v", err) + } +} diff --git a/services/api/internal/platform/gitprovider/spi_types.go b/services/api/internal/platform/gitprovider/spi_types.go index b8876791a..4dd841f00 100644 --- a/services/api/internal/platform/gitprovider/spi_types.go +++ b/services/api/internal/platform/gitprovider/spi_types.go @@ -225,6 +225,11 @@ type Capabilities struct { Reactions bool `json:"reactions"` WebhookManagement bool `json:"webhook_management"` IdentityDeletion bool `json:"identity_deletion"` + // IdentityDescription is true when the provider can say who an existing + // identity is (handle, display name, expiry) without a setup flow. Core + // uses it to fill in identities it holds without a handle, such as those + // created by a plugin's data migration. + IdentityDescription bool `json:"identity_description"` // Branches is true when branches can be looked up and created through the // provider (core uses it to link and create a task's branch). Branches bool `json:"branches"` @@ -379,6 +384,13 @@ type DeleteIdentityRequest struct { Identity IdentityRef `json:"identity"` } +// DescribeIdentityRequest asks who an existing identity is (capability +// IdentityDescription). Identity.Handle may be empty: that is the case the +// operation exists for. +type DescribeIdentityRequest struct { + Identity IdentityRef `json:"identity"` +} + // ── Repositories ───────────────────────────────────────────────────────────── // Repository is a repository an identity can see. diff --git a/services/api/internal/platform/gitprovider/spi_wiretypes.go b/services/api/internal/platform/gitprovider/spi_wiretypes.go index 093bee8e7..6a2a67f0a 100644 --- a/services/api/internal/platform/gitprovider/spi_wiretypes.go +++ b/services/api/internal/platform/gitprovider/spi_wiretypes.go @@ -16,8 +16,8 @@ func WireTypes() map[string]reflect.Type { "Info": t(Info{}), "Capabilities": t(Capabilities{}), "Terms": t(Terms{}), "BeginSetupRequest": t(BeginSetupRequest{}), "SetupStep": t(SetupStep{}), "FormField": t(FormField{}), "CompleteSetupRequest": t(CompleteSetupRequest{}), "Identity": t(Identity{}), "IdentityRef": t(IdentityRef{}), - "DeleteIdentityRequest": t(DeleteIdentityRequest{}), - "BeginAccessRequest": t(BeginAccessRequest{}), "CompleteAccessRequest": t(CompleteAccessRequest{}), "AccessGrant": t(AccessGrant{}), + "DeleteIdentityRequest": t(DeleteIdentityRequest{}), "DescribeIdentityRequest": t(DescribeIdentityRequest{}), + "BeginAccessRequest": t(BeginAccessRequest{}), "CompleteAccessRequest": t(CompleteAccessRequest{}), "AccessGrant": t(AccessGrant{}), "RepoRef": t(RepoRef{}), "Target": t(Target{}), "SubjectRef": t(SubjectRef{}), "Repository": t(Repository{}), "ListReposRequest": t(ListReposRequest{}), "RepositoryPage": t(RepositoryPage{}), "ResolveRepoRequest": t(ResolveRepoRequest{}), "ResolvedRepository": t(ResolvedRepository{}), diff --git a/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json b/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json index 3e22dcd94..fdae5d5f0 100644 --- a/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json +++ b/services/api/internal/platform/gitprovider/testdata/wire-shapes.golden.json @@ -122,6 +122,7 @@ "hidden_markers": "bool", "idempotent_writes": "bool", "identity_deletion": "bool", + "identity_description": "bool", "identity_kinds": [ "string" ], @@ -378,6 +379,13 @@ "provider_ref": "bytes" } }, + "DescribeIdentityRequest": { + "identity": { + "handle": "string", + "id": "string", + "provider_ref": "bytes" + } + }, "Diff": { "text": "string", "truncated": "bool" @@ -610,6 +618,7 @@ "hidden_markers": "bool", "idempotent_writes": "bool", "identity_deletion": "bool", + "identity_description": "bool", "identity_kinds": [ "string" ], diff --git a/services/api/internal/repository/postgres/git_repository.go b/services/api/internal/repository/postgres/git_repository.go index 2998bb0eb..ed13cb9d7 100644 --- a/services/api/internal/repository/postgres/git_repository.go +++ b/services/api/internal/repository/postgres/git_repository.go @@ -149,6 +149,24 @@ func (s *GitRepository) CreateIdentity(ctx context.Context, i *gitdom.Identity) return nil } +// UpdateIdentityProfile stores the handle, and when given the display name and +// expiry, the platform reported for an identity. The handle = ” guard keeps a +// handle set meanwhile (a reconnect) from being overwritten; zero rows +// (deleted or already named) is not an error. +func (s *GitRepository) UpdateIdentityProfile(ctx context.Context, id uuid.UUID, handle, displayName string, expiresAt *time.Time) error { + _, err := s.db.ExecContext(ctx, ` + UPDATE git_identities + SET handle = $2, + display_name = CASE WHEN $3 <> '' THEN $3 ELSE display_name END, + expires_at = COALESCE($4, expires_at), + updated_at = NOW() + WHERE id = $1 AND handle = ''`, id.String(), handle, displayName, expiresAt) + if err != nil { + return fmt.Errorf("git repo: update identity profile: %w", err) + } + return nil +} + func (s *GitRepository) getIdentity(ctx context.Context, where string, args ...any) (*gitdom.Identity, error) { var rec gitIdentityRecord if err := s.db.GetContext(ctx, &rec, `SELECT `+gitIdentityCols+` FROM git_identities WHERE `+where, args...); err != nil { diff --git a/services/api/internal/repository/postgres/git_repository_test.go b/services/api/internal/repository/postgres/git_repository_test.go index 517892205..4091c28d4 100644 --- a/services/api/internal/repository/postgres/git_repository_test.go +++ b/services/api/internal/repository/postgres/git_repository_test.go @@ -861,3 +861,46 @@ func TestGitStore_DeletingAConnectionLeavesItsRepositoriesToBeRebound(t *testing t.Fatalf("unknown repository: %v", err) } } + +func TestGitStore_UpdateIdentityProfile(t *testing.T) { + s, _ := newGitFx(t) + ctx := t.Context() + exp := time.Now().Add(48 * time.Hour).UTC().Truncate(time.Microsecond) + i := sharedIdentity("gitlab.com") + i.Handle, i.DisplayName = "", "GitLab connection" + if err := s.CreateIdentity(ctx, i); err != nil { + t.Fatal(err) + } + if err := s.UpdateIdentityProfile(ctx, i.ID, "paca-bot", "Paca Bot", &exp); err != nil { + t.Fatalf("update: %v", err) + } + got, err := s.FindIdentity(ctx, i.ID) + if err != nil || got.Handle != "paca-bot" || got.DisplayName != "Paca Bot" || got.ExpiresAt == nil || !got.ExpiresAt.Equal(exp) { + t.Fatalf("after update: %+v %v", got, err) + } + // A row that already has a handle is left unchanged. + if err := s.UpdateIdentityProfile(ctx, i.ID, "renamed", "Other", nil); err != nil { + t.Fatal(err) + } + got, _ = s.FindIdentity(ctx, i.ID) + if got.Handle != "paca-bot" || got.DisplayName != "Paca Bot" || got.ExpiresAt == nil || !got.ExpiresAt.Equal(exp) { + t.Errorf("a named identity was overwritten: %+v", got) + } + // An empty name and no expiry keep what is stored. + j := sharedIdentity("gitlab.com") + j.ProviderIdentityID, j.Handle, j.DisplayName = "tok-2", "", "GitLab connection" + if err := s.CreateIdentity(ctx, j); err != nil { + t.Fatal(err) + } + if err := s.UpdateIdentityProfile(ctx, j.ID, "bot2", "", nil); err != nil { + t.Fatal(err) + } + got, _ = s.FindIdentity(ctx, j.ID) + if got.Handle != "bot2" || got.DisplayName != "GitLab connection" || got.ExpiresAt != nil { + t.Errorf("empty values overwrote: %+v", got) + } + // An unknown identity is a no-op. + if err := s.UpdateIdentityProfile(ctx, uuid.New(), "x", "", nil); err != nil { + t.Errorf("unknown identity: %v", err) + } +} diff --git a/services/api/internal/service/git/describe_test.go b/services/api/internal/service/git/describe_test.go new file mode 100644 index 000000000..338fb4160 --- /dev/null +++ b/services/api/internal/service/git/describe_test.go @@ -0,0 +1,215 @@ +package gitsvc + +import ( + "bytes" + "context" + "errors" + "log/slog" + "strings" + "testing" + "time" + + "github.com/google/uuid" + + gitdom "github.com/Paca-AI/api/internal/domain/git" + "github.com/Paca-AI/api/internal/platform/gitprovider" +) + +func describingSetup(t *testing.T, capable bool) (*Service, *fakeStore, *fakeProv) { + t.Helper() + svc, st, pr := setup(t) + pr.info.ID = "p" + pr.info.Capabilities.IdentityDescription = capable + return svc, st, pr +} + +func TestDescribeNameless_FillsHandleAndPersists(t *testing.T) { + svc, st, pr := describingSetup(t, true) + exp := time.Now().Add(24 * time.Hour).UTC() + id := st.addIdentity("", "tok-1") + pr.describe = func(r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + return gitprovider.Identity{Ref: gitprovider.IdentityRef{ID: r.Identity.ID, Handle: "paca-bot"}, DisplayName: "Paca Bot", ExpiresAt: exp}, nil + } + for name, list := range map[string]func() ([]*gitdom.Identity, error){ + "identities": func() ([]*gitdom.Identity, error) { return svc.ListIdentities(context.Background()) }, + "connections": func() ([]*gitdom.Identity, error) { return svc.ListConnections(context.Background(), uuid.New()) }, + } { + id.Handle, id.DisplayName, id.ExpiresAt = "", "", nil + pr.describeLog = nil + got, err := list() + if err != nil || len(got) != 1 { + t.Fatalf("%s: %v %v", name, got, err) + } + if got[0].Handle != "paca-bot" || got[0].DisplayName != "Paca Bot" || got[0].ExpiresAt == nil || !got[0].ExpiresAt.Equal(exp) { + t.Errorf("%s: response not filled: %+v", name, got[0]) + } + if len(pr.describeLog) != 1 || pr.describeLog[0].Identity.ID != "tok-1" { + t.Errorf("%s: describe calls = %+v", name, pr.describeLog) + } + } + if st.profileUpdates != 2 { + t.Errorf("profile saved %d times, want 2", st.profileUpdates) + } + // Once named, a later listing does not ask again. + pr.describeLog = nil + if _, err := svc.ListIdentities(context.Background()); err != nil || len(pr.describeLog) != 0 { + t.Errorf("named identity described again: %v %d", err, len(pr.describeLog)) + } +} + +func TestDescribeNameless_ProviderErrorKeepsRequestOK(t *testing.T) { + svc, st, pr := describingSetup(t, true) + id := st.addIdentity("", "tok-1") + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + return gitprovider.Identity{}, errors.New("platform down") + } + got, err := svc.ListIdentities(context.Background()) + if err != nil || len(got) != 1 || id.Handle != "" || st.profileUpdates != 0 { + t.Fatalf("got=%v err=%v handle=%q saves=%d", got, err, id.Handle, st.profileUpdates) + } +} + +func TestDescribeNameless_CallerCancelDoesNotBackOff(t *testing.T) { + svc, st, pr := describingSetup(t, true) + st.addIdentity("", "tok-1") + st.addIdentity("", "tok-2") + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + cancel() // the client closes the request during the first call + return gitprovider.Identity{}, errors.New("context canceled") + } + if _, err := svc.ListIdentities(ctx); err != nil { + t.Fatal(err) + } + if len(pr.describeLog) != 1 || len(svc.describeFailed) != 0 { + t.Fatalf("calls=%d backoffs=%d, want 1 call and no backoff", len(pr.describeLog), len(svc.describeFailed)) + } +} + +// One 5 s budget covers the whole listing: with a timeout per call, three +// identities that never answer would each be asked. +func TestDescribeNameless_BudgetIsSharedAcrossIdentities(t *testing.T) { + old := describeTimeout + describeTimeout = 50 * time.Millisecond + defer func() { describeTimeout = old }() + svc, st, pr := describingSetup(t, true) + for _, tok := range []string{"tok-1", "tok-2", "tok-3"} { + st.addIdentity("", tok) + } + pr.describeCtx = func(ctx context.Context, _ gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + <-ctx.Done() // a platform that never answers + return gitprovider.Identity{}, ctx.Err() + } + start := time.Now() + if _, err := svc.ListIdentities(context.Background()); err != nil { + t.Fatal(err) + } + if len(pr.describeLog) != 1 { + t.Fatalf("calls = %d, want 1: the budget ran out in the first", len(pr.describeLog)) + } + if el := time.Since(start); el > 5*time.Second { + t.Errorf("listing took %v", el) + } + // The call that ran out of budget is paused, the ones never asked are not. + if len(svc.describeFailed) != 1 { + t.Errorf("backoffs = %d, want 1", len(svc.describeFailed)) + } +} + +func TestDescribeNameless_LogMasksSecrets(t *testing.T) { + var buf bytes.Buffer + old := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) + defer slog.SetDefault(old) + svc, st, pr := describingSetup(t, true) + st.addIdentity("", "tok-1") + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + return gitprovider.Identity{}, errors.New("401 for glpat-abcdefghij1234567890") + } + if _, err := svc.ListIdentities(context.Background()); err != nil { + t.Fatal(err) + } + out := buf.String() + if !strings.Contains(out, "describe identity failed") { + t.Fatalf("no log line: %q", out) + } + if strings.Contains(out, "glpat-abcdefghij1234567890") { + t.Errorf("token leaked in the log: %q", out) + } +} + +func TestDescribeNameless_SkipsWhenNotNeeded(t *testing.T) { + t.Run("handle present", func(t *testing.T) { + svc, st, pr := describingSetup(t, true) + st.addIdentity("already", "tok-1") + if _, err := svc.ListIdentities(context.Background()); err != nil || len(pr.describeLog) != 0 { + t.Fatalf("err=%v calls=%d", err, len(pr.describeLog)) + } + }) + t.Run("provider without the capability", func(t *testing.T) { + svc, st, pr := describingSetup(t, false) + st.addIdentity("", "tok-1") + if _, err := svc.ListConnections(context.Background(), uuid.New()); err != nil || len(pr.describeLog) != 0 { + t.Fatalf("err=%v calls=%d", err, len(pr.describeLog)) + } + }) +} + +func TestDescribeNameless_IgnoresUnusableAnswers(t *testing.T) { + for name, answer := range map[string]gitprovider.Identity{ + "other identity": {Ref: gitprovider.IdentityRef{ID: "someone-else", Handle: "intruder"}, DisplayName: "X"}, + "empty handle": {Ref: gitprovider.IdentityRef{ID: "tok-1"}, DisplayName: "X"}, + } { + t.Run(name, func(t *testing.T) { + svc, st, pr := describingSetup(t, true) + id := st.addIdentity("", "tok-1") + pr.describe = func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { return answer, nil } + if _, err := svc.ListIdentities(context.Background()); err != nil { + t.Fatal(err) + } + if id.Handle != "" || id.DisplayName != "" || st.profileUpdates != 0 { + t.Errorf("stored an unusable answer: %+v saves=%d", id, st.profileUpdates) + } + // It counts as a failure: the next listing does not ask again. + if len(pr.describeLog) != 1 || len(svc.describeFailed) != 1 { + t.Fatalf("calls=%d backoffs=%d, want 1 of each", len(pr.describeLog), len(svc.describeFailed)) + } + if _, err := svc.ListIdentities(context.Background()); err != nil || len(pr.describeLog) != 1 { + t.Errorf("asked again within the backoff: err=%v calls=%d", err, len(pr.describeLog)) + } + }) + } +} + +func TestDescribeNameless_BacksOffAfterFailure(t *testing.T) { + svc, st, pr := describingSetup(t, true) + now := time.Now() + svc.now = func() time.Time { return now } + id := st.addIdentity("", "tok-1") + fail := true + pr.describe = func(r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + if fail { + return gitprovider.Identity{}, errors.New("revoked") + } + return gitprovider.Identity{Ref: gitprovider.IdentityRef{ID: r.Identity.ID, Handle: "bot"}}, nil + } + list := func() { _, _ = svc.ListIdentities(context.Background()) } + + list() + list() + if len(pr.describeLog) != 1 { + t.Fatalf("calls within the backoff = %d, want 1", len(pr.describeLog)) + } + now = now.Add(describeBackoff + time.Second) + list() + if len(pr.describeLog) != 2 { + t.Fatalf("calls after the backoff = %d, want 2", len(pr.describeLog)) + } + fail = false + now = now.Add(describeBackoff + time.Second) + list() + if id.Handle != "bot" || len(svc.describeFailed) != 0 { + t.Errorf("success: handle=%q failures=%v", id.Handle, svc.describeFailed) + } +} diff --git a/services/api/internal/service/git/service.go b/services/api/internal/service/git/service.go index 07c2a6950..4fec983cf 100644 --- a/services/api/internal/service/git/service.go +++ b/services/api/internal/service/git/service.go @@ -34,6 +34,7 @@ type ProviderAPI interface { BeginIdentitySetup(ctx context.Context, req gitprovider.BeginSetupRequest) (gitprovider.SetupStep, error) CompleteIdentitySetup(ctx context.Context, req gitprovider.CompleteSetupRequest) (gitprovider.Identity, error) DeleteIdentity(ctx context.Context, req gitprovider.DeleteIdentityRequest) error + DescribeIdentity(ctx context.Context, req gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) BeginAccessGrant(ctx context.Context, req gitprovider.BeginAccessRequest) (gitprovider.SetupStep, error) CompleteAccessGrant(ctx context.Context, req gitprovider.CompleteAccessRequest) (gitprovider.AccessGrant, error) EnsureWebhook(ctx context.Context, req gitprovider.EnsureWebhookRequest) (gitprovider.WebhookRef, error) @@ -64,11 +65,13 @@ type Service struct { cache map[credKey]cachedCred reruns map[uuid.UUID][]time.Time // check re-runs per repository, last hour ci map[ciKey]cachedCI // CI summaries of change-request heads + + describeFailed map[uuid.UUID]time.Time // identity → last failed describe, see describeBackoff } // New returns a Service. func New(store gitdom.Store, providers Providers) *Service { - return &Service{store: store, providers: providers, now: time.Now, cache: map[credKey]cachedCred{}, reruns: map[uuid.UUID][]time.Time{}} + return &Service{store: store, providers: providers, now: time.Now, cache: map[credKey]cachedCred{}, reruns: map[uuid.UUID][]time.Time{}, describeFailed: map[uuid.UUID]time.Time{}} } type credKey struct { diff --git a/services/api/internal/service/git/service_test.go b/services/api/internal/service/git/service_test.go index f99b48b64..887203ac5 100644 --- a/services/api/internal/service/git/service_test.go +++ b/services/api/internal/service/git/service_test.go @@ -34,6 +34,8 @@ type fakeStore struct { sessions map[string]*gitdom.SetupSession access map[string]*gitdom.AccessSession bound map[string]uuid.UUID + + profileUpdates int } func newFakeStore() *fakeStore { @@ -100,6 +102,21 @@ func (f *fakeStore) DeleteAgentIdentity(_ context.Context, a uuid.UUID, _ string delete(f.agentIDs, a) return nil } +func (f *fakeStore) UpdateIdentityProfile(_ context.Context, id uuid.UUID, handle, name string, exp *time.Time) error { + i, ok := f.byID[id] + if !ok { + return gitdom.ErrNotFound + } + f.profileUpdates++ + i.Handle = handle + if name != "" { + i.DisplayName = name + } + if exp != nil { + i.ExpiresAt = exp + } + return nil +} func (f *fakeStore) DeleteIdentity(_ context.Context, id uuid.UUID) error { i, ok := f.byID[id] if !ok { @@ -339,10 +356,22 @@ type fakeProv struct { removed []gitprovider.RemoveWebhookRequest describes int + describe func(gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) + describeLog []gitprovider.DescribeIdentityRequest + describeCtx func(context.Context, gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) + list func(gitprovider.ListReposRequest) (gitprovider.RepositoryPage, error) listReqs []gitprovider.ListReposRequest } +func (p *fakeProv) DescribeIdentity(ctx context.Context, r gitprovider.DescribeIdentityRequest) (gitprovider.Identity, error) { + p.describeLog = append(p.describeLog, r) + if p.describeCtx != nil { + return p.describeCtx(ctx, r) + } + return p.describe(r) +} + func (p *fakeProv) ListRepositories(_ context.Context, r gitprovider.ListReposRequest) (gitprovider.RepositoryPage, error) { p.listReqs = append(p.listReqs, r) if p.list == nil { diff --git a/services/api/internal/service/git/setup.go b/services/api/internal/service/git/setup.go index 141e76f19..e262625bb 100644 --- a/services/api/internal/service/git/setup.go +++ b/services/api/internal/service/git/setup.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "encoding/hex" "errors" + "log/slog" "net/url" "strings" "time" @@ -299,6 +300,7 @@ func (s *Service) ListIdentities(ctx context.Context) ([]*gitdom.Identity, error } out = append(out, ids...) } + s.describeNameless(ctx, out) return out, nil } @@ -321,6 +323,7 @@ func (s *Service) ListConnections(ctx context.Context, projectID uuid.UUID) ([]* all = append(all, id.ID) } } + s.describeNameless(ctx, out) counts, err := s.store.CountRepositories(ctx, all) if err != nil { return nil, err @@ -331,6 +334,101 @@ func (s *Service) ListConnections(ctx context.Context, projectID uuid.UUID) ([]* return out, nil } +// describeTimeout bounds all the describe_identity calls of one listing, however +// many identities it has to name: listings must not hang on a slow platform. +// Whatever it leaves unnamed is asked on the next listing. +var describeTimeout = 5 * time.Second // a variable only so tests can shorten it + +// describeBackoff is how long after a failed describe the same identity is not +// asked again (a revoked token or a platform outage would otherwise slow every +// listing). It is kept in memory only, so a restart retries. +const describeBackoff = 15 * time.Minute + +// describeNameless asks the provider who each identity without a handle is, +// and stores the answer. Identities made by a plugin's data migration start +// that way (the migration cannot call the platform), so they could be neither +// named in Settings nor matched by an @mention. The call happens at most once +// per identity: after it the stored handle is no longer empty. It is best +// effort: any failure is logged and the caller carries on with the old values. +func (s *Service) describeNameless(ctx context.Context, ids []*gitdom.Identity) { + budget, cancel := context.WithTimeout(ctx, describeTimeout) + defer cancel() + canDescribe := map[string]bool{} // provider id → declares identity_description + for _, id := range ids { + if id.Handle != "" { + continue + } + if budget.Err() != nil { + return // not a failure of this identity, so no backoff either + } + ok, seen := canDescribe[id.ProviderID] + if !seen { + info, err := s.providers.Describe(budget, id.ProviderID) + ok = err == nil && info.Capabilities.IdentityDescription + canDescribe[id.ProviderID] = ok + } + if ok { + s.describeIdentity(ctx, budget, id) + } + } +} + +// describeIdentity asks within budget but saves with ctx, so an answer that +// arrives just in time is not lost. +func (s *Service) describeIdentity(ctx, budget context.Context, id *gitdom.Identity) { + s.mu.Lock() + failedAt, failed := s.describeFailed[id.ID] + s.mu.Unlock() + if failed && s.now().Sub(failedAt) < describeBackoff { + return + } + prov, err := s.providers.Provider(budget, id.ProviderID) + if err != nil { + return + } + got, err := prov.DescribeIdentity(budget, gitprovider.DescribeIdentityRequest{Identity: identityRef(id)}) + if err != nil { + if ctx.Err() != nil { + return // the caller gave up (client closed the request): the provider is not at fault + } + // The text comes from the plugin, so it is masked like any provider message. + slog.Warn("git: describe identity failed", "provider", id.ProviderID, "identity", id.ID, "err", RedactSecrets(err.Error())) + s.backOffDescribe(id.ID) + return + } + // An answer for another identity, or without a handle, is as useless as an + // error: it is not stored and the identity is not asked again right away. + if got.Ref.ID != id.ProviderIdentityID || got.Ref.Handle == "" { + slog.Warn("git: describe identity gave an unusable answer", "provider", id.ProviderID, "identity", id.ID) + s.backOffDescribe(id.ID) + return + } + s.mu.Lock() + delete(s.describeFailed, id.ID) + s.mu.Unlock() + var exp *time.Time + if !got.ExpiresAt.IsZero() { + exp = &got.ExpiresAt + } + if err := s.store.UpdateIdentityProfile(ctx, id.ID, got.Ref.Handle, got.DisplayName, exp); err != nil { + slog.Warn("git: save described identity failed", "provider", id.ProviderID, "identity", id.ID, "err", err) + return + } + id.Handle = got.Ref.Handle + if got.DisplayName != "" { + id.DisplayName = got.DisplayName + } + if exp != nil { + id.ExpiresAt = exp + } +} + +func (s *Service) backOffDescribe(id uuid.UUID) { + s.mu.Lock() + s.describeFailed[id] = s.now() + s.mu.Unlock() +} + func (s *Service) enabledProviders(ctx context.Context) (map[string]bool, error) { infos, err := s.providers.List(ctx) if err != nil {