From b8ae9cb6ee6640409ef42fb36b3b610bfe71e144 Mon Sep 17 00:00:00 2001 From: Guarzo Date: Fri, 31 Jul 2026 20:10:10 +0000 Subject: [PATCH] fix(api): replace broken Ash.Expr.ref/2 filters with nested keyword filters InTokenMap/ParentInTokenMap called Ash.Expr.ref/2 with reversed args and, even fixed, dynamic relationship-path refs don't resolve through filter hydration. Build filters as nested keyword lists instead (a form FilterCheck.filter/3 accepts directly and Ash resolves natively through relationships). Also fixes CreateParentInTokenMap's internal parent lookup, which silently returned no rows without an actor due to MapSystem's actor-scoped read preparation. Adds direct filter/3 tests against real queries/DB rows for both FilterCheck modules plus positive/negative CreateParentInTokenMap coverage. --- lib/wanderer_app/api/policies/map_scoped.ex | 44 ++++-- .../api/policies/map_scoped_test.exs | 145 ++++++++++++++++++ 2 files changed, 177 insertions(+), 12 deletions(-) diff --git a/lib/wanderer_app/api/policies/map_scoped.ex b/lib/wanderer_app/api/policies/map_scoped.ex index 9ac060ca..861725b0 100644 --- a/lib/wanderer_app/api/policies/map_scoped.ex +++ b/lib/wanderer_app/api/policies/map_scoped.ex @@ -35,7 +35,19 @@ defmodule WandererApp.Api.Policies.MapScoped do end defmodule InTokenMap do - @moduledoc "Filters rows down to those belonging to the token's map." + @moduledoc """ + Filters rows down to those belonging to the token's map. + + `Ash.Expr.ref/2` takes `(relationship_path, name)`. A dynamically built + relationship-path ref (e.g. `Ash.Expr.ref([:system], :map_id)`) does not + resolve through filter hydration the same way a literal + `expr(system.map_id == ...)` does — it raises + `Ash.Error.Unknown: "Invalid reference ..."` (see task-1-report.md, + fix-round-1 notes). We therefore build the filter as a nested keyword + list, which `FilterCheck.filter/3` accepts directly + (`@callback filter(...) :: Keyword.t() | Ash.Expr.t()`) and which Ash + resolves through relationships without needing a dynamic `ref/2` call. + """ use Ash.Policy.FilterCheck alias WandererApp.Api.ActorHelpers @@ -47,19 +59,24 @@ defmodule WandererApp.Api.Policies.MapScoped do path = Keyword.fetch!(opts, :path) case ActorHelpers.get_map(%{actor: actor}) do - %{id: map_id} -> ref_eq(path, map_id) + %{id: map_id} -> build_filter(path, map_id) _ -> expr(false) end end - defp ref_eq(path, v) do - {rel, [attr]} = Enum.split(path, -1) - expr(^Ash.Expr.ref(attr, rel) == ^v) + defp build_filter(path, map_id) do + path + |> Enum.reverse() + |> Enum.reduce(map_id, fn segment, acc -> [{segment, acc}] end) end end defmodule ParentInTokenMap do - @moduledoc "Filters rows down to those whose parent (via relationship path) belongs to the token's map." + @moduledoc """ + Filters rows down to those whose parent (via relationship path) belongs + to the token's map. See `InTokenMap` moduledoc for why the filter is + built as a nested keyword list rather than via a dynamic `ref/2` call. + """ use Ash.Policy.FilterCheck alias WandererApp.Api.ActorHelpers @@ -71,14 +88,16 @@ defmodule WandererApp.Api.Policies.MapScoped do path = Keyword.fetch!(opts, :path) case ActorHelpers.get_map(%{actor: actor}) do - %{id: map_id} -> - {rel, [attr]} = Enum.split(path, -1) - expr(^Ash.Expr.ref(attr, rel) == ^map_id) - - _ -> - expr(false) + %{id: map_id} -> build_filter(path, map_id) + _ -> expr(false) end end + + defp build_filter(path, map_id) do + path + |> Enum.reverse() + |> Enum.reduce(map_id, fn segment, acc -> [{segment, acc}] end) + end end defmodule WriteDirect do @@ -126,6 +145,7 @@ defmodule WandererApp.Api.Policies.MapScoped do parent_id when not is_nil(parent_id) <- Ash.Changeset.get_attribute(cs, fk) do case Ash.read_one( Ash.Query.filter(parent, id == ^parent_id and map_id == ^map_id), + actor: actor, authorize?: false ) do {:ok, nil} -> false diff --git a/test/wanderer_app/api/policies/map_scoped_test.exs b/test/wanderer_app/api/policies/map_scoped_test.exs index 5dd2a90e..03e3bd21 100644 --- a/test/wanderer_app/api/policies/map_scoped_test.exs +++ b/test/wanderer_app/api/policies/map_scoped_test.exs @@ -1,5 +1,6 @@ defmodule WandererApp.Api.Policies.MapScopedTest do use WandererApp.DataCase, async: true + require Ash.Query alias WandererApp.Api.Policies.MapScoped alias WandererApp.Api.ActorWithMap @@ -122,4 +123,148 @@ defmodule WandererApp.Api.Policies.MapScopedTest do refute MapScoped.CreateMapMatchesToken.match?(no_map_actor, %{changeset: cs}, []) end end + + describe "InTokenMap filter/3 (direct path, exercised against a real query)" do + setup do + map = insert(:map, %{}) + other_map = insert(:map, %{}) + system_in_map = insert(:map_system, %{map_id: map.id}) + system_in_other_map = insert(:map_system, %{map_id: other_map.id}) + actor = ActorWithMap.new(%{id: "u"}, %{id: map.id}) + + %{ + map: map, + actor: actor, + system_in_map: system_in_map, + system_in_other_map: system_in_other_map + } + end + + test "returned filter admits only rows in the token map", %{ + actor: actor, + system_in_map: system_in_map, + system_in_other_map: system_in_other_map + } do + filter = MapScoped.InTokenMap.filter(actor, %{}, path: [:map_id]) + + {:ok, results} = + WandererApp.Api.MapSystem + |> Ash.Query.filter(^filter) + |> Ash.read(actor: actor, authorize?: false) + + ids = Enum.map(results, & &1.id) + assert system_in_map.id in ids + refute system_in_other_map.id in ids + end + + test "returns expr(false) when actor has no token map" do + no_map_actor = ActorWithMap.new(%{id: "u"}, nil) + filter = MapScoped.InTokenMap.filter(no_map_actor, %{}, path: [:map_id]) + + {:ok, results} = + WandererApp.Api.MapSystem + |> Ash.Query.filter(^filter) + |> Ash.read(actor: no_map_actor, authorize?: false) + + assert results == [] + end + end + + describe "ParentInTokenMap filter/3 (relationship path, exercised against a real query)" do + setup do + map = insert(:map, %{}) + other_map = insert(:map, %{}) + system_in_map = insert(:map_system, %{map_id: map.id}) + system_in_other_map = insert(:map_system, %{map_id: other_map.id}) + + sig_in_map = + insert(:map_system_signature, %{system_id: system_in_map.id}) + + sig_in_other_map = + insert(:map_system_signature, %{system_id: system_in_other_map.id}) + + actor = ActorWithMap.new(%{id: "u"}, %{id: map.id}) + + %{actor: actor, sig_in_map: sig_in_map, sig_in_other_map: sig_in_other_map} + end + + test "returned filter admits only rows whose parent belongs to the token map", %{ + actor: actor, + sig_in_map: sig_in_map, + sig_in_other_map: sig_in_other_map + } do + filter = MapScoped.ParentInTokenMap.filter(actor, %{}, path: [:system, :map_id]) + + {:ok, results} = + WandererApp.Api.MapSystemSignature + |> Ash.Query.filter(^filter) + |> Ash.read(actor: actor, authorize?: false) + + ids = Enum.map(results, & &1.id) + assert sig_in_map.id in ids + refute sig_in_other_map.id in ids + end + + test "returns expr(false) when actor has no token map" do + no_map_actor = ActorWithMap.new(%{id: "u"}, nil) + filter = MapScoped.ParentInTokenMap.filter(no_map_actor, %{}, path: [:system, :map_id]) + + {:ok, results} = + WandererApp.Api.MapSystemSignature + |> Ash.Query.filter(^filter) + |> Ash.read(actor: no_map_actor, authorize?: false) + + assert results == [] + end + end + + describe "CreateParentInTokenMap" do + setup do + map = insert(:map, %{}) + other_map = insert(:map, %{}) + system_in_map = insert(:map_system, %{map_id: map.id}) + system_in_other_map = insert(:map_system, %{map_id: other_map.id}) + actor = ActorWithMap.new(%{id: "u"}, %{id: map.id}) + + %{ + actor: actor, + system_in_map: system_in_map, + system_in_other_map: system_in_other_map + } + end + + test "authorizes when the parent (looked up by fk) belongs to the token map", %{ + actor: actor, + system_in_map: system_in_map + } do + cs = + WandererApp.Api.MapSystemSignature + |> Ash.Changeset.new() + |> Ash.Changeset.force_change_attribute(:system_id, system_in_map.id) + + assert MapScoped.CreateParentInTokenMap.match?( + actor, + %{changeset: cs}, + parent_resource: WandererApp.Api.MapSystem, + fk: :system_id + ) + end + + test "denies when the parent belongs to a foreign map", %{ + actor: actor, + system_in_other_map: system_in_other_map + } do + cs = + WandererApp.Api.MapSystemSignature + |> Ash.Changeset.new() + |> Ash.Changeset.force_change_attribute(:system_id, system_in_other_map.id) + + refute MapScoped.CreateParentInTokenMap.match?( + actor, + %{changeset: cs}, + parent_resource: WandererApp.Api.MapSystem, + fk: :system_id + ) + end + end end