From 4192922fd38007192f8e2aafbf00e249ba10d46a Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 21:03:27 +0100 Subject: [PATCH] feat: implement authorization policies for Member resource --- lib/membership/member.ex | 37 +++++++- lib/mv/authorization/checks/has_permission.ex | 87 +++++++++++++++---- lib/mv/authorization/checks/no_actor.ex | 60 +++++++++++++ mix.exs | 1 + mix.lock | 1 + 5 files changed, 169 insertions(+), 17 deletions(-) create mode 100644 lib/mv/authorization/checks/no_actor.ex diff --git a/lib/membership/member.ex b/lib/membership/member.ex index d2ea07d..0cd7174 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -34,7 +34,8 @@ defmodule Mv.Membership.Member do """ use Ash.Resource, domain: Mv.Membership, - data_layer: AshPostgres.DataLayer + data_layer: AshPostgres.DataLayer, + authorizers: [Ash.Policy.Authorizer] require Ash.Query import Ash.Expr @@ -296,6 +297,40 @@ defmodule Mv.Membership.Member do end end + # Authorization Policies + # Order matters: Most specific policies first, then general permission check + policies do + # SYSTEM OPERATIONS: Allow operations without actor (seeds, tests, system jobs) + # This must come first to allow database seeding and test fixtures + # IMPORTANT: Use bypass so this short-circuits and doesn't require other policies + bypass action_type([:create, :read, :update, :destroy]) do + description "Allow system operations without actor (seeds, tests)" + authorize_if Mv.Authorization.Checks.NoActor + end + + # SPECIAL CASE: Users can always READ their linked member + # This allows users with ANY permission set to read their own linked member + # Check using the inverse relationship: User.member_id → Member.id + bypass action_type(:read) do + description "Users can always read member linked to their account" + authorize_if expr(id == ^actor(:member_id)) + end + + # GENERAL: Check permissions from user's role + # HasPermission handles update permissions correctly: + # - :own_data → can update linked member (scope :linked) + # - :read_only → cannot update any member (no update permission) + # - :normal_user → can update all members (scope :all) + # - :admin → can update all members (scope :all) + policy action_type([:read, :create, :update, :destroy]) do + description "Check permissions from user's role and permission set" + authorize_if Mv.Authorization.Checks.HasPermission + end + + # DEFAULT: Forbid if no policy matched + # Ash implicitly forbids if no policy authorized + end + @doc """ Filters members list based on email match priority. diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 345d6e4..3858bd6 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -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} -> @@ -150,15 +157,60 @@ defmodule Mv.Authorization.Checks.HasPermission do end end - # Helper to extract action from authorizer + # 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 defp get_action_from_authorizer(authorizer) do case authorizer.subject do - %{action: %{name: action}} -> action - %{action: action} when is_atom(action) -> action + %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> + action_type + + # Fallback for older Ash versions or different subject shapes + %{action: %{type: action_type}} when action_type in [:create, :read, :update, :destroy] -> + action_type + + _ -> + nil + 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 + # 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 + # Extract resource name from module (e.g., Mv.Membership.Member -> "Member") defp get_resource_name(resource) when is_atom(resource) do resource |> Module.split() |> List.last() @@ -190,21 +242,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 diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex new file mode 100644 index 0000000..c80ea9b --- /dev/null +++ b/lib/mv/authorization/checks/no_actor.ex @@ -0,0 +1,60 @@ +defmodule Mv.Authorization.Checks.NoActor do + @moduledoc """ + Custom Ash Policy Check that allows actions when no actor is present. + + This is primarily used for: + - Database seeding (priv/repo/seeds.exs) + - Test fixtures that create data without authentication + - Background jobs that operate on behalf of the system + + ## Security Note + + This check should only be used for specific actions where system-level + access is appropriate. It should always be combined with other policy + checks that validate actor-based permissions when an actor IS present. + + ## Usage in Policies + + policies do + # Allow seeding and system operations + policy action_type(:create) 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 + + - Returns `{:ok, true}` when actor is nil (allows action) + - Returns `{:ok, :unknown}` when actor is present (delegates to other policies) + - `auto_filter` returns nil (no filtering needed) + """ + + use Ash.Policy.Check + + @impl true + def describe(_opts) do + "allows actions when no actor is present (for seeds and system operations)" + end + + @impl true + def strict_check(actor, _authorizer, _opts) do + if is_nil(actor) do + # No actor present - allow (for seeds, tests, system operations) + {:ok, true} + else + # Actor present - let other policies decide + {:ok, :unknown} + end + end + + @impl true + def auto_filter(_actor, _authorizer, _opts) do + # No filtering needed - this check only validates presence/absence of actor + nil + end +end diff --git a/mix.exs b/mix.exs index 6aa5f9f..4092b7a 100644 --- a/mix.exs +++ b/mix.exs @@ -76,6 +76,7 @@ defmodule Mv.MixProject do {:mix_audit, "~> 2.1", only: [:dev, :test], runtime: false}, {:sobelow, "~> 0.14", only: [:dev, :test], runtime: false}, {:credo, "~> 1.7", only: [:dev, :test], runtime: false}, + {:picosat_elixir, "~> 0.1", only: [:dev, :test]}, {:ecto_commons, "~> 0.3"}, {:slugify, "~> 1.3"} ] diff --git a/mix.lock b/mix.lock index 1808eba..54b0e51 100644 --- a/mix.lock +++ b/mix.lock @@ -60,6 +60,7 @@ "phoenix_pubsub": {:hex, :phoenix_pubsub, "2.2.0", "ff3a5616e1bed6804de7773b92cbccfc0b0f473faf1f63d7daf1206c7aeaaa6f", [:mix], [], "hexpm", "adc313a5bf7136039f63cfd9668fde73bba0765e0614cba80c06ac9460ff3e96"}, "phoenix_template": {:hex, :phoenix_template, "1.0.4", "e2092c132f3b5e5b2d49c96695342eb36d0ed514c5b252a77048d5969330d639", [:mix], [{:phoenix_html, "~> 2.14.2 or ~> 3.0 or ~> 4.0", [hex: :phoenix_html, repo: "hexpm", optional: true]}], "hexpm", "2c0c81f0e5c6753faf5cca2f229c9709919aba34fab866d3bc05060c9c444206"}, "phoenix_view": {:hex, :phoenix_view, "2.0.4", "b45c9d9cf15b3a1af5fb555c674b525391b6a1fe975f040fb4d913397b31abf4", [:mix], [{:phoenix_html, "~> 2.14.2 or ~> 3.0 or ~> 4.0", [hex: :phoenix_html, repo: "hexpm", optional: true]}, {:phoenix_template, "~> 1.0", [hex: :phoenix_template, repo: "hexpm", optional: false]}], "hexpm", "4e992022ce14f31fe57335db27a28154afcc94e9983266835bb3040243eb620b"}, + "picosat_elixir": {:hex, :picosat_elixir, "0.2.3", "bf326d0f179fbb3b706bb2c15fbc367dacfa2517157d090fdfc32edae004c597", [:make, :mix], [{:elixir_make, "~> 0.6", [hex: :elixir_make, repo: "hexpm", optional: false]}], "hexpm", "f76c9db2dec9d2561ffaa9be35f65403d53e984e8cd99c832383b7ab78c16c66"}, "plug": {:hex, :plug, "1.19.1", "09bac17ae7a001a68ae393658aa23c7e38782be5c5c00c80be82901262c394c0", [:mix], [{:mime, "~> 1.0 or ~> 2.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2 or ~> 2.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4.3 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "560a0017a8f6d5d30146916862aaf9300b7280063651dd7e532b8be168511e62"}, "plug_crypto": {:hex, :plug_crypto, "2.1.1", "19bda8184399cb24afa10be734f84a16ea0a2bc65054e23a62bb10f06bc89491", [:mix], [], "hexpm", "6470bce6ffe41c8bd497612ffde1a7e4af67f36a15eea5f921af71cf3e11247c"}, "postgrex": {:hex, :postgrex, "0.21.1", "2c5cc830ec11e7a0067dd4d623c049b3ef807e9507a424985b8dcf921224cd88", [:mix], [{:db_connection, "~> 2.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "27d8d21c103c3cc68851b533ff99eef353e6a0ff98dc444ea751de43eb48bdac"},