Two polish follow-ups on the /api/v1 authz branch:
1. Map duplication never actually copied signatures. get_all_map_signatures
called `MapSystemSignature.by_system_id_all(%{system_id: id})`, but the
code interface is defined with `args: [:system_id]` and every other call
site passes the id positionally. The map form raised
Ash.Error.Query.InvalidArgument on :system_id, which the old
`{:error, _} -> []` swallow silently turned into "no signatures", so
duplication reported success while copying none. Fixed the call to pass
the id positionally; the surrounding reduce now propagates a read failure
as {:error, {:signature_read_failed, _}} and rolls back rather than
returning an incomplete duplicate as success.
2. Added a custom-endpoint test asserting the systems_and_connections IDOR
guard is not loosened into case-folding: an upper-cased own-map id must
still 404, confirming to_string/1 normalization guards rather than matches
loosely.
Verified: all /api/v1 authz suites, map_scoped units, and the four map
duplication suites pass (157/0 at seed 0).
The rationale in these comments is worth keeping, but the citations
(task-N, task-1-report.md, fix-round-1) point at branch-local planning
artifacts that no reader of the repository can resolve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two guards closing out the authz work.
internal_authz_regression_test.exs exercises the real internal call sites
that pass an `actor:`, so tightening a policy later cannot silently break the
app's own paths. Each test names the production site it mirrors.
Two corrections to the planned tests, both from reading the callers:
* Map.duplicate is invoked with conn.assigns[:current_character], a
Character (map_api_controller.ex:1276), and the :duplicate action sets
owner_id from context.actor.id. Map.owner is a belongs_to Character, so a
User actor would write a User id into a Character FK. The test uses a
Character and asserts owner_id comes back correct.
* :available is never called with a Character. Its only callers
(Maps.get_available_maps/1, Acls.get_available_acls/1) pass a User, and
FilterMapsByRoles reads actor.characters, which a Character struct lacks.
The suite reloads the User with `load: :characters` the way
CheckJsonApiAuth does, and covers the Character bypass via the write and
duplicate tests instead.
authz_includes_leak_test.exs covers `include=`, the one path that reaches a
resource without going through its own route. Every refutation is paired
with a positive control so it cannot pass vacuously.
Note Character is NOT an AshJsonApi resource (extensions: [AshCloak] only),
so ?include=owner emits a resource identifier with "type": null and an empty
attributes object -- no Character field is serialized at all. The negative
assertion is therefore about sensitive attributes being absent rather than
about the type name, which the planned test would have asserted and which
would never have matched.
GET /api/v1/maps/:map_id/systems_and_connections is a hand-written
controller, so it inherits none of the AshJsonApi route policies. It had two
independent holes, both fixed here:
1. No path-vs-token check. Any valid token could read ANY map's systems and
connections by passing that map's id in the path. Confirmed before the
fix: requesting another map's id returned 200 with its data.
2. Actor-less reads. `Ash.read!/1` was called with no actor, and the domain
gate is `authorize :when_requested`, which authorizes only when an
`actor:` key is present (ash/lib/ash/actions/helpers.ex:390) -- so the
reads ran entirely unauthorized.
A path/token mismatch returns 404 rather than 403 so the endpoint does not
confirm that another map exists, consistent with the filter-check behaviour
elsewhere on this surface.
The two fixes are verified independently by mutation, since either could
mask the other: reverting the guard alone fails 2 tests, and dropping
`actor:` alone fails 1. Note the second failure is the OWN-map positive
control going empty -- MapSystem's FilterSystemsByActorMap preparation
fails closed without an actor -- which is exactly why that test asserts
membership of a real id rather than just a 200.
Full suite: 392 tests, 14 failures, all in the two known-flaky pre-existing
suites; zero new.
MapSolarSystem, MapState, ShipTypeInfo and User expose no /api/v1 routes,
but they remain reachable as relationship `includes` and would become
reachable if a route were ever added. Enforce the guarantee at the Ash layer
rather than relying on the route list staying empty: token actors are
forbidden outright, trusted internal actors pass via the bypass.
Safe for internal callers. The domain gate is `authorize :when_requested`,
which authorizes only when an `actor:` key is present
(ash/lib/ash/actions/helpers.ex:390). Every internal caller of these four
reads them without an actor (e.g. ShipTypeInfo.read/0 in cached_info.ex), so
those paths skip authorization entirely. A test asserts this direction
explicitly so a future change that breaks production reads is caught.
Also replaces MapScoped.Trusted's struct pattern matching with is_struct/2.
A struct pattern is expanded at COMPILE time, so matching %Api.User{} made
the policy module depend on Api.User; once User itself gained a policy
referencing that module, the cycle closed and `mix compile --force` failed
with "deadlocked waiting on struct WandererApp.Api.User" (reproduced twice,
24 occurrences each). is_struct/2 checks the module at runtime and is
otherwise equivalent. MapScoped's 24 unit tests still pass unchanged.
Tests require %Ash.Error.Forbidden{} exclusively, never {:ok, []}, and seed
a row per resource first so no denial can pass vacuously over an empty
table. Verified non-vacuous by mutation: flipping forbid_if to authorize_if
fails the deny test.
Full suite: 386 tests, 14 failures, all in the two known-flaky pre-existing
suites; zero new.
Add the AclScoped policy module and apply policies to the ACL resources and
the user_activity audit log.
ACLs join to maps through MapAccessList rather than carrying a map_id, so
scoping tests for the existence of a join row: `exists(map_access_lists,
map_id == ^map_id)`. A literal expr is safe here because the relationship
path is static; the nested-keyword-list workaround in MapScoped exists only
because that module builds paths dynamically from an opt.
Status codes, deliberately mixed:
* ACL/member reads are FilterChecks -> an unlinked ACL is invisible
(list omits it, GET is 404).
* ACL/member writes are `forbid_if always()` -> 403 even for a LINKED
ACL. Unlike map-owned resources there is no "your own ACL" a map token
may legitimately mutate, so this is a hard denial rather than scoping.
* user_activity is hard-forbidden on every action -> 403. Deliberately not
a filter, which would return an empty 200 and imply the endpoint is
legitimately available to tokens.
AccessList gained `has_many :map_access_lists` (the missing inverse of
MapAccessList.access_list) so the read filter can resolve.
Tests pair each denial with a positive control and assert internal User
actors still read and write through the bypass. Verified non-vacuous by
mutation: neutering all scoped checks and forbids fails 11 of 16 tests. The
user_activity suite seeds a real row so its 403s are not a vacuous pass over
an empty table.
Full integration + web suite: 13 failures, identical to the pre-branch
baseline (zero new).
Add Ash policies to the three `system`-path /api/v1 resources, scoping each
to the routes it actually exposes:
* map_system_signature - read + destroy
* map_system_structure - full CRUD
* map_system_comment - read only
Read/update/destroy use FilterChecks (in_token_map / parent_in_token_map)
across the [:system, :map_id] path, so out-of-scope rows are invisible
rather than rejected: lists omit them and GET/PATCH/DELETE return 404. This
follows the amended global constraint from Task 2 -- filter checks keep the
:atomic strategy eligible and avoid confirming that a foreign row exists.
Structure create has no existing row to filter, so it uses the
CreateParentInTokenMap SimpleCheck to look up the parent system and returns
403 for a foreign parent.
Tests pair every denial with an own-map positive control. Verified
non-vacuous by mutation: replacing all six scoped checks with
`authorize_if always()` fails 11 of the 15 tests.
The shared `insert(:map_system_comment, ...)` factory is stale (it passes
map_id/solar_system_id/position_* which MapSystemComment's :create no longer
accepts); the test creates comments directly rather than expanding scope to
fix the factory.
Turns on `authorize :when_requested` on the WandererApp.Api domain and
attaches Ash.Policy.Authorizer policies to every route-exposed resource
scoped by a direct `map_id` (plus `map` itself, scoped by `:id`).
Composition is `bypass Trusted` (internal %User{}/%Character{} actors)
followed by action policies, per the design. Reads are filter-scoped;
creates are rejected by SimpleChecks.
Two behaviours worth calling out for reviewers:
BREAKING API CHANGE: POST /api/v1/map_systems (and map_connections) with
a client-supplied foreign map_id now returns 403 and creates nothing.
Previously InjectMapFromActor silently overrode it and returned 201.
Omitting map_id still injects the token's map and returns 201 -- that is
the path real clients use, and it now has an explicit positive-control
test. InjectMapFromActor itself is unmodified.
Foreign PATCH/DELETE returns 404, not 403. Update/destroy use the FILTER
check rather than the `write_direct/1` SimpleCheck: a SimpleCheck with
`requires_original_data?` disqualifies Ash's :atomic bulk strategy, which
broke atomic updates (NoMatchingBulkStrategy -> 400) and forced the
:stream destroy path, whose `Ash.Query.select([])` re-read returns
pkey-only records that then crash Jason encoding on %Ash.NotLoaded{}.
Filter-scoping makes out-of-scope rows invisible, so Ash reports
not_found. This deviates from the plan's "writes -> 403" constraint and
was accepted deliberately; it also avoids confirming that foreign rows
exist.
Adds simple_sat, required at runtime by Ash's policy authorizer.
Verified: 25/25 new authz matrix tests; full integration suite shows the
same 13 pre-existing failures as before this branch (no new failures);
no new compile warnings.
Guarzo