From b41f005d9ec00ff5c8544eb61438ef6de476b6b2 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 20 Feb 2026 18:24:20 +0100 Subject: [PATCH] refactor: apply review notes --- lib/membership/join_request.ex | 30 +++++----- .../changes/set_confirm_server_metadata.ex | 18 ++++++ lib/membership/membership.ex | 41 ++++++------- lib/mv/authorization/permission_sets.ex | 1 + priv/gettext/de/LC_MESSAGES/default.po | 15 ----- priv/gettext/en/LC_MESSAGES/default.po | 15 ----- .../20260220120000_add_join_requests.exs | 4 +- ...oin_requests_schema_version_to_integer.exs | 18 ++++++ test/mv/membership/join_request_test.exs | 60 ++++++++++++------- 9 files changed, 110 insertions(+), 92 deletions(-) create mode 100644 lib/membership/join_request/changes/set_confirm_server_metadata.ex create mode 100644 priv/repo/migrations/20260220120001_alter_join_requests_schema_version_to_integer.exs diff --git a/lib/membership/join_request.ex b/lib/membership/join_request.ex index 466840b..ce67820 100644 --- a/lib/membership/join_request.ex +++ b/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 diff --git a/lib/membership/join_request/changes/set_confirm_server_metadata.ex b/lib/membership/join_request/changes/set_confirm_server_metadata.ex new file mode 100644 index 0000000..23c9435 --- /dev/null +++ b/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 diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index 69a8110..2e8d082 100644 --- a/lib/membership/membership.ex +++ b/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 diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index fffc818..2686884 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/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: [ diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 0d661cf..c994491 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/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." diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 371a028..1cef9be 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/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 "" diff --git a/priv/repo/migrations/20260220120000_add_join_requests.exs b/priv/repo/migrations/20260220120000_add_join_requests.exs index 00f764d..7894ad9 100644 --- a/priv/repo/migrations/20260220120000_add_join_requests.exs +++ b/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 diff --git a/priv/repo/migrations/20260220120001_alter_join_requests_schema_version_to_integer.exs b/priv/repo/migrations/20260220120001_alter_join_requests_schema_version_to_integer.exs new file mode 100644 index 0000000..9e03173 --- /dev/null +++ b/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 diff --git a/test/mv/membership/join_request_test.exs b/test/mv/membership/join_request_test.exs index fa81231..19ab704 100644 --- a/test/mv/membership/join_request_test.exs +++ b/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