Revert "improvement: Simplify forbidden_due_to_strict_policy check (#2400)"

This reverts commit 4d58a06.

Author: zachdaniel

lib/ash/policy/authorizer/authorizer.ex

@@ -1810,32 +1810,122 @@ defmodule Ash.Policy.Authorizer do
          authorizer.action.type != :read do
       true
     else
-      check_context = %{resource: authorizer.resource}
-
-      {any_strict_error?, _authorizer} =
+      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.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
-
-            {cond_expr, authorizer} =
-              Policy.expand_constants(cond_expr, authorizer, check_context)
-
-            {complete_expr, authorizer} =
-              Policy.expand_constants(complete_expr, authorizer, check_context)
+        |> 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
+              )
 
-            case {cond_expr, complete_expr} do
-              {true, false} when strict? -> {:halt, {true, authorizer}}
-              {true, _} -> {:cont, {false, authorizer}}
-              _ -> {:cont, {acc, authorizer}}
+            if all_conditions_true? and policy_fails_statically?(updated_authorizer, policy) do
+              {:halt, true}
+            else
+              {:cont, false}
             end
+          else
+            {:cont, false}
+          end
         end)
-
-      any_strict_error?
+      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}}
+
+            {: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}}
+
+            {:ok, _, updated_auth} ->
+              {:cont, {status, updated_auth}}
+
+            {:error, updated_auth} ->
+              {:halt, {:forbidden, updated_auth}}
+          end
+
+        :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}}
+
+            {:error, updated_auth} ->
+              {:halt, {:forbidden, updated_auth}}
+          end
+      end
+    end)
+    |> elem(0)
+    |> Kernel.==(:forbidden)
+  end
+
   defp get_policies(authorizer) do
     %{
       authorizer

lib/ash/policy/policy.ex

@@ -41,22 +41,28 @@ defmodule Ash.Policy.Policy do
   def expression(policies, check_context) do
     policies
     |> List.wrap()
-    |> Enum.map(&{&1, policy_expression(&1, check_context)})
+    |> 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)
     |> 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.
@@ -65,7 +71,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)
@@ -173,9 +179,10 @@ defmodule Ash.Policy.Policy do
   def fetch_or_strict_check_fact(authorizer, {check_module, opts}) do
     authorizer.facts
     |> Enum.find_value(fn
-      {{^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
+      {{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
           {:ok, result}
         end
 
@@ -261,18 +268,6 @@ 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
@@ -299,13 +294,12 @@ 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()}
-  def expand_constants(expression, authorizer, check_context) do
+  defp expand_constants(expression, authorizer, check_context) do
     {expression, authorizer} =
       Expression.expand(expression, authorizer, fn
         expr, authorizer when is_variable(expr) ->