From b899a6ecf3cf137a9fead062293663905462dff0 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Fri, 10 Mar 2023 13:28:16 -0500 Subject: [PATCH] improvement: optimize policy check running with laziness Implemented lazy evaluation of individual checks, so that checks that are demonstrably irrelevant when building policies are not checked at all. This will often mean no need to visit the sat solver at all, or only with a very minimal set of filter checks. --- lib/ash/api/api.ex | 2 +- lib/ash/authorizer.ex | 2 +- lib/ash/engine/request.ex | 2 +- lib/ash/error/forbidden/policy.ex | 3 +- lib/ash/policy/authorizer.ex | 54 +-- lib/ash/policy/checker.ex | 7 +- lib/ash/policy/info.ex | 6 +- lib/ash/policy/policy.ex | 543 +++++++++++++++++++++--------- lib/ash/policy/sat_solver.ex | 28 +- test/support/authorizer.ex | 5 +- 10 files changed, 451 insertions(+), 201 deletions(-) diff --git a/lib/ash/api/api.ex b/lib/ash/api/api.ex index 002cf6e4..2eb31479 100644 --- a/lib/ash/api/api.ex +++ b/lib/ash/api/api.ex @@ -537,7 +537,7 @@ defmodule Ash.Api do {:error, error} -> {:halt, {:error, error}} - :authorized -> + {:authorized, _} -> {:cont, {true, query}} {:filter, _authorizer, filter} -> diff --git a/lib/ash/authorizer.ex b/lib/ash/authorizer.ex index 83f2b352..0a5e6fdb 100644 --- a/lib/ash/authorizer.ex +++ b/lib/ash/authorizer.ex @@ -17,7 +17,7 @@ defmodule Ash.Authorizer do ) :: state @callback strict_check_context(state) :: [atom] @callback strict_check(state, context) :: - :authorized + {:authorized, state} | {:continue, state} | {:filter, Keyword.t()} | {:filter, Keyword.t(), state} diff --git a/lib/ash/engine/request.ex b/lib/ash/engine/request.ex index 1f078034..2e411d25 100644 --- a/lib/ash/engine/request.ex +++ b/lib/ash/engine/request.ex @@ -552,7 +552,7 @@ defmodule Ash.Engine.Request do case missing_strict_check_dependencies?(authorizer, request) do [] -> case strict_check_authorizer(authorizer, request) do - :authorized -> + {:authorized, _authorizer} -> {:ok, set_authorizer_state(request, authorizer, :authorized), notifications, []} {:filter, authorizer_state, filter} -> diff --git a/lib/ash/error/forbidden/policy.ex b/lib/ash/error/forbidden/policy.ex index 697b5038..40bbc287 100644 --- a/lib/ash/error/forbidden/policy.ex +++ b/lib/ash/error/forbidden/policy.ex @@ -317,7 +317,8 @@ defmodule Ash.Error.Forbidden.Policy do "⛔" {:unknown, :unknown} -> - if success? && filter_check? && Policy.fetch_fact(facts, check.check) == :error do + if (success? && filter_check? && Policy.fetch_fact(facts, check.check) == :error) || + {:ok, :unknown} do "🔎" else "⬇" diff --git a/lib/ash/policy/authorizer.ex b/lib/ash/policy/authorizer.ex index b442b2e4..46e84b07 100644 --- a/lib/ash/policy/authorizer.ex +++ b/lib/ash/policy/authorizer.ex @@ -369,24 +369,21 @@ defmodule Ash.Policy.Authorizer do api: context.api } |> get_policies() - |> do_strict_check_facts() + |> strict_check_result() |> case do - {:ok, authorizer} -> - case strict_check_result(authorizer) do - :authorized -> - log_successful_policy_breakdown(authorizer) - :authorized + {:authorized, authorizer} -> + log_successful_policy_breakdown(authorizer) + {:authorized, authorizer} - {:filter, authorizer, filter} -> - log_successful_policy_breakdown(authorizer, filter) - {:filter, authorizer, filter} - - other -> - other - end + {:filter, authorizer, filter} -> + log_successful_policy_breakdown(authorizer, filter) + {:filter, authorizer, filter} {:error, error} -> {:error, error} + + {other, _authorizer} -> + other end end @@ -409,11 +406,11 @@ defmodule Ash.Policy.Authorizer do case {filter, require_check} do {[], []} -> - :authorized + {:authorized, authorizer} {_filters, []} -> if Enum.any?(filter, &(&1 == true)) do - :authorized + {:authorized, authorizer} else case filter do [filter] -> @@ -759,7 +756,20 @@ defmodule Ash.Policy.Authorizer do defp strict_check_result(authorizer) do case Checker.strict_check_scenarios(authorizer) do - {:ok, scenarios} -> + {:ok, true, authorizer} -> + {:authorized, authorizer} + + {:ok, false, authorizer} -> + {:error, + Ash.Error.Forbidden.Policy.exception( + facts: authorizer.facts, + policies: authorizer.policies, + resource: Map.get(authorizer, :resource), + action: Map.get(authorizer, :action), + scenarios: [] + )} + + {:ok, scenarios, authorizer} -> report_scenarios(authorizer, scenarios, "Potential Scenarios") case Checker.find_real_scenarios(scenarios, authorizer.facts) do @@ -768,7 +778,7 @@ defmodule Ash.Policy.Authorizer do real_scenarios -> report_scenarios(authorizer, real_scenarios, "Real Scenarios") - :authorized + {:authorized, authorizer} end {:error, :unsatisfiable} -> @@ -788,16 +798,6 @@ defmodule Ash.Policy.Authorizer do strict_filter(%{authorizer | scenarios: scenarios}) end - defp do_strict_check_facts(authorizer) do - case Checker.strict_check_facts(authorizer) do - {:ok, authorizer, new_facts} -> - {:ok, %{authorizer | facts: new_facts}} - - {:error, error} -> - {:error, error} - end - end - defp get_policies(authorizer) do %{ authorizer diff --git a/lib/ash/policy/checker.ex b/lib/ash/policy/checker.ex index e58ba1cd..c245bed7 100644 --- a/lib/ash/policy/checker.ex +++ b/lib/ash/policy/checker.ex @@ -117,11 +117,14 @@ defmodule Ash.Policy.Checker do def strict_check_scenarios(authorizer) do case Ash.Policy.Policy.solve(authorizer) do - {:ok, scenarios} -> + {:ok, value, authorizer} when is_boolean(value) -> + {:ok, value, authorizer} + + {:ok, scenarios, authorizer} -> {:ok, scenarios |> Ash.Policy.SatSolver.simplify_clauses() - |> remove_scenarios_with_impossible_facts(authorizer)} + |> remove_scenarios_with_impossible_facts(authorizer), authorizer} {:error, :unsatisfiable} -> {:error, :unsatisfiable} diff --git a/lib/ash/policy/info.ex b/lib/ash/policy/info.ex index 0a846d64..baff7857 100644 --- a/lib/ash/policy/info.ex +++ b/lib/ash/policy/info.ex @@ -49,7 +49,7 @@ defmodule Ash.Policy.Info do {:error, _error} -> false - :authorized -> + {:authorized, _} -> true {:filter, _, _} -> @@ -78,7 +78,7 @@ defmodule Ash.Policy.Info do {:error, _error} -> false - :authorized -> + {:authorized, _} -> true {:filter, _, _} -> @@ -242,7 +242,7 @@ defmodule Ash.Policy.Info do end defp run_check(actor, query, api: api, maybe_is: maybe_is) do - case Ash.Policy.Info.strict_check(actor, query, api) do + case strict_check(actor, query, api) do true -> true diff --git a/lib/ash/policy/policy.ex b/lib/ash/policy/policy.ex index a8afdb60..376ed851 100644 --- a/lib/ash/policy/policy.ex +++ b/lib/ash/policy/policy.ex @@ -23,90 +23,158 @@ defmodule Ash.Policy.Policy do def solve(authorizer) do authorizer.policies - |> build_requirements_expression(authorizer.facts) - |> Ash.Policy.SatSolver.solve(fn scenario, bindings -> - scenario - |> Ash.SatSolver.solutions_to_predicate_values(bindings) - |> Map.drop(@static_checks) - end) + |> build_requirements_expression(authorizer) + |> case do + {true, authorizer} -> + {:ok, true, authorizer} + + {false, authorizer} -> + {:ok, false, authorizer} + + {expression, authorizer} -> + Ash.Policy.SatSolver.solve(expression, fn scenario, bindings -> + scenario + |> Ash.SatSolver.solutions_to_predicate_values(bindings) + |> Map.drop(@static_checks) + end) + |> case do + {:ok, scenarios} -> + {:ok, scenarios, authorizer} + + other -> + other + end + end end - defp build_requirements_expression(policies, facts) do - at_least_one_policy_expression = at_least_one_policy_expression(policies, facts) + defp build_requirements_expression(policies, authorizer) do + {at_least_one_policy_expression, authorizer} = + at_least_one_policy_expression(policies, authorizer) if at_least_one_policy_expression == false do - false + {false, authorizer} else - policy_expression = + {policy_expression, authorizer} = if at_least_one_policy_expression == true do - compile_policy_expression(policies, facts) + compile_policy_expression(policies, authorizer) else - case {:and, at_least_one_policy_expression, compile_policy_expression(policies, facts)} do + {policy_expression, authorizer} = compile_policy_expression(policies, authorizer) + + case {:and, at_least_one_policy_expression, policy_expression} do {:and, false, _} -> - false + {false, authorizer} {:and, _, false} -> - false + {false, authorizer} {:and, true, true} -> - true + {true, authorizer} {:and, left, true} -> - left + {left, authorizer} {:and, true, right} -> - right + {right, authorizer} other -> - other + {other, authorizer} end end - used_facts = used_facts(policy_expression) - facts_expression = - facts + authorizer.facts |> Map.drop([true, false]) - |> Map.take(MapSet.to_list(used_facts)) |> Ash.Policy.SatSolver.facts_to_statement() if facts_expression do - {:and, facts_expression, policy_expression} + {{:and, facts_expression, policy_expression}, authorizer} else - policy_expression + {policy_expression, authorizer} end end end - defp used_facts({_op, l, r}) do - MapSet.union(used_facts(l), used_facts(r)) - end - - defp used_facts({:not, fact}) do - used_facts(fact) - end - - defp used_facts(other) do - MapSet.new([other]) - end - - def at_least_one_policy_expression(policies, facts) do + def at_least_one_policy_expression(policies, authorizer) do policies - |> Enum.map(&condition_expression(&1.condition, facts)) - |> Enum.filter(& &1) - |> Enum.reduce(false, fn - _, true -> - true + |> Enum.reduce({[], authorizer}, fn + policy, {condition_exprs, authorizer} when is_list(condition_exprs) -> + case condition_expression(policy.condition, authorizer) do + {nil, authorizer} -> + {condition_exprs, authorizer} - true, _ -> - true + {true, authorizer} -> + {true, authorizer} - false, acc -> - acc + {condition_expr, authorizer} -> + {[condition_expr | condition_exprs], authorizer} + end - condition, acc -> - {:or, condition, acc} + _, {true, authorizer} -> + {true, authorizer} end) + |> then(fn + {true, authorizer} -> + {true, authorizer} + + {condition_exprs, authorizer} -> + {condition_exprs + |> Enum.reduce(false, fn + _, true -> + true + + true, _ -> + true + + false, acc -> + acc + + condition, acc -> + {:or, condition, acc} + end), authorizer} + end) + end + + def fetch_or_strict_check_fact(authorizer, %{check_module: mod, check_opts: opts}) do + fetch_or_strict_check_fact(authorizer, {mod, opts}) + end + + def fetch_or_strict_check_fact(authorizer, {check_module, opts}) do + Enum.find_value(authorizer.facts, fn + {{fact_mod, fact_opts}, result} -> + if check_module == fact_mod && + Keyword.delete(fact_opts, :access_type) == + Keyword.delete(opts, :access_type) do + {:ok, result} + end + + _ -> + nil + end) + |> case do + nil -> + case check_module.strict_check(authorizer.actor, authorizer, opts) do + {:ok, value} when is_boolean(value) or value == :unknown -> + authorizer = %{ + authorizer + | facts: Map.put(authorizer.facts, {check_module, opts}, value) + } + + if value == :unknown do + {:error, authorizer} + else + {:ok, value, authorizer} + end + + {:error, error} -> + raise "Error produced by #{check_module}'s strict_checking logic: #{inspect(error)}" + end + + {:ok, :unknown} -> + {:error, authorizer} + + {:ok, value} -> + {:ok, value, authorizer} + end end def fetch_fact(facts, %{check_module: mod, check_opts: opts}) do @@ -132,72 +200,77 @@ defmodule Ash.Policy.Policy do nil -> :error + :unknown -> + :error + value -> value end end - defp condition_expression(condition, facts) do + defp condition_expression(condition, authorizer) do condition |> List.wrap() - |> Enum.reduce(nil, fn - condition, nil -> - case fetch_fact(facts, condition) do - {:ok, true} -> - true + |> Enum.reduce({nil, authorizer}, fn + condition, {nil, authorizer} -> + case fetch_or_strict_check_fact(authorizer, condition) do + {:ok, true, authorizer} -> + {true, authorizer} - {:ok, false} -> - false + {:ok, false, authorizer} -> + {false, authorizer} _ -> - condition + {condition, authorizer} end - _condition, false -> - false + _condition, {false, authorizer} -> + {false, authorizer} - condition, expression -> - case fetch_fact(facts, condition) do - {:ok, true} -> - expression + condition, {expression, authorizer} -> + case fetch_or_strict_check_fact(authorizer, condition) do + {:ok, true, authorizer} -> + {expression, authorizer} - {:ok, false} -> - false + {:ok, false, authorizer} -> + {false, authorizer} _ -> - {:and, condition, expression} + {{:and, condition, expression}, authorizer} end end) end - defp compile_policy_expression(policies, facts) - - defp compile_policy_expression([], _facts) do - false + defp compile_policy_expression([], authorizer) do + {false, authorizer} end defp compile_policy_expression( [%__MODULE__{condition: condition, policies: policies}], - facts + authorizer ) do - compiled_policies = compile_policy_expression(policies, facts) - condition_expression = condition_expression(condition, facts) + {condition_expression, authorizer} = condition_expression(condition, authorizer) case condition_expression do true -> - compiled_policies + compile_policy_expression(policies, authorizer) false -> - true + {true, authorizer} nil -> - compiled_policies + compile_policy_expression(policies, authorizer) condition_expression -> - if compiled_policies == true do - condition_expression - else - {:and, condition_expression, compiled_policies} + case compile_policy_expression(policies, authorizer) do + {true, authorizer} -> + {condition_expression, authorizer} + + {false, authorizer} -> + {{:not, condition_expression}, authorizer} + + {compiled_policies, authorizer} -> + {{:and, condition_expression, compiled_policies}, authorizer} end end end @@ -206,149 +279,315 @@ defmodule Ash.Policy.Policy do [ %__MODULE__{condition: condition, policies: policies, bypass?: bypass?} | rest ], - facts + authorizer ) do - condition_expression = condition_expression(condition, facts) + {condition_expression, authorizer} = condition_expression(condition, authorizer) case condition_expression do true -> if bypass? do - {:or, compile_policy_expression(policies, facts), - {:and, compile_policy_expression(rest, facts), - at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), facts)}} + case compile_policy_expression(policies, authorizer) do + {true, authorizer} -> + {true, authorizer} + + {false, authorizer} -> + {at_least_one_policy_expression, authorizer} = + at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), authorizer) + + {rest, authorizer} = compile_policy_expression(rest, authorizer) + + {{:and, rest, at_least_one_policy_expression}, authorizer} + + {policy_expression, authorizer} -> + {at_least_one_policy_expression, authorizer} = + at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), authorizer) + + {rest, authorizer} = compile_policy_expression(rest, authorizer) + {{:or, policy_expression, {:and, rest, at_least_one_policy_expression}}, authorizer} + end else - {:and, compile_policy_expression(policies, facts), - compile_policy_expression(rest, facts)} + case compile_policy_expression(policies, authorizer) do + {false, authorizer} -> + {false, authorizer} + + {true, authorizer} -> + compile_policy_expression(rest, authorizer) + + {policy_expression, authorizer} -> + case compile_policy_expression(rest, authorizer) do + {false, authorizer} -> + {false, authorizer} + + {true, authorizer} -> + {policy_expression, authorizer} + + {rest, authorizer} -> + {{:and, policy_expression, rest}, authorizer} + end + end end false -> - compile_policy_expression(rest, facts) + compile_policy_expression(rest, authorizer) nil -> if bypass? do - {:or, compile_policy_expression(policies, facts), - {:and, compile_policy_expression(rest, facts), - at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), facts)}} + case compile_policy_expression(policies, authorizer) do + {true, authorizer} -> + {true, authorizer} + + {false, authorizer} -> + {at_least_one_policy_expression, authorizer} = + at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), authorizer) + + rest = compile_policy_expression(rest, authorizer) + {:and, rest, at_least_one_policy_expression} + + {policy_expression, authorizer} -> + {at_least_one_policy_expression, authorizer} = + at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), authorizer) + + case compile_policy_expression(rest, authorizer) do + {false, authorizer} -> + {policy_expression, authorizer} + + {true, authorizer} -> + {{:or, policy_expression, at_least_one_policy_expression}, authorizer} + + {rest, authorizer} -> + {{:or, policy_expression, {:and, rest, at_least_one_policy_expression}}, + authorizer} + end + end else - {:and, compile_policy_expression(policies, facts), - compile_policy_expression(rest, facts)} + case compile_policy_expression(policies, authorizer) do + {true, authorizer} -> + compile_policy_expression(rest, authorizer) + + {false, authorizer} -> + {false, authorizer} + + {policy_expression, authorizer} -> + case compile_policy_expression(rest, authorizer) do + {true, authorizer} -> + {policy_expression, authorizer} + + {false, authorizer} -> + {false, authorizer} + + {rest, authorizer} -> + {{:and, policy_expression, rest}, authorizer} + end + end end condition_expression -> if bypass? do - {:or, {:and, condition_expression, compile_policy_expression(policies, facts)}, - {:and, compile_policy_expression(rest, facts), - at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), facts)}} + {at_least_one_policy_expression, authorizer} = + at_least_one_policy_expression(Enum.take_while(rest, &(!&1.bypass?)), authorizer) + + {condition_and_policy_expression, authorizer} = + case compile_policy_expression(policies, authorizer) do + {true, authorizer} -> + {condition_expression, authorizer} + + {false, authorizer} -> + {false, authorizer} + + {other, authorizer} -> + {{:and, condition_expression, other}, authorizer} + end + + case condition_and_policy_expression do + true -> + {true, authorizer} + + false -> + case compile_policy_expression(rest, authorizer) do + {true, authorizer} -> + {at_least_one_policy_expression, authorizer} + + {false, authorizer} -> + {false, authorizer} + + {rest, authorizer} -> + {{:and, rest, at_least_one_policy_expression}, authorizer} + end + + condition_and_policy_expression -> + case compile_policy_expression(rest, authorizer) do + {true, authorizer} -> + {at_least_one_policy_expression, authorizer} + + {false, authorizer} -> + {condition_and_policy_expression, authorizer} + + {rest, authorizer} -> + {{:or, condition_and_policy_expression, + {:and, rest, at_least_one_policy_expression}}, authorizer} + end + end else - {:or, {:and, condition_expression, compile_policy_expression(policies, facts)}, - {:and, {:not, condition_expression}, compile_policy_expression(rest, facts)}} + {condition_and_policy_expression, authorizer} = + case compile_policy_expression(policies, authorizer) do + {true, authorizer} -> + {condition, authorizer} + + {false, authorizer} -> + {false, authorizer} + + {policy_expression, authorizer} -> + {{:and, condition, policy_expression}, authorizer} + end + + case condition_and_policy_expression do + false -> + compile_policy_expression(rest, authorizer) + + true -> + {true, authorizer} + + condition_and_policy_expression -> + case compile_policy_expression(rest, authorizer) do + {true, authorizer} -> + {true, authorizer} + + {false, authorizer} -> + {condition_and_policy_expression, authorizer} + + {rest, authorizer} -> + {{:or, condition_and_policy_expression, rest}, authorizer} + end + end end end end defp compile_policy_expression( [%{type: :authorize_if} = clause], - facts + authorizer ) do - case fetch_fact(facts, clause) do - {:ok, true} -> - true + case fetch_or_strict_check_fact(authorizer, clause) do + {:ok, true, authorizer} -> + {true, authorizer} - {:ok, false} -> - false + {:ok, false, authorizer} -> + {false, authorizer} - :error -> - {clause.check_module, clause.check_opts} + {:error, authorizer} -> + {{clause.check_module, clause.check_opts}, authorizer} end end defp compile_policy_expression( [%{type: :authorize_if} = clause | rest], - facts + authorizer ) do - case fetch_fact(facts, clause) do - {:ok, true} -> - true + case fetch_or_strict_check_fact(authorizer, clause) do + {:ok, true, authorizer} -> + {true, authorizer} - {:ok, false} -> - compile_policy_expression(rest, facts) + {:ok, false, authorizer} -> + compile_policy_expression(rest, authorizer) - :error -> - {:or, {clause.check_module, clause.check_opts}, compile_policy_expression(rest, facts)} + {:error, authorizer} -> + {rest, authorizer} = compile_policy_expression(rest, authorizer) + {{:or, {clause.check_module, clause.check_opts}, rest}, authorizer} end end defp compile_policy_expression( [%{type: :authorize_unless} = clause], - facts + authorizer ) do - case fetch_fact(facts, clause) do - {:ok, true} -> - false + case fetch_or_strict_check_fact(authorizer, clause) do + {:ok, true, authorizer} -> + {false, authorizer} - {:ok, false} -> - true + {:ok, false, authorizer} -> + {true, authorizer} - :error -> - {clause.check_module, clause.check_opts} + {:error, authorizer} -> + {{clause.check_module, clause.check_opts}, authorizer} end end defp compile_policy_expression( [%{type: :authorize_unless} = clause | rest], - facts + authorizer ) do - case fetch_fact(facts, clause) do - {:ok, true} -> - compile_policy_expression(rest, facts) + case fetch_or_strict_check_fact(authorizer, clause) do + {:ok, true, authorizer} -> + compile_policy_expression(rest, authorizer) - {:ok, false} -> - true + {:ok, false, authorizer} -> + {true, authorizer} - :error -> - {:or, {:not, {clause.check_module, clause.check_opts}}, - compile_policy_expression(rest, facts)} + {:error, authorizer} -> + {rest, authorizer} = compile_policy_expression(rest, authorizer) + {{:or, {:not, {clause.check_module, clause.check_opts}}, rest}, authorizer} end end - defp compile_policy_expression([%{type: :forbid_if}], _facts) do - false + defp compile_policy_expression([%{type: :forbid_if}], authorizer) do + {false, authorizer} end defp compile_policy_expression( [%{type: :forbid_if} = clause | rest], - facts + authorizer ) do - case fetch_fact(facts, clause) do - {:ok, true} -> - false + case fetch_or_strict_check_fact(authorizer, clause) do + {:ok, true, authorizer} -> + {false, authorizer} - {:ok, false} -> - compile_policy_expression(rest, facts) + {:ok, false, authorizer} -> + compile_policy_expression(rest, authorizer) - :error -> - {:and, {:not, {clause.check_module, clause.check_opts}}, - compile_policy_expression(rest, facts)} + {:error, authorizer} -> + {rest, authorizer} = compile_policy_expression(rest, authorizer) + + case rest do + true -> + {{:not, {clause.check_module, clause.check_opts}}, authorizer} + + false -> + {false, authorizer} + + rest -> + {{:and, {:not, {clause.check_module, clause.check_opts}}, rest}, authorizer} + end end end - defp compile_policy_expression([%{type: :forbid_unless}], _facts) do - false + defp compile_policy_expression([%{type: :forbid_unless}], authorizer) do + {false, authorizer} end defp compile_policy_expression( [%{type: :forbid_unless} = clause | rest], - facts + authorizer ) do - case fetch_fact(facts, clause) do - {:ok, true} -> - compile_policy_expression(rest, facts) + case fetch_or_strict_check_fact(authorizer, clause) do + {:ok, true, authorizer} -> + compile_policy_expression(rest, authorizer) - {:ok, false} -> - false + {:ok, false, authorizer} -> + {false, authorizer} - :error -> - {:and, {clause.check_module, clause.check_opts}, compile_policy_expression(rest, facts)} + {:error, authorizer} -> + {rest, authorizer} = compile_policy_expression(rest, authorizer) + + case rest do + false -> + {false, authorizer} + + true -> + {{clause.check_module, clause.check_opts}, authorizer} + + rest -> + {{:and, {clause.check_module, clause.check_opts}, rest}, authorizer} + end end end end diff --git a/lib/ash/policy/sat_solver.ex b/lib/ash/policy/sat_solver.ex index 632ada47..e1b98905 100644 --- a/lib/ash/policy/sat_solver.ex +++ b/lib/ash/policy/sat_solver.ex @@ -109,19 +109,23 @@ defmodule Ash.Policy.SatSolver do end def facts_to_statement(facts) do - Enum.reduce(facts, nil, fn {fact, true?}, expr -> - expr_component = - if true? do - fact - else - {:not, fact} - end + Enum.reduce(facts, nil, fn + {_fact, :unknown}, expr -> + expr - if expr do - {:and, expr, expr_component} - else - expr_component - end + {fact, true?}, expr -> + expr_component = + if true? do + fact + else + {:not, fact} + end + + if expr do + {:and, expr, expr_component} + else + expr_component + end end) end diff --git a/test/support/authorizer.ex b/test/support/authorizer.ex index fa2c931f..27e463ba 100644 --- a/test/support/authorizer.ex +++ b/test/support/authorizer.ex @@ -27,7 +27,7 @@ defmodule Ash.Test.Authorizer do def strict_check_context(_), do: get(:strict_check_context, []) def strict_check(state, _), - do: get(:strict_check_result, :authorized) |> continue(state) + do: get(:strict_check_result, :authorized) |> continue(state) |> wrap_authorized(state) def check_context(_), do: [] @@ -36,6 +36,9 @@ defmodule Ash.Test.Authorizer do defp continue(:continue, state), do: {:continue, state} defp continue(other, _), do: other + defp wrap_authorized(:authorized, state), do: {:authorized, state} + defp wrap_authorized(other, _), do: other + defp get(key, default) do Agent.get(__MODULE__, &Map.get(&1, key)) || default catch