Skip to content

Simplify forbidden_due_to_strict_policy check #2400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 19, 2025
Merged

Simplify forbidden_due_to_strict_policy check #2400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 19 additions & 109 deletions lib/ash/policy/authorizer/authorizer.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1810,120 +1810,30 @@ defmodule Ash.Policy.Authorizer do
authorizer.action.type != :read do
true
else
if Enum.any?(authorizer.policies, fn policy ->
{all_conditions_true?, _authorizer} =
Enum.reduce_while(policy.condition || [], {true, authorizer}, fn
{check_module, check_opts}, {_acc, auth} ->
case Policy.fetch_or_strict_check_fact(auth, {check_module, check_opts}) do
{:ok, true, updated_auth} ->
{:cont, {true, updated_auth}}

_ ->
{:halt, {false, auth}}
end
end)

all_conditions_true?
end) do
authorizer.policies
|> Enum.reduce_while(false, fn policy, _acc ->
if policy.access_type == :strict do
{all_conditions_true?, updated_authorizer} =
Enum.reduce_while(
policy.condition || [],
{true, authorizer},
fn {check_module, check_opts}, {_acc, auth} ->
case Policy.fetch_or_strict_check_fact(auth, {check_module, check_opts}) do
{:ok, true, updated_auth} ->
{:cont, {true, updated_auth}}

_ ->
{:halt, {false, auth}}
end
end
)

if all_conditions_true? and policy_fails_statically?(updated_authorizer, policy) do
{:halt, true}
else
{:cont, false}
end
else
{:cont, false}
end
end)
else
true
end
end
end

defp policy_fails_statically?(authorizer, policy) do
Enum.reduce_while(policy.policies, {:forbidden, authorizer}, fn check, {status, auth} ->
case check.type do
:authorize_if ->
case Policy.fetch_or_strict_check_fact(
auth,
{check.check_module, check.check_opts}
) do
{:ok, true, updated_auth} ->
{:halt, {:authorized, updated_auth}}

{:ok, _, updated_auth} ->
{:cont, {status, updated_auth}}

{:error, updated_auth} ->
{:halt, {:forbidden, updated_auth}}
end

:forbid_if ->
case Policy.fetch_or_strict_check_fact(
auth,
{check.check_module, check.check_opts}
) do
{:ok, true, updated_auth} ->
{:halt, {:forbidden, updated_auth}}
check_context = %{resource: authorizer.resource}

{:ok, _, updated_auth} ->
{:cont, {status, updated_auth}}

{:error, updated_auth} ->
{:halt, {:forbidden, updated_auth}}
end

:authorize_unless ->
case Policy.fetch_or_strict_check_fact(
auth,
{check.check_module, check.check_opts}
) do
{:ok, false, updated_auth} ->
{:halt, {:authorized, updated_auth}}
{any_strict_error?, _authorizer} =
authorizer.policies
|> Enum.map(&{&1, Policy.policy_expression(&1, check_context)})
|> Enum.reduce_while({true, authorizer}, fn
{policy, {cond_expr, complete_expr}}, {acc, authorizer} ->
strict? = policy.access_type == :strict

{:ok, _, updated_auth} ->
{:cont, {status, updated_auth}}
{cond_expr, authorizer} =
Policy.expand_constants(cond_expr, authorizer, check_context)

{:error, updated_auth} ->
{:halt, {:forbidden, updated_auth}}
end
{complete_expr, authorizer} =
Policy.expand_constants(complete_expr, authorizer, check_context)

:forbid_unless ->
case Policy.fetch_or_strict_check_fact(
auth,
{check.check_module, check.check_opts}
) do
{:ok, false, updated_auth} ->
{:cont, {:forbidden, updated_auth}}

{:ok, _, updated_auth} ->
{:cont, {status, updated_auth}}
case {cond_expr, complete_expr} do
{true, false} when strict? -> {:halt, {true, authorizer}}
{true, _} -> {:cont, {false, authorizer}}
_ -> {:cont, {acc, authorizer}}
end
end)

{:error, updated_auth} ->
{:halt, {:forbidden, updated_auth}}
end
end
end)
|> elem(0)
|> Kernel.==(:forbidden)
any_strict_error?
end
end

defp get_policies(authorizer) do
Expand Down
38 changes: 22 additions & 16 deletions lib/ash/policy/policy.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,28 +41,22 @@ defmodule Ash.Policy.Policy do
def expression(policies, check_context) do
policies
|> List.wrap()
|> Enum.map(fn policy ->
# Simplification is important here to detect empty field policies
cond_expr = policy |> condition_expression() |> simplify_policy_expression(check_context)
pol_expr = policies_expression(policy)
complete_expr = b(cond_expr and pol_expr)
{policy, cond_expr, complete_expr}
end)
|> Enum.map(&{&1, policy_expression(&1, check_context)})
|> List.foldr({false, true}, fn
{%{bypass?: true}, _cond_expr, complete_expr}, {one_condition_matches, true} ->
{%{bypass?: true}, {_cond_expr, complete_expr}}, {one_condition_matches, true} ->
{
b(complete_expr or one_condition_matches),
complete_expr
}

{%FieldPolicy{bypass?: true}, true, complete_expr},
{%FieldPolicy{bypass?: true}, {true, complete_expr}},
{one_condition_matches, all_policies_match} ->
{
one_condition_matches,
b(complete_expr or all_policies_match)
}

{%{bypass?: true}, _cond_expr, complete_expr},
{%{bypass?: true}, {_cond_expr, complete_expr}},
{one_condition_matches, all_policies_match} ->
{
# Bypass should only contribute to "at least one policy applies" if it actually authorizes.
Expand All @@ -71,7 +65,7 @@ defmodule Ash.Policy.Policy do
b(complete_expr or all_policies_match)
}

{%{}, cond_expr, complete_expr}, {one_condition_matches, all_policies_match} ->
{%{}, {cond_expr, complete_expr}}, {one_condition_matches, all_policies_match} ->
{
b(cond_expr or one_condition_matches),
b(implied_by(complete_expr, cond_expr) and all_policies_match)
Expand Down Expand Up @@ -179,10 +173,9 @@ defmodule Ash.Policy.Policy do
def fetch_or_strict_check_fact(authorizer, {check_module, opts}) do
authorizer.facts
|> Enum.find_value(fn
{{fact_mod, fact_opts}, result} when result != :unknown ->
if check_module == fact_mod &&
Keyword.drop(fact_opts, [:access_type, :ash_field_policy?]) ==
Keyword.drop(opts, [:access_type, :ash_field_policy?]) do
{{^check_module, fact_opts}, result} when result != :unknown ->
if Keyword.drop(fact_opts, [:access_type, :ash_field_policy?]) ==
Keyword.drop(opts, [:access_type, :ash_field_policy?]) do
{:ok, result}
end

Expand Down Expand Up @@ -268,6 +261,18 @@ defmodule Ash.Policy.Policy do
end
end

@doc false
@spec policy_expression(policy :: t() | FieldPolicy.t(), check_context :: Check.context()) ::
{Expression.t(Check.ref()), Expression.t(Check.ref())}
def policy_expression(policy, check_context) do
# Simplification is important here to detect empty field policies
cond_expr = policy |> condition_expression() |> simplify_policy_expression(check_context)
pol_expr = policies_expression(policy)
complete_expr = b(cond_expr and pol_expr)

{cond_expr, complete_expr}
end

@spec condition_expression(policy :: t() | FieldPolicy.t()) :: Expression.t(Check.ref())
defp condition_expression(%{condition: condition}) do
condition
Expand All @@ -294,12 +299,13 @@ defmodule Ash.Policy.Policy do
end)
end

@doc false
@spec expand_constants(
expression :: Expression.t(Check.ref()),
authorizer :: Authorizer.t(),
check_context :: Check.context()
) :: {Expression.t(Check.ref()), Authorizer.t()}
defp expand_constants(expression, authorizer, check_context) do
def expand_constants(expression, authorizer, check_context) do
{expression, authorizer} =
Expression.expand(expression, authorizer, fn
expr, authorizer when is_variable(expr) ->
Expand Down