diff --git a/demo/lib/demo_web/item_actions/user_soft_delete.ex b/demo/lib/demo_web/item_actions/user_soft_delete.ex index 0ccb3bd12..6bb6d338a 100644 --- a/demo/lib/demo_web/item_actions/user_soft_delete.ex +++ b/demo/lib/demo_web/item_actions/user_soft_delete.ex @@ -67,14 +67,23 @@ defmodule DemoWeb.ItemActions.UserSoftDelete do try do updates = [set: [deleted_at: datetime]] + # Backpex re-read these items and authorized exactly them under this action's key before + # calling handle/3, so the write does not check again. {:ok, _count} = - Backpex.Resource.update_all(items, updates, "deleted", socket.assigns.live_resource) + Backpex.Resource.update_all(items, updates, socket.assigns, socket.assigns.live_resource, + event_name: "deleted", + authorize?: false + ) - # nullify the user_id in the posts owned by the users + # nullify the user_id in the posts owned by the users. This is a cascade write on another + # resource, not a user-initiated action on it, so it skips authorization deliberately. _nullified_posts = items |> Enum.map(fn item -> - Backpex.Resource.update_all(item.posts, [set: [user_id: nil]], "updated", DemoWeb.PostLive) + Backpex.Resource.update_all(item.posts, [set: [user_id: nil]], socket.assigns, DemoWeb.PostLive, + event_name: "updated", + authorize?: false + ) end) socket diff --git a/demo/test/demo_web/live/authorization_enforcement_test.exs b/demo/test/demo_web/live/authorization_enforcement_test.exs new file mode 100644 index 000000000..7b5961f2f --- /dev/null +++ b/demo/test/demo_web/live/authorization_enforcement_test.exs @@ -0,0 +1,386 @@ +defmodule DemoWeb.Live.AuthorizationEnforcementTest do + @moduledoc """ + End-to-end checks that Backpex enforces `can?/3` server-side. + + Every test here forges an event the UI would never send: the buttons are hidden or disabled, so + the only way to reach these code paths is a tampered payload. + """ + use DemoWeb.ConnCase, async: false + + import Demo.EctoFactory + import Phoenix.LiveViewTest + + alias Demo.Post + alias Demo.Repo + alias Demo.ShortLink + alias Demo.User + alias Phoenix.LiveView.Socket + + @moduletag :capture_log + + setup do + # `live/2` links the LiveView to the test process. These tests deliberately make it crash, so + # the EXIT signal has to arrive as a message instead of killing the test. + Process.flag(:trap_exit, true) + + :ok + end + + defp assign_socket(assigns) do + Enum.reduce(assigns, %Socket{}, fn {key, value}, socket -> + Phoenix.Component.assign(socket, key, value) + end) + end + + describe "forged item actions on a resource that denies the action" do + setup do + product = insert(:product) + + {:ok, short_link} = + Repo.insert(%ShortLink{short_key: "forgedkey", url: "https://example.com", product_id: product.id}) + + %{short_link: short_link} + end + + test "raises ForbiddenError and keeps the record", %{conn: conn, short_link: short_link} do + {:ok, view, _html} = live(conn, ~p"/admin/short-links") + + assert {{%Backpex.ForbiddenError{}, _stacktrace}, _mfa} = + catch_exit( + render_click(view, "item-action", %{"action-key" => "delete", "item-id" => short_link.short_key}) + ) + + assert Repo.get_by(ShortLink, short_key: "forgedkey") + end + end + + describe "forged item actions with a confirmation modal" do + test "an unauthorized item raises before the modal opens", %{conn: conn} do + admin = insert(:user, %{role: :admin}) + + {:ok, view, _html} = live(conn, ~p"/admin/users") + + assert {{%Backpex.ForbiddenError{}, _stacktrace}, _mfa} = + catch_exit( + render_click(view, "item-action", %{"action-key" => "user_soft_delete", "item-id" => admin.id}) + ) + + assert Repo.get(User, admin.id).deleted_at == nil + end + + test "a nonexistent item id raises NoResultsError", %{conn: conn} do + insert(:user) + + {:ok, view, _html} = live(conn, ~p"/admin/users") + + assert {{%Backpex.NoResultsError{}, _stacktrace}, _mfa} = + catch_exit(render_click(view, "item-action", %{"action-key" => "user_soft_delete", "item-id" => "0"})) + end + end + + describe "forged action keys" do + test "an unknown key on a row raises NoResultsError, not ArgumentError", %{conn: conn} do + user = insert(:user) + + {:ok, view, _html} = live(conn, ~p"/admin/users") + + assert {{%Backpex.NoResultsError{}, _stacktrace}, _mfa} = + catch_exit( + render_click(view, "item-action", %{ + "action-key" => "no_such_backpex_item_action_key", + "item-id" => user.id + }) + ) + end + + test "an unknown key on the toolbar raises NoResultsError, not ArgumentError", %{conn: conn} do + insert(:user) + + {:ok, view, _html} = live(conn, ~p"/admin/users") + + assert {{%Backpex.NoResultsError{}, _stacktrace}, _mfa} = + catch_exit(render_click(view, "item-action", %{"action-key" => "another_missing_action_key"})) + end + end + + describe "forged selection ids" do + test "an unknown id is ignored and never enters the selection", %{conn: conn} do + user = insert(:user) + + {:ok, view, _html} = live(conn, ~p"/admin/users") + + # A nil in `selected_items` would blow up in DemoWeb.UserLive.can?/3 on the next render. + render_click(view, "update-selected-items", %{"id" => "0"}) + + assert has_element?(view, "button[phx-value-action-key='user_soft_delete'][disabled]") + refute has_element?(view, "#select-input-#{user.id}[checked]") + + render_click(view, "update-selected-items", %{"id" => user.id}) + + assert has_element?(view, "#select-input-#{user.id}[checked]") + refute has_element?(view, "button[phx-value-action-key='user_soft_delete'][disabled]") + end + end + + describe "mixed selections" do + setup do + %{user: insert(:user, %{role: :user}), admin: insert(:user, %{role: :admin})} + end + + test "disable the bulk action button and say why", %{conn: conn, user: user, admin: admin} do + {:ok, view, _html} = live(conn, ~p"/admin/users") + + render_click(view, "update-selected-items", %{"id" => user.id}) + refute has_element?(view, "button[phx-value-action-key='user_soft_delete'][disabled]") + + # The admin's checkbox is disabled in the UI, so this selection can only be built by forging + # the event — but the button must still explain itself rather than being a dead end. + render_click(view, "update-selected-items", %{"id" => admin.id}) + + assert has_element?(view, "button[phx-value-action-key='user_soft_delete'][disabled]") + + assert has_element?( + view, + "button[phx-value-action-key='user_soft_delete'][title='Your selection contains items you may not apply this action to.']" + ) + end + + test "an item no action applies to cannot be selected", %{conn: conn, user: user, admin: admin} do + {:ok, view, _html} = live(conn, ~p"/admin/users") + + # `user_soft_delete` is the only bulk action on users and it is denied for admins, so an + # admin row can never take part in one. + assert has_element?(view, "#select-input-#{admin.id}[disabled]") + refute has_element?(view, "#select-input-#{user.id}[disabled]") + + assert has_element?( + view, + "#select-input-#{admin.id}[title='No action is available for this item.']" + ) + end + + test "select all skips items no action applies to", %{conn: conn, user: user, admin: admin} do + {:ok, view, _html} = live(conn, ~p"/admin/users") + + render_click(view, "toggle-item-selection", %{}) + + assert has_element?(view, "#select-input-#{user.id}[checked]") + refute has_element?(view, "#select-input-#{admin.id}[checked]") + + # A select-all that produced an unusable selection would be the dead end this avoids. + refute has_element?(view, "button[phx-value-action-key='user_soft_delete'][disabled]") + end + + test "empty selections say what to do instead of just being disabled", %{conn: conn} do + {:ok, view, _html} = live(conn, ~p"/admin/users") + + assert has_element?( + view, + "button[phx-value-action-key='user_soft_delete'][title='Select at least one item to use this action.']" + ) + end + + test "raise ForbiddenError when the bulk action is forged anyway", %{conn: conn, user: user, admin: admin} do + {:ok, view, _html} = live(conn, ~p"/admin/users") + + render_click(view, "update-selected-items", %{"id" => user.id}) + render_click(view, "update-selected-items", %{"id" => admin.id}) + + assert {{%Backpex.ForbiddenError{}, _stacktrace}, _mfa} = + catch_exit(render_click(view, "item-action", %{"action-key" => "user_soft_delete"})) + + assert Repo.get(User, user.id).deleted_at == nil + assert Repo.get(User, admin.id).deleted_at == nil + end + + test "are re-checked on submit when the selection is widened after the modal opened", %{ + conn: conn, + user: user, + admin: admin + } do + {:ok, view, _html} = live(conn, ~p"/admin/users") + + render_click(view, "update-selected-items", %{"id" => user.id}) + render_click(view, "item-action", %{"action-key" => "user_soft_delete"}) + + assert has_element?(view, "#resource-form") + + # The authorization state changes while the modal is open. Before the submit gate existed, + # the unauthorized item was silently filtered out and the action reported success. + render_click(view, "update-selected-items", %{"id" => admin.id}) + + assert {{%Backpex.ForbiddenError{}, _stacktrace}, _mfa} = + view + |> form("#resource-form", change: %{reason: "widened after opening"}) + |> render_submit() + |> catch_exit() + + assert Repo.get(User, user.id).deleted_at == nil + assert Repo.get(User, admin.id).deleted_at == nil + end + end + + describe "the selection follows the rows it points at" do + setup %{conn: conn} do + user = insert(:user, %{role: :user}) + + {:ok, view, _html} = live(conn, ~p"/admin/users") + + render_click(view, "update-selected-items", %{"id" => user.id}) + refute has_element?(view, "button[phx-value-action-key='user_soft_delete'][disabled]") + + %{user: user, view: view} + end + + test "an updated row is replaced in the selection, not only in the table", %{user: user, view: view} do + user |> Ecto.Changeset.change(role: :admin) |> Repo.update!() + + send(view.pid, {"backpex:updated", user}) + _html = render(view) + + # The button reads the *selection* to decide. If the selection still held the `role: :user` + # snapshot it would stay enabled and lead straight into the submit gate. + assert has_element?( + view, + "button[phx-value-action-key='user_soft_delete'][title='Your selection contains items you may not apply this action to.']" + ) + end + + test "a deleted row leaves the selection", %{user: user, view: view} do + Repo.delete!(user) + + send(view.pid, {"backpex:deleted", user}) + _html = render(view) + + assert has_element?( + view, + "button[phx-value-action-key='user_soft_delete'][title='Select at least one item to use this action.']" + ) + end + + test "a row that left the item query's scope leaves the selection too", %{user: user, view: view} do + # An update can take a row out of scope just as a delete removes it. Re-reading it returns + # nothing, so it must not linger in `items` or `selected_items` as `nil`. + user |> Ecto.Changeset.change(deleted_at: DateTime.utc_now(:second)) |> Repo.update!() + + send(view.pid, {"backpex:updated", user}) + html = render(view) + + refute html =~ "select-input-#{user.id}" + + assert has_element?( + view, + "button[phx-value-action-key='user_soft_delete'][title='Select at least one item to use this action.']" + ) + end + end + + describe "the selection is re-read before the submit gate" do + setup %{conn: conn} do + user = insert(:user, %{role: :user}) + + {:ok, view, _html} = live(conn, ~p"/admin/users") + + # Select the row and open the confirm modal. From here on `selected_items` holds a snapshot + # of the record as it was at this moment — every test below changes the row behind the + # LiveView's back, without a broadcast, so nothing refreshes it. + render_click(view, "update-selected-items", %{"id" => user.id}) + render_click(view, "item-action", %{"action-key" => "user_soft_delete"}) + + assert has_element?(view, "#resource-form") + + %{user: user, view: view} + end + + test "a record that turned unauthorized while the modal was open raises on submit", %{user: user, view: view} do + # `DemoWeb.UserLive.can?/3` denies `:user_soft_delete` for admins. The snapshot in + # `selected_items` still says `role: :user`, so only re-reading the row catches this. + user |> Ecto.Changeset.change(role: :admin) |> Repo.update!() + + assert {{%Backpex.ForbiddenError{}, _stacktrace}, _mfa} = + view + |> form("#resource-form", change: %{reason: "promoted while the modal was open"}) + |> render_submit() + |> catch_exit() + + assert Repo.get(User, user.id).deleted_at == nil + end + + test "a record deleted while the modal was open raises NoResultsError on submit", %{user: user, view: view} do + Repo.delete!(user) + + assert {{%Backpex.NoResultsError{}, _stacktrace}, _mfa} = + view + |> form("#resource-form", change: %{reason: "deleted while the modal was open"}) + |> render_submit() + |> catch_exit() + end + + test "a record that left the item query's scope raises NoResultsError on submit", %{user: user, view: view} do + # `DemoWeb.UserLive.item_query/3` hides soft-deleted users, so this row is gone as far as the + # resource is concerned even though it is still in the table. + user |> Ecto.Changeset.change(deleted_at: DateTime.utc_now(:second)) |> Repo.update!() + + assert {{%Backpex.NoResultsError{}, _stacktrace}, _mfa} = + view + |> form("#resource-form", change: %{reason: "soft deleted while the modal was open"}) + |> render_submit() + |> catch_exit() + end + + test "handle/3 gets the re-read record, not the snapshot", %{conn: conn, user: user, view: view} do + # The user had no posts when the row was selected, so the snapshot's `posts` is `[]`. + # `DemoWeb.ItemActions.UserSoftDelete.handle/3` nullifies `user_id` on the posts it is + # handed — the post below can only be reached through the re-read record. + post = insert(:post, user: user) + + result = + view + |> form("#resource-form", change: %{reason: "still allowed"}) + |> render_submit() + + assert {:ok, _view, html} = follow_redirect(result, conn) + assert html =~ "User has been deleted successfully." + + assert Repo.get(User, user.id).deleted_at != nil + assert Repo.get(Post, post.id).user_id == nil + end + end + + describe "resource actions" do + test "an authorized submit runs the action", %{conn: conn} do + insert(:user) + + {:ok, view, _html} = live(conn, ~p"/admin/users/invite/resource-action") + + result = + view + |> form("#resource-form", change: %{text: "Please join us"}) + |> render_submit(%{"change" => %{"users" => ["user_id_alex"]}, "save-type" => "save"}) + + assert {:ok, _view, html} = follow_redirect(result, conn) + assert html =~ "An email has been successfully sent" + end + + test "the submit gate raises when the action is denied while the form is open" do + # The route already refuses an action the user may not open, so the only way to reach the + # submit gate with a denial is a permission that changed while the modal was open. Drive the + # form component directly rather than pretend a demo resource can do that. + socket = + assign_socket( + live_action: :resource_action, + action_type: :resource, + live_resource: DemoWeb.InvoiceLive, + resource_action: %{module: DemoWeb.ResourceActions.Email}, + resource_action_id: :invite, + fields: [], + item: %{}, + return_to: "/admin/invoices" + ) + + assert_raise Backpex.ForbiddenError, fn -> + Backpex.FormComponent.handle_event("save", %{"change" => %{}, "save-type" => "save"}, socket) + end + end + end +end diff --git a/guides/actions/item-actions.md b/guides/actions/item-actions.md index 92db78b4c..86b06b151 100644 --- a/guides/actions/item-actions.md +++ b/guides/actions/item-actions.md @@ -179,25 +179,30 @@ defmodule DemoWeb.ItemAction.SoftDelete do @impl Backpex.ItemAction def handle(socket, items, data) do - datetime = DateTime.truncate(DateTime.utc_now(), :second) + datetime = DateTime.utc_now(:second) socket = try do - {:ok, _count_} = + # Backpex already authorized exactly these items under this action's key, so this write + # does not check again. See "Authorization" below. + {:ok, _items} = Backpex.Resource.update_all( - socket.assigns, items, [set: [deleted_at: datetime, reason: data.reason]], - "deleted" + socket.assigns, + socket.assigns.live_resource, + event_name: "deleted", + authorize?: false ) - socket - |> clear_flash() - |> put_flash(:info, "Item(s) successfully deleted.") - rescue socket |> clear_flash() - |> put_flash(:error, error) + |> put_flash(:info, "Item(s) successfully deleted.") + rescue + error -> + socket + |> clear_flash() + |> put_flash(:error, Exception.message(error)) end {:ok, socket} @@ -209,3 +214,66 @@ The above ItemAction require users to fill out the reason field before the actio > #### Important {: .note} > If your ItemAction has form fields, you must also implement the `c:Backpex.ItemAction.confirm/1` function. + +## Authorization + +Item actions are authorized against the key they are registered under. Implement [`can?/3`](Backpex.LiveResource.html#c:can?/3) in your resource configuration module: + +```elixir +# in your resource configuration file +@impl Backpex.LiveResource +def can?(_assigns, :soft_delete, item), do: item.role != :admin +def can?(_assigns, _action, _item), do: true +``` + +Backpex enforces this for you — you do not need to check it again inside `c:Backpex.ItemAction.handle/3`. There are five things to know: + +**Enforcement is strict.** A selection containing a single unauthorized item raises `Backpex.ForbiddenError`; items are never silently dropped. A stale or forged item id raises `Backpex.NoResultsError`. Raised from an event handler on a connected socket, either one crashes the LiveView and the client reloads — no error page and no message, only the guarantee that nothing was written. Users are kept away from the gates by the preflight checks, not by the gates' error reporting. Because a mixed selection would raise, the toolbar button is disabled whenever the selection is empty or contains an unauthorized item, and a row that is authorized for no bulk action at all cannot be selected. + +**Each gesture is authorized exactly once per step.** An action without a confirmation modal is authorized immediately before `c:Backpex.ItemAction.handle/3` runs. An action with one is authorized when the modal opens and again when it is submitted — the second check is deliberate, because a permission may be revoked, or the selection widened, while the modal is open. + +**The selection is re-read right before the execution gate.** A selection is a snapshot: rows are cached when they are selected, and a confirmation modal can stay open for as long as the user likes. Backpex therefore re-reads every selected item by its primary key immediately before the authoritative gate — the one that runs just before `c:Backpex.ItemAction.handle/3` — authorizes those fresh records, and hands *them* to `handle/3`. If another actor changed a row in the meantime, `can?/3` sees the new values, not the rendered ones. If a row was deleted, or left the resource's [`item_query/3`](item-query.html) scope, it comes back as `nil` and the gate raises `Backpex.NoResultsError` instead of writing to a record nobody checked. The re-read goes through the adapter, so `item_query/3` applies exactly as it does everywhere else. + +> #### The re-read is not a lock {: .warning} +> +> A window remains between the re-read and whatever `c:Backpex.ItemAction.handle/3` writes. Backpex deliberately does not open a transaction or lock the rows: your `handle/3` owns the write and decides what isolation it needs. An action that requires strict atomicity has to re-read and lock inside its own `handle/3` — for example in an `c:Ecto.Repo.transaction/2` with a `lock: "FOR UPDATE"` query. + +**`handle/3` gets the full selection, and is never called with `[]`.** For an empty selection Backpex skips the action entirely. + +**Inside `handle/3`, the items you were handed are already authorized.** They are the records Backpex just re-read, and the gate covered exactly those items under exactly this action's key — so a `Backpex.Resource` call that writes those same items should pass `authorize?: false`: + +```elixir +Backpex.Resource.delete_all(items, socket.assigns, socket.assigns.live_resource, authorize?: false) +``` + +Anything else the action writes is *not* covered by that gate and keeps the default check. `Backpex.Resource` mutations default to `:new` / `:edit` / `:delete`; pass `:authorization_action` when a different key is the right one to check. `assigns.item_action_key` holds the key this action is registered under for the duration of the `handle/3` call — Backpex clears it again afterwards — so the action does not need to hardcode it: + +```elixir +# writing other items of the same resource, under this action's key +Backpex.Resource.update_all(other_items, updates, socket.assigns, socket.assigns.live_resource, + authorization_action: socket.assigns.item_action_key +) +``` + +A cascade write to a *different* resource (nullifying a foreign key, for example) is not a user-initiated action on that resource at all — pass `authorize?: false`: + +```elixir +Backpex.Resource.update_all(item.posts, [set: [user_id: nil]], socket.assigns, MyAppWeb.PostLive, + event_name: "updated", + authorize?: false +) +``` + +> #### Do not swallow the gate {: .warning} +> +> This only applies to `Backpex.Resource` calls that are still gated — the ones you did *not* pass `authorize?: false`. A broad `rescue` around such a call catches `Backpex.ForbiddenError` and `Backpex.NoResultsError` too, turning a refused write into an ordinary flash message: the request looks handled, and the failure is filed under "something went wrong" instead of "you may not do this". Reraise them: +> +> ```elixir +> rescue +> error in [Backpex.ForbiddenError, Backpex.NoResultsError] -> +> reraise error, __STACKTRACE__ +> +> error -> +> # your own error handling +> end +> ``` diff --git a/guides/actions/resource-actions.md b/guides/actions/resource-actions.md index 82c5e269e..e84857751 100644 --- a/guides/actions/resource-actions.md +++ b/guides/actions/resource-actions.md @@ -87,4 +87,25 @@ We validate the email address using the `validate_email/2` function provided by > #### Info {: .info} > -> Each resource action has its own route. The route is defined by the `id` of the resource action. If you use the [`live_resource/3`](Backpex.Router.html#live_resources/3) macro, the route is automatically added to the live resource. \ No newline at end of file +> Each resource action has its own route. The route is defined by the `id` of the resource action. If you use the [`live_resource/3`](Backpex.Router.html#live_resources/3) macro, the route is automatically added to the live resource. + +## Authorization + +A resource action is authorized against its `id`, with a `nil` item: + +```elixir +# in your resource configuration file +@impl Backpex.LiveResource +def can?(assigns, :invite, _item), do: assigns.current_user.role == :admin +def can?(_assigns, _action, _item), do: true +``` + +Backpex checks this twice: when the modal is opened (the button is not rendered at all when the check fails, and the route itself refuses) and again when the form is submitted, so a permission revoked while the form was open cannot be used. An unauthorized submit raises `Backpex.ForbiddenError`, which crashes the LiveView and reloads the client — the write does not happen, and the user is not shown a 403 page or a message. See [Authorization](live-resource-authorization.html) for the details. + +If your [`handle/2`](Backpex.ResourceAction.html#c:handle/2) calls `Backpex.Resource` functions, note that those authorize against their own defaults — `:new` for `insert/6`, `:edit` for `update/6` and `update_all/5`, `:delete` for `delete_all/4` — not against the resource action's key. Pass `authorization_action:` when the resource action's own key is the right one to check, or `authorize?: false` when the write is a system side effect rather than a user action on that resource: + +```elixir +Backpex.Resource.update_all(items, updates, socket.assigns, MyAppWeb.UserLive, + authorization_action: :invite +) +``` \ No newline at end of file diff --git a/guides/authorization/live-resource-authorization.md b/guides/authorization/live-resource-authorization.md index a3cd9542a..84465c05c 100644 --- a/guides/authorization/live-resource-authorization.md +++ b/guides/authorization/live-resource-authorization.md @@ -50,4 +50,97 @@ The `can?` callback receives the following parameters: ## Return value -The `can?` callback must return a boolean value. If the return value is `true`, the action is allowed. If the return value is `false`, the action is denied. \ No newline at end of file +The `can?` callback must return a boolean value. If the return value is `true`, the action is allowed. If the return value is `false`, the action is denied. + +## Enforcement + +Backpex enforces `can?/3` centrally, through `Backpex.Authorization`. You do not need to repeat the check in your own actions. + +There are two kinds of checks, and both run: + +- **Preflight** — decides whether a control is rendered or disabled. A user never sees a button for something they may not do. +- **Gate** — runs immediately before something happens and raises `Backpex.ForbiddenError` when it fails. This is what makes a forged or stale event safe. + +> #### What a denial looks like {: .info} +> +> `Backpex.ForbiddenError` and `Backpex.NoResultsError` carry a `plug_status` of 403 and 404, but Phoenix LiveView only maps that to an HTTP status while a view **mounts** — the dead render. Raised from an event handler on a connected socket (which is where the item and resource action gates live), the LiveView process crashes and the client reloads the page. The user gets no error page and no flash message. +> +> That is the intended outcome: the point of a gate is that the write does not happen. Do not rely on it to communicate anything — a user should never reach a gate through the UI in the first place, because the preflight checks already hid or disabled the control. + +### Where the gates are + +| what happens | action checked | item | +| --- | --- | --- | +| `:index` / `:show` view mounts | `:index` / `:show` | the item, for `:show` | +| `:new` / `:edit` form mounts | `:new` / `:edit` | the item, for `:edit` | +| `Backpex.Resource.insert/6` | `:new` | `nil` | +| `Backpex.Resource.update/6` | `:edit` | the item | +| `Backpex.Resource.update_all/5` | `:edit` | each item | +| `Backpex.Resource.delete_all/4` | `:delete` | each item | +| item action without a confirm modal, before `handle/3` runs | the action key | each selected item, **re-read** | +| item action with a confirm modal, on open | the action key | each selected item, as rendered | +| item action with a confirm modal, on submit | the action key | each selected item, **re-read** | +| resource action, on open and on submit | the action key | `nil` | + +The `Backpex.Resource` gates run **before** the changeset is built and before `c:Backpex.Field.before_changeset/6` is called, so your own code never executes for an unauthorized request. + +Each gesture runs exactly one gate per step, so `can?/3` is not evaluated more times than there are decisions to make. A modal flow has two steps on purpose: the second check catches a permission revoked, or a selection widened, while the modal was open. + +### Item action gates re-read the selection + +A selection is a snapshot. Rows are cached in `selected_items` when they are selected, and a confirmation modal can stay open indefinitely. Authorizing that snapshot would mean checking values that may no longer be true, while the write that follows addresses the row by its primary key — so a record another actor changed in the meantime would be mutated under a permission it no longer has. + +Backpex therefore re-reads every selected item by its primary key immediately before the authoritative gate, checks `can?/3` against the **re-read** records, and passes those same records to `c:Backpex.ItemAction.handle/3`. Both execution paths do this: the direct dispatch of a confirm-less action, and the submit of a confirmation modal. The re-read goes through the adapter, so [`item_query/3`](item-query.html) applies — a row that was deleted, or that has left the query's scope, comes back as `nil` and raises `Backpex.NoResultsError`. + +The check when the modal *opens* is deliberately not re-read: it only decides whether the dialog appears, and the submit gate is the one that authorizes the write. + +> #### A re-read is not a lock {: .warning} +> +> A window remains between the re-read and whatever `c:Backpex.ItemAction.handle/3` writes. Backpex does not open a transaction or lock the rows here — `handle/3` owns the write and decides what isolation it needs. An action that requires strict atomicity has to re-read and lock inside its own `handle/3`, for example in an `c:Ecto.Repo.transaction/2` with a `lock: "FOR UPDATE"` query. + +### Strict semantics + +Checks over a selection are strict: a single unauthorized item raises, and nothing runs. Backpex does not silently drop items from a selection. + +A `nil` item — a stale or forged id — raises `Backpex.NoResultsError` and never reaches your `can?/3`, so you do not need clauses for it. + +Because a mixed selection would raise, the bulk action button is disabled whenever the selection is empty or contains any unauthorized item. A row that is authorized for none of the bulk actions cannot be selected at all — its checkbox is disabled, so a user cannot build a selection that has no usable action. + +### What `handle/3` may assume + +The items handed to `c:Backpex.ItemAction.handle/3` are the records Backpex just re-read, and they have already been authorized under that action's key. Writing exactly those items back is the same decision the gate just made, so pass `authorize?: false` rather than paying for a second evaluation of your `can?/3`: + +```elixir +Backpex.Resource.delete_all(items, socket.assigns, socket.assigns.live_resource, authorize?: false) +``` + +The guarantee covers only those items under that key. Writes to *other* items or to another resource keep the default gate. + +### Overriding the action and the escape hatch + +Every `Backpex.Resource` mutation accepts two options: + +- `:authorization_action` — authorize against this action instead of the default. It must be a non-nil atom. Item actions can pass `socket.assigns.item_action_key`, so an action registered under a custom key is authorized under that key. Backpex sets that assign just before calling `c:Backpex.ItemAction.handle/3` and clears it again when the call returns, so it is meaningful for exactly one dispatch. +- `authorize?: false` — skip the check. Use this for a write the gate already covered (see above), and for system or cascade writes that are not a user-initiated action on the resource being written, for example nullifying a foreign key on another resource. + +```elixir +Backpex.Resource.update_all(item.posts, [set: [user_id: nil]], socket.assigns, MyAppWeb.PostLive, + event_name: "updated", + authorize?: false +) +``` + +### Reads are not gated in `Backpex.Resource` + +`Backpex.Resource.list/4`, `get/4`, `count/4` and `reload/4` do not call `can?/3`. Row-level read filtering belongs in [`item_query/3`](item-query.html) — dropping rows after pagination would corrupt item counts and select-all. `:index` and `:show` are enforced when the view mounts. `reload/4` is the read the item action gates use before they authorize; it is the gate that follows it, not the read, that raises. + +### Calling the checks yourself + +If you build your own UI on top of Backpex, use `Backpex.Authorization` rather than calling `can?/3` directly: + +```elixir +Backpex.Authorization.can?(live_resource, assigns, :edit, item) +Backpex.Authorization.can_all?(live_resource, assigns, :delete, items) +Backpex.Authorization.authorize!(live_resource, assigns, :edit, item) +Backpex.Authorization.authorize_all!(live_resource, assigns, :delete, items) +``` \ No newline at end of file diff --git a/guides/upgrading/v0.21.md b/guides/upgrading/v0.21.md new file mode 100644 index 000000000..2cd844408 --- /dev/null +++ b/guides/upgrading/v0.21.md @@ -0,0 +1,254 @@ +# Upgrading to v0.21 + +## Bump Your Deps + +Update Backpex to the latest version: + +```elixir +defp deps do + [ + {:backpex, "~> 0.21.0"} + ] +end +``` + +v0.21 makes authorization something Backpex enforces rather than something every +call site has to remember. `c:Backpex.LiveResource.can?/3` is now evaluated +centrally — in `Backpex.Resource` before any mutation, and directly before every +item and resource action runs. + +If you never call `Backpex.Resource` yourself and never wrote a custom item +action, the only change you may notice is that a few forged or stale interactions +now raise instead of silently doing nothing. If you do, read on: two function +signatures changed. + +## 1. Security fix: the item action key no longer comes from the DOM + +> #### Security {: .error} +> +> Before v0.21, submitting an item action modal read the action key from the +> `phx-value-action-key` DOM parameter. The `can?/3` check ran against that +> client-supplied key while the server executed the module stored in +> `action_to_confirm` — a client could pass the key of an action it *is* allowed +> to perform and have a different, unauthorized action executed. This was +> actively bypassable, not merely easy to forget. + +The key is now taken from `socket.assigns.action_to_confirm.key`, which the view +sets when it opens the modal, and `phx-value-action-key` has been removed from +the form. Nothing to do on your side unless you rendered Backpex's form component +yourself with a hand-built `action-key` value. + +Client-supplied action keys are also no longer passed through +`String.to_existing_atom/1`. They are matched against the registered +`item_actions/1` and `resource_actions/0` keys, so an unknown key raises +`Backpex.NoResultsError` (404) instead of an `ArgumentError`. + +## 2. `delete_all/2` → `delete_all/4` + +`delete_all` needs the assigns to authorize the deletion. + +```diff +- Backpex.Resource.delete_all(items, socket.assigns.live_resource) ++ Backpex.Resource.delete_all(items, socket.assigns, socket.assigns.live_resource) +``` + +## 3. `update_all/3` and `update_all/4` → `update_all/5` + +`update_all` also needs the assigns, and `event_name` moved into the options. +The old head had a default argument (`event_name \\ "updated"`), so it defined +both an arity-3 and an arity-4 function. Both are gone: + +```diff +- Backpex.Resource.update_all(items, updates, MyAppWeb.UserLive) ++ Backpex.Resource.update_all(items, updates, socket.assigns, MyAppWeb.UserLive) + +- Backpex.Resource.update_all(items, updates, "deleted", MyAppWeb.UserLive) ++ Backpex.Resource.update_all(items, updates, socket.assigns, MyAppWeb.UserLive, event_name: "deleted") +``` + +> #### Watch the argument order {: .warning} +> +> The two old forms fail differently, and neither fails silently: +> +> * `update_all(items, updates, MyAppWeb.UserLive)` raises +> `UndefinedFunctionError` — there is no `update_all/3` any more. +> * `update_all(items, updates, "deleted", MyAppWeb.UserLive)` has the same arity +> as the new `update_all(items, updates, assigns, live_resource)`. A guard +> (`is_map(assigns) and not is_struct(assigns)`) makes it raise +> `FunctionClauseError` instead of quietly authorizing against the event name +> string. +> +> The same guard also rejects `socket` where `socket.assigns` belongs. If you see +> either error, you missed a call site. + +## 4. Central enforcement and default actions + +Every mutation in `Backpex.Resource` now authorizes before it does anything else +— before the changeset is built and before `c:Backpex.Field.before_changeset/6` +runs, so your own code never executes for an unauthorized request. + +| function | authorizes | item passed to `can?/3` | +| --- | --- | --- | +| `insert/6` | `:new` | `nil` | +| `update/6` | `:edit` | the item | +| `update_all/5` | `:edit` | each item | +| `delete_all/4` | `:delete` | each item | + +Note that `insert/6` checks with `nil`, consistent with every other `:new` check +in Backpex. A clause like `def can?(_assigns, :new, nil), do: false` is now +honored on save, not only when the form is opened. + +Reads (`list/4`, `get/4`, `count/4`) are **not** authorized here. `:index` and +`:show` are still enforced in the view layer — filtering rows after pagination +would corrupt counts and select-all. + +### What an item action's `handle/3` may assume + +Backpex calls `c:Backpex.ItemAction.handle/3` only after the gate authorized +**exactly those items under exactly that action's key**. The items you receive +are the ones the gate re-read from the data layer, not the snapshot that was +rendered. Writing those same items back is the decision the gate already made, +so pass `authorize?: false` instead of paying for a second evaluation of your +`can?/3`: + +```elixir +def handle(socket, items, _data) do + # already authorized by Backpex before this ran + Backpex.Resource.delete_all(items, socket.assigns, socket.assigns.live_resource, authorize?: false) + + {:ok, socket} +end +``` + +The guarantee covers nothing else. Writes to *other* items, or to another +resource, keep the default gate. + +### Overriding the action: `:authorization_action` + +When an action writes items the gate did not cover, `:authorization_action` +picks the key to check instead of the `:new` / `:edit` / `:delete` default: + +```elixir +Backpex.Resource.update_all(other_items, updates, socket.assigns, socket.assigns.live_resource, + authorization_action: socket.assigns.item_action_key +) +``` + +`assigns.item_action_key` is new in v0.21. Backpex sets it immediately before +calling `c:Backpex.ItemAction.handle/3` and clears it again afterwards, so it is +meaningful exactly for the duration of one dispatch and the action does not need +to know which key it was registered under. + +The option must be a non-nil atom; anything else raises `ArgumentError` rather +than reaching a permissive catch-all `can?/3` clause. + +### Skipping the check: `authorize?: false` + +System and cascade writes are not user-initiated actions on the resource they +touch. Skip the check explicitly: + +```elixir +# nullify the foreign key on another resource +Backpex.Resource.update_all(item.posts, [set: [user_id: nil]], socket.assigns, MyAppWeb.PostLive, + event_name: "updated", + authorize?: false +) +``` + +The option is deliberately explicit and greppable. Reach for it only when the +write is already covered by the gate, or when it really is not the user's action +on that resource. + +## 5. Item actions are strict now + +Previously, an item action silently filtered unauthorized items out of the +selection and then ran `handle/3` with what was left — including an empty list, +which usually reported success. That is gone. + +* A selection containing a single unauthorized item raises + `Backpex.ForbiddenError`. Nothing runs. +* A stale or forged item id raises `Backpex.NoResultsError`. `nil` never + reaches your `can?/3`. +* An unknown action key raises `Backpex.NoResultsError`. +* `handle/3` receives the **full** list of selected items, and is not called at + all for an empty selection. +* The bulk action button in the toolbar is disabled when the selection is empty + **or** contains any unauthorized item, so the UI never offers a click that + would raise. Its `title` says why. +* A row that is authorized for none of the bulk actions can no longer be + selected: its checkbox is disabled, and "select all" skips it. Without this a + user could build a selection whose every action is disabled, with no way to + tell which row caused it. + +Three consequences worth knowing: + +* **A raised gate is not an error page.** Both exceptions carry a `plug_status`, + but LiveView only maps that to an HTTP status while a view mounts. From an + event handler on a connected socket the LiveView process crashes and the client + reloads the page — no 403 page, no flash. That is intended: the gate exists so + the write does not happen, not to explain itself. The preflight checks are what + keep a user from reaching a gate through the UI at all. +* **Double-clicking a delete button** after the item is gone now raises + `Backpex.NoResultsError` — the LiveView crashes and reconnects instead of + flashing "0 items deleted". This is a deliberate trade-off for not leaking + whether an id exists. +* **The execution gate checks re-read records, not the rendered ones.** Right + before it authorizes, Backpex re-reads every selected item by its primary key + through the adapter, checks `can?/3` against those records and passes them to + `handle/3`. A row another actor changed while the confirmation modal was open + is judged by its current values; a row that was deleted, or that has left the + resource's `item_query/3` scope, comes back as `nil` and raises + `Backpex.NoResultsError`. The check when the modal *opens* still uses the + rendered items — it only decides whether the dialog appears. + + This is not a lock. A window remains between the re-read and whatever your + `handle/3` writes; Backpex opens no transaction and takes no row locks there. + An action that needs strict atomicity has to re-read and lock inside its own + `handle/3` (`c:Ecto.Repo.transaction/2` plus a `lock: "FOR UPDATE"` query). + +## 6. `handle_item_action/5` behavior change + +`Backpex.ItemAction.handle_item_action/5` is public. It no longer filters items; +it re-reads them, authorizes them and raises. If you call it yourself, expect +`Backpex.ForbiddenError` / `Backpex.NoResultsError` where you previously got a +shorter list, and expect `c:Backpex.ItemAction.handle/3` to receive the re-read +records rather than the list you passed in. `Backpex.ItemAction.authorize_fresh!/3` +is that re-read-then-authorize step on its own, and `Backpex.ItemAction.dispatch/5` +remains the unguarded "just run it" primitive. + +## 7. Do not let a `rescue` swallow the gate + +This applies to `Backpex.Resource` calls that are still gated — the ones you did +*not* pass `authorize?: false`. A broad `rescue` around such a call now catches +`Backpex.ForbiddenError` and `Backpex.NoResultsError` as well, turning a refused +write into an ordinary flash message. That is worse than the crash it replaces: +the request looks handled, and the failure is filed under "something went wrong" +instead of "you may not do this". Reraise them: + +```elixir +def handle(socket, items, _data) do + # ... +rescue + error in [Backpex.ForbiddenError, Backpex.NoResultsError] -> + reraise error, __STACKTRACE__ + + error -> + # your existing error handling +end +``` + +## Checklist for custom actions + +- [ ] Every `delete_all/2` call updated to `delete_all/4`. +- [ ] Every `update_all/3` **and** `update_all/4` call updated to `update_all/5`, + with `event_name:` moved into the options. The arity-3 form raises + `UndefinedFunctionError`, the arity-4 form `FunctionClauseError`. +- [ ] Item actions that write back the items they were handed pass + `authorize?: false` — the gate already covered them. +- [ ] Writes to *other* items pass `authorization_action:` when the default + `:new` / `:edit` / `:delete` is not the right key. +- [ ] Cascade or system writes pass `authorize?: false`. +- [ ] Broad `rescue` clauses around a *still-gated* `Backpex.Resource` call + reraise `Backpex.ForbiddenError` and `Backpex.NoResultsError`. +- [ ] `handle/3` implementations cope with receiving the full selection (they are + no longer handed a pre-filtered list, and are never called with `[]`). diff --git a/lib/backpex/authorization.ex b/lib/backpex/authorization.ex new file mode 100644 index 000000000..687ca16ff --- /dev/null +++ b/lib/backpex/authorization.ex @@ -0,0 +1,103 @@ +defmodule Backpex.Authorization do + @moduledoc """ + Central entry point for all Backpex authorization checks. + + Every check ultimately calls `c:Backpex.LiveResource.can?/3` on the given LiveResource. Routing all + checks through this module gives Backpex a single place to enforce authorization — and a single + place to extend it later (for example with a dedicated authorizer behaviour). + + There are two flavours of functions: + + * **Preflight** (`can?/4`, `can_all?/4`) — answer a question. Use these in the UI to decide whether + to render or disable a control. + * **Gates** (`authorize!/4`, `authorize_all!/4`) — enforce the answer. Use these right before + something actually happens. They raise instead of returning `false`. + + A gate is only as current as the item you hand it. Where the item comes from a cached selection + that may be minutes old — an item action, above all — re-read it first, so the check answers for + the record the write will actually touch. `Backpex.ItemAction.authorize_fresh!/3` pairs + `Backpex.Resource.reload/4` with `authorize_all!/4` for exactly that. + + ## Failure semantics + + * unauthorized → `Backpex.ForbiddenError` + * `nil` item in `authorize_all!/4` (a stale or forged item id) → `Backpex.NoResultsError` + + A `nil` item never reaches `c:Backpex.LiveResource.can?/3` through `authorize_all!/4`. This keeps + user implementations free of `nil` clauses they never asked for, and it does not leak whether an + id exists. + + Both exceptions carry a `plug_status` (403 and 404), but Phoenix LiveView only turns that into an + HTTP status while a view mounts — that is, on the dead render. Raised from `handle_event/3` on a + connected socket, the LiveView process crashes and the client reloads the page: the user sees no + error page and no message. What a gate guarantees is that the operation did not happen, not that + anyone is told why. + + ## Strict semantics + + `can_all?/4` and `authorize_all!/4` are strict: a single unauthorized item makes the whole call + fail. Backpex does not silently drop unauthorized items from a selection. + + Note that `Enum.all?/2` returns `true` for an empty list, so an empty selection passes vacuously. + Callers that need "empty means not allowed" (a disabled bulk action button, for example) must + handle the empty case themselves. + + ## Assigns, not the socket + + Every function here takes `assigns`, not a `%Phoenix.LiveView.Socket{}`. A guard enforces that: + passing the socket by mistake would authorize against the wrong context, and a struct must fail + loudly rather than reach a permissive `c:Backpex.LiveResource.can?/3` clause. + """ + + @doc """ + Returns whether `action` may be performed on `item` for the given LiveResource. + + Pass `nil` as `item` for actions that are not bound to a specific item (`:index`, `:new`, resource + actions). + """ + @spec can?(module(), map(), atom(), map() | nil) :: boolean() + def can?(live_resource, assigns, action, item) + when is_atom(live_resource) and is_map(assigns) and not is_struct(assigns) and is_atom(action) do + live_resource.can?(assigns, action, item) + end + + @doc """ + Returns whether `action` may be performed on **every** item in `items`. + + Returns `true` for an empty list. + """ + @spec can_all?(module(), map(), atom(), list()) :: boolean() + def can_all?(live_resource, assigns, action, items) when is_list(items) do + Enum.all?(items, &can?(live_resource, assigns, action, &1)) + end + + @doc """ + Ensures `action` may be performed on `item`, raising `Backpex.ForbiddenError` otherwise. + + Returns `:ok`. + """ + @spec authorize!(module(), map(), atom(), map() | nil) :: :ok + def authorize!(live_resource, assigns, action, item) do + if can?(live_resource, assigns, action, item) do + :ok + else + raise Backpex.ForbiddenError + end + end + + @doc """ + Ensures `action` may be performed on **every** item in `items`. + + Raises `Backpex.ForbiddenError` when any item is not authorized and `Backpex.NoResultsError` when + the list contains `nil` (a stale or forged item id). + + Returns `:ok`. An empty list passes. + """ + @spec authorize_all!(module(), map(), atom(), list()) :: :ok + def authorize_all!(live_resource, assigns, action, items) when is_list(items) do + Enum.each(items, fn + nil -> raise Backpex.NoResultsError + item -> authorize!(live_resource, assigns, action, item) + end) + end +end diff --git a/lib/backpex/field.ex b/lib/backpex/field.ex index b73072605..b8f7cd54e 100644 --- a/lib/backpex/field.ex +++ b/lib/backpex/field.ex @@ -424,10 +424,9 @@ defmodule Backpex.Field do def handle_index_editable(socket, value, change) do %{assigns: %{item: item, fields: fields, live_resource: live_resource} = assigns} = socket - if not live_resource.can?(assigns, :edit, item) do - raise Backpex.ForbiddenError - end - + # No `can?/3` check here: `Backpex.Resource.update/6` enforces `:edit` with the same assigns and + # item, before the changeset runs. Checking here as well would evaluate user code twice per + # inline edit for no added protection. opts = [ after_save_fun: fn item -> live_resource.on_item_updated(socket, item) diff --git a/lib/backpex/fields/belongs_to.ex b/lib/backpex/fields/belongs_to.ex index 2dba75eb1..79381d9ef 100644 --- a/lib/backpex/fields/belongs_to.ex +++ b/lib/backpex/fields/belongs_to.ex @@ -62,6 +62,7 @@ defmodule Backpex.Fields.BelongsTo do """ use Backpex.Field, config_schema: @config_schema import Ecto.Query + alias Backpex.Authorization alias Backpex.Router @impl Phoenix.LiveComponent @@ -238,7 +239,7 @@ defmodule Backpex.Fields.BelongsTo do live_resource = Map.get(field_options, :live_resource) link = - if live_resource && live_resource.can?(assigns, :show, value) do + if live_resource && Authorization.can?(live_resource, assigns, :show, value) do Router.get_path(socket, live_resource, params, :show, value) end diff --git a/lib/backpex/fields/has_many.ex b/lib/backpex/fields/has_many.ex index f4b815ad8..22c0e5279 100644 --- a/lib/backpex/fields/has_many.ex +++ b/lib/backpex/fields/has_many.ex @@ -81,6 +81,7 @@ defmodule Backpex.Fields.HasMany do import Ecto.Query alias Backpex.Adapters.Ecto, as: EctoAdapter + alias Backpex.Authorization alias Backpex.HTML.Form alias Backpex.Router @@ -456,7 +457,7 @@ defmodule Backpex.Fields.HasMany do } = assigns link = - if link_assocs and field_options.live_resource.can?(assigns, :show, item) do + if link_assocs and Authorization.can?(field_options.live_resource, assigns, :show, item) do Router.get_path(socket, Map.get(field_options, :live_resource), params, :show, item) end diff --git a/lib/backpex/fields/inline_crud.ex b/lib/backpex/fields/inline_crud.ex index 9f9918df5..506b3c0f9 100644 --- a/lib/backpex/fields/inline_crud.ex +++ b/lib/backpex/fields/inline_crud.ex @@ -92,6 +92,7 @@ defmodule Backpex.Fields.InlineCRUD do """ use Backpex.Field, config_schema: @config_schema + alias Backpex.Authorization alias Backpex.Router require Backpex @@ -261,7 +262,7 @@ defmodule Backpex.Fields.InlineCRUD do defp get_link(assigns, row) do live_resource = Map.get(assigns.field_options, :live_resource) - if live_resource && live_resource.can?(assigns, :show, row) do + if live_resource && Authorization.can?(live_resource, assigns, :show, row) do Router.get_path(assigns.socket, live_resource, assigns.params, :show, row) end end diff --git a/lib/backpex/html/resource.ex b/lib/backpex/html/resource.ex index 85cff7093..e14da7852 100644 --- a/lib/backpex/html/resource.ex +++ b/lib/backpex/html/resource.ex @@ -9,6 +9,7 @@ defmodule Backpex.HTML.Resource do import Backpex.HTML.Layout import Phoenix.LiveView.TagEngine + alias Backpex.Authorization alias Backpex.LiveResource alias Backpex.ResourceAction alias Backpex.Router @@ -105,7 +106,7 @@ defmodule Backpex.HTML.Resource do {_name, field_options} = field = Enum.find(fields, fn {field_name, _field_options} -> field_name == name end) readonly = - not live_resource.can?(assigns, :edit, item) or + not Authorization.can?(live_resource, assigns, :edit, item) or Backpex.Field.readonly?(field_options, assigns) assigns = @@ -875,7 +876,10 @@ defmodule Backpex.HTML.Resource do def resource_buttons(assigns) do ~H"""