improvement: optimize various solver boolean optimizations

improvement: more comprehensively remove unnecessary clauses
fix: resolve issue with `authorize_unless` and filter checks
improvement: prevent changing attributes and arguments after action validation

We allow for these changes inside of `before_action` calls, but otherwise
require that `force_change_attribute` is used, for example. This prevents
accidentally validating a changeset and then changing an attribute.

documentation/topics/policies.md

@@ -74,7 +74,7 @@ We check those from top to bottom, so the first one of those that returns `:auth
 ```elixir
 authorize_if IsSuperUser # if this is true
 
-# None of the rest of them matter matter
+# None of the rest of them matter
 forbid_if Deactivated 
 authorize_if IsAdminUser
 forbid_if RegularUserCanCreate

lib/ash/actions/read.ex

@@ -1194,6 +1194,8 @@ defmodule Ash.Actions.Read do
   end
 
   defp run_before_action(query) do
+    query = Ash.Query.put_context(query, :private, %{in_before_action?: true})
+
     query.before_action
     |> Enum.reduce({query, []}, fn before_action, {query, notifications} ->
       case before_action.(query) do
@@ -1204,6 +1206,9 @@ defmodule Ash.Actions.Read do
           {query, notifications}
       end
     end)
+    |> then(fn {query, notifications} ->
+      {Ash.Query.put_context(query, :private, %{in_before_action?: false}), notifications}
+    end)
   end
 
   defp run_after_action(query, results) do

lib/ash/changeset/changeset.ex

@@ -142,6 +142,49 @@ defmodule Ash.Changeset do
   require Ash.Tracer
   require Logger
 
+  defmacrop maybe_already_validated_error!(changeset, alternative \\ nil) do
+    {function, arity} = __CALLER__.function
+
+    if alternative do
+      quote do
+        changeset = unquote(changeset)
+
+        if changeset.__validated_for_action__ && !changeset.context[:private][:in_before_action?] do
+          raise ArgumentError, """
+          Changeset has already been validated for action #{inspect(changeset.__validated_for_action__)}.
+
+          For safety, we prevent any changes after that point because they will bypass validations or other action logic.. To proceed anyway,
+          you can use `#{unquote(alternative)}/#{unquote(arity)}`. However, you should prefer a pattern like the below, which makes
+          any custom changes *before* calling the action.
+
+            Resource
+            |> Ash.Changeset.new()
+            |> Ash.Changeset.#{unquote(function)}(...)
+            |> Ash.Changeset.for_create(...)
+          """
+        end
+      end
+    else
+      quote do
+        changeset = unquote(changeset)
+
+        if changeset.__validated_for_action__ && !changeset.context[:private][:in_before_action?] do
+          raise ArgumentError, """
+          Changeset has already been validated for action #{inspect(changeset.__validated_for_action__)}.
+
+          For safety, we prevent any changes using `#{unquote(function)}/#{unquote(arity)}` after that point because they will bypass validations or other action logic.
+          Instead, you should change or set this value before calling the action, like so:
+
+            Resource
+            |> Ash.Changeset.new()
+            |> Ash.Changeset.#{unquote(function)}(...)
+            |> Ash.Changeset.for_create(...)
+          """
+        end
+      end
+    end
+  end
+
   @doc """
   Returns a new changeset over a resource. Prefer `for_action` or `for_create`, etc. over this function if possible.
 
@@ -510,7 +553,6 @@ defmodule Ash.Changeset do
               |> set_authorize(opts)
               |> set_tracer(opts)
               |> set_tenant(opts[:tenant] || changeset.tenant)
-              |> Map.put(:__validated_for_action__, action.name)
               |> cast_params(action, params)
               |> set_argument_defaults(action)
               |> require_arguments(action)
@@ -523,6 +565,7 @@ defmodule Ash.Changeset do
               )
               |> add_validations(opts[:tracer], metadata, opts[:actor])
               |> mark_validated(action.name)
+              |> Map.put(:__validated_for_action__, action.name)
             end
           end
         end
@@ -646,7 +689,6 @@ defmodule Ash.Changeset do
               |> set_tenant(
                 opts[:tenant] || changeset.tenant || changeset.data.__metadata__[:tenant]
               )
-              |> Map.put(:__validated_for_action__, action.name)
               |> cast_params(action, params || %{})
               |> set_argument_defaults(action)
               |> require_arguments(action)
@@ -663,6 +705,7 @@ defmodule Ash.Changeset do
               |> add_validations(opts[:tracer], metadata, opts[:actor])
               |> mark_validated(action.name)
               |> eager_validate_identities()
+              |> Map.put(:__validated_for_action__, action.name)
 
             if Keyword.get(opts, :require?, true) do
               require_values(changeset, action.type)
@@ -1442,6 +1485,8 @@ defmodule Ash.Changeset do
   end
 
   defp run_around_actions(%{around_action: []} = changeset, func) do
+    changeset = put_context(changeset, :private, %{in_before_action?: true})
+
     {changeset, %{notifications: before_action_notifications}} =
       Enum.reduce_while(
         changeset.before_action,
@@ -1499,6 +1544,8 @@ defmodule Ash.Changeset do
         end
       )
 
+    changeset = put_context(changeset, :private, %{in_before_action?: false})
+
     case func.(changeset) do
       {:ok, result, instructions} ->
         run_after_actions(
@@ -2568,6 +2615,8 @@ defmodule Ash.Changeset do
   @doc "Change an attribute only if is not currently being changed"
   @spec change_new_attribute(t(), atom, term) :: t()
   def change_new_attribute(changeset, attribute, value) do
+    maybe_already_validated_error!(changeset, :force_change_new_attribute)
+
     if changing_attribute?(changeset, attribute) do
       changeset
     else
@@ -2583,6 +2632,8 @@ defmodule Ash.Changeset do
   """
   @spec change_new_attribute_lazy(t(), atom, (() -> any)) :: t()
   def change_new_attribute_lazy(changeset, attribute, func) do
+    maybe_already_validated_error!(changeset, :force_change_new_attribute_lazy)
+
     if changing_attribute?(changeset, attribute) do
       changeset
     else
@@ -2594,6 +2645,8 @@ defmodule Ash.Changeset do
   Add an argument to the changeset, which will be provided to the action
   """
   def set_argument(changeset, argument, value) do
+    maybe_already_validated_error!(changeset, :set_argument)
+
     if changeset.action do
       argument =
         Enum.find(
@@ -2640,6 +2693,8 @@ defmodule Ash.Changeset do
   Remove an argument from the changeset
   """
   def delete_argument(changeset, argument_or_arguments) do
+    maybe_already_validated_error!(changeset)
+
     argument_or_arguments
     |> List.wrap()
     |> Enum.reduce(changeset, fn argument, changeset ->
@@ -2651,6 +2706,8 @@ defmodule Ash.Changeset do
   Merge a map of arguments to the arguments list
   """
   def set_arguments(changeset, map) do
+    maybe_already_validated_error!(changeset)
+
     Enum.reduce(map, changeset, fn {key, value}, changeset ->
       set_argument(changeset, key, value)
     end)
@@ -2687,6 +2744,8 @@ defmodule Ash.Changeset do
   @doc "Calls `change_attribute/3` for each key/value pair provided"
   @spec change_attributes(t(), map | Keyword.t()) :: t()
   def change_attributes(changeset, changes) do
+    maybe_already_validated_error!(changeset, :force_change_attributes)
+
     Enum.reduce(changes, changeset, fn {key, value}, changeset ->
       change_attribute(changeset, key, value)
     end)
@@ -2695,6 +2754,8 @@ defmodule Ash.Changeset do
   @doc "Adds a change to the changeset, unless the value matches the existing value"
   @spec change_attribute(t(), atom, any) :: t()
   def change_attribute(changeset, attribute, value) do
+    maybe_already_validated_error!(changeset, :change_attribute)
+
     case Ash.Resource.Info.attribute(changeset.resource, attribute) do
       nil ->
         error =
@@ -2794,6 +2855,8 @@ defmodule Ash.Changeset do
   """
   @spec change_default_attribute(t(), atom, any) :: t()
   def change_default_attribute(changeset, attribute, value) do
+    maybe_already_validated_error!(changeset)
+
     case Ash.Resource.Info.attribute(changeset.resource, attribute) do
       nil ->
         error =

lib/ash/error/forbidden/policy.ex

@@ -45,12 +45,14 @@ defmodule Ash.Error.Forbidden.Policy do
   A check with a `⬇` means that it didn't determine if the policy was authorized or forbidden, and so moved on to the next check.
   `🌟` and `⛔` mean that the check was responsible for producing an authorized or forbidden (respectively) status.
 
+  When viewing successful authorization breakdowns, a `🔎` means that the policy or check was enforced via a filter.
+
   If no check results in a status (they all have `⬇`) then the policy is assumed to have failed. In some cases, however, the policy
   may have just been ignored, as described above.
   """
 
   @doc """
-  Print a report of an authorization failure
+  Print a report of an authorization failure from a forbidden error
 
   Options:
 
@@ -73,47 +75,12 @@ defmodule Ash.Error.Forbidden.Policy do
               policies: policies,
               must_pass_strict_check?: must_pass_strict_check?
             } ->
-              must_pass_strict_check? =
+              get_breakdown(
-                if must_pass_strict_check? do
+                facts,
-                  """
+                filter,
-                  Scenario must pass strict check only, meaning `runtime` policies cannot be checked.
+                policies,
-
+                Keyword.put(opts, :must_pass_strict_check?, must_pass_strict_check?)
-                  This requirement is generally used for filtering on related resources, when we can't fetch those
+              )
-                  related resources to run `runtime` policies. For this reason, you generally want your primary read
-                  actions on your resources to have standard policies which can be checked statically (like `actor_attribute_equals`)
-                  in addition to filter policies, like `expr(foo == :bar)`.
-                  """
-                else
-                  ""
-                end
-
-              policy_breakdown_title =
-                if Keyword.get(opts, :help_text?, true) do
-                  ["Policy Breakdown", @help_text]
-                else
-                  "Policy Breakdown"
-                end
-
-              policy_explanation =
-                policies
-                |> Enum.filter(&relevant?(&1, facts))
-                |> Enum.map(&explain_policy(&1, facts))
-                |> Enum.intersperse("\n")
-                |> title(policy_breakdown_title, false)
-
-              filter =
-                if filter do
-                  title(
-                    "Did not match filter expression #{inspect(filter)}",
-                    "Generated Filter"
-                  )
-                else
-                  ""
-                end
-
-              [must_pass_strict_check?, filter, policy_explanation]
-              |> Enum.filter(& &1)
-              |> Enum.intersperse("\n\n")
           end)
           |> Enum.intersperse("\n\n")
 
@@ -123,6 +90,71 @@ defmodule Ash.Error.Forbidden.Policy do
     end
   end
 
+  @doc """
+  Print a report of an authorization failure from authorization information.
+
+  Options:
+
+  - `:help_text?`: Defaults to true. Displays help text at the top of the policy breakdown.
+  - `:success?`: Defaults to false. Changes the messaging/graphics around to indicate successful policy authorization.
+  - `:must_pass_strict_check?`: Defaults to false. Adds a message about this authorization requiring passing strict check.
+  """
+  def get_breakdown(facts, filter, policies, opts \\ []) do
+    must_pass_strict_check? =
+      if opts[:must_pass_strict_check?] do
+        """
+        Scenario must pass strict check only, meaning `runtime` policies cannot be checked.
+
+        This requirement is generally used for filtering on related resources, when we can't fetch those
+        related resources to run `runtime` policies. For this reason, you generally want your primary read
+        actions on your resources to have standard policies which can be checked statically (like `actor_attribute_equals`)
+        in addition to filter policies, like `expr(foo == :bar)`.
+        """
+      else
+        ""
+      end
+
+    policy_breakdown_title =
+      if Keyword.get(opts, :help_text?, true) do
+        ["Policy Breakdown", @help_text]
+      else
+        "Policy Breakdown"
+      end
+
+    policy_explanation =
+      policies
+      |> Enum.filter(&relevant?(&1, facts))
+      |> Enum.map(&explain_policy(&1, facts, opts[:success?] || false))
+      |> Enum.intersperse("\n")
+      |> title(policy_breakdown_title, false)
+
+    filter =
+      if filter do
+        title(
+          "#{nicely_formatted_filter(filter)}",
+          "Generated Filter"
+        )
+      else
+        ""
+      end
+
+    [must_pass_strict_check?, filter, policy_explanation]
+    |> Enum.filter(& &1)
+    |> Enum.intersperse("\n\n")
+  end
+
+  defp nicely_formatted_filter([{:or, list}]) when is_list(list) do
+    "(" <> Enum.map_join(list, " or ", &nicely_formatted_filter/1) <> ")"
+  end
+
+  defp nicely_formatted_filter([{:and, list}]) when is_list(list) do
+    "(" <> Enum.map_join(list, " and ", &nicely_formatted_filter/1) <> ")"
+  end
+
+  defp nicely_formatted_filter(value) do
+    inspect(value)
+  end
+
   defp title_line(error) do
     cond do
       error.resource && error.action ->
@@ -150,7 +182,7 @@ defmodule Ash.Error.Forbidden.Policy do
   defp title(other, title, true), do: [title, ":\n", other]
   defp title(other, title, false), do: [title, "\n", other]
 
-  defp explain_policy(policy, facts) do
+  defp explain_policy(policy, facts, success?) do
     bypass =
       if policy.bypass? do
         "Bypass: "
@@ -161,12 +193,13 @@ defmodule Ash.Error.Forbidden.Policy do
     {condition_description, applies} = describe_conditions(policy.condition, facts)
 
     if applies == true do
-      {description, state} = describe_checks(policy.policies, facts)
+      {description, state} = describe_checks(policy.policies, facts, success?)
 
       tag =
         case state do
           :unknown ->
-            "⛔"
+            # In successful cases, this means we must have filtered
+            "🔎"
 
           :authorized ->
             "🌟"
@@ -258,7 +291,7 @@ defmodule Ash.Error.Forbidden.Policy do
     end
   end
 
-  defp describe_checks(checks, facts) do
+  defp describe_checks(checks, facts, success?) do
     {description, state} =
       Enum.reduce(checks, {[], :unknown}, fn check, {descriptions, state} ->
         new_state =
@@ -273,6 +306,8 @@ defmodule Ash.Error.Forbidden.Policy do
               other
           end
 
+        filter_check? = function_exported?(elem(check.check, 0), :auto_filter, 3)
+
         tag =
           case {state, new_state} do
             {:unknown, :authorized} ->
@@ -282,20 +317,32 @@ defmodule Ash.Error.Forbidden.Policy do
               "⛔"
 
             {:unknown, :unknown} ->
-              "⬇"
+              if success? && filter_check? && Policy.fetch_fact(facts, check.check) == :error do
+                "🔎"
+              else
+                "⬇"
+              end
 
             _ ->
               ""
           end
 
-        {[describe_check(check, Policy.fetch_fact(facts, check.check), tag) | descriptions],
+        {[
-         new_state}
+           describe_check(
+             check,
+             Policy.fetch_fact(facts, check.check),
+             tag,
+             success?,
+             filter_check?
+           )
+           | descriptions
+         ], new_state}
       end)
 
     {Enum.intersperse(Enum.reverse(description), "\n"), state}
   end
 
-  defp describe_check(check, fact_result, tag) do
+  defp describe_check(check, fact_result, tag, success?, filter_check?) do
     fact_result =
       case fact_result do
         {:ok, true} ->
@@ -305,7 +352,11 @@ defmodule Ash.Error.Forbidden.Policy do
           "✘"
 
         :error ->
-          "?"
+          if success? && filter_check? do
+            "✓"
+          else
+            "?"
+          end
       end
 
     [

lib/ash/policy/authorizer.ex

@@ -397,7 +397,18 @@ defmodule Ash.Policy.Authorizer do
     |> do_strict_check_facts()
     |> case do
       {:ok, authorizer} ->
-        strict_check_result(authorizer)
+        case strict_check_result(authorizer) do
+          :authorized ->
+            log_successful_policy_breakdown(authorizer)
+            :authorized
+
+          {:filter, authorizer, filter} ->
+            log_successful_policy_breakdown(authorizer, filter)
+            {:filter, authorizer, filter}
+
+          other ->
+            other
+        end
 
       {:error, error} ->
         {:error, error}
@@ -478,7 +489,7 @@ defmodule Ash.Policy.Authorizer do
       end)
       |> Map.new()
     end)
-    |> simplify_clauses()
+    |> Ash.Policy.SatSolver.simplify_clauses()
     |> Enum.reduce([], fn scenario, or_filters ->
       scenario
       |> Enum.map(fn
@@ -518,40 +529,20 @@ defmodule Ash.Policy.Authorizer do
     end)
   end
 
-  def simplify_clauses([scenario]), do: [scenario]
+  def print_tuple_boolean({op, l, r}) when op in [:and, :or] do
+    "(#{print_tuple_boolean(l)} #{op} #{print_tuple_boolean(r)})"
+  end
 
-  def simplify_clauses(scenarios) do
+  def print_tuple_boolean({:not, l}) do
-    scenarios
+    "not #{print_tuple_boolean(l)}"
-    |> Enum.map(fn scenario ->
+  end
-      scenario
-      |> Enum.flat_map(fn {fact, value} ->
-        if Enum.find(scenarios, fn other_scenario ->
-             other_scenario != scenario &&
-               Map.delete(other_scenario, fact) == Map.delete(scenario, fact) &&
-               Map.fetch(other_scenario, fact) == {:ok, !value}
-           end) do
-          [fact]
-        else
-          []
-        end
-      end)
-      |> case do
-        [] ->
-          scenario
 
-        facts ->
+  def print_tuple_boolean({check, opts}) do
-          Map.drop(scenario, facts)
+    check.describe(opts)
-      end
+  end
-    end)
-    |> Enum.reject(&(&1 == %{}))
-    |> Enum.uniq()
-    |> case do
-      ^scenarios ->
-        scenarios
 
-      new_scenarios ->
+  def print_tuple_boolean(v) do
-        simplify_clauses(new_scenarios)
+    inspect(v)
-    end
   end
 
   defp maybe_forbid_strict(authorizer) do
@@ -615,6 +606,41 @@ defmodule Ash.Policy.Authorizer do
           do_check_result(scenarios, authorizer, record)
       end
     end)
+    |> case do
+      {:ok, authorizer} ->
+        log_successful_policy_breakdown(authorizer)
+
+      other ->
+        other
+    end
+  end
+
+  defp log_successful_policy_breakdown(authorizer, filter \\ nil) do
+    case Ash.Policy.Info.log_successful_policy_breakdowns() do
+      nil ->
+        :ok
+
+      level ->
+        do_log_successful_policy_breakdown(authorizer, filter, level)
+    end
+  end
+
+  defp do_log_successful_policy_breakdown(authorizer, filter, level) do
+    title =
+      "Successful authorization: #{inspect(authorizer.resource)}.#{authorizer.action.name}\n"
+
+    Logger.log(
+      level,
+      [
+        title
+        | Ash.Error.Forbidden.Policy.get_breakdown(
+            authorizer.facts,
+            filter,
+            authorizer.policies,
+            success?: true
+          )
+      ]
+    )
   end
 
   defp do_check_result(cleaned_scenarios, authorizer, record) do

lib/ash/policy/check/relates_to_actor_via.ex

@@ -1,6 +1,6 @@
 defmodule Ash.Policy.Check.RelatesToActorVia do
   @moduledoc false
-  use Ash.Policy.FilterCheck
+  use Ash.Policy.FilterCheckWithContext
 
   require Ash.Expr
   import Ash.Filter.TemplateHelpers
@@ -12,7 +12,7 @@ defmodule Ash.Policy.Check.RelatesToActorVia do
   end
 
   @impl true
-  def filter(opts) do
+  def filter(_actor, _context, opts) do
     opts = Keyword.update!(opts, :relationship_path, &List.wrap/1)
     {last_relationship, to_many?} = relationship_info(opts[:resource], opts[:relationship_path])
 
@@ -34,7 +34,7 @@ defmodule Ash.Policy.Check.RelatesToActorVia do
   end
 
   @impl true
-  def reject(opts) do
+  def reject(actor, context, opts) do
     opts = Keyword.update!(opts, :relationship_path, &List.wrap/1)
     {last_relationship, to_many?} = relationship_info(opts[:resource], opts[:relationship_path])
 
@@ -43,7 +43,7 @@ defmodule Ash.Policy.Check.RelatesToActorVia do
       |> Ash.Resource.Info.primary_key()
 
     if to_many? do
-      Ash.Expr.expr(not (^filter(opts)))
+      Ash.Expr.expr(not (^filter(actor, context, opts)))
     else
       expr =
         Enum.reduce(pkey, nil, fn field, expr ->
@@ -54,7 +54,7 @@ defmodule Ash.Policy.Check.RelatesToActorVia do
           end
         end)
 
-      Ash.Expr.expr(not (^filter(opts)) or ^expr)
+      Ash.Expr.expr(not (^filter(actor, context, opts)) or ^expr)
     end
   end
 

lib/ash/policy/checker.ex

@@ -93,7 +93,10 @@ defmodule Ash.Policy.Checker do
   def strict_check_scenarios(authorizer) do
     case Ash.Policy.Policy.solve(authorizer) do
       {:ok, scenarios} ->
-        {:ok, remove_scenarios_with_impossible_facts(scenarios, authorizer)}
+        {:ok,
+         scenarios
+         |> Ash.Policy.SatSolver.simplify_clauses()
+         |> remove_scenarios_with_impossible_facts(authorizer)}
 
       {:error, :unsatisfiable} ->
         {:error, :unsatisfiable}

lib/ash/policy/filter_check.ex

@@ -107,21 +107,7 @@ defmodule Ash.Policy.FilterCheck do
                public?: false
              }) do
           {:ok, hydrated} ->
-            if changeset.context[:private][:pre_flight_authorization?] do
+            Ash.Filter.Runtime.do_match(nil, hydrated)
-              with {:no_related_refs, true} <-
-                     {:no_related_refs, no_related_references?(expression)},
-                   {:ok, fake_result} <- Ash.Changeset.apply_attributes(changeset, force?: true) do
-                Ash.Filter.Runtime.do_match(fake_result, hydrated)
-              else
-                {:no_related_refs, false} ->
-                  :unknown
-
-                {:error, error} ->
-                  {:halt, {:error, error}}
-              end
-            else
-              Ash.Filter.Runtime.do_match(nil, hydrated)
-            end
 
           {:error, error} ->
             {:error, error}

lib/ash/policy/filter_check_with_context.ex

@@ -0,0 +1,193 @@
+defmodule Ash.Policy.FilterCheckWithContext do
+  @moduledoc """
+  A type of check that is represented by a filter statement, and has access to the
+  """
+
+  @type options :: Keyword.t()
+  @type context :: %{
+          optional(:query) => Ash.Query.t(),
+          optional(:changeset) => Ash.Query.t(),
+          :action => Ash.Resource.Actions.action(),
+          :resource => Ash.Resource.t(),
+          :api => Ash.Api.t()
+        }
+
+  @callback filter(actor :: term, context(), options()) :: Keyword.t() | Ash.Expr.t()
+  @callback reject(actor :: term, context(), options()) :: Keyword.t() | Ash.Expr.t()
+  @optional_callbacks [reject: 3]
+
+  defmacro __using__(_) do
+    quote do
+      @behaviour Ash.Policy.FilterCheckWithContext
+      @behaviour Ash.Policy.Check
+
+      require Ash.Query
+
+      def type, do: :filter
+
+      def strict_check_context(opts) do
+        []
+      end
+
+      def strict_check(nil, authorizer, opts) do
+        if Ash.Filter.template_references_actor?(filter(nil, authorizer, opts)) do
+          {:ok, false}
+        else
+          try_strict_check(nil, authorizer, opts)
+        end
+      end
+
+      def strict_check(actor, authorizer, opts) do
+        try_strict_check(actor, authorizer, opts)
+      end
+
+      defp try_strict_check(actor, authorizer, opts) do
+        opts = Keyword.put_new(opts, :resource, authorizer.resource)
+
+        actor
+        |> filter(authorizer, opts)
+        |> Ash.Filter.build_filter_from_template(actor)
+        |> try_eval(authorizer)
+        |> case do
+          {:ok, false} ->
+            {:ok, false}
+
+          {:ok, nil} ->
+            {:ok, false}
+
+          {:ok, _} ->
+            {:ok, true}
+
+          _ ->
+            {:ok, :unknown}
+        end
+      end
+
+      defp try_eval(expression, %{query: %Ash.Query{} = query}) do
+        case Ash.Filter.hydrate_refs(expression, %{
+               resource: query.resource,
+               aggregates: query.aggregates,
+               calculations: query.calculations,
+               public?: false
+             }) do
+          {:ok, hydrated} ->
+            Ash.Filter.Runtime.do_match(nil, hydrated)
+
+          {:error, error} ->
+            {:error, error}
+        end
+      end
+
+      defp try_eval(expression, %{
+             resource: resource,
+             changeset: %Ash.Changeset{action_type: :create} = changeset
+           }) do
+        case Ash.Filter.hydrate_refs(expression, %{
+               resource: resource,
+               aggregates: %{},
+               calculations: %{},
+               public?: false
+             }) do
+          {:ok, hydrated} ->
+            Ash.Filter.Runtime.do_match(nil, hydrated)
+
+          {:error, error} ->
+            {:error, error}
+        end
+      end
+
+      defp try_eval(expression, %{
+             resource: resource,
+             changeset: %Ash.Changeset{data: data} = changeset
+           }) do
+        case Ash.Filter.hydrate_refs(expression, %{
+               resource: resource,
+               aggregates: %{},
+               calculations: %{},
+               public?: false
+             }) do
+          {:ok, hydrated} ->
+            # We don't want to authorize on stale data in real life
+            # but when using utilities to check if something *will* be authorized
+            # that is our intent
+            if changeset.context[:private][:pre_flight_authorization?] do
+              Ash.Filter.Runtime.do_match(data, hydrated)
+            else
+              Ash.Filter.Runtime.do_match(nil, hydrated)
+            end
+
+          {:error, error} ->
+            {:error, error}
+        end
+      end
+
+      defp try_eval(expression, %{resource: resource}) do
+        case Ash.Filter.hydrate_refs(expression, %{
+               resource: resource,
+               aggregates: %{},
+               calculations: %{},
+               public?: false
+             }) do
+          {:ok, hydrated} ->
+            Ash.Filter.Runtime.do_match(nil, hydrated)
+
+          {:error, error} ->
+            {:error, error}
+        end
+      end
+
+      defp no_related_references?(expression) do
+        expression
+        |> Ash.Filter.list_refs()
+        |> Enum.any?(&(&1.relationship_path != []))
+      end
+
+      def auto_filter(actor, authorizer, opts) do
+        opts = Keyword.put_new(opts, :resource, authorizer.resource)
+        Ash.Filter.build_filter_from_template(filter(actor, authorizer, opts), actor)
+      end
+
+      def auto_filter_not(actor, authorizer, opts) do
+        opts = Keyword.put_new(opts, :resource, authorizer.resource)
+        Ash.Filter.build_filter_from_template(reject(actor, authorizer, opts), actor)
+      end
+
+      def reject(actor, authorizer, opts) do
+        [not: filter(actor, authorizer, opts)]
+      end
+
+      def check(actor, data, authorizer, opts) do
+        pkey = Ash.Resource.Info.primary_key(authorizer.resource)
+
+        filter =
+          case data do
+            [record] -> Map.take(record, pkey)
+            records -> [or: Enum.map(data, &Map.take(&1, pkey))]
+          end
+
+        authorizer.resource
+        |> authorizer.api.query()
+        |> Ash.Query.filter(^filter)
+        |> Ash.Query.filter(^auto_filter(actor, authorizer, opts))
+        |> authorizer.api.read()
+        |> case do
+          {:ok, authorized_data} ->
+            authorized_pkeys = Enum.map(authorized_data, &Map.take(&1, pkey))
+
+            Enum.filter(data, fn record ->
+              Map.take(record, pkey) in authorized_pkeys
+            end)
+
+          {:error, error} ->
+            {:error, error}
+        end
+      end
+
+      defoverridable reject: 3
+    end
+  end
+
+  def is_filter_check?(module) do
+    :erlang.function_exported(module, :filter, 1)
+  end
+end

lib/ash/policy/info.ex

@@ -18,6 +18,11 @@ defmodule Ash.Policy.Info do
     Application.get_env(:ash, :policies)[:log_policy_breakdowns]
   end
 
+  @doc "Whether or not ash policy authorizer is configured to show policy breakdowns in error messages"
+  def log_successful_policy_breakdowns do
+    Application.get_env(:ash, :policies)[:log_successful_policy_breakdowns]
+  end
+
   @doc """
   A utility to determine if a given query/changeset would pass authorization.
 

lib/ash/policy/policy.ex

@@ -34,24 +34,78 @@ defmodule Ash.Policy.Policy do
   defp build_requirements_expression(policies, facts) do
     at_least_one_policy_expression = at_least_one_policy_expression(policies, facts)
 
-    policy_expression =
+    if at_least_one_policy_expression == false do
-      {:and, at_least_one_policy_expression, compile_policy_expression(policies, facts)}
+      false
-
-    facts_expression = Ash.Policy.SatSolver.facts_to_statement(Map.drop(facts, [true, false]))
-
-    if facts_expression do
-      {:and, facts_expression, policy_expression}
     else
-      policy_expression
+      policy_expression =
+        if at_least_one_policy_expression == true do
+          compile_policy_expression(policies, facts)
+        else
+          case {:and, at_least_one_policy_expression, compile_policy_expression(policies, facts)} do
+            {:and, false, _} ->
+              false
+
+            {:and, _, false} ->
+              false
+
+            {:and, true, true} ->
+              true
+
+            {:and, left, true} ->
+              left
+
+            {:and, true, right} ->
+              right
+
+            other ->
+              other
+          end
+        end
+
+      used_facts = used_facts(policy_expression)
+
+      facts_expression =
+        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}
+      else
+        policy_expression
+      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
     policies
     |> Enum.map(&condition_expression(&1.condition, facts))
     |> Enum.filter(& &1)
-    |> Enum.reduce(false, fn condition, acc ->
+    |> Enum.reduce(false, fn
-      {:or, condition, acc}
+      _, true ->
+        true
+
+      true, _ ->
+        true
+
+      false, acc ->
+        acc
+
+      condition, acc ->
+        {:or, condition, acc}
     end)
   end
 
@@ -140,7 +194,11 @@ defmodule Ash.Policy.Policy do
         compiled_policies
 
       condition_expression ->
-        {:and, condition_expression, compiled_policies}
+        if compiled_policies == true do
+          condition_expression
+        else
+          {:and, condition_expression, compiled_policies}
+        end
     end
   end
 
@@ -248,7 +306,8 @@ defmodule Ash.Policy.Policy do
         true
 
       :error ->
-        {:or, {clause.check_module, clause.check_opts}, compile_policy_expression(rest, facts)}
+        {:or, {:not, {clause.check_module, clause.check_opts}},
+         compile_policy_expression(rest, facts)}
     end
   end
 

lib/ash/policy/sat_solver.ex

@@ -51,46 +51,50 @@ defmodule Ash.Policy.SatSolver do
   def simplify_clauses([scenario]), do: [scenario]
 
   def simplify_clauses(scenarios) do
-    scenarios
+    unnecessary_clauses =
-    |> Enum.map(fn scenario ->
+      scenarios
-      scenario
+      |> Enum.with_index()
-      |> Enum.flat_map(fn {fact, value} ->
+      |> Enum.flat_map(fn {scenario, index} ->
-        if Enum.find(scenarios, fn other_scenario ->
+        scenario
-             other_scenario != scenario &&
+        |> Enum.flat_map(fn {fact, _value} ->
-               Map.delete(other_scenario, fact) == Map.delete(scenario, fact) &&
+          if Enum.find(scenarios, fn other_scenario ->
-               Map.fetch(other_scenario, fact) == {:ok, !value}
+               scenario_makes_fact_irrelevant?(other_scenario, scenario, fact)
-           end) do
+             end) do
-          [fact]
+            [fact]
-        else
+          else
-          []
+            []
-        end
+          end
+        end)
+        |> Enum.map(fn fact ->
+          {index, fact}
+        end)
       end)
-      |> case do
+      |> Enum.group_by(&elem(&1, 0), &elem(&1, 1))
-        [] ->
-          scenario
 
-        facts ->
+    case unnecessary_clauses do
-          Map.drop(scenario, facts)
+      empty when empty == %{} ->
-      end
-    end)
-    |> Enum.uniq()
-    |> case do
-      ^scenarios ->
         scenarios
 
-      new_scenarios ->
+      unnecessary_clauses ->
-        simplify_clauses(new_scenarios)
+        unnecessary_clauses
+        |> Enum.reduce(scenarios, fn {index, facts}, scenarios ->
+          List.update_at(scenarios, index, &Map.drop(&1, facts))
+        end)
+        |> Enum.reject(&(&1 == %{}))
+        |> Enum.uniq()
+        |> simplify_clauses()
     end
   end
 
+  def scenario_makes_fact_irrelevant?(potential_irrelevant_maker, _scenario, _fact)
+      when potential_irrelevant_maker == %{},
+      do: false
+
   def scenario_makes_fact_irrelevant?(potential_irrelevant_maker, scenario, fact) do
-    (Map.delete(potential_irrelevant_maker, fact) ==
+    scenario_is_subset?(Map.delete(potential_irrelevant_maker, fact), scenario) &&
-       Map.delete(scenario, fact) &&
+      Map.has_key?(potential_irrelevant_maker, fact) && Map.has_key?(scenario, fact) &&
-       Map.has_key?(potential_irrelevant_maker, fact) && Map.has_key?(scenario, fact) &&
+      Map.get(potential_irrelevant_maker, fact) !=
-       Map.get(potential_irrelevant_maker, fact) !=
+        Map.get(scenario, fact)
-         Map.get(scenario, fact)) ||
-      (!Map.has_key?(potential_irrelevant_maker, fact) &&
-         scenario_is_subset?(potential_irrelevant_maker, scenario))
   end
 
   defp scenario_is_subset?(left, right) do

lib/ash/query/query.ex

@@ -142,6 +142,28 @@ defmodule Ash.Query do
     defp or_empty(_, false), do: empty()
   end
 
+  defmacrop maybe_already_validated_error!(query) do
+    {function, _arity} = __CALLER__.function
+
+    quote do
+      query = unquote(query)
+
+      if query.__validated_for_action__ && !query.context[:private][:in_before_action?] do
+        raise ArgumentError, """
+        Changeset has already been validated for action #{inspect(query.__validated_for_action__)}.
+
+        For safety, we prevent any changes after that point because they will bypass validations or other action logic.
+        However, you should prefer a pattern like the below, which makes any custom modifications *before* calling the action.
+
+          Resource
+          |> Ash.Query.new()
+          |> Ash.Query.#{unquote(function)}(...)
+          |> Ash.Query.for_read(...)
+        """
+      end
+    end
+  end
+
   @doc """
   Attach a filter statement to the query.
 
@@ -305,12 +327,12 @@ defmodule Ash.Query do
           |> set_authorize?(opts)
           |> set_tracer(opts)
           |> Ash.Query.set_tenant(opts[:tenant] || query.tenant)
-          |> Map.put(:__validated_for_action__, action_name)
           |> cast_params(action, args)
           |> set_argument_defaults(action)
           |> require_arguments(action)
           |> run_preparations(action, opts[:actor], opts[:authorize?], opts[:tracer], metadata)
           |> add_action_filters(action, opts[:actor])
+          |> Map.put(:__validated_for_action__, action_name)
         end
       end
     else
@@ -1083,6 +1105,7 @@ defmodule Ash.Query do
   Add an argument to the query, which can be used in filter templates on actions
   """
   def set_argument(query, argument, value) do
+    maybe_already_validated_error!(query)
     query = to_query(query)
 
     if query.action do

lib/doc_index.ex

@@ -138,6 +138,7 @@ defmodule Ash.DocIndex do
          Ash.Policy.Check,
          Ash.Policy.Check.Builtins,
          Ash.Policy.FilterCheck,
+         Ash.Policy.FilterCheckWithContext,
          Ash.Policy.SimpleCheck
        ]},
       {"Introspection",

lib/sat_solver/sat_solver.ex

@@ -751,6 +751,11 @@ defmodule Ash.SatSolver do
     end)
     |> group_predicates(bindings)
     |> rebind()
+    |> unique_clauses()
+  end
+
+  defp unique_clauses({clauses, bindings}) do
+    {Enum.uniq(clauses), bindings}
   end
 
   defp group_predicates(expression, bindings) do
@@ -770,7 +775,8 @@ defmodule Ash.SatSolver do
     scenario
     |> Ash.SatSolver.Utils.ordered_sublists()
     |> Enum.filter(&can_be_used_as_group?(&1, all_scenarios, bindings))
-    |> Enum.sort_by(&(-length(&1)))
+    |> Enum.sort_by(&length/1)
+    |> remove_overlapping()
     |> Enum.reduce({scenario, bindings}, fn group, {scenario, bindings} ->
       bindings = add_group_binding(bindings, group)
 
@@ -779,6 +785,18 @@ defmodule Ash.SatSolver do
     end)
   end
 
+  defp remove_overlapping([]), do: []
+
+  defp remove_overlapping([item | rest]) do
+    if Enum.any?(item, fn n ->
+         Enum.any?(rest, &(n in &1 or -n in &1))
+       end) do
+      remove_overlapping(rest)
+    else
+      [item | remove_overlapping(rest)]
+    end
+  end
+
   def unbind(expression, %{temp_bindings: temp_bindings, old_bindings: old_bindings}) do
     expression =
       Enum.flat_map(expression, fn statement ->
@@ -809,11 +827,7 @@ defmodule Ash.SatSolver do
   end
 
   def expand_groups(expression) do
-    if Enum.any?(expression, &match?({:expand, _}, &1)) do
+    do_expand_groups(expression)
-      do_expand_groups(expression)
-    else
-      [expression]
-    end
   end
 
   defp do_expand_groups([]), do: [[]]
@@ -897,14 +911,14 @@ defmodule Ash.SatSolver do
     if bindings[:groups][group] do
       bindings
     else
-      new_binding = bindings[:current] + 1
+      binding = bindings[:current]
 
       bindings
-      |> Map.put(:current, new_binding)
       |> Map.put_new(:reverse_groups, %{})
-      |> Map.update!(:reverse_groups, &Map.put(&1, new_binding, group))
+      |> Map.update!(:reverse_groups, &Map.put(&1, binding, group))
       |> Map.put_new(:groups, %{})
-      |> Map.update!(:groups, &Map.put(&1, group, new_binding))
+      |> Map.update!(:groups, &Map.put(&1, group, binding))
+      |> Map.put(:current, binding + 1)
     end
   end
 

lib/sat_solver/utils.ex

@@ -41,9 +41,6 @@ defmodule Ash.SatSolver.Utils do
     list
     |> do_sublists_front()
     |> Enum.reject(fn
-      ^list ->
-        true
-
       [_] ->
         true
 

mix.exs

@@ -121,6 +121,7 @@ defmodule Ash.MixProject do
           Ash.Policy.Check,
           Ash.Policy.Check.Builtins,
           Ash.Policy.FilterCheck,
+          Ash.Policy.FilterCheckWithContext,
           Ash.Policy.SimpleCheck
         ],
         Introspection: [

test/actions/create_test.exs

@@ -433,9 +433,7 @@ defmodule Ash.Test.Actions.CreateTest do
       assert :tag in changeset.defaults
 
       force_changeset = Ash.Changeset.force_change_attribute(changeset, :tag, "foo")
-      non_force_changeset = Ash.Changeset.change_attribute(changeset, :tag, "bar")
       refute :tag in force_changeset.defaults
-      refute :tag in non_force_changeset.defaults
     end
 
     test "nil will error on required attribute with default" do

test/actions/update_test.exs

@@ -118,7 +118,7 @@ defmodule Ash.Test.Actions.UpdateTest do
           {:ok,
            changeset.data
            |> Ash.Changeset.for_update(:update, changeset.attributes)
-           |> Ash.Changeset.change_attribute(:name, "manual")
+           |> Ash.Changeset.force_change_attribute(:name, "manual")
            |> Ash.Test.Actions.UpdateTest.Api.update!()}
         end
       end

test/policy/complex_test.exs

@@ -7,7 +7,6 @@ defmodule Ash.Test.Policy.ComplexTest do
 
   setup do
     Application.put_env(:ash, :policies, show_policy_breakdowns?: true)
-    Logger.configure(level: :debug)
 
     on_exit(fn ->
       Application.delete_env(:ash, :policies)

test/policy/simple_test.exs

@@ -85,6 +85,12 @@ defmodule Ash.Test.Policy.SimpleTest do
     assert ids == Enum.sort([post1.id, post2.id])
   end
 
+  test "authorize_unless properly combines", %{user: user} do
+    Car
+    |> Ash.Changeset.for_create(:authorize_unless, %{users: [user.id]})
+    |> Api.create!(actor: user)
+  end
+
   test "filter checks work with many to many related data and a filter", %{user: user} do
     car1 =
       Car

test/support/policy_simple/resources/car.ex

@@ -16,6 +16,8 @@ defmodule Ash.Test.Support.PolicySimple.Car do
       argument(:users, {:array, :uuid})
       change(manage_relationship(:users, type: :append_and_remove))
     end
+
+    create :authorize_unless
   end
 
   attributes do
@@ -23,8 +25,14 @@ defmodule Ash.Test.Support.PolicySimple.Car do
   end
 
   policies do
-    policy always() do
+    policy action(:authorize_unless) do
-      authorize_if(expr(users.id == ^actor(:id)))
+      authorize_if never()
+      authorize_unless never()
+      authorize_if never()
+    end
+
+    policy action_type([:read, :update, :destroy]) do
+      authorize_if expr(users.id == ^actor(:id))
     end
   end