From 6f87f5ea2c621f4387e00bef38e4feb435cad062 Mon Sep 17 00:00:00 2001 From: Jamil Date: Fri, 20 Jun 2025 13:40:31 -0700 Subject: [PATCH] fix(portal): use account_id in index for agm hook (#9606) When reacting to `ActorGroupMembership` updates, we were issuing a query to expire Flows given an `actor_id, actor_group_id` combination. Unfortunately, this query never included an `account_id` to scope it, causing a table scan of flows and associated join tables to resolve it. To fix this, we introduce the `account_id` and ensure the expire flows uses this field to ensure only data for an account is considered in the query. Related: https://firezone-inc.sentry.io/issues/6346235615/events/e225e1c488cb4ea3896649aabd529c50 --- .../lib/domain/events/hooks/actor_group_memberships.ex | 6 ++++-- elixir/apps/domain/lib/domain/flows.ex | 3 ++- elixir/apps/domain/test/domain/actors_test.exs | 2 ++ .../adapters/google_workspace/jobs/sync_directory_test.exs | 1 + .../auth/adapters/jumpcloud/jobs/sync_directory_test.exs | 1 + .../adapters/microsoft_entra/jobs/sync_directory_test.exs | 1 + .../domain/auth/adapters/okta/jobs/sync_directory_test.exs | 1 + .../domain/events/hooks/actor_group_memberships_test.exs | 2 ++ elixir/apps/domain/test/domain/flows_test.exs | 4 +++- 9 files changed, 17 insertions(+), 4 deletions(-) diff --git a/elixir/apps/domain/lib/domain/events/hooks/actor_group_memberships.ex b/elixir/apps/domain/lib/domain/events/hooks/actor_group_memberships.ex index 19c5e533d..61e2cdd7b 100644 --- a/elixir/apps/domain/lib/domain/events/hooks/actor_group_memberships.ex +++ b/elixir/apps/domain/lib/domain/events/hooks/actor_group_memberships.ex @@ -16,11 +16,13 @@ defmodule Domain.Events.Hooks.ActorGroupMemberships do def on_update(_old_data, _data), do: :ok @impl true - def on_delete(%{"actor_id" => actor_id, "group_id" => group_id} = _old_data) do + def on_delete( + %{"account_id" => account_id, "actor_id" => actor_id, "group_id" => group_id} = _old_data + ) do Task.start(fn -> :ok = PubSub.Actor.Memberships.broadcast(actor_id, {:delete_membership, actor_id, group_id}) broadcast_access(:reject, actor_id, group_id) - {:ok, _flows} = Flows.expire_flows_for(actor_id, group_id) + {:ok, _flows} = Flows.expire_flows_for(account_id, actor_id, group_id) end) :ok diff --git a/elixir/apps/domain/lib/domain/flows.ex b/elixir/apps/domain/lib/domain/flows.ex index 8d28dccce..b3f3bd464 100644 --- a/elixir/apps/domain/lib/domain/flows.ex +++ b/elixir/apps/domain/lib/domain/flows.ex @@ -219,8 +219,9 @@ defmodule Domain.Flows do |> expire_flows(subject) end - def expire_flows_for(actor_id, group_id) do + def expire_flows_for(account_id, actor_id, group_id) do Flow.Query.all() + |> Flow.Query.by_account_id(account_id) |> Flow.Query.by_actor_id(actor_id) |> Flow.Query.by_policy_actor_group_id(group_id) |> expire_flows() diff --git a/elixir/apps/domain/test/domain/actors_test.exs b/elixir/apps/domain/test/domain/actors_test.exs index 94e5762c4..c0545ede1 100644 --- a/elixir/apps/domain/test/domain/actors_test.exs +++ b/elixir/apps/domain/test/domain/actors_test.exs @@ -1237,11 +1237,13 @@ defmodule Domain.ActorsTest do # TODO: WAL # These tests will be made redundant soon Events.Hooks.ActorGroupMemberships.on_delete(%{ + "account_id" => account.id, "actor_id" => identity1.actor_id, "group_id" => group1.id }) Events.Hooks.ActorGroupMemberships.on_delete(%{ + "account_id" => account.id, "actor_id" => identity2.actor_id, "group_id" => group2.id }) diff --git a/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs index 45b8da4ac..e4f7b2804 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs @@ -733,6 +733,7 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace.Jobs.SyncDirectoryTest do # Simulate WAL events Events.Hooks.ActorGroupMemberships.on_delete(%{ + "account_id" => deleted_identity.account_id, "actor_id" => deleted_identity.actor_id, "group_id" => deleted_group.id }) diff --git a/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs index dcbfbab15..0823ecd86 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs @@ -497,6 +497,7 @@ defmodule Domain.Auth.Adapters.JumpCloud.Jobs.SyncDirectoryTest do # Simulate the WAL events Events.Hooks.ActorGroupMemberships.on_delete(%{ + "account_id" => deleted_group.account_id, "actor_id" => actor.id, "group_id" => deleted_group.id }) diff --git a/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs index 262ec3d84..3f3f32d8e 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs @@ -564,6 +564,7 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra.Jobs.SyncDirectoryTest do # Simulate WAL events Events.Hooks.ActorGroupMemberships.on_delete(%{ + "account_id" => deleted_identity.account_id, "actor_id" => deleted_identity.actor_id, "group_id" => deleted_group.id }) diff --git a/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs index 650ff5ddd..406146fba 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs @@ -798,6 +798,7 @@ defmodule Domain.Auth.Adapters.Okta.Jobs.SyncDirectoryTest do # Simulate WAL events Events.Hooks.ActorGroupMemberships.on_delete(%{ + "account_id" => deleted_membership.account_id, "actor_id" => actor.id, "group_id" => deleted_group.id }) diff --git a/elixir/apps/domain/test/domain/events/hooks/actor_group_memberships_test.exs b/elixir/apps/domain/test/domain/events/hooks/actor_group_memberships_test.exs index 6b6aeb746..1ed5b81dc 100644 --- a/elixir/apps/domain/test/domain/events/hooks/actor_group_memberships_test.exs +++ b/elixir/apps/domain/test/domain/events/hooks/actor_group_memberships_test.exs @@ -82,6 +82,7 @@ defmodule Domain.Events.Hooks.ActorGroupMembershipsTest do group_id = "#{Ecto.UUID.generate()}" data = %{ + "account_id" => "#{Ecto.UUID.generate()}", "actor_id" => actor_id, "group_id" => group_id } @@ -105,6 +106,7 @@ defmodule Domain.Events.Hooks.ActorGroupMembershipsTest do assert :ok = on_delete(%{ + "account_id" => actor.account_id, "actor_id" => actor.id, "group_id" => actor_group.id }) diff --git a/elixir/apps/domain/test/domain/flows_test.exs b/elixir/apps/domain/test/domain/flows_test.exs index c4927b2e6..83182d9c9 100644 --- a/elixir/apps/domain/test/domain/flows_test.exs +++ b/elixir/apps/domain/test/domain/flows_test.exs @@ -982,7 +982,9 @@ defmodule Domain.FlowsTest do actor: actor, policy: policy } do - assert {:ok, [expired_flow]} = expire_flows_for(actor.id, policy.actor_group_id) + assert {:ok, [expired_flow]} = + expire_flows_for(actor.account_id, actor.id, policy.actor_group_id) + assert DateTime.diff(expired_flow.expires_at, DateTime.utc_now()) <= 1 assert expired_flow.id == flow.id end