improvement: raise when a keyset sort field is forbidden by a field policy

lib/ash/actions/read/read.ex

@@ -491,20 +491,25 @@ defmodule Ash.Actions.Read do
                      opts[:authorize?],
                      false
                    ) do
-              data
-              |> Helpers.restrict_field_access(query)
-              |> add_tenant(new_query)
-              |> attach_fields(opts[:initial_data], initial_query, query, missing_pkeys?)
-              |> cleanup_field_auth(query)
-              |> add_page(
-                query.action,
-                count,
-                query.sort,
-                query,
-                new_query,
-                opts
-              )
-              |> add_query(query, opts)
+              restricted =
+                data
+                |> Helpers.restrict_field_access(query)
+                |> add_tenant(new_query)
+                |> attach_fields(opts[:initial_data], initial_query, query, missing_pkeys?)
+                |> cleanup_field_auth(query)
+
+              with :ok <- reject_forbidden_keyset_sort(restricted, query) do
+                restricted
+                |> add_page(
+                  query.action,
+                  count,
+                  query.sort,
+                  query,
+                  new_query,
+                  opts
+                )
+                |> add_query(query, opts)
+              end
             else
               {:error, %Ash.Query{errors: errors} = query} ->
                 {:error, Ash.Error.to_error_class(errors, query: query)}
@@ -1108,19 +1113,24 @@ defmodule Ash.Actions.Read do
                         opts[:authorize?],
                         false
                       ) do
-                 data
-                 |> Helpers.restrict_field_access(query)
-                 |> add_tenant(query)
-                 |> attach_fields(nil, initial_query, query, false)
-                 |> cleanup_field_auth(query)
-                 |> add_page(
-                   query.action,
-                   count,
-                   query.sort,
-                   initial_query,
-                   query,
-                   opts
-                 )
+                 restricted =
+                   data
+                   |> Helpers.restrict_field_access(query)
+                   |> add_tenant(query)
+                   |> attach_fields(nil, initial_query, query, false)
+                   |> cleanup_field_auth(query)
+
+                 with :ok <- reject_forbidden_keyset_sort(restricted, query) do
+                   add_page(
+                     restricted,
+                     query.action,
+                     count,
+                     query.sort,
+                     initial_query,
+                     query,
+                     opts
+                   )
+                 end
                else
                  {:error, %Ash.Query{errors: errors} = query} ->
                    {:error, Ash.Error.to_error_class(errors, query: query)}
@@ -3052,6 +3062,40 @@ defmodule Ash.Actions.Read do
     end
   end
 
+  # A keyset cursor seeks against the sort field's value, so sorting a
+  # keyset-paginated read on a field a field policy forbids the actor from
+  # reading can't produce a usable cursor. Checked on the field-policy-resolved
+  # records (after `restrict_field_access`), so it covers both static and
+  # record-dependent field policies. Sorts from `Ash.Query.sort_input/2` are
+  # rewritten to be policy-aware earlier and encode `nil`, so they are unaffected.
+  defp reject_forbidden_keyset_sort(records, query) do
+    pagination = query.action && query.action.pagination
+
+    if query.page && pagination && pagination.keyset? &&
+         !use_data_layer_keyset?(query, pagination) do
+      case Ash.Page.Keyset.forbidden_sort_field(
+             List.wrap(records),
+             query.sort || [],
+             query.sort_input_indices || []
+           ) do
+        nil ->
+          :ok
+
+        field ->
+          {:error,
+           Ash.Error.to_error_class(
+             Ash.Error.Page.KeysetSortOnForbiddenField.exception(
+               resource: query.resource,
+               field: field
+             ),
+             query: query
+           )}
+      end
+    else
+      :ok
+    end
+  end
+
   defp remove_already_selected(fields, %struct{results: results})
        when struct in [Ash.Page.Keyset, Ash.Page.Offset],
        do: remove_already_selected(fields, results)

lib/ash/error/page/keyset_sort_on_forbidden_field.ex

@@ -0,0 +1,30 @@
+# SPDX-FileCopyrightText: 2019 ash contributors <https://github.com/ash-project/ash/graphs/contributors>
+#
+# SPDX-License-Identifier: MIT
+
+defmodule Ash.Error.Page.KeysetSortOnForbiddenField do
+  @moduledoc """
+  Used when a keyset-paginated read is sorted on a field a field policy forbids
+  the current actor from reading.
+
+  The field would be redacted on the returned rows, so the keyset cursor could
+  only be built from the redaction marker rather than the sort value, silently
+  breaking pagination. The read is rejected instead.
+
+  If the sort comes from user input, use `Ash.Query.sort_input/2`, which takes
+  field policies into account. Otherwise, avoid sorting on a field this actor
+  cannot read while using keyset pagination.
+  """
+
+  use Splode.Error, fields: [:resource, :field], class: :invalid
+
+  def message(%{resource: resource, field: field}) do
+    """
+    Cannot keyset-paginate #{inspect(resource)} sorted on #{inspect(field)}: a field policy forbids the current actor from reading #{inspect(field)}.
+
+    The field would be redacted on the returned rows, so the keyset cursor could only be built from the redaction marker rather than the sort value, which silently breaks pagination.
+
+    If this sort comes from user input, use `Ash.Query.sort_input/2`. Otherwise, don't sort on #{inspect(field)} with keyset pagination for an actor that cannot read it.
+    """
+  end
+end

lib/ash/page/keyset.ex

@@ -107,6 +107,35 @@ defmodule Ash.Page.Keyset do
     end)
   end
 
+  @doc false
+  # Returns the name of the first sort field that is redacted (`%Ash.ForbiddenField{}`)
+  # on any of the given records, or `nil`. A keyset cursor seeks against the sort
+  # field's value, so a field the actor cannot read can't produce a usable cursor.
+  # This is checked on the field-policy-resolved records, so it covers both static
+  # and record-dependent field policies.
+  #
+  # Sort positions in `sort_input_indices` come from `Ash.Query.sort_input/2` and
+  # were already rewritten to be policy-aware (they encode `nil`, not the value),
+  # so they are skipped.
+  def forbidden_sort_field(records, sort, sort_input_indices) do
+    Enum.find_value(records, fn record ->
+      sort
+      |> Enum.zip(field_values(record, sort))
+      |> Enum.with_index()
+      |> Enum.find_value(fn {{{field, _direction}, value}, index} ->
+        if index not in sort_input_indices and match?(%Ash.ForbiddenField{}, value) do
+          sort_field_name(field)
+        end
+      end)
+    end)
+  end
+
+  defp sort_field_name(%{__struct__: struct, name: name})
+       when struct in [Ash.Query.Calculation, Ash.Query.Aggregate],
+       do: name
+
+  defp sort_field_name(field), do: field
+
   @doc """
   Creates filters on the query using the query for the Keyset.
   """

test/policy/field_policies/keyset_pagination_test.exs

@@ -111,4 +111,140 @@ defmodule Ash.Test.Policy.FieldPolicy.KeysetPaginationTest do
     assert length(page2.results) == 1
     assert Enum.uniq(all_ids) == all_ids
   end
+
+  defmodule AdminDoc do
+    @moduledoc false
+    use Ash.Resource,
+      domain: Domain,
+      data_layer: Ash.DataLayer.Ets,
+      authorizers: [Ash.Policy.Authorizer]
+
+    ets do
+      private? true
+    end
+
+    attributes do
+      uuid_primary_key :id
+
+      attribute :title, :string do
+        public? true
+      end
+
+      attribute :secret_score, :integer do
+        public? true
+      end
+    end
+
+    actions do
+      default_accept [:title, :secret_score]
+      defaults [:create]
+
+      read :read do
+        primary? true
+        pagination keyset?: true, required?: false
+      end
+    end
+
+    policies do
+      policy always() do
+        authorize_if always()
+      end
+    end
+
+    field_policies do
+      field_policy :secret_score do
+        authorize_if actor_attribute_equals(:admin, true)
+      end
+
+      field_policy :* do
+        authorize_if always()
+      end
+    end
+  end
+
+  test "plain sort on a field the actor cannot read raises instead of silently breaking pagination" do
+    for i <- 1..4 do
+      AdminDoc
+      |> Ash.Changeset.for_create(:create, %{title: "t#{i}", secret_score: i * 10},
+        authorize?: false
+      )
+      |> Ash.create!()
+    end
+
+    # a field policy forbids a non-admin from reading `secret_score`, so the
+    # cursor could only be built from the redaction marker. Rather than silently
+    # returning an empty second page, the read is rejected.
+    assert {:error, %Ash.Error.Invalid{errors: errors}} =
+             AdminDoc
+             |> Ash.Query.sort(secret_score: :asc)
+             |> Ash.read(actor: %{admin: false}, page: [limit: 2])
+
+    assert Enum.any?(errors, &match?(%Ash.Error.Page.KeysetSortOnForbiddenField{}, &1))
+
+    # an admin can read the field, so pagination works
+    page1 =
+      AdminDoc
+      |> Ash.Query.sort(secret_score: :asc)
+      |> Ash.read!(actor: %{admin: true}, page: [limit: 2])
+
+    cursor = List.last(page1.results).__metadata__.keyset
+
+    page2 =
+      AdminDoc
+      |> Ash.Query.sort(secret_score: :asc)
+      |> Ash.read!(actor: %{admin: true}, page: [limit: 2, after: cursor])
+
+    assert length(page1.results) == 2
+    assert length(page2.results) == 2
+  end
+
+  test "plain sort on a readable field still paginates under field policies" do
+    for {owner, score} <- [{"me", 10}, {"someone_else", 20}, {"someone_else", 30}] do
+      OwnedDoc
+      |> Ash.Changeset.for_create(
+        :create,
+        %{title: "doc-#{score}", owner_id: owner, secret_score: score},
+        authorize?: false
+      )
+      |> Ash.create!()
+    end
+
+    page1 =
+      OwnedDoc
+      |> Ash.Query.sort(title: :asc)
+      |> Ash.read!(actor: %{id: "me"}, page: [limit: 2])
+
+    cursor = List.last(page1.results).__metadata__.keyset
+
+    page2 =
+      OwnedDoc
+      |> Ash.Query.sort(title: :asc)
+      |> Ash.read!(actor: %{id: "me"}, page: [limit: 2, after: cursor])
+
+    assert length(page1.results) == 2
+    assert length(page2.results) == 1
+  end
+
+  test "plain sort on a record-dependent forbidden field raises" do
+    for {owner, score} <- [{"me", 10}, {"someone_else", 20}, {"someone_else", 30}] do
+      OwnedDoc
+      |> Ash.Changeset.for_create(
+        :create,
+        %{title: "doc", owner_id: owner, secret_score: score},
+        authorize?: false
+      )
+      |> Ash.create!()
+    end
+
+    # `secret_score` is readable for the actor's own rows but not others'. The
+    # field is loaded to sort, so the cursor would otherwise be built from the
+    # real value for rows the actor can't read. Checking the redacted result
+    # rejects the read rather than leaking those values through the cursor.
+    assert {:error, %Ash.Error.Invalid{errors: errors}} =
+             OwnedDoc
+             |> Ash.Query.sort(secret_score: :asc)
+             |> Ash.read(actor: %{id: "me"}, page: [limit: 2])
+
+    assert Enum.any?(errors, &match?(%Ash.Error.Page.KeysetSortOnForbiddenField{}, &1))
+  end
 end