mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-25 12:05:06 +02:00
feat(github): expose read-only installation list to workspace members (MUL-2413)
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 <github@multica.ai>
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user