From dd26beb79ba49edbf08549cab868f34022236866 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Thu, 28 Jan 2021 14:47:59 -0500 Subject: [PATCH] chore: add more authorization tests chore: improve authorization test helper improvement: support `{:filter, _}` authorization results for changesets --- lib/ash/engine/request.ex | 96 ++++++++++++++++++++++------- lib/ash/filter/runtime.ex | 23 +++++++ lib/ash/options_helpers.ex | 2 +- mix.exs | 11 +++- test/actions/create_test.exs | 2 + test/actions/destroy_test.exs | 2 + test/actions/side_load_test.exs | 10 ++- test/actions/update_test.exs | 2 + test/authorizer/authorizer_test.exs | 86 ++++++++++++++++++++++++++ test/support/authorizer.ex | 45 +++++++++----- 10 files changed, 234 insertions(+), 45 deletions(-) create mode 100644 test/authorizer/authorizer_test.exs diff --git a/lib/ash/engine/request.ex b/lib/ash/engine/request.ex index 6a289e46..1f87c77c 100644 --- a/lib/ash/engine/request.ex +++ b/lib/ash/engine/request.ex @@ -507,31 +507,15 @@ defmodule Ash.Engine.Request do {:ok, set_authorizer_state(request, authorizer, :authorized)} {:filter, filter} -> - request - |> Map.update!(:query, &Ash.Query.filter(&1, ^filter)) - |> Map.update( - :authorization_filter, - filter, - &add_to_or_parse(&1, filter, request.resource) - ) - |> set_authorizer_state(authorizer, :authorized) - |> try_resolve([request.path ++ [:query]], false) + apply_filter(request, authorizer, filter, true) {:filter_and_continue, _, _} when strict_check_only? -> {:error, MustPassStrictCheck.exception(resource: request.resource)} {:filter_and_continue, filter, new_authorizer_state} -> - new_request = - request - |> Map.update!(:query, &Ash.Query.filter(&1, ^filter)) - |> Map.update( - :authorization_filter, - filter, - &add_to_or_parse(&1, filter, request.resource) - ) - |> set_authorizer_state(authorizer, new_authorizer_state) - - {:ok, new_request} + request + |> set_authorizer_state(authorizer, new_authorizer_state) + |> apply_filter(authorizer, filter) {:continue, _} when strict_check_only? -> {:error, MustPassStrictCheck.exception(resource: request.resource)} @@ -562,6 +546,42 @@ defmodule Ash.Engine.Request do end end + defp apply_filter(request, authorizer, filter, resolve_data? \\ false) + + defp apply_filter(%{action: %{type: :read}} = request, authorizer, filter, resolve_data?) do + request = + request + |> Map.update!(:query, &Ash.Query.filter(&1, ^filter)) + |> Map.update( + :authorization_filter, + filter, + &add_to_or_parse(&1, filter, request.resource) + ) + |> set_authorizer_state(authorizer, :authorized) + + if resolve_data? do + try_resolve(request, [request.path ++ [:query]], false) + else + {:ok, request} + end + end + + defp apply_filter(request, authorizer, filter, resolve_data?) do + case do_runtime_filter(request, filter) do + {:ok, request} -> + request = set_authorizer_state(request, authorizer, :authorized) + + if resolve_data? do + try_resolve(request, [request.path ++ [:query]], false) + else + {:ok, request} + end + + {:error, error} -> + {:error, error} + end + end + defp add_to_or_parse(existing_authorization_filter, filter, resource) do if existing_authorization_filter do Ash.Filter.add_to_filter(existing_authorization_filter, filter) @@ -652,10 +672,11 @@ defmodule Ash.Engine.Request do end end - defp do_runtime_filter(%{data: empty} = request, _filter) when empty in [nil, []], - do: {:ok, request} + defp do_runtime_filter(%{action: %{type: :read}, data: empty} = request, _filter) + when empty in [nil, []], + do: {:ok, request} - defp do_runtime_filter(request, filter) do + defp do_runtime_filter(%{action: %{type: :read}} = request, filter) do pkey = Ash.Resource.primary_key(request.resource) pkeys = @@ -692,6 +713,34 @@ defmodule Ash.Engine.Request do end end + defp do_runtime_filter(request, filter) do + pkey = Ash.Resource.primary_key(request.resource) + + pkey = + request.changeset.data + |> Map.take(pkey) + |> Map.to_list() + + new_query = + request.resource + |> Ash.Query.filter(^pkey) + |> Ash.Query.filter(^filter) + |> Ash.Query.limit(1) + + new_query + |> Ash.Actions.Read.unpaginated_read() + |> case do + {:ok, []} -> + {:error, Ash.Error.Forbidden.exception([])} + + {:ok, [_]} -> + {:ok, request} + + {:error, error} -> + {:error, error} + end + end + defp try_resolve(request, deps, internal?) do Enum.reduce_while(deps, {:ok, request, [], []}, fn dep, {:ok, request, notifications, skipped} -> @@ -918,6 +967,7 @@ defmodule Ash.Engine.Request do defp missing_strict_check_dependencies?(authorizer, request) do authorizer |> Authorizer.strict_check_context(authorizer_state(request, authorizer)) + |> List.wrap() |> Enum.filter(fn dependency -> match?(%UnresolvedField{}, Map.get(request, dependency)) end) diff --git a/lib/ash/filter/runtime.ex b/lib/ash/filter/runtime.ex index 6ee5048d..e8b00c0a 100644 --- a/lib/ash/filter/runtime.ex +++ b/lib/ash/filter/runtime.ex @@ -155,6 +155,9 @@ defmodule Ash.Filter.Runtime do {:error, error} -> {:error, error} + :unknown -> + :unknown + _ -> {:ok, false} end @@ -167,12 +170,18 @@ defmodule Ash.Filter.Runtime do {:error, error} -> {:error, error} + :unknown -> + :unknown + _ -> {:ok, false} end %Not{expression: expression} -> case do_match(record, expression) do + :unknown -> + :unknown + {:ok, match?} -> {:ok, !match?} @@ -191,9 +200,11 @@ defmodule Ash.Filter.Runtime do case resolve_expr(expr, record) do {:ok, resolved} -> {:cont, {:ok, [resolved | exprs]}} {:error, error} -> {:halt, {:error, error}} + :unknown -> {:halt, :unknown} end end) |> case do + :unknown -> :unknown {:ok, resolved} -> {:ok, Enum.reverse(resolved)} {:error, error} -> {:error, error} end @@ -225,6 +236,9 @@ defmodule Ash.Filter.Runtime do {:error, error} -> {:error, error} + :unknown -> + :unknown + _ -> {:ok, false} end @@ -238,6 +252,9 @@ defmodule Ash.Filter.Runtime do {:error, error} -> {:error, error} + :unknown -> + :unknown + _ -> {:ok, false} end @@ -281,6 +298,9 @@ defmodule Ash.Filter.Runtime do {:ok, false} -> {:ok, false} + + :unknown -> + :unknown end end @@ -291,6 +311,9 @@ defmodule Ash.Filter.Runtime do {:ok, false} -> do_match(record, right) + + :unknown -> + :unknown end end diff --git a/lib/ash/options_helpers.ex b/lib/ash/options_helpers.ex index 22dbfb86..d24a0f38 100644 --- a/lib/ash/options_helpers.ex +++ b/lib/ash/options_helpers.ex @@ -24,7 +24,7 @@ defmodule Ash.OptionsHelpers do |> sanitize_schema() |> Enum.map(fn {key, opts} -> if opts[:doc] do - {key, Keyword.update!(opts, :doc, &String.replace(&1, "\n", "\n "))} + {key, Keyword.update!(opts, :doc, &String.replace(&1, "\n\n", " \n"))} else {key, opts} end diff --git a/mix.exs b/mix.exs index 2f21dcd4..1d40b03d 100644 --- a/mix.exs +++ b/mix.exs @@ -104,6 +104,9 @@ defmodule Ash.MixProject do Ash.Query.Calculation, Ash.Calculation ], + values: [ + Ash.CiString + ], type: ~r/Ash.Type/, data_layer: ~r/Ash.DataLayer/, authorizer: ~r/Ash.Authorizer/, @@ -123,7 +126,7 @@ defmodule Ash.MixProject do "filter operators": ~r/Ash.Query.Operator/, "filter functions": ~r/Ash.Query.Function/, "query expressions": [ - Ash.Query.BooleanBooleanExpression, + Ash.Query.BooleanExpression, Ash.Query.Not, Ash.Query.Ref, Ash.Query.Call @@ -137,8 +140,10 @@ defmodule Ash.MixProject do miscellaneous: [ Ash.NotLoaded, Ash.Error.Stacktrace, - Ash.Query.Aggregate - ] + Ash.Query.Aggregate, + Ash.Query.Type + ], + comparable: ~r/Comparable/ ] ] end diff --git a/test/actions/create_test.exs b/test/actions/create_test.exs index 17dfd0d3..930e6d5f 100644 --- a/test/actions/create_test.exs +++ b/test/actions/create_test.exs @@ -565,6 +565,8 @@ defmodule Ash.Test.Actions.CreateTest do describe "unauthorized create" do test "it does not create the record" do + start_supervised({Ash.Test.Authorizer, check: :forbidden, strict_check: :continue}) + assert_raise(Ash.Error.Forbidden, fn -> Authorized |> new() diff --git a/test/actions/destroy_test.exs b/test/actions/destroy_test.exs index 0915ca83..56238a5a 100644 --- a/test/actions/destroy_test.exs +++ b/test/actions/destroy_test.exs @@ -120,6 +120,8 @@ defmodule Ash.Test.Actions.DestroyTest do |> new(%{name: "foobar"}) |> Api.create!() + start_supervised({Ash.Test.Authorizer, strict_check: :continue, check: :forbidden}) + assert_raise(Ash.Error.Forbidden, fn -> Api.destroy!(author, authorize?: true) end) diff --git a/test/actions/side_load_test.exs b/test/actions/side_load_test.exs index c50142bc..2b09cdef 100644 --- a/test/actions/side_load_test.exs +++ b/test/actions/side_load_test.exs @@ -1,6 +1,6 @@ defmodule Ash.Test.Actions.SideLoadTest do @moduledoc false - use ExUnit.Case, async: true + use ExUnit.Case, async: false require Ash.Query @@ -120,8 +120,12 @@ defmodule Ash.Test.Actions.SideLoadTest do end setup do - Process.put(:authorize?, true) - Process.put(:strict_check_context, [:query]) + start_supervised( + {Ash.Test.Authorizer, + strict_check: :authorized, + check: {:error, Ash.Error.Forbidden.exception([])}, + strict_check_context: [:query]} + ) :ok end diff --git a/test/actions/update_test.exs b/test/actions/update_test.exs index c8b8d4a7..ea5faa02 100644 --- a/test/actions/update_test.exs +++ b/test/actions/update_test.exs @@ -569,6 +569,8 @@ defmodule Ash.Test.Actions.UpdateTest do |> new(%{name: "bar"}) |> Api.create!() + start_supervised({Ash.Test.Authorizer, check: :forbidden, strict_check: :continue}) + assert_raise(Ash.Error.Forbidden, fn -> record |> new(%{name: "foo"}) diff --git a/test/authorizer/authorizer_test.exs b/test/authorizer/authorizer_test.exs new file mode 100644 index 00000000..d297e778 --- /dev/null +++ b/test/authorizer/authorizer_test.exs @@ -0,0 +1,86 @@ +defmodule Ash.Test.Changeset.AuthorizerTest do + @moduledoc false + use ExUnit.Case, async: false + + require Ash.Query + + defmodule Post do + use Ash.Resource, + data_layer: Ash.DataLayer.Ets, + authorizers: [ + Ash.Test.Authorizer + ] + + ets do + private? true + end + + attributes do + uuid_primary_key :id + + attribute :title, :string, allow_nil?: false + end + end + + defmodule Api do + use Ash.Api + + resources do + resource Post + end + end + + describe "strict check can filter results" do + test "a simple filter is applied" do + start_supervised( + {Ash.Test.Authorizer, + strict_check: {:filter, [title: "foo"]}, strict_check_context: [:query]} + ) + + Post + |> Ash.Changeset.for_create(:create, %{title: "test"}) + |> Api.create!() + + Post + |> Ash.Changeset.for_create(:create, %{title: "foo"}) + |> Api.create!() + + assert [%Post{title: "foo"}] = Api.read!(Post, authorize?: true) + end + + test "a simple filter can also be applied to changesets" do + start_supervised( + {Ash.Test.Authorizer, + strict_check: {:filter, [title: "foo"]}, strict_check_context: [:query, :changeset]} + ) + + # Filter always fails on creates + assert_raise Ash.Error.Forbidden, fn -> + Post + |> Ash.Changeset.for_create(:create, %{title: "test"}) + |> Api.create!(authorize?: true) + end + + good_post = + Post + |> Ash.Changeset.for_create(:create, %{title: "foo"}) + |> Api.create!() + + bad_post = + Post + |> Ash.Changeset.for_create(:create, %{title: "test"}) + |> Api.create!() + + # Filters apply to the base data + assert_raise Ash.Error.Forbidden, fn -> + bad_post + |> Ash.Changeset.for_update(:update, %{title: "next"}) + |> Api.update!(authorize?: true) + end + + good_post + |> Ash.Changeset.for_update(:update, %{title: "next"}) + |> Api.update!(authorize?: true) + end + end +end diff --git a/test/support/authorizer.ex b/test/support/authorizer.ex index 9850a20d..fa2c931f 100644 --- a/test/support/authorizer.ex +++ b/test/support/authorizer.ex @@ -5,26 +5,41 @@ defmodule Ash.Test.Authorizer do """ @behaviour Ash.Authorizer - alias Ash.Error.Forbidden + use Agent + + def start_link(opts) do + Agent.start_link( + fn -> + %{ + strict_check_result: maybe_forbidden(opts[:strict_check]), + check_result: maybe_forbidden(opts[:check]), + strict_check_context: opts[:strict_check_context] + } + end, + name: __MODULE__ + ) + end + + defp maybe_forbidden(:forbidden), do: {:error, Ash.Error.Forbidden.exception([])} + defp maybe_forbidden(other), do: other def initial_state(_, _, _, _), do: %{} - def strict_check_context(_), do: Process.get(:strict_check_context, []) + def strict_check_context(_), do: get(:strict_check_context, []) - def strict_check(_, _) do - if Process.get(:authorize?, false) do - :authorized - else - {:error, Forbidden.exception([])} - end - end + def strict_check(state, _), + do: get(:strict_check_result, :authorized) |> continue(state) def check_context(_), do: [] - def check(_, _) do - if Process.get(:authorize_check?, false) do - :authorized - else - {:error, :forbidden} - end + def check(state, _), do: get(:check_result, :authorized) |> continue(state) + + defp continue(:continue, state), do: {:continue, state} + defp continue(other, _), do: other + + defp get(key, default) do + Agent.get(__MODULE__, &Map.get(&1, key)) || default + catch + :exit, _ -> + default end end