refactor: apply review notes

lib/membership/join_request.ex

@@ -12,16 +12,25 @@ defmodule Mv.Membership.JoinRequest do
     data_layer: AshPostgres.DataLayer,
     authorizers: [Ash.Policy.Authorizer]
 
+  alias Ash.Policy.Check.Builtins, as: AshBuiltins
+
   postgres do
     table "join_requests"
     repo Mv.Repo
   end
 
   actions do
-    defaults [:read, :destroy]
+    defaults [:destroy]
+
+    # Admin: list and get by id (used with HasPermission)
+    read :admin_read do
+      description "List and get JoinRequests; requires permission (e.g. admin / normal_user)"
+      primary? true
+    end
 
     create :create do
       primary? true
+
       accept [
         :email,
         :confirmation_token_hash,
@@ -38,15 +47,9 @@ defmodule Mv.Membership.JoinRequest do
 
     create :confirm do
       description "Public action: create JoinRequest after confirmation link click (actor: nil)"
-      accept [
-        :email,
-        :confirmation_token_hash,
-        :status,
-        :submitted_at,
-        :source,
-        :schema_version,
-        :payload
-      ]
+      accept [:email, :confirmation_token_hash, :payload]
+
+      change Mv.Membership.JoinRequest.Changes.SetConfirmServerMetadata
     end
 
     update :update do
@@ -58,12 +61,11 @@ defmodule Mv.Membership.JoinRequest do
   policies do
     policy action(:confirm) do
       description "Allow public confirmation (actor nil) for join flow"
-      authorize_if Ash.Policy.Check.Builtins.actor_absent()
+      authorize_if AshBuiltins.actor_absent()
     end
 
-    policy action_type(:read) do
-      description "Allow read when actor nil (success page) or when user has permission"
-      authorize_if Ash.Policy.Check.Builtins.actor_absent()
+    policy action(:admin_read) do
+      description "List/get JoinRequests only with permission (admin, later normal_user)"
       authorize_if Mv.Authorization.Checks.HasPermission
     end
 

lib/membership/join_request/changes/set_confirm_server_metadata.ex

@@ -0,0 +1,18 @@
+defmodule Mv.Membership.JoinRequest.Changes.SetConfirmServerMetadata do
+  @moduledoc """
+  Ash Change that sets server-side metadata for the public :confirm action.
+
+  Client may only send :email, :confirmation_token_hash, :payload (concept §2.3.2).
+  This change sets: status, submitted_at, source, schema_version so they cannot be forged.
+  """
+  use Ash.Resource.Change
+
+  @impl true
+  def change(changeset, _opts, _context) do
+    changeset
+    |> Ash.Changeset.force_change_attribute(:status, "submitted")
+    |> Ash.Changeset.force_change_attribute(:submitted_at, DateTime.utc_now())
+    |> Ash.Changeset.force_change_attribute(:source, "public_join")
+    |> Ash.Changeset.force_change_attribute(:schema_version, 1)
+  end
+end

lib/membership/membership.ex

@@ -80,30 +80,30 @@ defmodule Mv.Membership do
     end
 
     resource Mv.Membership.JoinRequest do
-      define :list_join_requests, action: :read
-      define :get_join_request, action: :read, get_by: [:id]
+      define :list_join_requests, action: :admin_read
+      define :get_join_request, action: :admin_read, get_by: [:id]
       define :update_join_request, action: :update
       define :destroy_join_request, action: :destroy
     end
   end
 
-  # Idempotent confirm: implemented in code so duplicate token returns {:ok, existing} (concept §2.3.2)
+  # Idempotent confirm: duplicate token hits unique constraint -> return {:ok, nil} (no public read)
   @doc """
   Creates a JoinRequest after confirmation link click (public action with actor: nil).
 
   Idempotent: if a JoinRequest with the same `confirmation_token_hash` already exists,
-  returns `{:ok, existing}` instead of creating a duplicate (per concept §2.3.2).
+  returns `{:ok, nil}` (no record returned; no public read for security).
   """
   def confirm_join_request(attrs, opts \\ []) do
-    hash = attrs[:confirmation_token_hash] || attrs["confirmation_token_hash"]
+    case do_confirm_join_request(attrs, opts) do
+      {:ok, request} ->
+        {:ok, request}
 
-    if hash do
-      case get_join_request_by_confirmation_token_hash!(hash, opts) do
-        nil -> do_confirm_join_request(attrs, opts)
-        existing -> {:ok, existing}
-      end
-    else
-      do_confirm_join_request(attrs, opts)
+      {:error, %Ash.Error.Invalid{errors: errors}} = error ->
+        if unique_confirmation_token_violation?(errors), do: {:ok, nil}, else: error
+
+      other ->
+        other
     end
   end
 
@@ -113,17 +113,12 @@ defmodule Mv.Membership do
     |> Ash.create(Keyword.put(opts, :domain, __MODULE__))
   end
 
-  defp get_join_request_by_confirmation_token_hash!(hash, opts) do
-    opts = Keyword.put(opts, :domain, __MODULE__)
-
-    Mv.Membership.JoinRequest
-    |> Ash.Query.filter(confirmation_token_hash == ^hash)
-    |> Ash.read_one(opts)
-    |> case do
-      {:ok, %Mv.Membership.JoinRequest{} = existing} -> existing
-      {:ok, nil} -> nil
-      _ -> nil
-    end
+  defp unique_confirmation_token_violation?(errors) do
+    Enum.any?(errors, fn err ->
+      Map.get(err, :field) == :confirmation_token_hash or
+        ((pv = Map.get(err, :private_vars)) &&
+           (is_list(pv) and Keyword.get(pv, :constraint_type) == :unique))
+    end)
   end
 
   # Singleton pattern: Get the single settings record

lib/mv/authorization/permission_sets.ex

@@ -269,6 +269,7 @@ defmodule Mv.Authorization.PermissionSets do
           perm_all("Role") ++
           perm_all("Group") ++
           member_group_perms ++
+          perm_all("JoinRequest") ++
           perm_all("MembershipFeeType") ++
           perm_all("MembershipFeeCycle"),
       pages: [

priv/gettext/de/LC_MESSAGES/default.po

@@ -2608,18 +2608,3 @@ msgstr "Import"
 #, elixir-autogen, elixir-format
 msgid "Value type cannot be changed after creation"
 msgstr "Der Wertetyp kann nach dem Erstellen nicht mehr geändert werden."
-
-#~ #: lib/mv_web/live/import_export_live.ex
-#~ #, elixir-autogen, elixir-format, fuzzy
-#~ msgid "Export Members (CSV)"
-#~ msgstr "Mitglieder exportieren (CSV)"
-
-#~ #: lib/mv_web/live/import_export_live.ex
-#~ #, elixir-autogen, elixir-format
-#~ msgid "Export functionality will be available in a future release."
-#~ msgstr "Export-Funktionalität ist im nächsten release verfügbar."
-
-#~ #: lib/mv_web/live/import_export_live.ex
-#~ #, elixir-autogen, elixir-format
-#~ msgid "Import members from CSV files or export member data."
-#~ msgstr "Importiere Mitglieder aus CSV-Dateien oder exportiere Mitgliederdaten."

priv/gettext/en/LC_MESSAGES/default.po

@@ -2609,18 +2609,3 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "Value type cannot be changed after creation"
 msgstr ""
-
-#~ #: lib/mv_web/live/import_export_live.ex
-#~ #, elixir-autogen, elixir-format, fuzzy
-#~ msgid "Export Members (CSV)"
-#~ msgstr ""
-
-#~ #: lib/mv_web/live/import_export_live.ex
-#~ #, elixir-autogen, elixir-format
-#~ msgid "Export functionality will be available in a future release."
-#~ msgstr ""
-
-#~ #: lib/mv_web/live/import_export_live.ex
-#~ #, elixir-autogen, elixir-format
-#~ msgid "Import members from CSV files or export member data."
-#~ msgstr ""

priv/repo/migrations/20260220120000_add_join_requests.exs

@@ -34,8 +34,8 @@ defmodule Mv.Repo.Migrations.AddJoinRequests do
 
   def down do
     drop_if_exists unique_index(:join_requests, [:confirmation_token_hash],
-                  name: "join_requests_unique_confirmation_token_hash_index"
-                )
+                     name: "join_requests_unique_confirmation_token_hash_index"
+                   )
 
     drop table(:join_requests)
   end

priv/repo/migrations/20260220120001_alter_join_requests_schema_version_to_integer.exs

@@ -0,0 +1,18 @@
+defmodule Mv.Repo.Migrations.AlterJoinRequestsSchemaVersionToInteger do
+  @moduledoc """
+  Aligns schema_version with Ash attribute type :integer (concept review).
+  """
+  use Ecto.Migration
+
+  def up do
+    alter table(:join_requests) do
+      modify :schema_version, :integer, null: false
+    end
+  end
+
+  def down do
+    alter table(:join_requests) do
+      modify :schema_version, :bigint, null: false
+    end
+  end
+end

test/mv/membership/join_request_test.exs

@@ -7,21 +7,20 @@ defmodule Mv.Membership.JoinRequestTest do
   """
   use Mv.DataCase, async: false
 
+  alias Mv.Helpers.SystemActor
   alias Mv.Membership
   alias Mv.Membership.JoinRequest
 
   require Ash.Query
 
-  # Minimal valid attributes for the public :confirm action (per concept §2.3.2)
+  # Client-only attributes for :confirm (server sets status, submitted_at, source, schema_version)
   defp valid_confirm_attrs(opts \\ []) do
-    token = Keyword.get(opts, :confirmation_token_hash, "hash_#{System.unique_integer([:positive])}")
+    token =
+      Keyword.get(opts, :confirmation_token_hash, "hash_#{System.unique_integer([:positive])}")
+
     [
       email: "join_#{System.unique_integer([:positive])}@example.com",
       confirmation_token_hash: token,
-      status: "submitted",
-      submitted_at: DateTime.utc_now(),
-      source: "public_join",
-      schema_version: 1,
       payload: %{}
     ]
     |> Enum.into(%{})
@@ -39,19 +38,36 @@ defmodule Mv.Membership.JoinRequestTest do
       assert request.source == "public_join"
     end
 
-    test "read with actor nil succeeds for created join request" do
+    test "no public read: actor nil cannot read JoinRequest (by id or list)" do
       attrs = valid_confirm_attrs()
       {:ok, created} = Membership.confirm_join_request(attrs, actor: nil)
 
-      assert {:ok, %JoinRequest{} = read} =
-               Ash.get(JoinRequest, created.id, actor: nil, domain: Mv.Membership)
+      get_result = Ash.get(JoinRequest, created.id, actor: nil, domain: Mv.Membership)
 
-      assert read.id == created.id
-      assert read.email == created.email
+      assert match?({:error, %Ash.Error.Forbidden{}}, get_result) or
+               match?(
+                 {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Query.NotFound{}]}},
+                 get_result
+               )
+
+      list_result = JoinRequest |> Ash.read(actor: nil, domain: Mv.Membership)
+
+      assert match?({:error, %Ash.Error.Forbidden{}}, list_result) or
+               match?({:error, %Ash.Error.Invalid{}}, list_result) or
+               list_result == {:ok, []},
+             "actor nil must not see any JoinRequests: got #{inspect(list_result)}"
     end
 
     test "generic create with actor nil is forbidden" do
-      attrs = valid_confirm_attrs()
+      # Use full attrs required by :create so the only failure is policy, not validation
+      attrs =
+        valid_confirm_attrs()
+        |> Map.merge(%{
+          status: "submitted",
+          submitted_at: DateTime.utc_now(),
+          source: "public_join",
+          schema_version: 1
+        })
 
       assert {:error, %Ash.Error.Forbidden{errors: [%Ash.Error.Forbidden.Policy{}]}} =
                JoinRequest
@@ -62,25 +78,23 @@ defmodule Mv.Membership.JoinRequestTest do
 
   describe "Idempotency (confirmation_token_hash)" do
     test "second create with same confirmation_token_hash does not create duplicate" do
-      system_actor = Mv.Helpers.SystemActor.get_system_actor()
+      system_actor = SystemActor.get_system_actor()
       token = "idempotent_token_#{System.unique_integer([:positive])}"
       attrs1 = valid_confirm_attrs(confirmation_token_hash: token)
       attrs2 = valid_confirm_attrs(confirmation_token_hash: token)
       attrs2 = %{attrs2 | email: "other_#{System.unique_integer([:positive])}@example.com"}
 
-      assert {:ok, first} = Membership.confirm_join_request(attrs1, actor: nil)
+      assert {:ok, _first} = Membership.confirm_join_request(attrs1, actor: nil)
 
-      # Second call with same token: idempotent return {:ok, existing} (concept §2.3.2)
-      assert {:ok, second} = Membership.confirm_join_request(attrs2, actor: nil)
-      assert second.id == first.id, "idempotent confirm must return the existing record"
+      # Second call with same token: idempotent return {:ok, nil} (no public read)
+      assert {:ok, nil} = Membership.confirm_join_request(attrs2, actor: nil)
 
-      count =
-        JoinRequest
-        |> Ash.Query.filter(confirmation_token_hash == ^token)
-        |> Ash.read!(actor: system_actor, domain: Mv.Membership, authorize?: false)
-        |> length()
+      # Count via allowed admin read (no authorize?: false)
+      assert {:ok, list} = Membership.list_join_requests(actor: system_actor)
+      count = Enum.count(list, &(&1.confirmation_token_hash == token))
 
-      assert count == 1, "expected exactly one JoinRequest with this confirmation_token_hash, got #{count}"
+      assert count == 1,
+             "expected exactly one JoinRequest with this confirmation_token_hash, got #{count}"
     end
   end