diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 3858bd6..3512ef0 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -21,8 +21,8 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:all** - Authorizes without filtering (returns all records) - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - - Member: member.user.id == actor.id (via has_one :user relationship) - - CustomFieldValue: custom_field_value.member.user.id == actor.id (traverses member → user relationship!) + - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) + - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id) ## Error Handling @@ -129,9 +129,18 @@ defmodule Mv.Authorization.Checks.HasPermission do action = get_action_from_authorizer(authorizer) cond do - is_nil(actor) -> nil - is_nil(action) -> nil - true -> auto_filter_with_permissions(actor, resource, action) + is_nil(actor) -> + # No actor - deny access (fail-closed) + # Return filter that never matches (using impossible condition) + # This ensures no records are returned when actor is missing + [id: {:not, {:in, []}}] + + is_nil(action) -> + # Cannot determine action - deny access (fail-closed) + [id: {:not, {:in, []}}] + + true -> + auto_filter_with_permissions(actor, resource, action) end end @@ -148,12 +157,25 @@ defmodule Mv.Authorization.Checks.HasPermission do actor, resource_name ) do - :authorized -> nil - {:filter, filter_expr} -> filter_expr - false -> nil + :authorized -> + # :all scope - allow all records (no filter) + # Return empty keyword list (no filtering) + [] + + {:filter, filter_expr} -> + # :linked or :own scope - apply filter + # filter_expr is a keyword list from expr(...), return it directly + filter_expr + + false -> + # No permission - deny access (fail-closed) + # Return filter that never matches (using impossible condition) + [id: {:not, {:in, []}}] end else - _ -> nil + _ -> + # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) + [id: {:not, {:in, []}}] end end @@ -162,17 +184,27 @@ defmodule Mv.Authorization.Checks.HasPermission do # Action types: :create, :read, :update, :destroy # Action names: :create_member, :update_member, etc. # PermissionSets uses action types, not action names + # + # Prefer authorizer.action.type (stable API) over authorizer.subject (varies by context) defp get_action_from_authorizer(authorizer) do - case authorizer.subject do - %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> - action_type - - # Fallback for older Ash versions or different subject shapes - %{action: %{type: action_type}} when action_type in [:create, :read, :update, :destroy] -> + # Primary: Use authorizer.action.type (stable API) + case Map.get(authorizer, :action) do + %{type: action_type} when action_type in [:create, :read, :update, :destroy] -> action_type _ -> - nil + # Fallback: Try authorizer.subject (for compatibility with different Ash versions/contexts) + case Map.get(authorizer, :subject) do + %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> + action_type + + %{action: %{type: action_type}} + when action_type in [:create, :read, :update, :destroy] -> + action_type + + _ -> + nil + end end end diff --git a/test/mv/authorization/checks/has_permission_integration_test.exs b/test/mv/authorization/checks/has_permission_integration_test.exs index 5aa43a5..dad68ea 100644 --- a/test/mv/authorization/checks/has_permission_integration_test.exs +++ b/test/mv/authorization/checks/has_permission_integration_test.exs @@ -72,14 +72,15 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do end describe "Filter Expression Structure - :all scope" do - test "Admin can read all members without filter" do + test "Admin can read all members without filter (returns expr(true))" do actor = create_actor_with_role("admin") authorizer = create_authorizer(Mv.Membership.Member, :read) filter = HasPermission.auto_filter(actor, authorizer, []) - # :all scope should return nil (no filter needed) - assert is_nil(filter) + # :all scope should return [] (empty keyword list = no filter = allow all records) + # After auto_filter fix: no longer returns nil, returns [] instead + assert filter == [] end end diff --git a/test/mv/membership/member_policies_test.exs b/test/mv/membership/member_policies_test.exs index ce1e7ce..69b0e22 100644 --- a/test/mv/membership/member_policies_test.exs +++ b/test/mv/membership/member_policies_test.exs @@ -132,6 +132,8 @@ defmodule Mv.Membership.MemberPoliciesTest do end test "can update linked member", %{user: user, linked_member: linked_member} do + # Update is allowed via HasPermission check with :linked scope (not via special case) + # The special case policy only applies to :read actions {:ok, updated_member} = linked_member |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) @@ -367,23 +369,14 @@ defmodule Mv.Membership.MemberPoliciesTest do end end - describe "special case: user can always access linked member" do - test "own_data user can read linked member even without explicit permission" do - user = create_user_with_permission_set("own_data") - linked_member = create_linked_member_for_user(user) + describe "special case: user can always READ linked member" do + # Note: The special case policy only applies to :read actions. + # Updates are handled by HasPermission with :linked scope (if permission exists). - # Reload user to get updated member_id - {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) - {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) - - # Should succeed (special case policy takes precedence) - {:ok, member} = - Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) - - assert member.id == linked_member.id - end - - test "read_only user can read linked member (via special case)" do + test "read_only user can read linked member (via special case bypass)" do + # read_only has Member.read scope :all, but the special case ensures + # users can ALWAYS read their linked member, even if they had no read permission. + # This test verifies the special case works independently of permission sets. user = create_user_with_permission_set("read_only") linked_member = create_linked_member_for_user(user) @@ -391,14 +384,16 @@ defmodule Mv.Membership.MemberPoliciesTest do {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) - # Should succeed (special case policy takes precedence) + # Should succeed (special case bypass policy for :read takes precedence) {:ok, member} = Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) assert member.id == linked_member.id end - test "own_data user can update linked member even without explicit permission" do + test "own_data user can read linked member (via special case bypass)" do + # own_data has Member.read scope :linked, but the special case ensures + # users can ALWAYS read their linked member regardless of permission set. user = create_user_with_permission_set("own_data") linked_member = create_linked_member_for_user(user) @@ -406,7 +401,24 @@ defmodule Mv.Membership.MemberPoliciesTest do {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) - # Should succeed (special case policy takes precedence) + # Should succeed (special case bypass policy for :read takes precedence) + {:ok, member} = + Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) + + assert member.id == linked_member.id + end + + test "own_data user can update linked member (via HasPermission :linked scope)" do + # Update is NOT handled by special case - it's handled by HasPermission + # with :linked scope. own_data has Member.update scope :linked. + user = create_user_with_permission_set("own_data") + linked_member = create_linked_member_for_user(user) + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) + + # Should succeed via HasPermission check (not special case) {:ok, updated_member} = linked_member |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"})