From bc53ef7fe6e13108e13267faacd4a51b75a80d50 Mon Sep 17 00:00:00 2001 From: J Date: Thu, 2 Jul 2026 17:12:31 +0800 Subject: [PATCH] fix(github): allow one installation to bind multiple workspaces Connecting the same GitHub App installation in a second workspace silently overwrote the first workspace's binding: github_installation was UNIQUE(installation_id) and CreateGitHubInstallation's upsert overwrote workspace_id on conflict (#4823). Widen the uniqueness key to (workspace_id, installation_id) so each workspace keeps its own binding row, and teach the webhook/lifecycle paths to handle N bindings per installation_id: - CreateGitHubInstallation upserts per (workspace_id, installation_id). - Webhook lookup lists all bindings; PR/check_suite routing uses the oldest binding as the deterministic fallback and still routes per-repo via the existing workspace.repos registry. - installation.deleted/suspend drops every workspace binding and broadcasts to each affected workspace. - installation.created/unsuspend refreshes account metadata across all bindings. - Add a standalone index on installation_id (the dropped unique constraint was the only index behind the webhook lookup). MUL-3950 Co-authored-by: multica-agent --- server/internal/handler/github.go | 112 +++++----- server/internal/handler/github_test.go | 198 +++++++++++++++++- ...thub_installation_multi_workspace.down.sql | 13 ++ ...github_installation_multi_workspace.up.sql | 21 ++ server/pkg/db/generated/github.sql.go | 147 ++++++++++--- server/pkg/db/queries/github.sql | 28 ++- 6 files changed, 423 insertions(+), 96 deletions(-) create mode 100644 server/migrations/133_github_installation_multi_workspace.down.sql create mode 100644 server/migrations/133_github_installation_multi_workspace.up.sql diff --git a/server/internal/handler/github.go b/server/internal/handler/github.go index c956293b9a..417b4ebb49 100644 --- a/server/internal/handler/github.go +++ b/server/internal/handler/github.go @@ -681,19 +681,13 @@ func (h *Handler) handleInstallationEvent(ctx context.Context, body []byte) { } switch p.Action { case "deleted", "suspend": - // User removed the App on GitHub — drop our row so the workspace - // stops trusting this installation_id. We DELETE … RETURNING so - // the broadcast can be scoped to the right workspace; events - // without WorkspaceID are dropped by the realtime listener and - // would leave already-open Settings tabs stale. + // User removed/suspended the App on GitHub — trust in this + // installation_id is gone entirely, so drop every workspace binding. + // We DELETE … RETURNING so each broadcast can be scoped to its + // workspace; events without WorkspaceID are dropped by the realtime + // listener and would leave already-open Settings tabs stale. deleted, err := h.Queries.DeleteGitHubInstallationByInstallationID(ctx, p.Installation.ID) if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - if err := h.Queries.DeletePendingGitHubInstallation(ctx, p.Installation.ID); err != nil { - slog.Warn("github: delete pending installation failed", "err", err, "installation_id", p.Installation.ID) - } - return // already gone — nothing to broadcast - } slog.Warn("github: delete installation failed", "err", err, "installation_id", p.Installation.ID) return } @@ -703,10 +697,13 @@ func (h *Handler) handleInstallationEvent(ctx context.Context, body []byte) { // Broadcast the internal row id only — the numeric installation_id is // a management handle that non-admin members are not allowed to see. // The frontend invalidates the installations query on this event and - // does not read the broadcast payload directly. - h.publish(protocol.EventGitHubInstallationDeleted, uuidToString(deleted.WorkspaceID), "system", "", map[string]any{ - "id": uuidToString(deleted.ID), - }) + // does not read the broadcast payload directly. One broadcast per + // deleted binding so every affected workspace's Settings tab refreshes. + for _, row := range deleted { + h.publish(protocol.EventGitHubInstallationDeleted, uuidToString(row.WorkspaceID), "system", "", map[string]any{ + "id": uuidToString(row.ID), + }) + } case "created", "new_permissions_accepted", "unsuspend": login, accountType, avatar, ok := githubInstallationAccountFromPayload(p) if !ok { @@ -714,33 +711,33 @@ func (h *Handler) handleInstallationEvent(ctx context.Context, body []byte) { return } - // We don't know which workspace this maps to from the webhook alone. - // If the setup callback has not created the workspace binding yet, + // We don't know which workspace(s) this maps to from the webhook + // alone. If no setup callback has created a workspace binding yet, // keep the account metadata and let the callback consume it after it // creates github_installation. - existing, err := h.Queries.GetGitHubInstallationByInstallationID(ctx, p.Installation.ID) + existing, err := h.Queries.ListGitHubInstallationsByInstallationID(ctx, p.Installation.ID) if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - if _, err := h.Queries.UpsertPendingGitHubInstallation(ctx, db.UpsertPendingGitHubInstallationParams{ - InstallationID: p.Installation.ID, - AccountLogin: login, - AccountType: accountType, - AccountAvatarUrl: ptrToText(avatar), - }); err != nil { - slog.Warn("github: store pending installation failed", "err", err, "installation_id", p.Installation.ID) - } - return - } slog.Warn("github: lookup installation failed", "err", err, "installation_id", p.Installation.ID) return } - inst, err := h.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ - WorkspaceID: existing.WorkspaceID, + if len(existing) == 0 { + if _, err := h.Queries.UpsertPendingGitHubInstallation(ctx, db.UpsertPendingGitHubInstallationParams{ + InstallationID: p.Installation.ID, + AccountLogin: login, + AccountType: accountType, + AccountAvatarUrl: ptrToText(avatar), + }); err != nil { + slog.Warn("github: store pending installation failed", "err", err, "installation_id", p.Installation.ID) + } + return + } + // Refresh the account display metadata across every workspace binding; + // workspace_id and connected_by_id are left untouched. + refreshed, err := h.Queries.UpdateGitHubInstallationAccountByInstallationID(ctx, db.UpdateGitHubInstallationAccountByInstallationIDParams{ InstallationID: p.Installation.ID, AccountLogin: login, AccountType: accountType, AccountAvatarUrl: ptrToText(avatar), - ConnectedByID: existing.ConnectedByID, }) if err != nil { slog.Warn("github: refresh installation failed", "err", err) @@ -754,10 +751,12 @@ func (h *Handler) handleInstallationEvent(ctx context.Context, body []byte) { // callback with the "unknown" placeholder (e.g. because GitHub // App JWT auth wasn't configured, or this webhook arrived after // the user already loaded the page) would stay visibly stale - // until the user manually refreshes. - h.publish(protocol.EventGitHubInstallationCreated, uuidToString(inst.WorkspaceID), "system", "", map[string]any{ - "installation": githubInstallationToBroadcast(inst), - }) + // until the user manually refreshes. One broadcast per bound workspace. + for _, inst := range refreshed { + h.publish(protocol.EventGitHubInstallationCreated, uuidToString(inst.WorkspaceID), "system", "", map[string]any{ + "installation": githubInstallationToBroadcast(inst), + }) + } } } @@ -809,15 +808,20 @@ func (h *Handler) handlePullRequestEvent(ctx context.Context, body []byte) { if p.Installation.ID == 0 { return } - inst, err := h.Queries.GetGitHubInstallationByInstallationID(ctx, p.Installation.ID) + insts, err := h.Queries.ListGitHubInstallationsByInstallationID(ctx, p.Installation.ID) if err != nil { - // Webhook from an installation we never wired up — nothing we - // can attribute to a workspace, so drop it silently. - if !errors.Is(err, pgx.ErrNoRows) { - slog.Warn("github: lookup installation failed", "err", err) - } + slog.Warn("github: lookup installation failed", "err", err) return } + if len(insts) == 0 { + // Webhook from an installation we never wired up — nothing we + // can attribute to a workspace, so drop it silently. + return + } + // One installation can be bound to several workspaces; delivery is routed + // per-repo, so the binding only supplies the delivering account and the + // fallback workspace. The oldest binding is a deterministic fallback. + inst := insts[0] // Route to the workspace that owns this repo, not the installation's single // workspace — one installation can serve repos across several workspaces. @@ -1013,13 +1017,17 @@ func (h *Handler) handleCheckSuiteEvent(ctx context.Context, body []byte) { if p.Installation.ID == 0 { return } - inst, err := h.Queries.GetGitHubInstallationByInstallationID(ctx, p.Installation.ID) + insts, err := h.Queries.ListGitHubInstallationsByInstallationID(ctx, p.Installation.ID) if err != nil { - if !errors.Is(err, pgx.ErrNoRows) { - slog.Warn("github: lookup installation failed", "err", err) - } + slog.Warn("github: lookup installation failed", "err", err) return } + if len(insts) == 0 { + return + } + // Oldest binding is the deterministic routing fallback; see + // handlePullRequestEvent. + inst := insts[0] if len(p.CheckSuite.PullRequests) == 0 { // Forks emit suites whose `pull_requests` array is empty for // the upstream repo. We have no way to attribute the result @@ -1229,11 +1237,13 @@ const githubWebhookHost = "github.com" // resolveWorkspaceForRepo routes a delivery to the workspace whose repos // registry owns github.com/owner/name, so one installation can serve repos in -// several workspaces; falls back to the installation workspace when unmatched. -// The registry is admin-editable, so it overrides the verified installation -// binding only when owner == the delivering account (accountLogin) and the host -// matches — no cross-account capture. On ties the installation's own workspace -// wins, else the lowest id (query is ORDER BY id). +// several workspaces; falls back to the caller-supplied workspace when +// unmatched (callers pass the installation's oldest binding, since an +// installation may now be bound to several workspaces). The registry is +// admin-editable, so it overrides the verified installation binding only when +// owner == the delivering account (accountLogin) and the host matches — no +// cross-account capture. On ties the fallback workspace wins if it is among the +// matches, else the lowest id (query is ORDER BY id). func (h *Handler) resolveWorkspaceForRepo(ctx context.Context, fallback pgtype.UUID, accountLogin, owner, name string) pgtype.UUID { owner = strings.TrimSpace(owner) name = strings.TrimSpace(name) diff --git a/server/internal/handler/github_test.go b/server/internal/handler/github_test.go index 8173a2431b..ed68225404 100644 --- a/server/internal/handler/github_test.go +++ b/server/internal/handler/github_test.go @@ -438,13 +438,18 @@ func TestWebhook_UninstallReturnsWorkspaceForBroadcast(t *testing.T) { if err != nil { t.Fatalf("DeleteGitHubInstallationByInstallationID: %v", err) } - if uuidToString(deleted.WorkspaceID) != testWorkspaceID { - t.Errorf("expected returned workspace_id %s, got %s", testWorkspaceID, uuidToString(deleted.WorkspaceID)) + if len(deleted) != 1 { + t.Fatalf("expected 1 deleted binding, got %d", len(deleted)) } - // Re-deleting must surface ErrNoRows so the handler can short-circuit - // the broadcast (and not panic). - if _, err := testHandler.Queries.DeleteGitHubInstallationByInstallationID(ctx, installationID); err == nil { - t.Error("expected ErrNoRows on second delete, got nil") + if uuidToString(deleted[0].WorkspaceID) != testWorkspaceID { + t.Errorf("expected returned workspace_id %s, got %s", testWorkspaceID, uuidToString(deleted[0].WorkspaceID)) + } + // Re-deleting must return no rows so the handler skips the broadcast + // (and does not panic). + if again, err := testHandler.Queries.DeleteGitHubInstallationByInstallationID(ctx, installationID); err != nil { + t.Errorf("second delete errored: %v", err) + } else if len(again) != 0 { + t.Errorf("expected 0 rows on second delete, got %d", len(again)) } } @@ -2504,10 +2509,14 @@ func TestWebhook_InstallationCreatedRefreshesUnknownLogin(t *testing.T) { } // (a) The row's account_login must be the real login, not "unknown". - got, err := testHandler.Queries.GetGitHubInstallationByInstallationID(ctx, installationID) + rows, err := testHandler.Queries.ListGitHubInstallationsByInstallationID(ctx, installationID) if err != nil { - t.Fatalf("get installation: %v", err) + t.Fatalf("list installations: %v", err) } + if len(rows) != 1 { + t.Fatalf("expected 1 installation row, got %d", len(rows)) + } + got := rows[0] if got.AccountLogin != "real-octocat" { t.Errorf("account_login = %q, want %q (refresh did not overwrite the unknown placeholder)", got.AccountLogin, "real-octocat") @@ -2633,10 +2642,14 @@ func TestSetupCallback_ConsumesPendingInstallationCreated(t *testing.T) { t.Fatalf("setup callback redirect = %q, want github_connected=1", loc) } - got, err := testHandler.Queries.GetGitHubInstallationByInstallationID(ctx, installationID) + rows, err := testHandler.Queries.ListGitHubInstallationsByInstallationID(ctx, installationID) if err != nil { - t.Fatalf("get installation: %v", err) + t.Fatalf("list installations: %v", err) } + if len(rows) != 1 { + t.Fatalf("expected 1 installation row, got %d", len(rows)) + } + got := rows[0] if got.AccountLogin != "pending-octocat" { t.Errorf("account_login = %q, want pending-octocat (callback left the unknown placeholder)", got.AccountLogin) } @@ -2906,3 +2919,168 @@ func TestWebhook_RegistryDoesNotCaptureForeignAccount(t *testing.T) { t.Fatalf("foreign-account repo must not link to the squatter workspace, got %d links", len(linked)) } } + +// TestSecondWorkspaceBindDoesNotUnbindFirst is the #4823 regression: binding +// the same GitHub App installation in a second workspace must NOT overwrite the +// first workspace's binding. Both bindings coexist, and re-binding an existing +// (workspace, installation) pair upserts its row in place. +func TestSecondWorkspaceBindDoesNotUnbindFirst(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("handler test fixture not initialized (no DB?)") + } + ctx := context.Background() + const installationID int64 = 909090909 + + testPool.Exec(ctx, `DELETE FROM workspace WHERE slug = $1`, "multi-bind-ws-b") + wsB, err := testHandler.Queries.CreateWorkspace(ctx, db.CreateWorkspaceParams{ + Name: "multi-bind-ws-b", + Slug: "multi-bind-ws-b", + IssuePrefix: "MBB", + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM github_installation WHERE installation_id = $1`, installationID) + testPool.Exec(ctx, `DELETE FROM workspace WHERE id = $1`, wsB.ID) + }) + + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: parseUUID(testWorkspaceID), + InstallationID: installationID, + AccountLogin: "shared-org", + AccountType: "Organization", + }); err != nil { + t.Fatalf("bind workspace A: %v", err) + } + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: wsB.ID, + InstallationID: installationID, + AccountLogin: "shared-org", + AccountType: "Organization", + }); err != nil { + t.Fatalf("bind workspace B: %v", err) + } + + rows, err := testHandler.Queries.ListGitHubInstallationsByInstallationID(ctx, installationID) + if err != nil { + t.Fatalf("list installations: %v", err) + } + if len(rows) != 2 { + t.Fatalf("expected 2 bindings to coexist (silent unbind regression), got %d", len(rows)) + } + seen := map[string]bool{} + for _, r := range rows { + seen[uuidToString(r.WorkspaceID)] = true + } + if !seen[testWorkspaceID] || !seen[uuidToString(wsB.ID)] { + t.Errorf("both workspaces must retain a binding; got %v", seen) + } + + // Re-binding workspace A must upsert its own row in place, not add a third. + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: parseUUID(testWorkspaceID), + InstallationID: installationID, + AccountLogin: "shared-org-renamed", + AccountType: "Organization", + }); err != nil { + t.Fatalf("re-bind workspace A: %v", err) + } + rows, err = testHandler.Queries.ListGitHubInstallationsByInstallationID(ctx, installationID) + if err != nil { + t.Fatalf("list installations after re-bind: %v", err) + } + if len(rows) != 2 { + t.Fatalf("re-binding an existing (workspace, installation) must upsert, got %d rows", len(rows)) + } +} + +// TestWebhook_UninstallDeletesAllBindings verifies a GitHub-side app uninstall +// drops every workspace binding for the installation and broadcasts to each +// affected workspace so their Settings tabs refresh. +func TestWebhook_UninstallDeletesAllBindings(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("handler test fixture not initialized (no DB?)") + } + ctx := context.Background() + secret := "uninstall-all-secret" + t.Setenv("GITHUB_WEBHOOK_SECRET", secret) + const installationID int64 = 707070707 + + testPool.Exec(ctx, `DELETE FROM workspace WHERE slug = $1`, "uninstall-all-ws-b") + wsB, err := testHandler.Queries.CreateWorkspace(ctx, db.CreateWorkspaceParams{ + Name: "uninstall-all-ws-b", + Slug: "uninstall-all-ws-b", + IssuePrefix: "UAB", + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM github_installation WHERE installation_id = $1`, installationID) + testPool.Exec(ctx, `DELETE FROM workspace WHERE id = $1`, wsB.ID) + }) + + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: parseUUID(testWorkspaceID), + InstallationID: installationID, + AccountLogin: "shared-org", + AccountType: "Organization", + }); err != nil { + t.Fatalf("bind workspace A: %v", err) + } + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: wsB.ID, + InstallationID: installationID, + AccountLogin: "shared-org", + AccountType: "Organization", + }); err != nil { + t.Fatalf("bind workspace B: %v", err) + } + + gotWS := make(chan string, 2) + testHandler.Bus.Subscribe(protocol.EventGitHubInstallationDeleted, func(e events.Event) { + select { + case gotWS <- e.WorkspaceID: + default: + } + }) + + body, _ := json.Marshal(map[string]any{ + "action": "deleted", + "installation": map[string]any{"id": installationID}, + }) + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + sig := "sha256=" + hex.EncodeToString(mac.Sum(nil)) + rec := httptest.NewRecorder() + req := httptest.NewRequest("POST", "/api/webhooks/github", bytes.NewReader(body)) + req.Header.Set("X-GitHub-Event", "installation") + req.Header.Set("X-Hub-Signature-256", sig) + testHandler.HandleGitHubWebhook(rec, req) + if rec.Code != http.StatusAccepted { + t.Fatalf("webhook: expected 202, got %d (%s)", rec.Code, rec.Body.String()) + } + + rows, err := testHandler.Queries.ListGitHubInstallationsByInstallationID(ctx, installationID) + if err != nil { + t.Fatalf("list installations: %v", err) + } + if len(rows) != 0 { + t.Fatalf("expected all bindings deleted, got %d", len(rows)) + } + + seen := map[string]bool{} + deadline := time.After(2 * time.Second) + for len(seen) < 2 { + select { + case ws := <-gotWS: + seen[ws] = true + case <-deadline: + t.Fatalf("expected 2 deleted broadcasts (one per workspace), saw %v", seen) + } + } + if !seen[testWorkspaceID] || !seen[uuidToString(wsB.ID)] { + t.Errorf("deleted broadcasts must cover both workspaces; saw %v", seen) + } +} diff --git a/server/migrations/133_github_installation_multi_workspace.down.sql b/server/migrations/133_github_installation_multi_workspace.down.sql new file mode 100644 index 0000000000..4964401f10 --- /dev/null +++ b/server/migrations/133_github_installation_multi_workspace.down.sql @@ -0,0 +1,13 @@ +-- Revert to a single workspace per installation. +-- +-- NOTE: this will fail if any installation_id is bound to more than one +-- workspace (which the widened schema permits) — reverting a widened +-- uniqueness constraint requires the data to already satisfy the narrower one. +DROP INDEX IF EXISTS idx_github_installation_installation_id; + +ALTER TABLE github_installation + DROP CONSTRAINT github_installation_workspace_id_installation_id_key; + +ALTER TABLE github_installation + ADD CONSTRAINT github_installation_installation_id_key + UNIQUE (installation_id); diff --git a/server/migrations/133_github_installation_multi_workspace.up.sql b/server/migrations/133_github_installation_multi_workspace.up.sql new file mode 100644 index 0000000000..1908cf646a --- /dev/null +++ b/server/migrations/133_github_installation_multi_workspace.up.sql @@ -0,0 +1,21 @@ +-- Allow one GitHub App installation to bind to multiple workspaces. +-- +-- Previously UNIQUE(installation_id) forced a single workspace per installation. +-- Connecting the same GitHub account/org in a second workspace ran the +-- CreateGitHubInstallation upsert, whose ON CONFLICT (installation_id) silently +-- overwrote the first workspace's binding row (#4823). Widening the uniqueness +-- key to (workspace_id, installation_id) lets each workspace keep its own row, +-- and webhook delivery is routed per-repo via the workspace.repos registry. +ALTER TABLE github_installation + DROP CONSTRAINT github_installation_installation_id_key; + +ALTER TABLE github_installation + ADD CONSTRAINT github_installation_workspace_id_installation_id_key + UNIQUE (workspace_id, installation_id); + +-- The dropped UNIQUE(installation_id) also provided the index behind webhook +-- lookups by installation_id. The new composite constraint is keyed on +-- workspace_id first, so it cannot serve a bare installation_id lookup — add a +-- standalone index to keep that path fast. +CREATE INDEX IF NOT EXISTS idx_github_installation_installation_id + ON github_installation(installation_id); diff --git a/server/pkg/db/generated/github.sql.go b/server/pkg/db/generated/github.sql.go index 19d134b569..caba1c61bc 100644 --- a/server/pkg/db/generated/github.sql.go +++ b/server/pkg/db/generated/github.sql.go @@ -17,8 +17,7 @@ INSERT INTO github_installation ( ) VALUES ( $1, $2, $3, $4, $5, $6 ) -ON CONFLICT (installation_id) DO UPDATE SET - workspace_id = EXCLUDED.workspace_id, +ON CONFLICT (workspace_id, installation_id) DO UPDATE SET account_login = EXCLUDED.account_login, account_type = EXCLUDED.account_type, account_avatar_url = EXCLUDED.account_avatar_url, @@ -74,7 +73,7 @@ func (q *Queries) DeleteGitHubInstallation(ctx context.Context, arg DeleteGitHub return err } -const deleteGitHubInstallationByInstallationID = `-- name: DeleteGitHubInstallationByInstallationID :one +const deleteGitHubInstallationByInstallationID = `-- name: DeleteGitHubInstallationByInstallationID :many DELETE FROM github_installation WHERE installation_id = $1 RETURNING id, workspace_id ` @@ -84,11 +83,27 @@ type DeleteGitHubInstallationByInstallationIDRow struct { WorkspaceID pgtype.UUID `json:"workspace_id"` } -func (q *Queries) DeleteGitHubInstallationByInstallationID(ctx context.Context, installationID int64) (DeleteGitHubInstallationByInstallationIDRow, error) { - row := q.db.QueryRow(ctx, deleteGitHubInstallationByInstallationID, installationID) - var i DeleteGitHubInstallationByInstallationIDRow - err := row.Scan(&i.ID, &i.WorkspaceID) - return i, err +// GitHub-side uninstall/suspend removes trust in the installation entirely, so +// drop every workspace binding. Returns one row per deleted binding so the +// handler can broadcast to each affected workspace. +func (q *Queries) DeleteGitHubInstallationByInstallationID(ctx context.Context, installationID int64) ([]DeleteGitHubInstallationByInstallationIDRow, error) { + rows, err := q.db.Query(ctx, deleteGitHubInstallationByInstallationID, installationID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []DeleteGitHubInstallationByInstallationIDRow{} + for rows.Next() { + var i DeleteGitHubInstallationByInstallationIDRow + if err := rows.Scan(&i.ID, &i.WorkspaceID); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil } const deletePendingGitHubInstallation = `-- name: DeletePendingGitHubInstallation :exec @@ -183,28 +198,6 @@ func (q *Queries) GetGitHubInstallationByID(ctx context.Context, id pgtype.UUID) return i, err } -const getGitHubInstallationByInstallationID = `-- name: GetGitHubInstallationByInstallationID :one -SELECT id, workspace_id, installation_id, account_login, account_type, account_avatar_url, connected_by_id, created_at, updated_at FROM github_installation -WHERE installation_id = $1 -` - -func (q *Queries) GetGitHubInstallationByInstallationID(ctx context.Context, installationID int64) (GithubInstallation, error) { - row := q.db.QueryRow(ctx, getGitHubInstallationByInstallationID, installationID) - var i GithubInstallation - err := row.Scan( - &i.ID, - &i.WorkspaceID, - &i.InstallationID, - &i.AccountLogin, - &i.AccountType, - &i.AccountAvatarUrl, - &i.ConnectedByID, - &i.CreatedAt, - &i.UpdatedAt, - ) - return i, err -} - const getGitHubPullRequest = `-- name: GetGitHubPullRequest :one SELECT id, workspace_id, installation_id, repo_owner, repo_name, pr_number, title, state, html_url, branch, author_login, author_avatar_url, merged_at, closed_at, pr_created_at, pr_updated_at, created_at, updated_at, head_sha, mergeable_state, additions, deletions, changed_files FROM github_pull_request WHERE workspace_id = $1 AND repo_owner = $2 AND repo_name = $3 AND pr_number = $4 @@ -343,6 +336,45 @@ func (q *Queries) LinkIssueToPullRequest(ctx context.Context, arg LinkIssueToPul return err } +const listGitHubInstallationsByInstallationID = `-- name: ListGitHubInstallationsByInstallationID :many +SELECT id, workspace_id, installation_id, account_login, account_type, account_avatar_url, connected_by_id, created_at, updated_at FROM github_installation +WHERE installation_id = $1 +ORDER BY created_at ASC, id ASC +` + +// One installation_id can be bound to several workspaces; webhook routing lists +// every binding and picks the target workspace via the repos registry. Ordered +// so the oldest binding is the deterministic routing fallback (insts[0]). +func (q *Queries) ListGitHubInstallationsByInstallationID(ctx context.Context, installationID int64) ([]GithubInstallation, error) { + rows, err := q.db.Query(ctx, listGitHubInstallationsByInstallationID, installationID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []GithubInstallation{} + for rows.Next() { + var i GithubInstallation + if err := rows.Scan( + &i.ID, + &i.WorkspaceID, + &i.InstallationID, + &i.AccountLogin, + &i.AccountType, + &i.AccountAvatarUrl, + &i.ConnectedByID, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listGitHubInstallationsByWorkspace = `-- name: ListGitHubInstallationsByWorkspace :many SELECT id, workspace_id, installation_id, account_login, account_type, account_avatar_url, connected_by_id, created_at, updated_at FROM github_installation @@ -558,6 +590,61 @@ func (q *Queries) UnlinkIssueFromPullRequest(ctx context.Context, arg UnlinkIssu return err } +const updateGitHubInstallationAccountByInstallationID = `-- name: UpdateGitHubInstallationAccountByInstallationID :many +UPDATE github_installation +SET account_login = $2, + account_type = $3, + account_avatar_url = $4, + updated_at = now() +WHERE installation_id = $1 +RETURNING id, workspace_id, installation_id, account_login, account_type, account_avatar_url, connected_by_id, created_at, updated_at +` + +type UpdateGitHubInstallationAccountByInstallationIDParams struct { + InstallationID int64 `json:"installation_id"` + AccountLogin string `json:"account_login"` + AccountType string `json:"account_type"` + AccountAvatarUrl pgtype.Text `json:"account_avatar_url"` +} + +// Refresh the GitHub account display metadata across every workspace binding of +// an installation (fired by installation.created/new_permissions_accepted/ +// unsuspend). Leaves workspace_id and connected_by_id untouched. +func (q *Queries) UpdateGitHubInstallationAccountByInstallationID(ctx context.Context, arg UpdateGitHubInstallationAccountByInstallationIDParams) ([]GithubInstallation, error) { + rows, err := q.db.Query(ctx, updateGitHubInstallationAccountByInstallationID, + arg.InstallationID, + arg.AccountLogin, + arg.AccountType, + arg.AccountAvatarUrl, + ) + if err != nil { + return nil, err + } + defer rows.Close() + items := []GithubInstallation{} + for rows.Next() { + var i GithubInstallation + if err := rows.Scan( + &i.ID, + &i.WorkspaceID, + &i.InstallationID, + &i.AccountLogin, + &i.AccountType, + &i.AccountAvatarUrl, + &i.ConnectedByID, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const upsertGitHubPullRequest = `-- name: UpsertGitHubPullRequest :one INSERT INTO github_pull_request ( diff --git a/server/pkg/db/queries/github.sql b/server/pkg/db/queries/github.sql index 62afe1f91b..8bd3d9db52 100644 --- a/server/pkg/db/queries/github.sql +++ b/server/pkg/db/queries/github.sql @@ -7,9 +7,13 @@ SELECT * FROM github_installation WHERE workspace_id = $1 ORDER BY created_at ASC; --- name: GetGitHubInstallationByInstallationID :one +-- name: ListGitHubInstallationsByInstallationID :many +-- One installation_id can be bound to several workspaces; webhook routing lists +-- every binding and picks the target workspace via the repos registry. Ordered +-- so the oldest binding is the deterministic routing fallback (insts[0]). SELECT * FROM github_installation -WHERE installation_id = $1; +WHERE installation_id = $1 +ORDER BY created_at ASC, id ASC; -- name: GetGitHubInstallationByID :one SELECT * FROM github_installation @@ -21,8 +25,7 @@ INSERT INTO github_installation ( ) VALUES ( $1, $2, $3, $4, sqlc.narg('account_avatar_url'), sqlc.narg('connected_by_id') ) -ON CONFLICT (installation_id) DO UPDATE SET - workspace_id = EXCLUDED.workspace_id, +ON CONFLICT (workspace_id, installation_id) DO UPDATE SET account_login = EXCLUDED.account_login, account_type = EXCLUDED.account_type, account_avatar_url = EXCLUDED.account_avatar_url, @@ -33,10 +36,25 @@ RETURNING *; -- name: DeleteGitHubInstallation :exec DELETE FROM github_installation WHERE id = $1 AND workspace_id = $2; --- name: DeleteGitHubInstallationByInstallationID :one +-- name: DeleteGitHubInstallationByInstallationID :many +-- GitHub-side uninstall/suspend removes trust in the installation entirely, so +-- drop every workspace binding. Returns one row per deleted binding so the +-- handler can broadcast to each affected workspace. DELETE FROM github_installation WHERE installation_id = $1 RETURNING id, workspace_id; +-- name: UpdateGitHubInstallationAccountByInstallationID :many +-- Refresh the GitHub account display metadata across every workspace binding of +-- an installation (fired by installation.created/new_permissions_accepted/ +-- unsuspend). Leaves workspace_id and connected_by_id untouched. +UPDATE github_installation +SET account_login = $2, + account_type = $3, + account_avatar_url = sqlc.narg('account_avatar_url'), + updated_at = now() +WHERE installation_id = $1 +RETURNING *; + -- name: UpsertPendingGitHubInstallation :one INSERT INTO github_pending_installation ( installation_id, account_login, account_type, account_avatar_url