fix: pass full context to Ash.load! in cascade changes
#2537
Conversation
The `Ash.load!` calls in `cascade_destroy` and `cascade_update` were only passing `tenant: tenant`, missing the actor and other context options. This caused policy failures when policies require an actor. Use `Ash.Context.to_opts(context)` to pass all relevant context options (actor, tenant, tracer, context, authorize?), consistent with how `context_opts` is already passed to `Ash.bulk_destroy!`/`Ash.bulk_update!` in the same code path. Closes #2536
| |> Ash.load!( | ||
| [{relationship.name, load_query}], | ||
| tenant: tenant | ||
| Ash.Context.to_opts(context) |
There was a problem hiding this comment.
You can just do:
scope: context, but we should also do authorize?: false, so scope: context, authorize?: false to retain original behavior.
There was a problem hiding this comment.
wasn't authorized?: false removed intentionally in favor of having accessing_from set and having policies?
you didn't explicitly answer the question, but you merged the PR
There was a problem hiding this comment.
Ah, yeah you're right 👍 maybe a comment here saying that this is meant to run authorization.
| |> Ash.load!( | ||
| [{relationship.name, load_query}], | ||
| tenant: tenant | ||
| Ash.Context.to_opts(context) |
| |> Ash.load!( | ||
| [{relationship.name, load_query}], | ||
| tenant: tenant | ||
| scope: context, |
There was a problem hiding this comment.
Heads up that this function rebinds context above:
ash/lib/ash/resource/change/cascade_destroy.ex
Lines 299 to 303 in 32ee606
So it's missing the tenant here now
There was a problem hiding this comment.
We should not overwrite context in the first place to avoid this confusion.
Rename the rebound `context` variable to `action_context` to preserve the original context parameter containing tenant, actor, and tracer. This ensures `scope: context` in `Ash.load!` receives the full context.
|
@jimsynz my bad, @barnabasJ is right, we shouldn't be doing |
Authorization was intentionally removed in #1948 in favour of `accessing_from` policies. Passing `authorize?: false` here would bypass that intent.
4 participants
Summary
cascade_destroyandcascade_updateto pass full context (including actor) toAsh.load!tenant: tenantwas passed, causing policy failures when policies require an actorDetails
The
Ash.load!calls in both cascade changes were missing the actor from the context. This was a regression introduced in #1948 whenauthorize?: falsewas removed but the actor wasn't added.The fix uses
Ash.Context.to_opts(context)to pass all relevant context options (actor, tenant, tracer, context, authorize?), consistent with howcontext_optsis already passed toAsh.bulk_destroy!/Ash.bulk_update!in the same code path.Closes #2536