Security: Fix critical deny-filter bug and improve authorization
All checks were successful
continuous-integration/drone/push Build is passing
All checks were successful
continuous-integration/drone/push Build is passing
CRITICAL FIX: Deny-filter was allowing all records instead of denying Fix: User validation in Member now uses actor from changeset.context
This commit is contained in:
parent
e5eb3b7e89
commit
05cbd833bc
4 changed files with 25 additions and 332 deletions
|
|
@ -298,12 +298,12 @@ defmodule Mv.Membership.Member do
|
|||
# Authorization Policies
|
||||
# Order matters: Most specific policies first, then general permission check
|
||||
policies do
|
||||
# SYSTEM OPERATIONS: Allow CRUD operations without actor
|
||||
# SYSTEM OPERATIONS: Allow CRUD operations without actor (TEST ENVIRONMENT ONLY)
|
||||
# In test: All operations allowed (for test fixtures)
|
||||
# In production: Only :create and :read allowed (enforced by NoActor.check)
|
||||
# :read is needed for internal Ash lookups (e.g., relationship validation during user creation).
|
||||
# In production/dev: ALL operations denied without actor (fail-closed for security)
|
||||
# NoActor.check uses compile-time environment detection to prevent security issues
|
||||
bypass action_type([:create, :read, :update, :destroy]) do
|
||||
description "Allow system operations without actor (seeds, tests, internal lookups)"
|
||||
description "Allow system operations without actor (test environment only)"
|
||||
authorize_if Mv.Authorization.Checks.NoActor
|
||||
end
|
||||
|
||||
|
|
@ -397,8 +397,13 @@ defmodule Mv.Membership.Member do
|
|||
user_id = user_arg[:id]
|
||||
current_member_id = changeset.data.id
|
||||
|
||||
# Get actor from changeset context for authorization
|
||||
# If no actor is present, this will fail in production (fail-closed)
|
||||
actor = Map.get(changeset.context || %{}, :actor)
|
||||
|
||||
# Check the current state of the user in the database
|
||||
case Ash.get(Mv.Accounts.User, user_id) do
|
||||
# Pass actor to ensure proper authorization (User might have policies in future)
|
||||
case Ash.get(Mv.Accounts.User, user_id, actor: actor) do
|
||||
# User is free to be linked
|
||||
{:ok, %{member_id: nil}} ->
|
||||
:ok
|
||||
|
|
|
|||
|
|
@ -131,13 +131,12 @@ defmodule Mv.Authorization.Checks.HasPermission do
|
|||
cond do
|
||||
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, []}}]
|
||||
# Return filter that never matches (id IN [] = never matches)
|
||||
deny_filter()
|
||||
|
||||
is_nil(action) ->
|
||||
# Cannot determine action - deny access (fail-closed)
|
||||
[id: {:not, {:in, []}}]
|
||||
deny_filter()
|
||||
|
||||
true ->
|
||||
auto_filter_with_permissions(actor, resource, action)
|
||||
|
|
@ -169,16 +168,23 @@ defmodule Mv.Authorization.Checks.HasPermission do
|
|||
|
||||
false ->
|
||||
# No permission - deny access (fail-closed)
|
||||
# Return filter that never matches (using impossible condition)
|
||||
[id: {:not, {:in, []}}]
|
||||
deny_filter()
|
||||
end
|
||||
else
|
||||
_ ->
|
||||
# Error case (no role, invalid permission set, etc.) - deny access (fail-closed)
|
||||
[id: {:not, {:in, []}}]
|
||||
deny_filter()
|
||||
end
|
||||
end
|
||||
|
||||
# Helper function to return a filter that never matches (deny all records)
|
||||
# Used when authorization should be denied (fail-closed)
|
||||
# Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all)
|
||||
# NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG!
|
||||
defp deny_filter do
|
||||
[id: {:in, []}]
|
||||
end
|
||||
|
||||
# Helper to extract action type from authorizer
|
||||
# CRITICAL: Must use action_type, not action.name!
|
||||
# Action types: :create, :read, :update, :destroy
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue