Skip to content

Commit 83ccdaa

Browse files
EmyrkEmyrk
authored
chore: fixup quotas to only include groups you are a member of (coder#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
1 parent f619500 commit 83ccdaa

File tree

5 files changed

+159
-21
lines changed

5 files changed

+159
-21
lines changed

‎coderd/database/dbmem/dbmem.go

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3318,20 +3318,34 @@ func (q *FakeQuerier) GetQuotaAllowanceForUser(_ context.Context, userID uuid.UU
33183318
if member.UserID != userID {
33193319
continue
33203320
}
3321+
if _, err := q.getOrganizationByIDNoLock(member.GroupID); err == nil {
3322+
// This should never happen, but it has been reported in customer deployments.
3323+
// The SQL handles this case, and omits `group_members` rows in the
3324+
// Everyone group. It counts these distinctly via `organization_members` table.
3325+
continue
3326+
}
33213327
for _, group := range q.groups {
33223328
if group.ID == member.GroupID {
33233329
sum += int64(group.QuotaAllowance)
33243330
continue
33253331
}
33263332
}
33273333
}
3328-
// Grab the quota for the Everyone group.
3329-
for _, group := range q.groups {
3330-
if group.ID == group.OrganizationID {
3331-
sum += int64(group.QuotaAllowance)
3332-
break
3334+
3335+
// Grab the quota for the Everyone group iff the user is a member of
3336+
// said organization.
3337+
for _, mem := range q.organizationMembers {
3338+
if mem.UserID != userID {
3339+
continue
33333340
}
3341+
3342+
group, err := q.getGroupByIDNoLock(context.Background(), mem.OrganizationID)
3343+
if err != nil {
3344+
return -1, xerrors.Errorf("failed to get everyone group for org %q", mem.OrganizationID.String())
3345+
}
3346+
sum += int64(group.QuotaAllowance)
33343347
}
3348+
33353349
return sum, nil
33363350
}
33373351

‎coderd/database/querier_test.go

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,84 @@ func TestAuditLogDefaultLimit(t *testing.T) {
545545
require.Len(t, rows, 100)
546546
}
547547

548+
func TestWorkspaceQuotas(t *testing.T) {
549+
t.Parallel()
550+
orgMemberIDs := func(o database.OrganizationMember) uuid.UUID {
551+
return o.UserID
552+
}
553+
groupMemberIDs := func(m database.GroupMember) uuid.UUID {
554+
return m.UserID
555+
}
556+
557+
t.Run("CorruptedEveryone", func(t *testing.T) {
558+
t.Parallel()
559+
560+
ctx := testutil.Context(t, testutil.WaitLong)
561+
562+
db, _ := dbtestutil.NewDB(t)
563+
// Create an extra org as a distraction
564+
distract := dbgen.Organization(t, db, database.Organization{})
565+
_, err := db.InsertAllUsersGroup(ctx, distract.ID)
566+
require.NoError(t, err)
567+
568+
_, err = db.UpdateGroupByID(ctx, database.UpdateGroupByIDParams{
569+
QuotaAllowance: 15,
570+
ID: distract.ID,
571+
})
572+
require.NoError(t, err)
573+
574+
// Create an org with 2 users
575+
org := dbgen.Organization(t, db, database.Organization{})
576+
577+
everyoneGroup, err := db.InsertAllUsersGroup(ctx, org.ID)
578+
require.NoError(t, err)
579+
580+
// Add a quota to the everyone group
581+
_, err = db.UpdateGroupByID(ctx, database.UpdateGroupByIDParams{
582+
QuotaAllowance: 50,
583+
ID: everyoneGroup.ID,
584+
})
585+
require.NoError(t, err)
586+
587+
// Add people to the org
588+
one := dbgen.User(t, db, database.User{})
589+
two := dbgen.User(t, db, database.User{})
590+
memOne := dbgen.OrganizationMember(t, db, database.OrganizationMember{
591+
OrganizationID: org.ID,
592+
UserID: one.ID,
593+
})
594+
memTwo := dbgen.OrganizationMember(t, db, database.OrganizationMember{
595+
OrganizationID: org.ID,
596+
UserID: two.ID,
597+
})
598+
599+
// Fetch the 'Everyone' group members
600+
everyoneMembers, err := db.GetGroupMembersByGroupID(ctx, org.ID)
601+
require.NoError(t, err)
602+
603+
require.ElementsMatch(t, db2sdk.List(everyoneMembers, groupMemberIDs),
604+
db2sdk.List([]database.OrganizationMember{memOne, memTwo}, orgMemberIDs))
605+
606+
// Check the quota is correct.
607+
allowance, err := db.GetQuotaAllowanceForUser(ctx, one.ID)
608+
require.NoError(t, err)
609+
require.Equal(t, int64(50), allowance)
610+
611+
// Now try to corrupt the DB
612+
// Insert rows into the everyone group
613+
err = db.InsertGroupMember(ctx, database.InsertGroupMemberParams{
614+
UserID: memOne.UserID,
615+
GroupID: org.ID,
616+
})
617+
require.NoError(t, err)
618+
619+
// Ensure allowance remains the same
620+
allowance, err = db.GetQuotaAllowanceForUser(ctx, one.ID)
621+
require.NoError(t, err)
622+
require.Equal(t, int64(50), allowance)
623+
})
624+
}
625+
548626
// TestReadCustomRoles tests the input params returns the correct set of roles.
549627
func TestReadCustomRoles(t *testing.T) {
550628
t.Parallel()

‎coderd/database/queries.sql.go

Lines changed: 8 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/quotas.sql

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
-- name: GetQuotaAllowanceForUser :one
22
SELECT
3-
coalesce(SUM(quota_allowance), 0)::BIGINT
3+
coalesce(SUM(groups.quota_allowance), 0)::BIGINT
44
FROM
5-
groups g
6-
LEFT JOIN group_members gm ON
7-
g.id = gm.group_id
8-
WHERE
9-
user_id = $1
10-
OR
11-
g.id = g.organization_id;
5+
(
6+
-- Select all groups this user is a member of. This will also include
7+
-- the "Everyone" group for organizations the user is a member of.
8+
SELECT * FROM group_members_expanded WHERE @user_id = user_id
9+
) AS members
10+
INNER JOIN groups ON
11+
members.group_id = groups.id
12+
;
1213

1314
-- name: GetQuotaConsumedForUser :one
1415
WITH latest_builds AS (

‎enterprise/coderd/workspacequota_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,51 @@ func TestWorkspaceQuota(t *testing.T) {
233233
verifyQuota(ctx, t, client, 4, 4)
234234
require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status)
235235
})
236+
237+
// Ensures allowance from everyone groups only counts if you are an org member.
238+
// This was a bug where the group "Everyone" was being counted for all users,
239+
// regardless of membership.
240+
t.Run("AllowanceEveryone", func(t *testing.T) {
241+
t.Parallel()
242+
243+
dv := coderdtest.DeploymentValues(t)
244+
dv.Experiments = []string{string(codersdk.ExperimentMultiOrganization)}
245+
owner, first := coderdenttest.New(t, &coderdenttest.Options{
246+
Options: &coderdtest.Options{
247+
DeploymentValues: dv,
248+
},
249+
LicenseOptions: &coderdenttest.LicenseOptions{
250+
Features: license.Features{
251+
codersdk.FeatureTemplateRBAC: 1,
252+
codersdk.FeatureMultipleOrganizations: 1,
253+
},
254+
},
255+
})
256+
member, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID)
257+
258+
// Create a second organization
259+
second := coderdenttest.CreateOrganization(t, owner, coderdenttest.CreateOrganizationOptions{})
260+
261+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
262+
defer cancel()
263+
264+
// update everyone quotas
265+
//nolint:gocritic // using owner for simplicity
266+
_, err := owner.PatchGroup(ctx, first.OrganizationID, codersdk.PatchGroupRequest{
267+
QuotaAllowance: ptr.Ref(30),
268+
})
269+
require.NoError(t, err)
270+
271+
_, err = owner.PatchGroup(ctx, second.ID, codersdk.PatchGroupRequest{
272+
QuotaAllowance: ptr.Ref(15),
273+
})
274+
require.NoError(t, err)
275+
276+
verifyQuota(ctx, t, member, 0, 30)
277+
// This currently reports the total site wide quotas. We might want to
278+
// org scope this api call in the future.
279+
verifyQuota(ctx, t, owner, 0, 45)
280+
})
236281
}
237282

238283
func planWithCost(cost int32) []*proto.Response {

0 commit comments

Comments
 (0)