Skip to content

Commit 1e5438e

Browse files
EmyrkEmyrk
authored
feat: remove user from groups on org membership delete (coder#14701)
* feat: remove user from groups on org membership delete Groups inherently provide authz access to certain resources. If a user is removed from an organization, they should be removed from all their groups in said organization.
1 parent c145f11 commit 1e5438e

File tree

6 files changed

+196
-1
lines changed

6 files changed

+196
-1
lines changed

‎coderd/database/dbmem/dbmem.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1944,7 +1944,7 @@ func (q *FakeQuerier) DeleteOrganization(_ context.Context, id uuid.UUID) error
19441944
return sql.ErrNoRows
19451945
}
19461946

1947-
func (q *FakeQuerier) DeleteOrganizationMember(_ context.Context, arg database.DeleteOrganizationMemberParams) error {
1947+
func (q *FakeQuerier) DeleteOrganizationMember(ctx context.Context, arg database.DeleteOrganizationMemberParams) error {
19481948
err := validateDatabaseType(arg)
19491949
if err != nil {
19501950
return err
@@ -1959,6 +1959,16 @@ func (q *FakeQuerier) DeleteOrganizationMember(_ context.Context, arg database.D
19591959
if len(deleted) == 0 {
19601960
return sql.ErrNoRows
19611961
}
1962+
1963+
// Delete group member trigger
1964+
q.groupMembers = slices.DeleteFunc(q.groupMembers, func(member database.GroupMemberTable) bool {
1965+
if member.UserID != arg.UserID {
1966+
return false
1967+
}
1968+
g, _ := q.getGroupByIDNoLock(ctx, member.GroupID)
1969+
return g.OrganizationID == arg.OrganizationID
1970+
})
1971+
19621972
return nil
19631973
}
19641974

‎coderd/database/dump.sql

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

‎coderd/database/migrations/000252_group_member_trigger.down.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
DROP TRIGGER IF EXISTS trigger_delete_group_members_on_org_member_delete ON organization_members;
2+
DROP FUNCTION IF EXISTS delete_group_members_on_org_member_delete;

‎coderd/database/migrations/000252_group_member_trigger.up.sql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
CREATE FUNCTION delete_group_members_on_org_member_delete() RETURNS TRIGGER
2+
LANGUAGE plpgsql
3+
AS $$
4+
DECLARE
5+
BEGIN
6+
-- Remove the user from all groups associated with the same
7+
-- organization as the organization_member being deleted.
8+
DELETE FROM group_members
9+
WHERE
10+
user_id = OLD.user_id
11+
AND group_id IN (
12+
SELECT id
13+
FROM groups
14+
WHERE organization_id = OLD.organization_id
15+
);
16+
RETURN OLD;
17+
END;
18+
$$;
19+
20+
CREATE TRIGGER trigger_delete_group_members_on_org_member_delete
21+
BEFORE DELETE ON organization_members
22+
FOR EACH ROW
23+
EXECUTE PROCEDURE delete_group_members_on_org_member_delete();

‎coderd/database/querier_test.go

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,6 +1216,103 @@ func TestExpectOne(t *testing.T) {
12161216
})
12171217
}
12181218

1219+
func TestGroupRemovalTrigger(t *testing.T) {
1220+
t.Parallel()
1221+
1222+
db, _ := dbtestutil.NewDB(t)
1223+
1224+
orgA := dbgen.Organization(t, db, database.Organization{})
1225+
_, err := db.InsertAllUsersGroup(context.Background(), orgA.ID)
1226+
require.NoError(t, err)
1227+
1228+
orgB := dbgen.Organization(t, db, database.Organization{})
1229+
_, err = db.InsertAllUsersGroup(context.Background(), orgB.ID)
1230+
require.NoError(t, err)
1231+
1232+
orgs := []database.Organization{orgA, orgB}
1233+
1234+
user := dbgen.User(t, db, database.User{})
1235+
extra := dbgen.User(t, db, database.User{})
1236+
users := []database.User{user, extra}
1237+
1238+
groupA1 := dbgen.Group(t, db, database.Group{
1239+
OrganizationID: orgA.ID,
1240+
})
1241+
groupA2 := dbgen.Group(t, db, database.Group{
1242+
OrganizationID: orgA.ID,
1243+
})
1244+
1245+
groupB1 := dbgen.Group(t, db, database.Group{
1246+
OrganizationID: orgB.ID,
1247+
})
1248+
groupB2 := dbgen.Group(t, db, database.Group{
1249+
OrganizationID: orgB.ID,
1250+
})
1251+
1252+
groups := []database.Group{groupA1, groupA2, groupB1, groupB2}
1253+
1254+
// Add users to all organizations
1255+
for _, u := range users {
1256+
for _, o := range orgs {
1257+
dbgen.OrganizationMember(t, db, database.OrganizationMember{
1258+
OrganizationID: o.ID,
1259+
UserID: u.ID,
1260+
})
1261+
}
1262+
}
1263+
1264+
// Add users to all groups
1265+
for _, u := range users {
1266+
for _, g := range groups {
1267+
dbgen.GroupMember(t, db, database.GroupMemberTable{
1268+
GroupID: g.ID,
1269+
UserID: u.ID,
1270+
})
1271+
}
1272+
}
1273+
1274+
// Verify user is in all groups
1275+
ctx := testutil.Context(t, testutil.WaitLong)
1276+
onlyGroupIDs := func(row database.GetGroupsRow) uuid.UUID {
1277+
return row.Group.ID
1278+
}
1279+
userGroups, err := db.GetGroups(ctx, database.GetGroupsParams{
1280+
HasMemberID: user.ID,
1281+
})
1282+
require.NoError(t, err)
1283+
require.ElementsMatch(t, []uuid.UUID{
1284+
orgA.ID, orgB.ID, // Everyone groups
1285+
groupA1.ID, groupA2.ID, groupB1.ID, groupB2.ID, // Org groups
1286+
}, db2sdk.List(userGroups, onlyGroupIDs))
1287+
1288+
// Remove the user from org A
1289+
err = db.DeleteOrganizationMember(ctx, database.DeleteOrganizationMemberParams{
1290+
OrganizationID: orgA.ID,
1291+
UserID: user.ID,
1292+
})
1293+
require.NoError(t, err)
1294+
1295+
// Verify user is no longer in org A groups
1296+
userGroups, err = db.GetGroups(ctx, database.GetGroupsParams{
1297+
HasMemberID: user.ID,
1298+
})
1299+
require.NoError(t, err)
1300+
require.ElementsMatch(t, []uuid.UUID{
1301+
orgB.ID, // Everyone group
1302+
groupB1.ID, groupB2.ID, // Org groups
1303+
}, db2sdk.List(userGroups, onlyGroupIDs))
1304+
1305+
// Verify extra user is unchanged
1306+
extraUserGroups, err := db.GetGroups(ctx, database.GetGroupsParams{
1307+
HasMemberID: extra.ID,
1308+
})
1309+
require.NoError(t, err)
1310+
require.ElementsMatch(t, []uuid.UUID{
1311+
orgA.ID, orgB.ID, // Everyone groups
1312+
groupA1.ID, groupA2.ID, groupB1.ID, groupB2.ID, // Org groups
1313+
}, db2sdk.List(extraUserGroups, onlyGroupIDs))
1314+
}
1315+
12191316
func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) {
12201317
t.Helper()
12211318
require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg)

‎enterprise/members_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ func TestEnterpriseMembers(t *testing.T) {
2929
LicenseOptions: &coderdenttest.LicenseOptions{
3030
Features: license.Features{
3131
codersdk.FeatureMultipleOrganizations: 1,
32+
codersdk.FeatureTemplateRBAC: 1,
3233
},
3334
},
3435
})
@@ -39,6 +40,21 @@ func TestEnterpriseMembers(t *testing.T) {
3940
_, user := coderdtest.CreateAnotherUser(t, owner, secondOrg.ID)
4041

4142
ctx := testutil.Context(t, testutil.WaitMedium)
43+
44+
// Groups exist to ensure a user removed from the org loses their
45+
// group access.
46+
g1, err := orgAdminClient.CreateGroup(ctx, secondOrg.ID, codersdk.CreateGroupRequest{
47+
Name: "foo",
48+
DisplayName: "Foo",
49+
})
50+
require.NoError(t, err)
51+
52+
g2, err := orgAdminClient.CreateGroup(ctx, secondOrg.ID, codersdk.CreateGroupRequest{
53+
Name: "bar",
54+
DisplayName: "Bar",
55+
})
56+
require.NoError(t, err)
57+
4258
// Verify the org of 3 members
4359
members, err := orgAdminClient.OrganizationMembers(ctx, secondOrg.ID)
4460
require.NoError(t, err)
@@ -47,6 +63,25 @@ func TestEnterpriseMembers(t *testing.T) {
4763
[]uuid.UUID{first.UserID, user.ID, orgAdmin.ID},
4864
db2sdk.List(members, onlyIDs))
4965

66+
// Add the member to some groups
67+
_, err = orgAdminClient.PatchGroup(ctx, g1.ID, codersdk.PatchGroupRequest{
68+
AddUsers: []string{user.ID.String()},
69+
})
70+
require.NoError(t, err)
71+
72+
_, err = orgAdminClient.PatchGroup(ctx, g2.ID, codersdk.PatchGroupRequest{
73+
AddUsers: []string{user.ID.String()},
74+
})
75+
require.NoError(t, err)
76+
77+
// Verify group membership
78+
userGroups, err := orgAdminClient.Groups(ctx, codersdk.GroupArguments{
79+
HasMember: user.ID.String(),
80+
})
81+
require.NoError(t, err)
82+
// Everyone group + 2 groups
83+
require.Len(t, userGroups, 3)
84+
5085
// Delete a member
5186
err = orgAdminClient.DeleteOrganizationMember(ctx, secondOrg.ID, user.Username)
5287
require.NoError(t, err)
@@ -57,6 +92,13 @@ func TestEnterpriseMembers(t *testing.T) {
5792
require.ElementsMatch(t,
5893
[]uuid.UUID{first.UserID, orgAdmin.ID},
5994
db2sdk.List(members, onlyIDs))
95+
96+
// User should now belong to 0 groups
97+
userGroups, err = orgAdminClient.Groups(ctx, codersdk.GroupArguments{
98+
HasMember: user.ID.String(),
99+
})
100+
require.NoError(t, err)
101+
require.Len(t, userGroups, 0)
60102
})
61103

62104
t.Run("PostUser", func(t *testing.T) {

0 commit comments

Comments
 (0)