Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit83ccdaa

Browse files
authored
chore: fixup quotas to only include groups you are a member of (#14271)
* chore: fixup quotas to only include groups you are a member ofBefore 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 parentf619500 commit83ccdaa

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
ifmember.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:=rangeq.groups {
33223328
ifgroup.ID==member.GroupID {
33233329
sum+=int64(group.QuotaAllowance)
33243330
continue
33253331
}
33263332
}
33273333
}
3328-
// Grab the quota for the Everyone group.
3329-
for_,group:=rangeq.groups {
3330-
ifgroup.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:=rangeq.organizationMembers {
3338+
ifmem.UserID!=userID {
3339+
continue
33333340
}
3341+
3342+
group,err:=q.getGroupByIDNoLock(context.Background(),mem.OrganizationID)
3343+
iferr!=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
returnsum,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+
funcTestWorkspaceQuotas(t*testing.T) {
549+
t.Parallel()
550+
orgMemberIDs:=func(o database.OrganizationMember) uuid.UUID {
551+
returno.UserID
552+
}
553+
groupMemberIDs:=func(m database.GroupMember) uuid.UUID {
554+
returnm.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
funcTestReadCustomRoles(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 aboutcustomizing 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 gmON
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_expandedWHERE @user_id= user_id
9+
)AS members
10+
INNER JOIN groupsON
11+
members.group_id=groups.id
12+
;
1213

1314
-- name: GetQuotaConsumedForUser :one
1415
WITH latest_buildsAS (

‎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+
defercancel()
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
funcplanWithCost(costint32) []*proto.Response {

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp