From 27851ae16174964cdfbfa522302b2eb910c482e4 Mon Sep 17 00:00:00 2001 From: Lambda Date: Wed, 20 May 2026 01:24:47 +0800 Subject: [PATCH] feat(github): expose read-only installation list to workspace members (MUL-2413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relax `GET /api/workspaces/{id}/github/installations` from owner/admin-only to any workspace member so the Settings → Integrations tab no longer renders blank for non-admins (the original symptom of MUL-2413). The handler now reads the caller's role from the workspace middleware: - owner / admin keep the full row including the numeric `installation_id` (the connect / disconnect handle) and receive `can_manage: true`. - every other role (member / guest) receives rows with `installation_id` omitted and `can_manage: false`, giving them visibility into "is GitHub wired up?" without the management handle. `GET /github/connect` and `DELETE /github/installations/{id}` stay under the admin/owner middleware group — this PR only relaxes the read path. Tests: `TestListGitHubInstallations_RoleGating` exercises admin, owner, member, and guest paths against the real DB-backed handler fixture and asserts the field stripping + `can_manage` contract. Refs: MUL-2413 Co-authored-by: multica-agent --- packages/core/types/github.ts | 10 ++- server/cmd/server/router.go | 11 ++- server/internal/handler/github.go | 34 +++++++- server/internal/handler/github_test.go | 105 +++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 8 deletions(-) diff --git a/packages/core/types/github.ts b/packages/core/types/github.ts index dafbca2fe8..4a67fd2393 100644 --- a/packages/core/types/github.ts +++ b/packages/core/types/github.ts @@ -14,7 +14,10 @@ export type GitHubMergeableState = string; export interface GitHubInstallation { id: string; workspace_id: string; - installation_id: number; + /** GitHub's numeric installation id — the management handle used by the + * connect / disconnect flows. Omitted when the caller cannot manage + * integrations (see `ListGitHubInstallationsResponse.can_manage`). */ + installation_id?: number; account_login: string; account_type: "User" | "Organization"; account_avatar_url: string | null; @@ -57,6 +60,11 @@ export interface ListGitHubInstallationsResponse { installations: GitHubInstallation[]; /** Whether the deployment has GitHub App credentials configured. When false, the Connect button is hidden / disabled. */ configured: boolean; + /** Whether the caller can connect / disconnect installations. Non-admin + * members get `false` along with installations that omit `installation_id`. + * Older backends predating MUL-2413 omit the field; treat absence as + * `false` for read-only safety. */ + can_manage?: boolean; } export interface GitHubConnectResponse { diff --git a/server/cmd/server/router.go b/server/cmd/server/router.go index 647d7903bd..f021f37dcf 100644 --- a/server/cmd/server/router.go +++ b/server/cmd/server/router.go @@ -326,6 +326,11 @@ func NewRouterWithOptions(pool *pgxpool.Pool, hub *realtime.Hub, bus *events.Bus r.Get("/members", h.ListMembersWithUser) r.Post("/leave", h.LeaveWorkspace) r.Get("/invitations", h.ListWorkspaceInvitations) + // Listing GitHub installations is member-visible so the + // integrations tab no longer renders blank for non-admins; + // the handler strips the management handle and adds a + // can_manage hint so the UI can gate connect/disconnect. + r.Get("/github/installations", h.ListGitHubInstallations) }) // Admin-level access r.Group(func(r chi.Router) { @@ -342,12 +347,12 @@ func NewRouterWithOptions(pool *pgxpool.Pool, hub *realtime.Hub, bus *events.Bus // Owner-only access r.With(middleware.RequireWorkspaceRoleFromURL(queries, "id", "owner")).Delete("/", h.DeleteWorkspace) - // GitHub integration — admin-only operations live here so the - // nesting matches the rest of /api/workspaces/{id}/* routes. + // GitHub integration — connect / disconnect remain admin-only; + // the read-only list endpoint lives in the member-level group + // above so non-admins can see the workspace's connection state. r.Group(func(r chi.Router) { r.Use(middleware.RequireWorkspaceRoleFromURL(queries, "id", "owner", "admin")) r.Get("/github/connect", h.GitHubConnect) - r.Get("/github/installations", h.ListGitHubInstallations) r.Delete("/github/installations/{installationId}", h.DeleteGitHubInstallation) }) }) diff --git a/server/internal/handler/github.go b/server/internal/handler/github.go index 1eeda519e2..93247154d2 100644 --- a/server/internal/handler/github.go +++ b/server/internal/handler/github.go @@ -22,16 +22,24 @@ import ( "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" + "github.com/multica-ai/multica/server/internal/middleware" db "github.com/multica-ai/multica/server/pkg/db/generated" "github.com/multica-ai/multica/server/pkg/protocol" ) // ── Response shapes ───────────────────────────────────────────────────────── +// GitHubInstallationResponse is the JSON shape returned by the installation +// list endpoint and broadcast on installation-related WS events. +// +// InstallationID is admin-only: the numeric GitHub installation_id is the +// management handle used by the Connect/Disconnect flows, so non-admin +// members receive responses with the field omitted. See the list handler +// for the role gate. type GitHubInstallationResponse struct { ID string `json:"id"` WorkspaceID string `json:"workspace_id"` - InstallationID int64 `json:"installation_id"` + InstallationID *int64 `json:"installation_id,omitempty"` AccountLogin string `json:"account_login"` AccountType string `json:"account_type"` AccountAvatarURL *string `json:"account_avatar_url"` @@ -83,10 +91,11 @@ type GitHubConnectResponse struct { } func githubInstallationToResponse(i db.GithubInstallation) GitHubInstallationResponse { + instID := i.InstallationID return GitHubInstallationResponse{ ID: uuidToString(i.ID), WorkspaceID: uuidToString(i.WorkspaceID), - InstallationID: i.InstallationID, + InstallationID: &instID, AccountLogin: i.AccountLogin, AccountType: i.AccountType, AccountAvatarURL: textToPtr(i.AccountAvatarUrl), @@ -378,12 +387,21 @@ func fetchInstallationAccount(ctx context.Context, installationID int64) (login, // ── Listing / disconnect ──────────────────────────────────────────────────── +// ListGitHubInstallations returns the workspace's connected GitHub +// installations to any workspace member. Connect/disconnect remain +// admin-only at the router level, so the response carries a `can_manage` +// hint and strips the numeric `installation_id` for non-admin callers — +// they get visibility into "is GitHub wired up, and by whom?" without the +// management handle. func (h *Handler) ListGitHubInstallations(w http.ResponseWriter, r *http.Request) { workspaceID := chi.URLParam(r, "id") wsUUID, ok := parseUUIDOrBadRequest(w, workspaceID, "workspace id") if !ok { return } + member, _ := middleware.MemberFromContext(r.Context()) + canManage := roleAllowed(member.Role, "owner", "admin") + rows, err := h.Queries.ListGitHubInstallationsByWorkspace(r.Context(), wsUUID) if err != nil { writeError(w, http.StatusInternalServerError, "failed to list installations") @@ -391,9 +409,17 @@ func (h *Handler) ListGitHubInstallations(w http.ResponseWriter, r *http.Request } out := make([]GitHubInstallationResponse, 0, len(rows)) for _, row := range rows { - out = append(out, githubInstallationToResponse(row)) + resp := githubInstallationToResponse(row) + if !canManage { + resp.InstallationID = nil + } + out = append(out, resp) } - writeJSON(w, http.StatusOK, map[string]any{"installations": out, "configured": isGitHubConfigured()}) + writeJSON(w, http.StatusOK, map[string]any{ + "installations": out, + "configured": isGitHubConfigured(), + "can_manage": canManage, + }) } func (h *Handler) DeleteGitHubInstallation(w http.ResponseWriter, r *http.Request) { diff --git a/server/internal/handler/github_test.go b/server/internal/handler/github_test.go index 2f31017c7b..4f82f580d3 100644 --- a/server/internal/handler/github_test.go +++ b/server/internal/handler/github_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/multica-ai/multica/server/internal/middleware" db "github.com/multica-ai/multica/server/pkg/db/generated" ) @@ -1038,3 +1039,107 @@ func TestWebhook_PullRequest_MetadataPreservesMergeable(t *testing.T) { t.Errorf("expected mergeable_state preserved as clean after metadata event, got %+v", rows[0].MergeableState) } } + +// TestListGitHubInstallations_RoleGating covers the read-only relaxation +// in MUL-2413: the endpoint is now reachable by any workspace member, but +// the handler strips the numeric installation_id and reports `can_manage` +// based on the caller's role. Admins / owners still receive the full row. +func TestListGitHubInstallations_RoleGating(t *testing.T) { + if testHandler == nil { + t.Skip("handler test fixture not initialized (no DB?)") + } + ctx := context.Background() + + const installationID int64 = 42424242 + if _, err := testHandler.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: parseUUID(testWorkspaceID), + InstallationID: installationID, + AccountLogin: "role-gating-acct", + AccountType: "Organization", + }); err != nil { + t.Fatalf("CreateGitHubInstallation: %v", err) + } + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM github_installation WHERE workspace_id = $1`, testWorkspaceID) + }) + + call := func(t *testing.T, role string) map[string]any { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/workspaces/"+testWorkspaceID+"/github/installations", nil) + req = withURLParam(req, "id", testWorkspaceID) + req = req.WithContext(middleware.SetMemberContext(req.Context(), testWorkspaceID, db.Member{Role: role})) + w := httptest.NewRecorder() + testHandler.ListGitHubInstallations(w, req) + if w.Code != http.StatusOK { + t.Fatalf("ListGitHubInstallations(%s): %d %s", role, w.Code, w.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body (%s): %v", role, err) + } + return body + } + + t.Run("admin sees installation_id + can_manage true", func(t *testing.T) { + body := call(t, "admin") + if got, _ := body["can_manage"].(bool); !got { + t.Errorf("can_manage = %v, want true", body["can_manage"]) + } + installs, _ := body["installations"].([]any) + if len(installs) == 0 { + t.Fatalf("expected at least one installation row, got %v", installs) + } + row, _ := installs[0].(map[string]any) + gotID, ok := row["installation_id"].(float64) + if !ok { + t.Fatalf("admin response missing installation_id: %v", row) + } + if int64(gotID) != installationID { + t.Errorf("installation_id = %v, want %d", gotID, installationID) + } + }) + + t.Run("owner sees installation_id + can_manage true", func(t *testing.T) { + body := call(t, "owner") + if got, _ := body["can_manage"].(bool); !got { + t.Errorf("can_manage = %v, want true", body["can_manage"]) + } + installs, _ := body["installations"].([]any) + row, _ := installs[0].(map[string]any) + if _, ok := row["installation_id"]; !ok { + t.Errorf("owner response missing installation_id: %v", row) + } + }) + + t.Run("member sees row without installation_id and can_manage false", func(t *testing.T) { + body := call(t, "member") + canManage, _ := body["can_manage"].(bool) + if canManage { + t.Errorf("can_manage = true, want false for non-admin member") + } + installs, _ := body["installations"].([]any) + if len(installs) == 0 { + t.Fatalf("member should still see installation rows, got %v", installs) + } + row, _ := installs[0].(map[string]any) + if _, present := row["installation_id"]; present { + t.Errorf("installation_id must be omitted for non-admin members, row=%v", row) + } + // Display fields the read-only view still needs must round-trip. + if got, _ := row["account_login"].(string); got != "role-gating-acct" { + t.Errorf("account_login = %q, want role-gating-acct", got) + } + }) + + t.Run("guest is treated as read-only and can_manage is false", func(t *testing.T) { + body := call(t, "guest") + if canManage, _ := body["can_manage"].(bool); canManage { + t.Errorf("can_manage = true, want false for guest") + } + installs, _ := body["installations"].([]any) + row, _ := installs[0].(map[string]any) + if _, present := row["installation_id"]; present { + t.Errorf("installation_id must be omitted for guest, row=%v", row) + } + }) +}