Revert "improvement: make authorization failures behave consistently across reads"

This reverts commit ffa37d0c95.

lib/ash/policy/authorizer/authorizer.ex

@@ -299,12 +299,6 @@ defmodule Ash.Policy.Authorizer do
       Ash.Expr
     ],
     schema: [
-      statically_deniable_reads: [
-        type: {:one_of, [:error, :filter]},
-        default: :filter,
-        doc:
-          "When a read request is denied immediately, i.e we know its forbidden without ever running a query. `:filter` will filter all records out, and `:error` will result in a forbidden error."
-      ],
       default_access_type: [
         type: {:one_of, [:strict, :filter, :runtime]},
         default: :filter,
@@ -1175,7 +1169,7 @@ defmodule Ash.Policy.Authorizer do
       {_filters, _require_check} ->
         case global_filters(authorizer) do
           nil ->
-            maybe_forbid_strict(authorizer) |> IO.inspect()
+            maybe_forbid_strict(authorizer)
 
           {filters, scenarios_without_global} ->
             with {:ok, %Ash.Filter{expression: filter}} <-
@@ -1504,83 +1498,47 @@ defmodule Ash.Policy.Authorizer do
   end
 
   defp strict_check_result(authorizer, opts \\ []) do
-    case authorizer.policies do
-      [] ->
-        error =
-          Ash.Error.Forbidden.Policy.exception(
-            facts: authorizer.facts,
-            policies: authorizer.policies,
-            context_description: opts[:context_description],
-            for_fields: opts[:for_fields],
-            resource: Map.get(authorizer, :resource),
-            actor: Map.get(authorizer, :action),
-            action: Map.get(authorizer, :action),
-            scenarios: []
-          )
+    case Checker.strict_check_scenarios(authorizer) do
+      {:ok, true, authorizer} ->
+        {:authorized, authorizer}
 
-        {:error, error}
+      {:ok, none, authorizer} when none in [false, []] ->
+        {:error,
+         Ash.Error.Forbidden.Policy.exception(
+           facts: authorizer.facts,
+           policies: authorizer.policies,
+           context_description: opts[:context_description],
+           for_fields: opts[:for_fields],
+           resource: Map.get(authorizer, :resource),
+           actor: Map.get(authorizer, :action),
+           action: Map.get(authorizer, :action),
+           scenarios: []
+         )}
 
-      _ ->
-        case Checker.strict_check_scenarios(authorizer) do
-          {:ok, true, authorizer} ->
+      {:ok, scenarios, authorizer} ->
+        case Checker.find_real_scenarios(scenarios, authorizer.facts) do
+          [] ->
+            maybe_strict_filter(authorizer, scenarios)
+
+          _real_scenarios ->
             {:authorized, authorizer}
-
-          {:ok, none, authorizer} when none in [false, []] ->
-            # we construct the error to log policy breakdown
-            error =
-              Ash.Error.Forbidden.Policy.exception(
-                facts: authorizer.facts,
-                policies: authorizer.policies,
-                context_description: opts[:context_description],
-                for_fields: opts[:for_fields],
-                resource: Map.get(authorizer, :resource),
-                actor: Map.get(authorizer, :action),
-                action: Map.get(authorizer, :action),
-                scenarios: []
-              )
-
-            if authorizer.action.type == :read and
-                 Ash.Policy.Info.statically_deniable_reads(authorizer.resource) ==
-                   :filter do
-              {:filter, authorizer, false}
-            else
-              {:error, error}
-            end
-
-          {:ok, scenarios, authorizer} ->
-            case Checker.find_real_scenarios(scenarios, authorizer.facts) do
-              [] ->
-                maybe_strict_filter(authorizer, scenarios)
-
-              _real_scenarios ->
-                {:authorized, authorizer}
-            end
-
-          {:error, authorizer, :unsatisfiable} ->
-            # we construct the error to log policy breakdown
-            error =
-              Ash.Error.Forbidden.Policy.exception(
-                facts: authorizer.facts,
-                policies: authorizer.policies,
-                context_description: opts[:context_description],
-                for_fields: opts[:for_fields],
-                resource: Map.get(authorizer, :resource),
-                actor: Map.get(authorizer, :action),
-                action: Map.get(authorizer, :action),
-                scenarios: []
-              )
-
-            if authorizer.action.type == :read and
-                 Ash.Policy.Info.statically_deniable_reads(authorizer.resource) ==
-                   :filter do
-              {:filter, authorizer, false}
-            else
-              {:error, error}
-            end
-
-          {:error, _authorizer, exception} ->
-            {:error, Ash.Error.to_ash_error(exception)}
         end
+
+      {:error, authorizer, :unsatisfiable} ->
+        {:error,
+         Ash.Error.Forbidden.Policy.exception(
+           facts: authorizer.facts,
+           policies: authorizer.policies,
+           context_description: opts[:context_description],
+           for_fields: opts[:for_fields],
+           resource: Map.get(authorizer, :resource),
+           action: Map.get(authorizer, :action),
+           actor: Map.get(authorizer, :action),
+           scenarios: []
+         )}
+
+      {:error, _authorizer, exception} ->
+        {:error, Ash.Error.to_ash_error(exception)}
     end
   end
 

lib/ash/policy/info.ex

@@ -188,10 +188,6 @@ defmodule Ash.Policy.Info do
     Extension.get_opt(resource, [:policies], :default_access_type, :filter, false)
   end
 
-  def statically_deniable_reads(resource) do
-    Extension.get_opt(resource, [:policies], :statically_deniable_reads, :filter, false)
-  end
-
   # This should be done at compile time
   defp set_access_type(policies, default) when is_list(policies) do
     Enum.map(policies, &set_access_type(&1, default))

lib/ash/query/query.ex

@@ -568,7 +568,7 @@ defmodule Ash.Query do
   def timeout(query, timeout) do
     query = new(query)
 
-    if is_nil(timeout) || Ash.DataLayer.data_layer_can?(query.resource, :timeout) do
+    if Ash.DataLayer.data_layer_can?(query.resource, :timeout) || is_nil(timeout) do
       %{query | timeout: timeout}
     else
       add_error(query, TimeoutNotSupported.exception(resource: query.resource))

mix.exs

@@ -336,8 +336,7 @@ defmodule Ash.MixProject do
   defp deps do
     [
       # DSLs
-      # {:spark, "~> 2.1 and >= 2.2.22"},
-      {:spark, path: "../spark", override: true},
+      {:spark, "~> 2.1 and >= 2.2.22"},
       # Ash resources are backed by ecto scheams
       {:ecto, "~> 3.7"},
       # Used by the ETS data layer

test/actions/load_test.exs

@@ -792,8 +792,6 @@ defmodule Ash.Test.Actions.LoadTest do
 
       assert %{campaign_upcase: "HELLO WORLD"} = author
 
-      Application.put_env(:foo, :bar, true)
-
       assert %{campaign_upcase: "HELLO WORLD"} =
                Ash.load!(author, [:campaign_upcase], lazy?: true)
     end

test/policy/belongs_to_test.exs

@@ -76,7 +76,7 @@ defmodule Ash.Test.Policy.Actions.BelongsToTest do
       })
       |> Ash.create!()
 
-    assert_raise Ash.Error.NotFound, fn ->
+    assert_raise Ash.Error.Forbidden, fn ->
       post
       |> Ash.Changeset.for_update(:update_with_reviewer, %{
         reviewer_id: reviewer.id

test/policy/complex_test.exs

@@ -90,8 +90,11 @@ defmodule Ash.Test.Policy.ComplexTest do
   end
 
   test "it applies policies from the domain", %{me: me} do
-    assert [] ==
-             Ash.read!(Post, actor: %{me | forbidden_by_domain: true})
+    assert_raise Ash.Error.Forbidden,
+                 ~r/authorize unless: actor.forbidden_by_domain == true | ✓ |/,
+                 fn ->
+                   Ash.read!(Post, actor: %{me | forbidden_by_domain: true})
+                 end
   end
 
   test "it properly limits on reads of comments", %{
@@ -184,7 +187,8 @@ defmodule Ash.Test.Policy.ComplexTest do
 
     me |> Ash.load!([:bio_text], authorize?: true, actor: me)
 
-    assert [] ==
-             Ash.read!(Bio, actor: me, authorize?: true)
+    assert_raise Ash.Error.Forbidden, fn ->
+      Ash.read!(Bio, actor: me, authorize?: true)
+    end
   end
 end

test/policy/rbac_test.exs

@@ -53,7 +53,9 @@ defmodule Ash.Test.Policy.RbacTest do
       |> Ash.Query.filter(id == ^org.id)
       |> Ash.Query.load(files: File |> Ash.Query.select([:forbidden]))
 
-    assert [%{files: []}] = Ash.read!(query, actor: user)
+    assert_raise Ash.Error.Forbidden, fn ->
+      Ash.read!(query, actor: user) == []
+    end
 
     # specify no select (everything is selected)
     query =
@@ -61,7 +63,9 @@ defmodule Ash.Test.Policy.RbacTest do
       |> Ash.Query.filter(id == ^org.id)
       |> Ash.Query.load([:files])
 
-    assert [%{files: []}] = Ash.read!(query, actor: user)
+    assert_raise Ash.Error.Forbidden, fn ->
+      Ash.read!(query, actor: user) == []
+    end
 
     # select only an allowed field
     query =

test/policy/selecting_test.exs

@@ -132,7 +132,7 @@ defmodule Ash.Test.Policy.SelectingTest do
     refute is_nil(parent.owner_only_resource)
   end
 
-  test "guest is forbidden from seeing a forbidden field on the rel" do
+  test "guest is forbidden from querying if selecting a forbidden field on the rel" do
     parent =
       Parent
       |> Ash.Changeset.for_create(:create, %{owner_id: "owner", guest_id: "guest"})
@@ -144,13 +144,11 @@ defmodule Ash.Test.Policy.SelectingTest do
     |> Ash.Changeset.for_create(:create)
     |> Ash.create!(authorize?: false)
 
-    assert {:ok, parent} =
+    assert {:error, %Ash.Error.Forbidden{}} =
              Parent
              |> Ash.Query.for_read(:read)
              |> Ash.Query.load(:owner_only_resource)
              |> Ash.Query.limit(1)
              |> Ash.read_one(actor: %{id: "guest"})
-
-    assert %Ash.ForbiddenField{} = parent.owner_only_resource
   end
 end

test/policy/simple_test.exs

@@ -23,18 +23,6 @@ defmodule Ash.Test.Policy.SimpleTest do
     ]
   end
 
-  test "statically known policies will filter all results", %{user: user} do
-    Post
-    |> Ash.Changeset.for_create(:create, %{author: user.id, text: "aaa"})
-    |> Ash.create!(authorize?: false)
-
-    Post
-    |> Ash.Changeset.for_create(:create, %{author: user.id, text: "aaa"})
-    |> Ash.create!(authorize?: false)
-
-    Ash.read!(Post, action: :never_allowed, actor: user)
-  end
-
   test "bypass with condition does not apply subsequent filters", %{admin: admin, user: user} do
     Ash.create!(Ash.Changeset.for_create(Tweet, :create), authorize?: false)
 
@@ -332,9 +320,10 @@ defmodule Ash.Test.Policy.SimpleTest do
     |> Ash.Changeset.for_create(:create, %{user_id: user.id})
     |> Ash.create!(authorize?: false)
 
-    assert [] =
-             Ash.Test.Support.PolicySimple.Always
-             |> Ash.read!(authorize?: true, actor: user)
+    assert_raise Ash.Error.Forbidden, fn ->
+      Ash.Test.Support.PolicySimple.Always
+      |> Ash.read!(authorize?: true, actor: user)
+    end
   end
 
   test "two filter condition checks combine properly" do

test/policy/strict_condition_test.exs

@@ -42,9 +42,10 @@ defmodule Ash.Test.Policy.StrictConditionTest do
     |> Ash.Changeset.for_create(:create, %{visible: false}, authorize?: false)
     |> Ash.create!()
 
-    assert [] ==
-             Resource
-             |> Ash.Query.for_read(:read, %{}, actor: %{id: "foo"})
-             |> Ash.read!()
+    assert_raise Ash.Error.Forbidden, fn ->
+      Resource
+      |> Ash.Query.for_read(:read, %{}, actor: %{id: "foo"})
+      |> Ash.read!()
+    end
   end
 end

test/support/policy_simple/resources/post.ex

@@ -25,10 +25,6 @@ defmodule Ash.Test.Support.PolicySimple.Post do
       forbid_if expr(^arg(:from_an_admin?))
       authorize_if always()
     end
-
-    policy action(:never_allowed) do
-      authorize_if false
-    end
   end
 
   ets do
@@ -69,8 +65,6 @@ defmodule Ash.Test.Support.PolicySimple.Post do
         end
       end
     end
-
-    read :never_allowed
   end
 
   relationships do