From 62c3dd93707ac937e9d781699efc2d47015c538a Mon Sep 17 00:00:00 2001 From: Jamil Date: Sat, 14 Jun 2025 08:20:32 -0700 Subject: [PATCH] fix(portal): don't add service accounts to everyone group (#9530) In #9513 a bug was introduced that added all service accounts to the Everyone group. This fixes that by ensuring the `insert_all` query only cross joins where actor type is `:account_user, :account_admin_user`. Staging data will be manually fixed after this goes in. I briefly considered updating the delete clause of this query to "clean things up" by removing any found service accounts but that is a bit too defensive in my opinion - if there's no way a service account should make it into this group, then we shouldn't have code to expect it. This will all be going away in #8750 which should be much less brittle. --- .../domain/lib/domain/actors/group/query.ex | 5 +-- .../apps/domain/test/domain/actors_test.exs | 36 +++++++++++++------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/elixir/apps/domain/lib/domain/actors/group/query.ex b/elixir/apps/domain/lib/domain/actors/group/query.ex index 795214045..9b2446a2e 100644 --- a/elixir/apps/domain/lib/domain/actors/group/query.ex +++ b/elixir/apps/domain/lib/domain/actors/group/query.ex @@ -142,7 +142,7 @@ defmodule Domain.Actors.Group.Query do end # TODO: IDP Sync - # Remove this in favor of special case for everyone group + # See: https://github.com/firezone/firezone/issues/8750 # We use CTE here which should be very performant even for very large inserts and deletions def update_everyone_group_memberships(account_id) do # Delete memberships for actors and groups that are soft-deleted @@ -164,7 +164,7 @@ defmodule Domain.Actors.Group.Query do ) |> from(as: :agm) - # Insert memberships for the cross join of non-deleted actors and managed groups + # Insert memberships for the cross join of non-deleted user actors and managed groups insert_with_cte_fn = fn repo, _changes -> current_memberships_cte = from( @@ -173,6 +173,7 @@ defmodule Domain.Actors.Group.Query do where: is_nil(a.deleted_at) and a.account_id == ^account_id and + a.type in [:account_user, :account_admin_user] and g.type == :managed and g.account_id == ^account_id and is_nil(g.deleted_at), diff --git a/elixir/apps/domain/test/domain/actors_test.exs b/elixir/apps/domain/test/domain/actors_test.exs index 91f672642..ff5878237 100644 --- a/elixir/apps/domain/test/domain/actors_test.exs +++ b/elixir/apps/domain/test/domain/actors_test.exs @@ -1769,6 +1769,20 @@ defmodule Domain.ActorsTest do } end + test "doesn't include service accounts", %{ + account: account + } do + service_account = + Fixtures.Actors.create_actor(type: :service_account, account: account) + + Fixtures.Actors.create_managed_group(account: account, name: "Managed Group") + + refute Enum.any?( + Repo.all(Actors.Membership), + &(&1.actor_id == service_account.id) + ) + end + test "doesn't affect non-managed group memberships", %{ account: account, subject: subject, @@ -1843,7 +1857,7 @@ defmodule Domain.ActorsTest do } do # Create additional actors actor2 = Fixtures.Actors.create_actor(type: :account_user, account: account) - actor3 = Fixtures.Actors.create_actor(type: :service_account, account: account) + _actor3 = Fixtures.Actors.create_actor(type: :service_account, account: account) # Create multiple managed groups managed_group1 = @@ -1853,11 +1867,11 @@ defmodule Domain.ActorsTest do Fixtures.Actors.create_managed_group(account: account, name: "Managed Group 2") memberships = Repo.all(Actors.Membership) - # 3 actors × 2 managed groups - assert length(memberships) == 6 + # 2 user actors × 2 managed groups + assert length(memberships) == 4 - # Verify each actor is in each managed group - actor_ids = [actor1.id, actor2.id, actor3.id] + # Verify each user actor is in each managed group + actor_ids = [actor1.id, actor2.id] group_ids = [managed_group1.id, managed_group2.id] for actor_id <- actor_ids, group_id <- group_ids do @@ -1965,24 +1979,24 @@ defmodule Domain.ActorsTest do subject: subject } do actor2 = Fixtures.Actors.create_actor(type: :account_user, account: account) - actor3 = Fixtures.Actors.create_actor(type: :service_account, account: account) + _actor3 = Fixtures.Actors.create_actor(type: :service_account, account: account) managed_group1 = Fixtures.Actors.create_managed_group(account: account, name: "Group 1") managed_group2 = Fixtures.Actors.create_managed_group(account: account, name: "Group 2") - # 3 actors × 2 groups - assert length(Repo.all(Actors.Membership)) == 6 + # 2 user actors × 2 groups + assert length(Repo.all(Actors.Membership)) == 4 # Delete one actor and one group {:ok, _} = Actors.delete_actor(actor2, subject) {:ok, _} = Actors.delete_group(managed_group2, subject) memberships = Repo.all(Actors.Membership) - # 2 remaining actors × 1 remaining group - assert length(memberships) == 2 + # 1 remaining user actor × 1 remaining group + assert length(memberships) == 1 # Verify correct memberships remain - remaining_actor_ids = [actor1.id, actor3.id] + remaining_actor_ids = [actor1.id] assert Enum.all?(memberships, fn m -> m.actor_id in remaining_actor_ids && m.group_id == managed_group1.id