From ffa37d0c95d157b84703a7cfef73442f389b197c Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Fri, 30 Aug 2024 19:45:28 -0400 Subject: [PATCH] improvement: make authorization failures behave consistently across reads --- lib/ash/policy/authorizer/authorizer.ex | 118 +++++++++++++------ lib/ash/policy/info.ex | 4 + lib/ash/query/query.ex | 2 +- mix.exs | 3 +- test/actions/load_test.exs | 2 + test/policy/belongs_to_test.exs | 2 +- test/policy/complex_test.exs | 12 +- test/policy/rbac_test.exs | 8 +- test/policy/selecting_test.exs | 6 +- test/policy/simple_test.exs | 19 ++- test/policy/strict_condition_test.exs | 9 +- test/support/policy_simple/resources/post.ex | 6 + 12 files changed, 125 insertions(+), 66 deletions(-) diff --git a/lib/ash/policy/authorizer/authorizer.ex b/lib/ash/policy/authorizer/authorizer.ex index 52589b00..389ae5d5 100644 --- a/lib/ash/policy/authorizer/authorizer.ex +++ b/lib/ash/policy/authorizer/authorizer.ex @@ -299,6 +299,12 @@ 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, @@ -1169,7 +1175,7 @@ defmodule Ash.Policy.Authorizer do {_filters, _require_check} -> case global_filters(authorizer) do nil -> - maybe_forbid_strict(authorizer) + maybe_forbid_strict(authorizer) |> IO.inspect() {filters, scenarios_without_global} -> with {:ok, %Ash.Filter{expression: filter}} <- @@ -1498,47 +1504,83 @@ defmodule Ash.Policy.Authorizer do end defp strict_check_result(authorizer, opts \\ []) do - case Checker.strict_check_scenarios(authorizer) do - {:ok, true, authorizer} -> - {:authorized, authorizer} + 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: [] + ) - {: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: [] - )} + {:error, error} - {:ok, scenarios, authorizer} -> - case Checker.find_real_scenarios(scenarios, authorizer.facts) do - [] -> - maybe_strict_filter(authorizer, scenarios) - - _real_scenarios -> + _ -> + case Checker.strict_check_scenarios(authorizer) do + {:ok, true, authorizer} -> {: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 bda4fa26..4de6c3df 100644 --- a/lib/ash/policy/info.ex +++ b/lib/ash/policy/info.ex @@ -188,6 +188,10 @@ 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 39e4ed67..e2f4c584 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 Ash.DataLayer.data_layer_can?(query.resource, :timeout) || is_nil(timeout) do + if is_nil(timeout) || Ash.DataLayer.data_layer_can?(query.resource, :timeout) do %{query | timeout: timeout} else add_error(query, TimeoutNotSupported.exception(resource: query.resource)) diff --git a/mix.exs b/mix.exs index 5f08f900..ff778c0b 100644 --- a/mix.exs +++ b/mix.exs @@ -336,7 +336,8 @@ defmodule Ash.MixProject do defp deps do [ # DSLs - {:spark, "~> 2.1 and >= 2.2.22"}, + # {:spark, "~> 2.1 and >= 2.2.22"}, + {:spark, path: "../spark", override: true}, # 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 d78dabe0..3f2a0184 100644 --- a/test/actions/load_test.exs +++ b/test/actions/load_test.exs @@ -792,6 +792,8 @@ 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 6b499d45..e4562c91 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.Forbidden, fn -> + assert_raise Ash.Error.NotFound, 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 6cdb7ef5..0bb0a0af 100644 --- a/test/policy/complex_test.exs +++ b/test/policy/complex_test.exs @@ -90,11 +90,8 @@ defmodule Ash.Test.Policy.ComplexTest do end test "it applies policies from the domain", %{me: me} do - assert_raise Ash.Error.Forbidden, - ~r/authorize unless: actor.forbidden_by_domain == true | ✓ |/, - fn -> - Ash.read!(Post, actor: %{me | forbidden_by_domain: true}) - end + assert [] == + Ash.read!(Post, actor: %{me | forbidden_by_domain: true}) end test "it properly limits on reads of comments", %{ @@ -187,8 +184,7 @@ defmodule Ash.Test.Policy.ComplexTest do me |> Ash.load!([:bio_text], authorize?: true, actor: me) - assert_raise Ash.Error.Forbidden, fn -> - Ash.read!(Bio, actor: me, authorize?: true) - end + assert [] == + Ash.read!(Bio, actor: me, authorize?: true) end end diff --git a/test/policy/rbac_test.exs b/test/policy/rbac_test.exs index fc443399..aee21ad4 100644 --- a/test/policy/rbac_test.exs +++ b/test/policy/rbac_test.exs @@ -53,9 +53,7 @@ defmodule Ash.Test.Policy.RbacTest do |> Ash.Query.filter(id == ^org.id) |> Ash.Query.load(files: File |> Ash.Query.select([:forbidden])) - assert_raise Ash.Error.Forbidden, fn -> - Ash.read!(query, actor: user) == [] - end + assert [%{files: []}] = Ash.read!(query, actor: user) # specify no select (everything is selected) query = @@ -63,9 +61,7 @@ defmodule Ash.Test.Policy.RbacTest do |> Ash.Query.filter(id == ^org.id) |> Ash.Query.load([:files]) - assert_raise Ash.Error.Forbidden, fn -> - Ash.read!(query, actor: user) == [] - end + assert [%{files: []}] = Ash.read!(query, actor: user) # select only an allowed field query = diff --git a/test/policy/selecting_test.exs b/test/policy/selecting_test.exs index 67a045c1..3bfbd8c0 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 querying if selecting a forbidden field on the rel" do + test "guest is forbidden from seeing a forbidden field on the rel" do parent = Parent |> Ash.Changeset.for_create(:create, %{owner_id: "owner", guest_id: "guest"}) @@ -144,11 +144,13 @@ defmodule Ash.Test.Policy.SelectingTest do |> Ash.Changeset.for_create(:create) |> Ash.create!(authorize?: false) - assert {:error, %Ash.Error.Forbidden{}} = + assert {:ok, parent} = 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 6d0a5285..d96e91ca 100644 --- a/test/policy/simple_test.exs +++ b/test/policy/simple_test.exs @@ -23,6 +23,18 @@ 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) @@ -320,10 +332,9 @@ defmodule Ash.Test.Policy.SimpleTest do |> Ash.Changeset.for_create(:create, %{user_id: user.id}) |> Ash.create!(authorize?: false) - assert_raise Ash.Error.Forbidden, fn -> - Ash.Test.Support.PolicySimple.Always - |> Ash.read!(authorize?: true, actor: user) - end + assert [] = + Ash.Test.Support.PolicySimple.Always + |> Ash.read!(authorize?: true, actor: user) 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 9764f586..30982b35 100644 --- a/test/policy/strict_condition_test.exs +++ b/test/policy/strict_condition_test.exs @@ -42,10 +42,9 @@ defmodule Ash.Test.Policy.StrictConditionTest do |> Ash.Changeset.for_create(:create, %{visible: false}, authorize?: false) |> Ash.create!() - assert_raise Ash.Error.Forbidden, fn -> - Resource - |> Ash.Query.for_read(:read, %{}, actor: %{id: "foo"}) - |> Ash.read!() - end + assert [] == + Resource + |> Ash.Query.for_read(:read, %{}, actor: %{id: "foo"}) + |> Ash.read!() end end diff --git a/test/support/policy_simple/resources/post.ex b/test/support/policy_simple/resources/post.ex index 59790bc0..7514417c 100644 --- a/test/support/policy_simple/resources/post.ex +++ b/test/support/policy_simple/resources/post.ex @@ -25,6 +25,10 @@ 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 @@ -65,6 +69,8 @@ defmodule Ash.Test.Support.PolicySimple.Post do end end end + + read :never_allowed end relationships do