ash-project / ash_json_api

The JSON:API extension for the Ash Framework
https://hexdocs.pm/ash_json_api
MIT License
56 stars 40 forks source link

Policies-setup missing for certain queries #91

Closed ulbrich closed 7 months ago

ulbrich commented 1 year ago

Hi, I’m not sure whether this really is a bug, but for two JSON-API read-requests I get different resolution of the policies — one index and one get, but both share the same policy. I debugged along the stack trace (see below and can’t find the reason, why the policies of the same actor stay empty in the one and are populated with what I would expect in the other case.

Both share this policy:

policies do
  policy action_type(:read) do
    authorize_if(Ticker.Checks.ValidActorProvided)
  end
end

This one runs into an exception:

QUERY A: #Ash.Query<resource: Ticker.Accounts.User,
 filter: #Ash.Filter<extid == "bwb-257064">>
Request: GET /api/eventhub/users/bwb-257064
** (exit) an exception was raised:
    ** (Protocol.UndefinedError) protocol Enumerable not implemented for nil of type Atom
        (elixir 1.15.3) lib/enum.ex:1: Enumerable.impl_for!/1
        (elixir 1.15.3) lib/enum.ex:166: Enumerable.reduce/3
        (elixir 1.15.3) lib/enum.ex:4387: Enum.reduce/3
        (ash 2.14.0) lib/ash/policy/policy.ex:108: Ash.Policy.Policy.at_least_one_policy_expression/2
        (ash 2.14.0) lib/ash/policy/policy.ex:61: Ash.Policy.Policy.build_requirements_expression/2
        (ash 2.14.0) lib/ash/policy/policy.ex:26: Ash.Policy.Policy.solve/1
        (ash 2.14.0) lib/ash/policy/checker.ex:83: Ash.Policy.Checker.strict_check_scenarios/1
        (ash 2.14.0) lib/ash/policy/authorizer/authorizer.ex:1142: Ash.Policy.Authorizer.strict_check_result/2
        (ash 2.14.0) lib/ash/policy/authorizer/authorizer.ex:631: Ash.Policy.Authorizer.expression_for_field/5
        (ash 2.14.0) lib/ash/policy/authorizer/authorizer.ex:561: Ash.Policy.Authorizer.replace_refs/2
        (ash 2.14.0) lib/ash/policy/authorizer/authorizer.ex:518: Ash.Policy.Authorizer.alter_filter/3
        (ash 2.14.0) lib/ash/engine/request.ex:659: Ash.Engine.Request.alter_filter/2
        (ash 2.14.0) lib/ash/engine/request.ex:556: Ash.Engine.Request.do_strict_check/3
        (ash 2.14.0) lib/ash/engine/request.ex:524: anonymous fn/2 in Ash.Engine.Request.strict_check/2
        (elixir 1.15.3) lib/enum.ex:4830: Enumerable.List.reduce/3
        (elixir 1.15.3) lib/enum.ex:2564: Enum.reduce_while/3
        (ash 2.14.0) lib/ash/engine/request.ex:257: Ash.Engine.Request.do_next/1
        (ash 2.14.0) lib/ash/engine/request.ex:213: Ash.Engine.Request.next/1
        (ash 2.14.0) lib/ash/engine/engine.ex:712: Ash.Engine.advance_request/2
        (ash 2.14.0) lib/ash/engine/engine.ex:637: Ash.Engine.fully_advance_request/2

While this one works (deliberatly halted with an exception in match? to get the stack trace):

QUERY B: #Ash.Query<
  resource: Ticker.Accounts.User,
  arguments: %{
    search: "Jan"
  }
>
Request: GET /api/eventhub/users?search=Jan
** (exit) an exception was raised:
    ** (RuntimeError) DELIBERATELY HALTED
        (ticker 0.1.0) lib/ticker/checks.ex:63: Ticker.Checks.ValidActorProvided.match?/3
        (ticker 0.1.0) lib/ticker/checks.ex:56: Ticker.Checks.ValidActorProvided.strict_check/3
        (ash 2.14.0) lib/ash/policy/policy.ex:164: Ash.Policy.Policy.fetch_or_strict_check_fact/2
        (ash 2.14.0) lib/ash/policy/policy.ex:480: Ash.Policy.Policy.compile_policy_expression/2
        (ash 2.14.0) lib/ash/policy/policy.ex:319: Ash.Policy.Policy.compile_policy_expression/2
        (ash 2.14.0) lib/ash/policy/policy.ex:68: Ash.Policy.Policy.build_requirements_expression/2
        (ash 2.14.0) lib/ash/policy/policy.ex:26: Ash.Policy.Policy.solve/1
        (ash 2.14.0) lib/ash/policy/checker.ex:83: Ash.Policy.Checker.strict_check_scenarios/1
        (ash 2.14.0) lib/ash/policy/authorizer/authorizer.ex:1142: Ash.Policy.Authorizer.strict_check_result/2
        (ash 2.14.0) lib/ash/policy/authorizer/authorizer.ex:493: Ash.Policy.Authorizer.strict_check/2
        (ash 2.14.0) lib/ash/engine/request.ex:558: Ash.Engine.Request.do_strict_check/3
        (ash 2.14.0) lib/ash/engine/request.ex:524: anonymous fn/2 in Ash.Engine.Request.strict_check/2
        (elixir 1.15.3) lib/enum.ex:4830: Enumerable.List.reduce/3
        (elixir 1.15.3) lib/enum.ex:2564: Enum.reduce_while/3
        (ash 2.14.0) lib/ash/engine/request.ex:257: Ash.Engine.Request.do_next/1
        (ash 2.14.0) lib/ash/engine/request.ex:213: Ash.Engine.Request.next/1
        (ash 2.14.0) lib/ash/engine/engine.ex:712: Ash.Engine.advance_request/2
        (ash 2.14.0) lib/ash/engine/engine.ex:637: Ash.Engine.fully_advance_request/2
        (ash 2.14.0) lib/ash/engine/engine.ex:578: Ash.Engine.do_run_iteration/2
        (elixir 1.15.3) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3

Versions are most recent as of this writing (and both queries run nicely in production with older versions of the framework):

absinthe_plug: 1.5.8
ash: 2.14.0
ash_authentication: 3.11.7
ash_graphql: 0.25.13
ash_json_api: 0.33.0
ash_postgres: 1.3.41
ulbrich commented 1 year ago

Here’s the API definition including the authorization (and the requests work if I set authorize? to false):

defmodule Ticker.Api do
  use Ash.Api,
    extensions: [
      AshGraphql.Api,
      AshJsonApi.Api
    ]

  graphql do
    authorize?(true)
  end

  json_api do
    authorize?(true)
    router(TickerWeb.Api.Router)
  end

  resources do
    registry(Ticker.Api.Registry)
  end
end

Here are the routes:

json_api do
  type("user")

  routes do
    base("/users")

    index(:read_filtered)

    get(:read_by_extid) do
      route("/:extid")
    end

    post(:upsert_user) do
      route("/")
    end
  end
end

Maybe worth mentioning: The problem remains even when allowing very permissive access.

policies do
  policy always() do
    authorize_if always()
  end
end
zachdaniel commented 1 year ago

This appears to be something failing in our new field policy authorization code. I think this should be a relatively straightforward fix.

ulbrich commented 1 year ago

Hello Zach, at least the exception goes away in the JSAN-API as well as in the GraphQL implementation if I add this:

field_policies do
  field_policy :* do
    authorize_if always()
  end
end

That doesn’t fix some other same looking models, though. I added the same field policy and checked all other conditions but while the User model works now, others still fail. Yes, I forced a rebuild of everything.

zachdaniel commented 1 year ago

Hey @ulbrich would you mind trying main? I believe I've fixed this by handling empty field policy lists better.

ulbrich commented 1 year ago

Hi @zachdaniel, thanks for the fast reply: For the original User model that works now (with our without the field_policy above). For my Event model, I now get a different exception if (and only if) I add that field_policy and it works fine without:

[info] GET /api/eventhub/events/618
[debug] Processing with TickerWeb.Api.Router
  Parameters: %{}
  Pipelines: [:api]
[info] Sent 500 in 181ms
[error] #PID<0.2213.0> running TickerWeb.Endpoint (connection #PID<0.2141.0>, stream id 8) terminated
Server: localhost:4000 (http)
Request: GET /api/eventhub/events/618
** (exit) an exception was raised:
    ** (ArgumentError) comparison with nil is forbidden as it is unsafe. If you want to check if a value is nil, use is_nil/1 instead
        (ecto 3.10.3) lib/ecto/query/builder.ex:1048: Ecto.Query.Builder.not_nil!/1
        (ash_postgres 1.3.41) lib/expr.ex:612: anonymous fn/3 in AshPostgres.Expr.do_dynamic_expr/5
        (ecto 3.10.3) lib/ecto/query/builder/dynamic.ex:76: Ecto.Query.Builder.Dynamic.expand/3
        (ecto 3.10.3) lib/ecto/query/builder/dynamic.ex:46: Ecto.Query.Builder.Dynamic.fully_expand/2
        (ecto 3.10.3) lib/ecto/query/builder/filter.ex:133: Ecto.Query.Builder.Filter.filter!/7
        (elixir 1.15.3) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3
        (ash_postgres 1.3.41) lib/data_layer.ex:2175: AshPostgres.DataLayer.filter/4
        (ash 2.14.0) lib/ash/actions/read.ex:1365: anonymous fn/4 in Ash.Actions.Read.data_field/3
        (ash 2.14.0) lib/ash/engine/engine.ex:537: anonymous fn/2 in Ash.Engine.run_iteration/1
        (ash 2.14.0) lib/ash/engine/engine.ex:558: anonymous fn/4 in Ash.Engine.async/2
        (elixir 1.15.3) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
        (elixir 1.15.3) lib/task/supervised.ex:36: Task.Supervised.reply/4
        (ash 2.14.0) lib/ash/engine/engine.ex:552: Ash.Engine.async/2
        (elixir 1.15.3) lib/enum.ex:1693: Enum."-map/2-lists^map/1-1-"/2
        (ash 2.14.0) lib/ash/engine/engine.ex:702: Ash.Engine.start_pending_tasks/1
        (ash 2.14.0) lib/ash/engine/engine.ex:323: Ash.Engine.run_to_completion/1
        (ash 2.14.0) lib/ash/engine/engine.ex:252: Ash.Engine.do_run/2
        (ash 2.14.0) lib/ash/engine/engine.ex:148: Ash.Engine.run/2
        (ash 2.14.0) lib/ash/actions/read.ex:173: Ash.Actions.Read.do_run/3
        (ash 2.14.0) lib/ash/actions/read.ex:96: Ash.Actions.Read.run/3
zachdaniel commented 1 year ago

🤔 It seems strange that you'd be getting that error only for one resource and not the other. Is there anything special about that resource? Any special extensions its using?

ulbrich commented 1 year ago

With the User model, I do not have a lookup by id, but a custom one by extid (also not a UUID, but a unique string). The same is for the Alarm model. All other models have a classic integer id as lookup and only these models run into that exception. But again: If I leave out the field_properties block, it works fine.

Actions for these models:

actions do
  read :read do
    primary?(true)
  end
end

Example JSON-API for these models:

json_api do
  type("event")

  routes do
    base("/events")

    index(:read_filtered)

    get(:read) do
      route("/:id")
    end
  end
end
zachdaniel commented 8 months ago

I must have dropped the ball on this. Are you still experiencing this issue?

ulbrich commented 7 months ago

Looks good now. I guess, you can close the ticket.