Skip to content

feat(enterprise/coderd): allow system users to be added to groups #18341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+543 −172
Open

feat(enterprise/coderd): allow system users to be added to groups #18341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b3a731
Allow users to view and modify group membership for the prebuilds sys…
SasSwart Jun 12, 2025
4b63826
Fix Tests
SasSwart Jun 12, 2025
90b9078
Merge remote-tracking branch 'origin/main' into jjs/prebuilds-user-gr…
SasSwart Jul 14, 2025
af7c7cd
fix tests
SasSwart Jul 15, 2025
1cd68e3
create a prebuilds group in every org that needs it
SasSwart Jul 29, 2025
4c0c4d4
Merge remote-tracking branch 'origin/main' into jjs/prebuilds-user-gr…
SasSwart Jul 29, 2025
93c46e9
add lint rule
SasSwart Jul 29, 2025
b3728d3
add quotas tests to verify prebuilds behaviour
SasSwart Jul 30, 2025
efdfe9f
fix unit test context
SasSwart Jul 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,14 @@ var (
rbac.ResourceFile.Type: {
policy.ActionRead,
},
rbac.ResourceGroup.Type: {
policy.ActionRead,
policy.ActionCreate,
policy.ActionUpdate,
},
rbac.ResourceGroupMember.Type: {
policy.ActionRead,
},
Comment on lines +488 to +495
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add comments explaining why the prebuilds user needs this privilege?

}),
},
}),
Expand Down
50 changes: 19 additions & 31 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 3 additions & 15 deletions coderd/database/queries/groupmembers.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,13 @@
-- name: GetGroupMembers :many
SELECT * FROM group_members_expanded
WHERE CASE
WHEN @include_system::bool THEN TRUE
ELSE
user_is_system = false
END;
WHERE (@include_system::bool OR NOT user_is_system);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of evaluation of subexpressions is not defined. In particular, the inputs of an operator or function are not necessarily evaluated left-to-right or in any other fixed order.

https://www.postgresql.org/docs/17/sql-expressions.html#SYNTAX-EXPRESS-EVAL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was the one who suggested this change #18341 (comment)
I thought PostgreSQL would guarantee short-circuit evaluation for AND and OR operators, but you're absolutely right, the documentation explicitly states that evaluation order is not defined. TIL. Thanks for the correction!

Sorry about that @SasSwart 🙇‍♀️


-- name: GetGroupMembersByGroupID :many
SELECT *
FROM group_members_expanded
WHERE group_id = @group_id
-- Filter by system type
AND CASE
WHEN @include_system::bool THEN TRUE
ELSE
user_is_system = false
END;
AND (@include_system::bool OR NOT user_is_system);

-- name: GetGroupMembersCountByGroupID :one
-- Returns the total count of members in a group. Shows the total
Expand All @@ -25,11 +17,7 @@ SELECT COUNT(*)
FROM group_members_expanded
WHERE group_id = @group_id
-- Filter by system type
AND CASE
WHEN @include_system::bool THEN TRUE
ELSE
user_is_system = false
END;
AND (@include_system::bool OR NOT user_is_system);

-- InsertUserGroupsByName adds a user to all provided groups, if they exist.
-- name: InsertUserGroupsByName :exec
Expand Down
8 changes: 3 additions & 5 deletions coderd/database/queries/organizationmembers.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@ WHERE
ELSE true
END
-- Filter by system type
AND CASE
WHEN @include_system::bool THEN TRUE
ELSE
is_system = false
END;
AND (@include_system::bool OR NOT is_system);

-- name: InsertOrganizationMember :one
INSERT INTO
Expand Down Expand Up @@ -89,6 +85,8 @@ WHERE
organization_id = @organization_id
ELSE true
END
-- Filter by system type
AND (@include_system::bool OR NOT is_system)
Comment on lines +88 to +89
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding this filter here?

ORDER BY
-- Deterministic and consistent ordering of all users. This is to ensure consistent pagination.
LOWER(username) ASC OFFSET @offset_opt
Expand Down
12 changes: 4 additions & 8 deletions coderd/database/queries/users.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ FROM
users
WHERE
deleted = false
AND CASE WHEN @include_system::bool THEN TRUE ELSE is_system = false END;
AND (@include_system::bool OR NOT is_system);

-- name: GetActiveUserCount :one
SELECT
Expand All @@ -58,7 +58,7 @@ FROM
users
WHERE
status = 'active'::user_status AND deleted = false
AND CASE WHEN @include_system::bool THEN TRUE ELSE is_system = false END;
AND (@include_system::bool OR NOT is_system);

-- name: InsertUser :one
INSERT INTO
Expand Down Expand Up @@ -250,11 +250,7 @@ WHERE
created_at >= @created_after
ELSE true
END
AND CASE
WHEN @include_system::bool THEN TRUE
ELSE
is_system = false
END
AND (@include_system::bool OR NOT is_system)
AND CASE
WHEN @github_com_user_id :: bigint != 0 THEN
github_com_user_id = @github_com_user_id
Expand Down Expand Up @@ -364,7 +360,7 @@ RETURNING id, email, username, last_seen_at;
-- AllUserIDs returns all UserIDs regardless of user status or deletion.
-- name: AllUserIDs :many
SELECT DISTINCT id FROM USERS
WHERE CASE WHEN @include_system::bool THEN TRUE ELSE is_system = false END;
WHERE (@include_system::bool OR NOT is_system);

-- name: UpdateUserHashedOneTimePasscode :exec
UPDATE
Expand Down
3 changes: 2 additions & 1 deletion coderd/members.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func (api *API) listMembers(rw http.ResponseWriter, r *http.Request) {
members, err := api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{
OrganizationID: organization.ID,
UserID: uuid.Nil,
IncludeSystem: false,
IncludeSystem: true,
})
if httpapi.Is404Error(err) {
httpapi.ResourceNotFound(rw)
Expand Down Expand Up @@ -203,6 +203,7 @@ func (api *API) paginatedMembers(rw http.ResponseWriter, r *http.Request) {

paginatedMemberRows, err := api.Database.PaginatedOrganizationMembers(ctx, database.PaginatedOrganizationMembersParams{
OrganizationID: organization.ID,
IncludeSystem: true,
// #nosec G115 - Pagination limits are small and fit in int32
LimitOpt: int32(paginationParams.Limit),
// #nosec G115 - Pagination offsets are small and fit in int32
Expand Down
5 changes: 3 additions & 2 deletions coderd/members_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/stretchr/testify/require"

"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/db2sdk"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/codersdk"
Expand Down Expand Up @@ -62,9 +63,9 @@ func TestListMembers(t *testing.T) {
ctx := testutil.Context(t, testutil.WaitShort)
members, err := client.OrganizationMembers(ctx, first.OrganizationID)
require.NoError(t, err)
require.Len(t, members, 2)
require.Len(t, members, 3)
require.ElementsMatch(t,
[]uuid.UUID{first.UserID, user.ID},
[]uuid.UUID{first.UserID, user.ID, database.PrebuildsSystemUserID},
db2sdk.List(members, onlyIDs))
})
}
Expand Down
8 changes: 7 additions & 1 deletion docs/admin/templates/extending-templates/prebuilt-workspaces.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,12 +235,18 @@ The system always maintains the desired number of prebuilt workspaces for the ac

### Managing resource quotas

Prebuilt workspaces can be used in conjunction with [resource quotas](../../users/quotas.md).
To help prevent unexpected infrastructure costs, prebuilt workspaces can be used in conjunction with [resource quotas](../../users/quotas.md).
Because unclaimed prebuilt workspaces are owned by the `prebuilds` user, you can:

1. Configure quotas for any group that includes this user.
1. Set appropriate limits to balance prebuilt workspace availability with resource constraints.

When prebuilt workspaces are configured for an organization, Coder creates a "prebuilds" group in that organization and adds the prebuilds user to it. This group has a default quota allowance of 0, which you should adjust based on your needs:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the PR description to match?


- **Set a quota allowance** on the "prebuilds" group to control how many prebuilt workspaces can be provisioned
- **Monitor usage** to ensure the quota is appropriate for your desired number of prebuilt instances
- **Adjust as needed** based on your template costs and desired prebuilt workspace pool size

If a quota is exceeded, the prebuilt workspace will fail provisioning the same way other workspaces do.

### Template configuration best practices
Expand Down
18 changes: 9 additions & 9 deletions enterprise/coderd/groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) {

currentMembers, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
GroupID: group.ID,
IncludeSystem: false,
IncludeSystem: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need all these IncludeSystem: true cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, because I maintain that the prebuilds user should be visible. If we decide that we should hide the prebuilds user, then we can revert this back to false.

})
if err != nil {
httpapi.InternalServerError(rw, err)
Expand All @@ -180,7 +180,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) {
_, err := database.ExpectOne(api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{
OrganizationID: group.OrganizationID,
UserID: uuid.MustParse(id),
IncludeSystem: false,
IncludeSystem: true,
}))
if errors.Is(err, sql.ErrNoRows) {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Expand Down Expand Up @@ -296,7 +296,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) {

patchedMembers, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
GroupID: group.ID,
IncludeSystem: false,
IncludeSystem: true,
})
if err != nil {
httpapi.InternalServerError(rw, err)
Expand All @@ -307,7 +307,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) {

memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, database.GetGroupMembersCountByGroupIDParams{
GroupID: group.ID,
IncludeSystem: false,
IncludeSystem: true,
})
if err != nil {
httpapi.InternalServerError(rw, err)
Expand Down Expand Up @@ -353,7 +353,7 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) {

groupMembers, getMembersErr := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
GroupID: group.ID,
IncludeSystem: false,
IncludeSystem: true,
})
if getMembersErr != nil {
httpapi.InternalServerError(rw, getMembersErr)
Expand Down Expand Up @@ -407,7 +407,7 @@ func (api *API) group(rw http.ResponseWriter, r *http.Request) {

users, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
GroupID: group.ID,
IncludeSystem: false,
IncludeSystem: true,
})
if err != nil && !errors.Is(err, sql.ErrNoRows) {
httpapi.InternalServerError(rw, err)
Expand All @@ -416,7 +416,7 @@ func (api *API) group(rw http.ResponseWriter, r *http.Request) {

memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, database.GetGroupMembersCountByGroupIDParams{
GroupID: group.ID,
IncludeSystem: false,
IncludeSystem: true,
})
if err != nil {
httpapi.InternalServerError(rw, err)
Expand Down Expand Up @@ -512,15 +512,15 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) {
for _, group := range groups {
members, err := api.Database.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
GroupID: group.Group.ID,
IncludeSystem: false,
IncludeSystem: true,
})
if err != nil {
httpapi.InternalServerError(rw, err)
return
}
memberCount, err := api.Database.GetGroupMembersCountByGroupID(ctx, database.GetGroupMembersCountByGroupIDParams{
GroupID: group.Group.ID,
IncludeSystem: false,
IncludeSystem: true,
})
if err != nil {
httpapi.InternalServerError(rw, err)
Expand Down
4 changes: 2 additions & 2 deletions enterprise/coderd/groups_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,12 +862,12 @@ func TestGroup(t *testing.T) {
// The 'Everyone' group always has an ID that matches the organization ID.
group, err := userAdminClient.Group(ctx, user.OrganizationID)
require.NoError(t, err)
require.Len(t, group.Members, 4)
require.Len(t, group.Members, 5)
require.Equal(t, "Everyone", group.Name)
require.Equal(t, user.OrganizationID, group.OrganizationID)
require.Contains(t, group.Members, user1.ReducedUser)
require.Contains(t, group.Members, user2.ReducedUser)
require.NotContains(t, group.Members, prebuildsUser.ReducedUser)
require.Contains(t, group.Members, prebuildsUser.ReducedUser)
Copy link
Contributor

@ssncferreira ssncferreira Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include system users in the Everyone group? IIUC with this change, prebuilds users will inherit the quota from this group, is that something that we want? Shouldn't it be limited to the prebuilds group? As previously discussed, I'm not entirely convinced we should treat system users as normal users in these workflows 👀 but if there is a general consensus, I'm ok with it

})
}

Expand Down
Loading
Loading