diff --git a/pkg/accessreview/drivers/heroku.go b/pkg/accessreview/drivers/heroku.go index 5277997e7..292907455 100644 --- 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) } diff --git a/pkg/accessreview/drivers/heroku_test.go b/pkg/accessreview/drivers/heroku_test.go index 78d7bbf7a..743f8badc 100644 --- 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) + }) + } +}