Skip to content

Commit 6f9b1a3

Browse files
hugodutkahugodutka
authored
fix: allow group members to read group information (coder#14200)
* - allow group members to read basic Group info - allow group members to see they are part of the group, but not see that information about other members - add a GetGroupMembersCountByGroupID SQL query, which allows group members to see members count without revealing other information about the members - add the group_members_expanded db view - rewrite group member queries to use the group_members_expanded view - add the RBAC ResourceGroupMember and add it to relevant roles - rewrite GetGroupMembersByGroupID permission checks - make the GroupMember type contain all user fields - fix type issues coming from replacing User with GroupMember in group member queries - add the MemberTotalCount field to codersdk.Group - display `group.total_member_count` instead of `group.members.length` on the account page
1 parent 60218c4 commit 6f9b1a3

38 files changed

+734
-315
lines changed

‎coderd/apidoc/docs.go

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/db2sdk/db2sdk.go

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,35 @@ func ReducedUser(user database.User) codersdk.ReducedUser {
159159
}
160160
}
161161

162+
func UserFromGroupMember(member database.GroupMember) database.User {
163+
return database.User{
164+
ID: member.UserID,
165+
Email: member.UserEmail,
166+
Username: member.UserUsername,
167+
HashedPassword: member.UserHashedPassword,
168+
CreatedAt: member.UserCreatedAt,
169+
UpdatedAt: member.UserUpdatedAt,
170+
Status: member.UserStatus,
171+
RBACRoles: member.UserRbacRoles,
172+
LoginType: member.UserLoginType,
173+
AvatarURL: member.UserAvatarUrl,
174+
Deleted: member.UserDeleted,
175+
LastSeenAt: member.UserLastSeenAt,
176+
QuietHoursSchedule: member.UserQuietHoursSchedule,
177+
ThemePreference: member.UserThemePreference,
178+
Name: member.UserName,
179+
GithubComUserID: member.UserGithubComUserID,
180+
}
181+
}
182+
183+
func ReducedUserFromGroupMember(member database.GroupMember) codersdk.ReducedUser {
184+
return ReducedUser(UserFromGroupMember(member))
185+
}
186+
187+
func ReducedUsersFromGroupMembers(members []database.GroupMember) []codersdk.ReducedUser {
188+
return List(members, ReducedUserFromGroupMember)
189+
}
190+
162191
func ReducedUsers(users []database.User) []codersdk.ReducedUser {
163192
return List(users, ReducedUser)
164193
}
@@ -179,16 +208,17 @@ func Users(users []database.User, organizationIDs map[uuid.UUID][]uuid.UUID) []c
179208
})
180209
}
181210

182-
func Group(group database.Group, members []database.User) codersdk.Group {
211+
func Group(group database.Group, members []database.GroupMember, totalMemberCount int) codersdk.Group {
183212
return codersdk.Group{
184-
ID: group.ID,
185-
Name: group.Name,
186-
DisplayName: group.DisplayName,
187-
OrganizationID: group.OrganizationID,
188-
AvatarURL: group.AvatarURL,
189-
Members: ReducedUsers(members),
190-
QuotaAllowance: int(group.QuotaAllowance),
191-
Source: codersdk.GroupSource(group.Source),
213+
ID: group.ID,
214+
Name: group.Name,
215+
DisplayName: group.DisplayName,
216+
OrganizationID: group.OrganizationID,
217+
AvatarURL: group.AvatarURL,
218+
Members: ReducedUsersFromGroupMembers(members),
219+
TotalMemberCount: totalMemberCount,
220+
QuotaAllowance: int(group.QuotaAllowance),
221+
Source: codersdk.GroupSource(group.Source),
192222
}
193223
}
194224

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1396,11 +1396,19 @@ func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember,
13961396
return q.db.GetGroupMembers(ctx)
13971397
}
13981398

1399-
func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.User, error) {
1400-
if _, err := q.GetGroupByID(ctx, id); err != nil { // AuthZ check
1401-
return nil, err
1399+
func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) {
1400+
return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id)
1401+
}
1402+
1403+
func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) {
1404+
if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check
1405+
return 0, err
1406+
}
1407+
memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID)
1408+
if err != nil {
1409+
return 0, err
14021410
}
1403-
return q.db.GetGroupMembersByGroupID(ctx, id)
1411+
return memberCount, nil
14041412
}
14051413

14061414
func (q *querier) GetGroups(ctx context.Context) ([]database.Group, error) {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -305,8 +305,10 @@ func (s *MethodTestSuite) TestGroup() {
305305
}))
306306
s.Run("DeleteGroupMemberFromGroup", s.Subtest(func(db database.Store, check *expects) {
307307
g := dbgen.Group(s.T(), db, database.Group{})
308-
m := dbgen.GroupMember(s.T(), db, database.GroupMember{
308+
u := dbgen.User(s.T(), db, database.User{})
309+
m := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{
309310
GroupID: g.ID,
311+
UserID: u.ID,
310312
})
311313
check.Args(database.DeleteGroupMemberFromGroupParams{
312314
UserID: m.UserID,
@@ -326,11 +328,18 @@ func (s *MethodTestSuite) TestGroup() {
326328
}))
327329
s.Run("GetGroupMembersByGroupID", s.Subtest(func(db database.Store, check *expects) {
328330
g := dbgen.Group(s.T(), db, database.Group{})
329-
_ = dbgen.GroupMember(s.T(), db, database.GroupMember{})
331+
u := dbgen.User(s.T(), db, database.User{})
332+
gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
333+
check.Args(g.ID).Asserts(gm, policy.ActionRead)
334+
}))
335+
s.Run("GetGroupMembersCountByGroupID", s.Subtest(func(db database.Store, check *expects) {
336+
g := dbgen.Group(s.T(), db, database.Group{})
330337
check.Args(g.ID).Asserts(g, policy.ActionRead)
331338
}))
332339
s.Run("GetGroupMembers", s.Subtest(func(db database.Store, check *expects) {
333-
_ = dbgen.GroupMember(s.T(), db, database.GroupMember{})
340+
g := dbgen.Group(s.T(), db, database.Group{})
341+
u := dbgen.User(s.T(), db, database.User{})
342+
dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
334343
check.Asserts(rbac.ResourceSystem, policy.ActionRead)
335344
}))
336345
s.Run("GetGroups", s.Subtest(func(db database.Store, check *expects) {
@@ -339,7 +348,8 @@ func (s *MethodTestSuite) TestGroup() {
339348
}))
340349
s.Run("GetGroupsByOrganizationAndUserID", s.Subtest(func(db database.Store, check *expects) {
341350
g := dbgen.Group(s.T(), db, database.Group{})
342-
gm := dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g.ID})
351+
u := dbgen.User(s.T(), db, database.User{})
352+
gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
343353
check.Args(database.GetGroupsByOrganizationAndUserIDParams{
344354
OrganizationID: g.OrganizationID,
345355
UserID: gm.UserID,
@@ -368,7 +378,7 @@ func (s *MethodTestSuite) TestGroup() {
368378
u1 := dbgen.User(s.T(), db, database.User{})
369379
g1 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
370380
g2 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
371-
_ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g1.ID, UserID: u1.ID})
381+
_ = dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g1.ID, UserID: u1.ID})
372382
check.Args(database.InsertUserGroupsByNameParams{
373383
OrganizationID: o.ID,
374384
UserID: u1.ID,
@@ -380,8 +390,8 @@ func (s *MethodTestSuite) TestGroup() {
380390
u1 := dbgen.User(s.T(), db, database.User{})
381391
g1 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
382392
g2 := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
383-
_ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g1.ID, UserID: u1.ID})
384-
_ = dbgen.GroupMember(s.T(), db, database.GroupMember{GroupID: g2.ID, UserID: u1.ID})
393+
_ = dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g1.ID, UserID: u1.ID})
394+
_ = dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g2.ID, UserID: u1.ID})
385395
check.Args(u1.ID).Asserts(rbac.ResourceSystem, policy.ActionUpdate).Returns()
386396
}))
387397
s.Run("UpdateGroupByID", s.Subtest(func(db database.Store, check *expects) {

‎coderd/database/dbauthz/groupsauth_test.go

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -115,18 +115,15 @@ func TestGroupsAuth(t *testing.T) {
115115
Name: "GroupMember",
116116
Subject: rbac.Subject{
117117
ID: users[0].ID.String(),
118-
Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.ScopedRoleOrgMember(org.ID)}.Expand())),
118+
Roles: rbac.Roles(must(rbac.RoleIdentifiers{rbac.RoleMember(), rbac.ScopedRoleOrgMember(org.ID)}.Expand())),
119119
Groups: []string{
120-
group.Name,
120+
group.ID.String(),
121121
},
122122
Scope: rbac.ExpandableScope(rbac.ScopeAll),
123123
},
124-
// TODO: currently group members cannot see their own groups.
125-
// If this is fixed, these booleans should be flipped to true.
126-
ReadGroup: false,
127-
ReadMembers: false,
128-
// TODO: If fixed, they should only be able to see themselves
129-
// MembersExpected: 1,
124+
ReadGroup: true,
125+
ReadMembers: true,
126+
MembersExpected: 1,
130127
},
131128
{
132129
// Org admin in the incorrect organization
@@ -160,8 +157,7 @@ func TestGroupsAuth(t *testing.T) {
160157
require.NoError(t, err, "member read")
161158
require.Len(t, members, tc.MembersExpected, "member count found does not match")
162159
} else {
163-
require.Error(t, err, "member read")
164-
require.True(t, dbauthz.IsNotAuthorizedError(err), "not authorized error")
160+
require.Len(t, members, 0, "member count is not 0")
165161
}
166162
})
167163
}

‎coderd/database/dbgen/dbgen.go

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"database/sql"
77
"encoding/hex"
88
"encoding/json"
9+
"errors"
910
"fmt"
1011
"net"
1112
"strings"
@@ -374,18 +375,61 @@ func Group(t testing.TB, db database.Store, orig database.Group) database.Group
374375
return group
375376
}
376377

377-
func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) database.GroupMember {
378-
member := database.GroupMember{
379-
UserID: takeFirst(orig.UserID, uuid.New()),
380-
GroupID: takeFirst(orig.GroupID, uuid.New()),
381-
}
378+
// GroupMember requires a user + group to already exist.
379+
// Example for creating a group member for a random group + user.
380+
//
381+
// GroupMember(t, db, database.GroupMemberTable{
382+
// UserID: User(t, db, database.User{}).ID,
383+
// GroupID: Group(t, db, database.Group{
384+
// OrganizationID: must(db.GetDefaultOrganization(genCtx)).ID,
385+
// }).ID,
386+
// })
387+
func GroupMember(t testing.TB, db database.Store, member database.GroupMemberTable) database.GroupMember {
388+
require.NotEqual(t, member.UserID, uuid.Nil, "A user id is required to use 'dbgen.GroupMember', use 'dbgen.User'.")
389+
require.NotEqual(t, member.GroupID, uuid.Nil, "A group id is required to use 'dbgen.GroupMember', use 'dbgen.Group'.")
390+
382391
//nolint:gosimple
383392
err := db.InsertGroupMember(genCtx, database.InsertGroupMemberParams{
384393
UserID: member.UserID,
385394
GroupID: member.GroupID,
386395
})
387396
require.NoError(t, err, "insert group member")
388-
return member
397+
398+
user, err := db.GetUserByID(genCtx, member.UserID)
399+
if errors.Is(err, sql.ErrNoRows) {
400+
require.NoErrorf(t, err, "'dbgen.GroupMember' failed as the user with id %s does not exist. A user is required to use this function, use 'dbgen.User'.", member.UserID)
401+
}
402+
require.NoError(t, err, "get user by id")
403+
404+
group, err := db.GetGroupByID(genCtx, member.GroupID)
405+
if errors.Is(err, sql.ErrNoRows) {
406+
require.NoErrorf(t, err, "'dbgen.GroupMember' failed as the group with id %s does not exist. A group is required to use this function, use 'dbgen.Group'.", member.GroupID)
407+
}
408+
require.NoError(t, err, "get group by id")
409+
410+
groupMember := database.GroupMember{
411+
UserID: user.ID,
412+
UserEmail: user.Email,
413+
UserUsername: user.Username,
414+
UserHashedPassword: user.HashedPassword,
415+
UserCreatedAt: user.CreatedAt,
416+
UserUpdatedAt: user.UpdatedAt,
417+
UserStatus: user.Status,
418+
UserRbacRoles: user.RBACRoles,
419+
UserLoginType: user.LoginType,
420+
UserAvatarUrl: user.AvatarURL,
421+
UserDeleted: user.Deleted,
422+
UserLastSeenAt: user.LastSeenAt,
423+
UserQuietHoursSchedule: user.QuietHoursSchedule,
424+
UserThemePreference: user.ThemePreference,
425+
UserName: user.Name,
426+
UserGithubComUserID: user.GithubComUserID,
427+
OrganizationID: group.OrganizationID,
428+
GroupName: group.Name,
429+
GroupID: group.ID,
430+
}
431+
432+
return groupMember
389433
}
390434

391435
// ProvisionerJob is a bit more involved to get the values such as "completedAt", "startedAt", "cancelledAt" set. ps

‎coderd/database/dbgen/dbgen_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,8 @@ func TestGenerator(t *testing.T) {
102102
db := dbmem.New()
103103
g := dbgen.Group(t, db, database.Group{})
104104
u := dbgen.User(t, db, database.User{})
105-
exp := []database.User{u}
106-
dbgen.GroupMember(t, db, database.GroupMember{GroupID: g.ID, UserID: u.ID})
105+
gm := dbgen.GroupMember(t, db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
106+
exp := []database.GroupMember{gm}
107107

108108
require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), g.ID)))
109109
})

0 commit comments

Comments
 (0)