Skip to content

Commit de4fb8a

Browse files
SasSwartSasSwart
committed
ensure that system users are filtered and returned consistently
1 parent 896c881 commit de4fb8a

19 files changed

+227
-75
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1747,15 +1747,15 @@ func (q *querier) GetGroupMembers(ctx context.Context) ([]database.GroupMember,
17471747
return q.db.GetGroupMembers(ctx)
17481748
}
17491749

1750-
func (q *querier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) {
1751-
return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, id)
1750+
func (q *querier) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) {
1751+
return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupMembersByGroupID)(ctx, arg)
17521752
}
17531753

1754-
func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) {
1755-
if _, err := q.GetGroupByID(ctx, groupID); err != nil { // AuthZ check
1754+
func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) {
1755+
if _, err := q.GetGroupByID(ctx, arg.GroupID); err != nil { // AuthZ check
17561756
return 0, err
17571757
}
1758-
memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, groupID)
1758+
memberCount, err := q.db.GetGroupMembersCountByGroupID(ctx, arg)
17591759
if err != nil {
17601760
return 0, err
17611761
}

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -387,12 +387,18 @@ func (s *MethodTestSuite) TestGroup() {
387387
g := dbgen.Group(s.T(), db, database.Group{})
388388
u := dbgen.User(s.T(), db, database.User{})
389389
gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
390-
check.Args(g.ID).Asserts(gm, policy.ActionRead)
390+
check.Args(database.GetGroupMembersByGroupIDParams{
391+
GroupID: g.ID,
392+
IncludeSystem: false,
393+
}).Asserts(gm, policy.ActionRead)
391394
}))
392395
s.Run("GetGroupMembersCountByGroupID", s.Subtest(func(db database.Store, check *expects) {
393396
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
394397
g := dbgen.Group(s.T(), db, database.Group{})
395-
check.Args(g.ID).Asserts(g, policy.ActionRead)
398+
check.Args(database.GetGroupMembersCountByGroupIDParams{
399+
GroupID: g.ID,
400+
IncludeSystem: false,
401+
}).Asserts(g, policy.ActionRead)
396402
}))
397403
s.Run("GetGroupMembers", s.Subtest(func(db database.Store, check *expects) {
398404
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)

‎coderd/database/dbauthz/groupsauth_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,10 @@ func TestGroupsAuth(t *testing.T) {
147147
require.Error(t, err, "group read")
148148
}
149149

150-
members, err := db.GetGroupMembersByGroupID(actorCtx, group.ID)
150+
members, err := db.GetGroupMembersByGroupID(actorCtx, database.GetGroupMembersByGroupIDParams{
151+
GroupID: group.ID,
152+
IncludeSystem: false,
153+
})
151154
if tc.ReadMembers {
152155
require.NoError(t, err, "member read")
153156
require.Len(t, members, tc.MembersExpected, "member count found does not match")

‎coderd/database/dbgen/dbgen_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,10 @@ func TestGenerator(t *testing.T) {
105105
gm := dbgen.GroupMember(t, db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
106106
exp := []database.GroupMember{gm}
107107

108-
require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), g.ID)))
108+
require.Equal(t, exp, must(db.GetGroupMembersByGroupID(context.Background(), database.GetGroupMembersByGroupIDParams{
109+
GroupID: g.ID,
110+
IncludeSystem: false,
111+
})))
109112
})
110113

111114
t.Run("Organization", func(t *testing.T) {

‎coderd/database/dbmem/dbmem.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3427,17 +3427,17 @@ func (q *FakeQuerier) GetGroupMembers(ctx context.Context) ([]database.GroupMemb
34273427
return groupMembers, nil
34283428
}
34293429

3430-
func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID) ([]database.GroupMember, error) {
3430+
func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, arg database.GetGroupMembersByGroupIDParams) ([]database.GroupMember, error) {
34313431
q.mutex.RLock()
34323432
defer q.mutex.RUnlock()
34333433

3434-
if q.isEveryoneGroup(id) {
3435-
return q.getEveryoneGroupMembersNoLock(ctx, id), nil
3434+
if q.isEveryoneGroup(arg.GroupID) {
3435+
return q.getEveryoneGroupMembersNoLock(ctx, arg.GroupID), nil
34363436
}
34373437

34383438
var groupMembers []database.GroupMember
34393439
for _, member := range q.groupMembers {
3440-
if member.GroupID == id {
3440+
if member.GroupID == arg.GroupID {
34413441
groupMember, err := q.getGroupMemberNoLock(ctx, member.UserID, member.GroupID)
34423442
if errors.Is(err, errUserDeleted) {
34433443
continue
@@ -3452,8 +3452,11 @@ func (q *FakeQuerier) GetGroupMembersByGroupID(ctx context.Context, id uuid.UUID
34523452
return groupMembers, nil
34533453
}
34543454

3455-
func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error) {
3456-
users, err := q.GetGroupMembersByGroupID(ctx, groupID)
3455+
func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, arg database.GetGroupMembersCountByGroupIDParams) (int64, error) {
3456+
users, err := q.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{
3457+
GroupID: arg.GroupID,
3458+
IncludeSystem: arg.IncludeSystem,
3459+
})
34573460
if err != nil {
34583461
return 0, err
34593462
}

‎coderd/database/dbmetrics/querymetrics.go

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

‎coderd/database/dbmock/dbmock.go

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

‎coderd/database/dump.sql

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

‎coderd/database/migrations/000302_system_user.down.sql

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,39 @@
1+
DROP VIEW IF EXISTS group_members_expanded;
2+
CREATE VIEW group_members_expanded AS
3+
WITH all_members AS (
4+
SELECT group_members.user_id,
5+
group_members.group_id
6+
FROM group_members
7+
UNION
8+
SELECT organization_members.user_id,
9+
organization_members.organization_id AS group_id
10+
FROM organization_members
11+
)
12+
SELECT users.id AS user_id,
13+
users.email AS user_email,
14+
users.username AS user_username,
15+
users.hashed_password AS user_hashed_password,
16+
users.created_at AS user_created_at,
17+
users.updated_at AS user_updated_at,
18+
users.status AS user_status,
19+
users.rbac_roles AS user_rbac_roles,
20+
users.login_type AS user_login_type,
21+
users.avatar_url AS user_avatar_url,
22+
users.deleted AS user_deleted,
23+
users.last_seen_at AS user_last_seen_at,
24+
users.quiet_hours_schedule AS user_quiet_hours_schedule,
25+
users.name AS user_name,
26+
users.github_com_user_id AS user_github_com_user_id,
27+
groups.organization_id,
28+
groups.name AS group_name,
29+
all_members.group_id
30+
FROM ((all_members
31+
JOIN users ON ((users.id = all_members.user_id)))
32+
JOIN groups ON ((groups.id = all_members.group_id)))
33+
WHERE (users.deleted = false);
34+
35+
COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).';
36+
137
-- Remove system user from organizations
238
DELETE FROM organization_members
339
WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0';

‎coderd/database/migrations/000302_system_user.up.sql

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,9 @@ CREATE INDEX user_is_system_idx ON users USING btree (is_system);
55

66
COMMENT ON COLUMN users.is_system IS 'Determines if a user is a system user, and therefore cannot login or perform normal actions';
77

8-
-- TODO: tried using "none" for login type, but the migration produced this error: 'unsafe use of new value "none" of enum type login_type'
9-
-- -> not sure why though? it exists on the login_type enum.
108
INSERT INTO users (id, email, username, name, created_at, updated_at, status, rbac_roles, hashed_password, is_system, login_type)
119
VALUES ('c42fdf75-3097-471c-8c33-fb52454d81c0', 'prebuilds@system', 'prebuilds', 'Prebuilds Owner', now(), now(),
12-
'active', '{}', 'none', true, 'password'::login_type);
10+
'active', '{}', 'none', true, 'none'::login_type);
1311

1412
-- Create function to check system user modifications
1513
CREATE OR REPLACE FUNCTION prevent_system_user_changes()
@@ -37,6 +35,42 @@ CREATE TRIGGER prevent_system_user_deletions
3735
WHEN (OLD.is_system = true)
3836
EXECUTE FUNCTION prevent_system_user_changes();
3937

38+
DROP VIEW IF EXISTS group_members_expanded;
39+
CREATE VIEW group_members_expanded AS
40+
WITH all_members AS (
41+
SELECT group_members.user_id,
42+
group_members.group_id
43+
FROM group_members
44+
UNION
45+
SELECT organization_members.user_id,
46+
organization_members.organization_id AS group_id
47+
FROM organization_members
48+
)
49+
SELECT users.id AS user_id,
50+
users.email AS user_email,
51+
users.username AS user_username,
52+
users.hashed_password AS user_hashed_password,
53+
users.created_at AS user_created_at,
54+
users.updated_at AS user_updated_at,
55+
users.status AS user_status,
56+
users.rbac_roles AS user_rbac_roles,
57+
users.login_type AS user_login_type,
58+
users.avatar_url AS user_avatar_url,
59+
users.deleted AS user_deleted,
60+
users.last_seen_at AS user_last_seen_at,
61+
users.quiet_hours_schedule AS user_quiet_hours_schedule,
62+
users.name AS user_name,
63+
users.github_com_user_id AS user_github_com_user_id,
64+
users.is_system AS user_is_system,
65+
groups.organization_id,
66+
groups.name AS group_name,
67+
all_members.group_id
68+
FROM ((all_members
69+
JOIN users ON ((users.id = all_members.user_id)))
70+
JOIN groups ON ((groups.id = all_members.group_id)))
71+
WHERE (users.deleted = false);
72+
73+
COMMENT ON VIEW group_members_expanded IS 'Joins group members with user information, organization ID, group name. Includes both regular group members and organization members (as part of the "Everyone" group).';
4074
-- TODO: do we *want* to use the default org here? how do we handle multi-org?
4175
WITH default_org AS (SELECT id
4276
FROM organizations

0 commit comments

Comments
 (0)