From e11299c5c0836a9e11b0ff06315569a202584db4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 Aug 2024 15:54:57 -0500 Subject: [PATCH 1/5] chore: fixup quotas to only include groups you are a member of Before all everyone groups were included in the allowance. --- coderd/database/queries.sql.go | 16 ++++++++-------- coderd/database/queries/quotas.sql | 17 +++++++++-------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index e7b30643a2b2f..0d2a64d1c7b63 100644 --- a/coderd/database/queries.sql.go +++ b/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) { diff --git a/coderd/database/queries/quotas.sql b/coderd/database/queries/quotas.sql index 48b9a673c7f03..95679822c4f22 100644 --- a/coderd/database/queries/quotas.sql +++ b/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 ( From 7d2768201ba08fc1ab7a3bf048ae39d050cdf626 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 Aug 2024 16:05:08 -0500 Subject: [PATCH 2/5] chore: add unit test to execercise the bug --- coderd/database/dbmem/dbmem.go | 18 +++++++--- enterprise/coderd/workspacequota_test.go | 44 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index aa2665d3c3257..534c090e5e677 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3325,13 +3325,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 { - sum += int64(group.QuotaAllowance) - break + + // 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) } + return sum, nil } diff --git a/enterprise/coderd/workspacequota_test.go b/enterprise/coderd/workspacequota_test.go index 4ebad488942f3..48b1770e8ae40 100644 --- a/enterprise/coderd/workspacequota_test.go +++ b/enterprise/coderd/workspacequota_test.go @@ -233,6 +233,50 @@ 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 + _, 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 { From 3de50302f89ca1d4e053ce2da80202338c487042 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 Aug 2024 09:35:57 -0500 Subject: [PATCH 3/5] linting --- enterprise/coderd/workspacequota_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/enterprise/coderd/workspacequota_test.go b/enterprise/coderd/workspacequota_test.go index 48b1770e8ae40..0b375d5c1d9b0 100644 --- a/enterprise/coderd/workspacequota_test.go +++ b/enterprise/coderd/workspacequota_test.go @@ -262,6 +262,7 @@ func TestWorkspaceQuota(t *testing.T) { defer cancel() // update everyone quotas + //nolint:gocritic // using owner for simplicity _, err := owner.PatchGroup(ctx, first.OrganizationID, codersdk.PatchGroupRequest{ QuotaAllowance: ptr.Ref(30), }) From c85052b9467501157fbaf7e5a9f84854baf24a32 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 Aug 2024 11:23:12 -0500 Subject: [PATCH 4/5] add unit test to add rows into the everyone group --- coderd/database/querier_test.go | 78 +++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index f42755763ff55..73686593afc3b 100644 --- a/coderd/database/querier_test.go +++ b/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() From 3a59b2c2f38e25d86b75c316f90f8c9337a88535 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 Aug 2024 12:12:35 -0500 Subject: [PATCH 5/5] fixup dbmem --- coderd/database/dbmem/dbmem.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 534c090e5e677..83e7a2954607d 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/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)