From 8094e7cfd03a7ad9d9e76ba530720168dc5b9a0d Mon Sep 17 00:00:00 2001 From: Ludovic Vielle Date: Fri, 12 Jun 2026 18:23:04 +0200 Subject: [PATCH] Truncate access review roles with badge list Long role strings in the access review table broke row layout when drivers joined many roles into one comma-separated value. Expose roles as a string array in GraphQL by splitting the stored role at the API layer, and render the first three roles as badges with a "+X more" popover for the rest. Closes ENG-459. Signed-off-by: Ludovic Vielle --- .../_components/AccessEntryRolesCell.tsx | 88 +++++++++++++++++++ .../campaigns/CampaignDetailPage.tsx | 5 +- .../dialogs/AddCampaignSourceDialog.tsx | 2 +- pkg/accessreview/drivers/anthropic.go | 20 +++-- pkg/accessreview/drivers/anthropic_test.go | 25 +++--- pkg/accessreview/drivers/better_stack.go | 22 +++-- pkg/accessreview/drivers/better_stack_test.go | 23 ++--- pkg/accessreview/drivers/brex.go | 10 ++- pkg/accessreview/drivers/clickup.go | 18 ++-- pkg/accessreview/drivers/clickup_test.go | 2 +- pkg/accessreview/drivers/cloudflare.go | 8 +- pkg/accessreview/drivers/cloudflare_test.go | 2 +- pkg/accessreview/drivers/csv.go | 9 +- pkg/accessreview/drivers/cursor.go | 16 ++-- pkg/accessreview/drivers/cursor_test.go | 25 +++--- pkg/accessreview/drivers/datadog.go | 11 +-- pkg/accessreview/drivers/datadog_test.go | 4 +- pkg/accessreview/drivers/docusign.go | 9 +- pkg/accessreview/drivers/docusign_test.go | 2 +- pkg/accessreview/drivers/driver.go | 4 +- pkg/accessreview/drivers/github.go | 10 ++- pkg/accessreview/drivers/github_test.go | 2 +- pkg/accessreview/drivers/gitlab.go | 24 ++--- pkg/accessreview/drivers/gitlab_test.go | 2 +- pkg/accessreview/drivers/google_workspace.go | 6 +- .../drivers/google_workspace_test.go | 2 +- pkg/accessreview/drivers/grafana.go | 11 ++- pkg/accessreview/drivers/grafana_test.go | 4 +- pkg/accessreview/drivers/heroku.go | 19 +++- pkg/accessreview/drivers/heroku_test.go | 4 +- pkg/accessreview/drivers/hubspot.go | 76 +++++++++++----- pkg/accessreview/drivers/hubspot_test.go | 61 ++++++++++++- pkg/accessreview/drivers/intercom.go | 8 +- pkg/accessreview/drivers/linear.go | 10 +-- pkg/accessreview/drivers/linear_test.go | 2 +- pkg/accessreview/drivers/metabase.go | 8 +- pkg/accessreview/drivers/metabase_test.go | 4 +- pkg/accessreview/drivers/microsoft_365.go | 41 +-------- pkg/accessreview/drivers/neon.go | 22 +++-- pkg/accessreview/drivers/neon_test.go | 25 +++--- pkg/accessreview/drivers/netlify.go | 10 ++- pkg/accessreview/drivers/netlify_test.go | 2 +- pkg/accessreview/drivers/notion.go | 2 +- pkg/accessreview/drivers/openai.go | 14 +-- pkg/accessreview/drivers/openai_test.go | 2 +- pkg/accessreview/drivers/pagerduty.go | 10 ++- pkg/accessreview/drivers/pagerduty_test.go | 2 +- pkg/accessreview/drivers/posthog.go | 14 +-- pkg/accessreview/drivers/posthog_test.go | 18 ++-- pkg/accessreview/drivers/probo_memberships.go | 11 ++- pkg/accessreview/drivers/qovery.go | 18 ++-- pkg/accessreview/drivers/qovery_test.go | 21 ++--- pkg/accessreview/drivers/render.go | 22 +++-- pkg/accessreview/drivers/render_test.go | 25 +++--- pkg/accessreview/drivers/sendgrid.go | 16 ++-- pkg/accessreview/drivers/sendgrid_test.go | 22 ++--- pkg/accessreview/drivers/sentry.go | 11 ++- pkg/accessreview/drivers/sentry_test.go | 2 +- pkg/accessreview/drivers/signoz.go | 21 ++--- pkg/accessreview/drivers/signoz_test.go | 36 ++++---- pkg/accessreview/drivers/slack.go | 16 ++-- pkg/accessreview/drivers/slack_test.go | 2 +- pkg/accessreview/drivers/supabase.go | 10 ++- pkg/accessreview/drivers/supabase_test.go | 2 +- pkg/accessreview/drivers/tailscale.go | 9 +- pkg/accessreview/drivers/tailscale_test.go | 2 +- pkg/accessreview/drivers/tally.go | 6 +- pkg/accessreview/drivers/vercel.go | 10 ++- pkg/accessreview/drivers/vercel_test.go | 2 +- pkg/accessreview/drivers/zendesk.go | 10 ++- pkg/accessreview/drivers/zendesk_test.go | 6 +- pkg/accessreview/review_engine.go | 2 +- .../access_review_campaign_source_test.go | 2 +- pkg/coredata/access_review_entry.go | 22 ++--- .../access_review_entry_upsert_test.go | 68 ++++++++++++-- pkg/coredata/migrations/20260612T160000Z.sql | 26 ++++++ .../v1/graphql/access_review_campaign.graphql | 2 +- .../api/console/v1/types/access_review.go | 7 +- pkg/server/api/mcp/v1/specification.yaml | 10 ++- pkg/server/api/mcp/v1/types/access_review.go | 7 +- 80 files changed, 768 insertions(+), 378 deletions(-) create mode 100644 apps/console/src/pages/organizations/access-reviews/_components/AccessEntryRolesCell.tsx create mode 100644 pkg/coredata/migrations/20260612T160000Z.sql diff --git a/apps/console/src/pages/organizations/access-reviews/_components/AccessEntryRolesCell.tsx b/apps/console/src/pages/organizations/access-reviews/_components/AccessEntryRolesCell.tsx new file mode 100644 index 000000000..1ee113a8c --- /dev/null +++ b/apps/console/src/pages/organizations/access-reviews/_components/AccessEntryRolesCell.tsx @@ -0,0 +1,88 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +import { Badge, Td } from "@probo/ui"; +import * as Popover from "@radix-ui/react-popover"; +import { graphql, useFragment } from "react-relay"; + +import type { AccessEntryRolesCell_accessEntry$key } from "#/__generated__/core/AccessEntryRolesCell_accessEntry.graphql"; + +import { NotAvailable } from "./accessReviewHelpers"; + +const VISIBLE_ROLE_COUNT = 3; + +const accessEntryRolesCellFragment = graphql` + fragment AccessEntryRolesCell_accessEntry on AccessReviewEntry { + roles + } +`; + +type Props = { + accessEntryKey: AccessEntryRolesCell_accessEntry$key; +}; + +export function AccessEntryRolesCell({ accessEntryKey }: Props) { + const entry = useFragment(accessEntryRolesCellFragment, accessEntryKey); + const roles = entry.roles; + + if (roles.length === 0) { + return ( + + + + ); + } + + const visibleRoles = roles.slice(0, VISIBLE_ROLE_COUNT); + const hiddenRoles = roles.slice(VISIBLE_ROLE_COUNT); + + return ( + +
+ {visibleRoles.map((role, index) => ( + + {role} + + ))} + {hiddenRoles.length > 0 && ( + + + + + + +
+ {hiddenRoles.map((role, index) => ( + + {role} + + ))} +
+ + + + )} +
+ + ); +} diff --git a/apps/console/src/pages/organizations/access-reviews/campaigns/CampaignDetailPage.tsx b/apps/console/src/pages/organizations/access-reviews/campaigns/CampaignDetailPage.tsx index f840d1845..c9f68de90 100644 --- a/apps/console/src/pages/organizations/access-reviews/campaigns/CampaignDetailPage.tsx +++ b/apps/console/src/pages/organizations/access-reviews/campaigns/CampaignDetailPage.tsx @@ -56,6 +56,7 @@ import type { CampaignDetailPageQuery } from "#/__generated__/core/CampaignDetai import type { CampaignDetailPageStartMutation } from "#/__generated__/core/CampaignDetailPageStartMutation.graphql"; import { useOrganizationId } from "#/hooks/useOrganizationId"; +import { AccessEntryRolesCell } from "../_components/AccessEntryRolesCell"; import { decisionBadgeVariant, decisionLabel, @@ -169,7 +170,7 @@ export const campaignDetailPageQuery = graphql` id email fullName - role + ...AccessEntryRolesCell_accessEntry isAdmin active mfaStatus @@ -721,7 +722,7 @@ function CampaignSourceCard({ source, isPendingActions }: { source: CampaignSour {edge.node.email || } - {edge.node.role || } + {edge.node.isAdmin ? __("Yes") : __("No")} {edge.node.active == null diff --git a/apps/console/src/pages/organizations/access-reviews/dialogs/AddCampaignSourceDialog.tsx b/apps/console/src/pages/organizations/access-reviews/dialogs/AddCampaignSourceDialog.tsx index ffa59cc39..46d6e5ee0 100644 --- a/apps/console/src/pages/organizations/access-reviews/dialogs/AddCampaignSourceDialog.tsx +++ b/apps/console/src/pages/organizations/access-reviews/dialogs/AddCampaignSourceDialog.tsx @@ -56,7 +56,7 @@ const addScopeMutation = graphql` id email fullName - role + roles isAdmin mfaStatus lastLogin diff --git a/pkg/accessreview/drivers/anthropic.go b/pkg/accessreview/drivers/anthropic.go index 0c837d858..694a2c17a 100644 --- a/pkg/accessreview/drivers/anthropic.go +++ b/pkg/accessreview/drivers/anthropic.go @@ -71,7 +71,7 @@ func (d *AnthropicDriver) ListAccounts(ctx context.Context) ([]AccountRecord, er record := AccountRecord{ Email: u.Email, FullName: u.Name, - Role: anthropicRole(u.Role), + Roles: anthropicRoles(u.Role), IsAdmin: u.Role == "admin", ExternalID: u.ID, MFAStatus: coredata.MFAStatusUnknown, @@ -141,19 +141,23 @@ func (d *AnthropicDriver) fetchUsers(ctx context.Context, afterID string) (*anth return &resp, nil } -func anthropicRole(role string) string { +func anthropicRoles(role string) []string { + if role == "" { + return []string{} + } + switch role { case "admin": - return "Admin" + return []string{"Admin"} case "billing": - return "Billing" + return []string{"Billing"} case "developer": - return "Developer" + return []string{"Developer"} case "claude_code_user": - return "Claude Code User" + return []string{"Claude Code User"} case "user": - return "User" + return []string{"User"} default: - return role + return []string{role} } } diff --git a/pkg/accessreview/drivers/anthropic_test.go b/pkg/accessreview/drivers/anthropic_test.go index 52b6b9bf1..e374ea5ff 100644 --- a/pkg/accessreview/drivers/anthropic_test.go +++ b/pkg/accessreview/drivers/anthropic_test.go @@ -39,38 +39,39 @@ func TestAnthropicDriver(t *testing.T) { assert.NotEmpty(t, first.Email) assert.NotEmpty(t, first.FullName) assert.NotEmpty(t, first.ExternalID) - assert.Equal(t, "User", first.Role) + assert.Equal(t, []string{"User"}, first.Roles) assert.False(t, first.IsAdmin) assert.NotNil(t, first.CreatedAt) - assert.Equal(t, "Developer", records[1].Role) + assert.Equal(t, []string{"Developer"}, records[1].Roles) assert.False(t, records[1].IsAdmin) admin := records[2] - assert.Equal(t, "Admin", admin.Role) + assert.Equal(t, []string{"Admin"}, admin.Roles) assert.True(t, admin.IsAdmin) } -func TestAnthropicRole(t *testing.T) { +func TestAnthropicRoles(t *testing.T) { t.Parallel() cases := []struct { in string - want string + want []string }{ - {"admin", "Admin"}, - {"billing", "Billing"}, - {"developer", "Developer"}, - {"claude_code_user", "Claude Code User"}, - {"user", "User"}, - {"unknown_future_role", "unknown_future_role"}, + {"admin", []string{"Admin"}}, + {"billing", []string{"Billing"}}, + {"developer", []string{"Developer"}}, + {"claude_code_user", []string{"Claude Code User"}}, + {"user", []string{"User"}}, + {"unknown_future_role", []string{"unknown_future_role"}}, + {"", []string{}}, } for _, c := range cases { t.Run(c.in, func(t *testing.T) { t.Parallel() - assert.Equal(t, c.want, anthropicRole(c.in)) + assert.Equal(t, c.want, anthropicRoles(c.in)) }) } } diff --git a/pkg/accessreview/drivers/better_stack.go b/pkg/accessreview/drivers/better_stack.go index 6f9f18c94..c12ed4bbf 100644 --- a/pkg/accessreview/drivers/better_stack.go +++ b/pkg/accessreview/drivers/better_stack.go @@ -94,7 +94,7 @@ func (d *BetterStackDriver) ListAccounts(ctx context.Context) ([]AccountRecord, record := AccountRecord{ Email: member.Attributes.Email, FullName: strings.TrimSpace(member.Attributes.FirstName + " " + member.Attributes.LastName), - Role: betterStackRole(member.Attributes.Role), + Roles: betterStackRoles(member.Attributes.Role), Active: betterStackActive(member.Type), IsAdmin: betterStackIsAdmin(member.Attributes.Role), MFAStatus: coredata.MFAStatusUnknown, @@ -168,23 +168,27 @@ func (d *BetterStackDriver) fetchTeamMembersPage( return &resp, nil } -// betterStackRole maps a Better Stack role token to a human-readable label. +// betterStackRoles maps a Better Stack role token to a human-readable label. // Unknown roles (including the Enterprise "custom" roles) are passed through // unchanged so the reviewer still sees the source value. -func betterStackRole(role string) string { +func betterStackRoles(role string) []string { + if role == "" { + return []string{} + } + switch role { case "admin": - return "Admin" + return []string{"Admin"} case "billing_admin": - return "Billing admin" + return []string{"Billing admin"} case "team_lead": - return "Team lead" + return []string{"Team lead"} case "responder": - return "Responder" + return []string{"Responder"} case "member": - return "Member" + return []string{"Member"} default: - return role + return []string{role} } } diff --git a/pkg/accessreview/drivers/better_stack_test.go b/pkg/accessreview/drivers/better_stack_test.go index a5d23f02f..a0fd2e4ca 100644 --- a/pkg/accessreview/drivers/better_stack_test.go +++ b/pkg/accessreview/drivers/better_stack_test.go @@ -46,7 +46,7 @@ func TestBetterStackDriver(t *testing.T) { r := records[0] assert.Equal(t, "alice@example.com", r.Email) assert.Equal(t, "Alice Smith", r.FullName) - assert.Equal(t, "Admin", r.Role) + assert.Equal(t, []string{"Admin"}, r.Roles) assert.True(t, r.IsAdmin) assert.Equal(t, "101", r.ExternalID) require.NotNil(t, r.Active) @@ -107,28 +107,29 @@ func TestBetterStackDriverListAccountsError(t *testing.T) { assert.Contains(t, err.Error(), "unexpected status 401") } -func TestBetterStackRole(t *testing.T) { +func TestBetterStackRoles(t *testing.T) { t.Parallel() cases := []struct { in string - want string + want []string isAdmin bool }{ - {"admin", "Admin", true}, - {"billing_admin", "Billing admin", false}, - {"team_lead", "Team lead", true}, - {"responder", "Responder", false}, - {"member", "Member", false}, - {"custom", "custom", false}, - {"future_role", "future_role", false}, + {"admin", []string{"Admin"}, true}, + {"billing_admin", []string{"Billing admin"}, false}, + {"team_lead", []string{"Team lead"}, true}, + {"responder", []string{"Responder"}, false}, + {"member", []string{"Member"}, false}, + {"custom", []string{"custom"}, false}, + {"future_role", []string{"future_role"}, false}, + {"", []string{}, false}, } for _, c := range cases { t.Run(c.in, func(t *testing.T) { t.Parallel() - assert.Equal(t, c.want, betterStackRole(c.in)) + assert.Equal(t, c.want, betterStackRoles(c.in)) assert.Equal(t, c.isAdmin, betterStackIsAdmin(c.in)) }) } diff --git a/pkg/accessreview/drivers/brex.go b/pkg/accessreview/drivers/brex.go index 1a2690c8c..b27d6e152 100644 --- a/pkg/accessreview/drivers/brex.go +++ b/pkg/accessreview/drivers/brex.go @@ -19,6 +19,7 @@ import ( "encoding/json" "fmt" "net/http" + "strings" "go.probo.inc/probo/pkg/coredata" ) @@ -64,10 +65,17 @@ func (d *BrexDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error) } for _, u := range resp.Items { + role := strings.TrimSpace(u.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: u.Email, FullName: u.FirstName + " " + u.LastName, - Role: u.Role, + Roles: roles, Active: new(u.Status == "ACTIVE"), IsAdmin: false, ExternalID: u.ID, diff --git a/pkg/accessreview/drivers/clickup.go b/pkg/accessreview/drivers/clickup.go index c822c1278..b233a7346 100644 --- a/pkg/accessreview/drivers/clickup.go +++ b/pkg/accessreview/drivers/clickup.go @@ -101,13 +101,13 @@ func (d *ClickUpDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro records := make([]AccountRecord, 0, len(resp.Team.Members)) for _, m := range resp.Team.Members { - role := clickupRoleLabel(m.User.Role) + roles := clickupRoles(m.User.Role) isAdmin := m.User.Role == 1 || m.User.Role == 2 record := AccountRecord{ Email: m.User.Email, FullName: m.User.Username, - Role: role, + Roles: roles, IsAdmin: isAdmin, ExternalID: m.User.ID.String(), MFAStatus: coredata.MFAStatusUnknown, @@ -134,20 +134,20 @@ func (d *ClickUpDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro return records, nil } -// clickupRoleLabel maps ClickUp numeric role codes to human-readable +// clickupRoles maps ClickUp numeric role codes to human-readable // labels. Source: https://clickup.com/api (Team Members endpoint). -func clickupRoleLabel(role int) string { +func clickupRoles(role int) []string { switch role { case 1: - return "owner" + return []string{"owner"} case 2: - return "admin" + return []string{"admin"} case 3: - return "member" + return []string{"member"} case 4: - return "guest" + return []string{"guest"} default: - return "" + return []string{} } } diff --git a/pkg/accessreview/drivers/clickup_test.go b/pkg/accessreview/drivers/clickup_test.go index 66038f5fa..0be9d1bfa 100644 --- a/pkg/accessreview/drivers/clickup_test.go +++ b/pkg/accessreview/drivers/clickup_test.go @@ -46,5 +46,5 @@ func TestClickUpDriver(t *testing.T) { assert.NotEmpty(t, r.ExternalID) assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/cloudflare.go b/pkg/accessreview/drivers/cloudflare.go index 17f18526e..c2060ff2e 100644 --- a/pkg/accessreview/drivers/cloudflare.go +++ b/pkg/accessreview/drivers/cloudflare.go @@ -21,7 +21,6 @@ import ( "net/http" "net/url" "strconv" - "strings" "go.probo.inc/probo/pkg/coredata" ) @@ -170,9 +169,8 @@ func (d *CloudflareDriver) queryAllMembers(ctx context.Context, accountID string roles = append(roles, r.Name) } - role := "Member" - if len(roles) > 0 { - role = strings.Join(roles, ", ") + if len(roles) == 0 { + roles = []string{"Member"} } isAdmin := false @@ -192,7 +190,7 @@ func (d *CloudflareDriver) queryAllMembers(ctx context.Context, accountID string record := AccountRecord{ Email: m.User.Email, FullName: m.User.FirstName + " " + m.User.LastName, - Role: role, + Roles: roles, Active: new(m.Status == "accepted"), IsAdmin: isAdmin, ExternalID: m.ID, diff --git a/pkg/accessreview/drivers/cloudflare_test.go b/pkg/accessreview/drivers/cloudflare_test.go index 4a09e2bf7..e5e000ffe 100644 --- a/pkg/accessreview/drivers/cloudflare_test.go +++ b/pkg/accessreview/drivers/cloudflare_test.go @@ -38,5 +38,5 @@ func TestCloudflareDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/csv.go b/pkg/accessreview/drivers/csv.go index 1d210842e..bc3690273 100644 --- a/pkg/accessreview/drivers/csv.go +++ b/pkg/accessreview/drivers/csv.go @@ -83,7 +83,14 @@ func (d *CSVDriver) ListAccounts(_ context.Context) ([]AccountRecord, error) { } if idx, ok := colIndex["role"]; ok && idx < len(row) { - record.Role = strings.TrimSpace(row[idx]) + role := strings.TrimSpace(row[idx]) + + roles := []string{} + if role != "" { + roles = []string{role} + } + + record.Roles = roles } if idx, ok := colIndex["job_title"]; ok && idx < len(row) { diff --git a/pkg/accessreview/drivers/cursor.go b/pkg/accessreview/drivers/cursor.go index 49a836a5d..df1408241 100644 --- a/pkg/accessreview/drivers/cursor.go +++ b/pkg/accessreview/drivers/cursor.go @@ -96,7 +96,7 @@ func (d *CursorDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error records = append(records, AccountRecord{ Email: m.Email, FullName: m.Name, - Role: cursorRole(m.Role), + Roles: cursorRoles(m.Role), Active: &active, IsAdmin: cursorIsAdmin(m.Role), MFAStatus: coredata.MFAStatusUnknown, @@ -116,15 +116,19 @@ func cursorIsAdmin(role string) bool { return role == "owner" || role == "free-owner" } -func cursorRole(role string) string { +func cursorRoles(role string) []string { + if role == "" { + return []string{} + } + switch role { case "owner", "free-owner": - return "Owner" + return []string{"Owner"} case "member": - return "Member" + return []string{"Member"} case "removed": - return "Removed" + return []string{"Removed"} default: - return role + return []string{role} } } diff --git a/pkg/accessreview/drivers/cursor_test.go b/pkg/accessreview/drivers/cursor_test.go index 4c2fb7305..49b24ba01 100644 --- a/pkg/accessreview/drivers/cursor_test.go +++ b/pkg/accessreview/drivers/cursor_test.go @@ -40,7 +40,7 @@ func TestCursorDriver(t *testing.T) { member := records[0] assert.Equal(t, "jane@example.com", member.Email) assert.Equal(t, "Jane Doe", member.FullName) - assert.Equal(t, "Member", member.Role) + assert.Equal(t, []string{"Member"}, member.Roles) assert.False(t, member.IsAdmin) // The Cursor Admin API returns the member id as a string; it is used // verbatim as the stable ExternalID. @@ -49,7 +49,7 @@ func TestCursorDriver(t *testing.T) { assert.True(t, *member.Active) owner := records[1] - assert.Equal(t, "Owner", owner.Role) + assert.Equal(t, []string{"Owner"}, owner.Roles) assert.True(t, owner.IsAdmin) require.NotNil(t, owner.Active) assert.True(t, *owner.Active) @@ -57,7 +57,7 @@ func TestCursorDriver(t *testing.T) { // A removed member (role "removed", isRemoved true) is still returned, // flagged inactive rather than dropped, per the AccountRecord contract. removed := records[2] - assert.Equal(t, "Removed", removed.Role) + assert.Equal(t, []string{"Removed"}, removed.Roles) assert.False(t, removed.IsAdmin) require.NotNil(t, removed.Active) assert.False(t, *removed.Active) @@ -66,32 +66,33 @@ func TestCursorDriver(t *testing.T) { // carry role "removed" while isRemoved is still false. The role alone // must mark the account inactive. removedByRole := records[3] - assert.Equal(t, "Removed", removedByRole.Role) + assert.Equal(t, []string{"Removed"}, removedByRole.Roles) assert.False(t, removedByRole.IsAdmin) require.NotNil(t, removedByRole.Active) assert.False(t, *removedByRole.Active) } -func TestCursorRole(t *testing.T) { +func TestCursorRoles(t *testing.T) { t.Parallel() cases := []struct { in string - want string + want []string isAdmin bool }{ - {"owner", "Owner", true}, - {"free-owner", "Owner", true}, - {"member", "Member", false}, - {"removed", "Removed", false}, - {"unknown_future_role", "unknown_future_role", false}, + {"owner", []string{"Owner"}, true}, + {"free-owner", []string{"Owner"}, true}, + {"member", []string{"Member"}, false}, + {"removed", []string{"Removed"}, false}, + {"unknown_future_role", []string{"unknown_future_role"}, false}, + {"", []string{}, false}, } for _, c := range cases { t.Run(c.in, func(t *testing.T) { t.Parallel() - assert.Equal(t, c.want, cursorRole(c.in)) + assert.Equal(t, c.want, cursorRoles(c.in)) assert.Equal(t, c.isAdmin, cursorIsAdmin(c.in)) }) } diff --git a/pkg/accessreview/drivers/datadog.go b/pkg/accessreview/drivers/datadog.go index 8b1a4e2bf..cfa3fb796 100644 --- a/pkg/accessreview/drivers/datadog.go +++ b/pkg/accessreview/drivers/datadog.go @@ -107,19 +107,20 @@ func (d *DatadogDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro active := !u.Attributes.Disabled var ( - role string + roles []string isAdmin bool ) for _, r := range u.Relationships.Roles.Data { name := roleNames[r.ID] - if role == "" { - role = name + if name == "" { + continue } + roles = append(roles, name) + if strings.Contains(strings.ToLower(name), "admin") { isAdmin = true - role = name } } @@ -136,7 +137,7 @@ func (d *DatadogDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro records = append(records, AccountRecord{ Email: u.Attributes.Email, FullName: u.Attributes.Name, - Role: role, + Roles: roles, JobTitle: u.Attributes.Title, Active: &active, IsAdmin: isAdmin, diff --git a/pkg/accessreview/drivers/datadog_test.go b/pkg/accessreview/drivers/datadog_test.go index a03604a88..facca0bf2 100644 --- a/pkg/accessreview/drivers/datadog_test.go +++ b/pkg/accessreview/drivers/datadog_test.go @@ -42,7 +42,7 @@ func TestDatadogDriver(t *testing.T) { require.NotNil(t, r.Active) assert.True(t, *r.Active) assert.True(t, r.IsAdmin) - assert.Equal(t, "Datadog Admin Role", r.Role) + assert.Equal(t, []string{"Datadog Admin Role"}, r.Roles) assert.Equal(t, "Security Engineer", r.JobTitle) assert.Equal(t, coredata.AccessReviewEntryAccountTypeUser, r.AccountType) assert.Equal(t, coredata.MFAStatusEnabled, r.MFAStatus) @@ -56,7 +56,7 @@ func TestDatadogDriver(t *testing.T) { require.NotNil(t, r2.Active) assert.False(t, *r2.Active) assert.False(t, r2.IsAdmin) - assert.Equal(t, "Datadog Standard Role", r2.Role) + assert.Equal(t, []string{"Datadog Standard Role"}, r2.Roles) assert.Equal(t, coredata.AccessReviewEntryAccountTypeServiceAccount, r2.AccountType) assert.Equal(t, coredata.MFAStatusDisabled, r2.MFAStatus) } diff --git a/pkg/accessreview/drivers/docusign.go b/pkg/accessreview/drivers/docusign.go index 290cda806..53f7de817 100644 --- a/pkg/accessreview/drivers/docusign.go +++ b/pkg/accessreview/drivers/docusign.go @@ -90,10 +90,17 @@ func (d *DocuSignDriver) ListAccounts(ctx context.Context) ([]AccountRecord, err } for _, u := range resp.Users { + role := strings.TrimSpace(u.PermissionProfileName) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: u.Email, FullName: u.UserName, - Role: u.PermissionProfileName, + Roles: roles, JobTitle: u.JobTitle, Active: new(strings.EqualFold(u.UserStatus, "active")), IsAdmin: strings.EqualFold(u.IsAdmin, "True"), diff --git a/pkg/accessreview/drivers/docusign_test.go b/pkg/accessreview/drivers/docusign_test.go index f1344937d..cf6ac3e3b 100644 --- a/pkg/accessreview/drivers/docusign_test.go +++ b/pkg/accessreview/drivers/docusign_test.go @@ -38,5 +38,5 @@ func TestDocuSignDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/driver.go b/pkg/accessreview/drivers/driver.go index 50107f7fe..0911e45d2 100644 --- a/pkg/accessreview/drivers/driver.go +++ b/pkg/accessreview/drivers/driver.go @@ -37,8 +37,8 @@ import ( type AccountRecord struct { Email string FullName string - Role string // system role/permission (e.g. "Admin", "Viewer") - JobTitle string // HR job title / department (e.g. "Software Engineer") + Roles []string // system roles/permissions (e.g. "Admin", "Viewer") + JobTitle string // HR job title / department (e.g. "Software Engineer") Active *bool IsAdmin bool MFAStatus coredata.MFAStatus diff --git a/pkg/accessreview/drivers/github.go b/pkg/accessreview/drivers/github.go index 2a1dd7749..b67451958 100644 --- a/pkg/accessreview/drivers/github.go +++ b/pkg/accessreview/drivers/github.go @@ -21,6 +21,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "time" "go.gearno.de/kit/log" @@ -123,10 +124,17 @@ func (d *GitHubDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error } } + role := strings.TrimSpace(membership.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: profile.Email, FullName: fullName, - Role: membership.Role, + Roles: roles, Active: new(membership.State == "active"), IsAdmin: membership.Role == "admin", MFAStatus: mfaStatus, diff --git a/pkg/accessreview/drivers/github_test.go b/pkg/accessreview/drivers/github_test.go index 374920cf8..937644ef9 100644 --- a/pkg/accessreview/drivers/github_test.go +++ b/pkg/accessreview/drivers/github_test.go @@ -43,5 +43,5 @@ func TestGitHubDriver(t *testing.T) { r := records[0] assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/gitlab.go b/pkg/accessreview/drivers/gitlab.go index 3dbfe4271..e7ee79978 100644 --- a/pkg/accessreview/drivers/gitlab.go +++ b/pkg/accessreview/drivers/gitlab.go @@ -96,12 +96,12 @@ func (d *GitLabDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error active := m.State == "active" - role := gitlabAccessLevelLabel(m.AccessLevel) + roles := gitlabRoles(m.AccessLevel) record := AccountRecord{ Email: m.Email, FullName: fullName, - Role: role, + Roles: roles, Active: &active, IsAdmin: m.AccessLevel >= 50, // 50 = Owner MFAStatus: coredata.MFAStatusUnknown, @@ -149,25 +149,25 @@ func (d *GitLabDriver) queryMembers(ctx context.Context, endpoint string) ([]git return members, httpResp.Header.Get("Link"), nil } -// gitlabAccessLevelLabel maps GitLab numeric access levels to human +// gitlabRoles maps GitLab numeric access levels to human // labels. Source: https://docs.gitlab.com/api/members/#roles -func gitlabAccessLevelLabel(level int) string { +func gitlabRoles(level int) []string { switch level { case 5: - return "Minimal Access" + return []string{"Minimal Access"} case 10: - return "Guest" + return []string{"Guest"} case 15: - return "Planner" + return []string{"Planner"} case 20: - return "Reporter" + return []string{"Reporter"} case 30: - return "Developer" + return []string{"Developer"} case 40: - return "Maintainer" + return []string{"Maintainer"} case 50: - return "Owner" + return []string{"Owner"} default: - return "" + return []string{} } } diff --git a/pkg/accessreview/drivers/gitlab_test.go b/pkg/accessreview/drivers/gitlab_test.go index ebdc5fbcc..ee786a71d 100644 --- a/pkg/accessreview/drivers/gitlab_test.go +++ b/pkg/accessreview/drivers/gitlab_test.go @@ -43,7 +43,7 @@ func TestGitLabDriver(t *testing.T) { r := records[0] assert.NotEmpty(t, r.ExternalID) assert.NotEmpty(t, r.FullName) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) assert.Equal(t, coredata.MFAStatusUnknown, r.MFAStatus) require.NotNil(t, r.Active) assert.True(t, *r.Active) diff --git a/pkg/accessreview/drivers/google_workspace.go b/pkg/accessreview/drivers/google_workspace.go index 3f79b0251..8c88924fb 100644 --- a/pkg/accessreview/drivers/google_workspace.go +++ b/pkg/accessreview/drivers/google_workspace.go @@ -144,11 +144,11 @@ func (d *GoogleWorkspaceDriver) ListAccounts(ctx context.Context) ([]AccountReco switch { case u.IsAdmin: - rec.Role = "Super Admin" + rec.Roles = []string{"Super Admin"} case u.IsDelegatedAdmin: - rec.Role = "Delegated Admin" + rec.Roles = []string{"Delegated Admin"} default: - rec.Role = "User" + rec.Roles = []string{"User"} } records = append(records, rec) diff --git a/pkg/accessreview/drivers/google_workspace_test.go b/pkg/accessreview/drivers/google_workspace_test.go index 2e4c4aff2..6a7f9bc22 100644 --- a/pkg/accessreview/drivers/google_workspace_test.go +++ b/pkg/accessreview/drivers/google_workspace_test.go @@ -38,5 +38,5 @@ func TestGoogleWorkspaceDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/grafana.go b/pkg/accessreview/drivers/grafana.go index 713ea493d..56bf7f36e 100644 --- a/pkg/accessreview/drivers/grafana.go +++ b/pkg/accessreview/drivers/grafana.go @@ -75,11 +75,18 @@ func (d *GrafanaDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro continue } + role := strings.TrimSpace(u.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: email, FullName: strings.TrimSpace(u.Name), - Role: strings.TrimSpace(u.Role), - IsAdmin: strings.EqualFold(strings.TrimSpace(u.Role), "Admin"), + Roles: roles, + IsAdmin: strings.EqualFold(role, "Admin"), MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, AccountType: coredata.AccessReviewEntryAccountTypeUser, diff --git a/pkg/accessreview/drivers/grafana_test.go b/pkg/accessreview/drivers/grafana_test.go index 969edbefc..dbd5527c6 100644 --- a/pkg/accessreview/drivers/grafana_test.go +++ b/pkg/accessreview/drivers/grafana_test.go @@ -42,7 +42,7 @@ func TestGrafanaDriver(t *testing.T) { assert.Equal(t, "admin@example.com", records[0].Email) assert.Equal(t, "Admin User", records[0].FullName) - assert.Equal(t, "Admin", records[0].Role) + assert.Equal(t, []string{"Admin"}, records[0].Roles) assert.True(t, records[0].IsAdmin) assert.Equal(t, strconv.Itoa(1), records[0].ExternalID) require.NotNil(t, records[0].Active) @@ -51,7 +51,7 @@ func TestGrafanaDriver(t *testing.T) { assert.Equal(t, "viewer@example.com", records[1].Email) assert.Equal(t, "Viewer User", records[1].FullName) - assert.Equal(t, "Viewer", records[1].Role) + assert.Equal(t, []string{"Viewer"}, records[1].Roles) assert.False(t, records[1].IsAdmin) assert.Equal(t, strconv.Itoa(2), records[1].ExternalID) require.NotNil(t, records[1].Active) diff --git a/pkg/accessreview/drivers/heroku.go b/pkg/accessreview/drivers/heroku.go index 4c917985f..1d4481608 100644 --- a/pkg/accessreview/drivers/heroku.go +++ b/pkg/accessreview/drivers/heroku.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "time" "go.probo.inc/probo/pkg/coredata" @@ -153,10 +154,17 @@ func (d *HerokuDriver) listTeamMembers(ctx context.Context) ([]AccountRecord, er externalID = m.ID } + role := strings.TrimSpace(m.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: email, FullName: fullName, - Role: m.Role, + Roles: roles, IsAdmin: isAdmin, MFAStatus: mfaStatus, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, @@ -262,10 +270,17 @@ func (d *HerokuDriver) listPersonalAccounts(ctx context.Context) ([]AccountRecor // 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 { + role = strings.TrimSpace(role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + return AccountRecord{ Email: email, FullName: email, - Role: role, + Roles: roles, IsAdmin: isAdmin, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, diff --git a/pkg/accessreview/drivers/heroku_test.go b/pkg/accessreview/drivers/heroku_test.go index c3b77bed5..0c5116213 100644 --- a/pkg/accessreview/drivers/heroku_test.go +++ b/pkg/accessreview/drivers/heroku_test.go @@ -46,7 +46,7 @@ func TestHerokuDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.ExternalID) assert.NotEmpty(t, r.FullName) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) assert.Equal(t, coredata.MFAStatusEnabled, r.MFAStatus) assert.True(t, r.IsAdmin) require.NotNil(t, r.CreatedAt) @@ -106,7 +106,7 @@ func TestHerokuDriverPersonalAccount(t *testing.T) { 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.Equal(t, []string{"owner"}, byEmail["alice@example.com"].Roles) assert.False(t, byEmail["bob@example.com"].IsAdmin) } diff --git a/pkg/accessreview/drivers/hubspot.go b/pkg/accessreview/drivers/hubspot.go index a1a615f60..ba9075e8d 100644 --- a/pkg/accessreview/drivers/hubspot.go +++ b/pkg/accessreview/drivers/hubspot.go @@ -92,25 +92,12 @@ func (d *HubSpotDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro } for _, u := range resp.Results { - role := "User" - roleID := hubspotRoleID(u) - - if roleMap != nil && roleID != "" { - if name, ok := roleMap[roleID]; ok { - role = name - } else if u.SuperAdmin { - role = "Super Admin" - } - } else if u.SuperAdmin { - role = "Super Admin" - } - fullName := strings.TrimSpace(u.FirstName + " " + u.LastName) record := AccountRecord{ Email: u.Email, FullName: fullName, - Role: role, + Roles: hubspotRoles(u, roleMap), Active: hubspotUserActive(u), IsAdmin: u.SuperAdmin, ExternalID: u.ID, @@ -206,16 +193,65 @@ func (d *HubSpotDriver) fetchRoles(ctx context.Context) (map[string]string, erro return roleMap, nil } -func hubspotRoleID(user hubspotUser) string { - if user.RoleID != "" { - return user.RoleID +func hubspotRoles(user hubspotUser, roleMap map[string]string) []string { + roleIDs := hubspotRoleIDs(user) + + seen := make(map[string]struct{}, len(roleIDs)+1) + roles := make([]string, 0, len(roleIDs)+1) + + if roleMap != nil { + for _, id := range roleIDs { + name, ok := roleMap[id] + if !ok { + continue + } + + if _, dup := seen[name]; dup { + continue + } + + seen[name] = struct{}{} + roles = append(roles, name) + } } - if len(user.RoleIDs) > 0 { - return user.RoleIDs[0] + if user.SuperAdmin { + if _, dup := seen["Super Admin"]; !dup { + roles = append(roles, "Super Admin") + } } - return "" + if len(roles) > 0 { + return roles + } + + return []string{"User"} +} + +func hubspotRoleIDs(user hubspotUser) []string { + seen := make(map[string]struct{}, 1+len(user.RoleIDs)) + ids := make([]string, 0, 1+len(user.RoleIDs)) + + add := func(id string) { + if id == "" { + return + } + + if _, ok := seen[id]; ok { + return + } + + seen[id] = struct{}{} + ids = append(ids, id) + } + + add(user.RoleID) + + for _, id := range user.RoleIDs { + add(id) + } + + return ids } func hubspotUserActive(user hubspotUser) *bool { diff --git a/pkg/accessreview/drivers/hubspot_test.go b/pkg/accessreview/drivers/hubspot_test.go index c9e637858..159a0c9da 100644 --- a/pkg/accessreview/drivers/hubspot_test.go +++ b/pkg/accessreview/drivers/hubspot_test.go @@ -73,7 +73,7 @@ func TestHubSpotDriverArchivedUsers(t *testing.T) { require.NoError(t, err) require.Len(t, records, 2) - assert.Equal(t, "Sales Admin", records[0].Role) + assert.Equal(t, []string{"Sales Admin"}, records[0].Roles) require.NotNil(t, records[0].Active) assert.True(t, *records[0].Active) @@ -83,6 +83,65 @@ func TestHubSpotDriverArchivedUsers(t *testing.T) { assert.False(t, *records[1].Active) } +func TestHubSpotRoles(t *testing.T) { + t.Parallel() + + roleMap := map[string]string{ + "role-1": "Sales Admin", + "role-2": "Marketing Admin", + } + + tests := []struct { + name string + user hubspotUser + want []string + }{ + { + name: "multiple role IDs", + user: hubspotUser{RoleIDs: []string{"role-1", "role-2"}}, + want: []string{"Sales Admin", "Marketing Admin"}, + }, + { + name: "roleId and roleIds merged without duplicates", + user: hubspotUser{RoleID: "role-1", RoleIDs: []string{"role-1", "role-2"}}, + want: []string{"Sales Admin", "Marketing Admin"}, + }, + { + name: "unknown role falls back to user", + user: hubspotUser{RoleIDs: []string{"missing"}}, + want: []string{"User"}, + }, + { + name: "unknown role with super admin", + user: hubspotUser{RoleIDs: []string{"missing"}, SuperAdmin: true}, + want: []string{"Super Admin"}, + }, + { + name: "known role merged with super admin", + user: hubspotUser{RoleIDs: []string{"role-1"}, SuperAdmin: true}, + want: []string{"Sales Admin", "Super Admin"}, + }, + { + name: "multiple roles merged with super admin", + user: hubspotUser{RoleIDs: []string{"role-1", "role-2"}, SuperAdmin: true}, + want: []string{"Sales Admin", "Marketing Admin", "Super Admin"}, + }, + { + name: "no roles defaults to user", + user: hubspotUser{}, + want: []string{"User"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, hubspotRoles(tt.user, roleMap)) + }) + } +} + type roundTripFunc func(req *http.Request) (*http.Response, error) func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { diff --git a/pkg/accessreview/drivers/intercom.go b/pkg/accessreview/drivers/intercom.go index f814a253a..c45ec48b2 100644 --- a/pkg/accessreview/drivers/intercom.go +++ b/pkg/accessreview/drivers/intercom.go @@ -66,7 +66,7 @@ func (d *IntercomDriver) ListAccounts(ctx context.Context) ([]AccountRecord, err record := AccountRecord{ Email: a.Email, FullName: a.Name, - Role: intercomRole(a.HasInboxSeat), + Roles: intercomRoles(a.HasInboxSeat), JobTitle: a.JobTitle, IsAdmin: false, // Intercom API does not expose admin role information ExternalID: a.ID, @@ -117,10 +117,10 @@ func (d *IntercomDriver) fetchAdmins(ctx context.Context) (*intercomAdminsRespon // seat. The Intercom API does not expose a proper role field, so this is the // best approximation available: users with inbox seats are active agents, // those without are limited/viewer users. -func intercomRole(hasInboxSeat bool) string { +func intercomRoles(hasInboxSeat bool) []string { if hasInboxSeat { - return "Agent" + return []string{"Agent"} } - return "Viewer" + return []string{"Viewer"} } diff --git a/pkg/accessreview/drivers/linear.go b/pkg/accessreview/drivers/linear.go index d582cb757..091b63559 100644 --- a/pkg/accessreview/drivers/linear.go +++ b/pkg/accessreview/drivers/linear.go @@ -96,7 +96,7 @@ func (d *LinearDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error record := AccountRecord{ Email: u.Email, FullName: u.Name, - Role: linearRole(u.Admin, u.Guest), + Roles: linearRoles(u.Admin, u.Guest), Active: new(u.Active), IsAdmin: u.Admin, ExternalID: u.ID, @@ -200,13 +200,13 @@ query AccessReviewLinearUsers($after: String) { return &resp, nil } -func linearRole(admin, guest bool) string { +func linearRoles(admin, guest bool) []string { switch { case admin: - return "Admin" + return []string{"Admin"} case guest: - return "Guest" + return []string{"Guest"} default: - return "Member" + return []string{"Member"} } } diff --git a/pkg/accessreview/drivers/linear_test.go b/pkg/accessreview/drivers/linear_test.go index b39ba0964..cdcc57a9c 100644 --- a/pkg/accessreview/drivers/linear_test.go +++ b/pkg/accessreview/drivers/linear_test.go @@ -38,5 +38,5 @@ func TestLinearDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/metabase.go b/pkg/accessreview/drivers/metabase.go index aa2833bb8..b995e457f 100644 --- a/pkg/accessreview/drivers/metabase.go +++ b/pkg/accessreview/drivers/metabase.go @@ -78,7 +78,7 @@ func (d *MetabaseDriver) ListAccounts(ctx context.Context) ([]AccountRecord, err record := AccountRecord{ Email: u.Email, FullName: metabaseFullName(u), - Role: metabaseRole(u.IsSuperuser), + Roles: metabaseRoles(u.IsSuperuser), Active: new(u.IsActive), IsAdmin: u.IsSuperuser, ExternalID: strconv.Itoa(u.ID), @@ -176,12 +176,12 @@ func metabaseFullName(u metabaseUser) string { return strings.TrimSpace(strings.Join([]string{u.FirstName, u.LastName}, " ")) } -func metabaseRole(isSuperuser bool) string { +func metabaseRoles(isSuperuser bool) []string { if isSuperuser { - return "Admin" + return []string{"Admin"} } - return "User" + return []string{"User"} } // metabaseNameResolver resolves the Metabase site name by querying diff --git a/pkg/accessreview/drivers/metabase_test.go b/pkg/accessreview/drivers/metabase_test.go index d84465a1c..e2c4f0d2d 100644 --- a/pkg/accessreview/drivers/metabase_test.go +++ b/pkg/accessreview/drivers/metabase_test.go @@ -41,7 +41,7 @@ func TestMetabaseDriver(t *testing.T) { assert.Equal(t, "alice@example.com", records[0].Email) assert.Equal(t, "Alice A.", records[0].FullName) - assert.Equal(t, "Admin", records[0].Role) + assert.Equal(t, []string{"Admin"}, records[0].Roles) assert.True(t, records[0].IsAdmin) require.NotNil(t, records[0].Active) assert.True(t, *records[0].Active) @@ -51,7 +51,7 @@ func TestMetabaseDriver(t *testing.T) { assert.Equal(t, "bob@example.com", records[1].Email) assert.Equal(t, "Bob Builder", records[1].FullName) - assert.Equal(t, "User", records[1].Role) + assert.Equal(t, []string{"User"}, records[1].Roles) assert.False(t, records[1].IsAdmin) require.NotNil(t, records[1].Active) assert.False(t, *records[1].Active) diff --git a/pkg/accessreview/drivers/microsoft_365.go b/pkg/accessreview/drivers/microsoft_365.go index 452cabbf8..3ef8c3075 100644 --- a/pkg/accessreview/drivers/microsoft_365.go +++ b/pkg/accessreview/drivers/microsoft_365.go @@ -164,16 +164,16 @@ func (d *Microsoft365Driver) ListAccounts(ctx context.Context) ([]AccountRecord, } } - role := pickHighestRole(userRoles) - if role == "" { - role = "User" + roles := userRoles + if len(roles) == 0 { + roles = []string{"User"} } active := u.AccountEnabled rec := AccountRecord{ Email: email, FullName: u.DisplayName, - Role: role, + Roles: roles, JobTitle: u.JobTitle, Active: &active, IsAdmin: isAdmin, @@ -195,39 +195,6 @@ func (d *Microsoft365Driver) ListAccounts(ctx context.Context) ([]AccountRecord, return records, nil } -// pickHighestRole returns the most privileged admin role from the list, -// falling back to the first non-admin role when no admin role is present. -// Privilege order is hard-coded to Microsoft's well-known directory roles. -func pickHighestRole(roles []string) string { - priority := []string{ - "Global Administrator", - "Company Administrator", - "Privileged Role Administrator", - "Privileged Authentication Administrator", - "Security Administrator", - "Application Administrator", - "Cloud Application Administrator", - "User Administrator", - "Conditional Access Administrator", - "Compliance Administrator", - "Authentication Administrator", - } - - for _, p := range priority { - for _, r := range roles { - if r == p { - return r - } - } - } - - if len(roles) > 0 { - return roles[0] - } - - return "" -} - func (d *Microsoft365Driver) listUsers(ctx context.Context) ([]microsoft365User, error) { pageURL, err := buildMicrosoft365UsersURL() if err != nil { diff --git a/pkg/accessreview/drivers/neon.go b/pkg/accessreview/drivers/neon.go index aba484ed1..72c793d3c 100644 --- a/pkg/accessreview/drivers/neon.go +++ b/pkg/accessreview/drivers/neon.go @@ -90,7 +90,7 @@ func (d *NeonDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error) // The members endpoint exposes no display name; // fall back to the email. FullName: m.User.Email, - Role: neonRole(m.Member.Role), + Roles: neonRoles(m.Member.Role), // deactivated_at is absent for active accounts. Active: new(m.User.DeactivatedAt == ""), IsAdmin: neonIsAdmin(m.Member.Role), @@ -160,22 +160,26 @@ func (d *NeonDriver) queryMembers(ctx context.Context, cursor string) (*neonMemb return &resp, nil } -// neonRole maps Neon's lowercase member roles to their display form, +// neonRoles maps Neon's lowercase member roles to their display form, // passing unknown values through unchanged. -func neonRole(role string) string { +func neonRoles(role string) []string { + if role == "" { + return []string{} + } + switch strings.ToLower(role) { case "admin": - return "Admin" + return []string{"Admin"} case "member": - return "Member" + return []string{"Member"} case "editor": - return "Editor" + return []string{"Editor"} case "viewer": - return "Viewer" + return []string{"Viewer"} case "collaborator": - return "Collaborator" + return []string{"Collaborator"} default: - return role + return []string{role} } } diff --git a/pkg/accessreview/drivers/neon_test.go b/pkg/accessreview/drivers/neon_test.go index d99e004be..7efcec063 100644 --- a/pkg/accessreview/drivers/neon_test.go +++ b/pkg/accessreview/drivers/neon_test.go @@ -51,7 +51,7 @@ func TestNeonDriverListAccounts(t *testing.T) { // ExternalID is the stable account UUID (member.user_id). assert.Equal(t, "jane.doe@example.com", records[0].Email) assert.Equal(t, "jane.doe@example.com", records[0].FullName) - assert.Equal(t, "Admin", records[0].Role) + assert.Equal(t, []string{"Admin"}, records[0].Roles) assert.True(t, records[0].IsAdmin) assert.Equal(t, coredata.MFAStatusEnabled, records[0].MFAStatus) assert.Equal(t, "bbbbbbbb-1111-2222-3333-000000000001", records[0].ExternalID) @@ -62,7 +62,7 @@ func TestNeonDriverListAccounts(t *testing.T) { // Deactivated member with MFA disabled. assert.Equal(t, "john.smith@example.com", records[1].Email) - assert.Equal(t, "Member", records[1].Role) + assert.Equal(t, []string{"Member"}, records[1].Roles) assert.False(t, records[1].IsAdmin) assert.Equal(t, coredata.MFAStatusDisabled, records[1].MFAStatus) assert.Equal(t, "bbbbbbbb-1111-2222-3333-000000000002", records[1].ExternalID) @@ -72,7 +72,7 @@ func TestNeonDriverListAccounts(t *testing.T) { // Second page: editor with has_mfa omitted (Unknown) and an empty // user_id falling back to the membership ID. assert.Equal(t, "erin.lee@example.com", records[2].Email) - assert.Equal(t, "Editor", records[2].Role) + assert.Equal(t, []string{"Editor"}, records[2].Roles) assert.False(t, records[2].IsAdmin) assert.Equal(t, coredata.MFAStatusUnknown, records[2].MFAStatus) assert.Equal(t, "aaaaaaaa-1111-2222-3333-000000000003", records[2].ExternalID) @@ -101,27 +101,28 @@ func TestNeonDriverListAccountsError(t *testing.T) { assert.Contains(t, err.Error(), "unexpected status 401") } -func TestNeonRole(t *testing.T) { +func TestNeonRoles(t *testing.T) { t.Parallel() cases := []struct { in string - want string + want []string isAdmin bool }{ - {in: "admin", want: "Admin", isAdmin: true}, - {in: "member", want: "Member", isAdmin: false}, - {in: "editor", want: "Editor", isAdmin: false}, - {in: "viewer", want: "Viewer", isAdmin: false}, - {in: "collaborator", want: "Collaborator", isAdmin: false}, - {in: "future_role", want: "future_role", isAdmin: false}, + {in: "admin", want: []string{"Admin"}, isAdmin: true}, + {in: "member", want: []string{"Member"}, isAdmin: false}, + {in: "editor", want: []string{"Editor"}, isAdmin: false}, + {in: "viewer", want: []string{"Viewer"}, isAdmin: false}, + {in: "collaborator", want: []string{"Collaborator"}, isAdmin: false}, + {in: "future_role", want: []string{"future_role"}, isAdmin: false}, + {in: "", want: []string{}, isAdmin: false}, } for _, c := range cases { t.Run(c.in, func(t *testing.T) { t.Parallel() - assert.Equal(t, c.want, neonRole(c.in)) + assert.Equal(t, c.want, neonRoles(c.in)) assert.Equal(t, c.isAdmin, neonIsAdmin(c.in)) }) } diff --git a/pkg/accessreview/drivers/netlify.go b/pkg/accessreview/drivers/netlify.go index 9987ef143..483273c7a 100644 --- a/pkg/accessreview/drivers/netlify.go +++ b/pkg/accessreview/drivers/netlify.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "go.probo.inc/probo/pkg/coredata" "go.probo.inc/probo/pkg/rfc5988" @@ -78,10 +79,17 @@ func (d *NetlifyDriver) ListAccounts(ctx context.Context) ([]AccountRecord, erro } for _, m := range members { + role := strings.TrimSpace(m.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: m.Email, FullName: m.FullName, - Role: m.Role, + Roles: roles, ExternalID: m.ID, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, diff --git a/pkg/accessreview/drivers/netlify_test.go b/pkg/accessreview/drivers/netlify_test.go index a4365e6b1..fb73bdc9a 100644 --- a/pkg/accessreview/drivers/netlify_test.go +++ b/pkg/accessreview/drivers/netlify_test.go @@ -43,5 +43,5 @@ func TestNetlifyDriver(t *testing.T) { assert.NotEmpty(t, r.ExternalID) assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/notion.go b/pkg/accessreview/drivers/notion.go index aea31b5b4..403894319 100644 --- a/pkg/accessreview/drivers/notion.go +++ b/pkg/accessreview/drivers/notion.go @@ -80,7 +80,7 @@ func (d *NotionDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error record := AccountRecord{ Email: email, FullName: u.Name, - Role: "Member", + Roles: []string{"Member"}, IsAdmin: false, ExternalID: u.ID, MFAStatus: coredata.MFAStatusUnknown, diff --git a/pkg/accessreview/drivers/openai.go b/pkg/accessreview/drivers/openai.go index bcb83bc69..fcb4f0bb3 100644 --- a/pkg/accessreview/drivers/openai.go +++ b/pkg/accessreview/drivers/openai.go @@ -67,7 +67,7 @@ func (d *OpenAIDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error record := AccountRecord{ Email: u.Email, FullName: u.Name, - Role: openaiRole(u.Role), + Roles: openaiRoles(u.Role), Active: new(!u.Disabled), IsAdmin: u.Role == "owner", ExternalID: u.ID, @@ -134,13 +134,17 @@ func (d *OpenAIDriver) fetchUsers(ctx context.Context, after string) (*openaiUse return &resp, nil } -func openaiRole(role string) string { +func openaiRoles(role string) []string { + if role == "" { + return []string{} + } + switch role { case "owner": - return "Owner" + return []string{"Owner"} case "reader": - return "Reader" + return []string{"Reader"} default: - return "Member" + return []string{"Member"} } } diff --git a/pkg/accessreview/drivers/openai_test.go b/pkg/accessreview/drivers/openai_test.go index 656b46e17..e56e7a3f2 100644 --- a/pkg/accessreview/drivers/openai_test.go +++ b/pkg/accessreview/drivers/openai_test.go @@ -38,5 +38,5 @@ func TestOpenAIDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/pagerduty.go b/pkg/accessreview/drivers/pagerduty.go index fcbb2a4d7..5353d0f84 100644 --- a/pkg/accessreview/drivers/pagerduty.go +++ b/pkg/accessreview/drivers/pagerduty.go @@ -21,6 +21,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "time" "go.probo.inc/probo/pkg/coredata" @@ -77,10 +78,17 @@ func (d *PagerDutyDriver) ListAccounts(ctx context.Context) ([]AccountRecord, er for _, u := range page.Users { isAdmin := u.Role == "admin" || u.Role == "owner" + role := strings.TrimSpace(u.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: u.Email, FullName: u.Name, - Role: u.Role, + Roles: roles, IsAdmin: isAdmin, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, diff --git a/pkg/accessreview/drivers/pagerduty_test.go b/pkg/accessreview/drivers/pagerduty_test.go index ff3675bd4..6b017d1a9 100644 --- a/pkg/accessreview/drivers/pagerduty_test.go +++ b/pkg/accessreview/drivers/pagerduty_test.go @@ -38,6 +38,6 @@ func TestPagerDutyDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.ExternalID) assert.NotEmpty(t, r.FullName) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) assert.True(t, r.IsAdmin) } diff --git a/pkg/accessreview/drivers/posthog.go b/pkg/accessreview/drivers/posthog.go index 8a0d5eda4..7e2d57b76 100644 --- a/pkg/accessreview/drivers/posthog.go +++ b/pkg/accessreview/drivers/posthog.go @@ -292,7 +292,7 @@ func posthogAccountRecord(member posthogMember) AccountRecord { record := AccountRecord{ Email: member.User.Email, FullName: posthogFullName(member.User), - Role: posthogRole(member.Level, member.User.RoleAtOrganization), + Roles: posthogRoles(member.Level, member.User.RoleAtOrganization), IsAdmin: posthogIsAdmin(member.Level), ExternalID: member.User.UUID, MFAStatus: posthogMFAStatus(member.Is2FAEnabled), @@ -319,18 +319,18 @@ func posthogFullName(user posthogMemberUser) string { return strings.TrimSpace(strings.Join([]string{user.FirstName, user.LastName}, " ")) } -func posthogRole(level int, fallback string) string { +func posthogRoles(level int, fallback string) []string { switch { case level >= posthogMembershipLevelOwner: - return "Owner" + return []string{"Owner"} case level >= posthogMembershipLevelAdmin: - return "Admin" + return []string{"Admin"} case level >= posthogMembershipLevelMember: - return "Member" + return []string{"Member"} case fallback != "": - return fallback + return []string{fallback} default: - return "Member" + return []string{"Member"} } } diff --git a/pkg/accessreview/drivers/posthog_test.go b/pkg/accessreview/drivers/posthog_test.go index c902671b0..f497996e7 100644 --- a/pkg/accessreview/drivers/posthog_test.go +++ b/pkg/accessreview/drivers/posthog_test.go @@ -41,7 +41,7 @@ func TestPostHogDriverListAccounts(t *testing.T) { owner := records[0] assert.Equal(t, "owner@example.com", owner.Email) assert.Equal(t, "Olivia Owner", owner.FullName) - assert.Equal(t, "Owner", owner.Role) + assert.Equal(t, []string{"Owner"}, owner.Roles) assert.True(t, owner.IsAdmin) assert.Equal(t, coredata.MFAStatusEnabled, owner.MFAStatus) assert.Equal(t, "user-1", owner.ExternalID) @@ -50,7 +50,7 @@ func TestPostHogDriverListAccounts(t *testing.T) { member := records[1] assert.Equal(t, "member@example.com", member.Email) - assert.Equal(t, "Member", member.Role) + assert.Equal(t, []string{"Member"}, member.Roles) assert.False(t, member.IsAdmin) assert.Equal(t, coredata.MFAStatusDisabled, member.MFAStatus) require.NotNil(t, member.CreatedAt) @@ -58,7 +58,7 @@ func TestPostHogDriverListAccounts(t *testing.T) { admin := records[2] assert.Equal(t, "admin@example.com", admin.Email) - assert.Equal(t, "Admin", admin.Role) + assert.Equal(t, []string{"Admin"}, admin.Roles) assert.True(t, admin.IsAdmin) assert.Equal(t, coredata.MFAStatusUnknown, admin.MFAStatus) assert.Equal(t, "membership-3", admin.ExternalID) @@ -177,12 +177,12 @@ func TestPostHogNameResolver(t *testing.T) { } } -func TestPostHogRoleFallback(t *testing.T) { +func TestPostHogRolesFallback(t *testing.T) { t.Parallel() - assert.Equal(t, "Owner", posthogRole(15, "")) - assert.Equal(t, "Admin", posthogRole(8, "")) - assert.Equal(t, "Member", posthogRole(1, "")) - assert.Equal(t, "engineering", posthogRole(0, "engineering")) - assert.Equal(t, "Member", posthogRole(0, "")) + assert.Equal(t, []string{"Owner"}, posthogRoles(15, "")) + assert.Equal(t, []string{"Admin"}, posthogRoles(8, "")) + assert.Equal(t, []string{"Member"}, posthogRoles(1, "")) + assert.Equal(t, []string{"engineering"}, posthogRoles(0, "engineering")) + assert.Equal(t, []string{"Member"}, posthogRoles(0, "")) } diff --git a/pkg/accessreview/drivers/probo_memberships.go b/pkg/accessreview/drivers/probo_memberships.go index e80f19cde..75d5de719 100644 --- a/pkg/accessreview/drivers/probo_memberships.go +++ b/pkg/accessreview/drivers/probo_memberships.go @@ -17,6 +17,7 @@ package drivers import ( "context" "fmt" + "strings" "go.gearno.de/kit/pg" "go.probo.inc/probo/pkg/coredata" @@ -61,7 +62,13 @@ func (d *ProboMembershipsDriver) ListAccounts(ctx context.Context) ([]AccountRec } for _, account := range accounts { - role := account.Role + role := strings.TrimSpace(account.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + isAdmin := role == string(coredata.MembershipRoleOwner) || role == string(coredata.MembershipRoleAdmin) createdAt := account.CreatedAt @@ -70,7 +77,7 @@ func (d *ProboMembershipsDriver) ListAccounts(ctx context.Context) ([]AccountRec AccountRecord{ Email: account.Email, FullName: account.FullName, - Role: role, + Roles: roles, Active: new(account.State == string(coredata.ProfileStateActive)), IsAdmin: isAdmin, ExternalID: account.ID.String(), diff --git a/pkg/accessreview/drivers/qovery.go b/pkg/accessreview/drivers/qovery.go index 6b1123918..ff49a4780 100644 --- a/pkg/accessreview/drivers/qovery.go +++ b/pkg/accessreview/drivers/qovery.go @@ -101,7 +101,7 @@ func (d *QoveryDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error record := AccountRecord{ Email: member.Email, FullName: qoveryFullName(member), - Role: qoveryRole(member.Role), + Roles: qoveryRoles(member.Role), IsAdmin: qoveryIsAdmin(member.Role), MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, @@ -139,18 +139,22 @@ func qoveryFullName(member qoveryMember) string { return member.Email } -func qoveryRole(role string) string { +func qoveryRoles(role string) []string { + if role == "" { + return []string{} + } + switch strings.ToUpper(role) { case "OWNER": - return "Owner" + return []string{"Owner"} case "ADMIN": - return "Admin" + return []string{"Admin"} case "DEVELOPER": - return "Developer" + return []string{"Developer"} case "VIEWER": - return "Viewer" + return []string{"Viewer"} default: - return role + return []string{role} } } diff --git a/pkg/accessreview/drivers/qovery_test.go b/pkg/accessreview/drivers/qovery_test.go index c9abfa006..e4db03b08 100644 --- a/pkg/accessreview/drivers/qovery_test.go +++ b/pkg/accessreview/drivers/qovery_test.go @@ -54,7 +54,7 @@ func TestQoveryDriverListAccounts(t *testing.T) { // Qovery member IDs are the IdP subject (e.g. "google-oauth2|"). assert.Equal(t, "jane.doe@example.com", records[0].Email) assert.Equal(t, "Jane Doe", records[0].FullName) - assert.Equal(t, "Owner", records[0].Role) + assert.Equal(t, []string{"Owner"}, records[0].Roles) assert.True(t, records[0].IsAdmin) assert.Equal(t, "google-oauth2|100000000000000000001", records[0].ExternalID) require.NotNil(t, records[0].LastLogin) @@ -65,7 +65,7 @@ func TestQoveryDriverListAccounts(t *testing.T) { // leaves LastLogin nil. assert.Equal(t, "john.smith@example.com", records[1].Email) assert.Equal(t, "john", records[1].FullName) - assert.Equal(t, "Developer", records[1].Role) + assert.Equal(t, []string{"Developer"}, records[1].Roles) assert.False(t, records[1].IsAdmin) assert.Equal(t, "google-oauth2|100000000000000000002", records[1].ExternalID) assert.Nil(t, records[1].LastLogin) @@ -93,26 +93,27 @@ func TestQoveryDriverListAccountsError(t *testing.T) { assert.Contains(t, err.Error(), "unexpected status 401") } -func TestQoveryRole(t *testing.T) { +func TestQoveryRoles(t *testing.T) { t.Parallel() cases := []struct { in string - want string + want []string isAdmin bool }{ - {in: "OWNER", want: "Owner", isAdmin: true}, - {in: "ADMIN", want: "Admin", isAdmin: true}, - {in: "DEVELOPER", want: "Developer", isAdmin: false}, - {in: "VIEWER", want: "Viewer", isAdmin: false}, - {in: "future_role", want: "future_role", isAdmin: false}, + {in: "OWNER", want: []string{"Owner"}, isAdmin: true}, + {in: "ADMIN", want: []string{"Admin"}, isAdmin: true}, + {in: "DEVELOPER", want: []string{"Developer"}, isAdmin: false}, + {in: "VIEWER", want: []string{"Viewer"}, isAdmin: false}, + {in: "future_role", want: []string{"future_role"}, isAdmin: false}, + {in: "", want: []string{}, isAdmin: false}, } for _, c := range cases { t.Run(c.in, func(t *testing.T) { t.Parallel() - assert.Equal(t, c.want, qoveryRole(c.in)) + assert.Equal(t, c.want, qoveryRoles(c.in)) assert.Equal(t, c.isAdmin, qoveryIsAdmin(c.in)) }) } diff --git a/pkg/accessreview/drivers/render.go b/pkg/accessreview/drivers/render.go index fc879b75a..db86f026a 100644 --- a/pkg/accessreview/drivers/render.go +++ b/pkg/accessreview/drivers/render.go @@ -99,7 +99,7 @@ func (d *RenderDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error records = append(records, AccountRecord{ Email: member.Email, FullName: renderFullName(member), - Role: renderRole(member.Role), + Roles: renderRoles(member.Role), Active: renderActive(member.Status), IsAdmin: renderIsAdmin(member.Role), MFAStatus: renderMFAStatus(member.MFAEnabled), @@ -120,24 +120,28 @@ func renderFullName(member renderMember) string { return member.Email } -// renderRole maps Render's uppercase role enum to a human-readable label. +// renderRoles maps Render's uppercase role enum to a human-readable label. // Render documents ADMIN, DEVELOPER, WORKSPACE_CONTRIBUTOR, // WORKSPACE_BILLING, and WORKSPACE_VIEWER; unknown future roles fall through // to the raw value. -func renderRole(role string) string { +func renderRoles(role string) []string { + if role == "" { + return []string{} + } + switch strings.ToUpper(role) { case "ADMIN": - return "Admin" + return []string{"Admin"} case "DEVELOPER": - return "Developer" + return []string{"Developer"} case "WORKSPACE_CONTRIBUTOR": - return "Contributor" + return []string{"Contributor"} case "WORKSPACE_BILLING": - return "Billing" + return []string{"Billing"} case "WORKSPACE_VIEWER": - return "Viewer" + return []string{"Viewer"} default: - return role + return []string{role} } } diff --git a/pkg/accessreview/drivers/render_test.go b/pkg/accessreview/drivers/render_test.go index e257dd6ac..4c16be715 100644 --- a/pkg/accessreview/drivers/render_test.go +++ b/pkg/accessreview/drivers/render_test.go @@ -51,7 +51,7 @@ func TestRenderDriverListAccounts(t *testing.T) { // never the email. assert.Equal(t, "jane.doe@example.com", records[0].Email) assert.Equal(t, "Jane Doe", records[0].FullName) - assert.Equal(t, "Admin", records[0].Role) + assert.Equal(t, []string{"Admin"}, records[0].Roles) assert.True(t, records[0].IsAdmin) assert.Equal(t, coredata.MFAStatusEnabled, records[0].MFAStatus) assert.Equal(t, coredata.AccessReviewEntryAccountTypeUser, records[0].AccountType) @@ -63,7 +63,7 @@ func TestRenderDriverListAccounts(t *testing.T) { // Developer: active, MFA disabled, not an admin. assert.Equal(t, "john.smith@example.com", records[1].Email) assert.Equal(t, "John Smith", records[1].FullName) - assert.Equal(t, "Developer", records[1].Role) + assert.Equal(t, []string{"Developer"}, records[1].Roles) assert.False(t, records[1].IsAdmin) assert.Equal(t, coredata.MFAStatusDisabled, records[1].MFAStatus) assert.Equal(t, "usr-000000000000000000b2", records[1].ExternalID) @@ -74,7 +74,7 @@ func TestRenderDriverListAccounts(t *testing.T) { // email so the row is never nameless. assert.Equal(t, "sam.viewer@example.com", records[2].Email) assert.Equal(t, "sam.viewer@example.com", records[2].FullName) - assert.Equal(t, "Viewer", records[2].Role) + assert.Equal(t, []string{"Viewer"}, records[2].Roles) assert.False(t, records[2].IsAdmin) assert.Equal(t, coredata.MFAStatusDisabled, records[2].MFAStatus) assert.Equal(t, "usr-000000000000000000c3", records[2].ExternalID) @@ -105,27 +105,28 @@ func TestRenderDriverListAccountsError(t *testing.T) { assert.NotContains(t, err.Error(), "unauthorized") } -func TestRenderRole(t *testing.T) { +func TestRenderRoles(t *testing.T) { t.Parallel() cases := []struct { in string - want string + want []string isAdmin bool }{ - {in: "ADMIN", want: "Admin", isAdmin: true}, - {in: "DEVELOPER", want: "Developer", isAdmin: false}, - {in: "WORKSPACE_CONTRIBUTOR", want: "Contributor", isAdmin: false}, - {in: "WORKSPACE_BILLING", want: "Billing", isAdmin: false}, - {in: "WORKSPACE_VIEWER", want: "Viewer", isAdmin: false}, - {in: "future_role", want: "future_role", isAdmin: false}, + {in: "ADMIN", want: []string{"Admin"}, isAdmin: true}, + {in: "DEVELOPER", want: []string{"Developer"}, isAdmin: false}, + {in: "WORKSPACE_CONTRIBUTOR", want: []string{"Contributor"}, isAdmin: false}, + {in: "WORKSPACE_BILLING", want: []string{"Billing"}, isAdmin: false}, + {in: "WORKSPACE_VIEWER", want: []string{"Viewer"}, isAdmin: false}, + {in: "future_role", want: []string{"future_role"}, isAdmin: false}, + {in: "", want: []string{}, isAdmin: false}, } for _, c := range cases { t.Run(c.in, func(t *testing.T) { t.Parallel() - assert.Equal(t, c.want, renderRole(c.in)) + assert.Equal(t, c.want, renderRoles(c.in)) assert.Equal(t, c.isAdmin, renderIsAdmin(c.in)) }) } diff --git a/pkg/accessreview/drivers/sendgrid.go b/pkg/accessreview/drivers/sendgrid.go index 62efdb3e1..b545f2647 100644 --- a/pkg/accessreview/drivers/sendgrid.go +++ b/pkg/accessreview/drivers/sendgrid.go @@ -103,7 +103,7 @@ func (d *SendGridDriver) ListAccounts(ctx context.Context) ([]AccountRecord, err records = append(records, AccountRecord{ Email: teammate.Email, FullName: sendGridFullName(teammate.FirstName, teammate.LastName), - Role: sendGridRole(teammate.UserType, teammate.IsAdmin), + Roles: sendGridRoles(teammate.UserType, teammate.IsAdmin), IsAdmin: teammate.IsAdmin, // SendGrid exposes no UUID for teammates; the username is the // only stable handle. For unified accounts it equals the email. @@ -209,22 +209,22 @@ func sendGridFullName(firstName, lastName string) string { return strings.TrimSpace(strings.Join([]string{firstName, lastName}, " ")) } -func sendGridRole(userType string, isAdmin bool) string { +func sendGridRoles(userType string, isAdmin bool) []string { switch userType { case "owner": - return "Owner" + return []string{"Owner"} case "admin": - return "Admin" + return []string{"Admin"} case "teammate": - return "Teammate" + return []string{"Teammate"} case "": if isAdmin { - return "Admin" + return []string{"Admin"} } - return "Teammate" + return []string{"Teammate"} default: - return userType + return []string{userType} } } diff --git a/pkg/accessreview/drivers/sendgrid_test.go b/pkg/accessreview/drivers/sendgrid_test.go index 7b657525f..784cd26c1 100644 --- a/pkg/accessreview/drivers/sendgrid_test.go +++ b/pkg/accessreview/drivers/sendgrid_test.go @@ -43,7 +43,7 @@ func TestSendGridDriver(t *testing.T) { owner := records[0] assert.Equal(t, "owner@example.com", owner.Email) assert.Empty(t, owner.FullName) - assert.Equal(t, "Owner", owner.Role) + assert.Equal(t, []string{"Owner"}, owner.Roles) assert.True(t, owner.IsAdmin) assert.Equal(t, "owner@example.com", owner.ExternalID) assert.Equal(t, coredata.AccessReviewEntryAccountTypeUser, owner.AccountType) @@ -62,7 +62,7 @@ func TestSendGridDriver(t *testing.T) { teammate := records[1] assert.Equal(t, "taylor@example.com", teammate.Email) assert.Equal(t, "Taylor Teammate", teammate.FullName) - assert.Equal(t, "Teammate", teammate.Role) + assert.Equal(t, []string{"Teammate"}, teammate.Roles) assert.False(t, teammate.IsAdmin) // Non-unified teammate: username is a handle distinct from the email. assert.Equal(t, "taylor-teammate", teammate.ExternalID) @@ -70,27 +70,27 @@ func TestSendGridDriver(t *testing.T) { assert.Equal(t, coredata.MFAStatusEnabled, teammate.MFAStatus) } -func TestSendGridRole(t *testing.T) { +func TestSendGridRoles(t *testing.T) { t.Parallel() tests := []struct { name string userType string isAdmin bool - want string + want []string }{ - {name: "owner", userType: "owner", isAdmin: true, want: "Owner"}, - {name: "admin", userType: "admin", isAdmin: true, want: "Admin"}, - {name: "teammate", userType: "teammate", isAdmin: false, want: "Teammate"}, - {name: "empty admin", userType: "", isAdmin: true, want: "Admin"}, - {name: "empty teammate", userType: "", isAdmin: false, want: "Teammate"}, - {name: "unknown", userType: "custom-role", isAdmin: false, want: "custom-role"}, + {name: "owner", userType: "owner", isAdmin: true, want: []string{"Owner"}}, + {name: "admin", userType: "admin", isAdmin: true, want: []string{"Admin"}}, + {name: "teammate", userType: "teammate", isAdmin: false, want: []string{"Teammate"}}, + {name: "empty admin", userType: "", isAdmin: true, want: []string{"Admin"}}, + {name: "empty teammate", userType: "", isAdmin: false, want: []string{"Teammate"}}, + {name: "unknown", userType: "custom-role", isAdmin: false, want: []string{"custom-role"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tt.want, sendGridRole(tt.userType, tt.isAdmin)) + assert.Equal(t, tt.want, sendGridRoles(tt.userType, tt.isAdmin)) }) } } diff --git a/pkg/accessreview/drivers/sentry.go b/pkg/accessreview/drivers/sentry.go index a0a654a66..26321d64d 100644 --- a/pkg/accessreview/drivers/sentry.go +++ b/pkg/accessreview/drivers/sentry.go @@ -21,6 +21,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "time" "go.probo.inc/probo/pkg/coredata" @@ -120,7 +121,8 @@ func (d *SentryDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error active = active && m.User.IsActive } - isAdmin := m.OrgRole == "admin" || m.OrgRole == "owner" + role := strings.TrimSpace(m.OrgRole) + isAdmin := role == "admin" || role == "owner" mfaStatus := coredata.MFAStatusUnknown @@ -134,10 +136,15 @@ func (d *SentryDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error authMethod := sentryAuthMethod(m.Flags, m.User) + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: m.Email, FullName: fullName, - Role: m.OrgRole, + Roles: roles, Active: new(active), IsAdmin: isAdmin, ExternalID: m.ID, diff --git a/pkg/accessreview/drivers/sentry_test.go b/pkg/accessreview/drivers/sentry_test.go index 576492b43..26e1c6179 100644 --- a/pkg/accessreview/drivers/sentry_test.go +++ b/pkg/accessreview/drivers/sentry_test.go @@ -45,7 +45,7 @@ func TestSentryDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } func TestSentryDriverListAccountsStaleSlug(t *testing.T) { diff --git a/pkg/accessreview/drivers/signoz.go b/pkg/accessreview/drivers/signoz.go index 04e3f0876..592a3ebfb 100644 --- a/pkg/accessreview/drivers/signoz.go +++ b/pkg/accessreview/drivers/signoz.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/url" + "slices" "strings" "time" @@ -77,14 +78,14 @@ func (d *SigNozDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error continue } - role := sigNozRole(u.Role) + roles := sigNozRoles(u.Role) record := AccountRecord{ Email: email, FullName: strings.TrimSpace(u.DisplayName), - Role: role, + Roles: roles, Active: sigNozActiveStatus(u.Status), - IsAdmin: u.IsRoot || strings.EqualFold(role, "Admin"), + IsAdmin: u.IsRoot || slices.Contains(roles, "Admin"), MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, AccountType: coredata.AccessReviewEntryAccountTypeUser, @@ -146,26 +147,26 @@ func (d *SigNozDriver) queryUsers(ctx context.Context) ([]sigNozUser, error) { return users, nil } -// sigNozRole normalizes a SigNoz role string (ADMIN / EDITOR / VIEWER, or the +// sigNozRoles normalizes a SigNoz role string (ADMIN / EDITOR / VIEWER, or the // managed-role display names signoz-admin / signoz-editor / signoz-viewer) // into a stable label, preserving unknown custom roles verbatim. Matching is // exact (not substring) so a custom role merely containing "admin" is not // silently promoted to Admin. -func sigNozRole(raw string) string { +func sigNozRoles(raw string) []string { role := strings.TrimSpace(raw) if role == "" { - return "User" + return []string{} } switch strings.ToLower(role) { case "admin", "signoz-admin": - return "Admin" + return []string{"Admin"} case "editor", "signoz-editor": - return "Editor" + return []string{"Editor"} case "viewer", "signoz-viewer": - return "Viewer" + return []string{"Viewer"} default: - return role + return []string{role} } } diff --git a/pkg/accessreview/drivers/signoz_test.go b/pkg/accessreview/drivers/signoz_test.go index a1ee90cf6..e861f8ffd 100644 --- a/pkg/accessreview/drivers/signoz_test.go +++ b/pkg/accessreview/drivers/signoz_test.go @@ -47,7 +47,7 @@ func TestSigNozDriver(t *testing.T) { // ADMIN role -> admin. assert.Equal(t, "admin@example.com", records[0].Email) assert.Equal(t, "Admin User", records[0].FullName) - assert.Equal(t, "Admin", records[0].Role) + assert.Equal(t, []string{"Admin"}, records[0].Roles) assert.True(t, records[0].IsAdmin) assert.Equal(t, "00000000-0000-4000-8000-000000000001", records[0].ExternalID) assert.Equal(t, coredata.MFAStatusUnknown, records[0].MFAStatus) @@ -57,25 +57,25 @@ func TestSigNozDriver(t *testing.T) { // isRoot -> admin even with a non-admin role. assert.Equal(t, "owner@example.com", records[1].Email) - assert.Equal(t, "Viewer", records[1].Role) + assert.Equal(t, []string{"Viewer"}, records[1].Roles) assert.True(t, records[1].IsAdmin) // Managed-role display name -> Editor; not admin. assert.Equal(t, "editor@example.com", records[2].Email) - assert.Equal(t, "Editor", records[2].Role) + assert.Equal(t, []string{"Editor"}, records[2].Roles) assert.False(t, records[2].IsAdmin) require.NotNil(t, records[2].Active) assert.True(t, *records[2].Active) // pending_invite -> inactive. assert.Equal(t, "invited@example.com", records[3].Email) - assert.Equal(t, "Viewer", records[3].Role) + assert.Equal(t, []string{"Viewer"}, records[3].Roles) require.NotNil(t, records[3].Active) assert.False(t, *records[3].Active) // deleted -> inactive. assert.Equal(t, "removed@example.com", records[4].Email) - assert.Equal(t, "Editor", records[4].Role) + assert.Equal(t, []string{"Editor"}, records[4].Roles) require.NotNil(t, records[4].Active) assert.False(t, *records[4].Active) @@ -121,22 +121,22 @@ func TestSigNozDriverListAccountsErrorStatus(t *testing.T) { assert.Contains(t, err.Error(), "unexpected status 403") } -func TestSigNozRole(t *testing.T) { +func TestSigNozRoles(t *testing.T) { t.Parallel() - for in, want := range map[string]string{ - "ADMIN": "Admin", - "signoz-admin": "Admin", - "EDITOR": "Editor", - "signoz-editor": "Editor", - "VIEWER": "Viewer", - "signoz-viewer": "Viewer", - "": "User", - " ": "User", - "custom-role": "custom-role", // unknown role preserved verbatim - "superadmin": "superadmin", // contains "admin" but must NOT be promoted + for in, want := range map[string][]string{ + "ADMIN": {"Admin"}, + "signoz-admin": {"Admin"}, + "EDITOR": {"Editor"}, + "signoz-editor": {"Editor"}, + "VIEWER": {"Viewer"}, + "signoz-viewer": {"Viewer"}, + "": {}, + " ": {}, + "custom-role": {"custom-role"}, // unknown role preserved verbatim + "superadmin": {"superadmin"}, // contains "admin" but must NOT be promoted } { - assert.Equalf(t, want, sigNozRole(in), "role %q", in) + assert.Equalf(t, want, sigNozRoles(in), "role %q", in) } } diff --git a/pkg/accessreview/drivers/slack.go b/pkg/accessreview/drivers/slack.go index d7b38ae55..e539062eb 100644 --- a/pkg/accessreview/drivers/slack.go +++ b/pkg/accessreview/drivers/slack.go @@ -100,7 +100,7 @@ func (d *SlackDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error) Email: m.Profile.Email, FullName: m.RealName, JobTitle: m.Profile.Title, - Role: slackRole(m), + Roles: slackRoles(m), Active: new(!m.Deleted), IsAdmin: m.IsAdmin || m.IsOwner || m.IsPrimaryOwner, ExternalID: m.ID, @@ -163,20 +163,20 @@ func (d *SlackDriver) queryUsers(ctx context.Context, cursor string) (*slackUser return &resp, nil } -func slackRole(m slackMember) string { +func slackRoles(m slackMember) []string { switch { case m.IsPrimaryOwner: - return "Primary Owner" + return []string{"Primary Owner"} case m.IsOwner: - return "Owner" + return []string{"Owner"} case m.IsAdmin: - return "Admin" + return []string{"Admin"} case m.IsUltraRestricted: - return "Ultra Restricted" + return []string{"Ultra Restricted"} case m.IsRestricted: - return "Restricted" + return []string{"Restricted"} default: - return "Member" + return []string{"Member"} } } diff --git a/pkg/accessreview/drivers/slack_test.go b/pkg/accessreview/drivers/slack_test.go index 9aff2dda9..d4e2c52a1 100644 --- a/pkg/accessreview/drivers/slack_test.go +++ b/pkg/accessreview/drivers/slack_test.go @@ -46,5 +46,5 @@ func TestSlackDriver(t *testing.T) { require.NotEmpty(t, r.Email, "expected at least one record with an email") assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/supabase.go b/pkg/accessreview/drivers/supabase.go index c98f79849..912c6a1f9 100644 --- a/pkg/accessreview/drivers/supabase.go +++ b/pkg/accessreview/drivers/supabase.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "go.probo.inc/probo/pkg/coredata" ) @@ -62,10 +63,17 @@ func (d *SupabaseDriver) ListAccounts(ctx context.Context) ([]AccountRecord, err isAdmin := m.RoleName == "Owner" || m.RoleName == "Administrator" + role := strings.TrimSpace(m.RoleName) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: m.Email, FullName: m.UserName, - Role: m.RoleName, + Roles: roles, IsAdmin: isAdmin, ExternalID: m.UserID, MFAStatus: mfaStatus, diff --git a/pkg/accessreview/drivers/supabase_test.go b/pkg/accessreview/drivers/supabase_test.go index ecdfbe474..e28901edd 100644 --- a/pkg/accessreview/drivers/supabase_test.go +++ b/pkg/accessreview/drivers/supabase_test.go @@ -42,5 +42,5 @@ func TestSupabaseDriver(t *testing.T) { r := records[0] assert.NotEmpty(t, r.FullName) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) } diff --git a/pkg/accessreview/drivers/tailscale.go b/pkg/accessreview/drivers/tailscale.go index d984d0f68..e4441e3ce 100644 --- a/pkg/accessreview/drivers/tailscale.go +++ b/pkg/accessreview/drivers/tailscale.go @@ -75,10 +75,17 @@ func (d *TailscaleDriver) ListAccounts(ctx context.Context) ([]AccountRecord, er continue } + role := strings.TrimSpace(u.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + record := AccountRecord{ Email: email, FullName: u.DisplayName, - Role: u.Role, + Roles: roles, Active: tailscaleUserActive(u.Status), IsAdmin: tailscaleUserIsAdmin(u.Role), ExternalID: u.ID, diff --git a/pkg/accessreview/drivers/tailscale_test.go b/pkg/accessreview/drivers/tailscale_test.go index 6e0db2e03..b059d8a0d 100644 --- a/pkg/accessreview/drivers/tailscale_test.go +++ b/pkg/accessreview/drivers/tailscale_test.go @@ -37,6 +37,6 @@ func TestTailscaleDriver(t *testing.T) { r := records[0] assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.ExternalID) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) assert.NotNil(t, r.Active) } diff --git a/pkg/accessreview/drivers/tally.go b/pkg/accessreview/drivers/tally.go index 79e78dd59..ac3b9609f 100644 --- a/pkg/accessreview/drivers/tally.go +++ b/pkg/accessreview/drivers/tally.go @@ -178,7 +178,7 @@ func (d *TallyDriver) listInvites(ctx context.Context) ([]AccountRecord, error) MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, AccountType: coredata.AccessReviewEntryAccountTypeUser, - Role: "Invited", + Roles: tallyRoles(), } if record.Email != "" { @@ -188,3 +188,7 @@ func (d *TallyDriver) listInvites(ctx context.Context) ([]AccountRecord, error) return records, nil } + +func tallyRoles() []string { + return []string{"Invited"} +} diff --git a/pkg/accessreview/drivers/vercel.go b/pkg/accessreview/drivers/vercel.go index ff6b441d2..10cf2b97b 100644 --- a/pkg/accessreview/drivers/vercel.go +++ b/pkg/accessreview/drivers/vercel.go @@ -21,6 +21,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "go.probo.inc/probo/pkg/coredata" ) @@ -92,11 +93,18 @@ func (d *VercelDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error fullName = m.Username } + role := strings.TrimSpace(m.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + confirmed := m.Confirmed record := AccountRecord{ Email: m.Email, FullName: fullName, - Role: m.Role, + Roles: roles, Active: &confirmed, IsAdmin: m.Role == "OWNER" || m.Role == "owner", MFAStatus: coredata.MFAStatusUnknown, diff --git a/pkg/accessreview/drivers/vercel_test.go b/pkg/accessreview/drivers/vercel_test.go index 743fa9ead..9516fc3fd 100644 --- a/pkg/accessreview/drivers/vercel_test.go +++ b/pkg/accessreview/drivers/vercel_test.go @@ -43,7 +43,7 @@ func TestVercelDriver(t *testing.T) { assert.NotEmpty(t, r.Email) assert.NotEmpty(t, r.ExternalID) assert.NotEmpty(t, r.FullName) - assert.NotEmpty(t, r.Role) + assert.NotEmpty(t, r.Roles) assert.True(t, r.IsAdmin) require.NotNil(t, r.Active) assert.True(t, *r.Active) diff --git a/pkg/accessreview/drivers/zendesk.go b/pkg/accessreview/drivers/zendesk.go index a01dac695..a1d2c8f5e 100644 --- a/pkg/accessreview/drivers/zendesk.go +++ b/pkg/accessreview/drivers/zendesk.go @@ -21,6 +21,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "go.probo.inc/probo/pkg/coredata" ) @@ -129,10 +130,17 @@ func zendeskRecord(u zendeskUser) AccountRecord { lastLogin = *u.LastLoginAt } + role := strings.TrimSpace(u.Role) + + roles := []string{} + if role != "" { + roles = []string{role} + } + return AccountRecord{ Email: u.Email, FullName: u.Name, - Role: u.Role, + Roles: roles, Active: &active, IsAdmin: isAdmin, MFAStatus: mfaStatus, diff --git a/pkg/accessreview/drivers/zendesk_test.go b/pkg/accessreview/drivers/zendesk_test.go index a9f3c887c..08278a9da 100644 --- a/pkg/accessreview/drivers/zendesk_test.go +++ b/pkg/accessreview/drivers/zendesk_test.go @@ -44,7 +44,7 @@ func TestZendeskDriver(t *testing.T) { require.NotNil(t, r.Active) assert.True(t, *r.Active) assert.True(t, r.IsAdmin) - assert.Equal(t, "admin", r.Role) + assert.Equal(t, []string{"admin"}, r.Roles) assert.Equal(t, coredata.AccessReviewEntryAccountTypeUser, r.AccountType) assert.Equal(t, coredata.MFAStatusEnabled, r.MFAStatus) assert.Equal(t, coredata.AccessReviewEntryAuthMethodUnknown, r.AuthMethod) @@ -59,7 +59,7 @@ func TestZendeskDriver(t *testing.T) { require.NotNil(t, r2.Active) assert.True(t, *r2.Active) assert.False(t, r2.IsAdmin) - assert.Equal(t, "agent", r2.Role) + assert.Equal(t, []string{"agent"}, r2.Roles) assert.Equal(t, coredata.MFAStatusDisabled, r2.MFAStatus) assert.Nil(t, r2.LastLogin) } @@ -85,7 +85,7 @@ func TestZendeskRecord_FieldMapping(t *testing.T) { require.NotNil(t, rec.Active) assert.False(t, *rec.Active, "a suspended user must be inactive") assert.Equal(t, coredata.MFAStatusUnknown, rec.MFAStatus, "null 2FA must stay unknown") - assert.Equal(t, "Light agent", rec.Role, "custom role names pass through") + assert.Equal(t, []string{"Light agent"}, rec.Roles, "custom role names pass through") assert.False(t, rec.IsAdmin) assert.Equal(t, "42", rec.ExternalID) assert.Nil(t, rec.LastLogin) diff --git a/pkg/accessreview/review_engine.go b/pkg/accessreview/review_engine.go index 0b1b97561..b213aaab1 100644 --- a/pkg/accessreview/review_engine.go +++ b/pkg/accessreview/review_engine.go @@ -131,7 +131,7 @@ func (s *Service) FetchSource( AccessReviewCampaignSourceID: campaignSource.ID, Email: account.Email, FullName: account.FullName, - Role: account.Role, + Roles: account.Roles, JobTitle: account.JobTitle, IsAdmin: account.IsAdmin, MFAStatus: account.MFAStatus, diff --git a/pkg/coredata/access_review_campaign_source_test.go b/pkg/coredata/access_review_campaign_source_test.go index 83b7deee6..d6507c6c0 100644 --- a/pkg/coredata/access_review_campaign_source_test.go +++ b/pkg/coredata/access_review_campaign_source_test.go @@ -41,7 +41,7 @@ func insertAccessReviewEntry(t *testing.T, ctx context.Context, client *pg.Clien AccessReviewCampaignSourceID: fx.campaignSourceID, Email: accountKey, FullName: "Snapshot User", - Role: "member", + Roles: []string{"member"}, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, AccountType: coredata.AccessReviewEntryAccountTypeUser, diff --git a/pkg/coredata/access_review_entry.go b/pkg/coredata/access_review_entry.go index a3e6f275c..e99bce987 100644 --- a/pkg/coredata/access_review_entry.go +++ b/pkg/coredata/access_review_entry.go @@ -37,7 +37,7 @@ type ( IdentityID *gid.GID `db:"identity_id"` Email string `db:"email"` FullName string `db:"full_name"` - Role string `db:"role"` + Roles []string `db:"roles"` JobTitle string `db:"job_title"` IsAdmin bool `db:"is_admin"` MFAStatus MFAStatus `db:"mfa_status"` @@ -125,7 +125,7 @@ SELECT identity_id, email, full_name, - role, + roles, job_title, is_admin, mfa_status, @@ -192,7 +192,7 @@ INSERT INTO identity_id, email, full_name, - role, + roles, job_title, is_admin, mfa_status, @@ -222,7 +222,7 @@ VALUES ( @identity_id, @email, @full_name, - @role, + COALESCE(@roles, '{}'::TEXT[]), @job_title, @is_admin, @mfa_status, @@ -254,7 +254,7 @@ VALUES ( "identity_id": e.IdentityID, "email": e.Email, "full_name": e.FullName, - "role": e.Role, + "roles": e.Roles, "job_title": e.JobTitle, "is_admin": e.IsAdmin, "mfa_status": e.MFAStatus, @@ -346,7 +346,7 @@ SELECT identity_id, email, full_name, - role, + roles, job_title, is_admin, mfa_status, @@ -414,7 +414,7 @@ SELECT identity_id, email, full_name, - role, + roles, job_title, is_admin, mfa_status, @@ -630,7 +630,7 @@ INSERT INTO access_review_entries ( identity_id, email, full_name, - role, + roles, job_title, is_admin, mfa_status, @@ -659,7 +659,7 @@ INSERT INTO access_review_entries ( @identity_id, @email, @full_name, - @role, + COALESCE(@roles, '{}'::TEXT[]), @job_title, @is_admin, @mfa_status, @@ -683,7 +683,7 @@ INSERT INTO access_review_entries ( ON CONFLICT (access_review_campaign_source_id, account_key) DO UPDATE SET email = EXCLUDED.email, full_name = EXCLUDED.full_name, - role = EXCLUDED.role, + roles = EXCLUDED.roles, job_title = EXCLUDED.job_title, is_admin = EXCLUDED.is_admin, mfa_status = EXCLUDED.mfa_status, @@ -706,7 +706,7 @@ ON CONFLICT (access_review_campaign_source_id, account_key) DO UPDATE SET "identity_id": e.IdentityID, "email": e.Email, "full_name": e.FullName, - "role": e.Role, + "roles": e.Roles, "job_title": e.JobTitle, "is_admin": e.IsAdmin, "mfa_status": e.MFAStatus, diff --git a/pkg/coredata/access_review_entry_upsert_test.go b/pkg/coredata/access_review_entry_upsert_test.go index 46f95b3b9..c688b103d 100644 --- a/pkg/coredata/access_review_entry_upsert_test.go +++ b/pkg/coredata/access_review_entry_upsert_test.go @@ -19,6 +19,7 @@ import ( "testing" "time" + "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.gearno.de/kit/pg" @@ -152,7 +153,7 @@ func TestAccessReviewEntry_Upsert_FreezesDecidedFields(t *testing.T) { originalFlags := []coredata.AccessReviewEntryFlag{coredata.AccessReviewEntryFlagNew} originalEmail := "old@example.com" originalFullName := "Old Name" - originalRole := "viewer" + originalRoles := []string{"viewer"} t0 := time.Now().UTC().Truncate(time.Microsecond) @@ -165,7 +166,7 @@ func TestAccessReviewEntry_Upsert_FreezesDecidedFields(t *testing.T) { AccessReviewCampaignSourceID: fx.campaignSourceID, Email: originalEmail, FullName: originalFullName, - Role: originalRole, + Roles: originalRoles, JobTitle: "", IsAdmin: false, MFAStatus: coredata.MFAStatusUnknown, @@ -214,7 +215,7 @@ func TestAccessReviewEntry_Upsert_FreezesDecidedFields(t *testing.T) { t2 := decisionTime.Add(1 * time.Hour) secondEmail := "new@example.com" secondFullName := "New Name" - secondRole := "admin" + secondRoles := []string{"admin"} refresh := &coredata.AccessReviewEntry{ ID: gid.New(tenantID, coredata.AccessReviewEntryEntityType), // ignored by ON CONFLICT OrganizationID: fx.organizationID, @@ -222,7 +223,7 @@ func TestAccessReviewEntry_Upsert_FreezesDecidedFields(t *testing.T) { AccessReviewCampaignSourceID: fx.campaignSourceID, Email: secondEmail, FullName: secondFullName, - Role: secondRole, + Roles: secondRoles, JobTitle: "", IsAdmin: true, MFAStatus: coredata.MFAStatusEnabled, @@ -271,7 +272,7 @@ func TestAccessReviewEntry_Upsert_FreezesDecidedFields(t *testing.T) { // Columns that ARE refreshed on every poll. assert.Equal(t, secondEmail, loaded.Email) assert.Equal(t, secondFullName, loaded.FullName) - assert.Equal(t, secondRole, loaded.Role) + assert.Equal(t, secondRoles, loaded.Roles) assert.True(t, loaded.IsAdmin) assert.Equal(t, coredata.MFAStatusEnabled, loaded.MFAStatus) assert.Equal(t, coredata.AccessReviewEntryAuthMethodSSO, loaded.AuthMethod) @@ -303,7 +304,7 @@ func TestAccessReviewEntry_Upsert_RefreshesSourceTrackingFields(t *testing.T) { AccessReviewCampaignSourceID: fx.campaignSourceID, Email: "old@example.com", FullName: "Old Name", - Role: "viewer", + Roles: []string{"viewer"}, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, AccountType: coredata.AccessReviewEntryAccountTypeUser, @@ -329,7 +330,7 @@ func TestAccessReviewEntry_Upsert_RefreshesSourceTrackingFields(t *testing.T) { AccessReviewCampaignSourceID: fx.campaignSourceID, Email: "new@example.com", FullName: "New Name", - Role: "admin", + Roles: []string{"admin"}, MFAStatus: coredata.MFAStatusEnabled, AuthMethod: coredata.AccessReviewEntryAuthMethodSSO, AccountType: coredata.AccessReviewEntryAccountTypeUser, @@ -356,7 +357,7 @@ func TestAccessReviewEntry_Upsert_RefreshesSourceTrackingFields(t *testing.T) { // Source-tracking columns advanced to the second poll's values. assert.Equal(t, "new@example.com", loaded.Email) assert.Equal(t, "New Name", loaded.FullName) - assert.Equal(t, "admin", loaded.Role) + assert.Equal(t, []string{"admin"}, loaded.Roles) assert.Equal(t, coredata.MFAStatusEnabled, loaded.MFAStatus) assert.Equal(t, coredata.AccessReviewEntryAuthMethodSSO, loaded.AuthMethod) @@ -393,7 +394,7 @@ func TestAccessReviewEntry_Upsert_InsertsActiveAccount(t *testing.T) { AccessReviewCampaignSourceID: fx.campaignSourceID, Email: "active@example.com", FullName: "Active User", - Role: "member", + Roles: []string{"member"}, MFAStatus: coredata.MFAStatusUnknown, AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, AccountType: coredata.AccessReviewEntryAccountTypeUser, @@ -424,3 +425,52 @@ func TestAccessReviewEntry_Upsert_InsertsActiveAccount(t *testing.T) { assert.Nil(t, loaded.DecidedBy) assert.Nil(t, loaded.DecidedAt) } + +func TestAccessReviewEntry_Upsert_NilRolesWritesEmptyArray(t *testing.T) { + t.Parallel() + + client := test.PGClient(t) + ctx := context.Background() + fx := seedAccessReviewEntryFixture(t, ctx, client) + + tenantID := fx.scope.GetTenantID() + t0 := time.Now().UTC().Truncate(time.Microsecond) + + entryID := gid.New(tenantID, coredata.AccessReviewEntryEntityType) + entry := &coredata.AccessReviewEntry{ + ID: entryID, + OrganizationID: fx.organizationID, + AccessReviewCampaignID: fx.campaignID, + AccessReviewCampaignSourceID: fx.campaignSourceID, + Email: "nil-roles@example.com", + FullName: "Nil Roles User", + Roles: nil, + MFAStatus: coredata.MFAStatusUnknown, + AuthMethod: coredata.AccessReviewEntryAuthMethodUnknown, + AccountType: coredata.AccessReviewEntryAccountTypeUser, + ExternalID: "ext-nil-roles", + AccountKey: "nil-roles@example.com", + IncrementalTag: coredata.AccessReviewEntryIncrementalTagNew, + Flags: []coredata.AccessReviewEntryFlag{}, + FlagReasons: []string{}, + Decision: coredata.AccessReviewEntryDecisionPending, + CreatedAt: t0, + UpdatedAt: t0, + } + + require.NoError(t, client.WithTx(ctx, func(ctx context.Context, tx pg.Tx) error { + return entry.Upsert(ctx, tx, fx.scope) + })) + + var roles []string + + require.NoError(t, client.WithConn(ctx, func(ctx context.Context, conn pg.Querier) error { + return conn.QueryRow( + ctx, + `SELECT roles FROM access_review_entries WHERE id = @id`, + pgx.StrictNamedArgs{"id": entryID}, + ).Scan(&roles) + })) + + assert.Equal(t, []string{}, roles) +} diff --git a/pkg/coredata/migrations/20260612T160000Z.sql b/pkg/coredata/migrations/20260612T160000Z.sql new file mode 100644 index 000000000..172fd8d72 --- /dev/null +++ b/pkg/coredata/migrations/20260612T160000Z.sql @@ -0,0 +1,26 @@ +-- Copyright (c) 2026 Probo Inc . +-- +-- Permission to use, copy, modify, and/or distribute this software for any +-- purpose with or without fee is hereby granted, provided that the above +-- copyright notice and this permission notice appear in all copies. +-- +-- THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +-- REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +-- AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +-- INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +-- LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +-- OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +-- PERFORMANCE OF THIS SOFTWARE. + +ALTER TABLE access_review_entries + ADD COLUMN roles TEXT[] NOT NULL DEFAULT '{}'; + +UPDATE access_review_entries +SET roles = CASE + WHEN role = '' THEN '{}'::TEXT[] + WHEN position(', ' in role) > 0 THEN string_to_array(role, ', ') + ELSE ARRAY[role] +END; + +ALTER TABLE access_review_entries + DROP COLUMN role; diff --git a/pkg/server/api/console/v1/graphql/access_review_campaign.graphql b/pkg/server/api/console/v1/graphql/access_review_campaign.graphql index 1e7a8cb7d..fd79f4a4a 100644 --- a/pkg/server/api/console/v1/graphql/access_review_campaign.graphql +++ b/pkg/server/api/console/v1/graphql/access_review_campaign.graphql @@ -377,7 +377,7 @@ type AccessReviewEntry implements Node { campaignSource: AccessReviewCampaignSource! @goField(forceResolver: true) email: String! fullName: String! - role: String! + roles: [String!]! jobTitle: String! isAdmin: Boolean! active: Boolean diff --git a/pkg/server/api/console/v1/types/access_review.go b/pkg/server/api/console/v1/types/access_review.go index ce44947af..ef49d6a94 100644 --- a/pkg/server/api/console/v1/types/access_review.go +++ b/pkg/server/api/console/v1/types/access_review.go @@ -278,6 +278,11 @@ func NewAccessReviewEntryEdge(e *coredata.AccessReviewEntry, orderBy coredata.Ac } func NewAccessReviewEntry(e *coredata.AccessReviewEntry) *AccessReviewEntry { + roles := e.Roles + if roles == nil { + roles = []string{} + } + entry := &AccessReviewEntry{ ID: e.ID, Campaign: &AccessReviewCampaign{ @@ -288,7 +293,7 @@ func NewAccessReviewEntry(e *coredata.AccessReviewEntry) *AccessReviewEntry { }, Email: e.Email, FullName: e.FullName, - Role: e.Role, + Roles: roles, JobTitle: e.JobTitle, IsAdmin: e.IsAdmin, Active: e.Active, diff --git a/pkg/server/api/mcp/v1/specification.yaml b/pkg/server/api/mcp/v1/specification.yaml index 094af4d37..6286e8200 100644 --- a/pkg/server/api/mcp/v1/specification.yaml +++ b/pkg/server/api/mcp/v1/specification.yaml @@ -7684,7 +7684,7 @@ components: - access_review_campaign_source_id - email - full_name - - role + - roles - job_title - is_admin - mfa_status @@ -7712,9 +7712,11 @@ components: full_name: type: string description: User full name - role: - type: string - description: User role in the system + roles: + type: array + items: + type: string + description: User roles in the system job_title: type: string description: User job title diff --git a/pkg/server/api/mcp/v1/types/access_review.go b/pkg/server/api/mcp/v1/types/access_review.go index bad36d688..02e9dd708 100644 --- a/pkg/server/api/mcp/v1/types/access_review.go +++ b/pkg/server/api/mcp/v1/types/access_review.go @@ -68,13 +68,18 @@ func NewAccessReviewCampaign(c *coredata.AccessReviewCampaign) *AccessReviewCamp } func NewAccessReviewEntry(e *coredata.AccessReviewEntry) *AccessReviewEntry { + roles := e.Roles + if roles == nil { + roles = []string{} + } + entry := &AccessReviewEntry{ ID: e.ID, CampaignID: e.AccessReviewCampaignID, AccessReviewCampaignSourceID: e.AccessReviewCampaignSourceID, Email: e.Email, FullName: e.FullName, - Role: e.Role, + Roles: roles, JobTitle: e.JobTitle, IsAdmin: e.IsAdmin, Active: e.Active,