diff --git a/documentation/topics/security/policies.md b/documentation/topics/security/policies.md index 34c8c94f..cc52c329 100644 --- a/documentation/topics/security/policies.md +++ b/documentation/topics/security/policies.md @@ -172,6 +172,41 @@ Policy groups can _not_ contain bypass policies. The purpose of policy groups is about the behavior of policies. When you see a policy group, you know that no policies inside that group will interact with policies in other policy groups, unless they also apply. +### Access Type + +Policies have an "access type" that determines when they are applied. By default, `access_type` is `:filter`. +When applied to a read action, `:filter` will result in a filtered read. For other action types, the filter will be evaluated +to determine if a forbidden error should be raised. + +There are three access types, and they determine the _latest point in the process_ that any check contained by a policy can be applied. + +- `strict` - All checks must be applied statically. These result in a forbidden error if they are not met. +- `filter` - All checks must be applied either statically or as a filter. These result in a filtered read if they are not met, and a + forbidden error for other action types. +- `runtime` - This allows checks to be run _after_ the data has been read. It is exceedingly rare that you would need to use this access type. + +For example, given this policy: + +```elixir +policy action(:read_hidden) do + authorize_if expr(actor.is_admin == true) +end +``` + +A non-admin using the `:read_hidden` action would see an empty list of records, rather than a forbidden error. + +However, with this policy + +```elixir +policy action(:read_hidden) do + access_type :strict + + authorize_if expr(actor.is_admin == true) +end +``` + +A non-admin using the `:read_hidden` action would see a forbidden error. + ## Checks Checks evaluate from top to bottom within a policy. A check can produce one of three results, the same that a policy can produce. While checks are not necessarily evaluated in order, they _logically apply_ in that order, so you may as well think of it in that way. It can be thought of as a step-through algorithm. diff --git a/lib/ash/policy/authorizer/authorizer.ex b/lib/ash/policy/authorizer/authorizer.ex index c063778c..803f004e 100644 --- a/lib/ash/policy/authorizer/authorizer.ex +++ b/lib/ash/policy/authorizer/authorizer.ex @@ -1530,18 +1530,22 @@ defmodule Ash.Policy.Authorizer do end {:error, authorizer, :unsatisfiable} -> - {:error, - Ash.Error.Forbidden.Policy.exception( - facts: authorizer.facts, - domain: Map.get(authorizer, :domain), - 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: [] - )} + if forbidden_due_to_strict_policy?(authorizer) do + {:error, + Ash.Error.Forbidden.Policy.exception( + facts: authorizer.facts, + domain: Map.get(authorizer, :domain), + 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: [] + )} + else + {:filter, authorizer, false} + end {:error, _authorizer, exception} -> {:error, Ash.Error.to_ash_error(exception)} @@ -1552,6 +1556,60 @@ defmodule Ash.Policy.Authorizer do strict_filter(%{authorizer | scenarios: scenarios}) end + defp forbidden_due_to_strict_policy?(authorizer) do + if authorizer.for_fields || authorizer.action.type != :read do + true + else + authorizer.policies + |> Enum.any?(fn policy -> + policy.access_type == :strict and + Enum.all?(policy.condition || [], fn {check_module, check_opts} -> + Policy.fetch_fact(authorizer.facts, {check_module, check_opts}) == {:ok, true} + end) and + policy_fails_statically?(authorizer, policy) + end) + end + end + + defp policy_fails_statically?(authorizer, policy) do + Enum.reduce_while(policy.policies, :forbidden, fn check, status -> + case check.type do + :authorize_if -> + if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) == + {:ok, true} do + {:halt, :authorized} + else + {:cont, status} + end + + :forbid_if -> + if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) == + {:ok, true} do + {:halt, :forbidden} + else + {:cont, status} + end + + :authorize_unless -> + if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) == + {:ok, true} do + {:cont, status} + else + {:halt, :authorized} + end + + :forbid_unless -> + if Policy.fetch_fact(authorizer.facts, {check.check_module, check.check_opts}) == + {:ok, true} do + {:cont, status} + else + {:halt, :forbidden} + end + end + end) + |> Kernel.==(:forbidden) + end + defp get_policies(authorizer) do %{ authorizer diff --git a/lib/ash/policy/check/relating_to_actor.ex b/lib/ash/policy/check/relating_to_actor.ex index a82d7dc3..981a9f18 100644 --- a/lib/ash/policy/check/relating_to_actor.ex +++ b/lib/ash/policy/check/relating_to_actor.ex @@ -68,6 +68,8 @@ defmodule Ash.Policy.Check.RelatingToActor do end end + def filter(_, _, _), do: false + defp take_keys(input, primary_key) do Enum.map(primary_key, fn key -> Map.get(input, key) || Map.get(input, to_string(key)) diff --git a/test/policy/complex_test.exs b/test/policy/complex_test.exs index 6cdb7ef5..7879c6a3 100644 --- a/test/policy/complex_test.exs +++ b/test/policy/complex_test.exs @@ -187,8 +187,6 @@ 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..5de2f6e4 100644 --- a/test/policy/selecting_test.exs +++ b/test/policy/selecting_test.exs @@ -144,7 +144,7 @@ defmodule Ash.Test.Policy.SelectingTest do |> Ash.Changeset.for_create(:create) |> Ash.create!(authorize?: false) - assert {:error, %Ash.Error.Forbidden{}} = + assert {:ok, %{owner_only_resource: nil}} = Parent |> Ash.Query.for_read(:read) |> Ash.Query.load(:owner_only_resource) diff --git a/test/policy/simple_test.exs b/test/policy/simple_test.exs index 6f821769..7548f1a4 100644 --- a/test/policy/simple_test.exs +++ b/test/policy/simple_test.exs @@ -49,6 +49,31 @@ defmodule Ash.Test.Policy.SimpleTest do end end + defmodule ResourceWithAStrictReadPolicy do + use Ash.Resource, + domain: Ash.Test.Domain, + authorizers: [Ash.Policy.Authorizer] + + attributes do + uuid_primary_key :id + end + + actions do + defaults [:create, :read] + end + + policies do + policy action_type(:read) do + access_type :strict + authorize_if actor_attribute_equals(:admin, true) + end + + policy action_type(:read) do + authorize_if expr(id == ^actor(:id)) + end + end + end + setup do Application.put_env(:ash, :policies, show_policy_breakdowns?: true) @@ -81,6 +106,27 @@ defmodule Ash.Test.Policy.SimpleTest do end end + test "strict read policies do not result in a filter" do + thing = + ResourceWithAStrictReadPolicy + |> Ash.create!(authorize?: false) + + actor = %{id: thing, admin: false} + + assert_raise Ash.Error.Forbidden, fn -> + ResourceWithAStrictReadPolicy + |> Ash.Query.new() + |> Ash.DataLayer.Simple.set_data([thing]) + |> Ash.read!(actor: actor) + end + + assert [] = + ResourceWithAStrictReadPolicy + |> Ash.Query.new() + |> Ash.DataLayer.Simple.set_data([%{thing | id: Ash.UUID.generate()}]) + |> Ash.read!(actor: %{admin: true}) + 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) diff --git a/test/support/policy_complex/domain.ex b/test/support/policy_complex/domain.ex index 39f7d892..4ca084a5 100644 --- a/test/support/policy_complex/domain.ex +++ b/test/support/policy_complex/domain.ex @@ -13,6 +13,7 @@ defmodule Ash.Test.Support.PolicyComplex.Domain do policies do policy always() do + access_type :strict authorize_unless actor_attribute_equals(:forbidden_by_domain, true) end end