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

@@ -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

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
 

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
 

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

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.

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
 

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

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
 

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)

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

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(

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()