diff --git a/lib/ash/policy/authorizer/authorizer.ex b/lib/ash/policy/authorizer/authorizer.ex index 389ae5d5..52589b00 100644 --- a/lib/ash/policy/authorizer/authorizer.ex +++ b/lib/ash/policy/authorizer/authorizer.ex @@ -299,12 +299,6 @@ defmodule Ash.Policy.Authorizer do Ash.Expr ], schema: [ - statically_deniable_reads: [ - type: {:one_of, [:error, :filter]}, - default: :filter, - doc: - "When a read request is denied immediately, i.e we know its forbidden without ever running a query. `:filter` will filter all records out, and `:error` will result in a forbidden error." - ], default_access_type: [ type: {:one_of, [:strict, :filter, :runtime]}, default: :filter, @@ -1175,7 +1169,7 @@ defmodule Ash.Policy.Authorizer do {_filters, _require_check} -> case global_filters(authorizer) do nil -> - maybe_forbid_strict(authorizer) |> IO.inspect() + maybe_forbid_strict(authorizer) {filters, scenarios_without_global} -> with {:ok, %Ash.Filter{expression: filter}} <- @@ -1504,83 +1498,47 @@ defmodule Ash.Policy.Authorizer do end defp strict_check_result(authorizer, opts \\ []) do - case authorizer.policies do - [] -> - error = - Ash.Error.Forbidden.Policy.exception( - facts: authorizer.facts, - policies: authorizer.policies, - context_description: opts[:context_description], - for_fields: opts[:for_fields], - resource: Map.get(authorizer, :resource), - actor: Map.get(authorizer, :action), - action: Map.get(authorizer, :action), - scenarios: [] - ) + case Checker.strict_check_scenarios(authorizer) do + {:ok, true, authorizer} -> + {:authorized, authorizer} - {:error, error} + {:ok, none, authorizer} when none in [false, []] -> + {:error, + Ash.Error.Forbidden.Policy.exception( + facts: authorizer.facts, + policies: authorizer.policies, + context_description: opts[:context_description], + for_fields: opts[:for_fields], + resource: Map.get(authorizer, :resource), + actor: Map.get(authorizer, :action), + action: Map.get(authorizer, :action), + scenarios: [] + )} - _ -> - case Checker.strict_check_scenarios(authorizer) do - {:ok, true, authorizer} -> + {:ok, scenarios, authorizer} -> + case Checker.find_real_scenarios(scenarios, authorizer.facts) do + [] -> + maybe_strict_filter(authorizer, scenarios) + + _real_scenarios -> {:authorized, authorizer} - - {:ok, none, authorizer} when none in [false, []] -> - # we construct the error to log policy breakdown - error = - Ash.Error.Forbidden.Policy.exception( - facts: authorizer.facts, - policies: authorizer.policies, - context_description: opts[:context_description], - for_fields: opts[:for_fields], - resource: Map.get(authorizer, :resource), - actor: Map.get(authorizer, :action), - action: Map.get(authorizer, :action), - scenarios: [] - ) - - if authorizer.action.type == :read and - Ash.Policy.Info.statically_deniable_reads(authorizer.resource) == - :filter do - {:filter, authorizer, false} - else - {:error, error} - end - - {:ok, scenarios, authorizer} -> - case Checker.find_real_scenarios(scenarios, authorizer.facts) do - [] -> - maybe_strict_filter(authorizer, scenarios) - - _real_scenarios -> - {:authorized, authorizer} - end - - {:error, authorizer, :unsatisfiable} -> - # we construct the error to log policy breakdown - error = - Ash.Error.Forbidden.Policy.exception( - facts: authorizer.facts, - policies: authorizer.policies, - context_description: opts[:context_description], - for_fields: opts[:for_fields], - resource: Map.get(authorizer, :resource), - actor: Map.get(authorizer, :action), - action: Map.get(authorizer, :action), - scenarios: [] - ) - - if authorizer.action.type == :read and - Ash.Policy.Info.statically_deniable_reads(authorizer.resource) == - :filter do - {:filter, authorizer, false} - else - {:error, error} - end - - {:error, _authorizer, exception} -> - {:error, Ash.Error.to_ash_error(exception)} end + + {:error, authorizer, :unsatisfiable} -> + {:error, + Ash.Error.Forbidden.Policy.exception( + facts: authorizer.facts, + policies: authorizer.policies, + context_description: opts[:context_description], + for_fields: opts[:for_fields], + resource: Map.get(authorizer, :resource), + action: Map.get(authorizer, :action), + actor: Map.get(authorizer, :action), + scenarios: [] + )} + + {:error, _authorizer, exception} -> + {:error, Ash.Error.to_ash_error(exception)} end end diff --git a/lib/ash/policy/info.ex b/lib/ash/policy/info.ex index 4de6c3df..bda4fa26 100644 --- a/lib/ash/policy/info.ex +++ b/lib/ash/policy/info.ex @@ -188,10 +188,6 @@ defmodule Ash.Policy.Info do Extension.get_opt(resource, [:policies], :default_access_type, :filter, false) end - def statically_deniable_reads(resource) do - Extension.get_opt(resource, [:policies], :statically_deniable_reads, :filter, false) - end - # This should be done at compile time defp set_access_type(policies, default) when is_list(policies) do Enum.map(policies, &set_access_type(&1, default)) diff --git a/lib/ash/query/query.ex b/lib/ash/query/query.ex index e2f4c584..39e4ed67 100644 --- a/lib/ash/query/query.ex +++ b/lib/ash/query/query.ex @@ -568,7 +568,7 @@ defmodule Ash.Query do def timeout(query, timeout) do query = new(query) - if is_nil(timeout) || Ash.DataLayer.data_layer_can?(query.resource, :timeout) do + if Ash.DataLayer.data_layer_can?(query.resource, :timeout) || is_nil(timeout) do %{query | timeout: timeout} else add_error(query, TimeoutNotSupported.exception(resource: query.resource)) diff --git a/mix.exs b/mix.exs index ff778c0b..5f08f900 100644 --- a/mix.exs +++ b/mix.exs @@ -336,8 +336,7 @@ defmodule Ash.MixProject do defp deps do [ # DSLs - # {:spark, "~> 2.1 and >= 2.2.22"}, - {:spark, path: "../spark", override: true}, + {:spark, "~> 2.1 and >= 2.2.22"}, # Ash resources are backed by ecto scheams {:ecto, "~> 3.7"}, # Used by the ETS data layer diff --git a/test/actions/load_test.exs b/test/actions/load_test.exs index 3f2a0184..d78dabe0 100644 --- a/test/actions/load_test.exs +++ b/test/actions/load_test.exs @@ -792,8 +792,6 @@ defmodule Ash.Test.Actions.LoadTest do assert %{campaign_upcase: "HELLO WORLD"} = author - Application.put_env(:foo, :bar, true) - assert %{campaign_upcase: "HELLO WORLD"} = Ash.load!(author, [:campaign_upcase], lazy?: true) end diff --git a/test/policy/belongs_to_test.exs b/test/policy/belongs_to_test.exs index e4562c91..6b499d45 100644 --- a/test/policy/belongs_to_test.exs +++ b/test/policy/belongs_to_test.exs @@ -76,7 +76,7 @@ defmodule Ash.Test.Policy.Actions.BelongsToTest do }) |> Ash.create!() - assert_raise Ash.Error.NotFound, fn -> + assert_raise Ash.Error.Forbidden, fn -> post |> Ash.Changeset.for_update(:update_with_reviewer, %{ reviewer_id: reviewer.id diff --git a/test/policy/complex_test.exs b/test/policy/complex_test.exs index 0bb0a0af..6cdb7ef5 100644 --- a/test/policy/complex_test.exs +++ b/test/policy/complex_test.exs @@ -90,8 +90,11 @@ defmodule Ash.Test.Policy.ComplexTest do end test "it applies policies from the domain", %{me: me} do - assert [] == - Ash.read!(Post, actor: %{me | forbidden_by_domain: true}) + assert_raise Ash.Error.Forbidden, + ~r/authorize unless: actor.forbidden_by_domain == true | ✓ |/, + fn -> + Ash.read!(Post, actor: %{me | forbidden_by_domain: true}) + end end test "it properly limits on reads of comments", %{ @@ -184,7 +187,8 @@ defmodule Ash.Test.Policy.ComplexTest do me |> Ash.load!([:bio_text], authorize?: true, actor: me) - assert [] == - Ash.read!(Bio, actor: me, authorize?: true) + assert_raise Ash.Error.Forbidden, fn -> + Ash.read!(Bio, actor: me, authorize?: true) + end end end diff --git a/test/policy/rbac_test.exs b/test/policy/rbac_test.exs index aee21ad4..fc443399 100644 --- a/test/policy/rbac_test.exs +++ b/test/policy/rbac_test.exs @@ -53,7 +53,9 @@ defmodule Ash.Test.Policy.RbacTest do |> Ash.Query.filter(id == ^org.id) |> Ash.Query.load(files: File |> Ash.Query.select([:forbidden])) - assert [%{files: []}] = Ash.read!(query, actor: user) + assert_raise Ash.Error.Forbidden, fn -> + Ash.read!(query, actor: user) == [] + end # specify no select (everything is selected) query = @@ -61,7 +63,9 @@ defmodule Ash.Test.Policy.RbacTest do |> Ash.Query.filter(id == ^org.id) |> Ash.Query.load([:files]) - assert [%{files: []}] = Ash.read!(query, actor: user) + assert_raise Ash.Error.Forbidden, fn -> + Ash.read!(query, actor: user) == [] + end # select only an allowed field query = diff --git a/test/policy/selecting_test.exs b/test/policy/selecting_test.exs index 3bfbd8c0..67a045c1 100644 --- a/test/policy/selecting_test.exs +++ b/test/policy/selecting_test.exs @@ -132,7 +132,7 @@ defmodule Ash.Test.Policy.SelectingTest do refute is_nil(parent.owner_only_resource) end - test "guest is forbidden from seeing a forbidden field on the rel" do + test "guest is forbidden from querying if selecting a forbidden field on the rel" do parent = Parent |> Ash.Changeset.for_create(:create, %{owner_id: "owner", guest_id: "guest"}) @@ -144,13 +144,11 @@ defmodule Ash.Test.Policy.SelectingTest do |> Ash.Changeset.for_create(:create) |> Ash.create!(authorize?: false) - assert {:ok, parent} = + assert {:error, %Ash.Error.Forbidden{}} = Parent |> Ash.Query.for_read(:read) |> Ash.Query.load(:owner_only_resource) |> Ash.Query.limit(1) |> Ash.read_one(actor: %{id: "guest"}) - - assert %Ash.ForbiddenField{} = parent.owner_only_resource end end diff --git a/test/policy/simple_test.exs b/test/policy/simple_test.exs index d96e91ca..6d0a5285 100644 --- a/test/policy/simple_test.exs +++ b/test/policy/simple_test.exs @@ -23,18 +23,6 @@ defmodule Ash.Test.Policy.SimpleTest do ] end - test "statically known policies will filter all results", %{user: user} do - Post - |> Ash.Changeset.for_create(:create, %{author: user.id, text: "aaa"}) - |> Ash.create!(authorize?: false) - - Post - |> Ash.Changeset.for_create(:create, %{author: user.id, text: "aaa"}) - |> Ash.create!(authorize?: false) - - Ash.read!(Post, action: :never_allowed, actor: user) - end - test "bypass with condition does not apply subsequent filters", %{admin: admin, user: user} do Ash.create!(Ash.Changeset.for_create(Tweet, :create), authorize?: false) @@ -332,9 +320,10 @@ defmodule Ash.Test.Policy.SimpleTest do |> Ash.Changeset.for_create(:create, %{user_id: user.id}) |> Ash.create!(authorize?: false) - assert [] = - Ash.Test.Support.PolicySimple.Always - |> Ash.read!(authorize?: true, actor: user) + assert_raise Ash.Error.Forbidden, fn -> + Ash.Test.Support.PolicySimple.Always + |> Ash.read!(authorize?: true, actor: user) + end end test "two filter condition checks combine properly" do diff --git a/test/policy/strict_condition_test.exs b/test/policy/strict_condition_test.exs index 30982b35..9764f586 100644 --- a/test/policy/strict_condition_test.exs +++ b/test/policy/strict_condition_test.exs @@ -42,9 +42,10 @@ defmodule Ash.Test.Policy.StrictConditionTest do |> Ash.Changeset.for_create(:create, %{visible: false}, authorize?: false) |> Ash.create!() - assert [] == - Resource - |> Ash.Query.for_read(:read, %{}, actor: %{id: "foo"}) - |> Ash.read!() + assert_raise Ash.Error.Forbidden, fn -> + Resource + |> Ash.Query.for_read(:read, %{}, actor: %{id: "foo"}) + |> Ash.read!() + end end end diff --git a/test/support/policy_simple/resources/post.ex b/test/support/policy_simple/resources/post.ex index 7514417c..59790bc0 100644 --- a/test/support/policy_simple/resources/post.ex +++ b/test/support/policy_simple/resources/post.ex @@ -25,10 +25,6 @@ defmodule Ash.Test.Support.PolicySimple.Post do forbid_if expr(^arg(:from_an_admin?)) authorize_if always() end - - policy action(:never_allowed) do - authorize_if false - end end ets do @@ -69,8 +65,6 @@ defmodule Ash.Test.Support.PolicySimple.Post do end end end - - read :never_allowed end relationships do