From 37755a870b1efeb12f991b4a68667ab2067c8f5a Mon Sep 17 00:00:00 2001 From: Tore Pettersen Date: Mon, 15 Jul 2024 14:16:52 +0200 Subject: [PATCH] feat: Allow field policies to hide private fields (#1289) * Allow field policies to hide private fields * Create option for how to handle private fields * Improve docs --- .formatter.exs | 1 + documentation/topics/security/policies.md | 24 +++++++ lib/ash/changeset/changeset.ex | 8 ++- lib/ash/policy/authorizer/authorizer.ex | 24 ++++++- .../add_missing_field_policies.ex | 37 ++++++----- lib/ash/policy/info.ex | 4 ++ test/policy/field_policy_test.exs | 66 ++++++++++++++++++- test/support/policy_field/resources/post.ex | 63 ++++++++++++++++++ test/support/policy_field/resources/ticket.ex | 6 ++ test/support/policy_field/resources/user.ex | 6 ++ 10 files changed, 219 insertions(+), 20 deletions(-) create mode 100644 test/support/policy_field/resources/post.ex diff --git a/.formatter.exs b/.formatter.exs index 6df3cc80..126a8188 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -163,6 +163,7 @@ spark_locals_without_parens = [ primary?: 1, primary_key?: 1, private?: 1, + private_fields: 1, public?: 1, publish: 2, publish: 3, diff --git a/documentation/topics/security/policies.md b/documentation/topics/security/policies.md index 777a7b68..1302935f 100644 --- a/documentation/topics/security/policies.md +++ b/documentation/topics/security/policies.md @@ -359,6 +359,30 @@ In results, forbidden fields will be replaced with a special value: `%Ash.Forbid When these fields are referred to in filters, they will be replaced with an expression that evaluates to `nil`. To support this behavior, only simple and filter checks are allowed in field policies. +### Handeling private fields in internal functions + +When calling internal functions like `Ash.read!/1`, private fields will by default always be shown. +Even if field policies applies to the resource. You can change the default behaviour by setting the +`private_fields` option on field policies. + +```elixir +field_policies do + private_fields :include +end +``` + +The different options are: +- `:show` will always show private fields +- `:hide` will always hide private fields +- `:include` will let you to write field policies for private fields and private fields + will be shown or hidden depending on the outcome of the policy + +If you want to overwrite the default option that is `:show`, you can do that by setting a global flag: + +```elixir +config :ash, :policies, private_fields: :include +``` + ## Debugging and Logging ### Policy Breakdowns diff --git a/lib/ash/changeset/changeset.ex b/lib/ash/changeset/changeset.ex index a8885a80..2525b98a 100644 --- a/lib/ash/changeset/changeset.ex +++ b/lib/ash/changeset/changeset.ex @@ -538,12 +538,16 @@ defmodule Ash.Changeset do Provide a list of field types to narrow down the returned results. """ - def accessing(changeset, types \\ [:attributes, :relationships, :calculations, :attributes]) do + def accessing( + changeset, + types \\ [:attributes, :relationships, :calculations, :attributes], + only_public? \\ true + ) do changeset.resource |> Ash.Query.new() |> Ash.Query.load(changeset.load) |> Map.put(:select, changeset.select) - |> Ash.Query.accessing(types) + |> Ash.Query.accessing(types, only_public?) end @spec fully_atomic_changeset( diff --git a/lib/ash/policy/authorizer/authorizer.ex b/lib/ash/policy/authorizer/authorizer.ex index 493823fe..b63c8389 100644 --- a/lib/ash/policy/authorizer/authorizer.ex +++ b/lib/ash/policy/authorizer/authorizer.ex @@ -356,6 +356,15 @@ defmodule Ash.Policy.Authorizer do entities: [ @field_policy_bypass, @field_policy + ], + schema: [ + private_fields: [ + type: {:one_of, [:show, :hide, :include]}, + default: Application.compile_env(:ash, :policies)[:private_fields] || :show, + doc: """ + How private fields should be handeled by field policies in internal functions. See the [Policies guide](documentation/topics/security/policies.md#field-policies) for more. + """ + ] ] } @@ -895,13 +904,24 @@ defmodule Ash.Policy.Authorizer do # and we don't need to add any calculations {:ok, query_or_changeset, authorizer} else + only_public? = + case Ash.Policy.Info.private_fields_policy(query_or_changeset.resource) do + :include -> false + :show -> true + :hide -> false + end + accessing_fields = case query_or_changeset do %Ash.Query{} = query -> - Ash.Query.accessing(query, [:attributes, :calculations, :aggregates]) + Ash.Query.accessing(query, [:attributes, :calculations, :aggregates], only_public?) %Ash.Changeset{} = changeset -> - Ash.Changeset.accessing(changeset, [:attributes, :calculations, :aggregates]) + Ash.Changeset.accessing( + changeset, + [:attributes, :calculations, :aggregates], + only_public? + ) end pkey = Ash.Resource.Info.primary_key(query_or_changeset.resource) diff --git a/lib/ash/policy/authorizer/transformers/add_missing_field_policies.ex b/lib/ash/policy/authorizer/transformers/add_missing_field_policies.ex index 4dbabbcb..01a43749 100644 --- a/lib/ash/policy/authorizer/transformers/add_missing_field_policies.ex +++ b/lib/ash/policy/authorizer/transformers/add_missing_field_policies.ex @@ -9,24 +9,31 @@ defmodule Ash.Policy.Authorizer.Transformers.AddMissingFieldPolicies do def after?(_), do: true def transform(dsl) do - non_pkey_fields = - dsl - |> Ash.Resource.Info.fields([:aggregates, :calculations, :attributes]) - |> Enum.reject(fn - %{public?: false} -> - true - - %{primary_key?: true} -> - true - - _ -> - false - end) - |> Enum.map(& &1.name) - if Enum.empty?(Ash.Policy.Info.field_policies(dsl)) do {:ok, dsl} else + exclude_private_fields? = + case Ash.Policy.Info.private_fields_policy(dsl) do + :include -> false + :show -> true + :hide -> true + end + + non_pkey_fields = + dsl + |> Ash.Resource.Info.fields([:aggregates, :calculations, :attributes]) + |> Enum.reject(fn + %{public?: false} when exclude_private_fields? -> + true + + %{primary_key?: true} -> + true + + _ -> + false + end) + |> Enum.map(& &1.name) + dsl |> replace_asterisk(non_pkey_fields) |> ensure_field_coverage(non_pkey_fields) diff --git a/lib/ash/policy/info.ex b/lib/ash/policy/info.ex index f46b6168..8d8a36cc 100644 --- a/lib/ash/policy/info.ex +++ b/lib/ash/policy/info.ex @@ -143,6 +143,10 @@ defmodule Ash.Policy.Info do |> set_access_type(default_access_type(resource)) end + def private_fields_policy(resource) do + Extension.get_opt(resource, [:field_policies], :private_fields) + end + def policies(domain, resource) do if domain do do_policies(domain) ++ do_policies(resource) diff --git a/test/policy/field_policy_test.exs b/test/policy/field_policy_test.exs index 81d09f73..d503128a 100644 --- a/test/policy/field_policy_test.exs +++ b/test/policy/field_policy_test.exs @@ -4,7 +4,7 @@ defmodule Ash.Test.Policy.FieldPolicyTest do require Ash.Query - alias Ash.Test.Support.PolicyField.{Ticket, User} + alias Ash.Test.Support.PolicyField.{Post, Ticket, User} setup do rep = @@ -44,6 +44,13 @@ defmodule Ash.Test.Policy.FieldPolicyTest do representative_id: rep.id, reporter_id: other_user.id }) + ), + post: + Ash.create!( + Ash.Changeset.for_create(Post, :create, %{ + representative_id: rep.id, + reporter_id: user.id + }) ) ] end @@ -230,6 +237,63 @@ defmodule Ash.Test.Policy.FieldPolicyTest do end end + describe "private_fields" do + test "when reading a private value and private_fields_policy is :hide, its value is not displayed", + %{ + user: user, + ticket: ticket + } do + assert %Ash.ForbiddenField{field: :top_secret, type: :attribute} == + Ticket + |> Ash.Query.select(:top_secret) + |> Ash.Query.for_read(:read, %{}, authorize?: true, actor: user) + |> Ash.Query.filter(id == ^ticket.id) + |> Ash.read_one!() + |> Map.get(:top_secret) + end + + test "when reading a private value and private_fields_policy is :show, its value is displayed", + %{ + user: user + } do + assert nil == + User + |> Ash.Query.select(:top_secret) + |> Ash.Query.for_read(:read, %{}, authorize?: true, actor: user) + |> Ash.Query.filter(id == ^user.id) + |> Ash.read_one!() + |> Map.get(:top_secret) + end + + test "when reading a private value, covered by field policy the user is not supposed to see, + it's value is not displayed", + %{ + post: post + } do + assert %Ash.ForbiddenField{field: :internal_status, type: :attribute} == + Post + |> Ash.Query.select(:internal_status) + |> Ash.Query.for_read(:read, %{}, authorize?: true) + |> Ash.Query.filter(id == ^post.id) + |> Ash.read_one!() + |> Map.get(:internal_status) + end + + test "when reading a private value, covered by field policy the user is can see, its value is displayed", + %{ + user: user, + post: post + } do + assert nil == + Post + |> Ash.Query.select(:internal_status) + |> Ash.Query.for_read(:read, %{}, authorize?: true, actor: user) + |> Ash.Query.filter(id == ^post.id) + |> Ash.read_one!() + |> Map.get(:internal_status) + end + end + describe "filters" do test "filters are replaced with the appropriate field policies", %{ representative: representative, diff --git a/test/support/policy_field/resources/post.ex b/test/support/policy_field/resources/post.ex new file mode 100644 index 00000000..e046ab10 --- /dev/null +++ b/test/support/policy_field/resources/post.ex @@ -0,0 +1,63 @@ +defmodule Ash.Test.Support.PolicyField.Post do + @moduledoc false + use Ash.Resource, + domain: Ash.Test.Support.PolicyField.Domain, + data_layer: Ash.DataLayer.Ets, + authorizers: [Ash.Policy.Authorizer] + + ets do + private? true + end + + actions do + default_accept :* + defaults [:read, :destroy, create: :*, update: :*] + end + + attributes do + uuid_primary_key :id + + attribute :internal_status, :string do + public?(false) + end + + attribute :title, :string do + public?(true) + end + + attribute :description, :string do + public?(true) + end + end + + relationships do + belongs_to :representative, Ash.Test.Support.PolicyField.User do + public?(true) + allow_nil? false + end + + belongs_to :reporter, Ash.Test.Support.PolicyField.User do + public?(true) + allow_nil? false + end + end + + policies do + policy always() do + authorize_if always() + end + end + + field_policies do + private_fields :include + + field_policy :internal_status do + authorize_if relates_to_actor_via(:representative) + authorize_if relates_to_actor_via(:reporter) + end + + field_policy :* do + authorize_if always() + end + end +end diff --git a/test/support/policy_field/resources/ticket.ex b/test/support/policy_field/resources/ticket.ex index 2952bfd9..318b26c2 100644 --- a/test/support/policy_field/resources/ticket.ex +++ b/test/support/policy_field/resources/ticket.ex @@ -17,6 +17,10 @@ defmodule Ash.Test.Support.PolicyField.Ticket do attributes do uuid_primary_key :id + attribute :top_secret, :string do + public?(false) + end + attribute :internal_status, :string do public?(true) end @@ -49,6 +53,8 @@ defmodule Ash.Test.Support.PolicyField.Ticket do end field_policies do + private_fields :hide + field_policy :status do authorize_if relates_to_actor_via(:representative) authorize_if relates_to_actor_via(:reporter) diff --git a/test/support/policy_field/resources/user.ex b/test/support/policy_field/resources/user.ex index 6c55e049..276ebed6 100644 --- a/test/support/policy_field/resources/user.ex +++ b/test/support/policy_field/resources/user.ex @@ -26,6 +26,10 @@ defmodule Ash.Test.Support.PolicyField.User do public?(true) # only you can see your own points end + + attribute :top_secret, :string do + public?(false) + end end relationships do @@ -49,6 +53,8 @@ defmodule Ash.Test.Support.PolicyField.User do end field_policies do + private_fields :show + field_policy_bypass :* do authorize_if actor_attribute_equals(:role, :admin) end