From 005c1bc6c115a5393b8bb07d390591d58dea924c Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Wed, 22 Feb 2023 20:12:08 -0500 Subject: [PATCH] fix: allow argument references in policies --- .../error/forbidden/cannot_filter_creates.ex | 16 ++++++----- lib/ash/policy/check/action.ex | 11 ++++++-- lib/ash/policy/filter_check.ex | 27 ++++++++++++++++--- lib/ash/policy/filter_check_with_context.ex | 16 ++++++++--- test/policy/simple_test.exs | 12 +++++++++ test/support/policy_simple/resources/tweet.ex | 18 +++++++++++-- 6 files changed, 83 insertions(+), 17 deletions(-) diff --git a/lib/ash/error/forbidden/cannot_filter_creates.ex b/lib/ash/error/forbidden/cannot_filter_creates.ex index 05e1b595..2fbd39d4 100644 --- a/lib/ash/error/forbidden/cannot_filter_creates.ex +++ b/lib/ash/error/forbidden/cannot_filter_creates.ex @@ -11,21 +11,23 @@ defmodule Ash.Error.Forbidden.CannotFilterCreates do def message(_) do """ - Filter checks cannot be used with create actions. + Cannot use a filter to authorize a create. If you are using Ash.Policy.Authorizer: - To solve for this, use other checks, or write a custom check. - Many expressions, like those that reference relationships, require using custom checks for create actions. - Expressions that only reference the actor or context, for example `expr(^actor(:is_admin) == true)` will work fine. + Expressions that only reference the actor or context, for example `expr(^actor(:is_admin) == true)` will work + because those are evaluated without needing to reference data. + + For create actions, there is no data yet. In the future we may support referencing simple attributes and those + references will be referring to the values of the data about to be created, but at this time we do not. Given a policy like: ```elixir policy expr(special == true) do - authorize_if expr(allows_special == true) + authorize_if expr(allows_special == true) end ``` @@ -33,7 +35,7 @@ defmodule Ash.Error.Forbidden.CannotFilterCreates do ```elixir policy [expr(special == true), action_type([:read, :update, :destroy])] do - authorize_if expr(allows_special == true) + authorize_if expr(allows_special == true) end ``` @@ -41,7 +43,7 @@ defmodule Ash.Error.Forbidden.CannotFilterCreates do ```elixir policy [changing_attributes(special: [to: true]), action_type(:create)] do - authorize_if changing_attributes(special: [to: true]) + authorize_if changing_attributes(special: [to: true]) end ``` diff --git a/lib/ash/policy/check/action.ex b/lib/ash/policy/check/action.ex index be90236c..895808b5 100644 --- a/lib/ash/policy/check/action.ex +++ b/lib/ash/policy/check/action.ex @@ -4,11 +4,18 @@ defmodule Ash.Policy.Check.Action do @impl true def describe(options) do - "action == #{inspect(options[:action])}" + operator = + if is_list(options[:action]) do + "in" + else + "==" + end + + "action #{operator} #{inspect(options[:action])}" end @impl true def match?(_actor, %{action: %{name: name}}, options) do - name == options[:action] + name in List.wrap(options[:action]) end end diff --git a/lib/ash/policy/filter_check.ex b/lib/ash/policy/filter_check.ex index 842904e6..7987777f 100644 --- a/lib/ash/policy/filter_check.ex +++ b/lib/ash/policy/filter_check.ex @@ -64,7 +64,7 @@ defmodule Ash.Policy.FilterCheck do opts |> filter() - |> Ash.Filter.build_filter_from_template(actor) + |> Ash.Filter.build_filter_from_template(actor, Ash.Policy.FilterCheck.args(authorizer)) |> try_eval(authorizer) |> case do {:ok, false} -> @@ -165,12 +165,22 @@ defmodule Ash.Policy.FilterCheck do def auto_filter(actor, authorizer, opts) do opts = Keyword.put_new(opts, :resource, authorizer.resource) - Ash.Filter.build_filter_from_template(filter(opts), actor) + + Ash.Filter.build_filter_from_template( + filter(opts), + actor, + Ash.Policy.FilterCheck.args(authorizer) + ) end def auto_filter_not(actor, authorizer, opts) do opts = Keyword.put_new(opts, :resource, authorizer.resource) - Ash.Filter.build_filter_from_template(reject(opts), actor) + + Ash.Filter.build_filter_from_template( + reject(opts), + actor, + Ash.Policy.FilterCheck.args(authorizer) + ) end def reject(opts) do @@ -211,4 +221,15 @@ defmodule Ash.Policy.FilterCheck do def is_filter_check?(module) do :erlang.function_exported(module, :filter, 1) end + + @doc false + def args(%{changeset: %{arguments: arguments}}) do + arguments + end + + def args(%{query: %{arguments: arguments}}) do + arguments + end + + def args(_), do: %{} end diff --git a/lib/ash/policy/filter_check_with_context.ex b/lib/ash/policy/filter_check_with_context.ex index 66bc2e46..05ed4984 100644 --- a/lib/ash/policy/filter_check_with_context.ex +++ b/lib/ash/policy/filter_check_with_context.ex @@ -48,7 +48,7 @@ defmodule Ash.Policy.FilterCheckWithContext do actor |> filter(authorizer, opts) - |> Ash.Filter.build_filter_from_template(actor) + |> Ash.Filter.build_filter_from_template(actor, Ash.Policy.FilterCheck.args(authorizer)) |> try_eval(authorizer) |> case do {:ok, false} -> @@ -149,12 +149,22 @@ defmodule Ash.Policy.FilterCheckWithContext do def auto_filter(actor, authorizer, opts) do opts = Keyword.put_new(opts, :resource, authorizer.resource) - Ash.Filter.build_filter_from_template(filter(actor, authorizer, opts), actor) + + Ash.Filter.build_filter_from_template( + filter(actor, authorizer, opts), + actor, + Ash.Policy.FilterCheck.args(authorizer) + ) end def auto_filter_not(actor, authorizer, opts) do opts = Keyword.put_new(opts, :resource, authorizer.resource) - Ash.Filter.build_filter_from_template(reject(actor, authorizer, opts), actor) + + Ash.Filter.build_filter_from_template( + reject(actor, authorizer, opts), + actor, + Ash.Policy.FilterCheck.args(authorizer) + ) end def reject(actor, authorizer, opts) do diff --git a/test/policy/simple_test.exs b/test/policy/simple_test.exs index cf0bba65..301fd1cd 100644 --- a/test/policy/simple_test.exs +++ b/test/policy/simple_test.exs @@ -19,6 +19,18 @@ defmodule Ash.Test.Policy.SimpleTest do assert [] = Api.read!(Tweet, actor: user) end + test "arguments can be referenced in expression policies", %{admin: admin, user: user} do + Tweet + |> Ash.Changeset.for_create(:create_foo, %{foo: "foo", user_id: admin.id}, actor: user) + |> Api.create!() + + assert_raise Ash.Error.Forbidden, fn -> + Tweet + |> Ash.Changeset.for_create(:create_foo, %{foo: "bar", user_id: admin.id}, actor: user) + |> Api.create!() + end + end + test "filter checks work on create/update/destroy actions", %{user: user} do user2 = Api.create!(Ash.Changeset.new(User)) diff --git a/test/support/policy_simple/resources/tweet.ex b/test/support/policy_simple/resources/tweet.ex index f99b5609..27daf565 100644 --- a/test/support/policy_simple/resources/tweet.ex +++ b/test/support/policy_simple/resources/tweet.ex @@ -10,6 +10,10 @@ defmodule Ash.Test.Support.PolicySimple.Tweet do actions do defaults [:create, :read, :update, :destroy] + + create :create_foo do + argument :foo, :string + end end attributes do @@ -21,12 +25,22 @@ defmodule Ash.Test.Support.PolicySimple.Tweet do authorize_if always() end - policy always() do + policy action_type([:read, :update, :destroy]) do authorize_if(expr(user_id == ^actor(:id))) end + + policy action(:create) do + authorize_if relating_to_actor(:user) + end + + policy action(:create_foo) do + authorize_if expr(^arg(:foo) == "foo") + end end relationships do - belongs_to :user, Ash.Test.Support.PolicySimple.User + belongs_to :user, Ash.Test.Support.PolicySimple.User do + attribute_writable? true + end end end