ash-project / ash_paper_trail

The extension for keeping an audit log of changes to your Ash resources.
https://hexdocs.pm/ash_paper_trail
MIT License
28 stars 13 forks source link

Ash.Domain requires that authorization is run impact `create_new_version/1` #81

Closed tunchamroeun closed 1 month ago

tunchamroeun commented 1 month ago

Ash.Domain requires that authorization is run impact create_new_version/1 #80

Flow: inserted student_services -> rollback by create action on student_service_versions

Domain config

defmodule ServiceApp.Api do
  use Ash.Domain,
    extensions: [
      AshPaperTrail.Domain
    ]
  authorization do
    authorize(:always)
  end
end

Error

Screenshot 2024-06-13 at 12 37 57 at night
zachdaniel commented 1 month ago

Ah, interesting. 🤔 perhaps what ought to happen in that case is that we should just force authorization to happen instead of raising an error on authorize?: false. That allows extensions to say authorize?: false and just be more gracefully ignored. What do you think?

tunchamroeun commented 1 month ago

How come errors are raining down in ash3? The previous version was running smoothly, do you have any idea why this is happening?

zachdaniel commented 1 month ago

Did you update your app as part of the upgrade to have authorize :always?

tunchamroeun commented 1 month ago

No, it is previous code from ash 2

zachdaniel commented 1 month ago

Regardless of why it's happening now but not before, it is correct behavior that it is failing right now (or at least the currently intended behavior).

We have some options:

  1. Fx it "locally" here by having AshPaperTrail check to see if the domain requires authorization and if it does, we can not set authorize?: false.
  2. Make it such that if you say authorize?: false when a domain is set to authorize :always that instead of making that an error, it just silently ignores it and sets authorize?: true instead.
  3. add another option, which tells it to adopt the behavior of number 2 above.
  4. make the change and release a major version change to AshPaperTrail that removes the authorize?: false.
zachdaniel commented 1 month ago

It must have been a bug in Ash 2 that was fixed in Ash 3 then, as that is how it was supposed to work (although clearly not ideal for this scenario).

zachdaniel commented 1 month ago

In fact, it certainly was. Certain system use cases were unable to determine whether or not authorization was required because we didn't always have to know the domain when building a changeset. Now we do, and therefore we're properly producing the error here.

zachdaniel commented 1 month ago

Probably what we should do is the first option, for now, and then open an issue to do the 4th option on the next major version release of ash_paper_trail.

tunchamroeun commented 1 month ago

I've noticed in ash_paper_trail that setting 'authorize?: false' seems alright since there's no policy applied. However, it might be inherited from the domain configuration, in my opinion.

tunchamroeun commented 1 month ago

I've got you. I'm working on this issue.

zachdaniel commented 1 month ago

👍 you can use Ash.Domain.Info.authorize(changeset.domain) == :always to know if the domain requires authorization always.