From b0593de99a0f3b4a60268e2f0fc8d4cf94dc3a01 Mon Sep 17 00:00:00 2001 From: Guarzo Date: Sat, 1 Aug 2026 20:09:48 +0000 Subject: [PATCH] refactor(api): normalize policy style and split out duplication fixes Review cleanups, no behavior change to the authorization model. * Normalize the trusted-actor bypass to `MapScoped.trusted()` in all 18 resources. Half previously used the bare `MapScoped.Trusted` module and half the helper. The blocks are meant to be scanned as one repeated template, so an inconsistent first line defeats the point. * Drop the `WriteDirect` check. After update/destroy moved to filter checks it had a single caller, `MapDefaultSettings.create`, where it is equivalent to `create_map_matches_token/0`: map_id is `allow_nil? false` with no InjectMapFromActor, so the "absent map_id" branch is unreachable and an omitted map_id fails validation before persisting. Removes ~35 lines and one concept from the policy module. Comments in map.ex / map_system.ex / map_connection.ex that referenced it by name were updated. * Document why CreateMapMatchesToken reads params before the attribute and checks both key types. This is the check enforcing the foreign-map_id breaking change, and the ordering is load-bearing: InjectMapFromActor overwrites the attribute with the token's map before policies run, so reading only the attribute would compare the token map against itself and authorize every foreign map_id. The map-duplication fixes that were previously bundled here have moved to their own branch (fix/map-duplication-attrs) -- they are unrelated to /api/v1 scoping and fix a bug that reproduces on main. --- lib/wanderer_app/api/map.ex | 4 +- lib/wanderer_app/api/map_access_list.ex | 2 +- .../api/map_character_settings.ex | 2 +- lib/wanderer_app/api/map_connection.ex | 4 +- lib/wanderer_app/api/map_default_settings.ex | 9 +- lib/wanderer_app/api/map_subscription.ex | 2 +- lib/wanderer_app/api/map_system.ex | 4 +- lib/wanderer_app/api/map_user_settings.ex | 2 +- lib/wanderer_app/api/policies/map_scoped.ex | 57 +++------- .../map/operations/duplication.ex | 103 ++++++++++-------- .../api/internal_authz_regression_test.exs | 5 +- .../api/policies/map_scoped_test.exs | 45 -------- 12 files changed, 92 insertions(+), 147 deletions(-) diff --git a/lib/wanderer_app/api/map.ex b/lib/wanderer_app/api/map.ex index 0789bf3c..a30e44e4 100644 --- a/lib/wanderer_app/api/map.ex +++ b/lib/wanderer_app/api/map.ex @@ -12,7 +12,7 @@ defmodule WandererApp.Api.Map do require Logger policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end @@ -24,7 +24,7 @@ defmodule WandererApp.Api.Map do forbid_if always() end - # Filter check rather than `write_direct/1`; see map_connection.ex for why. + # Filter check rather than a SimpleCheck; see map_connection.ex for why. policy action_type([:update, :destroy]) do authorize_if WandererApp.Api.Policies.MapScoped.in_token_map([:id]) end diff --git a/lib/wanderer_app/api/map_access_list.ex b/lib/wanderer_app/api/map_access_list.ex index 5d4922d7..b23f4f1b 100644 --- a/lib/wanderer_app/api/map_access_list.ex +++ b/lib/wanderer_app/api/map_access_list.ex @@ -8,7 +8,7 @@ defmodule WandererApp.Api.MapAccessList do authorizers: [Ash.Policy.Authorizer] policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end diff --git a/lib/wanderer_app/api/map_character_settings.ex b/lib/wanderer_app/api/map_character_settings.ex index fdc715c6..a8c5e2f0 100644 --- a/lib/wanderer_app/api/map_character_settings.ex +++ b/lib/wanderer_app/api/map_character_settings.ex @@ -8,7 +8,7 @@ defmodule WandererApp.Api.MapCharacterSettings do authorizers: [Ash.Policy.Authorizer] policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end diff --git a/lib/wanderer_app/api/map_connection.ex b/lib/wanderer_app/api/map_connection.ex index eb911af7..f15eeb1c 100644 --- a/lib/wanderer_app/api/map_connection.ex +++ b/lib/wanderer_app/api/map_connection.ex @@ -9,7 +9,7 @@ defmodule WandererApp.Api.MapConnection do primary_read_warning?: false policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end @@ -21,7 +21,7 @@ defmodule WandererApp.Api.MapConnection do authorize_if WandererApp.Api.Policies.MapScoped.create_map_matches_token() end - # Update/destroy use the FILTER check, not the SimpleCheck `write_direct/1`. + # Update/destroy use a FILTER check rather than a SimpleCheck. # A SimpleCheck requiring original data disqualifies Ash's `:atomic` bulk # strategy, which both breaks atomic updates (NoMatchingBulkStrategy) and # forces the `:stream` destroy path, whose `Ash.Query.select([])` re-read diff --git a/lib/wanderer_app/api/map_default_settings.ex b/lib/wanderer_app/api/map_default_settings.ex index c2e30eaf..397a0107 100644 --- a/lib/wanderer_app/api/map_default_settings.ex +++ b/lib/wanderer_app/api/map_default_settings.ex @@ -11,7 +11,7 @@ defmodule WandererApp.Api.MapDefaultSettings do authorizers: [Ash.Policy.Authorizer] policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end @@ -19,9 +19,12 @@ defmodule WandererApp.Api.MapDefaultSettings do authorize_if WandererApp.Api.Policies.MapScoped.in_token_map([:map_id]) end - # Create has no existing row to filter, so it keeps the SimpleCheck. + # Create has no existing row to filter, so it uses a simple check rather + # than a filter check. `map_id` is `allow_nil? false` here and there is no + # `InjectMapFromActor` change, so the "absent map_id" branch of this check + # is unreachable -- an omitted map_id fails validation before persisting. policy action_type(:create) do - authorize_if WandererApp.Api.Policies.MapScoped.write_direct(:map_id) + authorize_if WandererApp.Api.Policies.MapScoped.create_map_matches_token() end # Update/destroy use the filter check; see map_connection.ex for why. diff --git a/lib/wanderer_app/api/map_subscription.ex b/lib/wanderer_app/api/map_subscription.ex index 336353f2..8c9eeab0 100644 --- a/lib/wanderer_app/api/map_subscription.ex +++ b/lib/wanderer_app/api/map_subscription.ex @@ -8,7 +8,7 @@ defmodule WandererApp.Api.MapSubscription do authorizers: [Ash.Policy.Authorizer] policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end diff --git a/lib/wanderer_app/api/map_system.ex b/lib/wanderer_app/api/map_system.ex index b616f6b7..d4dd4520 100644 --- a/lib/wanderer_app/api/map_system.ex +++ b/lib/wanderer_app/api/map_system.ex @@ -29,7 +29,7 @@ defmodule WandererApp.Api.MapSystem do primary_read_warning?: false policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end @@ -41,7 +41,7 @@ defmodule WandererApp.Api.MapSystem do authorize_if WandererApp.Api.Policies.MapScoped.create_map_matches_token() end - # Update/destroy use the FILTER check, not the SimpleCheck `write_direct/1`. + # Update/destroy use a FILTER check rather than a SimpleCheck. # A SimpleCheck requiring original data disqualifies Ash's `:atomic` bulk # strategy, which both breaks atomic updates (NoMatchingBulkStrategy) and # forces the `:stream` destroy path, whose `Ash.Query.select([])` re-read diff --git a/lib/wanderer_app/api/map_user_settings.ex b/lib/wanderer_app/api/map_user_settings.ex index 4502cbf9..906e8dba 100644 --- a/lib/wanderer_app/api/map_user_settings.ex +++ b/lib/wanderer_app/api/map_user_settings.ex @@ -8,7 +8,7 @@ defmodule WandererApp.Api.MapUserSettings do authorizers: [Ash.Policy.Authorizer] policies do - bypass WandererApp.Api.Policies.MapScoped.Trusted do + bypass WandererApp.Api.Policies.MapScoped.trusted() do authorize_if always() end diff --git a/lib/wanderer_app/api/policies/map_scoped.ex b/lib/wanderer_app/api/policies/map_scoped.ex index beb8a9b9..c50a0904 100644 --- a/lib/wanderer_app/api/policies/map_scoped.ex +++ b/lib/wanderer_app/api/policies/map_scoped.ex @@ -10,7 +10,6 @@ defmodule WandererApp.Api.Policies.MapScoped do def trusted, do: {__MODULE__.Trusted, []} def in_token_map(path) when is_list(path), do: {__MODULE__.InTokenMap, path: path} - def write_direct(attr \\ :map_id), do: {__MODULE__.WriteDirect, attr: attr} def parent_in_token_map(path) when is_list(path), do: {__MODULE__.ParentInTokenMap, path: path} def create_parent_in_token_map(parent_resource, fk), @@ -110,44 +109,6 @@ defmodule WandererApp.Api.Policies.MapScoped do end end - defmodule WriteDirect do - @moduledoc """ - Authorizes a write when the changeset's map_id attribute matches the token's map. - - Reads `changeset.data` (via `Ash.Changeset.get_attribute/2`'s fallback to - `get_data/2`) to cover update/destroy actions where the scoped attribute - isn't part of the change itself (e.g. `map.ex`'s `write_direct(:id)`, or - an update that doesn't touch `map_id`). Ash's atomic-query update/destroy - path skips fetching original data as an optimization unless a check - declares `requires_original_data?/2` — without this override, - `changeset.data` becomes `%Ash.Changeset.OriginalDataNotAvailable{}` and - every atomically-eligible update is wrongly denied. - """ - use Ash.Policy.SimpleCheck - alias WandererApp.Api.ActorHelpers - - @impl true - def describe(_), do: "changeset target belongs to the token's map" - - @impl true - def requires_original_data?(_authorizer, _opts), do: true - - @impl true - def match?(actor, %{changeset: %Ash.Changeset{} = cs}, opts) do - attr = Keyword.get(opts, :attr, :map_id) - - with %{id: map_id} <- ActorHelpers.get_map(%{actor: actor}), - row_map_id when not is_nil(row_map_id) <- - Ash.Changeset.get_attribute(cs, attr) do - row_map_id == map_id - else - _ -> false - end - end - - def match?(_actor, _ctx, _opts), do: false - end - defmodule CreateParentInTokenMap do @moduledoc """ Authorizes a create when the row's parent (looked up by foreign key) @@ -200,6 +161,24 @@ defmodule WandererApp.Api.Policies.MapScoped do def match?(actor, %{changeset: %Ash.Changeset{} = cs}, _opts) do case ActorHelpers.get_map(%{actor: actor}) do %{id: map_id} -> + # Check the raw params BEFORE the attribute, and check both key + # types, because each source answers a different question: + # + # * `params["map_id"]` -- what a JSON:API client actually sent. This + # is the value being authorized, and it must be read from params + # because `InjectMapFromActor` overwrites the *attribute* with the + # token's map before policies run. Reading only the attribute + # would therefore compare the token map against itself and + # authorize every foreign map_id. + # * `params[:map_id]` -- the same value when the changeset was built + # internally with atom keys (Ash does not normalize params). + # * the attribute -- the fallback for resources with no + # `InjectMapFromActor` change, where map_id is set directly and + # never appears in params. + # + # `nil` means the client supplied nothing, so the token's map is + # injected (or `allow_nil? false` rejects it) -- either way there is + # no foreign id to forbid. supplied = Map.get(cs.params || %{}, "map_id") || Map.get(cs.params || %{}, :map_id) || Ash.Changeset.get_attribute(cs, :map_id) diff --git a/lib/wanderer_app/map/operations/duplication.ex b/lib/wanderer_app/map/operations/duplication.ex index 7033005c..308f0f2c 100644 --- a/lib/wanderer_app/map/operations/duplication.ex +++ b/lib/wanderer_app/map/operations/duplication.ex @@ -94,11 +94,28 @@ defmodule WandererApp.Map.Operations.Duplication do # Copy a single system defp copy_single_system(source_system, new_map_id) do - # Same allowlist approach as connections -- see acceptable_attrs/3 below for - # why a denylist was the wrong shape here. + # Get all attributes from the source system, excluding system-managed fields and metadata + excluded_fields = [ + # System managed fields + :id, + :inserted_at, + :updated_at, + :map_id, + :map, + # Ash/Ecto metadata fields + :__meta__, + :__lateral_join_source__, + :__metadata__, + :__order__, + :aggregates, + :calculations + ] + + # Convert the source system struct to a map and filter out excluded fields system_attrs = source_system - |> acceptable_attrs(MapSystem, :create) + |> Map.from_struct() + |> Map.drop(excluded_fields) |> Map.put(:map_id, new_map_id) MapSystem.create(system_attrs) @@ -128,41 +145,34 @@ defmodule WandererApp.Map.Operations.Duplication do # Copy a single connection with updated system references defp copy_single_connection(source_connection, new_map_id, system_mapping) do + # Get all attributes from the source connection, excluding system-managed fields and metadata + excluded_fields = [ + # System managed fields + :id, + :inserted_at, + :updated_at, + :map_id, + :map, + # Ash/Ecto metadata fields + :__meta__, + :__lateral_join_source__, + :__metadata__, + :__order__, + :aggregates, + :calculations + ] + + # Convert the source connection struct to a map and filter out excluded fields connection_attrs = source_connection - |> acceptable_attrs(MapConnection, :create) + |> Map.from_struct() + |> Map.drop(excluded_fields) |> Map.put(:map_id, new_map_id) |> update_system_references(system_mapping) MapConnection.create(connection_attrs) end - # Build the attribute map from what the target action actually ACCEPTS, - # rather than by dropping a hand-maintained list of fields to exclude. - # - # This previously used `Map.from_struct() |> Map.drop(excluded_fields)`, a - # denylist: every attribute not explicitly named was forwarded to `create`. - # When commit 2f86fc9f added `locked_at` / `locked_by` / `locked_by_id` to - # MapConnection without adding them to the `:create` accept list, duplication - # started failing with Ash.Error.Invalid.NoSuchInput and every map - # duplication returned 500. - # - # An allowlist derived from the action cannot drift: a new attribute is - # copied only if the action accepts it, and is silently skipped otherwise. - defp acceptable_attrs(source, resource, action_name) do - accepted = - resource - |> Ash.Resource.Info.action(action_name) - |> Map.get(:accept, []) - |> MapSet.new() - - source - |> Map.from_struct() - |> Map.filter(fn {key, value} -> - MapSet.member?(accepted, key) and not match?(%Ash.NotLoaded{}, value) - end) - end - # Update system references in connection attributes using the system mapping defp update_system_references(connection_attrs, system_mapping) do connection_attrs @@ -192,31 +202,28 @@ defmodule WandererApp.Map.Operations.Duplication do Logger.debug("Copying signatures for map #{source_map.id}") # Get signatures by iterating through systems - with {:ok, source_signatures} <- get_all_map_signatures(source_map.id, system_mapping) do - Enum.reduce_while(source_signatures, {:ok, []}, fn source_signature, - {:ok, acc_signatures} -> - case copy_single_signature(source_signature, new_map.id, system_mapping) do - {:ok, new_signature} -> - {:cont, {:ok, [new_signature | acc_signatures]}} + source_signatures = get_all_map_signatures(source_map.id, system_mapping) - {:error, reason} -> - {:halt, {:error, {:signature_copy_failed, reason}}} - end - end) - end + Enum.reduce_while(source_signatures, {:ok, []}, fn source_signature, {:ok, acc_signatures} -> + case copy_single_signature(source_signature, new_map.id, system_mapping) do + {:ok, new_signature} -> + {:cont, {:ok, [new_signature | acc_signatures]}} + + {:error, reason} -> + {:halt, {:error, {:signature_copy_failed, reason}}} + end + end) end - # Get all signatures for a map by querying each system. A read failure for any - # system aborts the copy with an error rather than silently omitting those - # signatures, which would return an incomplete duplicate reported as success. + # Get all signatures for a map by querying each system defp get_all_map_signatures(_source_map_id, system_mapping) do # Get source system IDs and query signatures for each source_system_ids = Map.keys(system_mapping) - Enum.reduce_while(source_system_ids, {:ok, []}, fn system_id, {:ok, acc} -> - case MapSystemSignature.by_system_id_all(system_id) do - {:ok, signatures} -> {:cont, {:ok, acc ++ signatures}} - {:error, reason} -> {:halt, {:error, {:signature_read_failed, reason}}} + Enum.flat_map(source_system_ids, fn system_id -> + case MapSystemSignature.by_system_id_all(%{system_id: system_id}) do + {:ok, signatures} -> signatures + {:error, _} -> [] end end) end diff --git a/test/wanderer_app/api/internal_authz_regression_test.exs b/test/wanderer_app/api/internal_authz_regression_test.exs index 340fbe92..211ebf10 100644 --- a/test/wanderer_app/api/internal_authz_regression_test.exs +++ b/test/wanderer_app/api/internal_authz_regression_test.exs @@ -63,8 +63,9 @@ defmodule WandererApp.Api.InternalAuthzRegressionTest do end describe "write actions with internal actors" do - # map_default_settings is the resource that still uses the WriteDirect - # SimpleCheck, so it is the most sensitive to changes in that check. + # map_default_settings is the only resource whose create is guarded by + # CreateMapMatchesToken while map_id is `allow_nil? false` with no + # InjectMapFromActor, so it is the most sensitive to changes in that check. test "MapDefaultSettings.create works with a Character actor", %{char: char, map: map} do assert {:ok, _} = WandererApp.Api.MapDefaultSettings.create( diff --git a/test/wanderer_app/api/policies/map_scoped_test.exs b/test/wanderer_app/api/policies/map_scoped_test.exs index 434654c2..959316f8 100644 --- a/test/wanderer_app/api/policies/map_scoped_test.exs +++ b/test/wanderer_app/api/policies/map_scoped_test.exs @@ -41,14 +41,6 @@ defmodule WandererApp.Api.Policies.MapScopedTest do MapScoped.create_parent_in_token_map(WandererApp.Api.MapSystem, :system_id) end - test "write_direct/1 default" do - assert {MapScoped.WriteDirect, attr: :map_id} = MapScoped.write_direct() - end - - test "write_direct/1 custom attr" do - assert {MapScoped.WriteDirect, attr: :system_id} = MapScoped.write_direct(:system_id) - end - test "parent_in_token_map/1" do assert {MapScoped.ParentInTokenMap, path: [:system, :map_id]} = MapScoped.parent_in_token_map([:system, :map_id]) @@ -59,43 +51,6 @@ defmodule WandererApp.Api.Policies.MapScopedTest do end end - describe "WriteDirect" do - setup do - map_id = Ecto.UUID.generate() - actor = ActorWithMap.new(%{id: "u"}, %{id: map_id}) - %{map_id: map_id, actor: actor} - end - - test "authorizes when changeset attribute matches token map", %{map_id: map_id, actor: actor} do - cs = - %WandererApp.Api.MapSystem{} - |> Ash.Changeset.new() - |> Ash.Changeset.force_change_attribute(:map_id, map_id) - - assert MapScoped.WriteDirect.match?(actor, %{changeset: cs}, attr: :map_id) - end - - test "denies when changeset attribute is a foreign map", %{actor: actor} do - cs = - %WandererApp.Api.MapSystem{} - |> Ash.Changeset.new() - |> Ash.Changeset.force_change_attribute(:map_id, Ecto.UUID.generate()) - - refute MapScoped.WriteDirect.match?(actor, %{changeset: cs}, attr: :map_id) - end - - test "denies when actor has no token map", %{map_id: map_id} do - no_map_actor = ActorWithMap.new(%{id: "u"}, nil) - - cs = - %WandererApp.Api.MapSystem{} - |> Ash.Changeset.new() - |> Ash.Changeset.force_change_attribute(:map_id, map_id) - - refute MapScoped.WriteDirect.match?(no_map_actor, %{changeset: cs}, attr: :map_id) - end - end - describe "CreateMapMatchesToken" do setup do map_id = Ecto.UUID.generate()