Fix policy authorization query (#2818)

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

@@ -25,14 +25,17 @@ defmodule Domain.Resources.Resource.Query do
   end
 
   def by_authorized_actor_id(queryable \\ not_deleted(), actor_id) do
-    subquery = Domain.Policies.Policy.Query.by_actor_id(actor_id)
+    subquery =
+      Domain.Policies.Policy.Query.by_actor_id(actor_id)
+      |> where([policies: policies], policies.resource_id == parent_as(:resources).id)
+      |> limit(1)
 
     queryable
     |> join(
-      :inner,
+      :inner_lateral,
       [resources: resources],
       policies in subquery(subquery),
-      on: policies.resource_id == resources.id,
+      on: true,
       as: :authorized_by_policies
     )
     # Note: this will only write one of policies to a map, which means that

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

@@ -49,15 +49,16 @@ defmodule Domain.ResourcesTest do
 
       assert fetch_resource_by_id(resource.id, subject) == {:error, :not_found}
 
-      Fixtures.Policies.create_policy(
-        account: account,
-        actor_group: actor_group,
-        resource: resource
-      )
+      policy =
+        Fixtures.Policies.create_policy(
+          account: account,
+          actor_group: actor_group,
+          resource: resource
+        )
 
       assert {:ok, fetched_resource} = fetch_resource_by_id(resource.id, subject)
       assert fetched_resource.id == resource.id
-      refute is_nil(fetched_resource.authorized_by_policy)
+      assert fetched_resource.authorized_by_policy.id == policy.id
     end
 
     test "returns deleted resources", %{account: account, subject: subject} do
@@ -152,15 +153,49 @@ defmodule Domain.ResourcesTest do
 
       assert fetch_and_authorize_resource_by_id(resource.id, subject) == {:error, :not_found}
 
-      Fixtures.Policies.create_policy(
-        account: account,
-        actor_group: actor_group,
-        resource: resource
-      )
+      policy =
+        Fixtures.Policies.create_policy(
+          account: account,
+          actor_group: actor_group,
+          resource: resource
+        )
 
       assert {:ok, fetched_resource} = fetch_and_authorize_resource_by_id(resource.id, subject)
       assert fetched_resource.id == resource.id
-      refute is_nil(fetched_resource.authorized_by_policy)
+      assert fetched_resource.authorized_by_policy.id == policy.id
+    end
+
+    test "returns authorized resource using one of multiple policies for account user", %{
+      account: account
+    } do
+      actor = Fixtures.Actors.create_actor(type: :account_user, account: account)
+      identity = Fixtures.Auth.create_identity(account: account, actor: actor)
+      subject = Fixtures.Auth.create_subject(identity: identity)
+      resource = Fixtures.Resources.create_resource(account: account)
+
+      actor_group1 = Fixtures.Actors.create_group(account: account)
+      Fixtures.Actors.create_membership(account: account, actor: actor, group: actor_group1)
+
+      policy1 =
+        Fixtures.Policies.create_policy(
+          account: account,
+          actor_group: actor_group1,
+          resource: resource
+        )
+
+      actor_group2 = Fixtures.Actors.create_group(account: account)
+      Fixtures.Actors.create_membership(account: account, actor: actor, group: actor_group2)
+
+      policy2 =
+        Fixtures.Policies.create_policy(
+          account: account,
+          actor_group: actor_group2,
+          resource: resource
+        )
+
+      assert {:ok, fetched_resource} = fetch_and_authorize_resource_by_id(resource.id, subject)
+      assert fetched_resource.id == resource.id
+      assert fetched_resource.authorized_by_policy.id in [policy1.id, policy2.id]
     end
 
     test "does not return deleted resources", %{account: account, actor: actor, subject: subject} do
@@ -180,6 +215,66 @@ defmodule Domain.ResourcesTest do
       assert fetch_and_authorize_resource_by_id(resource.id, subject) == {:error, :not_found}
     end
 
+    test "does not authorize using deleted policies", %{
+      account: account,
+      actor: actor,
+      subject: subject
+    } do
+      resource = Fixtures.Resources.create_resource(account: account)
+      actor_group = Fixtures.Actors.create_group(account: account)
+      Fixtures.Actors.create_membership(account: account, actor: actor, group: actor_group)
+
+      Fixtures.Policies.create_policy(
+        account: account,
+        actor_group: actor_group,
+        resource: resource
+      )
+      |> Fixtures.Policies.delete_policy()
+
+      assert fetch_and_authorize_resource_by_id(resource.id, subject) == {:error, :not_found}
+    end
+
+    test "does not authorize using deleted group membership", %{
+      account: account,
+      subject: subject
+    } do
+      resource = Fixtures.Resources.create_resource(account: account)
+      actor_group = Fixtures.Actors.create_group(account: account)
+
+      # memberships are not soft deleted
+      # Fixtures.Actors.create_membership(account: account, actor: actor, group: actor_group)
+
+      Fixtures.Policies.create_policy(
+        account: account,
+        actor_group: actor_group,
+        resource: resource
+      )
+
+      assert fetch_and_authorize_resource_by_id(resource.id, subject) == {:error, :not_found}
+    end
+
+    test "does not authorize using deleted group", %{
+      account: account,
+      actor: actor,
+      subject: subject
+    } do
+      resource = Fixtures.Resources.create_resource(account: account)
+
+      actor_group =
+        Fixtures.Actors.create_group(account: account)
+        |> Fixtures.Actors.delete_group()
+
+      Fixtures.Actors.create_membership(account: account, actor: actor, group: actor_group)
+
+      Fixtures.Policies.create_policy(
+        account: account,
+        actor_group: actor_group,
+        resource: resource
+      )
+
+      assert fetch_and_authorize_resource_by_id(resource.id, subject) == {:error, :not_found}
+    end
+
     test "does not return resources in other accounts", %{subject: subject} do
       resource = Fixtures.Resources.create_resource()
       assert fetch_and_authorize_resource_by_id(resource.id, subject) == {:error, :not_found}
@@ -288,14 +383,17 @@ defmodule Domain.ResourcesTest do
       assert length(resources) == 1
       assert hd(resources).authorized_by_policy.id == policy.id
 
-      Fixtures.Policies.create_policy(
-        account: account,
-        actor_group: actor_group,
-        resource: resource2
-      )
+      policy2 =
+        Fixtures.Policies.create_policy(
+          account: account,
+          actor_group: actor_group,
+          resource: resource2
+        )
 
-      assert {:ok, resources} = list_authorized_resources(subject)
-      assert length(resources) == 2
+      assert {:ok, resources2} = list_authorized_resources(subject)
+      assert length(resources2) == 2
+
+      assert hd(resources2 -- resources).authorized_by_policy.id == policy2.id
     end
 
     test "returns authorized resources for account admin subject", %{