Fix: HasPermission auto_filter and strict_check implementation

Fixes security issue where auto_filter returned nil instead of proper
filter expressions, which could lead to incorrect authorization behavior.
This commit is contained in:
Moritz 2026-01-08 22:54:47 +01:00
parent 9d58c9d1ef
commit 6cd18545bd
Signed by: moritz
GPG key ID: 1020A035E5DD0824
3 changed files with 83 additions and 38 deletions

View file

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