From 5b9531c1ea22d7eb1b702c42a132d3d5023a3700 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Tue, 31 Jan 2023 19:02:24 +0000 Subject: [PATCH 1/2] fix: omit users for 'Everyone' group in response --- coderd/database/querier.go | 1 - coderd/database/queries.sql.go | 49 ----------------------------- coderd/database/queries/groups.sql | 12 ------- enterprise/coderd/groups_test.go | 20 ++++++++++++ enterprise/coderd/templates.go | 7 +---- enterprise/coderd/templates_test.go | 9 +++--- 6 files changed, 26 insertions(+), 72 deletions(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 88327f4400392..616912acc07da 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -32,7 +32,6 @@ type sqlcQuerier interface { GetAPIKeysByLoginType(ctx context.Context, loginType LoginType) ([]APIKey, error) GetAPIKeysLastUsedAfter(ctx context.Context, lastUsed time.Time) ([]APIKey, error) GetActiveUserCount(ctx context.Context) (int64, error) - GetAllOrganizationMembers(ctx context.Context, organizationID uuid.UUID) ([]User, error) // GetAuditLogsBefore retrieves `row_limit` number of audit logs before the provided // ID. GetAuditLogsOffset(ctx context.Context, arg GetAuditLogsOffsetParams) ([]GetAuditLogsOffsetRow, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 28d29f5ba0b0e..e9d9793f89b1e 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1002,55 +1002,6 @@ func (q *sqlQuerier) DeleteGroupMember(ctx context.Context, userID uuid.UUID) er return err } -const getAllOrganizationMembers = `-- name: GetAllOrganizationMembers :many -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 -FROM - users -JOIN - organization_members -ON - users.id = organization_members.user_id -WHERE - organization_members.organization_id = $1 -` - -func (q *sqlQuerier) GetAllOrganizationMembers(ctx context.Context, organizationID uuid.UUID) ([]User, error) { - rows, err := q.db.QueryContext(ctx, getAllOrganizationMembers, organizationID) - if err != nil { - return nil, err - } - defer rows.Close() - var items []User - for rows.Next() { - var i User - if err := rows.Scan( - &i.ID, - &i.Email, - &i.Username, - &i.HashedPassword, - &i.CreatedAt, - &i.UpdatedAt, - &i.Status, - &i.RBACRoles, - &i.LoginType, - &i.AvatarURL, - &i.Deleted, - &i.LastSeenAt, - ); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const getGroupByID = `-- name: GetGroupByID :one SELECT id, name, organization_id, avatar_url, quota_allowance diff --git a/coderd/database/queries/groups.sql b/coderd/database/queries/groups.sql index 3aa5c77e22771..19fc7e9478d6b 100644 --- a/coderd/database/queries/groups.sql +++ b/coderd/database/queries/groups.sql @@ -36,18 +36,6 @@ AND AND users.deleted = 'false'; --- name: GetAllOrganizationMembers :many -SELECT - users.* -FROM - users -JOIN - organization_members -ON - users.id = organization_members.user_id -WHERE - organization_members.organization_id = $1; - -- name: GetGroupsByOrganizationID :many SELECT * diff --git a/enterprise/coderd/groups_test.go b/enterprise/coderd/groups_test.go index b57f15cf3e434..a6081f1a5a478 100644 --- a/enterprise/coderd/groups_test.go +++ b/enterprise/coderd/groups_test.go @@ -605,6 +605,26 @@ func TestGroup(t *testing.T) { require.NotContains(t, group.Members, user1) require.Contains(t, group.Members, user2) }) + + t.Run("everyoneGroupReturnsEmpty", func(t *testing.T) { + t.Parallel() + + client := coderdenttest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + + _ = coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }) + ctx, _ := testutil.Context(t) + // 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.Equal(t, "Everyone", group.Name) + require.Equal(t, user.OrganizationID, group.OrganizationID) + }) } // TODO: test auth. diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index 961ee09f0b5d0..c75bd9abdfb4d 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -78,16 +78,11 @@ func (api *API) templateACL(rw http.ResponseWriter, r *http.Request) { for _, group := range dbGroups { var members []database.User - if group.Name == database.AllUsersGroup { - members, err = api.Database.GetAllOrganizationMembers(ctx, group.OrganizationID) - } else { - members, err = api.Database.GetGroupMembers(ctx, group.ID) - } + members, err = api.Database.GetGroupMembers(ctx, group.ID) if err != nil { httpapi.InternalServerError(rw, err) return } - groups = append(groups, codersdk.TemplateGroup{ Group: convertGroup(group.Group, members), Role: convertToTemplateRole(group.Actions), diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index 725c6cc5eb621..1e591fcd30b79 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -66,7 +66,7 @@ func TestTemplateACL(t *testing.T) { require.Contains(t, acl.Users, templateUser3) }) - t.Run("allUsersGroup", func(t *testing.T) { + t.Run("everyoneGroup", func(t *testing.T) { t.Parallel() client := coderdenttest.New(t, nil) user := coderdtest.CreateFirstUser(t, client) @@ -76,7 +76,8 @@ func TestTemplateACL(t *testing.T) { }, }) - _, user1 := coderdtest.CreateAnotherUserWithUser(t, client, user.OrganizationID) + // Create a user to assert they aren't returned in the response. + _, _ = coderdtest.CreateAnotherUserWithUser(t, client, user.OrganizationID) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) @@ -87,8 +88,8 @@ func TestTemplateACL(t *testing.T) { require.NoError(t, err) require.Len(t, acl.Groups, 1) - require.Len(t, acl.Groups[0].Members, 2) - require.Contains(t, acl.Groups[0].Members, user1) + // We don't return members for the 'Everyone' group. + require.Len(t, acl.Groups[0].Members, 0) require.Len(t, acl.Users, 0) }) From d1468cf4c9b095141f74fdc094bba3af987ebd99 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Tue, 31 Jan 2023 19:21:23 +0000 Subject: [PATCH 2/2] rm fake method --- coderd/database/databasefake/databasefake.go | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index b4ca053b0cc73..5ca9d856f617f 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -4051,24 +4051,6 @@ func (q *fakeQuerier) GetGroupsByOrganizationID(_ context.Context, organizationI return groups, nil } -func (q *fakeQuerier) GetAllOrganizationMembers(_ context.Context, organizationID uuid.UUID) ([]database.User, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - - var users []database.User - for _, member := range q.organizationMembers { - if member.OrganizationID == organizationID { - for _, user := range q.users { - if user.ID == member.UserID { - users = append(users, user) - } - } - } - } - - return users, nil -} - func (q *fakeQuerier) DeleteGroupByID(_ context.Context, id uuid.UUID) error { q.mutex.Lock() defer q.mutex.Unlock()