improvement: show informative error explaining the use of filter checks with create actions

improvement: show the actor's primary key in policy breakdowns
improvement: add an expanded description option to checks
improvement: use expanded description to display filled in filter templates in policy breakdowns
This commit is contained in:
Zach Daniel 2024-09-06 09:45:17 -04:00
parent b13181730b
commit 5f5cccc2ea
12 changed files with 308 additions and 80 deletions

View file

@ -146,7 +146,7 @@ defmodule Ash.Actions.Create do
if opts[:authorize?] do
case Ash.can(changeset, opts[:actor],
alter_source?: true,
pre_flight?: false,
pre_flight?: true,
return_forbidden_error?: true,
maybe_is: false
) do

View file

@ -1977,7 +1977,7 @@ defmodule Ash.Actions.Read do
agg.query
|> Ash.Query.set_context(%{private: %{require_actor?: false}})
|> Ash.Query.for_read(read_action, %{}, domain: domain)
|> Ash.Query.for_read(read_action, %{}, domain: domain, actor: actor, tenant: tenant)
else
agg.query
end

View file

@ -423,21 +423,22 @@ defmodule Ash.Can do
authorizers ->
authorizers
|> Enum.reduce_while(
{false, base_query},
fn {authorizer, authorizer_state, context}, {_authorized?, query} ->
{false, base_query, []},
fn {authorizer, authorizer_state, context}, {_authorized?, query, authorizers} ->
case authorizer.strict_check(authorizer_state, context) do
{:error, %{class: :forbidden} = e} when is_exception(e) ->
{:halt, {false, e}}
{:halt, {false, e, {authorizer, authorizer_state, context}}}
{:error, error} ->
{:halt, {:error, error}}
{:halt, {:error, authorizer, error}}
{:authorized, _} ->
{:cont, {true, query}}
{:authorized, authorizer_state} ->
{:cont, {true, query, [{authorizer, authorizer_state, context} | authorizers]}}
:forbidden ->
{:halt,
{false, Ash.Authorizer.exception(authorizer, :forbidden, authorizer_state)}}
{false, Ash.Authorizer.exception(authorizer, :forbidden, authorizer_state),
{authorizer, authorizer_state, context}}}
_ when not is_nil(context.action_input) ->
raise """
@ -470,7 +471,7 @@ defmodule Ash.Can do
authorizer,
authorizer_state,
opts
)}}
), [{authorizer, authorizer_state, context} | authorizers]}}
{:filter, filter} ->
filter =
@ -496,13 +497,13 @@ defmodule Ash.Can do
authorizer,
authorizer_state,
opts
)}}
), [{authorizer, authorizer_state, context} | authorizers]}}
{:continue, authorizer_state} ->
if opts[:no_check?] do
{:halt,
opts[:on_must_pass_strict_check] ||
{:error,
{:error, {authorizer, authorizer_state, context},
Ash.Authorizer.exception(
authorizer,
:must_pass_strict_check,
@ -525,13 +526,17 @@ defmodule Ash.Can do
end
)
{:cont, {true, query_with_hook}}
{:cont,
{true, query_with_hook,
[{authorizer, authorizer_state, context} | authorizers]}}
else
if opts[:maybe_is] == false do
{:halt,
{false, Ash.Authorizer.exception(authorizer, :forbidden, authorizer_state)}}
{false, Ash.Authorizer.exception(authorizer, :forbidden, authorizer_state),
{authorizer, authorizer_state, context}}}
else
{:halt, {:maybe, nil}}
{:halt,
{:maybe, nil, [{authorizer, authorizer_state, context} | authorizers]}}
end
end
end
@ -551,11 +556,12 @@ defmodule Ash.Can do
end
if opts[:no_check?] || !match?(%Ash.Query{}, subject) do
Ash.Authorizer.exception(
authorizer,
:must_pass_strict_check,
authorizer_state
)
{:error, {authorizer, authorizer_state, context},
Ash.Authorizer.exception(
authorizer,
:must_pass_strict_check,
authorizer_state
)}
else
if opts[:alter_source?] do
query_with_hook =
@ -578,13 +584,17 @@ defmodule Ash.Can do
end
end)
{:cont, {true, query_with_hook}}
{:cont,
{true, query_with_hook,
[{authorizer, authorizer_state, context} | authorizers]}}
else
if opts[:maybe_is] == false do
{:halt,
{false, Ash.Authorizer.exception(authorizer, :forbidden, authorizer_state)}}
{false, Ash.Authorizer.exception(authorizer, :forbidden, authorizer_state),
authorizer}}
else
{:halt, {:maybe, nil}}
{:halt,
{:maybe, nil, [{authorizer, authorizer_state, context} | authorizers]}}
end
end
end
@ -592,10 +602,13 @@ defmodule Ash.Can do
end
)
|> case do
{:error, error} ->
{:error, _authorizer, error} ->
{:error, error}
{true, query} when not is_nil(query) ->
{true, nil, _} ->
{:ok, true}
{true, query, authorizers} when not is_nil(query) ->
if opts[:run_queries?] do
run_queries(subject, actor, opts, authorizers, query)
else
@ -606,26 +619,19 @@ defmodule Ash.Can do
end
end
{false, error} ->
{false, error, authorizer} ->
if opts[:return_forbidden_error?] do
{:ok, false, error || authorizer_exception(authorizers)}
{:ok, false, error || authorizer_exception([authorizer])}
else
{:ok, false}
end
{other, _} ->
{:ok, other}
end
|> case do
{:ok, :maybe} ->
{:maybe, _v, authorizers} ->
if opts[:maybe_is] == false && opts[:return_forbidden_error?] do
{:ok, false, authorizer_exception(authorizers)}
else
{:ok, opts[:maybe_is]}
end
other ->
other
end
end
end
@ -768,11 +774,7 @@ defmodule Ash.Can do
end
%Ash.Changeset{} ->
if opts[:return_forbidden_error?] do
{:ok, false, authorizer_exception(authorizers)}
else
{:ok, false}
end
raise Ash.Error.Forbidden.CannotFilterCreates, filter: query.filter
end
end

View file

@ -1,15 +1,23 @@
defmodule Ash.Error.Forbidden.CannotFilterCreates do
@moduledoc "Used when a create action would be filtered"
use Splode.Error, fields: [], class: :forbidden
use Splode.Error, fields: [:filter], class: :forbidden
def message(error) do
filter =
case error.filter do
%Ash.Filter{expression: expression} -> expression
other -> other
end
def message(_) do
"""
Cannot use a filter to authorize a create.
Filter: #{inspect(filter)}
If you are using Ash.Policy.Authorizer:
Many expressions, like those that reference relationships, require using custom checks for create actions.
Many expressions, like those that reference relationships, require using custom checks when used with create actions.
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.
@ -19,27 +27,21 @@ defmodule Ash.Error.Forbidden.CannotFilterCreates do
Given a policy like:
```elixir
policy expr(special == true) do
authorize_if expr(allows_special == true)
end
```
policy expr(special == true) do
authorize_if expr(allows_special == true)
end
You would rewrite it to not include create actions like so:
```elixir
policy [expr(special == true), action_type([:read, :update, :destroy])] do
authorize_if expr(allows_special == true)
end
```
policy [expr(special == true), action_type([:read, :update, :destroy])] do
authorize_if expr(allows_special == true)
end
At which point you could add a `create` specific policy:
```elixir
policy [changing_attributes(special: [to: true]), action_type(:create)] do
authorize_if changing_attributes(special: [to: true])
end
```
policy [changing_attributes(special: [to: true]), action_type(:create)] do
authorize_if changing_attributes(special: [to: true])
end
In these cases, you may also end up wanting to write a custom check.
"""

View file

@ -15,6 +15,7 @@ defmodule Ash.Error.Forbidden.Policy do
policy_breakdown?: false,
must_pass_strict_check?: false,
for_fields: nil,
subject: nil,
context_description: nil,
policies: [],
resource: nil,
@ -93,7 +94,9 @@ defmodule Ash.Error.Forbidden.Policy do
Keyword.merge(opts,
domain: error.domain,
resource: error.resource,
actor: error.actor,
must_pass_strict_check?: must_pass_strict_check?,
subject: error.subject,
context_description: error.context_description,
for_fields: error.for_fields
)
@ -162,6 +165,28 @@ defmodule Ash.Error.Forbidden.Policy do
["Policy Breakdown#{policy_context_description}"]
end
actor =
case opts[:actor] do
nil ->
"unknown actor"
%resource{} = actor ->
if Ash.Resource.Info.resource?(resource) do
case Ash.Resource.Info.primary_key(actor) do
[] ->
" Actor: #{inspect(actor)}"
fields ->
" #{Ash.Resource.Info.short_name(resource)}: #{inspect(Map.take(actor, fields))}"
end
else
" Actor: #{inspect(actor)}"
end
actor ->
" Actor: #{inspect(actor)}"
end
policy_explanation =
policies
|> Kernel.||([])
@ -197,8 +222,13 @@ defmodule Ash.Error.Forbidden.Policy do
end
|> then(fn {policies, title} ->
policies
|> Enum.map(&explain_policy(&1, facts, opts[:success?] || false))
|> Enum.map(
&explain_policy(&1, facts, opts[:success?] || false, opts[:actor], opts[:subject])
)
|> Enum.intersperse("\n")
|> then(fn list ->
Enum.concat([actor, "\n\n"], list)
end)
|> title(title, false)
end)
@ -247,7 +277,7 @@ defmodule Ash.Error.Forbidden.Policy do
defp relevant?(policy, facts) do
Enum.all?(policy.condition || [], fn condition ->
Policy.fetch_fact(facts, condition) == {:ok, true}
Policy.fetch_fact(facts, condition) != {:ok, false}
end)
end
@ -255,7 +285,7 @@ defmodule Ash.Error.Forbidden.Policy do
defp title(other, title, true), do: [title, ":\n", other]
defp title(other, title, false), do: [title, "\n", other]
defp explain_policy(policy, facts, success?) do
defp explain_policy(policy, facts, success?, actor, subject) do
bypass =
if policy.bypass? do
"Bypass: "
@ -263,10 +293,11 @@ defmodule Ash.Error.Forbidden.Policy do
""
end
{condition_description, applies} = describe_conditions(policy.condition, facts)
{condition_description, applies} =
describe_conditions(policy.condition, facts, actor, subject)
if applies == true do
{description, state} = describe_checks(policy.policies, facts, success?)
{description, state} = describe_checks(policy.policies, facts, success?, actor, subject)
tag =
case state do
@ -306,7 +337,7 @@ defmodule Ash.Error.Forbidden.Policy do
end
end
defp describe_conditions(condition, facts) do
defp describe_conditions(condition, facts, actor, subject) do
condition
|> List.wrap()
|> Enum.reduce({[], true}, fn condition, {conditions, status} ->
@ -335,7 +366,7 @@ defmodule Ash.Error.Forbidden.Policy do
end
end
{[["condition: ", mod.describe(opts)] | conditions], new_status}
{[["condition: ", describe(mod, opts, actor, subject)] | conditions], new_status}
end)
|> then(fn {conditions, status} ->
conditions =
@ -357,7 +388,7 @@ defmodule Ash.Error.Forbidden.Policy do
end)
end
defp describe_checks(checks, facts, success?) do
defp describe_checks(checks, facts, success?, actor, subject) do
{description, state} =
Enum.reduce(checks, {[], :unknown}, fn check, {descriptions, state} ->
new_state =
@ -400,7 +431,9 @@ defmodule Ash.Error.Forbidden.Policy do
Policy.fetch_fact(facts, check.check),
tag,
success?,
filter_check?
filter_check?,
actor,
subject
)
| descriptions
], new_state}
@ -409,7 +442,7 @@ defmodule Ash.Error.Forbidden.Policy do
{Enum.intersperse(Enum.reverse(description), "\n"), state}
end
defp describe_check(check, fact_result, tag, success?, filter_check?) do
defp describe_check(check, fact_result, tag, success?, filter_check?, actor, subject) do
fact_result =
case fact_result do
{:ok, true} ->
@ -428,7 +461,7 @@ defmodule Ash.Error.Forbidden.Policy do
[
check_type(check),
": ",
check.check_module.describe(check.check_opts),
describe(check.check_module, check.check_opts, actor, subject),
" | ",
fact_result,
" | ",
@ -436,6 +469,41 @@ defmodule Ash.Error.Forbidden.Policy do
]
end
defp describe(mod, opts, actor, subject) do
description = mod.describe(opts)
if subject && function_exported?(mod, :expand_description, 3) do
authorizer =
%Ash.Policy.Authorizer{
subject: subject,
actor: actor
}
key =
case subject do
%Ash.Changeset{} -> :changeset
%Ash.Query{} -> :query
%Ash.ActionInput{} -> :action_input
end
authorizer = Map.put(authorizer, key, subject)
case mod.expand_description(actor, authorizer, opts) do
{:ok, desc} ->
if mod.prefer_expanded_description?() do
desc
else
description <> " | #{desc}"
end
_ ->
description
end
else
description
end
end
defp unknown_or_error_glyph(success?, filter_check?) when success? and filter_check?, do: ""
defp unknown_or_error_glyph(_, _), do: "?"

View file

@ -483,6 +483,7 @@ defmodule Ash.Policy.Authorizer do
scenarios: Map.get(state, :scenarios),
facts: Map.get(state, :facts),
policies: Map.get(state, :policies),
subject: Map.get(state, :subject),
resource: Map.get(state, :resource),
action: Map.get(state, :action),
actor: Map.get(state, :actor),
@ -497,6 +498,7 @@ defmodule Ash.Policy.Authorizer do
scenarios: Map.get(state, :scenarios),
facts: Map.get(state, :facts),
domain: Map.get(state, :domain),
subject: Map.get(state, :subject),
policies: Map.get(state, :policies),
resource: Map.get(state, :resource),
action: Map.get(state, :action),
@ -510,6 +512,7 @@ defmodule Ash.Policy.Authorizer do
scenarios: Map.get(state, :scenarios),
domain: Map.get(state, :domain),
facts: Map.get(state, :facts),
subject: Map.get(state, :subject),
policies: Map.get(state, :policies),
resource: Map.get(state, :resource),
action: Map.get(state, :action),
@ -621,16 +624,20 @@ defmodule Ash.Policy.Authorizer do
|> strict_check_result()
|> case do
{:authorized, authorizer} ->
authorizer = strict_check_all_facts(authorizer)
log_successful_policy_breakdown(authorizer)
{:authorized, strict_check_all_facts(authorizer)}
{:authorized, authorizer}
{:filter, authorizer, filter} ->
authorizer = strict_check_all_facts(authorizer)
log_successful_policy_breakdown(authorizer, filter)
{:filter, strict_check_all_facts(authorizer), filter}
{:filter, authorizer, filter}
{:filter_and_continue, filter, authorizer} ->
authorizer = strict_check_all_facts(authorizer)
log_successful_policy_breakdown(authorizer, filter)
{:filter, strict_check_all_facts(authorizer), filter}
{:filter, authorizer, filter}
{:error, error} ->
{:error, error}
@ -1122,7 +1129,8 @@ defmodule Ash.Policy.Authorizer do
),
do: authorizer
defp strict_check_all_facts(authorizer) do
@doc false
def strict_check_all_facts(authorizer) do
case Checker.strict_check_all_facts(authorizer) do
{:ok, authorizer, new_facts} ->
%{authorizer | facts: new_facts}
@ -1512,10 +1520,11 @@ defmodule Ash.Policy.Authorizer do
facts: authorizer.facts,
domain: Map.get(authorizer, :domain),
policies: authorizer.policies,
subject: authorizer.subject,
context_description: opts[:context_description],
for_fields: opts[:for_fields],
resource: Map.get(authorizer, :resource),
actor: Map.get(authorizer, :action),
actor: Map.get(authorizer, :actor),
action: Map.get(authorizer, :action),
scenarios: []
)}
@ -1536,6 +1545,7 @@ defmodule Ash.Policy.Authorizer do
facts: authorizer.facts,
domain: Map.get(authorizer, :domain),
policies: authorizer.policies,
subject: authorizer.subject,
context_description: opts[:context_description],
for_fields: opts[:for_fields],
resource: Map.get(authorizer, :resource),

View file

@ -48,6 +48,16 @@ defmodule Ash.Policy.Check do
@doc "Describe the check in human readable format, given the options"
@callback describe(options()) :: String.t()
@doc "Expands the description of the check, given the actor and subject"
@callback expand_description(
actor(),
authorizer(),
options()
) :: {:ok, String.t()} | :none
@doc "Whether or not the expanded description should replace the basic description in breakdowns"
@callback prefer_expanded_description?() :: boolean()
@doc "Whether or not your check requires the original data of a changeset (if applicable)"
@callback requires_original_data?(actor(), options()) :: boolean()
@ -59,7 +69,7 @@ defmodule Ash.Policy.Check do
`:simple` checks can use `Ash.Policy.SimpleCheck` for simplicity
"""
@callback type() :: check_type()
@optional_callbacks check: 4, auto_filter: 3
@optional_callbacks check: 4, auto_filter: 3, expand_description: 3
def defines_check?(module) do
:erlang.function_exported(module, :check, 4)
@ -78,8 +88,9 @@ defmodule Ash.Policy.Check do
def type, do: :manual
def requires_original_data?(_, _), do: false
def prefer_expanded_description?, do: false
defoverridable type: 0, requires_original_data?: 2
defoverridable type: 0, requires_original_data?: 2, prefer_expanded_description?: 0
end
end
end

View file

@ -11,4 +11,7 @@ defmodule Ash.Policy.Check.Expression do
def filter(_, _, opts) do
opts[:expr]
end
@impl true
def prefer_expanded_description?, do: true
end

View file

@ -27,8 +27,11 @@ defmodule Ash.Policy.Checker do
authorizer,
facts
) do
{:ok, authorizer, facts} -> {:cont, {:ok, authorizer, facts}}
{:error, error} -> {:halt, {:error, error}}
{:ok, authorizer, facts} ->
{:cont, {:ok, authorizer, facts}}
{:error, error} ->
{:halt, {:error, error}}
end
end)
|> case do

View file

@ -343,7 +343,31 @@ defmodule Ash.Policy.FilterCheck do
end
end
defoverridable reject: 3, requires_original_data?: 2
def expand_description(actor, authorizer, opts) do
changeset =
case authorizer.subject do
%Ash.Changeset{} = changeset -> changeset
_ -> nil
end
{:ok,
actor
|> filter(authorizer, opts)
|> Ash.Expr.fill_template(
actor,
authorizer.subject.arguments,
authorizer.subject.context,
changeset
)
|> inspect()}
end
def prefer_expanded_description?, do: false
defoverridable reject: 3,
requires_original_data?: 2,
expand_description: 3,
prefer_expanded_description?: 0
end
end

View file

@ -29,7 +29,9 @@ defmodule Ash.Policy.SimpleCheck do
end
end
defoverridable requires_original_data?: 2
def prefer_expanded_description?, do: false
defoverridable requires_original_data?: 2, prefer_expanded_description?: 0
end
end
end

View file

@ -74,6 +74,84 @@ defmodule Ash.Test.Policy.SimpleTest do
end
end
defmodule ResourceWithAnImpossibleCreatePolicy do
use Ash.Resource,
domain: Ash.Test.Domain,
data_layer: Ash.DataLayer.Ets,
authorizers: [Ash.Policy.Authorizer]
ets do
private? true
end
attributes do
uuid_primary_key :id
end
actions do
defaults [:create, :read]
end
policies do
policy action(:create) do
authorize_if expr(self.id == ^actor(:id))
end
end
relationships do
belongs_to :self, ResourceWithAnImpossibleCreatePolicy do
filterable? true
source_attribute :id
destination_attribute :id
end
end
end
defmodule OldEnoughToDrink do
use Ash.Policy.FilterCheck
@impl true
def describe(_opts) do
"is old enough to drink"
end
@impl true
def filter(_, _, _opts) do
expr(age > 21)
end
end
defmodule ResourceWithFailedFilterTest do
use Ash.Resource,
domain: Ash.Test.Domain,
data_layer: Ash.DataLayer.Ets,
authorizers: [Ash.Policy.Authorizer]
ets do
private? true
end
attributes do
uuid_primary_key :id
attribute :age, :integer
end
actions do
defaults [:create, :read, :update]
end
policies do
policy action(:create) do
authorize_if always()
end
policy action(:update) do
authorize_if OldEnoughToDrink
authorize_if expr(id == ^actor(:id))
end
end
end
setup do
old_env = Application.get_env(:ash, :policies, [])
@ -112,6 +190,31 @@ defmodule Ash.Test.Policy.SimpleTest do
end
end
test "an impossible create policy shows the correct error message" do
assert_raise Ash.Error.Forbidden, ~r/Cannot use a filter to authorize a create/, fn ->
ResourceWithAnImpossibleCreatePolicy
|> Ash.create!(%{}, actor: %{id: 10})
end
end
test "a filter check shows a more in-depth breakdown of filter checks" do
actor_id = Ash.UUID.generate()
exception =
assert_raise Ash.Error.Forbidden, fn ->
ResourceWithFailedFilterTest
|> Ash.create!(%{}, actor: %{id: actor_id})
|> Ash.Changeset.for_update(:update, %{})
|> Ash.update!(%{}, actor: %{id: actor_id})
end
message = Exception.message(exception)
assert message =~ "Actor: %{id: \"#{actor_id}\"}"
assert message =~ "authorize if: is old enough to drink | age > 21 | ? | 🔎"
assert message =~ "authorize if: id == \"#{actor_id}\" | ? | 🔎"
end
test "strict read policies do not result in a filter" do
thing =
ResourceWithAStrictReadPolicy