ash icon indicating copy to clipboard operation
ash copied to clipboard

Policies for updates can generate invalid ecto code when loading other resources

Open StrongFennecs opened this issue 4 months ago • 8 comments

Code of Conduct

  • [x] I agree to follow this project's Code of Conduct

AI Policy

  • [x] I agree to follow this project's AI Policy, or I agree that AI was not used while creating this issue.

Versions

When upgrading from 3.5.23 to 3.5.34

Operating system

ubuntu 24, macos

Current Behavior

I have a policy that requires loading a related resource to check some conditions. This generates an invalid ecto query that tries to insert a subquery into the from of an update_all query.

In our case:

We have some resource A, eg Organisations, that has a has_many relationship with some other resource B, eg UserOrganisations,

Previously we had a policy that uses an expr( ) filter to check if an actor is an admin;

    policy action(:update) do
      authorize_if expr(exists(user_organisations, user_id == ^actor(:id) and role == "admin"))
    end

When bumping ash, this seems to generate invalid ecto-- it tries to insert a subquery in the from of an update_all

     * ** (Ecto.QueryError) `update_all` does not allow subqueries in `from` in query:
     
   from o0 in subquery(from o0 in Accounts.Organisation,
       as: 0,
       left_join: u1 in subquery(from u0 in Accounts.UserOrganisation,
       as: 500,
       where: is_nil(
       type(
         as(500).archived_at,
         {:parameterized,
          {Ash.Type.UtcDatetimeUsec.EctoType,
           precision: :microsecond, cast_dates_as: :start_of_day, timezone: :utc}}
       )
     ),

For us this also occurs in a similar way with a forbid_unless statement

forbid_unless relates_to_actor_via([:user_organisation, :user])

where the user_organisation again is generating a subquery.

This is the only similar kinda situation with ecto:

https://groups.google.com/g/elixir-ecto/c/4rjWQoyYhmI/m/9Vz14hUfAgAJ

Reproduction

Create a policy on a resource that requires loading another resource. So a resource A with has_many B. Have a policy that tries to do something on A depending on some property in B, eg if B is an admin.

Expected Behavior

Ash should not generate invalid ecto code or at least fail to compile? If it tries to insert a subquery in from, should that not become a seperate query before somehow?

StrongFennecs avatar Aug 18 '25 01:08 StrongFennecs

Would it be possible @StrongFennecs to make a reproduction repository for this?

ken-kost avatar Aug 20 '25 09:08 ken-kost

Yeah I think a reproduction will be necessary. I will revisit once a reproduction exists (if @ken-kost doesn't get to it first 😉 )

zachdaniel avatar Aug 21 '25 23:08 zachdaniel

@ken-kost your repro was against ash core but this would only happen on ash_postgres tests.

zachdaniel avatar Aug 22 '25 20:08 zachdaniel

@ken-kost your repro was against ash core but this would only happen on ash_postgres tests.

Okay, so I made a test on ash_postgres. Only on post with author named John can this action be called.

in Post:

    policy action(:update_for_john) do
      authorize_if(expr(exists(author, first_name == "John" and id == ^actor(:id))))
    end

made a :update_for_john action and test is:

    author =
      AshPostgres.Test.Author
      |> Ash.Changeset.for_create(:create, %{first_name: "is", last_name: "match"})
      |> Ash.create!()

    post =
      AshPostgres.Test.Post
      |> Ash.Changeset.for_create(:create, %{title: "match"})
      |> Ash.Changeset.manage_relationship(:author, author, type: :append_and_remove)
      |> Ash.create!()

    assert {:error, %Ash.Error.Forbidden{}} =
             post
             |> Ash.Changeset.for_update(:update_for_john, %{}, authorize?: true, actor: author)
             |> Ash.update()

    john =
      AshPostgres.Test.Author
      |> Ash.Changeset.for_create(:create, %{first_name: "John", last_name: "match"})
      |> Ash.create!()

    johns_post =
      AshPostgres.Test.Post
      |> Ash.Changeset.for_create(:create, %{title: "match"})
      |> Ash.Changeset.manage_relationship(:author, john, type: :append_and_remove)
      |> Ash.create!()
      |> Ash.load!(:author)

    {:ok, _johns_post} =
      johns_post
      |> Ash.Changeset.for_update(:update_for_john, %{title: "John's match"},
        authorize?: true,
        actor: john
      )
      |> Ash.update()

and it passes. I also tried without exists and that also works.

ken-kost avatar Aug 24 '25 11:08 ken-kost

Awesome thanks for that. In that case we'll need a reproduction from the original poster :)

zachdaniel avatar Aug 24 '25 21:08 zachdaniel

Sure, yes looks like the issue is actually bumping ash postgres from 2.6.8 to 2.6.15 or 2.6.16 actually and not ash - ash can be bumped fine.

I think theres probably a few more authorisations involved with our example, Ill have to go try come up with a example at some point later this week.

StrongFennecs avatar Aug 25 '25 02:08 StrongFennecs

Seems like adding require_atomic? false in corresponding locations where update is called that triggers the authorisation fixed it in our tests -- So possible more like a bad error message?

StrongFennecs avatar Aug 25 '25 03:08 StrongFennecs

Unlikely, I think it just sends you down a different code path. We should fix the original underlying bug.

zachdaniel avatar Aug 25 '25 13:08 zachdaniel