From 2c7b98d6c1ed19cbe53061a62e2c8c6d1d361cae Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Tue, 15 Aug 2023 22:54:18 +0000 Subject: [PATCH 01/13] feat: allow setting quotas on Everyone group --- coderd/database/dbauthz/dbauthz.go | 6 +- coderd/database/dbauthz/dbauthz_test.go | 2 +- coderd/database/dbfake/dbfake.go | 26 ++-- coderd/database/dbgen/dbgen_test.go | 5 +- coderd/database/dbmetrics/dbmetrics.go | 2 +- coderd/database/dbmock/dbmock.go | 2 +- coderd/database/modelmethods.go | 4 + coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 37 ++++-- coderd/database/queries/groupmembers.sql | 20 +++- coderd/database/queries/groups.sql | 4 +- coderd/database/queries/quotas.sql | 8 +- enterprise/cli/grouplist_test.go | 16 ++- enterprise/coderd/groups.go | 86 ++++++++++---- enterprise/coderd/groups_test.go | 112 +++++++++++++++++- enterprise/coderd/templates.go | 10 +- enterprise/coderd/templates_test.go | 3 +- enterprise/coderd/userauth_test.go | 7 +- enterprise/coderd/workspacequota_test.go | 22 ++-- site/src/pages/GroupsPage/GroupPage.tsx | 12 +- .../GroupsPage/SettingsGroupPageView.tsx | 2 + 21 files changed, 306 insertions(+), 82 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 76d998a83dffe..26369fb82a7e5 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -916,11 +916,11 @@ func (q *querier) GetGroupByOrgAndName(ctx context.Context, arg database.GetGrou return fetch(q.log, q.auth, q.db.GetGroupByOrgAndName)(ctx, arg) } -func (q *querier) GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]database.User, error) { - if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check +func (q *querier) GetGroupMembers(ctx context.Context, arg database.GetGroupMembersParams) ([]database.User, error) { + if _, err := q.GetGroupByID(ctx, arg.ID); err != nil { // AuthZ check return nil, err } - return q.db.GetGroupMembers(ctx, groupID) + return q.db.GetGroupMembers(ctx, arg) } func (q *querier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index d6ad41f51408a..ccf4c81719074 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -288,7 +288,7 @@ func (s *MethodTestSuite) TestGroup() { s.Run("GetGroupMembers", s.Subtest(func(db database.Store, check *expects) { g := dbgen.Group(s.T(), db, database.Group{}) _ = dbgen.GroupMember(s.T(), db, database.GroupMember{}) - check.Args(g.ID).Asserts(g, rbac.ActionRead) + check.Args(database.GetGroupMembersParams{ID: g.ID, OrganizationID: g.OrganizationID}).Asserts(g, rbac.ActionRead) })) s.Run("InsertAllUsersGroup", s.Subtest(func(db database.Store, check *expects) { o := dbgen.Organization(s.T(), db, database.Organization{}) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 07ba63caa17f1..bf789ff40a52a 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -1376,13 +1376,18 @@ func (q *FakeQuerier) GetGroupByOrgAndName(_ context.Context, arg database.GetGr return database.Group{}, sql.ErrNoRows } -func (q *FakeQuerier) GetGroupMembers(_ context.Context, groupID uuid.UUID) ([]database.User, error) { +func (q *FakeQuerier) GetGroupMembers(_ context.Context, arg database.GetGroupMembersParams) ([]database.User, error) { q.mutex.RLock() defer q.mutex.RUnlock() + if arg.ID == arg.OrganizationID { + var cp []database.User + return append(cp, q.users...), nil + } + var members []database.GroupMember for _, member := range q.groupMembers { - if member.GroupID == groupID { + if member.GroupID == arg.ID { members = append(members, member) } } @@ -1401,17 +1406,12 @@ func (q *FakeQuerier) GetGroupMembers(_ context.Context, groupID uuid.UUID) ([]d return users, nil } -func (q *FakeQuerier) GetGroupsByOrganizationID(_ context.Context, organizationID uuid.UUID) ([]database.Group, error) { +func (q *FakeQuerier) GetGroupsByOrganizationID(_ context.Context, _ uuid.UUID) ([]database.Group, error) { q.mutex.RLock() defer q.mutex.RUnlock() var groups []database.Group - for _, group := range q.groups { - // Omit the allUsers group. - if group.OrganizationID == organizationID && group.ID != organizationID { - groups = append(groups, group) - } - } + groups = append(groups, q.groups...) return groups, nil } @@ -1838,9 +1838,17 @@ func (q *FakeQuerier) GetQuotaAllowanceForUser(_ context.Context, userID uuid.UU for _, group := range q.groups { if group.ID == member.GroupID { sum += int64(group.QuotaAllowance) + continue } } } + // Grab the quota for the Everyone group. + for _, group := range q.groups { + if group.ID == group.OrganizationID { + sum += int64(group.QuotaAllowance) + break + } + } return sum, nil } diff --git a/coderd/database/dbgen/dbgen_test.go b/coderd/database/dbgen/dbgen_test.go index 5509455f7a586..8c6e5175c416d 100644 --- a/coderd/database/dbgen/dbgen_test.go +++ b/coderd/database/dbgen/dbgen_test.go @@ -105,7 +105,10 @@ func TestGenerator(t *testing.T) { exp := []database.User{u} dbgen.GroupMember(t, db, database.GroupMember{GroupID: g.ID, UserID: u.ID}) - require.Equal(t, exp, must(db.GetGroupMembers(context.Background(), g.ID))) + require.Equal(t, exp, must(db.GetGroupMembers(context.Background(), database.GetGroupMembersParams{ + ID: g.ID, + OrganizationID: g.OrganizationID, + }))) }) t.Run("Organization", func(t *testing.T) { diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 613ac28f68c59..f537df0e9acb3 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -370,7 +370,7 @@ func (m metricsStore) GetGroupByOrgAndName(ctx context.Context, arg database.Get return group, err } -func (m metricsStore) GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]database.User, error) { +func (m metricsStore) GetGroupMembers(ctx context.Context, groupID database.GetGroupMembersParams) ([]database.User, error) { start := time.Now() users, err := m.s.GetGroupMembers(ctx, groupID) m.queryLatencies.WithLabelValues("GetGroupMembers").Observe(time.Since(start).Seconds()) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 49e80eb168bef..30943d9d073cd 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -702,7 +702,7 @@ func (mr *MockStoreMockRecorder) GetGroupByOrgAndName(arg0, arg1 interface{}) *g } // GetGroupMembers mocks base method. -func (m *MockStore) GetGroupMembers(arg0 context.Context, arg1 uuid.UUID) ([]database.User, error) { +func (m *MockStore) GetGroupMembers(arg0 context.Context, arg1 database.GetGroupMembersParams) ([]database.User, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetGroupMembers", arg0, arg1) ret0, _ := ret[0].([]database.User) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 3daaec35da595..958e59f3f6cb6 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -362,3 +362,7 @@ func ConvertWorkspaceRows(rows []GetWorkspacesRow) []Workspace { return workspaces } + +func (g Group) IsEveryoneGroup() bool { + return g.ID == g.OrganizationID +} diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 46e45b693236c..92290ade8d613 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -72,7 +72,7 @@ type sqlcQuerier interface { GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) GetGroupByID(ctx context.Context, id uuid.UUID) (Group, error) GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrgAndNameParams) (Group, error) - GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]User, error) + GetGroupMembers(ctx context.Context, arg GetGroupMembersParams) ([]User, error) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) GetHungProvisionerJobs(ctx context.Context, updatedAt time.Time) ([]ProvisionerJob, error) GetLastUpdateCheck(ctx context.Context) (string, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index e8e77cbfaa02f..3adc491102bd8 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1069,20 +1069,39 @@ SELECT users.id, users.email, users.username, users.hashed_password, users.created_at, users.updated_at, users.status, users.rbac_roles, users.login_type, users.avatar_url, users.deleted, users.last_seen_at, users.quiet_hours_schedule FROM users -JOIN +LEFT JOIN group_members ON - users.id = group_members.user_id + CASE WHEN $1:: uuid != $2 :: uuid THEN + group_members.user_id = users.id + END +LEFT JOIN + organization_members +ON + CASE WHEN $1 :: uuid = $2 :: uuid THEN + organization_members.user_id = users.id + END WHERE - group_members.group_id = $1 + CASE WHEN $1 :: uuid != $2 :: uuid THEN + group_members.group_id = $1 + ELSE true END +AND + CASE WHEN $1 :: uuid = $2 :: uuid THEN + organization_members.organization_id = $2 + ELSE true END AND users.status = 'active' AND users.deleted = 'false' ` -func (q *sqlQuerier) GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]User, error) { - rows, err := q.db.QueryContext(ctx, getGroupMembers, groupID) +type GetGroupMembersParams struct { + ID uuid.UUID `db:"id" json:"id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` +} + +func (q *sqlQuerier) GetGroupMembers(ctx context.Context, arg GetGroupMembersParams) ([]User, error) { + rows, err := q.db.QueryContext(ctx, getGroupMembers, arg.ID, arg.OrganizationID) if err != nil { return nil, err } @@ -1244,8 +1263,6 @@ FROM groups WHERE organization_id = $1 -AND - id != $1 ` func (q *sqlQuerier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) { @@ -3398,11 +3415,13 @@ const getQuotaAllowanceForUser = `-- name: GetQuotaAllowanceForUser :one SELECT coalesce(SUM(quota_allowance), 0)::BIGINT FROM - group_members gm -JOIN groups g ON + groups g +LEFT JOIN group_members gm ON g.id = gm.group_id WHERE user_id = $1 +OR + g.id = g.organization_id ` func (q *sqlQuerier) GetQuotaAllowanceForUser(ctx context.Context, userID uuid.UUID) (int64, error) { diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index a7c04d85f20a8..921e20ece1f0d 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -3,12 +3,26 @@ SELECT users.* FROM users -JOIN +LEFT JOIN group_members ON - users.id = group_members.user_id + CASE WHEN @id:: uuid != @organization_id :: uuid THEN + group_members.user_id = users.id + END +LEFT JOIN + organization_members +ON + CASE WHEN @id :: uuid = @organization_id :: uuid THEN + organization_members.user_id = users.id + END WHERE - group_members.group_id = $1 + CASE WHEN @id :: uuid != @organization_id :: uuid THEN + group_members.group_id = @id + ELSE true END +AND + CASE WHEN @id :: uuid = @organization_id :: uuid THEN + organization_members.organization_id = @organization_id + ELSE true END AND users.status = 'active' AND diff --git a/coderd/database/queries/groups.sql b/coderd/database/queries/groups.sql index da47116983c87..e772d21a5840f 100644 --- a/coderd/database/queries/groups.sql +++ b/coderd/database/queries/groups.sql @@ -26,9 +26,7 @@ SELECT FROM groups WHERE - organization_id = $1 -AND - id != $1; + organization_id = $1; -- name: InsertGroup :one INSERT INTO groups ( diff --git a/coderd/database/queries/quotas.sql b/coderd/database/queries/quotas.sql index c640ba02ce982..48b9a673c7f03 100644 --- a/coderd/database/queries/quotas.sql +++ b/coderd/database/queries/quotas.sql @@ -2,11 +2,13 @@ SELECT coalesce(SUM(quota_allowance), 0)::BIGINT FROM - group_members gm -JOIN groups g ON + groups g +LEFT JOIN group_members gm ON g.id = gm.group_id WHERE - user_id = $1; + user_id = $1 +OR + g.id = g.organization_id; -- name: GetQuotaConsumedForUser :one WITH latest_builds AS ( diff --git a/enterprise/cli/grouplist_test.go b/enterprise/cli/grouplist_test.go index 90e054a03faab..ffdb159982bfb 100644 --- a/enterprise/cli/grouplist_test.go +++ b/enterprise/cli/grouplist_test.go @@ -74,10 +74,10 @@ func TestGroupList(t *testing.T) { } }) - t.Run("NoGroups", func(t *testing.T) { + t.Run("EveryoneGroup", func(t *testing.T) { t.Parallel() - client, _ := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ codersdk.FeatureTemplateRBAC: 1, }, @@ -87,13 +87,19 @@ func TestGroupList(t *testing.T) { pty := ptytest.New(t) - inv.Stderr = pty.Output() + inv.Stdout = pty.Output() clitest.SetupConfig(t, client, conf) err := inv.Run() require.NoError(t, err) - pty.ExpectMatch("No groups found") - pty.ExpectMatch("coder groups create ") + matches := []string{ + "NAME", "ORGANIZATION ID", "MEMBERS", " AVATAR URL", + "Everyone", user.OrganizationID.String(), coderdtest.FirstUserParams.Email, "", + } + + for _, match := range matches { + pty.ExpectMatch(match) + } }) } diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index 620e64530c1be..e42ed7afada07 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -102,36 +102,59 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { ) defer commitAudit() - currentMembers, currentMembersErr := api.Database.GetGroupMembers(ctx, group.ID) - if currentMembersErr != nil { - httpapi.InternalServerError(rw, currentMembersErr) + var req codersdk.PatchGroupRequest + if !httpapi.Read(ctx, rw, r, &req) { return } - aReq.Old = group.Auditable(currentMembers) + // If the name matches the existing group name pretend we aren't + // updating the name at all. + if req.Name == group.Name { + req.Name = "" + } - var req codersdk.PatchGroupRequest - if !httpapi.Read(ctx, rw, r, &req) { + if group.ID == group.OrganizationID && req.Name != "" { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Cannot rename the %q group!", database.AllUsersGroup), + }) return } - if req.Name != "" && req.Name == database.AllUsersGroup { + if group.ID == group.OrganizationID && (req.DisplayName != nil && *req.DisplayName != "") { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("%q is a reserved group name!", database.AllUsersGroup), + Message: fmt.Sprintf("Cannot update the Display Name for the %q group!", database.AllUsersGroup), }) return } - // If the name matches the existing group name pretend we aren't - // updating the name at all. - if req.Name == group.Name { - req.Name = "" + if req.Name != "" && req.Name == database.AllUsersGroup { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("%q is a reserved group name!", database.AllUsersGroup), + }) + return } users := make([]string, 0, len(req.AddUsers)+len(req.RemoveUsers)) users = append(users, req.AddUsers...) users = append(users, req.RemoveUsers...) + if len(users) > 0 && group.Name == database.AllUsersGroup { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Cannot add or remove users from the %q group!", database.AllUsersGroup), + }) + return + } + + currentMembers, currentMembersErr := api.Database.GetGroupMembers(ctx, database.GetGroupMembersParams{ + ID: group.ID, + OrganizationID: group.OrganizationID, + }) + if currentMembersErr != nil { + httpapi.InternalServerError(rw, currentMembersErr) + return + } + aReq.Old = group.Auditable(currentMembers) + for _, id := range users { if _, err := uuid.Parse(id); err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ @@ -156,6 +179,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { return } } + if req.Name != "" && req.Name != group.Name { _, err := api.Database.GetGroupByOrgAndName(ctx, database.GetGroupByOrgAndNameParams{ OrganizationID: group.OrganizationID, @@ -230,7 +254,9 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { } } return nil - }, nil) + }, &sql.TxOptions{ + Isolation: sql.LevelRepeatableRead, + }) if database.IsUniqueViolation(err) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Cannot add the same user to a group twice!", @@ -250,7 +276,10 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { return } - patchedMembers, err := api.Database.GetGroupMembers(ctx, group.ID) + patchedMembers, err := api.Database.GetGroupMembers(ctx, database.GetGroupMembersParams{ + ID: group.ID, + OrganizationID: group.OrganizationID, + }) if err != nil { httpapi.InternalServerError(rw, err) return @@ -283,14 +312,6 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { ) defer commitAudit() - groupMembers, getMembersErr := api.Database.GetGroupMembers(ctx, group.ID) - if getMembersErr != nil { - httpapi.InternalServerError(rw, getMembersErr) - return - } - - aReq.Old = group.Auditable(groupMembers) - if group.Name == database.AllUsersGroup { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("%q is a reserved group and cannot be deleted!", database.AllUsersGroup), @@ -298,6 +319,17 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { return } + groupMembers, getMembersErr := api.Database.GetGroupMembers(ctx, database.GetGroupMembersParams{ + ID: group.ID, + OrganizationID: group.OrganizationID, + }) + if getMembersErr != nil { + httpapi.InternalServerError(rw, getMembersErr) + return + } + + aReq.Old = group.Auditable(groupMembers) + err := api.Database.DeleteGroupByID(ctx, group.ID) if err != nil { httpapi.InternalServerError(rw, err) @@ -336,7 +368,10 @@ func (api *API) group(rw http.ResponseWriter, r *http.Request) { group = httpmw.GroupParam(r) ) - users, err := api.Database.GetGroupMembers(ctx, group.ID) + users, err := api.Database.GetGroupMembers(ctx, database.GetGroupMembersParams{ + ID: group.ID, + OrganizationID: group.OrganizationID, + }) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { httpapi.InternalServerError(rw, err) return @@ -381,7 +416,10 @@ func (api *API) groups(rw http.ResponseWriter, r *http.Request) { resp := make([]codersdk.Group, 0, len(groups)) for _, group := range groups { - members, err := api.Database.GetGroupMembers(ctx, group.ID) + members, err := api.Database.GetGroupMembers(ctx, database.GetGroupMembersParams{ + ID: group.ID, + OrganizationID: group.OrganizationID, + }) if err != nil { httpapi.InternalServerError(rw, err) return diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 5999fa47b1f6d..f430e78bea8b3 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -399,7 +399,7 @@ func TestPatchGroup(t *testing.T) { require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) }) - t.Run("allUsers", func(t *testing.T) { + t.Run("ReservedName", func(t *testing.T) { t.Parallel() client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ @@ -421,6 +421,107 @@ func TestPatchGroup(t *testing.T) { require.True(t, ok) require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) }) + + t.Run("EveryoneGroup", func(t *testing.T) { + t.Parallel() + t.Run("NoUpdateName", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + ctx := testutil.Context(t, testutil.WaitLong) + _, err := client.PatchGroup(ctx, user.OrganizationID, codersdk.PatchGroupRequest{ + Name: "hi", + }) + require.Error(t, err) + cerr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }) + + t.Run("NoUpdateDisplayName", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + ctx := testutil.Context(t, testutil.WaitLong) + _, err := client.PatchGroup(ctx, user.OrganizationID, codersdk.PatchGroupRequest{ + DisplayName: ptr.Ref("hi"), + }) + require.Error(t, err) + cerr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }) + + t.Run("NoAddUsers", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + _, user2 := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + + ctx := testutil.Context(t, testutil.WaitLong) + _, err := client.PatchGroup(ctx, user.OrganizationID, codersdk.PatchGroupRequest{ + AddUsers: []string{user2.ID.String()}, + }) + require.Error(t, err) + cerr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }) + + t.Run("NoRmUsers", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + + ctx := testutil.Context(t, testutil.WaitLong) + _, err := client.PatchGroup(ctx, user.OrganizationID, codersdk.PatchGroupRequest{ + RemoveUsers: []string{user.UserID.String()}, + }) + require.Error(t, err) + cerr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }) + + t.Run("UpdateQuota", func(t *testing.T) { + t.Parallel() + + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }}) + + ctx := testutil.Context(t, testutil.WaitLong) + group, err := client.Group(ctx, user.OrganizationID) + require.NoError(t, err) + + require.Equal(t, 0, group.QuotaAllowance) + + expectedQuota := 123 + group, err = client.PatchGroup(ctx, user.OrganizationID, codersdk.PatchGroupRequest{ + QuotaAllowance: ptr.Ref(expectedQuota), + }) + require.NoError(t, err) + require.Equal(t, expectedQuota, group.QuotaAllowance) + }) + }) } // TODO: test auth. @@ -591,13 +692,17 @@ func TestGroup(t *testing.T) { codersdk.FeatureTemplateRBAC: 1, }, }}) + _, user1 := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + _, user2 := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + ctx := testutil.Context(t, testutil.WaitLong) // The 'Everyone' group always has an ID that matches the organization ID. group, err := client.Group(ctx, user.OrganizationID) require.NoError(t, err) - require.Len(t, group.Members, 0) + require.Len(t, group.Members, 3) require.Equal(t, "Everyone", group.Name) require.Equal(t, user.OrganizationID, group.OrganizationID) + require.Contains(t, group.Members, user1, user2) }) } @@ -641,7 +746,8 @@ func TestGroups(t *testing.T) { groups, err := client.GroupsByOrganization(ctx, user.OrganizationID) require.NoError(t, err) - require.Len(t, groups, 2) + // 'Everyone' group + 2 custom groups. + require.Len(t, groups, 3) require.Contains(t, groups, group1) require.Contains(t, groups, group2) }) diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index 5ba96712aaa89..60b26a184cb4a 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -58,7 +58,10 @@ func (api *API) templateAvailablePermissions(rw http.ResponseWriter, r *http.Req sdkGroups := make([]codersdk.Group, 0, len(groups)) for _, group := range groups { // nolint:gocritic - members, err := api.Database.GetGroupMembers(dbauthz.AsSystemRestricted(ctx), group.ID) + members, err := api.Database.GetGroupMembers(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersParams{ + ID: group.ID, + OrganizationID: group.OrganizationID, + }) if err != nil { httpapi.InternalServerError(rw, err) return @@ -128,7 +131,10 @@ func (api *API) templateACL(rw http.ResponseWriter, r *http.Request) { // them read the group members. // We should probably at least return more truncated user data here. // nolint:gocritic - members, err = api.Database.GetGroupMembers(dbauthz.AsSystemRestricted(ctx), group.ID) + members, err = api.Database.GetGroupMembers(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersParams{ + ID: group.ID, + OrganizationID: group.OrganizationID, + }) if err != nil { httpapi.InternalServerError(rw, err) return diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index 68e91a3ff0975..254d444eb18b2 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -373,8 +373,7 @@ func TestTemplateACL(t *testing.T) { require.NoError(t, err) require.Len(t, acl.Groups, 1) - // We don't return members for the 'Everyone' group. - require.Len(t, acl.Groups[0].Members, 0) + require.Len(t, acl.Groups[0].Members, 2) require.Len(t, acl.Users, 0) }) diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index 176a308595205..12688c9c106bb 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -525,7 +525,7 @@ func TestGroupSync(t *testing.T) { require.NoError(t, err) for _, group := range orgGroups { - if slice.Contains(tc.initialOrgGroups, group.Name) { + if slice.Contains(tc.initialOrgGroups, group.Name) || group.OrganizationID == group.ID { require.Equal(t, group.Source, codersdk.GroupSourceUser) } else { require.Equal(t, group.Source, codersdk.GroupSourceOIDC) @@ -543,6 +543,7 @@ func TestGroupSync(t *testing.T) { } delete(orgGroupsMap, expected) } + delete(orgGroupsMap, database.AllUsersGroup) require.Empty(t, orgGroupsMap, "unexpected groups found") expectedUserGroups := make(map[string]struct{}) @@ -554,7 +555,9 @@ func TestGroupSync(t *testing.T) { userInGroup := slice.ContainsCompare(group.Members, codersdk.User{Email: user.Email}, func(a, b codersdk.User) bool { return a.Email == b.Email }) - if _, ok := expectedUserGroups[group.Name]; ok { + if group.ID == group.OrganizationID { + require.True(t, userInGroup, "user cannot be removed from 'Everyone' group") + } else if _, ok := expectedUserGroups[group.Name]; ok { require.Truef(t, userInGroup, "user should be in group %s", group.Name) } else { require.Falsef(t, userInGroup, "user should not be in group %s", group.Name) diff --git a/enterprise/coderd/workspacequota_test.go b/enterprise/coderd/workspacequota_test.go index a142c86535c4d..112db37e75b57 100644 --- a/enterprise/coderd/workspacequota_test.go +++ b/enterprise/coderd/workspacequota_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/coderd/util/ptr" "github.com/coder/coder/codersdk" "github.com/coder/coder/enterprise/coderd/coderdenttest" "github.com/coder/coder/enterprise/coderd/license" @@ -53,7 +54,14 @@ func TestWorkspaceQuota(t *testing.T) { verifyQuota(ctx, t, client, 0, 0) - // Add user to two groups, granting them a total budget of 3. + // Patch the 'Everyone' group to verify its quota allowance is being accounted for. + _, err := client.PatchGroup(ctx, user.OrganizationID, codersdk.PatchGroupRequest{ + QuotaAllowance: ptr.Ref(1), + }) + require.NoError(t, err) + verifyQuota(ctx, t, client, 0, 1) + + // Add user to two groups, granting them a total budget of 4. group1, err := client.CreateGroup(ctx, user.OrganizationID, codersdk.CreateGroupRequest{ Name: "test-1", QuotaAllowance: 1, @@ -76,7 +84,7 @@ func TestWorkspaceQuota(t *testing.T) { }) require.NoError(t, err) - verifyQuota(ctx, t, client, 0, 3) + verifyQuota(ctx, t, client, 0, 4) authToken := uuid.NewString() version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ @@ -105,7 +113,7 @@ func TestWorkspaceQuota(t *testing.T) { // Spin up three workspaces fine var wg sync.WaitGroup - for i := 0; i < 3; i++ { + for i := 0; i < 4; i++ { wg.Add(1) go func() { defer wg.Done() @@ -115,14 +123,14 @@ func TestWorkspaceQuota(t *testing.T) { }() } wg.Wait() - verifyQuota(ctx, t, client, 3, 3) + verifyQuota(ctx, t, client, 4, 4) // Next one must fail workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build := coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) // Consumed shouldn't bump - verifyQuota(ctx, t, client, 3, 3) + verifyQuota(ctx, t, client, 4, 4) require.Equal(t, codersdk.WorkspaceStatusFailed, build.Status) require.Contains(t, build.Job.Error, "quota") @@ -138,7 +146,7 @@ func TestWorkspaceQuota(t *testing.T) { }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJob(t, client, build.ID) - verifyQuota(ctx, t, client, 2, 3) + verifyQuota(ctx, t, client, 3, 4) break } @@ -146,7 +154,7 @@ func TestWorkspaceQuota(t *testing.T) { workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) build = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) - verifyQuota(ctx, t, client, 3, 3) + verifyQuota(ctx, t, client, 4, 4) require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) }) } diff --git a/site/src/pages/GroupsPage/GroupPage.tsx b/site/src/pages/GroupsPage/GroupPage.tsx index 967d4b8df066a..b700ff611f64a 100644 --- a/site/src/pages/GroupsPage/GroupPage.tsx +++ b/site/src/pages/GroupsPage/GroupPage.tsx @@ -124,6 +124,7 @@ export const GroupPage: React.FC = () => {