chore: fixup quotas to only include groups you are a member of (#14271)

* chore: fixup quotas to only include groups you are a member of

Before all everyone groups were included in the allowance.

* chore: add unit test to execercise the bug
* add unit test to add rows into the everyone group

coderd/database/dbmem/dbmem.go

@@ -3318,6 +3318,12 @@ func (q *FakeQuerier) GetQuotaAllowanceForUser(_ context.Context, userID uuid.UU
 		if member.UserID != userID {
 			continue
 		}
+		if _, err := q.getOrganizationByIDNoLock(member.GroupID); err == nil {
+			// This should never happen, but it has been reported in customer deployments.
+			// The SQL handles this case, and omits `group_members` rows in the
+			// Everyone group. It counts these distinctly via `organization_members` table.
+			continue
+		}
 		for _, group := range q.groups {
 			if group.ID == member.GroupID {
 				sum += int64(group.QuotaAllowance)
@@ -3325,13 +3331,21 @@ func (q *FakeQuerier) GetQuotaAllowanceForUser(_ context.Context, userID uuid.UU
 			}
 		}
 	}
-	// Grab the quota for the Everyone group.
-	for _, group := range q.groups {
-		if group.ID == group.OrganizationID {
+
+	// Grab the quota for the Everyone group iff the user is a member of
+	// said organization.
+	for _, mem := range q.organizationMembers {
+		if mem.UserID != userID {
+			continue
+		}
+
+		group, err := q.getGroupByIDNoLock(context.Background(), mem.OrganizationID)
+		if err != nil {
+			return -1, xerrors.Errorf("failed to get everyone group for org %q", mem.OrganizationID.String())
+		}
 		sum += int64(group.QuotaAllowance)
-			break
-		}
 	}
+
 	return sum, nil
 }
 

coderd/database/querier_test.go

@@ -545,6 +545,84 @@ func TestAuditLogDefaultLimit(t *testing.T) {
 	require.Len(t, rows, 100)
 }
 
+func TestWorkspaceQuotas(t *testing.T) {
+	t.Parallel()
+	orgMemberIDs := func(o database.OrganizationMember) uuid.UUID {
+		return o.UserID
+	}
+	groupMemberIDs := func(m database.GroupMember) uuid.UUID {
+		return m.UserID
+	}
+
+	t.Run("CorruptedEveryone", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := testutil.Context(t, testutil.WaitLong)
+
+		db, _ := dbtestutil.NewDB(t)
+		// Create an extra org as a distraction
+		distract := dbgen.Organization(t, db, database.Organization{})
+		_, err := db.InsertAllUsersGroup(ctx, distract.ID)
+		require.NoError(t, err)
+
+		_, err = db.UpdateGroupByID(ctx, database.UpdateGroupByIDParams{
+			QuotaAllowance: 15,
+			ID:             distract.ID,
+		})
+		require.NoError(t, err)
+
+		// Create an org with 2 users
+		org := dbgen.Organization(t, db, database.Organization{})
+
+		everyoneGroup, err := db.InsertAllUsersGroup(ctx, org.ID)
+		require.NoError(t, err)
+
+		// Add a quota to the everyone group
+		_, err = db.UpdateGroupByID(ctx, database.UpdateGroupByIDParams{
+			QuotaAllowance: 50,
+			ID:             everyoneGroup.ID,
+		})
+		require.NoError(t, err)
+
+		// Add people to the org
+		one := dbgen.User(t, db, database.User{})
+		two := dbgen.User(t, db, database.User{})
+		memOne := dbgen.OrganizationMember(t, db, database.OrganizationMember{
+			OrganizationID: org.ID,
+			UserID:         one.ID,
+		})
+		memTwo := dbgen.OrganizationMember(t, db, database.OrganizationMember{
+			OrganizationID: org.ID,
+			UserID:         two.ID,
+		})
+
+		// Fetch the 'Everyone' group members
+		everyoneMembers, err := db.GetGroupMembersByGroupID(ctx, org.ID)
+		require.NoError(t, err)
+
+		require.ElementsMatch(t, db2sdk.List(everyoneMembers, groupMemberIDs),
+			db2sdk.List([]database.OrganizationMember{memOne, memTwo}, orgMemberIDs))
+
+		// Check the quota is correct.
+		allowance, err := db.GetQuotaAllowanceForUser(ctx, one.ID)
+		require.NoError(t, err)
+		require.Equal(t, int64(50), allowance)
+
+		// Now try to corrupt the DB
+		// Insert rows into the everyone group
+		err = db.InsertGroupMember(ctx, database.InsertGroupMemberParams{
+			UserID:  memOne.UserID,
+			GroupID: org.ID,
+		})
+		require.NoError(t, err)
+
+		// Ensure allowance remains the same
+		allowance, err = db.GetQuotaAllowanceForUser(ctx, one.ID)
+		require.NoError(t, err)
+		require.Equal(t, int64(50), allowance)
+	})
+}
+
 // TestReadCustomRoles tests the input params returns the correct set of roles.
 func TestReadCustomRoles(t *testing.T) {
 	t.Parallel()

coderd/database/queries.sql.go

@@ -6223,15 +6223,15 @@ func (q *sqlQuerier) UpdateWorkspaceProxyDeleted(ctx context.Context, arg Update
 
 const getQuotaAllowanceForUser = `-- name: GetQuotaAllowanceForUser :one
 SELECT
-	coalesce(SUM(quota_allowance), 0)::BIGINT
+	coalesce(SUM(groups.quota_allowance), 0)::BIGINT
 FROM
-	groups g
-LEFT JOIN group_members gm ON
-	g.id = gm.group_id
-WHERE
-	user_id = $1
-OR
-    g.id = g.organization_id
+	(
+		-- Select all groups this user is a member of. This will also include
+		-- the "Everyone" group for organizations the user is a member of.
+		SELECT user_id, user_email, user_username, user_hashed_password, user_created_at, user_updated_at, user_status, user_rbac_roles, user_login_type, user_avatar_url, user_deleted, user_last_seen_at, user_quiet_hours_schedule, user_theme_preference, user_name, user_github_com_user_id, organization_id, group_name, group_id FROM group_members_expanded WHERE $1 = user_id
+	) AS members
+INNER JOIN groups ON
+	members.group_id = groups.id
 `
 
 func (q *sqlQuerier) GetQuotaAllowanceForUser(ctx context.Context, userID uuid.UUID) (int64, error) {

coderd/database/queries/quotas.sql

@@ -1,14 +1,15 @@
 -- name: GetQuotaAllowanceForUser :one
 SELECT
-	coalesce(SUM(quota_allowance), 0)::BIGINT
+	coalesce(SUM(groups.quota_allowance), 0)::BIGINT
 FROM
-	groups g
-LEFT JOIN group_members gm ON
-	g.id = gm.group_id
-WHERE
-	user_id = $1
-OR
-    g.id = g.organization_id;
+	(
+		-- Select all groups this user is a member of. This will also include
+		-- the "Everyone" group for organizations the user is a member of.
+		SELECT * FROM group_members_expanded WHERE @user_id = user_id
+	) AS members
+INNER JOIN groups ON
+	members.group_id = groups.id
+;
 
 -- name: GetQuotaConsumedForUser :one
 WITH latest_builds AS (

enterprise/coderd/workspacequota_test.go

@@ -233,6 +233,51 @@ func TestWorkspaceQuota(t *testing.T) {
 		verifyQuota(ctx, t, client, 4, 4)
 		require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status)
 	})
+
+	// Ensures allowance from everyone groups only counts if you are an org member.
+	// This was a bug where the group "Everyone" was being counted for all users,
+	// regardless of membership.
+	t.Run("AllowanceEveryone", func(t *testing.T) {
+		t.Parallel()
+
+		dv := coderdtest.DeploymentValues(t)
+		dv.Experiments = []string{string(codersdk.ExperimentMultiOrganization)}
+		owner, first := coderdenttest.New(t, &coderdenttest.Options{
+			Options: &coderdtest.Options{
+				DeploymentValues: dv,
+			},
+			LicenseOptions: &coderdenttest.LicenseOptions{
+				Features: license.Features{
+					codersdk.FeatureTemplateRBAC:          1,
+					codersdk.FeatureMultipleOrganizations: 1,
+				},
+			},
+		})
+		member, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID)
+
+		// Create a second organization
+		second := coderdenttest.CreateOrganization(t, owner, coderdenttest.CreateOrganizationOptions{})
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		// update everyone quotas
+		//nolint:gocritic // using owner for simplicity
+		_, err := owner.PatchGroup(ctx, first.OrganizationID, codersdk.PatchGroupRequest{
+			QuotaAllowance: ptr.Ref(30),
+		})
+		require.NoError(t, err)
+
+		_, err = owner.PatchGroup(ctx, second.ID, codersdk.PatchGroupRequest{
+			QuotaAllowance: ptr.Ref(15),
+		})
+		require.NoError(t, err)
+
+		verifyQuota(ctx, t, member, 0, 30)
+		// This currently reports the total site wide quotas. We might want to
+		// org scope this api call in the future.
+		verifyQuota(ctx, t, owner, 0, 45)
+	})
 }
 
 func planWithCost(cost int32) []*proto.Response {