coder · sreya · Aug 17, 2023 · Aug 15, 2023 · Aug 16, 2023 · Aug 16, 2023
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -916,11 +916,11 @@ func (q *querier) GetGroupByOrgAndName(ctx context.Context, arg database.GetGrou
 	return fetch(q.log, q.auth, q.db.GetGroupByOrgAndName)(ctx, arg)
 }
 
-func (q *querier) GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]database.User, error) {
-	if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check
+func (q *querier) GetGroupMembers(ctx context.Context, id uuid.UUID) ([]database.User, error) {
+	if _, err := q.GetGroupByID(ctx, id); err != nil { // AuthZ check
 		return nil, err
 	}
-	return q.db.GetGroupMembers(ctx, groupID)
+	return q.db.GetGroupMembers(ctx, id)
 }
 
 func (q *querier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) {

diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go
@@ -611,6 +611,44 @@ func uniqueSortedUUIDs(uuids []uuid.UUID) []uuid.UUID {
 	return unique
 }
 
+func (q *FakeQuerier) getOrganizationMember(orgID uuid.UUID) []database.OrganizationMember {
+	var members []database.OrganizationMember
+	for _, member := range q.organizationMembers {
+		if member.OrganizationID == orgID {
+			members = append(members, member)
+		}
+	}
+
+	return members
+}
+
+// getEveryoneGroupMembers fetches all the users in an organization.
+func (q *FakeQuerier) getEveryoneGroupMembers(orgID uuid.UUID) []database.User {
+	var (
+		everyone   []database.User
+		orgMembers = q.getOrganizationMember(orgID)
+	)
+	for _, member := range orgMembers {
+		user, err := q.GetUserByID(context.TODO(), member.UserID)
+		if err != nil {
+			return nil
+		}
+		everyone = append(everyone, user)
+	}
+	return everyone
+}
+
+// isEveryoneGroup returns true if the provided ID matches
+// an organization ID.
+func (q *FakeQuerier) isEveryoneGroup(id uuid.UUID) bool {
+	for _, org := range q.organizations {
+		if org.ID == id {
+			return true
+		}
+	}
+	return false
+}
+
 func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error {
 	return xerrors.New("AcquireLock must only be called within a transaction")
 }
@@ -1376,13 +1414,17 @@ func (q *FakeQuerier) GetGroupByOrgAndName(_ context.Context, arg database.GetGr
 	return database.Group{}, sql.ErrNoRows
 }
 
-func (q *FakeQuerier) GetGroupMembers(_ context.Context, groupID uuid.UUID) ([]database.User, error) {
+func (q *FakeQuerier) GetGroupMembers(_ context.Context, id uuid.UUID) ([]database.User, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
+	if q.isEveryoneGroup(id) {
+		return q.getEveryoneGroupMembers(id), nil
+	}
+
 	var members []database.GroupMember
 	for _, member := range q.groupMembers {
-		if member.GroupID == groupID {
+		if member.GroupID == id {
 			members = append(members, member)
 		}
 	}
@@ -1401,14 +1443,13 @@ func (q *FakeQuerier) GetGroupMembers(_ context.Context, groupID uuid.UUID) ([]d
 	return users, nil
 }
 
-func (q *FakeQuerier) GetGroupsByOrganizationID(_ context.Context, organizationID uuid.UUID) ([]database.Group, error) {
+func (q *FakeQuerier) GetGroupsByOrganizationID(_ context.Context, id uuid.UUID) ([]database.Group, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
-	var groups []database.Group
+	groups := make([]database.Group, 0, len(q.groups))
 	for _, group := range q.groups {
-		// Omit the allUsers group.
-		if group.OrganizationID == organizationID && group.ID != organizationID {
+		if group.OrganizationID == id {
 			groups = append(groups, group)
 		}
 	}
@@ -1838,9 +1879,17 @@ func (q *FakeQuerier) GetQuotaAllowanceForUser(_ context.Context, userID uuid.UU
 		for _, group := range q.groups {
 			if group.ID == member.GroupID {
 				sum += int64(group.QuotaAllowance)
+				continue
 			}
 		}
 	}
+	// Grab the quota for the Everyone group.
+	for _, group := range q.groups {
+		if group.ID == group.OrganizationID {
+			sum += int64(group.QuotaAllowance)
+			break
+		}
+	}
 	return sum, nil
 }
 
@@ -3546,7 +3595,7 @@ func (q *FakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP
 func (q *FakeQuerier) InsertAllUsersGroup(ctx context.Context, orgID uuid.UUID) (database.Group, error) {
 	return q.InsertGroup(ctx, database.InsertGroupParams{
 		ID:             orgID,
-		Name:           database.AllUsersGroup,
+		Name:           database.EveryoneGroup,
 		DisplayName:    "",
 		OrganizationID: orgID,
 	})

diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go
@@ -84,7 +84,7 @@ func (g Group) Auditable(users []User) AuditableGroup {
 	}
 }
 
-const AllUsersGroup = "Everyone"
+const EveryoneGroup = "Everyone"
 
 func (s APIKeyScope) ToRBAC() rbac.ScopeName {
 	switch s {
@@ -362,3 +362,7 @@ func ConvertWorkspaceRows(rows []GetWorkspacesRow) []Workspace {
 
 	return workspaces
 }
+
+func (g Group) IsEveryone() bool {
+	return g.ID == g.OrganizationID
+}
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/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql
@@ -3,12 +3,23 @@ SELECT
 	users.*
 FROM
 	users
-JOIN
+-- If the group is a user made group, then we need to check the group_members table.
+LEFT JOIN
 	group_members
 ON
-	users.id = group_members.user_id
+	group_members.user_id = users.id AND
+	group_members.group_id = @group_id
+-- If it is the "Everyone" group, then we need to check the organization_members table.
+LEFT JOIN
+	organization_members
+ON
+	organization_members.user_id = users.id AND
+	organization_members.organization_id = @group_id
 WHERE
-	group_members.group_id = $1
+	-- In either case, the group_id will only match an org or a group.
+    (group_members.group_id = @group_id
+         OR
+     organization_members.organization_id = @group_id)
 AND
 	users.status = 'active'
 AND

diff --git a/coderd/database/queries/groups.sql b/coderd/database/queries/groups.sql
@@ -26,9 +26,7 @@ SELECT
 FROM
 	groups
 WHERE
-	organization_id = $1
-AND
-	id != $1;
+	organization_id = $1;
 
 -- name: InsertGroup :one
 INSERT INTO groups (

diff --git a/coderd/database/queries/quotas.sql b/coderd/database/queries/quotas.sql
@@ -2,11 +2,13 @@
 SELECT
 	coalesce(SUM(quota_allowance), 0)::BIGINT
 FROM
-	group_members gm
-JOIN groups g ON
+	groups g
+LEFT JOIN group_members gm ON
 	g.id = gm.group_id
 WHERE
-	user_id = $1;
+	user_id = $1
+OR
+    g.id = g.organization_id;
 
 -- name: GetQuotaConsumedForUser :one
 WITH latest_builds AS (

diff --git a/coderd/database/tx.go b/coderd/database/tx.go
@@ -0,0 +1,49 @@
+package database
+
+import (
+	"database/sql"
+
+	"github.com/lib/pq"
+	"golang.org/x/xerrors"
+)
+
+const maxRetries = 5
+
+// ReadModifyUpdate is a helper function to run a db transaction that reads some
+// object(s), modifies some of the data, and writes the modified object(s) back
+// to the database.  It is run in a transaction at RepeatableRead isolation so
+// that if another database client also modifies the data we are writing and
+// commits, then the transaction is rolled back and restarted.
+//
+// This is needed because we typically read all object columns, modify some
+// subset, and then write all columns.  Consider an object with columns A, B and
+// initial values A=1, B=1.  Two database clients work simultaneously, with one
+// client attempting to set A=2, and another attempting to set B=2.  They both
+// initially read A=1, B=1, and then one writes A=2, B=1, and the other writes
+// A=1, B=2.  With default PostgreSQL isolation of ReadCommitted, both of these
+// transactions would succeed and we end up with either A=2, B=1 or A=1, B=2.
+// One or other client gets their transaction wiped out even though the data
+// they wanted to change didn't conflict.
+//
+// If we run at RepeatableRead isolation, then one or other transaction will
+// fail.  Let's say the transaction that sets A=2 succeeds.  Then the first B=2
+// transaction fails, but here we retry.  The second attempt we read A=2, B=1,
+// then write A=2, B=2 as desired, and this succeeds.
+func ReadModifyUpdate(db Store, f func(tx Store) error,
+) error {
+	var err error
+	for retries := 0; retries < maxRetries; retries++ {
+		err = db.InTx(f, &sql.TxOptions{
+			Isolation: sql.LevelRepeatableRead,
+		})
+		var pqe *pq.Error
+		if xerrors.As(err, &pqe) {
+			if pqe.Code == "40001" {
+				// serialization error, retry
+				continue
+			}
+		}
+		return err
+	}
+	return xerrors.Errorf("too many errors; last error: %w", err)
+}