coder · Emyrk · Sep 18, 2024 · Sep 17, 2024 · Sep 17, 2024 · Sep 18, 2024
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1944,7 +1944,7 @@ func (q *FakeQuerier) DeleteOrganization(_ context.Context, id uuid.UUID) error
 	return sql.ErrNoRows
 }
 
-func (q *FakeQuerier) DeleteOrganizationMember(_ context.Context, arg database.DeleteOrganizationMemberParams) error {
+func (q *FakeQuerier) DeleteOrganizationMember(ctx context.Context, arg database.DeleteOrganizationMemberParams) error {
 	err := validateDatabaseType(arg)
 	if err != nil {
 		return err
@@ -1959,6 +1959,16 @@ func (q *FakeQuerier) DeleteOrganizationMember(_ context.Context, arg database.D
 	if len(deleted) == 0 {
 		return sql.ErrNoRows
 	}
+
+	// Delete group member trigger
+	q.groupMembers = slices.DeleteFunc(q.groupMembers, func(member database.GroupMemberTable) bool {
+		if member.UserID != arg.UserID {
+			return false
+		}
+		g, _ := q.getGroupByIDNoLock(ctx, member.GroupID)
+		return g.OrganizationID == arg.OrganizationID
+	})
+
 	return nil
 }
 

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000252_group_member_trigger.down.sql b/coderd/database/migrations/000252_group_member_trigger.down.sql
@@ -0,0 +1,2 @@
+DROP TRIGGER IF EXISTS trigger_delete_group_members_on_org_member_delete ON organization_members;
+DROP FUNCTION IF EXISTS delete_group_members_on_org_member_delete;
diff --git a/coderd/database/migrations/000252_group_member_trigger.up.sql b/coderd/database/migrations/000252_group_member_trigger.up.sql
@@ -0,0 +1,23 @@
+CREATE FUNCTION delete_group_members_on_org_member_delete() RETURNS TRIGGER
+	LANGUAGE plpgsql
+AS $$
+DECLARE
+BEGIN
+	-- Remove the user from all groups associated with the same
+	-- organization as the organization_member being deleted.
+	DELETE FROM group_members
+	WHERE
+		user_id = OLD.user_id
+		AND group_id IN (
+			SELECT id
+			FROM groups
+			WHERE organization_id = OLD.organization_id
+		);
+	RETURN OLD;
+END;
+$$;
+
+CREATE TRIGGER trigger_delete_group_members_on_org_member_delete
+	BEFORE DELETE ON organization_members
+	FOR EACH ROW
+EXECUTE PROCEDURE delete_group_members_on_org_member_delete();
diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go
@@ -1216,6 +1216,103 @@ func TestExpectOne(t *testing.T) {
 	})
 }
 
+func TestGroupRemovalTrigger(t *testing.T) {
+	t.Parallel()
+
+	db, _ := dbtestutil.NewDB(t)
+
+	orgA := dbgen.Organization(t, db, database.Organization{})
+	_, err := db.InsertAllUsersGroup(context.Background(), orgA.ID)
+	require.NoError(t, err)
+
+	orgB := dbgen.Organization(t, db, database.Organization{})
+	_, err = db.InsertAllUsersGroup(context.Background(), orgB.ID)
+	require.NoError(t, err)
+
+	orgs := []database.Organization{orgA, orgB}
+
+	user := dbgen.User(t, db, database.User{})
+	extra := dbgen.User(t, db, database.User{})
+	users := []database.User{user, extra}
+
+	groupA1 := dbgen.Group(t, db, database.Group{
+		OrganizationID: orgA.ID,
+	})
+	groupA2 := dbgen.Group(t, db, database.Group{
+		OrganizationID: orgA.ID,
+	})
+
+	groupB1 := dbgen.Group(t, db, database.Group{
+		OrganizationID: orgB.ID,
+	})
+	groupB2 := dbgen.Group(t, db, database.Group{
+		OrganizationID: orgB.ID,
+	})
+
+	groups := []database.Group{groupA1, groupA2, groupB1, groupB2}
+
+	// Add users to all organizations
+	for _, u := range users {
+		for _, o := range orgs {
+			dbgen.OrganizationMember(t, db, database.OrganizationMember{
+				OrganizationID: o.ID,
+				UserID:         u.ID,
+			})
+		}
+	}
+
+	// Add users to all groups
+	for _, u := range users {
+		for _, g := range groups {
+			dbgen.GroupMember(t, db, database.GroupMemberTable{
+				GroupID: g.ID,
+				UserID:  u.ID,
+			})
+		}
+	}
+
+	// Verify user is in all groups
+	ctx := testutil.Context(t, testutil.WaitLong)
+	onlyGroupIDs := func(row database.GetGroupsRow) uuid.UUID {
+		return row.Group.ID
+	}
+	userGroups, err := db.GetGroups(ctx, database.GetGroupsParams{
+		HasMemberID: user.ID,
+	})
+	require.NoError(t, err)
+	require.ElementsMatch(t, []uuid.UUID{
+		orgA.ID, orgB.ID, // Everyone groups
+		groupA1.ID, groupA2.ID, groupB1.ID, groupB2.ID, // Org groups
+	}, db2sdk.List(userGroups, onlyGroupIDs))
+
+	// Remove the user from org A
+	err = db.DeleteOrganizationMember(ctx, database.DeleteOrganizationMemberParams{
+		OrganizationID: orgA.ID,
+		UserID:         user.ID,
+	})
+	require.NoError(t, err)
+
+	// Verify user is no longer in org A groups
+	userGroups, err = db.GetGroups(ctx, database.GetGroupsParams{
+		HasMemberID: user.ID,
+	})
+	require.NoError(t, err)
+	require.ElementsMatch(t, []uuid.UUID{
+		orgB.ID,                // Everyone group
+		groupB1.ID, groupB2.ID, // Org groups
+	}, db2sdk.List(userGroups, onlyGroupIDs))
+
+	// Verify extra user is unchanged
+	extraUserGroups, err := db.GetGroups(ctx, database.GetGroupsParams{
+		HasMemberID: extra.ID,
+	})
+	require.NoError(t, err)
+	require.ElementsMatch(t, []uuid.UUID{
+		orgA.ID, orgB.ID, // Everyone groups
+		groupA1.ID, groupA2.ID, groupB1.ID, groupB2.ID, // Org groups
+	}, db2sdk.List(extraUserGroups, onlyGroupIDs))
+}
+
 func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) {
 	t.Helper()
 	require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg)

diff --git a/enterprise/members_test.go b/enterprise/members_test.go
@@ -29,6 +29,7 @@ func TestEnterpriseMembers(t *testing.T) {
 			LicenseOptions: &coderdenttest.LicenseOptions{
 				Features: license.Features{
 					codersdk.FeatureMultipleOrganizations: 1,
+					codersdk.FeatureTemplateRBAC:          1,
 				},
 			},
 		})
@@ -39,6 +40,21 @@ func TestEnterpriseMembers(t *testing.T) {
 		_, user := coderdtest.CreateAnotherUser(t, owner, secondOrg.ID)
 
 		ctx := testutil.Context(t, testutil.WaitMedium)
+
+		// Groups exist to ensure a user removed from the org loses their
+		// group access.
+		g1, err := orgAdminClient.CreateGroup(ctx, secondOrg.ID, codersdk.CreateGroupRequest{
+			Name:        "foo",
+			DisplayName: "Foo",
+		})
+		require.NoError(t, err)
+
+		g2, err := orgAdminClient.CreateGroup(ctx, secondOrg.ID, codersdk.CreateGroupRequest{
+			Name:        "bar",
+			DisplayName: "Bar",
+		})
+		require.NoError(t, err)
+
 		// Verify the org of 3 members
 		members, err := orgAdminClient.OrganizationMembers(ctx, secondOrg.ID)
 		require.NoError(t, err)
@@ -47,6 +63,25 @@ func TestEnterpriseMembers(t *testing.T) {
 			[]uuid.UUID{first.UserID, user.ID, orgAdmin.ID},
 			db2sdk.List(members, onlyIDs))
 
+		// Add the member to some groups
+		_, err = orgAdminClient.PatchGroup(ctx, g1.ID, codersdk.PatchGroupRequest{
+			AddUsers: []string{user.ID.String()},
+		})
+		require.NoError(t, err)
+
+		_, err = orgAdminClient.PatchGroup(ctx, g2.ID, codersdk.PatchGroupRequest{
+			AddUsers: []string{user.ID.String()},
+		})
+		require.NoError(t, err)
+
+		// Verify group membership
+		userGroups, err := orgAdminClient.Groups(ctx, codersdk.GroupArguments{
+			HasMember: user.ID.String(),
+		})
+		require.NoError(t, err)
+		// Everyone group + 2 groups
+		require.Len(t, userGroups, 3)
+
 		// Delete a member
 		err = orgAdminClient.DeleteOrganizationMember(ctx, secondOrg.ID, user.Username)
 		require.NoError(t, err)
@@ -57,6 +92,13 @@ func TestEnterpriseMembers(t *testing.T) {
 		require.ElementsMatch(t,
 			[]uuid.UUID{first.UserID, orgAdmin.ID},
 			db2sdk.List(members, onlyIDs))
+
+		// User should now belong to 0 groups
+		userGroups, err = orgAdminClient.Groups(ctx, codersdk.GroupArguments{
+			HasMember: user.ID.String(),
+		})
+		require.NoError(t, err)
+		require.Len(t, userGroups, 0)
 	})
 
 	t.Run("PostUser", func(t *testing.T) {
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		DROP TRIGGER IF EXISTS trigger_delete_group_members_on_org_member_delete ON organization_members;
		DROP FUNCTION IF EXISTS delete_group_members_on_org_member_delete;