diff --git a/docs/development-progress-log.md b/docs/development-progress-log.md index 84687d1..5d777ff 100644 --- a/docs/development-progress-log.md +++ b/docs/development-progress-log.md @@ -797,8 +797,8 @@ end **Subtask 1 – JoinRequest resource and public policies (done):** - Resource: `Mv.Membership.JoinRequest` with attributes (status, email, first_name, last_name, form_data, schema_version, confirmation_token_hash, confirmation_token_expires_at, submitted_at, etc.), actions `submit` (create), `get_by_confirmation_token_hash` (read), `confirm` (update). Migration: `20260309141437_add_join_requests.exs`. - Policies: Public actions allowed with `actor: nil` via `Mv.Authorization.Checks.ActorIsNil` (submit, get_by_confirmation_token_hash, confirm); default read remains Forbidden for unauthenticated. -- Domain: `Mv.Membership.submit_join_request/2`, `Mv.Membership.confirm_join_request/2` (token hashing and lookup in domain). -- Test file: `test/membership/join_request_test.exs` – all tests pass; policy test (read with actor nil → Forbidden) unskipped. Expired-token test still skipped (fixture for expired token to be added in Subtask 2 or later). +- Domain: `Mv.Membership.submit_join_request/2`, `Mv.Membership.confirm_join_request/2` (token hashing via `JoinRequest.hash_confirmation_token/1`, lookup, expiry check, idempotency for :submitted/:approved/:rejected). +- Test file: `test/membership/join_request_test.exs` – all tests pass; policy test and expired-token test implemented. ### Test Data Management diff --git a/lib/membership/join_request.ex b/lib/membership/join_request.ex index d42aa1e..2519089 100644 --- a/lib/membership/join_request.ex +++ b/lib/membership/join_request.ex @@ -80,6 +80,7 @@ defmodule Mv.Membership.JoinRequest do validate present(:email), on: [:create] end + # Attributes are backend-internal for now; set public? true when exposing via AshJsonApi/AshGraphql attributes do uuid_primary_key :id @@ -130,4 +131,17 @@ defmodule Mv.Membership.JoinRequest do create_timestamp :inserted_at update_timestamp :updated_at end + + # Public helpers (used by SetConfirmationToken change and domain confirm_join_request) + + @doc """ + Returns the SHA256 hash of the confirmation token (lowercase hex). + + Used when creating a join request (submit) and when confirming by token. + Only one implementation ensures algorithm changes stay in sync. + """ + @spec hash_confirmation_token(String.t()) :: String.t() + def hash_confirmation_token(token) when is_binary(token) do + :crypto.hash(:sha256, token) |> Base.encode16(case: :lower) + end end diff --git a/lib/membership/join_request/changes/confirm_request.ex b/lib/membership/join_request/changes/confirm_request.ex index 477561f..a9ff047 100644 --- a/lib/membership/join_request/changes/confirm_request.ex +++ b/lib/membership/join_request/changes/confirm_request.ex @@ -3,15 +3,23 @@ defmodule Mv.Membership.JoinRequest.Changes.ConfirmRequest do Sets the join request to submitted (confirmation link clicked). Used by the confirm action after the user clicks the confirmation link. - Token hash is kept so that a second click (idempotent) can still find the record - and return success without changing it. + Only applies when the current status is `:pending_confirmation`, so that + direct calls to the confirm action are idempotent and never overwrite + :submitted, :approved, or :rejected. Token hash is kept so a second click + can still find the record and return success without changing it. """ use Ash.Resource.Change @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t() def change(changeset, _opts, _context) do - changeset - |> Ash.Changeset.force_change_attribute(:status, :submitted) - |> Ash.Changeset.force_change_attribute(:submitted_at, DateTime.utc_now()) + current_status = Ash.Changeset.get_data(changeset, :status) + + if current_status == :pending_confirmation do + changeset + |> Ash.Changeset.force_change_attribute(:status, :submitted) + |> Ash.Changeset.force_change_attribute(:submitted_at, DateTime.utc_now()) + else + changeset + end end end diff --git a/lib/membership/join_request/changes/set_confirmation_token.ex b/lib/membership/join_request/changes/set_confirmation_token.ex index b052799..cce7b3a 100644 --- a/lib/membership/join_request/changes/set_confirmation_token.ex +++ b/lib/membership/join_request/changes/set_confirmation_token.ex @@ -2,11 +2,15 @@ defmodule Mv.Membership.JoinRequest.Changes.SetConfirmationToken do @moduledoc """ Hashes the confirmation token and sets expiry for the join request (submit flow). + Uses `JoinRequest.hash_confirmation_token/1` so hashing logic lives in one place. + Reads the :confirmation_token argument, stores only its SHA256 hash and sets confirmation_token_expires_at (e.g. 24h). Raw token is never persisted. """ use Ash.Resource.Change + alias Mv.Membership.JoinRequest + @confirmation_validity_hours 24 @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t() @@ -14,7 +18,7 @@ defmodule Mv.Membership.JoinRequest.Changes.SetConfirmationToken do token = Ash.Changeset.get_argument(changeset, :confirmation_token) if is_binary(token) and token != "" do - hash = token_hash(token) + hash = JoinRequest.hash_confirmation_token(token) expires_at = DateTime.utc_now() |> DateTime.add(@confirmation_validity_hours, :hour) changeset @@ -25,8 +29,4 @@ defmodule Mv.Membership.JoinRequest.Changes.SetConfirmationToken do changeset end end - - defp token_hash(token) do - :crypto.hash(:sha256, token) |> Base.encode16(case: :lower) - end end diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index 66e2f9b..d967b38 100644 --- a/lib/membership/membership.ex +++ b/lib/membership/membership.ex @@ -353,19 +353,20 @@ defmodule Mv.Membership do @doc """ Confirms a join request by token (public confirmation link). - Hashes the token, finds the JoinRequest by confirmation_token_hash, then updates - to status :submitted and invalidates the token. Idempotent: if already submitted, - returns the existing record without changing it. + Hashes the token, finds the JoinRequest by confirmation_token_hash, checks that + the token has not expired, then updates to status :submitted. Idempotent: if + already submitted, approved, or rejected, returns the existing record without changing it. ## Options - `:actor` - Must be nil for public confirm (policy allows only unauthenticated). ## Returns - - `{:ok, request}` - Updated or already-submitted JoinRequest + - `{:ok, request}` - Updated or already-processed JoinRequest + - `{:error, :token_expired}` - Token was found but confirmation_token_expires_at is in the past - `{:error, error}` - Token unknown/invalid or authorization error """ def confirm_join_request(token, opts \\ []) when is_binary(token) do - hash = confirmation_token_hash(token) + hash = JoinRequest.hash_confirmation_token(token) actor = Keyword.get(opts, :actor) query = @@ -378,20 +379,28 @@ defmodule Mv.Membership do {:error, NotFoundError.exception(resource: JoinRequest)} {:ok, request} -> - if request.status == :submitted do - {:ok, request} - else - request - |> Ash.Changeset.for_update(:confirm, %{}, domain: __MODULE__) - |> Ash.update(domain: __MODULE__, actor: actor) - end + do_confirm_request(request, actor) {:error, error} -> {:error, error} end end - defp confirmation_token_hash(token) do - :crypto.hash(:sha256, token) |> Base.encode16(case: :lower) + defp do_confirm_request(request, _actor) + when request.status in [:submitted, :approved, :rejected] do + {:ok, request} end + + defp do_confirm_request(request, actor) do + if expired?(request.confirmation_token_expires_at) do + {:error, :token_expired} + else + request + |> Ash.Changeset.for_update(:confirm, %{}, domain: __MODULE__) + |> Ash.update(domain: __MODULE__, actor: actor) + end + end + + defp expired?(nil), do: true + defp expired?(expires_at), do: DateTime.compare(expires_at, DateTime.utc_now()) == :lt end diff --git a/priv/repo/migrations/20260309141437_add_join_requests.exs b/priv/repo/migrations/20260309141437_add_join_requests.exs index e4921ca..966e4ea 100644 --- a/priv/repo/migrations/20260309141437_add_join_requests.exs +++ b/priv/repo/migrations/20260309141437_add_join_requests.exs @@ -9,6 +9,7 @@ defmodule Mv.Repo.Migrations.AddJoinRequests do use Ecto.Migration def up do + # uuid_generate_v7() is provided by initialize_extensions migration (custom function) create table(:join_requests, primary_key: false) do add :id, :uuid, null: false, default: fragment("uuid_generate_v7()"), primary_key: true diff --git a/test/membership/join_request_test.exs b/test/membership/join_request_test.exs index 2123730..f40c9ec 100644 --- a/test/membership/join_request_test.exs +++ b/test/membership/join_request_test.exs @@ -98,8 +98,20 @@ defmodule Mv.Membership.JoinRequestTest do assert {:error, _} = Membership.confirm_join_request("nonexistent-token", actor: nil) end - @tag :skip - test "returns error when token is expired (requires fixture for expired token)" + test "returns error when token is expired" do + token = "expired-token-#{System.unique_integer([:positive])}" + attrs = Map.put(@valid_submit_attrs, :confirmation_token, token) + + {:ok, request} = Membership.submit_join_request(attrs, actor: nil) + past = DateTime.add(DateTime.utc_now(), -1, :hour) + id_binary = Ecto.UUID.dump!(request.id) + + from(j in "join_requests", where: fragment("id = ?", ^id_binary)) + |> Repo.update_all(set: [confirmation_token_expires_at: past]) + + assert {:error, :token_expired} = + Membership.confirm_join_request(token, actor: nil) + end end describe "policies (actor: nil)" do