From bad62c1e8a59cccc43fc6de04b575f459bfa820c Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Mon, 22 Jul 2024 10:01:39 -0400 Subject: [PATCH] improvement: add `authorize_with` fallback option to bulk actions improvement: store non-expr atomic changes in attributes for simplicity fix: make `relating_to_actor` built-in-check aware of atomics closes #1327 --- .formatter.exs | 2 + .../dsls/DSL:-Ash.Policy.Authorizer.md | 8 +- lib/ash.ex | 10 +- lib/ash/actions/destroy/bulk.ex | 2 +- lib/ash/actions/update/bulk.ex | 5 +- lib/ash/changeset/changeset.ex | 108 ++++++++++++------ lib/ash/code_interface.ex | 12 +- lib/ash/policy/check/relating_to_actor.ex | 73 ++++++++---- test/policy/simple_test.exs | 52 +++++++++ test/support/policy_simple/resources/tweet.ex | 9 ++ 10 files changed, 202 insertions(+), 79 deletions(-) diff --git a/.formatter.exs b/.formatter.exs index 126a81887..6cfbca164 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -36,6 +36,7 @@ spark_locals_without_parens = [ belongs_to: 2, belongs_to: 3, broadcast_type: 1, + bypass: 0, bypass: 1, bypass: 2, calculate: 2, @@ -152,6 +153,7 @@ spark_locals_without_parens = [ pagination: 1, parse_attribute: 1, plural_name: 1, + policy: 0, policy: 1, policy: 2, pre_check?: 1, diff --git a/documentation/dsls/DSL:-Ash.Policy.Authorizer.md b/documentation/dsls/DSL:-Ash.Policy.Authorizer.md index ce8cb8bcd..1be64e30c 100644 --- a/documentation/dsls/DSL:-Ash.Policy.Authorizer.md +++ b/documentation/dsls/DSL:-Ash.Policy.Authorizer.md @@ -91,7 +91,7 @@ end ## policies.policy ```elixir -policy condition +policy condition \\ nil ``` @@ -314,7 +314,7 @@ Target: `Ash.Policy.Policy` ## policies.bypass ```elixir -bypass condition +bypass condition \\ nil ``` @@ -594,7 +594,7 @@ end ## field_policies.field_policy_bypass ```elixir -field_policy_bypass fields, condition \\ {Ash.Policy.Check.Static, [result: true]} +field_policy_bypass fields, condition \\ nil ``` @@ -791,7 +791,7 @@ Target: `Ash.Policy.FieldPolicy` ## field_policies.field_policy ```elixir -field_policy fields, condition \\ {Ash.Policy.Check.Static, [result: true]} +field_policy fields, condition \\ nil ``` diff --git a/lib/ash.ex b/lib/ash.ex index 9e61f227c..339c109ca 100644 --- a/lib/ash.ex +++ b/lib/ash.ex @@ -309,15 +309,19 @@ defmodule Ash do ], authorize_query_with: [ type: {:one_of, [:filter, :error]}, - default: :filter, doc: - "If set to `:error`, instead of filtering unauthorized query results, unauthorized query results will raise an appropriate forbidden error" + "If set to `:error`, instead of filtering unauthorized query results, unauthorized query results will raise an appropriate forbidden error. Uses `authorize_with` if not set." ], authorize_changeset_with: [ + type: {:one_of, [:filter, :error]}, + doc: + "If set to `:error`, instead of filtering unauthorized changes, unauthorized changes will raise an appropriate forbidden error. Uses `authorize_with` if not set." + ], + authorize_with: [ type: {:one_of, [:filter, :error]}, default: :filter, doc: - "If set to `:error`, instead of filtering unauthorized changes, unauthorized changes will raise an appropriate forbidden error" + "If set to `:error`, instead of filtering unauthorized query results, unauthorized query results will raise an appropriate forbidden error." ], context: [ type: :map, diff --git a/lib/ash/actions/destroy/bulk.ex b/lib/ash/actions/destroy/bulk.ex index 957df86e6..6b8d390d4 100644 --- a/lib/ash/actions/destroy/bulk.ex +++ b/lib/ash/actions/destroy/bulk.ex @@ -1112,7 +1112,7 @@ defmodule Ash.Actions.Destroy.Bulk do return_forbidden_error?: true, pre_flight?: false, atomic_changeset: atomic_changeset, - filter_with: opts[:authorize_query_with] || :filter, + filter_with: opts[:authorize_query_with] || opts[:authorize_with] || :filter, run_queries?: false, maybe_is: false, alter_source?: true diff --git a/lib/ash/actions/update/bulk.ex b/lib/ash/actions/update/bulk.ex index 5efe39c12..c7146f488 100644 --- a/lib/ash/actions/update/bulk.ex +++ b/lib/ash/actions/update/bulk.ex @@ -500,6 +500,7 @@ defmodule Ash.Actions.Update.Bulk do %Ash.Changeset{valid?: true} = atomic_changeset <- Ash.Changeset.handle_allow_nil_atomics(atomic_changeset, opts[:actor]), atomic_changeset <- sort_atomic_changes(atomic_changeset), + atomic_changeset <- Ash.Changeset.move_attributes_to_atomics(atomic_changeset), {:ok, data_layer_query} <- Ash.Query.data_layer_query(query) do case Ash.DataLayer.update_query( @@ -1240,7 +1241,7 @@ defmodule Ash.Actions.Update.Bulk do maybe_is: false, pre_flight?: false, atomic_changeset: atomic_changeset, - filter_with: opts[:authorize_query_with] || :filter, + filter_with: opts[:authorize_query_with] || opts[:authorize_with] || :filter, run_queries?: false, alter_source?: true, no_check?: true @@ -1275,7 +1276,7 @@ defmodule Ash.Actions.Update.Bulk do on_must_pass_strict_check: {:error, %Ash.Error.Forbidden.InitialDataRequired{source: "must pass strict check"}}, - filter_with: opts[:authorize_changeset_with] || :filter, + filter_with: opts[:authorize_changeset_with] || opts[:authorize_with] || :filter, alter_source?: true, run_queries?: false, base_query: query diff --git a/lib/ash/changeset/changeset.ex b/lib/ash/changeset/changeset.ex index d1a573327..2085ae58f 100644 --- a/lib/ash/changeset/changeset.ex +++ b/lib/ash/changeset/changeset.ex @@ -947,16 +947,21 @@ defmodule Ash.Changeset do {:cont, %{changeset | arguments: Map.put(changeset.arguments, key, value)}} attribute = Ash.Resource.Info.attribute(changeset.resource, key) -> - case Ash.Type.cast_atomic(attribute.type, value, attribute.constraints) do - {:atomic, atomic} -> - atomic = set_error_field(atomic, attribute.name) - {:cont, atomic_update(changeset, attribute.name, {:atomic, atomic})} + if Ash.Expr.expr?(value) do + case Ash.Type.cast_atomic(attribute.type, value, attribute.constraints) do + {:atomic, atomic} -> + atomic = set_error_field(atomic, attribute.name) + {:cont, atomic_update(changeset, attribute.name, {:atomic, atomic})} - {:error, error} -> - {:cont, add_invalid_errors(value, :attribute, changeset, attribute, error)} + {:error, error} -> + {:cont, add_invalid_errors(value, :attribute, changeset, attribute, error)} - {:not_atomic, reason} -> - {:halt, {:not_atomic, reason}} + {:not_atomic, reason} -> + {:halt, {:not_atomic, reason}} + end + else + {:cont, + %{changeset | attributes: Map.put(changeset.attributes, attribute.name, value)}} end match?("_" <> _, key) -> @@ -987,15 +992,19 @@ defmodule Ash.Changeset do attribute = Ash.Resource.Info.attribute(changeset.resource, key) -> cond do attribute.name in action.accept -> - case Ash.Type.cast_atomic(attribute.type, value, attribute.constraints) do - {:atomic, atomic} -> - {:cont, atomic_update(changeset, attribute.name, {:atomic, atomic})} + if Ash.Expr.expr?(value) do + case Ash.Type.cast_atomic(attribute.type, value, attribute.constraints) do + {:atomic, atomic} -> + {:cont, atomic_update(changeset, attribute.name, {:atomic, atomic})} - {:error, error} -> - {:cont, add_invalid_errors(value, :attribute, changeset, attribute, error)} + {:error, error} -> + {:cont, add_invalid_errors(value, :attribute, changeset, attribute, error)} - {:not_atomic, reason} -> - {:halt, {:not_atomic, reason}} + {:not_atomic, reason} -> + {:halt, {:not_atomic, reason}} + end + else + {:cont, force_change_attribute(changeset, attribute.name, value)} end key in List.wrap(opts[:skip_unknown_inputs]) -> @@ -1457,7 +1466,7 @@ defmodule Ash.Changeset do set_error_field(value, attribute.name) end - %{changeset | atomics: Keyword.put(changeset.atomics, key, value)} + %{changeset | atomics: Keyword.put(changeset.atomics, attribute.name, value)} |> record_atomic_update_for_atomic_upgrade(attribute.name, value) {:not_atomic, message} -> @@ -1468,6 +1477,13 @@ defmodule Ash.Changeset do end end + @doc false + def move_attributes_to_atomics(changeset) do + Enum.reduce(changeset.attributes, changeset, fn {key, value}, changeset -> + %{changeset | atomics: Keyword.put_new(changeset.atomics, key, value)} + end) + end + @doc false def handle_allow_nil_atomics(changeset, actor) do changeset.atomics @@ -2256,26 +2272,29 @@ defmodule Ash.Changeset do end defp do_hydrate_atomic_refs(changeset, actor) do - Enum.reduce_while(changeset.atomics, {:ok, %{changeset | atomics: []}}, fn {key, expr}, - {:ok, changeset} -> - expr = - Ash.Expr.fill_template( - expr, - actor, - changeset.arguments, - changeset.context, - changeset - ) + Enum.reduce_while( + changeset.atomics, + {:ok, %{changeset | atomics: []}}, + fn {key, expr}, {:ok, changeset} -> + expr = + Ash.Expr.fill_template( + expr, + actor, + changeset.arguments, + changeset.context, + changeset + ) - case Ash.Filter.hydrate_refs(expr, %{resource: changeset.resource, public?: false}) do - {:ok, expr} -> - {:cont, {:ok, %{changeset | atomics: Keyword.put(changeset.atomics, key, expr)}}} + case Ash.Filter.hydrate_refs(expr, %{resource: changeset.resource, public?: false}) do + {:ok, expr} -> + {:cont, {:ok, %{changeset | atomics: Keyword.put(changeset.atomics, key, expr)}}} - {:error, error} -> - {:halt, - {:not_atomic, "Failed to validate expression #{inspect(expr)}: #{inspect(error)}"}} + {:error, error} -> + {:halt, + {:not_atomic, "Failed to validate expression #{inspect(expr)}: #{inspect(error)}"}} + end end - end) + ) end @doc false @@ -4582,9 +4601,9 @@ defmodule Ash.Changeset do """ @spec update_change(t(), atom, (any -> any)) :: t() def update_change(changeset, attribute, fun) do - case Ash.Changeset.fetch_change(changeset, attribute) do + case fetch_change(changeset, attribute) do {:ok, change} -> - Ash.Changeset.force_change_attribute(changeset, attribute, fun.(change)) + force_change_attribute(changeset, attribute, fun.(change)) :error -> changeset @@ -4773,7 +4792,15 @@ defmodule Ash.Changeset do ), casted}, {{:ok, casted}, _} <- {Ash.Type.apply_constraints(attribute.type, casted, constraints), casted} do - data_value = Map.get(changeset.data, attribute.name) + data_value = + case changeset.data do + %Ash.Changeset.OriginalDataNotAvailable{} -> + nil + + data -> + Map.get(data, attribute.name) + end + changeset = remove_default(changeset, attribute.name) cond do @@ -4895,7 +4922,14 @@ defmodule Ash.Changeset do {:ok, casted} <- handle_change(changeset, attribute, casted, constraints), {:ok, casted} <- Ash.Type.apply_constraints(attribute.type, casted, constraints) do - data_value = Map.get(changeset.data, attribute.name) + data_value = + case changeset.data do + %Ash.Changeset.OriginalDataNotAvailable{} -> + nil + + data -> + Map.get(data, attribute.name) + end changeset = remove_default(changeset, attribute.name) diff --git a/lib/ash/code_interface.ex b/lib/ash/code_interface.ex index a5ae509df..ea94bfd4d 100644 --- a/lib/ash/code_interface.ex +++ b/lib/ash/code_interface.ex @@ -832,8 +832,7 @@ defmodule Ash.CodeInterface do bulk_opts |> Keyword.put(:return_records?, true) |> Keyword.put(:return_errors?, true) - |> Keyword.put_new(:authorize_query_with, :error) - |> Keyword.put_new(:authorize_changeset_with, :error) + |> Keyword.put_new(:authorize_with, :error) |> Keyword.put(:notify?, true) else bulk_opts @@ -903,8 +902,7 @@ defmodule Ash.CodeInterface do bulk_opts |> Keyword.put(:return_records?, true) |> Keyword.put(:return_errors?, true) - |> Keyword.put_new(:authorize_query_with, :error) - |> Keyword.put_new(:authorize_changeset_with, :error) + |> Keyword.put_new(:authorize_with, :error) |> Keyword.put(:notify?, true) else bulk_opts @@ -1051,8 +1049,7 @@ defmodule Ash.CodeInterface do bulk_opts |> Keyword.put(:return_records?, opts[:return_destroyed?]) |> Keyword.put(:return_errors?, true) - |> Keyword.put_new(:authorize_query_with, :error) - |> Keyword.put_new(:authorize_changeset_with, :error) + |> Keyword.put_new(:authorize_with, :error) |> Keyword.put(:notify?, true) else Keyword.put(bulk_opts, :return_records?, opts[:return_destroyed?]) @@ -1139,8 +1136,7 @@ defmodule Ash.CodeInterface do bulk_opts |> Keyword.put(:return_records?, opts[:return_destroyed?]) |> Keyword.put(:return_errors?, true) - |> Keyword.put_new(:authorize_query_with, :error) - |> Keyword.put_new(:authorize_changeset_with, :error) + |> Keyword.put_new(:authorize_with, :error) |> Keyword.put(:notify?, true) else Keyword.put(bulk_opts, :return_records?, opts[:return_destroyed?]) diff --git a/lib/ash/policy/check/relating_to_actor.ex b/lib/ash/policy/check/relating_to_actor.ex index 54286a3fe..a82d7dc3c 100644 --- a/lib/ash/policy/check/relating_to_actor.ex +++ b/lib/ash/policy/check/relating_to_actor.ex @@ -1,6 +1,6 @@ defmodule Ash.Policy.Check.RelatingToActor do @moduledoc "This check is true when the specified relationship is being changed to the current actor." - use Ash.Policy.SimpleCheck + use Ash.Policy.FilterCheck @impl true def describe(opts) do @@ -8,41 +8,66 @@ defmodule Ash.Policy.Check.RelatingToActor do end @impl true - def requires_original_data?(_, _), do: false + def filter(nil, _, _), do: false - @impl true - def match?(nil, _, _), do: false - - def match?(actor, %{changeset: %Ash.Changeset{} = changeset}, opts) do + def filter(actor, %{changeset: %Ash.Changeset{} = changeset}, opts) do resource = changeset.resource relationship = Ash.Resource.Info.relationship(resource, opts[:relationship]) - unless relationship.type == :belongs_to do - raise "Can only use `belongs_to` relationships in relating_to_actor checks" - end - - if Ash.Changeset.changing_attribute?(changeset, relationship.source_attribute) do - Ash.Changeset.get_attribute(changeset, relationship.source_attribute) == - Map.get(actor, relationship.destination_attribute) + if is_nil(Map.get(actor, relationship.destination_attribute)) do + false else - case changeset.relationships[relationship.name] do - [{[input], opts}] -> - primary_key = Ash.Resource.Info.primary_key(relationship.destination) + unless relationship.type == :belongs_to do + raise "Can only use `belongs_to` relationships in relating_to_actor checks" + end - actor_keys = take_keys(actor, primary_key) - input_keys = take_keys(input, primary_key) + if Ash.Changeset.changing_attribute?(changeset, relationship.source_attribute) do + if changeset.action.type == :create do + actor_value = Map.get(actor, relationship.destination_attribute) - opts[:on_lookup] == :relate and Enum.all?(actor_keys, & &1) && - Enum.all?(input_keys, & &1) and input_keys == actor_keys + case Map.fetch(changeset.attributes, relationship.source_attribute) do + {:ok, ^actor_value} -> + if Keyword.has_key?(changeset.atomics, relationship.source_attribute) do + expr( + ^atomic_ref(relationship.source_attribute) == + ^Map.get(actor, relationship.destination_attribute) + ) + else + true + end - _ -> - false + _ -> + false + end + else + if Keyword.has_key?(changeset.atomics, relationship.source_attribute) do + expr( + ^atomic_ref(relationship.source_attribute) == + ^Map.get(actor, relationship.destination_attribute) + ) + else + Ash.Changeset.get_attribute(changeset, relationship.source_attribute) == + Map.get(actor, relationship.destination_attribute) + end + end + else + case changeset.relationships[relationship.name] do + [{[input], opts}] -> + primary_key = Ash.Resource.Info.primary_key(relationship.destination) + + actor_keys = take_keys(actor, primary_key) + input_keys = take_keys(input, primary_key) + + opts[:on_lookup] == :relate and Enum.all?(actor_keys, & &1) && + Enum.all?(input_keys, & &1) and input_keys == actor_keys + + _ -> + false + end end end end - def match?(_, _, _), 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)) diff --git a/test/policy/simple_test.exs b/test/policy/simple_test.exs index 65b1018f3..f4bfa27ee 100644 --- a/test/policy/simple_test.exs +++ b/test/policy/simple_test.exs @@ -66,6 +66,58 @@ defmodule Ash.Test.Policy.SimpleTest do end end + test "relating_to_actor/1 works when creating", %{user: user} do + Tweet + |> Ash.Changeset.for_create(:create, %{user_id: user.id}) + |> Ash.create!(authorize?: true, actor: user) + + assert_raise Ash.Error.Forbidden, fn -> + Tweet + |> Ash.Changeset.for_create(:create, %{user_id: Ash.UUID.generate()}) + |> Ash.create!(authorize?: true, actor: user) + end + end + + test "relating_to_actor/1 works when updating", %{user: user} do + Tweet + |> Ash.Changeset.for_create(:create, %{user_id: user.id}) + |> Ash.create!(authorize?: false, actor: user) + + Ash.bulk_update!(Tweet, :set_user, %{user_id: user.id}, + actor: user, + authorize?: true, + authorize_with: :error + ) + + assert_raise Ash.Error.Forbidden, fn -> + Ash.bulk_update!(Tweet, :set_user, %{user_id: Ash.UUID.generate()}, + actor: user, + authorize?: true, + authorize_with: :error + ) + end + end + + test "relating_to_actor/1 works when updating non-atomically", %{user: user} do + tweet = + Tweet + |> Ash.Changeset.for_create(:create, %{user_id: user.id}) + |> Ash.create!(authorize?: false, actor: user) + + tweet + |> Ash.Changeset.for_update(:set_user, %{user_id: user.id}, actor: user, authorize?: true) + |> Ash.update!() + + assert_raise Ash.Error.Forbidden, fn -> + tweet + |> Ash.Changeset.for_update(:set_user, %{user_id: Ash.UUID.generate()}, + actor: user, + authorize?: true + ) + |> Ash.update!() + end + end + test "filter checks work on update/destroy actions", %{user: user} do tweet = Tweet diff --git a/test/support/policy_simple/resources/tweet.ex b/test/support/policy_simple/resources/tweet.ex index fd77c4c5c..eb12c0d80 100644 --- a/test/support/policy_simple/resources/tweet.ex +++ b/test/support/policy_simple/resources/tweet.ex @@ -13,6 +13,11 @@ defmodule Ash.Test.Support.PolicySimple.Tweet do default_accept :* defaults [:read, :destroy, create: :*, update: :*] + update :set_user do + argument :user_id, :uuid, allow_nil?: false + change atomic_update(:user_id, arg(:user_id)) + end + create :create_foo do argument :foo, :string end @@ -39,6 +44,10 @@ defmodule Ash.Test.Support.PolicySimple.Tweet do authorize_if relating_to_actor(:user) end + policy action(:set_user) do + authorize_if relating_to_actor(:user) + end + policy action(:create_foo) do authorize_if expr(is_foo(foo: ^arg(:foo))) end