ensure that system users are filtered and returned consistently

coder · SasSwart · Mar 25, 2025 · Mar 12, 2025 · Mar 13, 2025 · Mar 13, 2025
commit de4fb8ac80e3dc8d0815ecc28ce3dda453daaa7b
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -1747,15 +1747,15 @@ func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember,
 	return q.db.GetGroupMembers(ctx)
 }
 
-func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) {
-	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id)
+func (q *querier) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) {
+	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, arg)
 }
 
-func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) {
-	if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check
+func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) {
+	if _, err := q.GetGroupByID(ctx, arg.GroupID); err != nil { // AuthZ check
 		return 0, err
 	}
-	memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID)
+	memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, arg)
 	if err != nil {
 		return 0, err
 	}

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -387,12 +387,18 @@ func (s *MethodTestSuite) TestGroup() {
 		g := dbgen.Group(s.T(), db, database.Group{})
 		u := dbgen.User(s.T(), db, database.User{})
 		gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
-		check.Args(g.ID).Asserts(gm, policy.ActionRead)
+		check.Args(database.GetGroupMembersByGroupIDParams{
+			GroupID:       g.ID,
+			IncludeSystem: false,
+		}).Asserts(gm, policy.ActionRead)
 	}))
 	s.Run("GetGroupMembersCountByGroupID", s.Subtest(func(db database.Store, check *expects) {
 		dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
 		g := dbgen.Group(s.T(), db, database.Group{})
-		check.Args(g.ID).Asserts(g, policy.ActionRead)
+		check.Args(database.GetGroupMembersCountByGroupIDParams{
+			GroupID:       g.ID,
+			IncludeSystem: false,
+		}).Asserts(g, policy.ActionRead)
 	}))
 	s.Run("GetGroupMembers", s.Subtest(func(db database.Store, check *expects) {
 		dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)

diff --git a/coderd/database/dbauthz/groupsauth_test.go b/coderd/database/dbauthz/groupsauth_test.go
@@ -147,7 +147,10 @@ func TestGroupsAuth(t *testing.T) {
 				require.Error(t, err, "group read")
 			}
 
-			members, err := db.GetGroupMembersByGroupID(actorCtx, group.ID)
+			members, err := db.GetGroupMembersByGroupID(actorCtx, database.GetGroupMembersByGroupIDParams{
+				GroupID:       group.ID,
+				IncludeSystem: false,
+			})
 			if tc.ReadMembers {
 				require.NoError(t, err, "member read")
 				require.Len(t, members, tc.MembersExpected, "member count found does not match")

diff --git a/coderd/database/dbgen/dbgen_test.go b/coderd/database/dbgen/dbgen_test.go
@@ -105,7 +105,10 @@ func TestGenerator(t *testing.T) {
 		gm := dbgen.GroupMember(t, db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
 		exp := []database.GroupMember{gm}
 
-		require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), g.ID)))
+		require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), database.GetGroupMembersByGroupIDParams{
+			GroupID:       g.ID,
+			IncludeSystem: false,
+		})))
 	})
 
 	t.Run("Organization", func(t *testing.T) {

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -3427,17 +3427,17 @@
 	return groupMembers, nil
 }
 
-func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) {
+func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
-	if q.isEveryoneGroup(id) {
-		return q.getEveryoneGroupMembersNoLock(ctx, id), nil
+	if q.isEveryoneGroup(arg.GroupID) {
+		return q.getEveryoneGroupMembersNoLock(ctx, arg.GroupID), nil
 	}
 
 	var groupMembers []database.GroupMember
 	for _, member := range q.groupMembers {
-		if member.GroupID == id {
+		if member.GroupID == arg.GroupID {
 			groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, member.GroupID)
 			if errors.Is(err, errUserDeleted) {
 				continue
@@ -3452,8 +3452,11 @@
 	return groupMembers, nil
 }
 
-func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) {
-	users, err := q.GetGroupMembersByGroupID(ctx, groupID)
+func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) {
+	users, err := q.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
+		GroupID:       arg.GroupID,
+		IncludeSystem: arg.IncludeSystem,
+	})
 	if err != nil {
 		return 0, err
 	}

diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go
diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go
diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000302_system_user.down.sql b/coderd/database/migrations/000302_system_user.down.sql
@@ -1,3 +1,39 @@
+DROP VIEW IF EXISTS group_members_expanded;
+CREATE VIEW group_members_expanded AS
+ WITH all_members AS (
+         SELECT group_members.user_id,
+            group_members.group_id
+           FROM group_members
+        UNION
+         SELECT organization_members.user_id,
+            organization_members.organization_id AS group_id
+           FROM organization_members
+        )
+ SELECT users.id AS user_id,
+    users.email AS user_email,
+    users.username AS user_username,
+    users.hashed_password AS user_hashed_password,
+    users.created_at AS user_created_at,
+    users.updated_at AS user_updated_at,
+    users.status AS user_status,
+    users.rbac_roles AS user_rbac_roles,
+    users.login_type AS user_login_type,
+    users.avatar_url AS user_avatar_url,
+    users.deleted AS user_deleted,
+    users.last_seen_at AS user_last_seen_at,
+    users.quiet_hours_schedule AS user_quiet_hours_schedule,
+    users.name AS user_name,
+    users.github_com_user_id AS user_github_com_user_id,
+    groups.organization_id,
+    groups.name AS group_name,
+    all_members.group_id
+   FROM ((all_members
+     JOIN users ON ((users.id = all_members.user_id)))
+     JOIN groups ON ((groups.id = all_members.group_id)))
+  WHERE (users.deleted = false);
+
+COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).';
+
 -- Remove system user from organizations
 DELETE FROM organization_members
 WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0';

diff --git a/coderd/database/migrations/000302_system_user.up.sql b/coderd/database/migrations/000302_system_user.up.sql
@@ -5,11 +5,9 @@ CREATE INDEX user_is_system_idx ON users USING btree (is_system);
 
 COMMENT ON COLUMN users.is_system IS 'Determines if a user is a system user, and therefore cannot login or perform normal actions';
 
--- TODO: tried using "none" for login type, but the migration produced this error: 'unsafe use of new value "none" of enum type login_type'
---  	 -> not sure why though? it exists on the login_type enum.
 INSERT INTO users (id, email, username, name, created_at, updated_at, status, rbac_roles, hashed_password, is_system, login_type)
 VALUES ('c42fdf75-3097-471c-8c33-fb52454d81c0', 'prebuilds@system', 'prebuilds', 'Prebuilds Owner', now(), now(),
-		'active', '{}', 'none', true, 'password'::login_type);
+		'active', '{}', 'none', true, 'none'::login_type);
 
 -- Create function to check system user modifications
 CREATE OR REPLACE FUNCTION prevent_system_user_changes()
@@ -37,6 +35,42 @@ CREATE TRIGGER prevent_system_user_deletions
 	WHEN (OLD.is_system = true)
 EXECUTE FUNCTION prevent_system_user_changes();
 
+DROP VIEW IF EXISTS group_members_expanded;
+CREATE VIEW group_members_expanded AS
+ WITH all_members AS (
+         SELECT group_members.user_id,
+            group_members.group_id
+           FROM group_members
+        UNION
+         SELECT organization_members.user_id,
+            organization_members.organization_id AS group_id
+           FROM organization_members
+        )
+ SELECT users.id AS user_id,
+    users.email AS user_email,
+    users.username AS user_username,
+    users.hashed_password AS user_hashed_password,
+    users.created_at AS user_created_at,
+    users.updated_at AS user_updated_at,
+    users.status AS user_status,
+    users.rbac_roles AS user_rbac_roles,
+    users.login_type AS user_login_type,
+    users.avatar_url AS user_avatar_url,
+    users.deleted AS user_deleted,
+    users.last_seen_at AS user_last_seen_at,
+    users.quiet_hours_schedule AS user_quiet_hours_schedule,
+    users.name AS user_name,
+    users.github_com_user_id AS user_github_com_user_id,
+    users.is_system AS user_is_system,
+    groups.organization_id,
+    groups.name AS group_name,
+    all_members.group_id
+   FROM ((all_members
+     JOIN users ON ((users.id = all_members.user_id)))
+     JOIN groups ON ((groups.id = all_members.group_id)))
+  WHERE (users.deleted = false);
+
+COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).';
 -- TODO: do we *want* to use the default org here? how do we handle multi-org?
 WITH default_org AS (SELECT id
 					 FROM organizations

diff --git a/coderd/database/models.go b/coderd/database/models.go
diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go
@@ -1505,7 +1505,10 @@ func TestWorkspaceQuotas(t *testing.T) {
 		})
 
 		// Fetch the 'Everyone' group members
-		everyoneMembers, err := db.GetGroupMembersByGroupID(ctx, org.ID)
+		everyoneMembers, err := db.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
+			GroupID:       everyoneGroup.ID,
+			IncludeSystem: false,
+		})
 		require.NoError(t, err)
 
 		require.ElementsMatch(t, db2sdk.List(everyoneMembers, groupMemberIDs),