diff --git a/cli/sharing.go b/cli/sharing.go index 61428d3b37..61c0f75dd3 100644 --- a/cli/sharing.go +++ b/cli/sharing.go @@ -312,13 +312,14 @@ func workspaceACLToTable(ctx context.Context, acl *codersdk.WorkspaceACL) (strin continue } - for _, user := range group.Members { - outputRows = append(outputRows, workspaceShareRow{ - User: user.Username, - Group: group.Name, - Role: group.Role, - }) - } + // The ACL endpoint intentionally omits the group's member roster to + // avoid leaking member PII, so we display one row per group rather + // than one row per member. + outputRows = append(outputRows, workspaceShareRow{ + User: defaultGroupDisplay, + Group: group.Name, + Role: group.Role, + }) } out, err := formatter.Format(ctx, outputRows) if err != nil { diff --git a/cli/sharing_test.go b/cli/sharing_test.go index 26ad858d09..fa8026554d 100644 --- a/cli/sharing_test.go +++ b/cli/sharing_test.go @@ -205,6 +205,48 @@ func TestSharingStatus(t *testing.T) { } assert.True(t, found, "expected to find username %s with role %s in the output: %s", toShareWithUser.Username, codersdk.WorkspaceRoleUse, out.String()) }) + + t.Run("ListSharedGroups", func(t *testing.T) { + t.Parallel() + + var ( + client, db = coderdtest.NewWithDatabase(t, nil) + orgOwner = coderdtest.CreateFirstUser(t, client) + workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID, rbac.ScopedRoleOrgAuditor(orgOwner.OrganizationID)) + workspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ + OwnerID: workspaceOwner.ID, + OrganizationID: orgOwner.OrganizationID, + }).Do().Workspace + ctx = testutil.Context(t, testutil.WaitMedium) + ) + + // The Everyone group always exists for an organization and shares the + // organization's ID. The workspace ACL endpoint no longer returns the + // group's member roster, so the CLI must still list the group itself. + err := client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{ + GroupRoles: map[string]codersdk.WorkspaceRole{ + orgOwner.OrganizationID.String(): codersdk.WorkspaceRoleUse, + }, + }) + require.NoError(t, err) + + inv, root := clitest.New(t, "sharing", "status", workspace.Name) + clitest.SetupConfig(t, workspaceOwnerClient, root) + + out := new(bytes.Buffer) + inv.Stdout = out + err = inv.WithContext(ctx).Run() + require.NoError(t, err) + + found := false + for _, line := range strings.Split(out.String(), "\n") { + if strings.Contains(line, database.EveryoneGroup) && strings.Contains(line, string(codersdk.WorkspaceRoleUse)) { + found = true + break + } + } + assert.True(t, found, "expected to find group %s with role %s in the output: %s", database.EveryoneGroup, codersdk.WorkspaceRoleUse, out.String()) + }) } func TestSharingRemove(t *testing.T) { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 92eaf464e7..2c296d6ffc 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -2300,13 +2300,13 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) { return } - // This is largely based on the template ACL implementation, and is far from - // ideal. Usually, when we use the System context it's because we need to - // run some query that won't actually be exposed to the user. That is not - // the case here. This data goes directly to an unauthorized user. We are - // just straight up breaking security promises. - // - // TODO: This needs to be fixed before GA. Currently in beta. + // Callers are authorized to read this workspace, not necessarily the + // users and groups on its ACL. We deliberately use the System context to + // look up that data, but only return minimal identity information that is + // safe to expose to anyone who can read the ACL: MinimalUser for ACL users + // (no email or other PII) and group identity plus a member count for ACL + // groups (no member roster). This mirrors the chat ACL and template + // available-ACL endpoints. // Fetch all of the users and their organization memberships userIDs := make([]uuid.UUID, 0, len(workspaceACL.Users)) @@ -2318,7 +2318,8 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) { } userIDs = append(userIDs, id) } - // For context see https://github.com/coder/coder/pull/19375 + // ACL users are returned as MinimalUser, which contains no PII, so it is + // safe to fetch them under the System context. // nolint:gocritic dbUsers, err := api.Database.GetUsersByIDs(dbauthz.AsSystemRestricted(ctx), userIDs) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { @@ -2350,7 +2351,8 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) { // before making the DB call. dbGroups := make([]database.GetGroupsRow, 0) if len(groupIDs) > 0 { - // For context see https://github.com/coder/coder/pull/19375 + // Group identity must be visible to anyone who can read the ACL so + // that owners and shared users can see and manage entries. // nolint:gocritic dbGroups, err = api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{GroupIds: groupIDs}) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { @@ -2359,26 +2361,30 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) { } } + // Fetch member counts for all groups in a single query to avoid an N+1 + // lookup. We intentionally do not populate the per-group member rosters: + // callers authorized to read the ACL are not necessarily authorized to + // read group membership, and the roster includes member PII. Only the + // total member count is returned (see Group.TotalMemberCount). + // nolint:gocritic + countRows, err := api.Database.GetGroupMembersCountByGroupIDs(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDsParams{ + GroupIds: groupIDs, + IncludeSystem: false, + }) + if err != nil && !xerrors.Is(err, sql.ErrNoRows) { + httpapi.InternalServerError(rw, err) + return + } + countByGroup := make(map[uuid.UUID]int64, len(countRows)) + for _, row := range countRows { + countByGroup[row.GroupID] = row.MemberCount + } + groups := make([]codersdk.WorkspaceGroup, 0, len(dbGroups)) for _, it := range dbGroups { - var members []database.GroupMember - // For context see https://github.com/coder/coder/pull/19375 - // nolint:gocritic - members, err = api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{ - GroupID: it.Group.ID, - IncludeSystem: false, - }) - if err != nil { - httpapi.InternalServerError(rw, err) - return - } groups = append(groups, codersdk.WorkspaceGroup{ - Group: db2sdk.Group(database.GetGroupsRow{ - Group: it.Group, - OrganizationName: it.OrganizationName, - OrganizationDisplayName: it.OrganizationDisplayName, - }, members, len(members)), - Role: convertToWorkspaceRole(workspaceACL.Groups[it.Group.ID.String()].Permissions), + Group: db2sdk.Group(it, nil, int(countByGroup[it.Group.ID])), + Role: convertToWorkspaceRole(workspaceACL.Groups[it.Group.ID.String()].Permissions), }) } diff --git a/enterprise/cli/sharing_test.go b/enterprise/cli/sharing_test.go index 6e1e3c8dd4..c972cd6cd9 100644 --- a/enterprise/cli/sharing_test.go +++ b/enterprise/cli/sharing_test.go @@ -216,14 +216,17 @@ func TestSharingStatus(t *testing.T) { err = inv.WithContext(ctx).Run() require.NoError(t, err) + // The ACL endpoint omits group member rosters to avoid leaking member + // PII, so the output lists the group itself rather than its members. found := false for _, line := range strings.Split(out.String(), "\n") { - if strings.Contains(line, orgMember.Username) && strings.Contains(line, string(codersdk.WorkspaceRoleUse)) && strings.Contains(line, group.Name) { + if strings.Contains(line, group.Name) && strings.Contains(line, string(codersdk.WorkspaceRoleUse)) { found = true break } } - assert.True(t, found, "expected to find username %s with role %s in the output: %s", orgMember.Username, codersdk.WorkspaceRoleUse, out.String()) + assert.True(t, found, "expected to find group %s with role %s in the output: %s", group.Name, codersdk.WorkspaceRoleUse, out.String()) + assert.NotContains(t, out.String(), orgMember.Username, "group member roster must not be exposed in sharing status output") }) } diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 1915fabe85..117deff7a6 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -4477,6 +4477,69 @@ func TestUpdateWorkspaceACL(t *testing.T) { require.Equal(t, workspaceACL.Groups[0].Role, codersdk.WorkspaceRoleAdmin) }) + // A user who has merely been shared a workspace must not be able to + // enumerate the full roster and PII of groups on that workspace's ACL. + // The endpoint returns the group identity and total member count only. + t.Run("GroupMembersNotReturned", func(t *testing.T) { + t.Parallel() + + dv := coderdtest.DeploymentValues(t) + + adminClient, adminUser := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + DeploymentValues: dv, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }, + }) + orgID := adminUser.OrganizationID + client, _ := coderdtest.CreateAnotherUser(t, adminClient, orgID) + sharedClient, sharedUser := coderdtest.CreateAnotherUser(t, adminClient, orgID) + _, member := coderdtest.CreateAnotherUser(t, adminClient, orgID) + group := coderdtest.CreateGroup(t, adminClient, orgID, "bloob", member) + + tv := coderdtest.CreateTemplateVersion(t, adminClient, orgID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, adminClient, tv.ID) + template := coderdtest.CreateTemplate(t, adminClient, orgID, tv.ID) + + ws := coderdtest.CreateWorkspace(t, client, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID) + + ctx := testutil.Context(t, testutil.WaitMedium) + err := client.UpdateWorkspaceACL(ctx, ws.ID, codersdk.UpdateWorkspaceACL{ + UserRoles: map[string]codersdk.WorkspaceRole{ + sharedUser.ID.String(): codersdk.WorkspaceRoleUse, + }, + GroupRoles: map[string]codersdk.WorkspaceRole{ + group.ID.String(): codersdk.WorkspaceRoleUse, + }, + }) + require.NoError(t, err) + + // The low-privilege shared user can read the ACL, but must not see + // the group's member roster (which would expose member emails and + // other PII). Only the total member count is returned. + workspaceACL, err := sharedClient.WorkspaceACL(ctx, ws.ID) + require.NoError(t, err) + require.Len(t, workspaceACL.Groups, 1) + require.Equal(t, group.ID, workspaceACL.Groups[0].ID) + require.Equal(t, codersdk.WorkspaceRoleUse, workspaceACL.Groups[0].Role) + require.Equal(t, 1, workspaceACL.Groups[0].TotalMemberCount) + require.Empty(t, workspaceACL.Groups[0].Members) + + // The workspace owner sees the same count-only shape; the roster is + // omitted for all callers. + workspaceACL, err = client.WorkspaceACL(ctx, ws.ID) + require.NoError(t, err) + require.Len(t, workspaceACL.Groups, 1) + require.Equal(t, 1, workspaceACL.Groups[0].TotalMemberCount) + require.Empty(t, workspaceACL.Groups[0].Members) + }) + t.Run("UnknownIDs", func(t *testing.T) { t.Parallel()