fix: static checks with conditions could be overly or insufficiently restrictive

lib/ash/policy/authorizer/authorizer.ex

@@ -682,72 +682,74 @@ defmodule Ash.Policy.Authorizer do
     end)
     # primary key doesn't have policies on it, and so is nil here
     |> Map.drop([nil, []])
-    |> Enum.reduce({query_or_changeset, authorizer}, fn {policies, fields},
+    |> Enum.reduce(
-                                                        {query_or_changeset, authorizer} ->
+      {query_or_changeset, authorizer},
-      {expr, authorizer} =
+      fn {policies, fields}, {query_or_changeset, authorizer} ->
-        case strict_check_result(
+        {expr, authorizer} =
-               %{
+          case strict_check_result(
-                 authorizer
+                 %{
-                 | policies: policies
+                   authorizer
-               }
+                   | policies: policies
-               |> add_query_or_changeset(query_or_changeset),
+                 }
-               for_fields: fields,
+                 |> add_query_or_changeset(query_or_changeset),
-               context_description: "selecting or loading fields"
+                 for_fields: fields,
-             ) do
+                 context_description: "selecting or loading fields"
-          {:authorized, authorizer} ->
+               ) do
-            {true, authorizer}
+            {:authorized, authorizer} ->
+              {true, authorizer}
 
-          {:error, _} ->
+            {:error, _} ->
-            {false, authorizer}
+              {false, authorizer}
 
-          {:filter, authorizer, filter} ->
+            {:filter, authorizer, filter} ->
-            {filter, authorizer}
+              {filter, authorizer}
 
-          {:filter_and_continue, filter, _authorizer} ->
+            {:filter_and_continue, filter, _authorizer} ->
-            raise """
+              raise """
-            Was given a partial filter for a field policy for fields #{inspect(fields)}.
+              Was given a partial filter for a field policy for fields #{inspect(fields)}.
 
-            Filter: #{inspect(filter)}
+              Filter: #{inspect(filter)}
 
-            Field policies must currently use only filter checks or simple checks.
+              Field policies must currently use only filter checks or simple checks.
-            """
+              """
 
-          {:continue, _} ->
+            {:continue, _} ->
-            raise """
+              raise """
-            Detected necessity for a runtime check for a field policy for fields #{inspect(fields)}.
+              Detected necessity for a runtime check for a field policy for fields #{inspect(fields)}.
 
-            Field policies must currently use only filter checks or simple checks.
+              Field policies must currently use only filter checks or simple checks.
-            """
+              """
+          end
+
+        # This is a hack that we need to clean up.
+        # Creating this kind of expression should be its own thing that we do
+        # with something in the `Expr` module
+
+        %{expression: expr} = Ash.Filter.parse!(query_or_changeset.resource, expr)
+
+        {:ok, calculation} =
+          Ash.Query.Calculation.new(
+            {:__ash_fields_are_visible__, fields},
+            Ash.Resource.Calculation.Expression,
+            [expr: expr],
+            :boolean
+          )
+
+        case query_or_changeset do
+          %Ash.Query{} = query ->
+            {Ash.Query.load(
+               query,
+               calculation
+             ), authorizer}
+
+          %Ash.Changeset{} = query ->
+            {Ash.Changeset.load(
+               query,
+               calculation
+             ), authorizer}
         end
-
-      # This is a hack that we need to clean up.
-      # Creating this kind of expression should be its own thing that we do
-      # with something in the `Expr` module
-
-      %{expression: expr} = Ash.Filter.parse!(query_or_changeset.resource, expr)
-
-      {:ok, calculation} =
-        Ash.Query.Calculation.new(
-          {:__ash_fields_are_visible__, fields},
-          Ash.Resource.Calculation.Expression,
-          [expr: expr],
-          :boolean
-        )
-
-      case query_or_changeset do
-        %Ash.Query{} = query ->
-          {Ash.Query.load(
-             query,
-             calculation
-           ), authorizer}
-
-        %Ash.Changeset{} = query ->
-          {Ash.Changeset.load(
-             query,
-             calculation
-           ), authorizer}
       end
-    end)
+    )
     |> then(fn {result, authorizer} ->
       {:ok, result, authorizer}
     end)
@@ -785,15 +787,13 @@ defmodule Ash.Policy.Authorizer do
     {filterable, require_check} =
       authorizer.scenarios
       |> Enum.split_with(fn scenario ->
-        scenario
+        Enum.all?(scenario, fn {{check_module, opts}, _} ->
-        |> Enum.reject(fn {{check_module, opts}, _} ->
           opts[:access_type] == :filter ||
             match?(
               {:ok, _},
               Ash.Policy.Policy.fetch_fact(authorizer.facts, {check_module, opts})
             ) || check_module.type() == :filter
         end)
-        |> Enum.empty?()
       end)
 
     filter = strict_filters(filterable, authorizer)

lib/ash/policy/checker.ex

@@ -66,7 +66,7 @@ defmodule Ash.Policy.Checker do
     scenario
     |> Map.drop([true, false])
     |> Enum.reduce_while(:reality, fn {fact, requirement}, status ->
-      case Map.fetch(facts, fact) do
+      case Policy.fetch_fact(facts, fact) do
         {:ok, ^requirement} ->
           {:cont, status}
 

lib/ash/policy/policy.ex

@@ -152,8 +152,8 @@ defmodule Ash.Policy.Policy do
     Enum.find_value(authorizer.facts, fn
       {{fact_mod, fact_opts}, result} ->
         if check_module == fact_mod &&
-             Keyword.delete(fact_opts, :access_type) ==
+             Keyword.drop(fact_opts, [:access_type, :ash_field_policy?]) ==
-               Keyword.delete(opts, :access_type) do
+               Keyword.drop(opts, [:access_type, :ash_field_policy?]) do
           {:ok, result}
         end
 
@@ -198,8 +198,8 @@ defmodule Ash.Policy.Policy do
     Enum.find_value(facts, fn
       {{fact_mod, fact_opts}, result} ->
         if mod == fact_mod &&
-             Keyword.delete(fact_opts, :access_type) ==
+             Keyword.drop(fact_opts, [:access_type, :ash_field_policy?]) ==
-               Keyword.delete(opts, :access_type) do
+               Keyword.drop(opts, [:access_type, :ash_field_policy?]) do
           {:ok, result}
         end
 
@@ -275,13 +275,14 @@ defmodule Ash.Policy.Policy do
       condition_expression ->
         case compile_policy_expression(policies, authorizer) do
           {true, authorizer} ->
-            {condition_expression, authorizer}
+            {true, authorizer}
 
           {false, authorizer} ->
             {{:not, condition_expression}, authorizer}
 
           {compiled_policies, authorizer} ->
-            {{:and, condition_expression, compiled_policies}, authorizer}
+            {{:or, {:and, condition_expression, compiled_policies}, {:not, condition_expression}},
+             authorizer}
         end
     end
   end

lib/ash/resource/relationships/belongs_to.ex

@@ -100,7 +100,7 @@ defmodule Ash.Resource.Relationships.BelongsTo do
   def opt_schema, do: @opt_schema
 
   @doc false
-  # sobelow_skip ["DOS.StringToAtom"]
+  # sobelow_skip ["DOS.BinToAtom"]
   def transform(%{source_attribute: source_attribute, name: name} = relationship) do
     {:ok, %{relationship | source_attribute: source_attribute || :"#{name}_id"}}
   end

lib/ash/resource/relationships/many_to_many.ex

@@ -87,7 +87,7 @@ defmodule Ash.Resource.Relationships.ManyToMany do
   def opt_schema, do: @opt_schema
 
   @doc false
-  # sobelow_skip ["DOS.StringToAtom"]
+  # sobelow_skip ["DOS.BinToAtom"]
   def transform(%{join_relationship: join_relationship, name: name} = relationship) do
     {:ok, %{relationship | join_relationship: join_relationship || :"#{name}_join_assoc"}}
   end

test/policy/field_policies/expr_conditions_test.exs

@@ -10,55 +10,6 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do
     end
   end
 
-  defmodule ResourceWithExprCondition do
-    use Ash.Resource,
-      data_layer: Ash.DataLayer.Ets,
-      authorizers: [Ash.Policy.Authorizer]
-
-    attributes do
-      uuid_primary_key :id
-
-      attribute :name, :string
-    end
-
-    field_policies do
-      field_policy :name, [expr(name == ^actor(:name))] do
-        authorize_if always()
-      end
-    end
-
-    policies do
-      policy always() do
-        authorize_if always()
-      end
-    end
-
-    code_interface do
-      define_for Api
-
-      define :create
-      define :read
-    end
-
-    actions do
-      defaults [:create, :read]
-    end
-  end
-
-  test "expr condition forbids field if it does not match" do
-    ResourceWithExprCondition.create!(%{name: "foo"})
-    ResourceWithExprCondition.create!(%{name: "bar"})
-
-    assert [
-             %{name: "bar"},
-             %{name: %Ash.ForbiddenField{field: :name, type: :attribute}}
-           ] =
-             ResourceWithExprCondition.read!(
-               actor: %{name: "bar"},
-               query: ResourceWithExprCondition |> Ash.Query.sort([:name])
-             )
-  end
-
   defmodule ResourceWithMultiplePoliciesForOneField do
     use Ash.Resource,
       data_layer: Ash.DataLayer.Ets,
@@ -68,6 +19,8 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do
       uuid_primary_key :id
 
       attribute :name, :string
+      attribute :other_name, :string
+      attribute :other_other_name, :string
     end
 
     field_policies do
@@ -78,10 +31,27 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do
       field_policy :name, expr(name == ^actor(:name)) do
         authorize_if always()
       end
+
+      field_policy :other_name, [actor_attribute_equals(:admin, true)] do
+        forbid_if always()
+      end
+
+      field_policy :other_name, expr(name == ^actor(:name)) do
+        forbid_if always()
+      end
+
+      field_policy :other_other_name, [actor_attribute_equals(:admin, true)] do
+        authorize_if always()
+      end
+
+      field_policy :other_other_name, expr(name == ^actor(:name)) do
+        forbid_if always()
+      end
     end
 
     policies do
       policy always() do
+        forbid_if never()
         authorize_if always()
       end
     end
@@ -99,80 +69,29 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do
   end
 
   test "multiple field policies for the same field with different conditions work" do
-    ResourceWithMultiplePoliciesForOneField.create!(%{name: "foo"})
+    ResourceWithMultiplePoliciesForOneField.create!(%{
-    ResourceWithMultiplePoliciesForOneField.create!(%{name: "baz"})
+      name: "foo",
+      other_name: "foo",
+      other_other_name: "foo"
+    })
+
+    ResourceWithMultiplePoliciesForOneField.create!(%{
+      name: "baz",
+      other_name: "baz",
+      other_other_name: "bar"
+    })
 
     assert [
-             %{name: "baz"},
+             %{
-             %{name: "foo"}
+               name: "baz",
+               other_name: %Ash.ForbiddenField{},
+               other_other_name: %Ash.ForbiddenField{}
+             },
+             %{name: "foo", other_name: %Ash.ForbiddenField{}, other_other_name: "foo"}
            ] =
              ResourceWithMultiplePoliciesForOneField.read!(
                actor: %{name: "baz", admin: true},
                query: ResourceWithMultiplePoliciesForOneField |> Ash.Query.sort([:name])
              )
   end
-
-  defmodule ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions do
-    use Ash.Resource,
-      data_layer: Ash.DataLayer.Ets,
-      authorizers: [Ash.Policy.Authorizer]
-
-    attributes do
-      uuid_primary_key :id
-
-      attribute :name, :string
-    end
-
-    field_policies do
-      field_policy :name, [actor_attribute_equals(:admin, true)] do
-        authorize_if always()
-      end
-
-      # I saw in the generated spark_dsl_config, that the extra check options are
-      # set for expr inside the field policy, but not for conditions I think.
-      # The behaviour is different if I set them here.
-      field_policy :name,
-                   {Ash.Policy.Check.Expression,
-                    [
-                      expr: expr(name == ^actor(:name)),
-                      ash_field_policy?: true,
-                      access_type: :filter
-                    ]} do
-        authorize_if always()
-      end
-    end
-
-    policies do
-      policy always() do
-        authorize_if always()
-      end
-    end
-
-    code_interface do
-      define_for Api
-
-      define :create
-      define :read
-    end
-
-    actions do
-      defaults [:create, :read]
-    end
-  end
-
-  test "multiple field policies for the same field with different conditions work (extra check options)" do
-    ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions.create!(%{name: "foo"})
-    ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions.create!(%{name: "baz"})
-
-    assert [
-             %{name: "baz"},
-             %{name: "foo"}
-           ] =
-             ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions.read!(
-               actor: %{name: "baz", admin: true},
-               query:
-                 ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions
-                 |> Ash.Query.sort([:name])
-             )
-  end
 end