Review solo Heroku accounts via app collaborators

A personal Heroku account has no Team, so GET /teams returns an empty list and the team-members path has nothing to fetch. Solo accounts grant access per app, so enumerate the user's personal apps and collect each app's owner plus its collaborators, deduped by Heroku user ID. An empty teamID (or the reserved personal-account slug) now selects this personal mode. Factor the Range/Next-Range pagination shared by the members, apps, and collaborators endpoints into herokuListAll. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
2026-05-28 22:22:46 +02:00
parent 558d40fb46
commit 05fc9508ec
2 changed files with 364 additions and 72 deletions
--- a/pkg/accessreview/drivers/heroku.go
+++ b/pkg/accessreview/drivers/heroku.go
@@ -25,9 +25,28 @@ import (
 	"go.probo.inc/probo/pkg/coredata"
 )

-// HerokuDriver fetches team members from the Heroku Platform API using
-// a pre-authenticated HTTP client (Bearer token). Pagination is via
-// Heroku's Range / Next-Range header pair (RFC 7233 style).
+const (
+	// herokuPersonalAccountSlug is the reserved org-picker slug for a
+	// personal Heroku account (one with no Team). Heroku Teams are an
+	// opt-in paid construct, so a solo account has nothing in GET /teams;
+	// selecting this entry runs the driver in personal mode (app owner +
+	// collaborators) instead of team-member mode.
+	herokuPersonalAccountSlug = "@personal"
+
+	// herokuPersonalAccountDisplayName is the picker label and source name
+	// shown for a personal Heroku account.
+	herokuPersonalAccountDisplayName = "Personal account"
+)
+
+// HerokuDriver fetches members from the Heroku Platform API using a
+// pre-authenticated HTTP client (Bearer token). Pagination is via Heroku's
+// Range / Next-Range header pair (RFC 7233 style).
+//
+// The driver runs in one of two modes:
+//   - team mode (teamID set): list the members of GET /teams/{id}/members.
+//   - personal mode (teamID empty or the personal-account slug): a solo
+//     Heroku account has no Team, so access is granted per-app; enumerate
+//     the personal apps' owners and collaborators instead.
 //
 // Notes on data quality:
 //   - The team-members endpoint does not expose suspension state, so
@@ -36,6 +55,8 @@ import (
 //     the API still reports `two_factor_authentication`. The driver
 //     populates MFAStatus from that field and lets the access-review
 //     UI surface federation context separately.
+//   - The collaborators endpoint does not expose MFA, so personal-mode
+//     records leave MFAStatus unknown.
 type HerokuDriver struct {
 	httpClient *http.Client
 	teamID     string
@@ -69,101 +90,239 @@ type herokuTeamMember struct {
 	} `json:"user"`
 }

-func (d *HerokuDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error) {
-	var records []AccountRecord
+type herokuApp struct {
+	ID    string `json:"id"`
+	Owner struct {
+		ID    string `json:"id"`
+		Email string `json:"email"`
+	} `json:"owner"`
+	// Team is nil for personal apps and set for team-owned apps; we use it
+	// to keep personal mode scoped to the user's own apps.
+	Team *struct {
+		ID string `json:"id"`
+	} `json:"team"`
+}

+type herokuCollaborator struct {
+	Role      string `json:"role"`
+	CreatedAt string `json:"created_at"`
+	User      struct {
+		ID    string `json:"id"`
+		Email string `json:"email"`
+	} `json:"user"`
+}
+
+func (d *HerokuDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error) {
+	if d.teamID == "" || d.teamID == herokuPersonalAccountSlug {
+		return d.listPersonalAccounts(ctx)
+	}
+
+	return d.listTeamMembers(ctx)
+}
+
+func (d *HerokuDriver) listTeamMembers(ctx context.Context) ([]AccountRecord, error) {
 	endpoint, err := url.JoinPath("https://api.heroku.com", "teams", url.PathEscape(d.teamID), "members")
 	if err != nil {
 		return nil, fmt.Errorf("cannot build heroku members URL: %w", err)
 	}

-	rangeHeader := ""
+	members, err := herokuListAll[herokuTeamMember](ctx, d.httpClient, endpoint, "members")
+	if err != nil {
+		return nil, fmt.Errorf("cannot list heroku team members: %w", err)
+	}

-	for range maxPaginationPages {
-		members, nextRange, err := d.queryMembers(ctx, endpoint, rangeHeader)
-		if err != nil {
-			return nil, err
+	var records []AccountRecord
+
+	for _, m := range members {
+		email := m.Email
+		if email == "" {
+			email = m.User.Email
 		}

-		for _, m := range members {
-			email := m.Email
-			if email == "" {
-				email = m.User.Email
+		fullName := m.User.Name
+
+		mfaStatus := coredata.MFAStatusDisabled
+		if m.TwoFactorAuthentication {
+			mfaStatus = coredata.MFAStatusEnabled
+		}
+
+		isAdmin := m.Role == "admin" || m.Role == "owner"
+
+		externalID := m.User.ID
+		if externalID == "" {
+			externalID = m.ID
+		}
+
+		record := AccountRecord{
+			Email:       email,
+			FullName:    fullName,
+			Role:        m.Role,
+			IsAdmin:     isAdmin,
+			MFAStatus:   mfaStatus,
+			AuthMethod:  coredata.AccessEntryAuthMethodUnknown,
+			AccountType: coredata.AccessEntryAccountTypeUser,
+			ExternalID:  externalID,
+		}
+
+		if m.CreatedAt != "" {
+			if t, err := time.Parse(time.RFC3339, m.CreatedAt); err == nil {
+				record.CreatedAt = &t
+			}
+		}
+
+		records = append(records, record)
+	}
+
+	return records, nil
+}
+
+// listPersonalAccounts enumerates the people with access to a personal
+// (non-team) Heroku account: the owner of each personal app plus every
+// collaborator on it, deduplicated by Heroku user ID. Personal accounts
+// have no Team, so there are no team members to list; access is granted
+// per-app via collaborators.
+func (d *HerokuDriver) listPersonalAccounts(ctx context.Context) ([]AccountRecord, error) {
+	// GET /apps returns every app the token can see, both owned and
+	// collaborated-on. We skip team-owned apps below (those belong to a
+	// team-scoped review); the remaining personal apps are the account
+	// under review. Scoping strictly to apps the connector owns would need
+	// the account ID from GET /account, which requires the identity OAuth
+	// scope we deliberately do not request.
+	apps, err := herokuListAll[herokuApp](ctx, d.httpClient, "https://api.heroku.com/apps", "apps")
+	if err != nil {
+		return nil, fmt.Errorf("cannot list heroku personal apps: %w", err)
+	}
+
+	var records []AccountRecord
+
+	// Dedupe by Heroku user ID across all apps: a user collaborating on
+	// several apps is one account in the review. An admin grant on any
+	// app wins.
+	seen := make(map[string]int)
+
+	upsert := func(rec AccountRecord) {
+		key := rec.ExternalID
+		if key == "" {
+			key = rec.Email
+		}
+
+		if key == "" {
+			return
+		}
+
+		if i, ok := seen[key]; ok {
+			if rec.IsAdmin {
+				records[i].IsAdmin = true
 			}

-			fullName := m.User.Name
+			return
+		}

-			mfaStatus := coredata.MFAStatusDisabled
-			if m.TwoFactorAuthentication {
-				mfaStatus = coredata.MFAStatusEnabled
-			}
+		seen[key] = len(records)
+		records = append(records, rec)
+	}

-			isAdmin := m.Role == "admin" || m.Role == "owner"
+	for _, app := range apps {
+		// Skip team-owned apps; those belong to a team-scoped review.
+		if app.Team != nil {
+			continue
+		}

-			externalID := m.User.ID
-			if externalID == "" {
-				externalID = m.ID
-			}
+		// The owner always has access, whether or not they also appear in
+		// the collaborators list.
+		upsert(herokuPersonalRecord(app.Owner.ID, app.Owner.Email, "owner", true))

-			record := AccountRecord{
-				Email:       email,
-				FullName:    fullName,
-				Role:        m.Role,
-				IsAdmin:     isAdmin,
-				MFAStatus:   mfaStatus,
-				AuthMethod:  coredata.AccessEntryAuthMethodUnknown,
-				AccountType: coredata.AccessEntryAccountTypeUser,
-				ExternalID:  externalID,
-			}
+		endpoint, err := url.JoinPath("https://api.heroku.com", "apps", url.PathEscape(app.ID), "collaborators")
+		if err != nil {
+			return nil, fmt.Errorf("cannot build heroku collaborators URL: %w", err)
+		}

-			if m.CreatedAt != "" {
-				if t, err := time.Parse(time.RFC3339, m.CreatedAt); err == nil {
+		collaborators, err := herokuListAll[herokuCollaborator](ctx, d.httpClient, endpoint, "collaborators")
+		if err != nil {
+			return nil, fmt.Errorf("cannot list heroku collaborators for app %q: %w", app.ID, err)
+		}
+
+		for _, c := range collaborators {
+			record := herokuPersonalRecord(c.User.ID, c.User.Email, c.Role, c.Role == "admin" || c.Role == "owner")
+
+			if c.CreatedAt != "" {
+				if t, err := time.Parse(time.RFC3339, c.CreatedAt); err == nil {
 					record.CreatedAt = &t
 				}
 			}

-			records = append(records, record)
+			upsert(record)
+		}
+	}
+
+	return records, nil
+}
+
+// herokuPersonalRecord builds an AccountRecord for a person with access to a
+// personal Heroku app. These endpoints expose no display name or MFA signal,
+// so the email doubles as the full name and MFA is left unknown.
+func herokuPersonalRecord(externalID, email, role string, isAdmin bool) AccountRecord {
+	return AccountRecord{
+		Email:       email,
+		FullName:    email,
+		Role:        role,
+		IsAdmin:     isAdmin,
+		MFAStatus:   coredata.MFAStatusUnknown,
+		AuthMethod:  coredata.AccessEntryAuthMethodUnknown,
+		AccountType: coredata.AccessEntryAccountTypeUser,
+		ExternalID:  externalID,
+	}
+}
+
+// herokuListAll fetches every page of a Heroku collection endpoint,
+// following the Range / Next-Range pagination header pair, and decodes each
+// page into T. label names the resource in error messages (e.g. "members").
+func herokuListAll[T any](ctx context.Context, client *http.Client, endpoint, label string) ([]T, error) {
+	var all []T
+
+	rangeHeader := ""
+
+	for range maxPaginationPages {
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil)
+		if err != nil {
+			return nil, fmt.Errorf("cannot create heroku %s request: %w", label, err)
 		}

+		req.Header.Set("Accept", "application/vnd.heroku+json; version=3")
+
+		if rangeHeader != "" {
+			req.Header.Set("Range", rangeHeader)
+		}
+
+		httpResp, err := client.Do(req)
+		if err != nil {
+			return nil, fmt.Errorf("cannot execute heroku %s request: %w", label, err)
+		}
+
+		// Heroku returns 206 Partial Content for ranged responses with more
+		// pages, and 200 OK for the final/only page.
+		if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 {
+			_ = httpResp.Body.Close()
+			return nil, fmt.Errorf("cannot fetch heroku %s: unexpected status %d", label, httpResp.StatusCode)
+		}
+
+		var page []T
+		if err := json.NewDecoder(httpResp.Body).Decode(&page); err != nil {
+			_ = httpResp.Body.Close()
+			return nil, fmt.Errorf("cannot decode heroku %s response: %w", label, err)
+		}
+
+		nextRange := httpResp.Header.Get("Next-Range")
+		_ = httpResp.Body.Close()
+
+		all = append(all, page...)
+
 		if nextRange == "" {
-			return records, nil
+			return all, nil
 		}

 		rangeHeader = nextRange
 	}

-	return nil, fmt.Errorf("cannot list all heroku accounts: %w", ErrPaginationLimitReached)
-}
-
-func (d *HerokuDriver) queryMembers(ctx context.Context, endpoint, rangeHeader string) ([]herokuTeamMember, string, error) {
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil)
-	if err != nil {
-		return nil, "", fmt.Errorf("cannot create heroku members request: %w", err)
-	}
-
-	req.Header.Set("Accept", "application/vnd.heroku+json; version=3")
-
-	if rangeHeader != "" {
-		req.Header.Set("Range", rangeHeader)
-	}
-
-	httpResp, err := d.httpClient.Do(req)
-	if err != nil {
-		return nil, "", fmt.Errorf("cannot execute heroku members request: %w", err)
-	}
-
-	defer func() { _ = httpResp.Body.Close() }()
-
-	// Heroku returns 206 Partial Content for ranged responses with more
-	// pages, and 200 OK for the final/only page.
-	if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 {
-		return nil, "", fmt.Errorf("cannot fetch heroku members: unexpected status %d", httpResp.StatusCode)
-	}
-
-	var members []herokuTeamMember
-	if err := json.NewDecoder(httpResp.Body).Decode(&members); err != nil {
-		return nil, "", fmt.Errorf("cannot decode heroku members response: %w", err)
-	}
-
-	return members, httpResp.Header.Get("Next-Range"), nil
+	return nil, fmt.Errorf("cannot list all heroku %s: %w", label, ErrPaginationLimitReached)
 }
--- a/pkg/accessreview/drivers/heroku_test.go
+++ b/pkg/accessreview/drivers/heroku_test.go
@@ -16,6 +16,8 @@ package drivers

 import (
 	"context"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"testing"

@@ -49,3 +51,134 @@ func TestHerokuDriver(t *testing.T) {
 	assert.True(t, r.IsAdmin)
 	require.NotNil(t, r.CreatedAt)
 }
+
+// TestHerokuDriverPersonalAccount exercises personal mode (empty teamID): a
+// solo account with no Team is reviewed via its apps' owner + collaborators.
+// It verifies the owner is always included, collaborators are deduped across
+// apps, and team-owned apps are skipped.
+func TestHerokuDriverPersonalAccount(t *testing.T) {
+	t.Parallel()
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		assert.Equal(t, http.MethodGet, r.Method)
+		w.Header().Set("Content-Type", "application/json")
+
+		switch r.URL.Path {
+		case "/apps":
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`[
+				{"id":"app-1","name":"one","owner":{"id":"u-alice","email":"alice@example.com"},"team":null},
+				{"id":"app-2","name":"two","owner":{"id":"u-alice","email":"alice@example.com"},"team":null},
+				{"id":"app-3","name":"teamed","owner":{"id":"u-x","email":"x@example.com"},"team":{"id":"team-1"}}
+			]`))
+		case "/apps/app-1/collaborators":
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`[
+				{"id":"c-1","role":"owner","user":{"id":"u-alice","email":"alice@example.com"}},
+				{"id":"c-2","role":"member","user":{"id":"u-bob","email":"bob@example.com"}}
+			]`))
+		case "/apps/app-2/collaborators":
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`[
+				{"id":"c-3","role":"member","user":{"id":"u-bob","email":"bob@example.com"}},
+				{"id":"c-4","role":"member","user":{"id":"u-carol","email":"carol@example.com"}}
+			]`))
+		default:
+			t.Errorf("unexpected request to %s", r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer srv.Close()
+
+	client := &http.Client{Transport: &hostRewriter{target: srv.URL}}
+
+	records, err := NewHerokuDriver(client, "").ListAccounts(context.Background())
+	require.NoError(t, err)
+
+	byEmail := make(map[string]AccountRecord, len(records))
+	for _, r := range records {
+		byEmail[r.Email] = r
+	}
+
+	require.Len(t, records, 3)
+	assert.Contains(t, byEmail, "alice@example.com")
+	assert.Contains(t, byEmail, "bob@example.com")
+	assert.Contains(t, byEmail, "carol@example.com")
+
+	assert.True(t, byEmail["alice@example.com"].IsAdmin)
+	assert.Equal(t, "owner", byEmail["alice@example.com"].Role)
+	assert.False(t, byEmail["bob@example.com"].IsAdmin)
+}
+
+// TestHerokuDriverPersonalAccountSlug verifies the reserved personal-account
+// slug routes to personal mode just like an empty teamID.
+func TestHerokuDriverPersonalAccountSlug(t *testing.T) {
+	t.Parallel()
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+
+		switch r.URL.Path {
+		case "/apps":
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`[{"id":"app-1","name":"one","owner":{"id":"u-alice","email":"alice@example.com"},"team":null}]`))
+		case "/apps/app-1/collaborators":
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`[]`))
+		default:
+			t.Errorf("unexpected request to %s", r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer srv.Close()
+
+	client := &http.Client{Transport: &hostRewriter{target: srv.URL}}
+
+	records, err := NewHerokuDriver(client, herokuPersonalAccountSlug).ListAccounts(context.Background())
+	require.NoError(t, err)
+	require.Len(t, records, 1)
+	assert.Equal(t, "alice@example.com", records[0].Email)
+}
+
+// TestHerokuDriverPersonalAccountErrors verifies non-2xx responses on the
+// personal-mode endpoints propagate as errors rather than empty results.
+func TestHerokuDriverPersonalAccountErrors(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name      string
+		appsCode  int
+		collabFor string
+	}{
+		{name: "apps list fails", appsCode: http.StatusInternalServerError},
+		{name: "collaborators fail", appsCode: http.StatusOK, collabFor: "app-1"},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+
+				switch r.URL.Path {
+				case "/apps":
+					w.WriteHeader(tc.appsCode)
+					_, _ = w.Write([]byte(`[{"id":"app-1","name":"one","owner":{"id":"u-alice","email":"alice@example.com"},"team":null}]`))
+				case "/apps/app-1/collaborators":
+					w.WriteHeader(http.StatusForbidden)
+					_, _ = w.Write([]byte(`{"id":"forbidden"}`))
+				default:
+					t.Errorf("unexpected request to %s", r.URL.Path)
+					w.WriteHeader(http.StatusNotFound)
+				}
+			}))
+			defer srv.Close()
+
+			client := &http.Client{Transport: &hostRewriter{target: srv.URL}}
+
+			_, err := NewHerokuDriver(client, "").ListAccounts(context.Background())
+			require.Error(t, err)
+		})
+	}
+}