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
This commit is contained in:
Jamil
2025-06-20 13:40:31 -07:00
committed by GitHub
parent ddb3dc8ce0
commit 6f87f5ea2c
9 changed files with 17 additions and 4 deletions

View File

@@ -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

View File

@@ -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()

View File

@@ -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
})

View File

@@ -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
})

View File

@@ -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
})

View File

@@ -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
})

View File

@@ -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
})

View File

@@ -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
})

View File

@@ -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