From 890a4d37522c601667e41fbce8d0eacd9865933d Mon Sep 17 00:00:00 2001 From: Moritz Date: Wed, 4 Feb 2026 09:19:57 +0100 Subject: [PATCH] MemberGroup: restrict bypass to own_data via MemberGroupReadLinkedForOwnData - ActorPermissionSetIs check; bypass policy filters by member_id for own_data only. - Admin with member_id still gets :all via HasPermission. Tests added. --- lib/membership/member_group.ex | 7 +-- .../checks/actor_permission_set_is.ex | 44 +++++++++++++ .../member_group_read_linked_for_own_data.ex | 63 +++++++++++++++++++ .../membership/member_group_policies_test.exs | 33 ++++++++++ 4 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 lib/mv/authorization/checks/actor_permission_set_is.ex create mode 100644 lib/mv/authorization/checks/member_group_read_linked_for_own_data.ex diff --git a/lib/membership/member_group.ex b/lib/membership/member_group.ex index fe8b2b9..22a1f70 100644 --- a/lib/membership/member_group.ex +++ b/lib/membership/member_group.ex @@ -42,7 +42,6 @@ defmodule Mv.Membership.MemberGroup do data_layer: AshPostgres.DataLayer, authorizers: [Ash.Policy.Authorizer] - import Ash.Expr require Ash.Query postgres do @@ -58,13 +57,13 @@ defmodule Mv.Membership.MemberGroup do end end - # Authorization: read uses bypass for :linked (own_data list) then HasPermission for :all; + # Authorization: read uses bypass for :linked (own_data only) then HasPermission for :all; # create/destroy use HasPermission (normal_user + admin only). - # Order: bypass first so own_data gets expr filter; HasPermission then authorizes :all for others. + # Single check: own_data gets filter via auto_filter; admin does not match, gets :all from HasPermission. policies do bypass action_type(:read) do description "own_data: read only member_groups where member_id == actor.member_id" - authorize_if expr(member_id == ^actor(:member_id)) + authorize_if Mv.Authorization.Checks.MemberGroupReadLinkedForOwnData end policy action_type(:read) do diff --git a/lib/mv/authorization/checks/actor_permission_set_is.ex b/lib/mv/authorization/checks/actor_permission_set_is.ex new file mode 100644 index 0000000..deb9382 --- /dev/null +++ b/lib/mv/authorization/checks/actor_permission_set_is.ex @@ -0,0 +1,44 @@ +defmodule Mv.Authorization.Checks.ActorPermissionSetIs do + @moduledoc """ + Policy check: true when the actor's role has the given permission_set_name. + + Used to restrict bypass policies (e.g. MemberGroup read by member_id) to actors + with a specific permission set (e.g. "own_data") so that admin with member_id + still gets :all scope from HasPermission, not the bypass filter. + + ## Usage + + # In a resource policy (both conditions must hold for the bypass) + bypass action_type(:read) do + authorize_if expr(member_id == ^actor(:member_id)) + authorize_if {Mv.Authorization.Checks.ActorPermissionSetIs, permission_set_name: "own_data"} + end + + ## Options + + - `:permission_set_name` (required) - String or atom, e.g. `"own_data"` or `:own_data` + """ + use Ash.Policy.SimpleCheck + + alias Mv.Authorization.Actor + + @impl true + def describe(opts) do + name = opts[:permission_set_name] || "?" + "actor has permission set #{name}" + end + + @impl true + def match?(actor, _context, opts) do + case opts[:permission_set_name] do + nil -> + false + + expected -> + case Actor.permission_set_name(actor) do + nil -> false + actual -> to_string(expected) == to_string(actual) + end + end + end +end diff --git a/lib/mv/authorization/checks/member_group_read_linked_for_own_data.ex b/lib/mv/authorization/checks/member_group_read_linked_for_own_data.ex new file mode 100644 index 0000000..a553fde --- /dev/null +++ b/lib/mv/authorization/checks/member_group_read_linked_for_own_data.ex @@ -0,0 +1,63 @@ +defmodule Mv.Authorization.Checks.MemberGroupReadLinkedForOwnData do + @moduledoc """ + Policy check for MemberGroup read: true only when actor has permission set "own_data" + AND record.member_id == actor.member_id. + + Used in a bypass so that own_data gets the linked filter (via auto_filter for list queries), + while admin with member_id does not match and gets :all from HasPermission. + + - With a record (e.g. get by id): returns true only when own_data and member_id match. + - Without a record (list query): strict_check returns false; auto_filter adds filter when own_data. + """ + use Ash.Policy.Check + + alias Mv.Authorization.Checks.ActorPermissionSetIs + + @impl true + def type, do: :filter + + @impl true + def describe(_opts), + do: "own_data can read only member_groups where member_id == actor.member_id" + + @impl true + def strict_check(actor, authorizer, _opts) do + record = get_record_from_authorizer(authorizer) + is_own_data = ActorPermissionSetIs.match?(actor, authorizer, permission_set_name: "own_data") + + cond do + # List query + own_data: return :unknown so authorizer applies auto_filter (keyword list) + is_nil(record) and is_own_data -> + {:ok, :unknown} + + is_nil(record) -> + {:ok, false} + + not is_own_data -> + {:ok, false} + + record.member_id == actor.member_id -> + {:ok, true} + + true -> + {:ok, false} + end + end + + @impl true + def auto_filter(actor, _authorizer, _opts) do + if ActorPermissionSetIs.match?(actor, nil, permission_set_name: "own_data") && + Map.get(actor, :member_id) do + [member_id: actor.member_id] + else + [] + end + end + + defp get_record_from_authorizer(authorizer) do + case authorizer.subject do + %{data: data} when not is_nil(data) -> data + _ -> nil + end + end +end diff --git a/test/mv/membership/member_group_policies_test.exs b/test/mv/membership/member_group_policies_test.exs index d35d0ea..ecac2f4 100644 --- a/test/mv/membership/member_group_policies_test.exs +++ b/test/mv/membership/member_group_policies_test.exs @@ -184,6 +184,39 @@ defmodule Mv.Membership.MemberGroupPoliciesTest do assert mg.id in ids end + test "admin with member_id set (linked to member) still reads all member_groups", %{ + actor: actor + } do + # Admin linked to a member (e.g. viewing as member context) must still get :all scope, + # not restricted to linked member's groups (bypass is only for own_data). + admin = Mv.Fixtures.user_with_role_fixture("admin") + linked_member = create_member_fixture() + other_member = create_member_fixture() + group_a = create_group_fixture() + group_b = create_group_fixture() + + admin = + admin + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.force_change_attribute(:member_id, linked_member.id) + |> Ash.update(actor: actor) + + {:ok, admin} = Ash.load(admin, :role, domain: Mv.Accounts, actor: actor) + + mg_linked = create_member_group_fixture(linked_member.id, group_a.id) + mg_other = create_member_group_fixture(other_member.id, group_b.id) + + {:ok, list} = + Mv.Membership.MemberGroup + |> Ash.read(actor: admin, domain: Mv.Membership) + + ids = Enum.map(list, & &1.id) + assert mg_linked.id in ids, "Admin with member_id must see linked member's MemberGroups" + + assert mg_other.id in ids, + "Admin with member_id must see all MemberGroups (:all), not only linked" + end + test "can create member_group", %{user: user, actor: _actor} do member = create_member_fixture() group = create_group_fixture()