improvement: make read policies more consistent, always preferring to filter over raise

docs: document access type

documentation/topics/security/policies.md

@@ -172,6 +172,41 @@ Policy groups can _not_ contain bypass policies. The purpose of policy groups is
 about the behavior of policies. When you see a policy group, you know that no policies inside that group will
 interact with policies in other policy groups, unless they also apply.
 
+### Access Type
+
+Policies have an "access type" that determines when they are applied. By default, `access_type` is `:filter`.
+When applied to a read action, `:filter` will result in a filtered read. For other action types, the filter will be evaluated
+to determine if a forbidden error should be raised.
+
+There are three access types, and they determine the _latest point in the process_ that any check contained by a policy can be applied.
+
+- `strict` - All checks must be applied statically. These result in a forbidden error if they are not met.
+- `filter` - All checks must be applied either statically or as a filter. These result in a filtered read if they are not met, and a
+  forbidden error for other action types.
+- `runtime` - This allows checks to be run _after_ the data has been read. It is exceedingly rare that you would need to use this access type.
+
+For example, given this policy:
+
+```elixir
+policy action(:read_hidden) do
+  authorize_if expr(actor.is_admin == true)
+end
+```
+
+A non-admin using the `:read_hidden` action would see an empty list of records, rather than a forbidden error.
+
+However, with this policy
+
+```elixir
+policy action(:read_hidden) do
+  access_type :strict
+
+  authorize_if expr(actor.is_admin == true)
+end
+```
+
+A non-admin using the `:read_hidden` action would see a forbidden error.
+
 ## Checks
 
 Checks evaluate from top to bottom within a policy. A check can produce one of three results, the same that a policy can produce. While checks are not necessarily evaluated in order, they _logically apply_ in that order, so you may as well think of it in that way. It can be thought of as a step-through algorithm.

lib/ash/policy/authorizer/authorizer.ex

@@ -1530,18 +1530,22 @@ defmodule Ash.Policy.Authorizer do
         end
 
       {:error, authorizer, :unsatisfiable} ->
-        {:error,
+        if forbidden_due_to_strict_policy?(authorizer) do
-         Ash.Error.Forbidden.Policy.exception(
+          {:error,
-           facts: authorizer.facts,
+           Ash.Error.Forbidden.Policy.exception(
-           domain: Map.get(authorizer, :domain),
+             facts: authorizer.facts,
-           policies: authorizer.policies,
+             domain: Map.get(authorizer, :domain),
-           context_description: opts[:context_description],
+             policies: authorizer.policies,
-           for_fields: opts[:for_fields],
+             context_description: opts[:context_description],
-           resource: Map.get(authorizer, :resource),
+             for_fields: opts[:for_fields],
-           action: Map.get(authorizer, :action),
+             resource: Map.get(authorizer, :resource),
-           actor: Map.get(authorizer, :action),
+             action: Map.get(authorizer, :action),
-           scenarios: []
+             actor: Map.get(authorizer, :action),
-         )}
+             scenarios: []
+           )}
+        else
+          {:filter, authorizer, false}
+        end
 
       {:error, _authorizer, exception} ->
         {:error, Ash.Error.to_ash_error(exception)}
@@ -1552,6 +1556,60 @@ defmodule Ash.Policy.Authorizer do
     strict_filter(%{authorizer | scenarios: scenarios})
   end
 
+  defp forbidden_due_to_strict_policy?(authorizer) do
+    if authorizer.for_fields || authorizer.action.type != :read do
+      true
+    else
+      authorizer.policies
+      |> Enum.any?(fn policy ->
+        policy.access_type == :strict and
+          Enum.all?(policy.condition || [], fn {check_module, check_opts} ->
+            Policy.fetch_fact(authorizer.facts, {check_module, check_opts}) == {:ok, true}
+          end) and
+          policy_fails_statically?(authorizer, policy)
+      end)
+    end
+  end
+
+  defp policy_fails_statically?(authorizer, policy) do
+    Enum.reduce_while(policy.policies, :forbidden, fn check, status ->
+      case check.type do
+        :authorize_if ->
+          if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) ==
+               {:ok, true} do
+            {:halt, :authorized}
+          else
+            {:cont, status}
+          end
+
+        :forbid_if ->
+          if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) ==
+               {:ok, true} do
+            {:halt, :forbidden}
+          else
+            {:cont, status}
+          end
+
+        :authorize_unless ->
+          if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) ==
+               {:ok, true} do
+            {:cont, status}
+          else
+            {:halt, :authorized}
+          end
+
+        :forbid_unless ->
+          if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) ==
+               {:ok, true} do
+            {:cont, status}
+          else
+            {:halt, :forbidden}
+          end
+      end
+    end)
+    |> Kernel.==(:forbidden)
+  end
+
   defp get_policies(authorizer) do
     %{
       authorizer

lib/ash/policy/check/relating_to_actor.ex

@@ -68,6 +68,8 @@ defmodule Ash.Policy.Check.RelatingToActor do
     end
   end
 
+  def filter(_, _, _), do: false
+
   defp take_keys(input, primary_key) do
     Enum.map(primary_key, fn key ->
       Map.get(input, key) || Map.get(input, to_string(key))

test/policy/complex_test.exs

@@ -187,8 +187,6 @@ defmodule Ash.Test.Policy.ComplexTest do
 
     me |> Ash.load!([:bio_text], authorize?: true, actor: me)
 
-    assert_raise Ash.Error.Forbidden, fn ->
+    assert [] = Ash.read!(Bio, actor: me, authorize?: true)
-      Ash.read!(Bio, actor: me, authorize?: true)
-    end
   end
 end

test/policy/rbac_test.exs

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

test/policy/selecting_test.exs

@@ -144,7 +144,7 @@ defmodule Ash.Test.Policy.SelectingTest do
     |> Ash.Changeset.for_create(:create)
     |> Ash.create!(authorize?: false)
 
-    assert {:error, %Ash.Error.Forbidden{}} =
+    assert {:ok, %{owner_only_resource: nil}} =
              Parent
              |> Ash.Query.for_read(:read)
              |> Ash.Query.load(:owner_only_resource)

test/policy/simple_test.exs

@@ -49,6 +49,31 @@ defmodule Ash.Test.Policy.SimpleTest do
     end
   end
 
+  defmodule ResourceWithAStrictReadPolicy do
+    use Ash.Resource,
+      domain: Ash.Test.Domain,
+      authorizers: [Ash.Policy.Authorizer]
+
+    attributes do
+      uuid_primary_key :id
+    end
+
+    actions do
+      defaults [:create, :read]
+    end
+
+    policies do
+      policy action_type(:read) do
+        access_type :strict
+        authorize_if actor_attribute_equals(:admin, true)
+      end
+
+      policy action_type(:read) do
+        authorize_if expr(id == ^actor(:id))
+      end
+    end
+  end
+
   setup do
     Application.put_env(:ash, :policies, show_policy_breakdowns?: true)
 
@@ -81,6 +106,27 @@ defmodule Ash.Test.Policy.SimpleTest do
                  end
   end
 
+  test "strict read policies do not result in a filter" do
+    thing =
+      ResourceWithAStrictReadPolicy
+      |> Ash.create!(authorize?: false)
+
+    actor = %{id: thing, admin: false}
+
+    assert_raise Ash.Error.Forbidden, fn ->
+      ResourceWithAStrictReadPolicy
+      |> Ash.Query.new()
+      |> Ash.DataLayer.Simple.set_data([thing])
+      |> Ash.read!(actor: actor)
+    end
+
+    assert [] =
+             ResourceWithAStrictReadPolicy
+             |> Ash.Query.new()
+             |> Ash.DataLayer.Simple.set_data([%{thing | id: Ash.UUID.generate()}])
+             |> Ash.read!(actor: %{admin: true})
+  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)
 

test/support/policy_complex/domain.ex

@@ -13,6 +13,7 @@ defmodule Ash.Test.Support.PolicyComplex.Domain do
 
   policies do
     policy always() do
+      access_type :strict
       authorize_unless actor_attribute_equals(:forbidden_by_domain, true)
     end
   end