feat: remove user from groups on org membership delete

Emyrk · Emyrk · commit 12dfd05127d3 · 2024-09-17T10:05:53.000-05:00
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.
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -1922,7 +1922,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
@@ -1937,6 +1937,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/000251_group_member_trigger.down.sql b/coderd/database/migrations/000251_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/000251_group_member_trigger.up.sql b/coderd/database/migrations/000251_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/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 @@`
	`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;`