🌐 US-Proxy
class="logged-out env-production page-responsive" style="word-wrap: break-word;" >
Skip to content

Commit d70055a

Browse files
jdomeracki-codersreya
jdomeracki-coder
and
sreya
authored
fix: only return group member count for workspace acl (#26206) (#27882)
Backport of #26206 Original PR: #26206 — fix: only return group member count for workspace acl Merge commit: c7ddcce Requested by: @jdomeracki-coder Clean cherry-pick, no conflicts. --- _Opened by Coder Agents on behalf of @jdomeracki-coder._ Co-authored-by: Jon Ayers <jon@coder.com>
1 parent 5c059ba commit d70055a

5 files changed

Lines changed: 150 additions & 35 deletions

File tree

‎cli/sharing.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -312,13 +312,14 @@ func workspaceACLToTable(ctx context.Context, acl *codersdk.WorkspaceACL) (strin
312312
continue
313313
}
314314

315-
for _, user := range group.Members {
316-
outputRows = append(outputRows, workspaceShareRow{
317-
User: user.Username,
318-
Group: group.Name,
319-
Role: group.Role,
320-
})
321-
}
315+
// The ACL endpoint intentionally omits the group's member roster to
316+
// avoid leaking member PII, so we display one row per group rather
317+
// than one row per member.
318+
outputRows = append(outputRows, workspaceShareRow{
319+
User: defaultGroupDisplay,
320+
Group: group.Name,
321+
Role: group.Role,
322+
})
322323
}
323324
out, err := formatter.Format(ctx, outputRows)
324325
if err != nil {

‎cli/sharing_test.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,48 @@ func TestSharingStatus(t *testing.T) {
205205
}
206206
assert.True(t, found, "expected to find username %s with role %s in the output: %s", toShareWithUser.Username, codersdk.WorkspaceRoleUse, out.String())
207207
})
208+
209+
t.Run("ListSharedGroups", func(t *testing.T) {
210+
t.Parallel()
211+
212+
var (
213+
client, db = coderdtest.NewWithDatabase(t, nil)
214+
orgOwner = coderdtest.CreateFirstUser(t, client)
215+
workspaceOwnerClient, workspaceOwner = coderdtest.CreateAnotherUser(t, client, orgOwner.OrganizationID, rbac.ScopedRoleOrgAuditor(orgOwner.OrganizationID))
216+
workspace = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{
217+
OwnerID: workspaceOwner.ID,
218+
OrganizationID: orgOwner.OrganizationID,
219+
}).Do().Workspace
220+
ctx = testutil.Context(t, testutil.WaitMedium)
221+
)
222+
223+
// The Everyone group always exists for an organization and shares the
224+
// organization's ID. The workspace ACL endpoint no longer returns the
225+
// group's member roster, so the CLI must still list the group itself.
226+
err := client.UpdateWorkspaceACL(ctx, workspace.ID, codersdk.UpdateWorkspaceACL{
227+
GroupRoles: map[string]codersdk.WorkspaceRole{
228+
orgOwner.OrganizationID.String(): codersdk.WorkspaceRoleUse,
229+
},
230+
})
231+
require.NoError(t, err)
232+
233+
inv, root := clitest.New(t, "sharing", "status", workspace.Name)
234+
clitest.SetupConfig(t, workspaceOwnerClient, root)
235+
236+
out := new(bytes.Buffer)
237+
inv.Stdout = out
238+
err = inv.WithContext(ctx).Run()
239+
require.NoError(t, err)
240+
241+
found := false
242+
for _, line := range strings.Split(out.String(), "\n") {
243+
if strings.Contains(line, database.EveryoneGroup) && strings.Contains(line, string(codersdk.WorkspaceRoleUse)) {
244+
found = true
245+
break
246+
}
247+
}
248+
assert.True(t, found, "expected to find group %s with role %s in the output: %s", database.EveryoneGroup, codersdk.WorkspaceRoleUse, out.String())
249+
})
208250
}
209251

210252
func TestSharingRemove(t *testing.T) {

‎coderd/workspaces.go‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2372,13 +2372,13 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) {
23722372
return
23732373
}
23742374

2375-
// This is largely based on the template ACL implementation, and is far from
2376-
// ideal. Usually, when we use the System context it's because we need to
2377-
// run some query that won't actually be exposed to the user. That is not
2378-
// the case here. This data goes directly to an unauthorized user. We are
2379-
// just straight up breaking security promises.
2380-
//
2381-
// TODO: This needs to be fixed before GA. Currently in beta.
2375+
// Callers are authorized to read this workspace, not necessarily the
2376+
// users and groups on its ACL. We deliberately use the System context to
2377+
// look up that data, but only return minimal identity information that is
2378+
// safe to expose to anyone who can read the ACL: MinimalUser for ACL users
2379+
// (no email or other PII) and group identity plus a member count for ACL
2380+
// groups (no member roster). This mirrors the chat ACL and template
2381+
// available-ACL endpoints.
23822382

23832383
// Fetch all of the users and their organization memberships
23842384
userIDs := make([]uuid.UUID, 0, len(workspaceACL.Users))
@@ -2390,7 +2390,8 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) {
23902390
}
23912391
userIDs = append(userIDs, id)
23922392
}
2393-
// For context see https://github.com/coder/coder/pull/19375
2393+
// ACL users are returned as MinimalUser, which contains no PII, so it is
2394+
// safe to fetch them under the System context.
23942395
// nolint:gocritic
23952396
dbUsers, err := api.Database.GetUsersByIDs(dbauthz.AsSystemRestricted(ctx), userIDs)
23962397
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
@@ -2422,7 +2423,8 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) {
24222423
// before making the DB call.
24232424
dbGroups := make([]database.GetGroupsRow, 0)
24242425
if len(groupIDs) > 0 {
2425-
// For context see https://github.com/coder/coder/pull/19375
2426+
// Group identity must be visible to anyone who can read the ACL so
2427+
// that owners and shared users can see and manage entries.
24262428
// nolint:gocritic
24272429
dbGroups, err = api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{GroupIds: groupIDs})
24282430
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
@@ -2431,26 +2433,30 @@ func (api *API) workspaceACL(rw http.ResponseWriter, r *http.Request) {
24312433
}
24322434
}
24332435

2436+
// Fetch member counts for all groups in a single query to avoid an N+1
2437+
// lookup. We intentionally do not populate the per-group member rosters:
2438+
// callers authorized to read the ACL are not necessarily authorized to
2439+
// read group membership, and the roster includes member PII. Only the
2440+
// total member count is returned (see Group.TotalMemberCount).
2441+
// nolint:gocritic
2442+
countRows, err := api.Database.GetGroupMembersCountByGroupIDs(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDsParams{
2443+
GroupIds: groupIDs,
2444+
IncludeSystem: false,
2445+
})
2446+
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
2447+
httpapi.InternalServerError(rw, err)
2448+
return
2449+
}
2450+
countByGroup := make(map[uuid.UUID]int64, len(countRows))
2451+
for _, row := range countRows {
2452+
countByGroup[row.GroupID] = row.MemberCount
2453+
}
2454+
24342455
groups := make([]codersdk.WorkspaceGroup, 0, len(dbGroups))
24352456
for _, it := range dbGroups {
2436-
var members []database.GroupMember
2437-
// For context see https://github.com/coder/coder/pull/19375
2438-
// nolint:gocritic
2439-
members, err = api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{
2440-
GroupID: it.Group.ID,
2441-
IncludeSystem: false,
2442-
})
2443-
if err != nil {
2444-
httpapi.InternalServerError(rw, err)
2445-
return
2446-
}
24472457
groups = append(groups, codersdk.WorkspaceGroup{
2448-
Group: db2sdk.Group(database.GetGroupsRow{
2449-
Group: it.Group,
2450-
OrganizationName: it.OrganizationName,
2451-
OrganizationDisplayName: it.OrganizationDisplayName,
2452-
}, members, len(members)),
2453-
Role: convertToWorkspaceRole(workspaceACL.Groups[it.Group.ID.String()].Permissions),
2458+
Group: db2sdk.Group(it, nil, int(countByGroup[it.Group.ID])),
2459+
Role: convertToWorkspaceRole(workspaceACL.Groups[it.Group.ID.String()].Permissions),
24542460
})
24552461
}
24562462

‎enterprise/cli/sharing_test.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -216,14 +216,17 @@ func TestSharingStatus(t *testing.T) {
216216
err = inv.WithContext(ctx).Run()
217217
require.NoError(t, err)
218218

219+
// The ACL endpoint omits group member rosters to avoid leaking member
220+
// PII, so the output lists the group itself rather than its members.
219221
found := false
220222
for _, line := range strings.Split(out.String(), "\n") {
221-
if strings.Contains(line, orgMember.Username) && strings.Contains(line, string(codersdk.WorkspaceRoleUse)) && strings.Contains(line, group.Name) {
223+
if strings.Contains(line, group.Name) && strings.Contains(line, string(codersdk.WorkspaceRoleUse)) {
222224
found = true
223225
break
224226
}
225227
}
226-
assert.True(t, found, "expected to find username %s with role %s in the output: %s", orgMember.Username, codersdk.WorkspaceRoleUse, out.String())
228+
assert.True(t, found, "expected to find group %s with role %s in the output: %s", group.Name, codersdk.WorkspaceRoleUse, out.String())
229+
assert.NotContains(t, out.String(), orgMember.Username, "group member roster must not be exposed in sharing status output")
227230
})
228231
}
229232

‎enterprise/coderd/workspaces_test.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4477,6 +4477,69 @@ func TestUpdateWorkspaceACL(t *testing.T) {
44774477
require.Equal(t, workspaceACL.Groups[0].Role, codersdk.WorkspaceRoleAdmin)
44784478
})
44794479

4480+
// A user who has merely been shared a workspace must not be able to
4481+
// enumerate the full roster and PII of groups on that workspace's ACL.
4482+
// The endpoint returns the group identity and total member count only.
4483+
t.Run("GroupMembersNotReturned", func(t *testing.T) {
4484+
t.Parallel()
4485+
4486+
dv := coderdtest.DeploymentValues(t)
4487+
4488+
adminClient, adminUser := coderdenttest.New(t, &coderdenttest.Options{
4489+
Options: &coderdtest.Options{
4490+
IncludeProvisionerDaemon: true,
4491+
DeploymentValues: dv,
4492+
},
4493+
LicenseOptions: &coderdenttest.LicenseOptions{
4494+
Features: license.Features{
4495+
codersdk.FeatureTemplateRBAC: 1,
4496+
},
4497+
},
4498+
})
4499+
orgID := adminUser.OrganizationID
4500+
client, _ := coderdtest.CreateAnotherUser(t, adminClient, orgID)
4501+
sharedClient, sharedUser := coderdtest.CreateAnotherUser(t, adminClient, orgID)
4502+
_, member := coderdtest.CreateAnotherUser(t, adminClient, orgID)
4503+
group := coderdtest.CreateGroup(t, adminClient, orgID, "bloob", member)
4504+
4505+
tv := coderdtest.CreateTemplateVersion(t, adminClient, orgID, nil)
4506+
coderdtest.AwaitTemplateVersionJobCompleted(t, adminClient, tv.ID)
4507+
template := coderdtest.CreateTemplate(t, adminClient, orgID, tv.ID)
4508+
4509+
ws := coderdtest.CreateWorkspace(t, client, template.ID)
4510+
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, ws.LatestBuild.ID)
4511+
4512+
ctx := testutil.Context(t, testutil.WaitMedium)
4513+
err := client.UpdateWorkspaceACL(ctx, ws.ID, codersdk.UpdateWorkspaceACL{
4514+
UserRoles: map[string]codersdk.WorkspaceRole{
4515+
sharedUser.ID.String(): codersdk.WorkspaceRoleUse,
4516+
},
4517+
GroupRoles: map[string]codersdk.WorkspaceRole{
4518+
group.ID.String(): codersdk.WorkspaceRoleUse,
4519+
},
4520+
})
4521+
require.NoError(t, err)
4522+
4523+
// The low-privilege shared user can read the ACL, but must not see
4524+
// the group's member roster (which would expose member emails and
4525+
// other PII). Only the total member count is returned.
4526+
workspaceACL, err := sharedClient.WorkspaceACL(ctx, ws.ID)
4527+
require.NoError(t, err)
4528+
require.Len(t, workspaceACL.Groups, 1)
4529+
require.Equal(t, group.ID, workspaceACL.Groups[0].ID)
4530+
require.Equal(t, codersdk.WorkspaceRoleUse, workspaceACL.Groups[0].Role)
4531+
require.Equal(t, 1, workspaceACL.Groups[0].TotalMemberCount)
4532+
require.Empty(t, workspaceACL.Groups[0].Members)
4533+
4534+
// The workspace owner sees the same count-only shape; the roster is
4535+
// omitted for all callers.
4536+
workspaceACL, err = client.WorkspaceACL(ctx, ws.ID)
4537+
require.NoError(t, err)
4538+
require.Len(t, workspaceACL.Groups, 1)
4539+
require.Equal(t, 1, workspaceACL.Groups[0].TotalMemberCount)
4540+
require.Empty(t, workspaceACL.Groups[0].Members)
4541+
})
4542+
44804543
t.Run("UnknownIDs", func(t *testing.T) {
44814544
t.Parallel()
44824545

0 commit comments

Comments
 (0)