fix(portal): Use old policy for broadcasting events when updated (#8550)

A regression was introduced in d0f0de0f8d
whereupon we started using the updated policy record for broadcasting
the `delete_policy` and `expire_flows` events. This caused a security
issue because if the actor group changed from `Everyone` to `thomas`,
for example, we'd only expire flows and broadcast policy removal (i.e.
resource removal) events for `thomas`, and `Everyone` would still have
access granted by the old policy.

To fix this, we broadcast the destructive events to the old policy, so
that its `actor_group_id` and `resource_id` are used, and not the new
policy's.

Fixes #8549

elixir/apps/domain/lib/domain/policies.ex

@@ -94,8 +94,8 @@ defmodule Domain.Policies do
         with: &Policy.Changeset.update(&1, attrs),
         after_update_commit: &broadcast_policy_events(:update, &1),
         after_breaking_update_commit: fn updated_policy, _changeset ->
-          {:ok, _flows} = Flows.expire_flows_for(updated_policy, subject)
-          :ok = broadcast_policy_events(:delete, updated_policy)
+          {:ok, _flows} = Flows.expire_flows_for(policy, subject)
+          :ok = broadcast_policy_events(:delete, policy)
           :ok = broadcast_policy_events(:create, updated_policy)
         end
       )

elixir/apps/domain/lib/domain/resources.ex

@@ -285,9 +285,9 @@ defmodule Domain.Resources do
         after_breaking_update_commit: fn updated_resource, _changeset ->
           # The :delete resource event broadcast is a no-op.
           # This is used to reset the resource on the client and gateway in case filters, conditions, etc are changed.
-          {:ok, _flows} = Flows.expire_flows_for(updated_resource, subject)
+          {:ok, _flows} = Flows.expire_flows_for(resource, subject)
 
-          :ok = broadcast_resource_events(:delete, updated_resource)
+          :ok = broadcast_resource_events(:delete, resource)
           :ok = broadcast_resource_events(:create, updated_resource)
         end
       )

elixir/apps/domain/test/domain/policies_test.exs

@@ -582,25 +582,29 @@ defmodule Domain.PoliciesTest do
     } do
       flow = Fixtures.Flows.create_flow(account: account, subject: subject, policy: policy)
       new_resource = Fixtures.Resources.create_resource(account: account)
+      new_actor_group = Fixtures.Actors.create_group(account: account)
 
       :ok = subscribe_to_events_for_policy(policy)
       :ok = subscribe_to_events_for_actor_group(policy.actor_group_id)
       :ok = Domain.Flows.subscribe_to_flow_expiration_events(flow)
 
-      attrs = %{resource_id: new_resource.id}
+      attrs = %{resource_id: new_resource.id, actor_group_id: new_actor_group.id}
 
-      assert {:updated, updated_policy} =
-               update_policy(policy, attrs, subject)
+      assert {:updated, updated_policy} = update_policy(policy, attrs, subject)
 
+      # Updating a policy sends delete and create events
       assert_receive {:delete_policy, policy_id}
+      assert policy_id == policy.id
+
+      assert_receive {:create_policy, policy_id}
       assert policy_id == updated_policy.id
 
       assert_receive {:reject_access, policy_id, actor_group_id, resource_id}
-      assert policy_id == updated_policy.id
-      assert actor_group_id == updated_policy.actor_group_id
-      assert resource_id == updated_policy.resource_id
+      assert policy_id == policy.id
+      assert actor_group_id == policy.actor_group_id
+      assert resource_id == policy.resource_id
 
-      assert_received {:expire_flow, flow_id, _flow_client_id, _flow_resource_id}
+      assert_receive {:expire_flow, flow_id, _flow_client_id, _flow_resource_id}
       assert flow_id == flow.id
     end
 

elixir/apps/domain/test/domain/resources_test.exs

@@ -1394,7 +1394,7 @@ defmodule Domain.ResourcesTest do
       assert resource.name == "foo"
     end
 
-    test "allows to update client address", %{resource: resource, subject: subject} do
+    test "allows to update resource address", %{resource: resource, subject: subject} do
       attrs = %{"address_description" => "http://#{resource.address}:1234/foo"}
       assert {:updated, resource} = update_resource(resource, attrs, subject)
       assert resource.address_description == attrs["address_description"]
@@ -1482,10 +1482,13 @@ defmodule Domain.ResourcesTest do
       assert {:updated, updated_resource} =
                update_resource(resource, attrs, subject)
 
+      # Resource id doesn't change when updated, but for clarity we still test that we receive the
+      # destructive events for the old resource id and the creation events for the new resource id.
       flow_id = flow.id
       updated_resource_id = updated_resource.id
-      assert_receive {:expire_flow, ^flow_id, _client_id, ^updated_resource_id}
-      assert_receive {:delete_resource, ^updated_resource_id}
+      resource_id = resource.id
+      assert_receive {:expire_flow, ^flow_id, _client_id, ^resource_id}
+      assert_receive {:delete_resource, ^resource_id}
       assert_receive {:create_resource, ^updated_resource_id}
     end