From ab8e0cb0a5df595be0b7c3d6804847b3ce12b129 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 18 May 2026 21:29:09 +0000 Subject: [PATCH 1/7] fix(enterprise/coderd): drop N+1 in template ACL available endpoint PLAT-149: The /api/v2/templates/{template}/acl/available endpoint made 1 + 2*N database queries (one per group for members + one per group for the count), causing ~2 minute responses on deployments with several thousand groups. The frontend only renders Group.TotalMemberCount on this endpoint and never displays per-group members. Replace the per-group lookups with a single aggregate query (GetGroupMembersCountByGroupIDs) and stop fetching the full member rows for each group. The endpoint now makes a constant 2 DB queries: 1. GetGroups (unchanged) 2. GetGroupMembersCountByGroupIDs (new bulk count) The Group.Members field is left empty in this response; consumers should use Group.TotalMemberCount, which is already documented as the canonical value. --- coderd/database/dbauthz/dbauthz.go | 12 +++++ coderd/database/dbauthz/dbauthz_test.go | 14 ++++++ coderd/database/dbmetrics/querymetrics.go | 8 ++++ coderd/database/dbmock/dbmock.go | 15 ++++++ coderd/database/querier.go | 5 ++ coderd/database/queries.sql.go | 50 ++++++++++++++++++++ coderd/database/queries/groupmembers.sql | 16 +++++++ enterprise/coderd/templates.go | 47 ++++++++++--------- enterprise/coderd/templates_test.go | 56 +++++++++++++++++++++++ 9 files changed, 202 insertions(+), 21 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index f8733b410a94d..5b1c97c9e06f0 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -3476,6 +3476,18 @@ func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, arg databas return memberCount, nil } +func (q *querier) GetGroupMembersCountByGroupIDs(ctx context.Context, arg database.GetGroupMembersCountByGroupIDsParams) ([]database.GetGroupMembersCountByGroupIDsRow, error) { + // Mirror GetGroupMembersCountByGroupID: the caller needs ResourceGroup + // read access on each group, but does not need ResourceGroupMember + // read access for the count itself. + for _, id := range arg.GroupIds { + if _, err := q.GetGroupByID(ctx, id); err != nil { // AuthZ check + return nil, err + } + } + return q.db.GetGroupMembersCountByGroupIDs(ctx, arg) +} + func (q *querier) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err == nil { // Optimize this query for system users as it is used in telemetry. diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 22fb3902f7573..c462302a0768c 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1820,6 +1820,20 @@ func (s *MethodTestSuite) TestGroup() { check.Args(arg).Asserts(g, policy.ActionRead) })) + s.Run("GetGroupMembersCountByGroupIDs", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { + g1 := testutil.Fake(s.T(), faker, database.Group{}) + g2 := testutil.Fake(s.T(), faker, database.Group{}) + arg := database.GetGroupMembersCountByGroupIDsParams{GroupIds: []uuid.UUID{g1.ID, g2.ID}, IncludeSystem: false} + rows := []database.GetGroupMembersCountByGroupIDsRow{ + {GroupID: g1.ID, MemberCount: 1}, + {GroupID: g2.ID, MemberCount: 2}, + } + dbm.EXPECT().GetGroupByID(gomock.Any(), g1.ID).Return(g1, nil).AnyTimes() + dbm.EXPECT().GetGroupByID(gomock.Any(), g2.ID).Return(g2, nil).AnyTimes() + dbm.EXPECT().GetGroupMembersCountByGroupIDs(gomock.Any(), arg).Return(rows, nil).AnyTimes() + check.Args(arg).Asserts(g1, policy.ActionRead, g2, policy.ActionRead).Returns(rows) + })) + s.Run("GetGroupMembers", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { dbm.EXPECT().GetGroupMembers(gomock.Any(), false).Return([]database.GroupMember{}, nil).AnyTimes() check.Args(false).Asserts(rbac.ResourceSystem, policy.ActionRead) diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 2e8cb7c306c96..fc07c3e6a8432 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -1945,6 +1945,14 @@ func (m queryMetricsStore) GetGroupMembersCountByGroupID(ctx context.Context, ar return r0, r1 } +func (m queryMetricsStore) GetGroupMembersCountByGroupIDs(ctx context.Context, arg database.GetGroupMembersCountByGroupIDsParams) ([]database.GetGroupMembersCountByGroupIDsRow, error) { + start := time.Now() + r0, r1 := m.s.GetGroupMembersCountByGroupIDs(ctx, arg) + m.queryLatencies.WithLabelValues("GetGroupMembersCountByGroupIDs").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetGroupMembersCountByGroupIDs").Inc() + return r0, r1 +} + func (m queryMetricsStore) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) { start := time.Now() r0, r1 := m.s.GetGroups(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 46e61d8cbbff7..1832525d7c0dc 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -3616,6 +3616,21 @@ func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupID(ctx, arg any) *go return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupID", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupID), ctx, arg) } +// GetGroupMembersCountByGroupIDs mocks base method. +func (m *MockStore) GetGroupMembersCountByGroupIDs(ctx context.Context, arg database.GetGroupMembersCountByGroupIDsParams) ([]database.GetGroupMembersCountByGroupIDsRow, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetGroupMembersCountByGroupIDs", ctx, arg) + ret0, _ := ret[0].([]database.GetGroupMembersCountByGroupIDsRow) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetGroupMembersCountByGroupIDs indicates an expected call of GetGroupMembersCountByGroupIDs. +func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupIDs(ctx, arg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupMembersCountByGroupIDs", reflect.TypeOf((*MockStore)(nil).GetGroupMembersCountByGroupIDs), ctx, arg) +} + // GetGroups mocks base method. func (m *MockStore) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.GetGroupsRow, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 274c8eb1796cd..d88ee73bb5616 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -473,6 +473,11 @@ type sqlcQuerier interface { // count even if the caller does not have read access to ResourceGroupMember. // They only need ResourceGroup read access. GetGroupMembersCountByGroupID(ctx context.Context, arg GetGroupMembersCountByGroupIDParams) (int64, error) + // Returns the total member count for each of the given group IDs in a + // single query. Used to avoid N+1 lookups when listing many groups. Like + // GetGroupMembersCountByGroupID, the count is returned even when the + // caller does not have read access to individual group members. + GetGroupMembersCountByGroupIDs(ctx context.Context, arg GetGroupMembersCountByGroupIDsParams) ([]GetGroupMembersCountByGroupIDsRow, error) GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error) GetHealthSettings(ctx context.Context) (string, error) GetInboxNotificationByID(ctx context.Context, id uuid.UUID) (InboxNotification, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 5fbc873c701de..42c79b5b96c99 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -12689,6 +12689,56 @@ func (q *sqlQuerier) GetGroupMembersCountByGroupID(ctx context.Context, arg GetG return count, err } +const getGroupMembersCountByGroupIDs = `-- name: GetGroupMembersCountByGroupIDs :many +SELECT + group_id, + COUNT(*) AS member_count +FROM group_members_expanded +WHERE group_id = ANY($1 :: uuid[]) + AND CASE + WHEN $2::bool THEN TRUE + ELSE user_is_system = false + END +GROUP BY group_id +` + +type GetGroupMembersCountByGroupIDsParams struct { + GroupIds []uuid.UUID `db:"group_ids" json:"group_ids"` + IncludeSystem bool `db:"include_system" json:"include_system"` +} + +type GetGroupMembersCountByGroupIDsRow struct { + GroupID uuid.UUID `db:"group_id" json:"group_id"` + MemberCount int64 `db:"member_count" json:"member_count"` +} + +// Returns the total member count for each of the given group IDs in a +// single query. Used to avoid N+1 lookups when listing many groups. Like +// GetGroupMembersCountByGroupID, the count is returned even when the +// caller does not have read access to individual group members. +func (q *sqlQuerier) GetGroupMembersCountByGroupIDs(ctx context.Context, arg GetGroupMembersCountByGroupIDsParams) ([]GetGroupMembersCountByGroupIDsRow, error) { + rows, err := q.db.QueryContext(ctx, getGroupMembersCountByGroupIDs, pq.Array(arg.GroupIds), arg.IncludeSystem) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetGroupMembersCountByGroupIDsRow + for rows.Next() { + var i GetGroupMembersCountByGroupIDsRow + if err := rows.Scan(&i.GroupID, &i.MemberCount); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const insertGroupMember = `-- name: InsertGroupMember :exec INSERT INTO group_members (user_id, group_id) diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 4e5469317aade..fd167d219a716 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -142,6 +142,22 @@ WHERE group_id = @group_id user_is_system = false END; +-- name: GetGroupMembersCountByGroupIDs :many +-- Returns the total member count for each of the given group IDs in a +-- single query. Used to avoid N+1 lookups when listing many groups. Like +-- GetGroupMembersCountByGroupID, the count is returned even when the +-- caller does not have read access to individual group members. +SELECT + group_id, + COUNT(*) AS member_count +FROM group_members_expanded +WHERE group_id = ANY(@group_ids :: uuid[]) + AND CASE + WHEN @include_system::bool THEN TRUE + ELSE user_is_system = false + END +GROUP BY group_id; + -- InsertUserGroupsByID adds a user to all provided groups, if they exist. -- name: InsertUserGroupsByID :many WITH groups AS ( diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index 62c1b355678c4..bdf3d628877d5 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -60,29 +60,34 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req return } - sdkGroups := make([]codersdk.Group, 0, len(groups)) - for _, group := range groups { - // nolint:gocritic - members, err := api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{ - GroupID: group.Group.ID, - IncludeSystem: false, - }) - if err != nil { - httpapi.InternalServerError(rw, err) - return - } + // Fetch member counts for all groups in a single query to avoid an + // N+1 lookup pattern that was making this endpoint extremely slow on + // deployments with many groups. The per-group member lists are + // intentionally not populated here: callers of this endpoint only + // surface total_member_count (see Group.TotalMemberCount, which is + // already documented as the canonical value). + groupIDs := make([]uuid.UUID, len(groups)) + for i, g := range groups { + groupIDs[i] = g.Group.ID + } - // nolint:gocritic - memberCount, err := api.Database.GetGroupMembersCountByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDParams{ - GroupID: group.Group.ID, - IncludeSystem: false, - }) - if err != nil { - httpapi.InternalServerError(rw, err) - return - } + // nolint:gocritic // Same justification as the GetGroups call above. + countRows, err := api.Database.GetGroupMembersCountByGroupIDs(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersCountByGroupIDsParams{ + GroupIds: groupIDs, + IncludeSystem: false, + }) + if err != nil { + httpapi.InternalServerError(rw, err) + return + } + countByGroup := make(map[uuid.UUID]int64, len(countRows)) + for _, row := range countRows { + countByGroup[row.GroupID] = row.MemberCount + } - sdkGroups = append(sdkGroups, db2sdk.Group(group, members, int(memberCount))) + sdkGroups := make([]codersdk.Group, 0, len(groups)) + for _, group := range groups { + sdkGroups = append(sdkGroups, db2sdk.Group(group, nil, int(countByGroup[group.Group.ID]))) } httpapi.Write(ctx, rw, http.StatusOK, codersdk.ACLAvailable{ diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index 57c0977be1364..f5edfa7f3c950 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -1241,6 +1241,62 @@ func TestTemplateACL(t *testing.T) { }) require.NoError(t, err) }) + + // Regression test for PLAT-149. Previously this endpoint did an N+1 + // fetch of every group's members and member count. Verify that the + // member count is returned correctly for many groups, and that the + // per-group members list is no longer populated (callers should rely + // on TotalMemberCount). + t.Run("AvailableReturnsGroupMemberCounts", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + admin, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()) + + // Create a couple of users we can stuff into groups. + _, alice := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + _, bob := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + _, carol := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + // emptyGroup: zero non-system members. + // singleGroup: alice only. + // fullGroup: alice + bob + carol. + emptyGroup := coderdtest.CreateGroup(t, admin, user.OrganizationID, "empty-group") + singleGroup := coderdtest.CreateGroup(t, admin, user.OrganizationID, "single-group", alice) + fullGroup := coderdtest.CreateGroup(t, admin, user.OrganizationID, "full-group", alice, bob, carol) + + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + + ctx := testutil.Context(t, testutil.WaitLong) + + available, err := admin.TemplateACLAvailable(ctx, template.ID) + require.NoError(t, err) + + wantCounts := map[uuid.UUID]int{ + emptyGroup.ID: 0, + singleGroup.ID: 1, + fullGroup.ID: 3, + } + + found := map[uuid.UUID]bool{} + for _, group := range available.Groups { + if want, ok := wantCounts[group.ID]; ok { + found[group.ID] = true + require.Equal(t, want, group.TotalMemberCount, + "unexpected total_member_count for group %q", group.Name) + require.Empty(t, group.Members, + "members must not be populated by the available endpoint for group %q", group.Name) + } + } + for id := range wantCounts { + require.True(t, found[id], "group %s missing from available response", id) + } + }) } func TestUpdateTemplateACL(t *testing.T) { From afc652e6c6b83dfc1afe08a5a53f5844de0a6beb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 18 May 2026 22:31:37 +0000 Subject: [PATCH 2/7] fix(enterprise/coderd): apply q and limit to groups in template ACL available The /api/v2/templates/{template}/acl/available endpoint already honored q and limit for the users half of its response, but silently ignored them for groups. The autocomplete on the template permissions page sends q=&limit=25 on every keystroke; without server-side filtering, deployments with thousands of groups would still ship every group on each keystroke even after the N+1 fix. - Add 'search' and 'limit_opt' params to GetGroups. Search applies a case-insensitive substring filter against groups.name and groups.display_name. A limit of 0 keeps the existing 'no limit' behavior so other callers of GetGroups (idpsync, telemetry, provisionerd, dynamicparameters, workspaces) are unaffected. - Add ORDER BY so pagination is stable across calls. Sort by the value the UI displays (display_name, falling back to name). - Parse q via searchquery.Users so search semantics match the users half exactly. Parse limit via ParsePagination. - Bump the codersdk TemplateACLAvailable signature to accept a UsersRequest (parity with the JS SDK and the other paginated endpoints). Update both existing call sites. Companion test TestTemplateACL/AvailableHonorsGroupSearchAndLimit covers both the q filter and the limit cap. --- coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 18 +++++++++ coderd/database/queries/groups.sql | 14 +++++++ codersdk/templates.go | 16 ++++++-- enterprise/coderd/templates.go | 17 ++++++++ enterprise/coderd/templates_test.go | 61 ++++++++++++++++++++++++++++- 6 files changed, 122 insertions(+), 5 deletions(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index d88ee73bb5616..32e38750ecaad 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -478,6 +478,7 @@ type sqlcQuerier interface { // GetGroupMembersCountByGroupID, the count is returned even when the // caller does not have read access to individual group members. GetGroupMembersCountByGroupIDs(ctx context.Context, arg GetGroupMembersCountByGroupIDsParams) ([]GetGroupMembersCountByGroupIDsRow, error) + // A limit of 0 means "no limit". GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error) GetHealthSettings(ctx context.Context) (string, error) GetInboxNotificationByID(ctx context.Context, id uuid.UUID) (InboxNotification, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 42c79b5b96c99..b63e3d3793331 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -12956,6 +12956,19 @@ WHERE groups.id = ANY($4) ELSE true END + -- Filter by group name or display name (substring, case-insensitive). + AND CASE WHEN $5 :: text != '' THEN ( + groups.name ILIKE concat('%', $5, '%') + OR groups.display_name ILIKE concat('%', $5, '%') + ) + ELSE true + END +ORDER BY + -- Deterministic ordering for stable pagination. Sort by the value + -- the UI displays (display_name, falling back to name). + LOWER(COALESCE(NULLIF(groups.display_name, ''), groups.name)) ASC, + groups.id ASC +LIMIT NULLIF($6 :: int, 0) ` type GetGroupsParams struct { @@ -12963,6 +12976,8 @@ type GetGroupsParams struct { HasMemberID uuid.UUID `db:"has_member_id" json:"has_member_id"` GroupNames []string `db:"group_names" json:"group_names"` GroupIds []uuid.UUID `db:"group_ids" json:"group_ids"` + Search string `db:"search" json:"search"` + LimitOpt int32 `db:"limit_opt" json:"limit_opt"` } type GetGroupsRow struct { @@ -12971,12 +12986,15 @@ type GetGroupsRow struct { OrganizationDisplayName string `db:"organization_display_name" json:"organization_display_name"` } +// A limit of 0 means "no limit". func (q *sqlQuerier) GetGroups(ctx context.Context, arg GetGroupsParams) ([]GetGroupsRow, error) { rows, err := q.db.QueryContext(ctx, getGroups, arg.OrganizationID, arg.HasMemberID, pq.Array(arg.GroupNames), pq.Array(arg.GroupIds), + arg.Search, + arg.LimitOpt, ) if err != nil { return nil, err diff --git a/coderd/database/queries/groups.sql b/coderd/database/queries/groups.sql index 3413e5832e27d..7a03ca69afd14 100644 --- a/coderd/database/queries/groups.sql +++ b/coderd/database/queries/groups.sql @@ -78,6 +78,20 @@ WHERE groups.id = ANY(@group_ids) ELSE true END + -- Filter by group name or display name (substring, case-insensitive). + AND CASE WHEN @search :: text != '' THEN ( + groups.name ILIKE concat('%', @search, '%') + OR groups.display_name ILIKE concat('%', @search, '%') + ) + ELSE true + END +ORDER BY + -- Deterministic ordering for stable pagination. Sort by the value + -- the UI displays (display_name, falling back to name). + LOWER(COALESCE(NULLIF(groups.display_name, ''), groups.name)) ASC, + groups.id ASC +-- A limit of 0 means "no limit". +LIMIT NULLIF(@limit_opt :: int, 0) ; -- name: InsertGroup :one diff --git a/codersdk/templates.go b/codersdk/templates.go index d0c52761189cc..87fea25fb9cc2 100644 --- a/codersdk/templates.go +++ b/codersdk/templates.go @@ -375,9 +375,19 @@ func (c *Client) UpdateTemplateACL(ctx context.Context, templateID uuid.UUID, re return nil } -// TemplateACLAvailable returns available users + groups that can be assigned template perms -func (c *Client) TemplateACLAvailable(ctx context.Context, templateID uuid.UUID) (ACLAvailable, error) { - res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s/acl/available", templateID), nil) +// TemplateACLAvailable returns available users + groups that can be assigned +// template perms. The optional req controls the q/limit/offset query +// parameters applied server-side; pass codersdk.UsersRequest{} when no +// filtering is desired. +func (c *Client) TemplateACLAvailable(ctx context.Context, templateID uuid.UUID, req UsersRequest) (ACLAvailable, error) { + res, err := c.Request( + ctx, + http.MethodGet, + fmt.Sprintf("/api/v2/templates/%s/acl/available", templateID), + nil, + req.Pagination.asRequestOption(), + req.asRequestOption(), + ) if err != nil { return ACLAvailable{}, err } diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index bdf3d628877d5..ce507e43cef15 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -9,6 +9,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog/v3" + agpl "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" @@ -17,6 +18,7 @@ import ( "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/rbac/acl" "github.com/coder/coder/v2/coderd/rbac/policy" + "github.com/coder/coder/v2/coderd/searchquery" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" ) @@ -50,10 +52,25 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req return } + // Apply the same q/limit semantics to groups as the users half of this + // response. The autocomplete sends q=&limit=25; previously these + // were ignored server-side for groups, so deployments with thousands of + // groups paid for fetching and shipping every group on each keystroke. + // api.AGPL.GetUsers above already validated both, so parsing them again + // here cannot fail in a new way. + userFilter, _ := searchquery.Users(r.URL.Query().Get("q")) + groupPagination, ok := agpl.ParsePagination(rw, r) + if !ok { + return + } + // Perm check is the template update check. // nolint:gocritic groups, err := api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{ OrganizationID: template.OrganizationID, + Search: userFilter.Search, + // #nosec G115 - Pagination limits are small and fit in int32 + LimitOpt: int32(groupPagination.Limit), }) if err != nil { httpapi.InternalServerError(rw, err) diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index f5edfa7f3c950..57acd598350f1 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -1274,7 +1274,7 @@ func TestTemplateACL(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) - available, err := admin.TemplateACLAvailable(ctx, template.ID) + available, err := admin.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{}) require.NoError(t, err) wantCounts := map[uuid.UUID]int{ @@ -1297,6 +1297,63 @@ func TestTemplateACL(t *testing.T) { require.True(t, found[id], "group %s missing from available response", id) } }) + + // Companion to the AvailableReturnsGroupMemberCounts test above. Verifies + // that the q query parameter applies a server-side substring filter on + // group name / display_name, and that limit caps the number of groups + // returned. The autocomplete sends both on each keystroke; before + // PLAT-149 both were ignored for groups. + t.Run("AvailableHonorsGroupSearchAndLimit", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + admin, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()) + + // Create a handful of groups with predictable names so we can + // pin assertions to specific substrings. + engAlpha := coderdtest.CreateGroup(t, admin, user.OrganizationID, "engineering-alpha") + engBeta := coderdtest.CreateGroup(t, admin, user.OrganizationID, "engineering-beta") + design := coderdtest.CreateGroup(t, admin, user.OrganizationID, "design") + sales := coderdtest.CreateGroup(t, admin, user.OrganizationID, "sales") + + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + + ctx := testutil.Context(t, testutil.WaitLong) + + groupIDs := func(available codersdk.ACLAvailable) []uuid.UUID { + ids := make([]uuid.UUID, 0, len(available.Groups)) + for _, g := range available.Groups { + ids = append(ids, g.ID) + } + return ids + } + + // q filters by group name / display_name substring. + filtered, err := admin.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{ + SearchQuery: "engineering", + }) + require.NoError(t, err) + got := groupIDs(filtered) + require.ElementsMatch(t, []uuid.UUID{engAlpha.ID, engBeta.ID}, got, + "q=engineering should return only engineering-* groups, got %v", got) + require.NotContains(t, got, design.ID) + require.NotContains(t, got, sales.ID) + + // limit caps the number of groups returned. With 4 user-created + // groups plus the implicit Everyone group, asking for 2 must + // return at most 2 groups. + limited, err := admin.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{ + Pagination: codersdk.Pagination{Limit: 2}, + }) + require.NoError(t, err) + require.Len(t, limited.Groups, 2, + "limit=2 should cap groups to 2, got %d", len(limited.Groups)) + }) } func TestUpdateTemplateACL(t *testing.T) { @@ -1682,7 +1739,7 @@ func TestUpdateTemplateACL(t *testing.T) { require.NoError(t, err) // Should be able to see user 3 - available, err := client2.TemplateACLAvailable(ctx, template.ID) + available, err := client2.TemplateACLAvailable(ctx, template.ID, codersdk.UsersRequest{}) require.NoError(t, err) userFound := false for _, avail := range available.Users { From b2256414fcc8a02bfce52df18d25e460eec5e569 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 09:26:47 -0500 Subject: [PATCH 3/7] diffs --- coderd/database/dbauthz/dbauthz.go | 11 ++++------- enterprise/coderd/templates.go | 9 +++------ 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 5b1c97c9e06f0..b0c27039348be 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -3477,13 +3477,10 @@ func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, arg databas } func (q *querier) GetGroupMembersCountByGroupIDs(ctx context.Context, arg database.GetGroupMembersCountByGroupIDsParams) ([]database.GetGroupMembersCountByGroupIDsRow, error) { - // Mirror GetGroupMembersCountByGroupID: the caller needs ResourceGroup - // read access on each group, but does not need ResourceGroupMember - // read access for the count itself. - for _, id := range arg.GroupIds { - if _, err := q.GetGroupByID(ctx, id); err != nil { // AuthZ check - return nil, err - } + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceGroup); err != nil { + // Ideally we would check read access on each group ID, but that would be N queries. + // So this function is really only usable by admins. + return nil, err } return q.db.GetGroupMembersCountByGroupIDs(ctx, arg) } diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index ce507e43cef15..dab834babd66a 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -52,12 +52,9 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req return } - // Apply the same q/limit semantics to groups as the users half of this - // response. The autocomplete sends q=&limit=25; previously these - // were ignored server-side for groups, so deployments with thousands of - // groups paid for fetching and shipping every group on each keystroke. - // api.AGPL.GetUsers above already validated both, so parsing them again - // here cannot fail in a new way. + // Apply the same q/limit semantics to groups as the users half of this response. + // The query semantics are defined for the users, which is awkward. But we can + // just reuse the search part of the query which is a fuzzy match. userFilter, _ := searchquery.Users(r.URL.Query().Get("q")) groupPagination, ok := agpl.ParsePagination(rw, r) if !ok { From 97078b9325262701af928a293862633211a70cbd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 11:52:02 -0500 Subject: [PATCH 4/7] fix authz test --- coderd/database/dbauthz/dbauthz_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index c462302a0768c..93c67817a556e 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1831,7 +1831,7 @@ func (s *MethodTestSuite) TestGroup() { dbm.EXPECT().GetGroupByID(gomock.Any(), g1.ID).Return(g1, nil).AnyTimes() dbm.EXPECT().GetGroupByID(gomock.Any(), g2.ID).Return(g2, nil).AnyTimes() dbm.EXPECT().GetGroupMembersCountByGroupIDs(gomock.Any(), arg).Return(rows, nil).AnyTimes() - check.Args(arg).Asserts(g1, policy.ActionRead, g2, policy.ActionRead).Returns(rows) + check.Args(arg).Asserts(rbac.ResourceGroup, policy.ActionRead).Returns(rows) })) s.Run("GetGroupMembers", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { From fb8ac7117084fb73054f3c01313d27049d630e19 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 11:57:15 -0500 Subject: [PATCH 5/7] revert group ordering --- coderd/database/queries.sql.go | 10 ++-------- coderd/database/queries/groups.sql | 5 ----- enterprise/coderd/parameters_test.go | 4 +++- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index b63e3d3793331..a310520325bf0 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -12963,11 +12963,6 @@ WHERE ) ELSE true END -ORDER BY - -- Deterministic ordering for stable pagination. Sort by the value - -- the UI displays (display_name, falling back to name). - LOWER(COALESCE(NULLIF(groups.display_name, ''), groups.name)) ASC, - groups.id ASC LIMIT NULLIF($6 :: int, 0) ` @@ -14600,9 +14595,8 @@ RETURNING id func (q *sqlQuerier) DeleteLicense(ctx context.Context, id int32) (int32, error) { row := q.db.QueryRowContext(ctx, deleteLicense, id) - var id_2 int32 - err := row.Scan(&id_2) - return id_2, err + err := row.Scan(&id) + return id, err } const getLicenseByID = `-- name: GetLicenseByID :one diff --git a/coderd/database/queries/groups.sql b/coderd/database/queries/groups.sql index 7a03ca69afd14..39742d55350f1 100644 --- a/coderd/database/queries/groups.sql +++ b/coderd/database/queries/groups.sql @@ -85,11 +85,6 @@ WHERE ) ELSE true END -ORDER BY - -- Deterministic ordering for stable pagination. Sort by the value - -- the UI displays (display_name, falling back to name). - LOWER(COALESCE(NULLIF(groups.display_name, ''), groups.name)) ASC, - groups.id ASC -- A limit of 0 means "no limit". LIMIT NULLIF(@limit_opt :: int, 0) ; diff --git a/enterprise/coderd/parameters_test.go b/enterprise/coderd/parameters_test.go index bda9e3c59e021..4522c8ad30864 100644 --- a/enterprise/coderd/parameters_test.go +++ b/enterprise/coderd/parameters_test.go @@ -35,7 +35,9 @@ func TestDynamicParametersOwnerGroups(t *testing.T) { _, noGroupUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) // Create the group to be asserted - group := coderdtest.CreateGroup(t, ownerClient, owner.OrganizationID, "bloob", templateAdminUser) + // Make the group name something after "Everyone" when sorted alphabetically. + // The test wants to check that `Everyone` is the default, which is the first alphabetical group in the test. + group := coderdtest.CreateGroup(t, ownerClient, owner.OrganizationID, "zebra", templateAdminUser) dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/groups/main.tf") require.NoError(t, err) From 4b502ec681f2e4e102268c46c10bcf495ad0b6d0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 19 May 2026 17:12:52 +0000 Subject: [PATCH 6/7] make gne --- coderd/database/queries.sql.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a310520325bf0..4505fe0e3572e 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -14595,8 +14595,9 @@ RETURNING id func (q *sqlQuerier) DeleteLicense(ctx context.Context, id int32) (int32, error) { row := q.db.QueryRowContext(ctx, deleteLicense, id) - err := row.Scan(&id) - return id, err + var id_2 int32 + err := row.Scan(&id_2) + return id_2, err } const getLicenseByID = `-- name: GetLicenseByID :one From b06490c557fd897836f8de81ae15c9333f5766a2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 20 May 2026 15:30:21 -0500 Subject: [PATCH 7/7] PR feedback --- coderd/database/dbauthz/dbauthz_test.go | 2 -- enterprise/coderd/templates.go | 9 ++++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 93c67817a556e..8a6d8393b170e 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1828,8 +1828,6 @@ func (s *MethodTestSuite) TestGroup() { {GroupID: g1.ID, MemberCount: 1}, {GroupID: g2.ID, MemberCount: 2}, } - dbm.EXPECT().GetGroupByID(gomock.Any(), g1.ID).Return(g1, nil).AnyTimes() - dbm.EXPECT().GetGroupByID(gomock.Any(), g2.ID).Return(g2, nil).AnyTimes() dbm.EXPECT().GetGroupMembersCountByGroupIDs(gomock.Any(), arg).Return(rows, nil).AnyTimes() check.Args(arg).Asserts(rbac.ResourceGroup, policy.ActionRead).Returns(rows) })) diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index dab834babd66a..9ef07271fcda3 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -55,7 +55,14 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req // Apply the same q/limit semantics to groups as the users half of this response. // The query semantics are defined for the users, which is awkward. But we can // just reuse the search part of the query which is a fuzzy match. - userFilter, _ := searchquery.Users(r.URL.Query().Get("q")) + userFilter, verr := searchquery.Users(r.URL.Query().Get("q")) + if len(verr) > 0 { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid user search query.", + Validations: verr, + }) + return + } groupPagination, ok := agpl.ParsePagination(rw, r) if !ok { return