ash-project / ash

A declarative, extensible framework for building Elixir applications.
https://www.ash-hq.org
MIT License
1.57k stars 208 forks source link

Policy breakdowns say that no policies apply to a request even when they do #1440

Closed sevenseacat closed 3 weeks ago

sevenseacat commented 3 weeks ago

Describe the bug

It appears to be something in here somewhere https://github.com/ash-project/ash/blob/main/lib/ash/error/forbidden/policy.ex#L165-L197

I don't know how it determines whether or not a policy is relevant, but something isn't quite right. Even when policies are applied, it's printing out a message saying that no policies are applied.

To Reproduce

I don't have a failing test because I couldn't work out how to test that an error was raised without a specific message, but in Ash.Test.Policy.SimpleTest if you define a resource like:

  defmodule ResourceWithPolicies do
    use Ash.Resource,
      domain: Ash.Test.Domain,
      data_layer: Ash.DataLayer.Simple,
      authorizers: [Ash.Policy.Authorizer]

    attributes do
      uuid_primary_key :id
      attribute :name, :string
    end

    actions do
      defaults [:create]
    end

    policies do
      policy action_type(:create) do
        authorize_if expr(name == "Test")
      end
    end
  end

And a test like this:

  test "breakdowns for actions where policies apply don't show unrelated errors" do
    Ash.create!(ResourceWithPolicies, %{})
  end

The error in the test is:

  1) test breakdowns for actions where policies apply don't show unrelated errors (Ash.Test.Policy.SimpleTest)
     test/policy/simple_test.exs:137
     ** (Ash.Error.Forbidden) Forbidden Error

     * forbidden:

     Ash.Test.Policy.SimpleTest.ResourceWithPolicies.create

     No policy conditions applied to this request.
     For safety, at least one policy must apply to all requests.

       Policy | ?:
         condition: action.type == :create
       (elixir 1.17.2) lib/process.ex:864: Process.info/2
       (ash 3.4.5) lib/ash/error/forbidden/policy.ex:9: Ash.Error.Forbidden.Policy."exception (overridable 2)"/1
       (ash 3.4.5) lib/ash/error/forbidden/policy.ex:29: Ash.Error.Forbidden.Policy.exception/1
       (ash 3.4.5) lib/ash/can.ex:772: Ash.Can.run_queries/5
       (ash 3.4.5) lib/ash/can.ex:600: Ash.Can.run_check/4
       (ash 3.4.5) lib/ash/can.ex:124: Ash.Can.can/4
       (ash 3.4.5) lib/ash.ex:1317: Ash.can/3
       (ash 3.4.5) lib/ash/actions/create/create.ex:147: Ash.Actions.Create.authorize/2
       (ash 3.4.5) lib/ash/actions/create/create.ex:111: Ash.Actions.Create.do_run/4
       (ash 3.4.5) lib/ash/actions/create/create.ex:50: Ash.Actions.Create.run/4
       (ash 3.4.5) lib/ash.ex:2136: Ash.create!/3
       test/policy/simple_test.exs:138: Ash.Test.Policy.SimpleTest."test breakdowns for actions where policies apply don't show unrelated errors"/1
       (ex_unit 1.17.2) lib/ex_unit/runner.ex:485: ExUnit.Runner.exec_test/2
       (stdlib 6.0.1) timer.erl:590: :timer.tc/2
       (ex_unit 1.17.2) lib/ex_unit/runner.ex:407: anonymous fn/6 in ExUnit.Runner.spawn_test_monitor/4
     code: Ash.create!(ResourceWithPolicies, %{})
     stacktrace:
       (elixir 1.17.2) lib/process.ex:864: Process.info/2
       (ash 3.4.5) lib/ash/error/forbidden.ex:3: Ash.Error.Forbidden.exception/1
       (ash 3.4.5) deps/splode/lib/splode.ex:211: Ash.Error.to_class/2
       (ash 3.4.5) lib/ash/error/error.ex:66: Ash.Error.to_error_class/2
       (ash 3.4.5) lib/ash/actions/create/create.ex:135: Ash.Actions.Create.do_run/4
       (ash 3.4.5) lib/ash/actions/create/create.ex:50: Ash.Actions.Create.run/4
       (ash 3.4.5) lib/ash.ex:2136: Ash.create!/3
       test/policy/simple_test.exs:138: (test)

Expected behavior

The error message shouldn't tell me that no policies are applied, because one policy was applied and it failed.

Runtime

zachdaniel commented 3 weeks ago

Fixed in 5f5cccc2ea916534d2e77e2ee8446c69bb2c34fb

The "facts" used by the authorizer were actually not loaded when building the error which would lead to many breakdowns being filled with ?, so fixing this should also just make policy breakdowns correct in general again.