From dbd0a572920537406d7bb8a11e59ba1ff8f61b10 Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Wed, 4 Feb 2026 09:19:37 +0100
Subject: [PATCH] Secure regenerate_cycles: require can?(:create,
 MembershipFeeCycle) in handler

- Handler returns flash error when non-admin triggers event (e.g. DevTools).
- Test: read_only cannot create MembershipFeeCycle so handler rejects.
---
 .../show/membership_fees_component.ex         | 75 +++++++++++--------
 .../member_live/show_membership_fees_test.exs | 13 ++++
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/lib/mv_web/live/member_live/show/membership_fees_component.ex b/lib/mv_web/live/member_live/show/membership_fees_component.ex
index d074ffa..34495ae 100644
--- a/lib/mv_web/live/member_live/show/membership_fees_component.ex
+++ b/lib/mv_web/live/member_live/show/membership_fees_component.ex
@@ -568,45 +568,54 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do
   end
 
   def handle_event("regenerate_cycles", _params, socket) do
-    # Button is only shown when can_create_cycle (normal_user and admin). Cycle generation uses system actor.
-    socket = assign(socket, :regenerating, true)
-    member = socket.assigns.member
+    # Server-side authorization: do not rely on UI hiding the button (e.g. read_only could trigger via DevTools).
+    actor = current_actor(socket)
 
-    case CycleGenerator.generate_cycles_for_member(member.id) do
-      {:ok, _new_cycles, _notifications} ->
-        actor = current_actor(socket)
+    if can?(actor, :create, MembershipFeeCycle) do
+      socket = assign(socket, :regenerating, true)
+      member = socket.assigns.member
 
-        updated_member =
-          member
-          |> Ash.load!(
-            [
-              :membership_fee_type,
-              membership_fee_cycles: [:membership_fee_type]
-            ],
-            actor: actor
-          )
+      case CycleGenerator.generate_cycles_for_member(member.id) do
+        {:ok, _new_cycles, _notifications} ->
+          actor = current_actor(socket)
 
-        cycles =
-          Enum.sort_by(
-            updated_member.membership_fee_cycles || [],
-            & &1.cycle_start,
-            {:desc, Date}
-          )
+          updated_member =
+            member
+            |> Ash.load!(
+              [
+                :membership_fee_type,
+                membership_fee_cycles: [:membership_fee_type]
+              ],
+              actor: actor
+            )
 
-        send(self(), {:member_updated, updated_member})
+          cycles =
+            Enum.sort_by(
+              updated_member.membership_fee_cycles || [],
+              & &1.cycle_start,
+              {:desc, Date}
+            )
 
-        {:noreply,
-         socket
-         |> assign(:member, updated_member)
-         |> assign(:cycles, cycles)
-         |> assign(:regenerating, false)
-         |> put_flash(:info, gettext("Cycles regenerated successfully"))}
+          send(self(), {:member_updated, updated_member})
 
-      {:error, error} ->
-        {:noreply,
-         socket
-         |> assign(:regenerating, false)
-         |> put_flash(:error, format_error(error))}
+          {:noreply,
+           socket
+           |> assign(:member, updated_member)
+           |> assign(:cycles, cycles)
+           |> assign(:regenerating, false)
+           |> put_flash(:info, gettext("Cycles regenerated successfully"))}
+
+        {:error, error} ->
+          {:noreply,
+           socket
+           |> assign(:regenerating, false)
+           |> put_flash(:error, format_error(error))}
+      end
+    else
+      {:noreply,
+       socket
+       |> assign(:regenerating, false)
+       |> put_flash(:error, format_error(%Ash.Error.Forbidden{}))}
     end
   end
 
diff --git a/test/mv_web/member_live/show_membership_fees_test.exs b/test/mv_web/member_live/show_membership_fees_test.exs
index 331780f..1f447b8 100644
--- a/test/mv_web/member_live/show_membership_fees_test.exs
+++ b/test/mv_web/member_live/show_membership_fees_test.exs
@@ -320,6 +320,19 @@ defmodule MvWeb.MemberLive.ShowMembershipFeesTest do
     end
   end
 
+  describe "read_only cannot trigger regenerate_cycles (handler enforces can?)" do
+    @tag role: :read_only
+    test "read_only cannot create MembershipFeeCycle so regenerate_cycles handler would show flash error",
+         %{current_user: read_only_user} do
+      # The regenerate_cycles handler checks can?(actor, :create, MembershipFeeCycle) before
+      # calling the generator. If a read_only user triggered the event (e.g. via DevTools),
+      # the handler returns flash error and no new cycles are created.
+      # This test verifies the condition the handler uses.
+      refute MvWeb.Authorization.can?(read_only_user, :create, MembershipFeeCycle),
+             "read_only must not be allowed to create MembershipFeeCycle so handler rejects regenerate_cycles"
+    end
+  end
+
   describe "confirm_delete_all_cycles handler (policy enforced)" do
     @tag role: :admin
     test "admin can delete all cycles via UI and cycles are removed", %{conn: conn} do