coder · Emyrk · Jun 12, 2024 · Jun 12, 2024 · Jun 12, 2024 · Jun 12, 2024
diff --git a/cli/cliui/table.go b/cli/cliui/table.go
@@ -205,6 +205,24 @@ func renderTable(out any, sort string, headers table.Row, filterColumns []string
 				}
 			}
 
+			// Guard against nil dereferences
+			if v != nil {
+				rt := reflect.TypeOf(v)
+				switch rt.Kind() {
+				case reflect.Slice:
+					// By default, the behavior is '%v', which just returns a string like
+					// '[a b c]'. This will add commas in between each value.
+					strs := make([]string, 0)
+					vt := reflect.ValueOf(v)
+					for i := 0; i < vt.Len(); i++ {
+						strs = append(strs, fmt.Sprintf("%v", vt.Index(i).Interface()))
+					}
+					v = "[" + strings.Join(strs, ", ") + "]"
+				default:
+					// Leave it as it is
+				}
+			}
+
 			rowSlice[i] = v
 		}
 

diff --git a/cli/cliui/table_test.go b/cli/cliui/table_test.go
@@ -138,10 +138,10 @@ func Test_DisplayTable(t *testing.T) {
 		t.Parallel()
 
 		expected := `
-NAME  AGE  ROLES    SUB 1 NAME  SUB 1 AGE  SUB 2 NAME  SUB 2 AGE  SUB 3 INNER NAME  SUB 3 INNER AGE  SUB 4       TIME                  TIME PTR
-bar    20  [a]      bar1               21  <nil>       <nil>      bar3                           23  {bar4 24 }  2022-08-02T15:49:10Z  <nil>
-baz    30  []       baz1               31  <nil>       <nil>      baz3                           33  {baz4 34 }  2022-08-02T15:49:10Z  <nil>
-foo    10  [a b c]  foo1               11  foo2        12         foo3                           13  {foo4 14 }  2022-08-02T15:49:10Z  2022-08-02T15:49:10Z
+NAME  AGE  ROLES      SUB 1 NAME  SUB 1 AGE  SUB 2 NAME  SUB 2 AGE  SUB 3 INNER NAME  SUB 3 INNER AGE  SUB 4       TIME                  TIME PTR
+bar    20  [a]        bar1               21  <nil>       <nil>      bar3                           23  {bar4 24 }  2022-08-02T15:49:10Z  <nil>
+baz    30  []         baz1               31  <nil>       <nil>      baz3                           33  {baz4 34 }  2022-08-02T15:49:10Z  <nil>
+foo    10  [a, b, c]  foo1               11  foo2        12         foo3                           13  {foo4 14 }  2022-08-02T15:49:10Z  2022-08-02T15:49:10Z
 		`
 
 		// Test with non-pointer values.
@@ -165,10 +165,10 @@ foo    10  [a b c]  foo1               11  foo2        12         foo3
 		t.Parallel()
 
 		expected := `
-NAME  AGE  ROLES    SUB 1 NAME  SUB 1 AGE  SUB 2 NAME  SUB 2 AGE  SUB 3 INNER NAME  SUB 3 INNER AGE  SUB 4       TIME                  TIME PTR
-foo    10  [a b c]  foo1               11  foo2        12         foo3                           13  {foo4 14 }  2022-08-02T15:49:10Z  2022-08-02T15:49:10Z
-bar    20  [a]      bar1               21  <nil>       <nil>      bar3                           23  {bar4 24 }  2022-08-02T15:49:10Z  <nil>
-baz    30  []       baz1               31  <nil>       <nil>      baz3                           33  {baz4 34 }  2022-08-02T15:49:10Z  <nil>
+NAME  AGE  ROLES      SUB 1 NAME  SUB 1 AGE  SUB 2 NAME  SUB 2 AGE  SUB 3 INNER NAME  SUB 3 INNER AGE  SUB 4       TIME                  TIME PTR
+foo    10  [a, b, c]  foo1               11  foo2        12         foo3                           13  {foo4 14 }  2022-08-02T15:49:10Z  2022-08-02T15:49:10Z
+bar    20  [a]        bar1               21  <nil>       <nil>      bar3                           23  {bar4 24 }  2022-08-02T15:49:10Z  <nil>
+baz    30  []         baz1               31  <nil>       <nil>      baz3                           33  {baz4 34 }  2022-08-02T15:49:10Z  <nil>
 		`
 
 		out, err := cliui.DisplayTable(in, "age", nil)
@@ -235,12 +235,12 @@ Alice   25
 	t.Run("WithSeparator", func(t *testing.T) {
 		t.Parallel()
 		expected := `
-NAME  AGE  ROLES    SUB 1 NAME  SUB 1 AGE  SUB 2 NAME  SUB 2 AGE  SUB 3 INNER NAME  SUB 3 INNER AGE  SUB 4       TIME                  TIME PTR              
-bar    20  [a]      bar1               21  <nil>       <nil>      bar3                           23  {bar4 24 }  2022-08-02T15:49:10Z  <nil>                 
--------------------------------------------------------------------------------------------------------------------------------------------------------------
-baz    30  []       baz1               31  <nil>       <nil>      baz3                           33  {baz4 34 }  2022-08-02T15:49:10Z  <nil>                 
--------------------------------------------------------------------------------------------------------------------------------------------------------------
-foo    10  [a b c]  foo1               11  foo2        12         foo3                           13  {foo4 14 }  2022-08-02T15:49:10Z  2022-08-02T15:49:10Z 
+NAME  AGE  ROLES      SUB 1 NAME  SUB 1 AGE  SUB 2 NAME  SUB 2 AGE  SUB 3 INNER NAME  SUB 3 INNER AGE  SUB 4       TIME                  TIME PTR
+bar    20  [a]        bar1               21  <nil>       <nil>      bar3                           23  {bar4 24 }  2022-08-02T15:49:10Z  <nil>
+---------------------------------------------------------------------------------------------------------------------------------------------------------------
+baz    30  []         baz1               31  <nil>       <nil>      baz3                           33  {baz4 34 }  2022-08-02T15:49:10Z  <nil>
+---------------------------------------------------------------------------------------------------------------------------------------------------------------
+foo    10  [a, b, c]  foo1               11  foo2        12         foo3                           13  {foo4 14 }  2022-08-02T15:49:10Z  2022-08-02T15:49:10Z
 		`
 
 		var inlineIn []any

diff --git a/cli/organization.go b/cli/organization.go
@@ -18,11 +18,10 @@ import (
 
 func (r *RootCmd) organizations() *serpent.Command {
 	cmd := &serpent.Command{
-		Annotations: workspaceCommand,
-		Use:         "organizations [subcommand]",
-		Short:       "Organization related commands",
-		Aliases:     []string{"organization", "org", "orgs"},
-		Hidden:      true, // Hidden until these commands are complete.
+		Use:     "organizations [subcommand]",
+		Short:   "Organization related commands",
+		Aliases: []string{"organization", "org", "orgs"},
+		Hidden:  true, // Hidden until these commands are complete.
 		Handler: func(inv *serpent.Invocation) error {
 			return inv.Command.HelpHandler(inv)
 		},
@@ -31,6 +30,7 @@ func (r *RootCmd) organizations() *serpent.Command {
 			r.switchOrganization(),
 			r.createOrganization(),
 			r.organizationRoles(),
+			r.organizationMembers(),
 		},
 	}
 

diff --git a/cli/organizationmembers.go b/cli/organizationmembers.go
@@ -0,0 +1,52 @@
+package cli
+
+import (
+	"fmt"
+
+	"golang.org/x/xerrors"
+
+	"github.com/coder/coder/v2/cli/cliui"
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/serpent"
+)
+
+func (r *RootCmd) organizationMembers() *serpent.Command {
+	formatter := cliui.NewOutputFormatter(
+		cliui.TableFormat([]codersdk.OrganizationMemberWithName{}, []string{"username", "organization_roles"}),
+		cliui.JSONFormat(),
+	)
+
+	client := new(codersdk.Client)
+	cmd := &serpent.Command{
+		Use:     "members",
+		Short:   "List all organization members",
+		Aliases: []string{"member"},
+		Middleware: serpent.Chain(
+			serpent.RequireNArgs(0),
+			r.InitClient(client),
+		),
+		Handler: func(inv *serpent.Invocation) error {
+			ctx := inv.Context()
+			organization, err := CurrentOrganization(r, inv, client)
+			if err != nil {
+				return err
+			}
+
+			res, err := client.OrganizationMembers(ctx, organization.ID)
+			if err != nil {
+				return xerrors.Errorf("fetch members: %w", err)
+			}
+
+			out, err := formatter.Format(inv.Context(), res)
+			if err != nil {
+				return err
+			}
+
+			_, err = fmt.Fprintln(inv.Stdout, out)
+			return err
+		},
+	}
+	formatter.AttachOptions(&cmd.Options)
+
+	return cmd
+}
diff --git a/cli/organizationmembers_test.go b/cli/organizationmembers_test.go
@@ -0,0 +1,36 @@
+package cli_test
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/v2/cli/clitest"
+	"github.com/coder/coder/v2/coderd/coderdtest"
+	"github.com/coder/coder/v2/coderd/rbac"
+	"github.com/coder/coder/v2/testutil"
+)
+
+func TestListOrganizationMembers(t *testing.T) {
+	t.Parallel()
+
+	t.Run("OK", func(t *testing.T) {
+		t.Parallel()
+
+		ownerClient := coderdtest.New(t, &coderdtest.Options{})
+		owner := coderdtest.CreateFirstUser(t, ownerClient)
+		client, user := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleUserAdmin())
+
+		ctx := testutil.Context(t, testutil.WaitMedium)
+		inv, root := clitest.New(t, "organization", "members", "-c", "user_id,username,roles")
+		clitest.SetupConfig(t, client, root)
+
+		buf := new(bytes.Buffer)
+		inv.Stdout = buf
+		err := inv.WithContext(ctx).Run()
+		require.NoError(t, err)
+		require.Contains(t, buf.String(), user.Username)
+		require.Contains(t, buf.String(), owner.UserID.String())
+	})
+}
diff --git a/coderd/members.go b/coderd/members.go
@@ -1,16 +1,17 @@
 package coderd
 
 import (
+	"context"
 	"net/http"
 
 	"github.com/google/uuid"
-
-	"github.com/coder/coder/v2/coderd/database/db2sdk"
-	"github.com/coder/coder/v2/coderd/rbac"
+	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/coderd/httpmw"
+	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/codersdk"
 )
 
@@ -41,7 +42,13 @@ func (api *API) listMembers(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	httpapi.Write(ctx, rw, http.StatusOK, db2sdk.List(members, convertOrganizationMemberRow))
+	resp, err := convertOrganizationMemberRows(ctx, api.Database, members)
+	if err != nil {
+		httpapi.InternalServerError(rw, err)
+		return
+	}
+
+	httpapi.Write(ctx, rw, http.StatusOK, resp)
 }
 
 // @Summary Assign role to organization member
@@ -87,30 +94,101 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	httpapi.Write(ctx, rw, http.StatusOK, convertOrganizationMember(updatedUser))
+	resp, err := convertOrganizationMembers(ctx, api.Database, []database.OrganizationMember{updatedUser})
+	if err != nil {
+		httpapi.InternalServerError(rw, err)
+		return
+	}
+	if len(resp) != 1 {
+		httpapi.InternalServerError(rw, xerrors.Errorf("failed to serialize member to response, update still succeeded"))
+		return
+	}
+	httpapi.Write(ctx, rw, http.StatusOK, resp[0])
 }
 
-func convertOrganizationMember(mem database.OrganizationMember) codersdk.OrganizationMember {
-	convertedMember := codersdk.OrganizationMember{
-		UserID:         mem.UserID,
-		OrganizationID: mem.OrganizationID,
-		CreatedAt:      mem.CreatedAt,
-		UpdatedAt:      mem.UpdatedAt,
-		Roles:          make([]codersdk.SlimRole, 0, len(mem.Roles)),
+// convertOrganizationMembers batches the role lookup to make only 1 sql call
+// We
+func convertOrganizationMembers(ctx context.Context, db database.Store, mems []database.OrganizationMember) ([]codersdk.OrganizationMember, error) {
+	converted := make([]codersdk.OrganizationMember, 0, len(mems))
+	roleLookup := make([]database.NameOrganizationPair, 0)
+
+	for _, m := range mems {
+		converted = append(converted, codersdk.OrganizationMember{
+			UserID:         m.UserID,
+			OrganizationID: m.OrganizationID,
+			CreatedAt:      m.CreatedAt,
+			UpdatedAt:      m.UpdatedAt,
+			Roles: db2sdk.List(m.Roles, func(r string) codersdk.SlimRole {
+				// If it is a built-in role, no lookups are needed.
+				rbacRole, err := rbac.RoleByName(rbac.RoleIdentifier{Name: r, OrganizationID: m.OrganizationID})
+				if err == nil {
+					return db2sdk.SlimRole(rbacRole)
+				}
+
+				// We know the role name and the organization ID. We are missing the
+				// display name. Append the lookup parameter, so we can get the display name
+				roleLookup = append(roleLookup, database.NameOrganizationPair{
+					Name:           r,
+					OrganizationID: m.OrganizationID,
+				})
+				return codersdk.SlimRole{
+					Name:           r,
+					DisplayName:    "",
+					OrganizationID: m.OrganizationID.String(),
+				}
+			}),
+		})
+	}
+
+	customRoles, err := db.CustomRoles(ctx, database.CustomRolesParams{
+		LookupRoles:     roleLookup,
+		ExcludeOrgRoles: false,
+		OrganizationID:  uuid.UUID{},
+	})
+	if err != nil {
+		// We are missing the display names, but that is not absolutely required. So just
+		// return the converted and the names will be used instead of the display names.
+		return converted, xerrors.Errorf("lookup custom roles: %w", err)
+	}
+
+	// Now map the customRoles back to the slimRoles for their display name.
+	customRolesMap := make(map[string]database.CustomRole)
+	for _, role := range customRoles {
+		customRolesMap[role.RoleIdentifier().UniqueName()] = role
 	}
 
-	for _, roleName := range mem.Roles {
-		rbacRole, _ := rbac.RoleByName(rbac.RoleIdentifier{Name: roleName, OrganizationID: mem.OrganizationID})
-		convertedMember.Roles = append(convertedMember.Roles, db2sdk.SlimRole(rbacRole))
+	for i := range converted {
+		for j, role := range converted[i].Roles {
+			if cr, ok := customRolesMap[role.UniqueName()]; ok {
+				converted[i].Roles[j].DisplayName = cr.DisplayName
+			}
+		}
 	}
-	return convertedMember
+
+	return converted, nil
 }
 
-func convertOrganizationMemberRow(row database.OrganizationMembersRow) codersdk.OrganizationMemberWithName {
-	convertedMember := codersdk.OrganizationMemberWithName{
-		Username:           row.Username,
-		OrganizationMember: convertOrganizationMember(row.OrganizationMember),
+func convertOrganizationMemberRows(ctx context.Context, db database.Store, rows []database.OrganizationMembersRow) ([]codersdk.OrganizationMemberWithName, error) {
+	members := make([]database.OrganizationMember, 0)
+	for _, row := range rows {
+		members = append(members, row.OrganizationMember)
+	}
+
+	convertedMembers, err := convertOrganizationMembers(ctx, db, members)
+	if err != nil {
+		return nil, err
+	}
+	if len(convertedMembers) != len(rows) {
+		return nil, xerrors.Errorf("conversion failed, mismatch slice lengths")
+	}
+
+	converted := make([]codersdk.OrganizationMemberWithName, 0)
+	for i := range convertedMembers {
+		converted = append(converted, codersdk.OrganizationMemberWithName{
+			Username:           rows[i].Username,
+			OrganizationMember: convertedMembers[i],
+		})
 	}
 
-	return convertedMember
+	return converted, nil
 }
diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go
@@ -96,6 +96,10 @@ func (r RoleIdentifier) String() string {
 	return r.Name
 }
 
+func (r RoleIdentifier) UniqueName() string {
+	return r.String()
+}
+
 func (r *RoleIdentifier) MarshalJSON() ([]byte, error) {
 	return json.Marshal(r.String())
 }

diff --git a/codersdk/organizations.go b/codersdk/organizations.go
@@ -51,11 +51,11 @@ type Organization struct {
 }
 
 type OrganizationMember struct {
-	UserID         uuid.UUID  `db:"user_id" json:"user_id" format:"uuid"`
-	OrganizationID uuid.UUID  `db:"organization_id" json:"organization_id" format:"uuid"`
-	CreatedAt      time.Time  `db:"created_at" json:"created_at" format:"date-time"`
-	UpdatedAt      time.Time  `db:"updated_at" json:"updated_at" format:"date-time"`
-	Roles          []SlimRole `db:"roles" json:"roles"`
+	UserID         uuid.UUID  `table:"user id" json:"user_id" format:"uuid"`
+	OrganizationID uuid.UUID  `table:"organization id" json:"organization_id" format:"uuid"`
+	CreatedAt      time.Time  `table:"created at" json:"created_at" format:"date-time"`
+	UpdatedAt      time.Time  `table:"updated at" json:"updated_at" format:"date-time"`
+	Roles          []SlimRole `table:"organization_roles" json:"roles"`
 }
 
 type OrganizationMemberWithName struct {

diff --git a/codersdk/roles.go b/codersdk/roles.go
@@ -19,6 +19,22 @@ type SlimRole struct {
 	OrganizationID string `json:"organization_id,omitempty"`
 }
 
+func (s SlimRole) String() string {
+	if s.DisplayName != "" {
+		return s.DisplayName
+	}
+	return s.Name
+}
+
+// UniqueName concatenates the organization ID to create a globally unique
+// string name for the role.
+func (s SlimRole) UniqueName() string {
+	if s.OrganizationID != "" {
+		return s.Name + ":" + s.OrganizationID
+	}
+	return s.Name
+}
+
 type AssignableRoles struct {
 	Role       `table:"r,recursive_inline"`
 	Assignable bool `json:"assignable" table:"assignable"`