Merge branch 'main' into feature/330_import_service_skeleton

lib/mv/authorization/checks/has_permission.ex

@@ -21,8 +21,8 @@ defmodule Mv.Authorization.Checks.HasPermission do
   - **:all** - Authorizes without filtering (returns all records)
   - **:own** - Filters to records where record.id == actor.id
   - **:linked** - Filters based on resource type:
-    - Member: member.user.id == actor.id (via has_one :user relationship)
-    - CustomFieldValue: custom_field_value.member.user.id == actor.id (traverses member → user relationship!)
+    - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship)
+    - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id)
 
   ## Error Handling
 
@@ -59,6 +59,7 @@ defmodule Mv.Authorization.Checks.HasPermission do
   def strict_check(actor, authorizer, _opts) do
     resource = authorizer.resource
     action = get_action_from_authorizer(authorizer)
+    record = get_record_from_authorizer(authorizer)
 
     cond do
       is_nil(actor) ->
@@ -76,12 +77,12 @@ defmodule Mv.Authorization.Checks.HasPermission do
         {:ok, false}
 
       true ->
-        strict_check_with_permissions(actor, resource, action)
+        strict_check_with_permissions(actor, resource, action, record)
     end
   end
 
   # Helper function to reduce nesting depth
-  defp strict_check_with_permissions(actor, resource, action) do
+  defp strict_check_with_permissions(actor, resource, action, record) do
     with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- actor,
          {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name),
          permissions <- PermissionSets.get_permissions(ps_atom),
@@ -93,9 +94,15 @@ defmodule Mv.Authorization.Checks.HasPermission do
              actor,
              resource_name
            ) do
-        :authorized -> {:ok, true}
-        {:filter, _} -> {:ok, :unknown}
-        false -> {:ok, false}
+        :authorized ->
+          {:ok, true}
+
+        {:filter, filter_expr} ->
+          # For strict_check on single records, evaluate the filter against the record
+          evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name)
+
+        false ->
+          {:ok, false}
       end
     else
       %{role: nil} ->
@@ -122,9 +129,17 @@ defmodule Mv.Authorization.Checks.HasPermission do
     action = get_action_from_authorizer(authorizer)
 
     cond do
-      is_nil(actor) -> nil
-      is_nil(action) -> nil
-      true -> auto_filter_with_permissions(actor, resource, action)
+      is_nil(actor) ->
+        # No actor - deny access (fail-closed)
+        # Return filter that never matches (expr(false) = match none)
+        deny_filter()
+
+      is_nil(action) ->
+        # Cannot determine action - deny access (fail-closed)
+        deny_filter()
+
+      true ->
+        auto_filter_with_permissions(actor, resource, action)
     end
   end
 
@@ -141,21 +156,97 @@ defmodule Mv.Authorization.Checks.HasPermission do
              actor,
              resource_name
            ) do
-        :authorized -> nil
-        {:filter, filter_expr} -> filter_expr
-        false -> nil
+        :authorized ->
+          # :all scope - allow all records (no filter)
+          # Return empty keyword list (no filtering)
+          []
+
+        {:filter, filter_expr} ->
+          # :linked or :own scope - apply filter
+          # filter_expr is a keyword list from expr(...), return it directly
+          filter_expr
+
+        false ->
+          # No permission - deny access (fail-closed)
+          deny_filter()
       end
     else
+      _ ->
+        # Error case (no role, invalid permission set, etc.) - deny access (fail-closed)
+        deny_filter()
+    end
+  end
+
+  # Helper function to return a filter that never matches (deny all records)
+  # Used when authorization should be denied (fail-closed)
+  #
+  # Using `expr(false)` avoids depending on the primary key being named `:id`.
+  # This is more robust than [id: {:in, []}] which assumes the primary key is `:id`.
+  defp deny_filter do
+    expr(false)
+  end
+
+  # Helper to extract action type from authorizer
+  # CRITICAL: Must use action_type, not action.name!
+  # Action types: :create, :read, :update, :destroy
+  # Action names: :create_member, :update_member, etc.
+  # PermissionSets uses action types, not action names
+  #
+  # Prefer authorizer.action.type (stable API) over authorizer.subject (varies by context)
+  defp get_action_from_authorizer(authorizer) do
+    # Primary: Use authorizer.action.type (stable API)
+    case Map.get(authorizer, :action) do
+      %{type: action_type} when action_type in [:create, :read, :update, :destroy] ->
+        action_type
+
+      _ ->
+        # Fallback: Try authorizer.subject (for compatibility with different Ash versions/contexts)
+        case Map.get(authorizer, :subject) do
+          %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] ->
+            action_type
+
+          %{action: %{type: action_type}}
+          when action_type in [:create, :read, :update, :destroy] ->
+            action_type
+
+          _ ->
+            nil
+        end
+    end
+  end
+
+  # Helper to extract record from authorizer for strict_check
+  defp get_record_from_authorizer(authorizer) do
+    case authorizer.subject do
+      %{data: data} when not is_nil(data) -> data
       _ -> nil
     end
   end
 
-  # Helper to extract action from authorizer
-  defp get_action_from_authorizer(authorizer) do
-    case authorizer.subject do
-      %{action: %{name: action}} -> action
-      %{action: action} when is_atom(action) -> action
-      _ -> nil
+  # Evaluate filter expression for strict_check on single records
+  # For :linked scope with Member resource: id == actor.member_id
+  defp evaluate_filter_for_strict_check(_filter_expr, actor, record, resource_name) do
+    case {resource_name, record} do
+      {"Member", %{id: member_id}} when not is_nil(member_id) ->
+        # Check if this member's ID matches the actor's member_id
+        if member_id == actor.member_id do
+          {:ok, true}
+        else
+          {:ok, false}
+        end
+
+      {"CustomFieldValue", %{member_id: cfv_member_id}} when not is_nil(cfv_member_id) ->
+        # Check if this CFV's member_id matches the actor's member_id
+        if cfv_member_id == actor.member_id do
+          {:ok, true}
+        else
+          {:ok, false}
+        end
+
+      _ ->
+        # For other cases or when record is not available, return :unknown
+        # This will cause Ash to use auto_filter instead
+        {:ok, :unknown}
     end
   end
 
@@ -190,21 +281,24 @@ defmodule Mv.Authorization.Checks.HasPermission do
   end
 
   # Scope: linked - Filter based on user relationship (resource-specific!)
-  # Uses Ash relationships: Member has_one :user, CustomFieldValue belongs_to :member
+  # IMPORTANT: Understand the relationship direction!
+  # - User belongs_to :member (User.member_id → Member.id)
+  # - Member has_one :user (inverse, no FK on Member)
   defp apply_scope(:linked, actor, resource_name) do
     case resource_name do
       "Member" ->
-        # Member has_one :user → filter by user.id == actor.id
-        {:filter, expr(user.id == ^actor.id)}
+        # User.member_id → Member.id (inverse relationship)
+        # Filter: member.id == actor.member_id
+        {:filter, expr(id == ^actor.member_id)}
 
       "CustomFieldValue" ->
-        # CustomFieldValue belongs_to :member → member has_one :user
-        # Traverse: custom_field_value.member.user.id == actor.id
-        {:filter, expr(member.user.id == ^actor.id)}
+        # CustomFieldValue.member_id → Member.id → User.member_id
+        # Filter: custom_field_value.member_id == actor.member_id
+        {:filter, expr(member_id == ^actor.member_id)}
 
       _ ->
-        # Fallback for other resources: try user relationship first, then user_id
-        {:filter, expr(user.id == ^actor.id or user_id == ^actor.id)}
+        # Fallback for other resources
+        {:filter, expr(user_id == ^actor.id)}
     end
   end
 

lib/mv/authorization/checks/no_actor.ex

@@ -0,0 +1,74 @@
+defmodule Mv.Authorization.Checks.NoActor do
+  @moduledoc """
+  Custom Ash Policy Check that allows actions when no actor is present.
+
+  **IMPORTANT:** This check ONLY works in test environment for security reasons.
+  In production/dev, ALL operations without an actor are denied.
+
+  ## Security Note
+
+  This check uses compile-time environment detection to prevent accidental
+  security issues in production. In production, ALL operations (including :create
+  and :read) will be denied if no actor is present.
+
+  For seeds and system operations in production, use an admin actor instead:
+
+      admin_user = get_admin_user()
+      Ash.create!(resource, attrs, actor: admin_user)
+
+  ## Usage in Policies
+
+      policies do
+        # Allow system operations without actor (TEST ENVIRONMENT ONLY)
+        # In test: All operations allowed
+        # In production: ALL operations denied (fail-closed)
+        bypass action_type([:create, :read, :update, :destroy]) do
+          authorize_if NoActor
+        end
+        
+        # Check permissions when actor is present
+        policy action_type([:read, :create, :update, :destroy]) do
+          authorize_if HasPermission
+        end
+      end
+
+  ## Behavior
+
+  - In test environment: Returns `true` when actor is nil (allows all operations)
+  - In production/dev: Returns `false` when actor is nil (denies all operations - fail-closed)
+  - Returns `false` when actor is present (delegates to other policies)
+  """
+
+  use Ash.Policy.SimpleCheck
+
+  # Compile-time check: Only allow no-actor bypass in test environment
+  @allow_no_actor_bypass Mix.env() == :test
+  # Alternative (if you want to control via config):
+  # @allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false)
+
+  @impl true
+  def describe(_opts) do
+    if @allow_no_actor_bypass do
+      "allows actions when no actor is present (test environment only)"
+    else
+      "denies all actions when no actor is present (production/dev - fail-closed)"
+    end
+  end
+
+  @impl true
+  def match?(nil, _context, _opts) do
+    # Actor is nil
+    if @allow_no_actor_bypass do
+      # Test environment: Allow all operations
+      true
+    else
+      # Production/dev: Deny all operations (fail-closed for security)
+      false
+    end
+  end
+
+  def match?(_actor, _context, _opts) do
+    # Actor is present - don't match (let other policies decide)
+    false
+  end
+end

lib/mv/email_sync/changes/sync_member_email_to_user.ex

@@ -41,8 +41,10 @@ defmodule Mv.EmailSync.Changes.SyncMemberEmailToUser do
     Ash.Changeset.around_transaction(changeset, fn cs, callback ->
       result = callback.(cs)
 
+      actor = Map.get(changeset.context, :actor)
+
       with {:ok, member} <- Helpers.extract_record(result),
-           linked_user <- Loader.get_linked_user(member) do
+           linked_user <- Loader.get_linked_user(member, actor) do
         Helpers.sync_email_to_linked_record(result, linked_user, new_email)
       else
         _ -> result

lib/mv/email_sync/changes/sync_user_email_to_member.ex

@@ -33,7 +33,17 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do
     if Map.get(context, :syncing_email, false) do
       changeset
     else
-      sync_email(changeset)
+      # Ensure actor is in changeset context - get it from context if available
+      actor = Map.get(changeset.context, :actor) || Map.get(context, :actor)
+
+      changeset_with_actor =
+        if actor && !Map.has_key?(changeset.context, :actor) do
+          Ash.Changeset.put_context(changeset, :actor, actor)
+        else
+          changeset
+        end
+
+      sync_email(changeset_with_actor)
     end
   end
 
@@ -42,7 +52,7 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do
       result = callback.(cs)
 
       with {:ok, record} <- Helpers.extract_record(result),
-           {:ok, user, member} <- get_user_and_member(record) do
+           {:ok, user, member} <- get_user_and_member(record, cs) do
         # When called from Member-side, we need to update the member in the result
         # When called from User-side, we update the linked member in DB only
         case record do
@@ -61,15 +71,19 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do
   end
 
   # Retrieves user and member - works for both resource types
-  defp get_user_and_member(%Mv.Accounts.User{} = user) do
-    case Loader.get_linked_member(user) do
+  defp get_user_and_member(%Mv.Accounts.User{} = user, changeset) do
+    actor = Map.get(changeset.context, :actor)
+
+    case Loader.get_linked_member(user, actor) do
       nil -> {:error, :no_member}
       member -> {:ok, user, member}
     end
   end
 
-  defp get_user_and_member(%Mv.Membership.Member{} = member) do
-    case Loader.load_linked_user!(member) do
+  defp get_user_and_member(%Mv.Membership.Member{} = member, changeset) do
+    actor = Map.get(changeset.context, :actor)
+
+    case Loader.load_linked_user!(member, actor) do
       {:ok, user} -> {:ok, user, member}
       error -> error
     end

lib/mv/email_sync/loader.ex

@@ -2,15 +2,30 @@ defmodule Mv.EmailSync.Loader do
   @moduledoc """
   Helper functions for loading linked records in email synchronization.
   Centralizes the logic for retrieving related User/Member entities.
+
+  ## Authorization
+
+  This module runs systemically and accepts optional actor parameters.
+  When called from hooks/changes, actor is extracted from changeset context.
+  When called directly, actor should be provided for proper authorization.
+
+  All functions accept an optional `actor` parameter that is passed to Ash operations
+  to ensure proper authorization checks are performed.
   """
+  alias Mv.Helpers
 
   @doc """
   Loads the member linked to a user, returns nil if not linked or on error.
-  """
-  def get_linked_member(%{member_id: nil}), do: nil
 
-  def get_linked_member(%{member_id: id}) do
-    case Ash.get(Mv.Membership.Member, id) do
+  Accepts optional actor for authorization.
+  """
+  def get_linked_member(user, actor \\ nil)
+  def get_linked_member(%{member_id: nil}, _actor), do: nil
+
+  def get_linked_member(%{member_id: id}, actor) do
+    opts = Helpers.ash_actor_opts(actor)
+
+    case Ash.get(Mv.Membership.Member, id, opts) do
       {:ok, member} -> member
       {:error, _} -> nil
     end
@@ -18,9 +33,13 @@ defmodule Mv.EmailSync.Loader do
 
   @doc """
   Loads the user linked to a member, returns nil if not linked or on error.
+
+  Accepts optional actor for authorization.
   """
-  def get_linked_user(member) do
-    case Ash.load(member, :user) do
+  def get_linked_user(member, actor \\ nil) do
+    opts = Helpers.ash_actor_opts(actor)
+
+    case Ash.load(member, :user, opts) do
       {:ok, %{user: user}} -> user
       {:error, _} -> nil
     end
@@ -29,9 +48,13 @@ defmodule Mv.EmailSync.Loader do
   @doc """
   Loads the user linked to a member, returning an error tuple if not linked.
   Useful when a link is required for the operation.
+
+  Accepts optional actor for authorization.
   """
-  def load_linked_user!(member) do
-    case Ash.load(member, :user) do
+  def load_linked_user!(member, actor \\ nil) do
+    opts = Helpers.ash_actor_opts(actor)
+
+    case Ash.load(member, :user, opts) do
       {:ok, %{user: user}} when not is_nil(user) -> {:ok, user}
       {:ok, _} -> {:error, :no_linked_user}
       {:error, _} = error -> error

lib/mv/helpers.ex

@@ -0,0 +1,27 @@
+defmodule Mv.Helpers do
+  @moduledoc """
+  Shared helper functions used across the Mv application.
+
+  Provides utilities that are not specific to a single domain or layer.
+  """
+
+  @doc """
+  Converts an actor to Ash options list for authorization.
+  Returns empty list if actor is nil.
+
+  This helper ensures consistent actor handling across all Ash operations
+  in the application, whether called from LiveViews, changes, validations,
+  or other contexts.
+
+  ## Examples
+
+      opts = ash_actor_opts(actor)
+      Ash.read(query, opts)
+
+      opts = ash_actor_opts(nil)
+      # => []
+  """
+  @spec ash_actor_opts(Mv.Accounts.User.t() | nil) :: keyword()
+  def ash_actor_opts(nil), do: []
+  def ash_actor_opts(actor) when not is_nil(actor), do: [actor: actor]
+end

lib/mv/membership/member/validations/email_not_used_by_other_user.ex

@@ -8,6 +8,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do
   This allows creating members with the same email as unlinked users.
   """
   use Ash.Resource.Validation
+  alias Mv.Helpers
 
   @doc """
   Validates email uniqueness across linked Member-User pairs.
@@ -29,7 +30,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do
   def validate(changeset, _opts, _context) do
     email_changing? = Ash.Changeset.changing_attribute?(changeset, :email)
 
-    linked_user_id = get_linked_user_id(changeset.data)
+    actor = Map.get(changeset.context || %{}, :actor)
+    linked_user_id = get_linked_user_id(changeset.data, actor)
     is_linked? = not is_nil(linked_user_id)
 
     # Only validate if member is already linked AND email is changing
@@ -38,19 +40,21 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do
 
     if should_validate? do
       new_email = Ash.Changeset.get_attribute(changeset, :email)
-      check_email_uniqueness(new_email, linked_user_id)
+      check_email_uniqueness(new_email, linked_user_id, actor)
     else
       :ok
     end
   end
 
-  defp check_email_uniqueness(email, exclude_user_id) do
+  defp check_email_uniqueness(email, exclude_user_id, actor) do
     query =
       Mv.Accounts.User
       |> Ash.Query.filter(email == ^email)
       |> maybe_exclude_id(exclude_user_id)
 
-    case Ash.read(query) do
+    opts = Helpers.ash_actor_opts(actor)
+
+    case Ash.read(query, opts) do
       {:ok, []} ->
         :ok
 
@@ -65,8 +69,10 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do
   defp maybe_exclude_id(query, nil), do: query
   defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id)
 
-  defp get_linked_user_id(member_data) do
-    case Ash.load(member_data, :user) do
+  defp get_linked_user_id(member_data, actor) do
+    opts = Helpers.ash_actor_opts(actor)
+
+    case Ash.load(member_data, :user, opts) do
       {:ok, %{user: %{id: id}}} -> id
       _ -> nil
     end

lib/mv/membership_fees/cycle_generator.ex

@@ -28,6 +28,15 @@ defmodule Mv.MembershipFees.CycleGenerator do
   Uses PostgreSQL advisory locks to prevent race conditions when generating
   cycles for the same member concurrently.
 
+  ## Authorization
+
+  This module runs systemically and accepts optional actor parameters.
+  When called from hooks/changes, actor is extracted from changeset context.
+  When called directly, actor should be provided for proper authorization.
+
+  All functions accept an optional `actor` parameter in the `opts` keyword list
+  that is passed to Ash operations to ensure proper authorization checks are performed.
+
   ## Examples
 
       # Generate cycles for a single member
@@ -77,7 +86,9 @@ defmodule Mv.MembershipFees.CycleGenerator do
   def generate_cycles_for_member(member_or_id, opts \\ [])
 
   def generate_cycles_for_member(member_id, opts) when is_binary(member_id) do
-    case load_member(member_id) do
+    actor = Keyword.get(opts, :actor)
+
+    case load_member(member_id, actor: actor) do
       {:ok, member} -> generate_cycles_for_member(member, opts)
       {:error, reason} -> {:error, reason}
     end
@@ -87,25 +98,27 @@ defmodule Mv.MembershipFees.CycleGenerator do
     today = Keyword.get(opts, :today, Date.utc_today())
     skip_lock? = Keyword.get(opts, :skip_lock?, false)
 
-    do_generate_cycles_with_lock(member, today, skip_lock?)
+    do_generate_cycles_with_lock(member, today, skip_lock?, opts)
   end
 
   # Generate cycles with lock handling
   # Returns {:ok, cycles, notifications} - notifications are never sent here,
   # they should be returned to the caller (e.g., via after_action hook)
-  defp do_generate_cycles_with_lock(member, today, true = _skip_lock?) do
+  defp do_generate_cycles_with_lock(member, today, true = _skip_lock?, opts) do
     # Lock already set by caller (e.g., regenerate_cycles_on_type_change)
     # Just generate cycles without additional locking
-    do_generate_cycles(member, today)
+    actor = Keyword.get(opts, :actor)
+    do_generate_cycles(member, today, actor: actor)
   end
 
-  defp do_generate_cycles_with_lock(member, today, false) do
+  defp do_generate_cycles_with_lock(member, today, false, opts) do
     lock_key = :erlang.phash2(member.id)
+    actor = Keyword.get(opts, :actor)
 
     Repo.transaction(fn ->
       Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key])
 
-      case do_generate_cycles(member, today) do
+      case do_generate_cycles(member, today, actor: actor) do
         {:ok, cycles, notifications} ->
           # Return cycles and notifications - do NOT send notifications here
           # They will be sent by the caller (e.g., via after_action hook)
@@ -222,21 +235,33 @@ defmodule Mv.MembershipFees.CycleGenerator do
 
   # Private functions
 
-  defp load_member(member_id) do
-    Member
-    |> Ash.Query.filter(id == ^member_id)
-    |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles])
-    |> Ash.read_one()
-    |> case do
+  defp load_member(member_id, opts) do
+    actor = Keyword.get(opts, :actor)
+
+    query =
+      Member
+      |> Ash.Query.filter(id == ^member_id)
+      |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles])
+
+    result =
+      if actor do
+        Ash.read_one(query, actor: actor)
+      else
+        Ash.read_one(query)
+      end
+
+    case result do
       {:ok, nil} -> {:error, :member_not_found}
       {:ok, member} -> {:ok, member}
       {:error, reason} -> {:error, reason}
     end
   end
 
-  defp do_generate_cycles(member, today) do
+  defp do_generate_cycles(member, today, opts) do
+    actor = Keyword.get(opts, :actor)
+
     # Reload member with relationships to ensure fresh data
-    case load_member(member.id) do
+    case load_member(member.id, actor: actor) do
       {:ok, member} ->
         cond do
           is_nil(member.membership_fee_type_id) ->
@@ -246,7 +271,7 @@ defmodule Mv.MembershipFees.CycleGenerator do
             {:error, :no_join_date}
 
           true ->
-            generate_missing_cycles(member, today)
+            generate_missing_cycles(member, today, actor: actor)
         end
 
       {:error, reason} ->
@@ -254,7 +279,8 @@ defmodule Mv.MembershipFees.CycleGenerator do
     end
   end
 
-  defp generate_missing_cycles(member, today) do
+  defp generate_missing_cycles(member, today, opts) do
+    actor = Keyword.get(opts, :actor)
     fee_type = member.membership_fee_type
     interval = fee_type.interval
     amount = fee_type.amount
@@ -270,7 +296,7 @@ defmodule Mv.MembershipFees.CycleGenerator do
     # Only generate if start_date <= end_date
     if start_date && Date.compare(start_date, end_date) != :gt do
       cycle_starts = generate_cycle_starts(start_date, end_date, interval)
-      create_cycles(cycle_starts, member.id, fee_type.id, amount)
+      create_cycles(cycle_starts, member.id, fee_type.id, amount, actor: actor)
     else
       {:ok, [], []}
     end
@@ -365,7 +391,8 @@ defmodule Mv.MembershipFees.CycleGenerator do
     end
   end
 
-  defp create_cycles(cycle_starts, member_id, fee_type_id, amount) do
+  defp create_cycles(cycle_starts, member_id, fee_type_id, amount, opts) do
+    actor = Keyword.get(opts, :actor)
     # Always use return_notifications?: true to collect notifications
     # Notifications will be returned to the caller, who is responsible for
     # sending them (e.g., via after_action hook returning {:ok, result, notifications})
@@ -380,7 +407,7 @@ defmodule Mv.MembershipFees.CycleGenerator do
         }
 
         handle_cycle_creation_result(
-          Ash.create(MembershipFeeCycle, attrs, return_notifications?: true),
+          Ash.create(MembershipFeeCycle, attrs, return_notifications?: true, actor: actor),
           cycle_start
         )
       end)