simplify RBAC check on group member count

coder · Emyrk · Aug 13, 2024 · Aug 6, 2024 · Aug 6, 2024 · Aug 6, 2024
commit e5c5f3f8a9e03591bb45cd48a10e7d58ae95d1e1
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -1400,22 +1400,8 @@ func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([
 	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id)
 }
 
-func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) {
-	group, err := q.GetGroupByID(ctx, groupID)
-	if err != nil {
-		return 0, err
-	}
-	memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID)
-	if err != nil {
-		return 0, err
-	}
-	if err := q.authorizeContext(ctx, policy.ActionRead, database.GroupMembersCountRBACHelper{
-		GroupID:        groupID,
-		OrganizationID: group.OrganizationID,
-	}); err != nil {
-		return 0, err
-	}
-	return memberCount, nil
+func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) {
+	return fetch(q.log, q.auth, q.db.GetGroupMembersCountByGroupID)(ctx, groupID)
 }
 
 func (q *querier) GetGroups(ctx context.Context) ([]database.Group, error) {

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -2536,12 +2536,20 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(_ context.Context, id uuid.UUID)
 	return members, 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, groupID uuid.UUID) (database.GetGroupMembersCountByGroupIDRow, error) {
+	members, err := q.GetGroupMembersByGroupID(ctx, groupID)
 	if err != nil {
-		return 0, err
+		return database.GetGroupMembersCountByGroupIDRow{}, err
+	}
+	group, err := q.GetGroupByID(ctx, groupID)
+	if err != nil {
+		return database.GetGroupMembersCountByGroupIDRow{}, err
 	}
-	return int64(len(users)), nil
+	return database.GetGroupMembersCountByGroupIDRow{
+		OrganizationID: group.OrganizationID,
+		GroupID:        groupID,
+		MemberCount:    int64(len(members)),
+	}, nil
 }
 
 func (q *FakeQuerier) GetGroups(_ context.Context) ([]database.Group, error) {

diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go
diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go
diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go
@@ -172,34 +172,27 @@ func (v TemplateVersion) RBACObjectNoTemplate() rbac.Object {
 	return rbac.ResourceTemplate.InOrg(v.OrganizationID)
 }
 
-func (g Group) RBACObject() rbac.Object {
-	return rbac.ResourceGroup.WithID(g.ID).
-		InOrg(g.OrganizationID).
+func groupRBACObject(groupID, organizationID uuid.UUID) rbac.Object {
+	return rbac.ResourceGroup.WithID(groupID).
+		InOrg(organizationID).
 		// Group members can read the group.
 		WithGroupACL(map[string][]policy.Action{
-			g.ID.String(): {
+			groupID.String(): {
 				policy.ActionRead,
 			},
 		})
 }
 
-func (gm GroupMember) RBACObject() rbac.Object {
-	return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String())
+func (g Group) RBACObject() rbac.Object {
+	return groupRBACObject(g.ID, g.OrganizationID)
 }
 
-type GroupMembersCountRBACHelper struct {
-	GroupID        uuid.UUID
-	OrganizationID uuid.UUID
+func (gm GroupMember) RBACObject() rbac.Object {
+	return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String())
 }
 
-func (r GroupMembersCountRBACHelper) RBACObject() rbac.Object {
-	return rbac.ResourceGroup.WithID(r.GroupID).InOrg(r.OrganizationID).
-		// Group members can read the member count.
-		WithGroupACL(map[string][]policy.Action{
-			r.GroupID.String(): {
-				policy.ActionRead,
-			},
-		})
+func (r GetGroupMembersCountByGroupIDRow) RBACObject() rbac.Object {
+	return groupRBACObject(r.GroupID, r.OrganizationID)
 }
 
 func (w GetWorkspaceByAgentIDRow) RBACObject() rbac.Object {

diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql
@@ -5,7 +5,17 @@ SELECT * FROM group_members_expanded;
 SELECT * FROM group_members_expanded WHERE group_id = @group_id;
 
 -- name: GetGroupMembersCountByGroupID :one
-SELECT COUNT(*) FROM group_members_expanded WHERE group_id = @group_id;
+SELECT 
+    gme.organization_id,
+    gme.group_id,
+    COUNT(*) as member_count
+FROM 
+    group_members_expanded gme
+WHERE 
+    gme.group_id = @group_id
+-- This aggregation is guaranteed to return a single row
+GROUP BY 
+    gme.organization_id, gme.group_id;
 
 -- InsertUserGroupsByName adds a user to all provided groups, if they exist.
 -- name: InsertUserGroupsByName :exec

diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go
@@ -453,7 +453,7 @@ func (api *API) reducedGroupsByUserAndOrganization(rw http.ResponseWriter, r *ht
 			return
 		}
 
-		resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount)))
+		resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount.MemberCount)))
 	}
 
 	httpapi.Write(ctx, rw, http.StatusOK, resp)
-Original file line number
+Diff line change
@@ Expand Up @@
     			return
     		}
-    		resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount)))
+    		resp = append(resp, db2sdk.ReducedGroup(group, int(memberCount.MemberCount)))
     	}
     	httpapi.Write(ctx, rw, http.StatusOK, resp)
@@ Expand Down @@