From 8826c9ec35f01e1cc224d1020bf2613cd78c95ae Mon Sep 17 00:00:00 2001
From: Riccardo Binetti <rbino@gmx.com>
Date: Wed, 27 Mar 2024 15:07:53 +0100
Subject: [PATCH] fix: enforce multitenancy on aggregates (#952)

The multitenancy attribute was already considered as a filter in
aggregates, but the presence of the tenant was not enforced
---
 lib/ash/actions/aggregate.ex       |  3 ++-
 lib/ash/actions/read/read.ex       |  3 ++-
 test/actions/multitenancy_test.exs | 24 ++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/lib/ash/actions/aggregate.ex b/lib/ash/actions/aggregate.ex
index cbe1e37b..21600551 100644
--- a/lib/ash/actions/aggregate.ex
+++ b/lib/ash/actions/aggregate.ex
@@ -6,7 +6,8 @@ defmodule Ash.Actions.Aggregate do
     query = %{query | api: api}
     {query, opts} = Ash.Actions.Helpers.add_process_context(query.api, query, opts)
 
-    with %{valid?: true} = query <- Ash.Actions.Read.handle_attribute_multitenancy(query) do
+    with %{valid?: true} = query <- Ash.Actions.Read.handle_attribute_multitenancy(query),
+         :ok <- Ash.Actions.Read.validate_multitenancy(query) do
       aggregates
       |> Enum.group_by(fn
         %Ash.Query.Aggregate{} = aggregate ->
diff --git a/lib/ash/actions/read/read.ex b/lib/ash/actions/read/read.ex
index 022e9ae6..c828eca5 100644
--- a/lib/ash/actions/read/read.ex
+++ b/lib/ash/actions/read/read.ex
@@ -1155,7 +1155,8 @@ defmodule Ash.Actions.Read do
     end
   end
 
-  defp validate_multitenancy(query) do
+  @doc false
+  def validate_multitenancy(query) do
     if is_nil(Ash.Resource.Info.multitenancy_strategy(query.resource)) ||
          Ash.Resource.Info.multitenancy_global?(query.resource) || query.tenant do
       :ok
diff --git a/test/actions/multitenancy_test.exs b/test/actions/multitenancy_test.exs
index 48796498..5ad2dd3f 100644
--- a/test/actions/multitenancy_test.exs
+++ b/test/actions/multitenancy_test.exs
@@ -132,6 +132,18 @@ defmodule Ash.Actions.MultitenancyTest do
       assert User |> Ash.Query.set_tenant(tenant2) |> Api.read!() == []
     end
 
+    test "a record written to one tenant cannot be read from another with aggregate queries", %{
+      tenant1: tenant1,
+      tenant2: tenant2
+    } do
+      User
+      |> Ash.Changeset.new()
+      |> Ash.Changeset.set_tenant(tenant1)
+      |> Api.create!()
+
+      assert User |> Ash.Query.set_tenant(tenant2) |> Api.list!(:name) == []
+    end
+
     test "a record can be updated in a tenant", %{tenant1: tenant1, tenant2: tenant2} do
       User
       |> Ash.Changeset.new()
@@ -208,5 +220,17 @@ defmodule Ash.Actions.MultitenancyTest do
       result = Comment |> Api.read()
       assert {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Invalid.TenantRequired{}]}} = result
     end
+
+    test "an aggregate cannot be used without tenant specified", %{
+      tenant1: tenant1
+    } do
+      Comment
+      |> Ash.Changeset.new()
+      |> Ash.Changeset.set_tenant(tenant1)
+      |> Api.create!()
+
+      result = User |> Api.count()
+      assert {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Invalid.TenantRequired{}]}} = result
+    end
   end
 end