Skip to content

Commit 2f6687a

Browse files
sreyasreya
authored
feat: expose Everyone group through UI (#9117)
- Allows setting quota allowances on the 'Everyone' group.
1 parent 8910f05 commit 2f6687a

23 files changed

+455
-77
lines changed

‎coderd/database/dbauthz/dbauthz.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -916,11 +916,11 @@ func (q *querier) GetGroupByOrgAndName(ctx context.Context, arg database.GetGrou
916916
return fetch(q.log, q.auth, q.db.GetGroupByOrgAndName)(ctx, arg)
917917
}
918918

919-
func (q *querier) GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]database.User, error) {
920-
if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check
919+
func (q *querier) GetGroupMembers(ctx context.Context, id uuid.UUID) ([]database.User, error) {
920+
if _, err := q.GetGroupByID(ctx, id); err != nil { // AuthZ check
921921
return nil, err
922922
}
923-
return q.db.GetGroupMembers(ctx, groupID)
923+
return q.db.GetGroupMembers(ctx, id)
924924
}
925925

926926
func (q *querier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) {

‎coderd/database/dbfake/dbfake.go

+56-7
Original file line numberDiff line numberDiff line change
@@ -613,6 +613,44 @@ func uniqueSortedUUIDs(uuids []uuid.UUID) []uuid.UUID {
613613
return unique
614614
}
615615

616+
func (q *FakeQuerier) getOrganizationMember(orgID uuid.UUID) []database.OrganizationMember {
617+
var members []database.OrganizationMember
618+
for _, member := range q.organizationMembers {
619+
if member.OrganizationID == orgID {
620+
members = append(members, member)
621+
}
622+
}
623+
624+
return members
625+
}
626+
627+
// getEveryoneGroupMembers fetches all the users in an organization.
628+
func (q *FakeQuerier) getEveryoneGroupMembers(orgID uuid.UUID) []database.User {
629+
var (
630+
everyone []database.User
631+
orgMembers = q.getOrganizationMember(orgID)
632+
)
633+
for _, member := range orgMembers {
634+
user, err := q.GetUserByID(context.TODO(), member.UserID)
635+
if err != nil {
636+
return nil
637+
}
638+
everyone = append(everyone, user)
639+
}
640+
return everyone
641+
}
642+
643+
// isEveryoneGroup returns true if the provided ID matches
644+
// an organization ID.
645+
func (q *FakeQuerier) isEveryoneGroup(id uuid.UUID) bool {
646+
for _, org := range q.organizations {
647+
if org.ID == id {
648+
return true
649+
}
650+
}
651+
return false
652+
}
653+
616654
func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error {
617655
return xerrors.New("AcquireLock must only be called within a transaction")
618656
}
@@ -1378,13 +1416,17 @@ func (q *FakeQuerier) GetGroupByOrgAndName(_ context.Context, arg database.GetGr
13781416
return database.Group{}, sql.ErrNoRows
13791417
}
13801418

1381-
func (q *FakeQuerier) GetGroupMembers(_ context.Context, groupID uuid.UUID) ([]database.User, error) {
1419+
func (q *FakeQuerier) GetGroupMembers(_ context.Context, id uuid.UUID) ([]database.User, error) {
13821420
q.mutex.RLock()
13831421
defer q.mutex.RUnlock()
13841422

1423+
if q.isEveryoneGroup(id) {
1424+
return q.getEveryoneGroupMembers(id), nil
1425+
}
1426+
13851427
var members []database.GroupMember
13861428
for _, member := range q.groupMembers {
1387-
if member.GroupID == groupID {
1429+
if member.GroupID == id {
13881430
members = append(members, member)
13891431
}
13901432
}
@@ -1403,14 +1445,13 @@ func (q *FakeQuerier) GetGroupMembers(_ context.Context, groupID uuid.UUID) ([]d
14031445
return users, nil
14041446
}
14051447

1406-
func (q *FakeQuerier) GetGroupsByOrganizationID(_ context.Context, organizationID uuid.UUID) ([]database.Group, error) {
1448+
func (q *FakeQuerier) GetGroupsByOrganizationID(_ context.Context, id uuid.UUID) ([]database.Group, error) {
14071449
q.mutex.RLock()
14081450
defer q.mutex.RUnlock()
14091451

1410-
var groups []database.Group
1452+
groups := make([]database.Group, 0, len(q.groups))
14111453
for _, group := range q.groups {
1412-
// Omit the allUsers group.
1413-
if group.OrganizationID == organizationID && group.ID != organizationID {
1454+
if group.OrganizationID == id {
14141455
groups = append(groups, group)
14151456
}
14161457
}
@@ -1840,9 +1881,17 @@ func (q *FakeQuerier) GetQuotaAllowanceForUser(_ context.Context, userID uuid.UU
18401881
for _, group := range q.groups {
18411882
if group.ID == member.GroupID {
18421883
sum += int64(group.QuotaAllowance)
1884+
continue
18431885
}
18441886
}
18451887
}
1888+
// Grab the quota for the Everyone group.
1889+
for _, group := range q.groups {
1890+
if group.ID == group.OrganizationID {
1891+
sum += int64(group.QuotaAllowance)
1892+
break
1893+
}
1894+
}
18461895
return sum, nil
18471896
}
18481897

@@ -3548,7 +3597,7 @@ func (q *FakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP
35483597
func (q *FakeQuerier) InsertAllUsersGroup(ctx context.Context, orgID uuid.UUID) (database.Group, error) {
35493598
return q.InsertGroup(ctx, database.InsertGroupParams{
35503599
ID: orgID,
3551-
Name: database.AllUsersGroup,
3600+
Name: database.EveryoneGroup,
35523601
DisplayName: "",
35533602
OrganizationID: orgID,
35543603
})

‎coderd/database/modelmethods.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func (g Group) Auditable(users []User) AuditableGroup {
8484
}
8585
}
8686

87-
const AllUsersGroup = "Everyone"
87+
const EveryoneGroup = "Everyone"
8888

8989
func (s APIKeyScope) ToRBAC() rbac.ScopeName {
9090
switch s {
@@ -362,3 +362,7 @@ func ConvertWorkspaceRows(rows []GetWorkspacesRow) []Workspace {
362362

363363
return workspaces
364364
}
365+
366+
func (g Group) IsEveryone() bool {
367+
return g.ID == g.OrganizationID
368+
}

‎coderd/database/querier.go

+2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

+18-7
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/groupmembers.sql

+14-3
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,23 @@ SELECT
33
users.*
44
FROM
55
users
6-
JOIN
6+
-- If the group is a user made group, then we need to check the group_members table.
7+
LEFT JOIN
78
group_members
89
ON
9-
users.id = group_members.user_id
10+
group_members.user_id = users.id AND
11+
group_members.group_id = @group_id
12+
-- If it is the "Everyone" group, then we need to check the organization_members table.
13+
LEFT JOIN
14+
organization_members
15+
ON
16+
organization_members.user_id = users.id AND
17+
organization_members.organization_id = @group_id
1018
WHERE
11-
group_members.group_id = $1
19+
-- In either case, the group_id will only match an org or a group.
20+
(group_members.group_id = @group_id
21+
OR
22+
organization_members.organization_id = @group_id)
1223
AND
1324
users.status = 'active'
1425
AND

‎coderd/database/queries/groups.sql

+1-3
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,7 @@ SELECT
2626
FROM
2727
groups
2828
WHERE
29-
organization_id = $1
30-
AND
31-
id != $1;
29+
organization_id = $1;
3230

3331
-- name: InsertGroup :one
3432
INSERT INTO groups (

‎coderd/database/queries/quotas.sql

+5-3
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22
SELECT
33
coalesce(SUM(quota_allowance), 0)::BIGINT
44
FROM
5-
group_members gm
6-
JOIN groups g ON
5+
groups g
6+
LEFT JOIN group_members gm ON
77
g.id = gm.group_id
88
WHERE
9-
user_id = $1;
9+
user_id = $1
10+
OR
11+
g.id = g.organization_id;
1012

1113
-- name: GetQuotaConsumedForUser :one
1214
WITH latest_builds AS (

‎coderd/database/tx.go

+49
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
package database
2+
3+
import (
4+
"database/sql"
5+
6+
"github.com/lib/pq"
7+
"golang.org/x/xerrors"
8+
)
9+
10+
const maxRetries = 5
11+
12+
// ReadModifyUpdate is a helper function to run a db transaction that reads some
13+
// object(s), modifies some of the data, and writes the modified object(s) back
14+
// to the database. It is run in a transaction at RepeatableRead isolation so
15+
// that if another database client also modifies the data we are writing and
16+
// commits, then the transaction is rolled back and restarted.
17+
//
18+
// This is needed because we typically read all object columns, modify some
19+
// subset, and then write all columns. Consider an object with columns A, B and
20+
// initial values A=1, B=1. Two database clients work simultaneously, with one
21+
// client attempting to set A=2, and another attempting to set B=2. They both
22+
// initially read A=1, B=1, and then one writes A=2, B=1, and the other writes
23+
// A=1, B=2. With default PostgreSQL isolation of ReadCommitted, both of these
24+
// transactions would succeed and we end up with either A=2, B=1 or A=1, B=2.
25+
// One or other client gets their transaction wiped out even though the data
26+
// they wanted to change didn't conflict.
27+
//
28+
// If we run at RepeatableRead isolation, then one or other transaction will
29+
// fail. Let's say the transaction that sets A=2 succeeds. Then the first B=2
30+
// transaction fails, but here we retry. The second attempt we read A=2, B=1,
31+
// then write A=2, B=2 as desired, and this succeeds.
32+
func ReadModifyUpdate(db Store, f func(tx Store) error,
33+
) error {
34+
var err error
35+
for retries := 0; retries < maxRetries; retries++ {
36+
err = db.InTx(f, &sql.TxOptions{
37+
Isolation: sql.LevelRepeatableRead,
38+
})
39+
var pqe *pq.Error
40+
if xerrors.As(err, &pqe) {
41+
if pqe.Code == "40001" {
42+
// serialization error, retry
43+
continue
44+
}
45+
}
46+
return err
47+
}
48+
return xerrors.Errorf("too many errors; last error: %w", err)
49+
}

‎coderd/database/tx_test.go

+81
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
package database_test
2+
3+
import (
4+
"database/sql"
5+
"testing"
6+
7+
"github.com/golang/mock/gomock"
8+
"github.com/lib/pq"
9+
"github.com/stretchr/testify/require"
10+
"golang.org/x/xerrors"
11+
12+
"github.com/coder/coder/coderd/database"
13+
"github.com/coder/coder/coderd/database/dbmock"
14+
)
15+
16+
func TestReadModifyUpdate_OK(t *testing.T) {
17+
t.Parallel()
18+
19+
mDB := dbmock.NewMockStore(gomock.NewController(t))
20+
21+
mDB.EXPECT().
22+
InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}).
23+
Times(1).
24+
Return(nil)
25+
err := database.ReadModifyUpdate(mDB, func(tx database.Store) error {
26+
return nil
27+
})
28+
require.NoError(t, err)
29+
}
30+
31+
func TestReadModifyUpdate_RetryOK(t *testing.T) {
32+
t.Parallel()
33+
34+
mDB := dbmock.NewMockStore(gomock.NewController(t))
35+
36+
firstUpdate := mDB.EXPECT().
37+
InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}).
38+
Times(1).
39+
Return(&pq.Error{Code: pq.ErrorCode("40001")})
40+
mDB.EXPECT().
41+
InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}).
42+
After(firstUpdate).
43+
Times(1).
44+
Return(nil)
45+
46+
err := database.ReadModifyUpdate(mDB, func(tx database.Store) error {
47+
return nil
48+
})
49+
require.NoError(t, err)
50+
}
51+
52+
func TestReadModifyUpdate_HardError(t *testing.T) {
53+
t.Parallel()
54+
55+
mDB := dbmock.NewMockStore(gomock.NewController(t))
56+
57+
mDB.EXPECT().
58+
InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}).
59+
Times(1).
60+
Return(xerrors.New("a bad thing happened"))
61+
62+
err := database.ReadModifyUpdate(mDB, func(tx database.Store) error {
63+
return nil
64+
})
65+
require.ErrorContains(t, err, "a bad thing happened")
66+
}
67+
68+
func TestReadModifyUpdate_TooManyRetries(t *testing.T) {
69+
t.Parallel()
70+
71+
mDB := dbmock.NewMockStore(gomock.NewController(t))
72+
73+
mDB.EXPECT().
74+
InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}).
75+
Times(5).
76+
Return(&pq.Error{Code: pq.ErrorCode("40001")})
77+
err := database.ReadModifyUpdate(mDB, func(tx database.Store) error {
78+
return nil
79+
})
80+
require.ErrorContains(t, err, "too many errors")
81+
}

0 commit comments

Comments
 (0)