From 2f6687a4752ba0eddcc936d3edc4631a950273e0 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 17 Aug 2023 13:25:16 -0500 Subject: [PATCH] feat: expose Everyone group through UI (#9117) - Allows setting quota allowances on the 'Everyone' group. --- coderd/database/dbauthz/dbauthz.go | 6 +- coderd/database/dbfake/dbfake.go | 63 ++++++++-- coderd/database/modelmethods.go | 6 +- coderd/database/querier.go | 2 + coderd/database/queries.sql.go | 25 ++-- coderd/database/queries/groupmembers.sql | 17 ++- coderd/database/queries/groups.sql | 4 +- coderd/database/queries/quotas.sql | 8 +- coderd/database/tx.go | 49 ++++++++ coderd/database/tx_test.go | 81 ++++++++++++ coderd/organizations.go | 2 +- coderd/users.go | 2 +- codersdk/groups.go | 4 + enterprise/cli/grouplist_test.go | 16 ++- enterprise/coderd/groups.go | 75 +++++++---- enterprise/coderd/groups_test.go | 116 +++++++++++++++++- enterprise/coderd/templates_test.go | 3 +- enterprise/coderd/userauth_test.go | 7 +- enterprise/coderd/workspacequota_test.go | 22 ++-- scripts/develop.sh | 2 +- site/src/pages/GroupsPage/GroupPage.tsx | 13 +- .../GroupsPage/SettingsGroupPageView.tsx | 3 + site/src/utils/groups.ts | 12 ++ 23 files changed, 458 insertions(+), 80 deletions(-) create mode 100644 coderd/database/tx.go create mode 100644 coderd/database/tx_test.go diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 896040efc3..38766121c4 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, id uuid.UUID) ([]database.User, error) { + if _, err := q.GetGroupByID(ctx, id); err != nil { // AuthZ check return nil, err } - return q.db.GetGroupMembers(ctx, groupID) + return q.db.GetGroupMembers(ctx, id) } func (q *querier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) { diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 8de3ee49be..8186564961 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -613,6 +613,44 @@ func uniqueSortedUUIDs(uuids []uuid.UUID) []uuid.UUID { return unique } +func (q *FakeQuerier) getOrganizationMember(orgID uuid.UUID) []database.OrganizationMember { + var members []database.OrganizationMember + for _, member := range q.organizationMembers { + if member.OrganizationID == orgID { + members = append(members, member) + } + } + + return members +} + +// getEveryoneGroupMembers fetches all the users in an organization. +func (q *FakeQuerier) getEveryoneGroupMembers(orgID uuid.UUID) []database.User { + var ( + everyone []database.User + orgMembers = q.getOrganizationMember(orgID) + ) + for _, member := range orgMembers { + user, err := q.GetUserByID(context.TODO(), member.UserID) + if err != nil { + return nil + } + everyone = append(everyone, user) + } + return everyone +} + +// isEveryoneGroup returns true if the provided ID matches +// an organization ID. +func (q *FakeQuerier) isEveryoneGroup(id uuid.UUID) bool { + for _, org := range q.organizations { + if org.ID == id { + return true + } + } + return false +} + func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error { return xerrors.New("AcquireLock must only be called within a transaction") } @@ -1378,13 +1416,17 @@ 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, id uuid.UUID) ([]database.User, error) { q.mutex.RLock() defer q.mutex.RUnlock() + if q.isEveryoneGroup(id) { + return q.getEveryoneGroupMembers(id), nil + } + var members []database.GroupMember for _, member := range q.groupMembers { - if member.GroupID == groupID { + if member.GroupID == id { members = append(members, member) } } @@ -1403,14 +1445,13 @@ 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, id uuid.UUID) ([]database.Group, error) { q.mutex.RLock() defer q.mutex.RUnlock() - var groups []database.Group + groups := make([]database.Group, 0, len(q.groups)) for _, group := range q.groups { - // Omit the allUsers group. - if group.OrganizationID == organizationID && group.ID != organizationID { + if group.OrganizationID == id { groups = append(groups, group) } } @@ -1840,9 +1881,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 } @@ -3548,7 +3597,7 @@ func (q *FakeQuerier) InsertAPIKey(_ context.Context, arg database.InsertAPIKeyP func (q *FakeQuerier) InsertAllUsersGroup(ctx context.Context, orgID uuid.UUID) (database.Group, error) { return q.InsertGroup(ctx, database.InsertGroupParams{ ID: orgID, - Name: database.AllUsersGroup, + Name: database.EveryoneGroup, DisplayName: "", OrganizationID: orgID, }) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 3daaec35da..760d63fae8 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -84,7 +84,7 @@ func (g Group) Auditable(users []User) AuditableGroup { } } -const AllUsersGroup = "Everyone" +const EveryoneGroup = "Everyone" func (s APIKeyScope) ToRBAC() rbac.ScopeName { switch s { @@ -362,3 +362,7 @@ func ConvertWorkspaceRows(rows []GetWorkspacesRow) []Workspace { return workspaces } + +func (g Group) IsEveryone() bool { + return g.ID == g.OrganizationID +} diff --git a/coderd/database/querier.go b/coderd/database/querier.go index d9f0d31df5..3a8f973071 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -72,6 +72,8 @@ 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) + // If the group is a user made group, then we need to check the group_members table. + // If it is the "Everyone" group, then we need to check the organization_members table. GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]User, error) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) GetHungProvisionerJobs(ctx context.Context, updatedAt time.Time) ([]ProvisionerJob, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a5f48eb900..1a8c7598f7 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1069,18 +1069,29 @@ 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 -WHERE + group_members.user_id = users.id AND group_members.group_id = $1 +LEFT JOIN + organization_members +ON + organization_members.user_id = users.id AND + organization_members.organization_id = $1 +WHERE + -- In either case, the group_id will only match an org or a group. + (group_members.group_id = $1 + OR + organization_members.organization_id = $1) AND users.status = 'active' AND users.deleted = 'false' ` +// If the group is a user made group, then we need to check the group_members table. +// If it is the "Everyone" group, then we need to check the organization_members table. func (q *sqlQuerier) GetGroupMembers(ctx context.Context, groupID uuid.UUID) ([]User, error) { rows, err := q.db.QueryContext(ctx, getGroupMembers, groupID) if err != nil { @@ -1244,8 +1255,6 @@ FROM groups WHERE organization_id = $1 -AND - id != $1 ` func (q *sqlQuerier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) { @@ -3398,11 +3407,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 a7c04d85f2..0b3d0a33f4 100644 --- a/coderd/database/queries/groupmembers.sql +++ b/coderd/database/queries/groupmembers.sql @@ -3,12 +3,23 @@ SELECT users.* FROM users -JOIN +-- If the group is a user made group, then we need to check the group_members table. +LEFT JOIN group_members ON - users.id = group_members.user_id + group_members.user_id = users.id AND + group_members.group_id = @group_id +-- If it is the "Everyone" group, then we need to check the organization_members table. +LEFT JOIN + organization_members +ON + organization_members.user_id = users.id AND + organization_members.organization_id = @group_id WHERE - group_members.group_id = $1 + -- In either case, the group_id will only match an org or a group. + (group_members.group_id = @group_id + OR + organization_members.organization_id = @group_id) AND users.status = 'active' AND diff --git a/coderd/database/queries/groups.sql b/coderd/database/queries/groups.sql index da47116983..e772d21a58 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 c640ba02ce..48b9a673c7 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/coderd/database/tx.go b/coderd/database/tx.go new file mode 100644 index 0000000000..43da15f3f0 --- /dev/null +++ b/coderd/database/tx.go @@ -0,0 +1,49 @@ +package database + +import ( + "database/sql" + + "github.com/lib/pq" + "golang.org/x/xerrors" +) + +const maxRetries = 5 + +// ReadModifyUpdate is a helper function to run a db transaction that reads some +// object(s), modifies some of the data, and writes the modified object(s) back +// to the database. It is run in a transaction at RepeatableRead isolation so +// that if another database client also modifies the data we are writing and +// commits, then the transaction is rolled back and restarted. +// +// This is needed because we typically read all object columns, modify some +// subset, and then write all columns. Consider an object with columns A, B and +// initial values A=1, B=1. Two database clients work simultaneously, with one +// client attempting to set A=2, and another attempting to set B=2. They both +// initially read A=1, B=1, and then one writes A=2, B=1, and the other writes +// A=1, B=2. With default PostgreSQL isolation of ReadCommitted, both of these +// transactions would succeed and we end up with either A=2, B=1 or A=1, B=2. +// One or other client gets their transaction wiped out even though the data +// they wanted to change didn't conflict. +// +// If we run at RepeatableRead isolation, then one or other transaction will +// fail. Let's say the transaction that sets A=2 succeeds. Then the first B=2 +// transaction fails, but here we retry. The second attempt we read A=2, B=1, +// then write A=2, B=2 as desired, and this succeeds. +func ReadModifyUpdate(db Store, f func(tx Store) error, +) error { + var err error + for retries := 0; retries < maxRetries; retries++ { + err = db.InTx(f, &sql.TxOptions{ + Isolation: sql.LevelRepeatableRead, + }) + var pqe *pq.Error + if xerrors.As(err, &pqe) { + if pqe.Code == "40001" { + // serialization error, retry + continue + } + } + return err + } + return xerrors.Errorf("too many errors; last error: %w", err) +} diff --git a/coderd/database/tx_test.go b/coderd/database/tx_test.go new file mode 100644 index 0000000000..cd89a64cb8 --- /dev/null +++ b/coderd/database/tx_test.go @@ -0,0 +1,81 @@ +package database_test + +import ( + "database/sql" + "testing" + + "github.com/golang/mock/gomock" + "github.com/lib/pq" + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + + "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/database/dbmock" +) + +func TestReadModifyUpdate_OK(t *testing.T) { + t.Parallel() + + mDB := dbmock.NewMockStore(gomock.NewController(t)) + + mDB.EXPECT(). + InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}). + Times(1). + Return(nil) + err := database.ReadModifyUpdate(mDB, func(tx database.Store) error { + return nil + }) + require.NoError(t, err) +} + +func TestReadModifyUpdate_RetryOK(t *testing.T) { + t.Parallel() + + mDB := dbmock.NewMockStore(gomock.NewController(t)) + + firstUpdate := mDB.EXPECT(). + InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}). + Times(1). + Return(&pq.Error{Code: pq.ErrorCode("40001")}) + mDB.EXPECT(). + InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}). + After(firstUpdate). + Times(1). + Return(nil) + + err := database.ReadModifyUpdate(mDB, func(tx database.Store) error { + return nil + }) + require.NoError(t, err) +} + +func TestReadModifyUpdate_HardError(t *testing.T) { + t.Parallel() + + mDB := dbmock.NewMockStore(gomock.NewController(t)) + + mDB.EXPECT(). + InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}). + Times(1). + Return(xerrors.New("a bad thing happened")) + + err := database.ReadModifyUpdate(mDB, func(tx database.Store) error { + return nil + }) + require.ErrorContains(t, err, "a bad thing happened") +} + +func TestReadModifyUpdate_TooManyRetries(t *testing.T) { + t.Parallel() + + mDB := dbmock.NewMockStore(gomock.NewController(t)) + + mDB.EXPECT(). + InTx(gomock.Any(), &sql.TxOptions{Isolation: sql.LevelRepeatableRead}). + Times(5). + Return(&pq.Error{Code: pq.ErrorCode("40001")}) + err := database.ReadModifyUpdate(mDB, func(tx database.Store) error { + return nil + }) + require.ErrorContains(t, err, "too many errors") +} diff --git a/coderd/organizations.go b/coderd/organizations.go index 13906baef6..17e7f458eb 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -94,7 +94,7 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) { _, err = tx.InsertAllUsersGroup(ctx, organization.ID) if err != nil { - return xerrors.Errorf("create %q group: %w", database.AllUsersGroup, err) + return xerrors.Errorf("create %q group: %w", database.EveryoneGroup, err) } return nil }, nil) diff --git a/coderd/users.go b/coderd/users.go index 29a6d56015..0da749d135 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1095,7 +1095,7 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create _, err = tx.InsertAllUsersGroup(ctx, organization.ID) if err != nil { - return xerrors.Errorf("create %q group: %w", database.AllUsersGroup, err) + return xerrors.Errorf("create %q group: %w", database.EveryoneGroup, err) } } diff --git a/codersdk/groups.go b/codersdk/groups.go index b50455d693..2796a776a9 100644 --- a/codersdk/groups.go +++ b/codersdk/groups.go @@ -35,6 +35,10 @@ type Group struct { Source GroupSource `json:"source"` } +func (g Group) IsEveryone() bool { + return g.ID == g.OrganizationID +} + func (c *Client) CreateGroup(ctx context.Context, orgID uuid.UUID, req CreateGroupRequest) (Group, error) { res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/groups", orgID.String()), diff --git a/enterprise/cli/grouplist_test.go b/enterprise/cli/grouplist_test.go index 90e054a03f..022f615a3d 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("Everyone", 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 620e64530c..f927aa3361 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -46,9 +46,9 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request) return } - if req.Name == database.AllUsersGroup { + if req.Name == database.EveryoneGroup { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("%q is a reserved keyword and cannot be used for a group name.", database.AllUsersGroup), + Message: fmt.Sprintf("%q is a reserved keyword and cannot be used for a group name.", database.EveryoneGroup), }) return } @@ -102,36 +102,56 @@ 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) - return - } - - aReq.Old = group.Auditable(currentMembers) - var req codersdk.PatchGroupRequest if !httpapi.Read(ctx, rw, r, &req) { return } - 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 - } - // 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 group.IsEveryone() && req.Name != "" { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Cannot rename the %q group!", database.EveryoneGroup), + }) + return + } + + if group.IsEveryone() && (req.DisplayName != nil && *req.DisplayName != "") { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Cannot update the Display Name for the %q group!", database.EveryoneGroup), + }) + return + } + + if req.Name == database.EveryoneGroup { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("%q is a reserved group name!", database.EveryoneGroup), + }) + 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.EveryoneGroup { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: fmt.Sprintf("Cannot add or remove users from the %q group!", database.EveryoneGroup), + }) + return + } + + currentMembers, err := api.Database.GetGroupMembers(ctx, group.ID) + if err != nil { + httpapi.InternalServerError(rw, err) + 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 +176,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, @@ -169,8 +190,7 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { } } - err := api.Database.InTx(func(tx database.Store) error { - var err error + err = database.ReadModifyUpdate(api.Database, func(tx database.Store) error { group, err = tx.GetGroupByID(ctx, group.ID) if err != nil { return xerrors.Errorf("get group by ID: %w", err) @@ -230,7 +250,8 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { } } return nil - }, nil) + }) + if database.IsUniqueViolation(err) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Cannot add the same user to a group twice!", @@ -283,6 +304,13 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { ) defer commitAudit() + if group.Name == database.EveryoneGroup { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("%q is a reserved group and cannot be deleted!", database.EveryoneGroup), + }) + return + } + groupMembers, getMembersErr := api.Database.GetGroupMembers(ctx, group.ID) if getMembersErr != nil { httpapi.InternalServerError(rw, getMembersErr) @@ -291,13 +319,6 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { 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), - }) - return - } - err := api.Database.DeleteGroupByID(ctx, group.ID) if err != nil { httpapi.InternalServerError(rw, err) diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index 5999fa47b1..3794829746 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -105,7 +105,7 @@ func TestCreateGroup(t *testing.T) { }}) ctx := testutil.Context(t, testutil.WaitLong) _, err := client.CreateGroup(ctx, user.OrganizationID, codersdk.CreateGroupRequest{ - Name: database.AllUsersGroup, + Name: database.EveryoneGroup, }) require.Error(t, err) cerr, ok := codersdk.AsError(err) @@ -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{ @@ -414,13 +414,114 @@ func TestPatchGroup(t *testing.T) { require.NoError(t, err) group, err = client.PatchGroup(ctx, group.ID, codersdk.PatchGroupRequest{ - Name: database.AllUsersGroup, + Name: database.EveryoneGroup, }) require.Error(t, err) cerr, ok := codersdk.AsError(err) require.True(t, ok) require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) }) + + t.Run("Everyone", 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.StatusForbidden, 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.StatusForbidden, 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_test.go b/enterprise/coderd/templates_test.go index 68e91a3ff0..254d444eb1 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 176a308595..0b773871b9 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.IsEveryone() { 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.EveryoneGroup) 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.IsEveryone() { + 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 a142c86535..112db37e75 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/scripts/develop.sh b/scripts/develop.sh index cc1ab23f05..fc21ae8b64 100755 --- a/scripts/develop.sh +++ b/scripts/develop.sh @@ -131,7 +131,7 @@ fatal() { trap 'fatal "Script encountered an error"' ERR cdroot - start_cmd API "" "${CODER_DEV_SHIM}" server --http-address 0.0.0.0:3000 --swagger-enable --access-url "http://127.0.0.1:3000" --dangerous-allow-cors-requests=true "$@" + start_cmd API "" "${CODER_DEV_SHIM}" server --http-address 0.0.0.0:3000 --swagger-enable --dangerous-allow-cors-requests=true "$@" echo '== Waiting for Coder to become ready' # Start the timeout in the background so interrupting this script diff --git a/site/src/pages/GroupsPage/GroupPage.tsx b/site/src/pages/GroupsPage/GroupPage.tsx index 967d4b8df0..a1174336c4 100644 --- a/site/src/pages/GroupsPage/GroupPage.tsx +++ b/site/src/pages/GroupsPage/GroupPage.tsx @@ -38,6 +38,7 @@ import { TableToolbar, } from "components/TableToolbar/TableToolbar" import { UserAvatar } from "components/UserAvatar/UserAvatar" +import { isEveryoneGroup } from "utils/groups" const AddGroupMember: React.FC<{ isLoading: boolean @@ -124,6 +125,7 @@ export const GroupPage: React.FC = () => {