diff --git a/lib/ash/policy/authorizer/authorizer.ex b/lib/ash/policy/authorizer/authorizer.ex index 18395125..6e9fb19c 100644 --- a/lib/ash/policy/authorizer/authorizer.ex +++ b/lib/ash/policy/authorizer/authorizer.ex @@ -682,72 +682,74 @@ defmodule Ash.Policy.Authorizer do end) # primary key doesn't have policies on it, and so is nil here |> Map.drop([nil, []]) - |> Enum.reduce({query_or_changeset, authorizer}, fn {policies, fields}, - {query_or_changeset, authorizer} -> - {expr, authorizer} = - case strict_check_result( - %{ - authorizer - | policies: policies - } - |> add_query_or_changeset(query_or_changeset), - for_fields: fields, - context_description: "selecting or loading fields" - ) do - {:authorized, authorizer} -> - {true, authorizer} + |> Enum.reduce( + {query_or_changeset, authorizer}, + fn {policies, fields}, {query_or_changeset, authorizer} -> + {expr, authorizer} = + case strict_check_result( + %{ + authorizer + | policies: policies + } + |> add_query_or_changeset(query_or_changeset), + for_fields: fields, + context_description: "selecting or loading fields" + ) do + {:authorized, authorizer} -> + {true, authorizer} - {:error, _} -> - {false, authorizer} + {:error, _} -> + {false, authorizer} - {:filter, authorizer, filter} -> - {filter, authorizer} + {:filter, authorizer, filter} -> + {filter, authorizer} - {:filter_and_continue, filter, _authorizer} -> - raise """ - Was given a partial filter for a field policy for fields #{inspect(fields)}. + {:filter_and_continue, filter, _authorizer} -> + raise """ + Was given a partial filter for a field policy for fields #{inspect(fields)}. - Filter: #{inspect(filter)} + Filter: #{inspect(filter)} - Field policies must currently use only filter checks or simple checks. - """ + Field policies must currently use only filter checks or simple checks. + """ - {:continue, _} -> - raise """ - Detected necessity for a runtime check for a field policy for fields #{inspect(fields)}. + {:continue, _} -> + raise """ + Detected necessity for a runtime check for a field policy for fields #{inspect(fields)}. - Field policies must currently use only filter checks or simple checks. - """ + Field policies must currently use only filter checks or simple checks. + """ + end + + # This is a hack that we need to clean up. + # Creating this kind of expression should be its own thing that we do + # with something in the `Expr` module + + %{expression: expr} = Ash.Filter.parse!(query_or_changeset.resource, expr) + + {:ok, calculation} = + Ash.Query.Calculation.new( + {:__ash_fields_are_visible__, fields}, + Ash.Resource.Calculation.Expression, + [expr: expr], + :boolean + ) + + case query_or_changeset do + %Ash.Query{} = query -> + {Ash.Query.load( + query, + calculation + ), authorizer} + + %Ash.Changeset{} = query -> + {Ash.Changeset.load( + query, + calculation + ), authorizer} end - - # This is a hack that we need to clean up. - # Creating this kind of expression should be its own thing that we do - # with something in the `Expr` module - - %{expression: expr} = Ash.Filter.parse!(query_or_changeset.resource, expr) - - {:ok, calculation} = - Ash.Query.Calculation.new( - {:__ash_fields_are_visible__, fields}, - Ash.Resource.Calculation.Expression, - [expr: expr], - :boolean - ) - - case query_or_changeset do - %Ash.Query{} = query -> - {Ash.Query.load( - query, - calculation - ), authorizer} - - %Ash.Changeset{} = query -> - {Ash.Changeset.load( - query, - calculation - ), authorizer} end - end) + ) |> then(fn {result, authorizer} -> {:ok, result, authorizer} end) @@ -785,15 +787,13 @@ defmodule Ash.Policy.Authorizer do {filterable, require_check} = authorizer.scenarios |> Enum.split_with(fn scenario -> - scenario - |> Enum.reject(fn {{check_module, opts}, _} -> + Enum.all?(scenario, fn {{check_module, opts}, _} -> opts[:access_type] == :filter || match?( {:ok, _}, Ash.Policy.Policy.fetch_fact(authorizer.facts, {check_module, opts}) ) || check_module.type() == :filter end) - |> Enum.empty?() end) filter = strict_filters(filterable, authorizer) diff --git a/lib/ash/policy/checker.ex b/lib/ash/policy/checker.ex index 6074b508..ac08254a 100644 --- a/lib/ash/policy/checker.ex +++ b/lib/ash/policy/checker.ex @@ -66,7 +66,7 @@ defmodule Ash.Policy.Checker do scenario |> Map.drop([true, false]) |> Enum.reduce_while(:reality, fn {fact, requirement}, status -> - case Map.fetch(facts, fact) do + case Policy.fetch_fact(facts, fact) do {:ok, ^requirement} -> {:cont, status} diff --git a/lib/ash/policy/policy.ex b/lib/ash/policy/policy.ex index b10fe58a..f5d58cc8 100644 --- a/lib/ash/policy/policy.ex +++ b/lib/ash/policy/policy.ex @@ -152,8 +152,8 @@ defmodule Ash.Policy.Policy do Enum.find_value(authorizer.facts, fn {{fact_mod, fact_opts}, result} -> if check_module == fact_mod && - Keyword.delete(fact_opts, :access_type) == - Keyword.delete(opts, :access_type) do + Keyword.drop(fact_opts, [:access_type, :ash_field_policy?]) == + Keyword.drop(opts, [:access_type, :ash_field_policy?]) do {:ok, result} end @@ -198,8 +198,8 @@ defmodule Ash.Policy.Policy do Enum.find_value(facts, fn {{fact_mod, fact_opts}, result} -> if mod == fact_mod && - Keyword.delete(fact_opts, :access_type) == - Keyword.delete(opts, :access_type) do + Keyword.drop(fact_opts, [:access_type, :ash_field_policy?]) == + Keyword.drop(opts, [:access_type, :ash_field_policy?]) do {:ok, result} end @@ -275,13 +275,14 @@ defmodule Ash.Policy.Policy do condition_expression -> case compile_policy_expression(policies, authorizer) do {true, authorizer} -> - {condition_expression, authorizer} + {true, authorizer} {false, authorizer} -> {{:not, condition_expression}, authorizer} {compiled_policies, authorizer} -> - {{:and, condition_expression, compiled_policies}, authorizer} + {{:or, {:and, condition_expression, compiled_policies}, {:not, condition_expression}}, + authorizer} end end end diff --git a/lib/ash/resource/relationships/belongs_to.ex b/lib/ash/resource/relationships/belongs_to.ex index ca7b0d37..b8e3a160 100644 --- a/lib/ash/resource/relationships/belongs_to.ex +++ b/lib/ash/resource/relationships/belongs_to.ex @@ -100,7 +100,7 @@ defmodule Ash.Resource.Relationships.BelongsTo do def opt_schema, do: @opt_schema @doc false - # sobelow_skip ["DOS.StringToAtom"] + # sobelow_skip ["DOS.BinToAtom"] def transform(%{source_attribute: source_attribute, name: name} = relationship) do {:ok, %{relationship | source_attribute: source_attribute || :"#{name}_id"}} end diff --git a/lib/ash/resource/relationships/many_to_many.ex b/lib/ash/resource/relationships/many_to_many.ex index dc4cb513..f82d9203 100644 --- a/lib/ash/resource/relationships/many_to_many.ex +++ b/lib/ash/resource/relationships/many_to_many.ex @@ -87,7 +87,7 @@ defmodule Ash.Resource.Relationships.ManyToMany do def opt_schema, do: @opt_schema @doc false - # sobelow_skip ["DOS.StringToAtom"] + # sobelow_skip ["DOS.BinToAtom"] def transform(%{join_relationship: join_relationship, name: name} = relationship) do {:ok, %{relationship | join_relationship: join_relationship || :"#{name}_join_assoc"}} end diff --git a/test/policy/field_policies/expr_conditions_test.exs b/test/policy/field_policies/expr_conditions_test.exs index 83dc6bb6..606868b9 100644 --- a/test/policy/field_policies/expr_conditions_test.exs +++ b/test/policy/field_policies/expr_conditions_test.exs @@ -10,55 +10,6 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do end end - defmodule ResourceWithExprCondition do - use Ash.Resource, - data_layer: Ash.DataLayer.Ets, - authorizers: [Ash.Policy.Authorizer] - - attributes do - uuid_primary_key :id - - attribute :name, :string - end - - field_policies do - field_policy :name, [expr(name == ^actor(:name))] do - authorize_if always() - end - end - - policies do - policy always() do - authorize_if always() - end - end - - code_interface do - define_for Api - - define :create - define :read - end - - actions do - defaults [:create, :read] - end - end - - test "expr condition forbids field if it does not match" do - ResourceWithExprCondition.create!(%{name: "foo"}) - ResourceWithExprCondition.create!(%{name: "bar"}) - - assert [ - %{name: "bar"}, - %{name: %Ash.ForbiddenField{field: :name, type: :attribute}} - ] = - ResourceWithExprCondition.read!( - actor: %{name: "bar"}, - query: ResourceWithExprCondition |> Ash.Query.sort([:name]) - ) - end - defmodule ResourceWithMultiplePoliciesForOneField do use Ash.Resource, data_layer: Ash.DataLayer.Ets, @@ -68,6 +19,8 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do uuid_primary_key :id attribute :name, :string + attribute :other_name, :string + attribute :other_other_name, :string end field_policies do @@ -78,10 +31,27 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do field_policy :name, expr(name == ^actor(:name)) do authorize_if always() end + + field_policy :other_name, [actor_attribute_equals(:admin, true)] do + forbid_if always() + end + + field_policy :other_name, expr(name == ^actor(:name)) do + forbid_if always() + end + + field_policy :other_other_name, [actor_attribute_equals(:admin, true)] do + authorize_if always() + end + + field_policy :other_other_name, expr(name == ^actor(:name)) do + forbid_if always() + end end policies do policy always() do + forbid_if never() authorize_if always() end end @@ -99,80 +69,29 @@ defmodule Ash.Test.Policy.FieldPolicy.ExpressionConditionTest do end test "multiple field policies for the same field with different conditions work" do - ResourceWithMultiplePoliciesForOneField.create!(%{name: "foo"}) - ResourceWithMultiplePoliciesForOneField.create!(%{name: "baz"}) + ResourceWithMultiplePoliciesForOneField.create!(%{ + name: "foo", + other_name: "foo", + other_other_name: "foo" + }) + + ResourceWithMultiplePoliciesForOneField.create!(%{ + name: "baz", + other_name: "baz", + other_other_name: "bar" + }) assert [ - %{name: "baz"}, - %{name: "foo"} + %{ + name: "baz", + other_name: %Ash.ForbiddenField{}, + other_other_name: %Ash.ForbiddenField{} + }, + %{name: "foo", other_name: %Ash.ForbiddenField{}, other_other_name: "foo"} ] = ResourceWithMultiplePoliciesForOneField.read!( actor: %{name: "baz", admin: true}, query: ResourceWithMultiplePoliciesForOneField |> Ash.Query.sort([:name]) ) end - - defmodule ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions do - use Ash.Resource, - data_layer: Ash.DataLayer.Ets, - authorizers: [Ash.Policy.Authorizer] - - attributes do - uuid_primary_key :id - - attribute :name, :string - end - - field_policies do - field_policy :name, [actor_attribute_equals(:admin, true)] do - authorize_if always() - end - - # I saw in the generated spark_dsl_config, that the extra check options are - # set for expr inside the field policy, but not for conditions I think. - # The behaviour is different if I set them here. - field_policy :name, - {Ash.Policy.Check.Expression, - [ - expr: expr(name == ^actor(:name)), - ash_field_policy?: true, - access_type: :filter - ]} do - authorize_if always() - end - end - - policies do - policy always() do - authorize_if always() - end - end - - code_interface do - define_for Api - - define :create - define :read - end - - actions do - defaults [:create, :read] - end - end - - test "multiple field policies for the same field with different conditions work (extra check options)" do - ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions.create!(%{name: "foo"}) - ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions.create!(%{name: "baz"}) - - assert [ - %{name: "baz"}, - %{name: "foo"} - ] = - ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions.read!( - actor: %{name: "baz", admin: true}, - query: - ResourceWithMultiplePoliciesForOneFieldWithExtraCheckOptions - |> Ash.Query.sort([:name]) - ) - end end